[PATCHv7 0/4] Add Freescale FTM PWM driver.

[PATCHv7 0/4] Add Freescale FTM PWM driver.

[Xiubo Li]

This patch series is the Freescale FTM PWM implementation. And there
are 8 channels most supported by the FTM PWM. This implementation is
only compatible with device tree definition.

This patch series is based on linux-next and has been tested on Vybrid
VF610 Tower board using device tree.


Changes in v7:
- Add big-endian mode support.
- Add FTM mutex lock.
- Add period time check with the current running pwm(s).
- Recode the counter clock source selecting.
- Sort some header files alphabetically, etc.

Changes in v6:
- Remove "fsl,pwm-counter-clk".

Changes in v5:
- Remove active/idle pinctrl stuff.

Changes in v4:
- Check for the result and return an error for devm_kzalloc().
- Move pinmux setting from the SoC file to the board specific file.
- Revise the written mistake of 'ret |= FTMSC_CLKEXT;' --> 'reg |= FTMSC_CLKEXT;'.


Changes in v3:
- Remove "availabe" field.
- Remove "fsl,pwm-avaliable-chs" property.
- ...

Changes in v2:
- Remove PWM CPWM/EPWM feature and sysfs.
- Remove some redundant code.
- Revise some code for more readable.
- Remove "fsl,pwm-clk-ps", "fsl,pwm-number", "fsl,pwm-channels",etc.
- Add "fsl,pwm-avaliable-chs", "fsl,pwm-counter-clk", etc.
- Support 8 channels default in dtsi file.
- Add counter clock source selection.
- Rename some function name, macro name, etc.
- Use PWM's and OF's existing function interfaces.
- Split clk_unprepare_enable to clk_unprepare and clk_enable,etc.
- ...

Added in v1:
- Add Freescale FTM PWM driver support.
- Add Freescale FTM PWM node for VF610.
- Enable Enables FTM PWM device for Vybrid VF610 TOWER.
- Add device tree bindings for Freescale.

[PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Xiubo Li]

The FTM PWM device can be found on Vybrid VF610 Tower and
Layerscape LS-1 SoCs.

Signed-off-by: Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Signed-off-by: Alison Wang <b18965-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Signed-off-by: Jingchang Lu <b35083-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Reviewed-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/pwm/Kconfig       |  10 +
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-fsl-ftm.c | 508 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 519 insertions(+)
 create mode 100644 drivers/pwm/pwm-fsl-ftm.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index eece329..c77c571 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -71,6 +71,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 8b754e4..9029a12 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ 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_IMX)		+= pwm-imx.o
+obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
new file mode 100644
index 0000000..22715e7
--- /dev/null
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -0,0 +1,508 @@
+/*
+ *  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_SYS,
+	FSL_PWM_CLK_FIX,
+	FSL_PWM_CLK_EXT,
+	FSL_PWM_CLK_MAX,
+};
+
+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;
+	int big_endian;
+};
+
+static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
+		const void __iomem *addr)
+{
+	u32 val;
+
+	val = __raw_readl(addr);
+
+	if (likely(fpc->big_endian))
+		val = be32_to_cpu(val);
+	else
+		val = le32_to_cpu(val);
+	rmb();
+
+	return val;
+}
+
+static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc,
+		u32 val, void __iomem *addr)
+{
+	wmb();
+	if (likely(fpc->big_endian))
+		val = cpu_to_be32(val);
+	else
+		val = cpu_to_le32(val);
+
+	__raw_writel(val, addr);
+}
+
+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 inline int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc,
+		int 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;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static inline 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, int index)
+{
+	int ret;
+	unsigned long cycles;
+
+	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 ps.\n");
+		return 0;
+	}
+
+	cycles = fsl_pwm_calculate_cycles(fpc, period_ns);
+
+	return cycles;
+}
+
+static unsigned long fsl_pwm_calculate_period(struct fsl_pwm_chip *fpc,
+		unsigned long period_ns)
+{
+	unsigned long cycles;
+	int m0, m1;
+	unsigned long fix_rate, ext_rate;
+	struct clk *cnt_clk[FSL_PWM_CLK_MAX];
+
+	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];
+	cycles = fsl_pwm_calculate_period_cycles(fpc, period_ns, m0);
+
+	return cycles;
+}
+
+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 = fsl_pwm_readl(fpc, 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)
+{
+	u32 val, period, duty;
+	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+
+	mutex_lock(&fpc->lock);
+	if (fpc->period_ns && fpc->period_ns != period_ns) {
+		dev_err(fpc->chip.dev, "fail to config pwm%d, the period "
+				"time should be the same with the current "
+				"running pwm(s).\n", pwm->hwpwm);
+		mutex_unlock(&fpc->lock);
+		return -EINVAL;
+	}
+
+	if (!fpc->period_ns && duty_ns) {
+		period = fsl_pwm_calculate_period(fpc, period_ns);
+		if (!period) {
+			dev_err(fpc->chip.dev, "fail to calculate the "
+					"period cycles.\n");
+			mutex_unlock(&fpc->lock);
+			return -EINVAL;
+		}
+
+		val = fsl_pwm_readl(fpc, fpc->base + FTM_SC);
+		val &= ~(FTM_SC_PS_MASK << FTM_SC_PS_SHIFT);
+		val |= fpc->clk_ps;
+		fsl_pwm_writel(fpc, val, fpc->base + FTM_SC);
+		fsl_pwm_writel(fpc, 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);
+
+	fsl_pwm_writel(fpc, FTM_CSC_MSB | FTM_CSC_ELSB,
+			fpc->base + FTM_CSC(pwm->hwpwm));
+	fsl_pwm_writel(fpc, 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)
+{
+	u32 val;
+	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+
+	val = fsl_pwm_readl(fpc, fpc->base + FTM_POL);
+	if (polarity == PWM_POLARITY_INVERSED)
+		val |= BIT(pwm->hwpwm);
+	else
+		val &= ~BIT(pwm->hwpwm);
+	fsl_pwm_writel(fpc, val, fpc->base + FTM_POL);
+
+	return 0;
+}
+
+static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc)
+{
+	int ret;
+	u32 val;
+
+	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 = fsl_pwm_readl(fpc, fpc->base + FTM_SC);
+	val &= ~(FTM_SC_CLK_MASK << FTM_SC_CLK_SHIFT);
+	val |= fpc->counter_clk_select;
+	fsl_pwm_writel(fpc, val, fpc->base + FTM_SC);
+
+	return 0;
+}
+
+static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	int ret;
+	u32 val;
+	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+
+	val = fsl_pwm_readl(fpc, fpc->base + FTM_OUTMASK);
+	val &= ~BIT(pwm->hwpwm);
+	fsl_pwm_writel(fpc, val, fpc->base + FTM_OUTMASK);
+
+	mutex_lock(&fpc->lock);
+	ret = fsl_counter_clock_enable(fpc);
+	mutex_unlock(&fpc->lock);
+
+	return ret;
+}
+
+static inline void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc)
+{
+	u32 val;
+
+	if (--fpc->counter_clk_enable)
+		return;
+
+	val = fsl_pwm_readl(fpc, fpc->base + FTM_SC);
+	val &= ~(FTM_SC_CLK_MASK << FTM_SC_CLK_SHIFT);
+	fsl_pwm_writel(fpc, 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)
+{
+	u32 val;
+	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+
+	val = fsl_pwm_readl(fpc, fpc->base + FTM_OUTMASK);
+	val |= BIT(pwm->hwpwm);
+	fsl_pwm_writel(fpc, val, fpc->base + FTM_OUTMASK);
+
+	mutex_lock(&fpc->lock);
+	fsl_counter_clock_disable(fpc);
+
+	val = fsl_pwm_readl(fpc, 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)
+{
+	int ret;
+	struct fsl_pwm_chip *fpc;
+	struct resource *res;
+	struct device_node *np = pdev->dev.of_node;
+
+	fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL);
+	if (!fpc)
+		return -ENOMEM;
+
+	mutex_init(&fpc->lock);
+
+	fpc->chip.dev = &pdev->dev;
+
+	fpc->big_endian = of_property_read_bool(np, "big-endian");
+
+	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(fpc->chip.dev, "ftm_sys");
+	if (IS_ERR(fpc->sys_clk)) {
+		dev_err(fpc->chip.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;
+
+	fsl_pwm_writel(fpc, 0x00, fpc->base + FTM_CNTIN);
+	fsl_pwm_writel(fpc, 0x00, fpc->base + FTM_OUTINIT);
+	fsl_pwm_writel(fpc, 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);
+
+	mutex_destroy(&fpc->lock);
+
+	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 <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>");
+MODULE_ALIAS("platform:fsl-ftm-pwm");
+MODULE_LICENSE("GPL");
-- 
1.8.4


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Thierry Reding]

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

On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote:
[...]
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 8b754e4..9029a12 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -5,6 +5,7 @@ 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_IMX)		+= pwm-imx.o
> +obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o

This needs to move up one line to preserve the alphabetical ordering.

> diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
[...]
> +struct fsl_pwm_chip {
[...]
> +	int big_endian;

This should be of type bool.

> +};
> +
> +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
> +		const void __iomem *addr)
> +{
> +	u32 val;
> +
> +	val = __raw_readl(addr);
> +
> +	if (likely(fpc->big_endian))

The likely() probably isn't very useful in this case. But if you want to
keep it, it should at least be reversed, since little-endian is actually
the default (you have to specify the big-endian property to activate the
big endian mode).

> +		val = be32_to_cpu(val);
> +	else
> +		val = le32_to_cpu(val);
> +	rmb();

I'd prefer the rmb() to follow the __raw_readl() immediately to make the
relationship more explicit.

> +
> +	return val;
> +}
> +
> +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc,
> +		u32 val, void __iomem *addr)
> +{
> +	wmb();
> +	if (likely(fpc->big_endian))
> +		val = cpu_to_be32(val);
> +	else
> +		val = cpu_to_le32(val);
> +
> +	__raw_writel(val, addr);

Same here. wmb() should precede __raw_writel() immediately.

> +static inline int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc,
> +		int index)

Please align arguments on subsequent lines with the first argument on
the first line. There are a few more of those in this file, please check
all other functions as well.

Also I think it's better to turn the FSL_PWM_CLK_* enum into a named
enum and reuse the type here. That way you get proper type-checking.

> +	switch (index) {
[...]
> +	default:
> +		return -EINVAL;

And with proper type checking you don't need this default case anymore.

> +static unsigned long fsl_pwm_calculate_period_cycles(struct fsl_pwm_chip *fpc,
> +		unsigned long period_ns, int index)
> +{
> +	int ret;
> +	unsigned long cycles;
> +
> +	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 ps.\n");

No need for the terminating fullstop (.) here, but perhaps you could
write out what "ps" actually stands for. Also including the error code
in the error message would be good since you're not propagating the
error to the caller.

> +	cycles = fsl_pwm_calculate_cycles(fpc, period_ns);
> +
> +	return cycles;

This can simply be "return fsl_pwm_calculate_cycles(fpc, period_ns);",
in which case you can get rid of the cycles variable.

> +static unsigned long fsl_pwm_calculate_period(struct fsl_pwm_chip *fpc,
> +		unsigned long period_ns)
> +{
[...]
> +	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;
> +
> +	}

There's a gratuitous blank line above this one.

> +static int fsl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			  int duty_ns, int period_ns)
> +{
> +	u32 val, period, duty;
> +	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> +
> +	mutex_lock(&fpc->lock);
> +	if (fpc->period_ns && fpc->period_ns != period_ns) {

There should be a blank line between the above two.

> +		dev_err(fpc->chip.dev, "fail to config pwm%d, the period "
> +				"time should be the same with the current "
> +				"running pwm(s).\n", pwm->hwpwm);

I think I'd make this error message more terse and put more information
into a comment. Perhaps something like this:

	/*
	 * The FSL 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 -EINVAL;

I think something like -EBUSY would be more appropriate here.

> +	if (!fpc->period_ns && duty_ns) {
> +		period = fsl_pwm_calculate_period(fpc, period_ns);
> +		if (!period) {
> +			dev_err(fpc->chip.dev, "fail to calculate the "

"failed"

> +					"period cycles.\n");

And no fullstop either. Also I don't think you need "cycles" here. In
fact something equally accurate like this:

			dev_err(fpc->chip.dev, "failed to calculate period\n");

Still fits within 80 characters.

> +		fpc->period_ns = period_ns;
> +	}
> +	mutex_unlock(&fpc->lock);

This could use a blank line as well.

> +static int fsl_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +				enum pwm_polarity polarity)
> +{
> +	u32 val;
> +	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> +
> +	val = fsl_pwm_readl(fpc, fpc->base + FTM_POL);
> +	if (polarity == PWM_POLARITY_INVERSED)

Blank line between the above.

> +		val |= BIT(pwm->hwpwm);
> +	else
> +		val &= ~BIT(pwm->hwpwm);
> +	fsl_pwm_writel(fpc, val, fpc->base + FTM_POL);

And here.

> +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	u32 val;
> +	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> +
> +	val = fsl_pwm_readl(fpc, fpc->base + FTM_OUTMASK);
> +	val |= BIT(pwm->hwpwm);
> +	fsl_pwm_writel(fpc, val, fpc->base + FTM_OUTMASK);
> +
> +	mutex_lock(&fpc->lock);
> +	fsl_counter_clock_disable(fpc);
> +
> +	val = fsl_pwm_readl(fpc, fpc->base + FTM_OUTMASK);
> +	if ((val & 0xFF) == 0xFF) {
> +		fpc->period_ns = 0;
> +		fpc->counter_clk_en = NULL;
> +	}
> +	mutex_unlock(&fpc->lock);

Same here.

> +static int fsl_pwm_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct fsl_pwm_chip *fpc;
> +	struct resource *res;
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL);
> +	if (!fpc)
> +		return -ENOMEM;
> +
> +	mutex_init(&fpc->lock);
> +
> +	fpc->chip.dev = &pdev->dev;
> +
> +	fpc->big_endian = of_property_read_bool(np, "big-endian");
> +
> +	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(fpc->chip.dev, "ftm_sys");

For consistency I'd use &pdev->dev here...

> +	if (IS_ERR(fpc->sys_clk)) {
> +		dev_err(fpc->chip.dev,

... and here.

> +	ret = pwmchip_add(&fpc->chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to add PWM chip %d\n", ret);

Perhaps "failed to add PWM chip: %d\n", so that %d isn't confused to
mean the PWM chip number?

> +static int fsl_pwm_remove(struct platform_device *pdev)
> +{
> +	struct fsl_pwm_chip *fpc = platform_get_drvdata(pdev);
> +
> +	mutex_destroy(&fpc->lock);

I think you can drop this. Once fsl_pwm_remove() exists, the memory
pointed to by fpc will be freed, so you won't be able to access the
mutex anyway.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Russell King - ARM Linux]

On Tue, Dec 17, 2013 at 12:10:22PM +0100, Thierry Reding wrote:
> On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote:
> > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
> > +		const void __iomem *addr)
> > +{
> > +	u32 val;
> > +
> > +	val = __raw_readl(addr);
> > +
> > +	if (likely(fpc->big_endian))
> 
> The likely() probably isn't very useful in this case. But if you want to
> keep it, it should at least be reversed, since little-endian is actually
> the default (you have to specify the big-endian property to activate the
> big endian mode).
> 
> > +		val = be32_to_cpu(val);
> > +	else
> > +		val = le32_to_cpu(val);

This will also cause sparse errors, because when sparse is enabled, these
expect __le32 or __be32 arguments, not u32.

> > +	rmb();
> 
> I'd prefer the rmb() to follow the __raw_readl() immediately to make the
> relationship more explicit.

A better question to ask is: why is this barrier here?  What memory
ordering operations is it trying to serialise?

> > +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc,
> > +		u32 val, void __iomem *addr)
> > +{
> > +	wmb();
> > +	if (likely(fpc->big_endian))
> > +		val = cpu_to_be32(val);
> > +	else
> > +		val = cpu_to_le32(val);
> > +
> > +	__raw_writel(val, addr);
> 
> Same here. wmb() should precede __raw_writel() immediately.

Same comments here - what memory operations is the wmb() trying to
serialise?  Does this PWM driver somehow end up doing DMA?

Re: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Tomasz Figa]

On Tuesday 17 of December 2013 11:51:36 Russell King - ARM Linux wrote:
> On Tue, Dec 17, 2013 at 12:10:22PM +0100, Thierry Reding wrote:
> > On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote:
> > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
> > > +		const void __iomem *addr)
> > > +{
> > > +	u32 val;
> > > +
> > > +	val = __raw_readl(addr);
> > > +
> > > +	if (likely(fpc->big_endian))
> > 
> > The likely() probably isn't very useful in this case. But if you want to
> > keep it, it should at least be reversed, since little-endian is actually
> > the default (you have to specify the big-endian property to activate the
> > big endian mode).
> > 
> > > +		val = be32_to_cpu(val);
> > > +	else
> > > +		val = le32_to_cpu(val);
> 
> This will also cause sparse errors, because when sparse is enabled, these
> expect __le32 or __be32 arguments, not u32.

My question is why can't you just create two sets of accessors, one big
endian and one little endian, add two function pointers to your
fsl_pwm_chip struct and let the driver set the to correct accessors in
probe?

This would eliminate the problem with types Russell mentioned and IMHO
make the code cleaner.

Best regards,
Tomasz

> 
> > > +	rmb();
> > 
> > I'd prefer the rmb() to follow the __raw_readl() immediately to make the
> > relationship more explicit.
> 
> A better question to ask is: why is this barrier here?  What memory
> ordering operations is it trying to serialise?

I'd also add a question why __raw accessors are used here.

Best regards,
Tomasz

Re: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Thierry Reding]

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

On Tue, Dec 17, 2013 at 01:00:10PM +0100, Tomasz Figa wrote:
> On Tuesday 17 of December 2013 11:51:36 Russell King - ARM Linux wrote:
> > On Tue, Dec 17, 2013 at 12:10:22PM +0100, Thierry Reding wrote:
> > > On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote:
> > > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
> > > > +		const void __iomem *addr)
> > > > +{
> > > > +	u32 val;
> > > > +
> > > > +	val = __raw_readl(addr);
> > > > +
> > > > +	if (likely(fpc->big_endian))
> > > 
> > > The likely() probably isn't very useful in this case. But if you want to
> > > keep it, it should at least be reversed, since little-endian is actually
> > > the default (you have to specify the big-endian property to activate the
> > > big endian mode).
> > > 
> > > > +		val = be32_to_cpu(val);
> > > > +	else
> > > > +		val = le32_to_cpu(val);
> > 
> > This will also cause sparse errors, because when sparse is enabled, these
> > expect __le32 or __be32 arguments, not u32.
> 
> My question is why can't you just create two sets of accessors, one big
> endian and one little endian, add two function pointers to your
> fsl_pwm_chip struct and let the driver set the to correct accessors in
> probe?

I guess that would be one possibility.

> This would eliminate the problem with types Russell mentioned and IMHO
> make the code cleaner.

I fail to see how that would eliminate the problem with the types. That
said I don't actually see sparse complaining about any type mismatches.
That's probably because the various macros implicitly cast to u32.

> > > > +	rmb();
> > > 
> > > I'd prefer the rmb() to follow the __raw_readl() immediately to make the
> > > relationship more explicit.
> > 
> > A better question to ask is: why is this barrier here?  What memory
> > ordering operations is it trying to serialise?
> 
> I'd also add a question why __raw accessors are used here.

Because both readl() and writel() explicitly perform little endian
accesses.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

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

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

Re: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Tomasz Figa]

On Tuesday 17 of December 2013 13:45:06 Thierry Reding wrote:
> On Tue, Dec 17, 2013 at 01:00:10PM +0100, Tomasz Figa wrote:
> > On Tuesday 17 of December 2013 11:51:36 Russell King - ARM Linux wrote:
> > > On Tue, Dec 17, 2013 at 12:10:22PM +0100, Thierry Reding wrote:
> > > > On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote:
> > > > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
> > > > > +		const void __iomem *addr)
> > > > > +{
> > > > > +	u32 val;
> > > > > +
> > > > > +	val = __raw_readl(addr);
> > > > > +
> > > > > +	if (likely(fpc->big_endian))
> > > > 
> > > > The likely() probably isn't very useful in this case. But if you want to
> > > > keep it, it should at least be reversed, since little-endian is actually
> > > > the default (you have to specify the big-endian property to activate the
> > > > big endian mode).
> > > > 
> > > > > +		val = be32_to_cpu(val);
> > > > > +	else
> > > > > +		val = le32_to_cpu(val);
> > > 
> > > This will also cause sparse errors, because when sparse is enabled, these
> > > expect __le32 or __be32 arguments, not u32.
> > 
> > My question is why can't you just create two sets of accessors, one big
> > endian and one little endian, add two function pointers to your
> > fsl_pwm_chip struct and let the driver set the to correct accessors in
> > probe?
> 
> I guess that would be one possibility.
> 
> > This would eliminate the problem with types Russell mentioned and IMHO
> > make the code cleaner.
> 
> I fail to see how that would eliminate the problem with the types. That
> said I don't actually see sparse complaining about any type mismatches.
> That's probably because the various macros implicitly cast to u32.

Well, in BE variant you would read the register using __raw_readl() into
a __be32 and then get an u32 from be32_to_cpu() and return it. Similarly
for writes

In LE variant, readl()/writel() could be used directly.

> 
> > > > > +	rmb();
> > > > 
> > > > I'd prefer the rmb() to follow the __raw_readl() immediately to make the
> > > > relationship more explicit.
> > > 
> > > A better question to ask is: why is this barrier here?  What memory
> > > ordering operations is it trying to serialise?
> > 
> > I'd also add a question why __raw accessors are used here.
> 
> Because both readl() and writel() explicitly perform little endian
> accesses.

Right. Thanks for pointing this out.

Best regards,
Tomasz

Re: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Russell King - ARM Linux]

On Tue, Dec 17, 2013 at 01:54:35PM +0100, Tomasz Figa wrote:
> On Tuesday 17 of December 2013 13:45:06 Thierry Reding wrote:
> > I fail to see how that would eliminate the problem with the types. That
> > said I don't actually see sparse complaining about any type mismatches.
> > That's probably because the various macros implicitly cast to u32.
> 
> Well, in BE variant you would read the register using __raw_readl() into
> a __be32 and then get an u32 from be32_to_cpu() and return it. Similarly
> for writes

__raw_readl() returns a u32, so you'll get a warning trying to assign a
u32 to a __be32.

We do have ioread32() and ioread32be() which do the appropriate conversion,
as well as the write versions too.  They both include the barrier if you're
overly concerned about that.

Re: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Tomasz Figa]

On Tuesday 17 of December 2013 13:04:35 Russell King - ARM Linux wrote:
> On Tue, Dec 17, 2013 at 01:54:35PM +0100, Tomasz Figa wrote:
> > On Tuesday 17 of December 2013 13:45:06 Thierry Reding wrote:
> > > I fail to see how that would eliminate the problem with the types. That
> > > said I don't actually see sparse complaining about any type mismatches.
> > > That's probably because the various macros implicitly cast to u32.
> > 
> > Well, in BE variant you would read the register using __raw_readl() into
> > a __be32 and then get an u32 from be32_to_cpu() and return it. Similarly
> > for writes
> 
> __raw_readl() returns a u32, so you'll get a warning trying to assign a
> u32 to a __be32.
> 
> We do have ioread32() and ioread32be() which do the appropriate conversion,
> as well as the write versions too.  They both include the barrier if you're
> overly concerned about that.

Nice. I wasn't aware of their existence. Thanks.

Best regards,
Tomasz

Re: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Thierry Reding]

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

On Tue, Dec 17, 2013 at 01:04:35PM +0000, Russell King - ARM Linux wrote:
> On Tue, Dec 17, 2013 at 01:54:35PM +0100, Tomasz Figa wrote:
> > On Tuesday 17 of December 2013 13:45:06 Thierry Reding wrote:
> > > I fail to see how that would eliminate the problem with the types. That
> > > said I don't actually see sparse complaining about any type mismatches.
> > > That's probably because the various macros implicitly cast to u32.
> > 
> > Well, in BE variant you would read the register using __raw_readl() into
> > a __be32 and then get an u32 from be32_to_cpu() and return it. Similarly
> > for writes
> 
> __raw_readl() returns a u32, so you'll get a warning trying to assign a
> u32 to a __be32.

If sparse doesn't complain about the original code here, does that mean
we have a bug that should be fixed?

> We do have ioread32() and ioread32be() which do the appropriate conversion,
> as well as the write versions too.  They both include the barrier if you're
> overly concerned about that.

A few years ago GregKH commented in response to a patch that ioread*()
weren't supposed to be used for memory-mapped only devices. The original
purpose apparently was to allow drivers to work with both I/O and
memory-mapped devices.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Li.Xiubo]

> On Tue, Dec 17, 2013 at 01:54:35PM +0100, Tomasz Figa wrote:
> > On Tuesday 17 of December 2013 13:45:06 Thierry Reding wrote:
> > > I fail to see how that would eliminate the problem with the types.
> > > That said I don't actually see sparse complaining about any type
> mismatches.
> > > That's probably because the various macros implicitly cast to u32.
> >
> > Well, in BE variant you would read the register using __raw_readl()
> > into a __be32 and then get an u32 from be32_to_cpu() and return it.
> > Similarly for writes
> 
> __raw_readl() returns a u32, so you'll get a warning trying to assign a
> u32 to a __be32.
> 

If all are ok about this method.
How about the following:

+static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
+                               const void __iomem *addr)
+{
+       u32 val;
+
+       val = __raw_readl(addr);
+
+       if (likely(fpc->big_endian))
+               val = be32_to_cpu((__force __be32)val);
+       else
+               val = le32_to_cpu((__force __le32)val);
+       rmb();
+
+       return val;
+}
+
+static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc,
+                               u32 val, void __iomem *addr)
+{
+       wmb();
+       if (likely(fpc->big_endian))
+               val = (__force u32)cpu_to_be32(val);
+       else
+               val = (__force u32)cpu_to_le32(val);
+
+       __raw_writel(val, addr);
+}

Best Regards,
Xiubo

RE: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Li.Xiubo]

> On Tue, Dec 17, 2013 at 01:00:10PM +0100, Tomasz Figa wrote:
> > On Tuesday 17 of December 2013 11:51:36 Russell King - ARM Linux wrote:
> > > On Tue, Dec 17, 2013 at 12:10:22PM +0100, Thierry Reding wrote:
> > > > On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote:
> > > > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
> > > > > +		const void __iomem *addr)
> > > > > +{
> > > > > +	u32 val;
> > > > > +
> > > > > +	val = __raw_readl(addr);
> > > > > +
> > > > > +	if (likely(fpc->big_endian))
> > > >
> > > > The likely() probably isn't very useful in this case. But if you
> > > > want to keep it, it should at least be reversed, since
> > > > little-endian is actually the default (you have to specify the
> > > > big-endian property to activate the big endian mode).
> > > >
> > > > > +		val = be32_to_cpu(val);
> > > > > +	else
> > > > > +		val = le32_to_cpu(val);
> > >
> > > This will also cause sparse errors, because when sparse is enabled,
> > > these expect __le32 or __be32 arguments, not u32.
> >
> > My question is why can't you just create two sets of accessors, one
> > big endian and one little endian, add two function pointers to your
> > fsl_pwm_chip struct and let the driver set the to correct accessors in
> > probe?
> 
> I guess that would be one possibility.
>

Yes, that's one possibility.

If so, it must deference the function pointers and do the C stack push/pop stuff
every time when doing the accesses. For instance, but for some devices(USB, NET,
DMA..), we need to do many accesses every time in the frequent interrupt handler,
and I think the inline type functions will be more efficiency.

In LS-1 series platforms, there are many devices that need to do the same work like
this, and could these be moved to some global files ?


--
Xiubo

Re: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Thierry Reding]

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

On Tue, Dec 17, 2013 at 11:51:36AM +0000, Russell King - ARM Linux wrote:
> On Tue, Dec 17, 2013 at 12:10:22PM +0100, Thierry Reding wrote:
> > On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote:
> > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
> > > +		const void __iomem *addr)
> > > +{
> > > +	u32 val;
> > > +
> > > +	val = __raw_readl(addr);
> > > +
> > > +	if (likely(fpc->big_endian))
> > 
> > The likely() probably isn't very useful in this case. But if you want to
> > keep it, it should at least be reversed, since little-endian is actually
> > the default (you have to specify the big-endian property to activate the
> > big endian mode).
> > 
> > > +		val = be32_to_cpu(val);
> > > +	else
> > > +		val = le32_to_cpu(val);
> 
> This will also cause sparse errors, because when sparse is enabled, these
> expect __le32 or __be32 arguments, not u32.
> 
> > > +	rmb();
> > 
> > I'd prefer the rmb() to follow the __raw_readl() immediately to make the
> > relationship more explicit.
> 
> A better question to ask is: why is this barrier here?  What memory
> ordering operations is it trying to serialise?

I suppose that this was done so that the accesses would essentially
remain the same as those performed by readl() and writel().

> > > +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc,
> > > +		u32 val, void __iomem *addr)
> > > +{
> > > +	wmb();
> > > +	if (likely(fpc->big_endian))
> > > +		val = cpu_to_be32(val);
> > > +	else
> > > +		val = cpu_to_le32(val);
> > > +
> > > +	__raw_writel(val, addr);
> > 
> > Same here. wmb() should precede __raw_writel() immediately.
> 
> Same comments here - what memory operations is the wmb() trying to
> serialise?  Does this PWM driver somehow end up doing DMA?

Not that I can see. But if my understanding is correct, not using the
barriers would allow the compiler and CPU to reorder accesses, and by
that cause the register accesses to potentially happen in the wrong
order.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Russell King - ARM Linux]

On Tue, Dec 17, 2013 at 01:24:33PM +0100, Thierry Reding wrote:
> On Tue, Dec 17, 2013 at 11:51:36AM +0000, Russell King - ARM Linux wrote:
> > Same comments here - what memory operations is the wmb() trying to
> > serialise?  Does this PWM driver somehow end up doing DMA?
> 
> Not that I can see. But if my understanding is correct, not using the
> barriers would allow the compiler and CPU to reorder accesses, and by
> that cause the register accesses to potentially happen in the wrong
> order.

The compiler won't reorder them, but the CPU may if it meets certain
criteria.  The architecture guarantees that accesses to device memory
within a (minimum of) 1KB block will be ordered.

The ARM ARM is slightly ambiguous in how this is applied - in one
place it says that "Accesses must arrive at any particular memory-mapped
peripheral or block of memory in program order" and another part it
says "The size of a memory mapped peripheral, or a block of memory,
is IMPLEMENTATION DEFINED, but is not smaller than 1KByte.  Note
This implies that the maximum memory-mapped peripheral size for which
the architecture guarantees order for all implementations is 1KB."  See
page A3-148.

What this means (to me at least) is that on any SoC, the architecture
guarantees that accesses _within_ a 1KB device memory block will always
be ordered, but two accesses outside of a 1KB block _to the same device_
is implementation defined whether it is ordered or not.

The interesting point here though is that the "note" contradicts the
first definition if you have (eg) AMBA Primecell peripherals which are
generally 4KB in size, since if the architecture only guarantees 1KB,
then accesses _may_ _not_ arrive at one primecell in program order.
Hence, the note is a direct contradiction of the first definition.

Re: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Thierry Reding]

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

On Tue, Dec 17, 2013 at 12:58:32PM +0000, Russell King - ARM Linux wrote:
> On Tue, Dec 17, 2013 at 01:24:33PM +0100, Thierry Reding wrote:
> > On Tue, Dec 17, 2013 at 11:51:36AM +0000, Russell King - ARM Linux wrote:
> > > Same comments here - what memory operations is the wmb() trying to
> > > serialise?  Does this PWM driver somehow end up doing DMA?
> > 
> > Not that I can see. But if my understanding is correct, not using the
> > barriers would allow the compiler and CPU to reorder accesses, and by
> > that cause the register accesses to potentially happen in the wrong
> > order.
> 
> The compiler won't reorder them, but the CPU may if it meets certain
> criteria.  The architecture guarantees that accesses to device memory
> within a (minimum of) 1KB block will be ordered.
> 
> The ARM ARM is slightly ambiguous in how this is applied - in one
> place it says that "Accesses must arrive at any particular memory-mapped
> peripheral or block of memory in program order" and another part it
> says "The size of a memory mapped peripheral, or a block of memory,
> is IMPLEMENTATION DEFINED, but is not smaller than 1KByte.  Note
> This implies that the maximum memory-mapped peripheral size for which
> the architecture guarantees order for all implementations is 1KB."  See
> page A3-148.

None of the ARM ARM versions that I have seem to have page A3-148. Which
version should I be looking at? Not that I'm in any way doubting what
you're saying, I'd just like to make sure to have the correct reference
to look at in the future.

> What this means (to me at least) is that on any SoC, the architecture
> guarantees that accesses _within_ a 1KB device memory block will always
> be ordered, but two accesses outside of a 1KB block _to the same device_
> is implementation defined whether it is ordered or not.

This means at least every ARM SoC would behave that way. Since this
driver doesn't have an explicit dependency on ARM I assume it could
eventually be used on a different architecture. Even more so since
there's Freescale in the name.

> The interesting point here though is that the "note" contradicts the
> first definition if you have (eg) AMBA Primecell peripherals which are
> generally 4KB in size, since if the architecture only guarantees 1KB,
> then accesses _may_ _not_ arrive at one primecell in program order.
> Hence, the note is a direct contradiction of the first definition.

Interesting indeed. Perhaps implementation defined in this case means
that an implementation would have to adjust the size of a memory mapped
peripheral or block of memory accordingly, depending on the largest
block within the SoC.

I suppose, though, that if the architecture doesn't give any guarantees
about it, we can't safely assume that the implementation will.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support

[Li.Xiubo]

> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -5,6 +5,7 @@ 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_IMX)		+= pwm-imx.o
> > +obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
> 
> This needs to move up one line to preserve the alphabetical ordering.
> 

Yes, it had been. It caused by the rebase operation and after that I
haven't aware about this. I will revise this.


> > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> [...]
> > +struct fsl_pwm_chip {
> [...]
> > +	int big_endian;
> 
> This should be of type bool.

I'll revise it.

> 
> > +};
> > +
> > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
> > +		const void __iomem *addr)
> > +{
> > +	u32 val;
> > +
> > +	val = __raw_readl(addr);
> > +
> > +	if (likely(fpc->big_endian))
> 
> The likely() probably isn't very useful in this case. But if you want to
> keep it, it should at least be reversed, since little-endian is actually
> the default (you have to specify the big-endian property to activate the
> big endian mode).
> 

So why added the likely() for bit endian mode is that this driver maybe used
for ARM and like PowerPC architectures, and in PowerPC platforms and LS-1 series
the big endian mode will be used almostly. Now only VF610-twr is the little
endian mode devices, and this is also why I added the big endian mode support.


> > +		val = be32_to_cpu(val);
> > +	else
> > +		val = le32_to_cpu(val);
> > +	rmb();
> 
> I'd prefer the rmb() to follow the __raw_readl() immediately to make the
> relationship more explicit.
> 

The fsl_pwm_readl() and fsl_pwm_writel() are almost copied the readl() and
writel().

For instance:
+ static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
+                 const void __iomem *addr)
+ {
+         u32 val;
+
+         val = __raw_readl(addr);
+ 
+         if (likely(fpc->big_endian))
+                 val = be32_to_cpu(val);
+         else
+                 val = le32_to_cpu(val);
+         rmb();
+
+         return val;
+ }
As we can see, this is one inline function, if locating the rmb() just follow
__raw_readl() immediately, I'm not very sure the compiler will do the following
reorders ?

In some function calls the fsl_pwm_readl(Address1) and little-endain mode is used:

The original code is:
tmp = fsl_pwm_readl(Address1)
<Do calculating about the tmp value returned from fsl_pwm_readl here>

Maybe reordered to :
tmp = raw_readl(addr);
rmb(); 
<Do calculating about the tmp value returned from fsl_pwm_readl here>
tmp = le32_to_cpu(tmp);
...

As we have known that in the fsl_pwm_readl(), the 'val' used by le32_to_cpu() has
a dependency on __raw_readl(), so the compiler will grantee the right orders, and
also the Arch will grantee the right orders too.

This is why I located the rmb() just before 'return val'.

> > +static inline int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc,
> > +		int index)
> 
> Please align arguments on subsequent lines with the first argument on the
> first line. There are a few more of those in this file, please check all
> other functions as well.
> 
> Also I think it's better to turn the FSL_PWM_CLK_* enum into a named enum
> and reuse the type here. That way you get proper type-checking.
> 
> > +	switch (index) {
> [...]
> > +	default:
> > +		return -EINVAL;
> 
> And with proper type checking you don't need this default case anymore.
>

I'll revise this.


 
> > +static unsigned long fsl_pwm_calculate_period_cycles(struct fsl_pwm_chip
> *fpc,
> > +		unsigned long period_ns, int index) {
> > +	int ret;
> > +	unsigned long cycles;
> > +
> > +	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 ps.\n");
> 
> No need for the terminating fullstop (.) here, but perhaps you could write
> out what "ps" actually stands for. Also including the error code in the
> error message would be good since you're not propagating the error to the
> caller.
> 

Yes, I will.

> > +	cycles = fsl_pwm_calculate_cycles(fpc, period_ns);
> > +
> > +	return cycles;
> 
> This can simply be "return fsl_pwm_calculate_cycles(fpc, period_ns);", in
> which case you can get rid of the cycles variable.
> 

That's better.

> > +static unsigned long fsl_pwm_calculate_period(struct fsl_pwm_chip *fpc,
> > +		unsigned long period_ns)
> > +{
> [...]
> > +	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;
> > +
> > +	}
> 
> There's a gratuitous blank line above this one.
> 
I'll remove it.

> > +		dev_err(fpc->chip.dev, "fail to config pwm%d, the period "
> > +				"time should be the same with the current "
> > +				"running pwm(s).\n", pwm->hwpwm);
> 
> I think I'd make this error message more terse and put more information
> into a comment. Perhaps something like this:
> 
> 	/*
> 	 * The FSL 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 -EINVAL;
> 
> I think something like -EBUSY would be more appropriate here.
> 

Agree, I will use this.

> > +	if (!fpc->period_ns && duty_ns) {
> > +		period = fsl_pwm_calculate_period(fpc, period_ns);
> > +		if (!period) {
> > +			dev_err(fpc->chip.dev, "fail to calculate the "
> 
> "failed"
> 
> > +					"period cycles.\n");
> 
> And no fullstop either. Also I don't think you need "cycles" here. In fact
> something equally accurate like this:
> 
> 			dev_err(fpc->chip.dev, "failed to calculate period\n");
> 
> Still fits within 80 characters.
> 

Yes, I will.

> > +		fpc->period_ns = period_ns;
> > +	}
> > +	mutex_unlock(&fpc->lock);
> 
> This could use a blank line as well.
> 

I'll add it.

> > +static int fsl_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > +				enum pwm_polarity polarity)
> > +{
> > +	u32 val;
> > +	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> > +
> > +	val = fsl_pwm_readl(fpc, fpc->base + FTM_POL);
> > +	if (polarity == PWM_POLARITY_INVERSED)
> 
> Blank line between the above.
>
I will.

 
> > +static int fsl_pwm_probe(struct platform_device *pdev) {
> > +	int ret;
> > +	struct fsl_pwm_chip *fpc;
> > +	struct resource *res;
> > +	struct device_node *np = pdev->dev.of_node;
> > +
> > +	fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL);
> > +	if (!fpc)
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&fpc->lock);
> > +
> > +	fpc->chip.dev = &pdev->dev;
> > +
> > +	fpc->big_endian = of_property_read_bool(np, "big-endian");
> > +
> > +	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(fpc->chip.dev, "ftm_sys");
> 
> For consistency I'd use &pdev->dev here...
> 
> > +	if (IS_ERR(fpc->sys_clk)) {
> > +		dev_err(fpc->chip.dev,
> 
> ... and here.
> 

I will.

> > +	ret = pwmchip_add(&fpc->chip);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "failed to add PWM chip %d\n", ret);
> 
> Perhaps "failed to add PWM chip: %d\n", so that %d isn't confused to mean
> the PWM chip number?
> 
Yes, that's better.

> > +static int fsl_pwm_remove(struct platform_device *pdev) {
> > +	struct fsl_pwm_chip *fpc = platform_get_drvdata(pdev);
> > +
> > +	mutex_destroy(&fpc->lock);
> 
> I think you can drop this. Once fsl_pwm_remove() exists, the memory pointed
> to by fpc will be freed, so you won't be able to access the mutex anyway.
> 

Yes, actually it is.
Well I think this can make the code more robust and uniform.



Thanks,

--
Xiubo

[PATCHv7 2/4] ARM: dts: Add Freescale FTM PWM node for VF610.

[Xiubo Li]

This adds devicetree node for VF610, and there are 8 channels
supported.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 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 67d929c..9bea0b2 100644
--- a/arch/arm/boot/dts/vf610.dtsi
+++ b/arch/arm/boot/dts/vf610.dtsi
@@ -140,6 +140,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

[PATCHv7 3/4] ARM: dts: Enables FTM PWM device for Vybrid VF610 TOWER board.

[Xiubo Li]

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 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 82d352f..3130f85 100644
--- a/arch/arm/boot/dts/vf610-twr.dts
+++ b/arch/arm/boot/dts/vf610-twr.dts
@@ -83,6 +83,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

[PATCHv7 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.

[Xiubo Li]

This adds the binding documentation for Freescale FlexTimer Module
(FTM) PWM driver under Documentation/devicetree/bindings/pwm/.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
Acked-by: Kumar Gala <galak@codeaurora.org>
---
 .../devicetree/bindings/pwm/pwm-fsl-ftm.txt        | 38 ++++++++++++++++++++++
 1 file changed, 38 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..6ae6377
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
@@ -0,0 +1,38 @@
+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.
+- big-endian: If this property is absent, the little endian mode will be in
+  use as default, or the big endian mode will be in use for all the device
+  registers.
+
+
+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>;
+		big-endian;
+};
-- 
1.8.4