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

[Philippe Mathieu-Daudé]

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,

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;
>   }