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