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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Fri, Mar 19, 2021 at 3:12 PM Alexander A Sverdlin
<alexander.sverdlin@xxxxxxxxx> wrote:
>
> From: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx/
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>
> ---
> 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