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 [off-list ref] wrote:

Hi Prabhakar,

On Thu, May 12, 2022 at 7:36 PM Lad, Prabhakar
[off-list ref] wrote:

quoted

On Thu, May 12, 2022 at 8:39 AM Geert Uytterhoeven [off-list ref] wrote:

quoted

On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar
[off-list ref] wrote:

quoted

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 <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

quoted

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

quoted

 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)

quoted

quoted

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.

quoted

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