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

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

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