[PATCH v2 00/10] pwm: imx: Provide atomic operation for IMX PWM driver

[PATCH v2 00/10] pwm: imx: Provide atomic operation for IMX PWM driver

[Lukasz Majewski]

This patch set brings atomic operation to i.MX's PWMv2 driver.

This work has been supported and suggested by Boris Brezillon [1] and 
Stefan Agner, by showing how simple the transition could be :-).

It has been divided into several steps:

- Provide different pwm ops for PWMv1 and PWM v2

- Separate PWMv1 commits from "generic" and non atomic PWM code.

  NOTE: Since I do not have board with PWMv1, I would like to ask somebody
        for testing

- Move some imx_config_v2 code to separate functions

- Provide PWM atomic implementation (the ->apply() driver) for PWMv2 in a
  single patch for better readability.
	
- Remove redundant PWM code (disable, enable, config callbacks)

- Update proper documentation entries

- Provide support for polarity inversion on top of atomic PWM rework

Test HW:
--------
This patch set has been tested on i.MX6q board with vanilla 4.7, 4.8 and
v4.9-rc2 kernel SHA1: 07d9a380680d1c0eb51ef87ff2eab5c994949e69


The PWM operation has been tested with pwm_bl backlight driver by changing
its brightness.

[1]: http://patchwork.ozlabs.org/patch/685402/


Lothar Wassmann (2):
  pwm: print error messages with pr_err() instead of pr_debug()
  pwm: core: make the PWM_POLARITY flag in DTB optional

Lukasz Majewski (8):
  pwm: imx: Add separate set of pwm ops for PWMv1 and PWMv2
  pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm
    operation
  pwm: imx: Move PWMv2 software reset code to a separate function
  pwm: imx: Move PWMv2 wait for fifo slot code to a separate function
  pwm: imx: Provide atomic PWM support for i.MX PWMv2
  pwm: imx: Remove redundant i.MX PWMv2 code
  pwm: imx: doc: Update imx-pwm.txt documentation entry
  pwm: imx: Add polarity inversion support to i.MX's PWMv2

 Documentation/devicetree/bindings/pwm/imx-pwm.txt |   6 +-
 drivers/pwm/core.c                                |  26 ++-
 drivers/pwm/pwm-imx.c                             | 255 ++++++++++++----------
 3 files changed, 152 insertions(+), 135 deletions(-)

-- 
2.1.4

[PATCH v2 01/10] pwm: print error messages with pr_err() instead of pr_debug()

[Lukasz Majewski]

From: Lothar Wassmann <LW@KARO-electronics.de>

Make the messages that are printed in case of fatal errors actually
visible to the user without having to recompile the driver with
debugging enabled.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
---
 drivers/pwm/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 172ef82..ec7179f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -663,13 +663,13 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
 	err = of_parse_phandle_with_args(np, "pwms", "#pwm-cells", index,
 					 &args);
 	if (err) {
-		pr_debug("%s(): can't parse \"pwms\" property\n", __func__);
+		pr_err("%s(): can't parse \"pwms\" property\n", __func__);
 		return ERR_PTR(err);
 	}
 
 	pc = of_node_to_pwmchip(args.np);
 	if (IS_ERR(pc)) {
-		pr_debug("%s(): PWM chip not found\n", __func__);
+		pr_err("%s(): PWM chip not found\n", __func__);
 		pwm = ERR_CAST(pc);
 		goto put;
 	}
-- 
2.1.4

[PATCH v2 02/10] pwm: imx: Add separate set of pwm ops for PWMv1 and PWMv2

[Lukasz Majewski]

This patch provides separate set of pwm ops utilized by
i.MX's PWMv1 and PWMv2.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
Acked-by: Shawn Guo <shawn.guo@linaro.org>
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
---
Changes for v2:
- New patch
---
 drivers/pwm/pwm-imx.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index d600fd5..ea3ce79 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -240,7 +240,14 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	clk_disable_unprepare(imx->clk_per);
 }
 
-static struct pwm_ops imx_pwm_ops = {
+static struct pwm_ops imx_pwm_ops_v1 = {
+	.enable = imx_pwm_enable,
+	.disable = imx_pwm_disable,
+	.config = imx_pwm_config,
+	.owner = THIS_MODULE,
+};
+
+static struct pwm_ops imx_pwm_ops_v2 = {
 	.enable = imx_pwm_enable,
 	.disable = imx_pwm_disable,
 	.config = imx_pwm_config,
@@ -251,16 +258,19 @@ struct imx_pwm_data {
 	int (*config)(struct pwm_chip *chip,
 		struct pwm_device *pwm, int duty_ns, int period_ns);
 	void (*set_enable)(struct pwm_chip *chip, bool enable);
+	struct pwm_ops *pwm_ops;
 };
 
 static struct imx_pwm_data imx_pwm_data_v1 = {
 	.config = imx_pwm_config_v1,
 	.set_enable = imx_pwm_set_enable_v1,
+	.pwm_ops = &imx_pwm_ops_v1,
 };
 
 static struct imx_pwm_data imx_pwm_data_v2 = {
 	.config = imx_pwm_config_v2,
 	.set_enable = imx_pwm_set_enable_v2,
+	.pwm_ops = &imx_pwm_ops_v2,
 };
 
 static const struct of_device_id imx_pwm_dt_ids[] = {
@@ -282,6 +292,8 @@ static int imx_pwm_probe(struct platform_device *pdev)
 	if (!of_id)
 		return -ENODEV;
 
+	data = of_id->data;
+
 	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
 	if (imx == NULL)
 		return -ENOMEM;
@@ -300,7 +312,7 @@ static int imx_pwm_probe(struct platform_device *pdev)
 		return PTR_ERR(imx->clk_ipg);
 	}
 
-	imx->chip.ops = &imx_pwm_ops;
+	imx->chip.ops = data->pwm_ops;
 	imx->chip.dev = &pdev->dev;
 	imx->chip.base = -1;
 	imx->chip.npwm = 1;
@@ -311,7 +323,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(imx->mmio_base))
 		return PTR_ERR(imx->mmio_base);
 
-	data = of_id->data;
 	imx->config = data->config;
 	imx->set_enable = data->set_enable;
 
-- 
2.1.4

[PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Lukasz Majewski]

The code has been rewritten to remove "generic" calls to
imx_pwm_{enable|disable|config}.

Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
implementation.

Suggested-by: Stefan Agner <stefan@agner.ch>
Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
Changes for v2:
- Add missing clock unprepare for clk_ipg
- Enable peripheral PWM clock (clk_per)
---
 drivers/pwm/pwm-imx.c | 50 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index ea3ce79..822eb5a 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -65,8 +65,6 @@ struct imx_chip {
 static int imx_pwm_config_v1(struct pwm_chip *chip,
 		struct pwm_device *pwm, int duty_ns, int period_ns)
 {
-	struct imx_chip *imx = to_imx_chip(chip);
-
 	/*
 	 * The PWM subsystem allows for exact frequencies. However,
 	 * I cannot connect a scope on my device to the PWM line and
@@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
 	 * both the prescaler (/1 .. /128) and then by CLKSEL
 	 * (/2 .. /16).
 	 */
+	struct imx_chip *imx = to_imx_chip(chip);
 	u32 max = readl(imx->mmio_base + MX1_PWMP);
 	u32 p = max * duty_ns / period_ns;
+	int ret;
+
+	ret = clk_prepare_enable(imx->clk_ipg);
+	if (ret)
+		return ret;
+
 	writel(max - p, imx->mmio_base + MX1_PWMS);
 
+	clk_disable_unprepare(imx->clk_ipg);
+
 	return 0;
 }
 
-static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
+static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct imx_chip *imx = to_imx_chip(chip);
+	int ret;
 	u32 val;
 
+	ret = clk_prepare_enable(imx->clk_ipg);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(imx->clk_per);
+	if (ret)
+		return ret;
+
 	val = readl(imx->mmio_base + MX1_PWMC);
+	val |= MX1_PWMC_EN;
+	writel(val, imx->mmio_base + MX1_PWMC);
 
-	if (enable)
-		val |= MX1_PWMC_EN;
-	else
-		val &= ~MX1_PWMC_EN;
+	clk_disable_unprepare(imx->clk_ipg);
+
+	return 0;
+}
+
+static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct imx_chip *imx = to_imx_chip(chip);
+	u32 val;
+
+	val = readl(imx->mmio_base + MX1_PWMC);
+	val &= ~MX1_PWMC_EN;
 
 	writel(val, imx->mmio_base + MX1_PWMC);
+
+	clk_disable_unprepare(imx->clk_per);
 }
 
 static int imx_pwm_config_v2(struct pwm_chip *chip,
@@ -241,9 +269,9 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 }
 
 static struct pwm_ops imx_pwm_ops_v1 = {
-	.enable = imx_pwm_enable,
-	.disable = imx_pwm_disable,
-	.config = imx_pwm_config,
+	.enable = imx_pwm_enable_v1,
+	.disable = imx_pwm_disable_v1,
+	.config = imx_pwm_config_v1,
 	.owner = THIS_MODULE,
 };
 
@@ -262,8 +290,6 @@ struct imx_pwm_data {
 };
 
 static struct imx_pwm_data imx_pwm_data_v1 = {
-	.config = imx_pwm_config_v1,
-	.set_enable = imx_pwm_set_enable_v1,
 	.pwm_ops = &imx_pwm_ops_v1,
 };
 
-- 
2.1.4

[PATCH v2 04/10] pwm: imx: Move PWMv2 software reset code to a separate function

[Lukasz Majewski]

The software reset code has been extracted from imx_pwm_config_v2 function
and moved to new one - imx_pwm_sw_reset().

This change reduces the overall size of imx_pwm_config_v2() and prepares
it for atomic PWM operation.

Suggested-by: Stefan Agner <stefan@agner.ch>
Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>

---
Changes for v2:
- Add missing parenthesis
---
 drivers/pwm/pwm-imx.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 822eb5a..8174ae6 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -134,6 +134,25 @@ static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
 	clk_disable_unprepare(imx->clk_per);
 }
 
+static void imx_pwm_sw_reset(struct pwm_chip *chip)
+{
+	struct imx_chip *imx = to_imx_chip(chip);
+	struct device *dev = chip->dev;
+	int wait_count = 0;
+	u32 cr;
+
+	writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
+	do {
+		usleep_range(200, 1000);
+		cr = readl(imx->mmio_base + MX3_PWMCR);
+	} while ((cr & MX3_PWMCR_SWR) &&
+		 (wait_count++ < MX3_PWM_SWR_LOOP));
+
+	if (cr & MX3_PWMCR_SWR)
+		dev_warn(dev, "software reset timeout\n");
+}
+
+
 static int imx_pwm_config_v2(struct pwm_chip *chip,
 		struct pwm_device *pwm, int duty_ns, int period_ns)
 {
@@ -143,7 +162,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 	unsigned long period_cycles, duty_cycles, prescale;
 	unsigned int period_ms;
 	bool enable = pwm_is_enabled(pwm);
-	int wait_count = 0, fifoav;
+	int fifoav;
 	u32 cr, sr;
 
 	/*
@@ -166,15 +185,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 				dev_warn(dev, "there is no free FIFO slot\n");
 		}
 	} else {
-		writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
-		do {
-			usleep_range(200, 1000);
-			cr = readl(imx->mmio_base + MX3_PWMCR);
-		} while ((cr & MX3_PWMCR_SWR) &&
-			 (wait_count++ < MX3_PWM_SWR_LOOP));
-
-		if (cr & MX3_PWMCR_SWR)
-			dev_warn(dev, "software reset timeout\n");
+		imx_pwm_sw_reset(chip);
 	}
 
 	c = clk_get_rate(imx->clk_per);
-- 
2.1.4

[PATCH v2 05/10] pwm: imx: Move PWMv2 wait for fifo slot code to a separate function

[Lukasz Majewski]

The code, which waits for fifo slot, has been extracted from
imx_pwm_config_v2 function and moved to new one - imx_pwm_wait_fifo_slot().

This change reduces the overall size of imx_pwm_config_v2() and prepares
it for atomic PWM operation.

Suggested-by: Stefan Agner <stefan@agner.ch>
Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
Changes for v2:
- None
---
 drivers/pwm/pwm-imx.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 8174ae6..f7c7534 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -152,18 +152,36 @@ static void imx_pwm_sw_reset(struct pwm_chip *chip)
 		dev_warn(dev, "software reset timeout\n");
 }
 
+static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
+				   struct pwm_device *pwm)
+{
+	struct imx_chip *imx = to_imx_chip(chip);
+	struct device *dev = chip->dev;
+	unsigned int period_ms;
+	int fifoav;
+	u32 sr;
+
+	sr = readl(imx->mmio_base + MX3_PWMSR);
+	fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
+	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
+		period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
+					 NSEC_PER_MSEC);
+		msleep(period_ms);
+
+		sr = readl(imx->mmio_base + MX3_PWMSR);
+		if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
+			dev_warn(dev, "there is no free FIFO slot\n");
+	}
+}
 
 static int imx_pwm_config_v2(struct pwm_chip *chip,
 		struct pwm_device *pwm, int duty_ns, int period_ns)
 {
 	struct imx_chip *imx = to_imx_chip(chip);
-	struct device *dev = chip->dev;
 	unsigned long long c;
 	unsigned long period_cycles, duty_cycles, prescale;
-	unsigned int period_ms;
 	bool enable = pwm_is_enabled(pwm);
-	int fifoav;
-	u32 cr, sr;
+	u32 cr;
 
 	/*
 	 * i.MX PWMv2 has a 4-word sample FIFO.
@@ -172,21 +190,10 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 	 * wait for a full PWM cycle to get a relinquished FIFO slot
 	 * when the controller is enabled and the FIFO is fully loaded.
 	 */
-	if (enable) {
-		sr = readl(imx->mmio_base + MX3_PWMSR);
-		fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
-		if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
-			period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
-						 NSEC_PER_MSEC);
-			msleep(period_ms);
-
-			sr = readl(imx->mmio_base + MX3_PWMSR);
-			if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
-				dev_warn(dev, "there is no free FIFO slot\n");
-		}
-	} else {
+	if (enable)
+		imx_pwm_wait_fifo_slot(chip, pwm);
+	else
 		imx_pwm_sw_reset(chip);
-	}
 
 	c = clk_get_rate(imx->clk_per);
 	c = c * period_ns;
-- 
2.1.4

[PATCH v2 06/10] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Lukasz Majewski]

This commit provides apply() callback implementation for i.MX's PWMv2.

Suggested-by: Stefan Agner <stefan@agner.ch>
Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
Changes for v2:
- None
---
 drivers/pwm/pwm-imx.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index f7c7534..6b7d2f1 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -174,6 +174,81 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
 	}
 }
 
+static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
+			    struct pwm_state *state)
+{
+	unsigned long period_cycles, duty_cycles, prescale;
+	struct imx_chip *imx = to_imx_chip(chip);
+	struct pwm_state cstate;
+	unsigned long long c;
+	u32 cr = 0;
+	int ret;
+
+	ret = clk_prepare_enable(imx->clk_ipg);
+	if (ret)
+		return ret;
+
+	pwm_get_state(pwm, &cstate);
+
+	c = clk_get_rate(imx->clk_per);
+	c *= state->period;
+
+	do_div(c, 1000000000);
+	period_cycles = c;
+
+	prescale = period_cycles / 0x10000 + 1;
+
+	period_cycles /= prescale;
+	c = (unsigned long long)period_cycles * state->duty_cycle;
+	do_div(c, state->period);
+	duty_cycles = c;
+
+	/*
+	 * according to imx pwm RM, the real period value should be
+	 * PERIOD value in PWMPR plus 2.
+	 */
+	if (period_cycles > 2)
+		period_cycles -= 2;
+	else
+		period_cycles = 0;
+
+	/* Enable the clock if the PWM is being enabled. */
+	if (state->enabled && !cstate.enabled) {
+		ret = clk_prepare_enable(imx->clk_per);
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * Wait for a free FIFO slot if the PWM is already enabled, and flush
+	 * the FIFO if the PWM was disabled and is about to be enabled.
+	 */
+	if (cstate.enabled)
+		imx_pwm_wait_fifo_slot(chip, pwm);
+	else if (state->enabled)
+		imx_pwm_sw_reset(chip);
+
+	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
+
+	cr |= MX3_PWMCR_PRESCALER(prescale) |
+	      MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
+	      MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
+
+	if (state->enabled)
+		cr |= MX3_PWMCR_EN;
+
+	writel(cr, imx->mmio_base + MX3_PWMCR);
+
+	/* Disable the clock if the PWM is being disabled. */
+	if (!state->enabled && cstate.enabled)
+		clk_disable_unprepare(imx->clk_per);
+
+	clk_disable_unprepare(imx->clk_ipg);
+
+	return 0;
+}
+
 static int imx_pwm_config_v2(struct pwm_chip *chip,
 		struct pwm_device *pwm, int duty_ns, int period_ns)
 {
@@ -297,6 +372,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
 	.enable = imx_pwm_enable,
 	.disable = imx_pwm_disable,
 	.config = imx_pwm_config,
+	.apply = imx_pwm_apply_v2,
 	.owner = THIS_MODULE,
 };
 
-- 
2.1.4

[PATCH v2 07/10] pwm: imx: Remove redundant i.MX PWMv2 code

[Lukasz Majewski]

The code providing functionality surpassed by the atomic PWM is not needed
anymore and hence can be removed.

Suggested-by: Stefan Agner <stefan@agner.ch>
Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
Changes for v2:
- None
---
 drivers/pwm/pwm-imx.c | 127 --------------------------------------------------
 1 file changed, 127 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 6b7d2f1..0132439a 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -54,10 +54,6 @@ struct imx_chip {
 	void __iomem	*mmio_base;
 
 	struct pwm_chip	chip;
-
-	int (*config)(struct pwm_chip *chip,
-		struct pwm_device *pwm, int duty_ns, int period_ns);
-	void (*set_enable)(struct pwm_chip *chip, bool enable);
 };
 
 #define to_imx_chip(chip)	container_of(chip, struct imx_chip, chip)
@@ -249,118 +245,6 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
-static int imx_pwm_config_v2(struct pwm_chip *chip,
-		struct pwm_device *pwm, int duty_ns, int period_ns)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	unsigned long long c;
-	unsigned long period_cycles, duty_cycles, prescale;
-	bool enable = pwm_is_enabled(pwm);
-	u32 cr;
-
-	/*
-	 * i.MX PWMv2 has a 4-word sample FIFO.
-	 * In order to avoid FIFO overflow issue, we do software reset
-	 * to clear all sample FIFO if the controller is disabled or
-	 * wait for a full PWM cycle to get a relinquished FIFO slot
-	 * when the controller is enabled and the FIFO is fully loaded.
-	 */
-	if (enable)
-		imx_pwm_wait_fifo_slot(chip, pwm);
-	else
-		imx_pwm_sw_reset(chip);
-
-	c = clk_get_rate(imx->clk_per);
-	c = c * period_ns;
-	do_div(c, 1000000000);
-	period_cycles = c;
-
-	prescale = period_cycles / 0x10000 + 1;
-
-	period_cycles /= prescale;
-	c = (unsigned long long)period_cycles * duty_ns;
-	do_div(c, period_ns);
-	duty_cycles = c;
-
-	/*
-	 * according to imx pwm RM, the real period value should be
-	 * PERIOD value in PWMPR plus 2.
-	 */
-	if (period_cycles > 2)
-		period_cycles -= 2;
-	else
-		period_cycles = 0;
-
-	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
-	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
-
-	cr = MX3_PWMCR_PRESCALER(prescale) |
-		MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
-		MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
-
-	if (enable)
-		cr |= MX3_PWMCR_EN;
-
-	writel(cr, imx->mmio_base + MX3_PWMCR);
-
-	return 0;
-}
-
-static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	u32 val;
-
-	val = readl(imx->mmio_base + MX3_PWMCR);
-
-	if (enable)
-		val |= MX3_PWMCR_EN;
-	else
-		val &= ~MX3_PWMCR_EN;
-
-	writel(val, imx->mmio_base + MX3_PWMCR);
-}
-
-static int imx_pwm_config(struct pwm_chip *chip,
-		struct pwm_device *pwm, int duty_ns, int period_ns)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	int ret;
-
-	ret = clk_prepare_enable(imx->clk_ipg);
-	if (ret)
-		return ret;
-
-	ret = imx->config(chip, pwm, duty_ns, period_ns);
-
-	clk_disable_unprepare(imx->clk_ipg);
-
-	return ret;
-}
-
-static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	int ret;
-
-	ret = clk_prepare_enable(imx->clk_per);
-	if (ret)
-		return ret;
-
-	imx->set_enable(chip, true);
-
-	return 0;
-}
-
-static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-
-	imx->set_enable(chip, false);
-
-	clk_disable_unprepare(imx->clk_per);
-}
-
 static struct pwm_ops imx_pwm_ops_v1 = {
 	.enable = imx_pwm_enable_v1,
 	.disable = imx_pwm_disable_v1,
@@ -369,17 +253,11 @@ static struct pwm_ops imx_pwm_ops_v1 = {
 };
 
 static struct pwm_ops imx_pwm_ops_v2 = {
-	.enable = imx_pwm_enable,
-	.disable = imx_pwm_disable,
-	.config = imx_pwm_config,
 	.apply = imx_pwm_apply_v2,
 	.owner = THIS_MODULE,
 };
 
 struct imx_pwm_data {
-	int (*config)(struct pwm_chip *chip,
-		struct pwm_device *pwm, int duty_ns, int period_ns);
-	void (*set_enable)(struct pwm_chip *chip, bool enable);
 	struct pwm_ops *pwm_ops;
 };
 
@@ -388,8 +266,6 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
 };
 
 static struct imx_pwm_data imx_pwm_data_v2 = {
-	.config = imx_pwm_config_v2,
-	.set_enable = imx_pwm_set_enable_v2,
 	.pwm_ops = &imx_pwm_ops_v2,
 };
 
@@ -443,9 +319,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(imx->mmio_base))
 		return PTR_ERR(imx->mmio_base);
 
-	imx->config = data->config;
-	imx->set_enable = data->set_enable;
-
 	ret = pwmchip_add(&imx->chip);
 	if (ret < 0)
 		return ret;
-- 
2.1.4

[PATCH v2 08/10] pwm: core: make the PWM_POLARITY flag in DTB optional

[Lukasz Majewski]

From: Lothar Wassmann <LW@KARO-electronics.de>

Change the pwm chip driver registration, so that a chip driver that
supports polarity inversion can still be used with DTBs that don't
provide the 'PWM_POLARITY' flag.

This is done to provide polarity inversion support for the pwm-imx
driver without having to modify all existing DTS files.

Signed-off-by: Lothar Wassmann <LW@KARO-electronics.de>
Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
Changes for v2:
- None
---
 drivers/pwm/core.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index ec7179f..d80e5c5 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -137,9 +137,14 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
+	/* check, whether the driver supports a third cell for flags */
 	if (pc->of_pwm_n_cells < 3)
 		return ERR_PTR(-EINVAL);
 
+	/* flags in the third cell are optional */
+	if (args->args_count < 2)
+		return ERR_PTR(-EINVAL);
+
 	if (args->args[0] >= pc->npwm)
 		return ERR_PTR(-EINVAL);
 
@@ -148,11 +153,10 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 		return pwm;
 
 	pwm->args.period = args->args[1];
+	pwm->args.polarity = PWM_POLARITY_NORMAL;
 
-	if (args->args[2] & PWM_POLARITY_INVERTED)
+	if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED)
 		pwm->args.polarity = PWM_POLARITY_INVERSED;
-	else
-		pwm->args.polarity = PWM_POLARITY_NORMAL;
 
 	return pwm;
 }
@@ -163,9 +167,14 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
+	/* sanity check driver support */
 	if (pc->of_pwm_n_cells < 2)
 		return ERR_PTR(-EINVAL);
 
+	/* all cells are required */
+	if (args->args_count != pc->of_pwm_n_cells)
+		return ERR_PTR(-EINVAL);
+
 	if (args->args[0] >= pc->npwm)
 		return ERR_PTR(-EINVAL);
 
@@ -674,13 +683,6 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
 		goto put;
 	}
 
-	if (args.args_count != pc->of_pwm_n_cells) {
-		pr_debug("%s: wrong #pwm-cells for %s\n", np->full_name,
-			 args.np->full_name);
-		pwm = ERR_PTR(-EINVAL);
-		goto put;
-	}
-
 	pwm = pc->of_xlate(pc, &args);
 	if (IS_ERR(pwm))
 		goto put;
-- 
2.1.4

[PATCH v2 10/10] pwm: imx: Add polarity inversion support to i.MX's PWMv2

[Lukasz Majewski]

With this patch the polarity settings for i.MX's PWMv2 is now supported
on top of atomic PWM setting

Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
Changes for v2:
- New patch
---
 drivers/pwm/pwm-imx.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 0132439a..92b4ba0 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -38,6 +38,7 @@
 #define MX3_PWMCR_DOZEEN		(1 << 24)
 #define MX3_PWMCR_WAITEN		(1 << 23)
 #define MX3_PWMCR_DBGEN			(1 << 22)
+#define MX3_PWMCR_POUTC			(1 << 18)
 #define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
 #define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
 #define MX3_PWMCR_SWR			(1 << 3)
@@ -234,6 +235,9 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (state->enabled)
 		cr |= MX3_PWMCR_EN;
 
+	if (state->polarity == PWM_POLARITY_INVERSED)
+		cr |= MX3_PWMCR_POUTC;
+
 	writel(cr, imx->mmio_base + MX3_PWMCR);
 
 	/* Disable the clock if the PWM is being disabled. */
@@ -258,6 +262,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
 };
 
 struct imx_pwm_data {
+	bool polarity_supported;
 	struct pwm_ops *pwm_ops;
 };
 
@@ -266,6 +271,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
 };
 
 static struct imx_pwm_data imx_pwm_data_v2 = {
+	.polarity_supported = true,
 	.pwm_ops = &imx_pwm_ops_v2,
 };
 
@@ -313,6 +319,11 @@ static int imx_pwm_probe(struct platform_device *pdev)
 	imx->chip.base = -1;
 	imx->chip.npwm = 1;
 	imx->chip.can_sleep = true;
+	if (data->polarity_supported) {
+		dev_dbg(&pdev->dev, "PWM supports output inversion\n");
+		imx->chip.of_xlate = of_pwm_xlate_with_flags;
+		imx->chip.of_pwm_n_cells = 3;
+	}
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
-- 
2.1.4

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Boris Brezillon]

On Thu, 27 Oct 2016 08:29:39 +0200
Lukasz Majewski <l.majewski@majess.pl> wrote:

> The code has been rewritten to remove "generic" calls to
> imx_pwm_{enable|disable|config}.
> 
> Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> implementation.
> 
> Suggested-by: Stefan Agner <stefan@agner.ch>
> Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - Add missing clock unprepare for clk_ipg
> - Enable peripheral PWM clock (clk_per)
> ---
>  drivers/pwm/pwm-imx.c | 50 ++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index ea3ce79..822eb5a 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -65,8 +65,6 @@ struct imx_chip {
>  static int imx_pwm_config_v1(struct pwm_chip *chip,
>  		struct pwm_device *pwm, int duty_ns, int period_ns)
>  {
> -	struct imx_chip *imx = to_imx_chip(chip);
> -
>  	/*
>  	 * The PWM subsystem allows for exact frequencies. However,
>  	 * I cannot connect a scope on my device to the PWM line and
> @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
>  	 * both the prescaler (/1 .. /128) and then by CLKSEL
>  	 * (/2 .. /16).
>  	 */
> +	struct imx_chip *imx = to_imx_chip(chip);
>  	u32 max = readl(imx->mmio_base + MX1_PWMP);
>  	u32 p = max * duty_ns / period_ns;
> +	int ret;
> +
> +	ret = clk_prepare_enable(imx->clk_ipg);
> +	if (ret)
> +		return ret;
> +
>  	writel(max - p, imx->mmio_base + MX1_PWMS);
>  
> +	clk_disable_unprepare(imx->clk_ipg);
> +
>  	return 0;
>  }
>  
> -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct imx_chip *imx = to_imx_chip(chip);
> +	int ret;
>  	u32 val;
>  
> +	ret = clk_prepare_enable(imx->clk_ipg);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(imx->clk_per);
> +	if (ret)
> +		return ret;
> +
>  	val = readl(imx->mmio_base + MX1_PWMC);
> +	val |= MX1_PWMC_EN;
> +	writel(val, imx->mmio_base + MX1_PWMC);
>  
> -	if (enable)
> -		val |= MX1_PWMC_EN;
> -	else
> -		val &= ~MX1_PWMC_EN;
> +	clk_disable_unprepare(imx->clk_ipg);
> +
> +	return 0;
> +}
> +
> +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct imx_chip *imx = to_imx_chip(chip);
> +	u32 val;
> +
> +	val = readl(imx->mmio_base + MX1_PWMC);
> +	val &= ~MX1_PWMC_EN;
>  
>  	writel(val, imx->mmio_base + MX1_PWMC);

Are you sure you don't need to enable the ipg clk when manipulating the
PWMC register?
If it's not needed here, then it's probably not needed in
imx_pwm_enable_v1() either.

> +
> +	clk_disable_unprepare(imx->clk_per);
>  }
>  
>  static int imx_pwm_config_v2(struct pwm_chip *chip,
> @@ -241,9 +269,9 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  }
>  
>  static struct pwm_ops imx_pwm_ops_v1 = {
> -	.enable = imx_pwm_enable,
> -	.disable = imx_pwm_disable,
> -	.config = imx_pwm_config,
> +	.enable = imx_pwm_enable_v1,
> +	.disable = imx_pwm_disable_v1,
> +	.config = imx_pwm_config_v1,
>  	.owner = THIS_MODULE,
>  };
>  
> @@ -262,8 +290,6 @@ struct imx_pwm_data {
>  };
>  
>  static struct imx_pwm_data imx_pwm_data_v1 = {
> -	.config = imx_pwm_config_v1,
> -	.set_enable = imx_pwm_set_enable_v1,
>  	.pwm_ops = &imx_pwm_ops_v1,
>  };
>  

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Lukasz Majewski]

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

Hi Boris,

> On Thu, 27 Oct 2016 08:29:39 +0200
> Lukasz Majewski <l.majewski@majess.pl> wrote:
> 
> > The code has been rewritten to remove "generic" calls to
> > imx_pwm_{enable|disable|config}.
> > 
> > Such approach would facilitate switch to atomic PWM (a.k.a
> > ->apply()) implementation.
> > 
> > Suggested-by: Stefan Agner <stefan@agner.ch>
> > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > ---
> > Changes for v2:
> > - Add missing clock unprepare for clk_ipg
> > - Enable peripheral PWM clock (clk_per)
> > ---
> >  drivers/pwm/pwm-imx.c | 50
> > ++++++++++++++++++++++++++++++++++++++------------ 1 file changed,
> > 38 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index ea3ce79..822eb5a 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -65,8 +65,6 @@ struct imx_chip {
> >  static int imx_pwm_config_v1(struct pwm_chip *chip,
> >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> >  {
> > -	struct imx_chip *imx = to_imx_chip(chip);
> > -
> >  	/*
> >  	 * The PWM subsystem allows for exact frequencies. However,
> >  	 * I cannot connect a scope on my device to the PWM line
> > and @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct
> > pwm_chip *chip,
> >  	 * both the prescaler (/1 .. /128) and then by CLKSEL
> >  	 * (/2 .. /16).
> >  	 */
> > +	struct imx_chip *imx = to_imx_chip(chip);
> >  	u32 max = readl(imx->mmio_base + MX1_PWMP);
> >  	u32 p = max * duty_ns / period_ns;
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(imx->clk_ipg);
> > +	if (ret)
> > +		return ret;
> > +
> >  	writel(max - p, imx->mmio_base + MX1_PWMS);
> >  
> > +	clk_disable_unprepare(imx->clk_ipg);
> > +
> >  	return 0;
> >  }
> >  
> > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool
> > enable) +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct
> > pwm_device *pwm) {
> >  	struct imx_chip *imx = to_imx_chip(chip);
> > +	int ret;
> >  	u32 val;
> >  
> > +	ret = clk_prepare_enable(imx->clk_ipg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_prepare_enable(imx->clk_per);
> > +	if (ret)
> > +		return ret;
> > +
> >  	val = readl(imx->mmio_base + MX1_PWMC);
> > +	val |= MX1_PWMC_EN;
> > +	writel(val, imx->mmio_base + MX1_PWMC);
> >  
> > -	if (enable)
> > -		val |= MX1_PWMC_EN;
> > -	else
> > -		val &= ~MX1_PWMC_EN;
> > +	clk_disable_unprepare(imx->clk_ipg);
> > +
> > +	return 0;
> > +}
> > +
> > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct
> > pwm_device *pwm) +{
> > +	struct imx_chip *imx = to_imx_chip(chip);
> > +	u32 val;
> > +
> > +	val = readl(imx->mmio_base + MX1_PWMC);
> > +	val &= ~MX1_PWMC_EN;
> >  
> >  	writel(val, imx->mmio_base + MX1_PWMC);
> 
> Are you sure you don't need to enable the ipg clk when manipulating
> the PWMC register?
> If it's not needed here, then it's probably not needed in
> imx_pwm_enable_v1() either.

Yes, probably it is needed.

As I've mentioned in the cover letter - I do not have PWMv1 HW so I can
only compile test the code.

(And here support from the community is very welcome).

Best regards,
Łukasz Majewski

> 
> > +
> > +	clk_disable_unprepare(imx->clk_per);
> >  }
> >  
> >  static int imx_pwm_config_v2(struct pwm_chip *chip,
> > @@ -241,9 +269,9 @@ static void imx_pwm_disable(struct pwm_chip
> > *chip, struct pwm_device *pwm) }
> >  
> >  static struct pwm_ops imx_pwm_ops_v1 = {
> > -	.enable = imx_pwm_enable,
> > -	.disable = imx_pwm_disable,
> > -	.config = imx_pwm_config,
> > +	.enable = imx_pwm_enable_v1,
> > +	.disable = imx_pwm_disable_v1,
> > +	.config = imx_pwm_config_v1,
> >  	.owner = THIS_MODULE,
> >  };
> >  
> > @@ -262,8 +290,6 @@ struct imx_pwm_data {
> >  };
> >  
> >  static struct imx_pwm_data imx_pwm_data_v1 = {
> > -	.config = imx_pwm_config_v1,
> > -	.set_enable = imx_pwm_set_enable_v1,
> >  	.pwm_ops = &imx_pwm_ops_v1,
> >  };
> >  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Sascha Hauer]

