Re: [PATCH] bcm2838: Add GIC-400 timer interupt connections

[Peter Maydell <peter.maydell@linaro.org>]

On Sun, 16 Feb 2025 at 03:54, Sourojeet Adhikari
<s23adhik@csclub.uwaterloo.ca> wrote:
>
> Hello everyone,
>
> This patch adds support for the system timer interrupts
> in QEMU's BCM2838 model. It defines a new constant
> called GIC400_TIMER_INT, and connects 4 timer interupts
> to the GIC-400.
> Previously timer interupts were not being routed to the
> interupt controller, causing scheduling, and some other
> stuff to have issues (me and a few friends bumped into
> this, and that's why this was written lol).
>
> Signed-off-by: Sourojeet Adhikari <s23adhik@csclub.uwaterloo.ca>

Hi; thanks for sending this patch, but I'm afraid this doesn't
look like a correct fix for the bug you've run into.

> ---
>   hw/arm/bcm2838.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/hw/arm/bcm2838.c b/hw/arm/bcm2838.c
> index ddb7c5f..0a4179f 100644
> --- a/hw/arm/bcm2838.c
> +++ b/hw/arm/bcm2838.c
> @@ -21,6 +21,8 @@
>   #define GIC400_TIMER_S_EL1_IRQ      13
>   #define GIC400_TIMER_NS_EL1_IRQ     14
>   #define GIC400_LEGACY_IRQ           15
> +#define GIC400_TIMER_INT            (96 - 32)
> +
>
>   /* Number of external interrupt lines to configure the GIC with */
>   #define GIC_NUM_IRQS                192
> @@ -176,6 +178,15 @@ static void bcm2838_realize(DeviceState *dev, Error
> **errp)
>                       qdev_get_gpio_in(gicdev, PPI(n, VIRTUAL_PMU_IRQ)));
>       }
>
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&ps_base->systmr), 0,
> +        qdev_get_gpio_in(DEVICE(&s->gic), GIC400_TIMER_INT +
> INTERRUPT_TIMER0));

The values passed to qdev_get_gpio_in(DEVICE(&s->gic), ...)
should be GIC_SPI_INTERRUPT_* values, which we define in
include/hw/arm/bcm2838_peripherals.h. You can add new ones for
the four timers.

> +    sysbus_connect_irq(SYS_BUS_DEVICE(&ps_base->systmr), 1,
> +        qdev_get_gpio_in(DEVICE(&s->gic), GIC400_TIMER_INT +
> INTERRUPT_TIMER1));
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&ps_base->systmr), 2,
> +        qdev_get_gpio_in(DEVICE(&s->gic), GIC400_TIMER_INT +
> INTERRUPT_TIMER2));
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&ps_base->systmr), 3,
> +        qdev_get_gpio_in(DEVICE(&s->gic), GIC400_TIMER_INT +
> INTERRUPT_TIMER3));

The systmr INTERRUPT_TIMER0..3 sysbus IRQ outputs are already
being wired up in the function bcm_soc_peripherals_common_realize()
in hw/arm/bcm2835_peripherals.c (to the TYPE_BCM2835_IC
interrupt controller), and it isn't valid to wire one input
directly to multiple outputs.

In fact it looks like we are currently getting this wrong for
all of the interrupts that need to be wired to both the
"legacy interrupt controller" and the GIC. I think at the moment
what happens is that the wiring to the GIC will happen last
and this overrides the earlier wiring to the legacy interrupt
controller, so code using the latter won't work correctly.

thanks
-- PMM