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

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

[Marc Jones]

This patch fixes a number of comments and other issues raised by Peter. 
More comments inline.

Peter Stuge wrote:
> 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?

added defines for the MSRs and comments for the values.
> > +/**
> > 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.

Good summary. Added to the function header with slight modifications.
> > msr_t msrGlcpSysRstpll;
>
> This guy could use a better name. At least no caps.

changed to msr_glcp_sys_pll
> > 	msrGlcpSysRstpll.lo &=
> > 	    ~(0xFF << RSTPPL_LOWER_HOLD_COUNT_SHIFT);
> > 	msrGlcpSysRstpll.lo |=
> > 	    (0xDE << RSTPPL_LOWER_HOLD_COUNT_SHIFT);
>
> Why 0xDE ?

Added comment - because AMD recommends setting based on stability testing
> > /*      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.

I didn't change this. There was still some question about hlt() on the 
list.
> > +/**
> > 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?

done (I hope, I'm not doxygen guy)
> > +/**
> > Return the Geode Link clock rate.  Rates in this system are always
> > returned as multkiples of 33 Mhz.
> > */
> >
> > +u32 geode_link_speed(void)
>
> Ditto.

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?

done
> > +/**
> > 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?

yup
> > msrnum = GLCP_DELAY_CONTROLS;
> > msr = rdmsr(msrnum);
> > if (msr.lo & ~(0x7C0)) {
> > return;
> > }
>
> The juju must stay but 0x7c0 seems like a #define?

yup
> > 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?

seemed like it was enough code change that this would get it's own patch 
. I think I can get this done tomorrow
> > +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 in a prior patch
> > +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

Marc