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

[Qemu-devel] [PATCH 0/8] 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. The very last patch is administrative
and adds some more files to sam460ex section in MAINTAINERS as was
discussed before.

Regards,
BALATON Zoltan

BALATON Zoltan (8):
  smbus: Add a helper to generate SPD EEPROM data
  sam460ex: Clean up SPD EEPROM creation
  ppc4xx: Disable debug logging by default
  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
  MAINTAINERS: Add more files to sam460ex

 MAINTAINERS             |   6 ++
 hw/i2c/smbus_eeprom.c   | 128 ++++++++++++++++++++++++++++++++++
 hw/ppc/ppc440_bamboo.c  |   2 +-
 hw/ppc/ppc440_uc.c      |  70 ++++++++++---------
 hw/ppc/ppc4xx_devs.c    |   7 +-
 hw/ppc/sam460ex.c       | 177 ++++++------------------------------------------
 include/hw/i2c/smbus.h  |   3 +
 include/hw/ppc/ppc4xx.h |   2 +-
 8 files changed, 197 insertions(+), 198 deletions(-)

-- 
2.13.7

[Qemu-devel] [PATCH 5/8] 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>
---
 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 6/8] 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.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/ppc440_uc.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 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);
         }
     }
 }
-- 
2.13.7

[Qemu-devel] [PATCH 4/8] 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>
---
 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 2bb91ed21b..7cbd2f54c6 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 3/8] ppc4xx: Disable debug logging by default

[BALATON Zoltan]

Debug logs were left enabled in ppc4xx_devs.c whereas in other files
these are normally not enabled. Disable it here as well.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/ppc4xx_devs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index 8c6f3c9577..9b6e4c60fa 100644
--- a/hw/ppc/ppc4xx_devs.c
+++ b/hw/ppc/ppc4xx_devs.c
@@ -32,8 +32,7 @@
 #include "exec/address-spaces.h"
 #include "qemu/error-report.h"
 
-#define DEBUG_UIC
-
+/*#define DEBUG_UIC*/
 
 #ifdef DEBUG_UIC
 #  define LOG_UIC(...) qemu_log_mask(CPU_LOG_INT, ## __VA_ARGS__)
-- 
2.13.7

[Qemu-devel] [PATCH 7/8] 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: 3344 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>
---
 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..414dc19c1d 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 7cbd2f54c6..7cc7d205d1 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 8/8] MAINTAINERS: Add more files to sam460ex

[BALATON Zoltan]

The sm501 model belonged to SH before but that seems to be inactive
now and latest changes were for sam460ex which is the more active user
of this device at the moment so let's adopt sm501 for sam460ex.

Also add device tree and firmware sources and binaries.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 180695f5d3..d2b71f8c4c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1029,8 +1029,14 @@ sam460ex
 M: BALATON Zoltan <balaton@eik.bme.hu>
 L: qemu-ppc@nongnu.org
 S: Maintained
+F: hw/ppc/sam460ex.c
+F: hw/ppc/ppc440_pcix.c
+F: hw/display/sm501*
 F: hw/ide/sii3112.c
 F: hw/timer/m41t80.c
+F: pc-bios/canyonlands.dt[sb]
+F: pc-bios/u-boot-sam460ex-20100605.bin
+F: roms/u-boot-sam460ex
 
 SH4 Machines
 ------------
-- 
2.13.7

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

[BALATON Zoltan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 7649 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>
---
 hw/ppc/sam460ex.c | 169 ++++++------------------------------------------------
 1 file changed, 16 insertions(+), 153 deletions(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 4b051c0950..2bb91ed21b 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,22 @@ 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;
+    int success;
 
     cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
     env = &cpu->env;
@@ -439,8 +308,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 +318,19 @@ 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]);
+    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

[Qemu-devel] [PATCH 1/8] 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>
---
 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();
+    }
+    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;
+        }
+    }
+
+    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
-- 
2.13.7

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

[David Gibson]

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

Re: [Qemu-devel] [PATCH 2/8] sam460ex: Clean up SPD EEPROM creation

[David Gibson]

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

On Wed, Jan 02, 2019 at 03:06:38AM +0100, BALATON Zoltan wrote:
> 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>
> ---
>  hw/ppc/sam460ex.c | 169 ++++++------------------------------------------------
>  1 file changed, 16 insertions(+), 153 deletions(-)
> 
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 4b051c0950..2bb91ed21b 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,22 @@ 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;
> +    int success;
>  
>      cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
>      env = &cpu->env;
> @@ -439,8 +308,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 +318,19 @@ 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]);
> +    if (spd_data) {
> +        spd_data[20] = 4; /* SO-DIMM module */
> +        smbus_eeprom_init_one(i2c, 0x50, spd_data);

Any specific reason this case is handled outside spd_data_generate()?

> +    }
> +    /* 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);

-- 
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 4/8] ppc4xx: Use ram_addr_t in ppc4xx_sdram_adjust()

[David Gibson]

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

On Wed, Jan 02, 2019 at 03:06:38AM +0100, BALATON Zoltan wrote:
> 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.

So, technically I think these should be 'hwaddr' (which represents a
guest physical address) rather tham ram_addr_t which
represents... something subtley different I've never properly
understood.

> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  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 2bb91ed21b..7cbd2f54c6 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[],

-- 
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 5/8] ppc4xx: Rename ppc4xx_sdram_t in ppc440_uc.c to ppc440_sdram_t

[David Gibson]

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

On Wed, Jan 02, 2019 at 03:06:38AM +0100, BALATON Zoltan wrote:
> 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;

-- 
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 6/8] ppc4xx: Pass array index to function instead of pointer into the array

[David Gibson]

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

On Wed, Jan 02, 2019 at 03:06:38AM +0100, BALATON Zoltan wrote:
> 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.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/ppc440_uc.c | 36 ++++++++++++++++--------------------
>  1 file changed, 16 insertions(+), 20 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);
>          }
>      }
>  }

-- 
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 3/8] ppc4xx: Disable debug logging by default

[David Gibson]

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

On Wed, Jan 02, 2019 at 03:06:38AM +0100, BALATON Zoltan wrote:
> Debug logs were left enabled in ppc4xx_devs.c whereas in other files
> these are normally not enabled. Disable it here as well.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Applied, thanks.

> ---
>  hw/ppc/ppc4xx_devs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index 8c6f3c9577..9b6e4c60fa 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -32,8 +32,7 @@
>  #include "exec/address-spaces.h"
>  #include "qemu/error-report.h"
>  
> -#define DEBUG_UIC
> -
> +/*#define DEBUG_UIC*/
>  
>  #ifdef DEBUG_UIC
>  #  define LOG_UIC(...) qemu_log_mask(CPU_LOG_INT, ## __VA_ARGS__)

-- 
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 8/8] MAINTAINERS: Add more files to sam460ex

[David Gibson]

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

On Wed, Jan 02, 2019 at 03:06:38AM +0100, BALATON Zoltan wrote:
> The sm501 model belonged to SH before but that seems to be inactive
> now and latest changes were for sam460ex which is the more active user
> of this device at the moment so let's adopt sm501 for sam460ex.
> 
> Also add device tree and firmware sources and binaries.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Applied, thanks.

> ---
>  MAINTAINERS | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 180695f5d3..d2b71f8c4c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1029,8 +1029,14 @@ sam460ex
>  M: BALATON Zoltan <balaton@eik.bme.hu>
>  L: qemu-ppc@nongnu.org
>  S: Maintained
> +F: hw/ppc/sam460ex.c
> +F: hw/ppc/ppc440_pcix.c
> +F: hw/display/sm501*
>  F: hw/ide/sii3112.c
>  F: hw/timer/m41t80.c
> +F: pc-bios/canyonlands.dt[sb]
> +F: pc-bios/u-boot-sam460ex-20100605.bin
> +F: roms/u-boot-sam460ex
>  
>  SH4 Machines
>  ------------

-- 
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 1/8] smbus: Add a helper to generate SPD EEPROM data

[BALATON Zoltan]

On Wed, 2 Jan 2019, David Gibson wrote:
> 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.

Good idea but I'm not sure it's needed. This is the only fatal error here 
(apart from g_malloc0 failing which should also abort) and in practice 
this could only happen if a caller specifies wrong type which is quite 
unlikely given that it's an enum so value outside of that would fail to 
compile with a warning (promoted to error by default). So this default 
case is only really here to please the compiler.

>> +    }
>> +    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?

The memory size is given by the user so it can be a config error (like 
when board has DDR2 but user sets memory size to 64MB).

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

I'd leave that to the caller to decide and not abort in this function. It 
will warn user that config is unexpected for the board but does not 
prevent it and try to give something that might still work (e.g. DDR 
instead of DDR2). Then the caller can check the returned data and abort if 
it insists that only DDR2 will work for the machine. Otherwise we'd make 
it impossible to use non-standard memory sizes for cases when it would 
still work (like when the OS does not check SPD EEPROMs and would happily 
use whatever memory size). I think it's already possible to start machines 
with odd memory sizes so that's not new and I did not want to prevent this 
if SPD EEPROMs are added (just SPD can't describe all memory in that 
case which may only be a problem for firmware but not when directly 
booting a kernel for example).

The idea of this function is to generate some data to start from instead 
of the static data some boards now have. which is often sufficient for the 
board as is but it could be checked or modified further to fit the 
specific needs of the board. As those needs can be widely different I did 
not attempt to handle all of them in this function to keep it generic.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH 2/8] sam460ex: Clean up SPD EEPROM creation

[BALATON Zoltan]

On Wed, 2 Jan 2019, David Gibson wrote:
> On Wed, Jan 02, 2019 at 03:06:38AM +0100, BALATON Zoltan wrote:
>> +    /* 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]);
>> +    if (spd_data) {
>> +        spd_data[20] = 4; /* SO-DIMM module */
>> +        smbus_eeprom_init_one(i2c, 0x50, spd_data);
>
> Any specific reason this case is handled outside spd_data_generate()?

As explained in previous reply I tried to keep number of options to the 
function to minimum and not handle every board specific case in that 
function. This board has SO-DIMM instead of regular DIMM socket but most 
of the SPD data is the same so it's easy enough to patch it here to match 
what real hardware may have. Otherwise spd_data_generate should have a lot 
of options for all different possibilites different boards might have.

(Also this is an example that unexpected configs might still work:

$ qemu-system-ppc -M sam460ex -m 64 -serial stdio
qemu-system-ppc: warning: Memory size is too small for SDRAM type, adjusting type
[...]
DRAM:  64 MiB (ECC not enabled, 460 MHz, CL0)

This SoC has a DRAM controller which accepts both DDR and DDR2 and the 
firmware does not care either so it works even if this cannot happen on 
real hardware. The spd_data[20] is different for DDR2 and DDR/SDR but in 
the latter it's ~WE latency which does not matter either so it can be set 
unconditionally. If this was not working the board code could check 
spd_data[2] != DDR2 and then abort itself.)

Regards,
BALATON Zoltan

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

[David Gibson]

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

On Wed, Jan 02, 2019 at 01:36:04PM +0100, BALATON Zoltan wrote:
> On Wed, 2 Jan 2019, David Gibson wrote:
> > 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.
> 
> Good idea but I'm not sure it's needed. This is the only fatal error here
> (apart from g_malloc0 failing which should also abort) and in practice this
> could only happen if a caller specifies wrong type which is quite unlikely
> given that it's an enum so value outside of that would fail to compile with
> a warning (promoted to error by default). So this default case is only
> really here to please the compiler.

Ok.  If the only reason you'd hit the default case is a bug in the
caller, then just use a g_assert_not_reached() rather than
error_report().  As a rule of thumb use asserts or aborts for things
that have to be bugs in the code, error_report() for things that
indicate a user or configuration error.

> > > +    }
> > > +    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?
> 
> The memory size is given by the user so it can be a config error (like when
> board has DDR2 but user sets memory size to 64MB).
> 
> > 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.
> 
> I'd leave that to the caller to decide and not abort in this
> function.

Right.  The obvious way to do that is to have this function take an
Error *, and use error_setg() where you have explicit warns now.  The
caller can pass &error_fatal if it just wants to treat that as a user
error, and do something else if it wants to implement a fallback.

> It
> will warn user that config is unexpected for the board but does not prevent
> it and try to give something that might still work (e.g. DDR instead of
> DDR2). Then the caller can check the returned data and abort if it insists
> that only DDR2 will work for the machine. Otherwise we'd make it impossible
> to use non-standard memory sizes for cases when it would still work (like
> when the OS does not check SPD EEPROMs and would happily use whatever memory
> size). I think it's already possible to start machines with odd memory sizes
> so that's not new and I did not want to prevent this if SPD EEPROMs are
> added (just SPD can't describe all memory in that case which may only be a
> problem for firmware but not when directly booting a kernel for example).
> 
> The idea of this function is to generate some data to start from instead of
> the static data some boards now have. which is often sufficient for the
> board as is but it could be checked or modified further to fit the specific
> needs of the board. As those needs can be widely different I did not attempt
> to handle all of them in this function to keep it generic.

