The current code causes a slight glitch on the pinctrl settings when used. Since commit ab78029 (drivers/pinctrl: grab default handles from device core), the device core will automatically set the default pins. This causes the pins to be momentarily set to the default and then to the sleep state in register_m_can_dev(). By adding an optional "enable" state, boards can set the default pin state to be disabled and avoid the glitch when the switch from default to sleep first occurs. If the "enable" state is not available pinctrl_get_select() falls back to using the "default" pinctrl state.
Fixes: c9b3bce18da4 ("can: m_can: select pinctrl state in each suspend/resume function") Signed-off-by: Marek Vasut marex@denx.de Cc: Bich Hemon bich.hemon@st.com Cc: Grygorii Strashko grygorii.strashko@ti.com Cc: J.D. Schroeder jay.schroeder@garmin.com Cc: Marc Kleine-Budde mkl@pengutronix.de Cc: Roger Quadros rogerq@ti.com Cc: linux-stable stable@vger.kernel.org To: linux-can@vger.kernel.org --- NOTE: This is commit 033365191136 ("can: c_can: Fix default pinmux glitch at init") adapted for m_can driver. --- drivers/net/can/m_can/m_can.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index 02c5795b73936..afb6760b17427 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1243,12 +1243,20 @@ static void m_can_chip_config(struct net_device *dev) static void m_can_start(struct net_device *dev) { struct m_can_classdev *cdev = netdev_priv(dev); + struct pinctrl *p;
/* basic m_can configuration */ m_can_chip_config(dev);
cdev->can.state = CAN_STATE_ERROR_ACTIVE;
+ /* Attempt to use "active" if available else use "default" */ + p = pinctrl_get_select(cdev->dev, "active"); + if (!IS_ERR(p)) + pinctrl_put(p); + else + pinctrl_pm_select_default_state(cdev->dev); + m_can_enable_all_interrupts(cdev); }
On 17/12/2019 12:07, Marek Vasut wrote:
The current code causes a slight glitch on the pinctrl settings when used. Since commit ab78029 (drivers/pinctrl: grab default handles from device core), the device core will automatically set the default pins. This causes the pins to be momentarily set to the default and then to the sleep state in register_m_can_dev(). By adding an optional "enable" state, boards can set the default pin state to be disabled and avoid the glitch when the switch from default to sleep first occurs. If the "enable" state is not available pinctrl_get_select() falls back to using the "default" pinctrl state.
Fixes: c9b3bce18da4 ("can: m_can: select pinctrl state in each suspend/resume function") Signed-off-by: Marek Vasut marex@denx.de Cc: Bich Hemon bich.hemon@st.com Cc: Grygorii Strashko grygorii.strashko@ti.com Cc: J.D. Schroeder jay.schroeder@garmin.com Cc: Marc Kleine-Budde mkl@pengutronix.de Cc: Roger Quadros rogerq@ti.com Cc: linux-stable stable@vger.kernel.org To: linux-can@vger.kernel.org
NOTE: This is commit 033365191136 ("can: c_can: Fix default pinmux glitch at init") adapted for m_can driver.
drivers/net/can/m_can/m_can.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index 02c5795b73936..afb6760b17427 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1243,12 +1243,20 @@ static void m_can_chip_config(struct net_device *dev) static void m_can_start(struct net_device *dev) { struct m_can_classdev *cdev = netdev_priv(dev);
- struct pinctrl *p;
/* basic m_can configuration */ m_can_chip_config(dev); cdev->can.state = CAN_STATE_ERROR_ACTIVE;
- /* Attempt to use "active" if available else use "default" */
- p = pinctrl_get_select(cdev->dev, "active");
- if (!IS_ERR(p))
pinctrl_put(p);- else
pinctrl_pm_select_default_state(cdev->dev);- m_can_enable_all_interrupts(cdev); }
May be init state should be used - #define PINCTRL_STATE_INIT "init" instead?
On 12/17/19 11:42 AM, Grygorii Strashko wrote:
On 17/12/2019 12:07, Marek Vasut wrote:
The current code causes a slight glitch on the pinctrl settings when used. Since commit ab78029 (drivers/pinctrl: grab default handles from device core), the device core will automatically set the default pins. This causes the pins to be momentarily set to the default and then to the sleep state in register_m_can_dev(). By adding an optional "enable" state, boards can set the default pin state to be disabled and avoid the glitch when the switch from default to sleep first occurs. If the "enable" state is not available pinctrl_get_select() falls back to using the "default" pinctrl state.
Fixes: c9b3bce18da4 ("can: m_can: select pinctrl state in each suspend/resume function") Signed-off-by: Marek Vasut marex@denx.de Cc: Bich Hemon bich.hemon@st.com Cc: Grygorii Strashko grygorii.strashko@ti.com Cc: J.D. Schroeder jay.schroeder@garmin.com Cc: Marc Kleine-Budde mkl@pengutronix.de Cc: Roger Quadros rogerq@ti.com Cc: linux-stable stable@vger.kernel.org To: linux-can@vger.kernel.org
NOTE: This is commit 033365191136 ("can: c_can: Fix default pinmux glitch at init") adapted for m_can driver.
drivers/net/can/m_can/m_can.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index 02c5795b73936..afb6760b17427 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1243,12 +1243,20 @@ static void m_can_chip_config(struct net_device *dev) static void m_can_start(struct net_device *dev) { struct m_can_classdev *cdev = netdev_priv(dev); + struct pinctrl *p; /* basic m_can configuration */ m_can_chip_config(dev); cdev->can.state = CAN_STATE_ERROR_ACTIVE; + /* Attempt to use "active" if available else use "default" */ + p = pinctrl_get_select(cdev->dev, "active"); + if (!IS_ERR(p)) + pinctrl_put(p); + else + pinctrl_pm_select_default_state(cdev->dev);
m_can_enable_all_interrupts(cdev); }
May be init state should be used - #define PINCTRL_STATE_INIT "init" instead?
I'm not sure I quite understand -- how ?
On 17/12/2019 12:55, Marek Vasut wrote:
On 12/17/19 11:42 AM, Grygorii Strashko wrote:
On 17/12/2019 12:07, Marek Vasut wrote:
The current code causes a slight glitch on the pinctrl settings when used. Since commit ab78029 (drivers/pinctrl: grab default handles from device core), the device core will automatically set the default pins. This causes the pins to be momentarily set to the default and then to the sleep state in register_m_can_dev(). By adding an optional "enable" state, boards can set the default pin state to be disabled and avoid the glitch when the switch from default to sleep first occurs. If the "enable" state is not available pinctrl_get_select() falls back to using the "default" pinctrl state.
Fixes: c9b3bce18da4 ("can: m_can: select pinctrl state in each suspend/resume function") Signed-off-by: Marek Vasut marex@denx.de Cc: Bich Hemon bich.hemon@st.com Cc: Grygorii Strashko grygorii.strashko@ti.com Cc: J.D. Schroeder jay.schroeder@garmin.com Cc: Marc Kleine-Budde mkl@pengutronix.de Cc: Roger Quadros rogerq@ti.com Cc: linux-stable stable@vger.kernel.org To: linux-can@vger.kernel.org
NOTE: This is commit 033365191136 ("can: c_can: Fix default pinmux glitch at init") adapted for m_can driver.
drivers/net/can/m_can/m_can.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index 02c5795b73936..afb6760b17427 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1243,12 +1243,20 @@ static void m_can_chip_config(struct net_device *dev) static void m_can_start(struct net_device *dev) { struct m_can_classdev *cdev = netdev_priv(dev); + struct pinctrl *p; /* basic m_can configuration */ m_can_chip_config(dev); cdev->can.state = CAN_STATE_ERROR_ACTIVE; + /* Attempt to use "active" if available else use "default" */ + p = pinctrl_get_select(cdev->dev, "active"); + if (!IS_ERR(p)) + pinctrl_put(p); + else + pinctrl_pm_select_default_state(cdev->dev);
m_can_enable_all_interrupts(cdev); }
May be init state should be used - #define PINCTRL_STATE_INIT "init" instead?
I'm not sure I quite understand -- how ?
Sry, for delayed reply.
I've looked at m_can code and think issue is a little bit deeper (but I might be wrong as i'm not can expert and below based on code review).
First, what going on: probe: really_probe() pinctrl_bind_pins() if (IS_ERR(dev->pins->init_state)) { ret = pinctrl_select_state(dev->pins->p, dev->pins->default_state); } else { ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state); } [GS] So at this point default_state or init_state is set
ret = dev->bus->probe(dev); m_can_plat_probe() m_can_class_register() m_can_clk_start() pm_runtime_get_sync() m_can_runtime_resume() [GS] Still default_state or init_state is active
register_m_can_dev() [GS] at this point m_can netdev is registered, which may lead to .ndo_open = m_can_open() call
m_can_clk_stop() pm_runtime_put_sync() [GS] if .ndo_open() was called before it will be a nop m_can_runtime_suspend() m_can_class_suspend()
if (netif_running(ndev)) { netif_stop_queue(ndev); netif_device_detach(ndev); m_can_stop(ndev); m_can_clk_stop(cdev); [GS] if .ndo_open() was called before it will lead to deadlock here So, most probably, it will cause deadlock in case of "ifconfig <m_can_dev> up down" case }
pinctrl_pm_select_sleep_state(dev); [GS] at this point sleep_state will be set - i assume it's the root cause of your glitch. Note - As per code, the pinctrl default_state will never ever configured again, so if after probe m_can will go through PM runtime suspend/resume cycle it will not work any more.
pinctrl_init_done() [GS] will do nothing in case !init_state
As per above, if sleep_state is defined the m_can seems should not work at all without your patch, as there is no code path to switch back sleep_state->default_state. And over all PM runtime m_can code is mixed with System suspend code and so not correct.
Also, the very good question - Is it really required to toggle pinctrl states as part of PM runtime? (usually it's enough to handle it only during System suspend).
On 12/19/19 2:37 PM, Grygorii Strashko wrote:
On 17/12/2019 12:55, Marek Vasut wrote:
On 12/17/19 11:42 AM, Grygorii Strashko wrote:
On 17/12/2019 12:07, Marek Vasut wrote:
The current code causes a slight glitch on the pinctrl settings when used. Since commit ab78029 (drivers/pinctrl: grab default handles from device core), the device core will automatically set the default pins. This causes the pins to be momentarily set to the default and then to the sleep state in register_m_can_dev(). By adding an optional "enable" state, boards can set the default pin state to be disabled and avoid the glitch when the switch from default to sleep first occurs. If the "enable" state is not available pinctrl_get_select() falls back to using the "default" pinctrl state.
Fixes: c9b3bce18da4 ("can: m_can: select pinctrl state in each suspend/resume function") Signed-off-by: Marek Vasut marex@denx.de Cc: Bich Hemon bich.hemon@st.com Cc: Grygorii Strashko grygorii.strashko@ti.com Cc: J.D. Schroeder jay.schroeder@garmin.com Cc: Marc Kleine-Budde mkl@pengutronix.de Cc: Roger Quadros rogerq@ti.com Cc: linux-stable stable@vger.kernel.org To: linux-can@vger.kernel.org
NOTE: This is commit 033365191136 ("can: c_can: Fix default pinmux glitch at init") adapted for m_can driver.
drivers/net/can/m_can/m_can.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index 02c5795b73936..afb6760b17427 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1243,12 +1243,20 @@ static void m_can_chip_config(struct net_device *dev) static void m_can_start(struct net_device *dev) { struct m_can_classdev *cdev = netdev_priv(dev); + struct pinctrl *p; /* basic m_can configuration */ m_can_chip_config(dev); cdev->can.state = CAN_STATE_ERROR_ACTIVE; + /* Attempt to use "active" if available else use "default" */ + p = pinctrl_get_select(cdev->dev, "active"); + if (!IS_ERR(p)) + pinctrl_put(p); + else + pinctrl_pm_select_default_state(cdev->dev);
m_can_enable_all_interrupts(cdev); }
May be init state should be used - #define PINCTRL_STATE_INIT "init" instead?
I'm not sure I quite understand -- how ?
Sry, for delayed reply.
I've looked at m_can code and think issue is a little bit deeper (but I might be wrong as i'm not can expert and below based on code review).
First, what going on: probe: really_probe() pinctrl_bind_pins() if (IS_ERR(dev->pins->init_state)) { ret = pinctrl_select_state(dev->pins->p, dev->pins->default_state); } else { ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state); } [GS] So at this point default_state or init_state is set
ret = dev->bus->probe(dev); m_can_plat_probe() m_can_class_register() m_can_clk_start() pm_runtime_get_sync() m_can_runtime_resume() [GS] Still default_state or init_state is active
register_m_can_dev() [GS] at this point m_can netdev is registered, which may lead to .ndo_open = m_can_open() call
m_can_clk_stop() pm_runtime_put_sync() [GS] if .ndo_open() was called before it will be a nop m_can_runtime_suspend() m_can_class_suspend()
if (netif_running(ndev)) { netif_stop_queue(ndev); netif_device_detach(ndev); m_can_stop(ndev); m_can_clk_stop(cdev); [GS] if .ndo_open() was called before it will lead to deadlock here So, most probably, it will cause deadlock in case of "ifconfig <m_can_dev> up down" case }
pinctrl_pm_select_sleep_state(dev); [GS] at this point sleep_state will be set - i assume it's the root cause of your glitch. Note - As per code, the pinctrl default_state will never ever configured again, so if after probe m_can will go through PM runtime suspend/resume cycle it will not work any more.
pinctrl_init_done() [GS] will do nothing in case !init_state
As per above, if sleep_state is defined the m_can seems should not work at all without your patch, as there is no code path to switch back sleep_state->default_state. And over all PM runtime m_can code is mixed with System suspend code and so not correct.
Also, the very good question - Is it really required to toggle pinctrl states as part of PM runtime? (usually it's enough to handle it only during System suspend).
I suspect this discussion is somewhat a separate topic from what this patch is trying to fix ?
On 19/12/2019 23:47, Marek Vasut wrote:
On 12/19/19 2:37 PM, Grygorii Strashko wrote:
On 17/12/2019 12:55, Marek Vasut wrote:
On 12/17/19 11:42 AM, Grygorii Strashko wrote:
On 17/12/2019 12:07, Marek Vasut wrote:
The current code causes a slight glitch on the pinctrl settings when used. Since commit ab78029 (drivers/pinctrl: grab default handles from device core), the device core will automatically set the default pins. This causes the pins to be momentarily set to the default and then to the sleep state in register_m_can_dev(). By adding an optional "enable" state, boards can set the default pin state to be disabled and avoid the glitch when the switch from default to sleep first occurs. If the "enable" state is not available pinctrl_get_select() falls back to using the "default" pinctrl state.
Fixes: c9b3bce18da4 ("can: m_can: select pinctrl state in each suspend/resume function") Signed-off-by: Marek Vasut marex@denx.de Cc: Bich Hemon bich.hemon@st.com Cc: Grygorii Strashko grygorii.strashko@ti.com Cc: J.D. Schroeder jay.schroeder@garmin.com Cc: Marc Kleine-Budde mkl@pengutronix.de Cc: Roger Quadros rogerq@ti.com Cc: linux-stable stable@vger.kernel.org To: linux-can@vger.kernel.org
NOTE: This is commit 033365191136 ("can: c_can: Fix default pinmux glitch at init") adapted for m_can driver.
drivers/net/can/m_can/m_can.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index 02c5795b73936..afb6760b17427 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1243,12 +1243,20 @@ static void m_can_chip_config(struct net_device *dev) static void m_can_start(struct net_device *dev) { struct m_can_classdev *cdev = netdev_priv(dev); + struct pinctrl *p; /* basic m_can configuration */ m_can_chip_config(dev); cdev->can.state = CAN_STATE_ERROR_ACTIVE; + /* Attempt to use "active" if available else use "default" */ + p = pinctrl_get_select(cdev->dev, "active"); + if (!IS_ERR(p)) + pinctrl_put(p); + else + pinctrl_pm_select_default_state(cdev->dev);
m_can_enable_all_interrupts(cdev); }
May be init state should be used - #define PINCTRL_STATE_INIT "init" instead?
I'm not sure I quite understand -- how ?
Sry, for delayed reply.
I've looked at m_can code and think issue is a little bit deeper (but I might be wrong as i'm not can expert and below based on code review).
First, what going on: probe: really_probe() pinctrl_bind_pins() if (IS_ERR(dev->pins->init_state)) { ret = pinctrl_select_state(dev->pins->p, dev->pins->default_state); } else { ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state); } [GS] So at this point default_state or init_state is set
ret = dev->bus->probe(dev); m_can_plat_probe() m_can_class_register() m_can_clk_start() pm_runtime_get_sync() m_can_runtime_resume() [GS] Still default_state or init_state is active
register_m_can_dev() [GS] at this point m_can netdev is registered, which may lead to .ndo_open = m_can_open() call
m_can_clk_stop() pm_runtime_put_sync() [GS] if .ndo_open() was called before it will be a nop m_can_runtime_suspend() m_can_class_suspend()
if (netif_running(ndev)) { netif_stop_queue(ndev); netif_device_detach(ndev); m_can_stop(ndev); m_can_clk_stop(cdev); [GS] if .ndo_open() was called before it will lead to deadlock here So, most probably, it will cause deadlock in case of "ifconfig <m_can_dev> up down" case }
pinctrl_pm_select_sleep_state(dev); [GS] at this point sleep_state will be set - i assume it's the root cause of your glitch. Note - As per code, the pinctrl default_state will never ever configured again, so if after probe m_can will go through PM runtime suspend/resume cycle it will not work any more.
pinctrl_init_done() [GS] will do nothing in case !init_state
As per above, if sleep_state is defined the m_can seems should not work at all without your patch, as there is no code path to switch back sleep_state->default_state. And over all PM runtime m_can code is mixed with System suspend code and so not correct.
Also, the very good question - Is it really required to toggle pinctrl states as part of PM runtime? (usually it's enough to handle it only during System suspend).
I suspect this discussion is somewhat a separate topic from what this patch is trying to fix ?
Not exactly. The reason you need this patch is misuse of PM runtime vs pin control in this driver. And this has to be fixed first of all.
I feel that just removing of m_can_class_suspend() call from m_can_runtime_suspend() will fix things for you - it will toggle pin states only during Suspend to RAM cycle.
On 12/21/19 11:53 AM, Grygorii Strashko wrote:
On 19/12/2019 23:47, Marek Vasut wrote:
On 12/19/19 2:37 PM, Grygorii Strashko wrote:
On 17/12/2019 12:55, Marek Vasut wrote:
On 12/17/19 11:42 AM, Grygorii Strashko wrote:
On 17/12/2019 12:07, Marek Vasut wrote:
The current code causes a slight glitch on the pinctrl settings when used. Since commit ab78029 (drivers/pinctrl: grab default handles from device core), the device core will automatically set the default pins. This causes the pins to be momentarily set to the default and then to the sleep state in register_m_can_dev(). By adding an optional "enable" state, boards can set the default pin state to be disabled and avoid the glitch when the switch from default to sleep first occurs. If the "enable" state is not available pinctrl_get_select() falls back to using the "default" pinctrl state.
Fixes: c9b3bce18da4 ("can: m_can: select pinctrl state in each suspend/resume function") Signed-off-by: Marek Vasut marex@denx.de Cc: Bich Hemon bich.hemon@st.com Cc: Grygorii Strashko grygorii.strashko@ti.com Cc: J.D. Schroeder jay.schroeder@garmin.com Cc: Marc Kleine-Budde mkl@pengutronix.de Cc: Roger Quadros rogerq@ti.com Cc: linux-stable stable@vger.kernel.org To: linux-can@vger.kernel.org
NOTE: This is commit 033365191136 ("can: c_can: Fix default pinmux glitch at init") adapted for m_can driver.
drivers/net/can/m_can/m_can.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index 02c5795b73936..afb6760b17427 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1243,12 +1243,20 @@ static void m_can_chip_config(struct net_device *dev) static void m_can_start(struct net_device *dev) { struct m_can_classdev *cdev = netdev_priv(dev); + struct pinctrl *p; /* basic m_can configuration */ m_can_chip_config(dev); cdev->can.state = CAN_STATE_ERROR_ACTIVE; + /* Attempt to use "active" if available else use "default" */ + p = pinctrl_get_select(cdev->dev, "active"); + if (!IS_ERR(p)) + pinctrl_put(p); + else + pinctrl_pm_select_default_state(cdev->dev);
m_can_enable_all_interrupts(cdev); }
May be init state should be used - #define PINCTRL_STATE_INIT "init" instead?
I'm not sure I quite understand -- how ?
Sry, for delayed reply.
I've looked at m_can code and think issue is a little bit deeper (but I might be wrong as i'm not can expert and below based on code review).
First, what going on: probe: really_probe() pinctrl_bind_pins() if (IS_ERR(dev->pins->init_state)) { ret = pinctrl_select_state(dev->pins->p, dev->pins->default_state); } else { ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state); } [GS] So at this point default_state or init_state is set
ret = dev->bus->probe(dev); m_can_plat_probe() m_can_class_register() m_can_clk_start() pm_runtime_get_sync() m_can_runtime_resume() [GS] Still default_state or init_state is active
register_m_can_dev() [GS] at this point m_can netdev is registered, which may lead to .ndo_open = m_can_open() call
m_can_clk_stop() pm_runtime_put_sync() [GS] if .ndo_open() was called before it will be a nop m_can_runtime_suspend() m_can_class_suspend()
if (netif_running(ndev)) { netif_stop_queue(ndev); netif_device_detach(ndev); m_can_stop(ndev); m_can_clk_stop(cdev); [GS] if .ndo_open() was called before it will lead to deadlock here So, most probably, it will cause deadlock in case of "ifconfig <m_can_dev> up down" case }
pinctrl_pm_select_sleep_state(dev); [GS] at this point sleep_state will be set - i assume it's the root cause of your glitch. Note - As per code, the pinctrl default_state will never ever configured again, so if after probe m_can will go through PM runtime suspend/resume cycle it will not work any more.
pinctrl_init_done() [GS] will do nothing in case !init_state
As per above, if sleep_state is defined the m_can seems should not work at all without your patch, as there is no code path to switch back sleep_state->default_state. And over all PM runtime m_can code is mixed with System suspend code and so not correct.
Also, the very good question - Is it really required to toggle pinctrl states as part of PM runtime? (usually it's enough to handle it only during System suspend).
I suspect this discussion is somewhat a separate topic from what this patch is trying to fix ?
Not exactly.
I see ?
The reason you need this patch is misuse of PM runtime vs pin control in this driver. And this has to be fixed first of all.
But then the C_CAN also misuses the PM runtime ? I mean, this patch does literally what the same patch for C_CAN does, so maybe this is a more general problem and needs a separate fix -- unless tristating the pins when the block if disabled is the right thing to do, which it might be.
I feel that just removing of m_can_class_suspend() call from m_can_runtime_suspend() will fix things for you - it will toggle pin states only during Suspend to RAM cycle.
I need to configure the pins on boot, this has nothing to do with suspend/resume.
On 21/12/2019 13:00, Marek Vasut wrote:
On 12/21/19 11:53 AM, Grygorii Strashko wrote:
On 19/12/2019 23:47, Marek Vasut wrote:
On 12/19/19 2:37 PM, Grygorii Strashko wrote:
On 17/12/2019 12:55, Marek Vasut wrote:
On 12/17/19 11:42 AM, Grygorii Strashko wrote:
On 17/12/2019 12:07, Marek Vasut wrote: > The current code causes a slight glitch on the pinctrl settings when > used. > Since commit ab78029 (drivers/pinctrl: grab default handles from > device core), > the device core will automatically set the default pins. This causes > the pins > to be momentarily set to the default and then to the sleep state in > register_m_can_dev(). By adding an optional "enable" state, boards > can > set the > default pin state to be disabled and avoid the glitch when the switch > from > default to sleep first occurs. If the "enable" state is not available > pinctrl_get_select() falls back to using the "default" pinctrl state. > > Fixes: c9b3bce18da4 ("can: m_can: select pinctrl state in each > suspend/resume function") > Signed-off-by: Marek Vasut marex@denx.de > Cc: Bich Hemon bich.hemon@st.com > Cc: Grygorii Strashko grygorii.strashko@ti.com > Cc: J.D. Schroeder jay.schroeder@garmin.com > Cc: Marc Kleine-Budde mkl@pengutronix.de > Cc: Roger Quadros rogerq@ti.com > Cc: linux-stable stable@vger.kernel.org > To: linux-can@vger.kernel.org > --- > NOTE: This is commit 033365191136 ("can: c_can: Fix default pinmux > glitch at init") > adapted for m_can driver. > --- > drivers/net/can/m_can/m_can.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/net/can/m_can/m_can.c > b/drivers/net/can/m_can/m_can.c > index 02c5795b73936..afb6760b17427 100644 > --- a/drivers/net/can/m_can/m_can.c > +++ b/drivers/net/can/m_can/m_can.c > @@ -1243,12 +1243,20 @@ static void m_can_chip_config(struct > net_device *dev) > static void m_can_start(struct net_device *dev) > { > struct m_can_classdev *cdev = netdev_priv(dev); > + struct pinctrl *p; > /* basic m_can configuration */ > m_can_chip_config(dev); > cdev->can.state = CAN_STATE_ERROR_ACTIVE; > + /* Attempt to use "active" if available else use > "default" */ > + p = pinctrl_get_select(cdev->dev, "active"); > + if (!IS_ERR(p)) > + pinctrl_put(p); > + else > + pinctrl_pm_select_default_state(cdev->dev); > + > m_can_enable_all_interrupts(cdev); > } >
May be init state should be used - #define PINCTRL_STATE_INIT "init" instead?
I'm not sure I quite understand -- how ?
Sry, for delayed reply.
I've looked at m_can code and think issue is a little bit deeper (but I might be wrong as i'm not can expert and below based on code review).
First, what going on: probe: really_probe() pinctrl_bind_pins() if (IS_ERR(dev->pins->init_state)) { ret = pinctrl_select_state(dev->pins->p, dev->pins->default_state); } else { ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state); } [GS] So at this point default_state or init_state is set
ret = dev->bus->probe(dev); m_can_plat_probe() m_can_class_register() m_can_clk_start() pm_runtime_get_sync() m_can_runtime_resume() [GS] Still default_state or init_state is active
register_m_can_dev() [GS] at this point m_can netdev is registered, which may lead to .ndo_open = m_can_open() call
m_can_clk_stop() pm_runtime_put_sync() [GS] if .ndo_open() was called before it will be a nop m_can_runtime_suspend() m_can_class_suspend()
if (netif_running(ndev)) { netif_stop_queue(ndev); netif_device_detach(ndev); m_can_stop(ndev); m_can_clk_stop(cdev); [GS] if .ndo_open() was called before it will lead to deadlock here So, most probably, it will cause deadlock in case of "ifconfig <m_can_dev> up down" case }
pinctrl_pm_select_sleep_state(dev); [GS] at this point sleep_state will be set - i assume it's the root cause of your glitch. Note - As per code, the pinctrl default_state will never ever configured again, so if after probe m_can will go through PM runtime suspend/resume cycle it will not work any more.
pinctrl_init_done() [GS] will do nothing in case !init_state
As per above, if sleep_state is defined the m_can seems should not work at all without your patch, as there is no code path to switch back sleep_state->default_state. And over all PM runtime m_can code is mixed with System suspend code and so not correct.
Also, the very good question - Is it really required to toggle pinctrl states as part of PM runtime? (usually it's enough to handle it only during System suspend).
I suspect this discussion is somewhat a separate topic from what this patch is trying to fix ?
Not exactly.
I see ?
The reason you need this patch is misuse of PM runtime vs pin control in this driver. And this has to be fixed first of all.
But then the C_CAN also misuses the PM runtime ? I mean, this patch does literally what the same patch for C_CAN does, so maybe this is a more general problem and needs a separate fix -- unless tristating the pins when the block if disabled is the right thing to do, which it might be.
I feel that just removing of m_can_class_suspend() call from m_can_runtime_suspend() will fix things for you - it will toggle pin states only during Suspend to RAM cycle.
I need to configure the pins on boot, this has nothing to do with suspend/resume.
Then just use default_state in DT and do not define sleep state. Sry, I see no reason for your patch at all.
And please, try my proposal - don't make me feel I wasted my time doing all above analysis.
Note. commit ab78029 (drivers/pinctrl: grab default handles from device core) is from Tue Jan 22 10:56:14 2013, while m_can_platfrom is from Thu May 9 11:11:05 2019. So, it's incorrect to even say in commit message that smth. was changed for m_can "Since commit ab78029".
On 12/21/19 12:12 PM, Grygorii Strashko wrote:
On 21/12/2019 13:00, Marek Vasut wrote:
On 12/21/19 11:53 AM, Grygorii Strashko wrote:
On 19/12/2019 23:47, Marek Vasut wrote:
On 12/19/19 2:37 PM, Grygorii Strashko wrote:
On 17/12/2019 12:55, Marek Vasut wrote:
On 12/17/19 11:42 AM, Grygorii Strashko wrote: > > > On 17/12/2019 12:07, Marek Vasut wrote: >> The current code causes a slight glitch on the pinctrl settings >> when >> used. >> Since commit ab78029 (drivers/pinctrl: grab default handles from >> device core), >> the device core will automatically set the default pins. This >> causes >> the pins >> to be momentarily set to the default and then to the sleep state in >> register_m_can_dev(). By adding an optional "enable" state, boards >> can >> set the >> default pin state to be disabled and avoid the glitch when the >> switch >> from >> default to sleep first occurs. If the "enable" state is not >> available >> pinctrl_get_select() falls back to using the "default" pinctrl >> state. >> >> Fixes: c9b3bce18da4 ("can: m_can: select pinctrl state in each >> suspend/resume function") >> Signed-off-by: Marek Vasut marex@denx.de >> Cc: Bich Hemon bich.hemon@st.com >> Cc: Grygorii Strashko grygorii.strashko@ti.com >> Cc: J.D. Schroeder jay.schroeder@garmin.com >> Cc: Marc Kleine-Budde mkl@pengutronix.de >> Cc: Roger Quadros rogerq@ti.com >> Cc: linux-stable stable@vger.kernel.org >> To: linux-can@vger.kernel.org >> --- >> NOTE: This is commit 033365191136 ("can: c_can: Fix default pinmux >> glitch at init") >> adapted for m_can driver. >> --- >> drivers/net/can/m_can/m_can.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/net/can/m_can/m_can.c >> b/drivers/net/can/m_can/m_can.c >> index 02c5795b73936..afb6760b17427 100644 >> --- a/drivers/net/can/m_can/m_can.c >> +++ b/drivers/net/can/m_can/m_can.c >> @@ -1243,12 +1243,20 @@ static void m_can_chip_config(struct >> net_device *dev) >> static void m_can_start(struct net_device *dev) >> { >> struct m_can_classdev *cdev = netdev_priv(dev); >> + struct pinctrl *p; >> /* basic m_can configuration */ >> m_can_chip_config(dev); >> cdev->can.state = CAN_STATE_ERROR_ACTIVE; >> + /* Attempt to use "active" if available else use >> "default" */ >> + p = pinctrl_get_select(cdev->dev, "active"); >> + if (!IS_ERR(p)) >> + pinctrl_put(p); >> + else >> + pinctrl_pm_select_default_state(cdev->dev); >> + >> m_can_enable_all_interrupts(cdev); >> } >> > > May be init state should be used - #define PINCTRL_STATE_INIT "init" > instead?
I'm not sure I quite understand -- how ?
Sry, for delayed reply.
I've looked at m_can code and think issue is a little bit deeper (but I might be wrong as i'm not can expert and below based on code review).
First, what going on: probe: really_probe() pinctrl_bind_pins() if (IS_ERR(dev->pins->init_state)) { ret = pinctrl_select_state(dev->pins->p, dev->pins->default_state); } else { ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state); } [GS] So at this point default_state or init_state is set
ret = dev->bus->probe(dev); m_can_plat_probe() m_can_class_register() m_can_clk_start() pm_runtime_get_sync() m_can_runtime_resume() [GS] Still default_state or init_state is active
register_m_can_dev() [GS] at this point m_can netdev is registered, which may lead to .ndo_open = m_can_open() call
m_can_clk_stop() pm_runtime_put_sync() [GS] if .ndo_open() was called before it will be a nop m_can_runtime_suspend() m_can_class_suspend()
if (netif_running(ndev)) { netif_stop_queue(ndev); netif_device_detach(ndev); m_can_stop(ndev); m_can_clk_stop(cdev); [GS] if .ndo_open() was called before it will lead to deadlock here So, most probably, it will cause deadlock in case of "ifconfig <m_can_dev> up down" case }
pinctrl_pm_select_sleep_state(dev); [GS] at this point sleep_state will be set - i assume it's the root cause of your glitch. Note - As per code, the pinctrl default_state will never ever configured again, so if after probe m_can will go through PM runtime suspend/resume cycle it will not work any more.
pinctrl_init_done() [GS] will do nothing in case !init_state
As per above, if sleep_state is defined the m_can seems should not work at all without your patch, as there is no code path to switch back sleep_state->default_state. And over all PM runtime m_can code is mixed with System suspend code and so not correct.
Also, the very good question - Is it really required to toggle pinctrl states as part of PM runtime? (usually it's enough to handle it only during System suspend).
I suspect this discussion is somewhat a separate topic from what this patch is trying to fix ?
Not exactly.
I see ?
The reason you need this patch is misuse of PM runtime vs pin control in this driver. And this has to be fixed first of all.
But then the C_CAN also misuses the PM runtime ? I mean, this patch does literally what the same patch for C_CAN does, so maybe this is a more general problem and needs a separate fix -- unless tristating the pins when the block if disabled is the right thing to do, which it might be.
I feel that just removing of m_can_class_suspend() call from m_can_runtime_suspend() will fix things for you - it will toggle pin states only during Suspend to RAM cycle.
I need to configure the pins on boot, this has nothing to do with suspend/resume.
Then just use default_state in DT and do not define sleep state. Sry, I see no reason for your patch at all.
Sry, my board does not work without this patch, so I see reason enough. Presumably the author of the C_CAN patch did see similar reason.
Mind you, STM32MP1 does define both states already.
And please, try my proposal - don't make me feel I wasted my time doing all above analysis.
But your proposal stops switching the pin states when the core is suspended, I don't think that's what's it supposed to do.
Note. commit ab78029 (drivers/pinctrl: grab default handles from device core) is from Tue Jan 22 10:56:14 2013, while m_can_platfrom is from Thu May 9 11:11:05 2019. So, it's incorrect to even say in commit message that smth. was changed for m_can "Since commit ab78029".
That's not what the commit message says though.
On 21/12/2019 13:47, Marek Vasut wrote:
On 12/21/19 12:12 PM, Grygorii Strashko wrote:
On 21/12/2019 13:00, Marek Vasut wrote:
On 12/21/19 11:53 AM, Grygorii Strashko wrote:
On 19/12/2019 23:47, Marek Vasut wrote:
On 12/19/19 2:37 PM, Grygorii Strashko wrote:
On 17/12/2019 12:55, Marek Vasut wrote: > On 12/17/19 11:42 AM, Grygorii Strashko wrote: >> >> >> On 17/12/2019 12:07, Marek Vasut wrote: >>> The current code causes a slight glitch on the pinctrl settings >>> when >>> used. >>> Since commit ab78029 (drivers/pinctrl: grab default handles from >>> device core), >>> the device core will automatically set the default pins. This >>> causes >>> the pins >>> to be momentarily set to the default and then to the sleep state in >>> register_m_can_dev(). By adding an optional "enable" state, boards >>> can >>> set the >>> default pin state to be disabled and avoid the glitch when the >>> switch >>> from >>> default to sleep first occurs. If the "enable" state is not >>> available >>> pinctrl_get_select() falls back to using the "default" pinctrl >>> state. >>> >>> Fixes: c9b3bce18da4 ("can: m_can: select pinctrl state in each >>> suspend/resume function") >>> Signed-off-by: Marek Vasut marex@denx.de >>> Cc: Bich Hemon bich.hemon@st.com >>> Cc: Grygorii Strashko grygorii.strashko@ti.com >>> Cc: J.D. Schroeder jay.schroeder@garmin.com >>> Cc: Marc Kleine-Budde mkl@pengutronix.de >>> Cc: Roger Quadros rogerq@ti.com >>> Cc: linux-stable stable@vger.kernel.org >>> To: linux-can@vger.kernel.org >>> --- >>> NOTE: This is commit 033365191136 ("can: c_can: Fix default pinmux >>> glitch at init") >>> adapted for m_can driver. >>> --- >>> drivers/net/can/m_can/m_can.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/drivers/net/can/m_can/m_can.c >>> b/drivers/net/can/m_can/m_can.c >>> index 02c5795b73936..afb6760b17427 100644 >>> --- a/drivers/net/can/m_can/m_can.c >>> +++ b/drivers/net/can/m_can/m_can.c >>> @@ -1243,12 +1243,20 @@ static void m_can_chip_config(struct >>> net_device *dev) >>> static void m_can_start(struct net_device *dev) >>> { >>> struct m_can_classdev *cdev = netdev_priv(dev); >>> + struct pinctrl *p; >>> /* basic m_can configuration */ >>> m_can_chip_config(dev); >>> cdev->can.state = CAN_STATE_ERROR_ACTIVE; >>> + /* Attempt to use "active" if available else use >>> "default" */ >>> + p = pinctrl_get_select(cdev->dev, "active"); >>> + if (!IS_ERR(p)) >>> + pinctrl_put(p); >>> + else >>> + pinctrl_pm_select_default_state(cdev->dev); >>> + >>> m_can_enable_all_interrupts(cdev); >>> } >>> >> >> May be init state should be used - #define PINCTRL_STATE_INIT "init" >> instead? > > I'm not sure I quite understand -- how ? >
Sry, for delayed reply.
I've looked at m_can code and think issue is a little bit deeper (but I might be wrong as i'm not can expert and below based on code review).
First, what going on: probe: really_probe() pinctrl_bind_pins() if (IS_ERR(dev->pins->init_state)) { ret = pinctrl_select_state(dev->pins->p, dev->pins->default_state); } else { ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state); } [GS] So at this point default_state or init_state is set
ret = dev->bus->probe(dev); m_can_plat_probe() m_can_class_register() m_can_clk_start() pm_runtime_get_sync() m_can_runtime_resume() [GS] Still default_state or init_state is active
register_m_can_dev() [GS] at this point m_can netdev is registered, which may lead to .ndo_open = m_can_open() call
m_can_clk_stop() pm_runtime_put_sync() [GS] if .ndo_open() was called before it will be a nop m_can_runtime_suspend() m_can_class_suspend()
if (netif_running(ndev)) { netif_stop_queue(ndev); netif_device_detach(ndev); m_can_stop(ndev); m_can_clk_stop(cdev); [GS] if .ndo_open() was called before it will lead to deadlock here So, most probably, it will cause deadlock in case of "ifconfig <m_can_dev> up down" case }
pinctrl_pm_select_sleep_state(dev); [GS] at this point sleep_state will be set - i assume it's the root cause of your glitch. Note - As per code, the pinctrl default_state will never ever configured again, so if after probe m_can will go through PM runtime suspend/resume cycle it will not work any more.
pinctrl_init_done() [GS] will do nothing in case !init_state
As per above, if sleep_state is defined the m_can seems should not work at all without your patch, as there is no code path to switch back sleep_state->default_state. And over all PM runtime m_can code is mixed with System suspend code and so not correct.
Also, the very good question - Is it really required to toggle pinctrl states as part of PM runtime? (usually it's enough to handle it only during System suspend).
I suspect this discussion is somewhat a separate topic from what this patch is trying to fix ?
Not exactly.
I see ?
The reason you need this patch is misuse of PM runtime vs pin control in this driver. And this has to be fixed first of all.
But then the C_CAN also misuses the PM runtime ? I mean, this patch does literally what the same patch for C_CAN does, so maybe this is a more general problem and needs a separate fix -- unless tristating the pins when the block if disabled is the right thing to do, which it might be.
I feel that just removing of m_can_class_suspend() call from m_can_runtime_suspend() will fix things for you - it will toggle pin states only during Suspend to RAM cycle.
I need to configure the pins on boot, this has nothing to do with suspend/resume.
Then just use default_state in DT and do not define sleep state. Sry, I see no reason for your patch at all.
Sry, my board does not work without this patch, so I see reason enough. Presumably the author of the C_CAN patch did see similar reason.
You continue referring to C_CAN, but it's implemented in a different way. 1) It has no PM runtime callbacks in c_can_platform.c 2) it manually switches the pin states in .ndo_open()/close 3) it has requirement to enable pins after HW IP is enabled
Mind you, STM32MP1 does define both states already.
And please, try my proposal - don't make me feel I wasted my time doing all above analysis.
But your proposal stops switching the pin states when the core is suspended, I don't think that's what's it supposed to do.
Right, but a) it expected to fix your issue b) fix deadlock on if config down
And: What exactly do you mean by "core is suspended"? And how is expected to work from you point of view - can interface dowm-> sleep pins; can interface up -> default/active pins;? smth else? How has it worked before c_can_platform.c was introduced? And what other people should after your patch - add non documented "active" state to every m_can DT node?
I understand you issue, but your patch is not a fix - it's w/a and root cause has to be fixed.
From this discussion, it seems that right way to fix it could be: 1) fix m_can_runtime_suspend() as per above 2) move pinctrl_pm_select_xx() in m_can.c .ndo_open/ndo_close() some where 3) add "init" state == "sleep" state and in m_can_class_register() (or register_m_can_dev()) force "sleep" state. m_can should be kept in "sleep" state this way until m_can.c .ndo_open() is called.
[1] commit ef0eebc05130 ("drivers/pinctrl: Add the concept of an "init" state")
linux-stable-mirror@lists.linaro.org
-
Grygorii Strashko -
Marek Vasut