[PATCH 0/4] Add PM support to STM32 LP Timer drivers

[PATCH 0/4] Add PM support to STM32 LP Timer drivers

[Fabrice Gasnier]

This patch series adds power management support for STM32 LP Timer
drivers:
- PWM driver
- IIO counter driver
- Document the pinctrl states for sleep mode

Fabrice Gasnier (4):
  dt-bindings: pwm-stm32-lp: document pinctrl sleep state
  pwm: stm32-lp: Add power management support
  dt-bindings: iio: stm32-lptimer-counter: document pinctrl sleep state
  iio: counter: stm32-lptimer: Add power management support

 .../bindings/iio/counter/stm32-lptimer-cnt.txt     |  8 ++--
 .../devicetree/bindings/pwm/pwm-stm32-lp.txt       |  9 ++--
 drivers/iio/counter/stm32-lptimer-cnt.c            | 55 ++++++++++++++++++++++
 drivers/pwm/pwm-stm32-lp.c                         | 38 +++++++++++++++
 4 files changed, 104 insertions(+), 6 deletions(-)

-- 
1.9.1

[PATCH 1/4] dt-bindings: pwm-stm32-lp: document pinctrl sleep state

[Fabrice Gasnier]

Add documentation for pinctrl sleep state on STM32 LPTimer PWM.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 Documentation/devicetree/bindings/pwm/pwm-stm32-lp.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32-lp.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32-lp.txt
index bd23302..6521bc4 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-stm32-lp.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-stm32-lp.txt
@@ -11,8 +11,10 @@ Required parameters:
 			bindings defined in pwm.txt.
 
 Optional properties:
-- pinctrl-names: 	Set to "default".
-- pinctrl-0: 		Phandle pointing to pin configuration node for PWM.
+- pinctrl-names: 	Set to "default". An additional "sleep" state can be
+			defined to set pins in sleep state when in low power.
+- pinctrl-n: 		Phandle(s) pointing to pin configuration node for PWM,
+			respectively for "default" and "sleep" states.
 
 Example:
 	timer@40002400 {
@@ -21,7 +23,8 @@ Example:
 		pwm {
 			compatible = "st,stm32-pwm-lp";
 			#pwm-cells = <3>;
-			pinctrl-names = "default";
+			pinctrl-names = "default", "sleep";
 			pinctrl-0 = <&lppwm1_pins>;
+			pinctrl-1 = <&lppwm1_sleep_pins>;
 		};
 	};
-- 
1.9.1

[PATCH 2/4] pwm: stm32-lp: Add power management support

[Fabrice Gasnier]

Add suspend/resume PM sleep ops. When going to low power, disable
active PWM channel. Active PWM channel is resumed, by calling
pwm_apply_state(). This is inspired by Thierry's comment in [1].
Don't touch inactive channels, as it may be used by other LPTimer MFD
child driver.
[1]https://lkml.org/lkml/2017/12/5/175

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/pwm/pwm-stm32-lp.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c
index 0059b24c..0c40d48 100644
--- a/drivers/pwm/pwm-stm32-lp.c
+++ b/drivers/pwm/pwm-stm32-lp.c
@@ -13,6 +13,7 @@
 #include <linux/mfd/stm32-lptimer.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 
@@ -20,6 +21,8 @@ struct stm32_pwm_lp {
 	struct pwm_chip chip;
 	struct clk *clk;
 	struct regmap *regmap;
+	struct pwm_state suspend;
+	bool suspended;
 };
 
 static inline struct stm32_pwm_lp *to_stm32_pwm_lp(struct pwm_chip *chip)
@@ -223,6 +226,40 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev)
 	return pwmchip_remove(&priv->chip);
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int stm32_pwm_lp_suspend(struct device *dev)
+{
+	struct stm32_pwm_lp *priv = dev_get_drvdata(dev);
+
+	pwm_get_state(&priv->chip.pwms[0], &priv->suspend);
+	priv->suspended = priv->suspend.enabled;
+
+	/* safe to call pwm_disable() for already disabled pwm */
+	pwm_disable(&priv->chip.pwms[0]);
+
+	return pinctrl_pm_select_sleep_state(dev);
+}
+
+static int stm32_pwm_lp_resume(struct device *dev)
+{
+	struct stm32_pwm_lp *priv = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pinctrl_pm_select_default_state(dev);
+	if (ret)
+		return ret;
+
+	/* Only restore suspended pwm, not to disrupt other MFD child */
+	if (!priv->suspended)
+		return 0;
+
+	return pwm_apply_state(&priv->chip.pwms[0], &priv->suspend);
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(stm32_pwm_lp_pm_ops, stm32_pwm_lp_suspend,
+			 stm32_pwm_lp_resume);
+
 static const struct of_device_id stm32_pwm_lp_of_match[] = {
 	{ .compatible = "st,stm32-pwm-lp", },
 	{},
@@ -235,6 +272,7 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev)
 	.driver	= {
 		.name = "stm32-pwm-lp",
 		.of_match_table = of_match_ptr(stm32_pwm_lp_of_match),
+		.pm = &stm32_pwm_lp_pm_ops,
 	},
 };
 module_platform_driver(stm32_pwm_lp_driver);
-- 
1.9.1

Re: [PATCH 2/4] pwm: stm32-lp: Add power management support

[Tomasz Duszynski]

On Tue, Feb 05, 2019 at 01:40:27PM +0100, Fabrice Gasnier wrote:
> Add suspend/resume PM sleep ops. When going to low power, disable
> active PWM channel. Active PWM channel is resumed, by calling
> pwm_apply_state(). This is inspired by Thierry's comment in [1].
> Don't touch inactive channels, as it may be used by other LPTimer MFD
> child driver.
> [1]https://lkml.org/lkml/2017/12/5/175
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
>  drivers/pwm/pwm-stm32-lp.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c
> index 0059b24c..0c40d48 100644
> --- a/drivers/pwm/pwm-stm32-lp.c
> +++ b/drivers/pwm/pwm-stm32-lp.c
> @@ -13,6 +13,7 @@
>  #include <linux/mfd/stm32-lptimer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
>
> @@ -20,6 +21,8 @@ struct stm32_pwm_lp {
>  	struct pwm_chip chip;
>  	struct clk *clk;
>  	struct regmap *regmap;
> +	struct pwm_state suspend;
> +	bool suspended;
>  };
>
>  static inline struct stm32_pwm_lp *to_stm32_pwm_lp(struct pwm_chip *chip)
> @@ -223,6 +226,40 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev)
>  	return pwmchip_remove(&priv->chip);
>  }
>
> +#ifdef CONFIG_PM_SLEEP

You might consider dropping ifdefs and marking pm functions with
__maybe_unused instead. In case CONFIG_PM_SLEEP=n then these two guys
will be removed and pm ops structure will be empty.

> +static int stm32_pwm_lp_suspend(struct device *dev)
> +{
> +	struct stm32_pwm_lp *priv = dev_get_drvdata(dev);
> +

I guess you first need to get platform_device from dev and eventually
stm32_pwm_lp. Wondering how this works now.

> +	pwm_get_state(&priv->chip.pwms[0], &priv->suspend);
> +	priv->suspended = priv->suspend.enabled;
> +
> +	/* safe to call pwm_disable() for already disabled pwm */
> +	pwm_disable(&priv->chip.pwms[0]);
> +
> +	return pinctrl_pm_select_sleep_state(dev);
> +}
> +
> +static int stm32_pwm_lp_resume(struct device *dev)
> +{
> +	struct stm32_pwm_lp *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pinctrl_pm_select_default_state(dev);
> +	if (ret)
> +		return ret;
> +
> +	/* Only restore suspended pwm, not to disrupt other MFD child */
> +	if (!priv->suspended)
> +		return 0;

Would it make sense to use suspend.enabled directly?

> +
> +	return pwm_apply_state(&priv->chip.pwms[0], &priv->suspend);
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(stm32_pwm_lp_pm_ops, stm32_pwm_lp_suspend,
> +			 stm32_pwm_lp_resume);
> +
>  static const struct of_device_id stm32_pwm_lp_of_match[] = {
>  	{ .compatible = "st,stm32-pwm-lp", },
>  	{},
> @@ -235,6 +272,7 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev)
>  	.driver	= {
>  		.name = "stm32-pwm-lp",
>  		.of_match_table = of_match_ptr(stm32_pwm_lp_of_match),
> +		.pm = &stm32_pwm_lp_pm_ops,
>  	},
>  };
>  module_platform_driver(stm32_pwm_lp_driver);
> --
> 1.9.1
>

Re: [PATCH 2/4] pwm: stm32-lp: Add power management support

[Fabrice Gasnier]

On 2/5/19 7:30 PM, Tomasz Duszynski wrote:
> On Tue, Feb 05, 2019 at 01:40:27PM +0100, Fabrice Gasnier wrote:
>> Add suspend/resume PM sleep ops. When going to low power, disable
>> active PWM channel. Active PWM channel is resumed, by calling
>> pwm_apply_state(). This is inspired by Thierry's comment in [1].
>> Don't touch inactive channels, as it may be used by other LPTimer MFD
>> child driver.
>> [1]https://lkml.org/lkml/2017/12/5/175
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> ---
>>  drivers/pwm/pwm-stm32-lp.c | 38 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>
>> diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c
>> index 0059b24c..0c40d48 100644
>> --- a/drivers/pwm/pwm-stm32-lp.c
>> +++ b/drivers/pwm/pwm-stm32-lp.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/mfd/stm32-lptimer.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>> +#include <linux/pinctrl/consumer.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/pwm.h>
>>
>> @@ -20,6 +21,8 @@ struct stm32_pwm_lp {
>>  	struct pwm_chip chip;
>>  	struct clk *clk;
>>  	struct regmap *regmap;
>> +	struct pwm_state suspend;
>> +	bool suspended;
>>  };
>>
>>  static inline struct stm32_pwm_lp *to_stm32_pwm_lp(struct pwm_chip *chip)
>> @@ -223,6 +226,40 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev)
>>  	return pwmchip_remove(&priv->chip);
>>  }
>>
>> +#ifdef CONFIG_PM_SLEEP
> 
> You might consider dropping ifdefs and marking pm functions with
> __maybe_unused instead. In case CONFIG_PM_SLEEP=n then these two guys
> will be removed and pm ops structure will be empty.

Hi Tomasz,

Thanks for this suggestion. I can do this change. I'll wait for more
feedback from Uwe and Thierry before sending a v2 with that.

> 
>> +static int stm32_pwm_lp_suspend(struct device *dev)
>> +{
>> +	struct stm32_pwm_lp *priv = dev_get_drvdata(dev);
>> +
> 
> I guess you first need to get platform_device from dev and eventually
> stm32_pwm_lp. Wondering how this works now.

This should be safe for now. This works because the probe routine calls:
	platform_set_drvdata(pdev, priv);

And the underlying call is dev_set_drvdata()
static inline void platform_set_drvdata(struct platform_device *pdev,
					void *data)
{
	dev_set_drvdata(&pdev->dev, data);
}

> 
>> +	pwm_get_state(&priv->chip.pwms[0], &priv->suspend);
>> +	priv->suspended = priv->suspend.enabled;
>> +
>> +	/* safe to call pwm_disable() for already disabled pwm */
>> +	pwm_disable(&priv->chip.pwms[0]);
>> +
>> +	return pinctrl_pm_select_sleep_state(dev);
>> +}
>> +
>> +static int stm32_pwm_lp_resume(struct device *dev)
>> +{
>> +	struct stm32_pwm_lp *priv = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	ret = pinctrl_pm_select_default_state(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Only restore suspended pwm, not to disrupt other MFD child */
>> +	if (!priv->suspended)
>> +		return 0;
> 
> Would it make sense to use suspend.enabled directly?

I propose to keep priv->suspended. Using 'suspend.enabled' directly
would simply not work as the pwm_disable() call in
stm32_pwm_lp_suspend() routine marks the 'suspend' state.enabled = false.
That's why it's saved in the suspend routine, to be restored upon resume.

Thanks for reviewing,
Best regards,
Fabrice

> 
>> +
>> +	return pwm_apply_state(&priv->chip.pwms[0], &priv->suspend);
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(stm32_pwm_lp_pm_ops, stm32_pwm_lp_suspend,
>> +			 stm32_pwm_lp_resume);
>> +
>>  static const struct of_device_id stm32_pwm_lp_of_match[] = {
>>  	{ .compatible = "st,stm32-pwm-lp", },
>>  	{},
>> @@ -235,6 +272,7 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev)
>>  	.driver	= {
>>  		.name = "stm32-pwm-lp",
>>  		.of_match_table = of_match_ptr(stm32_pwm_lp_of_match),
>> +		.pm = &stm32_pwm_lp_pm_ops,
>>  	},
>>  };
>>  module_platform_driver(stm32_pwm_lp_driver);
>> --
>> 1.9.1
>>

Re: [PATCH 2/4] pwm: stm32-lp: Add power management support

[Uwe Kleine-König]

Hello,

On Tue, Feb 05, 2019 at 01:40:27PM +0100, Fabrice Gasnier wrote:
> Add suspend/resume PM sleep ops. When going to low power, disable
> active PWM channel. Active PWM channel is resumed, by calling
> pwm_apply_state(). This is inspired by Thierry's comment in [1].
> Don't touch inactive channels, as it may be used by other LPTimer MFD
> child driver.
> [1]https://lkml.org/lkml/2017/12/5/175
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
>  drivers/pwm/pwm-stm32-lp.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c
> index 0059b24c..0c40d48 100644
> --- a/drivers/pwm/pwm-stm32-lp.c
> +++ b/drivers/pwm/pwm-stm32-lp.c
> @@ -13,6 +13,7 @@
>  #include <linux/mfd/stm32-lptimer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
>  
> @@ -20,6 +21,8 @@ struct stm32_pwm_lp {
>  	struct pwm_chip chip;
>  	struct clk *clk;
>  	struct regmap *regmap;
> +	struct pwm_state suspend;
> +	bool suspended;
>  };
>  
>  static inline struct stm32_pwm_lp *to_stm32_pwm_lp(struct pwm_chip *chip)
> @@ -223,6 +226,40 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev)
>  	return pwmchip_remove(&priv->chip);
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int stm32_pwm_lp_suspend(struct device *dev)
> +{
> +	struct stm32_pwm_lp *priv = dev_get_drvdata(dev);
> +
> +	pwm_get_state(&priv->chip.pwms[0], &priv->suspend);
> +	priv->suspended = priv->suspend.enabled;
> +
> +	/* safe to call pwm_disable() for already disabled pwm */
> +	pwm_disable(&priv->chip.pwms[0]);
> +
> +	return pinctrl_pm_select_sleep_state(dev);

IMHO a PWM should not stop if the PWM user didn't call pwm_disable() (or
pwm_apply_state() with .enabled = false).

