[LinuxBIOS] SPD Logical and Physical Banks

Hello,
Can someone please explain to me the difference between Logical and  
Physical Banks on SDRAM? I am looking for a quick and dirty way to  
determine if the SDRAM module is single sided or double sided through  
SPD. Would I use the Logical (SPD Byte 17) or Physical (SPD Byte 5)  
Bank for that?

Thanks - Joe

[Corey Osgood]

[e-mail] wrote:
> Hello,
> Can someone please explain to me the difference between Logical and  
> Physical Banks on SDRAM? I am looking for a quick and dirty way to  
> determine if the SDRAM module is single sided or double sided through  
> SPD. Would I use the Logical (SPD Byte 17) or Physical (SPD Byte 5)  
> Bank for that?
>
> Thanks - Joe

Well, in the best case, neither. Read the Intel SPD standard
(http://www.intel.com/design/chipsets/memory/spdsd12a.pdf), I've checked
several PC66, 100, and 133 dimms, and all of them so far support it.
Check out bytes 126 and 127. If that doesn't work out though, you're
looking for byte 5.

-Corey

[Uwe Hermann]

On Tue, May 15, 2007 at 04:45:53AM -0400, Corey Osgood wrote:
> [e-mail] wrote:
> > Hello,
> > Can someone please explain to me the difference between Logical and  
> > Physical Banks on SDRAM? I am looking for a quick and dirty way to  
> > determine if the SDRAM module is single sided or double sided through  
> > SPD. Would I use the Logical (SPD Byte 17) or Physical (SPD Byte 5)  
> > Bank for that?
> >
> > Thanks - Joe
>
> Well, in the best case, neither. Read the Intel SPD standard
> (http://www.intel.com/design/chipsets/memory/spdsd12a.pdf), I've checked
> several PC66, 100, and 133 dimms, and all of them so far support it.
> Check out bytes 126 and 127. If that doesn't work out though, you're
> looking for byte 5.

Just to prevent unnecessary duplicated work: please don't write
SPD-related functions for 440BX. I'm about to finish a patch which moves
all generic SPD-related functions into src/sdram/spd.c where all
chipsets can use them.

The SPD-only stuff should be chipset-independent, only the
chipset-specific stuff should be handled in the respective raminit.c.

Uwe.

Quoting Uwe Hermann :
> On Tue, May 15, 2007 at 04:45:53AM -0400, Corey Osgood wrote:
> > [e-mail] wrote:
> > > Hello,
> > > Can someone please explain to me the difference between Logical and
> > > Physical Banks on SDRAM? I am looking for a quick and dirty way to
> > > determine if the SDRAM module is single sided or double sided through
> > > SPD. Would I use the Logical (SPD Byte 17) or Physical (SPD Byte 5)
> > > Bank for that?
> > >
> > > Thanks - Joe
> >
> > Well, in the best case, neither. Read the Intel SPD standard
> > (http://www.intel.com/design/chipsets/memory/spdsd12a.pdf), I've checked
> > several PC66, 100, and 133 dimms, and all of them so far support it.
> > Check out bytes 126 and 127. If that doesn't work out though, you're
> > looking for byte 5.
>
> Just to prevent unnecessary duplicated work: please don't write
> SPD-related functions for 440BX. I'm about to finish a patch which moves
> all generic SPD-related functions into src/sdram/spd.c where all
> chipsets can use them.
>
> The SPD-only stuff should be chipset-independent, only the
> chipset-specific stuff should be handled in the respective raminit.c.

Uwe, This is for the intel 82830 chipset I have been working on. It is  
for raminit.c, it think it would be much eaisier to setup the DRB  
register if you could determine if the SDRAM module is single sided or  
double sided through SPD.

Thanks - Joe

[Corey Osgood]

Uwe Hermann wrote:
> On Tue, May 15, 2007 at 04:45:53AM -0400, Corey Osgood wrote:
> > [e-mail] wrote:
> > > Hello,
> > > Can someone please explain to me the difference between Logical and  
> > > Physical Banks on SDRAM? I am looking for a quick and dirty way to  
> > > determine if the SDRAM module is single sided or double sided through  
> > > SPD. Would I use the Logical (SPD Byte 17) or Physical (SPD Byte 5)  
> > > Bank for that?
> > >
> > > Thanks - Joe
> >
> > Well, in the best case, neither. Read the Intel SPD standard
> > (http://www.intel.com/design/chipsets/memory/spdsd12a.pdf), I've checked
> > several PC66, 100, and 133 dimms, and all of them so far support it.
> > Check out bytes 126 and 127. If that doesn't work out though, you're
> > looking for byte 5.
>
> Just to prevent unnecessary duplicated work: please don't write
> SPD-related functions for 440BX. I'm about to finish a patch which moves
> all generic SPD-related functions into src/sdram/spd.c where all
> chipsets can use them.
>
> The SPD-only stuff should be chipset-independent, only the
> chipset-specific stuff should be handled in the respective raminit.c.
>
> Uwe.

That figures. Just finished a ton of SPD stuff for the 440bx last night.
Any luck on getting cache-as-ram going? I'm running out of registers
with my code, perhaps yours is more register-friendly.

-Corey

[Uwe Hermann]

On Tue, May 15, 2007 at 04:03:19PM -0400, Corey Osgood wrote:
> > Just to prevent unnecessary duplicated work: please don't write
> > SPD-related functions for 440BX. I'm about to finish a patch which moves
> > all generic SPD-related functions into src/sdram/spd.c where all
> > chipsets can use them.
> >
> > The SPD-only stuff should be chipset-independent, only the
> > chipset-specific stuff should be handled in the respective raminit.c.
>
> That figures. Just finished a ton of SPD stuff for the 440bx last night.

Hm, sorry... I only started on this patch today, I hope this didn't
cause too much trouble...

I'll post a draft soon, so that people can see which functions are
affected. This needs an abuild-run and tests on a bunch of different
hardware targets to make sure nothing breaks...
> Any luck on getting cache-as-ram going? I'm running out of registers
> with my code, perhaps yours is more register-friendly.

I have a version which _compiles_, but I haven't tested on real hardware
whether it actually works. Will do so soon, though.

Uwe.

[Corey Osgood]

Uwe Hermann wrote:
> On Tue, May 15, 2007 at 04:03:19PM -0400, Corey Osgood wrote:
> > > Just to prevent unnecessary duplicated work: please don't write
> > > SPD-related functions for 440BX. I'm about to finish a patch which moves
> > > all generic SPD-related functions into src/sdram/spd.c where all
> > > chipsets can use them.
> > >
> > > The SPD-only stuff should be chipset-independent, only the
> > > chipset-specific stuff should be handled in the respective raminit.c.
> >
> > That figures. Just finished a ton of SPD stuff for the 440bx last night.
>
> Hm, sorry... I only started on this patch today, I hope this didn't
> cause too much trouble...
>
> I'll post a draft soon, so that people can see which functions are
> affected. This needs an abuild-run and tests on a bunch of different
> hardware targets to make sure nothing breaks...

Well, if you want to compare notes at all, I've attached my raminit.c
(the file, not a patch). If you could, please maintain the MODEL_440ZX
stuff, as that's necessary for 440zx support. MODEL_440ZX will be
defined in the mainboard auto.c, once this is all up and running, and
those ifdef's are the ONLY differences between the two northbridges.
However, there's no way to detect the difference at runtime that I can
find, so a define is the only real way.
> > Any luck on getting cache-as-ram going? I'm running out of registers
> > with my code, perhaps yours is more register-friendly.
>
> I have a version which _compiles_, but I haven't tested on real hardware
> whether it actually works. Will do so soon, though.

Well, good luck! I've got the rest of the day free, so I hope to hear
from you soon.

-Corey

[Uwe Hermann]

On Tue, May 15, 2007 at 04:24:26PM -0400, Corey Osgood wrote:
> > I'll post a draft soon, so that people can see which functions are
> > affected. This needs an abuild-run and tests on a bunch of different
> > hardware targets to make sure nothing breaks...
>
> Well, if you want to compare notes at all, I've attached my raminit.c
> (the file, not a patch).

Oh, that's fine. Those were not the sort of SPD functions I was talking
about, we didn't duplicate any work :-) Your functions merely _use_
SPD functions (or spd_read_byte/smbus_read_byte per se), if at all.

I posted a draft patch of my SPD refactoring stuff for preliminary review...
> If you could, please maintain the MODEL_440ZX
> stuff, as that's necessary for 440zx support. MODEL_440ZX will be
> defined in the mainboard auto.c, once this is all up and running, and
> those ifdef's are the ONLY differences between the two northbridges.
> However, there's no way to detect the difference at runtime that I can
> find, so a define is the only real way.

Hm, why is that? Don't they have some sort of different IDs we can read
from some register?

But if that's not possible, a #define is fine too, IMO. It's great that
you planned for merged code beween 440BX/ZX/FX/etc.

Maybe we should rename the 'i440bx' directory to 'i440xx' later to make
it clear that (later) multiple chipsets will be supported?

Uwe.

[Corey Osgood]

Uwe Hermann wrote:
> On Tue, May 15, 2007 at 04:24:26PM -0400, Corey Osgood wrote:
> > > I'll post a draft soon, so that people can see which functions are
> > > affected. This needs an abuild-run and tests on a bunch of different
> > > hardware targets to make sure nothing breaks...
> >
> > Well, if you want to compare notes at all, I've attached my raminit.c
> > (the file, not a patch).
>
> Oh, that's fine. Those were not the sort of SPD functions I was talking
> about, we didn't duplicate any work :-) Your functions merely _use_
> SPD functions (or spd_read_byte/smbus_read_byte per se), if at all.
>
> I posted a draft patch of my SPD refactoring stuff for preliminary review...

Yep, I've been looking over it. Definitely a WIP, but good work! And
good to know my work wasn't for nothing.
> > If you could, please maintain the MODEL_440ZX
> > stuff, as that's necessary for 440zx support. MODEL_440ZX will be
> > defined in the mainboard auto.c, once this is all up and running, and
> > those ifdef's are the ONLY differences between the two northbridges.
> > However, there's no way to detect the difference at runtime that I can
> > find, so a define is the only real way.
>
> Hm, why is that? Don't they have some sort of different IDs we can read
> from some register?
>
> But if that's not possible, a #define is fine too, IMO. It's great that
> you planned for merged code beween 440BX/ZX/FX/etc.

Not as far as I can tell, even the revision codes are shared between the
two. Perhaps some IO register varies, I haven't checked those yet. The
device IDs, etc, though are all identical, and the datasheet was made in
such a rush they forgot to change "bx" to "zx" in several places. A
define might be the best way anyways, since the north bridge will never
vary (in normal situations) between one boot and the next.
> Maybe we should rename the 'i440bx' directory to 'i440xx' later to make
> it clear that (later) multiple chipsets will be supported?

Sounds good, but lets not mess anything up for the moment. I'll probably
sneak that in when I send in the p2-99's patch, once everything is up
and running. Not sure how many other chipsets we'd be able to cram in
there, some of the other 440-series are quite a bit more different (lx,
gx), so perhaps 440bx_zx (or something similar) would be more appropriate.

-Corey

[Uwe Hermann]

Hi Corey,

here are a few more comments on the file/patch:

On Tue, May 15, 2007 at 04:24:26PM -0400, Corey Osgood wrote:
> Index: raminit.c
>
> --- raminit.c	(Revision 2671)
> +++ raminit.c	(Arbeitskopie)
> @@ -68,75 +69,19 @@
>
> /* Table format: register, bitmask, value. */
>  static const long register_values[] = {
> /* NBXCFG - NBX Configuration Register
> * 0x50
> *
> * [31:24] SDRAM Row Without ECC
> *         0 = ECC components are populated in this row
> *         1 = ECC components are not populated in this row

[...]
> * [01:00] Reserved
> */
> // TODO
> NBXCFG, 0x00000000, 0xff00a00c,

Hm, please keep these values and comments here for now. They might not
be needed for every register, but I think I remember that _some_ should
be set to "sane" defaults even _during_ RAM init. So it might make sense to
set them twice (here, and later in the code).
> * [5:5] Module Mode Configuration (MMCONFIG)
> *       TODO
> *       Set by external strapping options
> * 	 SDRAMPWR MMCONFIG CKE Operation
> *	    0       0       3 DIMM, CKE[5:0] driven, self-refresh entry
> *	    		    staggered. SDRAM dynamic power down available.
> *	    X       1       3 DIMM, CKE0 only, self-refresh entry not 
> *	    		    staggered. SDRAM dynamic power down unavailable.
> *	    1       0       4 DIMM, GCKE only, self-refresh entry staggered.
> *	    		    SDRAM dynamic power down unavailable.

Ack, thanks.
> @@ -189,30 +134,6 @@
>    PAM5, 0x00000000, 0x00,
>    PAM6, 0x00000000, 0x00,
> /* DRB[0:7] - DRAM Row Boundary Registers
> * 0x60 - 0x67
> *
> * An array of 8 byte registers, which hold the ending memory address
> * assigned to each pair of DIMMs, in 8MB granularity.   
> *
> * 0x60 DRB0 = Total memory in row0 (in 8 MB)
> * 0x61 DRB1 = Total memory in row0+1 (in 8 MB)
> * 0x62 DRB2 = Total memory in row0+1+2 (in 8 MB)
> * 0x63 DRB3 = Total memory in row0+1+2+3 (in 8 MB)
> * 0x64 DRB4 = Total memory in row0+1+2+3+4 (in 8 MB)
> * 0x65 DRB5 = Total memory in row0+1+2+3+4+5 (in 8 MB)
> * 0x66 DRB6 = Total memory in row0+1+2+3+4+5+6 (in 8 MB)
> * 0x67 DRB7 = Total memory in row0+1+2+3+4+5+6+7 (in 8 MB)
> */
> // TODO
> DRB0, 0x00, 0x00,
> DRB1, 0x00, 0x00,
> DRB2, 0x00, 0x00,
> DRB3, 0x00, 0x00,
> DRB4, 0x00, 0x00,
> DRB5, 0x00, 0x00,
> DRB6, 0x00, 0x00,
> DRB7, 0x00, 0x00,

See above, please keep this.
> @@ -390,7 +311,7 @@
>    reg = pci_read_config8(ctrl->d0, DRAMC);
>
> for (i = 0; i < DIMM_SOCKETS; i++) {
> value = spd_read_byte(ctrl->channel0[i], SPD_REFRESH);
> value = smbus_read_byte(ctrl->channel0[i], SPD_REFRESH);

Nope, please use spd_read_byte() everywhere.
> +static void spd_set_nbxcfg(const struct mem_controller *ctrl)

Please don't hardcode register names in function names. Use generic
function names which explain the step/work the function performs, e.g.

set_cas_latencies()
enable_timers()
set_refresh_rate()
etc.

The register might or might not have the same name in a similar chipset,
but the steps you have to perform are usually the same.

Also, generic names make the code much better readable and
understandable for new developers who try to understand it.
> * [31:24] SDRAM Row Without ECC
> *         0 = ECC components are populated in this row
> *         1 = ECC components are not populated in this row */
> /* Note: there's no way to tell 440zx from bx via hardware */
> /* 440ZX doesn't do ECC, and needs bits 31-24 set to 0, so skip this */
> #ifndef MODEL_440ZX

I'd even say you shouldn't worry about ECC all that much right now.
Just set it to off until everything else works fine. ECC is nice to
have, but non-essential.
> for(i = 0; i < 4; i++)

4 -> DIMM_SOCKETS

Please use readable #defines instead of "magic values" whereever
possible.
> {
> /* Check if this dimm supports ECC */
> spd_data = smbus_read_byte(ctrl->channel0[i], 11);

11 -> SPD_DIMM_CONFIG_TYPE

(same for all other SPD constants)
> /* If it does *not*, or there's no dimm present, set bits for both
>    sides to disable ECC, otherwise leave them enabled */ 
> if(spd_data != 2)

Even this '2' should be a #define in spd.h, maybe.
> /* Set the DRAM frequency to the lowest of all dimms */
> /* TODO: do this with the previous loop */
> for(i = 0; i < 4; i++)
> {
> spd_data = smbus_read_byte(ctrl->channel0[i], 126);
> /* If we find any 66MHz dimms, set the dimm speed to 66MHz and break */
> /* Note: This depends on the dimm following the Intel SPD standard */
> if(spd_data == 0x66)
> {
> 	nbxcfg |= 1 << 13;
> 	i = 6; //just to make sure

Huh?
> * [04:04] Reserved
> * [03:03] USWC Write Post During I/O Bridge Access Enable (UWPIO)
> *         1 = Enable
> *         0 = Disable
> * [02:02] In-Order Queue Depth (IOQD)
> *         1 = In-order queue = maximum
> *         0 = A7# is sampled asserted (i.e., 0)
> * [01:00] Reserved
> */
> nbxcfg |= 0xc; //sets bits 3 and 2 to 1
> PRINT_DEBUG("Value of NBXCFG calculated to 0x");
> PRINT_DEBUG_HEX32(nbxcfg);
> PRINT_DEBUG("/r/n");

\r\n
> /* Count to 8 since there are 8 possible rows, but count by 2 to check by dimm */
> /* Should 8 be replaced with DIMM_SOCKETS * 2? */

Yes, probably.
> for(i = 0; i < 8; i = i + 2)
> {
> /* First check if this row is populated with SDR SDRAM */
> if(smbus_read_byte(ctrl->channel0[i/2], 2) == 0x04)

This is very confusing. I think it would be easier to understand if you
count from 0..15 in steps of 1 and adapt the number where needed.
> 	dimm_size = rows * row_size * 4;
> 	print_debug("Found 0x");
> 	print_debug_hex8(dimm_size);
> 	print_debug("MB DIMM in slot ");
> 	print_debug_hex8(i/2);
> 	print_debug("\r\n");
> 	/* Convert dimms_size to be in 8MB chunks */
> 	dimm_size = dimm_size >> 3;

dimm_size /= 8;

Please don't use bit-manipulations where you really do arithmetic
operations (just makes the code harder to understand).

(don't worry about "optimizations" here, that's the job of the compiler)

In general the patch looks quite good. Please feel free to send single
patches for functions which are finished and usable, so we can merge
them one by one (and always check if the RAM still verifies).
No need to wait until all of the functions are finished...

Uwe.

[Corey Osgood]

Uwe Hermann wrote:
> Hi Corey,
>
> here are a few more comments on the file/patch:
>
> On Tue, May 15, 2007 at 04:24:26PM -0400, Corey Osgood wrote:
> > Index: raminit.c
> >
> > --- raminit.c	(Revision 2671)
> > +++ raminit.c	(Arbeitskopie)
> > @@ -68,75 +69,19 @@
> >
> > /* Table format: register, bitmask, value. */
> >  static const long register_values[] = {
> > /* NBXCFG - NBX Configuration Register
> > * 0x50
> > *
> > * [31:24] SDRAM Row Without ECC
> > *         0 = ECC components are populated in this row
> > *         1 = ECC components are not populated in this row
>
> [...]
> > * [01:00] Reserved
> > */
> > // TODO
> > NBXCFG, 0x00000000, 0xff00a00c,
>
> Hm, please keep these values and comments here for now. They might not
> be needed for every register, but I think I remember that _some_ should
> be set to "sane" defaults even _during_ RAM init. So it might make sense to
> set them twice (here, and later in the code).

Okay. If you notice, I've moved all the comments down into the spd
setup, but the values can stay (here and DRB), at least until I can test
it out. I'll put something in for a comment about it being an initial
value and checking the spd function.
> > @@ -390,7 +311,7 @@
> >    reg = pci_read_config8(ctrl->d0, DRAMC);
> >
> > for (i = 0; i < DIMM_SOCKETS; i++) {
> > value = spd_read_byte(ctrl->channel0[i], SPD_REFRESH);
> > value = smbus_read_byte(ctrl->channel0[i], SPD_REFRESH);
>
> Nope, please use spd_read_byte() everywhere.

spd_read_byte was removed to try an use a few less registers, I'm also
not using sdram_initialize for the same reason. Both got compiling a
little further, but not far enough.
> > +static void spd_set_nbxcfg(const struct mem_controller *ctrl)
>
> Please don't hardcode register names in function names. Use generic
> function names which explain the step/work the function performs, e.g.
>
> set_cas_latencies()
> enable_timers()
> set_refresh_rate()
> etc.
>
> The register might or might not have the same name in a similar chipset,
> but the steps you have to perform are usually the same.
>
> Also, generic names make the code much better readable and
> understandable for new developers who try to understand it.

Alright, sounds good, will take care of.
> > * [31:24] SDRAM Row Without ECC
> > *         0 = ECC components are populated in this row
> > *         1 = ECC components are not populated in this row */
> > /* Note: there's no way to tell 440zx from bx via hardware */
> > /* 440ZX doesn't do ECC, and needs bits 31-24 set to 0, so skip this */
> > #ifndef MODEL_440ZX
>
> I'd even say you shouldn't worry about ECC all that much right now.
> Just set it to off until everything else works fine. ECC is nice to
> have, but non-essential.

True, but it was only a few minutes of work to implement. ECC shouldn't
actually be enabled unless some other bit is set to 1. I put a comment
in that I wasn't going to mess with it.
> > /* Set the DRAM frequency to the lowest of all dimms */
> > /* TODO: do this with the previous loop */
> > for(i = 0; i < 4; i++)
> > {
> > spd_data = smbus_read_byte(ctrl->channel0[i], 126);
> > /* If we find any 66MHz dimms, set the dimm speed to 66MHz and break */
> > /* Note: This depends on the dimm following the Intel SPD standard */
> > if(spd_data == 0x66)
> > {
> > 	nbxcfg |= 1 << 13;
> > 	i = 6; //just to make sure
>
> Huh?

To make sure the loop is broken, I wasn't sure if break (which should be
the next line) would break out of the if statement or the for loop, so I
threw that in there (if i=6, i<4). It was very late (2am or so) when I
was writing this, and it shouldn't be necessary.
> > nbxcfg |= 0xc; //sets bits 3 and 2 to 1
> > PRINT_DEBUG("Value of NBXCFG calculated to 0x");
> > PRINT_DEBUG_HEX32(nbxcfg);
> > PRINT_DEBUG("/r/n");
>
> \r\n

good catch, fixed.
> > /* Count to 8 since there are 8 possible rows, but count by 2 to check by dimm */
> > /* Should 8 be replaced with DIMM_SOCKETS * 2? */
>
> Yes, probably.
> > for(i = 0; i < 8; i = i + 2)
> > {
> > /* First check if this row is populated with SDR SDRAM */
> > if(smbus_read_byte(ctrl->channel0[i/2], 2) == 0x04)
>
> This is very confusing. I think it would be easier to understand if you
> count from 0..15 in steps of 1 and adapt the number where needed.

Fixed. It now counts 0 to DIMM_SOCKETS in steps of 1, to hit all the dimms.
> > 	dimm_size = rows * row_size * 4;
> > 	print_debug("Found 0x");
> > 	print_debug_hex8(dimm_size);
> > 	print_debug("MB DIMM in slot ");
> > 	print_debug_hex8(i/2);
> > 	print_debug("\r\n");
> > 	/* Convert dimms_size to be in 8MB chunks */
> > 	dimm_size = dimm_size >> 3;
>
> dimm_size /= 8;
>
> Please don't use bit-manipulations where you really do arithmetic
> operations (just makes the code harder to understand).
>
> (don't worry about "optimizations" here, that's the job of the compiler)

Fixed. I wasn't even really thinking about readability, just if it would
work. This still needs (IMO) quite a bit of cleanup before sending in a
real patch.
> In general the patch looks quite good. Please feel free to send single
> patches for functions which are finished and usable, so we can merge
> them one by one (and always check if the RAM still verifies).
> No need to wait until all of the functions are finished...

Thanks! I'll get on this soon. I'm going to try to rip out as much of
the complex/non-essential stuff as possible to see how close I can get
to the limitations of romcc. It looks like we will need cache as ram
before we can do the full ram setup in the pre-ram code though.

-Corey

[Peter Stuge]

On Thu, May 17, 2007 at 03:48:45PM -0400, Corey Osgood wrote:
> > Nope, please use spd_read_byte() everywhere.
>
> spd_read_byte was removed to try an use a few less registers,

Rather use macros then, but both those functions should probably be
inline because they are so simple.
> > > for(i = 0; i < 4; i++)
> > > {
> > > spd_data = smbus_read_byte(ctrl->channel0[i], 126);
> > > if(spd_data == 0x66)
> > > {
> > > 	i = 6; //just to make sure
> >
> > Huh?
>
> To make sure the loop is broken, I wasn't sure if break (which
> should be the next line) would break out of the if statement or the
> for loop,

break exits for(), while() and switch() but not if().

//Peter