Hi,
>-----Original Message-----
>From: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
>Sent: Tuesday, June 13, 2023 5:00 PM
>To: Goud, Srinivas <srinivas.goud@xxxxxxx>
>Cc: wg@xxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
>kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; gcnu.goud@xxxxxxxxx; git (AMD-
>Xilinx) <git@xxxxxxx>; michal.simek@xxxxxxxxxx; linux-can@xxxxxxxxxxxxxxx;
>linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>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@xxxxxxx>
>> ---
>> 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 |
