Hi All, > -----Original Message----- > From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > Sent: 28 March 2025 10:43 > Subject: Re: [PATCH v7 05/18] can: rcar_canfd: Drop RCANFD_GERFL_EEF* macros in RCANFD_GERFL_ERR > > On 28/03/2025 at 19:15, Biju Das wrote: > > 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 > >>>> gpriv->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) > > Ack. Pretty close to my initial suggestion of > > #define RCANFD_GERFL_EEF0_7 GENMASK(23, 16) > FIELD_PREP(RCANFD_GERFL_EEF0_7, 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 > > Thank you :) I will drop the redundant macro -#define RCANFD_GERFL_EEF(ch) BIT(16 + (ch)) As same can be done with FIELD_PREP(RCANFD_GERFL_EEF, ch) Cheers, Biju
