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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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
>