Hi Cedric,
> -----Original Message-----
> From: Cédric Le Goater <c...@kaod.org>
> Sent: Thursday, January 9, 2025 3:02 PM
> To: Jamin Lin <jamin_...@aspeedtech.com>; Andrew Jeffery
> <and...@codeconstruct.com.au>; Peter Maydell <peter.mayd...@linaro.org>;
> Steven Lee <steven_...@aspeedtech.com>; Troy Lee <leet...@gmail.com>;
> Joel Stanley <j...@jms.id.au>; open list:ASPEED BMCs
> <qemu-...@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_...@aspeedtech.com>; Yunlin Tang
> <yunlin.t...@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 <and...@codeconstruct.com.au>
> >> Sent: Thursday, January 9, 2025 9:59 AM
> >> To: Jamin Lin <jamin_...@aspeedtech.com>; Cédric Le Goater
> >> <c...@kaod.org>; Peter Maydell <peter.mayd...@linaro.org>; Steven Lee
> >> <steven_...@aspeedtech.com>; Troy Lee <leet...@gmail.com>; Joel
> >> Stanley <j...@jms.id.au>; open list:ASPEED BMCs
> >> <qemu-...@nongnu.org>; open list:All patches CC here
> >> <qemu-devel@nongnu.org>
> >> Cc: Troy Lee <troy_...@aspeedtech.com>; Yunlin Tang
> >> <yunlin.t...@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_...@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.
>
