[PATCH v6 0/2] PWM support for HiFive Unleashed

[PATCH v6 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.

v6
- Remove the global property 'sifive,period-ns'
- Implement free and request callbacks to maintain user counts.
- Add user_count member to struct pwm_sifive_ddata
- Allow period change only if user_count is one
- Add pwm_sifive_enable function to enable/disable PWM
- Change calculation logic of frac (in pwm_sifive_apply)
- Remove state correction
- Remove pwm_sifive_xlate function
- Clock to be enabled only when PWM is enabled
- Other minor fixes

v5
- Correct the order of compatible string properties
- PWM state correction to be done always
- Other minor fixes based upon feedback on v4

v4
- Rename the property sifive,approx-period-ns to sifive,period-ns
- Rename macros with appropriate names
- Remove unused macros
- Rename struct sifive_pwm_device to struct pwm_sifive_ddata
- Rename function prefix as per driver name
- Set deglitch bit before changing output waveform.
- Other minor fixes based upon feedback on v3

v3
- Add a link to the reference manaul
- Use appropriate apis for division operation
- Add check for polarity
- Enable clk before calling clk_get_rate
- Other minor fixes based upon feedback on v2

V2 changed from V1:
- Remove inclusion of dt-bindings/pwm/pwm.h
- Remove artificial alignments
- Replace ioread32/iowrite32 with readl/writel
- Remove camelcase
- Change dev_info to dev_dbg for unnecessary log
- Correct typo in driver name
- Remove use of of_match_ptr macro
- 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         |  30 ++
 drivers/pwm/Kconfig                                |  11 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-sifive.c                           | 311 +++++++++++++++++++++
 4 files changed, 353 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
 create mode 100644 drivers/pwm/pwm-sifive.c

-- 
1.9.1

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

[Yash Shah]

DT documentation for PWM controller added.

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         | 30 ++++++++++++++++++++++
 1 file changed, 30 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..3b9c64c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
@@ -0,0 +1,30 @@
+SiFive PWM controller
+
+Unlike most other PWM controllers, the SiFive PWM controller currently only
+supports one period for all channels in the PWM. All PWMs need to run at
+the same period. The period also has significant restrictions on the values
+it can achieve, which the driver rounds to the nearest achievable period.
+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
+
+Required properties:
+- compatible: Should be "sifive,$socname-pwm" and "sifive,pwmX".
+  Please refer to sifive-blocks-ip-versioning.txt for details.
+- 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 3. See pwm.txt in this directory
+  for a description of the cell format.
+- interrupts: one interrupt per PWM channel
+
+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 = <3>;
+};
-- 
1.9.1

