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

[Jamin Lin]

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_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;
> 
> 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é <philmd@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;
> >   }