[PATCH v2 0/5] can: rcar_canfd: Add support for V3U flavor

[PATCH v2 0/5] can: rcar_canfd: Add support for V3U flavor

[Ulrich Hecht]

Hi!

This adds CANFD support for V3U (R8A779A0) SoCs. The V3U's IP supports up to
eight channels and has some other minor differences to the Gen3 variety:

- changes to some register offsets and layouts
- absence of "classic CAN" registers, both modes are handled through the
  CANFD register set

This patch set tries to accommodate these changes in a minimally intrusive
way.

This revision attempts to address issues raised by reviewers to the extent
possible, adds board enablement, a missing clock and some minor fixes. See
below for details.

It has been sucessfully tested remotely on a V3U Falcon board, but only with
channels 0 and 1. We were not able to get higher channels to work in both
directions yet. It is not currently clear if this is an issue with the
driver, the board or the silicon, but the BSP vendor driver only works with
channels 0 and 1 as well, so my bet is on one of the latter. For this
reason, this series only enables known-working channels 0 and 1 on Falcon.

CU
Uli


Changes since v1:
- clk: added missing CANFD module clock
- driver: fixed tests for RZ/G2L so they won't break V3U
- driver: simplified two macros
- DT: enabled devices 0 and 1 on Falcon board
- DT: changed assigned-clock-rates to 80000000
- DT: added interrupt names


Ulrich Hecht (5):
  clk: renesas: r8a779a0: add CANFD module clock
  can: rcar_canfd: Add support for r8a779a0 SoC
  arm64: dts: renesas: r8a779a0: Add CANFD device node
  arm64: dts: renesas: r8a779a0-falcon: enable CANFD 0 and 1
  dt-bindings: can: renesas,rcar-canfd: Document r8a779a0 support

 .../bindings/net/can/renesas,rcar-canfd.yaml  |   2 +
 .../boot/dts/renesas/r8a779a0-falcon.dts      |  24 ++
 arch/arm64/boot/dts/renesas/r8a779a0.dtsi     |  56 +++++
 drivers/clk/renesas/r8a779a0-cpg-mssr.c       |   1 +
 drivers/net/can/rcar/rcar_canfd.c             | 231 ++++++++++++------
 5 files changed, 236 insertions(+), 78 deletions(-)

-- 
2.20.1

[PATCH v2 1/5] clk: renesas: r8a779a0: add CANFD module clock

[Ulrich Hecht]

Adds "canfd0" to mod clocks.

Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 drivers/clk/renesas/r8a779a0-cpg-mssr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/renesas/r8a779a0-cpg-mssr.c b/drivers/clk/renesas/r8a779a0-cpg-mssr.c
index 1c09d4ebe90f..fadd8a1718c6 100644
--- a/drivers/clk/renesas/r8a779a0-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a779a0-cpg-mssr.c
@@ -136,6 +136,7 @@ static const struct mssr_mod_clk r8a779a0_mod_clks[] __initconst = {
 	DEF_MOD("avb3",		214,	R8A779A0_CLK_S3D2),
 	DEF_MOD("avb4",		215,	R8A779A0_CLK_S3D2),
 	DEF_MOD("avb5",		216,	R8A779A0_CLK_S3D2),
+	DEF_MOD("canfd0",	328,	R8A779A0_CLK_CANFD),
 	DEF_MOD("csi40",	331,	R8A779A0_CLK_CSI0),
 	DEF_MOD("csi41",	400,	R8A779A0_CLK_CSI0),
 	DEF_MOD("csi42",	401,	R8A779A0_CLK_CSI0),
-- 
2.20.1

[PATCH v2 2/5] can: rcar_canfd: Add support for r8a779a0 SoC

[Ulrich Hecht]

Adds support for the CANFD IP variant in the V3U SoC.

Differences to controllers in other SoCs are limited to an increase in
the number of channels from two to eight, an absence of dedicated
registers for "classic" CAN mode, and a number of differences in magic
numbers (register offsets and layouts).

Inspired by BSP patch by Kazuya Mizuguchi.

Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 drivers/net/can/rcar/rcar_canfd.c | 231 ++++++++++++++++++++----------
 1 file changed, 153 insertions(+), 78 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index ff9d0f5ae0dd..b1c9870d2a82 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -44,10 +44,13 @@
 enum rcanfd_chip_id {
 	RENESAS_RCAR_GEN3 = 0,
 	RENESAS_RZG2L,
+	RENESAS_R8A779A0,
 };
 
 /* Global register bits */
 
+#define IS_V3U (gpriv->chip_id == RENESAS_R8A779A0)
+
 /* RSCFDnCFDGRMCFG */
 #define RCANFD_GRMCFG_RCMC		BIT(0)
 
@@ -79,6 +82,7 @@ enum rcanfd_chip_id {
 #define RCANFD_GSTS_GNOPM		(BIT(0) | BIT(1) | BIT(2) | BIT(3))
 
 /* RSCFDnCFDGERFL / RSCFDnGERFL */
+#define RCANFD_GERFL_EEF0_7		GENMASK(23, 16)
 #define RCANFD_GERFL_EEF1		BIT(17)
 #define RCANFD_GERFL_EEF0		BIT(16)
 #define RCANFD_GERFL_CMPOF		BIT(3)	/* CAN FD only */
@@ -86,20 +90,24 @@ enum rcanfd_chip_id {
 #define RCANFD_GERFL_MES		BIT(1)
 #define RCANFD_GERFL_DEF		BIT(0)
 
-#define RCANFD_GERFL_ERR(gpriv, x)	((x) & (RCANFD_GERFL_EEF1 |\
-					RCANFD_GERFL_EEF0 | RCANFD_GERFL_MES |\
-					(gpriv->fdmode ?\
-					 RCANFD_GERFL_CMPOF : 0)))
+#define RCANFD_GERFL_ERR(gpriv, x)	((x) & ((IS_V3U ? RCANFD_GERFL_EEF0_7 : \
+					(RCANFD_GERFL_EEF0 | RCANFD_GERFL_EEF1)) | \
+					RCANFD_GERFL_MES | ((gpriv)->fdmode ? \
+					RCANFD_GERFL_CMPOF : 0)))
 
 /* AFL Rx rules registers */
 
 /* RSCFDnCFDGAFLCFG0 / RSCFDnGAFLCFG0 */
-#define RCANFD_GAFLCFG_SETRNC(n, x)	(((x) & 0xff) << (24 - n * 8))
-#define RCANFD_GAFLCFG_GETRNC(n, x)	(((x) >> (24 - n * 8)) & 0xff)
+#define RCANFD_GAFLCFG_SETRNC(n, x)	(((x) & (IS_V3U ? 0x1ff : 0xff)) << \
+					 ((IS_V3U ? 16 : 24) - (n) * (IS_V3U ? \
+					 16 : 8)))
+#define RCANFD_GAFLCFG_GETRNC(n, x)	(((x) >> ((IS_V3U ? 16 : 24) - (n) * \
+					 (IS_V3U ? 16 : 8))) & \
+					 (IS_V3U ? 0x1ff : 0xff))
 
 /* RSCFDnCFDGAFLECTR / RSCFDnGAFLECTR */
 #define RCANFD_GAFLECTR_AFLDAE		BIT(8)
-#define RCANFD_GAFLECTR_AFLPN(x)	((x) & 0x1f)
+#define RCANFD_GAFLECTR_AFLPN(x)	((x) & (IS_V3U ? 0x7f : 0x1f))
 
 /* RSCFDnCFDGAFLIDj / RSCFDnGAFLIDj */
 #define RCANFD_GAFLID_GAFLLB		BIT(29)
@@ -116,9 +124,12 @@ enum rcanfd_chip_id {
 #define RCANFD_CFG_BRP(x)		(((x) & 0x3ff) << 0)
 
 /* RSCFDnCFDCmNCFG - CAN FD only */
-#define RCANFD_NCFG_NTSEG2(x)		(((x) & 0x1f) << 24)
-#define RCANFD_NCFG_NTSEG1(x)		(((x) & 0x7f) << 16)
-#define RCANFD_NCFG_NSJW(x)		(((x) & 0x1f) << 11)
+#define RCANFD_NCFG_NTSEG2(x)		(((x) & (IS_V3U ? 0x7f : 0x1f)) << \
+					 (IS_V3U ? 25 : 24))
+#define RCANFD_NCFG_NTSEG1(x)		(((x) & (IS_V3U ? 0xff : 0x7f)) << \
+					 (IS_V3U ? 17 : 16))
+#define RCANFD_NCFG_NSJW(x)		(((x) & (IS_V3U ? 0x7f : 0x1f)) << \
+					 (IS_V3U ? 10 : 11))
 #define RCANFD_NCFG_NBRP(x)		(((x) & 0x3ff) << 0)
 
 /* RSCFDnCFDCmCTR / RSCFDnCmCTR */
@@ -180,11 +191,15 @@ enum rcanfd_chip_id {
 
 /* RSCFDnCFDCmDCFG */
 #define RCANFD_DCFG_DSJW(x)		(((x) & 0x7) << 24)
-#define RCANFD_DCFG_DTSEG2(x)		(((x) & 0x7) << 20)
-#define RCANFD_DCFG_DTSEG1(x)		(((x) & 0xf) << 16)
+#define RCANFD_DCFG_DTSEG2(x)		(((x) & (IS_V3U ? 0x0f : 0x7)) << \
+					 (IS_V3U ? 16 : 20))
+#define RCANFD_DCFG_DTSEG1(x)		(((x) & (IS_V3U ? 0x1f : 0xf)) << \
+					 (IS_V3U ? 8 : 16))
 #define RCANFD_DCFG_DBRP(x)		(((x) & 0xff) << 0)
 
 /* RSCFDnCFDCmFDCFG */
+#define RCANFD_FDCFG_CLOE		BIT(30)
+#define RCANFD_FDCFG_FDOE		BIT(28)
 #define RCANFD_FDCFG_TDCE		BIT(9)
 #define RCANFD_FDCFG_TDCOC		BIT(8)
 #define RCANFD_FDCFG_TDCO(x)		(((x) & 0x7f) >> 16)
@@ -219,10 +234,10 @@ enum rcanfd_chip_id {
 /* Common FIFO bits */
 
 /* RSCFDnCFDCFCCk */
-#define RCANFD_CFCC_CFTML(x)		(((x) & 0xf) << 20)
-#define RCANFD_CFCC_CFM(x)		(((x) & 0x3) << 16)
+#define RCANFD_CFCC_CFTML(x)		(((x) & 0xf) << (IS_V3U ? 16 : 20))
+#define RCANFD_CFCC_CFM(x)		(((x) & 0x3) << (IS_V3U ? 8 : 16))
 #define RCANFD_CFCC_CFIM		BIT(12)
-#define RCANFD_CFCC_CFDC(x)		(((x) & 0x7) << 8)
+#define RCANFD_CFCC_CFDC(x)		(((x) & 0x7) << (IS_V3U ? 21 : 8))
 #define RCANFD_CFCC_CFPLS(x)		(((x) & 0x7) << 4)
 #define RCANFD_CFCC_CFTXIE		BIT(2)
 #define RCANFD_CFCC_CFE			BIT(0)
@@ -282,33 +297,32 @@ enum rcanfd_chip_id {
 #define RCANFD_GTSC			(0x0094)
 /* RSCFDnCFDGAFLECTR / RSCFDnGAFLECTR */
 #define RCANFD_GAFLECTR			(0x0098)
-/* RSCFDnCFDGAFLCFG0 / RSCFDnGAFLCFG0 */
-#define RCANFD_GAFLCFG0			(0x009c)
-/* RSCFDnCFDGAFLCFG1 / RSCFDnGAFLCFG1 */
-#define RCANFD_GAFLCFG1			(0x00a0)
+/* RSCFDnCFDGAFLCFG / RSCFDnGAFLCFG */
+#define RCANFD_GAFLCFG(ch)		(0x009c + (0x04 * ((ch) / 2)))
 /* RSCFDnCFDRMNB / RSCFDnRMNB */
 #define RCANFD_RMNB			(0x00a4)
 /* RSCFDnCFDRMND / RSCFDnRMND */
 #define RCANFD_RMND(y)			(0x00a8 + (0x04 * (y)))
 
 /* RSCFDnCFDRFCCx / RSCFDnRFCCx */
-#define RCANFD_RFCC(x)			(0x00b8 + (0x04 * (x)))
+#define RCANFD_RFCC(x)			((IS_V3U ? 0x00c0 : 0x00b8) + \
+					 (0x04 * (x)))
 /* RSCFDnCFDRFSTSx / RSCFDnRFSTSx */
-#define RCANFD_RFSTS(x)			(0x00d8 + (0x04 * (x)))
+#define RCANFD_RFSTS(x)			(RCANFD_RFCC(x) + 0x20)
 /* RSCFDnCFDRFPCTRx / RSCFDnRFPCTRx */
-#define RCANFD_RFPCTR(x)		(0x00f8 + (0x04 * (x)))
+#define RCANFD_RFPCTR(x)		(RCANFD_RFCC(x) + 0x40)
 
 /* Common FIFO Control registers */
 
 /* RSCFDnCFDCFCCx / RSCFDnCFCCx */
-#define RCANFD_CFCC(ch, idx)		(0x0118 + (0x0c * (ch)) + \
-					 (0x04 * (idx)))
+#define RCANFD_CFCC(ch, idx)		((IS_V3U ? 0x0120 : 0x0118) + \
+					 (0x0c * (ch)) + (0x04 * (idx)))
 /* RSCFDnCFDCFSTSx / RSCFDnCFSTSx */
-#define RCANFD_CFSTS(ch, idx)		(0x0178 + (0x0c * (ch)) + \
-					 (0x04 * (idx)))
+#define RCANFD_CFSTS(ch, idx)		((IS_V3U ? 0x01e0 : 0x0178) + \
+					 (0x0c * (ch)) + (0x04 * (idx)))
 /* RSCFDnCFDCFPCTRx / RSCFDnCFPCTRx */
-#define RCANFD_CFPCTR(ch, idx)		(0x01d8 + (0x0c * (ch)) + \
-					 (0x04 * (idx)))
+#define RCANFD_CFPCTR(ch, idx)		((IS_V3U ? 0x0240 : 0x01d8) + \
+					 (0x0c * (ch)) + (0x04 * (idx)))
 
 /* RSCFDnCFDFESTS / RSCFDnFESTS */
 #define RCANFD_FESTS			(0x0238)
@@ -415,6 +429,12 @@ enum rcanfd_chip_id {
 /* RSCFDnRPGACCr */
 #define RCANFD_C_RPGACC(r)		(0x1900 + (0x04 * (r)))
 
+/* R-Car V3U Classical and CAN FD mode specific register map */
+#define RCANFD_V3U_CFDCFG		(0x1314)
+#define RCANFD_V3U_DCFG(m)		(0x1400 + (0x20 * (m)))
+
+#define RCANFD_V3U_GAFL_OFFSET		(0x1800)
+
 /* CAN FD mode specific register map */
 
 /* RSCFDnCFDCmXXX -> RCANFD_F_XXX(m) */
@@ -434,7 +454,7 @@ enum rcanfd_chip_id {
 #define RCANFD_F_RMDF(q, b)		(0x200c + (0x04 * (b)) + (0x20 * (q)))
 
 /* RSCFDnCFDRFXXx -> RCANFD_F_RFXX(x) */
-#define RCANFD_F_RFOFFSET		(0x3000)
+#define RCANFD_F_RFOFFSET		(IS_V3U ? 0x6000 : 0x3000)
 #define RCANFD_F_RFID(x)		(RCANFD_F_RFOFFSET + (0x80 * (x)))
 #define RCANFD_F_RFPTR(x)		(RCANFD_F_RFOFFSET + 0x04 + \
 					 (0x80 * (x)))
@@ -444,7 +464,7 @@ enum rcanfd_chip_id {
 					 (0x80 * (x)) + (0x04 * (df)))
 
 /* RSCFDnCFDCFXXk -> RCANFD_F_CFXX(ch, k) */
-#define RCANFD_F_CFOFFSET		(0x3400)
+#define RCANFD_F_CFOFFSET		(IS_V3U ? 0x6400 : 0x3400)
 #define RCANFD_F_CFID(ch, idx)		(RCANFD_F_CFOFFSET + (0x180 * (ch)) + \
 					 (0x80 * (idx)))
 #define RCANFD_F_CFPTR(ch, idx)		(RCANFD_F_CFOFFSET + 0x04 + \
@@ -470,7 +490,7 @@ enum rcanfd_chip_id {
 #define RCANFD_FIFO_DEPTH		8	/* Tx FIFO depth */
 #define RCANFD_NAPI_WEIGHT		8	/* Rx poll quota */
 
-#define RCANFD_NUM_CHANNELS		2	/* Two channels max */
+#define RCANFD_NUM_CHANNELS		8	/* Eight channels max */
 #define RCANFD_CHANNELS_MASK		BIT((RCANFD_NUM_CHANNELS) - 1)
 
 #define RCANFD_GAFL_PAGENUM(entry)	((entry) / 16)
@@ -522,6 +542,7 @@ struct rcar_canfd_global {
 	struct reset_control *rstc1;
 	struct reset_control *rstc2;
 	enum rcanfd_chip_id chip_id;
+	u32 max_channels;
 };
 
 /* CAN FD mode nominal rate constants */
@@ -629,6 +650,25 @@ static void rcar_canfd_tx_failure_cleanup(struct net_device *ndev)
 		can_free_echo_skb(ndev, i, NULL);
 }
 
+static void rcar_canfd_set_mode(struct rcar_canfd_global *gpriv)
+{
+	if (gpriv->chip_id == RENESAS_R8A779A0) {
+		if (gpriv->fdmode)
+			rcar_canfd_set_bit(gpriv->base, RCANFD_V3U_CFDCFG,
+					   RCANFD_FDCFG_FDOE);
+		else
+			rcar_canfd_set_bit(gpriv->base, RCANFD_V3U_CFDCFG,
+					   RCANFD_FDCFG_CLOE);
+	} else {
+		if (gpriv->fdmode)
+			rcar_canfd_set_bit(gpriv->base, RCANFD_GRMCFG,
+					   RCANFD_GRMCFG_RCMC);
+		else
+			rcar_canfd_clear_bit(gpriv->base, RCANFD_GRMCFG,
+					     RCANFD_GRMCFG_RCMC);
+	}
+}
+
 static int rcar_canfd_reset_controller(struct rcar_canfd_global *gpriv)
 {
 	u32 sts, ch;
@@ -661,15 +701,10 @@ static int rcar_canfd_reset_controller(struct rcar_canfd_global *gpriv)
 	rcar_canfd_write(gpriv->base, RCANFD_GERFL, 0x0);
 
 	/* Set the controller into appropriate mode */
-	if (gpriv->fdmode)
-		rcar_canfd_set_bit(gpriv->base, RCANFD_GRMCFG,
-				   RCANFD_GRMCFG_RCMC);
-	else
-		rcar_canfd_clear_bit(gpriv->base, RCANFD_GRMCFG,
-				     RCANFD_GRMCFG_RCMC);
+	rcar_canfd_set_mode(gpriv);
 
 	/* Transition all Channels to reset mode */
-	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
+	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) {
 		rcar_canfd_clear_bit(gpriv->base,
 				     RCANFD_CCTR(ch), RCANFD_CCTR_CSLPR);
 
@@ -710,7 +745,7 @@ static void rcar_canfd_configure_controller(struct rcar_canfd_global *gpriv)
 	rcar_canfd_set_bit(gpriv->base, RCANFD_GCFG, cfg);
 
 	/* Channel configuration settings */
-	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
+	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) {
 		rcar_canfd_set_bit(gpriv->base, RCANFD_CCTR(ch),
 				   RCANFD_CCTR_ERRD);
 		rcar_canfd_update_bit(gpriv->base, RCANFD_CCTR(ch),
@@ -730,7 +765,7 @@ static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
 		start = 0; /* Channel 0 always starts from 0th rule */
 	} else {
 		/* Get number of Channel 0 rules and adjust */
-		cfg = rcar_canfd_read(gpriv->base, RCANFD_GAFLCFG0);
+		cfg = rcar_canfd_read(gpriv->base, RCANFD_GAFLCFG(ch));
 		start = RCANFD_GAFLCFG_GETRNC(0, cfg);
 	}
 
@@ -741,12 +776,16 @@ static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
 			    RCANFD_GAFLECTR_AFLDAE));
 
 	/* Write number of rules for channel */
-	rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG0,
+	rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG(ch),
 			   RCANFD_GAFLCFG_SETRNC(ch, num_rules));
-	if (gpriv->fdmode)
-		offset = RCANFD_F_GAFL_OFFSET;
-	else
-		offset = RCANFD_C_GAFL_OFFSET;
+	if (gpriv->chip_id == RENESAS_R8A779A0) {
+		offset = RCANFD_V3U_GAFL_OFFSET;
+	} else {
+		if (gpriv->fdmode)
+			offset = RCANFD_F_GAFL_OFFSET;
+		else
+			offset = RCANFD_C_GAFL_OFFSET;
+	}
 
 	/* Accept all IDs */
 	rcar_canfd_write(gpriv->base, RCANFD_GAFLID(offset, start), 0);
@@ -755,8 +794,8 @@ static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
 	/* Any data length accepted */
 	rcar_canfd_write(gpriv->base, RCANFD_GAFLP0(offset, start), 0);
 	/* Place the msg in corresponding Rx FIFO entry */
-	rcar_canfd_write(gpriv->base, RCANFD_GAFLP1(offset, start),
-			 RCANFD_GAFLP1_GAFLFDP(ridx));
+	rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLP1(offset, start),
+			   RCANFD_GAFLP1_GAFLFDP(ridx));
 
 	/* Disable write access to page */
 	rcar_canfd_clear_bit(gpriv->base,
@@ -1041,6 +1080,7 @@ static void rcar_canfd_error(struct net_device *ndev, u32 cerfl,
 static void rcar_canfd_tx_done(struct net_device *ndev)
 {
 	struct rcar_canfd_channel *priv = netdev_priv(ndev);
+	struct rcar_canfd_global *gpriv = priv->gpriv;
 	struct net_device_stats *stats = &ndev->stats;
 	u32 sts;
 	unsigned long flags;
@@ -1096,7 +1136,7 @@ static irqreturn_t rcar_canfd_global_err_interrupt(int irq, void *dev_id)
 	struct rcar_canfd_global *gpriv = dev_id;
 	u32 ch;
 
-	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS)
+	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels)
 		rcar_canfd_handle_global_err(gpriv, ch);
 
 	return IRQ_HANDLED;
@@ -1126,7 +1166,7 @@ static irqreturn_t rcar_canfd_global_receive_fifo_interrupt(int irq, void *dev_i
 	struct rcar_canfd_global *gpriv = dev_id;
 	u32 ch;
 
-	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS)
+	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels)
 		rcar_canfd_handle_global_receive(gpriv, ch);
 
 	return IRQ_HANDLED;
@@ -1140,7 +1180,7 @@ static irqreturn_t rcar_canfd_global_interrupt(int irq, void *dev_id)
 	/* Global error interrupts still indicate a condition specific
 	 * to a channel. RxFIFO interrupt is a global interrupt.
 	 */
