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