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

[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