-	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
+	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) {
 		rcar_canfd_handle_global_err(gpriv, ch);
 		rcar_canfd_handle_global_receive(gpriv, ch);
 	}
@@ -1198,7 +1238,7 @@ static irqreturn_t rcar_canfd_channel_tx_interrupt(int irq, void *dev_id)
 	struct rcar_canfd_global *gpriv = dev_id;
 	u32 ch;
 
-	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS)
+	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels)
 		rcar_canfd_handle_channel_tx(gpriv, ch);
 
 	return IRQ_HANDLED;
@@ -1230,7 +1270,7 @@ static irqreturn_t rcar_canfd_channel_err_interrupt(int irq, void *dev_id)
 	struct rcar_canfd_global *gpriv = dev_id;
 	u32 ch;
 
-	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS)
+	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels)
 		rcar_canfd_handle_channel_err(gpriv, ch);
 
 	return IRQ_HANDLED;
@@ -1242,7 +1282,7 @@ static irqreturn_t rcar_canfd_channel_interrupt(int irq, void *dev_id)
 	u32 ch;
 
 	/* Common FIFO is a per channel resource */
-	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
+	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) {
 		rcar_canfd_handle_channel_err(gpriv, ch);
 		rcar_canfd_handle_channel_tx(gpriv, ch);
 	}
@@ -1253,6 +1293,7 @@ static irqreturn_t rcar_canfd_channel_interrupt(int irq, void *dev_id)
 static void rcar_canfd_set_bittiming(struct net_device *dev)
 {
 	struct rcar_canfd_channel *priv = netdev_priv(dev);
+	struct rcar_canfd_global *gpriv = priv->gpriv;
 	const struct can_bittiming *bt = &priv->can.bittiming;
 	const struct can_bittiming *dbt = &priv->can.data_bittiming;
 	u16 brp, sjw, tseg1, tseg2;
@@ -1288,8 +1329,17 @@ static void rcar_canfd_set_bittiming(struct net_device *dev)
 			   brp, sjw, tseg1, tseg2);
 	} else {
 		/* Classical CAN only mode */
-		cfg = (RCANFD_CFG_TSEG1(tseg1) | RCANFD_CFG_BRP(brp) |
-			RCANFD_CFG_SJW(sjw) | RCANFD_CFG_TSEG2(tseg2));
+		if (gpriv->chip_id == RENESAS_R8A779A0) {
+			cfg = (RCANFD_NCFG_NTSEG1(tseg1) |
+			       RCANFD_NCFG_NBRP(brp) |
+			       RCANFD_NCFG_NSJW(sjw) |
+			       RCANFD_NCFG_NTSEG2(tseg2));
+		} else {
+			cfg = (RCANFD_CFG_TSEG1(tseg1) |
+			       RCANFD_CFG_BRP(brp) |
+			       RCANFD_CFG_SJW(sjw) |
+			       RCANFD_CFG_TSEG2(tseg2));
+		}
 
 		rcar_canfd_write(priv->base, RCANFD_CCFG(ch), cfg);
 		netdev_dbg(priv->ndev,
@@ -1301,6 +1351,7 @@ static void rcar_canfd_set_bittiming(struct net_device *dev)
 static int rcar_canfd_start(struct net_device *ndev)
 {
 	struct rcar_canfd_channel *priv = netdev_priv(ndev);
+	struct rcar_canfd_global *gpriv = priv->gpriv;
 	int err = -EOPNOTSUPP;
 	u32 sts, ch = priv->channel;
 	u32 ridx = ch + RCANFD_RFFIFO_IDX;
@@ -1372,6 +1423,7 @@ static int rcar_canfd_open(struct net_device *ndev)
 static void rcar_canfd_stop(struct net_device *ndev)
 {
 	struct rcar_canfd_channel *priv = netdev_priv(ndev);
+	struct rcar_canfd_global *gpriv = priv->gpriv;
 	int err;
 	u32 sts, ch = priv->channel;
 	u32 ridx = ch + RCANFD_RFFIFO_IDX;
@@ -1415,6 +1467,7 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
 					 struct net_device *ndev)
 {
 	struct rcar_canfd_channel *priv = netdev_priv(ndev);
+	struct rcar_canfd_global *gpriv = priv->gpriv;
 	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
 	u32 sts = 0, id, dlc;
 	unsigned long flags;
@@ -1435,13 +1488,15 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
 
 	dlc = RCANFD_CFPTR_CFDLC(can_fd_len2dlc(cf->len));
 
-	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
+	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
+	    gpriv->chip_id == RENESAS_R8A779A0) {
 		rcar_canfd_write(priv->base,
 				 RCANFD_F_CFID(ch, RCANFD_CFFIFO_IDX), id);
 		rcar_canfd_write(priv->base,
 				 RCANFD_F_CFPTR(ch, RCANFD_CFFIFO_IDX), dlc);
 
-		if (can_is_canfd_skb(skb)) {
+		if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
+		    can_is_canfd_skb(skb)) {
 			/* CAN FD frame format */
 			sts |= RCANFD_CFFDCSTS_CFFDF;
 			if (cf->flags & CANFD_BRS)
@@ -1488,22 +1543,29 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
 static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
 {
 	struct net_device_stats *stats = &priv->ndev->stats;
+	struct rcar_canfd_global *gpriv = priv->gpriv;
 	struct canfd_frame *cf;
 	struct sk_buff *skb;
 	u32 sts = 0, id, dlc;
 	u32 ch = priv->channel;
 	u32 ridx = ch + RCANFD_RFFIFO_IDX;
 
-	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
+	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
+	    gpriv->chip_id == RENESAS_R8A779A0) {
 		id = rcar_canfd_read(priv->base, RCANFD_F_RFID(ridx));
 		dlc = rcar_canfd_read(priv->base, RCANFD_F_RFPTR(ridx));
 
 		sts = rcar_canfd_read(priv->base, RCANFD_F_RFFDSTS(ridx));
-		if (sts & RCANFD_RFFDSTS_RFFDF)
-			skb = alloc_canfd_skb(priv->ndev, &cf);
-		else
+		if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
+			if (sts & RCANFD_RFFDSTS_RFFDF)
+				skb = alloc_canfd_skb(priv->ndev, &cf);
+			else
+				skb = alloc_can_skb(priv->ndev,
+						    (struct can_frame **)&cf);
+		} else {
 			skb = alloc_can_skb(priv->ndev,
 					    (struct can_frame **)&cf);
+		}
 	} else {
 		id = rcar_canfd_read(priv->base, RCANFD_C_RFID(ridx));
 		dlc = rcar_canfd_read(priv->base, RCANFD_C_RFPTR(ridx));
@@ -1541,10 +1603,16 @@ static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
 		}
 	} else {
 		cf->len = can_cc_dlc2len(RCANFD_RFPTR_RFDLC(dlc));
-		if (id & RCANFD_RFID_RFRTR)
+		if (id & RCANFD_RFID_RFRTR) {
 			cf->can_id |= CAN_RTR_FLAG;
-		else
-			rcar_canfd_get_data(priv, cf, RCANFD_C_RFDF(ridx, 0));
+		} else {
+			if (gpriv->chip_id == RENESAS_R8A779A0)
+				rcar_canfd_get_data(priv, cf,
+						    RCANFD_F_RFDF(ridx, 0));
+			else
+				rcar_canfd_get_data(priv, cf,
+						    RCANFD_C_RFDF(ridx, 0));
+		}
 	}
 
 	/* Write 0xff to RFPC to increment the CPU-side
@@ -1563,6 +1631,7 @@ static int rcar_canfd_rx_poll(struct napi_struct *napi, int quota)
 {
 	struct rcar_canfd_channel *priv =
 		container_of(napi, struct rcar_canfd_channel, napi);
+	struct rcar_canfd_global *gpriv = priv->gpriv;
 	int num_pkts;
 	u32 sts;
 	u32 ch = priv->channel;
@@ -1762,21 +1831,25 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 	int g_err_irq, g_recc_irq;
 	bool fdmode = true;			/* CAN FD only mode - default */
 	enum rcanfd_chip_id chip_id;
+	int max_channels;
+	char name[9];
+	int i;
 
 	chip_id = (uintptr_t)of_device_get_match_data(&pdev->dev);
+	max_channels = chip_id == RENESAS_R8A779A0 ? 8 : 2;
 
 	if (of_property_read_bool(pdev->dev.of_node, "renesas,no-can-fd"))
 		fdmode = false;			/* Classical CAN only mode */
 
-	of_child = of_get_child_by_name(pdev->dev.of_node, "channel0");
-	if (of_child && of_device_is_available(of_child))
-		channels_mask |= BIT(0);	/* Channel 0 */
-
-	of_child = of_get_child_by_name(pdev->dev.of_node, "channel1");
-	if (of_child && of_device_is_available(of_child))
-		channels_mask |= BIT(1);	/* Channel 1 */
+	strcpy(name, "channelX");
+	for (i = 0; i < max_channels; ++i) {
+		name[7] = '0' + i;
+		of_child = of_get_child_by_name(pdev->dev.of_node, name);
+		if (of_child && of_device_is_available(of_child))
+			channels_mask |= BIT(i);
+	}
 
-	if (chip_id == RENESAS_RCAR_GEN3) {
+	if (chip_id != RENESAS_RZG2L) {
 		ch_irq = platform_get_irq_byname_optional(pdev, "ch_int");
 		if (ch_irq < 0) {
 			/* For backward compatibility get irq by index */
@@ -1812,6 +1885,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 	gpriv->channels_mask = channels_mask;
 	gpriv->fdmode = fdmode;
 	gpriv->chip_id = chip_id;
+	gpriv->max_channels = max_channels;
 
 	if (gpriv->chip_id == RENESAS_RZG2L) {
 		gpriv->rstc1 = devm_reset_control_get_exclusive(&pdev->dev, "rstp_n");
@@ -1853,7 +1927,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 	}
 	fcan_freq = clk_get_rate(gpriv->can_clk);
 
-	if (gpriv->fcan == RCANFD_CANFDCLK && gpriv->chip_id == RENESAS_RCAR_GEN3)
+	if (gpriv->fcan == RCANFD_CANFDCLK && gpriv->chip_id != RENESAS_RZG2L)
 		/* CANFD clock is further divided by (1/2) within the IP */
 		fcan_freq /= 2;
 
@@ -1865,7 +1939,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 	gpriv->base = addr;
 
 	/* Request IRQ that's common for both channels */
-	if (gpriv->chip_id == RENESAS_RCAR_GEN3) {
+	if (gpriv->chip_id != RENESAS_RZG2L) {
 		err = devm_request_irq(&pdev->dev, ch_irq,
 				       rcar_canfd_channel_interrupt, 0,
 				       "canfd.ch_int", gpriv);
@@ -1931,7 +2005,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 	rcar_canfd_configure_controller(gpriv);
 
 	/* Configure per channel attributes */
-	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
+	for_each_set_bit(ch, &gpriv->channels_mask, max_channels) {
 		/* Configure Channel's Rx fifo */
 		rcar_canfd_configure_rx(gpriv, ch);
 
@@ -1957,7 +2031,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 		goto fail_mode;
 	}
 
-	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
+	for_each_set_bit(ch, &gpriv->channels_mask, max_channels) {
 		err = rcar_canfd_channel_probe(gpriv, ch, fcan_freq);
 		if (err)
 			goto fail_channel;
@@ -1969,7 +2043,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 	return 0;
 
 fail_channel:
-	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS)
+	for_each_set_bit(ch, &gpriv->channels_mask, max_channels)
 		rcar_canfd_channel_remove(gpriv, ch);
 fail_mode:
 	rcar_canfd_disable_global_interrupts(gpriv);
@@ -1990,7 +2064,7 @@ static int rcar_canfd_remove(struct platform_device *pdev)
 	rcar_canfd_reset_controller(gpriv);
 	rcar_canfd_disable_global_interrupts(gpriv);
 
-	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
+	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) {
 		rcar_canfd_disable_channel_interrupts(gpriv->ch[ch]);
 		rcar_canfd_channel_remove(gpriv, ch);
 	}
@@ -2020,6 +2094,7 @@ static SIMPLE_DEV_PM_OPS(rcar_canfd_pm_ops, rcar_canfd_suspend,
 static const __maybe_unused struct of_device_id rcar_canfd_of_table[] = {
 	{ .compatible = "renesas,rcar-gen3-canfd", .data = (void *)RENESAS_RCAR_GEN3 },
 	{ .compatible = "renesas,rzg2l-canfd", .data = (void *)RENESAS_RZG2L },
+	{ .compatible = "renesas,r8a779a0-canfd", .data = (void *)RENESAS_R8A779A0 },
 	{ }
 };
 
-- 
2.20.1

[PATCH v2 3/5] arm64: dts: renesas: r8a779a0: Add CANFD device node

[Ulrich Hecht]

This patch adds a CANFD device node for r8a779a0.

Based on patch by Kazuya Mizuguchi.

Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 arch/arm64/boot/dts/renesas/r8a779a0.dtsi | 56 +++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a779a0.dtsi b/arch/arm64/boot/dts/renesas/r8a779a0.dtsi
index 1e7ed12ebc87..dfde33fa40e3 100644
--- a/arch/arm64/boot/dts/renesas/r8a779a0.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a779a0.dtsi
@@ -24,6 +24,13 @@
 		i2c6 = &i2c6;
 	};
 
+	/* External CAN clock - to be overridden by boards that provide it */
+	can_clk: can {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <0>;
+	};
+
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
@@ -581,6 +588,55 @@
 			status = "disabled";
 		};
 
+		canfd: can@e6660000 {
+			compatible = "renesas,r8a779a0-canfd";
+			reg = <0 0xe6660000 0 0x8000>;
+			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>,
+					<GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "ch_int", "g_int";
+			clocks = <&cpg CPG_MOD 328>,
+				 <&cpg CPG_CORE R8A779A0_CLK_CANFD>,
+				 <&can_clk>;
+			clock-names = "fck", "canfd", "can_clk";
+			assigned-clocks = <&cpg CPG_CORE R8A779A0_CLK_CANFD>;
+			assigned-clock-rates = <80000000>;
+			power-domains = <&sysc R8A779A0_PD_ALWAYS_ON>;
+			resets = <&cpg 328>;
+			status = "disabled";
+
+			channel0 {
+				status = "disabled";
+			};
+
+			channel1 {
+				status = "disabled";
+			};
+
+			channel2 {
+				status = "disabled";
+			};
+
+			channel3 {
+				status = "disabled";
+			};
+
+			channel4 {
+				status = "disabled";
+			};
+
+			channel5 {
+				status = "disabled";
+			};
+
+			channel6 {
+				status = "disabled";
+			};
+
+			channel7 {
+				status = "disabled";
+			};
+		};
+
 		avb0: ethernet@e6800000 {
 			compatible = "renesas,etheravb-r8a779a0",
 				     "renesas,etheravb-rcar-gen3";
-- 
2.20.1

[PATCH v2 4/5] arm64: dts: renesas: r8a779a0-falcon: enable CANFD 0 and 1

[Ulrich Hecht]

Enables confirmed-working CAN interfaces 0 and 1 on the Falcon board.

Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 .../boot/dts/renesas/r8a779a0-falcon.dts      | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts b/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
index e46dc9aa0a43..75e27f950ac0 100644
--- a/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
+++ b/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
@@ -65,4 +65,28 @@
 		};
 
 	};
+
+	canfd0_pins: canfd0 {
+		groups = "canfd0_data";
+		function = "canfd0";
+	};
+
+	canfd1_pins: canfd1 {
+		groups = "canfd1_data";
+		function = "canfd1";
+	};
+};
+
+&canfd {
+	pinctrl-0 = <&canfd0_pins>, <&canfd1_pins>;
+	pinctrl-names = "default";
+	status = "okay";
+
+	channel0 {
+		status = "okay";
+	};
+
+	channel1 {
+		status = "okay";
+	};
 };
-- 
2.20.1

[PATCH v2 5/5] dt-bindings: can: renesas,rcar-canfd: Document r8a779a0 support

[Ulrich Hecht]

Document support for rcar_canfd on R8A779A0 (V3U) SoCs.

Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 .../devicetree/bindings/net/can/renesas,rcar-canfd.yaml         | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
index 546c6e6d2fb0..91a3554ca950 100644
--- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
+++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
@@ -35,6 +35,8 @@ properties:
               - renesas,r9a07g044-canfd    # RZ/G2{L,LC}
           - const: renesas,rzg2l-canfd     # RZ/G2L family
 
+      - const: renesas,r8a779a0-canfd      # R-Car V3U
+
   reg:
     maxItems: 1
 
-- 
2.20.1

Re: [PATCH v2 1/5] clk: renesas: r8a779a0: add CANFD module clock

[Geert Uytterhoeven]

Hi Uli,

On Tue, Jan 11, 2022 at 5:22 PM Ulrich Hecht <uli+renesas@fpond.eu> wrote:
> Adds "canfd0" to mod clocks.
>
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>

Thanks for your patch!

> --- a/drivers/clk/renesas/r8a779a0-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a779a0-cpg-mssr.c
> @@ -136,6 +136,7 @@ static const struct mssr_mod_clk r8a779a0_mod_clks[] __initconst = {
>         DEF_MOD("avb3",         214,    R8A779A0_CLK_S3D2),
>         DEF_MOD("avb4",         215,    R8A779A0_CLK_S3D2),
>         DEF_MOD("avb5",         216,    R8A779A0_CLK_S3D2),
> +       DEF_MOD("canfd0",       328,    R8A779A0_CLK_CANFD),

The datasheet calls this "canfd".

>         DEF_MOD("csi40",        331,    R8A779A0_CLK_CSI0),
>         DEF_MOD("csi41",        400,    R8A779A0_CLK_CSI0),
>         DEF_MOD("csi42",        401,    R8A779A0_CLK_CSI0),

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-clk-for-v5.18 with the above fixed.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 1/5] clk: renesas: r8a779a0: add CANFD module clock

[Ulrich Hecht]

> On 01/12/2022 9:44 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/r8a779a0-cpg-mssr.c
> > +++ b/drivers/clk/renesas/r8a779a0-cpg-mssr.c
> > @@ -136,6 +136,7 @@ static const struct mssr_mod_clk r8a779a0_mod_clks[] __initconst = {
> >         DEF_MOD("avb3",         214,    R8A779A0_CLK_S3D2),
> >         DEF_MOD("avb4",         215,    R8A779A0_CLK_S3D2),
> >         DEF_MOD("avb5",         216,    R8A779A0_CLK_S3D2),
> > +       DEF_MOD("canfd0",       328,    R8A779A0_CLK_CANFD),
> 
> The datasheet calls this "canfd".
> 
> >         DEF_MOD("csi40",        331,    R8A779A0_CLK_CSI0),
> >         DEF_MOD("csi41",        400,    R8A779A0_CLK_CSI0),
> >         DEF_MOD("csi42",        401,    R8A779A0_CLK_CSI0),
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> i.e. will queue in renesas-clk-for-v5.18 with the above fixed.

Don't do that! There already is a DIV4 clock called "canfd", and using that name twice breaks stuff. The BSP calls this clock "can-fd" for that reason.

CU
Uli

Re: [PATCH v2 1/5] clk: renesas: r8a779a0: add CANFD module clock

[Geert Uytterhoeven]

Hi Uli,

On Wed, Jan 12, 2022 at 9:56 AM Ulrich Hecht <uli@fpond.eu> wrote:
> > On 01/12/2022 9:44 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > --- a/drivers/clk/renesas/r8a779a0-cpg-mssr.c
> > > +++ b/drivers/clk/renesas/r8a779a0-cpg-mssr.c
> > > @@ -136,6 +136,7 @@ static const struct mssr_mod_clk r8a779a0_mod_clks[] __initconst = {
> > >         DEF_MOD("avb3",         214,    R8A779A0_CLK_S3D2),
> > >         DEF_MOD("avb4",         215,    R8A779A0_CLK_S3D2),
> > >         DEF_MOD("avb5",         216,    R8A779A0_CLK_S3D2),
> > > +       DEF_MOD("canfd0",       328,    R8A779A0_CLK_CANFD),
> >
> > The datasheet calls this "canfd".
> >
> > >         DEF_MOD("csi40",        331,    R8A779A0_CLK_CSI0),
> > >         DEF_MOD("csi41",        400,    R8A779A0_CLK_CSI0),
> > >         DEF_MOD("csi42",        401,    R8A779A0_CLK_CSI0),
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > i.e. will queue in renesas-clk-for-v5.18 with the above fixed.
>
> Don't do that! There already is a DIV4 clock called "canfd", and using that name twice breaks stuff. The BSP calls this clock "can-fd" for that reason.

Thanks, I stand corrected.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 2/5] can: rcar_canfd: Add support for r8a779a0 SoC

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]

On 11.01.2022 17:22:28, Ulrich Hecht wrote:
> Adds support for the CANFD IP variant in the V3U SoC.
> 
> Differences to controllers in other SoCs are limited to an increase in
> the number of channels from two to eight, an absence of dedicated
> registers for "classic" CAN mode, and a number of differences in magic
> numbers (register offsets and layouts).
> 
> Inspired by BSP patch by Kazuya Mizuguchi.
> 
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> ---
>  drivers/net/can/rcar/rcar_canfd.c | 231 ++++++++++++++++++++----------
>  1 file changed, 153 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index ff9d0f5ae0dd..b1c9870d2a82 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -44,10 +44,13 @@
>  enum rcanfd_chip_id {
>  	RENESAS_RCAR_GEN3 = 0,
>  	RENESAS_RZG2L,
> +	RENESAS_R8A779A0,
>  };
>  
>  /* Global register bits */
>  
> +#define IS_V3U (gpriv->chip_id == RENESAS_R8A779A0)

I really don't like this macro, as it silently relies on gpriv....and
I really don't like this use of this macro in the other macros that lead
to 2 or even 3 ternary operators hiding inside them. Is there any chance
to change this?

Please add at least the gpriv argument to IS_V3U().....

[...]

> -	of_child = of_get_child_by_name(pdev->dev.of_node, "channel1");
> -	if (of_child && of_device_is_available(of_child))
> -		channels_mask |= BIT(1);	/* Channel 1 */
> +	strcpy(name, "channelX");

please use strlcpy()

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/5] can: rcar_canfd: Add support for r8a779a0 SoC

[Ulrich Hecht]

> On 01/12/2022 7:43 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > +#define IS_V3U (gpriv->chip_id == RENESAS_R8A779A0)
> 
> I really don't like this macro, as it silently relies on gpriv....and
> I really don't like this use of this macro in the other macros that lead
> to 2 or even 3 ternary operators hiding inside them. Is there any chance
> to change this?

Good point. I guess I should turn that into a function.

CU
Uli

Re: [PATCH v2 2/5] can: rcar_canfd: Add support for r8a779a0 SoC

[Geert Uytterhoeven]

Hi Marc,

On Wed, Jan 12, 2022 at 7:43 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 11.01.2022 17:22:28, Ulrich Hecht wrote:
> > Adds support for the CANFD IP variant in the V3U SoC.
> >
> > Differences to controllers in other SoCs are limited to an increase in
> > the number of channels from two to eight, an absence of dedicated
> > registers for "classic" CAN mode, and a number of differences in magic
> > numbers (register offsets and layouts).
> >
> > Inspired by BSP patch by Kazuya Mizuguchi.
> >
> > Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>

> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c

> > -     of_child = of_get_child_by_name(pdev->dev.of_node, "channel1");
> > -     if (of_child && of_device_is_available(of_child))
> > -             channels_mask |= BIT(1);        /* Channel 1 */
> > +     strcpy(name, "channelX");
>
> please use strlcpy()

Why? To cause a silent failure instead of a possible crash, in the unlikely
case the buffer is shrinked or the string is enlarged?

What about preinitializing it at declaration time instead:

    char name[9] = "channelX";

?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 3/5] arm64: dts: renesas: r8a779a0: Add CANFD device node

[Geert Uytterhoeven]

On Tue, Jan 11, 2022 at 5:22 PM Ulrich Hecht <uli+renesas@fpond.eu> wrote:
> This patch adds a CANFD device node for r8a779a0.
>
> Based on patch by Kazuya Mizuguchi.
>
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 4/5] arm64: dts: renesas: r8a779a0-falcon: enable CANFD 0 and 1

[Geert Uytterhoeven]

Hi Uli,

On Tue, Jan 11, 2022 at 5:22 PM Ulrich Hecht <uli+renesas@fpond.eu> wrote:
> Enables confirmed-working CAN interfaces 0 and 1 on the Falcon board.
>
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Perhaps you want to describe can_clk (40 MHz) and its pinctrl, too?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 5/5] dt-bindings: can: renesas,rcar-canfd: Document r8a779a0 support

