Patchwork 440BX registered SDRAM support

login
register
about
Submitter Keith Hui
Date 2011-03-31 03:38:24
Message ID <AANLkTim4Xf+zwr9cUYamNYdNKyrCdhY1z5YKHYgkMiZM@mail.gmail.com>
Download mbox | patch
Permalink /patch/2822/
State New
Headers show

Comments

Keith Hui - 2011-03-31 03:38:24
On Tue, Mar 29, 2011 at 5:34 PM, Stefan Reinauer
<stefan.reinauer@coreboot.org> wrote:
> * Keith Hui <buurin@gmail.com> [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
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 <buurin@gmail.com>
Keith Hui - 2011-04-01 19:28:22
ping?

On Wed, Mar 30, 2011 at 11:38 PM, Keith Hui <buurin@gmail.com> wrote:
> On Tue, Mar 29, 2011 at 5:34 PM, Stefan Reinauer
> <stefan.reinauer@coreboot.org> wrote:
>> * Keith Hui <buurin@gmail.com> [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
>
Idwer Vollering - 2011-04-06 22:19:39
2011/4/1 Keith Hui <buurin@gmail.com>:
> 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 <buurin@gmail.com>
>
> 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);
> }
>
Keith Hui - 2011-04-07 01:32:08
On Wed, Apr 6, 2011 at 6:19 PM, Idwer Vollering <vidwer@gmail.com> 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 <buurin@gmail.com>)

-			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

Patch

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) {
-				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
+	} else if (sd) {
+		i = 0x08; // SDRAM
+	} 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);
 }