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

[Sourojeet Adhikari <s23adhik@csclub.uwaterloo.ca>]

Hello, thank you for taking the time to review the patch,
and tell us what we could improve.

> 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.
>
Hm, I'll make those changes (hopefully) by Wednesday next week.
I'm a bit busy, so it might take a bit of time to do it.

> 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.
I'll try reading through the relevant sections and send an
updated patch later next week. From what I can tell it falls
under the bcm2835_pheripherals.c file, right?


On 2025-02-24 08:22, Peter Maydell wrote:
> 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