[Qemu-devel] [PATCH 0/5] ast2400: some cleanups and a simple memory controller model

[Qemu-devel] [PATCH 0/5] ast2400: some cleanups and a simple memory controller model

[Cédric Le Goater]

Hello,

Here are a couple of cleanups and a very basic memory controller model
providing a configuration register. This is needed to determine the
RAM size of the SOC, only used by U-Boot as of today.

Ultimately, we would want to add more support for U-Boot and be able
to boot from flash 0 but the device model of the m25p80 object is not
designed to use a memory region. And so, it is difficult to define a
rom device for the flash backend.

Some initial patches were proposed but they are not being friendly
with the object interface. Work in progress.

Thanks,

C. 

Cédric Le Goater (5):
  hw/misc: fix typo in Aspeed SCU hw-strap2 property name
  ast2400: replace aspeed_smc_is_implemented()
  ast2400: pretend DMAs are done for U-boot
  ast2400: externalize revision numbers
  ast2400: add a memory controller device model

 hw/arm/ast2400.c              |  17 ++-
 hw/misc/Makefile.objs         |   2 +-
 hw/misc/aspeed_scu.c          |   6 +-
 hw/misc/aspeed_sdmc.c         | 245 ++++++++++++++++++++++++++++++++++++++++++
 hw/ssi/aspeed_smc.c           |  39 ++++---
 include/hw/arm/ast2400.h      |   2 +
 include/hw/misc/aspeed_scu.h  |   5 +
 include/hw/misc/aspeed_sdmc.h |  31 ++++++
 8 files changed, 320 insertions(+), 27 deletions(-)
 create mode 100644 hw/misc/aspeed_sdmc.c
 create mode 100644 include/hw/misc/aspeed_sdmc.h

-- 
2.1.4

[Qemu-devel] [PATCH 1/5] hw/misc: fix typo in Aspeed SCU hw-strap2 property name

[Cédric Le Goater]

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/misc/aspeed_scu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 23f51752b0d2..b61c05ea4dbc 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -255,7 +255,7 @@ static const VMStateDescription vmstate_aspeed_scu = {
 static Property aspeed_scu_properties[] = {
     DEFINE_PROP_UINT32("silicon-rev", AspeedSCUState, silicon_rev, 0),
     DEFINE_PROP_UINT32("hw-strap1", AspeedSCUState, hw_strap1, 0),
-    DEFINE_PROP_UINT32("hw-strap2", AspeedSCUState, hw_strap1, 0),
+    DEFINE_PROP_UINT32("hw-strap2", AspeedSCUState, hw_strap2, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.1.4

[Qemu-devel] [PATCH 2/5] ast2400: replace aspeed_smc_is_implemented()

[Cédric Le Goater]

aspeed_smc_is_implemented() filters invalid registers in a peculiar
way. Let's remove it and open code the if conditions. It serves the
same purpose, the aesthetic is better, and new registers can easily be
added.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index a371e302d448..854474b642ea 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -281,12 +281,6 @@ static void aspeed_smc_reset(DeviceState *d)
     aspeed_smc_update_cs(s);
 }
 
-static bool aspeed_smc_is_implemented(AspeedSMCState *s, hwaddr addr)
-{
-    return (addr == s->r_conf || addr == s->r_timings || addr == s->r_ce_ctrl ||
-            (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs));
-}
-
 static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
 {
     AspeedSMCState *s = ASPEED_SMC(opaque);
@@ -300,13 +294,16 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
         return 0;
     }
 
-    if (!aspeed_smc_is_implemented(s, addr)) {
+    if (addr == s->r_conf ||
+        addr == s->r_timings ||
+        addr == s->r_ce_ctrl ||
+        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs)) {
+        return s->regs[addr];
+    } else {
         qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
-                __func__, addr);
+                      __func__, addr);
         return 0;
     }
-
-    return s->regs[addr];
 }
 
 static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
@@ -324,20 +321,18 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
         return;
     }
 
-    if (!aspeed_smc_is_implemented(s, addr)) {
+    if (addr == s->r_conf ||
+        addr == s->r_timings ||
+        addr == s->r_ce_ctrl) {
+        s->regs[addr] = value;
+    } else if (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs) {
+        s->regs[addr] = value;
+        aspeed_smc_update_cs(s);
+    } else {
         qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
                       __func__, addr);
         return;
     }
-
-    /*
-     * Not much to do apart from storing the value and set the cs
-     * lines if the register is a controlling one.
-     */
-    s->regs[addr] = value;
-    if (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs) {
-        aspeed_smc_update_cs(s);
-    }
 }
 
 static const MemoryRegionOps aspeed_smc_ops = {
-- 
2.1.4

[Qemu-devel] [PATCH 3/5] ast2400: pretend DMAs are done for U-boot

[Cédric Le Goater]

U-boot does SPI timing calibration using DMA tranfers. To let the
initialization continue, we fake success by setting the DMA status of
the Interrupt Control Register.

For the moment, DMA support is not required as it is not used in
normal operation.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 854474b642ea..d319e04a27f0 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -273,6 +273,9 @@ static void aspeed_smc_reset(DeviceState *d)
 
     memset(s->regs, 0, sizeof s->regs);
 
+    /* Pretend DMA is done (u-boot initialization) */
+    s->regs[R_INTR_CTRL] = INTR_CTRL_DMA_STATUS;
+
     /* Unselect all slaves */
     for (i = 0; i < s->num_cs; ++i) {
         s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
@@ -297,6 +300,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
     if (addr == s->r_conf ||
         addr == s->r_timings ||
         addr == s->r_ce_ctrl ||
+        addr == R_INTR_CTRL ||
         (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs)) {
         return s->regs[addr];
     } else {
-- 
2.1.4

[Qemu-devel] [PATCH 4/5] ast2400: externalize revision numbers

[Cédric Le Goater]

AST2400_A0_SILICON_REV is defined twice. Fix this by including the
definition in the header file as well as the routine to check if a
silicon revision is supported. It will useful to reuse in other
controllers.

Let's add also AST2500_A0_SILICON_REV for future use.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/ast2400.c             | 2 --
 hw/misc/aspeed_scu.c         | 4 +---
 include/hw/misc/aspeed_scu.h | 5 +++++
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
index 055584362073..326fdb36eed5 100644
--- a/hw/arm/ast2400.c
+++ b/hw/arm/ast2400.c
@@ -34,8 +34,6 @@
 #define AST2400_FMC_FLASH_BASE   0x20000000
 #define AST2400_SPI_FLASH_BASE   0x30000000
 
-#define AST2400_A0_SILICON_REV   0x02000303
-
 static const int uart_irqs[] = { 9, 32, 33, 34, 10 };
 static const int timer_irqs[] = { 16, 17, 18, 35, 36, 37, 38, 39, };
 
diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index b61c05ea4dbc..c7e2c8263f55 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -88,8 +88,6 @@
 #define PROT_KEY_UNLOCK 0x1688A8A8
 #define SCU_IO_REGION_SIZE 0x20000
 
-#define AST2400_A0_SILICON_REV     0x02000303U
-
 static const uint32_t ast2400_a0_resets[ASPEED_SCU_NR_REGS] = {
      [SYS_RST_CTRL]    = 0xFFCFFEDCU,
      [CLK_SEL]         = 0xF3F40000U,
@@ -212,7 +210,7 @@ static void aspeed_scu_reset(DeviceState *dev)
 
 static uint32_t aspeed_silicon_revs[] = { AST2400_A0_SILICON_REV, };
 
-static bool is_supported_silicon_rev(uint32_t silicon_rev)
+bool is_supported_silicon_rev(uint32_t silicon_rev)
 {
     int i;
 
diff --git a/include/hw/misc/aspeed_scu.h b/include/hw/misc/aspeed_scu.h
index 6b8e46f85fad..fdfd982288f2 100644
--- a/include/hw/misc/aspeed_scu.h
+++ b/include/hw/misc/aspeed_scu.h
@@ -31,4 +31,9 @@ typedef struct AspeedSCUState {
     uint32_t hw_strap2;
 } AspeedSCUState;
 
+#define AST2400_A0_SILICON_REV   0x02000303U
+#define AST2500_A0_SILICON_REV   0x04000303U
+
+extern bool is_supported_silicon_rev(uint32_t silicon_rev);
+
 #endif /* ASPEED_SCU_H */
-- 
2.1.4

[Qemu-devel] [PATCH 5/5] ast2400: add a memory controller device model

[Cédric Le Goater]

The uboot in the previous release of the SDK was using a hardcoded
value for memory size. This is not true anymore, the value is now
retrieved from the memory controller.

Below is a model for this device, only supporting unlock and
configuration. Without it, we endup running a guest with 64MB, which
is a bit low nowdays. It uses a 'silicon-rev' property and ram_size to
build a default value. Some bits should be linked to SCU strapping
registers but it seems a bit complex to add for the current need.

The model is ready for the AST2500 SOC.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 Changes since v1:
 
 - fixed compile issue (missing header hw/arm/ast2400.h)
 - added bit definitions, for AST2400 and AST2500
 - added default configuration depending on the silicon revision
 - calculated ram bits using ram_size instead of using a property

 hw/arm/ast2400.c              |  15 +++
 hw/misc/Makefile.objs         |   2 +-
 hw/misc/aspeed_sdmc.c         | 245 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/arm/ast2400.h      |   2 +
 include/hw/misc/aspeed_sdmc.h |  31 ++++++
 5 files changed, 294 insertions(+), 1 deletion(-)
 create mode 100644 hw/misc/aspeed_sdmc.c
 create mode 100644 include/hw/misc/aspeed_sdmc.h

diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
index 326fdb36eed5..136bf6464e1d 100644
--- a/hw/arm/ast2400.c
+++ b/hw/arm/ast2400.c
@@ -27,6 +27,7 @@
 #define AST2400_FMC_BASE         0X1E620000
 #define AST2400_SPI_BASE         0X1E630000
 #define AST2400_VIC_BASE         0x1E6C0000
+#define AST2400_SDMC_BASE        0x1E6E0000
 #define AST2400_SCU_BASE         0x1E6E2000
 #define AST2400_TIMER_BASE       0x1E782000
 #define AST2400_I2C_BASE         0x1E78A000
@@ -97,6 +98,12 @@ static void ast2400_init(Object *obj)
     object_initialize(&s->spi, sizeof(s->spi), "aspeed.smc.spi");
     object_property_add_child(obj, "spi", OBJECT(&s->spi), NULL);
     qdev_set_parent_bus(DEVICE(&s->spi), sysbus_get_default());
+
+    object_initialize(&s->sdmc, sizeof(s->sdmc), TYPE_ASPEED_SDMC);
+    object_property_add_child(obj, "sdmc", OBJECT(&s->sdmc), NULL);
+    qdev_set_parent_bus(DEVICE(&s->sdmc), sysbus_get_default());
+    qdev_prop_set_uint32(DEVICE(&s->sdmc), "silicon-rev",
+                         AST2400_A0_SILICON_REV);
 }
 
 static void ast2400_realize(DeviceState *dev, Error **errp)
@@ -183,6 +190,14 @@ static void ast2400_realize(DeviceState *dev, Error **errp)
     }
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 0, AST2400_SPI_BASE);
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 1, AST2400_SPI_FLASH_BASE);
+
+    /* SDMC - SDRAM Memory Controller */
+    object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, AST2400_SDMC_BASE);
 }
 
 static void ast2400_class_init(ObjectClass *oc, void *data)
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 4cfbd1024a93..1a89615a6270 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -52,4 +52,4 @@ obj-$(CONFIG_PVPANIC) += pvpanic.o
 obj-$(CONFIG_EDU) += edu.o
 obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o
 obj-$(CONFIG_AUX) += auxbus.o
-obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o
+obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
new file mode 100644
index 000000000000..08e780c3b9d4
--- /dev/null
+++ b/hw/misc/aspeed_sdmc.c
@@ -0,0 +1,245 @@
+/*
+ * ASPEED SDRAM Memory Controller
+ *
+ * Copyright (C) 2016 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/misc/aspeed_sdmc.h"
+#include "hw/misc/aspeed_scu.h"
+#include "hw/qdev-properties.h"
+#include "qapi/error.h"
+#include "trace.h"
+
+/* Protection Key Register */
+#define R_PROT            (0x00 / 4)
+#define   PROT_KEY_UNLOCK     0xFC600309
+
+/* Configuration Register */
+#define R_CONF            (0x04 / 4)
+
+/*
+ * Configuration register Ox4 (for Aspeed AST2400 SOC)
+ *
+ * These are for the record and future use. ASPEED_SDMC_DRAM_SIZE is
+ * what we care about right now as it is checked by U-Boot to
+ * determine the RAM size.
+ */
+
+#define ASPEED_SDMC_AST2300_COMPAT      (1 << 10)
+#define ASPEED_SDMC_SCRAMBLE_PATTERN    (1 << 9)
+#define ASPEED_SDMC_DATA_SCRAMBLE       (1 << 8)
+#define ASPEED_SDMC_ECC_ENABLE          (1 << 7)
+#define ASPEED_SDMC_VGA_COMPAT          (1 << 6) /* readonly */
+#define ASPEED_SDMC_DRAM_BANK           (1 << 5)
+#define ASPEED_SDMC_DRAM_BURST          (1 << 4)
+#define ASPEED_SDMC_VGA_APERTURE(x)     ((x & 0x3) << 2) /* readonly */
+#define     ASPEED_SDMC_VGA_8MB             0x0
+#define     ASPEED_SDMC_VGA_16MB            0x1
+#define     ASPEED_SDMC_VGA_32MB            0x2
+#define     ASPEED_SDMC_VGA_64MB            0x3
+#define ASPEED_SDMC_DRAM_SIZE(x)        (x & 0x3)
+#define     ASPEED_SDMC_DRAM_64MB           0x0
+#define     ASPEED_SDMC_DRAM_128MB          0x1
+#define     ASPEED_SDMC_DRAM_256MB          0x2
+#define     ASPEED_SDMC_DRAM_512MB          0x3
+
+/*
+ * Configuration register Ox4 (for Aspeed AST2500 SOC and higher)
+ *
+ * Incompatibilities are annoted in the list. ASPEED_SDMC_HW_VERSION
+ * should be set to 1 for the AST2500 SOC.
+ */
+#define ASPEED_SDMC_HW_VERSION(x)       ((x & 0xf) << 28) /* readonly */
+#define ASPEED_SDMC_SW_VERSION          ((x & 0xff) << 20)
+#define ASPEED_SDMC_CACHE_INITIAL_DONE  (1 << 19) /* readonly */
+#define ASPEED_SDMC_CACHE_DDR4_CONF     (1 << 13)
+#define ASPEED_SDMC_CACHE_INITIAL       (1 << 12)
+#define ASPEED_SDMC_CACHE_RANGE_CTRL    (1 << 11)
+#define ASPEED_SDMC_CACHE_ENABLE        (1 << 10) /* differs from AST2400 */
+#define ASPEED_SDMC_DRAM_TYPE           (1 << 4)  /* differs from AST2400 */
+
+/* DRAM size definitions differs */
+#define     ASPEED_SDMC_AST2500_128MB       0x0
+#define     ASPEED_SDMC_AST2500_256MB       0x1
+#define     ASPEED_SDMC_AST2500_512MB       0x2
+#define     ASPEED_SDMC_AST2500_1024MB      0x3
+
+
+static uint64_t aspeed_sdmc_read(void *opaque, hwaddr addr, unsigned size)
+{
+    AspeedSDMCState *s = ASPEED_SDMC(opaque);
+
+    addr >>= 2;
+
+    if (addr >= ARRAY_SIZE(s->regs)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
+                      __func__, addr);
+        return 0;
+    }
+
+    return s->regs[addr];
+}
+
+static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
+                             unsigned int size)
+{
+    AspeedSDMCState *s = ASPEED_SDMC(opaque);
+
+    addr >>= 2;
+
+    if (addr >= ARRAY_SIZE(s->regs)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
+                      __func__, addr);
+        return;
+    }
+
+    if (addr != R_PROT && s->regs[R_PROT] != PROT_KEY_UNLOCK) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
+        return;
+    }
+
+    if (addr == R_CONF) {
+        /* Make sure readonly bits are kept */
+        switch (s->silicon_rev) {
+        case AST2400_A0_SILICON_REV:
+            data &= 0x000007B3;
+            break;
+        case AST2500_A0_SILICON_REV:
+            data &= 0x0FF03FB3;
+            break;
+        default:
+            g_assert_not_reached();
+        }
+    }
+
+    s->regs[addr] = data;
+}
+
+static const MemoryRegionOps aspeed_sdmc_ops = {
+    .read = aspeed_sdmc_read,
+    .write = aspeed_sdmc_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
+    .valid.unaligned = false,
+};
+
+static int ast2400_rambits(void)
+{
+    switch (ram_size >> 20) {
+    case   64: return ASPEED_SDMC_DRAM_64MB;
+    case  128: return ASPEED_SDMC_DRAM_128MB;
+    case  256: return ASPEED_SDMC_DRAM_256MB;
+    case  512: return ASPEED_SDMC_DRAM_512MB;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
+                      HWADDR_PRIx "\n", __func__, ram_size);
+        break;
+    }
+
+    /* set a minimum default */
+    return ASPEED_SDMC_DRAM_64MB;
+}
+
+static int ast2500_rambits(void)
+{
+    switch (ram_size >> 20) {
+    case  128: return ASPEED_SDMC_AST2500_128MB;
+    case  256: return ASPEED_SDMC_AST2500_256MB;
+    case  512: return ASPEED_SDMC_AST2500_512MB;
+    case 1024: return ASPEED_SDMC_AST2500_1024MB;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
+                      HWADDR_PRIx "\n", __func__, ram_size);
+        break;
+    }
+
+    /* set a minimum default */
+    return ASPEED_SDMC_AST2500_128MB;
+}
+
+static void aspeed_sdmc_reset(DeviceState *dev)
+{
+    AspeedSDMCState *s = ASPEED_SDMC(dev);
+
+    memset(s->regs, 0, sizeof(s->regs));
+
+    /* Set ram size bit and defaults values */
+    switch (s->silicon_rev) {
+    case AST2400_A0_SILICON_REV:
+        s->regs[R_CONF] |=
+            ASPEED_SDMC_VGA_COMPAT |
+            ASPEED_SDMC_DRAM_SIZE(ast2400_rambits());
+        break;
+
+    case AST2500_A0_SILICON_REV:
+        s->regs[R_CONF] |=
+            ASPEED_SDMC_HW_VERSION(1) |
+            ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
+            ASPEED_SDMC_DRAM_SIZE(ast2500_rambits());
+    default:
+        g_assert_not_reached();
+    }
+}
+
+static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    AspeedSDMCState *s = ASPEED_SDMC(dev);
+
+    if (!is_supported_silicon_rev(s->silicon_rev)) {
+        error_setg(errp, "Unknown silicon revision: 0x%" PRIx32,
+                s->silicon_rev);
+        return;
+    }
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s,
+                          TYPE_ASPEED_SDMC, 0x1000);
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static const VMStateDescription vmstate_aspeed_sdmc = {
+    .name = "aspeed.sdmc",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, AspeedSDMCState, ASPEED_SDMC_NR_REGS),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property aspeed_sdmc_properties[] = {
+    DEFINE_PROP_UINT32("silicon-rev", AspeedSDMCState, silicon_rev, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void aspeed_sdmc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    dc->realize = aspeed_sdmc_realize;
+    dc->reset = aspeed_sdmc_reset;
+    dc->desc = "ASPEED SDRAM Memory Controller";
+    dc->vmsd = &vmstate_aspeed_sdmc;
+    dc->props = aspeed_sdmc_properties;
+}
+
+static const TypeInfo aspeed_sdmc_info = {
+    .name = TYPE_ASPEED_SDMC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AspeedSDMCState),
+    .class_init = aspeed_sdmc_class_init,
+};
+
+static void aspeed_sdmc_register_types(void)
+{
+    type_register_static(&aspeed_sdmc_info);
+}
+
+type_init(aspeed_sdmc_register_types);
diff --git a/include/hw/arm/ast2400.h b/include/hw/arm/ast2400.h
index 7833bc716cd8..e68807d475b7 100644
--- a/include/hw/arm/ast2400.h
+++ b/include/hw/arm/ast2400.h
@@ -15,6 +15,7 @@
 #include "hw/arm/arm.h"
 #include "hw/intc/aspeed_vic.h"
 #include "hw/misc/aspeed_scu.h"
+#include "hw/misc/aspeed_sdmc.h"
 #include "hw/timer/aspeed_timer.h"
 #include "hw/i2c/aspeed_i2c.h"
 #include "hw/ssi/aspeed_smc.h"
@@ -32,6 +33,7 @@ typedef struct AST2400State {
     AspeedSCUState scu;
     AspeedSMCState smc;
     AspeedSMCState spi;
+    AspeedSDMCState sdmc;
 } AST2400State;
 
 #define TYPE_AST2400 "ast2400"
diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
new file mode 100644
index 000000000000..7e081f6d2b86
--- /dev/null
+++ b/include/hw/misc/aspeed_sdmc.h
@@ -0,0 +1,31 @@
+/*
+ * ASPEED SDRAM Memory Controller
+ *
+ * Copyright (C) 2016 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+#ifndef ASPEED_SDMC_H
+#define ASPEED_SDMC_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_ASPEED_SDMC "aspeed.sdmc"
+#define ASPEED_SDMC(obj) OBJECT_CHECK(AspeedSDMCState, (obj), TYPE_ASPEED_SDMC)
+
+#define ASPEED_SDMC_NR_REGS (0x8 >> 2)
+
+typedef struct AspeedSDMCState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
+    MemoryRegion iomem;
+
+    uint32_t regs[ASPEED_SDMC_NR_REGS];
+    uint32_t silicon_rev;
+
+} AspeedSDMCState;
+
+#endif /* ASPEED_SDMC_H */
-- 
2.1.4

