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

[Andrew Jeffery]

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_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;
>  }
>  
>  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):

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