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".