[PATCH 0/7] can: mcan: Add MCAN support for FSD SoC

[PATCH 0/7] can: mcan: Add MCAN support for FSD SoC

[Vivek Yadav]

Add support for MCAN instances present on the FSD platform.
Also add support for handling error correction code (ECC) for MCAN message RAM.

Sriranjani P (2):
  dt-bindings: Document the SYSREG specific compatibles found on FSD SoC
  arm64: dts: fsd: add sysreg device node

Vivek Yadav (5):
  dt-bindings: can: mcan: Add ECC functionality to message ram
  can: mcan: enable peripheral clk to access mram
  arm64: dts: fsd: Add MCAN device node
  can: m_can: Add ECC functionality for message RAM
  arm64: dts: fsd: Add support for error correction code for message RAM

 .../devicetree/bindings/arm/tesla-sysreg.yaml | 50 +++++++++++
 .../bindings/net/can/bosch,m_can.yaml         |  4 +
 MAINTAINERS                                   |  1 +
 arch/arm64/boot/dts/tesla/fsd-evb.dts         | 16 ++++
 arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi    | 28 +++++++
 arch/arm64/boot/dts/tesla/fsd.dtsi            | 82 +++++++++++++++++++
 drivers/net/can/m_can/m_can.c                 | 73 +++++++++++++++++
 drivers/net/can/m_can/m_can.h                 | 24 ++++++
 drivers/net/can/m_can/m_can_platform.c        | 11 ++-
 9 files changed, 288 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/arm/tesla-sysreg.yaml

-- 
2.17.1

[PATCH 1/7] dt-bindings: Document the SYSREG specific compatibles found on FSD SoC

[Vivek Yadav]

From: Sriranjani P <sriranjani.p@samsung.com>

Describe the compatible properties for SYSREG controllers found on
FSD SoC.

Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
Signed-off-by: Pankaj Kumar Dubey <pankaj.dubey@samsung.com>
Signed-off-by: Ravi Patel <ravi.patel@samsung.com>
Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
---
 .../devicetree/bindings/arm/tesla-sysreg.yaml | 50 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/tesla-sysreg.yaml

diff --git a/Documentation/devicetree/bindings/arm/tesla-sysreg.yaml b/Documentation/devicetree/bindings/arm/tesla-sysreg.yaml
new file mode 100644
index 000000000000..bbcc6dd75918
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/tesla-sysreg.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/tesla-sysreg.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Tesla Full Self-Driving platform's system registers
+
+maintainers:
+  - Alim Akhtar <alim.akhtar@samsung.com>
+
+description: |
+  This is a system control registers block, providing multiple low level
+  platform functions like board detection and identification, software
+  interrupt generation.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - tesla,sysreg_fsys0
+              - tesla,sysreg_peric
+          - const: syscon
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      sysreg_fsys0: system-controller@15030000 {
+            compatible = "tesla,sysreg_fsys0", "syscon";
+            reg = <0x0 0x15030000 0x0 0x1000>;
+      };
+
+      sysreg_peric: system-controller@14030000 {
+            compatible = "tesla,sysreg_peric", "syscon";
+            reg = <0x0 0x14030000 0x0 0x1000>;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index a198da986146..56995e7d63ad 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2943,6 +2943,7 @@ M:	linux-fsd@tesla.com
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 L:	linux-samsung-soc@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/arm/tesla-sysreg.yaml
 F:	arch/arm64/boot/dts/tesla*
 
 ARM/TETON BGA MACHINE SUPPORT
-- 
2.17.1

[PATCH 2/7] dt-bindings: can: mcan: Add ECC functionality to message ram

[Vivek Yadav]

Whenever the data is transferred or stored on message ram, there are
inherent risks of it being lost or corruption known as single-bit errors.

ECC constantly scans data as it is processed to the message ram, using a
method known as parity checking and raise the error signals for corruption.

Add error correction code config property to enable/disable the
error correction code (ECC) functionality for Message RAM used to create
valid ECC checksums.

Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>
---
 Documentation/devicetree/bindings/net/can/bosch,m_can.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
index 26aa0830eea1..0ba3691863d7 100644
--- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
+++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
@@ -50,6 +50,10 @@ properties:
       - const: hclk
       - const: cclk
 
+  mram-ecc-cfg:
+    items:
+      - description: M_CAN ecc registers map with configuration register offset
+
   bosch,mram-cfg:
     description: |
       Message RAM configuration data.
-- 
2.17.1

[PATCH 3/7] arm64: dts: fsd: add sysreg device node

[Vivek Yadav]

From: Sriranjani P <sriranjani.p@samsung.com>

Add SYSREG controller device node, which is available in PERIC and FSYS0
block of FSD SoC.

Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
Signed-off-by: Pankaj Kumar Dubey <pankaj.dubey@samsung.com>
Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
---
 arch/arm64/boot/dts/tesla/fsd.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi b/arch/arm64/boot/dts/tesla/fsd.dtsi
index f35bc5a288c2..3d8ebbfc27f4 100644
--- a/arch/arm64/boot/dts/tesla/fsd.dtsi
+++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
@@ -518,6 +518,16 @@
 				"dout_cmu_fsys1_shared0div4";
 		};
 
+		sysreg_peric: system-controller@14030000 {
+			compatible = "tesla,sysreg_peric", "syscon";
+			reg = <0x0 0x14030000 0x0 0x1000>;
+		};
+
+		sysreg_fsys0: system-controller@15030000 {
+			compatible = "tesla,sysreg_fsys0", "syscon";
+			reg = <0x0 0x15030000 0x0 0x1000>;
+		};
+
 		mdma0: dma-controller@10100000 {
 			compatible = "arm,pl330", "arm,primecell";
 			reg = <0x0 0x10100000 0x0 0x1000>;
-- 
2.17.1

[PATCH 4/7] can: mcan: enable peripheral clk to access mram

[Vivek Yadav]

When we try to access the mcan message ram addresses, make sure hclk is
not gated by any other drivers or disabled. Enable the clock (hclk) before
accessing the mram and disable it after that.

This is required in case if by-default hclk is gated.

Signed-off-by: Ravi Patel <ravi.patel@samsung.com>
Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>
---
 drivers/net/can/m_can/m_can_platform.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
index eee47bad0592..5aab025775f9 100644
--- a/drivers/net/can/m_can/m_can_platform.c
+++ b/drivers/net/can/m_can/m_can_platform.c
@@ -140,10 +140,17 @@ static int m_can_plat_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, mcan_class);
 
