[LinuxBIOS] [patch 06/10] Geode LX: CPU

[Uwe Hermann]

OK, some more comments and questions on the code:

On Thu, May 03, 2007 at 12:15:54PM -0600, Marc Jones wrote:
> +	; Delay Controls based on DIMM loading. UGH!
> +	; # of Devices = Module Width (SPD6) / Device Width(SPD13) * Physical Banks(SPD5)
> +	; Note - We only support module width of 64.

Limitations such as these should probably go into the wiki status table,
and also in some central file in the svn repository (this is not only LX
related, we should document the limitations of all boards in this way, IMO).


> +;								   "FUTURE ROBUSTNESS" PROPOSAL
> +;								   ----------------------------
> +;		DIMM	 Max MBUS					   MC 0x2000001A bits 26:24
> +;DIMMs	devices	 Frequency	 MCP 0x4C00000F Setting		 vvv
> +;-----	-------	 ---------	 ----------------------	 ----------
> +;1		 4		 400MHz		 0x82*100FF 0x56960004		  4
> +;1		 8		 400MHz		 0x82*100AA 0x56960004		  4
> +;1		 16		 400MHz		 0x82*10055 0x56960004		  4
> +;
> +;2		 4,4	 400MHz		 0x82710000 0x56960004		  4
> +;2		 8,8	 400MHz		 0xC27100A5 0x56960004		  4	*** OUT OF PUBLISHED ENVELOPE ***


Just curious -- what is a "published envelope" here?


> +;No VTT termination
> +;-------------------------------------
> +;ADDR/CTL have 22 ohm series R
> +;DQ/DQM/DQS have 33 ohm series R
> +;
> +;		DIMM	 Max MBUS
> +;DIMMs	devices	 Frequency	 MCP 0x4C00000F Setting
> +;-----	-------	 ---------	 ----------------------
> +;1		 4		 400MHz		 0xF2F100FF 0x56960004		  4			The MC changes improve Salsa.

What is Salsa?


> +	msr.hi = msr.lo = 0;
> +
> +	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;

Where do these "magic numbers" come from? Are they listed (or deducible
from) public data sheets? Can we add a human-readable comment as to what
exactly these do?


> +	/*
> +	; Castle performance setting.
> +	; Enable Quack for fewer re-RAS on the MC

Castle? Quack? Are these special LX terms?


> -	case MEMSIZE: 
> -		// who cares. 
> +	case MEMSIZE:
> +		// who cares.
>  		eax = 128 * 1024;
>  		ret = 0;
>  		break;

Is this a good thing? Why hardcode the value?


> Index: LinuxBIOSv2/src/include/cpu/amd/geode_post_code.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ LinuxBIOSv2/src/include/cpu/amd/geode_post_code.h	2007-05-03 11:22:07.000000000 -0600
[...]
> +/* standard AMD post definitions -- might as well use them. */
> +#define POST_Output_Port				(0x080)	/*	port to write post codes to*/
> +
> +#define POST_preSioInit					(0x000)	/* geode.asm*/
> +#define POST_clockInit					(0x001)	/* geode.asm*/
> +#define POST_CPURegInit					(0x002)	/* geode.asm*/

Do we want all-caps #defines everywhere? (see also below)

Are the /*geode.asm*/ references needed or useful? Sounds like legacy
comments, which should probably be updated.


> + * Write to a Virtual Register
> + * AX = Class/Index
> + * CX = data to write
      ^^

> +static inline void vrWrite(uint16_t wClassIndex, uint16_t wData)
> +{
> +	outl(((uint32_t) VR_UNLOCK << 16) | wClassIndex, VRC_INDEX);
> +	outw(wData, VRC_DATA);
> +}

AX, CX? Remainders from earlier asm versions?


> +/* DRAM specifications use the following naming conventions for SPD locations */
> +#define SPD_tRP                             SPD_MIN_ROW_PRECHARGE_TIME
> +#define SPD_tRRD                            SPD_MIN_ROWACTIVE_TO_ROWACTIVE
> +#define SPD_tRCD                            SPD_MIN_RAS_TO_CAS_DELAY
> +#define SPD_tRAS                            SPD_MIN_ACTIVE_TO_PRECHARGE_DELAY
> +#define SPD_BANK_DENSITY                    SPD_DENSITY_OF_EACH_ROW_ON_MODULE
> +#define SPD_ADDRESS_CMD_HOLD                SPD_CMD_SIGNAL_INPUT_HOLD_TIME
> +#define SPD_tRC									41	/* SDRAM Device Minimum Active to Active/Auto Refresh Time (tRC) */
> +#define SPD_tRFC								42	/* SDRAM Device Minimum Auto Refresh to Active/Auto Refresh (tRFC) */

Quick cosmetic poll -- do we want SPD_tRP or SPD_TRP? I know tRP is the
"correct" term, but #defines are usually all-caps. What should we use?


> +/* the following code is from crt0.S.lb */
> +/* This takes the place of the post-CAR funtions that the K8 uses to setup the stack and copy LB low.*/
> +
> +#ifndef CONSOLE_DEBUG_TX_STRING
> +	/* uses:	 esp, ebx, ax, dx */
> +# define __CRT_CONSOLE_TX_STRING(string) \
> +	mov	string, %ebx	; \
> +	CALLSP(crt_console_tx_string)

Is this whole serial stuff needed here? Does it have to be
duplicated? If so, why?


> +	intel_chip_post_macro(0x11)		/* post 11 */

Yet another POST_CODE/post_code() implementation? Can we drop it?


> +str_copying_to_ram:  .string "Copying LinuxBIOS to ram.\r\n"

ram -> RAM, please.


Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070504/6eb8945a/attachment.sig>