[PATCH 0/6] Add support for RZ/N1 SJA1000 CAN controller

[PATCH 0/6] Add support for RZ/N1 SJA1000 CAN controller

[Biju Das]

This patch series aims to add support for RZ/N1 SJA1000 CAN controller.

The SJA1000 CAN controller on RZ/N1 SoC has some differences compared
to others like it has no clock divider register (CDR) support and it has
no HW loopback(HW doesn't see tx messages on rx), so introduced a new
compatible 'renesas,rzn1-sja1000' to handle these differences.

Ref:
 [1] https://lore.kernel.org/linux-renesas-soc/20220701162320.102165-1-biju.das.jz@bp.renesas.com/T/#t

Biju Das (6):
  dt-bindings: can: sja1000: Convert to json-schema
  dt-bindings: can: nxp,sja1000: Document RZ/N1{D,S} support
  can: sja1000: Add Quirks for RZ/N1 SJA1000 CAN controller
  can: sja1000: Use of_device_get_match_data to get device data
  can: sja1000: Change the return type as void for SoC specific init
  can: sja1000: Add support for RZ/N1 SJA1000 CAN Controller

 .../bindings/net/can/nxp,sja1000.yaml         | 128 ++++++++++++++++++
 .../devicetree/bindings/net/can/sja1000.txt   |  58 --------
 drivers/net/can/sja1000/sja1000.c             |  17 ++-
 drivers/net/can/sja1000/sja1000.h             |   4 +-
 drivers/net/can/sja1000/sja1000_platform.c    |  52 ++++---
 5 files changed, 176 insertions(+), 83 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
 delete mode 100644 Documentation/devicetree/bindings/net/can/sja1000.txt

-- 
2.25.1

[PATCH 1/6] dt-bindings: can: sja1000: Convert to json-schema

[Biju Das]

Convert the NXP SJA1000 CAN Controller Device Tree binding
documentation to json-schema.

Update the example to match reality.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 .../bindings/net/can/nxp,sja1000.yaml         | 106 ++++++++++++++++++
 .../devicetree/bindings/net/can/sja1000.txt   |  58 ----------
 2 files changed, 106 insertions(+), 58 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
 delete mode 100644 Documentation/devicetree/bindings/net/can/sja1000.txt

diff --git a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
new file mode 100644
index 000000000000..91d0f1b25d10
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
@@ -0,0 +1,106 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/can/nxp,sja1000.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
+
+maintainers:
+  - Wolfgang Grandegger <wg@grandegger.com>
+
+properties:
+  compatible:
+    oneOf:
+      - description: NXP SJA1000 CAN Controller
+        const: nxp,sja1000
+      - description: Technologic Systems SJA1000 CAN Controller
+        const: technologic,sja1000
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  reg-io-width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: I/O register width (in bytes) implemented by this device
+    default: 1
+    enum: [ 1, 2, 4 ]
+
+  nxp,external-clock-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 16000000
+    description: |
+      Frequency of the external oscillator clock in Hz.
+      The internal clock frequency used by the SJA1000 is half of that value.
+
+  nxp,tx-output-mode:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1, 2, 3 ]
+    default: 1
+    description: |
+      operation mode of the TX output control logic.  Valid values are:
+        <0x0> : bi-phase output mode
+        <0x1> : normal output mode (default)
+        <0x2> : test output mode
+        <0x3> : clock output mode
+
+  nxp,tx-output-config:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0x01, 0x02, 0x04, 0x06, 0x08, 0x10, 0x20, 0x30 ]
+    default: 0x02
+    description: |
+      TX output pin configuration.  Valid values are:
+        <0x01> : TX0 invert
+        <0x02> : TX0 pull-down (default)
+        <0x04> : TX0 pull-up
+        <0x06> : TX0 push-pull
+        <0x08> : TX1 invert
+        <0x10> : TX1 pull-down
+        <0x20> : TX1 pull-up
+        <0x30> : TX1 push-pull
+
+  nxp,clock-out-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      clock frequency in Hz on the CLKOUT pin.
+      If not specified or if the specified value is 0, the CLKOUT pin
+      will be disabled.
+
+  nxp,no-comparator-bypass:
+    type: boolean
+    description: Allows to disable the CAN input comparator.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+allOf:
+  - $ref: can-controller.yaml#
+
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              const: technologic,sja1000
+    then:
+      required:
+        - reg-io-width
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    can@1a000 {
+            #address-cells = <0x2>;
+            #size-cells = <0x1>;
+
+            compatible = "technologic,sja1000";
+            reg = <0x1a000 0x100>;
+            interrupts = <1>;
+            nxp,external-clock-frequency = <24000000>;
+    };
diff --git a/Documentation/devicetree/bindings/net/can/sja1000.txt b/Documentation/devicetree/bindings/net/can/sja1000.txt
deleted file mode 100644
index ac3160eca96a..000000000000
--- a/Documentation/devicetree/bindings/net/can/sja1000.txt
+++ /dev/null
@@ -1,58 +0,0 @@
-Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
-
-Required properties:
-
-- compatible : should be one of "nxp,sja1000", "technologic,sja1000".
-
-- reg : should specify the chip select, address offset and size required
-	to map the registers of the SJA1000. The size is usually 0x80.
-
-- interrupts: property with a value describing the interrupt source
-	(number and sensitivity) required for the SJA1000.
-
-Optional properties:
-
-- reg-io-width : Specify the size (in bytes) of the IO accesses that
-	should be performed on the device.  Valid value is 1, 2 or 4.
-	This property is ignored for technologic version.
-	Default to 1 (8 bits).
-
-- nxp,external-clock-frequency : Frequency of the external oscillator
-	clock in Hz. Note that the internal clock frequency used by the
-	SJA1000 is half of that value. If not specified, a default value
-	of 16000000 (16 MHz) is used.
-
-- nxp,tx-output-mode : operation mode of the TX output control logic:
-	<0x0> : bi-phase output mode
-	<0x1> : normal output mode (default)
-	<0x2> : test output mode
-	<0x3> : clock output mode
-
-- nxp,tx-output-config : TX output pin configuration:
-	<0x01> : TX0 invert
-	<0x02> : TX0 pull-down (default)
-	<0x04> : TX0 pull-up
-	<0x06> : TX0 push-pull
-	<0x08> : TX1 invert
-	<0x10> : TX1 pull-down
-	<0x20> : TX1 pull-up
-	<0x30> : TX1 push-pull
-
-- nxp,clock-out-frequency : clock frequency in Hz on the CLKOUT pin.
-	If not specified or if the specified value is 0, the CLKOUT pin
-	will be disabled.
-
-- nxp,no-comparator-bypass : Allows to disable the CAN input comparator.
-
-For further information, please have a look to the SJA1000 data sheet.
-
-Examples:
-
-can@3,100 {
-	compatible = "nxp,sja1000";
-	reg = <3 0x100 0x80>;
-	interrupts = <2 0>;
-	interrupt-parent = <&mpic>;
-	nxp,external-clock-frequency = <16000000>;
-};
-
-- 
2.25.1

[PATCH 2/6] dt-bindings: can: nxp,sja1000: Document RZ/N1{D,S} support

[Biju Das]

Add CAN binding documentation for Renesas RZ/N1 SoC.

The SJA1000 CAN controller on RZ/N1 SoC has some differences compared
to others like it has no clock divider register (CDR) support and it has
no HW loopback(HW doesn't see tx messages on rx), so introduced a new
compatible 'renesas,rzn1-sja1000' to handle these differences.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 .../bindings/net/can/nxp,sja1000.yaml         | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
index 91d0f1b25d10..d0d374b979ec 100644
--- a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
+++ b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
@@ -16,6 +16,12 @@ properties:
         const: nxp,sja1000
       - description: Technologic Systems SJA1000 CAN Controller
         const: technologic,sja1000
+      - description: Renesas RZ/N1 SJA1000 CAN Controller
+        items:
+          - enum:
+              - renesas,r9a06g032-sja1000 # RZ/N1D
+              - renesas,r9a06g033-sja1000 # RZ/N1S
+          - const: renesas,rzn1-sja1000 # RZ/N1
 
   reg:
     maxItems: 1
@@ -23,6 +29,12 @@ properties:
   interrupts:
     maxItems: 1
 
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: can_clk
+
   reg-io-width:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: I/O register width (in bytes) implemented by this device
@@ -91,6 +103,16 @@ allOf:
       required:
         - reg-io-width
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,rzn1-sja1000
+    then:
+      required:
+        - clocks
+        - clock-names
+
 unevaluatedProperties: false
 
 examples:
-- 
2.25.1

[PATCH 3/6] can: sja1000: Add Quirks for RZ/N1 SJA1000 CAN controller

[Biju Das]

Chapter 6.5.16 of the RZ/N1 Peripheral Manual mentions the below
differences compared to the reference Philips SJA1000 device.

Handling of Transmitted Messages:
 * The CAN controller does not copy transmitted messages to the receive
   buffer, unlike the reference device.

Clock Divider Register:
 * This register is not supported

This patch adds device quirks to handle these differences.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/can/sja1000/sja1000.c | 17 +++++++++++------
 drivers/net/can/sja1000/sja1000.h |  4 +++-
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 2e7638f98cf1..49cf4fc4d896 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -183,8 +183,9 @@ static void chipset_init(struct net_device *dev)
 {
 	struct sja1000_priv *priv = netdev_priv(dev);
 
-	/* set clock divider and output control register */
-	priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN);
+	if (!(priv->flags & SJA1000_NO_CDR_REG_QUIRK))
+		/* set clock divider and output control register */
+		priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN);
 
 	/* set acceptance filter (accept all) */
 	priv->write_reg(priv, SJA1000_ACCC0, 0x00);
@@ -208,9 +209,11 @@ static void sja1000_start(struct net_device *dev)
 	if (priv->can.state != CAN_STATE_STOPPED)
 		set_reset_mode(dev);
 
-	/* Initialize chip if uninitialized at this stage */
-	if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN))
-		chipset_init(dev);
+	if (!(priv->flags & SJA1000_NO_CDR_REG_QUIRK)) {
+		/* Initialize chip if uninitialized at this stage */
+		if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN))
+			chipset_init(dev);
+	}
 
 	/* Clear error counters and error code capture */
 	priv->write_reg(priv, SJA1000_TXERR, 0x0);
@@ -652,12 +655,14 @@ static const struct net_device_ops sja1000_netdev_ops = {
 
 int register_sja1000dev(struct net_device *dev)
 {
+	struct sja1000_priv *priv = netdev_priv(dev);
 	int ret;
 
 	if (!sja1000_probe_chip(dev))
 		return -ENODEV;
 
-	dev->flags |= IFF_ECHO;	/* we support local echo */
+	if (!(priv->flags & SJA1000_NO_HW_LOOPBACK_QUIRK))
+		dev->flags |= IFF_ECHO;	/* we support local echo */
 	dev->netdev_ops = &sja1000_netdev_ops;
 
 	set_reset_mode(dev);
diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
index 9d46398f8154..d0b8ce3f70ec 100644
--- a/drivers/net/can/sja1000/sja1000.h
+++ b/drivers/net/can/sja1000/sja1000.h
@@ -145,7 +145,9 @@
 /*
  * Flags for sja1000priv.flags
  */
-#define SJA1000_CUSTOM_IRQ_HANDLER 0x1
+#define SJA1000_CUSTOM_IRQ_HANDLER	BIT(0)
+#define SJA1000_NO_CDR_REG_QUIRK	BIT(1)
+#define SJA1000_NO_HW_LOOPBACK_QUIRK	BIT(2)
 
 /*
  * SJA1000 private data structure
-- 
2.25.1

[PATCH 4/6] can: sja1000: Use of_device_get_match_data to get device data

[Biju Das]

This patch replaces of_match_device->of_device_get_match_data
to get pointer to device data.

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

diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
index f9ec7bd8dfac..24ea0f76e130 100644
--- a/drivers/net/can/sja1000/sja1000_platform.c
+++ b/drivers/net/can/sja1000/sja1000_platform.c
@@ -210,7 +210,6 @@ static int sp_probe(struct platform_device *pdev)
 	struct resource *res_mem, *res_irq = NULL;
 	struct sja1000_platform_data *pdata;
 	struct device_node *of = pdev->dev.of_node;
-	const struct of_device_id *of_id;
 	const struct sja1000_of_data *of_data = NULL;
 	size_t priv_sz = 0;
 
@@ -243,11 +242,9 @@ static int sp_probe(struct platform_device *pdev)
 			return -ENODEV;
 	}
 
-	of_id = of_match_device(sp_of_table, &pdev->dev);
-	if (of_id && of_id->data) {
-		of_data = of_id->data;
+	of_data = of_device_get_match_data(&pdev->dev);
+	if (of_data)
 		priv_sz = of_data->priv_sz;
-	}
 
 	dev = alloc_sja1000dev(priv_sz);
 	if (!dev)
-- 
2.25.1

[PATCH 5/6] can: sja1000: Change the return type as void for SoC specific init

[Biju Das]

Change the return type as void for SoC specific init function as it
always return 0.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/can/sja1000/sja1000_platform.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
index 24ea0f76e130..5f3d362e0da5 100644
--- a/drivers/net/can/sja1000/sja1000_platform.c
+++ b/drivers/net/can/sja1000/sja1000_platform.c
@@ -31,7 +31,7 @@ MODULE_LICENSE("GPL v2");
 
 struct sja1000_of_data {
 	size_t  priv_sz;
-	int     (*init)(struct sja1000_priv *priv, struct device_node *of);
+	void    (*init)(struct sja1000_priv *priv, struct device_node *of);
 };
 
 struct technologic_priv {
@@ -94,15 +94,13 @@ static void sp_technologic_write_reg16(const struct sja1000_priv *priv,
 	spin_unlock_irqrestore(&tp->io_lock, flags);
 }
 
-static int sp_technologic_init(struct sja1000_priv *priv, struct device_node *of)
+static void sp_technologic_init(struct sja1000_priv *priv, struct device_node *of)
 {
 	struct technologic_priv *tp = priv->priv;
 
 	priv->read_reg = sp_technologic_read_reg16;
 	priv->write_reg = sp_technologic_write_reg16;
 	spin_lock_init(&tp->io_lock);
-
-	return 0;
 }
 
 static void sp_populate(struct sja1000_priv *priv,
@@ -266,11 +264,8 @@ static int sp_probe(struct platform_device *pdev)
 	if (of) {
 		sp_populate_of(priv, of);
 
-		if (of_data && of_data->init) {
-			err = of_data->init(priv, of);
-			if (err)
-				goto exit_free;
-		}
+		if (of_data && of_data->init)
+			of_data->init(priv, of);
 	} else {
 		sp_populate(priv, pdata, res_mem->flags);
 	}
-- 
2.25.1

[PATCH 6/6] can: sja1000: Add support for RZ/N1 SJA1000 CAN Controller

[Biju Das]

The SJA1000 CAN controller on RZ/N1 SoC has some differences compared
to others like it has no clock divider register (CDR) support and it has
no HW loopback(HW doesn't see tx messages on rx).

This patch adds support for RZ/N1 SJA1000 CAN Controller.

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

diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
index 5f3d362e0da5..8e63af76a013 100644
--- a/drivers/net/can/sja1000/sja1000_platform.c
+++ b/drivers/net/can/sja1000/sja1000_platform.c
@@ -14,6 +14,7 @@
 #include <linux/irq.h>
 #include <linux/can/dev.h>
 #include <linux/can/platform/sja1000.h>
+#include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -103,6 +104,11 @@ static void sp_technologic_init(struct sja1000_priv *priv, struct device_node *o
 	spin_lock_init(&tp->io_lock);
 }
 
+static void sp_rzn1_init(struct sja1000_priv *priv, struct device_node *of)
+{
+	priv->flags = SJA1000_NO_CDR_REG_QUIRK | SJA1000_NO_HW_LOOPBACK_QUIRK;
+}
+
 static void sp_populate(struct sja1000_priv *priv,
 			struct sja1000_platform_data *pdata,
 			unsigned long resource_mem_flags)
@@ -153,11 +159,13 @@ static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of)
 		priv->write_reg = sp_write_reg8;
 	}
 
-	err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
-	if (!err)
-		priv->can.clock.freq = prop / 2;
-	else
-		priv->can.clock.freq = SP_CAN_CLOCK; /* default */
+	if (!priv->can.clock.freq) {
+		err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
+		if (!err)
+			priv->can.clock.freq = prop / 2;
+		else
+			priv->can.clock.freq = SP_CAN_CLOCK; /* default */
+	}
 
 	err = of_property_read_u32(of, "nxp,tx-output-mode", &prop);
 	if (!err)
@@ -192,8 +200,13 @@ static struct sja1000_of_data technologic_data = {
 	.init = sp_technologic_init,
 };
 
+static struct sja1000_of_data renesas_data = {
+	.init = sp_rzn1_init,
+};
+
 static const struct of_device_id sp_of_table[] = {
 	{ .compatible = "nxp,sja1000", .data = NULL, },
+	{ .compatible = "renesas,rzn1-sja1000", .data = &renesas_data, },
 	{ .compatible = "technologic,sja1000", .data = &technologic_data, },
 	{ /* sentinel */ },
 };
@@ -210,6 +223,7 @@ static int sp_probe(struct platform_device *pdev)
 	struct device_node *of = pdev->dev.of_node;
 	const struct sja1000_of_data *of_data = NULL;
 	size_t priv_sz = 0;
+	struct clk *clk;
 
 	pdata = dev_get_platdata(&pdev->dev);
 	if (!pdata && !of) {
@@ -262,6 +276,16 @@ static int sp_probe(struct platform_device *pdev)
 	priv->reg_base = addr;
 
 	if (of) {
+		clk = devm_clk_get_optional(&pdev->dev, "can_clk");
+		if (IS_ERR(clk))
+			return dev_err_probe(&pdev->dev, PTR_ERR(clk), "no CAN clk");
+
+		if (clk) {
+			priv->can.clock.freq  = clk_get_rate(clk) / 2;
+			if (!priv->can.clock.freq)
+				return dev_err_probe(&pdev->dev, -EINVAL, "Zero CAN clk rate");
+		}
+
 		sp_populate_of(priv, of);
 
 		if (of_data && of_data->init)
-- 
2.25.1

Re: [PATCH 1/6] dt-bindings: can: sja1000: Convert to json-schema

[Marc Kleine-Budde]

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

On 02.07.2022 15:01:25, Biju Das wrote:
> Convert the NXP SJA1000 CAN Controller Device Tree binding
> documentation to json-schema.
> 
> Update the example to match reality.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  .../bindings/net/can/nxp,sja1000.yaml         | 106 ++++++++++++++++++
>  .../devicetree/bindings/net/can/sja1000.txt   |  58 ----------
>  2 files changed, 106 insertions(+), 58 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
>  delete mode 100644 Documentation/devicetree/bindings/net/can/sja1000.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> new file mode 100644
> index 000000000000..91d0f1b25d10
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> @@ -0,0 +1,106 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/can/nxp,sja1000.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
> +
> +maintainers:
> +  - Wolfgang Grandegger <wg@grandegger.com>

Please add:

allOf:
  - $ref: can-controller.yaml#

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 0/6] Add support for RZ/N1 SJA1000 CAN controller

[Marc Kleine-Budde]

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

On 02.07.2022 15:01:24, Biju Das wrote:
> This patch series aims to add support for RZ/N1 SJA1000 CAN controller.
> 
> The SJA1000 CAN controller on RZ/N1 SoC has some differences compared
> to others like it has no clock divider register (CDR) support and it has
> no HW loopback(HW doesn't see tx messages on rx), so introduced a new
               ^^^

please add a space here.

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 2/6] dt-bindings: can: nxp,sja1000: Document RZ/N1{D,S} support

[Marc Kleine-Budde]

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

On 02.07.2022 15:01:26, Biju Das wrote:
> Add CAN binding documentation for Renesas RZ/N1 SoC.
> 
> The SJA1000 CAN controller on RZ/N1 SoC has some differences compared
> to others like it has no clock divider register (CDR) support and it has
> no HW loopback(HW doesn't see tx messages on rx), so introduced a new
               ^^^

please add space.

> compatible 'renesas,rzn1-sja1000' to handle these differences.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  .../bindings/net/can/nxp,sja1000.yaml         | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> index 91d0f1b25d10..d0d374b979ec 100644
> --- a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> +++ b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> @@ -16,6 +16,12 @@ properties:
>          const: nxp,sja1000
>        - description: Technologic Systems SJA1000 CAN Controller
>          const: technologic,sja1000
> +      - description: Renesas RZ/N1 SJA1000 CAN Controller
> +        items:
> +          - enum:
> +              - renesas,r9a06g032-sja1000 # RZ/N1D
> +              - renesas,r9a06g033-sja1000 # RZ/N1S
> +          - const: renesas,rzn1-sja1000 # RZ/N1
>  
>    reg:
>      maxItems: 1
> @@ -23,6 +29,12 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: can_clk
> +
>    reg-io-width:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description: I/O register width (in bytes) implemented by this device
> @@ -91,6 +103,16 @@ allOf:
>        required:
>          - reg-io-width
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: renesas,rzn1-sja1000
> +    then:
> +      required:
> +        - clocks
> +        - clock-names
> +
>  unevaluatedProperties: false
>  
>  examples:

Can you add an example, too?

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 3/6] can: sja1000: Add Quirks for RZ/N1 SJA1000 CAN controller

[Marc Kleine-Budde]

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

On 02.07.2022 15:01:27, Biju Das wrote:
> Chapter 6.5.16 of the RZ/N1 Peripheral Manual mentions the below
> differences compared to the reference Philips SJA1000 device.
> 
> Handling of Transmitted Messages:
>  * The CAN controller does not copy transmitted messages to the receive
>    buffer, unlike the reference device.
> 
> Clock Divider Register:
>  * This register is not supported
> 
> This patch adds device quirks to handle these differences.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/net/can/sja1000/sja1000.c | 17 +++++++++++------
>  drivers/net/can/sja1000/sja1000.h |  4 +++-
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index 2e7638f98cf1..49cf4fc4d896 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -183,8 +183,9 @@ static void chipset_init(struct net_device *dev)
>  {
>  	struct sja1000_priv *priv = netdev_priv(dev);
>  
> -	/* set clock divider and output control register */
> -	priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN);
> +	if (!(priv->flags & SJA1000_NO_CDR_REG_QUIRK))
> +		/* set clock divider and output control register */
> +		priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN);
>  
>  	/* set acceptance filter (accept all) */
>  	priv->write_reg(priv, SJA1000_ACCC0, 0x00);
> @@ -208,9 +209,11 @@ static void sja1000_start(struct net_device *dev)
>  	if (priv->can.state != CAN_STATE_STOPPED)
>  		set_reset_mode(dev);
>  
> -	/* Initialize chip if uninitialized at this stage */
> -	if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN))
> -		chipset_init(dev);
> +	if (!(priv->flags & SJA1000_NO_CDR_REG_QUIRK)) {
> +		/* Initialize chip if uninitialized at this stage */
> +		if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN))
> +			chipset_init(dev);
> +	}
>  
>  	/* Clear error counters and error code capture */
>  	priv->write_reg(priv, SJA1000_TXERR, 0x0);
> @@ -652,12 +655,14 @@ static const struct net_device_ops sja1000_netdev_ops = {
>  
>  int register_sja1000dev(struct net_device *dev)
>  {
> +	struct sja1000_priv *priv = netdev_priv(dev);
>  	int ret;
>  
>  	if (!sja1000_probe_chip(dev))
>  		return -ENODEV;
>  
> -	dev->flags |= IFF_ECHO;	/* we support local echo */
> +	if (!(priv->flags & SJA1000_NO_HW_LOOPBACK_QUIRK))
> +		dev->flags |= IFF_ECHO;	/* we support local echo */
>  	dev->netdev_ops = &sja1000_netdev_ops;
>  
>  	set_reset_mode(dev);
> diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
> index 9d46398f8154..d0b8ce3f70ec 100644
> --- a/drivers/net/can/sja1000/sja1000.h
> +++ b/drivers/net/can/sja1000/sja1000.h
> @@ -145,7 +145,9 @@
>  /*
>   * Flags for sja1000priv.flags
>   */
> -#define SJA1000_CUSTOM_IRQ_HANDLER 0x1
> +#define SJA1000_CUSTOM_IRQ_HANDLER	BIT(0)
> +#define SJA1000_NO_CDR_REG_QUIRK	BIT(1)
> +#define SJA1000_NO_HW_LOOPBACK_QUIRK	BIT(2)

Please name these defines SJA1000_QUIRK_*

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 4/6] can: sja1000: Use of_device_get_match_data to get device data

[Marc Kleine-Budde]

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

On 02.07.2022 15:01:28, Biju Das wrote:
> This patch replaces of_match_device->of_device_get_match_data
> to get pointer to device data.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/net/can/sja1000/sja1000_platform.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
> index f9ec7bd8dfac..24ea0f76e130 100644
> --- a/drivers/net/can/sja1000/sja1000_platform.c
> +++ b/drivers/net/can/sja1000/sja1000_platform.c
> @@ -210,7 +210,6 @@ static int sp_probe(struct platform_device *pdev)
>  	struct resource *res_mem, *res_irq = NULL;
>  	struct sja1000_platform_data *pdata;
>  	struct device_node *of = pdev->dev.of_node;
> -	const struct of_device_id *of_id;
>  	const struct sja1000_of_data *of_data = NULL;
>  	size_t priv_sz = 0;
>  
> @@ -243,11 +242,9 @@ static int sp_probe(struct platform_device *pdev)
>  			return -ENODEV;
>  	}
>  
> -	of_id = of_match_device(sp_of_table, &pdev->dev);
> -	if (of_id && of_id->data) {
> -		of_data = of_id->data;
> +	of_data = of_device_get_match_data(&pdev->dev);

Can you use device_get_match_data() instead?

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 6/6] can: sja1000: Add support for RZ/N1 SJA1000 CAN Controller

[Marc Kleine-Budde]

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

On 02.07.2022 15:01:30, Biju Das wrote:
> The SJA1000 CAN controller on RZ/N1 SoC has some differences compared
> to others like it has no clock divider register (CDR) support and it has
> no HW loopback(HW doesn't see tx messages on rx).
> 
> This patch adds support for RZ/N1 SJA1000 CAN Controller.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/net/can/sja1000/sja1000_platform.c | 34 ++++++++++++++++++----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
> index 5f3d362e0da5..8e63af76a013 100644
> --- a/drivers/net/can/sja1000/sja1000_platform.c
> +++ b/drivers/net/can/sja1000/sja1000_platform.c
> @@ -14,6 +14,7 @@
>  #include <linux/irq.h>
>  #include <linux/can/dev.h>
>  #include <linux/can/platform/sja1000.h>
> +#include <linux/clk.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -103,6 +104,11 @@ static void sp_technologic_init(struct sja1000_priv *priv, struct device_node *o
>  	spin_lock_init(&tp->io_lock);
>  }
>  
> +static void sp_rzn1_init(struct sja1000_priv *priv, struct device_node *of)
> +{
> +	priv->flags = SJA1000_NO_CDR_REG_QUIRK | SJA1000_NO_HW_LOOPBACK_QUIRK;
> +}
> +
>  static void sp_populate(struct sja1000_priv *priv,
>  			struct sja1000_platform_data *pdata,
>  			unsigned long resource_mem_flags)
> @@ -153,11 +159,13 @@ static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of)
>  		priv->write_reg = sp_write_reg8;
>  	}
>  
> -	err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
> -	if (!err)
> -		priv->can.clock.freq = prop / 2;
> -	else
> -		priv->can.clock.freq = SP_CAN_CLOCK; /* default */
> +	if (!priv->can.clock.freq) {
> +		err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
> +		if (!err)
> +			priv->can.clock.freq = prop / 2;
> +		else
> +			priv->can.clock.freq = SP_CAN_CLOCK; /* default */
> +	}
>  
>  	err = of_property_read_u32(of, "nxp,tx-output-mode", &prop);
>  	if (!err)
> @@ -192,8 +200,13 @@ static struct sja1000_of_data technologic_data = {
>  	.init = sp_technologic_init,
>  };
>  
> +static struct sja1000_of_data renesas_data = {
> +	.init = sp_rzn1_init,
> +};
> +
>  static const struct of_device_id sp_of_table[] = {
>  	{ .compatible = "nxp,sja1000", .data = NULL, },
> +	{ .compatible = "renesas,rzn1-sja1000", .data = &renesas_data, },
>  	{ .compatible = "technologic,sja1000", .data = &technologic_data, },
>  	{ /* sentinel */ },
>  };
> @@ -210,6 +223,7 @@ static int sp_probe(struct platform_device *pdev)
>  	struct device_node *of = pdev->dev.of_node;
>  	const struct sja1000_of_data *of_data = NULL;
>  	size_t priv_sz = 0;
> +	struct clk *clk;
>  
>  	pdata = dev_get_platdata(&pdev->dev);
>  	if (!pdata && !of) {
> @@ -262,6 +276,16 @@ static int sp_probe(struct platform_device *pdev)
>  	priv->reg_base = addr;
>  
>  	if (of) {
> +		clk = devm_clk_get_optional(&pdev->dev, "can_clk");
> +		if (IS_ERR(clk))
> +			return dev_err_probe(&pdev->dev, PTR_ERR(clk), "no CAN clk");

Please take care of releasing all acquired resources.

> +
> +		if (clk) {
> +			priv->can.clock.freq  = clk_get_rate(clk) / 2;
> +			if (!priv->can.clock.freq)
> +				return dev_err_probe(&pdev->dev, -EINVAL, "Zero CAN clk rate");

same here.

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 3/6] can: sja1000: Add Quirks for RZ/N1 SJA1000 CAN controller

[Marc Kleine-Budde]

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

On 02.07.2022 15:01:27, Biju Das wrote:
> Chapter 6.5.16 of the RZ/N1 Peripheral Manual mentions the below
> differences compared to the reference Philips SJA1000 device.
> 
> Handling of Transmitted Messages:
>  * The CAN controller does not copy transmitted messages to the receive
>    buffer, unlike the reference device.

This is something different than....

>  int register_sja1000dev(struct net_device *dev)
>  {
> +	struct sja1000_priv *priv = netdev_priv(dev);
>  	int ret;
>  
>  	if (!sja1000_probe_chip(dev))
>  		return -ENODEV;
>  
> -	dev->flags |= IFF_ECHO;	/* we support local echo */
> +	if (!(priv->flags & SJA1000_NO_HW_LOOPBACK_QUIRK))
> +		dev->flags |= IFF_ECHO;	/* we support local echo */

... the IFF_ECHO.

IFF_ECHO set means the driver cals can_put_echo_skb() before TX and
can_get_echo_skb() after TX complete interrupt.

| irqreturn_t sja1000_interrupt(int irq, void *dev_id)
[...]
| 	while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&
| 	       (n < SJA1000_MAX_IRQ)) {
| 
| 		status = priv->read_reg(priv, SJA1000_SR);
| 		/* check for absent controller due to hw unplug */
| 		if (status == 0xFF && sja1000_is_absent(priv))
| 			goto out;
| 
| 		if (isrc & IRQ_WUI)
| 			netdev_warn(dev, "wakeup interrupt\n");
| 
| 		if (isrc & IRQ_TI) {
| 			/* transmission buffer released */
| 			if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
| 			    !(status & SR_TCS)) {
| 				stats->tx_errors++;
| 				can_free_echo_skb(dev, 0, NULL);
| 			} else {

Please add a netdev_info() for debugging and verify that you get a TX
complete IRQ.

| 				/* transmission complete */
| 				stats->tx_bytes += can_get_echo_skb(dev, 0, NULL);
| 				stats->tx_packets++;
| 			}
| 			netif_wake_queue(dev);
| 		}


If your hardware doesn't support hardware loopback (configured via
CMD_SRR):

| 	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
| 		cmd_reg_val |= CMD_SRR;
| 	else
| 		cmd_reg_val |= CMD_TR;

then don't set CAN_CTRLMODE_LOOPBACK in priv->can.ctrlmode_supported.

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 6/6] can: sja1000: Add support for RZ/N1 SJA1000 CAN Controller

[Marc Kleine-Budde]

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

On 02.07.2022 15:01:30, Biju Das wrote:
> The SJA1000 CAN controller on RZ/N1 SoC has some differences compared
> to others like it has no clock divider register (CDR) support and it has
> no HW loopback(HW doesn't see tx messages on rx).
> 
> This patch adds support for RZ/N1 SJA1000 CAN Controller.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/net/can/sja1000/sja1000_platform.c | 34 ++++++++++++++++++----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
> index 5f3d362e0da5..8e63af76a013 100644
> --- a/drivers/net/can/sja1000/sja1000_platform.c
> +++ b/drivers/net/can/sja1000/sja1000_platform.c
[...]
> @@ -262,6 +276,16 @@ static int sp_probe(struct platform_device *pdev)
>  	priv->reg_base = addr;
>  
>  	if (of) {
> +		clk = devm_clk_get_optional(&pdev->dev, "can_clk");
> +		if (IS_ERR(clk))
> +			return dev_err_probe(&pdev->dev, PTR_ERR(clk), "no CAN clk");
> +
> +		if (clk) {
> +			priv->can.clock.freq  = clk_get_rate(clk) / 2;
> +			if (!priv->can.clock.freq)
> +				return dev_err_probe(&pdev->dev, -EINVAL, "Zero CAN clk rate");
> +		}

There's no clk_prepare_enable in the driver. You might go the quick and
dirty way an enable the clock right here. IIRC there's a new convenience
function to get and enable a clock, managed bei devm. Uwe (Cc'ed) can
point you in the right direction.

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 3/6] can: sja1000: Add Quirks for RZ/N1 SJA1000 CAN controller

[Biju Das]

Hi Marc,

Thanks for the feedback.

> Subject: Re: [PATCH 3/6] can: sja1000: Add Quirks for RZ/N1 SJA1000 CAN
> controller
> 
> On 02.07.2022 15:01:27, Biju Das wrote:
> > Chapter 6.5.16 of the RZ/N1 Peripheral Manual mentions the below
> > differences compared to the reference Philips SJA1000 device.
> >
> > Handling of Transmitted Messages:
> >  * The CAN controller does not copy transmitted messages to the
> receive
> >    buffer, unlike the reference device.
> 
> This is something different than....
> 
> >  int register_sja1000dev(struct net_device *dev)  {
> > +	struct sja1000_priv *priv = netdev_priv(dev);
> >  	int ret;
> >
> >  	if (!sja1000_probe_chip(dev))
> >  		return -ENODEV;
> >
> > -	dev->flags |= IFF_ECHO;	/* we support local echo */
> > +	if (!(priv->flags & SJA1000_NO_HW_LOOPBACK_QUIRK))
> > +		dev->flags |= IFF_ECHO;	/* we support local echo */
> 
> ... the IFF_ECHO.
> 
> IFF_ECHO set means the driver cals can_put_echo_skb() before TX and
> can_get_echo_skb() after TX complete interrupt.

OK.

> 
> | irqreturn_t sja1000_interrupt(int irq, void *dev_id)
> [...]
> | 	while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&
> | 	       (n < SJA1000_MAX_IRQ)) {
> |
> | 		status = priv->read_reg(priv, SJA1000_SR);
> | 		/* check for absent controller due to hw unplug */
> | 		if (status == 0xFF && sja1000_is_absent(priv))
> | 			goto out;
> |
> | 		if (isrc & IRQ_WUI)
> | 			netdev_warn(dev, "wakeup interrupt\n");
> |
> | 		if (isrc & IRQ_TI) {
> | 			/* transmission buffer released */
> | 			if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
> | 			    !(status & SR_TCS)) {
> | 				stats->tx_errors++;
> | 				can_free_echo_skb(dev, 0, NULL);
> | 			} else {
> 
> Please add a netdev_info() for debugging and verify that you get a TX
> complete IRQ.

I have put prints and I confirm I get Tx complete IRQ.

--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -526,6 +526,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
                                stats->tx_errors++;
                                can_free_echo_skb(dev, 0, NULL);
                        } else {
+                               netdev_info(dev,"#### Completed message in IRQ");
                                /* transmission complete */

root@rzn1d400-db:/rzn1_tests/linux# ./test-can.sh
Testing sending from can0 to can1 at 100000 bps
[   30.032757] sja1000_platform 52105000.can can1: setting BTR0=0x0e BTR1=0x1c
[   30.062123] IPv6: ADDRCONF(NETDEV_CHANGE): can1: link becomes ready
[   30.128729] sja1000_platform 52104000.can can0: setting BTR0=0x0e BTR1=0x1c
[   31.130232] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
[   32.237998] sja1000_platform 52104000.can can0: #### Completed message in IRQ
[   32.246774] sja1000_platform 52104000.can can0: #### Completed message in IRQ
....
.....
[   33.210809] sja1000_platform 52104000.can can0: #### Completed message in IRQ
[   33.221272] sja1000_platform 52104000.can can0: #### Completed message in IRQ
Testing sending from can0 to can1 at 500000 bps
[   33.366862] sja1000_platform 52105000.can can1: setting BTR0=0x02 BTR1=0x1c
[   33.455829] sja1000_platform 52104000.can can0: setting BTR0=0x02 BTR1=0x1c
[   35.546544] sja1000_platform 52104000.can can0: #### Completed message in IRQ
[   35.554487] sja1000_platform 52104000.can can0: #### Completed message in IRQ
[   35.562322] sja1000_platform 52104000.can can0: #### Completed message in IRQ
.....
[   36.520433] sja1000_platform 52104000.can can0: #### Completed message in IRQ
[   36.530376] sja1000_platform 52104000.can can0: #### Completed message in IRQ
Testing sending from can0 to can1 at 1000000 bps
[   36.667887] sja1000_platform 52105000.can can1: setting BTR0=0x01 BTR1=0x27
[   36.759561] sja1000_platform 52104000.can can0: setting BTR0=0x01 BTR1=0x27
[   38.849459] sja1000_platform 52104000.can can0: #### Completed message in IRQ
[   38.857118] sja1000_platform 52104000.can can0: #### Completed message in IRQ
[   38.864611] sja1000_platform 52104000.can can0: #### Completed message in IRQ
....
[   39.830284] sja1000_platform 52104000.can can0: #### Completed message in IRQ
root@rzn1d400-db:/rzn1_tests/linux# cat /proc/interrupts | grep can
 35:        300          0 GIC-0 127 Level     can0
 36:        300          0 GIC-0 128 Level     can1
root@rzn1d400-db:/rzn1_tests/linux# ./test-can.sh can1 can0
Testing sending from can1 to can0 at 100000 bps
[  111.972815] sja1000_platform 52104000.can can0: setting BTR0=0x0e BTR1=0x1c
[  112.064622] sja1000_platform 52105000.can can1: setting BTR0=0x0e BTR1=0x1c
[  114.159798] sja1000_platform 52105000.can can1: #### Completed message in IRQ
[  114.168361] sja1000_platform 52105000.can can1: #### Completed message in IRQ
.....
[  115.141437] sja1000_platform 52105000.can can1: #### Completed message in IRQ
Testing sending from can1 to can0 at 500000 bps
[  115.272323] sja1000_platform 52104000.can can0: setting BTR0=0x02 BTR1=0x1c
[  115.365008] sja1000_platform 52105000.can can1: setting BTR0=0x02 BTR1=0x1c
[  117.445187] sja1000_platform 52105000.can can1: #### Completed message in IRQ
[  117.460489] sja1000_platform 52105000.can can1: #### Completed message in IRQ
[  117.470574] sja1000_platform 52105000.can can1: #### Completed message in IRQ
....
[  118.430442] sja1000_platform 52105000.can can1: #### Completed message in IRQ
[  118.440381] sja1000_platform 52105000.can can1: #### Completed message in IRQ
Testing sending from can1 to can0 at 1000000 bps
[  118.564981] sja1000_platform 52104000.can can0: setting BTR0=0x01 BTR1=0x27
[  118.655100] sja1000_platform 52105000.can can1: setting BTR0=0x01 BTR1=0x27
[  120.731882] sja1000_platform 52105000.can can1: #### Completed message in IRQ
[  120.740605] sja1000_platform 52105000.can can1: #### Completed message in IRQ
...
[  121.710274] sja1000_platform 52105000.can can1: #### Completed message in IRQ
[  121.720370] sja1000_platform 52105000.can can1: #### Completed message in IRQ
root@rzn1d400-db:/rzn1_tests/linux#root@rzn1d400-db:/rzn1_tests/linux# cat /proc/interrupts | grep can
 35:        600          0 GIC-0 127 Level     can0
 36:        600          0 GIC-0 128 Level     can1


> 
> | 				/* transmission complete */
> | 				stats->tx_bytes += can_get_echo_skb(dev, 0,
> NULL);
> | 				stats->tx_packets++;
> | 			}
> | 			netif_wake_queue(dev);
> | 		}
> 
> 
> If your hardware doesn't support hardware loopback (configured via
> CMD_SRR):

But our HW supports, CMD_SRR.

b4 bCan_SRR Self Reception Request
1: Set when a message is to be transmitted and received simultaneously
[Condition of “Cleared to 0”]
● Software Reset (Set “1” in bCan_RM)
● Switch to “Bus Off” (Set “1” in bCan_BS)

Cheers,
Biju

> 
> | 	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> | 		cmd_reg_val |= CMD_SRR;
> | 	else
> | 		cmd_reg_val |= CMD_TR;
> 
> then don't set CAN_CTRLMODE_LOOPBACK in priv->can.ctrlmode_supported.

RE: [PATCH 1/6] dt-bindings: can: sja1000: Convert to json-schema

[Biju Das]

Hi Marc,

Thanks for the feedback.

> Subject: Re: [PATCH 1/6] dt-bindings: can: sja1000: Convert to json-
> schema
> 
> On 02.07.2022 15:01:25, Biju Das wrote:
> > Convert the NXP SJA1000 CAN Controller Device Tree binding
> > documentation to json-schema.
> >
> > Update the example to match reality.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  .../bindings/net/can/nxp,sja1000.yaml         | 106
> ++++++++++++++++++
> >  .../devicetree/bindings/net/can/sja1000.txt   |  58 ----------
> >  2 files changed, 106 insertions(+), 58 deletions(-)  create mode
> > 100644 Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> >  delete mode 100644
> > Documentation/devicetree/bindings/net/can/sja1000.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> > b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> > new file mode 100644
> > index 000000000000..91d0f1b25d10
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> > @@ -0,0 +1,106 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/can/nxp,sja1000.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Memory mapped SJA1000 CAN controller from NXP (formerly
> > +Philips)
> > +
> > +maintainers:
> > +  - Wolfgang Grandegger <wg@grandegger.com>
> 
> Please add:
> 
> allOf:
>   - $ref: can-controller.yaml#

It is already there at bottom. Will move up as you suggested.

Cheers,
Biju

RE: [PATCH 2/6] dt-bindings: can: nxp,sja1000: Document RZ/N1{D,S} support

[Biju Das]

Hi Marc,

Thanks for the feedback.

> Subject: Re: [PATCH 2/6] dt-bindings: can: nxp,sja1000: Document
> RZ/N1{D,S} support
> 
> On 02.07.2022 15:01:26, Biju Das wrote:
> > Add CAN binding documentation for Renesas RZ/N1 SoC.
> >
> > The SJA1000 CAN controller on RZ/N1 SoC has some differences compared
> > to others like it has no clock divider register (CDR) support and it
> > has no HW loopback(HW doesn't see tx messages on rx), so introduced a
> > new
>                ^^^
> 
> please add space.

OK.

> 
> > compatible 'renesas,rzn1-sja1000' to handle these differences.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  .../bindings/net/can/nxp,sja1000.yaml         | 22
> +++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> > b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> > index 91d0f1b25d10..d0d374b979ec 100644
> > --- a/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> > +++ b/Documentation/devicetree/bindings/net/can/nxp,sja1000.yaml
> > @@ -16,6 +16,12 @@ properties:
> >          const: nxp,sja1000
> >        - description: Technologic Systems SJA1000 CAN Controller
> >          const: technologic,sja1000
> > +      - description: Renesas RZ/N1 SJA1000 CAN Controller
> > +        items:
> > +          - enum:
> > +              - renesas,r9a06g032-sja1000 # RZ/N1D
> > +              - renesas,r9a06g033-sja1000 # RZ/N1S
> > +          - const: renesas,rzn1-sja1000 # RZ/N1
> >
> >    reg:
> >      maxItems: 1
> > @@ -23,6 +29,12 @@ properties:
> >    interrupts:
> >      maxItems: 1
> >
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    const: can_clk
> > +
> >    reg-io-width:
> >      $ref: /schemas/types.yaml#/definitions/uint32
> >      description: I/O register width (in bytes) implemented by this
> > device @@ -91,6 +103,16 @@ allOf:
> >        required:
> >          - reg-io-width
> >
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: renesas,rzn1-sja1000
> > +    then:
> > +      required:
> > +        - clocks
> > +        - clock-names
> > +
> >  unevaluatedProperties: false
> >
> >  examples:
> 
> Can you add an example, too?


OK, Will add in V2.

Cheers,
Biju

RE: [PATCH 3/6] can: sja1000: Add Quirks for RZ/N1 SJA1000 CAN controller

[Biju Das]

Hi Marc,

Thanks for the feedback.

> Subject: Re: [PATCH 3/6] can: sja1000: Add Quirks for RZ/N1 SJA1000 CAN
> controller
> 
> On 02.07.2022 15:01:27, Biju Das wrote:
> > Chapter 6.5.16 of the RZ/N1 Peripheral Manual mentions the below
> > differences compared to the reference Philips SJA1000 device.
> >
> > Handling of Transmitted Messages:
> >  * The CAN controller does not copy transmitted messages to the
> receive
> >    buffer, unlike the reference device.
> >
> > Clock Divider Register:
> >  * This register is not supported
> >
> > This patch adds device quirks to handle these differences.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/net/can/sja1000/sja1000.c | 17 +++++++++++------
> > drivers/net/can/sja1000/sja1000.h |  4 +++-
> >  2 files changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/can/sja1000/sja1000.c
> > b/drivers/net/can/sja1000/sja1000.c
> > index 2e7638f98cf1..49cf4fc4d896 100644
> > --- a/drivers/net/can/sja1000/sja1000.c
> > +++ b/drivers/net/can/sja1000/sja1000.c
> > @@ -183,8 +183,9 @@ static void chipset_init(struct net_device *dev)
> > {
> >  	struct sja1000_priv *priv = netdev_priv(dev);
> >
> > -	/* set clock divider and output control register */
> > -	priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN);
> > +	if (!(priv->flags & SJA1000_NO_CDR_REG_QUIRK))
> > +		/* set clock divider and output control register */
> > +		priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN);
> >
> >  	/* set acceptance filter (accept all) */
> >  	priv->write_reg(priv, SJA1000_ACCC0, 0x00); @@ -208,9 +209,11 @@
> > static void sja1000_start(struct net_device *dev)
> >  	if (priv->can.state != CAN_STATE_STOPPED)
> >  		set_reset_mode(dev);
> >
> > -	/* Initialize chip if uninitialized at this stage */
> > -	if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN))
> > -		chipset_init(dev);
> > +	if (!(priv->flags & SJA1000_NO_CDR_REG_QUIRK)) {
> > +		/* Initialize chip if uninitialized at this stage */
> > +		if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN))
> > +			chipset_init(dev);
> > +	}
> >
> >  	/* Clear error counters and error code capture */
> >  	priv->write_reg(priv, SJA1000_TXERR, 0x0); @@ -652,12 +655,14 @@
> > static const struct net_device_ops sja1000_netdev_ops = {
> >
> >  int register_sja1000dev(struct net_device *dev)  {
> > +	struct sja1000_priv *priv = netdev_priv(dev);
> >  	int ret;
> >
> >  	if (!sja1000_probe_chip(dev))
> >  		return -ENODEV;
> >
> > -	dev->flags |= IFF_ECHO;	/* we support local echo */
> > +	if (!(priv->flags & SJA1000_NO_HW_LOOPBACK_QUIRK))
> > +		dev->flags |= IFF_ECHO;	/* we support local echo */
> >  	dev->netdev_ops = &sja1000_netdev_ops;
> >
> >  	set_reset_mode(dev);
> > diff --git a/drivers/net/can/sja1000/sja1000.h
> > b/drivers/net/can/sja1000/sja1000.h
> > index 9d46398f8154..d0b8ce3f70ec 100644
> > --- a/drivers/net/can/sja1000/sja1000.h
> > +++ b/drivers/net/can/sja1000/sja1000.h
> > @@ -145,7 +145,9 @@
> >  /*
> >   * Flags for sja1000priv.flags
> >   */
> > -#define SJA1000_CUSTOM_IRQ_HANDLER 0x1
> > +#define SJA1000_CUSTOM_IRQ_HANDLER	BIT(0)
> > +#define SJA1000_NO_CDR_REG_QUIRK	BIT(1)
> > +#define SJA1000_NO_HW_LOOPBACK_QUIRK	BIT(2)
> 
> Please name these defines SJA1000_QUIRK_*

Agreed, Will fix this in V2.

Cheers,
Biju

RE: [PATCH 4/6] can: sja1000: Use of_device_get_match_data to get device data

[Biju Das]

Hi Marc,

Thanks for the feedback.


> Subject: Re: [PATCH 4/6] can: sja1000: Use of_device_get_match_data to
> get device data
> 
> On 02.07.2022 15:01:28, Biju Das wrote:
> > This patch replaces of_match_device->of_device_get_match_data
> > to get pointer to device data.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/net/can/sja1000/sja1000_platform.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/can/sja1000/sja1000_platform.c
> b/drivers/net/can/sja1000/sja1000_platform.c
> > index f9ec7bd8dfac..24ea0f76e130 100644
> > --- a/drivers/net/can/sja1000/sja1000_platform.c
> > +++ b/drivers/net/can/sja1000/sja1000_platform.c
> > @@ -210,7 +210,6 @@ static int sp_probe(struct platform_device *pdev)
> >  	struct resource *res_mem, *res_irq = NULL;
> >  	struct sja1000_platform_data *pdata;
> >  	struct device_node *of = pdev->dev.of_node;
> > -	const struct of_device_id *of_id;
> >  	const struct sja1000_of_data *of_data = NULL;
> >  	size_t priv_sz = 0;
> >
> > @@ -243,11 +242,9 @@ static int sp_probe(struct platform_device *pdev)
> >  			return -ENODEV;
> >  	}
> >
> > -	of_id = of_match_device(sp_of_table, &pdev->dev);
> > -	if (of_id && of_id->data) {
> > -		of_data = of_id->data;
> > +	of_data = of_device_get_match_data(&pdev->dev);
> 
> Can you use device_get_match_data() instead?

Agreed, Will use device_get_match_data()

Cheers,
Biju

RE: [PATCH 6/6] can: sja1000: Add support for RZ/N1 SJA1000 CAN Controller

[Biju Das]

Hi Marc and Uwe,

> Subject: Re: [PATCH 6/6] can: sja1000: Add support for RZ/N1 SJA1000 CAN
> Controller
> 
> On 02.07.2022 15:01:30, Biju Das wrote:
> > The SJA1000 CAN controller on RZ/N1 SoC has some differences compared
> > to others like it has no clock divider register (CDR) support and it
> > has no HW loopback(HW doesn't see tx messages on rx).
> >
> > This patch adds support for RZ/N1 SJA1000 CAN Controller.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/net/can/sja1000/sja1000_platform.c | 34
> > ++++++++++++++++++----
> >  1 file changed, 29 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/can/sja1000/sja1000_platform.c
> > b/drivers/net/can/sja1000/sja1000_platform.c
> > index 5f3d362e0da5..8e63af76a013 100644
> > --- a/drivers/net/can/sja1000/sja1000_platform.c
> > +++ b/drivers/net/can/sja1000/sja1000_platform.c
> [...]
> > @@ -262,6 +276,16 @@ static int sp_probe(struct platform_device *pdev)
> >  	priv->reg_base = addr;
> >
> >  	if (of) {
> > +		clk = devm_clk_get_optional(&pdev->dev, "can_clk");
> > +		if (IS_ERR(clk))
> > +			return dev_err_probe(&pdev->dev, PTR_ERR(clk), "no CAN
> clk");
> > +
> > +		if (clk) {
> > +			priv->can.clock.freq  = clk_get_rate(clk) / 2;
> > +			if (!priv->can.clock.freq)
> > +				return dev_err_probe(&pdev->dev, -EINVAL, "Zero
> CAN clk rate");
> > +		}
> 
> There's no clk_prepare_enable in the driver. You might go the quick and
> dirty way an enable the clock right here. IIRC there's a new convenience
> function to get and enable a clock, managed bei devm. Uwe (Cc'ed) can
> point you in the right direction.

 + clk

As per the patch history devm version for clk_prepare_enable is rejected[1], so the individual drivers implemented the same using devm_add_action_or_reset [2].
So shall I implement devm version here as well?

[1]https://lkml.iu.edu/hypermail/linux/kernel/2103.1/01556.html

[2] https://elixir.bootlin.com/linux/v5.19-rc4/source/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c#L266

Cheers,
Biju

Re: [PATCH 6/6] can: sja1000: Add support for RZ/N1 SJA1000 CAN Controller

[Uwe Kleine-König]

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

On Sun, Jul 03, 2022 at 07:15:16AM +0000, Biju Das wrote:
> Hi Marc and Uwe,
> 
> > Subject: Re: [PATCH 6/6] can: sja1000: Add support for RZ/N1 SJA1000 CAN
> > Controller
> > 
> > On 02.07.2022 15:01:30, Biju Das wrote:
> > > The SJA1000 CAN controller on RZ/N1 SoC has some differences compared
> > > to others like it has no clock divider register (CDR) support and it
> > > has no HW loopback(HW doesn't see tx messages on rx).
> > >
> > > This patch adds support for RZ/N1 SJA1000 CAN Controller.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > >  drivers/net/can/sja1000/sja1000_platform.c | 34
> > > ++++++++++++++++++----
> > >  1 file changed, 29 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/net/can/sja1000/sja1000_platform.c
> > > b/drivers/net/can/sja1000/sja1000_platform.c
> > > index 5f3d362e0da5..8e63af76a013 100644
> > > --- a/drivers/net/can/sja1000/sja1000_platform.c
> > > +++ b/drivers/net/can/sja1000/sja1000_platform.c
> > [...]
> > > @@ -262,6 +276,16 @@ static int sp_probe(struct platform_device *pdev)
> > >  	priv->reg_base = addr;
> > >
> > >  	if (of) {
> > > +		clk = devm_clk_get_optional(&pdev->dev, "can_clk");
> > > +		if (IS_ERR(clk))
> > > +			return dev_err_probe(&pdev->dev, PTR_ERR(clk), "no CAN
> > clk");
> > > +
> > > +		if (clk) {
> > > +			priv->can.clock.freq  = clk_get_rate(clk) / 2;
> > > +			if (!priv->can.clock.freq)
> > > +				return dev_err_probe(&pdev->dev, -EINVAL, "Zero
> > CAN clk rate");
> > > +		}
> > 
> > There's no clk_prepare_enable in the driver. You might go the quick and
> > dirty way an enable the clock right here. IIRC there's a new convenience
> > function to get and enable a clock, managed bei devm. Uwe (Cc'ed) can
> > point you in the right direction.
> 
>  + clk
> 
> As per the patch history devm version for clk_prepare_enable is rejected[1], so the individual drivers implemented the same using devm_add_action_or_reset [2].
> So shall I implement devm version here as well?

You want to make use of 7ef9651e9792b08eb310c6beb202cbc947f43cab (which
is currently in next). If you cherry-pick this to an older kernel
version, make sure to also pick
8b3d743fc9e2542822826890b482afabf0e7522a.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

RE: [PATCH 6/6] can: sja1000: Add support for RZ/N1 SJA1000 CAN Controller

[Biju Das]

Hi Uwe,

Thanks for the feedback.

> Subject: Re: [PATCH 6/6] can: sja1000: Add support for RZ/N1 SJA1000 CAN
> Controller
> 
> On Sun, Jul 03, 2022 at 07:15:16AM +0000, Biju Das wrote:
> > Hi Marc and Uwe,
> >
> > > Subject: Re: [PATCH 6/6] can: sja1000: Add support for RZ/N1 SJA1000
> > > CAN Controller
> > >
> > > On 02.07.2022 15:01:30, Biju Das wrote:
> > > > The SJA1000 CAN controller on RZ/N1 SoC has some differences
> > > > compared to others like it has no clock divider register (CDR)
> > > > support and it has no HW loopback(HW doesn't see tx messages on
> rx).
> > > >
> > > > This patch adds support for RZ/N1 SJA1000 CAN Controller.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > ---
> > > >  drivers/net/can/sja1000/sja1000_platform.c | 34
> > > > ++++++++++++++++++----
> > > >  1 file changed, 29 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/net/can/sja1000/sja1000_platform.c
> > > > b/drivers/net/can/sja1000/sja1000_platform.c
> > > > index 5f3d362e0da5..8e63af76a013 100644
> > > > --- a/drivers/net/can/sja1000/sja1000_platform.c
> > > > +++ b/drivers/net/can/sja1000/sja1000_platform.c
> > > [...]
> > > > @@ -262,6 +276,16 @@ static int sp_probe(struct platform_device
> *pdev)
> > > >  	priv->reg_base = addr;
> > > >
> > > >  	if (of) {
> > > > +		clk = devm_clk_get_optional(&pdev->dev, "can_clk");
> > > > +		if (IS_ERR(clk))
> > > > +			return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> "no CAN
> > > clk");
> > > > +
> > > > +		if (clk) {
> > > > +			priv->can.clock.freq  = clk_get_rate(clk) / 2;
> > > > +			if (!priv->can.clock.freq)
> > > > +				return dev_err_probe(&pdev->dev, -EINVAL,
> "Zero
> > > CAN clk rate");
> > > > +		}
> > >
> > > There's no clk_prepare_enable in the driver. You might go the quick
> > > and dirty way an enable the clock right here. IIRC there's a new
> > > convenience function to get and enable a clock, managed bei devm.
> > > Uwe (Cc'ed) can point you in the right direction.
> >
> >  + clk
> >
> > As per the patch history devm version for clk_prepare_enable is
> rejected[1], so the individual drivers implemented the same using
> devm_add_action_or_reset [2].
> > So shall I implement devm version here as well?
> 
> You want to make use of 7ef9651e9792b08eb310c6beb202cbc947f43cab (which
> is currently in next). If you cherry-pick this to an older kernel
> version, make sure to also pick
> 8b3d743fc9e2542822826890b482afabf0e7522a.

Ok will use "devm_clk_get_optional_enabled" and send  V2.

Cheers,
Biju