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