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

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

[Srinivas Goud]

Add ECC feature support to Tx and Rx FIFOs for Xilinx CAN Controller.
Part of this feature configuration and counter registers added in
Xilinx AXI CAN Controller for 1bit/2bit ECC errors count and reset.
Also driver reports 1bit/2bit ECC errors for FIFOs based on
ECC error interrupts.

Add xlnx,has-ecc optional property for Xilinx AXI CAN controller to
support ECC if the ECC block is enabled in the HW.

Add ethtool stats interface for getting all the ECC errors information.

There is no public documentation for it available.

---
BRANCH: linux-can-next/master

Changes in v4:
Fix DT binding check warning
Update xlnx,has-ecc property description

Changes in v3:
Update mailing list
Update commit description

Changes in v2:
Address review comments
Add ethtool stats interface
Update commit description


Srinivas Goud (3):
  dt-bindings: can: xilinx_can: Add ECC property 'xlnx,has-ecc'
  can: xilinx_can: Add ECC support
  can: xilinx_can: Add ethtool stats interface for ECC errors

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

-- 
2.1.1

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

[Srinivas Goud]

ECC feature added to Tx and Rx FIFOs for Xilinx AXI CAN Controller.
Part of this feature configuration and counter registers added in
IP for 1bit/2bit ECC errors.

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

Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
---
Changes in v4:
Fix binding check warning
Update property description 

Changes in v3:
Update commit description

Changes in v2:
None

 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 64d57c3..50a2671 100644
--- a/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
+++ b/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
@@ -49,6 +49,10 @@ properties:
   resets:
     maxItems: 1
 
+  xlnx,has-ecc:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: CAN Tx and Rx fifo has ECC (AXI CAN)
+
 required:
   - compatible
   - reg
@@ -137,6 +141,7 @@ examples:
         interrupts = <GIC_SPI 59 IRQ_TYPE_EDGE_RISING>;
         tx-fifo-depth = <0x40>;
         rx-fifo-depth = <0x40>;
+        xlnx,has-ecc;
     };
 
   - |
-- 
2.1.1

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

[Marc Kleine-Budde]

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

On 01.09.2023 14:10:43, Srinivas Goud wrote:
> ECC feature added to Tx and Rx FIFOs for Xilinx AXI CAN Controller.
> Part of this feature configuration and counter registers added in
> IP for 1bit/2bit ECC errors.
> 
> xlnx,has-ecc is optional property and added to Xilinx AXI CAN Controller
> node if ECC block enabled in the HW
> 
> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
> ---
> Changes in v4:
> Fix binding check warning
> Update property description 
> 
> Changes in v3:
> Update commit description
> 
> Changes in v2:
> None
> 
>  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 64d57c3..50a2671 100644
> --- a/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
> +++ b/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
> @@ -49,6 +49,10 @@ properties:
>    resets:
>      maxItems: 1
>  
> +  xlnx,has-ecc:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: CAN Tx and Rx fifo has ECC (AXI CAN)

Are there 2 FIFOs? If so I'd phrase it this way:
"CAN TX and RX FIFOs have ECC support (AXI CAN)" - or -
"CAN TX and RX FIFOs support ECC"

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 --]

[PATCH v4 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>
---
Changes in v4:
None

Changes in v3:
None

Changes in v2:
Address review comments

 drivers/net/can/xilinx_can.c | 129 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 114 insertions(+), 15 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index abe58f1..798b32b 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -53,18 +53,23 @@ enum xcan_reg {
 	XCAN_AFR_OFFSET		= 0x60, /* Acceptance Filter */
 
 	/* only on CAN FD cores */
-	XCAN_F_BRPR_OFFSET	= 0x088, /* Data Phase Baud Rate
-					  * Prescaler
-					  */
-	XCAN_F_BTR_OFFSET	= 0x08C, /* Data Phase Bit Timing */
-	XCAN_TRR_OFFSET		= 0x0090, /* TX Buffer Ready Request */
-	XCAN_AFR_EXT_OFFSET	= 0x00E0, /* Acceptance Filter */
-	XCAN_FSR_OFFSET		= 0x00E8, /* RX FIFO Status */
-	XCAN_TXMSG_BASE_OFFSET	= 0x0100, /* TX Message Space */
+	XCAN_F_BRPR_OFFSET	= 0x88, /* Data Phase Baud Rate Prescaler */
+	XCAN_F_BTR_OFFSET	= 0x8C, /* Data Phase Bit Timing */
+	XCAN_TRR_OFFSET		= 0x90, /* TX Buffer Ready Request */
+
+	/* 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 */
+
+	XCAN_AFR_EXT_OFFSET	= 0xE0, /* Acceptance Filter */
+	XCAN_FSR_OFFSET		= 0xE8, /* RX FIFO Status */
+	XCAN_TXMSG_BASE_OFFSET	= 0x100, /* TX Message Space */
+	XCAN_AFR_2_MASK_OFFSET  = 0xA00, /* Acceptance Filter MASK */
+	XCAN_AFR_2_ID_OFFSET    = 0xA04, /* Acceptance Filter ID */
 	XCAN_RXMSG_BASE_OFFSET	= 0x1100, /* RX Message Space */
 	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 */
 };
 
 #define XCAN_FRAME_ID_OFFSET(frame_base)	((frame_base) + 0x00)
@@ -124,6 +129,12 @@ enum xcan_reg {
 #define XCAN_IXR_TXFLL_MASK		0x00000004 /* Tx FIFO Full intr */
 #define XCAN_IXR_TXOK_MASK		0x00000002 /* TX successful intr */
 #define XCAN_IXR_ARBLST_MASK		0x00000001 /* Arbitration lost intr */
+#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_IDR_ID1_MASK		0xFFE00000 /* Standard msg identifier */
 #define XCAN_IDR_SRR_MASK		0x00100000 /* Substitute remote TXreq */
 #define XCAN_IDR_IDE_MASK		0x00080000 /* Identifier extension */
@@ -137,6 +148,11 @@ 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_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 */
@@ -202,6 +218,13 @@ struct xcan_devtype_data {
  * @devtype:			Device type specific constants
  * @transceiver:		Optional pointer to associated CAN transceiver
  * @rstc:			Pointer to reset control
+ * @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;
@@ -221,6 +244,13 @@ struct xcan_priv {
 	struct xcan_devtype_data devtype;
 	struct phy *transceiver;
 	struct reset_control *rstc;
+	bool ecc_enable;
+	u64 ecc_2bit_rxfifo_cnt;
+	u64 ecc_1bit_rxfifo_cnt;
+	u64 ecc_2bit_txolfifo_cnt;
+	u64 ecc_1bit_txolfifo_cnt;
+	u64 ecc_2bit_txtlfifo_cnt;
+	u64 ecc_1bit_txtlfifo_cnt;
 };
 
 /* CAN Bittiming constants as per Xilinx CAN specs */
@@ -523,6 +553,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;
 
@@ -1127,6 +1162,58 @@ 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 %lld\n",
+				   __func__, priv->ecc_2bit_rxfifo_cnt);
+		}
+		if (isr & XCAN_IXR_E1BERX_MASK) {
+			priv->ecc_1bit_rxfifo_cnt +=
+				FIELD_GET(XCAN_ECC_1BIT_CNT_MASK, reg_ecc);
+			netdev_dbg(ndev, "%s: RX FIFO 1bit ECC error count %lld\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 %lld\n",
+				   __func__, priv->ecc_2bit_txolfifo_cnt);
+		}
+		if (isr & XCAN_IXR_E1BETXOL_MASK) {
+			priv->ecc_1bit_txolfifo_cnt +=
+				FIELD_GET(XCAN_ECC_1BIT_CNT_MASK, reg_ecc);
+			netdev_dbg(ndev, "%s: TXOL FIFO 1bit ECC error count %lld\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 %lld\n",
+				   __func__, priv->ecc_2bit_txtlfifo_cnt);
+		}
+		if (isr & XCAN_IXR_E1BETXTL_MASK) {
+			priv->ecc_1bit_txtlfifo_cnt +=
+				FIELD_GET(XCAN_ECC_1BIT_CNT_MASK, reg_ecc);
+			netdev_dbg(ndev, "%s: TXTL FIFO 1bit ECC error count %lld\n",
+				   __func__, priv->ecc_1bit_txtlfifo_cnt);
+		}
+
+		/* The counter reaches its maximum at 0xffff and does not overflow.
+		 * Accept the small race window between reading and resetting 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);
@@ -1354,9 +1441,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);
@@ -1374,10 +1460,17 @@ 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);
@@ -1796,6 +1889,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;
@@ -1912,6 +2006,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

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

[Marc Kleine-Budde]

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

On 01.09.2023 14:10:44, 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>
> ---
> Changes in v4:
> None
> 
> Changes in v3:
> None
> 
> Changes in v2:
> Address review comments
> 
>  drivers/net/can/xilinx_can.c | 129 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 114 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index abe58f1..798b32b 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -53,18 +53,23 @@ enum xcan_reg {
>  	XCAN_AFR_OFFSET		= 0x60, /* Acceptance Filter */
>  
>  	/* only on CAN FD cores */
> -	XCAN_F_BRPR_OFFSET	= 0x088, /* Data Phase Baud Rate
> -					  * Prescaler
> -					  */
> -	XCAN_F_BTR_OFFSET	= 0x08C, /* Data Phase Bit Timing */
> -	XCAN_TRR_OFFSET		= 0x0090, /* TX Buffer Ready Request */
> -	XCAN_AFR_EXT_OFFSET	= 0x00E0, /* Acceptance Filter */
> -	XCAN_FSR_OFFSET		= 0x00E8, /* RX FIFO Status */
> -	XCAN_TXMSG_BASE_OFFSET	= 0x0100, /* TX Message Space */

These look like unrelated changes to me. Either move them into a
separate patch (before you add the new offsets) or remove them
completely. Please use tabs not spaces for indention, as the rest of
this file does.

> +	XCAN_F_BRPR_OFFSET	= 0x88, /* Data Phase Baud Rate Prescaler */
> +	XCAN_F_BTR_OFFSET	= 0x8C, /* Data Phase Bit Timing */
> +	XCAN_TRR_OFFSET		= 0x90, /* TX Buffer Ready Request */
> +
> +	/* 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 */
> +
> +	XCAN_AFR_EXT_OFFSET	= 0xE0, /* Acceptance Filter */
> +	XCAN_FSR_OFFSET		= 0xE8, /* RX FIFO Status */
> +	XCAN_TXMSG_BASE_OFFSET	= 0x100, /* TX Message Space */
> +	XCAN_AFR_2_MASK_OFFSET  = 0xA00, /* Acceptance Filter MASK */
> +	XCAN_AFR_2_ID_OFFSET    = 0xA04, /* Acceptance Filter ID */
>  	XCAN_RXMSG_BASE_OFFSET	= 0x1100, /* RX Message Space */
>  	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 */
>  };
>  
>  #define XCAN_FRAME_ID_OFFSET(frame_base)	((frame_base) + 0x00)
> @@ -124,6 +129,12 @@ enum xcan_reg {
>  #define XCAN_IXR_TXFLL_MASK		0x00000004 /* Tx FIFO Full intr */
>  #define XCAN_IXR_TXOK_MASK		0x00000002 /* TX successful intr */
>  #define XCAN_IXR_ARBLST_MASK		0x00000001 /* Arbitration lost intr */
> +#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 */

Use tabs for indention not spaces as the rest of the file.

>  #define XCAN_IDR_ID1_MASK		0xFFE00000 /* Standard msg identifier */
>  #define XCAN_IDR_SRR_MASK		0x00100000 /* Substitute remote TXreq */
>  #define XCAN_IDR_IDE_MASK		0x00080000 /* Identifier extension */
> @@ -137,6 +148,11 @@ 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_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 */
> @@ -202,6 +218,13 @@ struct xcan_devtype_data {
>   * @devtype:			Device type specific constants
>   * @transceiver:		Optional pointer to associated CAN transceiver
>   * @rstc:			Pointer to reset control
> + * @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;
> @@ -221,6 +244,13 @@ struct xcan_priv {
>  	struct xcan_devtype_data devtype;
>  	struct phy *transceiver;
>  	struct reset_control *rstc;
> +	bool ecc_enable;
> +	u64 ecc_2bit_rxfifo_cnt;
> +	u64 ecc_1bit_rxfifo_cnt;
> +	u64 ecc_2bit_txolfifo_cnt;
> +	u64 ecc_1bit_txolfifo_cnt;
> +	u64 ecc_2bit_txtlfifo_cnt;
> +	u64 ecc_1bit_txtlfifo_cnt;
>  };
>  
>  /* CAN Bittiming constants as per Xilinx CAN specs */
> @@ -523,6 +553,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;
>  
> @@ -1127,6 +1162,58 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
>  		priv->can.can_stats.bus_error++;
>  	}
>  
> +	if (priv->ecc_enable) {
> +		u32 reg_ecc;

It is better to read out all 3 counters directly one after the other,
then reset them. Then do the updates of the stats. This way you can keep
the lock short in the next patch.

Marc

> +
> +		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 %lld\n",
> +				   __func__, priv->ecc_2bit_rxfifo_cnt);
> +		}
> +		if (isr & XCAN_IXR_E1BERX_MASK) {
> +			priv->ecc_1bit_rxfifo_cnt +=
> +				FIELD_GET(XCAN_ECC_1BIT_CNT_MASK, reg_ecc);
> +			netdev_dbg(ndev, "%s: RX FIFO 1bit ECC error count %lld\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 %lld\n",
> +				   __func__, priv->ecc_2bit_txolfifo_cnt);
> +		}
> +		if (isr & XCAN_IXR_E1BETXOL_MASK) {
> +			priv->ecc_1bit_txolfifo_cnt +=
> +				FIELD_GET(XCAN_ECC_1BIT_CNT_MASK, reg_ecc);
> +			netdev_dbg(ndev, "%s: TXOL FIFO 1bit ECC error count %lld\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 %lld\n",
> +				   __func__, priv->ecc_2bit_txtlfifo_cnt);
> +		}
> +		if (isr & XCAN_IXR_E1BETXTL_MASK) {
> +			priv->ecc_1bit_txtlfifo_cnt +=
> +				FIELD_GET(XCAN_ECC_1BIT_CNT_MASK, reg_ecc);
> +			netdev_dbg(ndev, "%s: TXTL FIFO 1bit ECC error count %lld\n",
> +				   __func__, priv->ecc_1bit_txtlfifo_cnt);
> +		}
> +
> +		/* The counter reaches its maximum at 0xffff and does not overflow.
> +		 * Accept the small race window between reading and resetting 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);
> @@ -1354,9 +1441,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);
> @@ -1374,10 +1460,17 @@ 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);
> @@ -1796,6 +1889,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;
> @@ -1912,6 +2006,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 --]

[PATCH v4 3/3] can: xilinx_can: Add ethtool stats interface for ECC errors

[Srinivas Goud]

Add ethtool stats interface for reading FIFO 1bit/2bit
ECC errors information.

Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
---
Changes in v4:
None

Changes in v3:
None

Changes in v2:
Add ethtool stats interface

 drivers/net/can/xilinx_can.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 798b32b..50e0c9d 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -219,6 +219,7 @@ struct xcan_devtype_data {
  * @transceiver:		Optional pointer to associated CAN transceiver
  * @rstc:			Pointer to reset control
  * @ecc_enable:			ECC enable flag
+ * @stats_lock:			Lock for synchronizing hardware stats
  * @ecc_2bit_rxfifo_cnt:	RXFIFO 2bit ECC count
  * @ecc_1bit_rxfifo_cnt:	RXFIFO 1bit ECC count
  * @ecc_2bit_txolfifo_cnt:	TXOLFIFO 2bit ECC count
@@ -245,6 +246,7 @@ struct xcan_priv {
 	struct phy *transceiver;
 	struct reset_control *rstc;
 	bool ecc_enable;
+	spinlock_t stats_lock; /* Lock for synchronizing hardware stats */
 	u64 ecc_2bit_rxfifo_cnt;
 	u64 ecc_1bit_rxfifo_cnt;
 	u64 ecc_2bit_txolfifo_cnt;
