| Message ID | 20210317155919.41450-2-alexander.sverdlin@nokia.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: pl061: Support implementations without GPIOINTR line | expand |
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.
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
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.
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
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.
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
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
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.
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
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;