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
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 <[email protected]>
Signed-off-by: Vivek Yadav <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Vivek Yadav <[email protected]>
---
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
From: Sriranjani P <[email protected]>
Add SYSREG controller device node, which is available in PERIC and FSYS0
block of FSD SoC.
Signed-off-by: Alim Akhtar <[email protected]>
Signed-off-by: Pankaj Kumar Dubey <[email protected]>
Signed-off-by: Sriranjani P <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Vivek Yadav <[email protected]>
---
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
Hi Vivek,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.1-rc1 next-20221021]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Vivek-Yadav/dt-bindings-Document-the-SYSREG-specific-compatibles-found-on-FSD-SoC/20221021-204010
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20221021095833.62406-7-vivek.2311%40samsung.com
patch subject: [PATCH 6/7] can: m_can: Add ECC functionality for message RAM
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/05f01ceda6f70e1942c5530e04637da412b914da
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Vivek-Yadav/dt-bindings-Document-the-SYSREG-specific-compatibles-found-on-FSD-SoC/20221021-204010
git checkout 05f01ceda6f70e1942c5530e04637da412b914da
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/net/can/m_can/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/net/can/m_can/m_can.c:1538:5: warning: no previous prototype for 'm_can_config_mram_ecc_check' [-Wmissing-prototypes]
1538 | int m_can_config_mram_ecc_check(struct m_can_classdev *cdev, int enable,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/m_can_config_mram_ecc_check +1538 drivers/net/can/m_can/m_can.c
1537
> 1538 int m_can_config_mram_ecc_check(struct m_can_classdev *cdev, int enable,
1539 struct device_node *np)
1540 {
1541 int val = 0;
1542 int offset = 0, ret = 0;
1543 int delay_count = MRAM_INIT_TIMEOUT;
1544 struct m_can_mraminit *mraminit = &cdev->mraminit_sys;
1545
1546 mraminit->syscon = syscon_regmap_lookup_by_phandle(np,
1547 "mram-ecc-cfg");
1548 if (IS_ERR(mraminit->syscon)) {
1549 /* can fail with -EPROBE_DEFER */
1550 ret = PTR_ERR(mraminit->syscon);
1551 return ret;
1552 }
1553
1554 if (of_property_read_u32_index(np, "mram-ecc-cfg", 1,
1555 &mraminit->reg)) {
1556 dev_err(cdev->dev, "couldn't get the mraminit reg. offset!\n");
1557 return -ENODEV;
1558 }
1559
1560 val = ((MRAM_ECC_ENABLE_MASK | MRAM_CFG_VALID_MASK |
1561 MRAM_INIT_ENABLE_MASK) << offset);
1562 regmap_clear_bits(mraminit->syscon, mraminit->reg, val);
1563
1564 if (enable) {
1565 val = (MRAM_ECC_ENABLE_MASK | MRAM_INIT_ENABLE_MASK) << offset;
1566 regmap_set_bits(mraminit->syscon, mraminit->reg, val);
1567 }
1568
1569 /* after enable or disable valid flag need to be set*/
1570 val = (MRAM_CFG_VALID_MASK << offset);
1571 regmap_set_bits(mraminit->syscon, mraminit->reg, val);
1572
1573 if (enable) {
1574 do {
1575 regmap_read(mraminit->syscon, mraminit->reg, &val);
1576
1577 if (val & (MRAM_INIT_DONE_MASK << offset))
1578 return 0;
1579
1580 udelay(1);
1581 } while (delay_count--);
1582
1583 return -ENODEV;
1584 }
1585
1586 return 0;
1587 }
1588
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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 <[email protected]>
> Signed-off-by: Vivek Yadav <[email protected]>
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 |
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 <[email protected]>
> Signed-off-by: Vivek Yadav <[email protected]>
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 |
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 |
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 |
You should add the DT people on Cc:
- [email protected]
- Rob Herring <[email protected]>
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 <[email protected]>
> Signed-off-by: Vivek Yadav <[email protected]>
> ---
> 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 |
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 <[email protected]>
> ---
> 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 |
> -----Original Message-----
> From: Marc Kleine-Budde <[email protected]>
> Sent: 25 October 2022 13:03
> To: Vivek Yadav <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> 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%2Fhttp://www.pengutronix.de%2F |
> Vertretung West/Dortmund | Phone: +49-231-2826-924 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> -----Original Message-----
> From: Marc Kleine-Budde <[email protected]>
> Sent: 25 October 2022 12:55
> To: Vivek Yadav <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> 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 <[email protected]>
> > Signed-off-by: Vivek Yadav <[email protected]>
>
> 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%2Fhttp://www.pengutronix.de%2F |
> Vertretung West/Dortmund | Phone: +49-231-2826-924 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> -----Original Message-----
> From: Marc Kleine-Budde <[email protected]>
> Sent: 25 October 2022 13:02
> To: Vivek Yadav <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 2/7] dt-bindings: can: mcan: Add ECC functionality to
> message ram
>
> You should add the DT people on Cc:
> - [email protected]
> - Rob Herring <[email protected]>
>
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 <[email protected]>
> > Signed-off-by: Vivek Yadav <[email protected]>
> > ---
> > 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%2Fhttp://www.pengutronix.de%2F |
> Vertretung West/Dortmund | Phone: +49-231-2826-924 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> -----Original Message-----
> From: Marc Kleine-Budde <[email protected]>
> Sent: 25 October 2022 13:15
> To: Vivek Yadav <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Sriranjani P
> <[email protected]>
> 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 <[email protected]>
> > Signed-off-by: Vivek Yadav <[email protected]>
>
> 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%2Fhttp://www.pengutronix.de%2F |
> Vertretung West/Dortmund | Phone: +49-231-2826-924 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> -----Original Message-----
> From: Marc Kleine-Budde <[email protected]>
> Sent: 25 October 2022 13:14
> To: Vivek Yadav <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> 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%2Fhttp://www.pengutronix.de%2F |
> Vertretung West/Dortmund | Phone: +49-231-2826-924 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 21/10/2022 11:58, Vivek Yadav wrote:
> From: Sriranjani P <[email protected]>
>
> Add SYSREG controller device node, which is available in PERIC and FSYS0
> block of FSD SoC.
>
> Signed-off-by: Alim Akhtar <[email protected]>
> Signed-off-by: Pankaj Kumar Dubey <[email protected]>
> Signed-off-by: Sriranjani P <[email protected]>
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
> -----Original Message-----
> From: Marc Kleine-Budde <[email protected]>
> Sent: 25 October 2022 13:46
> To: Vivek Yadav <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> 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 <[email protected]>
> > ---
> > 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%2Fhttp://www.pengutronix.de%2F |
> Vertretung West/Dortmund | Phone: +49-231-2826-924 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |