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@xxxxxxxxxxxxxx> > --- > 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
