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

[PATCHv8 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 v8:
- Fix some issues pointed by Thierry.
- Fix the _readl/_writel of sparse check.

- And the ftm_readl()/ftm_writel() are just copied from the
  readl()/writel(), and then add the big endian check.



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.

[snip] v1~v6

[PATCHv8 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@freescale.com>
Signed-off-by: Alison Wang <b18965@freescale.com>
Signed-off-by: Jingchang Lu <b35083@freescale.com>
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/pwm/Kconfig       |  10 +
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-fsl-ftm.c | 514 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 525 insertions(+)
 create mode 100644 drivers/pwm/pwm-fsl-ftm.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 3f66427..ec4bf78 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..b335db1 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.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..c201e20
--- /dev/null
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -0,0 +1,514 @@
+/*
+ *  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;
+	bool big_endian;
+};
+
+static inline u32 ftm_readl(bool big_endian, const void __iomem *addr)
+{
+	u32 val;
+
+	val = __raw_readl(addr);
+
+	if (likely(big_endian))
+		val = be32_to_cpu((__force __be32)val);
+	else
+		val = le32_to_cpu((__force __le32)val);
+	rmb();
+
+	return val;
+}
+
+static inline void ftm_writel(bool big_endian, u32 val, void __iomem *addr)
+{
+	wmb();
+	if (likely(big_endian))
+		val = (__force u32)cpu_to_be32(val);
+	else
+		val = (__force u32)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, 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 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,
+				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 = ftm_readl(fpc->big_endian, 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 = ftm_readl(fpc->big_endian, fpc->base + FTM_SC);
+		val &= ~(FTM_SC_PS_MASK << FTM_SC_PS_SHIFT);
+		val |= fpc->clk_ps;
+		ftm_writel(fpc->big_endian, val, fpc->base + FTM_SC);
+		ftm_writel(fpc->big_endian, 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);
+
+	ftm_writel(fpc->big_endian, FTM_CSC_MSB | FTM_CSC_ELSB,
+			fpc->base + FTM_CSC(pwm->hwpwm));
+	ftm_writel(fpc->big_endian, 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 = ftm_readl(fpc->big_endian, fpc->base + FTM_POL);
+
+	if (polarity == PWM_POLARITY_INVERSED)
+		val |= BIT(pwm->hwpwm);
+	else
+		val &= ~BIT(pwm->hwpwm);
+
+	ftm_writel(fpc->big_endian, 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 = ftm_readl(fpc->big_endian, fpc->base + FTM_SC);
+	val &= ~(FTM_SC_CLK_MASK << FTM_SC_CLK_SHIFT);
+	val |= fpc->counter_clk_select;
+	ftm_writel(fpc->big_endian, 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 = ftm_readl(fpc->big_endian, fpc->base + FTM_OUTMASK);
+	val &= ~BIT(pwm->hwpwm);
+	ftm_writel(fpc->big_endian, 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 = ftm_readl(fpc->big_endian, fpc->base + FTM_SC);
+	val &= ~(FTM_SC_CLK_MASK << FTM_SC_CLK_SHIFT);
+	ftm_writel(fpc->big_endian, 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 = ftm_readl(fpc->big_endian, fpc->base + FTM_OUTMASK);
+	val |= BIT(pwm->hwpwm);
+	ftm_writel(fpc->big_endian, val, fpc->base + FTM_OUTMASK);
+
+	mutex_lock(&fpc->lock);
+
+	fsl_counter_clock_disable(fpc);
+
+	val = ftm_readl(fpc->big_endian, 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;
+	struct device_node *np = pdev->dev.of_node;
+	int ret;
+
+	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(&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;
+
+	ftm_writel(fpc->big_endian, 0x00, fpc->base + FTM_CNTIN);
+	ftm_writel(fpc->big_endian, 0x00, fpc->base + FTM_OUTINIT);
+	ftm_writel(fpc->big_endian, 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@freescale.com>");
+MODULE_ALIAS("platform:fsl-ftm-pwm");
+MODULE_LICENSE("GPL");
-- 
1.8.4

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

[Arnd Bergmann]

On Thursday 02 January 2014 17:57:01 Xiubo Li wrote:
> +static inline u32 ftm_readl(bool big_endian, const void __iomem *addr)
> +{
> +       u32 val;
> +
> +       val = __raw_readl(addr);
> +
> +       if (likely(big_endian))
> +               val = be32_to_cpu((__force __be32)val);
> +       else
> +               val = le32_to_cpu((__force __le32)val);
> +       rmb();
> +
> +       return val;
> +}
> +
> +static inline void ftm_writel(bool big_endian, u32 val, void __iomem *addr)
> +{
> +       wmb();
> +       if (likely(big_endian))
> +               val = (__force u32)cpu_to_be32(val);
> +       else
> +               val = (__force u32)cpu_to_le32(val);
> +
> +       __raw_writel(val, addr);

These functions definitely need some comments regarding why this is necessary.
I assume that after 8 rounds of review (that I did not read) this has been
discussed already, so please explain above the functions why you don't just
use ioread32be/ioread32 and iowrite32be/iowrite32 here.

Also all callers seem to be of the style

       val = ftm_readl(fpc->big_endian, fpc->base + FTM_SC);

so I wonder why you don't just pass the fpc pointer to the function
and make it

static inline void ftm_readl(struct fsl_pwm_chip *fpc, unsigned long reg, u32 val)
{
	if (fpc->big_endian)
		iowrite32be(val, fpc->base + reg);
	else
		iowrite32(val, fpc->base + reg);
}

	Arnd

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

[Li.Xiubo]

Hi Arnd,


> Subject: Re: [PATCHv8 1/4] pwm: Add Freescale FTM PWM driver support
> 
> On Thursday 02 January 2014 17:57:01 Xiubo Li wrote:
> > +static inline u32 ftm_readl(bool big_endian, const void __iomem *addr)
> > +{
> > +       u32 val;
> > +
> > +       val = __raw_readl(addr);
> > +
> > +       if (likely(big_endian))
> > +               val = be32_to_cpu((__force __be32)val);
> > +       else
> > +               val = le32_to_cpu((__force __le32)val);
> > +       rmb();
> > +
> > +       return val;
> > +}
> > +
> > +static inline void ftm_writel(bool big_endian, u32 val, void __iomem *addr)
> > +{
> > +       wmb();
> > +       if (likely(big_endian))
> > +               val = (__force u32)cpu_to_be32(val);
> > +       else
> > +               val = (__force u32)cpu_to_le32(val);
> > +
> > +       __raw_writel(val, addr);
> 
> These functions definitely need some comments regarding why this is necessary.
> I assume that after 8 rounds of review (that I did not read) this has been
> discussed already, so please explain above the functions why you don't just
> use ioread32be/ioread32 and iowrite32be/iowrite32 here.
>

As Thierry has commonts in some days before:
----
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.
----
I'm not very sure.

The big-endian supports is added at v7~v8 patch series.


> Also all callers seem to be of the style
> 
>        val = ftm_readl(fpc->big_endian, fpc->base + FTM_SC);
> 
> so I wonder why you don't just pass the fpc pointer to the function
> and make it
> 
> static inline void ftm_readl(struct fsl_pwm_chip *fpc, unsigned long reg, u32
> val)
> {
> 	if (fpc->big_endian)
> 		iowrite32be(val, fpc->base + reg);
> 	else
> 		iowrite32(val, fpc->base + reg);
> }
> 

Yes, it was.

I thought this will make the ftm_readl/ftm_writel look more brief like other drivers.

 
Thanks very much.

--
Best Regards,
Xiubo

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

[Arnd Bergmann]

On Friday 03 January 2014, Li.Xiubo@freescale.com wrote:
> > Subject: Re: [PATCHv8 1/4] pwm: Add Freescale FTM PWM driver support
> > 
> > On Thursday 02 January 2014 17:57:01 Xiubo Li wrote:
> > > +static inline u32 ftm_readl(bool big_endian, const void __iomem *addr)
> > > +{
> > > +       u32 val;
> > > +
> > > +       val = __raw_readl(addr);
> > > +
> > > +       if (likely(big_endian))
> > > +               val = be32_to_cpu((__force __be32)val);
> > > +       else
> > > +               val = le32_to_cpu((__force __le32)val);
> > > +       rmb();
> > > +
> > > +       return val;
> > > +}
> > > +
> > > +static inline void ftm_writel(bool big_endian, u32 val, void __iomem *addr)
> > > +{
> > > +       wmb();
> > > +       if (likely(big_endian))
> > > +               val = (__force u32)cpu_to_be32(val);
> > > +       else
> > > +               val = (__force u32)cpu_to_le32(val);
> > > +
> > > +       __raw_writel(val, addr);
> > 
> > These functions definitely need some comments regarding why this is necessary.
> > I assume that after 8 rounds of review (that I did not read) this has been
> > discussed already, so please explain above the functions why you don't just
> > use ioread32be/ioread32 and iowrite32be/iowrite32 here.
> >
> 
> As Thierry has commonts in some days before:
> ----
> 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.
> ----
> I'm not very sure.

Adding Greg here to make sure we have a common understanding in the future.

ioread*() was indeed introduced for devices that can be both I/O and memory
space, and is a bit slowed than readl() on architectures that don't have
memory mapped I/O space (i.e. x86).

However, ioread32be is the only architecture-independent big-endian
accessor function (some architectures have in_be32, but that doesn't
work on ARM), and as mandated by Linus a couple of years ago when
we wanted to take some shortcuts on powerpc, ioread32 has to be
semantically the same as readl when used on pointers returned from
ioremap() rather than ioport_map, and ioread32be is just the
byte-swapped version of it.

We have a couple of drivers using ioread* because of the need for
big-endian data access, and I would continue recommending that unless
I hear a good argument against it.

> The big-endian supports is added at v7~v8 patch series.

Yes, that version looks fine and it avoids the above problem nicely,
but I'd still like to make sure we come to a conclusion on the problem
of big-endian I/O accessors.

	Arnd

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

[Li.Xiubo]

Hi Arnd,

> 
> Adding Greg here to make sure we have a common understanding in the future.
> 
> ioread*() was indeed introduced for devices that can be both I/O and memory
> space, and is a bit slowed than readl() on architectures that don't have
> memory mapped I/O space (i.e. x86).
> 
> However, ioread32be is the only architecture-independent big-endian
> accessor function (some architectures have in_be32, but that doesn't
> work on ARM), and as mandated by Linus a couple of years ago when
> we wanted to take some shortcuts on powerpc, ioread32 has to be
> semantically the same as readl when used on pointers returned from
> ioremap() rather than ioport_map, and ioread32be is just the
> byte-swapped version of it.
> 
> We have a couple of drivers using ioread* because of the need for
> big-endian data access, and I would continue recommending that unless
> I hear a good argument against it.
> 
> > The big-endian supports is added at v7~v8 patch series.
> 
> Yes, that version looks fine and it avoids the above problem nicely,
> but I'd still like to make sure we come to a conclusion on the problem
> of big-endian I/O accessors.
> 
> 

How about the following ?
-----
+static inline u32 ftm_readl(struct fsl_pwm_chip *fpc, unsigned long reg)
+{
+       if (fpc->big_endian)
+               return ioread32be(fpc->base + reg);
+       else
+               return ioread32(fpc->base + reg);
+}
+
+static inline void ftm_writel(struct fsl_pwm_chip *fpc, u32 val,
+                             unsigned long reg)
+{
+       if (fpc->big_endian)
+               iowrite32be(val, fpc->base + reg);
+       else
+               iowrite32(val, fpc->base + reg);
+}
------

I'll send another version soon.
Does this patch series has other problems needed to fix or revise ?

Thanks,

Best Regards,
Xiubo

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

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

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