Convert all boards using fake SPD entries to struct spd_entry, thereby making sure we return 0xff for nonexisiting entries and shrinking the data structure by 85%. As a bonus, the various initram.c for boards with fake SPD are now almost identical.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: LinuxBIOSv3-spd/mainboard/artecgroup/dbe61/initram.c =================================================================== --- LinuxBIOSv3-spd/mainboard/artecgroup/dbe61/initram.c (Revision 621) +++ LinuxBIOSv3-spd/mainboard/artecgroup/dbe61/initram.c (Arbeitskopie) @@ -55,41 +55,59 @@ * Lead Free (P) * DDR400 3-3-3 (D43) */ -/* SPD array */ -static const u8 spdbytes[] = { - [SPD_ACCEPTABLE_CAS_LATENCIES] = 0x10, - [SPD_BANK_DENSITY] = 0x40, - [SPD_DEVICE_ATTRIBUTES_GENERAL] = 0xff, - [SPD_MEMORY_TYPE] = 7, - [SPD_MIN_CYCLE_TIME_AT_CAS_MAX] = 10, /* A guess for the tRAC value */ - [SPD_MODULE_ATTRIBUTES] = 0xff, /* FIXME later when we figure out. */ - [SPD_NUM_BANKS_PER_SDRAM] = 4, - [SPD_PRIMARY_SDRAM_WIDTH] = 8, - [SPD_NUM_DIMM_BANKS] = 1, /* ALIX1.C is 1 bank. */ - [SPD_NUM_COLUMNS] = 0xa, - [SPD_NUM_ROWS] = 3, - [SPD_REFRESH] = 0x3a, - [SPD_SDRAM_CYCLE_TIME_2ND] = 60, - [SPD_SDRAM_CYCLE_TIME_3RD] = 75, - [SPD_tRAS] = 40, - [SPD_tRCD] = 15, - [SPD_tRFC] = 70, - [SPD_tRP] = 15, - [SPD_tRRD] = 10, + +struct spd_entry { + u8 address; + u8 data; };
-u8 spd_read_byte(u16 device, u8 address) +/* Save space by using a short list of SPD values used by Geode LX Memory init */ +static const struct spd_entry spdbytes[] = { + {SPD_ACCEPTABLE_CAS_LATENCIES, 0x10}, + {SPD_BANK_DENSITY, 0x40}, + {SPD_DEVICE_ATTRIBUTES_GENERAL, 0xff}, + {SPD_MEMORY_TYPE, 7}, + {SPD_MIN_CYCLE_TIME_AT_CAS_MAX, 10}, /* A guess for the tRAC value */ + {SPD_MODULE_ATTRIBUTES, 0xff}, /* FIXME later when we figure out. */ + {SPD_NUM_BANKS_PER_SDRAM, 4}, + {SPD_PRIMARY_SDRAM_WIDTH, 8}, + {SPD_NUM_DIMM_BANKS, 1}, /* ALIX1.C is 1 bank. */ + {SPD_NUM_COLUMNS, 0xa}, + {SPD_NUM_ROWS, 3}, + {SPD_REFRESH, 0x3a}, + {SPD_SDRAM_CYCLE_TIME_2ND, 60}, + {SPD_SDRAM_CYCLE_TIME_3RD, 75}, + {SPD_tRAS, 40}, + {SPD_tRCD, 15}, + {SPD_tRFC, 70}, + {SPD_tRP, 15}, + {SPD_tRRD, 10}, +}; + +/** + * Given an SMBUS device, and an address in that device, return the value of SPD + * for that device. In this mainboard, the only one that can return is DIMM0. + * @param device The device number + * @param address The address in SPD rom to return the value of + * @returns The value + */ +static u8 spd_read_byte(u16 device, u8 address) { + int i; + /* returns 0xFF on any failures */ + u8 ret = 0xff; + printk(BIOS_DEBUG, "spd_read_byte dev %04x\n", device); - - if (device != (0x50 << 1)) { - printk(BIOS_DEBUG, " returns 0xff\n"); - return 0xff; + if (device == DIMM0) { + for (i = 0; i < ARRAY_SIZE(spd_table); i++) { + if (spd_table[i].address == address) { + ret = spd_table[i].data; + } + } }
- printk(BIOS_DEBUG, " addr %02x returns %02x\n", address, spdbytes[address]); - - return spdbytes[address]; + printk(BIOS_DEBUG, " addr %02x returns %02x\n", address, ret); + return ret; }
/** Index: LinuxBIOSv3-spd/mainboard/artecgroup/dbe62/initram.c =================================================================== --- LinuxBIOSv3-spd/mainboard/artecgroup/dbe62/initram.c (Revision 621) +++ LinuxBIOSv3-spd/mainboard/artecgroup/dbe62/initram.c (Arbeitskopie) @@ -45,36 +45,35 @@ * @ 200 ns, data-out window, 1.6; access, +- 70 ns; dqs-dq skew: .4ns * http://www.micron.com/products/partdetail?part=MT46V16M16P-5B */ + struct spd_entry { u8 address; u8 data; };
/* Save space by using a short list of SPD values used by Geode LX Memory init */ - static const struct spd_entry spdbytes[] = { - {SPD_ACCEPTABLE_CAS_LATENCIES, 0x10}, - {SPD_BANK_DENSITY, 0x40}, - {SPD_DEVICE_ATTRIBUTES_GENERAL, 0xff}, - {SPD_MEMORY_TYPE, 7}, - {SPD_MIN_CYCLE_TIME_AT_CAS_MAX, 10, /* A guess for the tRAC value */ - {SPD_MODULE_ATTRIBUTES, 0xff, /* FIXME later when we figure out. */ - {SPD_NUM_BANKS_PER_SDRAM, 4}, - {SPD_PRIMARY_SDRAM_WIDTH, 8}, - {SPD_NUM_DIMM_BANKS, 1}, - {SPD_NUM_COLUMNS, 0xa}, - {SPD_NUM_ROWS, 3}, - {SPD_REFRESH, 0x3a}, - {SPD_SDRAM_CYCLE_TIME_2ND, 60}, - {SPD_SDRAM_CYCLE_TIME_3RD, 75}, - {SPD_tRAS, 40}, - {SPD_tRCD, 15}, - {SPD_tRFC, 70}, - {SPD_tRP, 15}, - {SPD_tRRD, 10, + {SPD_ACCEPTABLE_CAS_LATENCIES, 0x10}, + {SPD_BANK_DENSITY, 0x40}, + {SPD_DEVICE_ATTRIBUTES_GENERAL, 0xff}, + {SPD_MEMORY_TYPE, 7}, + {SPD_MIN_CYCLE_TIME_AT_CAS_MAX, 10}, /* A guess for the tRAC value */ + {SPD_MODULE_ATTRIBUTES, 0xff}, /* FIXME later when we figure out. */ + {SPD_NUM_BANKS_PER_SDRAM, 4}, + {SPD_PRIMARY_SDRAM_WIDTH, 8}, + {SPD_NUM_DIMM_BANKS, 1}, + {SPD_NUM_COLUMNS, 0xa}, + {SPD_NUM_ROWS, 3}, + {SPD_REFRESH, 0x3a}, + {SPD_SDRAM_CYCLE_TIME_2ND, 60}, + {SPD_SDRAM_CYCLE_TIME_3RD, 75}, + {SPD_tRAS, 40}, + {SPD_tRCD, 15}, + {SPD_tRFC, 70}, + {SPD_tRP, 15}, + {SPD_tRRD, 10}, };
- /** * Given an SMBUS device, and an address in that device, return the value of SPD * for that device. In this mainboard, the only one that can return is DIMM0. @@ -89,9 +88,9 @@ u8 ret = 0xff;
printk(BIOS_DEBUG, "spd_read_byte dev %04x\n", device); - if (device == DIMM0){ - for (i=0; i < (sizeof spd_table/sizeof spd_table[0]); i++){ - if (spd_table[i].address == address){ + if (device == DIMM0) { + for (i = 0; i < ARRAY_SIZE(spd_table); i++) { + if (spd_table[i].address == address) { ret = spd_table[i].data; } } Index: LinuxBIOSv3-spd/mainboard/pcengines/alix1c/initram.c =================================================================== --- LinuxBIOSv3-spd/mainboard/pcengines/alix1c/initram.c (Revision 621) +++ LinuxBIOSv3-spd/mainboard/pcengines/alix1c/initram.c (Arbeitskopie) @@ -55,41 +55,59 @@ * Lead Free (P) * DDR400 3-3-3 (D43) */ -/* SPD array */ -static const u8 spdbytes[] = { - [SPD_ACCEPTABLE_CAS_LATENCIES] = 0x10, - [SPD_BANK_DENSITY] = 0x40, - [SPD_DEVICE_ATTRIBUTES_GENERAL] = 0xff, - [SPD_MEMORY_TYPE] = 7, - [SPD_MIN_CYCLE_TIME_AT_CAS_MAX] = 10, /* A guess for the tRAC value */ - [SPD_MODULE_ATTRIBUTES] = 0xff, /* FIXME later when we figure out. */ - [SPD_NUM_BANKS_PER_SDRAM] = 4, - [SPD_PRIMARY_SDRAM_WIDTH] = 8, - [SPD_NUM_DIMM_BANKS] = 1, /* ALIX1.C is 1 bank. */ - [SPD_NUM_COLUMNS] = 0xa, - [SPD_NUM_ROWS] = 3, - [SPD_REFRESH] = 0x3a, - [SPD_SDRAM_CYCLE_TIME_2ND] = 60, - [SPD_SDRAM_CYCLE_TIME_3RD] = 75, - [SPD_tRAS] = 40, - [SPD_tRCD] = 15, - [SPD_tRFC] = 70, - [SPD_tRP] = 15, - [SPD_tRRD] = 10, + +struct spd_entry { + u8 address; + u8 data; };
-u8 spd_read_byte(u16 device, u8 address) +/* Save space by using a short list of SPD values used by Geode LX Memory init */ +static const struct spd_entry spdbytes[] = { + {SPD_ACCEPTABLE_CAS_LATENCIES, 0x10}, + {SPD_BANK_DENSITY, 0x40}, + {SPD_DEVICE_ATTRIBUTES_GENERAL, 0xff}, + {SPD_MEMORY_TYPE, 7}, + {SPD_MIN_CYCLE_TIME_AT_CAS_MAX, 10}, /* A guess for the tRAC value */ + {SPD_MODULE_ATTRIBUTES, 0xff}, /* FIXME later when we figure out. */ + {SPD_NUM_BANKS_PER_SDRAM, 4}, + {SPD_PRIMARY_SDRAM_WIDTH, 8}, + {SPD_NUM_DIMM_BANKS, 1}, /* ALIX1.C is 1 bank. */ + {SPD_NUM_COLUMNS, 0xa}, + {SPD_NUM_ROWS, 3}, + {SPD_REFRESH, 0x3a}, + {SPD_SDRAM_CYCLE_TIME_2ND, 60}, + {SPD_SDRAM_CYCLE_TIME_3RD, 75}, + {SPD_tRAS, 40}, + {SPD_tRCD, 15}, + {SPD_tRFC, 70}, + {SPD_tRP, 15}, + {SPD_tRRD, 10}, +}; + +/** + * Given an SMBUS device, and an address in that device, return the value of SPD + * for that device. In this mainboard, the only one that can return is DIMM0. + * @param device The device number + * @param address The address in SPD rom to return the value of + * @returns The value + */ +static u8 spd_read_byte(u16 device, u8 address) { + int i; + /* returns 0xFF on any failures */ + u8 ret = 0xff; + printk(BIOS_DEBUG, "spd_read_byte dev %04x\n", device); - - if (device != (0x50 << 1)) { - printk(BIOS_DEBUG, " returns 0xff\n"); - return 0xff; + if (device == DIMM0) { + for (i = 0; i < ARRAY_SIZE(spd_table); i++) { + if (spd_table[i].address == address) { + ret = spd_table[i].data; + } + } }
- printk(BIOS_DEBUG, " addr %02x returns %02x\n", address, spdbytes[address]); - - return spdbytes[address]; + printk(BIOS_DEBUG, " addr %02x returns %02x\n", address, ret); + return ret; }
/**
On Tue, Feb 26, 2008 at 07:53:16PM +0100, Carl-Daniel Hailfinger wrote:
Convert all boards using fake SPD entries to struct spd_entry, thereby making sure we return 0xff for nonexisiting entries and shrinking the data structure by 85%.
Can you elaborate? 85% less in RAM or in ROM? How did you measure this?
As a bonus, the various initram.c for boards with fake SPD are now almost identical.
Nice! (next step: generic code in lib/, drop per-board code)
+struct spd_entry {
- u8 address;
- u8 data;
};
As this is the same in each file it should probably go in some header.
Uwe.
On Tue, Feb 26, 2008 at 11:04 AM, Uwe Hermann uwe@hermann-uwe.de wrote:
On Tue, Feb 26, 2008 at 07:53:16PM +0100, Carl-Daniel Hailfinger wrote:
Convert all boards using fake SPD entries to struct spd_entry, thereby making sure we return 0xff for nonexisiting entries and shrinking the data structure by 85%.
Can you elaborate? 85% less in RAM or in ROM? How did you measure this?
due to the big sparse struct. It's not a huge deal after compression :-)
As a bonus, the various initram.c for boards with fake SPD are now almost identical.
Nice! (next step: generic code in lib/, drop per-board code)
yeah but be careful.
There are lots of special cases. I think we could close out most of them as follows: provide an array called valid_spd_device which contains a 0-terminate list of valid SPDs.
Then the function above can, first: 1. for loop to find out if spd is valid, if not, return 0xff 2. for loop that is currently there.
I can provide sample code if this is not clear.
But this is still cool. I will test on alix1c tonight and report back.
ron
On 26.02.2008 20:11, ron minnich wrote:
On Tue, Feb 26, 2008 at 11:04 AM, Uwe Hermann uwe@hermann-uwe.de wrote:
On Tue, Feb 26, 2008 at 07:53:16PM +0100, Carl-Daniel Hailfinger wrote:
Convert all boards using fake SPD entries to struct spd_entry, thereby making sure we return 0xff for nonexisiting entries and shrinking the data structure by 85%.
Can you elaborate? 85% less in RAM or in ROM? How did you measure this?
due to the big sparse struct. It's not a huge deal after compression :-)
As a bonus, the various initram.c for boards with fake SPD are now almost identical.
Nice! (next step: generic code in lib/, drop per-board code)
yeah but be careful.
There are lots of special cases. I think we could close out most of them as follows: provide an array called valid_spd_device which contains a 0-terminate list of valid SPDs.
You mean you want to simulate multiple SPD EEPROMs?
Then the function above can, first:
- for loop to find out if spd is valid, if not, return 0xff
- for loop that is currently there.
I can provide sample code if this is not clear.
I think it is clear if my understanding above is right.
But this is still cool. I will test on alix1c tonight and report back.
Thanks!
Regards, Carl-Daniel
On Tue, Feb 26, 2008 at 4:14 PM, Peter Stuge peter@stuge.se wrote:
On Wed, Feb 27, 2008 at 01:09:41AM +0100, Carl-Daniel Hailfinger wrote:
an array called valid_spd_device
Should we put fake SPD in the dts? It is, after all, hardware description. Or no?
We talked about this a lot about 15 months ago. It's a harder question than it seems at first.
The problem is that we have to be careful to take the 'everything in dts' only as far as it makes sense. Right now, we generate code from the dts to C structs and do a lot of build-time checking. As we add more things to dts, we should add more code generation. We do not want to have a situation where we're doing this at runtime: spdval = value("/dram/spd/tpr"); because there is lots of build time checking we lose (e.g. somebody specifies a totally wacky 32-bit number for what is a u8 value).
I like the dts but I'm not sure we want every single thing in it. But I'm not claiming that the balance is perfect .
thanks
ron
On Tue, Feb 26, 2008 at 05:55:02PM -0800, ron minnich wrote:
As we add more things to dts, we should add more code generation.
Agreed. I don't think we can generate initram though.
We do not want to have a situation where we're doing this at runtime: spdval = value("/dram/spd/tpr"); because there is lots of build time checking we lose (e.g. somebody specifies a totally wacky 32-bit number for what is a u8 value).
I still think contents should be checked at build time. Actually, for static SPD it would be an improvement to do some validation at build time rather than failing in initram.
I like the dts but I'm not sure we want every single thing in it.
Not every thing, but I would like it to abstract the board.
//Peter
With a few minor mods to get it to compile (naming) it boots to linux with graphics on alix1c and builds on dbe61.
Acked-by: Ronald G. Minnich rminnich@gmail.com Committed rev 622
On 27.02.2008 06:28, ron minnich wrote:
With a few minor mods to get it to compile (naming)
Sorry about the compilation problem.
it boots to linux with graphics on alix1c and builds on dbe61.
That's great! Thanks for testing, fixing and committing.
Acked-by: Ronald G. Minnich rminnich@gmail.com Committed rev 622
The "multiple SPD" patch is next on my list once I know whether you want to simulate multiple independent SPD EEPROMS (for multiple RAM chips) or multiple related SPD EEPROMS.
Regards, Carl-Daniel
On Wed, Feb 27, 2008 at 3:28 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
The "multiple SPD" patch is next on my list once I know whether you want to simulate multiple independent SPD EEPROMS (for multiple RAM chips) or multiple related SPD EEPROMS.
Good question!
I think both. One board I at least has soldered-on and DIMMs. So the array might be:
struct dimarray { u8 dimm_address; struct spd_entry *table; int tablesize};
struct valid_spd_devices spd_table = { {0xa0, spd_table_onboard, nelem(spd_table_onboard)}, {0xa1, NULL,}, {0,0}};
Note that if we go to a generic function we need to have the size in the struct.
This starts to raise the question: is the generic function more trouble that it is worth? I am now beginning to think so. What if the second DIMM is an spd device? There are lots of variations possible, and it may just be simpler not to get too fancy.
ron
ron minnich wrote:
On Wed, Feb 27, 2008 at 3:28 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
The "multiple SPD" patch is next on my list once I know whether you want to simulate multiple independent SPD EEPROMS (for multiple RAM chips) or multiple related SPD EEPROMS.
Good question!
I think both. One board I at least has soldered-on and DIMMs. So the array might be:
struct dimarray { u8 dimm_address; struct spd_entry *table; int tablesize};
struct valid_spd_devices spd_table = { {0xa0, spd_table_onboard, nelem(spd_table_onboard)}, {0xa1, NULL,}, {0,0}};
Note that if we go to a generic function we need to have the size in the struct.
This starts to raise the question: is the generic function more trouble that it is worth? I am now beginning to think so. What if the second DIMM is an spd device? There are lots of variations possible, and it may just be simpler not to get too fancy.
ron
There are platforms with soldered-on and a dimm slot. That and the smbus addressing switching in k8 is why it is in mainboard.
As for the SPD in the dts (which I assume means in the statictree), I don't think so but for very minor reasons. It would take up a bunch of space in the statictree when it is only needed by initram. It would also be more convoluted to access it in the statictree. If someone has a good reason, I could be convinced that it should go in the dts.
Marc
On 27.02.2008 17:55, Marc Jones wrote:
ron minnich wrote:
On Wed, Feb 27, 2008 at 3:28 AM, Carl-Daniel Hailfinger wrote:
The "multiple SPD" patch is next on my list once I know whether you want
to simulate multiple independent SPD EEPROMS (for multiple RAM chips) or multiple related SPD EEPROMS.
Good question!
I think both. One board I at least has soldered-on and DIMMs. So the array might be:
struct dimarray { u8 dimm_address; struct spd_entry *table; int tablesize};
struct valid_spd_devices spd_table = { {0xa0, spd_table_onboard, nelem(spd_table_onboard)}, {0xa1, NULL,}, {0,0}};
Note that if we go to a generic function we need to have the size in the struct.
This starts to raise the question: is the generic function more trouble that it is worth? I am now beginning to think so. What if the second DIMM is an spd device? There are lots of variations possible, and it may just be simpler not to get too fancy.
There are platforms with soldered-on and a dimm slot. That and the smbus addressing switching in k8 is why it is in mainboard.
As for the SPD in the dts (which I assume means in the statictree), I don't think so but for very minor reasons. It would take up a bunch of space in the statictree when it is only needed by initram. It would also be more convoluted to access it in the statictree. If someone has a good reason, I could be convinced that it should go in the dts.
Indeed, keep the SPD outside the dts if at all possible. initram is a piece of code which does not care about the device tree and that's good.
I'd still try to keep the functions as similar as possible between mainboards, but the benefit of keeping them inside mainboard/$vendor/$model/initram.c is huge.
By the way, the fake SPD stuff also has another rather big benefit: You can read out the SPD under factory BIOS and supply the values as fake SPD to facilitate early board bringup, especially if SMBUS is acting up.
Regards, Carl-Daniel
On Wed, Feb 27, 2008 at 09:55:54AM -0700, Marc Jones wrote:
As for the SPD in the dts (which I assume means in the statictree)
Not neccessarily why I wanted it in the dts.
I don't think so but for very minor reasons. It would take up a bunch of space in the statictree when it is only needed by initram.
Needed yes, but maybe the information would be interesting for operating systems. EDAC?
It would also be more convoluted to access it in the statictree. If someone has a good reason, I could be convinced that it should go in the dts.
I view the dts as hardware description; relevant parts of netlist and bom distilled a bit. I think it makes sense to include RAM parts as well, if known.
//Peter
* Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [080226 19:53]:
Convert all boards using fake SPD entries to struct spd_entry, thereby making sure we return 0xff for nonexisiting entries and shrinking the data structure by 85%.
85% of 128 bytes? Therefore this is O(n) now instead of O(1) in ROM accesses. Are you sure?
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
On 29.02.2008 21:47, Stefan Reinauer wrote:
- Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [080226 19:53]:
Convert all boards using fake SPD entries to struct spd_entry, thereby making sure we return 0xff for nonexisiting entries and shrinking the data structure by 85%.
85% of 128 bytes? Therefore this is O(n) now instead of O(1) in ROM accesses. Are you sure?
Well, since we need the fake SPD in initram, we can't compress it and really lose that space in ROM. That's why I decided to accept higher lookup cost to achieve space savings. However, if anybody can show that the increased code complexity costs us more ROM space than what we save by converting the data structure from a direct access array to an iterated list, I'm all for changing it back.
Regards, Carl-Daniel
* Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [080229 22:04]:
85% of 128 bytes? Therefore this is O(n) now instead of O(1) in ROM accesses. Are you sure?
Well, since we need the fake SPD in initram, we can't compress it and really lose that space in ROM. That's why I decided to accept higher lookup cost to achieve space savings. However, if anybody can show that the increased code complexity costs us more ROM space than what we save by converting the data structure from a direct access array to an iterated list, I'm all for changing it back.
Oh I had assumed you had already analyzed the code space savings before claiming your method saves 85% by running in a loop instead of once.
Stefan
On 29.02.2008 22:16, Stefan Reinauer wrote:
- Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [080229 22:04]:
85% of 128 bytes? Therefore this is O(n) now instead of O(1) in ROM accesses. Are you sure?
Well, since we need the fake SPD in initram, we can't compress it and really lose that space in ROM. That's why I decided to accept higher lookup cost to achieve space savings. However, if anybody can show that the increased code complexity costs us more ROM space than what we save by converting the data structure from a direct access array to an iterated list, I'm all for changing it back.
Oh I had assumed you had already analyzed the code space savings before claiming your method saves 85% by running in a loop instead of once.
Sorry, I forgot that. Statistics for gcc 4.2.1, alix1c initram:
| old | new spd_read_byte size | 107 | 109 total size | 10000 | 9989
If the SPD array had been populated correctly in the old version (including checksum bytes), the savings would have been more substantial. Oh well.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Marc Jones -
Peter Stuge -
ron minnich -
Stefan Reinauer -
Uwe Hermann