diff mbox series

[12/15] hw/timer/arm_timer: Pass timer output IRQ as parameter to arm_timer_new

Message ID 20230531203559.29140-13-philmd@linaro.org
State Superseded
Headers show
Series hw/timer/arm_timer: QOM'ify ARM_TIMER and correct sysbus/irq in ICP_PIT | expand

Commit Message

Philippe Mathieu-Daudé May 31, 2023, 8:35 p.m. UTC 
Both SP804State/IcpPitState peek at ArmTimerState internal state.
This is fine so far but we want to convert ArmTimerState to QOM
where peeking at QOM state internal should be avoided.
ArmTimerState's IRQ is just a pointer, so we can pass/set it via
argument, avoiding accessing ArmTimerState internal state except
from the arm_timer_*() methods.

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

Comments

Peter Maydell June 8, 2023, 3 p.m. UTC | #1 
On Wed, 31 May 2023 at 21:37, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Both SP804State/IcpPitState peek at ArmTimerState internal state.
> This is fine so far but we want to convert ArmTimerState to QOM
> where peeking at QOM state internal should be avoided.
> ArmTimerState's IRQ is just a pointer, so we can pass/set it via
> argument, avoiding accessing ArmTimerState internal state except
> from the arm_timer_*() methods.

If we convert ArmTimerState to QOM then shouldn't the
irq become a sysbus IRQ on the ArmTimerState object,
and the looking at the internals go away automatically?

-- PMM
Philippe Mathieu-Daudé July 4, 2023, 2:29 p.m. UTC | #2 
On 8/6/23 17:00, Peter Maydell wrote:
> On Wed, 31 May 2023 at 21:37, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Both SP804State/IcpPitState peek at ArmTimerState internal state.
>> This is fine so far but we want to convert ArmTimerState to QOM
>> where peeking at QOM state internal should be avoided.
>> ArmTimerState's IRQ is just a pointer, so we can pass/set it via
>> argument, avoiding accessing ArmTimerState internal state except
>> from the arm_timer_*() methods.
> 
> If we convert ArmTimerState to QOM then shouldn't the
> irq become a sysbus IRQ on the ArmTimerState object,
> and the looking at the internals go away automatically?

This is what happen two patches later in "hw/timer/arm_timer:
QOM'ify ARM_TIMER" (which is hard to split).

At this step ArmTimerState is not yet a QOM object (neither
SysBus), so we can not create a SysBus IRQ.
diff mbox series

Patch

diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 19789ad2c4..6f444e1789 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -177,7 +177,7 @@  static void arm_timer_reset(ArmTimerState *s)
     s->control = TIMER_CTRL_IE;
 }
 
-static ArmTimerState *arm_timer_new(uint32_t freq)
+static ArmTimerState *arm_timer_new(uint32_t freq, qemu_irq irq_out)
 {
     ArmTimerState *s;
 
@@ -185,6 +185,7 @@  static ArmTimerState *arm_timer_new(uint32_t freq)
     s->freq = freq;
     arm_timer_reset(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;
@@ -207,6 +208,7 @@  struct SP804State {
     uint32_t freq[2];
     int level[2];
     qemu_irq irq;
+    qemu_irq irq_in[2];
 };
 
 static const uint8_t sp804_ids[] = {
@@ -309,8 +311,8 @@  static void sp804_realize(DeviceState *dev, Error **errp)
     SP804State *s = SP804(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]);
     }
 }
 
@@ -319,7 +321,7 @@  static void sp804_unrealize(DeviceState *dev)
     SP804State *s = SP804(dev);
 
     for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
-        qemu_free_irq(s->timer[i]->irq);
+        qemu_free_irq(s->irq_in[i]);
     }
 }
 
@@ -349,6 +351,7 @@  struct IntegratorPitState {
 
     MemoryRegion iomem;
     ArmTimerState *timer[3];
+    qemu_irq irq_in[3];
 };
 
 static uint64_t icp_pit_read(void *opaque, hwaddr offset,
@@ -400,8 +403,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,