On 12/06/2025 at 00:37, Geert Uytterhoeven wrote:
> Reuse the existing Channel Data Bitrate Configuration Register offset
> member in the register configuration as the base offset for all related
> channel-specific registers.
> Rename the member and update the (incorrect) comment to reflect this.
> Replace the function-like channel-specific register offset macros by
> inline functions.
>
> This fixes the offsets of all other (currently unused) channel-specific
> registers on R-Car Gen4 and RZ/G3E, and allows us to replace
> RCANFD_GEN4_FDCFG() by the more generic rcar_canfd_f_cfdcfg().
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> Reviewed-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> ---
> v2:
> - Add Reviewed-by.
> - Replace function-like macros by inline functions,
Thanks!
> - s/addresses/offsets/.
> ---
> drivers/net/can/rcar/rcar_canfd.c | 52 ++++++++++++++++++++++---------
> 1 file changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index dded509793bb93ec..8baf8a928da757f2 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -425,19 +425,10 @@
> #define RCANFD_C_RPGACC(r) (0x1900 + (0x04 * (r)))
>
> /* R-Car Gen4 Classical and CAN FD mode specific register map */
> -#define RCANFD_GEN4_FDCFG(m) (0x1404 + (0x20 * (m)))
> -
> #define RCANFD_GEN4_GAFL_OFFSET (0x1800)
>
> /* CAN FD mode specific register map */
>
> -/* RSCFDnCFDCmXXX -> RCANFD_F_XXX(m) */
> -#define RCANFD_F_DCFG(gpriv, m) ((gpriv)->info->regs->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)))
> -#define RCANFD_F_CFDCRC(m) (0x0510 + (0x20 * (m)))
> -
> /* RSCFDnCFDGAFLXXXj offset */
> #define RCANFD_F_GAFL_OFFSET (0x1000)
>
> @@ -510,7 +501,7 @@ struct rcar_canfd_regs {
> u16 cfcc; /* Common FIFO Configuration/Control Register */
> u16 cfsts; /* Common FIFO Status Register */
> u16 cfpctr; /* Common FIFO Pointer Control Register */
> - u16 f_dcfg; /* Global FD Configuration Register */
> + u16 coffset; /* Channel Data Bitrate Configuration Register */
> u16 rfoffset; /* Receive FIFO buffer access ID register */
> u16 cfoffset; /* Transmit/receive FIFO buffer access ID register */
> };
> @@ -641,7 +632,7 @@ static const struct rcar_canfd_regs rcar_gen3_regs = {
> .cfcc = 0x0118,
> .cfsts = 0x0178,
> .cfpctr = 0x01d8,
> - .f_dcfg = 0x0500,
> + .coffset = 0x0500,
> .rfoffset = 0x3000,
> .cfoffset = 0x3400,
> };
> @@ -651,7 +642,7 @@ static const struct rcar_canfd_regs rcar_gen4_regs = {
> .cfcc = 0x0120,
> .cfsts = 0x01e0,
> .cfpctr = 0x0240,
> - .f_dcfg = 0x1400,
> + .coffset = 0x1400,
> .rfoffset = 0x6000,
> .cfoffset = 0x6400,
> };
> @@ -800,6 +791,37 @@ static void rcar_canfd_put_data(struct rcar_canfd_channel *priv,
> rcar_canfd_write(priv->base, off + i * sizeof(u32), data[i]);
> }
>
> +/* RSCFDnCFDCmXXX -> rcar_canfd_f_xxx(gpriv, ch) */
> +static inline unsigned int rcar_canfd_f_dcfg(struct rcar_canfd_global *gpriv,
> + unsigned int ch)
> +{
> + return gpriv->info->regs->coffset + 0x00 + 0x20 * ch;
> +}
> +
> +static inline unsigned int rcar_canfd_f_cfdcfg(struct rcar_canfd_global *gpriv,
> + unsigned int ch)
> +{
> + return gpriv->info->regs->coffset + 0x04 + 0x20 * ch;
> +}
> +
> +static inline unsigned int rcar_canfd_f_cfdctr(struct rcar_canfd_global *gpriv,
> + unsigned int ch)
> +{
> + return gpriv->info->regs->coffset + 0x08 + 0x20 * ch;
> +}
> +
> +static inline unsigned int rcar_canfd_f_cfdsts(struct rcar_canfd_global *gpriv,
> + unsigned int ch)
> +{
> + return gpriv->info->regs->coffset + 0x0c + 0x20 * ch;
> +}
> +
> +static inline unsigned int rcar_canfd_f_cfdcrc(struct rcar_canfd_global *gpriv,
> + unsigned int ch)
> +{
> + return gpriv->info->regs->coffset + 0x10 + 0x20 * ch;
> +}
> +
> static void rcar_canfd_tx_failure_cleanup(struct net_device *ndev)
> {
> u32 i;
> @@ -827,8 +849,8 @@ static void rcar_canfd_set_mode(struct rcar_canfd_global *gpriv)
>
> for_each_set_bit(ch, &gpriv->channels_mask,
> gpriv->info->max_channels)
> - rcar_canfd_set_bit(gpriv->base, RCANFD_GEN4_FDCFG(ch),
> - val);
> + rcar_canfd_set_bit(gpriv->base,
> + rcar_canfd_f_cfdcfg(gpriv, ch), val);
> } else {
> if (gpriv->fdmode)
> rcar_canfd_set_bit(gpriv->base, RCANFD_GRMCFG,
> @@ -1468,7 +1490,7 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
> cfg = (RCANFD_DCFG_DTSEG1(gpriv, tseg1) | RCANFD_DCFG_DBRP(brp) |
> RCANFD_DCFG_DSJW(gpriv, sjw) | RCANFD_DCFG_DTSEG2(gpriv, tseg2));
>
> - rcar_canfd_write(priv->base, RCANFD_F_DCFG(gpriv, ch), cfg);
> + rcar_canfd_write(priv->base, rcar_canfd_f_dcfg(gpriv, ch), cfg);
> } else {
> /* Classical CAN only mode */
> if (gpriv->info->shared_can_regs) {
Thinking of your code, you are still using some magic numbers, e.g.
0x04 + 0x20 * ch
to access your registers. But at the end those magic numbers are just describing
a memory layout.
I think this can be describe as a C structure. This is what I have in mind:
--------------8<--------------
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 1e559c0ff038..487f40320c20 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -560,10 +560,21 @@ struct rcar_canfd_channel {
spinlock_t tx_lock; /* To protect tx path */
};
+struct rcar_canfd_f {
+ u32 dcfg;
+ u32 cfdcfg;
+ u32 cfdctr;
+ u32 cfdsts;
+ u32 cfdcrc;
+ u32 padding[3];
+};
+static_assert(sizeof(struct rcar_canfd_f) == 0x20);
+
/* Global priv data */
struct rcar_canfd_global {
struct rcar_canfd_channel *ch[RCANFD_NUM_CHANNELS];
void __iomem *base; /* Register base address */
+ struct rcar_canfd_f __iomem *cbase;
struct platform_device *pdev; /* Respective platform device */
struct clk *clkp; /* Peripheral clock */
struct clk *can_clk; /* fCAN clock */
@@ -803,6 +814,26 @@ static void rcar_canfd_update_bit(void __iomem *base, u32 reg,
rcar_canfd_update(mask, val, base + reg);
}
+static inline u32 rcar_canfd_read_reg(void __iomem *addr)
+{
+ return readl(addr);
+}
+
+static inline void rcar_canfd_write_reg(void __iomem *addr, u32 val)
+{
+ writel(val, addr);
+}
+
+static void rcar_canfd_set_bit_reg(void __iomem *addr, u32 val)
+{
+ rcar_canfd_update(val, val, addr);
+}
+
+static void rcar_canfd_update_bit_reg(void __iomem *addr, u32 mask, u32 val)
+{
+ rcar_canfd_update(mask, val, addr);
+}
+
static void rcar_canfd_get_data(struct rcar_canfd_channel *priv,
struct canfd_frame *cf, u32 off)
{
@@ -825,37 +856,6 @@ static void rcar_canfd_put_data(struct rcar_canfd_channel *priv,
rcar_canfd_write(priv->base, off + i * sizeof(u32), data[i]);
}
-/* RSCFDnCFDCmXXX -> rcar_canfd_f_xxx(gpriv, ch) */
-static inline unsigned int rcar_canfd_f_dcfg(struct rcar_canfd_global *gpriv,
- unsigned int ch)
-{
- return gpriv->info->regs->coffset + 0x00 + 0x20 * ch;
-}
-
-static inline unsigned int rcar_canfd_f_cfdcfg(struct rcar_canfd_global *gpriv,
- unsigned int ch)
-{
- return gpriv->info->regs->coffset + 0x04 + 0x20 * ch;
-}
-
-static inline unsigned int rcar_canfd_f_cfdctr(struct rcar_canfd_global *gpriv,
- unsigned int ch)
-{
- return gpriv->info->regs->coffset + 0x08 + 0x20 * ch;
-}
-
-static inline unsigned int rcar_canfd_f_cfdsts(struct rcar_canfd_global *gpriv,
- unsigned int ch)
-{
- return gpriv->info->regs->coffset + 0x0c + 0x20 * ch;
-}
-
-static inline unsigned int rcar_canfd_f_cfdcrc(struct rcar_canfd_global *gpriv,
- unsigned int ch)
-{
- return gpriv->info->regs->coffset + 0x10 + 0x20 * ch;
-}
-
static void rcar_canfd_tx_failure_cleanup(struct net_device *ndev)
{
u32 i;
@@ -883,8 +883,7 @@ static void rcar_canfd_set_mode(struct rcar_canfd_global *gpriv)
for_each_set_bit(ch, &gpriv->channels_mask,
gpriv->info->max_channels)
- rcar_canfd_set_bit(gpriv->base,
- rcar_canfd_f_cfdcfg(gpriv, ch), val);
+ rcar_canfd_set_bit_reg(&gpriv->cbase[ch].cfdcfg, val);
} else {
if (gpriv->fdmode)
rcar_canfd_set_bit(gpriv->base, RCANFD_GRMCFG,
@@ -1533,7 +1532,7 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
cfg = (RCANFD_DCFG_DTSEG1(gpriv, tseg1) | RCANFD_DCFG_DBRP(brp) |
RCANFD_DCFG_DSJW(gpriv, sjw) | RCANFD_DCFG_DTSEG2(gpriv, tseg2));
- rcar_canfd_write(priv->base, rcar_canfd_f_dcfg(gpriv, ch), cfg);
+ rcar_canfd_write_reg(&gpriv->cbase[ch].dcfg, cfg);
/* Transceiver Delay Compensation */
if (priv->can.ctrlmode & CAN_CTRLMODE_TDC_AUTO) {
@@ -1546,8 +1545,8 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
tdco = min(tdc->tdcv + tdc->tdco, tdc_const->tdco_max) - 1;
}
- rcar_canfd_update_bit(gpriv->base, rcar_canfd_f_cfdcfg(gpriv, ch), mask,
- tdcmode | FIELD_PREP(RCANFD_FDCFG_TDCO, tdco));
+ rcar_canfd_update_bit_reg(&gpriv->cbase[ch].cfdcfg, mask,
+ tdcmode | FIELD_PREP(RCANFD_FDCFG_TDCO, tdco));
}
static int rcar_canfd_start(struct net_device *ndev)
@@ -1861,7 +1860,7 @@ static int rcar_canfd_rx_poll(struct napi_struct *napi, int quota)
static unsigned int rcar_canfd_get_tdcr(struct rcar_canfd_global *gpriv,
unsigned int ch)
{
- u32 sts = rcar_canfd_read(gpriv->base, rcar_canfd_f_cfdsts(gpriv, ch));
+ u32 sts = rcar_canfd_read_reg(&gpriv->cbase[ch].cfdsts);
u32 tdcr = FIELD_GET(RCANFD_FDSTS_TDCR, sts);
return tdcr & (gpriv->info->tdc_const->tdcv_max - 1);
@@ -2170,6 +2169,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
goto fail_dev;
}
gpriv->base = addr;
+ gpriv->cbase = gpriv->base + gpriv->info->regs->coffset;
/* Request IRQ that's common for both channels */
if (info->shared_global_irqs) {
-------------->8--------------
(this applies on top of the last patch of your series)
To be honnest, I am happy to accept your patch as it is now, but what
do you think of the above? I think that this approach works with your
other macro as well.
Yours sincerely,
Vincent Mailhol