Re: [Qemu-devel] [PATCH 0/5] ast2400: some cleanups and a simple memory controller model

[Peter Maydell]

On 8 July 2016 at 17:06, Cédric Le Goater <clg@kaod.org> wrote:
> Hello,
>
> Here are a couple of cleanups and a very basic memory controller model
> providing a configuration register. This is needed to determine the
> RAM size of the SOC, only used by U-Boot as of today.
>
> Ultimately, we would want to add more support for U-Boot and be able
> to boot from flash 0 but the device model of the m25p80 object is not
> designed to use a memory region. And so, it is difficult to define a
> rom device for the flash backend.
>
> Some initial patches were proposed but they are not being friendly
> with the object interface. Work in progress.

Hi; the release process is now at the point where I wouldn't really
want to put this into 2.7; I can be persuaded if you have a solid
justification, though.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/5] ast2400: some cleanups and a simple memory controller model

[Cédric Le Goater]

Hello Peter,

On 07/12/2016 04:19 PM, Peter Maydell wrote:
> On 8 July 2016 at 17:06, Cédric Le Goater <clg@kaod.org> wrote:
>> Hello,
>>
>> Here are a couple of cleanups and a very basic memory controller model
>> providing a configuration register. This is needed to determine the
>> RAM size of the SOC, only used by U-Boot as of today.
>>
>> Ultimately, we would want to add more support for U-Boot and be able
>> to boot from flash 0 but the device model of the m25p80 object is not
>> designed to use a memory region. And so, it is difficult to define a
>> rom device for the flash backend.
>>
>> Some initial patches were proposed but they are not being friendly
>> with the object interface. Work in progress.
> 
> Hi; the release process is now at the point where I wouldn't really
> want to put this into 2.7; I can be persuaded if you have a solid
> justification, though.

Here is my take to it  :

* 1/5 hw/misc: fix typo in Aspeed SCU hw-strap2 property name
  
  That's a minor fix

* 2/5 ast2400: replace aspeed_smc_is_implemented()

  That is not a fix but it removes some ugliness in the code, which 
  you had noticed, and which prevents us from fixing possible issues 
  nicely. So this is a preventive cleanup :)

* 3/5 ast2400: pretend DMAs are done for U-boot

  This is a work around fix. U-boot[1] uses DMAs to calibrate the 
  SPI flash module. It will spin until the DMA transfers are done.

* 4/5 ast2400: externalize revision numbers

  This is minor fix de-duplicating code and a prereq for the next
  patch.

* 5/5  ast2400: add a memory controller device model

  That's an enhancement. It adds a basic device model for the memory
  controller. This is needed by some version of the SDK relying on
  the configuration register to know how much RAM the SOC has. Older
  versions used a hardcoded value.  


The first patches should not be too much of a problem regarding the 
soft freeze. The last can wait 2.8 if you prefer, we need more work 
to fully support u-boot anyhow. I would be happy to have review 
though :) The model is very much like the SCU.

Thanks,

C.

[1] not supported yet in qemu because of the lack of a rom device 
    memory region needed to boot from the flash module.

Re: [Qemu-devel] [PATCH 0/5] ast2400: some cleanups and a simple memory controller model

[Cédric Le Goater]

On 07/12/2016 06:20 PM, Cédric Le Goater wrote:
> Hello Peter,
> 
> On 07/12/2016 04:19 PM, Peter Maydell wrote:
>> On 8 July 2016 at 17:06, Cédric Le Goater <clg@kaod.org> wrote:
>>> Hello,
>>>
>>> Here are a couple of cleanups and a very basic memory controller model
>>> providing a configuration register. This is needed to determine the
>>> RAM size of the SOC, only used by U-Boot as of today.
>>>
>>> Ultimately, we would want to add more support for U-Boot and be able
>>> to boot from flash 0 but the device model of the m25p80 object is not
>>> designed to use a memory region. And so, it is difficult to define a
>>> rom device for the flash backend.
>>>
>>> Some initial patches were proposed but they are not being friendly
>>> with the object interface. Work in progress.
>>
>> Hi; the release process is now at the point where I wouldn't really
>> want to put this into 2.7; I can be persuaded if you have a solid
>> justification, though.
> 
> Here is my take to it  :
> 
> * 1/5 hw/misc: fix typo in Aspeed SCU hw-strap2 property name
>   
>   That's a minor fix
> 
> * 2/5 ast2400: replace aspeed_smc_is_implemented()
> 
>   That is not a fix but it removes some ugliness in the code, which 
>   you had noticed, and which prevents us from fixing possible issues 
>   nicely. So this is a preventive cleanup :)
> 
> * 3/5 ast2400: pretend DMAs are done for U-boot
> 
>   This is a work around fix. U-boot[1] uses DMAs to calibrate the 
>   SPI flash module. It will spin until the DMA transfers are done.
> 
> * 4/5 ast2400: externalize revision numbers
> 
>   This is minor fix de-duplicating code and a prereq for the next
>   patch.
> 
> * 5/5  ast2400: add a memory controller device model
> 
>   That's an enhancement. It adds a basic device model for the memory
>   controller. This is needed by some version of the SDK relying on
>   the configuration register to know how much RAM the SOC has. Older
>   versions used a hardcoded value.  

oh yes and that's a v2. first version was sent on 06/29/2016.

Thanks,

C. 

> The first patches should not be too much of a problem regarding the 
> soft freeze. The last can wait 2.8 if you prefer, we need more work 
> to fully support u-boot anyhow. I would be happy to have review 
> though :) The model is very much like the SCU.
> 
> Thanks,
> 
> C.
> 
> [1] not supported yet in qemu because of the lack of a rom device 
>     memory region needed to boot from the flash module.
> 
> 

Re: [Qemu-devel] [PATCH 0/5] ast2400: some cleanups and a simple memory controller model

[Peter Maydell]

On 12 July 2016 at 17:20, Cédric Le Goater <clg@kaod.org> wrote:
> On 07/12/2016 04:19 PM, Peter Maydell wrote:
>> Hi; the release process is now at the point where I wouldn't really
>> want to put this into 2.7; I can be persuaded if you have a solid
>> justification, though.
>
> Here is my take to it  :
>
> * 1/5 hw/misc: fix typo in Aspeed SCU hw-strap2 property name
>
>   That's a minor fix
>
> * 2/5 ast2400: replace aspeed_smc_is_implemented()
>
>   That is not a fix but it removes some ugliness in the code, which
>   you had noticed, and which prevents us from fixing possible issues
>   nicely. So this is a preventive cleanup :)
>
> * 3/5 ast2400: pretend DMAs are done for U-boot
>
>   This is a work around fix. U-boot[1] uses DMAs to calibrate the
>   SPI flash module. It will spin until the DMA transfers are done.
>
> * 4/5 ast2400: externalize revision numbers
>
>   This is minor fix de-duplicating code and a prereq for the next
>   patch.
>
> * 5/5  ast2400: add a memory controller device model
>
>   That's an enhancement. It adds a basic device model for the memory
>   controller. This is needed by some version of the SDK relying on
>   the configuration register to know how much RAM the SOC has. Older
>   versions used a hardcoded value.
>
>
> The first patches should not be too much of a problem regarding the
> soft freeze. The last can wait 2.8 if you prefer, we need more work
> to fully support u-boot anyhow. I would be happy to have review
> though :) The model is very much like the SCU.

Thanks for the explanation. Patches 1-4 look straightforward
and I have put them in my target-arm.next tree. Since patch
5 isn't sufficient to get u-boot to work I think we should
defer it to 2.8. It is still on my to-review list.

-- PMM

Re: [Qemu-devel] [PATCH 5/5] ast2400: add a memory controller device model

[Peter Maydell]

On 8 July 2016 at 17:06, Cédric Le Goater <clg@kaod.org> wrote:
> The uboot in the previous release of the SDK was using a hardcoded
> value for memory size. This is not true anymore, the value is now
> retrieved from the memory controller.
>
> Below is a model for this device, only supporting unlock and
> configuration. Without it, we endup running a guest with 64MB, which
> is a bit low nowdays. It uses a 'silicon-rev' property and ram_size to
> build a default value. Some bits should be linked to SCU strapping
> registers but it seems a bit complex to add for the current need.
>
> The model is ready for the AST2500 SOC.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Just a few nits below.

> +/*
> + * Configuration register Ox4 (for Aspeed AST2400 SOC)
> + *
> + * These are for the record and future use. ASPEED_SDMC_DRAM_SIZE is
> + * what we care about right now as it is checked by U-Boot to
> + * determine the RAM size.
> + */
> +
> +#define ASPEED_SDMC_AST2300_COMPAT      (1 << 10)
> +#define ASPEED_SDMC_SCRAMBLE_PATTERN    (1 << 9)
> +#define ASPEED_SDMC_DATA_SCRAMBLE       (1 << 8)
> +#define ASPEED_SDMC_ECC_ENABLE          (1 << 7)
> +#define ASPEED_SDMC_VGA_COMPAT          (1 << 6) /* readonly */
> +#define ASPEED_SDMC_DRAM_BANK           (1 << 5)
> +#define ASPEED_SDMC_DRAM_BURST          (1 << 4)
> +#define ASPEED_SDMC_VGA_APERTURE(x)     ((x & 0x3) << 2) /* readonly */
> +#define     ASPEED_SDMC_VGA_8MB             0x0
> +#define     ASPEED_SDMC_VGA_16MB            0x1
> +#define     ASPEED_SDMC_VGA_32MB            0x2
> +#define     ASPEED_SDMC_VGA_64MB            0x3
> +#define ASPEED_SDMC_DRAM_SIZE(x)        (x & 0x3)
> +#define     ASPEED_SDMC_DRAM_64MB           0x0
> +#define     ASPEED_SDMC_DRAM_128MB          0x1
> +#define     ASPEED_SDMC_DRAM_256MB          0x2
> +#define     ASPEED_SDMC_DRAM_512MB          0x3
> +
> +/*
> + * Configuration register Ox4 (for Aspeed AST2500 SOC and higher)
> + *
> + * Incompatibilities are annoted in the list. ASPEED_SDMC_HW_VERSION

"noted" or "annotated".

> + * should be set to 1 for the AST2500 SOC.
> + */
> +#define ASPEED_SDMC_HW_VERSION(x)       ((x & 0xf) << 28) /* readonly */
> +#define ASPEED_SDMC_SW_VERSION          ((x & 0xff) << 20)
> +#define ASPEED_SDMC_CACHE_INITIAL_DONE  (1 << 19) /* readonly */
> +#define ASPEED_SDMC_CACHE_DDR4_CONF     (1 << 13)
> +#define ASPEED_SDMC_CACHE_INITIAL       (1 << 12)
> +#define ASPEED_SDMC_CACHE_RANGE_CTRL    (1 << 11)
> +#define ASPEED_SDMC_CACHE_ENABLE        (1 << 10) /* differs from AST2400 */
> +#define ASPEED_SDMC_DRAM_TYPE           (1 << 4)  /* differs from AST2400 */
> +
> +/* DRAM size definitions differs */
> +#define     ASPEED_SDMC_AST2500_128MB       0x0
> +#define     ASPEED_SDMC_AST2500_256MB       0x1
> +#define     ASPEED_SDMC_AST2500_512MB       0x2
> +#define     ASPEED_SDMC_AST2500_1024MB      0x3
> +

