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

[Wolfgang Grandegger <wg@grandegger.com>]

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.