[Geert Uytterhoeven]

On Tue, Jan 11, 2022 at 5:22 PM Ulrich Hecht <uli+renesas@fpond.eu> wrote:
> Document support for rcar_canfd on R8A779A0 (V3U) SoCs.

R-Car V3U

> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 2/5] can: rcar_canfd: Add support for r8a779a0 SoC

[Vincent MAILHOL]

  Hi Ulrich,

On Wed. 12 Jan 2022 at 01:22, Ulrich Hecht <uli+renesas@fpond.eu> a wrote:
>
> Adds support for the CANFD IP variant in the V3U SoC.
>
> Differences to controllers in other SoCs are limited to an increase in
> the number of channels from two to eight, an absence of dedicated
> registers for "classic" CAN mode, and a number of differences in magic
> numbers (register offsets and layouts).
>
> Inspired by BSP patch by Kazuya Mizuguchi.
>
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> ---
>  drivers/net/can/rcar/rcar_canfd.c | 231 ++++++++++++++++++++----------
>  1 file changed, 153 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index ff9d0f5ae0dd..b1c9870d2a82 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -44,10 +44,13 @@
>  enum rcanfd_chip_id {
>         RENESAS_RCAR_GEN3 = 0,
>         RENESAS_RZG2L,
> +       RENESAS_R8A779A0,
>  };
>
>  /* Global register bits */
>
> +#define IS_V3U (gpriv->chip_id == RENESAS_R8A779A0)
> +
>  /* RSCFDnCFDGRMCFG */
>  #define RCANFD_GRMCFG_RCMC             BIT(0)
>
> @@ -79,6 +82,7 @@ enum rcanfd_chip_id {
>  #define RCANFD_GSTS_GNOPM              (BIT(0) | BIT(1) | BIT(2) | BIT(3))
>
>  /* RSCFDnCFDGERFL / RSCFDnGERFL */
> +#define RCANFD_GERFL_EEF0_7            GENMASK(23, 16)
>  #define RCANFD_GERFL_EEF1              BIT(17)
>  #define RCANFD_GERFL_EEF0              BIT(16)
>  #define RCANFD_GERFL_CMPOF             BIT(3)  /* CAN FD only */
> @@ -86,20 +90,24 @@ enum rcanfd_chip_id {
>  #define RCANFD_GERFL_MES               BIT(1)
>  #define RCANFD_GERFL_DEF               BIT(0)
>

I have a couple of comments *in addition* to Mark’s one on the IS_V3U.

>
> -#define RCANFD_GERFL_ERR(gpriv, x)     ((x) & (RCANFD_GERFL_EEF1 |\
> -                                       RCANFD_GERFL_EEF0 | RCANFD_GERFL_MES |\
> -                                       (gpriv->fdmode ?\
> -                                        RCANFD_GERFL_CMPOF : 0)))
> +#define RCANFD_GERFL_ERR(gpriv, x)     ((x) & ((IS_V3U ? RCANFD_GERFL_EEF0_7 : \
> +                                       (RCANFD_GERFL_EEF0 | RCANFD_GERFL_EEF1)) | \
> +                                       RCANFD_GERFL_MES | ((gpriv)->fdmode ? \
> +                                       RCANFD_GERFL_CMPOF : 0)))

The indentation is hard to read. Here you follow the 80
characters per line, but the indentation does not highlight
the logic of the different branches of the ternary operator.

Does something in this fashion make more sense?

+#define RCANFD_GERFL_ERR(gpriv, x)                                     \
+(                                                                      \
+        (x) & ((IS_V3U ? RCANFD_GERFL_EEF0_7 :                          \
+                         RCANFD_GERFL_EEF0 | RCANFD_GERFL_EEF1) |       \
+               RCANFD_GERFL_MES |                                       \
+               ((gpriv)->fdmode ? RCANFD_GERFL_CMPOF : 0))              \
+)

(I guess that my mailer will break all the tabulations, sorry for that).

>  /* AFL Rx rules registers */
>
>  /* RSCFDnCFDGAFLCFG0 / RSCFDnGAFLCFG0 */
> -#define RCANFD_GAFLCFG_SETRNC(n, x)    (((x) & 0xff) << (24 - n * 8))
> -#define RCANFD_GAFLCFG_GETRNC(n, x)    (((x) >> (24 - n * 8)) & 0xff)
> +#define RCANFD_GAFLCFG_SETRNC(n, x)    (((x) & (IS_V3U ? 0x1ff : 0xff)) << \
> +                                        ((IS_V3U ? 16 : 24) - (n) * (IS_V3U ? \
> +                                        16 : 8)))
> +#define RCANFD_GAFLCFG_GETRNC(n, x)    (((x) >> ((IS_V3U ? 16 : 24) - (n) * \
> +                                        (IS_V3U ? 16 : 8))) & \
> +                                        (IS_V3U ? 0x1ff : 0xff))

Similarly, here, you try to factorize all the code, but this is not
helping readability.

What do you think of doing this instead:

+/* RSCFDnCFDGAFLCFG0 / RSCFDnGAFLCFG0 */
+#define RCANFD_GAFLCFG_SETRNC(n, x)                                    \
+        (IS_V3U ?                                                       \
+                ((x) & 0x1ff) << (16 - n * 16) :                        \
+                ((x) & 0xff) << (24 - n * 8))

Same comments apply to other macros as well.

