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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 14 Feb 2019 13:25:27 +0530
From:   Yash Shah <yash.shah@...ive.com>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Cc:     Palmer Dabbelt <palmer@...ive.com>, linux-pwm@...r.kernel.org,
        linux-riscv@...ts.infradead.org,
        Thierry Reding <thierry.reding@...il.com>, robh+dt@...nel.org,
        mark.rutland@....com, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Sachin Ghadi <sachin.ghadi@...ive.com>,
        Paul Walmsley <paul.walmsley@...ive.com>
Subject: Re: [PATCH v6 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

On Wed, Feb 13, 2019 at 4:05 PM Uwe Kleine-König
<u.kleine-koenig@...gutronix.de> wrote:
>
> Hello,
>
> On Wed, Feb 13, 2019 at 02:56:18PM +0530, Yash Shah wrote:
> > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> >
> > Signed-off-by: Wesley W. Terpstra <wesley@...ive.com>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <atish.patra@....com>
> > Signed-off-by: Yash Shah <yash.shah@...ive.com>
> > ---
> >  drivers/pwm/Kconfig      |  11 ++
> >  drivers/pwm/Makefile     |   1 +
> >  drivers/pwm/pwm-sifive.c | 311 +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 323 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-sifive.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index a8f47df..4a61d1a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -380,6 +380,17 @@ config PWM_SAMSUNG
> >         To compile this driver as a module, choose M here: the module
> >         will be called pwm-samsung.
> >
> > +config PWM_SIFIVE
> > +     tristate "SiFive PWM support"
> > +     depends on OF
> > +     depends on COMMON_CLK
> > +     depends on RISCV || COMPILE_TEST
> > +     help
> > +       Generic PWM framework driver for SiFive SoCs.
> > +
> > +       To compile this driver as a module, choose M here: the module
> > +       will be called pwm-sifive.
> > +
> >  config PWM_SPEAR
> >       tristate "STMicroelectronics SPEAr PWM support"
> >       depends on PLAT_SPEAR
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 9c676a0..30089ca 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR)              += pwm-rcar.o
> >  obj-$(CONFIG_PWM_RENESAS_TPU)        += pwm-renesas-tpu.o
> >  obj-$(CONFIG_PWM_ROCKCHIP)   += pwm-rockchip.o
> >  obj-$(CONFIG_PWM_SAMSUNG)    += pwm-samsung.o
> > +obj-$(CONFIG_PWM_SIFIVE)     += pwm-sifive.o
> >  obj-$(CONFIG_PWM_SPEAR)              += pwm-spear.o
> >  obj-$(CONFIG_PWM_STI)                += pwm-sti.o
> >  obj-$(CONFIG_PWM_STM32)              += pwm-stm32.o
> > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > new file mode 100644
> > index 0000000..c0eb90e
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,311 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2017-2018 SiFive
> > + * For SiFive's PWM IP block documentation please refer Chapter 14 of
> > + * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/slab.h>
> > +
> > +/* Register offsets */
> > +#define PWM_SIFIVE_PWMCFG            0x0
> > +#define PWM_SIFIVE_PWMCOUNT          0x8
> > +#define PWM_SIFIVE_PWMS                      0x10
> > +#define PWM_SIFIVE_PWMCMP0           0x20
> > +
> > +/* PWMCFG fields */
> > +#define PWM_SIFIVE_PWMCFG_SCALE              0
> > +#define PWM_SIFIVE_PWMCFG_STICKY     8
> > +#define PWM_SIFIVE_PWMCFG_ZERO_CMP   9
> > +#define PWM_SIFIVE_PWMCFG_DEGLITCH   10
> > +#define PWM_SIFIVE_PWMCFG_EN_ALWAYS  12
>
> PWM_SIFIVE_PWMCFG_EN_ALWAYS is always used as
>
>         BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS)
>
> so defining this as BIT(12) directly makes some expressions below easier
> to read.

Sure, will do that.

>
> > +#define PWM_SIFIVE_PWMCFG_EN_ONCE    13
> > +#define PWM_SIFIVE_PWMCFG_CENTER     16
> > +#define PWM_SIFIVE_PWMCFG_GANG               24
> > +#define PWM_SIFIVE_PWMCFG_IP         28
> > +
> > +/* PWM_SIFIVE_SIZE_PWMCMP is used to calculate offset for pwmcmpX registers */
> > +#define PWM_SIFIVE_SIZE_PWMCMP               4
> > +#define PWM_SIFIVE_CMPWIDTH          16
> > +
> > +struct pwm_sifive_ddata {
> > +     struct pwm_chip chip;
> > +     struct notifier_block notifier;
> > +     struct clk *clk;
> > +     void __iomem *regs;
> > +     unsigned int real_period;
> > +     int user_count;
> > +};
> > +
> > +static inline
> > +struct pwm_sifive_ddata *pwm_sifive_chip_to_ddata(struct pwm_chip *c)
> > +{
> > +     return container_of(c, struct pwm_sifive_ddata, chip);
> > +}
> > +
> > +static int pwm_sifive_request(struct pwm_chip *chip, struct pwm_device *dev)
> > +{
> > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +
> > +     pwm->user_count++;
> > +
> > +     return 0;
> > +}
> > +
> > +static void pwm_sifive_free(struct pwm_chip *chip, struct pwm_device *dev)
> > +{
> > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +
> > +     pwm->user_count--;
> > +}
> > +
> > +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> > +                                 unsigned long rate)
> > +{
> > +     /* (1 << (16+scale)) * 10^9/rate = real_period */
>
> Maybe you want to mention here the relation between 16 and
> PWM_SIFIVE_CMPWIDTH.

Sure, will change it to
/* (1 << (PWM_SIFIVE_CMPWIDTH+scale)) * 10^9/rate = real_period */

>
> > +     unsigned long scale_pow =
> > +                     div64_ul(pwm->real_period * (u64)rate, NSEC_PER_SEC);
> > +     int scale = clamp(ilog2(scale_pow) - PWM_SIFIVE_CMPWIDTH, 0, 0xf);
> > +
> > +     writel((1 << PWM_SIFIVE_PWMCFG_EN_ALWAYS) | (scale <<
> > +            PWM_SIFIVE_PWMCFG_SCALE), pwm->regs + PWM_SIFIVE_PWMCFG);
>
> I think this would be better readable with the newline after the |. With
> my editor's configuration when broken like this, the 2nd line would be
> intended with the opening ( after the |.
>
> > +
> > +     /* As scale <= 15 the shift operation cannot overflow. */
> > +     pwm->real_period = div64_ul(1000000000ULL << (PWM_SIFIVE_CMPWIDTH +
> > +                                 scale), rate);
>
> ditto. Maybe break after the =?
>
> > +     dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
> > +}
> > +
> > +static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> > +                              struct pwm_state *state)
> > +{
> > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +     u32 duty;
> > +     int val;
> > +
> > +     duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm *
> > +                  PWM_SIFIVE_SIZE_PWMCMP);
> > +
> > +     val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > +     state->enabled = (val & BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS)) > 0;
> > +
> > +     val &= 0x0F;
> > +     pwm->real_period = div64_ul(1000000000ULL << (PWM_SIFIVE_CMPWIDTH +
> > +                                 val), clk_get_rate(pwm->clk));
>
> Another bad line break.

Will add temp variable to split the above expressions.

>
> > +
> > +     state->period = pwm->real_period;
> > +     state->duty_cycle = ((u64)duty * pwm->real_period) >>
> > +                         PWM_SIFIVE_CMPWIDTH;
> > +     state->polarity = PWM_POLARITY_INVERSED;
> > +}
> > +
> > +static int pwm_sifive_enable(struct pwm_chip *chip, struct pwm_device *dev,
> > +                          bool enable)
> > +{
> > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +     u32 val;
> > +     int ret;
> > +
> > +     if (enable) {
> > +             ret = clk_enable(pwm->clk);
> > +             if (ret) {
> > +                     dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > +     if (enable)
> > +             val |= BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS);
> > +     else
> > +             val &= ~BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS);
> > +
> > +     writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > +     if (!enable)
> > +             clk_disable(pwm->clk);
>
> A disabled PWM is supposed to output an inactive signal. If the PWM runs
> at (near) 100% and you disable it, does it reliably give that inactive
> signal after completing the currently running period?

Yes, you are right, it just freezes at that state (100%).
What if I set duty cycle = 0 if (!state->enabled) before disabling the PWM?

>
> > +
> > +     return 0;
> > +}
> > +
> > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> > +                         struct pwm_state *state)
> > +{
> > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +     unsigned int duty_cycle;
> > +     u32 frac, val;
> > +     struct pwm_state cur_state;
> > +     bool enabled;
> > +     int ret;
> > +
> > +     pwm_get_state(dev, &cur_state);
> > +     enabled = cur_state.enabled;
> > +
> > +     if (state->polarity != PWM_POLARITY_INVERSED)
> > +             return -EINVAL;
> > +
> > +     if (state->period != cur_state.period) {
> > +             if (pwm->user_count != 1)
> > +                     return -EINVAL;
>
> I think we need locking here. Consider two pwm users on two CPUs:
>
>         CPU1                                    CPU2
>         pwm_sifive_apply(pwm0, period=A, ...)
>          check user_count==1 -> good
>          ...                                    pwm1 = pwm_get(...)
>          ...                                    pwm_sifive_apply(pwm1, period=B...)
>          ...                                      configure based on B
>          pwm_sifive_update_clock()

mutex_lock();
  if (pwm->user_count != 1)
                  return -EINVAL;
mutex_unlock();
Something like this?

>
> Also I wonder if we should change the period if the user requested
> enabled=false.

You want me to NOT update period if enabled=false, right?

>
> > +             pwm->real_period = state->period;
> > +             pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
>
> If you change from
>
>         .period = A
>         .duty_cycle = B
>
> to
>
>         .period = C
>         .duty_cycle = D
>
> the output pin might see a period with
>
>         .period = C
>         .duty_cycle = B
>
> right? I think this is not fixable, but this needs a prominent comment.

Good point. Is the below comment good enough?
/* When changing both duty cycle and period, the old duty cycle might
be active with new the period settings for a period */

>
> > +     }
> > +
> > +     if (!state->enabled && enabled) {
> > +             ret = pwm_sifive_enable(chip, dev, false);
> > +             if (ret)
> > +                     return ret;
> > +             enabled = false;
> > +     }
> > +
> > +     duty_cycle = state->duty_cycle;
> > +     frac = div_u64((u64)duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) +
> > +                    (1 << PWM_SIFIVE_CMPWIDTH) / 2, state->period);
> > +     /* The hardware cannot generate a 100% duty cycle */
>
> @Thierry: Do you consider this bad enough that pwm_apply_state should
> fail if 100% is requested?
>
> > +     frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > +
> > +     val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > +     val |= BIT(PWM_SIFIVE_PWMCFG_DEGLITCH);
> > +     writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > +     writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm *
> > +            PWM_SIFIVE_SIZE_PWMCMP);
> > +
> > +     val &= ~BIT(PWM_SIFIVE_PWMCFG_DEGLITCH);
>
> Doesn't that come too early? I thought the right thing was to keep it
> set all the time. With this code I think you might see a duty cycle of
> 50 when going from 60 to 40.

We cannot set it all the time.
Setting it all the time makes every alternate period to remain high
(latched state).
As per the manual, it needs to be set when reprogramming and must be
cleared afterwards.

>
> > +     writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > +     if (state->enabled != enabled) {
> > +             ret = pwm_sifive_enable(chip, dev, state->enabled);
> > +             if (ret)
> > +                     return ret;
> > +     }
>
> FTR: This doesn't block until the newly configured state is active.
>
> > +
> > +     return 0;
> > +}
>
> Best regards
> Uwe

Thanks for your comments

>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Powered by blists - more mailing lists