[RFC v2 0/2] PWM support for HiFive Unleashed

[RFC v2 0/2] PWM support for HiFive Unleashed

[Yash Shah]

This patch series adds PWM drivers and DT documentation for 
HiFive Unleashed board. The patches are mostly based on Wesley's patch.
V2 of this patchset incorporates below items pointed out in v1.

V2 changed from V1:
  1. Remove inclusion of dt-bindings/pwm/pwm.h
  2. Remove artificial alignments
  3. Replace ioread32/iowrite32 with readl/writel
  4. Remove camelcase
  5. Change dev_info to dev_dbg for unnecessary log
  6. Correct typo in driver name
  7. Remove use of of_match_ptr macro
  8. Update the DT compatible strings and Add reference to a common
     versioning document

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         |  44 ++++
 drivers/pwm/Kconfig                                |  10 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-sifive.c                           | 229 +++++++++++++++++++++
 4 files changed, 284 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
 create mode 100644 drivers/pwm/pwm-sifive.c

-- 
1.9.1

[RFC v2 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         | 44 ++++++++++++++++++++++
 1 file changed, 44 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..250d8ee
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
@@ -0,0 +1,44 @@
+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: should be something similar to "sifive,<chip>-pwm" for
+	      the PWM as integrated on a particular chip, and
+	      "sifive,pwm<version>" for the general PWM IP block
+	      programming model. Supported compatible strings are:
+	      "sifive,fu540-c000-pwm" for the SiFive PWM v0 as
+	      integrated onto the SiFive FU540 chip, and "sifive,pwm0"
+	      for the SiFive PWM v0 IP block with no chip integration
+	      tweaks.
+- reg: physical base address and length of the controller's registers
+- clocks: The frequency the controller runs at
+- #pwm-cells: Should be 2.
+  The first cell is the PWM channel number
+  The second cell is the PWM polarity
+- sifive,approx-period: 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 = <1000000>;
+};
-- 
1.9.1

[RFC v2 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 | 229 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 240 insertions(+)
 create mode 100644 drivers/pwm/pwm-sifive.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 27e5dd4..da85557 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -378,6 +378,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..26913b6
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017-2018 SiFive
+ */
+#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 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;
+
+	duty_cycle = state->duty_cycle;
+	if (!state->enabled)
+		duty_cycle = 0;
+
+	frac = ((u64)duty_cycle << 16) / state->period;
+	frac = min(frac, 0xFFFFU);
+
+	writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
+
+	if (state->enabled) {
+		state->period = pwm->real_period;
+		state->duty_cycle = ((u64)frac * pwm->real_period) >> 16;
+	}
+
+	return 0;
+}
+
+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 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);
+
+	pwm->real_period = (1000000000ULL << (16 + scale)) / rate;
+}
+
+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",
+				   &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)) {
+		dev_err(dev, "Unable to find controller clock\n");
+		return PTR_ERR(pwm->clk);
+	}
+
+	/* 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);
+		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);
+		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);
+
+	clk_notifier_unregister(pwm->clk, &pwm->notifier);
+	return pwmchip_remove(&pwm->chip);
+}
+
+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

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

[Uwe Kleine-König]

On Fri, Dec 14, 2018 at 11:50:41AM +0530, Yash Shah wrote:
> 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         | 44 ++++++++++++++++++++++
>  1 file changed, 44 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..250d8ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> @@ -0,0 +1,44 @@
> +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: should be something similar to "sifive,<chip>-pwm" for
> +	      the PWM as integrated on a particular chip, and
> +	      "sifive,pwm<version>" for the general PWM IP block
> +	      programming model. Supported compatible strings are:
> +	      "sifive,fu540-c000-pwm" for the SiFive PWM v0 as
> +	      integrated onto the SiFive FU540 chip, and "sifive,pwm0"
> +	      for the SiFive PWM v0 IP block with no chip integration
> +	      tweaks.
> +- reg: physical base address and length of the controller's registers
> +- clocks: The frequency the controller runs at

This is unusual and the example below lists a clock phandle (which is
the common thing), so I guess the description is just wrong.

> +- #pwm-cells: Should be 2.
> +  The first cell is the PWM channel number
> +  The second cell is the PWM polarity
> +- sifive,approx-period: the driver will get as close to this period as it can

What is the unit? I'd drop "approx", that the driver might not be able
to exactly hit the specified period is (IMHO) obvious and doesn't need
to be mentioned in the property name.

> +- 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 = <1000000>;
> +};

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Uwe Kleine-König]

On Fri, Dec 14, 2018 at 11:50:42AM +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 | 229 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 240 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sifive.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 27e5dd4..da85557 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -378,6 +378,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..26913b6
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017-2018 SiFive

If there is a publically available reference manual, please add a link
to it here.

> + */
> +#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 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;
> +
> +	duty_cycle = state->duty_cycle;
> +	if (!state->enabled)
> +		duty_cycle = 0;