-- 
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 2/8] sam460ex: Clean up SPD EEPROM creation

[David Gibson]

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

On Wed, Jan 02, 2019 at 01:49:44PM +0100, BALATON Zoltan wrote:
> On Wed, 2 Jan 2019, David Gibson wrote:
> > On Wed, Jan 02, 2019 at 03:06:38AM +0100, BALATON Zoltan wrote:
> > > +    /* 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]);
> > > +    if (spd_data) {
> > > +        spd_data[20] = 4; /* SO-DIMM module */
> > > +        smbus_eeprom_init_one(i2c, 0x50, spd_data);
> > 
> > Any specific reason this case is handled outside spd_data_generate()?
> 
> As explained in previous reply I tried to keep number of options to the
> function to minimum and not handle every board specific case in that
> function. This board has SO-DIMM instead of regular DIMM socket but most of
> the SPD data is the same so it's easy enough to patch it here to match what
> real hardware may have. Otherwise spd_data_generate should have a lot of
> options for all different possibilites different boards might have.

Ok, fair enough.

> 
> (Also this is an example that unexpected configs might still work:
> 
> $ qemu-system-ppc -M sam460ex -m 64 -serial stdio
> qemu-system-ppc: warning: Memory size is too small for SDRAM type, adjusting type
> [...]
> DRAM:  64 MiB (ECC not enabled, 460 MHz, CL0)
> 
> This SoC has a DRAM controller which accepts both DDR and DDR2 and the
> firmware does not care either so it works even if this cannot happen on real
> hardware. The spd_data[20] is different for DDR2 and DDR/SDR but in the
> latter it's ~WE latency which does not matter either so it can be set
> unconditionally. If this was not working the board code could check
> spd_data[2] != DDR2 and then abort itself.)
> 
> Regards,
> BALATON Zoltan
> 

-- 
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 4/8] ppc4xx: Use ram_addr_t in ppc4xx_sdram_adjust()

[BALATON Zoltan]

On Wed, 2 Jan 2019, David Gibson wrote:
> On Wed, Jan 02, 2019 at 03:06:38AM +0100, BALATON Zoltan wrote:
>> 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.
>
> So, technically I think these should be 'hwaddr' (which represents a
> guest physical address) rather tham ram_addr_t which
> represents... something subtley different I've never properly
> understood.

I don't understand the difference either but ram_size in MachineState 
where this value comes from is ram_addr_t now so I've left is for now. If 
someone knows which type should this be can change it in another patch 
later.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH 4/8] ppc4xx: Use ram_addr_t in ppc4xx_sdram_adjust()

[David Gibson]

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

On Thu, Jan 03, 2019 at 03:03:20PM +0100, BALATON Zoltan wrote:
> On Wed, 2 Jan 2019, David Gibson wrote:
> > On Wed, Jan 02, 2019 at 03:06:38AM +0100, BALATON Zoltan wrote:
> > > 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.
> > 
> > So, technically I think these should be 'hwaddr' (which represents a
> > guest physical address) rather tham ram_addr_t which
> > represents... something subtley different I've never properly
> > understood.
> 
> I don't understand the difference either but ram_size in MachineState where
> this value comes from is ram_addr_t now so I've left is for now. If someone
> knows which type should this be can change it in another patch
> later.

Ok, fair enough.

-- 
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 4/8] ppc4xx: Use ram_addr_t in ppc4xx_sdram_adjust()

[BALATON Zoltan]

On Fri, 4 Jan 2019, David Gibson wrote:
> On Thu, Jan 03, 2019 at 03:03:20PM +0100, BALATON Zoltan wrote:
>> On Wed, 2 Jan 2019, David Gibson wrote:
>>> On Wed, Jan 02, 2019 at 03:06:38AM +0100, BALATON Zoltan wrote:
>>>> 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.
>>>
>>> So, technically I think these should be 'hwaddr' (which represents a
>>> guest physical address) rather tham ram_addr_t which
>>> represents... something subtley different I've never properly
>>> understood.
>>
>> I don't understand the difference either but ram_size in MachineState where
>> this value comes from is ram_addr_t now so I've left is for now. If someone
>> knows which type should this be can change it in another patch
>> later.
>
> Ok, fair enough.

Then will you take v3 of this series or is there anything else that should 
be corrected?

Regards,
BALATON Zoltan