[LinuxBIOS] r387 - in LinuxBIOSv3/arch/x86: . geodelx

Peter Stuge peter at stuge.se
Thu Jun 28 01:31:53 CEST 2007


On Wed, Jun 27, 2007 at 11:07:10PM +0200, svn at openbios.org wrote:
> +++ LinuxBIOSv3/arch/x86/Makefile	2007-06-27 21:07:10 UTC (rev 387)
..


> +$(obj)/arch/x86/geodelx/stage0.o: $(src)/arch/x86/geodelx/stage0.S
> +	$(Q)mkdir -p $(obj)/arch/x86/geodelx

Uh? The rule depends on a file in a directory created in the rule?


> +	$(Q)printf "  CC      $(subst $(shell pwd)/,,$(@))\n"
> +	$(Q)$(CC) -E $(LINUXBIOSINCLUDE) $< \
> +		-o $(obj)/arch/x86/stage0_asm.s -DBOOTBLK=0x1f00 \
> +		-DRESRVED=0xf0 -DDATE=\"`date +%Y/%m/%d`\"

Maybe even the svn rev date?


> +	/* Setup access to the cache for under 640K. Note MC not setup yet. */
> +	msr.hi = 0x20000000;
> +	msr.lo = 0xfff80;
> +	wrmsr(MSR_GLIU0 + 0x20, msr);
> +
> +	msr.hi = 0x20000000;
> +	msr.lo = 0x80fffe0;
> +	wrmsr(MSR_GLIU0 + 0x21, msr);
> +
> +	msr.hi = 0x20000000;
> +	msr.lo = 0xfff80;
> +	wrmsr(MSR_GLIU1 + 0x20, msr);
> +
> +	msr.hi = 0x20000000;
> +	msr.lo = 0x80fffe0;
> +	wrmsr(MSR_GLIU1 + 0x21, msr);

No nice #defines around for these?



> +/** 
> +  * start_time1 Starts Timer 1 for port 61 use. FIXME try to figure
> +  * out what these values mean.
> +  */
> +void start_timer1(void)
> +{
> +	outb(0x56, 0x43);
> +	outb(0x12, 0x41);
> +}

0x43 is PIT command/control.
0x41 is PIT counter 1.

0x56 means write counter 1 lower 8 bits in next IO, set the counter
mode to square wave generator (count down to 0 from programmed value
twice in a row, alternating the output signal) counting in 16-bit
binary mode.

0x12 is written to the counter.

Used for RAM refresh on XT/AT but the port 61 reference indicates it
has to do with the (emulated?) keyboard controller.



> +	msr_t msrGlcpSysRstpll;

This guy could use a better name. At least no caps.


> +			msrGlcpSysRstpll.lo &=
> +			    ~(0xFF << RSTPPL_LOWER_HOLD_COUNT_SHIFT);
> +			msrGlcpSysRstpll.lo |=
> +			    (0xDE << RSTPPL_LOWER_HOLD_COUNT_SHIFT);

Why 0xDE ?


> +		/*      You should never get here..... The chip has reset. */
> +		printk(BIOS_EMERG,"CONFIGURING PLL FAILURE -- HALT\n");
> +		post_code(POST_PLL_RESET_FAIL);
> +		__asm__ __volatile__("hlt\n");

How about that hlt() function? Remember we want to put panic room
code there when we have too much spare time.


> +/**
> + * Return the CPU clock rate. Rates in this system are always returned
> + * as multkiples of 33 Mhz.
> + *
> + */
> +u32 cpu_speed(void)

Is there a doxygen syntax for the return value? Also please say which
unit the value is in. MHz?


> +/**
> + * Return the Geode Link clock rate.  Rates in this system are always
> + * returned as multkiples of 33 Mhz.
> + *
> + */
> +u32 geode_link_speed(void)

Ditto.


