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

[Peter Stuge]

On Wed, Jun 27, 2007 at 11:07:10PM +0200, [e-mail] 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