[Qemu-devel] [PATCH v3 6/6] sam460ex: Fix support for memory larger than 1GB

[Qemu-devel] [PATCH v3 6/6] sam460ex: Fix support for memory larger than 1GB

[BALATON Zoltan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3400 bytes --]

Fix the encoding of larger memory modules in the SoC registers which
allows specifying more than 1GB memory for sam460ex. Well, only 2GB
due to SoC and firmware restrictions which was the only missing value
compared to what the real hardware supports. The SoC should support up
to 4GB but when setting that the firmware hangs during memory test.
This may be an overflow bug in the firmware which I did not try to
debug but this may affect real hardware as well.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v2: lower case hex constants to match others around it

 hw/ppc/ppc440_uc.c | 22 ++++++++++++----------
 hw/ppc/sam460ex.c  |  6 ++++--
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index 60dbb35eee..c489368905 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -2,7 +2,7 @@
  * QEMU PowerPC 440 embedded processors emulation
  *
  * Copyright (c) 2012 François Revol
- * Copyright (c) 2016-2018 BALATON Zoltan
+ * Copyright (c) 2016-2019 BALATON Zoltan
  *
  * This work is licensed under the GNU GPL license version 2 or later.
  *
@@ -505,10 +505,6 @@ enum {
     SDRAM_PLBADDUHB = 0x50,
 };
 
-/* XXX: TOFIX: some patches have made this code become inconsistent:
- *      there are type inconsistencies, mixing hwaddr, target_ulong
- *      and uint32_t
- */
 static uint32_t sdram_bcr(hwaddr ram_base, hwaddr ram_size)
 {
     uint32_t bcr;
@@ -538,11 +534,17 @@ static uint32_t sdram_bcr(hwaddr ram_base, hwaddr ram_size)
     case (1 * GiB):
         bcr = 0xe000;
         break;
+    case (2 * GiB):
+        bcr = 0xc000;
+        break;
+    case (4 * GiB):
+        bcr = 0x8000;
+        break;
     default:
         error_report("invalid RAM size " TARGET_FMT_plx, ram_size);
         return 0;
     }
-    bcr |= ram_base & 0xFF800000;
+    bcr |= ram_base >> 2 & 0xffe00000;
     bcr |= 1;
 
     return bcr;
@@ -550,12 +552,12 @@ static uint32_t sdram_bcr(hwaddr ram_base, hwaddr ram_size)
 
 static inline hwaddr sdram_base(uint32_t bcr)
 {
-    return bcr & 0xFF800000;
+    return (bcr & 0xffe00000) << 2;
 }
 
-static target_ulong sdram_size(uint32_t bcr)
+static uint64_t sdram_size(uint32_t bcr)
 {
-    target_ulong size;
+    uint64_t size;
     int sh;
 
     sh = 1024 - ((bcr >> 6) & 0x3ff);
@@ -575,7 +577,7 @@ static void sdram_set_bcr(ppc440_sdram_t *sdram, int i,
                                     &sdram->ram_memories[i]);
         object_unparent(OBJECT(&sdram->containers[i]));
     }
