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

[claudiu beznea]

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.

> 
> 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
>