[PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume

[PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in
gpio_irq_{en,dis}able*()") dropped the configuration of ISEL from
rzg2l_gpio_irq_enable()/rzg2l_gpio_irq_disable() and moved it to
rzg2l_gpio_child_to_parent_hwirq()/rzg2l_gpio_irq_domain_free() to fix
spurious IRQs.

The resume code used rzg2l_gpio_irq_enable() (called from
rzg2l_gpio_irq_restore()) to reconfigure the wakeup interrupts. Some
drivers (e.g. Ethernet) may also reconfigure interrupts in their own code,
eventually calling rzg2l_gpio_irq_enable(), when these are not wakeup
interrupts.

After commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL
in gpio_irq_{en,dis}able*()"), ISEL was no longer configured properly after
resume.

Fix this by adding rzg2l_gpio_irq_endisable() back into
rzg2l_gpio_irq_enable(), and by using its unlocked variant in
rzg2l_gpio_irq_restore(). Having IRQs enable in rzg2l_gpio_irq_enable()
should be safe with respect to spurious IRQs, as in the probe case IRQs are
enabled anyway in rzg2l_gpio_child_to_parent_hwirq(). No spurious IRQs
were detected on suspend/resume tests (executed on RZ/G3S).

Fixes: 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in gpio_irq_{en,dis}able*(")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index b182b3b8a542..6ae1ee3ffc81 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -2428,7 +2428,7 @@ static int rzg2l_gpio_get_gpioint(unsigned int virq, struct rzg2l_pinctrl *pctrl
 }
 
 static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
-				     unsigned int hwirq, bool enable)
+				     unsigned int hwirq, bool enable, bool lock)
 {
 	const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[hwirq];
 	u64 *pin_data = pin_desc->drv_data;
@@ -2443,12 +2443,16 @@ static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
 		addr += 4;
 	}
 
-	spin_lock_irqsave(&pctrl->lock, flags);
+	if (lock)
+		spin_lock_irqsave(&pctrl->lock, flags);
+
 	if (enable)
 		writel(readl(addr) | BIT(bit * 8), addr);
 	else
 		writel(readl(addr) & ~BIT(bit * 8), addr);
-	spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	if (lock)
+		spin_unlock_irqrestore(&pctrl->lock, flags);
 }
 
 static void rzg2l_gpio_irq_disable(struct irq_data *d)
@@ -2460,15 +2464,22 @@ static void rzg2l_gpio_irq_disable(struct irq_data *d)
 	gpiochip_disable_irq(gc, hwirq);
 }
 
-static void rzg2l_gpio_irq_enable(struct irq_data *d)
+static void rzg2l_gpio_irq_enable_helper(struct irq_data *d, bool lock)
 {
 	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);
 
 	gpiochip_enable_irq(gc, hwirq);
+	rzg2l_gpio_irq_endisable(pctrl, hwirq, true, lock);
 	irq_chip_enable_parent(d);
 }
 
+static void rzg2l_gpio_irq_enable(struct irq_data *d)
+{
+	rzg2l_gpio_irq_enable_helper(d, true);
+}
+
 static int rzg2l_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 {
 	return irq_chip_set_type_parent(d, type);
@@ -2570,7 +2581,7 @@ static int rzg2l_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
 		goto err;
 	}
 
-	rzg2l_gpio_irq_endisable(pctrl, child, true);
+	rzg2l_gpio_irq_endisable(pctrl, child, true, true);
 	pctrl->hwirq[irq] = child;
 	irq += pctrl->data->hwcfg->tint_start_index;
 
@@ -2617,7 +2628,7 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
 		spin_lock_irqsave(&pctrl->lock, flags);
 		ret = rzg2l_gpio_irq_set_type(data, irqd_get_trigger_type(data));
 		if (!ret && !irqd_irq_disabled(data))
-			rzg2l_gpio_irq_enable(data);
+			rzg2l_gpio_irq_enable_helper(data, false);
 		spin_unlock_irqrestore(&pctrl->lock, flags);
 
 		if (ret)