[PATCH v6 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      |  11 ++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-sifive.c | 311 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 323 insertions(+)
 create mode 100644 drivers/pwm/pwm-sifive.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index a8f47df..4a61d1a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -380,6 +380,17 @@ config PWM_SAMSUNG
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-samsung.
 
+config PWM_SIFIVE
+	tristate "SiFive PWM support"
+	depends on OF
+	depends on COMMON_CLK
+	depends on RISCV || COMPILE_TEST
+	help
+	  Generic PWM framework driver for SiFive SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-sifive.
+
 config PWM_SPEAR
 	tristate "STMicroelectronics SPEAr PWM support"
 	depends on PLAT_SPEAR
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9c676a0..30089ca 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
 obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
+obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
 obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
new file mode 100644
index 0000000..c0eb90e
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,311 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017-2018 SiFive
+ * For SiFive's PWM IP block documentation please refer Chapter 14 of
+ * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf
+ */
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+/* Register offsets */
+#define PWM_SIFIVE_PWMCFG		0x0
+#define PWM_SIFIVE_PWMCOUNT		0x8
+#define PWM_SIFIVE_PWMS			0x10
+#define PWM_SIFIVE_PWMCMP0		0x20
+
+/* PWMCFG fields */
+#define PWM_SIFIVE_PWMCFG_SCALE		0
+#define PWM_SIFIVE_PWMCFG_STICKY	8
+#define PWM_SIFIVE_PWMCFG_ZERO_CMP	9
+#define PWM_SIFIVE_PWMCFG_DEGLITCH	10
+#define PWM_SIFIVE_PWMCFG_EN_ALWAYS	12
+#define PWM_SIFIVE_PWMCFG_EN_ONCE	13
+#define PWM_SIFIVE_PWMCFG_CENTER	16
+#define PWM_SIFIVE_PWMCFG_GANG		24
+#define PWM_SIFIVE_PWMCFG_IP		28
+
+/* PWM_SIFIVE_SIZE_PWMCMP is used to calculate offset for pwmcmpX registers */
+#define PWM_SIFIVE_SIZE_PWMCMP		4
+#define PWM_SIFIVE_CMPWIDTH		16
+
+struct pwm_sifive_ddata {
+	struct pwm_chip	chip;
+	struct notifier_block notifier;
+	struct clk *clk;
+	void __iomem *regs;
+	unsigned int real_period;
+	int user_count;
+};
+
+static inline
+struct pwm_sifive_ddata *pwm_sifive_chip_to_ddata(struct pwm_chip *c)
+{
+	return container_of(c, struct pwm_sifive_ddata, chip);
+}
+
+static int pwm_sifive_request(struct pwm_chip *chip, struct pwm_device *dev)
+{
+	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
+
+	pwm->user_count++;
+
+	return 0;
+}
+
+static void pwm_sifive_free(struct pwm_chip *chip, struct pwm_device *dev)
+{
+	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
+
+	pwm->user_count--;
+}
+
+static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
+				    unsigned long rate)
+{
+	/* (1 << (16+scale)) * 10^9/rate = real_period */
+	unsigned long scale_pow =
+			div64_ul(pwm->real_period * (u64)rate, NSEC_PER_SEC);
+	int scale = clamp(ilog2(scale_pow) - PWM_SIFIVE_CMPWIDTH, 0, 0xf);
+
+	writel((1 << PWM_SIFIVE_PWMCFG_EN_ALWAYS) | (scale <<
+	       PWM_SIFIVE_PWMCFG_SCALE), pwm->regs + PWM_SIFIVE_PWMCFG);
+
+	/* As scale <= 15 the shift operation cannot overflow. */
+	pwm->real_period = div64_ul(1000000000ULL << (PWM_SIFIVE_CMPWIDTH +
+				    scale), rate);
+	dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
+}
+
+static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *dev,
+				 struct pwm_state *state)
+{
+	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
+	u32 duty;
+	int val;
+
+	duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm *
+		     PWM_SIFIVE_SIZE_PWMCMP);
+
+	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
+	state->enabled = (val & BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS)) > 0;
+
+	val &= 0x0F;
+	pwm->real_period = div64_ul(1000000000ULL << (PWM_SIFIVE_CMPWIDTH +
+				    val), clk_get_rate(pwm->clk));
+
+	state->period = pwm->real_period;
+	state->duty_cycle = ((u64)duty * pwm->real_period) >>
+			    PWM_SIFIVE_CMPWIDTH;
+	state->polarity = PWM_POLARITY_INVERSED;
+}
+
+static int pwm_sifive_enable(struct pwm_chip *chip, struct pwm_device *dev,
+			     bool enable)
+{
+	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
+	u32 val;
+	int ret;
+
+	if (enable) {
+		ret = clk_enable(pwm->clk);
+		if (ret) {
+			dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
+			return ret;
+		}
+	}
+
+	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
+
+	if (enable)
+		val |= BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS);
+	else
+		val &= ~BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS);
+
+	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
+
+	if (!enable)
+		clk_disable(pwm->clk);
+
+	return 0;
+}
+
+static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
+			    struct pwm_state *state)
+{
+	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
+	unsigned int duty_cycle;
+	u32 frac, val;
+	struct pwm_state cur_state;
+	bool enabled;
+	int ret;
+
+	pwm_get_state(dev, &cur_state);
+	enabled = cur_state.enabled;
+
+	if (state->polarity != PWM_POLARITY_INVERSED)
+		return -EINVAL;
+
+	if (state->period != cur_state.period) {
+		if (pwm->user_count != 1)
+			return -EINVAL;
+		pwm->real_period = state->period;
+		pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
+	}
+
+	if (!state->enabled && enabled) {
+		ret = pwm_sifive_enable(chip, dev, false);
+		if (ret)
+			return ret;
+		enabled = false;
+	}
+
+	duty_cycle = state->duty_cycle;
+	frac = div_u64((u64)duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) +
+		       (1 << PWM_SIFIVE_CMPWIDTH) / 2, state->period);
+	/* The hardware cannot generate a 100% duty cycle */
+	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
+
+	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
+	val |= BIT(PWM_SIFIVE_PWMCFG_DEGLITCH);
+	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
+
+	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm *
+	       PWM_SIFIVE_SIZE_PWMCMP);
+
+	val &= ~BIT(PWM_SIFIVE_PWMCFG_DEGLITCH);
+	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
+
+	if (state->enabled != enabled) {
+		ret = pwm_sifive_enable(chip, dev, state->enabled);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct pwm_ops pwm_sifive_ops = {
+	.request = pwm_sifive_request,
+	.free = pwm_sifive_free,
+	.get_state = pwm_sifive_get_state,
+	.apply = pwm_sifive_apply,
+	.owner = THIS_MODULE,
+};
+
+static int pwm_sifive_clock_notifier(struct notifier_block *nb,
+				     unsigned long event, void *data)
+{
+	struct clk_notifier_data *ndata = data;
+	struct pwm_sifive_ddata *pwm =
+		container_of(nb, struct pwm_sifive_ddata, notifier);
+
+	if (event == POST_RATE_CHANGE)
+		pwm_sifive_update_clock(pwm, ndata->new_rate);
+
+	return NOTIFY_OK;
+}
+
+static int pwm_sifive_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pwm_sifive_ddata *pwm;
+	struct pwm_chip *chip;
+	struct resource *res;
+	int ret;
+
+	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	chip = &pwm->chip;
+	chip->dev = dev;
+	chip->ops = &pwm_sifive_ops;
+	chip->of_pwm_n_cells = 3;
+	chip->base = -1;
+	chip->npwm = 4;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pwm->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pwm->regs)) {
+		dev_err(dev, "Unable to map IO resources\n");
+		return PTR_ERR(pwm->regs);
+	}
+
+	pwm->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pwm->clk)) {
+		if (PTR_ERR(pwm->clk) != -EPROBE_DEFER)
+			dev_err(dev, "Unable to find controller clock\n");
+		return PTR_ERR(pwm->clk);
+	}
+
+	ret = clk_prepare_enable(pwm->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clock for pwm: %d\n", ret);
+		return ret;
+	}
+
+	/* Watch for changes to underlying clock frequency */
+	pwm->notifier.notifier_call = pwm_sifive_clock_notifier;
+	ret = clk_notifier_register(pwm->clk, &pwm->notifier);
+	if (ret) {
+		dev_err(dev, "failed to register clock notifier: %d\n", ret);
+		goto disable_clk;
+	}
+
+	ret = pwmchip_add(chip);
+	if (ret < 0) {
+		dev_err(dev, "cannot register PWM: %d\n", ret);
+		goto unregister_clk;
+	}
+
+	if (!pwm_is_enabled(pwm->chip.pwms))
+		clk_disable(pwm->clk);
+
+	platform_set_drvdata(pdev, pwm);
+	dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
+
+	return 0;
+
+unregister_clk:
+	clk_notifier_unregister(pwm->clk, &pwm->notifier);
+disable_clk:
+	clk_disable_unprepare(pwm->clk);
+
+	return ret;
+}
+
+static int pwm_sifive_remove(struct platform_device *dev)
+{
+	struct pwm_sifive_ddata *pwm = platform_get_drvdata(dev);
+	int ret;
+
+	ret = pwmchip_remove(&pwm->chip);
+	clk_notifier_unregister(pwm->clk, &pwm->notifier);
+	if (!pwm_is_enabled(pwm->chip.pwms))
+		clk_disable(pwm->clk);
+	clk_unprepare(pwm->clk);
+	return ret;
+}
+
+static const struct of_device_id pwm_sifive_of_match[] = {
+	{ .compatible = "sifive,pwm0" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, pwm_sifive_of_match);
+
+static struct platform_driver pwm_sifive_driver = {
+	.probe = pwm_sifive_probe,
+	.remove = pwm_sifive_remove,
+	.driver = {
+		.name = "pwm-sifive",
+		.of_match_table = pwm_sifive_of_match,
+	},
+};
+module_platform_driver(pwm_sifive_driver);
+
+MODULE_DESCRIPTION("SiFive PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

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

[Uwe Kleine-König]

Hello,

On Wed, Feb 13, 2019 at 02:56:18PM +0530, Yash Shah wrote:
> Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> 
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
>  drivers/pwm/Kconfig      |  11 ++
>  drivers/pwm/Makefile     |   1 +
>  drivers/pwm/pwm-sifive.c | 311 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 323 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sifive.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a8f47df..4a61d1a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -380,6 +380,17 @@ config PWM_SAMSUNG
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-samsung.
>  
> +config PWM_SIFIVE
> +	tristate "SiFive PWM support"
> +	depends on OF
> +	depends on COMMON_CLK
> +	depends on RISCV || COMPILE_TEST
> +	help
> +	  Generic PWM framework driver for SiFive SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-sifive.
> +
>  config PWM_SPEAR
>  	tristate "STMicroelectronics SPEAr PWM support"
>  	depends on PLAT_SPEAR
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9c676a0..30089ca 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
>  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
>  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> +obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
>  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
>  obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
>  obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> new file mode 100644
> index 0000000..c0eb90e
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017-2018 SiFive
> + * For SiFive's PWM IP block documentation please refer Chapter 14 of
> + * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> + */
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +/* Register offsets */
> +#define PWM_SIFIVE_PWMCFG		0x0
> +#define PWM_SIFIVE_PWMCOUNT		0x8
> +#define PWM_SIFIVE_PWMS			0x10
> +#define PWM_SIFIVE_PWMCMP0		0x20
> +
> +/* PWMCFG fields */
> +#define PWM_SIFIVE_PWMCFG_SCALE		0
> +#define PWM_SIFIVE_PWMCFG_STICKY	8
> +#define PWM_SIFIVE_PWMCFG_ZERO_CMP	9
> +#define PWM_SIFIVE_PWMCFG_DEGLITCH	10
> +#define PWM_SIFIVE_PWMCFG_EN_ALWAYS	12

PWM_SIFIVE_PWMCFG_EN_ALWAYS is always used as

	BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS)

so defining this as BIT(12) directly makes some expressions below easier
to read.

> +#define PWM_SIFIVE_PWMCFG_EN_ONCE	13
> +#define PWM_SIFIVE_PWMCFG_CENTER	16
> +#define PWM_SIFIVE_PWMCFG_GANG		24
> +#define PWM_SIFIVE_PWMCFG_IP		28
> +
> +/* PWM_SIFIVE_SIZE_PWMCMP is used to calculate offset for pwmcmpX registers */
> +#define PWM_SIFIVE_SIZE_PWMCMP		4
> +#define PWM_SIFIVE_CMPWIDTH		16
> +
> +struct pwm_sifive_ddata {
> +	struct pwm_chip	chip;
> +	struct notifier_block notifier;
> +	struct clk *clk;
> +	void __iomem *regs;
> +	unsigned int real_period;
> +	int user_count;
> +};
> +
> +static inline
> +struct pwm_sifive_ddata *pwm_sifive_chip_to_ddata(struct pwm_chip *c)
> +{
> +	return container_of(c, struct pwm_sifive_ddata, chip);
> +}
> +
> +static int pwm_sifive_request(struct pwm_chip *chip, struct pwm_device *dev)
> +{
> +	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> +
> +	pwm->user_count++;
> +
> +	return 0;
> +}
> +
> +static void pwm_sifive_free(struct pwm_chip *chip, struct pwm_device *dev)
> +{
> +	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> +
> +	pwm->user_count--;
> +}
> +
> +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> +				    unsigned long rate)
> +{
> +	/* (1 << (16+scale)) * 10^9/rate = real_period */

Maybe you want to mention here the relation between 16 and
PWM_SIFIVE_CMPWIDTH.

> +	unsigned long scale_pow =
> +			div64_ul(pwm->real_period * (u64)rate, NSEC_PER_SEC);
> +	int scale = clamp(ilog2(scale_pow) - PWM_SIFIVE_CMPWIDTH, 0, 0xf);
> +
> +	writel((1 << PWM_SIFIVE_PWMCFG_EN_ALWAYS) | (scale <<
> +	       PWM_SIFIVE_PWMCFG_SCALE), pwm->regs + PWM_SIFIVE_PWMCFG);

I think this would be better readable with the newline after the |. With
my editor's configuration when broken like this, the 2nd line would be
intended with the opening ( after the |.

> +
> +	/* As scale <= 15 the shift operation cannot overflow. */
> +	pwm->real_period = div64_ul(1000000000ULL << (PWM_SIFIVE_CMPWIDTH +
> +				    scale), rate);

ditto. Maybe break after the =?

> +	dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
> +}
> +
> +static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> +				 struct pwm_state *state)
> +{
> +	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> +	u32 duty;
> +	int val;
> +
> +	duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm *
> +		     PWM_SIFIVE_SIZE_PWMCMP);
> +
> +	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> +	state->enabled = (val & BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS)) > 0;
> +
> +	val &= 0x0F;
> +	pwm->real_period = div64_ul(1000000000ULL << (PWM_SIFIVE_CMPWIDTH +
> +				    val), clk_get_rate(pwm->clk));

