Hi Biju,
On Fri, 28 Mar 2025 at 10:51, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> > On 28/03/2025 at 18:21, Biju Das wrote:
> > >> -----Original Message-----
> > >> From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> > >> Sent: 28 March 2025 09:10
> > >> Subject: Re: [PATCH v7 05/18] can: rcar_canfd: Drop RCANFD_GERFL_EEF*
> > >> macros in RCANFD_GERFL_ERR
> > >>
> > >> On 26/03/2025 à 21:19, Biju Das wrote:
> > >>> The macros RCANFD_GERFL_EEF* in RCANFD_GERFL_ERR can be replaced by
> > >>> gpriv->channels_mask << 16.
> > >>>
> > >>> After this drop the macro RCANFD_GERFL_EEF0_7 as it is unused.
> > >>>
> > >>> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > >>> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > >>> --- a/drivers/net/can/rcar/rcar_canfd.c
> > >>> +++ b/drivers/net/can/rcar/rcar_canfd.c
> > >>> @@ -74,7 +74,6 @@
> > >>> #define RCANFD_GSTS_GNOPM (BIT(0) | BIT(1) | BIT(2) | BIT(3))
> > >>>
> > >>> /* RSCFDnCFDGERFL / RSCFDnGERFL */
> > >>> -#define RCANFD_GERFL_EEF0_7 GENMASK(23, 16)
> > >>> #define RCANFD_GERFL_EEF(ch) BIT(16 + (ch))
> > >>> #define RCANFD_GERFL_CMPOF BIT(3) /* CAN FD only */
> > >>> #define RCANFD_GERFL_THLES BIT(2)
> > >>> @@ -82,9 +81,7 @@
> > >>> #define RCANFD_GERFL_DEF BIT(0)
> > >>>
> > >>> #define RCANFD_GERFL_ERR(gpriv, x) \
> > >>> - ((x) & (reg_gen4(gpriv, RCANFD_GERFL_EEF0_7, \
> > >>> - RCANFD_GERFL_EEF(0) | RCANFD_GERFL_EEF(1)) | \
> > >>> - RCANFD_GERFL_MES | \
> > >>> + ((x) & ((gpriv->channels_mask << 16) | RCANFD_GERFL_MES | \
> > >>
> > >> The previous RCANFD_GERFL_EEF0_7 macro documented that the register
> > >> was from bit index 16 to bit index 23. Here, we loose this piece of information.
> > >>
> > >> What about:
> > >>
> > >> FIELD_PREP(RCANFD_GERFL_EEF0_7, gpriv->channels_mask)
> > >
> > > For all SoCs, ECC Error flag for Channel x (a.k.a. EEFx) starts from BIT position 16.
> > >
> > > By using gpriv->channels_mask, we allow only enabled channels and <<
> > > 16 says it is from Bit position 16.
> >
> > Yes, it starts at bit 16, but at which bit does it end?
> >
> > The GENMASK() helps to document the register names. Of course is works if you replace the FIELD_PREP
> > with a left shift, but you are replacing some meaningful information about the register name, register
> > start bit and end bit by just a start bit value. See? You just lost the register name and end bit
> > info.
> >
> > FIELD_PREP() is not only about doing the correct shift but also about documenting that you are putting
> > the value into a specific register.
> >
> > When reading:
> >
> > FIELD_PREP(RCANFD_GERFL_EEF0_7, gpriv->channels_mask)
> >
> > I immediately understand that you are putting the gpriv->channels_mask value into the GERFL_EEF0_7
> > register and I can look at the datasheet for details about that GERFL_EEF0_7 if I want to.
>
> Gen4 has 8 channels (GENMASK(16, 23)
> G3E has 6 channels (GENMASK(16, 21)
> V4M has 4 channels (GENMASK(16, 19)
> V3H_2 has 3 channels (GENMASK(16,18)
> All other SoCs has 2 channels (GENMASK(16,17)
>
> So you mean, I should replace RCANFD_GERFL_EEF0_7 with a
>
> Generic one RCANFD_GERFL_EEF(x) GENMASK(16, 16 + (x) - 1) to handle
> this differences
>
> Where x is the number of supported channels(info->max_channels)
>
> and then use
>
> FIELD_PREP(RCANFD_GERFL_EEF(x), gpriv->channels_mask)
Just use
#define RCANFD_GERFL_EEF GENMASK(23, 16)
and
FIELD_PREP(RCANFD_GERFL_EEF, gpriv->channels_mask)
As channels_mask can never have bits set for non-existing channels,
all reserved bits above EEF in the GERFL register will be zero.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
