RE: [PATCH v3 09/13] irqchip/renesas-rzv2h: Add tssr_k variable to struct rzv2h_hw_info

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux