Re: [PATCH v7 4/8] PWM: add PWM driver for STM32 plaftorm

[Benjamin Gaignard]

2017-01-18 11:08 GMT+01:00 Thierry Reding <thierry.reding@xxxxxxxxx>:
> On Thu, Jan 05, 2017 at 10:25:40AM +0100, Benjamin Gaignard wrote:
>> This driver adds support for PWM driver on STM32 platform.
>> The SoC have multiple instances of the hardware IP and each
>> of them could have small differences: number of channels,
>> complementary output, auto reload register size...
>>
>> version 6:
>> - change st,breakinput parameter to make it usuable for stm32f7 too.
>>
>> version 4:
>> - detect at probe time hardware capabilities
>> - fix comments done on v2 and v3
>> - use PWM atomic ops
>>
>> version 2:
>> - only keep one comptatible
>> - use DT parameters to discover hardware block configuration
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxx>
>> ---
>>  drivers/pwm/Kconfig     |   9 +
>>  drivers/pwm/Makefile    |   1 +
>>  drivers/pwm/pwm-stm32.c | 434 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 444 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-stm32.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index f92dd41..88035c0 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -397,6 +397,15 @@ config PWM_STI
>>         To compile this driver as a module, choose M here: the module
>>         will be called pwm-sti.
>>
>> +config PWM_STM32
>> +     tristate "STMicroelectronics STM32 PWM"
>> +     depends on (ARCH_STM32 && OF && MFD_STM32_TIMERS) || COMPILE_TEST
>> +     help
>> +       Generic PWM framework driver for STM32 SoCs.
>> +
>> +       To compile this driver as a module, choose M here: the module
>> +       will be called pwm-stm32.
>> +
>>  config PWM_STMPE
>>       bool "STMPE expander PWM export"
>>       depends on MFD_STMPE
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index a48bdb5..346a83b 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -38,6 +38,7 @@ obj-$(CONFIG_PWM_ROCKCHIP)  += pwm-rockchip.o
>>  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_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.c b/drivers/pwm/pwm-stm32.c
>> new file mode 100644
>> index 0000000..fcf0a78
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-stm32.c
>> @@ -0,0 +1,434 @@
>> +/*
>> + * Copyright (C) STMicroelectronics 2016
>> + *
>> + * Author: Gerald Baeza <gerald.baeza@xxxxxx>
>> + *
>> + * License terms: GNU General Public License (GPL), version 2
>> + *
>> + * Inspired by timer-stm32.c from Maxime Coquelin
>> + *             pwm-atmel.c from Bo Shen
>> + */
>> +
>> +#include <linux/mfd/stm32-timers.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/of.h>
>
> Can you please sort these alphabetically?

sure

>
>> +
>> +#define CCMR_CHANNEL_SHIFT 8
>> +#define CCMR_CHANNEL_MASK  0xFF
>> +#define MAX_BREAKINPUT 2
>
> Okay, this answers my question regarding the st,breakinput property. I
> still think it'd be good to have this in the binding documentation just
> to avoid having to look at implementation to find out.
>
>> +
>> +struct stm32_pwm {
>> +     struct pwm_chip chip;
>> +     struct device *dev;
>> +     struct clk *clk;
>> +     struct regmap *regmap;
>> +     unsigned int caps;
>
> This seems completely unused?

Yes I will remove it

>
>> +     unsigned int npwm;
>
> It's somewhat redundant to have this here, since the same information is
> already contained in struct pwm_chip.npwm.
>
> Since you use this primarily for detection, how about you make the
> stm32_pwm_detect_channels() function return the value and store it in a
> local variable in ->probe()? That might be useful also because you
> need to check the return value of regmap_update_bits() which technically
> could fail.
>

I will remove npwm field and put the result of stm32_pwm_detect_channels()
directly on chip.npwm.

regmap functions could failed (even if I haven't experiment that case)
but testing all
return make the code unreadable so I have decide to not test it....

>> +     u32 max_arr;
>> +     bool have_complementary_output;
>> +};
>> +
>> +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);
>> +}
>> +
>> +static u32 active_channels(struct stm32_pwm *dev)
>> +{
>> +     u32 ccer;
>> +
>> +     regmap_read(dev->regmap, TIM_CCER, &ccer);
>> +
>> +     return ccer & TIM_CCER_CCXE;
>> +}
>
> This looks like something that you could track in software, but this is
> probably fine, too. Again, technically regmap_read() could fail, so you
> might want to consider adding some code to handle it. In practice it
> probably won't, so maybe you don't.