@@ -1164,6 +1166,9 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
 
 	if (priv->ecc_enable) {
 		u32 reg_ecc;
+		unsigned long flags;
+
+		spin_lock_irqsave(&priv->stats_lock, flags);
 
 		reg_ecc = priv->read_reg(priv, XCAN_RXFIFO_ECC_OFFSET);
 		if (isr & XCAN_IXR_E2BERX_MASK) {
@@ -1212,6 +1217,8 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
 		 */
 		priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, XCAN_ECC_CFG_REECRX_MASK |
 				XCAN_ECC_CFG_REECTXOL_MASK | XCAN_ECC_CFG_REECTXTL_MASK);
+
+		spin_unlock_irqrestore(&priv->stats_lock, flags);
 	}
 
 	if (cf.can_id) {
@@ -1639,6 +1646,23 @@ static int xcan_get_auto_tdcv(const struct net_device *ndev, u32 *tdcv)
 	return 0;
 }
 
+static void ethtool_get_ethtool_stats(struct net_device *ndev,
+				      struct ethtool_stats *stats, u64 *data)
+{
+	struct xcan_priv *priv = netdev_priv(ndev);
+	unsigned long flags;
+	int i = 0;
+
+	spin_lock_irqsave(&priv->stats_lock, flags);
+	data[i++] = priv->ecc_2bit_rxfifo_cnt;
+	data[i++] = priv->ecc_1bit_rxfifo_cnt;
+	data[i++] = priv->ecc_2bit_txolfifo_cnt;
+	data[i++] = priv->ecc_1bit_txolfifo_cnt;
+	data[i++] = priv->ecc_2bit_txtlfifo_cnt;
+	data[i++] = priv->ecc_1bit_txtlfifo_cnt;
+	spin_unlock_irqrestore(&priv->stats_lock, flags);
+}
+
 static const struct net_device_ops xcan_netdev_ops = {
 	.ndo_open	= xcan_open,
 	.ndo_stop	= xcan_close,
@@ -1648,6 +1672,7 @@ static const struct net_device_ops xcan_netdev_ops = {
 
 static const struct ethtool_ops xcan_ethtool_ops = {
 	.get_ts_info = ethtool_op_get_ts_info,
+	.get_ethtool_stats = ethtool_get_ethtool_stats,
 };
 
 /**
-- 
2.1.1

Re: [PATCH v4 3/3] can: xilinx_can: Add ethtool stats interface for ECC errors

[Marc Kleine-Budde]

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

On 01.09.2023 14:10:45, Srinivas Goud wrote:
> Add ethtool stats interface for reading FIFO 1bit/2bit
> ECC errors information.
> 
> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
> ---
> Changes in v4:
> None
> 
> Changes in v3:
> None
> 
> Changes in v2:
> Add ethtool stats interface
> 
>  drivers/net/can/xilinx_can.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 798b32b..50e0c9d 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -219,6 +219,7 @@ struct xcan_devtype_data {
>   * @transceiver:		Optional pointer to associated CAN transceiver
>   * @rstc:			Pointer to reset control
>   * @ecc_enable:			ECC enable flag
> + * @stats_lock:			Lock for synchronizing hardware stats

To be precise: The lock is about the access of the 64 bit variables not
about the hardware access:
"Lock for accessing the "ecc_*bit_*fifo_cnt" stats"

>   * @ecc_2bit_rxfifo_cnt:	RXFIFO 2bit ECC count
>   * @ecc_1bit_rxfifo_cnt:	RXFIFO 1bit ECC count
>   * @ecc_2bit_txolfifo_cnt:	TXOLFIFO 2bit ECC count
> @@ -245,6 +246,7 @@ struct xcan_priv {
>  	struct phy *transceiver;
>  	struct reset_control *rstc;
>  	bool ecc_enable;
> +	spinlock_t stats_lock; /* Lock for synchronizing hardware stats */
>  	u64 ecc_2bit_rxfifo_cnt;
>  	u64 ecc_1bit_rxfifo_cnt;
>  	u64 ecc_2bit_txolfifo_cnt;
> @@ -1164,6 +1166,9 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
>  
>  	if (priv->ecc_enable) {
>  		u32 reg_ecc;
> +		unsigned long flags;

nitpick: move the flags before the reg_ecc.

> +
> +		spin_lock_irqsave(&priv->stats_lock, flags);
>  
>  		reg_ecc = priv->read_reg(priv, XCAN_RXFIFO_ECC_OFFSET);
>  		if (isr & XCAN_IXR_E2BERX_MASK) {
> @@ -1212,6 +1217,8 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
>  		 */
>  		priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, XCAN_ECC_CFG_REECRX_MASK |
>  				XCAN_ECC_CFG_REECTXOL_MASK | XCAN_ECC_CFG_REECTXTL_MASK);
> +
> +		spin_unlock_irqrestore(&priv->stats_lock, flags);
>  	}
>  
>  	if (cf.can_id) {
> @@ -1639,6 +1646,23 @@ static int xcan_get_auto_tdcv(const struct net_device *ndev, u32 *tdcv)
>  	return 0;
>  }
>  
> +static void ethtool_get_ethtool_stats(struct net_device *ndev,
> +				      struct ethtool_stats *stats, u64 *data)
> +{
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +	unsigned long flags;
> +	int i = 0;
> +
> +	spin_lock_irqsave(&priv->stats_lock, flags);
> +	data[i++] = priv->ecc_2bit_rxfifo_cnt;
> +	data[i++] = priv->ecc_1bit_rxfifo_cnt;
> +	data[i++] = priv->ecc_2bit_txolfifo_cnt;
> +	data[i++] = priv->ecc_1bit_txolfifo_cnt;
> +	data[i++] = priv->ecc_2bit_txtlfifo_cnt;
> +	data[i++] = priv->ecc_1bit_txtlfifo_cnt;
> +	spin_unlock_irqrestore(&priv->stats_lock, flags);
> +}
> +
>  static const struct net_device_ops xcan_netdev_ops = {
>  	.ndo_open	= xcan_open,
>  	.ndo_stop	= xcan_close,
> @@ -1648,6 +1672,7 @@ static const struct net_device_ops xcan_netdev_ops = {
>  
>  static const struct ethtool_ops xcan_ethtool_ops = {
>  	.get_ts_info = ethtool_op_get_ts_info,
> +	.get_ethtool_stats = ethtool_get_ethtool_stats,

You also should implement .get_strings and .get_sset_count. Have you
tested your patch with "ethtool -S can0"?

>  };
>  
>  /**
> -- 
> 2.1.1
> 
> 

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 v4 3/3] can: xilinx_can: Add ethtool stats interface for ECC errors

[Marc Kleine-Budde]

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

On 01.09.2023 14:10:45, Srinivas Goud wrote:
> Add ethtool stats interface for reading FIFO 1bit/2bit
> ECC errors information.

I just figured out, that there is an u64 stats helper:

https://elixir.bootlin.com/linux/latest/source/include/linux/u64_stats_sync.h

Please make use of this one.

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 --]