[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