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

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

v9
- Use appropriate bitfield macros
- Add approx_period in pwm_sifive_ddata struct and related changes
- Correct the eqn for calculation of frac (in pwm_sifive_apply)
- Other minor fixes

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                           | 338 +++++++++++++++++++++
 4 files changed, 383 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
 create mode 100644 drivers/pwm/pwm-sifive.c

-- 
1.9.1

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

v9
- Use appropriate bitfield macros
- Add approx_period in pwm_sifive_ddata struct and related changes
- Correct the eqn for calculation of frac (in pwm_sifive_apply)
- Other minor fixes

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                           | 338 +++++++++++++++++++++
 4 files changed, 383 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 v9 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 v9 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 v9 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 | 338 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 350 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..4233557
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,338 @@
+// 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>
+#include <linux/bitfield.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		GENMASK(3, 0)
+#define PWM_SIFIVE_PWMCFG_STICKY	BIT(8)
+#define PWM_SIFIVE_PWMCFG_ZERO_CMP	BIT(9)
+#define PWM_SIFIVE_PWMCFG_DEGLITCH	BIT(10)
+#define PWM_SIFIVE_PWMCFG_EN_ALWAYS	BIT(12)
+#define PWM_SIFIVE_PWMCFG_EN_ONCE	BIT(13)
+#define PWM_SIFIVE_PWMCFG_CENTER	BIT(16)
+#define PWM_SIFIVE_PWMCFG_GANG		BIT(24)
+#define PWM_SIFIVE_PWMCFG_IP		BIT(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
+
+#define div_u64_round(a, b) \
+	({typeof(b) __b = b; div_u64((a) + __b / 2, __b); })
+
+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;
+	unsigned int approx_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;
+	/*
+	 * 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.
+	 * (1 << (PWM_SIFIVE_CMPWIDTH + scale)) * 10^9/rate = period
+	 */
+	unsigned long scale_pow =
+			div64_ul(pwm->approx_period * (u64)rate, NSEC_PER_SEC);
+	int scale = clamp(ilog2(scale_pow) - PWM_SIFIVE_CMPWIDTH, 0, 0xf);
+
+	val = PWM_SIFIVE_PWMCFG_EN_ALWAYS |
+	      FIELD_PREP(PWM_SIFIVE_PWMCFG_SCALE, scale);
+	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
+
+	/* As scale <= 15 the shift operation cannot overflow. */
+	num = (unsigned long long)NSEC_PER_SEC << (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;
+
+	duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 +
+		     dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
+
+	state->enabled = duty > 0;
+
+	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
+	if (!(val & PWM_SIFIVE_PWMCFG_EN_ALWAYS))
+		state->enabled = false;
+
+	state->period = pwm->real_period;
+	state->duty_cycle =
+		(u64)duty * pwm->real_period >> PWM_SIFIVE_CMPWIDTH;
+	state->polarity = PWM_POLARITY_INVERSED;
+}
+
+static int pwm_sifive_enable(struct pwm_chip *chip, 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;
+	u32 frac;
+	struct pwm_state cur_state;
+	bool enabled;
+	int ret = 0;
+	unsigned long num;
+
+	if (state->polarity != PWM_POLARITY_INVERSED)
+		return -EINVAL;
+
+	ret = clk_enable(pwm->clk);
+	if (ret) {
+		dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
+		return ret;
+	}
+
+	mutex_lock(&pwm->lock);
+	pwm_get_state(dev, &cur_state);
+
+	enabled = cur_state.enabled;
+
+	if (state->period != pwm->approx_period) {
+		if (pwm->user_count != 1) {
+			ret = -EBUSY;
+			goto exit;
+		}
+		pwm->approx_period = state->period;
+		pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
+	}
+
+	duty_cycle = state->duty_cycle;
+	if (!state->enabled)
+		duty_cycle = 0;
+
+	num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
+	frac = div_u64_round(num, state->period);
+	/* The hardware cannot generate a 100% duty cycle */
+	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
+
+	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
+	       dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
+
+	if (state->enabled != enabled) {
+		ret = pwm_sifive_enable(chip, state->enabled);
+		if (ret)
+			goto exit;
+	}
+
+exit:
+	clk_disable(pwm->clk);
+	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;
+
+	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;
+	}
+
+	/* Initialize PWM */
+	pwm->approx_period = PWM_SIFIVE_DEFAULT_PERIOD;
+	pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
+
+	ret = pwmchip_add(chip);
+	if (ret < 0) {
+		dev_err(dev, "cannot register PWM: %d\n", ret);
+		goto unregister_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 v9 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 | 338 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 350 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..4233557
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,338 @@
+// 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>
+#include <linux/bitfield.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		GENMASK(3, 0)
+#define PWM_SIFIVE_PWMCFG_STICKY	BIT(8)
+#define PWM_SIFIVE_PWMCFG_ZERO_CMP	BIT(9)
+#define PWM_SIFIVE_PWMCFG_DEGLITCH	BIT(10)
+#define PWM_SIFIVE_PWMCFG_EN_ALWAYS	BIT(12)
+#define PWM_SIFIVE_PWMCFG_EN_ONCE	BIT(13)
+#define PWM_SIFIVE_PWMCFG_CENTER	BIT(16)
+#define PWM_SIFIVE_PWMCFG_GANG		BIT(24)
+#define PWM_SIFIVE_PWMCFG_IP		BIT(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
+
+#define div_u64_round(a, b) \
+	({typeof(b) __b = b; div_u64((a) + __b / 2, __b); })
+
+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;
+	unsigned int approx_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;
+	/*
+	 * 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.
+	 * (1 << (PWM_SIFIVE_CMPWIDTH + scale)) * 10^9/rate = period
+	 */
+	unsigned long scale_pow =
+			div64_ul(pwm->approx_period * (u64)rate, NSEC_PER_SEC);
+	int scale = clamp(ilog2(scale_pow) - PWM_SIFIVE_CMPWIDTH, 0, 0xf);
+
+	val = PWM_SIFIVE_PWMCFG_EN_ALWAYS |
+	      FIELD_PREP(PWM_SIFIVE_PWMCFG_SCALE, scale);
+	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
+
+	/* As scale <= 15 the shift operation cannot overflow. */
+	num = (unsigned long long)NSEC_PER_SEC << (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;
+
+	duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 +
+		     dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
+
+	state->enabled = duty > 0;
+
+	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
+	if (!(val & PWM_SIFIVE_PWMCFG_EN_ALWAYS))
+		state->enabled = false;
+
+	state->period = pwm->real_period;
+	state->duty_cycle =
+		(u64)duty * pwm->real_period >> PWM_SIFIVE_CMPWIDTH;
+	state->polarity = PWM_POLARITY_INVERSED;
+}
+
+static int pwm_sifive_enable(struct pwm_chip *chip, 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;
+	u32 frac;
+	struct pwm_state cur_state;
+	bool enabled;
+	int ret = 0;
+	unsigned long num;
+
+	if (state->polarity != PWM_POLARITY_INVERSED)
+		return -EINVAL;
+
+	ret = clk_enable(pwm->clk);
+	if (ret) {
+		dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
+		return ret;
+	}
+
+	mutex_lock(&pwm->lock);
+	pwm_get_state(dev, &cur_state);
+
+	enabled = cur_state.enabled;
+
+	if (state->period != pwm->approx_period) {
+		if (pwm->user_count != 1) {
+			ret = -EBUSY;
+			goto exit;
+		}
+		pwm->approx_period = state->period;
+		pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
+	}
+
+	duty_cycle = state->duty_cycle;
+	if (!state->enabled)
+		duty_cycle = 0;
+
+	num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
+	frac = div_u64_round(num, state->period);
+	/* The hardware cannot generate a 100% duty cycle */
+	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
+
+	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
+	       dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
+
+	if (state->enabled != enabled) {
+		ret = pwm_sifive_enable(chip, state->enabled);
+		if (ret)
+			goto exit;
+	}
+
+exit:
+	clk_disable(pwm->clk);
+	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;
+
+	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;
+	}
+
+	/* Initialize PWM */
+	pwm->approx_period = PWM_SIFIVE_DEFAULT_PERIOD;
+	pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
+
+	ret = pwmchip_add(chip);
+	if (ret < 0) {
+		dev_err(dev, "cannot register PWM: %d\n", ret);
+		goto unregister_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

[PATCH v9 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 | 338 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 350 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..4233557
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,338 @@
+// 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>
+#include <linux/bitfield.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		GENMASK(3, 0)
+#define PWM_SIFIVE_PWMCFG_STICKY	BIT(8)
+#define PWM_SIFIVE_PWMCFG_ZERO_CMP	BIT(9)
+#define PWM_SIFIVE_PWMCFG_DEGLITCH	BIT(10)
+#define PWM_SIFIVE_PWMCFG_EN_ALWAYS	BIT(12)
+#define PWM_SIFIVE_PWMCFG_EN_ONCE	BIT(13)
+#define PWM_SIFIVE_PWMCFG_CENTER	BIT(16)
+#define PWM_SIFIVE_PWMCFG_GANG		BIT(24)
+#define PWM_SIFIVE_PWMCFG_IP		BIT(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
+
+#define div_u64_round(a, b) \
+	({typeof(b) __b = b; div_u64((a) + __b / 2, __b); })
+
+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;
+	unsigned int approx_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;
+	/*
+	 * 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.
+	 * (1 << (PWM_SIFIVE_CMPWIDTH + scale)) * 10^9/rate = period
+	 */
+	unsigned long scale_pow =
+			div64_ul(pwm->approx_period * (u64)rate, NSEC_PER_SEC);
+	int scale = clamp(ilog2(scale_pow) - PWM_SIFIVE_CMPWIDTH, 0, 0xf);
+
+	val = PWM_SIFIVE_PWMCFG_EN_ALWAYS |
+	      FIELD_PREP(PWM_SIFIVE_PWMCFG_SCALE, scale);
+	writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
+
+	/* As scale <= 15 the shift operation cannot overflow. */
+	num = (unsigned long long)NSEC_PER_SEC << (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;
+
+	duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 +
+		     dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
+
+	state->enabled = duty > 0;
+
+	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
+	if (!(val & PWM_SIFIVE_PWMCFG_EN_ALWAYS))
+		state->enabled = false;
+
+	state->period = pwm->real_period;
+	state->duty_cycle =
+		(u64)duty * pwm->real_period >> PWM_SIFIVE_CMPWIDTH;
+	state->polarity = PWM_POLARITY_INVERSED;
+}
+
+static int pwm_sifive_enable(struct pwm_chip *chip, 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;
+	u32 frac;
+	struct pwm_state cur_state;
+	bool enabled;
+	int ret = 0;
+	unsigned long num;
+
+	if (state->polarity != PWM_POLARITY_INVERSED)
+		return -EINVAL;
+
+	ret = clk_enable(pwm->clk);
+	if (ret) {
+		dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
+		return ret;
+	}
+
+	mutex_lock(&pwm->lock);
+	pwm_get_state(dev, &cur_state);
+
+	enabled = cur_state.enabled;
+
+	if (state->period != pwm->approx_period) {
+		if (pwm->user_count != 1) {
+			ret = -EBUSY;
+			goto exit;
+		}
+		pwm->approx_period = state->period;
+		pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
+	}
+
+	duty_cycle = state->duty_cycle;
+	if (!state->enabled)
+		duty_cycle = 0;
+
+	num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
+	frac = div_u64_round(num, state->period);
+	/* The hardware cannot generate a 100% duty cycle */
+	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
+
+	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
+	       dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
+
+	if (state->enabled != enabled) {
+		ret = pwm_sifive_enable(chip, state->enabled);
+		if (ret)
+			goto exit;
+	}
+
+exit:
+	clk_disable(pwm->clk);
+	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;
+
+	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;
+	}
+
+	/* Initialize PWM */
+	pwm->approx_period = PWM_SIFIVE_DEFAULT_PERIOD;
+	pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
+
+	ret = pwmchip_add(chip);
+	if (ret < 0) {
+		dev_err(dev, "cannot register PWM: %d\n", ret);
+		goto unregister_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

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

[Uwe Kleine-König]

Hello,

there are just a few minor things left I commented below.

On Tue, Mar 12, 2019 at 01:41:29PM +0530, Yash Shah wrote:
> +#define div_u64_round(a, b) \
> +	({typeof(b) __b = b; div_u64((a) + __b / 2, __b); })

Parenthesis around b please. I guess I didn't had them in my suggestion
either, sorry for that.

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

There is only a single caller for this function. I wonder if it is
trivial enough to fold it into pwm_sifive_apply.

> +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> +			    struct pwm_state *state)
> +{
> +	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> +	unsigned int duty_cycle;
> +	u32 frac;
> +	struct pwm_state cur_state;
> +	bool enabled;
> +	int ret = 0;
> +	unsigned long num;
> +
> +	if (state->polarity != PWM_POLARITY_INVERSED)
> +		return -EINVAL;
> +
> +	ret = clk_enable(pwm->clk);
> +	if (ret) {
> +		dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> +		return ret;
> +	}
> +
> +	mutex_lock(&pwm->lock);
> +	pwm_get_state(dev, &cur_state);
> +
> +	enabled = cur_state.enabled;
> +
> +	if (state->period != pwm->approx_period) {
> +		if (pwm->user_count != 1) {
> +			ret = -EBUSY;
> +			goto exit;
> +		}
> +		pwm->approx_period = state->period;
> +		pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
> +	}
> +
> +	duty_cycle = state->duty_cycle;
> +	if (!state->enabled)
> +		duty_cycle = 0;
> +
> +	num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
> +	frac = div_u64_round(num, state->period);
> +	/* The hardware cannot generate a 100% duty cycle */
> +	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> +
> +	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
> +	       dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
> +
> +	if (state->enabled != enabled) {
> +		ret = pwm_sifive_enable(chip, state->enabled);
> +		if (ret)
> +			goto exit;

This goto is a noop.

> +	}
> +
> +exit:
> +	clk_disable(pwm->clk);
> +	mutex_unlock(&pwm->lock);
> +	return ret;
> +}

There are a few other things that could be improved, but I think they
could be addressed later. For some of these I don't even know what to
suggest, for some Thierry might not agree it is worth fixing:

 - rounding
   how to round? When should a request declined, when is rounding ok?
   There is still "if (state->period != pwm->approx_period) return -EBUSY"
   in this driver. This is better than before, but if state-period ==
   pwm->approx_period + 1 the result (if accepted) might be the same as
   without the +1 and so returning -EBUSY for one case and accepting the
   other is strange.
 - don't call PWM API functions designed for consumers (here: pwm_get_state)
 - Move div_u64_round to include/linux/math64.h

Best regards
Uwe

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

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

[Uwe Kleine-König]

Hello,

there are just a few minor things left I commented below.

On Tue, Mar 12, 2019 at 01:41:29PM +0530, Yash Shah wrote:
> +#define div_u64_round(a, b) \
> +	({typeof(b) __b = b; div_u64((a) + __b / 2, __b); })

Parenthesis around b please. I guess I didn't had them in my suggestion
either, sorry for that.

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

There is only a single caller for this function. I wonder if it is
trivial enough to fold it into pwm_sifive_apply.

> +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> +			    struct pwm_state *state)
> +{
> +	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> +	unsigned int duty_cycle;
> +	u32 frac;
> +	struct pwm_state cur_state;
> +	bool enabled;
> +	int ret = 0;
> +	unsigned long num;
> +
> +	if (state->polarity != PWM_POLARITY_INVERSED)
> +		return -EINVAL;
> +
> +	ret = clk_enable(pwm->clk);
> +	if (ret) {
> +		dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> +		return ret;
> +	}
> +
> +	mutex_lock(&pwm->lock);
> +	pwm_get_state(dev, &cur_state);
> +
> +	enabled = cur_state.enabled;
> +
> +	if (state->period != pwm->approx_period) {
> +		if (pwm->user_count != 1) {
> +			ret = -EBUSY;
> +			goto exit;
> +		}
> +		pwm->approx_period = state->period;
> +		pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
> +	}
> +
> +	duty_cycle = state->duty_cycle;
> +	if (!state->enabled)
> +		duty_cycle = 0;
> +
> +	num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
> +	frac = div_u64_round(num, state->period);
> +	/* The hardware cannot generate a 100% duty cycle */
> +	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> +
> +	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
> +	       dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
> +
> +	if (state->enabled != enabled) {
> +		ret = pwm_sifive_enable(chip, state->enabled);
> +		if (ret)
> +			goto exit;

This goto is a noop.

> +	}
> +
> +exit:
> +	clk_disable(pwm->clk);
> +	mutex_unlock(&pwm->lock);
> +	return ret;
> +}

There are a few other things that could be improved, but I think they
could be addressed later. For some of these I don't even know what to
suggest, for some Thierry might not agree it is worth fixing:

 - rounding
   how to round? When should a request declined, when is rounding ok?
   There is still "if (state->period != pwm->approx_period) return -EBUSY"
   in this driver. This is better than before, but if state-period ==
   pwm->approx_period + 1 the result (if accepted) might be the same as
   without the +1 and so returning -EBUSY for one case and accepting the
   other is strange.
 - don't call PWM API functions designed for consumers (here: pwm_get_state)
 - Move div_u64_round to include/linux/math64.h

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 v9 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Uwe Kleine-König]

Hello,

there are just a few minor things left I commented below.

On Tue, Mar 12, 2019 at 01:41:29PM +0530, Yash Shah wrote:
> +#define div_u64_round(a, b) \
> +	({typeof(b) __b = b; div_u64((a) + __b / 2, __b); })

Parenthesis around b please. I guess I didn't had them in my suggestion
either, sorry for that.

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

There is only a single caller for this function. I wonder if it is
trivial enough to fold it into pwm_sifive_apply.

> +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> +			    struct pwm_state *state)
> +{
> +	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> +	unsigned int duty_cycle;
> +	u32 frac;
> +	struct pwm_state cur_state;
> +	bool enabled;
> +	int ret = 0;
> +	unsigned long num;
> +
> +	if (state->polarity != PWM_POLARITY_INVERSED)
> +		return -EINVAL;
> +
> +	ret = clk_enable(pwm->clk);
> +	if (ret) {
> +		dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> +		return ret;
> +	}
> +
> +	mutex_lock(&pwm->lock);
> +	pwm_get_state(dev, &cur_state);
> +
> +	enabled = cur_state.enabled;
> +
> +	if (state->period != pwm->approx_period) {
> +		if (pwm->user_count != 1) {
> +			ret = -EBUSY;
> +			goto exit;
> +		}
> +		pwm->approx_period = state->period;
> +		pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
> +	}
> +
> +	duty_cycle = state->duty_cycle;
> +	if (!state->enabled)
> +		duty_cycle = 0;
> +
> +	num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
> +	frac = div_u64_round(num, state->period);
> +	/* The hardware cannot generate a 100% duty cycle */
> +	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> +
> +	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
> +	       dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
> +
> +	if (state->enabled != enabled) {
> +		ret = pwm_sifive_enable(chip, state->enabled);
> +		if (ret)
> +			goto exit;

This goto is a noop.

> +	}
> +
> +exit:
> +	clk_disable(pwm->clk);
> +	mutex_unlock(&pwm->lock);
> +	return ret;
> +}

