[LinuxBIOS] [PATCH] geode_v3_cleanup.patch - Re: r387 - in LinuxBIOSv3/arch/x86: . geodelx
Marc Jones
marc.jones at amd.com
Tue Jul 3 01:55:12 CEST 2007
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, 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?
>
>
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
--
Marc Jones
Senior Software Engineer
(970) 226-9684 Office
mailto:Marc.Jones at amd.com
http://www.amd.com/embeddedprocessors
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: geode_v3_cleanup.patch
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070702/0ba6b862/attachment.ksh>
More information about the coreboot
mailing list