gpio: pl061: Support implementations without GPIOINTR line

Message ID 20210317155919.41450-2-alexander.sverdlin@nokia.com
State New
Headers show
Series
  • gpio: pl061: Support implementations without GPIOINTR line
Related show

Commit Message

Alexander Sverdlin March 17, 2021, 3:59 p.m.
From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

There are several implementations of PL061 which lack GPIOINTR signal in
hardware and only have individual GPIOMIS[7:0] interrupts. Use the
hierarchical interrupt support of the gpiolib in these cases (if at least 8
IRQs are configured for the PL061).

One in-tree example is arch/arm/boot/dts/axm55xx.dtsi, PL061 instances have
8 IRQs defined, but current driver supports only the first one, so only one
pin would work as IRQ trigger.

Link: https://lore.kernel.org/linux-gpio/CACRpkdZpYzpMDWqJobSYH=JHgB74HbCQihOtexs+sVyo6SRJdA@mail.gmail.com/
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 drivers/gpio/Kconfig      |  1 +
 drivers/gpio/gpio-pl061.c | 46 +++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 40 insertions(+), 7 deletions(-)

Comments

Alexander Sverdlin March 18, 2021, 8:04 a.m. | #1
Hello all!

Please ignore the below patch, the implementation is incomplete!

On 17/03/2021 16:59, Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

> 

> There are several implementations of PL061 which lack GPIOINTR signal in

> hardware and only have individual GPIOMIS[7:0] interrupts. Use the

> hierarchical interrupt support of the gpiolib in these cases (if at least 8

> IRQs are configured for the PL061).

> 

> One in-tree example is arch/arm/boot/dts/axm55xx.dtsi, PL061 instances have

> 8 IRQs defined, but current driver supports only the first one, so only one

> pin would work as IRQ trigger.

> 

> Link: https://lore.kernel.org/linux-gpio/CACRpkdZpYzpMDWqJobSYH=JHgB74HbCQihOtexs+sVyo6SRJdA@mail.gmail.com/

> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

> ---

>  drivers/gpio/Kconfig      |  1 +

>  drivers/gpio/gpio-pl061.c | 46 +++++++++++++++++++++++++++++++++++++++-------

>  2 files changed, 40 insertions(+), 7 deletions(-)

> 

> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig

> index b8013cf..6601758 100644

> --- a/drivers/gpio/Kconfig

> +++ b/drivers/gpio/Kconfig

> @@ -426,6 +426,7 @@ config GPIO_PL061

>  	depends on ARM_AMBA

>  	select IRQ_DOMAIN

>  	select GPIOLIB_IRQCHIP

> +	select IRQ_DOMAIN_HIERARCHY

>  	help

>  	  Say yes here to support the PrimeCell PL061 GPIO device

>  

> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c

> index 3439120..3c70386 100644

> --- a/drivers/gpio/gpio-pl061.c

> +++ b/drivers/gpio/gpio-pl061.c

> @@ -282,6 +282,23 @@ static int pl061_irq_set_wake(struct irq_data *d, unsigned int state)

>  	return irq_set_irq_wake(pl061->parent_irq, state);

>  }

>  

> +static int pl061_child_to_parent_hwirq(struct gpio_chip *gc, unsigned int child,

> +				       unsigned int child_type,

> +				       unsigned int *parent,

> +				       unsigned int *parent_type)

> +{

> +	struct amba_device *adev = to_amba_device(gc->parent);

> +	unsigned int irq = adev->irq[child];

> +	struct irq_data *d = irq_get_irq_data(irq);

> +

> +	if (!d)

> +		return -EINVAL;

> +

> +	*parent_type = irqd_get_trigger_type(d);

> +	*parent = irqd_to_hwirq(d);

> +	return 0;

> +}

> +

>  static int pl061_probe(struct amba_device *adev, const struct amba_id *id)

>  {

>  	struct device *dev = &adev->dev;

> @@ -332,16 +349,31 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)

>  

>  	girq = &pl061->gc.irq;

>  	girq->chip = &pl061->irq_chip;

> -	girq->parent_handler = pl061_irq_handler;

> -	girq->num_parents = 1;

> -	girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),

> -				     GFP_KERNEL);

> -	if (!girq->parents)

> -		return -ENOMEM;

> -	girq->parents[0] = irq;

>  	girq->default_type = IRQ_TYPE_NONE;

>  	girq->handler = handle_bad_irq;

>  

> +	/*

> +	 * There are some PL061 implementations which lack GPIOINTR in hardware

> +	 * and only have individual GPIOMIS[7:0] signals. We distinguish them by

> +	 * the number of IRQs assigned to the AMBA device.

> +	 */

> +	if (!adev->irq[PL061_GPIO_NR - 1]) {

> +		WARN_ON(adev->irq[1]);

> +

> +		girq->parent_handler = pl061_irq_handler;

> +		girq->num_parents = 1;

> +		girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),

> +					     GFP_KERNEL);

> +		if (!girq->parents)

> +			return -ENOMEM;

> +		girq->parents[0] = irq;

> +	} else {

> +		girq->fwnode = dev->fwnode;

> +		girq->parent_domain =

> +			irq_get_irq_data(adev->irq[PL061_GPIO_NR - 1])->domain;

> +		girq->child_to_parent_hwirq = pl061_child_to_parent_hwirq;

> +	}

> +

>  	ret = devm_gpiochip_add_data(dev, &pl061->gc, pl061);

>  	if (ret)

>  		return ret;

> 


-- 
Best regards,
Alexander Sverdlin.
Linus Walleij March 20, 2021, 11:28 a.m. | #2
Hi Alexander,

I think I answered some stuff around this patch in my previous
mail but just reiterating so it's clear:

On Wed, Mar 17, 2021 at 4:59 PM Alexander A Sverdlin
<alexander.sverdlin@nokia.com> wrote:

> @@ -426,6 +426,7 @@ config GPIO_PL061

>         depends on ARM_AMBA

>         select IRQ_DOMAIN

>         select GPIOLIB_IRQCHIP

> +       select IRQ_DOMAIN_HIERARCHY


I think this needs to be optional otherwise you activate hierarchical
IRQs on a lot of systems that don't have it.

select IRQ_DOMAIN_HIERARCHY if ARCH_OMAP_...

This leads to having to use some if IS_ENABLED and
maybe even ifdef to make it compile without hierarchies.

> +       if (!adev->irq[PL061_GPIO_NR - 1]) {

> +               WARN_ON(adev->irq[1]);

> +

> +               girq->parent_handler = pl061_irq_handler;

> +               girq->num_parents = 1;

> +               girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),

> +                                            GFP_KERNEL);

> +               if (!girq->parents)

> +                       return -ENOMEM;

> +               girq->parents[0] = irq;

> +       } else {

> +               girq->fwnode = dev->fwnode;

> +               girq->parent_domain =

> +                       irq_get_irq_data(adev->irq[PL061_GPIO_NR - 1])->domain;

> +               girq->child_to_parent_hwirq = pl061_child_to_parent_hwirq;

> +       }


This is starting to look right :)

But use the top-level board DT compatible to determine that
hiearchy is needed, and implement a per-soc child_to_parent_hwirq()
and do not attempt to get the IRQs from the device tree.

Yours,
Linus Walleij
Alexander Sverdlin March 22, 2021, 8:52 a.m. | #3
Hi!

On 20/03/2021 12:28, Linus Walleij wrote:
> This is starting to look right :)

> 

> But use the top-level board DT compatible to determine that

> hiearchy is needed, and implement a per-soc child_to_parent_hwirq()

> and do not attempt to get the IRQs from the device tree.


No! We have all 3 variants on the same board (without IRQs as well)!
Even AXXIA has 1-parent and 8-parent variant in the same upstream DT!

-- 
Best regards,
Alexander Sverdlin.
Linus Walleij March 22, 2021, 9:32 a.m. | #4
On Mon, Mar 22, 2021 at 9:52 AM Alexander Sverdlin
<alexander.sverdlin@nokia.com> wrote:
> On 20/03/2021 12:28, Linus Walleij wrote:

> > This is starting to look right :)

> >

> > But use the top-level board DT compatible to determine that

> > hiearchy is needed, and implement a per-soc child_to_parent_hwirq()

> > and do not attempt to get the IRQs from the device tree.

>

> No! We have all 3 variants on the same board (without IRQs as well)!

> Even AXXIA has 1-parent and 8-parent variant in the same upstream DT!


OK we have to discern it somehow. Since the SoC integration is
specific for these PL061 instances, we would normally add a
unique compatible string for this version of the GPIO controller.

The compatible field is intended to say how this hardware
works after all. I would even say the original PL061 has
been modified to pull out individual IRQ lines so the cell is
arguably no more compatible with "arm,pl061".
As far as I understand the original PrimeCell can't really
do that, someone has been hacking the VHDL code.

However since this is a PrimeCell, first check if the
PrimeCell ID number has been updated in the hardware.
(Just hack drivers/amba/bus.c to print what it finds in the
PID/CID registers when probing.) If LSI have been nice
enough to update this ID with something unique then
that can be used to determine the variant.

If the PrimeCell ID has not been updated (and this happens)
I'd say we need to use a unique compatible string.

You'll have to update this first:
Documentation/devicetree/bindings/gpio/pl061-gpio.yaml

I think it should be something like

compatible = "lsi,<soc-name>-pl061", "arm,primecell";

Where <soc-name> is something reasonable for this
SoC unless LSI have their own name for this modified
block on this SoC. I think it needs to be SoC-unique
since I bet it will be routing the IRQs to different HW IRQs
every time a new SoC is made.

Then augment the behaviour in the PL061 driver accordingly
when this new compatible is found, using the HW offsets
for this SoC.

Yours,
Linus Walleij
Alexander Sverdlin March 22, 2021, 9:46 a.m. | #5
Hi!

On 22/03/2021 10:32, Linus Walleij wrote:
> I think it should be something like

> 

> compatible = "lsi,<soc-name>-pl061", "arm,primecell";

> 

> Where <soc-name> is something reasonable for this

> SoC unless LSI have their own name for this modified

> block on this SoC. I think it needs to be SoC-unique

> since I bet it will be routing the IRQs to different HW IRQs

> every time a new SoC is made.

> 

> Then augment the behaviour in the PL061 driver accordingly

> when this new compatible is found, using the HW offsets

> for this SoC.


But there are standard PL061 and these without common IRQ line within one SoC.
Are you sure that's what we want, that same DTS will contain different compatible
string for this? Sounds non-obvious and error-prone to me.

And this is really something we can auto-detect. We even discussed this already:
https://lore.kernel.org/linux-gpio/CACRpkdZpYzpMDWqJobSYH=JHgB74HbCQihOtexs+sVyo6SRJdA@mail.gmail.com/

"I would make a patch that:

- If the device has 1 IRQ line, assume it is GPIOINTR and work
  as before.

- If the component has 8 IRQ lines, create a hierarchical IRQdomain
  and chip using a gpiolib core helper.

- If not 1 or 8 lines, bail out with an error.

Yours,
Linus Walleij" 

-- 
Best regards,
Alexander Sverdlin.
Linus Walleij March 22, 2021, 12:04 p.m. | #6
On Mon, Mar 22, 2021 at 10:46 AM Alexander Sverdlin
<alexander.sverdlin@nokia.com> wrote:

> But there are standard PL061 and these without common IRQ line within one SoC.

> Are you sure that's what we want, that same DTS will contain different compatible

> string for this? Sounds non-obvious and error-prone to me.


So this is indeed a standard feature of the PL061
that doesn not warrant a special compatible string.
So I was wrong about that.

I was wrong about more things:

> And this is really something we can auto-detect. We even discussed this already:

> https://lore.kernel.org/linux-gpio/CACRpkdZpYzpMDWqJobSYH=JHgB74HbCQihOtexs+sVyo6SRJdA@mail.gmail.com/

(...)
> - If the component has 8 IRQ lines, create a hierarchical IRQdomain

>   and chip using a gpiolib core helper.

>

> - If not 1 or 8 lines, bail out with an error.


Don't trust that guy, he's often confused and has no
idea what he's doing ;)

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?

Yours,
Linus Walleij
Linus Walleij March 22, 2021, 12:17 p.m. | #7
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
Alexander Sverdlin March 22, 2021, 12:36 p.m. | #8
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.
Linus Walleij March 22, 2021, 12:49 p.m. | #9
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

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b8013cf..6601758 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -426,6 +426,7 @@  config GPIO_PL061
 	depends on ARM_AMBA
 	select IRQ_DOMAIN
 	select GPIOLIB_IRQCHIP
+	select IRQ_DOMAIN_HIERARCHY
 	help
 	  Say yes here to support the PrimeCell PL061 GPIO device
 
diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 3439120..3c70386 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -282,6 +282,23 @@  static int pl061_irq_set_wake(struct irq_data *d, unsigned int state)
 	return irq_set_irq_wake(pl061->parent_irq, state);
 }
 
+static int pl061_child_to_parent_hwirq(struct gpio_chip *gc, unsigned int child,
+				       unsigned int child_type,
+				       unsigned int *parent,
+				       unsigned int *parent_type)
+{
+	struct amba_device *adev = to_amba_device(gc->parent);
+	unsigned int irq = adev->irq[child];
+	struct irq_data *d = irq_get_irq_data(irq);
+
+	if (!d)
+		return -EINVAL;
+
+	*parent_type = irqd_get_trigger_type(d);
+	*parent = irqd_to_hwirq(d);
+	return 0;
+}
+
 static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	struct device *dev = &adev->dev;
@@ -332,16 +349,31 @@  static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 
 	girq = &pl061->gc.irq;
 	girq->chip = &pl061->irq_chip;
-	girq->parent_handler = pl061_irq_handler;
-	girq->num_parents = 1;
-	girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
-				     GFP_KERNEL);
-	if (!girq->parents)
-		return -ENOMEM;
-	girq->parents[0] = irq;
 	girq->default_type = IRQ_TYPE_NONE;
 	girq->handler = handle_bad_irq;
 
+	/*
+	 * There are some PL061 implementations which lack GPIOINTR in hardware
+	 * and only have individual GPIOMIS[7:0] signals. We distinguish them by
+	 * the number of IRQs assigned to the AMBA device.
+	 */
+	if (!adev->irq[PL061_GPIO_NR - 1]) {
+		WARN_ON(adev->irq[1]);
+
+		girq->parent_handler = pl061_irq_handler;
+		girq->num_parents = 1;
+		girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
+					     GFP_KERNEL);
+		if (!girq->parents)
+			return -ENOMEM;
+		girq->parents[0] = irq;
+	} else {
+		girq->fwnode = dev->fwnode;
+		girq->parent_domain =
+			irq_get_irq_data(adev->irq[PL061_GPIO_NR - 1])->domain;
+		girq->child_to_parent_hwirq = pl061_child_to_parent_hwirq;
+	}
+
 	ret = devm_gpiochip_add_data(dev, &pl061->gc, pl061);
 	if (ret)
 		return ret;