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