[PATCH v2 00/19] hw/timer/arm_timer: QOM'ify ARM_TIMER and correct sysbus/irq in ICP_PIT

[PATCH v2 00/19] hw/timer/arm_timer: QOM'ify ARM_TIMER and correct sysbus/irq in ICP_PIT

[Philippe Mathieu-Daudé]

This series converts the ARM_TIMER model to QOM.

Doing so we also correct an abuse of SysBus IRQ in
the ICP PIT model.

Since v1:
- Added pm215's R-b tags
- Addressed Mark/Peter review comments
  - Drop '*State' suffix from structure names
  - Use OR-IRQ gate
  - Drop sp804_unrealize()
  - Implement Resettable API
- MMIO-map timer regions into parents

Regards,

Phil.

Philippe Mathieu-Daudé (19):
  hw/timer/arm_timer: Declare QOM types using DEFINE_TYPES() macro
  hw/timer/arm_timer: Remove pointless cast from void *
  hw/timer/arm_timer: Move SP804 code around
  hw/timer/arm_timer: CamelCase rename icp_pit_state -> IntegratorPIT
  hw/timer/arm_timer: CamelCase rename arm_timer_state -> ArmTimer
  hw/timer/arm_timer: Rename SP804State -> SP804Timer
  hw/timer/arm_timer: Rename TYPE_SP804 -> TYPE_SP804_TIMER
  hw/timer/arm_timer: Extract arm_timer_reset_hold()
  hw/timer/arm_timer: Convert read/write handlers to MemoryRegionOps
    ones
  hw/timer/arm_timer: Rename arm_timer_init() -> arm_timer_new()
  hw/timer/arm_timer: Convert ArmTimer::freq to uint32_t type
  hw/timer/arm_timer: Use array of frequency in SP804Timer
  hw/timer/arm_timer: Iterate on timers using for() loop statement
  hw/timer/arm_timer: Pass timer output IRQ as parameter to
    arm_timer_new
  hw/timer/arm_timer: Fix misuse of SysBus IRQ in IntegratorPIT
  hw/timer/arm_timer: Extract icp_pit_realize() from icp_pit_init()
  hw/timer/arm_timer: QDev'ify ARM_TIMER
  hw/timer/arm_timer: Map ARM_TIMER MMIO regions into IntegratorPIT
  hw/timer/arm_timer: Map ARM_TIMER MMIO regions into SP804Timer

 hw/timer/arm_timer.c | 353 +++++++++++++++++++++++++------------------
 hw/timer/Kconfig     |   1 +
 2 files changed, 203 insertions(+), 151 deletions(-)

-- 
2.38.1

[PATCH v2 01/19] hw/timer/arm_timer: Declare QOM types using DEFINE_TYPES() macro

[Philippe Mathieu-Daudé]

When multiple QOM types are registered in the same file,
it is simpler to use the the DEFINE_TYPES() macro. Replace
the type_init() / type_register_static() combination.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/arm_timer.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 69c8863472..e410b37a23 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -380,13 +380,6 @@ static void icp_pit_init(Object *obj)
        save themselves.  */
 }
 
-static const TypeInfo icp_pit_info = {
-    .name          = TYPE_INTEGRATOR_PIT,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(icp_pit_state),
-    .instance_init = icp_pit_init,
-};
-
 static Property sp804_properties[] = {
     DEFINE_PROP_UINT32("freq0", SP804State, freq0, 1000000),
     DEFINE_PROP_UINT32("freq1", SP804State, freq1, 1000000),
@@ -402,18 +395,20 @@ static void sp804_class_init(ObjectClass *klass, void *data)
     k->vmsd = &vmstate_sp804;
 }
 
