| Message ID | 20240305183922.138727-6-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Fix spurious TINT IRQ and enhancements | expand |
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
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
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
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
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); }
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(-)