RE: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the interrupt type on resume

[Biju Das <biju.das.jz@xxxxxxxxxxxxxx>]

Hi Claudiu,
 
> 
> On 20.03.2024 13:08, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@xxxxxxxxx>
> >> Sent: Wednesday, March 20, 2024 10:43 AM
> >> Subject: [PATCH v3 2/2] pinctrl: renesas: rzg2l: Configure the
> >> interrupt type on resume
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >>
> >> Commit dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT
> >> source at the same time") removed the setup of TINT from
> >> rzg2l_irqc_irq_enable(). To address the spourious interrupt issue the
> >> setup of TINT has been moved in rzg2l_tint_set_edge() though
> >> rzg2l_disable_tint_and_set_tint_source(). With this, the interrupts are not properly re-configured
> after a suspend-to-RAM cycle. To address this issue and avoid spurious interrupts while resumming set
> the interrupt type before enabling it.
> >
> > Just a question,
> >
> > Previously you don't save/restore irq_type() register during suspend/resume()??
> > After STR, any way we will lose that information.
> 
> Part of IA55 registers are saved/restored in IA55 suspend/resume functions.
> 
> The rest of configuration (enable and TINT) was done though pinctrl driver because IA55 is resumed
> before pinctrl driver and if we enable the interrupt at that point the pin may be in unwanted state and
> IA55 may report invalid interrupts.
> 
> As TINT was removed from enable we need to handle it now.

OK got it, So previously you are not storing/restoring TINT source along with interrupt type()in IA55.

But depend upon the pinctrl() which just call enable() without irq_set_type().
Now the issue is fixed by calling set_type() + enable().

Cheers,
Biju


> >
> >
> >>
> >> Fixes: dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT
> >> source at the same time")
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >> ---
> >>  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 22 ++++++++++++----------
> >>  1 file changed, 12 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> >> b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> >> index 93916553bcc7..4fee3b0e6c5e 100644
> >> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> >> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> >> @@ -2045,7 +2045,9 @@ static void rzg2l_gpio_irq_restore(struct
> >> rzg2l_pinctrl *pctrl)
> >>
> >>  	for (unsigned int i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) {
> >>  		struct irq_data *data;
> >> +		unsigned long flags;
> >>  		unsigned int virq;
> >> +		int ret;
> >>
> >>  		if (!pctrl->hwirq[i])
> >>  			continue;
> >> @@ -2063,17 +2065,17 @@ static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
> >>  			continue;
> >>  		}
> >>
> >> -		if (!irqd_irq_disabled(data)) {
> >> -			unsigned long flags;
> >> -
> >> -			/*
> >> -			 * This has to be atomically executed to protect against a concurrent
> >> -			 * interrupt.
> >> -			 */
> >> -			raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
> >> +		/*
> >> +		 * This has to be atomically executed to protect against a concurrent
> >> +		 * interrupt.
> >> +		 */
> >> +		raw_spin_lock_irqsave(&pctrl->lock.rlock, flags);
> >> +		ret = rzg2l_gpio_irq_set_type(data, irqd_get_trigger_type(data));
> >> +		if (ret)
> >> +			dev_crit(pctrl->dev, "Failed to set IRQ type for virq=%u\n", virq);
> >> +		else if (!irqd_irq_disabled(data))
> >>  			rzg2l_gpio_irq_enable(data);
> >> -			raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
> >> -		}
> >> +		raw_spin_unlock_irqrestore(&pctrl->lock.rlock, flags);
> >>  	}
> >>  }
> >>
> >> --
> >> 2.39.2
> >