Resend this patch series, just adding the missing and new Reviewed-by and
Acked-by infomation of this patch series.
Xiubo Li (4):
pwm: Add Freescale FTM PWM driver support
ARM: dts: vf610: Add Freescale FTM PWM node.
ARM: dts: vf610-twr: Enables FTM PWM device.
Documentation: Add device tree bindings for Freescale FTM PWM.
.../devicetree/bindings/pwm/pwm-fsl-ftm.txt | 34 ++
arch/arm/boot/dts/vf610-twr.dts | 6 +
arch/arm/boot/dts/vf610.dtsi | 13 +
drivers/pwm/Kconfig | 10 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-fsl-ftm.c | 479 +++++++++++++++++++++
6 files changed, 543 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
create mode 100644 drivers/pwm/pwm-fsl-ftm.c
--
1.8.4
This adds devicetree node for VF610, and there are 8 channels
supported.
Signed-off-by: Xiubo Li <[email protected]>
Reviewed-by: Sascha Hauer <[email protected]>
Reviewed-by: Yuan Yao <[email protected]>
---
arch/arm/boot/dts/vf610.dtsi | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi
index d31ce1b..6cce861 100644
--- a/arch/arm/boot/dts/vf610.dtsi
+++ b/arch/arm/boot/dts/vf610.dtsi
@@ -152,6 +152,19 @@
clock-names = "pit";
};
+ pwm0: pwm@40038000 {
+ compatible = "fsl,vf610-ftm-pwm";
+ #pwm-cells = <3>;
+ reg = <0x40038000 0x1000>;
+ clock-names = "ftm_sys", "ftm_ext",
+ "ftm_fix", "ftm_cnt_clk_en";
+ clocks = <&clks VF610_CLK_FTM0>,
+ <&clks VF610_CLK_FTM0_EXT_SEL>,
+ <&clks VF610_CLK_FTM0_FIX_SEL>,
+ <&clks VF610_CLK_FTM0_EXT_FIX_EN>;
+ status = "disabled";
+ };
+
wdog@4003e000 {
compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
reg = <0x4003e000 0x1000>;
--
1.8.4
Signed-off-by: Xiubo Li <[email protected]>
Reviewed-by: Sascha Hauer <[email protected]>
Reviewed-by: Yuan Yao <[email protected]>
---
arch/arm/boot/dts/vf610-twr.dts | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
index c3a3237..d5c6a39 100644
--- a/arch/arm/boot/dts/vf610-twr.dts
+++ b/arch/arm/boot/dts/vf610-twr.dts
@@ -100,6 +100,12 @@
status = "okay";
};
+&pwm0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_pwm0_1>;
+ status = "okay";
+};
+
&uart1 {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_uart1_1>;
--
1.8.4
The FTM PWM device can be found on Vybrid VF610 Tower and
Layerscape LS-1 SoCs.
Signed-off-by: Xiubo Li <[email protected]>
Signed-off-by: Alison Wang <[email protected]>
Signed-off-by: Jingchang Lu <[email protected]>
Reviewed-by: Sascha Hauer <[email protected]>
Reviewed-by: Yuan Yao <[email protected]>
---
drivers/pwm/Kconfig | 10 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-fsl-ftm.c | 479 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 490 insertions(+)
create mode 100644 drivers/pwm/pwm-fsl-ftm.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 6a2a1e0a..9a4c641 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -80,6 +80,16 @@ config PWM_EP93XX
To compile this driver as a module, choose M here: the module
will be called pwm-ep93xx.
+config PWM_FSL_FTM
+ tristate "Freescale FlexTimer Module (FTM) PWM support"
+ depends on OF
+ help
+ Generic FTM PWM framework driver for Freescale VF610 and
+ Layerscape LS-1 SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-fsl-ftm.
+
config PWM_IMX
tristate "i.MX PWM support"
depends on ARCH_MXC
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 1b99cfb..c22905c 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o
obj-$(CONFIG_PWM_ATMEL_TCB) += pwm-atmel-tcb.o
obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o
obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o
+obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o
obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
new file mode 100644
index 0000000..60467b3
--- /dev/null
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -0,0 +1,479 @@
+/*
+ * Freescale FlexTimer Module (FTM) PWM Driver
+ *
+ * Copyright 2012-2013 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+#define FTM_SC 0x00
+#define FTM_SC_CLK_MASK 0x3
+#define FTM_SC_CLK_SHIFT 3
+#define FTM_SC_CLK(c) (((c) + 1) << FTM_SC_CLK_SHIFT)
+#define FTM_SC_PS_MASK 0x7
+#define FTM_SC_PS_SHIFT 0
+
+#define FTM_CNT 0x04
+#define FTM_MOD 0x08
+
+#define FTM_CSC_BASE 0x0C
+#define FTM_CSC_MSB BIT(5)
+#define FTM_CSC_MSA BIT(4)
+#define FTM_CSC_ELSB BIT(3)
+#define FTM_CSC_ELSA BIT(2)
+#define FTM_CSC(_channel) (FTM_CSC_BASE + ((_channel) * 8))
+
+#define FTM_CV_BASE 0x10
+#define FTM_CV(_channel) (FTM_CV_BASE + ((_channel) * 8))
+
+#define FTM_CNTIN 0x4C
+#define FTM_STATUS 0x50
+
+#define FTM_MODE 0x54
+#define FTM_MODE_FTMEN BIT(0)
+#define FTM_MODE_INIT BIT(2)
+#define FTM_MODE_PWMSYNC BIT(3)
+
+#define FTM_SYNC 0x58
+#define FTM_OUTINIT 0x5C
+#define FTM_OUTMASK 0x60
+#define FTM_COMBINE 0x64
+#define FTM_DEADTIME 0x68
+#define FTM_EXTTRIG 0x6C
+#define FTM_POL 0x70
+#define FTM_FMS 0x74
+#define FTM_FILTER 0x78
+#define FTM_FLTCTRL 0x7C
+#define FTM_QDCTRL 0x80
+#define FTM_CONF 0x84
+#define FTM_FLTPOL 0x88
+#define FTM_SYNCONF 0x8C
+#define FTM_INVCTRL 0x90
+#define FTM_SWOCTRL 0x94
+#define FTM_PWMLOAD 0x98
+
+enum fsl_pwm_clk {
+ FSL_PWM_CLK_SYS,
+ FSL_PWM_CLK_FIX,
+ FSL_PWM_CLK_EXT,
+};
+
+struct fsl_pwm_chip {
+ struct pwm_chip chip;
+
+ struct mutex lock;
+
+ struct clk *sys_clk;
+ struct clk *counter_clk;
+ struct clk *counter_clk_en;
+ unsigned int counter_clk_select;
+ unsigned int counter_clk_enable;
+ unsigned int clk_ps;
+
+ void __iomem *base;
+
+ int period_ns;
+};
+
+static inline struct fsl_pwm_chip *to_fsl_chip(struct pwm_chip *chip)
+{
+ return container_of(chip, struct fsl_pwm_chip, chip);
+}
+
+static int fsl_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+
+ return clk_prepare_enable(fpc->sys_clk);
+}
+
+static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+
+ clk_disable_unprepare(fpc->sys_clk);
+}
+
+static int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc,
+ enum fsl_pwm_clk index)
+{
+ unsigned long sys_rate, cnt_rate;
+ unsigned long long ratio;
+
+ sys_rate = clk_get_rate(fpc->sys_clk);
+ if (!sys_rate)
+ return -EINVAL;
+
+ cnt_rate = clk_get_rate(fpc->counter_clk);
+ if (!cnt_rate)
+ return -EINVAL;
+
+ switch (index) {
+ case FSL_PWM_CLK_SYS:
+ fpc->clk_ps = 1;
+ break;
+ case FSL_PWM_CLK_FIX:
+ ratio = 2 * cnt_rate - 1;
+ do_div(ratio, sys_rate);
+ fpc->clk_ps = ratio;
+ break;
+ case FSL_PWM_CLK_EXT:
+ ratio = 4 * cnt_rate - 1;
+ do_div(ratio, sys_rate);
+ fpc->clk_ps = ratio;
+ break;
+ }
+
+ return 0;
+}
+
+static unsigned long fsl_pwm_calculate_cycles(struct fsl_pwm_chip *fpc,
+ unsigned long period_ns)
+{
+ unsigned long long c, c0;
+
+ c = clk_get_rate(fpc->counter_clk);
+ c = c * period_ns;
+ do_div(c, 1000000000UL);
+
+ do {
+ c0 = c;
+ do_div(c0, (1 << fpc->clk_ps));
+ if (c0 <= 0xFFFF)
+ return (unsigned long)c0;
+ } while (++fpc->clk_ps < 8);
+
+ return 0;
+}
+
+static unsigned long fsl_pwm_calculate_period_cycles(struct fsl_pwm_chip *fpc,
+ unsigned long period_ns,
+ enum fsl_pwm_clk index)
+{
+ int ret;
+
+ fpc->counter_clk_select = FTM_SC_CLK(index);
+
+ ret = fsl_pwm_calculate_default_ps(fpc, index);
+ if (ret) {
+ dev_err(fpc->chip.dev, "failed to calculate default "
+ "prescaler: %d\n", ret);
+ return 0;
+ }
+
+ return fsl_pwm_calculate_cycles(fpc, period_ns);
+}
+
+static unsigned long fsl_pwm_calculate_period(struct fsl_pwm_chip *fpc,
+ unsigned long period_ns)
+{
+ struct clk *cnt_clk[3];
+ enum fsl_pwm_clk m0, m1;
+ unsigned long fix_rate, ext_rate, cycles;
+
+ fpc->counter_clk = fpc->sys_clk;
+ cycles = fsl_pwm_calculate_period_cycles(fpc, period_ns,
+ FSL_PWM_CLK_SYS);
+ if (cycles)
+ return cycles;
+
+ cnt_clk[FSL_PWM_CLK_FIX] = devm_clk_get(fpc->chip.dev, "ftm_fix");
+ if (IS_ERR(cnt_clk[FSL_PWM_CLK_FIX]))
+ return PTR_ERR(cnt_clk[FSL_PWM_CLK_FIX]);
+
+ cnt_clk[FSL_PWM_CLK_EXT] = devm_clk_get(fpc->chip.dev, "ftm_ext");
+ if (IS_ERR(cnt_clk[FSL_PWM_CLK_EXT]))
+ return PTR_ERR(cnt_clk[FSL_PWM_CLK_EXT]);
+
+ fpc->counter_clk_en = devm_clk_get(fpc->chip.dev, "ftm_cnt_clk_en");
+ if (IS_ERR(fpc->counter_clk_en))
+ return PTR_ERR(fpc->counter_clk_en);
+
+ fix_rate = clk_get_rate(cnt_clk[FSL_PWM_CLK_FIX]);
+ ext_rate = clk_get_rate(cnt_clk[FSL_PWM_CLK_EXT]);
+
+ if (fix_rate > ext_rate) {
+ m0 = FSL_PWM_CLK_FIX;
+ m1 = FSL_PWM_CLK_EXT;
+ } else {
+ m0 = FSL_PWM_CLK_EXT;
+ m1 = FSL_PWM_CLK_FIX;
+ }
+
+ fpc->counter_clk = cnt_clk[m0];
+ cycles = fsl_pwm_calculate_period_cycles(fpc, period_ns, m0);
+ if (cycles)
+ return cycles;
+
+ fpc->counter_clk = cnt_clk[m1];
+
+ return fsl_pwm_calculate_period_cycles(fpc, period_ns, m0);
+}
+
+static unsigned long fsl_pwm_calculate_duty(struct fsl_pwm_chip *fpc,
+ unsigned long period_ns,
+ unsigned long duty_ns)
+{
+ unsigned long long val, duty;
+
+ val = readl(fpc->base + FTM_MOD);
+ duty = duty_ns * (val + 1);
+ do_div(duty, period_ns);
+
+ return (unsigned long)duty;
+}
+
+static int fsl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+ struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+ u32 val, period, duty;
+
+ mutex_lock(&fpc->lock);
+
+ /*
+ * The Freescale FTM controller supports only a single period for
+ * all PWM channels, therefore incompatible changes need to be
+ * refused.
+ */
+ if (fpc->period_ns && fpc->period_ns != period_ns) {
+ dev_err(fpc->chip.dev,
+ "conflicting period requested for PWM %u\n",
+ pwm->hwpwm);
+ mutex_unlock(&fpc->lock);
+ return -EBUSY;
+ }
+
+ if (!fpc->period_ns && duty_ns) {
+ period = fsl_pwm_calculate_period(fpc, period_ns);
+ if (!period) {
+ dev_err(fpc->chip.dev, "failed to calculate period\n");
+ mutex_unlock(&fpc->lock);
+ return -EINVAL;
+ }
+
+ val = readl(fpc->base + FTM_SC);
+ val &= ~(FTM_SC_PS_MASK << FTM_SC_PS_SHIFT);
+ val |= fpc->clk_ps;
+ writel(val, fpc->base + FTM_SC);
+ writel(period - 1, fpc->base + FTM_MOD);
+
+ fpc->period_ns = period_ns;
+ }
+
+ mutex_unlock(&fpc->lock);
+
+ duty = fsl_pwm_calculate_duty(fpc, period_ns, duty_ns);
+
+ writel(FTM_CSC_MSB | FTM_CSC_ELSB, fpc->base + FTM_CSC(pwm->hwpwm));
+ writel(duty, fpc->base + FTM_CV(pwm->hwpwm));
+
+ return 0;
+}
+
+static int fsl_pwm_set_polarity(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ enum pwm_polarity polarity)
+{
+ struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+ u32 val;
+
+ val = readl(fpc->base + FTM_POL);
+
+ if (polarity == PWM_POLARITY_INVERSED)
+ val |= BIT(pwm->hwpwm);
+ else
+ val &= ~BIT(pwm->hwpwm);
+
+ writel(val, fpc->base + FTM_POL);
+
+ return 0;
+}
+
+static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc)
+{
+ u32 val;
+ int ret;
+
+ if (fpc->counter_clk_enable++)
+ return 0;
+
+ ret = clk_prepare_enable(fpc->counter_clk);
+ if (ret) {
+ fpc->counter_clk_enable--;
+ return ret;
+ }
+
+ ret = clk_prepare_enable(fpc->counter_clk_en);
+ if (ret) {
+ fpc->counter_clk_enable--;
+ return ret;
+ }
+
+ /* select counter clock source */
+ val = readl(fpc->base + FTM_SC);
+ val &= ~(FTM_SC_CLK_MASK << FTM_SC_CLK_SHIFT);
+ val |= fpc->counter_clk_select;
+ writel(val, fpc->base + FTM_SC);
+
+ return 0;
+}
+
+static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+ u32 val;
+ int ret;
+
+ val = readl(fpc->base + FTM_OUTMASK);
+ val &= ~BIT(pwm->hwpwm);
+ writel(val, fpc->base + FTM_OUTMASK);
+
+ mutex_lock(&fpc->lock);
+ ret = fsl_counter_clock_enable(fpc);
+ mutex_unlock(&fpc->lock);
+
+ return ret;
+}
+
+static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc)
+{
+ u32 val;
+
+ if (--fpc->counter_clk_enable)
+ return;
+
+ val = readl(fpc->base + FTM_SC);
+ val &= ~(FTM_SC_CLK_MASK << FTM_SC_CLK_SHIFT);
+ writel(val, fpc->base + FTM_SC);
+
+ clk_disable_unprepare(fpc->counter_clk_en);
+ clk_disable_unprepare(fpc->counter_clk);
+}
+
+static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+ u32 val;
+
+ val = readl(fpc->base + FTM_OUTMASK);
+ val |= BIT(pwm->hwpwm);
+ writel(val, fpc->base + FTM_OUTMASK);
+
+ mutex_lock(&fpc->lock);
+
+ fsl_counter_clock_disable(fpc);
+
+ val = readl(fpc->base + FTM_OUTMASK);
+
+ if ((val & 0xFF) == 0xFF) {
+ fpc->period_ns = 0;
+ fpc->counter_clk_en = NULL;
+ }
+
+ mutex_unlock(&fpc->lock);
+}
+
+static const struct pwm_ops fsl_pwm_ops = {
+ .request = fsl_pwm_request,
+ .free = fsl_pwm_free,
+ .config = fsl_pwm_config,
+ .set_polarity = fsl_pwm_set_polarity,
+ .enable = fsl_pwm_enable,
+ .disable = fsl_pwm_disable,
+ .owner = THIS_MODULE,
+};
+
+static int fsl_pwm_probe(struct platform_device *pdev)
+{
+ struct fsl_pwm_chip *fpc;
+ struct resource *res;
+ int ret;
+
+ fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL);
+ if (!fpc)
+ return -ENOMEM;
+
+ mutex_init(&fpc->lock);
+
+ fpc->chip.dev = &pdev->dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ fpc->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(fpc->base))
+ return PTR_ERR(fpc->base);
+
+ fpc->sys_clk = devm_clk_get(&pdev->dev, "ftm_sys");
+ if (IS_ERR(fpc->sys_clk)) {
+ dev_err(&pdev->dev,
+ "failed to get \"ftm_sys\" clock\n");
+ return PTR_ERR(fpc->sys_clk);
+ }
+
+ ret = clk_prepare_enable(fpc->sys_clk);
+ if (ret)
+ return ret;
+
+ writel(0x00, fpc->base + FTM_CNTIN);
+ writel(0x00, fpc->base + FTM_OUTINIT);
+ writel(0xFF, fpc->base + FTM_OUTMASK);
+ clk_disable_unprepare(fpc->sys_clk);
+
+ fpc->chip.ops = &fsl_pwm_ops;
+ fpc->chip.of_xlate = of_pwm_xlate_with_flags;
+ fpc->chip.of_pwm_n_cells = 3;
+ fpc->chip.base = -1;
+ fpc->chip.npwm = 8;
+
+ ret = pwmchip_add(&fpc->chip);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to add PWM chip : %d\n", ret);
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, fpc);
+
+ return 0;
+}
+
+static int fsl_pwm_remove(struct platform_device *pdev)
+{
+ struct fsl_pwm_chip *fpc = platform_get_drvdata(pdev);
+
+ return pwmchip_remove(&fpc->chip);
+}
+
+static const struct of_device_id fsl_pwm_dt_ids[] = {
+ { .compatible = "fsl,vf610-ftm-pwm", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, fsl_pwm_dt_ids);
+
+static struct platform_driver fsl_pwm_driver = {
+ .driver = {
+ .name = "fsl-ftm-pwm",
+ .of_match_table = fsl_pwm_dt_ids,
+ },
+ .probe = fsl_pwm_probe,
+ .remove = fsl_pwm_remove,
+};
+module_platform_driver(fsl_pwm_driver);
+
+MODULE_DESCRIPTION("Freescale FlexTimer Module PWM Driver");
+MODULE_AUTHOR("Xiubo Li <[email protected]>");
+MODULE_ALIAS("platform:fsl-ftm-pwm");
+MODULE_LICENSE("GPL");
--
1.8.4
This adds the binding documentation for Freescale FlexTimer Module
(FTM) PWM driver under Documentation/devicetree/bindings/pwm/.
Signed-off-by: Xiubo Li <[email protected]>
Reviewed-by: Sascha Hauer <[email protected]>
Reviewed-by: Yuan Yao <[email protected]>
Acked-by: Kumar Gala <[email protected]>
---
.../devicetree/bindings/pwm/pwm-fsl-ftm.txt | 34 ++++++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
diff --git a/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
new file mode 100644
index 0000000..1c310df
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
@@ -0,0 +1,34 @@
+Freescale FlexTimer Module (FTM) PWM controller
+
+Required properties:
+- compatible: Should be "fsl,vf610-ftm-pwm".
+- reg: Physical base address and length of the controller's registers
+- #pwm-cells: Should be 3. See pwm.txt in this directory for a description of
+ the cells format.
+- clock-names : Should include the following module clock source entries:
+ "ftm_sys" (module clock, also can be used as counter clock),
+ "ftm_ext" (external counter clock),
+ "ftm_fix" (fixed counter clock),
+ "ftm_cnt_clk_en" (external and fixed counter clock enable/disable).
+- clocks : Must contain a clock specifier for each entry in clock-names,
+ See clock/clock-bindings.txt for details of the property values.
+- pinctrl-names: Must contain a "default" entry.
+- pinctrl-NNN: One property must exist for each entry in pinctrl-names.
+ See pinctrl/pinctrl-bindings.txt for details of the property values.
+
+
+Example:
+
+pwm0: pwm@40038000 {
+ compatible = "fsl,vf610-ftm-pwm";
+ reg = <0x40038000 0x1000>;
+ #pwm-cells = <3>;
+ clock-names = "ftm_sys", "ftm_ext",
+ "ftm_fix", "ftm_cnt_clk_en";
+ clocks = <&clks VF610_CLK_FTM0>,
+ <&clks VF610_CLK_FTM0_EXT_SEL>,
+ <&clks VF610_CLK_FTM0_FIX_SEL>,
+ <&clks VF610_CLK_FTM0_EXT_FIX_EN>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_pwm0_1>;
+};
--
1.8.4
Thierry, Ping ? :)
> -----Original Message-----
> From: Xiubo Li [mailto:[email protected]]
> Sent: Wednesday, February 19, 2014 4:39 PM
> To: [email protected]; [email protected]
> Cc: [email protected]; Xiubo Li-B47053
> Subject: [PATCHv9 Resend 0/4] Add Freescale FTM PWM driver
>
> Resend this patch series, just adding the missing and new Reviewed-by and
> Acked-by infomation of this patch series.
>
>
> Xiubo Li (4):
> pwm: Add Freescale FTM PWM driver support
> ARM: dts: vf610: Add Freescale FTM PWM node.
> ARM: dts: vf610-twr: Enables FTM PWM device.
> Documentation: Add device tree bindings for Freescale FTM PWM.
>
> .../devicetree/bindings/pwm/pwm-fsl-ftm.txt | 34 ++
> arch/arm/boot/dts/vf610-twr.dts | 6 +
> arch/arm/boot/dts/vf610.dtsi | 13 +
> drivers/pwm/Kconfig | 10 +
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-fsl-ftm.c | 479
> +++++++++++++++++++++
> 6 files changed, 543 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> create mode 100644 drivers/pwm/pwm-fsl-ftm.c
>
> --
> 1.8.4
>
Hi,
Sorry for taking so long to get back to you. Things have been quite busy
lately. A few more comments below, but we're getting there.
On Wed, Feb 19, 2014 at 04:38:54PM +0800, Xiubo Li wrote:
[...]
> diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
[...]
> +static unsigned long fsl_pwm_calculate_period(struct fsl_pwm_chip *fpc,
> + unsigned long period_ns)
> +{
> + struct clk *cnt_clk[3];
> + enum fsl_pwm_clk m0, m1;
> + unsigned long fix_rate, ext_rate, cycles;
> +
> + fpc->counter_clk = fpc->sys_clk;
> + cycles = fsl_pwm_calculate_period_cycles(fpc, period_ns,
> + FSL_PWM_CLK_SYS);
> + if (cycles)
> + return cycles;
> +
> + cnt_clk[FSL_PWM_CLK_FIX] = devm_clk_get(fpc->chip.dev, "ftm_fix");
> + if (IS_ERR(cnt_clk[FSL_PWM_CLK_FIX]))
> + return PTR_ERR(cnt_clk[FSL_PWM_CLK_FIX]);
> +
> + cnt_clk[FSL_PWM_CLK_EXT] = devm_clk_get(fpc->chip.dev, "ftm_ext");
> + if (IS_ERR(cnt_clk[FSL_PWM_CLK_EXT]))
> + return PTR_ERR(cnt_clk[FSL_PWM_CLK_EXT]);
> +
> + fpc->counter_clk_en = devm_clk_get(fpc->chip.dev, "ftm_cnt_clk_en");
> + if (IS_ERR(fpc->counter_clk_en))
> + return PTR_ERR(fpc->counter_clk_en);
You shouldn't do this. You're obtaining a reference to each of these
clocks whenever pwm_config() is called. And devres will only clean those
up after the driver is unbound. Can't you simply keep a reference to
these within struct fsl_pwm_chip?
> +static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc)
> +{
> + u32 val;
> + int ret;
> +
> + if (fpc->counter_clk_enable++)
This function is always called with the fpc->lock held, so you could
make this much easier by incrementing the .counter_clk_enable field only
at the very end of the function. That way...
> + return 0;
> +
> + ret = clk_prepare_enable(fpc->counter_clk);
> + if (ret) {
> + fpc->counter_clk_enable--;
... this won't be necessary...
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(fpc->counter_clk_en);
> + if (ret) {
> + fpc->counter_clk_enable--;
... and neither will this.
> +static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> + u32 val;
> + int ret;
> +
> + val = readl(fpc->base + FTM_OUTMASK);
> + val &= ~BIT(pwm->hwpwm);
> + writel(val, fpc->base + FTM_OUTMASK);
> +
> + mutex_lock(&fpc->lock);
I think you want to extend the lock to cover the FTM_OUTMASK register
access as well because there could be a race between pwm_enable() and
pwm_disable().
> + ret = fsl_counter_clock_enable(fpc);
> + mutex_unlock(&fpc->lock);
> +
> + return ret;
> +}
Can this function be moved somewhere else so fsl_counter_clock_enable()
and fsl_counter_clock_disable() are grouped together?
> +static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc)
> +{
> + u32 val;
> +
> + if (--fpc->counter_clk_enable)
> + return;
This is going to break. Consider the case where you call pwm_disable()
on a PWM device and fpc->counter_clk_enable == 1. In that case, this
will decrement counter_clk_enable to 0 and proceed with the remainder of
this function.
Now you call pwm_disable() again. The above will decrement again and
cause fpc->counter_clk_enable to wrap around to UINT_MAX.
So I think a more correct implementation would be:
/*
* already disabled, do nothing (perhaps output warning message
* to catch unbalanced calls? )
*/
if (fpc->counter_clk_enable == 0)
return;
/* there are still users, so can't disable yet */
if (--fpc->counter_clk_enable > 0)
return;
/* no users left, disable clock */
> +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> + u32 val;
> +
> + val = readl(fpc->base + FTM_OUTMASK);
> + val |= BIT(pwm->hwpwm);
> + writel(val, fpc->base + FTM_OUTMASK);
> +
> + mutex_lock(&fpc->lock);
This lock should also include the access to FTM_OUTMASK above.
> +static int fsl_pwm_probe(struct platform_device *pdev)
> +{
[...]
> + fpc->sys_clk = devm_clk_get(&pdev->dev, "ftm_sys");
> + if (IS_ERR(fpc->sys_clk)) {
> + dev_err(&pdev->dev,
> + "failed to get \"ftm_sys\" clock\n");
The above easily fits on a single line, no need for the wrapping.
> + return PTR_ERR(fpc->sys_clk);
> + }
> +
> + ret = clk_prepare_enable(fpc->sys_clk);
> + if (ret)
> + return ret;
> +
> + writel(0x00, fpc->base + FTM_CNTIN);
> + writel(0x00, fpc->base + FTM_OUTINIT);
> + writel(0xFF, fpc->base + FTM_OUTMASK);
> + clk_disable_unprepare(fpc->sys_clk);
This looks out of place somehow, perhaps it should be moved off into a
separate function? fsl_pwm_init() perhaps.
Thierry
On Wed, Feb 19, 2014 at 04:38:57PM +0800, Xiubo Li wrote:
> This adds the binding documentation for Freescale FlexTimer Module
> (FTM) PWM driver under Documentation/devicetree/bindings/pwm/.
>
> Signed-off-by: Xiubo Li <[email protected]>
> Reviewed-by: Sascha Hauer <[email protected]>
> Reviewed-by: Yuan Yao <[email protected]>
> Acked-by: Kumar Gala <[email protected]>
> ---
> .../devicetree/bindings/pwm/pwm-fsl-ftm.txt | 34 ++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
[...]
> +- clocks : Must contain a clock specifier for each entry in clock-names,
Nit: A slightly more correct way to say this would be: "Must contain a
phandle and clock specifier...".
Thierry
> > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> [...]
> > +- clocks : Must contain a clock specifier for each entry in clock-names,
>
> Nit: A slightly more correct way to say this would be: "Must contain a
> phandle and clock specifier...".
>
Yes, I will fix this.
Thanks very much,
--
Best Regards,
Xiubo
Hi Thierry,
Thanks very much, I will fix them all.
:)
--
Best Regards,
Xiubo
> Sorry for taking so long to get back to you. Things have been quite busy
> lately. A few more comments below, but we're getting there.
>
> On Wed, Feb 19, 2014 at 04:38:54PM +0800, Xiubo Li wrote:
> [...]
> > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> [...]
> > +static unsigned long fsl_pwm_calculate_period(struct fsl_pwm_chip *fpc,
> > + unsigned long period_ns)
> > +{
> > + struct clk *cnt_clk[3];
> > + enum fsl_pwm_clk m0, m1;
> > + unsigned long fix_rate, ext_rate, cycles;
> > +
> > + fpc->counter_clk = fpc->sys_clk;
> > + cycles = fsl_pwm_calculate_period_cycles(fpc, period_ns,
> > + FSL_PWM_CLK_SYS);
> > + if (cycles)
> > + return cycles;
> > +
> > + cnt_clk[FSL_PWM_CLK_FIX] = devm_clk_get(fpc->chip.dev, "ftm_fix");
> > + if (IS_ERR(cnt_clk[FSL_PWM_CLK_FIX]))
> > + return PTR_ERR(cnt_clk[FSL_PWM_CLK_FIX]);
> > +
> > + cnt_clk[FSL_PWM_CLK_EXT] = devm_clk_get(fpc->chip.dev, "ftm_ext");
> > + if (IS_ERR(cnt_clk[FSL_PWM_CLK_EXT]))
> > + return PTR_ERR(cnt_clk[FSL_PWM_CLK_EXT]);
> > +
> > + fpc->counter_clk_en = devm_clk_get(fpc->chip.dev, "ftm_cnt_clk_en");
> > + if (IS_ERR(fpc->counter_clk_en))
> > + return PTR_ERR(fpc->counter_clk_en);
>
> You shouldn't do this. You're obtaining a reference to each of these
> clocks whenever pwm_config() is called. And devres will only clean those
> up after the driver is unbound. Can't you simply keep a reference to
> these within struct fsl_pwm_chip?
>
> > +static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc)
> > +{
> > + u32 val;
> > + int ret;
> > +
> > + if (fpc->counter_clk_enable++)
>
> This function is always called with the fpc->lock held, so you could
> make this much easier by incrementing the .counter_clk_enable field only
> at the very end of the function. That way...
>
> > + return 0;
> > +
> > + ret = clk_prepare_enable(fpc->counter_clk);
> > + if (ret) {
> > + fpc->counter_clk_enable--;
>
> ... this won't be necessary...
>
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(fpc->counter_clk_en);
> > + if (ret) {
> > + fpc->counter_clk_enable--;
>
> ... and neither will this.
>
> > +static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> > + u32 val;
> > + int ret;
> > +
> > + val = readl(fpc->base + FTM_OUTMASK);
> > + val &= ~BIT(pwm->hwpwm);
> > + writel(val, fpc->base + FTM_OUTMASK);
> > +
> > + mutex_lock(&fpc->lock);
>
> I think you want to extend the lock to cover the FTM_OUTMASK register
> access as well because there could be a race between pwm_enable() and
> pwm_disable().
>
> > + ret = fsl_counter_clock_enable(fpc);
> > + mutex_unlock(&fpc->lock);
> > +
> > + return ret;
> > +}
>
> Can this function be moved somewhere else so fsl_counter_clock_enable()
> and fsl_counter_clock_disable() are grouped together?
>
> > +static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc)
> > +{
> > + u32 val;
> > +
> > + if (--fpc->counter_clk_enable)
> > + return;
>
> This is going to break. Consider the case where you call pwm_disable()
> on a PWM device and fpc->counter_clk_enable == 1. In that case, this
> will decrement counter_clk_enable to 0 and proceed with the remainder of
> this function.
>
> Now you call pwm_disable() again. The above will decrement again and
> cause fpc->counter_clk_enable to wrap around to UINT_MAX.
>
> So I think a more correct implementation would be:
>
> /*
> * already disabled, do nothing (perhaps output warning message
> * to catch unbalanced calls? )
> */
> if (fpc->counter_clk_enable == 0)
> return;
>
> /* there are still users, so can't disable yet */
> if (--fpc->counter_clk_enable > 0)
> return;
>
> /* no users left, disable clock */
>
> > +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> > + u32 val;
> > +
> > + val = readl(fpc->base + FTM_OUTMASK);
> > + val |= BIT(pwm->hwpwm);
> > + writel(val, fpc->base + FTM_OUTMASK);
> > +
> > + mutex_lock(&fpc->lock);
>
> This lock should also include the access to FTM_OUTMASK above.
>
> > +static int fsl_pwm_probe(struct platform_device *pdev)
> > +{
> [...]
> > + fpc->sys_clk = devm_clk_get(&pdev->dev, "ftm_sys");
> > + if (IS_ERR(fpc->sys_clk)) {
> > + dev_err(&pdev->dev,
> > + "failed to get \"ftm_sys\" clock\n");
>
> The above easily fits on a single line, no need for the wrapping.
>
> > + return PTR_ERR(fpc->sys_clk);
> > + }
> > +
> > + ret = clk_prepare_enable(fpc->sys_clk);
> > + if (ret)
> > + return ret;
> > +
> > + writel(0x00, fpc->base + FTM_CNTIN);
> > + writel(0x00, fpc->base + FTM_OUTINIT);
> > + writel(0xFF, fpc->base + FTM_OUTMASK);
> > + clk_disable_unprepare(fpc->sys_clk);
>
> This looks out of place somehow, perhaps it should be moved off into a
> separate function? fsl_pwm_init() perhaps.
>
> Thierry