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

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

[Biju Das]

This patch series aims to fix the spurious 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 masking of the
interrupts need to be done before setting the interrupt detection method.
Then set the TINT interrupt detection method followed by clearing the
status register for fixing spurious IRQ.

Patch#1 in this series flushes the posted write
patch#2 and #3 simplifies the code and reused the same in patch#3
patch#4 fixes spurious irq
patch#5 drops removing/adding tint source during disable()/enable()

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:~#

v1->v2:
 * Dropped pinctrl dependency as it hits next.
 * Updated commit header and description for patch#1.
 * Extended the flushing of posted write to IRQ interrupt as well.
 * Updated patch#2 commit description *_tint_eoi()->*_clear_tint_int().
 * Added Rb tag from Geert for patch#2.
 * Added Patch#3 is for replacing rzg2l_irq_eoi()->rzg2l_clear_irq_int().
 * Updated commit header and description for patch#4.
 * Extended spurious IRQ fix for TINT to IRQ as well.
 * Updated the logic for rzg2l_disable_tint_and_set_tint_source() and
   rzg2l_tint_set_edge().
 * Merged patch#4 and #5 and updated commit description.

Biju Das (5):
  irqchip/renesas-rzg2l: Flush posted write
  irqchip/renesas-rzg2l: Rename rzg2l_tint_eoi()
  irqchip/renesas-rzg2l: Rename rzg2l_irq_eoi()
  irqchip/renesas-rzg2l: Fix spurious IRQ
  irqchip/renesas-rzg2l: Use TIEN for enable/disable

 drivers/irqchip/irq-renesas-rzg2l.c | 101 ++++++++++++++++++----------
 1 file changed, 66 insertions(+), 35 deletions(-)

-- 
2.25.1

[PATCH v2 1/5] irqchip/renesas-rzg2l: Flush posted write

[Biju Das]

The irq_eoi() callback of the RZ/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.

Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * Updated commit header and description.
 * Extended the flushing of posted write to IRQ interrupt aswell.
---
 drivers/irqchip/irq-renesas-rzg2l.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 9494fc26259c..5285bc817dd0 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -99,8 +99,14 @@ static void rzg2l_irq_eoi(struct irq_data *d)
 	 * ISCR can only be cleared if the type is falling-edge, rising-edge or
 	 * falling/rising-edge.
 	 */
-	if ((iscr & bit) && (iitsr & IITSR_IITSEL_MASK(hw_irq)))
+	if ((iscr & bit) && (iitsr & IITSR_IITSEL_MASK(hw_irq))) {
 		writel_relaxed(iscr & ~bit, priv->base + ISCR);
+		/*
+		 * Enforce that the posted write is flushed to prevent that the
+		 * just handled interrupt is raised again.
+		 */
+		readl_relaxed(priv->base + ISCR);
+	}
 }
 
 static void rzg2l_tint_eoi(struct irq_data *d)
@@ -111,8 +117,14 @@ 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);
+		/*
+		 * Enforce that the posted write is flushed to prevent that the
+		 * just handled interrupt is raised again.
+		 */
+		readl_relaxed(priv->base + TSCR);
+	}
 }
 
 static void rzg2l_irqc_eoi(struct irq_data *d)
-- 
2.25.1

[PATCH v2 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>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v1->v2:
 * Updated commit description rzg2l_tint_eoi()->rzg2l_clear_tint_int().
 * Added Rb tag from Geert.
---
 drivers/irqchip/irq-renesas-rzg2l.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 5285bc817dd0..599e0aba5cc0 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -109,11 +109,9 @@ static void rzg2l_irq_eoi(struct irq_data *d)
 	}
 }
 
-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);
@@ -136,7 +134,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

[PATCH v2 3/5] irqchip/renesas-rzg2l: Rename rzg2l_irq_eoi()

[Biju Das]

Rename rzg2l_irq_eoi()->rzg2l_clear_irq_int() and simplify the code by
removing redundant priv local variable.

Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * New patch.
---
 drivers/irqchip/irq-renesas-rzg2l.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 599e0aba5cc0..8133f05590b6 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -85,10 +85,9 @@ static struct rzg2l_irqc_priv *irq_data_to_priv(struct irq_data *data)
 	return data->domain->host_data;
 }
 