Another bad line break.

> +
> +	state->period = pwm->real_period;
> +	state->duty_cycle = ((u64)duty * pwm->real_period) >>
> +			    PWM_SIFIVE_CMPWIDTH;
> +	state->polarity = PWM_POLARITY_INVERSED;
> +}
> +
> +static int pwm_sifive_enable(struct pwm_chip *chip, struct pwm_device *dev,
> +			     bool enable)
> +{
> +	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> +	u32 val;
> +	int ret;
> +
> +	if (enable) {
> +		ret = clk_enable(pwm->clk);
> +		if (ret) {
> +			dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> +	if (enable)
> +		val |= BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS);
> +	else
> +		val &= ~BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS);
> +
> +	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> +	if (!enable)
> +		clk_disable(pwm->clk);

A disabled PWM is supposed to output an inactive signal. If the PWM runs
at (near) 100% and you disable it, does it reliably give that inactive
signal after completing the currently running period?

> +
> +	return 0;
> +}
> +
> +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> +			    struct pwm_state *state)
> +{
> +	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> +	unsigned int duty_cycle;
> +	u32 frac, val;
> +	struct pwm_state cur_state;
> +	bool enabled;
> +	int ret;
> +
> +	pwm_get_state(dev, &cur_state);
> +	enabled = cur_state.enabled;
> +
> +	if (state->polarity != PWM_POLARITY_INVERSED)
> +		return -EINVAL;
> +
> +	if (state->period != cur_state.period) {
> +		if (pwm->user_count != 1)
> +			return -EINVAL;

I think we need locking here. Consider two pwm users on two CPUs:

	CPU1					CPU2
	pwm_sifive_apply(pwm0, period=A, ...)
	 check user_count==1 -> good
	 ...					pwm1 = pwm_get(...)
	 ...					pwm_sifive_apply(pwm1, period=B...)
	 ...					  configure based on B
	 pwm_sifive_update_clock()

Also I wonder if we should change the period if the user requested
enabled=false.

> +		pwm->real_period = state->period;
> +		pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));

