Re: [coreboot] [PATCH][again] 440BX raminit cleanup

[Keith Hui]

On Tue, May 18, 2010 at 8:41 PM,   wrote:
> Send coreboot mailing list submissions to
>        [e-mail]
>
> To subscribe or unsubscribe via the World Wide Web, visit
>        http://www.coreboot.org/mailman/listinfo/coreboot
> or, via email, send a message with subject or body 'help' to
>        [e-mail]
>
> You can reach the person managing the list at
>        [e-mail]
>
> When replying, please edit your Subject line so it is more specific
> than "Re: Contents of coreboot digest..."
>
> Today's Topics:
>
> 1. Re: [PATCH][again] 440BX raminit cleanup (Stefan Reinauer)
>   2. Re: H8QME 128GB RAM not Booting (Myles Watson)
>   3. [commit] r5569 - in trunk/src/superio/winbond: w83627ehg
>      w83627hf (repository service)
>   4. Advice how to install Linux to get it working with FILO
>      (Joop Boonen)
>   5. Re: FILO bug disk not seen at ata-0 (Doesn't try to detect on
>      ATA only SIL3114) (Kevin O'Connor)
>   6. ask for support for some hardware (Benediktus Anindito)
>
> Message: 1
> Date: Tue, 18 May 2010 18:02:26 +0200
> From: Stefan Reinauer 
> To: [e-mail]
> Subject: Re: [coreboot] [PATCH][again] 440BX raminit cleanup
> Message-ID: 
> Content-Type: text/plain; charset=windows-1252
> > A number of cleanups for 440BX raminit code.
> >
> > Resolves a number of TODOs items within, and clarified a number of other TODOs.
> > Change register_values[] from long to u8 (byte). For what we are doing
> > this is sufficient and makes it only 1/4 the size.
> > Remove a hard-coding of SDRAMC register that is redundant and now
> > incorrect, now that SDRAMC is conditioned on SDRAMPWR_4DIMM Kconfig
> > and set through register_values[].
> > RPS registers are now set in runtime code; remove it from
> > register_values[] table.
> > Bring DUMPNORTH() back. The code it refers to is still there.
> > Move #define of NB up so the DUMPNORTH() macro can use it.
> >
> > Signed-off-by: Keith Hui
>
> -// no dump_pci_device in src/northbridge/intel/i440bx
> -// #define DUMPNORTH() dump_pci_device(PCI_DEV(0, 0, 0))
> -#define DUMPNORTH()
> +#define DUMPNORTH() dump_pci_device(NB)
>
> Does this still compile with CONFIG_DEBUG_RAM_SETUP set?

Yes it does. And it runs. Idwer posted a boot log here with this patch applied.
> Bits referencing empty rows are ?don?t care?.
>
> that looks odd...

That is straight from the 440BX datasheet.
> /* TODO: Set SDRAMC. */
> pci_write_config16(NB, SDRAMC, 0x0010); /* SDRAMPWR=1: 4 DIMM config */
>
> This part is deleted without a replacement. Is it not needed? Is this
> set somewhere else?

Set somewhere else. It is now an entry in the register_values[] array.
Value is set conditionally at compile time by CONFIG_SDRAMPWR_4DIMM.

Thanks
Keith

[PATCH][again] 440BX raminit cleanup

[Uwe Hermann]

On Tue, May 18, 2010 at 09:37:49PM -0400, Keith Hui wrote:
> > > A number of cleanups for 440BX raminit code.

[...]
> > > Signed-off-by: Keith Hui

Thanks, r5820 with some minor changes.
> > Bits referencing empty rows are ?don?t care?.
> >
> > that looks odd...
>
> That is straight from the 440BX datasheet.

I think Stefan meant the funny characters (displayed as "?" here in my
mailer) not the "don't care" wording itself. I fixed that in the commit.
> > /* TODO: Set SDRAMC. */
> > pci_write_config16(NB, SDRAMC, 0x0010); /* SDRAMPWR=1: 4 DIMM config */
> >
> > This part is deleted without a replacement. Is it not needed? Is this
> > set somewhere else?
>
> Set somewhere else. It is now an entry in the register_values[] array.
> Value is set conditionally at compile time by CONFIG_SDRAMPWR_4DIMM.

Yep, confirmed. Without this hunk all 3-DIMM-slot boards would not boot,
confirmed on at least the ASUS P2B and A-Trend ATC-6220.

Uwe.