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

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

v8
- Typo corrections
- Remove active_user and related code
- Do not clear PWM_SIFIVE_PWMCFG_EN_ALWAYS
- Other minor fixes

v7
- Modify description of compatible property in DT documentation
- Use mutex locks at appropriate places
- Fix all bad line breaks
- Allow enabling/disabling PWM only when the user is the only active user
- Remove Deglitch logic
- Other minor fixes

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 macros with appropriate names
- Remove unused macros
- Rename struct sifive_pwm_device to struct pwm_sifive_ddata
- Rename function prefix as per driver name
- 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         |  33 ++
 drivers/pwm/Kconfig                                |  11 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-sifive.c                           | 345 +++++++++++++++++++++
 4 files changed, 390 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
 create mode 100644 drivers/pwm/pwm-sifive.c

-- 
1.9.1

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

v8
- Typo corrections
- Remove active_user and related code
- Do not clear PWM_SIFIVE_PWMCFG_EN_ALWAYS
- Other minor fixes

v7
- Modify description of compatible property in DT documentation
- Use mutex locks at appropriate places
- Fix all bad line breaks
- Allow enabling/disabling PWM only when the user is the only active user
- Remove Deglitch logic
- Other minor fixes

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 macros with appropriate names
- Remove unused macros
- Rename struct sifive_pwm_device to struct pwm_sifive_ddata
- Rename function prefix as per driver name
- 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         |  33 ++
 drivers/pwm/Kconfig                                |  11 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-sifive.c                           | 345 +++++++++++++++++++++
 4 files changed, 390 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
 create mode 100644 drivers/pwm/pwm-sifive.c

-- 
1.9.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v8 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/pwm/pwm-sifive.txt         | 33 ++++++++++++++++++++++
 1 file changed, 33 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..36447e3
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
@@ -0,0 +1,33 @@
+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,<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

[PATCH v8 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/pwm/pwm-sifive.txt         | 33 ++++++++++++++++++++++
 1 file changed, 33 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..36447e3
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
@@ -0,0 +1,33 @@
+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,<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

[PATCH v8 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/pwm/pwm-sifive.txt         | 33 ++++++++++++++++++++++
 1 file changed, 33 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..36447e3
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
@@ -0,0 +1,33 @@
+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,<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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v8 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 | 345 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 357 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..6679ec7
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,345 @@
+// 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
+ *
+ * Limitations:
+ * - 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).
+ * - The hardware cannot generate a 100% duty cycle.
+ * - The hardware generates only inverted output.
+ */
+#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	BIT(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
+#define PWM_SIFIVE_DEFAULT_PERIOD	10000000
+
+struct pwm_sifive_ddata {
+	struct pwm_chip	chip;
+	struct mutex lock; /* lock to protect user_count */
+	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);
+
+	mutex_lock(&pwm->lock);
+	pwm->user_count++;
+	mutex_unlock(&pwm->lock);
+
+	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);
+
+	mutex_lock(&pwm->lock);
+	pwm->user_count--;
+	mutex_unlock(&pwm->lock);
+}
+
+static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
+				    unsigned long rate)
+{
+	u32 val;
+	unsigned long long num;
+	/* (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);
+
+	val = PWM_SIFIVE_PWMCFG_EN_ALWAYS | (scale << PWM_SIFIVE_PWMCFG_SCALE);
+	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
+
+	/* As scale <= 15 the shift operation cannot overflow. */
+	num = 1000000000ULL << (PWM_SIFIVE_CMPWIDTH + scale);
+	pwm->real_period = div64_ul(num, 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, val;
+	unsigned long long num;
+	int ret;
+
+	ret = clk_enable(pwm->clk);
+	if (ret)
+		return;
+
+	duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 +
+		     dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
+
+	state->enabled = duty > 0;
+
+	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
+	val &= 0x0F;
+	num = 1000000000ULL << (PWM_SIFIVE_CMPWIDTH + val);
+	pwm->real_period = div64_ul(num, 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;
+
+	clk_disable(pwm->clk);
+}
+
+static int pwm_sifive_enable(struct pwm_chip *chip, bool enable)
+{
+	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
+	int ret;
+
+	if (enable) {
+		ret = clk_enable(pwm->clk);
+		if (ret) {
+			dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
+			return ret;
+		}
+	}
+
+	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, x;
+	u32 frac;
+	struct pwm_state cur_state;
+	bool enabled;
+	int ret = 0;
+	unsigned long num;
+
+	if (state->polarity != PWM_POLARITY_INVERSED)
+		return -EINVAL;
+
+	mutex_lock(&pwm->lock);
+	pwm_get_state(dev, &cur_state);
+	enabled = cur_state.enabled;
+
+	if (state->period != cur_state.period) {
+		if (pwm->user_count != 1) {
+			ret = -EINVAL;
+			goto exit;
+		}
+		pwm->real_period = state->period;
+		pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
+	}
+
+	duty_cycle = state->duty_cycle;
+	if (!state->enabled)
+		duty_cycle = 0;
+
+	x = 1U << PWM_SIFIVE_CMPWIDTH;
+	num = (u64)duty_cycle * x + x / 2;
+	frac = div_u64(num, state->period);
+	/* The hardware cannot generate a 100% duty cycle */
+	frac = min(frac, x - 1);
+
+	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
+	       dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
+
+	if (!state->enabled && enabled) {
+		ret = pwm_sifive_enable(chip, false);
+		if (ret)
+			goto exit;
+		enabled = false;
+	}
+
+	if (state->enabled && !enabled) {
+		ret = pwm_sifive_enable(chip, state->enabled);
+		if (ret)
+			goto exit;
+	}
+
+exit:
+	mutex_unlock(&pwm->lock);
+	return ret;
+}
+
+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, ch;
+	bool is_enabled = false;
+
+	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	mutex_init(&pwm->lock);
+	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;
+	}
+
+	/* Initialize PWM */
+	pwm->real_period = PWM_SIFIVE_DEFAULT_PERIOD;
+	pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
+
+	for (ch = 0; ch < pwm->chip.npwm; ch++) {
+		if (pwm_is_enabled(&pwm->chip.pwms[ch])) {
+			is_enabled = true;
+			break;
+		}
+	}
+	if (!is_enabled)
+		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, ch;
+	bool is_enabled = false;
+
+	ret = pwmchip_remove(&pwm->chip);
+	clk_notifier_unregister(pwm->clk, &pwm->notifier);
+
+	for (ch = 0; ch < pwm->chip.npwm; ch++) {
+		if (pwm_is_enabled(&pwm->chip.pwms[ch])) {
+			is_enabled = true;
+			break;
+		}
+	}
+	if (is_enabled)
+		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

[PATCH v8 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 | 345 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 357 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..6679ec7
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,345 @@
+// 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
+ *
+ * Limitations:
+ * - 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).
+ * - The hardware cannot generate a 100% duty cycle.
+ * - The hardware generates only inverted output.
+ */
+#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	BIT(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
+#define PWM_SIFIVE_DEFAULT_PERIOD	10000000
+
+struct pwm_sifive_ddata {
+	struct pwm_chip	chip;
+	struct mutex lock; /* lock to protect user_count */
+	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);
+
+	mutex_lock(&pwm->lock);
+	pwm->user_count++;
+	mutex_unlock(&pwm->lock);
+
+	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);
+
+	mutex_lock(&pwm->lock);
+	pwm->user_count--;
+	mutex_unlock(&pwm->lock);
+}
+
+static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
+				    unsigned long rate)
+{
+	u32 val;
+	unsigned long long num;
+	/* (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);
+
+	val = PWM_SIFIVE_PWMCFG_EN_ALWAYS | (scale << PWM_SIFIVE_PWMCFG_SCALE);
+	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
+
+	/* As scale <= 15 the shift operation cannot overflow. */
+	num = 1000000000ULL << (PWM_SIFIVE_CMPWIDTH + scale);
+	pwm->real_period = div64_ul(num, 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, val;
+	unsigned long long num;
+	int ret;
+
+	ret = clk_enable(pwm->clk);
+	if (ret)
+		return;
+
+	duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 +
+		     dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
+
+	state->enabled = duty > 0;
+
+	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
+	val &= 0x0F;
+	num = 1000000000ULL << (PWM_SIFIVE_CMPWIDTH + val);
+	pwm->real_period = div64_ul(num, 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;
+
+	clk_disable(pwm->clk);
+}
+
+static int pwm_sifive_enable(struct pwm_chip *chip, bool enable)
+{
+	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
+	int ret;
+
+	if (enable) {
+		ret = clk_enable(pwm->clk);
+		if (ret) {
+			dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
+			return ret;
+		}
+	}
+
+	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, x;
+	u32 frac;
+	struct pwm_state cur_state;
+	bool enabled;
+	int ret = 0;
+	unsigned long num;
+
+	if (state->polarity != PWM_POLARITY_INVERSED)
+		return -EINVAL;
+
+	mutex_lock(&pwm->lock);
+	pwm_get_state(dev, &cur_state);
+	enabled = cur_state.enabled;
+
+	if (state->period != cur_state.period) {
+		if (pwm->user_count != 1) {
+			ret = -EINVAL;
+			goto exit;
+		}
+		pwm->real_period = state->period;
+		pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
+	}
+
+	duty_cycle = state->duty_cycle;
+	if (!state->enabled)
+		duty_cycle = 0;
+
+	x = 1U << PWM_SIFIVE_CMPWIDTH;
+	num = (u64)duty_cycle * x + x / 2;
+	frac = div_u64(num, state->period);
+	/* The hardware cannot generate a 100% duty cycle */
+	frac = min(frac, x - 1);
+
+	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
+	       dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
+
+	if (!state->enabled && enabled) {
+		ret = pwm_sifive_enable(chip, false);
+		if (ret)
+			goto exit;
+		enabled = false;
+	}
+
+	if (state->enabled && !enabled) {
+		ret = pwm_sifive_enable(chip, state->enabled);
+		if (ret)
+			goto exit;
+	}
+
+exit:
+	mutex_unlock(&pwm->lock);
+	return ret;
+}
+
+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, ch;
+	bool is_enabled = false;
+
+	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	mutex_init(&pwm->lock);
+	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;
+	}
+
+	/* Initialize PWM */
+	pwm->real_period = PWM_SIFIVE_DEFAULT_PERIOD;
+	pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
+
+	for (ch = 0; ch < pwm->chip.npwm; ch++) {
+		if (pwm_is_enabled(&pwm->chip.pwms[ch])) {
+			is_enabled = true;
+			break;
+		}
+	}
+	if (!is_enabled)
+		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, ch;
+	bool is_enabled = false;
+
+	ret = pwmchip_remove(&pwm->chip);
+	clk_notifier_unregister(pwm->clk, &pwm->notifier);
+
+	for (ch = 0; ch < pwm->chip.npwm; ch++) {
+		if (pwm_is_enabled(&pwm->chip.pwms[ch])) {
+			is_enabled = true;
+			break;
+		}
+	}
+	if (is_enabled)
+		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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v8 0/2] PWM support for HiFive Unleashed

