[PATCH 0/2] Add PM support to STM32 Timer PWM

[PATCH 0/2] Add PM support to STM32 Timer PWM

[Fabrice Gasnier]

This patch series adds power management support for STM32 Timer PWM:
- Document the pinctrl sleep state for STM32 Timer PWM
- STM32 Timer PWM driver

Fabrice Gasnier (2):
  dt-bindings: pwm-stm32: document pinctrl sleep state
  pwm: stm32: add power management support

 .../devicetree/bindings/pwm/pwm-stm32.txt          |  8 ++-
 drivers/pwm/pwm-stm32.c                            | 82 ++++++++++++++++------
 2 files changed, 67 insertions(+), 23 deletions(-)

-- 
2.7.4

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

[Fabrice Gasnier]

Add documentation for pinctrl sleep state that can be used by
STM32 timers PWM.

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

diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
index a8690bf..f1620c1 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
@@ -5,8 +5,9 @@ See ../mfd/stm32-timers.txt for details about the parent node.
 
 Required parameters:
 - compatible:		Must be "st,stm32-pwm".
-- pinctrl-names: 	Set to "default".
-- pinctrl-0: 		List of phandles pointing to pin configuration nodes for PWM module.
+- pinctrl-names: 	Set to "default". An additional "sleep" state can be
+			defined to set pins in sleep state when in low power.
+- pinctrl-n: 		List of phandles pointing to pin configuration nodes for PWM module.
 			For Pinctrl properties see ../pinctrl/pinctrl-bindings.txt
 - #pwm-cells:		Should be set to 3. This PWM chip uses the default 3 cells
 			bindings defined in pwm.txt.
@@ -32,7 +33,8 @@ Example:
 			compatible = "st,stm32-pwm";
 			#pwm-cells = <3>;
 			pinctrl-0	= <&pwm1_pins>;
-			pinctrl-names	= "default";
+			pinctrl-1	= <&pwm1_sleep_pins>;
+			pinctrl-names	= "default", "sleep";
 			st,breakinput = <0 1 5>;
 		};
 	};
-- 
2.7.4

[PATCH 2/2] pwm: stm32: add power management support

[Fabrice Gasnier]

Add suspend/resume PM sleep ops. When going to low power, enforce the PWM
channel isn't active. Let the PWM consumers disable it during their own
suspend sequence, see [1]. So, perform a check here, and handle the
pinctrl states. Also restore the break inputs upon resume, as registers
content may be lost when going to low power mode.

[1] https://lkml.org/lkml/2019/2/5/770

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

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 740e2de..9bcd73a 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -12,6 +12,7 @@
 #include <linux/mfd/stm32-timers.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 
@@ -19,6 +20,12 @@
 #define CCMR_CHANNEL_MASK  0xFF
 #define MAX_BREAKINPUT 2
 
