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

[PATCH v3] 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>
---
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
 	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;
-- 
2.10.2

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

[Andy Shevchenko]

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
>


-- 
With Best Regards,
Andy Shevchenko

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

[Alexander Sverdlin]

Hello Andy,

>> 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.

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/

[...]

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

-- 
Best regards,
Alexander Sverdlin.

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

[Linus Walleij]

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

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

[Alexander Sverdlin]

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.

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

[Linus Walleij]

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