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

["Cédric Le Goater" <clg@kaod.org>]

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.