[PATCH 1/6] can: flexcan: fix state transition regression

[PATCH 1/6] can: flexcan: fix state transition regression

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

From 08db02ac30d379838be511bc12b851dad1ef8363 Mon Sep 17 00:00:00 2001
From: Zhu Yi <yi.zhu5@cn.bosch.com>
Date: Wed, 30 Aug 2017 13:57:50 +0800
Subject: [PATCH 1/6] can: flexcan: fix state transition regression

Update state upon any interrupt to report correct state transitions
in case the flexcan core enabled the broken error state quirk fix.

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 13f0f21..df4bfb8 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -765,8 +765,9 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 		flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT, &regs->esr);
 	}
 
-	/* state change interrupt */
-	if (reg_esr & FLEXCAN_ESR_ERR_STATE)
+	/* 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_ERR_STATE))
 		flexcan_irq_state(dev, reg_esr);
 
 	/* bus error IRQ - handle if bus error reporting is activated */
-- 
2.7.4

Re: [PATCH 1/6] can: flexcan: fix state transition regression

[Wolfgang Grandegger]

Hello ZHU Yi,

usually a [PATCH 0/x] is coming first telling what the series does. We
use "git format-patch --cover-letter ..." for that purpose.

Am 31.08.2017 um 10:24 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
>>From 08db02ac30d379838be511bc12b851dad1ef8363 Mon Sep 17 00:00:00 2001
> From: Zhu Yi <yi.zhu5@cn.bosch.com>
> Date: Wed, 30 Aug 2017 13:57:50 +0800
> Subject: [PATCH 1/6] can: flexcan: fix state transition regression
> 
> Update state upon any interrupt to report correct state transitions
> in case the flexcan core enabled the broken error state quirk fix.
> 
> 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 | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 13f0f21..df4bfb8 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -765,8 +765,9 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>   		flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT, &regs->esr);
>   	}
>   
> -	/* state change interrupt */
> -	if (reg_esr & FLEXCAN_ESR_ERR_STATE)
> +	/* 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_ERR_STATE))
>   		flexcan_irq_state(dev, reg_esr);

This means that flexcan_irq_state() is called for any interrupt source.
That's what we had in the pre rx-offload version. Thinking about it:
do we need that (overhead for RX and TX)? Maybe the following works already:

 +	if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
 +	    ((priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_ERR_STATE) &&
 +		(reg_esr & FLEXCAN_ESR_ERR_STATE)))

>   
>   	/* bus error IRQ - handle if bus error reporting is activated */
> 

RE: [PATCH 1/6] can: flexcan: fix state transition regression

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

Hello Wolfgang,

> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
> Sent: Thursday, August 31, 2017 7:37 PM
> 
> Hello ZHU Yi,
> 
> usually a [PATCH 0/x] is coming first telling what the series does. We
> use "git format-patch --cover-letter ..." for that purpose.
OK. Thanks for your hint!
I'll prepare the updated patch series with cover letter later.

> 
> Am 31.08.2017 um 10:24 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
> [...]
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index 13f0f21..df4bfb8 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -765,8 +765,9 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> >   		flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT, &regs->esr);
> >   	}
> >
> > -	/* state change interrupt */
> > -	if (reg_esr & FLEXCAN_ESR_ERR_STATE)
> > +	/* 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_ERR_STATE))
> >   		flexcan_irq_state(dev, reg_esr);
> 
> This means that flexcan_irq_state() is called for any interrupt source.
> That's what we had in the pre rx-offload version. Thinking about it:
> do we need that (overhead for RX and TX)? Maybe the following works already:
> 
>  +	if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
>  +	    ((priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_ERR_STATE) &&
>  +		(reg_esr & FLEXCAN_ESR_ERR_STATE)))
I'm not sure that I understood your proposal correctly, the guess is
the second "reg_esr & FLEXCAN_ESR_ERR_STATE" is a typo, do you mean
"reg_esr & FLEXCAN_ESR_BUS_ERR"?

If we only call flexcan_irq_state() upon state + error interrupt, then:
(new core: with    [TR]WRN_INT, e.g., i.MX6,
 old core: without [TR]WRN_INT, e.g., i.MX53)
1. error state increases, enough for both new and old cores.
    active ---> warning ---> passive ---> bus off
                   ^            ^            ^
                WRN_INT/     ERR_INT      BOFF_INT
	        ERR_INT(old core)

2. error state decreases, not enough for both new and old cores.
    active <--- warning <--- passive
       ^           ^
    [TR]X       WRN_INT/
	        [TR]X(old core)
The [TR]X interrupt is required for the state decreases, and the
overhead of them IMHO is not overkill:
1. If flexcan_irq_state() is called when the state is not change,
   then, the overhead is read FLT_CONF and/or read error counter,
   do O(1) calculation and compare the state, then quit.

2. If flexcan_irq_state() is called when the state is changed,
   then, that's what help us to report correct state transitions.

Of course, if flexcan core can generate appropriate state interrupts
in its next version (FlexCAN4?), then we don't need such overhead
at all, but when will this wish come true? :)

Best regards
Yi

Re: [PATCH 1/6] can: flexcan: fix state transition regression

[Wolfgang Grandegger]

Hello ZHU Yi,

Am 01.09.2017 um 04:48 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
> Hello Wolfgang,
> 
>> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>> Sent: Thursday, August 31, 2017 7:37 PM
>>
>> Hello ZHU Yi,
>>
>> usually a [PATCH 0/x] is coming first telling what the series does. We
>> use "git format-patch --cover-letter ..." for that purpose.
> OK. Thanks for your hint!
> I'll prepare the updated patch series with cover letter later.

You patches are also white-space mangled.

>>
>> Am 31.08.2017 um 10:24 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
>> [...]
>>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>>> index 13f0f21..df4bfb8 100644
>>> --- a/drivers/net/can/flexcan.c
>>> +++ b/drivers/net/can/flexcan.c
>>> @@ -765,8 +765,9 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>>>    		flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT, &regs->esr);
>>>    	}
>>>
>>> -	/* state change interrupt */
>>> -	if (reg_esr & FLEXCAN_ESR_ERR_STATE)
>>> +	/* 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_ERR_STATE))
>>>    		flexcan_irq_state(dev, reg_esr);
>>
>> This means that flexcan_irq_state() is called for any interrupt source.
>> That's what we had in the pre rx-offload version. Thinking about it:
>> do we need that (overhead for RX and TX)? Maybe the following works already:
>>
>>   +	if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
>>   +	    ((priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_ERR_STATE) &&
>>   +		(reg_esr & FLEXCAN_ESR_ERR_STATE)))
> I'm not sure that I understood your proposal correctly, the guess is
> the second "reg_esr & FLEXCAN_ESR_ERR_STATE" is a typo, do you mean
> "reg_esr & FLEXCAN_ESR_BUS_ERR"

Typo, sorry, I mean "(reg_esr & FLEXCAN_ESR_ERR_BUS)".


> If we only call flexcan_irq_state() upon state + error interrupt, then:
> (new core: with    [TR]WRN_INT, e.g., i.MX6,
>   old core: without [TR]WRN_INT, e.g., i.MX53)

The "FLEXCAN_QUIRK_BROKEN_PERR_STATE" is for the i.MX6. Only thinking 
about the core here.

> 1. error state increases, enough for both new and old cores.
>      active ---> warning ---> passive ---> bus off
>                     ^            ^            ^
>                  WRN_INT/     ERR_INT      BOFF_INT
> 	        ERR_INT(old core)
> 
> 2. error state decreases, not enough for both new and old cores.
>      active <--- warning <--- passive
>         ^           ^
>      [TR]X       WRN_INT/
> 	        [TR]X(old core)

I was oping that the error warning interrupt occurs when the error 
counter falls again below error passive. Then we could enable the error 
interrupt on "active -> waning" and disable it on "passive -> warning" 
state changes. If that does work, than we can disable the error 
interrupt not before the "warning -> active" change.

> The [TR]X interrupt is required for the state decreases, and the
> overhead of them IMHO is not overkill:
> 1. If flexcan_irq_state() is called when the state is not change,
>     then, the overhead is read FLT_CONF and/or read error counter,
>     do O(1) calculation and compare the state, then quit.

Yes, there is just a little overhead.

> 2. If flexcan_irq_state() is called when the state is changed,
>     then, that's what help us to report correct state transitions.

The only difference between "new core" and "old core":

new core: error interrupt enabled between error warning and passive.
old core: error interrupt enabled between error active and passive.

Not sure it it's worth the extra code.

> Of course, if flexcan core can generate appropriate state interrupts
> in its next version (FlexCAN4?), then we don't need such overhead
> at all, but when will this wish come true? :)

Not on the i.MX6q, for sure ;).

Wolfgang.

RE: [PATCH 1/6] can: flexcan: fix state transition regression

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

Hello Wolfgang,

> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
> Sent: Friday, September 01, 2017 11:15 PM
> 
> [...]
> >> usually a [PATCH 0/x] is coming first telling what the series does. We
> >> use "git format-patch --cover-letter ..." for that purpose.
> > OK. Thanks for your hint!
> > I'll prepare the updated patch series with cover letter later.
> 
> You patches are also white-space mangled.
Perhaps because of the outlook. Although I've tested send to myself and
copy the text then use "git am" to apply, it works in my local, but
seems can not work when it send out. :(

Can we send the updated patches in separate e-mails, with attachments
instead of inline text? Because we cannot easily switch to other
mail client...

Or, do you have other proposal?

> 
> [...]
> > I'm not sure that I understood your proposal correctly, the guess is
> > the second "reg_esr & FLEXCAN_ESR_ERR_STATE" is a typo, do you mean
> > "reg_esr & FLEXCAN_ESR_BUS_ERR"
> 
> Typo, sorry, I mean "(reg_esr & FLEXCAN_ESR_ERR_BUS)".
Sorry I made another typo too. :)

> 
> > If we only call flexcan_irq_state() upon state + error interrupt, then:
> > (new core: with    [TR]WRN_INT, e.g., i.MX6,
> >   old core: without [TR]WRN_INT, e.g., i.MX53)
> 
> The "FLEXCAN_QUIRK_BROKEN_PERR_STATE" is for the i.MX6. Only thinking
> about the core here.
> 
> > 1. error state increases, enough for both new and old cores.
> >      active ---> warning ---> passive ---> bus off
> >                     ^            ^            ^
> >                  WRN_INT/     ERR_INT      BOFF_INT
> > 	        ERR_INT(old core)
> >
> > 2. error state decreases, not enough for both new and old cores.
> >      active <--- warning <--- passive
> >         ^           ^
> >      [TR]X       WRN_INT/
> > 	        [TR]X(old core)
> 
> I was oping that the error warning interrupt occurs when the error
> counter falls again below error passive. Then we could enable the error
> interrupt on "active -> waning" and disable it on "passive -> warning"
> state changes. If that does work, than we can disable the error
> interrupt not before the "warning -> active" change.
Disable error interrupt on "passive -> warning" cannot handle the case:
1. Disconnect the cable, until error passive reported
2. Connect the cable, and wait error warning reported
3. Disconnect the cable while it is still error warning
Due to the error interrupt is disabled, so it cannot report error
passive again.

> 
> > The [TR]X interrupt is required for the state decreases, and the
> > overhead of them IMHO is not overkill:
> > 1. If flexcan_irq_state() is called when the state is not change,
> >     then, the overhead is read FLT_CONF and/or read error counter,
> >     do O(1) calculation and compare the state, then quit.
> 
> Yes, there is just a little overhead.
> 
> > 2. If flexcan_irq_state() is called when the state is changed,
> >     then, that's what help us to report correct state transitions.
> 
> The only difference between "new core" and "old core":
> 
> new core: error interrupt enabled between error warning and passive.
> old core: error interrupt enabled between error active and passive.
> 
> Not sure it it's worth the extra code.
Yes, it is worth. :) Please see the test result in 
https://marc.info/?l=linux-can&m=150424626206586&w=2, where we can
optimize for the new core further.

Best regards
Yi

Re: [PATCH 1/6] can: flexcan: fix state transition regression

[Wolfgang Grandegger]

Hello ZHU Yi,

Am 04.09.2017 um 12:01 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
> Hello Wolfgang,
> 
>> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>> Sent: Friday, September 01, 2017 11:15 PM
>>
>> [...]
>>>> usually a [PATCH 0/x] is coming first telling what the series does. We
>>>> use "git format-patch --cover-letter ..." for that purpose.
>>> OK. Thanks for your hint!
>>> I'll prepare the updated patch series with cover letter later.
>>
>> You patches are also white-space mangled.
> Perhaps because of the outlook. Although I've tested send to myself and
> copy the text then use "git am" to apply, it works in my local, but
> seems can not work when it send out. :(
> 
> Can we send the updated patches in separate e-mails, with attachments
> instead of inline text? Because we cannot easily switch to other
> mail client...
> 
> Or, do you have other proposal?
> 
>>
>> [...]
>>> I'm not sure that I understood your proposal correctly, the guess is
>>> the second "reg_esr & FLEXCAN_ESR_ERR_STATE" is a typo, do you mean
>>> "reg_esr & FLEXCAN_ESR_BUS_ERR"
>>
>> Typo, sorry, I mean "(reg_esr & FLEXCAN_ESR_ERR_BUS)".
> Sorry I made another typo too. :)
> 
>>
>>> If we only call flexcan_irq_state() upon state + error interrupt, then:
>>> (new core: with    [TR]WRN_INT, e.g., i.MX6,
>>>    old core: without [TR]WRN_INT, e.g., i.MX53)
>>
>> The "FLEXCAN_QUIRK_BROKEN_PERR_STATE" is for the i.MX6. Only thinking
>> about the core here.
>>
>>> 1. error state increases, enough for both new and old cores.
>>>       active ---> warning ---> passive ---> bus off
>>>                      ^            ^            ^
>>>                   WRN_INT/     ERR_INT      BOFF_INT
>>> 	        ERR_INT(old core)
>>>
>>> 2. error state decreases, not enough for both new and old cores.
>>>       active <--- warning <--- passive
>>>          ^           ^
>>>       [TR]X       WRN_INT/
>>> 	        [TR]X(old core)
>>
>> I was oping that the error warning interrupt occurs when the error
>> counter falls again below error passive. Then we could enable the error
>> interrupt on "active -> waning" and disable it on "passive -> warning"
>> state changes. If that does work, than we can disable the error
>> interrupt not before the "warning -> active" change.
> Disable error interrupt on "passive -> warning" cannot handle the case:
> 1. Disconnect the cable, until error passive reported
> 2. Connect the cable, and wait error warning reported
> 3. Disconnect the cable while it is still error warning
> Due to the error interrupt is disabled, so it cannot report error
> passive again.
> 
>>
>>> The [TR]X interrupt is required for the state decreases, and the
>>> overhead of them IMHO is not overkill:
>>> 1. If flexcan_irq_state() is called when the state is not change,
>>>      then, the overhead is read FLT_CONF and/or read error counter,
>>>      do O(1) calculation and compare the state, then quit.
>>
>> Yes, there is just a little overhead.
>>
>>> 2. If flexcan_irq_state() is called when the state is changed,
>>>      then, that's what help us to report correct state transitions.
>>
>> The only difference between "new core" and "old core":
>>
>> new core: error interrupt enabled between error warning and passive.
>> old core: error interrupt enabled between error active and passive.
>>
>> Not sure it it's worth the extra code.
> Yes, it is worth. :) Please see the test result in
> https://marc.info/?l=linux-can&m=150424626206586&w=2, where we can
> optimize for the new core further.

I have already prepared the following answer:

"I think we should throttle the interrupt rate as much as possible. 
Therefore I would go for the special treatment of old and new cores."

Will have  a closer look to your new approach when I'm back in the office.

Wolfgang.

[PATCH 1/6] can: flexcan: fix state transition regression

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

From bd1550533993968f20e972e72fa89e547264e09c Mon Sep 17 00:00:00 2001
From: Zhu Yi <yi.zhu5@cn.bosch.com>
Date: Wed, 30 Aug 2017 13:57:50 +0800
Subject: [PATCH 1/6] can: flexcan: fix state transition regression

Update state upon any interrupt to report correct state transitions
in case the flexcan core enabled the broken error state quirk fix.

Signed-off-by: Zhu Yi <yi.zhu5@cn.bosch.com>
Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
Acked-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/flexcan.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 13f0f21..df4bfb8 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -765,8 +765,9 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 		flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT, &regs->esr);
 	}
 
-	/* state change interrupt */
-	if (reg_esr & FLEXCAN_ESR_ERR_STATE)
+	/* 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_ERR_STATE))
 		flexcan_irq_state(dev, reg_esr);
 
 	/* bus error IRQ - handle if bus error reporting is activated */
-- 
2.7.4