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

From: Fabrice Gasnier
Date: Tue Oct 01 2019 - 04:18:55 EST

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@xxxxxx>
>> ---
>> 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
>