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).
Once I have completed that task, I will revise the timer model with your
suggestion.
I will update you later.
Thanks again for your input.
Jamin
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c index
> 4868651ad489..c7486af4ced2 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -239,9 +239,8 @@ static uint64_t aspeed_timer_get_value(AspeedTimer
> *t, int reg)
> return value;
> }
>
> -static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned
> size)
> +static uint64_t aspeed_timer_read_generic(AspeedTimerCtrlState *s,
> +hwaddr offset)
> {
> - AspeedTimerCtrlState *s = opaque;
> const int reg = (offset & 0xf) / 4;
> uint64_t value;
>
> @@ -256,13 +255,20 @@ static uint64_t aspeed_timer_read(void *opaque,
> hwaddr offset, unsigned size)
> value = aspeed_timer_get_value(&s->timers[(offset >> 4) - 1], reg);
> break;
> default:
> - value = ASPEED_TIMER_GET_CLASS(s)->read(s, offset);
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%"
> HWADDR_PRIx "\n",
> + __func__, offset);
> + value = 0;
> break;
> }
> - trace_aspeed_timer_read(offset, size, value);
> return value;
> }
>
> +static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned
> +size) {
> + AspeedTimerCtrlState *s = opaque;
> + return ASPEED_TIMER_GET_CLASS(s)->read(s, offset, size); }
> +
> static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int
> reg,
> uint32_t value) { @@ -431,12
> +437,11 @@ static void aspeed_timer_set_ctrl2(AspeedTimerCtrlState *s,
> uint32_t value)
> trace_aspeed_timer_set_ctrl2(value);
> }
>
> -static void aspeed_timer_write(void *opaque, hwaddr offset, uint64_t value,
> - unsigned size)
> +static void aspeed_timer_write_generic(AspeedTimerCtrlState *s, hwaddr
> offset,
> + uint64_t value)
> {
> const uint32_t tv = (uint32_t)(value & 0xFFFFFFFF);
> const int reg = (offset & 0xf) / 4;
> - AspeedTimerCtrlState *s = opaque;
>
> switch (offset) {
> /* Control Registers */
> @@ -451,11 +456,20 @@ static void aspeed_timer_write(void *opaque,
> hwaddr offset, uint64_t value,
> aspeed_timer_set_value(s, (offset >> TIMER_NR_REGS) - 1, reg, tv);
> break;
> default:
> - ASPEED_TIMER_GET_CLASS(s)->write(s, offset, value);
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%"
> HWADDR_PRIx "\n",
> + __func__, offset);
> + value = 0;
> break;
> }
> }
>
> +static void aspeed_timer_write(void *opaque, hwaddr offset, uint64_t value,
> + unsigned size) {
> + AspeedTimerCtrlState *s = opaque;
> + ASPEED_TIMER_GET_CLASS(s)->write(s, offset, value); }
> +
> static const MemoryRegionOps aspeed_timer_ops = {
> .read = aspeed_timer_read,
> .write = aspeed_timer_write,
> @@ -465,7 +479,7 @@ static const MemoryRegionOps aspeed_timer_ops = {
> .valid.unaligned = false,
> };
>
> -static uint64_t aspeed_2400_timer_read(AspeedTimerCtrlState *s, hwaddr
> offset)
> +static uint64_t aspeed_2400_timer_read(AspeedTimerCtrlState *s, hwaddr
> +offset, unsigned size)
> {
> uint64_t value;
>
> @@ -475,17 +489,18 @@ static uint64_t
> aspeed_2400_timer_read(AspeedTimerCtrlState *s, hwaddr offset)
> break;
> case 0x38:
> case 0x3C:
> - default:
> qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%"
> HWADDR_PRIx "\n",
> __func__, offset);
> value = 0;
> break;
> + default:
> + return aspeed_timer_read_generic(s, offset);
> }
> + trace_aspeed_timer_read(offset, size, value);
> return value;
> }
>
> -static void aspeed_2400_timer_write(AspeedTimerCtrlState *s, hwaddr
> offset,
> - uint64_t value)
> +static void aspeed_2400_timer_write(AspeedTimerCtrlState *s, hwaddr
> +offset, uint64_t value)
> {
> const uint32_t tv = (uint32_t)(value & 0xFFFFFFFF);
>
> @@ -495,10 +510,12 @@ static void
> aspeed_2400_timer_write(AspeedTimerCtrlState *s, hwaddr offset,
> break;
> case 0x38:
> case 0x3C:
> - default:
> qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%"
> HWADDR_PRIx "\n",
> __func__, offset);
> break;
> + default:
> + aspeed_timer_write_generic(s, offset, value);
> + break;
> }
> }
>
> diff --git a/include/hw/timer/aspeed_timer.h
> b/include/hw/timer/aspeed_timer.h index 07dc6b6f2cbd..540b23656815
> 100644
> --- a/include/hw/timer/aspeed_timer.h
> +++ b/include/hw/timer/aspeed_timer.h
> @@ -71,7 +71,7 @@ struct AspeedTimerCtrlState { struct AspeedTimerClass
> {
> SysBusDeviceClass parent_class;
>
> - uint64_t (*read)(AspeedTimerCtrlState *s, hwaddr offset);
> + uint64_t (*read)(AspeedTimerCtrlState *s, hwaddr offset, unsigned
> + size);
> void (*write)(AspeedTimerCtrlState *s, hwaddr offset, uint64_t
> value); };
>