I don't understand all the PM details, but I think there is no defined
order in which devices are signalled to suspend. If so the PWM might be
stopped before its consumer. Then the PWM changes state without the
consumer being aware of that.

I understand Thierry's position in the link you provided in the commit
log consistant with my position here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] pwm: stm32-lp: Add power management support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3416 bytes --]

On Tue, Feb 05, 2019 at 09:47:32PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Feb 05, 2019 at 01:40:27PM +0100, Fabrice Gasnier wrote:
> > Add suspend/resume PM sleep ops. When going to low power, disable
> > active PWM channel. Active PWM channel is resumed, by calling
> > pwm_apply_state(). This is inspired by Thierry's comment in [1].
> > Don't touch inactive channels, as it may be used by other LPTimer MFD
> > child driver.
> > [1]https://lkml.org/lkml/2017/12/5/175
> > 
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> > ---
> >  drivers/pwm/pwm-stm32-lp.c | 38 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> > 
> > diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c
> > index 0059b24c..0c40d48 100644
> > --- a/drivers/pwm/pwm-stm32-lp.c
> > +++ b/drivers/pwm/pwm-stm32-lp.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/mfd/stm32-lptimer.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/pinctrl/consumer.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pwm.h>
> >  
> > @@ -20,6 +21,8 @@ struct stm32_pwm_lp {
> >  	struct pwm_chip chip;
> >  	struct clk *clk;
> >  	struct regmap *regmap;
> > +	struct pwm_state suspend;
> > +	bool suspended;
> >  };
> >  
> >  static inline struct stm32_pwm_lp *to_stm32_pwm_lp(struct pwm_chip *chip)
> > @@ -223,6 +226,40 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev)
> >  	return pwmchip_remove(&priv->chip);
> >  }
> >  
> > +#ifdef CONFIG_PM_SLEEP
> > +static int stm32_pwm_lp_suspend(struct device *dev)
> > +{
> > +	struct stm32_pwm_lp *priv = dev_get_drvdata(dev);
> > +
> > +	pwm_get_state(&priv->chip.pwms[0], &priv->suspend);
> > +	priv->suspended = priv->suspend.enabled;
> > +
> > +	/* safe to call pwm_disable() for already disabled pwm */
> > +	pwm_disable(&priv->chip.pwms[0]);
> > +
> > +	return pinctrl_pm_select_sleep_state(dev);
> 
> IMHO a PWM should not stop if the PWM user didn't call pwm_disable() (or
> pwm_apply_state() with .enabled = false).
> 
> I don't understand all the PM details, but I think there is no defined
> order in which devices are signalled to suspend. If so the PWM might be
> stopped before its consumer. Then the PWM changes state without the
> consumer being aware of that.
> 
> I understand Thierry's position in the link you provided in the commit
> log consistant with my position here.

Agreed, we should let users of the PWM take care of resuming the PWM in
the state and at the point in time that they require so. PWM users will
also likely do a pwm_disable() during their suspend implementation, so
doing this again in a PWM ->suspend() is not necessary, even if perhaps
harmless.

So this leaves only the pinctrl_pm_select_sleep_state() call. That seems
fine, but I'm not sure that that's currently guaranteed to work. I think
we may need to implement device link support in the PWM framework to
guarantee the proper suspend/resume sequencing. Otherwise you may end up
in a situation where the PWM is actually suspended before the user and
glitch the pins before the user has a chance to disable the PWM.

I think it'd be fine to merge the driver change here first before we add
device link support if this works for you. Just mentioning the issue
here in case you ever run into it.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] pwm: stm32-lp: Add power management support

[Fabrice Gasnier]

