[v7,07/18] can: rcar_canfd: Add rnc_stride variable to struct rcar_canfd_hw_info

Commit Message

Biju Das March 26, 2025, 12:19 p.m. UTC

R-Car Gen4 packs 2 RNC values in a 32-bit word, whereas R-Car Gen3 packs
up to 4 values in a 32-bit word. Handle this difference by adding
rnc_stride variable to struct rcar_canfd_hw_info and update the macro
RCANFD_GAFLCFG.

Reviewed-by: Geert Uytterhoeven <[e-mail]>
Signed-off-by: Biju Das <[e-mail]>
---
v6->v7:
 * Collected tag.
v5->v6:
 * Replaced RCANFD_RNC_PER_REG macro with rnc_stride variable.
 * Updated commit description
 * Dropped Rb tag.
v5:
 * New patch.
---
 drivers/net/can/rcar/rcar_canfd.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Vincent Mailhol March 28, 2025, 10:36 a.m. UTC | #1

On 26/03/2025 at 21:19, Biju Das wrote:
> R-Car Gen4 packs 2 RNC values in a 32-bit word, whereas R-Car Gen3 packs
> up to 4 values in a 32-bit word. Handle this difference by adding
> rnc_stride variable to struct rcar_canfd_hw_info and update the macro
> RCANFD_GAFLCFG.
> 
> Reviewed-by: Geert Uytterhoeven <[e-mail]>
> Signed-off-by: Biju Das <[e-mail]>
> ---
> v6->v7:
>  * Collected tag.
> v5->v6:
>  * Replaced RCANFD_RNC_PER_REG macro with rnc_stride variable.
>  * Updated commit description
>  * Dropped Rb tag.
> v5:
>  * New patch.
> ---
>  drivers/net/can/rcar/rcar_canfd.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 32d700962d69..0001c8043c25 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -291,7 +291,7 @@
>  /* RSCFDnCFDGAFLECTR / RSCFDnGAFLECTR */
>  #define RCANFD_GAFLECTR			(0x0098)
>  /* RSCFDnCFDGAFLCFG / RSCFDnGAFLCFG */
> -#define RCANFD_GAFLCFG(ch)		(0x009c + (0x04 * ((ch) / 2)))
> +#define RCANFD_GAFLCFG(gpriv, ch)	(0x009c + (0x04 * ((ch) / (gpriv)->info->rnc_stride)))

I find it rather hard to follow the logic here. Your are multiplying by
four and then dividing again to get the good results. Here, I guess that
0x04 represents sizeof(u32), but this needs some thinking to figure that
out.

Wouldn't it be more intuitive to instead store the size in bytes of the
RNC value?

#define RCANFD_GAFLCFG(gpriv, ch) \
	(0x009c + (gpriv)->info->sizeof_rnc * (ch))

This way, no more 0x04 magic number and it is easier to process a
multiplication than a division in your head when reading the code.

>  /* RSCFDnCFDRMNB / RSCFDnRMNB */
>  #define RCANFD_RMNB			(0x00a4)
>  /* RSCFDnCFDRMND / RSCFDnRMND */
> @@ -505,6 +505,7 @@ struct rcar_canfd_global;
>  
>  struct rcar_canfd_hw_info {
>  	u16 num_supported_rules;
> +	u8 rnc_stride;
>  	u8 max_channels;
>  	u8 postdiv;
>  	/* hardware features */
> @@ -582,6 +583,7 @@ static const struct can_bittiming_const rcar_canfd_bittiming_const = {
>  
>  static const struct rcar_canfd_hw_info rcar_gen3_hw_info = {
>  	.num_supported_rules = 256,
> +	.rnc_stride = 4,
>  	.max_channels = 2,
>  	.postdiv = 2,
>  	.shared_global_irqs = 1,
> @@ -589,6 +591,7 @@ static const struct rcar_canfd_hw_info rcar_gen3_hw_info = {
>  
>  static const struct rcar_canfd_hw_info rcar_gen4_hw_info = {
>  	.num_supported_rules = 512,
> +	.rnc_stride = 2,
>  	.max_channels = 8,
>  	.postdiv = 2,
>  	.shared_global_irqs = 1,
> @@ -596,6 +599,7 @@ static const struct rcar_canfd_hw_info rcar_gen4_hw_info = {
>  
>  static const struct rcar_canfd_hw_info rzg2l_hw_info = {
>  	.num_supported_rules = 256,
> +	.rnc_stride = 4,
>  	.max_channels = 2,
>  	.postdiv = 1,
>  	.multi_channel_irqs = 1,
> @@ -797,7 +801,7 @@ static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
>  			    RCANFD_GAFLECTR_AFLDAE));
>  
>  	/* Write number of rules for channel */
> -	rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG(ch),
> +	rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG(gpriv, ch),
>  			   RCANFD_GAFLCFG_SETRNC(gpriv, ch, num_rules));
>  	if (is_gen4(gpriv))
>  		offset = RCANFD_GEN4_GAFL_OFFSET;


Yours sincerely,
Vincent Mailhol

Geert Uytterhoeven March 28, 2025, 10:45 a.m. UTC | #2

Hi Vincent,

On Fri, 28 Mar 2025 at 11:36, Vincent Mailhol
<[e-mail]> wrote:
> On 26/03/2025 at 21:19, Biju Das wrote:
> > R-Car Gen4 packs 2 RNC values in a 32-bit word, whereas R-Car Gen3 packs
> > up to 4 values in a 32-bit word. Handle this difference by adding
> > rnc_stride variable to struct rcar_canfd_hw_info and update the macro
> > RCANFD_GAFLCFG.
> >
> > Reviewed-by: Geert Uytterhoeven <[e-mail]>
> > Signed-off-by: Biju Das <[e-mail]>
> > ---
> > v6->v7:
> >  * Collected tag.
> > v5->v6:
> >  * Replaced RCANFD_RNC_PER_REG macro with rnc_stride variable.
> >  * Updated commit description
> >  * Dropped Rb tag.
> > v5:
> >  * New patch.
> > ---
> >  drivers/net/can/rcar/rcar_canfd.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> > index 32d700962d69..0001c8043c25 100644
> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -291,7 +291,7 @@
> >  /* RSCFDnCFDGAFLECTR / RSCFDnGAFLECTR */
> >  #define RCANFD_GAFLECTR                      (0x0098)
> >  /* RSCFDnCFDGAFLCFG / RSCFDnGAFLCFG */
> > -#define RCANFD_GAFLCFG(ch)           (0x009c + (0x04 * ((ch) / 2)))
> > +#define RCANFD_GAFLCFG(gpriv, ch)    (0x009c + (0x04 * ((ch) / (gpriv)->info->rnc_stride)))
>
> I find it rather hard to follow the logic here. Your are multiplying by
> four and then dividing again to get the good results. Here, I guess that
> 0x04 represents sizeof(u32), but this needs some thinking to figure that
> out.

The division is done first...

> Wouldn't it be more intuitive to instead store the size in bytes of the
> RNC value?
>
> #define RCANFD_GAFLCFG(gpriv, ch) \
>         (0x009c + (gpriv)->info->sizeof_rnc * (ch))
>
> This way, no more 0x04 magic number and it is easier to process a
> multiplication than a division in your head when reading the code.

... so this is not equivalent.

Gr{oetje,eeting}s,

                        Geert

Biju Das March 28, 2025, 3:39 p.m. UTC | #3

Hi Vincent,

Thanks for the feedback.

> -----Original Message-----
> From: Vincent Mailhol <[e-mail]>
> Sent: 28 March 2025 10:37
> Subject: Re: [PATCH v7 07/18] can: rcar_canfd: Add rnc_stride variable to struct rcar_canfd_hw_info
> 
> On 26/03/2025 at 21:19, Biju Das wrote:
> > R-Car Gen4 packs 2 RNC values in a 32-bit word, whereas R-Car Gen3
> > packs up to 4 values in a 32-bit word. Handle this difference by
> > adding rnc_stride variable to struct rcar_canfd_hw_info and update the
> > macro RCANFD_GAFLCFG.
> >
> > Reviewed-by: Geert Uytterhoeven <[e-mail]>
> > Signed-off-by: Biju Das <[e-mail]>
> > ---
> > v6->v7:
> >  * Collected tag.
> > v5->v6:
> >  * Replaced RCANFD_RNC_PER_REG macro with rnc_stride variable.
> >  * Updated commit description
> >  * Dropped Rb tag.
> > v5:
> >  * New patch.
> > ---
> >  drivers/net/can/rcar/rcar_canfd.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/can/rcar/rcar_canfd.c
> > b/drivers/net/can/rcar/rcar_canfd.c
> > index 32d700962d69..0001c8043c25 100644
> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -291,7 +291,7 @@
> >  /* RSCFDnCFDGAFLECTR / RSCFDnGAFLECTR */
> >  #define RCANFD_GAFLECTR			(0x0098)
> >  /* RSCFDnCFDGAFLCFG / RSCFDnGAFLCFG */
> > -#define RCANFD_GAFLCFG(ch)		(0x009c + (0x04 * ((ch) / 2)))
> > +#define RCANFD_GAFLCFG(gpriv, ch)	(0x009c + (0x04 * ((ch) / (gpriv)->info->rnc_stride)))
> 
> I find it rather hard to follow the logic here. Your are multiplying by four and then dividing again
> to get the good results. Here, I guess that
> 0x04 represents sizeof(u32), but this needs some thinking to figure that out.
> 
> Wouldn't it be more intuitive to instead store the size in bytes of the RNC value?
> 
> #define RCANFD_GAFLCFG(gpriv, ch) \
> 	(0x009c + (gpriv)->info->sizeof_rnc * (ch))
> 
> This way, no more 0x04 magic number and it is easier to process a multiplication than a division in
> your head when reading the code.

Now the macro simplified as after introducing setrnc() [1]
#define RCANFD_GAFLCFG(w)		(0x009c + (0x04 * (w)))

Where "w" is the index mentioned in the hardware manual.

> 
> >  /* RSCFDnCFDRMNB / RSCFDnRMNB */
> >  #define RCANFD_RMNB			(0x00a4)
> >  /* RSCFDnCFDRMND / RSCFDnRMND */
> > @@ -505,6 +505,7 @@ struct rcar_canfd_global;
> >
> >  struct rcar_canfd_hw_info {
> >  	u16 num_supported_rules;
> > +	u8 rnc_stride;
> >  	u8 max_channels;
> >  	u8 postdiv;
> >  	/* hardware features */
> > @@ -582,6 +583,7 @@ static const struct can_bittiming_const
> > rcar_canfd_bittiming_const = {
> >
> >  static const struct rcar_canfd_hw_info rcar_gen3_hw_info = {
> >  	.num_supported_rules = 256,
> > +	.rnc_stride = 4,
> >  	.max_channels = 2,
> >  	.postdiv = 2,
> >  	.shared_global_irqs = 1,
> > @@ -589,6 +591,7 @@ static const struct rcar_canfd_hw_info
> > rcar_gen3_hw_info = {
> >
> >  static const struct rcar_canfd_hw_info rcar_gen4_hw_info = {
> >  	.num_supported_rules = 512,
> > +	.rnc_stride = 2,
> >  	.max_channels = 8,
> >  	.postdiv = 2,
> >  	.shared_global_irqs = 1,
> > @@ -596,6 +599,7 @@ static const struct rcar_canfd_hw_info
> > rcar_gen4_hw_info = {
> >
> >  static const struct rcar_canfd_hw_info rzg2l_hw_info = {
> >  	.num_supported_rules = 256,
> > +	.rnc_stride = 4,
> >  	.max_channels = 2,
> >  	.postdiv = 1,
> >  	.multi_channel_irqs = 1,
> > @@ -797,7 +801,7 @@ static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
> >  			    RCANFD_GAFLECTR_AFLDAE));
> >
> >  	/* Write number of rules for channel */
> > -	rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG(ch),
> > +	rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG(gpriv, ch),
> >  			   RCANFD_GAFLCFG_SETRNC(gpriv, ch, num_rules));

By introducing setrnc(), this patch is no more needed as
rnc_stride is local variable inside strnc(). So I would like to drop this
patch in next version.

[1]
static void rcar_canfd_setrnc(struct rcar_canfd_global *gpriv, u32 ch,
			      int num_rules)
{
	u32 shift, rnc, offset, w, rnc_stride;

	rnc_stride = 32 / gpriv->info->rnc_field_width;
	shift = 32 - ((ch % rnc_stride + 1) * gpriv->info->rnc_field_width);
	rnc = (num_rules & (gpriv->info->num_supported_rules - 1)) << shift;
	w = ch / rnc_stride;
	offset = RCANFD_GAFLCFG(w);
	rcar_canfd_set_bit(gpriv->base, offset, rnc);
}


Cheers,
Biju

Vincent Mailhol March 28, 2025, 3:51 p.m. UTC | #4

On 29/03/2025 at 00:39, Biju Das wrote:
> Hi Vincent,
> 
> Thanks for the feedback.
> 
>> -----Original Message-----
>> From: Vincent Mailhol <[e-mail]>
>> Sent: 28 March 2025 10:37
>> Subject: Re: [PATCH v7 07/18] can: rcar_canfd: Add rnc_stride variable to struct rcar_canfd_hw_info
>>
>> On 26/03/2025 at 21:19, Biju Das wrote:
>>> R-Car Gen4 packs 2 RNC values in a 32-bit word, whereas R-Car Gen3
>>> packs up to 4 values in a 32-bit word. Handle this difference by
>>> adding rnc_stride variable to struct rcar_canfd_hw_info and update the
>>> macro RCANFD_GAFLCFG.
>>>
>>> Reviewed-by: Geert Uytterhoeven <[e-mail]>
>>> Signed-off-by: Biju Das <[e-mail]>
>>> ---
>>> v6->v7:
>>>  * Collected tag.
>>> v5->v6:
>>>  * Replaced RCANFD_RNC_PER_REG macro with rnc_stride variable.
>>>  * Updated commit description
>>>  * Dropped Rb tag.
>>> v5:
>>>  * New patch.
>>> ---
>>>  drivers/net/can/rcar/rcar_canfd.c | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/can/rcar/rcar_canfd.c
>>> b/drivers/net/can/rcar/rcar_canfd.c
>>> index 32d700962d69..0001c8043c25 100644
>>> --- a/drivers/net/can/rcar/rcar_canfd.c
>>> +++ b/drivers/net/can/rcar/rcar_canfd.c
>>> @@ -291,7 +291,7 @@
>>>  /* RSCFDnCFDGAFLECTR / RSCFDnGAFLECTR */
>>>  #define RCANFD_GAFLECTR			(0x0098)
>>>  /* RSCFDnCFDGAFLCFG / RSCFDnGAFLCFG */
>>> -#define RCANFD_GAFLCFG(ch)		(0x009c + (0x04 * ((ch) / 2)))
>>> +#define RCANFD_GAFLCFG(gpriv, ch)	(0x009c + (0x04 * ((ch) / (gpriv)->info->rnc_stride)))
>>
>> I find it rather hard to follow the logic here. Your are multiplying by four and then dividing again
>> to get the good results. Here, I guess that
>> 0x04 represents sizeof(u32), but this needs some thinking to figure that out.
>>
>> Wouldn't it be more intuitive to instead store the size in bytes of the RNC value?
>>
>> #define RCANFD_GAFLCFG(gpriv, ch) \
>> 	(0x009c + (gpriv)->info->sizeof_rnc * (ch))
>>
>> This way, no more 0x04 magic number and it is easier to process a multiplication than a division in
>> your head when reading the code.
> 
> Now the macro simplified as after introducing setrnc() [1]
> #define RCANFD_GAFLCFG(w)		(0x009c + (0x04 * (w)))
> 
> Where "w" is the index mentioned in the hardware manual.
> 
>>
>>>  /* RSCFDnCFDRMNB / RSCFDnRMNB */
>>>  #define RCANFD_RMNB			(0x00a4)
>>>  /* RSCFDnCFDRMND / RSCFDnRMND */
>>> @@ -505,6 +505,7 @@ struct rcar_canfd_global;
>>>
>>>  struct rcar_canfd_hw_info {
>>>  	u16 num_supported_rules;
>>> +	u8 rnc_stride;
>>>  	u8 max_channels;
>>>  	u8 postdiv;
>>>  	/* hardware features */
>>> @@ -582,6 +583,7 @@ static const struct can_bittiming_const
>>> rcar_canfd_bittiming_const = {
>>>
>>>  static const struct rcar_canfd_hw_info rcar_gen3_hw_info = {
>>>  	.num_supported_rules = 256,
>>> +	.rnc_stride = 4,
>>>  	.max_channels = 2,
>>>  	.postdiv = 2,
>>>  	.shared_global_irqs = 1,
>>> @@ -589,6 +591,7 @@ static const struct rcar_canfd_hw_info
>>> rcar_gen3_hw_info = {
>>>
>>>  static const struct rcar_canfd_hw_info rcar_gen4_hw_info = {
>>>  	.num_supported_rules = 512,
>>> +	.rnc_stride = 2,
>>>  	.max_channels = 8,
>>>  	.postdiv = 2,
>>>  	.shared_global_irqs = 1,
>>> @@ -596,6 +599,7 @@ static const struct rcar_canfd_hw_info
>>> rcar_gen4_hw_info = {
>>>
>>>  static const struct rcar_canfd_hw_info rzg2l_hw_info = {
>>>  	.num_supported_rules = 256,
>>> +	.rnc_stride = 4,
>>>  	.max_channels = 2,
>>>  	.postdiv = 1,
>>>  	.multi_channel_irqs = 1,
>>> @@ -797,7 +801,7 @@ static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
>>>  			    RCANFD_GAFLECTR_AFLDAE));
>>>
>>>  	/* Write number of rules for channel */
>>> -	rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG(ch),
>>> +	rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG(gpriv, ch),
>>>  			   RCANFD_GAFLCFG_SETRNC(gpriv, ch, num_rules));
> 
> By introducing setrnc(), this patch is no more needed as
> rnc_stride is local variable inside strnc(). So I would like to drop this
> patch in next version.
> 
> [1]
> static void rcar_canfd_setrnc(struct rcar_canfd_global *gpriv, u32 ch,
> 			      int num_rules)
> {
> 	u32 shift, rnc, offset, w, rnc_stride;
> 
> 	rnc_stride = 32 / gpriv->info->rnc_field_width;
> 	shift = 32 - ((ch % rnc_stride + 1) * gpriv->info->rnc_field_width);
> 	rnc = (num_rules & (gpriv->info->num_supported_rules - 1)) << shift;
> 	w = ch / rnc_stride;
> 	offset = RCANFD_GAFLCFG(w);
> 	rcar_canfd_set_bit(gpriv->base, offset, rnc);
> }

Yes, yes, yes!

See, that's way better that the previous macros!

No more parenthesis madness, the intermediate variables get a proper
name, you do not need so separately store the rnc_field_width and the
rnc_stride. All good.

Sorry for the poor feedback earlier on, but I am glad that I insisted
now that I see the nice improvement.


Yours sincerely,
Vincent Mailhol

Geert Uytterhoeven March 28, 2025, 4:03 p.m. UTC | #5

Hi Biju,

On Fri, 28 Mar 2025 at 16:39, Biju Das <[e-mail]> wrote:
> By introducing setrnc(), this patch is no more needed as
> rnc_stride is local variable inside strnc(). So I would like to drop this
> patch in next version.
>
> [1]
> static void rcar_canfd_setrnc(struct rcar_canfd_global *gpriv, u32 ch,
>                               int num_rules)
> {
>         u32 shift, rnc, offset, w, rnc_stride;
>
>         rnc_stride = 32 / gpriv->info->rnc_field_width;
>         shift = 32 - ((ch % rnc_stride + 1) * gpriv->info->rnc_field_width);
>         rnc = (num_rules & (gpriv->info->num_supported_rules - 1)) << shift;
>         w = ch / rnc_stride;
>         offset = RCANFD_GAFLCFG(w);
>         rcar_canfd_set_bit(gpriv->base, offset, rnc);
> }

Nice!

Please combine variable declaration and assignment.
While at it, perhaps "unsigned int ch" and "unsigned int num_rules"?
Actually about everything above should be unsigned int, except rnc.

I know this existed before, but do we need

    num_rules & (gpriv->info->num_supported_rules - 1)

? That "&" only works as long as num_supported_rules is a power of two,
and I think num_rules can never be larger than num_supported_rules anyway.

Gr{oetje,eeting}s,

                        Geert

Biju Das March 28, 2025, 4:46 p.m. UTC | #6

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <[e-mail]>
> Sent: 28 March 2025 16:03
> Subject: Re: [PATCH v7 07/18] can: rcar_canfd: Add rnc_stride variable to struct rcar_canfd_hw_info
> 
> Hi Biju,
> 
> On Fri, 28 Mar 2025 at 16:39, Biju Das <[e-mail]> wrote:
> > By introducing setrnc(), this patch is no more needed as rnc_stride is
> > local variable inside strnc(). So I would like to drop this patch in
> > next version.
> >
> > [1]
> > static void rcar_canfd_setrnc(struct rcar_canfd_global *gpriv, u32 ch,
> >                               int num_rules) {
> >         u32 shift, rnc, offset, w, rnc_stride;
> >
> >         rnc_stride = 32 / gpriv->info->rnc_field_width;
> >         shift = 32 - ((ch % rnc_stride + 1) * gpriv->info->rnc_field_width);
> >         rnc = (num_rules & (gpriv->info->num_supported_rules - 1)) << shift;
> >         w = ch / rnc_stride;
> >         offset = RCANFD_GAFLCFG(w);
> >         rcar_canfd_set_bit(gpriv->base, offset, rnc); }
> 
> Nice!
> 
> Please combine variable declaration and assignment.
> While at it, perhaps "unsigned int ch" and "unsigned int num_rules"?
> Actually about everything above should be unsigned int, except rnc.
> 
> I know this existed before, but do we need
> 
>     num_rules & (gpriv->info->num_supported_rules - 1)
> 
> ? That "&" only works as long as num_supported_rules is a power of two, and I think num_rules can
> never be larger than num_supported_rules anyway.

But this will make sure it fits into field_width for num_rules.

You mean drop "num_supported_rules" and use mum_rules directly ??

rnc = num_rules << shift;

Cheers,
Biju

Geert Uytterhoeven March 28, 2025, 5:11 p.m. UTC | #7

Hi Biju,

On Fri, 28 Mar 2025 at 17:46, Biju Das <[e-mail]> wrote:
> > From: Geert Uytterhoeven <[e-mail]>
> > On Fri, 28 Mar 2025 at 16:39, Biju Das <[e-mail]> wrote:
> > > By introducing setrnc(), this patch is no more needed as rnc_stride is
> > > local variable inside strnc(). So I would like to drop this patch in
> > > next version.
> > >
> > > [1]
> > > static void rcar_canfd_setrnc(struct rcar_canfd_global *gpriv, u32 ch,
> > >                               int num_rules) {
> > >         u32 shift, rnc, offset, w, rnc_stride;
> > >
> > >         rnc_stride = 32 / gpriv->info->rnc_field_width;
> > >         shift = 32 - ((ch % rnc_stride + 1) * gpriv->info->rnc_field_width);
> > >         rnc = (num_rules & (gpriv->info->num_supported_rules - 1)) << shift;
> > >         w = ch / rnc_stride;
> > >         offset = RCANFD_GAFLCFG(w);
> > >         rcar_canfd_set_bit(gpriv->base, offset, rnc); }
> >
> > Nice!
> >
> > Please combine variable declaration and assignment.
> > While at it, perhaps "unsigned int ch" and "unsigned int num_rules"?
> > Actually about everything above should be unsigned int, except rnc.
> >
> > I know this existed before, but do we need
> >
> >     num_rules & (gpriv->info->num_supported_rules - 1)
> >
> > ? That "&" only works as long as num_supported_rules is a power of two, and I think num_rules can
> > never be larger than num_supported_rules anyway.
>
> But this will make sure it fits into field_width for num_rules.

Can it ever not fit?

> You mean drop "num_supported_rules" and use mum_rules directly ??
>
> rnc = num_rules << shift;

Exactly.

Gr{oetje,eeting}s,

                        Geert

Biju Das March 28, 2025, 5:14 p.m. UTC | #8

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <[e-mail]>
> Sent: 28 March 2025 17:11
> Subject: Re: [PATCH v7 07/18] can: rcar_canfd: Add rnc_stride variable to struct rcar_canfd_hw_info
> 
> Hi Biju,
> 
> On Fri, 28 Mar 2025 at 17:46, Biju Das <[e-mail]> wrote:
> > > From: Geert Uytterhoeven <[e-mail]> On Fri, 28 Mar 2025
> > > at 16:39, Biju Das <[e-mail]> wrote:
> > > > By introducing setrnc(), this patch is no more needed as
> > > > rnc_stride is local variable inside strnc(). So I would like to
> > > > drop this patch in next version.
> > > >
> > > > [1]
> > > > static void rcar_canfd_setrnc(struct rcar_canfd_global *gpriv, u32 ch,
> > > >                               int num_rules) {
> > > >         u32 shift, rnc, offset, w, rnc_stride;
> > > >
> > > >         rnc_stride = 32 / gpriv->info->rnc_field_width;
> > > >         shift = 32 - ((ch % rnc_stride + 1) * gpriv->info->rnc_field_width);
> > > >         rnc = (num_rules & (gpriv->info->num_supported_rules - 1)) << shift;
> > > >         w = ch / rnc_stride;
> > > >         offset = RCANFD_GAFLCFG(w);
> > > >         rcar_canfd_set_bit(gpriv->base, offset, rnc); }
> > >
> > > Nice!
> > >
> > > Please combine variable declaration and assignment.
> > > While at it, perhaps "unsigned int ch" and "unsigned int num_rules"?
> > > Actually about everything above should be unsigned int, except rnc.
> > >
> > > I know this existed before, but do we need
> > >
> > >     num_rules & (gpriv->info->num_supported_rules - 1)
> > >
> > > ? That "&" only works as long as num_supported_rules is a power of
> > > two, and I think num_rules can never be larger than num_supported_rules anyway.
> >
> > But this will make sure it fits into field_width for num_rules.
> 
> Can it ever not fit?

No. ATM, we set num_rules = 1. So, it is safe to drop "num_supported_rules".

Cheers,
Biju

Patch

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 32d700962d69..0001c8043c25 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -291,7 +291,7 @@ 
 /* RSCFDnCFDGAFLECTR / RSCFDnGAFLECTR */
 #define RCANFD_GAFLECTR			(0x0098)
 /* RSCFDnCFDGAFLCFG / RSCFDnGAFLCFG */
-#define RCANFD_GAFLCFG(ch)		(0x009c + (0x04 * ((ch) / 2)))
+#define RCANFD_GAFLCFG(gpriv, ch)	(0x009c + (0x04 * ((ch) / (gpriv)->info->rnc_stride)))
 /* RSCFDnCFDRMNB / RSCFDnRMNB */
 #define RCANFD_RMNB			(0x00a4)
 /* RSCFDnCFDRMND / RSCFDnRMND */
@@ -505,6 +505,7 @@  struct rcar_canfd_global;
 
 struct rcar_canfd_hw_info {
 	u16 num_supported_rules;
+	u8 rnc_stride;
 	u8 max_channels;
 	u8 postdiv;
 	/* hardware features */
@@ -582,6 +583,7 @@  static const struct can_bittiming_const rcar_canfd_bittiming_const = {
 
 static const struct rcar_canfd_hw_info rcar_gen3_hw_info = {
 	.num_supported_rules = 256,
+	.rnc_stride = 4,
 	.max_channels = 2,
 	.postdiv = 2,
 	.shared_global_irqs = 1,
@@ -589,6 +591,7 @@  static const struct rcar_canfd_hw_info rcar_gen3_hw_info = {
 
 static const struct rcar_canfd_hw_info rcar_gen4_hw_info = {
 	.num_supported_rules = 512,
+	.rnc_stride = 2,
 	.max_channels = 8,
 	.postdiv = 2,
 	.shared_global_irqs = 1,
@@ -596,6 +599,7 @@  static const struct rcar_canfd_hw_info rcar_gen4_hw_info = {
 
 static const struct rcar_canfd_hw_info rzg2l_hw_info = {
 	.num_supported_rules = 256,
+	.rnc_stride = 4,
 	.max_channels = 2,
 	.postdiv = 1,
 	.multi_channel_irqs = 1,
@@ -797,7 +801,7 @@  static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
 			    RCANFD_GAFLECTR_AFLDAE));
 
 	/* Write number of rules for channel */
-	rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG(ch),
+	rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG(gpriv, ch),
 			   RCANFD_GAFLCFG_SETRNC(gpriv, ch, num_rules));
 	if (is_gen4(gpriv))
 		offset = RCANFD_GEN4_GAFL_OFFSET;