| Message ID | a977bd81f7c1f2896837671545fbb01ebf286f4a.1546532844.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | Misc sam460ex related patches | expand |
Hi Zoltan, On 1/3/19 5:27 PM, BALATON Zoltan wrote: > There are several boards with SPD EEPROMs that are now using > duplicated or slightly different hard coded data. Add a helper to > generate SPD data for a memory module of given type and size that > could be used by these boards (either as is or with further changes if > needed) which should help cleaning this up and avoid further duplication. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > v3: Fixed a tab indent > v2: Added errp parameter to pass errors back to caller > > hw/i2c/smbus_eeprom.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++ > include/hw/i2c/smbus.h | 3 ++ > 2 files changed, 133 insertions(+) > > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c > index f18aa3de35..bef24a1ca4 100644 > --- a/hw/i2c/smbus_eeprom.c > +++ b/hw/i2c/smbus_eeprom.c > @@ -23,6 +23,9 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/error-report.h" > +#include "qemu/units.h" > +#include "qapi/error.h" > #include "hw/hw.h" > #include "hw/i2c/i2c.h" > #include "hw/i2c/smbus.h" > @@ -162,3 +165,130 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, > smbus_eeprom_init_one(smbus, 0x50 + i, eeprom_buf + (i * 256)); > } > } > + > +/* Generate SDRAM SPD EEPROM data describing a module of type and size */ > +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size, > + Error **errp) > +{ > + uint8_t *spd; > + uint8_t nbanks; > + uint16_t density; > + uint32_t size; > + int min_log2, max_log2, sz_log2; > + int i; > + > + switch (type) { > + case SDR: > + min_log2 = 2; > + max_log2 = 9; > + break; > + case DDR: > + min_log2 = 5; > + max_log2 = 12; > + break; > + case DDR2: > + min_log2 = 7; > + max_log2 = 14; > + break; > + default: > + g_assert_not_reached(); > + } > + size = ram_size >> 20; /* work in terms of megabytes */ > + if (size < 4) { > + error_setg(errp, "SDRAM size is too small"); > + return NULL; > + } > + sz_log2 = 31 - clz32(size); > + size = 1U << sz_log2; > + if (ram_size > size * MiB) { > + error_setg(errp, "SDRAM size 0x"RAM_ADDR_FMT" is not a power of 2, " > + "truncating to %u MB", ram_size, size); > + } > + if (sz_log2 < min_log2) { > + error_setg(errp, > + "Memory size is too small for SDRAM type, adjusting type"); > + if (size >= 32) { > + type = DDR; > + min_log2 = 5; > + max_log2 = 12; > + } else { > + type = SDR; > + min_log2 = 2; > + max_log2 = 9; > + } > + } > + > + nbanks = 1; > + while (sz_log2 > max_log2 && nbanks < 8) { > + sz_log2--; > + nbanks++; > + } > + > + if (size > (1ULL << sz_log2) * nbanks) { > + error_setg(errp, "Memory size is too big for SDRAM, truncating"); > + } > + > + /* split to 2 banks if possible to avoid a bug in MIPS Malta firmware */ > + if (nbanks == 1 && sz_log2 > min_log2) { > + sz_log2--; > + nbanks++; > + } > + > + density = 1ULL << (sz_log2 - 2); > + switch (type) { > + case DDR2: > + density = (density & 0xe0) | (density >> 8 & 0x1f); > + break; > + case DDR: > + density = (density & 0xf8) | (density >> 8 & 0x07); > + break; > + case SDR: > + default: > + density &= 0xff; > + break; > + } > + > + spd = g_malloc0(256); I think this buffer is eeprom-dependant, not SPD related. Wouldn't it be cleaner to pass the (previously created) buffer as argument such: /* return true on success */ bool spd_data_fill(void *buf, size_t bufsize, enum sdram_type type, ram_addr_t ram_size, Error **errp); or return something else like ssize_t. > + spd[0] = 128; /* data bytes in EEPROM */ > + spd[1] = 8; /* log2 size of EEPROM */ > + spd[2] = type; > + spd[3] = 13; /* row address bits */ > + spd[4] = 10; /* column address bits */ > + spd[5] = (type == DDR2 ? nbanks - 1 : nbanks); > + spd[6] = 64; /* module data width */ > + /* reserved / data width high */ > + spd[8] = 4; /* interface voltage level */ > + spd[9] = 0x25; /* highest CAS latency */ > + spd[10] = 1; /* access time */ > + /* DIMM configuration 0 = non-ECC */ > + spd[12] = 0x82; /* refresh requirements */ > + spd[13] = 8; /* primary SDRAM width */ > + /* ECC SDRAM width */ > + spd[15] = (type == DDR2 ? 0 : 1); /* reserved / delay for random col rd */ > + spd[16] = 12; /* burst lengths supported */ > + spd[17] = 4; /* banks per SDRAM device */ > + spd[18] = 12; /* ~CAS latencies supported */ > + spd[19] = (type == DDR2 ? 0 : 1); /* reserved / ~CS latencies supported */ > + spd[20] = 2; /* DIMM type / ~WE latencies */ > + /* module features */ > + /* memory chip features */ > + spd[23] = 0x12; /* clock cycle time @ medium CAS latency */ > + /* data access time */ > + /* clock cycle time @ short CAS latency */ > + /* data access time */ > + spd[27] = 20; /* min. row precharge time */ > + spd[28] = 15; /* min. row active row delay */ > + spd[29] = 20; /* min. ~RAS to ~CAS delay */ > + spd[30] = 45; /* min. active to precharge time */ > + spd[31] = density; > + spd[32] = 20; /* addr/cmd setup time */ > + spd[33] = 8; /* addr/cmd hold time */ > + spd[34] = 20; /* data input setup time */ > + spd[35] = 8; /* data input hold time */ > + > + /* checksum */ > + for (i = 0; i < 63; i++) { > + spd[63] += spd[i]; > + } > + return spd; > +} > diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h > index d8b1b9ee81..d3dd0fcb14 100644 > --- a/include/hw/i2c/smbus.h > +++ b/include/hw/i2c/smbus.h > @@ -93,4 +93,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf); > void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, > const uint8_t *eeprom_spd, int size); > > +enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 }; > +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size, Error **errp); > + > #endif >
On Wed, 9 Jan 2019, Philippe Mathieu-Daudé wrote: > On 1/3/19 5:27 PM, BALATON Zoltan wrote: >> There are several boards with SPD EEPROMs that are now using >> duplicated or slightly different hard coded data. Add a helper to >> generate SPD data for a memory module of given type and size that >> could be used by these boards (either as is or with further changes if >> needed) which should help cleaning this up and avoid further duplication. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> v3: Fixed a tab indent >> v2: Added errp parameter to pass errors back to caller >> >> hw/i2c/smbus_eeprom.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++ >> include/hw/i2c/smbus.h | 3 ++ >> 2 files changed, 133 insertions(+) >> >> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c >> index f18aa3de35..bef24a1ca4 100644 >> --- a/hw/i2c/smbus_eeprom.c >> +++ b/hw/i2c/smbus_eeprom.c [...] >> + >> + spd = g_malloc0(256); > > I think this buffer is eeprom-dependant, not SPD related. This function is called spd_data_generate(). It specifically generates SPD EEPROM data and nothing else. as you see below data is hardcoded so would not work for other EEPROMs (the first two bytes even specify EEPROM size, hence I don't think size needs to be passed as a parameter. > Wouldn't it be cleaner to pass the (previously created) buffer as > argument such: > > /* return true on success */ > bool spd_data_fill(void *buf, size_t bufsize, > enum sdram_type type, ram_addr_t ram_size, > Error **errp); It could take a previously created buffer but it's simpler this way and one less error to handle by the caller so I don't like adding two more parameters for this. > or return something else like ssize_t. Again, the current version is simpler IMO so while this aims to be generic to be used by other boards but still not completely generic for all kinds of EEPROMs. Just for SPD EEPROMs commonly found on SDR, DDR and DDR2 memory modules. Anything else (even DDR3) is too dissimilar so those will need another function not this one. Regards, BALATON Zoltan > >> + spd[0] = 128; /* data bytes in EEPROM */ >> + spd[1] = 8; /* log2 size of EEPROM */ >> + spd[2] = type; >> + spd[3] = 13; /* row address bits */ >> + spd[4] = 10; /* column address bits */ >> + spd[5] = (type == DDR2 ? nbanks - 1 : nbanks); >> + spd[6] = 64; /* module data width */ >> + /* reserved / data width high */ >> + spd[8] = 4; /* interface voltage level */ >> + spd[9] = 0x25; /* highest CAS latency */ >> + spd[10] = 1; /* access time */ >> + /* DIMM configuration 0 = non-ECC */ >> + spd[12] = 0x82; /* refresh requirements */ >> + spd[13] = 8; /* primary SDRAM width */ >> + /* ECC SDRAM width */ >> + spd[15] = (type == DDR2 ? 0 : 1); /* reserved / delay for random col rd */ >> + spd[16] = 12; /* burst lengths supported */ >> + spd[17] = 4; /* banks per SDRAM device */ >> + spd[18] = 12; /* ~CAS latencies supported */ >> + spd[19] = (type == DDR2 ? 0 : 1); /* reserved / ~CS latencies supported */ >> + spd[20] = 2; /* DIMM type / ~WE latencies */ >> + /* module features */ >> + /* memory chip features */ >> + spd[23] = 0x12; /* clock cycle time @ medium CAS latency */ >> + /* data access time */ >> + /* clock cycle time @ short CAS latency */ >> + /* data access time */ >> + spd[27] = 20; /* min. row precharge time */ >> + spd[28] = 15; /* min. row active row delay */ >> + spd[29] = 20; /* min. ~RAS to ~CAS delay */ >> + spd[30] = 45; /* min. active to precharge time */ >> + spd[31] = density; >> + spd[32] = 20; /* addr/cmd setup time */ >> + spd[33] = 8; /* addr/cmd hold time */ >> + spd[34] = 20; /* data input setup time */ >> + spd[35] = 8; /* data input hold time */ >> + >> + /* checksum */ >> + for (i = 0; i < 63; i++) { >> + spd[63] += spd[i]; >> + } >> + return spd; >> +} >> diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h >> index d8b1b9ee81..d3dd0fcb14 100644 >> --- a/include/hw/i2c/smbus.h >> +++ b/include/hw/i2c/smbus.h >> @@ -93,4 +93,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf); >> void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, >> const uint8_t *eeprom_spd, int size); >> >> +enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 }; >> +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size, Error **errp); >> + >> #endif >> > >
On 1/9/19 1:15 PM, BALATON Zoltan wrote: > On Wed, 9 Jan 2019, Philippe Mathieu-Daudé wrote: >> On 1/3/19 5:27 PM, BALATON Zoltan wrote: >>> There are several boards with SPD EEPROMs that are now using >>> duplicated or slightly different hard coded data. Add a helper to >>> generate SPD data for a memory module of given type and size that >>> could be used by these boards (either as is or with further changes if >>> needed) which should help cleaning this up and avoid further >>> duplication. >>> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> --- >>> v3: Fixed a tab indent >>> v2: Added errp parameter to pass errors back to caller >>> >>> hw/i2c/smbus_eeprom.c | 130 >>> +++++++++++++++++++++++++++++++++++++++++++++++++ >>> include/hw/i2c/smbus.h | 3 ++ >>> 2 files changed, 133 insertions(+) >>> >>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c >>> index f18aa3de35..bef24a1ca4 100644 >>> --- a/hw/i2c/smbus_eeprom.c >>> +++ b/hw/i2c/smbus_eeprom.c > [...] >>> + >>> + spd = g_malloc0(256); >> >> I think this buffer is eeprom-dependant, not SPD related. > > This function is called spd_data_generate(). It specifically generates > SPD EEPROM data and nothing else. as you see below data is hardcoded so > would not work for other EEPROMs (the first two bytes even specify > EEPROM size, hence I don't think size needs to be passed as a parameter. Well this is why worried me at first, because you alloc 256 bytes ... > >> Wouldn't it be cleaner to pass the (previously created) buffer as >> argument such: >> >> /* return true on success */ >> bool spd_data_fill(void *buf, size_t bufsize, >> enum sdram_type type, ram_addr_t ram_size, >> Error **errp); > > It could take a previously created buffer but it's simpler this way and > one less error to handle by the caller so I don't like adding two more > parameters for this. > >> or return something else like ssize_t. > > Again, the current version is simpler IMO so while this aims to be > generic to be used by other boards but still not completely generic for > all kinds of EEPROMs. Just for SPD EEPROMs commonly found on SDR, DDR > and DDR2 memory modules. Anything else (even DDR3) is too dissimilar so > those will need another function not this one. > > Regards, > BALATON Zoltan > >> >>> + spd[0] = 128; /* data bytes in EEPROM */ ... for a 128 bytes EEPROM. Maybe we can find a compromise at a quick fix with: /* no other size currently supported */ static const size_t spd_eeprom_size = 128; spd = g_malloc0(spd_eeprom_size); ... spd[0] = spd_eeprom_size; spd[1] = 1 + ctzl(spd_eeprom_size); >>> + spd[1] = 8; /* log2 size of EEPROM */ >>> + spd[2] = type; >>> + spd[3] = 13; /* row address bits */ >>> + spd[4] = 10; /* column address bits */ >>> + spd[5] = (type == DDR2 ? nbanks - 1 : nbanks); >>> + spd[6] = 64; /* module data width */ >>> + /* reserved / data width high */ >>> + spd[8] = 4; /* interface voltage level */ >>> + spd[9] = 0x25; /* highest CAS latency */ >>> + spd[10] = 1; /* access time */ >>> + /* DIMM configuration 0 = non-ECC */ >>> + spd[12] = 0x82; /* refresh requirements */ >>> + spd[13] = 8; /* primary SDRAM width */ >>> + /* ECC SDRAM width */ >>> + spd[15] = (type == DDR2 ? 0 : 1); /* reserved / delay for random >>> col rd */ >>> + spd[16] = 12; /* burst lengths supported */ >>> + spd[17] = 4; /* banks per SDRAM device */ >>> + spd[18] = 12; /* ~CAS latencies supported */ >>> + spd[19] = (type == DDR2 ? 0 : 1); /* reserved / ~CS latencies >>> supported */ >>> + spd[20] = 2; /* DIMM type / ~WE latencies */ >>> + /* module features */ >>> + /* memory chip features */ >>> + spd[23] = 0x12; /* clock cycle time @ medium CAS latency */ >>> + /* data access time */ >>> + /* clock cycle time @ short CAS latency */ >>> + /* data access time */ >>> + spd[27] = 20; /* min. row precharge time */ >>> + spd[28] = 15; /* min. row active row delay */ >>> + spd[29] = 20; /* min. ~RAS to ~CAS delay */ >>> + spd[30] = 45; /* min. active to precharge time */ >>> + spd[31] = density; >>> + spd[32] = 20; /* addr/cmd setup time */ >>> + spd[33] = 8; /* addr/cmd hold time */ >>> + spd[34] = 20; /* data input setup time */ >>> + spd[35] = 8; /* data input hold time */ >>> + >>> + /* checksum */ >>> + for (i = 0; i < 63; i++) { >>> + spd[63] += spd[i]; >>> + } >>> + return spd; >>> +} >>> diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h >>> index d8b1b9ee81..d3dd0fcb14 100644 >>> --- a/include/hw/i2c/smbus.h >>> +++ b/include/hw/i2c/smbus.h >>> @@ -93,4 +93,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t >>> address, uint8_t *eeprom_buf); >>> void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, >>> const uint8_t *eeprom_spd, int size); >>> >>> +enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 }; >>> +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size, >>> Error **errp); >>> + >>> #endif >>> >> >>
On Wed, 9 Jan 2019, Philippe Mathieu-Daudé wrote: > On 1/9/19 1:15 PM, BALATON Zoltan wrote: >> On Wed, 9 Jan 2019, Philippe Mathieu-Daudé wrote: >>> On 1/3/19 5:27 PM, BALATON Zoltan wrote: >>>> There are several boards with SPD EEPROMs that are now using >>>> duplicated or slightly different hard coded data. Add a helper to >>>> generate SPD data for a memory module of given type and size that >>>> could be used by these boards (either as is or with further changes if >>>> needed) which should help cleaning this up and avoid further >>>> duplication. >>>> >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> --- >>>> v3: Fixed a tab indent >>>> v2: Added errp parameter to pass errors back to caller >>>> >>>> ?hw/i2c/smbus_eeprom.c? | 130 >>>> +++++++++++++++++++++++++++++++++++++++++++++++++ >>>> ?include/hw/i2c/smbus.h |?? 3 ++ >>>> ?2 files changed, 133 insertions(+) >>>> >>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c >>>> index f18aa3de35..bef24a1ca4 100644 >>>> --- a/hw/i2c/smbus_eeprom.c >>>> +++ b/hw/i2c/smbus_eeprom.c >> [...] >>>> + >>>> +??? spd = g_malloc0(256); >>> >>> I think this buffer is eeprom-dependant, not SPD related. >> >> This function is called spd_data_generate(). It specifically generates >> SPD EEPROM data and nothing else. as you see below data is hardcoded so >> would not work for other EEPROMs (the first two bytes even specify >> EEPROM size, hence I don't think size needs to be passed as a parameter. > > Well this is why worried me at first, because you alloc 256 bytes ... > >> >>> Wouldn't it be cleaner to pass the (previously created) buffer as >>> argument such: >>> >>> ?/* return true on success */ >>> ?bool spd_data_fill(void *buf, size_t bufsize, >>> ??????????????????? enum sdram_type type, ram_addr_t ram_size, >>> ??????????????????? Error **errp); >> >> It could take a previously created buffer but it's simpler this way and >> one less error to handle by the caller so I don't like adding two more >> parameters for this. >> >>> or return something else like ssize_t. >> >> Again, the current version is simpler IMO so while this aims to be >> generic to be used by other boards but still not completely generic for >> all kinds of EEPROMs. Just for SPD EEPROMs commonly found on SDR, DDR >> and DDR2 memory modules. Anything else (even DDR3) is too dissimilar so >> those will need another function not this one. >> >> Regards, >> BALATON Zoltan >> >>> >>>> +??? spd[0] = 128;?? /* data bytes in EEPROM */ > > ... for a 128 bytes EEPROM. No, I allocate 256 bytes for a 256 bytes EEPROM of which the first 128 bytes are containing SPD data as described in for example: https://en.wikipedia.org/wiki/Serial_presence_detect This also matches the previous code that allocated 256 bytes (and still does, see smbus_eeprom_init() function just above this one). > Maybe we can find a compromise at a quick fix with: > > /* no other size currently supported */ > static const size_t spd_eeprom_size = 128; > > spd = g_malloc0(spd_eeprom_size); > ... > > spd[0] = spd_eeprom_size; > spd[1] = 1 + ctzl(spd_eeprom_size); This does not match static SPD data currently in the code elsewhere which all start with 128, 8,... so I'm not sure some firmware would dislike a non-usual eeprom with 128, 4. My intention was to remove static SPD data that's present elsewhere and replace it with nearly identical data generated by this function. The data also has to match what's normally found on real hardware so maybe try dumping data from some memory modules and check what they have and if your suggestion is common then we could go with that otherwise I'd rather waste 128 bytes (or half a kilobyte for 4 modules) than get compatibility problems due to presenting unusual data to firmwares and other guest software that parse SPD data. Unless someone else also thinks it's a good idea to go with unusual SPD data to save a few bytes. Regards, BALATON Zoltan >>>> +??? spd[1] = 8;???? /* log2 size of EEPROM */ >>>> +??? spd[2] = type; >>>> +??? spd[3] = 13;??? /* row address bits */ >>>> +??? spd[4] = 10;??? /* column address bits */ >>>> +??? spd[5] = (type == DDR2 ? nbanks - 1 : nbanks); >>>> +??? spd[6] = 64;??? /* module data width */ >>>> +??????????????????? /* reserved / data width high */ >>>> +??? spd[8] = 4;???? /* interface voltage level */ >>>> +??? spd[9] = 0x25;? /* highest CAS latency */ >>>> +??? spd[10] = 1;??? /* access time */ >>>> +??????????????????? /* DIMM configuration 0 = non-ECC */ >>>> +??? spd[12] = 0x82; /* refresh requirements */ >>>> +??? spd[13] = 8;??? /* primary SDRAM width */ >>>> +??????????????????? /* ECC SDRAM width */ >>>> +??? spd[15] = (type == DDR2 ? 0 : 1); /* reserved / delay for random >>>> col rd */ >>>> +??? spd[16] = 12;?? /* burst lengths supported */ >>>> +??? spd[17] = 4;??? /* banks per SDRAM device */ >>>> +??? spd[18] = 12;?? /* ~CAS latencies supported */ >>>> +??? spd[19] = (type == DDR2 ? 0 : 1); /* reserved / ~CS latencies >>>> supported */ >>>> +??? spd[20] = 2;??? /* DIMM type / ~WE latencies */ >>>> +??????????????????? /* module features */ >>>> +??????????????????? /* memory chip features */ >>>> +??? spd[23] = 0x12; /* clock cycle time @ medium CAS latency */ >>>> +??????????????????? /* data access time */ >>>> +??????????????????? /* clock cycle time @ short CAS latency */ >>>> +??????????????????? /* data access time */ >>>> +??? spd[27] = 20;?? /* min. row precharge time */ >>>> +??? spd[28] = 15;?? /* min. row active row delay */ >>>> +??? spd[29] = 20;?? /* min. ~RAS to ~CAS delay */ >>>> +??? spd[30] = 45;?? /* min. active to precharge time */ >>>> +??? spd[31] = density; >>>> +??? spd[32] = 20;?? /* addr/cmd setup time */ >>>> +??? spd[33] = 8;??? /* addr/cmd hold time */ >>>> +??? spd[34] = 20;?? /* data input setup time */ >>>> +??? spd[35] = 8;??? /* data input hold time */ >>>> + >>>> +??? /* checksum */ >>>> +??? for (i = 0; i < 63; i++) { >>>> +??????? spd[63] += spd[i]; >>>> +??? } >>>> +??? return spd; >>>> +} >>>> diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h >>>> index d8b1b9ee81..d3dd0fcb14 100644 >>>> --- a/include/hw/i2c/smbus.h >>>> +++ b/include/hw/i2c/smbus.h >>>> @@ -93,4 +93,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t >>>> address, uint8_t *eeprom_buf); >>>> ?void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, >>>> ??????????????????????? const uint8_t *eeprom_spd, int size); >>>> >>>> +enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 }; >>>> +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size, >>>> Error **errp); >>>> + >>>> ?#endif >>>> >>> >>> > >
On Wed, 9 Jan 2019, BALATON Zoltan wrote: > On Wed, 9 Jan 2019, Philippe Mathieu-Daudé wrote: >> On 1/9/19 1:15 PM, BALATON Zoltan wrote: >>> On Wed, 9 Jan 2019, Philippe Mathieu-Daudé wrote: >>>> On 1/3/19 5:27 PM, BALATON Zoltan wrote: >>>>> There are several boards with SPD EEPROMs that are now using >>>>> duplicated or slightly different hard coded data. Add a helper to >>>>> generate SPD data for a memory module of given type and size that >>>>> could be used by these boards (either as is or with further changes if >>>>> needed) which should help cleaning this up and avoid further >>>>> duplication. >>>>> >>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>>> --- >>>>> v3: Fixed a tab indent >>>>> v2: Added errp parameter to pass errors back to caller >>>>> >>>>> ?hw/i2c/smbus_eeprom.c? | 130 >>>>> +++++++++++++++++++++++++++++++++++++++++++++++++ >>>>> ?include/hw/i2c/smbus.h |?? 3 ++ >>>>> ?2 files changed, 133 insertions(+) >>>>> >>>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c >>>>> index f18aa3de35..bef24a1ca4 100644 >>>>> --- a/hw/i2c/smbus_eeprom.c >>>>> +++ b/hw/i2c/smbus_eeprom.c >>> [...] >>>>> + >>>>> +??? spd = g_malloc0(256); >>>> >>>> I think this buffer is eeprom-dependant, not SPD related. >>> >>> This function is called spd_data_generate(). It specifically generates >>> SPD EEPROM data and nothing else. as you see below data is hardcoded so >>> would not work for other EEPROMs (the first two bytes even specify >>> EEPROM size, hence I don't think size needs to be passed as a parameter. >> >> Well this is why worried me at first, because you alloc 256 bytes ... >> >>> >>>> Wouldn't it be cleaner to pass the (previously created) buffer as >>>> argument such: >>>> >>>> ?/* return true on success */ >>>> ?bool spd_data_fill(void *buf, size_t bufsize, >>>> ??????????????????? enum sdram_type type, ram_addr_t ram_size, >>>> ??????????????????? Error **errp); >>> >>> It could take a previously created buffer but it's simpler this way and >>> one less error to handle by the caller so I don't like adding two more >>> parameters for this. >>> >>>> or return something else like ssize_t. >>> >>> Again, the current version is simpler IMO so while this aims to be >>> generic to be used by other boards but still not completely generic for >>> all kinds of EEPROMs. Just for SPD EEPROMs commonly found on SDR, DDR >>> and DDR2 memory modules. Anything else (even DDR3) is too dissimilar so >>> those will need another function not this one. >>> >>> Regards, >>> BALATON Zoltan >>> >>>> >>>>> +??? spd[0] = 128;?? /* data bytes in EEPROM */ >> >> ... for a 128 bytes EEPROM. > > No, I allocate 256 bytes for a 256 bytes EEPROM of which the first 128 bytes > are containing SPD data as described in for example: > > https://en.wikipedia.org/wiki/Serial_presence_detect > > This also matches the previous code that allocated 256 bytes (and still does, > see smbus_eeprom_init() function just above this one). > >> Maybe we can find a compromise at a quick fix with: >> >> /* no other size currently supported */ >> static const size_t spd_eeprom_size = 128; >> >> spd = g_malloc0(spd_eeprom_size); >> ... >> >> spd[0] = spd_eeprom_size; >> spd[1] = 1 + ctzl(spd_eeprom_size); > > This does not match static SPD data currently in the code elsewhere which all > start with 128, 8,... so I'm not sure some firmware would dislike a non-usual > eeprom with 128, 4. My intention was to remove static SPD data that's present > elsewhere and replace it with nearly identical data generated by this > function. The data also has to match what's normally found on real hardware > so maybe try dumping data from some memory modules and check what they have > and if your suggestion is common then we could go with that otherwise I'd > rather waste 128 bytes (or half a kilobyte for 4 modules) than get > compatibility problems due to presenting unusual data to firmwares and other > guest software that parse SPD data. > > Unless someone else also thinks it's a good idea to go with unusual SPD data > to save a few bytes. Even then it would not work. Whole smbus_eeprom.c seems to have EEPROM size == 256 hardcoded all over the place, so... > Regards, > BALATON Zoltan > >>>>> +??? spd[1] = 8;???? /* log2 size of EEPROM */ >>>>> +??? spd[2] = type; >>>>> +??? spd[3] = 13;??? /* row address bits */ >>>>> +??? spd[4] = 10;??? /* column address bits */ >>>>> +??? spd[5] = (type == DDR2 ? nbanks - 1 : nbanks); >>>>> +??? spd[6] = 64;??? /* module data width */ >>>>> +??????????????????? /* reserved / data width high */ >>>>> +??? spd[8] = 4;???? /* interface voltage level */ >>>>> +??? spd[9] = 0x25;? /* highest CAS latency */ >>>>> +??? spd[10] = 1;??? /* access time */ >>>>> +??????????????????? /* DIMM configuration 0 = non-ECC */ >>>>> +??? spd[12] = 0x82; /* refresh requirements */ >>>>> +??? spd[13] = 8;??? /* primary SDRAM width */ >>>>> +??????????????????? /* ECC SDRAM width */ >>>>> +??? spd[15] = (type == DDR2 ? 0 : 1); /* reserved / delay for random >>>>> col rd */ >>>>> +??? spd[16] = 12;?? /* burst lengths supported */ >>>>> +??? spd[17] = 4;??? /* banks per SDRAM device */ >>>>> +??? spd[18] = 12;?? /* ~CAS latencies supported */ >>>>> +??? spd[19] = (type == DDR2 ? 0 : 1); /* reserved / ~CS latencies >>>>> supported */ >>>>> +??? spd[20] = 2;??? /* DIMM type / ~WE latencies */ >>>>> +??????????????????? /* module features */ >>>>> +??????????????????? /* memory chip features */ >>>>> +??? spd[23] = 0x12; /* clock cycle time @ medium CAS latency */ >>>>> +??????????????????? /* data access time */ >>>>> +??????????????????? /* clock cycle time @ short CAS latency */ >>>>> +??????????????????? /* data access time */ >>>>> +??? spd[27] = 20;?? /* min. row precharge time */ >>>>> +??? spd[28] = 15;?? /* min. row active row delay */ >>>>> +??? spd[29] = 20;?? /* min. ~RAS to ~CAS delay */ >>>>> +??? spd[30] = 45;?? /* min. active to precharge time */ >>>>> +??? spd[31] = density; >>>>> +??? spd[32] = 20;?? /* addr/cmd setup time */ >>>>> +??? spd[33] = 8;??? /* addr/cmd hold time */ >>>>> +??? spd[34] = 20;?? /* data input setup time */ >>>>> +??? spd[35] = 8;??? /* data input hold time */ >>>>> + >>>>> +??? /* checksum */ >>>>> +??? for (i = 0; i < 63; i++) { >>>>> +??????? spd[63] += spd[i]; >>>>> +??? } >>>>> +??? return spd; >>>>> +} >>>>> diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h >>>>> index d8b1b9ee81..d3dd0fcb14 100644 >>>>> --- a/include/hw/i2c/smbus.h >>>>> +++ b/include/hw/i2c/smbus.h >>>>> @@ -93,4 +93,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t >>>>> address, uint8_t *eeprom_buf); >>>>> ?void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, >>>>> ??????????????????????? const uint8_t *eeprom_spd, int size); >>>>> >>>>> +enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 }; >>>>> +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size, >>>>> Error **errp); >>>>> + >>>>> ?#endif >>>>> >>>> >>>> >> >
On 1/9/19 7:05 PM, BALATON Zoltan wrote: > On Wed, 9 Jan 2019, BALATON Zoltan wrote: >> On Wed, 9 Jan 2019, Philippe Mathieu-Daudé wrote: >>> On 1/9/19 1:15 PM, BALATON Zoltan wrote: >>>> On Wed, 9 Jan 2019, Philippe Mathieu-Daudé wrote: >>>>> On 1/3/19 5:27 PM, BALATON Zoltan wrote: >>>>>> There are several boards with SPD EEPROMs that are now using >>>>>> duplicated or slightly different hard coded data. Add a helper to >>>>>> generate SPD data for a memory module of given type and size that >>>>>> could be used by these boards (either as is or with further >>>>>> changes if >>>>>> needed) which should help cleaning this up and avoid further >>>>>> duplication. >>>>>> >>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>>>> --- >>>>>> v3: Fixed a tab indent >>>>>> v2: Added errp parameter to pass errors back to caller >>>>>> >>>>>> ?hw/i2c/smbus_eeprom.c? | 130 >>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> ?include/hw/i2c/smbus.h |?? 3 ++ >>>>>> ?2 files changed, 133 insertions(+) >>>>>> >>>>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c >>>>>> index f18aa3de35..bef24a1ca4 100644 >>>>>> --- a/hw/i2c/smbus_eeprom.c >>>>>> +++ b/hw/i2c/smbus_eeprom.c >>>> [...] >>>>>> + >>>>>> +??? spd = g_malloc0(256); >>>>> >>>>> I think this buffer is eeprom-dependant, not SPD related. >>>> >>>> This function is called spd_data_generate(). It specifically generates >>>> SPD EEPROM data and nothing else. as you see below data is hardcoded so >>>> would not work for other EEPROMs (the first two bytes even specify >>>> EEPROM size, hence I don't think size needs to be passed as a >>>> parameter. >>> >>> Well this is why worried me at first, because you alloc 256 bytes ... >>> >>>> >>>>> Wouldn't it be cleaner to pass the (previously created) buffer as >>>>> argument such: >>>>> >>>>> ?/* return true on success */ >>>>> ?bool spd_data_fill(void *buf, size_t bufsize, >>>>> ??????????????????? enum sdram_type type, ram_addr_t ram_size, >>>>> ??????????????????? Error **errp); >>>> >>>> It could take a previously created buffer but it's simpler this way and >>>> one less error to handle by the caller so I don't like adding two more >>>> parameters for this. >>>> >>>>> or return something else like ssize_t. >>>> >>>> Again, the current version is simpler IMO so while this aims to be >>>> generic to be used by other boards but still not completely generic for >>>> all kinds of EEPROMs. Just for SPD EEPROMs commonly found on SDR, DDR >>>> and DDR2 memory modules. Anything else (even DDR3) is too dissimilar so >>>> those will need another function not this one. >>>> >>>> Regards, >>>> BALATON Zoltan >>>> >>>>> >>>>>> +??? spd[0] = 128;?? /* data bytes in EEPROM */ >>> >>> ... for a 128 bytes EEPROM. >> >> No, I allocate 256 bytes for a 256 bytes EEPROM of which the first 128 >> bytes are containing SPD data as described in for example: >> >> https://en.wikipedia.org/wiki/Serial_presence_detect >> >> This also matches the previous code that allocated 256 bytes (and >> still does, see smbus_eeprom_init() function just above this one). >> >>> Maybe we can find a compromise at a quick fix with: >>> >>> /* no other size currently supported */ >>> static const size_t spd_eeprom_size = 128; >>> >>> spd = g_malloc0(spd_eeprom_size); >>> ... >>> >>> spd[0] = spd_eeprom_size; >>> spd[1] = 1 + ctzl(spd_eeprom_size); >> >> This does not match static SPD data currently in the code elsewhere >> which all start with 128, 8,... so I'm not sure some firmware would >> dislike a non-usual eeprom with 128, 4. My intention was to remove >> static SPD data that's present elsewhere and replace it with nearly >> identical data generated by this function. The data also has to match >> what's normally found on real hardware so maybe try dumping data from >> some memory modules and check what they have and if your suggestion is >> common then we could go with that otherwise I'd rather waste 128 bytes >> (or half a kilobyte for 4 modules) than get compatibility problems due >> to presenting unusual data to firmwares and other guest software that >> parse SPD data. >> >> Unless someone else also thinks it's a good idea to go with unusual >> SPD data to save a few bytes. > > Even then it would not work. Whole smbus_eeprom.c seems to have EEPROM > size == 256 hardcoded all over the place, so... Yes, this 'device' needs love^H^H^H^Hcleanup. Thanks for the info you provided. Regards, Phil.
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c index f18aa3de35..bef24a1ca4 100644 --- a/hw/i2c/smbus_eeprom.c +++ b/hw/i2c/smbus_eeprom.c @@ -23,6 +23,9 @@ */ #include "qemu/osdep.h" +#include "qemu/error-report.h" +#include "qemu/units.h" +#include "qapi/error.h" #include "hw/hw.h" #include "hw/i2c/i2c.h" #include "hw/i2c/smbus.h" @@ -162,3 +165,130 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, smbus_eeprom_init_one(smbus, 0x50 + i, eeprom_buf + (i * 256)); } } + +/* Generate SDRAM SPD EEPROM data describing a module of type and size */ +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size, + Error **errp) +{ + uint8_t *spd; + uint8_t nbanks; + uint16_t density; + uint32_t size; + int min_log2, max_log2, sz_log2; + int i; + + switch (type) { + case SDR: + min_log2 = 2; + max_log2 = 9; + break; + case DDR: + min_log2 = 5; + max_log2 = 12; + break; + case DDR2: + min_log2 = 7; + max_log2 = 14; + break; + default: + g_assert_not_reached(); + } + size = ram_size >> 20; /* work in terms of megabytes */ + if (size < 4) { + error_setg(errp, "SDRAM size is too small"); + return NULL; + } + sz_log2 = 31 - clz32(size); + size = 1U << sz_log2; + if (ram_size > size * MiB) { + error_setg(errp, "SDRAM size 0x"RAM_ADDR_FMT" is not a power of 2, " + "truncating to %u MB", ram_size, size); + } + if (sz_log2 < min_log2) { + error_setg(errp, + "Memory size is too small for SDRAM type, adjusting type"); + if (size >= 32) { + type = DDR; + min_log2 = 5; + max_log2 = 12; + } else { + type = SDR; + min_log2 = 2; + max_log2 = 9; + } + } + + nbanks = 1; + while (sz_log2 > max_log2 && nbanks < 8) { + sz_log2--; + nbanks++; + } + + if (size > (1ULL << sz_log2) * nbanks) { + error_setg(errp, "Memory size is too big for SDRAM, truncating"); + } + + /* split to 2 banks if possible to avoid a bug in MIPS Malta firmware */ + if (nbanks == 1 && sz_log2 > min_log2) { + sz_log2--; + nbanks++; + } + + density = 1ULL << (sz_log2 - 2); + switch (type) { + case DDR2: + density = (density & 0xe0) | (density >> 8 & 0x1f); + break; + case DDR: + density = (density & 0xf8) | (density >> 8 & 0x07); + break; + case SDR: + default: + density &= 0xff; + break; + } + + spd = g_malloc0(256); + spd[0] = 128; /* data bytes in EEPROM */ + spd[1] = 8; /* log2 size of EEPROM */ + spd[2] = type; + spd[3] = 13; /* row address bits */ + spd[4] = 10; /* column address bits */ + spd[5] = (type == DDR2 ? nbanks - 1 : nbanks); + spd[6] = 64; /* module data width */ + /* reserved / data width high */ + spd[8] = 4; /* interface voltage level */ + spd[9] = 0x25; /* highest CAS latency */ + spd[10] = 1; /* access time */ + /* DIMM configuration 0 = non-ECC */ + spd[12] = 0x82; /* refresh requirements */ + spd[13] = 8; /* primary SDRAM width */ + /* ECC SDRAM width */ + spd[15] = (type == DDR2 ? 0 : 1); /* reserved / delay for random col rd */ + spd[16] = 12; /* burst lengths supported */ + spd[17] = 4; /* banks per SDRAM device */ + spd[18] = 12; /* ~CAS latencies supported */ + spd[19] = (type == DDR2 ? 0 : 1); /* reserved / ~CS latencies supported */ + spd[20] = 2; /* DIMM type / ~WE latencies */ + /* module features */ + /* memory chip features */ + spd[23] = 0x12; /* clock cycle time @ medium CAS latency */ + /* data access time */ + /* clock cycle time @ short CAS latency */ + /* data access time */ + spd[27] = 20; /* min. row precharge time */ + spd[28] = 15; /* min. row active row delay */ + spd[29] = 20; /* min. ~RAS to ~CAS delay */ + spd[30] = 45; /* min. active to precharge time */ + spd[31] = density; + spd[32] = 20; /* addr/cmd setup time */ + spd[33] = 8; /* addr/cmd hold time */ + spd[34] = 20; /* data input setup time */ + spd[35] = 8; /* data input hold time */ + + /* checksum */ + for (i = 0; i < 63; i++) { + spd[63] += spd[i]; + } + return spd; +} diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h index d8b1b9ee81..d3dd0fcb14 100644 --- a/include/hw/i2c/smbus.h +++ b/include/hw/i2c/smbus.h @@ -93,4 +93,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf); void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, const uint8_t *eeprom_spd, int size); +enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 }; +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size, Error **errp); + #endif
There are several boards with SPD EEPROMs that are now using duplicated or slightly different hard coded data. Add a helper to generate SPD data for a memory module of given type and size that could be used by these boards (either as is or with further changes if needed) which should help cleaning this up and avoid further duplication. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- v3: Fixed a tab indent v2: Added errp parameter to pass errors back to caller hw/i2c/smbus_eeprom.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++ include/hw/i2c/smbus.h | 3 ++ 2 files changed, 133 insertions(+)