> +	speed = ((((msr.hi >> RSTPLL_UPPER_GLMULT_SHIFT) & 0x1F) + 1) * 333) / 10;
> +	if ((((((msr.hi >> RSTPLL_UPPER_GLMULT_SHIFT) & 0x1F) + 1) * 333) % 10) > 5) {

Maybe that 0x1F is a #define?


> +/**
> + * Return the PCI bus clock rate.  Rates in this system are always
> + * returned as multkiples of 33 Mhz.
> + *
> + */
> +u32 pci_speed(void)

Doxygen return value comment here too?


> +	msrnum = GLCP_DELAY_CONTROLS;
> +	msr = rdmsr(msrnum);
> +	if (msr.lo & ~(0x7C0)) {
> +		return;
> +	}

The juju must stay but 0x7c0 seems like a #define?


> +	if (spdbyte0 == 0 || spdbyte1 == 0) {
> +		/* one dimm solution */
> +		if (spdbyte1 == 0) {
> +			msr.hi |= 0x000800000;
> +		}
> +		spdbyte0 += spdbyte1;
> +		if (spdbyte0 > 8) {
> +			/* large dimm */
> +			if (glspeed < 334) {
> +				msr.hi |= 0x0837100AA;
> +				msr.lo |= 0x056960004;
> +			} else {
> +				msr.hi |= 0x082710055;
> +				msr.lo |= 0x056960004;
> +			}
> +		} else if (spdbyte0 > 4) {
> +			/* medium dimm */
> +			if (glspeed < 334) {
> +				msr.hi |= 0x0837100AA;
> +				msr.lo |= 0x056960004;
> +			} else {
> +				msr.hi |= 0x0827100AA;
> +				msr.lo |= 0x056960004;
> +			}
> +		} else {
> +			/* small dimm */
> +			if (glspeed < 334) {
> +				msr.hi |= 0x0837100FF;
> +				msr.lo |= 0x056960004;
> +			} else {
> +				msr.hi |= 0x0827100FF;
> +				msr.lo |= 0x056960004;
> +			}
> +		}
> +	} else {
> +		/* two dimm solution */
> +		spdbyte0 += spdbyte1;
> +		if (spdbyte0 > 24) {
> +			/* huge dimms */
> +			if (glspeed < 334) {
> +				msr.hi |= 0x0B37100A5;
> +				msr.lo |= 0x056960004;
> +			} else {
> +				msr.hi |= 0x0B2710000;
> +				msr.lo |= 0x056960004;
> +			}
> +		} else if (spdbyte0 > 16) {
> +			/* large dimms */
> +			if (glspeed < 334) {
> +				msr.hi |= 0x0B37100A5;
> +				msr.lo |= 0x056960004;
> +			} else {
> +				msr.hi |= 0x0B27100A5;
> +				msr.lo |= 0x056960004;
> +			}
> +		} else if (spdbyte0 >= 8) {
> +			/* medium dimms */
> +			if (glspeed < 334) {
> +				msr.hi |= 0x0937100A5;
> +				msr.lo |= 0x056960004;
> +			} else {
> +				msr.hi |= 0x0C27100A5;
> +				msr.lo |= 0x056960004;
> +			}
> +		} else {
> +			/* small dimms */
> +			if (glspeed < 334) {
> +				msr.hi |= 0x0837100A5;
> +				msr.lo |= 0x056960004;
> +			} else {
> +				msr.hi |= 0x082710000;
> +				msr.lo |= 0x056960004;
> +			}
> +		}

Each if statements just changes a few bits. Couldn't this be made
more readable?


> +DCacheSetupBad:
> +	hlt		/* Issues */
> +	jmp DCacheSetupBad

Hehe, yes, we have issues. Should we aim for panic room code in
assembly?


> +lout:
> +	/* Restore the BIST result. */
> +	movl	%ebp, %eax
> +	/* We need to set ebp? No need. */
> +	movl	%esp, %ebp
> +	pushl	%eax		/* BIST */
> +	call	stage1_main
> +	/* We will not go back. */

I remember commenting on this before; please handle a return or
change it to a jmp. (I like the latter.)


> +fixed_mtrr_msr:
> +	.long   0x250, 0x258, 0x259
> +	.long   0x268, 0x269, 0x26A
> +	.long   0x26B, 0x26C, 0x26D
> +	.long   0x26E, 0x26F
> +var_mtrr_msr:
> +	.long   0x200, 0x201, 0x202, 0x203
> +	.long   0x204, 0x205, 0x206, 0x207

..or this would be executed on return..


//Peter




More information about the coreboot mailing list