-    sdram->bcr[i] = bcr & 0xFFDEE001;
+    sdram->bcr[i] = bcr & 0xffe0ffc1;
     if (enabled && (bcr & 1)) {
         memory_region_init(&sdram->containers[i], NULL, "sdram-containers",
                            sdram_size(bcr));
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 233f81bb56..ac575be813 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -76,9 +76,11 @@
 #define UART_FREQ 11059200
 #define SDRAM_NR_BANKS 4
 
-/* FIXME: See u-boot.git 8ac41e, also fix in ppc440_uc.c */
+/* The SoC could also handle 4 GiB but firmware does not work with that. */
+/* Maybe it overflows a signed 32 bit number somewhere? */
 static const ram_addr_t ppc460ex_sdram_bank_sizes[] = {
-    1 * GiB, 512 * MiB, 256 * MiB, 128 * MiB, 64 * MiB, 32 * MiB, 0
+    2 * GiB, 1 * GiB, 512 * MiB, 256 * MiB, 128 * MiB, 64 * MiB,
+    32 * MiB, 0
 };
 
 struct boot_info {
-- 
2.13.7

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

[BALATON Zoltan]

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);
+    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
-- 
2.13.7

[Qemu-devel] [PATCH v3 0/6] Misc sam460ex related patches

[BALATON Zoltan]

The last code patch in this series fixes memory size larger than 1GB
for sam460ex, other patches are just clean ups I've made along the way.

The first patch is intended to be generic and may be useful for other
boards which currently have their own SPD EEPROM data or don't yet
generate any SPD data just have TODO comments instead. These are MIPS
malta and fulong2e, ARM integratorcp and maybe aspeed, and the PIIX
and Q35 pc machines. I did not try to change these as I have no way to
test them throughly. Patch 2 converts sam460ex to use this function.
Other patches are misc cleanups.

Regards,
BALATON Zoltan

v3: A tab indent got in by accident hence v3 but also include smilar
    cleanup for sdram_set_bcd() in another file
v2: Address review comments, omitting patches already merged

BALATON Zoltan (6):
  smbus: Add a helper to generate SPD EEPROM data
  sam460ex: Clean up SPD EEPROM creation
  ppc4xx: Use ram_addr_t in ppc4xx_sdram_adjust()
  ppc4xx: Rename ppc4xx_sdram_t in ppc440_uc.c to ppc440_sdram_t
  ppc4xx: Pass array index to function instead of pointer into the array
  sam460ex: Fix support for memory larger than 1GB

 hw/i2c/smbus_eeprom.c   | 130 ++++++++++++++++++++++++++++++++++
 hw/ppc/ppc440_bamboo.c  |   2 +-
 hw/ppc/ppc440_uc.c      |  70 +++++++++----------
 hw/ppc/ppc4xx_devs.c    |  48 ++++++-------
 hw/ppc/sam460ex.c       | 181 +++++++-----------------------------------------
 include/hw/i2c/smbus.h  |   3 +
 include/hw/ppc/ppc4xx.h |   2 +-
 7 files changed, 216 insertions(+), 220 deletions(-)

-- 
2.13.7

[Qemu-devel] [PATCH v3 3/6] ppc4xx: Use ram_addr_t in ppc4xx_sdram_adjust()

[BALATON Zoltan]

To avoid overflow if larger values are added later use ram_addr_t for
the sdram_bank_sizes parameter to match ram_size to which it is compared.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v2: No change. Review comment suggested ram_size should be hwaddr too
    but it's ram_addr_t in MachineState now so this should do until
    someone decides to clean that up in a separate patch.

 hw/ppc/ppc440_bamboo.c  | 2 +-
 hw/ppc/ppc4xx_devs.c    | 4 ++--
 hw/ppc/sam460ex.c       | 2 +-
 include/hw/ppc/ppc4xx.h | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index b8aa55d526..8277c0f784 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -49,7 +49,7 @@
 
 #define PPC440EP_SDRAM_NR_BANKS 4
 
-static const unsigned int ppc440ep_sdram_bank_sizes[] = {
+static const ram_addr_t ppc440ep_sdram_bank_sizes[] = {
     256 * MiB, 128 * MiB, 64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 0
 };
 
diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index 9b6e4c60fa..9418478575 100644
--- a/hw/ppc/ppc4xx_devs.c
+++ b/hw/ppc/ppc4xx_devs.c
@@ -679,12 +679,12 @@ ram_addr_t ppc4xx_sdram_adjust(ram_addr_t ram_size, int nr_banks,
                                MemoryRegion ram_memories[],
                                hwaddr ram_bases[],
                                hwaddr ram_sizes[],
-                               const unsigned int sdram_bank_sizes[])
+                               const ram_addr_t sdram_bank_sizes[])
 {
     MemoryRegion *ram = g_malloc0(sizeof(*ram));
     ram_addr_t size_left = ram_size;
     ram_addr_t base = 0;
-    unsigned int bank_size;
+    ram_addr_t bank_size;
     int i;
     int j;
 
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index b99aed26e0..233f81bb56 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -77,7 +77,7 @@
 #define SDRAM_NR_BANKS 4
 
 /* FIXME: See u-boot.git 8ac41e, also fix in ppc440_uc.c */
-static const unsigned int ppc460ex_sdram_bank_sizes[] = {
+static const ram_addr_t ppc460ex_sdram_bank_sizes[] = {
     1 * GiB, 512 * MiB, 256 * MiB, 128 * MiB, 64 * MiB, 32 * MiB, 0
 };
 
diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
index 3a2a04c8ce..39a7ba1ce6 100644
--- a/include/hw/ppc/ppc4xx.h
+++ b/include/hw/ppc/ppc4xx.h
@@ -43,7 +43,7 @@ ram_addr_t ppc4xx_sdram_adjust(ram_addr_t ram_size, int nr_banks,
                                MemoryRegion ram_memories[],
                                hwaddr ram_bases[],
                                hwaddr ram_sizes[],
-                               const unsigned int sdram_bank_sizes[]);
+                               const ram_addr_t sdram_bank_sizes[]);
 
 void ppc4xx_sdram_init (CPUPPCState *env, qemu_irq irq, int nbanks,
                         MemoryRegion ram_memories[],
-- 
2.13.7

[Qemu-devel] [PATCH v3 5/6] ppc4xx: Pass array index to function instead of pointer into the array

[BALATON Zoltan]

The sdram_set_bcr() function in ppc440_uc.c takes a pointer into an
array then calculates its index from that. It's simpler and easier to
just pass the index which simplifies both the function and its callers.
Do similar cleanup in ppc4xx_devs.c to similar function.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v3: Also do similar cleanup in ppc4xx_devs.c

 hw/ppc/ppc440_uc.c   | 36 ++++++++++++++++--------------------
 hw/ppc/ppc4xx_devs.c | 44 ++++++++++++++++++++------------------------
 2 files changed, 36 insertions(+), 44 deletions(-)

diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index e46f59fba8..60dbb35eee 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -564,28 +564,26 @@ static target_ulong sdram_size(uint32_t bcr)
     return size;
 }
 
-static void sdram_set_bcr(ppc440_sdram_t *sdram,
-                          uint32_t *bcrp, uint32_t bcr, int enabled)
+static void sdram_set_bcr(ppc440_sdram_t *sdram, int i,
+                          uint32_t bcr, int enabled)
 {
-    unsigned n = bcrp - sdram->bcr;
-
-    if (*bcrp & 1) {
-        /* Unmap RAM */
+    if (sdram->bcr[i] & 1) {
+        /* First unmap RAM if enabled */
         memory_region_del_subregion(get_system_memory(),
-                                    &sdram->containers[n]);
-        memory_region_del_subregion(&sdram->containers[n],
-                                    &sdram->ram_memories[n]);
-        object_unparent(OBJECT(&sdram->containers[n]));
+                                    &sdram->containers[i]);
+        memory_region_del_subregion(&sdram->containers[i],
+                                    &sdram->ram_memories[i]);
+        object_unparent(OBJECT(&sdram->containers[i]));
     }
-    *bcrp = bcr & 0xFFDEE001;
+    sdram->bcr[i] = bcr & 0xFFDEE001;
     if (enabled && (bcr & 1)) {
-        memory_region_init(&sdram->containers[n], NULL, "sdram-containers",
+        memory_region_init(&sdram->containers[i], NULL, "sdram-containers",
                            sdram_size(bcr));
-        memory_region_add_subregion(&sdram->containers[n], 0,
-                                    &sdram->ram_memories[n]);
+        memory_region_add_subregion(&sdram->containers[i], 0,
+                                    &sdram->ram_memories[i]);
         memory_region_add_subregion(get_system_memory(),
                                     sdram_base(bcr),
-                                    &sdram->containers[n]);
+                                    &sdram->containers[i]);
     }
 }
 
@@ -595,12 +593,10 @@ static void sdram_map_bcr(ppc440_sdram_t *sdram)
 
     for (i = 0; i < sdram->nbanks; i++) {
         if (sdram->ram_sizes[i] != 0) {
-            sdram_set_bcr(sdram,
-                          &sdram->bcr[i],
-                          sdram_bcr(sdram->ram_bases[i], sdram->ram_sizes[i]),
-                          1);
+            sdram_set_bcr(sdram, i, sdram_bcr(sdram->ram_bases[i],
+                                              sdram->ram_sizes[i]), 1);
         } else {
-            sdram_set_bcr(sdram, &sdram->bcr[i], 0, 0);
+            sdram_set_bcr(sdram, i, 0, 0);
         }
     }
 }
diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index 9418478575..fdfeb67e65 100644
--- a/hw/ppc/ppc4xx_devs.c
+++ b/hw/ppc/ppc4xx_devs.c
@@ -405,36 +405,34 @@ static target_ulong sdram_size (uint32_t bcr)
     return size;
 }
 
-static void sdram_set_bcr(ppc4xx_sdram_t *sdram,
-                          uint32_t *bcrp, uint32_t bcr, int enabled)
+static void sdram_set_bcr(ppc4xx_sdram_t *sdram, int i,
+                          uint32_t bcr, int enabled)
 {
-    unsigned n = bcrp - sdram->bcr;
-
-    if (*bcrp & 0x00000001) {
+    if (sdram->bcr[i] & 0x00000001) {
         /* Unmap RAM */
 #ifdef DEBUG_SDRAM
         printf("%s: unmap RAM area " TARGET_FMT_plx " " TARGET_FMT_lx "\n",
-               __func__, sdram_base(*bcrp), sdram_size(*bcrp));
+               __func__, sdram_base(sdram->bcr[i]), sdram_size(sdram->bcr[i]));
 #endif
         memory_region_del_subregion(get_system_memory(),
-                                    &sdram->containers[n]);
-        memory_region_del_subregion(&sdram->containers[n],
-                                    &sdram->ram_memories[n]);
-        object_unparent(OBJECT(&sdram->containers[n]));
+                                    &sdram->containers[i]);
+        memory_region_del_subregion(&sdram->containers[i],
+                                    &sdram->ram_memories[i]);
+        object_unparent(OBJECT(&sdram->containers[i]));
     }
-    *bcrp = bcr & 0xFFDEE001;
+    sdram->bcr[i] = bcr & 0xFFDEE001;
     if (enabled && (bcr & 0x00000001)) {
 #ifdef DEBUG_SDRAM
         printf("%s: Map RAM area " TARGET_FMT_plx " " TARGET_FMT_lx "\n",
                __func__, sdram_base(bcr), sdram_size(bcr));
 #endif
-        memory_region_init(&sdram->containers[n], NULL, "sdram-containers",
+        memory_region_init(&sdram->containers[i], NULL, "sdram-containers",
                            sdram_size(bcr));
-        memory_region_add_subregion(&sdram->containers[n], 0,
-                                    &sdram->ram_memories[n]);
+        memory_region_add_subregion(&sdram->containers[i], 0,
+                                    &sdram->ram_memories[i]);
         memory_region_add_subregion(get_system_memory(),
                                     sdram_base(bcr),
-                                    &sdram->containers[n]);
+                                    &sdram->containers[i]);
     }
 }
 
@@ -444,12 +442,10 @@ static void sdram_map_bcr (ppc4xx_sdram_t *sdram)
 
     for (i = 0; i < sdram->nbanks; i++) {
         if (sdram->ram_sizes[i] != 0) {
-            sdram_set_bcr(sdram,
-                          &sdram->bcr[i],
-                          sdram_bcr(sdram->ram_bases[i], sdram->ram_sizes[i]),
-                          1);
+            sdram_set_bcr(sdram, i, sdram_bcr(sdram->ram_bases[i],
+                                              sdram->ram_sizes[i]), 1);
         } else {
-            sdram_set_bcr(sdram, &sdram->bcr[i], 0x00000000, 0);
+            sdram_set_bcr(sdram, i, 0x00000000, 0);
         }
     }
 }
@@ -589,16 +585,16 @@ static void dcr_write_sdram (void *opaque, int dcrn, uint32_t val)
             sdram->pmit = (val & 0xF8000000) | 0x07C00000;
             break;
         case 0x40: /* SDRAM_B0CR */
-            sdram_set_bcr(sdram, &sdram->bcr[0], val, sdram->cfg & 0x80000000);
+            sdram_set_bcr(sdram, 0, val, sdram->cfg & 0x80000000);
             break;
         case 0x44: /* SDRAM_B1CR */
