[PATCH v1 0/3] Support timer for AST2700

[PATCH v1 0/3] Support timer for AST2700

[Jamin Lin via]

v1:
  - Support timer for AST2700
  - Introduce new "aspeed_2700_timer_read" and "aspeed_2700_timer_write"
    callback functions and "aspeed_2700_timer_ops" memory
    region operation for AST2700.
    Introduce a new ast2700 class to support AST2700.

Jamin Lin (3):
  hw/timer/aspeed: Support different memory region ops
  hw/timer/aspeed: Add AST2700 Support
  aspeed/soc: Support Timer for AST2700

 hw/arm/aspeed_ast27x0.c         |  17 +++
 hw/timer/aspeed_timer.c         | 231 +++++++++++++++++++++++++++++++-
 include/hw/timer/aspeed_timer.h |   2 +
 3 files changed, 249 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v1 0/3] Support timer for AST2700

[Jamin Lin via]

v1:
  - Support timer for AST2700
  - Introduce new "aspeed_2700_timer_read" and "aspeed_2700_timer_write"
    callback functions and "aspeed_2700_timer_ops" memory
    region operation for AST2700.
    Introduce a new ast2700 class to support AST2700.

Jamin Lin (3):
  hw/timer/aspeed: Support different memory region ops
  hw/timer/aspeed: Add AST2700 Support
  aspeed/soc: Support Timer for AST2700

 hw/arm/aspeed_ast27x0.c         |  17 +++
 hw/timer/aspeed_timer.c         | 231 +++++++++++++++++++++++++++++++-
 include/hw/timer/aspeed_timer.h |   2 +
 3 files changed, 249 insertions(+), 1 deletion(-)

-- 
2.25.1

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

[Jamin Lin via]

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;
 };
 
 #endif /* ASPEED_TIMER_H */
-- 
2.25.1

[PATCH v1 2/3] hw/timer/aspeed: Add AST2700 Support

[Jamin Lin via]

The timer controller include 8 sets of 32-bit decrement counters, based on
either PCLK or 1MHZ clock and the design of timer controller between AST2600
and AST2700 are almost the same.

The different is that the register set have a significant change in AST2700.
TIMER0 – TIMER7 has their own individua control and interrupt status register.
In other words, users are able to set timer control in register TMC10 with
different TIMER base address and clear timer control and interrupt status in
register TMC14 with different TIMER base address.

Both "aspeed_timer_read" and "aspeed_timer_write" callback functions are not
compatible AST2700. Introduce new "aspeed_2700_timer_read" and
"aspeed_2700_timer_write" callback functions and "aspeed_2700_timer_ops" memory
region operation for AST2700. Introduce a new ast2700 class to support AST2700.

The base address of TIMER0 to TIMER7 as following.
Base Address of Timer 0 = 0x12C1_0000
Base Address of Timer 1 = 0x12C1_0040
Base Address of Timer 2 = 0x12C1_0080
Base Address of Timer 3 = 0x12C1_00C0
Base Address of Timer 4 = 0x12C1_0100
Base Address of Timer 5 = 0x12C1_0140
Base Address of Timer 6 = 0x12C1_0180
Base Address of Timer 7 = 0x12C1_01C0

The register address space of each TIMER is "0x40" , so uses the following
formula to get the index and register of each TIMER.

timer_index = offset >> 6;
timer_offset = offset & 0x3f;

The TMC010 is a counter control set and interrupt status register. Write "1" to
TMC10[3:0] will set the specific bits to "1". Introduce a new
"aspeed_2700_timer_set_ctrl" function to handle this register behavior.

The TMC014 is a counter control clear and interrupt status register, to clear
the specific bits to "0", it should write "1" to  TMC14[3:0] on the same bit
position. Introduce a new "aspeed_2700_timer_clear_ctrl" function to handle
this register behavior. TMC014 does not support read operation.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/timer/aspeed_timer.c         | 224 ++++++++++++++++++++++++++++++++
 include/hw/timer/aspeed_timer.h |   1 +
 2 files changed, 225 insertions(+)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 970bf1d79d..de4d78a0fb 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -593,6 +593,214 @@ static void aspeed_2600_timer_write(AspeedTimerCtrlState *s, hwaddr offset,
     }
 }
 
+static void aspeed_2700_timer_set_ctrl(AspeedTimerCtrlState *s, int index,
+                                    uint32_t reg)
+{
+    const uint8_t overflow_interrupt_mask = BIT(op_overflow_interrupt);
+    const uint8_t external_clock_mask = BIT(op_external_clock);
+    const uint8_t pulse_enable_mask = BIT(op_pulse_enable);
+    const uint8_t enable_mask = BIT(op_enable);
+    AspeedTimer *t;
+    uint8_t t_old;
+    uint8_t t_new;
+    int shift;
+
+    /*
+     * Only 1 will set the specific bits to 1
+     * Handle a dependency between the 'enable' and remaining three
+     * configuration bits - i.e. if more than one bit in the control set has
+     * set, including the 'enable' bit, perform configuration and then
+     * enable the timer.
+     * Interrupt Status bit should not be set.
+     */
+
+     t = &s->timers[index];
+     shift = index * TIMER_CTRL_BITS;
+
+     t_old = (s->ctrl >> shift) & TIMER_CTRL_MASK;
+     t_new = reg & TIMER_CTRL_MASK;
+
+    if (!(t_old & external_clock_mask) &&
+        (t_new & external_clock_mask)) {
+        aspeed_timer_ctrl_external_clock(t, true);
+        s->ctrl = deposit32(s->ctrl, shift + op_external_clock, 1, 1);
+    }
+
+    if (!(t_old & overflow_interrupt_mask) &&
+        (t_new & overflow_interrupt_mask)) {
+        aspeed_timer_ctrl_overflow_interrupt(t, true);
+        s->ctrl = deposit32(s->ctrl, shift + op_overflow_interrupt, 1, 1);
+    }
+
+
+    if (!(t_old & pulse_enable_mask) &&
+        (t_new & pulse_enable_mask)) {
+        aspeed_timer_ctrl_pulse_enable(t, true);
+        s->ctrl = deposit32(s->ctrl, shift + op_pulse_enable, 1, 1);
+    }
+
+    /* If we are enabling, do so last */
+    if (!(t_old & enable_mask) &&
+        (t_new & enable_mask)) {
+        aspeed_timer_ctrl_enable(t, true);
+        s->ctrl = deposit32(s->ctrl, shift + op_enable, 1, 1);
+    }
+}
+
+static void aspeed_2700_timer_clear_ctrl(AspeedTimerCtrlState *s, int index,
+                                    uint32_t reg)
+{
+    const uint8_t overflow_interrupt_mask = BIT(op_overflow_interrupt);
+    const uint8_t external_clock_mask = BIT(op_external_clock);
+    const uint8_t pulse_enable_mask = BIT(op_pulse_enable);
+    const uint8_t enable_mask = BIT(op_enable);
+    AspeedTimer *t;
+    uint8_t t_old;
+    uint8_t t_new;
+    int shift;
+
+    /*
+     * Only 1 will clear the specific bits to 0
+     * Handle a dependency between the 'enable' and remaining three
+     * configuration bits - i.e. if more than one bit in the control set has
+     * clear, including the 'enable' bit, then disable the timer and perform
+     * configuration
+     */
+
+     t = &s->timers[index];
+     shift = index * TIMER_CTRL_BITS;
+
+     t_old = (s->ctrl >> shift) & TIMER_CTRL_MASK;
+     t_new = reg & TIMER_CTRL_MASK;
+
+    /* If we are disabling, do so first */
+    if ((t_old & enable_mask) &&
+        (t_new & enable_mask)) {
+        aspeed_timer_ctrl_enable(t, false);
+        s->ctrl = deposit32(s->ctrl, shift + op_enable, 1, 0);
+    }
+
+    if ((t_old & external_clock_mask) &&
+        (t_new & external_clock_mask)) {
+        aspeed_timer_ctrl_external_clock(t, false);
+        s->ctrl = deposit32(s->ctrl, shift + op_external_clock, 1, 0);
+    }
+
+    if ((t_old & overflow_interrupt_mask) &&
+        (t_new & overflow_interrupt_mask)) {
+        aspeed_timer_ctrl_overflow_interrupt(t, false);
+        s->ctrl = deposit32(s->ctrl, shift + op_overflow_interrupt, 1, 0);
+    }
+
+    if ((t_old & pulse_enable_mask) &&
+        (t_new & pulse_enable_mask)) {
+        aspeed_timer_ctrl_pulse_enable(t, false);
+        s->ctrl = deposit32(s->ctrl, shift + op_pulse_enable, 1, 0);
+    }
+
+    if ((t_old & pulse_enable_mask) &&
+        (t_new & pulse_enable_mask)) {
+        aspeed_timer_ctrl_pulse_enable(t, false);
+        s->ctrl = deposit32(s->ctrl, shift + op_pulse_enable, 1, 0);
+    }
+
+    /* Clear interrupt status */
+    if (reg & 0x10000) {
+        s->irq_sts = deposit32(s->irq_sts, index, 1, 0);
+    }
+}
+
+static uint64_t aspeed_2700_timer_read(void *opaque, hwaddr offset,
+                                    uint32_t size)
+{
+    AspeedTimerCtrlState *s = ASPEED_TIMER(opaque);
+    uint32_t timer_offset = offset & 0x3f;
+    int timer_index = offset >> 6;
+    uint64_t value = 0;
+
+    if (timer_index >= ASPEED_TIMER_NR_TIMERS) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: offset 0x%" PRIx64 " out of bounds\n",
+                      __func__, offset);
+        return 0;
+    }
+
+    switch (timer_offset) {
+    /*
+     * Counter Status
+     * Counter Reload
+     * Counter First Matching
+     * Counter Second Matching
+     */
+    case 0x00 ... 0x0C:
+        value = aspeed_timer_get_value(&s->timers[timer_index],
+                                       timer_offset >> 2);
+        break;
+    /* Counter Control and Interrupt Status */
+    case 0x10:
+        value = deposit64(value, 0, 4,
+                          extract32(s->ctrl, timer_index * 4, 4));
+        value = deposit64(value, 16, 1,
+                          extract32(s->irq_sts, timer_index, 1));
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: no getter for offset 0x%"
+                     PRIx64"\n", __func__, offset);
+        return 0;
+    }
+    trace_aspeed_timer_read(offset, size, value);
+    return value;
+}
+
+static void aspeed_2700_timer_write(void *opaque, hwaddr offset,
+                                uint64_t value, uint32_t size)
+{
+    const uint32_t timer_value = (uint32_t)(value & 0xFFFFFFFF);
+    AspeedTimerCtrlState *s = ASPEED_TIMER(opaque);
+    uint32_t timer_offset = offset & 0x3f;
+    int timer_index = offset >> 6;
+
+    if (timer_index >= ASPEED_TIMER_NR_TIMERS) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: offset 0x%" PRIx64 " out of bounds\n",
+                      __func__, offset);
+    }
+
+    switch (timer_offset) {
+    /*
+     * Counter Status
+     * Counter Reload
+     * Counter First Matching
+     * Counter Second Matching
+     */
+    case 0x00 ... 0x0C:
+        aspeed_timer_set_value(s, timer_index, timer_offset >> 2,
+                               timer_value);
+        break;
+    /* Counter Control Set and Interrupt Status */
+    case 0x10:
+        aspeed_2700_timer_set_ctrl(s, timer_index, timer_value);
+        break;
+    /* Counter Control Clear and Interrupr Status */
+    case 0x14:
+        aspeed_2700_timer_clear_ctrl(s, timer_index, timer_value);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: no setter for offset 0x%"
+                      PRIx64"\n", __func__, offset);
+        break;
+    }
+}
+
+static const MemoryRegionOps aspeed_2700_timer_ops = {
+    .read = aspeed_2700_timer_read,
+    .write = aspeed_2700_timer_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
+    .valid.unaligned = false,
+};
+
 static void aspeed_init_one_timer(AspeedTimerCtrlState *s, uint8_t id)
 {
     AspeedTimer *t = &s->timers[id];
@@ -769,6 +977,21 @@ static const TypeInfo aspeed_1030_timer_info = {
     .class_init = aspeed_1030_timer_class_init,
 };
 
+static void aspeed_2700_timer_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedTimerClass *awc = ASPEED_TIMER_CLASS(klass);
+
+    dc->desc = "ASPEED 2700 Timer";
+    awc->reg_ops = &aspeed_2700_timer_ops;
+}
+
+static const TypeInfo aspeed_2700_timer_info = {
+    .name = TYPE_ASPEED_2700_TIMER,
+    .parent = TYPE_ASPEED_TIMER,
+    .class_init = aspeed_2700_timer_class_init,
+};
+
 static void aspeed_timer_register_types(void)
 {
     type_register_static(&aspeed_timer_info);
@@ -776,6 +999,7 @@ static void aspeed_timer_register_types(void)
     type_register_static(&aspeed_2500_timer_info);
     type_register_static(&aspeed_2600_timer_info);
     type_register_static(&aspeed_1030_timer_info);
+    type_register_static(&aspeed_2700_timer_info);
 }
 
 type_init(aspeed_timer_register_types)
diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h
index 8d0b15f055..2247dc20ab 100644
--- a/include/hw/timer/aspeed_timer.h
+++ b/include/hw/timer/aspeed_timer.h
@@ -32,6 +32,7 @@ OBJECT_DECLARE_TYPE(AspeedTimerCtrlState, AspeedTimerClass, ASPEED_TIMER)
 #define TYPE_ASPEED_2500_TIMER TYPE_ASPEED_TIMER "-ast2500"
 #define TYPE_ASPEED_2600_TIMER TYPE_ASPEED_TIMER "-ast2600"
 #define TYPE_ASPEED_1030_TIMER TYPE_ASPEED_TIMER "-ast1030"
+#define TYPE_ASPEED_2700_TIMER TYPE_ASPEED_TIMER "-ast2700"
 
 #define ASPEED_TIMER_NR_TIMERS 8
 
-- 
2.25.1

[PATCH v1 2/3] hw/timer/aspeed: Add AST2700 Support

[Jamin Lin via]

The timer controller include 8 sets of 32-bit decrement counters, based on
either PCLK or 1MHZ clock and the design of timer controller between AST2600
and AST2700 are almost the same.

The different is that the register set have a significant change in AST2700.
TIMER0 – TIMER7 has their own individua control and interrupt status register.
In other words, users are able to set timer control in register TMC10 with
different TIMER base address and clear timer control and interrupt status in
register TMC14 with different TIMER base address.

Both "aspeed_timer_read" and "aspeed_timer_write" callback functions are not
compatible AST2700. Introduce new "aspeed_2700_timer_read" and
"aspeed_2700_timer_write" callback functions and "aspeed_2700_timer_ops" memory
region operation for AST2700. Introduce a new ast2700 class to support AST2700.

The base address of TIMER0 to TIMER7 as following.
Base Address of Timer 0 = 0x12C1_0000
Base Address of Timer 1 = 0x12C1_0040
Base Address of Timer 2 = 0x12C1_0080
Base Address of Timer 3 = 0x12C1_00C0
Base Address of Timer 4 = 0x12C1_0100
Base Address of Timer 5 = 0x12C1_0140
Base Address of Timer 6 = 0x12C1_0180
Base Address of Timer 7 = 0x12C1_01C0

The register address space of each TIMER is "0x40" , so uses the following
formula to get the index and register of each TIMER.

timer_index = offset >> 6;
timer_offset = offset & 0x3f;

The TMC010 is a counter control set and interrupt status register. Write "1" to
TMC10[3:0] will set the specific bits to "1". Introduce a new
"aspeed_2700_timer_set_ctrl" function to handle this register behavior.

The TMC014 is a counter control clear and interrupt status register, to clear
the specific bits to "0", it should write "1" to  TMC14[3:0] on the same bit
position. Introduce a new "aspeed_2700_timer_clear_ctrl" function to handle
this register behavior. TMC014 does not support read operation.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/timer/aspeed_timer.c         | 224 ++++++++++++++++++++++++++++++++
 include/hw/timer/aspeed_timer.h |   1 +
 2 files changed, 225 insertions(+)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 970bf1d79d..de4d78a0fb 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -593,6 +593,214 @@ static void aspeed_2600_timer_write(AspeedTimerCtrlState *s, hwaddr offset,
     }
 }
 
