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 <p...@fb.com>
Signed-off-by: Cédric Le Goater <c...@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.