On 2/5/19 11:25 PM, Thierry Reding wrote:
> On Tue, Feb 05, 2019 at 09:47:32PM +0100, Uwe Kleine-König wrote:
>> Hello,
>>
>> On Tue, Feb 05, 2019 at 01:40:27PM +0100, Fabrice Gasnier wrote:
>>> Add suspend/resume PM sleep ops. When going to low power, disable
>>> active PWM channel. Active PWM channel is resumed, by calling
>>> pwm_apply_state(). This is inspired by Thierry's comment in [1].
>>> Don't touch inactive channels, as it may be used by other LPTimer MFD
>>> child driver.
>>> [1]https://lkml.org/lkml/2017/12/5/175
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>> ---
>>>  drivers/pwm/pwm-stm32-lp.c | 38 ++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 38 insertions(+)
>>>
>>> diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c
>>> index 0059b24c..0c40d48 100644
>>> --- a/drivers/pwm/pwm-stm32-lp.c
>>> +++ b/drivers/pwm/pwm-stm32-lp.c
>>> @@ -13,6 +13,7 @@
>>>  #include <linux/mfd/stm32-lptimer.h>
>>>  #include <linux/module.h>
>>>  #include <linux/of.h>
>>> +#include <linux/pinctrl/consumer.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/pwm.h>
>>>  
>>> @@ -20,6 +21,8 @@ struct stm32_pwm_lp {
>>>  	struct pwm_chip chip;
>>>  	struct clk *clk;
>>>  	struct regmap *regmap;
>>> +	struct pwm_state suspend;
>>> +	bool suspended;
>>>  };
>>>  
>>>  static inline struct stm32_pwm_lp *to_stm32_pwm_lp(struct pwm_chip *chip)
>>> @@ -223,6 +226,40 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev)
>>>  	return pwmchip_remove(&priv->chip);
>>>  }
>>>  
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int stm32_pwm_lp_suspend(struct device *dev)
>>> +{
>>> +	struct stm32_pwm_lp *priv = dev_get_drvdata(dev);
>>> +
>>> +	pwm_get_state(&priv->chip.pwms[0], &priv->suspend);
>>> +	priv->suspended = priv->suspend.enabled;
>>> +
>>> +	/* safe to call pwm_disable() for already disabled pwm */
>>> +	pwm_disable(&priv->chip.pwms[0]);
>>> +
>>> +	return pinctrl_pm_select_sleep_state(dev);
>>
>> IMHO a PWM should not stop if the PWM user didn't call pwm_disable() (or
>> pwm_apply_state() with .enabled = false).
>>
>> I don't understand all the PM details, but I think there is no defined
>> order in which devices are signalled to suspend. If so the PWM might be
>> stopped before its consumer. Then the PWM changes state without the
>> consumer being aware of that.
>>
>> I understand Thierry's position in the link you provided in the commit
>> log consistant with my position here.
> 
> Agreed, we should let users of the PWM take care of resuming the PWM in
> the state and at the point in time that they require so. PWM users will
> also likely do a pwm_disable() during their suspend implementation, so
> doing this again in a PWM ->suspend() is not necessary, even if perhaps
> harmless.
> 
> So this leaves only the pinctrl_pm_select_sleep_state() call. That seems
> fine, but I'm not sure that that's currently guaranteed to work. I think
> we may need to implement device link support in the PWM framework to
> guarantee the proper suspend/resume sequencing. Otherwise you may end up
> in a situation where the PWM is actually suspended before the user and
> glitch the pins before the user has a chance to disable the PWM.

Hi Uwe, Thierry,

I agree with both of you on the analysis.

> 
> I think it'd be fine to merge the driver change here first before we add
> device link support if this works for you. Just mentioning the issue
> here in case you ever run into it.

If you agree with the current approach, I can send a V2 with Tomasz's
suggestion to remove the ifdefs and use __maybe_unused instead.

Thanks for reviewing,
Best Regards,
Fabrice
> 
> Thierry
> 

Re: [PATCH 2/4] pwm: stm32-lp: Add power management support

[Uwe Kleine-König]

On Wed, Feb 06, 2019 at 09:42:48AM +0100, Fabrice Gasnier wrote:
> If you agree with the current approach, I can send a V2 with Tomasz's
> suggestion to remove the ifdefs and use __maybe_unused instead.

I think the suspend callback should have something like:

	if (is_still_enabled) {
		/*
		 * The consumer didn't stop us, so refuse to suspend.
		 */
		dev_err(dev, "The consumer didn't stop us, so refuse to suspend.\n");
		return -EBUSY;
	}

This way there are no bad surprises if the pwm is suspended before its
consumer and it's obvious what is missing.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] pwm: stm32-lp: Add power management support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

