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?