[Andreas Schwab]

On Mär 01 2019, Yash Shah <yash.shah@sifive.com> wrote:

> This patch series adds a PWM driver and DT documentation
> for HiFive Unleashed board. The patches are mostly based on
> Wesley's patch.

That doesn't appear to work with the heartbeat trigger, the led doesn't
flash.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v8 0/2] PWM support for HiFive Unleashed

[Andreas Schwab]

On Mär 01 2019, Yash Shah <yash.shah@sifive.com> wrote:

> This patch series adds a PWM driver and DT documentation
> for HiFive Unleashed board. The patches are mostly based on
> Wesley's patch.

That doesn't appear to work with the heartbeat trigger, the led doesn't
flash.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v8 0/2] PWM support for HiFive Unleashed

[Yash Shah]

On Mon, Mar 4, 2019 at 4:39 PM Andreas Schwab <schwab@suse.de> wrote:
>
> On Mär 01 2019, Yash Shah <yash.shah@sifive.com> wrote:
>
> > This patch series adds a PWM driver and DT documentation
> > for HiFive Unleashed board. The patches are mostly based on
> > Wesley's patch.
>
> That doesn't appear to work with the heartbeat trigger, the led doesn't
> flash.

Make sure you modify the DTS file since with this patch the period is
now passed through the conventional way.
Example:
pwmleds {
    compatible = "pwm-leds";
    heartbeat {
        pwms = <&L45 0 100000 0>;
        max-brightness = <255>;
        linux,default-trigger = "heartbeat";
    };
};

I have tested the heartbeat trigger on HiFive Unleashed board and it's
working with this PWM patch.


>
> Andreas.
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."

Re: [PATCH v8 0/2] PWM support for HiFive Unleashed

[Yash Shah]

On Mon, Mar 4, 2019 at 4:39 PM Andreas Schwab <schwab@suse.de> wrote:
>
> On Mär 01 2019, Yash Shah <yash.shah@sifive.com> wrote:
>
> > This patch series adds a PWM driver and DT documentation
> > for HiFive Unleashed board. The patches are mostly based on
> > Wesley's patch.
>
> That doesn't appear to work with the heartbeat trigger, the led doesn't
> flash.

Make sure you modify the DTS file since with this patch the period is
now passed through the conventional way.
Example:
pwmleds {
    compatible = "pwm-leds";
    heartbeat {
        pwms = <&L45 0 100000 0>;
        max-brightness = <255>;
        linux,default-trigger = "heartbeat";
    };
};

I have tested the heartbeat trigger on HiFive Unleashed board and it's
working with this PWM patch.


>
> Andreas.
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v8 0/2] PWM support for HiFive Unleashed

[Andreas Schwab]

On Mär 05 2019, Yash Shah <yash.shah@sifive.com> wrote:

> On Mon, Mar 4, 2019 at 4:39 PM Andreas Schwab <schwab@suse.de> wrote:
>>
>> On Mär 01 2019, Yash Shah <yash.shah@sifive.com> wrote:
>>
>> > This patch series adds a PWM driver and DT documentation
>> > for HiFive Unleashed board. The patches are mostly based on
>> > Wesley's patch.
>>
>> That doesn't appear to work with the heartbeat trigger, the led doesn't
>> flash.
>
> Make sure you modify the DTS file

Which DTS file?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v8 0/2] PWM support for HiFive Unleashed

[Andreas Schwab]

On Mär 05 2019, Yash Shah <yash.shah@sifive.com> wrote:

> On Mon, Mar 4, 2019 at 4:39 PM Andreas Schwab <schwab@suse.de> wrote:
>>
>> On Mär 01 2019, Yash Shah <yash.shah@sifive.com> wrote:
>>
>> > This patch series adds a PWM driver and DT documentation
>> > for HiFive Unleashed board. The patches are mostly based on
>> > Wesley's patch.
>>
>> That doesn't appear to work with the heartbeat trigger, the led doesn't
>> flash.
>
> Make sure you modify the DTS file

Which DTS file?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Uwe Kleine-König]

Hello,

On Fri, Mar 01, 2019 at 04:23:19PM +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 | 345 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 357 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..6679ec7
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,345 @@
> +// 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
> + *
> + * Limitations:
> + * - 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).
> + * - The hardware cannot generate a 100% duty cycle.
> + * - The hardware generates only inverted output.
> + */
> +#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	BIT(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

It's a bit inconsistent to have one of them use BIT and the others not.
For consistency please use BIT for all defines. (They are unused
anyhow.) For PWM_SIFIVE_PWMCFG_SCALE use:

	#define PWM_SIFIVE_PWMCFG_SCALE GENMASK(3, 0)

and then FIELD_GET and FIELD_PREP to access the values.

> +/* PWM_SIFIVE_SIZE_PWMCMP is used to calculate offset for pwmcmpX registers */
> +#define PWM_SIFIVE_SIZE_PWMCMP		4
> +#define PWM_SIFIVE_CMPWIDTH		16
> +#define PWM_SIFIVE_DEFAULT_PERIOD	10000000
> +
> +struct pwm_sifive_ddata {
> +	struct pwm_chip	chip;
> +	struct mutex lock; /* lock to protect user_count */
> +	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);
> +
> +	mutex_lock(&pwm->lock);
> +	pwm->user_count++;
> +	mutex_unlock(&pwm->lock);
> +
> +	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);
> +
> +	mutex_lock(&pwm->lock);
> +	pwm->user_count--;
> +	mutex_unlock(&pwm->lock);
> +}
> +
> +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> +				    unsigned long rate)
> +{
> +	u32 val;
> +	unsigned long long num;
> +	/* (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);

I tried for some time to verify the code here and this would have been
easier with a more verbose comment. Something like:

/*
 * The PWM unit is used with pwmzerocmp=0, so the only way to modify the
 * period length is using pwmscale which provides the number of bits the
 * counter is shifted before being feed to the comparators. A period
 * lasts (1 << (PWM_SIFIVE_CMPWIDTH + pwmscale)) clock ticks.
 */

There is a bad rounding effect here. I don't know the machine's details,
and so will consider a parent clock running at 250 MHz. So one clock tick
is 4 ns long and the smallest period length is 4 ns << 16 == 262144 ns.
Consider further an initial target period of 10000000 ns (which is
PWM_SIFIVE_DEFAULT_PERIOD).

The calculation here results in scale_pow = 2500000 and so scale = 5.

> +	val = PWM_SIFIVE_PWMCFG_EN_ALWAYS | (scale << PWM_SIFIVE_PWMCFG_SCALE);
> +	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> +	/* As scale <= 15 the shift operation cannot overflow. */
> +	num = 1000000000ULL << (PWM_SIFIVE_CMPWIDTH + scale);
> +	pwm->real_period = div64_ul(num, rate);
> +	dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
> +}

