bug in geodelx dram setup code in v3?

[ron minnich]

The code:
/**
 * Delay Control Settings table from AMD (MCP 0x4C00000F).
 */
static 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 },
};

/*
 * 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
 */

static void set_delay_control(u8 dimm0, u8 dimm1)

There's no support here for a non-terminated bus! Just that one comment.

Also:
 * Settings for single DIMM and no VTT termination (like DB800 platform)
 * 0xF2F100FF 0x56960004

Does this mean 1 DIMM, any number of devs? This is not clear.

A change:
static void set_delay_control(u8 dimm0, u8 dimm1, int terminated)

terminated is 0 if there is no termination. Then set 4c00000f accordingly?

ron

[Marc Jones]

ron minnich wrote:
...
> There's no support here for a non-terminated bus! Just that one comment.
>
> Also:
> Settings for single DIMM and no VTT termination (like DB800 platform)
> 0xF2F100FF 0x56960004
>
> Does this mean 1 DIMM, any number of devs? This is not clear.

Yes, see the table in v2. There isn't code in v2 either. I'll work 
something up next week.
> A change:
> static void set_delay_control(u8 dimm0, u8 dimm1, int terminated)
>
> terminated is 0 if there is no termination. Then set 4c00000f accordingly?

Seems reasonable.
Marc

[ron minnich]

On Fri, Jul 11, 2008 at 3:56 PM, Marc Jones  wrote:
> ron minnich wrote:
> ...
> > There's no support here for a non-terminated bus! Just that one comment.
> >
> > Also:
> > Settings for single DIMM and no VTT termination (like DB800 platform)
> > 0xF2F100FF 0x56960004
> >
> > Does this mean 1 DIMM, any number of devs? This is not clear.
>
> Yes, see the table in v2. There isn't code in v2 either. I'll work something
> up next week.
> > A change:
> > static void set_delay_control(u8 dimm0, u8 dimm1, int terminated)
> >
> > terminated is 0 if there is no termination. Then set 4c00000f accordingly?
>
> Seems reasonable.

patch attached.

ron

[Mart Raudsepp]

Hello,

Ühel kenal päeval, P, 2008-07-27 kell 22:11, kirjutas ron minnich:
> Index: mainboard/artecgroup/dbe62/initram.c
>
> --- mainboard/artecgroup/dbe62/initram.c        (revision 698)
> +++ mainboard/artecgroup/dbe62/initram.c        (working copy)
> @@ -33,7 +33,7 @@
>  #include <northbridge/amd/geodelx/raminit.h>
>  #include <spd.h>
>
> -#define MANUALCONF 1           /* Do manual strapped PLL config */
> +#define MANUALCONF 0           /* Do manual strapped PLL config */

It's correct to do automatic PLL config from bootstraps here, but this
is unrelated to the provided log message
> #define PLLMSRHI 0x000003d9    /* manual settings for the PLL */
>  #define PLLMSRLO 0x07de0080    /* from factory bios */
>  #define DIMM0 ((u8) 0xA0)
> @@ -123,6 +123,7 @@
>    */
>  int main(void)
>  {
>   struct msr msr;
>   void dumplxmsrs(void);
>
>   u8 smb_devices[] =  {
>
> @@ -140,7 +141,7 @@
>         pll_reset(MANUALCONF, PLLMSRHI, PLLMSRLO);
>         printk(BIOS_DEBUG, "done pll reset\n");
>   cpu_reg_init(0, DIMM0, DIMM1);
>   cpu_reg_init(0, DIMM0, DIMM1, 0);
>   printk(BIOS_DEBUG, "done cpu reg init\n");
>
>   sdram_set_registers();

So we have it unterminated and it was set up as terminated before or
that wasn't an issue for your memtest quest?
> @@ -152,6 +153,16 @@
>         sdram_enable(DIMM0, DIMM1);
>         printk(BIOS_DEBUG, "done sdram enable\n");
>   /* factory bios sets writethrough on RCONF! */
>   /* This is just a hack put here because no sane mainboard
>    * would ever require writethrough. This is not worth any
>    * visibility in Kconfig or dts or anything for that matter.
>    */
>   msr = rdmsr(CPU_RCONF_DEFAULT);
>   msr.lo |= RCONF_WT(RCONF_DEFAULT_LOWER_SYSRC_SHIFT);
>   wrmsr(CPU_RCONF_DEFAULT, msr);
>   printk(BIOS_DEBUG, "Set write through\n");

What's this? Factory BIOS is GSW? Artec LinuxBIOS-v2 branch should be
the reference here instead in that case and if that had it
write-through, then it might be wrong, would have to check.
Also unrelated to the commit log message as provided
>     dumplxmsrs();
>     /* Check low memory */
>     /* The RAM is working now. Leave this test commented out but

Regards,
Mart Raudsepp
Artec Design LLC

[Paul Menzel]

Dear Ron,

just nitpicking.

Am Sonntag, den 27.07.2008, 22:11 -0700 schrieb ron minnich:
> Index: arch/x86/geodelx/geodelx.c
>
> --- arch/x86/geodelx/geodelx.c  (revision 698)
> +++ arch/x86/geodelx/geodelx.c  (working copy)
> @@ -313,8 +313,9 @@
> @param dimm0 The SMBus address of DIMM 0 (mainboard dependent).
> @param dimm1 The SMBus address of DIMM 1 (mainboard dependent).
> @param terminated The bus is terminated.
> */
>
> -static void set_delay_control(u8 dimm0, u8 dimm1)
> +static void set_delay_control(u8 dimm0, u8 dimm1, int terminated)
>  {
>         u32 glspeed;
>         u8 spdbyte0, spdbyte1, dimms, i;
> @@ -376,7 +377,10 @@
>     spdbyte0 += spdbyte1;
>
>   for (i = 0; i < ARRAY_SIZE(delay_control_table); i++) {
>   if ((dimms == 1) && (! terminated)) {
>           msr.hi = 0xF2F100FF;
>           msr.lo =  0x56960004;

Two spaces after =.
>   } else for (i = 0; i < ARRAY_SIZE(delay_control_table); i++) {
>           if ((dimms == delay_control_table[i].dimms) &&
>               (spdbyte0 <= delay_control_table[i].devices)) {
>                   if (glspeed < 334) {
> Index: mainboard/artecgroup/dbe62/initram.c
>
> --- mainboard/artecgroup/dbe62/initram.c        (revision 698)
> +++ mainboard/artecgroup/dbe62/initram.c        (working copy)
> @@ -152,6 +153,16 @@
>         sdram_enable(DIMM0, DIMM1);
>         printk(BIOS_DEBUG, "done sdram enable\n");
>   /* factory bios sets writethrough on RCONF! */
>   /* This is just a hack put here because no sane mainboard
>    * would ever require writethrough. This is not worth any
>    * visibility in Kconfig or dts or anything for that matter.

anything *else* (?), but I am no native speaker.
>    */
>   msr = rdmsr(CPU_RCONF_DEFAULT);
>   msr.lo |= RCONF_WT(RCONF_DEFAULT_LOWER_SYSRC_SHIFT);
>   wrmsr(CPU_RCONF_DEFAULT, msr);
>   printk(BIOS_DEBUG, "Set write through\n");

You should settle for one way of writing write-through (see Mart),
writethrough or write through.

Thanks,

Paul

[Carl-Daniel Hailfinger]

On 28.07.2008 07:11, ron minnich wrote:
> On Fri, Jul 11, 2008 at 3:56 PM, Marc Jones  wrote:
> > ron minnich wrote:
> > ...
> > > There's no support here for a non-terminated bus! Just that one comment.
> > >
> > > Also:
> > > Settings for single DIMM and no VTT termination (like DB800 platform)
> > > 0xF2F100FF 0x56960004
> > >
> > > Does this mean 1 DIMM, any number of devs? This is not clear.
> >
> > Yes, see the table in v2. There isn't code in v2 either. I'll work something
> > up next week.
> > > A change:
> > > static void set_delay_control(u8 dimm0, u8 dimm1, int terminated)
> > >
> > > terminated is 0 if there is no termination. Then set 4c00000f accordingly?
> >
> > Seems reasonable.
>
> patch attached.
>
> Not all mainboards terminate dram. This change adds a 'terminated' parameter
> to cpu_reg_init so that the non-terminated case can be handled properly.

Please mention the conceptually separate dbe62 changes as well or move
them to another commit.
> Signed-off-by: Ronald G. Minnich

With the comments below addressed, the patch is:
Acked-by: Carl-Daniel Hailfinger
> Index: arch/x86/geodelx/geodelx.c
>
> --- arch/x86/geodelx/geodelx.c	(revision 698)
> +++ arch/x86/geodelx/geodelx.c	(working copy)
> @@ -313,8 +313,9 @@
> @param dimm0 The SMBus address of DIMM 0 (mainboard dependent).
> @param dimm1 The SMBus address of DIMM 1 (mainboard dependent).
> @param terminated The bus is terminated.

Please add "(mainboard dependent)" to the new parameter as well.
There's one superfluous space at the end of the line.
> */
> -static void set_delay_control(u8 dimm0, u8 dimm1)
> +static void set_delay_control(u8 dimm0, u8 dimm1, int terminated)
>  {
>    u32 glspeed;
>    u8 spdbyte0, spdbyte1, dimms, i;
> @@ -376,7 +377,10 @@
>
> spdbyte0 += spdbyte1;
> for (i = 0; i < ARRAY_SIZE(delay_control_table); i++) {
> if ((dimms == 1) && (! terminated)) {
> msr.hi = 0xF2F100FF;
> msr.lo =  0x56960004;

Superfluous whitespace after =.
> } else for (i = 0; i < ARRAY_SIZE(delay_control_table); i++) {
> if ((dimms == delay_control_table[i].dimms) &&
>    (spdbyte0 <= delay_control_table[i].devices)) {
> 	if (glspeed < 334) {
>
> @@ -400,8 +404,9 @@
>                        setting in future.
> @param dimm0 SMBus address of DIMM 0 (mainboard dependent).
> @param dimm1 SMBus address of DIMM 1 (mainboard dependent).
> @param terminated The bus is terminated. Mainboard-dependent.

Please make the "mainboard-dependent" remark formatting consistent with
the two lines above. Also, there's a superfluous space at the end of the
line.
> */
> -void cpu_reg_init(int debug_clock_disable, u8 dimm0, u8 dimm1)
> +void cpu_reg_init(int debug_clock_disable, u8 dimm0, u8 dimm1, int terminated)
>  {
>    struct msr msr;
>
> @@ -432,7 +437,7 @@
>    wrmsr(GLIU1_PORT_ACTIVE, msr);
>
> /* Set the Delay Control in GLCP. */
> set_delay_control(dimm0, dimm1);
>
> set_delay_control(dimm0, dimm1, terminated);
>
> /* Enable RSDC. */
> msr = rdmsr(CPU_AC_SMM_CTL);
> Index: include/arch/x86/amd_geodelx.h
>
> --- include/arch/x86/amd_geodelx.h	(revision 698)
> +++ include/arch/x86/amd_geodelx.h	(working copy)
> @@ -1281,7 +1281,7 @@
>  u32 geode_link_speed(void);
>  void geodelx_msr_init(void);
>  void pll_reset(int manualconf, u32 pll_hi, u32 pll_lo);
> -void cpu_reg_init(int debug_clock_disable, u8 dimm0, u8 dimm1);
> +void cpu_reg_init(int debug_clock_disable, u8 dimm0, u8 dimm1, int terminated);
>  void system_preinit(void);
>  void msr_init(void);
>  void geode_pre_payload(void);
> Index: mainboard/adl/msm800sev/initram.c
> ===================================================================
> --- mainboard/adl/msm800sev/initram.c	(revision 698)
> +++ mainboard/adl/msm800sev/initram.c	(working copy)
> @@ -50,7 +50,7 @@
>
> pll_reset(MANUALCONF, PLLMSRHI, PLLMSRLO);
> cpu_reg_init(0, DIMM0, DIMM1);
> cpu_reg_init(0, DIMM0, DIMM1, 1);
> sdram_set_registers();
> sdram_set_spd_registers(DIMM0, DIMM1);
> sdram_enable(DIMM0, DIMM1);
> Index: mainboard/amd/db800/initram.c
>
> --- mainboard/amd/db800/initram.c	(revision 698)
> +++ mainboard/amd/db800/initram.c	(working copy)
> @@ -86,7 +86,7 @@
>    pll_reset(MANUALCONF, PLLMSRHI, PLLMSRLO);
>    printk(BIOS_DEBUG, "done pll reset\n");
> cpu_reg_init(0, DIMM0, DIMM1);
>
> cpu_reg_init(0, DIMM0, DIMM1, 0);
> printk(BIOS_DEBUG, "done cpu reg init\n");
>
> sdram_set_registers();
> Index: mainboard/amd/norwich/initram.c
>
> --- mainboard/amd/norwich/initram.c	(revision 698)
> +++ mainboard/amd/norwich/initram.c	(working copy)
> @@ -86,7 +86,7 @@
>    pll_reset(MANUALCONF, PLLMSRHI, PLLMSRLO);
>    printk(BIOS_DEBUG, "done pll reset\n");
> cpu_reg_init(0, DIMM0, DIMM1);
>
> cpu_reg_init(0, DIMM0, DIMM1, 1);
> printk(BIOS_DEBUG, "done cpu reg init\n");
>
> sdram_set_registers();
> Index: mainboard/artecgroup/dbe61/initram.c
>
> --- mainboard/artecgroup/dbe61/initram.c	(revision 698)
> +++ mainboard/artecgroup/dbe61/initram.c	(working copy)
> @@ -149,7 +149,7 @@
>    pll_reset(MANUALCONF, PLLMSRHI, PLLMSRLO);
>    printk(BIOS_DEBUG, "done pll reset\n");
> cpu_reg_init(0, DIMM0, DIMM1);
>
> cpu_reg_init(0, DIMM0, DIMM1, 0);
> printk(BIOS_DEBUG, "done cpu reg init\n");
>
> sdram_set_registers();
> Index: mainboard/artecgroup/dbe62/initram.c
>
> --- mainboard/artecgroup/dbe62/initram.c	(revision 698)
> +++ mainboard/artecgroup/dbe62/initram.c	(working copy)
> @@ -33,7 +33,7 @@
>  #include <northbridge/amd/geodelx/raminit.h>
>  #include <spd.h>
>
> -#define MANUALCONF 1		/* Do manual strapped PLL config */
> +#define MANUALCONF 0		/* Do manual strapped PLL config */
>  #define PLLMSRHI 0x000003d9	/* manual settings for the PLL */
>  #define PLLMSRLO 0x07de0080	/* from factory bios */
>  #define DIMM0 ((u8) 0xA0)
> @@ -123,6 +123,7 @@
>    */
>  int main(void)
>  {
>
> struct msr msr;
> void dumplxmsrs(void);
>
> u8 smb_devices[] =  {
>
> @@ -140,7 +141,7 @@
>    pll_reset(MANUALCONF, PLLMSRHI, PLLMSRLO);
>    printk(BIOS_DEBUG, "done pll reset\n");
> cpu_reg_init(0, DIMM0, DIMM1);
>
> cpu_reg_init(0, DIMM0, DIMM1, 0);
> printk(BIOS_DEBUG, "done cpu reg init\n");
>
> sdram_set_registers();
>
> @@ -152,6 +153,16 @@
>    sdram_enable(DIMM0, DIMM1);
>    printk(BIOS_DEBUG, "done sdram enable\n");
> /* factory bios sets writethrough on RCONF! */
> /* This is just a hack put here because no sane mainboard
> * would ever require writethrough. This is not worth any
> * visibility in Kconfig or dts or anything for that matter.
> */
> msr = rdmsr(CPU_RCONF_DEFAULT);
> msr.lo |= RCONF_WT(RCONF_DEFAULT_LOWER_SYSRC_SHIFT);
> wrmsr(CPU_RCONF_DEFAULT, msr);
> printk(BIOS_DEBUG, "Set write through\n");
> dumplxmsrs();
>  /* Check low memory */
>  /* The RAM is working now. Leave this test commented out but
> Index: mainboard/pcengines/alix1c/initram.c
>
> --- mainboard/pcengines/alix1c/initram.c	(revision 698)
> +++ mainboard/pcengines/alix1c/initram.c	(working copy)
> @@ -148,7 +148,7 @@
>    pll_reset(MANUALCONF, PLLMSRHI, PLLMSRLO);
>    printk(BIOS_DEBUG, "done pll reset\n");
> cpu_reg_init(0, DIMM0, DIMM1);
>
> cpu_reg_init(0, DIMM0, DIMM1, 1);
> printk(BIOS_DEBUG, "done cpu reg init\n");
>
> sdram_set_registers();
> Index: mainboard/pcengines/alix2c3/initram.c
>
> --- mainboard/pcengines/alix2c3/initram.c	(revision 698)
> +++ mainboard/pcengines/alix2c3/initram.c	(working copy)
> @@ -145,7 +145,7 @@
>    pll_reset(MANUALCONF, PLLMSRHI, PLLMSRLO);
>    printk(BIOS_DEBUG, "done pll reset\n");
> cpu_reg_init(0, DIMM0, DIMM1);
>
> cpu_reg_init(0, DIMM0, DIMM1, 1);
> printk(BIOS_DEBUG, "done cpu reg init\n");
>
> sdram_set_registers();

Regards,
Carl-Daniel

[ron minnich]

Thanks for the comments. Mart, the factory BIOS I have sets
writethrough. If it should no I will remove this setting. It did not
help anyway. I will correct this patch and resubmit.

ron

[ron minnich]

committed, with all commits taken into account,
Committed revision 699.

thanks

ron