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

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

[Sourojeet Adhikari]

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>
---
  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));
+    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));
+
      /* Connect UART0 to the interrupt controller */
      sysbus_connect_irq(SYS_BUS_DEVICE(&ps_base->uart0), 0,
                         qdev_get_gpio_in(gicdev, GIC_SPI_INTERRUPT_UART0));
-- 
2.48.1

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

[Peter Maydell]

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

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

[Sourojeet Adhikari]

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

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

[Peter Maydell]

On Thu, 27 Feb 2025 at 09:15, Sourojeet Adhikari
<s23adhik@csclub.uwaterloo.ca> wrote:
> > 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?

Yes. To expand a bit, QEMU's qemu_irq abstraction must
always be wired exactly 1-to-1, from a single output to
a single input. Wiring either one input to multiple outputs
or one output to multiple inputs will not behave correctly
(and unfortunately we don't have an easy way to assert()
if code in QEMU gets this wrong).

So for cases where you want the one-to-many behaviour you need
to create an object of TYPE_SPLIT_IRQ. This has one input and
multiple outputs, so you can connect your wire from device A's
output to the splitter's input, and then connect outputs
from the splitter to devices B, C, etc. (In this case A
would be the timer, and B, C the two interrupt controllers.)
Searching the source code for TYPE_SPLIT_IRQ will give some
places where it's used. (Ignore the qdev_new(TYPE_SPLIT_IRQ)
ones, those are a code pattern we use in board models, not
in SoC device models.)

In this specific bcm2838 case, it's a little more awkward,
because one of the two interrupt controllers is created inside
bcm2835_peripherals.c and one of them is created outside it.
Since bcm2838 is already reaching inside the bcm2835_peripherals
object I guess the simplest thing is:
 * create a splitter object in bcm2835_peripherals.c for
   every IRQ line that needs to be connected to both
   interrupt controllers (probably easiest to have an array
   of splitter objects, irq_splitter[])
 * in bcm2835_peripherals.c, connect the device's outbound
   IRQ to the input of the appropriate splitter, and
   connect output 0 of that splitter to the BCM2835_IC
   correct interrupt controller input
 * in bcm2838.c, connect output 0 of ps_base->irq_splitter[n]
   to the correct GIC input

(This is kind of breaking the abstraction layer that ideally
exists where the code that creates and uses a device doesn't
try to look "inside" it at any subparts it might have. We
could, for instance, instead make the bcm2835_peripherals
object expose its own qemu_irq outputs which were the second
outputs of the splitters, so that the bcm2838.c code wasn't
looking inside and finding the splitters directly. But I
think that's more awkward than it's worth. It's also possible
that we have the split between the main SoC and the
peripheral object wrong and either both interrupt controllers
or neither should be inside the peripheral object; but
reshuffling things like that would be a lot of work too.)

(PS: for the other "not 1:1" case, where you want to connect
many qemu_irqs outputs together into one input, the usual semantics
you want is to logically-OR the interrupt lines together, and
so you use TYPE_OR_IRQ for that.)

thanks
-- PMM

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

[Sourojeet Adhikari]

[-- Attachment #1: Type: text/plain, Size: 4650 bytes --]

On 2025-02-27 10:17, Peter Maydell wrote:

> On Thu, 27 Feb 2025 at 09:15, Sourojeet Adhikari
> <s23adhik@csclub.uwaterloo.ca> wrote:
>>> 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?
> Yes. To expand a bit, QEMU's qemu_irq abstraction must
> always be wired exactly 1-to-1, from a single output to
> a single input. Wiring either one input to multiple outputs
> or one output to multiple inputs will not behave correctly
> (and unfortunately we don't have an easy way to assert()
> if code in QEMU gets this wrong).
>
> So for cases where you want the one-to-many behaviour you need
> to create an object of TYPE_SPLIT_IRQ. This has one input and
> multiple outputs, so you can connect your wire from device A's
> output to the splitter's input, and then connect outputs
> from the splitter to devices B, C, etc. (In this case A
> would be the timer, and B, C the two interrupt controllers.)
> Searching the source code for TYPE_SPLIT_IRQ will give some
> places where it's used. (Ignore the qdev_new(TYPE_SPLIT_IRQ)
> ones, those are a code pattern we use in board models, not
> in SoC device models.)
>
> In this specific bcm2838 case, it's a little more awkward,
> because one of the two interrupt controllers is created inside
> bcm2835_peripherals.c and one of them is created outside it.
> Since bcm2838 is already reaching inside the bcm2835_peripherals
> object I guess the simplest thing is:
>   * create a splitter object in bcm2835_peripherals.c for
>     every IRQ line that needs to be connected to both
>     interrupt controllers (probably easiest to have an array
>     of splitter objects, irq_splitter[])
>   * in bcm2835_peripherals.c, connect the device's outbound
>     IRQ to the input of the appropriate splitter, and
>     connect output 0 of that splitter to the BCM2835_IC
>     correct interrupt controller input
>   * in bcm2838.c, connect output 0 of ps_base->irq_splitter[n]
>     to the correct GIC input
>
> (This is kind of breaking the abstraction layer that ideally
> exists where the code that creates and uses a device doesn't
> try to look "inside" it at any subparts it might have. We
> could, for instance, instead make the bcm2835_peripherals
> object expose its own qemu_irq outputs which were the second
> outputs of the splitters, so that the bcm2838.c code wasn't
> looking inside and finding the splitters directly. But I
> think that's more awkward than it's worth. It's also possible
> that we have the split between the main SoC and the
> peripheral object wrong and either both interrupt controllers
> or neither should be inside the peripheral object; but
> reshuffling things like that would be a lot of work too.)

This weekend I'll try my best to mess around, and get the solution
you proposed working. From what I can tell, I (personally) think , the not-reshuffling things approach might be a bit better here. Since otherwise it'd turn into a somewhat sizeable patch pretty quick, and is a lot of work, for something that's not *too* big of an issue. I do have access to a raspberry pi if you think I should do any kind of testing before doing the reshuffling.

On another note, do you think it's reasonable to add what you said here into the development documentation (paraphrased, and if not already documented). If I do write a patch to the documentation, can/should I cc you on it?

> (PS: for the other "not 1:1" case, where you want to connect
> many qemu_irqs outputs together into one input, the usual semantics
> you want is to logically-OR the interrupt lines together, and
> so you use TYPE_OR_IRQ for that.)

(oh oki, I'll make sure to do that on the upcoming patch then,
thank you!)

(P.S the patch probably won't be coming till next week since I have quite a bit of work outside of my programming stuff to do. Should hopfully be done by Wednesday next week though?)

[-- Attachment #2: Type: text/html, Size: 5789 bytes --]

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

[Peter Maydell]

On Sat, 1 Mar 2025 at 01:47, Sourojeet Adhikari
<s23adhik@csclub.uwaterloo.ca> wrote:
>
> On 2025-02-27 10:17, Peter Maydell wrote:
>
> On Thu, 27 Feb 2025 at 09:15, Sourojeet Adhikari
> <s23adhik@csclub.uwaterloo.ca> wrote:
>
> 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?
>
> Yes. To expand a bit, QEMU's qemu_irq abstraction must
> always be wired exactly 1-to-1, from a single output to
> a single input. Wiring either one input to multiple outputs
> or one output to multiple inputs will not behave correctly
> (and unfortunately we don't have an easy way to assert()
> if code in QEMU gets this wrong).
>
> So for cases where you want the one-to-many behaviour you need
> to create an object of TYPE_SPLIT_IRQ. This has one input and
> multiple outputs, so you can connect your wire from device A's
> output to the splitter's input, and then connect outputs
> from the splitter to devices B, C, etc. (In this case A
> would be the timer, and B, C the two interrupt controllers.)
> Searching the source code for TYPE_SPLIT_IRQ will give some
> places where it's used. (Ignore the qdev_new(TYPE_SPLIT_IRQ)
> ones, those are a code pattern we use in board models, not
> in SoC device models.)
>
> In this specific bcm2838 case, it's a little more awkward,
> because one of the two interrupt controllers is created inside
> bcm2835_peripherals.c and one of them is created outside it.
> Since bcm2838 is already reaching inside the bcm2835_peripherals
> object I guess the simplest thing is:
>  * create a splitter object in bcm2835_peripherals.c for
>    every IRQ line that needs to be connected to both
>    interrupt controllers (probably easiest to have an array
>    of splitter objects, irq_splitter[])
>  * in bcm2835_peripherals.c, connect the device's outbound
>    IRQ to the input of the appropriate splitter, and
>    connect output 0 of that splitter to the BCM2835_IC
>    correct interrupt controller input
>  * in bcm2838.c, connect output 0 of ps_base->irq_splitter[n]
>    to the correct GIC input
>
> (This is kind of breaking the abstraction layer that ideally
> exists where the code that creates and uses a device doesn't
> try to look "inside" it at any subparts it might have. We
> could, for instance, instead make the bcm2835_peripherals
> object expose its own qemu_irq outputs which were the second
> outputs of the splitters, so that the bcm2838.c code wasn't
> looking inside and finding the splitters directly. But I
> think that's more awkward than it's worth. It's also possible
> that we have the split between the main SoC and the
> peripheral object wrong and either both interrupt controllers
> or neither should be inside the peripheral object; but
> reshuffling things like that would be a lot of work too.)
>
> This weekend I'll try my best to mess around, and get the solution
> you proposed working. From what I can tell, I (personally) think , the not-reshuffling things approach might be a bit better here. Since otherwise it'd turn into a somewhat sizeable patch pretty quick, and is a lot of work, for something that's not *too* big of an issue. I do have access to a raspberry pi if you think I should do any kind of testing before doing the reshuffling.

Yeah, to be clear, what I'm suggesting is that we should
not do that reshuffling, exactly because it is a lot of
work and it's not that important. Better to just fix the bug.

> On another note, do you think it's reasonable to add what you said
> here into the development documentation (paraphrased, and if not
> already documented). If I do write a patch to the documentation,
> can/should I cc you on it?

In general, yes, we should at least document this kind of
beartrap. The difficulty is finding some good place to do it
(there are two broad locations: in a doc comment on the
function(s) for "connect a qemu_irq", and in some more
general "how to do device/board stuff" place in docs/devel/).
Feel free to cc me on any patch you send about that.

> (PS: for the other "not 1:1" case, where you want to connect
> many qemu_irqs outputs together into one input, the usual semantics
> you want is to logically-OR the interrupt lines together, and
> so you use TYPE_OR_IRQ for that.)
>
> (oh oki, I'll make sure to do that on the upcoming patch then,
> thank you!)

I think you won't need the OR gate part -- I mentioned it just
for completeness.

> (P.S the patch probably won't be coming till next week since I
> have quite a bit of work outside of my programming stuff to do.
> Should hopfully be done by Wednesday next week though?)

That's fine -- since this is a bug fix we don't have to worry
about getting it in before softfreeze.

-- PMM

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

[Sourojeet Adhikari]

Hello,

So far I've figured out that I need to add a member called SplitIRQ
in the BCMSocPeripheralBaseState struct, which is of size defined
by `BCM2835_NUM_SPLITTERS`. Then from what I can tell through
reading through the codebase, I should do something similar to
what happens in `exynos4210.c` in the `exynos4210_init_board_irqs`
function?

Sorry for taking a while to get back and still having quite a few
questions about this. Thank you for helping me out with writting
this patch.

Sourojeet Adhikari

On 2025-03-01 09:09, Peter Maydell wrote:
> On Sat, 1 Mar 2025 at 01:47, Sourojeet Adhikari
> <s23adhik@csclub.uwaterloo.ca> wrote:
>> On 2025-02-27 10:17, Peter Maydell wrote:
>>
>> On Thu, 27 Feb 2025 at 09:15, Sourojeet Adhikari
>> <s23adhik@csclub.uwaterloo.ca> wrote:
>>
>> 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?
>>
>> Yes. To expand a bit, QEMU's qemu_irq abstraction must
>> always be wired exactly 1-to-1, from a single output to
>> a single input. Wiring either one input to multiple outputs
>> or one output to multiple inputs will not behave correctly
>> (and unfortunately we don't have an easy way to assert()
>> if code in QEMU gets this wrong).
>>
>> So for cases where you want the one-to-many behaviour you need
>> to create an object of TYPE_SPLIT_IRQ. This has one input and
>> multiple outputs, so you can connect your wire from device A's
>> output to the splitter's input, and then connect outputs
>> from the splitter to devices B, C, etc. (In this case A
>> would be the timer, and B, C the two interrupt controllers.)
>> Searching the source code for TYPE_SPLIT_IRQ will give some
>> places where it's used. (Ignore the qdev_new(TYPE_SPLIT_IRQ)
>> ones, those are a code pattern we use in board models, not
>> in SoC device models.)
>>
>> In this specific bcm2838 case, it's a little more awkward,
>> because one of the two interrupt controllers is created inside
>> bcm2835_peripherals.c and one of them is created outside it.
>> Since bcm2838 is already reaching inside the bcm2835_peripherals
>> object I guess the simplest thing is:
>>   * create a splitter object in bcm2835_peripherals.c for
>>     every IRQ line that needs to be connected to both
>>     interrupt controllers (probably easiest to have an array
>>     of splitter objects, irq_splitter[])
>>   * in bcm2835_peripherals.c, connect the device's outbound
>>     IRQ to the input of the appropriate splitter, and
>>     connect output 0 of that splitter to the BCM2835_IC
>>     correct interrupt controller input
>>   * in bcm2838.c, connect output 0 of ps_base->irq_splitter[n]
>>     to the correct GIC input
>>
>> (This is kind of breaking the abstraction layer that ideally
>> exists where the code that creates and uses a device doesn't
>> try to look "inside" it at any subparts it might have. We
>> could, for instance, instead make the bcm2835_peripherals
>> object expose its own qemu_irq outputs which were the second
>> outputs of the splitters, so that the bcm2838.c code wasn't
>> looking inside and finding the splitters directly. But I
>> think that's more awkward than it's worth. It's also possible
>> that we have the split between the main SoC and the
>> peripheral object wrong and either both interrupt controllers
>> or neither should be inside the peripheral object; but
>> reshuffling things like that would be a lot of work too.)
>>
>> This weekend I'll try my best to mess around, and get the solution
>> you proposed working. From what I can tell, I (personally) think , the not-reshuffling things approach might be a bit better here. Since otherwise it'd turn into a somewhat sizeable patch pretty quick, and is a lot of work, for something that's not *too* big of an issue. I do have access to a raspberry pi if you think I should do any kind of testing before doing the reshuffling.
> Yeah, to be clear, what I'm suggesting is that we should
> not do that reshuffling, exactly because it is a lot of
> work and it's not that important. Better to just fix the bug.
>
>> On another note, do you think it's reasonable to add what you said
>> here into the development documentation (paraphrased, and if not
>> already documented). If I do write a patch to the documentation,
>> can/should I cc you on it?
> In general, yes, we should at least document this kind of
> beartrap. The difficulty is finding some good place to do it
> (there are two broad locations: in a doc comment on the
> function(s) for "connect a qemu_irq", and in some more
> general "how to do device/board stuff" place in docs/devel/).
> Feel free to cc me on any patch you send about that.
>
>> (PS: for the other "not 1:1" case, where you want to connect
>> many qemu_irqs outputs together into one input, the usual semantics
>> you want is to logically-OR the interrupt lines together, and
>> so you use TYPE_OR_IRQ for that.)
>>
>> (oh oki, I'll make sure to do that on the upcoming patch then,
>> thank you!)
> I think you won't need the OR gate part -- I mentioned it just
> for completeness.
>
>> (P.S the patch probably won't be coming till next week since I
>> have quite a bit of work outside of my programming stuff to do.
>> Should hopfully be done by Wednesday next week though?)
> That's fine -- since this is a bug fix we don't have to worry
> about getting it in before softfreeze.
>
> -- PMM

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

[Peter Maydell]

On Mon, 10 Mar 2025 at 06:29, Sourojeet Adhikari
<s23adhik@csclub.uwaterloo.ca> wrote:
>
> Hello,
>
> So far I've figured out that I need to add a member called SplitIRQ
> in the BCMSocPeripheralBaseState struct, which is of size defined
> by `BCM2835_NUM_SPLITTERS`. Then from what I can tell through
> reading through the codebase, I should do something similar to
> what happens in `exynos4210.c` in the `exynos4210_init_board_irqs`
> function?

QEMU qdev devices have a multiple-stage initialization process:
 (1) 'init', which in this case means calling object_initialize_child().
     In this case you should do this in raspi_peripherals_base_init(),
     where we do this for the other child devices of the common
     peripheral base object.
 (2) set properties (in this case the "num-lines" property, which
     needs to be set to 2). You do this in the parent object's 'realize'
     method, in this case bcm_soc_peripherals_common_realize().
     There are several different equivalent APIs to set a simple
     integer property; using qdev_prop_set_uint16() like the exynos
     code does is fine.
 (3) realize the device by calling qdev_realize(); this has to be
     done after all properties are set.
 (4) wire up the input and output GPIO lines (you want to wire up
     a GPIO only once the devices on both ends have been realized).
     For this case, in the bcm_soc_peripherals_common_realize()
     function we're going to wire up the the device's outbound
     IRQ to the input of the appropriate splitter, and connect
     output 0 of that splitter to the BCM2835_IC correct interrupt
     controller input.
 (5) The last bit of wiring, where we connect splitter output 1 to
     the correct GIC input, we do in bcm2838.c, and only after
     we've called 'realize' for both the bcm2835_peripherals object
     and the GIC object.

> Sorry for taking a while to get back and still having quite a few
> questions about this. Thank you for helping me out with writting
> this patch.

No worries; this stuff is all quite complicated and we don't
really document it very well.

thanks
-- PMM