| Message ID | 20240212113712.71878-2-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 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
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);
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)
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(-)