-static const TypeInfo sp804_info = {
-    .name          = TYPE_SP804,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(SP804State),
-    .instance_init = sp804_init,
-    .class_init    = sp804_class_init,
+static const TypeInfo arm_timer_types[] = {
+    {
+        .name           = TYPE_INTEGRATOR_PIT,
+        .parent         = TYPE_SYS_BUS_DEVICE,
+        .instance_size  = sizeof(icp_pit_state),
+        .instance_init  = icp_pit_init,
+
+    }, {
+        .name           = TYPE_SP804,
+        .parent         = TYPE_SYS_BUS_DEVICE,
+        .instance_size  = sizeof(SP804State),
+        .instance_init  = sp804_init,
+        .class_init     = sp804_class_init,
+    }
 };
 
-static void arm_timer_register_types(void)
-{
-    type_register_static(&icp_pit_info);
-    type_register_static(&sp804_info);
-}
-
-type_init(arm_timer_register_types)
+DEFINE_TYPES(arm_timer_types)
-- 
2.38.1

Re: [PATCH v2 01/19] hw/timer/arm_timer: Declare QOM types using DEFINE_TYPES() macro

[Richard Henderson]

On 7/4/23 16:49, Philippe Mathieu-Daudé wrote:
> When multiple QOM types are registered in the same file,
> it is simpler to use the the DEFINE_TYPES() macro. Replace
> the type_init() / type_register_static() combination.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/timer/arm_timer.c | 35 +++++++++++++++--------------------
>   1 file changed, 15 insertions(+), 20 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 02/19] hw/timer/arm_timer: Remove pointless cast from void *

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/arm_timer.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index e410b37a23..30a34a9a92 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -52,7 +52,7 @@ static void arm_timer_update(arm_timer_state *s)
 
 static uint32_t arm_timer_read(void *opaque, hwaddr offset)
 {
-    arm_timer_state *s = (arm_timer_state *)opaque;
+    arm_timer_state *s = opaque;
 
     switch (offset >> 2) {
     case 0: /* TimerLoad */
@@ -99,7 +99,7 @@ static void arm_timer_recalibrate(arm_timer_state *s, int reload)
 static void arm_timer_write(void *opaque, hwaddr offset,
                             uint32_t value)
 {
-    arm_timer_state *s = (arm_timer_state *)opaque;
+    arm_timer_state *s = opaque;
     int freq;
 
     switch (offset >> 2) {
@@ -154,7 +154,7 @@ static void arm_timer_write(void *opaque, hwaddr offset,
 
 static void arm_timer_tick(void *opaque)
 {
-    arm_timer_state *s = (arm_timer_state *)opaque;
+    arm_timer_state *s = opaque;
     s->int_level = 1;
     arm_timer_update(s);
 }
@@ -214,7 +214,7 @@ static const uint8_t sp804_ids[] = {
 /* Merge the IRQs from the two component devices.  */
 static void sp804_set_irq(void *opaque, int irq, int level)
 {
-    SP804State *s = (SP804State *)opaque;
+    SP804State *s = opaque;
 
     s->level[irq] = level;
     qemu_set_irq(s->irq, s->level[0] || s->level[1]);
@@ -223,7 +223,7 @@ static void sp804_set_irq(void *opaque, int irq, int level)
 static uint64_t sp804_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
-    SP804State *s = (SP804State *)opaque;
+    SP804State *s = opaque;
 
     if (offset < 0x20) {
         return arm_timer_read(s->timer[0], offset);
@@ -255,7 +255,7 @@ static uint64_t sp804_read(void *opaque, hwaddr offset,
 static void sp804_write(void *opaque, hwaddr offset,
                         uint64_t value, unsigned size)
 {
-    SP804State *s = (SP804State *)opaque;
+    SP804State *s = opaque;
 
     if (offset < 0x20) {
         arm_timer_write(s->timer[0], offset, value);
@@ -324,7 +324,7 @@ struct icp_pit_state {
 static uint64_t icp_pit_read(void *opaque, hwaddr offset,
                              unsigned size)
 {
-    icp_pit_state *s = (icp_pit_state *)opaque;
+    icp_pit_state *s = opaque;
     int n;
 
     /* ??? Don't know the PrimeCell ID for this device.  */
@@ -340,7 +340,7 @@ static uint64_t icp_pit_read(void *opaque, hwaddr offset,
 static void icp_pit_write(void *opaque, hwaddr offset,
                           uint64_t value, unsigned size)
 {
-    icp_pit_state *s = (icp_pit_state *)opaque;
+    icp_pit_state *s = opaque;
     int n;
 
     n = offset >> 8;
-- 
2.38.1

Re: [PATCH v2 02/19] hw/timer/arm_timer: Remove pointless cast from void *

[Richard Henderson]

On 7/4/23 16:49, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/timer/arm_timer.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 03/19] hw/timer/arm_timer: Move SP804 code around

[Philippe Mathieu-Daudé]

Move sp804_properties[] and sp804_class_init() around
with the rest of SP804 code code. What follows the
"Integrator/CP timer module." is strictly ICP related.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/arm_timer.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 30a34a9a92..0e5d5d0f6d 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -309,6 +309,21 @@ static void sp804_realize(DeviceState *dev, Error **errp)
     s->timer[1]->irq = qemu_allocate_irq(sp804_set_irq, s, 1);
 }
 
+static Property sp804_properties[] = {
+    DEFINE_PROP_UINT32("freq0", SP804State, freq0, 1000000),
+    DEFINE_PROP_UINT32("freq1", SP804State, freq1, 1000000),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void sp804_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *k = DEVICE_CLASS(klass);
+
+    k->realize = sp804_realize;
+    device_class_set_props(k, sp804_properties);
+    k->vmsd = &vmstate_sp804;
+}
+
 /* Integrator/CP timer module.  */
 
 #define TYPE_INTEGRATOR_PIT "integrator_pit"
@@ -380,21 +395,6 @@ static void icp_pit_init(Object *obj)
        save themselves.  */
 }
 
-static Property sp804_properties[] = {
-    DEFINE_PROP_UINT32("freq0", SP804State, freq0, 1000000),
-    DEFINE_PROP_UINT32("freq1", SP804State, freq1, 1000000),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void sp804_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *k = DEVICE_CLASS(klass);
-
-    k->realize = sp804_realize;
-    device_class_set_props(k, sp804_properties);
-    k->vmsd = &vmstate_sp804;
-}
-
 static const TypeInfo arm_timer_types[] = {
     {
         .name           = TYPE_INTEGRATOR_PIT,
-- 
2.38.1

Re: [PATCH v2 03/19] hw/timer/arm_timer: Move SP804 code around

[Richard Henderson]

On 7/4/23 16:49, Philippe Mathieu-Daudé wrote:
> Move sp804_properties[] and sp804_class_init() around
> with the rest of SP804 code code. What follows the
> "Integrator/CP timer module." is strictly ICP related.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/timer/arm_timer.c | 30 +++++++++++++++---------------
>   1 file changed, 15 insertions(+), 15 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 04/19] hw/timer/arm_timer: CamelCase rename icp_pit_state -> IntegratorPIT

[Philippe Mathieu-Daudé]

Following docs/devel/style.rst guidelines, rename icp_pit_state
using CamelCase as IntegratorPIT (PIT is an acronym).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/arm_timer.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 0e5d5d0f6d..c741e89cb4 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -327,9 +327,9 @@ static void sp804_class_init(ObjectClass *klass, void *data)
 /* Integrator/CP timer module.  */
 
 #define TYPE_INTEGRATOR_PIT "integrator_pit"
-OBJECT_DECLARE_SIMPLE_TYPE(icp_pit_state, INTEGRATOR_PIT)
+OBJECT_DECLARE_SIMPLE_TYPE(IntegratorPIT, INTEGRATOR_PIT)
 
-struct icp_pit_state {
+struct IntegratorPIT {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
@@ -339,7 +339,7 @@ struct icp_pit_state {
 static uint64_t icp_pit_read(void *opaque, hwaddr offset,
                              unsigned size)
 {
-    icp_pit_state *s = opaque;
+    IntegratorPIT *s = opaque;
     int n;
 
     /* ??? Don't know the PrimeCell ID for this device.  */
@@ -355,7 +355,7 @@ static uint64_t icp_pit_read(void *opaque, hwaddr offset,
 static void icp_pit_write(void *opaque, hwaddr offset,
                           uint64_t value, unsigned size)
 {
-    icp_pit_state *s = opaque;
+    IntegratorPIT *s = opaque;
     int n;
 
     n = offset >> 8;
@@ -375,7 +375,7 @@ static const MemoryRegionOps icp_pit_ops = {
 
 static void icp_pit_init(Object *obj)
 {
-    icp_pit_state *s = INTEGRATOR_PIT(obj);
+    IntegratorPIT *s = INTEGRATOR_PIT(obj);
     SysBusDevice *dev = SYS_BUS_DEVICE(obj);
 
     /* Timer 0 runs at the system clock speed (40MHz).  */
@@ -399,7 +399,7 @@ static const TypeInfo arm_timer_types[] = {
     {
         .name           = TYPE_INTEGRATOR_PIT,
         .parent         = TYPE_SYS_BUS_DEVICE,
-        .instance_size  = sizeof(icp_pit_state),
+        .instance_size  = sizeof(IntegratorPIT),
         .instance_init  = icp_pit_init,
 
     }, {
-- 
2.38.1

Re: [PATCH v2 04/19] hw/timer/arm_timer: CamelCase rename icp_pit_state -> IntegratorPIT

[Richard Henderson]

On 7/4/23 16:49, Philippe Mathieu-Daudé wrote:
> Following docs/devel/style.rst guidelines, rename icp_pit_state
> using CamelCase as IntegratorPIT (PIT is an acronym).
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/timer/arm_timer.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 05/19] hw/timer/arm_timer: CamelCase rename arm_timer_state -> ArmTimer

[Philippe Mathieu-Daudé]

Following docs/devel/style.rst guidelines, rename arm_timer_state
as ArmTimer.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/arm_timer.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index c741e89cb4..8a2939483f 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -36,11 +36,11 @@ typedef struct {
     int freq;
     int int_level;
     qemu_irq irq;
-} arm_timer_state;
+} ArmTimer;
 
 /* Check all active timers, and schedule the next timer interrupt.  */
 
-static void arm_timer_update(arm_timer_state *s)
+static void arm_timer_update(ArmTimer *s)
 {
     /* Update interrupts.  */
     if (s->int_level && (s->control & TIMER_CTRL_IE)) {
@@ -52,7 +52,7 @@ static void arm_timer_update(arm_timer_state *s)
 
 static uint32_t arm_timer_read(void *opaque, hwaddr offset)
 {
-    arm_timer_state *s = opaque;
+    ArmTimer *s = opaque;
 
     switch (offset >> 2) {
     case 0: /* TimerLoad */
@@ -79,7 +79,7 @@ static uint32_t arm_timer_read(void *opaque, hwaddr offset)
  * Reset the timer limit after settings have changed.
  * May only be called from inside a ptimer transaction block.
  */
-static void arm_timer_recalibrate(arm_timer_state *s, int reload)
+static void arm_timer_recalibrate(ArmTimer *s, int reload)
 {
     uint32_t limit;
 
@@ -99,7 +99,7 @@ static void arm_timer_recalibrate(arm_timer_state *s, int reload)
 static void arm_timer_write(void *opaque, hwaddr offset,
                             uint32_t value)
 {
-    arm_timer_state *s = opaque;
+    ArmTimer *s = opaque;
     int freq;
 
     switch (offset >> 2) {
@@ -154,7 +154,7 @@ static void arm_timer_write(void *opaque, hwaddr offset,
 
 static void arm_timer_tick(void *opaque)
 {
-    arm_timer_state *s = opaque;
+    ArmTimer *s = opaque;
     s->int_level = 1;
     arm_timer_update(s);
 }
@@ -164,19 +164,19 @@ static const VMStateDescription vmstate_arm_timer = {
     .version_id = 1,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32(control, arm_timer_state),
-        VMSTATE_UINT32(limit, arm_timer_state),
-        VMSTATE_INT32(int_level, arm_timer_state),
-        VMSTATE_PTIMER(timer, arm_timer_state),
+        VMSTATE_UINT32(control, ArmTimer),
+        VMSTATE_UINT32(limit, ArmTimer),
+        VMSTATE_INT32(int_level, ArmTimer),
+        VMSTATE_PTIMER(timer, ArmTimer),
         VMSTATE_END_OF_LIST()
     }
 };
 
-static arm_timer_state *arm_timer_init(uint32_t freq)
+static ArmTimer *arm_timer_init(uint32_t freq)
 {
-    arm_timer_state *s;
+    ArmTimer *s;
 
-    s = g_new0(arm_timer_state, 1);
+    s = g_new0(ArmTimer, 1);
     s->freq = freq;
     s->control = TIMER_CTRL_IE;
 
@@ -198,7 +198,7 @@ struct SP804State {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
-    arm_timer_state *timer[2];
+    ArmTimer *timer[2];
     uint32_t freq0, freq1;
     int level[2];
     qemu_irq irq;
@@ -333,7 +333,7 @@ struct IntegratorPIT {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
-    arm_timer_state *timer[3];
+    ArmTimer *timer[3];
 };
 
 static uint64_t icp_pit_read(void *opaque, hwaddr offset,
-- 
2.38.1

Re: [PATCH v2 05/19] hw/timer/arm_timer: CamelCase rename arm_timer_state -> ArmTimer

[Richard Henderson]

On 7/4/23 16:49, Philippe Mathieu-Daudé wrote:
> Following docs/devel/style.rst guidelines, rename arm_timer_state
> as ArmTimer.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/timer/arm_timer.c | 30 +++++++++++++++---------------
>   1 file changed, 15 insertions(+), 15 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 06/19] hw/timer/arm_timer: Rename SP804State -> SP804Timer

[Philippe Mathieu-Daudé]

Following docs/devel/style.rst guidelines, rename SP804State
as SP804Timer.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/timer/arm_timer.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 8a2939483f..41045de8ed 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -192,9 +192,9 @@ static ArmTimer *arm_timer_init(uint32_t freq)
  */
 
 #define TYPE_SP804 "sp804"
-OBJECT_DECLARE_SIMPLE_TYPE(SP804State, SP804)
+OBJECT_DECLARE_SIMPLE_TYPE(SP804Timer, SP804)
 
-struct SP804State {
+struct SP804Timer {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
@@ -214,7 +214,7 @@ static const uint8_t sp804_ids[] = {
 /* Merge the IRQs from the two component devices.  */
 static void sp804_set_irq(void *opaque, int irq, int level)
 {
-    SP804State *s = opaque;
+    SP804Timer *s = opaque;
 
     s->level[irq] = level;
     qemu_set_irq(s->irq, s->level[0] || s->level[1]);
@@ -223,7 +223,7 @@ static void sp804_set_irq(void *opaque, int irq, int level)
 static uint64_t sp804_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
-    SP804State *s = opaque;
+    SP804Timer *s = opaque;
 
     if (offset < 0x20) {
         return arm_timer_read(s->timer[0], offset);
@@ -255,7 +255,7 @@ static uint64_t sp804_read(void *opaque, hwaddr offset,
 static void sp804_write(void *opaque, hwaddr offset,
                         uint64_t value, unsigned size)
 {
-    SP804State *s = opaque;
+    SP804Timer *s = opaque;
 
     if (offset < 0x20) {
         arm_timer_write(s->timer[0], offset, value);
@@ -283,14 +283,14 @@ static const VMStateDescription vmstate_sp804 = {
     .version_id = 1,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
-        VMSTATE_INT32_ARRAY(level, SP804State, 2),
+        VMSTATE_INT32_ARRAY(level, SP804Timer, 2),
         VMSTATE_END_OF_LIST()
     }
 };
 
 static void sp804_init(Object *obj)
 {
-    SP804State *s = SP804(obj);
+    SP804Timer *s = SP804(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
     sysbus_init_irq(sbd, &s->irq);
@@ -301,7 +301,7 @@ static void sp804_init(Object *obj)
 
 static void sp804_realize(DeviceState *dev, Error **errp)
 {
-    SP804State *s = SP804(dev);
+    SP804Timer *s = SP804(dev);
 
     s->timer[0] = arm_timer_init(s->freq0);
     s->timer[1] = arm_timer_init(s->freq1);
@@ -310,8 +310,8 @@ static void sp804_realize(DeviceState *dev, Error **errp)
 }
 
 static Property sp804_properties[] = {
-    DEFINE_PROP_UINT32("freq0", SP804State, freq0, 1000000),
-    DEFINE_PROP_UINT32("freq1", SP804State, freq1, 1000000),
+    DEFINE_PROP_UINT32("freq0", SP804Timer, freq0, 1000000),
+    DEFINE_PROP_UINT32("freq1", SP804Timer, freq1, 1000000),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -405,7 +405,7 @@ static const TypeInfo arm_timer_types[] = {
     }, {
         .name           = TYPE_SP804,
         .parent         = TYPE_SYS_BUS_DEVICE,
-        .instance_size  = sizeof(SP804State),
+        .instance_size  = sizeof(SP804Timer),
         .instance_init  = sp804_init,
         .class_init     = sp804_class_init,
     }
-- 
2.38.1

Re: [PATCH v2 06/19] hw/timer/arm_timer: Rename SP804State -> SP804Timer

[Richard Henderson]

On 7/4/23 16:49, Philippe Mathieu-Daudé wrote:
> Following docs/devel/style.rst guidelines, rename SP804State
> as SP804Timer.
> 
> Suggested-by: Peter Maydell<peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/timer/arm_timer.c | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 07/19] hw/timer/arm_timer: Rename TYPE_SP804 -> TYPE_SP804_TIMER

[Philippe Mathieu-Daudé]

Having a QOM object using its device type as suffix is
often helpful.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/timer/arm_timer.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 41045de8ed..8dae845998 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -191,8 +191,8 @@ static ArmTimer *arm_timer_init(uint32_t freq)
  * https://developer.arm.com/documentation/ddi0271/latest/
  */
 
-#define TYPE_SP804 "sp804"
-OBJECT_DECLARE_SIMPLE_TYPE(SP804Timer, SP804)
+#define TYPE_SP804_TIMER "sp804"
+OBJECT_DECLARE_SIMPLE_TYPE(SP804Timer, SP804_TIMER)
 
 struct SP804Timer {
     SysBusDevice parent_obj;
@@ -290,7 +290,7 @@ static const VMStateDescription vmstate_sp804 = {
 
 static void sp804_init(Object *obj)
 {
-    SP804Timer *s = SP804(obj);
+    SP804Timer *s = SP804_TIMER(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
     sysbus_init_irq(sbd, &s->irq);
@@ -301,7 +301,7 @@ static void sp804_init(Object *obj)
 
 static void sp804_realize(DeviceState *dev, Error **errp)
 {
-    SP804Timer *s = SP804(dev);
+    SP804Timer *s = SP804_TIMER(dev);
 
     s->timer[0] = arm_timer_init(s->freq0);
     s->timer[1] = arm_timer_init(s->freq1);
@@ -403,7 +403,7 @@ static const TypeInfo arm_timer_types[] = {
         .instance_init  = icp_pit_init,
 
     }, {
-        .name           = TYPE_SP804,
+        .name           = TYPE_SP804_TIMER,
         .parent         = TYPE_SYS_BUS_DEVICE,
         .instance_size  = sizeof(SP804Timer),
         .instance_init  = sp804_init,
-- 
2.38.1

Re: [PATCH v2 07/19] hw/timer/arm_timer: Rename TYPE_SP804 -> TYPE_SP804_TIMER

[Richard Henderson]

On 7/4/23 16:50, Philippe Mathieu-Daudé wrote:
> Having a QOM object using its device type as suffix is
> often helpful.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/timer/arm_timer.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 08/19] hw/timer/arm_timer: Extract arm_timer_reset_hold()

[Philippe Mathieu-Daudé]

Extract arm_timer_reset_hold() before converting this model to
QOM/QDev in few commits. This will become our ResettableHoldPhase
handler.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/timer/arm_timer.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 8dae845998..0d7fac4d78 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -172,13 +172,20 @@ static const VMStateDescription vmstate_arm_timer = {
     }
 };
 
+static void arm_timer_reset_hold(ArmTimer *s)
+{
+    s->limit = 0;
+    s->int_level = 0;
+    s->control = TIMER_CTRL_IE;
+}
+
 static ArmTimer *arm_timer_init(uint32_t freq)
 {
     ArmTimer *s;
 
     s = g_new0(ArmTimer, 1);
     s->freq = freq;
-    s->control = TIMER_CTRL_IE;
+    arm_timer_reset_hold(s);
 
     s->timer = ptimer_init(arm_timer_tick, s, PTIMER_POLICY_LEGACY);
     vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_arm_timer, s);
-- 
2.38.1

Re: [PATCH v2 08/19] hw/timer/arm_timer: Extract arm_timer_reset_hold()

[Richard Henderson]

On 7/4/23 16:50, Philippe Mathieu-Daudé wrote:
> Extract arm_timer_reset_hold() before converting this model to
> QOM/QDev in few commits. This will become our ResettableHoldPhase
> handler.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/timer/arm_timer.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 09/19] hw/timer/arm_timer: Convert read/write handlers to MemoryRegionOps ones

[Philippe Mathieu-Daudé]

In order to simplify the QOM convertion of ARM_TIMER in a few
commits, start converting the read/write() handlers to follow
the MemoryRegionOps::read/write() prototypes.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/timer/arm_timer.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 0d7fac4d78..cbd82e8365 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -50,7 +50,7 @@ static void arm_timer_update(ArmTimer *s)
     }
 }
 
-static uint32_t arm_timer_read(void *opaque, hwaddr offset)
+static uint64_t arm_timer_read(void *opaque, hwaddr offset, unsigned size)
 {
     ArmTimer *s = opaque;
 
@@ -97,7 +97,7 @@ static void arm_timer_recalibrate(ArmTimer *s, int reload)
 }
 
 static void arm_timer_write(void *opaque, hwaddr offset,
-                            uint32_t value)
+                            uint64_t value, unsigned size)
 {
     ArmTimer *s = opaque;
     int freq;
@@ -233,10 +233,10 @@ static uint64_t sp804_read(void *opaque, hwaddr offset,
     SP804Timer *s = opaque;
 
     if (offset < 0x20) {
-        return arm_timer_read(s->timer[0], offset);
+        return arm_timer_read(&s->timer[0], offset, size);
     }
     if (offset < 0x40) {
-        return arm_timer_read(s->timer[1], offset - 0x20);
+        return arm_timer_read(&s->timer[1], offset - 0x20, size);
     }
 
     /* TimerPeriphID */
@@ -265,12 +265,12 @@ static void sp804_write(void *opaque, hwaddr offset,
     SP804Timer *s = opaque;
 
     if (offset < 0x20) {
-        arm_timer_write(s->timer[0], offset, value);
+        arm_timer_write(&s->timer[0], offset, value, size);
         return;
     }
 
     if (offset < 0x40) {
-        arm_timer_write(s->timer[1], offset - 0x20, value);
+        arm_timer_write(&s->timer[1], offset - 0x20, value, size);
         return;
     }
 
@@ -356,7 +356,7 @@ static uint64_t icp_pit_read(void *opaque, hwaddr offset,
         return 0;
     }
 
-    return arm_timer_read(s->timer[n], offset & 0xff);
+    return arm_timer_read(&s->timer[n], offset & 0xff, size);
 }
 
 static void icp_pit_write(void *opaque, hwaddr offset,
@@ -371,7 +371,7 @@ static void icp_pit_write(void *opaque, hwaddr offset,
         return;
     }
 
-    arm_timer_write(s->timer[n], offset & 0xff, value);
+    arm_timer_write(&s->timer[n], offset & 0xff, value, size);
 }
 
 static const MemoryRegionOps icp_pit_ops = {
-- 
2.38.1

Re: [PATCH v2 09/19] hw/timer/arm_timer: Convert read/write handlers to MemoryRegionOps ones

[Richard Henderson]

On 7/4/23 16:50, Philippe Mathieu-Daudé wrote:
> In order to simplify the QOM convertion of ARM_TIMER in a few
> commits, start converting the read/write() handlers to follow
> the MemoryRegionOps::read/write() prototypes.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/timer/arm_timer.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 10/19] hw/timer/arm_timer: Rename arm_timer_init() -> arm_timer_new()

[Philippe Mathieu-Daudé]

QDev models often use foo_new() as the combination of
foo_init() + foo_realize(). Here arm_timer_init() is
a such combination, so rename it as arm_timer_new() to
emphasis the returned device is already realized.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/arm_timer.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index cbd82e8365..4ef34b0f60 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -179,7 +179,7 @@ static void arm_timer_reset_hold(ArmTimer *s)
     s->control = TIMER_CTRL_IE;
 }
 
-static ArmTimer *arm_timer_init(uint32_t freq)
+static ArmTimer *arm_timer_new(uint32_t freq)
 {
     ArmTimer *s;
 
@@ -310,8 +310,8 @@ static void sp804_realize(DeviceState *dev, Error **errp)
 {
     SP804Timer *s = SP804_TIMER(dev);
 
-    s->timer[0] = arm_timer_init(s->freq0);
-    s->timer[1] = arm_timer_init(s->freq1);
+    s->timer[0] = arm_timer_new(s->freq0);
+    s->timer[1] = arm_timer_new(s->freq1);
     s->timer[0]->irq = qemu_allocate_irq(sp804_set_irq, s, 0);
     s->timer[1]->irq = qemu_allocate_irq(sp804_set_irq, s, 1);
 }
@@ -386,10 +386,10 @@ static void icp_pit_init(Object *obj)
     SysBusDevice *dev = SYS_BUS_DEVICE(obj);
 
     /* Timer 0 runs at the system clock speed (40MHz).  */
-    s->timer[0] = arm_timer_init(40000000);
+    s->timer[0] = arm_timer_new(40000000);
     /* The other two timers run at 1MHz.  */
-    s->timer[1] = arm_timer_init(1000000);
-    s->timer[2] = arm_timer_init(1000000);
+    s->timer[1] = arm_timer_new(1000000);
+    s->timer[2] = arm_timer_new(1000000);
 
     sysbus_init_irq(dev, &s->timer[0]->irq);
     sysbus_init_irq(dev, &s->timer[1]->irq);
-- 
2.38.1

Re: [PATCH v2 10/19] hw/timer/arm_timer: Rename arm_timer_init() -> arm_timer_new()

[Richard Henderson]

On 7/4/23 16:50, Philippe Mathieu-Daudé wrote:
> QDev models often use foo_new() as the combination of
> foo_init() + foo_realize(). Here arm_timer_init() is
> a such combination, so rename it as arm_timer_new() to
> emphasis the returned device is already realized.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/timer/arm_timer.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 11/19] hw/timer/arm_timer: Convert ArmTimer::freq to uint32_t type

[Philippe Mathieu-Daudé]

In preparation of accessing ArmTimer::freq as a QOM property,
convert it to uint32_t (so we'll be able to use DEFINE_PROP_UINT32).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/arm_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 4ef34b0f60..2b5fb75577 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -33,7 +33,7 @@ typedef struct {
     ptimer_state *timer;
     uint32_t control;
     uint32_t limit;
-    int freq;
+    uint32_t freq;
     int int_level;
     qemu_irq irq;
 } ArmTimer;
-- 
2.38.1

Re: [PATCH v2 11/19] hw/timer/arm_timer: Convert ArmTimer::freq to uint32_t type

[Richard Henderson]

On 7/4/23 16:50, Philippe Mathieu-Daudé wrote:
> In preparation of accessing ArmTimer::freq as a QOM property,
> convert it to uint32_t (so we'll be able to use DEFINE_PROP_UINT32).
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/timer/arm_timer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH v2 12/19] hw/timer/arm_timer: Use array of frequency in SP804Timer

[Philippe Mathieu-Daudé]

SP804Timer use arrays for timers and IRQ levels. Be consistent
and use another one for the frequencies. This will allow to
simplify using for() loop statement in the next commit.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/arm_timer.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 2b5fb75577..0ad0e55df8 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -206,7 +206,7 @@ struct SP804Timer {
 
     MemoryRegion iomem;
     ArmTimer *timer[2];
-    uint32_t freq0, freq1;
+    uint32_t freq[2];
     int level[2];
     qemu_irq irq;
 };
@@ -310,15 +310,15 @@ static void sp804_realize(DeviceState *dev, Error **errp)
 {
     SP804Timer *s = SP804_TIMER(dev);
 
-    s->timer[0] = arm_timer_new(s->freq0);
-    s->timer[1] = arm_timer_new(s->freq1);
+    s->timer[0] = arm_timer_new(s->freq[0]);
+    s->timer[1] = arm_timer_new(s->freq[1]);
     s->timer[0]->irq = qemu_allocate_irq(sp804_set_irq, s, 0);
     s->timer[1]->irq = qemu_allocate_irq(sp804_set_irq, s, 1);
 }
 
 static Property sp804_properties[] = {
-    DEFINE_PROP_UINT32("freq0", SP804Timer, freq0, 1000000),
-    DEFINE_PROP_UINT32("freq1", SP804Timer, freq1, 1000000),
+    DEFINE_PROP_UINT32("freq0", SP804Timer, freq[0], 1000000),
+    DEFINE_PROP_UINT32("freq1", SP804Timer, freq[1], 1000000),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.38.1

Re: [PATCH v2 12/19] hw/timer/arm_timer: Use array of frequency in SP804Timer

[Richard Henderson]

On 7/4/23 16:50, Philippe Mathieu-Daudé wrote:
> SP804Timer use arrays for timers and IRQ levels. Be consistent
> and use another one for the frequencies. This will allow to
> simplify using for() loop statement in the next commit.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/timer/arm_timer.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 13/19] hw/timer/arm_timer: Iterate on timers using for() loop statement

[Philippe Mathieu-Daudé]

The same pattern is used for each timer, 2 or 3 times. To avoid
too much code churn in the next commits, iterate on the number
of timers using a for() loop statement.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/arm_timer.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 0ad0e55df8..68cd50314f 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -310,10 +310,10 @@ static void sp804_realize(DeviceState *dev, Error **errp)
 {
     SP804Timer *s = SP804_TIMER(dev);
 
-    s->timer[0] = arm_timer_new(s->freq[0]);
-    s->timer[1] = arm_timer_new(s->freq[1]);
-    s->timer[0]->irq = qemu_allocate_irq(sp804_set_irq, s, 0);
-    s->timer[1]->irq = qemu_allocate_irq(sp804_set_irq, s, 1);
+    for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
+        s->timer[i] = arm_timer_new(s->freq[i]);
+        s->timer[i]->irq = qemu_allocate_irq(sp804_set_irq, s, i);
+    }
 }
 
 static Property sp804_properties[] = {
@@ -382,18 +382,19 @@ static const MemoryRegionOps icp_pit_ops = {
 
 static void icp_pit_init(Object *obj)
 {
+    static const uint32_t tmr_freq[] = {
+        /* Timer 0 runs at the system clock speed (40MHz).  */
+        40000000,
+        /* The other two timers run at 1MHz.  */
+        1000000, 1000000
+    };
     IntegratorPIT *s = INTEGRATOR_PIT(obj);
     SysBusDevice *dev = SYS_BUS_DEVICE(obj);
 
-    /* Timer 0 runs at the system clock speed (40MHz).  */
-    s->timer[0] = arm_timer_new(40000000);
-    /* The other two timers run at 1MHz.  */
-    s->timer[1] = arm_timer_new(1000000);
-    s->timer[2] = arm_timer_new(1000000);
-
-    sysbus_init_irq(dev, &s->timer[0]->irq);
-    sysbus_init_irq(dev, &s->timer[1]->irq);
-    sysbus_init_irq(dev, &s->timer[2]->irq);
+    for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
+        s->timer[i] = arm_timer_new(tmr_freq[i]);
+        sysbus_init_irq(dev, &s->timer[i]->irq);
+    }
 
     memory_region_init_io(&s->iomem, obj, &icp_pit_ops, s,
                           "icp_pit", 0x1000);
-- 
2.38.1

Re: [PATCH v2 13/19] hw/timer/arm_timer: Iterate on timers using for() loop statement

[Richard Henderson]

On 7/4/23 16:50, Philippe Mathieu-Daudé wrote:
> The same pattern is used for each timer, 2 or 3 times. To avoid
> too much code churn in the next commits, iterate on the number
> of timers using a for() loop statement.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/timer/arm_timer.c | 27 ++++++++++++++-------------
>   1 file changed, 14 insertions(+), 13 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 14/19] hw/timer/arm_timer: Pass timer output IRQ as parameter to arm_timer_new

[Philippe Mathieu-Daudé]

Both SP804Timer/IntegratorPIT peek at ArmTimer internal state.
This is fine so far but we want to convert ArmTimer to QOM
where peeking at QOM state internal should be avoided.
ArmTimer's IRQ is just a pointer, so we can pass/set it via
argument, avoiding accessing ArmTimer internal state except
from the arm_timer_*() methods.

Once ArmTimer get QOM'ified (in a few commits), it will
inherit the SysBus API. This IRQ will then become a SysBus
IRQ within this ArmTimer object.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/timer/arm_timer.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 68cd50314f..f6bec28884 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -179,7 +179,7 @@ static void arm_timer_reset_hold(ArmTimer *s)
     s->control = TIMER_CTRL_IE;
 }
 
-static ArmTimer *arm_timer_new(uint32_t freq)
+static ArmTimer *arm_timer_new(uint32_t freq, qemu_irq irq_out)
 {
     ArmTimer *s;
 
@@ -187,6 +187,7 @@ static ArmTimer *arm_timer_new(uint32_t freq)
     s->freq = freq;
     arm_timer_reset_hold(s);
 
+    s->irq = irq_out;
     s->timer = ptimer_init(arm_timer_tick, s, PTIMER_POLICY_LEGACY);
     vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_arm_timer, s);
     return s;
@@ -209,6 +210,7 @@ struct SP804Timer {
     uint32_t freq[2];
     int level[2];
     qemu_irq irq;
+    qemu_irq irq_in[2];
 };
 
 static const uint8_t sp804_ids[] = {
@@ -311,8 +313,8 @@ static void sp804_realize(DeviceState *dev, Error **errp)
     SP804Timer *s = SP804_TIMER(dev);
 
     for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
-        s->timer[i] = arm_timer_new(s->freq[i]);
-        s->timer[i]->irq = qemu_allocate_irq(sp804_set_irq, s, i);
+        s->irq_in[i] = qemu_allocate_irq(sp804_set_irq, s, i);
+        s->timer[i] = arm_timer_new(s->freq[i], s->irq_in[i]);
     }
 }
 
@@ -341,6 +343,7 @@ struct IntegratorPIT {
 
     MemoryRegion iomem;
     ArmTimer *timer[3];
+    qemu_irq irq_in[3];
 };
 
 static uint64_t icp_pit_read(void *opaque, hwaddr offset,
@@ -392,8 +395,8 @@ static void icp_pit_init(Object *obj)
     SysBusDevice *dev = SYS_BUS_DEVICE(obj);
 
     for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
-        s->timer[i] = arm_timer_new(tmr_freq[i]);
-        sysbus_init_irq(dev, &s->timer[i]->irq);
+        s->timer[i] = arm_timer_new(tmr_freq[i], s->irq_in[i]);
+        sysbus_init_irq(dev, &s->irq_in[i]);
     }
 
     memory_region_init_io(&s->iomem, obj, &icp_pit_ops, s,
-- 
2.38.1

Re: [PATCH v2 14/19] hw/timer/arm_timer: Pass timer output IRQ as parameter to arm_timer_new

[Richard Henderson]

On 7/4/23 16:50, Philippe Mathieu-Daudé wrote:
> Both SP804Timer/IntegratorPIT peek at ArmTimer internal state.
> This is fine so far but we want to convert ArmTimer to QOM
> where peeking at QOM state internal should be avoided.
> ArmTimer's IRQ is just a pointer, so we can pass/set it via
> argument, avoiding accessing ArmTimer internal state except
> from the arm_timer_*() methods.
> 
> Once ArmTimer get QOM'ified (in a few commits), it will
> inherit the SysBus API. This IRQ will then become a SysBus
> IRQ within this ArmTimer object.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/timer/arm_timer.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)

Acked-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 15/19] hw/timer/arm_timer: Fix misuse of SysBus IRQ in IntegratorPIT

[Philippe Mathieu-Daudé]

SysBus IRQ are *output* IRQs. As some sort of simplification
to avoid to forward it, IntegratorPIT misuses it as ARM timer
input IRQ. Fix that by using a simple IRQ forwarder handler.

Note: sysbus_pass_irq() forwards GPIOs and IRQs from a container
to an inner device but only work with an entire set of IRQs, so
we can not use it here where we forward a single IRQ from each
timer.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/arm_timer.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index f6bec28884..aae7f3cf9d 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -344,6 +344,7 @@ struct IntegratorPIT {
     MemoryRegion iomem;
     ArmTimer *timer[3];
     qemu_irq irq_in[3];
+    qemu_irq irq[3];
 };
 
 static uint64_t icp_pit_read(void *opaque, hwaddr offset,
@@ -383,6 +384,13 @@ static const MemoryRegionOps icp_pit_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static void icp_pit_fwd_irq(void *opaque, int n, int level)
+{
+    IntegratorPIT *s = opaque;
+
+    qemu_set_irq(s->irq[n], level);
+}
+
 static void icp_pit_init(Object *obj)
 {
     static const uint32_t tmr_freq[] = {
@@ -394,9 +402,14 @@ static void icp_pit_init(Object *obj)
     IntegratorPIT *s = INTEGRATOR_PIT(obj);
     SysBusDevice *dev = SYS_BUS_DEVICE(obj);
 
+    qdev_init_gpio_in_named(DEVICE(obj), icp_pit_fwd_irq,
+                            "timer-in", ARRAY_SIZE(s->timer));
+
     for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
         s->timer[i] = arm_timer_new(tmr_freq[i], s->irq_in[i]);
-        sysbus_init_irq(dev, &s->irq_in[i]);
+        sysbus_init_irq(dev, &s->irq[i]);
+        sysbus_connect_irq(dev, i,
+                           qdev_get_gpio_in_named(DEVICE(obj), "timer-in", i));
     }
 
     memory_region_init_io(&s->iomem, obj, &icp_pit_ops, s,
-- 
2.38.1

Re: [PATCH v2 15/19] hw/timer/arm_timer: Fix misuse of SysBus IRQ in IntegratorPIT

[Richard Henderson]

On 7/4/23 16:50, Philippe Mathieu-Daudé wrote:
> SysBus IRQ are*output*  IRQs. As some sort of simplification
> to avoid to forward it, IntegratorPIT misuses it as ARM timer
> input IRQ. Fix that by using a simple IRQ forwarder handler.
> 
> Note: sysbus_pass_irq() forwards GPIOs and IRQs from a container
> to an inner device but only work with an entire set of IRQs, so
> we can not use it here where we forward a single IRQ from each
> timer.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/timer/arm_timer.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)

Acked-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 16/19] hw/timer/arm_timer: Extract icp_pit_realize() from icp_pit_init()

[Philippe Mathieu-Daudé]

To make the next commit easier to digest, extract icp_pit_realize()
from icp_pit_init() as a preliminary step.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/timer/arm_timer.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index aae7f3cf9d..30e29cc166 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -408,8 +408,6 @@ static void icp_pit_init(Object *obj)
     for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
         s->timer[i] = arm_timer_new(tmr_freq[i], s->irq_in[i]);
         sysbus_init_irq(dev, &s->irq[i]);
-        sysbus_connect_irq(dev, i,
-                           qdev_get_gpio_in_named(DEVICE(obj), "timer-in", i));
     }
 
     memory_region_init_io(&s->iomem, obj, &icp_pit_ops, s,
@@ -419,12 +417,31 @@ static void icp_pit_init(Object *obj)
        save themselves.  */
 }
 
+static void icp_pit_realize(DeviceState *dev, Error **errp)
+{
+    IntegratorPIT *s = INTEGRATOR_PIT(dev);
+
+    for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
+        SysBusDevice *tmr = SYS_BUS_DEVICE(&s->timer[i]);
+
+        sysbus_connect_irq(tmr, i, qdev_get_gpio_in_named(dev, "timer-in", i));
+    }
+}
+
+static void icp_pit_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *k = DEVICE_CLASS(klass);
+
+    k->realize = icp_pit_realize;
+}
+
 static const TypeInfo arm_timer_types[] = {
     {
         .name           = TYPE_INTEGRATOR_PIT,
         .parent         = TYPE_SYS_BUS_DEVICE,
         .instance_size  = sizeof(IntegratorPIT),
         .instance_init  = icp_pit_init,
+        .class_init     = icp_pit_class_init,
 
     }, {
         .name           = TYPE_SP804_TIMER,
-- 
2.38.1

Re: [PATCH v2 16/19] hw/timer/arm_timer: Extract icp_pit_realize() from icp_pit_init()

[Richard Henderson]

On 7/4/23 16:50, Philippe Mathieu-Daudé wrote:
> To make the next commit easier to digest, extract icp_pit_realize()
> from icp_pit_init() as a preliminary step.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/timer/arm_timer.c | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 17/19] hw/timer/arm_timer: QDev'ify ARM_TIMER

[Philippe Mathieu-Daudé]

Introduce the ARM_TIMER sysbus device, exposing one output IRQ
and a single MMIO region.

arm_timer_new() is converted as QOM instance init()/finalize()
handlers. Note in arm_timer_finalize() we release a ptimer handle
which was previously leaked.

ArmTimer is directly embedded into SP804Timer/IntegratorPIT,
and is initialized as a QOM child.

Since the timer frequency belongs to ARM_TIMER, have it hold the
QOM property. SP804Timer/IntegratorPIT directly access it.

For IntegratorPIT, each ARM_TIMER sysbus output IRQ is wired as
input IRQ.

For the SP804Timer, we add a TYPE_OR_IRQ to OR the ARM_TIMER sysbus
output IRQs together, exposing a single output IRQ.
The Kconfig entry have to select the OR_IRQ dependency.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/timer/arm_timer.c | 139 ++++++++++++++++++++++++++++++++-----------
 hw/timer/Kconfig     |   1 +
 2 files changed, 105 insertions(+), 35 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 30e29cc166..8207723ab5 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -12,11 +12,13 @@
 #include "migration/vmstate.h"
 #include "qemu/timer.h"
 #include "hw/irq.h"
+#include "hw/or-irq.h"
 #include "hw/ptimer.h"
 #include "hw/qdev-properties.h"
 #include "qemu/module.h"
 #include "qemu/log.h"
 #include "qom/object.h"
+#include "qapi/error.h"
 
 /* Common timer implementation.  */
 
@@ -29,14 +31,20 @@
 #define TIMER_CTRL_PERIODIC     (1 << 6)
 #define TIMER_CTRL_ENABLE       (1 << 7)
 
-typedef struct {
+#define TYPE_ARM_TIMER "arm-timer"
+OBJECT_DECLARE_SIMPLE_TYPE(ArmTimer, ARM_TIMER)
+
+struct ArmTimer {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
     ptimer_state *timer;
     uint32_t control;
     uint32_t limit;
     uint32_t freq;
     int int_level;
     qemu_irq irq;
-} ArmTimer;
+};
 
 /* Check all active timers, and schedule the next timer interrupt.  */
 
@@ -152,6 +160,14 @@ static void arm_timer_write(void *opaque, hwaddr offset,
     arm_timer_update(s);
 }
 
+static const MemoryRegionOps arm_timer_ops = {
+    .read = arm_timer_read,
+    .write = arm_timer_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl.min_access_size = 4,
+    .impl.max_access_size = 4,
+};
+
 static void arm_timer_tick(void *opaque)
 {
     ArmTimer *s = opaque;
@@ -172,25 +188,49 @@ static const VMStateDescription vmstate_arm_timer = {
     }
 };
 
-static void arm_timer_reset_hold(ArmTimer *s)
+static void arm_timer_reset_hold(Object *obj)
 {
+    ArmTimer *s = ARM_TIMER(obj);
+
     s->limit = 0;
     s->int_level = 0;
     s->control = TIMER_CTRL_IE;
 }
 
-static ArmTimer *arm_timer_new(uint32_t freq, qemu_irq irq_out)
+static void arm_timer_init(Object *obj)
 {
-    ArmTimer *s;
+    ArmTimer *s = ARM_TIMER(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
-    s = g_new0(ArmTimer, 1);
-    s->freq = freq;
-    arm_timer_reset_hold(s);
-
-    s->irq = irq_out;
     s->timer = ptimer_init(arm_timer_tick, s, PTIMER_POLICY_LEGACY);
-    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_arm_timer, s);
-    return s;
+
+    sysbus_init_irq(sbd, &s->irq);
+
+    memory_region_init_io(&s->iomem, obj, &arm_timer_ops, s,
+                          "arm_timer", 0x20);
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static void arm_timer_finalize(Object *obj)
+{
+    ArmTimer *s = ARM_TIMER(obj);
+
+    ptimer_free(s->timer);
+}
+
+static Property arm_timer_properties[] = {
+    DEFINE_PROP_UINT32("freq", ArmTimer, freq, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void arm_timer_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    ResettableClass *rc = RESETTABLE_CLASS(oc);
+
+    dc->vmsd = &vmstate_arm_timer;
+    device_class_set_props(dc, arm_timer_properties);
+    rc->phases.hold = arm_timer_reset_hold;
 }
 
 /*
@@ -206,11 +246,10 @@ struct SP804Timer {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
-    ArmTimer *timer[2];
-    uint32_t freq[2];
-    int level[2];
+    ArmTimer timer[2];
+    int mig_v1_level[2];
+    OrIRQState irq_orgate;
     qemu_irq irq;
-    qemu_irq irq_in[2];
 };
 
 static const uint8_t sp804_ids[] = {
@@ -220,15 +259,6 @@ static const uint8_t sp804_ids[] = {
     0xd, 0xf0, 0x05, 0xb1
 };
 
-/* Merge the IRQs from the two component devices.  */
-static void sp804_set_irq(void *opaque, int irq, int level)
-{
-    SP804Timer *s = opaque;
-
-    s->level[irq] = level;
-    qemu_set_irq(s->irq, s->level[0] || s->level[1]);
-}
-
 static uint64_t sp804_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
@@ -287,12 +317,26 @@ static const MemoryRegionOps sp804_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static int sp804_post_load(void *opaque, int version_id)
+{
+    SP804Timer *s = opaque;
+
+    if (version_id < 2) {
+        for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
+            qemu_set_irq(qdev_get_gpio_in(DEVICE(&s->irq_orgate), i),
+                         s->mig_v1_level[i]);
+        }
+    }
+    return 0;
+}
+
 static const VMStateDescription vmstate_sp804 = {
     .name = "sp804",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
+    .post_load = sp804_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_INT32_ARRAY(level, SP804Timer, 2),
+        VMSTATE_INT32_ARRAY(mig_v1_level, SP804Timer, 2),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -306,21 +350,36 @@ static void sp804_init(Object *obj)
     memory_region_init_io(&s->iomem, obj, &sp804_ops, s,
                           "sp804", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
+
+    object_initialize_child(obj, "timer-irq-orgate",
+                            &s->irq_orgate, TYPE_OR_IRQ);
+
+    for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
+        object_initialize_child(obj, "timer[*]", &s->timer[i], TYPE_ARM_TIMER);
+    }
 }
 
 static void sp804_realize(DeviceState *dev, Error **errp)
 {
     SP804Timer *s = SP804_TIMER(dev);
 
+    object_property_set_int(OBJECT(&s->irq_orgate),
+                            "num-lines", 2, &error_fatal);
+    qdev_realize(DEVICE(&s->irq_orgate), NULL, &error_fatal);
+
     for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
-        s->irq_in[i] = qemu_allocate_irq(sp804_set_irq, s, i);
-        s->timer[i] = arm_timer_new(s->freq[i], s->irq_in[i]);
+        SysBusDevice *tmr = SYS_BUS_DEVICE(&s->timer[i]);
+
+        if (!sysbus_realize(tmr, errp)) {
+            return;
+        }
+        sysbus_connect_irq(tmr, 0, qdev_get_gpio_in(DEVICE(&s->irq_orgate), i));
     }
 }
 
 static Property sp804_properties[] = {
-    DEFINE_PROP_UINT32("freq0", SP804Timer, freq[0], 1000000),
-    DEFINE_PROP_UINT32("freq1", SP804Timer, freq[1], 1000000),
+    DEFINE_PROP_UINT32("freq0", SP804Timer, timer[0].freq, 1000000),
+    DEFINE_PROP_UINT32("freq1", SP804Timer, timer[1].freq, 1000000),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -342,8 +401,7 @@ struct IntegratorPIT {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
-    ArmTimer *timer[3];
-    qemu_irq irq_in[3];
+    ArmTimer timer[3];
     qemu_irq irq[3];
 };
 
@@ -406,7 +464,8 @@ static void icp_pit_init(Object *obj)
                             "timer-in", ARRAY_SIZE(s->timer));
 
     for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
-        s->timer[i] = arm_timer_new(tmr_freq[i], s->irq_in[i]);
+        object_initialize_child(obj, "timer[*]", &s->timer[i], TYPE_ARM_TIMER);
+        qdev_prop_set_uint32(DEVICE(&s->timer[i]), "freq", tmr_freq[i]);
         sysbus_init_irq(dev, &s->irq[i]);
     }
 
@@ -424,7 +483,10 @@ static void icp_pit_realize(DeviceState *dev, Error **errp)
     for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
         SysBusDevice *tmr = SYS_BUS_DEVICE(&s->timer[i]);
 
-        sysbus_connect_irq(tmr, i, qdev_get_gpio_in_named(dev, "timer-in", i));
+        if (!sysbus_realize(tmr, errp)) {
+            return;
+        }
+        sysbus_connect_irq(tmr, 0, qdev_get_gpio_in_named(dev, "timer-in", i));
     }
 }
 
@@ -437,6 +499,13 @@ static void icp_pit_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo arm_timer_types[] = {
     {
+        .name           = TYPE_ARM_TIMER,
+        .parent         = TYPE_SYS_BUS_DEVICE,
+        .instance_size  = sizeof(ArmTimer),
+        .instance_init  = arm_timer_init,
+        .instance_finalize = arm_timer_finalize,
+        .class_init     = arm_timer_class_init,
+    }, {
         .name           = TYPE_INTEGRATOR_PIT,
         .parent         = TYPE_SYS_BUS_DEVICE,
         .instance_size  = sizeof(IntegratorPIT),
diff --git a/hw/timer/Kconfig b/hw/timer/Kconfig
index 010be7ed1f..61b048f49c 100644
--- a/hw/timer/Kconfig
+++ b/hw/timer/Kconfig
@@ -1,6 +1,7 @@
 config ARM_TIMER
     bool
     select PTIMER
+    select OR_IRQ
 
 config ARM_MPTIMER
     bool
-- 
2.38.1

Re: [PATCH v2 17/19] hw/timer/arm_timer: QDev'ify ARM_TIMER

[Peter Maydell]

On Tue, 4 Jul 2023 at 15:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Introduce the ARM_TIMER sysbus device, exposing one output IRQ
> and a single MMIO region.
>
> arm_timer_new() is converted as QOM instance init()/finalize()
> handlers. Note in arm_timer_finalize() we release a ptimer handle
> which was previously leaked.
>
> ArmTimer is directly embedded into SP804Timer/IntegratorPIT,
> and is initialized as a QOM child.
>
> Since the timer frequency belongs to ARM_TIMER, have it hold the
> QOM property. SP804Timer/IntegratorPIT directly access it.
>
> For IntegratorPIT, each ARM_TIMER sysbus output IRQ is wired as
> input IRQ.
>
> For the SP804Timer, we add a TYPE_OR_IRQ to OR the ARM_TIMER sysbus
> output IRQs together, exposing a single output IRQ.
> The Kconfig entry have to select the OR_IRQ dependency.

How much did you test the migration compat handling in this patch?
TYPE_OR_IRQ has its own migration state, and I forget how
it works when you migrate from a machine without a device
to one that has it. Does it just work and the extra device
ends up with the same state it has at reset?

Anyway, we should list in the commit message all the
affected boards (whether that's "migration break" or
"migration compat, forwards only").

> +static int sp804_post_load(void *opaque, int version_id)
> +{
> +    SP804Timer *s = opaque;
> +
> +    if (version_id < 2) {
> +        for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
> +            qemu_set_irq(qdev_get_gpio_in(DEVICE(&s->irq_orgate), i),
> +                         s->mig_v1_level[i]);
> +        }
> +    }
> +    return 0;
> +}

Is it really OK to call qemu_set_irq() in a post_load
handler? This is going to end up causing the OR gate
to call qemu_set_irq() on whatever its output is connected
to, and there's no guarantee about migration order between
us and whatever that other device is...

thanks
-- PMM

[PATCH v2 18/19] hw/timer/arm_timer: Map ARM_TIMER MMIO regions into IntegratorPIT

[Philippe Mathieu-Daudé]

Instead of manually forwarding MMIO accesses to each ARM_TIMER,
let have the generic memory code dispatch that for us.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/timer/arm_timer.c | 43 ++++---------------------------------------
 1 file changed, 4 insertions(+), 39 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 8207723ab5..7b455aff4d 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -393,6 +393,7 @@ static void sp804_class_init(ObjectClass *klass, void *data)
 }
 
 /* Integrator/CP timer module.  */
+/* ??? Don't know the PrimeCell ID for this device.  */
 
 #define TYPE_INTEGRATOR_PIT "integrator_pit"
 OBJECT_DECLARE_SIMPLE_TYPE(IntegratorPIT, INTEGRATOR_PIT)
@@ -405,43 +406,6 @@ struct IntegratorPIT {
     qemu_irq irq[3];
 };
 
-static uint64_t icp_pit_read(void *opaque, hwaddr offset,
-                             unsigned size)
-{
-    IntegratorPIT *s = opaque;
-    int n;
-
-    /* ??? Don't know the PrimeCell ID for this device.  */
-    n = offset >> 8;
-    if (n > 2) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad timer %d\n", __func__, n);
-        return 0;
-    }
-
-    return arm_timer_read(&s->timer[n], offset & 0xff, size);
-}
-
-static void icp_pit_write(void *opaque, hwaddr offset,
-                          uint64_t value, unsigned size)
-{
-    IntegratorPIT *s = opaque;
-    int n;
-
-    n = offset >> 8;
-    if (n > 2) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad timer %d\n", __func__, n);
-        return;
-    }
-
-    arm_timer_write(&s->timer[n], offset & 0xff, value, size);
-}
-
-static const MemoryRegionOps icp_pit_ops = {
-    .read = icp_pit_read,
-    .write = icp_pit_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-};
-
 static void icp_pit_fwd_irq(void *opaque, int n, int level)
 {
     IntegratorPIT *s = opaque;
@@ -469,8 +433,7 @@ static void icp_pit_init(Object *obj)
         sysbus_init_irq(dev, &s->irq[i]);
     }
 
-    memory_region_init_io(&s->iomem, obj, &icp_pit_ops, s,
-                          "icp_pit", 0x1000);
+    memory_region_init(&s->iomem, obj, "icp_pit", 0x1000);
     sysbus_init_mmio(dev, &s->iomem);
     /* This device has no state to save/restore.  The component timers will
        save themselves.  */
@@ -487,6 +450,8 @@ static void icp_pit_realize(DeviceState *dev, Error **errp)
             return;
         }
         sysbus_connect_irq(tmr, 0, qdev_get_gpio_in_named(dev, "timer-in", i));
+        memory_region_add_subregion(&s->iomem, 0x100 * i,
+                                    sysbus_mmio_get_region(tmr, 0));
     }
 }
 
-- 
2.38.1

Re: [PATCH v2 18/19] hw/timer/arm_timer: Map ARM_TIMER MMIO regions into IntegratorPIT

[Richard Henderson]

On 7/4/23 16:50, Philippe Mathieu-Daudé wrote:
> Instead of manually forwarding MMIO accesses to each ARM_TIMER,
> let have the generic memory code dispatch that for us.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/timer/arm_timer.c | 43 ++++---------------------------------------
>   1 file changed, 4 insertions(+), 39 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 19/19] hw/timer/arm_timer: Map ARM_TIMER MMIO regions into SP804Timer

[Philippe Mathieu-Daudé]

Instead of manually forwarding MMIO accesses to each ARM_TIMER,
let have the generic memory code dispatch that for us.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/timer/arm_timer.c | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 7b455aff4d..f8d65732dc 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -262,15 +262,6 @@ static const uint8_t sp804_ids[] = {
 static uint64_t sp804_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
-    SP804Timer *s = opaque;
-
-    if (offset < 0x20) {
-        return arm_timer_read(&s->timer[0], offset, size);
-    }
-    if (offset < 0x40) {
-        return arm_timer_read(&s->timer[1], offset - 0x20, size);
-    }
-
     /* TimerPeriphID */
     if (offset >= 0xfe0 && offset <= 0xffc) {
         return sp804_ids[(offset - 0xfe0) >> 2];
@@ -294,18 +285,6 @@ static uint64_t sp804_read(void *opaque, hwaddr offset,
 static void sp804_write(void *opaque, hwaddr offset,
                         uint64_t value, unsigned size)
 {
-    SP804Timer *s = opaque;
-
-    if (offset < 0x20) {
-        arm_timer_write(&s->timer[0], offset, value, size);
-        return;
-    }
-
-    if (offset < 0x40) {
-        arm_timer_write(&s->timer[1], offset - 0x20, value, size);
-        return;
-    }
-
     /* Technically we could be writing to the Test Registers, but not likely */
     qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset %x\n",
                   __func__, (int)offset);
@@ -374,6 +353,8 @@ static void sp804_realize(DeviceState *dev, Error **errp)
             return;
         }
         sysbus_connect_irq(tmr, 0, qdev_get_gpio_in(DEVICE(&s->irq_orgate), i));
+        memory_region_add_subregion_overlap(&s->iomem, 0x20 * i,
+                                            sysbus_mmio_get_region(tmr, 0), 1);
     }
 }
 
-- 
2.38.1

Re: [PATCH v2 19/19] hw/timer/arm_timer: Map ARM_TIMER MMIO regions into SP804Timer

[Richard Henderson]

On 7/4/23 16:50, Philippe Mathieu-Daudé wrote:
> Instead of manually forwarding MMIO accesses to each ARM_TIMER,
> let have the generic memory code dispatch that for us.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/timer/arm_timer.c | 23 ++---------------------
>   1 file changed, 2 insertions(+), 21 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 00/19] hw/timer/arm_timer: QOM'ify ARM_TIMER and correct sysbus/irq in ICP_PIT

[Philippe Mathieu-Daudé]

On 4/7/23 16:49, Philippe Mathieu-Daudé wrote:
> This series converts the ARM_TIMER model to QOM.
> 
> Doing so we also correct an abuse of SysBus IRQ in
> the ICP PIT model.
> 
> Since v1:
> - Added pm215's R-b tags
> - Addressed Mark/Peter review comments
>    - Drop '*State' suffix from structure names
>    - Use OR-IRQ gate

(forgot to mention migration normally taken care of)

>    - Drop sp804_unrealize()
>    - Implement Resettable API
> - MMIO-map timer regions into parents
Also, from here it should be easier to add the
ARM AP804 used by BCM2835; see Thomas Venriès's
patch v2:
https://patchwork.kernel.org/project/qemu-devel/patch/1507728290-6062-1-git-send-email-thomas.venries@gmail.com/