Hi Thomas,
Thanks for the feedback.
> -----Original Message-----
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> 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@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.
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
