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.