| Message ID | 20250326122003.122976-9-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 |
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@glider.be> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > 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 % (2 + 1)) * 16) = 32 - ((n % 3) * 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? More generally, it is really hard to review and understand what this macro does. 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. > /* RSCFDnCFDGAFLECTR / RSCFDnGAFLECTR */ > #define RCANFD_GAFLECTR_AFLDAE BIT(8) > @@ -506,6 +506,7 @@ struct rcar_canfd_global; > struct rcar_canfd_hw_info { > u16 num_supported_rules; > u8 rnc_stride; > + u8 rnc_field_width; Following my previous comment on patch #7, if you replace rcar_canfd_hw_info->rnc_stride by sizeof_rnc, then you can get the width with: rcar_canfd_hw_info->sizeof_rnc * BITS_PER_BYTE > u8 max_channels; > u8 postdiv; > /* hardware features */ > @@ -584,6 +585,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, > + .rnc_field_width = 8, > .max_channels = 2, > .postdiv = 2, > .shared_global_irqs = 1, > @@ -592,6 +594,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, > + .rnc_field_width = 16, > .max_channels = 8, > .postdiv = 2, > .shared_global_irqs = 1, > @@ -600,6 +603,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, > + .rnc_field_width = 8, > .max_channels = 2, > .postdiv = 1, > .multi_channel_irqs = 1, Yours sincerely, Vincent Mailhol
Hi Vincent, On Fri, 28 Mar 2025 at 11:39, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > 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@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 > > @@ -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 16 > 1 16 - ((1 & 1) * 16) = 16 0 > 2 16 - ((2 & 1) * 16) = 0 16 > 3 16 - ((3 & 1) * 16) = 16 0 > 4 16 - ((4 & 1) * 16) = 0 16 > > After: > > (32 - ((n % rnc_stride + 1)) * rnc_field_width) = > 32 - ((n % (2 + 1)) * 16) = You got the operator precedence wrong: should be (n % 2) + 1 instead of n % (2 + 1). > 32 - ((n % 3) * 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? > > More generally, it is really hard to review and understand what this > macro does. It prepares a value for storing in a 16-bit or 8-bit field of a 32-bit register, where the fields are numbered MSB-first. Gr{oetje,eeting}s, Geert
Hi Vincent, > -----Original Message----- > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > 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@glider.be> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > 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
On 28/03/2025 at 19:54, Biju Das wrote: > Hi Vincent, > >> -----Original Message----- >> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> >> 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@glider.be> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>> --- >>> 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
Hi Vincent, Thanks for the feedback. > -----Original Message----- > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > Sent: 28 March 2025 11:21 > Subject: Re: [PATCH v7 08/18] can: rcar_canfd: Simplify RCANFD_GAFLCFG_SETRNC macro > > On 28/03/2025 at 19:54, Biju Das wrote: > > Hi Vincent, > > > >> -----Original Message----- > >> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > >> 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@glider.be> > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >>> --- > >>> 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…) I agree, too much parenthesis in macro is a noise. > > >> 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. OK, will convert this into a function. Cheers, Biju
Hi Vincent, On Fri, 28 Mar 2025 at 12:21, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > On 28/03/2025 at 19:54, Biju Das wrote: > >> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > >> 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@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 > >>> @@ -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. I agree the heavy use of macros can be improved. E.g. handling the access to the RNC field is spread across two macros: - RCANFD_GAFLCFG_SETRNC() handles ch % something, - RCANFD_GAFLCFG() handles ch / something. A single function to write to the RNC field would make this more readable. This is just an example, there are many more of these... Gr{oetje,eeting}s, Geert
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))) /* RSCFDnCFDGAFLECTR / RSCFDnGAFLECTR */ #define RCANFD_GAFLECTR_AFLDAE BIT(8) @@ -506,6 +506,7 @@ struct rcar_canfd_global; struct rcar_canfd_hw_info { u16 num_supported_rules; u8 rnc_stride; + u8 rnc_field_width; u8 max_channels; u8 postdiv; /* hardware features */ @@ -584,6 +585,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, + .rnc_field_width = 8, .max_channels = 2, .postdiv = 2, .shared_global_irqs = 1, @@ -592,6 +594,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, + .rnc_field_width = 16, .max_channels = 8, .postdiv = 2, .shared_global_irqs = 1, @@ -600,6 +603,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, + .rnc_field_width = 8, .max_channels = 2, .postdiv = 1, .multi_channel_irqs = 1,
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@glider.be> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- 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(-)