[PATCH] pwm: Add PWM driver for SiFive SoC

[Yash Shah]

> -----Original Message-----
> From: Heiko Schocher <hs at denx.de>
> Sent: 15 April 2020 22:34
> To: Yash Shah <yash.shah at sifive.com>
> Cc: martyn.welch at collabora.co.uk; u-boot at lists.denx.de
> Subject: Re: [PATCH] pwm: Add PWM driver for SiFive SoC
> 
> [External Email] Do not click links or attachments unless you recognize the
> sender and know the content is safe
> 
> Hello Yash Shah,
> 
> Am 15.04.2020 um 08:32 schrieb Yash Shah:
> > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed
> > SoC This driver is simple port of Linux pwm sifive driver.
> 
> So please add more information from exatly which linux version this patch is
> from, see:
> 
> https://www.denx.de/wiki/U-
> Boot/Patches#Attributing_Code_Copyrights_Sign

Sure, will refer this doc and send a v2 accordingly.

> 
> > Signed-off-by: Yash Shah <yash.shah at sifive.com>
> > ---
> >   drivers/pwm/Kconfig      |   6 ++
> >   drivers/pwm/Makefile     |   1 +
> >   drivers/pwm/pwm-sifive.c | 181
> +++++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 188 insertions(+)
> >   create mode 100644 drivers/pwm/pwm-sifive.c
> 
> You introduce a new devicetree node, please add a documentation in
> doc/device-tree-bindings/pwm/

Will add the dt documentation.

> 
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > 1f36fc7..d6ea9ee 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -40,6 +40,12 @@ config PWM_SANDBOX
> >         useful. The PWM can be enabled but is not connected to any outputs
> >         so this is not very useful.
> >
> > +config PWM_SIFIVE
> > +     bool "Enable support for SiFive PWM"
> > +     depends on DM_PWM
> > +     help
> > +       This PWM is found SiFive's FU540 and other SoCs.
> > +
> >   config PWM_TEGRA
> >       bool "Enable support for the Tegra PWM"
> >       depends on DM_PWM
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > a837c35..e2d4185 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -12,6 +12,7 @@ obj-$(CONFIG_DM_PWM)                += pwm-uclass.o
> >
> >   obj-$(CONFIG_PWM_EXYNOS)    += exynos_pwm.o
> >   obj-$(CONFIG_PWM_IMX)               += pwm-imx.o pwm-imx-util.o
> > +obj-$(CONFIG_PWM_SIFIVE)     += pwm-sifive.o
> 
> Please keep this list sorted.

Sure. My bad.

> 
> >   obj-$(CONFIG_PWM_ROCKCHIP)  += rk_pwm.o
> >   obj-$(CONFIG_PWM_SANDBOX)   += sandbox_pwm.o
> >   obj-$(CONFIG_PWM_TEGRA)             += tegra_pwm.o
> > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c new
> > file mode 100644 index 0000000..c54297e
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,181 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2020 SiFive, Inc
> > + * For SiFive's PWM IP block documentation please refer Chapter 14 of
> > + * Reference Manual :
> > +https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> > + *
> > + * Limitations:
> > + * - When changing both duty cycle and period, we cannot prevent in
> > + *   software that the output might produce a period with mixed
> > + *   settings (new period length and old duty cycle).
> > + * - The hardware cannot generate a 100% duty cycle.
> > + * - The hardware generates only inverted output.
> > + */
> > +
> > +#include <common.h>
> > +#include <clk.h>
> > +#include <div64.h>
> > +#include <dm.h>
> > +#include <pwm.h>
> > +#include <regmap.h>
> > +#include <linux/io.h>
> > +#include <linux/log2.h>
> > +#include <linux/bitfield.h>
> > +
> > +/* PWMCFG fields */
> > +#define PWM_SIFIVE_PWMCFG_SCALE         GENMASK(3, 0)
> > +#define PWM_SIFIVE_PWMCFG_STICKY        BIT(8)
> > +#define PWM_SIFIVE_PWMCFG_ZERO_CMP      BIT(9)
> > +#define PWM_SIFIVE_PWMCFG_DEGLITCH      BIT(10)
> > +#define PWM_SIFIVE_PWMCFG_EN_ALWAYS     BIT(12)
> > +#define PWM_SIFIVE_PWMCFG_EN_ONCE       BIT(13)
> > +#define PWM_SIFIVE_PWMCFG_CENTER        BIT(16)
> > +#define PWM_SIFIVE_PWMCFG_GANG          BIT(24)
> > +#define PWM_SIFIVE_PWMCFG_IP            BIT(28)
> > +
> > +/* PWM_SIFIVE_SIZE_PWMCMP is used to calculate offset for pwmcmpX
> registers */
> > +#define PWM_SIFIVE_SIZE_PWMCMP          4
> > +#define PWM_SIFIVE_CMPWIDTH             16
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +struct pwm_sifive_regs {
> > +     unsigned long cfg;
> > +     unsigned long cnt;
> > +     unsigned long pwms;
> > +     unsigned long cmp0;
> > +};
> > +
> > +struct pwm_sifive_data {
> > +     struct pwm_sifive_regs regs;
> > +};
> > +
> > +struct pwm_sifive_priv {
> > +     fdt_addr_t base;
> > +     ulong freq;
> > +     const struct pwm_sifive_data *data; };
> > +
> > +static int pwm_sifive_set_invert(struct udevice *dev, uint channel,
> > +                              bool polarity) {
> > +     debug("%s: Do not support polarity\n", __func__);
> > +
> > +     return 0;
> > +}
> 
> You don;t need this function.
> 
> > +
> > +static int pwm_sifive_set_config(struct udevice *dev, uint channel,
> > +                              uint period_ns, uint duty_ns) {
> > +     struct pwm_sifive_priv *priv = dev_get_priv(dev);
> > +     const struct pwm_sifive_regs *regs = &priv->data->regs;
> > +     unsigned long scale_pow;
> > +     unsigned long long num;
> > +     u32 scale, val = 0, frac;
> > +
> > +     debug("%s: period_ns=%u, duty_ns=%u\n", __func__, period_ns,
> > + duty_ns);
> > +
> > +     /*
> > +      * The PWM unit is used with pwmzerocmp=0, so the only way to
> modify the
> > +      * period length is using pwmscale which provides the number of bits
> the
> > +      * counter is shifted before being feed to the comparators. A period
> > +      * lasts (1 << (PWM_SIFIVE_CMPWIDTH + pwmscale)) clock ticks.
> > +      * (1 << (PWM_SIFIVE_CMPWIDTH + scale)) * 10^9/rate = period
> > +      */
> > +     scale_pow = lldiv((uint64_t)priv->freq * period_ns, 1000000000);
> > +     scale = clamp(ilog2(scale_pow) - PWM_SIFIVE_CMPWIDTH, 0, 0xf);
> > +     val |= FIELD_PREP(PWM_SIFIVE_PWMCFG_SCALE, scale);
> > +
> > +     /*
> > +      * The problem of output producing mixed setting as mentioned at top,
> > +      * occurs here. To minimize the window for this problem, we are
> > +      * calculating the register values first and then writing them
> > +      * consecutively
> > +      */
> > +     num = (u64)duty_ns * (1U << PWM_SIFIVE_CMPWIDTH);
> > +     frac = DIV_ROUND_CLOSEST_ULL(num, period_ns);
> > +     frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > +
> > +     writel(val, (void __iomem *)priv->base + regs->cfg);
> 
> Do you really need this cast on each readl/writel call?

Will fix this in v2.
Thanks for your comments.

- Yash