[PATCH 0/3] can: xilinx_can: Add ECC feature support

[PATCH 0/3] can: xilinx_can: Add ECC feature support

[Srinivas Goud]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 871 bytes --]

ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller.
Part of this feature configuration and counter registers added 
in Xilinx CAN Controller for 1bit/2bit ECC errors count and reset.
Please find more details in PG096 v5.1 document.

xlnx,has-ecc is optional property and added to Xilinx CAN Controller 
node if ECC block enabled in the HW.

Driver reports 1bit/2bit ECC errors for FIFO's based on ECC error interrupts
and also create debugfs entry for reading all the ECC errors.


Srinivas Goud (3):
  dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’
  can: xilinx_can: Add ECC support
  can: xilinx_can: Add debugfs support for ECC

 .../devicetree/bindings/net/can/xilinx,can.yaml    |   5 +
 drivers/net/can/xilinx_can.c                       | 169 ++++++++++++++++++++-
 2 files changed, 169 insertions(+), 5 deletions(-)

-- 
2.1.1

[PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’

[Srinivas Goud]

ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller.
Part of this feature configuration and counter registers
added in IP for 1bit/2bit ECC errors.
Please find more details in PG096 v5.1 document.

xlnx,has-ecc is optional property and added to Xilinx CAN
Controller node if ECC block enabled in the HW.

Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
---
 Documentation/devicetree/bindings/net/can/xilinx,can.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/xilinx,can.yaml b/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
index 897d2cb..13503ae 100644
--- a/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
+++ b/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
@@ -46,6 +46,10 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: CAN Tx mailbox buffer count (CAN FD)
 
+  xlnx,has-ecc:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: CAN Tx and Rx fifo ECC enable flag (AXI CAN)
+
 required:
   - compatible
   - reg
@@ -134,6 +138,7 @@ examples:
         interrupts = <GIC_SPI 59 IRQ_TYPE_EDGE_RISING>;
         tx-fifo-depth = <0x40>;
         rx-fifo-depth = <0x40>;
+        xlnx,has-ecc;
     };
 
   - |
-- 
2.1.1

[PATCH 2/3] can: xilinx_can: Add ECC support

[Srinivas Goud]

Add ECC support for Xilinx CAN Controller, so this driver reports
1bit/2bit ECC errors for FIFO's based on ECC error interrupt.
ECC feature for Xilinx CAN Controller selected through
'xlnx,has-ecc' DT property

Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
---
 drivers/net/can/xilinx_can.c | 109 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 104 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 43c812e..311e435 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -63,6 +63,12 @@ enum xcan_reg {
 	XCAN_RXMSG_2_BASE_OFFSET	= 0x2100, /* RX Message Space */
 	XCAN_AFR_2_MASK_OFFSET	= 0x0A00, /* Acceptance Filter MASK */
 	XCAN_AFR_2_ID_OFFSET	= 0x0A04, /* Acceptance Filter ID */
+
+	/* only on AXI CAN cores */
+	XCAN_ECC_CFG_OFFSET	= 0xC8,	/* ECC Configuration */
+	XCAN_TXTLFIFO_ECC_OFFSET	= 0xCC, /* TXTL FIFO ECC error counter */
+	XCAN_TXOLFIFO_ECC_OFFSET	= 0xD0, /* TXOL FIFO ECC error counter */
+	XCAN_RXFIFO_ECC_OFFSET		= 0xD4, /* RX FIFO ECC error counter */
 };
 
 #define XCAN_FRAME_ID_OFFSET(frame_base)	((frame_base) + 0x00)
@@ -135,6 +141,17 @@ enum xcan_reg {
 #define XCAN_2_FSR_RI_MASK		0x0000003F /* RX Read Index */
 #define XCAN_DLCR_EDL_MASK		0x08000000 /* EDL Mask in DLC */
 #define XCAN_DLCR_BRS_MASK		0x04000000 /* BRS Mask in DLC */
+#define XCAN_IXR_E2BERX_MASK		BIT(23) /* RX FIFO two bit ECC error */
+#define XCAN_IXR_E1BERX_MASK		BIT(22) /* RX FIFO one bit ECC error */
+#define XCAN_IXR_E2BETXOL_MASK		BIT(21) /* TXOL FIFO two bit ECC error */
+#define XCAN_IXR_E1BETXOL_MASK		BIT(20) /* TXOL FIFO One bit ECC error */
+#define XCAN_IXR_E2BETXTL_MASK		BIT(19) /* TXTL FIFO Two bit ECC error */
+#define XCAN_IXR_E1BETXTL_MASK		BIT(18) /* TXTL FIFO One bit ECC error */
+#define XCAN_ECC_CFG_REECRX_MASK	BIT(2) /* Reset RX FIFO ECC error counters */
+#define XCAN_ECC_CFG_REECTXOL_MASK	BIT(1) /* Reset TXOL FIFO ECC error counters */
+#define XCAN_ECC_CFG_REECTXTL_MASK	BIT(0) /* Reset TXTL FIFO ECC error counters */
+#define XCAN_ECC_1BIT_CNT_MASK		GENMASK(15, 0) /* FIFO ECC 1bit count mask */
+#define XCAN_ECC_2BIT_CNT_MASK		GENMASK(31, 16) /* FIFO ECC 2bit count mask */
 
 /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */
 #define XCAN_BRPR_TDC_ENABLE		BIT(16) /* Transmitter Delay Compensation (TDC) Enable */
@@ -198,6 +215,13 @@ struct xcan_devtype_data {
  * @bus_clk:			Pointer to struct clk
  * @can_clk:			Pointer to struct clk
  * @devtype:			Device type specific constants
+ * @ecc_enable:			ECC enable flag
+ * @ecc_2bit_rxfifo_cnt:	RXFIFO 2bit ECC count
+ * @ecc_1bit_rxfifo_cnt:	RXFIFO 1bit ECC count
+ * @ecc_2bit_txolfifo_cnt:	TXOLFIFO 2bit ECC count
+ * @ecc_1bit_txolfifo_cnt:	TXOLFIFO 1bit ECC count
+ * @ecc_2bit_txtlfifo_cnt:	TXTLFIFO 2bit ECC count
+ * @ecc_1bit_txtlfifo_cnt:	TXTLFIFO 1bit ECC count
  */
 struct xcan_priv {
 	struct can_priv can;
@@ -215,6 +239,13 @@ struct xcan_priv {
 	struct clk *bus_clk;
 	struct clk *can_clk;
 	struct xcan_devtype_data devtype;
+	bool ecc_enable;
+	u32 ecc_2bit_rxfifo_cnt;
+	u32 ecc_1bit_rxfifo_cnt;
+	u32 ecc_2bit_txolfifo_cnt;
+	u32 ecc_1bit_txolfifo_cnt;
+	u32 ecc_2bit_txtlfifo_cnt;
+	u32 ecc_1bit_txtlfifo_cnt;
 };
 
 /* CAN Bittiming constants as per Xilinx CAN specs */
@@ -517,6 +548,11 @@ static int xcan_chip_start(struct net_device *ndev)
 		XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
 		XCAN_IXR_ARBLST_MASK | xcan_rx_int_mask(priv);
 
+	if (priv->ecc_enable)
+		ier |= XCAN_IXR_E2BERX_MASK | XCAN_IXR_E1BERX_MASK |
+			XCAN_IXR_E2BETXOL_MASK | XCAN_IXR_E1BETXOL_MASK |
+			XCAN_IXR_E2BETXTL_MASK | XCAN_IXR_E1BETXTL_MASK;
+
 	if (priv->devtype.flags & XCAN_FLAG_RXMNF)
 		ier |= XCAN_IXR_RXMNF_MASK;
 
@@ -1121,6 +1157,56 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
 		priv->can.can_stats.bus_error++;
 	}
 
+	if (priv->ecc_enable) {
+		u32 reg_ecc;
+
+		reg_ecc = priv->read_reg(priv, XCAN_RXFIFO_ECC_OFFSET);
+		if (isr & XCAN_IXR_E2BERX_MASK) {
+			priv->ecc_2bit_rxfifo_cnt +=
+				FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, reg_ecc);
+			netdev_dbg(ndev, "%s: RX FIFO 2bit ECC error count %d\n",
+				   __func__, priv->ecc_2bit_rxfifo_cnt);
+		}
+		if (isr & XCAN_IXR_E1BERX_MASK) {
+			priv->ecc_1bit_rxfifo_cnt += reg_ecc &
+				XCAN_ECC_1BIT_CNT_MASK;
+			netdev_dbg(ndev, "%s: RX FIFO 1bit ECC error count %d\n",
+				   __func__, priv->ecc_1bit_rxfifo_cnt);
+		}
+
+		reg_ecc = priv->read_reg(priv, XCAN_TXOLFIFO_ECC_OFFSET);
+		if (isr & XCAN_IXR_E2BETXOL_MASK) {
+			priv->ecc_2bit_txolfifo_cnt +=
+				FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, reg_ecc);
+			netdev_dbg(ndev, "%s: TXOL FIFO 2bit ECC error count %d\n",
+				   __func__, priv->ecc_2bit_txolfifo_cnt);
+		}
+		if (isr & XCAN_IXR_E1BETXOL_MASK) {
+			priv->ecc_1bit_txolfifo_cnt += reg_ecc &
+				XCAN_ECC_1BIT_CNT_MASK;
+			netdev_dbg(ndev, "%s: TXOL FIFO 1bit ECC error count %d\n",
+				   __func__, priv->ecc_1bit_txolfifo_cnt);
+		}
+
+		reg_ecc = priv->read_reg(priv, XCAN_TXTLFIFO_ECC_OFFSET);
+		if (isr & XCAN_IXR_E2BETXTL_MASK) {
+			priv->ecc_2bit_txtlfifo_cnt +=
+				FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, reg_ecc);
+			netdev_dbg(ndev, "%s: TXTL FIFO 2bit ECC error count %d\n",
+				   __func__, priv->ecc_2bit_txtlfifo_cnt);
+		}
+		if (isr & XCAN_IXR_E1BETXTL_MASK) {
+			priv->ecc_1bit_txtlfifo_cnt += reg_ecc &
+				XCAN_ECC_1BIT_CNT_MASK;
+			netdev_dbg(ndev, "%s: TXTL FIFO 1bit ECC error count %d\n",
+				   __func__, priv->ecc_1bit_txtlfifo_cnt);
+		}
+
+		/* Reset FIFO ECC counters */
+		priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, XCAN_ECC_CFG_REECRX_MASK |
+				XCAN_ECC_CFG_REECTXOL_MASK | XCAN_ECC_CFG_REECTXTL_MASK);
+	}
+
 	if (cf.can_id) {
 		struct can_frame *skb_cf;
 		struct sk_buff *skb = alloc_can_err_skb(ndev, &skb_cf);
@@ -1348,9 +1434,8 @@ static irqreturn_t xcan_interrupt(int irq, void *dev_id)
 {
 	struct net_device *ndev = (struct net_device *)dev_id;
 	struct xcan_priv *priv = netdev_priv(ndev);
-	u32 isr, ier;
-	u32 isr_errors;
 	u32 rx_int_mask = xcan_rx_int_mask(priv);
+	u32 isr, ier, isr_errors, mask;
 
 	/* Get the interrupt status from Xilinx CAN */
 	isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
@@ -1368,10 +1453,18 @@ static irqreturn_t xcan_interrupt(int irq, void *dev_id)
 	if (isr & XCAN_IXR_TXOK_MASK)
 		xcan_tx_interrupt(ndev, isr);
 
+	mask = XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
+		XCAN_IXR_BSOFF_MASK | XCAN_IXR_ARBLST_MASK |
+		XCAN_IXR_RXMNF_MASK;
+
+	if (priv->ecc_enable)
+		mask |= XCAN_IXR_E2BERX_MASK | XCAN_IXR_E1BERX_MASK |
+			XCAN_IXR_E2BETXOL_MASK | XCAN_IXR_E1BETXOL_MASK |
+			XCAN_IXR_E2BETXTL_MASK | XCAN_IXR_E1BETXTL_MASK;
+
 	/* Check for the type of error interrupt and Processing it */
-	isr_errors = isr & (XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
-			    XCAN_IXR_BSOFF_MASK | XCAN_IXR_ARBLST_MASK |
-			    XCAN_IXR_RXMNF_MASK);
+	isr_errors = isr & mask;
+
 	if (isr_errors) {
 		priv->write_reg(priv, XCAN_ICR_OFFSET, isr_errors);
 		xcan_err_interrupt(ndev, isr);
@@ -1783,6 +1876,7 @@ static int xcan_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv = netdev_priv(ndev);
+	priv->ecc_enable = of_property_read_bool(pdev->dev.of_node, "xlnx,has-ecc");
 	priv->dev = &pdev->dev;
 	priv->can.bittiming_const = devtype->bittiming_const;
 	priv->can.do_set_mode = xcan_do_set_mode;
@@ -1880,6 +1974,11 @@ static int xcan_probe(struct platform_device *pdev)
 		   priv->reg_base, ndev->irq, priv->can.clock.freq,
 		   hw_tx_max, priv->tx_max);
 
+	if (priv->ecc_enable)
+		/* Reset FIFO ECC counters */
+		priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, XCAN_ECC_CFG_REECRX_MASK |
+			XCAN_ECC_CFG_REECTXOL_MASK | XCAN_ECC_CFG_REECTXTL_MASK);
+
 	return 0;
 
 err_disableclks:
-- 
2.1.1

[PATCH 3/3] can: xilinx_can: Add debugfs support for ECC

[Srinivas Goud]

Create debugfs entry for reading all the FIFO ECC errors.

Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
---
 drivers/net/can/xilinx_can.c | 62 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 311e435..f7bf31a 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -29,6 +29,7 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 #include <linux/pm_runtime.h>
+#include <linux/debugfs.h>
 
 #define DRIVER_NAME	"xilinx_can"
 
@@ -183,6 +184,9 @@ enum xcan_reg {
 #define XCAN_FLAG_RX_FIFO_MULTI	0x0010
 #define XCAN_FLAG_CANFD_2	0x0020
 
+/* ECC counters buffer length */
+#define XCAN_ECC_CNT_BUF_LEN	100
+
 enum xcan_ip_type {
 	XAXI_CAN = 0,
 	XZYNQ_CANPS,
@@ -222,6 +226,7 @@ struct xcan_devtype_data {
  * @ecc_1bit_txolfifo_cnt:	TXOLFIFO 1bit ECC count
  * @ecc_2bit_txtlfifo_cnt:	TXTLFIFO 2bit ECC count
  * @ecc_1bit_txtlfifo_cnt:	TXTLFIFO 1bit ECC count
+ * @debugfs:			Directory entry for debugfs
  */
 struct xcan_priv {
 	struct can_priv can;
@@ -246,6 +251,7 @@ struct xcan_priv {
 	u32 ecc_1bit_txolfifo_cnt;
 	u32 ecc_2bit_txtlfifo_cnt;
 	u32 ecc_1bit_txtlfifo_cnt;
+	struct dentry *debugfs;
 };
 
 /* CAN Bittiming constants as per Xilinx CAN specs */
@@ -1736,6 +1742,56 @@ static int __maybe_unused xcan_runtime_resume(struct device *dev)
 	return 0;
 }
 
+static ssize_t read_ecc_cnt_status(struct file *file, char __user *user_buf,
+				   size_t count, loff_t *ppos)
+{
+	unsigned int len = 0, buf_len = XCAN_ECC_CNT_BUF_LEN;
+	struct net_device *ndev = file->private_data;
+	struct xcan_priv *priv = netdev_priv(ndev);
+	ssize_t ret_cnt;
+	char *buf;
+
+	buf = kzalloc(buf_len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	len = scnprintf(buf + len, buf_len - len,
+			"%d\n", priv->ecc_2bit_rxfifo_cnt);
+	len += scnprintf(buf + len, buf_len - len,
+			"%d\n", priv->ecc_1bit_rxfifo_cnt);
+	len += scnprintf(buf + len, buf_len - len,
+			"%d\n", priv->ecc_2bit_txolfifo_cnt);
+	len += scnprintf(buf + len, buf_len - len,
+			"%d\n", priv->ecc_1bit_txolfifo_cnt);
+	len += scnprintf(buf + len, buf_len - len,
+			"%d\n", priv->ecc_2bit_txtlfifo_cnt);
+	len += scnprintf(buf + len, buf_len - len,
+			"%d\n", priv->ecc_1bit_txtlfifo_cnt);
+	ret_cnt = simple_read_from_buffer(user_buf, count, ppos, buf, len);
+
+	kfree(buf);
+
+	return ret_cnt;
+}
+
+static const struct file_operations read_ecc_fops = {
+	.open = simple_open,
+	.read = read_ecc_cnt_status,
+	.llseek = generic_file_llseek,
+};
+
+static void setup_debugfs(struct net_device *ndev)
+{
+	struct xcan_priv *priv = netdev_priv(ndev);
+
+	priv->debugfs = debugfs_create_dir(dev_name(priv->dev), NULL);
+	if (!priv->debugfs)
+		return;
+
+	debugfs_create_file("read_ecc_cnt", 0644, priv->debugfs,
+			    ndev, &read_ecc_fops);
+}
+
 static const struct dev_pm_ops xcan_dev_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
 	SET_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
@@ -1974,10 +2030,12 @@ static int xcan_probe(struct platform_device *pdev)
 		   priv->reg_base, ndev->irq, priv->can.clock.freq,
 		   hw_tx_max, priv->tx_max);
 
-	if (priv->ecc_enable)
+	if (priv->ecc_enable) {
+		setup_debugfs(ndev);
 		/* Reset FIFO ECC counters */
 		priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, XCAN_ECC_CFG_REECRX_MASK |
 			XCAN_ECC_CFG_REECTXOL_MASK | XCAN_ECC_CFG_REECTXTL_MASK);
+	}
 
 	return 0;
 
@@ -2000,7 +2058,9 @@ static int xcan_probe(struct platform_device *pdev)
 static int xcan_remove(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct xcan_priv *priv = netdev_priv(ndev);
 
+	debugfs_remove_recursive(priv->debugfs);
 	unregister_candev(ndev);
 	pm_runtime_disable(&pdev->dev);
 	free_candev(ndev);
-- 
2.1.1

Re: [PATCH 3/3] can: xilinx_can: Add debugfs support for ECC

[Marc Kleine-Budde]

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

On 12.06.2023 17:12:57, Srinivas Goud wrote:
> Create debugfs entry for reading all the FIFO ECC errors.

Please attach the stats to the ethtool stats interface:
| https://elixir.bootlin.com/linux/v6.3/source/include/linux/ethtool.h#L826

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’

[Marc Kleine-Budde]

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

On 12.06.2023 17:12:55, Srinivas Goud wrote:
> ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller.
> Part of this feature configuration and counter registers
> added in IP for 1bit/2bit ECC errors.
> Please find more details in PG096 v5.1 document.
> 
> xlnx,has-ecc is optional property and added to Xilinx CAN
> Controller node if ECC block enabled in the HW.
> 
> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>

Is there a way to introspect the IP core to check if this feature is
compiled in?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’

[Krzysztof Kozlowski]

On 12/06/2023 13:42, Srinivas Goud wrote:
> ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller.
> Part of this feature configuration and counter registers
> added in IP for 1bit/2bit ECC errors.
> Please find more details in PG096 v5.1 document.
> 
> xlnx,has-ecc is optional property and added to Xilinx CAN
> Controller node if ECC block enabled in the HW.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least DT list (maybe more), so this won't be tested by our
tools. Performing review on untested code might be a waste of time, thus
I will skip this patch entirely till you follow the process allowing the
patch to be tested.

Please kindly resend and include all necessary To/Cc entries.

Best regards,
Krzysztof

Re: [PATCH 2/3] can: xilinx_can: Add ECC support

[Marc Kleine-Budde]

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

On 12.06.2023 17:12:56, Srinivas Goud wrote:
> Add ECC support for Xilinx CAN Controller, so this driver reports
> 1bit/2bit ECC errors for FIFO's based on ECC error interrupt.
> ECC feature for Xilinx CAN Controller selected through
> 'xlnx,has-ecc' DT property
> 
> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
> ---
>  drivers/net/can/xilinx_can.c | 109 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 104 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 43c812e..311e435 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -63,6 +63,12 @@ enum xcan_reg {
>  	XCAN_RXMSG_2_BASE_OFFSET	= 0x2100, /* RX Message Space */
>  	XCAN_AFR_2_MASK_OFFSET	= 0x0A00, /* Acceptance Filter MASK */
>  	XCAN_AFR_2_ID_OFFSET	= 0x0A04, /* Acceptance Filter ID */
> +
> +	/* only on AXI CAN cores */
> +	XCAN_ECC_CFG_OFFSET	= 0xC8,	/* ECC Configuration */
> +	XCAN_TXTLFIFO_ECC_OFFSET	= 0xCC, /* TXTL FIFO ECC error counter */
> +	XCAN_TXOLFIFO_ECC_OFFSET	= 0xD0, /* TXOL FIFO ECC error counter */
> +	XCAN_RXFIFO_ECC_OFFSET		= 0xD4, /* RX FIFO ECC error counter */
>  };
>  
>  #define XCAN_FRAME_ID_OFFSET(frame_base)	((frame_base) + 0x00)
> @@ -135,6 +141,17 @@ enum xcan_reg {
>  #define XCAN_2_FSR_RI_MASK		0x0000003F /* RX Read Index */
>  #define XCAN_DLCR_EDL_MASK		0x08000000 /* EDL Mask in DLC */
>  #define XCAN_DLCR_BRS_MASK		0x04000000 /* BRS Mask in DLC */
> +#define XCAN_IXR_E2BERX_MASK		BIT(23) /* RX FIFO two bit ECC error */
> +#define XCAN_IXR_E1BERX_MASK		BIT(22) /* RX FIFO one bit ECC error */
> +#define XCAN_IXR_E2BETXOL_MASK		BIT(21) /* TXOL FIFO two bit ECC error */
> +#define XCAN_IXR_E1BETXOL_MASK		BIT(20) /* TXOL FIFO One bit ECC error */
> +#define XCAN_IXR_E2BETXTL_MASK		BIT(19) /* TXTL FIFO Two bit ECC error */
> +#define XCAN_IXR_E1BETXTL_MASK		BIT(18) /* TXTL FIFO One bit ECC error */
> +#define XCAN_ECC_CFG_REECRX_MASK	BIT(2) /* Reset RX FIFO ECC error counters */
> +#define XCAN_ECC_CFG_REECTXOL_MASK	BIT(1) /* Reset TXOL FIFO ECC error counters */
> +#define XCAN_ECC_CFG_REECTXTL_MASK	BIT(0) /* Reset TXTL FIFO ECC error counters */
> +#define XCAN_ECC_1BIT_CNT_MASK		GENMASK(15, 0) /* FIFO ECC 1bit count mask */
> +#define XCAN_ECC_2BIT_CNT_MASK		GENMASK(31, 16) /* FIFO ECC 2bit count mask */
>  
>  /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */
>  #define XCAN_BRPR_TDC_ENABLE		BIT(16) /* Transmitter Delay Compensation (TDC) Enable */
> @@ -198,6 +215,13 @@ struct xcan_devtype_data {
>   * @bus_clk:			Pointer to struct clk
>   * @can_clk:			Pointer to struct clk
>   * @devtype:			Device type specific constants
> + * @ecc_enable:			ECC enable flag
> + * @ecc_2bit_rxfifo_cnt:	RXFIFO 2bit ECC count
> + * @ecc_1bit_rxfifo_cnt:	RXFIFO 1bit ECC count
> + * @ecc_2bit_txolfifo_cnt:	TXOLFIFO 2bit ECC count
> + * @ecc_1bit_txolfifo_cnt:	TXOLFIFO 1bit ECC count
> + * @ecc_2bit_txtlfifo_cnt:	TXTLFIFO 2bit ECC count
> + * @ecc_1bit_txtlfifo_cnt:	TXTLFIFO 1bit ECC count
>   */
>  struct xcan_priv {
>  	struct can_priv can;
> @@ -215,6 +239,13 @@ struct xcan_priv {
>  	struct clk *bus_clk;
>  	struct clk *can_clk;
>  	struct xcan_devtype_data devtype;
> +	bool ecc_enable;
> +	u32 ecc_2bit_rxfifo_cnt;
> +	u32 ecc_1bit_rxfifo_cnt;
> +	u32 ecc_2bit_txolfifo_cnt;
> +	u32 ecc_1bit_txolfifo_cnt;
> +	u32 ecc_2bit_txtlfifo_cnt;
> +	u32 ecc_1bit_txtlfifo_cnt;
>  };
>  
>  /* CAN Bittiming constants as per Xilinx CAN specs */
> @@ -517,6 +548,11 @@ static int xcan_chip_start(struct net_device *ndev)
>  		XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
>  		XCAN_IXR_ARBLST_MASK | xcan_rx_int_mask(priv);
>  
> +	if (priv->ecc_enable)
> +		ier |= XCAN_IXR_E2BERX_MASK | XCAN_IXR_E1BERX_MASK |
> +			XCAN_IXR_E2BETXOL_MASK | XCAN_IXR_E1BETXOL_MASK |
> +			XCAN_IXR_E2BETXTL_MASK | XCAN_IXR_E1BETXTL_MASK;
> +
>  	if (priv->devtype.flags & XCAN_FLAG_RXMNF)
>  		ier |= XCAN_IXR_RXMNF_MASK;
>  
> @@ -1121,6 +1157,56 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
>  		priv->can.can_stats.bus_error++;
>  	}
>  
> +	if (priv->ecc_enable) {
> +		u32 reg_ecc;
> +
> +		reg_ecc = priv->read_reg(priv, XCAN_RXFIFO_ECC_OFFSET);
> +		if (isr & XCAN_IXR_E2BERX_MASK) {
> +			priv->ecc_2bit_rxfifo_cnt +=
> +				FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, reg_ecc);
> +			netdev_dbg(ndev, "%s: RX FIFO 2bit ECC error count %d\n",
> +				   __func__, priv->ecc_2bit_rxfifo_cnt);
> +		}
> +		if (isr & XCAN_IXR_E1BERX_MASK) {
> +			priv->ecc_1bit_rxfifo_cnt += reg_ecc &
> +				XCAN_ECC_1BIT_CNT_MASK;

Please use FIELD_GET here, too.

> +			netdev_dbg(ndev, "%s: RX FIFO 1bit ECC error count %d\n",
> +				   __func__, priv->ecc_1bit_rxfifo_cnt);
> +		}
> +
> +		reg_ecc = priv->read_reg(priv, XCAN_TXOLFIFO_ECC_OFFSET);
> +		if (isr & XCAN_IXR_E2BETXOL_MASK) {
> +			priv->ecc_2bit_txolfifo_cnt +=
> +				FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, reg_ecc);
> +			netdev_dbg(ndev, "%s: TXOL FIFO 2bit ECC error count %d\n",
> +				   __func__, priv->ecc_2bit_txolfifo_cnt);
> +		}
> +		if (isr & XCAN_IXR_E1BETXOL_MASK) {
> +			priv->ecc_1bit_txolfifo_cnt += reg_ecc &
> +				XCAN_ECC_1BIT_CNT_MASK;

same here

> +			netdev_dbg(ndev, "%s: TXOL FIFO 1bit ECC error count %d\n",
> +				   __func__, priv->ecc_1bit_txolfifo_cnt);
> +		}
> +
> +		reg_ecc = priv->read_reg(priv, XCAN_TXTLFIFO_ECC_OFFSET);
> +		if (isr & XCAN_IXR_E2BETXTL_MASK) {
> +			priv->ecc_2bit_txtlfifo_cnt +=
> +				FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, reg_ecc);
> +			netdev_dbg(ndev, "%s: TXTL FIFO 2bit ECC error count %d\n",
> +				   __func__, priv->ecc_2bit_txtlfifo_cnt);
> +		}
> +		if (isr & XCAN_IXR_E1BETXTL_MASK) {
> +			priv->ecc_1bit_txtlfifo_cnt += reg_ecc &
> +				XCAN_ECC_1BIT_CNT_MASK;

same here

> +			netdev_dbg(ndev, "%s: TXTL FIFO 1bit ECC error count %d\n",
> +				   __func__, priv->ecc_1bit_txtlfifo_cnt);
> +		}
> +
> +		/* Reset FIFO ECC counters */
> +		priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, XCAN_ECC_CFG_REECRX_MASK |
> +				XCAN_ECC_CFG_REECTXOL_MASK | XCAN_ECC_CFG_REECTXTL_MASK);

This is racy - you will lose events that occur between reading the
register value and clearing the register. You can save the old value and
add the difference between the new and the old value to the total
counter. What happens when the counter overflows? The following
pseudocode should handle the u16 counter rolling over to 0:

    u16 old, new;
    s16 inc;
    u64 total;

    ...

    inc = (s16)(new - old);
    if (inc < 0)
        /* error handling */
    total += inc;

BTW: When converting to ethtool statistics, a lock is required to keep
the u64 values correct on 32-bit systems and the individual statistics
consistent.

> +	}
> +
>  	if (cf.can_id) {
>  		struct can_frame *skb_cf;
>  		struct sk_buff *skb = alloc_can_err_skb(ndev, &skb_cf);
> @@ -1348,9 +1434,8 @@ static irqreturn_t xcan_interrupt(int irq, void *dev_id)
>  {
>  	struct net_device *ndev = (struct net_device *)dev_id;
>  	struct xcan_priv *priv = netdev_priv(ndev);
> -	u32 isr, ier;
> -	u32 isr_errors;
>  	u32 rx_int_mask = xcan_rx_int_mask(priv);
> +	u32 isr, ier, isr_errors, mask;
>  
>  	/* Get the interrupt status from Xilinx CAN */
>  	isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> @@ -1368,10 +1453,18 @@ static irqreturn_t xcan_interrupt(int irq, void *dev_id)
>  	if (isr & XCAN_IXR_TXOK_MASK)
>  		xcan_tx_interrupt(ndev, isr);
>  
> +	mask = XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
> +		XCAN_IXR_BSOFF_MASK | XCAN_IXR_ARBLST_MASK |
> +		XCAN_IXR_RXMNF_MASK;
> +
> +	if (priv->ecc_enable)
> +		mask |= XCAN_IXR_E2BERX_MASK | XCAN_IXR_E1BERX_MASK |
> +			XCAN_IXR_E2BETXOL_MASK | XCAN_IXR_E1BETXOL_MASK |
> +			XCAN_IXR_E2BETXTL_MASK | XCAN_IXR_E1BETXTL_MASK;
> +
>  	/* Check for the type of error interrupt and Processing it */
> -	isr_errors = isr & (XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
> -			    XCAN_IXR_BSOFF_MASK | XCAN_IXR_ARBLST_MASK |
> -			    XCAN_IXR_RXMNF_MASK);
> +	isr_errors = isr & mask;
> +

nitpick: don't add this extra newline

>  	if (isr_errors) {
>  		priv->write_reg(priv, XCAN_ICR_OFFSET, isr_errors);
>  		xcan_err_interrupt(ndev, isr);
> @@ -1783,6 +1876,7 @@ static int xcan_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	priv = netdev_priv(ndev);
> +	priv->ecc_enable = of_property_read_bool(pdev->dev.of_node, "xlnx,has-ecc");
>  	priv->dev = &pdev->dev;
>  	priv->can.bittiming_const = devtype->bittiming_const;
>  	priv->can.do_set_mode = xcan_do_set_mode;
> @@ -1880,6 +1974,11 @@ static int xcan_probe(struct platform_device *pdev)
>  		   priv->reg_base, ndev->irq, priv->can.clock.freq,
>  		   hw_tx_max, priv->tx_max);
>  
> +	if (priv->ecc_enable)
> +		/* Reset FIFO ECC counters */
> +		priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, XCAN_ECC_CFG_REECRX_MASK |
> +			XCAN_ECC_CFG_REECTXOL_MASK | XCAN_ECC_CFG_REECTXTL_MASK);
> +
>  	return 0;
>  
>  err_disableclks:
> -- 
> 2.1.1
> 
> 

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

RE: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’

[Goud, Srinivas]

Hi,

>-----Original Message-----
>From: Marc Kleine-Budde <mkl@pengutronix.de>
>Sent: Tuesday, June 13, 2023 1:23 PM
>To: Goud, Srinivas <srinivas.goud@amd.com>
>Cc: wg@grandegger.com; davem@davemloft.net; edumazet@google.com;
>kuba@kernel.org; pabeni@redhat.com; gcnu.goud@gmail.com; git (AMD-
>Xilinx) <git@amd.com>; michal.simek@xilinx.com; linux-can@vger.kernel.org;
>linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property
>‘xlnx,has-ecc’
>
>On 12.06.2023 17:12:55, Srinivas Goud wrote:
>> ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller.
>> Part of this feature configuration and counter registers added in IP
>> for 1bit/2bit ECC errors.
>> Please find more details in PG096 v5.1 document.
>>
>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller
>> node if ECC block enabled in the HW.
>>
>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
>
>Is there a way to introspect the IP core to check if this feature is compiled in?
There is no way(IP registers) to indicate whether ECC feature is enabled or not.

>
>Marc
>
>--
>Pengutronix e.K.                 | Marc Kleine-Budde          |
>Embedded Linux                   | https://www.pengutronix.de |
>Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
>Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Thanks,
Srinivas

Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’

[Krzysztof Kozlowski]

On 14/06/2023 12:22, Goud, Srinivas wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Marc Kleine-Budde <mkl@pengutronix.de>
>> Sent: Tuesday, June 13, 2023 1:23 PM
>> To: Goud, Srinivas <srinivas.goud@amd.com>
>> Cc: wg@grandegger.com; davem@davemloft.net; edumazet@google.com;
>> kuba@kernel.org; pabeni@redhat.com; gcnu.goud@gmail.com; git (AMD-
>> Xilinx) <git@amd.com>; michal.simek@xilinx.com; linux-can@vger.kernel.org;
>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property
>> ‘xlnx,has-ecc’
>>
>> On 12.06.2023 17:12:55, Srinivas Goud wrote:
>>> ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller.
>>> Part of this feature configuration and counter registers added in IP
>>> for 1bit/2bit ECC errors.
>>> Please find more details in PG096 v5.1 document.
>>>
>>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller
>>> node if ECC block enabled in the HW.
>>>
>>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
>>
>> Is there a way to introspect the IP core to check if this feature is compiled in?
> There is no way(IP registers) to indicate whether ECC feature is enabled or not.

Isn't this then deductible from compatible? Your binding claims it is
only for AXI CAN, so xlnx,axi-can-1.00.a, even though you did not
restrict it in the binding.

Best regards,
Krzysztof

RE: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’

[Goud, Srinivas]

Hi

>-----Original Message-----
>From: Krzysztof Kozlowski <krzk@kernel.org>
>Sent: Wednesday, June 14, 2023 4:41 PM
>To: Goud, Srinivas <srinivas.goud@amd.com>; Marc Kleine-Budde
><mkl@pengutronix.de>
>Cc: wg@grandegger.com; davem@davemloft.net; edumazet@google.com;
>kuba@kernel.org; pabeni@redhat.com; gcnu.goud@gmail.com; git (AMD-
>Xilinx) <git@amd.com>; michal.simek@xilinx.com; linux-can@vger.kernel.org;
>linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property
>‘xlnx,has-ecc’
>
>On 14/06/2023 12:22, Goud, Srinivas wrote:
>> Hi,
>>
>>> -----Original Message-----
>>> From: Marc Kleine-Budde <mkl@pengutronix.de>
>>> Sent: Tuesday, June 13, 2023 1:23 PM
>>> To: Goud, Srinivas <srinivas.goud@amd.com>
>>> Cc: wg@grandegger.com; davem@davemloft.net; edumazet@google.com;
>>> kuba@kernel.org; pabeni@redhat.com; gcnu.goud@gmail.com; git (AMD-
>>> Xilinx) <git@amd.com>; michal.simek@xilinx.com;
>>> linux-can@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>> linux-kernel@vger.kernel.org
>>> Subject: Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC
>>> property ‘xlnx,has-ecc’
>>>
>>> On 12.06.2023 17:12:55, Srinivas Goud wrote:
>>>> ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller.
>>>> Part of this feature configuration and counter registers added in IP
>>>> for 1bit/2bit ECC errors.
>>>> Please find more details in PG096 v5.1 document.
>>>>
>>>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller
>>>> node if ECC block enabled in the HW.
>>>>
>>>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
>>>
>>> Is there a way to introspect the IP core to check if this feature is compiled in?
>> There is no way(IP registers) to indicate whether ECC feature is enabled or
>not.
>
>Isn't this then deductible from compatible? Your binding claims it is only for
>AXI CAN, so xlnx,axi-can-1.00.a, even though you did not restrict it in the
>binding.
Agree it is only for AXI CAN(xlnx,axi-can-1.00.a) but ECC feature is
configurable option to the user.
ECC is added as optional configuration(enable/disable) feature 
to the existing AXI CAN.

Thanks,
Srinivas

Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’

[Krzysztof Kozlowski]

On 16/06/2023 12:13, Goud, Srinivas wrote:
>>>>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller
>>>>> node if ECC block enabled in the HW.
>>>>>
>>>>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
>>>>
>>>> Is there a way to introspect the IP core to check if this feature is compiled in?
>>> There is no way(IP registers) to indicate whether ECC feature is enabled or
>> not.
>>
>> Isn't this then deductible from compatible? Your binding claims it is only for
>> AXI CAN, so xlnx,axi-can-1.00.a, even though you did not restrict it in the
>> binding.
> Agree it is only for AXI CAN(xlnx,axi-can-1.00.a) but ECC feature is
> configurable option to the user.
> ECC is added as optional configuration(enable/disable) feature 
> to the existing AXI CAN.

Why boards would like not to have ECC? I understand that someone told
you "make it configurable in DTS", but that's not really a reason for
us. Why this is suitable for DTS?

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’

[Michal Simek]

On 6/16/23 12:38, Krzysztof Kozlowski wrote:
> On 16/06/2023 12:13, Goud, Srinivas wrote:
>>>>>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller
>>>>>> node if ECC block enabled in the HW.
>>>>>>
>>>>>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
>>>>>
>>>>> Is there a way to introspect the IP core to check if this feature is compiled in?
>>>> There is no way(IP registers) to indicate whether ECC feature is enabled or
>>> not.
>>>
>>> Isn't this then deductible from compatible? Your binding claims it is only for
>>> AXI CAN, so xlnx,axi-can-1.00.a, even though you did not restrict it in the
>>> binding.
>> Agree it is only for AXI CAN(xlnx,axi-can-1.00.a) but ECC feature is
>> configurable option to the user.
>> ECC is added as optional configuration(enable/disable) feature
>> to the existing AXI CAN.
> 
> Why boards would like not to have ECC? I understand that someone told
> you "make it configurable in DTS", but that's not really a reason for
> us. Why this is suitable for DTS?

Let me jump to this. This is core for programmable logic where HW designers of 
this IP added couple of feature which can be enabled or disable based on 
customer need. It means it is not SW option if ECC is enable but it is HW choice 
if ECC is present in the HW or not.
Selection if ECC should be used is up to every customer to decide.
We are not able to get information why customers choosing ECC enabled/disabled 
but I can imagine that with ECC disable less fpga resources are used.

Thanks,
Michal

Re: [PATCH 2/3] can: xilinx_can: Add ECC support

[Marc Kleine-Budde]

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

On 12.06.2023 17:12:56, Srinivas Goud wrote:
> Add ECC support for Xilinx CAN Controller, so this driver reports
> 1bit/2bit ECC errors for FIFO's based on ECC error interrupt.
> ECC feature for Xilinx CAN Controller selected through
> 'xlnx,has-ecc' DT property
> 
> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
> ---
>  drivers/net/can/xilinx_can.c | 109 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 104 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 43c812e..311e435 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -63,6 +63,12 @@ enum xcan_reg {
>  	XCAN_RXMSG_2_BASE_OFFSET	= 0x2100, /* RX Message Space */
>  	XCAN_AFR_2_MASK_OFFSET	= 0x0A00, /* Acceptance Filter MASK */
>  	XCAN_AFR_2_ID_OFFSET	= 0x0A04, /* Acceptance Filter ID */
> +
> +	/* only on AXI CAN cores */
> +	XCAN_ECC_CFG_OFFSET	= 0xC8,	/* ECC Configuration */
> +	XCAN_TXTLFIFO_ECC_OFFSET	= 0xCC, /* TXTL FIFO ECC error counter */
> +	XCAN_TXOLFIFO_ECC_OFFSET	= 0xD0, /* TXOL FIFO ECC error counter */
> +	XCAN_RXFIFO_ECC_OFFSET		= 0xD4, /* RX FIFO ECC error counter */

Please keep the enum sorted by address.

>  };
>  
>  #define XCAN_FRAME_ID_OFFSET(frame_base)	((frame_base) + 0x00)
> @@ -135,6 +141,17 @@ enum xcan_reg {
>  #define XCAN_2_FSR_RI_MASK		0x0000003F /* RX Read Index */
>  #define XCAN_DLCR_EDL_MASK		0x08000000 /* EDL Mask in DLC */
>  #define XCAN_DLCR_BRS_MASK		0x04000000 /* BRS Mask in DLC */
> +#define XCAN_IXR_E2BERX_MASK		BIT(23) /* RX FIFO two bit ECC error */
> +#define XCAN_IXR_E1BERX_MASK		BIT(22) /* RX FIFO one bit ECC error */
> +#define XCAN_IXR_E2BETXOL_MASK		BIT(21) /* TXOL FIFO two bit ECC error */
> +#define XCAN_IXR_E1BETXOL_MASK		BIT(20) /* TXOL FIFO One bit ECC error */
> +#define XCAN_IXR_E2BETXTL_MASK		BIT(19) /* TXTL FIFO Two bit ECC error */
> +#define XCAN_IXR_E1BETXTL_MASK		BIT(18) /* TXTL FIFO One bit ECC error */

Please move the XCAN_IXR next to the other XCAN_IXR.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH 0/3] can: xilinx_can: Add ECC feature support

[Marc Kleine-Budde]

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

On 12.06.2023 17:12:54, Srinivas Goud wrote:
> ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller.
> Part of this feature configuration and counter registers added 
> in Xilinx CAN Controller for 1bit/2bit ECC errors count and reset.
> Please find more details in PG096 v5.1 document.

The document "PG096 (v5.1) May 16, 2023 CAN v5.1" [1] lists the
XCAN_ECC_CFG_OFFSET as reserved, although it has a section "ECC
Configuration Register".

[1] https://docs.xilinx.com/viewer/book-attachment/Bv6XZP9HRonCGi58fl10dw/ch1ZLpOt4UKWNub7DXjJ7Q

The other registers (XCAN_TXTLFIFO_ECC_OFFSET, XCAN_TXOLFIFO_ECC_OFFSET,
XCAN_TXOLFIFO_ECC_OFFSET) are also listed as reserved and not even
mentioned on the document. Am I missing something?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’

[Krzysztof Kozlowski]

On 16/06/2023 12:44, Michal Simek wrote:
> 
> 
> On 6/16/23 12:38, Krzysztof Kozlowski wrote:
>> On 16/06/2023 12:13, Goud, Srinivas wrote:
>>>>>>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller
>>>>>>> node if ECC block enabled in the HW.
>>>>>>>
>>>>>>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
>>>>>>
>>>>>> Is there a way to introspect the IP core to check if this feature is compiled in?
>>>>> There is no way(IP registers) to indicate whether ECC feature is enabled or
>>>> not.
>>>>
>>>> Isn't this then deductible from compatible? Your binding claims it is only for
>>>> AXI CAN, so xlnx,axi-can-1.00.a, even though you did not restrict it in the
>>>> binding.
>>> Agree it is only for AXI CAN(xlnx,axi-can-1.00.a) but ECC feature is
>>> configurable option to the user.
>>> ECC is added as optional configuration(enable/disable) feature
>>> to the existing AXI CAN.
>>
>> Why boards would like not to have ECC? I understand that someone told
>> you "make it configurable in DTS", but that's not really a reason for
>> us. Why this is suitable for DTS?
> 
> Let me jump to this. This is core for programmable logic where HW designers of 
> this IP added couple of feature which can be enabled or disable based on 
> customer need. It means it is not SW option if ECC is enable but it is HW choice 
> if ECC is present in the HW or not.
> Selection if ECC should be used is up to every customer to decide.
> We are not able to get information why customers choosing ECC enabled/disabled 
> but I can imagine that with ECC disable less fpga resources are used.

Thanks for the explanation. Apologies for being picky, but you are in
minority when adding such properties with true hardware meaning. Most of
the submissions of such properties add them to control the bits in register.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’

[Krzysztof Kozlowski]

On 14/06/2023 13:11, Krzysztof Kozlowski wrote:
> On 14/06/2023 12:22, Goud, Srinivas wrote:
>> Hi,
>>
>>> -----Original Message-----
>>> From: Marc Kleine-Budde <mkl@pengutronix.de>
>>> Sent: Tuesday, June 13, 2023 1:23 PM
>>> To: Goud, Srinivas <srinivas.goud@amd.com>
>>> Cc: wg@grandegger.com; davem@davemloft.net; edumazet@google.com;
>>> kuba@kernel.org; pabeni@redhat.com; gcnu.goud@gmail.com; git (AMD-
>>> Xilinx) <git@amd.com>; michal.simek@xilinx.com; linux-can@vger.kernel.org;
>>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>>> Subject: Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property
>>> ‘xlnx,has-ecc’
>>>
>>> On 12.06.2023 17:12:55, Srinivas Goud wrote:
>>>> ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller.
>>>> Part of this feature configuration and counter registers added in IP
>>>> for 1bit/2bit ECC errors.
>>>> Please find more details in PG096 v5.1 document.
>>>>
>>>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller
>>>> node if ECC block enabled in the HW.
>>>>
>>>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
>>>
>>> Is there a way to introspect the IP core to check if this feature is compiled in?
>> There is no way(IP registers) to indicate whether ECC feature is enabled or not.
> 
> Isn't this then deductible from compatible? Your binding claims it is
> only for AXI CAN, so xlnx,axi-can-1.00.a, even though you did not
> restrict it in the binding.

BTW, this is not an ACK, because this was not tested by automation. I
don't understand why get_maintainers.pl are so tricky to use, but
nevertheless I require resend to satisfy automation.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’

[Michal Simek]

On 6/17/23 09:31, Krzysztof Kozlowski wrote:
> On 16/06/2023 12:44, Michal Simek wrote:
>>
>>
>> On 6/16/23 12:38, Krzysztof Kozlowski wrote:
>>> On 16/06/2023 12:13, Goud, Srinivas wrote:
>>>>>>>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller
>>>>>>>> node if ECC block enabled in the HW.
>>>>>>>>
>>>>>>>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
>>>>>>>
>>>>>>> Is there a way to introspect the IP core to check if this feature is compiled in?
>>>>>> There is no way(IP registers) to indicate whether ECC feature is enabled or
>>>>> not.
>>>>>
>>>>> Isn't this then deductible from compatible? Your binding claims it is only for
>>>>> AXI CAN, so xlnx,axi-can-1.00.a, even though you did not restrict it in the
>>>>> binding.
>>>> Agree it is only for AXI CAN(xlnx,axi-can-1.00.a) but ECC feature is
>>>> configurable option to the user.
>>>> ECC is added as optional configuration(enable/disable) feature
>>>> to the existing AXI CAN.
>>>
>>> Why boards would like not to have ECC? I understand that someone told
>>> you "make it configurable in DTS", but that's not really a reason for
>>> us. Why this is suitable for DTS?
>>
>> Let me jump to this. This is core for programmable logic where HW designers of
>> this IP added couple of feature which can be enabled or disable based on
>> customer need. It means it is not SW option if ECC is enable but it is HW choice
>> if ECC is present in the HW or not.
>> Selection if ECC should be used is up to every customer to decide.
>> We are not able to get information why customers choosing ECC enabled/disabled
>> but I can imagine that with ECC disable less fpga resources are used.
> 
> Thanks for the explanation. Apologies for being picky, but you are in
> minority when adding such properties with true hardware meaning. Most of
> the submissions of such properties add them to control the bits in register.

No issue at all. We are talking to HW designers to change their mindset and help 
us with automatic detection of these features but truth is that every such a 
feature means fpga resources that's why they are trying to avoid it to save them 
and help customers to fit as much as possible to their fpgas. Because bigger 
fpga is more expensive and also consumes more power.

Thanks,
Michal

Re: [PATCH 0/3] can: xilinx_can: Add ECC feature support

[Michal Simek]

Hi Marc,

On 6/16/23 13:12, Marc Kleine-Budde wrote:
> On 12.06.2023 17:12:54, Srinivas Goud wrote:
>> ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller.
>> Part of this feature configuration and counter registers added
>> in Xilinx CAN Controller for 1bit/2bit ECC errors count and reset.
>> Please find more details in PG096 v5.1 document.
> 
> The document "PG096 (v5.1) May 16, 2023 CAN v5.1" [1] lists the
> XCAN_ECC_CFG_OFFSET as reserved, although it has a section "ECC
> Configuration Register".
> 
> [1] https://docs.xilinx.com/viewer/book-attachment/Bv6XZP9HRonCGi58fl10dw/ch1ZLpOt4UKWNub7DXjJ7Q
> 
> The other registers (XCAN_TXTLFIFO_ECC_OFFSET, XCAN_TXOLFIFO_ECC_OFFSET,
> XCAN_TXOLFIFO_ECC_OFFSET) are also listed as reserved and not even
> mentioned on the document. Am I missing something?

We cross check available public documentation with HW team and there is no 
public documentation for this feature yet. We didn't get any exact day when 
documentation is going to be released.
Unfortunately it is not the first or even last time when this is happening but I 
still think is good to get this feature done properly till the time when 
documentation catch it up. Please let me know if you have any concern about it.

Thanks,
Michal

Re: [PATCH 0/3] can: xilinx_can: Add ECC feature support

[Marc Kleine-Budde]

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

On 23.06.2023 09:48:16, Michal Simek wrote:
> Hi Marc,
> 
> On 6/16/23 13:12, Marc Kleine-Budde wrote:
> > On 12.06.2023 17:12:54, Srinivas Goud wrote:
> > > ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller.
> > > Part of this feature configuration and counter registers added
> > > in Xilinx CAN Controller for 1bit/2bit ECC errors count and reset.
> > > Please find more details in PG096 v5.1 document.
> > 
> > The document "PG096 (v5.1) May 16, 2023 CAN v5.1" [1] lists the
> > XCAN_ECC_CFG_OFFSET as reserved, although it has a section "ECC
> > Configuration Register".
> > 
> > [1] https://docs.xilinx.com/viewer/book-attachment/Bv6XZP9HRonCGi58fl10dw/ch1ZLpOt4UKWNub7DXjJ7Q
> > 
> > The other registers (XCAN_TXTLFIFO_ECC_OFFSET, XCAN_TXOLFIFO_ECC_OFFSET,
> > XCAN_TXOLFIFO_ECC_OFFSET) are also listed as reserved and not even
> > mentioned on the document. Am I missing something?
> 
> We cross check available public documentation with HW team and there is no
> public documentation for this feature yet. We didn't get any exact day when
> documentation is going to be released.

It's a pity, but's that the way it's sometimes is.

> Unfortunately it is not the first or even last time when this is happening
> but I still think is good to get this feature done properly till the time
> when documentation catch it up. Please let me know if you have any concern
> about it.

No problem, update the cover letter and mention that the documentation
is not public available, yet. For reference we can keep the reference to
the internal doc (and mention that).

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

RE: [PATCH 2/3] can: xilinx_can: Add ECC support

[Goud, Srinivas]

Hi,

>-----Original Message-----
>From: Marc Kleine-Budde <mkl@pengutronix.de>
>Sent: Tuesday, June 13, 2023 5:00 PM
>To: Goud, Srinivas <srinivas.goud@amd.com>
>Cc: wg@grandegger.com; davem@davemloft.net; edumazet@google.com;
>kuba@kernel.org; pabeni@redhat.com; gcnu.goud@gmail.com; git (AMD-
>Xilinx) <git@amd.com>; michal.simek@xilinx.com; linux-can@vger.kernel.org;
>linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH 2/3] can: xilinx_can: Add ECC support
>
>On 12.06.2023 17:12:56, Srinivas Goud wrote:
>> Add ECC support for Xilinx CAN Controller, so this driver reports
>> 1bit/2bit ECC errors for FIFO's based on ECC error interrupt.
>> ECC feature for Xilinx CAN Controller selected through 'xlnx,has-ecc'
>> DT property
>>
>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
>> ---
>>  drivers/net/can/xilinx_can.c | 109
>> +++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 104 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/can/xilinx_can.c
>> b/drivers/net/can/xilinx_can.c index 43c812e..311e435 100644
>> --- a/drivers/net/can/xilinx_can.c
>> +++ b/drivers/net/can/xilinx_can.c
>> @@ -63,6 +63,12 @@ enum xcan_reg {
>>  	XCAN_RXMSG_2_BASE_OFFSET	= 0x2100, /* RX Message Space */
>>  	XCAN_AFR_2_MASK_OFFSET	= 0x0A00, /* Acceptance Filter MASK */
>>  	XCAN_AFR_2_ID_OFFSET	= 0x0A04, /* Acceptance Filter ID */
>> +
>> +	/* only on AXI CAN cores */
>> +	XCAN_ECC_CFG_OFFSET	= 0xC8,	/* ECC Configuration */
>> +	XCAN_TXTLFIFO_ECC_OFFSET	= 0xCC, /* TXTL FIFO ECC error counter
>*/
>> +	XCAN_TXOLFIFO_ECC_OFFSET	= 0xD0, /* TXOL FIFO ECC error counter
>*/
>> +	XCAN_RXFIFO_ECC_OFFSET		= 0xD4, /* RX FIFO ECC error
>counter */
>>  };
>>
>>  #define XCAN_FRAME_ID_OFFSET(frame_base)	((frame_base) + 0x00)
>> @@ -135,6 +141,17 @@ enum xcan_reg {
>>  #define XCAN_2_FSR_RI_MASK		0x0000003F /* RX Read Index
>*/
>>  #define XCAN_DLCR_EDL_MASK		0x08000000 /* EDL Mask in
>DLC */
>>  #define XCAN_DLCR_BRS_MASK		0x04000000 /* BRS Mask in
>DLC */
>> +#define XCAN_IXR_E2BERX_MASK		BIT(23) /* RX FIFO two bit ECC
>error */
>> +#define XCAN_IXR_E1BERX_MASK		BIT(22) /* RX FIFO one bit ECC
>error */
>> +#define XCAN_IXR_E2BETXOL_MASK		BIT(21) /* TXOL FIFO two bit
>ECC error */
>> +#define XCAN_IXR_E1BETXOL_MASK		BIT(20) /* TXOL FIFO One bit
>ECC error */
>> +#define XCAN_IXR_E2BETXTL_MASK		BIT(19) /* TXTL FIFO Two bit
>ECC error */
>> +#define XCAN_IXR_E1BETXTL_MASK		BIT(18) /* TXTL FIFO One bit
>ECC error */
>> +#define XCAN_ECC_CFG_REECRX_MASK	BIT(2) /* Reset RX FIFO ECC
>error counters */
>> +#define XCAN_ECC_CFG_REECTXOL_MASK	BIT(1) /* Reset TXOL FIFO ECC
>error counters */
>> +#define XCAN_ECC_CFG_REECTXTL_MASK	BIT(0) /* Reset TXTL FIFO ECC
>error counters */
>> +#define XCAN_ECC_1BIT_CNT_MASK		GENMASK(15, 0) /*
>FIFO ECC 1bit count mask */
>> +#define XCAN_ECC_2BIT_CNT_MASK		GENMASK(31, 16) /*
>FIFO ECC 2bit count mask */
>>
>>  /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */
>>  #define XCAN_BRPR_TDC_ENABLE		BIT(16) /* Transmitter Delay
>Compensation (TDC) Enable */
>> @@ -198,6 +215,13 @@ struct xcan_devtype_data {
>>   * @bus_clk:			Pointer to struct clk
>>   * @can_clk:			Pointer to struct clk
>>   * @devtype:			Device type specific constants
>> + * @ecc_enable:			ECC enable flag
>> + * @ecc_2bit_rxfifo_cnt:	RXFIFO 2bit ECC count
>> + * @ecc_1bit_rxfifo_cnt:	RXFIFO 1bit ECC count
>> + * @ecc_2bit_txolfifo_cnt:	TXOLFIFO 2bit ECC count
>> + * @ecc_1bit_txolfifo_cnt:	TXOLFIFO 1bit ECC count
>> + * @ecc_2bit_txtlfifo_cnt:	TXTLFIFO 2bit ECC count
>> + * @ecc_1bit_txtlfifo_cnt:	TXTLFIFO 1bit ECC count
>>   */
>>  struct xcan_priv {
>>  	struct can_priv can;
>> @@ -215,6 +239,13 @@ struct xcan_priv {
>>  	struct clk *bus_clk;
>>  	struct clk *can_clk;
>>  	struct xcan_devtype_data devtype;
>> +	bool ecc_enable;
>> +	u32 ecc_2bit_rxfifo_cnt;
>> +	u32 ecc_1bit_rxfifo_cnt;
>> +	u32 ecc_2bit_txolfifo_cnt;
>> +	u32 ecc_1bit_txolfifo_cnt;
>> +	u32 ecc_2bit_txtlfifo_cnt;
>> +	u32 ecc_1bit_txtlfifo_cnt;
>>  };
>>
>>  /* CAN Bittiming constants as per Xilinx CAN specs */ @@ -517,6
>> +548,11 @@ static int xcan_chip_start(struct net_device *ndev)
>>  		XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
>>  		XCAN_IXR_ARBLST_MASK | xcan_rx_int_mask(priv);
>>
>> +	if (priv->ecc_enable)
>> +		ier |= XCAN_IXR_E2BERX_MASK | XCAN_IXR_E1BERX_MASK |
>> +			XCAN_IXR_E2BETXOL_MASK |
>XCAN_IXR_E1BETXOL_MASK |
>> +			XCAN_IXR_E2BETXTL_MASK |
>XCAN_IXR_E1BETXTL_MASK;
>> +
>>  	if (priv->devtype.flags & XCAN_FLAG_RXMNF)
>>  		ier |= XCAN_IXR_RXMNF_MASK;
>>
>> @@ -1121,6 +1157,56 @@ static void xcan_err_interrupt(struct net_device
>*ndev, u32 isr)
>>  		priv->can.can_stats.bus_error++;
>>  	}
>>
>> +	if (priv->ecc_enable) {
>> +		u32 reg_ecc;
>> +
>> +		reg_ecc = priv->read_reg(priv, XCAN_RXFIFO_ECC_OFFSET);
>> +		if (isr & XCAN_IXR_E2BERX_MASK) {
>> +			priv->ecc_2bit_rxfifo_cnt +=
>> +				FIELD_GET(XCAN_ECC_2BIT_CNT_MASK,
>reg_ecc);
>> +			netdev_dbg(ndev, "%s: RX FIFO 2bit ECC error count
>%d\n",
>> +				   __func__, priv->ecc_2bit_rxfifo_cnt);
>> +		}
>> +		if (isr & XCAN_IXR_E1BERX_MASK) {
>> +			priv->ecc_1bit_rxfifo_cnt += reg_ecc &
>> +				XCAN_ECC_1BIT_CNT_MASK;
>
>Please use FIELD_GET here, too.
>
>> +			netdev_dbg(ndev, "%s: RX FIFO 1bit ECC error count
>%d\n",
>> +				   __func__, priv->ecc_1bit_rxfifo_cnt);
>> +		}
>> +
>> +		reg_ecc = priv->read_reg(priv, XCAN_TXOLFIFO_ECC_OFFSET);
>> +		if (isr & XCAN_IXR_E2BETXOL_MASK) {
>> +			priv->ecc_2bit_txolfifo_cnt +=
>> +				FIELD_GET(XCAN_ECC_2BIT_CNT_MASK,
>reg_ecc);
>> +			netdev_dbg(ndev, "%s: TXOL FIFO 2bit ECC error count
>%d\n",
>> +				   __func__, priv->ecc_2bit_txolfifo_cnt);
>> +		}
>> +		if (isr & XCAN_IXR_E1BETXOL_MASK) {
>> +			priv->ecc_1bit_txolfifo_cnt += reg_ecc &
>> +				XCAN_ECC_1BIT_CNT_MASK;
>
>same here
>
>> +			netdev_dbg(ndev, "%s: TXOL FIFO 1bit ECC error count
>%d\n",
>> +				   __func__, priv->ecc_1bit_txolfifo_cnt);
>> +		}
>> +
>> +		reg_ecc = priv->read_reg(priv, XCAN_TXTLFIFO_ECC_OFFSET);
>> +		if (isr & XCAN_IXR_E2BETXTL_MASK) {
>> +			priv->ecc_2bit_txtlfifo_cnt +=
>> +				FIELD_GET(XCAN_ECC_2BIT_CNT_MASK,
>reg_ecc);
>> +			netdev_dbg(ndev, "%s: TXTL FIFO 2bit ECC error count
>%d\n",
>> +				   __func__, priv->ecc_2bit_txtlfifo_cnt);
>> +		}
>> +		if (isr & XCAN_IXR_E1BETXTL_MASK) {
>> +			priv->ecc_1bit_txtlfifo_cnt += reg_ecc &
>> +				XCAN_ECC_1BIT_CNT_MASK;
>
>same here
>
>> +			netdev_dbg(ndev, "%s: TXTL FIFO 1bit ECC error count
>%d\n",
>> +				   __func__, priv->ecc_1bit_txtlfifo_cnt);
>> +		}
>> +
>> +		/* Reset FIFO ECC counters */
>> +		priv->write_reg(priv, XCAN_ECC_CFG_OFFSET,
>XCAN_ECC_CFG_REECRX_MASK |
>> +				XCAN_ECC_CFG_REECTXOL_MASK |
>XCAN_ECC_CFG_REECTXTL_MASK);
>
>This is racy - you will lose events that occur between reading the register value
>and clearing the register. You can save the old value and add the difference
>between the new and the old value to the total counter. What happens when
>the counter overflows? The following pseudocode should handle the u16
>counter rolling over to 0:
As per IP specifications when counter reaching maximum value of 0xFFFF will 
stays there until reset.

Not sure we can avoid this race condition completely, as we need to reset
the counters after reaching the 0xFFFF to avoid losing the events.

To minimize the race condition, we can reset the counters after reaching 
the events to 0xFFFF instead of resetting for every interrupt. 
Can you please suggest if we can go this approach.

Regards,
Srinivas
>
>    u16 old, new;
>    s16 inc;
>    u64 total;
>
>    ...
>
>    inc = (s16)(new - old);
>    if (inc < 0)
>        /* error handling */
>    total += inc;
>
>BTW: When converting to ethtool statistics, a lock is required to keep the u64
>values correct on 32-bit systems and the individual statistics consistent.
>
>> +	}
>> +
>>  	if (cf.can_id) {
>>  		struct can_frame *skb_cf;
>>  		struct sk_buff *skb = alloc_can_err_skb(ndev, &skb_cf); @@ -
>1348,9
>> +1434,8 @@ static irqreturn_t xcan_interrupt(int irq, void *dev_id)  {
>>  	struct net_device *ndev = (struct net_device *)dev_id;
>>  	struct xcan_priv *priv = netdev_priv(ndev);
>> -	u32 isr, ier;
>> -	u32 isr_errors;
>>  	u32 rx_int_mask = xcan_rx_int_mask(priv);
>> +	u32 isr, ier, isr_errors, mask;
>>
>>  	/* Get the interrupt status from Xilinx CAN */
>>  	isr = priv->read_reg(priv, XCAN_ISR_OFFSET); @@ -1368,10 +1453,18
>@@
>> static irqreturn_t xcan_interrupt(int irq, void *dev_id)
>>  	if (isr & XCAN_IXR_TXOK_MASK)
>>  		xcan_tx_interrupt(ndev, isr);
>>
>> +	mask = XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
>> +		XCAN_IXR_BSOFF_MASK | XCAN_IXR_ARBLST_MASK |
>> +		XCAN_IXR_RXMNF_MASK;
>> +
>> +	if (priv->ecc_enable)
>> +		mask |= XCAN_IXR_E2BERX_MASK | XCAN_IXR_E1BERX_MASK
>|
>> +			XCAN_IXR_E2BETXOL_MASK |
>XCAN_IXR_E1BETXOL_MASK |
>> +			XCAN_IXR_E2BETXTL_MASK |
>XCAN_IXR_E1BETXTL_MASK;
>> +
>>  	/* Check for the type of error interrupt and Processing it */
>> -	isr_errors = isr & (XCAN_IXR_ERROR_MASK |
>XCAN_IXR_RXOFLW_MASK |
>> -			    XCAN_IXR_BSOFF_MASK | XCAN_IXR_ARBLST_MASK
>|
>> -			    XCAN_IXR_RXMNF_MASK);
>> +	isr_errors = isr & mask;
>> +
>
>nitpick: don't add this extra newline
>
>>  	if (isr_errors) {
>>  		priv->write_reg(priv, XCAN_ICR_OFFSET, isr_errors);
>>  		xcan_err_interrupt(ndev, isr);
>> @@ -1783,6 +1876,7 @@ static int xcan_probe(struct platform_device
>*pdev)
>>  		return -ENOMEM;
>>
>>  	priv = netdev_priv(ndev);
>> +	priv->ecc_enable = of_property_read_bool(pdev->dev.of_node,
>> +"xlnx,has-ecc");
>>  	priv->dev = &pdev->dev;
>>  	priv->can.bittiming_const = devtype->bittiming_const;
>>  	priv->can.do_set_mode = xcan_do_set_mode; @@ -1880,6 +1974,11
>@@
>> static int xcan_probe(struct platform_device *pdev)
>>  		   priv->reg_base, ndev->irq, priv->can.clock.freq,
>>  		   hw_tx_max, priv->tx_max);
>>
>> +	if (priv->ecc_enable)
>> +		/* Reset FIFO ECC counters */
>> +		priv->write_reg(priv, XCAN_ECC_CFG_OFFSET,
>XCAN_ECC_CFG_REECRX_MASK |
>> +			XCAN_ECC_CFG_REECTXOL_MASK |
>XCAN_ECC_CFG_REECTXTL_MASK);
>> +
>>  	return 0;
>>
>>  err_disableclks:
>> --
>> 2.1.1
>>
>>
>
>--
>Pengutronix e.K.                 | Marc Kleine-Budde          |
>Embedded Linux                   | https://www.pengutronix.de |
>Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
>Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Re: RE: [PATCH 2/3] can: xilinx_can: Add ECC support

[Marc Kleine-Budde]

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

On 21.07.2023 05:23:02, Goud, Srinivas wrote:
> >> +	if (priv->ecc_enable) {
> >> +		u32 reg_ecc;
> >> +
> >> +		reg_ecc = priv->read_reg(priv, XCAN_RXFIFO_ECC_OFFSET);
> >> +		if (isr & XCAN_IXR_E2BERX_MASK) {
> >> +			priv->ecc_2bit_rxfifo_cnt +=
> >> +				FIELD_GET(XCAN_ECC_2BIT_CNT_MASK,
> >reg_ecc);
> >> +			netdev_dbg(ndev, "%s: RX FIFO 2bit ECC error count
> >%d\n",
> >> +				   __func__, priv->ecc_2bit_rxfifo_cnt);
> >> +		}
> >> +		if (isr & XCAN_IXR_E1BERX_MASK) {
> >> +			priv->ecc_1bit_rxfifo_cnt += reg_ecc &
> >> +				XCAN_ECC_1BIT_CNT_MASK;
> >
> >Please use FIELD_GET here, too.
> >
> >> +			netdev_dbg(ndev, "%s: RX FIFO 1bit ECC error count
> >%d\n",
> >> +				   __func__, priv->ecc_1bit_rxfifo_cnt);
> >> +		}
> >> +
> >> +		reg_ecc = priv->read_reg(priv, XCAN_TXOLFIFO_ECC_OFFSET);
> >> +		if (isr & XCAN_IXR_E2BETXOL_MASK) {
> >> +			priv->ecc_2bit_txolfifo_cnt +=
> >> +				FIELD_GET(XCAN_ECC_2BIT_CNT_MASK,
> >reg_ecc);
> >> +			netdev_dbg(ndev, "%s: TXOL FIFO 2bit ECC error count
> >%d\n",
> >> +				   __func__, priv->ecc_2bit_txolfifo_cnt);
> >> +		}
> >> +		if (isr & XCAN_IXR_E1BETXOL_MASK) {
> >> +			priv->ecc_1bit_txolfifo_cnt += reg_ecc &
> >> +				XCAN_ECC_1BIT_CNT_MASK;
> >
> >same here
> >
> >> +			netdev_dbg(ndev, "%s: TXOL FIFO 1bit ECC error count
> >%d\n",
> >> +				   __func__, priv->ecc_1bit_txolfifo_cnt);
> >> +		}
> >> +
> >> +		reg_ecc = priv->read_reg(priv, XCAN_TXTLFIFO_ECC_OFFSET);
> >> +		if (isr & XCAN_IXR_E2BETXTL_MASK) {
> >> +			priv->ecc_2bit_txtlfifo_cnt +=
> >> +				FIELD_GET(XCAN_ECC_2BIT_CNT_MASK,
> >reg_ecc);
> >> +			netdev_dbg(ndev, "%s: TXTL FIFO 2bit ECC error count
> >%d\n",
> >> +				   __func__, priv->ecc_2bit_txtlfifo_cnt);
> >> +		}
> >> +		if (isr & XCAN_IXR_E1BETXTL_MASK) {
> >> +			priv->ecc_1bit_txtlfifo_cnt += reg_ecc &
> >> +				XCAN_ECC_1BIT_CNT_MASK;
> >
> >same here
> >
> >> +			netdev_dbg(ndev, "%s: TXTL FIFO 1bit ECC error count
> >%d\n",
> >> +				   __func__, priv->ecc_1bit_txtlfifo_cnt);
> >> +		}
> >> +
> >> +		/* Reset FIFO ECC counters */
> >> +		priv->write_reg(priv, XCAN_ECC_CFG_OFFSET,
> >XCAN_ECC_CFG_REECRX_MASK |
> >> +				XCAN_ECC_CFG_REECTXOL_MASK |
> >XCAN_ECC_CFG_REECTXTL_MASK);
> >
> >This is racy - you will lose events that occur between reading the register value
> >and clearing the register. You can save the old value and add the difference
> >between the new and the old value to the total counter. What happens when
> >the counter overflows? The following pseudocode should handle the u16
> >counter rolling over to 0:
> As per IP specifications when counter reaching maximum value of 0xFFFF will 
> stays there until reset.
> 
> Not sure we can avoid this race condition completely, as we need to reset
> the counters after reaching the 0xFFFF to avoid losing the events.
> 
> To minimize the race condition, we can reset the counters after reaching 
> the events to 0xFFFF instead of resetting for every interrupt. 
> Can you please suggest if we can go this approach.

Ok, the counter doesn't overflow. To keep the logic in the driver
simple, I think it's best to read the value and reset the counter
directly in the next line. Please add a comment like: "The counter
reaches its maximum at 0xffff and does not overflow. Accept the small
race window between reading and resetting."

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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