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



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux