Hi Philippe,
> Subject: Re: [PATCH v1 1/3] hw/timer/aspeed: Support different memory region
> ops
>
> Hi Jamin,
>
> On 16/12/24 08:53, Jamin Lin via 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;
>
> Simpler (and safer) to initialize a common field once, in the parent class,
> timer_class_init(). Otherwise,
>
Thanks for review and suggestion.
Will fix it.
Jamin
> Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
>
> > }
> >
> > 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;
> > }