@Thierry: You see, this driver is cheating in the same way that I
suggested to implement for imx.

> +
> +	frac = ((u64)duty_cycle << 16) / state->period;

You must not use / to divide an u64 (unless you're on a 64 bit arch).

> +	frac = min(frac, 0xFFFFU);

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?

You should also check polarity (and fail if it's !=
PWM_POLARITY_INVERSED?).

If state->duty_cycle == state->period, we end up with frac = 0xffff.
Does that mean the chip cannot output 100%?

> +	writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> +
> +	if (state->enabled) {
> +		state->period = pwm->real_period;
> +		state->duty_cycle = ((u64)frac * pwm->real_period) >> 16;
> +	}

Is this the expected behaviour of .apply to update *state? (I think it's
a good idea, but I think it misses official blessing.)

> +	return 0;
> +}

How does a period start with this PWM hardware. The expected behaviour
would be to start with low level for duty_cycle and then high for the
rest of the period (given that the polarity is always inversed). Is this
what the hardware actually does?

If the duty cycle changes, is the currently running period completed
before the new setting gets active? If yes, .apply is supposed to block
until the new setting is active.

> +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 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;

A single space before the = please.

> +	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);

What happens with the output if you don't set the BIT_PWM_EN_ALWAYS bit?

> +	pwm->real_period = (1000000000ULL << (16 + scale)) / rate;

I suggest commenting this assignment with something like: "As scale <=
15 the shift operation cannot overflow." You must use div64_ul for
dividing an unsigned long long variable. Can it happen that the result
is too big to be hold by read_period (which is an unsigned int only)?

Maybe add a dev_dbg with the new real_period here.

> +}
> +
> +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",
> +				   &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)) {
> +		dev_err(dev, "Unable to find controller clock\n");

Please don't emit an error message if PTR_ERR(pwm->clk) is
-EPROBE_DEFER.

> +		return PTR_ERR(pwm->clk);
> +	}
> +
> +	/* 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);
> +		return ret;
> +	}
> +
> +	/* Initialize PWM config */
> +	sifive_pwm_update_clock(pwm, clk_get_rate(pwm->clk));

You're supposed to call clk_get_rate only after you enabled the clk.

> +	ret = pwmchip_add(chip);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot register PWM: %d\n", ret);
> +		clk_notifier_unregister(pwm->clk, &pwm->notifier);
> +		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);
> +
> +	clk_notifier_unregister(pwm->clk, &pwm->notifier);
> +	return pwmchip_remove(&pwm->chip);

In probe you setup the clk notifier before calling pwmchip_add. So it's
a good habit to do it the other way round in .remove.

> +}

You're not using the irq that according to the dt binding is required?!

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Rob Herring]

On Fri, Dec 14, 2018 at 11:50:41AM +0530, Yash Shah wrote:
> 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         | 44 ++++++++++++++++++++++
>  1 file changed, 44 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..250d8ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> @@ -0,0 +1,44 @@
> +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: should be something similar to "sifive,<chip>-pwm" for
> +	      the PWM as integrated on a particular chip, and
> +	      "sifive,pwm<version>" for the general PWM IP block
> +	      programming model. Supported compatible strings are:
> +	      "sifive,fu540-c000-pwm" for the SiFive PWM v0 as
> +	      integrated onto the SiFive FU540 chip, and "sifive,pwm0"
> +	      for the SiFive PWM v0 IP block with no chip integration
> +	      tweaks.