+struct stm32_breakinput {
+	u32 index;
+	u32 level;
+	u32 filter;
+};
+
 struct stm32_pwm {
 	struct pwm_chip chip;
 	struct mutex lock; /* protect pwm config/enable */
@@ -26,15 +33,11 @@ struct stm32_pwm {
 	struct regmap *regmap;
 	u32 max_arr;
 	bool have_complementary_output;
+	struct stm32_breakinput breakinput[MAX_BREAKINPUT];
+	unsigned int nbreakinput;
 	u32 capture[4] ____cacheline_aligned; /* DMA'able buffer */
 };
 
-struct stm32_breakinput {
-	u32 index;
-	u32 level;
-	u32 filter;
-};
-
 static inline struct stm32_pwm *to_stm32_pwm_dev(struct pwm_chip *chip)
 {
 	return container_of(chip, struct stm32_pwm, chip);
@@ -512,15 +515,27 @@ static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
 	return (bdtr & bke) ? 0 : -EINVAL;
 }
 
-static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
+static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < priv->nbreakinput && !ret; i++) {
+		ret = stm32_pwm_set_breakinput(priv,
+					       priv->breakinput[i].index,
+					       priv->breakinput[i].level,
+					       priv->breakinput[i].filter);
+	}
+
+	return ret;
+}
+
+static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
 				       struct device_node *np)
 {
-	struct stm32_breakinput breakinput[MAX_BREAKINPUT];
-	int nb, ret, i, array_size;
+	int nb, ret, array_size;
 
 	nb = of_property_count_elems_of_size(np, "st,breakinput",
 					     sizeof(struct stm32_breakinput));
-
 	/*
 	 * Because "st,breakinput" parameter is optional do not make probe
 	 * failed if it doesn't exist.
@@ -531,20 +546,14 @@ static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
 	if (nb > MAX_BREAKINPUT)
 		return -EINVAL;
 
+	priv->nbreakinput = nb;
 	array_size = nb * sizeof(struct stm32_breakinput) / sizeof(u32);
 	ret = of_property_read_u32_array(np, "st,breakinput",
-					 (u32 *)breakinput, array_size);
+					 (u32 *)priv->breakinput, array_size);
 	if (ret)
 		return ret;
 
-	for (i = 0; i < nb && !ret; i++) {
-		ret = stm32_pwm_set_breakinput(priv,
-					       breakinput[i].index,
-					       breakinput[i].level,
-					       breakinput[i].filter);
-	}
-
-	return ret;
+	return stm32_pwm_apply_breakinputs(priv);
 }
 
 static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
@@ -614,7 +623,7 @@ static int stm32_pwm_probe(struct platform_device *pdev)
 	if (!priv->regmap || !priv->clk)
 		return -EINVAL;
 
-	ret = stm32_pwm_apply_breakinputs(priv, np);
+	ret = stm32_pwm_probe_breakinputs(priv, np);
 	if (ret)
 		return ret;
 
@@ -647,6 +656,38 @@ static int stm32_pwm_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int __maybe_unused stm32_pwm_suspend(struct device *dev)
+{
+	struct stm32_pwm *priv = dev_get_drvdata(dev);
+	struct pwm_state state;
+	unsigned int i;
+
+	for (i = 0; i < priv->chip.npwm; i++) {
+		pwm_get_state(&priv->chip.pwms[i], &state);
+		if (state.enabled) {
+			dev_err(dev, "The consumer didn't stop us (%s)\n",
+				priv->chip.pwms[i].label);
+			return -EBUSY;
+		}
+	}
+
+	return pinctrl_pm_select_sleep_state(dev);
+}
+
+static int __maybe_unused stm32_pwm_resume(struct device *dev)
+{
+	struct stm32_pwm *priv = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pinctrl_pm_select_default_state(dev);
+	if (ret)
+		return ret;
+
+	return stm32_pwm_apply_breakinputs(priv);
+}
+
+static SIMPLE_DEV_PM_OPS(stm32_pwm_pm_ops, stm32_pwm_suspend, stm32_pwm_resume);
+
 static const struct of_device_id stm32_pwm_of_match[] = {
 	{ .compatible = "st,stm32-pwm",	},
 	{ /* end node */ },
@@ -659,6 +700,7 @@ static struct platform_driver stm32_pwm_driver = {
 	.driver	= {
 		.name = "stm32-pwm",
 		.of_match_table = stm32_pwm_of_match,
+		.pm = &stm32_pwm_pm_ops,
 	},
 };
 module_platform_driver(stm32_pwm_driver);
-- 
2.7.4

Re: [PATCH 2/2] pwm: stm32: add power management support

[Uwe Kleine-König]

Hello Fabrice,

On Mon, Sep 30, 2019 at 05:39:11PM +0200, Fabrice Gasnier wrote:
> Add suspend/resume PM sleep ops. When going to low power, enforce the PWM
> channel isn't active. Let the PWM consumers disable it during their own
> suspend sequence, see [1]. So, perform a check here, and handle the
> pinctrl states. Also restore the break inputs upon resume, as registers
> content may be lost when going to low power mode.
> 
> [1] https://lkml.org/lkml/2019/2/5/770
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
>  drivers/pwm/pwm-stm32.c | 82 +++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 62 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 740e2de..9bcd73a 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -12,6 +12,7 @@
>  #include <linux/mfd/stm32-timers.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
>  
> @@ -19,6 +20,12 @@
>  #define CCMR_CHANNEL_MASK  0xFF
>  #define MAX_BREAKINPUT 2
>  
> +struct stm32_breakinput {
> +	u32 index;
> +	u32 level;
> +	u32 filter;
> +};
> +
>  struct stm32_pwm {
>  	struct pwm_chip chip;
>  	struct mutex lock; /* protect pwm config/enable */
> @@ -26,15 +33,11 @@ struct stm32_pwm {
>  	struct regmap *regmap;
>  	u32 max_arr;
>  	bool have_complementary_output;
> +	struct stm32_breakinput breakinput[MAX_BREAKINPUT];
> +	unsigned int nbreakinput;
>  	u32 capture[4] ____cacheline_aligned; /* DMA'able buffer */
>  };
>  
> -struct stm32_breakinput {
> -	u32 index;
> -	u32 level;
> -	u32 filter;
> -};
> -
>  static inline struct stm32_pwm *to_stm32_pwm_dev(struct pwm_chip *chip)
>  {
>  	return container_of(chip, struct stm32_pwm, chip);
> @@ -512,15 +515,27 @@ static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
>  	return (bdtr & bke) ? 0 : -EINVAL;
>  }
>  
> -static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
> +static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv)
> +{
> +	int i, ret = 0;
> +
> +	for (i = 0; i < priv->nbreakinput && !ret; i++) {
> +		ret = stm32_pwm_set_breakinput(priv,
> +					       priv->breakinput[i].index,
> +					       priv->breakinput[i].level,
> +					       priv->breakinput[i].filter);
> +	}
> +
> +	return ret;
> +}