-            sdram_set_bcr(sdram, &sdram->bcr[1], val, sdram->cfg & 0x80000000);
+            sdram_set_bcr(sdram, 1, val, sdram->cfg & 0x80000000);
             break;
         case 0x48: /* SDRAM_B2CR */
-            sdram_set_bcr(sdram, &sdram->bcr[2], val, sdram->cfg & 0x80000000);
+            sdram_set_bcr(sdram, 2, val, sdram->cfg & 0x80000000);
             break;
         case 0x4C: /* SDRAM_B3CR */
-            sdram_set_bcr(sdram, &sdram->bcr[3], val, sdram->cfg & 0x80000000);
+            sdram_set_bcr(sdram, 3, val, sdram->cfg & 0x80000000);
             break;
         case 0x80: /* SDRAM_TR */
             sdram->tr = val & 0x018FC01F;
-- 
2.13.7

[Qemu-devel] [PATCH v3 4/6] ppc4xx: Rename ppc4xx_sdram_t in ppc440_uc.c to ppc440_sdram_t

[BALATON Zoltan]

There's already a struct with the same name in ppc4xx_devs.c. They are
not used outside their files so don't clash but they are also not
identical so rename the ppc440 specific one to distinguish them.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/ppc440_uc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index 9360f781ce..e46f59fba8 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -481,7 +481,7 @@ void ppc4xx_sdr_init(CPUPPCState *env)
 
 /*****************************************************************************/
 /* SDRAM controller */
