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@xxxxxxx>
>>>>>> Cc: Bich Hemon <bich.hemon@xxxxxx>
>>>>>> Cc: Grygorii Strashko <grygorii.strashko@xxxxxx>
>>>>>> Cc: J.D. Schroeder <jay.schroeder@xxxxxxxxxx>
>>>>>> Cc: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
>>>>>> Cc: Roger Quadros <rogerq@xxxxxx>
>>>>>> Cc: linux-stable <stable@xxxxxxxxxxxxxxx>
>>>>>> To: linux-can@xxxxxxxxxxxxxxx
>>>>>> ---
>>>>>> 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.
