diff mbox series

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

Message ID 20240320104230.446400-3-claudiu.beznea.uj@bp.renesas.com
State New
Headers show
Series pinctrl: renesas: rzg2l: fixes for pinctrl driver | expand

Commit Message

claudiu beznea March 20, 2024, 10:42 a.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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.

Fixes: dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT source at the same time")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

Comments

Biju Das March 20, 2024, 11:08 a.m. UTC | #1 
Hi Claudiu,

> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> 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@bp.renesas.com>
> 
> 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.

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@bp.renesas.com>
> ---
>  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
claudiu beznea March 20, 2024, 11:28 a.m. UTC | #2 
On 20.03.2024 13:08, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@tuxon.dev>
>> 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@bp.renesas.com>
>>
>> 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@bp.renesas.com>
>> ---
>>  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
>
Biju Das March 20, 2024, 11:39 a.m. UTC | #3 
Hi Claudiu,
 
> 
> On 20.03.2024 13:08, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@tuxon.dev>
> >> 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@bp.renesas.com>
> >>
> >> 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@bp.renesas.com>
> >> ---
> >>  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
> >
Geert Uytterhoeven April 18, 2024, 2:07 p.m. UTC | #4 
Hi Claudiu,

On Wed, Mar 20, 2024 at 11:43 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> 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.
>
> Fixes: dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT source at the same time")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- 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);
>         }
>  }

LGTM, but I'd rather move the dev_crit() outside (i.e. after) the
critical section.

Gr{oetje,eeting}s,

                        Geert
claudiu beznea April 19, 2024, 5:48 a.m. UTC | #5 
On 18.04.2024 17:07, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Wed, Mar 20, 2024 at 11:43 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> 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.
>>
>> Fixes: dce0919c83c3 ("irqchip/renesas-rzg2l: Do not set TIEN and TINT source at the same time")
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- 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);
>>         }
>>  }
> 
> LGTM, but I'd rather move the dev_crit() outside (i.e. after) the
> critical section.

I was in balance about having it as proposed. I'll send an update to move
it outside.

Thank you,
Claudiu Beznea

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
diff mbox series

Patch

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);
 	}
 }