Re: [PATCH v4 5/7] irqchip/renesas-rzv2h: Replace single irq_chip with per-region irq_chip instances

[Geert Uytterhoeven]

Hi Prabhakar,

On Tue, 24 Feb 2026 at 19:00, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Replace the single rzv2h_icu_chip and its dispatcher callbacks with
> dedicated irq_chip instances for each interrupt region: NMI, IRQ, and
> TINT.
>
> Move the irqd_is_level_type() check ahead of the scoped_guard in
> rzv2h_icu_tint_eoi() and rzv2h_icu_irq_eoi() to avoid acquiring the
> spinlock unnecessarily for level-type interrupts.
>
> Drop the ICU_TINT_START guard from rzv2h_tint_irq_endisable() since it
> is now only reachable via the TINT chip path.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>

Thanks for your patch!

> --- a/drivers/irqchip/irq-renesas-rzv2h.c
> +++ b/drivers/irqchip/irq-renesas-rzv2h.c
> @@ -169,32 +169,50 @@ static inline struct rzv2h_icu_priv *irq_data_to_priv(struct irq_data *data)
>         return data->domain->host_data;
>  }
>
> -static void rzv2h_icu_eoi(struct irq_data *d)
> +static void rzv2h_icu_tint_eoi(struct irq_data *d)
>  {
>         struct rzv2h_icu_priv *priv = irq_data_to_priv(d);
>         unsigned int hw_irq = irqd_to_hwirq(d);
>         unsigned int tintirq_nr;
>         u32 bit;
>
> -       scoped_guard(raw_spinlock, &priv->lock) {
> -               if (hw_irq >= ICU_TINT_START) {
> -                       tintirq_nr = hw_irq - ICU_TINT_START;
> -                       bit = BIT(tintirq_nr);
> -                       if (!irqd_is_level_type(d))
> -                               writel_relaxed(bit, priv->base + priv->info->t_offs + ICU_TSCLR);
> -               } else if (hw_irq >= ICU_IRQ_START) {
> -                       tintirq_nr = hw_irq - ICU_IRQ_START;
> -                       bit = BIT(tintirq_nr);
> -                       if (!irqd_is_level_type(d))
> -                               writel_relaxed(bit, priv->base + ICU_ISCLR);
> -               } else {
> -                       writel_relaxed(ICU_NSCLR_NCLR, priv->base + ICU_NSCLR);
> -               }
> +       if (!irqd_is_level_type(d)) {
> +               tintirq_nr = hw_irq - ICU_TINT_START;
> +               bit = BIT(tintirq_nr);
> +               scoped_guard(raw_spinlock, &priv->lock)
> +                       writel_relaxed(bit, priv->base + priv->info->t_offs + ICU_TSCLR);

With the big switch() decoupled from the scoped_guard(), the code
becomes easier to read. Do you actually need the scoped_guard()?
The write is not RMW, but just setting a single bit.

>         }
>
>         irq_chip_eoi_parent(d);
>  }
>
> +static void rzv2h_icu_irq_eoi(struct irq_data *d)
> +{
> +       struct rzv2h_icu_priv *priv = irq_data_to_priv(d);
> +       unsigned int hw_irq = irqd_to_hwirq(d);
> +       unsigned int tintirq_nr;
> +       u32 bit;
> +
> +       if (!irqd_is_level_type(d)) {
> +               tintirq_nr = hw_irq - ICU_IRQ_START;
> +               bit = BIT(tintirq_nr);
> +               scoped_guard(raw_spinlock, &priv->lock)
> +                       writel_relaxed(bit, priv->base + ICU_ISCLR);

Likewise.

> +       }
> +
> +       irq_chip_eoi_parent(d);
> +}
> +
> +static void rzv2h_icu_nmi_eoi(struct irq_data *d)
> +{
> +       struct rzv2h_icu_priv *priv = irq_data_to_priv(d);
> +
> +       scoped_guard(raw_spinlock, &priv->lock)
> +               writel_relaxed(ICU_NSCLR_NCLR, priv->base + ICU_NSCLR);

Likewise.

> +
> +       irq_chip_eoi_parent(d);
> +}
> +
>  static void rzv2h_tint_irq_endisable(struct irq_data *d, bool enable)
>  {
>         struct rzv2h_icu_priv *priv = irq_data_to_priv(d);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds