> -----Original Message----- > From: Heiko Schocher <[email protected]> > Sent: 15 April 2020 22:34 > To: Yash Shah <[email protected]> > Cc: [email protected]; [email protected] > 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 <[email protected]> > > --- > > 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
