| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 1 Oct 2019 10:18:31 +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 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@...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
>
Powered by blists - more mailing lists