If you change from

	.period = A
	.duty_cycle = B

to

	.period = C
	.duty_cycle = D

the output pin might see a period with

	.period = C
	.duty_cycle = B

right? I think this is not fixable, but this needs a prominent comment.

> +	}
> +
> +	if (!state->enabled && enabled) {
> +		ret = pwm_sifive_enable(chip, dev, false);
> +		if (ret)
> +			return ret;
> +		enabled = false;
> +	}
> +
> +	duty_cycle = state->duty_cycle;
> +	frac = div_u64((u64)duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) +
> +		       (1 << PWM_SIFIVE_CMPWIDTH) / 2, state->period);
> +	/* The hardware cannot generate a 100% duty cycle */

@Thierry: Do you consider this bad enough that pwm_apply_state should
fail if 100% is requested?

> +	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> +
> +	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> +	val |= BIT(PWM_SIFIVE_PWMCFG_DEGLITCH);
> +	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> +	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm *
> +	       PWM_SIFIVE_SIZE_PWMCMP);
> +
> +	val &= ~BIT(PWM_SIFIVE_PWMCFG_DEGLITCH);

Doesn't that come too early? I thought the right thing was to keep it
set all the time. With this code I think you might see a duty cycle of
50 when going from 60 to 40.

> +	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> +	if (state->enabled != enabled) {
> +		ret = pwm_sifive_enable(chip, dev, state->enabled);
> +		if (ret)
> +			return ret;
> +	}

FTR: This doesn't block until the newly configured state is active.

> +
> +	return 0;
> +}

Best regards
Uwe

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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1747 bytes --]

On Wed, Feb 13, 2019 at 11:34:59AM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 13, 2019 at 02:56:18PM +0530, Yash Shah wrote:
[...]
> > +	unsigned long scale_pow =
> > +			div64_ul(pwm->real_period * (u64)rate, NSEC_PER_SEC);
> > +	int scale = clamp(ilog2(scale_pow) - PWM_SIFIVE_CMPWIDTH, 0, 0xf);
> > +
> > +	writel((1 << PWM_SIFIVE_PWMCFG_EN_ALWAYS) | (scale <<
> > +	       PWM_SIFIVE_PWMCFG_SCALE), pwm->regs + PWM_SIFIVE_PWMCFG);
> 
> I think this would be better readable with the newline after the |. With
> my editor's configuration when broken like this, the 2nd line would be
> intended with the opening ( after the |.
> 
> > +
> > +	/* As scale <= 15 the shift operation cannot overflow. */
> > +	pwm->real_period = div64_ul(1000000000ULL << (PWM_SIFIVE_CMPWIDTH +
> > +				    scale), rate);
> 
> ditto. Maybe break after the =?
> 
> > +	dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
> > +}
> > +
> > +static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> > +				 struct pwm_state *state)
> > +{
> > +	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +	u32 duty;
> > +	int val;
> > +
> > +	duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm *
> > +		     PWM_SIFIVE_SIZE_PWMCMP);
> > +
> > +	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > +	state->enabled = (val & BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS)) > 0;
> > +
> > +	val &= 0x0F;
> > +	pwm->real_period = div64_ul(1000000000ULL << (PWM_SIFIVE_CMPWIDTH +
> > +				    val), clk_get_rate(pwm->clk));
> 
> Another bad line break.

Maybe just split all of these very long expressions up by introducing
temporary variables to make things more palatable?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Rob Herring]

On Wed, Feb 13, 2019 at 02:56:17PM +0530, Yash Shah wrote:
> DT documentation for PWM controller added.
> 
> 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         | 30 ++++++++++++++++++++++
>  1 file changed, 30 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..3b9c64c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> @@ -0,0 +1,30 @@
> +SiFive PWM controller
> +
> +Unlike most other PWM controllers, the SiFive PWM controller currently only
> +supports one period for all channels in the PWM. All PWMs need to run at
> +the same period. The period also has significant restrictions on the values
> +it can achieve, which the driver rounds to the nearest achievable period.
> +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
> +
> +Required properties:
> +- compatible: Should be "sifive,$socname-pwm" and "sifive,pwmX".
> +  Please refer to sifive-blocks-ip-versioning.txt for details.

Please specify what soc and IP version are used. Is sifive,pwm10 valid?

> +- 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 3. See pwm.txt in this directory
> +  for a description of the cell format.
> +- interrupts: one interrupt per PWM channel
> +
> +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 = <3>;
> +};
> -- 
> 1.9.1
> 

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

[Yash Shah]

On Wed, Feb 13, 2019 at 4:05 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Wed, Feb 13, 2019 at 02:56:18PM +0530, Yash Shah wrote:
> > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> >
> > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> >  drivers/pwm/Kconfig      |  11 ++
> >  drivers/pwm/Makefile     |   1 +
> >  drivers/pwm/pwm-sifive.c | 311 +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 323 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-sifive.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index a8f47df..4a61d1a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -380,6 +380,17 @@ config PWM_SAMSUNG
> >         To compile this driver as a module, choose M here: the module
> >         will be called pwm-samsung.
> >
> > +config PWM_SIFIVE
> > +     tristate "SiFive PWM support"
> > +     depends on OF
> > +     depends on COMMON_CLK
> > +     depends on RISCV || COMPILE_TEST
> > +     help
> > +       Generic PWM framework driver for SiFive SoCs.
> > +
> > +       To compile this driver as a module, choose M here: the module
> > +       will be called pwm-sifive.
> > +
> >  config PWM_SPEAR
> >       tristate "STMicroelectronics SPEAr PWM support"
> >       depends on PLAT_SPEAR
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 9c676a0..30089ca 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR)              += pwm-rcar.o
> >  obj-$(CONFIG_PWM_RENESAS_TPU)        += pwm-renesas-tpu.o
> >  obj-$(CONFIG_PWM_ROCKCHIP)   += pwm-rockchip.o
> >  obj-$(CONFIG_PWM_SAMSUNG)    += pwm-samsung.o
> > +obj-$(CONFIG_PWM_SIFIVE)     += pwm-sifive.o
> >  obj-$(CONFIG_PWM_SPEAR)              += pwm-spear.o
> >  obj-$(CONFIG_PWM_STI)                += pwm-sti.o
> >  obj-$(CONFIG_PWM_STM32)              += pwm-stm32.o
> > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > new file mode 100644
> > index 0000000..c0eb90e
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,311 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2017-2018 SiFive
> > + * For SiFive's PWM IP block documentation please refer Chapter 14 of
> > + * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/slab.h>
> > +
> > +/* Register offsets */
> > +#define PWM_SIFIVE_PWMCFG            0x0
> > +#define PWM_SIFIVE_PWMCOUNT          0x8
> > +#define PWM_SIFIVE_PWMS                      0x10
> > +#define PWM_SIFIVE_PWMCMP0           0x20
> > +
> > +/* PWMCFG fields */
> > +#define PWM_SIFIVE_PWMCFG_SCALE              0
> > +#define PWM_SIFIVE_PWMCFG_STICKY     8
> > +#define PWM_SIFIVE_PWMCFG_ZERO_CMP   9
> > +#define PWM_SIFIVE_PWMCFG_DEGLITCH   10
> > +#define PWM_SIFIVE_PWMCFG_EN_ALWAYS  12
>
> PWM_SIFIVE_PWMCFG_EN_ALWAYS is always used as
>
>         BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS)
>
> so defining this as BIT(12) directly makes some expressions below easier
> to read.

