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

[PATCH 0/4] 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.


Cédric Le Goater (4):
  aspeed/wdt: Add trace events
  aspeed/smc: Dump address offset in trace events
  aspeed/wdt: Add an alias for the MMIO region
  aspeed/smc: Improve support for the alternate boot function

 include/hw/ssi/aspeed_smc.h      |  3 ++
 include/hw/watchdog/wdt_aspeed.h |  1 +
 hw/arm/aspeed_ast2600.c          |  2 +
 hw/ssi/aspeed_smc.c              | 84 ++++++++++++++++++++++++++++++--
 hw/watchdog/wdt_aspeed.c         | 20 ++++++--
 hw/ssi/trace-events              |  1 +
 hw/watchdog/trace-events         |  4 ++
 7 files changed, 107 insertions(+), 8 deletions(-)

-- 
2.31.1

[PATCH 1/4] aspeed/wdt: Add trace events

[Cédric Le Goater]

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

diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index 69c37af9a6e9..146ffcd71301 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -19,6 +19,7 @@
 #include "hw/sysbus.h"
 #include "hw/watchdog/wdt_aspeed.h"
 #include "migration/vmstate.h"
+#include "trace.h"
 
 #define WDT_STATUS                      (0x00 / 4)
 #define WDT_RELOAD_VALUE                (0x04 / 4)
@@ -60,6 +61,8 @@ static uint64_t aspeed_wdt_read(void *opaque, hwaddr offset, unsigned size)
 {
     AspeedWDTState *s = ASPEED_WDT(opaque);
 
+    trace_aspeed_wdt_read(offset, size);
+
     offset >>= 2;
 
     switch (offset) {
@@ -140,6 +143,8 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
     AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(s);
     bool enable;
 
+    trace_aspeed_wdt_write(offset, size, data);
+
     offset >>= 2;
 
     switch (offset) {
diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
index c3bafbffa911..e7523e22aaf2 100644
--- a/hw/watchdog/trace-events
+++ b/hw/watchdog/trace-events
@@ -5,3 +5,7 @@ cmsdk_apb_watchdog_read(uint64_t offset, uint64_t data, unsigned size) "CMSDK AP
 cmsdk_apb_watchdog_write(uint64_t offset, uint64_t data, unsigned size) "CMSDK APB watchdog write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 cmsdk_apb_watchdog_reset(void) "CMSDK APB watchdog: reset"
 cmsdk_apb_watchdog_lock(uint32_t lock) "CMSDK APB watchdog: lock %" PRIu32
+
+# wdt-aspeed.c
+aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
+aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
-- 
2.31.1

[PATCH 2/4] aspeed/smc: Dump address offset in trace events

[Cédric Le Goater]

The register index is currently printed and this is confusing.

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

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 7129341c129e..8a988c167604 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -728,7 +728,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
          addr < R_SEG_ADDR0 + asc->max_peripherals) ||
         (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + asc->max_peripherals)) {
 
-        trace_aspeed_smc_read(addr, size, s->regs[addr]);
+        trace_aspeed_smc_read(addr << 2, size, s->regs[addr]);
 
         return s->regs[addr];
     } else {
@@ -1029,10 +1029,10 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
     AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
     uint32_t value = data;
 
-    addr >>= 2;
-
     trace_aspeed_smc_write(addr, size, data);
 
+    addr >>= 2;
+
     if (addr == s->r_conf ||
         (addr >= s->r_timings &&
          addr < s->r_timings + asc->nregs_timings) ||
-- 
2.31.1

[PATCH 3/4] aspeed/wdt: Add an alias for the MMIO region

[Cédric Le Goater]

Initialize the region in the instance_init handler because we will
want to link this region in the FMC object before the WDT object is
realized.

Cc: Peter Delevoryas <pdel@fb.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/watchdog/wdt_aspeed.h |  1 +
 hw/watchdog/wdt_aspeed.c         | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
index f945cd6c5833..008e7d9b498c 100644
--- a/include/hw/watchdog/wdt_aspeed.h
+++ b/include/hw/watchdog/wdt_aspeed.h
@@ -29,6 +29,7 @@ struct AspeedWDTState {
 
     /*< public >*/
     MemoryRegion iomem;
+    MemoryRegion iomem_alias;
     uint32_t regs[ASPEED_WDT_REGS_MAX];
 
     AspeedSCUState *scu;
diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index 146ffcd71301..6426f3a77494 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -261,6 +261,16 @@ static void aspeed_wdt_timer_expired(void *dev)
 
 #define PCLK_HZ 24000000
 
+static void aspeed_wdt_instance_init(Object *obj)
+{
+    AspeedWDTState *s = ASPEED_WDT(obj);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
+                          TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
+    memory_region_init_alias(&s->iomem_alias, OBJECT(s),
+                             TYPE_ASPEED_WDT ".alias",
+                             &s->iomem, 0, ASPEED_WDT_REGS_MAX * 4);
+}
 static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
@@ -275,9 +285,7 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
      */
     s->pclk_freq = PCLK_HZ;
 
-    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);
+    sysbus_init_mmio(sbd, &s->iomem_alias);
 }
 
 static Property aspeed_wdt_properties[] = {
@@ -301,6 +309,7 @@ static void aspeed_wdt_class_init(ObjectClass *klass, void *data)
 static const TypeInfo aspeed_wdt_info = {
     .parent = TYPE_SYS_BUS_DEVICE,
     .name  = TYPE_ASPEED_WDT,
+    .instance_init  = aspeed_wdt_instance_init,
     .instance_size  = sizeof(AspeedWDTState),
     .class_init = aspeed_wdt_class_init,
     .class_size    = sizeof(AspeedWDTClass),
-- 
2.31.1

[PATCH 4/4] 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 0384357a9510..2c53d77899a8 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -353,6 +353,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

Re: [PATCH 2/4] aspeed/smc: Dump address offset in trace events

[Francisco Iglesias]

On [2021 Oct 04] Mon 17:46:33, Cédric Le Goater wrote:
> The register index is currently printed and this is confusing.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

> ---
>  hw/ssi/aspeed_smc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 7129341c129e..8a988c167604 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -728,7 +728,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
>           addr < R_SEG_ADDR0 + asc->max_peripherals) ||
>          (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + asc->max_peripherals)) {
>  
> -        trace_aspeed_smc_read(addr, size, s->regs[addr]);
> +        trace_aspeed_smc_read(addr << 2, size, s->regs[addr]);
>  
>          return s->regs[addr];
>      } else {
> @@ -1029,10 +1029,10 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
>      AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
>      uint32_t value = data;
>  
> -    addr >>= 2;
> -
>      trace_aspeed_smc_write(addr, size, data);
>  
> +    addr >>= 2;
> +
>      if (addr == s->r_conf ||
>          (addr >= s->r_timings &&
>           addr < s->r_timings + asc->nregs_timings) ||
> -- 
> 2.31.1
> 
> 

Re: [PATCH 1/4] aspeed/wdt: Add trace events

[Francisco Iglesias]

On [2021 Oct 04] Mon 17:46:32, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

> ---
>  hw/watchdog/wdt_aspeed.c | 5 +++++
>  hw/watchdog/trace-events | 4 ++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index 69c37af9a6e9..146ffcd71301 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -19,6 +19,7 @@
>  #include "hw/sysbus.h"
>  #include "hw/watchdog/wdt_aspeed.h"
>  #include "migration/vmstate.h"
> +#include "trace.h"
>  
>  #define WDT_STATUS                      (0x00 / 4)
>  #define WDT_RELOAD_VALUE                (0x04 / 4)
> @@ -60,6 +61,8 @@ static uint64_t aspeed_wdt_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      AspeedWDTState *s = ASPEED_WDT(opaque);
>  
> +    trace_aspeed_wdt_read(offset, size);
> +
>      offset >>= 2;
>  
>      switch (offset) {
> @@ -140,6 +143,8 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
>      AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(s);
>      bool enable;
>  
> +    trace_aspeed_wdt_write(offset, size, data);
> +
>      offset >>= 2;
>  
>      switch (offset) {
> diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
> index c3bafbffa911..e7523e22aaf2 100644
> --- a/hw/watchdog/trace-events
> +++ b/hw/watchdog/trace-events
> @@ -5,3 +5,7 @@ cmsdk_apb_watchdog_read(uint64_t offset, uint64_t data, unsigned size) "CMSDK AP
>  cmsdk_apb_watchdog_write(uint64_t offset, uint64_t data, unsigned size) "CMSDK APB watchdog write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
>  cmsdk_apb_watchdog_reset(void) "CMSDK APB watchdog: reset"
>  cmsdk_apb_watchdog_lock(uint32_t lock) "CMSDK APB watchdog: lock %" PRIu32
> +
> +# wdt-aspeed.c
> +aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> +aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
> -- 
> 2.31.1
> 
> 

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

[Cédric Le Goater]

On 10/4/21 17:46, Cédric Le Goater 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
>      
> Thanks
> 
> C.
> 
> 
> Cédric Le Goater (4):
>    aspeed/wdt: Add trace events
>    aspeed/smc: Dump address offset in trace events
>    aspeed/wdt: Add an alias for the MMIO region
>    aspeed/smc: Improve support for the alternate boot function


Andrew, Peter D., Joel,

Would you have time to tell me what you think about the last 2 patches ?
It would be a nice extension for the Fuji in QEMU 6.2.

Here are some images for tests.

   https://github.com/peterdelevoryas/openbmc/releases/download/fuji-v0.1-alpha/fuji.mtd

This one is recent :

   https://github.com/peterdelevoryas/openbmc/releases/download/fuji.mtd.0/fuji.mtd


   U-Boot 2019.04 fuji-bd6ee58668 (Sep 13 2021 - 21:29:46 +0000)
   [    0.000000] Linux version 5.10.23-fuji (oe-user@oe-host) (arm-fb-linux-gnueabi-gcc (GCC) 9.3.0, GNU ld (GNU Binutils) 2.34.0.20200220) #1 SMP Thu Sep 9 23:22:29 UTC 2021


Thanks,

C.


> 
>   include/hw/ssi/aspeed_smc.h      |  3 ++
>   include/hw/watchdog/wdt_aspeed.h |  1 +
>   hw/arm/aspeed_ast2600.c          |  2 +
>   hw/ssi/aspeed_smc.c              | 84 ++++++++++++++++++++++++++++++--
>   hw/watchdog/wdt_aspeed.c         | 20 ++++++--
>   hw/ssi/trace-events              |  1 +
>   hw/watchdog/trace-events         |  4 ++
>   7 files changed, 107 insertions(+), 8 deletions(-)
> 

Re: [PATCH 1/4] aspeed/wdt: Add trace events

[Philippe Mathieu-Daudé]

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

> +# wdt-aspeed.c
> +aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> +aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64

"size=%u", otherwise:

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

Re: [PATCH 2/4] aspeed/smc: Dump address offset in trace events

[Philippe Mathieu-Daudé]

On 10/4/21 17:46, Cédric Le Goater wrote:
> The register index is currently printed and this is confusing.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ssi/aspeed_smc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

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

Re: [PATCH 3/4] aspeed/wdt: Add an alias for the MMIO region

[Philippe Mathieu-Daudé]

Hi Cédric,

On 10/4/21 17:46, Cédric Le Goater wrote:
> Initialize the region in the instance_init handler because we will
> want to link this region in the FMC object before the WDT object is
> realized.
> 
> Cc: Peter Delevoryas <pdel@fb.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/watchdog/wdt_aspeed.h |  1 +
>  hw/watchdog/wdt_aspeed.c         | 15 ++++++++++++---
>  2 files changed, 13 insertions(+), 3 deletions(-)

> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index 146ffcd71301..6426f3a77494 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -261,6 +261,16 @@ static void aspeed_wdt_timer_expired(void *dev)
>  
>  #define PCLK_HZ 24000000
>  
> +static void aspeed_wdt_instance_init(Object *obj)
> +{
> +    AspeedWDTState *s = ASPEED_WDT(obj);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
> +                          TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
> +    memory_region_init_alias(&s->iomem_alias, OBJECT(s),
> +                             TYPE_ASPEED_WDT ".alias",
> +                             &s->iomem, 0, ASPEED_WDT_REGS_MAX * 4);
> +}
>  static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
>  {
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> @@ -275,9 +285,7 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
>       */
>      s->pclk_freq = PCLK_HZ;
>  
> -    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);
> +    sysbus_init_mmio(sbd, &s->iomem_alias);

Exposing an alias that way seems odd. Don't you want to use/expose
a container instead?

Anyhow, by moving memory_region_init_io() in init(), the region is
available for linking before realize(), so why do you need an alias?

Re: [PATCH 3/4] aspeed/wdt: Add an alias for the MMIO region

[Cédric Le Goater]

Hello Phil,

On 10/18/21 11:04, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
> 
> On 10/4/21 17:46, Cédric Le Goater wrote:
>> Initialize the region in the instance_init handler because we will
>> want to link this region in the FMC object before the WDT object is
>> realized.
>>
>> Cc: Peter Delevoryas <pdel@fb.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   include/hw/watchdog/wdt_aspeed.h |  1 +
>>   hw/watchdog/wdt_aspeed.c         | 15 ++++++++++++---
>>   2 files changed, 13 insertions(+), 3 deletions(-)
> 
>> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
>> index 146ffcd71301..6426f3a77494 100644
>> --- a/hw/watchdog/wdt_aspeed.c
>> +++ b/hw/watchdog/wdt_aspeed.c
>> @@ -261,6 +261,16 @@ static void aspeed_wdt_timer_expired(void *dev)
>>   
>>   #define PCLK_HZ 24000000
>>   
>> +static void aspeed_wdt_instance_init(Object *obj)
>> +{
>> +    AspeedWDTState *s = ASPEED_WDT(obj);
>> +
>> +    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
>> +                          TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
>> +    memory_region_init_alias(&s->iomem_alias, OBJECT(s),
>> +                             TYPE_ASPEED_WDT ".alias",
>> +                             &s->iomem, 0, ASPEED_WDT_REGS_MAX * 4);
>> +}
>>   static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
>>   {
>>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> @@ -275,9 +285,7 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
>>        */
>>       s->pclk_freq = PCLK_HZ;
>>   
>> -    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);
>> +    sysbus_init_mmio(sbd, &s->iomem_alias);
> 
> Exposing an alias that way seems odd. Don't you want to use/expose
> a container instead?

I think I got confused by changes in e9c568dbc225 ("hw/arm/aspeed:
Do not sysbus-map mmio flash region directly, use alias") :)

You are right. This needs a container, not an alias. I will respin
and fix the aspeed-smc flash address space also.

> Anyhow, by moving memory_region_init_io() in init(), the region is
> available for linking before realize(), so why do you need an alias?

yes.

I still need to initialize the watchdog models before the FMC. There
will be a little change in the order.

Thanks,

C.