RE: [PATCH v1 1/3] hw/timer/aspeed: Support different memory region ops

[Jamin Lin]

Hi Cedric,

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Thursday, January 9, 2025 3:02 PM
> To: Jamin Lin <jamin_lin@aspeedtech.com>; Andrew Jeffery
> <andrew@codeconstruct.com.au>; Peter Maydell <peter.maydell@linaro.org>;
> Steven Lee <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>;
> Joel Stanley <joel@jms.id.au>; open list:ASPEED BMCs
> <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang
> <yunlin.tang@aspeedtech.com>
> Subject: Re: [PATCH v1 1/3] hw/timer/aspeed: Support different memory region
> ops
> 
> On 1/9/25 03:26, Jamin Lin wrote:
> > Hi Andrew,
> >
> >> From: Andrew Jeffery <andrew@codeconstruct.com.au>
> >> Sent: Thursday, January 9, 2025 9:59 AM
> >> To: Jamin Lin <jamin_lin@aspeedtech.com>; Cédric Le Goater
> >> <clg@kaod.org>; Peter Maydell <peter.maydell@linaro.org>; Steven Lee
> >> <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Joel
> >> Stanley <joel@jms.id.au>; open list:ASPEED BMCs
> >> <qemu-arm@nongnu.org>; open list:All patches CC here
> >> <qemu-devel@nongnu.org>
> >> Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang
> >> <yunlin.tang@aspeedtech.com>
> >> Subject: Re: [PATCH v1 1/3] hw/timer/aspeed: Support different memory
> >> region ops
> >>
> >> On Mon, 2024-12-16 at 15:53 +0800, Jamin Lin wrote:
> >>> It set "aspeed_timer_ops" struct which containing read and write
> >>> callbacks to be used when I/O is performed on the TIMER region.
> >>>
> >>> Besides, in the previous design of ASPEED SOCs, the timer registers
> >>> address space are contiguous.
> >>>
> >>> ex: TMC00-TMC0C are used for TIMER0.
> >>> ex: TMC10-TMC1C are used for TIMER1.
> >>> ex: TMC80-TMC8C are used for TIMER7.
> >>>
> >>> The TMC30 is a control register and TMC34 is an interrupt status
> >>> register for TIMER0-TIMER7.
> >>>
> >>> However, the register set have a significant change in AST2700. The
> >>> TMC00-TMC3C are used for TIMER0 and TMC40-TMC7C are used for
> >> TIMER1.
> >>> In additional, TMC20-TMC3C and TMC60-TMC7C are reserved registers
> >>> for
> >>> TIMER0 and TIMER1, respectively.
> >>>
> >>> Besides, each TIMER has their own control and interrupt status
> >>> register.
> >>> In other words, users are able to set control and interrupt status
> >>> for
> >>> TIMER0 in one register. Both aspeed_timer_read and
> >>> aspeed_timer_write callback functions are not compatible AST2700.
> >>>
> >>> Introduce a new "const MemoryRegionOps *" attribute in
> >>> AspeedTIMERClass and use it in aspeed_timer_realize function.
> >>>
> >>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> >>> ---
> >>>   hw/timer/aspeed_timer.c         | 7 ++++++-
> >>>   include/hw/timer/aspeed_timer.h | 1 +
> >>>   2 files changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c index
> >>> 149f7cc5a6..970bf1d79d 100644
> >>> --- a/hw/timer/aspeed_timer.c
> >>> +++ b/hw/timer/aspeed_timer.c
> >>> @@ -606,6 +606,7 @@ static void aspeed_timer_realize(DeviceState
> >>> *dev, Error **errp)
> >>>       int i;
> >>>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >>>       AspeedTimerCtrlState *s = ASPEED_TIMER(dev);
> >>> +    AspeedTimerClass *atc = ASPEED_TIMER_GET_CLASS(s);
> >>>
> >>>       assert(s->scu);
> >>>
> >>> @@ -613,7 +614,7 @@ static void aspeed_timer_realize(DeviceState
> >>> *dev, Error **errp)
> >>>           aspeed_init_one_timer(s, i);
> >>>           sysbus_init_irq(sbd, &s->timers[i].irq);
> >>>       }
> >>> -    memory_region_init_io(&s->iomem, OBJECT(s),
> &aspeed_timer_ops,
> >> s,
> >>> +    memory_region_init_io(&s->iomem, OBJECT(s), atc->reg_ops, s,
> >>>                             TYPE_ASPEED_TIMER, 0x1000);
> >>
> >>
> >>>       sysbus_init_mmio(sbd, &s->iomem);
> >>>   }
> >>> @@ -708,6 +709,7 @@ static void
> >>> aspeed_2400_timer_class_init(ObjectClass *klass, void *data)
> >>>       dc->desc = "ASPEED 2400 Timer";
> >>>       awc->read = aspeed_2400_timer_read;
> >>>       awc->write = aspeed_2400_timer_write;
> >>> +    awc->reg_ops = &aspeed_timer_ops;
> >>>   }
> >>>
> >>>   static const TypeInfo aspeed_2400_timer_info = { @@ -724,6 +726,7
> >>> @@ static void aspeed_2500_timer_class_init(ObjectClass *klass, void
> >>> *data)
> >>>       dc->desc = "ASPEED 2500 Timer";
> >>>       awc->read = aspeed_2500_timer_read;
> >>>       awc->write = aspeed_2500_timer_write;
> >>> +    awc->reg_ops = &aspeed_timer_ops;
> >>>   }
> >>>
> >>>   static const TypeInfo aspeed_2500_timer_info = { @@ -740,6 +743,7
> >>> @@ static void aspeed_2600_timer_class_init(ObjectClass *klass, void
> >>> *data)
> >>>       dc->desc = "ASPEED 2600 Timer";
> >>>       awc->read = aspeed_2600_timer_read;
> >>>       awc->write = aspeed_2600_timer_write;
> >>> +    awc->reg_ops = &aspeed_timer_ops;
> >>>   }
> >>>
> >>>   static const TypeInfo aspeed_2600_timer_info = { @@ -756,6 +760,7
> >>> @@ static void aspeed_1030_timer_class_init(ObjectClass *klass, void
> >>> *data)
> >>>       dc->desc = "ASPEED 1030 Timer";
> >>>       awc->read = aspeed_2600_timer_read;
> >>>       awc->write = aspeed_2600_timer_write;
> >>> +    awc->reg_ops = &aspeed_timer_ops;
> >>>   }
> >>>
> >>>   static const TypeInfo aspeed_1030_timer_info = { diff --git
> >>> a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h
> >>> index 07dc6b6f2c..8d0b15f055 100644
> >>> --- a/include/hw/timer/aspeed_timer.h
> >>> +++ b/include/hw/timer/aspeed_timer.h
> >>> @@ -73,6 +73,7 @@ struct AspeedTimerClass {
> >>>
> >>>       uint64_t (*read)(AspeedTimerCtrlState *s, hwaddr offset);
> >>>       void (*write)(AspeedTimerCtrlState *s, hwaddr offset, uint64_t
> >>> value);
> >>> +    const MemoryRegionOps *reg_ops;
> >>
> >> So given the layout changes for the AST2700, perhaps we can improve
> >> the way we've organised the call delegation?
> >>
> >> Currently the callbacks in `aspeed_timer_ops` are generic
> >> (aspeed_timer_read(), aspeed_timer_write()), and then we specialise
> >> some bits in the default label of the switch statement by delegating
> >> to the SoC-specific callbacks.
> >>
> >> Perhaps we should instead call through the SoC-specific callbacks
> >> first, and then have those call the generic op implementation for
> >> accesses to registers have common behaviours across the AST2[456]00 SoCs.
> >>
> >> With that perspective, the change in layout for the AST2700 is
> >> effectively a specialisation for all the registers. Later, if there's
> >> some tinkering with the timer registers for a hypothetical AST2800,
> >> we can follow the same strategy by extracting out the common
> >> behaviours for the AST2700 and AST2800, and invoke them through the
> default label.
> >>
> >> As a quick PoC to demonstrate my line of thinking (not compiled, not
> >> tested, only converts AST2400):
> >>
> > Thank you for your review and suggestion.
> > Currently, I am working on QEMU to support the "AST2700 A1" boot(I should
> refactor INTC model).
> 
> Is that the reason why the QEMU ast2700-evb machine doesn't boot with the
> v09.04 SDK images ?
> 
Yes, "ast2700-default-obmc.tar.gz" is used for AST2700 A1.
The design between the AST2700 A0 and A1 is different, especially the INTC 
controllers. 
I am refactoring the INTC model to enable the AST2700 A1 to boot into the OS.
If you want to test SDK v09.04, please use "ast2700-a0-default-obmc.tar.gz".
I estimate to send the v1 patch to support A1 in the end of this month before 
the Chinese New Year.

> > Once I have completed that task, I will revise the timer model with your
> suggestion.
> 
> Please replace suffix '_generic' by '_common' when you do.

Got it.
Thanks for suggestion.

Jamin
> 
> 
> 
> Thanks,
> 
> C.
>