| Message ID | 20250128104714.80807-10-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add Support for RZ/G3E ICU | expand |
Hi Biju, On Tue, 28 Jan 2025 at 11:47, Biju Das <biju.das.jz@bp.renesas.com> 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@renesas.com> > Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- 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); tssel_shift = field_width; tien = BIT(field_width - 1); tint_nr / tssr_k = tint_nr * field_width / 32; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
Hi Geert, Thanks for the feedback. > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > 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@bp.renesas.com> 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@renesas.com> > > Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > --- 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
diff --git a/drivers/irqchip/irq-renesas-rzv2h.c b/drivers/irqchip/irq-renesas-rzv2h.c index b52f5a85ce1c..745614f6991c 100644 --- 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; }; /** @@ -145,8 +145,8 @@ static void rzv2h_tint_irq_endisable(struct irq_data *d, bool enable) return; tint_nr = hw_irq - ICU_TINT_START; - k = ICU_TSSR_K(tint_nr); - tssel_n = ICU_TSSR_TSSEL_N(tint_nr); + k = tint_nr / priv->info->tssr_k; + tssel_n = tint_nr % priv->info->tssr_k; guard(raw_spinlock)(&priv->lock); tssr = readl_relaxed(priv->base + priv->info->t_offs + ICU_TSSR(k)); @@ -308,8 +308,8 @@ static int rzv2h_tint_set_type(struct irq_data *d, unsigned int type) hwirq = irqd_to_hwirq(d); tint_nr = hwirq - ICU_TINT_START; - tssr_k = ICU_TSSR_K(tint_nr); - tssel_n = ICU_TSSR_TSSEL_N(tint_nr); + tssr_k = tint_nr / priv->info->tssr_k; + tssel_n = tint_nr % priv->info->tssr_k; titsr_k = ICU_TITSR_K(tint_nr); titsel_n = ICU_TITSR_TITSEL_N(tint_nr); @@ -519,6 +519,7 @@ static int rzv2h_icu_init_common(struct device_node *node, struct device_node *p static const struct rzv2h_hw_info rzv2h_hw_params = { .t_offs = 0, .max_tssel = ICU_RZV2H_TSSEL_MAX_VAL, + .tssr_k = 4, }; static int rzv2h_icu_init(struct device_node *node, struct device_node *parent)