-typedef struct ppc4xx_sdram_t {
+typedef struct ppc440_sdram_t {
     uint32_t addr;
     int nbanks;
     MemoryRegion containers[4]; /* used for clipping */
@@ -489,7 +489,7 @@ typedef struct ppc4xx_sdram_t {
     hwaddr ram_bases[4];
     hwaddr ram_sizes[4];
     uint32_t bcr[4];
-} ppc4xx_sdram_t;
+} ppc440_sdram_t;
 
 enum {
     SDRAM0_CFGADDR = 0x10,
@@ -564,7 +564,7 @@ static target_ulong sdram_size(uint32_t bcr)
     return size;
 }
 
-static void sdram_set_bcr(ppc4xx_sdram_t *sdram,
+static void sdram_set_bcr(ppc440_sdram_t *sdram,
                           uint32_t *bcrp, uint32_t bcr, int enabled)
 {
     unsigned n = bcrp - sdram->bcr;
@@ -589,7 +589,7 @@ static void sdram_set_bcr(ppc4xx_sdram_t *sdram,
     }
 }
 
-static void sdram_map_bcr(ppc4xx_sdram_t *sdram)
+static void sdram_map_bcr(ppc440_sdram_t *sdram)
 {
     int i;
 
@@ -607,7 +607,7 @@ static void sdram_map_bcr(ppc4xx_sdram_t *sdram)
 
 static uint32_t dcr_read_sdram(void *opaque, int dcrn)
 {
-    ppc4xx_sdram_t *sdram = opaque;
+    ppc440_sdram_t *sdram = opaque;
     uint32_t ret = 0;
 
     switch (dcrn) {
@@ -658,7 +658,7 @@ static uint32_t dcr_read_sdram(void *opaque, int dcrn)
 
 static void dcr_write_sdram(void *opaque, int dcrn, uint32_t val)
 {
-    ppc4xx_sdram_t *sdram = opaque;
+    ppc440_sdram_t *sdram = opaque;
 
     switch (dcrn) {
     case SDRAM_R0BAS:
@@ -689,7 +689,7 @@ static void dcr_write_sdram(void *opaque, int dcrn, uint32_t val)
 
 static void sdram_reset(void *opaque)
 {
-    ppc4xx_sdram_t *sdram = opaque;
+    ppc440_sdram_t *sdram = opaque;
 
     sdram->addr = 0;
 }
@@ -699,7 +699,7 @@ void ppc440_sdram_init(CPUPPCState *env, int nbanks,
                        hwaddr *ram_bases, hwaddr *ram_sizes,
                        int do_init)
 {
-    ppc4xx_sdram_t *sdram;
+    ppc440_sdram_t *sdram;
 
     sdram = g_malloc0(sizeof(*sdram));
     sdram->nbanks = nbanks;
-- 
2.13.7

[Qemu-devel] [PATCH v3 2/6] sam460ex: Clean up SPD EEPROM creation

[BALATON Zoltan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 7768 bytes --]

Get rid of code from MIPS Malta board used to create SPD EEPROM data
(parts of which was not even needed for sam460ex) and use the generic
spd_data_generate() function to simplify this.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v2: update for changes in patch 1

 hw/ppc/sam460ex.c | 173 +++++++-----------------------------------------------
 1 file changed, 20 insertions(+), 153 deletions(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 4b051c0950..b99aed26e0 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -2,7 +2,7 @@
  * QEMU aCube Sam460ex board emulation
  *
  * Copyright (c) 2012 François Revol
- * Copyright (c) 2016-2018 BALATON Zoltan
+ * Copyright (c) 2016-2019 BALATON Zoltan
  *
  * This file is derived from hw/ppc440_bamboo.c,
  * the copyright for that material belongs to the original owners.
@@ -87,135 +87,6 @@ struct boot_info {
     uint32_t entry;
 };
 
-/*****************************************************************************/
-/* SPD eeprom content from mips_malta.c */
-
-struct _eeprom24c0x_t {
-  uint8_t tick;
-  uint8_t address;
-  uint8_t command;
-  uint8_t ack;
-  uint8_t scl;
-  uint8_t sda;
-  uint8_t data;
-  uint8_t contents[256];
-};
-
-typedef struct _eeprom24c0x_t eeprom24c0x_t;
-
-static eeprom24c0x_t spd_eeprom = {
-    .contents = {
-        /* 00000000: */ 0x80, 0x08, 0xFF, 0x0D, 0x0A, 0xFF, 0x40, 0x00,
-        /* 00000008: */ 0x04, 0x75, 0x54, 0x00, 0x82, 0x08, 0x00, 0x01,
-        /* 00000010: */ 0x8F, 0x04, 0x02, 0x01, 0x01, 0x00, 0x00, 0x00,
-        /* 00000018: */ 0x00, 0x00, 0x00, 0x14, 0x0F, 0x14, 0x2D, 0xFF,
-        /* 00000020: */ 0x15, 0x08, 0x15, 0x08, 0x00, 0x00, 0x00, 0x00,
-        /* 00000028: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-        /* 00000030: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-        /* 00000038: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x12, 0xD0,
-        /* 00000040: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-        /* 00000048: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-        /* 00000050: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-        /* 00000058: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-        /* 00000060: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-        /* 00000068: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-        /* 00000070: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-        /* 00000078: */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x64, 0xF4,
-    },
-};
-
-static void generate_eeprom_spd(uint8_t *eeprom, ram_addr_t ram_size)
-{
-    enum { SDR = 0x4, DDR1 = 0x7, DDR2 = 0x8 } type;
-    uint8_t *spd = spd_eeprom.contents;
-    uint8_t nbanks = 0;
-    uint16_t density = 0;
-    int i;
-
-    /* work in terms of MB */
-    ram_size /= MiB;
-
-    while ((ram_size >= 4) && (nbanks <= 2)) {
-        int sz_log2 = MIN(31 - clz32(ram_size), 14);
-        nbanks++;
-        density |= 1 << (sz_log2 - 2);
-        ram_size -= 1 << sz_log2;
-    }
-
-    /* split to 2 banks if possible */
-    if ((nbanks == 1) && (density > 1)) {
-        nbanks++;
-        density >>= 1;
-    }
-
-    if (density & 0xff00) {
-        density = (density & 0xe0) | ((density >> 8) & 0x1f);
-        type = DDR2;
-    } else if (!(density & 0x1f)) {
-        type = DDR2;
-    } else {
-        type = SDR;
-    }
-
-    if (ram_size) {
-        warn_report("SPD cannot represent final " RAM_ADDR_FMT "MB"
-                    " of SDRAM", ram_size);
-    }
-
-    /* fill in SPD memory information */
-    spd[2] = type;
-    spd[5] = nbanks;
-    spd[31] = density;
-
-    /* XXX: this is totally random */
-    spd[9] = 0x10; /* CAS tcyc */
-    spd[18] = 0x20; /* CAS bit */
-    spd[23] = 0x10; /* CAS tcyc */
-    spd[25] = 0x10; /* CAS tcyc */
-
-    /* checksum */
-    spd[63] = 0;
-    for (i = 0; i < 63; i++) {
-        spd[63] += spd[i];
-    }
-
-    /* copy for SMBUS */
-    memcpy(eeprom, spd, sizeof(spd_eeprom.contents));
-}
-
-static void generate_eeprom_serial(uint8_t *eeprom)
-{
-    int i, pos = 0;
-    uint8_t mac[6] = { 0x00 };
-    uint8_t sn[5] = { 0x01, 0x23, 0x45, 0x67, 0x89 };
-
-    /* version */
-    eeprom[pos++] = 0x01;
-
-    /* count */
-    eeprom[pos++] = 0x02;
-
-    /* MAC address */
-    eeprom[pos++] = 0x01; /* MAC */
-    eeprom[pos++] = 0x06; /* length */
-    memcpy(&eeprom[pos], mac, sizeof(mac));
-    pos += sizeof(mac);
-
-    /* serial number */
-    eeprom[pos++] = 0x02; /* serial */
-    eeprom[pos++] = 0x05; /* length */
-    memcpy(&eeprom[pos], sn, sizeof(sn));
-    pos += sizeof(sn);
-
-    /* checksum */
-    eeprom[pos] = 0;
-    for (i = 0; i < pos; i++) {
-        eeprom[pos] += eeprom[i];
-    }
-}
-
-/*****************************************************************************/
-
 static int sam460ex_load_uboot(void)
 {
     DriveInfo *dinfo;
@@ -393,24 +264,23 @@ static void sam460ex_init(MachineState *machine)
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *isa = g_new(MemoryRegion, 1);
     MemoryRegion *ram_memories = g_new(MemoryRegion, SDRAM_NR_BANKS);
-    hwaddr ram_bases[SDRAM_NR_BANKS];
-    hwaddr ram_sizes[SDRAM_NR_BANKS];
+    hwaddr ram_bases[SDRAM_NR_BANKS] = {0};
+    hwaddr ram_sizes[SDRAM_NR_BANKS] = {0};
     MemoryRegion *l2cache_ram = g_new(MemoryRegion, 1);
     qemu_irq *irqs, *uic[4];
     PCIBus *pci_bus;
     PowerPCCPU *cpu;
     CPUPPCState *env;
-    PPC4xxI2CState *i2c[2];
+    I2CBus *i2c;
     hwaddr entry = UBOOT_ENTRY;
     hwaddr loadaddr = 0;
     target_long initrd_size = 0;
     DeviceState *dev;
     SysBusDevice *sbdev;
-    int success;
-    int i;
     struct boot_info *boot_info;
-    const size_t smbus_eeprom_size = 8 * 256;
-    uint8_t *smbus_eeprom_buf = g_malloc0(smbus_eeprom_size);
+    uint8_t *spd_data;
+    Error *err = NULL;
+    int success;
 
     cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
     env = &cpu->env;
@@ -439,8 +309,6 @@ static void sam460ex_init(MachineState *machine)
     uic[3] = ppcuic_init(env, &uic[0][16], 0xf0, 0, 1);
 
     /* SDRAM controller */
-    memset(ram_bases, 0, sizeof(ram_bases));
-    memset(ram_sizes, 0, sizeof(ram_sizes));
     /* put all RAM on first bank because board has one slot
      * and firmware only checks that */
     machine->ram_size = ppc4xx_sdram_adjust(machine->ram_size, 1,
@@ -451,23 +319,22 @@ static void sam460ex_init(MachineState *machine)
     ppc440_sdram_init(env, SDRAM_NR_BANKS, ram_memories,
                       ram_bases, ram_sizes, 1);
 
-    /* generate SPD EEPROM data */
-    for (i = 0; i < SDRAM_NR_BANKS; i++) {
-        generate_eeprom_spd(&smbus_eeprom_buf[i * 256], ram_sizes[i]);
-    }
-    generate_eeprom_serial(&smbus_eeprom_buf[4 * 256]);
-    generate_eeprom_serial(&smbus_eeprom_buf[6 * 256]);
-
-    /* IIC controllers */
+    /* IIC controllers and devices */
     dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600700, uic[0][2]);
-    i2c[0] = PPC4xx_I2C(dev);
-    object_property_set_bool(OBJECT(dev), true, "realized", NULL);
-    smbus_eeprom_init(i2c[0]->bus, 8, smbus_eeprom_buf, smbus_eeprom_size);
-    g_free(smbus_eeprom_buf);
-    i2c_create_slave(i2c[0]->bus, "m41t80", 0x68);
+    i2c = PPC4xx_I2C(dev)->bus;
+    /* SPD EEPROM on RAM module */
+    spd_data = spd_data_generate(DDR2, ram_sizes[0], &err);
+    if (err) {
+        warn_report_err(err);
+    }
+    if (spd_data) {
+        spd_data[20] = 4; /* SO-DIMM module */
+        smbus_eeprom_init_one(i2c, 0x50, spd_data);
+    }
+    /* RTC */
+    i2c_create_slave(i2c, "m41t80", 0x68);
 
     dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600800, uic[0][3]);
-    i2c[1] = PPC4xx_I2C(dev);
 
     /* External bus controller */
     ppc405_ebc_init(env);
-- 
2.13.7

Re: [Qemu-devel] [PATCH v3 0/6] Misc sam460ex related patches

[David Gibson]

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

On Thu, Jan 03, 2019 at 05:27:24PM +0100, BALATON Zoltan wrote:
> The last code patch in this series fixes memory size larger than 1GB
> for sam460ex, other patches are just clean ups I've made along the way.
> 
> The first patch is intended to be generic and may be useful for other
> boards which currently have their own SPD EEPROM data or don't yet
> generate any SPD data just have TODO comments instead. These are MIPS
> malta and fulong2e, ARM integratorcp and maybe aspeed, and the PIIX
> and Q35 pc machines. I did not try to change these as I have no way to
> test them throughly. Patch 2 converts sam460ex to use this function.
> Other patches are misc cleanups.

Applied to ppc-for-4.0 (missed the pull request I sent earlier today,
though).


> 
> Regards,
> BALATON Zoltan
> 
> v3: A tab indent got in by accident hence v3 but also include smilar
>     cleanup for sdram_set_bcd() in another file
> v2: Address review comments, omitting patches already merged
> 
> BALATON Zoltan (6):
>   smbus: Add a helper to generate SPD EEPROM data
>   sam460ex: Clean up SPD EEPROM creation
>   ppc4xx: Use ram_addr_t in ppc4xx_sdram_adjust()
>   ppc4xx: Rename ppc4xx_sdram_t in ppc440_uc.c to ppc440_sdram_t
>   ppc4xx: Pass array index to function instead of pointer into the array
>   sam460ex: Fix support for memory larger than 1GB
> 
>  hw/i2c/smbus_eeprom.c   | 130 ++++++++++++++++++++++++++++++++++
>  hw/ppc/ppc440_bamboo.c  |   2 +-
>  hw/ppc/ppc440_uc.c      |  70 +++++++++----------
>  hw/ppc/ppc4xx_devs.c    |  48 ++++++-------
>  hw/ppc/sam460ex.c       | 181 +++++++-----------------------------------------
>  include/hw/i2c/smbus.h  |   3 +
>  include/hw/ppc/ppc4xx.h |   2 +-
>  7 files changed, 216 insertions(+), 220 deletions(-)
> 

-- 
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 --]

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

[Philippe Mathieu-Daudé]

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
> 

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 <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
>>
>
>

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

[Philippe Mathieu-Daudé]

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
>>>
>>
>>

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/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
>>>>
>>>
>>>
>
>

Re: [Qemu-devel] [Qemu-ppc] [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 <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
>>>>> 
>>>> 
>>>> 
>> 
>

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

[Philippe Mathieu-Daudé]

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.