-static void rzg2l_irq_eoi(struct irq_data *d)
+static void rzg2l_clear_irq_int(struct rzg2l_irqc_priv *priv, unsigned int hwirq)
 {
-	unsigned int hw_irq = irqd_to_hwirq(d) - IRQC_IRQ_START;
-	struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
+	unsigned int hw_irq = hwirq - IRQC_IRQ_START;
 	u32 bit = BIT(hw_irq);
 	u32 iitsr, iscr;
 
@@ -132,7 +131,7 @@ static void rzg2l_irqc_eoi(struct irq_data *d)
 
 	raw_spin_lock(&priv->lock);
 	if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT)
-		rzg2l_irq_eoi(d);
+		rzg2l_clear_irq_int(priv, hw_irq);
 	else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ)
 		rzg2l_clear_tint_int(priv, hw_irq);
 	raw_spin_unlock(&priv->lock);
-- 
2.25.1

[PATCH v2 4/5] irqchip/renesas-rzg2l: Fix spurious IRQ

[Biju Das]

On RZ/G2L interrupt chip, interrupt masking is required before changing
the NMI, IRQ, TINT interrupt settings. Apart from this, after setting
the edge type it is required to clear interrupt status register in
order to avoid spurious IRQ.

For IRQ edge type, use raw_spin_lock()->raw_spin_lock_irqsave() and in
case of TINT edge type use TIEN for interrupt masking. Then set the
interrupt detection register followed by clearing interrupt status
register to fix the spurious IRQ.

Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * Updated commit header and description.
 * Extended spurious IRQ fix to IRQ as well.
 * Updated the logic for rzg2l_disable_tint_and_set_tint_source() and
   rzg2l_tint_set_edge().
---
 drivers/irqchip/irq-renesas-rzg2l.c | 41 ++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 8133f05590b6..e793b8f07dac 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -181,8 +181,11 @@ static void rzg2l_irqc_irq_enable(struct irq_data *d)
 
 static int rzg2l_irq_set_type(struct irq_data *d, unsigned int type)
 {
-	unsigned int hw_irq = irqd_to_hwirq(d) - IRQC_IRQ_START;
 	struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
+	unsigned int hwirq = irqd_to_hwirq(d);
+	u32 iitseln = hwirq - IRQC_IRQ_START;
+	bool clear_irq_int = false;
+	unsigned long flags;
 	u16 sense, tmp;
 
 	switch (type & IRQ_TYPE_SENSE_MASK) {
@@ -192,37 +195,59 @@ static int rzg2l_irq_set_type(struct irq_data *d, unsigned int type)
 
 	case IRQ_TYPE_EDGE_FALLING:
 		sense = IITSR_IITSEL_EDGE_FALLING;
+		clear_irq_int = true;
 		break;
 
 	case IRQ_TYPE_EDGE_RISING:
 		sense = IITSR_IITSEL_EDGE_RISING;
+		clear_irq_int = true;
 		break;
 
 	case IRQ_TYPE_EDGE_BOTH:
 		sense = IITSR_IITSEL_EDGE_BOTH;
+		clear_irq_int = true;
 		break;
 
 	default:
 		return -EINVAL;
 	}
 
-	raw_spin_lock(&priv->lock);
+	raw_spin_lock_irqsave(&priv->lock, flags);
 	tmp = readl_relaxed(priv->base + IITSR);
-	tmp &= ~IITSR_IITSEL_MASK(hw_irq);
-	tmp |= IITSR_IITSEL(hw_irq, sense);
+	tmp &= ~IITSR_IITSEL_MASK(iitseln);
+	tmp |= IITSR_IITSEL(iitseln, sense);
+	if (clear_irq_int)
+		rzg2l_clear_irq_int(priv, hwirq);
 	writel_relaxed(tmp, priv->base + IITSR);
-	raw_spin_unlock(&priv->lock);
+	raw_spin_unlock_irqrestore(&priv->lock, flags);
 
 	return 0;
 }
 
+static u32 rzg2l_disable_tint_and_set_tint_source(struct irq_data *d, struct rzg2l_irqc_priv *priv,
+						  u32 reg, u32 tssr_offset, u8 tssr_index)
+{
+	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;
+}
+
 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);
 	u8 index, sense;
