[PATCH v2 0/5] Renesas RZ/G2L CANFD support

[PATCH v2 0/5] Renesas RZ/G2L CANFD support

[Lad Prabhakar]

Hi All,

This patch series adds CANFD support to Renesas RZ/G2L family.

CANFD block on RZ/G2L SoC is almost identical to one found on
R-Car Gen3 SoC's. On RZ/G2L SoC interrupt sources for each channel
are split into individual sources.

Cheers,
Prabhakar

Changes for v2:
* Added interrupt-names property and marked it as required for 
  RZ/G2L family
* Added descriptions for reset property
* Re-used irq handlers on RZ/G2L SoC
* Added new enum for chip_id
* Dropped R9A07G044_LAST_CORE_CLK
* Dropped patch (clk: renesas: r9a07g044-cpg: Add clock and reset
  entries for CANFD) as its been merged into renesas tree

Lad Prabhakar (5):
  dt-bindings: net: can: renesas,rcar-canfd: Document RZ/G2L SoC
  can: rcar_canfd: Add support for RZ/G2L family
  dt-bindings: clk: r9a07g044-cpg: Add entry for P0_DIV2 core clock
  clk: renesas: r9a07g044-cpg: Add entry for fixed clock P0_DIV2
  arm64: dts: renesas: r9a07g044: Add CANFD node

 .../bindings/net/can/renesas,rcar-canfd.yaml  |  66 ++++++-
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi    |  42 +++++
 drivers/clk/renesas/r9a07g044-cpg.c           |   3 +-
 drivers/net/can/rcar/rcar_canfd.c             | 178 +++++++++++++++---
 include/dt-bindings/clock/r9a07g044-cpg.h     |   1 +
 5 files changed, 252 insertions(+), 38 deletions(-)


base-commit: 2734d6c1b1a089fb593ef6a23d4b70903526fe0c
-- 
2.17.1

[PATCH v2 1/5] dt-bindings: net: can: renesas,rcar-canfd: Document RZ/G2L SoC

[Lad Prabhakar]

Add CANFD binding documentation for Renesas RZ/G2L SoC.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 .../bindings/net/can/renesas,rcar-canfd.yaml  | 66 +++++++++++++++++--
 1 file changed, 60 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
index 0b33ba9ccb47..4fb6dd370904 100644
--- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
+++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
@@ -30,13 +30,15 @@ properties:
               - renesas,r8a77995-canfd     # R-Car D3
           - const: renesas,rcar-gen3-canfd # R-Car Gen3 and RZ/G2
 
+      - items:
+          - enum:
+              - renesas,r9a07g044-canfd    # RZ/G2{L,LC}
+          - const: renesas,rzg2l-canfd     # RZ/G2L family
+
   reg:
     maxItems: 1
 
-  interrupts:
-    items:
-      - description: Channel interrupt
-      - description: Global interrupt
+  interrupts: true
 
   clocks:
     maxItems: 3
@@ -50,8 +52,7 @@ properties:
   power-domains:
     maxItems: 1
 
-  resets:
-    maxItems: 1
+  resets: true
 
   renesas,no-can-fd:
     $ref: /schemas/types.yaml#/definitions/flag
@@ -91,6 +92,59 @@ required:
   - channel0
   - channel1
 
+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - renesas,rzg2l-canfd
+then:
+  properties:
+    interrupts:
+      items:
+        - description: CAN global error interrupt
+        - description: CAN receive FIFO interrupt
+        - description: CAN0 error interrupt
+        - description: CAN0 transmit interrupt
+        - description: CAN0 transmit/receive FIFO receive completion interrupt
+        - description: CAN1 error interrupt
+        - description: CAN1 transmit interrupt
+        - description: CAN1 transmit/receive FIFO receive completion interrupt
+
+    interrupt-names:
+      items:
+        - const: g_error
+        - const: g_rx_fifo
+        - const: can0_error
+        - const: can0_tx
+        - const: can0_tx_rx_fifo_receive_completion
+        - const: can1_error
+        - const: can1_tx
+        - const: can1_tx_rx_fifo_receive_completion
+
+    resets:
+      items:
+        - description: CANFD_RSTP_N
+        - description: CANFD_RSTC_N
+
+  required:
+    - interrupt-names
+else:
+  properties:
+    interrupts:
+      items:
+        - description: Channel interrupt
+        - description: Global interrupt
+
+    interrupt-names:
+      items:
+        - const: ch_int
+        - const: g_int
+
+    resets:
+      items:
+        - description: CANFD reset
+
 unevaluatedProperties: false
 
 examples:
-- 
2.17.1

[PATCH v2 2/5] can: rcar_canfd: Add support for RZ/G2L family

[Lad Prabhakar]

CANFD block on RZ/G2L SoC is almost identical to one found on
R-Car Gen3 SoC's. On RZ/G2L SoC interrupt sources for each channel
are split into different sources and the IP doesn't divide (1/2)
CANFD clock within the IP.

This patch adds compatible string for RZ/G2L family and registers
the irq handlers required for CANFD operation. IRQ numbers are now
fetched based on names instead of indices. For backward compatibility
on non RZ/G2L SoC's we fallback reading based on indices.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/can/rcar/rcar_canfd.c | 178 ++++++++++++++++++++++++------
 1 file changed, 147 insertions(+), 31 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 311e6ca3bdc4..d4affc002fb3 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -37,9 +37,15 @@
 #include <linux/bitmap.h>
 #include <linux/bitops.h>
 #include <linux/iopoll.h>
+#include <linux/reset.h>
 
 #define RCANFD_DRV_NAME			"rcar_canfd"
 
+enum rcanfd_chip_id {
+	RENESAS_RCAR_GEN3 = 0,
+	RENESAS_RZG2L,
+};
+
 /* Global register bits */
 
 /* RSCFDnCFDGRMCFG */
@@ -513,6 +519,9 @@ struct rcar_canfd_global {
 	enum rcar_canfd_fcanclk fcan;	/* CANFD or Ext clock */
 	unsigned long channels_mask;	/* Enabled channels mask */
 	bool fdmode;			/* CAN FD or Classical CAN only mode */
+	struct reset_control *rstc1;     /* Pointer to reset source1 */
+	struct reset_control *rstc2;     /* Pointer to reset source2 */
+	enum rcanfd_chip_id chip_id;
 };
 
 /* CAN FD mode nominal rate constants */
@@ -1577,6 +1586,45 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
 	priv->can.clock.freq = fcan_freq;
 	dev_info(&pdev->dev, "can_clk rate is %u\n", priv->can.clock.freq);
 
+	if (gpriv->chip_id == RENESAS_RZG2L) {
+		char *irq_name;
+		int err_irq;
+		int tx_irq;
+
+		err_irq = platform_get_irq_byname(pdev, ch == 0 ? "can0_error" : "can1_error");
+		if (err_irq < 0) {
+			err = err_irq;
+			goto fail;
+		}
+
+		tx_irq = platform_get_irq_byname(pdev, ch == 0 ? "can0_tx" : "can1_tx");
+		if (tx_irq < 0) {
+			err = tx_irq;
+			goto fail;
+		}
+
+		irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+					  "canfd.chnerr%d", ch);
+		err = devm_request_irq(&pdev->dev, err_irq,
+				       rcar_canfd_channel_interrupt, 0,
+				       irq_name, gpriv);
+		if (err) {
+			dev_err(&pdev->dev, "devm_request_irq CH Err(%d) failed, error %d\n",
+				err_irq, err);
+			goto fail;
+		}
+		irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+					  "canfd.chntx%d", ch);
+		err = devm_request_irq(&pdev->dev, tx_irq,
+				       rcar_canfd_channel_interrupt, 0,
+				       irq_name, gpriv);
+		if (err) {
+			dev_err(&pdev->dev, "devm_request_irq Tx (%d) failed, error %d\n",
+				tx_irq, err);
+			goto fail;
+		}
+	}
+
 	if (gpriv->fdmode) {
 		priv->can.bittiming_const = &rcar_canfd_nom_bittiming_const;
 		priv->can.data_bittiming_const =
@@ -1635,8 +1683,11 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 	struct rcar_canfd_global *gpriv;
 	struct device_node *of_child;
 	unsigned long channels_mask = 0;
-	int err, ch_irq, g_irq;
+	int err, ch_irq, g_irq, g_rx_irq;
 	bool fdmode = true;			/* CAN FD only mode - default */
+	enum rcanfd_chip_id chip_id;
+
+	chip_id = (enum rcanfd_chip_id)of_device_get_match_data(&pdev->dev);
 
 	if (of_property_read_bool(pdev->dev.of_node, "renesas,no-can-fd"))
 		fdmode = false;			/* Classical CAN only mode */
@@ -1649,27 +1700,64 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 	if (of_child && of_device_is_available(of_child))
 		channels_mask |= BIT(1);	/* Channel 1 */
 
-	ch_irq = platform_get_irq(pdev, 0);
-	if (ch_irq < 0) {
-		err = ch_irq;
-		goto fail_dev;
-	}
+	if (chip_id == RENESAS_RCAR_GEN3) {
+		ch_irq = platform_get_irq_byname(pdev, "ch_int");
+		if (ch_irq < 0) {
+			/* For backward compatibility get irq by index */
+			ch_irq = platform_get_irq(pdev, 0);
+			if (ch_irq < 0)
+				return ch_irq;
+		}
 
-	g_irq = platform_get_irq(pdev, 1);
-	if (g_irq < 0) {
-		err = g_irq;
-		goto fail_dev;
+		g_irq = platform_get_irq_byname(pdev, "g_int");
+		if (g_irq < 0) {
+			/* For backward compatibility get irq by index */
+			g_irq = platform_get_irq(pdev, 1);
+			if (g_irq < 0)
+				return g_irq;
+		}
+	} else {
+		g_irq = platform_get_irq_byname(pdev, "g_error");
+		if (g_irq < 0)
+			return g_irq;
+
+		g_rx_irq = platform_get_irq_byname(pdev, "g_rx_fifo");
+		if (g_rx_irq < 0)
+			return g_rx_irq;
 	}
 
 	/* Global controller context */
 	gpriv = devm_kzalloc(&pdev->dev, sizeof(*gpriv), GFP_KERNEL);
-	if (!gpriv) {
-		err = -ENOMEM;
-		goto fail_dev;
-	}
+	if (!gpriv)
+		return -ENOMEM;
+
 	gpriv->pdev = pdev;
 	gpriv->channels_mask = channels_mask;
 	gpriv->fdmode = fdmode;
+	gpriv->chip_id = chip_id;
+
+	if (gpriv->chip_id == RENESAS_RZG2L) {
+		gpriv->rstc1 = devm_reset_control_get_exclusive_by_index(&pdev->dev, 0);
+		if (IS_ERR(gpriv->rstc1)) {
+			dev_err(&pdev->dev, "failed to get reset index 0\n");
+			return PTR_ERR(gpriv->rstc1);
+		}
+
+		err = reset_control_reset(gpriv->rstc1);
+		if (err)
+			return err;
+
+		gpriv->rstc2 = devm_reset_control_get_exclusive_by_index(&pdev->dev, 1);
+		if (IS_ERR(gpriv->rstc2)) {
+			dev_err(&pdev->dev, "failed to get reset index 1\n");
+			return PTR_ERR(gpriv->rstc2);
+		}
+		err = reset_control_reset(gpriv->rstc2);
+		if (err) {
+			reset_control_assert(gpriv->rstc1);
+			return err;
+		}
+	}
 
 	/* Peripheral clock */
 	gpriv->clkp = devm_clk_get(&pdev->dev, "fck");
@@ -1699,7 +1787,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 	}
 	fcan_freq = clk_get_rate(gpriv->can_clk);
 
-	if (gpriv->fcan == RCANFD_CANFDCLK)
+	if (gpriv->fcan == RCANFD_CANFDCLK && gpriv->chip_id == RENESAS_RCAR_GEN3)
 		/* CANFD clock is further divided by (1/2) within the IP */
 		fcan_freq /= 2;
 
@@ -1711,21 +1799,43 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 	gpriv->base = addr;
 
 	/* Request IRQ that's common for both channels */
-	err = devm_request_irq(&pdev->dev, ch_irq,
-			       rcar_canfd_channel_interrupt, 0,
-			       "canfd.chn", gpriv);
-	if (err) {
-		dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
-			ch_irq, err);
-		goto fail_dev;
-	}
-	err = devm_request_irq(&pdev->dev, g_irq,
-			       rcar_canfd_global_interrupt, 0,
-			       "canfd.gbl", gpriv);
-	if (err) {
-		dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
-			g_irq, err);
-		goto fail_dev;
+	if (gpriv->chip_id == RENESAS_RCAR_GEN3) {
+		err = devm_request_irq(&pdev->dev, ch_irq,
+				       rcar_canfd_channel_interrupt, 0,
+				       "canfd.chn", gpriv);
+		if (err) {
+			dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
+				ch_irq, err);
+			goto fail_dev;
+		}
+
+		err = devm_request_irq(&pdev->dev, g_irq,
+				       rcar_canfd_global_interrupt, 0,
+				       "canfd.gbl", gpriv);
+		if (err) {
+			dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
+				g_irq, err);
+			goto fail_dev;
+		}
+	} else {
+		err = devm_request_irq(&pdev->dev, g_rx_irq,
+				       rcar_canfd_global_interrupt, 0,
+				       "canfd.gblrx", gpriv);
+
+		if (err) {
+			dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
+				g_rx_irq, err);
+			goto fail_dev;
+		}
+
+		err = devm_request_irq(&pdev->dev, g_irq,
+				       rcar_canfd_global_interrupt, 0,
+				       "canfd.gblerr", gpriv);
+		if (err) {
+			dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
+				g_irq, err);
+			goto fail_dev;
+		}
 	}
 
 	/* Enable peripheral clock for register access */
@@ -1791,6 +1901,8 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 fail_clk:
 	clk_disable_unprepare(gpriv->clkp);
 fail_dev:
+	reset_control_assert(gpriv->rstc1);
+	reset_control_assert(gpriv->rstc2);
 	return err;
 }
 
@@ -1810,6 +1922,9 @@ static int rcar_canfd_remove(struct platform_device *pdev)
 	/* Enter global sleep mode */
 	rcar_canfd_set_bit(gpriv->base, RCANFD_GCTR, RCANFD_GCTR_GSLPR);
 	clk_disable_unprepare(gpriv->clkp);
+	reset_control_assert(gpriv->rstc1);
+	reset_control_assert(gpriv->rstc2);
+
 	return 0;
 }
 
@@ -1827,7 +1942,8 @@ static SIMPLE_DEV_PM_OPS(rcar_canfd_pm_ops, rcar_canfd_suspend,
 			 rcar_canfd_resume);
 
 static const struct of_device_id rcar_canfd_of_table[] = {
-	{ .compatible = "renesas,rcar-gen3-canfd" },
+	{ .compatible = "renesas,rcar-gen3-canfd", .data = (void *)RENESAS_RCAR_GEN3 },
+	{ .compatible = "renesas,rzg2l-canfd", .data = (void *)RENESAS_RZG2L },
 	{ }
 };
 
-- 
2.17.1

[PATCH v2 3/5] dt-bindings: clk: r9a07g044-cpg: Add entry for P0_DIV2 core clock

[Lad Prabhakar]

Add P0_DIV2 core clock required for CANFD module. CANFD core clock is
sourced from P0_DIV2 referenced from HW manual Rev.0.50.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 include/dt-bindings/clock/r9a07g044-cpg.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/dt-bindings/clock/r9a07g044-cpg.h b/include/dt-bindings/clock/r9a07g044-cpg.h
index 0728ad07ff7a..0bb17ff1a01a 100644
--- a/include/dt-bindings/clock/r9a07g044-cpg.h
+++ b/include/dt-bindings/clock/r9a07g044-cpg.h
@@ -30,6 +30,7 @@
 #define R9A07G044_CLK_P2		19
 #define R9A07G044_CLK_AT		20
 #define R9A07G044_OSCCLK		21
+#define R9A07G044_CLK_P0_DIV2		22
 
 /* R9A07G044 Module Clocks */
 #define R9A07G044_CA55_SCLK		0
-- 
2.17.1

[PATCH v2 4/5] clk: renesas: r9a07g044-cpg: Add entry for fixed clock P0_DIV2

[Lad Prabhakar]

Add entry for fixed core clock P0_DIV2 and assign LAST_DT_CORE_CLK
to R9A07G044_CLK_P0_DIV2.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/clk/renesas/r9a07g044-cpg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index f4ebbde358c6..523521a87713 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -16,7 +16,7 @@
 
 enum clk_ids {
 	/* Core Clock Outputs exported to DT */
-	LAST_DT_CORE_CLK = R9A07G044_OSCCLK,
+	LAST_DT_CORE_CLK = R9A07G044_CLK_P0_DIV2,
 
 	/* External Input Clocks */
 	CLK_EXTAL,
@@ -76,6 +76,7 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
 	DEF_FIXED("I", R9A07G044_CLK_I, CLK_PLL1, 1, 1),
 	DEF_DIV("P0", R9A07G044_CLK_P0, CLK_PLL2_DIV16, DIVPL2A,
 		dtable_1_32, CLK_DIVIDER_HIWORD_MASK),
+	DEF_FIXED("P0_DIV2", R9A07G044_CLK_P0_DIV2, R9A07G044_CLK_P0, 1, 2),
 	DEF_FIXED("TSU", R9A07G044_CLK_TSU, CLK_PLL2_DIV20, 1, 1),
 	DEF_DIV("P1", R9A07G044_CLK_P1, CLK_PLL3_DIV2_4,
 		DIVPL3B, dtable_1_32, CLK_DIVIDER_HIWORD_MASK),
-- 
2.17.1

[PATCH v2 5/5] arm64: dts: renesas: r9a07g044: Add CANFD node

[Lad Prabhakar]

Add CANFD node to R9A07G044 (RZ/G2L) SoC DTSI.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
index 28aafa34d583..24032f38e3a1 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
@@ -13,6 +13,13 @@
 	#address-cells = <2>;
 	#size-cells = <2>;
 
+	/* External CAN clock - to be overridden by boards that provide it */
+	can_clk: can {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <0>;
+	};
+
 	/* clock can be either from exclk or crystal oscillator (XIN/XOUT) */
 	extal_clk: extal {
 		compatible = "fixed-clock";
@@ -89,6 +96,41 @@
 			status = "disabled";
 		};
 
+		canfd: can@10050000 {
+			compatible = "renesas,r9a07g044-canfd", "renesas,rzg2l-canfd";
+			reg = <0 0x10050000 0 0x8000>;
+			interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 427 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 422 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 428 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 424 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 423 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 429 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "g_error", "g_rx_fifo",
+					  "can0_error", "can0_tx",
+					  "can0_tx_rx_fifo_receive_completion",
+					  "can1_error", "can1_tx",
+					  "can1_tx_rx_fifo_receive_completion";
+			clocks = <&cpg CPG_MOD R9A07G044_CANFD_PCLK>,
+				 <&cpg CPG_CORE R9A07G044_CLK_P0_DIV2>,
+				 <&can_clk>;
+			clock-names = "fck", "canfd", "can_clk";
+			assigned-clocks = <&cpg CPG_CORE R9A07G044_CLK_P0_DIV2>;
+			assigned-clock-rates = <50000000>;
+			resets = <&cpg R9A07G044_CANFD_RSTP_N>,
+				 <&cpg R9A07G044_CANFD_RSTC_N>;
+			power-domains = <&cpg>;
+			status = "disabled";
+
+			channel0 {
+				status = "disabled";
+			};
+			channel1 {
+				status = "disabled";
+			};
+		};
+
 		i2c0: i2c@10058000 {
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
2.17.1

Re: [PATCH v2 1/5] dt-bindings: net: can: renesas,rcar-canfd: Document RZ/G2L SoC

[Geert Uytterhoeven]

Hi Prabhakar,

On Mon, Jul 19, 2021 at 4:39 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Add CANFD binding documentation for Renesas RZ/G2L SoC.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Just some bikeshedding on the exact naming below ;-)

> --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> @@ -91,6 +92,59 @@ required:
>    - channel0
>    - channel1
>
> +if:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - renesas,rzg2l-canfd
> +then:
> +  properties:
> +    interrupts:
> +      items:
> +        - description: CAN global error interrupt
> +        - description: CAN receive FIFO interrupt
> +        - description: CAN0 error interrupt
> +        - description: CAN0 transmit interrupt
> +        - description: CAN0 transmit/receive FIFO receive completion interrupt
> +        - description: CAN1 error interrupt
> +        - description: CAN1 transmit interrupt
> +        - description: CAN1 transmit/receive FIFO receive completion interrupt
> +
> +    interrupt-names:
> +      items:
> +        - const: g_error
> +        - const: g_rx_fifo
> +        - const: can0_error

s/error/err/?

> +        - const: can0_tx
> +        - const: can0_tx_rx_fifo_receive_completion
> +        - const: can1_error
> +        - const: can1_tx
> +        - const: can1_tx_rx_fifo_receive_completion

s/receive/rx/?

Some are also a bit long to type.
Perhaps use naming closer to the User's Manual?

INTRCANGERR => g_err
INTRCANGRECC => g_recc
INTRCAN0ERR => ch0_err
INTRCAN0REC => ch0_rec
INTRCAN0TRX => ch0_trx
INTRCAN1ERR => ch1_err
INTRCAN1REC => ch1_rec
INTRCAN1TRX => ch1_trx

These do not have "_int" suffixes...

> +
> +    resets:
> +      items:
> +        - description: CANFD_RSTP_N
> +        - description: CANFD_RSTC_N
> +
> +  required:
> +    - interrupt-names
> +else:
> +  properties:
> +    interrupts:
> +      items:
> +        - description: Channel interrupt
> +        - description: Global interrupt
> +
> +    interrupt-names:
> +      items:
> +        - const: ch_int
> +        - const: g_int

... and these do have "_int" suffixes.

> +
> +    resets:
> +      items:
> +        - description: CANFD reset
> +
>  unevaluatedProperties: false

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] dt-bindings: net: can: renesas,rcar-canfd: Document RZ/G2L SoC

[Philipp Zabel]

Hi Lad,

On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar wrote:
> Add CANFD binding documentation for Renesas RZ/G2L SoC.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  .../bindings/net/can/renesas,rcar-canfd.yaml  | 66 +++++++++++++++++--
>  1 file changed, 60 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> index 0b33ba9ccb47..4fb6dd370904 100644
> --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> @@ -30,13 +30,15 @@ properties:
>                - renesas,r8a77995-canfd     # R-Car D3
>            - const: renesas,rcar-gen3-canfd # R-Car Gen3 and RZ/G2
>  
> +      - items:
> +          - enum:
> +              - renesas,r9a07g044-canfd    # RZ/G2{L,LC}
> +          - const: renesas,rzg2l-canfd     # RZ/G2L family
> +
>    reg:
>      maxItems: 1
>  
> -  interrupts:
> -    items:
> -      - description: Channel interrupt
> -      - description: Global interrupt
> +  interrupts: true
>  
>    clocks:
>      maxItems: 3
> @@ -50,8 +52,7 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> -  resets:
> -    maxItems: 1
> +  resets: true
>  
>    renesas,no-can-fd:
>      $ref: /schemas/types.yaml#/definitions/flag
> @@ -91,6 +92,59 @@ required:
>    - channel0
>    - channel1
>  
> +if:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - renesas,rzg2l-canfd
> +then:
> +  properties:
> +    interrupts:
> +      items:
> +        - description: CAN global error interrupt
> +        - description: CAN receive FIFO interrupt
> +        - description: CAN0 error interrupt
> +        - description: CAN0 transmit interrupt
> +        - description: CAN0 transmit/receive FIFO receive completion interrupt
> +        - description: CAN1 error interrupt
> +        - description: CAN1 transmit interrupt
> +        - description: CAN1 transmit/receive FIFO receive completion interrupt
> +
> +    interrupt-names:
> +      items:
> +        - const: g_error
> +        - const: g_rx_fifo
> +        - const: can0_error
> +        - const: can0_tx
> +        - const: can0_tx_rx_fifo_receive_completion
> +        - const: can1_error
> +        - const: can1_tx
> +        - const: can1_tx_rx_fifo_receive_completion
> +
> +    resets:
> +      items:
> +        - description: CANFD_RSTP_N
> +        - description: CANFD_RSTC_N

Do you know what the "P" and "C" stands for? It would be nice if the
description could tell us what the reset lines are used for.

I would prefer if you used these names (or shortened versions, for
example "rstp_n", "rstc_n") as "reset-names" and let the driver
reference the resets by name instead of by index.

regards
Philipp

Re: [PATCH v2 2/5] can: rcar_canfd: Add support for RZ/G2L family

[Philipp Zabel]

