Re: [PATCH 3/5] irqchip/renesas-rzg2l: Fix spurious TINT IRQ

From: Thomas Gleixner <tglx@linutronix.de>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Biju Das <biju.das.jz@bp.renesas.com>,
	Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	Marc Zyngier <maz@kernel.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Biju Das <biju.das.au@gmail.com>,
	linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH 3/5] irqchip/renesas-rzg2l: Fix spurious TINT IRQ
Date: Fri, 01 Mar 2024 16:36:14 +0100	[thread overview]
Message-ID: <8734tax8up.ffs@tglx> (raw)
In-Reply-To: <20240212113712.71878-4-biju.das.jz@bp.renesas.com>

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