Hi Vincent, > -----Original Message----- > From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > Sent: 28 March 2025 10:39 > Subject: Re: [PATCH v7 08/18] can: rcar_canfd: Simplify RCANFD_GAFLCFG_SETRNC macro > > On 26/03/2025 at 21:19, Biju Das wrote: > > The shift values in RCANFD_GAFLCFG_SETRNC are dictated by the field width: > > - R-Car Gen4 packs 2 values in a 32-bit word, using a field width > > of 16 bits, > > - R-Car Gen3 packs up to 4 values in a 32-bit word, using a field > > width of 8 bits. > > > > Add rnc_field_width variable to struct rcar_canfd_hw_info to handle > > this difference and simplify the shift value in RCANFD_GAFLCFG_SETRNC > > macro by using a formula (32 - (n % rnc_stride + 1) * rnc_field_width). > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> > > --- > > v6->7: > > * Collected tag. > > v5->6: > > * Replaced RCANFD_RNC_PER_REG macro with rnc_stride variable. > > * Updated commit description > > * Dropped the Rb tag. > > v5: > > * New patch. > > --- > > drivers/net/can/rcar/rcar_canfd.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/can/rcar/rcar_canfd.c > > b/drivers/net/can/rcar/rcar_canfd.c > > index 0001c8043c25..62cde1efa0c0 100644 > > --- a/drivers/net/can/rcar/rcar_canfd.c > > +++ b/drivers/net/can/rcar/rcar_canfd.c > > @@ -89,7 +89,7 @@ > > /* RSCFDnCFDGAFLCFG0 / RSCFDnGAFLCFG0 */ #define > > RCANFD_GAFLCFG_SETRNC(gpriv, n, x) \ > > (((x) & ((gpriv)->info->num_supported_rules - 1)) << \ > > - (reg_gen4(gpriv, 16, 24) - ((n) & 1) * reg_gen4(gpriv, 16, 8))) > > + (32 - (((n) % (gpriv)->info->rnc_stride + 1) * > > +(gpriv)->info->rnc_field_width))) > > I can not follow how this is the same. Let's take the gen4 as an example. Before: > > (reg_gen4(gpriv, 16, 24) - ((n) & 1) * reg_gen4(gpriv, 16, 8))) = > 16 - ((n & 1) * 16) > > So, I have: > > n shift value > --------------------------------- > 0 16 - ((0 & 1) * 16) = 0 > 1 16 - ((1 & 1) * 16) = 16 > 2 16 - ((2 & 1) * 16) = 0 > 3 16 - ((3 & 1) * 16) = 16 > 4 16 - ((4 & 1) * 16) = 0 > > After: > > (32 - ((n % rnc_stride + 1)) * rnc_field_width) = 32 - (n % rnc_stride) + 1 = > 32 - ((n % (2 + 1)) * 16) = > 32 - ((n % 3) * 16) 32 - ((n % 2) + 1)) * 16) = > > n shift value > --------------------------------- > 0 32 - ((0 % 3) * 16) = 32 > 1 32 - ((1 % 3) * 16) = 16 > 2 32 - ((2 % 3) * 16) = 0 > 3 32 - ((3 % 3) * 16) = 32 > 4 32 - ((4 % 3) * 16) = 16 > > Is there something wrong in my calculation? What am I missing? 0 32 - ((0 % 2) + 1) * 16) = 16 1 32 - ((1 % 2) + 1) * 16) = 0 > > > More generally, it is really hard to review and understand what this macro does. Macro is doing a simple calculation. (32 - (n % rnc_stride + 1) * rnc_field_width) > > Can add one more patch: > > can: rcar_canfd: turn RCANFD_GAFLCFG_SETRNC into a function > > and then apply your change? > > I do not see the reason why this needs to be a macro. If you make this a function, at least, it will > be easier to follow what is going on and the compiler optimizer will inline it anyway so you should > not get any penalty. I am leaving Marc, Geert to provide their feedback on this. Cheers, Biju