> +    if (addr == R_CONF) {
> +        /* Make sure readonly bits are kept */
> +        switch (s->silicon_rev) {
> +        case AST2400_A0_SILICON_REV:
> +            data &= 0x000007B3;
> +            break;
> +        case AST2500_A0_SILICON_REV:
> +            data &= 0x0FF03FB3;
> +            break;

Maybe these would be more readable using the symbolic constant
names you #defined above? (Or maybe not; your choice.)

> +        default:
> +            g_assert_not_reached();
> +        }
> +    }
> +
> +    s->regs[addr] = data;
> +}
> +
> +static const MemoryRegionOps aspeed_sdmc_ops = {
> +    .read = aspeed_sdmc_read,
> +    .write = aspeed_sdmc_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 4,
> +    .valid.unaligned = false,

.valid.unaligned = false is the default, you don't need to
specify it explicitly.

> +};
> +
> +static int ast2400_rambits(void)
> +{
> +    switch (ram_size >> 20) {
> +    case   64: return ASPEED_SDMC_DRAM_64MB;
> +    case  128: return ASPEED_SDMC_DRAM_128MB;
> +    case  256: return ASPEED_SDMC_DRAM_256MB;
> +    case  512: return ASPEED_SDMC_DRAM_512MB;

These should have newlines between the case line and the code.

> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
> +                      HWADDR_PRIx "\n", __func__, ram_size);
> +        break;
> +    }
> +
> +    /* set a minimum default */
> +    return ASPEED_SDMC_DRAM_64MB;
> +}
> +
> +static int ast2500_rambits(void)
> +{
> +    switch (ram_size >> 20) {
> +    case  128: return ASPEED_SDMC_AST2500_128MB;
> +    case  256: return ASPEED_SDMC_AST2500_256MB;
> +    case  512: return ASPEED_SDMC_AST2500_512MB;
> +    case 1024: return ASPEED_SDMC_AST2500_1024MB;

Ditto.

> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
> +                      HWADDR_PRIx "\n", __func__, ram_size);
> +        break;
> +    }
> +
> +    /* set a minimum default */
> +    return ASPEED_SDMC_AST2500_128MB;
> +}
> +
> +static void aspeed_sdmc_reset(DeviceState *dev)
> +{
> +    AspeedSDMCState *s = ASPEED_SDMC(dev);
> +
> +    memset(s->regs, 0, sizeof(s->regs));
> +
> +    /* Set ram size bit and defaults values */
> +    switch (s->silicon_rev) {
> +    case AST2400_A0_SILICON_REV:
> +        s->regs[R_CONF] |=
> +            ASPEED_SDMC_VGA_COMPAT |
> +            ASPEED_SDMC_DRAM_SIZE(ast2400_rambits());
> +        break;
> +
> +    case AST2500_A0_SILICON_REV:
> +        s->regs[R_CONF] |=
> +            ASPEED_SDMC_HW_VERSION(1) |
> +            ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
> +            ASPEED_SDMC_DRAM_SIZE(ast2500_rambits());

Missing "break;".

> +    default:
> +        g_assert_not_reached();
> +    }
> +}

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/5] ast2400: add a memory controller device model

[Cédric Le Goater]