+static void aspeed_2700_timer_set_ctrl(AspeedTimerCtrlState *s, int index,
+                                    uint32_t reg)
+{
+    const uint8_t overflow_interrupt_mask = BIT(op_overflow_interrupt);
+    const uint8_t external_clock_mask = BIT(op_external_clock);
+    const uint8_t pulse_enable_mask = BIT(op_pulse_enable);
+    const uint8_t enable_mask = BIT(op_enable);
+    AspeedTimer *t;
+    uint8_t t_old;
+    uint8_t t_new;
+    int shift;
+
+    /*
+     * Only 1 will set the specific bits to 1
+     * Handle a dependency between the 'enable' and remaining three
+     * configuration bits - i.e. if more than one bit in the control set has
+     * set, including the 'enable' bit, perform configuration and then
+     * enable the timer.
+     * Interrupt Status bit should not be set.
+     */
+
+     t = &s->timers[index];
+     shift = index * TIMER_CTRL_BITS;
+
+     t_old = (s->ctrl >> shift) & TIMER_CTRL_MASK;
+     t_new = reg & TIMER_CTRL_MASK;
+
+    if (!(t_old & external_clock_mask) &&
+        (t_new & external_clock_mask)) {
+        aspeed_timer_ctrl_external_clock(t, true);
+        s->ctrl = deposit32(s->ctrl, shift + op_external_clock, 1, 1);
+    }
+
+    if (!(t_old & overflow_interrupt_mask) &&
+        (t_new & overflow_interrupt_mask)) {
+        aspeed_timer_ctrl_overflow_interrupt(t, true);
+        s->ctrl = deposit32(s->ctrl, shift + op_overflow_interrupt, 1, 1);
+    }
+
+
+    if (!(t_old & pulse_enable_mask) &&
+        (t_new & pulse_enable_mask)) {
+        aspeed_timer_ctrl_pulse_enable(t, true);
+        s->ctrl = deposit32(s->ctrl, shift + op_pulse_enable, 1, 1);
+    }
+
+    /* If we are enabling, do so last */
+    if (!(t_old & enable_mask) &&
+        (t_new & enable_mask)) {
+        aspeed_timer_ctrl_enable(t, true);
+        s->ctrl = deposit32(s->ctrl, shift + op_enable, 1, 1);
+    }
+}
+
+static void aspeed_2700_timer_clear_ctrl(AspeedTimerCtrlState *s, int index,
+                                    uint32_t reg)
+{
+    const uint8_t overflow_interrupt_mask = BIT(op_overflow_interrupt);
+    const uint8_t external_clock_mask = BIT(op_external_clock);
+    const uint8_t pulse_enable_mask = BIT(op_pulse_enable);
+    const uint8_t enable_mask = BIT(op_enable);
+    AspeedTimer *t;
+    uint8_t t_old;
+    uint8_t t_new;
+    int shift;
+
+    /*
+     * Only 1 will clear the specific bits to 0
+     * Handle a dependency between the 'enable' and remaining three
+     * configuration bits - i.e. if more than one bit in the control set has
+     * clear, including the 'enable' bit, then disable the timer and perform
+     * configuration
+     */
+
+     t = &s->timers[index];
+     shift = index * TIMER_CTRL_BITS;
+
+     t_old = (s->ctrl >> shift) & TIMER_CTRL_MASK;
+     t_new = reg & TIMER_CTRL_MASK;
+
+    /* If we are disabling, do so first */
+    if ((t_old & enable_mask) &&
+        (t_new & enable_mask)) {
+        aspeed_timer_ctrl_enable(t, false);
+        s->ctrl = deposit32(s->ctrl, shift + op_enable, 1, 0);
+    }
+
+    if ((t_old & external_clock_mask) &&
+        (t_new & external_clock_mask)) {
+        aspeed_timer_ctrl_external_clock(t, false);
+        s->ctrl = deposit32(s->ctrl, shift + op_external_clock, 1, 0);
+    }
+
+    if ((t_old & overflow_interrupt_mask) &&
+        (t_new & overflow_interrupt_mask)) {
+        aspeed_timer_ctrl_overflow_interrupt(t, false);
+        s->ctrl = deposit32(s->ctrl, shift + op_overflow_interrupt, 1, 0);
+    }
+
+    if ((t_old & pulse_enable_mask) &&
+        (t_new & pulse_enable_mask)) {
+        aspeed_timer_ctrl_pulse_enable(t, false);
+        s->ctrl = deposit32(s->ctrl, shift + op_pulse_enable, 1, 0);
+    }
+
+    if ((t_old & pulse_enable_mask) &&
+        (t_new & pulse_enable_mask)) {
+        aspeed_timer_ctrl_pulse_enable(t, false);
+        s->ctrl = deposit32(s->ctrl, shift + op_pulse_enable, 1, 0);
+    }
+
+    /* Clear interrupt status */
+    if (reg & 0x10000) {
+        s->irq_sts = deposit32(s->irq_sts, index, 1, 0);
+    }
+}
+
+static uint64_t aspeed_2700_timer_read(void *opaque, hwaddr offset,
+                                    uint32_t size)
+{
+    AspeedTimerCtrlState *s = ASPEED_TIMER(opaque);
+    uint32_t timer_offset = offset & 0x3f;
+    int timer_index = offset >> 6;
+    uint64_t value = 0;
+
+    if (timer_index >= ASPEED_TIMER_NR_TIMERS) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: offset 0x%" PRIx64 " out of bounds\n",
+                      __func__, offset);
+        return 0;
+    }
+
+    switch (timer_offset) {
+    /*
+     * Counter Status
+     * Counter Reload
+     * Counter First Matching
+     * Counter Second Matching
+     */
+    case 0x00 ... 0x0C:
+        value = aspeed_timer_get_value(&s->timers[timer_index],
+                                       timer_offset >> 2);
+        break;
+    /* Counter Control and Interrupt Status */
+    case 0x10:
+        value = deposit64(value, 0, 4,
+                          extract32(s->ctrl, timer_index * 4, 4));
+        value = deposit64(value, 16, 1,
+                          extract32(s->irq_sts, timer_index, 1));
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: no getter for offset 0x%"
+                     PRIx64"\n", __func__, offset);
+        return 0;
+    }
+    trace_aspeed_timer_read(offset, size, value);
+    return value;
+}
+
+static void aspeed_2700_timer_write(void *opaque, hwaddr offset,
+                                uint64_t value, uint32_t size)
+{
+    const uint32_t timer_value = (uint32_t)(value & 0xFFFFFFFF);
+    AspeedTimerCtrlState *s = ASPEED_TIMER(opaque);
+    uint32_t timer_offset = offset & 0x3f;
+    int timer_index = offset >> 6;
+
+    if (timer_index >= ASPEED_TIMER_NR_TIMERS) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: offset 0x%" PRIx64 " out of bounds\n",
+                      __func__, offset);
+    }
+
+    switch (timer_offset) {
+    /*
+     * Counter Status
+     * Counter Reload
+     * Counter First Matching
+     * Counter Second Matching
+     */
+    case 0x00 ... 0x0C:
+        aspeed_timer_set_value(s, timer_index, timer_offset >> 2,
+                               timer_value);
+        break;
+    /* Counter Control Set and Interrupt Status */
+    case 0x10:
+        aspeed_2700_timer_set_ctrl(s, timer_index, timer_value);
+        break;
+    /* Counter Control Clear and Interrupr Status */
+    case 0x14:
+        aspeed_2700_timer_clear_ctrl(s, timer_index, timer_value);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: no setter for offset 0x%"
+                      PRIx64"\n", __func__, offset);
+        break;
+    }
+}
+
+static const MemoryRegionOps aspeed_2700_timer_ops = {
+    .read = aspeed_2700_timer_read,
+    .write = aspeed_2700_timer_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
+    .valid.unaligned = false,
+};
+
 static void aspeed_init_one_timer(AspeedTimerCtrlState *s, uint8_t id)
 {
     AspeedTimer *t = &s->timers[id];
@@ -769,6 +977,21 @@ static const TypeInfo aspeed_1030_timer_info = {
     .class_init = aspeed_1030_timer_class_init,
 };
 
+static void aspeed_2700_timer_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedTimerClass *awc = ASPEED_TIMER_CLASS(klass);
+
+    dc->desc = "ASPEED 2700 Timer";
+    awc->reg_ops = &aspeed_2700_timer_ops;
+}
+
+static const TypeInfo aspeed_2700_timer_info = {
+    .name = TYPE_ASPEED_2700_TIMER,
+    .parent = TYPE_ASPEED_TIMER,
+    .class_init = aspeed_2700_timer_class_init,
+};
+
 static void aspeed_timer_register_types(void)
 {
     type_register_static(&aspeed_timer_info);
@@ -776,6 +999,7 @@ static void aspeed_timer_register_types(void)
     type_register_static(&aspeed_2500_timer_info);
     type_register_static(&aspeed_2600_timer_info);
     type_register_static(&aspeed_1030_timer_info);
+    type_register_static(&aspeed_2700_timer_info);
 }
 
 type_init(aspeed_timer_register_types)
diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h
index 8d0b15f055..2247dc20ab 100644
--- a/include/hw/timer/aspeed_timer.h
+++ b/include/hw/timer/aspeed_timer.h
@@ -32,6 +32,7 @@ OBJECT_DECLARE_TYPE(AspeedTimerCtrlState, AspeedTimerClass, ASPEED_TIMER)
 #define TYPE_ASPEED_2500_TIMER TYPE_ASPEED_TIMER "-ast2500"
 #define TYPE_ASPEED_2600_TIMER TYPE_ASPEED_TIMER "-ast2600"
 #define TYPE_ASPEED_1030_TIMER TYPE_ASPEED_TIMER "-ast1030"
+#define TYPE_ASPEED_2700_TIMER TYPE_ASPEED_TIMER "-ast2700"
 
 #define ASPEED_TIMER_NR_TIMERS 8
 
-- 
2.25.1

[PATCH v1 3/3] aspeed/soc: Support Timer for AST2700

[Jamin Lin via]

Add Timer model for AST2700 Timer support. The Timer controller include 8 sets
of 32-bit decrement counters.

The base address of TIMER0 to TIMER7 as following.
Base Address of Timer 0 = 0x12C1_0000
Base Address of Timer 1 = 0x12C1_0040
Base Address of Timer 2 = 0x12C1_0080
Base Address of Timer 3 = 0x12C1_00C0
Base Address of Timer 4 = 0x12C1_0100
Base Address of Timer 5 = 0x12C1_0140
Base Address of Timer 6 = 0x12C1_0180
Base Address of Timer 7 = 0x12C1_01C0

The interrupt of TIMER0 to TIMER7 as following.
GICINT16 = TIMER 0 interrupt
GICINT17 = TIMER 1 interrupt
GICINT18 = TIMER 2 interrupt
GICINT19 = TIMER 3 interrupt
GICINT20 = TIMER 4 interrupt
GICINT21 = TIMER 5 interrupt
GICINT22 = TIMER 6 interrupt
GICINT23 = TIMER 7 interrupt

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed_ast27x0.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 23571584b2..292bb1c5b7 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -66,6 +66,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = {
     [ASPEED_DEV_GPIO]      =  0x14C0B000,
     [ASPEED_DEV_RTC]       =  0x12C0F000,
     [ASPEED_DEV_SDHCI]     =  0x14080000,
+    [ASPEED_DEV_TIMER1]    =  0x12C10000
 };
 
 #define AST2700_MAX_IRQ 256
@@ -397,6 +398,9 @@ static void aspeed_soc_ast2700_init(Object *obj)
 
     object_initialize_child(obj, "emmc-controller.sdhci", &s->emmc.slots[0],
                             TYPE_SYSBUS_SDHCI);
+
+    snprintf(typename, sizeof(typename), "aspeed.timer-%s", socname);
+    object_initialize_child(obj, "timerctrl", &s->timerctrl, typename);
 }
 
 /*
@@ -716,6 +720,19 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->emmc), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_EMMC));
 
+    /* Timer */
+    object_property_set_link(OBJECT(&s->timerctrl), "scu", OBJECT(&s->scu),
+                             &error_abort);
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->timerctrl), errp)) {
+        return;
+    }
+    aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->timerctrl), 0,
+                    sc->memmap[ASPEED_DEV_TIMER1]);
+    for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
+        irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
+    }
+
     create_unimplemented_device("ast2700.dpmcu", 0x11000000, 0x40000);
     create_unimplemented_device("ast2700.iomem0", 0x12000000, 0x01000000);
     create_unimplemented_device("ast2700.iomem1", 0x14000000, 0x01000000);
-- 
2.25.1

RE: [PATCH v1 2/3] hw/timer/aspeed: Add AST2700 Support

[Jamin Lin]

H Cedric, 

> Subject: Re: [PATCH v1 2/3] hw/timer/aspeed: Add AST2700 Support
> 
> On 12/16/24 08:53, Jamin Lin wrote:
> > The timer controller include 8 sets of 32-bit decrement counters,
> > based on either PCLK or 1MHZ clock and the design of timer controller
> > between AST2600 and AST2700 are almost the same.
> >
> > The different is that the register set have a significant change in AST2700.
> 
> You can drop the sentence above ^. It seems redundant.
> 
> > TIMER0 – TIMER7 has their own individua control and interrupt status
> register.
> 
>                                  individual
> 
> > In other words, users are able to set timer control in register TMC10
> > with different TIMER base address and clear timer control and
> > interrupt status in register TMC14 with different TIMER base address.
> >
> > Both "aspeed_timer_read" and "aspeed_timer_write" callback functions
> > are not compatible AST2700. Introduce new "aspeed_2700_timer_read" and
> > "aspeed_2700_timer_write" callback functions and
> > "aspeed_2700_timer_ops" memory region operation for AST2700. Introduce
> a new ast2700 class to support AST2700.
> >
> > The base address of TIMER0 to TIMER7 as following.
> > Base Address of Timer 0 = 0x12C1_0000
> > Base Address of Timer 1 = 0x12C1_0040
> > Base Address of Timer 2 = 0x12C1_0080
> > Base Address of Timer 3 = 0x12C1_00C0
> > Base Address of Timer 4 = 0x12C1_0100
> > Base Address of Timer 5 = 0x12C1_0140
> > Base Address of Timer 6 = 0x12C1_0180
> > Base Address of Timer 7 = 0x12C1_01C0
> >
> > The register address space of each TIMER is "0x40" , so uses the
> > following
> 
> I think this change would improve reading :
> 
>               " , so uses" -> "and uses"
> 
> > formula to get the index and register of each TIMER.
> >
> > timer_index = offset >> 6;
> > timer_offset = offset & 0x3f;
> >
> > The TMC010 is a counter control set and interrupt status register.
> > Write "1" to TMC10[3:0] will set the specific bits to "1". Introduce a
> > new "aspeed_2700_timer_set_ctrl" function to handle this register behavior.
> >
> > The TMC014 is a counter control clear and interrupt status register,
> > to clear the specific bits to "0", it should write "1" to  TMC14[3:0]
> > on the same bit position. Introduce a new
> > "aspeed_2700_timer_clear_ctrl" function to handle this register behavior.
> TMC014 does not support read operation.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> 
> The rest looks OK. So,
> 
> Acked-by: Cédric Le Goater <clg@redhat.com>
> 
> I would prefer Andrew to take look before merging this change though.
> 

