* [PATCH 0/2] PWM support for HiFive Unleashed @ 2019-01-11 8:22 Yash Shah 2019-01-11 8:22 ` [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller Yash Shah 2019-01-11 8:22 ` [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM Yash Shah 0 siblings, 2 replies; 20+ messages in thread From: Yash Shah @ 2019-01-11 8:22 UTC (permalink / raw) To: palmer, linux-pwm, linux-riscv Cc: mark.rutland, devicetree, linux-kernel, robh+dt, sachin.ghadi, Yash Shah, thierry.reding, paul.walmsley This patch series adds a PWM driver and DT documentation for HiFive Unleashed board. The patches are mostly based on Wesley's patch. Yash Shah (2): pwm: sifive: Add DT documentation for SiFive PWM Controller pwm: sifive: Add a driver for SiFive SoC PWM .../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++ drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++ 4 files changed, 294 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt create mode 100644 drivers/pwm/pwm-sifive.c -- 1.9.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller 2019-01-11 8:22 [PATCH 0/2] PWM support for HiFive Unleashed Yash Shah @ 2019-01-11 8:22 ` Yash Shah 2019-01-15 20:11 ` Uwe Kleine-König 2019-01-11 8:22 ` [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM Yash Shah 1 sibling, 1 reply; 20+ messages in thread From: Yash Shah @ 2019-01-11 8:22 UTC (permalink / raw) To: palmer, linux-pwm, linux-riscv Cc: mark.rutland, devicetree, linux-kernel, robh+dt, sachin.ghadi, Yash Shah, thierry.reding, paul.walmsley DT documentation for PWM controller added with updated compatible string. Signed-off-by: Wesley W. Terpstra <wesley@sifive.com> [Atish: Compatible string update] Signed-off-by: Atish Patra <atish.patra@wdc.com> Signed-off-by: Yash Shah <yash.shah@sifive.com> --- .../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt new file mode 100644 index 0000000..e0fc22a --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt @@ -0,0 +1,37 @@ +SiFive PWM controller + +Unlike most other PWM controllers, the SiFive PWM controller currently only +supports one period for all channels in the PWM. This is set globally in DTS. +The period also has significant restrictions on the values it can achieve, +which the driver rounds to the nearest achievable frequency. + +Required properties: +- compatible: Please refer to sifive-blocks-ip-versioning.txt +- reg: physical base address and length of the controller's registers +- clocks: Should contain a clock identifier for the PWM's parent clock. +- #pwm-cells: Should be 2. + The first cell is the PWM channel number + The second cell is the PWM polarity +- sifive,approx-period-ns: the driver will get as close to this period as it can +- interrupts: one interrupt per PWM channel + +PWM RTL that corresponds to the IP block version numbers can be found +here: + +https://github.com/sifive/sifive-blocks/tree/master/src/main/scala/devices/pwm + +Further information on the format of the IP +block-specific version numbers can be found in +Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt + +Examples: + +pwm: pwm@10020000 { + compatible = "sifive,fu540-c000-pwm","sifive,pwm0"; + reg = <0x0 0x10020000 0x0 0x1000>; + clocks = <&tlclk>; + interrupt-parent = <&plic>; + interrupts = <42 43 44 45>; + #pwm-cells = <2>; + sifive,approx-period-ns = <1000000>; +}; -- 1.9.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller 2019-01-11 8:22 ` [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller Yash Shah @ 2019-01-15 20:11 ` Uwe Kleine-König 2019-01-16 9:21 ` Yash Shah 0 siblings, 1 reply; 20+ messages in thread From: Uwe Kleine-König @ 2019-01-15 20:11 UTC (permalink / raw) To: Yash Shah Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel, robh+dt, sachin.ghadi, thierry.reding, paul.walmsley, linux-riscv Hello, this is v3, right? It is helpful to point this out to ease reviewing. On Fri, Jan 11, 2019 at 01:52:43PM +0530, Yash Shah wrote: > DT documentation for PWM controller added with updated compatible > string. Not sure what was updated here. But assuming this is compared to v2 this is not a helpful info in the commit log. > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com> > [Atish: Compatible string update] > Signed-off-by: Atish Patra <atish.patra@wdc.com> > Signed-off-by: Yash Shah <yash.shah@sifive.com> > --- > .../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt > new file mode 100644 > index 0000000..e0fc22a > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt > @@ -0,0 +1,37 @@ > +SiFive PWM controller > + > +Unlike most other PWM controllers, the SiFive PWM controller currently only > +supports one period for all channels in the PWM. This is set globally in DTS. > +The period also has significant restrictions on the values it can achieve, > +which the driver rounds to the nearest achievable frequency. > + > +Required properties: > +- compatible: Please refer to sifive-blocks-ip-versioning.txt While the description was too verbose in v2, this is too short. You should at least mention something like "sifive,pwmX" and "sifive,$cpuname-pwm" (or how ever that scheme works). > +- reg: physical base address and length of the controller's registers > +- clocks: Should contain a clock identifier for the PWM's parent clock. > +- #pwm-cells: Should be 2. > + The first cell is the PWM channel number > + The second cell is the PWM polarity I'd drop these two lines and refer to bindings/pwm/pwm.txt instead. > +- sifive,approx-period-ns: the driver will get as close to this period as it can As already said for v2: I'd drop "approx-". > +- interrupts: one interrupt per PWM channel > + > +PWM RTL that corresponds to the IP block version numbers can be found > +here: > + > +https://github.com/sifive/sifive-blocks/tree/master/src/main/scala/devices/pwm > + > +Further information on the format of the IP > +block-specific version numbers can be found in > +Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt > + > +Examples: > + > +pwm: pwm@10020000 { > + compatible = "sifive,fu540-c000-pwm","sifive,pwm0"; > + reg = <0x0 0x10020000 0x0 0x1000>; > + clocks = <&tlclk>; > + interrupt-parent = <&plic>; > + interrupts = <42 43 44 45>; > + #pwm-cells = <2>; > + sifive,approx-period-ns = <1000000>; > +}; > -- > 1.9.1 > > -- 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller 2019-01-15 20:11 ` Uwe Kleine-König @ 2019-01-16 9:21 ` Yash Shah 2019-01-21 11:20 ` Thierry Reding 0 siblings, 1 reply; 20+ messages in thread From: Yash Shah @ 2019-01-16 9:21 UTC (permalink / raw) To: Uwe Kleine-König Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt, linux-kernel, robh+dt, Sachin Ghadi, Thierry Reding, Paul Walmsley, linux-riscv On Wed, Jan 16, 2019 at 1:41 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > this is v3, right? It is helpful to point this out to ease reviewing. Yes, it is v3. Will take care of this in v4. > > On Fri, Jan 11, 2019 at 01:52:43PM +0530, Yash Shah wrote: > > DT documentation for PWM controller added with updated compatible > > string. > > Not sure what was updated here. But assuming this is compared to v2 this > is not a helpful info in the commit log. Ok, will remove the 'updated compatible string' part. > > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com> > > [Atish: Compatible string update] > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > Signed-off-by: Yash Shah <yash.shah@sifive.com> > > --- > > .../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt > > > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt > > new file mode 100644 > > index 0000000..e0fc22a > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt > > @@ -0,0 +1,37 @@ > > +SiFive PWM controller > > + > > +Unlike most other PWM controllers, the SiFive PWM controller currently only > > +supports one period for all channels in the PWM. This is set globally in DTS. > > +The period also has significant restrictions on the values it can achieve, > > +which the driver rounds to the nearest achievable frequency. > > + > > +Required properties: > > +- compatible: Please refer to sifive-blocks-ip-versioning.txt > > While the description was too verbose in v2, this is too short. You > should at least mention something like "sifive,pwmX" and > "sifive,$cpuname-pwm" (or how ever that scheme works). Will mention the above. > > > +- reg: physical base address and length of the controller's registers > > +- clocks: Should contain a clock identifier for the PWM's parent clock. > > +- #pwm-cells: Should be 2. > > + The first cell is the PWM channel number > > + The second cell is the PWM polarity > > I'd drop these two lines and refer to bindings/pwm/pwm.txt instead. Will be done. > > > +- sifive,approx-period-ns: the driver will get as close to this period as it can > > As already said for v2: I'd drop "approx-". Sure, will be done. > > > +- interrupts: one interrupt per PWM channel > > + > > +PWM RTL that corresponds to the IP block version numbers can be found > > +here: > > + > > +https://github.com/sifive/sifive-blocks/tree/master/src/main/scala/devices/pwm > > + > > +Further information on the format of the IP > > +block-specific version numbers can be found in > > +Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt > > + > > +Examples: > > + > > +pwm: pwm@10020000 { > > + compatible = "sifive,fu540-c000-pwm","sifive,pwm0"; > > + reg = <0x0 0x10020000 0x0 0x1000>; > > + clocks = <&tlclk>; > > + interrupt-parent = <&plic>; > > + interrupts = <42 43 44 45>; > > + #pwm-cells = <2>; > > + sifive,approx-period-ns = <1000000>; > > +}; > > -- > > 1.9.1 > > > > > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | Thanks for the comments. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller 2019-01-16 9:21 ` Yash Shah @ 2019-01-21 11:20 ` Thierry Reding 0 siblings, 0 replies; 20+ messages in thread From: Thierry Reding @ 2019-01-21 11:20 UTC (permalink / raw) To: Yash Shah Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt, linux-kernel, Sachin Ghadi, robh+dt, Paul Walmsley, Uwe Kleine-König, linux-riscv [-- Attachment #1.1: Type: text/plain, Size: 3106 bytes --] On Wed, Jan 16, 2019 at 02:51:31PM +0530, Yash Shah wrote: > On Wed, Jan 16, 2019 at 1:41 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > Hello, > > > > this is v3, right? It is helpful to point this out to ease reviewing. > > Yes, it is v3. Will take care of this in v4. > > > > > On Fri, Jan 11, 2019 at 01:52:43PM +0530, Yash Shah wrote: > > > DT documentation for PWM controller added with updated compatible > > > string. > > > > Not sure what was updated here. But assuming this is compared to v2 this > > is not a helpful info in the commit log. > > Ok, will remove the 'updated compatible string' part. > > > > > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com> > > > [Atish: Compatible string update] > > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > > Signed-off-by: Yash Shah <yash.shah@sifive.com> > > > --- > > > .../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++++++++++++++++++++ > > > 1 file changed, 37 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt > > > > > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt > > > new file mode 100644 > > > index 0000000..e0fc22a > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt > > > @@ -0,0 +1,37 @@ > > > +SiFive PWM controller > > > + > > > +Unlike most other PWM controllers, the SiFive PWM controller currently only > > > +supports one period for all channels in the PWM. This is set globally in DTS. > > > +The period also has significant restrictions on the values it can achieve, > > > +which the driver rounds to the nearest achievable frequency. > > > + > > > +Required properties: > > > +- compatible: Please refer to sifive-blocks-ip-versioning.txt > > > > While the description was too verbose in v2, this is too short. You > > should at least mention something like "sifive,pwmX" and > > "sifive,$cpuname-pwm" (or how ever that scheme works). > > Will mention the above. > > > > > > +- reg: physical base address and length of the controller's registers > > > +- clocks: Should contain a clock identifier for the PWM's parent clock. > > > +- #pwm-cells: Should be 2. > > > + The first cell is the PWM channel number > > > + The second cell is the PWM polarity > > > > I'd drop these two lines and refer to bindings/pwm/pwm.txt instead. > > Will be done. I don't think you can do that. You're omitting the second cell representing the period, so this is different from the standard binding and therefore needs to be explicit. That said, given the rest of the discussion in the other threads, maybe it no longer makes sense to set the period on the whole block, but rather just note in the binding that all PWMs need to run at the same period. If the driver already refuses to apply incompatible periods, your users are going to notice that they've got the DT wrong. This would have the advantage that you can use the standard bindings. Thierry [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-11 8:22 [PATCH 0/2] PWM support for HiFive Unleashed Yash Shah 2019-01-11 8:22 ` [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller Yash Shah @ 2019-01-11 8:22 ` Yash Shah 2019-01-15 15:27 ` Christoph Hellwig 2019-01-15 22:00 ` Uwe Kleine-König 1 sibling, 2 replies; 20+ messages in thread From: Yash Shah @ 2019-01-11 8:22 UTC (permalink / raw) To: palmer, linux-pwm, linux-riscv Cc: mark.rutland, devicetree, linux-kernel, robh+dt, sachin.ghadi, Yash Shah, thierry.reding, paul.walmsley 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 + 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"); -- 1.9.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-11 8:22 ` [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM Yash Shah @ 2019-01-15 15:27 ` Christoph Hellwig 2019-01-15 22:00 ` Uwe Kleine-König 1 sibling, 0 replies; 20+ messages in thread From: Christoph Hellwig @ 2019-01-15 15:27 UTC (permalink / raw) To: Yash Shah Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel, sachin.ghadi, robh+dt, thierry.reding, paul.walmsley, linux-riscv 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> _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-11 8:22 ` [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM Yash Shah 2019-01-15 15:27 ` Christoph Hellwig @ 2019-01-15 22:00 ` Uwe Kleine-König 2019-01-16 11:10 ` Yash Shah 2019-01-21 11:30 ` Thierry Reding 1 sibling, 2 replies; 20+ messages in thread From: Uwe Kleine-König @ 2019-01-15 22:00 UTC (permalink / raw) To: Yash Shah Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel, robh+dt, sachin.ghadi, thierry.reding, paul.walmsley, linux-riscv 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 -- 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-15 22:00 ` Uwe Kleine-König @ 2019-01-16 11:10 ` Yash Shah 2019-01-16 16:46 ` Uwe Kleine-König 2019-01-21 11:30 ` Thierry Reding 1 sibling, 1 reply; 20+ messages in thread From: Yash Shah @ 2019-01-16 11:10 UTC (permalink / raw) To: Uwe Kleine-König Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt, linux-kernel, robh+dt, Sachin Ghadi, Thierry Reding, Paul Walmsley, linux-riscv 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/ | _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-16 11:10 ` Yash Shah @ 2019-01-16 16:46 ` Uwe Kleine-König 2019-01-16 17:18 ` Paul Walmsley 2019-01-17 6:45 ` Yash Shah 0 siblings, 2 replies; 20+ messages in thread From: Uwe Kleine-König @ 2019-01-16 16:46 UTC (permalink / raw) To: Yash Shah Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt, linux-kernel, robh+dt, Sachin Ghadi, Thierry Reding, Paul Walmsley, linux-riscv 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 -- 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-16 16:46 ` Uwe Kleine-König @ 2019-01-16 17:18 ` Paul Walmsley 2019-01-16 17:28 ` Uwe Kleine-König 2019-01-17 6:45 ` Yash Shah 1 sibling, 1 reply; 20+ messages in thread From: Paul Walmsley @ 2019-01-16 17:18 UTC (permalink / raw) To: Uwe Kleine-König Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt, linux-kernel, robh+dt, Sachin Ghadi, Yash Shah, Thierry Reding, Paul Walmsley, linux-riscv [-- Attachment #1: Type: text/plain, Size: 2256 bytes --] 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 [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-16 17:18 ` Paul Walmsley @ 2019-01-16 17:28 ` Uwe Kleine-König 2019-01-16 19:29 ` Paul Walmsley 0 siblings, 1 reply; 20+ messages in thread From: Uwe Kleine-König @ 2019-01-16 17:28 UTC (permalink / raw) To: Paul Walmsley Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt, linux-kernel, robh+dt, Sachin Ghadi, Yash Shah, Thierry Reding, linux-riscv 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 -- 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-16 17:28 ` Uwe Kleine-König @ 2019-01-16 19:29 ` Paul Walmsley 2019-01-17 8:19 ` Uwe Kleine-König 0 siblings, 1 reply; 20+ messages in thread From: Paul Walmsley @ 2019-01-16 19:29 UTC (permalink / raw) To: Uwe Kleine-König Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt, linux-kernel, robh+dt, Sachin Ghadi, Yash Shah, Thierry Reding, Paul Walmsley, linux-riscv [-- Attachment #1: Type: text/plain, Size: 5407 bytes --] 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 [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-16 19:29 ` Paul Walmsley @ 2019-01-17 8:19 ` Uwe Kleine-König 2019-01-21 11:54 ` Thierry Reding 0 siblings, 1 reply; 20+ messages in thread From: Uwe Kleine-König @ 2019-01-17 8:19 UTC (permalink / raw) To: Paul Walmsley Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt, linux-kernel, robh+dt, Sachin Ghadi, Yash Shah, Thierry Reding, linux-riscv 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 -- 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-17 8:19 ` Uwe Kleine-König @ 2019-01-21 11:54 ` Thierry Reding 2019-01-21 15:11 ` Uwe Kleine-König 0 siblings, 1 reply; 20+ messages in thread From: Thierry Reding @ 2019-01-21 11:54 UTC (permalink / raw) To: Uwe Kleine-König Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt, linux-kernel, Sachin Ghadi, Yash Shah, robh+dt, Paul Walmsley, linux-riscv [-- Attachment #1.1: Type: text/plain, Size: 5188 bytes --] 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 [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-21 11:54 ` Thierry Reding @ 2019-01-21 15:11 ` Uwe Kleine-König 0 siblings, 0 replies; 20+ messages in thread From: Uwe Kleine-König @ 2019-01-21 15:11 UTC (permalink / raw) To: Thierry Reding Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt, linux-kernel, Sachin Ghadi, Yash Shah, robh+dt, kernel, Paul Walmsley, linux-riscv 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 -- 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-16 16:46 ` Uwe Kleine-König 2019-01-16 17:18 ` Paul Walmsley @ 2019-01-17 6:45 ` Yash Shah 2019-01-17 7:28 ` Uwe Kleine-König 1 sibling, 1 reply; 20+ messages in thread From: Yash Shah @ 2019-01-17 6:45 UTC (permalink / raw) To: Uwe Kleine-König Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt, linux-kernel, Sachin Ghadi, robh+dt, Thierry Reding, Paul Walmsley, linux-riscv 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 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-17 6:45 ` Yash Shah @ 2019-01-17 7:28 ` Uwe Kleine-König 0 siblings, 0 replies; 20+ messages in thread From: Uwe Kleine-König @ 2019-01-17 7:28 UTC (permalink / raw) To: Yash Shah Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt, linux-kernel, Sachin Ghadi, robh+dt, Thierry Reding, Paul Walmsley, 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 -- 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-15 22:00 ` Uwe Kleine-König 2019-01-16 11:10 ` Yash Shah @ 2019-01-21 11:30 ` Thierry Reding 2019-01-21 13:23 ` Uwe Kleine-König 1 sibling, 1 reply; 20+ messages in thread From: Thierry Reding @ 2019-01-21 11:30 UTC (permalink / raw) To: Uwe Kleine-König Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel, sachin.ghadi, Yash Shah, robh+dt, paul.walmsley, linux-riscv [-- Attachment #1.1: Type: text/plain, Size: 4236 bytes --] 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 [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM 2019-01-21 11:30 ` Thierry Reding @ 2019-01-21 13:23 ` Uwe Kleine-König 0 siblings, 0 replies; 20+ messages in thread From: Uwe Kleine-König @ 2019-01-21 13:23 UTC (permalink / raw) To: Thierry Reding Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel, sachin.ghadi, Yash Shah, robh+dt, paul.walmsley, linux-riscv 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 -- 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2019-01-21 15:11 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-11 8:22 [PATCH 0/2] PWM support for HiFive Unleashed Yash Shah 2019-01-11 8:22 ` [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller Yash Shah 2019-01-15 20:11 ` Uwe Kleine-König 2019-01-16 9:21 ` Yash Shah 2019-01-21 11:20 ` Thierry Reding 2019-01-11 8:22 ` [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM Yash Shah 2019-01-15 15:27 ` Christoph Hellwig 2019-01-15 22:00 ` Uwe Kleine-König 2019-01-16 11:10 ` Yash Shah 2019-01-16 16:46 ` Uwe Kleine-König 2019-01-16 17:18 ` Paul Walmsley 2019-01-16 17:28 ` Uwe Kleine-König 2019-01-16 19:29 ` Paul Walmsley 2019-01-17 8:19 ` Uwe Kleine-König 2019-01-21 11:54 ` Thierry Reding 2019-01-21 15:11 ` Uwe Kleine-König 2019-01-17 6:45 ` Yash Shah 2019-01-17 7:28 ` Uwe Kleine-König 2019-01-21 11:30 ` Thierry Reding 2019-01-21 13:23 ` Uwe Kleine-König
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).