On Wed, Feb 06, 2019 at 09:54:05AM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 06, 2019 at 09:42:48AM +0100, Fabrice Gasnier wrote:
> > If you agree with the current approach, I can send a V2 with Tomasz's
> > suggestion to remove the ifdefs and use __maybe_unused instead.
> 
> I think the suspend callback should have something like:
> 
> 	if (is_still_enabled) {
> 		/*
> 		 * The consumer didn't stop us, so refuse to suspend.
> 		 */
> 		dev_err(dev, "The consumer didn't stop us, so refuse to suspend.\n");
> 		return -EBUSY;
> 	}
> 
> This way there are no bad surprises if the pwm is suspended before its
> consumer and it's obvious what is missing.

Something that just occurred to me: perhaps as part of pwm_get() we
should track where we were being requested from so that we could give
hints in situations like this as to where the consumer is that forgot
to suspend the PWM.

I suppose we already have pwm_device.label to help with this, but
perhaps we could improve things if we stored __builtin_return_address
during pwm_get() to help users pinpoint where they need to look.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] pwm: stm32-lp: Add power management support

[Fabrice Gasnier]

On 2/6/19 1:55 PM, Thierry Reding wrote:
> On Wed, Feb 06, 2019 at 09:54:05AM +0100, Uwe Kleine-König wrote:
>> On Wed, Feb 06, 2019 at 09:42:48AM +0100, Fabrice Gasnier wrote:
>>> If you agree with the current approach, I can send a V2 with Tomasz's
>>> suggestion to remove the ifdefs and use __maybe_unused instead.
>>
>> I think the suspend callback should have something like:
>>
>> 	if (is_still_enabled) {
>> 		/*
>> 		 * The consumer didn't stop us, so refuse to suspend.
>> 		 */
>> 		dev_err(dev, "The consumer didn't stop us, so refuse to suspend.\n");
>> 		return -EBUSY;
>> 	}
>>
>> This way there are no bad surprises if the pwm is suspended before its
>> consumer and it's obvious what is missing.

Thierry, Uwe,

When the pwm is suspended before its consumer, the bad surprise is the
suspend request will fail... I'm not sure a new attempt may be better.
So, it looks like the only way to have this clean is by implementing the
device link e.g. via pwm_get() ?

> 
> Something that just occurred to me: perhaps as part of pwm_get() we
> should track where we were being requested from so that we could give
> hints in situations like this as to where the consumer is that forgot
> to suspend the PWM.

The current approach handles the situation where the consumer forgot to
suspend the PWM... I can add some warning about that in the suspend
routine, incl the label.

What do you think? What's the best approach ?

Please advise,
BR,
Fabrice
> 
> I suppose we already have pwm_device.label to help with this, but
> perhaps we could improve things if we stored __builtin_return_address
> during pwm_get() to help users pinpoint where they need to look.
> 
> Thierry
> 

[PATCH 3/4] dt-bindings: iio: stm32-lptimer-counter: document pinctrl sleep state

[Fabrice Gasnier]

Add documentation for optional pinctrl sleep state that can be used by
STM32 LPTimer encoder/counter.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 .../devicetree/bindings/iio/counter/stm32-lptimer-cnt.txt         | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/counter/stm32-lptimer-cnt.txt b/Documentation/devicetree/bindings/iio/counter/stm32-lptimer-cnt.txt
index a04aa5c..e90bc47 100644
--- a/Documentation/devicetree/bindings/iio/counter/stm32-lptimer-cnt.txt
+++ b/Documentation/devicetree/bindings/iio/counter/stm32-lptimer-cnt.txt
@@ -10,8 +10,9 @@ See ../mfd/stm32-lptimer.txt for details about the parent node.
 
 Required properties:
 - compatible:		Must be "st,stm32-lptimer-counter".
-- pinctrl-names: 	Set to "default".
-- pinctrl-0: 		List of phandles pointing to pin configuration nodes,
+- pinctrl-names: 	Set to "default". An additional "sleep" state can be
+			defined to set pins in sleep state.
+- pinctrl-n: 		List of phandles pointing to pin configuration nodes,
 			to set IN1/IN2 pins in mode of operation for Low-Power
 			Timer input on external pin.
 
@@ -21,7 +22,8 @@ Example:
 		...
 		counter {
 			compatible = "st,stm32-lptimer-counter";
-			pinctrl-names = "default";
+			pinctrl-names = "default", "sleep";
 			pinctrl-0 = <&lptim1_in_pins>;
+			pinctrl-1 = <&lptim1_sleep_in_pins>;
 		};
 	};
-- 
1.9.1

[PATCH 4/4] iio: counter: stm32-lptimer: Add power management support

[Fabrice Gasnier]

