Re: [PATCH 15/15] hw/timer/arm_timer: QOM'ify ARM_TIMER

Re: [PATCH 15/15] hw/timer/arm_timer: QOM'ify ARM_TIMER

[Mark Cave-Ayland]

On 31/05/2023 21:35, Philippe Mathieu-Daudé wrote:

> Introduce the ARM_TIMER sysbus device.
> 
> 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.
> 
> ArmTimerState is directly embedded into SP804State/IcpPitState,
> and is initialized as a QOM child.
> 
> Since the timer frequency belongs to ARM_TIMER, have it hold the
> QOM property. SP804State/IcpPitState directly access it.
> 
> Similarly the SP804State/IcpPitState input IRQ becomes the
> ARM_TIMER sysbus output IRQ.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/timer/arm_timer.c | 109 +++++++++++++++++++++++++++----------------
>   1 file changed, 70 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
> index 82123b40c0..a929fbba62 100644
> --- a/hw/timer/arm_timer.c
> +++ b/hw/timer/arm_timer.c
> @@ -17,6 +17,7 @@
>   #include "qemu/module.h"
>   #include "qemu/log.h"
>   #include "qom/object.h"
> +#include "qapi/error.h"
>   
>   /* Common timer implementation.  */
>   
> @@ -29,14 +30,18 @@
>   #define TIMER_CTRL_PERIODIC     (1 << 6)
>   #define TIMER_CTRL_ENABLE       (1 << 7)
>   
> -typedef struct {
> +#define TYPE_ARM_TIMER "arm-timer"
> +OBJECT_DECLARE_SIMPLE_TYPE(ArmTimerState, ARM_TIMER)

As per our QOM guidelines ArmTimerState and the OBJECT_* macro should live in a 
separate header file.

> +struct ArmTimerState {
> +    SysBusDevice parent_obj;

And don't forget to add a blank line here too.

>       ptimer_state *timer;
>       uint32_t control;
>       uint32_t limit;
>       uint32_t freq;
>       int int_level;
>       qemu_irq irq;
> -} ArmTimerState;
> +};
>   
>   /* Check all active timers, and schedule the next timer interrupt.  */
>   
> @@ -172,23 +177,42 @@ static const VMStateDescription vmstate_arm_timer = {
>       }
>   };
>   
> -static void arm_timer_reset(ArmTimerState *s)
> +static void arm_timer_reset(DeviceState *dev)
>   {
> +    ArmTimerState *s = ARM_TIMER(dev);
> +
>       s->control = TIMER_CTRL_IE;
>   }

If you're currently set up to test the ARM timers with these changes, is it worth 
considering converting this to use the Resettable interface at the same time?

