[PATCH v7 0/8] ppc4xx_sdram QOMify and clean ups

[PATCH v7 0/8] ppc4xx_sdram QOMify and clean ups

[BALATON Zoltan]

This is the end of the QOMify series originially started by Cédric
rebased on master now only including patches not yet merged. Patches
that still need review are 1-3 (these only move code to
ppc4xx_sdram.c) and 6-7 (unify DDR and DDR2 models to share code where
possible).

Regards,
BALATON Zoltan

v7: Rebase on master after merge of first part of the series
v6: Split patch moving sdram controller models together into smaller steps
v5: Add functions the enable sdram controller and call it from boards
v4: address more review comments
v3: Fix patches that got squashed during rebase
v2: address some review comments and try to avoid compile problem with
gcc 12.2 (untested)


BALATON Zoltan (8):
  ppc440_uc.c: Move DDR2 SDRAM controller model to ppc4xx_sdram.c
  ppc4xx_devs.c: Move DDR SDRAM controller model to ppc4xx_sdram.c
  ppc4xx_sdram: Move ppc4xx_sdram_banks() to ppc4xx_sdram.c
  ppc4xx_sdram: Use hwaddr for memory bank size
  ppc4xx_sdram: Rename local state variable for brevity
  ppc4xx_sdram: Generalise bank setup
  ppc4xx_sdram: Convert DDR SDRAM controller to new bank handling
  ppc4xx_sdram: Add errp parameter to ppc4xx_sdram_banks()

 hw/ppc/meson.build      |   3 +-
 hw/ppc/ppc440_uc.c      | 332 ------------------
 hw/ppc/ppc4xx_devs.c    | 414 ----------------------
 hw/ppc/ppc4xx_sdram.c   | 757 ++++++++++++++++++++++++++++++++++++++++
 hw/ppc/trace-events     |   1 +
 include/hw/ppc/ppc4xx.h |  20 +-
 6 files changed, 768 insertions(+), 759 deletions(-)
 create mode 100644 hw/ppc/ppc4xx_sdram.c

-- 
2.30.4

[PATCH v7 1/8] ppc440_uc.c: Move DDR2 SDRAM controller model to ppc4xx_sdram.c

[BALATON Zoltan]

In order to move PPC4xx SDRAM controller models together move out the
DDR2 controller model from ppc440_uc.c into a new ppc4xx_sdram.c file.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/meson.build    |   3 +-
 hw/ppc/ppc440_uc.c    | 332 ----------------------------------------
 hw/ppc/ppc4xx_sdram.c | 348 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 350 insertions(+), 333 deletions(-)
 create mode 100644 hw/ppc/ppc4xx_sdram.c

diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
index 32babc9b48..c927337da0 100644
--- a/hw/ppc/meson.build
+++ b/hw/ppc/meson.build
@@ -59,8 +59,9 @@ ppc_ss.add(when: 'CONFIG_PPC440', if_true: files(
   'ppc440_bamboo.c',
   'ppc440_pcix.c', 'ppc440_uc.c'))
 ppc_ss.add(when: 'CONFIG_PPC4XX', if_true: files(
+  'ppc4xx_devs.c',
   'ppc4xx_pci.c',
-  'ppc4xx_devs.c'))
+  'ppc4xx_sdram.c'))
 ppc_ss.add(when: 'CONFIG_SAM460EX', if_true: files('sam460ex.c'))
 # PReP
 ppc_ss.add(when: 'CONFIG_PREP', if_true: files('prep.c'))
diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index 5fbf44009e..651263926e 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -10,21 +10,14 @@
 
 #include "qemu/osdep.h"
 #include "qemu/units.h"
-#include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qemu/log.h"
-#include "qemu/module.h"
 #include "hw/irq.h"
-#include "exec/memory.h"
-#include "cpu.h"
 #include "hw/ppc/ppc4xx.h"
 #include "hw/qdev-properties.h"
 #include "hw/pci/pci.h"
-#include "sysemu/block-backend.h"
 #include "sysemu/reset.h"
 #include "ppc440.h"
-#include "qom/object.h"
-#include "trace.h"
 
 /*****************************************************************************/
 /* L2 Cache as SRAM */
@@ -478,331 +471,6 @@ void ppc4xx_sdr_init(CPUPPCState *env)
                      sdr, &dcr_read_sdr, &dcr_write_sdr);
 }
 
-/*****************************************************************************/
-/* SDRAM controller */
-enum {
-    SDRAM0_CFGADDR = 0x10,
-    SDRAM0_CFGDATA,
-    SDRAM_R0BAS = 0x40,
-    SDRAM_R1BAS,
-    SDRAM_R2BAS,
-    SDRAM_R3BAS,
-    SDRAM_CONF1HB = 0x45,
-    SDRAM_PLBADDULL = 0x4a,
-    SDRAM_CONF1LL = 0x4b,
-    SDRAM_CONFPATHB = 0x4f,
-    SDRAM_PLBADDUHB = 0x50,
-};
-
-static uint32_t sdram_ddr2_bcr(hwaddr ram_base, hwaddr ram_size)
-{
-    uint32_t bcr;
-
-    switch (ram_size) {
-    case 8 * MiB:
-        bcr = 0xffc0;
-        break;
-    case 16 * MiB:
-        bcr = 0xff80;
-        break;
-    case 32 * MiB:
-        bcr = 0xff00;
-        break;
-    case 64 * MiB:
-        bcr = 0xfe00;
-        break;
-    case 128 * MiB:
-        bcr = 0xfc00;
-        break;
-    case 256 * MiB:
-        bcr = 0xf800;
-        break;
-    case 512 * MiB:
-        bcr = 0xf000;
-        break;
-    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 >> 2 & 0xffe00000;
-    bcr |= 1;
-
-    return bcr;
-}
-
-static inline hwaddr sdram_ddr2_base(uint32_t bcr)
-{
-    return (bcr & 0xffe00000) << 2;
-}
-
-static uint64_t sdram_ddr2_size(uint32_t bcr)
-{
-    uint64_t size;
-    int sh;
-
-    sh = 1024 - ((bcr >> 6) & 0x3ff);
-    size = 8 * MiB * sh;
-
-    return size;
-}
-
-static void sdram_bank_map(Ppc4xxSdramBank *bank)
-{
-    memory_region_init(&bank->container, NULL, "sdram-container", bank->size);
-    memory_region_add_subregion(&bank->container, 0, &bank->ram);
-    memory_region_add_subregion(get_system_memory(), bank->base,
-                                &bank->container);
-}
-
-static void sdram_bank_unmap(Ppc4xxSdramBank *bank)
-{
-    memory_region_del_subregion(get_system_memory(), &bank->container);
-    memory_region_del_subregion(&bank->container, &bank->ram);
-    object_unparent(OBJECT(&bank->container));
-}
-
-static void sdram_ddr2_set_bcr(Ppc4xxSdramDdr2State *sdram, int i,
-                               uint32_t bcr, int enabled)
-{
-    if (sdram->bank[i].bcr & 1) {
-        /* First unmap RAM if enabled */
-        trace_ppc4xx_sdram_unmap(sdram_ddr2_base(sdram->bank[i].bcr),
-                                 sdram_ddr2_size(sdram->bank[i].bcr));
-        sdram_bank_unmap(&sdram->bank[i]);
-    }
-    sdram->bank[i].bcr = bcr & 0xffe0ffc1;
-    if (enabled && (bcr & 1)) {
-        trace_ppc4xx_sdram_map(sdram_ddr2_base(bcr), sdram_ddr2_size(bcr));
-        sdram_bank_map(&sdram->bank[i]);
-    }
-}
-
-static void sdram_ddr2_map_bcr(Ppc4xxSdramDdr2State *sdram)
-{
-    int i;
-
-    for (i = 0; i < sdram->nbanks; i++) {
-        if (sdram->bank[i].size) {
-            sdram_ddr2_set_bcr(sdram, i,
-                               sdram_ddr2_bcr(sdram->bank[i].base,
-                                              sdram->bank[i].size), 1);
-        } else {
-            sdram_ddr2_set_bcr(sdram, i, 0, 0);
-        }
-    }
-}
-
-static void sdram_ddr2_unmap_bcr(Ppc4xxSdramDdr2State *sdram)
-{
-    int i;
-
-    for (i = 0; i < sdram->nbanks; i++) {
-        if (sdram->bank[i].size) {
-            sdram_ddr2_set_bcr(sdram, i, sdram->bank[i].bcr & ~1, 0);
-        }
-    }
-}
-
-static uint32_t sdram_ddr2_dcr_read(void *opaque, int dcrn)
-{
-    Ppc4xxSdramDdr2State *sdram = opaque;
-    uint32_t ret = 0;
-
-    switch (dcrn) {
-    case SDRAM_R0BAS:
-    case SDRAM_R1BAS:
-    case SDRAM_R2BAS:
-    case SDRAM_R3BAS:
-        if (sdram->bank[dcrn - SDRAM_R0BAS].size) {
-            ret = sdram_ddr2_bcr(sdram->bank[dcrn - SDRAM_R0BAS].base,
-                                 sdram->bank[dcrn - SDRAM_R0BAS].size);
-        }
-        break;
-    case SDRAM_CONF1HB:
-    case SDRAM_CONF1LL:
-    case SDRAM_CONFPATHB:
-    case SDRAM_PLBADDULL:
-    case SDRAM_PLBADDUHB:
-        break;
-    case SDRAM0_CFGADDR:
-        ret = sdram->addr;
-        break;
-    case SDRAM0_CFGDATA:
-        switch (sdram->addr) {
-        case 0x14: /* SDRAM_MCSTAT (405EX) */
-        case 0x1F:
-            ret = 0x80000000;
-            break;
-        case 0x21: /* SDRAM_MCOPT2 */
-            ret = sdram->mcopt2;
-            break;
-        case 0x40: /* SDRAM_MB0CF */
-            ret = 0x00008001;
-            break;
-        case 0x7A: /* SDRAM_DLCR */
-            ret = 0x02000000;
-            break;
-        case 0xE1: /* SDR0_DDR0 */
-            ret = SDR0_DDR0_DDRM_ENCODE(1) | SDR0_DDR0_DDRM_DDR1;
-            break;
-        default:
-            break;
-        }
-        break;
-    default:
-        break;
-    }
-
-    return ret;
-}
-
-#define SDRAM_DDR2_MCOPT2_DCEN BIT(27)
-
-static void sdram_ddr2_dcr_write(void *opaque, int dcrn, uint32_t val)
-{
-    Ppc4xxSdramDdr2State *sdram = opaque;
-
-    switch (dcrn) {
-    case SDRAM_R0BAS:
-    case SDRAM_R1BAS:
-    case SDRAM_R2BAS:
-    case SDRAM_R3BAS:
-    case SDRAM_CONF1HB:
-    case SDRAM_CONF1LL:
-    case SDRAM_CONFPATHB:
-    case SDRAM_PLBADDULL:
-    case SDRAM_PLBADDUHB:
-        break;
-    case SDRAM0_CFGADDR:
-        sdram->addr = val;
-        break;
-    case SDRAM0_CFGDATA:
-        switch (sdram->addr) {
-        case 0x00: /* B0CR */
-            break;
-        case 0x21: /* SDRAM_MCOPT2 */
-            if (!(sdram->mcopt2 & SDRAM_DDR2_MCOPT2_DCEN) &&
-                (val & SDRAM_DDR2_MCOPT2_DCEN)) {
-                trace_ppc4xx_sdram_enable("enable");
-                /* validate all RAM mappings */
-                sdram_ddr2_map_bcr(sdram);
-                sdram->mcopt2 |= SDRAM_DDR2_MCOPT2_DCEN;
-            } else if ((sdram->mcopt2 & SDRAM_DDR2_MCOPT2_DCEN) &&
-                       !(val & SDRAM_DDR2_MCOPT2_DCEN)) {
-                trace_ppc4xx_sdram_enable("disable");
-                /* invalidate all RAM mappings */
-                sdram_ddr2_unmap_bcr(sdram);
-                sdram->mcopt2 &= ~SDRAM_DDR2_MCOPT2_DCEN;
-            }
-            break;
-        default:
-            break;
-        }
-        break;
-    default:
-        break;
-    }
-}
-
-static void ppc4xx_sdram_ddr2_reset(DeviceState *dev)
-{
-    Ppc4xxSdramDdr2State *sdram = PPC4xx_SDRAM_DDR2(dev);
-
-    sdram->addr = 0;
-    sdram->mcopt2 = 0;
-}
-
-static void ppc4xx_sdram_ddr2_realize(DeviceState *dev, Error **errp)
-{
-    Ppc4xxSdramDdr2State *s = PPC4xx_SDRAM_DDR2(dev);
-    Ppc4xxDcrDeviceState *dcr = PPC4xx_DCR_DEVICE(dev);
-    /*
-     * SoC also has 4 GiB but that causes problem with 32 bit
-     * builds (4*GiB overflows the 32 bit ram_addr_t).
-     */
-    const ram_addr_t valid_bank_sizes[] = {
-        2 * GiB, 1 * GiB, 512 * MiB, 256 * MiB, 128 * MiB,
-        64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 0
-    };
-
-    if (s->nbanks < 1 || s->nbanks > 4) {
-        error_setg(errp, "Invalid number of RAM banks");
-        return;
-    }
-    if (!s->dram_mr) {
-        error_setg(errp, "Missing dram memory region");
-        return;
-    }
-    ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank, valid_bank_sizes);
-
-    ppc4xx_dcr_register(dcr, SDRAM0_CFGADDR,
-                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
-    ppc4xx_dcr_register(dcr, SDRAM0_CFGDATA,
-                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
-
-    ppc4xx_dcr_register(dcr, SDRAM_R0BAS,
-                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
-    ppc4xx_dcr_register(dcr, SDRAM_R1BAS,
-                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
-    ppc4xx_dcr_register(dcr, SDRAM_R2BAS,
-                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
-    ppc4xx_dcr_register(dcr, SDRAM_R3BAS,
-                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
-    ppc4xx_dcr_register(dcr, SDRAM_CONF1HB,
-                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
-    ppc4xx_dcr_register(dcr, SDRAM_PLBADDULL,
-                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
-    ppc4xx_dcr_register(dcr, SDRAM_CONF1LL,
-                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
-    ppc4xx_dcr_register(dcr, SDRAM_CONFPATHB,
-                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
-    ppc4xx_dcr_register(dcr, SDRAM_PLBADDUHB,
-                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
-}
-
-static Property ppc4xx_sdram_ddr2_props[] = {
-    DEFINE_PROP_LINK("dram", Ppc4xxSdramDdr2State, dram_mr, TYPE_MEMORY_REGION,
-                     MemoryRegion *),
-    DEFINE_PROP_UINT32("nbanks", Ppc4xxSdramDdr2State, nbanks, 4),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void ppc4xx_sdram_ddr2_class_init(ObjectClass *oc, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(oc);
-
-    dc->realize = ppc4xx_sdram_ddr2_realize;
-    dc->reset = ppc4xx_sdram_ddr2_reset;
-    /* Reason: only works as function of a ppc4xx SoC */
-    dc->user_creatable = false;
-    device_class_set_props(dc, ppc4xx_sdram_ddr2_props);
-}
-
-void ppc4xx_sdram_ddr2_enable(Ppc4xxSdramDdr2State *s)
-{
-    sdram_ddr2_dcr_write(s, SDRAM0_CFGADDR, 0x21);
-    sdram_ddr2_dcr_write(s, SDRAM0_CFGDATA, 0x08000000);
-}
-
-static const TypeInfo ppc4xx_types[] = {
-    {
-        .name           = TYPE_PPC4xx_SDRAM_DDR2,
-        .parent         = TYPE_PPC4xx_DCR_DEVICE,
-        .instance_size  = sizeof(Ppc4xxSdramDdr2State),
-        .class_init     = ppc4xx_sdram_ddr2_class_init,
-    }
-};
-DEFINE_TYPES(ppc4xx_types)
-
 /*****************************************************************************/
 /* PLB to AHB bridge */
 enum {
diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
new file mode 100644
index 0000000000..b49a7ed60a
--- /dev/null
+++ b/hw/ppc/ppc4xx_sdram.c
@@ -0,0 +1,348 @@
+/*
+ * DDR2 SDRAM controller:
+ * Copyright (c) 2012 François Revol
+ * Copyright (c) 2016-2019 BALATON Zoltan
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+#include "qapi/error.h"
+#include "exec/address-spaces.h" /* get_system_memory() */
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include "hw/ppc/ppc4xx.h"
+#include "trace.h"
+
+/*****************************************************************************/
+/* Shared functions */
+
+static void sdram_bank_map(Ppc4xxSdramBank *bank)
+{
+    memory_region_init(&bank->container, NULL, "sdram-container", bank->size);
+    memory_region_add_subregion(&bank->container, 0, &bank->ram);
+    memory_region_add_subregion(get_system_memory(), bank->base,
+                                &bank->container);
+}
+
+static void sdram_bank_unmap(Ppc4xxSdramBank *bank)
+{
+    memory_region_del_subregion(get_system_memory(), &bank->container);
+    memory_region_del_subregion(&bank->container, &bank->ram);
+    object_unparent(OBJECT(&bank->container));
+}
+
+enum {
+    SDRAM0_CFGADDR = 0x010,
+    SDRAM0_CFGDATA = 0x011,
+};
+
+/*****************************************************************************/
+/* DDR2 SDRAM controller */
+enum {
+    SDRAM_R0BAS = 0x40,
+    SDRAM_R1BAS,
+    SDRAM_R2BAS,
+    SDRAM_R3BAS,
+    SDRAM_CONF1HB = 0x45,
+    SDRAM_PLBADDULL = 0x4a,
+    SDRAM_CONF1LL = 0x4b,
+    SDRAM_CONFPATHB = 0x4f,
+    SDRAM_PLBADDUHB = 0x50,
+};
+
+static uint32_t sdram_ddr2_bcr(hwaddr ram_base, hwaddr ram_size)
+{
+    uint32_t bcr;
+
+    switch (ram_size) {
+    case 8 * MiB:
+        bcr = 0xffc0;
+        break;
+    case 16 * MiB:
+        bcr = 0xff80;
+        break;
+    case 32 * MiB:
+        bcr = 0xff00;
+        break;
+    case 64 * MiB:
+        bcr = 0xfe00;
+        break;
+    case 128 * MiB:
+        bcr = 0xfc00;
+        break;
+    case 256 * MiB:
+        bcr = 0xf800;
+        break;
+    case 512 * MiB:
+        bcr = 0xf000;
+        break;
+    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 >> 2 & 0xffe00000;
+    bcr |= 1;
+
+    return bcr;
+}
+
+static inline hwaddr sdram_ddr2_base(uint32_t bcr)
+{
+    return (bcr & 0xffe00000) << 2;
+}
+
+static uint64_t sdram_ddr2_size(uint32_t bcr)
+{
+    uint64_t size;
+    int sh;
+
+    sh = 1024 - ((bcr >> 6) & 0x3ff);
+    size = 8 * MiB * sh;
+
+    return size;
+}
+
+static void sdram_ddr2_set_bcr(Ppc4xxSdramDdr2State *sdram, int i,
+                               uint32_t bcr, int enabled)
+{
+    if (sdram->bank[i].bcr & 1) {
+        /* First unmap RAM if enabled */
+        trace_ppc4xx_sdram_unmap(sdram_ddr2_base(sdram->bank[i].bcr),
+                                 sdram_ddr2_size(sdram->bank[i].bcr));
+        sdram_bank_unmap(&sdram->bank[i]);
+    }
+    sdram->bank[i].bcr = bcr & 0xffe0ffc1;
+    if (enabled && (bcr & 1)) {
+        trace_ppc4xx_sdram_map(sdram_ddr2_base(bcr), sdram_ddr2_size(bcr));
+        sdram_bank_map(&sdram->bank[i]);
+    }
+}
+
+static void sdram_ddr2_map_bcr(Ppc4xxSdramDdr2State *sdram)
+{
+    int i;
+
+    for (i = 0; i < sdram->nbanks; i++) {
+        if (sdram->bank[i].size) {
+            sdram_ddr2_set_bcr(sdram, i,
+                               sdram_ddr2_bcr(sdram->bank[i].base,
+                                              sdram->bank[i].size), 1);
+        } else {
+            sdram_ddr2_set_bcr(sdram, i, 0, 0);
+        }
+    }
+}
+
+static void sdram_ddr2_unmap_bcr(Ppc4xxSdramDdr2State *sdram)
+{
+    int i;
+
+    for (i = 0; i < sdram->nbanks; i++) {
+        if (sdram->bank[i].size) {
+            sdram_ddr2_set_bcr(sdram, i, sdram->bank[i].bcr & ~1, 0);
+        }
+    }
+}
+
+static uint32_t sdram_ddr2_dcr_read(void *opaque, int dcrn)
+{
+    Ppc4xxSdramDdr2State *sdram = opaque;
+    uint32_t ret = 0;
+
+    switch (dcrn) {
+    case SDRAM_R0BAS:
+    case SDRAM_R1BAS:
+    case SDRAM_R2BAS:
+    case SDRAM_R3BAS:
+        if (sdram->bank[dcrn - SDRAM_R0BAS].size) {
+            ret = sdram_ddr2_bcr(sdram->bank[dcrn - SDRAM_R0BAS].base,
+                                 sdram->bank[dcrn - SDRAM_R0BAS].size);
+        }
+        break;
+    case SDRAM_CONF1HB:
+    case SDRAM_CONF1LL:
+    case SDRAM_CONFPATHB:
+    case SDRAM_PLBADDULL:
+    case SDRAM_PLBADDUHB:
+        break;
+    case SDRAM0_CFGADDR:
+        ret = sdram->addr;
+        break;
+    case SDRAM0_CFGDATA:
+        switch (sdram->addr) {
+        case 0x14: /* SDRAM_MCSTAT (405EX) */
+        case 0x1F:
+            ret = 0x80000000;
+            break;
+        case 0x21: /* SDRAM_MCOPT2 */
+            ret = sdram->mcopt2;
+            break;
+        case 0x40: /* SDRAM_MB0CF */
+            ret = 0x00008001;
+            break;
+        case 0x7A: /* SDRAM_DLCR */
+            ret = 0x02000000;
+            break;
+        case 0xE1: /* SDR0_DDR0 */
+            ret = SDR0_DDR0_DDRM_ENCODE(1) | SDR0_DDR0_DDRM_DDR1;
+            break;
+        default:
+            break;
+        }
+        break;
+    default:
+        break;
+    }
+
+    return ret;
+}
+
+#define SDRAM_DDR2_MCOPT2_DCEN BIT(27)
+
+static void sdram_ddr2_dcr_write(void *opaque, int dcrn, uint32_t val)
+{
+    Ppc4xxSdramDdr2State *sdram = opaque;
+
+    switch (dcrn) {
+    case SDRAM_R0BAS:
+    case SDRAM_R1BAS:
+    case SDRAM_R2BAS:
+    case SDRAM_R3BAS:
+    case SDRAM_CONF1HB:
+    case SDRAM_CONF1LL:
+    case SDRAM_CONFPATHB:
+    case SDRAM_PLBADDULL:
+    case SDRAM_PLBADDUHB:
+        break;
+    case SDRAM0_CFGADDR:
+        sdram->addr = val;
+        break;
+    case SDRAM0_CFGDATA:
+        switch (sdram->addr) {
+        case 0x00: /* B0CR */
+            break;
+        case 0x21: /* SDRAM_MCOPT2 */
+            if (!(sdram->mcopt2 & SDRAM_DDR2_MCOPT2_DCEN) &&
+                (val & SDRAM_DDR2_MCOPT2_DCEN)) {
+                trace_ppc4xx_sdram_enable("enable");
+                /* validate all RAM mappings */
+                sdram_ddr2_map_bcr(sdram);
+                sdram->mcopt2 |= SDRAM_DDR2_MCOPT2_DCEN;
+            } else if ((sdram->mcopt2 & SDRAM_DDR2_MCOPT2_DCEN) &&
+                       !(val & SDRAM_DDR2_MCOPT2_DCEN)) {
+                trace_ppc4xx_sdram_enable("disable");
+                /* invalidate all RAM mappings */
+                sdram_ddr2_unmap_bcr(sdram);
+                sdram->mcopt2 &= ~SDRAM_DDR2_MCOPT2_DCEN;
+            }
+            break;
+        default:
+            break;
+        }
+        break;
+    default:
+        break;
+    }
+}
+
+static void ppc4xx_sdram_ddr2_reset(DeviceState *dev)
+{
+    Ppc4xxSdramDdr2State *sdram = PPC4xx_SDRAM_DDR2(dev);
+
+    sdram->addr = 0;
+    sdram->mcopt2 = 0;
+}
+
+static void ppc4xx_sdram_ddr2_realize(DeviceState *dev, Error **errp)
+{
+    Ppc4xxSdramDdr2State *s = PPC4xx_SDRAM_DDR2(dev);
+    Ppc4xxDcrDeviceState *dcr = PPC4xx_DCR_DEVICE(dev);
+    /*
+     * SoC also has 4 GiB but that causes problem with 32 bit
+     * builds (4*GiB overflows the 32 bit ram_addr_t).
+     */
+    const ram_addr_t valid_bank_sizes[] = {
+        2 * GiB, 1 * GiB, 512 * MiB, 256 * MiB, 128 * MiB,
+        64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 0
+    };
+
+    if (s->nbanks < 1 || s->nbanks > 4) {
+        error_setg(errp, "Invalid number of RAM banks");
+        return;
+    }
+    if (!s->dram_mr) {
+        error_setg(errp, "Missing dram memory region");
+        return;
+    }
+    ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank, valid_bank_sizes);
+
+    ppc4xx_dcr_register(dcr, SDRAM0_CFGADDR,
+                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
+    ppc4xx_dcr_register(dcr, SDRAM0_CFGDATA,
+                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
+
+    ppc4xx_dcr_register(dcr, SDRAM_R0BAS,
+                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
+    ppc4xx_dcr_register(dcr, SDRAM_R1BAS,
+                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
+    ppc4xx_dcr_register(dcr, SDRAM_R2BAS,
+                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
+    ppc4xx_dcr_register(dcr, SDRAM_R3BAS,
+                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
+    ppc4xx_dcr_register(dcr, SDRAM_CONF1HB,
+                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
+    ppc4xx_dcr_register(dcr, SDRAM_PLBADDULL,
+                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
+    ppc4xx_dcr_register(dcr, SDRAM_CONF1LL,
+                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
+    ppc4xx_dcr_register(dcr, SDRAM_CONFPATHB,
+                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
+    ppc4xx_dcr_register(dcr, SDRAM_PLBADDUHB,
+                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
+}
+
+static Property ppc4xx_sdram_ddr2_props[] = {
+    DEFINE_PROP_LINK("dram", Ppc4xxSdramDdr2State, dram_mr, TYPE_MEMORY_REGION,
+                     MemoryRegion *),
+    DEFINE_PROP_UINT32("nbanks", Ppc4xxSdramDdr2State, nbanks, 4),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ppc4xx_sdram_ddr2_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = ppc4xx_sdram_ddr2_realize;
+    dc->reset = ppc4xx_sdram_ddr2_reset;
+    /* Reason: only works as function of a ppc4xx SoC */
+    dc->user_creatable = false;
+    device_class_set_props(dc, ppc4xx_sdram_ddr2_props);
+}
+
+void ppc4xx_sdram_ddr2_enable(Ppc4xxSdramDdr2State *s)
+{
+    sdram_ddr2_dcr_write(s, SDRAM0_CFGADDR, 0x21);
+    sdram_ddr2_dcr_write(s, SDRAM0_CFGDATA, 0x08000000);
+}
+
+static const TypeInfo ppc4xx_sdram_types[] = {
+    {
+        .name           = TYPE_PPC4xx_SDRAM_DDR2,
+        .parent         = TYPE_PPC4xx_DCR_DEVICE,
+        .instance_size  = sizeof(Ppc4xxSdramDdr2State),
+        .class_init     = ppc4xx_sdram_ddr2_class_init,
+    }
+};
+
+DEFINE_TYPES(ppc4xx_sdram_types)
-- 
2.30.4

Re: [PATCH v7 1/8] ppc440_uc.c: Move DDR2 SDRAM controller model to ppc4xx_sdram.c

[Daniel Henrique Barboza]

Minimal nit down below:

On 10/19/22 13:02, BALATON Zoltan wrote:
> In order to move PPC4xx SDRAM controller models together move out the
> DDR2 controller model from ppc440_uc.c into a new ppc4xx_sdram.c file.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ppc/meson.build    |   3 +-
>   hw/ppc/ppc440_uc.c    | 332 ----------------------------------------
>   hw/ppc/ppc4xx_sdram.c | 348 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 350 insertions(+), 333 deletions(-)
>   create mode 100644 hw/ppc/ppc4xx_sdram.c
> 
> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
> index 32babc9b48..c927337da0 100644
> --- a/hw/ppc/meson.build
> +++ b/hw/ppc/meson.build
> @@ -59,8 +59,9 @@ ppc_ss.add(when: 'CONFIG_PPC440', if_true: files(
>     'ppc440_bamboo.c',
>     'ppc440_pcix.c', 'ppc440_uc.c'))
>   ppc_ss.add(when: 'CONFIG_PPC4XX', if_true: files(
> +  'ppc4xx_devs.c',
>     'ppc4xx_pci.c',
> -  'ppc4xx_devs.c'))
> +  'ppc4xx_sdram.c'))
>   ppc_ss.add(when: 'CONFIG_SAM460EX', if_true: files('sam460ex.c'))
>   # PReP
>   ppc_ss.add(when: 'CONFIG_PREP', if_true: files('prep.c'))
> diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
> index 5fbf44009e..651263926e 100644
> --- a/hw/ppc/ppc440_uc.c
> +++ b/hw/ppc/ppc440_uc.c
> @@ -10,21 +10,14 @@
>   
>   #include "qemu/osdep.h"
>   #include "qemu/units.h"
> -#include "qemu/error-report.h"
>   #include "qapi/error.h"
>   #include "qemu/log.h"
> -#include "qemu/module.h"
>   #include "hw/irq.h"
> -#include "exec/memory.h"
> -#include "cpu.h"
>   #include "hw/ppc/ppc4xx.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/pci/pci.h"
> -#include "sysemu/block-backend.h"
>   #include "sysemu/reset.h"
>   #include "ppc440.h"
> -#include "qom/object.h"
> -#include "trace.h"
>   
>   /*****************************************************************************/
>   /* L2 Cache as SRAM */
> @@ -478,331 +471,6 @@ void ppc4xx_sdr_init(CPUPPCState *env)
>                        sdr, &dcr_read_sdr, &dcr_write_sdr);
>   }
>   
> -/*****************************************************************************/
> -/* SDRAM controller */
> -enum {
> -    SDRAM0_CFGADDR = 0x10,
> -    SDRAM0_CFGDATA,
> -    SDRAM_R0BAS = 0x40,
> -    SDRAM_R1BAS,
> -    SDRAM_R2BAS,
> -    SDRAM_R3BAS,
> -    SDRAM_CONF1HB = 0x45,
> -    SDRAM_PLBADDULL = 0x4a,
> -    SDRAM_CONF1LL = 0x4b,
> -    SDRAM_CONFPATHB = 0x4f,
> -    SDRAM_PLBADDUHB = 0x50,
> -};
> -
> -static uint32_t sdram_ddr2_bcr(hwaddr ram_base, hwaddr ram_size)
> -{
> -    uint32_t bcr;
> -
> -    switch (ram_size) {
> -    case 8 * MiB:
> -        bcr = 0xffc0;
> -        break;
> -    case 16 * MiB:
> -        bcr = 0xff80;
> -        break;
> -    case 32 * MiB:
> -        bcr = 0xff00;
> -        break;
> -    case 64 * MiB:
> -        bcr = 0xfe00;
> -        break;
> -    case 128 * MiB:
> -        bcr = 0xfc00;
> -        break;
> -    case 256 * MiB:
> -        bcr = 0xf800;
> -        break;
> -    case 512 * MiB:
> -        bcr = 0xf000;
> -        break;
> -    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 >> 2 & 0xffe00000;
> -    bcr |= 1;
> -
> -    return bcr;
> -}
> -
> -static inline hwaddr sdram_ddr2_base(uint32_t bcr)
> -{
> -    return (bcr & 0xffe00000) << 2;
> -}
> -
> -static uint64_t sdram_ddr2_size(uint32_t bcr)
> -{
> -    uint64_t size;
> -    int sh;
> -
> -    sh = 1024 - ((bcr >> 6) & 0x3ff);
> -    size = 8 * MiB * sh;
> -
> -    return size;
> -}
> -
> -static void sdram_bank_map(Ppc4xxSdramBank *bank)
> -{
> -    memory_region_init(&bank->container, NULL, "sdram-container", bank->size);
> -    memory_region_add_subregion(&bank->container, 0, &bank->ram);
> -    memory_region_add_subregion(get_system_memory(), bank->base,
> -                                &bank->container);
> -}
> -
> -static void sdram_bank_unmap(Ppc4xxSdramBank *bank)
> -{
> -    memory_region_del_subregion(get_system_memory(), &bank->container);
> -    memory_region_del_subregion(&bank->container, &bank->ram);
> -    object_unparent(OBJECT(&bank->container));
> -}
> -
> -static void sdram_ddr2_set_bcr(Ppc4xxSdramDdr2State *sdram, int i,
> -                               uint32_t bcr, int enabled)
> -{
> -    if (sdram->bank[i].bcr & 1) {
> -        /* First unmap RAM if enabled */
> -        trace_ppc4xx_sdram_unmap(sdram_ddr2_base(sdram->bank[i].bcr),
> -                                 sdram_ddr2_size(sdram->bank[i].bcr));
> -        sdram_bank_unmap(&sdram->bank[i]);
> -    }
> -    sdram->bank[i].bcr = bcr & 0xffe0ffc1;
> -    if (enabled && (bcr & 1)) {
> -        trace_ppc4xx_sdram_map(sdram_ddr2_base(bcr), sdram_ddr2_size(bcr));
> -        sdram_bank_map(&sdram->bank[i]);
> -    }
> -}
> -
> -static void sdram_ddr2_map_bcr(Ppc4xxSdramDdr2State *sdram)
> -{
> -    int i;
> -
> -    for (i = 0; i < sdram->nbanks; i++) {
> -        if (sdram->bank[i].size) {
> -            sdram_ddr2_set_bcr(sdram, i,
> -                               sdram_ddr2_bcr(sdram->bank[i].base,
> -                                              sdram->bank[i].size), 1);
> -        } else {
> -            sdram_ddr2_set_bcr(sdram, i, 0, 0);
> -        }
> -    }
> -}
> -
> -static void sdram_ddr2_unmap_bcr(Ppc4xxSdramDdr2State *sdram)
> -{
> -    int i;
> -
> -    for (i = 0; i < sdram->nbanks; i++) {
> -        if (sdram->bank[i].size) {
> -            sdram_ddr2_set_bcr(sdram, i, sdram->bank[i].bcr & ~1, 0);
> -        }
> -    }
> -}
> -
> -static uint32_t sdram_ddr2_dcr_read(void *opaque, int dcrn)
> -{
> -    Ppc4xxSdramDdr2State *sdram = opaque;
> -    uint32_t ret = 0;
> -
> -    switch (dcrn) {
> -    case SDRAM_R0BAS:
> -    case SDRAM_R1BAS:
> -    case SDRAM_R2BAS:
> -    case SDRAM_R3BAS:
> -        if (sdram->bank[dcrn - SDRAM_R0BAS].size) {
> -            ret = sdram_ddr2_bcr(sdram->bank[dcrn - SDRAM_R0BAS].base,
> -                                 sdram->bank[dcrn - SDRAM_R0BAS].size);
> -        }
> -        break;
> -    case SDRAM_CONF1HB:
> -    case SDRAM_CONF1LL:
> -    case SDRAM_CONFPATHB:
> -    case SDRAM_PLBADDULL:
> -    case SDRAM_PLBADDUHB:
> -        break;
> -    case SDRAM0_CFGADDR:
> -        ret = sdram->addr;
> -        break;
> -    case SDRAM0_CFGDATA:
> -        switch (sdram->addr) {
> -        case 0x14: /* SDRAM_MCSTAT (405EX) */
> -        case 0x1F:
> -            ret = 0x80000000;
> -            break;
> -        case 0x21: /* SDRAM_MCOPT2 */
> -            ret = sdram->mcopt2;
> -            break;
> -        case 0x40: /* SDRAM_MB0CF */
> -            ret = 0x00008001;
> -            break;
> -        case 0x7A: /* SDRAM_DLCR */
> -            ret = 0x02000000;
> -            break;
> -        case 0xE1: /* SDR0_DDR0 */
> -            ret = SDR0_DDR0_DDRM_ENCODE(1) | SDR0_DDR0_DDRM_DDR1;
> -            break;
> -        default:
> -            break;
> -        }
> -        break;
> -    default:
> -        break;
> -    }
> -
> -    return ret;
> -}
> -
> -#define SDRAM_DDR2_MCOPT2_DCEN BIT(27)
> -
> -static void sdram_ddr2_dcr_write(void *opaque, int dcrn, uint32_t val)
> -{
> -    Ppc4xxSdramDdr2State *sdram = opaque;
> -
> -    switch (dcrn) {
> -    case SDRAM_R0BAS:
> -    case SDRAM_R1BAS:
> -    case SDRAM_R2BAS:
> -    case SDRAM_R3BAS:
> -    case SDRAM_CONF1HB:
> -    case SDRAM_CONF1LL:
> -    case SDRAM_CONFPATHB:
> -    case SDRAM_PLBADDULL:
> -    case SDRAM_PLBADDUHB:
> -        break;
> -    case SDRAM0_CFGADDR:
> -        sdram->addr = val;
> -        break;
> -    case SDRAM0_CFGDATA:
> -        switch (sdram->addr) {
> -        case 0x00: /* B0CR */
> -            break;
> -        case 0x21: /* SDRAM_MCOPT2 */
> -            if (!(sdram->mcopt2 & SDRAM_DDR2_MCOPT2_DCEN) &&
> -                (val & SDRAM_DDR2_MCOPT2_DCEN)) {
> -                trace_ppc4xx_sdram_enable("enable");
> -                /* validate all RAM mappings */
> -                sdram_ddr2_map_bcr(sdram);
> -                sdram->mcopt2 |= SDRAM_DDR2_MCOPT2_DCEN;
> -            } else if ((sdram->mcopt2 & SDRAM_DDR2_MCOPT2_DCEN) &&
> -                       !(val & SDRAM_DDR2_MCOPT2_DCEN)) {
> -                trace_ppc4xx_sdram_enable("disable");
> -                /* invalidate all RAM mappings */
> -                sdram_ddr2_unmap_bcr(sdram);
> -                sdram->mcopt2 &= ~SDRAM_DDR2_MCOPT2_DCEN;
> -            }
> -            break;
> -        default:
> -            break;
> -        }
> -        break;
> -    default:
> -        break;
> -    }
> -}
> -
> -static void ppc4xx_sdram_ddr2_reset(DeviceState *dev)
> -{
> -    Ppc4xxSdramDdr2State *sdram = PPC4xx_SDRAM_DDR2(dev);
> -
> -    sdram->addr = 0;
> -    sdram->mcopt2 = 0;
> -}
> -
> -static void ppc4xx_sdram_ddr2_realize(DeviceState *dev, Error **errp)
> -{
> -    Ppc4xxSdramDdr2State *s = PPC4xx_SDRAM_DDR2(dev);
> -    Ppc4xxDcrDeviceState *dcr = PPC4xx_DCR_DEVICE(dev);
> -    /*
> -     * SoC also has 4 GiB but that causes problem with 32 bit
> -     * builds (4*GiB overflows the 32 bit ram_addr_t).
> -     */
> -    const ram_addr_t valid_bank_sizes[] = {
> -        2 * GiB, 1 * GiB, 512 * MiB, 256 * MiB, 128 * MiB,
> -        64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 0
> -    };
> -
> -    if (s->nbanks < 1 || s->nbanks > 4) {
> -        error_setg(errp, "Invalid number of RAM banks");
> -        return;
> -    }
> -    if (!s->dram_mr) {
> -        error_setg(errp, "Missing dram memory region");
> -        return;
> -    }
> -    ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank, valid_bank_sizes);
> -
> -    ppc4xx_dcr_register(dcr, SDRAM0_CFGADDR,
> -                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> -    ppc4xx_dcr_register(dcr, SDRAM0_CFGDATA,
> -                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> -
> -    ppc4xx_dcr_register(dcr, SDRAM_R0BAS,
> -                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> -    ppc4xx_dcr_register(dcr, SDRAM_R1BAS,
> -                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> -    ppc4xx_dcr_register(dcr, SDRAM_R2BAS,
> -                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> -    ppc4xx_dcr_register(dcr, SDRAM_R3BAS,
> -                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> -    ppc4xx_dcr_register(dcr, SDRAM_CONF1HB,
> -                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> -    ppc4xx_dcr_register(dcr, SDRAM_PLBADDULL,
> -                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> -    ppc4xx_dcr_register(dcr, SDRAM_CONF1LL,
> -                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> -    ppc4xx_dcr_register(dcr, SDRAM_CONFPATHB,
> -                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> -    ppc4xx_dcr_register(dcr, SDRAM_PLBADDUHB,
> -                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> -}
> -
> -static Property ppc4xx_sdram_ddr2_props[] = {
> -    DEFINE_PROP_LINK("dram", Ppc4xxSdramDdr2State, dram_mr, TYPE_MEMORY_REGION,
> -                     MemoryRegion *),
> -    DEFINE_PROP_UINT32("nbanks", Ppc4xxSdramDdr2State, nbanks, 4),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
> -static void ppc4xx_sdram_ddr2_class_init(ObjectClass *oc, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(oc);
> -
> -    dc->realize = ppc4xx_sdram_ddr2_realize;
> -    dc->reset = ppc4xx_sdram_ddr2_reset;
> -    /* Reason: only works as function of a ppc4xx SoC */
> -    dc->user_creatable = false;
> -    device_class_set_props(dc, ppc4xx_sdram_ddr2_props);
> -}
> -
> -void ppc4xx_sdram_ddr2_enable(Ppc4xxSdramDdr2State *s)
> -{
> -    sdram_ddr2_dcr_write(s, SDRAM0_CFGADDR, 0x21);
> -    sdram_ddr2_dcr_write(s, SDRAM0_CFGDATA, 0x08000000);
> -}
> -
> -static const TypeInfo ppc4xx_types[] = {
> -    {
> -        .name           = TYPE_PPC4xx_SDRAM_DDR2,
> -        .parent         = TYPE_PPC4xx_DCR_DEVICE,
> -        .instance_size  = sizeof(Ppc4xxSdramDdr2State),
> -        .class_init     = ppc4xx_sdram_ddr2_class_init,
> -    }
> -};
> -DEFINE_TYPES(ppc4xx_types)
> -
>   /*****************************************************************************/
>   /* PLB to AHB bridge */
>   enum {
> diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
> new file mode 100644
> index 0000000000..b49a7ed60a
> --- /dev/null
> +++ b/hw/ppc/ppc4xx_sdram.c
> @@ -0,0 +1,348 @@
> +/*
> + * DDR2 SDRAM controller:
> + * Copyright (c) 2012 François Revol
> + * Copyright (c) 2016-2019 BALATON Zoltan


Shouldn't your Copyright be 2016-2022 for this new file?


The rest LGTM

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>


> + *
> + * This work is licensed under the GNU GPL license version 2 or later.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/units.h"
> +#include "qapi/error.h"
> +#include "exec/address-spaces.h" /* get_system_memory() */
> +#include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/ppc/ppc4xx.h"
> +#include "trace.h"
> +
> +/*****************************************************************************/
> +/* Shared functions */
> +
> +static void sdram_bank_map(Ppc4xxSdramBank *bank)
> +{
> +    memory_region_init(&bank->container, NULL, "sdram-container", bank->size);
> +    memory_region_add_subregion(&bank->container, 0, &bank->ram);
> +    memory_region_add_subregion(get_system_memory(), bank->base,
> +                                &bank->container);
> +}
> +
> +static void sdram_bank_unmap(Ppc4xxSdramBank *bank)
> +{
> +    memory_region_del_subregion(get_system_memory(), &bank->container);
> +    memory_region_del_subregion(&bank->container, &bank->ram);
> +    object_unparent(OBJECT(&bank->container));
> +}
> +
> +enum {
> +    SDRAM0_CFGADDR = 0x010,
> +    SDRAM0_CFGDATA = 0x011,
> +};
> +
> +/*****************************************************************************/
> +/* DDR2 SDRAM controller */
> +enum {
> +    SDRAM_R0BAS = 0x40,
> +    SDRAM_R1BAS,
> +    SDRAM_R2BAS,
> +    SDRAM_R3BAS,
> +    SDRAM_CONF1HB = 0x45,
> +    SDRAM_PLBADDULL = 0x4a,
> +    SDRAM_CONF1LL = 0x4b,
> +    SDRAM_CONFPATHB = 0x4f,
> +    SDRAM_PLBADDUHB = 0x50,
> +};
> +
> +static uint32_t sdram_ddr2_bcr(hwaddr ram_base, hwaddr ram_size)
> +{
> +    uint32_t bcr;
> +
> +    switch (ram_size) {
> +    case 8 * MiB:
> +        bcr = 0xffc0;
> +        break;
> +    case 16 * MiB:
> +        bcr = 0xff80;
> +        break;
> +    case 32 * MiB:
> +        bcr = 0xff00;
> +        break;
> +    case 64 * MiB:
> +        bcr = 0xfe00;
> +        break;
> +    case 128 * MiB:
> +        bcr = 0xfc00;
> +        break;
> +    case 256 * MiB:
> +        bcr = 0xf800;
> +        break;
> +    case 512 * MiB:
> +        bcr = 0xf000;
> +        break;
> +    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 >> 2 & 0xffe00000;
> +    bcr |= 1;
> +
> +    return bcr;
> +}
> +
> +static inline hwaddr sdram_ddr2_base(uint32_t bcr)
> +{
> +    return (bcr & 0xffe00000) << 2;
> +}
> +
> +static uint64_t sdram_ddr2_size(uint32_t bcr)
> +{
> +    uint64_t size;
> +    int sh;
> +
> +    sh = 1024 - ((bcr >> 6) & 0x3ff);
> +    size = 8 * MiB * sh;
> +
> +    return size;
> +}
> +
> +static void sdram_ddr2_set_bcr(Ppc4xxSdramDdr2State *sdram, int i,
> +                               uint32_t bcr, int enabled)
> +{
> +    if (sdram->bank[i].bcr & 1) {
> +        /* First unmap RAM if enabled */
> +        trace_ppc4xx_sdram_unmap(sdram_ddr2_base(sdram->bank[i].bcr),
> +                                 sdram_ddr2_size(sdram->bank[i].bcr));
> +        sdram_bank_unmap(&sdram->bank[i]);
> +    }
> +    sdram->bank[i].bcr = bcr & 0xffe0ffc1;
> +    if (enabled && (bcr & 1)) {
> +        trace_ppc4xx_sdram_map(sdram_ddr2_base(bcr), sdram_ddr2_size(bcr));
> +        sdram_bank_map(&sdram->bank[i]);
> +    }
> +}
> +
> +static void sdram_ddr2_map_bcr(Ppc4xxSdramDdr2State *sdram)
> +{
> +    int i;
> +
> +    for (i = 0; i < sdram->nbanks; i++) {
> +        if (sdram->bank[i].size) {
> +            sdram_ddr2_set_bcr(sdram, i,
> +                               sdram_ddr2_bcr(sdram->bank[i].base,
> +                                              sdram->bank[i].size), 1);
> +        } else {
> +            sdram_ddr2_set_bcr(sdram, i, 0, 0);
> +        }
> +    }
> +}
> +
> +static void sdram_ddr2_unmap_bcr(Ppc4xxSdramDdr2State *sdram)
> +{
> +    int i;
> +
> +    for (i = 0; i < sdram->nbanks; i++) {
> +        if (sdram->bank[i].size) {
> +            sdram_ddr2_set_bcr(sdram, i, sdram->bank[i].bcr & ~1, 0);
> +        }
> +    }
> +}
> +
> +static uint32_t sdram_ddr2_dcr_read(void *opaque, int dcrn)
> +{
> +    Ppc4xxSdramDdr2State *sdram = opaque;
> +    uint32_t ret = 0;
> +
> +    switch (dcrn) {
> +    case SDRAM_R0BAS:
> +    case SDRAM_R1BAS:
> +    case SDRAM_R2BAS:
> +    case SDRAM_R3BAS:
> +        if (sdram->bank[dcrn - SDRAM_R0BAS].size) {
> +            ret = sdram_ddr2_bcr(sdram->bank[dcrn - SDRAM_R0BAS].base,
> +                                 sdram->bank[dcrn - SDRAM_R0BAS].size);
> +        }
> +        break;
> +    case SDRAM_CONF1HB:
> +    case SDRAM_CONF1LL:
> +    case SDRAM_CONFPATHB:
> +    case SDRAM_PLBADDULL:
> +    case SDRAM_PLBADDUHB:
> +        break;
> +    case SDRAM0_CFGADDR:
> +        ret = sdram->addr;
> +        break;
> +    case SDRAM0_CFGDATA:
> +        switch (sdram->addr) {
> +        case 0x14: /* SDRAM_MCSTAT (405EX) */
> +        case 0x1F:
> +            ret = 0x80000000;
> +            break;
> +        case 0x21: /* SDRAM_MCOPT2 */
> +            ret = sdram->mcopt2;
> +            break;
> +        case 0x40: /* SDRAM_MB0CF */
> +            ret = 0x00008001;
> +            break;
> +        case 0x7A: /* SDRAM_DLCR */
> +            ret = 0x02000000;
> +            break;
> +        case 0xE1: /* SDR0_DDR0 */
> +            ret = SDR0_DDR0_DDRM_ENCODE(1) | SDR0_DDR0_DDRM_DDR1;
> +            break;
> +        default:
> +            break;
> +        }
> +        break;
> +    default:
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +#define SDRAM_DDR2_MCOPT2_DCEN BIT(27)
> +
> +static void sdram_ddr2_dcr_write(void *opaque, int dcrn, uint32_t val)
> +{
> +    Ppc4xxSdramDdr2State *sdram = opaque;
> +
> +    switch (dcrn) {
> +    case SDRAM_R0BAS:
> +    case SDRAM_R1BAS:
> +    case SDRAM_R2BAS:
> +    case SDRAM_R3BAS:
> +    case SDRAM_CONF1HB:
> +    case SDRAM_CONF1LL:
> +    case SDRAM_CONFPATHB:
> +    case SDRAM_PLBADDULL:
> +    case SDRAM_PLBADDUHB:
> +        break;
> +    case SDRAM0_CFGADDR:
> +        sdram->addr = val;
> +        break;
> +    case SDRAM0_CFGDATA:
> +        switch (sdram->addr) {
> +        case 0x00: /* B0CR */
> +            break;
> +        case 0x21: /* SDRAM_MCOPT2 */
> +            if (!(sdram->mcopt2 & SDRAM_DDR2_MCOPT2_DCEN) &&
> +                (val & SDRAM_DDR2_MCOPT2_DCEN)) {
> +                trace_ppc4xx_sdram_enable("enable");
> +                /* validate all RAM mappings */
> +                sdram_ddr2_map_bcr(sdram);
> +                sdram->mcopt2 |= SDRAM_DDR2_MCOPT2_DCEN;
> +            } else if ((sdram->mcopt2 & SDRAM_DDR2_MCOPT2_DCEN) &&
> +                       !(val & SDRAM_DDR2_MCOPT2_DCEN)) {
> +                trace_ppc4xx_sdram_enable("disable");
> +                /* invalidate all RAM mappings */
> +                sdram_ddr2_unmap_bcr(sdram);
> +                sdram->mcopt2 &= ~SDRAM_DDR2_MCOPT2_DCEN;
> +            }
> +            break;
> +        default:
> +            break;
> +        }
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
> +static void ppc4xx_sdram_ddr2_reset(DeviceState *dev)
> +{
> +    Ppc4xxSdramDdr2State *sdram = PPC4xx_SDRAM_DDR2(dev);
> +
> +    sdram->addr = 0;
> +    sdram->mcopt2 = 0;
> +}
> +
> +static void ppc4xx_sdram_ddr2_realize(DeviceState *dev, Error **errp)
> +{
> +    Ppc4xxSdramDdr2State *s = PPC4xx_SDRAM_DDR2(dev);
> +    Ppc4xxDcrDeviceState *dcr = PPC4xx_DCR_DEVICE(dev);
> +    /*
> +     * SoC also has 4 GiB but that causes problem with 32 bit
> +     * builds (4*GiB overflows the 32 bit ram_addr_t).
> +     */
> +    const ram_addr_t valid_bank_sizes[] = {
> +        2 * GiB, 1 * GiB, 512 * MiB, 256 * MiB, 128 * MiB,
> +        64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 0
> +    };
> +
> +    if (s->nbanks < 1 || s->nbanks > 4) {
> +        error_setg(errp, "Invalid number of RAM banks");
> +        return;
> +    }
> +    if (!s->dram_mr) {
> +        error_setg(errp, "Missing dram memory region");
> +        return;
> +    }
> +    ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank, valid_bank_sizes);
> +
> +    ppc4xx_dcr_register(dcr, SDRAM0_CFGADDR,
> +                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> +    ppc4xx_dcr_register(dcr, SDRAM0_CFGDATA,
> +                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> +
> +    ppc4xx_dcr_register(dcr, SDRAM_R0BAS,
> +                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> +    ppc4xx_dcr_register(dcr, SDRAM_R1BAS,
> +                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> +    ppc4xx_dcr_register(dcr, SDRAM_R2BAS,
> +                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> +    ppc4xx_dcr_register(dcr, SDRAM_R3BAS,
> +                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> +    ppc4xx_dcr_register(dcr, SDRAM_CONF1HB,
> +                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> +    ppc4xx_dcr_register(dcr, SDRAM_PLBADDULL,
> +                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> +    ppc4xx_dcr_register(dcr, SDRAM_CONF1LL,
> +                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> +    ppc4xx_dcr_register(dcr, SDRAM_CONFPATHB,
> +                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> +    ppc4xx_dcr_register(dcr, SDRAM_PLBADDUHB,
> +                        s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> +}
> +
> +static Property ppc4xx_sdram_ddr2_props[] = {
> +    DEFINE_PROP_LINK("dram", Ppc4xxSdramDdr2State, dram_mr, TYPE_MEMORY_REGION,
> +                     MemoryRegion *),
> +    DEFINE_PROP_UINT32("nbanks", Ppc4xxSdramDdr2State, nbanks, 4),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void ppc4xx_sdram_ddr2_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = ppc4xx_sdram_ddr2_realize;
> +    dc->reset = ppc4xx_sdram_ddr2_reset;
> +    /* Reason: only works as function of a ppc4xx SoC */
> +    dc->user_creatable = false;
> +    device_class_set_props(dc, ppc4xx_sdram_ddr2_props);
> +}
> +
> +void ppc4xx_sdram_ddr2_enable(Ppc4xxSdramDdr2State *s)
> +{
> +    sdram_ddr2_dcr_write(s, SDRAM0_CFGADDR, 0x21);
> +    sdram_ddr2_dcr_write(s, SDRAM0_CFGDATA, 0x08000000);
> +}
> +
> +static const TypeInfo ppc4xx_sdram_types[] = {
> +    {
> +        .name           = TYPE_PPC4xx_SDRAM_DDR2,
> +        .parent         = TYPE_PPC4xx_DCR_DEVICE,
> +        .instance_size  = sizeof(Ppc4xxSdramDdr2State),
> +        .class_init     = ppc4xx_sdram_ddr2_class_init,
> +    }
> +};
> +
> +DEFINE_TYPES(ppc4xx_sdram_types)

Re: [PATCH v7 1/8] ppc440_uc.c: Move DDR2 SDRAM controller model to ppc4xx_sdram.c

[BALATON Zoltan]

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

On Fri, 21 Oct 2022, Daniel Henrique Barboza wrote:
>> diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
>> new file mode 100644
>> index 0000000000..b49a7ed60a
>> --- /dev/null
>> +++ b/hw/ppc/ppc4xx_sdram.c
>> @@ -0,0 +1,348 @@
>> +/*
>> + * DDR2 SDRAM controller:
>> + * Copyright (c) 2012 François Revol
>> + * Copyright (c) 2016-2019 BALATON Zoltan
>
>
> Shouldn't your Copyright be 2016-2022 for this new file?
>
>
> The rest LGTM
>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Thanks for the review. I'm not sure about the date. I did not intend to 
increment this every year or with every little change. It mainly records 
the time it was originally wrritten or when major changes were made. I 
can't decide if these changes in this series are big enough to need a new 
copyright date but I don't mind either way so I let you decide. I'm OK 
with it as it is or also if you update it on merge.

Regards,
BALATON Zoltan

Re: [PATCH v7 1/8] ppc440_uc.c: Move DDR2 SDRAM controller model to ppc4xx_sdram.c

[Daniel Henrique Barboza]

On 10/21/22 21:09, BALATON Zoltan wrote:
> On Fri, 21 Oct 2022, Daniel Henrique Barboza wrote:
>>> diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
>>> new file mode 100644
>>> index 0000000000..b49a7ed60a
>>> --- /dev/null
>>> +++ b/hw/ppc/ppc4xx_sdram.c
>>> @@ -0,0 +1,348 @@
>>> +/*
>>> + * DDR2 SDRAM controller:
>>> + * Copyright (c) 2012 François Revol
>>> + * Copyright (c) 2016-2019 BALATON Zoltan
>>
>>
>> Shouldn't your Copyright be 2016-2022 for this new file?
>>
>>
>> The rest LGTM
>>
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> Thanks for the review. I'm not sure about the date. I did not intend to increment this every year or with every little change. It mainly records the time it was originally wrritten or when major changes were made. I can't decide if these changes in this series are big enough to need a new copyright date but I don't mind either way so I let you decide. I'm OK with it as it is or also if you update it on merge.

Let's leave it that way then.


Thanks,

Daniel

> 
> Regards,
> BALATON Zoltan

[PATCH v7 2/8] ppc4xx_devs.c: Move DDR SDRAM controller model to ppc4xx_sdram.c

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/ppc4xx_devs.c  | 352 ----------------------------------------
 hw/ppc/ppc4xx_sdram.c | 365 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 365 insertions(+), 352 deletions(-)

diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index 12af90f244..f737dbb3d6 100644
--- a/hw/ppc/ppc4xx_devs.c
+++ b/hw/ppc/ppc4xx_devs.c
@@ -24,357 +24,10 @@
 
 #include "qemu/osdep.h"
 #include "qemu/units.h"
-#include "sysemu/reset.h"
 #include "cpu.h"
-#include "hw/irq.h"
-#include "hw/ppc/ppc.h"
 #include "hw/ppc/ppc4xx.h"
 #include "hw/qdev-properties.h"
-#include "qemu/log.h"
-#include "exec/address-spaces.h"
-#include "qemu/error-report.h"
 #include "qapi/error.h"
-#include "trace.h"
-
-/*****************************************************************************/
-/* SDRAM controller */
-enum {
-    SDRAM0_CFGADDR = 0x010,
-    SDRAM0_CFGDATA = 0x011,
-};
-
-/*
- * 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_ddr_bcr(hwaddr ram_base, hwaddr ram_size)
-{
-    uint32_t bcr;
-
-    switch (ram_size) {
-    case 4 * MiB:
-        bcr = 0;
-        break;
-    case 8 * MiB:
-        bcr = 0x20000;
-        break;
-    case 16 * MiB:
-        bcr = 0x40000;
-        break;
-    case 32 * MiB:
-        bcr = 0x60000;
-        break;
-    case 64 * MiB:
-        bcr = 0x80000;
-        break;
-    case 128 * MiB:
-        bcr = 0xA0000;
-        break;
-    case 256 * MiB:
-        bcr = 0xC0000;
-        break;
-    default:
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: invalid RAM size 0x%" HWADDR_PRIx "\n", __func__,
-                      ram_size);
-        return 0;
-    }
-    bcr |= ram_base & 0xFF800000;
-    bcr |= 1;
-
-    return bcr;
-}
-
-static inline hwaddr sdram_ddr_base(uint32_t bcr)
-{
-    return bcr & 0xFF800000;
-}
-
-static target_ulong sdram_ddr_size(uint32_t bcr)
-{
-    target_ulong size;
-    int sh;
-
-    sh = (bcr >> 17) & 0x7;
-    if (sh == 7) {
-        size = -1;
-    } else {
-        size = (4 * MiB) << sh;
-    }
-
-    return size;
-}
-
-static void sdram_ddr_set_bcr(Ppc4xxSdramDdrState *sdram, int i,
-                              uint32_t bcr, int enabled)
-{
-    if (sdram->bank[i].bcr & 1) {
-        /* Unmap RAM */
-        trace_ppc4xx_sdram_unmap(sdram_ddr_base(sdram->bank[i].bcr),
-                                 sdram_ddr_size(sdram->bank[i].bcr));
-        memory_region_del_subregion(get_system_memory(),
-                                    &sdram->bank[i].container);
-        memory_region_del_subregion(&sdram->bank[i].container,
-                                    &sdram->bank[i].ram);
-        object_unparent(OBJECT(&sdram->bank[i].container));
-    }
-    sdram->bank[i].bcr = bcr & 0xFFDEE001;
-    if (enabled && (bcr & 1)) {
-        trace_ppc4xx_sdram_map(sdram_ddr_base(bcr), sdram_ddr_size(bcr));
-        memory_region_init(&sdram->bank[i].container, NULL, "sdram-container",
-                           sdram_ddr_size(bcr));
-        memory_region_add_subregion(&sdram->bank[i].container, 0,
-                                    &sdram->bank[i].ram);
-        memory_region_add_subregion(get_system_memory(),
-                                    sdram_ddr_base(bcr),
-                                    &sdram->bank[i].container);
-    }
-}
-
-static void sdram_ddr_map_bcr(Ppc4xxSdramDdrState *sdram)
-{
-    int i;
-
-    for (i = 0; i < sdram->nbanks; i++) {
-        if (sdram->bank[i].size != 0) {
-            sdram_ddr_set_bcr(sdram, i, sdram_ddr_bcr(sdram->bank[i].base,
-                                                      sdram->bank[i].size), 1);
-        } else {
-            sdram_ddr_set_bcr(sdram, i, 0, 0);
-        }
-    }
-}
-
-static void sdram_ddr_unmap_bcr(Ppc4xxSdramDdrState *sdram)
-{
-    int i;
-
-    for (i = 0; i < sdram->nbanks; i++) {
-        trace_ppc4xx_sdram_unmap(sdram_ddr_base(sdram->bank[i].bcr),
-                                 sdram_ddr_size(sdram->bank[i].bcr));
-        memory_region_del_subregion(get_system_memory(),
-                                    &sdram->bank[i].ram);
-    }
-}
-
-static uint32_t sdram_ddr_dcr_read(void *opaque, int dcrn)
-{
-    Ppc4xxSdramDdrState *sdram = opaque;
-    uint32_t ret;
-
-    switch (dcrn) {
-    case SDRAM0_CFGADDR:
-        ret = sdram->addr;
-        break;
-    case SDRAM0_CFGDATA:
-        switch (sdram->addr) {
-        case 0x00: /* SDRAM_BESR0 */
-            ret = sdram->besr0;
-            break;
-        case 0x08: /* SDRAM_BESR1 */
-            ret = sdram->besr1;
-            break;
-        case 0x10: /* SDRAM_BEAR */
-            ret = sdram->bear;
-            break;
-        case 0x20: /* SDRAM_CFG */
-            ret = sdram->cfg;
-            break;
-        case 0x24: /* SDRAM_STATUS */
-            ret = sdram->status;
-            break;
-        case 0x30: /* SDRAM_RTR */
-            ret = sdram->rtr;
-            break;
-        case 0x34: /* SDRAM_PMIT */
-            ret = sdram->pmit;
-            break;
-        case 0x40: /* SDRAM_B0CR */
-            ret = sdram->bank[0].bcr;
-            break;
-        case 0x44: /* SDRAM_B1CR */
-            ret = sdram->bank[1].bcr;
-            break;
-        case 0x48: /* SDRAM_B2CR */
-            ret = sdram->bank[2].bcr;
-            break;
-        case 0x4C: /* SDRAM_B3CR */
-            ret = sdram->bank[3].bcr;
-            break;
-        case 0x80: /* SDRAM_TR */
-            ret = -1; /* ? */
-            break;
-        case 0x94: /* SDRAM_ECCCFG */
-            ret = sdram->ecccfg;
-            break;
-        case 0x98: /* SDRAM_ECCESR */
-            ret = sdram->eccesr;
-            break;
-        default: /* Error */
-            ret = -1;
-            break;
-        }
-        break;
-    default:
-        /* Avoid gcc warning */
-        ret = 0;
-        break;
-    }
-
-    return ret;
-}
-
-static void sdram_ddr_dcr_write(void *opaque, int dcrn, uint32_t val)
-{
-    Ppc4xxSdramDdrState *sdram = opaque;
-
-    switch (dcrn) {
-    case SDRAM0_CFGADDR:
-        sdram->addr = val;
-        break;
-    case SDRAM0_CFGDATA:
-        switch (sdram->addr) {
-        case 0x00: /* SDRAM_BESR0 */
-            sdram->besr0 &= ~val;
-            break;
-        case 0x08: /* SDRAM_BESR1 */
-            sdram->besr1 &= ~val;
-            break;
-        case 0x10: /* SDRAM_BEAR */
-            sdram->bear = val;
-            break;
-        case 0x20: /* SDRAM_CFG */
-            val &= 0xFFE00000;
-            if (!(sdram->cfg & 0x80000000) && (val & 0x80000000)) {
-                trace_ppc4xx_sdram_enable("enable");
-                /* validate all RAM mappings */
-                sdram_ddr_map_bcr(sdram);
-                sdram->status &= ~0x80000000;
-            } else if ((sdram->cfg & 0x80000000) && !(val & 0x80000000)) {
-                trace_ppc4xx_sdram_enable("disable");
-                /* invalidate all RAM mappings */
-                sdram_ddr_unmap_bcr(sdram);
-                sdram->status |= 0x80000000;
-            }
-            if (!(sdram->cfg & 0x40000000) && (val & 0x40000000)) {
-                sdram->status |= 0x40000000;
-            } else if ((sdram->cfg & 0x40000000) && !(val & 0x40000000)) {
-                sdram->status &= ~0x40000000;
-            }
-            sdram->cfg = val;
-            break;
-        case 0x24: /* SDRAM_STATUS */
-            /* Read-only register */
-            break;
-        case 0x30: /* SDRAM_RTR */
-            sdram->rtr = val & 0x3FF80000;
-            break;
-        case 0x34: /* SDRAM_PMIT */
-            sdram->pmit = (val & 0xF8000000) | 0x07C00000;
-            break;
-        case 0x40: /* SDRAM_B0CR */
-            sdram_ddr_set_bcr(sdram, 0, val, sdram->cfg & 0x80000000);
-            break;
-        case 0x44: /* SDRAM_B1CR */
-            sdram_ddr_set_bcr(sdram, 1, val, sdram->cfg & 0x80000000);
-            break;
-        case 0x48: /* SDRAM_B2CR */
-            sdram_ddr_set_bcr(sdram, 2, val, sdram->cfg & 0x80000000);
-            break;
-        case 0x4C: /* SDRAM_B3CR */
-            sdram_ddr_set_bcr(sdram, 3, val, sdram->cfg & 0x80000000);
-            break;
-        case 0x80: /* SDRAM_TR */
-            sdram->tr = val & 0x018FC01F;
-            break;
-        case 0x94: /* SDRAM_ECCCFG */
-            sdram->ecccfg = val & 0x00F00000;
-            break;
-        case 0x98: /* SDRAM_ECCESR */
-            val &= 0xFFF0F000;
-            if (sdram->eccesr == 0 && val != 0) {
-                qemu_irq_raise(sdram->irq);
-            } else if (sdram->eccesr != 0 && val == 0) {
-                qemu_irq_lower(sdram->irq);
-            }
-            sdram->eccesr = val;
-            break;
-        default: /* Error */
-            break;
-        }
-        break;
-    }
-}
-
-static void ppc4xx_sdram_ddr_reset(DeviceState *dev)
-{
-    Ppc4xxSdramDdrState *sdram = PPC4xx_SDRAM_DDR(dev);
-
-    sdram->addr = 0;
-    sdram->bear = 0;
-    sdram->besr0 = 0; /* No error */
-    sdram->besr1 = 0; /* No error */
-    sdram->cfg = 0;
-    sdram->ecccfg = 0; /* No ECC */
-    sdram->eccesr = 0; /* No error */
-    sdram->pmit = 0x07C00000;
-    sdram->rtr = 0x05F00000;
-    sdram->tr = 0x00854009;
-    /* We pre-initialize RAM banks */
-    sdram->status = 0;
-    sdram->cfg = 0x00800000;
-}
-
-static void ppc4xx_sdram_ddr_realize(DeviceState *dev, Error **errp)
-{
-    Ppc4xxSdramDdrState *s = PPC4xx_SDRAM_DDR(dev);
-    Ppc4xxDcrDeviceState *dcr = PPC4xx_DCR_DEVICE(dev);
-    const ram_addr_t valid_bank_sizes[] = {
-        256 * MiB, 128 * MiB, 64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 4 * MiB, 0
-    };
-
-    if (s->nbanks < 1 || s->nbanks > 4) {
-        error_setg(errp, "Invalid number of RAM banks");
-        return;
-    }
-    if (!s->dram_mr) {
-        error_setg(errp, "Missing dram memory region");
-        return;
-    }
-    ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank, valid_bank_sizes);
-
-    sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
-
-    ppc4xx_dcr_register(dcr, SDRAM0_CFGADDR,
-                        s, &sdram_ddr_dcr_read, &sdram_ddr_dcr_write);
-    ppc4xx_dcr_register(dcr, SDRAM0_CFGDATA,
-                        s, &sdram_ddr_dcr_read, &sdram_ddr_dcr_write);
-}
-
-static Property ppc4xx_sdram_ddr_props[] = {
-    DEFINE_PROP_LINK("dram", Ppc4xxSdramDdrState, dram_mr, TYPE_MEMORY_REGION,
-                     MemoryRegion *),
-    DEFINE_PROP_UINT32("nbanks", Ppc4xxSdramDdrState, nbanks, 4),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void ppc4xx_sdram_ddr_class_init(ObjectClass *oc, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(oc);
-
-    dc->realize = ppc4xx_sdram_ddr_realize;
-    dc->reset = ppc4xx_sdram_ddr_reset;
-    /* Reason: only works as function of a ppc4xx SoC */
-    dc->user_creatable = false;
-    device_class_set_props(dc, ppc4xx_sdram_ddr_props);
-}
-
-void ppc4xx_sdram_ddr_enable(Ppc4xxSdramDdrState *s)
-{
-    sdram_ddr_dcr_write(s, SDRAM0_CFGADDR, 0x20);
-    sdram_ddr_dcr_write(s, SDRAM0_CFGDATA, 0x80000000);
-}
 
 /*
  * Split RAM between SDRAM banks.
@@ -963,11 +616,6 @@ static void ppc4xx_dcr_class_init(ObjectClass *oc, void *data)
 
 static const TypeInfo ppc4xx_types[] = {
     {
-        .name           = TYPE_PPC4xx_SDRAM_DDR,
-        .parent         = TYPE_PPC4xx_DCR_DEVICE,
-        .instance_size  = sizeof(Ppc4xxSdramDdrState),
-        .class_init     = ppc4xx_sdram_ddr_class_init,
-    }, {
         .name           = TYPE_PPC4xx_MAL,
         .parent         = TYPE_PPC4xx_DCR_DEVICE,
         .instance_size  = sizeof(Ppc4xxMalState),
diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
index b49a7ed60a..d88363bc3d 100644
--- a/hw/ppc/ppc4xx_sdram.c
+++ b/hw/ppc/ppc4xx_sdram.c
@@ -1,4 +1,27 @@
 /*
+ * QEMU PowerPC 4xx embedded processors SDRAM controller emulation
+ *
+ * DDR SDRAM controller:
+ * Copyright (c) 2007 Jocelyn Mayer
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
  * DDR2 SDRAM controller:
  * Copyright (c) 2012 François Revol
  * Copyright (c) 2016-2019 BALATON Zoltan
@@ -9,7 +32,9 @@
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "qapi/error.h"
+#include "qemu/log.h"
 #include "exec/address-spaces.h" /* get_system_memory() */
+#include "exec/cpu-defs.h" /* target_ulong */
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "hw/ppc/ppc4xx.h"
@@ -38,6 +63,341 @@ enum {
     SDRAM0_CFGDATA = 0x011,
 };
 
+/*****************************************************************************/
+/* DDR SDRAM controller */
+/*
+ * 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_ddr_bcr(hwaddr ram_base, hwaddr ram_size)
+{
+    uint32_t bcr;
+
+    switch (ram_size) {
+    case 4 * MiB:
+        bcr = 0;
+        break;
+    case 8 * MiB:
+        bcr = 0x20000;
+        break;
+    case 16 * MiB:
+        bcr = 0x40000;
+        break;
+    case 32 * MiB:
+        bcr = 0x60000;
+        break;
+    case 64 * MiB:
+        bcr = 0x80000;
+        break;
+    case 128 * MiB:
+        bcr = 0xA0000;
+        break;
+    case 256 * MiB:
+        bcr = 0xC0000;
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid RAM size 0x%" HWADDR_PRIx "\n", __func__,
+                      ram_size);
+        return 0;
+    }
+    bcr |= ram_base & 0xFF800000;
+    bcr |= 1;
+
+    return bcr;
+}
+
+static inline hwaddr sdram_ddr_base(uint32_t bcr)
+{
+    return bcr & 0xFF800000;
+}
+
+static target_ulong sdram_ddr_size(uint32_t bcr)
+{
+    target_ulong size;
+    int sh;
+
+    sh = (bcr >> 17) & 0x7;
+    if (sh == 7) {
+        size = -1;
+    } else {
+        size = (4 * MiB) << sh;
+    }
+
+    return size;
+}
+
+static void sdram_ddr_set_bcr(Ppc4xxSdramDdrState *sdram, int i,
+                              uint32_t bcr, int enabled)
+{
+    if (sdram->bank[i].bcr & 1) {
+        /* Unmap RAM */
+        trace_ppc4xx_sdram_unmap(sdram_ddr_base(sdram->bank[i].bcr),
+                                 sdram_ddr_size(sdram->bank[i].bcr));
+        memory_region_del_subregion(get_system_memory(),
+                                    &sdram->bank[i].container);
+        memory_region_del_subregion(&sdram->bank[i].container,
+                                    &sdram->bank[i].ram);
+        object_unparent(OBJECT(&sdram->bank[i].container));
+    }
+    sdram->bank[i].bcr = bcr & 0xFFDEE001;
+    if (enabled && (bcr & 1)) {
+        trace_ppc4xx_sdram_map(sdram_ddr_base(bcr), sdram_ddr_size(bcr));
+        memory_region_init(&sdram->bank[i].container, NULL, "sdram-container",
+                           sdram_ddr_size(bcr));
+        memory_region_add_subregion(&sdram->bank[i].container, 0,
+                                    &sdram->bank[i].ram);
+        memory_region_add_subregion(get_system_memory(),
+                                    sdram_ddr_base(bcr),
+                                    &sdram->bank[i].container);
+    }
+}
+
+static void sdram_ddr_map_bcr(Ppc4xxSdramDdrState *sdram)
+{
+    int i;
+
+    for (i = 0; i < sdram->nbanks; i++) {
+        if (sdram->bank[i].size != 0) {
+            sdram_ddr_set_bcr(sdram, i, sdram_ddr_bcr(sdram->bank[i].base,
+                                                      sdram->bank[i].size), 1);
+        } else {
+            sdram_ddr_set_bcr(sdram, i, 0, 0);
+        }
+    }
+}
+
+static void sdram_ddr_unmap_bcr(Ppc4xxSdramDdrState *sdram)
+{
+    int i;
+
+    for (i = 0; i < sdram->nbanks; i++) {
+        trace_ppc4xx_sdram_unmap(sdram_ddr_base(sdram->bank[i].bcr),
+                                 sdram_ddr_size(sdram->bank[i].bcr));
+        memory_region_del_subregion(get_system_memory(),
+                                    &sdram->bank[i].ram);
+    }
+}
+
+static uint32_t sdram_ddr_dcr_read(void *opaque, int dcrn)
+{
+    Ppc4xxSdramDdrState *sdram = opaque;
+    uint32_t ret;
+
+    switch (dcrn) {
+    case SDRAM0_CFGADDR:
+        ret = sdram->addr;
+        break;
+    case SDRAM0_CFGDATA:
+        switch (sdram->addr) {
+        case 0x00: /* SDRAM_BESR0 */
+            ret = sdram->besr0;
+            break;
+        case 0x08: /* SDRAM_BESR1 */
+            ret = sdram->besr1;
+            break;
+        case 0x10: /* SDRAM_BEAR */
+            ret = sdram->bear;
+            break;
+        case 0x20: /* SDRAM_CFG */
+            ret = sdram->cfg;
+            break;
+        case 0x24: /* SDRAM_STATUS */
+            ret = sdram->status;
+            break;
+        case 0x30: /* SDRAM_RTR */
+            ret = sdram->rtr;
+            break;
+        case 0x34: /* SDRAM_PMIT */
+            ret = sdram->pmit;
+            break;
+        case 0x40: /* SDRAM_B0CR */
+            ret = sdram->bank[0].bcr;
+            break;
+        case 0x44: /* SDRAM_B1CR */
+            ret = sdram->bank[1].bcr;
+            break;
+        case 0x48: /* SDRAM_B2CR */
+            ret = sdram->bank[2].bcr;
+            break;
+        case 0x4C: /* SDRAM_B3CR */
+            ret = sdram->bank[3].bcr;
+            break;
+        case 0x80: /* SDRAM_TR */
+            ret = -1; /* ? */
+            break;
+        case 0x94: /* SDRAM_ECCCFG */
+            ret = sdram->ecccfg;
+            break;
+        case 0x98: /* SDRAM_ECCESR */
+            ret = sdram->eccesr;
+            break;
+        default: /* Error */
+            ret = -1;
+            break;
+        }
+        break;
+    default:
+        /* Avoid gcc warning */
+        ret = 0;
+        break;
+    }
+
+    return ret;
+}
+
+static void sdram_ddr_dcr_write(void *opaque, int dcrn, uint32_t val)
+{
+    Ppc4xxSdramDdrState *sdram = opaque;
+
+    switch (dcrn) {
+    case SDRAM0_CFGADDR:
+        sdram->addr = val;
+        break;
+    case SDRAM0_CFGDATA:
+        switch (sdram->addr) {
+        case 0x00: /* SDRAM_BESR0 */
+            sdram->besr0 &= ~val;
+            break;
+        case 0x08: /* SDRAM_BESR1 */
+            sdram->besr1 &= ~val;
+            break;
+        case 0x10: /* SDRAM_BEAR */
+            sdram->bear = val;
+            break;
+        case 0x20: /* SDRAM_CFG */
+            val &= 0xFFE00000;
+            if (!(sdram->cfg & 0x80000000) && (val & 0x80000000)) {
+                trace_ppc4xx_sdram_enable("enable");
+                /* validate all RAM mappings */
+                sdram_ddr_map_bcr(sdram);
+                sdram->status &= ~0x80000000;
+            } else if ((sdram->cfg & 0x80000000) && !(val & 0x80000000)) {
+                trace_ppc4xx_sdram_enable("disable");
+                /* invalidate all RAM mappings */
+                sdram_ddr_unmap_bcr(sdram);
+                sdram->status |= 0x80000000;
+            }
+            if (!(sdram->cfg & 0x40000000) && (val & 0x40000000)) {
+                sdram->status |= 0x40000000;
+            } else if ((sdram->cfg & 0x40000000) && !(val & 0x40000000)) {
+                sdram->status &= ~0x40000000;
+            }
+            sdram->cfg = val;
+            break;
+        case 0x24: /* SDRAM_STATUS */
+            /* Read-only register */
+            break;
+        case 0x30: /* SDRAM_RTR */
+            sdram->rtr = val & 0x3FF80000;
+            break;
+        case 0x34: /* SDRAM_PMIT */
+            sdram->pmit = (val & 0xF8000000) | 0x07C00000;
+            break;
+        case 0x40: /* SDRAM_B0CR */
+            sdram_ddr_set_bcr(sdram, 0, val, sdram->cfg & 0x80000000);
+            break;
+        case 0x44: /* SDRAM_B1CR */
+            sdram_ddr_set_bcr(sdram, 1, val, sdram->cfg & 0x80000000);
+            break;
+        case 0x48: /* SDRAM_B2CR */
+            sdram_ddr_set_bcr(sdram, 2, val, sdram->cfg & 0x80000000);
+            break;
+        case 0x4C: /* SDRAM_B3CR */
+            sdram_ddr_set_bcr(sdram, 3, val, sdram->cfg & 0x80000000);
+            break;
+        case 0x80: /* SDRAM_TR */
+            sdram->tr = val & 0x018FC01F;
+            break;
+        case 0x94: /* SDRAM_ECCCFG */
+            sdram->ecccfg = val & 0x00F00000;
+            break;
+        case 0x98: /* SDRAM_ECCESR */
+            val &= 0xFFF0F000;
+            if (sdram->eccesr == 0 && val != 0) {
+                qemu_irq_raise(sdram->irq);
+            } else if (sdram->eccesr != 0 && val == 0) {
+                qemu_irq_lower(sdram->irq);
+            }
+            sdram->eccesr = val;
+            break;
+        default: /* Error */
+            break;
+        }
+        break;
+    }
+}
+
+static void ppc4xx_sdram_ddr_reset(DeviceState *dev)
+{
+    Ppc4xxSdramDdrState *sdram = PPC4xx_SDRAM_DDR(dev);
+
+    sdram->addr = 0;
+    sdram->bear = 0;
+    sdram->besr0 = 0; /* No error */
+    sdram->besr1 = 0; /* No error */
+    sdram->cfg = 0;
+    sdram->ecccfg = 0; /* No ECC */
+    sdram->eccesr = 0; /* No error */
+    sdram->pmit = 0x07C00000;
+    sdram->rtr = 0x05F00000;
+    sdram->tr = 0x00854009;
+    /* We pre-initialize RAM banks */
+    sdram->status = 0;
+    sdram->cfg = 0x00800000;
+}
+
+static void ppc4xx_sdram_ddr_realize(DeviceState *dev, Error **errp)
+{
+    Ppc4xxSdramDdrState *s = PPC4xx_SDRAM_DDR(dev);
+    Ppc4xxDcrDeviceState *dcr = PPC4xx_DCR_DEVICE(dev);
+    const ram_addr_t valid_bank_sizes[] = {
+        256 * MiB, 128 * MiB, 64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 4 * MiB, 0
+    };
+
+    if (s->nbanks < 1 || s->nbanks > 4) {
+        error_setg(errp, "Invalid number of RAM banks");
+        return;
+    }
+    if (!s->dram_mr) {
+        error_setg(errp, "Missing dram memory region");
+        return;
+    }
+    ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank, valid_bank_sizes);
+
+    sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
+
+    ppc4xx_dcr_register(dcr, SDRAM0_CFGADDR,
+                        s, &sdram_ddr_dcr_read, &sdram_ddr_dcr_write);
+    ppc4xx_dcr_register(dcr, SDRAM0_CFGDATA,
+                        s, &sdram_ddr_dcr_read, &sdram_ddr_dcr_write);
+}
+
+static Property ppc4xx_sdram_ddr_props[] = {
+    DEFINE_PROP_LINK("dram", Ppc4xxSdramDdrState, dram_mr, TYPE_MEMORY_REGION,
+                     MemoryRegion *),
+    DEFINE_PROP_UINT32("nbanks", Ppc4xxSdramDdrState, nbanks, 4),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ppc4xx_sdram_ddr_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = ppc4xx_sdram_ddr_realize;
+    dc->reset = ppc4xx_sdram_ddr_reset;
+    /* Reason: only works as function of a ppc4xx SoC */
+    dc->user_creatable = false;
+    device_class_set_props(dc, ppc4xx_sdram_ddr_props);
+}
+
+void ppc4xx_sdram_ddr_enable(Ppc4xxSdramDdrState *s)
+{
+    sdram_ddr_dcr_write(s, SDRAM0_CFGADDR, 0x20);
+    sdram_ddr_dcr_write(s, SDRAM0_CFGDATA, 0x80000000);
+}
+
 /*****************************************************************************/
 /* DDR2 SDRAM controller */
 enum {
@@ -338,6 +698,11 @@ void ppc4xx_sdram_ddr2_enable(Ppc4xxSdramDdr2State *s)
 
 static const TypeInfo ppc4xx_sdram_types[] = {
     {
+        .name           = TYPE_PPC4xx_SDRAM_DDR,
+        .parent         = TYPE_PPC4xx_DCR_DEVICE,
+        .instance_size  = sizeof(Ppc4xxSdramDdrState),
+        .class_init     = ppc4xx_sdram_ddr_class_init,
+    }, {
         .name           = TYPE_PPC4xx_SDRAM_DDR2,
         .parent         = TYPE_PPC4xx_DCR_DEVICE,
         .instance_size  = sizeof(Ppc4xxSdramDdr2State),
-- 
2.30.4

Re: [PATCH v7 2/8] ppc4xx_devs.c: Move DDR SDRAM controller model to ppc4xx_sdram.c

[Daniel Henrique Barboza]

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

On 10/19/22 13:02, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ppc/ppc4xx_devs.c  | 352 ----------------------------------------
>   hw/ppc/ppc4xx_sdram.c | 365 ++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 365 insertions(+), 352 deletions(-)
> 
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index 12af90f244..f737dbb3d6 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -24,357 +24,10 @@
>   
>   #include "qemu/osdep.h"
>   #include "qemu/units.h"
> -#include "sysemu/reset.h"
>   #include "cpu.h"
> -#include "hw/irq.h"
> -#include "hw/ppc/ppc.h"
>   #include "hw/ppc/ppc4xx.h"
>   #include "hw/qdev-properties.h"
> -#include "qemu/log.h"
> -#include "exec/address-spaces.h"
> -#include "qemu/error-report.h"
>   #include "qapi/error.h"
> -#include "trace.h"
> -
> -/*****************************************************************************/
> -/* SDRAM controller */
> -enum {
> -    SDRAM0_CFGADDR = 0x010,
> -    SDRAM0_CFGDATA = 0x011,
> -};
> -
> -/*
> - * 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_ddr_bcr(hwaddr ram_base, hwaddr ram_size)
> -{
> -    uint32_t bcr;
> -
> -    switch (ram_size) {
> -    case 4 * MiB:
> -        bcr = 0;
> -        break;
> -    case 8 * MiB:
> -        bcr = 0x20000;
> -        break;
> -    case 16 * MiB:
> -        bcr = 0x40000;
> -        break;
> -    case 32 * MiB:
> -        bcr = 0x60000;
> -        break;
> -    case 64 * MiB:
> -        bcr = 0x80000;
> -        break;
> -    case 128 * MiB:
> -        bcr = 0xA0000;
> -        break;
> -    case 256 * MiB:
> -        bcr = 0xC0000;
> -        break;
> -    default:
> -        qemu_log_mask(LOG_GUEST_ERROR,
> -                      "%s: invalid RAM size 0x%" HWADDR_PRIx "\n", __func__,
> -                      ram_size);
> -        return 0;
> -    }
> -    bcr |= ram_base & 0xFF800000;
> -    bcr |= 1;
> -
> -    return bcr;
> -}
> -
> -static inline hwaddr sdram_ddr_base(uint32_t bcr)
> -{
> -    return bcr & 0xFF800000;
> -}
> -
> -static target_ulong sdram_ddr_size(uint32_t bcr)
> -{
> -    target_ulong size;
> -    int sh;
> -
> -    sh = (bcr >> 17) & 0x7;
> -    if (sh == 7) {
> -        size = -1;
> -    } else {
> -        size = (4 * MiB) << sh;
> -    }
> -
> -    return size;
> -}
> -
> -static void sdram_ddr_set_bcr(Ppc4xxSdramDdrState *sdram, int i,
> -                              uint32_t bcr, int enabled)
> -{
> -    if (sdram->bank[i].bcr & 1) {
> -        /* Unmap RAM */
> -        trace_ppc4xx_sdram_unmap(sdram_ddr_base(sdram->bank[i].bcr),
> -                                 sdram_ddr_size(sdram->bank[i].bcr));
> -        memory_region_del_subregion(get_system_memory(),
> -                                    &sdram->bank[i].container);
> -        memory_region_del_subregion(&sdram->bank[i].container,
> -                                    &sdram->bank[i].ram);
> -        object_unparent(OBJECT(&sdram->bank[i].container));
> -    }
> -    sdram->bank[i].bcr = bcr & 0xFFDEE001;
> -    if (enabled && (bcr & 1)) {
> -        trace_ppc4xx_sdram_map(sdram_ddr_base(bcr), sdram_ddr_size(bcr));
> -        memory_region_init(&sdram->bank[i].container, NULL, "sdram-container",
> -                           sdram_ddr_size(bcr));
> -        memory_region_add_subregion(&sdram->bank[i].container, 0,
> -                                    &sdram->bank[i].ram);
> -        memory_region_add_subregion(get_system_memory(),
> -                                    sdram_ddr_base(bcr),
> -                                    &sdram->bank[i].container);
> -    }
> -}
> -
> -static void sdram_ddr_map_bcr(Ppc4xxSdramDdrState *sdram)
> -{
> -    int i;
> -
> -    for (i = 0; i < sdram->nbanks; i++) {
> -        if (sdram->bank[i].size != 0) {
> -            sdram_ddr_set_bcr(sdram, i, sdram_ddr_bcr(sdram->bank[i].base,
> -                                                      sdram->bank[i].size), 1);
> -        } else {
> -            sdram_ddr_set_bcr(sdram, i, 0, 0);
> -        }
> -    }
> -}
> -
> -static void sdram_ddr_unmap_bcr(Ppc4xxSdramDdrState *sdram)
> -{
> -    int i;
> -
> -    for (i = 0; i < sdram->nbanks; i++) {
> -        trace_ppc4xx_sdram_unmap(sdram_ddr_base(sdram->bank[i].bcr),
> -                                 sdram_ddr_size(sdram->bank[i].bcr));
> -        memory_region_del_subregion(get_system_memory(),
> -                                    &sdram->bank[i].ram);
> -    }
> -}
> -
> -static uint32_t sdram_ddr_dcr_read(void *opaque, int dcrn)
> -{
> -    Ppc4xxSdramDdrState *sdram = opaque;
> -    uint32_t ret;
> -
> -    switch (dcrn) {
> -    case SDRAM0_CFGADDR:
> -        ret = sdram->addr;
> -        break;
> -    case SDRAM0_CFGDATA:
> -        switch (sdram->addr) {
> -        case 0x00: /* SDRAM_BESR0 */
> -            ret = sdram->besr0;
> -            break;
> -        case 0x08: /* SDRAM_BESR1 */
> -            ret = sdram->besr1;
> -            break;
> -        case 0x10: /* SDRAM_BEAR */
> -            ret = sdram->bear;
> -            break;
> -        case 0x20: /* SDRAM_CFG */
> -            ret = sdram->cfg;
> -            break;
> -        case 0x24: /* SDRAM_STATUS */
> -            ret = sdram->status;
> -            break;
> -        case 0x30: /* SDRAM_RTR */
> -            ret = sdram->rtr;
> -            break;
> -        case 0x34: /* SDRAM_PMIT */
> -            ret = sdram->pmit;
> -            break;
> -        case 0x40: /* SDRAM_B0CR */
> -            ret = sdram->bank[0].bcr;
> -            break;
> -        case 0x44: /* SDRAM_B1CR */
> -            ret = sdram->bank[1].bcr;
> -            break;
> -        case 0x48: /* SDRAM_B2CR */
> -            ret = sdram->bank[2].bcr;
> -            break;
> -        case 0x4C: /* SDRAM_B3CR */
> -            ret = sdram->bank[3].bcr;
> -            break;
> -        case 0x80: /* SDRAM_TR */
> -            ret = -1; /* ? */
> -            break;
> -        case 0x94: /* SDRAM_ECCCFG */
> -            ret = sdram->ecccfg;
> -            break;
> -        case 0x98: /* SDRAM_ECCESR */
> -            ret = sdram->eccesr;
> -            break;
> -        default: /* Error */
> -            ret = -1;
> -            break;
> -        }
> -        break;
> -    default:
> -        /* Avoid gcc warning */
> -        ret = 0;
> -        break;
> -    }
> -
> -    return ret;
> -}
> -
> -static void sdram_ddr_dcr_write(void *opaque, int dcrn, uint32_t val)
> -{
> -    Ppc4xxSdramDdrState *sdram = opaque;
> -
> -    switch (dcrn) {
> -    case SDRAM0_CFGADDR:
> -        sdram->addr = val;
> -        break;
> -    case SDRAM0_CFGDATA:
> -        switch (sdram->addr) {
> -        case 0x00: /* SDRAM_BESR0 */
> -            sdram->besr0 &= ~val;
> -            break;
> -        case 0x08: /* SDRAM_BESR1 */
> -            sdram->besr1 &= ~val;
> -            break;
> -        case 0x10: /* SDRAM_BEAR */
> -            sdram->bear = val;
> -            break;
> -        case 0x20: /* SDRAM_CFG */
> -            val &= 0xFFE00000;
> -            if (!(sdram->cfg & 0x80000000) && (val & 0x80000000)) {
> -                trace_ppc4xx_sdram_enable("enable");
> -                /* validate all RAM mappings */
> -                sdram_ddr_map_bcr(sdram);
> -                sdram->status &= ~0x80000000;
> -            } else if ((sdram->cfg & 0x80000000) && !(val & 0x80000000)) {
> -                trace_ppc4xx_sdram_enable("disable");
> -                /* invalidate all RAM mappings */
> -                sdram_ddr_unmap_bcr(sdram);
> -                sdram->status |= 0x80000000;
> -            }
> -            if (!(sdram->cfg & 0x40000000) && (val & 0x40000000)) {
> -                sdram->status |= 0x40000000;
> -            } else if ((sdram->cfg & 0x40000000) && !(val & 0x40000000)) {
> -                sdram->status &= ~0x40000000;
> -            }
> -            sdram->cfg = val;
> -            break;
> -        case 0x24: /* SDRAM_STATUS */
> -            /* Read-only register */
> -            break;
> -        case 0x30: /* SDRAM_RTR */
> -            sdram->rtr = val & 0x3FF80000;
> -            break;
> -        case 0x34: /* SDRAM_PMIT */
> -            sdram->pmit = (val & 0xF8000000) | 0x07C00000;
> -            break;
> -        case 0x40: /* SDRAM_B0CR */
> -            sdram_ddr_set_bcr(sdram, 0, val, sdram->cfg & 0x80000000);
> -            break;
> -        case 0x44: /* SDRAM_B1CR */
> -            sdram_ddr_set_bcr(sdram, 1, val, sdram->cfg & 0x80000000);
> -            break;
> -        case 0x48: /* SDRAM_B2CR */
> -            sdram_ddr_set_bcr(sdram, 2, val, sdram->cfg & 0x80000000);
> -            break;
> -        case 0x4C: /* SDRAM_B3CR */
> -            sdram_ddr_set_bcr(sdram, 3, val, sdram->cfg & 0x80000000);
> -            break;
> -        case 0x80: /* SDRAM_TR */
> -            sdram->tr = val & 0x018FC01F;
> -            break;
> -        case 0x94: /* SDRAM_ECCCFG */
> -            sdram->ecccfg = val & 0x00F00000;
> -            break;
> -        case 0x98: /* SDRAM_ECCESR */
> -            val &= 0xFFF0F000;
> -            if (sdram->eccesr == 0 && val != 0) {
> -                qemu_irq_raise(sdram->irq);
> -            } else if (sdram->eccesr != 0 && val == 0) {
> -                qemu_irq_lower(sdram->irq);
> -            }
> -            sdram->eccesr = val;
> -            break;
> -        default: /* Error */
> -            break;
> -        }
> -        break;
> -    }
> -}
> -
> -static void ppc4xx_sdram_ddr_reset(DeviceState *dev)
> -{
> -    Ppc4xxSdramDdrState *sdram = PPC4xx_SDRAM_DDR(dev);
> -
> -    sdram->addr = 0;
> -    sdram->bear = 0;
> -    sdram->besr0 = 0; /* No error */
> -    sdram->besr1 = 0; /* No error */
> -    sdram->cfg = 0;
> -    sdram->ecccfg = 0; /* No ECC */
> -    sdram->eccesr = 0; /* No error */
> -    sdram->pmit = 0x07C00000;
> -    sdram->rtr = 0x05F00000;
> -    sdram->tr = 0x00854009;
> -    /* We pre-initialize RAM banks */
> -    sdram->status = 0;
> -    sdram->cfg = 0x00800000;
> -}
> -
> -static void ppc4xx_sdram_ddr_realize(DeviceState *dev, Error **errp)
> -{
> -    Ppc4xxSdramDdrState *s = PPC4xx_SDRAM_DDR(dev);
> -    Ppc4xxDcrDeviceState *dcr = PPC4xx_DCR_DEVICE(dev);
> -    const ram_addr_t valid_bank_sizes[] = {
> -        256 * MiB, 128 * MiB, 64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 4 * MiB, 0
> -    };
> -
> -    if (s->nbanks < 1 || s->nbanks > 4) {
> -        error_setg(errp, "Invalid number of RAM banks");
> -        return;
> -    }
> -    if (!s->dram_mr) {
> -        error_setg(errp, "Missing dram memory region");
> -        return;
> -    }
> -    ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank, valid_bank_sizes);
> -
> -    sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
> -
> -    ppc4xx_dcr_register(dcr, SDRAM0_CFGADDR,
> -                        s, &sdram_ddr_dcr_read, &sdram_ddr_dcr_write);
> -    ppc4xx_dcr_register(dcr, SDRAM0_CFGDATA,
> -                        s, &sdram_ddr_dcr_read, &sdram_ddr_dcr_write);
> -}
> -
> -static Property ppc4xx_sdram_ddr_props[] = {
> -    DEFINE_PROP_LINK("dram", Ppc4xxSdramDdrState, dram_mr, TYPE_MEMORY_REGION,
> -                     MemoryRegion *),
> -    DEFINE_PROP_UINT32("nbanks", Ppc4xxSdramDdrState, nbanks, 4),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
> -static void ppc4xx_sdram_ddr_class_init(ObjectClass *oc, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(oc);
> -
> -    dc->realize = ppc4xx_sdram_ddr_realize;
> -    dc->reset = ppc4xx_sdram_ddr_reset;
> -    /* Reason: only works as function of a ppc4xx SoC */
> -    dc->user_creatable = false;
> -    device_class_set_props(dc, ppc4xx_sdram_ddr_props);
> -}
> -
> -void ppc4xx_sdram_ddr_enable(Ppc4xxSdramDdrState *s)
> -{
> -    sdram_ddr_dcr_write(s, SDRAM0_CFGADDR, 0x20);
> -    sdram_ddr_dcr_write(s, SDRAM0_CFGDATA, 0x80000000);
> -}
>   
>   /*
>    * Split RAM between SDRAM banks.
> @@ -963,11 +616,6 @@ static void ppc4xx_dcr_class_init(ObjectClass *oc, void *data)
>   
>   static const TypeInfo ppc4xx_types[] = {
>       {
> -        .name           = TYPE_PPC4xx_SDRAM_DDR,
> -        .parent         = TYPE_PPC4xx_DCR_DEVICE,
> -        .instance_size  = sizeof(Ppc4xxSdramDdrState),
> -        .class_init     = ppc4xx_sdram_ddr_class_init,
> -    }, {
>           .name           = TYPE_PPC4xx_MAL,
>           .parent         = TYPE_PPC4xx_DCR_DEVICE,
>           .instance_size  = sizeof(Ppc4xxMalState),
> diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
> index b49a7ed60a..d88363bc3d 100644
> --- a/hw/ppc/ppc4xx_sdram.c
> +++ b/hw/ppc/ppc4xx_sdram.c
> @@ -1,4 +1,27 @@
>   /*
> + * QEMU PowerPC 4xx embedded processors SDRAM controller emulation
> + *
> + * DDR SDRAM controller:
> + * Copyright (c) 2007 Jocelyn Mayer
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + *
>    * DDR2 SDRAM controller:
>    * Copyright (c) 2012 François Revol
>    * Copyright (c) 2016-2019 BALATON Zoltan
> @@ -9,7 +32,9 @@
>   #include "qemu/osdep.h"
>   #include "qemu/units.h"
>   #include "qapi/error.h"
> +#include "qemu/log.h"
>   #include "exec/address-spaces.h" /* get_system_memory() */
> +#include "exec/cpu-defs.h" /* target_ulong */
>   #include "hw/irq.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/ppc/ppc4xx.h"
> @@ -38,6 +63,341 @@ enum {
>       SDRAM0_CFGDATA = 0x011,
>   };
>   
> +/*****************************************************************************/
> +/* DDR SDRAM controller */
> +/*
> + * 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_ddr_bcr(hwaddr ram_base, hwaddr ram_size)
> +{
> +    uint32_t bcr;
> +
> +    switch (ram_size) {
> +    case 4 * MiB:
> +        bcr = 0;
> +        break;
> +    case 8 * MiB:
> +        bcr = 0x20000;
> +        break;
> +    case 16 * MiB:
> +        bcr = 0x40000;
> +        break;
> +    case 32 * MiB:
> +        bcr = 0x60000;
> +        break;
> +    case 64 * MiB:
> +        bcr = 0x80000;
> +        break;
> +    case 128 * MiB:
> +        bcr = 0xA0000;
> +        break;
> +    case 256 * MiB:
> +        bcr = 0xC0000;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: invalid RAM size 0x%" HWADDR_PRIx "\n", __func__,
> +                      ram_size);
> +        return 0;
> +    }
> +    bcr |= ram_base & 0xFF800000;
> +    bcr |= 1;
> +
> +    return bcr;
> +}
> +
> +static inline hwaddr sdram_ddr_base(uint32_t bcr)
> +{
> +    return bcr & 0xFF800000;
> +}
> +
> +static target_ulong sdram_ddr_size(uint32_t bcr)
> +{
> +    target_ulong size;
> +    int sh;
> +
> +    sh = (bcr >> 17) & 0x7;
> +    if (sh == 7) {
> +        size = -1;
> +    } else {
> +        size = (4 * MiB) << sh;
> +    }
> +
> +    return size;
> +}
> +
> +static void sdram_ddr_set_bcr(Ppc4xxSdramDdrState *sdram, int i,
> +                              uint32_t bcr, int enabled)
> +{
> +    if (sdram->bank[i].bcr & 1) {
> +        /* Unmap RAM */
> +        trace_ppc4xx_sdram_unmap(sdram_ddr_base(sdram->bank[i].bcr),
> +                                 sdram_ddr_size(sdram->bank[i].bcr));
> +        memory_region_del_subregion(get_system_memory(),
> +                                    &sdram->bank[i].container);
> +        memory_region_del_subregion(&sdram->bank[i].container,
> +                                    &sdram->bank[i].ram);
> +        object_unparent(OBJECT(&sdram->bank[i].container));
> +    }
> +    sdram->bank[i].bcr = bcr & 0xFFDEE001;
> +    if (enabled && (bcr & 1)) {
> +        trace_ppc4xx_sdram_map(sdram_ddr_base(bcr), sdram_ddr_size(bcr));
> +        memory_region_init(&sdram->bank[i].container, NULL, "sdram-container",
> +                           sdram_ddr_size(bcr));
> +        memory_region_add_subregion(&sdram->bank[i].container, 0,
> +                                    &sdram->bank[i].ram);
> +        memory_region_add_subregion(get_system_memory(),
> +                                    sdram_ddr_base(bcr),
> +                                    &sdram->bank[i].container);
> +    }
> +}
> +
> +static void sdram_ddr_map_bcr(Ppc4xxSdramDdrState *sdram)
> +{
> +    int i;
> +
> +    for (i = 0; i < sdram->nbanks; i++) {
> +        if (sdram->bank[i].size != 0) {
> +            sdram_ddr_set_bcr(sdram, i, sdram_ddr_bcr(sdram->bank[i].base,
> +                                                      sdram->bank[i].size), 1);
> +        } else {
> +            sdram_ddr_set_bcr(sdram, i, 0, 0);
> +        }
> +    }
> +}
> +
> +static void sdram_ddr_unmap_bcr(Ppc4xxSdramDdrState *sdram)
> +{
> +    int i;
> +
> +    for (i = 0; i < sdram->nbanks; i++) {
> +        trace_ppc4xx_sdram_unmap(sdram_ddr_base(sdram->bank[i].bcr),
> +                                 sdram_ddr_size(sdram->bank[i].bcr));
> +        memory_region_del_subregion(get_system_memory(),
> +                                    &sdram->bank[i].ram);
> +    }
> +}
> +
> +static uint32_t sdram_ddr_dcr_read(void *opaque, int dcrn)
> +{
> +    Ppc4xxSdramDdrState *sdram = opaque;
> +    uint32_t ret;
> +
> +    switch (dcrn) {
> +    case SDRAM0_CFGADDR:
> +        ret = sdram->addr;
> +        break;
> +    case SDRAM0_CFGDATA:
> +        switch (sdram->addr) {
> +        case 0x00: /* SDRAM_BESR0 */
> +            ret = sdram->besr0;
> +            break;
> +        case 0x08: /* SDRAM_BESR1 */
> +            ret = sdram->besr1;
> +            break;
> +        case 0x10: /* SDRAM_BEAR */
> +            ret = sdram->bear;
> +            break;
> +        case 0x20: /* SDRAM_CFG */
> +            ret = sdram->cfg;
> +            break;
> +        case 0x24: /* SDRAM_STATUS */
> +            ret = sdram->status;
> +            break;
> +        case 0x30: /* SDRAM_RTR */
> +            ret = sdram->rtr;
> +            break;
> +        case 0x34: /* SDRAM_PMIT */
> +            ret = sdram->pmit;
> +            break;
> +        case 0x40: /* SDRAM_B0CR */
> +            ret = sdram->bank[0].bcr;
> +            break;
> +        case 0x44: /* SDRAM_B1CR */
> +            ret = sdram->bank[1].bcr;
> +            break;
> +        case 0x48: /* SDRAM_B2CR */
> +            ret = sdram->bank[2].bcr;
> +            break;
> +        case 0x4C: /* SDRAM_B3CR */
> +            ret = sdram->bank[3].bcr;
> +            break;
> +        case 0x80: /* SDRAM_TR */
> +            ret = -1; /* ? */
> +            break;
> +        case 0x94: /* SDRAM_ECCCFG */
> +            ret = sdram->ecccfg;
> +            break;
> +        case 0x98: /* SDRAM_ECCESR */
> +            ret = sdram->eccesr;
> +            break;
> +        default: /* Error */
> +            ret = -1;
> +            break;
> +        }
> +        break;
> +    default:
> +        /* Avoid gcc warning */
> +        ret = 0;
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +static void sdram_ddr_dcr_write(void *opaque, int dcrn, uint32_t val)
> +{
> +    Ppc4xxSdramDdrState *sdram = opaque;
> +
> +    switch (dcrn) {
> +    case SDRAM0_CFGADDR:
> +        sdram->addr = val;
> +        break;
> +    case SDRAM0_CFGDATA:
> +        switch (sdram->addr) {
> +        case 0x00: /* SDRAM_BESR0 */
> +            sdram->besr0 &= ~val;
> +            break;
> +        case 0x08: /* SDRAM_BESR1 */
> +            sdram->besr1 &= ~val;
> +            break;
> +        case 0x10: /* SDRAM_BEAR */
> +            sdram->bear = val;
> +            break;
> +        case 0x20: /* SDRAM_CFG */
> +            val &= 0xFFE00000;
> +            if (!(sdram->cfg & 0x80000000) && (val & 0x80000000)) {
> +                trace_ppc4xx_sdram_enable("enable");
> +                /* validate all RAM mappings */
> +                sdram_ddr_map_bcr(sdram);
> +                sdram->status &= ~0x80000000;
> +            } else if ((sdram->cfg & 0x80000000) && !(val & 0x80000000)) {
> +                trace_ppc4xx_sdram_enable("disable");
> +                /* invalidate all RAM mappings */
> +                sdram_ddr_unmap_bcr(sdram);
> +                sdram->status |= 0x80000000;
> +            }
> +            if (!(sdram->cfg & 0x40000000) && (val & 0x40000000)) {
> +                sdram->status |= 0x40000000;
> +            } else if ((sdram->cfg & 0x40000000) && !(val & 0x40000000)) {
> +                sdram->status &= ~0x40000000;
> +            }
> +            sdram->cfg = val;
> +            break;
> +        case 0x24: /* SDRAM_STATUS */
> +            /* Read-only register */
> +            break;
> +        case 0x30: /* SDRAM_RTR */
> +            sdram->rtr = val & 0x3FF80000;
> +            break;
> +        case 0x34: /* SDRAM_PMIT */
> +            sdram->pmit = (val & 0xF8000000) | 0x07C00000;
> +            break;
> +        case 0x40: /* SDRAM_B0CR */
> +            sdram_ddr_set_bcr(sdram, 0, val, sdram->cfg & 0x80000000);
> +            break;
> +        case 0x44: /* SDRAM_B1CR */
> +            sdram_ddr_set_bcr(sdram, 1, val, sdram->cfg & 0x80000000);
> +            break;
> +        case 0x48: /* SDRAM_B2CR */
> +            sdram_ddr_set_bcr(sdram, 2, val, sdram->cfg & 0x80000000);
> +            break;
> +        case 0x4C: /* SDRAM_B3CR */
> +            sdram_ddr_set_bcr(sdram, 3, val, sdram->cfg & 0x80000000);
> +            break;
> +        case 0x80: /* SDRAM_TR */
> +            sdram->tr = val & 0x018FC01F;
> +            break;
> +        case 0x94: /* SDRAM_ECCCFG */
> +            sdram->ecccfg = val & 0x00F00000;
> +            break;
> +        case 0x98: /* SDRAM_ECCESR */
> +            val &= 0xFFF0F000;
> +            if (sdram->eccesr == 0 && val != 0) {
> +                qemu_irq_raise(sdram->irq);
> +            } else if (sdram->eccesr != 0 && val == 0) {
> +                qemu_irq_lower(sdram->irq);
> +            }
> +            sdram->eccesr = val;
> +            break;
> +        default: /* Error */
> +            break;
> +        }
> +        break;
> +    }
> +}
> +
> +static void ppc4xx_sdram_ddr_reset(DeviceState *dev)
> +{
> +    Ppc4xxSdramDdrState *sdram = PPC4xx_SDRAM_DDR(dev);
> +
> +    sdram->addr = 0;
> +    sdram->bear = 0;
> +    sdram->besr0 = 0; /* No error */
> +    sdram->besr1 = 0; /* No error */
> +    sdram->cfg = 0;
> +    sdram->ecccfg = 0; /* No ECC */
> +    sdram->eccesr = 0; /* No error */
> +    sdram->pmit = 0x07C00000;
> +    sdram->rtr = 0x05F00000;
> +    sdram->tr = 0x00854009;
> +    /* We pre-initialize RAM banks */
> +    sdram->status = 0;
> +    sdram->cfg = 0x00800000;
> +}
> +
> +static void ppc4xx_sdram_ddr_realize(DeviceState *dev, Error **errp)
> +{
> +    Ppc4xxSdramDdrState *s = PPC4xx_SDRAM_DDR(dev);
> +    Ppc4xxDcrDeviceState *dcr = PPC4xx_DCR_DEVICE(dev);
> +    const ram_addr_t valid_bank_sizes[] = {
> +        256 * MiB, 128 * MiB, 64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 4 * MiB, 0
> +    };
> +
> +    if (s->nbanks < 1 || s->nbanks > 4) {
> +        error_setg(errp, "Invalid number of RAM banks");
> +        return;
> +    }
> +    if (!s->dram_mr) {
> +        error_setg(errp, "Missing dram memory region");
> +        return;
> +    }
> +    ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank, valid_bank_sizes);
> +
> +    sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
> +
> +    ppc4xx_dcr_register(dcr, SDRAM0_CFGADDR,
> +                        s, &sdram_ddr_dcr_read, &sdram_ddr_dcr_write);
> +    ppc4xx_dcr_register(dcr, SDRAM0_CFGDATA,
> +                        s, &sdram_ddr_dcr_read, &sdram_ddr_dcr_write);
> +}
> +
> +static Property ppc4xx_sdram_ddr_props[] = {
> +    DEFINE_PROP_LINK("dram", Ppc4xxSdramDdrState, dram_mr, TYPE_MEMORY_REGION,
> +                     MemoryRegion *),
> +    DEFINE_PROP_UINT32("nbanks", Ppc4xxSdramDdrState, nbanks, 4),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void ppc4xx_sdram_ddr_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = ppc4xx_sdram_ddr_realize;
> +    dc->reset = ppc4xx_sdram_ddr_reset;
> +    /* Reason: only works as function of a ppc4xx SoC */
> +    dc->user_creatable = false;
> +    device_class_set_props(dc, ppc4xx_sdram_ddr_props);
> +}
> +
> +void ppc4xx_sdram_ddr_enable(Ppc4xxSdramDdrState *s)
> +{
> +    sdram_ddr_dcr_write(s, SDRAM0_CFGADDR, 0x20);
> +    sdram_ddr_dcr_write(s, SDRAM0_CFGDATA, 0x80000000);
> +}
> +
>   /*****************************************************************************/
>   /* DDR2 SDRAM controller */
>   enum {
> @@ -338,6 +698,11 @@ void ppc4xx_sdram_ddr2_enable(Ppc4xxSdramDdr2State *s)
>   
>   static const TypeInfo ppc4xx_sdram_types[] = {
>       {
> +        .name           = TYPE_PPC4xx_SDRAM_DDR,
> +        .parent         = TYPE_PPC4xx_DCR_DEVICE,
> +        .instance_size  = sizeof(Ppc4xxSdramDdrState),
> +        .class_init     = ppc4xx_sdram_ddr_class_init,
> +    }, {
>           .name           = TYPE_PPC4xx_SDRAM_DDR2,
>           .parent         = TYPE_PPC4xx_DCR_DEVICE,
>           .instance_size  = sizeof(Ppc4xxSdramDdr2State),

[PATCH v7 3/8] ppc4xx_sdram: Move ppc4xx_sdram_banks() to ppc4xx_sdram.c

[BALATON Zoltan]

This function is only used by the ppc4xx memory controller models so
it can be made static.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/ppc4xx_devs.c    | 62 -----------------------------------------
 hw/ppc/ppc4xx_sdram.c   | 61 ++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/ppc4xx.h | 20 ++++++-------
 3 files changed, 69 insertions(+), 74 deletions(-)

diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index f737dbb3d6..c1d111465d 100644
--- a/hw/ppc/ppc4xx_devs.c
+++ b/hw/ppc/ppc4xx_devs.c
@@ -23,73 +23,11 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu/units.h"
 #include "cpu.h"
 #include "hw/ppc/ppc4xx.h"
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
 
-/*
- * Split RAM between SDRAM banks.
- *
- * sdram_bank_sizes[] must be in descending order, that is sizes[i] > sizes[i+1]
- * and must be 0-terminated.
- *
- * The 4xx SDRAM controller supports a small number of banks, and each bank
- * must be one of a small set of sizes. The number of banks and the supported
- * sizes varies by SoC.
- */
-void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
-                        Ppc4xxSdramBank ram_banks[],
-                        const ram_addr_t sdram_bank_sizes[])
-{
-    ram_addr_t size_left = memory_region_size(ram);
-    ram_addr_t base = 0;
-    ram_addr_t bank_size;
-    int i;
-    int j;
-
-    for (i = 0; i < nr_banks; i++) {
-        for (j = 0; sdram_bank_sizes[j] != 0; j++) {
-            bank_size = sdram_bank_sizes[j];
-            if (bank_size <= size_left) {
-                char name[32];
-
-                ram_banks[i].base = base;
-                ram_banks[i].size = bank_size;
-                base += bank_size;
-                size_left -= bank_size;
-                snprintf(name, sizeof(name), "ppc4xx.sdram%d", i);
-                memory_region_init_alias(&ram_banks[i].ram, NULL, name, ram,
-                                         ram_banks[i].base, ram_banks[i].size);
-                break;
-            }
-        }
-        if (!size_left) {
-            /* No need to use the remaining banks. */
-            break;
-        }
-    }
-
-    if (size_left) {
-        ram_addr_t used_size = memory_region_size(ram) - size_left;
-        GString *s = g_string_new(NULL);
-
-        for (i = 0; sdram_bank_sizes[i]; i++) {
-            g_string_append_printf(s, "%" PRIi64 "%s",
-                                   sdram_bank_sizes[i] / MiB,
-                                   sdram_bank_sizes[i + 1] ? ", " : "");
-        }
-        error_report("at most %d bank%s of %s MiB each supported",
-                     nr_banks, nr_banks == 1 ? "" : "s", s->str);
-        error_printf("Possible valid RAM size: %" PRIi64 " MiB\n",
-            used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB);
-
-        g_string_free(s, true);
-        exit(EXIT_FAILURE);
-    }
-}
-
 /*****************************************************************************/
 /* MAL */
 
diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
index d88363bc3d..62ef7d8f0d 100644
--- a/hw/ppc/ppc4xx_sdram.c
+++ b/hw/ppc/ppc4xx_sdram.c
@@ -43,6 +43,67 @@
 /*****************************************************************************/
 /* Shared functions */
 
+/*
+ * Split RAM between SDRAM banks.
+ *
+ * sdram_bank_sizes[] must be in descending order, that is sizes[i] > sizes[i+1]
+ * and must be 0-terminated.
+ *
+ * The 4xx SDRAM controller supports a small number of banks, and each bank
+ * must be one of a small set of sizes. The number of banks and the supported
+ * sizes varies by SoC.
+ */
+static void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
+                               Ppc4xxSdramBank ram_banks[],
+                               const ram_addr_t sdram_bank_sizes[])
+{
+    ram_addr_t size_left = memory_region_size(ram);
+    ram_addr_t base = 0;
+    ram_addr_t bank_size;
+    int i;
+    int j;
+
+    for (i = 0; i < nr_banks; i++) {
+        for (j = 0; sdram_bank_sizes[j] != 0; j++) {
+            bank_size = sdram_bank_sizes[j];
+            if (bank_size <= size_left) {
+                char name[32];
+
+                ram_banks[i].base = base;
+                ram_banks[i].size = bank_size;
+                base += bank_size;
+                size_left -= bank_size;
+                snprintf(name, sizeof(name), "ppc4xx.sdram%d", i);
+                memory_region_init_alias(&ram_banks[i].ram, NULL, name, ram,
+                                         ram_banks[i].base, ram_banks[i].size);
+                break;
+            }
+        }
+        if (!size_left) {
+            /* No need to use the remaining banks. */
+            break;
+        }
+    }
+
+    if (size_left) {
+        ram_addr_t used_size = memory_region_size(ram) - size_left;
+        GString *s = g_string_new(NULL);
+
+        for (i = 0; sdram_bank_sizes[i]; i++) {
+            g_string_append_printf(s, "%" PRIi64 "%s",
+                                   sdram_bank_sizes[i] / MiB,
+                                   sdram_bank_sizes[i + 1] ? ", " : "");
+        }
+        error_report("at most %d bank%s of %s MiB each supported",
+                     nr_banks, nr_banks == 1 ? "" : "s", s->str);
+        error_printf("Possible valid RAM size: %" PRIi64 " MiB\n",
+            used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB);
+
+        g_string_free(s, true);
+        exit(EXIT_FAILURE);
+    }
+}
+
 static void sdram_bank_map(Ppc4xxSdramBank *bank)
 {
     memory_region_init(&bank->container, NULL, "sdram-container", bank->size);
diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
index 10c6dd535f..f8c86e09ec 100644
--- a/include/hw/ppc/ppc4xx.h
+++ b/include/hw/ppc/ppc4xx.h
@@ -29,18 +29,6 @@
 #include "exec/memory.h"
 #include "hw/sysbus.h"
 
-typedef struct {
-    MemoryRegion ram;
-    MemoryRegion container; /* used for clipping */
-    hwaddr base;
-    hwaddr size;
-    uint32_t bcr;
-} Ppc4xxSdramBank;
-
-void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
-                        Ppc4xxSdramBank ram_banks[],
-                        const ram_addr_t sdram_bank_sizes[]);
-
 #define TYPE_PPC4xx_PCI_HOST_BRIDGE "ppc4xx-pcihost"
 
 /*
@@ -111,6 +99,14 @@ struct Ppc4xxEbcState {
 };
 
 /* SDRAM DDR controller */
+typedef struct {
+    MemoryRegion ram;
+    MemoryRegion container; /* used for clipping */
+    hwaddr base;
+    hwaddr size;
+    uint32_t bcr;
+} Ppc4xxSdramBank;
+
 #define SDR0_DDR0_DDRM_ENCODE(n)  ((((unsigned long)(n)) & 0x03) << 29)
 #define SDR0_DDR0_DDRM_DDR1       0x20000000
 #define SDR0_DDR0_DDRM_DDR2       0x40000000
-- 
2.30.4

Re: [PATCH v7 3/8] ppc4xx_sdram: Move ppc4xx_sdram_banks() to ppc4xx_sdram.c

[Daniel Henrique Barboza]

On 10/19/22 13:02, BALATON Zoltan wrote:
> This function is only used by the ppc4xx memory controller models so
> it can be made static.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   hw/ppc/ppc4xx_devs.c    | 62 -----------------------------------------
>   hw/ppc/ppc4xx_sdram.c   | 61 ++++++++++++++++++++++++++++++++++++++++
>   include/hw/ppc/ppc4xx.h | 20 ++++++-------
>   3 files changed, 69 insertions(+), 74 deletions(-)
> 
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index f737dbb3d6..c1d111465d 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -23,73 +23,11 @@
>    */
>   
>   #include "qemu/osdep.h"
> -#include "qemu/units.h"
>   #include "cpu.h"
>   #include "hw/ppc/ppc4xx.h"
>   #include "hw/qdev-properties.h"
>   #include "qapi/error.h"
>   
> -/*
> - * Split RAM between SDRAM banks.
> - *
> - * sdram_bank_sizes[] must be in descending order, that is sizes[i] > sizes[i+1]
> - * and must be 0-terminated.
> - *
> - * The 4xx SDRAM controller supports a small number of banks, and each bank
> - * must be one of a small set of sizes. The number of banks and the supported
> - * sizes varies by SoC.
> - */
> -void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
> -                        Ppc4xxSdramBank ram_banks[],
> -                        const ram_addr_t sdram_bank_sizes[])
> -{
> -    ram_addr_t size_left = memory_region_size(ram);
> -    ram_addr_t base = 0;
> -    ram_addr_t bank_size;
> -    int i;
> -    int j;
> -
> -    for (i = 0; i < nr_banks; i++) {
> -        for (j = 0; sdram_bank_sizes[j] != 0; j++) {
> -            bank_size = sdram_bank_sizes[j];
> -            if (bank_size <= size_left) {
> -                char name[32];
> -
> -                ram_banks[i].base = base;
> -                ram_banks[i].size = bank_size;
> -                base += bank_size;
> -                size_left -= bank_size;
> -                snprintf(name, sizeof(name), "ppc4xx.sdram%d", i);
> -                memory_region_init_alias(&ram_banks[i].ram, NULL, name, ram,
> -                                         ram_banks[i].base, ram_banks[i].size);
> -                break;
> -            }
> -        }
> -        if (!size_left) {
> -            /* No need to use the remaining banks. */
> -            break;
> -        }
> -    }
> -
> -    if (size_left) {
> -        ram_addr_t used_size = memory_region_size(ram) - size_left;
> -        GString *s = g_string_new(NULL);
> -
> -        for (i = 0; sdram_bank_sizes[i]; i++) {
> -            g_string_append_printf(s, "%" PRIi64 "%s",
> -                                   sdram_bank_sizes[i] / MiB,
> -                                   sdram_bank_sizes[i + 1] ? ", " : "");
> -        }
> -        error_report("at most %d bank%s of %s MiB each supported",
> -                     nr_banks, nr_banks == 1 ? "" : "s", s->str);
> -        error_printf("Possible valid RAM size: %" PRIi64 " MiB\n",
> -            used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB);
> -
> -        g_string_free(s, true);
> -        exit(EXIT_FAILURE);
> -    }
> -}
> -
>   /*****************************************************************************/
>   /* MAL */
>   
> diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
> index d88363bc3d..62ef7d8f0d 100644
> --- a/hw/ppc/ppc4xx_sdram.c
> +++ b/hw/ppc/ppc4xx_sdram.c
> @@ -43,6 +43,67 @@
>   /*****************************************************************************/
>   /* Shared functions */
>   
> +/*
> + * Split RAM between SDRAM banks.
> + *
> + * sdram_bank_sizes[] must be in descending order, that is sizes[i] > sizes[i+1]
> + * and must be 0-terminated.
> + *
> + * The 4xx SDRAM controller supports a small number of banks, and each bank
> + * must be one of a small set of sizes. The number of banks and the supported
> + * sizes varies by SoC.
> + */
> +static void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
> +                               Ppc4xxSdramBank ram_banks[],
> +                               const ram_addr_t sdram_bank_sizes[])
> +{
> +    ram_addr_t size_left = memory_region_size(ram);
> +    ram_addr_t base = 0;
> +    ram_addr_t bank_size;
> +    int i;
> +    int j;
> +
> +    for (i = 0; i < nr_banks; i++) {
> +        for (j = 0; sdram_bank_sizes[j] != 0; j++) {
> +            bank_size = sdram_bank_sizes[j];
> +            if (bank_size <= size_left) {
> +                char name[32];
> +
> +                ram_banks[i].base = base;
> +                ram_banks[i].size = bank_size;
> +                base += bank_size;
> +                size_left -= bank_size;
> +                snprintf(name, sizeof(name), "ppc4xx.sdram%d", i);
> +                memory_region_init_alias(&ram_banks[i].ram, NULL, name, ram,
> +                                         ram_banks[i].base, ram_banks[i].size);
> +                break;
> +            }
> +        }
> +        if (!size_left) {
> +            /* No need to use the remaining banks. */
> +            break;
> +        }
> +    }
> +
> +    if (size_left) {
> +        ram_addr_t used_size = memory_region_size(ram) - size_left;
> +        GString *s = g_string_new(NULL);
> +
> +        for (i = 0; sdram_bank_sizes[i]; i++) {
> +            g_string_append_printf(s, "%" PRIi64 "%s",
> +                                   sdram_bank_sizes[i] / MiB,
> +                                   sdram_bank_sizes[i + 1] ? ", " : "");
> +        }
> +        error_report("at most %d bank%s of %s MiB each supported",
> +                     nr_banks, nr_banks == 1 ? "" : "s", s->str);
> +        error_printf("Possible valid RAM size: %" PRIi64 " MiB\n",
> +            used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB);
> +
> +        g_string_free(s, true);
> +        exit(EXIT_FAILURE);
> +    }
> +}
> +
>   static void sdram_bank_map(Ppc4xxSdramBank *bank)
>   {
>       memory_region_init(&bank->container, NULL, "sdram-container", bank->size);
> diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
> index 10c6dd535f..f8c86e09ec 100644
> --- a/include/hw/ppc/ppc4xx.h
> +++ b/include/hw/ppc/ppc4xx.h
> @@ -29,18 +29,6 @@
>   #include "exec/memory.h"
>   #include "hw/sysbus.h"
>   
> -typedef struct {
> -    MemoryRegion ram;
> -    MemoryRegion container; /* used for clipping */
> -    hwaddr base;
> -    hwaddr size;
> -    uint32_t bcr;
> -} Ppc4xxSdramBank;
> -
> -void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
> -                        Ppc4xxSdramBank ram_banks[],
> -                        const ram_addr_t sdram_bank_sizes[]);
> -
>   #define TYPE_PPC4xx_PCI_HOST_BRIDGE "ppc4xx-pcihost"
>   
>   /*
> @@ -111,6 +99,14 @@ struct Ppc4xxEbcState {
>   };
>   
>   /* SDRAM DDR controller */
> +typedef struct {
> +    MemoryRegion ram;
> +    MemoryRegion container; /* used for clipping */
> +    hwaddr base;
> +    hwaddr size;
> +    uint32_t bcr;
> +} Ppc4xxSdramBank;
> +
>   #define SDR0_DDR0_DDRM_ENCODE(n)  ((((unsigned long)(n)) & 0x03) << 29)
>   #define SDR0_DDR0_DDRM_DDR1       0x20000000
>   #define SDR0_DDR0_DDRM_DDR2       0x40000000

[PATCH v7 4/8] ppc4xx_sdram: Use hwaddr for memory bank size

[BALATON Zoltan]

This resolves the target_ulong dependency that's clearly wrong and was
also noted in a fixme comment.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ppc/ppc4xx_sdram.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
index 62ef7d8f0d..2294747594 100644
--- a/hw/ppc/ppc4xx_sdram.c
+++ b/hw/ppc/ppc4xx_sdram.c
@@ -34,7 +34,6 @@
 #include "qapi/error.h"
 #include "qemu/log.h"
 #include "exec/address-spaces.h" /* get_system_memory() */
-#include "exec/cpu-defs.h" /* target_ulong */
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "hw/ppc/ppc4xx.h"
@@ -126,11 +125,6 @@ enum {
 
 /*****************************************************************************/
 /* DDR SDRAM controller */
-/*
- * 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_ddr_bcr(hwaddr ram_base, hwaddr ram_size)
 {
     uint32_t bcr;
@@ -174,9 +168,9 @@ static inline hwaddr sdram_ddr_base(uint32_t bcr)
     return bcr & 0xFF800000;
 }
 
-static target_ulong sdram_ddr_size(uint32_t bcr)
+static hwaddr sdram_ddr_size(uint32_t bcr)
 {
-    target_ulong size;
+    hwaddr size;
     int sh;
 
     sh = (bcr >> 17) & 0x7;
@@ -523,9 +517,9 @@ static inline hwaddr sdram_ddr2_base(uint32_t bcr)
     return (bcr & 0xffe00000) << 2;
 }
 
-static uint64_t sdram_ddr2_size(uint32_t bcr)
+static hwaddr sdram_ddr2_size(uint32_t bcr)
 {
-    uint64_t size;
+    hwaddr size;
     int sh;
 
     sh = 1024 - ((bcr >> 6) & 0x3ff);
-- 
2.30.4

[PATCH v7 5/8] ppc4xx_sdram: Rename local state variable for brevity

[BALATON Zoltan]

Rename the sdram local state variable to s in dcr read/write functions
and reset methods for better readability and to match realize methods.
Other places not converted will be changed or removed in subsequent
patches.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ppc/ppc4xx_sdram.c | 158 +++++++++++++++++++++---------------------
 1 file changed, 79 insertions(+), 79 deletions(-)

diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
index 2294747594..4bc53c8f01 100644
--- a/hw/ppc/ppc4xx_sdram.c
+++ b/hw/ppc/ppc4xx_sdram.c
@@ -237,56 +237,56 @@ static void sdram_ddr_unmap_bcr(Ppc4xxSdramDdrState *sdram)
 
 static uint32_t sdram_ddr_dcr_read(void *opaque, int dcrn)
 {
-    Ppc4xxSdramDdrState *sdram = opaque;
+    Ppc4xxSdramDdrState *s = opaque;
     uint32_t ret;
 
     switch (dcrn) {
     case SDRAM0_CFGADDR:
-        ret = sdram->addr;
+        ret = s->addr;
         break;
     case SDRAM0_CFGDATA:
-        switch (sdram->addr) {
+        switch (s->addr) {
         case 0x00: /* SDRAM_BESR0 */
-            ret = sdram->besr0;
+            ret = s->besr0;
             break;
         case 0x08: /* SDRAM_BESR1 */
-            ret = sdram->besr1;
+            ret = s->besr1;
             break;
         case 0x10: /* SDRAM_BEAR */
-            ret = sdram->bear;
+            ret = s->bear;
             break;
         case 0x20: /* SDRAM_CFG */
-            ret = sdram->cfg;
+            ret = s->cfg;
             break;
         case 0x24: /* SDRAM_STATUS */
-            ret = sdram->status;
+            ret = s->status;
             break;
         case 0x30: /* SDRAM_RTR */
-            ret = sdram->rtr;
+            ret = s->rtr;
             break;
         case 0x34: /* SDRAM_PMIT */
-            ret = sdram->pmit;
+            ret = s->pmit;
             break;
         case 0x40: /* SDRAM_B0CR */
-            ret = sdram->bank[0].bcr;
+            ret = s->bank[0].bcr;
             break;
         case 0x44: /* SDRAM_B1CR */
-            ret = sdram->bank[1].bcr;
+            ret = s->bank[1].bcr;
             break;
         case 0x48: /* SDRAM_B2CR */
-            ret = sdram->bank[2].bcr;
+            ret = s->bank[2].bcr;
             break;
         case 0x4C: /* SDRAM_B3CR */
-            ret = sdram->bank[3].bcr;
+            ret = s->bank[3].bcr;
             break;
         case 0x80: /* SDRAM_TR */
             ret = -1; /* ? */
             break;
         case 0x94: /* SDRAM_ECCCFG */
-            ret = sdram->ecccfg;
+            ret = s->ecccfg;
             break;
         case 0x98: /* SDRAM_ECCESR */
-            ret = sdram->eccesr;
+            ret = s->eccesr;
             break;
         default: /* Error */
             ret = -1;
@@ -304,78 +304,78 @@ static uint32_t sdram_ddr_dcr_read(void *opaque, int dcrn)
 
 static void sdram_ddr_dcr_write(void *opaque, int dcrn, uint32_t val)
 {
-    Ppc4xxSdramDdrState *sdram = opaque;
+    Ppc4xxSdramDdrState *s = opaque;
 
     switch (dcrn) {
     case SDRAM0_CFGADDR:
-        sdram->addr = val;
+        s->addr = val;
         break;
     case SDRAM0_CFGDATA:
-        switch (sdram->addr) {
+        switch (s->addr) {
         case 0x00: /* SDRAM_BESR0 */
-            sdram->besr0 &= ~val;
+            s->besr0 &= ~val;
             break;
         case 0x08: /* SDRAM_BESR1 */
-            sdram->besr1 &= ~val;
+            s->besr1 &= ~val;
             break;
         case 0x10: /* SDRAM_BEAR */
-            sdram->bear = val;
+            s->bear = val;
             break;
         case 0x20: /* SDRAM_CFG */
             val &= 0xFFE00000;
-            if (!(sdram->cfg & 0x80000000) && (val & 0x80000000)) {
+            if (!(s->cfg & 0x80000000) && (val & 0x80000000)) {
                 trace_ppc4xx_sdram_enable("enable");
                 /* validate all RAM mappings */
-                sdram_ddr_map_bcr(sdram);
-                sdram->status &= ~0x80000000;
-            } else if ((sdram->cfg & 0x80000000) && !(val & 0x80000000)) {
+                sdram_ddr_map_bcr(s);
+                s->status &= ~0x80000000;
+            } else if ((s->cfg & 0x80000000) && !(val & 0x80000000)) {
                 trace_ppc4xx_sdram_enable("disable");
                 /* invalidate all RAM mappings */
-                sdram_ddr_unmap_bcr(sdram);
-                sdram->status |= 0x80000000;
+                sdram_ddr_unmap_bcr(s);
+                s->status |= 0x80000000;
             }
-            if (!(sdram->cfg & 0x40000000) && (val & 0x40000000)) {
-                sdram->status |= 0x40000000;
-            } else if ((sdram->cfg & 0x40000000) && !(val & 0x40000000)) {
-                sdram->status &= ~0x40000000;
+            if (!(s->cfg & 0x40000000) && (val & 0x40000000)) {
+                s->status |= 0x40000000;
+            } else if ((s->cfg & 0x40000000) && !(val & 0x40000000)) {
+                s->status &= ~0x40000000;
             }
-            sdram->cfg = val;
+            s->cfg = val;
             break;
         case 0x24: /* SDRAM_STATUS */
             /* Read-only register */
             break;
         case 0x30: /* SDRAM_RTR */
-            sdram->rtr = val & 0x3FF80000;
+            s->rtr = val & 0x3FF80000;
             break;
         case 0x34: /* SDRAM_PMIT */
-            sdram->pmit = (val & 0xF8000000) | 0x07C00000;
+            s->pmit = (val & 0xF8000000) | 0x07C00000;
             break;
         case 0x40: /* SDRAM_B0CR */
-            sdram_ddr_set_bcr(sdram, 0, val, sdram->cfg & 0x80000000);
+            sdram_ddr_set_bcr(s, 0, val, s->cfg & 0x80000000);
             break;
         case 0x44: /* SDRAM_B1CR */
-            sdram_ddr_set_bcr(sdram, 1, val, sdram->cfg & 0x80000000);
+            sdram_ddr_set_bcr(s, 1, val, s->cfg & 0x80000000);
             break;
         case 0x48: /* SDRAM_B2CR */
-            sdram_ddr_set_bcr(sdram, 2, val, sdram->cfg & 0x80000000);
+            sdram_ddr_set_bcr(s, 2, val, s->cfg & 0x80000000);
             break;
         case 0x4C: /* SDRAM_B3CR */
-            sdram_ddr_set_bcr(sdram, 3, val, sdram->cfg & 0x80000000);
+            sdram_ddr_set_bcr(s, 3, val, s->cfg & 0x80000000);
             break;
         case 0x80: /* SDRAM_TR */
-            sdram->tr = val & 0x018FC01F;
+            s->tr = val & 0x018FC01F;
             break;
         case 0x94: /* SDRAM_ECCCFG */
-            sdram->ecccfg = val & 0x00F00000;
+            s->ecccfg = val & 0x00F00000;
             break;
         case 0x98: /* SDRAM_ECCESR */
             val &= 0xFFF0F000;
-            if (sdram->eccesr == 0 && val != 0) {
-                qemu_irq_raise(sdram->irq);
-            } else if (sdram->eccesr != 0 && val == 0) {
-                qemu_irq_lower(sdram->irq);
+            if (s->eccesr == 0 && val != 0) {
+                qemu_irq_raise(s->irq);
+            } else if (s->eccesr != 0 && val == 0) {
+                qemu_irq_lower(s->irq);
             }
-            sdram->eccesr = val;
+            s->eccesr = val;
             break;
         default: /* Error */
             break;
@@ -386,21 +386,21 @@ static void sdram_ddr_dcr_write(void *opaque, int dcrn, uint32_t val)
 
 static void ppc4xx_sdram_ddr_reset(DeviceState *dev)
 {
-    Ppc4xxSdramDdrState *sdram = PPC4xx_SDRAM_DDR(dev);
-
-    sdram->addr = 0;
-    sdram->bear = 0;
-    sdram->besr0 = 0; /* No error */
-    sdram->besr1 = 0; /* No error */
-    sdram->cfg = 0;
-    sdram->ecccfg = 0; /* No ECC */
-    sdram->eccesr = 0; /* No error */
-    sdram->pmit = 0x07C00000;
-    sdram->rtr = 0x05F00000;
-    sdram->tr = 0x00854009;
+    Ppc4xxSdramDdrState *s = PPC4xx_SDRAM_DDR(dev);
+
+    s->addr = 0;
+    s->bear = 0;
+    s->besr0 = 0; /* No error */
+    s->besr1 = 0; /* No error */
+    s->cfg = 0;
+    s->ecccfg = 0; /* No ECC */
+    s->eccesr = 0; /* No error */
+    s->pmit = 0x07C00000;
+    s->rtr = 0x05F00000;
+    s->tr = 0x00854009;
     /* We pre-initialize RAM banks */
-    sdram->status = 0;
-    sdram->cfg = 0x00800000;
+    s->status = 0;
+    s->cfg = 0x00800000;
 }
 
 static void ppc4xx_sdram_ddr_realize(DeviceState *dev, Error **errp)
@@ -572,7 +572,7 @@ static void sdram_ddr2_unmap_bcr(Ppc4xxSdramDdr2State *sdram)
 
 static uint32_t sdram_ddr2_dcr_read(void *opaque, int dcrn)
 {
-    Ppc4xxSdramDdr2State *sdram = opaque;
+    Ppc4xxSdramDdr2State *s = opaque;
     uint32_t ret = 0;
 
     switch (dcrn) {
@@ -580,9 +580,9 @@ static uint32_t sdram_ddr2_dcr_read(void *opaque, int dcrn)
     case SDRAM_R1BAS:
     case SDRAM_R2BAS:
     case SDRAM_R3BAS:
-        if (sdram->bank[dcrn - SDRAM_R0BAS].size) {
-            ret = sdram_ddr2_bcr(sdram->bank[dcrn - SDRAM_R0BAS].base,
-                                 sdram->bank[dcrn - SDRAM_R0BAS].size);
+        if (s->bank[dcrn - SDRAM_R0BAS].size) {
+            ret = sdram_ddr2_bcr(s->bank[dcrn - SDRAM_R0BAS].base,
+                                 s->bank[dcrn - SDRAM_R0BAS].size);
         }
         break;
     case SDRAM_CONF1HB:
@@ -592,16 +592,16 @@ static uint32_t sdram_ddr2_dcr_read(void *opaque, int dcrn)
     case SDRAM_PLBADDUHB:
         break;
     case SDRAM0_CFGADDR:
-        ret = sdram->addr;
+        ret = s->addr;
         break;
     case SDRAM0_CFGDATA:
-        switch (sdram->addr) {
+        switch (s->addr) {
         case 0x14: /* SDRAM_MCSTAT (405EX) */
         case 0x1F:
             ret = 0x80000000;
             break;
         case 0x21: /* SDRAM_MCOPT2 */
-            ret = sdram->mcopt2;
+            ret = s->mcopt2;
             break;
         case 0x40: /* SDRAM_MB0CF */
             ret = 0x00008001;
@@ -627,7 +627,7 @@ static uint32_t sdram_ddr2_dcr_read(void *opaque, int dcrn)
 
 static void sdram_ddr2_dcr_write(void *opaque, int dcrn, uint32_t val)
 {
-    Ppc4xxSdramDdr2State *sdram = opaque;
+    Ppc4xxSdramDdr2State *s = opaque;
 
     switch (dcrn) {
     case SDRAM_R0BAS:
@@ -641,25 +641,25 @@ static void sdram_ddr2_dcr_write(void *opaque, int dcrn, uint32_t val)
     case SDRAM_PLBADDUHB:
         break;
     case SDRAM0_CFGADDR:
-        sdram->addr = val;
+        s->addr = val;
         break;
     case SDRAM0_CFGDATA:
-        switch (sdram->addr) {
+        switch (s->addr) {
         case 0x00: /* B0CR */
             break;
         case 0x21: /* SDRAM_MCOPT2 */
-            if (!(sdram->mcopt2 & SDRAM_DDR2_MCOPT2_DCEN) &&
+            if (!(s->mcopt2 & SDRAM_DDR2_MCOPT2_DCEN) &&
                 (val & SDRAM_DDR2_MCOPT2_DCEN)) {
                 trace_ppc4xx_sdram_enable("enable");
                 /* validate all RAM mappings */
-                sdram_ddr2_map_bcr(sdram);
-                sdram->mcopt2 |= SDRAM_DDR2_MCOPT2_DCEN;
-            } else if ((sdram->mcopt2 & SDRAM_DDR2_MCOPT2_DCEN) &&
+                sdram_ddr2_map_bcr(s);
+                s->mcopt2 |= SDRAM_DDR2_MCOPT2_DCEN;
+            } else if ((s->mcopt2 & SDRAM_DDR2_MCOPT2_DCEN) &&
                        !(val & SDRAM_DDR2_MCOPT2_DCEN)) {
                 trace_ppc4xx_sdram_enable("disable");
                 /* invalidate all RAM mappings */
-                sdram_ddr2_unmap_bcr(sdram);
-                sdram->mcopt2 &= ~SDRAM_DDR2_MCOPT2_DCEN;
+                sdram_ddr2_unmap_bcr(s);
+                s->mcopt2 &= ~SDRAM_DDR2_MCOPT2_DCEN;
             }
             break;
         default:
@@ -673,10 +673,10 @@ static void sdram_ddr2_dcr_write(void *opaque, int dcrn, uint32_t val)
 
 static void ppc4xx_sdram_ddr2_reset(DeviceState *dev)
 {
-    Ppc4xxSdramDdr2State *sdram = PPC4xx_SDRAM_DDR2(dev);
+    Ppc4xxSdramDdr2State *s = PPC4xx_SDRAM_DDR2(dev);
 
-    sdram->addr = 0;
-    sdram->mcopt2 = 0;
+    s->addr = 0;
+    s->mcopt2 = 0;
 }
 
 static void ppc4xx_sdram_ddr2_realize(DeviceState *dev, Error **errp)
-- 
2.30.4

[PATCH v7 6/8] ppc4xx_sdram: Generalise bank setup

[BALATON Zoltan]

Currently only base and size are set on initial bank creation and bcr
value is computed on mapping the region. Set bcr at init so the bcr
encoding method becomes local to the controller model and mapping and
unmapping can operate on the bank so it can be shared between
different controller models. This patch converts the DDR2 controller.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/ppc4xx_sdram.c | 91 ++++++++++++++++++++++---------------------
 hw/ppc/trace-events   |  1 +
 2 files changed, 48 insertions(+), 44 deletions(-)

diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
index 4bc53c8f01..63a33b8fd4 100644
--- a/hw/ppc/ppc4xx_sdram.c
+++ b/hw/ppc/ppc4xx_sdram.c
@@ -105,6 +105,7 @@ static void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
 
 static void sdram_bank_map(Ppc4xxSdramBank *bank)
 {
+    trace_ppc4xx_sdram_map(bank->base, bank->size);
     memory_region_init(&bank->container, NULL, "sdram-container", bank->size);
     memory_region_add_subregion(&bank->container, 0, &bank->ram);
     memory_region_add_subregion(get_system_memory(), bank->base,
@@ -113,11 +114,26 @@ static void sdram_bank_map(Ppc4xxSdramBank *bank)
 
 static void sdram_bank_unmap(Ppc4xxSdramBank *bank)
 {
+    trace_ppc4xx_sdram_unmap(bank->base, bank->size);
     memory_region_del_subregion(get_system_memory(), &bank->container);
     memory_region_del_subregion(&bank->container, &bank->ram);
     object_unparent(OBJECT(&bank->container));
 }
 
+static void sdram_bank_set_bcr(Ppc4xxSdramBank *bank, uint32_t bcr,
+                               hwaddr base, hwaddr size, int enabled)
+{
+    if (memory_region_is_mapped(&bank->container)) {
+        sdram_bank_unmap(bank);
+    }
+    bank->bcr = bcr;
+    bank->base = base;
+    bank->size = size;
+    if (enabled && (bcr & 1)) {
+        sdram_bank_map(bank);
+    }
+}
+
 enum {
     SDRAM0_CFGADDR = 0x010,
     SDRAM0_CFGDATA = 0x011,
@@ -455,6 +471,8 @@ void ppc4xx_sdram_ddr_enable(Ppc4xxSdramDdrState *s)
 
 /*****************************************************************************/
 /* DDR2 SDRAM controller */
+#define SDRAM_DDR2_BCR_MASK 0xffe0ffc1
+
 enum {
     SDRAM_R0BAS = 0x40,
     SDRAM_R1BAS,
@@ -528,48 +546,6 @@ static hwaddr sdram_ddr2_size(uint32_t bcr)
     return size;
 }
 
-static void sdram_ddr2_set_bcr(Ppc4xxSdramDdr2State *sdram, int i,
-                               uint32_t bcr, int enabled)
-{
-    if (sdram->bank[i].bcr & 1) {
-        /* First unmap RAM if enabled */
-        trace_ppc4xx_sdram_unmap(sdram_ddr2_base(sdram->bank[i].bcr),
-                                 sdram_ddr2_size(sdram->bank[i].bcr));
-        sdram_bank_unmap(&sdram->bank[i]);
-    }
-    sdram->bank[i].bcr = bcr & 0xffe0ffc1;
-    if (enabled && (bcr & 1)) {
-        trace_ppc4xx_sdram_map(sdram_ddr2_base(bcr), sdram_ddr2_size(bcr));
-        sdram_bank_map(&sdram->bank[i]);
-    }
-}
-
-static void sdram_ddr2_map_bcr(Ppc4xxSdramDdr2State *sdram)
-{
-    int i;
-
-    for (i = 0; i < sdram->nbanks; i++) {
-        if (sdram->bank[i].size) {
-            sdram_ddr2_set_bcr(sdram, i,
-                               sdram_ddr2_bcr(sdram->bank[i].base,
-                                              sdram->bank[i].size), 1);
-        } else {
-            sdram_ddr2_set_bcr(sdram, i, 0, 0);
-        }
-    }
-}
-
-static void sdram_ddr2_unmap_bcr(Ppc4xxSdramDdr2State *sdram)
-{
-    int i;
-
-    for (i = 0; i < sdram->nbanks; i++) {
-        if (sdram->bank[i].size) {
-            sdram_ddr2_set_bcr(sdram, i, sdram->bank[i].bcr & ~1, 0);
-        }
-    }
-}
-
 static uint32_t sdram_ddr2_dcr_read(void *opaque, int dcrn)
 {
     Ppc4xxSdramDdr2State *s = opaque;
@@ -628,6 +604,7 @@ static uint32_t sdram_ddr2_dcr_read(void *opaque, int dcrn)
 static void sdram_ddr2_dcr_write(void *opaque, int dcrn, uint32_t val)
 {
     Ppc4xxSdramDdr2State *s = opaque;
+    int i;
 
     switch (dcrn) {
     case SDRAM_R0BAS:
@@ -652,13 +629,25 @@ static void sdram_ddr2_dcr_write(void *opaque, int dcrn, uint32_t val)
                 (val & SDRAM_DDR2_MCOPT2_DCEN)) {
                 trace_ppc4xx_sdram_enable("enable");
                 /* validate all RAM mappings */
-                sdram_ddr2_map_bcr(s);
+                for (i = 0; i < s->nbanks; i++) {
+                    if (s->bank[i].size) {
+                        sdram_bank_set_bcr(&s->bank[i], s->bank[i].bcr,
+                                           s->bank[i].base, s->bank[i].size,
+                                           1);
+                    }
+                }
                 s->mcopt2 |= SDRAM_DDR2_MCOPT2_DCEN;
             } else if ((s->mcopt2 & SDRAM_DDR2_MCOPT2_DCEN) &&
                        !(val & SDRAM_DDR2_MCOPT2_DCEN)) {
                 trace_ppc4xx_sdram_enable("disable");
                 /* invalidate all RAM mappings */
-                sdram_ddr2_unmap_bcr(s);
+                for (i = 0; i < s->nbanks; i++) {
+                    if (s->bank[i].size) {
+                        sdram_bank_set_bcr(&s->bank[i], s->bank[i].bcr,
+                                           s->bank[i].base, s->bank[i].size,
+                                           0);
+                    }
+                }
                 s->mcopt2 &= ~SDRAM_DDR2_MCOPT2_DCEN;
             }
             break;
@@ -691,6 +680,7 @@ static void ppc4xx_sdram_ddr2_realize(DeviceState *dev, Error **errp)
         2 * GiB, 1 * GiB, 512 * MiB, 256 * MiB, 128 * MiB,
         64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 0
     };
+    int i;
 
     if (s->nbanks < 1 || s->nbanks > 4) {
         error_setg(errp, "Invalid number of RAM banks");
@@ -701,6 +691,19 @@ static void ppc4xx_sdram_ddr2_realize(DeviceState *dev, Error **errp)
         return;
     }
     ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank, valid_bank_sizes);
+    for (i = 0; i < s->nbanks; i++) {
+        if (s->bank[i].size) {
+            s->bank[i].bcr = sdram_ddr2_bcr(s->bank[i].base, s->bank[i].size);
+            s->bank[i].bcr &= SDRAM_DDR2_BCR_MASK;
+            sdram_bank_set_bcr(&s->bank[i], s->bank[i].bcr,
+                               s->bank[i].base, s->bank[i].size, 0);
+        } else {
+            sdram_bank_set_bcr(&s->bank[i], 0, 0, 0, 0);
+        }
+        trace_ppc4xx_sdram_init(sdram_ddr2_base(s->bank[i].bcr),
+                                sdram_ddr2_size(s->bank[i].bcr),
+                                s->bank[i].bcr);
+    }
 
     ppc4xx_dcr_register(dcr, SDRAM0_CFGADDR,
                         s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
index a07d5aca0f..3b3e4211d4 100644
--- a/hw/ppc/trace-events
+++ b/hw/ppc/trace-events
@@ -179,3 +179,4 @@ ppc405ep_clocks_setup(const char *trace) "%s"
 ppc4xx_sdram_enable(const char *trace) "%s SDRAM controller"
 ppc4xx_sdram_unmap(uint64_t addr, uint64_t size) "Unmap RAM area 0x%" PRIx64 " size 0x%" PRIx64
 ppc4xx_sdram_map(uint64_t addr, uint64_t size) "Map RAM area 0x%" PRIx64 " size 0x%" PRIx64
+ppc4xx_sdram_init(uint64_t base, uint64_t size, uint32_t bcr) "Init RAM area 0x%" PRIx64 " size 0x%" PRIx64 " bcr 0x%x"
-- 
2.30.4

Re: [PATCH v7 6/8] ppc4xx_sdram: Generalise bank setup

[Daniel Henrique Barboza]

On 10/19/22 13:02, BALATON Zoltan wrote:
> Currently only base and size are set on initial bank creation and bcr
> value is computed on mapping the region. Set bcr at init so the bcr
> encoding method becomes local to the controller model and mapping and
> unmapping can operate on the bank so it can be shared between
> different controller models. This patch converts the DDR2 controller.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   hw/ppc/ppc4xx_sdram.c | 91 ++++++++++++++++++++++---------------------
>   hw/ppc/trace-events   |  1 +
>   2 files changed, 48 insertions(+), 44 deletions(-)
> 
> diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
> index 4bc53c8f01..63a33b8fd4 100644
> --- a/hw/ppc/ppc4xx_sdram.c
> +++ b/hw/ppc/ppc4xx_sdram.c
> @@ -105,6 +105,7 @@ static void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
>   
>   static void sdram_bank_map(Ppc4xxSdramBank *bank)
>   {
> +    trace_ppc4xx_sdram_map(bank->base, bank->size);
>       memory_region_init(&bank->container, NULL, "sdram-container", bank->size);
>       memory_region_add_subregion(&bank->container, 0, &bank->ram);
>       memory_region_add_subregion(get_system_memory(), bank->base,
> @@ -113,11 +114,26 @@ static void sdram_bank_map(Ppc4xxSdramBank *bank)
>   
>   static void sdram_bank_unmap(Ppc4xxSdramBank *bank)
>   {
> +    trace_ppc4xx_sdram_unmap(bank->base, bank->size);
>       memory_region_del_subregion(get_system_memory(), &bank->container);
>       memory_region_del_subregion(&bank->container, &bank->ram);
>       object_unparent(OBJECT(&bank->container));
>   }
>   
> +static void sdram_bank_set_bcr(Ppc4xxSdramBank *bank, uint32_t bcr,
> +                               hwaddr base, hwaddr size, int enabled)
> +{
> +    if (memory_region_is_mapped(&bank->container)) {
> +        sdram_bank_unmap(bank);
> +    }
> +    bank->bcr = bcr;
> +    bank->base = base;
> +    bank->size = size;
> +    if (enabled && (bcr & 1)) {
> +        sdram_bank_map(bank);
> +    }
> +}
> +
>   enum {
>       SDRAM0_CFGADDR = 0x010,
>       SDRAM0_CFGDATA = 0x011,
> @@ -455,6 +471,8 @@ void ppc4xx_sdram_ddr_enable(Ppc4xxSdramDdrState *s)
>   
>   /*****************************************************************************/
>   /* DDR2 SDRAM controller */
> +#define SDRAM_DDR2_BCR_MASK 0xffe0ffc1
> +
>   enum {
>       SDRAM_R0BAS = 0x40,
>       SDRAM_R1BAS,
> @@ -528,48 +546,6 @@ static hwaddr sdram_ddr2_size(uint32_t bcr)
>       return size;
>   }
>   
> -static void sdram_ddr2_set_bcr(Ppc4xxSdramDdr2State *sdram, int i,
> -                               uint32_t bcr, int enabled)
> -{
> -    if (sdram->bank[i].bcr & 1) {
> -        /* First unmap RAM if enabled */
> -        trace_ppc4xx_sdram_unmap(sdram_ddr2_base(sdram->bank[i].bcr),
> -                                 sdram_ddr2_size(sdram->bank[i].bcr));
> -        sdram_bank_unmap(&sdram->bank[i]);
> -    }
> -    sdram->bank[i].bcr = bcr & 0xffe0ffc1;
> -    if (enabled && (bcr & 1)) {
> -        trace_ppc4xx_sdram_map(sdram_ddr2_base(bcr), sdram_ddr2_size(bcr));
> -        sdram_bank_map(&sdram->bank[i]);
> -    }
> -}
> -
> -static void sdram_ddr2_map_bcr(Ppc4xxSdramDdr2State *sdram)
> -{
> -    int i;
> -
> -    for (i = 0; i < sdram->nbanks; i++) {
> -        if (sdram->bank[i].size) {
> -            sdram_ddr2_set_bcr(sdram, i,
> -                               sdram_ddr2_bcr(sdram->bank[i].base,
> -                                              sdram->bank[i].size), 1);
> -        } else {
> -            sdram_ddr2_set_bcr(sdram, i, 0, 0);
> -        }
> -    }
> -}
> -
> -static void sdram_ddr2_unmap_bcr(Ppc4xxSdramDdr2State *sdram)
> -{
> -    int i;
> -
> -    for (i = 0; i < sdram->nbanks; i++) {
> -        if (sdram->bank[i].size) {
> -            sdram_ddr2_set_bcr(sdram, i, sdram->bank[i].bcr & ~1, 0);
> -        }
> -    }
> -}
> -
>   static uint32_t sdram_ddr2_dcr_read(void *opaque, int dcrn)
>   {
>       Ppc4xxSdramDdr2State *s = opaque;
> @@ -628,6 +604,7 @@ static uint32_t sdram_ddr2_dcr_read(void *opaque, int dcrn)
>   static void sdram_ddr2_dcr_write(void *opaque, int dcrn, uint32_t val)
>   {
>       Ppc4xxSdramDdr2State *s = opaque;
> +    int i;
>   
>       switch (dcrn) {
>       case SDRAM_R0BAS:
> @@ -652,13 +629,25 @@ static void sdram_ddr2_dcr_write(void *opaque, int dcrn, uint32_t val)
>                   (val & SDRAM_DDR2_MCOPT2_DCEN)) {
>                   trace_ppc4xx_sdram_enable("enable");
>                   /* validate all RAM mappings */
> -                sdram_ddr2_map_bcr(s);
> +                for (i = 0; i < s->nbanks; i++) {
> +                    if (s->bank[i].size) {
> +                        sdram_bank_set_bcr(&s->bank[i], s->bank[i].bcr,
> +                                           s->bank[i].base, s->bank[i].size,
> +                                           1);
> +                    }
> +                }
>                   s->mcopt2 |= SDRAM_DDR2_MCOPT2_DCEN;
>               } else if ((s->mcopt2 & SDRAM_DDR2_MCOPT2_DCEN) &&
>                          !(val & SDRAM_DDR2_MCOPT2_DCEN)) {
>                   trace_ppc4xx_sdram_enable("disable");
>                   /* invalidate all RAM mappings */
> -                sdram_ddr2_unmap_bcr(s);
> +                for (i = 0; i < s->nbanks; i++) {
> +                    if (s->bank[i].size) {
> +                        sdram_bank_set_bcr(&s->bank[i], s->bank[i].bcr,
> +                                           s->bank[i].base, s->bank[i].size,
> +                                           0);
> +                    }
> +                }
>                   s->mcopt2 &= ~SDRAM_DDR2_MCOPT2_DCEN;
>               }
>               break;
> @@ -691,6 +680,7 @@ static void ppc4xx_sdram_ddr2_realize(DeviceState *dev, Error **errp)
>           2 * GiB, 1 * GiB, 512 * MiB, 256 * MiB, 128 * MiB,
>           64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 0
>       };
> +    int i;
>   
>       if (s->nbanks < 1 || s->nbanks > 4) {
>           error_setg(errp, "Invalid number of RAM banks");
> @@ -701,6 +691,19 @@ static void ppc4xx_sdram_ddr2_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>       ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank, valid_bank_sizes);
> +    for (i = 0; i < s->nbanks; i++) {
> +        if (s->bank[i].size) {
> +            s->bank[i].bcr = sdram_ddr2_bcr(s->bank[i].base, s->bank[i].size);
> +            s->bank[i].bcr &= SDRAM_DDR2_BCR_MASK;
> +            sdram_bank_set_bcr(&s->bank[i], s->bank[i].bcr,
> +                               s->bank[i].base, s->bank[i].size, 0);
> +        } else {
> +            sdram_bank_set_bcr(&s->bank[i], 0, 0, 0, 0);
> +        }
> +        trace_ppc4xx_sdram_init(sdram_ddr2_base(s->bank[i].bcr),
> +                                sdram_ddr2_size(s->bank[i].bcr),
> +                                s->bank[i].bcr);
> +    }
>   
>       ppc4xx_dcr_register(dcr, SDRAM0_CFGADDR,
>                           s, &sdram_ddr2_dcr_read, &sdram_ddr2_dcr_write);
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index a07d5aca0f..3b3e4211d4 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -179,3 +179,4 @@ ppc405ep_clocks_setup(const char *trace) "%s"
>   ppc4xx_sdram_enable(const char *trace) "%s SDRAM controller"
>   ppc4xx_sdram_unmap(uint64_t addr, uint64_t size) "Unmap RAM area 0x%" PRIx64 " size 0x%" PRIx64
>   ppc4xx_sdram_map(uint64_t addr, uint64_t size) "Map RAM area 0x%" PRIx64 " size 0x%" PRIx64
> +ppc4xx_sdram_init(uint64_t base, uint64_t size, uint32_t bcr) "Init RAM area 0x%" PRIx64 " size 0x%" PRIx64 " bcr 0x%x"

[PATCH v7 7/8] ppc4xx_sdram: Convert DDR SDRAM controller to new bank handling

[BALATON Zoltan]

Use the generic bank handling introduced in previous patch in the DDR
SDRAM controller too. This also fixes previously broken region unmap
due to sdram_ddr_unmap_bcr() ignoring container region so it crashed
with an assert when the guest tried to disable the controller.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/ppc4xx_sdram.c | 98 ++++++++++++++++---------------------------
 1 file changed, 37 insertions(+), 61 deletions(-)

diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
index 63a33b8fd4..7c097efe20 100644
--- a/hw/ppc/ppc4xx_sdram.c
+++ b/hw/ppc/ppc4xx_sdram.c
@@ -141,6 +141,8 @@ enum {
 
 /*****************************************************************************/
 /* DDR SDRAM controller */
+#define SDRAM_DDR_BCR_MASK 0xFFDEE001
+
 static uint32_t sdram_ddr_bcr(hwaddr ram_base, hwaddr ram_size)
 {
     uint32_t bcr;
@@ -199,58 +201,6 @@ static hwaddr sdram_ddr_size(uint32_t bcr)
     return size;
 }
 
-static void sdram_ddr_set_bcr(Ppc4xxSdramDdrState *sdram, int i,
-                              uint32_t bcr, int enabled)
-{
-    if (sdram->bank[i].bcr & 1) {
-        /* Unmap RAM */
-        trace_ppc4xx_sdram_unmap(sdram_ddr_base(sdram->bank[i].bcr),
-                                 sdram_ddr_size(sdram->bank[i].bcr));
-        memory_region_del_subregion(get_system_memory(),
-                                    &sdram->bank[i].container);
-        memory_region_del_subregion(&sdram->bank[i].container,
-                                    &sdram->bank[i].ram);
-        object_unparent(OBJECT(&sdram->bank[i].container));
-    }
-    sdram->bank[i].bcr = bcr & 0xFFDEE001;
-    if (enabled && (bcr & 1)) {
-        trace_ppc4xx_sdram_map(sdram_ddr_base(bcr), sdram_ddr_size(bcr));
-        memory_region_init(&sdram->bank[i].container, NULL, "sdram-container",
-                           sdram_ddr_size(bcr));
-        memory_region_add_subregion(&sdram->bank[i].container, 0,
-                                    &sdram->bank[i].ram);
-        memory_region_add_subregion(get_system_memory(),
-                                    sdram_ddr_base(bcr),
-                                    &sdram->bank[i].container);
-    }
-}
-
-static void sdram_ddr_map_bcr(Ppc4xxSdramDdrState *sdram)
-{
-    int i;
-
-    for (i = 0; i < sdram->nbanks; i++) {
-        if (sdram->bank[i].size != 0) {
-            sdram_ddr_set_bcr(sdram, i, sdram_ddr_bcr(sdram->bank[i].base,
-                                                      sdram->bank[i].size), 1);
-        } else {
-            sdram_ddr_set_bcr(sdram, i, 0, 0);
-        }
-    }
-}
-
-static void sdram_ddr_unmap_bcr(Ppc4xxSdramDdrState *sdram)
-{
-    int i;
-
-    for (i = 0; i < sdram->nbanks; i++) {
-        trace_ppc4xx_sdram_unmap(sdram_ddr_base(sdram->bank[i].bcr),
-                                 sdram_ddr_size(sdram->bank[i].bcr));
-        memory_region_del_subregion(get_system_memory(),
-                                    &sdram->bank[i].ram);
-    }
-}
-
 static uint32_t sdram_ddr_dcr_read(void *opaque, int dcrn)
 {
     Ppc4xxSdramDdrState *s = opaque;
@@ -321,6 +271,7 @@ static uint32_t sdram_ddr_dcr_read(void *opaque, int dcrn)
 static void sdram_ddr_dcr_write(void *opaque, int dcrn, uint32_t val)
 {
     Ppc4xxSdramDdrState *s = opaque;
+    int i;
 
     switch (dcrn) {
     case SDRAM0_CFGADDR:
@@ -342,12 +293,24 @@ static void sdram_ddr_dcr_write(void *opaque, int dcrn, uint32_t val)
             if (!(s->cfg & 0x80000000) && (val & 0x80000000)) {
                 trace_ppc4xx_sdram_enable("enable");
                 /* validate all RAM mappings */
-                sdram_ddr_map_bcr(s);
+                for (i = 0; i < s->nbanks; i++) {
+                    if (s->bank[i].size) {
+                        sdram_bank_set_bcr(&s->bank[i], s->bank[i].bcr,
+                                           s->bank[i].base, s->bank[i].size,
+                                           1);
+                    }
+                }
                 s->status &= ~0x80000000;
             } else if ((s->cfg & 0x80000000) && !(val & 0x80000000)) {
                 trace_ppc4xx_sdram_enable("disable");
                 /* invalidate all RAM mappings */
-                sdram_ddr_unmap_bcr(s);
+                for (i = 0; i < s->nbanks; i++) {
+                    if (s->bank[i].size) {
+                        sdram_bank_set_bcr(&s->bank[i], s->bank[i].bcr,
+                                           s->bank[i].base, s->bank[i].size,
+                                           0);
+                    }
+                }
                 s->status |= 0x80000000;
             }
             if (!(s->cfg & 0x40000000) && (val & 0x40000000)) {
@@ -367,16 +330,16 @@ static void sdram_ddr_dcr_write(void *opaque, int dcrn, uint32_t val)
             s->pmit = (val & 0xF8000000) | 0x07C00000;
             break;
         case 0x40: /* SDRAM_B0CR */
-            sdram_ddr_set_bcr(s, 0, val, s->cfg & 0x80000000);
-            break;
         case 0x44: /* SDRAM_B1CR */
-            sdram_ddr_set_bcr(s, 1, val, s->cfg & 0x80000000);
-            break;
         case 0x48: /* SDRAM_B2CR */
-            sdram_ddr_set_bcr(s, 2, val, s->cfg & 0x80000000);
-            break;
         case 0x4C: /* SDRAM_B3CR */
-            sdram_ddr_set_bcr(s, 3, val, s->cfg & 0x80000000);
+            i = (s->addr - 0x40) / 4;
+            val &= SDRAM_DDR_BCR_MASK;
+            if (s->bank[i].size) {
+                sdram_bank_set_bcr(&s->bank[i], val,
+                                   sdram_ddr_base(val), sdram_ddr_size(val),
+                                   s->cfg & 0x80000000);
+            }
             break;
         case 0x80: /* SDRAM_TR */
             s->tr = val & 0x018FC01F;
@@ -426,6 +389,7 @@ static void ppc4xx_sdram_ddr_realize(DeviceState *dev, Error **errp)
     const ram_addr_t valid_bank_sizes[] = {
         256 * MiB, 128 * MiB, 64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 4 * MiB, 0
     };
+    int i;
 
     if (s->nbanks < 1 || s->nbanks > 4) {
         error_setg(errp, "Invalid number of RAM banks");
@@ -436,6 +400,18 @@ static void ppc4xx_sdram_ddr_realize(DeviceState *dev, Error **errp)
         return;
     }
     ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank, valid_bank_sizes);
+    for (i = 0; i < s->nbanks; i++) {
+        if (s->bank[i].size) {
+            s->bank[i].bcr = sdram_ddr_bcr(s->bank[i].base, s->bank[i].size);
+            sdram_bank_set_bcr(&s->bank[i], s->bank[i].bcr,
+                               s->bank[i].base, s->bank[i].size, 0);
+        } else {
+            sdram_bank_set_bcr(&s->bank[i], 0, 0, 0, 0);
+        }
+        trace_ppc4xx_sdram_init(sdram_ddr_base(s->bank[i].bcr),
+                                sdram_ddr_size(s->bank[i].bcr),
+                                s->bank[i].bcr);
+    }
 
     sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
 
-- 
2.30.4

Re: [PATCH v7 7/8] ppc4xx_sdram: Convert DDR SDRAM controller to new bank handling

[Daniel Henrique Barboza]

On 10/19/22 13:02, BALATON Zoltan wrote:
> Use the generic bank handling introduced in previous patch in the DDR
> SDRAM controller too. This also fixes previously broken region unmap
> due to sdram_ddr_unmap_bcr() ignoring container region so it crashed
> with an assert when the guest tried to disable the controller.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   hw/ppc/ppc4xx_sdram.c | 98 ++++++++++++++++---------------------------
>   1 file changed, 37 insertions(+), 61 deletions(-)
> 
> diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
> index 63a33b8fd4..7c097efe20 100644
> --- a/hw/ppc/ppc4xx_sdram.c
> +++ b/hw/ppc/ppc4xx_sdram.c
> @@ -141,6 +141,8 @@ enum {
>   
>   /*****************************************************************************/
>   /* DDR SDRAM controller */
> +#define SDRAM_DDR_BCR_MASK 0xFFDEE001
> +
>   static uint32_t sdram_ddr_bcr(hwaddr ram_base, hwaddr ram_size)
>   {
>       uint32_t bcr;
> @@ -199,58 +201,6 @@ static hwaddr sdram_ddr_size(uint32_t bcr)
>       return size;
>   }
>   
> -static void sdram_ddr_set_bcr(Ppc4xxSdramDdrState *sdram, int i,
> -                              uint32_t bcr, int enabled)
> -{
> -    if (sdram->bank[i].bcr & 1) {
> -        /* Unmap RAM */
> -        trace_ppc4xx_sdram_unmap(sdram_ddr_base(sdram->bank[i].bcr),
> -                                 sdram_ddr_size(sdram->bank[i].bcr));
> -        memory_region_del_subregion(get_system_memory(),
> -                                    &sdram->bank[i].container);
> -        memory_region_del_subregion(&sdram->bank[i].container,
> -                                    &sdram->bank[i].ram);
> -        object_unparent(OBJECT(&sdram->bank[i].container));
> -    }
> -    sdram->bank[i].bcr = bcr & 0xFFDEE001;
> -    if (enabled && (bcr & 1)) {
> -        trace_ppc4xx_sdram_map(sdram_ddr_base(bcr), sdram_ddr_size(bcr));
> -        memory_region_init(&sdram->bank[i].container, NULL, "sdram-container",
> -                           sdram_ddr_size(bcr));
> -        memory_region_add_subregion(&sdram->bank[i].container, 0,
> -                                    &sdram->bank[i].ram);
> -        memory_region_add_subregion(get_system_memory(),
> -                                    sdram_ddr_base(bcr),
> -                                    &sdram->bank[i].container);
> -    }
> -}
> -
> -static void sdram_ddr_map_bcr(Ppc4xxSdramDdrState *sdram)
> -{
> -    int i;
> -
> -    for (i = 0; i < sdram->nbanks; i++) {
> -        if (sdram->bank[i].size != 0) {
> -            sdram_ddr_set_bcr(sdram, i, sdram_ddr_bcr(sdram->bank[i].base,
> -                                                      sdram->bank[i].size), 1);
> -        } else {
> -            sdram_ddr_set_bcr(sdram, i, 0, 0);
> -        }
> -    }
> -}
> -
> -static void sdram_ddr_unmap_bcr(Ppc4xxSdramDdrState *sdram)
> -{
> -    int i;
> -
> -    for (i = 0; i < sdram->nbanks; i++) {
> -        trace_ppc4xx_sdram_unmap(sdram_ddr_base(sdram->bank[i].bcr),
> -                                 sdram_ddr_size(sdram->bank[i].bcr));
> -        memory_region_del_subregion(get_system_memory(),
> -                                    &sdram->bank[i].ram);
> -    }
> -}
> -
>   static uint32_t sdram_ddr_dcr_read(void *opaque, int dcrn)
>   {
>       Ppc4xxSdramDdrState *s = opaque;
> @@ -321,6 +271,7 @@ static uint32_t sdram_ddr_dcr_read(void *opaque, int dcrn)
>   static void sdram_ddr_dcr_write(void *opaque, int dcrn, uint32_t val)
>   {
>       Ppc4xxSdramDdrState *s = opaque;
> +    int i;
>   
>       switch (dcrn) {
>       case SDRAM0_CFGADDR:
> @@ -342,12 +293,24 @@ static void sdram_ddr_dcr_write(void *opaque, int dcrn, uint32_t val)
>               if (!(s->cfg & 0x80000000) && (val & 0x80000000)) {
>                   trace_ppc4xx_sdram_enable("enable");
>                   /* validate all RAM mappings */
> -                sdram_ddr_map_bcr(s);
> +                for (i = 0; i < s->nbanks; i++) {
> +                    if (s->bank[i].size) {
> +                        sdram_bank_set_bcr(&s->bank[i], s->bank[i].bcr,
> +                                           s->bank[i].base, s->bank[i].size,
> +                                           1);
> +                    }
> +                }
>                   s->status &= ~0x80000000;
>               } else if ((s->cfg & 0x80000000) && !(val & 0x80000000)) {
>                   trace_ppc4xx_sdram_enable("disable");
>                   /* invalidate all RAM mappings */
> -                sdram_ddr_unmap_bcr(s);
> +                for (i = 0; i < s->nbanks; i++) {
> +                    if (s->bank[i].size) {
> +                        sdram_bank_set_bcr(&s->bank[i], s->bank[i].bcr,
> +                                           s->bank[i].base, s->bank[i].size,
> +                                           0);
> +                    }
> +                }
>                   s->status |= 0x80000000;
>               }
>               if (!(s->cfg & 0x40000000) && (val & 0x40000000)) {
> @@ -367,16 +330,16 @@ static void sdram_ddr_dcr_write(void *opaque, int dcrn, uint32_t val)
>               s->pmit = (val & 0xF8000000) | 0x07C00000;
>               break;
>           case 0x40: /* SDRAM_B0CR */
> -            sdram_ddr_set_bcr(s, 0, val, s->cfg & 0x80000000);
> -            break;
>           case 0x44: /* SDRAM_B1CR */
> -            sdram_ddr_set_bcr(s, 1, val, s->cfg & 0x80000000);
> -            break;
>           case 0x48: /* SDRAM_B2CR */
> -            sdram_ddr_set_bcr(s, 2, val, s->cfg & 0x80000000);
> -            break;
>           case 0x4C: /* SDRAM_B3CR */
> -            sdram_ddr_set_bcr(s, 3, val, s->cfg & 0x80000000);
> +            i = (s->addr - 0x40) / 4;
> +            val &= SDRAM_DDR_BCR_MASK;
> +            if (s->bank[i].size) {
> +                sdram_bank_set_bcr(&s->bank[i], val,
> +                                   sdram_ddr_base(val), sdram_ddr_size(val),
> +                                   s->cfg & 0x80000000);
> +            }
>               break;
>           case 0x80: /* SDRAM_TR */
>               s->tr = val & 0x018FC01F;
> @@ -426,6 +389,7 @@ static void ppc4xx_sdram_ddr_realize(DeviceState *dev, Error **errp)
>       const ram_addr_t valid_bank_sizes[] = {
>           256 * MiB, 128 * MiB, 64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 4 * MiB, 0
>       };
> +    int i;
>   
>       if (s->nbanks < 1 || s->nbanks > 4) {
>           error_setg(errp, "Invalid number of RAM banks");
> @@ -436,6 +400,18 @@ static void ppc4xx_sdram_ddr_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>       ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank, valid_bank_sizes);
> +    for (i = 0; i < s->nbanks; i++) {
> +        if (s->bank[i].size) {
> +            s->bank[i].bcr = sdram_ddr_bcr(s->bank[i].base, s->bank[i].size);
> +            sdram_bank_set_bcr(&s->bank[i], s->bank[i].bcr,
> +                               s->bank[i].base, s->bank[i].size, 0);
> +        } else {
> +            sdram_bank_set_bcr(&s->bank[i], 0, 0, 0, 0);
> +        }
> +        trace_ppc4xx_sdram_init(sdram_ddr_base(s->bank[i].bcr),
> +                                sdram_ddr_size(s->bank[i].bcr),
> +                                s->bank[i].bcr);
> +    }
>   
>       sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
>   

[PATCH v7 8/8] ppc4xx_sdram: Add errp parameter to ppc4xx_sdram_banks()

[BALATON Zoltan]

Do not exit from ppc4xx_sdram_banks() but report error via an errp
parameter instead.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ppc/ppc4xx_sdram.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/ppc4xx_sdram.c b/hw/ppc/ppc4xx_sdram.c
index 7c097efe20..8d7137faf3 100644
--- a/hw/ppc/ppc4xx_sdram.c
+++ b/hw/ppc/ppc4xx_sdram.c
@@ -52,10 +52,12 @@
  * must be one of a small set of sizes. The number of banks and the supported
  * sizes varies by SoC.
  */
-static void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
+static bool ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
                                Ppc4xxSdramBank ram_banks[],
-                               const ram_addr_t sdram_bank_sizes[])
+                               const ram_addr_t sdram_bank_sizes[],
+                               Error **errp)
 {
+    ERRP_GUARD();
     ram_addr_t size_left = memory_region_size(ram);
     ram_addr_t base = 0;
     ram_addr_t bank_size;
@@ -93,14 +95,16 @@ static void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
                                    sdram_bank_sizes[i] / MiB,
                                    sdram_bank_sizes[i + 1] ? ", " : "");
         }
-        error_report("at most %d bank%s of %s MiB each supported",
-                     nr_banks, nr_banks == 1 ? "" : "s", s->str);
-        error_printf("Possible valid RAM size: %" PRIi64 " MiB\n",
-            used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB);
+        error_setg(errp, "Invalid SDRAM banks");
+        error_append_hint(errp, "at most %d bank%s of %s MiB each supported\n",
+                          nr_banks, nr_banks == 1 ? "" : "s", s->str);
+        error_append_hint(errp, "Possible valid RAM size: %" PRIi64 " MiB\n",
+                  used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB);
 
         g_string_free(s, true);
-        exit(EXIT_FAILURE);
+        return false;
     }
+    return true;
 }
 
 static void sdram_bank_map(Ppc4xxSdramBank *bank)
@@ -399,7 +403,10 @@ static void ppc4xx_sdram_ddr_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "Missing dram memory region");
         return;
     }
-    ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank, valid_bank_sizes);
+    if (!ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank,
+                            valid_bank_sizes, errp)) {
+        return;
+    }
     for (i = 0; i < s->nbanks; i++) {
         if (s->bank[i].size) {
             s->bank[i].bcr = sdram_ddr_bcr(s->bank[i].base, s->bank[i].size);
@@ -666,7 +673,10 @@ static void ppc4xx_sdram_ddr2_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "Missing dram memory region");
         return;
     }
-    ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank, valid_bank_sizes);
+    if (!ppc4xx_sdram_banks(s->dram_mr, s->nbanks, s->bank,
+                            valid_bank_sizes, errp)) {
+        return;
+    }
     for (i = 0; i < s->nbanks; i++) {
         if (s->bank[i].size) {
             s->bank[i].bcr = sdram_ddr2_bcr(s->bank[i].base, s->bank[i].size);
-- 
2.30.4

Re: [PATCH v7 0/8] ppc4xx_sdram QOMify and clean ups

[Daniel Henrique Barboza]

Queued in gitlab.com/danielhb/qemu/tree/ppc-next.


Phil, your acks for these patches are using the f4bug@amsat.org email.
Let me know if you want to use your new work address instead.


Thanks,


Daniel

On 10/19/22 13:02, BALATON Zoltan wrote:
> This is the end of the QOMify series originially started by Cédric
> rebased on master now only including patches not yet merged. Patches
> that still need review are 1-3 (these only move code to
> ppc4xx_sdram.c) and 6-7 (unify DDR and DDR2 models to share code where
> possible).
> 
> Regards,
> BALATON Zoltan
> 
> v7: Rebase on master after merge of first part of the series
> v6: Split patch moving sdram controller models together into smaller steps
> v5: Add functions the enable sdram controller and call it from boards
> v4: address more review comments
> v3: Fix patches that got squashed during rebase
> v2: address some review comments and try to avoid compile problem with
> gcc 12.2 (untested)
> 
> 
> BALATON Zoltan (8):
>    ppc440_uc.c: Move DDR2 SDRAM controller model to ppc4xx_sdram.c
>    ppc4xx_devs.c: Move DDR SDRAM controller model to ppc4xx_sdram.c
>    ppc4xx_sdram: Move ppc4xx_sdram_banks() to ppc4xx_sdram.c
>    ppc4xx_sdram: Use hwaddr for memory bank size
>    ppc4xx_sdram: Rename local state variable for brevity
>    ppc4xx_sdram: Generalise bank setup
>    ppc4xx_sdram: Convert DDR SDRAM controller to new bank handling
>    ppc4xx_sdram: Add errp parameter to ppc4xx_sdram_banks()
> 
>   hw/ppc/meson.build      |   3 +-
>   hw/ppc/ppc440_uc.c      | 332 ------------------
>   hw/ppc/ppc4xx_devs.c    | 414 ----------------------
>   hw/ppc/ppc4xx_sdram.c   | 757 ++++++++++++++++++++++++++++++++++++++++
>   hw/ppc/trace-events     |   1 +
>   include/hw/ppc/ppc4xx.h |  20 +-
>   6 files changed, 768 insertions(+), 759 deletions(-)
>   create mode 100644 hw/ppc/ppc4xx_sdram.c
>