On 07/07/2017 11:23 AM, Thierry Reding wrote:
> On Fri, Jul 07, 2017 at 10:10:32AM +0200, Fabrice Gasnier wrote:
>> On 07/06/2017 09:43 AM, Thierry Reding wrote:
>>> On Wed, Jun 21, 2017 at 04:30:11PM +0200, Fabrice Gasnier wrote:
>>>> Add support for single PWM channel on Low-Power Timer, that can be
>>>> found on some STM32 platforms.
>>>>
>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxx>
>>>> ---
>>>> Changes in v2:
>>>> - s/Low Power/Low-Power
>>>> - update few comment lines
>>>> ---
>>>> drivers/pwm/Kconfig | 10 +++
>>>> drivers/pwm/Makefile | 1 +
>>>> drivers/pwm/pwm-stm32-lp.c | 216 +++++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 227 insertions(+)
>>>> create mode 100644 drivers/pwm/pwm-stm32-lp.c
>>>>
>>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>>> index 313c107..7cb982b 100644
>>>> --- a/drivers/pwm/Kconfig
>>>> +++ b/drivers/pwm/Kconfig
>>>> @@ -417,6 +417,16 @@ config PWM_STM32
>>>> To compile this driver as a module, choose M here: the module
>>>> will be called pwm-stm32.
>>>>
>>>> +config PWM_STM32_LP
>>>> + tristate "STMicroelectronics STM32 PWM LP"
>>>> + depends on MFD_STM32_LPTIMER || COMPILE_TEST
>>>> + help
>>>> + Generic PWM framework driver for STMicroelectronics STM32 SoCs
>>>> + with Low-Power Timer (LPTIM).
>>>> +
>>>> + To compile this driver as a module, choose M here: the module
>>>> + will be called pwm-stm32-lp.
>>>> +
>>>> config PWM_STMPE
>>>> bool "STMPE expander PWM export"
>>>> depends on MFD_STMPE
>>>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>>>> index 93da1f7..a3a4bee 100644
>>>> --- a/drivers/pwm/Makefile
>>>> +++ b/drivers/pwm/Makefile
>>>> @@ -40,6 +40,7 @@ obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
>>>> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
>>>> obj-$(CONFIG_PWM_STI) += pwm-sti.o
>>>> obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
>>>> +obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o
>>>> obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o
>>>> obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
>>>> obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
>>>> diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c
>>>> new file mode 100644
>>>> index 0000000..eb997a8
>>>> --- /dev/null
>>>> +++ b/drivers/pwm/pwm-stm32-lp.c
>>>> @@ -0,0 +1,216 @@
>>>> +/*
>>>> + * STM32 Low-Power Timer PWM driver
>>>> + *
>>>> + * Copyright (C) STMicroelectronics 2017
>>>> + *
>>>> + * Author: Gerald Baeza <gerald.baeza@xxxxxx>
>>>> + *
>>>> + * License terms: GNU General Public License (GPL), version 2
>>>> + *
>>>> + * Inspired by Gerald Baeza's pwm-stm32 driver
>>>> + */
>>>> +
>>>> +#include <linux/bitfield.h>
>>>> +#include <linux/mfd/stm32-lptimer.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/pwm.h>
>>>> +
>>>> +struct stm32_pwm_lp {
>>>> + struct pwm_chip chip;
>>>> + struct clk *clk;
>>>> + struct regmap *regmap;
>>>> +};
>>>> +
>>>> +static inline struct stm32_pwm_lp *to_stm32_pwm_lp(struct pwm_chip *chip)
>>>> +{
>>>> + return container_of(chip, struct stm32_pwm_lp, chip);
>>>> +}
>>>> +
>>>> +static const u8 prescalers[] = {1, 2, 4, 8, 16, 32, 64, 128};
>>>> +
>>>> +static int stm32_pwm_lp_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>>> + struct pwm_state *state)
>>>> +{
>>>> + struct stm32_pwm_lp *priv = to_stm32_pwm_lp(chip);
>>>> + unsigned long long prd, div, dty;
>>>> + struct pwm_state cstate;
>>>> + u32 val, mask, cfgr, wavpol, presc = 0;
>>>> + bool reenable = false;
>>>> + int ret;
>>>> +
>>>> + pwm_get_state(pwm, &cstate);
>>>> +
>>>> + if (!state->enabled) {
>>>> + if (cstate.enabled) {
>>>> + /* Disable LP timer */
>>>> + ret = regmap_write(priv->regmap, STM32_LPTIM_CR, 0);
>>>> + if (ret)
>>>> + return ret;
>>>> + clk_disable(priv->clk);
>>>> + }
>>>> + return 0;
>>>> + }
>>>> +
>>>> + /* Calculate the period and prescaler value */
>>>> + div = (unsigned long long)clk_get_rate(priv->clk) * state->period;
>>>> + do_div(div, NSEC_PER_SEC);
>>>> + prd = div;
>>>> + while (div > STM32_LPTIM_MAX_ARR) {
>>>> + presc++;
>>>> + if (presc >= ARRAY_SIZE(prescalers)) {
>>>> + dev_err(priv->chip.dev, "max prescaler exceeded\n");
>>>> + return -EINVAL;
>>>> + }
>>>> + div = prd;
>>>> + do_div(div, prescalers[presc]);
>>>> + }
>>>> + prd = div;
>>>> +
>>>> + /* Calculate the duty cycle */
>>>> + dty = prd * state->duty_cycle;
>>>> + do_div(dty, state->period);
>>>> +
>>>> + wavpol = FIELD_PREP(STM32_LPTIM_WAVPOL, state->polarity);
>>>> +
>>>> + if (!cstate.enabled) {
>>>> + ret = clk_enable(priv->clk);
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>
>>> Why do you need the checks here? Clock enabled are reference counted, so
>>> you could do the clk_enable() unconditionally.
>>
>> Hi Thierry,
>>
>> This clock is used to generate PWM (source for LP timer counter). I
>> enable it here as:
>> - required state is 'enabled'
>> - current state is 'disabled'.
>> PWM is being turned on: first enable clock, then configure & enable PWM
>> bellow.
>>
>> The opposite is done earlier, at the beginning of this routine:
>> - required state is 'disabled'
>> - current state is 'enabled'
>> PWM is turned off, then clock is disabled.
>>
>> Enable count should be balanced, and clock is enabled when required
>> (e.g. when PWM is 'on'). Doing it unconditionally here may cause
>> unbalanced enable count (e.g. any duty_cycle update would increase
>> enable count)
>> Is it ok to keep this ?
>
> The placement of the call suggested that you also need to enable the
> clock in order to access any of the registers. In such cases it's often
> simpler to take (and release) the reference to the clock irrespective
> of the current state.
>
> So the general sequence might look like this:
>
> /* allow access to registers */
> clk_enable();
>
> /* modify registers */
> ...
>
> /* enable clock to drive PWM counter */
> if (state->enabled && !cstate.enabled)
> clk_enable();
>
> /* disable clock to PWM counter */
> if (!state->enabled && cstate.enabled)
> clk_disable();
>
> /* access to registers no longer needed */
> clk_disable();
>
> This ensures that as long as you keep the "register" reference to the
> clock, the clock will remain on.
Hi Thierry,
I better see your point now. Regmap is already handling clock for
register access. I'll try to re-arrange things a little, so it's easier
to read and comment about enable/disable clock to PWM counter.
>
> There is a somewhat tricky situation that could happen if the initial
> clock reference count is not in sync. Consider the case where your
> ->get_state() determines that the PWM is enabled. cstate.enabled would
> be true, but the clock enable count would not be incremented. So you'd
> have to make sure to add code to the ->get_state() implementation that
> calls clk_enable() for each PWM that is enabled.
>
> In my opinion that would be the cleanest option and easiest to follow,
> but its more work to properly implement than I initially assumed, so
> I'm fine with the version that you have, too.
Thanks for these hints, I'll try to follow your advise on this:
- use get_state()
- call clk_enable() if PWM is already enabled.
>
>>> Speaking of which, I don't see a clk_prepare() anywhere. Doesn't the clk
>>> core warn about clk_enable() being called on a clock that's not been
>>> prepared?
>>
>> clk_get() and clk_prepare() happens in regmap layer, when probing mfd part:
>> -> stm32_lptimer_probe()
>> -> devm_regmap_init_mmio_clk()
>> -> __devm_regmap_init_mmio_clk()
>> -> regmap_mmio_gen_context()
>
> Okay, looks like we don't need it here, then.
>
>>>> + (presc != FIELD_GET(STM32_LPTIM_PRESC, cfgr))) {
>>>> + val = FIELD_PREP(STM32_LPTIM_PRESC, presc) | wavpol;
>>>> + mask = STM32_LPTIM_PRESC | STM32_LPTIM_WAVPOL;
>>>> +
>>>> + /* Must disable LP timer to modify CFGR */
>>>> + ret = regmap_write(priv->regmap, STM32_LPTIM_CR, 0);
>>>> + if (ret)
>>>> + goto err;
>>>> + reenable = true;
>>>
>>> The placement of this is somewhat odd. It suggests that it is somehow
>>> related to the disabling of the LP timer, whereas it really isn't.
>>
>> In case of prescaler or polarity change, CFGR register needs to be
>> updated. CFGR register must be modified only when LP timer HW is disabled.
>> - Initial choice is to use this flag, to temporarily disable HW, update
>> cfgr, then re-enable it. More thinking about this...
>
> What I find odd about the placement is that it is between the
> regmap_write() and the regmap_update_bits(). But it could just as well
> be after the regmap_update_bits() (or before regmap_write() for that
> matter). So the confusing thing is why it "breaks" the sequence of
> register accesses.
Ok, I'll move it before or after register accesses sequence.
>
>> - Another choice could be to refuse such a 'live' change and report
>> (busy?) error ? Then user would have to explicitly disable it, configure
>> new setting and re-enable it.
>>
>> Please let me know your opinion.
>
> The PWM subsystem doesn't give a guarantee that a live change is
> possible. Drivers always have to assume that the PWM may get disabled
> and reenabled as part of the sequence.
>
> That said, something like this could be added in the future if users
> come along that required this guarantee.
>
> For your driver, I think it's fine to keep this as-is.
Got it.
>
>>>> +static int stm32_pwm_lp_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct stm32_lptimer *ddata = dev_get_drvdata(pdev->dev.parent);
>>>> + struct stm32_pwm_lp *priv;
>>>> + int ret;
>>>> +
>>>> + if (IS_ERR_OR_NULL(ddata))
>>>> + return -EINVAL;
>>>
>>> It seems to me like this can never happen. How would you trigger this
>>> condition?
>>
>> Bad dt configuration can trigger this error: thinking of a
>> 'st,stm32-pwm-lp' dt node without proper mfd parent. Do you want me to
>> drop this ?
>> (or add comment about it ?)
>
> In my opinion we should trust DTB in this type of situation. If the DT
> binding says that the PWM node needs to be a child of an MFD, then the
> author of the DTB needs to make sure it is.
>
> For things that can be easily checked I think it makes sense to validate
> the DTB, but this check here is not enough for all situations, right?
> What if somebody added the device node as child to some unrelated node.
> ddata could be a valid pointer, but pointing at something that's not a
> struct stm32_lptimer at all. That's still pretty bad, and completely
> undetectable.
You're right. I'll remove this as well.
Thanks again,
Best Regards,
Fabrice
>
> Thierry
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
