* [PATCH 0/2] PWM support for HiFive Unleashed
@ 2019-01-11 8:22 ` Yash Shah
0 siblings, 0 replies; 44+ messages in thread
From: Yash Shah @ 2019-01-11 8:22 UTC (permalink / raw)
To: palmer, linux-pwm, linux-riscv
Cc: thierry.reding, robh+dt, mark.rutland, devicetree, linux-kernel,
sachin.ghadi, paul.walmsley, Yash Shah
This patch series adds a PWM driver and DT documentation
for HiFive Unleashed board. The patches are mostly based on
Wesley's patch.
Yash Shah (2):
pwm: sifive: Add DT documentation for SiFive PWM Controller
pwm: sifive: Add a driver for SiFive SoC PWM
.../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++
drivers/pwm/Kconfig | 10 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++
4 files changed, 294 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
create mode 100644 drivers/pwm/pwm-sifive.c
--
1.9.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 0/2] PWM support for HiFive Unleashed
@ 2019-01-11 8:22 ` Yash Shah
0 siblings, 0 replies; 44+ messages in thread
From: Yash Shah @ 2019-01-11 8:22 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
This patch series adds a PWM driver and DT documentation
for HiFive Unleashed board. The patches are mostly based on
Wesley's patch.
Yash Shah (2):
pwm: sifive: Add DT documentation for SiFive PWM Controller
pwm: sifive: Add a driver for SiFive SoC PWM
.../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++
drivers/pwm/Kconfig | 10 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++
4 files changed, 294 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
create mode 100644 drivers/pwm/pwm-sifive.c
--
1.9.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller
2019-01-11 8:22 ` Yash Shah
@ 2019-01-11 8:22 ` Yash Shah
-1 siblings, 0 replies; 44+ messages in thread
From: Yash Shah @ 2019-01-11 8:22 UTC (permalink / raw)
To: palmer, linux-pwm, linux-riscv
Cc: thierry.reding, robh+dt, mark.rutland, devicetree, linux-kernel,
sachin.ghadi, paul.walmsley, Yash Shah
DT documentation for PWM controller added with updated compatible
string.
Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
[Atish: Compatible string update]
Signed-off-by: Atish Patra <atish.patra@wdc.com>
Signed-off-by: Yash Shah <yash.shah@sifive.com>
---
.../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++++++++++++++++++++
1 file changed, 37 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
new file mode 100644
index 0000000..e0fc22a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
@@ -0,0 +1,37 @@
+SiFive PWM controller
+
+Unlike most other PWM controllers, the SiFive PWM controller currently only
+supports one period for all channels in the PWM. This is set globally in DTS.
+The period also has significant restrictions on the values it can achieve,
+which the driver rounds to the nearest achievable frequency.
+
+Required properties:
+- compatible: Please refer to sifive-blocks-ip-versioning.txt
+- reg: physical base address and length of the controller's registers
+- clocks: Should contain a clock identifier for the PWM's parent clock.
+- #pwm-cells: Should be 2.
+ The first cell is the PWM channel number
+ The second cell is the PWM polarity
+- sifive,approx-period-ns: the driver will get as close to this period as it can
+- interrupts: one interrupt per PWM channel
+
+PWM RTL that corresponds to the IP block version numbers can be found
+here:
+
+https://github.com/sifive/sifive-blocks/tree/master/src/main/scala/devices/pwm
+
+Further information on the format of the IP
+block-specific version numbers can be found in
+Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt
+
+Examples:
+
+pwm: pwm@10020000 {
+ compatible = "sifive,fu540-c000-pwm","sifive,pwm0";
+ reg = <0x0 0x10020000 0x0 0x1000>;
+ clocks = <&tlclk>;
+ interrupt-parent = <&plic>;
+ interrupts = <42 43 44 45>;
+ #pwm-cells = <2>;
+ sifive,approx-period-ns = <1000000>;
+};
--
1.9.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller
@ 2019-01-11 8:22 ` Yash Shah
0 siblings, 0 replies; 44+ messages in thread
From: Yash Shah @ 2019-01-11 8:22 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
DT documentation for PWM controller added with updated compatible
string.
Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
[Atish: Compatible string update]
Signed-off-by: Atish Patra <atish.patra@wdc.com>
Signed-off-by: Yash Shah <yash.shah@sifive.com>
---
.../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++++++++++++++++++++
1 file changed, 37 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
new file mode 100644
index 0000000..e0fc22a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
@@ -0,0 +1,37 @@
+SiFive PWM controller
+
+Unlike most other PWM controllers, the SiFive PWM controller currently only
+supports one period for all channels in the PWM. This is set globally in DTS.
+The period also has significant restrictions on the values it can achieve,
+which the driver rounds to the nearest achievable frequency.
+
+Required properties:
+- compatible: Please refer to sifive-blocks-ip-versioning.txt
+- reg: physical base address and length of the controller's registers
+- clocks: Should contain a clock identifier for the PWM's parent clock.
+- #pwm-cells: Should be 2.
+ The first cell is the PWM channel number
+ The second cell is the PWM polarity
+- sifive,approx-period-ns: the driver will get as close to this period as it can
+- interrupts: one interrupt per PWM channel
+
+PWM RTL that corresponds to the IP block version numbers can be found
+here:
+
+https://github.com/sifive/sifive-blocks/tree/master/src/main/scala/devices/pwm
+
+Further information on the format of the IP
+block-specific version numbers can be found in
+Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt
+
+Examples:
+
+pwm: pwm@10020000 {
+ compatible = "sifive,fu540-c000-pwm","sifive,pwm0";
+ reg = <0x0 0x10020000 0x0 0x1000>;
+ clocks = <&tlclk>;
+ interrupt-parent = <&plic>;
+ interrupts = <42 43 44 45>;
+ #pwm-cells = <2>;
+ sifive,approx-period-ns = <1000000>;
+};
--
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] 44+ messages in thread
* [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-11 8:22 ` Yash Shah
@ 2019-01-11 8:22 ` Yash Shah
-1 siblings, 0 replies; 44+ messages in thread
From: Yash Shah @ 2019-01-11 8:22 UTC (permalink / raw)
To: palmer, linux-pwm, linux-riscv
Cc: thierry.reding, robh+dt, mark.rutland, devicetree, linux-kernel,
sachin.ghadi, paul.walmsley, Yash Shah
Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
[Atish: Various fixes and code cleanup]
Signed-off-by: Atish Patra <atish.patra@wdc.com>
Signed-off-by: Yash Shah <yash.shah@sifive.com>
---
drivers/pwm/Kconfig | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 257 insertions(+)
create mode 100644 drivers/pwm/pwm-sifive.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index a8f47df..3bcaf6a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -380,6 +380,16 @@ config PWM_SAMSUNG
To compile this driver as a module, choose M here: the module
will be called pwm-samsung.
+config PWM_SIFIVE
+ tristate "SiFive PWM support"
+ depends on OF
+ depends on COMMON_CLK
+ help
+ Generic PWM framework driver for SiFive SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-sifive.
+
config PWM_SPEAR
tristate "STMicroelectronics SPEAr PWM support"
depends on PLAT_SPEAR
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9c676a0..30089ca 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
+obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
obj-$(CONFIG_PWM_STI) += pwm-sti.o
obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
new file mode 100644
index 0000000..7fee809
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,246 @@
+// 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 REG_PWMCFG 0x0
+#define REG_PWMCOUNT 0x8
+#define REG_PWMS 0x10
+#define REG_PWMCMP0 0x20
+
+/* PWMCFG fields */
+#define BIT_PWM_SCALE 0
+#define BIT_PWM_STICKY 8
+#define BIT_PWM_ZERO_ZMP 9
+#define BIT_PWM_DEGLITCH 10
+#define BIT_PWM_EN_ALWAYS 12
+#define BIT_PWM_EN_ONCE 13
+#define BIT_PWM0_CENTER 16
+#define BIT_PWM0_GANG 24
+#define BIT_PWM0_IP 28
+
+#define SIZE_PWMCMP 4
+#define MASK_PWM_SCALE 0xf
+
+struct sifive_pwm_device {
+ struct pwm_chip chip;
+ struct notifier_block notifier;
+ struct clk *clk;
+ void __iomem *regs;
+ unsigned int approx_period;
+ unsigned int real_period;
+};
+
+static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c)
+{
+ return container_of(c, struct sifive_pwm_device, chip);
+}
+
+static void sifive_pwm_get_state(struct pwm_chip *chip, struct pwm_device *dev,
+ struct pwm_state *state)
+{
+ struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+ u32 duty;
+
+ duty = readl(pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
+
+ state->period = pwm->real_period;
+ state->duty_cycle = ((u64)duty * pwm->real_period) >> 16;
+ state->polarity = PWM_POLARITY_INVERSED;
+ state->enabled = duty > 0;
+}
+
+static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
+ struct pwm_state *state)
+{
+ struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+ unsigned int duty_cycle;
+ u32 frac;
+
+ if (state->polarity != PWM_POLARITY_INVERSED)
+ return -EINVAL;
+
+ duty_cycle = state->duty_cycle;
+ if (!state->enabled)
+ duty_cycle = 0;
+
+ frac = div_u64((u64)duty_cycle << 16, state->period);
+ frac = min(frac, 0xFFFFU);
+
+ writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
+
+ if (state->enabled)
+ sifive_pwm_get_state(chip, dev, state);
+
+ return 0;
+}
+
+static const struct pwm_ops sifive_pwm_ops = {
+ .get_state = sifive_pwm_get_state,
+ .apply = sifive_pwm_apply,
+ .owner = THIS_MODULE,
+};
+
+static struct pwm_device *sifive_pwm_xlate(struct pwm_chip *chip,
+ const struct of_phandle_args *args)
+{
+ struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+ struct pwm_device *dev;
+
+ if (args->args[0] >= chip->npwm)
+ return ERR_PTR(-EINVAL);
+
+ dev = pwm_request_from_chip(chip, args->args[0], NULL);
+ if (IS_ERR(dev))
+ return dev;
+
+ /* The period cannot be changed on a per-PWM basis */
+ dev->args.period = pwm->real_period;
+ dev->args.polarity = PWM_POLARITY_NORMAL;
+ if (args->args[1] & PWM_POLARITY_INVERSED)
+ dev->args.polarity = PWM_POLARITY_INVERSED;
+
+ return dev;
+}
+
+static void sifive_pwm_update_clock(struct sifive_pwm_device *pwm,
+ unsigned long rate)
+{
+ /* (1 << (16+scale)) * 10^9/rate = real_period */
+ unsigned long scale_pow = (pwm->approx_period * (u64)rate) / 1000000000;
+ int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
+
+ writel((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE),
+ pwm->regs + REG_PWMCFG);
+
+ /* 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 sifive_pwm_clock_notifier(struct notifier_block *nb,
+ unsigned long event, void *data)
+{
+ struct clk_notifier_data *ndata = data;
+ struct sifive_pwm_device *pwm =
+ container_of(nb, struct sifive_pwm_device, notifier);
+
+ if (event == POST_RATE_CHANGE)
+ sifive_pwm_update_clock(pwm, ndata->new_rate);
+
+ return NOTIFY_OK;
+}
+
+static int sifive_pwm_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *node = pdev->dev.of_node;
+ struct sifive_pwm_device *pwm;
+ struct pwm_chip *chip;
+ struct resource *res;
+ int ret;
+
+ pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+ if (!pwm)
+ return -ENOMEM;
+
+ chip = &pwm->chip;
+ chip->dev = dev;
+ chip->ops = &sifive_pwm_ops;
+ chip->of_xlate = sifive_pwm_xlate;
+ chip->of_pwm_n_cells = 2;
+ chip->base = -1;
+ chip->npwm = 4;
+
+ ret = of_property_read_u32(node, "sifive,approx-period-ns",
+ &pwm->approx_period);
+ if (ret < 0) {
+ dev_err(dev, "Unable to read sifive,approx-period from DTS\n");
+ return ret;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ pwm->regs = devm_ioremap_resource(dev, res);
+ if (IS_ERR(pwm->regs)) {
+ dev_err(dev, "Unable to map IO resources\n");
+ return PTR_ERR(pwm->regs);
+ }
+
+ pwm->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(pwm->clk)) {
+ 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 = sifive_pwm_clock_notifier;
+ ret = clk_notifier_register(pwm->clk, &pwm->notifier);
+ if (ret) {
+ dev_err(dev, "failed to register clock notifier: %d\n", ret);
+ clk_disable_unprepare(pwm->clk);
+ return ret;
+ }
+
+ /* Initialize PWM config */
+ sifive_pwm_update_clock(pwm, clk_get_rate(pwm->clk));
+
+ ret = pwmchip_add(chip);
+ if (ret < 0) {
+ dev_err(dev, "cannot register PWM: %d\n", ret);
+ clk_notifier_unregister(pwm->clk, &pwm->notifier);
+ clk_disable_unprepare(pwm->clk);
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, pwm);
+ dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
+
+ return 0;
+}
+
+static int sifive_pwm_remove(struct platform_device *dev)
+{
+ struct sifive_pwm_device *pwm = platform_get_drvdata(dev);
+ 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 sifive_pwm_of_match[] = {
+ { .compatible = "sifive,pwm0" },
+ { .compatible = "sifive,fu540-c000-pwm" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, sifive_pwm_of_match);
+
+static struct platform_driver sifive_pwm_driver = {
+ .probe = sifive_pwm_probe,
+ .remove = sifive_pwm_remove,
+ .driver = {
+ .name = "pwm-sifive",
+ .of_match_table = sifive_pwm_of_match,
+ },
+};
+module_platform_driver(sifive_pwm_driver);
+
+MODULE_DESCRIPTION("SiFive PWM driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-11 8:22 ` Yash Shah
0 siblings, 0 replies; 44+ messages in thread
From: Yash Shah @ 2019-01-11 8:22 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 | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 257 insertions(+)
create mode 100644 drivers/pwm/pwm-sifive.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index a8f47df..3bcaf6a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -380,6 +380,16 @@ config PWM_SAMSUNG
To compile this driver as a module, choose M here: the module
will be called pwm-samsung.
+config PWM_SIFIVE
+ tristate "SiFive PWM support"
+ depends on OF
+ depends on COMMON_CLK
+ help
+ Generic PWM framework driver for SiFive SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-sifive.
+
config PWM_SPEAR
tristate "STMicroelectronics SPEAr PWM support"
depends on PLAT_SPEAR
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9c676a0..30089ca 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
+obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
obj-$(CONFIG_PWM_STI) += pwm-sti.o
obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
new file mode 100644
index 0000000..7fee809
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,246 @@
+// 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 REG_PWMCFG 0x0
+#define REG_PWMCOUNT 0x8
+#define REG_PWMS 0x10
+#define REG_PWMCMP0 0x20
+
+/* PWMCFG fields */
+#define BIT_PWM_SCALE 0
+#define BIT_PWM_STICKY 8
+#define BIT_PWM_ZERO_ZMP 9
+#define BIT_PWM_DEGLITCH 10
+#define BIT_PWM_EN_ALWAYS 12
+#define BIT_PWM_EN_ONCE 13
+#define BIT_PWM0_CENTER 16
+#define BIT_PWM0_GANG 24
+#define BIT_PWM0_IP 28
+
+#define SIZE_PWMCMP 4
+#define MASK_PWM_SCALE 0xf
+
+struct sifive_pwm_device {
+ struct pwm_chip chip;
+ struct notifier_block notifier;
+ struct clk *clk;
+ void __iomem *regs;
+ unsigned int approx_period;
+ unsigned int real_period;
+};
+
+static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c)
+{
+ return container_of(c, struct sifive_pwm_device, chip);
+}
+
+static void sifive_pwm_get_state(struct pwm_chip *chip, struct pwm_device *dev,
+ struct pwm_state *state)
+{
+ struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+ u32 duty;
+
+ duty = readl(pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
+
+ state->period = pwm->real_period;
+ state->duty_cycle = ((u64)duty * pwm->real_period) >> 16;
+ state->polarity = PWM_POLARITY_INVERSED;
+ state->enabled = duty > 0;
+}
+
+static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
+ struct pwm_state *state)
+{
+ struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+ unsigned int duty_cycle;
+ u32 frac;
+
+ if (state->polarity != PWM_POLARITY_INVERSED)
+ return -EINVAL;
+
+ duty_cycle = state->duty_cycle;
+ if (!state->enabled)
+ duty_cycle = 0;
+
+ frac = div_u64((u64)duty_cycle << 16, state->period);
+ frac = min(frac, 0xFFFFU);
+
+ writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
+
+ if (state->enabled)
+ sifive_pwm_get_state(chip, dev, state);
+
+ return 0;
+}
+
+static const struct pwm_ops sifive_pwm_ops = {
+ .get_state = sifive_pwm_get_state,
+ .apply = sifive_pwm_apply,
+ .owner = THIS_MODULE,
+};
+
+static struct pwm_device *sifive_pwm_xlate(struct pwm_chip *chip,
+ const struct of_phandle_args *args)
+{
+ struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+ struct pwm_device *dev;
+
+ if (args->args[0] >= chip->npwm)
+ return ERR_PTR(-EINVAL);
+
+ dev = pwm_request_from_chip(chip, args->args[0], NULL);
+ if (IS_ERR(dev))
+ return dev;
+
+ /* The period cannot be changed on a per-PWM basis */
+ dev->args.period = pwm->real_period;
+ dev->args.polarity = PWM_POLARITY_NORMAL;
+ if (args->args[1] & PWM_POLARITY_INVERSED)
+ dev->args.polarity = PWM_POLARITY_INVERSED;
+
+ return dev;
+}
+
+static void sifive_pwm_update_clock(struct sifive_pwm_device *pwm,
+ unsigned long rate)
+{
+ /* (1 << (16+scale)) * 10^9/rate = real_period */
+ unsigned long scale_pow = (pwm->approx_period * (u64)rate) / 1000000000;
+ int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
+
+ writel((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE),
+ pwm->regs + REG_PWMCFG);
+
+ /* 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 sifive_pwm_clock_notifier(struct notifier_block *nb,
+ unsigned long event, void *data)
+{
+ struct clk_notifier_data *ndata = data;
+ struct sifive_pwm_device *pwm =
+ container_of(nb, struct sifive_pwm_device, notifier);
+
+ if (event == POST_RATE_CHANGE)
+ sifive_pwm_update_clock(pwm, ndata->new_rate);
+
+ return NOTIFY_OK;
+}
+
+static int sifive_pwm_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *node = pdev->dev.of_node;
+ struct sifive_pwm_device *pwm;
+ struct pwm_chip *chip;
+ struct resource *res;
+ int ret;
+
+ pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+ if (!pwm)
+ return -ENOMEM;
+
+ chip = &pwm->chip;
+ chip->dev = dev;
+ chip->ops = &sifive_pwm_ops;
+ chip->of_xlate = sifive_pwm_xlate;
+ chip->of_pwm_n_cells = 2;
+ chip->base = -1;
+ chip->npwm = 4;
+
+ ret = of_property_read_u32(node, "sifive,approx-period-ns",
+ &pwm->approx_period);
+ if (ret < 0) {
+ dev_err(dev, "Unable to read sifive,approx-period from DTS\n");
+ return ret;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ pwm->regs = devm_ioremap_resource(dev, res);
+ if (IS_ERR(pwm->regs)) {
+ dev_err(dev, "Unable to map IO resources\n");
+ return PTR_ERR(pwm->regs);
+ }
+
+ pwm->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(pwm->clk)) {
+ 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 = sifive_pwm_clock_notifier;
+ ret = clk_notifier_register(pwm->clk, &pwm->notifier);
+ if (ret) {
+ dev_err(dev, "failed to register clock notifier: %d\n", ret);
+ clk_disable_unprepare(pwm->clk);
+ return ret;
+ }
+
+ /* Initialize PWM config */
+ sifive_pwm_update_clock(pwm, clk_get_rate(pwm->clk));
+
+ ret = pwmchip_add(chip);
+ if (ret < 0) {
+ dev_err(dev, "cannot register PWM: %d\n", ret);
+ clk_notifier_unregister(pwm->clk, &pwm->notifier);
+ clk_disable_unprepare(pwm->clk);
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, pwm);
+ dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
+
+ return 0;
+}
+
+static int sifive_pwm_remove(struct platform_device *dev)
+{
+ struct sifive_pwm_device *pwm = platform_get_drvdata(dev);
+ 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 sifive_pwm_of_match[] = {
+ { .compatible = "sifive,pwm0" },
+ { .compatible = "sifive,fu540-c000-pwm" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, sifive_pwm_of_match);
+
+static struct platform_driver sifive_pwm_driver = {
+ .probe = sifive_pwm_probe,
+ .remove = sifive_pwm_remove,
+ .driver = {
+ .name = "pwm-sifive",
+ .of_match_table = sifive_pwm_of_match,
+ },
+};
+module_platform_driver(sifive_pwm_driver);
+
+MODULE_DESCRIPTION("SiFive PWM driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-11 8:22 ` Yash Shah
(?)
@ 2019-01-15 15:27 ` Christoph Hellwig
-1 siblings, 0 replies; 44+ messages in thread
From: Christoph Hellwig @ 2019-01-15 15:27 UTC (permalink / raw)
To: Yash Shah
Cc: palmer, linux-pwm, linux-riscv, mark.rutland, devicetree,
linux-kernel, robh+dt, sachin.ghadi, thierry.reding,
paul.walmsley
From a general code quality point of view this looks fine to me.
I don't really know anything about the PWM subsystem, though:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-15 15:27 ` Christoph Hellwig
0 siblings, 0 replies; 44+ messages in thread
From: Christoph Hellwig @ 2019-01-15 15:27 UTC (permalink / raw)
To: Yash Shah
Cc: palmer, linux-pwm, linux-riscv, mark.rutland, devicetree,
linux-kernel, robh+dt, sachin.ghadi, thierry.reding,
paul.walmsley
>From a general code quality point of view this looks fine to me.
I don't really know anything about the PWM subsystem, though:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-15 15:27 ` Christoph Hellwig
0 siblings, 0 replies; 44+ messages in thread
From: Christoph Hellwig @ 2019-01-15 15:27 UTC (permalink / raw)
To: Yash Shah
Cc: mark.rutland, linux-pwm, devicetree, palmer, linux-kernel,
sachin.ghadi, robh+dt, thierry.reding, paul.walmsley,
linux-riscv
From a general code quality point of view this looks fine to me.
I don't really know anything about the PWM subsystem, though:
Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller
2019-01-11 8:22 ` Yash Shah
@ 2019-01-15 20:11 ` Uwe Kleine-König
-1 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-15 20:11 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,
this is v3, right? It is helpful to point this out to ease reviewing.
On Fri, Jan 11, 2019 at 01:52:43PM +0530, Yash Shah wrote:
> DT documentation for PWM controller added with updated compatible
> string.
Not sure what was updated here. But assuming this is compared to v2 this
is not a helpful info in the commit log.
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> [Atish: Compatible string update]
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
> .../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> new file mode 100644
> index 0000000..e0fc22a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> @@ -0,0 +1,37 @@
> +SiFive PWM controller
> +
> +Unlike most other PWM controllers, the SiFive PWM controller currently only
> +supports one period for all channels in the PWM. This is set globally in DTS.
> +The period also has significant restrictions on the values it can achieve,
> +which the driver rounds to the nearest achievable frequency.
> +
> +Required properties:
> +- compatible: Please refer to sifive-blocks-ip-versioning.txt
While the description was too verbose in v2, this is too short. You
should at least mention something like "sifive,pwmX" and
"sifive,$cpuname-pwm" (or how ever that scheme works).
> +- reg: physical base address and length of the controller's registers
> +- clocks: Should contain a clock identifier for the PWM's parent clock.
> +- #pwm-cells: Should be 2.
> + The first cell is the PWM channel number
> + The second cell is the PWM polarity
I'd drop these two lines and refer to bindings/pwm/pwm.txt instead.
> +- sifive,approx-period-ns: the driver will get as close to this period as it can
As already said for v2: I'd drop "approx-".
> +- interrupts: one interrupt per PWM channel
> +
> +PWM RTL that corresponds to the IP block version numbers can be found
> +here:
> +
> +https://github.com/sifive/sifive-blocks/tree/master/src/main/scala/devices/pwm
> +
> +Further information on the format of the IP
> +block-specific version numbers can be found in
> +Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt
> +
> +Examples:
> +
> +pwm: pwm@10020000 {
> + compatible = "sifive,fu540-c000-pwm","sifive,pwm0";
> + reg = <0x0 0x10020000 0x0 0x1000>;
> + clocks = <&tlclk>;
> + interrupt-parent = <&plic>;
> + interrupts = <42 43 44 45>;
> + #pwm-cells = <2>;
> + sifive,approx-period-ns = <1000000>;
> +};
> --
> 1.9.1
>
>
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller
@ 2019-01-15 20:11 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-15 20:11 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,
this is v3, right? It is helpful to point this out to ease reviewing.
On Fri, Jan 11, 2019 at 01:52:43PM +0530, Yash Shah wrote:
> DT documentation for PWM controller added with updated compatible
> string.
Not sure what was updated here. But assuming this is compared to v2 this
is not a helpful info in the commit log.
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> [Atish: Compatible string update]
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
> .../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> new file mode 100644
> index 0000000..e0fc22a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> @@ -0,0 +1,37 @@
> +SiFive PWM controller
> +
> +Unlike most other PWM controllers, the SiFive PWM controller currently only
> +supports one period for all channels in the PWM. This is set globally in DTS.
> +The period also has significant restrictions on the values it can achieve,
> +which the driver rounds to the nearest achievable frequency.
> +
> +Required properties:
> +- compatible: Please refer to sifive-blocks-ip-versioning.txt
While the description was too verbose in v2, this is too short. You
should at least mention something like "sifive,pwmX" and
"sifive,$cpuname-pwm" (or how ever that scheme works).
> +- reg: physical base address and length of the controller's registers
> +- clocks: Should contain a clock identifier for the PWM's parent clock.
> +- #pwm-cells: Should be 2.
> + The first cell is the PWM channel number
> + The second cell is the PWM polarity
I'd drop these two lines and refer to bindings/pwm/pwm.txt instead.
> +- sifive,approx-period-ns: the driver will get as close to this period as it can
As already said for v2: I'd drop "approx-".
> +- interrupts: one interrupt per PWM channel
> +
> +PWM RTL that corresponds to the IP block version numbers can be found
> +here:
> +
> +https://github.com/sifive/sifive-blocks/tree/master/src/main/scala/devices/pwm
> +
> +Further information on the format of the IP
> +block-specific version numbers can be found in
> +Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt
> +
> +Examples:
> +
> +pwm: pwm@10020000 {
> + compatible = "sifive,fu540-c000-pwm","sifive,pwm0";
> + reg = <0x0 0x10020000 0x0 0x1000>;
> + clocks = <&tlclk>;
> + interrupt-parent = <&plic>;
> + interrupts = <42 43 44 45>;
> + #pwm-cells = <2>;
> + sifive,approx-period-ns = <1000000>;
> +};
> --
> 1.9.1
>
>
--
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] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-11 8:22 ` Yash Shah
@ 2019-01-15 22:00 ` Uwe Kleine-König
-1 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-15 22:00 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 Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
>
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 257 insertions(+)
> create mode 100644 drivers/pwm/pwm-sifive.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a8f47df..3bcaf6a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> To compile this driver as a module, choose M here: the module
> will be called pwm-samsung.
>
> +config PWM_SIFIVE
> + tristate "SiFive PWM support"
> + depends on OF
> + depends on COMMON_CLK
I'd say add:
depends on MACH_SIFIVE || COMPILE_TEST
(I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> + 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..7fee809
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,246 @@
> +// 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
I wonder if such an instance should be only a single PWM instead of
four. Then you were more flexible with the period lengths (using
pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
I didn't understand how the deglitch logic works yet. Currently it is
not used which might result in four edges in a single period (which is
bad).
> + */
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +/* Register offsets */
> +#define REG_PWMCFG 0x0
> +#define REG_PWMCOUNT 0x8
> +#define REG_PWMS 0x10
> +#define REG_PWMCMP0 0x20
I suggest a common prefix for these defines. Something like
PWM_SIFIVE_
> +
> +/* PWMCFG fields */
> +#define BIT_PWM_SCALE 0
> +#define BIT_PWM_STICKY 8
> +#define BIT_PWM_ZERO_ZMP 9
the manual calls this "pwmzerocmp".
> +#define BIT_PWM_DEGLITCH 10
> +#define BIT_PWM_EN_ALWAYS 12
> +#define BIT_PWM_EN_ONCE 13
> +#define BIT_PWM0_CENTER 16
> +#define BIT_PWM0_GANG 24
> +#define BIT_PWM0_IP 28
Also a common prefix please. Something like PWM_SIFIVE_PWMCFG_ seems
sensible.
> +#define SIZE_PWMCMP 4
Please describe what this constant means. I think this is "ncmp" in the
reference manual. If so, using PWM_SIFIVE_NCMP as name instead seems
adequate.
> +#define MASK_PWM_SCALE 0xf
MASK_PWM_SCALE is unused, please drop it.
> +struct sifive_pwm_device {
> + struct pwm_chip chip;
> + struct notifier_block notifier;
> + struct clk *clk;
> + void __iomem *regs;
> + unsigned int approx_period;
> + unsigned int real_period;
> +};
I'd call this pwm_sifive_ddata. The prefix because the driver is called
pwm-sifive and ddata because this is driver data and not a device.
> +static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c)
> +{
> + return container_of(c, struct sifive_pwm_device, chip);
> +}
> +
> +static void sifive_pwm_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> + struct pwm_state *state)
given that the driver is called pwm-sifive, please use pwm_sifive as
function prefix.
> +{
> + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> + u32 duty;
> +
> + duty = readl(pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> +
> + state->period = pwm->real_period;
> + state->duty_cycle = ((u64)duty * pwm->real_period) >> 16;
In the reference manual this 16 is called "cmpwidth" I think. If I
understand correctly this might in theory be different from 16, so it
would be great if this would be at least a cpp symbol for now.
> + state->polarity = PWM_POLARITY_INVERSED;
> + state->enabled = duty > 0;
> +}
> +
> +static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
> + struct pwm_state *state)
> +{
> + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> + unsigned int duty_cycle;
> + u32 frac;
> +
> + if (state->polarity != PWM_POLARITY_INVERSED)
> + return -EINVAL;
> +
> + duty_cycle = state->duty_cycle;
> + if (!state->enabled)
> + duty_cycle = 0;
> +
> + frac = div_u64((u64)duty_cycle << 16, state->period);
> + frac = min(frac, 0xFFFFU);
In the previous review round I asked here:
| Also if real_period is for example 10 ms and the consumer requests
| duty=12 ms + period=100 ms, the hardware is configured for duty=1.2 ms +
| period=10 ms, right?
which you confirmed. IMHO this is not acceptable. If the period is
fixed, you should return -EINVAL (I think) if a different period is
requested.
> + writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
If you get a constant inactive output with frac=0 and a constant active
output with frac=0xffff the calculation above seems wrong to me.
(A value i written to the pwmcmpX register means a duty cycle of
(i * period / 0xffff). Your calculation assumes a divisor of 0x10000
however.)
> +
> + if (state->enabled)
> + sifive_pwm_get_state(chip, dev, state);
@Thierry: Should we bless this correction of state?
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops sifive_pwm_ops = {
> + .get_state = sifive_pwm_get_state,
> + .apply = sifive_pwm_apply,
> + .owner = THIS_MODULE,
> +};
> +
> +static struct pwm_device *sifive_pwm_xlate(struct pwm_chip *chip,
> + const struct of_phandle_args *args)
> +{
> + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> + struct pwm_device *dev;
> +
> + if (args->args[0] >= chip->npwm)
> + return ERR_PTR(-EINVAL);
> +
> + dev = pwm_request_from_chip(chip, args->args[0], NULL);
> + if (IS_ERR(dev))
> + return dev;
> +
> + /* The period cannot be changed on a per-PWM basis */
> + dev->args.period = pwm->real_period;
> + dev->args.polarity = PWM_POLARITY_NORMAL;
> + if (args->args[1] & PWM_POLARITY_INVERSED)
> + dev->args.polarity = PWM_POLARITY_INVERSED;
> +
> + return dev;
> +}
> +
> +static void sifive_pwm_update_clock(struct sifive_pwm_device *pwm,
> + unsigned long rate)
> +{
> + /* (1 << (16+scale)) * 10^9/rate = real_period */
> + unsigned long scale_pow = (pwm->approx_period * (u64)rate) / 1000000000;
NSEC_PER_SEC instead of 1000000000
> + int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
> +
> + writel((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE),
> + pwm->regs + REG_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 sifive_pwm_clock_notifier(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + struct clk_notifier_data *ndata = data;
> + struct sifive_pwm_device *pwm =
> + container_of(nb, struct sifive_pwm_device, notifier);
> +
> + if (event == POST_RATE_CHANGE)
> + sifive_pwm_update_clock(pwm, ndata->new_rate);
Does this need locking? (Maybe not with the current state.)
> +
> + return NOTIFY_OK;
> +}
> +
> +static int sifive_pwm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *node = pdev->dev.of_node;
> + struct sifive_pwm_device *pwm;
> + struct pwm_chip *chip;
> + struct resource *res;
> + int ret;
> +
> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> + if (!pwm)
> + return -ENOMEM;
> +
> + chip = &pwm->chip;
> + chip->dev = dev;
> + chip->ops = &sifive_pwm_ops;
> + chip->of_xlate = sifive_pwm_xlate;
> + chip->of_pwm_n_cells = 2;
> + chip->base = -1;
> + chip->npwm = 4;
> +
> + ret = of_property_read_u32(node, "sifive,approx-period-ns",
> + &pwm->approx_period);
> + if (ret < 0) {
> + dev_err(dev, "Unable to read sifive,approx-period from DTS\n");
> + return ret;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + pwm->regs = devm_ioremap_resource(dev, res);
> + if (IS_ERR(pwm->regs)) {
> + dev_err(dev, "Unable to map IO resources\n");
> + return PTR_ERR(pwm->regs);
> + }
> +
> + pwm->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(pwm->clk)) {
> + 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 = sifive_pwm_clock_notifier;
> + ret = clk_notifier_register(pwm->clk, &pwm->notifier);
> + if (ret) {
> + dev_err(dev, "failed to register clock notifier: %d\n", ret);
> + clk_disable_unprepare(pwm->clk);
> + return ret;
> + }
> +
> + /* Initialize PWM config */
> + sifive_pwm_update_clock(pwm, clk_get_rate(pwm->clk));
> +
> + ret = pwmchip_add(chip);
> + if (ret < 0) {
> + dev_err(dev, "cannot register PWM: %d\n", ret);
> + clk_notifier_unregister(pwm->clk, &pwm->notifier);
> + clk_disable_unprepare(pwm->clk);
> + return ret;
> + }
Can you please use a common error path using goto?
> + platform_set_drvdata(pdev, pwm);
> + dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
> +
> + return 0;
> +}
> +
> +static int sifive_pwm_remove(struct platform_device *dev)
> +{
> + struct sifive_pwm_device *pwm = platform_get_drvdata(dev);
> + 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 sifive_pwm_of_match[] = {
> + { .compatible = "sifive,pwm0" },
> + { .compatible = "sifive,fu540-c000-pwm" },
Do you really need both compatible strings here?
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, sifive_pwm_of_match);
> +
> +static struct platform_driver sifive_pwm_driver = {
> + .probe = sifive_pwm_probe,
> + .remove = sifive_pwm_remove,
> + .driver = {
> + .name = "pwm-sifive",
> + .of_match_table = sifive_pwm_of_match,
> + },
> +};
> +module_platform_driver(sifive_pwm_driver);
> +
> +MODULE_DESCRIPTION("SiFive PWM driver");
> +MODULE_LICENSE("GPL v2");
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-15 22:00 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-15 22:00 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 Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
>
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 257 insertions(+)
> create mode 100644 drivers/pwm/pwm-sifive.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a8f47df..3bcaf6a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> To compile this driver as a module, choose M here: the module
> will be called pwm-samsung.
>
> +config PWM_SIFIVE
> + tristate "SiFive PWM support"
> + depends on OF
> + depends on COMMON_CLK
I'd say add:
depends on MACH_SIFIVE || COMPILE_TEST
(I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> + 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..7fee809
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,246 @@
> +// 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
I wonder if such an instance should be only a single PWM instead of
four. Then you were more flexible with the period lengths (using
pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
I didn't understand how the deglitch logic works yet. Currently it is
not used which might result in four edges in a single period (which is
bad).
> + */
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +/* Register offsets */
> +#define REG_PWMCFG 0x0
> +#define REG_PWMCOUNT 0x8
> +#define REG_PWMS 0x10
> +#define REG_PWMCMP0 0x20
I suggest a common prefix for these defines. Something like
PWM_SIFIVE_
> +
> +/* PWMCFG fields */
> +#define BIT_PWM_SCALE 0
> +#define BIT_PWM_STICKY 8
> +#define BIT_PWM_ZERO_ZMP 9
the manual calls this "pwmzerocmp".
> +#define BIT_PWM_DEGLITCH 10
> +#define BIT_PWM_EN_ALWAYS 12
> +#define BIT_PWM_EN_ONCE 13
> +#define BIT_PWM0_CENTER 16
> +#define BIT_PWM0_GANG 24
> +#define BIT_PWM0_IP 28
Also a common prefix please. Something like PWM_SIFIVE_PWMCFG_ seems
sensible.
> +#define SIZE_PWMCMP 4
Please describe what this constant means. I think this is "ncmp" in the
reference manual. If so, using PWM_SIFIVE_NCMP as name instead seems
adequate.
> +#define MASK_PWM_SCALE 0xf
MASK_PWM_SCALE is unused, please drop it.
> +struct sifive_pwm_device {
> + struct pwm_chip chip;
> + struct notifier_block notifier;
> + struct clk *clk;
> + void __iomem *regs;
> + unsigned int approx_period;
> + unsigned int real_period;
> +};
I'd call this pwm_sifive_ddata. The prefix because the driver is called
pwm-sifive and ddata because this is driver data and not a device.
> +static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c)
> +{
> + return container_of(c, struct sifive_pwm_device, chip);
> +}
> +
> +static void sifive_pwm_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> + struct pwm_state *state)
given that the driver is called pwm-sifive, please use pwm_sifive as
function prefix.
> +{
> + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> + u32 duty;
> +
> + duty = readl(pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> +
> + state->period = pwm->real_period;
> + state->duty_cycle = ((u64)duty * pwm->real_period) >> 16;
In the reference manual this 16 is called "cmpwidth" I think. If I
understand correctly this might in theory be different from 16, so it
would be great if this would be at least a cpp symbol for now.
> + state->polarity = PWM_POLARITY_INVERSED;
> + state->enabled = duty > 0;
> +}
> +
> +static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
> + struct pwm_state *state)
> +{
> + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> + unsigned int duty_cycle;
> + u32 frac;
> +
> + if (state->polarity != PWM_POLARITY_INVERSED)
> + return -EINVAL;
> +
> + duty_cycle = state->duty_cycle;
> + if (!state->enabled)
> + duty_cycle = 0;
> +
> + frac = div_u64((u64)duty_cycle << 16, state->period);
> + frac = min(frac, 0xFFFFU);
In the previous review round I asked here:
| Also if real_period is for example 10 ms and the consumer requests
| duty=12 ms + period=100 ms, the hardware is configured for duty=1.2 ms +
| period=10 ms, right?
which you confirmed. IMHO this is not acceptable. If the period is
fixed, you should return -EINVAL (I think) if a different period is
requested.
> + writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
If you get a constant inactive output with frac=0 and a constant active
output with frac=0xffff the calculation above seems wrong to me.
(A value i written to the pwmcmpX register means a duty cycle of
(i * period / 0xffff). Your calculation assumes a divisor of 0x10000
however.)
> +
> + if (state->enabled)
> + sifive_pwm_get_state(chip, dev, state);
@Thierry: Should we bless this correction of state?
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops sifive_pwm_ops = {
> + .get_state = sifive_pwm_get_state,
> + .apply = sifive_pwm_apply,
> + .owner = THIS_MODULE,
> +};
> +
> +static struct pwm_device *sifive_pwm_xlate(struct pwm_chip *chip,
> + const struct of_phandle_args *args)
> +{
> + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> + struct pwm_device *dev;
> +
> + if (args->args[0] >= chip->npwm)
> + return ERR_PTR(-EINVAL);
> +
> + dev = pwm_request_from_chip(chip, args->args[0], NULL);
> + if (IS_ERR(dev))
> + return dev;
> +
> + /* The period cannot be changed on a per-PWM basis */
> + dev->args.period = pwm->real_period;
> + dev->args.polarity = PWM_POLARITY_NORMAL;
> + if (args->args[1] & PWM_POLARITY_INVERSED)
> + dev->args.polarity = PWM_POLARITY_INVERSED;
> +
> + return dev;
> +}
> +
> +static void sifive_pwm_update_clock(struct sifive_pwm_device *pwm,
> + unsigned long rate)
> +{
> + /* (1 << (16+scale)) * 10^9/rate = real_period */
> + unsigned long scale_pow = (pwm->approx_period * (u64)rate) / 1000000000;
NSEC_PER_SEC instead of 1000000000
> + int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
> +
> + writel((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE),
> + pwm->regs + REG_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 sifive_pwm_clock_notifier(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + struct clk_notifier_data *ndata = data;
> + struct sifive_pwm_device *pwm =
> + container_of(nb, struct sifive_pwm_device, notifier);
> +
> + if (event == POST_RATE_CHANGE)
> + sifive_pwm_update_clock(pwm, ndata->new_rate);
Does this need locking? (Maybe not with the current state.)
> +
> + return NOTIFY_OK;
> +}
> +
> +static int sifive_pwm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *node = pdev->dev.of_node;
> + struct sifive_pwm_device *pwm;
> + struct pwm_chip *chip;
> + struct resource *res;
> + int ret;
> +
> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> + if (!pwm)
> + return -ENOMEM;
> +
> + chip = &pwm->chip;
> + chip->dev = dev;
> + chip->ops = &sifive_pwm_ops;
> + chip->of_xlate = sifive_pwm_xlate;
> + chip->of_pwm_n_cells = 2;
> + chip->base = -1;
> + chip->npwm = 4;
> +
> + ret = of_property_read_u32(node, "sifive,approx-period-ns",
> + &pwm->approx_period);
> + if (ret < 0) {
> + dev_err(dev, "Unable to read sifive,approx-period from DTS\n");
> + return ret;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + pwm->regs = devm_ioremap_resource(dev, res);
> + if (IS_ERR(pwm->regs)) {
> + dev_err(dev, "Unable to map IO resources\n");
> + return PTR_ERR(pwm->regs);
> + }
> +
> + pwm->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(pwm->clk)) {
> + 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 = sifive_pwm_clock_notifier;
> + ret = clk_notifier_register(pwm->clk, &pwm->notifier);
> + if (ret) {
> + dev_err(dev, "failed to register clock notifier: %d\n", ret);
> + clk_disable_unprepare(pwm->clk);
> + return ret;
> + }
> +
> + /* Initialize PWM config */
> + sifive_pwm_update_clock(pwm, clk_get_rate(pwm->clk));
> +
> + ret = pwmchip_add(chip);
> + if (ret < 0) {
> + dev_err(dev, "cannot register PWM: %d\n", ret);
> + clk_notifier_unregister(pwm->clk, &pwm->notifier);
> + clk_disable_unprepare(pwm->clk);
> + return ret;
> + }
Can you please use a common error path using goto?
> + platform_set_drvdata(pdev, pwm);
> + dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
> +
> + return 0;
> +}
> +
> +static int sifive_pwm_remove(struct platform_device *dev)
> +{
> + struct sifive_pwm_device *pwm = platform_get_drvdata(dev);
> + 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 sifive_pwm_of_match[] = {
> + { .compatible = "sifive,pwm0" },
> + { .compatible = "sifive,fu540-c000-pwm" },
Do you really need both compatible strings here?
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, sifive_pwm_of_match);
> +
> +static struct platform_driver sifive_pwm_driver = {
> + .probe = sifive_pwm_probe,
> + .remove = sifive_pwm_remove,
> + .driver = {
> + .name = "pwm-sifive",
> + .of_match_table = sifive_pwm_of_match,
> + },
> +};
> +module_platform_driver(sifive_pwm_driver);
> +
> +MODULE_DESCRIPTION("SiFive PWM driver");
> +MODULE_LICENSE("GPL v2");
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] 44+ messages in thread
* Re: [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller
2019-01-15 20:11 ` Uwe Kleine-König
@ 2019-01-16 9:21 ` Yash Shah
-1 siblings, 0 replies; 44+ messages in thread
From: Yash Shah @ 2019-01-16 9:21 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 Wed, Jan 16, 2019 at 1:41 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> this is v3, right? It is helpful to point this out to ease reviewing.
Yes, it is v3. Will take care of this in v4.
>
> On Fri, Jan 11, 2019 at 01:52:43PM +0530, Yash Shah wrote:
> > DT documentation for PWM controller added with updated compatible
> > string.
>
> Not sure what was updated here. But assuming this is compared to v2 this
> is not a helpful info in the commit log.
Ok, will remove the 'updated compatible string' part.
>
> > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > [Atish: Compatible string update]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> > .../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++++++++++++++++++++
> > 1 file changed, 37 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > new file mode 100644
> > index 0000000..e0fc22a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > @@ -0,0 +1,37 @@
> > +SiFive PWM controller
> > +
> > +Unlike most other PWM controllers, the SiFive PWM controller currently only
> > +supports one period for all channels in the PWM. This is set globally in DTS.
> > +The period also has significant restrictions on the values it can achieve,
> > +which the driver rounds to the nearest achievable frequency.
> > +
> > +Required properties:
> > +- compatible: Please refer to sifive-blocks-ip-versioning.txt
>
> While the description was too verbose in v2, this is too short. You
> should at least mention something like "sifive,pwmX" and
> "sifive,$cpuname-pwm" (or how ever that scheme works).
Will mention the above.
>
> > +- reg: physical base address and length of the controller's registers
> > +- clocks: Should contain a clock identifier for the PWM's parent clock.
> > +- #pwm-cells: Should be 2.
> > + The first cell is the PWM channel number
> > + The second cell is the PWM polarity
>
> I'd drop these two lines and refer to bindings/pwm/pwm.txt instead.
Will be done.
>
> > +- sifive,approx-period-ns: the driver will get as close to this period as it can
>
> As already said for v2: I'd drop "approx-".
Sure, will be done.
>
> > +- interrupts: one interrupt per PWM channel
> > +
> > +PWM RTL that corresponds to the IP block version numbers can be found
> > +here:
> > +
> > +https://github.com/sifive/sifive-blocks/tree/master/src/main/scala/devices/pwm
> > +
> > +Further information on the format of the IP
> > +block-specific version numbers can be found in
> > +Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt
> > +
> > +Examples:
> > +
> > +pwm: pwm@10020000 {
> > + compatible = "sifive,fu540-c000-pwm","sifive,pwm0";
> > + reg = <0x0 0x10020000 0x0 0x1000>;
> > + clocks = <&tlclk>;
> > + interrupt-parent = <&plic>;
> > + interrupts = <42 43 44 45>;
> > + #pwm-cells = <2>;
> > + sifive,approx-period-ns = <1000000>;
> > +};
> > --
> > 1.9.1
> >
> >
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
Thanks for the comments.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller
@ 2019-01-16 9:21 ` Yash Shah
0 siblings, 0 replies; 44+ messages in thread
From: Yash Shah @ 2019-01-16 9:21 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 Wed, Jan 16, 2019 at 1:41 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> this is v3, right? It is helpful to point this out to ease reviewing.
Yes, it is v3. Will take care of this in v4.
>
> On Fri, Jan 11, 2019 at 01:52:43PM +0530, Yash Shah wrote:
> > DT documentation for PWM controller added with updated compatible
> > string.
>
> Not sure what was updated here. But assuming this is compared to v2 this
> is not a helpful info in the commit log.
Ok, will remove the 'updated compatible string' part.
>
> > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > [Atish: Compatible string update]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> > .../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++++++++++++++++++++
> > 1 file changed, 37 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > new file mode 100644
> > index 0000000..e0fc22a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > @@ -0,0 +1,37 @@
> > +SiFive PWM controller
> > +
> > +Unlike most other PWM controllers, the SiFive PWM controller currently only
> > +supports one period for all channels in the PWM. This is set globally in DTS.
> > +The period also has significant restrictions on the values it can achieve,
> > +which the driver rounds to the nearest achievable frequency.
> > +
> > +Required properties:
> > +- compatible: Please refer to sifive-blocks-ip-versioning.txt
>
> While the description was too verbose in v2, this is too short. You
> should at least mention something like "sifive,pwmX" and
> "sifive,$cpuname-pwm" (or how ever that scheme works).
Will mention the above.
>
> > +- reg: physical base address and length of the controller's registers
> > +- clocks: Should contain a clock identifier for the PWM's parent clock.
> > +- #pwm-cells: Should be 2.
> > + The first cell is the PWM channel number
> > + The second cell is the PWM polarity
>
> I'd drop these two lines and refer to bindings/pwm/pwm.txt instead.
Will be done.
>
> > +- sifive,approx-period-ns: the driver will get as close to this period as it can
>
> As already said for v2: I'd drop "approx-".
Sure, will be done.
>
> > +- interrupts: one interrupt per PWM channel
> > +
> > +PWM RTL that corresponds to the IP block version numbers can be found
> > +here:
> > +
> > +https://github.com/sifive/sifive-blocks/tree/master/src/main/scala/devices/pwm
> > +
> > +Further information on the format of the IP
> > +block-specific version numbers can be found in
> > +Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt
> > +
> > +Examples:
> > +
> > +pwm: pwm@10020000 {
> > + compatible = "sifive,fu540-c000-pwm","sifive,pwm0";
> > + reg = <0x0 0x10020000 0x0 0x1000>;
> > + clocks = <&tlclk>;
> > + interrupt-parent = <&plic>;
> > + interrupts = <42 43 44 45>;
> > + #pwm-cells = <2>;
> > + sifive,approx-period-ns = <1000000>;
> > +};
> > --
> > 1.9.1
> >
> >
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
Thanks for the comments.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-15 22:00 ` Uwe Kleine-König
@ 2019-01-16 11:10 ` Yash Shah
-1 siblings, 0 replies; 44+ messages in thread
From: Yash Shah @ 2019-01-16 11:10 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 Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> >
> > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 257 insertions(+)
> > create mode 100644 drivers/pwm/pwm-sifive.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index a8f47df..3bcaf6a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-samsung.
> >
> > +config PWM_SIFIVE
> > + tristate "SiFive PWM support"
> > + depends on OF
> > + depends on COMMON_CLK
>
> I'd say add:
>
> depends on MACH_SIFIVE || COMPILE_TEST
>
> (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
@Paul, Do you have any comments on this?
>
> > + 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..7fee809
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,246 @@
> > +// 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
>
> I wonder if such an instance should be only a single PWM instead of
> four. Then you were more flexible with the period lengths (using
> pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
>
> I didn't understand how the deglitch logic works yet. Currently it is
> not used which might result in four edges in a single period (which is
> bad).
I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in
REG_PWMCFG. Will do that.
>
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/slab.h>
> > +
> > +/* Register offsets */
> > +#define REG_PWMCFG 0x0
> > +#define REG_PWMCOUNT 0x8
> > +#define REG_PWMS 0x10
> > +#define REG_PWMCMP0 0x20
>
> I suggest a common prefix for these defines. Something like
> PWM_SIFIVE_
Sure.
>
> > +
> > +/* PWMCFG fields */
> > +#define BIT_PWM_SCALE 0
> > +#define BIT_PWM_STICKY 8
> > +#define BIT_PWM_ZERO_ZMP 9
>
> the manual calls this "pwmzerocmp".
Will fix this.
>
> > +#define BIT_PWM_DEGLITCH 10
> > +#define BIT_PWM_EN_ALWAYS 12
> > +#define BIT_PWM_EN_ONCE 13
> > +#define BIT_PWM0_CENTER 16
> > +#define BIT_PWM0_GANG 24
> > +#define BIT_PWM0_IP 28
>
> Also a common prefix please. Something like PWM_SIFIVE_PWMCFG_ seems
> sensible.
Sure.
>
> > +#define SIZE_PWMCMP 4
>
> Please describe what this constant means. I think this is "ncmp" in the
> reference manual. If so, using PWM_SIFIVE_NCMP as name instead seems
> adequate.
No, it is not ncmp. It is used to calculate the offset for pwmcmp registers.
I will add the description for the above constant.
>
> > +#define MASK_PWM_SCALE 0xf
>
> MASK_PWM_SCALE is unused, please drop it.
Sure.
>
> > +struct sifive_pwm_device {
> > + struct pwm_chip chip;
> > + struct notifier_block notifier;
> > + struct clk *clk;
> > + void __iomem *regs;
> > + unsigned int approx_period;
> > + unsigned int real_period;
> > +};
>
> I'd call this pwm_sifive_ddata. The prefix because the driver is called
> pwm-sifive and ddata because this is driver data and not a device.
Will be done.
>
> > +static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c)
> > +{
> > + return container_of(c, struct sifive_pwm_device, chip);
> > +}
> > +
> > +static void sifive_pwm_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> > + struct pwm_state *state)
>
> given that the driver is called pwm-sifive, please use pwm_sifive as
> function prefix.
Sure. Will change it for all functions.
>
> > +{
> > + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> > + u32 duty;
> > +
> > + duty = readl(pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > +
> > + state->period = pwm->real_period;
> > + state->duty_cycle = ((u64)duty * pwm->real_period) >> 16;
>
> In the reference manual this 16 is called "cmpwidth" I think. If I
> understand correctly this might in theory be different from 16, so it
> would be great if this would be at least a cpp symbol for now.
I assume you meant to add a macro for cmpwidth and use it here.
Will be done.
>
> > + state->polarity = PWM_POLARITY_INVERSED;
> > + state->enabled = duty > 0;
> > +}
> > +
> > +static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
> > + struct pwm_state *state)
> > +{
> > + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> > + unsigned int duty_cycle;
> > + u32 frac;
> > +
> > + if (state->polarity != PWM_POLARITY_INVERSED)
> > + return -EINVAL;
> > +
> > + duty_cycle = state->duty_cycle;
> > + if (!state->enabled)
> > + duty_cycle = 0;
> > +
> > + frac = div_u64((u64)duty_cycle << 16, state->period);
> > + frac = min(frac, 0xFFFFU);
>
> In the previous review round I asked here:
>
> | Also if real_period is for example 10 ms and the consumer requests
> | duty=12 ms + period=100 ms, the hardware is configured for duty=1.2 ms +
> | period=10 ms, right?
>
> which you confirmed. IMHO this is not acceptable. If the period is
> fixed, you should return -EINVAL (I think) if a different period is
> requested.
Will return -EINVAL on state->period != pwm->real_period
>
> > + writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
>
> If you get a constant inactive output with frac=0 and a constant active
> output with frac=0xffff the calculation above seems wrong to me.
> (A value i written to the pwmcmpX register means a duty cycle of
> (i * period / 0xffff). Your calculation assumes a divisor of 0x10000
> however.)
Not sure if I get you completely. But, if divisor of 0xffff is your concern then
does the below look ok?
frac = div_u64(((u64)duty_cycle << 16) - duty_cycle, state->period);
>
> > +
> > + if (state->enabled)
> > + sifive_pwm_get_state(chip, dev, state);
>
> @Thierry: Should we bless this correction of state?
>
> > +
> > + return 0;
> > +}
> > +
> > +static const struct pwm_ops sifive_pwm_ops = {
> > + .get_state = sifive_pwm_get_state,
> > + .apply = sifive_pwm_apply,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static struct pwm_device *sifive_pwm_xlate(struct pwm_chip *chip,
> > + const struct of_phandle_args *args)
> > +{
> > + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> > + struct pwm_device *dev;
> > +
> > + if (args->args[0] >= chip->npwm)
> > + return ERR_PTR(-EINVAL);
> > +
> > + dev = pwm_request_from_chip(chip, args->args[0], NULL);
> > + if (IS_ERR(dev))
> > + return dev;
> > +
> > + /* The period cannot be changed on a per-PWM basis */
> > + dev->args.period = pwm->real_period;
> > + dev->args.polarity = PWM_POLARITY_NORMAL;
> > + if (args->args[1] & PWM_POLARITY_INVERSED)
> > + dev->args.polarity = PWM_POLARITY_INVERSED;
> > +
> > + return dev;
> > +}
> > +
> > +static void sifive_pwm_update_clock(struct sifive_pwm_device *pwm,
> > + unsigned long rate)
> > +{
> > + /* (1 << (16+scale)) * 10^9/rate = real_period */
> > + unsigned long scale_pow = (pwm->approx_period * (u64)rate) / 1000000000;
>
> NSEC_PER_SEC instead of 1000000000
Will be done.
>
> > + int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
> > +
> > + writel((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE),
> > + pwm->regs + REG_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 sifive_pwm_clock_notifier(struct notifier_block *nb,
> > + unsigned long event, void *data)
> > +{
> > + struct clk_notifier_data *ndata = data;
> > + struct sifive_pwm_device *pwm =
> > + container_of(nb, struct sifive_pwm_device, notifier);
> > +
> > + if (event == POST_RATE_CHANGE)
> > + sifive_pwm_update_clock(pwm, ndata->new_rate);
>
> Does this need locking? (Maybe not with the current state.)
No. We can add it when required.
>
> > +
> > + return NOTIFY_OK;
> > +}
> > +
> > +static int sifive_pwm_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *node = pdev->dev.of_node;
> > + struct sifive_pwm_device *pwm;
> > + struct pwm_chip *chip;
> > + struct resource *res;
> > + int ret;
> > +
> > + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> > + if (!pwm)
> > + return -ENOMEM;
> > +
> > + chip = &pwm->chip;
> > + chip->dev = dev;
> > + chip->ops = &sifive_pwm_ops;
> > + chip->of_xlate = sifive_pwm_xlate;
> > + chip->of_pwm_n_cells = 2;
> > + chip->base = -1;
> > + chip->npwm = 4;
> > +
> > + ret = of_property_read_u32(node, "sifive,approx-period-ns",
> > + &pwm->approx_period);
> > + if (ret < 0) {
> > + dev_err(dev, "Unable to read sifive,approx-period from DTS\n");
> > + return ret;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + pwm->regs = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(pwm->regs)) {
> > + dev_err(dev, "Unable to map IO resources\n");
> > + return PTR_ERR(pwm->regs);
> > + }
> > +
> > + pwm->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(pwm->clk)) {
> > + 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 = sifive_pwm_clock_notifier;
> > + ret = clk_notifier_register(pwm->clk, &pwm->notifier);
> > + if (ret) {
> > + dev_err(dev, "failed to register clock notifier: %d\n", ret);
> > + clk_disable_unprepare(pwm->clk);
> > + return ret;
> > + }
> > +
> > + /* Initialize PWM config */
> > + sifive_pwm_update_clock(pwm, clk_get_rate(pwm->clk));
> > +
> > + ret = pwmchip_add(chip);
> > + if (ret < 0) {
> > + dev_err(dev, "cannot register PWM: %d\n", ret);
> > + clk_notifier_unregister(pwm->clk, &pwm->notifier);
> > + clk_disable_unprepare(pwm->clk);
> > + return ret;
> > + }
>
> Can you please use a common error path using goto?
Sure.
>
> > + platform_set_drvdata(pdev, pwm);
> > + dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
> > +
> > + return 0;
> > +}
> > +
> > +static int sifive_pwm_remove(struct platform_device *dev)
> > +{
> > + struct sifive_pwm_device *pwm = platform_get_drvdata(dev);
> > + 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 sifive_pwm_of_match[] = {
> > + { .compatible = "sifive,pwm0" },
> > + { .compatible = "sifive,fu540-c000-pwm" },
>
> Do you really need both compatible strings here?
I believe I can remove the second compatible string.
@Paul, Correct me if I am wrong.
>
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, sifive_pwm_of_match);
> > +
> > +static struct platform_driver sifive_pwm_driver = {
> > + .probe = sifive_pwm_probe,
> > + .remove = sifive_pwm_remove,
> > + .driver = {
> > + .name = "pwm-sifive",
> > + .of_match_table = sifive_pwm_of_match,
> > + },
> > +};
> > +module_platform_driver(sifive_pwm_driver);
> > +
> > +MODULE_DESCRIPTION("SiFive PWM driver");
> > +MODULE_LICENSE("GPL v2");
>
> Best regards
> Uwe
Thanks for the comments.
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-16 11:10 ` Yash Shah
0 siblings, 0 replies; 44+ messages in thread
From: Yash Shah @ 2019-01-16 11:10 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 Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> >
> > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 257 insertions(+)
> > create mode 100644 drivers/pwm/pwm-sifive.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index a8f47df..3bcaf6a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-samsung.
> >
> > +config PWM_SIFIVE
> > + tristate "SiFive PWM support"
> > + depends on OF
> > + depends on COMMON_CLK
>
> I'd say add:
>
> depends on MACH_SIFIVE || COMPILE_TEST
>
> (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
@Paul, Do you have any comments on this?
>
> > + 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..7fee809
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,246 @@
> > +// 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
>
> I wonder if such an instance should be only a single PWM instead of
> four. Then you were more flexible with the period lengths (using
> pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
>
> I didn't understand how the deglitch logic works yet. Currently it is
> not used which might result in four edges in a single period (which is
> bad).
I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in
REG_PWMCFG. Will do that.
>
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/slab.h>
> > +
> > +/* Register offsets */
> > +#define REG_PWMCFG 0x0
> > +#define REG_PWMCOUNT 0x8
> > +#define REG_PWMS 0x10
> > +#define REG_PWMCMP0 0x20
>
> I suggest a common prefix for these defines. Something like
> PWM_SIFIVE_
Sure.
>
> > +
> > +/* PWMCFG fields */
> > +#define BIT_PWM_SCALE 0
> > +#define BIT_PWM_STICKY 8
> > +#define BIT_PWM_ZERO_ZMP 9
>
> the manual calls this "pwmzerocmp".
Will fix this.
>
> > +#define BIT_PWM_DEGLITCH 10
> > +#define BIT_PWM_EN_ALWAYS 12
> > +#define BIT_PWM_EN_ONCE 13
> > +#define BIT_PWM0_CENTER 16
> > +#define BIT_PWM0_GANG 24
> > +#define BIT_PWM0_IP 28
>
> Also a common prefix please. Something like PWM_SIFIVE_PWMCFG_ seems
> sensible.
Sure.
>
> > +#define SIZE_PWMCMP 4
>
> Please describe what this constant means. I think this is "ncmp" in the
> reference manual. If so, using PWM_SIFIVE_NCMP as name instead seems
> adequate.
No, it is not ncmp. It is used to calculate the offset for pwmcmp registers.
I will add the description for the above constant.
>
> > +#define MASK_PWM_SCALE 0xf
>
> MASK_PWM_SCALE is unused, please drop it.
Sure.
>
> > +struct sifive_pwm_device {
> > + struct pwm_chip chip;
> > + struct notifier_block notifier;
> > + struct clk *clk;
> > + void __iomem *regs;
> > + unsigned int approx_period;
> > + unsigned int real_period;
> > +};
>
> I'd call this pwm_sifive_ddata. The prefix because the driver is called
> pwm-sifive and ddata because this is driver data and not a device.
Will be done.
>
> > +static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c)
> > +{
> > + return container_of(c, struct sifive_pwm_device, chip);
> > +}
> > +
> > +static void sifive_pwm_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> > + struct pwm_state *state)
>
> given that the driver is called pwm-sifive, please use pwm_sifive as
> function prefix.
Sure. Will change it for all functions.
>
> > +{
> > + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> > + u32 duty;
> > +
> > + duty = readl(pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > +
> > + state->period = pwm->real_period;
> > + state->duty_cycle = ((u64)duty * pwm->real_period) >> 16;
>
> In the reference manual this 16 is called "cmpwidth" I think. If I
> understand correctly this might in theory be different from 16, so it
> would be great if this would be at least a cpp symbol for now.
I assume you meant to add a macro for cmpwidth and use it here.
Will be done.
>
> > + state->polarity = PWM_POLARITY_INVERSED;
> > + state->enabled = duty > 0;
> > +}
> > +
> > +static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
> > + struct pwm_state *state)
> > +{
> > + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> > + unsigned int duty_cycle;
> > + u32 frac;
> > +
> > + if (state->polarity != PWM_POLARITY_INVERSED)
> > + return -EINVAL;
> > +
> > + duty_cycle = state->duty_cycle;
> > + if (!state->enabled)
> > + duty_cycle = 0;
> > +
> > + frac = div_u64((u64)duty_cycle << 16, state->period);
> > + frac = min(frac, 0xFFFFU);
>
> In the previous review round I asked here:
>
> | Also if real_period is for example 10 ms and the consumer requests
> | duty=12 ms + period=100 ms, the hardware is configured for duty=1.2 ms +
> | period=10 ms, right?
>
> which you confirmed. IMHO this is not acceptable. If the period is
> fixed, you should return -EINVAL (I think) if a different period is
> requested.
Will return -EINVAL on state->period != pwm->real_period
>
> > + writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
>
> If you get a constant inactive output with frac=0 and a constant active
> output with frac=0xffff the calculation above seems wrong to me.
> (A value i written to the pwmcmpX register means a duty cycle of
> (i * period / 0xffff). Your calculation assumes a divisor of 0x10000
> however.)
Not sure if I get you completely. But, if divisor of 0xffff is your concern then
does the below look ok?
frac = div_u64(((u64)duty_cycle << 16) - duty_cycle, state->period);
>
> > +
> > + if (state->enabled)
> > + sifive_pwm_get_state(chip, dev, state);
>
> @Thierry: Should we bless this correction of state?
>
> > +
> > + return 0;
> > +}
> > +
> > +static const struct pwm_ops sifive_pwm_ops = {
> > + .get_state = sifive_pwm_get_state,
> > + .apply = sifive_pwm_apply,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static struct pwm_device *sifive_pwm_xlate(struct pwm_chip *chip,
> > + const struct of_phandle_args *args)
> > +{
> > + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> > + struct pwm_device *dev;
> > +
> > + if (args->args[0] >= chip->npwm)
> > + return ERR_PTR(-EINVAL);
> > +
> > + dev = pwm_request_from_chip(chip, args->args[0], NULL);
> > + if (IS_ERR(dev))
> > + return dev;
> > +
> > + /* The period cannot be changed on a per-PWM basis */
> > + dev->args.period = pwm->real_period;
> > + dev->args.polarity = PWM_POLARITY_NORMAL;
> > + if (args->args[1] & PWM_POLARITY_INVERSED)
> > + dev->args.polarity = PWM_POLARITY_INVERSED;
> > +
> > + return dev;
> > +}
> > +
> > +static void sifive_pwm_update_clock(struct sifive_pwm_device *pwm,
> > + unsigned long rate)
> > +{
> > + /* (1 << (16+scale)) * 10^9/rate = real_period */
> > + unsigned long scale_pow = (pwm->approx_period * (u64)rate) / 1000000000;
>
> NSEC_PER_SEC instead of 1000000000
Will be done.
>
> > + int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
> > +
> > + writel((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE),
> > + pwm->regs + REG_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 sifive_pwm_clock_notifier(struct notifier_block *nb,
> > + unsigned long event, void *data)
> > +{
> > + struct clk_notifier_data *ndata = data;
> > + struct sifive_pwm_device *pwm =
> > + container_of(nb, struct sifive_pwm_device, notifier);
> > +
> > + if (event == POST_RATE_CHANGE)
> > + sifive_pwm_update_clock(pwm, ndata->new_rate);
>
> Does this need locking? (Maybe not with the current state.)
No. We can add it when required.
>
> > +
> > + return NOTIFY_OK;
> > +}
> > +
> > +static int sifive_pwm_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *node = pdev->dev.of_node;
> > + struct sifive_pwm_device *pwm;
> > + struct pwm_chip *chip;
> > + struct resource *res;
> > + int ret;
> > +
> > + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> > + if (!pwm)
> > + return -ENOMEM;
> > +
> > + chip = &pwm->chip;
> > + chip->dev = dev;
> > + chip->ops = &sifive_pwm_ops;
> > + chip->of_xlate = sifive_pwm_xlate;
> > + chip->of_pwm_n_cells = 2;
> > + chip->base = -1;
> > + chip->npwm = 4;
> > +
> > + ret = of_property_read_u32(node, "sifive,approx-period-ns",
> > + &pwm->approx_period);
> > + if (ret < 0) {
> > + dev_err(dev, "Unable to read sifive,approx-period from DTS\n");
> > + return ret;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + pwm->regs = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(pwm->regs)) {
> > + dev_err(dev, "Unable to map IO resources\n");
> > + return PTR_ERR(pwm->regs);
> > + }
> > +
> > + pwm->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(pwm->clk)) {
> > + 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 = sifive_pwm_clock_notifier;
> > + ret = clk_notifier_register(pwm->clk, &pwm->notifier);
> > + if (ret) {
> > + dev_err(dev, "failed to register clock notifier: %d\n", ret);
> > + clk_disable_unprepare(pwm->clk);
> > + return ret;
> > + }
> > +
> > + /* Initialize PWM config */
> > + sifive_pwm_update_clock(pwm, clk_get_rate(pwm->clk));
> > +
> > + ret = pwmchip_add(chip);
> > + if (ret < 0) {
> > + dev_err(dev, "cannot register PWM: %d\n", ret);
> > + clk_notifier_unregister(pwm->clk, &pwm->notifier);
> > + clk_disable_unprepare(pwm->clk);
> > + return ret;
> > + }
>
> Can you please use a common error path using goto?
Sure.
>
> > + platform_set_drvdata(pdev, pwm);
> > + dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
> > +
> > + return 0;
> > +}
> > +
> > +static int sifive_pwm_remove(struct platform_device *dev)
> > +{
> > + struct sifive_pwm_device *pwm = platform_get_drvdata(dev);
> > + 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 sifive_pwm_of_match[] = {
> > + { .compatible = "sifive,pwm0" },
> > + { .compatible = "sifive,fu540-c000-pwm" },
>
> Do you really need both compatible strings here?
I believe I can remove the second compatible string.
@Paul, Correct me if I am wrong.
>
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, sifive_pwm_of_match);
> > +
> > +static struct platform_driver sifive_pwm_driver = {
> > + .probe = sifive_pwm_probe,
> > + .remove = sifive_pwm_remove,
> > + .driver = {
> > + .name = "pwm-sifive",
> > + .of_match_table = sifive_pwm_of_match,
> > + },
> > +};
> > +module_platform_driver(sifive_pwm_driver);
> > +
> > +MODULE_DESCRIPTION("SiFive PWM driver");
> > +MODULE_LICENSE("GPL v2");
>
> Best regards
> Uwe
Thanks for the comments.
>
> --
> 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] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-16 11:10 ` Yash Shah
(?)
@ 2019-01-16 16:46 ` Uwe Kleine-König
-1 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-16 16:46 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 Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> > >
> > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > [Atish: Various fixes and code cleanup]
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > ---
> > > drivers/pwm/Kconfig | 10 ++
> > > drivers/pwm/Makefile | 1 +
> > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 257 insertions(+)
> > > create mode 100644 drivers/pwm/pwm-sifive.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index a8f47df..3bcaf6a 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > To compile this driver as a module, choose M here: the module
> > > will be called pwm-samsung.
> > >
> > > +config PWM_SIFIVE
> > > + tristate "SiFive PWM support"
> > > + depends on OF
> > > + depends on COMMON_CLK
> >
> > I'd say add:
> >
> > depends on MACH_SIFIVE || COMPILE_TEST
> >
> > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
>
> As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> @Paul, Do you have any comments on this?
If this is not going to be available at least protect it by
depends RISCV || COMPILE_TEST
> > I wonder if such an instance should be only a single PWM instead of
> > four. Then you were more flexible with the period lengths (using
> > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
> >
> > I didn't understand how the deglitch logic works yet. Currently it is
> > not used which might result in four edges in a single period (which is
> > bad).
>
> I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in
> REG_PWMCFG. Will do that.
This only works for the first pwm output though.
> > > +struct sifive_pwm_device {
> > > + struct pwm_chip chip;
> > > + struct notifier_block notifier;
> > > + struct clk *clk;
> > > + void __iomem *regs;
> > > + unsigned int approx_period;
When thinking about this a bit more, I think the better approach would
be to let the consumer change the period iff there is only one consumer.
Then you can drop that approx-period stuff and the result is more
flexible. (Having said that I still prefer making the driver provide
only a single PWM with the ability to have periods other than powers of
two.)
> > > + writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> >
> > If you get a constant inactive output with frac=0 and a constant active
> > output with frac=0xffff the calculation above seems wrong to me.
> > (A value i written to the pwmcmpX register means a duty cycle of
> > (i * period / 0xffff). Your calculation assumes a divisor of 0x10000
> > however.)
>
> Not sure if I get you completely. But, if divisor of 0xffff is your concern then
> does the below look ok?
>
> frac = div_u64(((u64)duty_cycle << 16) - duty_cycle, state->period);
This works (I think, didn't redo the maths), but I would prefer
frac = div_u64((u64)duty_cycle * 0xffff, state->period);
for better readability. (Maybe the compiler is even clever enough to see
this can be calculated as you suggested.)
> > > +static int sifive_pwm_clock_notifier(struct notifier_block *nb,
> > > + unsigned long event, void *data)
> > > +{
> > > + struct clk_notifier_data *ndata = data;
> > > + struct sifive_pwm_device *pwm =
> > > + container_of(nb, struct sifive_pwm_device, notifier);
> > > +
> > > + if (event == POST_RATE_CHANGE)
> > > + sifive_pwm_update_clock(pwm, ndata->new_rate);
> >
> > Does this need locking? (Maybe not with the current state.)
>
> No. We can add it when required.
My thought was, that the clk freq might change while .apply is active.
But given that you only configure the relative duty cycle this is
independent of the actual clk freq.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-16 16:46 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-16 16:46 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 Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> > >
> > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > [Atish: Various fixes and code cleanup]
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > ---
> > > drivers/pwm/Kconfig | 10 ++
> > > drivers/pwm/Makefile | 1 +
> > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 257 insertions(+)
> > > create mode 100644 drivers/pwm/pwm-sifive.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index a8f47df..3bcaf6a 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > To compile this driver as a module, choose M here: the module
> > > will be called pwm-samsung.
> > >
> > > +config PWM_SIFIVE
> > > + tristate "SiFive PWM support"
> > > + depends on OF
> > > + depends on COMMON_CLK
> >
> > I'd say add:
> >
> > depends on MACH_SIFIVE || COMPILE_TEST
> >
> > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
>
> As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> @Paul, Do you have any comments on this?
If this is not going to be available at least protect it by
depends RISCV || COMPILE_TEST
> > I wonder if such an instance should be only a single PWM instead of
> > four. Then you were more flexible with the period lengths (using
> > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
> >
> > I didn't understand how the deglitch logic works yet. Currently it is
> > not used which might result in four edges in a single period (which is
> > bad).
>
> I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in
> REG_PWMCFG. Will do that.
This only works for the first pwm output though.
> > > +struct sifive_pwm_device {
> > > + struct pwm_chip chip;
> > > + struct notifier_block notifier;
> > > + struct clk *clk;
> > > + void __iomem *regs;
> > > + unsigned int approx_period;
When thinking about this a bit more, I think the better approach would
be to let the consumer change the period iff there is only one consumer.
Then you can drop that approx-period stuff and the result is more
flexible. (Having said that I still prefer making the driver provide
only a single PWM with the ability to have periods other than powers of
two.)
> > > + writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> >
> > If you get a constant inactive output with frac=0 and a constant active
> > output with frac=0xffff the calculation above seems wrong to me.
> > (A value i written to the pwmcmpX register means a duty cycle of
> > (i * period / 0xffff). Your calculation assumes a divisor of 0x10000
> > however.)
>
> Not sure if I get you completely. But, if divisor of 0xffff is your concern then
> does the below look ok?
>
> frac = div_u64(((u64)duty_cycle << 16) - duty_cycle, state->period);
This works (I think, didn't redo the maths), but I would prefer
frac = div_u64((u64)duty_cycle * 0xffff, state->period);
for better readability. (Maybe the compiler is even clever enough to see
this can be calculated as you suggested.)
> > > +static int sifive_pwm_clock_notifier(struct notifier_block *nb,
> > > + unsigned long event, void *data)
> > > +{
> > > + struct clk_notifier_data *ndata = data;
> > > + struct sifive_pwm_device *pwm =
> > > + container_of(nb, struct sifive_pwm_device, notifier);
> > > +
> > > + if (event == POST_RATE_CHANGE)
> > > + sifive_pwm_update_clock(pwm, ndata->new_rate);
> >
> > Does this need locking? (Maybe not with the current state.)
>
> No. We can add it when required.
My thought was, that the clk freq might change while .apply is active.
But given that you only configure the relative duty cycle this is
independent of the actual clk freq.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-16 16:46 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-16 16:46 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 Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> > >
> > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > [Atish: Various fixes and code cleanup]
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > ---
> > > drivers/pwm/Kconfig | 10 ++
> > > drivers/pwm/Makefile | 1 +
> > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 257 insertions(+)
> > > create mode 100644 drivers/pwm/pwm-sifive.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index a8f47df..3bcaf6a 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > To compile this driver as a module, choose M here: the module
> > > will be called pwm-samsung.
> > >
> > > +config PWM_SIFIVE
> > > + tristate "SiFive PWM support"
> > > + depends on OF
> > > + depends on COMMON_CLK
> >
> > I'd say add:
> >
> > depends on MACH_SIFIVE || COMPILE_TEST
> >
> > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
>
> As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> @Paul, Do you have any comments on this?
If this is not going to be available at least protect it by
depends RISCV || COMPILE_TEST
> > I wonder if such an instance should be only a single PWM instead of
> > four. Then you were more flexible with the period lengths (using
> > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
> >
> > I didn't understand how the deglitch logic works yet. Currently it is
> > not used which might result in four edges in a single period (which is
> > bad).
>
> I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in
> REG_PWMCFG. Will do that.
This only works for the first pwm output though.
> > > +struct sifive_pwm_device {
> > > + struct pwm_chip chip;
> > > + struct notifier_block notifier;
> > > + struct clk *clk;
> > > + void __iomem *regs;
> > > + unsigned int approx_period;
When thinking about this a bit more, I think the better approach would
be to let the consumer change the period iff there is only one consumer.
Then you can drop that approx-period stuff and the result is more
flexible. (Having said that I still prefer making the driver provide
only a single PWM with the ability to have periods other than powers of
two.)
> > > + writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> >
> > If you get a constant inactive output with frac=0 and a constant active
> > output with frac=0xffff the calculation above seems wrong to me.
> > (A value i written to the pwmcmpX register means a duty cycle of
> > (i * period / 0xffff). Your calculation assumes a divisor of 0x10000
> > however.)
>
> Not sure if I get you completely. But, if divisor of 0xffff is your concern then
> does the below look ok?
>
> frac = div_u64(((u64)duty_cycle << 16) - duty_cycle, state->period);
This works (I think, didn't redo the maths), but I would prefer
frac = div_u64((u64)duty_cycle * 0xffff, state->period);
for better readability. (Maybe the compiler is even clever enough to see
this can be calculated as you suggested.)
> > > +static int sifive_pwm_clock_notifier(struct notifier_block *nb,
> > > + unsigned long event, void *data)
> > > +{
> > > + struct clk_notifier_data *ndata = data;
> > > + struct sifive_pwm_device *pwm =
> > > + container_of(nb, struct sifive_pwm_device, notifier);
> > > +
> > > + if (event == POST_RATE_CHANGE)
> > > + sifive_pwm_update_clock(pwm, ndata->new_rate);
> >
> > Does this need locking? (Maybe not with the current state.)
>
> No. We can add it when required.
My thought was, that the clk freq might change while .apply is active.
But given that you only configure the relative duty cycle this is
independent of the actual clk freq.
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] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-16 16:46 ` Uwe Kleine-König
@ 2019-01-16 17:18 ` Paul Walmsley
-1 siblings, 0 replies; 44+ messages in thread
From: Paul Walmsley @ 2019-01-16 17:18 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Yash Shah, Palmer Dabbelt, linux-pwm, linux-riscv,
Thierry Reding, robh+dt, mark.rutland, devicetree, linux-kernel,
Sachin Ghadi, Paul Walmsley
[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]
On Wed, 16 Jan 2019, Uwe Kleine-König wrote:
> On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > index a8f47df..3bcaf6a 100644
> > > > --- a/drivers/pwm/Kconfig
> > > > +++ b/drivers/pwm/Kconfig
> > > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > > To compile this driver as a module, choose M here: the module
> > > > will be called pwm-samsung.
> > > >
> > > > +config PWM_SIFIVE
> > > > + tristate "SiFive PWM support"
> > > > + depends on OF
> > > > + depends on COMMON_CLK
> > >
> > > I'd say add:
> > >
> > > depends on MACH_SIFIVE || COMPILE_TEST
> > >
> > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> >
> > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > @Paul, Do you have any comments on this?
>
> If this is not going to be available at least protect it by
>
> depends RISCV || COMPILE_TEST
There's nothing RISC-V or SiFive SoC-specific about this driver or IP
block. The HDL for this IP block is open-source and posted on Github.
The IP block and driver would work unchanged on an ARM or MIPS SoC, and in
fact, SiFive does design ARM-based SoCs as well. Likewise, any other SoC
vendor could take the HDL for this IP block from the git tree and
implement it on their own SoC.
More generally: it's a basic principle of Linux device drivers that they
should be buildable for any architecture. The idea here is to prevent
developers from burying architecture or SoC-specific hacks into the
driver. So there shouldn't be any architecture or SoC-specific code in
any device driver, unless it's abstracted in some way - ideally through a
common framework.
So from this point of view, neither "depends MACH_SIFIVE" nor "depends
RISCV" would be appropriate. Similarly, the equivalents for other
architectures (e.g. "ARCH_ARM") or SoC manufacturers (e.g.,
"MACH_SAMSUNG") wouldn't be appropriate for any generic IP block device
driver like this one.
- Paul
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-16 17:18 ` Paul Walmsley
0 siblings, 0 replies; 44+ messages in thread
From: Paul Walmsley @ 2019-01-16 17:18 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, robh+dt, Sachin Ghadi, Yash Shah, Thierry Reding,
Paul Walmsley, linux-riscv
[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]
On Wed, 16 Jan 2019, Uwe Kleine-König wrote:
> On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > index a8f47df..3bcaf6a 100644
> > > > --- a/drivers/pwm/Kconfig
> > > > +++ b/drivers/pwm/Kconfig
> > > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > > To compile this driver as a module, choose M here: the module
> > > > will be called pwm-samsung.
> > > >
> > > > +config PWM_SIFIVE
> > > > + tristate "SiFive PWM support"
> > > > + depends on OF
> > > > + depends on COMMON_CLK
> > >
> > > I'd say add:
> > >
> > > depends on MACH_SIFIVE || COMPILE_TEST
> > >
> > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> >
> > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > @Paul, Do you have any comments on this?
>
> If this is not going to be available at least protect it by
>
> depends RISCV || COMPILE_TEST
There's nothing RISC-V or SiFive SoC-specific about this driver or IP
block. The HDL for this IP block is open-source and posted on Github.
The IP block and driver would work unchanged on an ARM or MIPS SoC, and in
fact, SiFive does design ARM-based SoCs as well. Likewise, any other SoC
vendor could take the HDL for this IP block from the git tree and
implement it on their own SoC.
More generally: it's a basic principle of Linux device drivers that they
should be buildable for any architecture. The idea here is to prevent
developers from burying architecture or SoC-specific hacks into the
driver. So there shouldn't be any architecture or SoC-specific code in
any device driver, unless it's abstracted in some way - ideally through a
common framework.
So from this point of view, neither "depends MACH_SIFIVE" nor "depends
RISCV" would be appropriate. Similarly, the equivalents for other
architectures (e.g. "ARCH_ARM") or SoC manufacturers (e.g.,
"MACH_SAMSUNG") wouldn't be appropriate for any generic IP block device
driver like this one.
- Paul
[-- 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] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-16 17:18 ` Paul Walmsley
(?)
@ 2019-01-16 17:28 ` Uwe Kleine-König
-1 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-16 17:28 UTC (permalink / raw)
To: Paul Walmsley
Cc: Yash Shah, Palmer Dabbelt, linux-pwm, linux-riscv,
Thierry Reding, robh+dt, mark.rutland, devicetree, linux-kernel,
Sachin Ghadi
On Wed, Jan 16, 2019 at 09:18:45AM -0800, Paul Walmsley wrote:
> On Wed, 16 Jan 2019, Uwe Kleine-König wrote:
>
> > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > index a8f47df..3bcaf6a 100644
> > > > > --- a/drivers/pwm/Kconfig
> > > > > +++ b/drivers/pwm/Kconfig
> > > > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > > > To compile this driver as a module, choose M here: the module
> > > > > will be called pwm-samsung.
> > > > >
> > > > > +config PWM_SIFIVE
> > > > > + tristate "SiFive PWM support"
> > > > > + depends on OF
> > > > > + depends on COMMON_CLK
> > > >
> > > > I'd say add:
> > > >
> > > > depends on MACH_SIFIVE || COMPILE_TEST
> > > >
> > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> > >
> > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > > @Paul, Do you have any comments on this?
> >
> > If this is not going to be available at least protect it by
> >
> > depends RISCV || COMPILE_TEST
>
> There's nothing RISC-V or SiFive SoC-specific about this driver or IP
> block. The HDL for this IP block is open-source and posted on Github.
> The IP block and driver would work unchanged on an ARM or MIPS SoC, and in
> fact, SiFive does design ARM-based SoCs as well. Likewise, any other SoC
> vendor could take the HDL for this IP block from the git tree and
> implement it on their own SoC.
>
> More generally: it's a basic principle of Linux device drivers that they
> should be buildable for any architecture. The idea here is to prevent
> developers from burying architecture or SoC-specific hacks into the
> driver. So there shouldn't be any architecture or SoC-specific code in
> any device driver, unless it's abstracted in some way - ideally through a
> common framework.
>
> So from this point of view, neither "depends MACH_SIFIVE" nor "depends
> RISCV" would be appropriate. Similarly, the equivalents for other
> architectures (e.g. "ARCH_ARM") or SoC manufacturers (e.g.,
> "MACH_SAMSUNG") wouldn't be appropriate for any generic IP block device
> driver like this one.
The dependency serves two purposes:
a) ensure that the build requirements are fulfilled.
b) restrict configuration to actually existing machines
When considering b) it doesn't make sense to enable the driver for (say)
a machine that is powered by an ARM SoC by TI. If you still want to
compile test the sifive driver for ARM, enable COMPILE_TEST. That's what
this symbol is there for.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-16 17:28 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-16 17:28 UTC (permalink / raw)
To: Paul Walmsley
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, robh+dt, Sachin Ghadi, Yash Shah, Thierry Reding,
linux-riscv
On Wed, Jan 16, 2019 at 09:18:45AM -0800, Paul Walmsley wrote:
> On Wed, 16 Jan 2019, Uwe Kleine-König wrote:
>
> > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > index a8f47df..3bcaf6a 100644
> > > > > --- a/drivers/pwm/Kconfig
> > > > > +++ b/drivers/pwm/Kconfig
> > > > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > > > To compile this driver as a module, choose M here: the module
> > > > > will be called pwm-samsung.
> > > > >
> > > > > +config PWM_SIFIVE
> > > > > + tristate "SiFive PWM support"
> > > > > + depends on OF
> > > > > + depends on COMMON_CLK
> > > >
> > > > I'd say add:
> > > >
> > > > depends on MACH_SIFIVE || COMPILE_TEST
> > > >
> > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> > >
> > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > > @Paul, Do you have any comments on this?
> >
> > If this is not going to be available at least protect it by
> >
> > depends RISCV || COMPILE_TEST
>
> There's nothing RISC-V or SiFive SoC-specific about this driver or IP
> block. The HDL for this IP block is open-source and posted on Github.
> The IP block and driver would work unchanged on an ARM or MIPS SoC, and in
> fact, SiFive does design ARM-based SoCs as well. Likewise, any other SoC
> vendor could take the HDL for this IP block from the git tree and
> implement it on their own SoC.
>
> More generally: it's a basic principle of Linux device drivers that they
> should be buildable for any architecture. The idea here is to prevent
> developers from burying architecture or SoC-specific hacks into the
> driver. So there shouldn't be any architecture or SoC-specific code in
> any device driver, unless it's abstracted in some way - ideally through a
> common framework.
>
> So from this point of view, neither "depends MACH_SIFIVE" nor "depends
> RISCV" would be appropriate. Similarly, the equivalents for other
> architectures (e.g. "ARCH_ARM") or SoC manufacturers (e.g.,
> "MACH_SAMSUNG") wouldn't be appropriate for any generic IP block device
> driver like this one.
The dependency serves two purposes:
a) ensure that the build requirements are fulfilled.
b) restrict configuration to actually existing machines
When considering b) it doesn't make sense to enable the driver for (say)
a machine that is powered by an ARM SoC by TI. If you still want to
compile test the sifive driver for ARM, enable COMPILE_TEST. That's what
this symbol is there for.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-16 17:28 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-16 17:28 UTC (permalink / raw)
To: Paul Walmsley
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, robh+dt, Sachin Ghadi, Yash Shah, Thierry Reding,
linux-riscv
On Wed, Jan 16, 2019 at 09:18:45AM -0800, Paul Walmsley wrote:
> On Wed, 16 Jan 2019, Uwe Kleine-König wrote:
>
> > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > index a8f47df..3bcaf6a 100644
> > > > > --- a/drivers/pwm/Kconfig
> > > > > +++ b/drivers/pwm/Kconfig
> > > > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > > > To compile this driver as a module, choose M here: the module
> > > > > will be called pwm-samsung.
> > > > >
> > > > > +config PWM_SIFIVE
> > > > > + tristate "SiFive PWM support"
> > > > > + depends on OF
> > > > > + depends on COMMON_CLK
> > > >
> > > > I'd say add:
> > > >
> > > > depends on MACH_SIFIVE || COMPILE_TEST
> > > >
> > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> > >
> > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > > @Paul, Do you have any comments on this?
> >
> > If this is not going to be available at least protect it by
> >
> > depends RISCV || COMPILE_TEST
>
> There's nothing RISC-V or SiFive SoC-specific about this driver or IP
> block. The HDL for this IP block is open-source and posted on Github.
> The IP block and driver would work unchanged on an ARM or MIPS SoC, and in
> fact, SiFive does design ARM-based SoCs as well. Likewise, any other SoC
> vendor could take the HDL for this IP block from the git tree and
> implement it on their own SoC.
>
> More generally: it's a basic principle of Linux device drivers that they
> should be buildable for any architecture. The idea here is to prevent
> developers from burying architecture or SoC-specific hacks into the
> driver. So there shouldn't be any architecture or SoC-specific code in
> any device driver, unless it's abstracted in some way - ideally through a
> common framework.
>
> So from this point of view, neither "depends MACH_SIFIVE" nor "depends
> RISCV" would be appropriate. Similarly, the equivalents for other
> architectures (e.g. "ARCH_ARM") or SoC manufacturers (e.g.,
> "MACH_SAMSUNG") wouldn't be appropriate for any generic IP block device
> driver like this one.
The dependency serves two purposes:
a) ensure that the build requirements are fulfilled.
b) restrict configuration to actually existing machines
When considering b) it doesn't make sense to enable the driver for (say)
a machine that is powered by an ARM SoC by TI. If you still want to
compile test the sifive driver for ARM, enable COMPILE_TEST. That's what
this symbol is there for.
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] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-16 17:28 ` Uwe Kleine-König
@ 2019-01-16 19:29 ` Paul Walmsley
-1 siblings, 0 replies; 44+ messages in thread
From: Paul Walmsley @ 2019-01-16 19:29 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Paul Walmsley, Yash Shah, Palmer Dabbelt, linux-pwm, linux-riscv,
Thierry Reding, robh+dt, mark.rutland, devicetree, linux-kernel,
Sachin Ghadi
[-- Attachment #1: Type: text/plain, Size: 5407 bytes --]
On Wed, 16 Jan 2019, Uwe Kleine-König wrote:
> On Wed, Jan 16, 2019 at 09:18:45AM -0800, Paul Walmsley wrote:
> > On Wed, 16 Jan 2019, Uwe Kleine-König wrote:
> >
> > > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > > index a8f47df..3bcaf6a 100644
> > > > > > --- a/drivers/pwm/Kconfig
> > > > > > +++ b/drivers/pwm/Kconfig
> > > > > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > > > > To compile this driver as a module, choose M here: the module
> > > > > > will be called pwm-samsung.
> > > > > >
> > > > > > +config PWM_SIFIVE
> > > > > > + tristate "SiFive PWM support"
> > > > > > + depends on OF
> > > > > > + depends on COMMON_CLK
> > > > >
> > > > > I'd say add:
> > > > >
> > > > > depends on MACH_SIFIVE || COMPILE_TEST
> > > > >
> > > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> > > >
> > > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > > > @Paul, Do you have any comments on this?
> > >
> > > If this is not going to be available at least protect it by
> > >
> > > depends RISCV || COMPILE_TEST
> >
> > There's nothing RISC-V or SiFive SoC-specific about this driver or IP
> > block. The HDL for this IP block is open-source and posted on Github.
> > The IP block and driver would work unchanged on an ARM or MIPS SoC, and in
> > fact, SiFive does design ARM-based SoCs as well. Likewise, any other SoC
> > vendor could take the HDL for this IP block from the git tree and
> > implement it on their own SoC.
> >
> > More generally: it's a basic principle of Linux device drivers that they
> > should be buildable for any architecture. The idea here is to prevent
> > developers from burying architecture or SoC-specific hacks into the
> > driver. So there shouldn't be any architecture or SoC-specific code in
> > any device driver, unless it's abstracted in some way - ideally through a
> > common framework.
> >
> > So from this point of view, neither "depends MACH_SIFIVE" nor "depends
> > RISCV" would be appropriate. Similarly, the equivalents for other
> > architectures (e.g. "ARCH_ARM") or SoC manufacturers (e.g.,
> > "MACH_SAMSUNG") wouldn't be appropriate for any generic IP block device
> > driver like this one.
>
> The dependency serves two purposes:
>
> a) ensure that the build requirements are fulfilled.
> b) restrict configuration to actually existing machines
>
> When considering b) it doesn't make sense to enable the driver for (say)
> a machine that is powered by an ARM SoC by TI. If you still want to
> compile test the sifive driver for ARM, enable COMPILE_TEST. That's what
> this symbol is there for.
COMPILE_TEST made slightly more sense before the broad availability of
open-source soft cores, SoC integration, and IP; and before powerful,
inexpensive FPGAs and SoCs with FPGA fabrics were common.
Even back then, COMPILE_TEST was starting to look questionable. IP blocks
from CPU- and SoC vendor-independent libraries, like DesignWare and the
Cadence IP libraries, were integrated on SoCs across varying CPU
architectures. (Fortunately, looking at the tree, subsystem maintainers
with DesignWare drivers seem to have largely avoided adding architecture
or SoC-specific Kconfig restrictions there - and thus have also avoided
the need for COMPILE_TEST.) If an unnecessary architecture Kconfig
dependency was added for a leaf driver, that Kconfig line would either
need to be patched out by hand, or if present, COMPILE_TEST would need to
be enabled.
This was less of a problem then. There were very few FPGA Linux users,
and they were mostly working on internal proprietary projects. FPGAs that
could run Linux at a reasonable speed, including MMUs and FPUs, were
expensive or were missing good tool support. So most FPGA Linux
development was restricted to ASIC vendors - the Samsungs, Qualcomms,
NVIDIAs of the world - for prototyping, and those FPGA platforms never
made it outside those companies.
The situation is different now. The major FPGA vendors have inexpensive
FPGA SoCs with full-featured CPU hard macros. The development boards can
be quite affordable (< USD 300 for the Xilinx Ultra96). There are also
now open-source SoC HDL implementations - including MMUs, FPUs, and
peripherals like PWM and UART - for the conventional FPGA market. These
can run at mid-1990's clock rates - slow by modern standards but still
quite usable.
So or vendor-specific IP blocks that are unlikely to ever be reused by
anyone else, like the NVIDIA or TI PWM blocks, maybe there's still some
justification for COMPILE_TEST. But for drivers for open-source,
SoC-independent peripheral IP blocks - or even for proprietary IP blocks
from library vendors like Synopsys or Cadence - like this PWM driver, we
shouldn't add unnecessary architecture, SoC vendor, or COMPILE_TEST
Kconfig dependencies. They will just force users to patch the kernel or
enable COMPILE_TEST for kernels that are actually meant to run on real
hardware.
- Paul
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-16 19:29 ` Paul Walmsley
0 siblings, 0 replies; 44+ messages in thread
From: Paul Walmsley @ 2019-01-16 19:29 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, robh+dt, Sachin Ghadi, Yash Shah, Thierry Reding,
Paul Walmsley, linux-riscv
[-- Attachment #1: Type: text/plain, Size: 5407 bytes --]
On Wed, 16 Jan 2019, Uwe Kleine-König wrote:
> On Wed, Jan 16, 2019 at 09:18:45AM -0800, Paul Walmsley wrote:
> > On Wed, 16 Jan 2019, Uwe Kleine-König wrote:
> >
> > > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > > index a8f47df..3bcaf6a 100644
> > > > > > --- a/drivers/pwm/Kconfig
> > > > > > +++ b/drivers/pwm/Kconfig
> > > > > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > > > > To compile this driver as a module, choose M here: the module
> > > > > > will be called pwm-samsung.
> > > > > >
> > > > > > +config PWM_SIFIVE
> > > > > > + tristate "SiFive PWM support"
> > > > > > + depends on OF
> > > > > > + depends on COMMON_CLK
> > > > >
> > > > > I'd say add:
> > > > >
> > > > > depends on MACH_SIFIVE || COMPILE_TEST
> > > > >
> > > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> > > >
> > > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > > > @Paul, Do you have any comments on this?
> > >
> > > If this is not going to be available at least protect it by
> > >
> > > depends RISCV || COMPILE_TEST
> >
> > There's nothing RISC-V or SiFive SoC-specific about this driver or IP
> > block. The HDL for this IP block is open-source and posted on Github.
> > The IP block and driver would work unchanged on an ARM or MIPS SoC, and in
> > fact, SiFive does design ARM-based SoCs as well. Likewise, any other SoC
> > vendor could take the HDL for this IP block from the git tree and
> > implement it on their own SoC.
> >
> > More generally: it's a basic principle of Linux device drivers that they
> > should be buildable for any architecture. The idea here is to prevent
> > developers from burying architecture or SoC-specific hacks into the
> > driver. So there shouldn't be any architecture or SoC-specific code in
> > any device driver, unless it's abstracted in some way - ideally through a
> > common framework.
> >
> > So from this point of view, neither "depends MACH_SIFIVE" nor "depends
> > RISCV" would be appropriate. Similarly, the equivalents for other
> > architectures (e.g. "ARCH_ARM") or SoC manufacturers (e.g.,
> > "MACH_SAMSUNG") wouldn't be appropriate for any generic IP block device
> > driver like this one.
>
> The dependency serves two purposes:
>
> a) ensure that the build requirements are fulfilled.
> b) restrict configuration to actually existing machines
>
> When considering b) it doesn't make sense to enable the driver for (say)
> a machine that is powered by an ARM SoC by TI. If you still want to
> compile test the sifive driver for ARM, enable COMPILE_TEST. That's what
> this symbol is there for.
COMPILE_TEST made slightly more sense before the broad availability of
open-source soft cores, SoC integration, and IP; and before powerful,
inexpensive FPGAs and SoCs with FPGA fabrics were common.
Even back then, COMPILE_TEST was starting to look questionable. IP blocks
from CPU- and SoC vendor-independent libraries, like DesignWare and the
Cadence IP libraries, were integrated on SoCs across varying CPU
architectures. (Fortunately, looking at the tree, subsystem maintainers
with DesignWare drivers seem to have largely avoided adding architecture
or SoC-specific Kconfig restrictions there - and thus have also avoided
the need for COMPILE_TEST.) If an unnecessary architecture Kconfig
dependency was added for a leaf driver, that Kconfig line would either
need to be patched out by hand, or if present, COMPILE_TEST would need to
be enabled.
This was less of a problem then. There were very few FPGA Linux users,
and they were mostly working on internal proprietary projects. FPGAs that
could run Linux at a reasonable speed, including MMUs and FPUs, were
expensive or were missing good tool support. So most FPGA Linux
development was restricted to ASIC vendors - the Samsungs, Qualcomms,
NVIDIAs of the world - for prototyping, and those FPGA platforms never
made it outside those companies.
The situation is different now. The major FPGA vendors have inexpensive
FPGA SoCs with full-featured CPU hard macros. The development boards can
be quite affordable (< USD 300 for the Xilinx Ultra96). There are also
now open-source SoC HDL implementations - including MMUs, FPUs, and
peripherals like PWM and UART - for the conventional FPGA market. These
can run at mid-1990's clock rates - slow by modern standards but still
quite usable.
So or vendor-specific IP blocks that are unlikely to ever be reused by
anyone else, like the NVIDIA or TI PWM blocks, maybe there's still some
justification for COMPILE_TEST. But for drivers for open-source,
SoC-independent peripheral IP blocks - or even for proprietary IP blocks
from library vendors like Synopsys or Cadence - like this PWM driver, we
shouldn't add unnecessary architecture, SoC vendor, or COMPILE_TEST
Kconfig dependencies. They will just force users to patch the kernel or
enable COMPILE_TEST for kernels that are actually meant to run on real
hardware.
- Paul
[-- 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] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-16 16:46 ` Uwe Kleine-König
@ 2019-01-17 6:45 ` Yash Shah
-1 siblings, 0 replies; 44+ messages in thread
From: Yash Shah @ 2019-01-17 6:45 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 Wed, Jan 16, 2019 at 10:16 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> > > >
> > > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > > [Atish: Various fixes and code cleanup]
> > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > > ---
> > > > drivers/pwm/Kconfig | 10 ++
> > > > drivers/pwm/Makefile | 1 +
> > > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 257 insertions(+)
> > > > create mode 100644 drivers/pwm/pwm-sifive.c
> > > >
> > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > index a8f47df..3bcaf6a 100644
> > > > --- a/drivers/pwm/Kconfig
> > > > +++ b/drivers/pwm/Kconfig
> > > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > > To compile this driver as a module, choose M here: the module
> > > > will be called pwm-samsung.
> > > >
> > > > +config PWM_SIFIVE
> > > > + tristate "SiFive PWM support"
> > > > + depends on OF
> > > > + depends on COMMON_CLK
> > >
> > > I'd say add:
> > >
> > > depends on MACH_SIFIVE || COMPILE_TEST
> > >
> > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> >
> > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > @Paul, Do you have any comments on this?
>
> If this is not going to be available at least protect it by
>
> depends RISCV || COMPILE_TEST
>
> > > I wonder if such an instance should be only a single PWM instead of
> > > four. Then you were more flexible with the period lengths (using
> > > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
> > >
> > > I didn't understand how the deglitch logic works yet. Currently it is
> > > not used which might result in four edges in a single period (which is
> > > bad).
> >
> > I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in
> > REG_PWMCFG. Will do that.
>
> This only works for the first pwm output though.
>
> > > > +struct sifive_pwm_device {
> > > > + struct pwm_chip chip;
> > > > + struct notifier_block notifier;
> > > > + struct clk *clk;
> > > > + void __iomem *regs;
> > > > + unsigned int approx_period;
>
> When thinking about this a bit more, I think the better approach would
> be to let the consumer change the period iff there is only one consumer.
> Then you can drop that approx-period stuff and the result is more
> flexible. (Having said that I still prefer making the driver provide
> only a single PWM with the ability to have periods other than powers of
> two.)
>
I am not confident about the implementation of the way you are suggesting.
As of now, I am going to stick with the current implementation.
> > > > + writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > >
> > > If you get a constant inactive output with frac=0 and a constant active
> > > output with frac=0xffff the calculation above seems wrong to me.
> > > (A value i written to the pwmcmpX register means a duty cycle of
> > > (i * period / 0xffff). Your calculation assumes a divisor of 0x10000
> > > however.)
> >
> > Not sure if I get you completely. But, if divisor of 0xffff is your concern then
> > does the below look ok?
> >
> > frac = div_u64(((u64)duty_cycle << 16) - duty_cycle, state->period);
>
> This works (I think, didn't redo the maths), but I would prefer
>
> frac = div_u64((u64)duty_cycle * 0xffff, state->period);
>
> for better readability. (Maybe the compiler is even clever enough to see
> this can be calculated as you suggested.)
Sure, will multiply it with 0xffff for better readability.
>
> > > > +static int sifive_pwm_clock_notifier(struct notifier_block *nb,
> > > > + unsigned long event, void *data)
> > > > +{
> > > > + struct clk_notifier_data *ndata = data;
> > > > + struct sifive_pwm_device *pwm =
> > > > + container_of(nb, struct sifive_pwm_device, notifier);
> > > > +
> > > > + if (event == POST_RATE_CHANGE)
> > > > + sifive_pwm_update_clock(pwm, ndata->new_rate);
> > >
> > > Does this need locking? (Maybe not with the current state.)
> >
> > No. We can add it when required.
>
> My thought was, that the clk freq might change while .apply is active.
> But given that you only configure the relative duty cycle this is
> independent of the actual clk freq.
>
> 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] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-17 6:45 ` Yash Shah
0 siblings, 0 replies; 44+ messages in thread
From: Yash Shah @ 2019-01-17 6:45 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, Sachin Ghadi, robh+dt, Thierry Reding,
Paul Walmsley, linux-riscv
On Wed, Jan 16, 2019 at 10:16 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> > > >
> > > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > > [Atish: Various fixes and code cleanup]
> > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > > ---
> > > > drivers/pwm/Kconfig | 10 ++
> > > > drivers/pwm/Makefile | 1 +
> > > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 257 insertions(+)
> > > > create mode 100644 drivers/pwm/pwm-sifive.c
> > > >
> > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > index a8f47df..3bcaf6a 100644
> > > > --- a/drivers/pwm/Kconfig
> > > > +++ b/drivers/pwm/Kconfig
> > > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > > To compile this driver as a module, choose M here: the module
> > > > will be called pwm-samsung.
> > > >
> > > > +config PWM_SIFIVE
> > > > + tristate "SiFive PWM support"
> > > > + depends on OF
> > > > + depends on COMMON_CLK
> > >
> > > I'd say add:
> > >
> > > depends on MACH_SIFIVE || COMPILE_TEST
> > >
> > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> >
> > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > @Paul, Do you have any comments on this?
>
> If this is not going to be available at least protect it by
>
> depends RISCV || COMPILE_TEST
>
> > > I wonder if such an instance should be only a single PWM instead of
> > > four. Then you were more flexible with the period lengths (using
> > > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
> > >
> > > I didn't understand how the deglitch logic works yet. Currently it is
> > > not used which might result in four edges in a single period (which is
> > > bad).
> >
> > I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in
> > REG_PWMCFG. Will do that.
>
> This only works for the first pwm output though.
>
> > > > +struct sifive_pwm_device {
> > > > + struct pwm_chip chip;
> > > > + struct notifier_block notifier;
> > > > + struct clk *clk;
> > > > + void __iomem *regs;
> > > > + unsigned int approx_period;
>
> When thinking about this a bit more, I think the better approach would
> be to let the consumer change the period iff there is only one consumer.
> Then you can drop that approx-period stuff and the result is more
> flexible. (Having said that I still prefer making the driver provide
> only a single PWM with the ability to have periods other than powers of
> two.)
>
I am not confident about the implementation of the way you are suggesting.
As of now, I am going to stick with the current implementation.
> > > > + writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> > >
> > > If you get a constant inactive output with frac=0 and a constant active
> > > output with frac=0xffff the calculation above seems wrong to me.
> > > (A value i written to the pwmcmpX register means a duty cycle of
> > > (i * period / 0xffff). Your calculation assumes a divisor of 0x10000
> > > however.)
> >
> > Not sure if I get you completely. But, if divisor of 0xffff is your concern then
> > does the below look ok?
> >
> > frac = div_u64(((u64)duty_cycle << 16) - duty_cycle, state->period);
>
> This works (I think, didn't redo the maths), but I would prefer
>
> frac = div_u64((u64)duty_cycle * 0xffff, state->period);
>
> for better readability. (Maybe the compiler is even clever enough to see
> this can be calculated as you suggested.)
Sure, will multiply it with 0xffff for better readability.
>
> > > > +static int sifive_pwm_clock_notifier(struct notifier_block *nb,
> > > > + unsigned long event, void *data)
> > > > +{
> > > > + struct clk_notifier_data *ndata = data;
> > > > + struct sifive_pwm_device *pwm =
> > > > + container_of(nb, struct sifive_pwm_device, notifier);
> > > > +
> > > > + if (event == POST_RATE_CHANGE)
> > > > + sifive_pwm_update_clock(pwm, ndata->new_rate);
> > >
> > > Does this need locking? (Maybe not with the current state.)
> >
> > No. We can add it when required.
>
> My thought was, that the clk freq might change while .apply is active.
> But given that you only configure the relative duty cycle this is
> independent of the actual clk freq.
>
> 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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-17 6:45 ` Yash Shah
@ 2019-01-17 7:28 ` Uwe Kleine-König
-1 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-17 7:28 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 Thu, Jan 17, 2019 at 12:15:38PM +0530, Yash Shah wrote:
> On Wed, Jan 16, 2019 at 10:16 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello,
> >
> > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> > > > >
> > > > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > > > [Atish: Various fixes and code cleanup]
> > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > > > ---
> > > > > drivers/pwm/Kconfig | 10 ++
> > > > > drivers/pwm/Makefile | 1 +
> > > > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > 3 files changed, 257 insertions(+)
> > > > > create mode 100644 drivers/pwm/pwm-sifive.c
> > > > >
> > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > index a8f47df..3bcaf6a 100644
> > > > > --- a/drivers/pwm/Kconfig
> > > > > +++ b/drivers/pwm/Kconfig
> > > > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > > > To compile this driver as a module, choose M here: the module
> > > > > will be called pwm-samsung.
> > > > >
> > > > > +config PWM_SIFIVE
> > > > > + tristate "SiFive PWM support"
> > > > > + depends on OF
> > > > > + depends on COMMON_CLK
> > > >
> > > > I'd say add:
> > > >
> > > > depends on MACH_SIFIVE || COMPILE_TEST
> > > >
> > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> > >
> > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > > @Paul, Do you have any comments on this?
> >
> > If this is not going to be available at least protect it by
> >
> > depends RISCV || COMPILE_TEST
> >
> > > > I wonder if such an instance should be only a single PWM instead of
> > > > four. Then you were more flexible with the period lengths (using
> > > > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
> > > >
> > > > I didn't understand how the deglitch logic works yet. Currently it is
> > > > not used which might result in four edges in a single period (which is
> > > > bad).
> > >
> > > I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in
> > > REG_PWMCFG. Will do that.
> >
> > This only works for the first pwm output though.
I mixed this up with pwmzerocmp; deglitch should work on all four
outputs.
> > > > > +struct sifive_pwm_device {
> > > > > + struct pwm_chip chip;
> > > > > + struct notifier_block notifier;
> > > > > + struct clk *clk;
> > > > > + void __iomem *regs;
> > > > > + unsigned int approx_period;
> >
> > When thinking about this a bit more, I think the better approach would
> > be to let the consumer change the period iff there is only one consumer.
> > Then you can drop that approx-period stuff and the result is more
> > flexible. (Having said that I still prefer making the driver provide
> > only a single PWM with the ability to have periods other than powers of
> > two.)
> >
>
> I am not confident about the implementation of the way you are suggesting.
> As of now, I am going to stick with the current implementation.
The idea is to count the users using the .request and .free callbacks.
Iff there is exactly one, allow changes to period.
But please consider moving to an abstraction that only provides a single
pwm instead.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-17 7:28 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-17 7:28 UTC (permalink / raw)
To: Yash Shah
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, Sachin Ghadi, robh+dt, Thierry Reding,
Paul Walmsley, linux-riscv
Hello,
On Thu, Jan 17, 2019 at 12:15:38PM +0530, Yash Shah wrote:
> On Wed, Jan 16, 2019 at 10:16 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello,
> >
> > On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> > > On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > > > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> > > > >
> > > > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > > > [Atish: Various fixes and code cleanup]
> > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > > > ---
> > > > > drivers/pwm/Kconfig | 10 ++
> > > > > drivers/pwm/Makefile | 1 +
> > > > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > 3 files changed, 257 insertions(+)
> > > > > create mode 100644 drivers/pwm/pwm-sifive.c
> > > > >
> > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > index a8f47df..3bcaf6a 100644
> > > > > --- a/drivers/pwm/Kconfig
> > > > > +++ b/drivers/pwm/Kconfig
> > > > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > > > To compile this driver as a module, choose M here: the module
> > > > > will be called pwm-samsung.
> > > > >
> > > > > +config PWM_SIFIVE
> > > > > + tristate "SiFive PWM support"
> > > > > + depends on OF
> > > > > + depends on COMMON_CLK
> > > >
> > > > I'd say add:
> > > >
> > > > depends on MACH_SIFIVE || COMPILE_TEST
> > > >
> > > > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> > >
> > > As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> > > @Paul, Do you have any comments on this?
> >
> > If this is not going to be available at least protect it by
> >
> > depends RISCV || COMPILE_TEST
> >
> > > > I wonder if such an instance should be only a single PWM instead of
> > > > four. Then you were more flexible with the period lengths (using
> > > > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
> > > >
> > > > I didn't understand how the deglitch logic works yet. Currently it is
> > > > not used which might result in four edges in a single period (which is
> > > > bad).
> > >
> > > I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in
> > > REG_PWMCFG. Will do that.
> >
> > This only works for the first pwm output though.
I mixed this up with pwmzerocmp; deglitch should work on all four
outputs.
> > > > > +struct sifive_pwm_device {
> > > > > + struct pwm_chip chip;
> > > > > + struct notifier_block notifier;
> > > > > + struct clk *clk;
> > > > > + void __iomem *regs;
> > > > > + unsigned int approx_period;
> >
> > When thinking about this a bit more, I think the better approach would
> > be to let the consumer change the period iff there is only one consumer.
> > Then you can drop that approx-period stuff and the result is more
> > flexible. (Having said that I still prefer making the driver provide
> > only a single PWM with the ability to have periods other than powers of
> > two.)
> >
>
> I am not confident about the implementation of the way you are suggesting.
> As of now, I am going to stick with the current implementation.
The idea is to count the users using the .request and .free callbacks.
Iff there is exactly one, allow changes to period.
But please consider moving to an abstraction that only provides a single
pwm instead.
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] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-16 19:29 ` Paul Walmsley
@ 2019-01-17 8:19 ` Uwe Kleine-König
-1 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-17 8:19 UTC (permalink / raw)
To: Paul Walmsley
Cc: Yash Shah, Palmer Dabbelt, linux-pwm, linux-riscv,
Thierry Reding, robh+dt, mark.rutland, devicetree, linux-kernel,
Sachin Ghadi
Hello Paul,
On Wed, Jan 16, 2019 at 11:29:35AM -0800, Paul Walmsley wrote:
> COMPILE_TEST made slightly more sense before the broad availability of
> open-source soft cores, SoC integration, and IP; and before powerful,
> inexpensive FPGAs and SoCs with FPGA fabrics were common.
>
> Even back then, COMPILE_TEST was starting to look questionable. IP blocks
> from CPU- and SoC vendor-independent libraries, like DesignWare and the
> Cadence IP libraries, were integrated on SoCs across varying CPU
> architectures. (Fortunately, looking at the tree, subsystem maintainers
> with DesignWare drivers seem to have largely avoided adding architecture
> or SoC-specific Kconfig restrictions there - and thus have also avoided
> the need for COMPILE_TEST.) If an unnecessary architecture Kconfig
> dependency was added for a leaf driver, that Kconfig line would either
> need to be patched out by hand, or if present, COMPILE_TEST would need to
> be enabled.
>
> This was less of a problem then. There were very few FPGA Linux users,
> and they were mostly working on internal proprietary projects. FPGAs that
> could run Linux at a reasonable speed, including MMUs and FPUs, were
> expensive or were missing good tool support. So most FPGA Linux
> development was restricted to ASIC vendors - the Samsungs, Qualcomms,
> NVIDIAs of the world - for prototyping, and those FPGA platforms never
> made it outside those companies.
>
> The situation is different now. The major FPGA vendors have inexpensive
> FPGA SoCs with full-featured CPU hard macros. The development boards can
> be quite affordable (< USD 300 for the Xilinx Ultra96). There are also
> now open-source SoC HDL implementations - including MMUs, FPUs, and
> peripherals like PWM and UART - for the conventional FPGA market. These
> can run at mid-1990's clock rates - slow by modern standards but still
> quite usable.
In my eyes it's better to make a driver not possible to enable out of
the box than offering to enable it while it most probably won't be used.
People who configure their own kernel and distribution kernel
maintainers will thank you. (Well ok, they won't notice, so they won't
actually tell you, but anyhow.) I'm a member of the Debian kernel team
and I see how many config items there are that are not explicitly
handled for the various different kernel images. If they were restricted
using COMPILE_TEST to just be possible to enable on machines where it is
known (or at least likely) to make sense that would help. Also when I do
a kernel version bump for a machine with a tailored kernel running (say)
on an ARM/i.MX SoC, I could more easily be careful about the relevant
changes when doing oldconfig if I were not asked about a whole bunch of
drivers that are sure to be irrelevant.
And if someone synthesizes this sifive PWM into an FPGA that is
connected to an OMAP, changing
depends RISCV || COMPILE_TEST
to something like
depends RISCV || MACH_OMAP || COMPILE_TEST
is a relatively low effort. (Or we introduce a symbol that tells that
the machine has an FPGA and based on that enable the sifive pwm driver.)
And if a single person comes up saying they need that driver on OMAP
I happily support such a change.
> So or vendor-specific IP blocks that are unlikely to ever be reused by
> anyone else, like the NVIDIA or TI PWM blocks, maybe there's still some
> justification for COMPILE_TEST. But for drivers for open-source,
> SoC-independent peripheral IP blocks - or even for proprietary IP blocks
> from library vendors like Synopsys or Cadence - like this PWM driver, we
> shouldn't add unnecessary architecture, SoC vendor, or COMPILE_TEST
> Kconfig dependencies. They will just force users to patch the kernel or
> enable COMPILE_TEST for kernels that are actually meant to run on real
> hardware.
I understand that downside. I just think that people who synthesize an
open source core into their machine and then run Linux on it are very
likely easily able to patch the Kconfig file to enable the needed
drivers and tell us. Also if you compare the number of people who hit
this problem to the number of people to have to decide if they need
PWM_SIFIVE and don't need it, I guess there will be an at least
big two-digit factor between them. And as soon as this OMAP machine with
the FPGA becomes mainstream enough that tutorials pop up in the web that
give you a copy&paste template to put the sifive pwm into it, I expect
the dependency is already fixed.
Yes, there is no big harm if you enable this driver and don't need
it. But there are hundreds of drivers, and together they do hurt.
Also if you support a "newbie" configuring their first kernel, this is
much less frightening and gives a much better impression if at least
most of the presented options matter. Also it is easier to pick your pwm
driver if it's presented in a list of ten driver than if the list has
100 items.
There are reasons for both approaches and we need to weight the
respective interests. In my eyes it's clear which is the better
approach, but obviously YMMV. So if you choose to not restrict the
kconfig symbol, at least do it consciously with the arguments for the
other side in mind. And please also consider that your position is
subjective because you're a kernel developer and personally don't care
much if configuring a kernel is difficult to a newcomer.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-17 8:19 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-17 8:19 UTC (permalink / raw)
To: Paul Walmsley
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, robh+dt, Sachin Ghadi, Yash Shah, Thierry Reding,
linux-riscv
Hello Paul,
On Wed, Jan 16, 2019 at 11:29:35AM -0800, Paul Walmsley wrote:
> COMPILE_TEST made slightly more sense before the broad availability of
> open-source soft cores, SoC integration, and IP; and before powerful,
> inexpensive FPGAs and SoCs with FPGA fabrics were common.
>
> Even back then, COMPILE_TEST was starting to look questionable. IP blocks
> from CPU- and SoC vendor-independent libraries, like DesignWare and the
> Cadence IP libraries, were integrated on SoCs across varying CPU
> architectures. (Fortunately, looking at the tree, subsystem maintainers
> with DesignWare drivers seem to have largely avoided adding architecture
> or SoC-specific Kconfig restrictions there - and thus have also avoided
> the need for COMPILE_TEST.) If an unnecessary architecture Kconfig
> dependency was added for a leaf driver, that Kconfig line would either
> need to be patched out by hand, or if present, COMPILE_TEST would need to
> be enabled.
>
> This was less of a problem then. There were very few FPGA Linux users,
> and they were mostly working on internal proprietary projects. FPGAs that
> could run Linux at a reasonable speed, including MMUs and FPUs, were
> expensive or were missing good tool support. So most FPGA Linux
> development was restricted to ASIC vendors - the Samsungs, Qualcomms,
> NVIDIAs of the world - for prototyping, and those FPGA platforms never
> made it outside those companies.
>
> The situation is different now. The major FPGA vendors have inexpensive
> FPGA SoCs with full-featured CPU hard macros. The development boards can
> be quite affordable (< USD 300 for the Xilinx Ultra96). There are also
> now open-source SoC HDL implementations - including MMUs, FPUs, and
> peripherals like PWM and UART - for the conventional FPGA market. These
> can run at mid-1990's clock rates - slow by modern standards but still
> quite usable.
In my eyes it's better to make a driver not possible to enable out of
the box than offering to enable it while it most probably won't be used.
People who configure their own kernel and distribution kernel
maintainers will thank you. (Well ok, they won't notice, so they won't
actually tell you, but anyhow.) I'm a member of the Debian kernel team
and I see how many config items there are that are not explicitly
handled for the various different kernel images. If they were restricted
using COMPILE_TEST to just be possible to enable on machines where it is
known (or at least likely) to make sense that would help. Also when I do
a kernel version bump for a machine with a tailored kernel running (say)
on an ARM/i.MX SoC, I could more easily be careful about the relevant
changes when doing oldconfig if I were not asked about a whole bunch of
drivers that are sure to be irrelevant.
And if someone synthesizes this sifive PWM into an FPGA that is
connected to an OMAP, changing
depends RISCV || COMPILE_TEST
to something like
depends RISCV || MACH_OMAP || COMPILE_TEST
is a relatively low effort. (Or we introduce a symbol that tells that
the machine has an FPGA and based on that enable the sifive pwm driver.)
And if a single person comes up saying they need that driver on OMAP
I happily support such a change.
> So or vendor-specific IP blocks that are unlikely to ever be reused by
> anyone else, like the NVIDIA or TI PWM blocks, maybe there's still some
> justification for COMPILE_TEST. But for drivers for open-source,
> SoC-independent peripheral IP blocks - or even for proprietary IP blocks
> from library vendors like Synopsys or Cadence - like this PWM driver, we
> shouldn't add unnecessary architecture, SoC vendor, or COMPILE_TEST
> Kconfig dependencies. They will just force users to patch the kernel or
> enable COMPILE_TEST for kernels that are actually meant to run on real
> hardware.
I understand that downside. I just think that people who synthesize an
open source core into their machine and then run Linux on it are very
likely easily able to patch the Kconfig file to enable the needed
drivers and tell us. Also if you compare the number of people who hit
this problem to the number of people to have to decide if they need
PWM_SIFIVE and don't need it, I guess there will be an at least
big two-digit factor between them. And as soon as this OMAP machine with
the FPGA becomes mainstream enough that tutorials pop up in the web that
give you a copy&paste template to put the sifive pwm into it, I expect
the dependency is already fixed.
Yes, there is no big harm if you enable this driver and don't need
it. But there are hundreds of drivers, and together they do hurt.
Also if you support a "newbie" configuring their first kernel, this is
much less frightening and gives a much better impression if at least
most of the presented options matter. Also it is easier to pick your pwm
driver if it's presented in a list of ten driver than if the list has
100 items.
There are reasons for both approaches and we need to weight the
respective interests. In my eyes it's clear which is the better
approach, but obviously YMMV. So if you choose to not restrict the
kconfig symbol, at least do it consciously with the arguments for the
other side in mind. And please also consider that your position is
subjective because you're a kernel developer and personally don't care
much if configuring a kernel is difficult to a newcomer.
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] 44+ messages in thread
* Re: [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller
2019-01-16 9:21 ` Yash Shah
@ 2019-01-21 11:20 ` Thierry Reding
-1 siblings, 0 replies; 44+ messages in thread
From: Thierry Reding @ 2019-01-21 11:20 UTC (permalink / raw)
To: Yash Shah
Cc: Uwe Kleine-König, Palmer Dabbelt, linux-pwm, linux-riscv,
robh+dt, mark.rutland, devicetree, linux-kernel, Sachin Ghadi,
Paul Walmsley
[-- Attachment #1: Type: text/plain, Size: 3106 bytes --]
On Wed, Jan 16, 2019 at 02:51:31PM +0530, Yash Shah wrote:
> On Wed, Jan 16, 2019 at 1:41 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello,
> >
> > this is v3, right? It is helpful to point this out to ease reviewing.
>
> Yes, it is v3. Will take care of this in v4.
>
> >
> > On Fri, Jan 11, 2019 at 01:52:43PM +0530, Yash Shah wrote:
> > > DT documentation for PWM controller added with updated compatible
> > > string.
> >
> > Not sure what was updated here. But assuming this is compared to v2 this
> > is not a helpful info in the commit log.
>
> Ok, will remove the 'updated compatible string' part.
>
> >
> > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > [Atish: Compatible string update]
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > ---
> > > .../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++++++++++++++++++++
> > > 1 file changed, 37 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > > new file mode 100644
> > > index 0000000..e0fc22a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > > @@ -0,0 +1,37 @@
> > > +SiFive PWM controller
> > > +
> > > +Unlike most other PWM controllers, the SiFive PWM controller currently only
> > > +supports one period for all channels in the PWM. This is set globally in DTS.
> > > +The period also has significant restrictions on the values it can achieve,
> > > +which the driver rounds to the nearest achievable frequency.
> > > +
> > > +Required properties:
> > > +- compatible: Please refer to sifive-blocks-ip-versioning.txt
> >
> > While the description was too verbose in v2, this is too short. You
> > should at least mention something like "sifive,pwmX" and
> > "sifive,$cpuname-pwm" (or how ever that scheme works).
>
> Will mention the above.
>
> >
> > > +- reg: physical base address and length of the controller's registers
> > > +- clocks: Should contain a clock identifier for the PWM's parent clock.
> > > +- #pwm-cells: Should be 2.
> > > + The first cell is the PWM channel number
> > > + The second cell is the PWM polarity
> >
> > I'd drop these two lines and refer to bindings/pwm/pwm.txt instead.
>
> Will be done.
I don't think you can do that. You're omitting the second cell
representing the period, so this is different from the standard binding
and therefore needs to be explicit.
That said, given the rest of the discussion in the other threads, maybe
it no longer makes sense to set the period on the whole block, but
rather just note in the binding that all PWMs need to run at the same
period. If the driver already refuses to apply incompatible periods,
your users are going to notice that they've got the DT wrong.
This would have the advantage that you can use the standard bindings.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller
@ 2019-01-21 11:20 ` Thierry Reding
0 siblings, 0 replies; 44+ messages in thread
From: Thierry Reding @ 2019-01-21 11:20 UTC (permalink / raw)
To: Yash Shah
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, Sachin Ghadi, robh+dt, Paul Walmsley,
Uwe Kleine-König, linux-riscv
[-- Attachment #1.1: Type: text/plain, Size: 3106 bytes --]
On Wed, Jan 16, 2019 at 02:51:31PM +0530, Yash Shah wrote:
> On Wed, Jan 16, 2019 at 1:41 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello,
> >
> > this is v3, right? It is helpful to point this out to ease reviewing.
>
> Yes, it is v3. Will take care of this in v4.
>
> >
> > On Fri, Jan 11, 2019 at 01:52:43PM +0530, Yash Shah wrote:
> > > DT documentation for PWM controller added with updated compatible
> > > string.
> >
> > Not sure what was updated here. But assuming this is compared to v2 this
> > is not a helpful info in the commit log.
>
> Ok, will remove the 'updated compatible string' part.
>
> >
> > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > [Atish: Compatible string update]
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > ---
> > > .../devicetree/bindings/pwm/pwm-sifive.txt | 37 ++++++++++++++++++++++
> > > 1 file changed, 37 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > > new file mode 100644
> > > index 0000000..e0fc22a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > > @@ -0,0 +1,37 @@
> > > +SiFive PWM controller
> > > +
> > > +Unlike most other PWM controllers, the SiFive PWM controller currently only
> > > +supports one period for all channels in the PWM. This is set globally in DTS.
> > > +The period also has significant restrictions on the values it can achieve,
> > > +which the driver rounds to the nearest achievable frequency.
> > > +
> > > +Required properties:
> > > +- compatible: Please refer to sifive-blocks-ip-versioning.txt
> >
> > While the description was too verbose in v2, this is too short. You
> > should at least mention something like "sifive,pwmX" and
> > "sifive,$cpuname-pwm" (or how ever that scheme works).
>
> Will mention the above.
>
> >
> > > +- reg: physical base address and length of the controller's registers
> > > +- clocks: Should contain a clock identifier for the PWM's parent clock.
> > > +- #pwm-cells: Should be 2.
> > > + The first cell is the PWM channel number
> > > + The second cell is the PWM polarity
> >
> > I'd drop these two lines and refer to bindings/pwm/pwm.txt instead.
>
> Will be done.
I don't think you can do that. You're omitting the second cell
representing the period, so this is different from the standard binding
and therefore needs to be explicit.
That said, given the rest of the discussion in the other threads, maybe
it no longer makes sense to set the period on the whole block, but
rather just note in the binding that all PWMs need to run at the same
period. If the driver already refuses to apply incompatible periods,
your users are going to notice that they've got the DT wrong.
This would have the advantage that you can use the standard bindings.
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] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-15 22:00 ` Uwe Kleine-König
@ 2019-01-21 11:30 ` Thierry Reding
-1 siblings, 0 replies; 44+ messages in thread
From: Thierry Reding @ 2019-01-21 11:30 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: 4236 bytes --]
On Tue, Jan 15, 2019 at 11:00:46PM +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> >
> > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 257 insertions(+)
> > create mode 100644 drivers/pwm/pwm-sifive.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index a8f47df..3bcaf6a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-samsung.
> >
> > +config PWM_SIFIVE
> > + tristate "SiFive PWM support"
> > + depends on OF
> > + depends on COMMON_CLK
>
> I'd say add:
>
> depends on MACH_SIFIVE || COMPILE_TEST
>
> (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
>
> > + 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..7fee809
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,246 @@
> > +// 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
>
> I wonder if such an instance should be only a single PWM instead of
> four. Then you were more flexible with the period lengths (using
> pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
I thought this IP only allowed a single period for all PWM channels in
the IP. If so, splitting this into four different devices is going to
complicate things because you'd have to coordinate between all four as
to which period is currently set.
> > +struct sifive_pwm_device {
> > + struct pwm_chip chip;
> > + struct notifier_block notifier;
> > + struct clk *clk;
> > + void __iomem *regs;
> > + unsigned int approx_period;
> > + unsigned int real_period;
> > +};
>
> I'd call this pwm_sifive_ddata. The prefix because the driver is called
> pwm-sifive and ddata because this is driver data and not a device.
I don't think there's a need to have an extra suffix. Just call this
sifive_pwm or pwm_sifive. There's no ambiguity in that name and it's
short and crisp.
> > + if (state->enabled)
> > + sifive_pwm_get_state(chip, dev, state);
>
> @Thierry: Should we bless this correction of state?
I'm not sure I understand why this correction is necessary. Is it okay
to request a state to be applied and when we're not able to set that
state we just set anything as close as possible? Sounds a bit risky to
me. What if somebody wants to use this in a case where precision
matters?
Now, if you're saying that there can't be such cases and we should
support this, then why restrict the state correction to when the PWM is
enabled? What's wrong with correcting it in either case?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-21 11:30 ` Thierry Reding
0 siblings, 0 replies; 44+ messages in thread
From: Thierry Reding @ 2019-01-21 11:30 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: 4236 bytes --]
On Tue, Jan 15, 2019 at 11:00:46PM +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> >
> > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 257 insertions(+)
> > create mode 100644 drivers/pwm/pwm-sifive.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index a8f47df..3bcaf6a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-samsung.
> >
> > +config PWM_SIFIVE
> > + tristate "SiFive PWM support"
> > + depends on OF
> > + depends on COMMON_CLK
>
> I'd say add:
>
> depends on MACH_SIFIVE || COMPILE_TEST
>
> (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
>
> > + 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..7fee809
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,246 @@
> > +// 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
>
> I wonder if such an instance should be only a single PWM instead of
> four. Then you were more flexible with the period lengths (using
> pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
I thought this IP only allowed a single period for all PWM channels in
the IP. If so, splitting this into four different devices is going to
complicate things because you'd have to coordinate between all four as
to which period is currently set.
> > +struct sifive_pwm_device {
> > + struct pwm_chip chip;
> > + struct notifier_block notifier;
> > + struct clk *clk;
> > + void __iomem *regs;
> > + unsigned int approx_period;
> > + unsigned int real_period;
> > +};
>
> I'd call this pwm_sifive_ddata. The prefix because the driver is called
> pwm-sifive and ddata because this is driver data and not a device.
I don't think there's a need to have an extra suffix. Just call this
sifive_pwm or pwm_sifive. There's no ambiguity in that name and it's
short and crisp.
> > + if (state->enabled)
> > + sifive_pwm_get_state(chip, dev, state);
>
> @Thierry: Should we bless this correction of state?
I'm not sure I understand why this correction is necessary. Is it okay
to request a state to be applied and when we're not able to set that
state we just set anything as close as possible? Sounds a bit risky to
me. What if somebody wants to use this in a case where precision
matters?
Now, if you're saying that there can't be such cases and we should
support this, then why restrict the state correction to when the PWM is
enabled? What's wrong with correcting it in either case?
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] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-17 8:19 ` Uwe Kleine-König
@ 2019-01-21 11:54 ` Thierry Reding
-1 siblings, 0 replies; 44+ messages in thread
From: Thierry Reding @ 2019-01-21 11:54 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Paul Walmsley, Yash Shah, Palmer Dabbelt, linux-pwm, linux-riscv,
robh+dt, mark.rutland, devicetree, linux-kernel, Sachin Ghadi
[-- Attachment #1: Type: text/plain, Size: 5188 bytes --]
On Thu, Jan 17, 2019 at 09:19:56AM +0100, Uwe Kleine-König wrote:
> Hello Paul,
>
> On Wed, Jan 16, 2019 at 11:29:35AM -0800, Paul Walmsley wrote:
> > COMPILE_TEST made slightly more sense before the broad availability of
> > open-source soft cores, SoC integration, and IP; and before powerful,
> > inexpensive FPGAs and SoCs with FPGA fabrics were common.
> >
> > Even back then, COMPILE_TEST was starting to look questionable. IP blocks
> > from CPU- and SoC vendor-independent libraries, like DesignWare and the
> > Cadence IP libraries, were integrated on SoCs across varying CPU
> > architectures. (Fortunately, looking at the tree, subsystem maintainers
> > with DesignWare drivers seem to have largely avoided adding architecture
> > or SoC-specific Kconfig restrictions there - and thus have also avoided
> > the need for COMPILE_TEST.) If an unnecessary architecture Kconfig
> > dependency was added for a leaf driver, that Kconfig line would either
> > need to be patched out by hand, or if present, COMPILE_TEST would need to
> > be enabled.
> >
> > This was less of a problem then. There were very few FPGA Linux users,
> > and they were mostly working on internal proprietary projects. FPGAs that
> > could run Linux at a reasonable speed, including MMUs and FPUs, were
> > expensive or were missing good tool support. So most FPGA Linux
> > development was restricted to ASIC vendors - the Samsungs, Qualcomms,
> > NVIDIAs of the world - for prototyping, and those FPGA platforms never
> > made it outside those companies.
> >
> > The situation is different now. The major FPGA vendors have inexpensive
> > FPGA SoCs with full-featured CPU hard macros. The development boards can
> > be quite affordable (< USD 300 for the Xilinx Ultra96). There are also
> > now open-source SoC HDL implementations - including MMUs, FPUs, and
> > peripherals like PWM and UART - for the conventional FPGA market. These
> > can run at mid-1990's clock rates - slow by modern standards but still
> > quite usable.
>
> In my eyes it's better to make a driver not possible to enable out of
> the box than offering to enable it while it most probably won't be used.
This might sound like a good idea in general, but it's also something
that is pretty much impossible to do. There's just no heuristic that
would allow you to determine with a sufficient degree of probability
that a driver won't be needed. Most of the PCI drivers that are
installed on my workstation aren't used, and I would venture to say
there are a lot of drivers that aren't used in, say, 95% of
installations. That doesn't mean that we should somehow make these
drivers difficult to install, or require someone to patch the kernel
to build them.
> People who configure their own kernel and distribution kernel
> maintainers will thank you. (Well ok, they won't notice, so they won't
> actually tell you, but anyhow.) I'm a member of the Debian kernel team
> and I see how many config items there are that are not explicitly
> handled for the various different kernel images. If they were restricted
> using COMPILE_TEST to just be possible to enable on machines where it is
> known (or at least likely) to make sense that would help. Also when I do
> a kernel version bump for a machine with a tailored kernel running (say)
> on an ARM/i.MX SoC, I could more easily be careful about the relevant
> changes when doing oldconfig if I were not asked about a whole bunch of
> drivers that are sure to be irrelevant.
I think the important thing here is that the driver is "default n". If
you're a developer and build your own kernels, you're pretty likely to
know already if a new kernel that you're installing contains that new
driver that you've been working on or waiting for. In that case you can
easily just enable it manually rather than go through make oldconfig and
wait for it to show up.
As for distribution kernel maintainers, are they really still building a
lot of different kernels? If so, it sounds like most of the multi-
platform efforts have been in vain. I would imagine that if, as a
distribution kernel team, you'd want to carefully evaluate for every
driver whether or not you'd want to include it. I would also imagine
that you'd want to provide your users with the most flexible kernel
possible, so that if they do have a system with an FPGA that you'd make
it possible for them to use pwm-sifive if they choose to synthesize it.
If you really want to create custom builds tailored to a single chip or
board, it's going to be a fair amount of work anyway. I've occasionally
done that in the past and usually just resorted to starting from an
allnoconfig configuration and then working my way up from there.
As for daily work as a developer, when I transition between kernel
versions, or from one linux-next to another, I typically end up doing
the manual equivalent of:
$ yes '' | make oldconfig
if I know that I'm not interested in any new features. But I also often
just look at what's new because it's interesting to see what's been
going on elsewhere.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-21 11:54 ` Thierry Reding
0 siblings, 0 replies; 44+ messages in thread
From: Thierry Reding @ 2019-01-21 11:54 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: 5188 bytes --]
On Thu, Jan 17, 2019 at 09:19:56AM +0100, Uwe Kleine-König wrote:
> Hello Paul,
>
> On Wed, Jan 16, 2019 at 11:29:35AM -0800, Paul Walmsley wrote:
> > COMPILE_TEST made slightly more sense before the broad availability of
> > open-source soft cores, SoC integration, and IP; and before powerful,
> > inexpensive FPGAs and SoCs with FPGA fabrics were common.
> >
> > Even back then, COMPILE_TEST was starting to look questionable. IP blocks
> > from CPU- and SoC vendor-independent libraries, like DesignWare and the
> > Cadence IP libraries, were integrated on SoCs across varying CPU
> > architectures. (Fortunately, looking at the tree, subsystem maintainers
> > with DesignWare drivers seem to have largely avoided adding architecture
> > or SoC-specific Kconfig restrictions there - and thus have also avoided
> > the need for COMPILE_TEST.) If an unnecessary architecture Kconfig
> > dependency was added for a leaf driver, that Kconfig line would either
> > need to be patched out by hand, or if present, COMPILE_TEST would need to
> > be enabled.
> >
> > This was less of a problem then. There were very few FPGA Linux users,
> > and they were mostly working on internal proprietary projects. FPGAs that
> > could run Linux at a reasonable speed, including MMUs and FPUs, were
> > expensive or were missing good tool support. So most FPGA Linux
> > development was restricted to ASIC vendors - the Samsungs, Qualcomms,
> > NVIDIAs of the world - for prototyping, and those FPGA platforms never
> > made it outside those companies.
> >
> > The situation is different now. The major FPGA vendors have inexpensive
> > FPGA SoCs with full-featured CPU hard macros. The development boards can
> > be quite affordable (< USD 300 for the Xilinx Ultra96). There are also
> > now open-source SoC HDL implementations - including MMUs, FPUs, and
> > peripherals like PWM and UART - for the conventional FPGA market. These
> > can run at mid-1990's clock rates - slow by modern standards but still
> > quite usable.
>
> In my eyes it's better to make a driver not possible to enable out of
> the box than offering to enable it while it most probably won't be used.
This might sound like a good idea in general, but it's also something
that is pretty much impossible to do. There's just no heuristic that
would allow you to determine with a sufficient degree of probability
that a driver won't be needed. Most of the PCI drivers that are
installed on my workstation aren't used, and I would venture to say
there are a lot of drivers that aren't used in, say, 95% of
installations. That doesn't mean that we should somehow make these
drivers difficult to install, or require someone to patch the kernel
to build them.
> People who configure their own kernel and distribution kernel
> maintainers will thank you. (Well ok, they won't notice, so they won't
> actually tell you, but anyhow.) I'm a member of the Debian kernel team
> and I see how many config items there are that are not explicitly
> handled for the various different kernel images. If they were restricted
> using COMPILE_TEST to just be possible to enable on machines where it is
> known (or at least likely) to make sense that would help. Also when I do
> a kernel version bump for a machine with a tailored kernel running (say)
> on an ARM/i.MX SoC, I could more easily be careful about the relevant
> changes when doing oldconfig if I were not asked about a whole bunch of
> drivers that are sure to be irrelevant.
I think the important thing here is that the driver is "default n". If
you're a developer and build your own kernels, you're pretty likely to
know already if a new kernel that you're installing contains that new
driver that you've been working on or waiting for. In that case you can
easily just enable it manually rather than go through make oldconfig and
wait for it to show up.
As for distribution kernel maintainers, are they really still building a
lot of different kernels? If so, it sounds like most of the multi-
platform efforts have been in vain. I would imagine that if, as a
distribution kernel team, you'd want to carefully evaluate for every
driver whether or not you'd want to include it. I would also imagine
that you'd want to provide your users with the most flexible kernel
possible, so that if they do have a system with an FPGA that you'd make
it possible for them to use pwm-sifive if they choose to synthesize it.
If you really want to create custom builds tailored to a single chip or
board, it's going to be a fair amount of work anyway. I've occasionally
done that in the past and usually just resorted to starting from an
allnoconfig configuration and then working my way up from there.
As for daily work as a developer, when I transition between kernel
versions, or from one linux-next to another, I typically end up doing
the manual equivalent of:
$ yes '' | make oldconfig
if I know that I'm not interested in any new features. But I also often
just look at what's new because it's interesting to see what's been
going on elsewhere.
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] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-21 11:30 ` Thierry Reding
(?)
@ 2019-01-21 13:23 ` Uwe Kleine-König
-1 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-21 13:23 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 Mon, Jan 21, 2019 at 12:30:39PM +0100, Thierry Reding wrote:
> On Tue, Jan 15, 2019 at 11:00:46PM +0100, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> > >
> > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > [Atish: Various fixes and code cleanup]
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > ---
> > > drivers/pwm/Kconfig | 10 ++
> > > drivers/pwm/Makefile | 1 +
> > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 257 insertions(+)
> > > create mode 100644 drivers/pwm/pwm-sifive.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index a8f47df..3bcaf6a 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > To compile this driver as a module, choose M here: the module
> > > will be called pwm-samsung.
> > >
> > > +config PWM_SIFIVE
> > > + tristate "SiFive PWM support"
> > > + depends on OF
> > > + depends on COMMON_CLK
> >
> > I'd say add:
> >
> > depends on MACH_SIFIVE || COMPILE_TEST
> >
> > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> >
> > > + 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..7fee809
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-sifive.c
> > > @@ -0,0 +1,246 @@
> > > +// 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
> >
> > I wonder if such an instance should be only a single PWM instead of
> > four. Then you were more flexible with the period lengths (using
> > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
>
> I thought this IP only allowed a single period for all PWM channels in
> the IP. If so, splitting this into four different devices is going to
> complicate things because you'd have to coordinate between all four as
> to which period is currently set.
Take a look at the above link, figure 6 is depicting how this IP works
(page 92 of the pdf). If you have pwmzerocmp=0 (and pwmcmpXgang=0) the
four outputs (pwmcmpXgpio) are independant and the counter only resets
on overflow of pwms. Then you have 4 outputs that can have their
duty_cycle (but not period) set individually. So period is restricted to
a count of clk cycles that is a power of two.
With pwmzerocmp=1, pwmcmp0 resets the counter with the following
effects:
- pwmcmp0gpio is always 0 (bad?)
- more finegrained control over the period length as the restriction to
powers of two is gone (good)
The other three output can then either be used as 3 PWM outputs with the
more flexible (but identical) period or only pwmcmp1gpio is used as a
single output (my favourite) and with pwmcmp2, pwmcmp3 and pwmcmp2gang=1
the output pwmcmp2gpio can be used as a secondary output to implement
stuff that was called "complementary mode" or "Push-pull mode" by
Claudiu Beznea.
> > > +struct sifive_pwm_device {
> > > + struct pwm_chip chip;
> > > + struct notifier_block notifier;
> > > + struct clk *clk;
> > > + void __iomem *regs;
> > > + unsigned int approx_period;
> > > + unsigned int real_period;
> > > +};
> >
> > I'd call this pwm_sifive_ddata. The prefix because the driver is called
> > pwm-sifive and ddata because this is driver data and not a device.
>
> I don't think there's a need to have an extra suffix. Just call this
> sifive_pwm or pwm_sifive. There's no ambiguity in that name and it's
> short and crisp.
fine for me if "_device" goes away.
> > > + if (state->enabled)
> > > + sifive_pwm_get_state(chip, dev, state);
> >
> > @Thierry: Should we bless this correction of state?
>
> I'm not sure I understand why this correction is necessary. Is it okay
> to request a state to be applied and when we're not able to set that
> state we just set anything as close as possible? Sounds a bit risky to
> me. What if somebody wants to use this in a case where precision
> matters?
There is always rounding involved. Where should we draw a line then?
Consider a parent clk running at 1000000001 Hz. Can you provide a duty
cycle of 1 ps? Of if the clk runs at 500000000 Hz, I can implement even
periods, but not the uneven. Should the driver fail if an uneven period
is requested? What should a user do if setting
.duty_cycle = 55, .period = 100,
fails? Assume the pwm can set only even duty_cycles? Or maybe duty_cycle
and period must be dividable by 3? Or maybe only periods that are a
power of two are possible? To get both, an API that can actually be
worked with and that provides precision, the driver needs to be allowed
to round (preferably in some defined way that is used for all devices)
and we need a function to determine the result without actually
configuring. That's how it works in the clk_* universe. There is
clk_round_rate and clk_set_rate and the explicit condition that
clk_set_rate(clk, somerate)
has the same effect as
clk_set_rate(clk, clk_round_rate(clk, somerate))
and after one of them we have
clk_get_rate(clk) = clk_round_rate(clk, somerate))
> Now, if you're saying that there can't be such cases and we should
> support this, then why restrict the state correction to when the PWM is
> enabled? What's wrong with correcting it in either case?
Either the correction should be done always or never.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-21 13:23 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-21 13:23 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 Mon, Jan 21, 2019 at 12:30:39PM +0100, Thierry Reding wrote:
> On Tue, Jan 15, 2019 at 11:00:46PM +0100, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> > >
> > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > [Atish: Various fixes and code cleanup]
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > ---
> > > drivers/pwm/Kconfig | 10 ++
> > > drivers/pwm/Makefile | 1 +
> > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 257 insertions(+)
> > > create mode 100644 drivers/pwm/pwm-sifive.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index a8f47df..3bcaf6a 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > To compile this driver as a module, choose M here: the module
> > > will be called pwm-samsung.
> > >
> > > +config PWM_SIFIVE
> > > + tristate "SiFive PWM support"
> > > + depends on OF
> > > + depends on COMMON_CLK
> >
> > I'd say add:
> >
> > depends on MACH_SIFIVE || COMPILE_TEST
> >
> > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> >
> > > + 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..7fee809
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-sifive.c
> > > @@ -0,0 +1,246 @@
> > > +// 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
> >
> > I wonder if such an instance should be only a single PWM instead of
> > four. Then you were more flexible with the period lengths (using
> > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
>
> I thought this IP only allowed a single period for all PWM channels in
> the IP. If so, splitting this into four different devices is going to
> complicate things because you'd have to coordinate between all four as
> to which period is currently set.
Take a look at the above link, figure 6 is depicting how this IP works
(page 92 of the pdf). If you have pwmzerocmp=0 (and pwmcmpXgang=0) the
four outputs (pwmcmpXgpio) are independant and the counter only resets
on overflow of pwms. Then you have 4 outputs that can have their
duty_cycle (but not period) set individually. So period is restricted to
a count of clk cycles that is a power of two.
With pwmzerocmp=1, pwmcmp0 resets the counter with the following
effects:
- pwmcmp0gpio is always 0 (bad?)
- more finegrained control over the period length as the restriction to
powers of two is gone (good)
The other three output can then either be used as 3 PWM outputs with the
more flexible (but identical) period or only pwmcmp1gpio is used as a
single output (my favourite) and with pwmcmp2, pwmcmp3 and pwmcmp2gang=1
the output pwmcmp2gpio can be used as a secondary output to implement
stuff that was called "complementary mode" or "Push-pull mode" by
Claudiu Beznea.
> > > +struct sifive_pwm_device {
> > > + struct pwm_chip chip;
> > > + struct notifier_block notifier;
> > > + struct clk *clk;
> > > + void __iomem *regs;
> > > + unsigned int approx_period;
> > > + unsigned int real_period;
> > > +};
> >
> > I'd call this pwm_sifive_ddata. The prefix because the driver is called
> > pwm-sifive and ddata because this is driver data and not a device.
>
> I don't think there's a need to have an extra suffix. Just call this
> sifive_pwm or pwm_sifive. There's no ambiguity in that name and it's
> short and crisp.
fine for me if "_device" goes away.
> > > + if (state->enabled)
> > > + sifive_pwm_get_state(chip, dev, state);
> >
> > @Thierry: Should we bless this correction of state?
>
> I'm not sure I understand why this correction is necessary. Is it okay
> to request a state to be applied and when we're not able to set that
> state we just set anything as close as possible? Sounds a bit risky to
> me. What if somebody wants to use this in a case where precision
> matters?
There is always rounding involved. Where should we draw a line then?
Consider a parent clk running at 1000000001 Hz. Can you provide a duty
cycle of 1 ps? Of if the clk runs at 500000000 Hz, I can implement even
periods, but not the uneven. Should the driver fail if an uneven period
is requested? What should a user do if setting
.duty_cycle = 55, .period = 100,
fails? Assume the pwm can set only even duty_cycles? Or maybe duty_cycle
and period must be dividable by 3? Or maybe only periods that are a
power of two are possible? To get both, an API that can actually be
worked with and that provides precision, the driver needs to be allowed
to round (preferably in some defined way that is used for all devices)
and we need a function to determine the result without actually
configuring. That's how it works in the clk_* universe. There is
clk_round_rate and clk_set_rate and the explicit condition that
clk_set_rate(clk, somerate)
has the same effect as
clk_set_rate(clk, clk_round_rate(clk, somerate))
and after one of them we have
clk_get_rate(clk) = clk_round_rate(clk, somerate))
> Now, if you're saying that there can't be such cases and we should
> support this, then why restrict the state correction to when the PWM is
> enabled? What's wrong with correcting it in either case?
Either the correction should be done always or never.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-21 13:23 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-21 13:23 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 Mon, Jan 21, 2019 at 12:30:39PM +0100, Thierry Reding wrote:
> On Tue, Jan 15, 2019 at 11:00:46PM +0100, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> > >
> > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > [Atish: Various fixes and code cleanup]
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > ---
> > > drivers/pwm/Kconfig | 10 ++
> > > drivers/pwm/Makefile | 1 +
> > > drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 257 insertions(+)
> > > create mode 100644 drivers/pwm/pwm-sifive.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index a8f47df..3bcaf6a 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > > To compile this driver as a module, choose M here: the module
> > > will be called pwm-samsung.
> > >
> > > +config PWM_SIFIVE
> > > + tristate "SiFive PWM support"
> > > + depends on OF
> > > + depends on COMMON_CLK
> >
> > I'd say add:
> >
> > depends on MACH_SIFIVE || COMPILE_TEST
> >
> > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> >
> > > + 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..7fee809
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-sifive.c
> > > @@ -0,0 +1,246 @@
> > > +// 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
> >
> > I wonder if such an instance should be only a single PWM instead of
> > four. Then you were more flexible with the period lengths (using
> > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
>
> I thought this IP only allowed a single period for all PWM channels in
> the IP. If so, splitting this into four different devices is going to
> complicate things because you'd have to coordinate between all four as
> to which period is currently set.
Take a look at the above link, figure 6 is depicting how this IP works
(page 92 of the pdf). If you have pwmzerocmp=0 (and pwmcmpXgang=0) the
four outputs (pwmcmpXgpio) are independant and the counter only resets
on overflow of pwms. Then you have 4 outputs that can have their
duty_cycle (but not period) set individually. So period is restricted to
a count of clk cycles that is a power of two.
With pwmzerocmp=1, pwmcmp0 resets the counter with the following
effects:
- pwmcmp0gpio is always 0 (bad?)
- more finegrained control over the period length as the restriction to
powers of two is gone (good)
The other three output can then either be used as 3 PWM outputs with the
more flexible (but identical) period or only pwmcmp1gpio is used as a
single output (my favourite) and with pwmcmp2, pwmcmp3 and pwmcmp2gang=1
the output pwmcmp2gpio can be used as a secondary output to implement
stuff that was called "complementary mode" or "Push-pull mode" by
Claudiu Beznea.
> > > +struct sifive_pwm_device {
> > > + struct pwm_chip chip;
> > > + struct notifier_block notifier;
> > > + struct clk *clk;
> > > + void __iomem *regs;
> > > + unsigned int approx_period;
> > > + unsigned int real_period;
> > > +};
> >
> > I'd call this pwm_sifive_ddata. The prefix because the driver is called
> > pwm-sifive and ddata because this is driver data and not a device.
>
> I don't think there's a need to have an extra suffix. Just call this
> sifive_pwm or pwm_sifive. There's no ambiguity in that name and it's
> short and crisp.
fine for me if "_device" goes away.
> > > + if (state->enabled)
> > > + sifive_pwm_get_state(chip, dev, state);
> >
> > @Thierry: Should we bless this correction of state?
>
> I'm not sure I understand why this correction is necessary. Is it okay
> to request a state to be applied and when we're not able to set that
> state we just set anything as close as possible? Sounds a bit risky to
> me. What if somebody wants to use this in a case where precision
> matters?
There is always rounding involved. Where should we draw a line then?
Consider a parent clk running at 1000000001 Hz. Can you provide a duty
cycle of 1 ps? Of if the clk runs at 500000000 Hz, I can implement even
periods, but not the uneven. Should the driver fail if an uneven period
is requested? What should a user do if setting
.duty_cycle = 55, .period = 100,
fails? Assume the pwm can set only even duty_cycles? Or maybe duty_cycle
and period must be dividable by 3? Or maybe only periods that are a
power of two are possible? To get both, an API that can actually be
worked with and that provides precision, the driver needs to be allowed
to round (preferably in some defined way that is used for all devices)
and we need a function to determine the result without actually
configuring. That's how it works in the clk_* universe. There is
clk_round_rate and clk_set_rate and the explicit condition that
clk_set_rate(clk, somerate)
has the same effect as
clk_set_rate(clk, clk_round_rate(clk, somerate))
and after one of them we have
clk_get_rate(clk) = clk_round_rate(clk, somerate))
> Now, if you're saying that there can't be such cases and we should
> support this, then why restrict the state correction to when the PWM is
> enabled? What's wrong with correcting it in either case?
Either the correction should be done always or never.
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] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
2019-01-21 11:54 ` Thierry Reding
@ 2019-01-21 15:11 ` Uwe Kleine-König
-1 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-21 15:11 UTC (permalink / raw)
To: Thierry Reding
Cc: Paul Walmsley, Yash Shah, Palmer Dabbelt, linux-pwm, linux-riscv,
robh+dt, mark.rutland, devicetree, linux-kernel, Sachin Ghadi,
kernel
Hello Thierry,
On Mon, Jan 21, 2019 at 12:54:55PM +0100, Thierry Reding wrote:
> On Thu, Jan 17, 2019 at 09:19:56AM +0100, Uwe Kleine-König wrote:
> > On Wed, Jan 16, 2019 at 11:29:35AM -0800, Paul Walmsley wrote:
> > > COMPILE_TEST made slightly more sense before the broad availability of
> > > open-source soft cores, SoC integration, and IP; and before powerful,
> > > inexpensive FPGAs and SoCs with FPGA fabrics were common.
> > >
> > > Even back then, COMPILE_TEST was starting to look questionable. IP blocks
> > > from CPU- and SoC vendor-independent libraries, like DesignWare and the
> > > Cadence IP libraries, were integrated on SoCs across varying CPU
> > > architectures. (Fortunately, looking at the tree, subsystem maintainers
> > > with DesignWare drivers seem to have largely avoided adding architecture
> > > or SoC-specific Kconfig restrictions there - and thus have also avoided
> > > the need for COMPILE_TEST.) If an unnecessary architecture Kconfig
> > > dependency was added for a leaf driver, that Kconfig line would either
> > > need to be patched out by hand, or if present, COMPILE_TEST would need to
> > > be enabled.
> > >
> > > This was less of a problem then. There were very few FPGA Linux users,
> > > and they were mostly working on internal proprietary projects. FPGAs that
> > > could run Linux at a reasonable speed, including MMUs and FPUs, were
> > > expensive or were missing good tool support. So most FPGA Linux
> > > development was restricted to ASIC vendors - the Samsungs, Qualcomms,
> > > NVIDIAs of the world - for prototyping, and those FPGA platforms never
> > > made it outside those companies.
> > >
> > > The situation is different now. The major FPGA vendors have inexpensive
> > > FPGA SoCs with full-featured CPU hard macros. The development boards can
> > > be quite affordable (< USD 300 for the Xilinx Ultra96). There are also
> > > now open-source SoC HDL implementations - including MMUs, FPUs, and
> > > peripherals like PWM and UART - for the conventional FPGA market. These
> > > can run at mid-1990's clock rates - slow by modern standards but still
> > > quite usable.
> >
> > In my eyes it's better to make a driver not possible to enable out of
> > the box than offering to enable it while it most probably won't be used.
>
> This might sound like a good idea in general, but it's also something
> that is pretty much impossible to do. There's just no heuristic that
> would allow you to determine with a sufficient degree of probability
> that a driver won't be needed. Most of the PCI drivers that are
> installed on my workstation aren't used, and I would venture to say
> there are a lot of drivers that aren't used in, say, 95% of
> installations. That doesn't mean that we should somehow make these
> drivers difficult to install, or require someone to patch the kernel
> to build them.
If there is a PCI card that can be plugged into your machine, the
corresponding driver should be selectable for the matching kernel.
The case here is different though. We're talking about a piece of
hardware that up to now only exists in a riscv machine (IIUC). Yes, I'm
aware that the design is publicly available, still I think this driver
should not be available for a person configuring a kernel for their x86
machine. When you (or someone else) comes around claiming that they have
a real use for this driver on a non-riscv architecture I support
expanding the dependency accordingly.
What I want to make aware of is that being able to enable a driver comes
at a (small) cost. Using a too broad dependency is quite usual because
the person who introduces a given symbol usually doesn't have to think
long if it should be enabled for a given kernel config or not; so it's
"only" other people's time that is wasted.
> > People who configure their own kernel and distribution kernel
> > maintainers will thank you. (Well ok, they won't notice, so they won't
> > actually tell you, but anyhow.) I'm a member of the Debian kernel team
> > and I see how many config items there are that are not explicitly
> > handled for the various different kernel images. If they were restricted
> > using COMPILE_TEST to just be possible to enable on machines where it is
> > known (or at least likely) to make sense that would help. Also when I do
> > a kernel version bump for a machine with a tailored kernel running (say)
> > on an ARM/i.MX SoC, I could more easily be careful about the relevant
> > changes when doing oldconfig if I were not asked about a whole bunch of
> > drivers that are sure to be irrelevant.
>
> I think the important thing here is that the driver is "default n". If
> you're a developer and build your own kernels, you're pretty likely to
> know already if a new kernel that you're installing contains that new
> driver that you've been working on or waiting for.
If you are a developer waiting for your driver you also would not miss
it because it was only selectable for riscv but you're the first who
synthesized it for an arm machine. So there is only little damage.
> In that case you can easily just enable it manually rather than go
> through make oldconfig and wait for it to show up.
>
> As for distribution kernel maintainers, are they really still building a
> lot of different kernels?
In the Debian kernel there are as of now 39 kernel images. Some of them
only differ by stuff that is irrelevant for driver selection (like rt).
But apart from these there is still mostly only a single image available
for a given machine because multi-platform efforts are not good enough
to allow cross-architecture kernels. Or kernels that can handle both big
and little endian.
> If so, it sounds like most of the multi-platform efforts have been in
> vain. I would imagine that if, as a distribution kernel team, you'd
> want to carefully evaluate for every driver whether or not you'd want
> to include it.
The Debian distro kernel I'm running has a .config file with 7317 config
items (grep CONFIG /boot/config-$(uname -r) | wc -l). 1922 of them are
disabled (grep is\ not\ set /boot/config-$(uname -r) | wc -l). If you
find the time to only go through the 1922 options that are disabled for
this amd64 kernel, tell me, I provide you the config file.
> I would also imagine that you'd want to provide your users with the
> most flexible kernel possible, so that if they do have a system with
> an FPGA that you'd make it possible for them to use pwm-sifive if they
> choose to synthesize it.
I would today probably choose to not enable pwm-sifive for Debian on a
non-riscv arch kernel because nobody told to have a use for it and the
cost of enabling the driver is that it takes hard disk space for several
thousand machines without any use.
And given that we here have the knowledge that up to now PWM_SIFIVE=[ym]
is only usable on riscv, I think we should put that into the
condition that makes the driver selectable. It's not hard for a distro
maintainer to find the same information; say it takes 5 minutes (that
works here because the driver under discussion has a link to the
reference manual in the header). Multiply that by the count of drivers.
> If you really want to create custom builds tailored to a single chip or
> board, it's going to be a fair amount of work anyway.
Ah right, this is a hard job, so it doesn't make a difference when we
make it a little bit harder?
> I've occasionally done that in the past and usually just resorted to
> starting from an allnoconfig configuration and then working my way up
> from there.
>
> As for daily work as a developer, when I transition between kernel
> versions, or from one linux-next to another, I typically end up doing
> the manual equivalent of:
>
> $ yes '' | make oldconfig
I usually start with $(make oldconfig) without the yes part. But when I
get 10th question in a row that is completely irrelevant for my target
machine because it doesn't have the graphics core that is found only on
Samsung ARM SoCs or the nand controllers that can only be found on some
powerpc machines I'm annoyed and I press and hold the Enter key.
I'm well aware that I'm missing some interesting stuff then, though,
which is sad.
> if I know that I'm not interested in any new features. But I also often
> just look at what's new because it's interesting to see what's been
> going on elsewhere.
The kernel config as a way to see what is going on elsewhere is a use
case that is broken by my preferred approach. Be warned that you already
miss stuff today if you only look there.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
@ 2019-01-21 15:11 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2019-01-21 15:11 UTC (permalink / raw)
To: Thierry Reding
Cc: mark.rutland, linux-pwm, devicetree, Palmer Dabbelt,
linux-kernel, Sachin Ghadi, Yash Shah, robh+dt, kernel,
Paul Walmsley, linux-riscv
Hello Thierry,
On Mon, Jan 21, 2019 at 12:54:55PM +0100, Thierry Reding wrote:
> On Thu, Jan 17, 2019 at 09:19:56AM +0100, Uwe Kleine-König wrote:
> > On Wed, Jan 16, 2019 at 11:29:35AM -0800, Paul Walmsley wrote:
> > > COMPILE_TEST made slightly more sense before the broad availability of
> > > open-source soft cores, SoC integration, and IP; and before powerful,
> > > inexpensive FPGAs and SoCs with FPGA fabrics were common.
> > >
> > > Even back then, COMPILE_TEST was starting to look questionable. IP blocks
> > > from CPU- and SoC vendor-independent libraries, like DesignWare and the
> > > Cadence IP libraries, were integrated on SoCs across varying CPU
> > > architectures. (Fortunately, looking at the tree, subsystem maintainers
> > > with DesignWare drivers seem to have largely avoided adding architecture
> > > or SoC-specific Kconfig restrictions there - and thus have also avoided
> > > the need for COMPILE_TEST.) If an unnecessary architecture Kconfig
> > > dependency was added for a leaf driver, that Kconfig line would either
> > > need to be patched out by hand, or if present, COMPILE_TEST would need to
> > > be enabled.
> > >
> > > This was less of a problem then. There were very few FPGA Linux users,
> > > and they were mostly working on internal proprietary projects. FPGAs that
> > > could run Linux at a reasonable speed, including MMUs and FPUs, were
> > > expensive or were missing good tool support. So most FPGA Linux
> > > development was restricted to ASIC vendors - the Samsungs, Qualcomms,
> > > NVIDIAs of the world - for prototyping, and those FPGA platforms never
> > > made it outside those companies.
> > >
> > > The situation is different now. The major FPGA vendors have inexpensive
> > > FPGA SoCs with full-featured CPU hard macros. The development boards can
> > > be quite affordable (< USD 300 for the Xilinx Ultra96). There are also
> > > now open-source SoC HDL implementations - including MMUs, FPUs, and
> > > peripherals like PWM and UART - for the conventional FPGA market. These
> > > can run at mid-1990's clock rates - slow by modern standards but still
> > > quite usable.
> >
> > In my eyes it's better to make a driver not possible to enable out of
> > the box than offering to enable it while it most probably won't be used.
>
> This might sound like a good idea in general, but it's also something
> that is pretty much impossible to do. There's just no heuristic that
> would allow you to determine with a sufficient degree of probability
> that a driver won't be needed. Most of the PCI drivers that are
> installed on my workstation aren't used, and I would venture to say
> there are a lot of drivers that aren't used in, say, 95% of
> installations. That doesn't mean that we should somehow make these
> drivers difficult to install, or require someone to patch the kernel
> to build them.
If there is a PCI card that can be plugged into your machine, the
corresponding driver should be selectable for the matching kernel.
The case here is different though. We're talking about a piece of
hardware that up to now only exists in a riscv machine (IIUC). Yes, I'm
aware that the design is publicly available, still I think this driver
should not be available for a person configuring a kernel for their x86
machine. When you (or someone else) comes around claiming that they have
a real use for this driver on a non-riscv architecture I support
expanding the dependency accordingly.
What I want to make aware of is that being able to enable a driver comes
at a (small) cost. Using a too broad dependency is quite usual because
the person who introduces a given symbol usually doesn't have to think
long if it should be enabled for a given kernel config or not; so it's
"only" other people's time that is wasted.
> > People who configure their own kernel and distribution kernel
> > maintainers will thank you. (Well ok, they won't notice, so they won't
> > actually tell you, but anyhow.) I'm a member of the Debian kernel team
> > and I see how many config items there are that are not explicitly
> > handled for the various different kernel images. If they were restricted
> > using COMPILE_TEST to just be possible to enable on machines where it is
> > known (or at least likely) to make sense that would help. Also when I do
> > a kernel version bump for a machine with a tailored kernel running (say)
> > on an ARM/i.MX SoC, I could more easily be careful about the relevant
> > changes when doing oldconfig if I were not asked about a whole bunch of
> > drivers that are sure to be irrelevant.
>
> I think the important thing here is that the driver is "default n". If
> you're a developer and build your own kernels, you're pretty likely to
> know already if a new kernel that you're installing contains that new
> driver that you've been working on or waiting for.
If you are a developer waiting for your driver you also would not miss
it because it was only selectable for riscv but you're the first who
synthesized it for an arm machine. So there is only little damage.
> In that case you can easily just enable it manually rather than go
> through make oldconfig and wait for it to show up.
>
> As for distribution kernel maintainers, are they really still building a
> lot of different kernels?
In the Debian kernel there are as of now 39 kernel images. Some of them
only differ by stuff that is irrelevant for driver selection (like rt).
But apart from these there is still mostly only a single image available
for a given machine because multi-platform efforts are not good enough
to allow cross-architecture kernels. Or kernels that can handle both big
and little endian.
> If so, it sounds like most of the multi-platform efforts have been in
> vain. I would imagine that if, as a distribution kernel team, you'd
> want to carefully evaluate for every driver whether or not you'd want
> to include it.
The Debian distro kernel I'm running has a .config file with 7317 config
items (grep CONFIG /boot/config-$(uname -r) | wc -l). 1922 of them are
disabled (grep is\ not\ set /boot/config-$(uname -r) | wc -l). If you
find the time to only go through the 1922 options that are disabled for
this amd64 kernel, tell me, I provide you the config file.
> I would also imagine that you'd want to provide your users with the
> most flexible kernel possible, so that if they do have a system with
> an FPGA that you'd make it possible for them to use pwm-sifive if they
> choose to synthesize it.
I would today probably choose to not enable pwm-sifive for Debian on a
non-riscv arch kernel because nobody told to have a use for it and the
cost of enabling the driver is that it takes hard disk space for several
thousand machines without any use.
And given that we here have the knowledge that up to now PWM_SIFIVE=[ym]
is only usable on riscv, I think we should put that into the
condition that makes the driver selectable. It's not hard for a distro
maintainer to find the same information; say it takes 5 minutes (that
works here because the driver under discussion has a link to the
reference manual in the header). Multiply that by the count of drivers.
> If you really want to create custom builds tailored to a single chip or
> board, it's going to be a fair amount of work anyway.
Ah right, this is a hard job, so it doesn't make a difference when we
make it a little bit harder?
> I've occasionally done that in the past and usually just resorted to
> starting from an allnoconfig configuration and then working my way up
> from there.
>
> As for daily work as a developer, when I transition between kernel
> versions, or from one linux-next to another, I typically end up doing
> the manual equivalent of:
>
> $ yes '' | make oldconfig
I usually start with $(make oldconfig) without the yes part. But when I
get 10th question in a row that is completely irrelevant for my target
machine because it doesn't have the graphics core that is found only on
Samsung ARM SoCs or the nand controllers that can only be found on some
powerpc machines I'm annoyed and I press and hold the Enter key.
I'm well aware that I'm missing some interesting stuff then, though,
which is sad.
> if I know that I'm not interested in any new features. But I also often
> just look at what's new because it's interesting to see what's been
> going on elsewhere.
The kernel config as a way to see what is going on elsewhere is a use
case that is broken by my preferred approach. Be warned that you already
miss stuff today if you only look there.
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] 44+ messages in thread
end of thread, other threads:[~2019-01-21 15:11 UTC | newest]
Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-11 8:22 [PATCH 0/2] PWM support for HiFive Unleashed Yash Shah
2019-01-11 8:22 ` Yash Shah
2019-01-11 8:22 ` [PATCH 1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller Yash Shah
2019-01-11 8:22 ` Yash Shah
2019-01-15 20:11 ` Uwe Kleine-König
2019-01-15 20:11 ` Uwe Kleine-König
2019-01-16 9:21 ` Yash Shah
2019-01-16 9:21 ` Yash Shah
2019-01-21 11:20 ` Thierry Reding
2019-01-21 11:20 ` Thierry Reding
2019-01-11 8:22 ` [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM Yash Shah
2019-01-11 8:22 ` Yash Shah
2019-01-15 15:27 ` Christoph Hellwig
2019-01-15 15:27 ` Christoph Hellwig
2019-01-15 15:27 ` Christoph Hellwig
2019-01-15 22:00 ` Uwe Kleine-König
2019-01-15 22:00 ` Uwe Kleine-König
2019-01-16 11:10 ` Yash Shah
2019-01-16 11:10 ` Yash Shah
2019-01-16 16:46 ` Uwe Kleine-König
2019-01-16 16:46 ` Uwe Kleine-König
2019-01-16 16:46 ` Uwe Kleine-König
2019-01-16 17:18 ` Paul Walmsley
2019-01-16 17:18 ` Paul Walmsley
2019-01-16 17:28 ` Uwe Kleine-König
2019-01-16 17:28 ` Uwe Kleine-König
2019-01-16 17:28 ` Uwe Kleine-König
2019-01-16 19:29 ` Paul Walmsley
2019-01-16 19:29 ` Paul Walmsley
2019-01-17 8:19 ` Uwe Kleine-König
2019-01-17 8:19 ` Uwe Kleine-König
2019-01-21 11:54 ` Thierry Reding
2019-01-21 11:54 ` Thierry Reding
2019-01-21 15:11 ` Uwe Kleine-König
2019-01-21 15:11 ` Uwe Kleine-König
2019-01-17 6:45 ` Yash Shah
2019-01-17 6:45 ` Yash Shah
2019-01-17 7:28 ` Uwe Kleine-König
2019-01-17 7:28 ` Uwe Kleine-König
2019-01-21 11:30 ` Thierry Reding
2019-01-21 11:30 ` Thierry Reding
2019-01-21 13:23 ` Uwe Kleine-König
2019-01-21 13:23 ` Uwe Kleine-König
2019-01-21 13:23 ` Uwe Kleine-König
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.