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

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

[Alexander A Sverdlin]

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;
-- 
2.4.6

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

[Alexander Sverdlin]

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.

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

[Linus Walleij]

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

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

[Alexander Sverdlin]

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.

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

[Linus Walleij]

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

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

[Alexander Sverdlin]

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.

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

[Linus Walleij]

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

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

[PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Alexander A Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

Existing (irq < 0) condition is always false because adev->irq has unsigned
type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
the mapping errors were silently ignored.

Seems that repairing this check would be backwards-incompatible and might
break the probe() for the implementations without IRQ support. Therefore
warn the user instead.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 drivers/gpio/gpio-pl061.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 5df7782..3439120 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -326,10 +326,8 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 
 	writeb(0, pl061->base + GPIOIE); /* disable irqs */
 	irq = adev->irq[0];
-	if (irq < 0) {
-		dev_err(&adev->dev, "invalid IRQ\n");
-		return -ENODEV;
-	}
+	if (!irq)
+		dev_warn(&adev->dev, "IRQ support disabled\n");
 	pl061->parent_irq = irq;
 
 	girq = &pl061->gc.irq;
-- 
2.4.6

Re: [PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Alexander Sverdlin]

Hello Andy,

On 18/03/2021 11:51, Andy Shevchenko wrote:
> 
> 
> On Wednesday, March 17, 2021, Alexander A Sverdlin <alexander.sverdlin@nokia.com <mailto:alexander.sverdlin@nokia.com>> wrote:
> 
>     From: Alexander Sverdlin <alexander.sverdlin@nokia.com <mailto:alexander.sverdlin@nokia.com>>
> 
>     Existing (irq < 0) condition is always false because adev->irq has unsigned
>     type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
>     the mapping errors were silently ignored.
> 
>     Seems that repairing this check would be backwards-incompatible and might
>     break the probe() for the implementations without IRQ support. Therefore
>     warn the user instead.
> 
>     Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com <mailto:alexander.sverdlin@nokia.com>>
>     ---
>      drivers/gpio/gpio-pl061.c | 6 ++----
>      1 file changed, 2 insertions(+), 4 deletions(-)
> 
>     diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
>     index 5df7782..3439120 100644
>     --- a/drivers/gpio/gpio-pl061.c
>     +++ b/drivers/gpio/gpio-pl061.c
>     @@ -326,10 +326,8 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
> 
>             writeb(0, pl061->base + GPIOIE); /* disable irqs */
>             irq = adev->irq[0];
>     -       if (irq < 0) {
>     -               dev_err(&adev->dev, "invalid IRQ\n");
>     -               return -ENODEV;
>     -       }
>     +       if (!irq)
>     +               dev_warn(&adev->dev, "IRQ support disabled\n");
> 
> 
> 
> I guess you need to preserve bailing out. Seems nobody hit this error path.

Do you mean preserve "return -ENODEV;"?
This never ever happened, because the "if" is "always false", irqs coming from irq[] cannot be
negative.
And there is another use-case actually: there are legal PL061 configurations without IRQs at all,
which simply work even trying to instantiate irq chip, but as devm_gpiochip_add_data() doesn't
fail with irq==0, this goes completely unnoticed and such a gpio bank works fine.

The proper way would be not even try to instantiate any irq chip in such case.
Let me know if I shall rework the patch this way.

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Andy Shevchenko]

On Thu, Mar 18, 2021 at 1:12 PM Alexander Sverdlin
<alexander.sverdlin@nokia.com> wrote:
> On 18/03/2021 11:51, Andy Shevchenko wrote:
> > On Wednesday, March 17, 2021, Alexander A Sverdlin <alexander.sverdlin@nokia.com <mailto:alexander.sverdlin@nokia.com>> wrote:

...

> >     Existing (irq < 0) condition is always false because adev->irq has unsigned
> >     type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
> >     the mapping errors were silently ignored.
> >
> >     Seems that repairing this check would be backwards-incompatible and might
> >     break the probe() for the implementations without IRQ support. Therefore
> >     warn the user instead.

...

> >     -       if (irq < 0) {
> >     -               dev_err(&adev->dev, "invalid IRQ\n");
> >     -               return -ENODEV;
> >     -       }
> >     +       if (!irq)
> >     +               dev_warn(&adev->dev, "IRQ support disabled\n");
> >
> >
> >
> > I guess you need to preserve bailing out. Seems nobody hit this error path.
>
> Do you mean preserve "return -ENODEV;"?
> This never ever happened, because the "if" is "always false", irqs coming from irq[] cannot be
> negative.
> And there is another use-case actually: there are legal PL061 configurations without IRQs at all,
> which simply work even trying to instantiate irq chip, but as devm_gpiochip_add_data() doesn't
> fail with irq==0, this goes completely unnoticed and such a gpio bank works fine.
>
> The proper way would be not even try to instantiate any irq chip in such case.
> Let me know if I shall rework the patch this way.

Yes, please, rewrite it like
if (irq > 0) {
 ... instantiate an IRQ chip ...
} else {
 // nothing. No warning is needed (as it wasn't ever before), perhaps
just a debug message
}
--
With Best Regards,
Andy Shevchenko

[PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Alexander A Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

Existing (irq < 0) condition is always false because adev->irq has unsigned
type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
the mapping errors were silently ignored.

Seems that repairing this check would be backwards-incompatible and might
break the probe() for the implementations without IRQ support. Therefore
warn the user instead.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 drivers/gpio/gpio-pl061.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 5df7782..3439120 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -326,10 +326,8 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 
 	writeb(0, pl061->base + GPIOIE); /* disable irqs */
 	irq = adev->irq[0];
-	if (irq < 0) {
-		dev_err(&adev->dev, "invalid IRQ\n");
-		return -ENODEV;
-	}
+	if (!irq)
+		dev_warn(&adev->dev, "IRQ support disabled\n");
 	pl061->parent_irq = irq;
 
 	girq = &pl061->gc.irq;
-- 
2.4.6

Re: [PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Bartosz Golaszewski]

wt., 3 mar 2020 o 10:29 Alexander A Sverdlin
<alexander.sverdlin@nokia.com> napisał(a):
>
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>
> Existing (irq < 0) condition is always false because adev->irq has unsigned
> type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
> the mapping errors were silently ignored.
>
> Seems that repairing this check would be backwards-incompatible and might
> break the probe() for the implementations without IRQ support. Therefore
> warn the user instead.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> ---
>  drivers/gpio/gpio-pl061.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 5df7782..3439120 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
> @@ -326,10 +326,8 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
>
>         writeb(0, pl061->base + GPIOIE); /* disable irqs */
>         irq = adev->irq[0];
> -       if (irq < 0) {
> -               dev_err(&adev->dev, "invalid IRQ\n");
> -               return -ENODEV;
> -       }
> +       if (!irq)
> +               dev_warn(&adev->dev, "IRQ support disabled\n");
>         pl061->parent_irq = irq;
>
>         girq = &pl061->gc.irq;
> --
> 2.4.6
>

What happens later on if irq == 0? Does irq_set_irq_wake() fail?

Bart

Re: [PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Alexander Sverdlin]

Hi!

On 04/03/2020 15:21, Bartosz Golaszewski wrote:
> wt., 3 mar 2020 o 10:29 Alexander A Sverdlin
> <alexander.sverdlin@nokia.com> napisał(a):
>> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>>
>> Existing (irq < 0) condition is always false because adev->irq has unsigned
>> type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
>> the mapping errors were silently ignored.
>>
>> Seems that repairing this check would be backwards-incompatible and might
>> break the probe() for the implementations without IRQ support. Therefore
>> warn the user instead.
>>
>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>> ---
>>  drivers/gpio/gpio-pl061.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
>> index 5df7782..3439120 100644
>> --- a/drivers/gpio/gpio-pl061.c
>> +++ b/drivers/gpio/gpio-pl061.c
>> @@ -326,10 +326,8 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
>>
>>         writeb(0, pl061->base + GPIOIE); /* disable irqs */
>>         irq = adev->irq[0];
>> -       if (irq < 0) {
>> -               dev_err(&adev->dev, "invalid IRQ\n");
>> -               return -ENODEV;
>> -       }
>> +       if (!irq)
>> +               dev_warn(&adev->dev, "IRQ support disabled\n");
>>         pl061->parent_irq = irq;
>>
>>         girq = &pl061->gc.irq;
>> --
>> 2.4.6
>>
> What happens later on if irq == 0? Does irq_set_irq_wake() fail?

Yes, would fail if IRQs would be requested from PL061:

int irq_set_irq_wake(unsigned int irq, unsigned int on)                                                                                                                                                            
{                                                                                                                                                                                                                  
        unsigned long flags;                                                                                                                                                                                       
        struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);                                                                                                                      
        int ret = 0;                                                                                                                                                                                               
                                                                                                                                                                                                                   
        if (!desc)                                                                                                                                                                                                 
                return -EINVAL;                                                                                                                                                                                    


-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Bartosz Golaszewski]

śr., 4 mar 2020 o 15:58 Alexander Sverdlin
<alexander.sverdlin@nokia.com> napisał(a):
>
> Hi!
>
> On 04/03/2020 15:21, Bartosz Golaszewski wrote:
> > wt., 3 mar 2020 o 10:29 Alexander A Sverdlin
> > <alexander.sverdlin@nokia.com> napisał(a):
> >> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> >>
> >> Existing (irq < 0) condition is always false because adev->irq has unsigned
> >> type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
> >> the mapping errors were silently ignored.
> >>
> >> Seems that repairing this check would be backwards-incompatible and might
> >> break the probe() for the implementations without IRQ support. Therefore
> >> warn the user instead.
> >>
> >> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> >> ---
> >>  drivers/gpio/gpio-pl061.c | 6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> >> index 5df7782..3439120 100644
> >> --- a/drivers/gpio/gpio-pl061.c
> >> +++ b/drivers/gpio/gpio-pl061.c
> >> @@ -326,10 +326,8 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
> >>
> >>         writeb(0, pl061->base + GPIOIE); /* disable irqs */
> >>         irq = adev->irq[0];
> >> -       if (irq < 0) {
> >> -               dev_err(&adev->dev, "invalid IRQ\n");
> >> -               return -ENODEV;
> >> -       }
> >> +       if (!irq)
> >> +               dev_warn(&adev->dev, "IRQ support disabled\n");
> >>         pl061->parent_irq = irq;
> >>
> >>         girq = &pl061->gc.irq;
> >> --
> >> 2.4.6
> >>
> > What happens later on if irq == 0? Does irq_set_irq_wake() fail?
>
> Yes, would fail if IRQs would be requested from PL061:
>
> int irq_set_irq_wake(unsigned int irq, unsigned int on)
> {
>         unsigned long flags;
>         struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
>         int ret = 0;
>
>         if (!desc)
>                 return -EINVAL;
>
>
> --
> Best regards,
> Alexander Sverdlin.

Ok, I'll go ahead and queue this then.

Thanks!
Bartosz