On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar wrote:
> CANFD block on RZ/G2L SoC is almost identical to one found on
> R-Car Gen3 SoC's. On RZ/G2L SoC interrupt sources for each channel
> are split into different sources and the IP doesn't divide (1/2)
> CANFD clock within the IP.
> 
> This patch adds compatible string for RZ/G2L family and registers
> the irq handlers required for CANFD operation. IRQ numbers are now
> fetched based on names instead of indices. For backward compatibility
> on non RZ/G2L SoC's we fallback reading based on indices.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/net/can/rcar/rcar_canfd.c | 178 ++++++++++++++++++++++++------
>  1 file changed, 147 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 311e6ca3bdc4..d4affc002fb3 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -37,9 +37,15 @@
[...]
> +	if (gpriv->chip_id == RENESAS_RZG2L) {
> +		gpriv->rstc1 = devm_reset_control_get_exclusive_by_index(&pdev->dev, 0);
> +		if (IS_ERR(gpriv->rstc1)) {
> +			dev_err(&pdev->dev, "failed to get reset index 0\n");

Please consider requesting the reset controls by name instead of by
index. See also my reply to the binding patch.

> +			return PTR_ERR(gpriv->rstc1);
> +		}
> +
> +		err = reset_control_reset(gpriv->rstc1);
> +		if (err)
> +			return err;

I suggest to wait until after all resource requests have succeeded
before triggering the resets, i.e. first get all reset controls and
clocks, etc., and only then trigger resets, enable clocks, and so on.

That way there will be no spurious resets in case of probe deferrals.

regards
Philipp

Re: [PATCH v2 2/5] can: rcar_canfd: Add support for RZ/G2L family

[Geert Uytterhoeven]

Hi Prabhakar,

On Mon, Jul 19, 2021 at 4:39 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> CANFD block on RZ/G2L SoC is almost identical to one found on
> R-Car Gen3 SoC's. On RZ/G2L SoC interrupt sources for each channel
> are split into different sources and the IP doesn't divide (1/2)
> CANFD clock within the IP.
>
> This patch adds compatible string for RZ/G2L family and registers
> the irq handlers required for CANFD operation. IRQ numbers are now
> fetched based on names instead of indices. For backward compatibility
> on non RZ/G2L SoC's we fallback reading based on indices.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -37,9 +37,15 @@
>  #include <linux/bitmap.h>
>  #include <linux/bitops.h>
>  #include <linux/iopoll.h>
> +#include <linux/reset.h>
>
>  #define RCANFD_DRV_NAME                        "rcar_canfd"
>
> +enum rcanfd_chip_id {
> +       RENESAS_RCAR_GEN3 = 0,
> +       RENESAS_RZG2L,
> +};
> +
>  /* Global register bits */
>
>  /* RSCFDnCFDGRMCFG */
> @@ -513,6 +519,9 @@ struct rcar_canfd_global {
>         enum rcar_canfd_fcanclk fcan;   /* CANFD or Ext clock */
>         unsigned long channels_mask;    /* Enabled channels mask */
>         bool fdmode;                    /* CAN FD or Classical CAN only mode */
> +       struct reset_control *rstc1;     /* Pointer to reset source1 */
> +       struct reset_control *rstc2;     /* Pointer to reset source2 */

Are these comments helpful? IMHO they're stating the obvious.

> +       enum rcanfd_chip_id chip_id;
>  };
>
>  /* CAN FD mode nominal rate constants */
> @@ -1577,6 +1586,45 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
>         priv->can.clock.freq = fcan_freq;
>         dev_info(&pdev->dev, "can_clk rate is %u\n", priv->can.clock.freq);
>
> +       if (gpriv->chip_id == RENESAS_RZG2L) {
> +               char *irq_name;
> +               int err_irq;
> +               int tx_irq;
> +
> +               err_irq = platform_get_irq_byname(pdev, ch == 0 ? "can0_error" : "can1_error");
> +               if (err_irq < 0) {
> +                       err = err_irq;
> +                       goto fail;
> +               }
> +
> +               tx_irq = platform_get_irq_byname(pdev, ch == 0 ? "can0_tx" : "can1_tx");
> +               if (tx_irq < 0) {
> +                       err = tx_irq;
> +                       goto fail;
> +               }
> +
> +               irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +                                         "canfd.chnerr%d", ch);

if (!irq_name) {
    ret = -ENOMEM;
    goto fail;
}

> +               err = devm_request_irq(&pdev->dev, err_irq,
> +                                      rcar_canfd_channel_interrupt, 0,
> +                                      irq_name, gpriv);
> +               if (err) {
> +                       dev_err(&pdev->dev, "devm_request_irq CH Err(%d) failed, error %d\n",
> +                               err_irq, err);
> +                       goto fail;
> +               }
> +               irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +                                         "canfd.chntx%d", ch);

Likewise.

> +               err = devm_request_irq(&pdev->dev, tx_irq,
> +                                      rcar_canfd_channel_interrupt, 0,
> +                                      irq_name, gpriv);
> +               if (err) {
> +                       dev_err(&pdev->dev, "devm_request_irq Tx (%d) failed, error %d\n",
> +                               tx_irq, err);
> +                       goto fail;
> +               }
> +       }
> +
>         if (gpriv->fdmode) {
>                 priv->can.bittiming_const = &rcar_canfd_nom_bittiming_const;
>                 priv->can.data_bittiming_const =

> @@ -1649,27 +1700,64 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>         if (of_child && of_device_is_available(of_child))
>                 channels_mask |= BIT(1);        /* Channel 1 */
>
> -       ch_irq = platform_get_irq(pdev, 0);
> -       if (ch_irq < 0) {
> -               err = ch_irq;
> -               goto fail_dev;
> -       }
> +       if (chip_id == RENESAS_RCAR_GEN3) {
> +               ch_irq = platform_get_irq_byname(pdev, "ch_int");

platform_get_irq_byname_optional()?
Unless you want to urge people to update their DTB.

> +               if (ch_irq < 0) {
> +                       /* For backward compatibility get irq by index */
> +                       ch_irq = platform_get_irq(pdev, 0);
> +                       if (ch_irq < 0)
> +                               return ch_irq;
> +               }
>
> -       g_irq = platform_get_irq(pdev, 1);
> -       if (g_irq < 0) {
> -               err = g_irq;
> -               goto fail_dev;
> +               g_irq = platform_get_irq_byname(pdev, "g_int");

Likewise,

> +               if (g_irq < 0) {
> +                       /* For backward compatibility get irq by index */
> +                       g_irq = platform_get_irq(pdev, 1);
> +                       if (g_irq < 0)
> +                               return g_irq;
> +               }
> +       } else {
> +               g_irq = platform_get_irq_byname(pdev, "g_error");
> +               if (g_irq < 0)
> +                       return g_irq;
> +
> +               g_rx_irq = platform_get_irq_byname(pdev, "g_rx_fifo");
> +               if (g_rx_irq < 0)
> +                       return g_rx_irq;
>         }
>

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] dt-bindings: clk: r9a07g044-cpg: Add entry for P0_DIV2 core clock

[Geert Uytterhoeven]

On Mon, Jul 19, 2021 at 4:39 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Add P0_DIV2 core clock required for CANFD module. CANFD core clock is
> sourced from P0_DIV2 referenced from HW manual Rev.0.50.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-r9a07g044-dt-binding-defs, to be shared by
renesas-clk-for-v5.15 and renesas-devel for v5.15.

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] clk: renesas: r9a07g044-cpg: Add entry for fixed clock P0_DIV2

[Geert Uytterhoeven]

On Mon, Jul 19, 2021 at 4:40 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Add entry for fixed core clock P0_DIV2 and assign LAST_DT_CORE_CLK
> to R9A07G044_CLK_P0_DIV2.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-clk-for-v5.15.

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] dt-bindings: net: can: renesas,rcar-canfd: Document RZ/G2L SoC

[Lad, Prabhakar]

Hi Philipp,

Thank you for the review.

On Tue, Jul 20, 2021 at 11:22 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Hi Lad,
>
> On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar wrote:
> > Add CANFD binding documentation for Renesas RZ/G2L SoC.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  .../bindings/net/can/renesas,rcar-canfd.yaml  | 66 +++++++++++++++++--
> >  1 file changed, 60 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > index 0b33ba9ccb47..4fb6dd370904 100644
> > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > @@ -30,13 +30,15 @@ properties:
> >                - renesas,r8a77995-canfd     # R-Car D3
> >            - const: renesas,rcar-gen3-canfd # R-Car Gen3 and RZ/G2
> >
> > +      - items:
> > +          - enum:
> > +              - renesas,r9a07g044-canfd    # RZ/G2{L,LC}
> > +          - const: renesas,rzg2l-canfd     # RZ/G2L family
> > +
> >    reg:
> >      maxItems: 1
> >
> > -  interrupts:
> > -    items:
> > -      - description: Channel interrupt
> > -      - description: Global interrupt
> > +  interrupts: true
> >
> >    clocks:
> >      maxItems: 3
> > @@ -50,8 +52,7 @@ properties:
> >    power-domains:
> >      maxItems: 1
> >
> > -  resets:
> > -    maxItems: 1
> > +  resets: true
> >
> >    renesas,no-can-fd:
> >      $ref: /schemas/types.yaml#/definitions/flag
> > @@ -91,6 +92,59 @@ required:
> >    - channel0
> >    - channel1
> >
> > +if:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - renesas,rzg2l-canfd
> > +then:
> > +  properties:
> > +    interrupts:
> > +      items:
> > +        - description: CAN global error interrupt
> > +        - description: CAN receive FIFO interrupt
> > +        - description: CAN0 error interrupt
> > +        - description: CAN0 transmit interrupt
> > +        - description: CAN0 transmit/receive FIFO receive completion interrupt
> > +        - description: CAN1 error interrupt
> > +        - description: CAN1 transmit interrupt
> > +        - description: CAN1 transmit/receive FIFO receive completion interrupt
> > +
> > +    interrupt-names:
> > +      items:
> > +        - const: g_error
> > +        - const: g_rx_fifo
> > +        - const: can0_error
> > +        - const: can0_tx
> > +        - const: can0_tx_rx_fifo_receive_completion
> > +        - const: can1_error
> > +        - const: can1_tx
> > +        - const: can1_tx_rx_fifo_receive_completion
> > +
> > +    resets:
> > +      items:
> > +        - description: CANFD_RSTP_N
> > +        - description: CANFD_RSTC_N
>
> Do you know what the "P" and "C" stands for? It would be nice if the
> description could tell us what the reset lines are used for.
>
unfortunately the HW manual does not mention  anything about "P" and "C" :(

> I would prefer if you used these names (or shortened versions, for
> example "rstp_n", "rstc_n") as "reset-names" and let the driver
> reference the resets by name instead of by index.
>
OK will do that and maxItems:2 for resets.

@Geert, for R-Car Gen3 does "canfd_rst" (as it's a module reset)
sounds good for reset-names? Or do you have any other suggestions?

Cheers,
Prabhakar

Re: [PATCH v2 1/5] dt-bindings: net: can: renesas,rcar-canfd: Document RZ/G2L SoC

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Tue, Jul 20, 2021 at 11:21 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Jul 19, 2021 at 4:39 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > Add CANFD binding documentation for Renesas RZ/G2L SoC.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Just some bikeshedding on the exact naming below ;-)
>
> > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > @@ -91,6 +92,59 @@ required:
> >    - channel0
> >    - channel1
> >
> > +if:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - renesas,rzg2l-canfd
> > +then:
> > +  properties:
> > +    interrupts:
> > +      items:
> > +        - description: CAN global error interrupt
> > +        - description: CAN receive FIFO interrupt
> > +        - description: CAN0 error interrupt
> > +        - description: CAN0 transmit interrupt
> > +        - description: CAN0 transmit/receive FIFO receive completion interrupt
> > +        - description: CAN1 error interrupt
> > +        - description: CAN1 transmit interrupt
> > +        - description: CAN1 transmit/receive FIFO receive completion interrupt
> > +
> > +    interrupt-names:
> > +      items:
> > +        - const: g_error
> > +        - const: g_rx_fifo
> > +        - const: can0_error
>
> s/error/err/?
>
> > +        - const: can0_tx
> > +        - const: can0_tx_rx_fifo_receive_completion
> > +        - const: can1_error
> > +        - const: can1_tx
> > +        - const: can1_tx_rx_fifo_receive_completion
>
> s/receive/rx/?
>
> Some are also a bit long to type.
> Perhaps use naming closer to the User's Manual?
>
> INTRCANGERR => g_err
> INTRCANGRECC => g_recc
> INTRCAN0ERR => ch0_err
> INTRCAN0REC => ch0_rec
> INTRCAN0TRX => ch0_trx
> INTRCAN1ERR => ch1_err
> INTRCAN1REC => ch1_rec
> INTRCAN1TRX => ch1_trx
>
> These do not have "_int" suffixes...
>
Agreed thanks for the input.

> > +
> > +    resets:
> > +      items:
> > +        - description: CANFD_RSTP_N
> > +        - description: CANFD_RSTC_N
> > +
> > +  required:
> > +    - interrupt-names
> > +else:
> > +  properties:
> > +    interrupts:
> > +      items:
> > +        - description: Channel interrupt
> > +        - description: Global interrupt
> > +
> > +    interrupt-names:
> > +      items:
> > +        - const: ch_int
> > +        - const: g_int
>
> ... and these do have "_int" suffixes.
>
indeed

Cheers,
Prabhakar
> > +
> > +    resets:
> > +      items:
> > +        - description: CANFD reset
> > +
> >  unevaluatedProperties: false
>
> 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 RZ/G2L family

[Lad, Prabhakar]

Hi Philipp,

Thank you for the review.

On Tue, Jul 20, 2021 at 11:23 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar wrote:
> > CANFD block on RZ/G2L SoC is almost identical to one found on
> > R-Car Gen3 SoC's. On RZ/G2L SoC interrupt sources for each channel
> > are split into different sources and the IP doesn't divide (1/2)
> > CANFD clock within the IP.
> >
> > This patch adds compatible string for RZ/G2L family and registers
> > the irq handlers required for CANFD operation. IRQ numbers are now
> > fetched based on names instead of indices. For backward compatibility
> > on non RZ/G2L SoC's we fallback reading based on indices.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/net/can/rcar/rcar_canfd.c | 178 ++++++++++++++++++++++++------
> >  1 file changed, 147 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> > index 311e6ca3bdc4..d4affc002fb3 100644
> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -37,9 +37,15 @@
> [...]
> > +     if (gpriv->chip_id == RENESAS_RZG2L) {
> > +             gpriv->rstc1 = devm_reset_control_get_exclusive_by_index(&pdev->dev, 0);
> > +             if (IS_ERR(gpriv->rstc1)) {
> > +                     dev_err(&pdev->dev, "failed to get reset index 0\n");
>
> Please consider requesting the reset controls by name instead of by
> index. See also my reply to the binding patch.
>
Will do.

> > +                     return PTR_ERR(gpriv->rstc1);
> > +             }
> > +
> > +             err = reset_control_reset(gpriv->rstc1);
> > +             if (err)
> > +                     return err;
>
> I suggest to wait until after all resource requests have succeeded
> before triggering the resets, i.e. first get all reset controls and
> clocks, etc., and only then trigger resets, enable clocks, and so on.
>
> That way there will be no spurious resets in case of probe deferrals.
>
Agreed, will update the code.

Cheers,
Prabhakar

> regards
> Philipp

Re: [PATCH v2 1/5] dt-bindings: net: can: renesas,rcar-canfd: Document RZ/G2L SoC

[Geert Uytterhoeven]

Hi Prabhakar,

On Tue, Jul 20, 2021 at 4:31 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Tue, Jul 20, 2021 at 11:22 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar wrote:
> > > Add CANFD binding documentation for Renesas RZ/G2L SoC.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

> > > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml

> > > +    resets:
> > > +      items:
> > > +        - description: CANFD_RSTP_N
> > > +        - description: CANFD_RSTC_N
> >
> > Do you know what the "P" and "C" stands for? It would be nice if the
> > description could tell us what the reset lines are used for.
> >
> unfortunately the HW manual does not mention  anything about "P" and "C" :(
>
> > I would prefer if you used these names (or shortened versions, for
> > example "rstp_n", "rstc_n") as "reset-names" and let the driver
> > reference the resets by name instead of by index.
> >
> OK will do that and maxItems:2 for resets.
>
> @Geert, for R-Car Gen3 does "canfd_rst" (as it's a module reset)
> sounds good for reset-names? Or do you have any other suggestions?

I wouldn't bother with reset-names on R-Car, as there is only a
single reset.

BTW, does there exist a generally-accepted reset-equivalent of "fck"
("Functional ClocK")?

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 RZ/G2L family

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Tue, Jul 20, 2021 at 11:31 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Jul 19, 2021 at 4:39 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > CANFD block on RZ/G2L SoC is almost identical to one found on
> > R-Car Gen3 SoC's. On RZ/G2L SoC interrupt sources for each channel
> > are split into different sources and the IP doesn't divide (1/2)
> > CANFD clock within the IP.
> >
> > This patch adds compatible string for RZ/G2L family and registers
> > the irq handlers required for CANFD operation. IRQ numbers are now
> > fetched based on names instead of indices. For backward compatibility
> > on non RZ/G2L SoC's we fallback reading based on indices.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
>
> Thanks for your patch!
>
> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -37,9 +37,15 @@
> >  #include <linux/bitmap.h>
> >  #include <linux/bitops.h>
> >  #include <linux/iopoll.h>
> > +#include <linux/reset.h>
> >
> >  #define RCANFD_DRV_NAME                        "rcar_canfd"
> >
> > +enum rcanfd_chip_id {
> > +       RENESAS_RCAR_GEN3 = 0,
> > +       RENESAS_RZG2L,
> > +};
> > +
> >  /* Global register bits */
> >
> >  /* RSCFDnCFDGRMCFG */
> > @@ -513,6 +519,9 @@ struct rcar_canfd_global {
> >         enum rcar_canfd_fcanclk fcan;   /* CANFD or Ext clock */
> >         unsigned long channels_mask;    /* Enabled channels mask */
> >         bool fdmode;                    /* CAN FD or Classical CAN only mode */
> > +       struct reset_control *rstc1;     /* Pointer to reset source1 */
> > +       struct reset_control *rstc2;     /* Pointer to reset source2 */
>
> Are these comments helpful? IMHO they're stating the obvious.
>
No :D will drop those.

> > +       enum rcanfd_chip_id chip_id;
> >  };
> >
> >  /* CAN FD mode nominal rate constants */
> > @@ -1577,6 +1586,45 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
> >         priv->can.clock.freq = fcan_freq;
> >         dev_info(&pdev->dev, "can_clk rate is %u\n", priv->can.clock.freq);
> >
> > +       if (gpriv->chip_id == RENESAS_RZG2L) {
> > +               char *irq_name;
> > +               int err_irq;
> > +               int tx_irq;
> > +
> > +               err_irq = platform_get_irq_byname(pdev, ch == 0 ? "can0_error" : "can1_error");
> > +               if (err_irq < 0) {
> > +                       err = err_irq;
> > +                       goto fail;
> > +               }
> > +
> > +               tx_irq = platform_get_irq_byname(pdev, ch == 0 ? "can0_tx" : "can1_tx");
> > +               if (tx_irq < 0) {
> > +                       err = tx_irq;
> > +                       goto fail;
> > +               }
> > +
> > +               irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > +                                         "canfd.chnerr%d", ch);
>
> if (!irq_name) {
>     ret = -ENOMEM;
>     goto fail;
> }
>
> > +               err = devm_request_irq(&pdev->dev, err_irq,
> > +                                      rcar_canfd_channel_interrupt, 0,
> > +                                      irq_name, gpriv);
> > +               if (err) {
> > +                       dev_err(&pdev->dev, "devm_request_irq CH Err(%d) failed, error %d\n",
> > +                               err_irq, err);
> > +                       goto fail;
> > +               }
> > +               irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > +                                         "canfd.chntx%d", ch);
>
> Likewise.
>
> > +               err = devm_request_irq(&pdev->dev, tx_irq,
> > +                                      rcar_canfd_channel_interrupt, 0,
> > +                                      irq_name, gpriv);
> > +               if (err) {
> > +                       dev_err(&pdev->dev, "devm_request_irq Tx (%d) failed, error %d\n",
> > +                               tx_irq, err);
> > +                       goto fail;
> > +               }
> > +       }
> > +
> >         if (gpriv->fdmode) {
> >                 priv->can.bittiming_const = &rcar_canfd_nom_bittiming_const;
> >                 priv->can.data_bittiming_const =
>
> > @@ -1649,27 +1700,64 @@ static int rcar_canfd_probe(struct platform_device *pdev)
> >         if (of_child && of_device_is_available(of_child))
> >                 channels_mask |= BIT(1);        /* Channel 1 */
> >
> > -       ch_irq = platform_get_irq(pdev, 0);
> > -       if (ch_irq < 0) {
> > -               err = ch_irq;
> > -               goto fail_dev;
> > -       }
> > +       if (chip_id == RENESAS_RCAR_GEN3) {
> > +               ch_irq = platform_get_irq_byname(pdev, "ch_int");
>
> platform_get_irq_byname_optional()?
> Unless you want to urge people to update their DTB.
>
Good point will change it to platform_get_irq_byname_optional().

> > +               if (ch_irq < 0) {
> > +                       /* For backward compatibility get irq by index */
> > +                       ch_irq = platform_get_irq(pdev, 0);
> > +                       if (ch_irq < 0)
> > +                               return ch_irq;
> > +               }
> >
> > -       g_irq = platform_get_irq(pdev, 1);
> > -       if (g_irq < 0) {
> > -               err = g_irq;
> > -               goto fail_dev;
> > +               g_irq = platform_get_irq_byname(pdev, "g_int");
>
> Likewise,
>
agreed

Cheers,
Prabhakar

> > +               if (g_irq < 0) {
> > +                       /* For backward compatibility get irq by index */
> > +                       g_irq = platform_get_irq(pdev, 1);
> > +                       if (g_irq < 0)
> > +                               return g_irq;
> > +               }
> > +       } else {
> > +               g_irq = platform_get_irq_byname(pdev, "g_error");
> > +               if (g_irq < 0)
> > +                       return g_irq;
> > +
> > +               g_rx_irq = platform_get_irq_byname(pdev, "g_rx_fifo");
> > +               if (g_rx_irq < 0)
> > +                       return g_rx_irq;
> >         }
> >
>
> 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] dt-bindings: net: can: renesas,rcar-canfd: Document RZ/G2L SoC

[Lad, Prabhakar]

Hi Geert,

On Tue, Jul 20, 2021 at 4:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, Jul 20, 2021 at 4:31 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Tue, Jul 20, 2021 at 11:22 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar wrote:
> > > > Add CANFD binding documentation for Renesas RZ/G2L SoC.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
>
> > > > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > > > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
>
> > > > +    resets:
> > > > +      items:
> > > > +        - description: CANFD_RSTP_N
> > > > +        - description: CANFD_RSTC_N
> > >
> > > Do you know what the "P" and "C" stands for? It would be nice if the
> > > description could tell us what the reset lines are used for.
> > >
> > unfortunately the HW manual does not mention  anything about "P" and "C" :(
> >
> > > I would prefer if you used these names (or shortened versions, for
> > > example "rstp_n", "rstc_n") as "reset-names" and let the driver
> > > reference the resets by name instead of by index.
> > >
> > OK will do that and maxItems:2 for resets.
> >
> > @Geert, for R-Car Gen3 does "canfd_rst" (as it's a module reset)
> > sounds good for reset-names? Or do you have any other suggestions?
>
> I wouldn't bother with reset-names on R-Car, as there is only a
> single reset.
>
OK will keep "description: CANFD reset" for R-Car as done in the
current patch and just add reset-names only for RZ/G2L SoC.

> BTW, does there exist a generally-accepted reset-equivalent of "fck"
> ("Functional ClocK")?
>
None that I am aware of (Couple of binding docs have "rst"), but maybe
Philipp could have some suggestions.

Cheers,
Prabhakar

> 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] dt-bindings: net: can: renesas,rcar-canfd: Document RZ/G2L SoC

[Philipp Zabel]

Hi Prabhakar,

On Tue, 2021-07-20 at 15:31 +0100, Lad, Prabhakar wrote:
> Hi Philipp,
> 
> Thank you for the review.
> 
> On Tue, Jul 20, 2021 at 11:22 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Hi Lad,

Sorry I mixed up your name.

> > On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar wrote:
> > > Add CANFD binding documentation for Renesas RZ/G2L SoC.
> > > 
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > >  .../bindings/net/can/renesas,rcar-canfd.yaml  | 66 +++++++++++++++++--
> > >  1 file changed, 60 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > > index 0b33ba9ccb47..4fb6dd370904 100644
> > > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > > @@ -30,13 +30,15 @@ properties:
> > >                - renesas,r8a77995-canfd     # R-Car D3
> > >            - const: renesas,rcar-gen3-canfd # R-Car Gen3 and RZ/G2
> > > 
> > > +      - items:
> > > +          - enum:
> > > +              - renesas,r9a07g044-canfd    # RZ/G2{L,LC}
> > > +          - const: renesas,rzg2l-canfd     # RZ/G2L family
> > > +
> > >    reg:
> > >      maxItems: 1
> > > 
> > > -  interrupts:
> > > -    items:
> > > -      - description: Channel interrupt
> > > -      - description: Global interrupt
> > > +  interrupts: true
> > > 
> > >    clocks:
> > >      maxItems: 3
> > > @@ -50,8 +52,7 @@ properties:
> > >    power-domains:
> > >      maxItems: 1
> > > 
> > > -  resets:
> > > -    maxItems: 1
> > > +  resets: true
> > > 
> > >    renesas,no-can-fd:
> > >      $ref: /schemas/types.yaml#/definitions/flag
> > > @@ -91,6 +92,59 @@ required:
> > >    - channel0
> > >    - channel1
> > > 
> > > +if:
> > > +  properties:
> > > +    compatible:
> > > +      contains:
> > > +        enum:
> > > +          - renesas,rzg2l-canfd
> > > +then:
> > > +  properties:
> > > +    interrupts:
> > > +      items:
> > > +        - description: CAN global error interrupt
> > > +        - description: CAN receive FIFO interrupt
> > > +        - description: CAN0 error interrupt
> > > +        - description: CAN0 transmit interrupt
> > > +        - description: CAN0 transmit/receive FIFO receive completion interrupt
> > > +        - description: CAN1 error interrupt
> > > +        - description: CAN1 transmit interrupt
> > > +        - description: CAN1 transmit/receive FIFO receive completion interrupt
> > > +
> > > +    interrupt-names:
> > > +      items:
> > > +        - const: g_error
> > > +        - const: g_rx_fifo
> > > +        - const: can0_error
> > > +        - const: can0_tx
> > > +        - const: can0_tx_rx_fifo_receive_completion
> > > +        - const: can1_error
> > > +        - const: can1_tx
> > > +        - const: can1_tx_rx_fifo_receive_completion
> > > +
> > > +    resets:
> > > +      items:
> > > +        - description: CANFD_RSTP_N
> > > +        - description: CANFD_RSTC_N
> > 
> > Do you know what the "P" and "C" stands for? It would be nice if the
> > description could tell us what the reset lines are used for.
> > 
> unfortunately the HW manual does not mention  anything about "P" and "C" :(

Yes, unfortunately this is all too common.

> > I would prefer if you used these names (or shortened versions, for
> > example "rstp_n", "rstc_n") as "reset-names" and let the driver
> > reference the resets by name instead of by index.
> > 
> OK will do that and maxItems:2 for resets.
> 
> @Geert, for R-Car Gen3 does "canfd_rst" (as it's a module reset)
> sounds good for reset-names? Or do you have any other suggestions?

I agree with Geert here. Assuming no second reset will be discovered for
R-Car Gen3 later, there is no need to invent a name.

regards
Philipp

Re: [PATCH v2 1/5] dt-bindings: net: can: renesas,rcar-canfd: Document RZ/G2L SoC

[Philipp Zabel]

Hi Geert,

On Tue, 2021-07-20 at 17:11 +0200, Geert Uytterhoeven wrote:
[...]
> I wouldn't bother with reset-names on R-Car, as there is only a
> single reset.
> 
> BTW, does there exist a generally-accepted reset-equivalent of "fck"
> ("Functional ClocK")?

Not really. There is "rst", which seems to be slightly more popular than
"reset". Some bindings use "core" to differentiate between the
functional reset and peripheral resets like bus or phy resets.

Ideally the reset-names would match the names of the reset inputs in the
IP core documentation (not the global names of the reset signals in the
SoC documentation). But more often than not they are not known.

regards
Philipp