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

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

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