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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6830 bytes --]

On Wed, Jan 02, 2019 at 03:06:38AM +0100, 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>
> ---
>  hw/i2c/smbus_eeprom.c  | 128 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/i2c/smbus.h |   3 ++
>  2 files changed, 131 insertions(+)
> 
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index f18aa3de35..a1f51eb921 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -23,6 +23,8 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> +#include "qemu/units.h"
>  #include "hw/hw.h"
>  #include "hw/i2c/i2c.h"
>  #include "hw/i2c/smbus.h"
> @@ -162,3 +164,129 @@ 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)
> +{
> +    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:
> +        error_report("Unknown SDRAM type");
> +        abort();

The error handling might work a little cleaner if you give this
function an Error ** parameter, then just pass in &error_abort from
the callers.

> +    }
> +    size = ram_size >> 20; /* work in terms of megabytes */
> +    if (size < 4) {
> +        error_report("SDRAM size is too small");
> +        return NULL;
> +    }
> +    sz_log2 = 31 - clz32(size);
> +    size = 1U << sz_log2;
> +    if (ram_size > size * MiB) {
> +        warn_report("SDRAM size 0x"RAM_ADDR_FMT" is not a power of 2, "
> +                    "truncating to %u MB", ram_size, size);
> +    }
> +    if (sz_log2 < min_log2) {
> +        warn_report("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;
> +        }

What do these various fall back cases represent?  Are they bugs in the
callers, or a user configuration error?

If the first, we should just assert or abort.  If the second I think
we should still die with a fatal error - allowing broken
configurations to work with just a warning is kind of begging to
maintain nasty compatiliby cruft in the future.

> +    }
> +
> +    nbanks = 1;
> +    while (sz_log2 > max_log2 && nbanks < 8) {
> +        sz_log2--;
> +        nbanks++;
> +    }
> +
> +    if (size > (1ULL << sz_log2) * nbanks) {
> +        warn_report("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..0adc2991b5 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);
> +
>  #endif

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]