Hi Vincent,
Thanks for the feedback.
> -----Original Message-----
> From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> Sent: 28 March 2025 14:19
> Subject: Re: [PATCH v7 14/18] can: rcar_canfd: Add register mapping 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 register offsets. Add
> > a mapping table to handle these differences.
> >
> > 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:
> > * Improved commit description by replacing has->have.
> > * Collected tag.
> > v3->v4:
> > * Added prefix RCANFD_* to enum rcar_canfd_reg_offset_id.
> > v3:
> > * New patch.
> > ---
> > drivers/net/can/rcar/rcar_canfd.c | 48
> > ++++++++++++++++++++++++++-----
> > 1 file changed, 41 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/can/rcar/rcar_canfd.c
> > b/drivers/net/can/rcar/rcar_canfd.c
> > index 360999e6ab45..a96cf499f04b 100644
> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -298,7 +298,7 @@
> > #define RCANFD_RMND(y) (0x00a8 + (0x04 * (y)))
> >
> > /* RSCFDnCFDRFCCx / RSCFDnRFCCx */
> > -#define RCANFD_RFCC(gpriv, x) (reg_gen4(gpriv, 0x00c0, 0x00b8) + (0x04 * (x)))
> > +#define RCANFD_RFCC(gpriv, x) ((gpriv)->info->regs[RCANFD_RFCC] + (0x04 * (x)))
> > /* RSCFDnCFDRFSTSx / RSCFDnRFSTSx */
> > #define RCANFD_RFSTS(gpriv, x) (RCANFD_RFCC(gpriv, x) + 0x20)
> > /* RSCFDnCFDRFPCTRx / RSCFDnRFPCTRx */ @@ -308,13 +308,13 @@
> >
> > /* RSCFDnCFDCFCCx / RSCFDnCFCCx */
> > #define RCANFD_CFCC(gpriv, ch, idx) \
> > - (reg_gen4(gpriv, 0x0120, 0x0118) + (0x0c * (ch)) + (0x04 * (idx)))
> > + ((gpriv)->info->regs[RCANFD_CFCC] + (0x0c * (ch)) + (0x04 * (idx)))
> > /* RSCFDnCFDCFSTSx / RSCFDnCFSTSx */
> > #define RCANFD_CFSTS(gpriv, ch, idx) \
> > - (reg_gen4(gpriv, 0x01e0, 0x0178) + (0x0c * (ch)) + (0x04 * (idx)))
> > + ((gpriv)->info->regs[RCANFD_CFSTS] + (0x0c * (ch)) + (0x04 * (idx)))
> > /* RSCFDnCFDCFPCTRx / RSCFDnCFPCTRx */ #define RCANFD_CFPCTR(gpriv,
> > ch, idx) \
> > - (reg_gen4(gpriv, 0x0240, 0x01d8) + (0x0c * (ch)) + (0x04 * (idx)))
> > + ((gpriv)->info->regs[RCANFD_CFPCTR] + (0x0c * (ch)) + (0x04 *
> > +(idx)))
> >
> > /* RSCFDnCFDFESTS / RSCFDnFESTS */
> > #define RCANFD_FESTS (0x0238)
> > @@ -430,7 +430,7 @@
> > /* CAN FD mode specific register map */
> >
> > /* RSCFDnCFDCmXXX -> RCANFD_F_XXX(m) */
> > -#define RCANFD_F_DCFG(gpriv, m) (reg_gen4(gpriv, 0x1400, 0x0500) + (0x20 * (m)))
> > +#define RCANFD_F_DCFG(gpriv, m) ((gpriv)->info->regs[RCANFD_F_DCFG] + (0x20 * (m)))
> > #define RCANFD_F_CFDCFG(m) (0x0504 + (0x20 * (m)))
> > #define RCANFD_F_CFDCTR(m) (0x0508 + (0x20 * (m)))
> > #define RCANFD_F_CFDSTS(m) (0x050c + (0x20 * (m)))
> > @@ -446,7 +446,7 @@
> > #define RCANFD_F_RMDF(q, b) (0x200c + (0x04 * (b)) + (0x20 * (q)))
> >
> > /* RSCFDnCFDRFXXx -> RCANFD_F_RFXX(x) */
> > -#define RCANFD_F_RFOFFSET(gpriv) reg_gen4(gpriv, 0x6000, 0x3000)
> > +#define RCANFD_F_RFOFFSET(gpriv) ((gpriv)->info->regs[RCANFD_RFOFFSET])
> > #define RCANFD_F_RFID(gpriv, x) (RCANFD_F_RFOFFSET(gpriv) + (0x80 * (x)))
> > #define RCANFD_F_RFPTR(gpriv, x) (RCANFD_F_RFOFFSET(gpriv) + 0x04 + (0x80 * (x)))
> > #define RCANFD_F_RFFDSTS(gpriv, x) (RCANFD_F_RFOFFSET(gpriv) + 0x08 + (0x80 * (x)))
> > @@ -454,7 +454,7 @@
> > (RCANFD_F_RFOFFSET(gpriv) + 0x0c + (0x80 * (x)) + (0x04 * (df)))
> >
> > /* RSCFDnCFDCFXXk -> RCANFD_F_CFXX(ch, k) */
> > -#define RCANFD_F_CFOFFSET(gpriv) reg_gen4(gpriv, 0x6400, 0x3400)
> > +#define RCANFD_F_CFOFFSET(gpriv) ((gpriv)->info->regs[RCANFD_CFOFFSET])
> >
> > #define RCANFD_F_CFID(gpriv, ch, idx) \
> > (RCANFD_F_CFOFFSET(gpriv) + (0x180 * (ch)) + (0x80 * (idx))) @@
> > -501,11 +501,22 @@
> > */
> > #define RCANFD_CFFIFO_IDX 0
> >
> > +enum rcar_canfd_reg_offset_id {
> > + RCANFD_RFCC, /* RX FIFO Configuration/Control Register */
> > + RCANFD_CFCC, /* Common FIFO Configuration/Control Register */
> > + RCANFD_CFSTS, /* Common FIFO Status Register */
> > + RCANFD_CFPCTR, /* Common FIFO Pointer Control Register */
> > + RCANFD_F_DCFG, /* Global FD Configuration Register */
> > + RCANFD_RFOFFSET, /* Receive FIFO buffer access ID register */
> > + RCANFD_CFOFFSET, /* Transmit/receive FIFO buffer access ID register */
> > +};
> > +
> > 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;
> > u16 num_supported_rules;
> > u8 rnc_stride;
> > u8 rnc_field_width;
> > @@ -612,9 +623,30 @@ static const struct can_bittiming_const rcar_canfd_bittiming_const = {
> > .brp_inc = 1,
> > };
> >
> > +static const u16 rcar_gen3_regs[] = {
> > + [RCANFD_RFCC] = 0x00b8,
> > + [RCANFD_CFCC] = 0x0118,
> > + [RCANFD_CFSTS] = 0x0178,
> > + [RCANFD_CFPCTR] = 0x01d8,
> > + [RCANFD_F_DCFG] = 0x0500,
> > + [RCANFD_RFOFFSET] = 0x3000,
> > + [RCANFD_CFOFFSET] = 0x3400,
> > +};
> > +
> > +static const u16 rcar_gen4_regs[] = {
> > + [RCANFD_RFCC] = 0x00c0,
> > + [RCANFD_CFCC] = 0x0120,
> > + [RCANFD_CFSTS] = 0x01e0,
> > + [RCANFD_CFPCTR] = 0x0240,
> > + [RCANFD_F_DCFG] = 0x1400,
> > + [RCANFD_RFOFFSET] = 0x6000,
> > + [RCANFD_CFOFFSET] = 0x6400,
> > +};
>
> The mapping is convinient when you want to iterate throught it. Here, if you just want to access the
> offset value from its name, a structure looks more appropriate. This way:
>
> gpriv->info->regs[RCANFD_RFOFFSET]
>
> becomes:
>
> gpriv->info->rfoffset
>
> and so on.
>
> Internally, it is the same layout, but no need for the enum anymore.
OK.
>
> Note that you can either inline those values in struct rcar_canfd_hw_info or have a separate struct
> rcar_regs, as you prefer!
I will go for separate struct as it reduces the memory.
Cheers,
Biju
