> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> 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 <[email protected]>
>
> 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
--
coreboot mailing list: [email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
