Re: [coreboot] [PATCH] 440BX registered SDRAM support

[Keith Hui]

> > if ((edosd & 0x84) == 0x84) {
> > edosd = 0x10; // Registered SDRAM
> > } else {
> > // Clear [4:3] in case it's EDO.
> > edosd &= 0x07;
> >
> > +//    } else if (edosd & 0x02) {
>
> Besides being commented out, this piece of code would never be executed,
> as there already is an else case.
> Also, modifying edosd in place is semi nice.

So is this good, not so good, or bad?

I want to know if I should split up edosd.
> Please clean this up before committing. Maybe consider using switch/case?
>
> Looks good otherwise.
> > +//        edosd |= 0x00;
> > if (edosd & 0x04) {
> > edosd |= 0x08; // SDRAM
> > }
> >
> > }
> > // Keep only [4:3].
> >
> > edosd &= 0x18;

Thanks
Keith

[PATCH] 440BX registered SDRAM support

[Stefan Reinauer]

* Keith Hui  [110329 06:11]:
> > > if ((edosd & 0x84) == 0x84) {
> > > edosd = 0x10; // Registered SDRAM
> > > } else {
> > > // Clear [4:3] in case it's EDO.
> > > edosd &= 0x07;
> > >
> > > +//    } else if (edosd & 0x02) {
> >
> > Besides being commented out, this piece of code would never be executed,
> > as there already is an else case.
> > Also, modifying edosd in place is semi nice.
>
> So is this good, not so good, or bad?
>
> I want to know if I should split up edosd.

Please do.

[PATCH] 440BX registered SDRAM support

[Keith Hui]

On Tue, Mar 29, 2011 at 5:34 PM, Stefan Reinauer
 wrote:
> Keith Hui  [110329 06:11]:
> > > > if ((edosd & 0x84) == 0x84) {
> > > > edosd = 0x10; // Registered SDRAM
> > > > } else {
> > > > // Clear [4:3] in case it's EDO.
> > > > edosd &= 0x07;
> > > >
> > > > +//    } else if (edosd & 0x02) {
> > >
> > > Besides being commented out, this piece of code would never be executed,
> > > as there already is an else case.
> > > Also, modifying edosd in place is semi nice.
> >
> > So is this good, not so good, or bad?
> >
> > I want to know if I should split up edosd.
>
> Please do.

And so I did. Signoff in the patch.

edosd was a romcc-inspired trick because variables were a scarce resource.

The nbxecc simplification in this patch completed one full pass of
memtest86+ each with a regular and registered ECC DIMM installed.

With this patch the 440BX romstage is 60 bytes smaller, freeing up an
extra 64 bytes in the image.

Thanks
Keith

[PATCH] 440BX registered SDRAM support

[Keith Hui]

ping?

On Wed, Mar 30, 2011 at 11:38 PM, Keith Hui  wrote:
> On Tue, Mar 29, 2011 at 5:34 PM, Stefan Reinauer
>  wrote:
> > Keith Hui  [110329 06:11]:
> > > > > if ((edosd & 0x84) == 0x84) {
> > > > > edosd = 0x10; // Registered SDRAM
> > > > > } else {
> > > > > // Clear [4:3] in case it's EDO.
> > > > > edosd &= 0x07;
> > > > >
> > > > > +//    } else if (edosd & 0x02) {
> > > >
> > > > Besides being commented out, this piece of code would never be executed,
> > > > as there already is an else case.
> > > > Also, modifying edosd in place is semi nice.
> > >
> > > So is this good, not so good, or bad?
> > >
> > > I want to know if I should split up edosd.
> >
> > Please do.
>
> And so I did. Signoff in the patch.
>
> edosd was a romcc-inspired trick because variables were a scarce resource.
>
> The nbxecc simplification in this patch completed one full pass of
> memtest86+ each with a regular and registered ECC DIMM installed.
>
> With this patch the 440BX romstage is 60 bytes smaller, freeing up an
> extra 64 bytes in the image.
>
> Thanks
> Keith

[PATCH] 440BX registered SDRAM support

[Idwer Vollering]

2011/4/1 Keith Hui :
> ping?

Adds support for initializing registered SDRAM modules on Intel 440BX
> northbridge.
>
> Drops unneeded romcc-inspired programming tricks.
>
> Only set nbxecc flags (see 440BX datasheet, page 3-16) when a non-ECC
> module has been detected
> in a row via SPD; also drops an unneeded intermediate variable used in
> setting them.
>
> Boot tested on ASUS P2B-LS with regular and registered ECC SDRAM under
> Linux and memtest86+.
>
> Signed-off-by: Keith Hui
> Index: src/northbridge/intel/i440bx/raminit.c
>
> --- src/northbridge/intel/i440bx/raminit.c (revision 6460)
> +++ src/northbridge/intel/i440bx/raminit.c (working copy)
> @@ -721,19 +721,23 @@
>  */
> static void set_dram_row_attributes(void)
> {
> int i, dra, drb, col, width, value, rps, edosd, ecc, nbxecc;
> int i, dra, drb, col, width, value, rps;
>
> u8 bpr; /* Top 8 bits of PGPOL */
> u8 nbxecc = 0; /* NBXCFG[31:24] */
> u8 edo, sd, regsd; /* EDO, SDRAM, registered SDRAM */
> edosd = 0;
> edo = 0;
> sd = 0;
> regsd = 1;
>
> rps = 0;
> drb = 0;
> bpr = 0;
> nbxecc = 0xff;
>
> for (i = 0; i < DIMM_SOCKETS; i++) {
> unsigned int device;
> device = DIMM0 + i;
> bpr >>= 2;
> nbxecc >>= 2;
>
> /* First check if a DIMM is actually present. */
> value = spd_read_byte(device, SPD_MEMORY_TYPE);
> @@ -742,13 +746,13 @@
> || value == SPD_MEMORY_TYPE_SDRAM) {
>
> if (value == SPD_MEMORY_TYPE_EDO) {
> edosd |= 0x02;
> edo = 1;
>
> } else if (value == SPD_MEMORY_TYPE_SDRAM) {

Can you add a #define for SPD_MEMORY_TYPE_REGISTERED_SDRAM to
src/include/spd.h as well ? If that is relevant to do, ofcourse.
> edosd |= 0x04;
> sd = 1;
>
> }
> PRINT_DEBUG("Found DIMM in slot %d\n", i);
> if (edosd == 0x06) {
> if (edo && sd) {
>
> print_err("Mixing EDO/SDRAM unsupported!\n");
> die("HALT\n");
> }
> @@ -764,24 +768,38 @@
> TODO: Other register than NBXCFG also needs this
> ECC information.
>
> */
> ecc = spd_read_byte(device, SPD_DIMM_CONFIG_TYPE);
> value = spd_read_byte(device, SPD_DIMM_CONFIG_TYPE);
>
> /* Data width */
> width = spd_read_byte(device, SPD_MODULE_DATA_WIDTH_LSB);
>
> /* Exclude error checking data width from page size calculations */
> if (ecc) {
> if (value) {
>
> value = spd_read_byte(device,
> SPD_ERROR_CHECKING_SDRAM_WIDTH);
> width -= value;
> /* ### ECC */
> /* Clear top 2 bits to help set up NBXCFG. */
> ecc &= 0x3f;
> nbxecc &= 0x3f;
>
> } else {
> /* Without ECC, top 2 bits should be 11. */
> ecc |= 0xc0;
> nbxecc |= 0xc0;
>
> }
> /* If any installed DIMM is *not* registered, this system cannot be
> configured for registered SDRAM.
> By registered, only the address and control lines need to be, which
> we can tell by reading SPD byte 21, bit 1.
> */
> value = spd_read_byte(device, SPD_MODULE_ATTRIBUTES);
> PRINT_DEBUG("DIMM is ");
> if ((value & 0x02) == 0) {
> regsd = 0;
> PRINT_DEBUG("not ");
> }
> PRINT_DEBUG("registered\n");
>
> /* Calculate page size in bits. */
> value = ((1 << col) * width);
>
> @@ -801,7 +819,6 @@
> Second bank of 1-bank DIMMs "doesn't have
> ECC" - or anything.
>
> */
> ecc |= 0x80;
>
> if (dra == 2) {
> dra = 0x0; /* 2KB */
> } else if (dra == 4) {
> @@ -878,7 +895,6 @@
>
> /* If there's no DIMM in the slot, set dra to 0x00. */
> dra = 0x00;
> ecc = 0xc0;
>
> /* Still have to propagate DRB over. */
> drb &= 0xff;
> drb |= (drb << 8);
> @@ -895,7 +911,6 @@
> drb >>= 8;
>
> rps |= (dra & 0x0f) << (i * 4);
> nbxecc = (nbxecc >> 2) | (ecc & 0xc0);
>
> }
>
> /* Set paging policy register. */
> @@ -910,20 +925,19 @@
> pci_write_config8(NB, NBXCFG + 3, nbxecc);
> PRINT_DEBUG("NBXECC[31:24] has been set to 0x%02x\n", nbxecc);
> /* Set DRAMC[4:3] to proper memory type (EDO/SDRAM).
> TODO: Registered SDRAM support.
> */
> edosd &= 0x07;
> if (edosd & 0x02) {
> edosd |= 0x00;
> } else if (edosd & 0x04) {
> edosd |= 0x08;
> /* Set DRAMC[4:3] to proper memory type (EDO/SDRAM/Registered SDRAM). */
> /* i will be used to set DRAMC[4:3]. */
> if (regsd && sd) {
> i = 0x10; // Registered SDRAM

The datasheets says that this are bits: i = 0x2, not 0x10.

+ } else if (sd) {
> i = 0x08; // SDRAM

i = 0x1, not 0x8

+ } else {
> i = 0; // EDO

}
> edosd &= 0x18;
>
> /* edosd is now in the form needed for DRAMC[4:3]. */
>
> value = pci_read_config8(NB, DRAMC) & 0xe7;
> value |= edosd;
> value |= i;
>
> pci_write_config8(NB, DRAMC, value);
> PRINT_DEBUG("DRAMC has been set to 0x%02x\n", value);
> }

[PATCH] 440BX registered SDRAM support

[Keith Hui]

On Wed, Apr 6, 2011 at 6:19 PM, Idwer Vollering  wrote:
> Can you add a #define for SPD_MEMORY_TYPE_REGISTERED_SDRAM to
> src/include/spd.h as well ? If that is relevant to do, ofcourse.

There isn't a separate "registered SDRAM" type under SPD_MEMORY_TYPE.
The SDRAM module being registered is described in byte 21, as
(SPD_MODULE_ATTRIBUTES & MODULE_REGISTERED).

*snip*
> > /* If any installed DIMM is *not* registered, this system cannot be
> > configured for registered SDRAM.
> > By registered, only the address and control lines need to be, which
> > we can tell by reading SPD byte 21, bit 1.
> > */
> > value = spd_read_byte(device, SPD_MODULE_ATTRIBUTES);
> > PRINT_DEBUG("DIMM is ");
> > if ((value & 0x02) == 0) {

Speaking of which, this should be done to the patch:
(Signed-off-by: Keith Hui )

-			if ((value & 0x02) == 0) {
+			if ((value & MODULE_REGISTERED) == 0) {
> > regsd = 0;
> > PRINT_DEBUG("not ");
> > }
> > PRINT_DEBUG("registered\n");

*snip*
> > /* Set DRAMC[4:3] to proper memory type (EDO/SDRAM).
> > TODO: Registered SDRAM support.
> > */
> > edosd &= 0x07;
> > if (edosd & 0x02) {
> > edosd |= 0x00;
> > } else if (edosd & 0x04) {
> > edosd |= 0x08;
> > /* Set DRAMC[4:3] to proper memory type (EDO/SDRAM/Registered SDRAM). */
> > /* i will be used to set DRAMC[4:3]. */
> > if (regsd && sd) {
> > i = 0x10; // Registered SDRAM
>
> The datasheets says that this are bits: i = 0x2, not 0x10.
> > } else if (sd) {
> > i = 0x08; // SDRAM
>
> i = 0x1, not 0x8

The relevant bits are in bits 4-3 of the register. So it actually is
(2 << 3) and (1 << 3), which becomes what you see. This is done so I
don't have to shift the bits later, which save a few instructions.

Thanks
Keith