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

[Prabhakar Mahadev Lad]

Hi Geert,

Thank you for the review.

(For some reason this hasn’t landed in my Gmail inbox)

> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 27 February 2026 10:29
> To: Prabhakar <prabhakar.csengg@xxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxx>; Philipp Zabel
> <p.zabel@xxxxxxxxxxxxxx>; magnus.damm <magnus.damm@xxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; linux-renesas-soc@xxxxxxxxxxxxxxx; Biju Das
> <biju.das.jz@xxxxxxxxxxxxxx>; Fabrizio Castro
> <fabrizio.castro.jz@xxxxxxxxxxx>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH v4 5/7] irqchip/renesas-rzv2h: Replace single irq_chip
> with per-region irq_chip instances
> 
> 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.
>
Right, we could get rid of it (and below as pointed). I'll make these changes and send a v5.

Cheers,
Prabhakar