[PATCH v4 00/11] pwm: imx: Provide atomic operation for IMX PWM driver

[PATCH v4 00/11] 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:

- Remove ipg clock and enable per clock when required (as proposed by Boris 
  Brezillon) - this is the most notable change for v4

- Provide different pwm ops for PWMv1 and PWMv2

- 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. This patch also includes some code
  to address potential issues on i.MX7 (lack of peripheral clock when accessing
  PWM registers).

- 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
v4.9 vanila kernel SHA1: 69973b830859bc6529a7a0468ba0d80ee5117826
It applies to v4.10-rc2

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

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


Boris Brezillon (1):
  pwm: imx: remove ipg clock and enable per clock when required

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                             | 272 ++++++++++------------
 3 files changed, 146 insertions(+), 158 deletions(-)

-- 
2.1.4

[PATCH v4 01/11] 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>
---
Changes for v4:
- None

Changes for v3:
- None
---
 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 v4 02/11] pwm: imx: remove ipg clock and enable per clock when required

[Lukasz Majewski]

From: Boris Brezillon <boris.brezillon@free-electrons.com>

The use of the ipg clock was introduced with commit 7b27c160c681
("pwm: i.MX: fix clock lookup").
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.

In the other hand, the imx7 IP requires the peripheral clock to be
enabled before accessing its registers. Since ->config() can be called
when the PWM is disabled (in which case, the peripheral clock is also
disabled), we need to surround the imx->config() with
clk_prepare_enable(per_clk)/clk_disable_unprepare(per_clk) calls.

Note that the imx7 IP was working fine so far because the ipg clock was
actually pointing to the peripheral clock, which guaranteed peripheral
clock activation even when ->config() was called when the PWM was
disabled.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes in v4:
- Enable per clk before calling imx->config()

Changes in v3:
- New patch
---
 drivers/pwm/pwm-imx.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index d600fd5..b1d1e50 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;
 
@@ -206,13 +205,13 @@ static int imx_pwm_config(struct pwm_chip *chip,
 	struct imx_chip *imx = to_imx_chip(chip);
 	int ret;
 
-	ret = clk_prepare_enable(imx->clk_ipg);
+	ret = clk_prepare_enable(imx->clk_per);
 	if (ret)
 		return ret;
 
 	ret = imx->config(chip, pwm, duty_ns, period_ns);
 
-	clk_disable_unprepare(imx->clk_ipg);
+	clk_disable_unprepare(imx->clk_per);
 
 	return ret;
 }
@@ -293,13 +292,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.1.4

[PATCH v4 03/11] pwm: imx: Add separate set of pwm ops for PWMv1 and PWMv2

[Lukasz Majewski]

From: Lukasz Majewski <l.majewski@majess.pl>

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 v4:
- None

Changes for v3:
- Adjust the code to work with ipg clock removed

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 b1d1e50..0fa480d 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -239,7 +239,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,
@@ -250,16 +257,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[] = {
@@ -281,6 +291,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;
@@ -292,7 +304,7 @@ static int imx_pwm_probe(struct platform_device *pdev)
 		return PTR_ERR(imx->clk_per);
 	}
 
-	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;
@@ -303,7 +315,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 v4 04/11] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Lukasz Majewski]

From: Lukasz Majewski <l.majewski@majess.pl>

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>
Reviewed-by: Stefan Agner <stefan@agner.ch>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
Changes for v4:
- None

[Minor code adjustment according to Stefan Agner comment]

Changes for v3:
- Remove ipg clock

Changes for v2:
- Add missing clock unprepare for clk_ipg
- Enable peripheral PWM clock (clk_per)
---
 drivers/pwm/pwm-imx.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 0fa480d..11e3f3e 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -90,19 +90,34 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
 	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_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;
+	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,
@@ -240,9 +255,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,
 };
 
@@ -261,8 +276,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 v4 05/11] pwm: imx: Move PWMv2 software reset code to a separate function

[Lukasz Majewski]

From: Lukasz Majewski <l.majewski@majess.pl>

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 v4:
- None

