[coreboot] [PATCH] Convert print_* to printk_* in K8 RAM init

Myles Watson mylesgw at gmail.com
Thu Jan 15 15:22:47 CET 2009



> -----Original Message-----
> From: coreboot-bounces at coreboot.org [mailto:coreboot-bounces at coreboot.org]
> On Behalf Of Carl-Daniel Hailfinger
> Sent: Wednesday, January 14, 2009 11:32 PM
> To: Coreboot
> Subject: [coreboot] [PATCH] Convert print_* to printk_* in K8 RAM init
> 
> Since all K8 targets now have CONFIG_USE_PRINTK_IN_CAR enabled, using
> print_* in K8 RAM init does not make sense anymore. Convert almost all
> print_* to printk_*.
> This improves readability a lot and makes the code shorter.
> 
> Add a warning about false negatives in the DIMM dual channel
> compatibility check. This needs to be implemented correctly.
> 
> Fix a few typos.
> 
> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
> 
> Index: LinuxBIOSv2-asus_m2a-vm/src/northbridge/amd/amdk8/raminit_f.c
> ===================================================================
> --- LinuxBIOSv2-asus_m2a-vm/src/northbridge/amd/amdk8/raminit_f.c
> 	(Revision 3850)
> +++ LinuxBIOSv2-asus_m2a-vm/src/northbridge/amd/amdk8/raminit_f.c
> 	(Arbeitskopie)
> @@ -34,34 +34,20 @@
>  #define QRANK_DIMM_SUPPORT 0
>  #endif
> 
> -static inline void print_raminit(const char *strval, uint32_t val)
> -{
>  #if CONFIG_USE_PRINTK_IN_CAR
> -	printk_debug("%s%08x\r\n", strval, val);
>  #else
> -	print_debug(strval); print_debug_hex32(val); print_debug("\r\n");
> +#error This file needs CONFIG_USE_PRINTK_IN_CAR
>  #endif
> -}
> 
> -#define RAM_TIMING_DEBUG 0
> +#define RAM_TIMING_DEBUG 1

Do we really want to always debug RAM timing?

> -static inline void print_tx(const char *strval, uint32_t val)
> -{
>  #if RAM_TIMING_DEBUG == 1
> -	print_raminit(strval, val);
> +#define printk_raminit printk_debug
> +#else
> +#define printk_raminit(fmt, arg...)   do {} while(0)

Why is the do{} while (0) needed here?

>  		4,	/* *Column addresses */
>  		5,	/* *Number of DIMM Ranks */
>  		6,	/* *Module Data Width*/
> -		9,	/* *Cycle time at highest CAS Latency CL=X */
>  		11,	/* *DIMM Conf Type */
>  		13,	/* *Pri SDRAM Width */
>  		17,	/* *Logical Banks */
> -		18,	/* *Supported CAS Latencies */
>  		20,	/* *DIMM Type Info */
>  		21,	/* *SDRAM Module Attributes */
> -		23,	/* *Cycle time at CAS Latnecy (CLX - 1) */
> -		26,	/* *Cycle time at CAS Latnecy (CLX - 2) */
>  		27,	/* *tRP Row precharge time */
>  		28,	/* *Minimum Row Active to Row Active Delay (tRRD) */
>  		29,	/* *tRCD RAS to CAS */
> @@ -1464,11 +1440,18 @@
>  		36,	/* *Write recovery time (tWR) */
>  		37,	/* *Internal write to read command delay (tRDP) */
>  		38,	/* *Internal read to precharge commanfd delay (tRTP)
*/
> +#warning Why is SPD address 41 checked twice?
>  		41,	/* *Extension of Byte 41 tRC and Byte 42 tRFC */
>  		41,	/* *Minimum Active to Active/Auto Refresh Time(Trc)
*/
>  		42,	/* *Minimum Auto Refresh Command Time(Trfc) */
> +#warning The SPD addresses below need special treatment like in
> spd_set_memclk. Right now they cause many false negatives.
> +		18,	/* *Supported CAS Latencies */
> +		9,	/* *Cycle time at highest CAS Latency CL=X */
> +		23,	/* *Cycle time at CAS Latency (CLX - 1) */
> +		26,	/* *Cycle time at CAS Latency (CLX - 2) */

I guess I'd rather leave them in order and add their numbers to the warning
message.

>  	};
>  	u32 dcl, dcm;
> +	u8 common_cl;
> 
>  /* S1G1 and AM2 sockets are Mod64BitMux capable. */
>  #if CPU_SOCKET_TYPE == 0x11 || CPU_SOCKET_TYPE == 0x12
> @@ -1497,6 +1480,14 @@
>  		}
>  		device0 = ctrl->channel0[i];
>  		device1 = ctrl->channel1[i];
> +		/* Abort if the chips don't support a common CAS latency. */
> +		common_cl = spd_read_byte(device0, 18) &
> spd_read_byte(device1, 18);
> +		if (!common_cl) {
> +			printk_debug("No common CAS latency supported\n");
> +			goto single_channel;
> +		} else {
> +			printk_raminit("Common CAS latency bitfield:
0x%02x\n",
> common_cl);
> +		}

I didn't see this part in the summary of the patch.


>  	if (!param->cycle_time) {
>  		die("min_cycle_time to low");
>  	}
> -	print_spew(param->name);
>  #ifdef DRAM_MIN_CYCLE_TIME
> -	print_debug(param->name);
> +	printk_debug(param->name);

It seems like you could get rid of this ifdef and just make it printk_debug
or printk_raminit.

Thanks,
Myles





More information about the coreboot mailing list