-	ret = m_can_init_ram(mcan_class);
+	/* clock needs to be enabled to access mram block */
+	ret = clk_prepare_enable(mcan_class->hclk);
 	if (ret)
 		goto probe_fail;
 
+	ret = m_can_init_ram(mcan_class);
+	if (ret)
+		goto mram_fail;
+
+	clk_disable_unprepare(mcan_class->hclk);
+
 	pm_runtime_enable(mcan_class->dev);
 	ret = m_can_class_register(mcan_class);
 	if (ret)
@@ -153,6 +160,8 @@ static int m_can_plat_probe(struct platform_device *pdev)
 
 out_runtime_disable:
 	pm_runtime_disable(mcan_class->dev);
+mram_fail:
+	clk_disable_unprepare(mcan_class->hclk);
 probe_fail:
 	m_can_class_free_dev(mcan_class->net);
 	return ret;
-- 
2.17.1

[PATCH 5/7] arm64: dts: fsd: Add MCAN device node

[Vivek Yadav]

Add MCAN device node and enable the same for FSD platform.
This also adds the required pin configuration for the same.

Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>
---
 arch/arm64/boot/dts/tesla/fsd-evb.dts      | 16 +++++
 arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 28 +++++++++
 arch/arm64/boot/dts/tesla/fsd.dtsi         | 68 ++++++++++++++++++++++
 3 files changed, 112 insertions(+)

diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts b/arch/arm64/boot/dts/tesla/fsd-evb.dts
index 1db6ddf03f01..af3862e9fe3b 100644
--- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
+++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
@@ -34,6 +34,22 @@
 	clock-frequency = <24000000>;
 };
 
+&m_can0 {
+	status = "okay";
+};
+
+&m_can1 {
+	status = "okay";
+};
+
+&m_can2 {
+	status = "okay";
+};
+
+&m_can3 {
+	status = "okay";
+};
+
 &serial_0 {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
index d0abb9aa0e9e..bb5289ebfef3 100644
--- a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
+++ b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
@@ -339,6 +339,34 @@
 		samsung,pin-pud = <FSD_PIN_PULL_UP>;
 		samsung,pin-drv = <FSD_PIN_DRV_LV1>;
 	};
+
+	m_can0_bus: m-can0-bus-pins {
+		samsung,pins = "gpd0-0", "gpd0-1";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
+	};
+
+	m_can1_bus: m-can1-bus-pins {
+		samsung,pins = "gpd0-2", "gpd0-3";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
+	};
+
+	m_can2_bus: m-can2-bus-pins {
+		samsung,pins = "gpd0-4", "gpd0-5";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
+	};
+
+	m_can3_bus: m-can3-bus-pins {
+		samsung,pins = "gpd0-6", "gpd0-7";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
+	};
 };
 
 &pinctrl_pmu {
diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi b/arch/arm64/boot/dts/tesla/fsd.dtsi
index 3d8ebbfc27f4..154fd3fc5895 100644
--- a/arch/arm64/boot/dts/tesla/fsd.dtsi
+++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
@@ -765,6 +765,74 @@
 			interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
+		m_can0: can@14088000 {
+			compatible = "bosch,m_can";
+			reg = <0x0 0x14088000 0x0 0x0200>,
+				<0x0 0x14080000 0x0 0x8000>;
+			reg-names = "m_can", "message_ram";
+			interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>,
+					<GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "int0", "int1";
+			pinctrl-names = "default";
+			pinctrl-0 = <&m_can0_bus>;
+			clocks = <&clock_peric PERIC_MCAN0_IPCLKPORT_PCLK>,
+				<&clock_peric PERIC_MCAN0_IPCLKPORT_CCLK>;
+			clock-names = "hclk", "cclk";
+			bosch,mram-cfg = <0x0 128 64 64 64 64 32 32>;
+			status = "disabled";
+		};
+
+		m_can1: can@14098000 {
+			compatible = "bosch,m_can";
+			reg = <0x0 0x14098000 0x0 0x0200>,
+				<0x0 0x14090000 0x0 0x8000>;
+			reg-names = "m_can", "message_ram";
+			interrupts = <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>,
+					<GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "int0", "int1";
+			pinctrl-names = "default";
+			pinctrl-0 = <&m_can1_bus>;
+			clocks = <&clock_peric PERIC_MCAN1_IPCLKPORT_PCLK>,
+				<&clock_peric PERIC_MCAN1_IPCLKPORT_CCLK>;
+			clock-names = "hclk", "cclk";
+			bosch,mram-cfg = <0x0 128 64 64 64 64 32 32>;
+			status = "disabled";
+		};
+
+		m_can2: can@140a8000 {
+			compatible = "bosch,m_can";
+			reg = <0x0 0x140a8000 0x0 0x0200>,
+				<0x0 0x140a0000 0x0 0x8000>;
+			reg-names = "m_can", "message_ram";
+			interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>,
+					<GIC_SPI 166 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "int0", "int1";
+			pinctrl-names = "default";
+			pinctrl-0 = <&m_can2_bus>;
+			clocks = <&clock_peric PERIC_MCAN2_IPCLKPORT_PCLK>,
+				<&clock_peric PERIC_MCAN2_IPCLKPORT_CCLK>;
+			clock-names = "hclk", "cclk";
+			bosch,mram-cfg = <0x0 128 64 64 64 64 32 32>;
+			status = "disabled";
+		};
+
+		m_can3: can@140b8000 {
+			compatible = "bosch,m_can";
+			reg = <0x0 0x140b8000 0x0 0x0200>,
+				<0x0 0x140b0000 0x0 0x8000>;
+			reg-names = "m_can", "message_ram";
+			interrupts = <GIC_SPI 168 IRQ_TYPE_LEVEL_HIGH>,
+					<GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "int0", "int1";
+			pinctrl-names = "default";
+			pinctrl-0 = <&m_can3_bus>;
+			clocks = <&clock_peric PERIC_MCAN3_IPCLKPORT_PCLK>,
+				<&clock_peric PERIC_MCAN3_IPCLKPORT_CCLK>;
+			clock-names = "hclk", "cclk";
+			bosch,mram-cfg = <0x0 128 64 64 64 64 32 32>;
+			status = "disabled";
+		};
+
 		spi_0: spi@14140000 {
 			compatible = "tesla,fsd-spi";
 			reg = <0x0 0x14140000 0x0 0x100>;
-- 
2.17.1

[PATCH 6/7] can: m_can: Add ECC functionality for message RAM

[Vivek Yadav]

Whenever MCAN Buffers and FIFOs are stored on message ram, there are
inherent risks of corruption known as single-bit errors.

Enable error correction code (ECC) data intigrity check for Message RAM
to create valid ECC checksums.

ECC uses a respective number of bits, which are added to each word as a
parity and that will raise the error signal on the corruption in the
Interrupt Register(IR).

Message RAM bit error controlled by input signal m_can_aeim_berr[0],
generated by an optional external parity / ECC logic attached to the
Message RAM.

This indicates either Bit Error detected and Corrected(BEC) or No bit
error detected when reading from Message RAM.

Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>
---
 drivers/net/can/m_can/m_can.c | 73 +++++++++++++++++++++++++++++++++++
 drivers/net/can/m_can/m_can.h | 24 ++++++++++++
 2 files changed, 97 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index dcb582563d5e..578146707d5b 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1535,9 +1535,62 @@ static void m_can_stop(struct net_device *dev)
 	cdev->can.state = CAN_STATE_STOPPED;
 }
 
+int m_can_config_mram_ecc_check(struct m_can_classdev *cdev, int enable,
+				struct device_node *np)
+{
+	int val = 0;
+	int offset = 0, ret = 0;
+	int delay_count = MRAM_INIT_TIMEOUT;
+	struct m_can_mraminit *mraminit = &cdev->mraminit_sys;
+
+	mraminit->syscon = syscon_regmap_lookup_by_phandle(np,
+							   "mram-ecc-cfg");
+	if (IS_ERR(mraminit->syscon)) {
+		/* can fail with -EPROBE_DEFER */
+		ret = PTR_ERR(mraminit->syscon);
+		return ret;
+	}
+
+	if (of_property_read_u32_index(np, "mram-ecc-cfg", 1,
+				       &mraminit->reg)) {
+		dev_err(cdev->dev, "couldn't get the mraminit reg. offset!\n");
+		return -ENODEV;
+	}
+
+	val = ((MRAM_ECC_ENABLE_MASK | MRAM_CFG_VALID_MASK |
+		MRAM_INIT_ENABLE_MASK) << offset);
+	regmap_clear_bits(mraminit->syscon, mraminit->reg, val);
+
+	if (enable) {
+		val = (MRAM_ECC_ENABLE_MASK | MRAM_INIT_ENABLE_MASK) << offset;
+		regmap_set_bits(mraminit->syscon, mraminit->reg, val);
+	}
+
+	/* after enable or disable valid flag need to be set*/
+	val = (MRAM_CFG_VALID_MASK << offset);
+	regmap_set_bits(mraminit->syscon, mraminit->reg, val);
+
+	if (enable) {
+		do {
+			regmap_read(mraminit->syscon, mraminit->reg, &val);
+
+			if (val & (MRAM_INIT_DONE_MASK << offset))
+				return 0;
+
+			udelay(1);
+		} while (delay_count--);
+
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 static int m_can_close(struct net_device *dev)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
+	struct device_node *np;
+	int err = 0;
 
 	netif_stop_queue(dev);
 
@@ -1557,6 +1610,15 @@ static int m_can_close(struct net_device *dev)
 	if (cdev->is_peripheral)
 		can_rx_offload_disable(&cdev->offload);
 
+	np = cdev->dev->of_node;
+
+	if (np && of_property_read_bool(np, "mram-ecc-cfg")) {
+		err = m_can_config_mram_ecc_check(cdev, ECC_DISABLE, np);
+		if (err < 0)
+			netdev_err(dev,
+				   "Message RAM ECC disable config failed\n");
+	}
+
 	close_candev(dev);
 
 	phy_power_off(cdev->transceiver);
@@ -1754,6 +1816,7 @@ static int m_can_open(struct net_device *dev)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	int err;
+	struct device_node *np;
 
 	err = phy_power_on(cdev->transceiver);
 	if (err)
@@ -1798,6 +1861,16 @@ static int m_can_open(struct net_device *dev)
 		goto exit_irq_fail;
 	}
 
+	np = cdev->dev->of_node;
+
+	if (np && of_property_read_bool(np, "mram-ecc-cfg")) {
+		err = m_can_config_mram_ecc_check(cdev, ECC_ENABLE, np);
+		if (err < 0) {
+			netdev_err(dev,
+				   "Message RAM ECC enable config failed\n");
+		}
+	}
+
 	/* start the m_can controller */
 	m_can_start(dev);
 
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 4c0267f9f297..3cbfdc64a7db 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -28,6 +28,8 @@
 #include <linux/can/dev.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/phy/phy.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
 
 /* m_can lec values */
 enum m_can_lec_type {
@@ -52,12 +54,33 @@ enum m_can_mram_cfg {
 	MRAM_CFG_NUM,
 };
 
+enum m_can_ecc_cfg {
+	ECC_DISABLE = 0,
+	ECC_ENABLE,
+};
+
 /* address offset and element number for each FIFO/Buffer in the Message RAM */
 struct mram_cfg {
 	u16 off;
 	u8  num;
 };
 
+/* MRAM_INIT_BITS */
+#define MRAM_CFG_VALID_SHIFT   5
+#define MRAM_CFG_VALID_MASK    BIT(MRAM_CFG_VALID_SHIFT)
+#define MRAM_ECC_ENABLE_SHIFT  3
+#define MRAM_ECC_ENABLE_MASK   BIT(MRAM_ECC_ENABLE_SHIFT)
+#define MRAM_INIT_ENABLE_SHIFT 1
+#define MRAM_INIT_ENABLE_MASK  BIT(MRAM_INIT_ENABLE_SHIFT)
+#define MRAM_INIT_DONE_SHIFT   0
+#define MRAM_INIT_DONE_MASK    BIT(MRAM_INIT_DONE_SHIFT)
+#define MRAM_INIT_TIMEOUT      50
+
+struct m_can_mraminit {
+	struct regmap *syscon;  /* for mraminit ctrl. reg. access */
+	unsigned int reg;       /* register index within syscon */
+};
+
 struct m_can_classdev;
 struct m_can_ops {
 	/* Device specific call backs */
@@ -92,6 +115,7 @@ struct m_can_classdev {
 	int pm_clock_support;
 	int is_peripheral;
 
+	struct m_can_mraminit mraminit_sys;     /* mraminit via syscon regmap */
 	struct mram_cfg mcfg[MRAM_CFG_NUM];
 };
 
-- 
2.17.1

[PATCH 7/7] arm64: dts: fsd: Add support for error correction code for message RAM

[Vivek Yadav]

Add mram-ecc-cfg property which indicates the error correction code config
and enable the same for FSD platform.

In FSD, error correction code (ECC) is configured via PERIC SYSREG registers.

Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>
---
 arch/arm64/boot/dts/tesla/fsd.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi b/arch/arm64/boot/dts/tesla/fsd.dtsi
index 154fd3fc5895..03d46a113612 100644
--- a/arch/arm64/boot/dts/tesla/fsd.dtsi
+++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
@@ -778,6 +778,7 @@
 			clocks = <&clock_peric PERIC_MCAN0_IPCLKPORT_PCLK>,
 				<&clock_peric PERIC_MCAN0_IPCLKPORT_CCLK>;
 			clock-names = "hclk", "cclk";
+			mram-ecc-cfg = <&sysreg_peric 0x700>;
 			bosch,mram-cfg = <0x0 128 64 64 64 64 32 32>;
 			status = "disabled";
 		};
@@ -795,6 +796,7 @@
 			clocks = <&clock_peric PERIC_MCAN1_IPCLKPORT_PCLK>,
 				<&clock_peric PERIC_MCAN1_IPCLKPORT_CCLK>;
 			clock-names = "hclk", "cclk";
+			mram-ecc-cfg = <&sysreg_peric 0x704>;
 			bosch,mram-cfg = <0x0 128 64 64 64 64 32 32>;
 			status = "disabled";
 		};
@@ -812,6 +814,7 @@
 			clocks = <&clock_peric PERIC_MCAN2_IPCLKPORT_PCLK>,
 				<&clock_peric PERIC_MCAN2_IPCLKPORT_CCLK>;
 			clock-names = "hclk", "cclk";
+			mram-ecc-cfg = <&sysreg_peric 0x708>;
 			bosch,mram-cfg = <0x0 128 64 64 64 64 32 32>;
 			status = "disabled";
 		};
@@ -829,6 +832,7 @@
 			clocks = <&clock_peric PERIC_MCAN3_IPCLKPORT_PCLK>,
 				<&clock_peric PERIC_MCAN3_IPCLKPORT_CCLK>;
 			clock-names = "hclk", "cclk";
+			mram-ecc-cfg = <&sysreg_peric 0x70c>;
 			bosch,mram-cfg = <0x0 128 64 64 64 64 32 32>;
 			status = "disabled";
 		};
-- 
2.17.1

Re: [PATCH 2/7] dt-bindings: can: mcan: Add ECC functionality to message ram

[Marc Kleine-Budde]

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

On 21.10.2022 15:28:28, Vivek Yadav wrote:
> Whenever the data is transferred or stored on message ram, there are
> inherent risks of it being lost or corruption known as single-bit errors.
> 
> ECC constantly scans data as it is processed to the message ram, using a
> method known as parity checking and raise the error signals for corruption.
> 
> Add error correction code config property to enable/disable the
> error correction code (ECC) functionality for Message RAM used to create
> valid ECC checksums.
> 
> Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
> Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>

Can you please add an example to the yaml that makes use of the
mram-ecc-cfg property?

regards,
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/7] dt-bindings: can: mcan: Add ECC functionality to message ram

[Marc Kleine-Budde]

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

You should add the DT people on Cc:
- devicetree@vger.kernel.org
- Rob Herring <robh+dt@kernel.org>

On 21.10.2022 15:28:28, Vivek Yadav wrote:
> Whenever the data is transferred or stored on message ram, there are
> inherent risks of it being lost or corruption known as single-bit errors.
> 
> ECC constantly scans data as it is processed to the message ram, using a
> method known as parity checking and raise the error signals for corruption.
> 
> Add error correction code config property to enable/disable the
> error correction code (ECC) functionality for Message RAM used to create
> valid ECC checksums.
> 
> Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
> Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>
> ---
>  Documentation/devicetree/bindings/net/can/bosch,m_can.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> index 26aa0830eea1..0ba3691863d7 100644
> --- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> +++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> @@ -50,6 +50,10 @@ properties:
>        - const: hclk
>        - const: cclk
>  
> +  mram-ecc-cfg:

This probably needs a prefix and a "$ref: /schemas/types.yaml#/definitions/phandle".

> +    items:
> +      - description: M_CAN ecc registers map with configuration register offset
> +
>    bosch,mram-cfg:
>      description: |
>        Message RAM configuration data.
> -- 
> 2.17.1
> 
> 

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/7] can: mcan: Add MCAN support for FSD SoC

[Marc Kleine-Budde]

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

On 21.10.2022 15:28:26, Vivek Yadav wrote:
> Add support for MCAN instances present on the FSD platform.
> Also add support for handling error correction code (ECC) for MCAN
> message RAM.

Some patches are missing your S-o-b.

regards,
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/7] can: mcan: enable peripheral clk to access mram

[Marc Kleine-Budde]

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

On 21.10.2022 15:28:30, Vivek Yadav wrote:
> When we try to access the mcan message ram addresses, make sure hclk is
> not gated by any other drivers or disabled. Enable the clock (hclk) before
> accessing the mram and disable it after that.
> 
> This is required in case if by-default hclk is gated.

From my point of view it makes no sense to init the RAM during probe.
Can you move the init_ram into the m_can_chip_config() function? The
clocks should be enabled then.

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 5/7] arm64: dts: fsd: Add MCAN device node

[Marc Kleine-Budde]

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

On 21.10.2022 15:28:31, Vivek Yadav wrote:
> Add MCAN device node and enable the same for FSD platform.
> This also adds the required pin configuration for the same.
> 
> Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
> Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>

Please add the DT people on Cc.

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/7] can: m_can: Add ECC functionality for message RAM

[Marc Kleine-Budde]

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

On 21.10.2022 15:28:32, Vivek Yadav wrote:
> Whenever MCAN Buffers and FIFOs are stored on message ram, there are
                                                        RAM
> inherent risks of corruption known as single-bit errors.
> 
> Enable error correction code (ECC) data intigrity check for Message RAM
> to create valid ECC checksums.
> 
> ECC uses a respective number of bits, which are added to each word as a
> parity and that will raise the error signal on the corruption in the
> Interrupt Register(IR).
> 
> Message RAM bit error controlled by input signal m_can_aeim_berr[0],
> generated by an optional external parity / ECC logic attached to the
> Message RAM.
> 
> This indicates either Bit Error detected and Corrected(BEC) or No bit
> error detected when reading from Message RAM.

There is no ECC error handler added to the code.

> Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>
> ---
>  drivers/net/can/m_can/m_can.c | 73 +++++++++++++++++++++++++++++++++++
>  drivers/net/can/m_can/m_can.h | 24 ++++++++++++
>  2 files changed, 97 insertions(+)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index dcb582563d5e..578146707d5b 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1535,9 +1535,62 @@ static void m_can_stop(struct net_device *dev)
>  	cdev->can.state = CAN_STATE_STOPPED;
>  }
>  
> +int m_can_config_mram_ecc_check(struct m_can_classdev *cdev, int enable,
static                                                          ^^^ bool
> +				struct device_node *np)
> +{
> +	int val = 0;
> +	int offset = 0, ret = 0;
> +	int delay_count = MRAM_INIT_TIMEOUT;
> +	struct m_can_mraminit *mraminit = &cdev->mraminit_sys;

Please sort by reverse Christmas tree.

> +
> +	mraminit->syscon = syscon_regmap_lookup_by_phandle(np,
> +							   "mram-ecc-cfg");

Please check if syscon_regmap_lookup_by_phandle_args() is better suited.

You probably want to do the syscon lookup once during
m_can_class_register().

> +	if (IS_ERR(mraminit->syscon)) {
> +		/* can fail with -EPROBE_DEFER */

m_can_config_mram_ecc_check() is called from m_can_open() and
m_can_close(), returning -EPROBE_DEFER makes no sense there.

> +		ret = PTR_ERR(mraminit->syscon);
> +		return ret;
> +	}
> +
> +	if (of_property_read_u32_index(np, "mram-ecc-cfg", 1,
> +				       &mraminit->reg)) {
> +		dev_err(cdev->dev, "couldn't get the mraminit reg. offset!\n");
> +		return -ENODEV;
> +	}
> +
> +	val = ((MRAM_ECC_ENABLE_MASK | MRAM_CFG_VALID_MASK |
> +		MRAM_INIT_ENABLE_MASK) << offset);

please make use of FIELD_PREP

> +	regmap_clear_bits(mraminit->syscon, mraminit->reg, val);
> +
> +	if (enable) {
> +		val = (MRAM_ECC_ENABLE_MASK | MRAM_INIT_ENABLE_MASK) << offset;

same here

> +		regmap_set_bits(mraminit->syscon, mraminit->reg, val);
> +	}
> +
> +	/* after enable or disable valid flag need to be set*/
                                                           ^^^
                                                           missing space
> +	val = (MRAM_CFG_VALID_MASK << offset);

here, too

> +	regmap_set_bits(mraminit->syscon, mraminit->reg, val);
> +
> +	if (enable) {
> +		do {
> +			regmap_read(mraminit->syscon, mraminit->reg, &val);
> +
> +			if (val & (MRAM_INIT_DONE_MASK << offset))
> +				return 0;
> +
> +			udelay(1);
> +		} while (delay_count--);

please make use of regmap_read_poll_timeout().

> +
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
>  static int m_can_close(struct net_device *dev)
>  {
>  	struct m_can_classdev *cdev = netdev_priv(dev);
> +	struct device_node *np;
> +	int err = 0;
>  
>  	netif_stop_queue(dev);
>  
> @@ -1557,6 +1610,15 @@ static int m_can_close(struct net_device *dev)
>  	if (cdev->is_peripheral)
>  		can_rx_offload_disable(&cdev->offload);
>  
> +	np = cdev->dev->of_node;
> +
> +	if (np && of_property_read_bool(np, "mram-ecc-cfg")) {
> +		err = m_can_config_mram_ecc_check(cdev, ECC_DISABLE, np);
> +		if (err < 0)
> +			netdev_err(dev,
> +				   "Message RAM ECC disable config failed\n");
> +	}
> +
>  	close_candev(dev);
>  
>  	phy_power_off(cdev->transceiver);
> @@ -1754,6 +1816,7 @@ static int m_can_open(struct net_device *dev)
>  {
>  	struct m_can_classdev *cdev = netdev_priv(dev);
>  	int err;
> +	struct device_node *np;
>  
>  	err = phy_power_on(cdev->transceiver);
>  	if (err)
> @@ -1798,6 +1861,16 @@ static int m_can_open(struct net_device *dev)
>  		goto exit_irq_fail;
>  	}
>  
> +	np = cdev->dev->of_node;
> +
> +	if (np && of_property_read_bool(np, "mram-ecc-cfg")) {
> +		err = m_can_config_mram_ecc_check(cdev, ECC_ENABLE, np);
> +		if (err < 0) {
> +			netdev_err(dev,
> +				   "Message RAM ECC enable config failed\n");
> +		}
> +	}
> +
>  	/* start the m_can controller */
>  	m_can_start(dev);
>  
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index 4c0267f9f297..3cbfdc64a7db 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -28,6 +28,8 @@
>  #include <linux/can/dev.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/phy/phy.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>

please make a separate patch that sorts these includes alphabetically,
then add the new includes sorted.

>  
>  /* m_can lec values */
>  enum m_can_lec_type {
> @@ -52,12 +54,33 @@ enum m_can_mram_cfg {
>  	MRAM_CFG_NUM,
>  };
>  
> +enum m_can_ecc_cfg {
> +	ECC_DISABLE = 0,
> +	ECC_ENABLE,
> +};
> +

unused

>  /* address offset and element number for each FIFO/Buffer in the Message RAM */
>  struct mram_cfg {
>  	u16 off;
>  	u8  num;
>  };
>  
> +/* MRAM_INIT_BITS */
> +#define MRAM_CFG_VALID_SHIFT   5
> +#define MRAM_CFG_VALID_MASK    BIT(MRAM_CFG_VALID_SHIFT)
> +#define MRAM_ECC_ENABLE_SHIFT  3
> +#define MRAM_ECC_ENABLE_MASK   BIT(MRAM_ECC_ENABLE_SHIFT)
> +#define MRAM_INIT_ENABLE_SHIFT 1
> +#define MRAM_INIT_ENABLE_MASK  BIT(MRAM_INIT_ENABLE_SHIFT)
> +#define MRAM_INIT_DONE_SHIFT   0
> +#define MRAM_INIT_DONE_MASK    BIT(MRAM_INIT_DONE_SHIFT)
> +#define MRAM_INIT_TIMEOUT      50

Please move these bits to the m_can.c file.

Add a common prefix M_CAN_ to them.

Remove the SHIFT values, directly define the MASK using BIT() (for
single set bits) or GEN_MASK() (for multiple set bits).

> +
> +struct m_can_mraminit {

Is this RAM init or ECC related?

> +	struct regmap *syscon;  /* for mraminit ctrl. reg. access */
> +	unsigned int reg;       /* register index within syscon */
> +};
> +
>  struct m_can_classdev;
>  struct m_can_ops {
>  	/* Device specific call backs */
> @@ -92,6 +115,7 @@ struct m_can_classdev {
>  	int pm_clock_support;
>  	int is_peripheral;
>  
> +	struct m_can_mraminit mraminit_sys;     /* mraminit via syscon regmap */
>  	struct mram_cfg mcfg[MRAM_CFG_NUM];
>  };
>  
> -- 
> 2.17.1
> 
>

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/7] dt-bindings: can: mcan: Add ECC functionality to message ram

[Vivek Yadav]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 25 October 2022 12:55
> To: Vivek Yadav <vivek.2311@samsung.com>
> Cc: rcsekar@samsung.com; wg@grandegger.com; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> pankaj.dubey@samsung.com; ravi.patel@samsung.com;
> alim.akhtar@samsung.com; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/7] dt-bindings: can: mcan: Add ECC functionality to
> message ram
> 
> On 21.10.2022 15:28:28, Vivek Yadav wrote:
> > Whenever the data is transferred or stored on message ram, there are
> > inherent risks of it being lost or corruption known as single-bit errors.
> >
> > ECC constantly scans data as it is processed to the message ram, using
> > a method known as parity checking and raise the error signals for
> corruption.
> >
> > Add error correction code config property to enable/disable the error
> > correction code (ECC) functionality for Message RAM used to create
> > valid ECC checksums.
> >
> > Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
> > Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>
> 
> Can you please add an example to the yaml that makes use of the mram-ecc-
> cfg property?
> 
Okay, I will add in next patch series.
> regards,
> Marc
> 
Thanks for review.
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   |
> https://protect2.fireeye.com/v1/url?k=ec0872b8-8d836798-ec09f9f7-
> 74fe485fb347-10f7a5cc45234a40&q=1&e=48595861-7733-4e80-bf96-
> bd85b4d16570&u=https%3A%2F%2Fwww.pengutronix.de%2F  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

RE: [PATCH 2/7] dt-bindings: can: mcan: Add ECC functionality to message ram

[Vivek Yadav]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 25 October 2022 13:02
> To: Vivek Yadav <vivek.2311@samsung.com>
> Cc: rcsekar@samsung.com; wg@grandegger.com; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> pankaj.dubey@samsung.com; ravi.patel@samsung.com;
> alim.akhtar@samsung.com; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/7] dt-bindings: can: mcan: Add ECC functionality to
> message ram
> 
> You should add the DT people on Cc:
> - devicetree@vger.kernel.org
> - Rob Herring <robh+dt@kernel.org>
> 
Okay, I will add them in the next patch series.
> On 21.10.2022 15:28:28, Vivek Yadav wrote:
> > Whenever the data is transferred or stored on message ram, there are
> > inherent risks of it being lost or corruption known as single-bit errors.
> >
> > ECC constantly scans data as it is processed to the message ram, using
> > a method known as parity checking and raise the error signals for
> corruption.
> >
> > Add error correction code config property to enable/disable the error
> > correction code (ECC) functionality for Message RAM used to create
> > valid ECC checksums.
> >
> > Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
> > Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>
> > ---
> >  Documentation/devicetree/bindings/net/can/bosch,m_can.yaml | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> > b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> > index 26aa0830eea1..0ba3691863d7 100644
> > --- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> > +++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> > @@ -50,6 +50,10 @@ properties:
> >        - const: hclk
> >        - const: cclk
> >
> > +  mram-ecc-cfg:
> 
> This probably needs a prefix and a "$ref:
> /schemas/types.yaml#/definitions/phandle".
> 
okay
I will add in the next patch series.
> > +    items:
> > +      - description: M_CAN ecc registers map with configuration
> > + register offset
> > +
> >    bosch,mram-cfg:
> >      description: |
> >        Message RAM configuration data.
> > --
> > 2.17.1
> >
> >
> 
> Marc
> 
Thanks for the review.
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   |
> https://protect2.fireeye.com/v1/url?k=79b0bfe8-195222b5-79b134a7-
> 000babd9f1ba-4774190ce98312a8&q=1&e=e3b63c25-f82a-4aa4-aaee-
> 156c142ee4c6&u=https%3A%2F%2Fwww.pengutronix.de%2F  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

RE: [PATCH 0/7] can: mcan: Add MCAN support for FSD SoC

[Vivek Yadav]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 25 October 2022 13:03
> To: Vivek Yadav <vivek.2311@samsung.com>
> Cc: rcsekar@samsung.com; wg@grandegger.com; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> pankaj.dubey@samsung.com; ravi.patel@samsung.com;
> alim.akhtar@samsung.com; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 0/7] can: mcan: Add MCAN support for FSD SoC
> 
> On 21.10.2022 15:28:26, Vivek Yadav wrote:
> > Add support for MCAN instances present on the FSD platform.
> > Also add support for handling error correction code (ECC) for MCAN
> > message RAM.
> 
> Some patches are missing your S-o-b.
> 
Okay, I will add.
> regards,
> Marc
> 
Thanks for the review.
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   |
> https://protect2.fireeye.com/v1/url?k=0b321df8-6ab908ce-0b3396b7-
> 000babff9b5d-2a0bd0286d8759f0&q=1&e=fed9ba03-7d7f-4421-bf55-
> 28e9d29d5ad6&u=https%3A%2F%2Fwww.pengutronix.de%2F  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

RE: [PATCH 5/7] arm64: dts: fsd: Add MCAN device node

[Vivek Yadav]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 25 October 2022 13:15
> To: Vivek Yadav <vivek.2311@samsung.com>
> Cc: rcsekar@samsung.com; wg@grandegger.com; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> pankaj.dubey@samsung.com; ravi.patel@samsung.com;
> alim.akhtar@samsung.com; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Sriranjani P
> <sriranjani.p@samsung.com>
> Subject: Re: [PATCH 5/7] arm64: dts: fsd: Add MCAN device node
> 
> On 21.10.2022 15:28:31, Vivek Yadav wrote:
> > Add MCAN device node and enable the same for FSD platform.
> > This also adds the required pin configuration for the same.
> >
> > Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
> > Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>
> 
> Please add the DT people on Cc.
Okay, I will add them in the next patch series.
> 
> Marc
> 
Thanks for the review.
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   |
> https://protect2.fireeye.com/v1/url?k=6c1d2429-0d96311f-6c1caf66-
> 000babff9b5d-435a1e79c4c5ee61&q=1&e=74fb5a49-eb28-4786-8c1d-
> 9aa91f25ec04&u=https%3A%2F%2Fwww.pengutronix.de%2F  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

RE: [PATCH 4/7] can: mcan: enable peripheral clk to access mram

[Vivek Yadav]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 25 October 2022 13:14
> To: Vivek Yadav <vivek.2311@samsung.com>
> Cc: rcsekar@samsung.com; wg@grandegger.com; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> pankaj.dubey@samsung.com; ravi.patel@samsung.com;
> alim.akhtar@samsung.com; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 4/7] can: mcan: enable peripheral clk to access mram
> 
> On 21.10.2022 15:28:30, Vivek Yadav wrote:
> > When we try to access the mcan message ram addresses, make sure hclk
> > is not gated by any other drivers or disabled. Enable the clock (hclk)
> > before accessing the mram and disable it after that.
> >
> > This is required in case if by-default hclk is gated.
> 
> From my point of view it makes no sense to init the RAM during probe.
> Can you move the init_ram into the m_can_chip_config() function? The
> clocks should be enabled then.
> 
As per my understanding, we should not remove message ram init from probe because if message ram init failed then there will be no 
Storing of Tx/Rx messages onto message ram, so it's better to confirm write operations onto message ram before CAN communication.