This should reference the common doc Paul has written and not re-explain 
the versioning scheme again.

> +- reg: physical base address and length of the controller's registers
> +- clocks: The frequency the controller runs at
> +- #pwm-cells: Should be 2.
> +  The first cell is the PWM channel number
> +  The second cell is the PWM polarity
> +- sifive,approx-period: the driver will get as close to this period as it can

Needs a unit suffix as defined in property-units.txt

> +- 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 = <1000000>;
> +};
> -- 
> 1.9.1
> 

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

[Yash Shah]

On Tue, Dec 18, 2018 at 10:50 PM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Dec 14, 2018 at 11:50:41AM +0530, Yash Shah wrote:
> > 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         | 44 ++++++++++++++++++++++
> >  1 file changed, 44 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..250d8ee
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > @@ -0,0 +1,44 @@
> > +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: should be something similar to "sifive,<chip>-pwm" for
> > +           the PWM as integrated on a particular chip, and
> > +           "sifive,pwm<version>" for the general PWM IP block
> > +           programming model. Supported compatible strings are:
> > +           "sifive,fu540-c000-pwm" for the SiFive PWM v0 as
> > +           integrated onto the SiFive FU540 chip, and "sifive,pwm0"
> > +           for the SiFive PWM v0 IP block with no chip integration
> > +           tweaks.
>
> This should reference the common doc Paul has written and not re-explain
> the versioning scheme again.

Ok sure.

>
> > +- reg: physical base address and length of the controller's registers
> > +- clocks: The frequency the controller runs at
> > +- #pwm-cells: Should be 2.
> > +  The first cell is the PWM channel number
> > +  The second cell is the PWM polarity
> > +- sifive,approx-period: the driver will get as close to this period as it can
>
> Needs a unit suffix as defined in property-units.txt

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 = <1000000>;
> > +};
> > --
> > 1.9.1
> >

Thanks for the comments!

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

[Yash Shah]

On Tue, Dec 18, 2018 at 2:46 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Fri, Dec 14, 2018 at 11:50:41AM +0530, Yash Shah wrote:
> > 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         | 44 ++++++++++++++++++++++
> >  1 file changed, 44 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..250d8ee
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > @@ -0,0 +1,44 @@
> > +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: should be something similar to "sifive,<chip>-pwm" for
> > +           the PWM as integrated on a particular chip, and
> > +           "sifive,pwm<version>" for the general PWM IP block
> > +           programming model. Supported compatible strings are:
> > +           "sifive,fu540-c000-pwm" for the SiFive PWM v0 as
> > +           integrated onto the SiFive FU540 chip, and "sifive,pwm0"
> > +           for the SiFive PWM v0 IP block with no chip integration
> > +           tweaks.
> > +- reg: physical base address and length of the controller's registers
> > +- clocks: The frequency the controller runs at
>
> This is unusual and the example below lists a clock phandle (which is
> the common thing), so I guess the description is just wrong.

You are right, I will correct the description.

>
> > +- #pwm-cells: Should be 2.
> > +  The first cell is the PWM channel number
> > +  The second cell is the PWM polarity
> > +- sifive,approx-period: the driver will get as close to this period as it can
>
> What is the unit? I'd drop "approx", that the driver might not be able
> to exactly hit the specified period is (IMHO) obvious and doesn't need
> to be mentioned in the property name.

The unit is nanoseconds. Will add the unit suffix to the property name.

>
> > +- 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 = <1000000>;
> > +};
>
> Best regards
> Uwe

Thanks for the comments!

>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Yash Shah]

On Tue, Dec 18, 2018 at 3:42 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Fri, Dec 14, 2018 at 11:50:42AM +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 | 229 +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 240 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-sifive.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 27e5dd4..da85557 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -378,6 +378,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..26913b6
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,229 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2017-2018 SiFive
>
> If there is a publically available reference manual, please add a link
> to it here.

Ok will add the link to the reference manual.

>
> > + */
> > +#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 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;
> > +
> > +     duty_cycle = state->duty_cycle;
> > +     if (!state->enabled)
> > +             duty_cycle = 0;
>
> @Thierry: You see, this driver is cheating in the same way that I
> suggested to implement for imx.
>
> > +
> > +     frac = ((u64)duty_cycle << 16) / state->period;
>
> You must not use / to divide an u64 (unless you're on a 64 bit arch).

Will use div_u64().

>
> > +     frac = min(frac, 0xFFFFU);
>
> 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?

Right.

>
> You should also check polarity (and fail if it's !=
> PWM_POLARITY_INVERSED?).

Will add the check for polarity.

>
> If state->duty_cycle == state->period, we end up with frac = 0xffff.
> Does that mean the chip cannot output 100%?

No, it does not mean that. The chip can output 100%

>
> > +     writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > +
> > +     if (state->enabled) {
> > +             state->period = pwm->real_period;
> > +             state->duty_cycle = ((u64)frac * pwm->real_period) >> 16;
> > +     }
>
> Is this the expected behaviour of .apply to update *state? (I think it's
> a good idea, but I think it misses official blessing.)

Ok, will update the *state by calling get_state() from .apply

>
> > +     return 0;
> > +}
>
> How does a period start with this PWM hardware. The expected behaviour
> would be to start with low level for duty_cycle and then high for the
> rest of the period (given that the polarity is always inversed). Is this
> what the hardware actually does?

Yes, Correct.

>
> If the duty cycle changes, is the currently running period completed
> before the new setting gets active? If yes, .apply is supposed to block
> until the new setting is active.

No, it is not the case.

>
> > +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 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;
>
> A single space before the = please.

Sure.

>
> > +     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);
>
> What happens with the output if you don't set the BIT_PWM_EN_ALWAYS bit?

If BIT_PWM_EN_ALWAYS is set, the PWM counter increments continuously.
If not set, PWM counter will be disabled. There won't be PWM output unless
BIT_PWM_EN_ONCE is set. In that case it will generate single PWM cycle and stop.

>
> > +     pwm->real_period = (1000000000ULL << (16 + scale)) / rate;
>
> I suggest commenting this assignment with something like: "As scale <=
> 15 the shift operation cannot overflow." You must use div64_ul for
> dividing an unsigned long long variable. Can it happen that the result
> is too big to be hold by read_period (which is an unsigned int only)?

Ok. Will add that comment and also use div64_ul for division.
Regarding the result, I don't think so it will be big enough to
overflow read_period.

>
> Maybe add a dev_dbg with the new real_period here.

Sure, will add it.

>
> > +}
> > +
> > +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",
> > +                                &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)) {
> > +             dev_err(dev, "Unable to find controller clock\n");
>
> Please don't emit an error message if PTR_ERR(pwm->clk) is
> -EPROBE_DEFER.

Will add an "if" check.

>
> > +             return PTR_ERR(pwm->clk);
> > +     }
> > +
> > +     /* 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);
> > +             return ret;
> > +     }
> > +
> > +     /* Initialize PWM config */
> > +     sifive_pwm_update_clock(pwm, clk_get_rate(pwm->clk));
>
> You're supposed to call clk_get_rate only after you enabled the clk.

Will fix this.

>
> > +     ret = pwmchip_add(chip);
> > +     if (ret < 0) {
> > +             dev_err(dev, "cannot register PWM: %d\n", ret);
> > +             clk_notifier_unregister(pwm->clk, &pwm->notifier);
> > +             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);
> > +
> > +     clk_notifier_unregister(pwm->clk, &pwm->notifier);
> > +     return pwmchip_remove(&pwm->chip);
>
> In probe you setup the clk notifier before calling pwmchip_add. So it's
> a good habit to do it the other way round in .remove.

Will change the sequence.

>
> > +}
>
> You're not using the irq that according to the dt binding is required?!

Yes, currently there is no use.

>
> Best regards
> Uwe

Thanks for the comments!

>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |