Message ID | 1547194964-16718-3-git-send-email-yash.shah@sifive.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | PWM support for HiFive Unleashed | expand |
From a general code quality point of view this looks fine to me.
I don't really know anything about the PWM subsystem, though:
Reviewed-by: Christoph Hellwig <hch@lst.de>
Hello, On Fri, Jan 11, 2019 at 01:52:44PM +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 | 10 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 257 insertions(+) > create mode 100644 drivers/pwm/pwm-sifive.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index a8f47df..3bcaf6a 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -380,6 +380,16 @@ 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 I'd say add: depends on MACH_SIFIVE || COMPILE_TEST (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.) > + 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..7fee809 > --- /dev/null > +++ b/drivers/pwm/pwm-sifive.c > @@ -0,0 +1,246 @@ > +// 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 I wonder if such an instance should be only a single PWM instead of four. Then you were more flexible with the period lengths (using pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode. I didn't understand how the deglitch logic works yet. Currently it is not used which might result in four edges in a single period (which is bad). > + */ > +#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 REG_PWMCFG 0x0 > +#define REG_PWMCOUNT 0x8 > +#define REG_PWMS 0x10 > +#define REG_PWMCMP0 0x20 I suggest a common prefix for these defines. Something like PWM_SIFIVE_ > + > +/* PWMCFG fields */ > +#define BIT_PWM_SCALE 0 > +#define BIT_PWM_STICKY 8 > +#define BIT_PWM_ZERO_ZMP 9 the manual calls this "pwmzerocmp". > +#define BIT_PWM_DEGLITCH 10 > +#define BIT_PWM_EN_ALWAYS 12 > +#define BIT_PWM_EN_ONCE 13 > +#define BIT_PWM0_CENTER 16 > +#define BIT_PWM0_GANG 24 > +#define BIT_PWM0_IP 28 Also a common prefix please. Something like PWM_SIFIVE_PWMCFG_ seems sensible. > +#define SIZE_PWMCMP 4 Please describe what this constant means. I think this is "ncmp" in the reference manual. If so, using PWM_SIFIVE_NCMP as name instead seems adequate. > +#define MASK_PWM_SCALE 0xf MASK_PWM_SCALE is unused, please drop it. > +struct sifive_pwm_device { > + struct pwm_chip chip; > + struct notifier_block notifier; > + struct clk *clk; > + void __iomem *regs; > + unsigned int approx_period; > + unsigned int real_period; > +}; I'd call this pwm_sifive_ddata. The prefix because the driver is called pwm-sifive and ddata because this is driver data and not a device. > +static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c) > +{ > + return container_of(c, struct sifive_pwm_device, chip); > +} > + > +static void sifive_pwm_get_state(struct pwm_chip *chip, struct pwm_device *dev, > + struct pwm_state *state) given that the driver is called pwm-sifive, please use pwm_sifive as function prefix. > +{ > + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip); > + u32 duty; > + > + duty = readl(pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP); > + > + state->period = pwm->real_period; > + state->duty_cycle = ((u64)duty * pwm->real_period) >> 16; In the reference manual this 16 is called "cmpwidth" I think. If I understand correctly this might in theory be different from 16, so it would be great if this would be at least a cpp symbol for now. > + state->polarity = PWM_POLARITY_INVERSED; > + state->enabled = duty > 0; > +} > + > +static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev, > + struct pwm_state *state) > +{ > + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip); > + unsigned int duty_cycle; > + u32 frac; > + > + if (state->polarity != PWM_POLARITY_INVERSED) > + return -EINVAL; > + > + duty_cycle = state->duty_cycle; > + if (!state->enabled) > + duty_cycle = 0; > + > + frac = div_u64((u64)duty_cycle << 16, state->period); > + frac = min(frac, 0xFFFFU); In the previous review round I asked here: | Also if real_period is for example 10 ms and the consumer requests | duty=12 ms + period=100 ms, the hardware is configured for duty=1.2 ms + | period=10 ms, right? which you confirmed. IMHO this is not acceptable. If the period is fixed, you should return -EINVAL (I think) if a different period is requested. > + writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP); If you get a constant inactive output with frac=0 and a constant active output with frac=0xffff the calculation above seems wrong to me. (A value i written to the pwmcmpX register means a duty cycle of (i * period / 0xffff). Your calculation assumes a divisor of 0x10000 however.) > + > + if (state->enabled) > + sifive_pwm_get_state(chip, dev, state); @Thierry: Should we bless this correction of state? > + > + return 0; > +} > + > +static const struct pwm_ops sifive_pwm_ops = { > + .get_state = sifive_pwm_get_state, > + .apply = sifive_pwm_apply, > + .owner = THIS_MODULE, > +}; > + > +static struct pwm_device *sifive_pwm_xlate(struct pwm_chip *chip, > + const struct of_phandle_args *args) > +{ > + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip); > + struct pwm_device *dev; > + > + if (args->args[0] >= chip->npwm) > + return ERR_PTR(-EINVAL); > + > + dev = pwm_request_from_chip(chip, args->args[0], NULL); > + if (IS_ERR(dev)) > + return dev; > + > + /* The period cannot be changed on a per-PWM basis */ > + dev->args.period = pwm->real_period; > + dev->args.polarity = PWM_POLARITY_NORMAL; > + if (args->args[1] & PWM_POLARITY_INVERSED) > + dev->args.polarity = PWM_POLARITY_INVERSED; > + > + return dev; > +} > + > +static void sifive_pwm_update_clock(struct sifive_pwm_device *pwm, > + unsigned long rate) > +{ > + /* (1 << (16+scale)) * 10^9/rate = real_period */ > + unsigned long scale_pow = (pwm->approx_period * (u64)rate) / 1000000000; NSEC_PER_SEC instead of 1000000000 > + int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf); > + > + writel((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE), > + pwm->regs + REG_PWMCFG); > + > + /* As scale <= 15 the shift operation cannot overflow. */ > + pwm->real_period = div64_ul(1000000000ULL << (16 + scale), rate); > + dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period); > +} > + > +static int sifive_pwm_clock_notifier(struct notifier_block *nb, > + unsigned long event, void *data) > +{ > + struct clk_notifier_data *ndata = data; > + struct sifive_pwm_device *pwm = > + container_of(nb, struct sifive_pwm_device, notifier); > + > + if (event == POST_RATE_CHANGE) > + sifive_pwm_update_clock(pwm, ndata->new_rate); Does this need locking? (Maybe not with the current state.) > + > + return NOTIFY_OK; > +} > + > +static int sifive_pwm_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *node = pdev->dev.of_node; > + struct sifive_pwm_device *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 = &sifive_pwm_ops; > + chip->of_xlate = sifive_pwm_xlate; > + chip->of_pwm_n_cells = 2; > + chip->base = -1; > + chip->npwm = 4; > + > + ret = of_property_read_u32(node, "sifive,approx-period-ns", > + &pwm->approx_period); > + if (ret < 0) { > + dev_err(dev, "Unable to read sifive,approx-period from DTS\n"); > + return ret; > + } > + > + 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 = sifive_pwm_clock_notifier; > + ret = clk_notifier_register(pwm->clk, &pwm->notifier); > + if (ret) { > + dev_err(dev, "failed to register clock notifier: %d\n", ret); > + clk_disable_unprepare(pwm->clk); > + return ret; > + } > + > + /* Initialize PWM config */ > + sifive_pwm_update_clock(pwm, clk_get_rate(pwm->clk)); > + > + ret = pwmchip_add(chip); > + if (ret < 0) { > + dev_err(dev, "cannot register PWM: %d\n", ret); > + clk_notifier_unregister(pwm->clk, &pwm->notifier); > + clk_disable_unprepare(pwm->clk); > + return ret; > + } Can you please use a common error path using goto? > + platform_set_drvdata(pdev, pwm); > + dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm); > + > + return 0; > +} > + > +static int sifive_pwm_remove(struct platform_device *dev) > +{ > + struct sifive_pwm_device *pwm = platform_get_drvdata(dev); > + int ret; > + > + ret = pwmchip_remove(&pwm->chip); > + clk_notifier_unregister(pwm->clk, &pwm->notifier); > + clk_disable_unprepare(pwm->clk); > + return ret; > +} > + > +static const struct of_device_id sifive_pwm_of_match[] = { > + { .compatible = "sifive,pwm0" }, > + { .compatible = "sifive,fu540-c000-pwm" }, Do you really need both compatible strings here? > + {}, > +}; > +MODULE_DEVICE_TABLE(of, sifive_pwm_of_match); > + > +static struct platform_driver sifive_pwm_driver = { > + .probe = sifive_pwm_probe, > + .remove = sifive_pwm_remove, > + .driver = { > + .name = "pwm-sifive", > + .of_match_table = sifive_pwm_of_match, > + }, > +}; > +module_platform_driver(sifive_pwm_driver); > + > +MODULE_DESCRIPTION("SiFive PWM driver"); > +MODULE_LICENSE("GPL v2"); Best regards Uwe
On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > On Fri, Jan 11, 2019 at 01:52:44PM +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 | 10 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 257 insertions(+) > > create mode 100644 drivers/pwm/pwm-sifive.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index a8f47df..3bcaf6a 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -380,6 +380,16 @@ 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 > > I'd say add: > > depends on MACH_SIFIVE || COMPILE_TEST > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.) As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available. @Paul, Do you have any comments on this? > > > + 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..7fee809 > > --- /dev/null > > +++ b/drivers/pwm/pwm-sifive.c > > @@ -0,0 +1,246 @@ > > +// 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 > > I wonder if such an instance should be only a single PWM instead of > four. Then you were more flexible with the period lengths (using > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode. > > I didn't understand how the deglitch logic works yet. Currently it is > not used which might result in four edges in a single period (which is > bad). I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in REG_PWMCFG. Will do that. > > > + */ > > +#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 REG_PWMCFG 0x0 > > +#define REG_PWMCOUNT 0x8 > > +#define REG_PWMS 0x10 > > +#define REG_PWMCMP0 0x20 > > I suggest a common prefix for these defines. Something like > PWM_SIFIVE_ Sure. > > > + > > +/* PWMCFG fields */ > > +#define BIT_PWM_SCALE 0 > > +#define BIT_PWM_STICKY 8 > > +#define BIT_PWM_ZERO_ZMP 9 > > the manual calls this "pwmzerocmp". Will fix this. > > > +#define BIT_PWM_DEGLITCH 10 > > +#define BIT_PWM_EN_ALWAYS 12 > > +#define BIT_PWM_EN_ONCE 13 > > +#define BIT_PWM0_CENTER 16 > > +#define BIT_PWM0_GANG 24 > > +#define BIT_PWM0_IP 28 > > Also a common prefix please. Something like PWM_SIFIVE_PWMCFG_ seems > sensible. Sure. > > > +#define SIZE_PWMCMP 4 > > Please describe what this constant means. I think this is "ncmp" in the > reference manual. If so, using PWM_SIFIVE_NCMP as name instead seems > adequate. No, it is not ncmp. It is used to calculate the offset for pwmcmp registers. I will add the description for the above constant. > > > +#define MASK_PWM_SCALE 0xf > > MASK_PWM_SCALE is unused, please drop it. Sure. > > > +struct sifive_pwm_device { > > + struct pwm_chip chip; > > + struct notifier_block notifier; > > + struct clk *clk; > > + void __iomem *regs; > > + unsigned int approx_period; > > + unsigned int real_period; > > +}; > > I'd call this pwm_sifive_ddata. The prefix because the driver is called > pwm-sifive and ddata because this is driver data and not a device. Will be done. > > > +static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c) > > +{ > > + return container_of(c, struct sifive_pwm_device, chip); > > +} > > + > > +static void sifive_pwm_get_state(struct pwm_chip *chip, struct pwm_device *dev, > > + struct pwm_state *state) > > given that the driver is called pwm-sifive, please use pwm_sifive as > function prefix. Sure. Will change it for all functions. > > > +{ > > + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip); > > + u32 duty; > > + > > + duty = readl(pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP); > > + > > + state->period = pwm->real_period; > > + state->duty_cycle = ((u64)duty * pwm->real_period) >> 16; > > In the reference manual this 16 is called "cmpwidth" I think. If I > understand correctly this might in theory be different from 16, so it > would be great if this would be at least a cpp symbol for now. I assume you meant to add a macro for cmpwidth and use it here. Will be done. > > > + state->polarity = PWM_POLARITY_INVERSED; > > + state->enabled = duty > 0; > > +} > > + > > +static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev, > > + struct pwm_state *state) > > +{ > > + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip); > > + unsigned int duty_cycle; > > + u32 frac; > > + > > + if (state->polarity != PWM_POLARITY_INVERSED) > > + return -EINVAL; > > + > > + duty_cycle = state->duty_cycle; > > + if (!state->enabled) > > + duty_cycle = 0; > > + > > + frac = div_u64((u64)duty_cycle << 16, state->period); > > + frac = min(frac, 0xFFFFU); > > In the previous review round I asked here: > > | Also if real_period is for example 10 ms and the consumer requests > | duty=12 ms + period=100 ms, the hardware is configured for duty=1.2 ms + > | period=10 ms, right? > > which you confirmed. IMHO this is not acceptable. If the period is > fixed, you should return -EINVAL (I think) if a different period is > requested. Will return -EINVAL on state->period != pwm->real_period > > > + writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP); > > If you get a constant inactive output with frac=0 and a constant active > output with frac=0xffff the calculation above seems wrong to me. > (A value i written to the pwmcmpX register means a duty cycle of > (i * period / 0xffff). Your calculation assumes a divisor of 0x10000 > however.) Not sure if I get you completely. But, if divisor of 0xffff is your concern then does the below look ok? frac = div_u64(((u64)duty_cycle << 16) - duty_cycle, state->period); > > > + > > + if (state->enabled) > > + sifive_pwm_get_state(chip, dev, state); > > @Thierry: Should we bless this correction of state? > > > + > > + return 0; > > +} > > + > > +static const struct pwm_ops sifive_pwm_ops = { > > + .get_state = sifive_pwm_get_state, > > + .apply = sifive_pwm_apply, > > + .owner = THIS_MODULE, > > +}; > > + > > +static struct pwm_device *sifive_pwm_xlate(struct pwm_chip *chip, > > + const struct of_phandle_args *args) > > +{ > > + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip); > > + struct pwm_device *dev; > > + > > + if (args->args[0] >= chip->npwm) > > + return ERR_PTR(-EINVAL); > > + > > + dev = pwm_request_from_chip(chip, args->args[0], NULL); > > + if (IS_ERR(dev)) > > + return dev; > > + > > + /* The period cannot be changed on a per-PWM basis */ > > + dev->args.period = pwm->real_period; > > + dev->args.polarity = PWM_POLARITY_NORMAL; > > + if (args->args[1] & PWM_POLARITY_INVERSED) > > + dev->args.polarity = PWM_POLARITY_INVERSED; > > + > > + return dev; > > +} > > + > > +static void sifive_pwm_update_clock(struct sifive_pwm_device *pwm, > > + unsigned long rate) > > +{ > > + /* (1 << (16+scale)) * 10^9/rate = real_period */ > > + unsigned long scale_pow = (pwm->approx_period * (u64)rate) / 1000000000; > > NSEC_PER_SEC instead of 1000000000 Will be done. > > > + int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf); > > + > > + writel((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE), > > + pwm->regs + REG_PWMCFG); > > + > > + /* As scale <= 15 the shift operation cannot overflow. */ > > + pwm->real_period = div64_ul(1000000000ULL << (16 + scale), rate); > > + dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period); > > +} > > + > > +static int sifive_pwm_clock_notifier(struct notifier_block *nb, > > + unsigned long event, void *data) > > +{ > > + struct clk_notifier_data *ndata = data; > > + struct sifive_pwm_device *pwm = > > + container_of(nb, struct sifive_pwm_device, notifier); > > + > > + if (event == POST_RATE_CHANGE) > > + sifive_pwm_update_clock(pwm, ndata->new_rate); > > Does this need locking? (Maybe not with the current state.) No. We can add it when required. > > > + > > + return NOTIFY_OK; > > +} > > + > > +static int sifive_pwm_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct device_node *node = pdev->dev.of_node; > > + struct sifive_pwm_device *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 = &sifive_pwm_ops; > > + chip->of_xlate = sifive_pwm_xlate; > > + chip->of_pwm_n_cells = 2; > > + chip->base = -1; > > + chip->npwm = 4; > > + > > + ret = of_property_read_u32(node, "sifive,approx-period-ns", > > + &pwm->approx_period); > > + if (ret < 0) { > > + dev_err(dev, "Unable to read sifive,approx-period from DTS\n"); > > + return ret; > > + } > > + > > + 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 = sifive_pwm_clock_notifier; > > + ret = clk_notifier_register(pwm->clk, &pwm->notifier); > > + if (ret) { > > + dev_err(dev, "failed to register clock notifier: %d\n", ret); > > + clk_disable_unprepare(pwm->clk); > > + return ret; > > + } > > + > > + /* Initialize PWM config */ > > + sifive_pwm_update_clock(pwm, clk_get_rate(pwm->clk)); > > + > > + ret = pwmchip_add(chip); > > + if (ret < 0) { > > + dev_err(dev, "cannot register PWM: %d\n", ret); > > + clk_notifier_unregister(pwm->clk, &pwm->notifier); > > + clk_disable_unprepare(pwm->clk); > > + return ret; > > + } > > Can you please use a common error path using goto? Sure. > > > + platform_set_drvdata(pdev, pwm); > > + dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm); > > + > > + return 0; > > +} > > + > > +static int sifive_pwm_remove(struct platform_device *dev) > > +{ > > + struct sifive_pwm_device *pwm = platform_get_drvdata(dev); > > + int ret; > > + > > + ret = pwmchip_remove(&pwm->chip); > > + clk_notifier_unregister(pwm->clk, &pwm->notifier); > > + clk_disable_unprepare(pwm->clk); > > + return ret; > > +} > > + > > +static const struct of_device_id sifive_pwm_of_match[] = { > > + { .compatible = "sifive,pwm0" }, > > + { .compatible = "sifive,fu540-c000-pwm" }, > > Do you really need both compatible strings here? I believe I can remove the second compatible string. @Paul, Correct me if I am wrong. > > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, sifive_pwm_of_match); > > + > > +static struct platform_driver sifive_pwm_driver = { > > + .probe = sifive_pwm_probe, > > + .remove = sifive_pwm_remove, > > + .driver = { > > + .name = "pwm-sifive", > > + .of_match_table = sifive_pwm_of_match, > > + }, > > +}; > > +module_platform_driver(sifive_pwm_driver); > > + > > +MODULE_DESCRIPTION("SiFive PWM driver"); > > +MODULE_LICENSE("GPL v2"); > > Best regards > Uwe Thanks for the comments. > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello, On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote: > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Fri, Jan 11, 2019 at 01:52:44PM +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 | 10 ++ > > > drivers/pwm/Makefile | 1 + > > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 257 insertions(+) > > > create mode 100644 drivers/pwm/pwm-sifive.c > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > index a8f47df..3bcaf6a 100644 > > > --- a/drivers/pwm/Kconfig > > > +++ b/drivers/pwm/Kconfig > > > @@ -380,6 +380,16 @@ 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 > > > > I'd say add: > > > > depends on MACH_SIFIVE || COMPILE_TEST > > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.) > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available. > @Paul, Do you have any comments on this? If this is not going to be available at least protect it by depends RISCV || COMPILE_TEST > > I wonder if such an instance should be only a single PWM instead of > > four. Then you were more flexible with the period lengths (using > > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode. > > > > I didn't understand how the deglitch logic works yet. Currently it is > > not used which might result in four edges in a single period (which is > > bad). > > I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in > REG_PWMCFG. Will do that. This only works for the first pwm output though. > > > +struct sifive_pwm_device { > > > + struct pwm_chip chip; > > > + struct notifier_block notifier; > > > + struct clk *clk; > > > + void __iomem *regs; > > > + unsigned int approx_period; When thinking about this a bit more, I think the better approach would be to let the consumer change the period iff there is only one consumer. Then you can drop that approx-period stuff and the result is more flexible. (Having said that I still prefer making the driver provide only a single PWM with the ability to have periods other than powers of two.) > > > + writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP); > > > > If you get a constant inactive output with frac=0 and a constant active > > output with frac=0xffff the calculation above seems wrong to me. > > (A value i written to the pwmcmpX register means a duty cycle of > > (i * period / 0xffff). Your calculation assumes a divisor of 0x10000 > > however.) > > Not sure if I get you completely. But, if divisor of 0xffff is your concern then > does the below look ok? > > frac = div_u64(((u64)duty_cycle << 16) - duty_cycle, state->period); This works (I think, didn't redo the maths), but I would prefer frac = div_u64((u64)duty_cycle * 0xffff, state->period); for better readability. (Maybe the compiler is even clever enough to see this can be calculated as you suggested.) > > > +static int sifive_pwm_clock_notifier(struct notifier_block *nb, > > > + unsigned long event, void *data) > > > +{ > > > + struct clk_notifier_data *ndata = data; > > > + struct sifive_pwm_device *pwm = > > > + container_of(nb, struct sifive_pwm_device, notifier); > > > + > > > + if (event == POST_RATE_CHANGE) > > > + sifive_pwm_update_clock(pwm, ndata->new_rate); > > > > Does this need locking? (Maybe not with the current state.) > > No. We can add it when required. My thought was, that the clk freq might change while .apply is active. But given that you only configure the relative duty cycle this is independent of the actual clk freq. Best regards Uwe
On Wed, 16 Jan 2019, Uwe Kleine-König wrote: > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote: > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote: > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > > index a8f47df..3bcaf6a 100644 > > > > --- a/drivers/pwm/Kconfig > > > > +++ b/drivers/pwm/Kconfig > > > > @@ -380,6 +380,16 @@ 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 > > > > > > I'd say add: > > > > > > depends on MACH_SIFIVE || COMPILE_TEST > > > > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.) > > > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available. > > @Paul, Do you have any comments on this? > > If this is not going to be available at least protect it by > > depends RISCV || COMPILE_TEST There's nothing RISC-V or SiFive SoC-specific about this driver or IP block. The HDL for this IP block is open-source and posted on Github. The IP block and driver would work unchanged on an ARM or MIPS SoC, and in fact, SiFive does design ARM-based SoCs as well. Likewise, any other SoC vendor could take the HDL for this IP block from the git tree and implement it on their own SoC. More generally: it's a basic principle of Linux device drivers that they should be buildable for any architecture. The idea here is to prevent developers from burying architecture or SoC-specific hacks into the driver. So there shouldn't be any architecture or SoC-specific code in any device driver, unless it's abstracted in some way - ideally through a common framework. So from this point of view, neither "depends MACH_SIFIVE" nor "depends RISCV" would be appropriate. Similarly, the equivalents for other architectures (e.g. "ARCH_ARM") or SoC manufacturers (e.g., "MACH_SAMSUNG") wouldn't be appropriate for any generic IP block device driver like this one. - Paul
On Wed, Jan 16, 2019 at 09:18:45AM -0800, Paul Walmsley wrote: > On Wed, 16 Jan 2019, Uwe Kleine-König wrote: > > > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote: > > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote: > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > > > index a8f47df..3bcaf6a 100644 > > > > > --- a/drivers/pwm/Kconfig > > > > > +++ b/drivers/pwm/Kconfig > > > > > @@ -380,6 +380,16 @@ 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 > > > > > > > > I'd say add: > > > > > > > > depends on MACH_SIFIVE || COMPILE_TEST > > > > > > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.) > > > > > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available. > > > @Paul, Do you have any comments on this? > > > > If this is not going to be available at least protect it by > > > > depends RISCV || COMPILE_TEST > > There's nothing RISC-V or SiFive SoC-specific about this driver or IP > block. The HDL for this IP block is open-source and posted on Github. > The IP block and driver would work unchanged on an ARM or MIPS SoC, and in > fact, SiFive does design ARM-based SoCs as well. Likewise, any other SoC > vendor could take the HDL for this IP block from the git tree and > implement it on their own SoC. > > More generally: it's a basic principle of Linux device drivers that they > should be buildable for any architecture. The idea here is to prevent > developers from burying architecture or SoC-specific hacks into the > driver. So there shouldn't be any architecture or SoC-specific code in > any device driver, unless it's abstracted in some way - ideally through a > common framework. > > So from this point of view, neither "depends MACH_SIFIVE" nor "depends > RISCV" would be appropriate. Similarly, the equivalents for other > architectures (e.g. "ARCH_ARM") or SoC manufacturers (e.g., > "MACH_SAMSUNG") wouldn't be appropriate for any generic IP block device > driver like this one. The dependency serves two purposes: a) ensure that the build requirements are fulfilled. b) restrict configuration to actually existing machines When considering b) it doesn't make sense to enable the driver for (say) a machine that is powered by an ARM SoC by TI. If you still want to compile test the sifive driver for ARM, enable COMPILE_TEST. That's what this symbol is there for. Best regards Uwe
On Wed, 16 Jan 2019, Uwe Kleine-König wrote: > On Wed, Jan 16, 2019 at 09:18:45AM -0800, Paul Walmsley wrote: > > On Wed, 16 Jan 2019, Uwe Kleine-König wrote: > > > > > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote: > > > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote: > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > > > > index a8f47df..3bcaf6a 100644 > > > > > > --- a/drivers/pwm/Kconfig > > > > > > +++ b/drivers/pwm/Kconfig > > > > > > @@ -380,6 +380,16 @@ 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 > > > > > > > > > > I'd say add: > > > > > > > > > > depends on MACH_SIFIVE || COMPILE_TEST > > > > > > > > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.) > > > > > > > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available. > > > > @Paul, Do you have any comments on this? > > > > > > If this is not going to be available at least protect it by > > > > > > depends RISCV || COMPILE_TEST > > > > There's nothing RISC-V or SiFive SoC-specific about this driver or IP > > block. The HDL for this IP block is open-source and posted on Github. > > The IP block and driver would work unchanged on an ARM or MIPS SoC, and in > > fact, SiFive does design ARM-based SoCs as well. Likewise, any other SoC > > vendor could take the HDL for this IP block from the git tree and > > implement it on their own SoC. > > > > More generally: it's a basic principle of Linux device drivers that they > > should be buildable for any architecture. The idea here is to prevent > > developers from burying architecture or SoC-specific hacks into the > > driver. So there shouldn't be any architecture or SoC-specific code in > > any device driver, unless it's abstracted in some way - ideally through a > > common framework. > > > > So from this point of view, neither "depends MACH_SIFIVE" nor "depends > > RISCV" would be appropriate. Similarly, the equivalents for other > > architectures (e.g. "ARCH_ARM") or SoC manufacturers (e.g., > > "MACH_SAMSUNG") wouldn't be appropriate for any generic IP block device > > driver like this one. > > The dependency serves two purposes: > > a) ensure that the build requirements are fulfilled. > b) restrict configuration to actually existing machines > > When considering b) it doesn't make sense to enable the driver for (say) > a machine that is powered by an ARM SoC by TI. If you still want to > compile test the sifive driver for ARM, enable COMPILE_TEST. That's what > this symbol is there for. COMPILE_TEST made slightly more sense before the broad availability of open-source soft cores, SoC integration, and IP; and before powerful, inexpensive FPGAs and SoCs with FPGA fabrics were common. Even back then, COMPILE_TEST was starting to look questionable. IP blocks from CPU- and SoC vendor-independent libraries, like DesignWare and the Cadence IP libraries, were integrated on SoCs across varying CPU architectures. (Fortunately, looking at the tree, subsystem maintainers with DesignWare drivers seem to have largely avoided adding architecture or SoC-specific Kconfig restrictions there - and thus have also avoided the need for COMPILE_TEST.) If an unnecessary architecture Kconfig dependency was added for a leaf driver, that Kconfig line would either need to be patched out by hand, or if present, COMPILE_TEST would need to be enabled. This was less of a problem then. There were very few FPGA Linux users, and they were mostly working on internal proprietary projects. FPGAs that could run Linux at a reasonable speed, including MMUs and FPUs, were expensive or were missing good tool support. So most FPGA Linux development was restricted to ASIC vendors - the Samsungs, Qualcomms, NVIDIAs of the world - for prototyping, and those FPGA platforms never made it outside those companies. The situation is different now. The major FPGA vendors have inexpensive FPGA SoCs with full-featured CPU hard macros. The development boards can be quite affordable (< USD 300 for the Xilinx Ultra96). There are also now open-source SoC HDL implementations - including MMUs, FPUs, and peripherals like PWM and UART - for the conventional FPGA market. These can run at mid-1990's clock rates - slow by modern standards but still quite usable. So or vendor-specific IP blocks that are unlikely to ever be reused by anyone else, like the NVIDIA or TI PWM blocks, maybe there's still some justification for COMPILE_TEST. But for drivers for open-source, SoC-independent peripheral IP blocks - or even for proprietary IP blocks from library vendors like Synopsys or Cadence - like this PWM driver, we shouldn't add unnecessary architecture, SoC vendor, or COMPILE_TEST Kconfig dependencies. They will just force users to patch the kernel or enable COMPILE_TEST for kernels that are actually meant to run on real hardware. - Paul
On Wed, Jan 16, 2019 at 10:16 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote: > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > On Fri, Jan 11, 2019 at 01:52:44PM +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 | 10 ++ > > > > drivers/pwm/Makefile | 1 + > > > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 257 insertions(+) > > > > create mode 100644 drivers/pwm/pwm-sifive.c > > > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > > index a8f47df..3bcaf6a 100644 > > > > --- a/drivers/pwm/Kconfig > > > > +++ b/drivers/pwm/Kconfig > > > > @@ -380,6 +380,16 @@ 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 > > > > > > I'd say add: > > > > > > depends on MACH_SIFIVE || COMPILE_TEST > > > > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.) > > > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available. > > @Paul, Do you have any comments on this? > > If this is not going to be available at least protect it by > > depends RISCV || COMPILE_TEST > > > > I wonder if such an instance should be only a single PWM instead of > > > four. Then you were more flexible with the period lengths (using > > > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode. > > > > > > I didn't understand how the deglitch logic works yet. Currently it is > > > not used which might result in four edges in a single period (which is > > > bad). > > > > I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in > > REG_PWMCFG. Will do that. > > This only works for the first pwm output though. > > > > > +struct sifive_pwm_device { > > > > + struct pwm_chip chip; > > > > + struct notifier_block notifier; > > > > + struct clk *clk; > > > > + void __iomem *regs; > > > > + unsigned int approx_period; > > When thinking about this a bit more, I think the better approach would > be to let the consumer change the period iff there is only one consumer. > Then you can drop that approx-period stuff and the result is more > flexible. (Having said that I still prefer making the driver provide > only a single PWM with the ability to have periods other than powers of > two.) > I am not confident about the implementation of the way you are suggesting. As of now, I am going to stick with the current implementation. > > > > + writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP); > > > > > > If you get a constant inactive output with frac=0 and a constant active > > > output with frac=0xffff the calculation above seems wrong to me. > > > (A value i written to the pwmcmpX register means a duty cycle of > > > (i * period / 0xffff). Your calculation assumes a divisor of 0x10000 > > > however.) > > > > Not sure if I get you completely. But, if divisor of 0xffff is your concern then > > does the below look ok? > > > > frac = div_u64(((u64)duty_cycle << 16) - duty_cycle, state->period); > > This works (I think, didn't redo the maths), but I would prefer > > frac = div_u64((u64)duty_cycle * 0xffff, state->period); > > for better readability. (Maybe the compiler is even clever enough to see > this can be calculated as you suggested.) Sure, will multiply it with 0xffff for better readability. > > > > > +static int sifive_pwm_clock_notifier(struct notifier_block *nb, > > > > + unsigned long event, void *data) > > > > +{ > > > > + struct clk_notifier_data *ndata = data; > > > > + struct sifive_pwm_device *pwm = > > > > + container_of(nb, struct sifive_pwm_device, notifier); > > > > + > > > > + if (event == POST_RATE_CHANGE) > > > > + sifive_pwm_update_clock(pwm, ndata->new_rate); > > > > > > Does this need locking? (Maybe not with the current state.) > > > > No. We can add it when required. > > My thought was, that the clk freq might change while .apply is active. > But given that you only configure the relative duty cycle this is > independent of the actual clk freq. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hello, On Thu, Jan 17, 2019 at 12:15:38PM +0530, Yash Shah wrote: > On Wed, Jan 16, 2019 at 10:16 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > Hello, > > > > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote: > > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > On Fri, Jan 11, 2019 at 01:52:44PM +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 | 10 ++ > > > > > drivers/pwm/Makefile | 1 + > > > > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++ > > > > > 3 files changed, 257 insertions(+) > > > > > create mode 100644 drivers/pwm/pwm-sifive.c > > > > > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > > > index a8f47df..3bcaf6a 100644 > > > > > --- a/drivers/pwm/Kconfig > > > > > +++ b/drivers/pwm/Kconfig > > > > > @@ -380,6 +380,16 @@ 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 > > > > > > > > I'd say add: > > > > > > > > depends on MACH_SIFIVE || COMPILE_TEST > > > > > > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.) > > > > > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available. > > > @Paul, Do you have any comments on this? > > > > If this is not going to be available at least protect it by > > > > depends RISCV || COMPILE_TEST > > > > > > I wonder if such an instance should be only a single PWM instead of > > > > four. Then you were more flexible with the period lengths (using > > > > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode. > > > > > > > > I didn't understand how the deglitch logic works yet. Currently it is > > > > not used which might result in four edges in a single period (which is > > > > bad). > > > > > > I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in > > > REG_PWMCFG. Will do that. > > > > This only works for the first pwm output though. I mixed this up with pwmzerocmp; deglitch should work on all four outputs. > > > > > +struct sifive_pwm_device { > > > > > + struct pwm_chip chip; > > > > > + struct notifier_block notifier; > > > > > + struct clk *clk; > > > > > + void __iomem *regs; > > > > > + unsigned int approx_period; > > > > When thinking about this a bit more, I think the better approach would > > be to let the consumer change the period iff there is only one consumer. > > Then you can drop that approx-period stuff and the result is more > > flexible. (Having said that I still prefer making the driver provide > > only a single PWM with the ability to have periods other than powers of > > two.) > > > > I am not confident about the implementation of the way you are suggesting. > As of now, I am going to stick with the current implementation. The idea is to count the users using the .request and .free callbacks. Iff there is exactly one, allow changes to period. But please consider moving to an abstraction that only provides a single pwm instead. Best regards Uwe
Hello Paul, On Wed, Jan 16, 2019 at 11:29:35AM -0800, Paul Walmsley wrote: > COMPILE_TEST made slightly more sense before the broad availability of > open-source soft cores, SoC integration, and IP; and before powerful, > inexpensive FPGAs and SoCs with FPGA fabrics were common. > > Even back then, COMPILE_TEST was starting to look questionable. IP blocks > from CPU- and SoC vendor-independent libraries, like DesignWare and the > Cadence IP libraries, were integrated on SoCs across varying CPU > architectures. (Fortunately, looking at the tree, subsystem maintainers > with DesignWare drivers seem to have largely avoided adding architecture > or SoC-specific Kconfig restrictions there - and thus have also avoided > the need for COMPILE_TEST.) If an unnecessary architecture Kconfig > dependency was added for a leaf driver, that Kconfig line would either > need to be patched out by hand, or if present, COMPILE_TEST would need to > be enabled. > > This was less of a problem then. There were very few FPGA Linux users, > and they were mostly working on internal proprietary projects. FPGAs that > could run Linux at a reasonable speed, including MMUs and FPUs, were > expensive or were missing good tool support. So most FPGA Linux > development was restricted to ASIC vendors - the Samsungs, Qualcomms, > NVIDIAs of the world - for prototyping, and those FPGA platforms never > made it outside those companies. > > The situation is different now. The major FPGA vendors have inexpensive > FPGA SoCs with full-featured CPU hard macros. The development boards can > be quite affordable (< USD 300 for the Xilinx Ultra96). There are also > now open-source SoC HDL implementations - including MMUs, FPUs, and > peripherals like PWM and UART - for the conventional FPGA market. These > can run at mid-1990's clock rates - slow by modern standards but still > quite usable. In my eyes it's better to make a driver not possible to enable out of the box than offering to enable it while it most probably won't be used. People who configure their own kernel and distribution kernel maintainers will thank you. (Well ok, they won't notice, so they won't actually tell you, but anyhow.) I'm a member of the Debian kernel team and I see how many config items there are that are not explicitly handled for the various different kernel images. If they were restricted using COMPILE_TEST to just be possible to enable on machines where it is known (or at least likely) to make sense that would help. Also when I do a kernel version bump for a machine with a tailored kernel running (say) on an ARM/i.MX SoC, I could more easily be careful about the relevant changes when doing oldconfig if I were not asked about a whole bunch of drivers that are sure to be irrelevant. And if someone synthesizes this sifive PWM into an FPGA that is connected to an OMAP, changing depends RISCV || COMPILE_TEST to something like depends RISCV || MACH_OMAP || COMPILE_TEST is a relatively low effort. (Or we introduce a symbol that tells that the machine has an FPGA and based on that enable the sifive pwm driver.) And if a single person comes up saying they need that driver on OMAP I happily support such a change. > So or vendor-specific IP blocks that are unlikely to ever be reused by > anyone else, like the NVIDIA or TI PWM blocks, maybe there's still some > justification for COMPILE_TEST. But for drivers for open-source, > SoC-independent peripheral IP blocks - or even for proprietary IP blocks > from library vendors like Synopsys or Cadence - like this PWM driver, we > shouldn't add unnecessary architecture, SoC vendor, or COMPILE_TEST > Kconfig dependencies. They will just force users to patch the kernel or > enable COMPILE_TEST for kernels that are actually meant to run on real > hardware. I understand that downside. I just think that people who synthesize an open source core into their machine and then run Linux on it are very likely easily able to patch the Kconfig file to enable the needed drivers and tell us. Also if you compare the number of people who hit this problem to the number of people to have to decide if they need PWM_SIFIVE and don't need it, I guess there will be an at least big two-digit factor between them. And as soon as this OMAP machine with the FPGA becomes mainstream enough that tutorials pop up in the web that give you a copy&paste template to put the sifive pwm into it, I expect the dependency is already fixed. Yes, there is no big harm if you enable this driver and don't need it. But there are hundreds of drivers, and together they do hurt. Also if you support a "newbie" configuring their first kernel, this is much less frightening and gives a much better impression if at least most of the presented options matter. Also it is easier to pick your pwm driver if it's presented in a list of ten driver than if the list has 100 items. There are reasons for both approaches and we need to weight the respective interests. In my eyes it's clear which is the better approach, but obviously YMMV. So if you choose to not restrict the kconfig symbol, at least do it consciously with the arguments for the other side in mind. And please also consider that your position is subjective because you're a kernel developer and personally don't care much if configuring a kernel is difficult to a newcomer. Best regards Uwe
On Tue, Jan 15, 2019 at 11:00:46PM +0100, Uwe Kleine-König wrote: > Hello, > > On Fri, Jan 11, 2019 at 01:52:44PM +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 | 10 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 257 insertions(+) > > create mode 100644 drivers/pwm/pwm-sifive.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index a8f47df..3bcaf6a 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -380,6 +380,16 @@ 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 > > I'd say add: > > depends on MACH_SIFIVE || COMPILE_TEST > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.) > > > + 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..7fee809 > > --- /dev/null > > +++ b/drivers/pwm/pwm-sifive.c > > @@ -0,0 +1,246 @@ > > +// 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 > > I wonder if such an instance should be only a single PWM instead of > four. Then you were more flexible with the period lengths (using > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode. I thought this IP only allowed a single period for all PWM channels in the IP. If so, splitting this into four different devices is going to complicate things because you'd have to coordinate between all four as to which period is currently set. > > +struct sifive_pwm_device { > > + struct pwm_chip chip; > > + struct notifier_block notifier; > > + struct clk *clk; > > + void __iomem *regs; > > + unsigned int approx_period; > > + unsigned int real_period; > > +}; > > I'd call this pwm_sifive_ddata. The prefix because the driver is called > pwm-sifive and ddata because this is driver data and not a device. I don't think there's a need to have an extra suffix. Just call this sifive_pwm or pwm_sifive. There's no ambiguity in that name and it's short and crisp. > > + if (state->enabled) > > + sifive_pwm_get_state(chip, dev, state); > > @Thierry: Should we bless this correction of state? I'm not sure I understand why this correction is necessary. Is it okay to request a state to be applied and when we're not able to set that state we just set anything as close as possible? Sounds a bit risky to me. What if somebody wants to use this in a case where precision matters? Now, if you're saying that there can't be such cases and we should support this, then why restrict the state correction to when the PWM is enabled? What's wrong with correcting it in either case? Thierry
On Thu, Jan 17, 2019 at 09:19:56AM +0100, Uwe Kleine-König wrote: > Hello Paul, > > On Wed, Jan 16, 2019 at 11:29:35AM -0800, Paul Walmsley wrote: > > COMPILE_TEST made slightly more sense before the broad availability of > > open-source soft cores, SoC integration, and IP; and before powerful, > > inexpensive FPGAs and SoCs with FPGA fabrics were common. > > > > Even back then, COMPILE_TEST was starting to look questionable. IP blocks > > from CPU- and SoC vendor-independent libraries, like DesignWare and the > > Cadence IP libraries, were integrated on SoCs across varying CPU > > architectures. (Fortunately, looking at the tree, subsystem maintainers > > with DesignWare drivers seem to have largely avoided adding architecture > > or SoC-specific Kconfig restrictions there - and thus have also avoided > > the need for COMPILE_TEST.) If an unnecessary architecture Kconfig > > dependency was added for a leaf driver, that Kconfig line would either > > need to be patched out by hand, or if present, COMPILE_TEST would need to > > be enabled. > > > > This was less of a problem then. There were very few FPGA Linux users, > > and they were mostly working on internal proprietary projects. FPGAs that > > could run Linux at a reasonable speed, including MMUs and FPUs, were > > expensive or were missing good tool support. So most FPGA Linux > > development was restricted to ASIC vendors - the Samsungs, Qualcomms, > > NVIDIAs of the world - for prototyping, and those FPGA platforms never > > made it outside those companies. > > > > The situation is different now. The major FPGA vendors have inexpensive > > FPGA SoCs with full-featured CPU hard macros. The development boards can > > be quite affordable (< USD 300 for the Xilinx Ultra96). There are also > > now open-source SoC HDL implementations - including MMUs, FPUs, and > > peripherals like PWM and UART - for the conventional FPGA market. These > > can run at mid-1990's clock rates - slow by modern standards but still > > quite usable. > > In my eyes it's better to make a driver not possible to enable out of > the box than offering to enable it while it most probably won't be used. This might sound like a good idea in general, but it's also something that is pretty much impossible to do. There's just no heuristic that would allow you to determine with a sufficient degree of probability that a driver won't be needed. Most of the PCI drivers that are installed on my workstation aren't used, and I would venture to say there are a lot of drivers that aren't used in, say, 95% of installations. That doesn't mean that we should somehow make these drivers difficult to install, or require someone to patch the kernel to build them. > People who configure their own kernel and distribution kernel > maintainers will thank you. (Well ok, they won't notice, so they won't > actually tell you, but anyhow.) I'm a member of the Debian kernel team > and I see how many config items there are that are not explicitly > handled for the various different kernel images. If they were restricted > using COMPILE_TEST to just be possible to enable on machines where it is > known (or at least likely) to make sense that would help. Also when I do > a kernel version bump for a machine with a tailored kernel running (say) > on an ARM/i.MX SoC, I could more easily be careful about the relevant > changes when doing oldconfig if I were not asked about a whole bunch of > drivers that are sure to be irrelevant. I think the important thing here is that the driver is "default n". If you're a developer and build your own kernels, you're pretty likely to know already if a new kernel that you're installing contains that new driver that you've been working on or waiting for. In that case you can easily just enable it manually rather than go through make oldconfig and wait for it to show up. As for distribution kernel maintainers, are they really still building a lot of different kernels? If so, it sounds like most of the multi- platform efforts have been in vain. I would imagine that if, as a distribution kernel team, you'd want to carefully evaluate for every driver whether or not you'd want to include it. I would also imagine that you'd want to provide your users with the most flexible kernel possible, so that if they do have a system with an FPGA that you'd make it possible for them to use pwm-sifive if they choose to synthesize it. If you really want to create custom builds tailored to a single chip or board, it's going to be a fair amount of work anyway. I've occasionally done that in the past and usually just resorted to starting from an allnoconfig configuration and then working my way up from there. As for daily work as a developer, when I transition between kernel versions, or from one linux-next to another, I typically end up doing the manual equivalent of: $ yes '' | make oldconfig if I know that I'm not interested in any new features. But I also often just look at what's new because it's interesting to see what's been going on elsewhere. Thierry
On Mon, Jan 21, 2019 at 12:30:39PM +0100, Thierry Reding wrote: > On Tue, Jan 15, 2019 at 11:00:46PM +0100, Uwe Kleine-König wrote: > > Hello, > > > > On Fri, Jan 11, 2019 at 01:52:44PM +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 | 10 ++ > > > drivers/pwm/Makefile | 1 + > > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 257 insertions(+) > > > create mode 100644 drivers/pwm/pwm-sifive.c > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > index a8f47df..3bcaf6a 100644 > > > --- a/drivers/pwm/Kconfig > > > +++ b/drivers/pwm/Kconfig > > > @@ -380,6 +380,16 @@ 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 > > > > I'd say add: > > > > depends on MACH_SIFIVE || COMPILE_TEST > > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.) > > > > > + 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..7fee809 > > > --- /dev/null > > > +++ b/drivers/pwm/pwm-sifive.c > > > @@ -0,0 +1,246 @@ > > > +// 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 > > > > I wonder if such an instance should be only a single PWM instead of > > four. Then you were more flexible with the period lengths (using > > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode. > > I thought this IP only allowed a single period for all PWM channels in > the IP. If so, splitting this into four different devices is going to > complicate things because you'd have to coordinate between all four as > to which period is currently set. Take a look at the above link, figure 6 is depicting how this IP works (page 92 of the pdf). If you have pwmzerocmp=0 (and pwmcmpXgang=0) the four outputs (pwmcmpXgpio) are independant and the counter only resets on overflow of pwms. Then you have 4 outputs that can have their duty_cycle (but not period) set individually. So period is restricted to a count of clk cycles that is a power of two. With pwmzerocmp=1, pwmcmp0 resets the counter with the following effects: - pwmcmp0gpio is always 0 (bad?) - more finegrained control over the period length as the restriction to powers of two is gone (good) The other three output can then either be used as 3 PWM outputs with the more flexible (but identical) period or only pwmcmp1gpio is used as a single output (my favourite) and with pwmcmp2, pwmcmp3 and pwmcmp2gang=1 the output pwmcmp2gpio can be used as a secondary output to implement stuff that was called "complementary mode" or "Push-pull mode" by Claudiu Beznea. > > > +struct sifive_pwm_device { > > > + struct pwm_chip chip; > > > + struct notifier_block notifier; > > > + struct clk *clk; > > > + void __iomem *regs; > > > + unsigned int approx_period; > > > + unsigned int real_period; > > > +}; > > > > I'd call this pwm_sifive_ddata. The prefix because the driver is called > > pwm-sifive and ddata because this is driver data and not a device. > > I don't think there's a need to have an extra suffix. Just call this > sifive_pwm or pwm_sifive. There's no ambiguity in that name and it's > short and crisp. fine for me if "_device" goes away. > > > + if (state->enabled) > > > + sifive_pwm_get_state(chip, dev, state); > > > > @Thierry: Should we bless this correction of state? > > I'm not sure I understand why this correction is necessary. Is it okay > to request a state to be applied and when we're not able to set that > state we just set anything as close as possible? Sounds a bit risky to > me. What if somebody wants to use this in a case where precision > matters? There is always rounding involved. Where should we draw a line then? Consider a parent clk running at 1000000001 Hz. Can you provide a duty cycle of 1 ps? Of if the clk runs at 500000000 Hz, I can implement even periods, but not the uneven. Should the driver fail if an uneven period is requested? What should a user do if setting .duty_cycle = 55, .period = 100, fails? Assume the pwm can set only even duty_cycles? Or maybe duty_cycle and period must be dividable by 3? Or maybe only periods that are a power of two are possible? To get both, an API that can actually be worked with and that provides precision, the driver needs to be allowed to round (preferably in some defined way that is used for all devices) and we need a function to determine the result without actually configuring. That's how it works in the clk_* universe. There is clk_round_rate and clk_set_rate and the explicit condition that clk_set_rate(clk, somerate) has the same effect as clk_set_rate(clk, clk_round_rate(clk, somerate)) and after one of them we have clk_get_rate(clk) = clk_round_rate(clk, somerate)) > Now, if you're saying that there can't be such cases and we should > support this, then why restrict the state correction to when the PWM is > enabled? What's wrong with correcting it in either case? Either the correction should be done always or never. Best regards Uwe
Hello Thierry, On Mon, Jan 21, 2019 at 12:54:55PM +0100, Thierry Reding wrote: > On Thu, Jan 17, 2019 at 09:19:56AM +0100, Uwe Kleine-König wrote: > > On Wed, Jan 16, 2019 at 11:29:35AM -0800, Paul Walmsley wrote: > > > COMPILE_TEST made slightly more sense before the broad availability of > > > open-source soft cores, SoC integration, and IP; and before powerful, > > > inexpensive FPGAs and SoCs with FPGA fabrics were common. > > > > > > Even back then, COMPILE_TEST was starting to look questionable. IP blocks > > > from CPU- and SoC vendor-independent libraries, like DesignWare and the > > > Cadence IP libraries, were integrated on SoCs across varying CPU > > > architectures. (Fortunately, looking at the tree, subsystem maintainers > > > with DesignWare drivers seem to have largely avoided adding architecture > > > or SoC-specific Kconfig restrictions there - and thus have also avoided > > > the need for COMPILE_TEST.) If an unnecessary architecture Kconfig > > > dependency was added for a leaf driver, that Kconfig line would either > > > need to be patched out by hand, or if present, COMPILE_TEST would need to > > > be enabled. > > > > > > This was less of a problem then. There were very few FPGA Linux users, > > > and they were mostly working on internal proprietary projects. FPGAs that > > > could run Linux at a reasonable speed, including MMUs and FPUs, were > > > expensive or were missing good tool support. So most FPGA Linux > > > development was restricted to ASIC vendors - the Samsungs, Qualcomms, > > > NVIDIAs of the world - for prototyping, and those FPGA platforms never > > > made it outside those companies. > > > > > > The situation is different now. The major FPGA vendors have inexpensive > > > FPGA SoCs with full-featured CPU hard macros. The development boards can > > > be quite affordable (< USD 300 for the Xilinx Ultra96). There are also > > > now open-source SoC HDL implementations - including MMUs, FPUs, and > > > peripherals like PWM and UART - for the conventional FPGA market. These > > > can run at mid-1990's clock rates - slow by modern standards but still > > > quite usable. > > > > In my eyes it's better to make a driver not possible to enable out of > > the box than offering to enable it while it most probably won't be used. > > This might sound like a good idea in general, but it's also something > that is pretty much impossible to do. There's just no heuristic that > would allow you to determine with a sufficient degree of probability > that a driver won't be needed. Most of the PCI drivers that are > installed on my workstation aren't used, and I would venture to say > there are a lot of drivers that aren't used in, say, 95% of > installations. That doesn't mean that we should somehow make these > drivers difficult to install, or require someone to patch the kernel > to build them. If there is a PCI card that can be plugged into your machine, the corresponding driver should be selectable for the matching kernel. The case here is different though. We're talking about a piece of hardware that up to now only exists in a riscv machine (IIUC). Yes, I'm aware that the design is publicly available, still I think this driver should not be available for a person configuring a kernel for their x86 machine. When you (or someone else) comes around claiming that they have a real use for this driver on a non-riscv architecture I support expanding the dependency accordingly. What I want to make aware of is that being able to enable a driver comes at a (small) cost. Using a too broad dependency is quite usual because the person who introduces a given symbol usually doesn't have to think long if it should be enabled for a given kernel config or not; so it's "only" other people's time that is wasted. > > People who configure their own kernel and distribution kernel > > maintainers will thank you. (Well ok, they won't notice, so they won't > > actually tell you, but anyhow.) I'm a member of the Debian kernel team > > and I see how many config items there are that are not explicitly > > handled for the various different kernel images. If they were restricted > > using COMPILE_TEST to just be possible to enable on machines where it is > > known (or at least likely) to make sense that would help. Also when I do > > a kernel version bump for a machine with a tailored kernel running (say) > > on an ARM/i.MX SoC, I could more easily be careful about the relevant > > changes when doing oldconfig if I were not asked about a whole bunch of > > drivers that are sure to be irrelevant. > > I think the important thing here is that the driver is "default n". If > you're a developer and build your own kernels, you're pretty likely to > know already if a new kernel that you're installing contains that new > driver that you've been working on or waiting for. If you are a developer waiting for your driver you also would not miss it because it was only selectable for riscv but you're the first who synthesized it for an arm machine. So there is only little damage. > In that case you can easily just enable it manually rather than go > through make oldconfig and wait for it to show up. > > As for distribution kernel maintainers, are they really still building a > lot of different kernels? In the Debian kernel there are as of now 39 kernel images. Some of them only differ by stuff that is irrelevant for driver selection (like rt). But apart from these there is still mostly only a single image available for a given machine because multi-platform efforts are not good enough to allow cross-architecture kernels. Or kernels that can handle both big and little endian. > If so, it sounds like most of the multi-platform efforts have been in > vain. I would imagine that if, as a distribution kernel team, you'd > want to carefully evaluate for every driver whether or not you'd want > to include it. The Debian distro kernel I'm running has a .config file with 7317 config items (grep CONFIG /boot/config-$(uname -r) | wc -l). 1922 of them are disabled (grep is\ not\ set /boot/config-$(uname -r) | wc -l). If you find the time to only go through the 1922 options that are disabled for this amd64 kernel, tell me, I provide you the config file. > I would also imagine that you'd want to provide your users with the > most flexible kernel possible, so that if they do have a system with > an FPGA that you'd make it possible for them to use pwm-sifive if they > choose to synthesize it. I would today probably choose to not enable pwm-sifive for Debian on a non-riscv arch kernel because nobody told to have a use for it and the cost of enabling the driver is that it takes hard disk space for several thousand machines without any use. And given that we here have the knowledge that up to now PWM_SIFIVE=[ym] is only usable on riscv, I think we should put that into the condition that makes the driver selectable. It's not hard for a distro maintainer to find the same information; say it takes 5 minutes (that works here because the driver under discussion has a link to the reference manual in the header). Multiply that by the count of drivers. > If you really want to create custom builds tailored to a single chip or > board, it's going to be a fair amount of work anyway. Ah right, this is a hard job, so it doesn't make a difference when we make it a little bit harder? > I've occasionally done that in the past and usually just resorted to > starting from an allnoconfig configuration and then working my way up > from there. > > As for daily work as a developer, when I transition between kernel > versions, or from one linux-next to another, I typically end up doing > the manual equivalent of: > > $ yes '' | make oldconfig I usually start with $(make oldconfig) without the yes part. But when I get 10th question in a row that is completely irrelevant for my target machine because it doesn't have the graphics core that is found only on Samsung ARM SoCs or the nand controllers that can only be found on some powerpc machines I'm annoyed and I press and hold the Enter key. I'm well aware that I'm missing some interesting stuff then, though, which is sad. > if I know that I'm not interested in any new features. But I also often > just look at what's new because it's interesting to see what's been > going on elsewhere. The kernel config as a way to see what is going on elsewhere is a use case that is broken by my preferred approach. Be warned that you already miss stuff today if you only look there. Best regards Uwe
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index a8f47df..3bcaf6a 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -380,6 +380,16 @@ 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 + 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..7fee809 --- /dev/null +++ b/drivers/pwm/pwm-sifive.c @@ -0,0 +1,246 @@ +// 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 REG_PWMCFG 0x0 +#define REG_PWMCOUNT 0x8 +#define REG_PWMS 0x10 +#define REG_PWMCMP0 0x20 + +/* PWMCFG fields */ +#define BIT_PWM_SCALE 0 +#define BIT_PWM_STICKY 8 +#define BIT_PWM_ZERO_ZMP 9 +#define BIT_PWM_DEGLITCH 10 +#define BIT_PWM_EN_ALWAYS 12 +#define BIT_PWM_EN_ONCE 13 +#define BIT_PWM0_CENTER 16 +#define BIT_PWM0_GANG 24 +#define BIT_PWM0_IP 28 + +#define SIZE_PWMCMP 4 +#define MASK_PWM_SCALE 0xf + +struct sifive_pwm_device { + struct pwm_chip chip; + struct notifier_block notifier; + struct clk *clk; + void __iomem *regs; + unsigned int approx_period; + unsigned int real_period; +}; + +static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c) +{ + return container_of(c, struct sifive_pwm_device, chip); +} + +static void sifive_pwm_get_state(struct pwm_chip *chip, struct pwm_device *dev, + struct pwm_state *state) +{ + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip); + u32 duty; + + duty = readl(pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP); + + state->period = pwm->real_period; + state->duty_cycle = ((u64)duty * pwm->real_period) >> 16; + state->polarity = PWM_POLARITY_INVERSED; + state->enabled = duty > 0; +} + +static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev, + struct pwm_state *state) +{ + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip); + unsigned int duty_cycle; + u32 frac; + + if (state->polarity != PWM_POLARITY_INVERSED) + return -EINVAL; + + duty_cycle = state->duty_cycle; + if (!state->enabled) + duty_cycle = 0; + + frac = div_u64((u64)duty_cycle << 16, state->period); + frac = min(frac, 0xFFFFU); + + writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP); + + if (state->enabled) + sifive_pwm_get_state(chip, dev, state); + + return 0; +} + +static const struct pwm_ops sifive_pwm_ops = { + .get_state = sifive_pwm_get_state, + .apply = sifive_pwm_apply, + .owner = THIS_MODULE, +}; + +static struct pwm_device *sifive_pwm_xlate(struct pwm_chip *chip, + const struct of_phandle_args *args) +{ + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip); + struct pwm_device *dev; + + if (args->args[0] >= chip->npwm) + return ERR_PTR(-EINVAL); + + dev = pwm_request_from_chip(chip, args->args[0], NULL); + if (IS_ERR(dev)) + return dev; + + /* The period cannot be changed on a per-PWM basis */ + dev->args.period = pwm->real_period; + dev->args.polarity = PWM_POLARITY_NORMAL; + if (args->args[1] & PWM_POLARITY_INVERSED) + dev->args.polarity = PWM_POLARITY_INVERSED; + + return dev; +} + +static void sifive_pwm_update_clock(struct sifive_pwm_device *pwm, + unsigned long rate) +{ + /* (1 << (16+scale)) * 10^9/rate = real_period */ + unsigned long scale_pow = (pwm->approx_period * (u64)rate) / 1000000000; + int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf); + + writel((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE), + pwm->regs + REG_PWMCFG); + + /* As scale <= 15 the shift operation cannot overflow. */ + pwm->real_period = div64_ul(1000000000ULL << (16 + scale), rate); + dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period); +} + +static int sifive_pwm_clock_notifier(struct notifier_block *nb, + unsigned long event, void *data) +{ + struct clk_notifier_data *ndata = data; + struct sifive_pwm_device *pwm = + container_of(nb, struct sifive_pwm_device, notifier); + + if (event == POST_RATE_CHANGE) + sifive_pwm_update_clock(pwm, ndata->new_rate); + + return NOTIFY_OK; +} + +static int sifive_pwm_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *node = pdev->dev.of_node; + struct sifive_pwm_device *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 = &sifive_pwm_ops; + chip->of_xlate = sifive_pwm_xlate; + chip->of_pwm_n_cells = 2; + chip->base = -1; + chip->npwm = 4; + + ret = of_property_read_u32(node, "sifive,approx-period-ns", + &pwm->approx_period); + if (ret < 0) { + dev_err(dev, "Unable to read sifive,approx-period from DTS\n"); + return ret; + } + + 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 = sifive_pwm_clock_notifier; + ret = clk_notifier_register(pwm->clk, &pwm->notifier); + if (ret) { + dev_err(dev, "failed to register clock notifier: %d\n", ret); + clk_disable_unprepare(pwm->clk); + return ret; + } + + /* Initialize PWM config */ + sifive_pwm_update_clock(pwm, clk_get_rate(pwm->clk)); + + ret = pwmchip_add(chip); + if (ret < 0) { + dev_err(dev, "cannot register PWM: %d\n", ret); + clk_notifier_unregister(pwm->clk, &pwm->notifier); + clk_disable_unprepare(pwm->clk); + return ret; + } + + platform_set_drvdata(pdev, pwm); + dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm); + + return 0; +} + +static int sifive_pwm_remove(struct platform_device *dev) +{ + struct sifive_pwm_device *pwm = platform_get_drvdata(dev); + int ret; + + ret = pwmchip_remove(&pwm->chip); + clk_notifier_unregister(pwm->clk, &pwm->notifier); + clk_disable_unprepare(pwm->clk); + return ret; +} + +static const struct of_device_id sifive_pwm_of_match[] = { + { .compatible = "sifive,pwm0" }, + { .compatible = "sifive,fu540-c000-pwm" }, + {}, +}; +MODULE_DEVICE_TABLE(of, sifive_pwm_of_match); + +static struct platform_driver sifive_pwm_driver = { + .probe = sifive_pwm_probe, + .remove = sifive_pwm_remove, + .driver = { + .name = "pwm-sifive", + .of_match_table = sifive_pwm_of_match, + }, +}; +module_platform_driver(sifive_pwm_driver); + +MODULE_DESCRIPTION("SiFive PWM driver"); +MODULE_LICENSE("GPL v2");