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

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

[-- 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 --]