On 29/03/2025 at 00:39, Biju Das wrote:
> Hi Vincent,
>
> Thanks for the feedback.
>
>> -----Original Message-----
>> From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
>> 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 <geert+renesas@xxxxxxxxx>
>>> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>>> ---
>>> 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
