[PATCH 1/4 RESEND] can: flexcan: Disable error interrupt when bus error reporting is disabled

[PATCH 1/4 RESEND] can: flexcan: Disable error interrupt when bus error reporting is disabled

[Alexander Stein]

In case we don't have FLEXCAN_HAS_BROKEN_ERR_STATE and the user set
CAN_CTRLMODE_BERR_REPORTING once it can not be unset again until reboot.
So in case neither hardware nor user wants the error interrupt disable
the bit.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/net/can/flexcan.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 33aea7eb9337..fd79dd40840b 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -862,6 +862,8 @@ static int flexcan_chip_start(struct net_device *dev)
 	if (priv->devtype_data->features & FLEXCAN_HAS_BROKEN_ERR_STATE ||
 	    priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
 		reg_ctrl |= FLEXCAN_CTRL_ERR_MSK;
+	else
+		reg_ctrl &= ~FLEXCAN_CTRL_ERR_MSK;
 
 	/* save for later use */
 	priv->reg_ctrl_default = reg_ctrl;
-- 
1.8.5.5

[PATCH 2/4] can: flexcan: Detect error passive state change

[Alexander Stein]

Assuming there is no CAN frame reception or CAN_CTRLMODE_BERR_REPORTING:
If the Tx error counter steps from 127 to 128 the hardware enters error
passive. But in this case the FLEXCAN_ESR_ERR_STATE bits are unset:
TWRN_INT and RWRN_INT are not set during the transition from <96 to >=96.
So we do not schedule NAPI thus not evaluating the error counters.
So we also check TX_WRN and RX_WRN in order to detect error passive.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/net/can/flexcan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index fd79dd40840b..5e391ec70a2b 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -721,7 +721,8 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	 * - bus error IRQ and bus error reporting is activated
 	 */
 	if ((reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) ||
-	    (reg_esr & FLEXCAN_ESR_ERR_STATE) ||
+	    (reg_esr & (FLEXCAN_ESR_ERR_STATE |
+			FLEXCAN_ESR_TX_WRN | FLEXCAN_ESR_RX_WRN)) ||
 	    flexcan_has_and_handle_berr(priv, reg_esr)) {
 		/*
 		 * The error bits are cleared on read,
-- 
1.8.5.5

Re: [PATCH 2/4] can: flexcan: Detect error passive state change

[Marc Kleine-Budde]

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

On 08/12/2014 03:43 PM, Alexander Stein wrote:
> Assuming there is no CAN frame reception or CAN_CTRLMODE_BERR_REPORTING:
> If the Tx error counter steps from 127 to 128 the hardware enters error
> passive. But in this case the FLEXCAN_ESR_ERR_STATE bits are unset:
> TWRN_INT and RWRN_INT are not set during the transition from <96 to >=96.

Which hardware are you using? According to the documentation TWRN_INT
and RWRN_INT _are_ actually set during the transition from <96 to >=96.

> So we do not schedule NAPI thus not evaluating the error counters.
> So we also check TX_WRN and RX_WRN in order to detect error passive.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  drivers/net/can/flexcan.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index fd79dd40840b..5e391ec70a2b 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -721,7 +721,8 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  	 * - bus error IRQ and bus error reporting is activated
>  	 */
>  	if ((reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) ||
> -	    (reg_esr & FLEXCAN_ESR_ERR_STATE) ||
> +	    (reg_esr & (FLEXCAN_ESR_ERR_STATE |
> +			FLEXCAN_ESR_TX_WRN | FLEXCAN_ESR_RX_WRN)) ||
>  	    flexcan_has_and_handle_berr(priv, reg_esr)) {
>  		/*
>  		 * The error bits are cleared on read,
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/4] can: flexcan: Detect error passive state change

[Alexander Stein]

On Friday 15 August 2014 10:07:06, Marc Kleine-Budde wrote:
> On 08/12/2014 03:43 PM, Alexander Stein wrote:
> > Assuming there is no CAN frame reception or CAN_CTRLMODE_BERR_REPORTING:
> > If the Tx error counter steps from 127 to 128 the hardware enters error
> > passive. But in this case the FLEXCAN_ESR_ERR_STATE bits are unset:
> > TWRN_INT and RWRN_INT are not set during the transition from <96 to >=96.
> 
> Which hardware are you using? According to the documentation TWRN_INT
> and RWRN_INT _are_ actually set during the transition from <96 to >=96.

Meh, wrong case description... What I wanted to write:
TWRN_INT and RWRN_INT are _only_ set during the transition from <96 to >=96.
And for the records I'm using a i.MX35.

> > So we do not schedule NAPI thus not evaluating the error counters.
> > So we also check TX_WRN and RX_WRN in order to detect error passive.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> > ---
> >  drivers/net/can/flexcan.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index fd79dd40840b..5e391ec70a2b 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -721,7 +721,8 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> >  	 * - bus error IRQ and bus error reporting is activated
> >  	 */
> >  	if ((reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) ||
> > -	    (reg_esr & FLEXCAN_ESR_ERR_STATE) ||
> > +	    (reg_esr & (FLEXCAN_ESR_ERR_STATE |
> > +			FLEXCAN_ESR_TX_WRN | FLEXCAN_ESR_RX_WRN)) ||
> >  	    flexcan_has_and_handle_berr(priv, reg_esr)) {
> >  		/*
> >  		 * The error bits are cleared on read,
> > 
> 
> Marc
> 
> 

-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: alexander.stein@systec-electronic.com
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082

[PATCH 3/4] can: flexcan: Put enabling error irq into separate function

[Alexander Stein]

This is a preparation for a further patch disabling/enabling that irq
when needed. This has no functional changes.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/net/can/flexcan.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 5e391ec70a2b..65102e5456c2 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -563,6 +563,18 @@ static void do_state(struct net_device *dev,
 	}
 }
 
+static void flexcan_enable_err_irq(struct flexcan_priv *priv)
+{
+	/* Enable the flag in reg_ctrl_default which will be written
+	 * in flexcan_poll
+	 */
+	if (priv->devtype_data->features & FLEXCAN_HAS_BROKEN_ERR_STATE ||
+	    priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
+		priv->reg_ctrl_default |= FLEXCAN_CTRL_ERR_MSK;
+	else
+		priv->reg_ctrl_default &= ~FLEXCAN_CTRL_ERR_MSK;
+}
+
 static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)
 {
 	struct flexcan_priv *priv = netdev_priv(dev);
@@ -855,21 +867,20 @@ static int flexcan_chip_start(struct net_device *dev)
 	reg_ctrl &= ~FLEXCAN_CTRL_TSYN;
 	reg_ctrl |= FLEXCAN_CTRL_BOFF_REC | FLEXCAN_CTRL_LBUF |
 		FLEXCAN_CTRL_ERR_STATE;
+
+	/* save for later use */
+	priv->reg_ctrl_default = reg_ctrl;
+
 	/*
 	 * enable the "error interrupt" (FLEXCAN_CTRL_ERR_MSK),
 	 * on most Flexcan cores, too. Otherwise we don't get
 	 * any error warning or passive interrupts.
 	 */
-	if (priv->devtype_data->features & FLEXCAN_HAS_BROKEN_ERR_STATE ||
-	    priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
-		reg_ctrl |= FLEXCAN_CTRL_ERR_MSK;
-	else
-		reg_ctrl &= ~FLEXCAN_CTRL_ERR_MSK;
+	flexcan_enable_err_irq(priv);
 
-	/* save for later use */
-	priv->reg_ctrl_default = reg_ctrl;
-	netdev_dbg(dev, "%s: writing ctrl=0x%08x", __func__, reg_ctrl);
-	flexcan_write(reg_ctrl, &regs->ctrl);
+	netdev_dbg(dev, "%s: writing ctrl=0x%08x", __func__,
+		   priv->reg_ctrl_default);
+	flexcan_write(priv->reg_ctrl_default, &regs->ctrl);
 
 	/* Abort any pending TX, mark Mailbox as INACTIVE */
 	flexcan_write(FLEXCAN_MB_CNT_CODE(0x4),
-- 
1.8.5.5

[PATCH 4/4] can: flexcan: Disable error irq when in error passive

[Alexander Stein]

If the error irq is enabled and the hardware is trying to send a CAN
message to an open bus it will receive a lot of acknowledge errors and
generating lots of interrupts. So error interrupts are disabled when
entering error passive and reenable them when leaving error passive.
We can only leave error passive by receiving CAN frames in which case NAPI
is scheduled anyway or by sending more CAN frames when send is possible
again. But with the transition of error counters from 96 to 95 the flags
TX_WRN and RX_WRN are gone again, so we need to remember the flags state
to detect a change there.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/net/can/flexcan.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 65102e5456c2..9f604c14ad92 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -207,6 +207,7 @@ struct flexcan_priv {
 
 	void __iomem *base;
 	u32 reg_esr;
+	int warn;
 	u32 reg_ctrl_default;
 
 	struct clk *clk_ipg;
@@ -575,6 +576,14 @@ static void flexcan_enable_err_irq(struct flexcan_priv *priv)
 		priv->reg_ctrl_default &= ~FLEXCAN_CTRL_ERR_MSK;
 }
 
+static void flexcan_disable_err_irq(struct flexcan_priv *priv)
+{
+	/* Disable the flag in reg_ctrl_default which will be written
+	 * in flexcan_poll
+	 */
+	priv->reg_ctrl_default &= ~FLEXCAN_CTRL_ERR_MSK;
+}
+
 static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)
 {
 	struct flexcan_priv *priv = netdev_priv(dev);
@@ -584,15 +593,19 @@ static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)
 	int flt;
 
 	flt = reg_esr & FLEXCAN_ESR_FLT_CONF_MASK;
+	priv->warn = reg_esr & (FLEXCAN_ESR_TX_WRN | FLEXCAN_ESR_RX_WRN);
 	if (likely(flt == FLEXCAN_ESR_FLT_CONF_ACTIVE)) {
 		if (likely(!(reg_esr & (FLEXCAN_ESR_TX_WRN |
 					FLEXCAN_ESR_RX_WRN))))
 			new_state = CAN_STATE_ERROR_ACTIVE;
 		else
 			new_state = CAN_STATE_ERROR_WARNING;
-	} else if (unlikely(flt == FLEXCAN_ESR_FLT_CONF_PASSIVE))
+
+		flexcan_enable_err_irq(priv);
+	} else if (unlikely(flt == FLEXCAN_ESR_FLT_CONF_PASSIVE)) {
 		new_state = CAN_STATE_ERROR_PASSIVE;
-	else
+		flexcan_disable_err_irq(priv);
+	} else
 		new_state = CAN_STATE_BUS_OFF;
 
 	/* state hasn't changed */
@@ -735,6 +748,8 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	if ((reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) ||
 	    (reg_esr & (FLEXCAN_ESR_ERR_STATE |
 			FLEXCAN_ESR_TX_WRN | FLEXCAN_ESR_RX_WRN)) ||
+	    (priv->warn !=
+		(reg_esr & (FLEXCAN_ESR_TX_WRN | FLEXCAN_ESR_RX_WRN))) ||
 	    flexcan_has_and_handle_berr(priv, reg_esr)) {
 		/*
 		 * The error bits are cleared on read,
-- 
1.8.5.5

Re: [PATCH 4/4] can: flexcan: Disable error irq when in error passive

[Marc Kleine-Budde]

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

On 08/12/2014 03:43 PM, Alexander Stein wrote:
> If the error irq is enabled and the hardware is trying to send a CAN
> message to an open bus it will receive a lot of acknowledge errors and
> generating lots of interrupts. So error interrupts are disabled when
> entering error passive and reenable them when leaving error passive.
> We can only leave error passive by receiving CAN frames in which case NAPI
> is scheduled anyway or by sending more CAN frames when send is possible
> again. But with the transition of error counters from 96 to 95 the flags
> TX_WRN and RX_WRN are gone again, so we need to remember the flags state
> to detect a change there.

There are use cases where the error interrupts even when in passive mode
are essential. You have a use case, where you'd like to turn them off
completely. Have you had a look at the bus error limiting patches?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 4/4] can: flexcan: Disable error irq when in error passive

[Alexander Stein]

On Thursday 14 August 2014 13:18:47, Marc Kleine-Budde wrote:
> On 08/12/2014 03:43 PM, Alexander Stein wrote:
> > If the error irq is enabled and the hardware is trying to send a CAN
> > message to an open bus it will receive a lot of acknowledge errors and
> > generating lots of interrupts. So error interrupts are disabled when
> > entering error passive and reenable them when leaving error passive.
> > We can only leave error passive by receiving CAN frames in which case NAPI
> > is scheduled anyway or by sending more CAN frames when send is possible
> > again. But with the transition of error counters from 96 to 95 the flags
> > TX_WRN and RX_WRN are gone again, so we need to remember the flags state
> > to detect a change there.
> 
> There are use cases where the error interrupts even when in passive mode
> are essential. You have a use case, where you'd like to turn them off
> completely. Have you had a look at the bus error limiting patches?

Do you mean "[PATCH v7] flexcan: add err_irq handler for flexcan" from Zhao Qiang?

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: alexander.stein@systec-electronic.com
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082

Re: [PATCH 4/4] can: flexcan: Disable error irq when in error passive

[Marc Kleine-Budde]

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

On 08/14/2014 03:19 PM, Alexander Stein wrote:
> On Thursday 14 August 2014 13:18:47, Marc Kleine-Budde wrote:
>> On 08/12/2014 03:43 PM, Alexander Stein wrote:
>>> If the error irq is enabled and the hardware is trying to send a CAN
>>> message to an open bus it will receive a lot of acknowledge errors and
>>> generating lots of interrupts. So error interrupts are disabled when
>>> entering error passive and reenable them when leaving error passive.
>>> We can only leave error passive by receiving CAN frames in which case NAPI
>>> is scheduled anyway or by sending more CAN frames when send is possible
>>> again. But with the transition of error counters from 96 to 95 the flags
>>> TX_WRN and RX_WRN are gone again, so we need to remember the flags state
>>> to detect a change there.
>>
>> There are use cases where the error interrupts even when in passive mode
>> are essential. You have a use case, where you'd like to turn them off
>> completely. Have you had a look at the bus error limiting patches?
> 
> Do you mean "[PATCH v7] flexcan: add err_irq handler for flexcan" from Zhao Qiang?

No, sorry I assumes you were following the discussion with Matthias
Klein and Sebastian Andrzej Siewior [1].

I was talking about the patch from
http://comments.gmane.org/gmane.linux.can/4070

The idea behind this series is to not enable the bus error interrupt
right after it has served, but after a configurable delay, e.g. 100ms.
Even with an open or otherwise faulty but, you then receive just 10
error frames per second.

Marc

[1] http://thread.gmane.org/gmane.linux.can/6271
[2] http://thread.gmane.org/gmane.linux.can/6280

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 4/4] can: flexcan: Disable error irq when in error passive

[Alexander Stein]

On Thursday 14 August 2014 15:58:19, Marc Kleine-Budde wrote:
> On 08/14/2014 03:19 PM, Alexander Stein wrote:
> > On Thursday 14 August 2014 13:18:47, Marc Kleine-Budde wrote:
> >> On 08/12/2014 03:43 PM, Alexander Stein wrote:
> >>> If the error irq is enabled and the hardware is trying to send a CAN
> >>> message to an open bus it will receive a lot of acknowledge errors and
> >>> generating lots of interrupts. So error interrupts are disabled when
> >>> entering error passive and reenable them when leaving error passive.
> >>> We can only leave error passive by receiving CAN frames in which case NAPI
> >>> is scheduled anyway or by sending more CAN frames when send is possible
> >>> again. But with the transition of error counters from 96 to 95 the flags
> >>> TX_WRN and RX_WRN are gone again, so we need to remember the flags state
> >>> to detect a change there.
> >>
> >> There are use cases where the error interrupts even when in passive mode
> >> are essential. You have a use case, where you'd like to turn them off
> >> completely. Have you had a look at the bus error limiting patches?
> > 
> > Do you mean "[PATCH v7] flexcan: add err_irq handler for flexcan" from Zhao Qiang?
> 
> No, sorry I assumes you were following the discussion with Matthias
> Klein and Sebastian Andrzej Siewior [1].
> 
> I was talking about the patch from
> http://comments.gmane.org/gmane.linux.can/4070
> 
> The idea behind this series is to not enable the bus error interrupt
> right after it has served, but after a configurable delay, e.g. 100ms.
> Even with an open or otherwise faulty but, you then receive just 10
> error frames per second.
> 
> Marc
> 
> [1] http://thread.gmane.org/gmane.linux.can/6271
> [2] http://thread.gmane.org/gmane.linux.can/6280

I didn't follow this thread. I mostly used the flexcan driver but noticed now some hickups.

Regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: alexander.stein@systec-electronic.com
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082

Re: [PATCH 1/4 RESEND] can: flexcan: Disable error interrupt when bus error reporting is disabled

[Marc Kleine-Budde]

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

On 08/12/2014 03:43 PM, Alexander Stein wrote:
> In case we don't have FLEXCAN_HAS_BROKEN_ERR_STATE and the user set
> CAN_CTRLMODE_BERR_REPORTING once it can not be unset again until reboot.
> So in case neither hardware nor user wants the error interrupt disable
> the bit.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>

For completeness: Applied to can, see other mail.

Thanks,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]