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

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

[Marek Vasut]

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);
 }
 
-- 
2.24.0.525.g8f36a354ae

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

[Grygorii Strashko]

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?

-- 
Best regards,
grygorii

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

[Grygorii Strashko]

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?

-- 
Best regards,
grygorii

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

[Marek Vasut]

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 ?

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

[Grygorii Strashko]

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

-- 
Best regards,
grygorii

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

[Grygorii Strashko]

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

-- 
Best regards,
grygorii

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

[Marek Vasut]

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 ?

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

[Grygorii Strashko]

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

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.


-- 
Best regards,
grygorii

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

[Grygorii Strashko]

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

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.


-- 
Best regards,
grygorii

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

[Marek Vasut]

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.

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

[Grygorii Strashko]

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.

And please, try my proposal - don't make me feel I wasted my time doing
all above analysis.

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


-- 
Best regards,
grygorii

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

[Grygorii Strashko]

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.

And please, try my proposal - don't make me feel I wasted my time doing
all above analysis.

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


-- 
Best regards,
grygorii

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

[Marek Vasut]

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.

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.

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

[Grygorii Strashko]

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

-- 
Best regards,
grygorii

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

[Grygorii Strashko]

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

-- 
Best regards,
grygorii