On 28/03/2025 at 19:54, Biju Das wrote: > 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 OK. Thanks. I didn't parse the parenthesis correctly. It's been a long week, sorry for the noise. >> 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) This is before adding the tons of parenthesis. I did my review of best effort and got it wrong not because the calculation is complication but because of the noise introduced by those parenthesis (plus admittedly some fatigue from the long week…) >> 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. The maintainers of the CAN drivers and Marc and myself. Of course, I do appreciate Geert feedback here. But for doing this maintenance as a volunteer, in exchange, I want you to make my review work (and the long term maintenance) easier. function-like macro are not welcome unless strictly needed and I am not keen on dropping this ask. Yours sincerely, Vincent Mailhol