On 07/25/2016 05:12 PM, Peter Maydell wrote:
> On 8 July 2016 at 17:06, Cédric Le Goater <clg@kaod.org> wrote:
>> The uboot in the previous release of the SDK was using a hardcoded
>> value for memory size. This is not true anymore, the value is now
>> retrieved from the memory controller.
>>
>> Below is a model for this device, only supporting unlock and
>> configuration. Without it, we endup running a guest with 64MB, which
>> is a bit low nowdays. It uses a 'silicon-rev' property and ram_size to
>> build a default value. Some bits should be linked to SCU strapping
>> registers but it seems a bit complex to add for the current need.
>>
>> The model is ready for the AST2500 SOC.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
> 
> Just a few nits below.
> 
>> +/*
>> + * Configuration register Ox4 (for Aspeed AST2400 SOC)
>> + *
>> + * These are for the record and future use. ASPEED_SDMC_DRAM_SIZE is
>> + * what we care about right now as it is checked by U-Boot to
>> + * determine the RAM size.
>> + */
>> +
>> +#define ASPEED_SDMC_AST2300_COMPAT      (1 << 10)
>> +#define ASPEED_SDMC_SCRAMBLE_PATTERN    (1 << 9)
>> +#define ASPEED_SDMC_DATA_SCRAMBLE       (1 << 8)
>> +#define ASPEED_SDMC_ECC_ENABLE          (1 << 7)
>> +#define ASPEED_SDMC_VGA_COMPAT          (1 << 6) /* readonly */
>> +#define ASPEED_SDMC_DRAM_BANK           (1 << 5)
>> +#define ASPEED_SDMC_DRAM_BURST          (1 << 4)
>> +#define ASPEED_SDMC_VGA_APERTURE(x)     ((x & 0x3) << 2) /* readonly */
>> +#define     ASPEED_SDMC_VGA_8MB             0x0
>> +#define     ASPEED_SDMC_VGA_16MB            0x1
>> +#define     ASPEED_SDMC_VGA_32MB            0x2
>> +#define     ASPEED_SDMC_VGA_64MB            0x3
>> +#define ASPEED_SDMC_DRAM_SIZE(x)        (x & 0x3)
>> +#define     ASPEED_SDMC_DRAM_64MB           0x0
>> +#define     ASPEED_SDMC_DRAM_128MB          0x1
>> +#define     ASPEED_SDMC_DRAM_256MB          0x2
>> +#define     ASPEED_SDMC_DRAM_512MB          0x3
>> +
>> +/*
>> + * Configuration register Ox4 (for Aspeed AST2500 SOC and higher)
>> + *
>> + * Incompatibilities are annoted in the list. ASPEED_SDMC_HW_VERSION
> 
> "noted" or "annotated".
> 
>> + * should be set to 1 for the AST2500 SOC.
>> + */
>> +#define ASPEED_SDMC_HW_VERSION(x)       ((x & 0xf) << 28) /* readonly */
>> +#define ASPEED_SDMC_SW_VERSION          ((x & 0xff) << 20)
>> +#define ASPEED_SDMC_CACHE_INITIAL_DONE  (1 << 19) /* readonly */
>> +#define ASPEED_SDMC_CACHE_DDR4_CONF     (1 << 13)
>> +#define ASPEED_SDMC_CACHE_INITIAL       (1 << 12)
>> +#define ASPEED_SDMC_CACHE_RANGE_CTRL    (1 << 11)
>> +#define ASPEED_SDMC_CACHE_ENABLE        (1 << 10) /* differs from AST2400 */
>> +#define ASPEED_SDMC_DRAM_TYPE           (1 << 4)  /* differs from AST2400 */
>> +
>> +/* DRAM size definitions differs */
>> +#define     ASPEED_SDMC_AST2500_128MB       0x0
>> +#define     ASPEED_SDMC_AST2500_256MB       0x1
>> +#define     ASPEED_SDMC_AST2500_512MB       0x2
>> +#define     ASPEED_SDMC_AST2500_1024MB      0x3
>> +
> 
>> +    if (addr == R_CONF) {
>> +        /* Make sure readonly bits are kept */
>> +        switch (s->silicon_rev) {
>> +        case AST2400_A0_SILICON_REV:
>> +            data &= 0x000007B3;
>> +            break;
>> +        case AST2500_A0_SILICON_REV:
>> +            data &= 0x0FF03FB3;
>> +            break;
> 
> Maybe these would be more readable using the symbolic constant
> names you #defined above? (Or maybe not; your choice.)

symbolic constants are better but I was lazy for these. I will see 
if I can build a mask from the above defines.

> 
>> +        default:
>> +            g_assert_not_reached();
>> +        }
>> +    }
>> +
>> +    s->regs[addr] = data;
>> +}
>> +
>> +static const MemoryRegionOps aspeed_sdmc_ops = {
>> +    .read = aspeed_sdmc_read,
>> +    .write = aspeed_sdmc_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .valid.min_access_size = 4,
>> +    .valid.max_access_size = 4,
>> +    .valid.unaligned = false,
> 
> .valid.unaligned = false is the default, you don't need to
> specify it explicitly.
> 
>> +};
>> +
>> +static int ast2400_rambits(void)
>> +{
>> +    switch (ram_size >> 20) {
>> +    case   64: return ASPEED_SDMC_DRAM_64MB;
>> +    case  128: return ASPEED_SDMC_DRAM_128MB;
>> +    case  256: return ASPEED_SDMC_DRAM_256MB;
>> +    case  512: return ASPEED_SDMC_DRAM_512MB;
> 
> These should have newlines between the case line and the code.

OK will do. 

>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
>> +                      HWADDR_PRIx "\n", __func__, ram_size);
>> +        break;
>> +    }
>> +
>> +    /* set a minimum default */
>> +    return ASPEED_SDMC_DRAM_64MB;
>> +}
>> +
>> +static int ast2500_rambits(void)
>> +{
>> +    switch (ram_size >> 20) {
>> +    case  128: return ASPEED_SDMC_AST2500_128MB;
>> +    case  256: return ASPEED_SDMC_AST2500_256MB;
>> +    case  512: return ASPEED_SDMC_AST2500_512MB;
>> +    case 1024: return ASPEED_SDMC_AST2500_1024MB;
> 
> Ditto.
> 
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
>> +                      HWADDR_PRIx "\n", __func__, ram_size);
>> +        break;
>> +    }
>> +
>> +    /* set a minimum default */
>> +    return ASPEED_SDMC_AST2500_128MB;
>> +}
>> +
>> +static void aspeed_sdmc_reset(DeviceState *dev)
>> +{
>> +    AspeedSDMCState *s = ASPEED_SDMC(dev);
>> +
>> +    memset(s->regs, 0, sizeof(s->regs));
>> +
>> +    /* Set ram size bit and defaults values */
>> +    switch (s->silicon_rev) {
>> +    case AST2400_A0_SILICON_REV:
>> +        s->regs[R_CONF] |=
>> +            ASPEED_SDMC_VGA_COMPAT |
>> +            ASPEED_SDMC_DRAM_SIZE(ast2400_rambits());
>> +        break;
>> +
>> +    case AST2500_A0_SILICON_REV:
>> +        s->regs[R_CONF] |=
>> +            ASPEED_SDMC_HW_VERSION(1) |
>> +            ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
>> +            ASPEED_SDMC_DRAM_SIZE(ast2500_rambits());
> 
> Missing "break;".

Indeed.

>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +}
> 
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks for the review, I will send an updated version.

C. 


> thanks
> -- PMM
>