[PATCH 0/5] Fix spurious TINT IRQ and enhancements

[PATCH 0/5] Fix spurious TINT IRQ and enhancements

[Biju Das]

This patch series aims to fix the spurious TINT IRQ as per the precaution
mentioned in section "8.8.3 Precaution when Changing Interrupt Settings"
of the latest RZ/G2L hardware manual. As per this we need to mask
the interrupts while setting the interrupt detection method. Apart from
this we need to clear interrupt status after setting TINT interrupt
detection method to the edge type.

Patch#1 in this series fixes HW race condition due to clearing delay
        by the cpu.
patch#2 simplifies the code and reused the same code in patch#3
patch#3 fixes spurious tint irq
patch#4 drops removing/adding tint source during disable()/enable()
patch#5 simplifies enable()/disable()

Before fix: Spurious TINT IRQ's during boot
root@smarc-rzg2l:~# cat /proc/interrupts | grep pinctrl
 67:          1          0 11030000.pinctrl 344 Edge      rtc-isl1208
 68:          0          0 11030000.pinctrl 378 Edge      SW3
 81:          1          0 11030000.pinctrl  17 Edge      1-003d
root@smarc-rzg2l:~#

After the fix:
root@smarc-rzg2l:~# cat /proc/interrupts | grep pinctrl
 67:          0          0 11030000.pinctrl 344 Edge      rtc-isl1208
 68:          0          0 11030000.pinctrl 378 Edge      SW3
 81:          0          0 11030000.pinctrl  17 Edge      1-003d
root@smarc-rzg2l:~#

This patch series is tested with [1]
[1] https://lore.kernel.org/all/20240206135115.151218-1-biju.das.jz@bp.renesas.com/

Biju Das (5):
  irqchip/renesas-rzg2l: Prevent IRQ HW race
  irqchip/renesas-rzg2l: Rename rzg2l_tint_eoi()
  irqchip/renesas-rzg2l: Fix spurious TINT IRQ
  irqchip/renesas-rzg2l: Use TIEN for enable/disable
  irqchip/renesas-rzg2l: Simplify rzg2l_irqc_irq_{en,dis}able()

 drivers/irqchip/irq-renesas-rzg2l.c | 88 ++++++++++++++++++++---------
 1 file changed, 61 insertions(+), 27 deletions(-)

-- 
2.25.1

[PATCH 1/5] irqchip/renesas-rzg2l: Prevent IRQ HW race

[Biju Das]

As per section "8.8.2 Clear Timing of Interrupt Cause" of the RZ/G2L
hardware manual (Rev.1.45 Jan, 2024), it is mentioned that we need to
clear the interrupt cause flag in the isr. It takes some time for the
cpu to clear the interrupt cause flag. Therefore, to prevent another
occurrence of interrupt due to this delay, the interrupt cause flag
is read after clearing.

Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/irqchip/irq-renesas-rzg2l.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 9494fc26259c..46f9b07e0e8a 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -111,8 +111,11 @@ static void rzg2l_tint_eoi(struct irq_data *d)
 	u32 reg;
 
 	reg = readl_relaxed(priv->base + TSCR);
-	if (reg & bit)
+	if (reg & bit) {
 		writel_relaxed(reg & ~bit, priv->base + TSCR);
+		/* Read to avoid irq generation due to irq clearing delay */
+		readl_relaxed(priv->base + TSCR);
+	}
 }
 
 static void rzg2l_irqc_eoi(struct irq_data *d)
-- 
2.25.1

Re: [PATCH 1/5] irqchip/renesas-rzg2l: Prevent IRQ HW race

[Thomas Gleixner]

On Mon, Feb 12 2024 at 11:37, Biju Das wrote:
> As per section "8.8.2 Clear Timing of Interrupt Cause" of the RZ/G2L
> hardware manual (Rev.1.45 Jan, 2024), it is mentioned that we need to
> clear the interrupt cause flag in the isr.

We need to clear? Please write changelogs in neutral tone. Also use
proper words instead of acronyms, this is not twatter.

I'm also failing to see the value of above sentence other that it
occupies space. The code already does that, no?

> It takes some time for the cpu to clear the interrupt cause
> flag. Therefore, to prevent another occurrence of interrupt due to
> this delay, the interrupt cause flag is read after clearing.

You really want to explain explicitely what the problem is. The above is
a novel 

Something like this:

  The irq_eoi() callback of the RX/G2L interrupt chip clears the
  relevant interrupt cause bit in the TSCR register.

  This write is not sufficient because the write is posted and therefore
  not guaranteed to immediately clear the bit. Due to that delay the CPU
  can raise the just handled interrupt again.

  Prevent this by reading the register back which causes the posted
  write to be flushed to the hardware before the read completes.

This uses the proper technical term 'posted write' which is well defined
and exactly the cause of the problem, no?

> Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/irqchip/irq-renesas-rzg2l.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
> index 9494fc26259c..46f9b07e0e8a 100644
> --- a/drivers/irqchip/irq-renesas-rzg2l.c
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> @@ -111,8 +111,11 @@ static void rzg2l_tint_eoi(struct irq_data *d)
>  	u32 reg;
>  
>  	reg = readl_relaxed(priv->base + TSCR);
> -	if (reg & bit)
> +	if (reg & bit) {
>  		writel_relaxed(reg & ~bit, priv->base + TSCR);
> +		/* Read to avoid irq generation due to irq clearing delay */

		/*
                 * Enforce that the posted write is flushed to prevent
                 * that the just handled interrupt is raised again.
                 */

Hmm?

> +		readl_relaxed(priv->base + TSCR);

Thanks,

        tglx

RE: [PATCH 1/5] irqchip/renesas-rzg2l: Prevent IRQ HW race

[Biju Das]

Hi Thomas Gleixner,

Thanks for the feedback.

> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, March 1, 2024 2:39 PM
> Subject: Re: [PATCH 1/5] irqchip/renesas-rzg2l: Prevent IRQ HW race
> 
> On Mon, Feb 12 2024 at 11:37, Biju Das wrote:
> > As per section "8.8.2 Clear Timing of Interrupt Cause" of the RZ/G2L
> > hardware manual (Rev.1.45 Jan, 2024), it is mentioned that we need to
> > clear the interrupt cause flag in the isr.
> 
> We need to clear? Please write changelogs in neutral tone. Also use proper words instead of acronyms,
> this is not twatter.
OK.

> 
> I'm also failing to see the value of above sentence other that it occupies space. The code already
> does that, no?

Ya, that sentence is not required.

> 
> > It takes some time for the cpu to clear the interrupt cause flag.
> > Therefore, to prevent another occurrence of interrupt due to this
> > delay, the interrupt cause flag is read after clearing.
> 
> You really want to explain explicitely what the problem is. The above is a novel
> 
> Something like this:
> 
>   The irq_eoi() callback of the RX/G2L interrupt chip clears the
>   relevant interrupt cause bit in the TSCR register.
> 
>   This write is not sufficient because the write is posted and therefore
>   not guaranteed to immediately clear the bit. Due to that delay the CPU
>   can raise the just handled interrupt again.
> 
>   Prevent this by reading the register back which causes the posted
>   write to be flushed to the hardware before the read completes.
> 
> This uses the proper technical term 'posted write' which is well defined and exactly the cause of the
> problem, no?

You are correct. The 'Posted write' is new technological term for me.


> 
> > Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller
> > driver")
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/irqchip/irq-renesas-rzg2l.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c
> > b/drivers/irqchip/irq-renesas-rzg2l.c
> > index 9494fc26259c..46f9b07e0e8a 100644
> > --- a/drivers/irqchip/irq-renesas-rzg2l.c
> > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > @@ -111,8 +111,11 @@ static void rzg2l_tint_eoi(struct irq_data *d)
> >  	u32 reg;
> >
> >  	reg = readl_relaxed(priv->base + TSCR);
> > -	if (reg & bit)
> > +	if (reg & bit) {
> >  		writel_relaxed(reg & ~bit, priv->base + TSCR);
> > +		/* Read to avoid irq generation due to irq clearing delay */
> 
> 		/*
>                  * Enforce that the posted write is flushed to prevent
>                  * that the just handled interrupt is raised again.
>                  */
> 
> Hmm?

Agreed, Will update accordingly.

Cheers,
Biju

> 
> > +		readl_relaxed(priv->base + TSCR);

[PATCH 2/5] irqchip/renesas-rzg2l: Rename rzg2l_tint_eoi()

[Biju Das]

Rename rzg2l_tint_eoi->rzg2l_clear_tint_int and simplify the code by
removing redundant priv and hw_irq local variables.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/irqchip/irq-renesas-rzg2l.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 46f9b07e0e8a..74c8cbb790e9 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -103,11 +103,10 @@ static void rzg2l_irq_eoi(struct irq_data *d)
 		writel_relaxed(iscr & ~bit, priv->base + ISCR);
 }
 
-static void rzg2l_tint_eoi(struct irq_data *d)
+static void rzg2l_clear_tint_int(struct rzg2l_irqc_priv *priv,
+				 unsigned int hwirq)
 {
-	unsigned int hw_irq = irqd_to_hwirq(d) - IRQC_TINT_START;
-	struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
-	u32 bit = BIT(hw_irq);
+	u32 bit = BIT(hwirq - IRQC_TINT_START);
 	u32 reg;
 
 	reg = readl_relaxed(priv->base + TSCR);
@@ -127,7 +126,7 @@ static void rzg2l_irqc_eoi(struct irq_data *d)
 	if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT)
 		rzg2l_irq_eoi(d);
 	else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ)
-		rzg2l_tint_eoi(d);
+		rzg2l_clear_tint_int(priv, hw_irq);
 	raw_spin_unlock(&priv->lock);
 	irq_chip_eoi_parent(d);
 }
-- 
2.25.1

Re: [PATCH 2/5] irqchip/renesas-rzg2l: Rename rzg2l_tint_eoi()

[Geert Uytterhoeven]

Hi Biju,

On Mon, Feb 12, 2024 at 12:37 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Rename rzg2l_tint_eoi->rzg2l_clear_tint_int and simplify the code by
> removing redundant priv and hw_irq local variables.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/irqchip/irq-renesas-rzg2l.c
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> @@ -103,11 +103,10 @@ static void rzg2l_irq_eoi(struct irq_data *d)
>                 writel_relaxed(iscr & ~bit, priv->base + ISCR);
>  }
>
> -static void rzg2l_tint_eoi(struct irq_data *d)
> +static void rzg2l_clear_tint_int(struct rzg2l_irqc_priv *priv,
> +                                unsigned int hwirq)
>  {
> -       unsigned int hw_irq = irqd_to_hwirq(d) - IRQC_TINT_START;
> -       struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> -       u32 bit = BIT(hw_irq);
> +       u32 bit = BIT(hwirq - IRQC_TINT_START);
>         u32 reg;
>
>         reg = readl_relaxed(priv->base + TSCR);
> @@ -127,7 +126,7 @@ static void rzg2l_irqc_eoi(struct irq_data *d)
>         if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT)
>                 rzg2l_irq_eoi(d);
>         else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ)
> -               rzg2l_tint_eoi(d);
> +               rzg2l_clear_tint_int(priv, hw_irq);