TIM_CCER_CCXE is a value that IIO timer can also read (not write) so
I have keep the same logic for pwm driver.

>
>> +
>> +static int write_ccrx(struct stm32_pwm *dev, struct pwm_device *pwm,
>> +                   u32 value)
>> +{
>> +     switch (pwm->hwpwm) {
>> +     case 0:
>> +             return regmap_write(dev->regmap, TIM_CCR1, value);
>> +     case 1:
>> +             return regmap_write(dev->regmap, TIM_CCR2, value);
>> +     case 2:
>> +             return regmap_write(dev->regmap, TIM_CCR3, value);
>> +     case 3:
>> +             return regmap_write(dev->regmap, TIM_CCR4, value);
>> +     }
>> +     return -EINVAL;
>> +}
>> +
>> +static int stm32_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> +                         int duty_ns, int period_ns)
>> +{
>> +     struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
>> +     unsigned long long prd, div, dty;
>> +     unsigned int prescaler = 0;
>> +     u32 ccmr, mask, shift;
>> +
>> +     /* Period and prescaler values depends on clock rate */
>> +     div = (unsigned long long)clk_get_rate(priv->clk) * period_ns;
>> +
>> +     do_div(div, NSEC_PER_SEC);
>> +     prd = div;
>> +
>> +     while (div > priv->max_arr) {
>> +             prescaler++;
>> +             div = prd;
>> +             do_div(div, (prescaler + 1));
>
> Nit: there's no need for the parentheses here.

okay

>> +     }
>> +
>> +     prd = div;
>> +
>> +     if (prescaler > MAX_TIM_PSC) {
>> +             dev_err(chip->dev, "prescaler exceeds the maximum value\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     /*
>> +      * All channels share the same prescaler and counter so when two
>> +      * channels are active at the same we can't change them
>
> Nit: "at the same time"?

okay

>
>> +      */
>> +     if (active_channels(priv) & ~(1 << pwm->hwpwm * 4)) {
>> +             u32 psc, arr;
>> +
>> +             regmap_read(priv->regmap, TIM_PSC, &psc);
>> +             regmap_read(priv->regmap, TIM_ARR, &arr);
>> +
>> +             if ((psc != prescaler) || (arr != prd - 1))
>> +                     return -EBUSY;
>> +     }
>> +
>> +     regmap_write(priv->regmap, TIM_PSC, prescaler);
>> +     regmap_write(priv->regmap, TIM_ARR, prd - 1);
>> +     regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, TIM_CR1_ARPE);
>> +
>> +     /* Calculate the duty cycles */
>> +     dty = prd * duty_ns;
>> +     do_div(dty, period_ns);
>> +
>> +     write_ccrx(priv, pwm, dty);
>> +
>> +     /* Configure output mode */
>> +     shift = (pwm->hwpwm & 0x1) * CCMR_CHANNEL_SHIFT;
>> +     ccmr = (TIM_CCMR_PE | TIM_CCMR_M1) << shift;
>> +     mask = CCMR_CHANNEL_MASK << shift;
>> +
>> +     if (pwm->hwpwm < 2)
>> +             regmap_update_bits(priv->regmap, TIM_CCMR1, mask, ccmr);
>> +     else
>> +             regmap_update_bits(priv->regmap, TIM_CCMR2, mask, ccmr);
>> +
>> +     regmap_update_bits(priv->regmap, TIM_BDTR,
>> +                        TIM_BDTR_MOE | TIM_BDTR_AOE,
>> +                        TIM_BDTR_MOE | TIM_BDTR_AOE);
>> +
>> +     return 0;
>> +}
>> +
>> +static int stm32_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>> +                               enum pwm_polarity polarity)
>> +{
>> +     u32 mask;
>> +     struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
>> +
>> +     mask = TIM_CCER_CC1P << (pwm->hwpwm * 4);
>> +     if (priv->have_complementary_output)
>> +             mask |= TIM_CCER_CC1NP << (pwm->hwpwm * 4);
>> +
>> +     regmap_update_bits(priv->regmap, TIM_CCER, mask,
>> +                        polarity == PWM_POLARITY_NORMAL ? 0 : mask);
>> +
>> +     return 0;
>> +}
>> +
>> +static int stm32_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +     u32 mask;
>> +     struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
>> +
>> +     clk_enable(priv->clk);
>
> This can fail, so its return value should be checked. Also, I don't see
> a clk_prepare() anywhere. Is that something that maybe the MFD driver
> should be doing? It currently isn't.