On Thu, Oct 27, 2016 at 09:40:05AM +0200, Boris Brezillon wrote:
> On Thu, 27 Oct 2016 08:29:39 +0200
> Lukasz Majewski <l.majewski@majess.pl> wrote:
> 
> > The code has been rewritten to remove "generic" calls to
> > imx_pwm_{enable|disable|config}.
> > 
> > Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> > implementation.
> > 
> > Suggested-by: Stefan Agner <stefan@agner.ch>
> > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > ---
> > Changes for v2:
> > - Add missing clock unprepare for clk_ipg
> > - Enable peripheral PWM clock (clk_per)
> > ---
> >  drivers/pwm/pwm-imx.c | 50 ++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 38 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index ea3ce79..822eb5a 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -65,8 +65,6 @@ struct imx_chip {
> >  static int imx_pwm_config_v1(struct pwm_chip *chip,
> >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> >  {
> > -	struct imx_chip *imx = to_imx_chip(chip);
> > -
> >  	/*
> >  	 * The PWM subsystem allows for exact frequencies. However,
> >  	 * I cannot connect a scope on my device to the PWM line and
> > @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
> >  	 * both the prescaler (/1 .. /128) and then by CLKSEL
> >  	 * (/2 .. /16).
> >  	 */
> > +	struct imx_chip *imx = to_imx_chip(chip);
> >  	u32 max = readl(imx->mmio_base + MX1_PWMP);
> >  	u32 p = max * duty_ns / period_ns;
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(imx->clk_ipg);
> > +	if (ret)
> > +		return ret;
> > +
> >  	writel(max - p, imx->mmio_base + MX1_PWMS);
> >  
> > +	clk_disable_unprepare(imx->clk_ipg);
> > +
> >  	return 0;
> >  }
> >  
> > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> > +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> >  	struct imx_chip *imx = to_imx_chip(chip);
> > +	int ret;
> >  	u32 val;
> >  
> > +	ret = clk_prepare_enable(imx->clk_ipg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_prepare_enable(imx->clk_per);
> > +	if (ret)
> > +		return ret;
> > +
> >  	val = readl(imx->mmio_base + MX1_PWMC);
> > +	val |= MX1_PWMC_EN;
> > +	writel(val, imx->mmio_base + MX1_PWMC);
> >  
> > -	if (enable)
> > -		val |= MX1_PWMC_EN;
> > -	else
> > -		val &= ~MX1_PWMC_EN;
> > +	clk_disable_unprepare(imx->clk_ipg);
> > +
> > +	return 0;
> > +}
> > +
> > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct imx_chip *imx = to_imx_chip(chip);
> > +	u32 val;
> > +
> > +	val = readl(imx->mmio_base + MX1_PWMC);
> > +	val &= ~MX1_PWMC_EN;
> >  
> >  	writel(val, imx->mmio_base + MX1_PWMC);
> 
> Are you sure you don't need to enable the ipg clk when manipulating the
> PWMC register?
> If it's not needed here, then it's probably not needed in
> imx_pwm_enable_v1() either.

As said, even the commit 7b27c160c68 introducing the register clk did not
enable the clock consistently for all register accesses. Maybe it's best
to include the following patch so that we can find a clear culprit and
do not bury the ipg clock changes in larger patches.

Sascha

-----------------------------8<-----------------------------------

>From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Mon, 31 Oct 2016 06:45:33 +0100
Subject: [PATCH] pwm: imx: remove ipg clock

The use of the ipg clock was introduced with commit 7b27c160c6. In the
commit message it was claimed that the ipg clock is enabled for register
accesses. This is true for the ->config() callback, but not for the
->set_enable() callback. Given that the ipg clock is not consistently
enabled for all register accesses we can assume that either it is not
required at all or that the current code does not work.
Remove the ipg clock code for now so that it's no longer in the way of
refactoring the driver.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/pwm/pwm-imx.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index d600fd5..70609ef2 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -49,7 +49,6 @@
 
 struct imx_chip {
 	struct clk	*clk_per;
-	struct clk	*clk_ipg;
 
 	void __iomem	*mmio_base;
 
@@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
 		struct pwm_device *pwm, int duty_ns, int period_ns)
 {
 	struct imx_chip *imx = to_imx_chip(chip);
-	int ret;
-
-	ret = clk_prepare_enable(imx->clk_ipg);
-	if (ret)
-		return ret;
 
-	ret = imx->config(chip, pwm, duty_ns, period_ns);
-
-	clk_disable_unprepare(imx->clk_ipg);
-
-	return ret;
+	return imx->config(chip, pwm, duty_ns, period_ns);
 }
 
 static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
 		return PTR_ERR(imx->clk_per);
 	}
 
-	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
-	if (IS_ERR(imx->clk_ipg)) {
-		dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
-				PTR_ERR(imx->clk_ipg));
-		return PTR_ERR(imx->clk_ipg);
-	}
-
 	imx->chip.ops = &imx_pwm_ops;
 	imx->chip.dev = &pdev->dev;
 	imx->chip.base = -1;
-- 
2.9.3

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Sascha Hauer]

On Thu, Oct 27, 2016 at 09:40:05AM +0200, Boris Brezillon wrote:
> On Thu, 27 Oct 2016 08:29:39 +0200
> Lukasz Majewski <l.majewski@majess.pl> wrote:
> 
> > The code has been rewritten to remove "generic" calls to
> > imx_pwm_{enable|disable|config}.
> > 
> > Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> > implementation.
> > 
> > Suggested-by: Stefan Agner <stefan@agner.ch>
> > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > ---
> > Changes for v2:
> > - Add missing clock unprepare for clk_ipg
> > - Enable peripheral PWM clock (clk_per)
> > ---
> >  drivers/pwm/pwm-imx.c | 50 ++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 38 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index ea3ce79..822eb5a 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -65,8 +65,6 @@ struct imx_chip {
> >  static int imx_pwm_config_v1(struct pwm_chip *chip,
> >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> >  {
> > -	struct imx_chip *imx = to_imx_chip(chip);
> > -
> >  	/*
> >  	 * The PWM subsystem allows for exact frequencies. However,
> >  	 * I cannot connect a scope on my device to the PWM line and
> > @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
> >  	 * both the prescaler (/1 .. /128) and then by CLKSEL
> >  	 * (/2 .. /16).
> >  	 */
> > +	struct imx_chip *imx = to_imx_chip(chip);
> >  	u32 max = readl(imx->mmio_base + MX1_PWMP);
> >  	u32 p = max * duty_ns / period_ns;
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(imx->clk_ipg);
> > +	if (ret)
> > +		return ret;
> > +
> >  	writel(max - p, imx->mmio_base + MX1_PWMS);
> >  
> > +	clk_disable_unprepare(imx->clk_ipg);
> > +
> >  	return 0;
> >  }
> >  
> > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> > +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> >  	struct imx_chip *imx = to_imx_chip(chip);
> > +	int ret;
> >  	u32 val;
> >  
> > +	ret = clk_prepare_enable(imx->clk_ipg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_prepare_enable(imx->clk_per);
> > +	if (ret)
> > +		return ret;
> > +
> >  	val = readl(imx->mmio_base + MX1_PWMC);
> > +	val |= MX1_PWMC_EN;
> > +	writel(val, imx->mmio_base + MX1_PWMC);
> >  
> > -	if (enable)
> > -		val |= MX1_PWMC_EN;
> > -	else
> > -		val &= ~MX1_PWMC_EN;
> > +	clk_disable_unprepare(imx->clk_ipg);
> > +
> > +	return 0;
> > +}
> > +
> > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct imx_chip *imx = to_imx_chip(chip);
> > +	u32 val;
> > +
> > +	val = readl(imx->mmio_base + MX1_PWMC);
> > +	val &= ~MX1_PWMC_EN;
> >  
> >  	writel(val, imx->mmio_base + MX1_PWMC);
> 
> Are you sure you don't need to enable the ipg clk when manipulating the
> PWMC register?
> If it's not needed here, then it's probably not needed in
> imx_pwm_enable_v1() either.

As said, even the commit 7b27c160c68 introducing the register clk did not
enable the clock consistently for all register accesses. Maybe it's best
to include the following patch so that we can find a clear culprit and
do not bury the ipg clock changes in larger patches.

Sascha

-----------------------------8<-----------------------------------

From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Mon, 31 Oct 2016 06:45:33 +0100
Subject: [PATCH] pwm: imx: remove ipg clock

The use of the ipg clock was introduced with commit 7b27c160c6. In the
commit message it was claimed that the ipg clock is enabled for register
accesses. This is true for the ->config() callback, but not for the
->set_enable() callback. Given that the ipg clock is not consistently
enabled for all register accesses we can assume that either it is not
required at all or that the current code does not work.
Remove the ipg clock code for now so that it's no longer in the way of
refactoring the driver.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/pwm/pwm-imx.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index d600fd5..70609ef2 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -49,7 +49,6 @@
 
 struct imx_chip {
 	struct clk	*clk_per;
-	struct clk	*clk_ipg;
 
 	void __iomem	*mmio_base;
 
@@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
 		struct pwm_device *pwm, int duty_ns, int period_ns)
 {
 	struct imx_chip *imx = to_imx_chip(chip);
-	int ret;
-
-	ret = clk_prepare_enable(imx->clk_ipg);
-	if (ret)
-		return ret;
 
-	ret = imx->config(chip, pwm, duty_ns, period_ns);
-
-	clk_disable_unprepare(imx->clk_ipg);
-
-	return ret;
+	return imx->config(chip, pwm, duty_ns, period_ns);
 }
 
 static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
 		return PTR_ERR(imx->clk_per);
 	}
 
-	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
-	if (IS_ERR(imx->clk_ipg)) {
-		dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
-				PTR_ERR(imx->clk_ipg));
-		return PTR_ERR(imx->clk_ipg);
-	}
-
 	imx->chip.ops = &imx_pwm_ops;
 	imx->chip.dev = &pdev->dev;
 	imx->chip.base = -1;
-- 
2.9.3

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Lukasz Majewski]

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

Hi Sascha,

> On Thu, Oct 27, 2016 at 09:40:05AM +0200, Boris Brezillon wrote:
> > On Thu, 27 Oct 2016 08:29:39 +0200
> > Lukasz Majewski <l.majewski@majess.pl> wrote:
> > 
> > > The code has been rewritten to remove "generic" calls to
> > > imx_pwm_{enable|disable|config}.
> > > 
> > > Such approach would facilitate switch to atomic PWM (a.k.a
> > > ->apply()) implementation.
> > > 
> > > Suggested-by: Stefan Agner <stefan@agner.ch>
> > > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > > ---
> > > Changes for v2:
> > > - Add missing clock unprepare for clk_ipg
> > > - Enable peripheral PWM clock (clk_per)
> > > ---
> > >  drivers/pwm/pwm-imx.c | 50
> > > ++++++++++++++++++++++++++++++++++++++------------ 1 file
> > > changed, 38 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > index ea3ce79..822eb5a 100644
> > > --- a/drivers/pwm/pwm-imx.c
> > > +++ b/drivers/pwm/pwm-imx.c
> > > @@ -65,8 +65,6 @@ struct imx_chip {
> > >  static int imx_pwm_config_v1(struct pwm_chip *chip,
> > >  		struct pwm_device *pwm, int duty_ns, int
> > > period_ns) {
> > > -	struct imx_chip *imx = to_imx_chip(chip);
> > > -
> > >  	/*
> > >  	 * The PWM subsystem allows for exact frequencies.
> > > However,
> > >  	 * I cannot connect a scope on my device to the PWM line
> > > and @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct
> > > pwm_chip *chip,
> > >  	 * both the prescaler (/1 .. /128) and then by CLKSEL
> > >  	 * (/2 .. /16).
> > >  	 */
> > > +	struct imx_chip *imx = to_imx_chip(chip);
> > >  	u32 max = readl(imx->mmio_base + MX1_PWMP);
> > >  	u32 p = max * duty_ns / period_ns;
> > > +	int ret;
> > > +
> > > +	ret = clk_prepare_enable(imx->clk_ipg);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	writel(max - p, imx->mmio_base + MX1_PWMS);
> > >  
> > > +	clk_disable_unprepare(imx->clk_ipg);
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool
> > > enable) +static int imx_pwm_enable_v1(struct pwm_chip *chip,
> > > struct pwm_device *pwm) {
> > >  	struct imx_chip *imx = to_imx_chip(chip);
> > > +	int ret;
> > >  	u32 val;
> > >  
> > > +	ret = clk_prepare_enable(imx->clk_ipg);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = clk_prepare_enable(imx->clk_per);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	val = readl(imx->mmio_base + MX1_PWMC);
> > > +	val |= MX1_PWMC_EN;
> > > +	writel(val, imx->mmio_base + MX1_PWMC);
> > >  
> > > -	if (enable)
> > > -		val |= MX1_PWMC_EN;
> > > -	else
> > > -		val &= ~MX1_PWMC_EN;
> > > +	clk_disable_unprepare(imx->clk_ipg);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct
> > > pwm_device *pwm) +{
> > > +	struct imx_chip *imx = to_imx_chip(chip);
> > > +	u32 val;
> > > +
> > > +	val = readl(imx->mmio_base + MX1_PWMC);
> > > +	val &= ~MX1_PWMC_EN;
> > >  
> > >  	writel(val, imx->mmio_base + MX1_PWMC);
> > 
> > Are you sure you don't need to enable the ipg clk when manipulating
> > the PWMC register?
> > If it's not needed here, then it's probably not needed in
> > imx_pwm_enable_v1() either.
> 
> As said, even the commit 7b27c160c68 introducing the register clk did
> not enable the clock consistently for all register accesses. 

If I might ask - do you have i.MX hardware with PWMv1? If yes, I would
be grateful for testing (and provide proper patch), since I don't posses
one.

> Maybe
> it's best to include the following patch so that we can find a clear
> culprit 

If we don't have HW to test the solution - why should we apply this
patch and introduce regression?



If you can provide (and test) fix for v1 - please prepare patch, so it
could be added on top of this patch series (as done with pwm polarity
inversion in this patch series).


> and do not bury the ipg clock changes in larger patches.
> 
> Sascha
> 
> -----------------------------8<-----------------------------------
> 
> From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Mon, 31 Oct 2016 06:45:33 +0100
> Subject: [PATCH] pwm: imx: remove ipg clock
> 
> The use of the ipg clock was introduced with commit 7b27c160c6. In the
> commit message it was claimed that the ipg clock is enabled for
> register accesses. This is true for the ->config() callback, but not
> for the ->set_enable() callback. Given that the ipg clock is not
> consistently enabled for all register accesses we can assume that
> either it is not required at all or that the current code does not
> work. Remove the ipg clock code for now so that it's no longer in the
> way of refactoring the driver.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/pwm/pwm-imx.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d600fd5..70609ef2 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -49,7 +49,6 @@
>  
>  struct imx_chip {
>  	struct clk	*clk_per;
> -	struct clk	*clk_ipg;
>  
>  	void __iomem	*mmio_base;
>  
> @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
>  		struct pwm_device *pwm, int duty_ns, int period_ns)
>  {
>  	struct imx_chip *imx = to_imx_chip(chip);
> -	int ret;
> -
> -	ret = clk_prepare_enable(imx->clk_ipg);
> -	if (ret)
> -		return ret;
>  
> -	ret = imx->config(chip, pwm, duty_ns, period_ns);
> -
> -	clk_disable_unprepare(imx->clk_ipg);
> -
> -	return ret;
> +	return imx->config(chip, pwm, duty_ns, period_ns);
>  }
>  
>  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device
> *pwm) @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct
> platform_device *pdev) return PTR_ERR(imx->clk_per);
>  	}
>  
> -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> -	if (IS_ERR(imx->clk_ipg)) {
> -		dev_err(&pdev->dev, "getting ipg clock failed with
> %ld\n",
> -				PTR_ERR(imx->clk_ipg));
> -		return PTR_ERR(imx->clk_ipg);
> -	}

And in that way also v2 would be affected.

My gut feeling now is that the "community" wants to solve too many
issues with PWM atomic support rework. 

Why cannot we add patches on top of already done work, but require
large patch series to be rewritten and resend ?

> -
>  	imx->chip.ops = &imx_pwm_ops;
>  	imx->chip.dev = &pdev->dev;
>  	imx->chip.base = -1;

Best regards,

Łukasz Majewski

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Sascha Hauer]

On Mon, Oct 31, 2016 at 09:06:00AM +0100, Lukasz Majewski wrote:
> Hi Sascha,
> 
> > On Thu, Oct 27, 2016 at 09:40:05AM +0200, Boris Brezillon wrote:
> > > On Thu, 27 Oct 2016 08:29:39 +0200
> > > Lukasz Majewski <l.majewski@majess.pl> wrote:
> > > 
> > > > The code has been rewritten to remove "generic" calls to
> > > > imx_pwm_{enable|disable|config}.
> > > > 
> > > > Such approach would facilitate switch to atomic PWM (a.k.a
> > > > ->apply()) implementation.
> > > > 
> > > > Suggested-by: Stefan Agner <stefan@agner.ch>
> > > > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > > > ---
> > > > Changes for v2:
> > > > - Add missing clock unprepare for clk_ipg
> > > > - Enable peripheral PWM clock (clk_per)
> > > > ---
> > > >  drivers/pwm/pwm-imx.c | 50
> > > > ++++++++++++++++++++++++++++++++++++++------------ 1 file
> > > > changed, 38 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > index ea3ce79..822eb5a 100644
> > > > --- a/drivers/pwm/pwm-imx.c
> > > > +++ b/drivers/pwm/pwm-imx.c
> > > > @@ -65,8 +65,6 @@ struct imx_chip {
> > > >  static int imx_pwm_config_v1(struct pwm_chip *chip,
> > > >  		struct pwm_device *pwm, int duty_ns, int
> > > > period_ns) {
> > > > -	struct imx_chip *imx = to_imx_chip(chip);
> > > > -
> > > >  	/*
> > > >  	 * The PWM subsystem allows for exact frequencies.
> > > > However,
> > > >  	 * I cannot connect a scope on my device to the PWM line
> > > > and @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct
> > > > pwm_chip *chip,
> > > >  	 * both the prescaler (/1 .. /128) and then by CLKSEL
> > > >  	 * (/2 .. /16).
> > > >  	 */
> > > > +	struct imx_chip *imx = to_imx_chip(chip);
> > > >  	u32 max = readl(imx->mmio_base + MX1_PWMP);
> > > >  	u32 p = max * duty_ns / period_ns;
> > > > +	int ret;
> > > > +
> > > > +	ret = clk_prepare_enable(imx->clk_ipg);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > >  	writel(max - p, imx->mmio_base + MX1_PWMS);
> > > >  
> > > > +	clk_disable_unprepare(imx->clk_ipg);
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool
> > > > enable) +static int imx_pwm_enable_v1(struct pwm_chip *chip,
> > > > struct pwm_device *pwm) {
> > > >  	struct imx_chip *imx = to_imx_chip(chip);
> > > > +	int ret;
> > > >  	u32 val;
> > > >  
> > > > +	ret = clk_prepare_enable(imx->clk_ipg);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = clk_prepare_enable(imx->clk_per);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > >  	val = readl(imx->mmio_base + MX1_PWMC);
> > > > +	val |= MX1_PWMC_EN;
> > > > +	writel(val, imx->mmio_base + MX1_PWMC);
> > > >  
> > > > -	if (enable)
> > > > -		val |= MX1_PWMC_EN;
> > > > -	else
> > > > -		val &= ~MX1_PWMC_EN;
> > > > +	clk_disable_unprepare(imx->clk_ipg);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct
> > > > pwm_device *pwm) +{
> > > > +	struct imx_chip *imx = to_imx_chip(chip);
> > > > +	u32 val;
> > > > +
> > > > +	val = readl(imx->mmio_base + MX1_PWMC);
> > > > +	val &= ~MX1_PWMC_EN;
> > > >  
> > > >  	writel(val, imx->mmio_base + MX1_PWMC);
> > > 
> > > Are you sure you don't need to enable the ipg clk when manipulating
> > > the PWMC register?
> > > If it's not needed here, then it's probably not needed in
> > > imx_pwm_enable_v1() either.
> > 
> > As said, even the commit 7b27c160c68 introducing the register clk did
> > not enable the clock consistently for all register accesses. 
> 
> If I might ask - do you have i.MX hardware with PWMv1? If yes, I would
> be grateful for testing (and provide proper patch), since I don't posses
> one.

PWMv1 is only found on i.MX1. While I indeed have hardware for this I
don't want to spend the time to blow the dust from it and search for a
PWM output pin on that.
BTW. i.MX1 does not have a real ipg clock, the dts file registers the
dummy clock for it. So on PWMv1 hardware the ipg clock is not needed for
sure.


> 
> > Maybe
> > it's best to include the following patch so that we can find a clear
> > culprit 
> 
> If we don't have HW to test the solution - why should we apply this
> patch and introduce regression?

The current state does not handle the ipg clock properly, it's broken
already. So it's probably better to remove the inconsistent code rather
than to keep it and to introduce regressions step by step and in the end
leaving the question "How could this ever have worked"?

> 
> 
> 
> If you can provide (and test) fix for v1 - please prepare patch, so it
> could be added on top of this patch series (as done with pwm polarity
> inversion in this patch series).

As said, PWMv1 does not need the ipg clock.

> >  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device
> > *pwm) @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct
> > platform_device *pdev) return PTR_ERR(imx->clk_per);
> >  	}
> >  
> > -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > -	if (IS_ERR(imx->clk_ipg)) {
> > -		dev_err(&pdev->dev, "getting ipg clock failed with
> > %ld\n",
> > -				PTR_ERR(imx->clk_ipg));
> > -		return PTR_ERR(imx->clk_ipg);
> > -	}
> 
> And in that way also v2 would be affected.
> 
> My gut feeling now is that the "community" wants to solve too many
> issues with PWM atomic support rework.

I provided this patch with the assumption that you integrate it in your
series. It facilitates your series because with it you no longer have to
try to keep code working that hasn't worked before already.

> 
> Why cannot we add patches on top of already done work, but require
> large patch series to be rewritten and resend ?

You can, but that only works when the maintainer already has accepted
the patches you depend on, which currently he hasn't. If for some reason
the maintainer doesn't accept the patches you depend on, then you'll
have to rework your series aswell. It might also be simple enough for
the maintainer to merge both series rather than building one upon the
other.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Sascha Hauer]