Add suspend/resume PM sleep ops. When going to low power, disable
active counter. Only active counter should be resumed: don't touch
disabled counter, as it may be used by other LPTimer MFD child driver.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/iio/counter/stm32-lptimer-cnt.c | 55 +++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/iio/counter/stm32-lptimer-cnt.c b/drivers/iio/counter/stm32-lptimer-cnt.c
index 42fb8ba..2a49cce 100644
--- a/drivers/iio/counter/stm32-lptimer-cnt.c
+++ b/drivers/iio/counter/stm32-lptimer-cnt.c
@@ -14,6 +14,7 @@
 #include <linux/iio/iio.h>
 #include <linux/mfd/stm32-lptimer.h>
 #include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 
 struct stm32_lptim_cnt {
@@ -23,6 +24,7 @@ struct stm32_lptim_cnt {
 	u32 preset;
 	u32 polarity;
 	u32 quadrature_mode;
+	bool enabled;
 };
 
 static int stm32_lptim_is_enabled(struct stm32_lptim_cnt *priv)
@@ -50,6 +52,7 @@ static int stm32_lptim_set_enable_state(struct stm32_lptim_cnt *priv,
 
 	if (!enable) {
 		clk_disable(priv->clk);
+		priv->enabled = false;
 		return 0;
 	}
 
@@ -79,6 +82,7 @@ static int stm32_lptim_set_enable_state(struct stm32_lptim_cnt *priv,
 		regmap_write(priv->regmap, STM32_LPTIM_CR, 0);
 		return ret;
 	}
+	priv->enabled = true;
 
 	/* Start LP timer in continuous mode */
 	return regmap_update_bits(priv->regmap, STM32_LPTIM_CR,
@@ -361,6 +365,56 @@ static int stm32_lptim_cnt_probe(struct platform_device *pdev)
 	return devm_iio_device_register(&pdev->dev, indio_dev);
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int stm32_lptim_cnt_suspend(struct device *dev)
+{
+	struct stm32_lptim_cnt *priv = dev_get_drvdata(dev);
+	int ret;
+
+	/* Only take care of enabled counter: don't disturb other MFD child */
+	if (priv->enabled) {
+		ret = stm32_lptim_setup(priv, 0);
+		if (ret)
+			return ret;
+
+		ret = stm32_lptim_set_enable_state(priv, 0);
+		if (ret)
+			return ret;
+
+		/* Force enable state for later resume */
+		priv->enabled = true;
+	}
+
+	return pinctrl_pm_select_sleep_state(dev);
+}
+
+static int stm32_lptim_cnt_resume(struct device *dev)
+{
+	struct stm32_lptim_cnt *priv = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pinctrl_pm_select_default_state(dev);
+	if (ret)
+		return ret;
+
+	if (priv->enabled) {
+		priv->enabled = false;
+		ret = stm32_lptim_setup(priv, 1);
+		if (ret)
+			return ret;
+
+		ret = stm32_lptim_set_enable_state(priv, 1);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(stm32_lptim_cnt_pm_ops, stm32_lptim_cnt_suspend,
+			 stm32_lptim_cnt_resume);
+
 static const struct of_device_id stm32_lptim_cnt_of_match[] = {
 	{ .compatible = "st,stm32-lptimer-counter", },
 	{},
@@ -372,6 +426,7 @@ static int stm32_lptim_cnt_probe(struct platform_device *pdev)
 	.driver = {
 		.name = "stm32-lptimer-counter",
 		.of_match_table = stm32_lptim_cnt_of_match,
+		.pm = &stm32_lptim_cnt_pm_ops,
 	},
 };
 module_platform_driver(stm32_lptim_cnt_driver);
-- 
1.9.1

Re: [PATCH 4/4] iio: counter: stm32-lptimer: Add power management support

[Jonathan Cameron]

On Tue, 5 Feb 2019 13:40:29 +0100
Fabrice Gasnier <fabrice.gasnier@st.com> wrote:

> Add suspend/resume PM sleep ops. When going to low power, disable
> active counter. Only active counter should be resumed: don't touch
> disabled counter, as it may be used by other LPTimer MFD child driver.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
I think this looks fine.  I'm not totally sure if patches 3,4 are
separable from 1,2 though so please let me know whether I should take
these now, or let all 4 go in together via some path?

Thanks,

Jonathan

> ---
>  drivers/iio/counter/stm32-lptimer-cnt.c | 55 +++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/drivers/iio/counter/stm32-lptimer-cnt.c b/drivers/iio/counter/stm32-lptimer-cnt.c
> index 42fb8ba..2a49cce 100644
> --- a/drivers/iio/counter/stm32-lptimer-cnt.c
> +++ b/drivers/iio/counter/stm32-lptimer-cnt.c
> @@ -14,6 +14,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/mfd/stm32-lptimer.h>
>  #include <linux/module.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  
>  struct stm32_lptim_cnt {
> @@ -23,6 +24,7 @@ struct stm32_lptim_cnt {
>  	u32 preset;
>  	u32 polarity;
>  	u32 quadrature_mode;
> +	bool enabled;
>  };
>  
>  static int stm32_lptim_is_enabled(struct stm32_lptim_cnt *priv)
> @@ -50,6 +52,7 @@ static int stm32_lptim_set_enable_state(struct stm32_lptim_cnt *priv,
>  
>  	if (!enable) {
>  		clk_disable(priv->clk);
> +		priv->enabled = false;
>  		return 0;
>  	}
>  
> @@ -79,6 +82,7 @@ static int stm32_lptim_set_enable_state(struct stm32_lptim_cnt *priv,
>  		regmap_write(priv->regmap, STM32_LPTIM_CR, 0);
>  		return ret;
>  	}
> +	priv->enabled = true;
>  
>  	/* Start LP timer in continuous mode */
>  	return regmap_update_bits(priv->regmap, STM32_LPTIM_CR,
> @@ -361,6 +365,56 @@ static int stm32_lptim_cnt_probe(struct platform_device *pdev)
>  	return devm_iio_device_register(&pdev->dev, indio_dev);
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int stm32_lptim_cnt_suspend(struct device *dev)
> +{
> +	struct stm32_lptim_cnt *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	/* Only take care of enabled counter: don't disturb other MFD child */
> +	if (priv->enabled) {
> +		ret = stm32_lptim_setup(priv, 0);
> +		if (ret)
> +			return ret;
> +
> +		ret = stm32_lptim_set_enable_state(priv, 0);
> +		if (ret)
> +			return ret;
> +
> +		/* Force enable state for later resume */
> +		priv->enabled = true;
> +	}
> +
> +	return pinctrl_pm_select_sleep_state(dev);
> +}
> +
> +static int stm32_lptim_cnt_resume(struct device *dev)
> +{
> +	struct stm32_lptim_cnt *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pinctrl_pm_select_default_state(dev);
> +	if (ret)
> +		return ret;
> +
> +	if (priv->enabled) {
> +		priv->enabled = false;
> +		ret = stm32_lptim_setup(priv, 1);
> +		if (ret)
> +			return ret;
> +
> +		ret = stm32_lptim_set_enable_state(priv, 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(stm32_lptim_cnt_pm_ops, stm32_lptim_cnt_suspend,
> +			 stm32_lptim_cnt_resume);
> +
>  static const struct of_device_id stm32_lptim_cnt_of_match[] = {
>  	{ .compatible = "st,stm32-lptimer-counter", },
>  	{},
> @@ -372,6 +426,7 @@ static int stm32_lptim_cnt_probe(struct platform_device *pdev)
>  	.driver = {
>  		.name = "stm32-lptimer-counter",
>  		.of_match_table = stm32_lptim_cnt_of_match,
> +		.pm = &stm32_lptim_cnt_pm_ops,
>  	},
>  };
>  module_platform_driver(stm32_lptim_cnt_driver);

Re: [PATCH 4/4] iio: counter: stm32-lptimer: Add power management support

[Uwe Kleine-König]

On Sat, Feb 09, 2019 at 04:21:33PM +0000, Jonathan Cameron wrote:
> On Tue, 5 Feb 2019 13:40:29 +0100
> Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> 
> > Add suspend/resume PM sleep ops. When going to low power, disable
> > active counter. Only active counter should be resumed: don't touch
> > disabled counter, as it may be used by other LPTimer MFD child driver.
> > 
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> I think this looks fine.  I'm not totally sure if patches 3,4 are
> separable from 1,2 though so please let me know whether I should take
> these now, or let all 4 go in together via some path?

I'd say these are orthogonal, so if you are happy with patches 3 and 4
you can take them without bothering for patches 1 and 2.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 4/4] iio: counter: stm32-lptimer: Add power management support

[Fabrice Gasnier]

On 2/10/19 10:33 PM, Uwe Kleine-König wrote:
> On Sat, Feb 09, 2019 at 04:21:33PM +0000, Jonathan Cameron wrote:
>> On Tue, 5 Feb 2019 13:40:29 +0100
>> Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
>>
>>> Add suspend/resume PM sleep ops. When going to low power, disable
>>> active counter. Only active counter should be resumed: don't touch
>>> disabled counter, as it may be used by other LPTimer MFD child driver.
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> I think this looks fine.  I'm not totally sure if patches 3,4 are
>> separable from 1,2 though so please let me know whether I should take
>> these now, or let all 4 go in together via some path?
> 
> I'd say these are orthogonal, so if you are happy with patches 3 and 4
> you can take them without bothering for patches 1 and 2.

Hi Uwe, Jonathan,

Thanks for reviewing. I confirm you can take patches 3 and 4. I'll send
a v2 for patches 1 and 2 separately.

Best regards,
Fabrice

> 
> Best regards
> Uwe
>