There are a few other things that could be improved, but I think they
could be addressed later. For some of these I don't even know what to
suggest, for some Thierry might not agree it is worth fixing:

 - rounding
   how to round? When should a request declined, when is rounding ok?
   There is still "if (state->period != pwm->approx_period) return -EBUSY"
   in this driver. This is better than before, but if state-period ==
   pwm->approx_period + 1 the result (if accepted) might be the same as
   without the +1 and so returning -EBUSY for one case and accepting the
   other is strange.
 - don't call PWM API functions designed for consumers (here: pwm_get_state)
 - Move div_u64_round to include/linux/math64.h

Best regards
Uwe

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

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

[Thierry Reding]

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

On Tue, Mar 12, 2019 at 10:17:39AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> there are just a few minor things left I commented below.
> 
> On Tue, Mar 12, 2019 at 01:41:29PM +0530, Yash Shah wrote:
> > +#define div_u64_round(a, b) \
> > +	({typeof(b) __b = b; div_u64((a) + __b / 2, __b); })
> 
> Parenthesis around b please. I guess I didn't had them in my suggestion
> either, sorry for that.

We don't really need the parentheses here, do we? The only operator that
has lower priority than the assignment is the comma operator, and that's
not going to work in the macro anyway, unless you put it inside a pair
of parentheses, in which case, well, you have the parentheses already.

> > +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;
> > +}
> 
> There is only a single caller for this function. I wonder if it is
> trivial enough to fold it into pwm_sifive_apply.

I think this is fine. pwm_sifive_apply() is already fairly long at this
point, so might as well split things up a little.

> > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> > +			    struct pwm_state *state)
> > +{
> > +	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +	unsigned int duty_cycle;
> > +	u32 frac;
> > +	struct pwm_state cur_state;
> > +	bool enabled;
> > +	int ret = 0;
> > +	unsigned long num;
> > +
> > +	if (state->polarity != PWM_POLARITY_INVERSED)
> > +		return -EINVAL;
> > +
> > +	ret = clk_enable(pwm->clk);
> > +	if (ret) {
> > +		dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	mutex_lock(&pwm->lock);
> > +	pwm_get_state(dev, &cur_state);
> > +
> > +	enabled = cur_state.enabled;
> > +
> > +	if (state->period != pwm->approx_period) {
> > +		if (pwm->user_count != 1) {
> > +			ret = -EBUSY;
> > +			goto exit;
> > +		}
> > +		pwm->approx_period = state->period;
> > +		pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
> > +	}
> > +
> > +	duty_cycle = state->duty_cycle;
> > +	if (!state->enabled)
> > +		duty_cycle = 0;
> > +
> > +	num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
> > +	frac = div_u64_round(num, state->period);
> > +	/* The hardware cannot generate a 100% duty cycle */
> > +	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > +
> > +	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
> > +	       dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
> > +
> > +	if (state->enabled != enabled) {
> > +		ret = pwm_sifive_enable(chip, state->enabled);
> > +		if (ret)
> > +			goto exit;
> 
> This goto is a noop.
> 
> > +	}
> > +
> > +exit:
> > +	clk_disable(pwm->clk);
> > +	mutex_unlock(&pwm->lock);
> > +	return ret;
> > +}
> 
> There are a few other things that could be improved, but I think they
> could be addressed later. For some of these I don't even know what to
> suggest, for some Thierry might not agree it is worth fixing:
> 
>  - rounding
>    how to round? When should a request declined, when is rounding ok?
>    There is still "if (state->period != pwm->approx_period) return -EBUSY"
>    in this driver. This is better than before, but if state-period ==
>    pwm->approx_period + 1 the result (if accepted) might be the same as
>    without the +1 and so returning -EBUSY for one case and accepting the
>    other is strange.

Perhaps a good idea would be to reject a configuration only after we've
determined that it is incompatible? If we're really going to end up with
the same configuration within a given margin of period or duty cycle and
we can't do much about it, there's little point in rejecting such
configurations.

>  - don't call PWM API functions designed for consumers (here: pwm_get_state)

Agreed. The driver can just access pwm_device.state directly.

>  - Move div_u64_round to include/linux/math64.h

Looks to me like DIV_ROUND_CLOSEST_ULL() is already pretty much this.
The only difference that I can see is that the divisor is 32-bit, but
since we pass in state->period, that already works fine.

Thierry

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

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4454 bytes --]

On Tue, Mar 12, 2019 at 10:17:39AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> there are just a few minor things left I commented below.
> 
> On Tue, Mar 12, 2019 at 01:41:29PM +0530, Yash Shah wrote:
> > +#define div_u64_round(a, b) \
> > +	({typeof(b) __b = b; div_u64((a) + __b / 2, __b); })
> 
> Parenthesis around b please. I guess I didn't had them in my suggestion
> either, sorry for that.

We don't really need the parentheses here, do we? The only operator that
has lower priority than the assignment is the comma operator, and that's
not going to work in the macro anyway, unless you put it inside a pair
of parentheses, in which case, well, you have the parentheses already.

> > +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;
> > +}
> 
> There is only a single caller for this function. I wonder if it is
> trivial enough to fold it into pwm_sifive_apply.

I think this is fine. pwm_sifive_apply() is already fairly long at this
point, so might as well split things up a little.

> > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> > +			    struct pwm_state *state)
> > +{
> > +	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +	unsigned int duty_cycle;
> > +	u32 frac;
> > +	struct pwm_state cur_state;
> > +	bool enabled;
> > +	int ret = 0;
> > +	unsigned long num;
> > +
> > +	if (state->polarity != PWM_POLARITY_INVERSED)
> > +		return -EINVAL;
> > +
> > +	ret = clk_enable(pwm->clk);
> > +	if (ret) {
> > +		dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	mutex_lock(&pwm->lock);
> > +	pwm_get_state(dev, &cur_state);
> > +
> > +	enabled = cur_state.enabled;
> > +
> > +	if (state->period != pwm->approx_period) {
> > +		if (pwm->user_count != 1) {
> > +			ret = -EBUSY;
> > +			goto exit;
> > +		}
> > +		pwm->approx_period = state->period;
> > +		pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
> > +	}
> > +
> > +	duty_cycle = state->duty_cycle;
> > +	if (!state->enabled)
> > +		duty_cycle = 0;
> > +
> > +	num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
> > +	frac = div_u64_round(num, state->period);
> > +	/* The hardware cannot generate a 100% duty cycle */
> > +	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > +
> > +	writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
> > +	       dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
> > +
> > +	if (state->enabled != enabled) {
> > +		ret = pwm_sifive_enable(chip, state->enabled);
> > +		if (ret)
> > +			goto exit;
> 
> This goto is a noop.
> 
> > +	}
> > +
> > +exit:
> > +	clk_disable(pwm->clk);
> > +	mutex_unlock(&pwm->lock);
> > +	return ret;
> > +}
> 
> There are a few other things that could be improved, but I think they
> could be addressed later. For some of these I don't even know what to
> suggest, for some Thierry might not agree it is worth fixing:
> 
>  - rounding
>    how to round? When should a request declined, when is rounding ok?
>    There is still "if (state->period != pwm->approx_period) return -EBUSY"
>    in this driver. This is better than before, but if state-period ==
>    pwm->approx_period + 1 the result (if accepted) might be the same as
>    without the +1 and so returning -EBUSY for one case and accepting the
>    other is strange.

Perhaps a good idea would be to reject a configuration only after we've
determined that it is incompatible? If we're really going to end up with
the same configuration within a given margin of period or duty cycle and
we can't do much about it, there's little point in rejecting such
configurations.

>  - don't call PWM API functions designed for consumers (here: pwm_get_state)

Agreed. The driver can just access pwm_device.state directly.

>  - Move div_u64_round to include/linux/math64.h

Looks to me like DIV_ROUND_CLOSEST_ULL() is already pretty much this.
The only difference that I can see is that the divisor is 32-bit, but
since we pass in state->period, that already works fine.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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

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

[Uwe Kleine-König]

On Tue, Mar 12, 2019 at 01:12:18PM +0100, Thierry Reding wrote:
> On Tue, Mar 12, 2019 at 10:17:39AM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > there are just a few minor things left I commented below.
> > 
> > On Tue, Mar 12, 2019 at 01:41:29PM +0530, Yash Shah wrote:
> > > +#define div_u64_round(a, b) \
> > > +	({typeof(b) __b = b; div_u64((a) + __b / 2, __b); })
> > 
> > Parenthesis around b please. I guess I didn't had them in my suggestion
> > either, sorry for that.
> 
> We don't really need the parentheses here, do we? The only operator that
> has lower priority than the assignment is the comma operator, and that's
> not going to work in the macro anyway, unless you put it inside a pair
> of parentheses, in which case, well, you have the parentheses already.

I thought that, too, but using parenthesis just always is a safe bet and
prevents people stumbling over this and spending time to come to the
conclusion that it is actually safe without them.

> > > +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;
> > > +}
> > 
> > There is only a single caller for this function. I wonder if it is
> > trivial enough to fold it into pwm_sifive_apply.
> 
> I think this is fine. pwm_sifive_apply() is already fairly long at this
> point, so might as well split things up a little.

I don't have a strong opinion here, so keeping as is is fine for me.

> > There are a few other things that could be improved, but I think they
> > could be addressed later. For some of these I don't even know what to
> > suggest, for some Thierry might not agree it is worth fixing:
> > 
> >  - rounding
> >    how to round? When should a request declined, when is rounding ok?
> >    There is still "if (state->period != pwm->approx_period) return -EBUSY"
> >    in this driver. This is better than before, but if state-period ==
> >    pwm->approx_period + 1 the result (if accepted) might be the same as
> >    without the +1 and so returning -EBUSY for one case and accepting the
> >    other is strange.
> 
> Perhaps a good idea would be to reject a configuration only after we've
> determined that it is incompatible? If we're really going to end up with
> the same configuration within a given margin of period or duty cycle and
> we can't do much about it, there's little point in rejecting such
> configurations.

It seems we agree here. Is this important enough to delay taking this
driver further? Currently the driver rejects too broad so if it annoys
someone this can still be fixed later and there is only little harm
(assuming correct error handling in the consumers).

> >  - don't call PWM API functions designed for consumers (here: pwm_get_state)
> 
> Agreed. The driver can just access pwm_device.state directly.

I wouldn't do this either. IMHO the driver should only look into its
hardware registers instead of using framework interna (or consumer API
calls).

> >  - Move div_u64_round to include/linux/math64.h
> 
> Looks to me like DIV_ROUND_CLOSEST_ULL() is already pretty much this.
> The only difference that I can see is that the divisor is 32-bit, but
> since we pass in state->period, that already works fine.

num (i.e. the divident) should be a u64, but it isn't. This needs
fixing. But I agree that DIV_ROUND_CLOSEST_ULL should be good enough
then.

Best regards
Uwe

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

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

[Uwe Kleine-König]

On Tue, Mar 12, 2019 at 01:12:18PM +0100, Thierry Reding wrote:
> On Tue, Mar 12, 2019 at 10:17:39AM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > there are just a few minor things left I commented below.
> > 
> > On Tue, Mar 12, 2019 at 01:41:29PM +0530, Yash Shah wrote:
> > > +#define div_u64_round(a, b) \
> > > +	({typeof(b) __b = b; div_u64((a) + __b / 2, __b); })
> > 
> > Parenthesis around b please. I guess I didn't had them in my suggestion
> > either, sorry for that.
> 
> We don't really need the parentheses here, do we? The only operator that
> has lower priority than the assignment is the comma operator, and that's
> not going to work in the macro anyway, unless you put it inside a pair
> of parentheses, in which case, well, you have the parentheses already.

I thought that, too, but using parenthesis just always is a safe bet and
prevents people stumbling over this and spending time to come to the
conclusion that it is actually safe without them.

> > > +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;
> > > +}
> > 
> > There is only a single caller for this function. I wonder if it is
> > trivial enough to fold it into pwm_sifive_apply.
> 
> I think this is fine. pwm_sifive_apply() is already fairly long at this
> point, so might as well split things up a little.

I don't have a strong opinion here, so keeping as is is fine for me.

> > There are a few other things that could be improved, but I think they
> > could be addressed later. For some of these I don't even know what to
> > suggest, for some Thierry might not agree it is worth fixing:
> > 
> >  - rounding
> >    how to round? When should a request declined, when is rounding ok?
> >    There is still "if (state->period != pwm->approx_period) return -EBUSY"
> >    in this driver. This is better than before, but if state-period ==
> >    pwm->approx_period + 1 the result (if accepted) might be the same as
> >    without the +1 and so returning -EBUSY for one case and accepting the
> >    other is strange.
> 
> Perhaps a good idea would be to reject a configuration only after we've
> determined that it is incompatible? If we're really going to end up with
> the same configuration within a given margin of period or duty cycle and
> we can't do much about it, there's little point in rejecting such
> configurations.

It seems we agree here. Is this important enough to delay taking this
driver further? Currently the driver rejects too broad so if it annoys
someone this can still be fixed later and there is only little harm
(assuming correct error handling in the consumers).

> >  - don't call PWM API functions designed for consumers (here: pwm_get_state)
> 
> Agreed. The driver can just access pwm_device.state directly.

I wouldn't do this either. IMHO the driver should only look into its
hardware registers instead of using framework interna (or consumer API
calls).

> >  - Move div_u64_round to include/linux/math64.h
> 
> Looks to me like DIV_ROUND_CLOSEST_ULL() is already pretty much this.
> The only difference that I can see is that the divisor is 32-bit, but
> since we pass in state->period, that already works fine.

num (i.e. the divident) should be a u64, but it isn't. This needs
fixing. But I agree that DIV_ROUND_CLOSEST_ULL should be good enough
then.

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 v9 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Thierry Reding]

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

On Tue, Mar 12, 2019 at 02:17:12PM +0100, Uwe Kleine-König wrote:
> On Tue, Mar 12, 2019 at 01:12:18PM +0100, Thierry Reding wrote:
> > On Tue, Mar 12, 2019 at 10:17:39AM +0100, Uwe Kleine-König wrote:
[...]
> > > There are a few other things that could be improved, but I think they
> > > could be addressed later. For some of these I don't even know what to
> > > suggest, for some Thierry might not agree it is worth fixing:
> > > 
> > >  - rounding
> > >    how to round? When should a request declined, when is rounding ok?
> > >    There is still "if (state->period != pwm->approx_period) return -EBUSY"
> > >    in this driver. This is better than before, but if state-period ==
> > >    pwm->approx_period + 1 the result (if accepted) might be the same as
> > >    without the +1 and so returning -EBUSY for one case and accepting the
> > >    other is strange.
> > 
> > Perhaps a good idea would be to reject a configuration only after we've
> > determined that it is incompatible? If we're really going to end up with
> > the same configuration within a given margin of period or duty cycle and
> > we can't do much about it, there's little point in rejecting such
> > configurations.
> 
> It seems we agree here. Is this important enough to delay taking this
> driver further? Currently the driver rejects too broad so if it annoys
> someone this can still be fixed later and there is only little harm
> (assuming correct error handling in the consumers).

I don't think it has to be a blocker. As you said, we'd be giving users
more flexibility, not restricting them, so it should be fine to do later
on.

> > >  - don't call PWM API functions designed for consumers (here: pwm_get_state)
> > 
> > Agreed. The driver can just access pwm_device.state directly.
> 
> I wouldn't do this either. IMHO the driver should only look into its
> hardware registers instead of using framework interna (or consumer API
> calls).

The current hardware state is already the software representation of
what the hardware has programmed. I think it's fair for drivers to make
use of that in order to avoid having to read back from hardware.
Especially if reading back from hardware might require switching on the
module to access registers.

Thierry

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

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2293 bytes --]

On Tue, Mar 12, 2019 at 02:17:12PM +0100, Uwe Kleine-König wrote:
> On Tue, Mar 12, 2019 at 01:12:18PM +0100, Thierry Reding wrote:
> > On Tue, Mar 12, 2019 at 10:17:39AM +0100, Uwe Kleine-König wrote:
[...]
> > > There are a few other things that could be improved, but I think they
> > > could be addressed later. For some of these I don't even know what to
> > > suggest, for some Thierry might not agree it is worth fixing:
> > > 
> > >  - rounding
> > >    how to round? When should a request declined, when is rounding ok?
> > >    There is still "if (state->period != pwm->approx_period) return -EBUSY"
> > >    in this driver. This is better than before, but if state-period ==
> > >    pwm->approx_period + 1 the result (if accepted) might be the same as
> > >    without the +1 and so returning -EBUSY for one case and accepting the
> > >    other is strange.
> > 
> > Perhaps a good idea would be to reject a configuration only after we've
> > determined that it is incompatible? If we're really going to end up with
> > the same configuration within a given margin of period or duty cycle and
> > we can't do much about it, there's little point in rejecting such
> > configurations.
> 
> It seems we agree here. Is this important enough to delay taking this
> driver further? Currently the driver rejects too broad so if it annoys
> someone this can still be fixed later and there is only little harm
> (assuming correct error handling in the consumers).

I don't think it has to be a blocker. As you said, we'd be giving users
more flexibility, not restricting them, so it should be fine to do later
on.

> > >  - don't call PWM API functions designed for consumers (here: pwm_get_state)
> > 
> > Agreed. The driver can just access pwm_device.state directly.
> 
> I wouldn't do this either. IMHO the driver should only look into its
> hardware registers instead of using framework interna (or consumer API
> calls).

The current hardware state is already the software representation of
what the hardware has programmed. I think it's fair for drivers to make
use of that in order to avoid having to read back from hardware.
Especially if reading back from hardware might require switching on the
module to access registers.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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

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

[Andreas Schwab]

On Mär 12 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.

Heartbeat trigger still doesn't work for me.

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 v9 0/2] PWM support for HiFive Unleashed

[Andreas Schwab]

On Mär 12 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.

Heartbeat trigger still doesn't work for me.

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 v9 0/2] PWM support for HiFive Unleashed

[Yash Shah]

On Tue, Mar 12, 2019 at 3:44 PM Andreas Schwab <schwab@suse.de> wrote:
>
> On Mär 12 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.
>
> Heartbeat trigger still doesn't work for me.

You need to make sure the period setting is passed via the
conventional way in DT file.
Example:
pwmleds {
    compatible = "pwm-leds";
    heartbeat {
        pwms = <&L45 0 10000000 0>;
        max-brightness = <255>;
        linux,default-trigger = "heartbeat";
    };
};

I have tested on HiFive unleashed board. To modify DT file I performed
the following steps:

Use the open-source FSBL from:
https://github.com/sifive/freedom-u540-c000-bootloader

Modify the fsbl/ux00_fsbl.dts file and re-build the fsbl.bin

Steps for using fsbl.bin in HiFive Unleashed board:
https://github.com/sifive/freedom-u540-c000-bootloader/issues/9

>
> 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 v9 0/2] PWM support for HiFive Unleashed

[Yash Shah]

On Tue, Mar 12, 2019 at 3:44 PM Andreas Schwab <schwab@suse.de> wrote:
>
> On Mär 12 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.
>
> Heartbeat trigger still doesn't work for me.

You need to make sure the period setting is passed via the
conventional way in DT file.
Example:
pwmleds {
    compatible = "pwm-leds";
    heartbeat {
        pwms = <&L45 0 10000000 0>;
        max-brightness = <255>;
        linux,default-trigger = "heartbeat";
    };
};

I have tested on HiFive unleashed board. To modify DT file I performed
the following steps:

Use the open-source FSBL from:
https://github.com/sifive/freedom-u540-c000-bootloader

Modify the fsbl/ux00_fsbl.dts file and re-build the fsbl.bin

Steps for using fsbl.bin in HiFive Unleashed board:
https://github.com/sifive/freedom-u540-c000-bootloader/issues/9

>
> 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 v9 0/2] PWM support for HiFive Unleashed

[Andreas Schwab]

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

> Use the open-source FSBL from:
> https://github.com/sifive/freedom-u540-c000-bootloader
>
> Modify the fsbl/ux00_fsbl.dts file and re-build the fsbl.bin

That doesn't even compile.

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 v9 0/2] PWM support for HiFive Unleashed

[Andreas Schwab]

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

> Use the open-source FSBL from:
> https://github.com/sifive/freedom-u540-c000-bootloader
>
> Modify the fsbl/ux00_fsbl.dts file and re-build the fsbl.bin

That doesn't even compile.

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 v9 0/2] PWM support for HiFive Unleashed

[Andreas Schwab]

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

> You need to make sure the period setting is passed via the
> conventional way in DT file.
> Example:
> pwmleds {
>     compatible = "pwm-leds";
>     heartbeat {
>         pwms = <&L45 0 10000000 0>;
>         max-brightness = <255>;
>         linux,default-trigger = "heartbeat";
>     };
> };

I've now managed to build a working FSBL with that change, but that
didn't change anything.  There is not even a heartbeat option in
/sys/class/leds/heartbeat/trigger any more.

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 v9 0/2] PWM support for HiFive Unleashed

[Andreas Schwab]

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

> You need to make sure the period setting is passed via the
> conventional way in DT file.
> Example:
> pwmleds {
>     compatible = "pwm-leds";
>     heartbeat {
>         pwms = <&L45 0 10000000 0>;
>         max-brightness = <255>;
>         linux,default-trigger = "heartbeat";
>     };
> };

I've now managed to build a working FSBL with that change, but that
didn't change anything.  There is not even a heartbeat option in
/sys/class/leds/heartbeat/trigger any more.

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 v9 0/2] PWM support for HiFive Unleashed

[Paul Walmsley]

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

Hi Yash


On 3/18/19 10:26 AM, Andreas Schwab wrote:
> On Mär 15 2019, Yash Shah <yash.shah@sifive.com> wrote:
>
>> You need to make sure the period setting is passed via the
>> conventional way in DT file.
>> Example:
>> pwmleds {
>>     compatible = "pwm-leds";
>>     heartbeat {
>>         pwms = <&L45 0 10000000 0>;
>>         max-brightness = <255>;
>>         linux,default-trigger = "heartbeat";
>>     };
>> };
> I've now managed to build a working FSBL with that change, but that
> didn't change anything.  There is not even a heartbeat option in
> /sys/class/leds/heartbeat/trigger any more.


Could you post a copy of the current DTS file and Kconfig that you're
using so that Andreas can duplicate your results?


thanks,


- Paul


[-- Attachment #2: Type: text/html, Size: 1434 bytes --]

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

[Yash Shah]

On Mon, Mar 18, 2019 at 10:56 PM Andreas Schwab <schwab@suse.de> wrote:
>
> On Mär 15 2019, Yash Shah <yash.shah@sifive.com> wrote:
>
> > You need to make sure the period setting is passed via the
> > conventional way in DT file.
> > Example:
> > pwmleds {
> >     compatible = "pwm-leds";
> >     heartbeat {
> >         pwms = <&L45 0 10000000 0>;
> >         max-brightness = <255>;
> >         linux,default-trigger = "heartbeat";
> >     };
> > };
>
> I've now managed to build a working FSBL with that change, but that
> didn't change anything.  There is not even a heartbeat option in
> /sys/class/leds/heartbeat/trigger any more.

Below is the copy of the pwm nodes in my DTS file:

L45: pwm@10020000 {
        compatible = "sifive,pwm0";
        interrupt-parent = <&plic0>;
        interrupts = <42 43 44 45>;
        reg = <0x0 0x10020000 0x0 0x1000>;
        reg-names = "control";
        clocks = <&prci 3>;
        #pwm-cells = <2>;
};
L46: pwm@10021000 {
        compatible = "sifive,pwm0";
        interrupt-parent = <&plic0>;
        interrupts = <46 47 48 49>;
        reg = <0x0 0x10021000 0x0 0x1000>;
        reg-names = "control";
        clocks = <&prci 3>;
        #pwm-cells = <2>;
};
pwmleds {
        compatible = "pwm-leds";
        heartbeat {
                pwms = <&L45 0 10000000 0>;
                max-brightness = <255>;
                linux,default-trigger = "heartbeat";
        };
};

The above works for me.
I just noticed that I have been using pwm-cells = 2, instead of 3.
Maybe that is the problem here.
I will suggest you test it on v11 patch in which I will fix this
pwm-cells issue.

Thanks for pointing it out.

>
> 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 v9 0/2] PWM support for HiFive Unleashed

[Yash Shah]