I will check the return value.
You are right clk_prepare() is done in mfd driver when calling
devm_regmap_init_mmio_clk()

>
>> +
>> +     /* Enable channel */
>> +     mask = TIM_CCER_CC1E << (pwm->hwpwm * 4);
>> +     if (priv->have_complementary_output)
>> +             mask |= TIM_CCER_CC1NE << (pwm->hwpwm * 4);
>> +
>> +     regmap_update_bits(priv->regmap, TIM_CCER, mask, mask);
>> +
>> +     /* Make sure that registers are updated */
>> +     regmap_update_bits(priv->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
>> +
>> +     /* Enable controller */
>> +     regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
>> +
>> +     return 0;
>> +}
>> +
>> +static void stm32_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +     u32 mask;
>> +     struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
>> +
>> +     /* Disable channel */
>> +     mask = TIM_CCER_CC1E << (pwm->hwpwm * 4);
>> +     if (priv->have_complementary_output)
>> +             mask |= TIM_CCER_CC1NE << (pwm->hwpwm * 4);
>> +
>> +     regmap_update_bits(priv->regmap, TIM_CCER, mask, 0);
>> +
>> +     /* When all channels are disabled, we can disable the controller */
>> +     if (!active_channels(priv))
>> +             regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
>> +
>> +     clk_disable(priv->clk);
>> +}
>> +
>> +static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> +                        struct pwm_state *state)
>> +{
>> +     struct pwm_state curstate;
>> +     bool enabled;
>> +     int ret;
>> +
>> +     pwm_get_state(pwm, &curstate);
>> +     enabled = curstate.enabled;
>
> There should be no need to do this in drivers. pwm_get_state() is for
> PWM API users. Drivers can directly dereference pwm->state.

ok

>
>> +
>> +     if (enabled && !state->enabled) {
>> +             stm32_pwm_disable(chip, pwm);
>> +             return 0;
>> +     }
>> +
>> +     if (state->polarity != curstate.polarity && enabled)
>> +             stm32_pwm_set_polarity(chip, pwm, state->polarity);
>
> So that's kind of a violation of atomic API semantics. The above means
> that if you have a PWM in the following state:
>
>         enabled: no
>         polarity: normal
>
> and want to set this:
>
>         enabled: yes
>         polarity: inversed
>
> then you will ignore the new polarity setting. What's the reason for
> "&& enabled) in the conditional above?

There is no reason, I will remove it.

>> +
>> +     ret = stm32_pwm_config(chip, pwm, state->duty_cycle, state->period);
>> +     if (ret)
>> +             return ret;
>> +
>> +     if (!enabled && state->enabled)
>> +             ret = stm32_pwm_enable(chip, pwm);
>> +
>> +     return ret;
>> +}
>
> Would it be possible to merge stm32_pwm_disable(), stm32_pwm_enable(),
> stm32_pwm_set_polarity() and stm32_pwm_config() into stm32_pwm_apply()?
> Part of the reason for the atomic API was to make it easier to write
> these drivers, but your implementation effectively copies what the
> transitional helpers do.
>
> It might not make a difference technically in your case, but I think
> it'd make the implementation more compact and set a better example for
> future reference.

hmm... it will create a fat function with lot of where
enabling/disabling/configuration
will be mixed I'm really not convince that will more compact and readable.

>
>> +
>> +static const struct pwm_ops stm32pwm_ops = {
>> +     .owner = THIS_MODULE,
>> +     .apply = stm32_pwm_apply,
>> +};
>> +
>> +static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
>> +                                 int level, int filter)
>> +{
>> +     u32 bdtr = TIM_BDTR_BKE;
>> +
>> +     if (level)
>> +             bdtr |= TIM_BDTR_BKP;
>> +
>> +     bdtr |= (filter & TIM_BDTR_BKF_MASK) << TIM_BDTR_BKF_SHIFT;
>> +
>> +     regmap_update_bits(priv->regmap,
>> +                        TIM_BDTR, TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF,
>> +                        bdtr);
>> +
>> +     regmap_read(priv->regmap, TIM_BDTR, &bdtr);
>> +
>> +     return (bdtr & TIM_BDTR_BKE) ? 0 : -EINVAL;
>> +}
>> +
>> +static int stm32_pwm_set_breakinput2(struct stm32_pwm *priv,
>> +                                  int level, int filter)
>> +{
>> +     u32 bdtr = TIM_BDTR_BK2E;
>> +
>> +     if (level)
>> +             bdtr |= TIM_BDTR_BK2P;
>> +
>> +     bdtr |= (filter & TIM_BDTR_BKF_MASK) << TIM_BDTR_BK2F_SHIFT;
>> +
>> +     regmap_update_bits(priv->regmap,
>> +                        TIM_BDTR, TIM_BDTR_BK2E |
>> +                        TIM_BDTR_BK2P |
>> +                        TIM_BDTR_BK2F,
>> +                        bdtr);
>> +
>> +     regmap_read(priv->regmap, TIM_BDTR, &bdtr);
>> +
>> +     return (bdtr & TIM_BDTR_BK2E) ? 0 : -EINVAL;
>> +}
>
> As far as I can tell the only difference here is the various bit
> positions. Can you collapse the above two functions and add a new
> parameter to unify some code?

