[PATCH v2] gpio: pl061: use all specified IRQs for chained handler

[PATCH v2] gpio: pl061: use all specified IRQs for chained handler

[Alexander Sverdlin]

There are several implementations of PL061 which lack GPIOINTR signal in
hardware and only have individual GPIOMIS[7:0] interrupts. It's possible
to support these variants with minimal changes to the driver just
requesting all these IRQs for the same chained handler. While the solution
seems to be suboptimal, this is just a quirk for some particular IPs.

Power Management (wakeup) is not expected to work with these IPs. Only the
basic GPIO functionality.

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.

Reported-by: Sławomir Stępień <slawomir.stepien@nokia.com>
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Cc: Krzysztof Adamski <krzysztof.adamski@nokia.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: linux-gpio@vger.kernel.org
Cc: Sławomir Stępień <slawomir.stepien@nokia.com>
---
Changes since v1:
- Added AMBA_NR_IRQS loop limit

 drivers/gpio/gpio-pl061.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 0a6bfd2b06e5..0e5205a9a924 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -343,8 +343,15 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 		dev_info(&adev->dev, "could not add irqchip\n");
 		return ret;
 	}
-	gpiochip_set_chained_irqchip(&pl061->gc, &pl061_irqchip,
-				     irq, pl061_irq_handler);
+
+	/*
+	 * There are some PL061 implementations which lack GPIOINTR in hardware
+	 * and only have individual GPIOMIS[7:0] signals. The loop below will
+	 * work for both cases, depending on the device tree.
+	 */
+	for (i = 0; adev->irq[i] && (i < AMBA_NR_IRQS); i++)
+		gpiochip_set_chained_irqchip(&pl061->gc, &pl061_irqchip,
+					     adev->irq[i], pl061_irq_handler);
 
 	amba_set_drvdata(adev, pl061);
 	dev_info(&adev->dev, "PL061 GPIO chip @%pa registered\n",
-- 
2.11.0

Re: [PATCH v2] gpio: pl061: use all specified IRQs for chained handler

[Linus Walleij]

On Wed, Feb 22, 2017 at 1:30 PM, Alexander Sverdlin
<alexander.sverdlin@nokia.com> wrote:

> There are several implementations of PL061 which lack GPIOINTR signal in
> hardware and only have individual GPIOMIS[7:0] interrupts. It's possible
> to support these variants with minimal changes to the driver just
> requesting all these IRQs for the same chained handler. While the solution
> seems to be suboptimal, this is just a quirk for some particular IPs.
>
> Power Management (wakeup) is not expected to work with these IPs. Only the
> basic GPIO functionality.
>
> 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.
>
> Reported-by: Sławomir Stępień <slawomir.stepien@nokia.com>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> Cc: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: linux-gpio@vger.kernel.org
> Cc: Sławomir Stępień <slawomir.stepien@nokia.com>
> ---
> Changes since v1:
> - Added AMBA_NR_IRQS loop limit

Nice in a way, but is this really what we want to do?

This means that the platform has assigned individual IRQs to
all of the lines and no IRQ to the accumulated IRQ GPIOINTR
if I understand it correctly.

In that case these IRQs are 1-to-1 and should be modeled using
a hierarchical irqdomain, because the loop in pl061_irq_handler()
is unnecessary, we already know which IRQ line has been fired,
right?

I think we need to add a mechanism in the gpiolib core to handle
also hierarchical irqdomains and this is a good opportunity.

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

Re: [PATCH v2] gpio: pl061: use all specified IRQs for chained handler

[Alexander Sverdlin]

Hi!

On 22/02/17 13:47, Linus Walleij wrote:
>> There are several implementations of PL061 which lack GPIOINTR signal in
>> hardware and only have individual GPIOMIS[7:0] interrupts. It's possible
>> to support these variants with minimal changes to the driver just
>> requesting all these IRQs for the same chained handler. While the solution
>> seems to be suboptimal, this is just a quirk for some particular IPs.
>>
>> Power Management (wakeup) is not expected to work with these IPs. Only the
>> basic GPIO functionality.
>>
>> 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.
>>
>> Reported-by: Sławomir Stępień <slawomir.stepien@nokia.com>
>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>> Cc: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Alexandre Courbot <gnurou@gmail.com>
>> Cc: linux-gpio@vger.kernel.org
>> Cc: Sławomir Stępień <slawomir.stepien@nokia.com>
>> ---
>> Changes since v1:
>> - Added AMBA_NR_IRQS loop limit
> Nice in a way, but is this really what we want to do?
> 
> This means that the platform has assigned individual IRQs to
> all of the lines and no IRQ to the accumulated IRQ GPIOINTR
> if I understand it correctly.
> 
> In that case these IRQs are 1-to-1 and should be modeled using
> a hierarchical irqdomain, because the loop in pl061_irq_handler()
> is unnecessary, we already know which IRQ line has been fired,
> right?
> 
> I think we need to add a mechanism in the gpiolib core to handle
> also hierarchical irqdomains and this is a good opportunity.
> 
> 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.

This was an option of course, the only this is, PL061 spec says, there is
GPIOINTR and if someone has forgot it, we can support it with a quirk.
It's not specified to work this way at all, so why spend any resources
and add second implementation to the driver?

But if you are sure it's the way to go -- I can provide another patch, as
I do not have strong opinion on this story.

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

Well, I'd divide it into "1" and "!= 1" cases... Who knows, maybe
only 4 GPIOs will be implemented...

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v2] gpio: pl061: use all specified IRQs for chained handler

[Linus Walleij]

On Wed, Feb 22, 2017 at 1:57 PM, Alexander Sverdlin
<alexander.sverdlin@nokia.com> wrote:

>> - If the component has 8 IRQ lines, create a hierarchical IRQdomain
>>   and chip using a gpiolib core helper.
>
> This was an option of course, the only this is, PL061 spec says, there is
> GPIOINTR and if someone has forgot it, we can support it with a quirk.

I don't know about that. The PL061 manual says:

"The contents of this register are made available externally through the
intra-chip (or on-chip) GPIOMIS[7:0] signals."

They don't say why, but it is reasonable to assume that this is an
either-or not both-and integration point.

EITHER you route GPIOMIS[7:0] to individual IRQ lines to the CPU
OR you route GPIOINTR to the CPU.

The first approach is in a sense more efficient, because you do not
need a second level read to figure out what IRQ was fired.

> It's not specified to work this way at all, so why spend any resources
> and add second implementation to the driver?

It's not specified at all how it's supposed to work. But can you really
see any other reason to make both GPIOMIS[7:0] and GPIOINTR
available that what I describe above?

>> - If not 1 or 8 lines, bail out with an error.
>
> Well, I'd divide it into "1" and "!= 1" cases... Who knows, maybe
> only 4 GPIOs will be implemented...

If you mean that the hardware engineer only route 4 of the signals,
then the ngpio property of the device tree node must be specified.
That is exactly what that property is for, see:
Documentation/devicetree/bindings/gpio/gpio.txt

If that is not specified, it should be 1 using GPIOINTR, 8 and
using GPIOMIS[7:0] or error.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: pl061: use all specified IRQs for chained handler

[Sławomir Stępień (Nokia - PL/Wroclaw)]

On Feb 23, 2017 10:45, Linus Walleij wrote:
> On Wed, Feb 22, 2017 at 1:57 PM, Alexander Sverdlin
> <alexander.sverdlin@nokia.com> wrote:
> 
> >> - If the component has 8 IRQ lines, create a hierarchical IRQdomain
> >>   and chip using a gpiolib core helper.
> >
> > This was an option of course, the only this is, PL061 spec says, there is
> > GPIOINTR and if someone has forgot it, we can support it with a quirk.
> 
> I don't know about that. The PL061 manual says:
> 
> "The contents of this register are made available externally through the
> intra-chip (or on-chip) GPIOMIS[7:0] signals."
> 
> They don't say why, but it is reasonable to assume that this is an
> either-or not both-and integration point.
> 
> EITHER you route GPIOMIS[7:0] to individual IRQ lines to the CPU
> OR you route GPIOINTR to the CPU.

On page 2-10, 2.3.4 you can read:

"a single interrupt output GPIOINTR and/or the individual interrupts can be 
sent to the interrupt controller."

Also see figure 2-1, 4-2.

Does it not says that there are two independent options available?

-- 
Best regards
Sławomir Stępień

Re: [PATCH v2] gpio: pl061: use all specified IRQs for chained handler

[Linus Walleij]

On Thu, Feb 23, 2017 at 1:58 PM, Sławomir Stępień (Nokia - PL/Wroclaw)
<slawomir.stepien@nokia.com> wrote:
> On Feb 23, 2017 10:45, Linus Walleij wrote:
>> On Wed, Feb 22, 2017 at 1:57 PM, Alexander Sverdlin
>> <alexander.sverdlin@nokia.com> wrote:
>>
>> >> - If the component has 8 IRQ lines, create a hierarchical IRQdomain
>> >>   and chip using a gpiolib core helper.
>> >
>> > This was an option of course, the only this is, PL061 spec says, there is
>> > GPIOINTR and if someone has forgot it, we can support it with a quirk.
>>
>> I don't know about that. The PL061 manual says:
>>
>> "The contents of this register are made available externally through the
>> intra-chip (or on-chip) GPIOMIS[7:0] signals."
>>
>> They don't say why, but it is reasonable to assume that this is an
>> either-or not both-and integration point.
>>
>> EITHER you route GPIOMIS[7:0] to individual IRQ lines to the CPU
>> OR you route GPIOINTR to the CPU.
>
> On page 2-10, 2.3.4 you can read:
>
> "a single interrupt output GPIOINTR and/or the individual interrupts can be
> sent to the interrupt controller."
>
> Also see figure 2-1, 4-2.
>
> Does it not says that there are two independent options available?

Ah I didn't see that. It just underscores the point even more :)

Anyways, from a software point of view you will want to either use
the individual IRQ lines *or* the aggregated IRQ line. Not both
at the same time (no usecase I can think of atleast).

Yours,
Linus Walleij