[PATCH 0/4] R-Car CANFD Improvements

[PATCH 0/4] R-Car CANFD Improvements

[Biju Das]

The calculation formula for nominal bit rate of classical CAN is same as
that of nominal bit rate of CANFD on the RZ/G3E SoC compared to other SoCs.
Add shared_bittiming variable to struct rcar_canfd_hw_info to handle this
difference.

Added the patch "Drop unused macros" to this serires.

Apart from this, replaced the RCANFD_NCFG_* and RCANFD_DCFG_* macros
with simple functions and replaced RCANFD_CFG_* macros with FIELD_PREP
macro.

Biju Das (4):
  can: rcar_canfd: Add shared_bittiming variable to struct
    rcar_canfd_hw_info
  can: rcar_canfd: Drop unused macros
  can: rcar_canfd: Replace RCANFD_NCFG_* and RCANFD_DCFG_* macros
  can: rcar_canfd: Replace RCANFD_CFG_* macros with FIELD_PREP

 drivers/net/can/rcar/rcar_canfd.c | 170 +++++++-----------------------
 1 file changed, 39 insertions(+), 131 deletions(-)

-- 
2.43.0

[PATCH 1/4] can: rcar_canfd: Add shared_bittiming variable to struct rcar_canfd_hw_info

[Biju Das]

The calculation formula for nominal bit rate of classical CAN is same as
that of nominal bit rate of CANFD on the RZ/G3E SoC compared to other SoCs.
Add shared_bittiming variable to struct rcar_canfd_hw_info to handle this
difference.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/net/can/rcar/rcar_canfd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 417d9196f41e..48eea9045691 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -554,6 +554,7 @@ struct rcar_canfd_hw_info {
 	unsigned ch_interface_mode:1;	/* Has channel interface mode */
 	unsigned shared_can_regs:1;	/* Has shared classical can registers */
 	unsigned external_clk:1;	/* Has external clock */
+	unsigned shared_bittiming:1;	/* Has shared nominal bittiming constants */
 };
 
 /* Channel priv data */
@@ -725,6 +726,7 @@ static const struct rcar_canfd_hw_info rcar_gen3_hw_info = {
 	.ch_interface_mode = 0,
 	.shared_can_regs = 0,
 	.external_clk = 1,
+	.shared_bittiming = 0,
 };
 
 static const struct rcar_canfd_hw_info rcar_gen4_hw_info = {
@@ -742,6 +744,7 @@ static const struct rcar_canfd_hw_info rcar_gen4_hw_info = {
 	.ch_interface_mode = 1,
 	.shared_can_regs = 1,
 	.external_clk = 1,
+	.shared_bittiming = 0,
 };
 
 static const struct rcar_canfd_hw_info rzg2l_hw_info = {
@@ -759,6 +762,7 @@ static const struct rcar_canfd_hw_info rzg2l_hw_info = {
 	.ch_interface_mode = 0,
 	.shared_can_regs = 0,
 	.external_clk = 1,
+	.shared_bittiming = 0,
 };
 
 static const struct rcar_canfd_hw_info r9a09g047_hw_info = {
@@ -776,6 +780,7 @@ static const struct rcar_canfd_hw_info r9a09g047_hw_info = {
 	.ch_interface_mode = 1,
 	.shared_can_regs = 1,
 	.external_clk = 0,
+	.shared_bittiming = 1,
 };
 
 /* Helper functions */
@@ -2005,7 +2010,10 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
 		priv->can.fd.do_get_auto_tdcv = rcar_canfd_get_auto_tdcv;
 	} else {
 		/* Controller starts in Classical CAN only mode */
-		priv->can.bittiming_const = &rcar_canfd_bittiming_const;
+		if (gpriv->info->shared_bittiming)
+			priv->can.bittiming_const = gpriv->info->nom_bittiming;
+		else
+			priv->can.bittiming_const = &rcar_canfd_bittiming_const;
 		priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;
 	}
 
-- 
2.43.0

[PATCH 2/4] can: rcar_canfd: Drop unused macros

[Biju Das]

Drop unused macros from the rcar_canfd.c.

Reported-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Closes: https://lore.kernel.org/all/7ff93ff9-f578-4be2-bdc6-5b09eab64fe6@wanadoo.fr/
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Added Rb tag from Geert and Vincent.
v1->v2:
 * Dropped the Rb tags.
 * Restored simple bit definitions as it serve as documentation.
 * Restored register offsets, as it will become anonymous gaps when the
   register offsets are replaced by C structs.
---
 drivers/net/can/rcar/rcar_canfd.c | 93 -------------------------------
 1 file changed, 93 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 48eea9045691..ce355f79e6b6 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -224,8 +224,6 @@
 
 /* RSCFDnCFDRFPTRx */
 #define RCANFD_RFPTR_RFDLC(x)		(((x) >> 28) & 0xf)
-#define RCANFD_RFPTR_RFPTR(x)		(((x) >> 16) & 0xfff)
-#define RCANFD_RFPTR_RFTS(x)		(((x) >> 0) & 0xffff)
 
 /* RSCFDnCFDRFFDSTSx */
 #define RCANFD_RFFDSTS_RFFDF		BIT(2)
@@ -257,12 +255,9 @@
 /* RSCFDnCFDCFIDk */
 #define RCANFD_CFID_CFIDE		BIT(31)
 #define RCANFD_CFID_CFRTR		BIT(30)
-#define RCANFD_CFID_CFID_MASK(x)	((x) & 0x1fffffff)
 
 /* RSCFDnCFDCFPTRk */
 #define RCANFD_CFPTR_CFDLC(x)		(((x) & 0xf) << 28)
-#define RCANFD_CFPTR_CFPTR(x)		(((x) & 0xfff) << 16)
-#define RCANFD_CFPTR_CFTS(x)		(((x) & 0xff) << 0)
 
 /* RSCFDnCFDCFFDCSTSk */
 #define RCANFD_CFFDCSTS_CFFDF		BIT(2)
@@ -328,59 +323,6 @@
 #define RCANFD_CFPCTR(gpriv, ch, idx) \
 	((gpriv)->info->regs->cfpctr + (0x0c * (ch)) + (0x04 * (idx)))
 
-/* RSCFDnCFDFESTS / RSCFDnFESTS */
-#define RCANFD_FESTS			(0x0238)
-/* RSCFDnCFDFFSTS / RSCFDnFFSTS */
-#define RCANFD_FFSTS			(0x023c)
-/* RSCFDnCFDFMSTS / RSCFDnFMSTS */
-#define RCANFD_FMSTS			(0x0240)
-/* RSCFDnCFDRFISTS / RSCFDnRFISTS */
-#define RCANFD_RFISTS			(0x0244)
-/* RSCFDnCFDCFRISTS / RSCFDnCFRISTS */
-#define RCANFD_CFRISTS			(0x0248)
-/* RSCFDnCFDCFTISTS / RSCFDnCFTISTS */
-#define RCANFD_CFTISTS			(0x024c)
-
-/* RSCFDnCFDTMCp / RSCFDnTMCp */
-#define RCANFD_TMC(p)			(0x0250 + (0x01 * (p)))
-/* RSCFDnCFDTMSTSp / RSCFDnTMSTSp */
-#define RCANFD_TMSTS(p)			(0x02d0 + (0x01 * (p)))
-
-/* RSCFDnCFDTMTRSTSp / RSCFDnTMTRSTSp */
-#define RCANFD_TMTRSTS(y)		(0x0350 + (0x04 * (y)))
-/* RSCFDnCFDTMTARSTSp / RSCFDnTMTARSTSp */
-#define RCANFD_TMTARSTS(y)		(0x0360 + (0x04 * (y)))
-/* RSCFDnCFDTMTCSTSp / RSCFDnTMTCSTSp */
-#define RCANFD_TMTCSTS(y)		(0x0370 + (0x04 * (y)))
-/* RSCFDnCFDTMTASTSp / RSCFDnTMTASTSp */
-#define RCANFD_TMTASTS(y)		(0x0380 + (0x04 * (y)))
-/* RSCFDnCFDTMIECy / RSCFDnTMIECy */
-#define RCANFD_TMIEC(y)			(0x0390 + (0x04 * (y)))
-
-/* RSCFDnCFDTXQCCm / RSCFDnTXQCCm */
-#define RCANFD_TXQCC(m)			(0x03a0 + (0x04 * (m)))
-/* RSCFDnCFDTXQSTSm / RSCFDnTXQSTSm */
-#define RCANFD_TXQSTS(m)		(0x03c0 + (0x04 * (m)))
-/* RSCFDnCFDTXQPCTRm / RSCFDnTXQPCTRm */
-#define RCANFD_TXQPCTR(m)		(0x03e0 + (0x04 * (m)))
-
-/* RSCFDnCFDTHLCCm / RSCFDnTHLCCm */
-#define RCANFD_THLCC(m)			(0x0400 + (0x04 * (m)))
-/* RSCFDnCFDTHLSTSm / RSCFDnTHLSTSm */
-#define RCANFD_THLSTS(m)		(0x0420 + (0x04 * (m)))
-/* RSCFDnCFDTHLPCTRm / RSCFDnTHLPCTRm */
-#define RCANFD_THLPCTR(m)		(0x0440 + (0x04 * (m)))
-
-/* RSCFDnCFDGTINTSTS0 / RSCFDnGTINTSTS0 */
-#define RCANFD_GTINTSTS0		(0x0460)
-/* RSCFDnCFDGTINTSTS1 / RSCFDnGTINTSTS1 */
-#define RCANFD_GTINTSTS1		(0x0464)
-/* RSCFDnCFDGTSTCFG / RSCFDnGTSTCFG */
-#define RCANFD_GTSTCFG			(0x0468)
-/* RSCFDnCFDGTSTCTR / RSCFDnGTSTCTR */
-#define RCANFD_GTSTCTR			(0x046c)
-/* RSCFDnCFDGLOCKK / RSCFDnGLOCKK */
-#define RCANFD_GLOCKK			(0x047c)
 /* RSCFDnCFDGRMCFG */
 #define RCANFD_GRMCFG			(0x04fc)
 
@@ -398,12 +340,6 @@
 /* RSCFDnGAFLXXXj offset */
 #define RCANFD_C_GAFL_OFFSET		(0x0500)
 
-/* RSCFDnRMXXXq -> RCANFD_C_RMXXX(q) */
-#define RCANFD_C_RMID(q)		(0x0600 + (0x10 * (q)))
-#define RCANFD_C_RMPTR(q)		(0x0604 + (0x10 * (q)))
-#define RCANFD_C_RMDF0(q)		(0x0608 + (0x10 * (q)))
-#define RCANFD_C_RMDF1(q)		(0x060c + (0x10 * (q)))
-
 /* RSCFDnRFXXx -> RCANFD_C_RFXX(x) */
 #define RCANFD_C_RFOFFSET	(0x0e00)
 #define RCANFD_C_RFID(x)	(RCANFD_C_RFOFFSET + (0x10 * (x)))
@@ -423,17 +359,6 @@
 #define RCANFD_C_CFDF(ch, idx, df) \
 	(RCANFD_C_CFOFFSET + 0x08 + (0x30 * (ch)) + (0x10 * (idx)) + (0x04 * (df)))
 
-/* RSCFDnTMXXp -> RCANFD_C_TMXX(p) */
-#define RCANFD_C_TMID(p)		(0x1000 + (0x10 * (p)))
-#define RCANFD_C_TMPTR(p)		(0x1004 + (0x10 * (p)))
-#define RCANFD_C_TMDF0(p)		(0x1008 + (0x10 * (p)))
-#define RCANFD_C_TMDF1(p)		(0x100c + (0x10 * (p)))
-
-/* RSCFDnTHLACCm */
-#define RCANFD_C_THLACC(m)		(0x1800 + (0x04 * (m)))
-/* RSCFDnRPGACCr */
-#define RCANFD_C_RPGACC(r)		(0x1900 + (0x04 * (r)))
-
 /* R-Car Gen4 Classical and CAN FD mode specific register map */
 #define RCANFD_GEN4_GAFL_OFFSET		(0x1800)
 
@@ -452,12 +377,6 @@ struct rcar_canfd_f_c {
 /* RSCFDnCFDGAFLXXXj offset */
 #define RCANFD_F_GAFL_OFFSET		(0x1000)
 
-/* RSCFDnCFDRMXXXq -> RCANFD_F_RMXXX(q) */
-#define RCANFD_F_RMID(q)		(0x2000 + (0x20 * (q)))
-#define RCANFD_F_RMPTR(q)		(0x2004 + (0x20 * (q)))
-#define RCANFD_F_RMFDSTS(q)		(0x2008 + (0x20 * (q)))
-#define RCANFD_F_RMDF(q, b)		(0x200c + (0x04 * (b)) + (0x20 * (q)))
-
 /* RSCFDnCFDRFXXx -> RCANFD_F_RFXX(x) */
 #define RCANFD_F_RFOFFSET(gpriv)	((gpriv)->info->regs->rfoffset)
 #define RCANFD_F_RFID(gpriv, x)		(RCANFD_F_RFOFFSET(gpriv) + (0x80 * (x)))
@@ -482,23 +401,11 @@ struct rcar_canfd_f_c {
 	(RCANFD_F_CFOFFSET(gpriv) + 0x0c + (0x180 * (ch)) + (0x80 * (idx)) + \
 	 (0x04 * (df)))
 
-/* RSCFDnCFDTMXXp -> RCANFD_F_TMXX(p) */
-#define RCANFD_F_TMID(p)		(0x4000 + (0x20 * (p)))
-#define RCANFD_F_TMPTR(p)		(0x4004 + (0x20 * (p)))
-#define RCANFD_F_TMFDCTR(p)		(0x4008 + (0x20 * (p)))
-#define RCANFD_F_TMDF(p, b)		(0x400c + (0x20 * (p)) + (0x04 * (b)))
-
-/* RSCFDnCFDTHLACCm */
-#define RCANFD_F_THLACC(m)		(0x6000 + (0x04 * (m)))
-/* RSCFDnCFDRPGACCr */
-#define RCANFD_F_RPGACC(r)		(0x6400 + (0x04 * (r)))
-
 /* Constants */
 #define RCANFD_FIFO_DEPTH		8	/* Tx FIFO depth */
 #define RCANFD_NAPI_WEIGHT		8	/* Rx poll quota */
 
 #define RCANFD_NUM_CHANNELS		8	/* Eight channels max */
-#define RCANFD_CHANNELS_MASK		BIT((RCANFD_NUM_CHANNELS) - 1)
 
 #define RCANFD_GAFL_PAGENUM(entry)	((entry) / 16)
 #define RCANFD_CHANNEL_NUMRULES		1	/* only one rule per channel */
-- 
2.43.0

[PATCH 3/4] can: rcar_canfd: Replace RCANFD_NCFG_* and RCANFD_DCFG_* macros

[Biju Das]

Replace RCANFD_NCFG_* and RCANFD_DCFG_* macros with functions for
simplifying the computation of configuration register.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/can/rcar/rcar_canfd.c | 57 +++++++++++++++----------------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index ce355f79e6b6..b5b059e83374 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -108,18 +108,6 @@
 #define RCANFD_CFG_TSEG1(x)		(((x) & 0xf) << 16)
 #define RCANFD_CFG_BRP(x)		(((x) & 0x3ff) << 0)
 
-/* RSCFDnCFDCmNCFG - CAN FD only */
-#define RCANFD_NCFG_NTSEG2(gpriv, x) \
-	(((x) & ((gpriv)->info->nom_bittiming->tseg2_max - 1)) << (gpriv)->info->sh->ntseg2)
-
-#define RCANFD_NCFG_NTSEG1(gpriv, x) \
-	(((x) & ((gpriv)->info->nom_bittiming->tseg1_max - 1)) << (gpriv)->info->sh->ntseg1)
-
-#define RCANFD_NCFG_NSJW(gpriv, x) \
-	(((x) & ((gpriv)->info->nom_bittiming->sjw_max - 1)) << (gpriv)->info->sh->nsjw)
-
-#define RCANFD_NCFG_NBRP(x)		(((x) & 0x3ff) << 0)
-
 /* RSCFDnCFDCmCTR / RSCFDnCmCTR */
 #define RCANFD_CCTR_CTME		BIT(24)
 #define RCANFD_CCTR_ERRD		BIT(23)
@@ -177,17 +165,6 @@
 
 #define RCANFD_CERFL_ERR(x)		((x) & (0x7fff)) /* above bits 14:0 */
 
-/* RSCFDnCFDCmDCFG */
-#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)) << (gpriv)->info->sh->dtseg2)
-
-#define RCANFD_DCFG_DTSEG1(gpriv, x) \
-	(((x) & ((gpriv)->info->data_bittiming->tseg1_max - 1)) << (gpriv)->info->sh->dtseg1)
-
-#define RCANFD_DCFG_DBRP(x)		(((x) & 0xff) << 0)
-
 /* RSCFDnCFDCmFDCFG */
 #define RCANFD_GEN4_FDCFG_CLOE		BIT(30)
 #define RCANFD_GEN4_FDCFG_FDOE		BIT(28)
@@ -736,6 +713,32 @@ static void rcar_canfd_update_bit_reg(void __iomem *addr, u32 mask, u32 val)
 	rcar_canfd_update(mask, val, addr);
 }
 
+static inline u32 rcar_canfd_compute_nominal_bit_rate_cfg(const struct rcar_canfd_hw_info *info,
+							  u16 tseg1, u16 brp, u16 sjw, u16 tseg2)
+{
+	u32 ntseg2, ntseg1, nsjw, nbrp;
+
+	ntseg2 = (tseg2 & (info->nom_bittiming->tseg2_max - 1)) << info->sh->ntseg2;
+	ntseg1 = (tseg1 & (info->nom_bittiming->tseg1_max - 1)) << info->sh->ntseg1;
+	nsjw = (sjw & (info->nom_bittiming->sjw_max - 1)) << info->sh->nsjw;
+	nbrp = brp & GENMASK(9, 0);
+
+	return (ntseg1 | nbrp | nsjw | ntseg2);
+}
+
+static inline u32 rcar_canfd_compute_data_bit_rate_cfg(const struct rcar_canfd_hw_info *info,
+						       u16 tseg1, u16 brp, u16 sjw, u16 tseg2)
+{
+	u32 dtseg2, dtseg1, dsjw, dbrp;
+
+	dtseg2 = (tseg2 & (info->data_bittiming->tseg2_max - 1)) << info->sh->dtseg2;
+	dtseg1 = (tseg1 & (info->data_bittiming->tseg1_max - 1)) << info->sh->dtseg1;
+	dsjw = (sjw & (info->data_bittiming->sjw_max - 1)) << 24;
+	dbrp = brp & GENMASK(7, 0);
+
+	return (dtseg1 | dbrp | dsjw | dtseg2);
+}
+
 static void rcar_canfd_get_data(struct rcar_canfd_channel *priv,
 				struct canfd_frame *cf, u32 off)
 {
@@ -1413,8 +1416,7 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
 	tseg2 = bt->phase_seg2 - 1;
 
 	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) || gpriv->info->shared_can_regs) {
-		cfg = (RCANFD_NCFG_NTSEG1(gpriv, tseg1) | RCANFD_NCFG_NBRP(brp) |
-		       RCANFD_NCFG_NSJW(gpriv, sjw) | RCANFD_NCFG_NTSEG2(gpriv, tseg2));
+		cfg = rcar_canfd_compute_nominal_bit_rate_cfg(gpriv->info, tseg1, brp, sjw, tseg2);
 	} else {
 		cfg = (RCANFD_CFG_TSEG1(tseg1) | RCANFD_CFG_BRP(brp) |
 		       RCANFD_CFG_SJW(sjw) | RCANFD_CFG_TSEG2(tseg2));
@@ -1430,10 +1432,7 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
 	sjw = dbt->sjw - 1;
 	tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
 	tseg2 = dbt->phase_seg2 - 1;
-
-	cfg = (RCANFD_DCFG_DTSEG1(gpriv, tseg1) | RCANFD_DCFG_DBRP(brp) |
-	       RCANFD_DCFG_DSJW(gpriv, sjw) | RCANFD_DCFG_DTSEG2(gpriv, tseg2));
-
+	cfg = rcar_canfd_compute_data_bit_rate_cfg(gpriv->info, tseg1, brp, sjw, tseg2);
 	writel(cfg, &gpriv->fcbase[ch].dcfg);
 
 	/* Transceiver Delay Compensation */
-- 
2.43.0

[PATCH 4/4] can: rcar_canfd: Replace RCANFD_CFG_* macros with FIELD_PREP

[Biju Das]

Replace RCANFD_CFG_* macros with simpler FIELD_PREP macro.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/can/rcar/rcar_canfd.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index b5b059e83374..dd87b4e8e688 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -102,12 +102,6 @@
 
 /* Channel register bits */
 
-/* RSCFDnCmCFG - Classical CAN only */
-#define RCANFD_CFG_SJW(x)		(((x) & 0x3) << 24)
-#define RCANFD_CFG_TSEG2(x)		(((x) & 0x7) << 20)
-#define RCANFD_CFG_TSEG1(x)		(((x) & 0xf) << 16)
-#define RCANFD_CFG_BRP(x)		(((x) & 0x3ff) << 0)
-
 /* RSCFDnCFDCmCTR / RSCFDnCmCTR */
 #define RCANFD_CCTR_CTME		BIT(24)
 #define RCANFD_CCTR_ERRD		BIT(23)
@@ -1418,8 +1412,8 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
 	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) || gpriv->info->shared_can_regs) {
 		cfg = rcar_canfd_compute_nominal_bit_rate_cfg(gpriv->info, tseg1, brp, sjw, tseg2);
 	} else {
-		cfg = (RCANFD_CFG_TSEG1(tseg1) | RCANFD_CFG_BRP(brp) |
-		       RCANFD_CFG_SJW(sjw) | RCANFD_CFG_TSEG2(tseg2));
+		cfg = FIELD_PREP(GENMASK(19, 16), tseg1) | FIELD_PREP(GENMASK(9, 0), brp) |
+		      FIELD_PREP(GENMASK(25, 24), sjw) | FIELD_PREP(GENMASK(22, 20), tseg2);
 	}
 
 	rcar_canfd_write(priv->base, RCANFD_CCFG(ch), cfg);
-- 
2.43.0

Re: [PATCH 4/4] can: rcar_canfd: Replace RCANFD_CFG_* macros with FIELD_PREP

[Vincent Mailhol]

Hi Biju,

On 04/07/2025 at 03:34, Biju Das wrote:
> Replace RCANFD_CFG_* macros with simpler FIELD_PREP macro.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/net/can/rcar/rcar_canfd.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index b5b059e83374..dd87b4e8e688 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -102,12 +102,6 @@
>  
>  /* Channel register bits */
>  
> -/* RSCFDnCmCFG - Classical CAN only */
> -#define RCANFD_CFG_SJW(x)		(((x) & 0x3) << 24)
> -#define RCANFD_CFG_TSEG2(x)		(((x) & 0x7) << 20)
> -#define RCANFD_CFG_TSEG1(x)		(((x) & 0xf) << 16)
> -#define RCANFD_CFG_BRP(x)		(((x) & 0x3ff) << 0)
> -
>  /* RSCFDnCFDCmCTR / RSCFDnCmCTR */
>  #define RCANFD_CCTR_CTME		BIT(24)
>  #define RCANFD_CCTR_ERRD		BIT(23)
> @@ -1418,8 +1412,8 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
>  	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) || gpriv->info->shared_can_regs) {
>  		cfg = rcar_canfd_compute_nominal_bit_rate_cfg(gpriv->info, tseg1, brp, sjw, tseg2);
>  	} else {
> -		cfg = (RCANFD_CFG_TSEG1(tseg1) | RCANFD_CFG_BRP(brp) |
> -		       RCANFD_CFG_SJW(sjw) | RCANFD_CFG_TSEG2(tseg2));
> +		cfg = FIELD_PREP(GENMASK(19, 16), tseg1) | FIELD_PREP(GENMASK(9, 0), brp) |
> +		      FIELD_PREP(GENMASK(25, 24), sjw) | FIELD_PREP(GENMASK(22, 20), tseg2);
>  	}

You can still keep the macro definition to give a meaning to the magic number:

  #define RCANFD_CFG_SJW_MASK GENMASK(25, 24)

and do a:

  FIELD_PREP(RCANFD_CFG_SJW_MASK, sjw)

(same for the other fields).

>  	rcar_canfd_write(priv->base, RCANFD_CCFG(ch), cfg);


Yours sincerely,
Vincent Mailhol

RE: [PATCH 4/4] can: rcar_canfd: Replace RCANFD_CFG_* macros with FIELD_PREP

[Biju Das]

Hi Vincent,

Thanks for the feedback.

> -----Original Message-----
> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> Sent: 04 July 2025 03:25
> Subject: Re: [PATCH 4/4] can: rcar_canfd: Replace RCANFD_CFG_* macros with FIELD_PREP
> 
> Hi Biju,
> 
> On 04/07/2025 at 03:34, Biju Das wrote:
> > Replace RCANFD_CFG_* macros with simpler FIELD_PREP macro.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/net/can/rcar/rcar_canfd.c | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> > index b5b059e83374..dd87b4e8e688 100644
> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -102,12 +102,6 @@
> >
> >  /* Channel register bits */
> >
> > -/* RSCFDnCmCFG - Classical CAN only */
> > -#define RCANFD_CFG_SJW(x)		(((x) & 0x3) << 24)
> > -#define RCANFD_CFG_TSEG2(x)		(((x) & 0x7) << 20)
> > -#define RCANFD_CFG_TSEG1(x)		(((x) & 0xf) << 16)
> > -#define RCANFD_CFG_BRP(x)		(((x) & 0x3ff) << 0)
> > -
> >  /* RSCFDnCFDCmCTR / RSCFDnCmCTR */
> >  #define RCANFD_CCTR_CTME		BIT(24)
> >  #define RCANFD_CCTR_ERRD		BIT(23)
> > @@ -1418,8 +1412,8 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
> >  	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) || gpriv->info->shared_can_regs) {
> >  		cfg = rcar_canfd_compute_nominal_bit_rate_cfg(gpriv->info, tseg1, brp, sjw, tseg2);
> >  	} else {
> > -		cfg = (RCANFD_CFG_TSEG1(tseg1) | RCANFD_CFG_BRP(brp) |
> > -		       RCANFD_CFG_SJW(sjw) | RCANFD_CFG_TSEG2(tseg2));
> > +		cfg = FIELD_PREP(GENMASK(19, 16), tseg1) | FIELD_PREP(GENMASK(9, 0), brp) |
> > +		      FIELD_PREP(GENMASK(25, 24), sjw) | FIELD_PREP(GENMASK(22, 20), tseg2);
> >  	}
> 
> You can still keep the macro definition to give a meaning to the magic number:
> 
>   #define RCANFD_CFG_SJW_MASK GENMASK(25, 24)
> 
> and do a:
> 
>   FIELD_PREP(RCANFD_CFG_SJW_MASK, sjw)

Are you ok for the below change to reduce the changes minimum??

-#define RCANFD_CFG_SJW(x)		(((x) & 0x3) << 24)
+#define RCANFD_CFG_SJW(x)		FIELD_PREP(GENMASK(25, 24), x))

Or

you want RCANFD_CFG_SJW_MASK as separate one as you suggested?

Cheers,
Biju

> 
> (same for the other fields).
> 
> >  	rcar_canfd_write(priv->base, RCANFD_CCFG(ch), cfg);
> 
> 
> Yours sincerely,
> Vincent Mailhol

Re: [PATCH 4/4] can: rcar_canfd: Replace RCANFD_CFG_* macros with FIELD_PREP

[Vincent Mailhol]

On 04/07/2025 at 19:12, Biju Das wrote:
> Hi Vincent,
> 
> Thanks for the feedback.
> 
>> -----Original Message-----
>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>> Sent: 04 July 2025 03:25
>> Subject: Re: [PATCH 4/4] can: rcar_canfd: Replace RCANFD_CFG_* macros with FIELD_PREP
>>
>> Hi Biju,
>>
>> On 04/07/2025 at 03:34, Biju Das wrote:
>>> Replace RCANFD_CFG_* macros with simpler FIELD_PREP macro.
>>>
>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>> ---
>>>  drivers/net/can/rcar/rcar_canfd.c | 10 ++--------
>>>  1 file changed, 2 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
>>> index b5b059e83374..dd87b4e8e688 100644
>>> --- a/drivers/net/can/rcar/rcar_canfd.c
>>> +++ b/drivers/net/can/rcar/rcar_canfd.c
>>> @@ -102,12 +102,6 @@
>>>
>>>  /* Channel register bits */
>>>
>>> -/* RSCFDnCmCFG - Classical CAN only */
>>> -#define RCANFD_CFG_SJW(x)		(((x) & 0x3) << 24)
>>> -#define RCANFD_CFG_TSEG2(x)		(((x) & 0x7) << 20)
>>> -#define RCANFD_CFG_TSEG1(x)		(((x) & 0xf) << 16)
>>> -#define RCANFD_CFG_BRP(x)		(((x) & 0x3ff) << 0)
>>> -
>>>  /* RSCFDnCFDCmCTR / RSCFDnCmCTR */
>>>  #define RCANFD_CCTR_CTME		BIT(24)
>>>  #define RCANFD_CCTR_ERRD		BIT(23)
>>> @@ -1418,8 +1412,8 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
>>>  	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) || gpriv->info->shared_can_regs) {
>>>  		cfg = rcar_canfd_compute_nominal_bit_rate_cfg(gpriv->info, tseg1, brp, sjw, tseg2);
>>>  	} else {
>>> -		cfg = (RCANFD_CFG_TSEG1(tseg1) | RCANFD_CFG_BRP(brp) |
>>> -		       RCANFD_CFG_SJW(sjw) | RCANFD_CFG_TSEG2(tseg2));
>>> +		cfg = FIELD_PREP(GENMASK(19, 16), tseg1) | FIELD_PREP(GENMASK(9, 0), brp) |
>>> +		      FIELD_PREP(GENMASK(25, 24), sjw) | FIELD_PREP(GENMASK(22, 20), tseg2);
>>>  	}
>>
>> You can still keep the macro definition to give a meaning to the magic number:
>>
>>   #define RCANFD_CFG_SJW_MASK GENMASK(25, 24)
>>
>> and do a:
>>
>>   FIELD_PREP(RCANFD_CFG_SJW_MASK, sjw)
> 
> Are you ok for the below change to reduce the changes minimum??
> 
> -#define RCANFD_CFG_SJW(x)		(((x) & 0x3) << 24)
> +#define RCANFD_CFG_SJW(x)		FIELD_PREP(GENMASK(25, 24), x))

No more function-like macro please :)

Normal macro definitions are totally welcome. These allow to attach a name to
otherwise totally meaningless magic numbers.

When it goes to the actual calculation, functions are best.

> Or
> 
> you want RCANFD_CFG_SJW_MASK as separate one as you suggested?

This please :)


Yours sincerely,
Vincent Mailhol