> -static ArmTimerState *arm_timer_new(uint32_t freq, qemu_irq irq_out)
> +static void arm_timer_init(Object *obj)
>   {
> -    ArmTimerState *s;
> +    ArmTimerState *s = ARM_TIMER(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>   
> -    s = g_new0(ArmTimerState, 1);
> -    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;
> +
> +    sysbus_init_irq(sbd, &s->irq);
> +}
> +
> +static void arm_timer_finalize(Object *obj)
> +{
> +    ArmTimerState *s = ARM_TIMER(obj);
> +
> +    ptimer_free(s->timer);
> +}
> +
> +static Property arm_timer_properties[] = {
> +    DEFINE_PROP_UINT32("freq", ArmTimerState, freq, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void arm_timer_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->reset = arm_timer_reset;
> +    dc->vmsd = &vmstate_arm_timer;
> +    device_class_set_props(dc, arm_timer_properties);
>   }
>   
>   /*
> @@ -204,11 +228,9 @@ struct SP804State {
>       SysBusDevice parent_obj;
>   
>       MemoryRegion iomem;
> -    ArmTimerState *timer[2];
> -    uint32_t freq[2];
> +    ArmTimerState timer[2];
>       int level[2];
>       qemu_irq irq;
> -    qemu_irq irq_in[2];
>   };
>   
>   static const uint8_t sp804_ids[] = {
> @@ -233,10 +255,10 @@ static uint64_t sp804_read(void *opaque, hwaddr offset,
>       SP804State *s = opaque;
>   
>       if (offset < 0x20) {
> -        return arm_timer_read(s->timer[0], offset);
> +        return arm_timer_read(&s->timer[0], offset);
>       }
>       if (offset < 0x40) {
> -        return arm_timer_read(s->timer[1], offset - 0x20);
> +        return arm_timer_read(&s->timer[1], offset - 0x20);
>       }
>   
>       /* TimerPeriphID */
> @@ -265,12 +287,12 @@ static void sp804_write(void *opaque, hwaddr offset,
>       SP804State *s = opaque;
>   
>       if (offset < 0x20) {
> -        arm_timer_write(s->timer[0], offset, value);
> +        arm_timer_write(&s->timer[0], offset, value);
>           return;
>       }
>   
>       if (offset < 0x40) {
> -        arm_timer_write(s->timer[1], offset - 0x20, value);
> +        arm_timer_write(&s->timer[1], offset - 0x20, value);
>           return;
>       }
>   
> @@ -304,6 +326,12 @@ static void sp804_init(Object *obj)
>       memory_region_init_io(&s->iomem, obj, &sp804_ops, s,
>                             "sp804", 0x1000);
>       sysbus_init_mmio(sbd, &s->iomem);
> +
> +    qdev_init_gpio_in_named(DEVICE(obj), sp804_set_irq,
> +                            "timer-in", ARRAY_SIZE(s->timer));
> +    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)
> @@ -311,23 +339,17 @@ static void sp804_realize(DeviceState *dev, Error **errp)
>       SP804State *s = SP804(dev);
>   
>       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]);
> -    }
> -}
> -
> -static void sp804_unrealize(DeviceState *dev)
> -{
> -    SP804State *s = SP804(dev);
> -
> -    for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
> -        qemu_free_irq(s->irq_in[i]);
> +        if (!sysbus_realize(SYS_BUS_DEVICE(&s->timer[i]), errp)) {
> +            return;
> +        }
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer[i]), 0,
> +                           qdev_get_gpio_in_named(dev, "timer-in", i));
>       }
>   }
>   
>   static Property sp804_properties[] = {
> -    DEFINE_PROP_UINT32("freq0", SP804State, freq[0], 1000000),
> -    DEFINE_PROP_UINT32("freq1", SP804State, freq[1], 1000000),
> +    DEFINE_PROP_UINT32("freq0", SP804State, timer[0].freq, 1000000),
> +    DEFINE_PROP_UINT32("freq1", SP804State, timer[1].freq, 1000000),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> @@ -336,7 +358,6 @@ static void sp804_class_init(ObjectClass *klass, void *data)
>       DeviceClass *k = DEVICE_CLASS(klass);
>   
>       k->realize = sp804_realize;
> -    k->unrealize = sp804_unrealize;
>       device_class_set_props(k, sp804_properties);
>       k->vmsd = &vmstate_sp804;
>   }
> @@ -350,8 +371,7 @@ struct IntegratorPitState {
>       SysBusDevice parent_obj;
>   
>       MemoryRegion iomem;
> -    ArmTimerState *timer[3];
> -    qemu_irq irq_in[3];
> +    ArmTimerState timer[3];
>       qemu_irq irq[3];
>   };
>   
> @@ -368,7 +388,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);
>   }
>   
>   static void icp_pit_write(void *opaque, hwaddr offset,
> @@ -383,7 +403,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);
>   }
>   
>   static const MemoryRegionOps icp_pit_ops = {
> @@ -414,7 +434,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]);
>       }
>   
> @@ -430,7 +451,10 @@ static void icp_pit_realize(DeviceState *dev, Error **errp)
>       IntegratorPitState *s = INTEGRATOR_PIT(dev);
>   
>       for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
> -        sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
> +        if (!sysbus_realize(SYS_BUS_DEVICE(&s->timer[i]), errp)) {
> +            return;
> +        }
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer[i]), 0,
>                              qdev_get_gpio_in_named(dev, "timer-in", i));
>       }
>   }
> @@ -444,6 +468,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(ArmTimerState),
> +        .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(IntegratorPitState),


ATB,

Mark.

Re: [PATCH 15/15] hw/timer/arm_timer: QOM'ify ARM_TIMER

[Mark Cave-Ayland]

On 03/06/2023 19:07, Mark Cave-Ayland wrote:

> On 31/05/2023 21:35, Philippe Mathieu-Daudé wrote:
> 
>> Introduce the ARM_TIMER sysbus device.
>>
>> 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.
>>
>> ArmTimerState is directly embedded into SP804State/IcpPitState,
>> and is initialized as a QOM child.
>>
>> Since the timer frequency belongs to ARM_TIMER, have it hold the
>> QOM property. SP804State/IcpPitState directly access it.
>>
>> Similarly the SP804State/IcpPitState input IRQ becomes the
>> ARM_TIMER sysbus output IRQ.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/timer/arm_timer.c | 109 +++++++++++++++++++++++++++----------------
>>   1 file changed, 70 insertions(+), 39 deletions(-)
>>
>> diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
>> index 82123b40c0..a929fbba62 100644
>> --- a/hw/timer/arm_timer.c
>> +++ b/hw/timer/arm_timer.c
>> @@ -17,6 +17,7 @@
>>   #include "qemu/module.h"
>>   #include "qemu/log.h"
>>   #include "qom/object.h"
>> +#include "qapi/error.h"
>>   /* Common timer implementation.  */
>> @@ -29,14 +30,18 @@
>>   #define TIMER_CTRL_PERIODIC     (1 << 6)
>>   #define TIMER_CTRL_ENABLE       (1 << 7)
>> -typedef struct {
>> +#define TYPE_ARM_TIMER "arm-timer"
>> +OBJECT_DECLARE_SIMPLE_TYPE(ArmTimerState, ARM_TIMER)
> 
> As per our QOM guidelines ArmTimerState and the OBJECT_* macro should live in a 
> separate header file.

Ah wait: does "ArmTimerState is directly embedded into SP804State/IcpPitState, and is 
initialized as a QOM child." mean that ARM_TIMER is never instantiated externally?

>> +struct ArmTimerState {
>> +    SysBusDevice parent_obj;
> 
> And don't forget to add a blank line here too.
> 
>>       ptimer_state *timer;
>>       uint32_t control;
>>       uint32_t limit;
>>       uint32_t freq;
>>       int int_level;
>>       qemu_irq irq;
>> -} ArmTimerState;
>> +};
>>   /* Check all active timers, and schedule the next timer interrupt.  */
>> @@ -172,23 +177,42 @@ static const VMStateDescription vmstate_arm_timer = {
>>       }
>>   };
>> -static void arm_timer_reset(ArmTimerState *s)
>> +static void arm_timer_reset(DeviceState *dev)
>>   {
>> +    ArmTimerState *s = ARM_TIMER(dev);
>> +
>>       s->control = TIMER_CTRL_IE;
>>   }
> 
> If you're currently set up to test the ARM timers with these changes, is it worth 
> considering converting this to use the Resettable interface at the same time?
> 
>> -static ArmTimerState *arm_timer_new(uint32_t freq, qemu_irq irq_out)
>> +static void arm_timer_init(Object *obj)
>>   {
>> -    ArmTimerState *s;
>> +    ArmTimerState *s = ARM_TIMER(obj);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> -    s = g_new0(ArmTimerState, 1);
>> -    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;
>> +
>> +    sysbus_init_irq(sbd, &s->irq);
>> +}
>> +
>> +static void arm_timer_finalize(Object *obj)
>> +{
>> +    ArmTimerState *s = ARM_TIMER(obj);
>> +
>> +    ptimer_free(s->timer);
>> +}
>> +
>> +static Property arm_timer_properties[] = {
>> +    DEFINE_PROP_UINT32("freq", ArmTimerState, freq, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void arm_timer_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +
>> +    dc->reset = arm_timer_reset;
>> +    dc->vmsd = &vmstate_arm_timer;
>> +    device_class_set_props(dc, arm_timer_properties);
>>   }
>>   /*
>> @@ -204,11 +228,9 @@ struct SP804State {
>>       SysBusDevice parent_obj;
>>       MemoryRegion iomem;
>> -    ArmTimerState *timer[2];
>> -    uint32_t freq[2];
>> +    ArmTimerState timer[2];
>>       int level[2];
>>       qemu_irq irq;
>> -    qemu_irq irq_in[2];
>>   };
>>   static const uint8_t sp804_ids[] = {
>> @@ -233,10 +255,10 @@ static uint64_t sp804_read(void *opaque, hwaddr offset,
>>       SP804State *s = opaque;
>>       if (offset < 0x20) {
>> -        return arm_timer_read(s->timer[0], offset);
>> +        return arm_timer_read(&s->timer[0], offset);
>>       }
>>       if (offset < 0x40) {
>> -        return arm_timer_read(s->timer[1], offset - 0x20);
>> +        return arm_timer_read(&s->timer[1], offset - 0x20);
>>       }
>>       /* TimerPeriphID */
>> @@ -265,12 +287,12 @@ static void sp804_write(void *opaque, hwaddr offset,
>>       SP804State *s = opaque;
>>       if (offset < 0x20) {
>> -        arm_timer_write(s->timer[0], offset, value);
>> +        arm_timer_write(&s->timer[0], offset, value);
>>           return;
>>       }
>>       if (offset < 0x40) {
>> -        arm_timer_write(s->timer[1], offset - 0x20, value);
>> +        arm_timer_write(&s->timer[1], offset - 0x20, value);
>>           return;
>>       }
>> @@ -304,6 +326,12 @@ static void sp804_init(Object *obj)
>>       memory_region_init_io(&s->iomem, obj, &sp804_ops, s,
>>                             "sp804", 0x1000);
>>       sysbus_init_mmio(sbd, &s->iomem);
>> +
>> +    qdev_init_gpio_in_named(DEVICE(obj), sp804_set_irq,
>> +                            "timer-in", ARRAY_SIZE(s->timer));
>> +    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)
>> @@ -311,23 +339,17 @@ static void sp804_realize(DeviceState *dev, Error **errp)
>>       SP804State *s = SP804(dev);
>>       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]);
>> -    }
>> -}
>> -
>> -static void sp804_unrealize(DeviceState *dev)
>> -{
>> -    SP804State *s = SP804(dev);
>> -
>> -    for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
>> -        qemu_free_irq(s->irq_in[i]);
>> +        if (!sysbus_realize(SYS_BUS_DEVICE(&s->timer[i]), errp)) {
>> +            return;
>> +        }
>> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer[i]), 0,
>> +                           qdev_get_gpio_in_named(dev, "timer-in", i));
>>       }
>>   }
>>   static Property sp804_properties[] = {
>> -    DEFINE_PROP_UINT32("freq0", SP804State, freq[0], 1000000),
>> -    DEFINE_PROP_UINT32("freq1", SP804State, freq[1], 1000000),
>> +    DEFINE_PROP_UINT32("freq0", SP804State, timer[0].freq, 1000000),
>> +    DEFINE_PROP_UINT32("freq1", SP804State, timer[1].freq, 1000000),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>> @@ -336,7 +358,6 @@ static void sp804_class_init(ObjectClass *klass, void *data)
>>       DeviceClass *k = DEVICE_CLASS(klass);
>>       k->realize = sp804_realize;
>> -    k->unrealize = sp804_unrealize;
>>       device_class_set_props(k, sp804_properties);
>>       k->vmsd = &vmstate_sp804;
>>   }
>> @@ -350,8 +371,7 @@ struct IntegratorPitState {
>>       SysBusDevice parent_obj;
>>       MemoryRegion iomem;
>> -    ArmTimerState *timer[3];
>> -    qemu_irq irq_in[3];
>> +    ArmTimerState timer[3];
>>       qemu_irq irq[3];
>>   };
>> @@ -368,7 +388,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);
>>   }
>>   static void icp_pit_write(void *opaque, hwaddr offset,
>> @@ -383,7 +403,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);
>>   }
>>   static const MemoryRegionOps icp_pit_ops = {
>> @@ -414,7 +434,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]);
>>       }
>> @@ -430,7 +451,10 @@ static void icp_pit_realize(DeviceState *dev, Error **errp)
>>       IntegratorPitState *s = INTEGRATOR_PIT(dev);
>>       for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
>> -        sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
>> +        if (!sysbus_realize(SYS_BUS_DEVICE(&s->timer[i]), errp)) {
>> +            return;
>> +        }
>> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer[i]), 0,
>>                              qdev_get_gpio_in_named(dev, "timer-in", i));
>>       }
>>   }
>> @@ -444,6 +468,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(ArmTimerState),
>> +        .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(IntegratorPitState),


ATB,

Mark.

Re: [PATCH 15/15] hw/timer/arm_timer: QOM'ify ARM_TIMER

[Philippe Mathieu-Daudé]

On 3/6/23 20:12, Mark Cave-Ayland wrote:
> On 03/06/2023 19:07, Mark Cave-Ayland wrote:
> 
>> On 31/05/2023 21:35, Philippe Mathieu-Daudé wrote:
>>
>>> Introduce the ARM_TIMER sysbus device.
>>>
>>> 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.
>>>
>>> ArmTimerState is directly embedded into SP804State/IcpPitState,
>>> and is initialized as a QOM child.
>>>
>>> Since the timer frequency belongs to ARM_TIMER, have it hold the
>>> QOM property. SP804State/IcpPitState directly access it.
>>>
>>> Similarly the SP804State/IcpPitState input IRQ becomes the
>>> ARM_TIMER sysbus output IRQ.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/timer/arm_timer.c | 109 +++++++++++++++++++++++++++----------------
>>>   1 file changed, 70 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
>>> index 82123b40c0..a929fbba62 100644
>>> --- a/hw/timer/arm_timer.c
>>> +++ b/hw/timer/arm_timer.c
>>> @@ -17,6 +17,7 @@
>>>   #include "qemu/module.h"
>>>   #include "qemu/log.h"
>>>   #include "qom/object.h"
>>> +#include "qapi/error.h"
>>>   /* Common timer implementation.  */
>>> @@ -29,14 +30,18 @@
>>>   #define TIMER_CTRL_PERIODIC     (1 << 6)
>>>   #define TIMER_CTRL_ENABLE       (1 << 7)
>>> -typedef struct {
>>> +#define TYPE_ARM_TIMER "arm-timer"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArmTimerState, ARM_TIMER)
>>
>> As per our QOM guidelines ArmTimerState and the OBJECT_* macro should 
>> live in a separate header file.
> 
> Ah wait: does "ArmTimerState is directly embedded into 
> SP804State/IcpPitState, and is initialized as a QOM child." mean that 
> ARM_TIMER is never instantiated externally?

Correct, while the type is exposed as any QOM type, it is internal to
the two devices, thus local to this unit.

I don't mind exposing the state to have a consistent QOM style.

What was discussed with Alex is:
- We don't need to convert all non-QOM devices, but
- Heterogeneous machines must contain only QOM devices;

- If a non-QOM device forces incorrect API use or abuses,
   better convert it.

>>> +struct ArmTimerState {
>>> +    SysBusDevice parent_obj;
>>
>> And don't forget to add a blank line here too.

OK.

>>>       ptimer_state *timer;
>>>       uint32_t control;
>>>       uint32_t limit;
>>>       uint32_t freq;
>>>       int int_level;
>>>       qemu_irq irq;
>>> -} ArmTimerState;
>>> +};

Re: [PATCH 15/15] hw/timer/arm_timer: QOM'ify ARM_TIMER

[Philippe Mathieu-Daudé]

On 3/6/23 20:07, Mark Cave-Ayland wrote:
> On 31/05/2023 21:35, Philippe Mathieu-Daudé wrote:
> 
>> Introduce the ARM_TIMER sysbus device.
>>
>> 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.
>>
>> ArmTimerState is directly embedded into SP804State/IcpPitState,
>> and is initialized as a QOM child.
>>
>> Since the timer frequency belongs to ARM_TIMER, have it hold the
>> QOM property. SP804State/IcpPitState directly access it.
>>
>> Similarly the SP804State/IcpPitState input IRQ becomes the
>> ARM_TIMER sysbus output IRQ.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/timer/arm_timer.c | 109 +++++++++++++++++++++++++++----------------
>>   1 file changed, 70 insertions(+), 39 deletions(-)


>> -static void arm_timer_reset(ArmTimerState *s)
>> +static void arm_timer_reset(DeviceState *dev)
>>   {
>> +    ArmTimerState *s = ARM_TIMER(dev);
>> +
>>       s->control = TIMER_CTRL_IE;
>>   }
> 
> If you're currently set up to test the ARM timers with these changes, is 
> it worth considering converting this to use the Resettable interface at 
> the same time?

Good point. Then ARM_TIMER doesn't need to inherit from SysBus: if the
parent device resets it explicitly, it can be a plan QDev.

Peter, what do you think?

Even generically, I wonder if a QDev could resets all its QOM children,
propagating each ResetType.

Re: [PATCH 15/15] hw/timer/arm_timer: QOM'ify ARM_TIMER

[Peter Maydell]

On Mon, 5 Jun 2023 at 11:20, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 3/6/23 20:07, Mark Cave-Ayland wrote:
> > On 31/05/2023 21:35, Philippe Mathieu-Daudé wrote:
> >
> >> Introduce the ARM_TIMER sysbus device.
> >>
> >> 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.
> >>
> >> ArmTimerState is directly embedded into SP804State/IcpPitState,
> >> and is initialized as a QOM child.
> >>
> >> Since the timer frequency belongs to ARM_TIMER, have it hold the
> >> QOM property. SP804State/IcpPitState directly access it.
> >>
> >> Similarly the SP804State/IcpPitState input IRQ becomes the
> >> ARM_TIMER sysbus output IRQ.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> ---
> >>   hw/timer/arm_timer.c | 109 +++++++++++++++++++++++++++----------------
> >>   1 file changed, 70 insertions(+), 39 deletions(-)
>
>
> >> -static void arm_timer_reset(ArmTimerState *s)
> >> +static void arm_timer_reset(DeviceState *dev)
> >>   {
> >> +    ArmTimerState *s = ARM_TIMER(dev);
> >> +
> >>       s->control = TIMER_CTRL_IE;
> >>   }
> >
> > If you're currently set up to test the ARM timers with these changes, is
> > it worth considering converting this to use the Resettable interface at
> > the same time?
>
> Good point. Then ARM_TIMER doesn't need to inherit from SysBus: if the
> parent device resets it explicitly, it can be a plan QDev.
>
> Peter, what do you think?

I'm not a super-fan of either plain qdevs or devices explicitly
resetting other devices. What we have today isn't great
(reset along the sysbus tree) but I feel like ad-hoc deviations
from it don't help and arguably hinder in any future attempts
to attack the problem more systematically.

> Even generically, I wonder if a QDev could resets all its QOM children,
> propagating each ResetType.

Propagating reset along the QOM tree is something I've thought
about, yes. The other option is to have a 'reset tree' (distinct
from both the QOM tree and the 'bus tree', but perhaps defaulting
to the QOM tree if not explicitly set otherwise). As usual, the
difficulty is the transition from the current setup to any
proposed new reset handling model. I haven't really had time
to think about this recently.

-- PMM

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

[Peter Maydell]

On Wed, 31 May 2023 at 21:36, Philippe Mathieu-Daudé <philmd@linaro.org> 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>

thanks
-- PMM

Re: [PATCH 02/15] hw/timer/arm_timer: Move SP804 code around

[Peter Maydell]

On Wed, 31 May 2023 at 21:36, Philippe Mathieu-Daudé <philmd@linaro.org> 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>
> ---
>  hw/timer/arm_timer.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 03/15] hw/timer/arm_timer: Add missing sp804_unrealize() handler

[Peter Maydell]

On Wed, 31 May 2023 at 21:36, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Release the IRQs allocated in sp804_realize() in the
> corresponding sp804_unrealize() handler.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/timer/arm_timer.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
> index 36f6586f80..5caf42649a 100644
> --- a/hw/timer/arm_timer.c
> +++ b/hw/timer/arm_timer.c
> @@ -309,6 +309,15 @@ static void sp804_realize(DeviceState *dev, Error **errp)
>      s->timer[1]->irq = qemu_allocate_irq(sp804_set_irq, s, 1);
>  }
>
> +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);
> +    }
> +}

I don't really see the purpose in this. It doesn't actually
avoid a leak if we ever destroy an SP804, because
s->timer[i] itself is memory allocated by arm_timer_init()
which we don't clean up (the arm_timer_state not being
a qdev). If we did convert arm_timer_state to qdev
then these interrupts should turn into being sysbus irqs
and gpio inputs on the relevant devices. (In fact if you
were willing to take the migration-compat hit you
could just have the sp804 create a 2 input OR gate and
wire the arm_timer irqs up to it and use the output
of the OR gate as the sp804 outbound interrupt line.)

thanks
-- PMM

Re: [PATCH 04/15] hw/timer/arm_timer: Remove pointless cast from void *

[Peter Maydell]

On Wed, 31 May 2023 at 21:36, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/timer/arm_timer.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 05/15] hw/timer/arm_timer: CamelCase rename icp_pit_state -> IntegratorPitState

[Peter Maydell]

On Wed, 31 May 2023 at 21:36, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Following docs/devel/style.rst guidelines, rename icp_pit_state
> using CamelCase as IntegratorPitState.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

though PIT here is an acronym so I think style would prefer
IntegratorPITState ?

Either way
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 06/15] hw/timer/arm_timer: CamelCase rename arm_timer_state -> ArmTimerState

[Peter Maydell]

On Wed, 31 May 2023 at 21:36, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Following docs/devel/style.rst guidelines, rename arm_timer_state
> as ArmTimerState.

Ending the type name with 'State' is also a little old fashioned.
But anyway
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 07/15] hw/timer/arm_timer: Extract arm_timer_reset()

[Peter Maydell]

On Wed, 31 May 2023 at 21:36, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Extract arm_timer_reset() before converting this model to QOM/QDev
> in few commits. This will become our DeviceReset handler.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/timer/arm_timer.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
> index 2cd8c99b4e..54318d0a57 100644
> --- a/hw/timer/arm_timer.c
> +++ b/hw/timer/arm_timer.c
> @@ -172,13 +172,18 @@ static const VMStateDescription vmstate_arm_timer = {
>      }
>  };
>
> +static void arm_timer_reset(ArmTimerState *s)
> +{
> +    s->control = TIMER_CTRL_IE;
> +}

Reset also should zero s->limit and s->int_level.
(in arm_timer_init() this was implicit in the g_new0()).)

thanks
-- PMM

Re: [PATCH 08/15] hw/timer/arm_timer: Rename arm_timer_init() -> arm_timer_new()

[Peter Maydell]

On Wed, 31 May 2023 at 21:36, Philippe Mathieu-Daudé <philmd@linaro.org> 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.

The other convention is that foo_new() does allocate-and-init
and foo_init() is init-in-place. But we get to the same
place either way.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

(It would be nice to do in-place rather than the
allocation here at some point.)

thanks
-- PMM

Re: [PATCH 09/15] hw/timer/arm_timer: Convert ArmTimerState::freq to uint32_t type

[Peter Maydell]

On Wed, 31 May 2023 at 21:36, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> In preparation of accessing ArmTimerState::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>
> ---
>  hw/timer/arm_timer.c | 2 +-

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 10/15] hw/timer/arm_timer: Use array of frequency in SP804State

[Peter Maydell]

On Wed, 31 May 2023 at 21:36, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> SP804State 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>

thanks
-- PMM

Re: [PATCH 11/15] hw/timer/arm_timer: Iterate on timers using for() loop statement

[Peter Maydell]

On Wed, 31 May 2023 at 21:37, Philippe Mathieu-Daudé <philmd@linaro.org> 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>

thanks
-- PMM

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

[Peter Maydell]

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

Re: [PATCH 13/15] hw/timer/arm_timer: Fix misuse of SysBus IRQ in IcpPitState

[Peter Maydell]

On Wed, 31 May 2023 at 21:37, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> SysBus IRQ are *output* IRQs. As some sort of simplification
> to avoid to forward it, IcpPitState misuses it as ARM timer
> input IRQ. Fix that by using a simple IRQ forwarder handler.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@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 6f444e1789..874f9b63bc 100644
> --- a/hw/timer/arm_timer.c
> +++ b/hw/timer/arm_timer.c
> @@ -352,6 +352,7 @@ struct IntegratorPitState {
>      MemoryRegion iomem;
>      ArmTimerState *timer[3];
>      qemu_irq irq_in[3];
> +    qemu_irq irq[3];
>  };
>
>  static uint64_t icp_pit_read(void *opaque, hwaddr offset,
> @@ -391,6 +392,13 @@ static const MemoryRegionOps icp_pit_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> +static void icp_pit_fwd_irq(void *opaque, int n, int level)
> +{
> +    IntegratorPitState *s = opaque;
> +
> +    qemu_set_irq(s->irq[n], level);
> +}
> +
>  static void icp_pit_init(Object *obj)
>  {
>      static const uint32_t tmr_freq[] = {
> @@ -402,9 +410,14 @@ static void icp_pit_init(Object *obj)
>      IntegratorPitState *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));
>      }

This feels a bit clunky but I think it's what we have to do,
since the various _pass_ APIs for forwarding GPIOs and
IRQs from a container to an inner device only work with
an entire set of IRQs.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 07/15] hw/timer/arm_timer: Extract arm_timer_reset()

[Philippe Mathieu-Daudé]

On 8/6/23 16:46, Peter Maydell wrote:
> On Wed, 31 May 2023 at 21:36, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Extract arm_timer_reset() before converting this model to QOM/QDev
>> in few commits. This will become our DeviceReset handler.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/timer/arm_timer.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)


>> +static void arm_timer_reset(ArmTimerState *s)
>> +{
>> +    s->control = TIMER_CTRL_IE;
>> +}
> 
> Reset also should zero s->limit and s->int_level.
> (in arm_timer_init() this was implicit in the g_new0()).)

Oops I missed that, thanks!

Re: [PATCH 13/15] hw/timer/arm_timer: Fix misuse of SysBus IRQ in IcpPitState

[Philippe Mathieu-Daudé]

On 8/6/23 17:09, Peter Maydell wrote:
> On Wed, 31 May 2023 at 21:37, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> SysBus IRQ are *output* IRQs. As some sort of simplification
>> to avoid to forward it, IcpPitState misuses it as ARM timer
>> input IRQ. Fix that by using a simple IRQ forwarder handler.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/timer/arm_timer.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)


>> +static void icp_pit_fwd_irq(void *opaque, int n, int level)
>> +{
>> +    IntegratorPitState *s = opaque;
>> +
>> +    qemu_set_irq(s->irq[n], level);
>> +}
>> +
>>   static void icp_pit_init(Object *obj)
>>   {
>>       static const uint32_t tmr_freq[] = {
>> @@ -402,9 +410,14 @@ static void icp_pit_init(Object *obj)
>>       IntegratorPitState *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));
>>       }
> 
> This feels a bit clunky but I think it's what we have to do,
> since the various _pass_ APIs for forwarding GPIOs and
> IRQs from a container to an inner device only work with
> an entire set of IRQs.

Indeed. sysbus_pass_irq() could resolve the last unused IRQ index
and amend starting at that index, but this doesn't seem a lot of
need for a such use (so far). We might consider it if we ever merge
sysbus IRQ API into qdev.

> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks!

Phil.

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

[Philippe Mathieu-Daudé]

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.

Re: [PATCH 03/15] hw/timer/arm_timer: Add missing sp804_unrealize() handler

[Philippe Mathieu-Daudé]

On 8/6/23 16:41, Peter Maydell wrote:
> On Wed, 31 May 2023 at 21:36, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Release the IRQs allocated in sp804_realize() in the
>> corresponding sp804_unrealize() handler.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/timer/arm_timer.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)


>> +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);
>> +    }
>> +}
> 
> I don't really see the purpose in this. It doesn't actually
> avoid a leak if we ever destroy an SP804, because
> s->timer[i] itself is memory allocated by arm_timer_init()
> which we don't clean up (the arm_timer_state not being
> a qdev). If we did convert arm_timer_state to qdev
> then these interrupts should turn into being sysbus irqs
> and gpio inputs on the relevant devices. (In fact if you
> were willing to take the migration-compat hit you
> could just have the sp804 create a 2 input OR gate and
> wire the arm_timer irqs up to it and use the output
> of the OR gate as the sp804 outbound interrupt line.)

Thank for the suggestion, I didn't notice the OR.