| Message ID | 20210319131205.62775-1-alexander.sverdlin@nokia.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] gpio: pl061: Support implementations without GPIOINTR line | expand |
On Fri, Mar 19, 2021 at 3:12 PM Alexander A Sverdlin <alexander.sverdlin@nokia.com> 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. an IRQ I'm wondering if the GPIO library support for IRQ hierarchy is what you are looking for. > Link: https://lore.kernel.org/linux-gpio/CACRpkdZpYzpMDWqJobSYH=JHgB74HbCQihOtexs+sVyo6SRJdA@mail.gmail.com/ > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> > --- > Changelog: > v3: pl061_populate_parent_fwspec() -> pl061_populate_parent_alloc_arg() > v2: Add pl061_populate_parent_fwspec() > > drivers/gpio/Kconfig | 1 + > drivers/gpio/gpio-pl061.c | 97 +++++++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 91 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index e3607ec..456c0a5 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -469,6 +469,7 @@ config GPIO_PL061 > depends on ARM_AMBA > select IRQ_DOMAIN > select GPIOLIB_IRQCHIP > + select IRQ_DOMAIN_HIERARCHY A nit-pick: perhaps group IRQ_ together, like select IRQ_DOMAIN select IRQ_DOMAIN_HIERARCHY select GPIOLIB_IRQCHIP ? > 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 f1b53dd..5bfb5f6 100644 > --- a/drivers/gpio/gpio-pl061.c > +++ b/drivers/gpio/gpio-pl061.c > @@ -24,6 +24,7 @@ > #include <linux/slab.h> > #include <linux/pinctrl/consumer.h> > #include <linux/pm.h> > +#include <linux/of_irq.h> A nit-pick: Perhaps before linux/p* ? > #define GPIODIR 0x400 > #define GPIOIS 0x404 > @@ -283,6 +284,69 @@ 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; > +} > + > +#ifdef CONFIG_OF > +static void *pl061_populate_parent_alloc_arg(struct gpio_chip *gc, > + unsigned int parent_hwirq, > + unsigned int parent_type) > +{ > + struct device_node *dn = to_of_node(gc->irq.fwnode); > + struct of_phandle_args pha; > + struct irq_fwspec *fwspec; > + int i; > + > + if (WARN_ON(!dn)) > + return NULL; > + > + fwspec = kmalloc(sizeof(*fwspec), GFP_KERNEL); > + if (!fwspec) > + return NULL; > + > + /* > + * This brute-force here is because of the fact PL061 is often paired > + * with GIC-v3, which has 3-cell IRQ specifier (SPI/PPI selection), and > + * unexpected range shifts in hwirq mapping (SPI IRQs are shifted by > + * 32). So this is about reversing of gic_irq_domain_translate(). > + */ > + for (i = 0; i < PL061_GPIO_NR; i++) { > + unsigned int p, pt; > + > + if (pl061_child_to_parent_hwirq(gc, i, parent_type, &p, &pt)) > + continue; > + if (p == parent_hwirq) > + break; > + } > + if (WARN_ON(i == PL061_GPIO_NR)) > + return NULL; > + > + if (WARN_ON(of_irq_parse_one(dn, i, &pha))) > + return NULL; > + > + fwspec->fwnode = gc->irq.parent_domain->fwnode; > + fwspec->param_count = pha.args_count; > + for (i = 0; i < pha.args_count; i++) > + fwspec->param[i] = pha.args[i]; > + > + return fwspec; > +} > +#endif > + > static int pl061_probe(struct amba_device *adev, const struct amba_id *id) > { > struct device *dev = &adev->dev; > @@ -330,16 +394,35 @@ 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]) { > + 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; > +#ifdef CONFIG_OF > + girq->populate_parent_alloc_arg = > + pl061_populate_parent_alloc_arg; > +#endif > + } else { > + 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; > + } > + > ret = devm_gpiochip_add_data(dev, &pl061->gc, pl061); > if (ret) > return ret; > -- > 2.10.2 >
On Fri, Mar 19, 2021 at 4:32 PM Alexander Sverdlin <alexander.sverdlin@nokia.com> wrote: > [Andy] > > I'm wondering if the GPIO library support for IRQ hierarchy is what > > you are looking for. It is indeed what should be used. > do you have a better suggestion? That's why I reference the below discussion from 2017, where > Linus Walleij suggested to use it. Well, the initial patch was just 11-liner and PL061 is > rather counterexample and doesn't fit well into the existing hierarchical infrastructure. > > >> Link: https://lore.kernel.org/linux-gpio/CACRpkdZpYzpMDWqJobSYH=JHgB74HbCQihOtexs+sVyo6SRJdA@mail.gmail.com/ Don't trust that guy. He's inexperienced with the new API and talks a lot of crap. ;) We now have a proper API for hierarchical IRQs on GPIO controllers, so we need to somehow detect that this is the case and act accordingly. 1. In Kconfig select IRQ_DOMAIN_HIERARCHY if ARCH_NOKIA_FOO 2. Look at other hierarchical GPIO IRQ drivers such as drivers/gpio/gpio-ixp4xx.c 3. Detect that we have a hierarchical situation. The hierarchical IRQ is determined by the chip integration so I would go and check the SoC compatible in the top-level DT, e.g.: if (of_machine_is_compatible("nokia,rock-n-roll-soc")) { /* Initialize the interrupt as hiearchical */ girq->fwnode =... girq->parent_domain = ... girq->child_to_parent_hwirq = pl061_child_to_parent_nokia_rock_n_roll; } else { 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; } This might need some #ifdef or IS_ENABLED() for systems without hierarchical IRQs. 4. Implement pl061_child_to_parent_nokia_rock_n_roll() Just use hardcoded hardware IRQ offsets like other drivers such as the ixp4xx does. Do not attempt to read any parent IRQs from the device tree, and assign no IRQ in the device tree. This is a side effect of the essentially per-soc pecularities around interrupts. The interrupt is not cascaded so it need special handling. I think it can be done with quite little code. Yours, Linus Walleij
Hello Linus, Andy, On 20/03/2021 12:10, Linus Walleij wrote: >>> I'm wondering if the GPIO library support for IRQ hierarchy is what >>> you are looking for. > It is indeed what should be used. and what has been used in my patch? >> do you have a better suggestion? That's why I reference the below discussion from 2017, where >> Linus Walleij suggested to use it. Well, the initial patch was just 11-liner and PL061 is >> rather counterexample and doesn't fit well into the existing hierarchical infrastructure. >> >>>> Link: https://lore.kernel.org/linux-gpio/CACRpkdZpYzpMDWqJobSYH=JHgB74HbCQihOtexs+sVyo6SRJdA@mail.gmail.com/ > Don't trust that guy. He's inexperienced with the new API and talks a lot > of crap. ;) > Yes, that's my impression now as well ;) [...] > 4. Implement pl061_child_to_parent_nokia_rock_n_roll() > Just use hardcoded hardware IRQ offsets like other drivers such as > the ixp4xx does. Do not attempt to read any parent IRQs from > the device tree, and assign no IRQ in the device tree. > > This is a side effect of the essentially per-soc pecularities around > interrupts. The interrupt is not cascaded so it need special > handling. > > I think it can be done with quite little code. Guys, have you actually looked onto my patch before these reviews? -- Best regards, Alexander Sverdlin.
On Mon, Mar 22, 2021 at 9:50 AM Alexander Sverdlin <alexander.sverdlin@nokia.com> wrote: > On 20/03/2021 12:10, Linus Walleij wrote: > >>> I'm wondering if the GPIO library support for IRQ hierarchy is what > >>> you are looking for. > > It is indeed what should be used. > > and what has been used in my patch? Yes you're right. > > I think it can be done with quite little code. > > Guys, have you actually looked onto my patch before these reviews? I don't know why I missed it, I guess my second mail is more to the point. Yours, Linus Walleij
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index e3607ec..456c0a5 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -469,6 +469,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 f1b53dd..5bfb5f6 100644 --- a/drivers/gpio/gpio-pl061.c +++ b/drivers/gpio/gpio-pl061.c @@ -24,6 +24,7 @@ #include <linux/slab.h> #include <linux/pinctrl/consumer.h> #include <linux/pm.h> +#include <linux/of_irq.h> #define GPIODIR 0x400 #define GPIOIS 0x404 @@ -283,6 +284,69 @@ 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; +} + +#ifdef CONFIG_OF +static void *pl061_populate_parent_alloc_arg(struct gpio_chip *gc, + unsigned int parent_hwirq, + unsigned int parent_type) +{ + struct device_node *dn = to_of_node(gc->irq.fwnode); + struct of_phandle_args pha; + struct irq_fwspec *fwspec; + int i; + + if (WARN_ON(!dn)) + return NULL; + + fwspec = kmalloc(sizeof(*fwspec), GFP_KERNEL); + if (!fwspec) + return NULL; + + /* + * This brute-force here is because of the fact PL061 is often paired + * with GIC-v3, which has 3-cell IRQ specifier (SPI/PPI selection), and + * unexpected range shifts in hwirq mapping (SPI IRQs are shifted by + * 32). So this is about reversing of gic_irq_domain_translate(). + */ + for (i = 0; i < PL061_GPIO_NR; i++) { + unsigned int p, pt; + + if (pl061_child_to_parent_hwirq(gc, i, parent_type, &p, &pt)) + continue; + if (p == parent_hwirq) + break; + } + if (WARN_ON(i == PL061_GPIO_NR)) + return NULL; + + if (WARN_ON(of_irq_parse_one(dn, i, &pha))) + return NULL; + + fwspec->fwnode = gc->irq.parent_domain->fwnode; + fwspec->param_count = pha.args_count; + for (i = 0; i < pha.args_count; i++) + fwspec->param[i] = pha.args[i]; + + return fwspec; +} +#endif + static int pl061_probe(struct amba_device *adev, const struct amba_id *id) { struct device *dev = &adev->dev; @@ -330,16 +394,35 @@ 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]) { + 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; +#ifdef CONFIG_OF + girq->populate_parent_alloc_arg = + pl061_populate_parent_alloc_arg; +#endif + } else { + 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; + } + ret = devm_gpiochip_add_data(dev, &pl061->gc, pl061); if (ret) return ret;