On Mon, Mar 18, 2019 at 10:56 PM Andreas Schwab <schwab@suse.de> wrote:
>
> On Mär 15 2019, Yash Shah <yash.shah@sifive.com> wrote:
>
> > You need to make sure the period setting is passed via the
> > conventional way in DT file.
> > Example:
> > pwmleds {
> >     compatible = "pwm-leds";
> >     heartbeat {
> >         pwms = <&L45 0 10000000 0>;
> >         max-brightness = <255>;
> >         linux,default-trigger = "heartbeat";
> >     };
> > };
>
> I've now managed to build a working FSBL with that change, but that
> didn't change anything.  There is not even a heartbeat option in
> /sys/class/leds/heartbeat/trigger any more.

Below is the copy of the pwm nodes in my DTS file:

L45: pwm@10020000 {
        compatible = "sifive,pwm0";
        interrupt-parent = <&plic0>;
        interrupts = <42 43 44 45>;
        reg = <0x0 0x10020000 0x0 0x1000>;
        reg-names = "control";
        clocks = <&prci 3>;
        #pwm-cells = <2>;
};
L46: pwm@10021000 {
        compatible = "sifive,pwm0";
        interrupt-parent = <&plic0>;
        interrupts = <46 47 48 49>;
        reg = <0x0 0x10021000 0x0 0x1000>;
        reg-names = "control";
        clocks = <&prci 3>;
        #pwm-cells = <2>;
};
pwmleds {
        compatible = "pwm-leds";
        heartbeat {
                pwms = <&L45 0 10000000 0>;
                max-brightness = <255>;
                linux,default-trigger = "heartbeat";
        };
};

The above works for me.
I just noticed that I have been using pwm-cells = 2, instead of 3.
Maybe that is the problem here.
I will suggest you test it on v11 patch in which I will fix this
pwm-cells issue.

Thanks for pointing it out.

>
> 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 v9 0/2] PWM support for HiFive Unleashed

[Yash Shah]

Hi Andreas,

On Tue, Mar 19, 2019 at 11:56 AM Yash Shah <yash.shah@sifive.com> wrote:
>
> On Mon, Mar 18, 2019 at 10:56 PM Andreas Schwab <schwab@suse.de> wrote:
> >
> > On Mär 15 2019, Yash Shah <yash.shah@sifive.com> wrote:
> >
> > > You need to make sure the period setting is passed via the
> > > conventional way in DT file.
> > > Example:
> > > pwmleds {
> > >     compatible = "pwm-leds";
> > >     heartbeat {
> > >         pwms = <&L45 0 10000000 0>;
> > >         max-brightness = <255>;
> > >         linux,default-trigger = "heartbeat";
> > >     };
> > > };
> >
> > I've now managed to build a working FSBL with that change, but that
> > didn't change anything.  There is not even a heartbeat option in
> > /sys/class/leds/heartbeat/trigger any more.
>
...
>
> The above works for me.
> I just noticed that I have been using pwm-cells = 2, instead of 3.
> Maybe that is the problem here.
> I will suggest you test it on v11 patch in which I will fix this
> pwm-cells issue.

I have sent out the v11 patchset, you can test the heartbeat
application with that patchset.
You still need to make that DT file modification which you previously
did, using fsbl.bin
Just for your reference, I am copying my DT file and kernel config
which I used for my test.
The same is available at dev/yashs/pwm_5.0-rc1 branch of
https://github.com/yashshah7/riscv-linux.git

/dts-v1/;

/*#include <linux/clk/sifive-fu540-prci.h>*/
#define PRCI_CLK_TLCLK 3

/ {
#address-cells = <2>;
#size-cells = <2>;
compatible = "sifive,fu540-c000";

aliases {
serial0 = &uart0;
serial1 = &uart1;
};

chosen {
};

cpus {
#address-cells = <1>;
#size-cells = <0>;
timebase-frequency = <1000000>;
cpu0: cpu@0 {
clock-frequency = <0>;
compatible = "sifive,u51", "sifive,rocket0", "riscv";
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <128>;
i-cache-size = <16384>;
reg = <0>;
riscv,isa = "rv64imac";
status = "okay";
cpu0_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu1: cpu@1 {
clock-frequency = <0>;
compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
d-cache-block-size = <64>;
d-cache-sets = <64>;
d-cache-size = <32768>;
d-tlb-sets = <1>;
d-tlb-size = <32>;
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <64>;
i-cache-size = <32768>;
i-tlb-sets = <1>;
i-tlb-size = <32>;
mmu-type = "riscv,sv39";
reg = <1>;
riscv,isa = "rv64imafdc";
status = "okay";
tlb-split;
cpu1_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu2: cpu@2 {
clock-frequency = <0>;
compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
d-cache-block-size = <64>;
d-cache-sets = <64>;
d-cache-size = <32768>;
d-tlb-sets = <1>;
d-tlb-size = <32>;
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <64>;
i-cache-size = <32768>;
i-tlb-sets = <1>;
i-tlb-size = <32>;
mmu-type = "riscv,sv39";
reg = <2>;
riscv,isa = "rv64imafdc";
status = "okay";
tlb-split;
cpu2_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu3: cpu@3 {
clock-frequency = <0>;
compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
d-cache-block-size = <64>;
d-cache-sets = <64>;
d-cache-size = <32768>;
d-tlb-sets = <1>;
d-tlb-size = <32>;
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <64>;
i-cache-size = <32768>;
i-tlb-sets = <1>;
i-tlb-size = <32>;
mmu-type = "riscv,sv39";
reg = <3>;
riscv,isa = "rv64imafdc";
status = "okay";
tlb-split;
cpu3_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu4: cpu@4 {
clock-frequency = <0>;
compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
d-cache-block-size = <64>;
d-cache-sets = <64>;
d-cache-size = <32768>;
d-tlb-sets = <1>;
d-tlb-size = <32>;
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <64>;
i-cache-size = <32768>;
i-tlb-sets = <1>;
i-tlb-size = <32>;
mmu-type = "riscv,sv39";
reg = <4>;
riscv,isa = "rv64imafdc";
status = "okay";
tlb-split;
cpu4_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
};
soc {
#address-cells = <2>;
#size-cells = <2>;
compatible = "sifive,fu540-soc", "simple-bus";
ranges;
prci: prci@10000000 {
compatible = "sifive,fu540-c000-prci";
reg = <0x0 0x10000000 0x0 0x1000>;
clocks = <&hfclk>, <&rtcclk>;
#clock-cells = <1>;
};
uart0: serial@10010000 {
compatible = "sifive,fu540-c000-uart", "sifive,uart0";
interrupt-parent = <&plic0>;
interrupts = <4>;
reg = <0x0 0x10010000 0x0 0x1000>;
clocks = <&prci PRCI_CLK_TLCLK>;
};
uart1: serial@10011000 {
compatible = "sifive,fu540-c000-uart", "sifive,uart0";
interrupt-parent = <&plic0>;
interrupts = <5>;
reg = <0x0 0x10011000 0x0 0x1000>;
clocks = <&prci PRCI_CLK_TLCLK>;
};
L5: clint@2000000 {
compatible = "riscv,clint0";
interrupts-extended = <
&cpu0_intc 3 &cpu0_intc 7
&cpu1_intc 3 &cpu1_intc 7
&cpu2_intc 3 &cpu2_intc 7
&cpu3_intc 3 &cpu3_intc 7
&cpu4_intc 3 &cpu4_intc 7>;
reg = <0x0 0x2000000 0x0 0x10000>;
                };
plic0: interrupt-controller@c000000 {
#interrupt-cells = <1>;
compatible = "riscv,plic0";
interrupt-controller;
interrupts-extended = <
&cpu0_intc 11
&cpu1_intc 11 &cpu1_intc 9
&cpu2_intc 11 &cpu2_intc 9
&cpu3_intc 11 &cpu3_intc 9
&cpu4_intc 11 &cpu4_intc 9>;
reg = <0x0 0xc000000 0x0 0x4000000>;
                        riscv,ndev = <53>;
};
L45: pwm@10020000 {
                        compatible = "sifive,pwm0";
                        interrupt-parent = <&plic0>;
                        interrupts = <42 43 44 45>;
                        reg = <0x0 0x10020000 0x0 0x1000>;
                        reg-names = "control";
                        clocks = <&prci 3>;
                        #pwm-cells = <3>;
                };
                L46: pwm@10021000 {
                        compatible = "sifive,pwm0";
                        interrupt-parent = <&plic0>;
                        interrupts = <46 47 48 49>;
                        reg = <0x0 0x10021000 0x0 0x1000>;
                        reg-names = "control";
                        clocks = <&prci 3>;
                        #pwm-cells = <3>;
                };
pwmleds {
                        compatible = "pwm-leds";
                        heartbeat {
                                pwms = <&L45 0 100000 0>;
                                max-brightness = <255>;
                                linux,default-trigger = "heartbeat";
                        };
                        mtd {
                                pwms = <&L45 1 100000 0>;
                                max-brightness = <255>;
                                linux,default-trigger = "mtd";
                        };
                        netdev {
                                pwms = <&L45 2 100000 0>;
                                max-brightness = <255>;
                                linux,default-trigger = "netdev";
                        };
                        panic {
                                pwms = <&L45 3 100000 0>;
                                max-brightness = <255>;
                                linux,default-trigger = "panic";
                        };
};
};
};


kernel config:
CONFIG_SYSVIPC=y
CONFIG_POSIX_MQUEUE=y
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG_PROC=y
CONFIG_CGROUPS=y
CONFIG_CGROUP_SCHED=y
CONFIG_CFS_BANDWIDTH=y
CONFIG_CGROUP_BPF=y
CONFIG_NAMESPACES=y
CONFIG_USER_NS=y
CONFIG_CHECKPOINT_RESTORE=y
CONFIG_BLK_DEV_INITRD=y
CONFIG_EXPERT=y
CONFIG_BPF_SYSCALL=y
CONFIG_SMP=y
CONFIG_PCI=y
CONFIG_PCIE_XILINX=y
CONFIG_MODULES=y
CONFIG_MODULE_UNLOAD=y
CONFIG_NET=y
CONFIG_PACKET=y
CONFIG_UNIX=y
CONFIG_INET=y
CONFIG_IP_MULTICAST=y
CONFIG_IP_ADVANCED_ROUTER=y
CONFIG_IP_PNP=y
CONFIG_IP_PNP_DHCP=y
CONFIG_IP_PNP_BOOTP=y
CONFIG_IP_PNP_RARP=y
CONFIG_NETLINK_DIAG=y
CONFIG_DEVTMPFS=y
CONFIG_BLK_DEV_LOOP=y
CONFIG_VIRTIO_BLK=y
CONFIG_BLK_DEV_SD=y
CONFIG_BLK_DEV_SR=y
CONFIG_ATA=y
CONFIG_SATA_AHCI=y
CONFIG_SATA_AHCI_PLATFORM=y
CONFIG_NETDEVICES=y
CONFIG_VIRTIO_NET=y
CONFIG_MACB=y
CONFIG_E1000E=y
CONFIG_R8169=y
CONFIG_MICROSEMI_PHY=y
CONFIG_INPUT_MOUSEDEV=y
CONFIG_SERIAL_8250=y
CONFIG_SERIAL_8250_CONSOLE=y
CONFIG_SERIAL_OF_PLATFORM=y
CONFIG_SERIAL_EARLYCON_RISCV_SBI=y
CONFIG_SERIAL_SIFIVE=y
CONFIG_SERIAL_SIFIVE_CONSOLE=y
CONFIG_HVC_RISCV_SBI=y
# CONFIG_PTP_1588_CLOCK is not set
CONFIG_DRM=y
CONFIG_DRM_RADEON=y
CONFIG_FRAMEBUFFER_CONSOLE=y
CONFIG_USB=y
CONFIG_USB_XHCI_HCD=y
CONFIG_USB_XHCI_PLATFORM=y
CONFIG_USB_EHCI_HCD=y
CONFIG_USB_EHCI_HCD_PLATFORM=y
CONFIG_USB_OHCI_HCD=y
CONFIG_USB_OHCI_HCD_PLATFORM=y
CONFIG_USB_STORAGE=y
CONFIG_USB_UAS=y
CONFIG_VIRTIO_MMIO=y
CONFIG_SIFIVE_PLIC=y
CONFIG_RAS=y
CONFIG_EXT4_FS=y
CONFIG_EXT4_FS_POSIX_ACL=y
CONFIG_AUTOFS4_FS=y
CONFIG_MSDOS_FS=y
CONFIG_VFAT_FS=y
CONFIG_TMPFS=y
CONFIG_TMPFS_POSIX_ACL=y
CONFIG_NFS_FS=y
CONFIG_NFS_V4=y
CONFIG_NFS_V4_1=y
CONFIG_NFS_V4_2=y
CONFIG_ROOT_NFS=y
CONFIG_CRYPTO_USER_API_HASH=y
CONFIG_PRINTK_TIME=y
# CONFIG_RCU_TRACE is not set
CONFIG_CMDLINE_BOOL=y
CONFIG_CMDLINE="console=ttySIF0,115200 ignore_loglevel debug"
CONFIG_CLK_SIFIVE=y
CONFIG_CLK_SIFIVE_FU540_PRCI=y

- Yash
> >
> > 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 v9 0/2] PWM support for HiFive Unleashed

[Yash Shah]

Hi Andreas,

On Tue, Mar 19, 2019 at 11:56 AM Yash Shah <yash.shah@sifive.com> wrote:
>
> On Mon, Mar 18, 2019 at 10:56 PM Andreas Schwab <schwab@suse.de> wrote:
> >
> > On Mär 15 2019, Yash Shah <yash.shah@sifive.com> wrote:
> >
> > > You need to make sure the period setting is passed via the
> > > conventional way in DT file.
> > > Example:
> > > pwmleds {
> > >     compatible = "pwm-leds";
> > >     heartbeat {
> > >         pwms = <&L45 0 10000000 0>;
> > >         max-brightness = <255>;
> > >         linux,default-trigger = "heartbeat";
> > >     };
> > > };
> >
> > I've now managed to build a working FSBL with that change, but that
> > didn't change anything.  There is not even a heartbeat option in
> > /sys/class/leds/heartbeat/trigger any more.
>
...
>
> The above works for me.
> I just noticed that I have been using pwm-cells = 2, instead of 3.
> Maybe that is the problem here.
> I will suggest you test it on v11 patch in which I will fix this
> pwm-cells issue.

I have sent out the v11 patchset, you can test the heartbeat
application with that patchset.
You still need to make that DT file modification which you previously
did, using fsbl.bin
Just for your reference, I am copying my DT file and kernel config
which I used for my test.
The same is available at dev/yashs/pwm_5.0-rc1 branch of
https://github.com/yashshah7/riscv-linux.git

/dts-v1/;

/*#include <linux/clk/sifive-fu540-prci.h>*/
#define PRCI_CLK_TLCLK 3

/ {
#address-cells = <2>;
#size-cells = <2>;
compatible = "sifive,fu540-c000";

aliases {
serial0 = &uart0;
serial1 = &uart1;
};

chosen {
};

cpus {
#address-cells = <1>;
#size-cells = <0>;
timebase-frequency = <1000000>;
cpu0: cpu@0 {
clock-frequency = <0>;
compatible = "sifive,u51", "sifive,rocket0", "riscv";
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <128>;
i-cache-size = <16384>;
reg = <0>;
riscv,isa = "rv64imac";
status = "okay";
cpu0_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu1: cpu@1 {
clock-frequency = <0>;
compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
d-cache-block-size = <64>;
d-cache-sets = <64>;
d-cache-size = <32768>;
d-tlb-sets = <1>;
d-tlb-size = <32>;
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <64>;
i-cache-size = <32768>;
i-tlb-sets = <1>;
i-tlb-size = <32>;
mmu-type = "riscv,sv39";
reg = <1>;
riscv,isa = "rv64imafdc";
status = "okay";
tlb-split;
cpu1_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu2: cpu@2 {
clock-frequency = <0>;
compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
d-cache-block-size = <64>;
d-cache-sets = <64>;
d-cache-size = <32768>;
d-tlb-sets = <1>;
d-tlb-size = <32>;
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <64>;
i-cache-size = <32768>;
i-tlb-sets = <1>;
i-tlb-size = <32>;
mmu-type = "riscv,sv39";
reg = <2>;
riscv,isa = "rv64imafdc";
status = "okay";
tlb-split;
cpu2_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu3: cpu@3 {
clock-frequency = <0>;
compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
d-cache-block-size = <64>;
d-cache-sets = <64>;
d-cache-size = <32768>;
d-tlb-sets = <1>;
d-tlb-size = <32>;
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <64>;
i-cache-size = <32768>;
i-tlb-sets = <1>;
i-tlb-size = <32>;
mmu-type = "riscv,sv39";
reg = <3>;
riscv,isa = "rv64imafdc";
status = "okay";
tlb-split;
cpu3_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu4: cpu@4 {
clock-frequency = <0>;
compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
d-cache-block-size = <64>;
d-cache-sets = <64>;
d-cache-size = <32768>;
d-tlb-sets = <1>;
d-tlb-size = <32>;
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <64>;
i-cache-size = <32768>;
i-tlb-sets = <1>;
i-tlb-size = <32>;
mmu-type = "riscv,sv39";
reg = <4>;
riscv,isa = "rv64imafdc";
status = "okay";
tlb-split;
cpu4_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
};
soc {
#address-cells = <2>;
#size-cells = <2>;
compatible = "sifive,fu540-soc", "simple-bus";
ranges;
prci: prci@10000000 {
compatible = "sifive,fu540-c000-prci";
reg = <0x0 0x10000000 0x0 0x1000>;
clocks = <&hfclk>, <&rtcclk>;
#clock-cells = <1>;
};
uart0: serial@10010000 {
compatible = "sifive,fu540-c000-uart", "sifive,uart0";
interrupt-parent = <&plic0>;
interrupts = <4>;
reg = <0x0 0x10010000 0x0 0x1000>;
clocks = <&prci PRCI_CLK_TLCLK>;
};
uart1: serial@10011000 {
compatible = "sifive,fu540-c000-uart", "sifive,uart0";
interrupt-parent = <&plic0>;
interrupts = <5>;
reg = <0x0 0x10011000 0x0 0x1000>;
clocks = <&prci PRCI_CLK_TLCLK>;
};
L5: clint@2000000 {
compatible = "riscv,clint0";
interrupts-extended = <
&cpu0_intc 3 &cpu0_intc 7
&cpu1_intc 3 &cpu1_intc 7
&cpu2_intc 3 &cpu2_intc 7
&cpu3_intc 3 &cpu3_intc 7
&cpu4_intc 3 &cpu4_intc 7>;
reg = <0x0 0x2000000 0x0 0x10000>;
                };
plic0: interrupt-controller@c000000 {
#interrupt-cells = <1>;
compatible = "riscv,plic0";
interrupt-controller;
interrupts-extended = <
&cpu0_intc 11
&cpu1_intc 11 &cpu1_intc 9
&cpu2_intc 11 &cpu2_intc 9
&cpu3_intc 11 &cpu3_intc 9
&cpu4_intc 11 &cpu4_intc 9>;
reg = <0x0 0xc000000 0x0 0x4000000>;
                        riscv,ndev = <53>;
};
L45: pwm@10020000 {
                        compatible = "sifive,pwm0";
                        interrupt-parent = <&plic0>;
                        interrupts = <42 43 44 45>;
                        reg = <0x0 0x10020000 0x0 0x1000>;
                        reg-names = "control";
                        clocks = <&prci 3>;
                        #pwm-cells = <3>;
                };
                L46: pwm@10021000 {
                        compatible = "sifive,pwm0";
                        interrupt-parent = <&plic0>;
                        interrupts = <46 47 48 49>;
                        reg = <0x0 0x10021000 0x0 0x1000>;
                        reg-names = "control";
                        clocks = <&prci 3>;
                        #pwm-cells = <3>;
                };
pwmleds {
                        compatible = "pwm-leds";
                        heartbeat {
                                pwms = <&L45 0 100000 0>;
                                max-brightness = <255>;
                                linux,default-trigger = "heartbeat";
                        };
                        mtd {
                                pwms = <&L45 1 100000 0>;
                                max-brightness = <255>;
                                linux,default-trigger = "mtd";
                        };
                        netdev {
                                pwms = <&L45 2 100000 0>;
                                max-brightness = <255>;
                                linux,default-trigger = "netdev";
                        };
                        panic {
                                pwms = <&L45 3 100000 0>;
                                max-brightness = <255>;
                                linux,default-trigger = "panic";
                        };
};
};
};


kernel config:
CONFIG_SYSVIPC=y
CONFIG_POSIX_MQUEUE=y
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG_PROC=y
CONFIG_CGROUPS=y
CONFIG_CGROUP_SCHED=y
CONFIG_CFS_BANDWIDTH=y
CONFIG_CGROUP_BPF=y
CONFIG_NAMESPACES=y
CONFIG_USER_NS=y
CONFIG_CHECKPOINT_RESTORE=y
CONFIG_BLK_DEV_INITRD=y
CONFIG_EXPERT=y
CONFIG_BPF_SYSCALL=y
CONFIG_SMP=y
CONFIG_PCI=y
CONFIG_PCIE_XILINX=y
CONFIG_MODULES=y
CONFIG_MODULE_UNLOAD=y
CONFIG_NET=y
CONFIG_PACKET=y
CONFIG_UNIX=y
CONFIG_INET=y
CONFIG_IP_MULTICAST=y
CONFIG_IP_ADVANCED_ROUTER=y
CONFIG_IP_PNP=y
CONFIG_IP_PNP_DHCP=y
CONFIG_IP_PNP_BOOTP=y
CONFIG_IP_PNP_RARP=y
CONFIG_NETLINK_DIAG=y
CONFIG_DEVTMPFS=y
CONFIG_BLK_DEV_LOOP=y
CONFIG_VIRTIO_BLK=y
CONFIG_BLK_DEV_SD=y
CONFIG_BLK_DEV_SR=y
CONFIG_ATA=y
CONFIG_SATA_AHCI=y
CONFIG_SATA_AHCI_PLATFORM=y
CONFIG_NETDEVICES=y
CONFIG_VIRTIO_NET=y
CONFIG_MACB=y
CONFIG_E1000E=y
CONFIG_R8169=y
CONFIG_MICROSEMI_PHY=y
CONFIG_INPUT_MOUSEDEV=y
CONFIG_SERIAL_8250=y
CONFIG_SERIAL_8250_CONSOLE=y
CONFIG_SERIAL_OF_PLATFORM=y
CONFIG_SERIAL_EARLYCON_RISCV_SBI=y
CONFIG_SERIAL_SIFIVE=y
CONFIG_SERIAL_SIFIVE_CONSOLE=y
CONFIG_HVC_RISCV_SBI=y
# CONFIG_PTP_1588_CLOCK is not set
CONFIG_DRM=y
CONFIG_DRM_RADEON=y
CONFIG_FRAMEBUFFER_CONSOLE=y
CONFIG_USB=y
CONFIG_USB_XHCI_HCD=y
CONFIG_USB_XHCI_PLATFORM=y
CONFIG_USB_EHCI_HCD=y
CONFIG_USB_EHCI_HCD_PLATFORM=y
CONFIG_USB_OHCI_HCD=y
CONFIG_USB_OHCI_HCD_PLATFORM=y
CONFIG_USB_STORAGE=y
CONFIG_USB_UAS=y
CONFIG_VIRTIO_MMIO=y
CONFIG_SIFIVE_PLIC=y
CONFIG_RAS=y
CONFIG_EXT4_FS=y
CONFIG_EXT4_FS_POSIX_ACL=y
CONFIG_AUTOFS4_FS=y
CONFIG_MSDOS_FS=y
CONFIG_VFAT_FS=y
CONFIG_TMPFS=y
CONFIG_TMPFS_POSIX_ACL=y
CONFIG_NFS_FS=y
CONFIG_NFS_V4=y
CONFIG_NFS_V4_1=y
CONFIG_NFS_V4_2=y
CONFIG_ROOT_NFS=y
CONFIG_CRYPTO_USER_API_HASH=y
CONFIG_PRINTK_TIME=y
# CONFIG_RCU_TRACE is not set
CONFIG_CMDLINE_BOOL=y
CONFIG_CMDLINE="console=ttySIF0,115200 ignore_loglevel debug"
CONFIG_CLK_SIFIVE=y
CONFIG_CLK_SIFIVE_FU540_PRCI=y

- Yash
> >
> > 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 v9 0/2] PWM support for HiFive Unleashed

[Andreas Schwab]

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

> I have sent out the v11 patchset, you can test the heartbeat
> application with that patchset.
> You still need to make that DT file modification which you previously
> did, using fsbl.bin

Why can't the driver make use of sifive,approx-period?

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 v9 0/2] PWM support for HiFive Unleashed

[Andreas Schwab]

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

> I have sent out the v11 patchset, you can test the heartbeat
> application with that patchset.
> You still need to make that DT file modification which you previously
> did, using fsbl.bin

Why can't the driver make use of sifive,approx-period?

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 v9 0/2] PWM support for HiFive Unleashed

[Yash Shah]

On Mon, Mar 25, 2019 at 5:28 PM Andreas Schwab <schwab@suse.de> wrote:
>
> On Mär 25 2019, Yash Shah <yash.shah@sifive.com> wrote:
>
> > I have sent out the v11 patchset, you can test the heartbeat
> > application with that patchset.
> > You still need to make that DT file modification which you previously
> > did, using fsbl.bin
>
> Why can't the driver make use of sifive,approx-period?

Because as per the review comments and discussions, it has been
decided that the driver will make use of the conventional interface to
pass period settings instead of 'sifive,approx-period'.

For your reference:
https://lkml.org/lkml/2019/2/6/159

>
> 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 v9 0/2] PWM support for HiFive Unleashed

[Yash Shah]

On Mon, Mar 25, 2019 at 5:28 PM Andreas Schwab <schwab@suse.de> wrote:
>
> On Mär 25 2019, Yash Shah <yash.shah@sifive.com> wrote:
>
> > I have sent out the v11 patchset, you can test the heartbeat
> > application with that patchset.
> > You still need to make that DT file modification which you previously
> > did, using fsbl.bin
>
> Why can't the driver make use of sifive,approx-period?

Because as per the review comments and discussions, it has been
decided that the driver will make use of the conventional interface to
pass period settings instead of 'sifive,approx-period'.

For your reference:
https://lkml.org/lkml/2019/2/6/159

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