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.