diff mbox series

[v7,05/18] can: rcar_canfd: Drop RCANFD_GERFL_EEF* macros in RCANFD_GERFL_ERR

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

Commit Message

Biju Das March 26, 2025, 12:19 p.m. UTC
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(-)

Comments

Vincent Mailhol March 28, 2025, 9:10 a.m. UTC | #1
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
Biju Das March 28, 2025, 9:21 a.m. UTC | #2
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
Vincent Mailhol March 28, 2025, 9:37 a.m. UTC | #3
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
Biju Das March 28, 2025, 9:51 a.m. UTC | #4
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
Geert Uytterhoeven March 28, 2025, 10:10 a.m. UTC | #5
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
Biju Das March 28, 2025, 10:15 a.m. UTC | #6
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
Marc Kleine-Budde March 28, 2025, 10:20 a.m. UTC | #7
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
Marc Kleine-Budde March 28, 2025, 10:21 a.m. UTC | #8
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
Geert Uytterhoeven March 28, 2025, 10:42 a.m. UTC | #9
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
Vincent Mailhol March 28, 2025, 10:43 a.m. UTC | #10
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
Vincent Mailhol March 28, 2025, 1:42 p.m. UTC | #11
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
Biju Das March 28, 2025, 3:47 p.m. UTC | #12
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 mbox series

Patch

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 */