-	u32 reg;
+	u32 reg, tssr;
 
 	switch (type & IRQ_TYPE_SENSE_MASK) {
 	case IRQ_TYPE_EDGE_RISING:
@@ -244,10 +269,14 @@ 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));
+	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);
 
 	return 0;
-- 
2.25.1

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

[Biju Das]

Currently hardware settings for TINT detection is not in sync with
TINT source as the enable/disable overrides source setting value leading
to hardware inconsistent state. For eg: consider the case GPIOINT0 is used
as TINT interrupt and configuring GPIOINT5 as edgetype. During disable the
clearing of the entire bytes of TINT source selection for GPIOINT5 is same
as GPIOINT0 with TIEN disabled. Other than this during enabling, the
setting of GPIOINT5 with TIEN results in spurious IRQ as due to a HW race,
it is possible that IP can use the TIEN with previous source value
(GPIOINT0).

So, it is better to just use TIEN for enable/disable and avoid modifying
TINT source selection register.This will make the consistent hardware
settings for detection method tied with TINT source and allows to simplify
the code.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * Merged patch#4 and #5 and updated commit description.
---
 drivers/irqchip/irq-renesas-rzg2l.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index e793b8f07dac..a7d47dbf7627 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -138,7 +138,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);
 
@@ -151,31 +151,24 @@ 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));
+		if (enable)
+			reg |= TIEN << TSSEL_SHIFT(tssr_offset);
+		else
+			reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset));
 		writel_relaxed(reg, priv->base + TSSR(tssr_index));
 		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) {
-		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);
-		u8 tssr_index = TSSR_INDEX(offset);
-		u32 reg;
-
-		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));
-		raw_spin_unlock(&priv->lock);
-	}
+	rzg2l_tint_irq_endisable(d, true);
 	irq_chip_enable_parent(d);
 }
 
-- 
2.25.1

Re: [PATCH v2 4/5] irqchip/renesas-rzg2l: Fix spurious IRQ

[Thomas Gleixner]

On Tue, Mar 05 2024 at 18:39, Biju Das wrote:

Sorry. I just noticed that this series fell through the cracks.

