[PATCH v4 4/6] can: flexcan: Consolidate and unify state change handling.

[PATCH v4 4/6] can: flexcan: Consolidate and unify state change handling.

[Andri Yngvason]

Replacing error state change handling with the new mechanism.

Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
---
Changes made since last proposal:
can: flexcan: add FLEXCAN_HAS_BROKEN_ERR_STATE for i.MX6
can: flexcan: adapt to newer can_change_state

 drivers/net/can/flexcan.c | 103 +++++++++-------------------------------------
 1 file changed, 19 insertions(+), 84 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 60f86bd..9f91735 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -266,7 +266,7 @@ static struct flexcan_devtype_data fsl_p1010_devtype_data = {
 };
 static struct flexcan_devtype_data fsl_imx28_devtype_data;
 static struct flexcan_devtype_data fsl_imx6q_devtype_data = {
-	.features = FLEXCAN_HAS_V10_FEATURES,
+	.features = FLEXCAN_HAS_V10_FEATURES | FLEXCAN_HAS_BROKEN_ERR_STATE,
 };
 static struct flexcan_devtype_data fsl_vf610_devtype_data = {
 	.features = FLEXCAN_HAS_V10_FEATURES | FLEXCAN_HAS_MECR_FEATURES,
@@ -577,98 +577,30 @@ static int flexcan_poll_bus_err(struct net_device *dev, u32 reg_esr)
 	return 1;
 }
 
-static void do_state(struct net_device *dev,
-		     struct can_frame *cf, enum can_state new_state)
-{
-	struct flexcan_priv *priv = netdev_priv(dev);
-	struct can_berr_counter bec;
-
-	__flexcan_get_berr_counter(dev, &bec);
-
-	switch (priv->can.state) {
-	case CAN_STATE_ERROR_ACTIVE:
-		/*
-		 * from: ERROR_ACTIVE
-		 * to  : ERROR_WARNING, ERROR_PASSIVE, BUS_OFF
-		 * =>  : there was a warning int
-		 */
-		if (new_state >= CAN_STATE_ERROR_WARNING &&
-		    new_state <= CAN_STATE_BUS_OFF) {
-			netdev_dbg(dev, "Error Warning IRQ\n");
-			priv->can.can_stats.error_warning++;
-
-			cf->can_id |= CAN_ERR_CRTL;
-			cf->data[1] = (bec.txerr > bec.rxerr) ?
-				CAN_ERR_CRTL_TX_WARNING :
-				CAN_ERR_CRTL_RX_WARNING;
-		}
-	case CAN_STATE_ERROR_WARNING:	/* fallthrough */
-		/*
-		 * from: ERROR_ACTIVE, ERROR_WARNING
-		 * to  : ERROR_PASSIVE, BUS_OFF
-		 * =>  : error passive int
-		 */
-		if (new_state >= CAN_STATE_ERROR_PASSIVE &&
-		    new_state <= CAN_STATE_BUS_OFF) {
-			netdev_dbg(dev, "Error Passive IRQ\n");
-			priv->can.can_stats.error_passive++;
-
-			cf->can_id |= CAN_ERR_CRTL;
-			cf->data[1] = (bec.txerr > bec.rxerr) ?
-				CAN_ERR_CRTL_TX_PASSIVE :
-				CAN_ERR_CRTL_RX_PASSIVE;
-		}
-		break;
-	case CAN_STATE_BUS_OFF:
-		netdev_err(dev, "BUG! "
-			   "hardware recovered automatically from BUS_OFF\n");
-		break;
-	default:
-		break;
-	}
-
-	/* process state changes depending on the new state */
-	switch (new_state) {
-	case CAN_STATE_ERROR_WARNING:
-		netdev_dbg(dev, "Error Warning\n");
-		cf->can_id |= CAN_ERR_CRTL;
-		cf->data[1] = (bec.txerr > bec.rxerr) ?
-			CAN_ERR_CRTL_TX_WARNING :
-			CAN_ERR_CRTL_RX_WARNING;
-		break;
-	case CAN_STATE_ERROR_ACTIVE:
-		netdev_dbg(dev, "Error Active\n");
-		cf->can_id |= CAN_ERR_PROT;
-		cf->data[2] = CAN_ERR_PROT_ACTIVE;
-		break;
-	case CAN_STATE_BUS_OFF:
-		cf->can_id |= CAN_ERR_BUSOFF;
-		can_bus_off(dev);
-		break;
-	default:
-		break;
-	}
-}
-
 static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)
 {
 	struct flexcan_priv *priv = netdev_priv(dev);
 	struct sk_buff *skb;
 	struct can_frame *cf;
-	enum can_state new_state;
+	enum can_state new_state = 0, rx_state = 0, tx_state = 0;
 	int flt;
+	struct can_berr_counter bec;
 
 	flt = reg_esr & FLEXCAN_ESR_FLT_CONF_MASK;
 	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))
+		tx_state = unlikely(reg_esr & FLEXCAN_ESR_TX_WRN) ?
+			   CAN_STATE_ERROR_WARNING : CAN_STATE_ERROR_ACTIVE;
+		rx_state = unlikely(reg_esr & FLEXCAN_ESR_RX_WRN) ?
+			   CAN_STATE_ERROR_WARNING : CAN_STATE_ERROR_ACTIVE;
+		new_state = max(tx_state, rx_state);
+	} else if (unlikely(flt == FLEXCAN_ESR_FLT_CONF_PASSIVE)) {
+		__flexcan_get_berr_counter(dev, &bec);
 		new_state = CAN_STATE_ERROR_PASSIVE;
-	else
+		rx_state = bec.rxerr >= bec.txerr ? new_state : 0;
+		tx_state = bec.rxerr <= bec.txerr ? new_state : 0;
+	} else {
 		new_state = CAN_STATE_BUS_OFF;
+	}
 
 	/* state hasn't changed */
 	if (likely(new_state == priv->can.state))
@@ -678,8 +610,11 @@ static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)
 	if (unlikely(!skb))
 		return 0;
 
-	do_state(dev, cf, new_state);
-	priv->can.state = new_state;
+	can_change_state(dev, cf, tx_state, rx_state);
+
+	if (unlikely(new_state == CAN_STATE_BUS_OFF))
+		can_bus_off(dev);
+
 	netif_receive_skb(skb);
 
 	dev->stats.rx_packets++;
-- 
1.9.1

Re: [PATCH v4 4/6] can: flexcan: Consolidate and unify state change handling.

[Wolfgang Grandegger]