Perhaps pass the tint number (i.e. "hw_irq - IRQC_TINT_START") instead?

>         raw_spin_unlock(&priv->lock);
>         irq_chip_eoi_parent(d);

I think it makes sense to make a similar change to rzg2l_irq_eoi().

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 2/5] irqchip/renesas-rzg2l: Rename rzg2l_tint_eoi()

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Monday, February 12, 2024 4:38 PM
> Subject: Re: [PATCH 2/5] irqchip/renesas-rzg2l: Rename rzg2l_tint_eoi()
> 
> Hi Biju,
> 
> On Mon, Feb 12, 2024 at 12:37 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Rename rzg2l_tint_eoi->rzg2l_clear_tint_int and simplify the code by
> > removing redundant priv and hw_irq local variables.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> > --- a/drivers/irqchip/irq-renesas-rzg2l.c
> > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > @@ -103,11 +103,10 @@ static void rzg2l_irq_eoi(struct irq_data *d)
> >                 writel_relaxed(iscr & ~bit, priv->base + ISCR);  }
> >
> > -static void rzg2l_tint_eoi(struct irq_data *d)
> > +static void rzg2l_clear_tint_int(struct rzg2l_irqc_priv *priv,
> > +                                unsigned int hwirq)
> >  {
> > -       unsigned int hw_irq = irqd_to_hwirq(d) - IRQC_TINT_START;
> > -       struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> > -       u32 bit = BIT(hw_irq);
> > +       u32 bit = BIT(hwirq - IRQC_TINT_START);
> >         u32 reg;
> >
> >         reg = readl_relaxed(priv->base + TSCR); @@ -127,7 +126,7 @@
> > static void rzg2l_irqc_eoi(struct irq_data *d)
> >         if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT)
> >                 rzg2l_irq_eoi(d);
> >         else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ)
> > -               rzg2l_tint_eoi(d);
> > +               rzg2l_clear_tint_int(priv, hw_irq);
> 
> Perhaps pass the tint number (i.e. "hw_irq - IRQC_TINT_START") instead?

Agreed.

> 
> >         raw_spin_unlock(&priv->lock);
> >         irq_chip_eoi_parent(d);
> 
> I think it makes sense to make a similar change to rzg2l_irq_eoi().

OK will do similar change.

Cheers,
Biju

Re: [PATCH 2/5] irqchip/renesas-rzg2l: Rename rzg2l_tint_eoi()

[Thomas Gleixner]

On Mon, Feb 12 2024 at 17:38, Geert Uytterhoeven wrote:
>> -static void rzg2l_tint_eoi(struct irq_data *d)
>> +static void rzg2l_clear_tint_int(struct rzg2l_irqc_priv *priv,
>> +                                unsigned int hwirq)
>>  {
>> -       unsigned int hw_irq = irqd_to_hwirq(d) - IRQC_TINT_START;
>> -       struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
>> -       u32 bit = BIT(hw_irq);
>> +       u32 bit = BIT(hwirq - IRQC_TINT_START);
>>         u32 reg;
>>
>>         reg = readl_relaxed(priv->base + TSCR);
>> @@ -127,7 +126,7 @@ static void rzg2l_irqc_eoi(struct irq_data *d)
>>         if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT)
>>                 rzg2l_irq_eoi(d);
>>         else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ)
>> -               rzg2l_tint_eoi(d);
>> +               rzg2l_clear_tint_int(priv, hw_irq);
>
> Perhaps pass the tint number (i.e. "hw_irq - IRQC_TINT_START")
> instead?

No. You have to do that on all call sites then. There is another coming
in the next patch AFAICT.

Thanks,

        tglx

[PATCH 3/5] irqchip/renesas-rzg2l: Fix spurious TINT IRQ

[Biju Das]

As per RZ/G2L hardware manual Rev.1.45 section 8.8.3 Precaution when
changing interrupt settings, we need to mask the interrupts while
setting the interrupt detection method. Apart from this we need to clear
interrupt status after setting TINT interrupt detection method to the
edge type.

First disable TINT enable and then set TINT source. After that set edge
type and then clear the interrupt status register.

Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/irqchip/irq-renesas-rzg2l.c | 46 ++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 74c8cbb790e9..c48c8e836dd1 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -117,6 +117,40 @@ static void rzg2l_clear_tint_int(struct rzg2l_irqc_priv *priv,
 	}
 }
 
+static void rzg2l_tint_endisable(struct rzg2l_irqc_priv *priv, u32 reg,
+				 u32 tssr_offset, u32 tssr_index,
+				 bool enable)
+{
+	if (enable)
+		reg |= TIEN << TSSEL_SHIFT(tssr_offset);
+	else
+		reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset));
+
+	writel_relaxed(reg, priv->base + TSSR(tssr_index));
+}
+
+static void rzg2l_disable_tint_and_set_tint_source(struct irq_data *d,
+						   struct rzg2l_irqc_priv *priv,
+						   u32 reg, u32 tssr_offset,
+						   u8 tssr_index)
+{
+	unsigned long tint = (uintptr_t)irq_data_get_irq_chip_data(d);
+	u32 val;
+
+	rzg2l_tint_endisable(priv, reg, tssr_offset, tssr_index, false);
+	val = (reg >> TSSEL_SHIFT(tssr_offset)) & ~TIEN;
+	if (tint != val) {
+		reg |= tint << TSSEL_SHIFT(tssr_offset);
+		writel_relaxed(reg, priv->base + TSSR(tssr_index));
+	}
+}
+
+static bool rzg2l_is_tint_enabled(struct rzg2l_irqc_priv *priv, u32 reg,
+				  u32 tssr_offset)
+{
+	return !!(reg & (TIEN << TSSEL_SHIFT(tssr_offset)));
+}
+
 static void rzg2l_irqc_eoi(struct irq_data *d)
 {
 	struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
@@ -214,8 +248,11 @@ static int rzg2l_tint_set_edge(struct irq_data *d, unsigned int type)
 	struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
 	unsigned int hwirq = irqd_to_hwirq(d);
 	u32 titseln = hwirq - IRQC_TINT_START;
+	u32 tssr_offset = TSSR_OFFSET(titseln);
+	u8 tssr_index = TSSR_INDEX(titseln);
+	bool tint_enable;
 	u8 index, sense;
-	u32 reg;
+	u32 reg, tssr;
 
 	switch (type & IRQ_TYPE_SENSE_MASK) {
 	case IRQ_TYPE_EDGE_RISING:
@@ -237,10 +274,17 @@ static int rzg2l_tint_set_edge(struct irq_data *d, unsigned int type)
 	}
 
 	raw_spin_lock(&priv->lock);
+	tssr = readl_relaxed(priv->base + TSSR(tssr_index));
+	tint_enable = rzg2l_is_tint_enabled(priv, tssr, tssr_offset);
+	rzg2l_disable_tint_and_set_tint_source(d, priv, tssr,
+					       tssr_offset, tssr_index);
 	reg = readl_relaxed(priv->base + TITSR(index));
 	reg &= ~(IRQ_MASK << (titseln * TITSEL_WIDTH));
 	reg |= sense << (titseln * TITSEL_WIDTH);
 	writel_relaxed(reg, priv->base + TITSR(index));
+	rzg2l_clear_tint_int(priv, hwirq);
+	if (tint_enable)
+		rzg2l_tint_endisable(priv, tssr, tssr_offset, tssr_index, true);
 	raw_spin_unlock(&priv->lock);
 
 	return 0;
-- 
2.25.1

Re: [PATCH 3/5] irqchip/renesas-rzg2l: Fix spurious TINT IRQ

[Thomas Gleixner]

On Mon, Feb 12 2024 at 11:37, Biju Das wrote:
> As per RZ/G2L hardware manual Rev.1.45 section 8.8.3 Precaution when
> changing interrupt settings, we need to mask the interrupts while
> setting the interrupt detection method. Apart from this we need to clear
> interrupt status after setting TINT interrupt detection method to the
> edge type.

No 'we|us|...' please. See:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

> First disable TINT enable and then set TINT source. After that set edge
> type and then clear the interrupt status register.

Again the above novel about the manual really does not explain what the
actual problem is. I can crystal ball it out from what the patch does,
but that really wants to be written out here.


> Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/irqchip/irq-renesas-rzg2l.c | 46 ++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
> index 74c8cbb790e9..c48c8e836dd1 100644
> --- a/drivers/irqchip/irq-renesas-rzg2l.c
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> @@ -117,6 +117,40 @@ static void rzg2l_clear_tint_int(struct rzg2l_irqc_priv *priv,
>  	}
>  }
>  
> +static void rzg2l_tint_endisable(struct rzg2l_irqc_priv *priv, u32 reg,
> +				 u32 tssr_offset, u32 tssr_index,
> +				 bool enable)

The 80 character limit does not exist anymore. It's 100 today. Please
get rid of the extra line breaks when adding new code.

> +{
> +	if (enable)
> +		reg |= TIEN << TSSEL_SHIFT(tssr_offset);
> +	else
> +		reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset));
> +
> +	writel_relaxed(reg, priv->base + TSSR(tssr_index));
> +}
> +
> +static void rzg2l_disable_tint_and_set_tint_source(struct irq_data *d,
> +						   struct rzg2l_irqc_priv *priv,
> +						   u32 reg, u32 tssr_offset,
> +						   u8 tssr_index)
> +{
> +	unsigned long tint = (uintptr_t)irq_data_get_irq_chip_data(d);
> +	u32 val;
> +
> +	rzg2l_tint_endisable(priv, reg, tssr_offset, tssr_index, false);
> +	val = (reg >> TSSEL_SHIFT(tssr_offset)) & ~TIEN;

So this shifts the byte which contains the control bit for the interrupt
down to bit 0-7 and clears the TIEN bit (7).

> +	if (tint != val) {

And now it compares it to the TINT value which was associated when the
interrupt was allocated. How is this correct?

Depending on tssr_offset reg is shifted right by tssr_offset * 8. Right?

So the comparison is only valid when tssr_offset == 3 because otherwise
bit 8-31 contain uncleared values, no?

> +		reg |= tint << TSSEL_SHIFT(tssr_offset);
> +		writel_relaxed(reg, priv->base + TSSR(tssr_index));

So this writes again to the same register as the unconditional write via
rzg2l_tint_endisable(). What is all this conditional voodoo for? 

	u32 tint = (u32)(uintptr_t)irq_data_get_irq_chip_data(d);

        /* Clear the relevant byte in reg */
        reg &= ~(TSSEL_MASK << TSSEL_SHIFT(tssr_offset));
        /* Set TINT and leave TIEN clear */
        reg |= tint << TSSEL_SHIFT(tssr_offset);
	writel_relaxed(reg, priv->base + TSSR(tssr_index));
        
Does exactly the correct thing without any conditional in a
comprehensible way, no?

> +	}
> +}
> +
> +static bool rzg2l_is_tint_enabled(struct rzg2l_irqc_priv *priv, u32 reg,
> +				  u32 tssr_offset)
> +{

The 'priv' argument is unused.

> +	return !!(reg & (TIEN << TSSEL_SHIFT(tssr_offset)));
> +}


> +
>  static void rzg2l_irqc_eoi(struct irq_data *d)
>  {
>  	struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> @@ -214,8 +248,11 @@ static int rzg2l_tint_set_edge(struct irq_data *d, unsigned int type)
>  	struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
>  	unsigned int hwirq = irqd_to_hwirq(d);
>  	u32 titseln = hwirq - IRQC_TINT_START;
> +	u32 tssr_offset = TSSR_OFFSET(titseln);
> +	u8 tssr_index = TSSR_INDEX(titseln);
> +	bool tint_enable;
>  	u8 index, sense;
> -	u32 reg;
> +	u32 reg, tssr;
>  
>  	switch (type & IRQ_TYPE_SENSE_MASK) {
>  	case IRQ_TYPE_EDGE_RISING:
> @@ -237,10 +274,17 @@ static int rzg2l_tint_set_edge(struct irq_data *d, unsigned int type)
>  	}
>  
>  	raw_spin_lock(&priv->lock);
> +	tssr = readl_relaxed(priv->base + TSSR(tssr_index));
> +	tint_enable = rzg2l_is_tint_enabled(priv, tssr, tssr_offset);
> +	rzg2l_disable_tint_and_set_tint_source(d, priv, tssr,
> +					       tssr_offset, tssr_index);
>  	reg = readl_relaxed(priv->base + TITSR(index));
>  	reg &= ~(IRQ_MASK << (titseln * TITSEL_WIDTH));
>  	reg |= sense << (titseln * TITSEL_WIDTH);
>  	writel_relaxed(reg, priv->base + TITSR(index));
> +	rzg2l_clear_tint_int(priv, hwirq);
> +	if (tint_enable)
> +		rzg2l_tint_endisable(priv, tssr, tssr_offset, tssr_index, true);
>  	raw_spin_unlock(&priv->lock);

This whole tint_enable logic is overly complex.

  	raw_spin_lock(&priv->lock);
	tssr = readl_relaxed(priv->base + TSSR(tssr_index));
	tssr = rzg2l_disable_tint_and_set_tint_source(d, priv, tssr, tssr_offset, tssr_index);
  	reg = readl_relaxed(priv->base + TITSR(index));
  	reg &= ~(IRQ_MASK << (titseln * TITSEL_WIDTH));
  	reg |= sense << (titseln * TITSEL_WIDTH);
  	writel_relaxed(reg, priv->base + TITSR(index));
	rzg2l_clear_tint_int(priv, hwirq);
	writel_relaxed(tssr, priv->base + TSSR(tssr_index));
 	raw_spin_unlock(&priv->lock);

All it needs is to let rzg2l_disable_tint_and_set_tint_source() return
the proper value for writing back.

	u32 tint = (u32)(uintptr_t)irq_data_get_irq_chip_data(d);
        u32 tien = reg & (TIEN << TSSEL_SHIFT(tssr_offset));

        /* Clear the relevant byte in reg */
        reg &= ~(TSSEL_MASK << TSSEL_SHIFT(tssr_offset));
        /* Set TINT and leave TIEN clear */
        reg |= tint << TSSEL_SHIFT(tssr_offset);
	writel_relaxed(reg, priv->base + TSSR(tssr_index));
        return reg | tien;

The extra unconditional TSSR write at the end of rzg2l_tint_set_edge()
is really not worth to optimize for.

Keep it simple and comprehensible. That's not a hotpath.

Thanks,

        tglx

RE: [PATCH 3/5] irqchip/renesas-rzg2l: Fix spurious TINT IRQ

[Biju Das]

Hi Thomas,

Thanks for the feedback.

> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, March 1, 2024 3:36 PM
> Subject: Re: [PATCH 3/5] irqchip/renesas-rzg2l: Fix spurious TINT IRQ
> 
> On Mon, Feb 12 2024 at 11:37, Biju Das wrote:
> > As per RZ/G2L hardware manual Rev.1.45 section 8.8.3 Precaution when
> > changing interrupt settings, we need to mask the interrupts while
> > setting the interrupt detection method. Apart from this we need to
> > clear interrupt status after setting TINT interrupt detection method
> > to the edge type.
> 
> No 'we|us|...' please. See:
> 
OK.

> > First disable TINT enable and then set TINT source. After that set
> > edge type and then clear the interrupt status register.
> 
> Again the above novel about the manual really does not explain what the actual problem is. I can
> crystal ball it out from what the patch does, but that really wants to be written out here.

Ok, will update and send.

> 
> 
> > Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller
> > driver")
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/irqchip/irq-renesas-rzg2l.c | 46
> > ++++++++++++++++++++++++++++-
> >  1 file changed, 45 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c
> > b/drivers/irqchip/irq-renesas-rzg2l.c
> > index 74c8cbb790e9..c48c8e836dd1 100644
> > --- a/drivers/irqchip/irq-renesas-rzg2l.c
> > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > @@ -117,6 +117,40 @@ static void rzg2l_clear_tint_int(struct rzg2l_irqc_priv *priv,
> >  	}
> >  }
> >
> > +static void rzg2l_tint_endisable(struct rzg2l_irqc_priv *priv, u32 reg,
> > +				 u32 tssr_offset, u32 tssr_index,
> > +				 bool enable)
> 
> The 80 character limit does not exist anymore. It's 100 today. Please get rid of the extra line breaks
> when adding new code.

Agreed.

> 
> > +{
> > +	if (enable)
> > +		reg |= TIEN << TSSEL_SHIFT(tssr_offset);
> > +	else
> > +		reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset));
> > +
> > +	writel_relaxed(reg, priv->base + TSSR(tssr_index)); }
> > +
> > +static void rzg2l_disable_tint_and_set_tint_source(struct irq_data *d,
> > +						   struct rzg2l_irqc_priv *priv,
> > +						   u32 reg, u32 tssr_offset,
> > +						   u8 tssr_index)
> > +{
> > +	unsigned long tint = (uintptr_t)irq_data_get_irq_chip_data(d);
> > +	u32 val;
> > +
> > +	rzg2l_tint_endisable(priv, reg, tssr_offset, tssr_index, false);
> > +	val = (reg >> TSSEL_SHIFT(tssr_offset)) & ~TIEN;
> 
> So this shifts the byte which contains the control bit for the interrupt down to bit 0-7 and clears the
> TIEN bit (7).
> 
> > +	if (tint != val) {
> 
> And now it compares it to the TINT value which was associated when the interrupt was allocated. How is
> this correct?

val is without tien, so basically it is comparing tint source. This is correct, but I agree it is hard
to understand.

> 
> Depending on tssr_offset reg is shifted right by tssr_offset * 8. Right?
Yes.

Offset = 0, shifted by 0
Offset = 1, shifted by 8

> 
> So the comparison is only valid when tssr_offset == 3 because otherwise bit 8-31 contain uncleared
> values, no?

Oops, you are correct.

> 
> > +		reg |= tint << TSSEL_SHIFT(tssr_offset);
> > +		writel_relaxed(reg, priv->base + TSSR(tssr_index));
> 
> So this writes again to the same register as the unconditional write via rzg2l_tint_endisable(). What
> is all this conditional voodoo for?
> 
> 	u32 tint = (u32)(uintptr_t)irq_data_get_irq_chip_data(d);
> 
>         /* Clear the relevant byte in reg */
>         reg &= ~(TSSEL_MASK << TSSEL_SHIFT(tssr_offset));
>         /* Set TINT and leave TIEN clear */
>         reg |= tint << TSSEL_SHIFT(tssr_offset);
> 	writel_relaxed(reg, priv->base + TSSR(tssr_index));
> 
> Does exactly the correct thing without any conditional in a comprehensible way, no?

Yes it looks complex.

> 
> > +	}
> > +}
> > +
> > +static bool rzg2l_is_tint_enabled(struct rzg2l_irqc_priv *priv, u32 reg,
> > +				  u32 tssr_offset)
> > +{
> 
> The 'priv' argument is unused.
> 
> > +	return !!(reg & (TIEN << TSSEL_SHIFT(tssr_offset))); }
> 
> 
> > +
> >  static void rzg2l_irqc_eoi(struct irq_data *d)  {
> >  	struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); @@ -214,8
> > +248,11 @@ static int rzg2l_tint_set_edge(struct irq_data *d, unsigned int type)
> >  	struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> >  	unsigned int hwirq = irqd_to_hwirq(d);
> >  	u32 titseln = hwirq - IRQC_TINT_START;
> > +	u32 tssr_offset = TSSR_OFFSET(titseln);
> > +	u8 tssr_index = TSSR_INDEX(titseln);
> > +	bool tint_enable;
> >  	u8 index, sense;
> > -	u32 reg;
> > +	u32 reg, tssr;
> >
> >  	switch (type & IRQ_TYPE_SENSE_MASK) {
> >  	case IRQ_TYPE_EDGE_RISING:
> > @@ -237,10 +274,17 @@ static int rzg2l_tint_set_edge(struct irq_data *d, unsigned int type)
> >  	}
> >
> >  	raw_spin_lock(&priv->lock);
> > +	tssr = readl_relaxed(priv->base + TSSR(tssr_index));
> > +	tint_enable = rzg2l_is_tint_enabled(priv, tssr, tssr_offset);
> > +	rzg2l_disable_tint_and_set_tint_source(d, priv, tssr,
> > +					       tssr_offset, tssr_index);
> >  	reg = readl_relaxed(priv->base + TITSR(index));
> >  	reg &= ~(IRQ_MASK << (titseln * TITSEL_WIDTH));
> >  	reg |= sense << (titseln * TITSEL_WIDTH);
> >  	writel_relaxed(reg, priv->base + TITSR(index));
> > +	rzg2l_clear_tint_int(priv, hwirq);
> > +	if (tint_enable)
> > +		rzg2l_tint_endisable(priv, tssr, tssr_offset, tssr_index, true);
> >  	raw_spin_unlock(&priv->lock);
> 

> This whole tint_enable logic is overly complex.

I agree, it is complex.

> 
>   	raw_spin_lock(&priv->lock);
> 	tssr = readl_relaxed(priv->base + TSSR(tssr_index));
> 	tssr = rzg2l_disable_tint_and_set_tint_source(d, priv, tssr, tssr_offset, tssr_index);
>   	reg = readl_relaxed(priv->base + TITSR(index));
>   	reg &= ~(IRQ_MASK << (titseln * TITSEL_WIDTH));
>   	reg |= sense << (titseln * TITSEL_WIDTH);
>   	writel_relaxed(reg, priv->base + TITSR(index));
> 	rzg2l_clear_tint_int(priv, hwirq);
> 	writel_relaxed(tssr, priv->base + TSSR(tssr_index));
>  	raw_spin_unlock(&priv->lock);
> 
> All it needs is to let rzg2l_disable_tint_and_set_tint_source() return the proper value for writing
> back.
> 
> 	u32 tint = (u32)(uintptr_t)irq_data_get_irq_chip_data(d);
>         u32 tien = reg & (TIEN << TSSEL_SHIFT(tssr_offset));
> 
>         /* Clear the relevant byte in reg */
>         reg &= ~(TSSEL_MASK << TSSEL_SHIFT(tssr_offset));
>         /* Set TINT and leave TIEN clear */
>         reg |= tint << TSSEL_SHIFT(tssr_offset);
> 	writel_relaxed(reg, priv->base + TSSR(tssr_index));
>         return reg | tien;
> 
> The extra unconditional TSSR write at the end of rzg2l_tint_set_edge() is really not worth to optimize
> for.

Your logic is simple and correct. Will use it in next version.

Cheers,
Biju

[PATCH 4/5] irqchip/renesas-rzg2l: Use TIEN for enable/disable

[Biju Das]

Use TIEN for enable/disable and avoid modifying TINT source selection
register.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/irqchip/irq-renesas-rzg2l.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index c48c8e836dd1..fbee400985a9 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -35,7 +35,6 @@
 #define TSSR(n)				(0x30 + ((n) * 4))
 #define TIEN				BIT(7)
 #define TSSEL_SHIFT(n)			(8 * (n))
-#define TSSEL_MASK			GENMASK(7, 0)
 #define IRQ_MASK			0x3
 
 #define TSSR_OFFSET(n)			((n) % 4)
@@ -178,8 +177,7 @@ static void rzg2l_irqc_irq_disable(struct irq_data *d)
 
 		raw_spin_lock(&priv->lock);
 		reg = readl_relaxed(priv->base + TSSR(tssr_index));
-		reg &= ~(TSSEL_MASK << TSSEL_SHIFT(tssr_offset));
-		writel_relaxed(reg, priv->base + TSSR(tssr_index));
+		rzg2l_tint_endisable(priv, reg, tssr_offset, tssr_index, false);
 		raw_spin_unlock(&priv->lock);
 	}
 	irq_chip_disable_parent(d);
@@ -190,7 +188,6 @@ static void rzg2l_irqc_irq_enable(struct irq_data *d)
 	unsigned int hw_irq = irqd_to_hwirq(d);
 
 	if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) {
-		unsigned long tint = (uintptr_t)irq_data_get_irq_chip_data(d);
 		struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
 		u32 offset = hw_irq - IRQC_TINT_START;
 		u32 tssr_offset = TSSR_OFFSET(offset);
@@ -199,8 +196,7 @@ static void rzg2l_irqc_irq_enable(struct irq_data *d)
 
 		raw_spin_lock(&priv->lock);
 		reg = readl_relaxed(priv->base + TSSR(tssr_index));
-		reg |= (TIEN | tint) << TSSEL_SHIFT(tssr_offset);
-		writel_relaxed(reg, priv->base + TSSR(tssr_index));
+		rzg2l_tint_endisable(priv, reg, tssr_offset, tssr_index, true);
 		raw_spin_unlock(&priv->lock);
 	}
 	irq_chip_enable_parent(d);
-- 
2.25.1

Re: [PATCH 4/5] irqchip/renesas-rzg2l: Use TIEN for enable/disable

[Thomas Gleixner]

On Mon, Feb 12 2024 at 11:37, Biju Das wrote:
> Use TIEN for enable/disable and avoid modifying TINT source selection
> register.

Why?

Changelogs are supposed to explain the WHY and not just decribe the
WHAT.

Thanks,

        tglx

RE: [PATCH 4/5] irqchip/renesas-rzg2l: Use TIEN for enable/disable

[Biju Das]

Hi Thomas Gleixner,

> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, March 1, 2024 2:16 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Biju Das <biju.das.jz@bp.renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@bp.renesas.com>; Marc Zyngier <maz@kernel.org>; Geert Uytterhoeven <geert+renesas@glider.be>;
> biju.das.au <biju.das.au@gmail.com>; linux-renesas-soc@vger.kernel.org
> Subject: Re: [PATCH 4/5] irqchip/renesas-rzg2l: Use TIEN for enable/disable
> 
> On Mon, Feb 12 2024 at 11:37, Biju Das wrote:
> > Use TIEN for enable/disable and avoid modifying TINT source selection
> > register.
> 
> Why?

This will lead to conflict in TINT detection register and TINT source
as we are modifying the source. This can also lead to spurious IRQ.

> 
> Changelogs are supposed to explain the WHY and not just decribe the WHAT.

OK, will do it in the next version.

Cheers,
Biju

[PATCH 5/5] irqchip/renesas-rzg2l: Simplify rzg2l_irqc_irq_{en,dis}able()

[Biju Das]

Simplify rzg2l_irqc_irq_{en,dis}able() by moving common code to
rzg2l_tint_irq_endisable().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/irqchip/irq-renesas-rzg2l.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index fbee400985a9..bfac2c0ecf01 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -164,7 +164,7 @@ static void rzg2l_irqc_eoi(struct irq_data *d)
 	irq_chip_eoi_parent(d);
 }
 
-static void rzg2l_irqc_irq_disable(struct irq_data *d)
+static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable)
 {
 	unsigned int hw_irq = irqd_to_hwirq(d);
 
@@ -177,28 +177,20 @@ static void rzg2l_irqc_irq_disable(struct irq_data *d)
 
 		raw_spin_lock(&priv->lock);
 		reg = readl_relaxed(priv->base + TSSR(tssr_index));
-		rzg2l_tint_endisable(priv, reg, tssr_offset, tssr_index, false);
+		rzg2l_tint_endisable(priv, reg, tssr_offset, tssr_index, enable);
 		raw_spin_unlock(&priv->lock);
 	}
+}
+
+static void rzg2l_irqc_irq_disable(struct irq_data *d)
+{
+	rzg2l_tint_irq_endisable(d, false);
 	irq_chip_disable_parent(d);
 }
 
 static void rzg2l_irqc_irq_enable(struct irq_data *d)
 {
-	unsigned int hw_irq = irqd_to_hwirq(d);
-
-	if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) {
-		struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
-		u32 offset = hw_irq - IRQC_TINT_START;
-		u32 tssr_offset = TSSR_OFFSET(offset);
-		u8 tssr_index = TSSR_INDEX(offset);
-		u32 reg;
-
-		raw_spin_lock(&priv->lock);
-		reg = readl_relaxed(priv->base + TSSR(tssr_index));
-		rzg2l_tint_endisable(priv, reg, tssr_offset, tssr_index, true);
-		raw_spin_unlock(&priv->lock);
-	}
+	rzg2l_tint_irq_endisable(d, true);
 	irq_chip_enable_parent(d);
 }
 
-- 
2.25.1

RE: [PATCH 0/5] Fix spurious TINT IRQ and enhancements

[Biju Das]

Hi Thomas,

Gentle ping.

Cheers,
Biju

> -----Original Message-----
> From: Biju Das <biju.das.jz@bp.renesas.com>
> Sent: Monday, February 12, 2024 11:37 AM
> Subject: [PATCH 0/5] Fix spurious TINT IRQ and enhancements
> 
> This patch series aims to fix the spurious TINT IRQ as per the precaution mentioned in section "8.8.3
> Precaution when Changing Interrupt Settings"
> of the latest RZ/G2L hardware manual. As per this we need to mask the interrupts while setting the
> interrupt detection method. Apart from this we need to clear interrupt status after setting TINT
> interrupt detection method to the edge type.
> 
> Patch#1 in this series fixes HW race condition due to clearing delay
>         by the cpu.
> patch#2 simplifies the code and reused the same code in patch#3
> patch#3 fixes spurious tint irq
> patch#4 drops removing/adding tint source during disable()/enable()
> patch#5 simplifies enable()/disable()
> 
> Before fix: Spurious TINT IRQ's during boot root@smarc-rzg2l:~# cat /proc/interrupts | grep pinctrl
>  67:          1          0 11030000.pinctrl 344 Edge      rtc-isl1208
>  68:          0          0 11030000.pinctrl 378 Edge      SW3
>  81:          1          0 11030000.pinctrl  17 Edge      1-003d
> root@smarc-rzg2l:~#
> 
> After the fix:
> root@smarc-rzg2l:~# cat /proc/interrupts | grep pinctrl
>  67:          0          0 11030000.pinctrl 344 Edge      rtc-isl1208
>  68:          0          0 11030000.pinctrl 378 Edge      SW3
>  81:          0          0 11030000.pinctrl  17 Edge      1-003d
> root@smarc-rzg2l:~#
> 
> 
> Biju Das (5):
>   irqchip/renesas-rzg2l: Prevent IRQ HW race
>   irqchip/renesas-rzg2l: Rename rzg2l_tint_eoi()
>   irqchip/renesas-rzg2l: Fix spurious TINT IRQ
>   irqchip/renesas-rzg2l: Use TIEN for enable/disable
>   irqchip/renesas-rzg2l: Simplify rzg2l_irqc_irq_{en,dis}able()
> 
>  drivers/irqchip/irq-renesas-rzg2l.c | 88 ++++++++++++++++++++---------
>  1 file changed, 61 insertions(+), 27 deletions(-)
> 
> --
> 2.25.1