So we can kept init_ram in the probe function only, but we will shift init_ram into m_can_dev_setup function by the time clks are already enabled.
> Marc
> 
Thanks for the review.
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://protect2.fireeye.com/v1/url?k=fc7bf79b-
> 9c996ac6-fc7a7cd4-000babd9f1ba-3024ea0d5d83d168&q=1&e=87d053cd-
> e4ab-41e4-a21b-
> c348747c0ce5&u=https%3A%2F%2Fwww.pengutronix.de%2F  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

RE: [PATCH 6/7] can: m_can: Add ECC functionality for message RAM

[Vivek Yadav]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 25 October 2022 13:46
> To: Vivek Yadav <vivek.2311@samsung.com>
> Cc: rcsekar@samsung.com; wg@grandegger.com; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> pankaj.dubey@samsung.com; ravi.patel@samsung.com;
> alim.akhtar@samsung.com; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 6/7] can: m_can: Add ECC functionality for message RAM
> 
> On 21.10.2022 15:28:32, Vivek Yadav wrote:
> > Whenever MCAN Buffers and FIFOs are stored on message ram, there are
>                                                         RAM
> > inherent risks of corruption known as single-bit errors.
> >
> > Enable error correction code (ECC) data intigrity check for Message
> > RAM to create valid ECC checksums.
> >
> > ECC uses a respective number of bits, which are added to each word as
> > a parity and that will raise the error signal on the corruption in the
> > Interrupt Register(IR).
> >
> > Message RAM bit error controlled by input signal m_can_aeim_berr[0],
> > generated by an optional external parity / ECC logic attached to the
> > Message RAM.
> >
I will remove this text from commit as this indicates the handling of ECC error.

