Message ID | 20250326122003.122976-6-biju.das.jz@bp.renesas.com (mailing list archive) |
---|---|
State | New |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Series | Add support for RZ/G3E CANFD | expand |
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@glider.be> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v6->v7: > * No change. > v5->v6: > * Collected tag. > v5: > * New patch. > --- > drivers/net/can/rcar/rcar_canfd.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c > index 565a91c2ca83..a9e962a1510e 100644 > --- 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) ? Yours sincerely, Vincent Mailhol
Hi Vincent, Thanks for the feedback. > -----Original Message----- > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > 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@glider.be> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v6->v7: > > * No change. > > v5->v6: > > * Collected tag. > > v5: > > * New patch. > > --- > > drivers/net/can/rcar/rcar_canfd.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/drivers/net/can/rcar/rcar_canfd.c > > b/drivers/net/can/rcar/rcar_canfd.c > > index 565a91c2ca83..a9e962a1510e 100644 > > --- 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. Gen4 has 7 channels G3E has 6 channels V4M has 4 channels V3H_2 has 3 channels All other SoCs has 2 channels Am I missing anything here? Cheers, Biju
On 28/03/2025 at 18:21, Biju Das wrote: > Hi Vincent, > > Thanks for the feedback. > >> -----Original Message----- >> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> >> 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@glider.be> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>> --- >>> v6->v7: >>> * No change. >>> v5->v6: >>> * Collected tag. >>> v5: >>> * New patch. >>> --- >>> drivers/net/can/rcar/rcar_canfd.c | 5 +---- >>> 1 file changed, 1 insertion(+), 4 deletions(-) >>> >>> diff --git a/drivers/net/can/rcar/rcar_canfd.c >>> b/drivers/net/can/rcar/rcar_canfd.c >>> index 565a91c2ca83..a9e962a1510e 100644 >>> --- 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. When reading: gpriv->channels_mask << 16 I just see the magic number 16. No clue what this means at first glance. > Gen4 has 7 channels > G3E has 6 channels > V4M has 4 channels > V3H_2 has 3 channels > All other SoCs has 2 channels > > Am I missing anything here? Yours sincerely, Vincent Mailhol
Hi Vincent, > -----Original Message----- > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > Sent: 28 March 2025 09:37 > Subject: Re: [PATCH v7 05/18] can: rcar_canfd: Drop RCANFD_GERFL_EEF* macros in RCANFD_GERFL_ERR > > On 28/03/2025 at 18:21, Biju Das wrote: > > Hi Vincent, > > > > Thanks for the feedback. > > > >> -----Original Message----- > >> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > >> 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@glider.be> > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >>> --- > >>> v6->v7: > >>> * No change. > >>> v5->v6: > >>> * Collected tag. > >>> v5: > >>> * New patch. > >>> --- > >>> drivers/net/can/rcar/rcar_canfd.c | 5 +---- > >>> 1 file changed, 1 insertion(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/net/can/rcar/rcar_canfd.c > >>> b/drivers/net/can/rcar/rcar_canfd.c > >>> index 565a91c2ca83..a9e962a1510e 100644 > >>> --- 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) Cheers, Biju
Hi Biju, On Fri, 28 Mar 2025 at 10:51, Biju Das <biju.das.jz@bp.renesas.com> wrote: > > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > > On 28/03/2025 at 18:21, Biju Das wrote: > > >> -----Original Message----- > > >> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > > >> 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@glider.be> > > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > >>> --- 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
Hi Geert and Vincent, > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > 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@bp.renesas.com> wrote: > > > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> On 28/03/2025 at > > > 18:21, Biju Das wrote: > > > >> -----Original Message----- > > > >> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > > > >> 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@glider.be> > > > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > >>> --- 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
On 28.03.2025 09:51:43, Biju Das wrote: > > 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) The mask for FIELD_PREP must be compile time constant. Geert recently posted a series to add non constant helpers: | https://lore.kernel.org/all/1824412519cb8791ab428065116927ee7b77cf35.1738329459.git.geert+renesas@glider.be/ It seems it wasn't applied yet... Marc
On 28.03.2025 11:10:23, Geert Uytterhoeven wrote: > > 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. That's even better than computing the mask at runtime! Marc
Hi Marc, On Fri, 28 Mar 2025 at 11:20, Marc Kleine-Budde <mkl@pengutronix.de> wrote: > On 28.03.2025 09:51:43, Biju Das wrote: > > > 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) > > The mask for FIELD_PREP must be compile time constant. > > Geert recently posted a series to add non constant helpers: > > | https://lore.kernel.org/all/1824412519cb8791ab428065116927ee7b77cf35.1738329459.git.geert+renesas@glider.be/ > > It seems it wasn't applied yet... Yeah, people keep on asking for more... #perfectistheenemyofgoodenough... Gr{oetje,eeting}s, Geert
On 28/03/2025 at 19:15, Biju Das wrote: > Hi Geert and Vincent, > >> -----Original Message----- >> From: Geert Uytterhoeven <geert@linux-m68k.org> >> 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@bp.renesas.com> wrote: >>>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> On 28/03/2025 at >>>> 18:21, Biju Das wrote: >>>>>> -----Original Message----- >>>>>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> >>>>>> 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@glider.be> >>>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >> >>>>>>> --- 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
On 28/03/2025 at 19:42, Geert Uytterhoeven wrote: > Hi Marc, > > On Fri, 28 Mar 2025 at 11:20, Marc Kleine-Budde <mkl@pengutronix.de> wrote: >> On 28.03.2025 09:51:43, Biju Das wrote: >>>> 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) >> >> The mask for FIELD_PREP must be compile time constant. >> >> Geert recently posted a series to add non constant helpers: >> >> | https://lore.kernel.org/all/1824412519cb8791ab428065116927ee7b77cf35.1738329459.git.geert+renesas@glider.be/ >> >> It seems it wasn't applied yet... > > Yeah, people keep on asking for more... > #perfectistheenemyofgoodenough... Modifying the most popular headers really takes a lot of effort. I experienced it first hand when I introduced the GEMASK_U*() fixed width macros: https://lore.kernel.org/all/20250326-fixed-type-genmasks-v8-0-24afed16ca00@wanadoo.fr/ Sorry to have been one of those asking for more. My intent was not to block the effort. Your answers were satisfactory to me. In my opinion, just adding a rationale to the patch description of: - why FIELD_PREP() can not be generalized to non-const mask - why the u32_get_bits() & Cie do not work as well would help to have it move forward. If you resend, I would give you my support. Yours sincerely, Vincent Mailhol
Hi All, > -----Original Message----- > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > 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@linux-m68k.org> > >> 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@bp.renesas.com> wrote: > >>>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> On 28/03/2025 at > >>>> 18:21, Biju Das wrote: > >>>>>> -----Original Message----- > >>>>>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > >>>>>> 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@glider.be> > >>>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >> > >>>>>>> --- 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
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c index 565a91c2ca83..a9e962a1510e 100644 --- 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 | \ ((gpriv)->fdmode ? RCANFD_GERFL_CMPOF : 0))) /* AFL Rx rules registers */