Then real_period ends up being 8388608 ns. If now the input clk increases
to 750 MHz it is 8388608 ns which is being used as target period length
and the calculation results in:

	scale_pow = 6291456
	scale = 6
	real_period = 5592405

I'd claim it would be better to use scale = 7 here which results in
11184810 which is nearer to the initially targeted 10000000 ns. (But we
cannot be sure as there is no rounding guide for the PWM framework.)

But worse than that is that if the input clock goes back to 250 MHz we
start with real_period = 5592405 and we get

	scale_pow = 1398101
	scale = 4
	real_period = 4194304

so we're not going back to the state we had when the clk was initially
running at 250 MHz.

To get the result independent of the prior configuration you better use
the real targeted period length as input instead of the last configured
approximation.

> +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, val;
> +	unsigned long long num;
> +	int ret;
> +
> +	ret = clk_enable(pwm->clk);
> +	if (ret)
> +		return;

Ideally we'd report state->enabled = 0 if the clk was off. I don't know
how this could be done reliably though.

> +	duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 +
> +		     dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
> +
> +	state->enabled = duty > 0;

If duty is bigger than 0 but PWM_SIFIVE_PWMCFG_EN_ALWAYS isn't set you
should report enabled = false, too.

> +	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> +	val &= 0x0F;

Maybe name that "scale" instead of "val"?

> +	num = 1000000000ULL << (PWM_SIFIVE_CMPWIDTH + val);

Is this (unsigned long long)NSEC_PER_SEC? (Ditto above in
pwm_sifive_update_clock().)

> +	pwm->real_period = div64_ul(num, 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;
> +
> +	clk_disable(pwm->clk);
> +}
> +
> +static int pwm_sifive_enable(struct pwm_chip *chip, bool enable)
> +{
> +	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> +	int ret;
> +
> +	if (enable) {
> +		ret = clk_enable(pwm->clk);
> +		if (ret) {
> +			dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	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, x;
> +	u32 frac;
> +	struct pwm_state cur_state;
> +	bool enabled;
> +	int ret = 0;
> +	unsigned long num;
> +
> +	if (state->polarity != PWM_POLARITY_INVERSED)
> +		return -EINVAL;
> +
> +	mutex_lock(&pwm->lock);
> +	pwm_get_state(dev, &cur_state);
> +	enabled = cur_state.enabled;
> +
> +	if (state->period != cur_state.period) {

Did you test this with more than one consumer? For sure the following
should work:

	pwm1 = pwm_get(.. the first ..);
	pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });

	pwm2 = pwm_get(.. the second ..);
	pwm_apply_state(pwm2, { .enabled = true, .period = 10000000, .... });

but for the second pwm_apply_state() run state->period is likely not
exactly 10000000.

> +		if (pwm->user_count != 1) {
> +			ret = -EINVAL;

EBUSY?

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

It's not ensured that pwm->clk is enabled here which is a pre-condition
to be allowed to call clk_get_rate().

> +	}
> +
> +	duty_cycle = state->duty_cycle;
> +	if (!state->enabled)
> +		duty_cycle = 0;
> +
> +	x = 1U << PWM_SIFIVE_CMPWIDTH;

"x" is a bad name.

> +	num = (u64)duty_cycle * x + x / 2;
> +	frac = div_u64(num, state->period);

I don't understand the "+ x / 2" part. Should this better be
"+ state->period / 2"? Something like

#define div_u64_round(a, b) ({typeof(b) __b = b; div_u64(a + __b / 2, __b)})

would make this less error prone.

> +	/* The hardware cannot generate a 100% duty cycle */
> +	frac = min(frac, x - 1);
> +
> +	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
> +	       dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
> +
> +	if (!state->enabled && enabled) {
> +		ret = pwm_sifive_enable(chip, false);
> +		if (ret)
> +			goto exit;
> +		enabled = false;
> +	}
> +
> +	if (state->enabled && !enabled) {
> +		ret = pwm_sifive_enable(chip, state->enabled);
> +		if (ret)
> +			goto exit;
> +	}

These two ifs can be combined to:

	if (state->enabled != enabled)
		ret = pwm_sifive_enable(chip, state->enabled);

> +
> +exit:
> +	mutex_unlock(&pwm->lock);
> +	return ret;
> +}
> +
> +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, ch;
> +	bool is_enabled = false;
> +
> +	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	mutex_init(&pwm->lock);
> +	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;
> +	}

After pwmchip_add is called the first consumer might appear...

> +	/* Initialize PWM */
> +	pwm->real_period = PWM_SIFIVE_DEFAULT_PERIOD;
> +	pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
> +
> +	for (ch = 0; ch < pwm->chip.npwm; ch++) {
> +		if (pwm_is_enabled(&pwm->chip.pwms[ch])) {
> +			is_enabled = true;
> +			break;
> +		}
> +	}
> +	if (!is_enabled)
> +		clk_disable(pwm->clk);

... so this should better be called after these initialisations.
(This also means you must not use pwm_is_enabled(), but I'd consider that
an upside, because this function is for PWM consumers, not
implementors.)

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

Best regards
Uwe

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

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

[Uwe Kleine-König]

Hello,

On Fri, Mar 01, 2019 at 04:23:19PM +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 | 345 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 357 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..6679ec7
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,345 @@
> +// 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
> + *
> + * Limitations:
> + * - 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).
> + * - The hardware cannot generate a 100% duty cycle.
> + * - The hardware generates only inverted output.
> + */
> +#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	BIT(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

It's a bit inconsistent to have one of them use BIT and the others not.
For consistency please use BIT for all defines. (They are unused
anyhow.) For PWM_SIFIVE_PWMCFG_SCALE use:

	#define PWM_SIFIVE_PWMCFG_SCALE GENMASK(3, 0)

and then FIELD_GET and FIELD_PREP to access the values.

> +/* PWM_SIFIVE_SIZE_PWMCMP is used to calculate offset for pwmcmpX registers */
> +#define PWM_SIFIVE_SIZE_PWMCMP		4
> +#define PWM_SIFIVE_CMPWIDTH		16
> +#define PWM_SIFIVE_DEFAULT_PERIOD	10000000
> +
> +struct pwm_sifive_ddata {
> +	struct pwm_chip	chip;
> +	struct mutex lock; /* lock to protect user_count */
> +	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);
> +
> +	mutex_lock(&pwm->lock);
> +	pwm->user_count++;
> +	mutex_unlock(&pwm->lock);
> +
> +	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);
> +
> +	mutex_lock(&pwm->lock);
> +	pwm->user_count--;
> +	mutex_unlock(&pwm->lock);
> +}
> +
> +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> +				    unsigned long rate)
> +{
> +	u32 val;
> +	unsigned long long num;
> +	/* (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);

I tried for some time to verify the code here and this would have been
easier with a more verbose comment. Something like:

/*
 * The PWM unit is used with pwmzerocmp=0, so the only way to modify the
 * period length is using pwmscale which provides the number of bits the
 * counter is shifted before being feed to the comparators. A period
 * lasts (1 << (PWM_SIFIVE_CMPWIDTH + pwmscale)) clock ticks.
 */

There is a bad rounding effect here. I don't know the machine's details,
and so will consider a parent clock running at 250 MHz. So one clock tick
is 4 ns long and the smallest period length is 4 ns << 16 == 262144 ns.
Consider further an initial target period of 10000000 ns (which is
PWM_SIFIVE_DEFAULT_PERIOD).

The calculation here results in scale_pow = 2500000 and so scale = 5.

> +	val = PWM_SIFIVE_PWMCFG_EN_ALWAYS | (scale << PWM_SIFIVE_PWMCFG_SCALE);
> +	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> +	/* As scale <= 15 the shift operation cannot overflow. */
> +	num = 1000000000ULL << (PWM_SIFIVE_CMPWIDTH + scale);
> +	pwm->real_period = div64_ul(num, rate);
> +	dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
> +}

Then real_period ends up being 8388608 ns. If now the input clk increases
to 750 MHz it is 8388608 ns which is being used as target period length
and the calculation results in:

	scale_pow = 6291456
	scale = 6
	real_period = 5592405

I'd claim it would be better to use scale = 7 here which results in
11184810 which is nearer to the initially targeted 10000000 ns. (But we
cannot be sure as there is no rounding guide for the PWM framework.)

But worse than that is that if the input clock goes back to 250 MHz we
start with real_period = 5592405 and we get

	scale_pow = 1398101
	scale = 4
	real_period = 4194304

so we're not going back to the state we had when the clk was initially
running at 250 MHz.

To get the result independent of the prior configuration you better use
the real targeted period length as input instead of the last configured
approximation.

> +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, val;
> +	unsigned long long num;
> +	int ret;
> +
> +	ret = clk_enable(pwm->clk);
> +	if (ret)
> +		return;

Ideally we'd report state->enabled = 0 if the clk was off. I don't know
how this could be done reliably though.

> +	duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 +
> +		     dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
> +
> +	state->enabled = duty > 0;

If duty is bigger than 0 but PWM_SIFIVE_PWMCFG_EN_ALWAYS isn't set you
should report enabled = false, too.

> +	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> +	val &= 0x0F;

Maybe name that "scale" instead of "val"?

> +	num = 1000000000ULL << (PWM_SIFIVE_CMPWIDTH + val);

Is this (unsigned long long)NSEC_PER_SEC? (Ditto above in
pwm_sifive_update_clock().)

> +	pwm->real_period = div64_ul(num, 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;
> +
> +	clk_disable(pwm->clk);
> +}
> +
> +static int pwm_sifive_enable(struct pwm_chip *chip, bool enable)
> +{
> +	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> +	int ret;
> +
> +	if (enable) {
> +		ret = clk_enable(pwm->clk);
> +		if (ret) {
> +			dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	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, x;
> +	u32 frac;
> +	struct pwm_state cur_state;
> +	bool enabled;
> +	int ret = 0;
> +	unsigned long num;
> +
> +	if (state->polarity != PWM_POLARITY_INVERSED)
> +		return -EINVAL;
> +
> +	mutex_lock(&pwm->lock);
> +	pwm_get_state(dev, &cur_state);
> +	enabled = cur_state.enabled;
> +
> +	if (state->period != cur_state.period) {

Did you test this with more than one consumer? For sure the following
should work:

	pwm1 = pwm_get(.. the first ..);
	pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });

	pwm2 = pwm_get(.. the second ..);
	pwm_apply_state(pwm2, { .enabled = true, .period = 10000000, .... });

but for the second pwm_apply_state() run state->period is likely not
exactly 10000000.

> +		if (pwm->user_count != 1) {
> +			ret = -EINVAL;

EBUSY?

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

It's not ensured that pwm->clk is enabled here which is a pre-condition
to be allowed to call clk_get_rate().

> +	}
> +
> +	duty_cycle = state->duty_cycle;
> +	if (!state->enabled)
> +		duty_cycle = 0;
> +
> +	x = 1U << PWM_SIFIVE_CMPWIDTH;

"x" is a bad name.

> +	num = (u64)duty_cycle * x + x / 2;
> +	frac = div_u64(num, state->period);

I don't understand the "+ x / 2" part. Should this better be
"+ state->period / 2"? Something like

#define div_u64_round(a, b) ({typeof(b) __b = b; div_u64(a + __b / 2, __b)})

would make this less error prone.

> +	/* The hardware cannot generate a 100% duty cycle */
> +	frac = min(frac, x - 1);
> +
> +	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
> +	       dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
> +
> +	if (!state->enabled && enabled) {
> +		ret = pwm_sifive_enable(chip, false);
> +		if (ret)
> +			goto exit;
> +		enabled = false;
> +	}
> +
> +	if (state->enabled && !enabled) {
> +		ret = pwm_sifive_enable(chip, state->enabled);
> +		if (ret)
> +			goto exit;
> +	}

These two ifs can be combined to:

	if (state->enabled != enabled)
		ret = pwm_sifive_enable(chip, state->enabled);

> +
> +exit:
> +	mutex_unlock(&pwm->lock);
> +	return ret;
> +}
> +
> +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, ch;
> +	bool is_enabled = false;
> +
> +	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	mutex_init(&pwm->lock);
> +	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;
> +	}

After pwmchip_add is called the first consumer might appear...

> +	/* Initialize PWM */
> +	pwm->real_period = PWM_SIFIVE_DEFAULT_PERIOD;
> +	pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
> +
> +	for (ch = 0; ch < pwm->chip.npwm; ch++) {
> +		if (pwm_is_enabled(&pwm->chip.pwms[ch])) {
> +			is_enabled = true;
> +			break;
> +		}
> +	}
> +	if (!is_enabled)
> +		clk_disable(pwm->clk);

... so this should better be called after these initialisations.
(This also means you must not use pwm_is_enabled(), but I'd consider that
an upside, because this function is for PWM consumers, not
implementors.)

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

Best regards
Uwe

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Yash Shah]

On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Fri, Mar 01, 2019 at 04:23:19PM +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 | 345 +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 357 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..6679ec7
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,345 @@
> > +// 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
> > + *
> > + * Limitations:
> > + * - 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).
> > + * - The hardware cannot generate a 100% duty cycle.
> > + * - The hardware generates only inverted output.
> > + */
> > +#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  BIT(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
>
> It's a bit inconsistent to have one of them use BIT and the others not.
> For consistency please use BIT for all defines. (They are unused
> anyhow.) For PWM_SIFIVE_PWMCFG_SCALE use:
>
>         #define PWM_SIFIVE_PWMCFG_SCALE GENMASK(3, 0)
>
> and then FIELD_GET and FIELD_PREP to access the values.

Sure will do that.

>
> > +/* PWM_SIFIVE_SIZE_PWMCMP is used to calculate offset for pwmcmpX registers */
> > +#define PWM_SIFIVE_SIZE_PWMCMP               4
> > +#define PWM_SIFIVE_CMPWIDTH          16
> > +#define PWM_SIFIVE_DEFAULT_PERIOD    10000000
> > +
> > +struct pwm_sifive_ddata {
> > +     struct pwm_chip chip;
> > +     struct mutex lock; /* lock to protect user_count */
> > +     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);
> > +
> > +     mutex_lock(&pwm->lock);
> > +     pwm->user_count++;
> > +     mutex_unlock(&pwm->lock);
> > +
> > +     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);
> > +
> > +     mutex_lock(&pwm->lock);
> > +     pwm->user_count--;
> > +     mutex_unlock(&pwm->lock);
> > +}
> > +
> > +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> > +                                 unsigned long rate)
> > +{
> > +     u32 val;
> > +     unsigned long long num;
> > +     /* (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);
>
> I tried for some time to verify the code here and this would have been
> easier with a more verbose comment. Something like:
>
> /*
>  * The PWM unit is used with pwmzerocmp=0, so the only way to modify the
>  * period length is using pwmscale which provides the number of bits the
>  * counter is shifted before being feed to the comparators. A period
>  * lasts (1 << (PWM_SIFIVE_CMPWIDTH + pwmscale)) clock ticks.
>  */

Ok, will add this.

>
> There is a bad rounding effect here. I don't know the machine's details,
> and so will consider a parent clock running at 250 MHz. So one clock tick
> is 4 ns long and the smallest period length is 4 ns << 16 == 262144 ns.
> Consider further an initial target period of 10000000 ns (which is
> PWM_SIFIVE_DEFAULT_PERIOD).
>
> The calculation here results in scale_pow = 2500000 and so scale = 5.
>
> > +     val = PWM_SIFIVE_PWMCFG_EN_ALWAYS | (scale << PWM_SIFIVE_PWMCFG_SCALE);
> > +     writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > +     /* As scale <= 15 the shift operation cannot overflow. */
> > +     num = 1000000000ULL << (PWM_SIFIVE_CMPWIDTH + scale);
> > +     pwm->real_period = div64_ul(num, rate);
> > +     dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
> > +}
>
> Then real_period ends up being 8388608 ns. If now the input clk increases
> to 750 MHz it is 8388608 ns which is being used as target period length
> and the calculation results in:
>
>         scale_pow = 6291456
>         scale = 6
>         real_period = 5592405
>
> I'd claim it would be better to use scale = 7 here which results in
> 11184810 which is nearer to the initially targeted 10000000 ns. (But we
> cannot be sure as there is no rounding guide for the PWM framework.)
>
> But worse than that is that if the input clock goes back to 250 MHz we
> start with real_period = 5592405 and we get
>
>         scale_pow = 1398101
>         scale = 4
>         real_period = 4194304
>
> so we're not going back to the state we had when the clk was initially
> running at 250 MHz.
>
> To get the result independent of the prior configuration you better use
> the real targeted period length as input instead of the last configured
> approximation.