On 11/28/2014 01:12 PM, Andri Yngvason wrote:
> Replacing error state change handling with the new mechanism.
> 
> Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
> ---
> Changes made since last proposal:
> can: flexcan: add FLEXCAN_HAS_BROKEN_ERR_STATE for i.MX6
> can: flexcan: adapt to newer can_change_state
> 
>  drivers/net/can/flexcan.c | 103 +++++++++-------------------------------------
>  1 file changed, 19 insertions(+), 84 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 60f86bd..9f91735 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -266,7 +266,7 @@ static struct flexcan_devtype_data fsl_p1010_devtype_data = {
>  };
>  static struct flexcan_devtype_data fsl_imx28_devtype_data;
>  static struct flexcan_devtype_data fsl_imx6q_devtype_data = {
> -	.features = FLEXCAN_HAS_V10_FEATURES,
> +	.features = FLEXCAN_HAS_V10_FEATURES | FLEXCAN_HAS_BROKEN_ERR_STATE,

Oops, this change is not related to the subject! Anyway, did it cure
your problems with state handling. Is it required for all i.MX6 cores then?

>  };
>  static struct flexcan_devtype_data fsl_vf610_devtype_data = {
>  	.features = FLEXCAN_HAS_V10_FEATURES | FLEXCAN_HAS_MECR_FEATURES,
> @@ -577,98 +577,30 @@ static int flexcan_poll_bus_err(struct net_device *dev, u32 reg_esr)
>  	return 1;
>  }
>  
> -static void do_state(struct net_device *dev,
> -		     struct can_frame *cf, enum can_state new_state)
> -{
> -	struct flexcan_priv *priv = netdev_priv(dev);
> -	struct can_berr_counter bec;
> -
> -	__flexcan_get_berr_counter(dev, &bec);
> -
> -	switch (priv->can.state) {
> -	case CAN_STATE_ERROR_ACTIVE:
> -		/*
> -		 * from: ERROR_ACTIVE
> -		 * to  : ERROR_WARNING, ERROR_PASSIVE, BUS_OFF
> -		 * =>  : there was a warning int
> -		 */
> -		if (new_state >= CAN_STATE_ERROR_WARNING &&
> -		    new_state <= CAN_STATE_BUS_OFF) {
> -			netdev_dbg(dev, "Error Warning IRQ\n");
> -			priv->can.can_stats.error_warning++;
> -
> -			cf->can_id |= CAN_ERR_CRTL;
> -			cf->data[1] = (bec.txerr > bec.rxerr) ?
> -				CAN_ERR_CRTL_TX_WARNING :
> -				CAN_ERR_CRTL_RX_WARNING;
> -		}
> -	case CAN_STATE_ERROR_WARNING:	/* fallthrough */
> -		/*
> -		 * from: ERROR_ACTIVE, ERROR_WARNING
> -		 * to  : ERROR_PASSIVE, BUS_OFF
> -		 * =>  : error passive int
> -		 */
> -		if (new_state >= CAN_STATE_ERROR_PASSIVE &&
> -		    new_state <= CAN_STATE_BUS_OFF) {
> -			netdev_dbg(dev, "Error Passive IRQ\n");

Maybe it's a good idea to have netdev_dbg's for state changes in
can_change_state() as well. Other opinions? Marc?

Apart from these to comments the patches 1-5 look good (but you should
drop the 6th).

Wolfgang.

Re: [PATCH v4 4/6] can: flexcan: Consolidate and unify state change handling.

[Andri Yngvason]

Quoting Wolfgang Grandegger (2014-11-30 20:23:57)
> On 11/28/2014 01:12 PM, Andri Yngvason wrote:
> > Replacing error state change handling with the new mechanism.
> > 
> > Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
> > ---
> > Changes made since last proposal:
> > can: flexcan: add FLEXCAN_HAS_BROKEN_ERR_STATE for i.MX6
> > can: flexcan: adapt to newer can_change_state
> > 
> >  drivers/net/can/flexcan.c | 103 +++++++++-------------------------------------
> >  1 file changed, 19 insertions(+), 84 deletions(-)
> > 
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index 60f86bd..9f91735 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -266,7 +266,7 @@ static struct flexcan_devtype_data fsl_p1010_devtype_data = {
> >  };
> >  static struct flexcan_devtype_data fsl_imx28_devtype_data;
> >  static struct flexcan_devtype_data fsl_imx6q_devtype_data = {
> > -     .features = FLEXCAN_HAS_V10_FEATURES,
> > +     .features = FLEXCAN_HAS_V10_FEATURES | FLEXCAN_HAS_BROKEN_ERR_STATE,
> 
> Oops, this change is not related to the subject! Anyway, did it cure
> your problems with state handling. Is it required for all i.MX6 cores then?
>
Yeah, I accidentally squashed this; intended to make this a separate patch. This
helps. It has the same effect as enabling berr-reporting. However, this only
cures one of the problems. This does not help with the case where no one else is
sending on the bus.

This problem is not limited to i.MX6. This is a problem for ALL FlexCAN cores.
This is a design flaw in FlexCAN cores. Maybe they did this part on a monday
after a weekend of binge-drinking. In any case, they really messed up with this
one.

I suggest that we remove the FLEXCAN_HAS_BROKEN_ERR_STATE flag and turn on error
interrupts permanently because they ALL have broken error state. 
> 
> >  };
> >  static struct flexcan_devtype_data fsl_vf610_devtype_data = {
> >       .features = FLEXCAN_HAS_V10_FEATURES | FLEXCAN_HAS_MECR_FEATURES,
> > @@ -577,98 +577,30 @@ static int flexcan_poll_bus_err(struct net_device *dev, u32 reg_esr)
> >       return 1;
> >  }
> >  
> > -static void do_state(struct net_device *dev,
> > -                  struct can_frame *cf, enum can_state new_state)
> > -{
> > -     struct flexcan_priv *priv = netdev_priv(dev);
> > -     struct can_berr_counter bec;
> > -
> > -     __flexcan_get_berr_counter(dev, &bec);
> > -
> > -     switch (priv->can.state) {
> > -     case CAN_STATE_ERROR_ACTIVE:
> > -             /*
> > -              * from: ERROR_ACTIVE
> > -              * to  : ERROR_WARNING, ERROR_PASSIVE, BUS_OFF
> > -              * =>  : there was a warning int
> > -              */
> > -             if (new_state >= CAN_STATE_ERROR_WARNING &&
> > -                 new_state <= CAN_STATE_BUS_OFF) {
> > -                     netdev_dbg(dev, "Error Warning IRQ\n");
> > -                     priv->can.can_stats.error_warning++;
> > -
> > -                     cf->can_id |= CAN_ERR_CRTL;
> > -                     cf->data[1] = (bec.txerr > bec.rxerr) ?
> > -                             CAN_ERR_CRTL_TX_WARNING :
> > -                             CAN_ERR_CRTL_RX_WARNING;
> > -             }
> > -     case CAN_STATE_ERROR_WARNING:   /* fallthrough */
> > -             /*
> > -              * from: ERROR_ACTIVE, ERROR_WARNING
> > -              * to  : ERROR_PASSIVE, BUS_OFF
> > -              * =>  : error passive int
> > -              */
> > -             if (new_state >= CAN_STATE_ERROR_PASSIVE &&
> > -                 new_state <= CAN_STATE_BUS_OFF) {
> > -                     netdev_dbg(dev, "Error Passive IRQ\n");
> 
> Maybe it's a good idea to have netdev_dbg's for state changes in
> can_change_state() as well. Other opinions? Marc?
> 
That might be useful. I'll add it.
>
> Apart from these to comments the patches 1-5 look good (but you should
> drop the 6th).
> 
Will do.

--
Andri

Re: [PATCH v4 4/6] can: flexcan: Consolidate and unify state change handling.

[Wolfgang Grandegger]

On Mon, 1 Dec 2014 11:09:23 +0000, Andri Yngvason
<andri.yngvason@marel.com> wrote:
> Quoting Wolfgang Grandegger (2014-11-30 20:23:57)
>> On 11/28/2014 01:12 PM, Andri Yngvason wrote:
>> > Replacing error state change handling with the new mechanism.
>> > 
>> > Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
>> > ---
>> > Changes made since last proposal:
>> > can: flexcan: add FLEXCAN_HAS_BROKEN_ERR_STATE for i.MX6
>> > can: flexcan: adapt to newer can_change_state
>> > 
>> >  drivers/net/can/flexcan.c | 103
>> >  +++++++++-------------------------------------
>> >  1 file changed, 19 insertions(+), 84 deletions(-)
>> > 
>> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>> > index 60f86bd..9f91735 100644
>> > --- a/drivers/net/can/flexcan.c
>> > +++ b/drivers/net/can/flexcan.c
>> > @@ -266,7 +266,7 @@ static struct flexcan_devtype_data
>> > fsl_p1010_devtype_data = {
>> >  };
>> >  static struct flexcan_devtype_data fsl_imx28_devtype_data;
>> >  static struct flexcan_devtype_data fsl_imx6q_devtype_data = {
>> > -     .features = FLEXCAN_HAS_V10_FEATURES,
>> > +     .features = FLEXCAN_HAS_V10_FEATURES |
>> > FLEXCAN_HAS_BROKEN_ERR_STATE,
>> 
>> Oops, this change is not related to the subject! Anyway, did it cure
>> your problems with state handling. Is it required for all i.MX6 cores
>> then?
>>
> Yeah, I accidentally squashed this; intended to make this a separate
> patch. This
> helps. It has the same effect as enabling berr-reporting. However, this
> only
> cures one of the problems. This does not help with the case where no one
> else is
> sending on the bus.
> 
> This problem is not limited to i.MX6. This is a problem for ALL FlexCAN
> cores.
> This is a design flaw in FlexCAN cores. Maybe they did this part on a
> monday
> after a weekend of binge-drinking. In any case, they really messed up
with
> this
> one.

#define FLEXCAN_HAS_BROKEN_ERR_STATE    BIT(2) /* [TR]WRN_INT not
connected */

They actually forgot to connect the [TR]WRN_INT line. Don't know what they
did the days before ;).

> I suggest that we remove the FLEXCAN_HAS_BROKEN_ERR_STATE flag and turn
on
> error
> interrupts permanently because they ALL have broken error state. 

Hm, how did you come to this conclusion? You just tested on an i.MX6,
right?
Anyway, the bus error reporting can cause very high CPU load and it should
be off when it's not needed.

Wolfgang.

Re: [PATCH v4 4/6] can: flexcan: Consolidate and unify state change handling.

[Andri Yngvason]

Quoting Wolfgang Grandegger (2014-12-01 11:37:03)
> On Mon, 1 Dec 2014 11:09:23 +0000, Andri Yngvason
> <andri.yngvason@marel.com> wrote:
> > Quoting Wolfgang Grandegger (2014-11-30 20:23:57)
> >> On 11/28/2014 01:12 PM, Andri Yngvason wrote:
> >> > Replacing error state change handling with the new mechanism.
> >> > 
> >> > Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
> >> > ---
> >> > Changes made since last proposal:
> >> > can: flexcan: add FLEXCAN_HAS_BROKEN_ERR_STATE for i.MX6
> >> > can: flexcan: adapt to newer can_change_state
> >> > 
> >> >  drivers/net/can/flexcan.c | 103
> >> >  +++++++++-------------------------------------
> >> >  1 file changed, 19 insertions(+), 84 deletions(-)
> >> > 
> >> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> >> > index 60f86bd..9f91735 100644
> >> > --- a/drivers/net/can/flexcan.c
> >> > +++ b/drivers/net/can/flexcan.c
> >> > @@ -266,7 +266,7 @@ static struct flexcan_devtype_data
> >> > fsl_p1010_devtype_data = {
> >> >  };
> >> >  static struct flexcan_devtype_data fsl_imx28_devtype_data;
> >> >  static struct flexcan_devtype_data fsl_imx6q_devtype_data = {
> >> > -     .features = FLEXCAN_HAS_V10_FEATURES,
> >> > +     .features = FLEXCAN_HAS_V10_FEATURES |
> >> > FLEXCAN_HAS_BROKEN_ERR_STATE,
> >> 
> >> Oops, this change is not related to the subject! Anyway, did it cure
> >> your problems with state handling. Is it required for all i.MX6 cores
> >> then?
> >>
> > Yeah, I accidentally squashed this; intended to make this a separate
> > patch. This
> > helps. It has the same effect as enabling berr-reporting. However, this
> > only
> > cures one of the problems. This does not help with the case where no one
> > else is
> > sending on the bus.
> > 
> > This problem is not limited to i.MX6. This is a problem for ALL FlexCAN
> > cores.
> > This is a design flaw in FlexCAN cores. Maybe they did this part on a
> > monday
> > after a weekend of binge-drinking. In any case, they really messed up
> with
> > this
> > one.
> 
> #define FLEXCAN_HAS_BROKEN_ERR_STATE    BIT(2) /* [TR]WRN_INT not
> connected */
> 
> They actually forgot to connect the [TR]WRN_INT line. Don't know what they
> did the days before ;).
> 
That's not a problem on the i.MX6, but that doesn't matter. The problem is that
there are no interrupts for the other state transitions. Whether WRN_INT is
connected or not isn't even relevant. WRN_INT is only triggered when
transitioning from error-active to error-warning. Everything else is missing by
design.
>
> > I suggest that we remove the FLEXCAN_HAS_BROKEN_ERR_STATE flag and turn
> on
> > error
> > interrupts permanently because they ALL have broken error state. 
> 
> Hm, how did you come to this conclusion? You just tested on an i.MX6,
> right?
> Anyway, the bus error reporting can cause very high CPU load and it should
> be off when it's not needed.
> 
I don't need to test this on other cores because, it can be logically derived
from the manual that interrupts for error state transitions were not a part of
the design.

This may sound quite incredible, but it is my understanding that this is indeed
the case. I really do hope that I'm wrong, but I doubt it.

--
Andri.

Re: [PATCH v4 4/6] can: flexcan: Consolidate and unify state change handling.

[Wolfgang Grandegger]

On Mon, 1 Dec 2014 11:51:58 +0000, Andri Yngvason
<andri.yngvason@marel.com> wrote:
> Quoting Wolfgang Grandegger (2014-12-01 11:37:03)
>> On Mon, 1 Dec 2014 11:09:23 +0000, Andri Yngvason
>> <andri.yngvason@marel.com> wrote:
>> > Quoting Wolfgang Grandegger (2014-11-30 20:23:57)
>> >> On 11/28/2014 01:12 PM, Andri Yngvason wrote:
>> >> > Replacing error state change handling with the new mechanism.
>> >> > 
>> >> > Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
>> >> > ---
>> >> > Changes made since last proposal:
>> >> > can: flexcan: add FLEXCAN_HAS_BROKEN_ERR_STATE for i.MX6
>> >> > can: flexcan: adapt to newer can_change_state
>> >> > 
>> >> >  drivers/net/can/flexcan.c | 103
>> >> >  +++++++++-------------------------------------
>> >> >  1 file changed, 19 insertions(+), 84 deletions(-)
>> >> > 
>> >> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>> >> > index 60f86bd..9f91735 100644
>> >> > --- a/drivers/net/can/flexcan.c
>> >> > +++ b/drivers/net/can/flexcan.c
>> >> > @@ -266,7 +266,7 @@ static struct flexcan_devtype_data
>> >> > fsl_p1010_devtype_data = {
>> >> >  };
>> >> >  static struct flexcan_devtype_data fsl_imx28_devtype_data;
>> >> >  static struct flexcan_devtype_data fsl_imx6q_devtype_data = {
>> >> > -     .features = FLEXCAN_HAS_V10_FEATURES,
>> >> > +     .features = FLEXCAN_HAS_V10_FEATURES |
>> >> > FLEXCAN_HAS_BROKEN_ERR_STATE,
>> >> 
>> >> Oops, this change is not related to the subject! Anyway, did it cure
>> >> your problems with state handling. Is it required for all i.MX6
cores
>> >> then?
>> >>
>> > Yeah, I accidentally squashed this; intended to make this a separate
>> > patch. This
>> > helps. It has the same effect as enabling berr-reporting. However,
this
>> > only
>> > cures one of the problems. This does not help with the case where no
>> > one
>> > else is
>> > sending on the bus.
>> > 
>> > This problem is not limited to i.MX6. This is a problem for ALL
FlexCAN
>> > cores.
>> > This is a design flaw in FlexCAN cores. Maybe they did this part on a
>> > monday
>> > after a weekend of binge-drinking. In any case, they really messed up
>> with
>> > this
>> > one.
>> 
>> #define FLEXCAN_HAS_BROKEN_ERR_STATE    BIT(2) /* [TR]WRN_INT not
>> connected */
>> 
>> They actually forgot to connect the [TR]WRN_INT line. Don't know what
>> they
>> did the days before ;).
>> 
> That's not a problem on the i.MX6, but that doesn't matter. The problem
is
> that
> there are no interrupts for the other state transitions. Whether WRN_INT
is
> connected or not isn't even relevant. WRN_INT is only triggered when
> transitioning from error-active to error-warning. Everything else is
> missing by
> design.

But that's another issue.

>> > I suggest that we remove the FLEXCAN_HAS_BROKEN_ERR_STATE flag and
turn
>> on
>> > error
>> > interrupts permanently because they ALL have broken error state. 
>> 
>> Hm, how did you come to this conclusion? You just tested on an i.MX6,
>> right?
>> Anyway, the bus error reporting can cause very high CPU load and it
>> should
>> be off when it's not needed.
>> 
> I don't need to test this on other cores because, it can be logically
> derived
> from the manual that interrupts for error state transitions were not a
> part of
> the design.
> 
> This may sound quite incredible, but it is my understanding that this is
> indeed
> the case. I really do hope that I'm wrong, but I doubt it.

As I said before. The error reporting is perfect on the SJA1000 and more
(for the Flexcan) or less worse on the other CAN controllers.

Wolfgang.

Re: [PATCH v4 4/6] can: flexcan: Consolidate and unify state change handling.

[Andri Yngvason]

Quoting Wolfgang Grandegger (2014-12-01 12:02:33)
> On Mon, 1 Dec 2014 11:51:58 +0000, Andri Yngvason
> <andri.yngvason@marel.com> wrote:
> > Quoting Wolfgang Grandegger (2014-12-01 11:37:03)
> >> On Mon, 1 Dec 2014 11:09:23 +0000, Andri Yngvason
> >> <andri.yngvason@marel.com> wrote:
> >> > Quoting Wolfgang Grandegger (2014-11-30 20:23:57)
> >> >> On 11/28/2014 01:12 PM, Andri Yngvason wrote:
> >> >> > Replacing error state change handling with the new mechanism.
> >> >> > 
> >> >> > Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
> >> >> > ---
> >> >> > Changes made since last proposal:
> >> >> > can: flexcan: add FLEXCAN_HAS_BROKEN_ERR_STATE for i.MX6
> >> >> > can: flexcan: adapt to newer can_change_state
> >> >> > 
> >> >> >  drivers/net/can/flexcan.c | 103
> >> >> >  +++++++++-------------------------------------
> >> >> >  1 file changed, 19 insertions(+), 84 deletions(-)
> >> >> > 
> >> >> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> >> >> > index 60f86bd..9f91735 100644
> >> >> > --- a/drivers/net/can/flexcan.c
> >> >> > +++ b/drivers/net/can/flexcan.c
> >> >> > @@ -266,7 +266,7 @@ static struct flexcan_devtype_data
> >> >> > fsl_p1010_devtype_data = {
> >> >> >  };
> >> >> >  static struct flexcan_devtype_data fsl_imx28_devtype_data;
> >> >> >  static struct flexcan_devtype_data fsl_imx6q_devtype_data = {
> >> >> > -     .features = FLEXCAN_HAS_V10_FEATURES,
> >> >> > +     .features = FLEXCAN_HAS_V10_FEATURES |
> >> >> > FLEXCAN_HAS_BROKEN_ERR_STATE,
> >> >> 
> >> >> Oops, this change is not related to the subject! Anyway, did it cure
> >> >> your problems with state handling. Is it required for all i.MX6
> cores
> >> >> then?
> >> >>
> >> > Yeah, I accidentally squashed this; intended to make this a separate
> >> > patch. This
> >> > helps. It has the same effect as enabling berr-reporting. However,
> this
> >> > only
> >> > cures one of the problems. This does not help with the case where no
> >> > one
> >> > else is
> >> > sending on the bus.
> >> > 
> >> > This problem is not limited to i.MX6. This is a problem for ALL
> FlexCAN
> >> > cores.
> >> > This is a design flaw in FlexCAN cores. Maybe they did this part on a
> >> > monday
> >> > after a weekend of binge-drinking. In any case, they really messed up
> >> with
> >> > this
> >> > one.
> >> 
> >> #define FLEXCAN_HAS_BROKEN_ERR_STATE    BIT(2) /* [TR]WRN_INT not
> >> connected */
> >> 
> >> They actually forgot to connect the [TR]WRN_INT line. Don't know what
> >> they
> >> did the days before ;).
> >> 
> > That's not a problem on the i.MX6, but that doesn't matter. The problem
> is
> > that
> > there are no interrupts for the other state transitions. Whether WRN_INT
> is
> > connected or not isn't even relevant. WRN_INT is only triggered when
> > transitioning from error-active to error-warning. Everything else is
> > missing by
> > design.
> 
> But that's another issue.
>
Yes, that's the issue that I'm having that requires me to turn on
berr-reporting or else turn on the bus error interrupts, which is what the flag
does to deal with the missing connections.
> 
> >> > I suggest that we remove the FLEXCAN_HAS_BROKEN_ERR_STATE flag and
> turn
> >> on
> >> > error
> >> > interrupts permanently because they ALL have broken error state. 
> >> 
> >> Hm, how did you come to this conclusion? You just tested on an i.MX6,
> >> right?
> >> Anyway, the bus error reporting can cause very high CPU load and it
> >> should
> >> be off when it's not needed.
> >> 
> > I don't need to test this on other cores because, it can be logically
> > derived
> > from the manual that interrupts for error state transitions were not a
> > part of
> > the design.
> > 
> > This may sound quite incredible, but it is my understanding that this is
> > indeed
> > the case. I really do hope that I'm wrong, but I doubt it.
> 
> As I said before. The error reporting is perfect on the SJA1000 and more
> (for the Flexcan) or less worse on the other CAN controllers.
> 

Yes. In any case, the warning interrupt is irrelevant, because we have to poll
the state anyway for the other states. Thus the FLEXCAN_HAS_BROKEN_ERR_STATE
flag is irrelevant.

The question that remains is: Should we enable bus error interrupts for all
flexcan cores or should we ignore the issue and allow the users to work around
it (if they wish) using the berr-reporting flag?

There is a middle-ground here: We could enable bus error interrupts when we get
the error-warning interrupt and disable them again when the state has reached
error-active again. In that case we would want to keep the BROKEN_ERR_STATE
flag.

--
Andri

Re: [PATCH v4 4/6] can: flexcan: Consolidate and unify state change handling.

[Wolfgang Grandegger]

On 12/01/2014 01:22 PM, Andri Yngvason wrote:
> Quoting Wolfgang Grandegger (2014-12-01 12:02:33)
>> On Mon, 1 Dec 2014 11:51:58 +0000, Andri Yngvason
>> <andri.yngvason@marel.com> wrote:
>>> Quoting Wolfgang Grandegger (2014-12-01 11:37:03)
>>>> On Mon, 1 Dec 2014 11:09:23 +0000, Andri Yngvason
>>>> <andri.yngvason@marel.com> wrote:
>>>>> Quoting Wolfgang Grandegger (2014-11-30 20:23:57)
>>>>>> On 11/28/2014 01:12 PM, Andri Yngvason wrote:
>>>>>>> Replacing error state change handling with the new mechanism.
>>>>>>>
>>>>>>> Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
>>>>>>> ---
>>>>>>> Changes made since last proposal:
>>>>>>> can: flexcan: add FLEXCAN_HAS_BROKEN_ERR_STATE for i.MX6
>>>>>>> can: flexcan: adapt to newer can_change_state
>>>>>>>
>>>>>>>  drivers/net/can/flexcan.c | 103
>>>>>>>  +++++++++-------------------------------------
>>>>>>>  1 file changed, 19 insertions(+), 84 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>>>>>>> index 60f86bd..9f91735 100644
>>>>>>> --- a/drivers/net/can/flexcan.c
>>>>>>> +++ b/drivers/net/can/flexcan.c
>>>>>>> @@ -266,7 +266,7 @@ static struct flexcan_devtype_data
>>>>>>> fsl_p1010_devtype_data = {
>>>>>>>  };
>>>>>>>  static struct flexcan_devtype_data fsl_imx28_devtype_data;
>>>>>>>  static struct flexcan_devtype_data fsl_imx6q_devtype_data = {
>>>>>>> -     .features = FLEXCAN_HAS_V10_FEATURES,
>>>>>>> +     .features = FLEXCAN_HAS_V10_FEATURES |
>>>>>>> FLEXCAN_HAS_BROKEN_ERR_STATE,
>>>>>>
>>>>>> Oops, this change is not related to the subject! Anyway, did it cure
>>>>>> your problems with state handling. Is it required for all i.MX6
>> cores
>>>>>> then?
>>>>>>
>>>>> Yeah, I accidentally squashed this; intended to make this a separate
>>>>> patch. This
>>>>> helps. It has the same effect as enabling berr-reporting. However,
>> this
>>>>> only
>>>>> cures one of the problems. This does not help with the case where no
>>>>> one
>>>>> else is
>>>>> sending on the bus.
>>>>>
>>>>> This problem is not limited to i.MX6. This is a problem for ALL
>> FlexCAN
>>>>> cores.
>>>>> This is a design flaw in FlexCAN cores. Maybe they did this part on a
>>>>> monday
>>>>> after a weekend of binge-drinking. In any case, they really messed up
>>>> with
>>>>> this
>>>>> one.
>>>>
>>>> #define FLEXCAN_HAS_BROKEN_ERR_STATE    BIT(2) /* [TR]WRN_INT not
>>>> connected */
>>>>
>>>> They actually forgot to connect the [TR]WRN_INT line. Don't know what
>>>> they
>>>> did the days before ;).
>>>>
>>> That's not a problem on the i.MX6, but that doesn't matter. The problem
>> is
>>> that
>>> there are no interrupts for the other state transitions. Whether WRN_INT
>> is
>>> connected or not isn't even relevant. WRN_INT is only triggered when
>>> transitioning from error-active to error-warning. Everything else is
>>> missing by
>>> design.
>>
>> But that's another issue.
>>
> Yes, that's the issue that I'm having that requires me to turn on
> berr-reporting or else turn on the bus error interrupts, which is what the flag
> does to deal with the missing connections.
>>
>>>>> I suggest that we remove the FLEXCAN_HAS_BROKEN_ERR_STATE flag and
>> turn
>>>> on
>>>>> error
>>>>> interrupts permanently because they ALL have broken error state. 
>>>>
>>>> Hm, how did you come to this conclusion? You just tested on an i.MX6,
>>>> right?
>>>> Anyway, the bus error reporting can cause very high CPU load and it
>>>> should
>>>> be off when it's not needed.
>>>>
>>> I don't need to test this on other cores because, it can be logically
>>> derived
>>> from the manual that interrupts for error state transitions were not a
>>> part of
>>> the design.
>>>
>>> This may sound quite incredible, but it is my understanding that this is
>>> indeed
>>> the case. I really do hope that I'm wrong, but I doubt it.
>>
>> As I said before. The error reporting is perfect on the SJA1000 and more
>> (for the Flexcan) or less worse on the other CAN controllers.
>>
> 
> Yes. In any case, the warning interrupt is irrelevant, because we have to poll
> the state anyway for the other states. Thus the FLEXCAN_HAS_BROKEN_ERR_STATE
> flag is irrelevant.

Well, it improves the situation a little bit. But now I understand your
point. Yes the Flexcan core is buggy in this respect.

> The question that remains is: Should we enable bus error interrupts for all
> flexcan cores or should we ignore the issue and allow the users to work around
> it (if they wish) using the berr-reporting flag?

Bus error reporting sometimes really harms and therefore I would leave
it as-is. There are also more recent cores and it would be nice to known
if they have improved the reporting of state changes further. Anyway,
this issue should be addressed by a separate patch series.

> There is a middle-ground here: We could enable bus error interrupts when we get
> the error-warning interrupt and disable them again when the state has reached
> error-active again. In that case we would want to keep the BROKEN_ERR_STATE
> flag.

Puh, that's far too sophisticated.

Wolfgang.

Re: [PATCH v4 4/6] can: flexcan: Consolidate and unify state change handling.

[Marc Kleine-Budde]

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

On 12/01/2014 08:42 PM, Wolfgang Grandegger wrote:
[...]
>> Yes. In any case, the warning interrupt is irrelevant, because we have to poll
>> the state anyway for the other states. Thus the FLEXCAN_HAS_BROKEN_ERR_STATE
>> flag is irrelevant.
> 
> Well, it improves the situation a little bit. But now I understand your
> point. Yes the Flexcan core is buggy in this respect.

Yes, just a bit :) This is probably why they added the m_can core on the
new imx6 soloX.

>> The question that remains is: Should we enable bus error interrupts for all
>> flexcan cores or should we ignore the issue and allow the users to work around
>> it (if they wish) using the berr-reporting flag?
> 
> Bus error reporting sometimes really harms and therefore I would leave
> it as-is. There are also more recent cores and it would be nice to known
> if they have improved the reporting of state changes further. Anyway,
> this issue should be addressed by a separate patch series.

>> There is a middle-ground here: We could enable bus error interrupts when we get
>> the error-warning interrupt and disable them again when the state has reached
>> error-active again. In that case we would want to keep the BROKEN_ERR_STATE
>> flag.
> 
> Puh, that's far too sophisticated.

I agree with Wolfgang. The Bus errors can make a system unresponsive to
unusable. There is still the bus error limiting patch lurking around,
which works around this problem.

regards,
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: 819 bytes --]

Re: [PATCH v4 4/6] can: flexcan: Consolidate and unify state change handling.

[Andri Yngvason]

Quoting Marc Kleine-Budde (2014-12-02 12:49:24)
> On 12/01/2014 08:42 PM, Wolfgang Grandegger wrote:
> [...]
> >> Yes. In any case, the warning interrupt is irrelevant, because we have to poll
> >> the state anyway for the other states. Thus the FLEXCAN_HAS_BROKEN_ERR_STATE
> >> flag is irrelevant.
> > 
> > Well, it improves the situation a little bit. But now I understand your
> > point. Yes the Flexcan core is buggy in this respect.
> 
> Yes, just a bit :) This is probably why they added the m_can core on the
> new imx6 soloX.
>
Interesting! Hopefully they'll make a quadX too. ;)
> 
> >> The question that remains is: Should we enable bus error interrupts for all
> >> flexcan cores or should we ignore the issue and allow the users to work around
> >> it (if they wish) using the berr-reporting flag?
> > 
> > Bus error reporting sometimes really harms and therefore I would leave
> > it as-is. There are also more recent cores and it would be nice to known
> > if they have improved the reporting of state changes further. Anyway,
> > this issue should be addressed by a separate patch series.
> 
> >> There is a middle-ground here: We could enable bus error interrupts when we get
> >> the error-warning interrupt and disable them again when the state has reached
> >> error-active again. In that case we would want to keep the BROKEN_ERR_STATE
> >> flag.
> > 
> > Puh, that's far too sophisticated.
> 
> I agree with Wolfgang. The Bus errors can make a system unresponsive to
> unusable. There is still the bus error limiting patch lurking around,
> which works around this problem.
> 
That's fine by me. It's good enough for me anyway, as is. I might get some
testing done on Saturday. 

--
Andri

Re: [PATCH v4 4/6] can: flexcan: Consolidate and unify state change handling.

[Marc Kleine-Budde]

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

On 12/02/2014 02:22 PM, Andri Yngvason wrote:
> That's fine by me. It's good enough for me anyway, as is. I might get some
> testing done on Saturday. 

I'll be on vacation then.

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: 819 bytes --]