[PATCH 0/2] PWM support for HiFive Unleashed

[PATCH 0/2] PWM support for HiFive Unleashed

[Yash Shah]

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

[PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller

[Yash Shah]

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

Re: [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller

[Uwe Kleine-König]

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

Re: [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller

[Yash Shah]

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

Re: [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller

[Thierry Reding]

[-- 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

[PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Yash Shah]

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

Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Christoph Hellwig]

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

Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Uwe Kleine-König]

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

Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Yash Shah]

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

Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Uwe Kleine-König]

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

Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Paul Walmsley]

[-- 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

Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Uwe Kleine-König]

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

Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Paul Walmsley]

[-- 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

Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Uwe Kleine-König]

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

Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Thierry Reding]

[-- 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

Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Uwe Kleine-König]

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

Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Yash Shah]

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

Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Uwe Kleine-König]

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

Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Thierry Reding]

[-- 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

Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Uwe Kleine-König]

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