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));
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
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
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/?...
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
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/?...
Cheers, Biju
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
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
linux-stable-mirror@lists.linaro.org
-
Biju Das -
Claudiu -
Claudiu Beznea
-
Geert Uytterhoeven