[LinuxBIOS] Proposed change to amdk8 memory controller code

[San Mehat]

Greets,

I've made some modifications locally here to the amdk8 memory code and
wanted to just chuck them out there to see what you guys think:

1 - Renamed DIMM_SOCKETS define to DIMM_SOCKETS_PER_CHANNEL since thats what
it actually is.
2 - Added 'channel0_pop' and 'channel1_pop' to 'struct mem_controller' and
modified 'raminit.c' to fill this in with the physical DIMM population map.

For us knowing the detected DIMM population map *at the mainboard level* is
very usefull.

Fire away ;)

-San

[Richard Smith]

> 1 - Renamed DIMM_SOCKETS define to DIMM_SOCKETS_PER_CHANNEL since thats
> what it actually is.

I'm all for making the names a lot more descriptive.  There are lots
of them that are really what they say they are.
> 2 - Added 'channel0_pop' and 'channel1_pop' to 'struct mem_controller' and
> modified 'raminit.c' to fill this in with the physical DIMM population map.

You are talking about what sockets are actually loaded with a DIMM right?
> For us knowing the detected DIMM population map *at the mainboard level* is
> very usefull.

Interesting.  For what?

[Richard Smith]

> I'm all for making the names a lot more descriptive.  There are lots
> of them that are really what they say they are.

Gotta slow down when I type.  That should have been "are not" what
they say they are.

[yhlu]

1. that would be too long, do we need DIMM_SOCKETS_PER_CHANNEL_PER_NODE?
2. that is controller array is "static const", you need to change the
attribute so you can change the value....that would break the code for
ROMCC....
dimm_mask already did popppp for you.
 for rev f.. I have made one CAR only raminit.c, it keep the dimmask and
needed dimm spd info in struct mem_info...
So don't need to read pci_conf or smbus to get info....
it include dimm_mask, x4_mask, x16_mask, ecc_mask, registered_mask,
is_Width128, is_opteron.....
 YH
 On 10/20/05, San Mehat  wrote:
> Greets,
>
> I've made some modifications locally here to the amdk8 memory code and
> wanted to just chuck them out there to see what you guys think:
>
> 1 - Renamed DIMM_SOCKETS define to DIMM_SOCKETS_PER_CHANNEL since thats
> what it actually is.
> 2 - Added 'channel0_pop' and 'channel1_pop' to 'struct mem_controller' and
> modified 'raminit.c' to fill this in with the physical DIMM population
> map.
>
> For us knowing the detected DIMM population map *at the mainboard level*
> is very usefull.
>
> Fire away ;)
>
> -San

[San Mehat]

1- I dont think we should be worried about names that are too long. I'd much
rather have a long name than a non-descriptive one (or in this case a
totally misleading one)
2- Changing the attribute shouldn't be that big of a deal.
3 - dimm_mask isn't exposed to the mainboard layer which is where we need
it.

As for 'why we need it', we have a post card which displays the DIMM
population map on boot for our ops guys...

On 10/20/05, yhlu  wrote:
> that would be too long, do we need DIMM_SOCKETS_PER_CHANNEL_PER_NODE?
> that is controller array is "static const", you need to change the
>
> attribute so you can change the value....that would break the code for
> ROMCC....
> dimm_mask already did popppp for you.
>  for rev f.. I have made one CAR only raminit.c, it keep the dimmask and
> needed dimm spd info in struct mem_info...
> So don't need to read pci_conf or smbus to get info....
> it include dimm_mask, x4_mask, x16_mask, ecc_mask, registered_mask,
> is_Width128, is_opteron.....
>  YH
>  On 10/20/05, San Mehat  wrote:
> > Greets,
> >
> > I've made some modifications locally here to the amdk8 memory code and
> > wanted to just chuck them out there to see what you guys think:
> >
> > 1 - Renamed DIMM_SOCKETS define to DIMM_SOCKETS_PER_CHANNEL since thats
> > what it actually is.
> > 2 - Added 'channel0_pop' and 'channel1_pop' to 'struct mem_controller'
> > and modified 'raminit.c' to fill this in with the physical DIMM
> > population map.
> >
> > For us knowing the detected DIMM population map *at the mainboard level*
> > is very usefull.
> >
> > Fire away ;)
> >
> > -San

[Ronald G Minnich]

San Mehat wrote:
> 1- I dont think we should be worried about names that are too long. I'd 
> much rather have a long name than a non-descriptive one (or in this case 
> a totally misleading one)
> 2- Changing the attribute shouldn't be that big of a deal.
> 3 - dimm_mask isn't exposed to the mainboard layer which is where we 
> need it.
>
> As for 'why we need it', we have a post card which displays the DIMM 
> population map on boot for our ops guys...

1. I'm ok with long names. These are rarely typed, more often read, and 
should be correct.
2. yeah
3. yeah, expose as much info as we can. We're a bios.

can you send us a diff  in -u format so we can take a look?

ron

[San Mehat]

Reworking the attribute is actually causing a huge pain in the ass with
relocations it looks like. I'm going to see if i can have the amdk8 MCT code
return the dimm_mask information back to the board via sdram_initialize().

I could either just have sdram_initialize() return a merged version of
dimm_mask which would limit the number of dimms supported, or I could pass
in an additional non-static 'population' structure which i think may be
better.

Any thoughts?.. I'm going to modify my stuff to pass in a new structure in
the meantime..

-san

On 10/20/05, Ronald G Minnich  wrote:
> San Mehat wrote:
> > 1- I dont think we should be worried about names that are too long. I'd
> > much rather have a long name than a non-descriptive one (or in this case
> > a totally misleading one)
> > 2- Changing the attribute shouldn't be that big of a deal.
> > 3 - dimm_mask isn't exposed to the mainboard layer which is where we
> > need it.
> >
> > As for 'why we need it', we have a post card which displays the DIMM
> > population map on boot for our ops guys...
> I'm ok with long names. These are rarely typed, more often read, and
>
> should be correct.
> 2. yeah
> 3. yeah, expose as much info as we can. We're a bios.
>
> can you send us a diff in -u format so we can take a look?
>
> ron

San Mehat  writes:
> Reworking the attribute is actually causing a huge pain in the ass with
> relocations it looks like. I'm going to see if i can have the amdk8 MCT code
> return the dimm_mask information back to the board via sdram_initialize().
>
> I could either just have sdram_initialize() return a merged version of
> dimm_mask which would limit the number of dimms supported, or I could pass in
> an additional non-static 'population' structure which i think may be better.
>
> Any thoughts?.. I'm going to modify my stuff to pass in a new structure in the
> meantime..

Is there any reason not to simply query for the data back at the mainboard
level?  It isn't like the discovery is hard.

Eric

[San Mehat]

No, but it adds yet more duplicated code. Since in most cases all memory
init code can figure out what the population map looks like on a channel
bases, pass that info to the board. The board module can swizzle the
population map by channel into population map by physical dimm slot (since
that mapping may be different on a per board level).

-San

On 10/28/05, Eric W. Biederman  wrote:
> San Mehat  writes:
> > Reworking the attribute is actually causing a huge pain in the ass with
> > relocations it looks like. I'm going to see if i can have the amdk8 MCT
>
> code
> > return the dimm_mask information back to the board via
>
> sdram_initialize().
> > I could either just have sdram_initialize() return a merged version of
> > dimm_mask which would limit the number of dimms supported, or I could
>
> pass in
> > an additional non-static 'population' structure which i think may be
>
> better.
> > Any thoughts?.. I'm going to modify my stuff to pass in a new structure
>
> in the
> > meantime..
>
> Is there any reason not to simply query for the data back at the mainboard
> level?  It isn't like the discovery is hard.
>
> Eric