[PATCH 3/4] can: flexcan: implement error passive state quirk

[PATCH 3/4] can: flexcan: implement error passive state quirk

[ZHU Yi (ST-FIR/ENG1-Zhu)]

From a44bf37a7276d8b985f7bdc92f094af80daeb3f9 Mon Sep 17 00:00:00 2001
From: Zhu Yi <yi.zhu5@cn.bosch.com>
Date: Thu, 31 Aug 2017 09:21:24 +0800
Subject: [PATCH 3/4] can: flexcan: implement error passive state quirk

Add FLEXCAN_QUIRK_BROKEN_PERR_STATE for better description
of the missing error passive interrupt quirk.

Error interrupt flooding may happen if the broken error state quirk
fix is enabled. For example, in case there is singled out node on
the bus and the node sends a frame, then error interrupt flooding
happens and will not stop because the node cannot go to bus off. The
flooding will stop after another node connected to the bus again.

If high bitrate configured on the low end system, then the flooding
may causes performance issue, hence, this patch mitigates this by:
1. disable error interrupt upon error passive state transition
2. re-enable error interrupt upon error warning state transition
3. disable/enable error interrupt upon error active state transition
   depends on FLEXCAN_QUIRK_BROKEN_WERR_STATE

In this way, the driver is still able to report correct state
transitions without additional latency. When there are bus problems,
flooding of error interrupts is limited to the number of frames
required to change state from error warning to error passive if the
core has [TR]WRN_INT connected (FLEXCAN_QUIRK_BROKEN_WERR_STATE is
not enabled), otherwise, the flooding is limited to the number of
frames required to change state from error active to error passive.

Signed-off-by: Zhu Yi <yi.zhu5@cn.bosch.com>
Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
---
 drivers/net/can/flexcan.c | 79 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 70 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index e163c55..52b69fc 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -182,14 +182,14 @@
 /* FLEXCAN hardware feature flags
  *
  * Below is some version info we got:
- *    SOC   Version   IP-Version  Glitch- [TR]WRN_INT Memory err RTR re-
- *                                Filter? connected?  detection  ception in MB
- *   MX25  FlexCAN2  03.00.00.00     no        no         no        no
- *   MX28  FlexCAN2  03.00.04.00    yes       yes         no        no
- *   MX35  FlexCAN2  03.00.00.00     no        no         no        no
- *   MX53  FlexCAN2  03.00.00.00    yes        no         no        no
- *   MX6s  FlexCAN3  10.00.12.00    yes       yes         no       yes
- *   VF610 FlexCAN3  ?               no       yes        yes       yes?
+ *    SOC   Version   IP-Version  Glitch- [TR]WRN_INT IRQ Err Memory err RTR re-
+ *                                Filter? connected?  Passive detection  ception in MB
+ *   MX25  FlexCAN2  03.00.00.00     no        no         ?       no        no
+ *   MX28  FlexCAN2  03.00.04.00    yes       yes         ?       no        no
+ *   MX35  FlexCAN2  03.00.00.00     no        no         ?       no        no
+ *   MX53  FlexCAN2  03.00.00.00    yes        no        no?      no        no
+ *   MX6s  FlexCAN3  10.00.12.00    yes       yes        no       no       yes
+ *   VF610 FlexCAN3  ?               no       yes         ?      yes       yes?
  *
  * Some SOCs do not have the RX_WARN & TX_WARN interrupt line connected.
  */
@@ -198,6 +198,7 @@
 #define FLEXCAN_QUIRK_ENABLE_EACEN_RRS	BIT(3) /* Enable EACEN and RRS bit in ctrl2 */
 #define FLEXCAN_QUIRK_DISABLE_MECR	BIT(4) /* Disable Memory error detection */
 #define FLEXCAN_QUIRK_USE_OFF_TIMESTAMP	BIT(5) /* Use timestamp based offloading */
+#define FLEXCAN_QUIRK_BROKEN_PERR_STATE	BIT(6) /* No interrupt for error passive */
 
 /* Structure of the message buffer */
 struct flexcan_mb {
@@ -335,6 +336,26 @@ static inline void flexcan_write(u32 val, void __iomem *addr)
 }
 #endif
 
+static inline void flexcan_error_irq_enable(const struct flexcan_priv *priv)
+{
+	struct flexcan_regs __iomem *regs = priv->regs;
+	u32 reg_ctrl;
+
+	reg_ctrl = flexcan_read(&regs->ctrl);
+	reg_ctrl |= FLEXCAN_CTRL_ERR_MSK;
+	flexcan_write(reg_ctrl, &regs->ctrl);
+}
+
+static inline void flexcan_error_irq_disable(const struct flexcan_priv *priv)
+{
+	struct flexcan_regs __iomem *regs = priv->regs;
+	u32 reg_ctrl;
+
+	reg_ctrl = flexcan_read(&regs->ctrl);
+	reg_ctrl &= ~FLEXCAN_CTRL_ERR_MSK;
+	flexcan_write(reg_ctrl, &regs->ctrl);
+}
+
 static inline int flexcan_transceiver_enable(const struct flexcan_priv *priv)
 {
 	if (!priv->reg_xceiver)
@@ -713,6 +734,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	struct flexcan_regs __iomem *regs = priv->regs;
 	irqreturn_t handled = IRQ_NONE;
 	u32 reg_iflag1, reg_esr;
+	enum can_state last_state = priv->can.state;
 
 	reg_iflag1 = flexcan_read(&regs->iflag1);
 
@@ -767,7 +789,8 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 
 	/* state change interrupt or broken error state quirk fix is enabled */
 	if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
-	    (priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_WERR_STATE))
+	    (priv->devtype_data->quirks & (FLEXCAN_QUIRK_BROKEN_WERR_STATE |
+	                                   FLEXCAN_QUIRK_BROKEN_PERR_STATE)))
 		flexcan_irq_state(dev, reg_esr);
 
 	/* bus error IRQ - handle if bus error reporting is activated */
@@ -775,6 +798,44 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	    (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
 		flexcan_irq_bus_err(dev, reg_esr);
 
+	/* availability of error interrupt among state transitions in case
+	 * bus error reporting is de-activated and
+	 * FLEXCAN_QUIRK_BROKEN_PERR_STATE is enabled:
+	 *  +--------------------------------------------------------------+
+	 *  | +----------------------------------------------+ [stopped /  |
+	 *  | |                                              |  sleeping] -+
+	 *  +-+-> active <-> warning <-> passive -> bus off -+
+	 *        ___________^^^^^^^^^^^^_______________________________
+	 *        disabled(1)  enabled             disabled
+	 *
+	 * (1): enabled if FLEXCAN_QUIRK_BROKEN_WERR_STATE is enabled
+	 */
+	if ((last_state != priv->can.state) &&
+	    (priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_PERR_STATE) &&
+	   !(priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) {
+		switch (priv->can.state) {
+		case CAN_STATE_ERROR_ACTIVE:
+			if (priv->devtype_data->quirks &
+			    FLEXCAN_QUIRK_BROKEN_WERR_STATE)
+				flexcan_error_irq_enable(priv);
+			else
+				flexcan_error_irq_disable(priv);
+			break;
+
+		case CAN_STATE_ERROR_WARNING:
+			flexcan_error_irq_enable(priv);
+			break;
+
+		case CAN_STATE_ERROR_PASSIVE:
+		case CAN_STATE_BUS_OFF:
+			flexcan_error_irq_disable(priv);
+			break;
+
+		default:
+			break;
+		}
+	}
+
 	return handled;
 }
 
-- 
2.7.4

Re: [PATCH 3/4] can: flexcan: implement error passive state quirk

[Wolfgang Grandegger]

Hello,

Am 05.09.2017 um 10:39 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
>>From a44bf37a7276d8b985f7bdc92f094af80daeb3f9 Mon Sep 17 00:00:00 2001
> From: Zhu Yi <yi.zhu5@cn.bosch.com>
> Date: Thu, 31 Aug 2017 09:21:24 +0800
> Subject: [PATCH 3/4] can: flexcan: implement error passive state quirk
> 
> Add FLEXCAN_QUIRK_BROKEN_PERR_STATE for better description
> of the missing error passive interrupt quirk.
> 
> Error interrupt flooding may happen if the broken error state quirk
> fix is enabled. For example, in case there is singled out node on
> the bus and the node sends a frame, then error interrupt flooding
> happens and will not stop because the node cannot go to bus off. The
> flooding will stop after another node connected to the bus again.
> 
> If high bitrate configured on the low end system, then the flooding
> may causes performance issue, hence, this patch mitigates this by:
> 1. disable error interrupt upon error passive state transition
> 2. re-enable error interrupt upon error warning state transition
> 3. disable/enable error interrupt upon error active state transition
>     depends on FLEXCAN_QUIRK_BROKEN_WERR_STATE
> 
> In this way, the driver is still able to report correct state
> transitions without additional latency. When there are bus problems,
> flooding of error interrupts is limited to the number of frames
> required to change state from error warning to error passive if the
> core has [TR]WRN_INT connected (FLEXCAN_QUIRK_BROKEN_WERR_STATE is
> not enabled), otherwise, the flooding is limited to the number of
> frames required to change state from error active to error passive.
> 
> Signed-off-by: Zhu Yi <yi.zhu5@cn.bosch.com>
> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
> ---
>   drivers/net/can/flexcan.c | 79 +++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 70 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index e163c55..52b69fc 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -182,14 +182,14 @@
>   /* FLEXCAN hardware feature flags
>    *
>    * Below is some version info we got:
> - *    SOC   Version   IP-Version  Glitch- [TR]WRN_INT Memory err RTR re-
> - *                                Filter? connected?  detection  ception in MB
> - *   MX25  FlexCAN2  03.00.00.00     no        no         no        no
> - *   MX28  FlexCAN2  03.00.04.00    yes       yes         no        no
> - *   MX35  FlexCAN2  03.00.00.00     no        no         no        no
> - *   MX53  FlexCAN2  03.00.00.00    yes        no         no        no
> - *   MX6s  FlexCAN3  10.00.12.00    yes       yes         no       yes
> - *   VF610 FlexCAN3  ?               no       yes        yes       yes?
> + *    SOC   Version   IP-Version  Glitch- [TR]WRN_INT IRQ Err Memory err RTR re-
> + *                                Filter? connected?  Passive detection  ception in MB
> + *   MX25  FlexCAN2  03.00.00.00     no        no         ?       no        no
> + *   MX28  FlexCAN2  03.00.04.00    yes       yes         ?       no        no
> + *   MX35  FlexCAN2  03.00.00.00     no        no         ?       no        no
> + *   MX53  FlexCAN2  03.00.00.00    yes        no        no?      no        no
> + *   MX6s  FlexCAN3  10.00.12.00    yes       yes        no       no       yes
> + *   VF610 FlexCAN3  ?               no       yes         ?      yes       yes?
>    *
>    * Some SOCs do not have the RX_WARN & TX_WARN interrupt line connected.
>    */
> @@ -198,6 +198,7 @@
>   #define FLEXCAN_QUIRK_ENABLE_EACEN_RRS	BIT(3) /* Enable EACEN and RRS bit in ctrl2 */
>   #define FLEXCAN_QUIRK_DISABLE_MECR	BIT(4) /* Disable Memory error detection */
>   #define FLEXCAN_QUIRK_USE_OFF_TIMESTAMP	BIT(5) /* Use timestamp based offloading */
> +#define FLEXCAN_QUIRK_BROKEN_PERR_STATE	BIT(6) /* No interrupt for error passive */
>   
>   /* Structure of the message buffer */
>   struct flexcan_mb {
> @@ -335,6 +336,26 @@ static inline void flexcan_write(u32 val, void __iomem *addr)
>   }
>   #endif
>   
> +static inline void flexcan_error_irq_enable(const struct flexcan_priv *priv)
> +{
> +	struct flexcan_regs __iomem *regs = priv->regs;
> +	u32 reg_ctrl;
> +
> +	reg_ctrl = flexcan_read(&regs->ctrl);
> +	reg_ctrl |= FLEXCAN_CTRL_ERR_MSK;
> +	flexcan_write(reg_ctrl, &regs->ctrl);
> +}
> +
> +static inline void flexcan_error_irq_disable(const struct flexcan_priv *priv)
> +{
> +	struct flexcan_regs __iomem *regs = priv->regs;
> +	u32 reg_ctrl;
> +
> +	reg_ctrl = flexcan_read(&regs->ctrl);
> +	reg_ctrl &= ~FLEXCAN_CTRL_ERR_MSK;
> +	flexcan_write(reg_ctrl, &regs->ctrl);
> +}
> +
>   static inline int flexcan_transceiver_enable(const struct flexcan_priv *priv)
>   {
>   	if (!priv->reg_xceiver)
> @@ -713,6 +734,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>   	struct flexcan_regs __iomem *regs = priv->regs;
>   	irqreturn_t handled = IRQ_NONE;
>   	u32 reg_iflag1, reg_esr;
> +	enum can_state last_state = priv->can.state;
>   
>   	reg_iflag1 = flexcan_read(&regs->iflag1);
>   
> @@ -767,7 +789,8 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>   
>   	/* state change interrupt or broken error state quirk fix is enabled */
>   	if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
> -	    (priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_WERR_STATE))
> +	    (priv->devtype_data->quirks & (FLEXCAN_QUIRK_BROKEN_WERR_STATE |
> +	                                   FLEXCAN_QUIRK_BROKEN_PERR_STATE)))
>   		flexcan_irq_state(dev, reg_esr);
>   
>   	/* bus error IRQ - handle if bus error reporting is activated */
> @@ -775,6 +798,44 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>   	    (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
>   		flexcan_irq_bus_err(dev, reg_esr);
>   
> +	/* availability of error interrupt among state transitions in case
> +	 * bus error reporting is de-activated and
> +	 * FLEXCAN_QUIRK_BROKEN_PERR_STATE is enabled:
> +	 *  +--------------------------------------------------------------+
> +	 *  | +----------------------------------------------+ [stopped /  |
> +	 *  | |                                              |  sleeping] -+
> +	 *  +-+-> active <-> warning <-> passive -> bus off -+
> +	 *        ___________^^^^^^^^^^^^_______________________________
> +	 *        disabled(1)  enabled             disabled
> +	 *
> +	 * (1): enabled if FLEXCAN_QUIRK_BROKEN_WERR_STATE is enabled
> +	 */
> +	if ((last_state != priv->can.state) &&
> +	    (priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_PERR_STATE) &&
> +	   !(priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) {

This block is only entered when FLEXCAN_QUIRK_BROKEN_PERR_STATE is set
but not for legacy cores. Or have I missed something.

> +		switch (priv->can.state) {
> +		case CAN_STATE_ERROR_ACTIVE:
> +			if (priv->devtype_data->quirks &
> +			    FLEXCAN_QUIRK_BROKEN_WERR_STATE)
> +				flexcan_error_irq_enable(priv);
> +			else
> +				flexcan_error_irq_disable(priv);
> +			break;
> +
> +		case CAN_STATE_ERROR_WARNING:
> +			flexcan_error_irq_enable(priv);
> +			break;
> +
> +		case CAN_STATE_ERROR_PASSIVE:
> +		case CAN_STATE_BUS_OFF:
> +			flexcan_error_irq_disable(priv);
> +			break;
> +
> +		default:
> +			break;
> +		}
> +	}
> +
>   	return handled;
>   }

Hope to find some time this week to test the patches on my i.MX53 and 
i.MX28 boards.

Wolfgang.

Re: [PATCH 3/4] can: flexcan: implement error passive state quirk

[Wolfgang Grandegger]

Hello ZHU Yi,

now testing on an i.MX53 board (quirks=2):

Am 06.09.2017 um 13:47 schrieb Wolfgang Grandegger:
> Hello,
> 
> Am 05.09.2017 um 10:39 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
>>> From a44bf37a7276d8b985f7bdc92f094af80daeb3f9 Mon Sep 17 00:00:00 2001
>> From: Zhu Yi <yi.zhu5@cn.bosch.com>
>> Date: Thu, 31 Aug 2017 09:21:24 +0800
>> Subject: [PATCH 3/4] can: flexcan: implement error passive state quirk
>>
>> Add FLEXCAN_QUIRK_BROKEN_PERR_STATE for better description
>> of the missing error passive interrupt quirk.
>>
>> Error interrupt flooding may happen if the broken error state quirk
>> fix is enabled. For example, in case there is singled out node on
>> the bus and the node sends a frame, then error interrupt flooding
>> happens and will not stop because the node cannot go to bus off. The
>> flooding will stop after another node connected to the bus again.
>>
>> If high bitrate configured on the low end system, then the flooding
>> may causes performance issue, hence, this patch mitigates this by:
>> 1. disable error interrupt upon error passive state transition
>> 2. re-enable error interrupt upon error warning state transition
>> 3. disable/enable error interrupt upon error active state transition
>>     depends on FLEXCAN_QUIRK_BROKEN_WERR_STATE
>>
>> In this way, the driver is still able to report correct state
>> transitions without additional latency. When there are bus problems,
>> flooding of error interrupts is limited to the number of frames
>> required to change state from error warning to error passive if the
>> core has [TR]WRN_INT connected (FLEXCAN_QUIRK_BROKEN_WERR_STATE is
>> not enabled), otherwise, the flooding is limited to the number of
>> frames required to change state from error active to error passive.
>>
>> Signed-off-by: Zhu Yi <yi.zhu5@cn.bosch.com>
>> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
>> ---
>>   drivers/net/can/flexcan.c | 79 
>> +++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 70 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>> index e163c55..52b69fc 100644
>> --- a/drivers/net/can/flexcan.c
>> +++ b/drivers/net/can/flexcan.c
>> @@ -182,14 +182,14 @@
>>   /* FLEXCAN hardware feature flags
>>    *
>>    * Below is some version info we got:
>> - *    SOC   Version   IP-Version  Glitch- [TR]WRN_INT Memory err RTR re-
>> - *                                Filter? connected?  detection  
>> ception in MB
>> - *   MX25  FlexCAN2  03.00.00.00     no        no         no        no
>> - *   MX28  FlexCAN2  03.00.04.00    yes       yes         no        no
>> - *   MX35  FlexCAN2  03.00.00.00     no        no         no        no
>> - *   MX53  FlexCAN2  03.00.00.00    yes        no         no        no
>> - *   MX6s  FlexCAN3  10.00.12.00    yes       yes         no       yes
>> - *   VF610 FlexCAN3  ?               no       yes        yes       yes?
>> + *    SOC   Version   IP-Version  Glitch- [TR]WRN_INT IRQ Err Memory 
>> err RTR re-
>> + *                                Filter? connected?  Passive 
>> detection  ception in MB
>> + *   MX25  FlexCAN2  03.00.00.00     no        no         ?       
>> no        no
>> + *   MX28  FlexCAN2  03.00.04.00    yes       yes         ?       
>> no        no
>> + *   MX35  FlexCAN2  03.00.00.00     no        no         ?       
>> no        no
>> + *   MX53  FlexCAN2  03.00.00.00    yes        no        no?      
>> no        no
>> + *   MX6s  FlexCAN3  10.00.12.00    yes       yes        no       
>> no       yes
>> + *   VF610 FlexCAN3  ?               no       yes         ?      
>> yes       yes?
>>    *
>>    * Some SOCs do not have the RX_WARN & TX_WARN interrupt line 
>> connected.
>>    */
>> @@ -198,6 +198,7 @@
>>   #define FLEXCAN_QUIRK_ENABLE_EACEN_RRS    BIT(3) /* Enable EACEN and 
>> RRS bit in ctrl2 */
>>   #define FLEXCAN_QUIRK_DISABLE_MECR    BIT(4) /* Disable Memory error 
>> detection */
>>   #define FLEXCAN_QUIRK_USE_OFF_TIMESTAMP    BIT(5) /* Use timestamp 
>> based offloading */
>> +#define FLEXCAN_QUIRK_BROKEN_PERR_STATE    BIT(6) /* No interrupt for 
>> error passive */
>>   /* Structure of the message buffer */
>>   struct flexcan_mb {
>> @@ -335,6 +336,26 @@ static inline void flexcan_write(u32 val, void 
>> __iomem *addr)
>>   }
>>   #endif
>> +static inline void flexcan_error_irq_enable(const struct flexcan_priv 
>> *priv)
>> +{
>> +    struct flexcan_regs __iomem *regs = priv->regs;
>> +    u32 reg_ctrl;
>> +
>> +    reg_ctrl = flexcan_read(&regs->ctrl);
>> +    reg_ctrl |= FLEXCAN_CTRL_ERR_MSK;
>> +    flexcan_write(reg_ctrl, &regs->ctrl);
>> +}
>> +
>> +static inline void flexcan_error_irq_disable(const struct 
>> flexcan_priv *priv)
>> +{
>> +    struct flexcan_regs __iomem *regs = priv->regs;
>> +    u32 reg_ctrl;
>> +
>> +    reg_ctrl = flexcan_read(&regs->ctrl);
>> +    reg_ctrl &= ~FLEXCAN_CTRL_ERR_MSK;
>> +    flexcan_write(reg_ctrl, &regs->ctrl);
>> +}
>> +
>>   static inline int flexcan_transceiver_enable(const struct 
>> flexcan_priv *priv)
>>   {
>>       if (!priv->reg_xceiver)
>> @@ -713,6 +734,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>>       struct flexcan_regs __iomem *regs = priv->regs;
>>       irqreturn_t handled = IRQ_NONE;
>>       u32 reg_iflag1, reg_esr;
>> +    enum can_state last_state = priv->can.state;
>>       reg_iflag1 = flexcan_read(&regs->iflag1);
>> @@ -767,7 +789,8 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>>       /* state change interrupt or broken error state quirk fix is 
>> enabled */
>>       if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
>> -        (priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_WERR_STATE))
>> +        (priv->devtype_data->quirks & (FLEXCAN_QUIRK_BROKEN_WERR_STATE |
>> +                                       
>> FLEXCAN_QUIRK_BROKEN_PERR_STATE)))
>>           flexcan_irq_state(dev, reg_esr);
>>       /* bus error IRQ - handle if bus error reporting is activated */
>> @@ -775,6 +798,44 @@ static irqreturn_t flexcan_irq(int irq, void 
>> *dev_id)
>>           (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
>>           flexcan_irq_bus_err(dev, reg_esr);

I added here:

	/* Clear interrupt bits in ESR */
	flexcan_write(reg_esr, &regs->esr);

But it seems not to have an effect. At least the manual tells that the
interrupt bits can (should?) be cleared.

>> +    /* availability of error interrupt among state transitions in case
>> +     * bus error reporting is de-activated and
>> +     * FLEXCAN_QUIRK_BROKEN_PERR_STATE is enabled:
>> +     *  +--------------------------------------------------------------+
>> +     *  | +----------------------------------------------+ [stopped /  |
>> +     *  | |                                              |  sleeping] -+
>> +     *  +-+-> active <-> warning <-> passive -> bus off -+
>> +     *        ___________^^^^^^^^^^^^_______________________________
>> +     *        disabled(1)  enabled             disabled
>> +     *
>> +     * (1): enabled if FLEXCAN_QUIRK_BROKEN_WERR_STATE is enabled
>> +     */
>> +    if ((last_state != priv->can.state) &&
>> +        (priv->devtype_data->quirks & 
>> FLEXCAN_QUIRK_BROKEN_PERR_STATE) &&
>> +       !(priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) {
> 
> This block is only entered when FLEXCAN_QUIRK_BROKEN_PERR_STATE is set
> but not for legacy cores. Or have I missed something.

I need here:

	    (priv->devtype_data->quirks & (FLEXCAN_QUIRK_BROKEN_PERR_STATE |
					   FLEXCAN_QUIRK_BROKEN_WERR_STATE)) &&

Or maybe better, add FLEXCAN_QUIRK_BROKEN_PERR_STATE to the legacy cores
as well.

> 
>> +        switch (priv->can.state) {
>> +        case CAN_STATE_ERROR_ACTIVE:
>> +            if (priv->devtype_data->quirks &
>> +                FLEXCAN_QUIRK_BROKEN_WERR_STATE)
>> +                flexcan_error_irq_enable(priv);
>> +            else
>> +                flexcan_error_irq_disable(priv);
>> +            break;
>> +
>> +        case CAN_STATE_ERROR_WARNING:
>> +            flexcan_error_irq_enable(priv);
>> +            break;
>> +
>> +        case CAN_STATE_ERROR_PASSIVE:
>> +        case CAN_STATE_BUS_OFF:
>> +            flexcan_error_irq_disable(priv);
>> +            break;
>> +
>> +        default:
>> +            break;
>> +        }
>> +    }
>> +
>>       return handled;
>>   }
> 
> Hope to find some time this week to test the patches on my i.MX53 and 
> i.MX28 boards.

Here is the annotated trace when unplugging and reconnecting the cable:

# ip link set can0 txqueuelen 1000 up type can bitrate 1000000
# cansend can0 123#abcd
flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x80 err=0/0

*** disconnect CAN cable ***

# cansend can0 123#abcd
flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x2042 err=8/0
flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x22252 err=128/0
flexcan 53fc8000.can can0: state 0->2 ctrl

It jumps immediately to error passive!

*** reconnect CAN cable ***

flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x200 reg_esr=0x2292 err=135/0

The error counter jumps to 135... weird!

# cansend can0 123#abcd
flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x200 reg_esr=0x290 err=134/0
...
# cansend can0 123#abcd
flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x200 reg_esr=0x290 err=128/0
# cansend can0 123#abcd
flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x200 reg_esr=0x280 err=127/0
flexcan 53fc8000.can can0: state 2->1 ctrl

Then it takes some transfers before error warning is reached.

# cansend can0 123#abcd
flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x280 err=126/0
...
# cansend can0 123#abcd
flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x280 err=97/0
# cansend can0 123#abcd
flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x280 err=96/0
# cansend can0 123#abcd
flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x80 err=95/0
flexcan 53fc8000.can can0: state 1->0 ctrl
# cansend can0 123#abcd
flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x80 err=94/0

*** disconnect CAN cable again ***

# cansend can0 123#abcd
flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x22242 err=98/0
flexcan 53fc8000.can can0: state 0->1 ctrl
flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x2252 err=130/0
flexcan 53fc8000.can can0: state 1->2 ctrl

This time error warning is signalled". And here is with short-circuiting
CAN low and high:

# cansend can0 123#abcd
flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x24252 err=136/0
flexcan 53fc8000.can can0: state 0->2 ctrl
flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x0 reg_esr=0x4034 err=0/0
flexcan 53fc8000.can can0: state 2->3 ctrl
# ip link set can0 type can restart

Even if the state reporting is kind of weird with that core, interrupt flooding
does not happen.

Wolfgang.

RE: [PATCH 3/4] can: flexcan: implement error passive state quirk

[ZHU Yi (ST-FIR/ENG1-Zhu)]

Hello Wolfgang,

>From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>Sent: Thursday, September 07, 2017 4:17 AM
>Hello ZHU Yi,
>
>now testing on an i.MX53 board (quirks=2):
>
>Am 06.09.2017 um 13:47 schrieb Wolfgang Grandegger:
>> Hello,
>>
>> Am 05.09.2017 um 10:39 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
>>>> From a44bf37a7276d8b985f7bdc92f094af80daeb3f9 Mon Sep 17 00:00:00 2001
>>> From: Zhu Yi <yi.zhu5@cn.bosch.com>
>>> Date: Thu, 31 Aug 2017 09:21:24 +0800
>>> Subject: [PATCH 3/4] can: flexcan: implement error passive state quirk
>>>
>>> Add FLEXCAN_QUIRK_BROKEN_PERR_STATE for better description
>>> of the missing error passive interrupt quirk.
>>>
>>> Error interrupt flooding may happen if the broken error state quirk
>>> fix is enabled. For example, in case there is singled out node on
>>> the bus and the node sends a frame, then error interrupt flooding
>>> happens and will not stop because the node cannot go to bus off. The
>>> flooding will stop after another node connected to the bus again.
>>>
>>> If high bitrate configured on the low end system, then the flooding
>>> may causes performance issue, hence, this patch mitigates this by:
>>> 1. disable error interrupt upon error passive state transition
>>> 2. re-enable error interrupt upon error warning state transition
>>> 3. disable/enable error interrupt upon error active state transition
>>>     depends on FLEXCAN_QUIRK_BROKEN_WERR_STATE
>>>
>>> In this way, the driver is still able to report correct state
>>> transitions without additional latency. When there are bus problems,
>>> flooding of error interrupts is limited to the number of frames
>>> required to change state from error warning to error passive if the
>>> core has [TR]WRN_INT connected (FLEXCAN_QUIRK_BROKEN_WERR_STATE is
>>> not enabled), otherwise, the flooding is limited to the number of
>>> frames required to change state from error active to error passive.
>>>
>>> Signed-off-by: Zhu Yi <yi.zhu5@cn.bosch.com>
>>> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
>>> ---
>>>   drivers/net/can/flexcan.c | 79
>>> +++++++++++++++++++++++++++++++++++++++++------
>>>   1 file changed, 70 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>>> index e163c55..52b69fc 100644
>>> --- a/drivers/net/can/flexcan.c
>>> +++ b/drivers/net/can/flexcan.c
>>> @@ -182,14 +182,14 @@
>>>   /* FLEXCAN hardware feature flags
>>>    *
>>>    * Below is some version info we got:
>>> - *    SOC   Version   IP-Version  Glitch- [TR]WRN_INT Memory err RTR re-
>>> - *                                Filter? connected?  detection
>>> ception in MB
>>> - *   MX25  FlexCAN2  03.00.00.00     no        no         no        no
>>> - *   MX28  FlexCAN2  03.00.04.00    yes       yes         no        no
>>> - *   MX35  FlexCAN2  03.00.00.00     no        no         no        no
>>> - *   MX53  FlexCAN2  03.00.00.00    yes        no         no        no
>>> - *   MX6s  FlexCAN3  10.00.12.00    yes       yes         no       yes
>>> - *   VF610 FlexCAN3  ?               no       yes        yes       yes?
>>> + *    SOC   Version   IP-Version  Glitch- [TR]WRN_INT IRQ Err Memory
>>> err RTR re-
>>> + *                                Filter? connected?  Passive
>>> detection  ception in MB
>>> + *   MX25  FlexCAN2  03.00.00.00     no        no         ?
>>> no        no
>>> + *   MX28  FlexCAN2  03.00.04.00    yes       yes         ?
>>> no        no
>>> + *   MX35  FlexCAN2  03.00.00.00     no        no         ?
>>> no        no
>>> + *   MX53  FlexCAN2  03.00.00.00    yes        no        no?
>>> no        no
>>> + *   MX6s  FlexCAN3  10.00.12.00    yes       yes        no
>>> no       yes
>>> + *   VF610 FlexCAN3  ?               no       yes         ?
>>> yes       yes?
>>>    *
>>>    * Some SOCs do not have the RX_WARN & TX_WARN interrupt line
>>> connected.
>>>    */
>>> @@ -198,6 +198,7 @@
>>>   #define FLEXCAN_QUIRK_ENABLE_EACEN_RRS    BIT(3) /* Enable EACEN and
>>> RRS bit in ctrl2 */
>>>   #define FLEXCAN_QUIRK_DISABLE_MECR    BIT(4) /* Disable Memory error
>>> detection */
>>>   #define FLEXCAN_QUIRK_USE_OFF_TIMESTAMP    BIT(5) /* Use timestamp
>>> based offloading */
>>> +#define FLEXCAN_QUIRK_BROKEN_PERR_STATE    BIT(6) /* No interrupt for
>>> error passive */
>>>   /* Structure of the message buffer */
>>>   struct flexcan_mb {
>>> @@ -335,6 +336,26 @@ static inline void flexcan_write(u32 val, void
>>> __iomem *addr)
>>>   }
>>>   #endif
>>> +static inline void flexcan_error_irq_enable(const struct flexcan_priv
>>> *priv)
>>> +{
>>> +    struct flexcan_regs __iomem *regs = priv->regs;
>>> +    u32 reg_ctrl;
>>> +
>>> +    reg_ctrl = flexcan_read(&regs->ctrl);
>>> +    reg_ctrl |= FLEXCAN_CTRL_ERR_MSK;
>>> +    flexcan_write(reg_ctrl, &regs->ctrl);
>>> +}
>>> +
>>> +static inline void flexcan_error_irq_disable(const struct
>>> flexcan_priv *priv)
>>> +{
>>> +    struct flexcan_regs __iomem *regs = priv->regs;
>>> +    u32 reg_ctrl;
>>> +
>>> +    reg_ctrl = flexcan_read(&regs->ctrl);
>>> +    reg_ctrl &= ~FLEXCAN_CTRL_ERR_MSK;
>>> +    flexcan_write(reg_ctrl, &regs->ctrl);
>>> +}
>>> +
>>>   static inline int flexcan_transceiver_enable(const struct
>>> flexcan_priv *priv)
>>>   {
>>>       if (!priv->reg_xceiver)
>>> @@ -713,6 +734,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>>>       struct flexcan_regs __iomem *regs = priv->regs;
>>>       irqreturn_t handled = IRQ_NONE;
>>>       u32 reg_iflag1, reg_esr;
>>> +    enum can_state last_state = priv->can.state;
>>>       reg_iflag1 = flexcan_read(&regs->iflag1);
>>> @@ -767,7 +789,8 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>>>       /* state change interrupt or broken error state quirk fix is
>>> enabled */
>>>       if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
>>> -        (priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_WERR_STATE))
>>> +        (priv->devtype_data->quirks & (FLEXCAN_QUIRK_BROKEN_WERR_STATE |
>>> +
>>> FLEXCAN_QUIRK_BROKEN_PERR_STATE)))
>>>           flexcan_irq_state(dev, reg_esr);
>>>       /* bus error IRQ - handle if bus error reporting is activated */
>>> @@ -775,6 +798,44 @@ static irqreturn_t flexcan_irq(int irq, void
>>> *dev_id)
>>>           (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
>>>           flexcan_irq_bus_err(dev, reg_esr);
>
>I added here:
>
>	/* Clear interrupt bits in ESR */
>	flexcan_write(reg_esr, &regs->esr);
>
>But it seems not to have an effect. At least the manual tells that the
>interrupt bits can (should?) be cleared.
The ESR interrupt bits are already cleared at
http://elixir.free-electrons.com/linux/latest/source/drivers/net/can/flexcan.c#L765
by "flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT, &regs->esr);".
So clear here again won't have effect.

>
>>> +    /* availability of error interrupt among state transitions in case
>>> +     * bus error reporting is de-activated and
>>> +     * FLEXCAN_QUIRK_BROKEN_PERR_STATE is enabled:
>>> +     *  +--------------------------------------------------------------+
>>> +     *  | +----------------------------------------------+ [stopped /  |
>>> +     *  | |                                              |  sleeping] -+
>>> +     *  +-+-> active <-> warning <-> passive -> bus off -+
>>> +     *        ___________^^^^^^^^^^^^_______________________________
>>> +     *        disabled(1)  enabled             disabled
>>> +     *
>>> +     * (1): enabled if FLEXCAN_QUIRK_BROKEN_WERR_STATE is enabled
>>> +     */
>>> +    if ((last_state != priv->can.state) &&
>>> +        (priv->devtype_data->quirks &
>>> FLEXCAN_QUIRK_BROKEN_PERR_STATE) &&
>>> +       !(priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) {
>>
>> This block is only entered when FLEXCAN_QUIRK_BROKEN_PERR_STATE is set
>> but not for legacy cores. Or have I missed something.
>
>I need here:
>
>	    (priv->devtype_data->quirks & (FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>					   FLEXCAN_QUIRK_BROKEN_WERR_STATE)) &&
>
>Or maybe better, add FLEXCAN_QUIRK_BROKEN_PERR_STATE to the legacy cores
>as well.
The reason why not add FLEXCAN_QUIRK_BROKEN_WERR_STATE here is to leave
the untested cores which enabled legacy FLEXCAN_QUIRK_BROKEN_ERR_STATE
before will not be affected by this change, e.g., p1010. If these cores
are test in future, then the quirks can be updated to throttle the
interrupt flooding as well.

And to our understanding, there should be no existing FlexCAN core
which missing [TR]WRN_INT but having error passive interrupt, that
means FLEXCAN_QUIRK_BROKEN_WERR_STATE should never been set alone
if set the [PW]ERR_STATE quirks according to the hardware feature.

The combination of these two quirks should be:
1. FLEXCAN_QUIRK_BROKEN_PERR_STATE is set for new core
2. FLEXCAN_QUIRK_BROKEN_WERR_STATE | FLEXCAN_QUIRK_BROKEN_PERR_STATE
   are set for old core
3. None of these quirks been set for the future core which capable of
   generate state interrupt for all state transitions

If we add the WERR_STATE here, the pros is the old core will automatic
take advantage of this change, the cons is the quirks do not need to
match the exact hardware feature, which may lead to a bit chaos IMHO.

What do you think?

>
>>
>>> +        switch (priv->can.state) {
>>> +        case CAN_STATE_ERROR_ACTIVE:
>>> +            if (priv->devtype_data->quirks &
>>> +                FLEXCAN_QUIRK_BROKEN_WERR_STATE)
>>> +                flexcan_error_irq_enable(priv);
>>> +            else
>>> +                flexcan_error_irq_disable(priv);
>>> +            break;
>>> +
>>> +        case CAN_STATE_ERROR_WARNING:
>>> +            flexcan_error_irq_enable(priv);
>>> +            break;
>>> +
>>> +        case CAN_STATE_ERROR_PASSIVE:
>>> +        case CAN_STATE_BUS_OFF:
>>> +            flexcan_error_irq_disable(priv);
>>> +            break;
>>> +
>>> +        default:
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>>       return handled;
>>>   }
>>
>> Hope to find some time this week to test the patches on my i.MX53 and
>> i.MX28 boards.
>
>Here is the annotated trace when unplugging and reconnecting the cable:
>
># ip link set can0 txqueuelen 1000 up type can bitrate 1000000
># cansend can0 123#abcd
>flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x80 err=0/0
>
>*** disconnect CAN cable ***
>
># cansend can0 123#abcd
>flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x2042 err=8/0
>flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x22252 err=128/0
>flexcan 53fc8000.can can0: state 0->2 ctrl
>
>It jumps immediately to error passive!
On i.MX6 it doesn't jump, supprise of i.MX53 :)

>
>*** reconnect CAN cable ***
>
>flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x200 reg_esr=0x2292 err=135/0
>
>The error counter jumps to 135... weird!
A possible explanation is, while reconnecting, the error counter first
decrease by 1, and then increase by 8, so we get 135. But if this is
the case, due to the auto-retransmission will only sent one frame, that
means i.MX53 doesn't strict to the error counter decrease rule:
12.1.4.2 Error counting
g) After the successful transmission of a frame (getting ACK and no
   error has been detected until EOF is finished), the transmit error
   counter shall be decremented by 1 unless it was already 0.
So we should get either 127 if succeed, or 128 if ACK error detected.

>
># cansend can0 123#abcd
>flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x200 reg_esr=0x290 err=134/0
>...
># cansend can0 123#abcd
>flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x200 reg_esr=0x290 err=128/0
># cansend can0 123#abcd
>flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x200 reg_esr=0x280 err=127/0
>flexcan 53fc8000.can can0: state 2->1 ctrl
>
>Then it takes some transfers before error warning is reached.
>
># cansend can0 123#abcd
>flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x280 err=126/0
>...
># cansend can0 123#abcd
>flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x280 err=97/0
># cansend can0 123#abcd
>flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x280 err=96/0
># cansend can0 123#abcd
>flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x80 err=95/0
>flexcan 53fc8000.can can0: state 1->0 ctrl
># cansend can0 123#abcd
>flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x80 err=94/0
>
>*** disconnect CAN cable again ***
>
># cansend can0 123#abcd
>flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x22242 err=98/0
>flexcan 53fc8000.can can0: state 0->1 ctrl
>flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x2252 err=130/0
>flexcan 53fc8000.can can0: state 1->2 ctrl
>
>This time error warning is signalled". And here is with short-circuiting
>CAN low and high:
>
># cansend can0 123#abcd
>flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x24252 err=136/0
>flexcan 53fc8000.can can0: state 0->2 ctrl
>flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x0 reg_esr=0x4034 err=0/0
>flexcan 53fc8000.can can0: state 2->3 ctrl
># ip link set can0 type can restart
>
>Even if the state reporting is kind of weird with that core, interrupt flooding
>does not happen.
Great! The patch seems work on this core too. :)

BTW, do you apply the patch from the inline text of the e-mail? We are
curious of whether outlook 2013 is able to send inline patch or not.

Thanks!

Best regards
Yi

Re: [PATCH 3/4] can: flexcan: implement error passive state quirk

[Wolfgang Grandegger]

Hello ZHU Yi,

Am 07.09.2017 um 08:01 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
> Hello Wolfgang,
> 
>> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>> Sent: Thursday, September 07, 2017 4:17 AM
>> Hello ZHU Yi,
>>
>> now testing on an i.MX53 board (quirks=2):
>>
>> Am 06.09.2017 um 13:47 schrieb Wolfgang Grandegger:
[snip]
>> I need here:
>> 
>> 	    (priv->devtype_data->quirks & (FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>> 					   FLEXCAN_QUIRK_BROKEN_WERR_STATE)) &&
>> 
>> Or maybe better, add FLEXCAN_QUIRK_BROKEN_PERR_STATE to the legacy cores
>> as well.
> The reason why not add FLEXCAN_QUIRK_BROKEN_WERR_STATE here is to leave
> the untested cores which enabled legacy FLEXCAN_QUIRK_BROKEN_ERR_STATE
> before will not be affected by this change, e.g., p1010. If these cores
> are test in future, then the quirks can be updated to throttle the
> interrupt flooding as well.

I tested the i.MX53. The P1010 is an old PowerPC SOC. Can't tell if it's 
error handling is different.

> And to our understanding, there should be no existing FlexCAN core
> which missing [TR]WRN_INT but having error passive interrupt, that
> means FLEXCAN_QUIRK_BROKEN_WERR_STATE should never been set alone
> if set the [PW]ERR_STATE quirks according to the hardware feature.

Yep.

> The combination of these two quirks should be:
> 1. FLEXCAN_QUIRK_BROKEN_PERR_STATE is set for new core
> 2. FLEXCAN_QUIRK_BROKEN_WERR_STATE | FLEXCAN_QUIRK_BROKEN_PERR_STATE
>    are set for old core
> 3. None of these quirks been set for the future core which capable of
>    generate state interrupt for all state transitions

Yep.

> If we add the WERR_STATE here, the pros is the old core will automatic
> take advantage of this change, the cons is the quirks do not need to
> match the exact hardware feature, which may lead to a bit chaos IMHO.

Well, it's not always possible to make it right for all hardware 
variants. People will speak up in case of problems.

> What do you think?

I would set FLEXCAN_QUIRK_BROKEN_PERR_STATE for the legacy cores as 
well. I have an i.MX28 board here as well. It should not need the 
quirks. Will test it later this week.

[snip]
>> Here is the annotated trace when unplugging and reconnecting the cable:
>>
>> # ip link set can0 txqueuelen 1000 up type can bitrate 1000000
>> # cansend can0 123#abcd
>> flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x80 err=0/0
>>
>> *** disconnect CAN cable ***
>>
>> # cansend can0 123#abcd
>> flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x2042 err=8/0
>> flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x22252 err=128/0
>> flexcan 53fc8000.can can0: state 0->2 ctrl
>>
>> It jumps immediately to error passive!
> On i.MX6 it doesn't jump, supprise of i.MX53 :)
> 
>>
>> *** reconnect CAN cable ***
>>
>> flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x200 reg_esr=0x2292 err=135/0
>>
>> The error counter jumps to 135... weird!
> A possible explanation is, while reconnecting, the error counter first
> decrease by 1, and then increase by 8, so we get 135. But if this is
> the case, due to the auto-retransmission will only sent one frame, that
> means i.MX53 doesn't strict to the error counter decrease rule:
> 12.1.4.2 Error counting
> g) After the successful transmission of a frame (getting ACK and no
>     error has been detected until EOF is finished), the transmit error
>     counter shall be decremented by 1 unless it was already 0.
> So we should get either 127 if succeed, or 128 if ACK error detected.

Well, the error handling of that core is just buggy.

>> # cansend can0 123#abcd
>> flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x200 reg_esr=0x290 err=134/0
>> ...
>> # cansend can0 123#abcd
>> flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x200 reg_esr=0x290 err=128/0
>> # cansend can0 123#abcd
>> flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x200 reg_esr=0x280 err=127/0
>> flexcan 53fc8000.can can0: state 2->1 ctrl
>>
>> Then it takes some transfers before error warning is reached.
>>
>> # cansend can0 123#abcd
>> flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x280 err=126/0
>> ...
>> # cansend can0 123#abcd
>> flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x280 err=97/0
>> # cansend can0 123#abcd
>> flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x280 err=96/0
>> # cansend can0 123#abcd
>> flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x80 err=95/0
>> flexcan 53fc8000.can can0: state 1->0 ctrl
>> # cansend can0 123#abcd
>> flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x200 reg_esr=0x80 err=94/0
>>
>> *** disconnect CAN cable again ***
>>
>> # cansend can0 123#abcd
>> flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x22242 err=98/0
>> flexcan 53fc8000.can can0: state 0->1 ctrl
>> flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x2252 err=130/0
>> flexcan 53fc8000.can can0: state 1->2 ctrl
>>
>> This time error warning is signalled". And here is with short-circuiting
>> CAN low and high:
>>
>> # cansend can0 123#abcd
>> flexcan 53fc8000.can can0: ctrl=0x11aec53 reg_iflag=0x0 reg_esr=0x24252 err=136/0
>> flexcan 53fc8000.can can0: state 0->2 ctrl
>> flexcan 53fc8000.can can0: ctrl=0x11aac53 reg_iflag=0x0 reg_esr=0x4034 err=0/0
>> flexcan 53fc8000.can can0: state 2->3 ctrl
>> # ip link set can0 type can restart
>>
>> Even if the state reporting is kind of weird with that core, interrupt flooding
>> does not happen.
> Great! The patch seems work on this core too. :)

Yes, it works for the i.MX53, at least.

> BTW, do you apply the patch from the inline text of the e-mail? We are
> curious of whether outlook 2013 is able to send inline patch or not.

No problem, "git am" worked like a charm.

Wolfgang.

RE: [PATCH 3/4] can: flexcan: implement error passive state quirk

[ZHU Yi (ST-FIR/ENG1-Zhu)]

Hello Wolfgang,

>From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>Sent: Thursday, September 07, 2017 3:27 PM
>
>[snip]
>>> I need here:
>>>
>>> 	    (priv->devtype_data->quirks & (FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>>> 					   FLEXCAN_QUIRK_BROKEN_WERR_STATE)) &&
>>>
>>> Or maybe better, add FLEXCAN_QUIRK_BROKEN_PERR_STATE to the legacy cores
>>> as well.
>[snip]
>> If we add the WERR_STATE here, the pros is the old core will automatic
>> take advantage of this change, the cons is the quirks do not need to
>> match the exact hardware feature, which may lead to a bit chaos IMHO.
>
>Well, it's not always possible to make it right for all hardware
>variants. People will speak up in case of problems.
Agree.

>
>> What do you think?
>
>I would set FLEXCAN_QUIRK_BROKEN_PERR_STATE for the legacy cores as
>well. I have an i.MX28 board here as well. It should not need the
>quirks. Will test it later this week.
Great! According to i.MX28's datasheet, the guess is it should need
the FLEXCAN_QUIRK_BROKEN_PERR_STATE enabled as well as i.MX6.
I'm looking forward to the results and hope it works! :)

>
>[snip]
>>> Even if the state reporting is kind of weird with that core, interrupt flooding
>>> does not happen.
>> Great! The patch seems work on this core too. :)
>
>Yes, it works for the i.MX53, at least.
>
>> BTW, do you apply the patch from the inline text of the e-mail? We are
>> curious of whether outlook 2013 is able to send inline patch or not.
>
>No problem, "git am" worked like a charm.
That would be a big relief for us. We were worried about that we can't
send valid inline patch via outlook, and needs to deal with attachment
in the end.

Thanks for the info! :)

Best regards
Yi

Re: [PATCH 3/4] can: flexcan: implement error passive state quirk

[Wolfgang Grandegger]

Hello ZHU Yi,

Am 07.09.2017 um 10:11 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
> Hello Wolfgang,
> 
>> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>> Sent: Thursday, September 07, 2017 3:27 PM
>>
>> [snip]
>>>> I need here:
>>>>
>>>> 	    (priv->devtype_data->quirks & (FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>>>> 					   FLEXCAN_QUIRK_BROKEN_WERR_STATE)) &&
>>>>
>>>> Or maybe better, add FLEXCAN_QUIRK_BROKEN_PERR_STATE to the legacy cores
>>>> as well.
>> [snip]
>>> If we add the WERR_STATE here, the pros is the old core will automatic
>>> take advantage of this change, the cons is the quirks do not need to
>>> match the exact hardware feature, which may lead to a bit chaos IMHO.
>>
>> Well, it's not always possible to make it right for all hardware
>> variants. People will speak up in case of problems.
> Agree.
> 
>>
>>> What do you think?
>>
>> I would set FLEXCAN_QUIRK_BROKEN_PERR_STATE for the legacy cores as
>> well. I have an i.MX28 board here as well. It should not need the
>> quirks. Will test it later this week.
> Great! According to i.MX28's datasheet, the guess is it should need
> the FLEXCAN_QUIRK_BROKEN_PERR_STATE enabled as well as i.MX6.
> I'm looking forward to the results and hope it works! :)

Yes, it needs:

static const struct flexcan_devtype_data fsl_imx28_devtype_data = {
	.quirks = FLEXCAN_QUIRK_BROKEN_PERR_STATE,
};

Error warning is always reported and with the quirk above also error
passive. After reconnecting the cable the error counter also jumps to
135. With bus-off, the device also reports 3->0 after bus error
recovery:

[  674.862816] flexcan 80032000.can can0: ctrl=0x11aac53 reg_iflag=0x0 reg_esr=0x24252 err=138/1
[  674.871542] flexcan 80032000.can can0: state 0->2 ctrl
[  674.877038] flexcan 80032000.can can0: ctrl=0x11aac53 reg_iflag=0x0 reg_esr=0x4034 err=0/0
[  674.885837] flexcan 80032000.can can0: state 2->3 ctrl
root@m28evk:~# [  674.985897] flexcan 80032000.can can0: ctrl=0x11aa053 reg_iflag=0x0 reg_esr=0x4 err=0/0
[  674.994090] flexcan 80032000.can can0: state 3->0 ctrl

That was missing on the i.MX53.

Wolfgang.

RE: [PATCH 3/4] can: flexcan: implement error passive state quirk

[ZHU Yi (ST-FIR/ENG1-Zhu)]

Hello Wolfgang,

>From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>Sent: Friday, September 08, 2017 2:17 AM
>
> [...]
>Error warning is always reported and with the quirk above also error
>passive. After reconnecting the cable the error counter also jumps to
>135. With bus-off, the device also reports 3->0 after bus error
>recovery:

i.MX6 don't report 3->0 after bus error recovery (remove short-circuit!?):
[   74.222135] flexcan 2090000.flexcan mob0: New error state: 2
[   74.222170] flexcan 2090000.flexcan mob0: reg_ctrl=0x13aac57, reg_iflag1=0x0, reg_esr=0x64252, err=136/0
[   74.222915] flexcan 2090000.flexcan mob0: New error state: 3
[   74.222933] flexcan 2090000.flexcan mob0: bus-off
[   74.222979] flexcan 2090000.flexcan mob0: reg_ctrl=0x13aac57, reg_iflag1=0x0, reg_esr=0x4036, err=0/0

I'm wondering what is the correct behavior?

The automatic recovering from bus off is disabled in FlexCAN by write
1 to BOFF_REC of CTRL, see
http://elixir.free-electrons.com/linux/latest/source/drivers/net/can/flexcan.c#L883.

Hence after the core enters bus off, it needs to stay in this state
until "restart-ms" or manually restart, according to:
12.1.4.4 Bus-off management
[...]
Upon a restart request, a node which is in the bus-off state shall be
integrating to the CAN communication (see 10.9.4) and may become
error-active (no longer bus-off) with its error counters both set to
zero after having monitored 128 occurrences of the idle condition
(see 4.28) on the bus (see Figure 28).

But according to below trace, seems i.MX28 still change state to error
active even there is no restart request.

>
>[  674.862816] flexcan 80032000.can can0: ctrl=0x11aac53 reg_iflag=0x0 reg_esr=0x24252 err=138/1
ctrl: BOFF_MSK=1b, ERR_MSK=0b, [TR]WRN_MSK=11b
esr:  TWRN_INT=1b, FLT_CONF=01b
so the state change to error passive upon a "warning" interrupt.
>[  674.871542] flexcan 80032000.can can0: state 0->2 ctrl

>[  674.877038] flexcan 80032000.can can0: ctrl=0x11aac53 reg_iflag=0x0 reg_esr=0x4034 err=0/0
ctrl: BOFF_MSK=1b, ERR_MSK=0b, [TR]WRN_MSK=11b
esr:  BOFF_INT=1b, FLT_CONF=11b
so the state change to bus off upon a "bus off" interrupt.
>[  674.885837] flexcan 80032000.can can0: state 2->3 ctrl

>root@m28evk:~# [  674.985897] flexcan 80032000.can can0: ctrl=0x11aa053 reg_iflag=0x0 reg_esr=0x4 err=0/0
ctrl: BOFF_MSK=1b, ERR_MSK=0b, [TR]WRN_MSK=00b
esr:  BOFF_INT=1b, FLT_CONF=00b
so the state change to error active upon a "bus off" interrupt.
>[  674.994090] flexcan 80032000.can can0: state 3->0 ctrl

The strange thing is the BOFF_INT generated again without restart,
and the [TR]WRN_MSK are cleared to 0 when this happen. Because:
1. if the node enters bus off, it should not generate irq anymore (right?)
2. driver code won't only clear [TR]WRN_MSK but leave BOFF_MSK set

I'm not sure is the core still able to send/receive frame when it is
in this "error active" state (without restart).

If it still works, then the core just performs some magical
auto-recovery which violates the BOFF_REC setting.

If it cannot work, then that means wrong state been reported, and
perhaps another quirk needed for i.MX28 to report bus off instead
of error active in this case.

What do you think?

Best regards
Yi

Re: [PATCH 3/4] can: flexcan: implement error passive state quirk

[Wolfgang Grandegger]

Hello ZHU Yi,

Am 08.09.2017 um 08:10 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
> Hello Wolfgang,
> 
>> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>> Sent: Friday, September 08, 2017 2:17 AM
>>
>> [...]
>> Error warning is always reported and with the quirk above also error
>> passive. After reconnecting the cable the error counter also jumps to
>> 135. With bus-off, the device also reports 3->0 after bus error
>> recovery:
> 
> i.MX6 don't report 3->0 after bus error recovery (remove short-circuit!?):
> [   74.222135] flexcan 2090000.flexcan mob0: New error state: 2
> [   74.222170] flexcan 2090000.flexcan mob0: reg_ctrl=0x13aac57, reg_iflag1=0x0, reg_esr=0x64252, err=136/0
> [   74.222915] flexcan 2090000.flexcan mob0: New error state: 3
> [   74.222933] flexcan 2090000.flexcan mob0: bus-off
> [   74.222979] flexcan 2090000.flexcan mob0: reg_ctrl=0x13aac57, reg_iflag1=0x0, reg_esr=0x4036, err=0/0
> 
> I'm wondering what is the correct behavior?
> 
> The automatic recovering from bus off is disabled in FlexCAN by write
> 1 to BOFF_REC of CTRL, see
> http://elixir.free-electrons.com/linux/latest/source/drivers/net/can/flexcan.c#L883.
> 
> Hence after the core enters bus off, it needs to stay in this state
> until "restart-ms" or manually restart, according to:

I forgot to mention, I used "restart-ms 100" for my tests with the i.MX28.

> 12.1.4.4 Bus-off management
> [...]
> Upon a restart request, a node which is in the bus-off state shall be
> integrating to the CAN communication (see 10.9.4) and may become
> error-active (no longer bus-off) with its error counters both set to
> zero after having monitored 128 occurrences of the idle condition
> (see 4.28) on the bus (see Figure 28).

On most controllers we do not use the hardware recovery procedure but
just restart the controller. In the past I already had plans to change
that:

http://socket-can.996257.n3.nabble.com/RFC-improve-and-consolidate-state-change-and-bus-off-handling-td2755.html

> But according to below trace, seems i.MX28 still change state to error
> active even there is no restart request.

Restart after 100ms was set. BTW: a restart message is always sent:

http://elixir.free-electrons.com/linux/latest/source/drivers/net/can/dev.c#L545

>> [  674.862816] flexcan 80032000.can can0: ctrl=0x11aac53 reg_iflag=0x0 reg_esr=0x24252 err=138/1
> ctrl: BOFF_MSK=1b, ERR_MSK=0b, [TR]WRN_MSK=11b
> esr:  TWRN_INT=1b, FLT_CONF=01b
> so the state change to error passive upon a "warning" interrupt.
>> [  674.871542] flexcan 80032000.can can0: state 0->2 ctrl
> 
>> [  674.877038] flexcan 80032000.can can0: ctrl=0x11aac53 reg_iflag=0x0 reg_esr=0x4034 err=0/0
> ctrl: BOFF_MSK=1b, ERR_MSK=0b, [TR]WRN_MSK=11b
> esr:  BOFF_INT=1b, FLT_CONF=11b
> so the state change to bus off upon a "bus off" interrupt.
>> [  674.885837] flexcan 80032000.can can0: state 2->3 ctrl
> 
>> root@m28evk:~# [  674.985897] flexcan 80032000.can can0: ctrl=0x11aa053 reg_iflag=0x0 reg_esr=0x4 err=0/0

The log above is approx. 100ms after the previous one.

> ctrl: BOFF_MSK=1b, ERR_MSK=0b, [TR]WRN_MSK=00b
> esr:  BOFF_INT=1b, FLT_CONF=00b
> so the state change to error active upon a "bus off" interrupt.

No, after restart.

>> [  674.994090] flexcan 80032000.can can0: state 3->0 ctrl
> 
> The strange thing is the BOFF_INT generated again without restart,
> and the [TR]WRN_MSK are cleared to 0 when this happen. Because:
> 1. if the node enters bus off, it should not generate irq anymore (right?)

Yes, but some controller still do.

> 2. driver code won't only clear [TR]WRN_MSK but leave BOFF_MSK set
> 
> I'm not sure is the core still able to send/receive frame when it is
> in this "error active" state (without restart).
> 
> If it still works, then the core just performs some magical
> auto-recovery which violates the BOFF_REC setting.

Maybe.

> If it cannot work, then that means wrong state been reported, and
> perhaps another quirk needed for i.MX28 to report bus off instead
> of error active in this case.
> 
> What do you think?

As mentioned above, "restart 100" was used for the link. Unfortunately,
bus-off handling is not consistent for all CAN controllers, as mentioned
in the link above.

Wolfgang.

RE: [PATCH 3/4] can: flexcan: implement error passive state quirk

[ZHU Yi (ST-FIR/ENG1-Zhu)]

Hello Wolfgang,

>From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>Sent: Friday, September 08, 2017 3:02 PM
>
>>> [...]
>>> Error warning is always reported and with the quirk above also error
>>> passive. After reconnecting the cable the error counter also jumps to
>>> 135. With bus-off, the device also reports 3->0 after bus error
>>> recovery:
>> [...]
>I forgot to mention, I used "restart-ms 100" for my tests with the i.MX28.
Ah, okay, that explains a lot. :)

>
>> [...]
>On most controllers we do not use the hardware recovery procedure but
>just restart the controller. In the past I already had plans to change
>that:
>
>http://socket-can.996257.n3.nabble.com/RFC-improve-and-consolidate-state-change-and-bus-off-handling-td2755.html
That's a good idea. The can_change_state() was implemented, but the
rest seems need a long way to go.

>
>> [...]
>>> root@m28evk:~# [  674.985897] flexcan 80032000.can can0: ctrl=0x11aa053 reg_iflag=0x0 reg_esr=0x4 err=0/0
>
>The log above is approx. 100ms after the previous one.
>
>> ctrl: BOFF_MSK=1b, ERR_MSK=0b, [TR]WRN_MSK=00b
>> esr:  BOFF_INT=1b, FLT_CONF=00b
>> so the state change to error active upon a "bus off" interrupt.
>
>No, after restart.
OK, although it is still weird that [TR]WRN_MSK is 0 after restart.

>
>>> [...]
>As mentioned above, "restart 100" was used for the link. Unfortunately,
>bus-off handling is not consistent for all CAN controllers, as mentioned
>in the link above.
Yes, you're right. Perhaps "brute-force stop and restart the controller"
is still the most solid way to handle bus-off among all different CAN
controllers.

On the other hand, that means the patch also works for i.MX28 in regards
of throttle irq flooding after enabled the PERR_STATE quirk, right?

Best regards
Yi

RE: [PATCH 3/4] can: flexcan: implement error passive state quirk

[ZHU Yi (ST-FIR/ENG1-Zhu)]

Hello Wolfgang,

Thanks a lot for worked with us to improve and test the patches in
these days, and appreciate your valuable inputs for making the
patches better. :)

According to the test results, seems the patches are worked fine with
i.MX6, i.MX53 and i.MX28 at least, so we're wondering:
1. Do you think the patches are ready for accept, or there're something
   left we should work on? And what do you think we can help with to
   make the patches finalize?

2. After the patches are ready, could you please help to create patches
   for i.MX53 and i.MX28 as you already tested them? So that we can fix
   state transitions and throttle interrupt flooding for these 3 cores
   at least.

Best regards
Yi

Re: [PATCH 3/4] can: flexcan: implement error passive state quirk

[Wolfgang Grandegger]

Hello ZHU Yi,

Am 11.09.2017 um 04:29 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
> Hello Wolfgang,
> 
> Thanks a lot for worked with us to improve and test the patches in
> these days, and appreciate your valuable inputs for making the
> patches better. :)
> 
> According to the test results, seems the patches are worked fine with
> i.MX6, i.MX53 and i.MX28 at least, so we're wondering:
> 1. Do you think the patches are ready for accept, or there're something
>     left we should work on? And what do you think we can help with to
>     make the patches finalize?

The patches are fine. You can add my:

"Acked-by: Wolfgang Grandegger <wg@grandegger.com>

> 2. After the patches are ready, could you please help to create patches
>     for i.MX53 and i.MX28 as you already tested them? So that we can fix
>     state transitions and throttle interrupt flooding for these 3 cores
>     at least.

Could you please add the patch below for the i.MX28:

From c8c61fd02e016ff890a9a0f672cc63790bc8a408 Mon Sep 17 00:00:00 2001
From: Wolfgang Grandegger <wg@grandegger.com>
Date: Mon, 11 Sep 2017 08:59:53 +0200
Subject: [PATCH] can: flexcan: fix i.MX28 state transition issue

Enable FLEXCAN_QUIRK_BROKEN_PERR_STATE for i.MX28 to report correct
state transitions, especially to error passive.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/flexcan.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 57b37446..c9b3a1a 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -285,7 +285,9 @@ static const struct flexcan_devtype_data fsl_p1010_devtype_data = {
 	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE,
 };
 
-static const struct flexcan_devtype_data fsl_imx28_devtype_data;
+static const struct flexcan_devtype_data fsl_imx28_devtype_data = {
+	.quirks = FLEXCAN_QUIRK_BROKEN_PERR_STATE,
+};
 
 static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
 	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
-- 
2.7.4

I think we should add the flag also for the "p1010", even if I have
only tested it on the i.MX53:

static const struct flexcan_devtype_data fsl_p1010_devtype_data = {
	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE,
};

Thanks,

Wolfgang.

RE: [PATCH 3/4] can: flexcan: implement error passive state quirk

[ZHU Yi (ST-FIR/ENG1-Zhu)]

Hello Wolfgang,

>From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>Sent: Monday, September 11, 2017 3:06 PM
>
>Hello ZHU Yi,
>
>Am 11.09.2017 um 04:29 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
>> [...]
>
>The patches are fine. You can add my:
>
>"Acked-by: Wolfgang Grandegger <wg@grandegger.com>
Thanks! :)

>
>> [...]
>
>Could you please add the patch below for the i.MX28:
>
>From c8c61fd02e016ff890a9a0f672cc63790bc8a408 Mon Sep 17 00:00:00 2001
>From: Wolfgang Grandegger <wg@grandegger.com>
>Date: Mon, 11 Sep 2017 08:59:53 +0200
>Subject: [PATCH] can: flexcan: fix i.MX28 state transition issue
>
>Enable FLEXCAN_QUIRK_BROKEN_PERR_STATE for i.MX28 to report correct
>state transitions, especially to error passive.
>
>Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>---
> drivers/net/can/flexcan.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>index 57b37446..c9b3a1a 100644
>--- a/drivers/net/can/flexcan.c
>+++ b/drivers/net/can/flexcan.c
>@@ -285,7 +285,9 @@ static const struct flexcan_devtype_data fsl_p1010_devtype_data = {
> 	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE,
> };
>
>-static const struct flexcan_devtype_data fsl_imx28_devtype_data;
>+static const struct flexcan_devtype_data fsl_imx28_devtype_data = {
>+	.quirks = FLEXCAN_QUIRK_BROKEN_PERR_STATE,
>+};
>
> static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
> 	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
>--
>2.7.4
Sure, it's done! :)

>
>I think we should add the flag also for the "p1010", even if I have
>only tested it on the i.MX53:
>
>static const struct flexcan_devtype_data fsl_p1010_devtype_data = {
>	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE,
>};
The WERR_STATE is already added for p1010, do you mean we should add
the PERR_STATE to p1010 as well?

Best regards
Yi

Re: [PATCH 3/4] can: flexcan: implement error passive state quirk

[Wolfgang Grandegger]

Hello,

Am 12.09.2017 um 03:59 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
> Hello Wolfgang,
> 
>> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>> Sent: Monday, September 11, 2017 3:06 PM
>>
>> Hello ZHU Yi,
>>
>> Am 11.09.2017 um 04:29 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
>>> [...]
>>
>> The patches are fine. You can add my:
>>
>> "Acked-by: Wolfgang Grandegger <wg@grandegger.com>
> Thanks! :)
> 
>>
>>> [...]
>>
>> Could you please add the patch below for the i.MX28:
>>
>>From c8c61fd02e016ff890a9a0f672cc63790bc8a408 Mon Sep 17 00:00:00 2001
>> From: Wolfgang Grandegger <wg@grandegger.com>
>> Date: Mon, 11 Sep 2017 08:59:53 +0200
>> Subject: [PATCH] can: flexcan: fix i.MX28 state transition issue
>>
>> Enable FLEXCAN_QUIRK_BROKEN_PERR_STATE for i.MX28 to report correct
>> state transitions, especially to error passive.
>>
>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>> ---
>> drivers/net/can/flexcan.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>> index 57b37446..c9b3a1a 100644
>> --- a/drivers/net/can/flexcan.c
>> +++ b/drivers/net/can/flexcan.c
>> @@ -285,7 +285,9 @@ static const struct flexcan_devtype_data fsl_p1010_devtype_data = {
>> 	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE,
>> };
>>
>> -static const struct flexcan_devtype_data fsl_imx28_devtype_data;
>> +static const struct flexcan_devtype_data fsl_imx28_devtype_data = {
>> +	.quirks = FLEXCAN_QUIRK_BROKEN_PERR_STATE,
>> +};
>>
>> static const struct flexcan_devtype_data fsl_imx6q_devtype_data = {
>> 	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
>> --
>> 2.7.4
> Sure, it's done! :)
> 
>>
>> I think we should add the flag also for the "p1010", even if I have
>> only tested it on the i.MX53:
>>
>> static const struct flexcan_devtype_data fsl_p1010_devtype_data = {
>> 	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE,
>> };
> The WERR_STATE is already added for p1010, do you mean we should add
> the PERR_STATE to p1010 as well?

Yes, because bus error interrupt flooding may be a problem on these 
systems, especially low-end system. And WERR_STATE enables the bus error 
interrupt from the beginning.

Wolfgang.

RE: [PATCH 3/4] can: flexcan: implement error passive state quirk

[ZHU Yi (ST-FIR/ENG1-Zhu)]

Hello Wolfgang,

>From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>Sent: Tuesday, September 12, 2017 2:54 PM
>>> [...]
>>> I think we should add the flag also for the "p1010", even if I have
>>> only tested it on the i.MX53:
>>>
>>> static const struct flexcan_devtype_data fsl_p1010_devtype_data = {
>>> 	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE,
>>> };
>> The WERR_STATE is already added for p1010, do you mean we should add
>> the PERR_STATE to p1010 as well?
>
>Yes, because bus error interrupt flooding may be a problem on these
>systems, especially low-end system. And WERR_STATE enables the bus error
>interrupt from the beginning.
OK, I'll send the updated patch series later. :)

Thanks
Yi