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