Changes for v3:
- None

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 11e3f3e..f0d78f3 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -120,6 +120,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)
 {
@@ -129,7 +148,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;
 
 	/*
@@ -152,15 +171,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 v4 06/11] pwm: imx: Move PWMv2 wait for fifo slot code to a separate function

[Lukasz Majewski]

From: Lukasz Majewski <l.majewski@majess.pl>

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 v4:
- None

Changes for v3:
- None

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 f0d78f3..60cdc5c 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -138,18 +138,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.
@@ -158,21 +176,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 v4 07/11] 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 v4:
- Avoid recalculation of PWM parameters when disabling PWM signal
- Unconditionally call clk_prepare_enable(imx->clk_per) and
  clk_disable_unprepare(imx->clk_per)

Changes for v3:
- Remove ipg clock enable/disable functions

Changes for v2:
- None
---
 drivers/pwm/pwm-imx.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 60cdc5c..134dd66 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -249,6 +249,72 @@ static int imx_pwm_config(struct pwm_chip *chip,
 	return ret;
 }
 
+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;
+	int ret;
+
+	pwm_get_state(pwm, &cstate);
+
+	if (state->enabled) {
+		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;
+
+		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);
+
+		writel(MX3_PWMCR_PRESCALER(prescale) |
+		       MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
+		       MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
+		       MX3_PWMCR_EN,
+		       imx->mmio_base + MX3_PWMCR);
+
+	} else {
+		writel(0, imx->mmio_base + MX3_PWMCR);
+
+		clk_disable_unprepare(imx->clk_per);
+	}
+
+	return 0;
+}
+
 static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct imx_chip *imx = to_imx_chip(chip);
@@ -280,6 +346,7 @@ static struct pwm_ops imx_pwm_ops_v1 = {
 };
 
 static struct pwm_ops imx_pwm_ops_v2 = {
+	.apply = imx_pwm_apply_v2,
 	.enable = imx_pwm_enable,
 	.disable = imx_pwm_disable,
 	.config = imx_pwm_config,
-- 
2.1.4

[PATCH v4 08/11] 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 v4:
- None

Changes for v3:
- None

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 134dd66..afca5f5 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -53,10 +53,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)
@@ -160,95 +156,6 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
 	}
 }
 
-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_per);
-	if (ret)
-		return ret;
-
-	ret = imx->config(chip, pwm, duty_ns, period_ns);
-
-	clk_disable_unprepare(imx->clk_per);
-
-	return ret;
-}
-
 static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 			    struct pwm_state *state)
 {
@@ -315,29 +222,6 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
-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,
@@ -347,16 +231,10 @@ static struct pwm_ops imx_pwm_ops_v1 = {
 
 static struct pwm_ops imx_pwm_ops_v2 = {
 	.apply = imx_pwm_apply_v2,
-	.enable = imx_pwm_enable,
-	.disable = imx_pwm_disable,
-	.config = imx_pwm_config,
 	.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;
 };
 
@@ -365,8 +243,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,
 };
 
@@ -413,9 +289,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 v4 09/11] 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 v4:
- None

Changes for v3:
- None

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 v4 10/11] pwm: imx: doc: Update imx-pwm.txt documentation entry

[Lukasz Majewski]

From: Lukasz Majewski <l.majewski@majess.pl>

The imx-pwm.txt documentation update as a preparation for polarity
support.

Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes for v4:
- None

Changes for v3:
- None

Changes for v2:
- New patch
---
 Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
index e00c2e9..c61bdf8 100644
--- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
@@ -6,8 +6,8 @@ Required properties:
   - "fsl,imx1-pwm" for PWM compatible with the one integrated on i.MX1
   - "fsl,imx27-pwm" for PWM compatible with the one integrated on i.MX27
 - reg: physical base address and length of the controller's registers
-- #pwm-cells: should be 2. See pwm.txt in this directory for a description of
-  the cells format.
+- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See pwm.txt
+  in this directory for a description of the cells format.
 - clocks : Clock specifiers for both ipg and per clocks.
 - clock-names : Clock names should include both "ipg" and "per"
 See the clock consumer binding,
@@ -17,7 +17,7 @@ See the clock consumer binding,
 Example:
 
 pwm1: pwm@53fb4000 {
-	#pwm-cells = <2>;
+	#pwm-cells = <3>;
 	compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
 	reg = <0x53fb4000 0x4000>;
 	clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
-- 
2.1.4

[PATCH v4 11/11] 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 v4:
- MX3_PWMCR_POUTC setting adjustements after apply_v2() previous changes

Changes for v3:
- None

Changes for v2:
- New patch
---
 drivers/pwm/pwm-imx.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index afca5f5..d35d83f 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)
@@ -164,6 +165,7 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct pwm_state cstate;
 	unsigned long long c;
 	int ret;
+	u32 cr;
 
 	pwm_get_state(pwm, &cstate);
 
@@ -207,12 +209,15 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
 		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
 
-		writel(MX3_PWMCR_PRESCALER(prescale) |
-		       MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
-		       MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
-		       MX3_PWMCR_EN,
-		       imx->mmio_base + MX3_PWMCR);
+		cr = MX3_PWMCR_PRESCALER(prescale) |
+			MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
+			MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
+			MX3_PWMCR_EN;
+
+		if (state->polarity == PWM_POLARITY_INVERSED)
+			cr |= MX3_PWMCR_POUTC;
 
+		writel(cr, imx->mmio_base + MX3_PWMCR);
 	} else {
 		writel(0, imx->mmio_base + MX3_PWMCR);
 
@@ -235,6 +240,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
 };
 
 struct imx_pwm_data {
+	bool polarity_supported;
 	struct pwm_ops *pwm_ops;
 };
 
@@ -243,6 +249,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,
 };
 
@@ -283,6 +290,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 v4 02/11] pwm: imx: remove ipg clock and enable per clock when required

[Boris Brezillon]

On Thu,  5 Jan 2017 00:36:45 +0100
Lukasz Majewski <lukma@denx.de> wrote:

> From: Boris Brezillon <boris.brezillon@free-electrons.com>

Can you keep Sascha as the author of this patch?

> 
> The use of the ipg clock was introduced with commit 7b27c160c681
> ("pwm: i.MX: fix clock lookup").
> 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.
> 
> In the other hand, the imx7 IP requires the peripheral clock to be
> enabled before accessing its registers. Since ->config() can be called
> when the PWM is disabled (in which case, the peripheral clock is also
> disabled), we need to surround the imx->config() with
> clk_prepare_enable(per_clk)/clk_disable_unprepare(per_clk) calls.
> 
> Note that the imx7 IP was working fine so far because the ipg clock was
> actually pointing to the peripheral clock, which guaranteed peripheral
> clock activation even when ->config() was called when the PWM was
> disabled.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes in v4:
> - Enable per clk before calling imx->config()
> 
> Changes in v3:
> - New patch
> ---
>  drivers/pwm/pwm-imx.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d600fd5..b1d1e50 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;
>  
> @@ -206,13 +205,13 @@ static int imx_pwm_config(struct pwm_chip *chip,
>  	struct imx_chip *imx = to_imx_chip(chip);
>  	int ret;
>  
> -	ret = clk_prepare_enable(imx->clk_ipg);
> +	ret = clk_prepare_enable(imx->clk_per);
>  	if (ret)
>  		return ret;
>  
>  	ret = imx->config(chip, pwm, duty_ns, period_ns);
>  
> -	clk_disable_unprepare(imx->clk_ipg);
> +	clk_disable_unprepare(imx->clk_per);
>  
>  	return ret;
>  }
> @@ -293,13 +292,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;

Re: [PATCH v4 02/11] pwm: imx: remove ipg clock and enable per clock when required

[Lukasz Majewski]

Hi Boris,

> On Thu,  5 Jan 2017 00:36:45 +0100
> Lukasz Majewski <lukma@denx.de> wrote:
> 
> > From: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
> Can you keep Sascha as the author of this patch?

But the patch differs from the original one - and that change was sent
by you - so I assume that you made the modifications.

Apparently there are two authors and we only have one "Author"
field :-) so I took the patch as it was sent by you.

Is the rest of the series correct?

Best regards,
Łukasz Majewski

> 
> > 
> > The use of the ipg clock was introduced with commit 7b27c160c681
> > ("pwm: i.MX: fix clock lookup").
> > 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.
> > 
> > In the other hand, the imx7 IP requires the peripheral clock to be
> > enabled before accessing its registers. Since ->config() can be
> > called when the PWM is disabled (in which case, the peripheral
> > clock is also disabled), we need to surround the imx->config() with
> > clk_prepare_enable(per_clk)/clk_disable_unprepare(per_clk) calls.
> > 
> > Note that the imx7 IP was working fine so far because the ipg clock
> > was actually pointing to the peripheral clock, which guaranteed
> > peripheral clock activation even when ->config() was called when
> > the PWM was disabled.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > Changes in v4:
> > - Enable per clk before calling imx->config()
> > 
> > Changes in v3:
> > - New patch
> > ---
> >  drivers/pwm/pwm-imx.c | 12 ++----------
> >  1 file changed, 2 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index d600fd5..b1d1e50 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;
> >  
> > @@ -206,13 +205,13 @@ static int imx_pwm_config(struct pwm_chip
> > *chip, struct imx_chip *imx = to_imx_chip(chip);
> >  	int ret;
> >  
> > -	ret = clk_prepare_enable(imx->clk_ipg);
> > +	ret = clk_prepare_enable(imx->clk_per);
> >  	if (ret)
> >  		return ret;
> >  
> >  	ret = imx->config(chip, pwm, duty_ns, period_ns);
> >  
> > -	clk_disable_unprepare(imx->clk_ipg);
> > +	clk_disable_unprepare(imx->clk_per);
> >  
> >  	return ret;
> >  }
> > @@ -293,13 +292,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;
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

Re: [PATCH v4 02/11] pwm: imx: remove ipg clock and enable per clock when required

[Boris Brezillon]

On Thu, 5 Jan 2017 08:26:09 +0100
Lukasz Majewski <lukma@denx.de> wrote:

> Hi Boris,
> 
> > On Thu,  5 Jan 2017 00:36:45 +0100
> > Lukasz Majewski <lukma@denx.de> wrote:
> >   
> > > From: Boris Brezillon <boris.brezillon@free-electrons.com>  
> > 
> > Can you keep Sascha as the author of this patch?  
> 
> But the patch differs from the original one - and that change was sent
> by you - so I assume that you made the modifications.
> 
> Apparently there are two authors and we only have one "Author"
> field :-) so I took the patch as it was sent by you.

Well, when the changes are substantial it can be justified, but here
the changes are trivial (2 lines changed), and I don't think it calls
for an authorship change (unless Sascha doesn't want to take the
responsibility for those 2 lines).

> 
> Is the rest of the series correct?

I'll have a look.

> 
> Best regards,
> Łukasz Majewski
> 
> >   
> > > 
> > > The use of the ipg clock was introduced with commit 7b27c160c681
> > > ("pwm: i.MX: fix clock lookup").
> > > 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.
> > > 
> > > In the other hand, the imx7 IP requires the peripheral clock to be
> > > enabled before accessing its registers. Since ->config() can be
> > > called when the PWM is disabled (in which case, the peripheral
> > > clock is also disabled), we need to surround the imx->config() with
> > > clk_prepare_enable(per_clk)/clk_disable_unprepare(per_clk) calls.
> > > 
> > > Note that the imx7 IP was working fine so far because the ipg clock
> > > was actually pointing to the peripheral clock, which guaranteed
> > > peripheral clock activation even when ->config() was called when
> > > the PWM was disabled.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > > Changes in v4:
> > > - Enable per clk before calling imx->config()
> > > 
> > > Changes in v3:
> > > - New patch
> > > ---
> > >  drivers/pwm/pwm-imx.c | 12 ++----------
> > >  1 file changed, 2 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > index d600fd5..b1d1e50 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;
> > >  
> > > @@ -206,13 +205,13 @@ static int imx_pwm_config(struct pwm_chip
> > > *chip, struct imx_chip *imx = to_imx_chip(chip);
> > >  	int ret;
> > >  
> > > -	ret = clk_prepare_enable(imx->clk_ipg);
> > > +	ret = clk_prepare_enable(imx->clk_per);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > >  	ret = imx->config(chip, pwm, duty_ns, period_ns);
> > >  
> > > -	clk_disable_unprepare(imx->clk_ipg);
> > > +	clk_disable_unprepare(imx->clk_per);
> > >  
> > >  	return ret;
> > >  }
> > > @@ -293,13 +292,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;  
> >   
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

Re: [PATCH v4 02/11] pwm: imx: remove ipg clock and enable per clock when required

[Lukasz Majewski]

Hi Boris,

> On Thu, 5 Jan 2017 08:26:09 +0100
> Lukasz Majewski <lukma@denx.de> wrote:
> 
> > Hi Boris,
> > 
> > > On Thu,  5 Jan 2017 00:36:45 +0100
> > > Lukasz Majewski <lukma@denx.de> wrote:
> > >   
> > > > From: Boris Brezillon <boris.brezillon@free-electrons.com>  
> > > 
> > > Can you keep Sascha as the author of this patch?  
> > 
> > But the patch differs from the original one - and that change was
> > sent by you - so I assume that you made the modifications.
> > 
> > Apparently there are two authors and we only have one "Author"
> > field :-) so I took the patch as it was sent by you.
> 
> Well, when the changes are substantial it can be justified, but here
> the changes are trivial (2 lines changed), and I don't think it calls
> for an authorship change (unless Sascha doesn't want to take the
> responsibility for those 2 lines).
> 

I will correct this if v5 is required, otherwise I can re-send only
this single patch.

> > 
> > Is the rest of the series correct?
> 
> I'll have a look.

Ok.

> 
> > 
> > Best regards,
> > Łukasz Majewski
> > 
> > >   
> > > > 
> > > > The use of the ipg clock was introduced with commit 7b27c160c681
> > > > ("pwm: i.MX: fix clock lookup").
> > > > 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.
> > > > 
> > > > In the other hand, the imx7 IP requires the peripheral clock to
> > > > be enabled before accessing its registers. Since ->config() can
> > > > be called when the PWM is disabled (in which case, the
> > > > peripheral clock is also disabled), we need to surround the
> > > > imx->config() with
> > > > clk_prepare_enable(per_clk)/clk_disable_unprepare(per_clk)
> > > > calls.
> > > > 
> > > > Note that the imx7 IP was working fine so far because the ipg
> > > > clock was actually pointing to the peripheral clock, which
> > > > guaranteed peripheral clock activation even when ->config() was
> > > > called when the PWM was disabled.
> > > > 
> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Signed-off-by: Boris Brezillon
> > > > <boris.brezillon@free-electrons.com> Cc: Philipp Zabel
> > > > <p.zabel@pengutronix.de> ---
> > > > Changes in v4:
> > > > - Enable per clk before calling imx->config()
> > > > 
> > > > Changes in v3:
> > > > - New patch
> > > > ---
> > > >  drivers/pwm/pwm-imx.c | 12 ++----------
> > > >  1 file changed, 2 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > index d600fd5..b1d1e50 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;
> > > >  
> > > > @@ -206,13 +205,13 @@ static int imx_pwm_config(struct pwm_chip
> > > > *chip, struct imx_chip *imx = to_imx_chip(chip);
> > > >  	int ret;
> > > >  
> > > > -	ret = clk_prepare_enable(imx->clk_ipg);
> > > > +	ret = clk_prepare_enable(imx->clk_per);
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > >  	ret = imx->config(chip, pwm, duty_ns, period_ns);
> > > >  
> > > > -	clk_disable_unprepare(imx->clk_ipg);
> > > > +	clk_disable_unprepare(imx->clk_per);
> > > >  
> > > >  	return ret;
> > > >  }
> > > > @@ -293,13 +292,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;  
> > >   
> > 
> > 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email:
> > wd@denx.de
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

Re: [PATCH v4 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Boris Brezillon]

On Thu,  5 Jan 2017 00:36:50 +0100
Lukasz Majewski <lukma@denx.de> wrote:

> 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 v4:
> - Avoid recalculation of PWM parameters when disabling PWM signal
> - Unconditionally call clk_prepare_enable(imx->clk_per) and
>   clk_disable_unprepare(imx->clk_per)

I don't see that one, but I'm not sure this is actually needed.
In the enabled path we enable the clk before accessing the registers,
and in the disable path, assuming you change the code according to my
suggestion, the clk should already be enabled when you write to
MX3_PWMCR.

> 
> Changes for v3:
> - Remove ipg clock enable/disable functions
> 
> Changes for v2:
> - None
> ---
>  drivers/pwm/pwm-imx.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 60cdc5c..134dd66 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -249,6 +249,72 @@ static int imx_pwm_config(struct pwm_chip *chip,
>  	return ret;
>  }
>  
> +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;
> +	int ret;
> +
> +	pwm_get_state(pwm, &cstate);
> +
> +	if (state->enabled) {
> +		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;
> +

Starting here...

> +		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);

... till here, should be replaced by:


		/*
		 * 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 {
			ret = clk_prepare_enable(imx->clk_per);
			if (ret)
				return ret;

			imx_pwm_sw_reset(chip);
		}

Otherwise you keep incrementing the prepare/enable counters of the clk
when you change the config of an already enabled PWM.

> +
> +		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> +		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> +
> +		writel(MX3_PWMCR_PRESCALER(prescale) |
> +		       MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> +		       MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
> +		       MX3_PWMCR_EN,
> +		       imx->mmio_base + MX3_PWMCR);
> +
> +	} else {

	} else if (cstate.enabled) {

Again, this is to avoid unbalanced prepare/enable counters on the per
clk if the PWM config is changed several times when the PWM is disabled.

> +		writel(0, imx->mmio_base + MX3_PWMCR);
> +
> +		clk_disable_unprepare(imx->clk_per);
> +	}
> +
> +	return 0;
> +}
> +
>  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct imx_chip *imx = to_imx_chip(chip);
> @@ -280,6 +346,7 @@ static struct pwm_ops imx_pwm_ops_v1 = {
>  };
>  
>  static struct pwm_ops imx_pwm_ops_v2 = {
> +	.apply = imx_pwm_apply_v2,
>  	.enable = imx_pwm_enable,
>  	.disable = imx_pwm_disable,
>  	.config = imx_pwm_config,

Re: [PATCH v4 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Lukasz Majewski]

On Thu, 5 Jan 2017 09:50:35 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Thu,  5 Jan 2017 00:36:50 +0100
> Lukasz Majewski <lukma@denx.de> wrote:
> 
> > 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 v4:
> > - Avoid recalculation of PWM parameters when disabling PWM signal
> > - Unconditionally call clk_prepare_enable(imx->clk_per) and
> >   clk_disable_unprepare(imx->clk_per)
> 
> I don't see that one, but I'm not sure this is actually needed.
> In the enabled path we enable the clk before accessing the registers,
> and in the disable path, assuming you change the code according to my
> suggestion, the clk should already be enabled when you write to
> MX3_PWMCR.
> 
> > 
> > Changes for v3:
> > - Remove ipg clock enable/disable functions
> > 
> > Changes for v2:
> > - None
> > ---
> >  drivers/pwm/pwm-imx.c | 67
> > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> > 67 insertions(+)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index 60cdc5c..134dd66 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -249,6 +249,72 @@ static int imx_pwm_config(struct pwm_chip
> > *chip, return ret;
> >  }
> >  
> > +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;
> > +	int ret;
> > +
> > +	pwm_get_state(pwm, &cstate);
> > +
> > +	if (state->enabled) {
> > +		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;
> > +
> 
> Starting here...
> 
> > +		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);
> 
> ... till here, should be replaced by:
> 
> 
> 		/*
> 		 * 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 {
> 			ret = clk_prepare_enable(imx->clk_per);
> 			if (ret)
> 				return ret;

In the other mail you mentioned that the clock should be enabled
unconditionally to fix issue on iMX7 when we want to access registers.

Now it depends on cstate.enabled flag. 

So we end up with 

if (state.enabled && !cstate.enabled)
	clk_preapre_enable();

which was the reason of iMX7 failure reported by Stefan to v3.


> 
> 			imx_pwm_sw_reset(chip);
> 		}
> 
> Otherwise you keep incrementing the prepare/enable counters of the clk
> when you change the config of an already enabled PWM.
> 
> > +
> > +		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > +		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > +
> > +		writel(MX3_PWMCR_PRESCALER(prescale) |
> > +		       MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> > +		       MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH
> > |
> > +		       MX3_PWMCR_EN,
> > +		       imx->mmio_base + MX3_PWMCR);
> > +
> > +	} else {
> 
> 	} else if (cstate.enabled) {
> 
> Again, this is to avoid unbalanced prepare/enable counters on the per
> clk if the PWM config is changed several times when the PWM is
> disabled.
> 
> > +		writel(0, imx->mmio_base + MX3_PWMCR);
> > +
> > +		clk_disable_unprepare(imx->clk_per);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device
> > *pwm) {
> >  	struct imx_chip *imx = to_imx_chip(chip);
> > @@ -280,6 +346,7 @@ static struct pwm_ops imx_pwm_ops_v1 = {
> >  };
> >  
> >  static struct pwm_ops imx_pwm_ops_v2 = {
> > +	.apply = imx_pwm_apply_v2,
> >  	.enable = imx_pwm_enable,
> >  	.disable = imx_pwm_disable,
> >  	.config = imx_pwm_config,
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

Re: [PATCH v4 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Boris Brezillon]

On Thu, 5 Jan 2017 10:03:47 +0100
Lukasz Majewski <lukma@denx.de> wrote:

> On Thu, 5 Jan 2017 09:50:35 +0100
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > On Thu,  5 Jan 2017 00:36:50 +0100
> > Lukasz Majewski <lukma@denx.de> wrote:
> >   
> > > 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 v4:
> > > - Avoid recalculation of PWM parameters when disabling PWM signal
> > > - Unconditionally call clk_prepare_enable(imx->clk_per) and
> > >   clk_disable_unprepare(imx->clk_per)  
> > 
> > I don't see that one, but I'm not sure this is actually needed.
> > In the enabled path we enable the clk before accessing the registers,
> > and in the disable path, assuming you change the code according to my
> > suggestion, the clk should already be enabled when you write to
> > MX3_PWMCR.
> >   
> > > 
> > > Changes for v3:
> > > - Remove ipg clock enable/disable functions
> > > 
> > > Changes for v2:
> > > - None
> > > ---
> > >  drivers/pwm/pwm-imx.c | 67
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> > > 67 insertions(+)
> > > 
> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > index 60cdc5c..134dd66 100644
> > > --- a/drivers/pwm/pwm-imx.c
> > > +++ b/drivers/pwm/pwm-imx.c
> > > @@ -249,6 +249,72 @@ static int imx_pwm_config(struct pwm_chip
> > > *chip, return ret;
> > >  }
> > >  
> > > +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;
> > > +	int ret;
> > > +
> > > +	pwm_get_state(pwm, &cstate);
> > > +
> > > +	if (state->enabled) {
> > > +		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;
> > > +  
> > 
> > Starting here...
> >   
> > > +		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);  
> > 
> > ... till here, should be replaced by:
> > 
> > 
> > 		/*
> > 		 * 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 {
> > 			ret = clk_prepare_enable(imx->clk_per);
> > 			if (ret)
> > 				return ret;  
> 
> In the other mail you mentioned that the clock should be enabled
> unconditionally to fix issue on iMX7 when we want to access registers.

When I said unconditionally, I meant call
clk_prepare_enable(imx->clk_per) just after pwm_get_state() (at the
beginning of the function) and call clk_disable_(imx->clk_per) just
before returning 0 at the end of the function. This way you're
guaranteed that the clk is always enabled when you access registers in
between. Of course, you still need the the clk_prepare_enable() and
clk_disable_unprepare() in the enable and disable path to keep the clk
enabled when the PWM is enabled, and to disable it when the clk is
being disabled.

But, while reviewing your patch I realized this was actually unneeded
(see the explanation in my previous review).

> 
> Now it depends on cstate.enabled flag. 
> 
> So we end up with 
> 
> if (state.enabled && !cstate.enabled)
> 	clk_preapre_enable();

Yep, and that's correct.

> 
> which was the reason of iMX7 failure reported by Stefan to v3.

Stefan, do you confirm that? I don't see how this can possibly fail
since the clk is either already enabled (cstate.enabled) or we enable
it (!cstate.enable) before accessing registers.

What's for sure is that your implementation is introducing possible
unbalanced ref counting on the per clk.

> 
> 
> > 
> > 			imx_pwm_sw_reset(chip);
> > 		}
> > 
> > Otherwise you keep incrementing the prepare/enable counters of the clk
> > when you change the config of an already enabled PWM.
> >   
> > > +
> > > +		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > > +		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > > +
> > > +		writel(MX3_PWMCR_PRESCALER(prescale) |
> > > +		       MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> > > +		       MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH
> > > |
> > > +		       MX3_PWMCR_EN,
> > > +		       imx->mmio_base + MX3_PWMCR);
> > > +
> > > +	} else {  
> > 
> > 	} else if (cstate.enabled) {
> > 
> > Again, this is to avoid unbalanced prepare/enable counters on the per
> > clk if the PWM config is changed several times when the PWM is
> > disabled.
> >   
> > > +		writel(0, imx->mmio_base + MX3_PWMCR);
> > > +
> > > +		clk_disable_unprepare(imx->clk_per);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device
> > > *pwm) {
> > >  	struct imx_chip *imx = to_imx_chip(chip);
> > > @@ -280,6 +346,7 @@ static struct pwm_ops imx_pwm_ops_v1 = {
> > >  };
> > >  
> > >  static struct pwm_ops imx_pwm_ops_v2 = {
> > > +	.apply = imx_pwm_apply_v2,
> > >  	.enable = imx_pwm_enable,
> > >  	.disable = imx_pwm_disable,
> > >  	.config = imx_pwm_config,  
> >   
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

Re: [PATCH v4 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Lukasz Majewski]

On Thu, 5 Jan 2017 10:19:35 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Thu, 5 Jan 2017 10:03:47 +0100
> Lukasz Majewski <lukma@denx.de> wrote:
> 
> > On Thu, 5 Jan 2017 09:50:35 +0100
> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> > 
> > > On Thu,  5 Jan 2017 00:36:50 +0100
> > > Lukasz Majewski <lukma@denx.de> wrote:
> > >   
> > > > 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 v4:
> > > > - Avoid recalculation of PWM parameters when disabling PWM
> > > > signal
> > > > - Unconditionally call clk_prepare_enable(imx->clk_per) and
> > > >   clk_disable_unprepare(imx->clk_per)  
> > > 
> > > I don't see that one, but I'm not sure this is actually needed.
> > > In the enabled path we enable the clk before accessing the
> > > registers, and in the disable path, assuming you change the code
> > > according to my suggestion, the clk should already be enabled
> > > when you write to MX3_PWMCR.
> > >   
> > > > 
> > > > Changes for v3:
> > > > - Remove ipg clock enable/disable functions
> > > > 
> > > > Changes for v2:
> > > > - None
> > > > ---
> > > >  drivers/pwm/pwm-imx.c | 67
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file
> > > > changed, 67 insertions(+)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > index 60cdc5c..134dd66 100644
> > > > --- a/drivers/pwm/pwm-imx.c
> > > > +++ b/drivers/pwm/pwm-imx.c
> > > > @@ -249,6 +249,72 @@ static int imx_pwm_config(struct pwm_chip
> > > > *chip, return ret;
> > > >  }
> > > >  
> > > > +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;
> > > > +	int ret;
> > > > +
> > > > +	pwm_get_state(pwm, &cstate);
> > > > +
> > > > +	if (state->enabled) {
> > > > +		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;
> > > > +  
> > > 
> > > Starting here...
> > >   
> > > > +		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);  
> > > 
> > > ... till here, should be replaced by:
> > > 
> > > 
> > > 		/*
> > > 		 * 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 {
> > > 			ret = clk_prepare_enable(imx->clk_per);
> > > 			if (ret)
> > > 				return ret;  
> > 
> > In the other mail you mentioned that the clock should be enabled
> > unconditionally to fix issue on iMX7 when we want to access
> > registers.
> 
> When I said unconditionally, I meant call
> clk_prepare_enable(imx->clk_per) just after pwm_get_state() (at the
> beginning of the function) and call clk_disable_(imx->clk_per) just
> before returning 0 at the end of the function. This way you're
> guaranteed that the clk is always enabled when you access registers in
> between. Of course, you still need the the clk_prepare_enable() and
> clk_disable_unprepare() in the enable and disable path to keep the clk
> enabled when the PWM is enabled, and to disable it when the clk is
> being disabled.

Why you didn't said this when I pasted pseudo code to the other e-mail
and explicitly asked if this is correct? By doing that I wanted to
avoid situations like this (so I will have to repost this again and
again .....).


> 
> But, while reviewing your patch I realized this was actually unneeded
> (see the explanation in my previous review).
> 
> > 
> > Now it depends on cstate.enabled flag. 
> > 
> > So we end up with 
> > 
> > if (state.enabled && !cstate.enabled)
> > 	clk_preapre_enable();
> 
> Yep, and that's correct.

And following patch:
http://patchwork.ozlabs.org/patch/709510/

address this issue.

> 
> > 
> > which was the reason of iMX7 failure reported by Stefan to v3.
> 
> Stefan, do you confirm that? I don't see how this can possibly fail
> since the clk is either already enabled (cstate.enabled) or we enable
> it (!cstate.enable) before accessing registers.

http://patchwork.ozlabs.org/patch/709510/

> 
> What's for sure is that your implementation is introducing possible
> unbalanced ref counting on the per clk.

You mean when somebody call twice ->apply() with "state->enabled".

> 
> > 
> > 
> > > 
> > > 			imx_pwm_sw_reset(chip);
> > > 		}
> > > 
> > > Otherwise you keep incrementing the prepare/enable counters of
> > > the clk when you change the config of an already enabled PWM.
> > >   
> > > > +
> > > > +		writel(duty_cycles, imx->mmio_base +
> > > > MX3_PWMSAR);
> > > > +		writel(period_cycles, imx->mmio_base +
> > > > MX3_PWMPR); +
> > > > +		writel(MX3_PWMCR_PRESCALER(prescale) |
> > > > +		       MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> > > > +		       MX3_PWMCR_DBGEN |
> > > > MX3_PWMCR_CLKSRC_IPG_HIGH |
> > > > +		       MX3_PWMCR_EN,
> > > > +		       imx->mmio_base + MX3_PWMCR);
> > > > +
> > > > +	} else {  
> > > 
> > > 	} else if (cstate.enabled) {
> > > 
> > > Again, this is to avoid unbalanced prepare/enable counters on the
> > > per clk if the PWM config is changed several times when the PWM is
> > > disabled.
> > >   
> > > > +		writel(0, imx->mmio_base + MX3_PWMCR);
> > > > +
> > > > +		clk_disable_unprepare(imx->clk_per);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static int imx_pwm_enable(struct pwm_chip *chip, struct
> > > > pwm_device *pwm) {
> > > >  	struct imx_chip *imx = to_imx_chip(chip);
> > > > @@ -280,6 +346,7 @@ static struct pwm_ops imx_pwm_ops_v1 = {
> > > >  };
> > > >  
> > > >  static struct pwm_ops imx_pwm_ops_v2 = {
> > > > +	.apply = imx_pwm_apply_v2,
> > > >  	.enable = imx_pwm_enable,
> > > >  	.disable = imx_pwm_disable,
> > > >  	.config = imx_pwm_config,  
> > >   
> > 
> > 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email:
> > wd@denx.de
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

Re: [PATCH v4 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Boris Brezillon]

On Thu, 5 Jan 2017 10:35:05 +0100
Lukasz Majewski <lukma@denx.de> wrote:

> On Thu, 5 Jan 2017 10:19:35 +0100
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > On Thu, 5 Jan 2017 10:03:47 +0100
> > Lukasz Majewski <lukma@denx.de> wrote:
> >   
> > > On Thu, 5 Jan 2017 09:50:35 +0100
> > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> > >   
> > > > On Thu,  5 Jan 2017 00:36:50 +0100
> > > > Lukasz Majewski <lukma@denx.de> wrote:
> > > >     
> > > > > 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 v4:
> > > > > - Avoid recalculation of PWM parameters when disabling PWM
> > > > > signal
> > > > > - Unconditionally call clk_prepare_enable(imx->clk_per) and
> > > > >   clk_disable_unprepare(imx->clk_per)    
> > > > 
> > > > I don't see that one, but I'm not sure this is actually needed.
> > > > In the enabled path we enable the clk before accessing the
> > > > registers, and in the disable path, assuming you change the code
> > > > according to my suggestion, the clk should already be enabled
> > > > when you write to MX3_PWMCR.
> > > >     
> > > > > 
> > > > > Changes for v3:
> > > > > - Remove ipg clock enable/disable functions
> > > > > 
> > > > > Changes for v2:
> > > > > - None
> > > > > ---
> > > > >  drivers/pwm/pwm-imx.c | 67
> > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file
> > > > > changed, 67 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > > index 60cdc5c..134dd66 100644
> > > > > --- a/drivers/pwm/pwm-imx.c
> > > > > +++ b/drivers/pwm/pwm-imx.c
> > > > > @@ -249,6 +249,72 @@ static int imx_pwm_config(struct pwm_chip
> > > > > *chip, return ret;
> > > > >  }
> > > > >  
> > > > > +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;
> > > > > +	int ret;
> > > > > +
> > > > > +	pwm_get_state(pwm, &cstate);
> > > > > +
> > > > > +	if (state->enabled) {
> > > > > +		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;
> > > > > +    
> > > > 
> > > > Starting here...
> > > >     
> > > > > +		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);    
> > > > 
> > > > ... till here, should be replaced by:
> > > > 
> > > > 
> > > > 		/*
> > > > 		 * 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 {
> > > > 			ret = clk_prepare_enable(imx->clk_per);
> > > > 			if (ret)
> > > > 				return ret;    
> > > 
> > > In the other mail you mentioned that the clock should be enabled
> > > unconditionally to fix issue on iMX7 when we want to access
> > > registers.  
> > 
> > When I said unconditionally, I meant call
> > clk_prepare_enable(imx->clk_per) just after pwm_get_state() (at the
> > beginning of the function) and call clk_disable_(imx->clk_per) just
> > before returning 0 at the end of the function. This way you're
> > guaranteed that the clk is always enabled when you access registers in
> > between. Of course, you still need the the clk_prepare_enable() and
> > clk_disable_unprepare() in the enable and disable path to keep the clk
> > enabled when the PWM is enabled, and to disable it when the clk is
> > being disabled.  
> 
> Why you didn't said this when I pasted pseudo code to the other e-mail
> and explicitly asked if this is correct? By doing that I wanted to
> avoid situations like this (so I will have to repost this again and
> again .....).

Because I thought the unconditional clk_enable/disable was not part of
your example and would be added later, and I must admit I didn't
carefully read the code chunk. Anyway, it's not a big deal, you're only
at v4.

> 
> 
> > 
> > But, while reviewing your patch I realized this was actually unneeded
> > (see the explanation in my previous review).
> >   
> > > 
> > > Now it depends on cstate.enabled flag. 
> > > 
> > > So we end up with 
> > > 
> > > if (state.enabled && !cstate.enabled)
> > > 	clk_preapre_enable();  
> > 
> > Yep, and that's correct.  
> 
> And following patch:
> http://patchwork.ozlabs.org/patch/709510/
> 
> address this issue.

Yes, that was needed because the enable/disable path were not
separated, and we were unconditionally writing to the IP registers
even when the PWM was already disabled (which is probably the case
generating the fault reported by Stefan). This is not the case anymore,
but let's wait for Stefan to confirm this.

Also note that Stefan's patch is introducing an 'unbalanced clk
prepapre/enable refcount' bug if ->apply() is called several times
consecutively with the same state->enabled value.

> 
> >   
> > > 
> > > which was the reason of iMX7 failure reported by Stefan to v3.  
> > 
> > Stefan, do you confirm that? I don't see how this can possibly fail
> > since the clk is either already enabled (cstate.enabled) or we enable
> > it (!cstate.enable) before accessing registers.  
> 
> http://patchwork.ozlabs.org/patch/709510/
> 
> > 
> > What's for sure is that your implementation is introducing possible
> > unbalanced ref counting on the per clk.  
> 
> You mean when somebody call twice ->apply() with "state->enabled".

Yep, when someone calls twice ->apply() with state->enabled set to true
or twice with state->enabled set to false.

Re: [PATCH v4 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Andy Shevchenko]

On Thu, Jan 5, 2017 at 11:19 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Thu, 5 Jan 2017 10:03:47 +0100
> Lukasz Majewski <lukma@denx.de> wrote:
>> >             /*
>> >              * 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) {

if (pwm_is_enabled()) ?

I think it's better to do whatever API provides to be less error prone.

>> >                     imx_pwm_wait_fifo_slot(chip, pwm);
>> >             } else {
>> >                     ret = clk_prepare_enable(imx->clk_per);
>> >                     if (ret)
>> >                             return ret;

>> if (state.enabled && !cstate.enabled)
>>       clk_preapre_enable();
>
> Yep, and that's correct.

!pwm_is_enabled() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Boris Brezillon]

On Thu, 5 Jan 2017 23:15:06 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jan 5, 2017 at 11:19 AM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > On Thu, 5 Jan 2017 10:03:47 +0100
> > Lukasz Majewski <lukma@denx.de> wrote:  
> >> >             /*
> >> >              * 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) {  
> 
> if (pwm_is_enabled()) ?
> 
> I think it's better to do whatever API provides to be less error prone.

Both pwm_is_enabled() and pwm_get_state()+struct pwm_state are part of
the PWM API, and I don't see how 'if (pwm_is_enabled())' is less error
prone than 'if (cstate.enabled)'.

This being said, I don't care much. It's mainly a matter of taste IMO,
so if others agree to switch to pwm_is_enabled() I'm fine with that.

> 
> >> >                     imx_pwm_wait_fifo_slot(chip, pwm);
> >> >             } else {
> >> >                     ret = clk_prepare_enable(imx->clk_per);
> >> >                     if (ret)
> >> >                             return ret;  
> 
> >> if (state.enabled && !cstate.enabled)
> >>       clk_preapre_enable();  
> >
> > Yep, and that's correct.  
> 
> !pwm_is_enabled() ?
> 

Re: [PATCH v4 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Lukasz Majewski]

Hi Stefan,

> On Thu, 5 Jan 2017 10:03:47 +0100
> Lukasz Majewski <lukma@denx.de> wrote:
> 
> > On Thu, 5 Jan 2017 09:50:35 +0100
> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> > 
> > > On Thu,  5 Jan 2017 00:36:50 +0100
> > > Lukasz Majewski <lukma@denx.de> wrote:
> > >   
> > > > 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 v4:
> > > > - Avoid recalculation of PWM parameters when disabling PWM
> > > > signal
> > > > - Unconditionally call clk_prepare_enable(imx->clk_per) and
> > > >   clk_disable_unprepare(imx->clk_per)  
> > > 
> > > I don't see that one, but I'm not sure this is actually needed.
> > > In the enabled path we enable the clk before accessing the
> > > registers, and in the disable path, assuming you change the code
> > > according to my suggestion, the clk should already be enabled
> > > when you write to MX3_PWMCR.
> > >   
> > > > 
> > > > Changes for v3:
> > > > - Remove ipg clock enable/disable functions
> > > > 
> > > > Changes for v2:
> > > > - None
> > > > ---
> > > >  drivers/pwm/pwm-imx.c | 67
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file
> > > > changed, 67 insertions(+)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > index 60cdc5c..134dd66 100644
> > > > --- a/drivers/pwm/pwm-imx.c
> > > > +++ b/drivers/pwm/pwm-imx.c
> > > > @@ -249,6 +249,72 @@ static int imx_pwm_config(struct pwm_chip
> > > > *chip, return ret;
> > > >  }
> > > >  
> > > > +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;
> > > > +	int ret;
> > > > +
> > > > +	pwm_get_state(pwm, &cstate);
> > > > +
> > > > +	if (state->enabled) {
> > > > +		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;
> > > > +  
> > > 
> > > Starting here...
> > >   
> > > > +		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);  
> > > 
> > > ... till here, should be replaced by:
> > > 
> > > 
> > > 		/*
> > > 		 * 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 {
> > > 			ret = clk_prepare_enable(imx->clk_per);
> > > 			if (ret)
> > > 				return ret;  
> > 
> > In the other mail you mentioned that the clock should be enabled
> > unconditionally to fix issue on iMX7 when we want to access
> > registers.
> 
> When I said unconditionally, I meant call
> clk_prepare_enable(imx->clk_per) just after pwm_get_state() (at the
> beginning of the function) and call clk_disable_(imx->clk_per) just
> before returning 0 at the end of the function. This way you're
> guaranteed that the clk is always enabled when you access registers in
> between. Of course, you still need the the clk_prepare_enable() and
> clk_disable_unprepare() in the enable and disable path to keep the clk
> enabled when the PWM is enabled, and to disable it when the clk is
> being disabled.
> 
> But, while reviewing your patch I realized this was actually unneeded
> (see the explanation in my previous review).
> 
> > 
> > Now it depends on cstate.enabled flag. 
> > 
> > So we end up with 
> > 
> > if (state.enabled && !cstate.enabled)
> > 	clk_preapre_enable();
> 
> Yep, and that's correct.
> 
> > 
> > which was the reason of iMX7 failure reported by Stefan to v3.
> 
> Stefan, do you confirm that? I don't see how this can possibly fail
> since the clk is either already enabled (cstate.enabled) or we enable
> it (!cstate.enable) before accessing registers.

Stefan, could you respond on this issue?

Thanks in advance,
Łukasz Majewski

> 
> What's for sure is that your implementation is introducing possible
> unbalanced ref counting on the per clk.
> 
> > 
> > 
> > > 
> > > 			imx_pwm_sw_reset(chip);
> > > 		}
> > > 
> > > Otherwise you keep incrementing the prepare/enable counters of
> > > the clk when you change the config of an already enabled PWM.
> > >   
> > > > +
> > > > +		writel(duty_cycles, imx->mmio_base +
> > > > MX3_PWMSAR);
> > > > +		writel(period_cycles, imx->mmio_base +
> > > > MX3_PWMPR); +
> > > > +		writel(MX3_PWMCR_PRESCALER(prescale) |
> > > > +		       MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> > > > +		       MX3_PWMCR_DBGEN |
> > > > MX3_PWMCR_CLKSRC_IPG_HIGH |
> > > > +		       MX3_PWMCR_EN,
> > > > +		       imx->mmio_base + MX3_PWMCR);
> > > > +
> > > > +	} else {  
> > > 
> > > 	} else if (cstate.enabled) {
> > > 
> > > Again, this is to avoid unbalanced prepare/enable counters on the
> > > per clk if the PWM config is changed several times when the PWM is
> > > disabled.
> > >   
> > > > +		writel(0, imx->mmio_base + MX3_PWMCR);
> > > > +
> > > > +		clk_disable_unprepare(imx->clk_per);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static int imx_pwm_enable(struct pwm_chip *chip, struct
> > > > pwm_device *pwm) {
> > > >  	struct imx_chip *imx = to_imx_chip(chip);
> > > > @@ -280,6 +346,7 @@ static struct pwm_ops imx_pwm_ops_v1 = {
> > > >  };
> > > >  
> > > >  static struct pwm_ops imx_pwm_ops_v2 = {
> > > > +	.apply = imx_pwm_apply_v2,
> > > >  	.enable = imx_pwm_enable,
> > > >  	.disable = imx_pwm_disable,
> > > >  	.config = imx_pwm_config,  
> > >   
> > 
> > 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email:
> > wd@denx.de
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

Re: [PATCH v4 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Stefan Agner]

Hi Lukasz,

Sry for the delay, I was traveling and had no access to hardware...

On 2017-01-05 01:53, Boris Brezillon wrote:
> On Thu, 5 Jan 2017 10:35:05 +0100
> Lukasz Majewski <lukma@denx.de> wrote:
> 
>> On Thu, 5 Jan 2017 10:19:35 +0100
>> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>>
>> > On Thu, 5 Jan 2017 10:03:47 +0100
>> > Lukasz Majewski <lukma@denx.de> wrote:
>> >
>> > > On Thu, 5 Jan 2017 09:50:35 +0100
>> > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>> > >
>> > > > On Thu,  5 Jan 2017 00:36:50 +0100
>> > > > Lukasz Majewski <lukma@denx.de> wrote:
>> > > >
>> > > > > 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 v4:
>> > > > > - Avoid recalculation of PWM parameters when disabling PWM
>> > > > > signal
>> > > > > - Unconditionally call clk_prepare_enable(imx->clk_per) and
>> > > > >   clk_disable_unprepare(imx->clk_per)
>> > > >
>> > > > I don't see that one, but I'm not sure this is actually needed.
>> > > > In the enabled path we enable the clk before accessing the
>> > > > registers, and in the disable path, assuming you change the code
>> > > > according to my suggestion, the clk should already be enabled
>> > > > when you write to MX3_PWMCR.
>> > > >
>> > > > >
>> > > > > Changes for v3:
>> > > > > - Remove ipg clock enable/disable functions
>> > > > >
>> > > > > Changes for v2:
>> > > > > - None
>> > > > > ---
>> > > > >  drivers/pwm/pwm-imx.c | 67
>> > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file
>> > > > > changed, 67 insertions(+)
>> > > > >
>> > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
>> > > > > index 60cdc5c..134dd66 100644
>> > > > > --- a/drivers/pwm/pwm-imx.c
>> > > > > +++ b/drivers/pwm/pwm-imx.c
>> > > > > @@ -249,6 +249,72 @@ static int imx_pwm_config(struct pwm_chip
>> > > > > *chip, return ret;
>> > > > >  }
>> > > > >
>> > > > > +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;
>> > > > > +	int ret;
>> > > > > +
>> > > > > +	pwm_get_state(pwm, &cstate);
>> > > > > +
>> > > > > +	if (state->enabled) {
>> > > > > +		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;
>> > > > > +
>> > > >
>> > > > Starting here...
>> > > >
>> > > > > +		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);
>> > > >
>> > > > ... till here, should be replaced by:
>> > > >
>> > > >
>> > > > 		/*
>> > > > 		 * 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 {
>> > > > 			ret = clk_prepare_enable(imx->clk_per);
>> > > > 			if (ret)
>> > > > 				return ret;
>> > >
>> > > In the other mail you mentioned that the clock should be enabled
>> > > unconditionally to fix issue on iMX7 when we want to access
>> > > registers.
>> >
>> > When I said unconditionally, I meant call
>> > clk_prepare_enable(imx->clk_per) just after pwm_get_state() (at the
>> > beginning of the function) and call clk_disable_(imx->clk_per) just
>> > before returning 0 at the end of the function. This way you're
>> > guaranteed that the clk is always enabled when you access registers in
>> > between. Of course, you still need the the clk_prepare_enable() and
>> > clk_disable_unprepare() in the enable and disable path to keep the clk
>> > enabled when the PWM is enabled, and to disable it when the clk is
>> > being disabled.
>>
>> Why you didn't said this when I pasted pseudo code to the other e-mail
>> and explicitly asked if this is correct? By doing that I wanted to
>> avoid situations like this (so I will have to repost this again and
>> again .....).
> 
> Because I thought the unconditional clk_enable/disable was not part of
> your example and would be added later, and I must admit I didn't
> carefully read the code chunk. Anyway, it's not a big deal, you're only
> at v4.
> 
>>
>>
>> >
>> > But, while reviewing your patch I realized this was actually unneeded
>> > (see the explanation in my previous review).
>> >
>> > >
>> > > Now it depends on cstate.enabled flag.
>> > >
>> > > So we end up with
>> > >
>> > > if (state.enabled && !cstate.enabled)
>> > > 	clk_preapre_enable();
>> >
>> > Yep, and that's correct.
>>
>> And following patch:
>> http://patchwork.ozlabs.org/patch/709510/
>>
>> address this issue.
> 
> Yes, that was needed because the enable/disable path were not
> separated, and we were unconditionally writing to the IP registers
> even when the PWM was already disabled (which is probably the case
> generating the fault reported by Stefan). This is not the case anymore,
> but let's wait for Stefan to confirm this.

With v4 as is, the kernel crashes/hangs on i.MX 7.

The function imx_pwm_apply_v2 gets first called with state->enabled 0,
cstate->enabled 0. This branches to else and leads to a register access
with clocks disabled (and if that would succeed, also an unbalanced
disable?...)

With the proposed change plus the additional change in the else branch
it works for me:

@@ -192,19 +193,20 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip,
struct pwm_device *pwm,
                else
                        period_cycles = 0;
 
-               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)
+               if (cstate.enabled) {
                        imx_pwm_wait_fifo_slot(chip, pwm);
-               else if (state->enabled)
+               } else if (state->enabled) {
+                       ret = clk_prepare_enable(imx->clk_per);
+                       if (ret)
+                               return ret;
+
                        imx_pwm_sw_reset(chip);
+               }
 
                writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
                writel(period_cycles, imx->mmio_base + MX3_PWMPR);
@@ -218,7 +220,7 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip,
struct pwm_device *pwm,
                        cr |= MX3_PWMCR_POUTC;
 
                writel(cr, imx->mmio_base + MX3_PWMCR);
-       } else {
+       } else if (cstate.enabled) {
                writel(0, imx->mmio_base + MX3_PWMCR);
 
                clk_disable_unprepare(imx->clk_per);


This would not disable a disabled PWM anymore, I guess at normal use not
a problem. Only at bootup it could end up left on, but I guess if we
care about boot time transition we should implement get_state, but
something which we can do in a follow up patch.

> 
> Also note that Stefan's patch is introducing an 'unbalanced clk
> prepapre/enable refcount' bug if ->apply() is called several times
> consecutively with the same state->enabled value.

Hm, agreed on that one.

--
Stefan

> 
>>
>> >
>> > >
>> > > which was the reason of iMX7 failure reported by Stefan to v3.
>> >
>> > Stefan, do you confirm that? I don't see how this can possibly fail
>> > since the clk is either already enabled (cstate.enabled) or we enable
>> > it (!cstate.enable) before accessing registers.
>>
>> http://patchwork.ozlabs.org/patch/709510/
>>
>> >
>> > What's for sure is that your implementation is introducing possible
>> > unbalanced ref counting on the per clk.
>>
>> You mean when somebody call twice ->apply() with "state->enabled".
> 
> Yep, when someone calls twice ->apply() with state->enabled set to true
> or twice with state->enabled set to false.

Re: [PATCH v4 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Boris Brezillon]

On Mon, 09 Jan 2017 19:14:43 -0800
Stefan Agner <stefan@agner.ch> wrote:

> >>  
> >> >
> >> > But, while reviewing your patch I realized this was actually unneeded
> >> > (see the explanation in my previous review).
> >> >  
> >> > >
> >> > > Now it depends on cstate.enabled flag.
> >> > >
> >> > > So we end up with
> >> > >
> >> > > if (state.enabled && !cstate.enabled)
> >> > > 	clk_preapre_enable();  
> >> >
> >> > Yep, and that's correct.  
> >>
> >> And following patch:
> >> http://patchwork.ozlabs.org/patch/709510/
> >>
> >> address this issue.  
> > 
> > Yes, that was needed because the enable/disable path were not
> > separated, and we were unconditionally writing to the IP registers
> > even when the PWM was already disabled (which is probably the case
> > generating the fault reported by Stefan). This is not the case anymore,
> > but let's wait for Stefan to confirm this.  
> 
> With v4 as is, the kernel crashes/hangs on i.MX 7.
> 
> The function imx_pwm_apply_v2 gets first called with state->enabled 0,
> cstate->enabled 0. This branches to else and leads to a register access
> with clocks disabled (and if that would succeed, also an unbalanced
> disable?...)
> 
> With the proposed change plus the additional change in the else branch
> it works for me:
> 
> @@ -192,19 +193,20 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip,
> struct pwm_device *pwm,
>                 else
>                         period_cycles = 0;
>  
> -               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)
> +               if (cstate.enabled) {
>                         imx_pwm_wait_fifo_slot(chip, pwm);
> -               else if (state->enabled)
> +               } else if (state->enabled) {
> +                       ret = clk_prepare_enable(imx->clk_per);
> +                       if (ret)
> +                               return ret;
> +
>                         imx_pwm_sw_reset(chip);
> +               }
>  
>                 writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>                 writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> @@ -218,7 +220,7 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip,
> struct pwm_device *pwm,
>                         cr |= MX3_PWMCR_POUTC;
>  
>                 writel(cr, imx->mmio_base + MX3_PWMCR);
> -       } else {
> +       } else if (cstate.enabled) {
>                 writel(0, imx->mmio_base + MX3_PWMCR);
>  
>                 clk_disable_unprepare(imx->clk_per);
> 
> 
> This would not disable a disabled PWM anymore, I guess at normal use not
> a problem. Only at bootup it could end up left on, but I guess if we
> care about boot time transition we should implement get_state, but
> something which we can do in a follow up patch.

Yep, that's a different problem which could be addressed by
implementing ->get_state(). Note that you don't necessary want to
disable the PWM at boot time, in some situation, when the PWM is
driving a critical device (like the VDDIODDR regulator), you want the
transition between the bootloader/firmware and Linux to be as smooth as
possible. Actually, 'initial state retrieval' and 'atomic changes'
were added to handle this case.

Stefan, one last thing, can you apply patch 2 alone and check if it
doesn't introduce a regression?

Thanks,

Boris

Re: [PATCH v4 02/11] pwm: imx: remove ipg clock and enable per clock when required

[Stefan Agner]

On 2017-01-04 15:36, Lukasz Majewski wrote:
> From: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
> The use of the ipg clock was introduced with commit 7b27c160c681
> ("pwm: i.MX: fix clock lookup").
> 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.
> 
> In the other hand, the imx7 IP requires the peripheral clock to be

Nit:

s/In/On?

> enabled before accessing its registers. Since ->config() can be called
> when the PWM is disabled (in which case, the peripheral clock is also
> disabled), we need to surround the imx->config() with
> clk_prepare_enable(per_clk)/clk_disable_unprepare(per_clk) calls.
> 
> Note that the imx7 IP was working fine so far because the ipg clock was
> actually pointing to the peripheral clock, which guaranteed peripheral
> clock activation even when ->config() was called when the PWM was
> disabled.

That is not entirely true, but almost, I would say:

"Note that the driver was working fine for the i.MX 7 IP so far because
the ipg and peripheral clock use the same hardware clock gate, which
guaranteed..."

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>

Reviewed-by: Stefan Agner <stefan@agner.ch>

Tried on-top of 4.10-rc3, only this patch, still works.

Tested-by: Stefan Agner <stefan@agner.ch>

--
Stefan

> ---
> Changes in v4:
> - Enable per clk before calling imx->config()
> 
> Changes in v3:
> - New patch
> ---
>  drivers/pwm/pwm-imx.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d600fd5..b1d1e50 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;
>  
> @@ -206,13 +205,13 @@ static int imx_pwm_config(struct pwm_chip *chip,
>  	struct imx_chip *imx = to_imx_chip(chip);
>  	int ret;
>  
> -	ret = clk_prepare_enable(imx->clk_ipg);
> +	ret = clk_prepare_enable(imx->clk_per);
>  	if (ret)
>  		return ret;
>  
>  	ret = imx->config(chip, pwm, duty_ns, period_ns);
>  
> -	clk_disable_unprepare(imx->clk_ipg);
> +	clk_disable_unprepare(imx->clk_per);
>  
>  	return ret;
>  }
> @@ -293,13 +292,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;