[PATCH v2 4/8] pwm: Add STM32 LPTimer PWM driver
Thierry Reding
thierry.reding at gmail.com
Fri Jul 7 02:23:16 PDT 2017
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 at st.com>
> >> ---
> >> 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 at st.com>
> >> + *
> >> + * 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.
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.
> > 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.
> - 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.
> >> +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.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170707/daef5b8a/attachment.sig>
More information about the linux-arm-kernel
mailing list