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

[Myles Watson]

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> 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
> 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