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

[BALATON Zoltan]

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.

> 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