Got it.
Thanks for your review, suggestion and kindly support.

Jamin

> Thanks,
> 
> C.
> 
> 
> > ---
> >   hw/timer/aspeed_timer.c         | 224
> ++++++++++++++++++++++++++++++++
> >   include/hw/timer/aspeed_timer.h |   1 +
> >   2 files changed, 225 insertions(+)
> >
> > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c index
> > 970bf1d79d..de4d78a0fb 100644
> > --- a/hw/timer/aspeed_timer.c
> > +++ b/hw/timer/aspeed_timer.c
> > @@ -593,6 +593,214 @@ static void
> aspeed_2600_timer_write(AspeedTimerCtrlState *s, hwaddr offset,
> >       }
> >   }
> >
> > +static void aspeed_2700_timer_set_ctrl(AspeedTimerCtrlState *s, int index,
> > +                                    uint32_t reg) {
> > +    const uint8_t overflow_interrupt_mask = BIT(op_overflow_interrupt);
> > +    const uint8_t external_clock_mask = BIT(op_external_clock);
> > +    const uint8_t pulse_enable_mask = BIT(op_pulse_enable);
> > +    const uint8_t enable_mask = BIT(op_enable);
> > +    AspeedTimer *t;
> > +    uint8_t t_old;
> > +    uint8_t t_new;
> > +    int shift;
> > +
> > +    /*
> > +     * Only 1 will set the specific bits to 1
> > +     * Handle a dependency between the 'enable' and remaining three
> > +     * configuration bits - i.e. if more than one bit in the control set has
> > +     * set, including the 'enable' bit, perform configuration and then
> > +     * enable the timer.
> > +     * Interrupt Status bit should not be set.
> > +     */
> > +
> > +     t = &s->timers[index];
> > +     shift = index * TIMER_CTRL_BITS;
> > +
> > +     t_old = (s->ctrl >> shift) & TIMER_CTRL_MASK;
> > +     t_new = reg & TIMER_CTRL_MASK;
> > +
> > +    if (!(t_old & external_clock_mask) &&
> > +        (t_new & external_clock_mask)) {
> > +        aspeed_timer_ctrl_external_clock(t, true);
> > +        s->ctrl = deposit32(s->ctrl, shift + op_external_clock, 1, 1);
> > +    }
> > +
> > +    if (!(t_old & overflow_interrupt_mask) &&
> > +        (t_new & overflow_interrupt_mask)) {
> > +        aspeed_timer_ctrl_overflow_interrupt(t, true);
> > +        s->ctrl = deposit32(s->ctrl, shift + op_overflow_interrupt, 1, 1);
> > +    }
> > +
> > +
> > +    if (!(t_old & pulse_enable_mask) &&
> > +        (t_new & pulse_enable_mask)) {
> > +        aspeed_timer_ctrl_pulse_enable(t, true);
> > +        s->ctrl = deposit32(s->ctrl, shift + op_pulse_enable, 1, 1);
> > +    }
> > +
> > +    /* If we are enabling, do so last */
> > +    if (!(t_old & enable_mask) &&
> > +        (t_new & enable_mask)) {
> > +        aspeed_timer_ctrl_enable(t, true);
> > +        s->ctrl = deposit32(s->ctrl, shift + op_enable, 1, 1);
> > +    }
> > +}
> > +
> > +static void aspeed_2700_timer_clear_ctrl(AspeedTimerCtrlState *s, int
> index,
> > +                                    uint32_t reg) {
> > +    const uint8_t overflow_interrupt_mask = BIT(op_overflow_interrupt);
> > +    const uint8_t external_clock_mask = BIT(op_external_clock);
> > +    const uint8_t pulse_enable_mask = BIT(op_pulse_enable);
> > +    const uint8_t enable_mask = BIT(op_enable);
> > +    AspeedTimer *t;
> > +    uint8_t t_old;
> > +    uint8_t t_new;
> > +    int shift;
> > +
> > +    /*
> > +     * Only 1 will clear the specific bits to 0
> > +     * Handle a dependency between the 'enable' and remaining three
> > +     * configuration bits - i.e. if more than one bit in the control set has
> > +     * clear, including the 'enable' bit, then disable the timer and perform
> > +     * configuration
> > +     */
> > +
> > +     t = &s->timers[index];
> > +     shift = index * TIMER_CTRL_BITS;
> > +
> > +     t_old = (s->ctrl >> shift) & TIMER_CTRL_MASK;
> > +     t_new = reg & TIMER_CTRL_MASK;
> > +
> > +    /* If we are disabling, do so first */
> > +    if ((t_old & enable_mask) &&
> > +        (t_new & enable_mask)) {
> > +        aspeed_timer_ctrl_enable(t, false);
> > +        s->ctrl = deposit32(s->ctrl, shift + op_enable, 1, 0);
> > +    }
> > +
> > +    if ((t_old & external_clock_mask) &&
> > +        (t_new & external_clock_mask)) {
> > +        aspeed_timer_ctrl_external_clock(t, false);
> > +        s->ctrl = deposit32(s->ctrl, shift + op_external_clock, 1, 0);
> > +    }
> > +
> > +    if ((t_old & overflow_interrupt_mask) &&
> > +        (t_new & overflow_interrupt_mask)) {
> > +        aspeed_timer_ctrl_overflow_interrupt(t, false);
> > +        s->ctrl = deposit32(s->ctrl, shift + op_overflow_interrupt, 1, 0);
> > +    }
> > +
> > +    if ((t_old & pulse_enable_mask) &&
> > +        (t_new & pulse_enable_mask)) {
> > +        aspeed_timer_ctrl_pulse_enable(t, false);
> > +        s->ctrl = deposit32(s->ctrl, shift + op_pulse_enable, 1, 0);
> > +    }
> > +
> > +    if ((t_old & pulse_enable_mask) &&
> > +        (t_new & pulse_enable_mask)) {
> > +        aspeed_timer_ctrl_pulse_enable(t, false);
> > +        s->ctrl = deposit32(s->ctrl, shift + op_pulse_enable, 1, 0);
> > +    }
> > +
> > +    /* Clear interrupt status */
> > +    if (reg & 0x10000) {
> > +        s->irq_sts = deposit32(s->irq_sts, index, 1, 0);
> > +    }
> > +}
> > +
> > +static uint64_t aspeed_2700_timer_read(void *opaque, hwaddr offset,
> > +                                    uint32_t size) {
> > +    AspeedTimerCtrlState *s = ASPEED_TIMER(opaque);
> > +    uint32_t timer_offset = offset & 0x3f;
> > +    int timer_index = offset >> 6;
> > +    uint64_t value = 0;
> > +
> > +    if (timer_index >= ASPEED_TIMER_NR_TIMERS) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: offset 0x%" PRIx64 " out of bounds\n",
> > +                      __func__, offset);
> > +        return 0;
> > +    }
> > +
> > +    switch (timer_offset) {
> > +    /*
> > +     * Counter Status
> > +     * Counter Reload
> > +     * Counter First Matching
> > +     * Counter Second Matching
> > +     */
> > +    case 0x00 ... 0x0C:
> > +        value = aspeed_timer_get_value(&s->timers[timer_index],
> > +                                       timer_offset >> 2);
> > +        break;
> > +    /* Counter Control and Interrupt Status */
> > +    case 0x10:
> > +        value = deposit64(value, 0, 4,
> > +                          extract32(s->ctrl, timer_index * 4, 4));
> > +        value = deposit64(value, 16, 1,
> > +                          extract32(s->irq_sts, timer_index, 1));
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: no getter for offset
> 0x%"
> > +                     PRIx64"\n", __func__, offset);
> > +        return 0;
> > +    }
> > +    trace_aspeed_timer_read(offset, size, value);
> > +    return value;
> > +}
> > +
> > +static void aspeed_2700_timer_write(void *opaque, hwaddr offset,
> > +                                uint64_t value, uint32_t size) {
> > +    const uint32_t timer_value = (uint32_t)(value & 0xFFFFFFFF);
> > +    AspeedTimerCtrlState *s = ASPEED_TIMER(opaque);
> > +    uint32_t timer_offset = offset & 0x3f;
> > +    int timer_index = offset >> 6;
> > +
> > +    if (timer_index >= ASPEED_TIMER_NR_TIMERS) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: offset 0x%" PRIx64 " out of bounds\n",
> > +                      __func__, offset);
> > +    }
> > +
> > +    switch (timer_offset) {
> > +    /*
> > +     * Counter Status
> > +     * Counter Reload
> > +     * Counter First Matching
> > +     * Counter Second Matching
> > +     */
> > +    case 0x00 ... 0x0C:
> > +        aspeed_timer_set_value(s, timer_index, timer_offset >> 2,
> > +                               timer_value);
> > +        break;
> > +    /* Counter Control Set and Interrupt Status */
> > +    case 0x10:
> > +        aspeed_2700_timer_set_ctrl(s, timer_index, timer_value);
> > +        break;
> > +    /* Counter Control Clear and Interrupr Status */
> > +    case 0x14:
> > +        aspeed_2700_timer_clear_ctrl(s, timer_index, timer_value);
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: no setter for offset
> 0x%"
> > +                      PRIx64"\n", __func__, offset);
> > +        break;
> > +    }
> > +}
> > +
> > +static const MemoryRegionOps aspeed_2700_timer_ops = {
> > +    .read = aspeed_2700_timer_read,
> > +    .write = aspeed_2700_timer_write,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +    .valid.min_access_size = 4,
> > +    .valid.max_access_size = 4,
> > +    .valid.unaligned = false,
> > +};
> > +
> >   static void aspeed_init_one_timer(AspeedTimerCtrlState *s, uint8_t id)
> >   {
> >       AspeedTimer *t = &s->timers[id]; @@ -769,6 +977,21 @@ static
> > const TypeInfo aspeed_1030_timer_info = {
> >       .class_init = aspeed_1030_timer_class_init,
> >   };
> >
> > +static void aspeed_2700_timer_class_init(ObjectClass *klass, void
> > +*data) {
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    AspeedTimerClass *awc = ASPEED_TIMER_CLASS(klass);
> > +
> > +    dc->desc = "ASPEED 2700 Timer";
> > +    awc->reg_ops = &aspeed_2700_timer_ops; }
> > +
> > +static const TypeInfo aspeed_2700_timer_info = {
> > +    .name = TYPE_ASPEED_2700_TIMER,
> > +    .parent = TYPE_ASPEED_TIMER,
> > +    .class_init = aspeed_2700_timer_class_init, };
> > +
> >   static void aspeed_timer_register_types(void)
> >   {
> >       type_register_static(&aspeed_timer_info);
> > @@ -776,6 +999,7 @@ static void aspeed_timer_register_types(void)
> >       type_register_static(&aspeed_2500_timer_info);
> >       type_register_static(&aspeed_2600_timer_info);
> >       type_register_static(&aspeed_1030_timer_info);
> > +    type_register_static(&aspeed_2700_timer_info);
> >   }
> >
> >   type_init(aspeed_timer_register_types)
> > diff --git a/include/hw/timer/aspeed_timer.h
> > b/include/hw/timer/aspeed_timer.h index 8d0b15f055..2247dc20ab 100644
> > --- a/include/hw/timer/aspeed_timer.h
> > +++ b/include/hw/timer/aspeed_timer.h
> > @@ -32,6 +32,7 @@ OBJECT_DECLARE_TYPE(AspeedTimerCtrlState,
> AspeedTimerClass, ASPEED_TIMER)
> >   #define TYPE_ASPEED_2500_TIMER TYPE_ASPEED_TIMER "-ast2500"
> >   #define TYPE_ASPEED_2600_TIMER TYPE_ASPEED_TIMER "-ast2600"
> >   #define TYPE_ASPEED_1030_TIMER TYPE_ASPEED_TIMER "-ast1030"
> > +#define TYPE_ASPEED_2700_TIMER TYPE_ASPEED_TIMER "-ast2700"
> >
> >   #define ASPEED_TIMER_NR_TIMERS 8
> >

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

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

RE: [PATCH v1 0/3] Support timer for AST2700

[Jamin Lin]

> From: Jamin Lin <jamin_lin@aspeedtech.com>
> Sent: Monday, December 16, 2024 3:54 PM
> To: Cédric Le Goater <clg@kaod.org>; Peter Maydell
> <peter.maydell@linaro.org>; Steven Lee <steven_lee@aspeedtech.com>; Troy
> Lee <leetroy@gmail.com>; Andrew Jeffery <andrew@codeconstruct.com.au>;
> Joel Stanley <joel@jms.id.au>; open list:ASPEED BMCs
> <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Jamin Lin <jamin_lin@aspeedtech.com>; Troy Lee
> <troy_lee@aspeedtech.com>; Yunlin Tang <yunlin.tang@aspeedtech.com>
> Subject: [PATCH v1 0/3] Support timer for AST2700
> 
> v1:
>   - Support timer for AST2700
>   - Introduce new "aspeed_2700_timer_read" and
> "aspeed_2700_timer_write"
>     callback functions and "aspeed_2700_timer_ops" memory
>     region operation for AST2700.
>     Introduce a new ast2700 class to support AST2700.
> 
> Jamin Lin (3):
>   hw/timer/aspeed: Support different memory region ops
>   hw/timer/aspeed: Add AST2700 Support
>   aspeed/soc: Support Timer for AST2700
> 
>  hw/arm/aspeed_ast27x0.c         |  17 +++
>  hw/timer/aspeed_timer.c         | 231
> +++++++++++++++++++++++++++++++-
>  include/hw/timer/aspeed_timer.h |   2 +
>  3 files changed, 249 insertions(+), 1 deletion(-)
>

Hi Andrew, 

Happy New Year 2025. 
Sorry to bother you.
If you have time, could you please help to review this patch series?
Thanks-Jamin

> --
> 2.25.1

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

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

[Jamin Lin]

Hi Andrew,

> From: Andrew Jeffery <andrew@codeconstruct.com.au>
> Sent: Thursday, January 9, 2025 9:59 AM
> To: Jamin Lin <jamin_lin@aspeedtech.com>; Cédric Le Goater <clg@kaod.org>;
> Peter Maydell <peter.maydell@linaro.org>; Steven Lee
> <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Joel Stanley
> <joel@jms.id.au>; open list:ASPEED BMCs <qemu-arm@nongnu.org>; open
> list:All patches CC here <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang
> <yunlin.tang@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_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):
> 
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);  };
> 

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

