Message-ID: <f0fd1926-9fca-99c5-4a54-99252b865bc6@st.com>
Date:   Tue, 1 Oct 2019 15:48:52 +0200
From:   Fabrice Gasnier <fabrice.gasnier@...com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
CC:     <thierry.reding@...il.com>, <robh+dt@...nel.org>,
        <alexandre.torgue@...com>, <mark.rutland@....com>,
        <mcoquelin.stm32@...il.com>, <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-pwm@...r.kernel.org>,
        <benjamin.gaignard@...com>,
        <linux-stm32@...md-mailman.stormreply.com>
Subject: Re: [PATCH 2/2] pwm: stm32: add power management support

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

Powered by blists - more mailing lists