Sure. will change it.

>
> > +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, val;
> > +     unsigned long long num;
> > +     int ret;
> > +
> > +     ret = clk_enable(pwm->clk);
> > +     if (ret)
> > +             return;
>
> Ideally we'd report state->enabled = 0 if the clk was off. I don't know
> how this could be done reliably though.
>
> > +     duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 +
> > +                  dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
> > +
> > +     state->enabled = duty > 0;
>
> If duty is bigger than 0 but PWM_SIFIVE_PWMCFG_EN_ALWAYS isn't set you
> should report enabled = false, too.

Will add this.

>
> > +     val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > +     val &= 0x0F;
>
> Maybe name that "scale" instead of "val"?

Yes, will change it.

>
> > +     num = 1000000000ULL << (PWM_SIFIVE_CMPWIDTH + val);
>
> Is this (unsigned long long)NSEC_PER_SEC? (Ditto above in
> pwm_sifive_update_clock().)

Yes, will change this too.

>
> > +     pwm->real_period = div64_ul(num, 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;
> > +
> > +     clk_disable(pwm->clk);
> > +}
> > +
> > +static int pwm_sifive_enable(struct pwm_chip *chip, bool enable)
> > +{
> > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +     int ret;
> > +
> > +     if (enable) {
> > +             ret = clk_enable(pwm->clk);
> > +             if (ret) {
> > +                     dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     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, x;
> > +     u32 frac;
> > +     struct pwm_state cur_state;
> > +     bool enabled;
> > +     int ret = 0;
> > +     unsigned long num;
> > +
> > +     if (state->polarity != PWM_POLARITY_INVERSED)
> > +             return -EINVAL;
> > +
> > +     mutex_lock(&pwm->lock);
> > +     pwm_get_state(dev, &cur_state);
> > +     enabled = cur_state.enabled;
> > +
> > +     if (state->period != cur_state.period) {
>
> Did you test this with more than one consumer? For sure the following
> should work:
>
>         pwm1 = pwm_get(.. the first ..);
>         pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });
>
>         pwm2 = pwm_get(.. the second ..);
>         pwm_apply_state(pwm2, { .enabled = true, .period = 10000000, .... });
>
> but for the second pwm_apply_state() run state->period is likely not
> exactly 10000000.

Yes, I have tested multiple consumers using sysfs interface. It is working.

>
> > +             if (pwm->user_count != 1) {
> > +                     ret = -EINVAL;
>
> EBUSY?

ok

>
> > +                     goto exit;
> > +             }
> > +             pwm->real_period = state->period;
> > +             pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
>
> It's not ensured that pwm->clk is enabled here which is a pre-condition
> to be allowed to call clk_get_rate().

Will fix this

>
> > +     }
> > +
> > +     duty_cycle = state->duty_cycle;
> > +     if (!state->enabled)
> > +             duty_cycle = 0;
> > +
> > +     x = 1U << PWM_SIFIVE_CMPWIDTH;
>
> "x" is a bad name.
>
> > +     num = (u64)duty_cycle * x + x / 2;
> > +     frac = div_u64(num, state->period);
>
> I don't understand the "+ x / 2" part. Should this better be
> "+ state->period / 2"? Something like

This eqn is as per your comments against v5 of this patch series.
 frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 <<
PWM_SIFIVE_CMPWIDTH) / 2) / period;

>
> #define div_u64_round(a, b) ({typeof(b) __b = b; div_u64(a + __b / 2, __b)})
>
> would make this less error prone.
>
> > +     /* The hardware cannot generate a 100% duty cycle */
> > +     frac = min(frac, x - 1);
> > +
> > +     writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
> > +            dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
> > +
> > +     if (!state->enabled && enabled) {
> > +             ret = pwm_sifive_enable(chip, false);
> > +             if (ret)
> > +                     goto exit;
> > +             enabled = false;
> > +     }
> > +
> > +     if (state->enabled && !enabled) {
> > +             ret = pwm_sifive_enable(chip, state->enabled);
> > +             if (ret)
> > +                     goto exit;
> > +     }
>
> These two ifs can be combined to:
>
>         if (state->enabled != enabled)
>                 ret = pwm_sifive_enable(chip, state->enabled);

Ok, will do that

>
> > +
> > +exit:
> > +     mutex_unlock(&pwm->lock);
> > +     return ret;
> > +}
> > +
> > +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, ch;
> > +     bool is_enabled = false;
> > +
> > +     pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> > +     if (!pwm)
> > +             return -ENOMEM;
> > +
> > +     mutex_init(&pwm->lock);
> > +     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;
> > +     }
>
> After pwmchip_add is called the first consumer might appear...
>
> > +     /* Initialize PWM */
> > +     pwm->real_period = PWM_SIFIVE_DEFAULT_PERIOD;
> > +     pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
> > +
> > +     for (ch = 0; ch < pwm->chip.npwm; ch++) {
> > +             if (pwm_is_enabled(&pwm->chip.pwms[ch])) {
> > +                     is_enabled = true;
> > +                     break;
> > +             }
> > +     }
> > +     if (!is_enabled)
> > +             clk_disable(pwm->clk);
>
> ... so this should better be called after these initialisations.
> (This also means you must not use pwm_is_enabled(), but I'd consider that
> an upside, because this function is for PWM consumers, not
> implementors.)

Ok, will change the sequence.

>
> > +     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;
> > +}
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Yash Shah]

On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Fri, Mar 01, 2019 at 04:23:19PM +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 | 345 +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 357 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..6679ec7
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,345 @@
> > +// 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
> > + *
> > + * Limitations:
> > + * - 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).
> > + * - The hardware cannot generate a 100% duty cycle.
> > + * - The hardware generates only inverted output.
> > + */
> > +#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  BIT(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
>
> It's a bit inconsistent to have one of them use BIT and the others not.
> For consistency please use BIT for all defines. (They are unused
> anyhow.) For PWM_SIFIVE_PWMCFG_SCALE use:
>
>         #define PWM_SIFIVE_PWMCFG_SCALE GENMASK(3, 0)
>
> and then FIELD_GET and FIELD_PREP to access the values.

Sure will do that.

>
> > +/* PWM_SIFIVE_SIZE_PWMCMP is used to calculate offset for pwmcmpX registers */
> > +#define PWM_SIFIVE_SIZE_PWMCMP               4
> > +#define PWM_SIFIVE_CMPWIDTH          16
> > +#define PWM_SIFIVE_DEFAULT_PERIOD    10000000
> > +
> > +struct pwm_sifive_ddata {
> > +     struct pwm_chip chip;
> > +     struct mutex lock; /* lock to protect user_count */
> > +     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);
> > +
> > +     mutex_lock(&pwm->lock);
> > +     pwm->user_count++;
> > +     mutex_unlock(&pwm->lock);
> > +
> > +     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);
> > +
> > +     mutex_lock(&pwm->lock);
> > +     pwm->user_count--;
> > +     mutex_unlock(&pwm->lock);
> > +}
> > +
> > +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> > +                                 unsigned long rate)
> > +{
> > +     u32 val;
> > +     unsigned long long num;
> > +     /* (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);
>
> I tried for some time to verify the code here and this would have been
> easier with a more verbose comment. Something like:
>
> /*
>  * The PWM unit is used with pwmzerocmp=0, so the only way to modify the
>  * period length is using pwmscale which provides the number of bits the
>  * counter is shifted before being feed to the comparators. A period
>  * lasts (1 << (PWM_SIFIVE_CMPWIDTH + pwmscale)) clock ticks.
>  */

Ok, will add this.

>
> There is a bad rounding effect here. I don't know the machine's details,
> and so will consider a parent clock running at 250 MHz. So one clock tick
> is 4 ns long and the smallest period length is 4 ns << 16 == 262144 ns.
> Consider further an initial target period of 10000000 ns (which is
> PWM_SIFIVE_DEFAULT_PERIOD).
>
> The calculation here results in scale_pow = 2500000 and so scale = 5.
>
> > +     val = PWM_SIFIVE_PWMCFG_EN_ALWAYS | (scale << PWM_SIFIVE_PWMCFG_SCALE);
> > +     writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > +     /* As scale <= 15 the shift operation cannot overflow. */
> > +     num = 1000000000ULL << (PWM_SIFIVE_CMPWIDTH + scale);
> > +     pwm->real_period = div64_ul(num, rate);
> > +     dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
> > +}
>
> Then real_period ends up being 8388608 ns. If now the input clk increases
> to 750 MHz it is 8388608 ns which is being used as target period length
> and the calculation results in:
>
>         scale_pow = 6291456
>         scale = 6
>         real_period = 5592405
>
> I'd claim it would be better to use scale = 7 here which results in
> 11184810 which is nearer to the initially targeted 10000000 ns. (But we
> cannot be sure as there is no rounding guide for the PWM framework.)
>
> But worse than that is that if the input clock goes back to 250 MHz we
> start with real_period = 5592405 and we get
>
>         scale_pow = 1398101
>         scale = 4
>         real_period = 4194304
>
> so we're not going back to the state we had when the clk was initially
> running at 250 MHz.
>
> To get the result independent of the prior configuration you better use
> the real targeted period length as input instead of the last configured
> approximation.

Sure. will change it.

>
> > +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, val;
> > +     unsigned long long num;
> > +     int ret;
> > +
> > +     ret = clk_enable(pwm->clk);
> > +     if (ret)
> > +             return;
>
> Ideally we'd report state->enabled = 0 if the clk was off. I don't know
> how this could be done reliably though.
>
> > +     duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 +
> > +                  dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
> > +
> > +     state->enabled = duty > 0;
>
> If duty is bigger than 0 but PWM_SIFIVE_PWMCFG_EN_ALWAYS isn't set you
> should report enabled = false, too.

Will add this.

>
> > +     val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > +     val &= 0x0F;
>
> Maybe name that "scale" instead of "val"?

Yes, will change it.

>
> > +     num = 1000000000ULL << (PWM_SIFIVE_CMPWIDTH + val);
>
> Is this (unsigned long long)NSEC_PER_SEC? (Ditto above in
> pwm_sifive_update_clock().)

Yes, will change this too.

>
> > +     pwm->real_period = div64_ul(num, 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;
> > +
> > +     clk_disable(pwm->clk);
> > +}
> > +
> > +static int pwm_sifive_enable(struct pwm_chip *chip, bool enable)
> > +{
> > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +     int ret;
> > +
> > +     if (enable) {
> > +             ret = clk_enable(pwm->clk);
> > +             if (ret) {
> > +                     dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     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, x;
> > +     u32 frac;
> > +     struct pwm_state cur_state;
> > +     bool enabled;
> > +     int ret = 0;
> > +     unsigned long num;
> > +
> > +     if (state->polarity != PWM_POLARITY_INVERSED)
> > +             return -EINVAL;
> > +
> > +     mutex_lock(&pwm->lock);
> > +     pwm_get_state(dev, &cur_state);
> > +     enabled = cur_state.enabled;
> > +
> > +     if (state->period != cur_state.period) {
>
> Did you test this with more than one consumer? For sure the following
> should work:
>
>         pwm1 = pwm_get(.. the first ..);
>         pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });
>
>         pwm2 = pwm_get(.. the second ..);
>         pwm_apply_state(pwm2, { .enabled = true, .period = 10000000, .... });
>
> but for the second pwm_apply_state() run state->period is likely not
> exactly 10000000.

Yes, I have tested multiple consumers using sysfs interface. It is working.

>
> > +             if (pwm->user_count != 1) {
> > +                     ret = -EINVAL;
>
> EBUSY?

ok

>
> > +                     goto exit;
> > +             }
> > +             pwm->real_period = state->period;
> > +             pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
>
> It's not ensured that pwm->clk is enabled here which is a pre-condition
> to be allowed to call clk_get_rate().

Will fix this

>
> > +     }
> > +
> > +     duty_cycle = state->duty_cycle;
> > +     if (!state->enabled)
> > +             duty_cycle = 0;
> > +
> > +     x = 1U << PWM_SIFIVE_CMPWIDTH;
>
> "x" is a bad name.
>
> > +     num = (u64)duty_cycle * x + x / 2;
> > +     frac = div_u64(num, state->period);
>
> I don't understand the "+ x / 2" part. Should this better be
> "+ state->period / 2"? Something like

This eqn is as per your comments against v5 of this patch series.
 frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 <<
PWM_SIFIVE_CMPWIDTH) / 2) / period;

>
> #define div_u64_round(a, b) ({typeof(b) __b = b; div_u64(a + __b / 2, __b)})
>
> would make this less error prone.
>
> > +     /* The hardware cannot generate a 100% duty cycle */
> > +     frac = min(frac, x - 1);
> > +
> > +     writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
> > +            dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
> > +
> > +     if (!state->enabled && enabled) {
> > +             ret = pwm_sifive_enable(chip, false);
> > +             if (ret)
> > +                     goto exit;
> > +             enabled = false;
> > +     }
> > +
> > +     if (state->enabled && !enabled) {
> > +             ret = pwm_sifive_enable(chip, state->enabled);
> > +             if (ret)
> > +                     goto exit;
> > +     }
>
> These two ifs can be combined to:
>
>         if (state->enabled != enabled)
>                 ret = pwm_sifive_enable(chip, state->enabled);

Ok, will do that

>
> > +
> > +exit:
> > +     mutex_unlock(&pwm->lock);
> > +     return ret;
> > +}
> > +
> > +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, ch;
> > +     bool is_enabled = false;
> > +
> > +     pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> > +     if (!pwm)
> > +             return -ENOMEM;
> > +
> > +     mutex_init(&pwm->lock);
> > +     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;
> > +     }
>
> After pwmchip_add is called the first consumer might appear...
>
> > +     /* Initialize PWM */
> > +     pwm->real_period = PWM_SIFIVE_DEFAULT_PERIOD;
> > +     pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
> > +
> > +     for (ch = 0; ch < pwm->chip.npwm; ch++) {
> > +             if (pwm_is_enabled(&pwm->chip.pwms[ch])) {
> > +                     is_enabled = true;
> > +                     break;
> > +             }
> > +     }
> > +     if (!is_enabled)
> > +             clk_disable(pwm->clk);
>
> ... so this should better be called after these initialisations.
> (This also means you must not use pwm_is_enabled(), but I'd consider that
> an upside, because this function is for PWM consumers, not
> implementors.)

Ok, will change the sequence.

>
> > +     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;
> > +}
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Uwe Kleine-König]

Hello,

On Fri, Mar 08, 2019 at 04:59:36PM +0530, Yash Shah wrote:
> On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > > +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, x;
> > > +     u32 frac;
> > > +     struct pwm_state cur_state;
> > > +     bool enabled;
> > > +     int ret = 0;
> > > +     unsigned long num;
> > > +
> > > +     if (state->polarity != PWM_POLARITY_INVERSED)
> > > +             return -EINVAL;
> > > +
> > > +     mutex_lock(&pwm->lock);
> > > +     pwm_get_state(dev, &cur_state);
> > > +     enabled = cur_state.enabled;
> > > +
> > > +     if (state->period != cur_state.period) {
> >
> > Did you test this with more than one consumer? For sure the following
> > should work:
> >
> >         pwm1 = pwm_get(.. the first ..);
> >         pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });
> >
> >         pwm2 = pwm_get(.. the second ..);
> >         pwm_apply_state(pwm2, { .enabled = true, .period = 10000000, .... });
> >
> > but for the second pwm_apply_state() run state->period is likely not
> > exactly 10000000.
> 
> Yes, I have tested multiple consumers using sysfs interface. It is working.

Can you provide details about your testing here? What is the parent clk
rate? Which settings did you test? Can you confirm my claim that the
above sequence would fail or point out my error in reasoning?

> > > +     x = 1U << PWM_SIFIVE_CMPWIDTH;
> > > +     num = (u64)duty_cycle * x + x / 2;
> > > +     frac = div_u64(num, state->period);
> >
> > I don't understand the "+ x / 2" part. Should this better be
> > "+ state->period / 2"? Something like
> 
> This eqn is as per your comments against v5 of this patch series.
>  frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 <<
> PWM_SIFIVE_CMPWIDTH) / 2) / period;

OK, then not only the code is wrong, but also my suggestion was. :-)

> > #define div_u64_round(a, b) ({typeof(b) __b = b; div_u64(a + __b / 2, __b)})
> >
> > would make this less error prone.

This still stands. It makes it easier to get the code right and makes it
easier to understand.

Best regards
Uwe

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

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

[Uwe Kleine-König]

Hello,

On Fri, Mar 08, 2019 at 04:59:36PM +0530, Yash Shah wrote:
> On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > > +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, x;
> > > +     u32 frac;
> > > +     struct pwm_state cur_state;
> > > +     bool enabled;
> > > +     int ret = 0;
> > > +     unsigned long num;
> > > +
> > > +     if (state->polarity != PWM_POLARITY_INVERSED)
> > > +             return -EINVAL;
> > > +
> > > +     mutex_lock(&pwm->lock);
> > > +     pwm_get_state(dev, &cur_state);
> > > +     enabled = cur_state.enabled;
> > > +
> > > +     if (state->period != cur_state.period) {
> >
> > Did you test this with more than one consumer? For sure the following
> > should work:
> >
> >         pwm1 = pwm_get(.. the first ..);
> >         pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });
> >
> >         pwm2 = pwm_get(.. the second ..);
> >         pwm_apply_state(pwm2, { .enabled = true, .period = 10000000, .... });
> >
> > but for the second pwm_apply_state() run state->period is likely not
> > exactly 10000000.
> 
> Yes, I have tested multiple consumers using sysfs interface. It is working.

Can you provide details about your testing here? What is the parent clk
rate? Which settings did you test? Can you confirm my claim that the
above sequence would fail or point out my error in reasoning?

> > > +     x = 1U << PWM_SIFIVE_CMPWIDTH;
> > > +     num = (u64)duty_cycle * x + x / 2;
> > > +     frac = div_u64(num, state->period);
> >
> > I don't understand the "+ x / 2" part. Should this better be
> > "+ state->period / 2"? Something like
> 
> This eqn is as per your comments against v5 of this patch series.
>  frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 <<
> PWM_SIFIVE_CMPWIDTH) / 2) / period;

OK, then not only the code is wrong, but also my suggestion was. :-)

> > #define div_u64_round(a, b) ({typeof(b) __b = b; div_u64(a + __b / 2, __b)})
> >
> > would make this less error prone.

This still stands. It makes it easier to get the code right and makes it
easier to understand.

Best regards
Uwe

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Yash Shah]

On Fri, Mar 8, 2019 at 5:27 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Fri, Mar 08, 2019 at 04:59:36PM +0530, Yash Shah wrote:
> > On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > > +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, x;
> > > > +     u32 frac;
> > > > +     struct pwm_state cur_state;
> > > > +     bool enabled;
> > > > +     int ret = 0;
> > > > +     unsigned long num;
> > > > +
> > > > +     if (state->polarity != PWM_POLARITY_INVERSED)
> > > > +             return -EINVAL;
> > > > +
> > > > +     mutex_lock(&pwm->lock);
> > > > +     pwm_get_state(dev, &cur_state);
> > > > +     enabled = cur_state.enabled;
> > > > +
> > > > +     if (state->period != cur_state.period) {
> > >
> > > Did you test this with more than one consumer? For sure the following
> > > should work:
> > >
> > >         pwm1 = pwm_get(.. the first ..);
> > >         pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });
> > >
> > >         pwm2 = pwm_get(.. the second ..);
> > >         pwm_apply_state(pwm2, { .enabled = true, .period = 10000000, .... });
> > >
> > > but for the second pwm_apply_state() run state->period is likely not
> > > exactly 10000000.
> >
> > Yes, I have tested multiple consumers using sysfs interface. It is working.
>
> Can you provide details about your testing here? What is the parent clk
> rate? Which settings did you test? Can you confirm my claim that the
> above sequence would fail or point out my error in reasoning?
>

I have tested on HiFive Unleashed board using sysfs interface.
Parent clk rate is around 512 Mhz.
I have tested scenarios as you mentioned above with various period and
duty_cycle values.

After considering your below suggestion,
 | To get the result independent of the prior configuration you better use
 | the real targeted period length as input instead of the last configured
 | approximation
I will introduce approx_period feild, which will be used as the
targeted period length.
Also, in pwm_sifive_get_state, I will make below change
- state->period = pwm->real_period;
+ state->period = pwm->approx_period.
So with this change in place, I believe the cur_state.period for the
second pwm_apply_state() above (pwm2) will be exactly 10000000

> > > > +     x = 1U << PWM_SIFIVE_CMPWIDTH;
> > > > +     num = (u64)duty_cycle * x + x / 2;
> > > > +     frac = div_u64(num, state->period);
> > >
> > > I don't understand the "+ x / 2" part. Should this better be
> > > "+ state->period / 2"? Something like
> >
> > This eqn is as per your comments against v5 of this patch series.
> >  frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 <<
> > PWM_SIFIVE_CMPWIDTH) / 2) / period;
>
> OK, then not only the code is wrong, but also my suggestion was. :-)
>
> > > #define div_u64_round(a, b) ({typeof(b) __b = b; div_u64(a + __b / 2, __b)})
> > >
> > > would make this less error prone.
>
> This still stands. It makes it easier to get the code right and makes it
> easier to understand.

Sure, will implement this.

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

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

[Yash Shah]

On Fri, Mar 8, 2019 at 5:27 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Fri, Mar 08, 2019 at 04:59:36PM +0530, Yash Shah wrote:
> > On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > > +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, x;
> > > > +     u32 frac;
> > > > +     struct pwm_state cur_state;
> > > > +     bool enabled;
> > > > +     int ret = 0;
> > > > +     unsigned long num;
> > > > +
> > > > +     if (state->polarity != PWM_POLARITY_INVERSED)
> > > > +             return -EINVAL;
> > > > +
> > > > +     mutex_lock(&pwm->lock);
> > > > +     pwm_get_state(dev, &cur_state);
> > > > +     enabled = cur_state.enabled;
> > > > +
> > > > +     if (state->period != cur_state.period) {
> > >
> > > Did you test this with more than one consumer? For sure the following
> > > should work:
> > >
> > >         pwm1 = pwm_get(.. the first ..);
> > >         pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });
> > >
> > >         pwm2 = pwm_get(.. the second ..);
> > >         pwm_apply_state(pwm2, { .enabled = true, .period = 10000000, .... });
> > >
> > > but for the second pwm_apply_state() run state->period is likely not
> > > exactly 10000000.
> >
> > Yes, I have tested multiple consumers using sysfs interface. It is working.
>
> Can you provide details about your testing here? What is the parent clk
> rate? Which settings did you test? Can you confirm my claim that the
> above sequence would fail or point out my error in reasoning?
>

I have tested on HiFive Unleashed board using sysfs interface.
Parent clk rate is around 512 Mhz.
I have tested scenarios as you mentioned above with various period and
duty_cycle values.

After considering your below suggestion,
 | To get the result independent of the prior configuration you better use
 | the real targeted period length as input instead of the last configured
 | approximation
I will introduce approx_period feild, which will be used as the
targeted period length.
Also, in pwm_sifive_get_state, I will make below change
- state->period = pwm->real_period;
+ state->period = pwm->approx_period.
So with this change in place, I believe the cur_state.period for the
second pwm_apply_state() above (pwm2) will be exactly 10000000

> > > > +     x = 1U << PWM_SIFIVE_CMPWIDTH;
> > > > +     num = (u64)duty_cycle * x + x / 2;
> > > > +     frac = div_u64(num, state->period);
> > >
> > > I don't understand the "+ x / 2" part. Should this better be
> > > "+ state->period / 2"? Something like
> >
> > This eqn is as per your comments against v5 of this patch series.
> >  frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 <<
> > PWM_SIFIVE_CMPWIDTH) / 2) / period;
>
> OK, then not only the code is wrong, but also my suggestion was. :-)
>
> > > #define div_u64_round(a, b) ({typeof(b) __b = b; div_u64(a + __b / 2, __b)})
> > >
> > > would make this less error prone.
>
> This still stands. It makes it easier to get the code right and makes it
> easier to understand.

Sure, will implement this.

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Uwe Kleine-König]

Hello,

On Mon, Mar 11, 2019 at 05:10:17PM +0530, Yash Shah wrote:
> On Fri, Mar 8, 2019 at 5:27 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Mar 08, 2019 at 04:59:36PM +0530, Yash Shah wrote:
> > > On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > +     if (state->period != cur_state.period) {
> > > >
> > > > Did you test this with more than one consumer? For sure the following
> > > > should work:
> > > >
> > > >         pwm1 = pwm_get(.. the first ..);
> > > >         pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });
> > > >
> > > >         pwm2 = pwm_get(.. the second ..);
> > > >         pwm_apply_state(pwm2, { .enabled = true, .period = 10000000, .... });
> > > >
> > > > but for the second pwm_apply_state() run state->period is likely not
> > > > exactly 10000000.
> > >
> > > Yes, I have tested multiple consumers using sysfs interface. It is working.
> >
> > Can you provide details about your testing here? What is the parent clk
> > rate? Which settings did you test? Can you confirm my claim that the
> > above sequence would fail or point out my error in reasoning?
> >
> 
> I have tested on HiFive Unleashed board using sysfs interface.
> Parent clk rate is around 512 Mhz.
> I have tested scenarios as you mentioned above with various period and
> duty_cycle values.
> 
> After considering your below suggestion,
>  | To get the result independent of the prior configuration you better use
>  | the real targeted period length as input instead of the last configured
>  | approximation
> I will introduce approx_period feild, which will be used as the
> targeted period length.
> Also, in pwm_sifive_get_state, I will make below change
> - state->period = pwm->real_period;
> + state->period = pwm->approx_period.
> So with this change in place, I believe the cur_state.period for the
> second pwm_apply_state() above (pwm2) will be exactly 10000000

I don't understand your intention completely. Just send a new patch
round, then I will gladly take another look.

Best regards
Uwe

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

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

[Uwe Kleine-König]

Hello,

On Mon, Mar 11, 2019 at 05:10:17PM +0530, Yash Shah wrote:
> On Fri, Mar 8, 2019 at 5:27 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Mar 08, 2019 at 04:59:36PM +0530, Yash Shah wrote:
> > > On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > +     if (state->period != cur_state.period) {
> > > >
> > > > Did you test this with more than one consumer? For sure the following
> > > > should work:
> > > >
> > > >         pwm1 = pwm_get(.. the first ..);
> > > >         pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });
> > > >
> > > >         pwm2 = pwm_get(.. the second ..);
> > > >         pwm_apply_state(pwm2, { .enabled = true, .period = 10000000, .... });
> > > >
> > > > but for the second pwm_apply_state() run state->period is likely not
> > > > exactly 10000000.
> > >
> > > Yes, I have tested multiple consumers using sysfs interface. It is working.
> >
> > Can you provide details about your testing here? What is the parent clk
> > rate? Which settings did you test? Can you confirm my claim that the
> > above sequence would fail or point out my error in reasoning?
> >
> 
> I have tested on HiFive Unleashed board using sysfs interface.
> Parent clk rate is around 512 Mhz.
> I have tested scenarios as you mentioned above with various period and
> duty_cycle values.
> 
> After considering your below suggestion,
>  | To get the result independent of the prior configuration you better use
>  | the real targeted period length as input instead of the last configured
>  | approximation
> I will introduce approx_period feild, which will be used as the
> targeted period length.
> Also, in pwm_sifive_get_state, I will make below change
> - state->period = pwm->real_period;
> + state->period = pwm->approx_period.
> So with this change in place, I believe the cur_state.period for the
> second pwm_apply_state() above (pwm2) will be exactly 10000000

I don't understand your intention completely. Just send a new patch
round, then I will gladly take another look.

Best regards
Uwe

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Yash Shah]

On Mon, Mar 11, 2019 at 6:59 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Mon, Mar 11, 2019 at 05:10:17PM +0530, Yash Shah wrote:
> > On Fri, Mar 8, 2019 at 5:27 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Fri, Mar 08, 2019 at 04:59:36PM +0530, Yash Shah wrote:
> > > > On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > +     if (state->period != cur_state.period) {
> > > > >
> > > > > Did you test this with more than one consumer? For sure the following
> > > > > should work:
> > > > >
> > > > >         pwm1 = pwm_get(.. the first ..);
> > > > >         pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });
> > > > >
> > > > >         pwm2 = pwm_get(.. the second ..);
> > > > >         pwm_apply_state(pwm2, { .enabled = true, .period = 10000000, .... });
> > > > >
> > > > > but for the second pwm_apply_state() run state->period is likely not
> > > > > exactly 10000000.
> > > >
> > > > Yes, I have tested multiple consumers using sysfs interface. It is working.
> > >
> > > Can you provide details about your testing here? What is the parent clk
> > > rate? Which settings did you test? Can you confirm my claim that the
> > > above sequence would fail or point out my error in reasoning?
> > >
> >
> > I have tested on HiFive Unleashed board using sysfs interface.
> > Parent clk rate is around 512 Mhz.
> > I have tested scenarios as you mentioned above with various period and
> > duty_cycle values.
> >
> > After considering your below suggestion,
> >  | To get the result independent of the prior configuration you better use
> >  | the real targeted period length as input instead of the last configured
> >  | approximation
> > I will introduce approx_period feild, which will be used as the
> > targeted period length.
> > Also, in pwm_sifive_get_state, I will make below change
> > - state->period = pwm->real_period;
> > + state->period = pwm->approx_period.
> > So with this change in place, I believe the cur_state.period for the
> > second pwm_apply_state() above (pwm2) will be exactly 10000000
>
> I don't understand your intention completely. Just send a new patch
> round, then I will gladly take another look.

I was planning to go with above-mentioned change but then I realized
that pwm state should always reflect the current hardware state, so I
have dropped the above idea.
Coming back to your concern on

 |     if (state->period != cur_state.period) {

It is not failing for the scenario you mentioned but failing for
another way around.

         pwm1 = pwm_get(.. the first ..);
         pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });

         pwm2 = pwm_get(.. the second ..);
         pwm_apply_state(pwm2, { .enabled = true, .period = 20000000, .... });

The pwm2 should get an error for period mismatch but with v8 patch, it
is not getting any error.

I am sending a v9 patch which has the fix for this.
With v9 changes, I have tested all scenarios and it is working fine.

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

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

[Yash Shah]

On Mon, Mar 11, 2019 at 6:59 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Mon, Mar 11, 2019 at 05:10:17PM +0530, Yash Shah wrote:
> > On Fri, Mar 8, 2019 at 5:27 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Fri, Mar 08, 2019 at 04:59:36PM +0530, Yash Shah wrote:
> > > > On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > +     if (state->period != cur_state.period) {
> > > > >
> > > > > Did you test this with more than one consumer? For sure the following
> > > > > should work:
> > > > >
> > > > >         pwm1 = pwm_get(.. the first ..);
> > > > >         pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });
> > > > >
> > > > >         pwm2 = pwm_get(.. the second ..);
> > > > >         pwm_apply_state(pwm2, { .enabled = true, .period = 10000000, .... });
> > > > >
> > > > > but for the second pwm_apply_state() run state->period is likely not
> > > > > exactly 10000000.
> > > >
> > > > Yes, I have tested multiple consumers using sysfs interface. It is working.
> > >
> > > Can you provide details about your testing here? What is the parent clk
> > > rate? Which settings did you test? Can you confirm my claim that the
> > > above sequence would fail or point out my error in reasoning?
> > >
> >
> > I have tested on HiFive Unleashed board using sysfs interface.
> > Parent clk rate is around 512 Mhz.
> > I have tested scenarios as you mentioned above with various period and
> > duty_cycle values.
> >
> > After considering your below suggestion,
> >  | To get the result independent of the prior configuration you better use
> >  | the real targeted period length as input instead of the last configured
> >  | approximation
> > I will introduce approx_period feild, which will be used as the
> > targeted period length.
> > Also, in pwm_sifive_get_state, I will make below change
> > - state->period = pwm->real_period;
> > + state->period = pwm->approx_period.
> > So with this change in place, I believe the cur_state.period for the
> > second pwm_apply_state() above (pwm2) will be exactly 10000000
>
> I don't understand your intention completely. Just send a new patch
> round, then I will gladly take another look.

I was planning to go with above-mentioned change but then I realized
that pwm state should always reflect the current hardware state, so I
have dropped the above idea.
Coming back to your concern on

 |     if (state->period != cur_state.period) {

It is not failing for the scenario you mentioned but failing for
another way around.

         pwm1 = pwm_get(.. the first ..);
         pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });

         pwm2 = pwm_get(.. the second ..);
         pwm_apply_state(pwm2, { .enabled = true, .period = 20000000, .... });

The pwm2 should get an error for period mismatch but with v8 patch, it
is not getting any error.

I am sending a v9 patch which has the fix for this.
With v9 changes, I have tested all scenarios and it is working fine.

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv