mcp251x hangs during ifup

mcp251x hangs during ifup

[Marc Kleine-Budde]

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

Hello,

in the armbian forum a problem with the mcp215x was reported:

https://forum.armbian.com/topic/14909-orange-pi-mcp2515-ip-up-freezes/

In the last comment (Posted Saturday at 10:05 PM) Aleksey posted a
mcp251x.c with a workaround. After cleanup the diff looks like this:

diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index f69fb4238a65..6d5fcf5a19a2 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -1221,7 +1221,7 @@ static int mcp251x_open(struct net_device *net)
        INIT_WORK(&priv->tx_work, mcp251x_tx_work_handler);
        INIT_WORK(&priv->restart_work, mcp251x_restart_work_handler);
-       ret = mcp251x_hw_wake(spi);
+       ret = mcp251x_hw_reset(spi);
        if (ret)
                goto out_free_wq;
        ret = mcp251x_setup(net, spi);

Vincent, it seems the gpio support has broken the driver on some
platforms. Do you have an idea?

Is RUNTIME_PM support now mandatory? So that mcp251x_can_suspend() and
mcp251x_can_resume() are now always called, does the driver break if
those functions are not compiled into the kernel?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: mcp251x hangs during ifup

[Vincent MAILHOL]

Hi Marc,

On Tue. 2 Mar 2021 at 01:58, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> Hello,
>
> in the armbian forum a problem with the mcp215x was reported:
>
> https://forum.armbian.com/topic/14909-orange-pi-mcp2515-ip-up-freezes/
>
> In the last comment (Posted Saturday at 10:05 PM) Aleksey posted a
> mcp251x.c with a workaround. After cleanup the diff looks like this:
>
> diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
> index f69fb4238a65..6d5fcf5a19a2 100644
> --- a/drivers/net/can/spi/mcp251x.c
> +++ b/drivers/net/can/spi/mcp251x.c
> @@ -1221,7 +1221,7 @@ static int mcp251x_open(struct net_device *net)
>         INIT_WORK(&priv->tx_work, mcp251x_tx_work_handler);
>         INIT_WORK(&priv->restart_work, mcp251x_restart_work_handler);
> -       ret = mcp251x_hw_wake(spi);
> +       ret = mcp251x_hw_reset(spi);
>         if (ret)
>                 goto out_free_wq;
>         ret = mcp251x_setup(net, spi);
>
> Vincent, it seems the gpio support has broken the driver on some
> platforms. Do you have an idea?

This mcp251x_hw_wake() function was introduced in below commit:
8ce8c0abcba3 ("can: mcp251x: only reset hardware as required")
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8ce8c0abcba314e1fe954a1840f6568bf5aef2ef

The GPIO support was then introduced later on in below commit:
2d52dabbef60 ("can: mcp251x: add GPIO support")
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d52dabbef60da91b72617519495fe77b621e6b8

I am adding Timo to the loop. He is the author of the two commits above.
I think he will be of better support than me here :)

> Is RUNTIME_PM support now mandatory? So that mcp251x_can_suspend() and
> mcp251x_can_resume() are now always called, does the driver break if
> those functions are not compiled into the kernel?

Re: mcp251x hangs during ifup

[Marc Kleine-Budde]

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

On 02.03.2021 10:40:21, Vincent MAILHOL wrote:
> On Tue. 2 Mar 2021 at 01:58, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >
> > Hello,
> >
> > in the armbian forum a problem with the mcp215x was reported:
> >
> > https://forum.armbian.com/topic/14909-orange-pi-mcp2515-ip-up-freezes/
> >
> > In the last comment (Posted Saturday at 10:05 PM) Aleksey posted a
> > mcp251x.c with a workaround. After cleanup the diff looks like this:
> >
> > diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
> > index f69fb4238a65..6d5fcf5a19a2 100644
> > --- a/drivers/net/can/spi/mcp251x.c
> > +++ b/drivers/net/can/spi/mcp251x.c
> > @@ -1221,7 +1221,7 @@ static int mcp251x_open(struct net_device *net)
> >         INIT_WORK(&priv->tx_work, mcp251x_tx_work_handler);
> >         INIT_WORK(&priv->restart_work, mcp251x_restart_work_handler);
> > -       ret = mcp251x_hw_wake(spi);
> > +       ret = mcp251x_hw_reset(spi);
> >         if (ret)
> >                 goto out_free_wq;
> >         ret = mcp251x_setup(net, spi);
> >
> > Vincent, it seems the gpio support has broken the driver on some
> > platforms. Do you have an idea?
> 
> This mcp251x_hw_wake() function was introduced in below commit:
> 8ce8c0abcba3 ("can: mcp251x: only reset hardware as required")
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8ce8c0abcba314e1fe954a1840f6568bf5aef2ef
> 
> The GPIO support was then introduced later on in below commit:
> 2d52dabbef60 ("can: mcp251x: add GPIO support")
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d52dabbef60da91b72617519495fe77b621e6b8
> 
> I am adding Timo to the loop. He is the author of the two commits above.
> I think he will be of better support than me here :)

Of course I wanted to add Timo, thanks. I think I've picked your e-mail,
as you've done the newest change on the driver after me :)

Sorry for the noise.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: mcp251x hangs during ifup

[Schlüßler, Timo]

On 02.03.2021 08:19:00, Marc wrote:
> On 02.03.2021 10:40:21, Vincent MAILHOL wrote:
> > This mcp251x_hw_wake() function was introduced in below commit:
> > 8ce8c0abcba3 ("can: mcp251x: only reset hardware as required") 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/com
> > mit/?id=8ce8c0abcba314e1fe954a1840f6568bf5aef2ef

> > I am adding Timo to the loop. He is the author of the two commits above.
> > I think he will be of better support than me here :)

> Of course I wanted to add Timo, thanks. I think I've picked your e-mail, as you've done the newest change on the driver after me :)

This CAN controller demands a software reset after power on which is done in mcp251x_can_probe -> mcp251x_hw_probe.
Since then resets are only performed when the power to the controller has been disabled. E.g. when mcp251x_can_suspend is called,
power is turned off and by setting AFTER_SUSPEND_POWER in priv->after_suspend it will be reenabled and reset in mcp251x_can_resume
and mcp251x_restart_work_handler.
Could it be that mcp251x_can_probe does not get called in any configuration? Or could it be that the controller gets power cycled during
operation with these Orange Pi boards?

Regards
Timo

Re: mcp251x hangs during ifup

[Marc Kleine-Budde]

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

On 02.03.2021 22:10:49, ukrfoil.com, Cтудиопак Украина, Восточный регион wrote:
> The reason for the terminal freezing on these boards (as an example of
> Nanopi NEO) occurs in this function when trying to disable interrupts
> disable_irq(spi->irq); If you comment out this given call, the hangs
> do not occur.

Nice debugging!

Can you give us the complete device tree and the output of:

    cat /proc/interrupts

Maybe there's a shared interrupt that's vital for your system.

One note:

| can0: can@0 {
|	reg = <0>;
|	compatible = "microchip,mcp2515";
|	pinctrl-names = "default";
|	pinctrl-0 = <&can0_pin_irq>;
|	interrupt-parent = <&pio>;
|	interrupts = <6 8 IRQ_TYPE_EDGE_FALLING>;

You better use IRQ_TYPE_LEVEL_LOW instead of IRQ_TYPE_EDGE_FALLING, but
that's unrelated to this problem.

|	clocks = <&can0_osc_fixed>;
|	spi-max-frequency = <10000000>;
|	gpio-controller;
|	gpio-cells = <2>;
|	status = "okay";
| };

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: mcp251x hangs during ifup

[Marc Kleine-Budde]

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

On 03.03.2021 09:44:54, ukrfoil.com, Cтудиопак Украина, Восточный регион wrote:
| root@nanopi-neo:~#cat /proc/interrupts
|            CPU0       CPU1       CPU2       CPU3      
|  25:          0          0          0          0     GICv2  50 Level     timer@1c20c00
|  26:          0          0          0          0     GICv2  29 Level     arch_timer
|  27:    1696369    3348041    3820461      82777     GICv2  30 Level     arch_timer
|  30:          0          0          0          0     GICv2  82 Level     1c02000.dma-controller
|  32:       5404          0          0          0     GICv2  92 Level     sunxi-mmc
|  34:          0          0          0          0     GICv2 103 Level     musb-hdrc.2.auto
|  35:          0          0          0          0     GICv2 104 Level     ehci_hcd:usb1
|  36:          0          0          0          0     GICv2 105 Level     ohci_hcd:usb2
|  37:          0          0          0          0     GICv2 110 Level     ehci_hcd:usb3
|  38:          0          0          0          0     GICv2 111 Level     ohci_hcd:usb4
|  42:       9952          0          0          0     GICv2 114 Level     eth0
|  43:          4          0          0          0     GICv2  97 Level     sun6i-spi
|  46:         49          0          0          0     GICv2  32 Level     ttyS0
|  47:  211302239          0          0          0     GICv2  38 Level     mv64xxx_i2c
|  48:   18992623          0          0          0     GICv2  39 Level     mv64xxx_i2c
|  49:          0          0          0          0     GICv2  72 Level     1f00000.rtc
|  63:          0          0          0          0     GICv2 152 Level     arm-pmu
|  64:          0          0          0          0     GICv2 153 Level     arm-pmu
|  65:          0          0          0          0     GICv2 154 Level     arm-pmu
|  66:          0          0          0          0     GICv2 155 Level     arm-pmu
| 111:          1          0          0          0  sunxi_pio_edge  44 Edge      usb0-id-det
| IPI0:          0          0          0          0  CPU wakeup interrupts
| IPI1:          0          0          0          0  Timer broadcast interrupts
| IPI2:         47         88         79         82  Rescheduling interrupts
| IPI3:     304963    2353917     362091       1661  Function call interrupts
| IPI4:          0          0          0          0  CPU stop interrupts
| IPI5:          0          0          0          0  IRQ work interrupts
| IPI6:          0          0          0          0  completion interrupts
| Err:          0
[...]
| root@nanopi-neo:~#cat /proc/interrupts
|            CPU0       CPU1       CPU2       CPU3      
|  25:          0          0          0          0     GICv2  50 Level     timer@1c20c00
|  26:          0          0          0          0     GICv2  29 Level     arch_timer
|  27:    1711040    3437940    3850320      84900     GICv2  30 Level     arch_timer
|  30:          0          0          0          0     GICv2  82 Level     1c02000.dma-controller
|  32:       5516          0          0          0     GICv2  92 Level     sunxi-mmc
|  34:          0          0          0          0     GICv2 103 Level     musb-hdrc.2.auto
|  35:          0          0          0          0     GICv2 104 Level     ehci_hcd:usb1
|  36:          0          0          0          0     GICv2 105 Level     ohci_hcd:usb2
|  37:          0          0          0          0     GICv2 110 Level     ehci_hcd:usb3
|  38:          0          0          0          0     GICv2 111 Level     ohci_hcd:usb4
|  42:      10190          0          0          0     GICv2 114 Level     eth0
|  43:         18          0          0          0     GICv2  97 Level     sun6i-spi
|  46:         49          0          0          0     GICv2  32 Level     ttyS0
|  47:  214592622          0          0          0     GICv2  38 Level     mv64xxx_i2c
|  48:   19314189          0          0          0     GICv2  39 Level     mv64xxx_i2c
|  49:          0          0          0          0     GICv2  72 Level     1f00000.rtc
|  63:          0          0          0          0     GICv2 152 Level     arm-pmu
|  64:          0          0          0          0     GICv2 153 Level     arm-pmu
|  65:          0          0          0          0     GICv2 154 Level     arm-pmu
|  66:          0          0          0          0     GICv2 155 Level     arm-pmu
| 107:          1          0          0          0  sunxi_pio_edge  40 Edge      spi0.0
| 111:          1          0          0          0  sunxi_pio_edge  44 Edge      usb0-id-det
| IPI0:          0          0          0          0  CPU wakeup interrupts
| IPI1:          0          0          0          0  Timer broadcast interrupts
| IPI2:         48         88         85         83  Rescheduling interrupts
| IPI3:     305542    2828247     362982       1704  Function call interrupts
| IPI4:          0          0          0          0  CPU stop interrupts
| IPI5:          0          0          0          0  IRQ work interrupts
| IPI6:          0          0          0          0  completion interrupts
| Err:          0

That looks good, the IRQ is not shared. Can you test again with
IRQ_TYPE_EDGE_FALLING in the DT instead of IRQ_TYPE_LEVEL_LOW?

| can0: can@0 {
|       reg = <0>;
|       compatible = "microchip,mcp2515";
|       pinctrl-names = "default";
|       pinctrl-0 = <&can0_pin_irq>;
|       interrupt-parent = <&pio>;
|       interrupts = <6 8 IRQ_TYPE_EDGE_FALLING>;

You better use IRQ_TYPE_LEVEL_LOW instead of IRQ_TYPE_EDGE_FALLING, but
that's unrelated to this problem.

|       clocks = <&can0_osc_fixed>;
|       spi-max-frequency = <10000000>;
|       gpio-controller;
|       gpio-cells = <2>;
|       status = "okay";
| };

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: mcp251x hangs during ifup

[Marc Kleine-Budde]

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

On 03.03.2021 10:00:47, ukrfoil.com, Cтудиопак Украина, Восточный регион wrote:
> this with the IRQ_TYPE_EDGE_FALLING value

> > That looks good, the IRQ is not shared. Can you test again with
> > IRQ_TYPE_EDGE_FALLING in the DT instead of IRQ_TYPE_LEVEL_LOW?

Doh! Need more coffee.

Of course I mean try with IRQ_TYPE_LEVEL_LOW and check if the unmodified
mcp2515 driver with disable_irq() does work then.

> >
> > | can0: can@0 {
> > |       reg = <0>;
> > |       compatible = "microchip,mcp2515";
> > |       pinctrl-names = "default";
> > |       pinctrl-0 = <&can0_pin_irq>;
> > |       interrupt-parent = <&pio>;
> > |       interrupts = <6 8 IRQ_TYPE_EDGE_FALLING>;
> >
> > You better use IRQ_TYPE_LEVEL_LOW instead of IRQ_TYPE_EDGE_FALLING, but
> > that's unrelated to this problem.
> >
> > |       clocks = <&can0_osc_fixed>;
> > |       spi-max-frequency = <10000000>;
> > |       gpio-controller;
> > |       gpio-cells = <2>;
> > |       status = "okay";
> > | };

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: mcp251x hangs during ifup

[Marc Kleine-Budde]

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

On 03.03.2021 10:24:56, ukrfoil.com, Cтудиопак Украина, Восточный регион wrote:
> I've tried this and that before - the effect is the same, I'll
> double-check it again later.

Thanks.

Timo, can you change the routine to keep the interrupts enabled?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: mcp251x hangs during ifup

[Schlüßler, Timo]

On 03.03.2021 09:34, Marc Kleine-Budde wrote:
> Timo, can you change the routine to keep the interrupts enabled?

I think it should be possible by ignoring the interrupt in the handler itself. As far as I remember disabling the
interrupts was implemented in the driver even before I added the GPIO support, so I don't know exactly why
it was made like this.
Maybe later this week I can have a closer look into this.

Regards
Timo

Re: mcp251x hangs during ifup

[Marc Kleine-Budde]

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

On 03.03.2021 20:46:25, ukrfoil.com, Cтудиопак Украина, Восточный регион wrote:
> You have set IRQ_TYPE_LEVEL_LOW correctly
> 
> After installation and with the default driver, everything works,
> there is no hang.

\o/

> It looks like I was wrong in my experiments, I was led astray, there
> is a lot of information on the Internet about setting
> IRQ_TYPE_EDGE_FALLING as correct. Therefore, I apologize for the time
> spent solving this problem. Thanks for the help.

No problem!

> data after changing to IRQ_TYPE_LEVEL_LOW
> 
> root@nanopi-neo:~#cat /proc/interrupts
> >            CPU0       CPU1       CPU2       CPU3
> > 107:          0          0          0          0  sunxi_pio_level  40 Level     spi0.0

This looks consistent.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]