Hi Vincent,
> -----Original Message-----
> From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> Sent: 28 March 2025 15:26
> Subject: Re: [PATCH v7 15/18] can: rcar_canfd: Add shift table to struct rcar_canfd_hw_info
>
> On 26/03/2025 at 21:19, Biju Das wrote:
> > R-Car Gen3 and Gen4 have some differences in the shift bits. Add a
> > shift table to handle these differences. After this drop the unused
> > functions reg_gen4() and is_gen4().
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v6->v7:
> > * No change.
> > v5->v6:
> > * No change.
> > v4->v5:
> > * Collected tag.
> > * Dropped RCANFD_FIRST_RNC_SH and RCANFD_SECOND_RNC_SH by using a
> > formula (32 - (n % rnc_per_reg + 1) * rnc_field_width.
> > v3->v4:
> > * Added prefix RCANFD_* to enum rcar_canfd_shift_id.
> > v3:
> > * New patch.
> > ---
> > drivers/net/can/rcar/rcar_canfd.c | 69
> > ++++++++++++++++++++++---------
> > 1 file changed, 50 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/net/can/rcar/rcar_canfd.c
> > b/drivers/net/can/rcar/rcar_canfd.c
> > index a96cf499f04b..20e591421cc6 100644
> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -111,13 +111,16 @@
> >
> > /* RSCFDnCFDCmNCFG - CAN FD only */
> > #define RCANFD_NCFG_NTSEG2(gpriv, x) \
> > - (((x) & ((gpriv)->info->nom_bittiming->tseg2_max - 1)) << reg_gen4(gpriv, 25, 24))
> > + (((x) & ((gpriv)->info->nom_bittiming->tseg2_max - 1)) << \
> > + (gpriv)->info->shift_table[RCANFD_NTSEG2_SH])
> >
> > #define RCANFD_NCFG_NTSEG1(gpriv, x) \
> > - (((x) & ((gpriv)->info->nom_bittiming->tseg1_max - 1)) << reg_gen4(gpriv, 17, 16))
> > + (((x) & ((gpriv)->info->nom_bittiming->tseg1_max - 1)) << \
> > + (gpriv)->info->shift_table[RCANFD_NTSEG1_SH])
> >
> > #define RCANFD_NCFG_NSJW(gpriv, x) \
> > - (((x) & ((gpriv)->info->nom_bittiming->sjw_max - 1)) << reg_gen4(gpriv, 10, 11))
> > + (((x) & ((gpriv)->info->nom_bittiming->sjw_max - 1)) << \
> > + (gpriv)->info->shift_table[RCANFD_NSJW_SH])
> >
> > #define RCANFD_NCFG_NBRP(x) (((x) & 0x3ff) << 0)
> >
> > @@ -182,10 +185,12 @@
> > #define RCANFD_DCFG_DSJW(gpriv, x) (((x) & ((gpriv)->info->data_bittiming->sjw_max - 1)) << 24)
> >
> > #define RCANFD_DCFG_DTSEG2(gpriv, x) \
> > - (((x) & ((gpriv)->info->data_bittiming->tseg2_max - 1)) << reg_gen4(gpriv, 16, 20))
> > + (((x) & ((gpriv)->info->data_bittiming->tseg2_max - 1)) << \
> > + (gpriv)->info->shift_table[RCANFD_DTSEG2_SH])
> >
> > #define RCANFD_DCFG_DTSEG1(gpriv, x) \
> > - (((x) & ((gpriv)->info->data_bittiming->tseg1_max - 1)) << reg_gen4(gpriv, 8, 16))
> > + (((x) & ((gpriv)->info->data_bittiming->tseg1_max - 1)) << \
> > + (gpriv)->info->shift_table[RCANFD_DTSEG1_SH])
> >
> > #define RCANFD_DCFG_DBRP(x) (((x) & 0xff) << 0)
> >
> > @@ -227,10 +232,10 @@
> >
> > /* RSCFDnCFDCFCCk */
> > #define RCANFD_CFCC_CFTML(gpriv, x) \
> > - (((x) & (gpriv)->info->max_cftml) << reg_gen4(gpriv, 16, 20))
> > -#define RCANFD_CFCC_CFM(gpriv, x) (((x) & 0x3) << reg_gen4(gpriv, 8, 16))
> > + (((x) & (gpriv)->info->max_cftml) << (gpriv)->info->shift_table[RCANFD_CFTML_SH])
> > +#define RCANFD_CFCC_CFM(gpriv, x) (((x) & 0x3) << (gpriv)->info->shift_table[RCANFD_CFM_SH])
> > #define RCANFD_CFCC_CFIM BIT(12)
> > -#define RCANFD_CFCC_CFDC(gpriv, x) (((x) & 0x7) << reg_gen4(gpriv, 21, 8))
> > +#define RCANFD_CFCC_CFDC(gpriv, x) (((x) & 0x7) << (gpriv)->info->shift_table[RCANFD_CFDC_SH])
> > #define RCANFD_CFCC_CFPLS(x) (((x) & 0x7) << 4)
> > #define RCANFD_CFCC_CFTXIE BIT(2)
> > #define RCANFD_CFCC_CFE BIT(0)
> > @@ -511,12 +516,24 @@ enum rcar_canfd_reg_offset_id {
> > RCANFD_CFOFFSET, /* Transmit/receive FIFO buffer access ID register */
> > };
> >
> > +enum rcar_canfd_shift_id {
> > + RCANFD_NTSEG2_SH, /* Nominal Bit Rate Time Segment 2 Control */
> > + RCANFD_NTSEG1_SH, /* Nominal Bit Rate Time Segment 1 Control */
> > + RCANFD_NSJW_SH, /* Nominal Bit Rate Resynchronization Jump Width Control */
> > + RCANFD_DTSEG2_SH, /* Data Bit Rate Time Segment 2 Control */
> > + RCANFD_DTSEG1_SH, /* Data Bit Rate Time Segment 1 Control */
> > + RCANFD_CFTML_SH, /* Common FIFO TX Message Buffer Link */
> > + RCANFD_CFM_SH, /* Common FIFO Mode */
> > + RCANFD_CFDC_SH, /* Common FIFO Depth Configuration */
> > +};
> > +
> > struct rcar_canfd_global;
> >
> > struct rcar_canfd_hw_info {
> > const struct can_bittiming_const *nom_bittiming;
> > const struct can_bittiming_const *data_bittiming;
> > const u16 *regs;
> > + const u8 *shift_table;
> > u16 num_supported_rules;
> > u8 rnc_stride;
> > u8 rnc_field_width;
> > @@ -643,10 +660,33 @@ static const u16 rcar_gen4_regs[] = {
> > [RCANFD_CFOFFSET] = 0x6400,
> > };
> >
> > +static const u8 rcar_gen3_shift_table[] = {
> > + [RCANFD_NTSEG2_SH] = 24,
> > + [RCANFD_NTSEG1_SH] = 16,
> > + [RCANFD_NSJW_SH] = 11,
> > + [RCANFD_DTSEG2_SH] = 20,
> > + [RCANFD_DTSEG1_SH] = 16,
> > + [RCANFD_CFTML_SH] = 20,
> > + [RCANFD_CFM_SH] = 16,
> > + [RCANFD_CFDC_SH] = 8,
> > +};
> > +
> > +static const u8 rcar_gen4_shift_table[] = {
> > + [RCANFD_NTSEG2_SH] = 25,
> > + [RCANFD_NTSEG1_SH] = 17,
> > + [RCANFD_NSJW_SH] = 10,
> > + [RCANFD_DTSEG2_SH] = 16,
> > + [RCANFD_DTSEG1_SH] = 8,
> > + [RCANFD_CFTML_SH] = 16,
> > + [RCANFD_CFM_SH] = 8,
> > + [RCANFD_CFDC_SH] = 21,
> > +};
>
> Exact same comment as previous patch. A structure looks more appropriate than an array here.
Agreed.
Cheers,
Biju
