Re: [coreboot] bug in geodelx dram setup code in v3?

[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