* [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-29 11:43 ` Yash Shah
0 siblings, 0 replies; 53+ messages in thread
From: Yash Shah @ 2019-01-29 11:43 UTC (permalink / raw)
To: palmer, linux-pwm, linux-riscv
Cc: mark.rutland, devicetree, linux-kernel, robh+dt, sachin.ghadi,
Yash Shah, thierry.reding, paul.walmsley
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 | 261 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 273 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..2b516f7
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,261 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017-2018 SiFive
+ * For SiFive's PWM IP block documentation please refer Chapter 14 of
+ * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf
+ */
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+/* Register offsets */
+#define PWM_SIFIVE_PWMCFG 0x0
+#define PWM_SIFIVE_PWMCOUNT 0x8
+#define PWM_SIFIVE_PWMS 0x10
+#define PWM_SIFIVE_PWMCMP0 0x20
+
+/* PWMCFG fields */
+#define PWM_SIFIVE_PWMCFG_SCALE 0
+#define PWM_SIFIVE_PWMCFG_STICKY 8
+#define PWM_SIFIVE_PWMCFG_ZERO_CMP 9
+#define PWM_SIFIVE_PWMCFG_DEGLITCH 10
+#define PWM_SIFIVE_PWMCFG_EN_ALWAYS 12
+#define PWM_SIFIVE_PWMCFG_EN_ONCE 13
+#define PWM_SIFIVE_PWMCFG_CENTER 16
+#define PWM_SIFIVE_PWMCFG_GANG 24
+#define PWM_SIFIVE_PWMCFG_IP 28
+
+/* SIZE_PWMCMP is used to calculate the offset for pwmcmpX registers */
+#define SIZE_PWMCMP 4
+#define CMPWIDTH 16
+
+struct pwm_sifive_ddata {
+ struct pwm_chip chip;
+ struct notifier_block notifier;
+ struct clk *clk;
+ void __iomem *regs;
+ unsigned int approx_period;
+ unsigned int real_period;
+};
+
+static inline struct pwm_sifive_ddata *to_pwm_sifive_chip(struct pwm_chip *c)
+{
+ return container_of(c, struct pwm_sifive_ddata, chip);
+}
+
+static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *dev,
+ struct pwm_state *state)
+{
+ struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
+ u32 duty;
+
+ duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
+
+ state->period = pwm->real_period;
+ state->duty_cycle = ((u64)duty * pwm->real_period) >> CMPWIDTH;
+ state->polarity = PWM_POLARITY_INVERSED;
+ state->enabled = duty > 0;
+}
+
+static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
+ struct pwm_state *state)
+{
+ struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
+ unsigned int duty_cycle;
+ u32 frac, val;
+
+ if (state->polarity != PWM_POLARITY_INVERSED)
+ return -EINVAL;
+
+ if (state->period != pwm->real_period)
+ return -EINVAL;
+
+ duty_cycle = state->duty_cycle;
+ if (!state->enabled)
+ duty_cycle = 0;
+
+ frac = div_u64((u64)duty_cycle * 0xFFFF, state->period);
+ frac = min(frac, 0xFFFFU);
+
+ val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
+ val |= (1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
+ writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
+
+ writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
+
+ val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
+ writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
+
+ pwm_sifive_get_state(chip, dev, state);
+
+ return 0;
+}
+
+static const struct pwm_ops pwm_sifive_ops = {
+ .get_state = pwm_sifive_get_state,
+ .apply = pwm_sifive_apply,
+ .owner = THIS_MODULE,
+};
+
+static struct pwm_device *pwm_sifive_xlate(struct pwm_chip *chip,
+ const struct of_phandle_args *args)
+{
+ struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
+ struct pwm_device *dev;
+
+ if (args->args[0] >= chip->npwm)
+ return ERR_PTR(-EINVAL);
+
+ dev = pwm_request_from_chip(chip, args->args[0], NULL);
+ if (IS_ERR(dev))
+ return dev;
+
+ /* The period cannot be changed on a per-PWM basis */
+ dev->args.period = pwm->real_period;
+ dev->args.polarity = PWM_POLARITY_NORMAL;
+ if (args->args[1] & PWM_POLARITY_INVERSED)
+ dev->args.polarity = PWM_POLARITY_INVERSED;
+
+ return dev;
+}
+
+static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
+ unsigned long rate)
+{
+ /* (1 << (16+scale)) * 10^9/rate = real_period */
+ unsigned long scale_pow =
+ (pwm->approx_period * (u64)rate) / NSEC_PER_SEC;
+ int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
+
+ writel((1 << PWM_SIFIVE_PWMCFG_EN_ALWAYS) | (scale <<
+ PWM_SIFIVE_PWMCFG_SCALE), pwm->regs + PWM_SIFIVE_PWMCFG);
+
+ /* As scale <= 15 the shift operation cannot overflow. */
+ pwm->real_period = div64_ul(1000000000ULL << (16 + scale), rate);
+ dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
+}
+
+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 device_node *node = pdev->dev.of_node;
+ struct pwm_sifive_ddata *pwm;
+ struct pwm_chip *chip;
+ struct resource *res;
+ int ret;
+
+ pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+ if (!pwm)
+ return -ENOMEM;
+
+ chip = &pwm->chip;
+ chip->dev = dev;
+ chip->ops = &pwm_sifive_ops;
+ chip->of_xlate = pwm_sifive_xlate;
+ chip->of_pwm_n_cells = 2;
+ chip->base = -1;
+ chip->npwm = 4;
+
+ ret = of_property_read_u32(node, "sifive,period-ns",
+ &pwm->approx_period);
+
+ if (ret < 0) {
+ dev_err(dev, "Unable to read sifive,period-ns from DTS\n");
+ return ret;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ pwm->regs = devm_ioremap_resource(dev, res);
+ if (IS_ERR(pwm->regs)) {
+ dev_err(dev, "Unable to map IO resources\n");
+ return PTR_ERR(pwm->regs);
+ }
+
+ pwm->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(pwm->clk)) {
+ if (PTR_ERR(pwm->clk) != -EPROBE_DEFER)
+ dev_err(dev, "Unable to find controller clock\n");
+ return PTR_ERR(pwm->clk);
+ }
+
+ ret = clk_prepare_enable(pwm->clk);
+ if (ret) {
+ dev_err(dev, "failed to enable clock for pwm: %d\n", ret);
+ return ret;
+ }
+
+ /* Watch for changes to underlying clock frequency */
+ pwm->notifier.notifier_call = 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 config */
+ 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;
+
+ ret = pwmchip_remove(&pwm->chip);
+ clk_notifier_unregister(pwm->clk, &pwm->notifier);
+ clk_disable_unprepare(pwm->clk);
+ return ret;
+}
+
+static const struct of_device_id 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
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-29 11:43 ` Yash Shah
0 siblings, 0 replies; 53+ messages in thread
From: Yash Shah @ 2019-01-29 11:43 UTC (permalink / raw)
To: palmer, linux-pwm, linux-riscv
Cc: mark.rutland, devicetree, linux-kernel, robh+dt, sachin.ghadi,
Yash Shah, thierry.reding, paul.walmsley
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 | 261 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 273 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..2b516f7
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,261 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017-2018 SiFive
+ * For SiFive's PWM IP block documentation please refer Chapter 14 of
+ * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf
+ */
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+/* Register offsets */
+#define PWM_SIFIVE_PWMCFG 0x0
+#define PWM_SIFIVE_PWMCOUNT 0x8
+#define PWM_SIFIVE_PWMS 0x10
+#define PWM_SIFIVE_PWMCMP0 0x20
+
+/* PWMCFG fields */
+#define PWM_SIFIVE_PWMCFG_SCALE 0
+#define PWM_SIFIVE_PWMCFG_STICKY 8
+#define PWM_SIFIVE_PWMCFG_ZERO_CMP 9
+#define PWM_SIFIVE_PWMCFG_DEGLITCH 10
+#define PWM_SIFIVE_PWMCFG_EN_ALWAYS 12
+#define PWM_SIFIVE_PWMCFG_EN_ONCE 13
+#define PWM_SIFIVE_PWMCFG_CENTER 16
+#define PWM_SIFIVE_PWMCFG_GANG 24
+#define PWM_SIFIVE_PWMCFG_IP 28
+
+/* SIZE_PWMCMP is used to calculate the offset for pwmcmpX registers */
+#define SIZE_PWMCMP 4
+#define CMPWIDTH 16
+
+struct pwm_sifive_ddata {
+ struct pwm_chip chip;
+ struct notifier_block notifier;
+ struct clk *clk;
+ void __iomem *regs;
+ unsigned int approx_period;
+ unsigned int real_period;
+};
+
+static inline struct pwm_sifive_ddata *to_pwm_sifive_chip(struct pwm_chip *c)
+{
+ return container_of(c, struct pwm_sifive_ddata, chip);
+}
+
+static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *dev,
+ struct pwm_state *state)
+{
+ struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
+ u32 duty;
+
+ duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
+
+ state->period = pwm->real_period;
+ state->duty_cycle = ((u64)duty * pwm->real_period) >> CMPWIDTH;
+ state->polarity = PWM_POLARITY_INVERSED;
+ state->enabled = duty > 0;
+}
+
+static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
+ struct pwm_state *state)
+{
+ struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
+ unsigned int duty_cycle;
+ u32 frac, val;
+
+ if (state->polarity != PWM_POLARITY_INVERSED)
+ return -EINVAL;
+
+ if (state->period != pwm->real_period)
+ return -EINVAL;
+
+ duty_cycle = state->duty_cycle;
+ if (!state->enabled)
+ duty_cycle = 0;
+
+ frac = div_u64((u64)duty_cycle * 0xFFFF, state->period);
+ frac = min(frac, 0xFFFFU);
+
+ val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
+ val |= (1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
+ writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
+
+ writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
+
+ val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
+ writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
+
+ pwm_sifive_get_state(chip, dev, state);
+
+ return 0;
+}
+
+static const struct pwm_ops pwm_sifive_ops = {
+ .get_state = pwm_sifive_get_state,
+ .apply = pwm_sifive_apply,
+ .owner = THIS_MODULE,
+};
+
+static struct pwm_device *pwm_sifive_xlate(struct pwm_chip *chip,
+ const struct of_phandle_args *args)
+{
+ struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
+ struct pwm_device *dev;
+
+ if (args->args[0] >= chip->npwm)
+ return ERR_PTR(-EINVAL);
+
+ dev = pwm_request_from_chip(chip, args->args[0], NULL);
+ if (IS_ERR(dev))
+ return dev;
+
+ /* The period cannot be changed on a per-PWM basis */
+ dev->args.period = pwm->real_period;
+ dev->args.polarity = PWM_POLARITY_NORMAL;
+ if (args->args[1] & PWM_POLARITY_INVERSED)
+ dev->args.polarity = PWM_POLARITY_INVERSED;
+
+ return dev;
+}
+
+static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
+ unsigned long rate)
+{
+ /* (1 << (16+scale)) * 10^9/rate = real_period */
+ unsigned long scale_pow =
+ (pwm->approx_period * (u64)rate) / NSEC_PER_SEC;
+ int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
+
+ writel((1 << PWM_SIFIVE_PWMCFG_EN_ALWAYS) | (scale <<
+ PWM_SIFIVE_PWMCFG_SCALE), pwm->regs + PWM_SIFIVE_PWMCFG);
+
+ /* As scale <= 15 the shift operation cannot overflow. */
+ pwm->real_period = div64_ul(1000000000ULL << (16 + scale), rate);
+ dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
+}
+
+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 device_node *node = pdev->dev.of_node;
+ struct pwm_sifive_ddata *pwm;
+ struct pwm_chip *chip;
+ struct resource *res;
+ int ret;
+
+ pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+ if (!pwm)
+ return -ENOMEM;
+
+ chip = &pwm->chip;
+ chip->dev = dev;
+ chip->ops = &pwm_sifive_ops;
+ chip->of_xlate = pwm_sifive_xlate;
+ chip->of_pwm_n_cells = 2;
+ chip->base = -1;
+ chip->npwm = 4;
+
+ ret = of_property_read_u32(node, "sifive,period-ns",
+ &pwm->approx_period);
+
+ if (ret < 0) {
+ dev_err(dev, "Unable to read sifive,period-ns from DTS\n");
+ return ret;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ pwm->regs = devm_ioremap_resource(dev, res);
+ if (IS_ERR(pwm->regs)) {
+ dev_err(dev, "Unable to map IO resources\n");
+ return PTR_ERR(pwm->regs);
+ }
+
+ pwm->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(pwm->clk)) {
+ if (PTR_ERR(pwm->clk) != -EPROBE_DEFER)
+ dev_err(dev, "Unable to find controller clock\n");
+ return PTR_ERR(pwm->clk);
+ }
+
+ ret = clk_prepare_enable(pwm->clk);
+ if (ret) {
+ dev_err(dev, "failed to enable clock for pwm: %d\n", ret);
+ return ret;
+ }
+
+ /* Watch for changes to underlying clock frequency */
+ pwm->notifier.notifier_call = 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 config */
+ 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;
+
+ ret = pwmchip_remove(&pwm->chip);
+ clk_notifier_unregister(pwm->clk, &pwm->notifier);
+ clk_disable_unprepare(pwm->clk);
+ return ret;
+}
+
+static const struct of_device_id 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
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-29 11:43 ` Yash Shah
(?)
@ 2019-02-05 8:21 ` kbuild test robot
-1 siblings, 0 replies; 53+ messages in thread
From: kbuild test robot @ 2019-02-05 8:21 UTC (permalink / raw)
To: Yash Shah
Cc: kbuild-all, palmer, linux-pwm, linux-riscv, thierry.reding,
robh+dt, mark.rutland, devicetree, linux-kernel, sachin.ghadi,
paul.walmsley, Yash Shah
[-- Attachment #1: Type: text/plain, Size: 920 bytes --]
Hi Yash,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pwm/for-next]
[also build test ERROR on v5.0-rc4 next-20190204]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Yash-Shah/pwm-sifive-Add-DT-documentation-for-SiFive-PWM-Controller/20190201-103646
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-8 (Debian 8.2.0-14) 8.2.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
>> ERROR: "__udivdi3" [drivers/pwm/pwm-sifive.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66632 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-05 8:21 ` kbuild test robot
0 siblings, 0 replies; 53+ messages in thread
From: kbuild test robot @ 2019-02-05 8:21 UTC (permalink / raw)
To: Yash Shah
Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel,
robh+dt, sachin.ghadi, Yash Shah, thierry.reding, kbuild-all,
paul.walmsley, linux-riscv
[-- Attachment #1: Type: text/plain, Size: 920 bytes --]
Hi Yash,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pwm/for-next]
[also build test ERROR on v5.0-rc4 next-20190204]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Yash-Shah/pwm-sifive-Add-DT-documentation-for-SiFive-PWM-Controller/20190201-103646
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-8 (Debian 8.2.0-14) 8.2.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
>> ERROR: "__udivdi3" [drivers/pwm/pwm-sifive.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66632 bytes --]
[-- Attachment #3: Type: text/plain, Size: 161 bytes --]
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-05 8:21 ` kbuild test robot
0 siblings, 0 replies; 53+ messages in thread
From: kbuild test robot @ 2019-02-05 8:21 UTC (permalink / raw)
Cc: kbuild-all, palmer, linux-pwm, linux-riscv, thierry.reding,
robh+dt, mark.rutland, devicetree, linux-kernel, sachin.ghadi,
paul.walmsley, Yash Shah
[-- Attachment #1: Type: text/plain, Size: 920 bytes --]
Hi Yash,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pwm/for-next]
[also build test ERROR on v5.0-rc4 next-20190204]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Yash-Shah/pwm-sifive-Add-DT-documentation-for-SiFive-PWM-Controller/20190201-103646
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-8 (Debian 8.2.0-14) 8.2.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
>> ERROR: "__udivdi3" [drivers/pwm/pwm-sifive.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66632 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-29 11:43 ` Yash Shah
(?)
@ 2019-02-05 17:25 ` kbuild test robot
-1 siblings, 0 replies; 53+ messages in thread
From: kbuild test robot @ 2019-02-05 17:25 UTC (permalink / raw)
To: Yash Shah
Cc: kbuild-all, palmer, linux-pwm, linux-riscv, thierry.reding,
robh+dt, mark.rutland, devicetree, linux-kernel, sachin.ghadi,
paul.walmsley, Yash Shah
[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]
Hi Yash,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pwm/for-next]
[also build test ERROR on v5.0-rc4 next-20190205]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Yash-Shah/pwm-sifive-Add-DT-documentation-for-SiFive-PWM-Controller/20190201-103646
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=arm
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: drivers/pwm/pwm-sifive.o: in function `pwm_sifive_update_clock':
>> pwm-sifive.c:(.text+0x390): undefined reference to `__aeabi_uldivmod'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67860 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-05 17:25 ` kbuild test robot
0 siblings, 0 replies; 53+ messages in thread
From: kbuild test robot @ 2019-02-05 17:25 UTC (permalink / raw)
To: Yash Shah
Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel,
robh+dt, sachin.ghadi, Yash Shah, thierry.reding, kbuild-all,
paul.walmsley, linux-riscv
[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]
Hi Yash,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pwm/for-next]
[also build test ERROR on v5.0-rc4 next-20190205]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Yash-Shah/pwm-sifive-Add-DT-documentation-for-SiFive-PWM-Controller/20190201-103646
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=arm
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: drivers/pwm/pwm-sifive.o: in function `pwm_sifive_update_clock':
>> pwm-sifive.c:(.text+0x390): undefined reference to `__aeabi_uldivmod'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67860 bytes --]
[-- Attachment #3: Type: text/plain, Size: 161 bytes --]
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-05 17:25 ` kbuild test robot
0 siblings, 0 replies; 53+ messages in thread
From: kbuild test robot @ 2019-02-05 17:25 UTC (permalink / raw)
Cc: kbuild-all, palmer, linux-pwm, linux-riscv, thierry.reding,
robh+dt, mark.rutland, devicetree, linux-kernel, sachin.ghadi,
paul.walmsley, Yash Shah
[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]
Hi Yash,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pwm/for-next]
[also build test ERROR on v5.0-rc4 next-20190205]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Yash-Shah/pwm-sifive-Add-DT-documentation-for-SiFive-PWM-Controller/20190201-103646
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=arm
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: drivers/pwm/pwm-sifive.o: in function `pwm_sifive_update_clock':
>> pwm-sifive.c:(.text+0x390): undefined reference to `__aeabi_uldivmod'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67860 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-29 11:43 ` Yash Shah
@ 2019-02-06 12:44 ` Thierry Reding
-1 siblings, 0 replies; 53+ messages in thread
From: Thierry Reding @ 2019-02-06 12:44 UTC (permalink / raw)
To: Yash Shah
Cc: palmer, linux-pwm, linux-riscv, robh+dt, mark.rutland,
devicetree, linux-kernel, sachin.ghadi, paul.walmsley
[-- Attachment #1: Type: text/plain, Size: 506 bytes --]
On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
[...]
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
[...]
> +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> + unsigned long rate)
> +{
> + /* (1 << (16+scale)) * 10^9/rate = real_period */
> + unsigned long scale_pow =
> + (pwm->approx_period * (u64)rate) / NSEC_PER_SEC;
I think you need another div64_ul() for this one to fix the linker error
that the 0-day builder was pointing out.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-06 12:44 ` Thierry Reding
0 siblings, 0 replies; 53+ messages in thread
From: Thierry Reding @ 2019-02-06 12:44 UTC (permalink / raw)
To: Yash Shah
Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel,
sachin.ghadi, robh+dt, paul.walmsley, linux-riscv
[-- Attachment #1.1: Type: text/plain, Size: 506 bytes --]
On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
[...]
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
[...]
> +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> + unsigned long rate)
> +{
> + /* (1 << (16+scale)) * 10^9/rate = real_period */
> + unsigned long scale_pow =
> + (pwm->approx_period * (u64)rate) / NSEC_PER_SEC;
I think you need another div64_ul() for this one to fix the linker error
that the 0-day builder was pointing out.
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
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-02-06 12:44 ` Thierry Reding
(?)
@ 2019-02-07 8:24 ` Yash Shah
-1 siblings, 0 replies; 53+ messages in thread
From: Yash Shah @ 2019-02-07 8:24 UTC (permalink / raw)
To: Thierry Reding
Cc: Palmer Dabbelt, linux-pwm, linux-riscv, robh+dt, mark.rutland,
devicetree, linux-kernel, Sachin Ghadi, Paul Walmsley
On Wed, Feb 6, 2019 at 6:14 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> [...]
> > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> [...]
> > +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> > + unsigned long rate)
> > +{
> > + /* (1 << (16+scale)) * 10^9/rate = real_period */
> > + unsigned long scale_pow =
> > + (pwm->approx_period * (u64)rate) / NSEC_PER_SEC;
>
> I think you need another div64_ul() for this one to fix the linker error
> that the 0-day builder was pointing out.
Yes, will fix this. Thanks for pointing it out.
>
> Thierry
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-07 8:24 ` Yash Shah
0 siblings, 0 replies; 53+ messages in thread
From: Yash Shah @ 2019-02-07 8:24 UTC (permalink / raw)
To: Thierry Reding
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, Sachin Ghadi, robh+dt, Paul Walmsley, linux-riscv
On Wed, Feb 6, 2019 at 6:14 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> [...]
> > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> [...]
> > +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> > + unsigned long rate)
> > +{
> > + /* (1 << (16+scale)) * 10^9/rate = real_period */
> > + unsigned long scale_pow =
> > + (pwm->approx_period * (u64)rate) / NSEC_PER_SEC;
>
> I think you need another div64_ul() for this one to fix the linker error
> that the 0-day builder was pointing out.
Yes, will fix this. Thanks for pointing it out.
>
> Thierry
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-07 8:24 ` Yash Shah
0 siblings, 0 replies; 53+ messages in thread
From: Yash Shah @ 2019-02-07 8:24 UTC (permalink / raw)
To: Thierry Reding
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, Sachin Ghadi, robh+dt, Paul Walmsley, linux-riscv
On Wed, Feb 6, 2019 at 6:14 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> [...]
> > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> [...]
> > +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> > + unsigned long rate)
> > +{
> > + /* (1 << (16+scale)) * 10^9/rate = real_period */
> > + unsigned long scale_pow =
> > + (pwm->approx_period * (u64)rate) / NSEC_PER_SEC;
>
> I think you need another div64_ul() for this one to fix the linker error
> that the 0-day builder was pointing out.
Yes, will fix this. Thanks for pointing it out.
>
> Thierry
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-29 11:43 ` Yash Shah
@ 2019-02-07 10:16 ` Uwe Kleine-König
-1 siblings, 0 replies; 53+ messages in thread
From: Uwe Kleine-König @ 2019-02-07 10:16 UTC (permalink / raw)
To: Yash Shah
Cc: palmer, linux-pwm, linux-riscv, thierry.reding, robh+dt,
mark.rutland, devicetree, linux-kernel, sachin.ghadi,
paul.walmsley
Hello,
On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
>
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sifive.c | 261 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 273 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..2b516f7
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,261 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017-2018 SiFive
> + * For SiFive's PWM IP block documentation please refer Chapter 14 of
> + * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> + */
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +/* Register offsets */
> +#define PWM_SIFIVE_PWMCFG 0x0
> +#define PWM_SIFIVE_PWMCOUNT 0x8
> +#define PWM_SIFIVE_PWMS 0x10
> +#define PWM_SIFIVE_PWMCMP0 0x20
> +
> +/* PWMCFG fields */
> +#define PWM_SIFIVE_PWMCFG_SCALE 0
> +#define PWM_SIFIVE_PWMCFG_STICKY 8
> +#define PWM_SIFIVE_PWMCFG_ZERO_CMP 9
> +#define PWM_SIFIVE_PWMCFG_DEGLITCH 10
> +#define PWM_SIFIVE_PWMCFG_EN_ALWAYS 12
> +#define PWM_SIFIVE_PWMCFG_EN_ONCE 13
> +#define PWM_SIFIVE_PWMCFG_CENTER 16
> +#define PWM_SIFIVE_PWMCFG_GANG 24
> +#define PWM_SIFIVE_PWMCFG_IP 28
> +
> +/* SIZE_PWMCMP is used to calculate the offset for pwmcmpX registers */
> +#define SIZE_PWMCMP 4
> +#define CMPWIDTH 16
Please add a PWM_SIFIVE_ prefix to these two.
> +struct pwm_sifive_ddata {
> + struct pwm_chip chip;
> + struct notifier_block notifier;
> + struct clk *clk;
> + void __iomem *regs;
> + unsigned int approx_period;
> + unsigned int real_period;
> +};
> +
> +static inline struct pwm_sifive_ddata *to_pwm_sifive_chip(struct pwm_chip *c)
even though this is inlined I would like this to have use the
pwm_sifive_ prefix, too.
> +{
> + return container_of(c, struct pwm_sifive_ddata, chip);
> +}
> +
> +static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> + struct pwm_state *state)
> +{
> + struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> + u32 duty;
> +
> + duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> +
> + state->period = pwm->real_period;
> + state->duty_cycle = ((u64)duty * pwm->real_period) >> CMPWIDTH;
> + state->polarity = PWM_POLARITY_INVERSED;
> + state->enabled = duty > 0;
> +}
> +
> +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> + struct pwm_state *state)
> +{
> + struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> + unsigned int duty_cycle;
> + u32 frac, val;
> +
> + if (state->polarity != PWM_POLARITY_INVERSED)
> + return -EINVAL;
> +
> + if (state->period != pwm->real_period)
> + return -EINVAL;
> +
> + duty_cycle = state->duty_cycle;
> + if (!state->enabled)
> + duty_cycle = 0;
> +
> + frac = div_u64((u64)duty_cycle * 0xFFFF, state->period);
> + frac = min(frac, 0xFFFFU);
If CMPWIDTH was only 8, we'd need 0xff here instead of 0xffff, right? So
better use
(1 << PWM_SIFIVE_CMPWIDTH) - 1
here, maybe "hidden" behind a cpp define.
In a previous review round I had doubts about the calculation of frac
and I think they were addressed wrongly.
Looking at the figure at the start of chapter 14.9 in the reference
manual: They use (different than the driver here) pwmcmp0=6 and
pwmzerocmp=1. Then a period is 7 clock cycles long and pwmcmpX must be 7
(for X > 0) to yield a 100% signal. So pwmcmpX must be greater than or
equal to the period (in clock cycles).
Here we're using pwmzerocmp=0, still the constraint
pwmcmpX >= period
must be true for a 100% cycle. (Right?) With pwmzerocmp=0 the period is
0x10000 clock cycles, so pwmcmpX must be >= 0x10000 to yield a 100%
signal. As this is not possible (pwmcmpX is a 16 bit value that can only
hold up to 0xffff) the output cannot yield 100%.
So I think the most correct implementation here is:
frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 << PWM_SIFIVE_CMPWIDTH) / 2) / period;
/* Sorry, we cannot do 100% :-( */
frac = min(frac, (1 << PWM_SIFIVE_CMPWIDTH) - 1);
Thierry: What is the correct rounding strategy? I assumed "round to
nearest" (with round half up). If we want something like clk_round_rate
for PWMs in the future rounding down might be easier to handle.
Note that pwm_sifive_get_state is already (well, or still) using this as
it has:
state->duty_cycle = ((u64)duty * pwm->real_period) >> CMPWIDTH;
which always yields something smaller than read_period as duty is
smaller than 1 << PWM_SIFIVE_CMPWIDTH.
I tried to understand the IP source at
https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/pwm/PWM.scala
to confirm (or disprove) this theory, but I failed.
Assuming my analysis is true, I'd like to have it documented
prominently in the driver that the hardware cannot generate a 100% duty
cycle.
> + val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> + val |= (1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
No parenthesis necessary. You might want to consider using the BIT macro
here, maybe already in the definition of PWM_SIFIVE_PWMCFG_DEGLITCH.
> + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> +
> + val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
> + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> + pwm_sifive_get_state(chip, dev, state);
Thierry: This changes the pwm_state. Is this how this should be done?
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops pwm_sifive_ops = {
> + .get_state = pwm_sifive_get_state,
> + .apply = pwm_sifive_apply,
> + .owner = THIS_MODULE,
> +};
> +
> +static struct pwm_device *pwm_sifive_xlate(struct pwm_chip *chip,
> + const struct of_phandle_args *args)
> +{
> + struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> + struct pwm_device *dev;
> +
> + if (args->args[0] >= chip->npwm)
> + return ERR_PTR(-EINVAL);
> +
> + dev = pwm_request_from_chip(chip, args->args[0], NULL);
> + if (IS_ERR(dev))
> + return dev;
> +
> + /* The period cannot be changed on a per-PWM basis */
> + dev->args.period = pwm->real_period;
> + dev->args.polarity = PWM_POLARITY_NORMAL;
> + if (args->args[1] & PWM_POLARITY_INVERSED)
> + dev->args.polarity = PWM_POLARITY_INVERSED;
The driver does only support PWM_POLARITY_INVERSED. So I guess this
should be:
dev->args.polarity = PWM_POLARITY_INVERSED;
if (!(args->args[1] & PWM_POLARITY_INVERSED))
dev_warn(dev, "...");
> +
> + return dev;
> +}
> +
> +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> + unsigned long rate)
> +{
> + /* (1 << (16+scale)) * 10^9/rate = real_period */
> + unsigned long scale_pow =
> + (pwm->approx_period * (u64)rate) / NSEC_PER_SEC;
> + int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
is this 16 in reality PWM_SIFIVE_CMPWIDTH?
> + writel((1 << PWM_SIFIVE_PWMCFG_EN_ALWAYS) | (scale <<
> + PWM_SIFIVE_PWMCFG_SCALE), pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> + /* As scale <= 15 the shift operation cannot overflow. */
> + pwm->real_period = div64_ul(1000000000ULL << (16 + scale), rate);
> + dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
> +}
> +
> +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 device_node *node = pdev->dev.of_node;
> + struct pwm_sifive_ddata *pwm;
> + struct pwm_chip *chip;
> + struct resource *res;
> + int ret;
> +
> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> + if (!pwm)
> + return -ENOMEM;
> +
> + chip = &pwm->chip;
> + chip->dev = dev;
> + chip->ops = &pwm_sifive_ops;
> + chip->of_xlate = pwm_sifive_xlate;
> + chip->of_pwm_n_cells = 2;
> + chip->base = -1;
> + chip->npwm = 4;
> +
> + ret = of_property_read_u32(node, "sifive,period-ns",
> + &pwm->approx_period);
> +
> + if (ret < 0) {
> + dev_err(dev, "Unable to read sifive,period-ns from DTS\n");
> + return ret;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + pwm->regs = devm_ioremap_resource(dev, res);
> + if (IS_ERR(pwm->regs)) {
> + dev_err(dev, "Unable to map IO resources\n");
> + return PTR_ERR(pwm->regs);
> + }
> +
> + pwm->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(pwm->clk)) {
> + if (PTR_ERR(pwm->clk) != -EPROBE_DEFER)
> + dev_err(dev, "Unable to find controller clock\n");
> + return PTR_ERR(pwm->clk);
> + }
> +
> + ret = clk_prepare_enable(pwm->clk);
> + if (ret) {
> + dev_err(dev, "failed to enable clock for pwm: %d\n", ret);
> + return ret;
> + }
> +
> + /* Watch for changes to underlying clock frequency */
> + pwm->notifier.notifier_call = 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;
> + }
Would it make sense to only enable the clock when the pwm is enabled?
(If yes, how does the output behave if the clock is disabled while the
hardware is enabled? I assume the output just freezes at the state where
it happens to be?)
> + /* Initialize PWM config */
> + 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;
> +
> + ret = pwmchip_remove(&pwm->chip);
> + clk_notifier_unregister(pwm->clk, &pwm->notifier);
> + clk_disable_unprepare(pwm->clk);
> + return ret;
> +}
> +
> +static const struct of_device_id 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");
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-07 10:16 ` Uwe Kleine-König
0 siblings, 0 replies; 53+ messages in thread
From: Uwe Kleine-König @ 2019-02-07 10:16 UTC (permalink / raw)
To: Yash Shah
Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel,
robh+dt, sachin.ghadi, thierry.reding, paul.walmsley,
linux-riscv
Hello,
On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
>
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sifive.c | 261 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 273 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..2b516f7
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,261 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017-2018 SiFive
> + * For SiFive's PWM IP block documentation please refer Chapter 14 of
> + * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> + */
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +/* Register offsets */
> +#define PWM_SIFIVE_PWMCFG 0x0
> +#define PWM_SIFIVE_PWMCOUNT 0x8
> +#define PWM_SIFIVE_PWMS 0x10
> +#define PWM_SIFIVE_PWMCMP0 0x20
> +
> +/* PWMCFG fields */
> +#define PWM_SIFIVE_PWMCFG_SCALE 0
> +#define PWM_SIFIVE_PWMCFG_STICKY 8
> +#define PWM_SIFIVE_PWMCFG_ZERO_CMP 9
> +#define PWM_SIFIVE_PWMCFG_DEGLITCH 10
> +#define PWM_SIFIVE_PWMCFG_EN_ALWAYS 12
> +#define PWM_SIFIVE_PWMCFG_EN_ONCE 13
> +#define PWM_SIFIVE_PWMCFG_CENTER 16
> +#define PWM_SIFIVE_PWMCFG_GANG 24
> +#define PWM_SIFIVE_PWMCFG_IP 28
> +
> +/* SIZE_PWMCMP is used to calculate the offset for pwmcmpX registers */
> +#define SIZE_PWMCMP 4
> +#define CMPWIDTH 16
Please add a PWM_SIFIVE_ prefix to these two.
> +struct pwm_sifive_ddata {
> + struct pwm_chip chip;
> + struct notifier_block notifier;
> + struct clk *clk;
> + void __iomem *regs;
> + unsigned int approx_period;
> + unsigned int real_period;
> +};
> +
> +static inline struct pwm_sifive_ddata *to_pwm_sifive_chip(struct pwm_chip *c)
even though this is inlined I would like this to have use the
pwm_sifive_ prefix, too.
> +{
> + return container_of(c, struct pwm_sifive_ddata, chip);
> +}
> +
> +static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> + struct pwm_state *state)
> +{
> + struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> + u32 duty;
> +
> + duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> +
> + state->period = pwm->real_period;
> + state->duty_cycle = ((u64)duty * pwm->real_period) >> CMPWIDTH;
> + state->polarity = PWM_POLARITY_INVERSED;
> + state->enabled = duty > 0;
> +}
> +
> +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> + struct pwm_state *state)
> +{
> + struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> + unsigned int duty_cycle;
> + u32 frac, val;
> +
> + if (state->polarity != PWM_POLARITY_INVERSED)
> + return -EINVAL;
> +
> + if (state->period != pwm->real_period)
> + return -EINVAL;
> +
> + duty_cycle = state->duty_cycle;
> + if (!state->enabled)
> + duty_cycle = 0;
> +
> + frac = div_u64((u64)duty_cycle * 0xFFFF, state->period);
> + frac = min(frac, 0xFFFFU);
If CMPWIDTH was only 8, we'd need 0xff here instead of 0xffff, right? So
better use
(1 << PWM_SIFIVE_CMPWIDTH) - 1
here, maybe "hidden" behind a cpp define.
In a previous review round I had doubts about the calculation of frac
and I think they were addressed wrongly.
Looking at the figure at the start of chapter 14.9 in the reference
manual: They use (different than the driver here) pwmcmp0=6 and
pwmzerocmp=1. Then a period is 7 clock cycles long and pwmcmpX must be 7
(for X > 0) to yield a 100% signal. So pwmcmpX must be greater than or
equal to the period (in clock cycles).
Here we're using pwmzerocmp=0, still the constraint
pwmcmpX >= period
must be true for a 100% cycle. (Right?) With pwmzerocmp=0 the period is
0x10000 clock cycles, so pwmcmpX must be >= 0x10000 to yield a 100%
signal. As this is not possible (pwmcmpX is a 16 bit value that can only
hold up to 0xffff) the output cannot yield 100%.
So I think the most correct implementation here is:
frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 << PWM_SIFIVE_CMPWIDTH) / 2) / period;
/* Sorry, we cannot do 100% :-( */
frac = min(frac, (1 << PWM_SIFIVE_CMPWIDTH) - 1);
Thierry: What is the correct rounding strategy? I assumed "round to
nearest" (with round half up). If we want something like clk_round_rate
for PWMs in the future rounding down might be easier to handle.
Note that pwm_sifive_get_state is already (well, or still) using this as
it has:
state->duty_cycle = ((u64)duty * pwm->real_period) >> CMPWIDTH;
which always yields something smaller than read_period as duty is
smaller than 1 << PWM_SIFIVE_CMPWIDTH.
I tried to understand the IP source at
https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/pwm/PWM.scala
to confirm (or disprove) this theory, but I failed.
Assuming my analysis is true, I'd like to have it documented
prominently in the driver that the hardware cannot generate a 100% duty
cycle.
> + val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> + val |= (1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
No parenthesis necessary. You might want to consider using the BIT macro
here, maybe already in the definition of PWM_SIFIVE_PWMCFG_DEGLITCH.
> + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> +
> + val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
> + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> + pwm_sifive_get_state(chip, dev, state);
Thierry: This changes the pwm_state. Is this how this should be done?
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops pwm_sifive_ops = {
> + .get_state = pwm_sifive_get_state,
> + .apply = pwm_sifive_apply,
> + .owner = THIS_MODULE,
> +};
> +
> +static struct pwm_device *pwm_sifive_xlate(struct pwm_chip *chip,
> + const struct of_phandle_args *args)
> +{
> + struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> + struct pwm_device *dev;
> +
> + if (args->args[0] >= chip->npwm)
> + return ERR_PTR(-EINVAL);
> +
> + dev = pwm_request_from_chip(chip, args->args[0], NULL);
> + if (IS_ERR(dev))
> + return dev;
> +
> + /* The period cannot be changed on a per-PWM basis */
> + dev->args.period = pwm->real_period;
> + dev->args.polarity = PWM_POLARITY_NORMAL;
> + if (args->args[1] & PWM_POLARITY_INVERSED)
> + dev->args.polarity = PWM_POLARITY_INVERSED;
The driver does only support PWM_POLARITY_INVERSED. So I guess this
should be:
dev->args.polarity = PWM_POLARITY_INVERSED;
if (!(args->args[1] & PWM_POLARITY_INVERSED))
dev_warn(dev, "...");
> +
> + return dev;
> +}
> +
> +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> + unsigned long rate)
> +{
> + /* (1 << (16+scale)) * 10^9/rate = real_period */
> + unsigned long scale_pow =
> + (pwm->approx_period * (u64)rate) / NSEC_PER_SEC;
> + int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
is this 16 in reality PWM_SIFIVE_CMPWIDTH?
> + writel((1 << PWM_SIFIVE_PWMCFG_EN_ALWAYS) | (scale <<
> + PWM_SIFIVE_PWMCFG_SCALE), pwm->regs + PWM_SIFIVE_PWMCFG);
> +
> + /* As scale <= 15 the shift operation cannot overflow. */
> + pwm->real_period = div64_ul(1000000000ULL << (16 + scale), rate);
> + dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
> +}
> +
> +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 device_node *node = pdev->dev.of_node;
> + struct pwm_sifive_ddata *pwm;
> + struct pwm_chip *chip;
> + struct resource *res;
> + int ret;
> +
> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> + if (!pwm)
> + return -ENOMEM;
> +
> + chip = &pwm->chip;
> + chip->dev = dev;
> + chip->ops = &pwm_sifive_ops;
> + chip->of_xlate = pwm_sifive_xlate;
> + chip->of_pwm_n_cells = 2;
> + chip->base = -1;
> + chip->npwm = 4;
> +
> + ret = of_property_read_u32(node, "sifive,period-ns",
> + &pwm->approx_period);
> +
> + if (ret < 0) {
> + dev_err(dev, "Unable to read sifive,period-ns from DTS\n");
> + return ret;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + pwm->regs = devm_ioremap_resource(dev, res);
> + if (IS_ERR(pwm->regs)) {
> + dev_err(dev, "Unable to map IO resources\n");
> + return PTR_ERR(pwm->regs);
> + }
> +
> + pwm->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(pwm->clk)) {
> + if (PTR_ERR(pwm->clk) != -EPROBE_DEFER)
> + dev_err(dev, "Unable to find controller clock\n");
> + return PTR_ERR(pwm->clk);
> + }
> +
> + ret = clk_prepare_enable(pwm->clk);
> + if (ret) {
> + dev_err(dev, "failed to enable clock for pwm: %d\n", ret);
> + return ret;
> + }
> +
> + /* Watch for changes to underlying clock frequency */
> + pwm->notifier.notifier_call = 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;
> + }
Would it make sense to only enable the clock when the pwm is enabled?
(If yes, how does the output behave if the clock is disabled while the
hardware is enabled? I assume the output just freezes at the state where
it happens to be?)
> + /* Initialize PWM config */
> + 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;
> +
> + ret = pwmchip_remove(&pwm->chip);
> + clk_notifier_unregister(pwm->clk, &pwm->notifier);
> + clk_disable_unprepare(pwm->clk);
> + return ret;
> +}
> +
> +static const struct of_device_id 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");
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
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-02-07 10:16 ` Uwe Kleine-König
@ 2019-02-11 11:26 ` Yash Shah
-1 siblings, 0 replies; 53+ messages in thread
From: Yash Shah @ 2019-02-11 11:26 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Palmer Dabbelt, linux-pwm, linux-riscv, Thierry Reding, robh+dt,
mark.rutland, devicetree, linux-kernel, Sachin Ghadi,
Paul Walmsley
On Thu, Feb 7, 2019 at 3:47 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> >
> > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> > drivers/pwm/Kconfig | 11 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-sifive.c | 261 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 273 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..2b516f7
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,261 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2017-2018 SiFive
> > + * For SiFive's PWM IP block documentation please refer Chapter 14 of
> > + * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/slab.h>
> > +
> > +/* Register offsets */
> > +#define PWM_SIFIVE_PWMCFG 0x0
> > +#define PWM_SIFIVE_PWMCOUNT 0x8
> > +#define PWM_SIFIVE_PWMS 0x10
> > +#define PWM_SIFIVE_PWMCMP0 0x20
> > +
> > +/* PWMCFG fields */
> > +#define PWM_SIFIVE_PWMCFG_SCALE 0
> > +#define PWM_SIFIVE_PWMCFG_STICKY 8
> > +#define PWM_SIFIVE_PWMCFG_ZERO_CMP 9
> > +#define PWM_SIFIVE_PWMCFG_DEGLITCH 10
> > +#define PWM_SIFIVE_PWMCFG_EN_ALWAYS 12
> > +#define PWM_SIFIVE_PWMCFG_EN_ONCE 13
> > +#define PWM_SIFIVE_PWMCFG_CENTER 16
> > +#define PWM_SIFIVE_PWMCFG_GANG 24
> > +#define PWM_SIFIVE_PWMCFG_IP 28
> > +
> > +/* SIZE_PWMCMP is used to calculate the offset for pwmcmpX registers */
> > +#define SIZE_PWMCMP 4
> > +#define CMPWIDTH 16
>
> Please add a PWM_SIFIVE_ prefix to these two.
Sure
>
> > +struct pwm_sifive_ddata {
> > + struct pwm_chip chip;
> > + struct notifier_block notifier;
> > + struct clk *clk;
> > + void __iomem *regs;
> > + unsigned int approx_period;
> > + unsigned int real_period;
> > +};
> > +
> > +static inline struct pwm_sifive_ddata *to_pwm_sifive_chip(struct pwm_chip *c)
>
> even though this is inlined I would like this to have use the
> pwm_sifive_ prefix, too.
Not sure what exactly you want me to do here.
>
> > +{
> > + return container_of(c, struct pwm_sifive_ddata, chip);
> > +}
> > +
> > +static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> > + struct pwm_state *state)
> > +{
> > + struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> > + u32 duty;
> > +
> > + duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > +
> > + state->period = pwm->real_period;
> > + state->duty_cycle = ((u64)duty * pwm->real_period) >> CMPWIDTH;
> > + state->polarity = PWM_POLARITY_INVERSED;
> > + state->enabled = duty > 0;
> > +}
> > +
> > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> > + struct pwm_state *state)
> > +{
> > + struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> > + unsigned int duty_cycle;
> > + u32 frac, val;
> > +
> > + if (state->polarity != PWM_POLARITY_INVERSED)
> > + return -EINVAL;
> > +
> > + if (state->period != pwm->real_period)
> > + return -EINVAL;
> > +
> > + duty_cycle = state->duty_cycle;
> > + if (!state->enabled)
> > + duty_cycle = 0;
> > +
> > + frac = div_u64((u64)duty_cycle * 0xFFFF, state->period);
> > + frac = min(frac, 0xFFFFU);
>
> If CMPWIDTH was only 8, we'd need 0xff here instead of 0xffff, right? So
> better use
>
> (1 << PWM_SIFIVE_CMPWIDTH) - 1
>
> here, maybe "hidden" behind a cpp define.
Yes, right. I will make this change.
>
> In a previous review round I had doubts about the calculation of frac
> and I think they were addressed wrongly.
> Looking at the figure at the start of chapter 14.9 in the reference
> manual: They use (different than the driver here) pwmcmp0=6 and
> pwmzerocmp=1. Then a period is 7 clock cycles long and pwmcmpX must be 7
> (for X > 0) to yield a 100% signal. So pwmcmpX must be greater than or
> equal to the period (in clock cycles).
>
> Here we're using pwmzerocmp=0, still the constraint
>
> pwmcmpX >= period
>
> must be true for a 100% cycle. (Right?) With pwmzerocmp=0 the period is
> 0x10000 clock cycles, so pwmcmpX must be >= 0x10000 to yield a 100%
> signal. As this is not possible (pwmcmpX is a 16 bit value that can only
> hold up to 0xffff) the output cannot yield 100%.
>
> So I think the most correct implementation here is:
>
> frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 << PWM_SIFIVE_CMPWIDTH) / 2) / period;
> /* Sorry, we cannot do 100% :-( */
> frac = min(frac, (1 << PWM_SIFIVE_CMPWIDTH) - 1);
>
I will once again go through the specs, if it seems correct I will
implement the above.
> Thierry: What is the correct rounding strategy? I assumed "round to
> nearest" (with round half up). If we want something like clk_round_rate
> for PWMs in the future rounding down might be easier to handle.
>
> Note that pwm_sifive_get_state is already (well, or still) using this as
> it has:
>
> state->duty_cycle = ((u64)duty * pwm->real_period) >> CMPWIDTH;
>
> which always yields something smaller than read_period as duty is
> smaller than 1 << PWM_SIFIVE_CMPWIDTH.
>
> I tried to understand the IP source at
> https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/pwm/PWM.scala
> to confirm (or disprove) this theory, but I failed.
>
> Assuming my analysis is true, I'd like to have it documented
> prominently in the driver that the hardware cannot generate a 100% duty
> cycle.
>
> > + val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > + val |= (1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
>
> No parenthesis necessary. You might want to consider using the BIT macro
> here, maybe already in the definition of PWM_SIFIVE_PWMCFG_DEGLITCH.
Sure, will use BIT macro.
>
> > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > +
> > + val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
> > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > + pwm_sifive_get_state(chip, dev, state);
>
> Thierry: This changes the pwm_state. Is this how this should be done?
I am removing this 'pwm_sifive_get_state' call (No state correction in any case)
>
> > +
> > + return 0;
> > +}
> > +
> > +static const struct pwm_ops pwm_sifive_ops = {
> > + .get_state = pwm_sifive_get_state,
> > + .apply = pwm_sifive_apply,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static struct pwm_device *pwm_sifive_xlate(struct pwm_chip *chip,
> > + const struct of_phandle_args *args)
> > +{
> > + struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> > + struct pwm_device *dev;
> > +
> > + if (args->args[0] >= chip->npwm)
> > + return ERR_PTR(-EINVAL);
> > +
> > + dev = pwm_request_from_chip(chip, args->args[0], NULL);
> > + if (IS_ERR(dev))
> > + return dev;
> > +
> > + /* The period cannot be changed on a per-PWM basis */
> > + dev->args.period = pwm->real_period;
> > + dev->args.polarity = PWM_POLARITY_NORMAL;
> > + if (args->args[1] & PWM_POLARITY_INVERSED)
> > + dev->args.polarity = PWM_POLARITY_INVERSED;
>
> The driver does only support PWM_POLARITY_INVERSED. So I guess this
> should be:
>
> dev->args.polarity = PWM_POLARITY_INVERSED;
> if (!(args->args[1] & PWM_POLARITY_INVERSED))
> dev_warn(dev, "...");
Ok, will make this change.
>
> > +
> > + return dev;
> > +}
> > +
> > +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> > + unsigned long rate)
> > +{
> > + /* (1 << (16+scale)) * 10^9/rate = real_period */
> > + unsigned long scale_pow =
> > + (pwm->approx_period * (u64)rate) / NSEC_PER_SEC;
> > + int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
>
> is this 16 in reality PWM_SIFIVE_CMPWIDTH?
Yes, it seems so. I will replace it with the macro.
>
> > + writel((1 << PWM_SIFIVE_PWMCFG_EN_ALWAYS) | (scale <<
> > + PWM_SIFIVE_PWMCFG_SCALE), pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > + /* As scale <= 15 the shift operation cannot overflow. */
> > + pwm->real_period = div64_ul(1000000000ULL << (16 + scale), rate);
> > + dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
> > +}
> > +
> > +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 device_node *node = pdev->dev.of_node;
> > + struct pwm_sifive_ddata *pwm;
> > + struct pwm_chip *chip;
> > + struct resource *res;
> > + int ret;
> > +
> > + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> > + if (!pwm)
> > + return -ENOMEM;
> > +
> > + chip = &pwm->chip;
> > + chip->dev = dev;
> > + chip->ops = &pwm_sifive_ops;
> > + chip->of_xlate = pwm_sifive_xlate;
> > + chip->of_pwm_n_cells = 2;
> > + chip->base = -1;
> > + chip->npwm = 4;
> > +
> > + ret = of_property_read_u32(node, "sifive,period-ns",
> > + &pwm->approx_period);
> > +
> > + if (ret < 0) {
> > + dev_err(dev, "Unable to read sifive,period-ns from DTS\n");
> > + return ret;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + pwm->regs = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(pwm->regs)) {
> > + dev_err(dev, "Unable to map IO resources\n");
> > + return PTR_ERR(pwm->regs);
> > + }
> > +
> > + pwm->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(pwm->clk)) {
> > + if (PTR_ERR(pwm->clk) != -EPROBE_DEFER)
> > + dev_err(dev, "Unable to find controller clock\n");
> > + return PTR_ERR(pwm->clk);
> > + }
> > +
> > + ret = clk_prepare_enable(pwm->clk);
> > + if (ret) {
> > + dev_err(dev, "failed to enable clock for pwm: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + /* Watch for changes to underlying clock frequency */
> > + pwm->notifier.notifier_call = 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;
> > + }
>
> Would it make sense to only enable the clock when the pwm is enabled?
> (If yes, how does the output behave if the clock is disabled while the
> hardware is enabled? I assume the output just freezes at the state where
> it happens to be?)
Yes. Not sure about the behaviour of hardware when the clock is disabled.
The behaviour should be like you said. If there is no strong reason
then we can keep it as it is right now
or else if you want I can make the necessary change.
>
> > + /* Initialize PWM config */
> > + 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;
> > +
> > + ret = pwmchip_remove(&pwm->chip);
> > + clk_notifier_unregister(pwm->clk, &pwm->notifier);
> > + clk_disable_unprepare(pwm->clk);
> > + return ret;
> > +}
> > +
> > +static const struct of_device_id 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");
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-11 11:26 ` Yash Shah
0 siblings, 0 replies; 53+ messages in thread
From: Yash Shah @ 2019-02-11 11:26 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, robh+dt, Sachin Ghadi, Thierry Reding,
Paul Walmsley, linux-riscv
On Thu, Feb 7, 2019 at 3:47 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> >
> > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> > drivers/pwm/Kconfig | 11 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-sifive.c | 261 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 273 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..2b516f7
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,261 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2017-2018 SiFive
> > + * For SiFive's PWM IP block documentation please refer Chapter 14 of
> > + * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/slab.h>
> > +
> > +/* Register offsets */
> > +#define PWM_SIFIVE_PWMCFG 0x0
> > +#define PWM_SIFIVE_PWMCOUNT 0x8
> > +#define PWM_SIFIVE_PWMS 0x10
> > +#define PWM_SIFIVE_PWMCMP0 0x20
> > +
> > +/* PWMCFG fields */
> > +#define PWM_SIFIVE_PWMCFG_SCALE 0
> > +#define PWM_SIFIVE_PWMCFG_STICKY 8
> > +#define PWM_SIFIVE_PWMCFG_ZERO_CMP 9
> > +#define PWM_SIFIVE_PWMCFG_DEGLITCH 10
> > +#define PWM_SIFIVE_PWMCFG_EN_ALWAYS 12
> > +#define PWM_SIFIVE_PWMCFG_EN_ONCE 13
> > +#define PWM_SIFIVE_PWMCFG_CENTER 16
> > +#define PWM_SIFIVE_PWMCFG_GANG 24
> > +#define PWM_SIFIVE_PWMCFG_IP 28
> > +
> > +/* SIZE_PWMCMP is used to calculate the offset for pwmcmpX registers */
> > +#define SIZE_PWMCMP 4
> > +#define CMPWIDTH 16
>
> Please add a PWM_SIFIVE_ prefix to these two.
Sure
>
> > +struct pwm_sifive_ddata {
> > + struct pwm_chip chip;
> > + struct notifier_block notifier;
> > + struct clk *clk;
> > + void __iomem *regs;
> > + unsigned int approx_period;
> > + unsigned int real_period;
> > +};
> > +
> > +static inline struct pwm_sifive_ddata *to_pwm_sifive_chip(struct pwm_chip *c)
>
> even though this is inlined I would like this to have use the
> pwm_sifive_ prefix, too.
Not sure what exactly you want me to do here.
>
> > +{
> > + return container_of(c, struct pwm_sifive_ddata, chip);
> > +}
> > +
> > +static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> > + struct pwm_state *state)
> > +{
> > + struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> > + u32 duty;
> > +
> > + duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > +
> > + state->period = pwm->real_period;
> > + state->duty_cycle = ((u64)duty * pwm->real_period) >> CMPWIDTH;
> > + state->polarity = PWM_POLARITY_INVERSED;
> > + state->enabled = duty > 0;
> > +}
> > +
> > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> > + struct pwm_state *state)
> > +{
> > + struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> > + unsigned int duty_cycle;
> > + u32 frac, val;
> > +
> > + if (state->polarity != PWM_POLARITY_INVERSED)
> > + return -EINVAL;
> > +
> > + if (state->period != pwm->real_period)
> > + return -EINVAL;
> > +
> > + duty_cycle = state->duty_cycle;
> > + if (!state->enabled)
> > + duty_cycle = 0;
> > +
> > + frac = div_u64((u64)duty_cycle * 0xFFFF, state->period);
> > + frac = min(frac, 0xFFFFU);
>
> If CMPWIDTH was only 8, we'd need 0xff here instead of 0xffff, right? So
> better use
>
> (1 << PWM_SIFIVE_CMPWIDTH) - 1
>
> here, maybe "hidden" behind a cpp define.
Yes, right. I will make this change.
>
> In a previous review round I had doubts about the calculation of frac
> and I think they were addressed wrongly.
> Looking at the figure at the start of chapter 14.9 in the reference
> manual: They use (different than the driver here) pwmcmp0=6 and
> pwmzerocmp=1. Then a period is 7 clock cycles long and pwmcmpX must be 7
> (for X > 0) to yield a 100% signal. So pwmcmpX must be greater than or
> equal to the period (in clock cycles).
>
> Here we're using pwmzerocmp=0, still the constraint
>
> pwmcmpX >= period
>
> must be true for a 100% cycle. (Right?) With pwmzerocmp=0 the period is
> 0x10000 clock cycles, so pwmcmpX must be >= 0x10000 to yield a 100%
> signal. As this is not possible (pwmcmpX is a 16 bit value that can only
> hold up to 0xffff) the output cannot yield 100%.
>
> So I think the most correct implementation here is:
>
> frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 << PWM_SIFIVE_CMPWIDTH) / 2) / period;
> /* Sorry, we cannot do 100% :-( */
> frac = min(frac, (1 << PWM_SIFIVE_CMPWIDTH) - 1);
>
I will once again go through the specs, if it seems correct I will
implement the above.
> Thierry: What is the correct rounding strategy? I assumed "round to
> nearest" (with round half up). If we want something like clk_round_rate
> for PWMs in the future rounding down might be easier to handle.
>
> Note that pwm_sifive_get_state is already (well, or still) using this as
> it has:
>
> state->duty_cycle = ((u64)duty * pwm->real_period) >> CMPWIDTH;
>
> which always yields something smaller than read_period as duty is
> smaller than 1 << PWM_SIFIVE_CMPWIDTH.
>
> I tried to understand the IP source at
> https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/pwm/PWM.scala
> to confirm (or disprove) this theory, but I failed.
>
> Assuming my analysis is true, I'd like to have it documented
> prominently in the driver that the hardware cannot generate a 100% duty
> cycle.
>
> > + val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > + val |= (1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
>
> No parenthesis necessary. You might want to consider using the BIT macro
> here, maybe already in the definition of PWM_SIFIVE_PWMCFG_DEGLITCH.
Sure, will use BIT macro.
>
> > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > +
> > + val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
> > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > + pwm_sifive_get_state(chip, dev, state);
>
> Thierry: This changes the pwm_state. Is this how this should be done?
I am removing this 'pwm_sifive_get_state' call (No state correction in any case)
>
> > +
> > + return 0;
> > +}
> > +
> > +static const struct pwm_ops pwm_sifive_ops = {
> > + .get_state = pwm_sifive_get_state,
> > + .apply = pwm_sifive_apply,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static struct pwm_device *pwm_sifive_xlate(struct pwm_chip *chip,
> > + const struct of_phandle_args *args)
> > +{
> > + struct pwm_sifive_ddata *pwm = to_pwm_sifive_chip(chip);
> > + struct pwm_device *dev;
> > +
> > + if (args->args[0] >= chip->npwm)
> > + return ERR_PTR(-EINVAL);
> > +
> > + dev = pwm_request_from_chip(chip, args->args[0], NULL);
> > + if (IS_ERR(dev))
> > + return dev;
> > +
> > + /* The period cannot be changed on a per-PWM basis */
> > + dev->args.period = pwm->real_period;
> > + dev->args.polarity = PWM_POLARITY_NORMAL;
> > + if (args->args[1] & PWM_POLARITY_INVERSED)
> > + dev->args.polarity = PWM_POLARITY_INVERSED;
>
> The driver does only support PWM_POLARITY_INVERSED. So I guess this
> should be:
>
> dev->args.polarity = PWM_POLARITY_INVERSED;
> if (!(args->args[1] & PWM_POLARITY_INVERSED))
> dev_warn(dev, "...");
Ok, will make this change.
>
> > +
> > + return dev;
> > +}
> > +
> > +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm,
> > + unsigned long rate)
> > +{
> > + /* (1 << (16+scale)) * 10^9/rate = real_period */
> > + unsigned long scale_pow =
> > + (pwm->approx_period * (u64)rate) / NSEC_PER_SEC;
> > + int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
>
> is this 16 in reality PWM_SIFIVE_CMPWIDTH?
Yes, it seems so. I will replace it with the macro.
>
> > + writel((1 << PWM_SIFIVE_PWMCFG_EN_ALWAYS) | (scale <<
> > + PWM_SIFIVE_PWMCFG_SCALE), pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > + /* As scale <= 15 the shift operation cannot overflow. */
> > + pwm->real_period = div64_ul(1000000000ULL << (16 + scale), rate);
> > + dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
> > +}
> > +
> > +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 device_node *node = pdev->dev.of_node;
> > + struct pwm_sifive_ddata *pwm;
> > + struct pwm_chip *chip;
> > + struct resource *res;
> > + int ret;
> > +
> > + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> > + if (!pwm)
> > + return -ENOMEM;
> > +
> > + chip = &pwm->chip;
> > + chip->dev = dev;
> > + chip->ops = &pwm_sifive_ops;
> > + chip->of_xlate = pwm_sifive_xlate;
> > + chip->of_pwm_n_cells = 2;
> > + chip->base = -1;
> > + chip->npwm = 4;
> > +
> > + ret = of_property_read_u32(node, "sifive,period-ns",
> > + &pwm->approx_period);
> > +
> > + if (ret < 0) {
> > + dev_err(dev, "Unable to read sifive,period-ns from DTS\n");
> > + return ret;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + pwm->regs = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(pwm->regs)) {
> > + dev_err(dev, "Unable to map IO resources\n");
> > + return PTR_ERR(pwm->regs);
> > + }
> > +
> > + pwm->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(pwm->clk)) {
> > + if (PTR_ERR(pwm->clk) != -EPROBE_DEFER)
> > + dev_err(dev, "Unable to find controller clock\n");
> > + return PTR_ERR(pwm->clk);
> > + }
> > +
> > + ret = clk_prepare_enable(pwm->clk);
> > + if (ret) {
> > + dev_err(dev, "failed to enable clock for pwm: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + /* Watch for changes to underlying clock frequency */
> > + pwm->notifier.notifier_call = 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;
> > + }
>
> Would it make sense to only enable the clock when the pwm is enabled?
> (If yes, how does the output behave if the clock is disabled while the
> hardware is enabled? I assume the output just freezes at the state where
> it happens to be?)
Yes. Not sure about the behaviour of hardware when the clock is disabled.
The behaviour should be like you said. If there is no strong reason
then we can keep it as it is right now
or else if you want I can make the necessary change.
>
> > + /* Initialize PWM config */
> > + 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;
> > +
> > + ret = pwmchip_remove(&pwm->chip);
> > + clk_notifier_unregister(pwm->clk, &pwm->notifier);
> > + clk_disable_unprepare(pwm->clk);
> > + return ret;
> > +}
> > +
> > +static const struct of_device_id 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");
>
> 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
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-02-11 11:26 ` Yash Shah
@ 2019-02-11 12:29 ` Uwe Kleine-König
-1 siblings, 0 replies; 53+ messages in thread
From: Uwe Kleine-König @ 2019-02-11 12:29 UTC (permalink / raw)
To: Yash Shah
Cc: Palmer Dabbelt, linux-pwm, linux-riscv, Thierry Reding, robh+dt,
mark.rutland, devicetree, linux-kernel, Sachin Ghadi,
Paul Walmsley
Hello,
On Mon, Feb 11, 2019 at 04:56:27PM +0530, Yash Shah wrote:
> On Thu, Feb 7, 2019 at 3:47 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> > > +struct pwm_sifive_ddata {
> > > + struct pwm_chip chip;
> > > + struct notifier_block notifier;
> > > + struct clk *clk;
> > > + void __iomem *regs;
> > > + unsigned int approx_period;
> > > + unsigned int real_period;
> > > +};
> > > +
> > > +static inline struct pwm_sifive_ddata *to_pwm_sifive_chip(struct pwm_chip *c)
> >
> > even though this is inlined I would like this to have use the
> > pwm_sifive_ prefix, too.
>
> Not sure what exactly you want me to do here.
I want this function to be named something like pwm_sifive_chip_to_ddata.
This way it has the common prefix (and is less confusing because
to..chip suggests that is converts something to a chip, but the outcome
is ...ddata).
> > In a previous review round I had doubts about the calculation of frac
> > and I think they were addressed wrongly.
> > Looking at the figure at the start of chapter 14.9 in the reference
> > manual: They use (different than the driver here) pwmcmp0=6 and
> > pwmzerocmp=1. Then a period is 7 clock cycles long and pwmcmpX must be 7
> > (for X > 0) to yield a 100% signal. So pwmcmpX must be greater than or
> > equal to the period (in clock cycles).
> >
> > Here we're using pwmzerocmp=0, still the constraint
> >
> > pwmcmpX >= period
> >
> > must be true for a 100% cycle. (Right?) With pwmzerocmp=0 the period is
> > 0x10000 clock cycles, so pwmcmpX must be >= 0x10000 to yield a 100%
> > signal. As this is not possible (pwmcmpX is a 16 bit value that can only
> > hold up to 0xffff) the output cannot yield 100%.
> >
> > So I think the most correct implementation here is:
> >
> > frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 << PWM_SIFIVE_CMPWIDTH) / 2) / period;
> > /* Sorry, we cannot do 100% :-( */
> > frac = min(frac, (1 << PWM_SIFIVE_CMPWIDTH) - 1);
> >
>
> I will once again go through the specs, if it seems correct I will
> implement the above.
I would just test this at runtime. Configure for 100% and check the wave
form if it is constant.
> > > +static int pwm_sifive_probe(struct platform_device *pdev)
> > > +{
> > > [...]
> > > + /* 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;
> > > + }
> >
> > Would it make sense to only enable the clock when the pwm is enabled?
> > (If yes, how does the output behave if the clock is disabled while the
> > hardware is enabled? I assume the output just freezes at the state where
> > it happens to be?)
>
> Yes. Not sure about the behaviour of hardware when the clock is disabled.
> The behaviour should be like you said. If there is no strong reason
> then we can keep it as it is right now
> or else if you want I can make the necessary change.
I would prefer to only have the clock enabled when it is actually used.
Note the common pitfall that you cannot disable the clk immediately
after configureing the duty cycle to 100% or 0% though because you must
only stop the clock when the newly configured parameters are active.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-11 12:29 ` Uwe Kleine-König
0 siblings, 0 replies; 53+ messages in thread
From: Uwe Kleine-König @ 2019-02-11 12:29 UTC (permalink / raw)
To: Yash Shah
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, robh+dt, Sachin Ghadi, Thierry Reding,
Paul Walmsley, linux-riscv
Hello,
On Mon, Feb 11, 2019 at 04:56:27PM +0530, Yash Shah wrote:
> On Thu, Feb 7, 2019 at 3:47 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> > > +struct pwm_sifive_ddata {
> > > + struct pwm_chip chip;
> > > + struct notifier_block notifier;
> > > + struct clk *clk;
> > > + void __iomem *regs;
> > > + unsigned int approx_period;
> > > + unsigned int real_period;
> > > +};
> > > +
> > > +static inline struct pwm_sifive_ddata *to_pwm_sifive_chip(struct pwm_chip *c)
> >
> > even though this is inlined I would like this to have use the
> > pwm_sifive_ prefix, too.
>
> Not sure what exactly you want me to do here.
I want this function to be named something like pwm_sifive_chip_to_ddata.
This way it has the common prefix (and is less confusing because
to..chip suggests that is converts something to a chip, but the outcome
is ...ddata).
> > In a previous review round I had doubts about the calculation of frac
> > and I think they were addressed wrongly.
> > Looking at the figure at the start of chapter 14.9 in the reference
> > manual: They use (different than the driver here) pwmcmp0=6 and
> > pwmzerocmp=1. Then a period is 7 clock cycles long and pwmcmpX must be 7
> > (for X > 0) to yield a 100% signal. So pwmcmpX must be greater than or
> > equal to the period (in clock cycles).
> >
> > Here we're using pwmzerocmp=0, still the constraint
> >
> > pwmcmpX >= period
> >
> > must be true for a 100% cycle. (Right?) With pwmzerocmp=0 the period is
> > 0x10000 clock cycles, so pwmcmpX must be >= 0x10000 to yield a 100%
> > signal. As this is not possible (pwmcmpX is a 16 bit value that can only
> > hold up to 0xffff) the output cannot yield 100%.
> >
> > So I think the most correct implementation here is:
> >
> > frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 << PWM_SIFIVE_CMPWIDTH) / 2) / period;
> > /* Sorry, we cannot do 100% :-( */
> > frac = min(frac, (1 << PWM_SIFIVE_CMPWIDTH) - 1);
> >
>
> I will once again go through the specs, if it seems correct I will
> implement the above.
I would just test this at runtime. Configure for 100% and check the wave
form if it is constant.
> > > +static int pwm_sifive_probe(struct platform_device *pdev)
> > > +{
> > > [...]
> > > + /* 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;
> > > + }
> >
> > Would it make sense to only enable the clock when the pwm is enabled?
> > (If yes, how does the output behave if the clock is disabled while the
> > hardware is enabled? I assume the output just freezes at the state where
> > it happens to be?)
>
> Yes. Not sure about the behaviour of hardware when the clock is disabled.
> The behaviour should be like you said. If there is no strong reason
> then we can keep it as it is right now
> or else if you want I can make the necessary change.
I would prefer to only have the clock enabled when it is actually used.
Note the common pitfall that you cannot disable the clk immediately
after configureing the duty cycle to 100% or 0% though because you must
only stop the clock when the newly configured parameters are active.
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
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-02-11 12:29 ` Uwe Kleine-König
@ 2019-02-13 12:34 ` Thierry Reding
-1 siblings, 0 replies; 53+ messages in thread
From: Thierry Reding @ 2019-02-13 12:34 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Yash Shah, Palmer Dabbelt, linux-pwm, linux-riscv, robh+dt,
mark.rutland, devicetree, linux-kernel, Sachin Ghadi,
Paul Walmsley
[-- Attachment #1: Type: text/plain, Size: 4322 bytes --]
On Mon, Feb 11, 2019 at 01:29:54PM +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Mon, Feb 11, 2019 at 04:56:27PM +0530, Yash Shah wrote:
> > On Thu, Feb 7, 2019 at 3:47 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> > > > +struct pwm_sifive_ddata {
> > > > + struct pwm_chip chip;
> > > > + struct notifier_block notifier;
> > > > + struct clk *clk;
> > > > + void __iomem *regs;
> > > > + unsigned int approx_period;
> > > > + unsigned int real_period;
> > > > +};
> > > > +
> > > > +static inline struct pwm_sifive_ddata *to_pwm_sifive_chip(struct pwm_chip *c)
> > >
> > > even though this is inlined I would like this to have use the
> > > pwm_sifive_ prefix, too.
> >
> > Not sure what exactly you want me to do here.
>
> I want this function to be named something like pwm_sifive_chip_to_ddata.
> This way it has the common prefix (and is less confusing because
> to..chip suggests that is converts something to a chip, but the outcome
> is ...ddata).
We've got plenty of these cast functions that are named to_*(). I would
argue that naming this one differently is confusing.
Also, I think this may have come from earlier review comments where I
suggested to name this structure struct pwm_sifive_chip, or just struct
pwm_sifive.
> > > In a previous review round I had doubts about the calculation of frac
> > > and I think they were addressed wrongly.
> > > Looking at the figure at the start of chapter 14.9 in the reference
> > > manual: They use (different than the driver here) pwmcmp0=6 and
> > > pwmzerocmp=1. Then a period is 7 clock cycles long and pwmcmpX must be 7
> > > (for X > 0) to yield a 100% signal. So pwmcmpX must be greater than or
> > > equal to the period (in clock cycles).
> > >
> > > Here we're using pwmzerocmp=0, still the constraint
> > >
> > > pwmcmpX >= period
> > >
> > > must be true for a 100% cycle. (Right?) With pwmzerocmp=0 the period is
> > > 0x10000 clock cycles, so pwmcmpX must be >= 0x10000 to yield a 100%
> > > signal. As this is not possible (pwmcmpX is a 16 bit value that can only
> > > hold up to 0xffff) the output cannot yield 100%.
> > >
> > > So I think the most correct implementation here is:
> > >
> > > frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 << PWM_SIFIVE_CMPWIDTH) / 2) / period;
> > > /* Sorry, we cannot do 100% :-( */
> > > frac = min(frac, (1 << PWM_SIFIVE_CMPWIDTH) - 1);
> > >
> >
> > I will once again go through the specs, if it seems correct I will
> > implement the above.
>
> I would just test this at runtime. Configure for 100% and check the wave
> form if it is constant.
>
> > > > +static int pwm_sifive_probe(struct platform_device *pdev)
> > > > +{
> > > > [...]
> > > > + /* 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;
> > > > + }
> > >
> > > Would it make sense to only enable the clock when the pwm is enabled?
> > > (If yes, how does the output behave if the clock is disabled while the
> > > hardware is enabled? I assume the output just freezes at the state where
> > > it happens to be?)
> >
> > Yes. Not sure about the behaviour of hardware when the clock is disabled.
> > The behaviour should be like you said. If there is no strong reason
> > then we can keep it as it is right now
> > or else if you want I can make the necessary change.
>
> I would prefer to only have the clock enabled when it is actually used.
> Note the common pitfall that you cannot disable the clk immediately
> after configureing the duty cycle to 100% or 0% though because you must
> only stop the clock when the newly configured parameters are active.
You can also disable the clock if the hardware will still apply the
configured parameters. This is really only an issue if disabling the
clock also stops the IP and prevents the configured parameters from
getting applied.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-13 12:34 ` Thierry Reding
0 siblings, 0 replies; 53+ messages in thread
From: Thierry Reding @ 2019-02-13 12:34 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, Sachin Ghadi, Yash Shah, robh+dt, Paul Walmsley,
linux-riscv
[-- Attachment #1.1: Type: text/plain, Size: 4322 bytes --]
On Mon, Feb 11, 2019 at 01:29:54PM +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Mon, Feb 11, 2019 at 04:56:27PM +0530, Yash Shah wrote:
> > On Thu, Feb 7, 2019 at 3:47 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> > > > +struct pwm_sifive_ddata {
> > > > + struct pwm_chip chip;
> > > > + struct notifier_block notifier;
> > > > + struct clk *clk;
> > > > + void __iomem *regs;
> > > > + unsigned int approx_period;
> > > > + unsigned int real_period;
> > > > +};
> > > > +
> > > > +static inline struct pwm_sifive_ddata *to_pwm_sifive_chip(struct pwm_chip *c)
> > >
> > > even though this is inlined I would like this to have use the
> > > pwm_sifive_ prefix, too.
> >
> > Not sure what exactly you want me to do here.
>
> I want this function to be named something like pwm_sifive_chip_to_ddata.
> This way it has the common prefix (and is less confusing because
> to..chip suggests that is converts something to a chip, but the outcome
> is ...ddata).
We've got plenty of these cast functions that are named to_*(). I would
argue that naming this one differently is confusing.
Also, I think this may have come from earlier review comments where I
suggested to name this structure struct pwm_sifive_chip, or just struct
pwm_sifive.
> > > In a previous review round I had doubts about the calculation of frac
> > > and I think they were addressed wrongly.
> > > Looking at the figure at the start of chapter 14.9 in the reference
> > > manual: They use (different than the driver here) pwmcmp0=6 and
> > > pwmzerocmp=1. Then a period is 7 clock cycles long and pwmcmpX must be 7
> > > (for X > 0) to yield a 100% signal. So pwmcmpX must be greater than or
> > > equal to the period (in clock cycles).
> > >
> > > Here we're using pwmzerocmp=0, still the constraint
> > >
> > > pwmcmpX >= period
> > >
> > > must be true for a 100% cycle. (Right?) With pwmzerocmp=0 the period is
> > > 0x10000 clock cycles, so pwmcmpX must be >= 0x10000 to yield a 100%
> > > signal. As this is not possible (pwmcmpX is a 16 bit value that can only
> > > hold up to 0xffff) the output cannot yield 100%.
> > >
> > > So I think the most correct implementation here is:
> > >
> > > frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 << PWM_SIFIVE_CMPWIDTH) / 2) / period;
> > > /* Sorry, we cannot do 100% :-( */
> > > frac = min(frac, (1 << PWM_SIFIVE_CMPWIDTH) - 1);
> > >
> >
> > I will once again go through the specs, if it seems correct I will
> > implement the above.
>
> I would just test this at runtime. Configure for 100% and check the wave
> form if it is constant.
>
> > > > +static int pwm_sifive_probe(struct platform_device *pdev)
> > > > +{
> > > > [...]
> > > > + /* 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;
> > > > + }
> > >
> > > Would it make sense to only enable the clock when the pwm is enabled?
> > > (If yes, how does the output behave if the clock is disabled while the
> > > hardware is enabled? I assume the output just freezes at the state where
> > > it happens to be?)
> >
> > Yes. Not sure about the behaviour of hardware when the clock is disabled.
> > The behaviour should be like you said. If there is no strong reason
> > then we can keep it as it is right now
> > or else if you want I can make the necessary change.
>
> I would prefer to only have the clock enabled when it is actually used.
> Note the common pitfall that you cannot disable the clk immediately
> after configureing the duty cycle to 100% or 0% though because you must
> only stop the clock when the newly configured parameters are active.
You can also disable the clock if the hardware will still apply the
configured parameters. This is really only an issue if disabling the
clock also stops the IP and prevents the configured parameters from
getting applied.
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
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-02-13 12:34 ` Thierry Reding
(?)
@ 2019-02-13 17:39 ` Uwe Kleine-König
-1 siblings, 0 replies; 53+ messages in thread
From: Uwe Kleine-König @ 2019-02-13 17:39 UTC (permalink / raw)
To: Thierry Reding
Cc: Yash Shah, Palmer Dabbelt, linux-pwm, linux-riscv, robh+dt,
mark.rutland, devicetree, linux-kernel, Sachin Ghadi,
Paul Walmsley
On Wed, Feb 13, 2019 at 01:34:05PM +0100, Thierry Reding wrote:
> On Mon, Feb 11, 2019 at 01:29:54PM +0100, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Mon, Feb 11, 2019 at 04:56:27PM +0530, Yash Shah wrote:
> > > On Thu, Feb 7, 2019 at 3:47 PM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> > > > > +struct pwm_sifive_ddata {
> > > > > + struct pwm_chip chip;
> > > > > + struct notifier_block notifier;
> > > > > + struct clk *clk;
> > > > > + void __iomem *regs;
> > > > > + unsigned int approx_period;
> > > > > + unsigned int real_period;
> > > > > +};
> > > > > +
> > > > > +static inline struct pwm_sifive_ddata *to_pwm_sifive_chip(struct pwm_chip *c)
> > > >
> > > > even though this is inlined I would like this to have use the
> > > > pwm_sifive_ prefix, too.
> > >
> > > Not sure what exactly you want me to do here.
> >
> > I want this function to be named something like pwm_sifive_chip_to_ddata.
> > This way it has the common prefix (and is less confusing because
> > to..chip suggests that is converts something to a chip, but the outcome
> > is ...ddata).
>
> We've got plenty of these cast functions that are named to_*(). I would
> argue that naming this one differently is confusing.
The benefit of consistenly use a common prefix for all functions of a
driver is that applying a filter when tracing is much easier. Sure, you
could argue that this function is inlined and too trivial to care about
in a trace. Recommending to not make any exceptions to the "common
prefix" rule is easier though than to judge all functions that don't use
it if it is worth to adhere to the rule or not.
> Also, I think this may have come from earlier review comments where I
> suggested to name this structure struct pwm_sifive_chip, or just struct
> pwm_sifive.
Even with this argument to_pwm_sifive_chip is a bad name as the driver
data structure is named neither pwm_sifive_chip nor pwm_sifive.
> > > > In a previous review round I had doubts about the calculation of frac
> > > > and I think they were addressed wrongly.
> > > > Looking at the figure at the start of chapter 14.9 in the reference
> > > > manual: They use (different than the driver here) pwmcmp0=6 and
> > > > pwmzerocmp=1. Then a period is 7 clock cycles long and pwmcmpX must be 7
> > > > (for X > 0) to yield a 100% signal. So pwmcmpX must be greater than or
> > > > equal to the period (in clock cycles).
> > > >
> > > > Here we're using pwmzerocmp=0, still the constraint
> > > >
> > > > pwmcmpX >= period
> > > >
> > > > must be true for a 100% cycle. (Right?) With pwmzerocmp=0 the period is
> > > > 0x10000 clock cycles, so pwmcmpX must be >= 0x10000 to yield a 100%
> > > > signal. As this is not possible (pwmcmpX is a 16 bit value that can only
> > > > hold up to 0xffff) the output cannot yield 100%.
> > > >
> > > > So I think the most correct implementation here is:
> > > >
> > > > frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 << PWM_SIFIVE_CMPWIDTH) / 2) / period;
> > > > /* Sorry, we cannot do 100% :-( */
> > > > frac = min(frac, (1 << PWM_SIFIVE_CMPWIDTH) - 1);
> > > >
> > >
> > > I will once again go through the specs, if it seems correct I will
> > > implement the above.
> >
> > I would just test this at runtime. Configure for 100% and check the wave
> > form if it is constant.
> >
> > > > > +static int pwm_sifive_probe(struct platform_device *pdev)
> > > > > +{
> > > > > [...]
> > > > > + /* 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;
> > > > > + }
> > > >
> > > > Would it make sense to only enable the clock when the pwm is enabled?
> > > > (If yes, how does the output behave if the clock is disabled while the
> > > > hardware is enabled? I assume the output just freezes at the state where
> > > > it happens to be?)
> > >
> > > Yes. Not sure about the behaviour of hardware when the clock is disabled.
> > > The behaviour should be like you said. If there is no strong reason
> > > then we can keep it as it is right now
> > > or else if you want I can make the necessary change.
> >
> > I would prefer to only have the clock enabled when it is actually used.
> > Note the common pitfall that you cannot disable the clk immediately
> > after configureing the duty cycle to 100% or 0% though because you must
> > only stop the clock when the newly configured parameters are active.
>
> You can also disable the clock if the hardware will still apply the
> configured parameters.
I read the pwm part of the reference manual and I'm convinced the
pitfall I pointed out is relevant here and the hardware will not put the
pin to the inactive level but just freeze on disable if the document is
accurate.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-13 17:39 ` Uwe Kleine-König
0 siblings, 0 replies; 53+ messages in thread
From: Uwe Kleine-König @ 2019-02-13 17:39 UTC (permalink / raw)
To: Thierry Reding
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, Sachin Ghadi, Yash Shah, robh+dt, Paul Walmsley,
linux-riscv
On Wed, Feb 13, 2019 at 01:34:05PM +0100, Thierry Reding wrote:
> On Mon, Feb 11, 2019 at 01:29:54PM +0100, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Mon, Feb 11, 2019 at 04:56:27PM +0530, Yash Shah wrote:
> > > On Thu, Feb 7, 2019 at 3:47 PM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> > > > > +struct pwm_sifive_ddata {
> > > > > + struct pwm_chip chip;
> > > > > + struct notifier_block notifier;
> > > > > + struct clk *clk;
> > > > > + void __iomem *regs;
> > > > > + unsigned int approx_period;
> > > > > + unsigned int real_period;
> > > > > +};
> > > > > +
> > > > > +static inline struct pwm_sifive_ddata *to_pwm_sifive_chip(struct pwm_chip *c)
> > > >
> > > > even though this is inlined I would like this to have use the
> > > > pwm_sifive_ prefix, too.
> > >
> > > Not sure what exactly you want me to do here.
> >
> > I want this function to be named something like pwm_sifive_chip_to_ddata.
> > This way it has the common prefix (and is less confusing because
> > to..chip suggests that is converts something to a chip, but the outcome
> > is ...ddata).
>
> We've got plenty of these cast functions that are named to_*(). I would
> argue that naming this one differently is confusing.
The benefit of consistenly use a common prefix for all functions of a
driver is that applying a filter when tracing is much easier. Sure, you
could argue that this function is inlined and too trivial to care about
in a trace. Recommending to not make any exceptions to the "common
prefix" rule is easier though than to judge all functions that don't use
it if it is worth to adhere to the rule or not.
> Also, I think this may have come from earlier review comments where I
> suggested to name this structure struct pwm_sifive_chip, or just struct
> pwm_sifive.
Even with this argument to_pwm_sifive_chip is a bad name as the driver
data structure is named neither pwm_sifive_chip nor pwm_sifive.
> > > > In a previous review round I had doubts about the calculation of frac
> > > > and I think they were addressed wrongly.
> > > > Looking at the figure at the start of chapter 14.9 in the reference
> > > > manual: They use (different than the driver here) pwmcmp0=6 and
> > > > pwmzerocmp=1. Then a period is 7 clock cycles long and pwmcmpX must be 7
> > > > (for X > 0) to yield a 100% signal. So pwmcmpX must be greater than or
> > > > equal to the period (in clock cycles).
> > > >
> > > > Here we're using pwmzerocmp=0, still the constraint
> > > >
> > > > pwmcmpX >= period
> > > >
> > > > must be true for a 100% cycle. (Right?) With pwmzerocmp=0 the period is
> > > > 0x10000 clock cycles, so pwmcmpX must be >= 0x10000 to yield a 100%
> > > > signal. As this is not possible (pwmcmpX is a 16 bit value that can only
> > > > hold up to 0xffff) the output cannot yield 100%.
> > > >
> > > > So I think the most correct implementation here is:
> > > >
> > > > frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 << PWM_SIFIVE_CMPWIDTH) / 2) / period;
> > > > /* Sorry, we cannot do 100% :-( */
> > > > frac = min(frac, (1 << PWM_SIFIVE_CMPWIDTH) - 1);
> > > >
> > >
> > > I will once again go through the specs, if it seems correct I will
> > > implement the above.
> >
> > I would just test this at runtime. Configure for 100% and check the wave
> > form if it is constant.
> >
> > > > > +static int pwm_sifive_probe(struct platform_device *pdev)
> > > > > +{
> > > > > [...]
> > > > > + /* 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;
> > > > > + }
> > > >
> > > > Would it make sense to only enable the clock when the pwm is enabled?
> > > > (If yes, how does the output behave if the clock is disabled while the
> > > > hardware is enabled? I assume the output just freezes at the state where
> > > > it happens to be?)
> > >
> > > Yes. Not sure about the behaviour of hardware when the clock is disabled.
> > > The behaviour should be like you said. If there is no strong reason
> > > then we can keep it as it is right now
> > > or else if you want I can make the necessary change.
> >
> > I would prefer to only have the clock enabled when it is actually used.
> > Note the common pitfall that you cannot disable the clk immediately
> > after configureing the duty cycle to 100% or 0% though because you must
> > only stop the clock when the newly configured parameters are active.
>
> You can also disable the clock if the hardware will still apply the
> configured parameters.
I read the pwm part of the reference manual and I'm convinced the
pitfall I pointed out is relevant here and the hardware will not put the
pin to the inactive level but just freeze on disable if the document is
accurate.
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
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-13 17:39 ` Uwe Kleine-König
0 siblings, 0 replies; 53+ messages in thread
From: Uwe Kleine-König @ 2019-02-13 17:39 UTC (permalink / raw)
To: Thierry Reding
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, Sachin Ghadi, Yash Shah, robh+dt, Paul Walmsley,
linux-riscv
On Wed, Feb 13, 2019 at 01:34:05PM +0100, Thierry Reding wrote:
> On Mon, Feb 11, 2019 at 01:29:54PM +0100, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Mon, Feb 11, 2019 at 04:56:27PM +0530, Yash Shah wrote:
> > > On Thu, Feb 7, 2019 at 3:47 PM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> > > > > +struct pwm_sifive_ddata {
> > > > > + struct pwm_chip chip;
> > > > > + struct notifier_block notifier;
> > > > > + struct clk *clk;
> > > > > + void __iomem *regs;
> > > > > + unsigned int approx_period;
> > > > > + unsigned int real_period;
> > > > > +};
> > > > > +
> > > > > +static inline struct pwm_sifive_ddata *to_pwm_sifive_chip(struct pwm_chip *c)
> > > >
> > > > even though this is inlined I would like this to have use the
> > > > pwm_sifive_ prefix, too.
> > >
> > > Not sure what exactly you want me to do here.
> >
> > I want this function to be named something like pwm_sifive_chip_to_ddata.
> > This way it has the common prefix (and is less confusing because
> > to..chip suggests that is converts something to a chip, but the outcome
> > is ...ddata).
>
> We've got plenty of these cast functions that are named to_*(). I would
> argue that naming this one differently is confusing.
The benefit of consistenly use a common prefix for all functions of a
driver is that applying a filter when tracing is much easier. Sure, you
could argue that this function is inlined and too trivial to care about
in a trace. Recommending to not make any exceptions to the "common
prefix" rule is easier though than to judge all functions that don't use
it if it is worth to adhere to the rule or not.
> Also, I think this may have come from earlier review comments where I
> suggested to name this structure struct pwm_sifive_chip, or just struct
> pwm_sifive.
Even with this argument to_pwm_sifive_chip is a bad name as the driver
data structure is named neither pwm_sifive_chip nor pwm_sifive.
> > > > In a previous review round I had doubts about the calculation of frac
> > > > and I think they were addressed wrongly.
> > > > Looking at the figure at the start of chapter 14.9 in the reference
> > > > manual: They use (different than the driver here) pwmcmp0=6 and
> > > > pwmzerocmp=1. Then a period is 7 clock cycles long and pwmcmpX must be 7
> > > > (for X > 0) to yield a 100% signal. So pwmcmpX must be greater than or
> > > > equal to the period (in clock cycles).
> > > >
> > > > Here we're using pwmzerocmp=0, still the constraint
> > > >
> > > > pwmcmpX >= period
> > > >
> > > > must be true for a 100% cycle. (Right?) With pwmzerocmp=0 the period is
> > > > 0x10000 clock cycles, so pwmcmpX must be >= 0x10000 to yield a 100%
> > > > signal. As this is not possible (pwmcmpX is a 16 bit value that can only
> > > > hold up to 0xffff) the output cannot yield 100%.
> > > >
> > > > So I think the most correct implementation here is:
> > > >
> > > > frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 << PWM_SIFIVE_CMPWIDTH) / 2) / period;
> > > > /* Sorry, we cannot do 100% :-( */
> > > > frac = min(frac, (1 << PWM_SIFIVE_CMPWIDTH) - 1);
> > > >
> > >
> > > I will once again go through the specs, if it seems correct I will
> > > implement the above.
> >
> > I would just test this at runtime. Configure for 100% and check the wave
> > form if it is constant.
> >
> > > > > +static int pwm_sifive_probe(struct platform_device *pdev)
> > > > > +{
> > > > > [...]
> > > > > + /* 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;
> > > > > + }
> > > >
> > > > Would it make sense to only enable the clock when the pwm is enabled?
> > > > (If yes, how does the output behave if the clock is disabled while the
> > > > hardware is enabled? I assume the output just freezes at the state where
> > > > it happens to be?)
> > >
> > > Yes. Not sure about the behaviour of hardware when the clock is disabled.
> > > The behaviour should be like you said. If there is no strong reason
> > > then we can keep it as it is right now
> > > or else if you want I can make the necessary change.
> >
> > I would prefer to only have the clock enabled when it is actually used.
> > Note the common pitfall that you cannot disable the clk immediately
> > after configureing the duty cycle to 100% or 0% though because you must
> > only stop the clock when the newly configured parameters are active.
>
> You can also disable the clock if the hardware will still apply the
> configured parameters.
I read the pwm part of the reference manual and I'm convinced the
pitfall I pointed out is relevant here and the hardware will not put the
pin to the inactive level but just freeze on disable if the document is
accurate.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-02-07 10:16 ` Uwe Kleine-König
@ 2019-02-13 12:37 ` Thierry Reding
-1 siblings, 0 replies; 53+ messages in thread
From: Thierry Reding @ 2019-02-13 12:37 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Yash Shah, palmer, linux-pwm, linux-riscv, robh+dt, mark.rutland,
devicetree, linux-kernel, sachin.ghadi, paul.walmsley
[-- Attachment #1: Type: text/plain, Size: 825 bytes --]
On Thu, Feb 07, 2019 at 11:16:57AM +0100, Uwe Kleine-König wrote:
> On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
[...]
> > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
[...]
> > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > +
> > + val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
> > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > + pwm_sifive_get_state(chip, dev, state);
>
> Thierry: This changes the pwm_state. Is this how this should be done?
Yes, I think that's fine. The PWM state should always reflect the
current hardware state. If the configuration that we program does not
reflect the state that was requested, that should be reflected in the
PWM state.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-13 12:37 ` Thierry Reding
0 siblings, 0 replies; 53+ messages in thread
From: Thierry Reding @ 2019-02-13 12:37 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel,
sachin.ghadi, Yash Shah, robh+dt, paul.walmsley, linux-riscv
[-- Attachment #1.1: Type: text/plain, Size: 825 bytes --]
On Thu, Feb 07, 2019 at 11:16:57AM +0100, Uwe Kleine-König wrote:
> On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
[...]
> > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
[...]
> > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > +
> > + val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
> > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > +
> > + pwm_sifive_get_state(chip, dev, state);
>
> Thierry: This changes the pwm_state. Is this how this should be done?
Yes, I think that's fine. The PWM state should always reflect the
current hardware state. If the configuration that we program does not
reflect the state that was requested, that should be reflected in the
PWM state.
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
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-02-13 12:37 ` Thierry Reding
(?)
@ 2019-02-14 15:59 ` Uwe Kleine-König
-1 siblings, 0 replies; 53+ messages in thread
From: Uwe Kleine-König @ 2019-02-14 15:59 UTC (permalink / raw)
To: Thierry Reding
Cc: Yash Shah, palmer, linux-pwm, linux-riscv, robh+dt, mark.rutland,
devicetree, linux-kernel, sachin.ghadi, paul.walmsley
On Wed, Feb 13, 2019 at 01:37:03PM +0100, Thierry Reding wrote:
> On Thu, Feb 07, 2019 at 11:16:57AM +0100, Uwe Kleine-König wrote:
> > On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> [...]
> > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> [...]
> > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > +
> > > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > > +
> > > + val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
> > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > +
> > > + pwm_sifive_get_state(chip, dev, state);
> >
> > Thierry: This changes the pwm_state. Is this how this should be done?
>
> Yes, I think that's fine. The PWM state should always reflect the
> current hardware state. If the configuration that we program does not
> reflect the state that was requested, that should be reflected in the
> PWM state.
I'm not sure you blessed what is really done here. If I do:
state.duty_cycle = state.period;
pwm_apply_state(pwm, &state);
the call in question doesn't only result in pwm->state.duty_cycle <
pwm->state.period, but it also corrects my local state variable (i.e. I
have state.duty_cycle < state.period afterwards).
Is this what you thought to be fine?
Also note that v6 dropped that call because of my doubts.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-14 15:59 ` Uwe Kleine-König
0 siblings, 0 replies; 53+ messages in thread
From: Uwe Kleine-König @ 2019-02-14 15:59 UTC (permalink / raw)
To: Thierry Reding
Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel,
sachin.ghadi, Yash Shah, robh+dt, paul.walmsley, linux-riscv
On Wed, Feb 13, 2019 at 01:37:03PM +0100, Thierry Reding wrote:
> On Thu, Feb 07, 2019 at 11:16:57AM +0100, Uwe Kleine-König wrote:
> > On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> [...]
> > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> [...]
> > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > +
> > > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > > +
> > > + val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
> > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > +
> > > + pwm_sifive_get_state(chip, dev, state);
> >
> > Thierry: This changes the pwm_state. Is this how this should be done?
>
> Yes, I think that's fine. The PWM state should always reflect the
> current hardware state. If the configuration that we program does not
> reflect the state that was requested, that should be reflected in the
> PWM state.
I'm not sure you blessed what is really done here. If I do:
state.duty_cycle = state.period;
pwm_apply_state(pwm, &state);
the call in question doesn't only result in pwm->state.duty_cycle <
pwm->state.period, but it also corrects my local state variable (i.e. I
have state.duty_cycle < state.period afterwards).
Is this what you thought to be fine?
Also note that v6 dropped that call because of my doubts.
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
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-02-14 15:59 ` Uwe Kleine-König
0 siblings, 0 replies; 53+ messages in thread
From: Uwe Kleine-König @ 2019-02-14 15:59 UTC (permalink / raw)
To: Thierry Reding
Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel,
sachin.ghadi, Yash Shah, robh+dt, paul.walmsley, linux-riscv
On Wed, Feb 13, 2019 at 01:37:03PM +0100, Thierry Reding wrote:
> On Thu, Feb 07, 2019 at 11:16:57AM +0100, Uwe Kleine-König wrote:
> > On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> [...]
> > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> [...]
> > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > +
> > > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > > +
> > > + val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
> > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > +
> > > + pwm_sifive_get_state(chip, dev, state);
> >
> > Thierry: This changes the pwm_state. Is this how this should be done?
>
> Yes, I think that's fine. The PWM state should always reflect the
> current hardware state. If the configuration that we program does not
> reflect the state that was requested, that should be reflected in the
> PWM state.
I'm not sure you blessed what is really done here. If I do:
state.duty_cycle = state.period;
pwm_apply_state(pwm, &state);
the call in question doesn't only result in pwm->state.duty_cycle <
pwm->state.period, but it also corrects my local state variable (i.e. I
have state.duty_cycle < state.period afterwards).
Is this what you thought to be fine?
Also note that v6 dropped that call because of my doubts.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-02-14 15:59 ` Uwe Kleine-König
(?)
@ 2019-03-12 7:27 ` Uwe Kleine-König
-1 siblings, 0 replies; 53+ messages in thread
From: Uwe Kleine-König @ 2019-03-12 7:27 UTC (permalink / raw)
To: Thierry Reding
Cc: Yash Shah, palmer, linux-pwm, linux-riscv, robh+dt, mark.rutland,
devicetree, linux-kernel, sachin.ghadi, paul.walmsley
Hello,
On Thu, Feb 14, 2019 at 04:59:03PM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 13, 2019 at 01:37:03PM +0100, Thierry Reding wrote:
> > On Thu, Feb 07, 2019 at 11:16:57AM +0100, Uwe Kleine-König wrote:
> > > On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> > [...]
> > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > [...]
> > > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > > +
> > > > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > > > +
> > > > + val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
> > > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > > +
> > > > + pwm_sifive_get_state(chip, dev, state);
> > >
> > > Thierry: This changes the pwm_state. Is this how this should be done?
> >
> > Yes, I think that's fine. The PWM state should always reflect the
> > current hardware state. If the configuration that we program does not
> > reflect the state that was requested, that should be reflected in the
> > PWM state.
>
> I'm not sure you blessed what is really done here. If I do:
>
> state.duty_cycle = state.period;
> pwm_apply_state(pwm, &state);
>
> the call in question doesn't only result in pwm->state.duty_cycle <
> pwm->state.period, but it also corrects my local state variable (i.e. I
> have state.duty_cycle < state.period afterwards).
>
> Is this what you thought to be fine?
I thought a bit about this and I'm convinced that updating struct
pwm_device::state is/might be fine, but changing the caller's struct pwm_state
that was passed to pwm_apply_state is not.
Consider a consumer who does:
#define PERIOD 5000000
#define DUTY_LITTLE 10
...
struct pwm_state state = {
.period = PERIOD,
.duty_cycle = DUTY_LITTLE,
.polarity = PWM_POLARITY_NORMAL,
.enabled = true,
};
pwm_apply_state(mypwm, &state);
...
state.duty_cycle = PERIOD / 2;
pwm_apply_state(mypwm, &state);
I think the second request should have state.period = 5000000 and not
some other value (that might only have chosen by the respective driver
because the first duty cycle was so short).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-03-12 7:27 ` Uwe Kleine-König
0 siblings, 0 replies; 53+ messages in thread
From: Uwe Kleine-König @ 2019-03-12 7:27 UTC (permalink / raw)
To: Thierry Reding
Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel,
sachin.ghadi, Yash Shah, robh+dt, paul.walmsley, linux-riscv
Hello,
On Thu, Feb 14, 2019 at 04:59:03PM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 13, 2019 at 01:37:03PM +0100, Thierry Reding wrote:
> > On Thu, Feb 07, 2019 at 11:16:57AM +0100, Uwe Kleine-König wrote:
> > > On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> > [...]
> > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > [...]
> > > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > > +
> > > > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > > > +
> > > > + val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
> > > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > > +
> > > > + pwm_sifive_get_state(chip, dev, state);
> > >
> > > Thierry: This changes the pwm_state. Is this how this should be done?
> >
> > Yes, I think that's fine. The PWM state should always reflect the
> > current hardware state. If the configuration that we program does not
> > reflect the state that was requested, that should be reflected in the
> > PWM state.
>
> I'm not sure you blessed what is really done here. If I do:
>
> state.duty_cycle = state.period;
> pwm_apply_state(pwm, &state);
>
> the call in question doesn't only result in pwm->state.duty_cycle <
> pwm->state.period, but it also corrects my local state variable (i.e. I
> have state.duty_cycle < state.period afterwards).
>
> Is this what you thought to be fine?
I thought a bit about this and I'm convinced that updating struct
pwm_device::state is/might be fine, but changing the caller's struct pwm_state
that was passed to pwm_apply_state is not.
Consider a consumer who does:
#define PERIOD 5000000
#define DUTY_LITTLE 10
...
struct pwm_state state = {
.period = PERIOD,
.duty_cycle = DUTY_LITTLE,
.polarity = PWM_POLARITY_NORMAL,
.enabled = true,
};
pwm_apply_state(mypwm, &state);
...
state.duty_cycle = PERIOD / 2;
pwm_apply_state(mypwm, &state);
I think the second request should have state.period = 5000000 and not
some other value (that might only have chosen by the respective driver
because the first duty cycle was so short).
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
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH v5 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-03-12 7:27 ` Uwe Kleine-König
0 siblings, 0 replies; 53+ messages in thread
From: Uwe Kleine-König @ 2019-03-12 7:27 UTC (permalink / raw)
To: Thierry Reding
Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel,
sachin.ghadi, Yash Shah, robh+dt, paul.walmsley, linux-riscv
Hello,
On Thu, Feb 14, 2019 at 04:59:03PM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 13, 2019 at 01:37:03PM +0100, Thierry Reding wrote:
> > On Thu, Feb 07, 2019 at 11:16:57AM +0100, Uwe Kleine-König wrote:
> > > On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote:
> > [...]
> > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > [...]
> > > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > > +
> > > > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > > > +
> > > > + val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH);
> > > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > > +
> > > > + pwm_sifive_get_state(chip, dev, state);
> > >
> > > Thierry: This changes the pwm_state. Is this how this should be done?
> >
> > Yes, I think that's fine. The PWM state should always reflect the
> > current hardware state. If the configuration that we program does not
> > reflect the state that was requested, that should be reflected in the
> > PWM state.
>
> I'm not sure you blessed what is really done here. If I do:
>
> state.duty_cycle = state.period;
> pwm_apply_state(pwm, &state);
>
> the call in question doesn't only result in pwm->state.duty_cycle <
> pwm->state.period, but it also corrects my local state variable (i.e. I
> have state.duty_cycle < state.period afterwards).
>
> Is this what you thought to be fine?
I thought a bit about this and I'm convinced that updating struct
pwm_device::state is/might be fine, but changing the caller's struct pwm_state
that was passed to pwm_apply_state is not.
Consider a consumer who does:
#define PERIOD 5000000
#define DUTY_LITTLE 10
...
struct pwm_state state = {
.period = PERIOD,
.duty_cycle = DUTY_LITTLE,
.polarity = PWM_POLARITY_NORMAL,
.enabled = true,
};
pwm_apply_state(mypwm, &state);
...
state.duty_cycle = PERIOD / 2;
pwm_apply_state(mypwm, &state);
I think the second request should have state.period = 5000000 and not
some other value (that might only have chosen by the respective driver
because the first duty cycle was so short).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 53+ messages in thread