Can you explain what the effect of this function is? This is something
that is lost during suspend?

I wonder why the patch is so big. There are some rearrangements that
should have no effect and I think it would be beneficial for
reviewability to split this patch in a patch that only does the
restructuring and than on top of that add the PM stuff.

> +
> +static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
>  				       struct device_node *np)
>  {
> -	struct stm32_breakinput breakinput[MAX_BREAKINPUT];
> -	int nb, ret, i, array_size;
> +	int nb, ret, array_size;
>  
>  	nb = of_property_count_elems_of_size(np, "st,breakinput",
>  					     sizeof(struct stm32_breakinput));
> -
>  	/*
>  	 * Because "st,breakinput" parameter is optional do not make probe
>  	 * failed if it doesn't exist.
> @@ -531,20 +546,14 @@ static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
>  	if (nb > MAX_BREAKINPUT)
>  		return -EINVAL;
>  
> +	priv->nbreakinput = nb;
>  	array_size = nb * sizeof(struct stm32_breakinput) / sizeof(u32);
>  	ret = of_property_read_u32_array(np, "st,breakinput",
> -					 (u32 *)breakinput, array_size);
> +					 (u32 *)priv->breakinput, array_size);
>  	if (ret)
>  		return ret;
>  
> -	for (i = 0; i < nb && !ret; i++) {
> -		ret = stm32_pwm_set_breakinput(priv,
> -					       breakinput[i].index,
> -					       breakinput[i].level,
> -					       breakinput[i].filter);
> -	}
> -
> -	return ret;
> +	return stm32_pwm_apply_breakinputs(priv);
>  }
>  
>  static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
> @@ -614,7 +623,7 @@ static int stm32_pwm_probe(struct platform_device *pdev)
>  	if (!priv->regmap || !priv->clk)
>  		return -EINVAL;
>  
> -	ret = stm32_pwm_apply_breakinputs(priv, np);
> +	ret = stm32_pwm_probe_breakinputs(priv, np);
>  	if (ret)
>  		return ret;
>  
> @@ -647,6 +656,38 @@ static int stm32_pwm_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int __maybe_unused stm32_pwm_suspend(struct device *dev)
> +{
> +	struct stm32_pwm *priv = dev_get_drvdata(dev);
> +	struct pwm_state state;
> +	unsigned int i;
> +
> +	for (i = 0; i < priv->chip.npwm; i++) {
> +		pwm_get_state(&priv->chip.pwms[i], &state);

pwm_get_state is a function designed to be used by PWM consumers. I
would prefer to check the hardware registers here instead.

What if there is no consumer and the PWM just happens to be enabled by
the bootloader? Or is this too minor an issue to be worth consideration?

Best regards
Uwe

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

Re: [PATCH 2/2] pwm: stm32: add power management support

[Fabrice Gasnier]

On 10/1/19 9:04 AM, Uwe Kleine-König wrote:
> Hello Fabrice,
> 
> On Mon, Sep 30, 2019 at 05:39:11PM +0200, Fabrice Gasnier wrote:
>> Add suspend/resume PM sleep ops. When going to low power, enforce the PWM
>> channel isn't active. Let the PWM consumers disable it during their own
>> suspend sequence, see [1]. So, perform a check here, and handle the
>> pinctrl states. Also restore the break inputs upon resume, as registers
>> content may be lost when going to low power mode.
>>
>> [1] https://lkml.org/lkml/2019/2/5/770
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> ---
>>  drivers/pwm/pwm-stm32.c | 82 +++++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 62 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
>> index 740e2de..9bcd73a 100644
>> --- a/drivers/pwm/pwm-stm32.c
>> +++ b/drivers/pwm/pwm-stm32.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/mfd/stm32-timers.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>> +#include <linux/pinctrl/consumer.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/pwm.h>
>>  
>> @@ -19,6 +20,12 @@
>>  #define CCMR_CHANNEL_MASK  0xFF
>>  #define MAX_BREAKINPUT 2
>>  
>> +struct stm32_breakinput {
>> +	u32 index;
>> +	u32 level;
>> +	u32 filter;
>> +};
>> +
>>  struct stm32_pwm {
>>  	struct pwm_chip chip;
>>  	struct mutex lock; /* protect pwm config/enable */
>> @@ -26,15 +33,11 @@ struct stm32_pwm {
>>  	struct regmap *regmap;
>>  	u32 max_arr;
>>  	bool have_complementary_output;
>> +	struct stm32_breakinput breakinput[MAX_BREAKINPUT];
>> +	unsigned int nbreakinput;
>>  	u32 capture[4] ____cacheline_aligned; /* DMA'able buffer */
>>  };
>>  
>> -struct stm32_breakinput {
>> -	u32 index;
>> -	u32 level;
>> -	u32 filter;
>> -};
>> -
>>  static inline struct stm32_pwm *to_stm32_pwm_dev(struct pwm_chip *chip)
>>  {
>>  	return container_of(chip, struct stm32_pwm, chip);
>> @@ -512,15 +515,27 @@ static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
>>  	return (bdtr & bke) ? 0 : -EINVAL;
>>  }
>>  
>> -static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
>> +static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv)
>> +{
>> +	int i, ret = 0;
>> +
>> +	for (i = 0; i < priv->nbreakinput && !ret; i++) {
>> +		ret = stm32_pwm_set_breakinput(priv,
>> +					       priv->breakinput[i].index,
>> +					       priv->breakinput[i].level,
>> +					       priv->breakinput[i].filter);
>> +	}
>> +
>> +	return ret;
>> +}
> 
> Can you explain what the effect of this function is? This is something
> that is lost during suspend?

