[Qemu-devel] [PATCH 0/7] arm: aspeed: Extend SDRAM controller

[Qemu-devel] [PATCH 0/7] arm: aspeed: Extend SDRAM controller

[Joel Stanley]

This series allow us to run upstream u-boot on the ast2500-evb model.
They have been tested with upstream u-boot, as well as openbmc's ast2500
and ast2500 images.

Cédric Le Goater (1):
  aspeed: add a max_ram_size property to the memory controller

Joel Stanley (6):
  aspeed_sdmc: Extend number of valid registers
  aspeed_sdmc: Fix saved values
  aspeed_sdmc: Set 'cache initial sequence' always true
  aspeed_sdmc: Init status alwlays idle
  aspeed_sdmc: Handle ECC training
  aspeed: Link SCU to the watchdog

 hw/arm/aspeed.c                  | 31 ++++++++++++++++++
 hw/arm/aspeed_soc.c              |  4 +++
 hw/misc/aspeed_sdmc.c            | 55 +++++++++++++++++++++-----------
 hw/watchdog/wdt_aspeed.c         | 20 ++++++++++++
 include/hw/misc/aspeed_sdmc.h    |  4 ++-
 include/hw/watchdog/wdt_aspeed.h |  1 +
 6 files changed, 95 insertions(+), 20 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/7] aspeed_sdmc: Extend number of valid registers

[Joel Stanley]

The SDMC on the ast2500 has 170 registers.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 include/hw/misc/aspeed_sdmc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
index 551c8afdf4be..682f0f5d56dc 100644
--- a/include/hw/misc/aspeed_sdmc.h
+++ b/include/hw/misc/aspeed_sdmc.h
@@ -14,7 +14,7 @@
 #define TYPE_ASPEED_SDMC "aspeed.sdmc"
 #define ASPEED_SDMC(obj) OBJECT_CHECK(AspeedSDMCState, (obj), TYPE_ASPEED_SDMC)
 
-#define ASPEED_SDMC_NR_REGS (0x8 >> 2)
+#define ASPEED_SDMC_NR_REGS (0x174 >> 2)
 
 typedef struct AspeedSDMCState {
     /*< private >*/
-- 
2.17.1

[Qemu-devel] [PATCH 2/7] aspeed_sdmc: Fix saved values

[Joel Stanley]

This fixes the intended protection of read-only values in the
configuration register. They were being always set to zero by mistake.

The read-only fields depend on the configured memory size of the system,
so they cannot be fixed at compile time. The most straight forward
option was to store them in the state structure.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 hw/misc/aspeed_sdmc.c         | 27 ++++++++-------------------
 include/hw/misc/aspeed_sdmc.h |  1 +
 2 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 0df008e52a18..24fd4aee2d82 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -126,10 +126,12 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
         case AST2400_A0_SILICON_REV:
         case AST2400_A1_SILICON_REV:
             data &= ~ASPEED_SDMC_READONLY_MASK;
+            data |= s->fixed_conf;
             break;
         case AST2500_A0_SILICON_REV:
         case AST2500_A1_SILICON_REV:
             data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
+            data |= s->fixed_conf;
             break;
         default:
             g_assert_not_reached();
@@ -198,25 +200,7 @@ static void aspeed_sdmc_reset(DeviceState *dev)
     memset(s->regs, 0, sizeof(s->regs));
 
     /* Set ram size bit and defaults values */
-    switch (s->silicon_rev) {
-    case AST2400_A0_SILICON_REV:
-    case AST2400_A1_SILICON_REV:
-        s->regs[R_CONF] |=
-            ASPEED_SDMC_VGA_COMPAT |
-            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
-        break;
-
-    case AST2500_A0_SILICON_REV:
-    case AST2500_A1_SILICON_REV:
-        s->regs[R_CONF] |=
-            ASPEED_SDMC_HW_VERSION(1) |
-            ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
-            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
-        break;
-
-    default:
-        g_assert_not_reached();
-    }
+    s->regs[R_CONF] = s->fixed_conf;
 }
 
 static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
@@ -234,10 +218,15 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
     case AST2400_A0_SILICON_REV:
     case AST2400_A1_SILICON_REV:
         s->ram_bits = ast2400_rambits(s);
+        s->fixed_conf = ASPEED_SDMC_VGA_COMPAT |
+            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
         break;
     case AST2500_A0_SILICON_REV:
     case AST2500_A1_SILICON_REV:
         s->ram_bits = ast2500_rambits(s);
+        s->fixed_conf = ASPEED_SDMC_HW_VERSION(1) |
+            ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
+            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
         break;
     default:
         g_assert_not_reached();
diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
index 682f0f5d56dc..e079c66a7d73 100644
--- a/include/hw/misc/aspeed_sdmc.h
+++ b/include/hw/misc/aspeed_sdmc.h
@@ -27,6 +27,7 @@ typedef struct AspeedSDMCState {
     uint32_t silicon_rev;
     uint32_t ram_bits;
     uint64_t ram_size;
+    uint32_t fixed_conf;
 
 } AspeedSDMCState;
 
-- 
2.17.1

[Qemu-devel] [PATCH 3/7] aspeed_sdmc: Set 'cache initial sequence' always true

[Joel Stanley]

The SDRAM training routine sets the 'Enable cache initial' bit, and then
waits for the 'cache initial sequence' to be done.

Have it always return done, as there is no other side effects that the
model needs to implement. This allows the upstream u-boot training to
proceed on the ast2500-evb board.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 hw/misc/aspeed_sdmc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 24fd4aee2d82..9ece545c4ffa 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -226,6 +226,7 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
         s->ram_bits = ast2500_rambits(s);
         s->fixed_conf = ASPEED_SDMC_HW_VERSION(1) |
             ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
+            ASPEED_SDMC_CACHE_INITIAL_DONE |
             ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
         break;
     default:
-- 
2.17.1

[Qemu-devel] [PATCH 4/7] aspeed_sdmc: Init status alwlays idle

[Joel Stanley]

The ast2500 SDRAM training routine busy waits on the 'init cycle busy
state' bit in DDR PHY Control/Status register #1 (MCR60).

This ensures the bit always reads zero, and allows training to
complete with upstream u-boot on the ast2500-evb.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 hw/misc/aspeed_sdmc.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 9ece545c4ffa..522e01ef8c0d 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -23,6 +23,10 @@
 /* Configuration Register */
 #define R_CONF            (0x04 / 4)
 
+/* Control/Status Register #1 (ast2500) */
+#define R_STATUS1         (0x60 / 4)
+#define   PHY_BUSY_STATE      BIT(0)
+
 /*
  * Configuration register Ox4 (for Aspeed AST2400 SOC)
  *
@@ -137,6 +141,17 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
             g_assert_not_reached();
         }
     }
+    if (s->silicon_rev == AST2500_A0_SILICON_REV ||
+            s->silicon_rev == AST2500_A1_SILICON_REV) {
+        switch (addr) {
+        case R_STATUS1:
+            /* Will never return 'busy' */
+            data &= ~PHY_BUSY_STATE;
+            break;
+        default:
+            break;
+        }
+    }
 
     s->regs[addr] = data;
 }
-- 
2.17.1

[Qemu-devel] [PATCH 5/7] aspeed_sdmc: Handle ECC training

[Joel Stanley]

This is required to ensure u-boot SDRAM training completes.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 hw/misc/aspeed_sdmc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 522e01ef8c0d..89de3138aff0 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -27,6 +27,10 @@
 #define R_STATUS1         (0x60 / 4)
 #define   PHY_BUSY_STATE      BIT(0)
 
+#define R_ECC_TEST_CTRL   (0x70 / 4)
+#define   ECC_TEST_FINISHED   BIT(12)
+#define   ECC_TEST_FAIL       BIT(13)
+
 /*
  * Configuration register Ox4 (for Aspeed AST2400 SOC)
  *
@@ -148,6 +152,11 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
             /* Will never return 'busy' */
             data &= ~PHY_BUSY_STATE;
             break;
+        case R_ECC_TEST_CTRL:
+            /* Always done, always happy */
+            data |= ECC_TEST_FINISHED;
+            data &= ~ECC_TEST_FAIL;
+            break;
         default:
             break;
         }
-- 
2.17.1

[Qemu-devel] [PATCH 6/7] aspeed: add a max_ram_size property to the memory controller

[Joel Stanley]

From: Cédric Le Goater <clg@kaod.org>

This will be used to construct a memory region beyond the RAM region
to let firmwares scan the address space with load/store to guess how
much RAM the SoC has.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 hw/arm/aspeed.c               | 31 +++++++++++++++++++++++++++++++
 hw/arm/aspeed_soc.c           |  2 ++
 hw/misc/aspeed_sdmc.c         |  3 +++
 include/hw/misc/aspeed_sdmc.h |  1 +
 4 files changed, 37 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index bb9d33848d3f..e078269266bc 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -31,6 +31,7 @@ static struct arm_boot_info aspeed_board_binfo = {
 typedef struct AspeedBoardState {
     AspeedSoCState soc;
     MemoryRegion ram;
+    MemoryRegion max_ram;
 } AspeedBoardState;
 
 typedef struct AspeedBoardConfig {
@@ -127,6 +128,27 @@ static const AspeedBoardConfig aspeed_boards[] = {
     },
 };
 
+/*
+ * The max ram region is for firmwares that scan the address space
+ * with load/store to guess how much RAM the SoC has.
+ */
+static uint64_t max_ram_read(void *opaque, hwaddr offset, unsigned size)
+{
+    return 0;
+}
+
+static void max_ram_write(void *opaque, hwaddr offset, uint64_t value,
+                           unsigned size)
+{
+    /* Disacard writes */
+}
+
+static const MemoryRegionOps max_ram_ops = {
+    .read = max_ram_read,
+    .write = max_ram_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
 #define FIRMWARE_ADDR 0x0
 
 static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
@@ -187,6 +209,7 @@ static void aspeed_board_init(MachineState *machine,
     AspeedBoardState *bmc;
     AspeedSoCClass *sc;
     DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
+    ram_addr_t max_ram_size;
 
     bmc = g_new0(AspeedBoardState, 1);
     object_initialize(&bmc->soc, (sizeof(bmc->soc)), cfg->soc_name);
@@ -226,6 +249,14 @@ static void aspeed_board_init(MachineState *machine,
     object_property_add_const_link(OBJECT(&bmc->soc), "ram", OBJECT(&bmc->ram),
                                    &error_abort);
 
+    max_ram_size = object_property_get_uint(OBJECT(&bmc->soc), "max-ram-size",
+                                            &error_abort);
+    memory_region_init_io(&bmc->max_ram, NULL, &max_ram_ops, NULL,
+                          "max_ram", max_ram_size  - ram_size);
+    memory_region_add_subregion(get_system_memory(),
+                                sc->info->sdram_base + ram_size,
+                                &bmc->max_ram);
+
     aspeed_board_init_flashes(&bmc->soc.fmc, cfg->fmc_model, &error_abort);
     aspeed_board_init_flashes(&bmc->soc.spi[0], cfg->spi_model, &error_abort);
 
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index e68911af0f90..a27233d4876b 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -155,6 +155,8 @@ static void aspeed_soc_init(Object *obj)
                          sc->info->silicon_rev);
     object_property_add_alias(obj, "ram-size", OBJECT(&s->sdmc),
                               "ram-size", &error_abort);
+    object_property_add_alias(obj, "max-ram-size", OBJECT(&s->sdmc),
+                              "max-ram-size", &error_abort);
 
     for (i = 0; i < sc->info->wdts_num; i++) {
         object_initialize(&s->wdt[i], sizeof(s->wdt[i]), TYPE_ASPEED_WDT);
diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 89de3138aff0..eec77f243508 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -242,12 +242,14 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
     case AST2400_A0_SILICON_REV:
     case AST2400_A1_SILICON_REV:
         s->ram_bits = ast2400_rambits(s);
+        s->max_ram_size = 512 << 20;
         s->fixed_conf = ASPEED_SDMC_VGA_COMPAT |
             ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
         break;
     case AST2500_A0_SILICON_REV:
     case AST2500_A1_SILICON_REV:
         s->ram_bits = ast2500_rambits(s);
+        s->max_ram_size = 1024 << 20;
         s->fixed_conf = ASPEED_SDMC_HW_VERSION(1) |
             ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
             ASPEED_SDMC_CACHE_INITIAL_DONE |
@@ -275,6 +277,7 @@ static const VMStateDescription vmstate_aspeed_sdmc = {
 static Property aspeed_sdmc_properties[] = {
     DEFINE_PROP_UINT32("silicon-rev", AspeedSDMCState, silicon_rev, 0),
     DEFINE_PROP_UINT64("ram-size", AspeedSDMCState, ram_size, 0),
+    DEFINE_PROP_UINT64("max-ram-size", AspeedSDMCState, max_ram_size, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
index e079c66a7d73..b3c926acae90 100644
--- a/include/hw/misc/aspeed_sdmc.h
+++ b/include/hw/misc/aspeed_sdmc.h
@@ -27,6 +27,7 @@ typedef struct AspeedSDMCState {
     uint32_t silicon_rev;
     uint32_t ram_bits;
     uint64_t ram_size;
+    uint64_t max_ram_size;
     uint32_t fixed_conf;
 
 } AspeedSDMCState;
-- 
2.17.1

[Qemu-devel] [PATCH 7/7] aspeed: Link SCU to the watchdog

[Joel Stanley]

The ast2500 uses the watchdog to reset the SDRAM controller. This
operation is usually performed by u-boot's memory training procedure,
and it is enabled by setting a bit in the SCU and then causing the
watchdog to expire. Therefore, we need the watchdog to be able to
acess the SCU's register space.

This does not cause a system reset, but only resets the SDRAM
controller.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 hw/arm/aspeed_soc.c              |  2 ++
 hw/watchdog/wdt_aspeed.c         | 20 ++++++++++++++++++++
 include/hw/watchdog/wdt_aspeed.h |  1 +
 3 files changed, 23 insertions(+)

diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index a27233d4876b..2cbacb4430bb 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -164,6 +164,8 @@ static void aspeed_soc_init(Object *obj)
         qdev_set_parent_bus(DEVICE(&s->wdt[i]), sysbus_get_default());
         qdev_prop_set_uint32(DEVICE(&s->wdt[i]), "silicon-rev",
                                     sc->info->silicon_rev);
+        object_property_add_const_link(OBJECT(&s->wdt[i]), "scu",
+                                       OBJECT(&s->scu), &error_abort);
     }
 
     object_initialize(&s->ftgmac100, sizeof(s->ftgmac100), TYPE_FTGMAC100);
diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index 95f6ad186d72..f5cb30c0b584 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -43,6 +43,9 @@
 
 #define WDT_RESTART_MAGIC               0x4755
 
+#define SCU_RESET_CONTROL1              (0x04 / 4)
+#define    SCU_RESET_SDRAM              BIT(0)
+
 static bool aspeed_wdt_is_enabled(const AspeedWDTState *s)
 {
     return s->regs[WDT_CTRL] & WDT_CTRL_ENABLE;
@@ -221,6 +224,13 @@ static void aspeed_wdt_timer_expired(void *dev)
 {
     AspeedWDTState *s = ASPEED_WDT(dev);
 
+    /* Do not reset on SDRAM controller reset */
+    if (s->scu->regs[SCU_RESET_CONTROL1] & SCU_RESET_SDRAM) {
+        timer_del(s->timer);
+        s->regs[WDT_CTRL] = 0;
+        return;
+    }
+
     qemu_log_mask(CPU_LOG_RESET, "Watchdog timer expired.\n");
     watchdog_perform_action();
     timer_del(s->timer);
@@ -232,6 +242,16 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     AspeedWDTState *s = ASPEED_WDT(dev);
+    Error *err = NULL;
+    Object *obj;
+
+    obj = object_property_get_link(OBJECT(dev), "scu", &err);
+    if (!obj) {
+        error_propagate(errp, err);
+        error_prepend(errp, "required link 'scu' not found: ");
+        return;
+    }
+    s->scu = ASPEED_SCU(obj);
 
     if (!is_supported_silicon_rev(s->silicon_rev)) {
         error_setg(errp, "Unknown silicon revision: 0x%" PRIx32,
diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
index 7de3e5c224fb..cc734bd7ae7b 100644
--- a/include/hw/watchdog/wdt_aspeed.h
+++ b/include/hw/watchdog/wdt_aspeed.h
@@ -26,6 +26,7 @@ typedef struct AspeedWDTState {
     MemoryRegion iomem;
     uint32_t regs[ASPEED_WDT_REGS_MAX];
 
+    AspeedSCUState *scu;
     uint32_t pclk_freq;
     uint32_t silicon_rev;
     uint32_t ext_pulse_width_mask;
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/7] aspeed_sdmc: Extend number of valid registers

[Cédric Le Goater]

On 08/07/2018 09:57 AM, Joel Stanley wrote:
> The SDMC on the ast2500 has 170 registers.


Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  include/hw/misc/aspeed_sdmc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
> index 551c8afdf4be..682f0f5d56dc 100644
> --- a/include/hw/misc/aspeed_sdmc.h
> +++ b/include/hw/misc/aspeed_sdmc.h
> @@ -14,7 +14,7 @@
>  #define TYPE_ASPEED_SDMC "aspeed.sdmc"
>  #define ASPEED_SDMC(obj) OBJECT_CHECK(AspeedSDMCState, (obj), TYPE_ASPEED_SDMC)
>  
> -#define ASPEED_SDMC_NR_REGS (0x8 >> 2)
> +#define ASPEED_SDMC_NR_REGS (0x174 >> 2)
>  
>  typedef struct AspeedSDMCState {
>      /*< private >*/
> 

Re: [Qemu-devel] [PATCH 2/7] aspeed_sdmc: Fix saved values

[Cédric Le Goater]

On 08/07/2018 09:57 AM, Joel Stanley wrote:
> This fixes the intended protection of read-only values in the
> configuration register. They were being always set to zero by mistake.

yes :/

> The read-only fields depend on the configured memory size of the system,
> so they cannot be fixed at compile time. The most straight forward
> option was to store them in the state structure.

We could also use an array of init values for registers, like SCU does, but
this is fine for now.  

> Signed-off-by: Joel Stanley <joel@jms.id.au>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/misc/aspeed_sdmc.c         | 27 ++++++++-------------------
>  include/hw/misc/aspeed_sdmc.h |  1 +
>  2 files changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 0df008e52a18..24fd4aee2d82 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -126,10 +126,12 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
>          case AST2400_A0_SILICON_REV:
>          case AST2400_A1_SILICON_REV:
>              data &= ~ASPEED_SDMC_READONLY_MASK;
> +            data |= s->fixed_conf;
>              break;
>          case AST2500_A0_SILICON_REV:
>          case AST2500_A1_SILICON_REV:
>              data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
> +            data |= s->fixed_conf;
>              break;
>          default:
>              g_assert_not_reached();
> @@ -198,25 +200,7 @@ static void aspeed_sdmc_reset(DeviceState *dev)
>      memset(s->regs, 0, sizeof(s->regs));
>  
>      /* Set ram size bit and defaults values */
> -    switch (s->silicon_rev) {
> -    case AST2400_A0_SILICON_REV:
> -    case AST2400_A1_SILICON_REV:
> -        s->regs[R_CONF] |=
> -            ASPEED_SDMC_VGA_COMPAT |
> -            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
> -        break;
> -
> -    case AST2500_A0_SILICON_REV:
> -    case AST2500_A1_SILICON_REV:
> -        s->regs[R_CONF] |=
> -            ASPEED_SDMC_HW_VERSION(1) |
> -            ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
> -            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
> -        break;
> -
> -    default:
> -        g_assert_not_reached();
> -    }
> +    s->regs[R_CONF] = s->fixed_conf;
>  }
>  
>  static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
> @@ -234,10 +218,15 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
>      case AST2400_A0_SILICON_REV:
>      case AST2400_A1_SILICON_REV:
>          s->ram_bits = ast2400_rambits(s);
> +        s->fixed_conf = ASPEED_SDMC_VGA_COMPAT |
> +            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
>          break;
>      case AST2500_A0_SILICON_REV:
>      case AST2500_A1_SILICON_REV:
>          s->ram_bits = ast2500_rambits(s);
> +        s->fixed_conf = ASPEED_SDMC_HW_VERSION(1) |
> +            ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
> +            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
>          break;
>      default:
>          g_assert_not_reached();
> diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
> index 682f0f5d56dc..e079c66a7d73 100644
> --- a/include/hw/misc/aspeed_sdmc.h
> +++ b/include/hw/misc/aspeed_sdmc.h
> @@ -27,6 +27,7 @@ typedef struct AspeedSDMCState {
>      uint32_t silicon_rev;
>      uint32_t ram_bits;
>      uint64_t ram_size;
> +    uint32_t fixed_conf;
>  
>  } AspeedSDMCState;
>  
> 

Re: [Qemu-devel] [PATCH 3/7] aspeed_sdmc: Set 'cache initial sequence' always true

[Cédric Le Goater]

On 08/07/2018 09:57 AM, Joel Stanley wrote:
> The SDRAM training routine sets the 'Enable cache initial' bit, and then
> waits for the 'cache initial sequence' to be done.
> 
> Have it always return done, as there is no other side effects that the
> model needs to implement. This allows the upstream u-boot training to
> proceed on the ast2500-evb board.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>



Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/misc/aspeed_sdmc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 24fd4aee2d82..9ece545c4ffa 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -226,6 +226,7 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
>          s->ram_bits = ast2500_rambits(s);
>          s->fixed_conf = ASPEED_SDMC_HW_VERSION(1) |
>              ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
> +            ASPEED_SDMC_CACHE_INITIAL_DONE |
>              ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
>          break;
>      default:
> 

Re: [Qemu-devel] [PATCH 4/7] aspeed_sdmc: Init status alwlays idle

[Cédric Le Goater]

On 08/07/2018 09:57 AM, Joel Stanley wrote:
> The ast2500 SDRAM training routine busy waits on the 'init cycle busy
> state' bit in DDR PHY Control/Status register #1 (MCR60).
> 
> This ensures the bit always reads zero, and allows training to
> complete with upstream u-boot on the ast2500-evb.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>



Reviewed-by: Cédric Le Goater <clg@kaod.org>

One comment below, no need to resend for that.

> ---
>  hw/misc/aspeed_sdmc.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 9ece545c4ffa..522e01ef8c0d 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -23,6 +23,10 @@
>  /* Configuration Register */
>  #define R_CONF            (0x04 / 4)
>  
> +/* Control/Status Register #1 (ast2500) */
> +#define R_STATUS1         (0x60 / 4)
> +#define   PHY_BUSY_STATE      BIT(0)
> +
>  /*
>   * Configuration register Ox4 (for Aspeed AST2400 SOC)
>   *
> @@ -137,6 +141,17 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
>              g_assert_not_reached();
>          }
>      }
> +    if (s->silicon_rev == AST2500_A0_SILICON_REV ||
> +            s->silicon_rev == AST2500_A1_SILICON_REV) {

Maybe use the ASPEED_IS_AST2500() ? 

It would be nice to have a set of macros we could use in the different
models testing the SoC silicon revision. How about :

#define ASPEED_AST2400 0x2
#define ASPEED_AST2500 0x4
#define ASPEED_AST2600 0x6

#define ASPEED_REVISION_MAJOR(si_rev)     (((si_rev) >> 24) & 0xff)

#define ASPEED_IS_AST2400(si_rev)  (ASPEED_REVISION_MAJOR(si_rev) == ASPEED_AST2400)
#define ASPEED_IS_AST2500(si_rev)  (ASPEED_REVISION_MAJOR(si_rev) == ASPEED_AST2500)
#define ASPEED_IS_AST2600(si_rev)  (ASPEED_REVISION_MAJOR(si_rev) == ASPEED_AST2600)


Thanks,

C.


> +        switch (addr) {
> +        case R_STATUS1:
> +            /* Will never return 'busy' */
> +            data &= ~PHY_BUSY_STATE;
> +            break;
> +        default:
> +            break;
> +        }
> +    }
>  
>      s->regs[addr] = data;
>  }
> 

Re: [Qemu-devel] [PATCH 5/7] aspeed_sdmc: Handle ECC training

[Cédric Le Goater]

On 08/07/2018 09:57 AM, Joel Stanley wrote:
> This is required to ensure u-boot SDRAM training completes.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>



Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/misc/aspeed_sdmc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 522e01ef8c0d..89de3138aff0 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -27,6 +27,10 @@
>  #define R_STATUS1         (0x60 / 4)
>  #define   PHY_BUSY_STATE      BIT(0)
>  
> +#define R_ECC_TEST_CTRL   (0x70 / 4)
> +#define   ECC_TEST_FINISHED   BIT(12)
> +#define   ECC_TEST_FAIL       BIT(13)
> +
>  /*
>   * Configuration register Ox4 (for Aspeed AST2400 SOC)
>   *
> @@ -148,6 +152,11 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
>              /* Will never return 'busy' */
>              data &= ~PHY_BUSY_STATE;
>              break;
> +        case R_ECC_TEST_CTRL:
> +            /* Always done, always happy */
> +            data |= ECC_TEST_FINISHED;
> +            data &= ~ECC_TEST_FAIL;
> +            break;
>          default:
>              break;
>          }
> 

Re: [Qemu-devel] [PATCH 6/7] aspeed: add a max_ram_size property to the memory controller

[Cédric Le Goater]

On 08/07/2018 09:57 AM, Joel Stanley wrote:
> From: Cédric Le Goater <clg@kaod.org>
> 
> This will be used to construct a memory region beyond the RAM region
> to let firmwares scan the address space with load/store to guess how
> much RAM the SoC has.

This is in another patch I can send later on.

Thanks,

C. 

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  hw/arm/aspeed.c               | 31 +++++++++++++++++++++++++++++++
>  hw/arm/aspeed_soc.c           |  2 ++
>  hw/misc/aspeed_sdmc.c         |  3 +++
>  include/hw/misc/aspeed_sdmc.h |  1 +
>  4 files changed, 37 insertions(+)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index bb9d33848d3f..e078269266bc 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -31,6 +31,7 @@ static struct arm_boot_info aspeed_board_binfo = {
>  typedef struct AspeedBoardState {
>      AspeedSoCState soc;
>      MemoryRegion ram;
> +    MemoryRegion max_ram;
>  } AspeedBoardState;
>  
>  typedef struct AspeedBoardConfig {
> @@ -127,6 +128,27 @@ static const AspeedBoardConfig aspeed_boards[] = {
>      },
>  };
>  
> +/*
> + * The max ram region is for firmwares that scan the address space
> + * with load/store to guess how much RAM the SoC has.
> + */
> +static uint64_t max_ram_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    return 0;
> +}
> +
> +static void max_ram_write(void *opaque, hwaddr offset, uint64_t value,
> +                           unsigned size)
> +{
> +    /* Disacard writes */
> +}
> +
> +static const MemoryRegionOps max_ram_ops = {
> +    .read = max_ram_read,
> +    .write = max_ram_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
>  #define FIRMWARE_ADDR 0x0
>  
>  static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
> @@ -187,6 +209,7 @@ static void aspeed_board_init(MachineState *machine,
>      AspeedBoardState *bmc;
>      AspeedSoCClass *sc;
>      DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
> +    ram_addr_t max_ram_size;
>  
>      bmc = g_new0(AspeedBoardState, 1);
>      object_initialize(&bmc->soc, (sizeof(bmc->soc)), cfg->soc_name);
> @@ -226,6 +249,14 @@ static void aspeed_board_init(MachineState *machine,
>      object_property_add_const_link(OBJECT(&bmc->soc), "ram", OBJECT(&bmc->ram),
>                                     &error_abort);
>  
> +    max_ram_size = object_property_get_uint(OBJECT(&bmc->soc), "max-ram-size",
> +                                            &error_abort);
> +    memory_region_init_io(&bmc->max_ram, NULL, &max_ram_ops, NULL,
> +                          "max_ram", max_ram_size  - ram_size);
> +    memory_region_add_subregion(get_system_memory(),
> +                                sc->info->sdram_base + ram_size,
> +                                &bmc->max_ram);
> +
>      aspeed_board_init_flashes(&bmc->soc.fmc, cfg->fmc_model, &error_abort);
>      aspeed_board_init_flashes(&bmc->soc.spi[0], cfg->spi_model, &error_abort);
>  
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index e68911af0f90..a27233d4876b 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -155,6 +155,8 @@ static void aspeed_soc_init(Object *obj)
>                           sc->info->silicon_rev);
>      object_property_add_alias(obj, "ram-size", OBJECT(&s->sdmc),
>                                "ram-size", &error_abort);
> +    object_property_add_alias(obj, "max-ram-size", OBJECT(&s->sdmc),
> +                              "max-ram-size", &error_abort);
>  
>      for (i = 0; i < sc->info->wdts_num; i++) {
>          object_initialize(&s->wdt[i], sizeof(s->wdt[i]), TYPE_ASPEED_WDT);
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 89de3138aff0..eec77f243508 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -242,12 +242,14 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
>      case AST2400_A0_SILICON_REV:
>      case AST2400_A1_SILICON_REV:
>          s->ram_bits = ast2400_rambits(s);
> +        s->max_ram_size = 512 << 20;
>          s->fixed_conf = ASPEED_SDMC_VGA_COMPAT |
>              ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
>          break;
>      case AST2500_A0_SILICON_REV:
>      case AST2500_A1_SILICON_REV:
>          s->ram_bits = ast2500_rambits(s);
> +        s->max_ram_size = 1024 << 20;
>          s->fixed_conf = ASPEED_SDMC_HW_VERSION(1) |
>              ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
>              ASPEED_SDMC_CACHE_INITIAL_DONE |
> @@ -275,6 +277,7 @@ static const VMStateDescription vmstate_aspeed_sdmc = {
>  static Property aspeed_sdmc_properties[] = {
>      DEFINE_PROP_UINT32("silicon-rev", AspeedSDMCState, silicon_rev, 0),
>      DEFINE_PROP_UINT64("ram-size", AspeedSDMCState, ram_size, 0),
> +    DEFINE_PROP_UINT64("max-ram-size", AspeedSDMCState, max_ram_size, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
> index e079c66a7d73..b3c926acae90 100644
> --- a/include/hw/misc/aspeed_sdmc.h
> +++ b/include/hw/misc/aspeed_sdmc.h
> @@ -27,6 +27,7 @@ typedef struct AspeedSDMCState {
>      uint32_t silicon_rev;
>      uint32_t ram_bits;
>      uint64_t ram_size;
> +    uint64_t max_ram_size;
>      uint32_t fixed_conf;
>  
>  } AspeedSDMCState;
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 6/7] aspeed: add a max_ram_size property to the memory controller

[Cédric Le Goater]

On 08/07/2018 12:29 PM, Cédric Le Goater wrote:
> On 08/07/2018 09:57 AM, Joel Stanley wrote:
>> From: Cédric Le Goater <clg@kaod.org>
>>
>> This will be used to construct a memory region beyond the RAM region
>> to let firmwares scan the address space with load/store to guess how
>> much RAM the SoC has.
> 
> This is in another patch I can send later on.

I now see that you have merged two patches. This is fine then.


Thanks,

C.

Re: [Qemu-devel] [PATCH 7/7] aspeed: Link SCU to the watchdog

[Cédric Le Goater]

On 08/07/2018 09:57 AM, Joel Stanley wrote:
> The ast2500 uses the watchdog to reset the SDRAM controller. This
> operation is usually performed by u-boot's memory training procedure,
> and it is enabled by setting a bit in the SCU and then causing the
> watchdog to expire. Therefore, we need the watchdog to be able to
> acess the SCU's register space.

... access ...

> This does not cause a system reset, but only resets the SDRAM
> controller.

Well, it does nothing below :) 

I think we should call device_reset() on all the devices that have their 
reset bit enabled in the SCU System Reset Control Register and in the  
Reset Mask Register of the watchdog being used.


Watchdog Reset Mask Register bits :

31:26 Reserved
   25 Enable reset Misc. SOC controller (WDT, RTC, Timer, UART, SRAM)
   24 Enable reset SPI controller
   23 Enable reset X-DMA controller
   22 Enable reset MCTP controller
   21 Enable reset GPIO controller
   20 Enable reset ADC controller
   19 Enable reset JTAG master controller
   18 Enable reset PECI controller
   17 Enable reset PWM controller
   16 Enable reset CRT mode 2D engine
   15 Enable reset MIC controller
   14 Enable reset SD/SDIO controller
   13 Enable reset LPC controller
   12 Enable reset HAC engine
   11 Enable reset Video engine
   10 Enable reset USB1.1 HID/USB2.0 Host EHCI2 controller
    9 Enable reset USB1.1 Host controller
    8 Enable reset USB2.0 Host/Hub controller
    7 Enable reset Graphics CRT controller
    6 Enable reset MAC#2 controller
    5 Enable reset MAC#1 controller
    4 Enable reset I2C controller
    3 Enable reset AHB bridges
    2 Enable reset SDRAM controller
    1 Enable reset Coprocessor
    0 Enable reset ARM

Default value is 0x23FFFF3 which means reset all devices a part from :

   SPI controller
   X-DMA controller
   MCTP controller
   AHB bridges
   SDRAM controller

SCU System Reset Control Register ( has a different layout :/)
  
   25 Reset X-DMA controller
   24 Reset MCTP controller
   23 Reset ADC controller
   22 Reset JTAG Master controller
   21 Reserved
   20 PEWAKE# pin output enable control
   19 PEWAKE# pin output value
   18 Reset MIC controller
   17 Reserved, must keep at value ”1”
   16 Reset SD/SDIO card controller
   15 Reset USB1.1 Host controller
   14 Reset USB2.0 Hub/Host controller
   13 Reset CRT controller
   12 Reset MAC#2 controller
   11 Reset MAC#1 controller
   10 Reset PECI controller
    9 Reset PWM controller
    8 Disable PCI bus controller and VGA controller
    7 Reset 2D engine
    6 Reset Video engine
    5 Reset LPC controller
    4 Reset HAC engine
    3 Reset USB1.1 HID controller
    2 Reset I2C/SMBus controller
    1 Reset AHB bridges
    0 Enable reset SDRAM controller when full chip Watchdog reset occur

Default value is 0xFFCFFEDC which means reset all devices a part from :

    5 Reset LPC controller
    1 Reset AHB bridges
    0 Enable reset SDRAM controller


> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  hw/arm/aspeed_soc.c              |  2 ++
>  hw/watchdog/wdt_aspeed.c         | 20 ++++++++++++++++++++
>  include/hw/watchdog/wdt_aspeed.h |  1 +
>  3 files changed, 23 insertions(+)
> 
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index a27233d4876b..2cbacb4430bb 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -164,6 +164,8 @@ static void aspeed_soc_init(Object *obj)
>          qdev_set_parent_bus(DEVICE(&s->wdt[i]), sysbus_get_default());
>          qdev_prop_set_uint32(DEVICE(&s->wdt[i]), "silicon-rev",
>                                      sc->info->silicon_rev);
> +        object_property_add_const_link(OBJECT(&s->wdt[i]), "scu",
> +                                       OBJECT(&s->scu), &error_abort);
>      }
>  
>      object_initialize(&s->ftgmac100, sizeof(s->ftgmac100), TYPE_FTGMAC100);
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index 95f6ad186d72..f5cb30c0b584 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -43,6 +43,9 @@
>  
>  #define WDT_RESTART_MAGIC               0x4755
>  
> +#define SCU_RESET_CONTROL1              (0x04 / 4)

Too bad that the Aspeed SCU registers are already defined in the .c file.  

> +#define    SCU_RESET_SDRAM              BIT(0)
> +
>  static bool aspeed_wdt_is_enabled(const AspeedWDTState *s)
>  {
>      return s->regs[WDT_CTRL] & WDT_CTRL_ENABLE;
> @@ -221,6 +224,13 @@ static void aspeed_wdt_timer_expired(void *dev)
>  {
>      AspeedWDTState *s = ASPEED_WDT(dev);
>  
> +    /* Do not reset on SDRAM controller reset */
> +    if (s->scu->regs[SCU_RESET_CONTROL1] & SCU_RESET_SDRAM) {
> +        timer_del(s->timer);
> +        s->regs[WDT_CTRL] = 0;
> +        return;
> +    }
> +
>      qemu_log_mask(CPU_LOG_RESET, "Watchdog timer expired.\n");
>      watchdog_perform_action();
>      timer_del(s->timer);
> @@ -232,6 +242,16 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
>  {
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>      AspeedWDTState *s = ASPEED_WDT(dev);
> +    Error *err = NULL;
> +    Object *obj;
> +
> +    obj = object_property_get_link(OBJECT(dev), "scu", &err);
> +    if (!obj) {
> +        error_propagate(errp, err);
> +        error_prepend(errp, "required link 'scu' not found: ");
> +        return;
> +    }
> +    s->scu = ASPEED_SCU(obj);
>  
>      if (!is_supported_silicon_rev(s->silicon_rev)) {
>          error_setg(errp, "Unknown silicon revision: 0x%" PRIx32,
> diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
> index 7de3e5c224fb..cc734bd7ae7b 100644
> --- a/include/hw/watchdog/wdt_aspeed.h
> +++ b/include/hw/watchdog/wdt_aspeed.h
> @@ -26,6 +26,7 @@ typedef struct AspeedWDTState {
>      MemoryRegion iomem;
>      uint32_t regs[ASPEED_WDT_REGS_MAX];
>  
> +    AspeedSCUState *scu;
>      uint32_t pclk_freq;
>      uint32_t silicon_rev;
>      uint32_t ext_pulse_width_mask;
> 

Re: [Qemu-devel] [PATCH 0/7] arm: aspeed: Extend SDRAM controller

[Cédric Le Goater]

On 08/07/2018 09:57 AM, Joel Stanley wrote:
> This series allow us to run upstream u-boot on the ast2500-evb model.
> They have been tested with upstream u-boot, as well as openbmc's ast2500
> and ast2500 images.

For all patches,

Tested-by: Cédric Le Goater <clg@kaod.org>

Patch [1-6] are good to merge. 

I think patch 7 is a good start. It enables support for mainline 
U-Boot that we can use to develop more U-Boot drivers (flash). 
But the device reset model needs a little more work to take into 
account the Reset Mask Register of the watchdog. 

We don't have to support all of them, only the SoC and SDRAM to 
start with.

Thanks,

C.

> 
> Cédric Le Goater (1):
>   aspeed: add a max_ram_size property to the memory controller
> 
> Joel Stanley (6):
>   aspeed_sdmc: Extend number of valid registers
>   aspeed_sdmc: Fix saved values
>   aspeed_sdmc: Set 'cache initial sequence' always true
>   aspeed_sdmc: Init status alwlays idle
>   aspeed_sdmc: Handle ECC training
>   aspeed: Link SCU to the watchdog
> 
>  hw/arm/aspeed.c                  | 31 ++++++++++++++++++
>  hw/arm/aspeed_soc.c              |  4 +++
>  hw/misc/aspeed_sdmc.c            | 55 +++++++++++++++++++++-----------
>  hw/watchdog/wdt_aspeed.c         | 20 ++++++++++++
>  include/hw/misc/aspeed_sdmc.h    |  4 ++-
>  include/hw/watchdog/wdt_aspeed.h |  1 +
>  6 files changed, 95 insertions(+), 20 deletions(-)
> 

Re: [Qemu-devel] [PATCH 6/7] aspeed: add a max_ram_size property to the memory controller

[Peter Maydell]

On 7 August 2018 at 08:57, Joel Stanley <joel@jms.id.au> wrote:
> From: Cédric Le Goater <clg@kaod.org>
>
> This will be used to construct a memory region beyond the RAM region
> to let firmwares scan the address space with load/store to guess how
> much RAM the SoC has.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  hw/arm/aspeed.c               | 31 +++++++++++++++++++++++++++++++
>  hw/arm/aspeed_soc.c           |  2 ++
>  hw/misc/aspeed_sdmc.c         |  3 +++
>  include/hw/misc/aspeed_sdmc.h |  1 +
>  4 files changed, 37 insertions(+)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index bb9d33848d3f..e078269266bc 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -31,6 +31,7 @@ static struct arm_boot_info aspeed_board_binfo = {
>  typedef struct AspeedBoardState {
>      AspeedSoCState soc;
>      MemoryRegion ram;
> +    MemoryRegion max_ram;
>  } AspeedBoardState;
>
>  typedef struct AspeedBoardConfig {
> @@ -127,6 +128,27 @@ static const AspeedBoardConfig aspeed_boards[] = {
>      },
>  };
>
> +/*
> + * The max ram region is for firmwares that scan the address space
> + * with load/store to guess how much RAM the SoC has.
> + */
> +static uint64_t max_ram_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    return 0;
> +}
> +
> +static void max_ram_write(void *opaque, hwaddr offset, uint64_t value,
> +                           unsigned size)
> +{
> +    /* Disacard writes */
> +}
> +
> +static const MemoryRegionOps max_ram_ops = {
> +    .read = max_ram_read,
> +    .write = max_ram_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
>  #define FIRMWARE_ADDR 0x0
>
>  static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
> @@ -187,6 +209,7 @@ static void aspeed_board_init(MachineState *machine,
>      AspeedBoardState *bmc;
>      AspeedSoCClass *sc;
>      DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
> +    ram_addr_t max_ram_size;
>
>      bmc = g_new0(AspeedBoardState, 1);
>      object_initialize(&bmc->soc, (sizeof(bmc->soc)), cfg->soc_name);
> @@ -226,6 +249,14 @@ static void aspeed_board_init(MachineState *machine,
>      object_property_add_const_link(OBJECT(&bmc->soc), "ram", OBJECT(&bmc->ram),
>                                     &error_abort);
>
> +    max_ram_size = object_property_get_uint(OBJECT(&bmc->soc), "max-ram-size",
> +                                            &error_abort);
> +    memory_region_init_io(&bmc->max_ram, NULL, &max_ram_ops, NULL,
> +                          "max_ram", max_ram_size  - ram_size);
> +    memory_region_add_subregion(get_system_memory(),
> +                                sc->info->sdram_base + ram_size,
> +                                &bmc->max_ram);

I'm surprised that you need the IO ops, ie that it doesn't work
just to define an empty container region with memory_region_init().

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/7] arm: aspeed: Extend SDRAM controller

[Peter Maydell]

On 7 August 2018 at 13:30, Cédric Le Goater <clg@kaod.org> wrote:
> On 08/07/2018 09:57 AM, Joel Stanley wrote:
>> This series allow us to run upstream u-boot on the ast2500-evb model.
>> They have been tested with upstream u-boot, as well as openbmc's ast2500
>> and ast2500 images.
>
> For all patches,
>
> Tested-by: Cédric Le Goater <clg@kaod.org>
>
> Patch [1-6] are good to merge.

I've applied 1-6 to target-arm.next.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 6/7] aspeed: add a max_ram_size property to the memory controller

[Cédric Le Goater]

On 08/16/2018 02:48 PM, Peter Maydell wrote:
> On 7 August 2018 at 08:57, Joel Stanley <joel@jms.id.au> wrote:
>> From: Cédric Le Goater <clg@kaod.org>
>>
>> This will be used to construct a memory region beyond the RAM region
>> to let firmwares scan the address space with load/store to guess how
>> much RAM the SoC has.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>>  hw/arm/aspeed.c               | 31 +++++++++++++++++++++++++++++++
>>  hw/arm/aspeed_soc.c           |  2 ++
>>  hw/misc/aspeed_sdmc.c         |  3 +++
>>  include/hw/misc/aspeed_sdmc.h |  1 +
>>  4 files changed, 37 insertions(+)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index bb9d33848d3f..e078269266bc 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -31,6 +31,7 @@ static struct arm_boot_info aspeed_board_binfo = {
>>  typedef struct AspeedBoardState {
>>      AspeedSoCState soc;
>>      MemoryRegion ram;
>> +    MemoryRegion max_ram;
>>  } AspeedBoardState;
>>
>>  typedef struct AspeedBoardConfig {
>> @@ -127,6 +128,27 @@ static const AspeedBoardConfig aspeed_boards[] = {
>>      },
>>  };
>>
>> +/*
>> + * The max ram region is for firmwares that scan the address space
>> + * with load/store to guess how much RAM the SoC has.
>> + */
>> +static uint64_t max_ram_read(void *opaque, hwaddr offset, unsigned size)
>> +{
>> +    return 0;
>> +}
>> +
>> +static void max_ram_write(void *opaque, hwaddr offset, uint64_t value,
>> +                           unsigned size)
>> +{
>> +    /* Disacard writes */
>> +}
>> +
>> +static const MemoryRegionOps max_ram_ops = {
>> +    .read = max_ram_read,
>> +    .write = max_ram_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>>  #define FIRMWARE_ADDR 0x0
>>
>>  static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
>> @@ -187,6 +209,7 @@ static void aspeed_board_init(MachineState *machine,
>>      AspeedBoardState *bmc;
>>      AspeedSoCClass *sc;
>>      DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
>> +    ram_addr_t max_ram_size;
>>
>>      bmc = g_new0(AspeedBoardState, 1);
>>      object_initialize(&bmc->soc, (sizeof(bmc->soc)), cfg->soc_name);
>> @@ -226,6 +249,14 @@ static void aspeed_board_init(MachineState *machine,
>>      object_property_add_const_link(OBJECT(&bmc->soc), "ram", OBJECT(&bmc->ram),
>>                                     &error_abort);
>>
>> +    max_ram_size = object_property_get_uint(OBJECT(&bmc->soc), "max-ram-size",
>> +                                            &error_abort);
>> +    memory_region_init_io(&bmc->max_ram, NULL, &max_ram_ops, NULL,
>> +                          "max_ram", max_ram_size  - ram_size);
>> +    memory_region_add_subregion(get_system_memory(),
>> +                                sc->info->sdram_base + ram_size,
>> +                                &bmc->max_ram);
> 
> I'm surprised that you need the IO ops, ie that it doesn't work
> just to define an empty container region with memory_region_init().

Initially, there was some logging in the IO ops, which was a nice to have.
But that was dropped in this patch. No a big issue I think.

Thanks,

C.