| Message ID | 20240305183922.138727-5-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| 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: 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?
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
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
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
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;
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(-)