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

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

[Linus Walleij]

On Mon, Mar 22, 2021 at 1:04 PM Linus Walleij <linus.walleij@linaro.org> 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.

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.

Yours,
Linus Walleij

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

[Alexander Sverdlin]

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@nokia.com/

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.

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

[Linus Walleij]

On Mon, Mar 22, 2021 at 1:36 PM Alexander Sverdlin
<alexander.sverdlin@nokia.com> wrote:

> > 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?

Why not? If the hardware is not 100% compatible due to
misc factors, then it needs special compatible strings.

See for example:
Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml
  compatible:
    oneOf:
      - items:
          - enum:
              - arm,arm11mp-gic
              - arm,cortex-a15-gic
              - arm,cortex-a7-gic
              - arm,cortex-a5-gic
              - arm,cortex-a9-gic
              - arm,eb11mp-gic
              - arm,gic-400
              - arm,pl390
              - arm,tc11mp-gic
              - qcom,msm-8660-qgic
              - qcom,msm-qgic2

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

The new patch (using the hierarchical IRQ chip) is much
better so no need to revert to that. The only remaining question
is really how we obtain the hardware offsets, whether they way
you do it in your patch (and which also happen to agree
with the existing bindings) or another way using a lot of
compatible strings.

Yours,
Linus Walleij