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 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 :) Yours sincerely, Vincent Mailhol
