[PATCH 1/1] can: sja1000: clear interrupts on start

[PATCH 1/1] can: sja1000: clear interrupts on start

[Mirza Krak]

From: Mirza Krak <mirza.krak@hostmobility.com>

According to SJA1000 data sheet error-warning (EI) interrupt is not
cleared by setting the controller in to reset-mode.

Then if we have the following case:
- system is suspended (echo mem > /sys/power/state) and SJA1000 is left
in operating state
- A bus error condition occurs which activates EI interrupt, system is
still suspended which means EI interrupt will be not be handled nor
cleared.

If the above two events occur, on resume there is no way to return the
SJA1000 to operating state, except to cycle power to it.

By simply reading the IR register on start we will clear any previous
conditions that could be present.

Signed-off-by: Mirza Krak <mirza.krak@hostmobility.com>
---
 drivers/net/can/sja1000/sja1000.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 7b92e911a616..f10834be48a5 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -218,6 +218,9 @@ static void sja1000_start(struct net_device *dev)
 	priv->write_reg(priv, SJA1000_RXERR, 0x0);
 	priv->read_reg(priv, SJA1000_ECC);
 
+	/* clear interrupt flags */
+	priv->read_reg(priv, SJA1000_IR);
+
 	/* leave reset mode */
 	set_normal_mode(dev);
 }
-- 
2.1.0

Re: [PATCH 1/1] can: sja1000: clear interrupts on start

[Marc Kleine-Budde]

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

On 11/10/2015 02:59 PM, Mirza Krak wrote:
> From: Mirza Krak <mirza.krak@hostmobility.com>
> 
> According to SJA1000 data sheet error-warning (EI) interrupt is not
> cleared by setting the controller in to reset-mode.
> 
> Then if we have the following case:
> - system is suspended (echo mem > /sys/power/state) and SJA1000 is left
> in operating state

This problem occurs only an hardware, where the SJA1000 is powered
during system suspend?

> - A bus error condition occurs which activates EI interrupt, system is
> still suspended which means EI interrupt will be not be handled nor
> cleared.
> 
> If the above two events occur, on resume there is no way to return the
> SJA1000 to operating state, except to cycle power to it.
> 
> By simply reading the IR register on start we will clear any previous
> conditions that could be present.

Doesn't the SJA1000 trigger an interrupt that is detected after
resuming? You add the fix to the open() function, which is triggered
during $(ifconfig up), how is related to suspend/resume? Does the
network layer call $(ifconfig down) during shutdown?

There isn't any suspend/resume code in the sja1000 driver, for me it
seems that we should add resume code that handles this problem.

> 
> Signed-off-by: Mirza Krak <mirza.krak@hostmobility.com>
> ---
>  drivers/net/can/sja1000/sja1000.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index 7b92e911a616..f10834be48a5 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -218,6 +218,9 @@ static void sja1000_start(struct net_device *dev)
>  	priv->write_reg(priv, SJA1000_RXERR, 0x0);
>  	priv->read_reg(priv, SJA1000_ECC);
>  
> +	/* clear interrupt flags */
> +	priv->read_reg(priv, SJA1000_IR);
> +
>  	/* leave reset mode */
>  	set_normal_mode(dev);
>  }
> 

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

Re: [PATCH 1/1] can: sja1000: clear interrupts on start

[Mirza Krak]

2015-11-11 9:04 GMT+01:00 Marc Kleine-Budde <mkl@pengutronix.de>:
> This problem occurs only an hardware, where the SJA1000 is powered
> during system suspend?

Correct, and the controller is in UP state.


> Doesn't the SJA1000 trigger an interrupt that is detected after
> resuming? You add the fix to the open() function, which is triggered
> during $(ifconfig up), how is related to suspend/resume? Does the
> network layer call $(ifconfig down) during shutdown?

It does not trigger another interrupt.

In our tests we have seen that IE and EPI flags are set in the IR
registered when we resume the system. If we do not clear these two
flags it does not produce any other interrupts. Setting the controller
in to reset mode does not clear the IE and EPI flags, I can understand
that IE flag is not cleared as this is stated in the data-sheet, can
not explain why EPI flag is not cleared though as data-sheet states 0
(reset) for both hardware reset and SETTING MOD.0 BY SOFTWARE (which
is the reset-mode)

Reason I put it in open() is so at least a DOWN/UP procedure returns
the controller to an operating state. Also if we do an UP we should
clear any earlier states that might exist in the registers like we do
with error counters and error code capture.

Network layer does not call $(ifconfig down) during shutdown.

> There isn't any suspend/resume code in the sja1000 driver, for me it
> seems that we should add resume code that handles this problem.

Yes, resume code should be implemented to handle this and other
problems (receive data). But still a DOWN/UP procedure should clear
any previous state that could exist in the controller registers.

-- 
Med Vänliga Hälsningar / Best Regards

*******************************************************************
Mirza Krak
Host Mobility AB
mirza.krak@hostmobility.com
Anders Personsgatan 12, 416 64 Göteborg
Sweden
http://www.hostmobility.com
Direct: +46 31 31 32 704
Phone: +46 31 31 32 700
Fax: +46 31 80 67 51
Mobile: +46 730 28 06 22
*******************************************************************

Re: [PATCH 1/1] can: sja1000: clear interrupts on start

[Marc Kleine-Budde]

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

On 11/11/2015 10:54 AM, Mirza Krak wrote:
> 2015-11-11 9:04 GMT+01:00 Marc Kleine-Budde <mkl@pengutronix.de>:
>> This problem occurs only an hardware, where the SJA1000 is powered
>> during system suspend?
> 
> Correct, and the controller is in UP state.
> 
>> Doesn't the SJA1000 trigger an interrupt that is detected after
>> resuming? You add the fix to the open() function, which is triggered
>> during $(ifconfig up), how is related to suspend/resume? Does the
>> network layer call $(ifconfig down) during shutdown?
> 
> It does not trigger another interrupt.

Can you put a scope on the interrupt pin and see if it's a problem of
the SoC and/or Linux or the SJA1000 not pulling the IRQ line.

> In our tests we have seen that IE and EPI flags are set in the IR
> registered when we resume the system. If we do not clear these two
> flags it does not produce any other interrupts. Setting the controller
> in to reset mode does not clear the IE and EPI flags, I can understand
> that IE flag is not cleared as this is stated in the data-sheet, can
> not explain why EPI flag is not cleared though as data-sheet states 0
> (reset) for both hardware reset and SETTING MOD.0 BY SOFTWARE (which
> is the reset-mode)
> 
> Reason I put it in open() is so at least a DOWN/UP procedure returns
> the controller to an operating state. Also if we do an UP we should
> clear any earlier states that might exist in the registers like we do
> with error counters and error code capture.

Yes - makes sense to have it in open().

> Network layer does not call $(ifconfig down) during shutdown.
> 
>> There isn't any suspend/resume code in the sja1000 driver, for me it
>> seems that we should add resume code that handles this problem.
> 
> Yes, resume code should be implemented to handle this and other
> problems (receive data). But still a DOWN/UP procedure should clear
> any previous state that could exist in the controller registers.

I'll add your patch to can/master and add stable on Cc. Looking forward
for some suspend/resume code :)

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

Re: [PATCH 1/1] can: sja1000: clear interrupts on start

[Mirza Krak]

2015-11-11 11:06 GMT+01:00 Marc Kleine-Budde <mkl@pengutronix.de>:
> On 11/11/2015 10:54 AM, Mirza Krak wrote:
>> 2015-11-11 9:04 GMT+01:00 Marc Kleine-Budde <mkl@pengutronix.de>:
>>> This problem occurs only an hardware, where the SJA1000 is powered
>>> during system suspend?
>>
>> Correct, and the controller is in UP state.
>>
>>> Doesn't the SJA1000 trigger an interrupt that is detected after
>>> resuming? You add the fix to the open() function, which is triggered
>>> during $(ifconfig up), how is related to suspend/resume? Does the
>>> network layer call $(ifconfig down) during shutdown?
>>
>> It does not trigger another interrupt.
>
> Can you put a scope on the interrupt pin and see if it's a problem of
> the SoC and/or Linux or the SJA1000 not pulling the IRQ line.

A little demonstration on what it looks like when the system is
resumed (added a printk in sja1000_start without my patch, though my
printk also clears IR register):

root@mx4-vcc:/opt/hm# ip -det link show can0
33: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state
UNKNOWN mode DEFAULT qlen 10
    link/can
    can state ERROR-ACTIVE (berr-counter tx 0 rx 128) restart-ms 0
    bitrate 500000 sample-point 0.875
    tq 250 prop-seg 3 phase-seg1 3 phase-seg2 1 sjw 1
    sja1000: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
    clock 12000000
root@mx4-vcc:/opt/hm# ifconfig can0 down
root@mx4-vcc:/opt/hm# ifconfig can0 up
[ 1612.621187] sja1000: MOD: 0x1
[ 1612.640992] sja1000: SR: 0x7c
[ 1612.660039] sja1000: IR: 0x24

Also did a measurement with the scope, it is the SJA1000 chip not
pulling the IRQ line.

I can actually send one frame when the controller is in this state but
never get a TI interrupt.

root@mx4-vcc-14381059:~/test/can_test# ip -det link show can0
17: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state
UNKNOWN mode DEFAULT qlen 10
    link/can
    can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
    bitrate 125000 sample-point 0.875
    tq 500 prop-seg 6 phase-seg1 7 phase-seg2 2 sjw 1
    sja1000: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
    clock 12000000
root@mx4-vcc-14381059:~/test/can_test# cansend can0 -i 123 0 1 2
interface = can0, family = 29, type = 3, proto = 1
root@mx4-vcc-14381059:~/test/can_test# cansend can0 -i 123 0 1 2
interface = can0, family = 29, type = 3, proto = 1
root@mx4-vcc-14381059:~/test/can_test# cansend can0 -i 123 0 1 2
interface = can0, family = 29, type = 3, proto = 1
root@mx4-vcc-14381059:~/test/can_test# cat /proc/interrupts | grep can0
205:          0          0      GPIO  can0
root@mx4-vcc-14381059:~/test/can_test#


## Second unit
root@mx4-t20-1000000:~/test/can_test# candump can0
interface = can0, family = 29, type = 3, proto = 1
<0x07b> [3] 00 01 02


>>> There isn't any suspend/resume code in the sja1000 driver, for me it
>>> seems that we should add resume code that handles this problem.
>>
>> Yes, resume code should be implemented to handle this and other
>> problems (receive data). But still a DOWN/UP procedure should clear
>> any previous state that could exist in the controller registers.
>
> I'll add your patch to can/master and add stable on Cc. Looking forward
> for some suspend/resume code :)

Looking forward to writing suspend/resume code :).

Some cred should go to Christian (added him to CC) for finding this
problem and reporting it to me.

-- 
Med Vänliga Hälsningar / Best Regards

*******************************************************************
Mirza Krak
Host Mobility AB
mirza.krak@hostmobility.com
Anders Personsgatan 12, 416 64 Göteborg
Sweden
http://www.hostmobility.com
Direct: +46 31 31 32 704
Phone: +46 31 31 32 700
Fax: +46 31 80 67 51
Mobile: +46 730 28 06 22
*******************************************************************

Re: [PATCH 1/1] can: sja1000: clear interrupts on start

[Marc Kleine-Budde]

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

On 11/11/2015 01:29 PM, Mirza Krak wrote:
>> Can you put a scope on the interrupt pin and see if it's a problem of
>> the SoC and/or Linux or the SJA1000 not pulling the IRQ line.
> 
> A little demonstration on what it looks like when the system is
> resumed (added a printk in sja1000_start without my patch, though my
> printk also clears IR register):
> 
> root@mx4-vcc:/opt/hm# ip -det link show can0
> 33: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state
> UNKNOWN mode DEFAULT qlen 10
>     link/can
>     can state ERROR-ACTIVE (berr-counter tx 0 rx 128) restart-ms 0
>     bitrate 500000 sample-point 0.875
>     tq 250 prop-seg 3 phase-seg1 3 phase-seg2 1 sjw 1
>     sja1000: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
>     clock 12000000
> root@mx4-vcc:/opt/hm# ifconfig can0 down
> root@mx4-vcc:/opt/hm# ifconfig can0 up
> [ 1612.621187] sja1000: MOD: 0x1
> [ 1612.640992] sja1000: SR: 0x7c
> [ 1612.660039] sja1000: IR: 0x24
> 
> Also did a measurement with the scope, it is the SJA1000 chip not
> pulling the IRQ line.
> 
> I can actually send one frame when the controller is in this state but
> never get a TI interrupt.

Thanks for testing.

[...]

>>> Yes, resume code should be implemented to handle this and other
>>> problems (receive data). But still a DOWN/UP procedure should clear
>>> any previous state that could exist in the controller registers.
>>
>> I'll add your patch to can/master and add stable on Cc. Looking forward
>> for some suspend/resume code :)
> 
> Looking forward to writing suspend/resume code :).
> 
> Some cred should go to Christian (added him to CC) for finding this
> problem and reporting it to me.

I'll add a

Reported-by: Christian Magnusson <Christian.Magnusson@semcon.com>

to the patch.

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

Re: [PATCH 1/1] can: sja1000: clear interrupts on start

[Mirza Krak]

2015-11-11 13:37 GMT+01:00 Marc Kleine-Budde <mkl@pengutronix.de>:
>> Some cred should go to Christian (added him to CC) for finding this
>> problem and reporting it to me.
>
> I'll add a
>
> Reported-by: Christian Magnusson <Christian.Magnusson@semcon.com>
>
> to the patch.
>

Can we just get an ACK that it is OK from Christian before we do it.



-- 
Med Vänliga Hälsningar / Best Regards

*******************************************************************
Mirza Krak
Host Mobility AB
mirza.krak@hostmobility.com
Anders Personsgatan 12, 416 64 Göteborg
Sweden
http://www.hostmobility.com
Direct: +46 31 31 32 704
Phone: +46 31 31 32 700
Fax: +46 31 80 67 51
Mobile: +46 730 28 06 22
*******************************************************************

Re: [PATCH 1/1] can: sja1000: clear interrupts on start

[Marc Kleine-Budde]

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

On 11/11/2015 01:46 PM, Mirza Krak wrote:
> 2015-11-11 13:37 GMT+01:00 Marc Kleine-Budde <mkl@pengutronix.de>:
>>> Some cred should go to Christian (added him to CC) for finding this
>>> problem and reporting it to me.
>>
>> I'll add a
>>
>> Reported-by: Christian Magnusson <Christian.Magnusson@semcon.com>
>>
>> to the patch.
> 
> Can we just get an ACK that it is OK from Christian before we do it.

Sure.

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

RE: [PATCH 1/1] can: sja1000: clear interrupts on start

[Christian Magnusson]

Thanks.. It's ok for me. Good that we finally found the problem with the hanging can-interfaces. :)

/Christian


Christian Magnusson
christian.magnusson@semcon.com


-----Original Message-----
From: Mirza Krak [mailto:mirza.krak@hostmobility.com] 
Sent: den 11 november 2015 13:46
To: Marc Kleine-Budde
Cc: wg@grandegger.com; andri.yngvason@marel.com; linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-kernel; Christian Magnusson
Subject: Re: [PATCH 1/1] can: sja1000: clear interrupts on start

2015-11-11 13:37 GMT+01:00 Marc Kleine-Budde <mkl@pengutronix.de>:
>> Some cred should go to Christian (added him to CC) for finding this 
>> problem and reporting it to me.
>
> I'll add a
>
> Reported-by: Christian Magnusson <Christian.Magnusson@semcon.com>
>
> to the patch.
>

Can we just get an ACK that it is OK from Christian before we do it.



--
Med Vänliga Hälsningar / Best Regards

*******************************************************************
Mirza Krak
Host Mobility AB
mirza.krak@hostmobility.com
Anders Personsgatan 12, 416 64 Göteborg
Sweden
http://www.hostmobility.com
Direct: +46 31 31 32 704
Phone: +46 31 31 32 700
Fax: +46 31 80 67 51
Mobile: +46 730 28 06 22
*******************************************************************