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