| Message ID | 1550049978-20705-3-git-send-email-yash.shah@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PWM support for HiFive Unleashed | expand |
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@sifive.com> > [Atish: Various fixes and code cleanup] > Signed-off-by: Atish Patra <atish.patra@wdc.com> > Signed-off-by: Yash Shah <yash.shah@sifive.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. > +#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. > + 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. > + > + 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? > + > + 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() Also I wonder if we should change the period if the user requested enabled=false. > + 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. > + } > + > + 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. > + 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
On Wed, Feb 13, 2019 at 11:34:59AM +0100, Uwe Kleine-König wrote: > On Wed, Feb 13, 2019 at 02:56:18PM +0530, Yash Shah wrote: [...] > > + 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. Maybe just split all of these very long expressions up by introducing temporary variables to make things more palatable? Thierry
On Wed, Feb 13, 2019 at 4:05 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.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@sifive.com> > > [Atish: Various fixes and code cleanup] > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > Signed-off-by: Yash Shah <yash.shah@sifive.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/ |
Hello, On Thu, Feb 14, 2019 at 01:25:27PM +0530, Yash Shah wrote: > On Wed, Feb 13, 2019 at 4:05 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Wed, Feb 13, 2019 at 02:56:18PM +0530, Yash Shah wrote: > > > +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? Then you only need to be sure that the inactive level is already latched to the pwmcmpXip output (which should only need one clock cycle if I'm not mistaken) before disabling the clock. > > > + 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? No, the lock needs to protect more. You must at least cover increasing and decreasing of user_count and you must hold the lock until the period update is completed. > > 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? I don't know for sure. Given that period is shared for all four PWM outputs it might be sensible to change it at least in a shadow variable and only do it when actually needed. (But maybe we can postpone that as it doesn't matter for correctness of the driver.) The question here is: In the following snippet: pwm0 = pwm_get(... the first pwm ...) pwm_apply_state(pwm0, { .enabled = true, .period = 4000 }); pwm_apply_state(pwm0, { .enabled = false, .period = 8000 }); pwm1 = pwm_get(... the second pwm ...) pwm_apply_state(pwm1, { .enabled = true, .period = 4000 }); pwm_apply_state(pwm0, { .enabled = true, .period = 8000 }); Which of the two last commands should fail? > > > + 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 */ I'd add some blame on the hardware. Something like: /* * 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). */ I'd say it makes sense to put this information at the top of the driver to have it in a prominent place. Also point out the inability to provide 100% duty cycle and that the hardware is limited to inverted output. Then all limitations are summarized in a single place. Maybe this mismatch could be made less likely by changing the order of the register accesses and a delay depending on pwms and old and new settings. But I'd say this is too much for now and can be addressed later when and if necessary. > > > + } > > > + > > > + 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? This question is still open. > > > + 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. I didn't find this in the manual. When looking at Figure 6 and the description of pwmdeglitch I have the impression that your statement is wrong. Setting pwmdeglitch only prevents the output from getting low during a period, it can only go low when pwms overflows (i.e. the start of a period). That's exactly what we want. Where is the misunderstanding? If however you clear pwmdeglitch after an update, consider the following series of events: - Assume pwmcmpX is 0x4000 and pwms is 0x5000, so pwmcmpXip is high. - You set pwmdeglitch and change pwmcmpX to 0x8000 while pwms advanced only little. Then pwmcmpXip remains high. - Then you clear pwmdeglitch at say pwms = 0x5020, this makes pwmcmpXip fall which we should prevent. Also note that when setting pwmdeglitch while configuring pwm0---if it really had the behaviour you pointed out---would interfere with the maybe running pwm1. So I'm convinced keeping pwmdeglitch active always is the right thing to do. Best regards Uwe
On Thu, Feb 14, 2019 at 2:04 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > On Thu, Feb 14, 2019 at 01:25:27PM +0530, Yash Shah wrote: > > On Wed, Feb 13, 2019 at 4:05 PM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > On Wed, Feb 13, 2019 at 02:56:18PM +0530, Yash Shah wrote: > > > > +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? > > Then you only need to be sure that the inactive level is already latched > to the pwmcmpXip output (which should only need one clock cycle if I'm > not mistaken) before disabling the clock. > > > > > + 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? > > No, the lock needs to protect more. You must at least cover increasing > and decreasing of user_count and you must hold the lock until the period > update is completed. Got your point. Will use locks at appropriate places > > > > 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? > > I don't know for sure. Given that period is shared for all four PWM > outputs it might be sensible to change it at least in a shadow variable > and only do it when actually needed. (But maybe we can postpone that as > it doesn't matter for correctness of the driver.) > > The question here is: In the following snippet: > > pwm0 = pwm_get(... the first pwm ...) > > pwm_apply_state(pwm0, { .enabled = true, .period = 4000 }); > pwm_apply_state(pwm0, { .enabled = false, .period = 8000 }); > > pwm1 = pwm_get(... the second pwm ...) > pwm_apply_state(pwm1, { .enabled = true, .period = 4000 }); > pwm_apply_state(pwm0, { .enabled = true, .period = 8000 }); > > Which of the two last commands should fail? > > > > > + 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 */ > > I'd add some blame on the hardware. Something like: > > /* > * 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). > */ > > I'd say it makes sense to put this information at the top of the driver > to have it in a prominent place. Also point out the inability to provide > 100% duty cycle and that the hardware is limited to inverted output. > Then all limitations are summarized in a single place. Sure, will do as you suggested. > > Maybe this mismatch could be made less likely by changing the order of > the register accesses and a delay depending on pwms and old and new > settings. But I'd say this is too much for now and can be addressed > later when and if necessary. > > > > > + } > > > > + > > > > + 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? > > This question is still open. > > > > > + 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. > > I didn't find this in the manual. When looking at Figure 6 and the > description of pwmdeglitch I have the impression that your statement is > wrong. > > Setting pwmdeglitch only prevents the output from getting low during a > period, it can only go low when pwms overflows (i.e. the start of a > period). That's exactly what we want. Where is the misunderstanding? > > If however you clear pwmdeglitch after an update, consider the following > series of events: > > - Assume pwmcmpX is 0x4000 and pwms is 0x5000, so pwmcmpXip is high. > - You set pwmdeglitch and change pwmcmpX to 0x8000 while pwms advanced > only little. Then pwmcmpXip remains high. > - Then you clear pwmdeglitch at say pwms = 0x5020, this makes pwmcmpXip > fall which we should prevent. > > Also note that when setting pwmdeglitch while configuring pwm0---if it > really had the behaviour you pointed out---would interfere with the > maybe running pwm1. > > So I'm convinced keeping pwmdeglitch active always is the right thing to > do. I agree that figure 6 suggests that pwmdeglitch should remain active through out. But for some strange reason when I set deglitch bit, I am seeing every alternate pwm cycle to remain high (latched) unless I clear the deglitch bit. I am debugging this issue however is it ok if I remove deglitch logic from driver until this issue has been root caused? > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
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 +#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 */ + 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); + + /* As scale <= 15 the shift operation cannot overflow. */ + pwm->real_period = div64_ul(1000000000ULL << (PWM_SIFIVE_CMPWIDTH + + scale), rate); + 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)); + + 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); + + 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; + pwm->real_period = state->period; + pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk)); + } + + 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 */ + 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); + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG); + + if (state->enabled != enabled) { + ret = pwm_sifive_enable(chip, dev, state->enabled); + if (ret) + return ret; + } + + return 0; +} + +static const struct pwm_ops pwm_sifive_ops = { + .request = pwm_sifive_request, + .free = pwm_sifive_free, + .get_state = pwm_sifive_get_state, + .apply = pwm_sifive_apply, + .owner = THIS_MODULE, +}; + +static int pwm_sifive_clock_notifier(struct notifier_block *nb, + unsigned long event, void *data) +{ + struct clk_notifier_data *ndata = data; + struct pwm_sifive_ddata *pwm = + container_of(nb, struct pwm_sifive_ddata, notifier); + + if (event == POST_RATE_CHANGE) + pwm_sifive_update_clock(pwm, ndata->new_rate); + + return NOTIFY_OK; +} + +static int pwm_sifive_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct pwm_sifive_ddata *pwm; + struct pwm_chip *chip; + struct resource *res; + int ret; + + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL); + if (!pwm) + return -ENOMEM; + + chip = &pwm->chip; + chip->dev = dev; + chip->ops = &pwm_sifive_ops; + chip->of_pwm_n_cells = 3; + chip->base = -1; + chip->npwm = 4; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + pwm->regs = devm_ioremap_resource(dev, res); + if (IS_ERR(pwm->regs)) { + dev_err(dev, "Unable to map IO resources\n"); + return PTR_ERR(pwm->regs); + } + + pwm->clk = devm_clk_get(dev, NULL); + if (IS_ERR(pwm->clk)) { + if (PTR_ERR(pwm->clk) != -EPROBE_DEFER) + dev_err(dev, "Unable to find controller clock\n"); + return PTR_ERR(pwm->clk); + } + + ret = clk_prepare_enable(pwm->clk); + if (ret) { + dev_err(dev, "failed to enable clock for pwm: %d\n", ret); + return ret; + } + + /* Watch for changes to underlying clock frequency */ + pwm->notifier.notifier_call = pwm_sifive_clock_notifier; + ret = clk_notifier_register(pwm->clk, &pwm->notifier); + if (ret) { + dev_err(dev, "failed to register clock notifier: %d\n", ret); + goto disable_clk; + } + + ret = pwmchip_add(chip); + if (ret < 0) { + dev_err(dev, "cannot register PWM: %d\n", ret); + goto unregister_clk; + } + + if (!pwm_is_enabled(pwm->chip.pwms)) + clk_disable(pwm->clk); + + platform_set_drvdata(pdev, pwm); + dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm); + + return 0; + +unregister_clk: + clk_notifier_unregister(pwm->clk, &pwm->notifier); +disable_clk: + clk_disable_unprepare(pwm->clk); + + return ret; +} + +static int pwm_sifive_remove(struct platform_device *dev) +{ + struct pwm_sifive_ddata *pwm = platform_get_drvdata(dev); + int ret; + + ret = pwmchip_remove(&pwm->chip); + clk_notifier_unregister(pwm->clk, &pwm->notifier); + if (!pwm_is_enabled(pwm->chip.pwms)) + clk_disable(pwm->clk); + clk_unprepare(pwm->clk); + return ret; +} + +static const struct of_device_id pwm_sifive_of_match[] = { + { .compatible = "sifive,pwm0" }, + {}, +}; +MODULE_DEVICE_TABLE(of, pwm_sifive_of_match); + +static struct platform_driver pwm_sifive_driver = { + .probe = pwm_sifive_probe, + .remove = pwm_sifive_remove, + .driver = { + .name = "pwm-sifive", + .of_match_table = pwm_sifive_of_match, + }, +}; +module_platform_driver(pwm_sifive_driver); + +MODULE_DESCRIPTION("SiFive PWM driver"); +MODULE_LICENSE("GPL v2");