Hi Geert,
Thanks for the feedback
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 11 February 2025 13:22
> Subject: Re: [PATCH v4 09/12] irqchip/renesas-rzv2h: Drop TSSR_TIEN macro
>
> Hi Biju,
>
> On Fri, 7 Feb 2025 at 12:37, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > On RZ/G3E, TIEN bit position is at 15 compared to 7 on RZ/V2H. The
> > macro
> > ICU_TSSR_TIEN(n) can be replaced with the inline logic BIT(field_width
> > - 1) << (n * fieldwidth) for supporting both SoCs.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> Thanks for your patch!
>
> > --- a/drivers/irqchip/irq-renesas-rzv2h.c
> > +++ b/drivers/irqchip/irq-renesas-rzv2h.c
> > @@ -66,7 +66,6 @@
> >
> > #define ICU_TSSR_TSSEL_PREP(tssel, n) ((tssel) << ((n) * 8))
> > #define ICU_TSSR_TSSEL_MASK(n) ICU_TSSR_TSSEL_PREP(0x7F, n)
> > -#define ICU_TSSR_TIEN(n) (BIT(7) << ((n) * 8))
> >
> > #define ICU_TITSR_K(tint_nr) ((tint_nr) / 16)
> > #define ICU_TITSR_TITSEL_N(tint_nr) ((tint_nr) % 16)
> > @@ -153,9 +152,9 @@ static void rzv2h_tint_irq_endisable(struct irq_data *d, bool enable)
> > guard(raw_spinlock)(&priv->lock);
> > tssr = readl_relaxed(priv->base + priv->info->t_offs + ICU_TSSR(k));
> > if (enable)
> > - tssr |= ICU_TSSR_TIEN(tssel_n);
> > + tssr |= BIT(priv->info->field_width - 1) << (tssel_n *
> > + priv->info->field_width);
>
> which can be shortened to:
>
> tssr |= BIT((tssel_n + 1) * priv->info->field_width - 1);
I agree as
2 ^ (f-1) << (t * f) = 2 ^(f-1) * 2 ^ (t * f)
= 2 ^ tf * 2 ^ f * 2 ^ -1
= 2 ^ (tf + f - 1)
= 2 ^ ((t+ 1) * f - 1)
>
> > else
> > - tssr &= ~ICU_TSSR_TIEN(tssel_n);
> > + tssr &= ~(BIT(priv->info->field_width - 1) << (tssel_n
> > + * priv->info->field_width));
> > writel_relaxed(tssr, priv->base + priv->info->t_offs +
> > ICU_TSSR(k)); }
> >
> > @@ -317,7 +316,7 @@ static int rzv2h_tint_set_type(struct irq_data *d,
> > unsigned int type)
> >
> > titsr_k = ICU_TITSR_K(tint_nr);
> > titsel_n = ICU_TITSR_TITSEL_N(tint_nr);
> > - tien = ICU_TSSR_TIEN(titsel_n);
> > + tien = BIT(priv->info->field_width - 1) << (titsel_n *
> > + priv->info->field_width);
>
> This should use "tssel_n" instead of "titsel_n" as the index.
> Note that this is a pre-existing bug, so you probably want to fix that in a separate patch (and move
> the line up, next to the other tssr calculations).
OK.
>
> Given you'll be introducing more shifting in the next patch, it may be worthwhile to store the shift
> value in a variable.
OK.
Cheers,
Biju