> > This indicates either Bit Error detected and Corrected(BEC) or No bit
> > error detected when reading from Message RAM.
> 
> There is no ECC error handler added to the code.
> 
Yes, we are not adding the ECC error handler in the code. 
As per my understanding this is already handled in <m_can_handle_other_err> function.

> > Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>
> > ---
> >  drivers/net/can/m_can/m_can.c | 73
> > +++++++++++++++++++++++++++++++++++
> >  drivers/net/can/m_can/m_can.h | 24 ++++++++++++
> >  2 files changed, 97 insertions(+)
> >
> > diff --git a/drivers/net/can/m_can/m_can.c
> > b/drivers/net/can/m_can/m_can.c index dcb582563d5e..578146707d5b
> > 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -1535,9 +1535,62 @@ static void m_can_stop(struct net_device *dev)
> >  	cdev->can.state = CAN_STATE_STOPPED;  }
> >
> > +int m_can_config_mram_ecc_check(struct m_can_classdev *cdev, int
> > +enable,
> static                                                          ^^^ bool
> > +				struct device_node *np)
> > +{
> > +	int val = 0;
> > +	int offset = 0, ret = 0;
> > +	int delay_count = MRAM_INIT_TIMEOUT;
> > +	struct m_can_mraminit *mraminit = &cdev->mraminit_sys;
> 
> Please sort by reverse Christmas tree.
> 
Okay, I will address this in next patch series.
> > +
> > +	mraminit->syscon = syscon_regmap_lookup_by_phandle(np,
> > +							   "mram-ecc-cfg");
> 
> Please check if syscon_regmap_lookup_by_phandle_args() is better suited.
> 
Okay, I will check and make it better.
> You probably want to do the syscon lookup once during
> m_can_class_register().
>
Yes, It should be once only, I will address this.
> > +	if (IS_ERR(mraminit->syscon)) {
> > +		/* can fail with -EPROBE_DEFER */
> 
> m_can_config_mram_ecc_check() is called from m_can_open() and
> m_can_close(), returning -EPROBE_DEFER makes no sense there.
> 
> > +		ret = PTR_ERR(mraminit->syscon);
> > +		return ret;
> > +	}
> > +
> > +	if (of_property_read_u32_index(np, "mram-ecc-cfg", 1,
> > +				       &mraminit->reg)) {
> > +		dev_err(cdev->dev, "couldn't get the mraminit reg.
> offset!\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	val = ((MRAM_ECC_ENABLE_MASK | MRAM_CFG_VALID_MASK |
> > +		MRAM_INIT_ENABLE_MASK) << offset);
> 
> please make use of FIELD_PREP
> 
Okay, I will do.
> > +	regmap_clear_bits(mraminit->syscon, mraminit->reg, val);
> > +
> > +	if (enable) {
> > +		val = (MRAM_ECC_ENABLE_MASK |
> MRAM_INIT_ENABLE_MASK) << offset;
> 
> same here
> 
okay
> > +		regmap_set_bits(mraminit->syscon, mraminit->reg, val);
> > +	}
> > +
> > +	/* after enable or disable valid flag need to be set*/
>                                                            ^^^
>                                                            missing space
> > +	val = (MRAM_CFG_VALID_MASK << offset);
> 
> here, too
> 
okay
> > +	regmap_set_bits(mraminit->syscon, mraminit->reg, val);
> > +
> > +	if (enable) {
> > +		do {
> > +			regmap_read(mraminit->syscon, mraminit->reg,
> &val);
> > +
> > +			if (val & (MRAM_INIT_DONE_MASK << offset))
> > +				return 0;
> > +
> > +			udelay(1);
> > +		} while (delay_count--);
> 
> please make use of regmap_read_poll_timeout().
> 
Okay, I will add this in next patch series.
> > +
> > +		return -ENODEV;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int m_can_close(struct net_device *dev)  {
> >  	struct m_can_classdev *cdev = netdev_priv(dev);
> > +	struct device_node *np;
> > +	int err = 0;
> >
> >  	netif_stop_queue(dev);
> >
> > @@ -1557,6 +1610,15 @@ static int m_can_close(struct net_device *dev)
> >  	if (cdev->is_peripheral)
> >  		can_rx_offload_disable(&cdev->offload);
> >
> > +	np = cdev->dev->of_node;
> > +
> > +	if (np && of_property_read_bool(np, "mram-ecc-cfg")) {
> > +		err = m_can_config_mram_ecc_check(cdev, ECC_DISABLE,
> np);
> > +		if (err < 0)
> > +			netdev_err(dev,
> > +				   "Message RAM ECC disable config
> failed\n");
> > +	}
> > +
> >  	close_candev(dev);
> >
> >  	phy_power_off(cdev->transceiver);
> > @@ -1754,6 +1816,7 @@ static int m_can_open(struct net_device *dev)  {
> >  	struct m_can_classdev *cdev = netdev_priv(dev);
> >  	int err;
> > +	struct device_node *np;
> >
> >  	err = phy_power_on(cdev->transceiver);
> >  	if (err)
> > @@ -1798,6 +1861,16 @@ static int m_can_open(struct net_device *dev)
> >  		goto exit_irq_fail;
> >  	}
> >
> > +	np = cdev->dev->of_node;
> > +
> > +	if (np && of_property_read_bool(np, "mram-ecc-cfg")) {
> > +		err = m_can_config_mram_ecc_check(cdev, ECC_ENABLE,
> np);
> > +		if (err < 0) {
> > +			netdev_err(dev,
> > +				   "Message RAM ECC enable config
> failed\n");
> > +		}
> > +	}
> > +
> >  	/* start the m_can controller */
> >  	m_can_start(dev);
> >
> > diff --git a/drivers/net/can/m_can/m_can.h
> > b/drivers/net/can/m_can/m_can.h index 4c0267f9f297..3cbfdc64a7db
> > 100644
> > --- a/drivers/net/can/m_can/m_can.h
> > +++ b/drivers/net/can/m_can/m_can.h
> > @@ -28,6 +28,8 @@
> >  #include <linux/can/dev.h>
> >  #include <linux/pinctrl/consumer.h>
> >  #include <linux/phy/phy.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/regmap.h>
> 
> please make a separate patch that sorts these includes alphabetically, then
> add the new includes sorted.
> 
Okay, I will post the separate patch for this.
> >
> >  /* m_can lec values */
> >  enum m_can_lec_type {
> > @@ -52,12 +54,33 @@ enum m_can_mram_cfg {
> >  	MRAM_CFG_NUM,
> >  };
> >
> > +enum m_can_ecc_cfg {
> > +	ECC_DISABLE = 0,
> > +	ECC_ENABLE,
> > +};
> > +
> 
> unused
> 
Okay, I will remove or make better use of this.
> >  /* address offset and element number for each FIFO/Buffer in the
> > Message RAM */  struct mram_cfg {
> >  	u16 off;
> >  	u8  num;
> >  };
> >
> > +/* MRAM_INIT_BITS */
> > +#define MRAM_CFG_VALID_SHIFT   5
> > +#define MRAM_CFG_VALID_MASK    BIT(MRAM_CFG_VALID_SHIFT)
> > +#define MRAM_ECC_ENABLE_SHIFT  3
> > +#define MRAM_ECC_ENABLE_MASK   BIT(MRAM_ECC_ENABLE_SHIFT)
> > +#define MRAM_INIT_ENABLE_SHIFT 1
> > +#define MRAM_INIT_ENABLE_MASK  BIT(MRAM_INIT_ENABLE_SHIFT)
> > +#define MRAM_INIT_DONE_SHIFT   0
> > +#define MRAM_INIT_DONE_MASK    BIT(MRAM_INIT_DONE_SHIFT)
> > +#define MRAM_INIT_TIMEOUT      50
> 
> Please move these bits to the m_can.c file.
> 
Okay, I will move.
> Add a common prefix M_CAN_ to them.
> 
Okay, I will address this in next patch series.
> Remove the SHIFT values, directly define the MASK using BIT() (for single set
> bits) or GEN_MASK() (for multiple set bits).
> 
> > +
> > +struct m_can_mraminit {
> 
> Is this RAM init or ECC related?
> 
This is for ECC only, I will give better naming for this for better understanding.
> > +	struct regmap *syscon;  /* for mraminit ctrl. reg. access */
> > +	unsigned int reg;       /* register index within syscon */
> > +};
> > +
> >  struct m_can_classdev;
> >  struct m_can_ops {
> >  	/* Device specific call backs */
> > @@ -92,6 +115,7 @@ struct m_can_classdev {
> >  	int pm_clock_support;
> >  	int is_peripheral;
> >
> > +	struct m_can_mraminit mraminit_sys;     /* mraminit via syscon
> regmap */
> >  	struct mram_cfg mcfg[MRAM_CFG_NUM];
> >  };
> >
> > --
> > 2.17.1
> >
> >
> 
> Marc
> 
Thank you for your feedback and reviewing the patches.
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   |
> https://protect2.fireeye.com/v1/url?k=7f1e79b1-1e956c87-7f1ff2fe-
> 74fe485cbff1-92aa04a06e5e6383&q=1&e=543e935e-4838-4692-b1da-
> d42741c9ad3f&u=https%3A%2F%2Fwww.pengutronix.de%2F  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/7] arm64: dts: fsd: add sysreg device node

[Krzysztof Kozlowski]

On 21/10/2022 11:58, Vivek Yadav wrote:
> From: Sriranjani P <sriranjani.p@samsung.com>
> 
> Add SYSREG controller device node, which is available in PERIC and FSYS0
> block of FSD SoC.
> 
> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> Signed-off-by: Pankaj Kumar Dubey <pankaj.dubey@samsung.com>
> Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>

You sent a v2 with correct CC list but I still would like to express
here my disappointment that you ignored kernel CC rules. You are pushing
SoC stuff bypassing SoC maintainers, so let's make it explicit for
Patchwork:

NAK.

Best regards,
Krzysztof