Re: [PATCH] gpio: pl061: Support implementations without GPIOINTR line

[Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>]

Hello Linus,

On 22/03/2021 13:17, Linus Walleij wrote:
>> The thing is that hierarchical interrupts are supposed to
>> connect the lines by absolute offsets that are *not* coming
>> from the device tree. This is the pattern taken by other
>> in-tree hierarchical GPIO controllers. We have repeatedly
>> NACKed patches adding all the IRQs to hierarchical
>> GPIO interrupt controllers, in favor of using hardcoded
>> offsets in the driver.
>>
>> Do you have some good idea of how we can achieve that?
> One way would be to stack more compatible strings:
> 
> compatible = "lsi,axm5516-primary-gpio", "arm,pl061", "arm,primecell";
> 
> Going from more to less specific. We see that this is a
> PL061 and that it is a primecell, but we also see that
> it is a version specifically integrated into the axm5516.

The problem is, it's not the only SoC with this "issue".
AXM56xx and AXC67xx will follow, and these "hardcoded offsets"
will be different. We are not going to add a compatible for
PL061 per SoC, are we?

Well, you can always merge v1:
https://lore.kernel.org/linux-gpio/20170222123049.17588-1-alexander.sverdlin@xxxxxxxxx/

I have a ported version of it as well.

> I do see that today it looks like this
> arch/arm/boot/dts/axm55xx.dtsi:
> 
> gpio0: gpio@2010092000 {
>     #gpio-cells = <2>;
>     compatible = "arm,pl061", "arm,primecell";
>     gpio-controller;
>     reg = <0x20 0x10092000 0x00 0x1000>;
>     interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
>         <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>,
>         <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
>         <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>,
>         <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>,
>         <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>,
>         <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>,
>         <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
>     clocks = <&clks AXXIA_CLK_PER>;
>     clock-names = "apb_pclk";
>     status = "disabled";
> };
> 
> (Indeed this doesn't currently work with Linux, thus this
> patch.)
> 
> It is indeed specified in the schema right now as:
> 
>   interrupts:
>     oneOf:
>       - maxItems: 1
>       - maxItems: 8
> 
> So from a devicetree PoV all is good. But it is not the
> way hierarchical IRQs are supposed to be done IIUC.
> The preferred solution is to use a specific compatible
> string and hardcoded offsets.
> 
> It'd be nice if the interrupt or DT binding people would say
> something about how they expect these hierarchical IRQs
> to be specified from the device tree. I'm just representing
> earlier review comments here, maybe they've changed
> their mind.

-- 
Best regards,
Alexander Sverdlin.