| Message ID | 20250207113653.21641-10-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add Support for RZ/G3E ICU | expand |
Hi Biju, On Fri, 7 Feb 2025 at 12:37, Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> 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); > 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). Given you'll be introducing more shifting in the next patch, it may be worthwhile to store the shift value in a variable. > > guard(raw_spinlock)(&priv->lock); Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > 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@bp.renesas.com> 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@bp.renesas.com> > > 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
diff --git a/drivers/irqchip/irq-renesas-rzv2h.c b/drivers/irqchip/irq-renesas-rzv2h.c index d96e4b2032b4..6d1de9277c7d 100644 --- 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); 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); guard(raw_spinlock)(&priv->lock);
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@bp.renesas.com> --- v4: * New patch --- drivers/irqchip/irq-renesas-rzv2h.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)