>  static int rzg2l_irq_set_type(struct irq_data *d, unsigned int type)
>  {
> -	unsigned int hw_irq = irqd_to_hwirq(d) - IRQC_IRQ_START;
>  	struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> +	unsigned int hwirq = irqd_to_hwirq(d);
> +	u32 iitseln = hwirq - IRQC_IRQ_START;
> +	bool clear_irq_int = false;
> +	unsigned long flags;
>  	u16 sense, tmp;
>  
>  	switch (type & IRQ_TYPE_SENSE_MASK) {
> @@ -192,37 +195,59 @@ static int rzg2l_irq_set_type(struct irq_data *d, unsigned int type)
>  
>  	case IRQ_TYPE_EDGE_FALLING:
>  		sense = IITSR_IITSEL_EDGE_FALLING;
> +		clear_irq_int = true;
>  		break;
>  
>  	case IRQ_TYPE_EDGE_RISING:
>  		sense = IITSR_IITSEL_EDGE_RISING;
> +		clear_irq_int = true;
>  		break;
>  
>  	case IRQ_TYPE_EDGE_BOTH:
>  		sense = IITSR_IITSEL_EDGE_BOTH;
> +		clear_irq_int = true;
>  		break;
>  
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	raw_spin_lock(&priv->lock);
> +	raw_spin_lock_irqsave(&priv->lock, flags);

irq_set_type() is always called with irq_desc::lock held and
interrupts disabled. What's exactly the point of this change?

RE: [PATCH v2 4/5] irqchip/renesas-rzg2l: Fix spurious IRQ

[Biju Das]

Hi Thomas,

> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Wednesday, March 13, 2024 2:38 PM
> Subject: Re: [PATCH v2 4/5] irqchip/renesas-rzg2l: Fix spurious IRQ
> 
> On Tue, Mar 05 2024 at 18:39, Biju Das wrote:
> 
> Sorry. I just noticed that this series fell through the cracks.
> 
> >  static int rzg2l_irq_set_type(struct irq_data *d, unsigned int type)
> > {
> > -	unsigned int hw_irq = irqd_to_hwirq(d) - IRQC_IRQ_START;
> >  	struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> > +	unsigned int hwirq = irqd_to_hwirq(d);
> > +	u32 iitseln = hwirq - IRQC_IRQ_START;
> > +	bool clear_irq_int = false;
> > +	unsigned long flags;
> >  	u16 sense, tmp;
> >
> >  	switch (type & IRQ_TYPE_SENSE_MASK) { @@ -192,37 +195,59 @@ static
> > int rzg2l_irq_set_type(struct irq_data *d, unsigned int type)
> >
> >  	case IRQ_TYPE_EDGE_FALLING:
> >  		sense = IITSR_IITSEL_EDGE_FALLING;
> > +		clear_irq_int = true;
> >  		break;
> >
> >  	case IRQ_TYPE_EDGE_RISING:
> >  		sense = IITSR_IITSEL_EDGE_RISING;
> > +		clear_irq_int = true;
> >  		break;
> >
> >  	case IRQ_TYPE_EDGE_BOTH:
> >  		sense = IITSR_IITSEL_EDGE_BOTH;
> > +		clear_irq_int = true;
> >  		break;
> >
> >  	default:
> >  		return -EINVAL;
> >  	}
> >
> > -	raw_spin_lock(&priv->lock);
> > +	raw_spin_lock_irqsave(&priv->lock, flags);
> 
> irq_set_type() is always called with irq_desc::lock held and interrupts disabled. What's exactly the
> point of this change?

Oops, in that case this change is not needed.

HW manual mentions, interrupt should be disabled while setting the value. 

I will drop this change in next version.

Cheers,
Biju

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

[Thomas Gleixner]

On Tue, Mar 05 2024 at 18:39, Biju Das wrote:
> Currently hardware settings for TINT detection is not in sync with
> TINT source as the enable/disable overrides source setting value leading
> to hardware inconsistent state. For eg: consider the case GPIOINT0 is used
> as TINT interrupt and configuring GPIOINT5 as edgetype. During disable the
> clearing of the entire bytes of TINT source selection for GPIOINT5 is same
> as GPIOINT0 with TIEN disabled. Other than this during enabling, the
> setting of GPIOINT5 with TIEN results in spurious IRQ as due to a HW race,
> it is possible that IP can use the TIEN with previous source value
> (GPIOINT0).
>
> So, it is better to just use TIEN for enable/disable and avoid modifying
> TINT source selection register.This will make the consistent hardware
> settings for detection method tied with TINT source and allows to simplify
> the code.

I have no idea how the subject and change log is related to what the
patch is doing.

The patch just consolidates the almost identical functionality of
rzg2l_irqc_irq_disable() and rzg2l_irqc_irq_enable() into a helper
function which is invoked from both places. The existing code already
uses TIEN for disable and enable, so what's the change?

IOW, it's zero functional change and completely unrelated to the above
blurb.

Thanks,

        tglx

RE: [PATCH v2 4/5] irqchip/renesas-rzg2l: Fix spurious IRQ

[Thomas Gleixner]

On Wed, Mar 13 2024 at 14:58, Biju Das wrote:
>> > -	raw_spin_lock(&priv->lock);
>> > +	raw_spin_lock_irqsave(&priv->lock, flags);
>> 
>> irq_set_type() is always called with irq_desc::lock held and interrupts disabled. What's exactly the
>> point of this change?
>
> Oops, in that case this change is not needed.
>
> HW manual mentions, interrupt should be disabled while setting the value. 
>
> I will drop this change in next version.

I fixed it up locally already. I'm going to merge 1-4 because those are
fixes (2/3 are preparatory). #5 wants a change log matching reality
though.

Thanks,

        tglx

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

[Biju Das]

Hi Thomas,

> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Wednesday, March 13, 2024 3:40 PM
> Subject: Re: [PATCH v2 5/5] irqchip/renesas-rzg2l: Use TIEN for enable/disable
> 
> On Tue, Mar 05 2024 at 18:39, Biju Das wrote:
> > Currently hardware settings for TINT detection is not in sync with
> > TINT source as the enable/disable overrides source setting value
> > leading to hardware inconsistent state. For eg: consider the case
> > GPIOINT0 is used as TINT interrupt and configuring GPIOINT5 as
> > edgetype. During disable the clearing of the entire bytes of TINT
> > source selection for GPIOINT5 is same as GPIOINT0 with TIEN disabled.
> > Other than this during enabling, the setting of GPIOINT5 with TIEN
> > results in spurious IRQ as due to a HW race, it is possible that IP
> > can use the TIEN with previous source value (GPIOINT0).
> >
> > So, it is better to just use TIEN for enable/disable and avoid
> > modifying TINT source selection register.This will make the consistent
> > hardware settings for detection method tied with TINT source and
> > allows to simplify the code.
> 
> I have no idea how the subject and change log is related to what the patch is doing.
> 
> The patch just consolidates the almost identical functionality of
> rzg2l_irqc_irq_disable() and rzg2l_irqc_irq_enable() into a helper function which is invoked from both
> places. The existing code already uses TIEN for disable and enable, so what's the change?
> 
> IOW, it's zero functional change and completely unrelated to the above blurb.

There is functional change. During disable, TINT source and TIEN cleared together

reg &= ~(TSSEL_MASK << TSSEL_SHIFT(tssr_offset));

During Enable, TINT source and TIEN set together

reg |= (TIEN | tint) << TSSEL_SHIFT(tssr_offset);

This patch avoids modifying TINT source register which avoids hw race
as mentioned by hardware team. According to them we should not
set TINT source and TIEN together.

I can update the change log.

Cheers,
Biju

RE: [PATCH v2 4/5] irqchip/renesas-rzg2l: Fix spurious IRQ

[Biju Das]

Hi Thomas,

> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Wednesday, March 13, 2024 3:43 PM
> Subject: RE: [PATCH v2 4/5] irqchip/renesas-rzg2l: Fix spurious IRQ
> 
> On Wed, Mar 13 2024 at 14:58, Biju Das wrote:
> >> > -	raw_spin_lock(&priv->lock);
> >> > +	raw_spin_lock_irqsave(&priv->lock, flags);
> >>
> >> irq_set_type() is always called with irq_desc::lock held and
> >> interrupts disabled. What's exactly the point of this change?
> >
> > Oops, in that case this change is not needed.
> >
> > HW manual mentions, interrupt should be disabled while setting the value.
> >
> > I will drop this change in next version.
> 
> I fixed it up locally already. I'm going to merge 1-4 because those are fixes (2/3 are preparatory). #5
> wants a change log matching reality though.

Thanks. I will sent #5 updating the change log.

Cheers,
Biju

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

[Thomas Gleixner]

On Wed, Mar 13 2024 at 15:59, Biju Das wrote:
>> From: Thomas Gleixner <tglx@linutronix.de>
>> IOW, it's zero functional change and completely unrelated to the above blurb.
>
> There is functional change. During disable, TINT source and TIEN cleared together
>
> reg &= ~(TSSEL_MASK << TSSEL_SHIFT(tssr_offset));
>
> During Enable, TINT source and TIEN set together
>
> reg |= (TIEN | tint) << TSSEL_SHIFT(tssr_offset);
>
> This patch avoids modifying TINT source register which avoids hw race
> as mentioned by hardware team. According to them we should not
> set TINT source and TIEN together.

Can you please split this into two pieces?

    1) The fix itself at both places

    2) The consolidation

That way it's obvious what this is doing. I really missed that subtle
change.

Thanks,

        tglx

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

[Biju Das]

Hi Thomas,

> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Thursday, March 14, 2024 8:59 AM
> Subject: RE: [PATCH v2 5/5] irqchip/renesas-rzg2l: Use TIEN for enable/disable
> 
> On Wed, Mar 13 2024 at 15:59, Biju Das wrote:
> >> From: Thomas Gleixner <tglx@linutronix.de> IOW, it's zero functional
> >> change and completely unrelated to the above blurb.
> >
> > There is functional change. During disable, TINT source and TIEN
> > cleared together
> >
> > reg &= ~(TSSEL_MASK << TSSEL_SHIFT(tssr_offset));
> >
> > During Enable, TINT source and TIEN set together
> >
> > reg |= (TIEN | tint) << TSSEL_SHIFT(tssr_offset);
> >
> > This patch avoids modifying TINT source register which avoids hw race
> > as mentioned by hardware team. According to them we should not set
> > TINT source and TIEN together.
> 
> Can you please split this into two pieces?
> 
>     1) The fix itself at both places
> 
>     2) The consolidation
> 
> That way it's obvious what this is doing. I really missed that subtle change.

OK, will split this patch into two.

Thanks,
Biju