Hi Vincent,
On Fri, 28 Mar 2025 at 11:39, Vincent Mailhol
<mailhol.vincent@xxxxxxxxxx> 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@xxxxxxxxx>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > --- 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