Sure, will do that.

>
> > +#define PWM_SIFIVE_PWMCFG_EN_ONCE    13
> > +#define PWM_SIFIVE_PWMCFG_CENTER     16
> > +#define PWM_SIFIVE_PWMCFG_GANG               24
> > +#define PWM_SIFIVE_PWMCFG_IP         28
> > +
> > +/* PWM_SIFIVE_SIZE_PWMCMP is used to calculate offset for pwmcmpX registers */
> > +#define PWM_SIFIVE_SIZE_PWMCMP               4
> > +#define PWM_SIFIVE_CMPWIDTH          16
> > +
> > +struct pwm_sifive_ddata {
> > +     struct pwm_chip chip;
> > +     struct notifier_block notifier;
> > +     struct clk *clk;
> > +     void __iomem *regs;
> > +     unsigned int real_period;
> > +     int user_count;
> > +};
> > +
> > +static inline
> > +struct pwm_sifive_ddata *pwm_sifive_chip_to_ddata(struct pwm_chip *c)
> > +{
> > +     return container_of(c, struct pwm_sifive_ddata, chip);
> > +}
> > +
> > +static int pwm_sifive_request(struct pwm_chip *chip, struct pwm_device *dev)
> > +{
> > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +
> > +     pwm->user_count++;
> > +
> > +     return 0;
> > +}
> > +
> > +static void pwm_sifive_free(struct pwm_chip *chip, struct pwm_device *dev)
> > +{
> > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +
> > +     pwm->user_count--;
> > +}
> > +
> > +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> > +                                 unsigned long rate)
> > +{
> > +     /* (1 << (16+scale)) * 10^9/rate = real_period */
>
> Maybe you want to mention here the relation between 16 and
> PWM_SIFIVE_CMPWIDTH.

Sure, will change it to
/* (1 << (PWM_SIFIVE_CMPWIDTH+scale)) * 10^9/rate = real_period */

>
> > +     unsigned long scale_pow =
> > +                     div64_ul(pwm->real_period * (u64)rate, NSEC_PER_SEC);
> > +     int scale = clamp(ilog2(scale_pow) - PWM_SIFIVE_CMPWIDTH, 0, 0xf);
> > +
> > +     writel((1 << PWM_SIFIVE_PWMCFG_EN_ALWAYS) | (scale <<
> > +            PWM_SIFIVE_PWMCFG_SCALE), pwm->regs + PWM_SIFIVE_PWMCFG);
>
> I think this would be better readable with the newline after the |. With
> my editor's configuration when broken like this, the 2nd line would be
> intended with the opening ( after the |.
>
> > +
> > +     /* As scale <= 15 the shift operation cannot overflow. */
> > +     pwm->real_period = div64_ul(1000000000ULL << (PWM_SIFIVE_CMPWIDTH +
> > +                                 scale), rate);
>
> ditto. Maybe break after the =?
>
> > +     dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
> > +}
> > +
> > +static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> > +                              struct pwm_state *state)
> > +{
> > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +     u32 duty;
> > +     int val;
> > +
> > +     duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm *
> > +                  PWM_SIFIVE_SIZE_PWMCMP);
> > +
> > +     val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > +     state->enabled = (val & BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS)) > 0;
> > +
> > +     val &= 0x0F;
> > +     pwm->real_period = div64_ul(1000000000ULL << (PWM_SIFIVE_CMPWIDTH +
> > +                                 val), clk_get_rate(pwm->clk));
>
> Another bad line break.

Will add temp variable to split the above expressions.

>
> > +
> > +     state->period = pwm->real_period;
> > +     state->duty_cycle = ((u64)duty * pwm->real_period) >>
> > +                         PWM_SIFIVE_CMPWIDTH;
> > +     state->polarity = PWM_POLARITY_INVERSED;
> > +}
> > +
> > +static int pwm_sifive_enable(struct pwm_chip *chip, struct pwm_device *dev,
> > +                          bool enable)
> > +{
> > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +     u32 val;
> > +     int ret;
> > +
> > +     if (enable) {
> > +             ret = clk_enable(pwm->clk);
> > +             if (ret) {
> > +                     dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > +     if (enable)
> > +             val |= BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS);
> > +     else
> > +             val &= ~BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS);
> > +
> > +     writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > +     if (!enable)
> > +             clk_disable(pwm->clk);
>
> A disabled PWM is supposed to output an inactive signal. If the PWM runs
> at (near) 100% and you disable it, does it reliably give that inactive
> signal after completing the currently running period?

Yes, you are right, it just freezes at that state (100%).
What if I set duty cycle = 0 if (!state->enabled) before disabling the PWM?

>
> > +
> > +     return 0;
> > +}
> > +
> > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> > +                         struct pwm_state *state)
> > +{
> > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +     unsigned int duty_cycle;
> > +     u32 frac, val;
> > +     struct pwm_state cur_state;
> > +     bool enabled;
> > +     int ret;
> > +
> > +     pwm_get_state(dev, &cur_state);
> > +     enabled = cur_state.enabled;
> > +
> > +     if (state->polarity != PWM_POLARITY_INVERSED)
> > +             return -EINVAL;
> > +
> > +     if (state->period != cur_state.period) {
> > +             if (pwm->user_count != 1)
> > +                     return -EINVAL;
>
> I think we need locking here. Consider two pwm users on two CPUs:
>
>         CPU1                                    CPU2
>         pwm_sifive_apply(pwm0, period=A, ...)
>          check user_count==1 -> good
>          ...                                    pwm1 = pwm_get(...)
>          ...                                    pwm_sifive_apply(pwm1, period=B...)
>          ...                                      configure based on B
>          pwm_sifive_update_clock()

mutex_lock();
  if (pwm->user_count != 1)
                  return -EINVAL;
mutex_unlock();
Something like this?

>
> Also I wonder if we should change the period if the user requested
> enabled=false.

You want me to NOT update period if enabled=false, right?

>
> > +             pwm->real_period = state->period;
> > +             pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
>
> If you change from
>
>         .period = A
>         .duty_cycle = B
>
> to
>
>         .period = C
>         .duty_cycle = D
>
> the output pin might see a period with
>
>         .period = C
>         .duty_cycle = B
>
> right? I think this is not fixable, but this needs a prominent comment.

Good point. Is the below comment good enough?
/* When changing both duty cycle and period, the old duty cycle might
be active with new the period settings for a period */

>
> > +     }
> > +
> > +     if (!state->enabled && enabled) {
> > +             ret = pwm_sifive_enable(chip, dev, false);
> > +             if (ret)
> > +                     return ret;
> > +             enabled = false;
> > +     }
> > +
> > +     duty_cycle = state->duty_cycle;
> > +     frac = div_u64((u64)duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) +
> > +                    (1 << PWM_SIFIVE_CMPWIDTH) / 2, state->period);
> > +     /* The hardware cannot generate a 100% duty cycle */
>
> @Thierry: Do you consider this bad enough that pwm_apply_state should
> fail if 100% is requested?
>
> > +     frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > +
> > +     val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > +     val |= BIT(PWM_SIFIVE_PWMCFG_DEGLITCH);
> > +     writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > +     writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm *
> > +            PWM_SIFIVE_SIZE_PWMCMP);
> > +
> > +     val &= ~BIT(PWM_SIFIVE_PWMCFG_DEGLITCH);
>
> Doesn't that come too early? I thought the right thing was to keep it
> set all the time. With this code I think you might see a duty cycle of
> 50 when going from 60 to 40.

We cannot set it all the time.
Setting it all the time makes every alternate period to remain high
(latched state).
As per the manual, it needs to be set when reprogramming and must be
cleared afterwards.

>
> > +     writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > +     if (state->enabled != enabled) {
> > +             ret = pwm_sifive_enable(chip, dev, state->enabled);
> > +             if (ret)
> > +                     return ret;
> > +     }
>
> FTR: This doesn't block until the newly configured state is active.
>
> > +
> > +     return 0;
> > +}
>
> Best regards
> Uwe

Thanks for your comments

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

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

[Yash Shah]

On Thu, Feb 14, 2019 at 2:07 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Feb 13, 2019 at 02:56:17PM +0530, Yash Shah wrote:
> > DT documentation for PWM controller added.
> >
> > 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         | 30 ++++++++++++++++++++++
> >  1 file changed, 30 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..3b9c64c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > @@ -0,0 +1,30 @@
> > +SiFive PWM controller
> > +
> > +Unlike most other PWM controllers, the SiFive PWM controller currently only
> > +supports one period for all channels in the PWM. All PWMs need to run at
> > +the same period. The period also has significant restrictions on the values
> > +it can achieve, which the driver rounds to the nearest achievable period.
> > +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
> > +
> > +Required properties:
> > +- compatible: Should be "sifive,$socname-pwm" and "sifive,pwmX".
> > +  Please refer to sifive-blocks-ip-versioning.txt for details.
>
> Please specify what soc and IP version are used. Is sifive,pwm10 valid?

Does the below description seem fine?

should be "sifive,<chip>-pwm" and "sifive,pwm<version>".
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. Please refer to sifive-blocks-ip-versioning.txt for details

>
> > +- 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 3. See pwm.txt in this directory
> > +  for a description of the cell format.
> > +- interrupts: one interrupt per PWM channel
> > +
> > +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 = <3>;
> > +};
> > --
> > 1.9.1
> >

Thanks for the comment

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

[Uwe Kleine-König]

Hello,

On Thu, Feb 14, 2019 at 01:25:27PM +0530, Yash Shah wrote:
> On Wed, Feb 13, 2019 at 4:05 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Wed, Feb 13, 2019 at 02:56:18PM +0530, Yash Shah wrote:
> > > +static int pwm_sifive_enable(struct pwm_chip *chip, struct pwm_device *dev,
> > > +                          bool enable)
> > > +{
> > > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > > +     u32 val;
> > > +     int ret;
> > > +
> > > +     if (enable) {
> > > +             ret = clk_enable(pwm->clk);
> > > +             if (ret) {
> > > +                     dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> > > +                     return ret;
> > > +             }
> > > +     }
> > > +
> > > +     val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > > +
> > > +     if (enable)
> > > +             val |= BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS);
> > > +     else
> > > +             val &= ~BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS);
> > > +
> > > +     writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > +
> > > +     if (!enable)
> > > +             clk_disable(pwm->clk);
> >
> > A disabled PWM is supposed to output an inactive signal. If the PWM runs
> > at (near) 100% and you disable it, does it reliably give that inactive
> > signal after completing the currently running period?
> 
> Yes, you are right, it just freezes at that state (100%).
> What if I set duty cycle = 0 if (!state->enabled) before disabling the PWM?

Then you only need to be sure that the inactive level is already latched
to the pwmcmpXip output (which should only need one clock cycle if I'm
not mistaken) before disabling the clock.

> > > +     return 0;
> > > +}
> > > +
> > > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> > > +                         struct pwm_state *state)
> > > +{
> > > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > > +     unsigned int duty_cycle;
> > > +     u32 frac, val;
> > > +     struct pwm_state cur_state;
> > > +     bool enabled;
> > > +     int ret;
> > > +
> > > +     pwm_get_state(dev, &cur_state);
> > > +     enabled = cur_state.enabled;
> > > +
> > > +     if (state->polarity != PWM_POLARITY_INVERSED)
> > > +             return -EINVAL;
> > > +
> > > +     if (state->period != cur_state.period) {
> > > +             if (pwm->user_count != 1)
> > > +                     return -EINVAL;
> >
> > I think we need locking here. Consider two pwm users on two CPUs:
> >
> >         CPU1                                    CPU2
> >         pwm_sifive_apply(pwm0, period=A, ...)
> >          check user_count==1 -> good
> >          ...                                    pwm1 = pwm_get(...)
> >          ...                                    pwm_sifive_apply(pwm1, period=B...)
> >          ...                                      configure based on B
> >          pwm_sifive_update_clock()
> 
> mutex_lock();
>   if (pwm->user_count != 1)
>                   return -EINVAL;
> mutex_unlock();
> Something like this?

No, the lock needs to protect more. You must at least cover increasing
and decreasing of user_count and you must hold the lock until the period
update is completed.

> > Also I wonder if we should change the period if the user requested
> > enabled=false.
> 
> You want me to NOT update period if enabled=false, right?

I don't know for sure. Given that period is shared for all four PWM
outputs it might be sensible to change it at least in a shadow variable
and only do it when actually needed. (But maybe we can postpone that as
it doesn't matter for correctness of the driver.)

The question here is: In the following snippet:

	pwm0 = pwm_get(... the first pwm ...)

	pwm_apply_state(pwm0, { .enabled = true, .period = 4000 });
	pwm_apply_state(pwm0, { .enabled = false, .period = 8000 });

	pwm1 = pwm_get(... the second pwm ...)
	pwm_apply_state(pwm1, { .enabled = true, .period = 4000 });
	pwm_apply_state(pwm0, { .enabled = true, .period = 8000 });

Which of the two last commands should fail?

> > > +             pwm->real_period = state->period;
> > > +             pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
> >
> > If you change from
> >
> >         .period = A
> >         .duty_cycle = B
> >
> > to
> >
> >         .period = C
> >         .duty_cycle = D
> >
> > the output pin might see a period with
> >
> >         .period = C
> >         .duty_cycle = B
> >
> > right? I think this is not fixable, but this needs a prominent comment.
> 
> Good point. Is the below comment good enough?
> /* When changing both duty cycle and period, the old duty cycle might
> be active with new the period settings for a period */

I'd add some blame on the hardware. Something like:

	/*
	 * When changing both duty cycle and period, we cannot prevent in
	 * software that the output might produce a period with mixed
	 * settings (new period length and old duty cycle).
	 */

I'd say it makes sense to put this information at the top of the driver
to have it in a prominent place. Also point out the inability to provide
100% duty cycle and that the hardware is limited to inverted output.
Then all limitations are summarized in a single place.

Maybe this mismatch could be made less likely by changing the order of
the register accesses and a delay depending on pwms and old and new
settings. But I'd say this is too much for now and can be addressed
later when and if necessary.

> > > +     }
> > > +
> > > +     if (!state->enabled && enabled) {
> > > +             ret = pwm_sifive_enable(chip, dev, false);
> > > +             if (ret)
> > > +                     return ret;
> > > +             enabled = false;
> > > +     }
> > > +
> > > +     duty_cycle = state->duty_cycle;
> > > +     frac = div_u64((u64)duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) +
> > > +                    (1 << PWM_SIFIVE_CMPWIDTH) / 2, state->period);
> > > +     /* The hardware cannot generate a 100% duty cycle */
> >
> > @Thierry: Do you consider this bad enough that pwm_apply_state should
> > fail if 100% is requested?

This question is still open.

> > > +     frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > > +
> > > +     val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > > +     val |= BIT(PWM_SIFIVE_PWMCFG_DEGLITCH);
> > > +     writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > +
> > > +     writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm *
> > > +            PWM_SIFIVE_SIZE_PWMCMP);
> > > +
> > > +     val &= ~BIT(PWM_SIFIVE_PWMCFG_DEGLITCH);
> >
> > Doesn't that come too early? I thought the right thing was to keep it
> > set all the time. With this code I think you might see a duty cycle of
> > 50 when going from 60 to 40.
> 
> We cannot set it all the time.
> Setting it all the time makes every alternate period to remain high
> (latched state).
> As per the manual, it needs to be set when reprogramming and must be
> cleared afterwards.

I didn't find this in the manual. When looking at Figure 6 and the
description of pwmdeglitch I have the impression that your statement is
wrong.

Setting pwmdeglitch only prevents the output from getting low during a
period, it can only go low when pwms overflows (i.e. the start of a
period). That's exactly what we want. Where is the misunderstanding?

If however you clear pwmdeglitch after an update, consider the following
series of events:

 - Assume pwmcmpX is 0x4000 and pwms is 0x5000, so pwmcmpXip is high.
 - You set pwmdeglitch and change pwmcmpX to 0x8000 while pwms advanced
   only little. Then pwmcmpXip remains high.
 - Then you clear pwmdeglitch at say pwms = 0x5020, this makes pwmcmpXip
   fall which we should prevent.

Also note that when setting pwmdeglitch while configuring pwm0---if it
really had the behaviour you pointed out---would interfere with the
maybe running pwm1.

So I'm convinced keeping pwmdeglitch active always is the right thing to
do.

Best regards
Uwe

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

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

[Yash Shah]

