| Message ID | 20170222123049.17588-1-alexander.sverdlin@nokia.com |
|---|---|
| State | New |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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...
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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",
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(-)