Re: [PATCH] can: m_can: Fix default pinmux glitch at init

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Automotive Discussions]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [CAN Bus]

  Powered by Linux