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

[Andri Yngvason <andri.yngvason@marel.com>]

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.