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

[Carl-Daniel Hailfinger]

On 15.01.2009 15:22, Myles Watson wrote:
> > -----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?

No. This is a local change that crept in. Thanks for noticing. I'll drop it.
> > -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?

I was going to point to http://kernelnewbies.org/FAQ/DoWhile0 but that
does not apply to empty statements. I blame having coded this at 4am.
Will drop.
> > 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.

I want to remove them altogether because they can't be treated specially
in that array. That reordering is there to make sure that we know if any
other mismatches are present in the DIMM. So in a way the new order
helps debugging with the old code. I'm not feeling strongly about this,
though.
> > };
> >    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.

I'll add the following sentence to the summary:
"Check for dual channel CAS latency compatibility."
> > 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.

I'll probably make it printk_debug.

Thanks for the review.
Any other comments before I repost?

Regards,
Carl-Daniel