LinuxLists.cc - [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Biju,

Thank you for the review.

On Thu, May 12, 2022 at 6:35 AM Biju Das <[email protected]> wrote:
>
> Hi Prabhakar,
>
> Thanks for the patch.
>
> > Prabhakar Mahadev Lad <[email protected]>
> > Subject: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to
> > handle GPIO interrupt
> >
> > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
> >
> > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be used as
> > IRQ lines at given time. Selection of pins as IRQ lines is handled by IA55
> > (which is the IRQC block) which sits in between the GPIO and GIC.
>
> Do we need to update bindings with interrupt-cells on [1] like [2] as it act as parent for GPIO interrupts?
>
Yes interrupt-controller and interrupt-parent needs to be added. I'm
wondering if "interrupt-cells" is not required. If the pin is an
interrupt it will be passed as an GPIO.

@Geert - your thoughts ?

Cheers,
Prabhakar

2022-05-13 08:23:31

by Marc Zyngier

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

On Wed, 11 May 2022 19:32:10 +0100,
Lad Prabhakar <[email protected]> wrote:
>
> Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
>
> GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> used as IRQ lines at given time. Selection of pins as IRQ lines
> is handled by IA55 (which is the IRQC block) which sits in between the
> GPIO and GIC.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> drivers/pinctrl/renesas/pinctrl-rzg2l.c | 202 ++++++++++++++++++++++++
> 1 file changed, 202 insertions(+)
>
> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index a48cac55152c..af2c739cdbaa 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -9,8 +9,10 @@
> #include <linux/clk.h>
> #include <linux/gpio/driver.h>
> #include <linux/io.h>
> +#include <linux/interrupt.h>
> #include <linux/module.h>
> #include <linux/of_device.h>
> +#include <linux/of_irq.h>
> #include <linux/pinctrl/pinconf-generic.h>
> #include <linux/pinctrl/pinconf.h>
> #include <linux/pinctrl/pinctrl.h>
> @@ -89,6 +91,7 @@
> #define PIN(n) (0x0800 + 0x10 + (n))
> #define IOLH(n) (0x1000 + (n) * 8)
> #define IEN(n) (0x1800 + (n) * 8)
> +#define ISEL(n) (0x2c80 + (n) * 8)
> #define PWPR (0x3014)
> #define SD_CH(n) (0x3000 + (n) * 4)
> #define QSPI (0x3008)
> @@ -112,6 +115,10 @@
> #define RZG2L_PIN_ID_TO_PORT_OFFSET(id) (RZG2L_PIN_ID_TO_PORT(id) + 0x10)
> #define RZG2L_PIN_ID_TO_PIN(id) ((id) % RZG2L_PINS_PER_PORT)
>
> +#define RZG2L_TINT_MAX_INTERRUPT 32
> +#define RZG2L_TINT_IRQ_START_INDEX 9
> +#define RZG2L_PACK_HWIRQ(t, i) (((t) << 16) | (i))
> +
> struct rzg2l_dedicated_configs {
> const char *name;
> u32 config;
> @@ -137,6 +144,9 @@ struct rzg2l_pinctrl {
>
> struct gpio_chip gpio_chip;
> struct pinctrl_gpio_range gpio_range;
> + DECLARE_BITMAP(tint_slot, RZG2L_TINT_MAX_INTERRUPT);
> + spinlock_t bitmap_lock;
> + unsigned int hwirq[RZG2L_TINT_MAX_INTERRUPT];
>
> spinlock_t lock;
> };
> @@ -883,6 +893,8 @@ static int rzg2l_gpio_get(struct gpio_chip *chip, unsigned int offset)
>
> static void rzg2l_gpio_free(struct gpio_chip *chip, unsigned int offset)
> {
> + unsigned int virq;
> +
> pinctrl_gpio_free(chip->base + offset);
>
> /*
> @@ -890,6 +902,10 @@ static void rzg2l_gpio_free(struct gpio_chip *chip, unsigned int offset)
> * drive the GPIO pin as an output.
> */
> rzg2l_gpio_direction_input(chip, offset);
> +
> + virq = irq_find_mapping(chip->irq.domain, offset);
> + if (virq)
> + irq_dispose_mapping(virq);
> }
>
> static const char * const rzg2l_gpio_names[] = {
> @@ -1104,14 +1120,190 @@ static struct {
> }
> };
>
> +static int rzg2l_gpio_get_gpioint(unsigned int virq)
> +{
> + unsigned int gpioint;
> + unsigned int i;
> + u32 port, bit;
> +
> + port = virq / 8;
> + bit = virq % 8;
> +
> + if (port >= ARRAY_SIZE(rzg2l_gpio_configs) ||
> + bit >= RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[port]))
> + return -EINVAL;
> +
> + gpioint = bit;
> + for (i = 0; i < port; i++)
> + gpioint += RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[i]);
> +
> + return gpioint;
> +}
> +
> +static void rzg2l_gpio_irq_domain_free(struct irq_domain *domain, unsigned int virq,
> + unsigned int nr_irqs)
> +{
> + struct irq_data *d;
> +
> + d = irq_domain_get_irq_data(domain, virq);
> + if (d) {
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
> + irq_hw_number_t hwirq = irqd_to_hwirq(d);
> + unsigned long flags;
> + unsigned int i;
> +
> + for (i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) {
> + if (pctrl->hwirq[i] == hwirq) {
> + spin_lock_irqsave(&pctrl->bitmap_lock, flags);
> + bitmap_release_region(pctrl->tint_slot, i, get_order(1));
> + spin_unlock_irqrestore(&pctrl->bitmap_lock, flags);
> + pctrl->hwirq[i] = 0;
> + break;
> + }
> + }
> + }
> + irq_domain_free_irqs_common(domain, virq, nr_irqs);
> +}
> +
> +static void rzg2l_gpio_irq_disable(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
> + unsigned int hwirq = irqd_to_hwirq(d);
> + unsigned long flags;
> + void __iomem *addr;
> + u32 port;
> + u8 bit;
> +
> + port = RZG2L_PIN_ID_TO_PORT(hwirq);
> + bit = RZG2L_PIN_ID_TO_PIN(hwirq);
> +
> + addr = pctrl->base + ISEL(port);
> + if (bit >= 4) {
> + bit -= 4;
> + addr += 4;
> + }
> +
> + spin_lock_irqsave(&pctrl->lock, flags);
> + writel(readl(addr) & ~BIT(bit * 8), addr);
> + spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> + irq_chip_disable_parent(d);
> +}
> +
> +static void rzg2l_gpio_irq_enable(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
> + unsigned int hwirq = irqd_to_hwirq(d);
> + unsigned long flags;
> + void __iomem *addr;
> + u32 port;
> + u8 bit;
> +
> + port = RZG2L_PIN_ID_TO_PORT(hwirq);
> + bit = RZG2L_PIN_ID_TO_PIN(hwirq);
> +
> + addr = pctrl->base + ISEL(port);
> + if (bit >= 4) {
> + bit -= 4;
> + addr += 4;
> + }
> +
> + spin_lock_irqsave(&pctrl->lock, flags);
> + writel(readl(addr) | BIT(bit * 8), addr);
> + spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> + irq_chip_enable_parent(d);
> +}
> +
> +static int rzg2l_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> + return irq_chip_set_type_parent(d, type);
> +}
> +
> +static void rzg2l_gpio_irqc_eoi(struct irq_data *d)
> +{
> + irq_chip_eoi_parent(d);
> +}
> +
> +static struct irq_chip rzg2l_gpio_irqchip = {
> + .name = "rzg2l-gpio",
> + .irq_disable = rzg2l_gpio_irq_disable,
> + .irq_enable = rzg2l_gpio_irq_enable,
> + .irq_mask = irq_chip_mask_parent,
> + .irq_unmask = irq_chip_unmask_parent,
> + .irq_set_type = rzg2l_gpio_irq_set_type,
> + .irq_eoi = rzg2l_gpio_irqc_eoi,

Please see the changes[1] that are queued in -next around immutable
GPIO irqchips. This needs to be made const, the enable/disable methods
have the right callbacks added, the resource management methods
plumbed, and the correct flag exposed.

> +};
> +
> +static int rzg2l_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
> + unsigned int child,
> + unsigned int child_type,
> + unsigned int *parent,
> + unsigned int *parent_type)
> +{
> + struct rzg2l_pinctrl *pctrl = gpiochip_get_data(gc);
> + unsigned long flags;
> + int gpioint, irq;
> +
> + gpioint = rzg2l_gpio_get_gpioint(child);
> + if (gpioint < 0)
> + return gpioint;
> +
> + spin_lock_irqsave(&pctrl->bitmap_lock, flags);
> + irq = bitmap_find_free_region(pctrl->tint_slot, RZG2L_TINT_MAX_INTERRUPT, get_order(1));
> + spin_unlock_irqrestore(&pctrl->bitmap_lock, flags);
> + if (irq < 0)
> + return -ENOSPC;
> + pctrl->hwirq[irq] = child;
> + irq += RZG2L_TINT_IRQ_START_INDEX;
> +
> + /* All these interrupts are level high in the CPU */
> + *parent_type = IRQ_TYPE_LEVEL_HIGH;
> + *parent = RZG2L_PACK_HWIRQ(gpioint, irq);
> + return 0;
> +}
> +
> +static void *rzg2l_gpio_populate_parent_fwspec(struct gpio_chip *chip,
> + unsigned int parent_hwirq,
> + unsigned int parent_type)
> +{
> + struct irq_fwspec *fwspec;
> +
> + fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
> + if (!fwspec)
> + return NULL;
> +
> + fwspec->fwnode = chip->irq.parent_domain->fwnode;
> + fwspec->param_count = 2;
> + fwspec->param[0] = parent_hwirq;
> + fwspec->param[1] = parent_type;
> +
> + return fwspec;
> +}
> +
> static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> {
> struct device_node *np = pctrl->dev->of_node;
> struct gpio_chip *chip = &pctrl->gpio_chip;
> const char *name = dev_name(pctrl->dev);
> + struct irq_domain *parent_domain;
> struct of_phandle_args of_args;
> + struct device_node *parent_np;
> + struct gpio_irq_chip *girq;
> int ret;
>
> + parent_np = of_irq_find_parent(np);
> + if (!parent_np)
> + return -ENXIO;
> +
> + parent_domain = irq_find_host(parent_np);
> + of_node_put(parent_np);
> + if (!parent_domain)
> + return -EPROBE_DEFER;
> +
> ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args);
> if (ret) {
> dev_err(pctrl->dev, "Unable to parse gpio-ranges\n");
> @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> chip->base = -1;
> chip->ngpio = of_args.args[2];
>
> + girq = &chip->irq;

Same thing, this needs to use the appropriate setter.

> + girq->chip = &rzg2l_gpio_irqchip;
> + girq->fwnode = of_node_to_fwnode(np);
> + girq->parent_domain = parent_domain;
> + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq;
> + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec;
> + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free;
> + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT;
> +
> pctrl->gpio_range.id = 0;
> pctrl->gpio_range.pin_base = 0;
> pctrl->gpio_range.base = 0;
> @@ -1253,6 +1454,7 @@ static int rzg2l_pinctrl_probe(struct platform_device *pdev)
> }
>
> spin_lock_init(&pctrl->lock);
> + spin_lock_init(&pctrl->bitmap_lock);
>
> platform_set_drvdata(pdev, pctrl);
>
> --
> 2.25.1
>
>

Thanks,

M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gpio-immutable

--
Without deviation from the norm, progress is not possible.

2022-05-13 09:53:00

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Geert,

Thank you for the review.

On Thu, May 12, 2022 at 8:39 AM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar
> <[email protected]> wrote:
> > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
>
> domain
>
ouch.

> > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> > used as IRQ lines at given time. Selection of pins as IRQ lines
>
> at a given time
>
will fix that.

> > is handled by IA55 (which is the IRQC block) which sits in between the
> > GPIO and GIC.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
>
> Thanks for your patch!
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>
> > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > {
> > struct device_node *np = pctrl->dev->of_node;
> > struct gpio_chip *chip = &pctrl->gpio_chip;
> > const char *name = dev_name(pctrl->dev);
> > + struct irq_domain *parent_domain;
> > struct of_phandle_args of_args;
> > + struct device_node *parent_np;
> > + struct gpio_irq_chip *girq;
> > int ret;
> >
> > + parent_np = of_irq_find_parent(np);
> > + if (!parent_np)
> > + return -ENXIO;
> > +
> > + parent_domain = irq_find_host(parent_np);
> > + of_node_put(parent_np);
> > + if (!parent_domain)
> > + return -EPROBE_DEFER;
> > +
> > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args);
> > if (ret) {
> > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n");
> > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > chip->base = -1;
> > chip->ngpio = of_args.args[2];
> >
> > + girq = &chip->irq;
> > + girq->chip = &rzg2l_gpio_irqchip;
> > + girq->fwnode = of_node_to_fwnode(np);
> > + girq->parent_domain = parent_domain;
> > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq;
> > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec;
> > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free;
> > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT;
> > +
>
> I think you need to provide a .init_valid_mask() callback, as
> gpiochip_irqchip_remove() relies on that for destroying interrupts.
Are you suggesting the callback to avoid looping through all the GPIO pins?

> However, the mask will need to be dynamic, as GPIO interrupts can be
> mapped and unmapped to one of the 32 available interrupts dynamically,
> right?
Yep that's correct.

> I'm not sure if that can be done easily: if gpiochip_irqchip_irq_valid()
> is ever called too early, before the mapping is done, it would fail.
>
The mask initialization is a one time process and that is during
adding the GPIO chip. At this stage we won't be knowing what will be
the valid GPIO pins used as interrupts. Maybe the core needs to
implement a callback which lands in the GPIO controller driver to tell
if the gpio irq line is valid. This way we can handle dynamic
interrupts.

Cheers,
Prabhakar

> > pctrl->gpio_range.id = 0;
> > pctrl->gpio_range.pin_base = 0;
> > pctrl->gpio_range.base = 0;
>
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds

2022-05-14 00:02:15

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Prabhakar,

On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar
<[email protected]> wrote:
> Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.

domain

> GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> used as IRQ lines at given time. Selection of pins as IRQ lines

at a given time

> is handled by IA55 (which is the IRQC block) which sits in between the
> GPIO and GIC.
>
> Signed-off-by: Lad Prabhakar <[email protected]>

Thanks for your patch!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c

> static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> {
> struct device_node *np = pctrl->dev->of_node;
> struct gpio_chip *chip = &pctrl->gpio_chip;
> const char *name = dev_name(pctrl->dev);
> + struct irq_domain *parent_domain;
> struct of_phandle_args of_args;
> + struct device_node *parent_np;
> + struct gpio_irq_chip *girq;
> int ret;
>
> + parent_np = of_irq_find_parent(np);
> + if (!parent_np)
> + return -ENXIO;
> +
> + parent_domain = irq_find_host(parent_np);
> + of_node_put(parent_np);
> + if (!parent_domain)
> + return -EPROBE_DEFER;
> +
> ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args);
> if (ret) {
> dev_err(pctrl->dev, "Unable to parse gpio-ranges\n");
> @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> chip->base = -1;
> chip->ngpio = of_args.args[2];
>
> + girq = &chip->irq;
> + girq->chip = &rzg2l_gpio_irqchip;
> + girq->fwnode = of_node_to_fwnode(np);
> + girq->parent_domain = parent_domain;
> + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq;
> + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec;
> + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free;
> + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT;
> +

I think you need to provide a .init_valid_mask() callback, as
gpiochip_irqchip_remove() relies on that for destroying interrupts.
However, the mask will need to be dynamic, as GPIO interrupts can be
mapped and unmapped to one of the 32 available interrupts dynamically,
right?
I'm not sure if that can be done easily: if gpiochip_irqchip_irq_valid()
is ever called too early, before the mapping is done, it would fail.

> pctrl->gpio_range.id = 0;
> pctrl->gpio_range.pin_base = 0;
> pctrl->gpio_range.base = 0;

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2022-05-14 00:09:00

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Prabhakar,

On Fri, May 13, 2022 at 3:56 PM Lad, Prabhakar
<[email protected]> wrote:
> On Fri, May 13, 2022 at 7:53 AM Geert Uytterhoeven <[email protected]> wrote:
> > On Thu, May 12, 2022 at 7:36 PM Lad, Prabhakar
> > <[email protected]> wrote:
> > > On Thu, May 12, 2022 at 8:39 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar
> > > > <[email protected]> wrote:
> > > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
> > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> > > > > used as IRQ lines at given time. Selection of pins as IRQ lines
> > > > > is handled by IA55 (which is the IRQC block) which sits in between the
> > > > > GPIO and GIC.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <[email protected]>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > >
> > > > > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > > > > {
> > > > > struct device_node *np = pctrl->dev->of_node;
> > > > > struct gpio_chip *chip = &pctrl->gpio_chip;
> > > > > const char *name = dev_name(pctrl->dev);
> > > > > + struct irq_domain *parent_domain;
> > > > > struct of_phandle_args of_args;
> > > > > + struct device_node *parent_np;
> > > > > + struct gpio_irq_chip *girq;
> > > > > int ret;
> > > > >
> > > > > + parent_np = of_irq_find_parent(np);
> > > > > + if (!parent_np)
> > > > > + return -ENXIO;
> > > > > +
> > > > > + parent_domain = irq_find_host(parent_np);
> > > > > + of_node_put(parent_np);
> > > > > + if (!parent_domain)
> > > > > + return -EPROBE_DEFER;
> > > > > +
> > > > > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args);
> > > > > if (ret) {
> > > > > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n");
> > > > > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > > > > chip->base = -1;
> > > > > chip->ngpio = of_args.args[2];
> > > > >
> > > > > + girq = &chip->irq;
> > > > > + girq->chip = &rzg2l_gpio_irqchip;
> > > > > + girq->fwnode = of_node_to_fwnode(np);
> > > > > + girq->parent_domain = parent_domain;
> > > > > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq;
> > > > > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec;
> > > > > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free;
> > > > > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT;
> > > > > +
> > > >
> > > > I think you need to provide a .init_valid_mask() callback, as
> > > > gpiochip_irqchip_remove() relies on that for destroying interrupts.
> > > Are you suggesting the callback to avoid looping through all the GPIO pins?
> >
> > gpiochip_irqchip_remove() does:
> >
> > /* Remove all IRQ mappings and delete the domain */
> > if (gc->irq.domain) {
> > unsigned int irq;
> >
> > for (offset = 0; offset < gc->ngpio; offset++) {
> > if (!gpiochip_irqchip_irq_valid(gc, offset))
> > continue;
> >
> > irq = irq_find_mapping(gc->irq.domain, offset);
> > irq_dispose_mapping(irq);
> > }
> >
> > irq_domain_remove(gc->irq.domain);
> >
> > }
> >
> > The main thing is not about avoiding to loop through all GPIO pins,
> > but to avoid irq_{find,dispose}_mapping() doing the wrong thing.
> So in our case if we don't implement valid masks, that would mean all
> the pins are valid. irq_find_mapping() would return 0 if no mapping is
> found to the corresponding offset and irq_dispose_mapping() would
> simply return back without doing anything if virq == 0.(In this patch
> rzg2l_gpio_free() does call irq_{find,dispose}_mapping())

But "offset" is a number from the GPIO offset space (0-122), while
irq_find_mapping() expects a number from the domain's IRQ space,
which is only 0-31?

> > The loop is over all GPIO offsets, while not all of them are mapped
> > to valid interrupts. Does the above work correctly?
> >
> I haven't tested unloading the pinctrl driver which should call
> gpiochip_irqchip_remove() (we don't have remove call back for pinctrl
> driver)
>
> > > > However, the mask will need to be dynamic, as GPIO interrupts can be
> > > > mapped and unmapped to one of the 32 available interrupts dynamically,
> > > > right?
> > > Yep that's correct.
> > >
> > > > I'm not sure if that can be done easily: if gpiochip_irqchip_irq_valid()
> > > > is ever called too early, before the mapping is done, it would fail.
> > > >
> > > The mask initialization is a one time process and that is during
> > > adding the GPIO chip. At this stage we won't be knowing what will be
> > > the valid GPIO pins used as interrupts. Maybe the core needs to
> > > implement a callback which lands in the GPIO controller driver to tell
> > > if the gpio irq line is valid. This way we can handle dynamic
> > > interrupts.
> >
> > Upon closer look, I think the mask is a red herring, and we don't
> > need it.
> Agreed.
>
> > But we do need to handle the (possible) mismatch between GPIO
> > offset (index) and IRQ offset in the above code.
> >
> Agreed, do you see any possibility of the mismatch I have missed?

gpiochip_to_irq():

if (irq_domain_is_hierarchy(domain)) {
struct irq_fwspec spec;

spec.fwnode = domain->fwnode;
spec.param_count = 2;
spec.param[0] = gc->irq.child_offset_to_irq(gc, offset);
spec.param[1] = IRQ_TYPE_NONE;

return irq_create_fwspec_mapping(&spec);
}

Same here: in the absence of a child_offset_to_irq() callback,
the default gpiochip_child_offset_to_irq_noop() will be used,
assuming an identity mapping between GPIO numbers and IRQ
numbers.

So perhaps
1. you need to provide a child_offset_to_irq() callback,
2. gpiochip_irqchip_remove() needs to apply the child_offset_to_irq()
mapping too?
3. you do need the mask, or let child_offset_to_irq() an error code,
to avoid irq_{find,dispose}_mapping() handling non-existent irqs?

Or am I missing something?

I guess this is easy to verify by adding some debug prints to the code.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2022-05-14 00:18:18

[permalink] [raw]

Subject: RE: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Prabhakar,

Thanks for the patch.

> Prabhakar Mahadev Lad <[email protected]>
> Subject: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to
> handle GPIO interrupt
>
> Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
>
> GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be used as
> IRQ lines at given time. Selection of pins as IRQ lines is handled by IA55
> (which is the IRQC block) which sits in between the GPIO and GIC.

Do we need to update bindings with interrupt-cells on [1] like [2] as it act as parent for GPIO interrupts?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/pinctrl/renesas,rzg2l-pinctrl.yaml?h=next-20220511

[2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/gpio/renesas,rcar-gpio.yaml?h=next-20220511#n81

Cheers,
Biju

>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> drivers/pinctrl/renesas/pinctrl-rzg2l.c | 202 ++++++++++++++++++++++++
> 1 file changed, 202 insertions(+)
>
> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index a48cac55152c..af2c739cdbaa 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -9,8 +9,10 @@
> #include <linux/clk.h>
> #include <linux/gpio/driver.h>
> #include <linux/io.h>
> +#include <linux/interrupt.h>
> #include <linux/module.h>
> #include <linux/of_device.h>
> +#include <linux/of_irq.h>
> #include <linux/pinctrl/pinconf-generic.h> #include
> <linux/pinctrl/pinconf.h> #include <linux/pinctrl/pinctrl.h> @@ -89,6
> +91,7 @@
> #define PIN(n) (0x0800 + 0x10 + (n))
> #define IOLH(n) (0x1000 + (n) * 8)
> #define IEN(n) (0x1800 + (n) * 8)
> +#define ISEL(n) (0x2c80 + (n) * 8)
> #define PWPR (0x3014)
> #define SD_CH(n) (0x3000 + (n) * 4)
> #define QSPI (0x3008)
> @@ -112,6 +115,10 @@
> #define RZG2L_PIN_ID_TO_PORT_OFFSET(id) (RZG2L_PIN_ID_TO_PORT(id) +
> 0x10)
> #define RZG2L_PIN_ID_TO_PIN(id) ((id) % RZG2L_PINS_PER_PORT)
>
> +#define RZG2L_TINT_MAX_INTERRUPT 32
> +#define RZG2L_TINT_IRQ_START_INDEX 9
> +#define RZG2L_PACK_HWIRQ(t, i) (((t) << 16) | (i))
> +
> struct rzg2l_dedicated_configs {
> const char *name;
> u32 config;
> @@ -137,6 +144,9 @@ struct rzg2l_pinctrl {
>
> struct gpio_chip gpio_chip;
> struct pinctrl_gpio_range gpio_range;
> + DECLARE_BITMAP(tint_slot, RZG2L_TINT_MAX_INTERRUPT);
> + spinlock_t bitmap_lock;
> + unsigned int hwirq[RZG2L_TINT_MAX_INTERRUPT];
>
> spinlock_t lock;
> };
> @@ -883,6 +893,8 @@ static int rzg2l_gpio_get(struct gpio_chip *chip,
> unsigned int offset)
>
> static void rzg2l_gpio_free(struct gpio_chip *chip, unsigned int offset)
> {
> + unsigned int virq;
> +
> pinctrl_gpio_free(chip->base + offset);
>
> /*
> @@ -890,6 +902,10 @@ static void rzg2l_gpio_free(struct gpio_chip *chip,
> unsigned int offset)
> * drive the GPIO pin as an output.
> */
> rzg2l_gpio_direction_input(chip, offset);
> +
> + virq = irq_find_mapping(chip->irq.domain, offset);
> + if (virq)
> + irq_dispose_mapping(virq);
> }
>
> static const char * const rzg2l_gpio_names[] = { @@ -1104,14 +1120,190 @@
> static struct {
> }
> };
>
> +static int rzg2l_gpio_get_gpioint(unsigned int virq) {
> + unsigned int gpioint;
> + unsigned int i;
> + u32 port, bit;
> +
> + port = virq / 8;
> + bit = virq % 8;
> +
> + if (port >= ARRAY_SIZE(rzg2l_gpio_configs) ||
> + bit >= RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[port]))
> + return -EINVAL;
> +
> + gpioint = bit;
> + for (i = 0; i < port; i++)
> + gpioint += RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[i]);
> +
> + return gpioint;
> +}
> +
> +static void rzg2l_gpio_irq_domain_free(struct irq_domain *domain, unsigned
> int virq,
> + unsigned int nr_irqs)
> +{
> + struct irq_data *d;
> +
> + d = irq_domain_get_irq_data(domain, virq);
> + if (d) {
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct rzg2l_pinctrl *pctrl = container_of(gc, struct
> rzg2l_pinctrl, gpio_chip);
> + irq_hw_number_t hwirq = irqd_to_hwirq(d);
> + unsigned long flags;
> + unsigned int i;
> +
> + for (i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) {
> + if (pctrl->hwirq[i] == hwirq) {
> + spin_lock_irqsave(&pctrl->bitmap_lock, flags);
> + bitmap_release_region(pctrl->tint_slot, i,
> get_order(1));
> + spin_unlock_irqrestore(&pctrl->bitmap_lock,
> flags);
> + pctrl->hwirq[i] = 0;
> + break;
> + }
> + }
> + }
> + irq_domain_free_irqs_common(domain, virq, nr_irqs); }
> +
> +static void rzg2l_gpio_irq_disable(struct irq_data *d) {
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl,
> gpio_chip);
> + unsigned int hwirq = irqd_to_hwirq(d);
> + unsigned long flags;
> + void __iomem *addr;
> + u32 port;
> + u8 bit;
> +
> + port = RZG2L_PIN_ID_TO_PORT(hwirq);
> + bit = RZG2L_PIN_ID_TO_PIN(hwirq);
> +
> + addr = pctrl->base + ISEL(port);
> + if (bit >= 4) {
> + bit -= 4;
> + addr += 4;
> + }
> +
> + spin_lock_irqsave(&pctrl->lock, flags);
> + writel(readl(addr) & ~BIT(bit * 8), addr);
> + spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> + irq_chip_disable_parent(d);
> +}
> +
> +static void rzg2l_gpio_irq_enable(struct irq_data *d) {
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl,
> gpio_chip);
> + unsigned int hwirq = irqd_to_hwirq(d);
> + unsigned long flags;
> + void __iomem *addr;
> + u32 port;
> + u8 bit;
> +
> + port = RZG2L_PIN_ID_TO_PORT(hwirq);
> + bit = RZG2L_PIN_ID_TO_PIN(hwirq);
> +
> + addr = pctrl->base + ISEL(port);
> + if (bit >= 4) {
> + bit -= 4;
> + addr += 4;
> + }
> +
> + spin_lock_irqsave(&pctrl->lock, flags);
> + writel(readl(addr) | BIT(bit * 8), addr);
> + spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> + irq_chip_enable_parent(d);
> +}
> +
> +static int rzg2l_gpio_irq_set_type(struct irq_data *d, unsigned int
> +type) {
> + return irq_chip_set_type_parent(d, type); }
> +
> +static void rzg2l_gpio_irqc_eoi(struct irq_data *d) {
> + irq_chip_eoi_parent(d);
> +}
> +
> +static struct irq_chip rzg2l_gpio_irqchip = {
> + .name = "rzg2l-gpio",
> + .irq_disable = rzg2l_gpio_irq_disable,
> + .irq_enable = rzg2l_gpio_irq_enable,
> + .irq_mask = irq_chip_mask_parent,
> + .irq_unmask = irq_chip_unmask_parent,
> + .irq_set_type = rzg2l_gpio_irq_set_type,
> + .irq_eoi = rzg2l_gpio_irqc_eoi,
> +};
> +
> +static int rzg2l_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
> + unsigned int child,
> + unsigned int child_type,
> + unsigned int *parent,
> + unsigned int *parent_type)
> +{
> + struct rzg2l_pinctrl *pctrl = gpiochip_get_data(gc);
> + unsigned long flags;
> + int gpioint, irq;
> +
> + gpioint = rzg2l_gpio_get_gpioint(child);
> + if (gpioint < 0)
> + return gpioint;
> +
> + spin_lock_irqsave(&pctrl->bitmap_lock, flags);
> + irq = bitmap_find_free_region(pctrl->tint_slot,
> RZG2L_TINT_MAX_INTERRUPT, get_order(1));
> + spin_unlock_irqrestore(&pctrl->bitmap_lock, flags);
> + if (irq < 0)
> + return -ENOSPC;
> + pctrl->hwirq[irq] = child;
> + irq += RZG2L_TINT_IRQ_START_INDEX;
> +
> + /* All these interrupts are level high in the CPU */
> + *parent_type = IRQ_TYPE_LEVEL_HIGH;
> + *parent = RZG2L_PACK_HWIRQ(gpioint, irq);
> + return 0;
> +}
> +
> +static void *rzg2l_gpio_populate_parent_fwspec(struct gpio_chip *chip,
> + unsigned int parent_hwirq,
> + unsigned int parent_type)
> +{
> + struct irq_fwspec *fwspec;
> +
> + fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
> + if (!fwspec)
> + return NULL;
> +
> + fwspec->fwnode = chip->irq.parent_domain->fwnode;
> + fwspec->param_count = 2;
> + fwspec->param[0] = parent_hwirq;
> + fwspec->param[1] = parent_type;
> +
> + return fwspec;
> +}
> +
> static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) {
> struct device_node *np = pctrl->dev->of_node;
> struct gpio_chip *chip = &pctrl->gpio_chip;
> const char *name = dev_name(pctrl->dev);
> + struct irq_domain *parent_domain;
> struct of_phandle_args of_args;
> + struct device_node *parent_np;
> + struct gpio_irq_chip *girq;
> int ret;
>
> + parent_np = of_irq_find_parent(np);
> + if (!parent_np)
> + return -ENXIO;
> +
> + parent_domain = irq_find_host(parent_np);
> + of_node_put(parent_np);
> + if (!parent_domain)
> + return -EPROBE_DEFER;
> +
> ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0,
> &of_args);
> if (ret) {
> dev_err(pctrl->dev, "Unable to parse gpio-ranges\n"); @@ -
> 1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl
> *pctrl)
> chip->base = -1;
> chip->ngpio = of_args.args[2];
>
> + girq = &chip->irq;
> + girq->chip = &rzg2l_gpio_irqchip;
> + girq->fwnode = of_node_to_fwnode(np);
> + girq->parent_domain = parent_domain;
> + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq;
> + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec;
> + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free;
> + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT;
> +
> pctrl->gpio_range.id = 0;
> pctrl->gpio_range.pin_base = 0;
> pctrl->gpio_range.base = 0;
> @@ -1253,6 +1454,7 @@ static int rzg2l_pinctrl_probe(struct platform_device
> *pdev)
> }
>
> spin_lock_init(&pctrl->lock);
> + spin_lock_init(&pctrl->bitmap_lock);
>
> platform_set_drvdata(pdev, pctrl);
>
> --
> 2.25.1

2022-05-14 00:47:41

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

On Fri, May 13, 2022 at 7:12 AM Biju Das <[email protected]> wrote:
>
>
>
> > -----Original Message-----
> > From: Biju Das
> > Sent: 12 May 2022 18:59
> > To: Lad, Prabhakar <[email protected]>
> > Cc: Prabhakar Mahadev Lad <[email protected]>; Geert
> > Uytterhoeven <[email protected]>; Linus Walleij
> > <[email protected]>; Thomas Gleixner <[email protected]>; Marc
> > Zyngier <[email protected]>; Rob Herring <[email protected]>; Krzysztof
> > Kozlowski <[email protected]>; Bartosz Golaszewski
> > <[email protected]>; Philipp Zabel <[email protected]>; linux-
> > [email protected]; [email protected]; linux-renesas-
> > [email protected]; [email protected]; Phil Edworthy
> > <[email protected]>
> > Subject: RE: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain
> > to handle GPIO interrupt
> >
> > Hi Prabhakar,
> >
> > > Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ
> > > domain to handle GPIO interrupt
> > >
> > > Hi Biju,
> > >
> > > Thank you for the review.
> > >
> > > On Thu, May 12, 2022 at 6:35 AM Biju Das <[email protected]>
> > > wrote:
> > > >
> > > > Hi Prabhakar,
> > > >
> > > > Thanks for the patch.
> > > >
> > > > > Prabhakar Mahadev Lad <[email protected]>
> > > > > Subject: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ
> > > > > domain to handle GPIO interrupt
> > > > >
> > > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
> > > > >
> > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can
> > > > > be used as IRQ lines at given time. Selection of pins as IRQ lines
> > > > > is handled by IA55 (which is the IRQC block) which sits in between
> > > > > the
> > > GPIO and GIC.
> > > >
> > > > Do we need to update bindings with interrupt-cells on [1] like [2]
> > > > as it
> > > act as parent for GPIO interrupts?
> > > >
> > > Yes interrupt-controller and interrupt-parent needs to be added. I'm
> > > wondering if "interrupt-cells" is not required. If the pin is an
> > > interrupt it will be passed as an GPIO.
> >
> > It is same as external interrupt case right?
> >
> > For eg:- Ethernet PHY case,
> >
> > interrupt-parent = <&irqc>;
> > interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> >
> > if you use GPIO, it will be like this right?
> >
> > interrupt-parent = <&pinctrl>;
> > interrupts = <RZG2L_GPIO(1, 0) IRQ_TYPE_LEVEL_LOW>;
>
> FYI,
>
> Previously, I have tested ADV HPD interrupt with below changes while investigating [1]
>
> interrupt-parent = <&pinctrl>;
> interrupts = <RZG2L_GPIO(2, 1) IRQ_TYPE_EDGE_FALLING>;
>
Right, #interrupt-cells=<2> , where the first cell is the GPIO pin and
the second cell is the flag.

Cheers,
Prabhakar

2022-05-14 01:05:00

[permalink] [raw]

Subject: RE: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Prabhakar,

> Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain
> to handle GPIO interrupt
>
> Hi Biju,
>
> Thank you for the review.
>
> On Thu, May 12, 2022 at 6:35 AM Biju Das <[email protected]>
> wrote:
> >
> > Hi Prabhakar,
> >
> > Thanks for the patch.
> >
> > > Prabhakar Mahadev Lad <[email protected]>
> > > Subject: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ
> > > domain to handle GPIO interrupt
> > >
> > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
> > >
> > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> > > used as IRQ lines at given time. Selection of pins as IRQ lines is
> > > handled by IA55 (which is the IRQC block) which sits in between the
> GPIO and GIC.
> >
> > Do we need to update bindings with interrupt-cells on [1] like [2] as it
> act as parent for GPIO interrupts?
> >
> Yes interrupt-controller and interrupt-parent needs to be added. I'm
> wondering if "interrupt-cells" is not required. If the pin is an interrupt
> it will be passed as an GPIO.

It is same as external interrupt case right?

For eg:- Ethernet PHY case,

interrupt-parent = <&irqc>;
interrupts = <3 IRQ_TYPE_LEVEL_LOW>;

if you use GPIO, it will be like this right?

interrupt-parent = <&pinctrl>;
interrupts = <RZG2L_GPIO(43, 0) IRQ_TYPE_LEVEL_LOW>;

Cheers,
Biju

>
> @Geert - your thoughts ?
>
> Cheers,
> Prabhakar

2022-05-14 01:06:26

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Prabhakar,

On Thu, May 12, 2022 at 7:36 PM Lad, Prabhakar
<[email protected]> wrote:
> On Thu, May 12, 2022 at 8:39 AM Geert Uytterhoeven <[email protected]> wrote:
> > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar
> > <[email protected]> wrote:
> > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
> > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> > > used as IRQ lines at given time. Selection of pins as IRQ lines
> > > is handled by IA55 (which is the IRQC block) which sits in between the
> > > GPIO and GIC.
> > >
> > > Signed-off-by: Lad Prabhakar <[email protected]>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> >
> > > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > > {
> > > struct device_node *np = pctrl->dev->of_node;
> > > struct gpio_chip *chip = &pctrl->gpio_chip;
> > > const char *name = dev_name(pctrl->dev);
> > > + struct irq_domain *parent_domain;
> > > struct of_phandle_args of_args;
> > > + struct device_node *parent_np;
> > > + struct gpio_irq_chip *girq;
> > > int ret;
> > >
> > > + parent_np = of_irq_find_parent(np);
> > > + if (!parent_np)
> > > + return -ENXIO;
> > > +
> > > + parent_domain = irq_find_host(parent_np);
> > > + of_node_put(parent_np);
> > > + if (!parent_domain)
> > > + return -EPROBE_DEFER;
> > > +
> > > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args);
> > > if (ret) {
> > > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n");
> > > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > > chip->base = -1;
> > > chip->ngpio = of_args.args[2];
> > >
> > > + girq = &chip->irq;
> > > + girq->chip = &rzg2l_gpio_irqchip;
> > > + girq->fwnode = of_node_to_fwnode(np);
> > > + girq->parent_domain = parent_domain;
> > > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq;
> > > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec;
> > > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free;
> > > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT;
> > > +
> >
> > I think you need to provide a .init_valid_mask() callback, as
> > gpiochip_irqchip_remove() relies on that for destroying interrupts.
> Are you suggesting the callback to avoid looping through all the GPIO pins?

gpiochip_irqchip_remove() does:

/* Remove all IRQ mappings and delete the domain */
if (gc->irq.domain) {
unsigned int irq;

for (offset = 0; offset < gc->ngpio; offset++) {
if (!gpiochip_irqchip_irq_valid(gc, offset))
continue;

irq = irq_find_mapping(gc->irq.domain, offset);
irq_dispose_mapping(irq);
}

irq_domain_remove(gc->irq.domain);

}

The main thing is not about avoiding to loop through all GPIO pins,
but to avoid irq_{find,dispose}_mapping() doing the wrong thing.
The loop is over all GPIO offsets, while not all of them are mapped
to valid interrupts. Does the above work correctly?

> > However, the mask will need to be dynamic, as GPIO interrupts can be
> > mapped and unmapped to one of the 32 available interrupts dynamically,
> > right?
> Yep that's correct.
>
> > I'm not sure if that can be done easily: if gpiochip_irqchip_irq_valid()
> > is ever called too early, before the mapping is done, it would fail.
> >
> The mask initialization is a one time process and that is during
> adding the GPIO chip. At this stage we won't be knowing what will be
> the valid GPIO pins used as interrupts. Maybe the core needs to
> implement a callback which lands in the GPIO controller driver to tell
> if the gpio irq line is valid. This way we can handle dynamic
> interrupts.

Upon closer look, I think the mask is a red herring, and we don't
need it.
But we do need to handle the (possible) mismatch between GPIO
offset (index) and IRQ offset in the above code.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2022-05-14 01:34:24

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Geert

On Fri, May 13, 2022 at 3:29 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Fri, May 13, 2022 at 3:56 PM Lad, Prabhakar
> <[email protected]> wrote:
> > On Fri, May 13, 2022 at 7:53 AM Geert Uytterhoeven <[email protected]> wrote:
> > > On Thu, May 12, 2022 at 7:36 PM Lad, Prabhakar
> > > <[email protected]> wrote:
> > > > On Thu, May 12, 2022 at 8:39 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar
> > > > > <[email protected]> wrote:
> > > > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
> > > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> > > > > > used as IRQ lines at given time. Selection of pins as IRQ lines
> > > > > > is handled by IA55 (which is the IRQC block) which sits in between the
> > > > > > GPIO and GIC.
> > > > > >
> > > > > > Signed-off-by: Lad Prabhakar <[email protected]>
> > > > >
> > > > > Thanks for your patch!
> > > > >
> > > > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > >
> > > > > > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > > > > > {
> > > > > > struct device_node *np = pctrl->dev->of_node;
> > > > > > struct gpio_chip *chip = &pctrl->gpio_chip;
> > > > > > const char *name = dev_name(pctrl->dev);
> > > > > > + struct irq_domain *parent_domain;
> > > > > > struct of_phandle_args of_args;
> > > > > > + struct device_node *parent_np;
> > > > > > + struct gpio_irq_chip *girq;
> > > > > > int ret;
> > > > > >
> > > > > > + parent_np = of_irq_find_parent(np);
> > > > > > + if (!parent_np)
> > > > > > + return -ENXIO;
> > > > > > +
> > > > > > + parent_domain = irq_find_host(parent_np);
> > > > > > + of_node_put(parent_np);
> > > > > > + if (!parent_domain)
> > > > > > + return -EPROBE_DEFER;
> > > > > > +
> > > > > > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args);
> > > > > > if (ret) {
> > > > > > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n");
> > > > > > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > > > > > chip->base = -1;
> > > > > > chip->ngpio = of_args.args[2];
> > > > > >
> > > > > > + girq = &chip->irq;
> > > > > > + girq->chip = &rzg2l_gpio_irqchip;
> > > > > > + girq->fwnode = of_node_to_fwnode(np);
> > > > > > + girq->parent_domain = parent_domain;
> > > > > > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq;
> > > > > > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec;
> > > > > > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free;
> > > > > > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT;
> > > > > > +
> > > > >
> > > > > I think you need to provide a .init_valid_mask() callback, as
> > > > > gpiochip_irqchip_remove() relies on that for destroying interrupts.
> > > > Are you suggesting the callback to avoid looping through all the GPIO pins?
> > >
> > > gpiochip_irqchip_remove() does:
> > >
> > > /* Remove all IRQ mappings and delete the domain */
> > > if (gc->irq.domain) {
> > > unsigned int irq;
> > >
> > > for (offset = 0; offset < gc->ngpio; offset++) {
> > > if (!gpiochip_irqchip_irq_valid(gc, offset))
> > > continue;
> > >
> > > irq = irq_find_mapping(gc->irq.domain, offset);
> > > irq_dispose_mapping(irq);
> > > }
> > >
> > > irq_domain_remove(gc->irq.domain);
> > >
> > > }
> > >
> > > The main thing is not about avoiding to loop through all GPIO pins,
> > > but to avoid irq_{find,dispose}_mapping() doing the wrong thing.
> > So in our case if we don't implement valid masks, that would mean all
> > the pins are valid. irq_find_mapping() would return 0 if no mapping is
> > found to the corresponding offset and irq_dispose_mapping() would
> > simply return back without doing anything if virq == 0.(In this patch
> > rzg2l_gpio_free() does call irq_{find,dispose}_mapping())
>
> But "offset" is a number from the GPIO offset space (0-122), while

The "offset" reported by kernel is 120-511:

root@smarc-rzg2l:~# cat /sys/kernel/debug/gpio
gpiochip0: GPIOs 120-511, parent: platform/11030000.pinctrl, 11030000.pinctrl:
gpio-120 (P0_0 )
gpio-121 (P0_1 )
gpio-122 (P0_2 )
gpio-123 (P0_3 )
gpio-124 (P0_4 )
.....
gpio-507 (P48_3 )
gpio-508 (P48_4 )
gpio-509 (P48_5 )
gpio-510 (P48_6 )
gpio-511 (P48_7 )

> irq_find_mapping() expects a number from the domain's IRQ space,
> which is only 0-31?
>
Nope, let me demonstrate with an example, I have configured the gpio
pins as GPIO keys in DTS:

+ keyboard {
+ compatible = "gpio-keys";
+ status = "okay";
+
+ key-1 {
+ gpios = <&pinctrl RZG2L_GPIO(43, 0) GPIO_ACTIVE_HIGH>;
+ linux,code = <KEY_1>;
+ linux,input-type = <EV_KEY>;
+ wakeup-source;
+ label = "SW1";
+ };
+
+ key-2 {
+ gpios = <&pinctrl RZG2L_GPIO(41, 0) GPIO_ACTIVE_HIGH>;
+ linux,code = <KEY_2>;
+ linux,input-type = <EV_KEY>;
+ wakeup-source;
+ label = "SW2";
+ };
+
+ key-3 {
+ gpios = <&pinctrl RZG2L_GPIO(43, 1) GPIO_ACTIVE_HIGH>;
+ linux,code = <KEY_3>;
+ linux,input-type = <EV_KEY>;
+ wakeup-source;
+ label = "SW3";
+ };
+ };

root@smarc-rzg2l:~# cat /proc/interrupts | grep SW
root@smarc-rzg2l:~#
root@smarc-rzg2l:~# insmod gpio_keys.ko
[ 925.002720] input: keyboard as /devices/platform/keyboard/input/input3
root@smarc-rzg2l:~# cat /proc/interrupts | grep SW
82: 0 0 11030000.pinctrl 344 Edge SW1
83: 0 0 11030000.pinctrl 328 Edge SW2
84: 0 0 11030000.pinctrl 345 Edge SW3
root@smarc-rzg2l:~#

In here 82/83/84 are virq and 344/328/345 are hwirq, which can be
confirmed from sysfs file:

root@smarc-rzg2l:~# cat /sys/kernel/debug/irq/irqs/82
handler: handle_fasteoi_irq
device: (null)
status: 0x00000001
istate: 0x00000000
ddepth: 0
wdepth: 0
dstate: 0x13400201
IRQ_TYPE_EDGE_RISING
IRQD_ACTIVATED
IRQD_IRQ_STARTED
IRQD_SINGLE_TARGET
IRQD_DEFAULT_TRIGGER_SET
IRQD_HANDLE_ENFORCE_IRQCTX
node: 0
affinity: 0-1
effectiv:
domain: :soc:pinctrl@11030000
hwirq: 0x158
chip: 11030000.pinctrl
flags: 0x800
IRQCHIP_IMMUTABLE
parent:
domain: :soc:interrupt-controller@110a0000
hwirq: 0x9
chip: rzg2l-irqc
flags: 0x15
IRQCHIP_SET_TYPE_MASKED
IRQCHIP_MASK_ON_SUSPEND
IRQCHIP_SKIP_SET_WAKE
parent:
domain: :soc:interrupt-controller@11900000-1
hwirq: 0x1dc
chip: GICv3
flags: 0x15
IRQCHIP_SET_TYPE_MASKED
IRQCHIP_MASK_ON_SUSPEND
IRQCHIP_SKIP_SET_WAKE
root@smarc-rzg2l:~#
root@smarc-rzg2l:~#
root@smarc-rzg2l:~# cat /sys/kernel/debug/irq/irqs/83
handler: handle_fasteoi_irq
device: (null)
status: 0x00000001
istate: 0x00000000
ddepth: 0
wdepth: 0
dstate: 0x13400201
IRQ_TYPE_EDGE_RISING
IRQD_ACTIVATED
IRQD_IRQ_STARTED
IRQD_SINGLE_TARGET
IRQD_DEFAULT_TRIGGER_SET
IRQD_HANDLE_ENFORCE_IRQCTX
node: 0
affinity: 0-1
effectiv:
domain: :soc:pinctrl@11030000
hwirq: 0x148
chip: 11030000.pinctrl
flags: 0x800
IRQCHIP_IMMUTABLE
parent:
domain: :soc:interrupt-controller@110a0000
hwirq: 0xa
chip: rzg2l-irqc
flags: 0x15
IRQCHIP_SET_TYPE_MASKED
IRQCHIP_MASK_ON_SUSPEND
IRQCHIP_SKIP_SET_WAKE
parent:
domain: :soc:interrupt-controller@11900000-1
hwirq: 0x1dd
chip: GICv3
flags: 0x15
IRQCHIP_SET_TYPE_MASKED
IRQCHIP_MASK_ON_SUSPEND
IRQCHIP_SKIP_SET_WAKE
root@smarc-rzg2l:~#
root@smarc-rzg2l:~# cat /sys/kernel/debug/irq/irqs/84
handler: handle_fasteoi_irq
device: (null)
status: 0x00000001
istate: 0x00000000
ddepth: 0
wdepth: 0
dstate: 0x13400201
IRQ_TYPE_EDGE_RISING
IRQD_ACTIVATED
IRQD_IRQ_STARTED
IRQD_SINGLE_TARGET
IRQD_DEFAULT_TRIGGER_SET
IRQD_HANDLE_ENFORCE_IRQCTX
node: 0
affinity: 0-1
effectiv:
domain: :soc:pinctrl@11030000
hwirq: 0x159
chip: 11030000.pinctrl
flags: 0x800
IRQCHIP_IMMUTABLE
parent:
domain: :soc:interrupt-controller@110a0000
hwirq: 0xb
chip: rzg2l-irqc
flags: 0x15
IRQCHIP_SET_TYPE_MASKED
IRQCHIP_MASK_ON_SUSPEND
IRQCHIP_SKIP_SET_WAKE
parent:
domain: :soc:interrupt-controller@11900000-1
hwirq: 0x1de
chip: GICv3
flags: 0x15
IRQCHIP_SET_TYPE_MASKED
IRQCHIP_MASK_ON_SUSPEND
IRQCHIP_SKIP_SET_WAKE
root@smarc-rzg2l:~#
root@smarc-rzg2l:~#
root@smarc-rzg2l:~#

root@smarc-rzg2l:~# rmmod gpio_keys.ko
[ 1143.037314] rzg2l_gpio_free offset:345 virq:84
[ 1143.042488] rzg2l_gpio_free offset:328 virq:83
[ 1143.048700] rzg2l_gpio_free offset:344 virq:82
root@smarc-rzg2l:~#
root@smarc-rzg2l:~#

I have added print in gpio_free callback where
irq_{find,dispose}_mapping()) prints the correct value above.

> > > The loop is over all GPIO offsets, while not all of them are mapped
> > > to valid interrupts. Does the above work correctly?
> > >
> > I haven't tested unloading the pinctrl driver which should call
> > gpiochip_irqchip_remove() (we don't have remove call back for pinctrl
> > driver)
> >
> > > > > However, the mask will need to be dynamic, as GPIO interrupts can be
> > > > > mapped and unmapped to one of the 32 available interrupts dynamically,
> > > > > right?
> > > > Yep that's correct.
> > > >
> > > > > I'm not sure if that can be done easily: if gpiochip_irqchip_irq_valid()
> > > > > is ever called too early, before the mapping is done, it would fail.
> > > > >
> > > > The mask initialization is a one time process and that is during
> > > > adding the GPIO chip. At this stage we won't be knowing what will be
> > > > the valid GPIO pins used as interrupts. Maybe the core needs to
> > > > implement a callback which lands in the GPIO controller driver to tell
> > > > if the gpio irq line is valid. This way we can handle dynamic
> > > > interrupts.
> > >
> > > Upon closer look, I think the mask is a red herring, and we don't
> > > need it.
> > Agreed.
> >
> > > But we do need to handle the (possible) mismatch between GPIO
> > > offset (index) and IRQ offset in the above code.
> > >
> > Agreed, do you see any possibility of the mismatch I have missed?
>
> gpiochip_to_irq():
>
> if (irq_domain_is_hierarchy(domain)) {
> struct irq_fwspec spec;
>
> spec.fwnode = domain->fwnode;
> spec.param_count = 2;
> spec.param[0] = gc->irq.child_offset_to_irq(gc, offset);
> spec.param[1] = IRQ_TYPE_NONE;
>
> return irq_create_fwspec_mapping(&spec);
> }
>
> Same here: in the absence of a child_offset_to_irq() callback,
> the default gpiochip_child_offset_to_irq_noop() will be used,
> assuming an identity mapping between GPIO numbers and IRQ
> numbers.
>
Agreed, gpiochip_child_offset_to_irq_noop will return the "offset",
but irq_create_fwspec_mapping() in gpiochip_to_irq() will return the
virq number which will not be equal to the offset.

I added the below change in gpio_keys.c where it calls gpiod_to_irq()
-> to_irq() and the below is the log:
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -589,6 +589,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
button->gpio, error);
return error;
}
+ dev_err(dev,"%s gpiod_to_irq() = (irq) %d\n",
__func__, irq);
+
bdata->irq = irq;
}

root@smarc-rzg2l:~# insmod gpio_keys.ko
[ 54.288678] gpio-keys keyboard: gpio_keys_setup_key gpiod_to_irq() = (irq) 82
[ 54.297230] gpio-keys keyboard: gpio_keys_setup_key gpiod_to_irq() = (irq) 83
[ 54.311256] gpio-keys keyboard: gpio_keys_setup_key gpiod_to_irq() = (irq) 84
[ 54.332560] input: keyboard as /devices/platform/keyboard/input/input0
root@smarc-rzg2l:~#

> So perhaps
> 1. you need to provide a child_offset_to_irq() callback,
> 2. gpiochip_irqchip_remove() needs to apply the child_offset_to_irq()
> mapping too?
> 3. you do need the mask, or let child_offset_to_irq() an error code,
> to avoid irq_{find,dispose}_mapping() handling non-existent irqs?
>
From the above logs, I don't think this is needed. Please correct me
if I am wrong.

> Or am I missing something?
>
> I guess this is easy to verify by adding some debug prints to the code.
>
Let me know if you want me to add debug prints at specific places.

Cheers,
Prabhakar

2022-05-14 01:50:36

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Marc,

Thank you for the review.

On Thu, May 12, 2022 at 12:15 PM Marc Zyngier <[email protected]> wrote:
>
> On Wed, 11 May 2022 19:32:10 +0100,
> Lad Prabhakar <[email protected]> wrote:
> >
> > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
> >
> > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> > used as IRQ lines at given time. Selection of pins as IRQ lines
> > is handled by IA55 (which is the IRQC block) which sits in between the
> > GPIO and GIC.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > drivers/pinctrl/renesas/pinctrl-rzg2l.c | 202 ++++++++++++++++++++++++
> > 1 file changed, 202 insertions(+)
> >
> > diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > index a48cac55152c..af2c739cdbaa 100644
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -9,8 +9,10 @@
> > #include <linux/clk.h>
> > #include <linux/gpio/driver.h>
> > #include <linux/io.h>
> > +#include <linux/interrupt.h>
> > #include <linux/module.h>
> > #include <linux/of_device.h>
> > +#include <linux/of_irq.h>
> > #include <linux/pinctrl/pinconf-generic.h>
> > #include <linux/pinctrl/pinconf.h>
> > #include <linux/pinctrl/pinctrl.h>
> > @@ -89,6 +91,7 @@
> > #define PIN(n) (0x0800 + 0x10 + (n))
> > #define IOLH(n) (0x1000 + (n) * 8)
> > #define IEN(n) (0x1800 + (n) * 8)
> > +#define ISEL(n) (0x2c80 + (n) * 8)
> > #define PWPR (0x3014)
> > #define SD_CH(n) (0x3000 + (n) * 4)
> > #define QSPI (0x3008)
> > @@ -112,6 +115,10 @@
> > #define RZG2L_PIN_ID_TO_PORT_OFFSET(id) (RZG2L_PIN_ID_TO_PORT(id) + 0x10)
> > #define RZG2L_PIN_ID_TO_PIN(id) ((id) % RZG2L_PINS_PER_PORT)
> >
> > +#define RZG2L_TINT_MAX_INTERRUPT 32
> > +#define RZG2L_TINT_IRQ_START_INDEX 9
> > +#define RZG2L_PACK_HWIRQ(t, i) (((t) << 16) | (i))
> > +
> > struct rzg2l_dedicated_configs {
> > const char *name;
> > u32 config;
> > @@ -137,6 +144,9 @@ struct rzg2l_pinctrl {
> >
> > struct gpio_chip gpio_chip;
> > struct pinctrl_gpio_range gpio_range;
> > + DECLARE_BITMAP(tint_slot, RZG2L_TINT_MAX_INTERRUPT);
> > + spinlock_t bitmap_lock;
> > + unsigned int hwirq[RZG2L_TINT_MAX_INTERRUPT];
> >
> > spinlock_t lock;
> > };
> > @@ -883,6 +893,8 @@ static int rzg2l_gpio_get(struct gpio_chip *chip, unsigned int offset)
> >
> > static void rzg2l_gpio_free(struct gpio_chip *chip, unsigned int offset)
> > {
> > + unsigned int virq;
> > +
> > pinctrl_gpio_free(chip->base + offset);
> >
> > /*
> > @@ -890,6 +902,10 @@ static void rzg2l_gpio_free(struct gpio_chip *chip, unsigned int offset)
> > * drive the GPIO pin as an output.
> > */
> > rzg2l_gpio_direction_input(chip, offset);
> > +
> > + virq = irq_find_mapping(chip->irq.domain, offset);
> > + if (virq)
> > + irq_dispose_mapping(virq);
> > }
> >
> > static const char * const rzg2l_gpio_names[] = {
> > @@ -1104,14 +1120,190 @@ static struct {
> > }
> > };
> >
> > +static int rzg2l_gpio_get_gpioint(unsigned int virq)
> > +{
> > + unsigned int gpioint;
> > + unsigned int i;
> > + u32 port, bit;
> > +
> > + port = virq / 8;
> > + bit = virq % 8;
> > +
> > + if (port >= ARRAY_SIZE(rzg2l_gpio_configs) ||
> > + bit >= RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[port]))
> > + return -EINVAL;
> > +
> > + gpioint = bit;
> > + for (i = 0; i < port; i++)
> > + gpioint += RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[i]);
> > +
> > + return gpioint;
> > +}
> > +
> > +static void rzg2l_gpio_irq_domain_free(struct irq_domain *domain, unsigned int virq,
> > + unsigned int nr_irqs)
> > +{
> > + struct irq_data *d;
> > +
> > + d = irq_domain_get_irq_data(domain, virq);
> > + if (d) {
> > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
> > + irq_hw_number_t hwirq = irqd_to_hwirq(d);
> > + unsigned long flags;
> > + unsigned int i;
> > +
> > + for (i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) {
> > + if (pctrl->hwirq[i] == hwirq) {
> > + spin_lock_irqsave(&pctrl->bitmap_lock, flags);
> > + bitmap_release_region(pctrl->tint_slot, i, get_order(1));
> > + spin_unlock_irqrestore(&pctrl->bitmap_lock, flags);
> > + pctrl->hwirq[i] = 0;
> > + break;
> > + }
> > + }
> > + }
> > + irq_domain_free_irqs_common(domain, virq, nr_irqs);
> > +}
> > +
> > +static void rzg2l_gpio_irq_disable(struct irq_data *d)
> > +{
> > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
> > + unsigned int hwirq = irqd_to_hwirq(d);
> > + unsigned long flags;
> > + void __iomem *addr;
> > + u32 port;
> > + u8 bit;
> > +
> > + port = RZG2L_PIN_ID_TO_PORT(hwirq);
> > + bit = RZG2L_PIN_ID_TO_PIN(hwirq);
> > +
> > + addr = pctrl->base + ISEL(port);
> > + if (bit >= 4) {
> > + bit -= 4;
> > + addr += 4;
> > + }
> > +
> > + spin_lock_irqsave(&pctrl->lock, flags);
> > + writel(readl(addr) & ~BIT(bit * 8), addr);
> > + spin_unlock_irqrestore(&pctrl->lock, flags);
> > +
> > + irq_chip_disable_parent(d);
> > +}
> > +
> > +static void rzg2l_gpio_irq_enable(struct irq_data *d)
> > +{
> > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
> > + unsigned int hwirq = irqd_to_hwirq(d);
> > + unsigned long flags;
> > + void __iomem *addr;
> > + u32 port;
> > + u8 bit;
> > +
> > + port = RZG2L_PIN_ID_TO_PORT(hwirq);
> > + bit = RZG2L_PIN_ID_TO_PIN(hwirq);
> > +
> > + addr = pctrl->base + ISEL(port);
> > + if (bit >= 4) {
> > + bit -= 4;
> > + addr += 4;
> > + }
> > +
> > + spin_lock_irqsave(&pctrl->lock, flags);
> > + writel(readl(addr) | BIT(bit * 8), addr);
> > + spin_unlock_irqrestore(&pctrl->lock, flags);
> > +
> > + irq_chip_enable_parent(d);
> > +}
> > +
> > +static int rzg2l_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> > +{
> > + return irq_chip_set_type_parent(d, type);
> > +}
> > +
> > +static void rzg2l_gpio_irqc_eoi(struct irq_data *d)
> > +{
> > + irq_chip_eoi_parent(d);
> > +}
> > +
> > +static struct irq_chip rzg2l_gpio_irqchip = {
> > + .name = "rzg2l-gpio",
> > + .irq_disable = rzg2l_gpio_irq_disable,
> > + .irq_enable = rzg2l_gpio_irq_enable,
> > + .irq_mask = irq_chip_mask_parent,
> > + .irq_unmask = irq_chip_unmask_parent,
> > + .irq_set_type = rzg2l_gpio_irq_set_type,
> > + .irq_eoi = rzg2l_gpio_irqc_eoi,
>
> Please see the changes[1] that are queued in -next around immutable
> GPIO irqchips. This needs to be made const, the enable/disable methods
> have the right callbacks added, the resource management methods
> plumbed, and the correct flag exposed.
>
Thank you for the pointer, I'll rebase my patches on top of it and
implement an immutable GPIO irqchip.

Cheers,
Prabhakar

> > +};
> > +
> > +static int rzg2l_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
> > + unsigned int child,
> > + unsigned int child_type,
> > + unsigned int *parent,
> > + unsigned int *parent_type)
> > +{
> > + struct rzg2l_pinctrl *pctrl = gpiochip_get_data(gc);
> > + unsigned long flags;
> > + int gpioint, irq;
> > +
> > + gpioint = rzg2l_gpio_get_gpioint(child);
> > + if (gpioint < 0)
> > + return gpioint;
> > +
> > + spin_lock_irqsave(&pctrl->bitmap_lock, flags);
> > + irq = bitmap_find_free_region(pctrl->tint_slot, RZG2L_TINT_MAX_INTERRUPT, get_order(1));
> > + spin_unlock_irqrestore(&pctrl->bitmap_lock, flags);
> > + if (irq < 0)
> > + return -ENOSPC;
> > + pctrl->hwirq[irq] = child;
> > + irq += RZG2L_TINT_IRQ_START_INDEX;
> > +
> > + /* All these interrupts are level high in the CPU */
> > + *parent_type = IRQ_TYPE_LEVEL_HIGH;
> > + *parent = RZG2L_PACK_HWIRQ(gpioint, irq);
> > + return 0;
> > +}
> > +
> > +static void *rzg2l_gpio_populate_parent_fwspec(struct gpio_chip *chip,
> > + unsigned int parent_hwirq,
> > + unsigned int parent_type)
> > +{
> > + struct irq_fwspec *fwspec;
> > +
> > + fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
> > + if (!fwspec)
> > + return NULL;
> > +
> > + fwspec->fwnode = chip->irq.parent_domain->fwnode;
> > + fwspec->param_count = 2;
> > + fwspec->param[0] = parent_hwirq;
> > + fwspec->param[1] = parent_type;
> > +
> > + return fwspec;
> > +}
> > +
> > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > {
> > struct device_node *np = pctrl->dev->of_node;
> > struct gpio_chip *chip = &pctrl->gpio_chip;
> > const char *name = dev_name(pctrl->dev);
> > + struct irq_domain *parent_domain;
> > struct of_phandle_args of_args;
> > + struct device_node *parent_np;
> > + struct gpio_irq_chip *girq;
> > int ret;
> >
> > + parent_np = of_irq_find_parent(np);
> > + if (!parent_np)
> > + return -ENXIO;
> > +
> > + parent_domain = irq_find_host(parent_np);
> > + of_node_put(parent_np);
> > + if (!parent_domain)
> > + return -EPROBE_DEFER;
> > +
> > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args);
> > if (ret) {
> > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n");
> > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > chip->base = -1;
> > chip->ngpio = of_args.args[2];
> >
> > + girq = &chip->irq;
>
> Same thing, this needs to use the appropriate setter.
>
> > + girq->chip = &rzg2l_gpio_irqchip;
> > + girq->fwnode = of_node_to_fwnode(np);
> > + girq->parent_domain = parent_domain;
> > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq;
> > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec;
> > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free;
> > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT;
> > +
> > pctrl->gpio_range.id = 0;
> > pctrl->gpio_range.pin_base = 0;
> > pctrl->gpio_range.base = 0;
> > @@ -1253,6 +1454,7 @@ static int rzg2l_pinctrl_probe(struct platform_device *pdev)
> > }
> >
> > spin_lock_init(&pctrl->lock);
> > + spin_lock_init(&pctrl->bitmap_lock);
> >
> > platform_set_drvdata(pdev, pctrl);
> >
> > --
> > 2.25.1
> >
> >
>
> Thanks,
>
> M.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gpio-immutable
>
> --
> Without deviation from the norm, progress is not possible.

2022-05-14 03:19:26

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Geert,

On Fri, May 13, 2022 at 7:53 AM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Thu, May 12, 2022 at 7:36 PM Lad, Prabhakar
> <[email protected]> wrote:
> > On Thu, May 12, 2022 at 8:39 AM Geert Uytterhoeven <[email protected]> wrote:
> > > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar
> > > <[email protected]> wrote:
> > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
> > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> > > > used as IRQ lines at given time. Selection of pins as IRQ lines
> > > > is handled by IA55 (which is the IRQC block) which sits in between the
> > > > GPIO and GIC.
> > > >
> > > > Signed-off-by: Lad Prabhakar <[email protected]>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > >
> > > > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > > > {
> > > > struct device_node *np = pctrl->dev->of_node;
> > > > struct gpio_chip *chip = &pctrl->gpio_chip;
> > > > const char *name = dev_name(pctrl->dev);
> > > > + struct irq_domain *parent_domain;
> > > > struct of_phandle_args of_args;
> > > > + struct device_node *parent_np;
> > > > + struct gpio_irq_chip *girq;
> > > > int ret;
> > > >
> > > > + parent_np = of_irq_find_parent(np);
> > > > + if (!parent_np)
> > > > + return -ENXIO;
> > > > +
> > > > + parent_domain = irq_find_host(parent_np);
> > > > + of_node_put(parent_np);
> > > > + if (!parent_domain)
> > > > + return -EPROBE_DEFER;
> > > > +
> > > > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args);
> > > > if (ret) {
> > > > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n");
> > > > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > > > chip->base = -1;
> > > > chip->ngpio = of_args.args[2];
> > > >
> > > > + girq = &chip->irq;
> > > > + girq->chip = &rzg2l_gpio_irqchip;
> > > > + girq->fwnode = of_node_to_fwnode(np);
> > > > + girq->parent_domain = parent_domain;
> > > > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq;
> > > > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec;
> > > > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free;
> > > > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT;
> > > > +
> > >
> > > I think you need to provide a .init_valid_mask() callback, as
> > > gpiochip_irqchip_remove() relies on that for destroying interrupts.
> > Are you suggesting the callback to avoid looping through all the GPIO pins?
>
> gpiochip_irqchip_remove() does:
>
> /* Remove all IRQ mappings and delete the domain */
> if (gc->irq.domain) {
> unsigned int irq;
>
> for (offset = 0; offset < gc->ngpio; offset++) {
> if (!gpiochip_irqchip_irq_valid(gc, offset))
> continue;
>
> irq = irq_find_mapping(gc->irq.domain, offset);
> irq_dispose_mapping(irq);
> }
>
> irq_domain_remove(gc->irq.domain);
>
> }
>
> The main thing is not about avoiding to loop through all GPIO pins,
> but to avoid irq_{find,dispose}_mapping() doing the wrong thing.
So in our case if we don't implement valid masks, that would mean all
the pins are valid. irq_find_mapping() would return 0 if no mapping is
found to the corresponding offset and irq_dispose_mapping() would
simply return back without doing anything if virq == 0.(In this patch
rzg2l_gpio_free() does call irq_{find,dispose}_mapping())

> The loop is over all GPIO offsets, while not all of them are mapped
> to valid interrupts. Does the above work correctly?
>
I haven't tested unloading the pinctrl driver which should call
gpiochip_irqchip_remove() (we don't have remove call back for pinctrl
driver)

> > > However, the mask will need to be dynamic, as GPIO interrupts can be
> > > mapped and unmapped to one of the 32 available interrupts dynamically,
> > > right?
> > Yep that's correct.
> >
> > > I'm not sure if that can be done easily: if gpiochip_irqchip_irq_valid()
> > > is ever called too early, before the mapping is done, it would fail.
> > >
> > The mask initialization is a one time process and that is during
> > adding the GPIO chip. At this stage we won't be knowing what will be
> > the valid GPIO pins used as interrupts. Maybe the core needs to
> > implement a callback which lands in the GPIO controller driver to tell
> > if the gpio irq line is valid. This way we can handle dynamic
> > interrupts.
>
> Upon closer look, I think the mask is a red herring, and we don't
> need it.
Agreed.

> But we do need to handle the (possible) mismatch between GPIO
> offset (index) and IRQ offset in the above code.
>
Agreed, do you see any possibility of the mismatch I have missed?

Cheers,
Prabhakar

2022-05-14 03:39:21

[permalink] [raw]

Subject: RE: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

> -----Original Message-----
> From: Biju Das
> Sent: 12 May 2022 18:59
> To: Lad, Prabhakar <[email protected]>
> Cc: Prabhakar Mahadev Lad <[email protected]>; Geert
> Uytterhoeven <[email protected]>; Linus Walleij
> <[email protected]>; Thomas Gleixner <[email protected]>; Marc
> Zyngier <[email protected]>; Rob Herring <[email protected]>; Krzysztof
> Kozlowski <[email protected]>; Bartosz Golaszewski
> <[email protected]>; Philipp Zabel <[email protected]>; linux-
> [email protected]; [email protected]; linux-renesas-
> [email protected]; [email protected]; Phil Edworthy
> <[email protected]>
> Subject: RE: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain
> to handle GPIO interrupt
>
> Hi Prabhakar,
>
> > Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ
> > domain to handle GPIO interrupt
> >
> > Hi Biju,
> >
> > Thank you for the review.
> >
> > On Thu, May 12, 2022 at 6:35 AM Biju Das <[email protected]>
> > wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > Thanks for the patch.
> > >
> > > > Prabhakar Mahadev Lad <[email protected]>
> > > > Subject: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ
> > > > domain to handle GPIO interrupt
> > > >
> > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
> > > >
> > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can
> > > > be used as IRQ lines at given time. Selection of pins as IRQ lines
> > > > is handled by IA55 (which is the IRQC block) which sits in between
> > > > the
> > GPIO and GIC.
> > >
> > > Do we need to update bindings with interrupt-cells on [1] like [2]
> > > as it
> > act as parent for GPIO interrupts?
> > >
> > Yes interrupt-controller and interrupt-parent needs to be added. I'm
> > wondering if "interrupt-cells" is not required. If the pin is an
> > interrupt it will be passed as an GPIO.
>
> It is same as external interrupt case right?
>
> For eg:- Ethernet PHY case,
>
> interrupt-parent = <&irqc>;
> interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
>
> if you use GPIO, it will be like this right?
>
> interrupt-parent = <&pinctrl>;
> interrupts = <RZG2L_GPIO(1, 0) IRQ_TYPE_LEVEL_LOW>;

FYI,

Previously, I have tested ADV HPD interrupt with below changes while investigating [1]

interrupt-parent = <&pinctrl>;
interrupts = <RZG2L_GPIO(2, 1) IRQ_TYPE_EDGE_FALLING>;

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220512&id=04b19d32213654e54ec819b6ac033360f1551902

>
> Cheers,
> Biju
>
>
>
>
>
>
> >
> > @Geert - your thoughts ?
> >
> > Cheers,
> > Prabhakar

2022-05-15 22:05:01

[permalink] [raw]

Subject: RE: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Prabhakar,

Thanks for the example.

> Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain
> to handle GPIO interrupt
>
> > But "offset" is a number from the GPIO offset space (0-122), while
>
> The "offset" reported by kernel is 120-511:
>
> root@smarc-rzg2l:~# cat /sys/kernel/debug/gpio
> gpiochip0: GPIOs 120-511, parent: platform/11030000.pinctrl,
> 11030000.pinctrl:
> gpio-120 (P0_0 )
> gpio-121 (P0_1 )
> gpio-122 (P0_2 )
> gpio-123 (P0_3 )
> gpio-124 (P0_4 )
> .....
> gpio-507 (P48_3 )
> gpio-508 (P48_4 )
> gpio-509 (P48_5 )
> gpio-510 (P48_6 )
> gpio-511 (P48_7 )
>
> > irq_find_mapping() expects a number from the domain's IRQ space, which
> > is only 0-31?
> >
> Nope, let me demonstrate with an example, I have configured the gpio pins
> as GPIO keys in DTS:
>
> + keyboard {
> + compatible = "gpio-keys";
> + status = "okay";
> +
> + key-1 {
> + gpios = <&pinctrl RZG2L_GPIO(43, 0)
> GPIO_ACTIVE_HIGH>;
> + linux,code = <KEY_1>;
> + linux,input-type = <EV_KEY>;
> + wakeup-source;
> + label = "SW1";
> + };
> +
> + key-2 {
> + gpios = <&pinctrl RZG2L_GPIO(41, 0)
> GPIO_ACTIVE_HIGH>;
> + linux,code = <KEY_2>;
> + linux,input-type = <EV_KEY>;
> + wakeup-source;
> + label = "SW2";
> + };
> +
> + key-3 {
> + gpios = <&pinctrl RZG2L_GPIO(43, 1)
> GPIO_ACTIVE_HIGH>;
> + linux,code = <KEY_3>;
> + linux,input-type = <EV_KEY>;
> + wakeup-source;
> + label = "SW3";
> + };
> + };
>
> root@smarc-rzg2l:~# cat /proc/interrupts | grep SW root@smarc-rzg2l:~#
> root@smarc-rzg2l:~# insmod gpio_keys.ko [ 925.002720] input: keyboard as
> /devices/platform/keyboard/input/input3
> root@smarc-rzg2l:~# cat /proc/interrupts | grep SW
> 82: 0 0 11030000.pinctrl 344 Edge SW1
> 83: 0 0 11030000.pinctrl 328 Edge SW2
> 84: 0 0 11030000.pinctrl 345 Edge SW3
> root@smarc-rzg2l:~#
>
> In here 82/83/84 are virq and 344/328/345 are hwirq, which can be confirmed
> from sysfs file:

From your example, Looks like

I believe from interrupt statistics point of view, cat /proc/interrupts should report actual gpioint number (0->122) corresponding to pin index for SW1, SW2 and SW3 ??

May be another mapping required for pinindex to gpioint to get proper statistics??

From usage point, another point is, who will track gpioint statistics, pinctrl driver or framework??

Example Use case:- create gpioint0-30 which will fill tint0-tint30.

Then insmod gpioint corresponding to SW1 and trigger 1 interrupt and check cat /proc/interrupts for tint31 and SW1
Then rmmode gpioint corresponding to SW1 and insmod SW2 and trigger 5 interrupts and check cat /proc/interrupts for tint31 and SW2
Then rmmode gpioint corresponding to SW2 and insmod SW3 and trigger 7 interrupts and check cat /proc/interrupts for tint31 and SW3
Then rmmode gpioint corresponding to SW3 and insmod SW1 and check cat /proc/interrupts for tint31 and SW1
Then rmmode gpioint corresponding to SW1 and insmod SW2 and check cat /proc/interrupts for tint31 and SW2

Tint31 should report 13 interrupts
gpioint corresponding to SW1 should report 1 interrupt
gpioint corresponding to SW2 should report 5 interrupts
gpioint corresponding to SW3 should report 7 interrupts

Cheers,
Biju

2022-05-16 10:24:00

[permalink] [raw]

Subject: RE: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Marc,

> Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ
> domain to handle GPIO interrupt
>
> On Mon, 16 May 2022 09:33:03 +0100,
> Biju Das <[email protected]> wrote:
> >
> > Hi Marc,
> >
> > .org>; Phil Edworthy
> > > <[email protected]>
> > > Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ
> > > domain to handle GPIO interrupt
> > >
> > > On Mon, 16 May 2022 08:20:47 +0100,
> > > Biju Das <[email protected]> wrote:
> > > >
> > > > > >
> > > > > > I believe from interrupt statistics point of view, cat
> > > > > > /proc/interrupts should report actual gpioint number (0->122)
> > > > > > corresponding to pin index for SW1, SW2 and SW3 ??
> > > > >
> > > > > No. There is no need for such userspace-visible behaviour.
> > > > > Userspace has no business tracking those. The required
> > > > > information is in debugfs, and that more than enough.
> > > >
> > > > Ok, So far I used cat /proc/interrupts for debugging, since I
> > > > don't need to enable DEBUG config for Enabling Debugfs for irq.
> > > > This Debugfs irq is new info to me.
> > > >
> > > > Our hardware manual has below info for usb-phy irq
> > > > 2H0_OBINT 126(InterruptID) SPI 94 IRQ 94 Level
> > > >
> > > > cat /proc/interrupts matches with GICV3 Interrupt ID/ type in the
> > > > HW
> > > manual
> > > > 113: 0 0 GICv3 126 Level 11c50200.usb-phy
> > > >
> > > > Debugfs is also showing similar info like hwirq and interrupt type.
> > > > But I don't know which field corresponds to number of interrupts?
> > > >
> > > > root@smarc-rzg2l:~# cat /sys/kernel/debug/irq/irqs/113
> > > > handler: handle_fasteoi_irq
> > > > device: (null)
> > > > status: 0x00000104
> > > > istate: 0x00000000
> > > > ddepth: 0
> > > > wdepth: 0
> > > > dstate: 0x13402204
> > > > IRQ_TYPE_LEVEL_HIGH
> > > > IRQD_LEVEL
> > > > IRQD_ACTIVATED
> > > > IRQD_IRQ_STARTED
> > > > IRQD_SINGLE_TARGET
> > > > IRQD_DEFAULT_TRIGGER_SET
> > > > IRQD_HANDLE_ENFORCE_IRQCTX
> > > > node: 0
> > > > affinity: 0-1
> > > > effectiv: 0
> > > > domain: :soc:interrupt-controller@11900000-1
> > > > hwirq: 0x7e
> > >
> > > 0x7e = 126 = 94 - 32 -> SPI94.
> > >
> > > What else do you need?
> >
> > OK, similar to GIC, I thought for gpio interrupts,
>
> Err. This *IS* the GIC.

Yes, tint0-31(IA55 driver) is connected to the GIC which is backend
On the frontend(Pincontrol driver) where we have 123 interrupts (gpioint0-122) is mapped to tint0_31.
So parent of a particular gpio interrupt can be changed during insmod/rmmod operation.

>
> > The hwirq should match with gpiointN mentioned in hwmanual. That is
> all.
>
> There is no such need.
>
> hwirq is whatever the driver decides it is, and userspace can't rely on it
> one way or another. If the driver author decides that it is more
> convenient to number hwirqs backwards, that's absolutely fine. If you are
> debugging, you have access to the driver code, the DT, and all the debugfs
> information. You can trace things back and forth as you please.

OK agreed.

Regards,
Biju

2022-05-16 12:55:07

[permalink] [raw]

Subject: RE: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Marc,

Thanks for the feedback.

> Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain
> to handle GPIO interrupt
>
> On Sun, 15 May 2022 06:13:22 +0100,
> Biju Das <[email protected]> wrote:
> >
> > Hi Prabhakar,
> >
> > Thanks for the example.
> >
> > > Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ
> > > domain to handle GPIO interrupt
> > >
> > > > But "offset" is a number from the GPIO offset space (0-122), while
> > >
> > > The "offset" reported by kernel is 120-511:
> > >
> > > root@smarc-rzg2l:~# cat /sys/kernel/debug/gpio
> > > gpiochip0: GPIOs 120-511, parent: platform/11030000.pinctrl,
> > > 11030000.pinctrl:
> > > gpio-120 (P0_0 )
> > > gpio-121 (P0_1 )
> > > gpio-122 (P0_2 )
> > > gpio-123 (P0_3 )
> > > gpio-124 (P0_4 )
> > > .....
> > > gpio-507 (P48_3 )
> > > gpio-508 (P48_4 )
> > > gpio-509 (P48_5 )
> > > gpio-510 (P48_6 )
> > > gpio-511 (P48_7 )
> > >
> > > > irq_find_mapping() expects a number from the domain's IRQ space,
> > > > which is only 0-31?
> > > >
> > > Nope, let me demonstrate with an example, I have configured the gpio
> > > pins as GPIO keys in DTS:
> > >
> > > + keyboard {
> > > + compatible = "gpio-keys";
> > > + status = "okay";
> > > +
> > > + key-1 {
> > > + gpios = <&pinctrl RZG2L_GPIO(43, 0)
> > > GPIO_ACTIVE_HIGH>;
> > > + linux,code = <KEY_1>;
> > > + linux,input-type = <EV_KEY>;
> > > + wakeup-source;
> > > + label = "SW1";
> > > + };
> > > +
> > > + key-2 {
> > > + gpios = <&pinctrl RZG2L_GPIO(41, 0)
> > > GPIO_ACTIVE_HIGH>;
> > > + linux,code = <KEY_2>;
> > > + linux,input-type = <EV_KEY>;
> > > + wakeup-source;
> > > + label = "SW2";
> > > + };
> > > +
> > > + key-3 {
> > > + gpios = <&pinctrl RZG2L_GPIO(43, 1)
> > > GPIO_ACTIVE_HIGH>;
> > > + linux,code = <KEY_3>;
> > > + linux,input-type = <EV_KEY>;
> > > + wakeup-source;
> > > + label = "SW3";
> > > + };
> > > + };
> > >
> > > root@smarc-rzg2l:~# cat /proc/interrupts | grep SW
> > > root@smarc-rzg2l:~# root@smarc-rzg2l:~# insmod gpio_keys.ko [
> > > 925.002720] input: keyboard as
> > > /devices/platform/keyboard/input/input3
> > > root@smarc-rzg2l:~# cat /proc/interrupts | grep SW
> > > 82: 0 0 11030000.pinctrl 344 Edge SW1
> > > 83: 0 0 11030000.pinctrl 328 Edge SW2
> > > 84: 0 0 11030000.pinctrl 345 Edge SW3
> > > root@smarc-rzg2l:~#
> > >
> > > In here 82/83/84 are virq and 344/328/345 are hwirq, which can be
> > > confirmed from sysfs file:
> >
> > From your example, Looks like
> >
> > I believe from interrupt statistics point of view, cat
> > /proc/interrupts should report actual gpioint number (0->122)
> > corresponding to pin index for SW1, SW2 and SW3 ??
>
> No. There is no need for such userspace-visible behaviour. Userspace has no
> business tracking those. The required information is in debugfs, and that
> more than enough.

Ok, So far I used cat /proc/interrupts for debugging, since I don't need to enable DEBUG config for
Enabling Debugfs for irq. This Debugfs irq is new info to me.

Our hardware manual has below info for usb-phy irq
2H0_OBINT 126(InterruptID) SPI 94 IRQ 94 Level

cat /proc/interrupts matches with GICV3 Interrupt ID/ type in the HW manual
113: 0 0 GICv3 126 Level 11c50200.usb-phy

Debugfs is also showing similar info like hwirq and interrupt type. But I don't know which field corresponds to number
of interrupts?

root@smarc-rzg2l:~# cat /sys/kernel/debug/irq/irqs/113
handler: handle_fasteoi_irq
device: (null)
status: 0x00000104
istate: 0x00000000
ddepth: 0
wdepth: 0
dstate: 0x13402204
IRQ_TYPE_LEVEL_HIGH
IRQD_LEVEL
IRQD_ACTIVATED
IRQD_IRQ_STARTED
IRQD_SINGLE_TARGET
IRQD_DEFAULT_TRIGGER_SET
IRQD_HANDLE_ENFORCE_IRQCTX
node: 0
affinity: 0-1
effectiv: 0
domain: :soc:interrupt-controller@11900000-1
hwirq: 0x7e
chip: GICv3
flags: 0x15
IRQCHIP_SET_TYPE_MASKED
IRQCHIP_MASK_ON_SUSPEND
IRQCHIP_SKIP_SET_WAKE

Now coming to current case,

Currently GPIO INT 0-122(123 interrupts) corresponding to 120-511(291 interrupts) with same invalid lines.

From a debugging point, If user has put same irq name for gpioints(cat /proc/interrupts case), then how do we distinguish these interrupts??
(using hwirq??)

For using Debugfs, Do we need to first execute cat /proc/interrupts to get virq and from there we need to use virq to get statistics, right?

Cheers,
Biju

2022-05-16 14:55:25

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Prabhakar,

On Fri, May 13, 2022 at 8:13 PM Lad, Prabhakar
<[email protected]> wrote:
> On Fri, May 13, 2022 at 3:29 PM Geert Uytterhoeven <[email protected]> wrote:
> > On Fri, May 13, 2022 at 3:56 PM Lad, Prabhakar
> > <[email protected]> wrote:
> > > On Fri, May 13, 2022 at 7:53 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > On Thu, May 12, 2022 at 7:36 PM Lad, Prabhakar
> > > > <[email protected]> wrote:
> > > > > On Thu, May 12, 2022 at 8:39 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > > > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar
> > > > > > <[email protected]> wrote:
> > > > > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
> > > > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> > > > > > > used as IRQ lines at given time. Selection of pins as IRQ lines
> > > > > > > is handled by IA55 (which is the IRQC block) which sits in between the
> > > > > > > GPIO and GIC.
> > > > > > >
> > > > > > > Signed-off-by: Lad Prabhakar <[email protected]>
> > > > > >
> > > > > > Thanks for your patch!
> > > > > >
> > > > > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > > >
> > > > > > > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > > > > > > {
> > > > > > > struct device_node *np = pctrl->dev->of_node;
> > > > > > > struct gpio_chip *chip = &pctrl->gpio_chip;
> > > > > > > const char *name = dev_name(pctrl->dev);
> > > > > > > + struct irq_domain *parent_domain;
> > > > > > > struct of_phandle_args of_args;
> > > > > > > + struct device_node *parent_np;
> > > > > > > + struct gpio_irq_chip *girq;
> > > > > > > int ret;
> > > > > > >
> > > > > > > + parent_np = of_irq_find_parent(np);
> > > > > > > + if (!parent_np)
> > > > > > > + return -ENXIO;
> > > > > > > +
> > > > > > > + parent_domain = irq_find_host(parent_np);
> > > > > > > + of_node_put(parent_np);
> > > > > > > + if (!parent_domain)
> > > > > > > + return -EPROBE_DEFER;
> > > > > > > +
> > > > > > > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args);
> > > > > > > if (ret) {
> > > > > > > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n");
> > > > > > > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > > > > > > chip->base = -1;
> > > > > > > chip->ngpio = of_args.args[2];
> > > > > > >
> > > > > > > + girq = &chip->irq;
> > > > > > > + girq->chip = &rzg2l_gpio_irqchip;
> > > > > > > + girq->fwnode = of_node_to_fwnode(np);
> > > > > > > + girq->parent_domain = parent_domain;
> > > > > > > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq;
> > > > > > > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec;
> > > > > > > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free;
> > > > > > > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT;
> > > > > > > +
> > > > > >
> > > > > > I think you need to provide a .init_valid_mask() callback, as
> > > > > > gpiochip_irqchip_remove() relies on that for destroying interrupts.
> > > > > Are you suggesting the callback to avoid looping through all the GPIO pins?
> > > >
> > > > gpiochip_irqchip_remove() does:
> > > >
> > > > /* Remove all IRQ mappings and delete the domain */
> > > > if (gc->irq.domain) {
> > > > unsigned int irq;
> > > >
> > > > for (offset = 0; offset < gc->ngpio; offset++) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > if (!gpiochip_irqchip_irq_valid(gc, offset))
> > > > continue;
> > > >
> > > > irq = irq_find_mapping(gc->irq.domain, offset);
> > > > irq_dispose_mapping(irq);
> > > > }
> > > >
> > > > irq_domain_remove(gc->irq.domain);
> > > >
> > > > }
> > > >
> > > > The main thing is not about avoiding to loop through all GPIO pins,
> > > > but to avoid irq_{find,dispose}_mapping() doing the wrong thing.
> > > So in our case if we don't implement valid masks, that would mean all
> > > the pins are valid. irq_find_mapping() would return 0 if no mapping is
> > > found to the corresponding offset and irq_dispose_mapping() would
> > > simply return back without doing anything if virq == 0.(In this patch
> > > rzg2l_gpio_free() does call irq_{find,dispose}_mapping())
> >
> > But "offset" is a number from the GPIO offset space (0-122), while
>
> The "offset" reported by kernel is 120-511:

Offsets 120-511 are global GPIO numbers, i.e. starting from
gpio_chip.base.
The loop in gpiochip_irqchip_remove() uses local GPIO numbers,
starting from zero.
So these offsets are not the same.

Likewise, I believe the "offset" passed to irq_find_mapping() is an
irq number (hwirq) local to the domain, i.e. also starting at 0.
And it must be smaller than the size (32) passed to
irq_domain_create_hierarchy().

When passed a non-zero size, irq_domain_create_hierarchy()
calls into __irq_domain_add(), with size == hwirq_max == 32:

/**
* __irq_domain_add() - Allocate a new irq_domain data structure
* @fwnode: firmware node for the interrupt controller
* @size: Size of linear map; 0 for radix mapping only
* @hwirq_max: Maximum number of interrupts supported by controller
* @direct_max: Maximum value of direct maps; Use ~0 for no limit; 0 for no
* direct mapping
* @ops: domain callbacks
* @host_data: Controller private data pointer
*
* Allocates and initializes an irq_domain structure.
* Returns pointer to IRQ domain, or NULL on failure.
*/
struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode,
unsigned int size,
irq_hw_number_t hwirq_max, int
direct_max,
const struct irq_domain_ops *ops,
void *host_data)

> > > > But we do need to handle the (possible) mismatch between GPIO
> > > > offset (index) and IRQ offset in the above code.
> > > >
> > > Agreed, do you see any possibility of the mismatch I have missed?
> >
> > gpiochip_to_irq():
> >
> > if (irq_domain_is_hierarchy(domain)) {
> > struct irq_fwspec spec;
> >
> > spec.fwnode = domain->fwnode;
> > spec.param_count = 2;
> > spec.param[0] = gc->irq.child_offset_to_irq(gc, offset);
> > spec.param[1] = IRQ_TYPE_NONE;
> >
> > return irq_create_fwspec_mapping(&spec);
> > }
> >
> > Same here: in the absence of a child_offset_to_irq() callback,
> > the default gpiochip_child_offset_to_irq_noop() will be used,
> > assuming an identity mapping between GPIO numbers and IRQ
> > numbers.
> >
> Agreed, gpiochip_child_offset_to_irq_noop will return the "offset",
> but irq_create_fwspec_mapping() in gpiochip_to_irq() will return the
> virq number which will not be equal to the offset.

Shouldn't spec.param[0] be in the range 0-31, as 32 is the size of
the IRQ domain allocated?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2022-05-16 20:23:54

[permalink] [raw]

Subject: RE: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

Hi Marc,

.org>; Phil Edworthy
> <[email protected]>
> Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ
> domain to handle GPIO interrupt
>
> On Mon, 16 May 2022 08:20:47 +0100,
> Biju Das <[email protected]> wrote:
> >
> > > >
> > > > I believe from interrupt statistics point of view, cat
> > > > /proc/interrupts should report actual gpioint number (0->122)
> > > > corresponding to pin index for SW1, SW2 and SW3 ??
> > >
> > > No. There is no need for such userspace-visible behaviour. Userspace
> > > has no business tracking those. The required information is in
> > > debugfs, and that more than enough.
> >
> > Ok, So far I used cat /proc/interrupts for debugging, since I don't
> > need to enable DEBUG config for Enabling Debugfs for irq. This Debugfs
> > irq is new info to me.
> >
> > Our hardware manual has below info for usb-phy irq
> > 2H0_OBINT 126(InterruptID) SPI 94 IRQ 94 Level
> >
> > cat /proc/interrupts matches with GICV3 Interrupt ID/ type in the HW
> manual
> > 113: 0 0 GICv3 126 Level 11c50200.usb-phy
> >
> > Debugfs is also showing similar info like hwirq and interrupt type.
> > But I don't know which field corresponds to number of interrupts?
> >
> > root@smarc-rzg2l:~# cat /sys/kernel/debug/irq/irqs/113
> > handler: handle_fasteoi_irq
> > device: (null)
> > status: 0x00000104
> > istate: 0x00000000
> > ddepth: 0
> > wdepth: 0
> > dstate: 0x13402204
> > IRQ_TYPE_LEVEL_HIGH
> > IRQD_LEVEL
> > IRQD_ACTIVATED
> > IRQD_IRQ_STARTED
> > IRQD_SINGLE_TARGET
> > IRQD_DEFAULT_TRIGGER_SET
> > IRQD_HANDLE_ENFORCE_IRQCTX
> > node: 0
> > affinity: 0-1
> > effectiv: 0
> > domain: :soc:interrupt-controller@11900000-1
> > hwirq: 0x7e
>
> 0x7e = 126 = 94 - 32 -> SPI94.
>
> What else do you need?

OK, similar to GIC, I thought for gpio interrupts,

The hwirq should match with gpiointN mentioned in hwmanual. That is all.
Any way it is minor thing, it may be not at all needed. Please ignore this.

Eg:-for gpioint0, it should be

root@smarc-rzg2l:~# cat /proc/interrupts | grep SW
82: 0 0 11030000.pinctrl 0 Edge XXX

Not like

root@smarc-rzg2l:~# cat /proc/interrupts | grep SW
82: 0 0 11030000.pinctrl 120 Edge XXX

Cheers,
Biju

>
> > chip: GICv3
> > flags: 0x15
> > IRQCHIP_SET_TYPE_MASKED
> > IRQCHIP_MASK_ON_SUSPEND
> > IRQCHIP_SKIP_SET_WAKE
> >
> > Now coming to current case,
> >
> > Currently GPIO INT 0-122(123 interrupts) corresponding to
> > 120-511(291 interrupts) with same invalid lines.
> >
> > From a debugging point, If user has put same irq name for gpioints(cat
> > /proc/interrupts case), then how do we distinguish these interrupts??
> > (using hwirq??)
>
> Yes.
>
> >
> > For using Debugfs, Do we need to first execute cat /proc/interrupts to
> > get virq and from there we need to use virq to get statistics, right?
>
> It depends what you want to do. /sys/kernel/debug/irq/irqs/ has the exact
> same information. The only thing /proc/interrupts has that debugfs doesn't
> is the per-CPU accounting of delivered interrupts.

2022-05-18 19:05:34