@@ -2640,7 +2651,7 @@ static void rzg2l_gpio_irq_domain_free(struct irq_domain *domain, unsigned int v
 
 		for (i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) {
 			if (pctrl->hwirq[i] == hwirq) {
-				rzg2l_gpio_irq_endisable(pctrl, hwirq, false);
+				rzg2l_gpio_irq_endisable(pctrl, hwirq, false, true);
 				rzg2l_gpio_free(gc, hwirq);
 				spin_lock_irqsave(&pctrl->bitmap_lock, flags);
 				bitmap_release_region(pctrl->tint_slot, i, get_order(1));
-- 
2.43.0

Re: [PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume

[Geert Uytterhoeven]

Hi Claudiu,

On Mon, 8 Sept 2025 at 16:42, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in
> gpio_irq_{en,dis}able*()") dropped the configuration of ISEL from
> rzg2l_gpio_irq_enable()/rzg2l_gpio_irq_disable() and moved it to
> rzg2l_gpio_child_to_parent_hwirq()/rzg2l_gpio_irq_domain_free() to fix
> spurious IRQs.
>
> The resume code used rzg2l_gpio_irq_enable() (called from
> rzg2l_gpio_irq_restore()) to reconfigure the wakeup interrupts. Some
> drivers (e.g. Ethernet) may also reconfigure interrupts in their own code,
> eventually calling rzg2l_gpio_irq_enable(), when these are not wakeup
> interrupts.
>
> After commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL
> in gpio_irq_{en,dis}able*()"), ISEL was no longer configured properly after
> resume.
>
> Fix this by adding rzg2l_gpio_irq_endisable() back into
> rzg2l_gpio_irq_enable(), and by using its unlocked variant in
> rzg2l_gpio_irq_restore(). Having IRQs enable in rzg2l_gpio_irq_enable()

enabled

> should be safe with respect to spurious IRQs, as in the probe case IRQs are
> enabled anyway in rzg2l_gpio_child_to_parent_hwirq(). No spurious IRQs
> were detected on suspend/resume tests (executed on RZ/G3S).
>
> Fixes: 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in gpio_irq_{en,dis}able*(")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

I have to admit I don't fully understand what is going on...

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -2428,7 +2428,7 @@ static int rzg2l_gpio_get_gpioint(unsigned int virq, struct rzg2l_pinctrl *pctrl
>  }
>
>  static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
> -                                    unsigned int hwirq, bool enable)
> +                                    unsigned int hwirq, bool enable, bool lock)
>  {
>         const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[hwirq];
>         u64 *pin_data = pin_desc->drv_data;
> @@ -2443,12 +2443,16 @@ static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
>                 addr += 4;
>         }
>
> -       spin_lock_irqsave(&pctrl->lock, flags);
> +       if (lock)
> +               spin_lock_irqsave(&pctrl->lock, flags);
> +
>         if (enable)
>                 writel(readl(addr) | BIT(bit * 8), addr);
>         else
>                 writel(readl(addr) & ~BIT(bit * 8), addr);
> -       spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +       if (lock)
> +               spin_unlock_irqrestore(&pctrl->lock, flags);
>  }

I am not so fond of these "if (lock) ..."-constructs, especially as
the function now takes two bool parameters, which is error-prone.

What about renaming rzg2l_gpio_irq_endisable() to
__rzg2l_gpio_irq_endisable(), and moving the locking to a wrapper
rzg2l_gpio_irq_endisable()?

    static void __rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
                                         unsigned int hwirq, bool enable)
    {
            /* old functionality without locking */
            ...
    }

    static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
                                        unsigned int hwirq, bool enable)
    {
            unsigned long flags;

            spin_lock_irqsave(&pctrl->lock, flags);
            __rzg2l_gpio_irq_endisable(pctrl, hwirq, enable);
            spin_unlock_irqrestore(&pctrl->lock, flags);
    }

Then no existing callers of rzg2l_gpio_irq_endisable() need to be
changed.

> @@ -2460,15 +2464,22 @@ static void rzg2l_gpio_irq_disable(struct irq_data *d)
>         gpiochip_disable_irq(gc, hwirq);
>  }
>
> -static void rzg2l_gpio_irq_enable(struct irq_data *d)
> +static void rzg2l_gpio_irq_enable_helper(struct irq_data *d, bool lock)

Here we can't do without the "lock" parameter, unless duplicating the
full body, so this is fine.  I'd rename it to __rzg2l_gpio_irq_enable(),
though.

>  {
>         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);
>
>         gpiochip_enable_irq(gc, hwirq);
> +       rzg2l_gpio_irq_endisable(pctrl, hwirq, true, lock);

if (lock)
    rzg2l_gpio_irq_endisable(pctrl, hwirq, true);
else
    __rzg2l_gpio_irq_endisable(pctrl, hwirq, true);

>         irq_chip_enable_parent(d);
>  }
>
> +static void rzg2l_gpio_irq_enable(struct irq_data *d)
> +{
> +       rzg2l_gpio_irq_enable_helper(d, true);

__rzg2l_gpio_irq_enable(d, true);

> +}
> +
>  static int rzg2l_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  {
>         return irq_chip_set_type_parent(d, type);
> @@ -2617,7 +2628,7 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
>                 spin_lock_irqsave(&pctrl->lock, flags);
>                 ret = rzg2l_gpio_irq_set_type(data, irqd_get_trigger_type(data));
>                 if (!ret && !irqd_irq_disabled(data))
> -                       rzg2l_gpio_irq_enable(data);
> +                       rzg2l_gpio_irq_enable_helper(data, false);

__rzg2l_gpio_irq_enable(data, false);

Before, the lock was taken again, while it was already held.
Didn't this cause a deadlock?

>                 spin_unlock_irqrestore(&pctrl->lock, flags);
>
>                 if (ret)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

RE: [PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume

[Biju Das]

Hi Claudiu,

> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: 08 September 2025 15:43
> Subject: [PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume
> 
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in
> gpio_irq_{en,dis}able*()") dropped the configuration of ISEL from
> rzg2l_gpio_irq_enable()/rzg2l_gpio_irq_disable() and moved it to
> rzg2l_gpio_child_to_parent_hwirq()/rzg2l_gpio_irq_domain_free() to fix spurious IRQs.
> 
> The resume code used rzg2l_gpio_irq_enable() (called from
> rzg2l_gpio_irq_restore()) to reconfigure the wakeup interrupts. Some drivers (e.g. Ethernet) may also
> reconfigure interrupts in their own code, eventually calling rzg2l_gpio_irq_enable(), when these are
> not wakeup interrupts.
> 
> After commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in
> gpio_irq_{en,dis}able*()"), ISEL was no longer configured properly after resume.
> 
> Fix this by adding rzg2l_gpio_irq_endisable() back into rzg2l_gpio_irq_enable(), and by using its
> unlocked variant in rzg2l_gpio_irq_restore(). Having IRQs enable in rzg2l_gpio_irq_enable() should be
> safe with respect to spurious IRQs, as in the probe case IRQs are enabled anyway in
> rzg2l_gpio_child_to_parent_hwirq(). No spurious IRQs were detected on suspend/resume tests (executed on
> RZ/G3S).

IIRC, I believe the issue is ISEL is not restored during resume. Can we restore this register just like
Schmitt register suspend/restore[1]

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20250911&id=837afa592c6234be82acb5d23e0a39e9befdaa85

Cheers,
Biju

> 
> Fixes: 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in gpio_irq_{en,dis}able*(")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index b182b3b8a542..6ae1ee3ffc81 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -2428,7 +2428,7 @@ static int rzg2l_gpio_get_gpioint(unsigned int virq, struct rzg2l_pinctrl
> *pctrl  }
> 
>  static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
> -				     unsigned int hwirq, bool enable)
> +				     unsigned int hwirq, bool enable, bool lock)
>  {
>  	const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[hwirq];
>  	u64 *pin_data = pin_desc->drv_data;
> @@ -2443,12 +2443,16 @@ static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
>  		addr += 4;
>  	}
> 
> -	spin_lock_irqsave(&pctrl->lock, flags);
> +	if (lock)
> +		spin_lock_irqsave(&pctrl->lock, flags);
> +
>  	if (enable)
>  		writel(readl(addr) | BIT(bit * 8), addr);
>  	else
>  		writel(readl(addr) & ~BIT(bit * 8), addr);
> -	spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	if (lock)
> +		spin_unlock_irqrestore(&pctrl->lock, flags);
>  }
> 
>  static void rzg2l_gpio_irq_disable(struct irq_data *d) @@ -2460,15 +2464,22 @@ static void
> rzg2l_gpio_irq_disable(struct irq_data *d)
>  	gpiochip_disable_irq(gc, hwirq);
>  }
> 
> -static void rzg2l_gpio_irq_enable(struct irq_data *d)
> +static void rzg2l_gpio_irq_enable_helper(struct irq_data *d, bool lock)
>  {
>  	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);
> 
>  	gpiochip_enable_irq(gc, hwirq);
> +	rzg2l_gpio_irq_endisable(pctrl, hwirq, true, lock);
>  	irq_chip_enable_parent(d);
>  }
> 
> +static void rzg2l_gpio_irq_enable(struct irq_data *d) {
> +	rzg2l_gpio_irq_enable_helper(d, true); }
> +
>  static int rzg2l_gpio_irq_set_type(struct irq_data *d, unsigned int type)  {
>  	return irq_chip_set_type_parent(d, type); @@ -2570,7 +2581,7 @@ static int
> rzg2l_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
>  		goto err;
>  	}
> 
> -	rzg2l_gpio_irq_endisable(pctrl, child, true);
> +	rzg2l_gpio_irq_endisable(pctrl, child, true, true);
>  	pctrl->hwirq[irq] = child;
>  	irq += pctrl->data->hwcfg->tint_start_index;
> 
> @@ -2617,7 +2628,7 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
>  		spin_lock_irqsave(&pctrl->lock, flags);
>  		ret = rzg2l_gpio_irq_set_type(data, irqd_get_trigger_type(data));
>  		if (!ret && !irqd_irq_disabled(data))
> -			rzg2l_gpio_irq_enable(data);
> +			rzg2l_gpio_irq_enable_helper(data, false);
>  		spin_unlock_irqrestore(&pctrl->lock, flags);
> 
>  		if (ret)
> @@ -2640,7 +2651,7 @@ static void rzg2l_gpio_irq_domain_free(struct irq_domain *domain, unsigned int v
> 
>  		for (i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) {
>  			if (pctrl->hwirq[i] == hwirq) {
> -				rzg2l_gpio_irq_endisable(pctrl, hwirq, false);
> +				rzg2l_gpio_irq_endisable(pctrl, hwirq, false, true);
>  				rzg2l_gpio_free(gc, hwirq);
>  				spin_lock_irqsave(&pctrl->bitmap_lock, flags);
>  				bitmap_release_region(pctrl->tint_slot, i, get_order(1));
> --
> 2.43.0

Re: [PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume

[Claudiu Beznea]

Hi, Geert,

On 9/11/25 12:53, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Mon, 8 Sept 2025 at 16:42, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in
>> gpio_irq_{en,dis}able*()") dropped the configuration of ISEL from
>> rzg2l_gpio_irq_enable()/rzg2l_gpio_irq_disable() and moved it to
>> rzg2l_gpio_child_to_parent_hwirq()/rzg2l_gpio_irq_domain_free() to fix
>> spurious IRQs.
>>
>> The resume code used rzg2l_gpio_irq_enable() (called from
>> rzg2l_gpio_irq_restore()) to reconfigure the wakeup interrupts. Some
>> drivers (e.g. Ethernet) may also reconfigure interrupts in their own code,
>> eventually calling rzg2l_gpio_irq_enable(), when these are not wakeup
>> interrupts.
>>
>> After commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL
>> in gpio_irq_{en,dis}able*()"), ISEL was no longer configured properly after
>> resume.
>>
>> Fix this by adding rzg2l_gpio_irq_endisable() back into
>> rzg2l_gpio_irq_enable(), and by using its unlocked variant in
>> rzg2l_gpio_irq_restore(). Having IRQs enable in rzg2l_gpio_irq_enable()
> 
> enabled
> 
>> should be safe with respect to spurious IRQs, as in the probe case IRQs are
>> enabled anyway in rzg2l_gpio_child_to_parent_hwirq(). No spurious IRQs
>> were detected on suspend/resume tests (executed on RZ/G3S).
>>
>> Fixes: 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in gpio_irq_{en,dis}able*(")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
> I have to admit I don't fully understand what is going on...

Sorry about that. Basically, ISEL is not properly configured as a result of
removing rzg2l_gpio_irq_endisable() from rzg2l_gpio_irq_enable() which was
previously called on interrupt reconfiguration path.

> 
>> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> @@ -2428,7 +2428,7 @@ static int rzg2l_gpio_get_gpioint(unsigned int virq, struct rzg2l_pinctrl *pctrl
>>  }
>>
>>  static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
>> -                                    unsigned int hwirq, bool enable)
>> +                                    unsigned int hwirq, bool enable, bool lock)
>>  {
>>         const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[hwirq];
>>         u64 *pin_data = pin_desc->drv_data;
>> @@ -2443,12 +2443,16 @@ static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
>>                 addr += 4;
>>         }
>>
>> -       spin_lock_irqsave(&pctrl->lock, flags);
>> +       if (lock)
>> +               spin_lock_irqsave(&pctrl->lock, flags);
>> +
>>         if (enable)
>>                 writel(readl(addr) | BIT(bit * 8), addr);
>>         else
>>                 writel(readl(addr) & ~BIT(bit * 8), addr);
>> -       spin_unlock_irqrestore(&pctrl->lock, flags);
>> +
>> +       if (lock)
>> +               spin_unlock_irqrestore(&pctrl->lock, flags);
>>  }
> 
> I am not so fond of these "if (lock) ..."-constructs, especially as
> the function now takes two bool parameters, which is error-prone.
> 
> What about renaming rzg2l_gpio_irq_endisable() to
> __rzg2l_gpio_irq_endisable(), and moving the locking to a wrapper
> rzg2l_gpio_irq_endisable()?

That was my other option but, if I remember correctly, it generated
duplicated code, thus I ended up with this.

> 
>     static void __rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
>                                          unsigned int hwirq, bool enable)
>     {
>             /* old functionality without locking */
>             ...
>     }
> 
>     static void rzg2l_gpio_irq_endisable(struct rzg2l_pinctrl *pctrl,
>                                         unsigned int hwirq, bool enable)
>     {
>             unsigned long flags;
> 
>             spin_lock_irqsave(&pctrl->lock, flags);
>             __rzg2l_gpio_irq_endisable(pctrl, hwirq, enable);
>             spin_unlock_irqrestore(&pctrl->lock, flags);
>     }
> 
> Then no existing callers of rzg2l_gpio_irq_endisable() need to be
> changed.
> 
>> @@ -2460,15 +2464,22 @@ static void rzg2l_gpio_irq_disable(struct irq_data *d)
>>         gpiochip_disable_irq(gc, hwirq);
>>  }
>>
>> -static void rzg2l_gpio_irq_enable(struct irq_data *d)
>> +static void rzg2l_gpio_irq_enable_helper(struct irq_data *d, bool lock)
> 
> Here we can't do without the "lock" parameter, unless duplicating the
> full body, so this is fine.  I'd rename it to __rzg2l_gpio_irq_enable(),
> though.
> 
>>  {
>>         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);
>>
>>         gpiochip_enable_irq(gc, hwirq);
>> +       rzg2l_gpio_irq_endisable(pctrl, hwirq, true, lock);
> 
> if (lock)
>     rzg2l_gpio_irq_endisable(pctrl, hwirq, true);
> else
>     __rzg2l_gpio_irq_endisable(pctrl, hwirq, true);
> 
>>         irq_chip_enable_parent(d);
>>  }
>>
>> +static void rzg2l_gpio_irq_enable(struct irq_data *d)
>> +{
>> +       rzg2l_gpio_irq_enable_helper(d, true);
> 
> __rzg2l_gpio_irq_enable(d, true);
> 
>> +}
>> +
>>  static int rzg2l_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>>  {
>>         return irq_chip_set_type_parent(d, type);
>> @@ -2617,7 +2628,7 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
>>                 spin_lock_irqsave(&pctrl->lock, flags);
>>                 ret = rzg2l_gpio_irq_set_type(data, irqd_get_trigger_type(data));
>>                 if (!ret && !irqd_irq_disabled(data))
>> -                       rzg2l_gpio_irq_enable(data);
>> +                       rzg2l_gpio_irq_enable_helper(data, false);
> 
> __rzg2l_gpio_irq_enable(data, false);
> 
> Before, the lock was taken again, while it was already held.
> Didn't this cause a deadlock?

The only locking issue I've seen around this code was fixed by commit
a39741d38c04 ("pinctrl: renesas: rzg2l: Use
spin_{lock,unlock}_irq{save,restore}"

I'll use the approach proposed by you in the next version.

Thank you for your review,
Claudiu

> 
>>                 spin_unlock_irqrestore(&pctrl->lock, flags);
>>
>>                 if (ret)
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume

[Claudiu Beznea]

Hi, Biju,

On 9/11/25 13:43, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@tuxon.dev>
>> Sent: 08 September 2025 15:43
>> Subject: [PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in
>> gpio_irq_{en,dis}able*()") dropped the configuration of ISEL from
>> rzg2l_gpio_irq_enable()/rzg2l_gpio_irq_disable() and moved it to
>> rzg2l_gpio_child_to_parent_hwirq()/rzg2l_gpio_irq_domain_free() to fix spurious IRQs.
>>
>> The resume code used rzg2l_gpio_irq_enable() (called from
>> rzg2l_gpio_irq_restore()) to reconfigure the wakeup interrupts. Some drivers (e.g. Ethernet) may also
>> reconfigure interrupts in their own code, eventually calling rzg2l_gpio_irq_enable(), when these are
>> not wakeup interrupts.
>>
>> After commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL in
>> gpio_irq_{en,dis}able*()"), ISEL was no longer configured properly after resume.
>>
>> Fix this by adding rzg2l_gpio_irq_endisable() back into rzg2l_gpio_irq_enable(), and by using its
>> unlocked variant in rzg2l_gpio_irq_restore(). Having IRQs enable in rzg2l_gpio_irq_enable() should be
>> safe with respect to spurious IRQs, as in the probe case IRQs are enabled anyway in
>> rzg2l_gpio_child_to_parent_hwirq(). No spurious IRQs were detected on suspend/resume tests (executed on
>> RZ/G3S).
> 
> IIRC, I believe the issue is ISEL is not restored during resume.

Yes

> Can we restore this register just like
> Schmitt register suspend/restore[1]

The IA55 would have to be configured for interrupts as well. Doing it in
other order will lead to spurious interrupts while resuming. The commit
254203f9a94c ("pinctrl: renesas: rzg2l: Add suspend/resume support") that
introduced this approach, mentions the following:

    Because interrupt signals are routed to IA55 interrupt controller and
    IA55 interrupt controller resumes before pin controller, patch restores
    also the configured interrupts just after pin settings are restored to
    avoid invalid interrupts while resuming.

Thank you,
Claudiu

> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20250911&id=837afa592c6234be82acb5d23e0a39e9befdaa85
> 
> Cheers,
> Biju
> 

RE: [PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume

[Biju Das]

HI Caludiu,

> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> Sent: 11 September 2025 14:24
> Subject: Re: [PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume
> 
> Hi, Biju,
> 
> On 9/11/25 13:43, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@tuxon.dev>
> >> Sent: 08 September 2025 15:43
> >> Subject: [PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> Commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL
> >> in
> >> gpio_irq_{en,dis}able*()") dropped the configuration of ISEL from
> >> rzg2l_gpio_irq_enable()/rzg2l_gpio_irq_disable() and moved it to
> >> rzg2l_gpio_child_to_parent_hwirq()/rzg2l_gpio_irq_domain_free() to fix spurious IRQs.
> >>
> >> The resume code used rzg2l_gpio_irq_enable() (called from
> >> rzg2l_gpio_irq_restore()) to reconfigure the wakeup interrupts. Some
> >> drivers (e.g. Ethernet) may also reconfigure interrupts in their own
> >> code, eventually calling rzg2l_gpio_irq_enable(), when these are not wakeup interrupts.
> >>
> >> After commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid
> >> configuring ISEL in gpio_irq_{en,dis}able*()"), ISEL was no longer configured properly after resume.
> >>
> >> Fix this by adding rzg2l_gpio_irq_endisable() back into
> >> rzg2l_gpio_irq_enable(), and by using its unlocked variant in
> >> rzg2l_gpio_irq_restore(). Having IRQs enable in
> >> rzg2l_gpio_irq_enable() should be safe with respect to spurious IRQs,
> >> as in the probe case IRQs are enabled anyway in rzg2l_gpio_child_to_parent_hwirq(). No spurious IRQs
> were detected on suspend/resume tests (executed on RZ/G3S).
> >
> > IIRC, I believe the issue is ISEL is not restored during resume.
> 
> Yes
> 
> > Can we restore this register just like Schmitt register
> > suspend/restore[1]
> 
> The IA55 would have to be configured for interrupts as well. Doing it in other order will lead to
> spurious interrupts while resuming. The commit 254203f9a94c ("pinctrl: renesas: rzg2l: Add
> suspend/resume support") that introduced this approach, mentions the following:
> 
>     Because interrupt signals are routed to IA55 interrupt controller and
>     IA55 interrupt controller resumes before pin controller, patch restores
>     also the configured interrupts just after pin settings are restored to
>     avoid invalid interrupts while resuming.

OK. So enable/disable Keep ISEL configuration as it is, so the pin gpio int always.
Which commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoidconfiguring ISEL in gpio_irq_{en,dis}able*()")
is doing.

The new addition is suspend/resume restores ISEL along with reconfiguring interrupts.

Is it correct?

Cheers,
Biju

Re: [PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume

[Claudiu Beznea]

On 9/11/25 16:39, Biju Das wrote:
> HI Caludiu,
> 
>> -----Original Message-----
>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>> Sent: 11 September 2025 14:24
>> Subject: Re: [PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume
>>
>> Hi, Biju,
>>
>> On 9/11/25 13:43, Biju Das wrote:
>>> Hi Claudiu,
>>>
>>>> -----Original Message-----
>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>> Sent: 08 September 2025 15:43
>>>> Subject: [PATCH] pinctrl: renesas: rzg2l: Fix ISEL restore on resume
>>>>
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> Commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid configuring ISEL
>>>> in
>>>> gpio_irq_{en,dis}able*()") dropped the configuration of ISEL from
>>>> rzg2l_gpio_irq_enable()/rzg2l_gpio_irq_disable() and moved it to
>>>> rzg2l_gpio_child_to_parent_hwirq()/rzg2l_gpio_irq_domain_free() to fix spurious IRQs.
>>>>
>>>> The resume code used rzg2l_gpio_irq_enable() (called from
>>>> rzg2l_gpio_irq_restore()) to reconfigure the wakeup interrupts. Some
>>>> drivers (e.g. Ethernet) may also reconfigure interrupts in their own
>>>> code, eventually calling rzg2l_gpio_irq_enable(), when these are not wakeup interrupts.
>>>>
>>>> After commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoid
>>>> configuring ISEL in gpio_irq_{en,dis}able*()"), ISEL was no longer configured properly after resume.
>>>>
>>>> Fix this by adding rzg2l_gpio_irq_endisable() back into
>>>> rzg2l_gpio_irq_enable(), and by using its unlocked variant in
>>>> rzg2l_gpio_irq_restore(). Having IRQs enable in
>>>> rzg2l_gpio_irq_enable() should be safe with respect to spurious IRQs,
>>>> as in the probe case IRQs are enabled anyway in rzg2l_gpio_child_to_parent_hwirq(). No spurious IRQs
>> were detected on suspend/resume tests (executed on RZ/G3S).
>>>
>>> IIRC, I believe the issue is ISEL is not restored during resume.
>>
>> Yes
>>
>>> Can we restore this register just like Schmitt register
>>> suspend/restore[1]
>>
>> The IA55 would have to be configured for interrupts as well. Doing it in other order will lead to
>> spurious interrupts while resuming. The commit 254203f9a94c ("pinctrl: renesas: rzg2l: Add
>> suspend/resume support") that introduced this approach, mentions the following:
>>
>>     Because interrupt signals are routed to IA55 interrupt controller and
>>     IA55 interrupt controller resumes before pin controller, patch restores
>>     also the configured interrupts just after pin settings are restored to
>>     avoid invalid interrupts while resuming.
> 
> OK. So enable/disable Keep ISEL configuration as it is, so the pin gpio int always.

Yes

> Which commit 1d2da79708cb ("pinctrl: renesas: rzg2l: Avoidconfiguring ISEL in gpio_irq_{en,dis}able*()")
> is doing.
> 
> The new addition is suspend/resume restores ISEL along with reconfiguring interrupts.
> 
> Is it correct?

This commit only fixes the ISEL restore on resume. The rest of interrupt
reconfiguration on resume was in place from previous commits.

Thank you,
Claudiu

> 
> Cheers,
> Biju
> 
> 
>