Yes it is all about bit shifting, I had try unify those two functions
with index has additional parameter
but it just add if() before each lines so no real benefit for code size.

>
>> +
>> +static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
>> +                                    struct device_node *np)
>> +{
>> +     struct stm32_breakinput breakinput[MAX_BREAKINPUT];
>> +     int nb, ret, i, 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.
>> +      */
>> +     if (nb <= 0)
>> +             return 0;
>> +
>> +     if (nb > MAX_BREAKINPUT)
>> +             return -EINVAL;
>> +
>> +     array_size = nb * sizeof(struct stm32_breakinput) / sizeof(u32);
>> +     ret = of_property_read_u32_array(np, "st,breakinput",
>> +                                      &breakinput[0].index, array_size);
>
> Maybe (u32 *)breakinput? That would make it more resilient against
> changes in ordering of fields in the struct. Granted, that's not likely
> to change, but I think it's a good idea in general to write code in a
> way that's safe in a more general case. That way if somebody ever were
> to copy from your code and then decide to reorder fields in their code
> things wouldn't fall apart.

Yes it is not suppose to change but I will use (u32 *)breakinput.

>> +     if (ret)
>> +             return ret;
>> +
>> +     for (i = 0; i < nb && !ret; i++) {
>> +             switch (breakinput[i].index) {
>> +             case 0:
>> +             {
>> +                     ret = stm32_pwm_set_breakinput(priv,
>> +                                                    breakinput[i].level,
>> +                                                    breakinput[i].filter);
>> +                     break;
>> +             }
>
> Curly braces are unnecessary here.

removed

>
>> +             case 1:
>> +             {
>> +                     ret = stm32_pwm_set_breakinput2(priv,
>> +                                                     breakinput[i].level,
>> +                                                     breakinput[i].filter);
>> +
>> +                     break;
>> +             }
>> +             default:
>> +             {
>> +                     ret = -EINVAL;
>> +                     break;
>> +             }
>> +             }
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
>> +{
>> +     u32 ccer;
>> +
>> +     /*
>> +      * If complementary bit doesn't exist writing 1 will have no
>> +      * effect so we can detect it.
>> +      */
>> +     regmap_update_bits(priv->regmap,
>> +                        TIM_CCER, TIM_CCER_CC1NE, TIM_CCER_CC1NE);
>> +     regmap_read(priv->regmap, TIM_CCER, &ccer);
>> +     regmap_update_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE, 0);
>
> This is strange: why are we disabling outputs here? Shouldn't the last
> line here undo the first instead?

Yes it should TIM_CCER_CC1NE not TIM_CCER_CCXE, I will fix it, thanks

>
>> +
>> +     priv->have_complementary_output = (ccer != 0);
>> +}
>> +
>> +static void stm32_pwm_detect_channels(struct stm32_pwm *priv)
>> +{
>> +     u32 ccer;
>> +
>> +     /*
>> +      * If channels enable bits don't exist writing 1 will have no
>> +      * effect so we can detect and count them.
>> +      */
>> +     regmap_update_bits(priv->regmap,
>> +                        TIM_CCER, TIM_CCER_CCXE, TIM_CCER_CCXE);
>> +     regmap_read(priv->regmap, TIM_CCER, &ccer);
>> +     regmap_update_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE, 0);
>
> Does this have the potential to glitch? I suspect that the clock may not
> be on at this point and therefore no PWM outputs will be generated, but
> is that guaranteed to always be the case?

Set TIM_CCER_CCXE isn't enough to enable PWM generation, TIM_CR1_CEN
in TIM_CR1 register must also to set so no risk of glitch here

>
> Thierry



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org â Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog