Hi Geert,
Thanks for the feedback.
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 28 January 2025 15:38
> Subject: Re: [PATCH v3 09/13] irqchip/renesas-rzv2h: Add tssr_k variable to struct rzv2h_hw_info
>
> Hi Biju,
>
> On Tue, 28 Jan 2025 at 11:47, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > On RZ/G3E the number of TSSR registers is 15 compared to 8 on RZ/V2H
> > and
>
> s/15/16/
>
> > each TSSR register can program 2 TINTs compared to 4 on RZ/V2H.
> >
> > Add tssr_k variable to struct rzv2h_hw_info to handle this difference
> > and drop the macros ICU_TSSR_K and ICU_TSSR_TSSEL_N.
> >
> > Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> > Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> > --- a/drivers/irqchip/irq-renesas-rzv2h.c
> > +++ b/drivers/irqchip/irq-renesas-rzv2h.c
> > @@ -64,8 +64,6 @@
> > #define ICU_TINT_LEVEL_HIGH 2
> > #define ICU_TINT_LEVEL_LOW 3
> >
> > -#define ICU_TSSR_K(tint_nr) ((tint_nr) / 4)
> > -#define ICU_TSSR_TSSEL_N(tint_nr) ((tint_nr) % 4)
> > #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))
> > @@ -84,10 +82,12 @@
> > * struct rzv2h_hw_info - Interrupt Control Unit controller hardware info structure.
> > * @t_offs: TINT offset
> > * @max_tssel: TSSEL max value
> > + * @tssr_k: TSSR index k
> > */
> > struct rzv2h_hw_info {
> > u16 t_offs;
> > u8 max_tssel;
> > + u8 tssr_k;
>
> So this is 4 on RZ/V2H, and 2 on RZ/G3E.
> In the next two patches you are adding:
>
> u16 tssel_mask; /* GENMASK(6, 0) resp. GENMASK(7, 0) */
> u8 tssel_shift; /* 8 resp. 16 */
> u16 tien; /* BIT(7) resp. BIT(15) */
>
> I think you're going a bit too far in the parametrization.
> The key difference between the two variants is that RZ/V2H uses a field width of 8 bits, while RZ/G3E
> uses 16 bits. From this single parameter, you can easily derive all other values, so there is no need
> to store them in struct rzv2h_hw_info:
>
> tssel_mask = GENMASK(field_width - 2, 0);
Yes, as for RZ/G3E, bits 8-->14 are reserved and write is ignored. So it is safe
to use GENMASK(14, 0) for RZ/G3E.
> tssel_shift = field_width;
> tien = BIT(field_width - 1);
> tint_nr / tssr_k = tint_nr * field_width / 32;
I agree this reduces the number of variables in struct rzv2h_hw_info.
I will use field_width in the next version.
Cheers,
Biju
