[PATCH v2 0/5] aspeed/smc: Improve support for the alternate boot function

[PATCH v2 0/5] aspeed/smc: Improve support for the alternate boot function

[Cédric Le Goater]

Hello,

The Aspeed SoCs have a dual boot function for firmware fail-over
recovery. The system auto-reboots from the second flash if the main
flash does not boot successfully within a certain amount of time. This
function is called alternate boot (ABR) in the FMC controllers.

On the AST2600, the ABR registers controlling the 2nd watchdog timer
were moved from the watchdog register to the FMC controller. To
control WDT2 through the FMC model register set, this series creates a
local address space on top of WDT2 memory region.

To test on the fuji-bmc machine, run :

    devmem 0x1e620064
    devmem 0x1e78504C 

    devmem 0x1e620064 32 0xffffffff
    devmem 0x1e620064
    devmem 0x1e78504C
    
Thanks

C.

Changes since v2:

 - introduce a container region for the WDT2 register address space
 - introduce a container region for the flash mmio address space

Cédric Le Goater (5):
  aspeed/wdt: Introduce a container for the MMIO region
  aspeed: Initialize the watchdog device models before the FMC models
  aspeed/smc: Improve support for the alternate boot function
  aspeed/smc: Use a container for the flash mmio address space
  speed/sdhci: Add trace events

 include/hw/ssi/aspeed_smc.h      |  5 +-
 include/hw/watchdog/wdt_aspeed.h |  1 +
 hw/arm/aspeed_ast2600.c          | 38 +++++++-------
 hw/arm/aspeed_soc.c              | 36 ++++++-------
 hw/sd/aspeed_sdhci.c             |  5 ++
 hw/ssi/aspeed_smc.c              | 89 +++++++++++++++++++++++++++++---
 hw/watchdog/wdt_aspeed.c         |  6 ++-
 hw/sd/trace-events               |  4 ++
 hw/ssi/trace-events              |  1 +
 9 files changed, 141 insertions(+), 44 deletions(-)

-- 
2.31.1

[PATCH v2 1/5] aspeed/wdt: Introduce a container for the MMIO region

[Cédric Le Goater]

On the AST2600, the 2nd watchdog timer can be controlled through the
FMC controller to disable the alternate boot function. Next changes
will map the WDT2 registers in the AST2600 FMC memory region. Add a
container on top of the register region for this purpose.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/watchdog/wdt_aspeed.h | 1 +
 hw/watchdog/wdt_aspeed.c         | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
index f945cd6c5833..14e91acb1284 100644
--- a/include/hw/watchdog/wdt_aspeed.h
+++ b/include/hw/watchdog/wdt_aspeed.h
@@ -28,6 +28,7 @@ struct AspeedWDTState {
     QEMUTimer *timer;
 
     /*< public >*/
+    MemoryRegion iomem_container;
     MemoryRegion iomem;
     uint32_t regs[ASPEED_WDT_REGS_MAX];
 
diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index 146ffcd71301..803e861a9c61 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -275,9 +275,13 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
      */
     s->pclk_freq = PCLK_HZ;
 
+    memory_region_init(&s->iomem_container, OBJECT(s),
+                       TYPE_ASPEED_WDT ".container", ASPEED_WDT_REGS_MAX * 4);
+    sysbus_init_mmio(sbd, &s->iomem_container);
+
     memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
                           TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
-    sysbus_init_mmio(sbd, &s->iomem);
+    memory_region_add_subregion(&s->iomem_container, 0x0, &s->iomem);
 }
 
 static Property aspeed_wdt_properties[] = {
-- 
2.31.1

[PATCH v2 2/5] aspeed: Initialize the watchdog device models before the FMC models

[Cédric Le Goater]

Next changes will map the WDT2 registers in the AST2600 FMC memory
region. Make sure the MemoryRegion pointers are correctly initialized
before setting the object links.

Do the same in the Aspeed AST2400 and AST2500 SoC models for
consistency.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed_ast2600.c | 36 ++++++++++++++++++------------------
 hw/arm/aspeed_soc.c     | 36 ++++++++++++++++++------------------
 2 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 0384357a9510..3a7aa910b157 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -148,6 +148,11 @@ static void aspeed_soc_ast2600_init(Object *obj)
     snprintf(typename, sizeof(typename), "aspeed.timer-%s", socname);
     object_initialize_child(obj, "timerctrl", &s->timerctrl, typename);
 
+    for (i = 0; i < sc->wdts_num; i++) {
+        snprintf(typename, sizeof(typename), "aspeed.wdt-%s", socname);
+        object_initialize_child(obj, "wdt[*]", &s->wdt[i], typename);
+    }
+
     snprintf(typename, sizeof(typename), "aspeed.adc-%s", socname);
     object_initialize_child(obj, "adc", &s->adc, typename);
 
@@ -175,11 +180,6 @@ static void aspeed_soc_ast2600_init(Object *obj)
     object_property_add_alias(obj, "max-ram-size", OBJECT(&s->sdmc),
                               "max-ram-size");
 
-    for (i = 0; i < sc->wdts_num; i++) {
-        snprintf(typename, sizeof(typename), "aspeed.wdt-%s", socname);
-        object_initialize_child(obj, "wdt[*]", &s->wdt[i], typename);
-    }
-
     for (i = 0; i < sc->macs_num; i++) {
         object_initialize_child(obj, "ftgmac100[*]", &s->ftgmac100[i],
                                 TYPE_FTGMAC100);
@@ -325,6 +325,19 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
     }
 
+    /* Watch dog */
+    for (i = 0; i < sc->wdts_num; i++) {
+        AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(&s->wdt[i]);
+
+        object_property_set_link(OBJECT(&s->wdt[i]), "scu", OBJECT(&s->scu),
+                                 &error_abort);
+        if (!sysbus_realize(SYS_BUS_DEVICE(&s->wdt[i]), errp)) {
+            return;
+        }
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0,
+                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
+    }
+
     /* ADC */
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->adc), errp)) {
         return;
@@ -395,19 +408,6 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     }
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, sc->memmap[ASPEED_DEV_SDMC]);
 
-    /* Watch dog */
-    for (i = 0; i < sc->wdts_num; i++) {
-        AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(&s->wdt[i]);
-
-        object_property_set_link(OBJECT(&s->wdt[i]), "scu", OBJECT(&s->scu),
-                                 &error_abort);
-        if (!sysbus_realize(SYS_BUS_DEVICE(&s->wdt[i]), errp)) {
-            return;
-        }
-        sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0,
-                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
-    }
-
     /* Net */
     for (i = 0; i < sc->macs_num; i++) {
         object_property_set_bool(OBJECT(&s->ftgmac100[i]), "aspeed", true,
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 7d53cf2f5133..2eb30d14cf94 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -162,6 +162,11 @@ static void aspeed_soc_init(Object *obj)
     snprintf(typename, sizeof(typename), "aspeed.timer-%s", socname);
     object_initialize_child(obj, "timerctrl", &s->timerctrl, typename);
 
+    for (i = 0; i < sc->wdts_num; i++) {
+        snprintf(typename, sizeof(typename), "aspeed.wdt-%s", socname);
+        object_initialize_child(obj, "wdt[*]", &s->wdt[i], typename);
+    }
+
     snprintf(typename, sizeof(typename), "aspeed.adc-%s", socname);
     object_initialize_child(obj, "adc", &s->adc, typename);
 
@@ -189,11 +194,6 @@ static void aspeed_soc_init(Object *obj)
     object_property_add_alias(obj, "max-ram-size", OBJECT(&s->sdmc),
                               "max-ram-size");
 
-    for (i = 0; i < sc->wdts_num; i++) {
-        snprintf(typename, sizeof(typename), "aspeed.wdt-%s", socname);
-        object_initialize_child(obj, "wdt[*]", &s->wdt[i], typename);
-    }
-
     for (i = 0; i < sc->macs_num; i++) {
         object_initialize_child(obj, "ftgmac100[*]", &s->ftgmac100[i],
                                 TYPE_FTGMAC100);
@@ -290,6 +290,19 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
     }
 
+    /* Watch dog */
+    for (i = 0; i < sc->wdts_num; i++) {
+        AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(&s->wdt[i]);
+
+        object_property_set_link(OBJECT(&s->wdt[i]), "scu", OBJECT(&s->scu),
+                                 &error_abort);
+        if (!sysbus_realize(SYS_BUS_DEVICE(&s->wdt[i]), errp)) {
+            return;
+        }
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0,
+                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
+    }
+
     /* ADC */
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->adc), errp)) {
         return;
@@ -354,19 +367,6 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     }
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, sc->memmap[ASPEED_DEV_SDMC]);
 
-    /* Watch dog */
-    for (i = 0; i < sc->wdts_num; i++) {
-        AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(&s->wdt[i]);
-
-        object_property_set_link(OBJECT(&s->wdt[i]), "scu", OBJECT(&s->scu),
-                                 &error_abort);
-        if (!sysbus_realize(SYS_BUS_DEVICE(&s->wdt[i]), errp)) {
-            return;
-        }
-        sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0,
-                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
-    }
-
     /* Net */
     for (i = 0; i < sc->macs_num; i++) {
         object_property_set_bool(OBJECT(&s->ftgmac100[i]), "aspeed", true,
-- 
2.31.1

[PATCH v2 3/5] aspeed/smc: Improve support for the alternate boot function

[Cédric Le Goater]

Map the WDT2 registers in the AST2600 FMC memory region by creating a
local address space on top of WDT2 memory region.

The model only implements the enable bit of the control register. The
reload register uses a 0.1s unit instead of a 1us. Values are
converted on the fly when doing the accesses. The restart register is
the same.

Cc: Peter Delevoryas <pdel@fb.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/aspeed_smc.h |  3 ++
 hw/arm/aspeed_ast2600.c     |  2 +
 hw/ssi/aspeed_smc.c         | 78 ++++++++++++++++++++++++++++++++++++-
 hw/ssi/trace-events         |  1 +
 4 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 75bc793bd269..ad3c80f2d809 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -76,6 +76,9 @@ struct AspeedSMCState {
     MemoryRegion *dram_mr;
     AddressSpace dram_as;
 
+    AddressSpace wdt2_as;
+    MemoryRegion *wdt2_mr;
+
     AspeedSMCFlash flashes[ASPEED_SMC_CS_MAX];
 
     uint8_t snoop_index;
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 3a7aa910b157..4abb0bb91e92 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -366,6 +366,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     }
 
     /* FMC, The number of CS is set at the board level */
+    object_property_set_link(OBJECT(&s->fmc), "wdt2", OBJECT(&s->wdt[2].iomem),
+                             &error_abort);
     object_property_set_link(OBJECT(&s->fmc), "dram", OBJECT(s->dram_mr),
                              &error_abort);
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->fmc), errp)) {
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 8a988c167604..1770985230b0 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -130,6 +130,8 @@
 #define   FMC_WDT2_CTRL_SINGLE_BOOT_MODE BIT(5)
 #define   FMC_WDT2_CTRL_BOOT_SOURCE      BIT(4) /* O: primary 1: alternate */
 #define   FMC_WDT2_CTRL_EN               BIT(0)
+#define R_FMC_WDT2_RELOAD   (0x68 / 4)
+#define R_FMC_WDT2_RESTART  (0x6C / 4)
 
 /* DMA Control/Status Register */
 #define R_DMA_CTRL        (0x80 / 4)
@@ -704,6 +706,54 @@ static void aspeed_smc_reset(DeviceState *d)
     s->snoop_dummies = 0;
 }
 
+#define ASPEED_WDT_RELOAD  0x04
+#define ASPEED_WDT_RESTART 0x08
+#define ASPEED_WDT_CTRL    0x0C
+
+static void aspeed_smc_wdt2_write(AspeedSMCState *s, uint32_t offset,
+                                  uint32_t value)
+{
+    MemTxResult result;
+
+    address_space_stl_le(&s->wdt2_as, offset, value, MEMTXATTRS_UNSPECIFIED,
+                         &result);
+    if (result != MEMTX_OK) {
+        aspeed_smc_error("WDT2 write failed @%08x", offset);
+        return;
+    }
+}
+
+static uint64_t aspeed_smc_wdt2_read(AspeedSMCState *s, uint32_t offset)
+{
+    MemTxResult result;
+    uint32_t value;
+
+    value = address_space_ldl_le(&s->wdt2_as, offset, MEMTXATTRS_UNSPECIFIED,
+                                &result);
+    if (result != MEMTX_OK) {
+        aspeed_smc_error("WDT2 read failed @%08x", offset);
+        return -1;
+    }
+    return value;
+}
+
+static void aspeed_smc_wdt2_enable(AspeedSMCState *s, bool enable)
+{
+    uint32_t value;
+
+    value = aspeed_smc_wdt2_read(s, ASPEED_WDT_CTRL);
+    if (value == -1) {
+        return;
+    }
+
+    value &= ~BIT(0);
+    value |= enable;
+
+    aspeed_smc_wdt2_write(s, ASPEED_WDT_CTRL, value);
+
+    trace_aspeed_smc_wdt2_enable(enable ? "en" : "dis");
+}
+
 static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
 {
     AspeedSMCState *s = ASPEED_SMC(opaque);
@@ -718,7 +768,6 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
         addr == R_CE_CMD_CTRL ||
         addr == R_INTR_CTRL ||
         addr == R_DUMMY_DATA ||
-        (aspeed_smc_has_wdt_control(asc) && addr == R_FMC_WDT2_CTRL) ||
         (aspeed_smc_has_dma(asc) && addr == R_DMA_CTRL) ||
         (aspeed_smc_has_dma(asc) && addr == R_DMA_FLASH_ADDR) ||
         (aspeed_smc_has_dma(asc) && addr == R_DMA_DRAM_ADDR) ||
@@ -731,6 +780,10 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
         trace_aspeed_smc_read(addr << 2, size, s->regs[addr]);
 
         return s->regs[addr];
+    } else if (aspeed_smc_has_wdt_control(asc) && addr == R_FMC_WDT2_CTRL) {
+        return aspeed_smc_wdt2_read(s, ASPEED_WDT_CTRL);
+    } else if (aspeed_smc_has_wdt_control(asc) && addr == R_FMC_WDT2_RELOAD) {
+        return aspeed_smc_wdt2_read(s, ASPEED_WDT_RELOAD) / 100000;
     } else {
         qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
                       __func__, addr);
@@ -1053,7 +1106,11 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
     } else if (addr == R_DUMMY_DATA) {
         s->regs[addr] = value & 0xff;
     } else if (aspeed_smc_has_wdt_control(asc) && addr == R_FMC_WDT2_CTRL) {
-        s->regs[addr] = value & FMC_WDT2_CTRL_EN;
+        aspeed_smc_wdt2_enable(s, !!(value & FMC_WDT2_CTRL_EN));
+    } else if (aspeed_smc_has_wdt_control(asc) && addr == R_FMC_WDT2_RELOAD) {
+        aspeed_smc_wdt2_write(s, ASPEED_WDT_RELOAD, value * 100000);
+    } else if (aspeed_smc_has_wdt_control(asc) && addr == R_FMC_WDT2_RESTART) {
+        aspeed_smc_wdt2_write(s, ASPEED_WDT_RESTART, value);
     } else if (addr == R_INTR_CTRL) {
         s->regs[addr] = value;
     } else if (aspeed_smc_has_dma(asc) && addr == R_DMA_CTRL) {
@@ -1108,6 +1165,16 @@ static void aspeed_smc_dma_setup(AspeedSMCState *s, Error **errp)
                        TYPE_ASPEED_SMC ".dma-dram");
 }
 
+static void aspeed_smc_wdt_setup(AspeedSMCState *s, Error **errp)
+{
+    if (!s->wdt2_mr) {
+        error_setg(errp, TYPE_ASPEED_SMC ": 'wdt2' link not set");
+        return;
+    }
+
+    address_space_init(&s->wdt2_as, s->wdt2_mr, TYPE_ASPEED_SMC ".wdt2");
+}
+
 static void aspeed_smc_realize(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
@@ -1189,6 +1256,11 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
     if (aspeed_smc_has_dma(asc)) {
         aspeed_smc_dma_setup(s, errp);
     }
+
+    /* WDT2 support */
+    if (aspeed_smc_has_wdt_control(asc)) {
+        aspeed_smc_wdt_setup(s, errp);
+    }
 }
 
 static const VMStateDescription vmstate_aspeed_smc = {
@@ -1208,6 +1280,8 @@ static Property aspeed_smc_properties[] = {
     DEFINE_PROP_BOOL("inject-failure", AspeedSMCState, inject_failure, false),
     DEFINE_PROP_LINK("dram", AspeedSMCState, dram_mr,
                      TYPE_MEMORY_REGION, MemoryRegion *),
+    DEFINE_PROP_LINK("wdt2", AspeedSMCState, wdt2_mr,
+                     TYPE_MEMORY_REGION, MemoryRegion *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
index 612d3d6087aa..0de79bf9c6a5 100644
--- a/hw/ssi/trace-events
+++ b/hw/ssi/trace-events
@@ -9,6 +9,7 @@ aspeed_smc_dma_checksum(uint32_t addr, uint32_t data) "0x%08x: 0x%08x"
 aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint32_t dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%08x size:0x%08x"
 aspeed_smc_write(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
 aspeed_smc_flash_select(int cs, const char *prefix) "CS%d %sselect"
+aspeed_smc_wdt2_enable(const char *prefix) "WDT2 is %sabled"
 
 # npcm7xx_fiu.c
 
-- 
2.31.1

[PATCH v2 4/5] aspeed/smc: Use a container for the flash mmio address space

[Cédric Le Goater]

Because AddressSpaces must not be sysbus-mapped, commit e9c568dbc225
("hw/arm/aspeed: Do not sysbus-map mmio flash region directly, use
alias") introduced an alias for the flash mmio region.

Using a container is cleaner.

Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/aspeed_smc.h |  2 +-
 hw/ssi/aspeed_smc.c         | 11 +++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index ad3c80f2d809..61d23ec1f13e 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -52,8 +52,8 @@ struct AspeedSMCState {
     SysBusDevice parent_obj;
 
     MemoryRegion mmio;
+    MemoryRegion mmio_flash_container;
     MemoryRegion mmio_flash;
-    MemoryRegion mmio_flash_alias;
 
     qemu_irq irq;
 
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 1770985230b0..d4f03881ddf5 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -1218,14 +1218,17 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
      * window in which the flash modules are mapped. The size and
      * address depends on the SoC model and controller type.
      */
+    memory_region_init(&s->mmio_flash_container, OBJECT(s),
+                       TYPE_ASPEED_SMC ".container",
+                       asc->flash_window_size);
+    sysbus_init_mmio(sbd, &s->mmio_flash_container);
+
     memory_region_init_io(&s->mmio_flash, OBJECT(s),
                           &aspeed_smc_flash_default_ops, s,
                           TYPE_ASPEED_SMC ".flash",
                           asc->flash_window_size);
-    memory_region_init_alias(&s->mmio_flash_alias, OBJECT(s),
-                             TYPE_ASPEED_SMC ".flash",
-                             &s->mmio_flash, 0, asc->flash_window_size);
-    sysbus_init_mmio(sbd, &s->mmio_flash_alias);
+    memory_region_add_subregion(&s->mmio_flash_container, 0x0,
+                                &s->mmio_flash);
 
     /*
      * Let's create a sub memory region for each possible peripheral. All
-- 
2.31.1

[PATCH v2 5/5] speed/sdhci: Add trace events

[Cédric Le Goater]

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/sd/aspeed_sdhci.c | 5 +++++
 hw/sd/trace-events   | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
index 3299844de6dc..df1bdf1fa4ed 100644
--- a/hw/sd/aspeed_sdhci.c
+++ b/hw/sd/aspeed_sdhci.c
@@ -14,6 +14,7 @@
 #include "hw/irq.h"
 #include "migration/vmstate.h"
 #include "hw/qdev-properties.h"
+#include "trace.h"
 
 #define ASPEED_SDHCI_INFO            0x00
 #define  ASPEED_SDHCI_INFO_SLOT1     (1 << 17)
@@ -60,6 +61,8 @@ static uint64_t aspeed_sdhci_read(void *opaque, hwaddr addr, unsigned int size)
         }
     }
 
+    trace_aspeed_sdhci_read(addr, size, (uint64_t) val);
+
     return (uint64_t)val;
 }
 
@@ -68,6 +71,8 @@ static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
 {
     AspeedSDHCIState *sdhci = opaque;
 
+    trace_aspeed_sdhci_write(addr, size, val);
+
     switch (addr) {
     case ASPEED_SDHCI_INFO:
         /* The RESET bit automatically clears. */
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 3cc2ef89ba6b..94a00557b26f 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -68,3 +68,7 @@ pl181_fifo_push(uint32_t data) "FIFO push 0x%08" PRIx32
 pl181_fifo_pop(uint32_t data) "FIFO pop 0x%08" PRIx32
 pl181_fifo_transfer_complete(void) "FIFO transfer complete"
 pl181_data_engine_idle(void) "data engine idle"
+
+# aspeed_sdhci.c
+aspeed_sdhci_read(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
+aspeed_sdhci_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
-- 
2.31.1

Re: [PATCH v2 0/5] aspeed/smc: Improve support for the alternate boot function

[Peter Delevoryas]

> On Oct 18, 2021, at 6:26 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> Hello,
> 
> The Aspeed SoCs have a dual boot function for firmware fail-over
> recovery. The system auto-reboots from the second flash if the main
> flash does not boot successfully within a certain amount of time. This
> function is called alternate boot (ABR) in the FMC controllers.
> 
> On the AST2600, the ABR registers controlling the 2nd watchdog timer
> were moved from the watchdog register to the FMC controller. To
> control WDT2 through the FMC model register set, this series creates a
> local address space on top of WDT2 memory region.
> 
> To test on the fuji-bmc machine, run :
> 
>    devmem 0x1e620064
>    devmem 0x1e78504C 
> 
>    devmem 0x1e620064 32 0xffffffff
>    devmem 0x1e620064
>    devmem 0x1e78504C

This looks good to me! I looked at the whole
patch series, I think all the changes look right.

By the way, just to make sure I’m understanding correctly:

The AST2400 datasheet shows only 2 watchdog timers, and
the first to be used as the primary system deadlock
reset (but still reboot from the primary flash), and the
second watchdog is designated as an alternate boot
watchdog, which reboots from secondary flash and is
only enabled if there’s a specific hw strap pin enabled,
and the second watchdog is usually disabled once booting
is successful, right?

The AST2600 datasheet shows there’s 8 watchdogs (but
we only have 4 declared in QEMU? I see only the first
four support external reset signals, maybe that’s why?)
but it doesn’t seem to say explicitly that the 2nd
watchdog is the alternate boot watchdog, it’s probably
just implied that the user read the AST2400/AST2500 docs
right? And the FMC registers are just an alias to write
to these watchdog 2 registers? Just curious, is it
strictly necessary to use the FMC registers to disable
the alternate boot watchdog, or could you just use the
old address, 0x1e78504C? In our OpenBMC initialization
for Fuji, we’re using the FMC registers, but would
it still work if we used the old addresses? Just curious,
the more I think about it, it seems odd to me that these
FMC watchdog registers exist if they’re just an alias.

Also, I was wondering: does the alternate boot
watchdog actually switch the flash device or flash
region that we boot from, or does it just reboot from
the primary partition? I don’t see anything in
watchdog_perform_action() that obviously indicates we’re
actually switching to a secondary flash, so I was curious
about that.

Thanks for adding this though! This is very useful, we’re
using QEMU more and more for testing, especially the
boot process, so more accurate emulation of this functionality
is great.

Thanks,
Peter

Reviewed-by: Peter Delevoryas <pdel@fb.com>

> 
> Thanks
> 
> C.
> 
> Changes since v2:
> 
> - introduce a container region for the WDT2 register address space
> - introduce a container region for the flash mmio address space
> 
> Cédric Le Goater (5):
>  aspeed/wdt: Introduce a container for the MMIO region
>  aspeed: Initialize the watchdog device models before the FMC models
>  aspeed/smc: Improve support for the alternate boot function
>  aspeed/smc: Use a container for the flash mmio address space
>  speed/sdhci: Add trace events
> 
> include/hw/ssi/aspeed_smc.h      |  5 +-
> include/hw/watchdog/wdt_aspeed.h |  1 +
> hw/arm/aspeed_ast2600.c          | 38 +++++++-------
> hw/arm/aspeed_soc.c              | 36 ++++++-------
> hw/sd/aspeed_sdhci.c             |  5 ++
> hw/ssi/aspeed_smc.c              | 89 +++++++++++++++++++++++++++++---
> hw/watchdog/wdt_aspeed.c         |  6 ++-
> hw/sd/trace-events               |  4 ++
> hw/ssi/trace-events              |  1 +
> 9 files changed, 141 insertions(+), 44 deletions(-)
> 
> -- 
> 2.31.1
> 
> 
> 

Re: [PATCH v2 0/5] aspeed/smc: Improve support for the alternate boot function

[Cédric Le Goater]

On 10/20/21 06:57, Peter Delevoryas wrote:
> 
> 
>> On Oct 18, 2021, at 6:26 AM, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> Hello,
>>
>> The Aspeed SoCs have a dual boot function for firmware fail-over
>> recovery. The system auto-reboots from the second flash if the main
>> flash does not boot successfully within a certain amount of time. This
>> function is called alternate boot (ABR) in the FMC controllers.
>>
>> On the AST2600, the ABR registers controlling the 2nd watchdog timer
>> were moved from the watchdog register to the FMC controller. To
>> control WDT2 through the FMC model register set, this series creates a
>> local address space on top of WDT2 memory region.
>>
>> To test on the fuji-bmc machine, run :
>>
>>     devmem 0x1e620064
>>     devmem 0x1e78504C
>>
>>     devmem 0x1e620064 32 0xffffffff
>>     devmem 0x1e620064
>>     devmem 0x1e78504C
> 
> This looks good to me! I looked at the whole
> patch series, I think all the changes look right.
> 
> By the way, just to make sure I’m understanding correctly:
> 
> The AST2400 datasheet shows only 2 watchdog timers, and
> the first to be used as the primary system deadlock
> reset (but still reboot from the primary flash), and the
> second watchdog is designated as an alternate boot
> watchdog, which reboots from secondary flash and is
> only enabled if there’s a specific hw strap pin enabled,
> and the second watchdog is usually disabled once booting
> is successful, right?

Yes. I think WDT2 was activated in uboot on these platforms.

> The AST2600 datasheet shows there’s 8 watchdogs (but
> we only have 4 declared in QEMU? I see only the first
> four support external reset signals, maybe that’s why?)

Indeed. The datasheet also says :

   Watchdog Timer (WDT) includes 4 sets of 32-bit decrement
   counters,

which might have induced us in error :) I will include a fix
for it.

> but it doesn’t seem to say explicitly that the 2nd
> watchdog is the alternate boot watchdog, 

True. That's my assumption for the model and we could also
instantiate a new watchdog in the SMC/FMC model.

> it’s probably
> just implied that the user read the AST2400/AST2500 docs right? 

I think Aspeed is cleaning up the WDT logic by moving "exotic"
features to other controllers. that would be why some registers
of WDT1 and WDT2 are exposed in the FMC register space for 4B
detection and alternate boot :

   FMC60: FMC WDT1 Control/Status Register for Address Mode Detection
   FMC64: FMC WDT2 Control/Status Register for Alternate Boot
   FMC68: FMC WDT2 Timer Reload Value Register
   FMC6C: FMC WDT2 Timer Restart Register

and the FMC also has a new signal/pin : GPIOY6/FWSPIABR to handle ABR.
That's the most important change.


> And the FMC registers are just an alias to write
> to these watchdog 2 registers? 

If this is the same watchdog mapped into the FMC, I would say yes
and the logic generate load/stores transactions on the AHB bus.
Adding an address space for the WDT registers in the model is the
closer we can get without implementing the bus protocol.

> Just curious, is it
> strictly necessary to use the FMC registers to disable
> the alternate boot watchdog, or could you just use the
> old address, 0x1e78504C? 

Hey, this is something to try on HW and check how both register
sets evolve. Would you have time ?

> In our OpenBMC initialization
> for Fuji, we’re using the FMC registers, but would
> it still work if we used the old addresses? Just curious,
> the more I think about it, it seems odd to me that these
> FMC watchdog registers exist if they’re just an alias.

We should ask the HW designers.

> Also, I was wondering: does the alternate boot
> watchdog actually switch the flash device or flash
> region that we boot from, or does it just reboot from
> the primary partition? 

No. This is not modeled.

> I don’t see anything in
> watchdog_perform_action() that obviously indicates we’re
> actually switching to a secondary flash, so I was curious
> about that.

It is certainly feasible but it would require some thinking on
how the models interact with one another.

If a FMC_WDT2 watchdog model is owned by the SMC model, it would
be simpler. That's seem to be going in the direction of your
questions :)

> Thanks for adding this though! This is very useful, we’re
> using QEMU more and more for testing, especially the
> boot process, so more accurate emulation of this functionality
> is great.

Good. That's the goal.

> Thanks,
> Peter
> 
> Reviewed-by: Peter Delevoryas <pdel@fb.com>

It's worth checking with the HW designers before pushing anything.

Thanks,

C.


> 
>>
>> Thanks
>>
>> C.
>>
>> Changes since v2:
>>
>> - introduce a container region for the WDT2 register address space
>> - introduce a container region for the flash mmio address space
>>
>> Cédric Le Goater (5):
>>   aspeed/wdt: Introduce a container for the MMIO region
>>   aspeed: Initialize the watchdog device models before the FMC models
>>   aspeed/smc: Improve support for the alternate boot function
>>   aspeed/smc: Use a container for the flash mmio address space
>>   speed/sdhci: Add trace events
>>
>> include/hw/ssi/aspeed_smc.h      |  5 +-
>> include/hw/watchdog/wdt_aspeed.h |  1 +
>> hw/arm/aspeed_ast2600.c          | 38 +++++++-------
>> hw/arm/aspeed_soc.c              | 36 ++++++-------
>> hw/sd/aspeed_sdhci.c             |  5 ++
>> hw/ssi/aspeed_smc.c              | 89 +++++++++++++++++++++++++++++---
>> hw/watchdog/wdt_aspeed.c         |  6 ++-
>> hw/sd/trace-events               |  4 ++
>> hw/ssi/trace-events              |  1 +
>> 9 files changed, 141 insertions(+), 44 deletions(-)
>>
>> -- 
>> 2.31.1
>>
>>
>>
> 

Re: [PATCH v2 1/5] aspeed/wdt: Introduce a container for the MMIO region

[Philippe Mathieu-Daudé]

On 10/18/21 15:26, Cédric Le Goater wrote:
> On the AST2600, the 2nd watchdog timer can be controlled through the
> FMC controller to disable the alternate boot function. Next changes
> will map the WDT2 registers in the AST2600 FMC memory region. Add a
> container on top of the register region for this purpose.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/watchdog/wdt_aspeed.h | 1 +
>  hw/watchdog/wdt_aspeed.c         | 6 +++++-
>  2 files changed, 6 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 2/5] aspeed: Initialize the watchdog device models before the FMC models

[Philippe Mathieu-Daudé]

On 10/18/21 15:26, Cédric Le Goater wrote:
> Next changes will map the WDT2 registers in the AST2600 FMC memory
> region. Make sure the MemoryRegion pointers are correctly initialized
> before setting the object links.
> 
> Do the same in the Aspeed AST2400 and AST2500 SoC models for
> consistency.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/arm/aspeed_ast2600.c | 36 ++++++++++++++++++------------------
>  hw/arm/aspeed_soc.c     | 36 ++++++++++++++++++------------------
>  2 files changed, 36 insertions(+), 36 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 4/5] aspeed/smc: Use a container for the flash mmio address space

[Philippe Mathieu-Daudé]

On 10/18/21 15:26, Cédric Le Goater wrote:
> Because AddressSpaces must not be sysbus-mapped, commit e9c568dbc225
> ("hw/arm/aspeed: Do not sysbus-map mmio flash region directly, use
> alias") introduced an alias for the flash mmio region.
> 
> Using a container is cleaner.
> 
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ssi/aspeed_smc.h |  2 +-
>  hw/ssi/aspeed_smc.c         | 11 +++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 5/5] speed/sdhci: Add trace events

[Philippe Mathieu-Daudé]

On 10/18/21 15:26, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/sd/aspeed_sdhci.c | 5 +++++
>  hw/sd/trace-events   | 4 ++++
>  2 files changed, 9 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 5/5] speed/sdhci: Add trace events

[Francisco Iglesias]

Hi Cedric,

On the subject s/speed/aspeed/. Otherwise:

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

/BR

On [2021 Oct 18] Mon 15:26:09, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/sd/aspeed_sdhci.c | 5 +++++
>  hw/sd/trace-events   | 4 ++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
> index 3299844de6dc..df1bdf1fa4ed 100644
> --- a/hw/sd/aspeed_sdhci.c
> +++ b/hw/sd/aspeed_sdhci.c
> @@ -14,6 +14,7 @@
>  #include "hw/irq.h"
>  #include "migration/vmstate.h"
>  #include "hw/qdev-properties.h"
> +#include "trace.h"
>  
>  #define ASPEED_SDHCI_INFO            0x00
>  #define  ASPEED_SDHCI_INFO_SLOT1     (1 << 17)
> @@ -60,6 +61,8 @@ static uint64_t aspeed_sdhci_read(void *opaque, hwaddr addr, unsigned int size)
>          }
>      }
>  
> +    trace_aspeed_sdhci_read(addr, size, (uint64_t) val);
> +
>      return (uint64_t)val;
>  }
>  
> @@ -68,6 +71,8 @@ static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
>  {
>      AspeedSDHCIState *sdhci = opaque;
>  
> +    trace_aspeed_sdhci_write(addr, size, val);
> +
>      switch (addr) {
>      case ASPEED_SDHCI_INFO:
>          /* The RESET bit automatically clears. */
> diff --git a/hw/sd/trace-events b/hw/sd/trace-events
> index 3cc2ef89ba6b..94a00557b26f 100644
> --- a/hw/sd/trace-events
> +++ b/hw/sd/trace-events
> @@ -68,3 +68,7 @@ pl181_fifo_push(uint32_t data) "FIFO push 0x%08" PRIx32
>  pl181_fifo_pop(uint32_t data) "FIFO pop 0x%08" PRIx32
>  pl181_fifo_transfer_complete(void) "FIFO transfer complete"
>  pl181_data_engine_idle(void) "data engine idle"
> +
> +# aspeed_sdhci.c
> +aspeed_sdhci_read(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
> +aspeed_sdhci_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
> -- 
> 2.31.1
> 
> 

Re: [PATCH v2 1/5] aspeed/wdt: Introduce a container for the MMIO region

[Francisco Iglesias]

On [2021 Oct 18] Mon 15:26:05, Cédric Le Goater wrote:
> On the AST2600, the 2nd watchdog timer can be controlled through the
> FMC controller to disable the alternate boot function. Next changes
> will map the WDT2 registers in the AST2600 FMC memory region. Add a
> container on top of the register region for this purpose.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
>  include/hw/watchdog/wdt_aspeed.h | 1 +
>  hw/watchdog/wdt_aspeed.c         | 6 +++++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
> index f945cd6c5833..14e91acb1284 100644
> --- a/include/hw/watchdog/wdt_aspeed.h
> +++ b/include/hw/watchdog/wdt_aspeed.h
> @@ -28,6 +28,7 @@ struct AspeedWDTState {
>      QEMUTimer *timer;
>  
>      /*< public >*/
> +    MemoryRegion iomem_container;
>      MemoryRegion iomem;
>      uint32_t regs[ASPEED_WDT_REGS_MAX];
>  
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index 146ffcd71301..803e861a9c61 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -275,9 +275,13 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
>       */
>      s->pclk_freq = PCLK_HZ;
>  
> +    memory_region_init(&s->iomem_container, OBJECT(s),
> +                       TYPE_ASPEED_WDT ".container", ASPEED_WDT_REGS_MAX * 4);
> +    sysbus_init_mmio(sbd, &s->iomem_container);
> +
>      memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
>                            TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
> -    sysbus_init_mmio(sbd, &s->iomem);
> +    memory_region_add_subregion(&s->iomem_container, 0x0, &s->iomem);
>  }
>  
>  static Property aspeed_wdt_properties[] = {
> -- 
> 2.31.1
> 
> 

Re: [PATCH v2 2/5] aspeed: Initialize the watchdog device models before the FMC models

[Francisco Iglesias]

On [2021 Oct 18] Mon 15:26:06, Cédric Le Goater wrote:
> Next changes will map the WDT2 registers in the AST2600 FMC memory
> region. Make sure the MemoryRegion pointers are correctly initialized
> before setting the object links.
> 
> Do the same in the Aspeed AST2400 and AST2500 SoC models for
> consistency.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
>  hw/arm/aspeed_ast2600.c | 36 ++++++++++++++++++------------------
>  hw/arm/aspeed_soc.c     | 36 ++++++++++++++++++------------------
>  2 files changed, 36 insertions(+), 36 deletions(-)
> 
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 0384357a9510..3a7aa910b157 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -148,6 +148,11 @@ static void aspeed_soc_ast2600_init(Object *obj)
>      snprintf(typename, sizeof(typename), "aspeed.timer-%s", socname);
>      object_initialize_child(obj, "timerctrl", &s->timerctrl, typename);
>  
> +    for (i = 0; i < sc->wdts_num; i++) {
> +        snprintf(typename, sizeof(typename), "aspeed.wdt-%s", socname);
> +        object_initialize_child(obj, "wdt[*]", &s->wdt[i], typename);
> +    }
> +
>      snprintf(typename, sizeof(typename), "aspeed.adc-%s", socname);
>      object_initialize_child(obj, "adc", &s->adc, typename);
>  
> @@ -175,11 +180,6 @@ static void aspeed_soc_ast2600_init(Object *obj)
>      object_property_add_alias(obj, "max-ram-size", OBJECT(&s->sdmc),
>                                "max-ram-size");
>  
> -    for (i = 0; i < sc->wdts_num; i++) {
> -        snprintf(typename, sizeof(typename), "aspeed.wdt-%s", socname);
> -        object_initialize_child(obj, "wdt[*]", &s->wdt[i], typename);
> -    }
> -
>      for (i = 0; i < sc->macs_num; i++) {
>          object_initialize_child(obj, "ftgmac100[*]", &s->ftgmac100[i],
>                                  TYPE_FTGMAC100);
> @@ -325,6 +325,19 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
>      }
>  
> +    /* Watch dog */
> +    for (i = 0; i < sc->wdts_num; i++) {
> +        AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(&s->wdt[i]);
> +
> +        object_property_set_link(OBJECT(&s->wdt[i]), "scu", OBJECT(&s->scu),
> +                                 &error_abort);
> +        if (!sysbus_realize(SYS_BUS_DEVICE(&s->wdt[i]), errp)) {
> +            return;
> +        }
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0,
> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> +    }
> +
>      /* ADC */
>      if (!sysbus_realize(SYS_BUS_DEVICE(&s->adc), errp)) {
>          return;
> @@ -395,19 +408,6 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>      }
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, sc->memmap[ASPEED_DEV_SDMC]);
>  
> -    /* Watch dog */
> -    for (i = 0; i < sc->wdts_num; i++) {
> -        AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(&s->wdt[i]);
> -
> -        object_property_set_link(OBJECT(&s->wdt[i]), "scu", OBJECT(&s->scu),
> -                                 &error_abort);
> -        if (!sysbus_realize(SYS_BUS_DEVICE(&s->wdt[i]), errp)) {
> -            return;
> -        }
> -        sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0,
> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> -    }
> -
>      /* Net */
>      for (i = 0; i < sc->macs_num; i++) {
>          object_property_set_bool(OBJECT(&s->ftgmac100[i]), "aspeed", true,
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 7d53cf2f5133..2eb30d14cf94 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -162,6 +162,11 @@ static void aspeed_soc_init(Object *obj)
>      snprintf(typename, sizeof(typename), "aspeed.timer-%s", socname);
>      object_initialize_child(obj, "timerctrl", &s->timerctrl, typename);
>  
> +    for (i = 0; i < sc->wdts_num; i++) {
> +        snprintf(typename, sizeof(typename), "aspeed.wdt-%s", socname);
> +        object_initialize_child(obj, "wdt[*]", &s->wdt[i], typename);
> +    }
> +
>      snprintf(typename, sizeof(typename), "aspeed.adc-%s", socname);
>      object_initialize_child(obj, "adc", &s->adc, typename);
>  
> @@ -189,11 +194,6 @@ static void aspeed_soc_init(Object *obj)
>      object_property_add_alias(obj, "max-ram-size", OBJECT(&s->sdmc),
>                                "max-ram-size");
>  
> -    for (i = 0; i < sc->wdts_num; i++) {
> -        snprintf(typename, sizeof(typename), "aspeed.wdt-%s", socname);
> -        object_initialize_child(obj, "wdt[*]", &s->wdt[i], typename);
> -    }
> -
>      for (i = 0; i < sc->macs_num; i++) {
>          object_initialize_child(obj, "ftgmac100[*]", &s->ftgmac100[i],
>                                  TYPE_FTGMAC100);
> @@ -290,6 +290,19 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
>      }
>  
> +    /* Watch dog */
> +    for (i = 0; i < sc->wdts_num; i++) {
> +        AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(&s->wdt[i]);
> +
> +        object_property_set_link(OBJECT(&s->wdt[i]), "scu", OBJECT(&s->scu),
> +                                 &error_abort);
> +        if (!sysbus_realize(SYS_BUS_DEVICE(&s->wdt[i]), errp)) {
> +            return;
> +        }
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0,
> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> +    }
> +
>      /* ADC */
>      if (!sysbus_realize(SYS_BUS_DEVICE(&s->adc), errp)) {
>          return;
> @@ -354,19 +367,6 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>      }
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, sc->memmap[ASPEED_DEV_SDMC]);
>  
> -    /* Watch dog */
> -    for (i = 0; i < sc->wdts_num; i++) {
> -        AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(&s->wdt[i]);
> -
> -        object_property_set_link(OBJECT(&s->wdt[i]), "scu", OBJECT(&s->scu),
> -                                 &error_abort);
> -        if (!sysbus_realize(SYS_BUS_DEVICE(&s->wdt[i]), errp)) {
> -            return;
> -        }
> -        sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0,
> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> -    }
> -
>      /* Net */
>      for (i = 0; i < sc->macs_num; i++) {
>          object_property_set_bool(OBJECT(&s->ftgmac100[i]), "aspeed", true,
> -- 
> 2.31.1
> 
> 

Re: [PATCH v2 4/5] aspeed/smc: Use a container for the flash mmio address space

[Francisco Iglesias]

On [2021 Oct 18] Mon 15:26:08, Cédric Le Goater wrote:
> Because AddressSpaces must not be sysbus-mapped, commit e9c568dbc225
> ("hw/arm/aspeed: Do not sysbus-map mmio flash region directly, use
> alias") introduced an alias for the flash mmio region.
> 
> Using a container is cleaner.
> 
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
>  include/hw/ssi/aspeed_smc.h |  2 +-
>  hw/ssi/aspeed_smc.c         | 11 +++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
> index ad3c80f2d809..61d23ec1f13e 100644
> --- a/include/hw/ssi/aspeed_smc.h
> +++ b/include/hw/ssi/aspeed_smc.h
> @@ -52,8 +52,8 @@ struct AspeedSMCState {
>      SysBusDevice parent_obj;
>  
>      MemoryRegion mmio;
> +    MemoryRegion mmio_flash_container;
>      MemoryRegion mmio_flash;
> -    MemoryRegion mmio_flash_alias;
>  
>      qemu_irq irq;
>  
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 1770985230b0..d4f03881ddf5 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -1218,14 +1218,17 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
>       * window in which the flash modules are mapped. The size and
>       * address depends on the SoC model and controller type.
>       */
> +    memory_region_init(&s->mmio_flash_container, OBJECT(s),
> +                       TYPE_ASPEED_SMC ".container",
> +                       asc->flash_window_size);
> +    sysbus_init_mmio(sbd, &s->mmio_flash_container);
> +
>      memory_region_init_io(&s->mmio_flash, OBJECT(s),
>                            &aspeed_smc_flash_default_ops, s,
>                            TYPE_ASPEED_SMC ".flash",
>                            asc->flash_window_size);
> -    memory_region_init_alias(&s->mmio_flash_alias, OBJECT(s),
> -                             TYPE_ASPEED_SMC ".flash",
> -                             &s->mmio_flash, 0, asc->flash_window_size);
> -    sysbus_init_mmio(sbd, &s->mmio_flash_alias);
> +    memory_region_add_subregion(&s->mmio_flash_container, 0x0,
> +                                &s->mmio_flash);
>  
>      /*
>       * Let's create a sub memory region for each possible peripheral. All
> -- 
> 2.31.1
> 
> 

Re: [PATCH v2 0/5] aspeed/smc: Improve support for the alternate boot function

[Cédric Le Goater]

>> And the FMC registers are just an alias to write
>> to these watchdog 2 registers? 
> 
> If this is the same watchdog mapped into the FMC, I would say yes
> and the logic generate load/stores transactions on the AHB bus.
> Adding an address space for the WDT registers in the model is the
> closer we can get without implementing the bus protocol.
> 
>> Just curious, is it
>> strictly necessary to use the FMC registers to disable
>> the alternate boot watchdog, or could you just use the
>> old address, 0x1e78504C? 
> 
> Hey, this is something to try on HW and check how both register
> sets evolve. Would you have time ?

Andrew did some experiments in the past and the two register sets
were evolving independently.

>> In our OpenBMC initialization
>> for Fuji, we’re using the FMC registers, but would
>> it still work if we used the old addresses? Just curious,
>> the more I think about it, it seems odd to me that these
>> FMC watchdog registers exist if they’re just an alias.
> 
> We should ask the HW designers.

Aspeed tells me its an independent logic. So, I will drop the
model from this patchset.

Thanks,

C.

Re: [PATCH v2 0/5] aspeed/smc: Improve support for the alternate boot function

[Peter Delevoryas]

> On Oct 21, 2021, at 11:11 PM, Cédric Le Goater <clg@kaod.org> wrote:
> 
>>> And the FMC registers are just an alias to write
>>> to these watchdog 2 registers? 
>> If this is the same watchdog mapped into the FMC, I would say yes
>> and the logic generate load/stores transactions on the AHB bus.
>> Adding an address space for the WDT registers in the model is the
>> closer we can get without implementing the bus protocol.
>>> Just curious, is it
>>> strictly necessary to use the FMC registers to disable
>>> the alternate boot watchdog, or could you just use the
>>> old address, 0x1e78504C? 
>> Hey, this is something to try on HW and check how both register
>> sets evolve. Would you have time ?
> 
> Andrew did some experiments in the past and the two register sets
> were evolving independently.

I see, yeah I looked at some hardware today and haven’t finished the experiments,
but it did seem that way. Also asked some more knowledgeable people
on my team and they confirmed it was necessary to use FMC_WDT2.

> 
>>> In our OpenBMC initialization
>>> for Fuji, we’re using the FMC registers, but would
>>> it still work if we used the old addresses? Just curious,
>>> the more I think about it, it seems odd to me that these
>>> FMC watchdog registers exist if they’re just an alias.
>> We should ask the HW designers.
> 
> Aspeed tells me its an independent logic. So, I will drop the
> model from this patchset.
> 

I see! Ok, thanks for investigating that!

> Thanks,
> 
> C.
>