On Thu, Aug 08, 2024 at 09:53:59AM +0200, Stefan Wiehler wrote:
> > You've missed a bunch of people/lists. Use get_maintainers.pl.
>
> Sorry, indeed, did not think about about PCI...
>
> > Can you provide some details on what these nodes look like. The
> > interrupt provider to an SoC device is a PCI device? That's weird...
>
> The DTO looks like this:
>
> watchdog {
> ...
> reg = <0x00002064 0x00000028>;
> ...
> interrupt-parent = <&gpio_17_0>;
> interrupts = <4 0x8>; // 8 -> IRQ_TYPE_LEVEL_LOW
> ...
> };
>
> And the base DT:
>
> ecam0: pci@878000000000 {
> ...
> #size-cells = <2>;
> #address-cells = <3>;
> ...
> gpio_17_0: gpio0@17,0 {
> ...
> reg = <0x8800 0 0 0 0>; /* DEVFN = 0x88 (17:0) */
> ...
> };
> ...
> };
>
> I completely agree it's a bit sketchy, but it's not me who came up with
> this ;-) Nevertheless I think other people might run into this issue of
> mismatching address sizes as well when no interrupt mapping was intended.
>
> > Note that of_irq_parse_raw() was refactored in 6.10, so your patch is
> > not going to apply.
>
> I'm aware of that and have adapted the patch accordingly.
>
> > That's not the right information to parse the address correctly. You
> > would need the device's #address-cells. However, in most cases we
> > don't really need to parse the address. The address is not really used
> > except in cases where interrupt routing matches the bus and so there
> > is only one size. That's effectively only PCI devices today. In that
> > case, the address size would always be equal, and the code implicitly
> > assumes that. It would be better if we could just detect when to use
> > the address or not. I think we'd have to look at 'interrupt-map-mask'
> > first to see whether or not to read the address cells. Or maybe we
> > could detect when the interrupt parent is the device's parent node.
> > Either way, this code is tricky and hard to change without breaking
> > something.
>
> Thanks for confirming that it's PCI only and no address size mismatch
> should occur. I also was thinking in the direction of checking first if
> interrupt mapping is intended and return early otherwise, but was
> worried to break something along the way...
>
> > A simpler way to fix this is just always pass in an address buffer of
> > 3 cells to of_irq_parse_raw. You would just need to copy the cells in
> > of_irq_parse_one() to a temporary buffer.
>
> That indeed sounds like the easiest solution to me; I'll send a new patch
> shortly. However I don't understand how you came up with an address
> buffer size of 3 - shouldn't it be MAX_PHANDLE_ARGS
> (addrsize = MAX_PHANDLE_ARGS and intsize = 0 in the worst case)?
There is simply no supported case of more than 3 address-cells. If
someone has 4, then their address translation is not going to work and
we won't even get to worrying about interrupts...
Rob