On Mon, Oct 31, 2016 at 06:59:04AM +0100, Sascha Hauer wrote:
> On Thu, Oct 27, 2016 at 09:40:05AM +0200, Boris Brezillon wrote:
> > On Thu, 27 Oct 2016 08:29:39 +0200
> > Lukasz Majewski <l.majewski@majess.pl> wrote:
> > 
> > > The code has been rewritten to remove "generic" calls to
> > > imx_pwm_{enable|disable|config}.
> > > 
> > > Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> > > implementation.
> > > 
> > > Suggested-by: Stefan Agner <stefan@agner.ch>
> > > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > > ---
> > > Changes for v2:
> > > - Add missing clock unprepare for clk_ipg
> > > - Enable peripheral PWM clock (clk_per)
> > > ---
> > >  drivers/pwm/pwm-imx.c | 50 ++++++++++++++++++++++++++++++++++++++------------
> > >  1 file changed, 38 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > index ea3ce79..822eb5a 100644
> > > --- a/drivers/pwm/pwm-imx.c
> > > +++ b/drivers/pwm/pwm-imx.c
> > > @@ -65,8 +65,6 @@ struct imx_chip {
> > >  static int imx_pwm_config_v1(struct pwm_chip *chip,
> > >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> > >  {
> > > -	struct imx_chip *imx = to_imx_chip(chip);
> > > -
> > >  	/*
> > >  	 * The PWM subsystem allows for exact frequencies. However,
> > >  	 * I cannot connect a scope on my device to the PWM line and
> > > @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
> > >  	 * both the prescaler (/1 .. /128) and then by CLKSEL
> > >  	 * (/2 .. /16).
> > >  	 */
> > > +	struct imx_chip *imx = to_imx_chip(chip);
> > >  	u32 max = readl(imx->mmio_base + MX1_PWMP);
> > >  	u32 p = max * duty_ns / period_ns;
> > > +	int ret;
> > > +
> > > +	ret = clk_prepare_enable(imx->clk_ipg);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	writel(max - p, imx->mmio_base + MX1_PWMS);
> > >  
> > > +	clk_disable_unprepare(imx->clk_ipg);
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> > > +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> > >  {
> > >  	struct imx_chip *imx = to_imx_chip(chip);
> > > +	int ret;
> > >  	u32 val;
> > >  
> > > +	ret = clk_prepare_enable(imx->clk_ipg);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = clk_prepare_enable(imx->clk_per);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	val = readl(imx->mmio_base + MX1_PWMC);
> > > +	val |= MX1_PWMC_EN;
> > > +	writel(val, imx->mmio_base + MX1_PWMC);
> > >  
> > > -	if (enable)
> > > -		val |= MX1_PWMC_EN;
> > > -	else
> > > -		val &= ~MX1_PWMC_EN;
> > > +	clk_disable_unprepare(imx->clk_ipg);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> > > +{
> > > +	struct imx_chip *imx = to_imx_chip(chip);
> > > +	u32 val;
> > > +
> > > +	val = readl(imx->mmio_base + MX1_PWMC);
> > > +	val &= ~MX1_PWMC_EN;
> > >  
> > >  	writel(val, imx->mmio_base + MX1_PWMC);
> > 
> > Are you sure you don't need to enable the ipg clk when manipulating the
> > PWMC register?
> > If it's not needed here, then it's probably not needed in
> > imx_pwm_enable_v1() either.
> 
> As said, even the commit 7b27c160c68 introducing the register clk did not
> enable the clock consistently for all register accesses. Maybe it's best
> to include the following patch so that we can find a clear culprit and
> do not bury the ipg clock changes in larger patches.
> 
> Sascha
> 
> -----------------------------8<-----------------------------------
> 
> From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Mon, 31 Oct 2016 06:45:33 +0100
> Subject: [PATCH] pwm: imx: remove ipg clock
> 
> The use of the ipg clock was introduced with commit 7b27c160c6. In the
> commit message it was claimed that the ipg clock is enabled for register
> accesses. This is true for the ->config() callback, but not for the
> ->set_enable() callback. Given that the ipg clock is not consistently
> enabled for all register accesses we can assume that either it is not
> required at all or that the current code does not work.
> Remove the ipg clock code for now so that it's no longer in the way of
> refactoring the driver.

For reference:

I verified on i.MX53 and i.MX25 that the ipg clock provided to the pwm
driver is not needed when accessing registers. I would have to verify
that on i.MX27 aswell, but I do not have a board handy at the moment.

The current assumption as discussed by Philipp and me is that the ipg
clk is only needed when the pwm output is driven by the ipg clk
(MX3_PWMCR[16:17] = MX3_PWMCR_CLKSRC_IPG)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Lukasz Majewski]

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

Hi Sascha,

> On Mon, Oct 31, 2016 at 06:59:04AM +0100, Sascha Hauer wrote:
> > On Thu, Oct 27, 2016 at 09:40:05AM +0200, Boris Brezillon wrote:
> > > On Thu, 27 Oct 2016 08:29:39 +0200
> > > Lukasz Majewski <l.majewski@majess.pl> wrote:
> > > 
> > > > The code has been rewritten to remove "generic" calls to
> > > > imx_pwm_{enable|disable|config}.
> > > > 
> > > > Such approach would facilitate switch to atomic PWM (a.k.a
> > > > ->apply()) implementation.
> > > > 
> > > > Suggested-by: Stefan Agner <stefan@agner.ch>
> > > > Suggested-by: Boris Brezillon
> > > > <boris.brezillon@free-electrons.com> Signed-off-by: Lukasz
> > > > Majewski <l.majewski@majess.pl> ---
> > > > Changes for v2:
> > > > - Add missing clock unprepare for clk_ipg
> > > > - Enable peripheral PWM clock (clk_per)
> > > > ---
> > > >  drivers/pwm/pwm-imx.c | 50
> > > > ++++++++++++++++++++++++++++++++++++++------------ 1 file
> > > > changed, 38 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > index ea3ce79..822eb5a 100644
> > > > --- a/drivers/pwm/pwm-imx.c
> > > > +++ b/drivers/pwm/pwm-imx.c
> > > > @@ -65,8 +65,6 @@ struct imx_chip {
> > > >  static int imx_pwm_config_v1(struct pwm_chip *chip,
> > > >  		struct pwm_device *pwm, int duty_ns, int
> > > > period_ns) {
> > > > -	struct imx_chip *imx = to_imx_chip(chip);
> > > > -
> > > >  	/*
> > > >  	 * The PWM subsystem allows for exact frequencies.
> > > > However,
> > > >  	 * I cannot connect a scope on my device to the PWM
> > > > line and @@ -84,26 +82,56 @@ static int
> > > > imx_pwm_config_v1(struct pwm_chip *chip,
> > > >  	 * both the prescaler (/1 .. /128) and then by CLKSEL
> > > >  	 * (/2 .. /16).
> > > >  	 */
> > > > +	struct imx_chip *imx = to_imx_chip(chip);
> > > >  	u32 max = readl(imx->mmio_base + MX1_PWMP);
> > > >  	u32 p = max * duty_ns / period_ns;
> > > > +	int ret;
> > > > +
> > > > +	ret = clk_prepare_enable(imx->clk_ipg);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > >  	writel(max - p, imx->mmio_base + MX1_PWMS);
> > > >  
> > > > +	clk_disable_unprepare(imx->clk_ipg);
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool
> > > > enable) +static int imx_pwm_enable_v1(struct pwm_chip *chip,
> > > > struct pwm_device *pwm) {
> > > >  	struct imx_chip *imx = to_imx_chip(chip);
> > > > +	int ret;
> > > >  	u32 val;
> > > >  
> > > > +	ret = clk_prepare_enable(imx->clk_ipg);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = clk_prepare_enable(imx->clk_per);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > >  	val = readl(imx->mmio_base + MX1_PWMC);
> > > > +	val |= MX1_PWMC_EN;
> > > > +	writel(val, imx->mmio_base + MX1_PWMC);
> > > >  
> > > > -	if (enable)
> > > > -		val |= MX1_PWMC_EN;
> > > > -	else
> > > > -		val &= ~MX1_PWMC_EN;
> > > > +	clk_disable_unprepare(imx->clk_ipg);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct
> > > > pwm_device *pwm) +{
> > > > +	struct imx_chip *imx = to_imx_chip(chip);
> > > > +	u32 val;
> > > > +
> > > > +	val = readl(imx->mmio_base + MX1_PWMC);
> > > > +	val &= ~MX1_PWMC_EN;
> > > >  
> > > >  	writel(val, imx->mmio_base + MX1_PWMC);
> > > 
> > > Are you sure you don't need to enable the ipg clk when
> > > manipulating the PWMC register?
> > > If it's not needed here, then it's probably not needed in
> > > imx_pwm_enable_v1() either.
> > 
> > As said, even the commit 7b27c160c68 introducing the register clk
> > did not enable the clock consistently for all register accesses.
> > Maybe it's best to include the following patch so that we can find
> > a clear culprit and do not bury the ipg clock changes in larger
> > patches.
> > 
> > Sascha
> > 
> > -----------------------------8<-----------------------------------
> > 
> > From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00
> > 2001 From: Sascha Hauer <s.hauer@pengutronix.de>
> > Date: Mon, 31 Oct 2016 06:45:33 +0100
> > Subject: [PATCH] pwm: imx: remove ipg clock
> > 
> > The use of the ipg clock was introduced with commit 7b27c160c6. In
> > the commit message it was claimed that the ipg clock is enabled for
> > register accesses. This is true for the ->config() callback, but
> > not for the ->set_enable() callback. Given that the ipg clock is
> > not consistently enabled for all register accesses we can assume
> > that either it is not required at all or that the current code does
> > not work. Remove the ipg clock code for now so that it's no longer
> > in the way of refactoring the driver.
> 
> For reference:
> 
> I verified on i.MX53 and i.MX25 that the ipg clock provided to the pwm
> driver is not needed when accessing registers.

In the v3 of the patch series (almost done) I can confirm that i.MX6q
works without ipg clock manipulation to access registers.

> I would have to verify
> that on i.MX27 aswell, but I do not have a board handy at the moment.
> 
> The current assumption as discussed by Philipp and me is that the ipg
> clk is only needed when the pwm output is driven by the ipg clk
> (MX3_PWMCR[16:17] = MX3_PWMCR_CLKSRC_IPG)

Interresting .... I must check if I'm able to test this on my (rather)
not accessible HW.

Best regards,
Łukasz Majewski

> 
> Sascha
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Lukasz Majewski]

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

Hi Sascha,

> The current assumption as discussed by Philipp and me is that the ipg
> clk is only needed when the pwm output is driven by the ipg clk
> (MX3_PWMCR[16:17] = MX3_PWMCR_CLKSRC_IPG)

At least on my setup (i.MX6q) the ipg clock (ipg_clk) don't need to be
explicitly enabled in the ->apply() callback (in the pwm-imx.c) when
MX3_PWMCR_CLKSRC_IPG (0x01 - ipg_clk) is selected as the PWM source.


Best regards,
Łukasz Majewski

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Sascha Hauer]

On Tue, Nov 01, 2016 at 06:57:23AM +0100, Lukasz Majewski wrote:
> Hi Sascha,
> 
> > The current assumption as discussed by Philipp and me is that the ipg
> > clk is only needed when the pwm output is driven by the ipg clk
> > (MX3_PWMCR[16:17] = MX3_PWMCR_CLKSRC_IPG)
> 
> At least on my setup (i.MX6q) the ipg clock (ipg_clk) don't need to be
> explicitly enabled in the ->apply() callback (in the pwm-imx.c) when
> MX3_PWMCR_CLKSRC_IPG (0x01 - ipg_clk) is selected as the PWM source.

No. If you look in the device tree you'll see that there is no special
gateable ipg clock for the PWM. Instead the SoC ipg clock is registered
for the PWM which is not gateable.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Lukasz Majewski]

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

Hi Sascha

> On Tue, Nov 01, 2016 at 06:57:23AM +0100, Lukasz Majewski wrote:
> > Hi Sascha,
> > 
> > > The current assumption as discussed by Philipp and me is that the
> > > ipg clk is only needed when the pwm output is driven by the ipg
> > > clk (MX3_PWMCR[16:17] = MX3_PWMCR_CLKSRC_IPG)
> > 
> > At least on my setup (i.MX6q) the ipg clock (ipg_clk) don't need to
> > be explicitly enabled in the ->apply() callback (in the pwm-imx.c)
> > when MX3_PWMCR_CLKSRC_IPG (0x01 - ipg_clk) is selected as the PWM
> > source.
> 
> No. If you look in the device tree you'll see that there is no special
> gateable ipg clock for the PWM. Instead the SoC ipg clock is
> registered for the PWM which is not gateable.

I do understand that the goal is to enable ipg clock only on demand
(when we access registers) and just wanted to say that the approach with
ipg enabled in dts works on my setup (and for now is sufficient).

I suppose that ipg gating support for PWM will be provided in a separate
patch.

> 
> Sascha
> 

Best regards,

Łukasz Majewski

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Sascha Hauer]

On Mon, Oct 31, 2016 at 10:29:37AM +0100, Sascha Hauer wrote:
> > accesses. This is true for the ->config() callback, but not for the
> > ->set_enable() callback. Given that the ipg clock is not consistently
> > enabled for all register accesses we can assume that either it is not
> > required at all or that the current code does not work.
> > Remove the ipg clock code for now so that it's no longer in the way of
> > refactoring the driver.
> 
> For reference:
> 
> I verified on i.MX53 and i.MX25 that the ipg clock provided to the pwm
> driver is not needed when accessing registers. I would have to verify
> that on i.MX27 aswell, but I do not have a board handy at the moment.

Also on i.MX27 disabling the PWMs ipg_clk does not disable register
accesses.

Sascha

> 
> The current assumption as discussed by Philipp and me is that the ipg
> clk is only needed when the pwm output is driven by the ipg clk
> (MX3_PWMCR[16:17] = MX3_PWMCR_CLKSRC_IPG)
> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Lothar Waßmann]

Hi,

On Mon, 31 Oct 2016 06:59:04 +0100 Sascha Hauer wrote:
> As said, even the commit 7b27c160c68 introducing the register clk did not
> enable the clock consistently for all register accesses. Maybe it's best
> to include the following patch so that we can find a clear culprit and
> do not bury the ipg clock changes in larger patches.
> 
> Sascha
> 
> -----------------------------8<-----------------------------------
> 
> From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Mon, 31 Oct 2016 06:45:33 +0100
> Subject: [PATCH] pwm: imx: remove ipg clock
> 
> The use of the ipg clock was introduced with commit 7b27c160c6. In the
> commit message it was claimed that the ipg clock is enabled for register
> accesses. This is true for the ->config() callback, but not for the
> ->set_enable() callback. Given that the ipg clock is not consistently
> enabled for all register accesses we can assume that either it is not
> required at all or that the current code does not work.
> Remove the ipg clock code for now so that it's no longer in the way of
> refactoring the driver.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/pwm/pwm-imx.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d600fd5..70609ef2 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -49,7 +49,6 @@
>  
>  struct imx_chip {
>  	struct clk	*clk_per;
> -	struct clk	*clk_ipg;
>  
>  	void __iomem	*mmio_base;
>  
> @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
>  		struct pwm_device *pwm, int duty_ns, int period_ns)
>  {
>  	struct imx_chip *imx = to_imx_chip(chip);
> -	int ret;
> -
> -	ret = clk_prepare_enable(imx->clk_ipg);
> -	if (ret)
> -		return ret;
>  
> -	ret = imx->config(chip, pwm, duty_ns, period_ns);
> -
> -	clk_disable_unprepare(imx->clk_ipg);
> -
> -	return ret;
> +	return imx->config(chip, pwm, duty_ns, period_ns);
>  }
>  
>  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
>  		return PTR_ERR(imx->clk_per);
>  	}
>  
> -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> -	if (IS_ERR(imx->clk_ipg)) {
> -		dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> -				PTR_ERR(imx->clk_ipg));
> -		return PTR_ERR(imx->clk_ipg);
> -	}
> -
>  	imx->chip.ops = &imx_pwm_ops;
>  	imx->chip.dev = &pdev->dev;
>  	imx->chip.base = -1;
>
If the IPG clock is not needed by the driver it should be removed from
DT as well.



Lothar Waßmann

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Sascha Hauer]

On Wed, Nov 02, 2016 at 08:18:52AM +0100, Lothar Waßmann wrote:
> Hi,
> 
> On Mon, 31 Oct 2016 06:59:04 +0100 Sascha Hauer wrote:
> > As said, even the commit 7b27c160c68 introducing the register clk did not
> > enable the clock consistently for all register accesses. Maybe it's best
> > to include the following patch so that we can find a clear culprit and
> > do not bury the ipg clock changes in larger patches.
> > 
> > Sascha
> > 
> > -----------------------------8<-----------------------------------
> > 
> > From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > Date: Mon, 31 Oct 2016 06:45:33 +0100
> > Subject: [PATCH] pwm: imx: remove ipg clock
> > 
> > The use of the ipg clock was introduced with commit 7b27c160c6. In the
> > commit message it was claimed that the ipg clock is enabled for register
> > accesses. This is true for the ->config() callback, but not for the
> > ->set_enable() callback. Given that the ipg clock is not consistently
> > enabled for all register accesses we can assume that either it is not
> > required at all or that the current code does not work.
> > Remove the ipg clock code for now so that it's no longer in the way of
> > refactoring the driver.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/pwm/pwm-imx.c | 19 +------------------
> >  1 file changed, 1 insertion(+), 18 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index d600fd5..70609ef2 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -49,7 +49,6 @@
> >  
> >  struct imx_chip {
> >  	struct clk	*clk_per;
> > -	struct clk	*clk_ipg;
> >  
> >  	void __iomem	*mmio_base;
> >  
> > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
> >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> >  {
> >  	struct imx_chip *imx = to_imx_chip(chip);
> > -	int ret;
> > -
> > -	ret = clk_prepare_enable(imx->clk_ipg);
> > -	if (ret)
> > -		return ret;
> >  
> > -	ret = imx->config(chip, pwm, duty_ns, period_ns);
> > -
> > -	clk_disable_unprepare(imx->clk_ipg);
> > -
> > -	return ret;
> > +	return imx->config(chip, pwm, duty_ns, period_ns);
> >  }
> >  
> >  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
> >  		return PTR_ERR(imx->clk_per);
> >  	}
> >  
> > -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > -	if (IS_ERR(imx->clk_ipg)) {
> > -		dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> > -				PTR_ERR(imx->clk_ipg));
> > -		return PTR_ERR(imx->clk_ipg);
> > -	}
> > -
> >  	imx->chip.ops = &imx_pwm_ops;
> >  	imx->chip.dev = &pdev->dev;
> >  	imx->chip.base = -1;
> >
> If the IPG clock is not needed by the driver it should be removed from
> DT as well.

No, it's only the half truth that it's not needed. It would indeed be
needed if the driver used the ipg clock as source for the PWM (PWMCR[17:16] = 0b01).
The driver currently doesn't do this, so it doesn't need the clock. We
should still leave the clocks in the dts files in case we decide to use
that clock later.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Lothar Waßmann]

Hi,

On Wed, 2 Nov 2016 08:36:14 +0100 Sascha Hauer wrote:
> On Wed, Nov 02, 2016 at 08:18:52AM +0100, Lothar Waßmann wrote:
> > Hi,
> > 
> > On Mon, 31 Oct 2016 06:59:04 +0100 Sascha Hauer wrote:
> > > As said, even the commit 7b27c160c68 introducing the register clk did not
> > > enable the clock consistently for all register accesses. Maybe it's best
> > > to include the following patch so that we can find a clear culprit and
> > > do not bury the ipg clock changes in larger patches.
> > > 
> > > Sascha
> > > 
> > > -----------------------------8<-----------------------------------
> > > 
> > > From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
> > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > Date: Mon, 31 Oct 2016 06:45:33 +0100
> > > Subject: [PATCH] pwm: imx: remove ipg clock
> > > 
> > > The use of the ipg clock was introduced with commit 7b27c160c6. In the
> > > commit message it was claimed that the ipg clock is enabled for register
> > > accesses. This is true for the ->config() callback, but not for the
> > > ->set_enable() callback. Given that the ipg clock is not consistently
> > > enabled for all register accesses we can assume that either it is not
> > > required at all or that the current code does not work.
> > > Remove the ipg clock code for now so that it's no longer in the way of
> > > refactoring the driver.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > >  drivers/pwm/pwm-imx.c | 19 +------------------
> > >  1 file changed, 1 insertion(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > index d600fd5..70609ef2 100644
> > > --- a/drivers/pwm/pwm-imx.c
> > > +++ b/drivers/pwm/pwm-imx.c
> > > @@ -49,7 +49,6 @@
> > >  
> > >  struct imx_chip {
> > >  	struct clk	*clk_per;
> > > -	struct clk	*clk_ipg;
> > >  
> > >  	void __iomem	*mmio_base;
> > >  
> > > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
> > >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> > >  {
> > >  	struct imx_chip *imx = to_imx_chip(chip);
> > > -	int ret;
> > > -
> > > -	ret = clk_prepare_enable(imx->clk_ipg);
> > > -	if (ret)
> > > -		return ret;
> > >  
> > > -	ret = imx->config(chip, pwm, duty_ns, period_ns);
> > > -
> > > -	clk_disable_unprepare(imx->clk_ipg);
> > > -
> > > -	return ret;
> > > +	return imx->config(chip, pwm, duty_ns, period_ns);
> > >  }
> > >  
> > >  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
> > >  		return PTR_ERR(imx->clk_per);
> > >  	}
> > >  
> > > -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > > -	if (IS_ERR(imx->clk_ipg)) {
> > > -		dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> > > -				PTR_ERR(imx->clk_ipg));
> > > -		return PTR_ERR(imx->clk_ipg);
> > > -	}
> > > -
> > >  	imx->chip.ops = &imx_pwm_ops;
> > >  	imx->chip.dev = &pdev->dev;
> > >  	imx->chip.base = -1;
> > >
> > If the IPG clock is not needed by the driver it should be removed from
> > DT as well.
> 
> No, it's only the half truth that it's not needed. It would indeed be
> needed if the driver used the ipg clock as source for the PWM (PWMCR[17:16] = 0b01).
>
That's a different story!
Currently the DT specifies two clocks for the PWM:
1. register access clock (which we now know is unnecessary)
2. PWM source clock
In the case mentioned above, the IPG clock has to be specified as the
SECOND clock entry in DT, because otherwise the clock won't be
enabled/disabled as required!



Lothar Waßmann

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Sascha Hauer]

On Wed, Nov 02, 2016 at 08:56:20AM +0100, Lothar Waßmann wrote:
> Hi,
> 
> On Wed, 2 Nov 2016 08:36:14 +0100 Sascha Hauer wrote:
> > On Wed, Nov 02, 2016 at 08:18:52AM +0100, Lothar Waßmann wrote:
> > > Hi,
> > > 
> > > On Mon, 31 Oct 2016 06:59:04 +0100 Sascha Hauer wrote:
> > > > As said, even the commit 7b27c160c68 introducing the register clk did not
> > > > enable the clock consistently for all register accesses. Maybe it's best
> > > > to include the following patch so that we can find a clear culprit and
> > > > do not bury the ipg clock changes in larger patches.
> > > > 
> > > > Sascha
> > > > 
> > > > -----------------------------8<-----------------------------------
> > > > 
> > > > From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
> > > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Date: Mon, 31 Oct 2016 06:45:33 +0100
> > > > Subject: [PATCH] pwm: imx: remove ipg clock
> > > > 
> > > > The use of the ipg clock was introduced with commit 7b27c160c6. In the
> > > > commit message it was claimed that the ipg clock is enabled for register
> > > > accesses. This is true for the ->config() callback, but not for the
> > > > ->set_enable() callback. Given that the ipg clock is not consistently
> > > > enabled for all register accesses we can assume that either it is not
> > > > required at all or that the current code does not work.
> > > > Remove the ipg clock code for now so that it's no longer in the way of
> > > > refactoring the driver.
> > > > 
> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > > ---
> > > >  drivers/pwm/pwm-imx.c | 19 +------------------
> > > >  1 file changed, 1 insertion(+), 18 deletions(-)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > index d600fd5..70609ef2 100644
> > > > --- a/drivers/pwm/pwm-imx.c
> > > > +++ b/drivers/pwm/pwm-imx.c
> > > > @@ -49,7 +49,6 @@
> > > >  
> > > >  struct imx_chip {
> > > >  	struct clk	*clk_per;
> > > > -	struct clk	*clk_ipg;
> > > >  
> > > >  	void __iomem	*mmio_base;
> > > >  
> > > > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
> > > >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> > > >  {
> > > >  	struct imx_chip *imx = to_imx_chip(chip);
> > > > -	int ret;
> > > > -
> > > > -	ret = clk_prepare_enable(imx->clk_ipg);
> > > > -	if (ret)
> > > > -		return ret;
> > > >  
> > > > -	ret = imx->config(chip, pwm, duty_ns, period_ns);
> > > > -
> > > > -	clk_disable_unprepare(imx->clk_ipg);
> > > > -
> > > > -	return ret;
> > > > +	return imx->config(chip, pwm, duty_ns, period_ns);
> > > >  }
> > > >  
> > > >  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > > @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
> > > >  		return PTR_ERR(imx->clk_per);
> > > >  	}
> > > >  
> > > > -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > > > -	if (IS_ERR(imx->clk_ipg)) {
> > > > -		dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> > > > -				PTR_ERR(imx->clk_ipg));
> > > > -		return PTR_ERR(imx->clk_ipg);
> > > > -	}
> > > > -
> > > >  	imx->chip.ops = &imx_pwm_ops;
> > > >  	imx->chip.dev = &pdev->dev;
> > > >  	imx->chip.base = -1;
> > > >
> > > If the IPG clock is not needed by the driver it should be removed from
> > > DT as well.
> > 
> > No, it's only the half truth that it's not needed. It would indeed be
> > needed if the driver used the ipg clock as source for the PWM (PWMCR[17:16] = 0b01).
> >
> That's a different story!
> Currently the DT specifies two clocks for the PWM:
> 1. register access clock (which we now know is unnecessary)
> 2. PWM source clock
> In the case mentioned above, the IPG clock has to be specified as the
> SECOND clock entry in DT, because otherwise the clock won't be
> enabled/disabled as required!

Since the driver gets its clock by name (clk_get(&pdev->dev, "per"/"ipg"))
the position in the DT doesn't matter at all.

The only thing that isn't accurate is that the "ipg" clock in the device
tree is not for register access, but itself a clock to be used as PWM
source. This is no functional problem though.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Lothar Waßmann]

Hi,

