Re: [LinuxBIOS] Geode delay control setting cleanup

[Uwe Hermann]

On Thu, Jul 05, 2007 at 02:21:25PM -0600, Marc Jones wrote:
> Last week Peter commented that the delay control setting code should be 
> made clearer. Here is what I came up with. I think that it makes more sense 
> now.

Yep, great stuff! We need more patches of this type!
> Index: LinuxBIOSv3/arch/x86/geodelx/geodelx.c
>
> --- LinuxBIOSv3.orig/arch/x86/geodelx/geodelx.c	2007-07-03 18:20:19.000000000 -0600
> +++ LinuxBIOSv3/arch/x86/geodelx/geodelx.c	2007-07-03 17:40:27.000000000 -0600
> @@ -247,6 +247,54 @@
>    }
>  }
>
> +/**
> Delay Control Settings Table from AMD (MCP 0x4C00000F)
> DIMMs	devices	  Slow (<=333MHz)         Fast (>334MHz)
> 1		4		 0x83*100FF 0x56960004	 0x82*100FF 0x56960004
> 1		8		 0x83*100AA 0x56960004	 0x82*100AA 0x56960004
> 1		16		 0x83*100AA 0x56960004	 0x82*10055 0x56960004

^
                                     Why "*" here?

This table is mostly identical to the struct/table below. If it's really
intended to be the same, I'd rather drop it (no duplication of
information if we can avoid it). If there's something important in
there, rather add a comment to the struct below...
> 2		8		 0x837100A5 0x56960004	 0x82710000 0x56960004
> 2		16		 0x937100A5 0x56960004	 0xC27100A5 0x56960004
> 2		20		 0xB37100A5 0x56960004	 0xB27100A5 0x56960004
> 2		24		 0xB37100A5 0x56960004	 0xB27100A5 0x56960004
> 2		32		 0xB37100A5 0x56960004	 0xB2710000 0x56960004
> =========================================================================
> Bit 55 (disable SDCLK 1,3,5) should be set if there is a single DIMM
> in slot 0, but it should be clear for all 2 DIMM settings and if a
> single DIMM is in slot 1. Bits 54:52 should always be set to '111'.
> Settings for single DIMM and no VTT termination (Like db800 platform)
> 0xF2F100FF 0x56960004
> ADDR/CTL have 22 ohm series R
> DQ/DQM/DQS have 33 ohm series R
>
> +*/
>
> +struct delay_controls {
> u8 dimms;
> u8 devices;
> u32 slow_hi;
> u32 slow_low;
> u32 fast_hi;
> u32 fast_low;
>
> +};
>
> +const struct delay_controls delay_control_table [] = {
> +/* DIMMs	devices	Slow (<=333MHz)				Fast (>334MHz)*/
> +{	1,		4,		0x0837100FF, 0x056960004,	0x0827100FF, 0x056960004},
> +{	1,		8,		0x0837100AA, 0x056960004,	0x0827100AA, 0x056960004},
> +{	1,		16,		0x0837100AA, 0x056960004,	0x082710055, 0x056960004},
> +{	2,		8,		0x0837100A5, 0x056960004,	0x082710000, 0x056960004},
> +{	2,		16,		0x0937100A5, 0x056960004,	0x0C27100A5, 0x056960004},
> +{	2,		20,		0x0B37100A5, 0x056960004,	0x0B27100A5, 0x056960004},
> +{	2,		24,		0x0B37100A5, 0x056960004,	0x0B27100A5, 0x056960004},
> +{	2,		32,		0x0B37100A5, 0x056960004,	0x0B2710000, 0x056960004}
> +};

As the stuct is only used once, you can do something like this, I think:

const struct delay_controls {
    u8 dimms;
    u8 devices;
    u32 slow_hi;
    u32 slow_low;
    u32 fast_hi;
    u32 fast_low;
} delay_control_table [] = {
    /* DIMMs Devs Slow (<=333MHz)            Fast (>334MHz) */
    { 1,     4,   0x0837100FF, 0x056960004,  0x0827100FF, 0x056960004 },
    { 1,     8,   0x0837100AA, 0x056960004,  0x0827100AA, 0x056960004 },
    { 1,     16,  0x0837100AA, 0x056960004,  0x082710055, 0x056960004 },
    { 2,     8,   0x0837100A5, 0x056960004,  0x082710000, 0x056960004 },
    { 2,     16,  0x0937100A5, 0x056960004,  0x0C27100A5, 0x056960004 },
    { 2,     20,  0x0B37100A5, 0x056960004,  0x0B27100A5, 0x056960004 },
    { 2,     24,  0x0B37100A5, 0x056960004,  0x0B27100A5, 0x056960004 },
    { 2,     32,  0x0B37100A5, 0x056960004,  0x0B2710000, 0x056960004 },
};

(note that I reformatted the table (mostly by using spaces for alignment) to
fit in 79 chars/line)

You may have to drop the 'const', not sure (this is untested).
> /**
> set_delay_control. This is Black Magic DRAM timing
> juju(http://www.thefreedictionary.com/juju) Dram delay depends on
>
> @@ -263,12 +311,10 @@
> @param dimm1 DIMM 1 SMBus address
> @param sram_width Data width of the SDRAM
>
> */
>
> void set_delay_control(u8 dimm0, u8 dimm1)
>  {
>    u32 msrnum, glspeed;
> u8 spdbyte0, spdbyte1;
> int numdimms = 0;
> u8 spdbyte0, spdbyte1, dimms, i;

Is this safe? The 'dimms' variable is not set to zero anymore?
> /* save some power, disable clock to second DIMM if it is empty */
> if (spdbyte1 == 0) {
> msr.hi |= 0x000800000;

Not critical, but maybe it makes sense to make this a #define, too?
> }
> spdbyte0 += spdbyte1;
> for(i = 0; i < ARRAY_SIZE(delay_control_table); i++){

^                                                ^
         space                                            space
> if((dimms == delay_control_table[i].dimms) &&

^
                 space
> 		(spdbyte0 <= delay_control_table[i].devices)) {
> if (glspeed < 334) {
> 		msr.hi |= 0x0837100A5;
> 		msr.lo |= 0x056960004;
> 		msr.hi |= delay_control_table[i].slow_hi;
> 		msr.lo |= delay_control_table[i].slow_low;
> } else {
> 		msr.hi |= 0x082710000;
> 		msr.lo |= 0x056960004;
> 		msr.hi |= delay_control_table[i].fast_hi;
> 		msr.lo |= delay_control_table[i].fast_low;

Great patch!

With the above issues fixed:

Acked-by: Uwe Hermann

Uwe.