Re: [Qemu-ppc] [Qemu-devel] [PATCH v3 1/6] smbus: Add a helper to generate SPD EEPROM data

[BALATON Zoltan]

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 <address@hidden>
> > > > > ---
> > > > > 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