On Wed, 2 Nov 2016 09:06:45 +0100 Sascha Hauer wrote:
> On Wed, Nov 02, 2016 at 08:56:20AM +0100, Lothar Waßmann wrote:
> > Hi,
> > 
> > On Wed, 2 Nov 2016 08:36:14 +0100 Sascha Hauer wrote:
> > > On Wed, Nov 02, 2016 at 08:18:52AM +0100, Lothar Waßmann wrote:
> > > > Hi,
> > > > 
> > > > On Mon, 31 Oct 2016 06:59:04 +0100 Sascha Hauer wrote:
> > > > > As said, even the commit 7b27c160c68 introducing the register clk did not
> > > > > enable the clock consistently for all register accesses. Maybe it's best
> > > > > to include the following patch so that we can find a clear culprit and
> > > > > do not bury the ipg clock changes in larger patches.
> > > > > 
> > > > > Sascha
> > > > > 
> > > > > -----------------------------8<-----------------------------------
> > > > > 
> > > > > From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
> > > > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > Date: Mon, 31 Oct 2016 06:45:33 +0100
> > > > > Subject: [PATCH] pwm: imx: remove ipg clock
> > > > > 
> > > > > The use of the ipg clock was introduced with commit 7b27c160c6. In the
> > > > > commit message it was claimed that the ipg clock is enabled for register
> > > > > accesses. This is true for the ->config() callback, but not for the
> > > > > ->set_enable() callback. Given that the ipg clock is not consistently
> > > > > enabled for all register accesses we can assume that either it is not
> > > > > required at all or that the current code does not work.
> > > > > Remove the ipg clock code for now so that it's no longer in the way of
> > > > > refactoring the driver.
> > > > > 
> > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > > > ---
> > > > >  drivers/pwm/pwm-imx.c | 19 +------------------
> > > > >  1 file changed, 1 insertion(+), 18 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > > index d600fd5..70609ef2 100644
> > > > > --- a/drivers/pwm/pwm-imx.c
> > > > > +++ b/drivers/pwm/pwm-imx.c
> > > > > @@ -49,7 +49,6 @@
> > > > >  
> > > > >  struct imx_chip {
> > > > >  	struct clk	*clk_per;
> > > > > -	struct clk	*clk_ipg;
> > > > >  
> > > > >  	void __iomem	*mmio_base;
> > > > >  
> > > > > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
> > > > >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> > > > >  {
> > > > >  	struct imx_chip *imx = to_imx_chip(chip);
> > > > > -	int ret;
> > > > > -
> > > > > -	ret = clk_prepare_enable(imx->clk_ipg);
> > > > > -	if (ret)
> > > > > -		return ret;
> > > > >  
> > > > > -	ret = imx->config(chip, pwm, duty_ns, period_ns);
> > > > > -
> > > > > -	clk_disable_unprepare(imx->clk_ipg);
> > > > > -
> > > > > -	return ret;
> > > > > +	return imx->config(chip, pwm, duty_ns, period_ns);
> > > > >  }
> > > > >  
> > > > >  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > > > @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
> > > > >  		return PTR_ERR(imx->clk_per);
> > > > >  	}
> > > > >  
> > > > > -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > > > > -	if (IS_ERR(imx->clk_ipg)) {
> > > > > -		dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> > > > > -				PTR_ERR(imx->clk_ipg));
> > > > > -		return PTR_ERR(imx->clk_ipg);
> > > > > -	}
> > > > > -
> > > > >  	imx->chip.ops = &imx_pwm_ops;
> > > > >  	imx->chip.dev = &pdev->dev;
> > > > >  	imx->chip.base = -1;
> > > > >
> > > > If the IPG clock is not needed by the driver it should be removed from
> > > > DT as well.
> > > 
> > > No, it's only the half truth that it's not needed. It would indeed be
> > > needed if the driver used the ipg clock as source for the PWM (PWMCR[17:16] = 0b01).
> > >
> > That's a different story!
> > Currently the DT specifies two clocks for the PWM:
> > 1. register access clock (which we now know is unnecessary)
> > 2. PWM source clock
> > In the case mentioned above, the IPG clock has to be specified as the
> > SECOND clock entry in DT, because otherwise the clock won't be
> > enabled/disabled as required!
> 
> Since the driver gets its clock by name (clk_get(&pdev->dev, "per"/"ipg"))
> the position in the DT doesn't matter at all.
> 
Do you really think so?
The driver does a lookup for a clock named 'ipg' which it doesn't use
at all with your proposed patcht and a lookup for the 'per' clock which
it enables/disables whenever the PWM output is switched inactive/active.
Since the clock named 'per' is the second clock in DTB it is moot to
have the ipg clock in the first position when intending to use it as
PWM source clock!

> The only thing that isn't accurate is that the "ipg" clock in the device
> tree is not for register access, but itself a clock to be used as PWM
> source. This is no functional problem though.
> 
That only happens to work accidentally, because the IPG clock will never
be switched off anyway. But it is semantically incorrect and should not
be promoted for others to copy...


Lothar Waßmann

Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Philipp Zabel]

Hi Lothar,

Am Mittwoch, den 02.11.2016, 09:51 +0100 schrieb Lothar Waßmann:
> Hi,
> 
> On Wed, 2 Nov 2016 09:06:45 +0100 Sascha Hauer wrote:
> > On Wed, Nov 02, 2016 at 08:56:20AM +0100, Lothar Waßmann wrote:
> > > Hi,
> > > 
> > > On Wed, 2 Nov 2016 08:36:14 +0100 Sascha Hauer wrote:
> > > > On Wed, Nov 02, 2016 at 08:18:52AM +0100, Lothar Waßmann wrote:
> > > > > Hi,
> > > > > 
> > > > > On Mon, 31 Oct 2016 06:59:04 +0100 Sascha Hauer wrote:
> > > > > > As said, even the commit 7b27c160c68 introducing the register clk did not
> > > > > > enable the clock consistently for all register accesses. Maybe it's best
> > > > > > to include the following patch so that we can find a clear culprit and
> > > > > > do not bury the ipg clock changes in larger patches.
> > > > > > 
> > > > > > Sascha
> > > > > > 
> > > > > > -----------------------------8<-----------------------------------
> > > > > > 
> > > > > > From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
> > > > > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > > Date: Mon, 31 Oct 2016 06:45:33 +0100
> > > > > > Subject: [PATCH] pwm: imx: remove ipg clock
> > > > > > 
> > > > > > The use of the ipg clock was introduced with commit 7b27c160c6. In the
> > > > > > commit message it was claimed that the ipg clock is enabled for register
> > > > > > accesses. This is true for the ->config() callback, but not for the
> > > > > > ->set_enable() callback. Given that the ipg clock is not consistently
> > > > > > enabled for all register accesses we can assume that either it is not
> > > > > > required at all or that the current code does not work.
> > > > > > Remove the ipg clock code for now so that it's no longer in the way of
> > > > > > refactoring the driver.
> > > > > > 
> > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > > > > ---
> > > > > >  drivers/pwm/pwm-imx.c | 19 +------------------
> > > > > >  1 file changed, 1 insertion(+), 18 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > > > index d600fd5..70609ef2 100644
> > > > > > --- a/drivers/pwm/pwm-imx.c
> > > > > > +++ b/drivers/pwm/pwm-imx.c
> > > > > > @@ -49,7 +49,6 @@
> > > > > >  
> > > > > >  struct imx_chip {
> > > > > >  	struct clk	*clk_per;
> > > > > > -	struct clk	*clk_ipg;
> > > > > >  
> > > > > >  	void __iomem	*mmio_base;
> > > > > >  
> > > > > > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
> > > > > >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> > > > > >  {
> > > > > >  	struct imx_chip *imx = to_imx_chip(chip);
> > > > > > -	int ret;
> > > > > > -
> > > > > > -	ret = clk_prepare_enable(imx->clk_ipg);
> > > > > > -	if (ret)
> > > > > > -		return ret;
> > > > > >  
> > > > > > -	ret = imx->config(chip, pwm, duty_ns, period_ns);
> > > > > > -
> > > > > > -	clk_disable_unprepare(imx->clk_ipg);
> > > > > > -
> > > > > > -	return ret;
> > > > > > +	return imx->config(chip, pwm, duty_ns, period_ns);
> > > > > >  }
> > > > > >  
> > > > > >  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > > > > @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
> > > > > >  		return PTR_ERR(imx->clk_per);
> > > > > >  	}
> > > > > >  
> > > > > > -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > > > > > -	if (IS_ERR(imx->clk_ipg)) {
> > > > > > -		dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> > > > > > -				PTR_ERR(imx->clk_ipg));
> > > > > > -		return PTR_ERR(imx->clk_ipg);
> > > > > > -	}
> > > > > > -
> > > > > >  	imx->chip.ops = &imx_pwm_ops;
> > > > > >  	imx->chip.dev = &pdev->dev;
> > > > > >  	imx->chip.base = -1;
> > > > > >
> > > > > If the IPG clock is not needed by the driver it should be removed from
> > > > > DT as well.
> > > > 
> > > > No, it's only the half truth that it's not needed. It would indeed be
> > > > needed if the driver used the ipg clock as source for the PWM (PWMCR[17:16] = 0b01).
> > > >
> > > That's a different story!
> > > Currently the DT specifies two clocks for the PWM:
> > > 1. register access clock (which we now know is unnecessary)
> > > 2. PWM source clock
> > > In the case mentioned above, the IPG clock has to be specified as the
> > > SECOND clock entry in DT, because otherwise the clock won't be
> > > enabled/disabled as required!
> > 
> > Since the driver gets its clock by name (clk_get(&pdev->dev, "per"/"ipg"))
> > the position in the DT doesn't matter at all.
> > 
> Do you really think so?

Could you elaborate why the position of the clock phandles in the clocks
property is an issue at all?

> The driver does a lookup for a clock named 'ipg' which it doesn't use
> at all with your proposed patcht

With the proposed patch the lookup is removed, too.

It could be added back to the driver if somebody ever has the need to
clock pwm output from the ipg_clk instead of the ipg_clk_highfreq input
to the pwm module.

>  and a lookup for the 'per' clock which
> it enables/disables whenever the PWM output is switched inactive/active.
> Since the clock named 'per' is the second clock in DTB it is moot to
> have the ipg clock in the first position when intending to use it as
> PWM source clock!

Since the clock lookup is by name, the order of the clocks could indeed
be changed, but what is gained from that?

> > The only thing that isn't accurate is that the "ipg" clock in the device
> > tree is not for register access, but itself a clock to be used as PWM
> > source. This is no functional problem though.
> > 
> That only happens to work accidentally, because the IPG clock will never
> be switched off anyway.

The "ipg" clock name here does not describe the global ipg root clock,
and as we just realized doesn't supply the registers. It just happens to
be the name of the "ipg_clk" input to the pwm module, which is used to
generate the pwm output if the CLKSRC field in PWMx_PWMCR[17:16] is set
to 0x1 (ipg_clk).

>  But it is semantically incorrect and should not
> be promoted for others to copy...

I don't agree. If at all, we are missing documented inputs (the 32k
clock).

regards
Philipp

Re: [PATCH v2 04/10] pwm: imx: Move PWMv2 software reset code to a separate function

[Philipp Zabel]

Hi Lukasz,

Am Donnerstag, den 27.10.2016, 08:29 +0200 schrieb Lukasz Majewski:
> The software reset code has been extracted from imx_pwm_config_v2 function
> and moved to new one - imx_pwm_sw_reset().
> 
> This change reduces the overall size of imx_pwm_config_v2() and prepares
> it for atomic PWM operation.
> 
> Suggested-by: Stefan Agner <stefan@agner.ch>
> Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>

Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

> ---
> Changes for v2:
> - Add missing parenthesis
> ---
>  drivers/pwm/pwm-imx.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 822eb5a..8174ae6 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -134,6 +134,25 @@ static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
>  	clk_disable_unprepare(imx->clk_per);
>  }
>  
> +static void imx_pwm_sw_reset(struct pwm_chip *chip)
> +{
> +	struct imx_chip *imx = to_imx_chip(chip);
> +	struct device *dev = chip->dev;
> +	int wait_count = 0;
> +	u32 cr;
> +
> +	writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
> +	do {
> +		usleep_range(200, 1000);
> +		cr = readl(imx->mmio_base + MX3_PWMCR);
> +	} while ((cr & MX3_PWMCR_SWR) &&
> +		 (wait_count++ < MX3_PWM_SWR_LOOP));

Not an issue for this patch, but this could be converted to use
readl_poll_timeout.

> +
> +	if (cr & MX3_PWMCR_SWR)
> +		dev_warn(dev, "software reset timeout\n");
> +}
> +
> +
>  static int imx_pwm_config_v2(struct pwm_chip *chip,
>  		struct pwm_device *pwm, int duty_ns, int period_ns)
>  {
> @@ -143,7 +162,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>  	unsigned long period_cycles, duty_cycles, prescale;
>  	unsigned int period_ms;
>  	bool enable = pwm_is_enabled(pwm);
> -	int wait_count = 0, fifoav;
> +	int fifoav;
>  	u32 cr, sr;
>  
>  	/*
> @@ -166,15 +185,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>  				dev_warn(dev, "there is no free FIFO slot\n");
>  		}
>  	} else {
> -		writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
> -		do {
> -			usleep_range(200, 1000);
> -			cr = readl(imx->mmio_base + MX3_PWMCR);
> -		} while ((cr & MX3_PWMCR_SWR) &&
> -			 (wait_count++ < MX3_PWM_SWR_LOOP));
> -
> -		if (cr & MX3_PWMCR_SWR)
> -			dev_warn(dev, "software reset timeout\n");
> +		imx_pwm_sw_reset(chip);
>  	}
>  
>  	c = clk_get_rate(imx->clk_per);

regards
Philipp