Hi Uwe,

Yes, that's what I explain in the commit message: ...registers content
may be lost when going to low power mode.
Do you think I need to rephrase ?

> 
> I wonder why the patch is so big. There are some rearrangements that
> should have no effect and I think it would be beneficial for
> reviewability to split this patch in a patch that only does the
> restructuring and than on top of that add the PM stuff.

I can split this to ease the review.
> 
>> +
>> +static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
>>  				       struct device_node *np)
>>  {
>> -	struct stm32_breakinput breakinput[MAX_BREAKINPUT];
>> -	int nb, ret, i, array_size;
>> +	int nb, ret, array_size;
>>  
>>  	nb = of_property_count_elems_of_size(np, "st,breakinput",
>>  					     sizeof(struct stm32_breakinput));
>> -
>>  	/*
>>  	 * Because "st,breakinput" parameter is optional do not make probe
>>  	 * failed if it doesn't exist.
>> @@ -531,20 +546,14 @@ static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
>>  	if (nb > MAX_BREAKINPUT)
>>  		return -EINVAL;
>>  
>> +	priv->nbreakinput = nb;
>>  	array_size = nb * sizeof(struct stm32_breakinput) / sizeof(u32);
>>  	ret = of_property_read_u32_array(np, "st,breakinput",
>> -					 (u32 *)breakinput, array_size);
>> +					 (u32 *)priv->breakinput, array_size);
>>  	if (ret)
>>  		return ret;
>>  
>> -	for (i = 0; i < nb && !ret; i++) {
>> -		ret = stm32_pwm_set_breakinput(priv,
>> -					       breakinput[i].index,
>> -					       breakinput[i].level,
>> -					       breakinput[i].filter);
>> -	}
>> -
>> -	return ret;
>> +	return stm32_pwm_apply_breakinputs(priv);
>>  }
>>  
>>  static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
>> @@ -614,7 +623,7 @@ static int stm32_pwm_probe(struct platform_device *pdev)
>>  	if (!priv->regmap || !priv->clk)
>>  		return -EINVAL;
>>  
>> -	ret = stm32_pwm_apply_breakinputs(priv, np);
>> +	ret = stm32_pwm_probe_breakinputs(priv, np);
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -647,6 +656,38 @@ static int stm32_pwm_remove(struct platform_device *pdev)
>>  	return 0;
>>  }
>>  
>> +static int __maybe_unused stm32_pwm_suspend(struct device *dev)
>> +{
>> +	struct stm32_pwm *priv = dev_get_drvdata(dev);
>> +	struct pwm_state state;
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < priv->chip.npwm; i++) {
>> +		pwm_get_state(&priv->chip.pwms[i], &state);
> 
> pwm_get_state is a function designed to be used by PWM consumers. I
> would prefer to check the hardware registers here instead.

It's also useful for PWM provider: This PWM driver is part of a MFD that
also take care of IIO trigger (can be used simultaneously). Simply
reading a register doesn't tell us that the timer is used/configured as
a PWM here. That's the reason to use this helper to read pwm->state.

Do you wish I add a comment to clarify this here ?

> 
> What if there is no consumer and the PWM just happens to be enabled by
> the bootloader? Or is this too minor an issue to be worth consideration?

That's the purpose of returning -EBUSY: "PWM should not stop if the PWM
user didn't call pwm_disable()" ... "to avoid situation where the PWM is
actually suspended before the user". This has been enforced in later
series with the device_link_add(). See our previous discussions here:
https://lkml.org/lkml/2019/2/5/770
So, I guess this would point exactly a lack for a PWM user to manage it
after the boot stage, in the kernel.

Could you please clarify, provide an example here ?

Thanks for reviewing,
BR,
Fabrice

> 
> Best regards
> Uwe
> 

Re: [PATCH 2/2] pwm: stm32: add power management support

[Uwe Kleine-König]

Hello Fabrice,

On Tue, Oct 01, 2019 at 10:18:31AM +0200, Fabrice Gasnier wrote:
> On 10/1/19 9:04 AM, Uwe Kleine-König wrote:
> > On Mon, Sep 30, 2019 at 05:39:11PM +0200, Fabrice Gasnier wrote:
> >> Add suspend/resume PM sleep ops. When going to low power, enforce the PWM
> >> channel isn't active. Let the PWM consumers disable it during their own
> >> suspend sequence, see [1]. So, perform a check here, and handle the
> >> pinctrl states. Also restore the break inputs upon resume, as registers
> >> content may be lost when going to low power mode.
> >>
> >> [1] https://lkml.org/lkml/2019/2/5/770
> >>
> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> >> ---
> >>  drivers/pwm/pwm-stm32.c | 82 +++++++++++++++++++++++++++++++++++++------------
> >>  1 file changed, 62 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> >> index 740e2de..9bcd73a 100644
> >> --- a/drivers/pwm/pwm-stm32.c
> >> +++ b/drivers/pwm/pwm-stm32.c
> >> @@ -12,6 +12,7 @@
> >>  #include <linux/mfd/stm32-timers.h>
> >>  #include <linux/module.h>
> >>  #include <linux/of.h>
> >> +#include <linux/pinctrl/consumer.h>
> >>  #include <linux/platform_device.h>
> >>  #include <linux/pwm.h>
> >>  
> >> @@ -19,6 +20,12 @@
> >>  #define CCMR_CHANNEL_MASK  0xFF
> >>  #define MAX_BREAKINPUT 2
> >>  
> >> +struct stm32_breakinput {
> >> +	u32 index;
> >> +	u32 level;
> >> +	u32 filter;
> >> +};
> >> +
> >>  struct stm32_pwm {
> >>  	struct pwm_chip chip;
> >>  	struct mutex lock; /* protect pwm config/enable */
> >> @@ -26,15 +33,11 @@ struct stm32_pwm {
> >>  	struct regmap *regmap;
> >>  	u32 max_arr;
> >>  	bool have_complementary_output;
> >> +	struct stm32_breakinput breakinput[MAX_BREAKINPUT];
> >> +	unsigned int nbreakinput;
> >>  	u32 capture[4] ____cacheline_aligned; /* DMA'able buffer */
> >>  };
> >>  
> >> -struct stm32_breakinput {
> >> -	u32 index;
> >> -	u32 level;
> >> -	u32 filter;
> >> -};
> >> -
> >>  static inline struct stm32_pwm *to_stm32_pwm_dev(struct pwm_chip *chip)
> >>  {
> >>  	return container_of(chip, struct stm32_pwm, chip);
> >> @@ -512,15 +515,27 @@ static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
> >>  	return (bdtr & bke) ? 0 : -EINVAL;
> >>  }
> >>  
> >> -static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
> >> +static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv)
> >> +{
> >> +	int i, ret = 0;
> >> +
> >> +	for (i = 0; i < priv->nbreakinput && !ret; i++) {
> >> +		ret = stm32_pwm_set_breakinput(priv,
> >> +					       priv->breakinput[i].index,
> >> +					       priv->breakinput[i].level,
> >> +					       priv->breakinput[i].filter);
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> > 
> > Can you explain what the effect of this function is? This is something
> > that is lost during suspend?
> 
> Yes, that's what I explain in the commit message: ...registers content
> may be lost when going to low power mode.
> Do you think I need to rephrase ?

Ah, right I missed it in the commit log. It might be worth adding that
to a code comment. Also having the purpose of this function described
would be great. Does it configure some electrical characteristics? Or
has it to do with pinmuxing? Why is an input relevant for a PWM?

> > I wonder why the patch is so big. There are some rearrangements that
> > should have no effect and I think it would be beneficial for
> > reviewability to split this patch in a patch that only does the
> > restructuring and than on top of that add the PM stuff.
> 
> I can split this to ease the review.
> > 
> >> +
> >> +static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
> >>  				       struct device_node *np)
> >>  {
> >> -	struct stm32_breakinput breakinput[MAX_BREAKINPUT];
> >> -	int nb, ret, i, array_size;
> >> +	int nb, ret, array_size;
> >>  
> >>  	nb = of_property_count_elems_of_size(np, "st,breakinput",
> >>  					     sizeof(struct stm32_breakinput));
> >> -
> >>  	/*
> >>  	 * Because "st,breakinput" parameter is optional do not make probe
> >>  	 * failed if it doesn't exist.
> >> @@ -531,20 +546,14 @@ static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
> >>  	if (nb > MAX_BREAKINPUT)
> >>  		return -EINVAL;
> >>  
> >> +	priv->nbreakinput = nb;
> >>  	array_size = nb * sizeof(struct stm32_breakinput) / sizeof(u32);
> >>  	ret = of_property_read_u32_array(np, "st,breakinput",
> >> -					 (u32 *)breakinput, array_size);
> >> +					 (u32 *)priv->breakinput, array_size);
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> -	for (i = 0; i < nb && !ret; i++) {
> >> -		ret = stm32_pwm_set_breakinput(priv,
> >> -					       breakinput[i].index,
> >> -					       breakinput[i].level,
> >> -					       breakinput[i].filter);
> >> -	}
> >> -
> >> -	return ret;
> >> +	return stm32_pwm_apply_breakinputs(priv);
> >>  }
> >>  
> >>  static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
> >> @@ -614,7 +623,7 @@ static int stm32_pwm_probe(struct platform_device *pdev)
> >>  	if (!priv->regmap || !priv->clk)
> >>  		return -EINVAL;
> >>  
> >> -	ret = stm32_pwm_apply_breakinputs(priv, np);
> >> +	ret = stm32_pwm_probe_breakinputs(priv, np);
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> @@ -647,6 +656,38 @@ static int stm32_pwm_remove(struct platform_device *pdev)
> >>  	return 0;
> >>  }
> >>  
> >> +static int __maybe_unused stm32_pwm_suspend(struct device *dev)
> >> +{
> >> +	struct stm32_pwm *priv = dev_get_drvdata(dev);
> >> +	struct pwm_state state;
> >> +	unsigned int i;
> >> +
> >> +	for (i = 0; i < priv->chip.npwm; i++) {
> >> +		pwm_get_state(&priv->chip.pwms[i], &state);
> > 
> > pwm_get_state is a function designed to be used by PWM consumers. I
> > would prefer to check the hardware registers here instead.
> 
> It's also useful for PWM provider: This PWM driver is part of a MFD that

I don't doubt "useful". But still you should only call it if you called
pwm_get (or a similar function) to get a PWM handle.

> also take care of IIO trigger (can be used simultaneously). Simply
> reading a register doesn't tell us that the timer is used/configured as
> a PWM here. That's the reason to use this helper to read pwm->state.

How can the pwm driver be bound and the hardware not be used a PWM?

> Do you wish I add a comment to clarify this here ?

No, I wish you inspect the hardware to determine what you need to know :-)

> > What if there is no consumer and the PWM just happens to be enabled by
> > the bootloader? Or is this too minor an issue to be worth consideration?
> 
> That's the purpose of returning -EBUSY: "PWM should not stop if the PWM
> user didn't call pwm_disable()" ... "to avoid situation where the PWM is
> actually suspended before the user". This has been enforced in later
> series with the device_link_add(). See our previous discussions here:
> https://lkml.org/lkml/2019/2/5/770
> So, I guess this would point exactly a lack for a PWM user to manage it
> after the boot stage, in the kernel.
> 
> Could you please clarify, provide an example here ?

This is something different than I asked for. Not having a consumer
isn't an error. Still the pwm might be running (for a good reason or
not). (This is more a question that affects how a driver should behave
in general, it is not specific to the stm32 driver here.)

Best regards
Uwe

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

Re: [PATCH 2/2] pwm: stm32: add power management support

[Fabrice Gasnier]

On 10/1/19 11:51 AM, Uwe Kleine-König wrote:
> Hello Fabrice,
> 
> On Tue, Oct 01, 2019 at 10:18:31AM +0200, Fabrice Gasnier wrote:
>> On 10/1/19 9:04 AM, Uwe Kleine-König wrote:
>>> On Mon, Sep 30, 2019 at 05:39:11PM +0200, Fabrice Gasnier wrote:
>>>> Add suspend/resume PM sleep ops. When going to low power, enforce the PWM
>>>> channel isn't active. Let the PWM consumers disable it during their own
>>>> suspend sequence, see [1]. So, perform a check here, and handle the
>>>> pinctrl states. Also restore the break inputs upon resume, as registers
>>>> content may be lost when going to low power mode.
>>>>
>>>> [1] https://lkml.org/lkml/2019/2/5/770
>>>>
>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>>> ---
>>>>  drivers/pwm/pwm-stm32.c | 82 +++++++++++++++++++++++++++++++++++++------------
>>>>  1 file changed, 62 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
>>>> index 740e2de..9bcd73a 100644
>>>> --- a/drivers/pwm/pwm-stm32.c
>>>> +++ b/drivers/pwm/pwm-stm32.c
>>>> @@ -12,6 +12,7 @@
>>>>  #include <linux/mfd/stm32-timers.h>
>>>>  #include <linux/module.h>
>>>>  #include <linux/of.h>
>>>> +#include <linux/pinctrl/consumer.h>
>>>>  #include <linux/platform_device.h>
>>>>  #include <linux/pwm.h>
>>>>  
>>>> @@ -19,6 +20,12 @@
>>>>  #define CCMR_CHANNEL_MASK  0xFF
>>>>  #define MAX_BREAKINPUT 2
>>>>  
>>>> +struct stm32_breakinput {
>>>> +	u32 index;
>>>> +	u32 level;
>>>> +	u32 filter;
>>>> +};
>>>> +
>>>>  struct stm32_pwm {
>>>>  	struct pwm_chip chip;
>>>>  	struct mutex lock; /* protect pwm config/enable */
>>>> @@ -26,15 +33,11 @@ struct stm32_pwm {
>>>>  	struct regmap *regmap;
>>>>  	u32 max_arr;
>>>>  	bool have_complementary_output;
>>>> +	struct stm32_breakinput breakinput[MAX_BREAKINPUT];
>>>> +	unsigned int nbreakinput;
>>>>  	u32 capture[4] ____cacheline_aligned; /* DMA'able buffer */
>>>>  };
>>>>  
>>>> -struct stm32_breakinput {
>>>> -	u32 index;
>>>> -	u32 level;
>>>> -	u32 filter;
>>>> -};
>>>> -
>>>>  static inline struct stm32_pwm *to_stm32_pwm_dev(struct pwm_chip *chip)
>>>>  {
>>>>  	return container_of(chip, struct stm32_pwm, chip);
>>>> @@ -512,15 +515,27 @@ static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
>>>>  	return (bdtr & bke) ? 0 : -EINVAL;
>>>>  }
>>>>  
>>>> -static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
>>>> +static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv)
>>>> +{
>>>> +	int i, ret = 0;
>>>> +
>>>> +	for (i = 0; i < priv->nbreakinput && !ret; i++) {
>>>> +		ret = stm32_pwm_set_breakinput(priv,
>>>> +					       priv->breakinput[i].index,
>>>> +					       priv->breakinput[i].level,
>>>> +					       priv->breakinput[i].filter);
>>>> +	}
>>>> +
>>>> +	return ret;
>>>> +}
>>>
>>> Can you explain what the effect of this function is? This is something
>>> that is lost during suspend?
>>
>> Yes, that's what I explain in the commit message: ...registers content
>> may be lost when going to low power mode.
>> Do you think I need to rephrase ?
> 
> Ah, right I missed it in the commit log. It might be worth adding that
> to a code comment. Also having the purpose of this function described
> would be great. Does it configure some electrical characteristics? Or
> has it to do with pinmuxing? Why is an input relevant for a PWM?

Hi Uwe,

I'll add a comment in the suspend routine to mention the need to restore
breakinput registers that may have been lost in low power.

Regarding the purpose of the break feature, maybe I can enhance comment
bellow (e.g. Because "st,breakinput" parameter is optional...) to
something like:

/*
 * Some timer instances can have BRK input pins (e.g. basically a fault
 * pin from the output power stage). The break feature allow a safe shut
 * down of the PWM outputs to a predefined state.
 * Because "st,breakinput" parameter is optional do not make probe
 * failed if it doesn't exist. Note the pinctrl handle must be inline
 * with "st,breakinput" property.
 */

FYI, the feature is described in Application note AN4277, "Using STM32
device PWM shut-down features..."

Would it answer your concern here? But I think this should be done in a
separate patch (not related to PM support).

> 
>>> I wonder why the patch is so big. There are some rearrangements that
>>> should have no effect and I think it would be beneficial for
>>> reviewability to split this patch in a patch that only does the
>>> restructuring and than on top of that add the PM stuff.
>>
>> I can split this to ease the review.
>>>
>>>> +
>>>> +static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
>>>>  				       struct device_node *np)
>>>>  {
>>>> -	struct stm32_breakinput breakinput[MAX_BREAKINPUT];
>>>> -	int nb, ret, i, array_size;
>>>> +	int nb, ret, array_size;
>>>>  
>>>>  	nb = of_property_count_elems_of_size(np, "st,breakinput",
>>>>  					     sizeof(struct stm32_breakinput));
>>>> -
>>>>  	/*
>>>>  	 * Because "st,breakinput" parameter is optional do not make probe
>>>>  	 * failed if it doesn't exist.
>>>> @@ -531,20 +546,14 @@ static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
>>>>  	if (nb > MAX_BREAKINPUT)
>>>>  		return -EINVAL;
>>>>  
>>>> +	priv->nbreakinput = nb;
>>>>  	array_size = nb * sizeof(struct stm32_breakinput) / sizeof(u32);
>>>>  	ret = of_property_read_u32_array(np, "st,breakinput",
>>>> -					 (u32 *)breakinput, array_size);
>>>> +					 (u32 *)priv->breakinput, array_size);
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> -	for (i = 0; i < nb && !ret; i++) {
>>>> -		ret = stm32_pwm_set_breakinput(priv,
>>>> -					       breakinput[i].index,
>>>> -					       breakinput[i].level,
>>>> -					       breakinput[i].filter);
>>>> -	}
>>>> -
>>>> -	return ret;
>>>> +	return stm32_pwm_apply_breakinputs(priv);
>>>>  }
>>>>  
>>>>  static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
>>>> @@ -614,7 +623,7 @@ static int stm32_pwm_probe(struct platform_device *pdev)
>>>>  	if (!priv->regmap || !priv->clk)
>>>>  		return -EINVAL;
>>>>  
>>>> -	ret = stm32_pwm_apply_breakinputs(priv, np);
>>>> +	ret = stm32_pwm_probe_breakinputs(priv, np);
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> @@ -647,6 +656,38 @@ static int stm32_pwm_remove(struct platform_device *pdev)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static int __maybe_unused stm32_pwm_suspend(struct device *dev)
>>>> +{
>>>> +	struct stm32_pwm *priv = dev_get_drvdata(dev);
>>>> +	struct pwm_state state;
>>>> +	unsigned int i;
>>>> +
>>>> +	for (i = 0; i < priv->chip.npwm; i++) {
>>>> +		pwm_get_state(&priv->chip.pwms[i], &state);
>>>
>>> pwm_get_state is a function designed to be used by PWM consumers. I
>>> would prefer to check the hardware registers here instead.
>>
>> It's also useful for PWM provider: This PWM driver is part of a MFD that
> 
> I don't doubt "useful". But still you should only call it if you called
> pwm_get (or a similar function) to get a PWM handle.
> 
>> also take care of IIO trigger (can be used simultaneously). Simply
>> reading a register doesn't tell us that the timer is used/configured as
>> a PWM here. That's the reason to use this helper to read pwm->state.
> 
> How can the pwm driver be bound and the hardware not be used a PWM?
> 
>> Do you wish I add a comment to clarify this here ?
> 
> No, I wish you inspect the hardware to determine what you need to know :-)

Ok, finally I found out the "active_channels()" routine does the job
(e.g. read CCER register), and is already used for that purpose (check
for active channels).
I'll use it in v2.

Thanks,
Fabrice

> 
>>> What if there is no consumer and the PWM just happens to be enabled by
>>> the bootloader? Or is this too minor an issue to be worth consideration?
>>
>> That's the purpose of returning -EBUSY: "PWM should not stop if the PWM
>> user didn't call pwm_disable()" ... "to avoid situation where the PWM is
>> actually suspended before the user". This has been enforced in later
>> series with the device_link_add(). See our previous discussions here:
>> https://lkml.org/lkml/2019/2/5/770
>> So, I guess this would point exactly a lack for a PWM user to manage it
>> after the boot stage, in the kernel.
>>
>> Could you please clarify, provide an example here ?
> 
> This is something different than I asked for. Not having a consumer
> isn't an error. Still the pwm might be running (for a good reason or
> not). (This is more a question that affects how a driver should behave
> in general, it is not specific to the stm32 driver here.)
> 
> Best regards
> Uwe
>