[coreboot] bug in geodelx dram setup code in v3?
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Mon Jul 28 09:09:43 CEST 2008
On 28.07.2008 07:11, ron minnich wrote:
> On Fri, Jul 11, 2008 at 3:56 PM, Marc Jones <Marc.Jones at amd.com> 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 <rminnich at gmail.com>
>
With the comments below addressed, the patch is:
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
> 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
--
http://www.hailfinger.org/
More information about the coreboot
mailing list