On Thu, Feb 14, 2019 at 2:04 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Thu, Feb 14, 2019 at 01:25:27PM +0530, Yash Shah wrote:
> > On Wed, Feb 13, 2019 at 4:05 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Wed, Feb 13, 2019 at 02:56:18PM +0530, Yash Shah wrote:
> > > > +static int pwm_sifive_enable(struct pwm_chip *chip, struct pwm_device *dev,
> > > > +                          bool enable)
> > > > +{
> > > > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > > > +     u32 val;
> > > > +     int ret;
> > > > +
> > > > +     if (enable) {
> > > > +             ret = clk_enable(pwm->clk);
> > > > +             if (ret) {
> > > > +                     dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> > > > +                     return ret;
> > > > +             }
> > > > +     }
> > > > +
> > > > +     val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > > > +
> > > > +     if (enable)
> > > > +             val |= BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS);
> > > > +     else
> > > > +             val &= ~BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS);
> > > > +
> > > > +     writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > > +
> > > > +     if (!enable)
> > > > +             clk_disable(pwm->clk);
> > >
> > > A disabled PWM is supposed to output an inactive signal. If the PWM runs
> > > at (near) 100% and you disable it, does it reliably give that inactive
> > > signal after completing the currently running period?
> >
> > Yes, you are right, it just freezes at that state (100%).
> > What if I set duty cycle = 0 if (!state->enabled) before disabling the PWM?
>
> Then you only need to be sure that the inactive level is already latched
> to the pwmcmpXip output (which should only need one clock cycle if I'm
> not mistaken) before disabling the clock.
>
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> > > > +                         struct pwm_state *state)
> > > > +{
> > > > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > > > +     unsigned int duty_cycle;
> > > > +     u32 frac, val;
> > > > +     struct pwm_state cur_state;
> > > > +     bool enabled;
> > > > +     int ret;
> > > > +
> > > > +     pwm_get_state(dev, &cur_state);
> > > > +     enabled = cur_state.enabled;
> > > > +
> > > > +     if (state->polarity != PWM_POLARITY_INVERSED)
> > > > +             return -EINVAL;
> > > > +
> > > > +     if (state->period != cur_state.period) {
> > > > +             if (pwm->user_count != 1)
> > > > +                     return -EINVAL;
> > >
> > > I think we need locking here. Consider two pwm users on two CPUs:
> > >
> > >         CPU1                                    CPU2
> > >         pwm_sifive_apply(pwm0, period=A, ...)
> > >          check user_count==1 -> good
> > >          ...                                    pwm1 = pwm_get(...)
> > >          ...                                    pwm_sifive_apply(pwm1, period=B...)
> > >          ...                                      configure based on B
> > >          pwm_sifive_update_clock()
> >
> > mutex_lock();
> >   if (pwm->user_count != 1)
> >                   return -EINVAL;
> > mutex_unlock();
> > Something like this?
>
> No, the lock needs to protect more. You must at least cover increasing
> and decreasing of user_count and you must hold the lock until the period
> update is completed.

Got your point. Will use locks at appropriate places

>
> > > Also I wonder if we should change the period if the user requested
> > > enabled=false.
> >
> > You want me to NOT update period if enabled=false, right?
>
> I don't know for sure. Given that period is shared for all four PWM
> outputs it might be sensible to change it at least in a shadow variable
> and only do it when actually needed. (But maybe we can postpone that as
> it doesn't matter for correctness of the driver.)
>
> The question here is: In the following snippet:
>
>         pwm0 = pwm_get(... the first pwm ...)
>
>         pwm_apply_state(pwm0, { .enabled = true, .period = 4000 });
>         pwm_apply_state(pwm0, { .enabled = false, .period = 8000 });
>
>         pwm1 = pwm_get(... the second pwm ...)
>         pwm_apply_state(pwm1, { .enabled = true, .period = 4000 });
>         pwm_apply_state(pwm0, { .enabled = true, .period = 8000 });
>
> Which of the two last commands should fail?
>
> > > > +             pwm->real_period = state->period;
> > > > +             pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
> > >
> > > If you change from
> > >
> > >         .period = A
> > >         .duty_cycle = B
> > >
> > > to
> > >
> > >         .period = C
> > >         .duty_cycle = D
> > >
> > > the output pin might see a period with
> > >
> > >         .period = C
> > >         .duty_cycle = B
> > >
> > > right? I think this is not fixable, but this needs a prominent comment.
> >
> > Good point. Is the below comment good enough?
> > /* When changing both duty cycle and period, the old duty cycle might
> > be active with new the period settings for a period */
>
> I'd add some blame on the hardware. Something like:
>
>         /*
>          * When changing both duty cycle and period, we cannot prevent in
>          * software that the output might produce a period with mixed
>          * settings (new period length and old duty cycle).
>          */
>
> I'd say it makes sense to put this information at the top of the driver
> to have it in a prominent place. Also point out the inability to provide
> 100% duty cycle and that the hardware is limited to inverted output.
> Then all limitations are summarized in a single place.

Sure, will do as you suggested.

>
> Maybe this mismatch could be made less likely by changing the order of
> the register accesses and a delay depending on pwms and old and new
> settings. But I'd say this is too much for now and can be addressed
> later when and if necessary.
>
> > > > +     }
> > > > +
> > > > +     if (!state->enabled && enabled) {
> > > > +             ret = pwm_sifive_enable(chip, dev, false);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +             enabled = false;
> > > > +     }
> > > > +
> > > > +     duty_cycle = state->duty_cycle;
> > > > +     frac = div_u64((u64)duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) +
> > > > +                    (1 << PWM_SIFIVE_CMPWIDTH) / 2, state->period);
> > > > +     /* The hardware cannot generate a 100% duty cycle */
> > >
> > > @Thierry: Do you consider this bad enough that pwm_apply_state should
> > > fail if 100% is requested?
>
> This question is still open.
>
> > > > +     frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > > > +
> > > > +     val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > > > +     val |= BIT(PWM_SIFIVE_PWMCFG_DEGLITCH);
> > > > +     writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > > +
> > > > +     writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm *
> > > > +            PWM_SIFIVE_SIZE_PWMCMP);
> > > > +
> > > > +     val &= ~BIT(PWM_SIFIVE_PWMCFG_DEGLITCH);
> > >
> > > Doesn't that come too early? I thought the right thing was to keep it
> > > set all the time. With this code I think you might see a duty cycle of
> > > 50 when going from 60 to 40.
> >
> > We cannot set it all the time.
> > Setting it all the time makes every alternate period to remain high
> > (latched state).
> > As per the manual, it needs to be set when reprogramming and must be
> > cleared afterwards.
>
> I didn't find this in the manual. When looking at Figure 6 and the
> description of pwmdeglitch I have the impression that your statement is
> wrong.
>
> Setting pwmdeglitch only prevents the output from getting low during a
> period, it can only go low when pwms overflows (i.e. the start of a
> period). That's exactly what we want. Where is the misunderstanding?
>
> If however you clear pwmdeglitch after an update, consider the following
> series of events:
>
>  - Assume pwmcmpX is 0x4000 and pwms is 0x5000, so pwmcmpXip is high.
>  - You set pwmdeglitch and change pwmcmpX to 0x8000 while pwms advanced
>    only little. Then pwmcmpXip remains high.
>  - Then you clear pwmdeglitch at say pwms = 0x5020, this makes pwmcmpXip
>    fall which we should prevent.
>
> Also note that when setting pwmdeglitch while configuring pwm0---if it
> really had the behaviour you pointed out---would interfere with the
> maybe running pwm1.
>
> So I'm convinced keeping pwmdeglitch active always is the right thing to
> do.

I agree that figure 6 suggests that pwmdeglitch should remain active
through out.
But for some strange reason when I set deglitch bit, I am seeing every
alternate pwm cycle to remain high (latched) unless I clear the
deglitch bit.
I am debugging this issue however is it ok if I remove deglitch logic
from driver until this issue has been root caused?

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