Hi Geert and Vincent, > -----Original Message----- > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> > Sent: 28 March 2025 10:10 > Subject: Re: [PATCH v7 05/18] can: rcar_canfd: Drop RCANFD_GERFL_EEF* macros in RCANFD_GERFL_ERR > > 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. Agreed. Will add this change in next version. Cheers, Biju