>  /* RSCFDnCFDGAFLECTR / RSCFDnGAFLECTR */
>  #define RCANFD_GAFLECTR_AFLDAE         BIT(8)
> -#define RCANFD_GAFLECTR_AFLPN(x)       ((x) & 0x1f)
> +#define RCANFD_GAFLECTR_AFLPN(x)       ((x) & (IS_V3U ? 0x7f : 0x1f))
>
>  /* RSCFDnCFDGAFLIDj / RSCFDnGAFLIDj */
>  #define RCANFD_GAFLID_GAFLLB           BIT(29)
> @@ -116,9 +124,12 @@ enum rcanfd_chip_id {
>  #define RCANFD_CFG_BRP(x)              (((x) & 0x3ff) << 0)
>
>  /* RSCFDnCFDCmNCFG - CAN FD only */
> -#define RCANFD_NCFG_NTSEG2(x)          (((x) & 0x1f) << 24)
> -#define RCANFD_NCFG_NTSEG1(x)          (((x) & 0x7f) << 16)
> -#define RCANFD_NCFG_NSJW(x)            (((x) & 0x1f) << 11)
> +#define RCANFD_NCFG_NTSEG2(x)          (((x) & (IS_V3U ? 0x7f : 0x1f)) << \
> +                                        (IS_V3U ? 25 : 24))
> +#define RCANFD_NCFG_NTSEG1(x)          (((x) & (IS_V3U ? 0xff : 0x7f)) << \
> +                                        (IS_V3U ? 17 : 16))
> +#define RCANFD_NCFG_NSJW(x)            (((x) & (IS_V3U ? 0x7f : 0x1f)) << \
> +                                        (IS_V3U ? 10 : 11))
>  #define RCANFD_NCFG_NBRP(x)            (((x) & 0x3ff) << 0)
>
>  /* RSCFDnCFDCmCTR / RSCFDnCmCTR */
> @@ -180,11 +191,15 @@ enum rcanfd_chip_id {
>
>  /* RSCFDnCFDCmDCFG */
>  #define RCANFD_DCFG_DSJW(x)            (((x) & 0x7) << 24)
> -#define RCANFD_DCFG_DTSEG2(x)          (((x) & 0x7) << 20)
> -#define RCANFD_DCFG_DTSEG1(x)          (((x) & 0xf) << 16)
> +#define RCANFD_DCFG_DTSEG2(x)          (((x) & (IS_V3U ? 0x0f : 0x7)) << \
> +                                        (IS_V3U ? 16 : 20))
> +#define RCANFD_DCFG_DTSEG1(x)          (((x) & (IS_V3U ? 0x1f : 0xf)) << \
> +                                        (IS_V3U ? 8 : 16))
>  #define RCANFD_DCFG_DBRP(x)            (((x) & 0xff) << 0)
>
>  /* RSCFDnCFDCmFDCFG */
> +#define RCANFD_FDCFG_CLOE              BIT(30)
> +#define RCANFD_FDCFG_FDOE              BIT(28)
>  #define RCANFD_FDCFG_TDCE              BIT(9)
>  #define RCANFD_FDCFG_TDCOC             BIT(8)
>  #define RCANFD_FDCFG_TDCO(x)           (((x) & 0x7f) >> 16)
> @@ -219,10 +234,10 @@ enum rcanfd_chip_id {
>  /* Common FIFO bits */
>
>  /* RSCFDnCFDCFCCk */
> -#define RCANFD_CFCC_CFTML(x)           (((x) & 0xf) << 20)
> -#define RCANFD_CFCC_CFM(x)             (((x) & 0x3) << 16)
> +#define RCANFD_CFCC_CFTML(x)           (((x) & 0xf) << (IS_V3U ? 16 : 20))
> +#define RCANFD_CFCC_CFM(x)             (((x) & 0x3) << (IS_V3U ? 8 : 16))
>  #define RCANFD_CFCC_CFIM               BIT(12)
> -#define RCANFD_CFCC_CFDC(x)            (((x) & 0x7) << 8)
> +#define RCANFD_CFCC_CFDC(x)            (((x) & 0x7) << (IS_V3U ? 21 : 8))
>  #define RCANFD_CFCC_CFPLS(x)           (((x) & 0x7) << 4)
>  #define RCANFD_CFCC_CFTXIE             BIT(2)
>  #define RCANFD_CFCC_CFE                        BIT(0)
> @@ -282,33 +297,32 @@ enum rcanfd_chip_id {
>  #define RCANFD_GTSC                    (0x0094)
>  /* RSCFDnCFDGAFLECTR / RSCFDnGAFLECTR */
>  #define RCANFD_GAFLECTR                        (0x0098)
> -/* RSCFDnCFDGAFLCFG0 / RSCFDnGAFLCFG0 */
> -#define RCANFD_GAFLCFG0                        (0x009c)
> -/* RSCFDnCFDGAFLCFG1 / RSCFDnGAFLCFG1 */
> -#define RCANFD_GAFLCFG1                        (0x00a0)
> +/* RSCFDnCFDGAFLCFG / RSCFDnGAFLCFG */
> +#define RCANFD_GAFLCFG(ch)             (0x009c + (0x04 * ((ch) / 2)))
>  /* RSCFDnCFDRMNB / RSCFDnRMNB */
>  #define RCANFD_RMNB                    (0x00a4)
>  /* RSCFDnCFDRMND / RSCFDnRMND */
>  #define RCANFD_RMND(y)                 (0x00a8 + (0x04 * (y)))
>
>  /* RSCFDnCFDRFCCx / RSCFDnRFCCx */
> -#define RCANFD_RFCC(x)                 (0x00b8 + (0x04 * (x)))
> +#define RCANFD_RFCC(x)                 ((IS_V3U ? 0x00c0 : 0x00b8) + \
> +                                        (0x04 * (x)))
>  /* RSCFDnCFDRFSTSx / RSCFDnRFSTSx */
> -#define RCANFD_RFSTS(x)                        (0x00d8 + (0x04 * (x)))
> +#define RCANFD_RFSTS(x)                        (RCANFD_RFCC(x) + 0x20)
>  /* RSCFDnCFDRFPCTRx / RSCFDnRFPCTRx */
> -#define RCANFD_RFPCTR(x)               (0x00f8 + (0x04 * (x)))
> +#define RCANFD_RFPCTR(x)               (RCANFD_RFCC(x) + 0x40)
>
>  /* Common FIFO Control registers */
>
>  /* RSCFDnCFDCFCCx / RSCFDnCFCCx */
> -#define RCANFD_CFCC(ch, idx)           (0x0118 + (0x0c * (ch)) + \
> -                                        (0x04 * (idx)))
> +#define RCANFD_CFCC(ch, idx)           ((IS_V3U ? 0x0120 : 0x0118) + \
> +                                        (0x0c * (ch)) + (0x04 * (idx)))
>  /* RSCFDnCFDCFSTSx / RSCFDnCFSTSx */
> -#define RCANFD_CFSTS(ch, idx)          (0x0178 + (0x0c * (ch)) + \
> -                                        (0x04 * (idx)))
> +#define RCANFD_CFSTS(ch, idx)          ((IS_V3U ? 0x01e0 : 0x0178) + \
> +                                        (0x0c * (ch)) + (0x04 * (idx)))
>  /* RSCFDnCFDCFPCTRx / RSCFDnCFPCTRx */
> -#define RCANFD_CFPCTR(ch, idx)         (0x01d8 + (0x0c * (ch)) + \
> -                                        (0x04 * (idx)))
> +#define RCANFD_CFPCTR(ch, idx)         ((IS_V3U ? 0x0240 : 0x01d8) + \
> +                                        (0x0c * (ch)) + (0x04 * (idx)))
>
>  /* RSCFDnCFDFESTS / RSCFDnFESTS */
>  #define RCANFD_FESTS                   (0x0238)
> @@ -415,6 +429,12 @@ enum rcanfd_chip_id {
>  /* RSCFDnRPGACCr */
>  #define RCANFD_C_RPGACC(r)             (0x1900 + (0x04 * (r)))
>
> +/* R-Car V3U Classical and CAN FD mode specific register map */
> +#define RCANFD_V3U_CFDCFG              (0x1314)
> +#define RCANFD_V3U_DCFG(m)             (0x1400 + (0x20 * (m)))
> +
> +#define RCANFD_V3U_GAFL_OFFSET         (0x1800)
> +
>  /* CAN FD mode specific register map */
>
>  /* RSCFDnCFDCmXXX -> RCANFD_F_XXX(m) */
> @@ -434,7 +454,7 @@ enum rcanfd_chip_id {
>  #define RCANFD_F_RMDF(q, b)            (0x200c + (0x04 * (b)) + (0x20 * (q)))
>
>  /* RSCFDnCFDRFXXx -> RCANFD_F_RFXX(x) */
> -#define RCANFD_F_RFOFFSET              (0x3000)
> +#define RCANFD_F_RFOFFSET              (IS_V3U ? 0x6000 : 0x3000)
>  #define RCANFD_F_RFID(x)               (RCANFD_F_RFOFFSET + (0x80 * (x)))
>  #define RCANFD_F_RFPTR(x)              (RCANFD_F_RFOFFSET + 0x04 + \
>                                          (0x80 * (x)))
> @@ -444,7 +464,7 @@ enum rcanfd_chip_id {
>                                          (0x80 * (x)) + (0x04 * (df)))
>
>  /* RSCFDnCFDCFXXk -> RCANFD_F_CFXX(ch, k) */
> -#define RCANFD_F_CFOFFSET              (0x3400)
> +#define RCANFD_F_CFOFFSET              (IS_V3U ? 0x6400 : 0x3400)
>  #define RCANFD_F_CFID(ch, idx)         (RCANFD_F_CFOFFSET + (0x180 * (ch)) + \
>                                          (0x80 * (idx)))
>  #define RCANFD_F_CFPTR(ch, idx)                (RCANFD_F_CFOFFSET + 0x04 + \
> @@ -470,7 +490,7 @@ enum rcanfd_chip_id {
>  #define RCANFD_FIFO_DEPTH              8       /* Tx FIFO depth */
>  #define RCANFD_NAPI_WEIGHT             8       /* Rx poll quota */
>
> -#define RCANFD_NUM_CHANNELS            2       /* Two channels max */
> +#define RCANFD_NUM_CHANNELS            8       /* Eight channels max */
>  #define RCANFD_CHANNELS_MASK           BIT((RCANFD_NUM_CHANNELS) - 1)
>
>  #define RCANFD_GAFL_PAGENUM(entry)     ((entry) / 16)
> @@ -522,6 +542,7 @@ struct rcar_canfd_global {
>         struct reset_control *rstc1;
>         struct reset_control *rstc2;
>         enum rcanfd_chip_id chip_id;
> +       u32 max_channels;
>  };
>
>  /* CAN FD mode nominal rate constants */
> @@ -629,6 +650,25 @@ static void rcar_canfd_tx_failure_cleanup(struct net_device *ndev)
>                 can_free_echo_skb(ndev, i, NULL);
>  }
>
> +static void rcar_canfd_set_mode(struct rcar_canfd_global *gpriv)
> +{
> +       if (gpriv->chip_id == RENESAS_R8A779A0) {
> +               if (gpriv->fdmode)
> +                       rcar_canfd_set_bit(gpriv->base, RCANFD_V3U_CFDCFG,
> +                                          RCANFD_FDCFG_FDOE);
> +               else
> +                       rcar_canfd_set_bit(gpriv->base, RCANFD_V3U_CFDCFG,
> +                                          RCANFD_FDCFG_CLOE);
> +       } else {
> +               if (gpriv->fdmode)
> +                       rcar_canfd_set_bit(gpriv->base, RCANFD_GRMCFG,
> +                                          RCANFD_GRMCFG_RCMC);
> +               else
> +                       rcar_canfd_clear_bit(gpriv->base, RCANFD_GRMCFG,
> +                                            RCANFD_GRMCFG_RCMC);
> +       }
> +}
> +
>  static int rcar_canfd_reset_controller(struct rcar_canfd_global *gpriv)
>  {
>         u32 sts, ch;
> @@ -661,15 +701,10 @@ static int rcar_canfd_reset_controller(struct rcar_canfd_global *gpriv)
>         rcar_canfd_write(gpriv->base, RCANFD_GERFL, 0x0);
>
>         /* Set the controller into appropriate mode */
> -       if (gpriv->fdmode)
> -               rcar_canfd_set_bit(gpriv->base, RCANFD_GRMCFG,
> -                                  RCANFD_GRMCFG_RCMC);
> -       else
> -               rcar_canfd_clear_bit(gpriv->base, RCANFD_GRMCFG,
> -                                    RCANFD_GRMCFG_RCMC);
> +       rcar_canfd_set_mode(gpriv);
>
>         /* Transition all Channels to reset mode */
> -       for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> +       for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) {
>                 rcar_canfd_clear_bit(gpriv->base,
>                                      RCANFD_CCTR(ch), RCANFD_CCTR_CSLPR);
>
> @@ -710,7 +745,7 @@ static void rcar_canfd_configure_controller(struct rcar_canfd_global *gpriv)
>         rcar_canfd_set_bit(gpriv->base, RCANFD_GCFG, cfg);
>
>         /* Channel configuration settings */
> -       for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> +       for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) {
>                 rcar_canfd_set_bit(gpriv->base, RCANFD_CCTR(ch),
>                                    RCANFD_CCTR_ERRD);
>                 rcar_canfd_update_bit(gpriv->base, RCANFD_CCTR(ch),
> @@ -730,7 +765,7 @@ static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
>                 start = 0; /* Channel 0 always starts from 0th rule */
>         } else {
>                 /* Get number of Channel 0 rules and adjust */
> -               cfg = rcar_canfd_read(gpriv->base, RCANFD_GAFLCFG0);
> +               cfg = rcar_canfd_read(gpriv->base, RCANFD_GAFLCFG(ch));
>                 start = RCANFD_GAFLCFG_GETRNC(0, cfg);
>         }
>
> @@ -741,12 +776,16 @@ static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
>                             RCANFD_GAFLECTR_AFLDAE));
>
>         /* Write number of rules for channel */
> -       rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG0,
> +       rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG(ch),
>                            RCANFD_GAFLCFG_SETRNC(ch, num_rules));
> -       if (gpriv->fdmode)
> -               offset = RCANFD_F_GAFL_OFFSET;
> -       else
> -               offset = RCANFD_C_GAFL_OFFSET;
> +       if (gpriv->chip_id == RENESAS_R8A779A0) {
> +               offset = RCANFD_V3U_GAFL_OFFSET;
> +       } else {
> +               if (gpriv->fdmode)
> +                       offset = RCANFD_F_GAFL_OFFSET;
> +               else
> +                       offset = RCANFD_C_GAFL_OFFSET;
> +       }
>
>         /* Accept all IDs */
>         rcar_canfd_write(gpriv->base, RCANFD_GAFLID(offset, start), 0);
> @@ -755,8 +794,8 @@ static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
>         /* Any data length accepted */
>         rcar_canfd_write(gpriv->base, RCANFD_GAFLP0(offset, start), 0);
>         /* Place the msg in corresponding Rx FIFO entry */
> -       rcar_canfd_write(gpriv->base, RCANFD_GAFLP1(offset, start),
> -                        RCANFD_GAFLP1_GAFLFDP(ridx));
> +       rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLP1(offset, start),
> +                          RCANFD_GAFLP1_GAFLFDP(ridx));
>
>         /* Disable write access to page */
>         rcar_canfd_clear_bit(gpriv->base,
> @@ -1041,6 +1080,7 @@ static void rcar_canfd_error(struct net_device *ndev, u32 cerfl,
>  static void rcar_canfd_tx_done(struct net_device *ndev)
>  {
>         struct rcar_canfd_channel *priv = netdev_priv(ndev);
> +       struct rcar_canfd_global *gpriv = priv->gpriv;
>         struct net_device_stats *stats = &ndev->stats;
>         u32 sts;
>         unsigned long flags;
> @@ -1096,7 +1136,7 @@ static irqreturn_t rcar_canfd_global_err_interrupt(int irq, void *dev_id)
>         struct rcar_canfd_global *gpriv = dev_id;
>         u32 ch;
>
> -       for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS)
> +       for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels)
>                 rcar_canfd_handle_global_err(gpriv, ch);
>
>         return IRQ_HANDLED;
> @@ -1126,7 +1166,7 @@ static irqreturn_t rcar_canfd_global_receive_fifo_interrupt(int irq, void *dev_i
>         struct rcar_canfd_global *gpriv = dev_id;
>         u32 ch;
>
> -       for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS)
> +       for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels)
>                 rcar_canfd_handle_global_receive(gpriv, ch);
>
>         return IRQ_HANDLED;
> @@ -1140,7 +1180,7 @@ static irqreturn_t rcar_canfd_global_interrupt(int irq, void *dev_id)
>         /* Global error interrupts still indicate a condition specific
>          * to a channel. RxFIFO interrupt is a global interrupt.
>          */
> -       for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> +       for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) {
>                 rcar_canfd_handle_global_err(gpriv, ch);
>                 rcar_canfd_handle_global_receive(gpriv, ch);
>         }
> @@ -1198,7 +1238,7 @@ static irqreturn_t rcar_canfd_channel_tx_interrupt(int irq, void *dev_id)
>         struct rcar_canfd_global *gpriv = dev_id;
>         u32 ch;
>
> -       for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS)
> +       for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels)
>                 rcar_canfd_handle_channel_tx(gpriv, ch);
>
>         return IRQ_HANDLED;
> @@ -1230,7 +1270,7 @@ static irqreturn_t rcar_canfd_channel_err_interrupt(int irq, void *dev_id)
>         struct rcar_canfd_global *gpriv = dev_id;
>         u32 ch;
>
> -       for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS)
> +       for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels)
>                 rcar_canfd_handle_channel_err(gpriv, ch);
>
>         return IRQ_HANDLED;
> @@ -1242,7 +1282,7 @@ static irqreturn_t rcar_canfd_channel_interrupt(int irq, void *dev_id)
>         u32 ch;
>
>         /* Common FIFO is a per channel resource */
> -       for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> +       for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) {
>                 rcar_canfd_handle_channel_err(gpriv, ch);
>                 rcar_canfd_handle_channel_tx(gpriv, ch);
>         }
> @@ -1253,6 +1293,7 @@ static irqreturn_t rcar_canfd_channel_interrupt(int irq, void *dev_id)
>  static void rcar_canfd_set_bittiming(struct net_device *dev)
>  {
>         struct rcar_canfd_channel *priv = netdev_priv(dev);
> +       struct rcar_canfd_global *gpriv = priv->gpriv;
>         const struct can_bittiming *bt = &priv->can.bittiming;
>         const struct can_bittiming *dbt = &priv->can.data_bittiming;
>         u16 brp, sjw, tseg1, tseg2;
> @@ -1288,8 +1329,17 @@ static void rcar_canfd_set_bittiming(struct net_device *dev)
>                            brp, sjw, tseg1, tseg2);
>         } else {
>                 /* Classical CAN only mode */
> -               cfg = (RCANFD_CFG_TSEG1(tseg1) | RCANFD_CFG_BRP(brp) |
> -                       RCANFD_CFG_SJW(sjw) | RCANFD_CFG_TSEG2(tseg2));
> +               if (gpriv->chip_id == RENESAS_R8A779A0) {
> +                       cfg = (RCANFD_NCFG_NTSEG1(tseg1) |
> +                              RCANFD_NCFG_NBRP(brp) |
> +                              RCANFD_NCFG_NSJW(sjw) |
> +                              RCANFD_NCFG_NTSEG2(tseg2));
> +               } else {
> +                       cfg = (RCANFD_CFG_TSEG1(tseg1) |
> +                              RCANFD_CFG_BRP(brp) |
> +                              RCANFD_CFG_SJW(sjw) |
> +                              RCANFD_CFG_TSEG2(tseg2));
> +               }
>
>                 rcar_canfd_write(priv->base, RCANFD_CCFG(ch), cfg);
>                 netdev_dbg(priv->ndev,
> @@ -1301,6 +1351,7 @@ static void rcar_canfd_set_bittiming(struct net_device *dev)
>  static int rcar_canfd_start(struct net_device *ndev)
>  {
>         struct rcar_canfd_channel *priv = netdev_priv(ndev);
> +       struct rcar_canfd_global *gpriv = priv->gpriv;
>         int err = -EOPNOTSUPP;
>         u32 sts, ch = priv->channel;
>         u32 ridx = ch + RCANFD_RFFIFO_IDX;
> @@ -1372,6 +1423,7 @@ static int rcar_canfd_open(struct net_device *ndev)
>  static void rcar_canfd_stop(struct net_device *ndev)
>  {
>         struct rcar_canfd_channel *priv = netdev_priv(ndev);
> +       struct rcar_canfd_global *gpriv = priv->gpriv;
>         int err;
>         u32 sts, ch = priv->channel;
>         u32 ridx = ch + RCANFD_RFFIFO_IDX;
> @@ -1415,6 +1467,7 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
>                                          struct net_device *ndev)
>  {
>         struct rcar_canfd_channel *priv = netdev_priv(ndev);
> +       struct rcar_canfd_global *gpriv = priv->gpriv;
>         struct canfd_frame *cf = (struct canfd_frame *)skb->data;
>         u32 sts = 0, id, dlc;
>         unsigned long flags;
> @@ -1435,13 +1488,15 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
>
>         dlc = RCANFD_CFPTR_CFDLC(can_fd_len2dlc(cf->len));
>
> -       if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> +       if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
> +           gpriv->chip_id == RENESAS_R8A779A0) {
>                 rcar_canfd_write(priv->base,
>                                  RCANFD_F_CFID(ch, RCANFD_CFFIFO_IDX), id);
>                 rcar_canfd_write(priv->base,
>                                  RCANFD_F_CFPTR(ch, RCANFD_CFFIFO_IDX), dlc);
>
> -               if (can_is_canfd_skb(skb)) {
> +               if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
> +                   can_is_canfd_skb(skb)) {

Could you explain why this additional check is needed?
My understanding is that can_is_canfd_skb(skb) being true implies
that the CAN_CTRLMODE_FD flag is set.

>                         /* CAN FD frame format */
>                         sts |= RCANFD_CFFDCSTS_CFFDF;
>                         if (cf->flags & CANFD_BRS)
> @@ -1488,22 +1543,29 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
>  static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
>  {
>         struct net_device_stats *stats = &priv->ndev->stats;
> +       struct rcar_canfd_global *gpriv = priv->gpriv;
>         struct canfd_frame *cf;
>         struct sk_buff *skb;
>         u32 sts = 0, id, dlc;
>         u32 ch = priv->channel;
>         u32 ridx = ch + RCANFD_RFFIFO_IDX;
>
> -       if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> +       if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
> +           gpriv->chip_id == RENESAS_R8A779A0) {

I guess that this is linked to the above comment. Does the
R8A779A0 chip support CAN-FD? If yes, why not simply use the
CAN_CTRLMODE_FD instead of adding this additional check?

>                 id = rcar_canfd_read(priv->base, RCANFD_F_RFID(ridx));
>                 dlc = rcar_canfd_read(priv->base, RCANFD_F_RFPTR(ridx));
>
>                 sts = rcar_canfd_read(priv->base, RCANFD_F_RFFDSTS(ridx));
> -               if (sts & RCANFD_RFFDSTS_RFFDF)
> -                       skb = alloc_canfd_skb(priv->ndev, &cf);
> -               else
> +               if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> +                       if (sts & RCANFD_RFFDSTS_RFFDF)
> +                               skb = alloc_canfd_skb(priv->ndev, &cf);
> +                       else
> +                               skb = alloc_can_skb(priv->ndev,
> +                                                   (struct can_frame **)&cf);
> +               } else {
>                         skb = alloc_can_skb(priv->ndev,
>                                             (struct can_frame **)&cf);
> +               }
>         } else {
>                 id = rcar_canfd_read(priv->base, RCANFD_C_RFID(ridx));
>                 dlc = rcar_canfd_read(priv->base, RCANFD_C_RFPTR(ridx));
> @@ -1541,10 +1603,16 @@ static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
>                 }
>         } else {
>                 cf->len = can_cc_dlc2len(RCANFD_RFPTR_RFDLC(dlc));
> -               if (id & RCANFD_RFID_RFRTR)
> +               if (id & RCANFD_RFID_RFRTR) {
>                         cf->can_id |= CAN_RTR_FLAG;
> -               else
> -                       rcar_canfd_get_data(priv, cf, RCANFD_C_RFDF(ridx, 0));
> +               } else {
> +                       if (gpriv->chip_id == RENESAS_R8A779A0)
> +                               rcar_canfd_get_data(priv, cf,
> +                                                   RCANFD_F_RFDF(ridx, 0));
> +                       else
> +                               rcar_canfd_get_data(priv, cf,
> +                                                   RCANFD_C_RFDF(ridx, 0));
> +               }
>         }
>
>         /* Write 0xff to RFPC to increment the CPU-side
> @@ -1563,6 +1631,7 @@ static int rcar_canfd_rx_poll(struct napi_struct *napi, int quota)
>  {
>         struct rcar_canfd_channel *priv =
>                 container_of(napi, struct rcar_canfd_channel, napi);
> +       struct rcar_canfd_global *gpriv = priv->gpriv;
>         int num_pkts;
>         u32 sts;
>         u32 ch = priv->channel;
> @@ -1762,21 +1831,25 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>         int g_err_irq, g_recc_irq;
>         bool fdmode = true;                     /* CAN FD only mode - default */
>         enum rcanfd_chip_id chip_id;
> +       int max_channels;
> +       char name[9];
> +       int i;
>
>         chip_id = (uintptr_t)of_device_get_match_data(&pdev->dev);
> +       max_channels = chip_id == RENESAS_R8A779A0 ? 8 : 2;
>
>         if (of_property_read_bool(pdev->dev.of_node, "renesas,no-can-fd"))
>                 fdmode = false;                 /* Classical CAN only mode */
>
> -       of_child = of_get_child_by_name(pdev->dev.of_node, "channel0");
> -       if (of_child && of_device_is_available(of_child))
> -               channels_mask |= BIT(0);        /* Channel 0 */
> -
> -       of_child = of_get_child_by_name(pdev->dev.of_node, "channel1");
> -       if (of_child && of_device_is_available(of_child))
> -               channels_mask |= BIT(1);        /* Channel 1 */
> +       strcpy(name, "channelX");
> +       for (i = 0; i < max_channels; ++i) {
> +               name[7] = '0' + i;
> +               of_child = of_get_child_by_name(pdev->dev.of_node, name);
> +               if (of_child && of_device_is_available(of_child))
> +                       channels_mask |= BIT(i);
> +       }
>
> -       if (chip_id == RENESAS_RCAR_GEN3) {
> +       if (chip_id != RENESAS_RZG2L) {
>                 ch_irq = platform_get_irq_byname_optional(pdev, "ch_int");
>                 if (ch_irq < 0) {
>                         /* For backward compatibility get irq by index */
> @@ -1812,6 +1885,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>         gpriv->channels_mask = channels_mask;
>         gpriv->fdmode = fdmode;
>         gpriv->chip_id = chip_id;
> +       gpriv->max_channels = max_channels;
>
>         if (gpriv->chip_id == RENESAS_RZG2L) {
>                 gpriv->rstc1 = devm_reset_control_get_exclusive(&pdev->dev, "rstp_n");
> @@ -1853,7 +1927,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>         }
>         fcan_freq = clk_get_rate(gpriv->can_clk);
>
> -       if (gpriv->fcan == RCANFD_CANFDCLK && gpriv->chip_id == RENESAS_RCAR_GEN3)
> +       if (gpriv->fcan == RCANFD_CANFDCLK && gpriv->chip_id != RENESAS_RZG2L)
>                 /* CANFD clock is further divided by (1/2) within the IP */
>                 fcan_freq /= 2;
>
> @@ -1865,7 +1939,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>         gpriv->base = addr;
>
>         /* Request IRQ that's common for both channels */
> -       if (gpriv->chip_id == RENESAS_RCAR_GEN3) {
> +       if (gpriv->chip_id != RENESAS_RZG2L) {
>                 err = devm_request_irq(&pdev->dev, ch_irq,
>                                        rcar_canfd_channel_interrupt, 0,
>                                        "canfd.ch_int", gpriv);
> @@ -1931,7 +2005,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>         rcar_canfd_configure_controller(gpriv);
>
>         /* Configure per channel attributes */
> -       for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> +       for_each_set_bit(ch, &gpriv->channels_mask, max_channels) {
>                 /* Configure Channel's Rx fifo */
>                 rcar_canfd_configure_rx(gpriv, ch);
>
> @@ -1957,7 +2031,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>                 goto fail_mode;
>         }
>
> -       for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> +       for_each_set_bit(ch, &gpriv->channels_mask, max_channels) {
>                 err = rcar_canfd_channel_probe(gpriv, ch, fcan_freq);
>                 if (err)
>                         goto fail_channel;
> @@ -1969,7 +2043,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>         return 0;
>
>  fail_channel:
> -       for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS)
> +       for_each_set_bit(ch, &gpriv->channels_mask, max_channels)
>                 rcar_canfd_channel_remove(gpriv, ch);
>  fail_mode:
>         rcar_canfd_disable_global_interrupts(gpriv);
> @@ -1990,7 +2064,7 @@ static int rcar_canfd_remove(struct platform_device *pdev)
>         rcar_canfd_reset_controller(gpriv);
>         rcar_canfd_disable_global_interrupts(gpriv);
>
> -       for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> +       for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) {
>                 rcar_canfd_disable_channel_interrupts(gpriv->ch[ch]);
>                 rcar_canfd_channel_remove(gpriv, ch);
>         }
> @@ -2020,6 +2094,7 @@ static SIMPLE_DEV_PM_OPS(rcar_canfd_pm_ops, rcar_canfd_suspend,
>  static const __maybe_unused struct of_device_id rcar_canfd_of_table[] = {
>         { .compatible = "renesas,rcar-gen3-canfd", .data = (void *)RENESAS_RCAR_GEN3 },
>         { .compatible = "renesas,rzg2l-canfd", .data = (void *)RENESAS_RZG2L },
> +       { .compatible = "renesas,r8a779a0-canfd", .data = (void *)RENESAS_R8A779A0 },
>         { }
>  };


Yours sincerely,
Vincent Mailhol

Re: [PATCH v2 2/5] can: rcar_canfd: Add support for r8a779a0 SoC

[Ulrich Hecht]

Thanks for your review.

> On 01/31/2022 3:08 AM Vincent MAILHOL <mailhol.vincent@wanadoo.fr> wrote:
> > @@ -1435,13 +1488,15 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
> >
> >         dlc = RCANFD_CFPTR_CFDLC(can_fd_len2dlc(cf->len));
> >
> > -       if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> > +       if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
> > +           gpriv->chip_id == RENESAS_R8A779A0) {
> >                 rcar_canfd_write(priv->base,
> >                                  RCANFD_F_CFID(ch, RCANFD_CFFIFO_IDX), id);
> >                 rcar_canfd_write(priv->base,
> >                                  RCANFD_F_CFPTR(ch, RCANFD_CFFIFO_IDX), dlc);
> >
> > -               if (can_is_canfd_skb(skb)) {
> > +               if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
> > +                   can_is_canfd_skb(skb)) {
> 
> Could you explain why this additional check is needed?
> My understanding is that can_is_canfd_skb(skb) being true implies
> that the CAN_CTRLMODE_FD flag is set.

That might indeed be redundant.

> 
> >                         /* CAN FD frame format */
> >                         sts |= RCANFD_CFFDCSTS_CFFDF;
> >                         if (cf->flags & CANFD_BRS)
> > @@ -1488,22 +1543,29 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
> >  static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
> >  {
> >         struct net_device_stats *stats = &priv->ndev->stats;
> > +       struct rcar_canfd_global *gpriv = priv->gpriv;
> >         struct canfd_frame *cf;
> >         struct sk_buff *skb;
> >         u32 sts = 0, id, dlc;
> >         u32 ch = priv->channel;
> >         u32 ridx = ch + RCANFD_RFFIFO_IDX;
> >
> > -       if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> > +       if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
> > +           gpriv->chip_id == RENESAS_R8A779A0) {
> 
> I guess that this is linked to the above comment. Does the
> R8A779A0 chip support CAN-FD? If yes, why not simply use the
> CAN_CTRLMODE_FD instead of adding this additional check?

The non-V3U Gen3 CAN controllers have two different ways to be driven, depending on whether they are in classic or CAN-FD mode. The V3U controller is driven the CAN-FD way in both modes and thus needs to have this branch taken no matter what mode it is in.

CU
Uli

Re: [PATCH v2 2/5] can: rcar_canfd: Add support for r8a779a0 SoC

[Vincent MAILHOL]

On Mon. 31 Jan 2022 at 18:24, Ulrich Hecht <uli@fpond.eu> wrote:
>
> Thanks for your review.
>
> > On 01/31/2022 3:08 AM Vincent MAILHOL <mailhol.vincent@wanadoo.fr> wrote:
> > > @@ -1435,13 +1488,15 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
> > >
> > >         dlc = RCANFD_CFPTR_CFDLC(can_fd_len2dlc(cf->len));
> > >
> > > -       if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> > > +       if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
> > > +           gpriv->chip_id == RENESAS_R8A779A0) {
> > >                 rcar_canfd_write(priv->base,
> > >                                  RCANFD_F_CFID(ch, RCANFD_CFFIFO_IDX), id);
> > >                 rcar_canfd_write(priv->base,
> > >                                  RCANFD_F_CFPTR(ch, RCANFD_CFFIFO_IDX), dlc);
> > >
> > > -               if (can_is_canfd_skb(skb)) {
> > > +               if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
> > > +                   can_is_canfd_skb(skb)) {
> >
> > Could you explain why this additional check is needed?
> > My understanding is that can_is_canfd_skb(skb) being true implies
> > that the CAN_CTRLMODE_FD flag is set.
>
> That might indeed be redundant.
>
> >
> > >                         /* CAN FD frame format */
> > >                         sts |= RCANFD_CFFDCSTS_CFFDF;
> > >                         if (cf->flags & CANFD_BRS)
> > > @@ -1488,22 +1543,29 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
> > >  static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
> > >  {
> > >         struct net_device_stats *stats = &priv->ndev->stats;
> > > +       struct rcar_canfd_global *gpriv = priv->gpriv;
> > >         struct canfd_frame *cf;
> > >         struct sk_buff *skb;
> > >         u32 sts = 0, id, dlc;
> > >         u32 ch = priv->channel;
> > >         u32 ridx = ch + RCANFD_RFFIFO_IDX;
> > >
> > > -       if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> > > +       if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
> > > +           gpriv->chip_id == RENESAS_R8A779A0) {
> >
> > I guess that this is linked to the above comment. Does the
> > R8A779A0 chip support CAN-FD? If yes, why not simply use the
> > CAN_CTRLMODE_FD instead of adding this additional check?
>
> The non-V3U Gen3 CAN controllers have two different ways to be driven, depending on whether they are in classic or CAN-FD mode. The V3U controller is driven the CAN-FD way in both modes and thus needs to have this branch taken no matter what mode it is in.

Ack.
Makes sense. So actually, this isn’t related to the previous comments :)

In my previous message, I added two comments toward the macro. I
just want to double check that you have seen these because they
are missing from your reply.

Re: [PATCH v2 2/5] can: rcar_canfd: Add support for r8a779a0 SoC

[Vincent MAILHOL]

 Two more comments!

On Mon. 12 Jan 2022 at 01:22, Ulrich Hecht <uli+renesas@fpond.eu> wrote:
> Adds support for the CANFD IP variant in the V3U SoC.
>
> Differences to controllers in other SoCs are limited to an increase in
> the number of channels from two to eight, an absence of dedicated
> registers for "classic" CAN mode, and a number of differences in magic
> numbers (register offsets and layouts).
>
> Inspired by BSP patch by Kazuya Mizuguchi.
>
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> ---
>  drivers/net/can/rcar/rcar_canfd.c | 231 ++++++++++++++++++++----------
>  1 file changed, 153 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index ff9d0f5ae0dd..b1c9870d2a82 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c

...

> @@ -1488,22 +1543,29 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
>  static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
>  {
>         struct net_device_stats *stats = &priv->ndev->stats;
> +       struct rcar_canfd_global *gpriv = priv->gpriv;
>         struct canfd_frame *cf;
>         struct sk_buff *skb;
>         u32 sts = 0, id, dlc;
>         u32 ch = priv->channel;
>         u32 ridx = ch + RCANFD_RFFIFO_IDX;
>
> -       if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> +       if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
> +           gpriv->chip_id == RENESAS_R8A779A0) {
>                 id = rcar_canfd_read(priv->base, RCANFD_F_RFID(ridx));
>                 dlc = rcar_canfd_read(priv->base, RCANFD_F_RFPTR(ridx));
>
>                 sts = rcar_canfd_read(priv->base, RCANFD_F_RFFDSTS(ridx));
> -               if (sts & RCANFD_RFFDSTS_RFFDF)
> -                       skb = alloc_canfd_skb(priv->ndev, &cf);
> -               else
> +               if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> +                       if (sts & RCANFD_RFFDSTS_RFFDF)
> +                               skb = alloc_canfd_skb(priv->ndev, &cf);
> +                       else
> +                               skb = alloc_can_skb(priv->ndev,
> +                                                   (struct can_frame **)&cf);
> +               } else {
>                         skb = alloc_can_skb(priv->ndev,
>                                             (struct can_frame **)&cf);

It seems to me that we can factorize the two alloc_can_skb() calls:

+               if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
+                   sts & RCANFD_RFFDSTS_RFFDF)
+                       skb = alloc_canfd_skb(priv->ndev, &cf);
+               else
+                       skb = alloc_can_skb(priv->ndev, (struct
can_frame **)&cf);

> +               }
>         } else {
>                 id = rcar_canfd_read(priv->base, RCANFD_C_RFID(ridx));
>                 dlc = rcar_canfd_read(priv->base, RCANFD_C_RFPTR(ridx));
> @@ -1541,10 +1603,16 @@ static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
>                 }
>         } else {
>                 cf->len = can_cc_dlc2len(RCANFD_RFPTR_RFDLC(dlc));
> -               if (id & RCANFD_RFID_RFRTR)
> +               if (id & RCANFD_RFID_RFRTR) {
>                         cf->can_id |= CAN_RTR_FLAG;
> -               else
> -                       rcar_canfd_get_data(priv, cf, RCANFD_C_RFDF(ridx, 0));
> +               } else {
> +                       if (gpriv->chip_id == RENESAS_R8A779A0)
> +                               rcar_canfd_get_data(priv, cf,
> +                                                   RCANFD_F_RFDF(ridx, 0));
> +                       else
> +                               rcar_canfd_get_data(priv, cf,
> +                                                   RCANFD_C_RFDF(ridx, 0));
> +               }

Put the else if on a single line and remove one level of indentation:

+               else if (gpriv->chip_id == RENESAS_R8A779A0)
+                       rcar_canfd_get_data(priv, cf, RCANFD_F_RFDF(ridx, 0));
+               else
+                       rcar_canfd_get_data(priv, cf, RCANFD_C_RFDF(ridx, 0));

Also, a global comment, once you turn IS_V3U to an inline
function, you can use it in place of the many
"gpriv->chip_id == RENESAS_R8A779A0" checks.


Yours sincerely,
Vincent Mailhol