[Cédric Le Goater]

On 1/9/25 03:26, Jamin Lin wrote:
> Hi Andrew,
> 
>> From: Andrew Jeffery <andrew@codeconstruct.com.au>
>> Sent: Thursday, January 9, 2025 9:59 AM
>> To: Jamin Lin <jamin_lin@aspeedtech.com>; Cédric Le Goater <clg@kaod.org>;
>> Peter Maydell <peter.maydell@linaro.org>; Steven Lee
>> <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Joel Stanley
>> <joel@jms.id.au>; open list:ASPEED BMCs <qemu-arm@nongnu.org>; open
>> list:All patches CC here <qemu-devel@nongnu.org>
>> Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang
>> <yunlin.tang@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_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):
>>
> Thank you for your review and suggestion.
> Currently, I am working on QEMU to support the "AST2700 A1" boot(I should refactor INTC model).

Is that the reason why the QEMU ast2700-evb machine doesn't boot
with the v09.04 SDK images ?

> Once I have completed that task, I will revise the timer model with your suggestion.

Please replace suffix '_generic' by '_common' when you do.



Thanks,

C.

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

[Jamin Lin]

Hi Cedric,

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Thursday, January 9, 2025 3:02 PM
> To: Jamin Lin <jamin_lin@aspeedtech.com>; Andrew Jeffery
> <andrew@codeconstruct.com.au>; Peter Maydell <peter.maydell@linaro.org>;
> Steven Lee <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>;
> Joel Stanley <joel@jms.id.au>; open list:ASPEED BMCs
> <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang
> <yunlin.tang@aspeedtech.com>
> Subject: Re: [PATCH v1 1/3] hw/timer/aspeed: Support different memory region
> ops
> 
> On 1/9/25 03:26, Jamin Lin wrote:
> > Hi Andrew,
> >
> >> From: Andrew Jeffery <andrew@codeconstruct.com.au>
> >> Sent: Thursday, January 9, 2025 9:59 AM
> >> To: Jamin Lin <jamin_lin@aspeedtech.com>; Cédric Le Goater
> >> <clg@kaod.org>; Peter Maydell <peter.maydell@linaro.org>; Steven Lee
> >> <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Joel
> >> Stanley <joel@jms.id.au>; open list:ASPEED BMCs
> >> <qemu-arm@nongnu.org>; open list:All patches CC here
> >> <qemu-devel@nongnu.org>
> >> Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang
> >> <yunlin.tang@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_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):
> >>
> > Thank you for your review and suggestion.
> > Currently, I am working on QEMU to support the "AST2700 A1" boot(I should
> refactor INTC model).
> 
> Is that the reason why the QEMU ast2700-evb machine doesn't boot with the
> v09.04 SDK images ?
> 
Yes, "ast2700-default-obmc.tar.gz" is used for AST2700 A1.
The design between the AST2700 A0 and A1 is different, especially the INTC controllers. 
I am refactoring the INTC model to enable the AST2700 A1 to boot into the OS.
If you want to test SDK v09.04, please use "ast2700-a0-default-obmc.tar.gz".
I estimate to send the v1 patch to support A1 in the end of this month before the Chinese New Year.

> > Once I have completed that task, I will revise the timer model with your
> suggestion.
> 
> Please replace suffix '_generic' by '_common' when you do.

Got it.
Thanks for suggestion.

Jamin
> 
> 
> 
> Thanks,
> 
> C.
>