[PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver

[PATCH 0/6] 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:
- 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) in a single patch
  for better readability.

- Remove redundant PWM code (disable, enable, config callbacks)

- Clean up the driver infrastructure

- Provide "polarity_supported" flag to indicate support for polarity 
  inversion

This work should be applied on top of following commits:

http://patchwork.ozlabs.org/patch/679706/
http://patchwork.ozlabs.org/patch/679707/
http://patchwork.ozlabs.org/patch/679680/


Test HW:
--------
This patch set has been tested on i.MX6q board with vanilla 4.7 kernel.
It applies clearly on 4.9-rcX SHA1: 0c2b6dc4fd4fa13796b319aae969a009f03222c6


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

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


Lukasz Majewski (6):
  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 IMXv2 PWM
  pwm: imx: Remove redundant IMX PWMv2 code
  pwm: imx: Introduce "polarity_supported" flag to PWMv2 driver

 drivers/pwm/pwm-imx.c | 262 ++++++++++++++++++++++----------------------------
 1 file changed, 114 insertions(+), 148 deletions(-)

-- 
2.1.4

[PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Lukasz Majewski]

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

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

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

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index c37d223..83e43d5 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -66,8 +66,6 @@ struct imx_chip {
 static int imx_pwm_config_v1(struct pwm_chip *chip,
 		struct pwm_device *pwm, int duty_ns, int period_ns)
 {
-	struct imx_chip *imx = to_imx_chip(chip);
-
 	/*
 	 * The PWM subsystem allows for exact frequencies. However,
 	 * I cannot connect a scope on my device to the PWM line and
@@ -85,26 +83,52 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
 	 * both the prescaler (/1 .. /128) and then by CLKSEL
 	 * (/2 .. /16).
 	 */
+	struct imx_chip *imx = to_imx_chip(chip);
 	u32 max = readl(imx->mmio_base + MX1_PWMP);
 	u32 p = max * duty_ns / period_ns;
+	int ret;
+
+	ret = clk_prepare_enable(imx->clk_ipg);
+	if (ret)
+		return ret;
+
 	writel(max - p, imx->mmio_base + MX1_PWMS);
 
+	clk_disable_unprepare(imx->clk_ipg);
+
 	return 0;
 }
 
-static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
+static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct imx_chip *imx = to_imx_chip(chip);
+	int ret;
 	u32 val;
 
+	ret = clk_prepare_enable(imx->clk_ipg);
+	if (ret)
+		return ret;
+
 	val = readl(imx->mmio_base + MX1_PWMC);
+	val |= MX1_PWMC_EN;
+	writel(val, imx->mmio_base + MX1_PWMC);
 
-	if (enable)
-		val |= MX1_PWMC_EN;
-	else
-		val &= ~MX1_PWMC_EN;
+	clk_disable_unprepare(imx->clk_per);
+
+	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,
@@ -269,9 +293,9 @@ static int imx_pwm_set_polarity(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,
 };
 
@@ -291,8 +315,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

Re: [PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Boris Brezillon]

On Sun, 23 Oct 2016 23:45:41 +0200
Lukasz Majewski <l.majewski@majess.pl> wrote:

> The code has been rewritten to remove "generic" calls to
> imx_pwm_{enable|disable|config}.
> 
> Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> implementation.
> 
> Suggested-by: Stefan Agner <stefan@agner.ch>
> Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>

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

> ---
>  drivers/pwm/pwm-imx.c | 46 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index c37d223..83e43d5 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -66,8 +66,6 @@ struct imx_chip {
>  static int imx_pwm_config_v1(struct pwm_chip *chip,
>  		struct pwm_device *pwm, int duty_ns, int period_ns)
>  {
> -	struct imx_chip *imx = to_imx_chip(chip);
> -
>  	/*
>  	 * The PWM subsystem allows for exact frequencies. However,
>  	 * I cannot connect a scope on my device to the PWM line and
> @@ -85,26 +83,52 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
>  	 * both the prescaler (/1 .. /128) and then by CLKSEL
>  	 * (/2 .. /16).
>  	 */
> +	struct imx_chip *imx = to_imx_chip(chip);
>  	u32 max = readl(imx->mmio_base + MX1_PWMP);
>  	u32 p = max * duty_ns / period_ns;
> +	int ret;
> +
> +	ret = clk_prepare_enable(imx->clk_ipg);
> +	if (ret)
> +		return ret;
> +
>  	writel(max - p, imx->mmio_base + MX1_PWMS);
>  
> +	clk_disable_unprepare(imx->clk_ipg);
> +
>  	return 0;
>  }
>  
> -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct imx_chip *imx = to_imx_chip(chip);
> +	int ret;
>  	u32 val;
>  
> +	ret = clk_prepare_enable(imx->clk_ipg);
> +	if (ret)
> +		return ret;
> +
>  	val = readl(imx->mmio_base + MX1_PWMC);
> +	val |= MX1_PWMC_EN;
> +	writel(val, imx->mmio_base + MX1_PWMC);
>  
> -	if (enable)
> -		val |= MX1_PWMC_EN;
> -	else
> -		val &= ~MX1_PWMC_EN;
> +	clk_disable_unprepare(imx->clk_per);
> +
> +	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,
> @@ -269,9 +293,9 @@ static int imx_pwm_set_polarity(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,
>  };
>  
> @@ -291,8 +315,6 @@ struct imx_pwm_data {
>  };
>  
>  static struct imx_pwm_data imx_pwm_data_v1 = {
> -	.config = imx_pwm_config_v1,
> -	.set_enable = imx_pwm_set_enable_v1,
>  	.pwm_ops = &imx_pwm_ops_v1,
>  };
>  

Re: [PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Sascha Hauer]

On Sun, Oct 23, 2016 at 11:45:41PM +0200, Lukasz Majewski wrote:
> The code has been rewritten to remove "generic" calls to
> imx_pwm_{enable|disable|config}.
> 
> Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> implementation.
> 
> Suggested-by: Stefan Agner <stefan@agner.ch>
> Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
>  drivers/pwm/pwm-imx.c | 46 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index c37d223..83e43d5 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -66,8 +66,6 @@ struct imx_chip {
>  static int imx_pwm_config_v1(struct pwm_chip *chip,
>  		struct pwm_device *pwm, int duty_ns, int period_ns)
>  {
> -	struct imx_chip *imx = to_imx_chip(chip);
> -
>  	/*
>  	 * The PWM subsystem allows for exact frequencies. However,
>  	 * I cannot connect a scope on my device to the PWM line and
> @@ -85,26 +83,52 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
>  	 * both the prescaler (/1 .. /128) and then by CLKSEL
>  	 * (/2 .. /16).
>  	 */
> +	struct imx_chip *imx = to_imx_chip(chip);
>  	u32 max = readl(imx->mmio_base + MX1_PWMP);
>  	u32 p = max * duty_ns / period_ns;
> +	int ret;
> +
> +	ret = clk_prepare_enable(imx->clk_ipg);
> +	if (ret)
> +		return ret;
> +
>  	writel(max - p, imx->mmio_base + MX1_PWMS);
>  
> +	clk_disable_unprepare(imx->clk_ipg);
> +
>  	return 0;
>  }
>  
> -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct imx_chip *imx = to_imx_chip(chip);
> +	int ret;
>  	u32 val;
>  
> +	ret = clk_prepare_enable(imx->clk_ipg);
> +	if (ret)
> +		return ret;
> +
>  	val = readl(imx->mmio_base + MX1_PWMC);
> +	val |= MX1_PWMC_EN;
> +	writel(val, imx->mmio_base + MX1_PWMC);
>  
> -	if (enable)
> -		val |= MX1_PWMC_EN;
> -	else
> -		val &= ~MX1_PWMC_EN;
> +	clk_disable_unprepare(imx->clk_per);

This looks wrong. You start by enabling clk_ipg which is needed for
register access, but then end with disabling clk_per which is needed for
driving the actual PWM output. This function should probably enable
clk_per on entry and enable clk_ipg while accessing registers.

Sascha

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

Re: [PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Boris Brezillon]

On Tue, 25 Oct 2016 07:54:54 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Sun, Oct 23, 2016 at 11:45:41PM +0200, Lukasz Majewski wrote:
> > The code has been rewritten to remove "generic" calls to
> > imx_pwm_{enable|disable|config}.
> > 
> > Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> > implementation.
> > 
> > Suggested-by: Stefan Agner <stefan@agner.ch>
> > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > ---
> >  drivers/pwm/pwm-imx.c | 46 ++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 34 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index c37d223..83e43d5 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -66,8 +66,6 @@ struct imx_chip {
> >  static int imx_pwm_config_v1(struct pwm_chip *chip,
> >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> >  {
> > -	struct imx_chip *imx = to_imx_chip(chip);
> > -
> >  	/*
> >  	 * The PWM subsystem allows for exact frequencies. However,
> >  	 * I cannot connect a scope on my device to the PWM line and
> > @@ -85,26 +83,52 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
> >  	 * both the prescaler (/1 .. /128) and then by CLKSEL
> >  	 * (/2 .. /16).
> >  	 */
> > +	struct imx_chip *imx = to_imx_chip(chip);
> >  	u32 max = readl(imx->mmio_base + MX1_PWMP);
> >  	u32 p = max * duty_ns / period_ns;
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(imx->clk_ipg);
> > +	if (ret)
> > +		return ret;
> > +
> >  	writel(max - p, imx->mmio_base + MX1_PWMS);
> >  
> > +	clk_disable_unprepare(imx->clk_ipg);
> > +
> >  	return 0;
> >  }
> >  
> > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> > +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> >  	struct imx_chip *imx = to_imx_chip(chip);
> > +	int ret;
> >  	u32 val;
> >  
> > +	ret = clk_prepare_enable(imx->clk_ipg);
> > +	if (ret)
> > +		return ret;
> > +
> >  	val = readl(imx->mmio_base + MX1_PWMC);
> > +	val |= MX1_PWMC_EN;
> > +	writel(val, imx->mmio_base + MX1_PWMC);
> >  
> > -	if (enable)
> > -		val |= MX1_PWMC_EN;
> > -	else
> > -		val &= ~MX1_PWMC_EN;
> > +	clk_disable_unprepare(imx->clk_per);  
> 
> This looks wrong. You start by enabling clk_ipg which is needed for
> register access, but then end with disabling clk_per which is needed for
> driving the actual PWM output. This function should probably enable
> clk_per on entry and enable clk_ipg while accessing registers.

Oh, I didn't notice there was 2 different clocks here. This probably
means you have to enable clk_ipg when manipulating the registers in
->disable().

One question, if there's a separate clk for the registers, why don't we
leave this clock enabled, and disable it in ->suspend() or ->remove()
instead of enabling/disabling it each time we access the registers?

Re: [PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Sascha Hauer]

On Tue, Oct 25, 2016 at 08:27:37AM +0200, Boris Brezillon wrote:
> On Tue, 25 Oct 2016 07:54:54 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > On Sun, Oct 23, 2016 at 11:45:41PM +0200, Lukasz Majewski wrote:
> > > The code has been rewritten to remove "generic" calls to
> > > imx_pwm_{enable|disable|config}.
> > > 
> > > Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> > > implementation.
> > > 
> > > Suggested-by: Stefan Agner <stefan@agner.ch>
> > > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > > ---
> > >  drivers/pwm/pwm-imx.c | 46 ++++++++++++++++++++++++++++++++++------------
> > >  1 file changed, 34 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > index c37d223..83e43d5 100644
> > > --- a/drivers/pwm/pwm-imx.c
> > > +++ b/drivers/pwm/pwm-imx.c
> > > @@ -66,8 +66,6 @@ struct imx_chip {
> > >  static int imx_pwm_config_v1(struct pwm_chip *chip,
> > >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> > >  {
> > > -	struct imx_chip *imx = to_imx_chip(chip);
> > > -
> > >  	/*
> > >  	 * The PWM subsystem allows for exact frequencies. However,
> > >  	 * I cannot connect a scope on my device to the PWM line and
> > > @@ -85,26 +83,52 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
> > >  	 * both the prescaler (/1 .. /128) and then by CLKSEL
> > >  	 * (/2 .. /16).
> > >  	 */
> > > +	struct imx_chip *imx = to_imx_chip(chip);
> > >  	u32 max = readl(imx->mmio_base + MX1_PWMP);
> > >  	u32 p = max * duty_ns / period_ns;
> > > +	int ret;
> > > +
> > > +	ret = clk_prepare_enable(imx->clk_ipg);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	writel(max - p, imx->mmio_base + MX1_PWMS);
> > >  
> > > +	clk_disable_unprepare(imx->clk_ipg);
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> > > +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> > >  {
> > >  	struct imx_chip *imx = to_imx_chip(chip);
> > > +	int ret;
> > >  	u32 val;
> > >  
> > > +	ret = clk_prepare_enable(imx->clk_ipg);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	val = readl(imx->mmio_base + MX1_PWMC);
> > > +	val |= MX1_PWMC_EN;
> > > +	writel(val, imx->mmio_base + MX1_PWMC);
> > >  
> > > -	if (enable)
> > > -		val |= MX1_PWMC_EN;
> > > -	else
> > > -		val &= ~MX1_PWMC_EN;
> > > +	clk_disable_unprepare(imx->clk_per);  
> > 
> > This looks wrong. You start by enabling clk_ipg which is needed for
> > register access, but then end with disabling clk_per which is needed for
> > driving the actual PWM output. This function should probably enable
> > clk_per on entry and enable clk_ipg while accessing registers.
> 
> Oh, I didn't notice there was 2 different clocks here. This probably
> means you have to enable clk_ipg when manipulating the registers in
> ->disable().
> 
> One question, if there's a separate clk for the registers, why don't we
> leave this clock enabled, and disable it in ->suspend() or ->remove()
> instead of enabling/disabling it each time we access the registers?

Well, if we don't need this clock, then why not save the power and
disable it?
However, I'll have to ask Philipp about this clock. It was introduced in

commit 7b27c160c68152581c702b9f1fe362338d2a0cad
Author: Philipp Zabel <p.zabel@pengutronix.de>
Date:   Mon Jun 25 16:15:20 2012 +0200

And even back then the additional clock was not enabled for all register
accesses, so handling this seems broken from the start.

Sascha


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

Re: [PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Boris Brezillon]

On Tue, 25 Oct 2016 08:32:59 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Tue, Oct 25, 2016 at 08:27:37AM +0200, Boris Brezillon wrote:
> > On Tue, 25 Oct 2016 07:54:54 +0200
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >   
> > > On Sun, Oct 23, 2016 at 11:45:41PM +0200, Lukasz Majewski wrote:  
> > > > The code has been rewritten to remove "generic" calls to
> > > > imx_pwm_{enable|disable|config}.
> > > > 
> > > > Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> > > > implementation.
> > > > 
> > > > Suggested-by: Stefan Agner <stefan@agner.ch>
> > > > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > > > ---
> > > >  drivers/pwm/pwm-imx.c | 46 ++++++++++++++++++++++++++++++++++------------
> > > >  1 file changed, 34 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > index c37d223..83e43d5 100644
> > > > --- a/drivers/pwm/pwm-imx.c
> > > > +++ b/drivers/pwm/pwm-imx.c
> > > > @@ -66,8 +66,6 @@ struct imx_chip {
> > > >  static int imx_pwm_config_v1(struct pwm_chip *chip,
> > > >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> > > >  {
> > > > -	struct imx_chip *imx = to_imx_chip(chip);
> > > > -
> > > >  	/*
> > > >  	 * The PWM subsystem allows for exact frequencies. However,
> > > >  	 * I cannot connect a scope on my device to the PWM line and
> > > > @@ -85,26 +83,52 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
> > > >  	 * both the prescaler (/1 .. /128) and then by CLKSEL
> > > >  	 * (/2 .. /16).
> > > >  	 */
> > > > +	struct imx_chip *imx = to_imx_chip(chip);
> > > >  	u32 max = readl(imx->mmio_base + MX1_PWMP);
> > > >  	u32 p = max * duty_ns / period_ns;
> > > > +	int ret;
> > > > +
> > > > +	ret = clk_prepare_enable(imx->clk_ipg);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > >  	writel(max - p, imx->mmio_base + MX1_PWMS);
> > > >  
> > > > +	clk_disable_unprepare(imx->clk_ipg);
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> > > > +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> > > >  {
> > > >  	struct imx_chip *imx = to_imx_chip(chip);
> > > > +	int ret;
> > > >  	u32 val;
> > > >  
> > > > +	ret = clk_prepare_enable(imx->clk_ipg);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > >  	val = readl(imx->mmio_base + MX1_PWMC);
> > > > +	val |= MX1_PWMC_EN;
> > > > +	writel(val, imx->mmio_base + MX1_PWMC);
> > > >  
> > > > -	if (enable)
> > > > -		val |= MX1_PWMC_EN;
> > > > -	else
> > > > -		val &= ~MX1_PWMC_EN;
> > > > +	clk_disable_unprepare(imx->clk_per);    
> > > 
> > > This looks wrong. You start by enabling clk_ipg which is needed for
> > > register access, but then end with disabling clk_per which is needed for
> > > driving the actual PWM output. This function should probably enable
> > > clk_per on entry and enable clk_ipg while accessing registers.  
> > 
> > Oh, I didn't notice there was 2 different clocks here. This probably
> > means you have to enable clk_ipg when manipulating the registers in  
> > ->disable().  
> > 
> > One question, if there's a separate clk for the registers, why don't we
> > leave this clock enabled, and disable it in ->suspend() or ->remove()
> > instead of enabling/disabling it each time we access the registers?  
> 
> Well, if we don't need this clock, then why not save the power and
> disable it?

That's true, especially if enabling the clock is instantaneous (i.e. the
clock driver does not have to wait for the clk to be ready).

> However, I'll have to ask Philipp about this clock. It was introduced in
> 
> commit 7b27c160c68152581c702b9f1fe362338d2a0cad
> Author: Philipp Zabel <p.zabel@pengutronix.de>
> Date:   Mon Jun 25 16:15:20 2012 +0200
> 
> And even back then the additional clock was not enabled for all register
> accesses, so handling this seems broken from the start.
> 
> Sascha
> 
> 

Re: [PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Lukasz Majewski]

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

Hi Sascha,

> On Sun, Oct 23, 2016 at 11:45:41PM +0200, Lukasz Majewski wrote:
> > The code has been rewritten to remove "generic" calls to
> > imx_pwm_{enable|disable|config}.
> > 
> > Such approach would facilitate switch to atomic PWM (a.k.a
> > ->apply()) implementation.
> > 
> > Suggested-by: Stefan Agner <stefan@agner.ch>
> > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > ---
> >  drivers/pwm/pwm-imx.c | 46
> > ++++++++++++++++++++++++++++++++++------------ 1 file changed, 34
> > insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index c37d223..83e43d5 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -66,8 +66,6 @@ struct imx_chip {
> >  static int imx_pwm_config_v1(struct pwm_chip *chip,
> >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> >  {
> > -	struct imx_chip *imx = to_imx_chip(chip);
> > -
> >  	/*
> >  	 * The PWM subsystem allows for exact frequencies. However,
> >  	 * I cannot connect a scope on my device to the PWM line
> > and @@ -85,26 +83,52 @@ static int imx_pwm_config_v1(struct
> > pwm_chip *chip,
> >  	 * both the prescaler (/1 .. /128) and then by CLKSEL
> >  	 * (/2 .. /16).
> >  	 */
> > +	struct imx_chip *imx = to_imx_chip(chip);
> >  	u32 max = readl(imx->mmio_base + MX1_PWMP);
> >  	u32 p = max * duty_ns / period_ns;
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(imx->clk_ipg);
> > +	if (ret)
> > +		return ret;
> > +
> >  	writel(max - p, imx->mmio_base + MX1_PWMS);
> >  
> > +	clk_disable_unprepare(imx->clk_ipg);
> > +
> >  	return 0;
> >  }
> >  
> > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool
> > enable) +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct
> > pwm_device *pwm) {
> >  	struct imx_chip *imx = to_imx_chip(chip);
> > +	int ret;
> >  	u32 val;
> >  
> > +	ret = clk_prepare_enable(imx->clk_ipg);
> > +	if (ret)
> > +		return ret;
> > +
> >  	val = readl(imx->mmio_base + MX1_PWMC);
> > +	val |= MX1_PWMC_EN;
> > +	writel(val, imx->mmio_base + MX1_PWMC);
> >  
> > -	if (enable)
> > -		val |= MX1_PWMC_EN;
> > -	else
> > -		val &= ~MX1_PWMC_EN;
> > +	clk_disable_unprepare(imx->clk_per);
> 
> This looks wrong. You start by enabling clk_ipg which is needed for
> register access, but then end with disabling clk_per which is needed
> for driving the actual PWM output. This function should probably
> enable clk_per on entry and enable clk_ipg while accessing registers.

This was my omission. You are right, there are two clocks. One is
feeding PWM block and another allows access to registers.

Similar scheme is used with PWMv2. I will correct this when preparing
version 2 of this patch set.

Thanks for feedback,

Łukasz Majewski

> 
> Sascha
> 


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

[PATCH 2/6] pwm: imx: Move PWMv2 software reset code to a separate function

[Lukasz Majewski]

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

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

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

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 83e43d5..fac5c93 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -131,6 +131,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)
 {
@@ -140,7 +159,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;
 
 	/*
@@ -162,17 +181,8 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 			if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
 				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");
-	}
+	} else
+		imx_pwm_sw_reset(chip);
 
 	c = clk_get_rate(imx->clk_per);
 	c = c * period_ns;
-- 
2.1.4

Re: [PATCH 2/6] pwm: imx: Move PWMv2 software reset code to a separate function

[Boris Brezillon]

On Sun, 23 Oct 2016 23:45:42 +0200
Lukasz Majewski <l.majewski@majess.pl> wrote:

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

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

Just a nit below ;).

BTW, can't you just merge path 2 and 3?

> ---
>  drivers/pwm/pwm-imx.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 83e43d5..fac5c93 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -131,6 +131,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)
>  {
> @@ -140,7 +159,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;
>  
>  	/*
> @@ -162,17 +181,8 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>  			if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
>  				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");
> -	}
> +	} else
> +		imx_pwm_sw_reset(chip);

	} else {
		imx_pwm_sw_reset(chip);
	}

>  
>  	c = clk_get_rate(imx->clk_per);
>  	c = c * period_ns;

Re: [PATCH 2/6] pwm: imx: Move PWMv2 software reset code to a separate function

[Lukasz Majewski]

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

Hi Boris,

> On Sun, 23 Oct 2016 23:45:42 +0200
> Lukasz Majewski <l.majewski@majess.pl> wrote:
> 
> > 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>
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
> Just a nit below ;).
> 
> BTW, can't you just merge path 2 and 3?

I do prefer to have many small patches touching and changing just one
thing.

> 
> > ---
> >  drivers/pwm/pwm-imx.c | 34 ++++++++++++++++++++++------------
> >  1 file changed, 22 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index 83e43d5..fac5c93 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -131,6 +131,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)
> >  {
> > @@ -140,7 +159,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;
> >  
> >  	/*
> > @@ -162,17 +181,8 @@ static int imx_pwm_config_v2(struct pwm_chip
> > *chip, if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
> >  				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");
> > -	}
> > +	} else
> > +		imx_pwm_sw_reset(chip);
> 
> 	} else {
> 		imx_pwm_sw_reset(chip);
> 	}

OK.

> 
> >  
> >  	c = clk_get_rate(imx->clk_per);
> >  	c = c * period_ns;
> 

Best regards,

Łukasz Majewski

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

[PATCH 3/6] pwm: imx: Move PWMv2 wait for fifo slot code to a separate function

[Lukasz Majewski]

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

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

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

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index fac5c93..f3577c5 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -149,18 +149,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.
@@ -169,19 +187,9 @@ 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);
-- 
2.1.4

Re: [PATCH 3/6] pwm: imx: Move PWMv2 wait for fifo slot code to a separate function

[Boris Brezillon]

On Sun, 23 Oct 2016 23:45:43 +0200
Lukasz Majewski <l.majewski@majess.pl> wrote:

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

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

> ---
>  drivers/pwm/pwm-imx.c | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index fac5c93..f3577c5 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -149,18 +149,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.
> @@ -169,19 +187,9 @@ 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);

[PATCH 4/6] pwm: imx: Provide atomic PWM support for IMXv2 PWM

[Lukasz Majewski]

This commit provides apply() callback implementation for IMX'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>
---
 drivers/pwm/pwm-imx.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

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

Re: [PATCH 4/6] pwm: imx: Provide atomic PWM support for IMXv2 PWM

[Boris Brezillon]

On Sun, 23 Oct 2016 23:45:44 +0200
Lukasz Majewski <l.majewski@majess.pl> wrote:

> This commit provides apply() callback implementation for IMX'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>

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

[PATCH 5/6] pwm: imx: Remove redundant IMX PWMv2 code

[Lukasz Majewski]

The code providing functionality surpassed by the PWM atomic 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>
---
 drivers/pwm/pwm-imx.c | 155 --------------------------------------------------
 1 file changed, 155 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index df25379..02d3dfd 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -55,10 +55,6 @@ struct imx_chip {
 	void __iomem	*mmio_base;
 
 	struct pwm_chip	chip;
-
-	int (*config)(struct pwm_chip *chip,
-		struct pwm_device *pwm, int duty_ns, int period_ns);
-	void (*set_enable)(struct pwm_chip *chip, bool enable);
 };
 
 #define to_imx_chip(chip)	container_of(chip, struct imx_chip, chip)
@@ -249,145 +245,6 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
-static int imx_pwm_config_v2(struct pwm_chip *chip,
-		struct pwm_device *pwm, int duty_ns, int period_ns)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	unsigned long long c;
-	unsigned long period_cycles, duty_cycles, prescale;
-	bool enable = pwm_is_enabled(pwm);
-	u32 cr;
-
-	/*
-	 * i.MX PWMv2 has a 4-word sample FIFO.
-	 * In order to avoid FIFO overflow issue, we do software reset
-	 * to clear all sample FIFO if the controller is disabled or
-	 * wait for a full PWM cycle to get a relinquished FIFO slot
-	 * when the controller is enabled and the FIFO is fully loaded.
-	 */
-	if (enable)
-		imx_pwm_wait_fifo_slot(chip, pwm);
-	else
-		imx_pwm_sw_reset(chip);
-
-	c = clk_get_rate(imx->clk_per);
-	c = c * period_ns;
-	do_div(c, 1000000000);
-	period_cycles = c;
-
-	prescale = period_cycles / 0x10000 + 1;
-
-	period_cycles /= prescale;
-	c = (unsigned long long)period_cycles * duty_ns;
-	do_div(c, period_ns);
-	duty_cycles = c;
-
-	/*
-	 * according to imx pwm RM, the real period value should be
-	 * PERIOD value in PWMPR plus 2.
-	 */
-	if (period_cycles > 2)
-		period_cycles -= 2;
-	else
-		period_cycles = 0;
-
-	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
-	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
-
-	cr = MX3_PWMCR_PRESCALER(prescale) |
-		MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
-		MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
-
-	if (enable)
-		cr |= MX3_PWMCR_EN;
-
-	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
-		cr |= MX3_PWMCR_POUTC;
-
-	writel(cr, imx->mmio_base + MX3_PWMCR);
-
-	return 0;
-}
-
-static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	u32 val;
-
-	val = readl(imx->mmio_base + MX3_PWMCR);
-
-	if (enable)
-		val |= MX3_PWMCR_EN;
-	else
-		val &= ~MX3_PWMCR_EN;
-
-	writel(val, imx->mmio_base + MX3_PWMCR);
-}
-
-static int imx_pwm_config(struct pwm_chip *chip,
-		struct pwm_device *pwm, int duty_ns, int period_ns)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	int ret;
-
-	ret = clk_prepare_enable(imx->clk_ipg);
-	if (ret)
-		return ret;
-
-	ret = imx->config(chip, pwm, duty_ns, period_ns);
-
-	clk_disable_unprepare(imx->clk_ipg);
-
-	return ret;
-}
-
-static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	int ret;
-
-	ret = clk_prepare_enable(imx->clk_per);
-	if (ret)
-		return ret;
-
-	imx->set_enable(chip, true);
-
-	return 0;
-}
-
-static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-
-	imx->set_enable(chip, false);
-
-	clk_disable_unprepare(imx->clk_per);
-}
-
-static int imx_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
-				enum pwm_polarity polarity)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	u32 val;
-
-	if (polarity == pwm->args.polarity)
-		return 0;
-
-	val = readl(imx->mmio_base + MX3_PWMCR);
-
-	if (polarity == PWM_POLARITY_INVERSED)
-		val |= MX3_PWMCR_POUTC;
-	else
-		val &= ~MX3_PWMCR_POUTC;
-
-	writel(val, imx->mmio_base + MX3_PWMCR);
-
-	dev_dbg(imx->chip.dev, "%s: polarity set to %s\n", __func__,
-		polarity == PWM_POLARITY_INVERSED ? "inverted" : "normal");
-
-	return 0;
-}
-
 static struct pwm_ops imx_pwm_ops_v1 = {
 	.enable = imx_pwm_enable_v1,
 	.disable = imx_pwm_disable_v1,
@@ -396,18 +253,11 @@ static struct pwm_ops imx_pwm_ops_v1 = {
 };
 
 static struct pwm_ops imx_pwm_ops_v2 = {
-	.enable = imx_pwm_enable,
-	.disable = imx_pwm_disable,
-	.set_polarity = imx_pwm_set_polarity,
-	.config = imx_pwm_config,
 	.apply = imx_pwm_apply_v2,
 	.owner = THIS_MODULE,
 };
 
 struct imx_pwm_data {
-	int (*config)(struct pwm_chip *chip,
-		struct pwm_device *pwm, int duty_ns, int period_ns);
-	void (*set_enable)(struct pwm_chip *chip, bool enable);
 	struct pwm_ops *pwm_ops;
 };
 
@@ -416,8 +266,6 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
 };
 
 static struct imx_pwm_data imx_pwm_data_v2 = {
-	.config = imx_pwm_config_v2,
-	.set_enable = imx_pwm_set_enable_v2,
 	.pwm_ops = &imx_pwm_ops_v2,
 };
 
@@ -476,9 +324,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

Re: [PATCH 5/6] pwm: imx: Remove redundant IMX PWMv2 code

[Boris Brezillon]

On Sun, 23 Oct 2016 23:45:45 +0200
Lukasz Majewski <l.majewski@majess.pl> wrote:

> The code providing functionality surpassed by the PWM atomic 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>

> ---
>  drivers/pwm/pwm-imx.c | 155 --------------------------------------------------
>  1 file changed, 155 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index df25379..02d3dfd 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -55,10 +55,6 @@ struct imx_chip {
>  	void __iomem	*mmio_base;
>  
>  	struct pwm_chip	chip;
> -
> -	int (*config)(struct pwm_chip *chip,
> -		struct pwm_device *pwm, int duty_ns, int period_ns);
> -	void (*set_enable)(struct pwm_chip *chip, bool enable);
>  };
>  
>  #define to_imx_chip(chip)	container_of(chip, struct imx_chip, chip)
> @@ -249,145 +245,6 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return 0;
>  }
>  
> -static int imx_pwm_config_v2(struct pwm_chip *chip,
> -		struct pwm_device *pwm, int duty_ns, int period_ns)
> -{
> -	struct imx_chip *imx = to_imx_chip(chip);
> -	unsigned long long c;
> -	unsigned long period_cycles, duty_cycles, prescale;
> -	bool enable = pwm_is_enabled(pwm);
> -	u32 cr;
> -
> -	/*
> -	 * i.MX PWMv2 has a 4-word sample FIFO.
> -	 * In order to avoid FIFO overflow issue, we do software reset
> -	 * to clear all sample FIFO if the controller is disabled or
> -	 * wait for a full PWM cycle to get a relinquished FIFO slot
> -	 * when the controller is enabled and the FIFO is fully loaded.
> -	 */
> -	if (enable)
> -		imx_pwm_wait_fifo_slot(chip, pwm);
> -	else
> -		imx_pwm_sw_reset(chip);
> -
> -	c = clk_get_rate(imx->clk_per);
> -	c = c * period_ns;
> -	do_div(c, 1000000000);
> -	period_cycles = c;
> -
> -	prescale = period_cycles / 0x10000 + 1;
> -
> -	period_cycles /= prescale;
> -	c = (unsigned long long)period_cycles * duty_ns;
> -	do_div(c, period_ns);
> -	duty_cycles = c;
> -
> -	/*
> -	 * according to imx pwm RM, the real period value should be
> -	 * PERIOD value in PWMPR plus 2.
> -	 */
> -	if (period_cycles > 2)
> -		period_cycles -= 2;
> -	else
> -		period_cycles = 0;
> -
> -	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> -	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> -
> -	cr = MX3_PWMCR_PRESCALER(prescale) |
> -		MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> -		MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
> -
> -	if (enable)
> -		cr |= MX3_PWMCR_EN;
> -
> -	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> -		cr |= MX3_PWMCR_POUTC;
> -
> -	writel(cr, imx->mmio_base + MX3_PWMCR);
> -
> -	return 0;
> -}
> -
> -static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
> -{
> -	struct imx_chip *imx = to_imx_chip(chip);
> -	u32 val;
> -
> -	val = readl(imx->mmio_base + MX3_PWMCR);
> -
> -	if (enable)
> -		val |= MX3_PWMCR_EN;
> -	else
> -		val &= ~MX3_PWMCR_EN;
> -
> -	writel(val, imx->mmio_base + MX3_PWMCR);
> -}
> -
> -static int imx_pwm_config(struct pwm_chip *chip,
> -		struct pwm_device *pwm, int duty_ns, int period_ns)
> -{
> -	struct imx_chip *imx = to_imx_chip(chip);
> -	int ret;
> -
> -	ret = clk_prepare_enable(imx->clk_ipg);
> -	if (ret)
> -		return ret;
> -
> -	ret = imx->config(chip, pwm, duty_ns, period_ns);
> -
> -	clk_disable_unprepare(imx->clk_ipg);
> -
> -	return ret;
> -}
> -
> -static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> -	struct imx_chip *imx = to_imx_chip(chip);
> -	int ret;
> -
> -	ret = clk_prepare_enable(imx->clk_per);
> -	if (ret)
> -		return ret;
> -
> -	imx->set_enable(chip, true);
> -
> -	return 0;
> -}
> -
> -static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> -	struct imx_chip *imx = to_imx_chip(chip);
> -
> -	imx->set_enable(chip, false);
> -
> -	clk_disable_unprepare(imx->clk_per);
> -}
> -
> -static int imx_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> -				enum pwm_polarity polarity)
> -{
> -	struct imx_chip *imx = to_imx_chip(chip);
> -	u32 val;
> -
> -	if (polarity == pwm->args.polarity)
> -		return 0;
> -
> -	val = readl(imx->mmio_base + MX3_PWMCR);
> -
> -	if (polarity == PWM_POLARITY_INVERSED)
> -		val |= MX3_PWMCR_POUTC;
> -	else
> -		val &= ~MX3_PWMCR_POUTC;
> -
> -	writel(val, imx->mmio_base + MX3_PWMCR);
> -
> -	dev_dbg(imx->chip.dev, "%s: polarity set to %s\n", __func__,
> -		polarity == PWM_POLARITY_INVERSED ? "inverted" : "normal");
> -
> -	return 0;
> -}
> -
>  static struct pwm_ops imx_pwm_ops_v1 = {
>  	.enable = imx_pwm_enable_v1,
>  	.disable = imx_pwm_disable_v1,
> @@ -396,18 +253,11 @@ static struct pwm_ops imx_pwm_ops_v1 = {
>  };
>  
>  static struct pwm_ops imx_pwm_ops_v2 = {
> -	.enable = imx_pwm_enable,
> -	.disable = imx_pwm_disable,
> -	.set_polarity = imx_pwm_set_polarity,
> -	.config = imx_pwm_config,
>  	.apply = imx_pwm_apply_v2,
>  	.owner = THIS_MODULE,
>  };
>  
>  struct imx_pwm_data {
> -	int (*config)(struct pwm_chip *chip,
> -		struct pwm_device *pwm, int duty_ns, int period_ns);
> -	void (*set_enable)(struct pwm_chip *chip, bool enable);
>  	struct pwm_ops *pwm_ops;
>  };
>  
> @@ -416,8 +266,6 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
>  };
>  
>  static struct imx_pwm_data imx_pwm_data_v2 = {
> -	.config = imx_pwm_config_v2,
> -	.set_enable = imx_pwm_set_enable_v2,
>  	.pwm_ops = &imx_pwm_ops_v2,
>  };
>  
> @@ -476,9 +324,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;

[PATCH 6/6] pwm: imx: Introduce "polarity_supported" flag to PWMv2 driver

[Lukasz Majewski]

The need for set_polarity() function has been removed by implementing
PWM atomic support (apply() callback).

To indicate that the PWMv2 supports polarity inversion, new flag -
"polarity_supported" has been introduced.

Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
 drivers/pwm/pwm-imx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 02d3dfd..be3034d 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -258,6 +258,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
 };
 
 struct imx_pwm_data {
+	bool polarity_supported;
 	struct pwm_ops *pwm_ops;
 };
 
@@ -266,6 +267,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
 };
 
 static struct imx_pwm_data imx_pwm_data_v2 = {
+	.polarity_supported = true,
 	.pwm_ops = &imx_pwm_ops_v2,
 };
 
@@ -313,7 +315,7 @@ static int imx_pwm_probe(struct platform_device *pdev)
 	imx->chip.base = -1;
 	imx->chip.npwm = 1;
 	imx->chip.can_sleep = true;
-	if (data->pwm_ops->set_polarity) {
+	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;
-- 
2.1.4

Re: [PATCH 6/6] pwm: imx: Introduce "polarity_supported" flag to PWMv2 driver

[Boris Brezillon]

On Sun, 23 Oct 2016 23:45:46 +0200
Lukasz Majewski <l.majewski@majess.pl> wrote:

> The need for set_polarity() function has been removed by implementing
> PWM atomic support (apply() callback).
> 
> To indicate that the PWMv2 supports polarity inversion, new flag -
> "polarity_supported" has been introduced.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
>  drivers/pwm/pwm-imx.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 02d3dfd..be3034d 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -258,6 +258,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
>  };
>  
>  struct imx_pwm_data {
> +	bool polarity_supported;
>  	struct pwm_ops *pwm_ops;
>  };
>  
> @@ -266,6 +267,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
>  };
>  
>  static struct imx_pwm_data imx_pwm_data_v2 = {
> +	.polarity_supported = true,
>  	.pwm_ops = &imx_pwm_ops_v2,
>  };
>  
> @@ -313,7 +315,7 @@ static int imx_pwm_probe(struct platform_device *pdev)
>  	imx->chip.base = -1;
>  	imx->chip.npwm = 1;
>  	imx->chip.can_sleep = true;
> -	if (data->pwm_ops->set_polarity) {
> +	if (data->polarity_supported) {

You're still breaking backward compatibility with DTs defining
#pwm-cells = 2.

Please test the #pwm-cells value before deciding which of_xlate should
be used.

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

Re: [PATCH 6/6] pwm: imx: Introduce "polarity_supported" flag to PWMv2 driver

[Boris Brezillon]

On Mon, 24 Oct 2016 17:28:52 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Sun, 23 Oct 2016 23:45:46 +0200
> Lukasz Majewski <l.majewski@majess.pl> wrote:
> 
> > The need for set_polarity() function has been removed by implementing
> > PWM atomic support (apply() callback).
> > 
> > To indicate that the PWMv2 supports polarity inversion, new flag -
> > "polarity_supported" has been introduced.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > ---
> >  drivers/pwm/pwm-imx.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index 02d3dfd..be3034d 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -258,6 +258,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
> >  };
> >  
> >  struct imx_pwm_data {
> > +	bool polarity_supported;
> >  	struct pwm_ops *pwm_ops;
> >  };
> >  
> > @@ -266,6 +267,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
> >  };
> >  
> >  static struct imx_pwm_data imx_pwm_data_v2 = {
> > +	.polarity_supported = true,
> >  	.pwm_ops = &imx_pwm_ops_v2,
> >  };
> >  
> > @@ -313,7 +315,7 @@ static int imx_pwm_probe(struct platform_device *pdev)
> >  	imx->chip.base = -1;
> >  	imx->chip.npwm = 1;
> >  	imx->chip.can_sleep = true;
> > -	if (data->pwm_ops->set_polarity) {
> > +	if (data->polarity_supported) {  
> 
> You're still breaking backward compatibility with DTs defining
> #pwm-cells = 2.
> 
> Please test the #pwm-cells value before deciding which of_xlate should
> be used.

Nevermind, I didn't look at [1] and [2].
But still, your series is not bisectable: this change should be part of
patch 5 where you remove the ->set_polarity implementation. Otherwise,
this means you don't support polarity setting between patch 5 and 6.

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

[1]http://patchwork.ozlabs.org/patch/679706/
[2]http://patchwork.ozlabs.org/patch/679707/

Re: [PATCH 6/6] pwm: imx: Introduce "polarity_supported" flag to PWMv2 driver

[Lukasz Majewski]

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

Hi Boris,

> On Mon, 24 Oct 2016 17:28:52 +0200
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > On Sun, 23 Oct 2016 23:45:46 +0200
> > Lukasz Majewski <l.majewski@majess.pl> wrote:
> > 
> > > The need for set_polarity() function has been removed by
> > > implementing PWM atomic support (apply() callback).
> > > 
> > > To indicate that the PWMv2 supports polarity inversion, new flag -
> > > "polarity_supported" has been introduced.
> > > 
> > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > > ---
> > >  drivers/pwm/pwm-imx.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > index 02d3dfd..be3034d 100644
> > > --- a/drivers/pwm/pwm-imx.c
> > > +++ b/drivers/pwm/pwm-imx.c
> > > @@ -258,6 +258,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
> > >  };
> > >  
> > >  struct imx_pwm_data {
> > > +	bool polarity_supported;
> > >  	struct pwm_ops *pwm_ops;
> > >  };
> > >  
> > > @@ -266,6 +267,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
> > >  };
> > >  
> > >  static struct imx_pwm_data imx_pwm_data_v2 = {
> > > +	.polarity_supported = true,
> > >  	.pwm_ops = &imx_pwm_ops_v2,
> > >  };
> > >  
> > > @@ -313,7 +315,7 @@ static int imx_pwm_probe(struct
> > > platform_device *pdev) imx->chip.base = -1;
> > >  	imx->chip.npwm = 1;
> > >  	imx->chip.can_sleep = true;
> > > -	if (data->pwm_ops->set_polarity) {
> > > +	if (data->polarity_supported) {  
> > 
> > You're still breaking backward compatibility with DTs defining
> > #pwm-cells = 2.
> > 
> > Please test the #pwm-cells value before deciding which of_xlate
> > should be used.
> 
> Nevermind, I didn't look at [1] and [2].

Yes, some patches are required to make this code work. Especially, I
wanted to explicitly reuse and credit work already done by
Bhuvanchandra.

> But still, your series is not bisectable: this change should be part
> of patch 5 where you remove the ->set_polarity implementation.
> Otherwise, this means you don't support polarity setting between
> patch 5 and 6.

Frankly speaking, I did it on purpose, to have operations in commits
logically separated.

I personally, do detest commits which blur the picture and are not
corresponding to one single logical change - for example remove some
large chunk of code and also add some tiny, new flag.

For me it is not a problem to have polarity disabled between patches 5
and 6, since at the end of the day we have it enabled.

Thanks for your support and review,

Best regards,

Łukasz Majewski

> 
> > 
> > >  		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;  
> > 
> 
> [1]http://patchwork.ozlabs.org/patch/679706/
> [2]http://patchwork.ozlabs.org/patch/679707/


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

Re: [PATCH 6/6] pwm: imx: Introduce "polarity_supported" flag to PWMv2 driver

[Boris Brezillon]

Hi Lukasz,

On Mon, 24 Oct 2016 23:14:58 +0200
Lukasz Majewski <l.majewski@majess.pl> wrote:

> Hi Boris,
> 
> > On Mon, 24 Oct 2016 17:28:52 +0200
> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >   
> > > On Sun, 23 Oct 2016 23:45:46 +0200
> > > Lukasz Majewski <l.majewski@majess.pl> wrote:
> > >   
> > > > The need for set_polarity() function has been removed by
> > > > implementing PWM atomic support (apply() callback).
> > > > 
> > > > To indicate that the PWMv2 supports polarity inversion, new flag -
> > > > "polarity_supported" has been introduced.
> > > > 
> > > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > > > ---
> > > >  drivers/pwm/pwm-imx.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > index 02d3dfd..be3034d 100644
> > > > --- a/drivers/pwm/pwm-imx.c
> > > > +++ b/drivers/pwm/pwm-imx.c
> > > > @@ -258,6 +258,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
> > > >  };
> > > >  
> > > >  struct imx_pwm_data {
> > > > +	bool polarity_supported;
> > > >  	struct pwm_ops *pwm_ops;
> > > >  };
> > > >  
> > > > @@ -266,6 +267,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
> > > >  };
> > > >  
> > > >  static struct imx_pwm_data imx_pwm_data_v2 = {
> > > > +	.polarity_supported = true,
> > > >  	.pwm_ops = &imx_pwm_ops_v2,
> > > >  };
> > > >  
> > > > @@ -313,7 +315,7 @@ static int imx_pwm_probe(struct
> > > > platform_device *pdev) imx->chip.base = -1;
> > > >  	imx->chip.npwm = 1;
> > > >  	imx->chip.can_sleep = true;
> > > > -	if (data->pwm_ops->set_polarity) {
> > > > +	if (data->polarity_supported) {    
> > > 
> > > You're still breaking backward compatibility with DTs defining
> > > #pwm-cells = 2.
> > > 
> > > Please test the #pwm-cells value before deciding which of_xlate
> > > should be used.  
> > 
> > Nevermind, I didn't look at [1] and [2].  
> 
> Yes, some patches are required to make this code work. Especially, I
> wanted to explicitly reuse and credit work already done by
> Bhuvanchandra.
> 
> > But still, your series is not bisectable: this change should be part
> > of patch 5 where you remove the ->set_polarity implementation.
> > Otherwise, this means you don't support polarity setting between
> > patch 5 and 6.  
> 
> Frankly speaking, I did it on purpose, to have operations in commits
> logically separated.
> 
> I personally, do detest commits which blur the picture and are not
> corresponding to one single logical change - for example remove some
> large chunk of code and also add some tiny, new flag.
> 
> For me it is not a problem to have polarity disabled between patches 5
> and 6, since at the end of the day we have it enabled.

It's really simple to make this series bisectable, all you have to do
is move patch 6 before patch 5. This being said, I really think you
should follow Stefan's recommendation: base your changes on mainline
and switch to the atomic hook before supporting polarity setting.

Regards,

Boris

Re: [PATCH 6/6] pwm: imx: Introduce "polarity_supported" flag to PWMv2 driver

[Lukasz Majewski]

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

Hi Boris,

> Hi Lukasz,
> 
> On Mon, 24 Oct 2016 23:14:58 +0200
> Lukasz Majewski <l.majewski@majess.pl> wrote:
> 
> > Hi Boris,
> > 
> > > On Mon, 24 Oct 2016 17:28:52 +0200
> > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> > >   
> > > > On Sun, 23 Oct 2016 23:45:46 +0200
> > > > Lukasz Majewski <l.majewski@majess.pl> wrote:
> > > >   
> > > > > The need for set_polarity() function has been removed by
> > > > > implementing PWM atomic support (apply() callback).
> > > > > 
> > > > > To indicate that the PWMv2 supports polarity inversion, new
> > > > > flag - "polarity_supported" has been introduced.
> > > > > 
> > > > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > > > > ---
> > > > >  drivers/pwm/pwm-imx.c | 4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > > index 02d3dfd..be3034d 100644
> > > > > --- a/drivers/pwm/pwm-imx.c
> > > > > +++ b/drivers/pwm/pwm-imx.c
> > > > > @@ -258,6 +258,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
> > > > >  };
> > > > >  
> > > > >  struct imx_pwm_data {
> > > > > +	bool polarity_supported;
> > > > >  	struct pwm_ops *pwm_ops;
> > > > >  };
> > > > >  
> > > > > @@ -266,6 +267,7 @@ static struct imx_pwm_data
> > > > > imx_pwm_data_v1 = { };
> > > > >  
> > > > >  static struct imx_pwm_data imx_pwm_data_v2 = {
> > > > > +	.polarity_supported = true,
> > > > >  	.pwm_ops = &imx_pwm_ops_v2,
> > > > >  };
> > > > >  
> > > > > @@ -313,7 +315,7 @@ static int imx_pwm_probe(struct
> > > > > platform_device *pdev) imx->chip.base = -1;
> > > > >  	imx->chip.npwm = 1;
> > > > >  	imx->chip.can_sleep = true;
> > > > > -	if (data->pwm_ops->set_polarity) {
> > > > > +	if (data->polarity_supported) {    
> > > > 
> > > > You're still breaking backward compatibility with DTs defining
> > > > #pwm-cells = 2.
> > > > 
> > > > Please test the #pwm-cells value before deciding which of_xlate
> > > > should be used.  
> > > 
> > > Nevermind, I didn't look at [1] and [2].  
> > 
> > Yes, some patches are required to make this code work. Especially, I
> > wanted to explicitly reuse and credit work already done by
> > Bhuvanchandra.
> > 
> > > But still, your series is not bisectable: this change should be
> > > part of patch 5 where you remove the ->set_polarity
> > > implementation. Otherwise, this means you don't support polarity
> > > setting between patch 5 and 6.  
> > 
> > Frankly speaking, I did it on purpose, to have operations in commits
> > logically separated.
> > 
> > I personally, do detest commits which blur the picture and are not
> > corresponding to one single logical change - for example remove some
> > large chunk of code and also add some tiny, new flag.
> > 
> > For me it is not a problem to have polarity disabled between
> > patches 5 and 6, since at the end of the day we have it enabled.
> 
> It's really simple to make this series bisectable, all you have to do
> is move patch 6 before patch 5.

Hmm... You are right, I do wonder why I didn't get this idea from the
very beginning.

> This being said, I really think you
> should follow Stefan's recommendation: base your changes on mainline
> and switch to the atomic hook before supporting polarity setting.

I will do my best :-)

Best regards,

Łukasz Majewski

> 
> Regards,
> 
> Boris


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

Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver

[Boris Brezillon]

On Sun, 23 Oct 2016 23:45:40 +0200
Lukasz Majewski <l.majewski@majess.pl> wrote:

> 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:
> - 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) in a single patch
>   for better readability.
> 
> - Remove redundant PWM code (disable, enable, config callbacks)
> 
> - Clean up the driver infrastructure
> 
> - Provide "polarity_supported" flag to indicate support for polarity 
>   inversion
> 
> This work should be applied on top of following commits:
> 
> http://patchwork.ozlabs.org/patch/679706/
> http://patchwork.ozlabs.org/patch/679707/
> http://patchwork.ozlabs.org/patch/679680/

I'm not sure I follow the logic here. Has patch [1] already been
applied? If that's not the case, then you should just drop it and put
your changes on top of mainline.

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

Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver

[Lukasz Majewski]

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

Hi Boris,

> On Sun, 23 Oct 2016 23:45:40 +0200
> Lukasz Majewski <l.majewski@majess.pl> wrote:
> 
> > 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:
> > - 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) in a
> > single patch for better readability.
> > 
> > - Remove redundant PWM code (disable, enable, config callbacks)
> > 
> > - Clean up the driver infrastructure
> > 
> > - Provide "polarity_supported" flag to indicate support for
> > polarity inversion
> > 
> > This work should be applied on top of following commits:
> > 
> > http://patchwork.ozlabs.org/patch/679706/ 
[2]

> > http://patchwork.ozlabs.org/patch/679707/
[3]

> > http://patchwork.ozlabs.org/patch/679680/
> 
> I'm not sure I follow the logic here. Has patch [1] already been
> applied? If that's not the case, then you should just drop it and put
> your changes on top of mainline.
> 
> [1]http://patchwork.ozlabs.org/patch/679680/

Patches [2] and [3] have been developed initially by Lothar and
subsequently picked up by Bhuvanchandra. There is no issue with them.

The patch [1] is a bit more tricky. The work has been done by
Bhuvanchandra, which adds DTS and core support for polarity inversion.

This code works and utilizes the "old" PWM API with enable, disable and
config. However, Stefan had some comments about the placement for the
polarity setting (in the .config_v2()) and proposed switch to atomic
API.

To make things easier and cleaner, I decided to put my atomic API
rework on top of those patches. In this way I can credit the previous
work and avoid rewriting DTS polarity inversion code already developed
and validated by Bhuvanchandra.



Best regards,
Łukasz Majewski





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

Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver

[Stefan Agner]

Hi Lukasz,

Thanks for your work, great to see this coming along! :-)

On 2016-10-24 23:26, Lukasz Majewski wrote:
> Hi Boris,
> 
>> On Sun, 23 Oct 2016 23:45:40 +0200
>> Lukasz Majewski <l.majewski@majess.pl> wrote:
>>
>> > 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:
>> > - 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) in a
>> > single patch for better readability.
>> >
>> > - Remove redundant PWM code (disable, enable, config callbacks)
>> >
>> > - Clean up the driver infrastructure
>> >
>> > - Provide "polarity_supported" flag to indicate support for
>> > polarity inversion
>> >
>> > This work should be applied on top of following commits:
>> >
>> > http://patchwork.ozlabs.org/patch/679706/
> [2]
> 
>> > http://patchwork.ozlabs.org/patch/679707/
> [3]
> 
>> > http://patchwork.ozlabs.org/patch/679680/
>>
>> I'm not sure I follow the logic here. Has patch [1] already been
>> applied? If that's not the case, then you should just drop it and put
>> your changes on top of mainline.
>>
>> [1]http://patchwork.ozlabs.org/patch/679680/
> 
> Patches [2] and [3] have been developed initially by Lothar and
> subsequently picked up by Bhuvanchandra. There is no issue with them.

As such none of this will get merged since all patchset have known
flaws...

Generally, it is ok to refer to other patchset being a prerequisite, but
that only makes sense if those patch set are still actively worked on
(by somebody other than you).

In this case I really recommend to create a new, complete patchset.

> 
> The patch [1] is a bit more tricky. The work has been done by
> Bhuvanchandra, which adds DTS and core support for polarity inversion.
> 
> This code works and utilizes the "old" PWM API with enable, disable and
> config. However, Stefan had some comments about the placement for the
> polarity setting (in the .config_v2()) and proposed switch to atomic
> API.

Part of the reason I advocated for the atomic API is to make adding the
polarity functionality easier. It does not archive this goal if we add
the "flawed" code first and then transition to the atomic API.

> 
> To make things easier and cleaner, I decided to put my atomic API
> rework on top of those patches. In this way I can credit the previous
> work and avoid rewriting DTS polarity inversion code already developed
> and validated by Bhuvanchandra.

When you apply the patches using git apply, the authorship and signoffs
will stay. There is no problem in including other peoples work into your
patchset, credit will still be given. If you have to change another
persons patch, you typically also add your signoff to show that you
worked on it too.

Here is how I would do it:

1. Start a new branch from mainline (or even -next).
2. Implement the transition to the new atomic API and test it as such
alone (this way we have no polarity support influence yet, just clean
transition to a new API)
3. Cherry pick the PWM core changes for the optional 2/3 args driver
support (they should apply cleanly)
4. Cherry pick (they likely will fail to merge) or reimplement the PWM
polarity driver changes on top of atomic API
5. Cherry pick device tree changes

With this approach we'll end up with a nice history where we should end
up with a fully functional PWM system between every patch.

Btw, past perfect tense is not really usual in commit messages.
SubmittingPatches chapter 2 has some tips on writing good commit
messages:
https://www.kernel.org/doc/Documentation/SubmittingPatches

--
Stefan

Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver

[Lukasz Majewski]

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

Hi Stefan,

> Hi Lukasz,
> 
> Thanks for your work, great to see this coming along! :-)
> 
> On 2016-10-24 23:26, Lukasz Majewski wrote:
> > Hi Boris,
> > 
> >> On Sun, 23 Oct 2016 23:45:40 +0200
> >> Lukasz Majewski <l.majewski@majess.pl> wrote:
> >>
> >> > 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:
> >> > - 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) in a
> >> > single patch for better readability.
> >> >
> >> > - Remove redundant PWM code (disable, enable, config callbacks)
> >> >
> >> > - Clean up the driver infrastructure
> >> >
> >> > - Provide "polarity_supported" flag to indicate support for
> >> > polarity inversion
> >> >
> >> > This work should be applied on top of following commits:
> >> >
> >> > http://patchwork.ozlabs.org/patch/679706/
> > [2]
> > 
> >> > http://patchwork.ozlabs.org/patch/679707/
> > [3]
> > 
> >> > http://patchwork.ozlabs.org/patch/679680/
> >>
> >> I'm not sure I follow the logic here. Has patch [1] already been
> >> applied? If that's not the case, then you should just drop it and
> >> put your changes on top of mainline.
> >>
> >> [1]http://patchwork.ozlabs.org/patch/679680/
> > 
> > Patches [2] and [3] have been developed initially by Lothar and
> > subsequently picked up by Bhuvanchandra. There is no issue with
> > them.
> 
> As such none of this will get merged since all patchset have known
> flaws...
> 
> Generally, it is ok to refer to other patchset being a prerequisite,
> but that only makes sense if those patch set are still actively
> worked on (by somebody other than you).
> 
> In this case I really recommend to create a new, complete patchset.
> 
> > 
> > The patch [1] is a bit more tricky. The work has been done by
> > Bhuvanchandra, which adds DTS and core support for polarity
> > inversion.
> > 
> > This code works and utilizes the "old" PWM API with enable, disable
> > and config. However, Stefan had some comments about the placement
> > for the polarity setting (in the .config_v2()) and proposed switch
> > to atomic API.
> 
> Part of the reason I advocated for the atomic API is to make adding
> the polarity functionality easier. It does not archive this goal if
> we add the "flawed" code first and then transition to the atomic API.
> 
> > 
> > To make things easier and cleaner, I decided to put my atomic API
> > rework on top of those patches. In this way I can credit the
> > previous work and avoid rewriting DTS polarity inversion code
> > already developed and validated by Bhuvanchandra.
> 
> When you apply the patches using git apply, the authorship and
> signoffs will stay. There is no problem in including other peoples
> work into your patchset, credit will still be given. If you have to
> change another persons patch, you typically also add your signoff to
> show that you worked on it too.

I do wanted to reuse as much work as possible (especially that the code
was working).

> 
> Here is how I would do it:
> 
> 1. Start a new branch from mainline (or even -next).

With the newest mainline 4.9-rcX
SHA:0c2b6dc4fd4fa13796b319aae969a009f03222c6

the i.MX6q is not booting. Apparently I do need to wait for things to
calm down.

The last working version is v4.8, which PWM's code is the same as v4.7.

> 2. Implement the transition to the new atomic API and test it as such
> alone (this way we have no polarity support influence yet, just clean
> transition to a new API)

I _just_ needed to add polarity support (by setting one bit) to the
driver, so I ended up with rewriting the whole PWM i.MX driver :-).

> 3. Cherry pick the PWM core changes for the optional 2/3 args driver
> support (they should apply cleanly)
> 4. Cherry pick (they likely will fail to merge) or reimplement the PWM
> polarity driver changes on top of atomic API
> 5. Cherry pick device tree changes
> 
> With this approach we'll end up with a nice history where we should
> end up with a fully functional PWM system between every patch.

I'm fine with proposed approach. I will prepare v2 of patches soon.

> 
> Btw, past perfect tense is not really usual in commit messages.
> SubmittingPatches chapter 2 has some tips on writing good commit
> messages:
> https://www.kernel.org/doc/Documentation/SubmittingPatches

OK.

Thanks you for your support,

Best regards,

Łukasz Majewski

> 
> --
> Stefan


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

Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver

[Fabio Estevam]

Hi Lukasz,

On Tue, Oct 25, 2016 at 5:07 AM, Lukasz Majewski <l.majewski@majess.pl> wrote:

> With the newest mainline 4.9-rcX
> SHA:0c2b6dc4fd4fa13796b319aae969a009f03222c6
>
> the i.MX6q is not booting. Apparently I do need to wait for things to
> calm down.

Does this commit help with the boot issue you observed?

Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver

[Fabio Estevam]

On Tue, Oct 25, 2016 at 3:08 PM, Fabio Estevam <festevam@gmail.com> wrote:

>> the i.MX6q is not booting. Apparently I do need to wait for things to
>> calm down.
>
> Does this commit help with the boot issue you observed?

Sorry, missed to put the commit. Here it goes:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/arch/arm/mach-imx/gpc.c?id=eef0b282bb586259d35548851cf6a4ce847bb804

Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver

[Lukasz Majewski]

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

On Tue, 25 Oct 2016 15:09:03 -0200
Fabio Estevam <festevam@gmail.com> wrote:

> On Tue, Oct 25, 2016 at 3:08 PM, Fabio Estevam <festevam@gmail.com>
> wrote:
> 
> >> the i.MX6q is not booting. Apparently I do need to wait for things
> >> to calm down.
> >
> > Does this commit help with the boot issue you observed?
> 
> Sorry, missed to put the commit. Here it goes:
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/arch/arm/mach-imx/gpc.c?id=eef0b282bb586259d35548851cf6a4ce847bb804

I can confirm that the above patch fixed the problem.

My iMX6q board boots on 4.9.0-rc2

Thanks for support,

Łukasz Majewski

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

[PATCH v3 RESEND 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 enable/disable code (as proposed by Sascha Hauer) - this
  is the most notable change for v3

- 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.

- 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


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

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


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

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

Sascha Hauer (1):
  pwm: imx: remove ipg clock

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

-- 
2.1.4

[PATCH v3 RESEND 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 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 v3 RESEND 02/11] pwm: imx: remove ipg clock

[Lukasz Majewski]

From: Sascha Hauer <s.hauer@pengutronix.de>

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.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
---
[commit message text refactored by Lukasz Majewski <l.majewski@majess.pl>]
---
Changes for v3:
- New patch
---
 drivers/pwm/pwm-imx.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

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

Re: [PATCH v3 RESEND 02/11] pwm: imx: remove ipg clock

[Stefan Agner]

On 2016-12-26 23:55, Lukasz Majewski wrote:
> From: Sascha Hauer <s.hauer@pengutronix.de>
> 
> 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.

Hi Lukasz,

Has my concern addressed in any way with this resend?
https://lkml.org/lkml/2016/11/22/729

Breaking hardware is usually not an option :-)

I checked the i.MX 7 reference manual again, and in this case the
peripheral access clock is a clock line named "ipg_clk_s" (Table 12-20),
with a clock root "PWM1_CLK_ROOT" (Table 5-12). In i.MX 7 all clocks are
behind a single gate, so in fact it does not matter which clock we take.
Given that others have peripheral access behind the "pwm" gate, I guess
we should take the "pwm" gate...

--
Stefan

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> [commit message text refactored by Lukasz Majewski <l.majewski@majess.pl>]
> ---
> Changes for v3:
> - New patch
> ---
>  drivers/pwm/pwm-imx.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d600fd5..70609ef2 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -49,7 +49,6 @@
>  
>  struct imx_chip {
>  	struct clk	*clk_per;
> -	struct clk	*clk_ipg;
>  
>  	void __iomem	*mmio_base;
>  
> @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
>  		struct pwm_device *pwm, int duty_ns, int period_ns)
>  {
>  	struct imx_chip *imx = to_imx_chip(chip);
> -	int ret;
> -
> -	ret = clk_prepare_enable(imx->clk_ipg);
> -	if (ret)
> -		return ret;
>  
> -	ret = imx->config(chip, pwm, duty_ns, period_ns);
> -
> -	clk_disable_unprepare(imx->clk_ipg);
> -
> -	return ret;
> +	return imx->config(chip, pwm, duty_ns, period_ns);
>  }
>  
>  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
>  		return PTR_ERR(imx->clk_per);
>  	}
>  
> -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> -	if (IS_ERR(imx->clk_ipg)) {
> -		dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> -				PTR_ERR(imx->clk_ipg));
> -		return PTR_ERR(imx->clk_ipg);
> -	}
> -
>  	imx->chip.ops = &imx_pwm_ops;
>  	imx->chip.dev = &pdev->dev;
>  	imx->chip.base = -1;

Re: [PATCH v3 RESEND 02/11] pwm: imx: remove ipg clock

[Lukasz Majewski]

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

Hi Stefan,

> On 2016-12-26 23:55, Lukasz Majewski wrote:
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > 
> > 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.
> 
> Hi Lukasz,
> 
> Has my concern addressed in any way with this resend?
> https://lkml.org/lkml/2016/11/22/729

Unfortunately not, since I don't have iMX7 for testing.

> 
> Breaking hardware is usually not an option :-)

Yes, I know, but

Please look on the patch set from my perspective:

I originally wanted to add polarity inversion to PWM. Then, there was
the request from you and Boris to go with "atomicity" support, so I
converted the driver to support it.

This patch set has been resent on purpose at the end of merge window,
so we do have some time to fix it if it would be accepted to -next
tree (or any other PWM related one). Moreover, the burden for preparing
patches would be smaller - since we all have agreed that "atomicity" is
a more than welcome feature.


> 
> I checked the i.MX 7 reference manual again, and in this case the
> peripheral access clock is a clock line named "ipg_clk_s" (Table
> 12-20), with a clock root "PWM1_CLK_ROOT" (Table 5-12). In i.MX 7 all
> clocks are behind a single gate, so in fact it does not matter which
> clock we take. Given that others have peripheral access behind the
> "pwm" gate, I guess we should take the "pwm" gate...


If possible please prepare a patch. It would be the best solution.

Thanks in advance,
Łukasz Majewski

> 
> --
> Stefan
> 
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > [commit message text refactored by Lukasz Majewski
> > <l.majewski@majess.pl>] ---
> > Changes for v3:
> > - New patch
> > ---
> >  drivers/pwm/pwm-imx.c | 19 +------------------
> >  1 file changed, 1 insertion(+), 18 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index d600fd5..70609ef2 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -49,7 +49,6 @@
> >  
> >  struct imx_chip {
> >  	struct clk	*clk_per;
> > -	struct clk	*clk_ipg;
> >  
> >  	void __iomem	*mmio_base;
> >  
> > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip
> > *chip, struct pwm_device *pwm, int duty_ns, int period_ns)
> >  {
> >  	struct imx_chip *imx = to_imx_chip(chip);
> > -	int ret;
> > -
> > -	ret = clk_prepare_enable(imx->clk_ipg);
> > -	if (ret)
> > -		return ret;
> >  
> > -	ret = imx->config(chip, pwm, duty_ns, period_ns);
> > -
> > -	clk_disable_unprepare(imx->clk_ipg);
> > -
> > -	return ret;
> > +	return imx->config(chip, pwm, duty_ns, period_ns);
> >  }
> >  
> >  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device
> > *pwm) @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct
> > platform_device *pdev) return PTR_ERR(imx->clk_per);
> >  	}
> >  
> > -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > -	if (IS_ERR(imx->clk_ipg)) {
> > -		dev_err(&pdev->dev, "getting ipg clock failed with
> > %ld\n",
> > -				PTR_ERR(imx->clk_ipg));
> > -		return PTR_ERR(imx->clk_ipg);
> > -	}
> > -
> >  	imx->chip.ops = &imx_pwm_ops;
> >  	imx->chip.dev = &pdev->dev;
> >  	imx->chip.base = -1;


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

Re: [PATCH v3 RESEND 02/11] pwm: imx: remove ipg clock

[Lukasz Majewski]

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

Hi Stefan,

> Hi Stefan,
> 
> > On 2016-12-26 23:55, Lukasz Majewski wrote:
> > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > 
> > > 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.
> > 
> > Hi Lukasz,
> > 
> > Has my concern addressed in any way with this resend?
> > https://lkml.org/lkml/2016/11/22/729
> 
> Unfortunately not, since I don't have iMX7 for testing.
> 
> > 
> > Breaking hardware is usually not an option :-)
> 
> Yes, I know, but
> 
> Please look on the patch set from my perspective:
> 
> I originally wanted to add polarity inversion to PWM. Then, there was
> the request from you and Boris to go with "atomicity" support, so I
> converted the driver to support it.
> 
> This patch set has been resent on purpose at the end of merge window,
> so we do have some time to fix it if it would be accepted to -next
> tree (or any other PWM related one). Moreover, the burden for
> preparing patches would be smaller - since we all have agreed that
> "atomicity" is a more than welcome feature.
> 
> 
> > 
> > I checked the i.MX 7 reference manual again, and in this case the
> > peripheral access clock is a clock line named "ipg_clk_s" (Table
> > 12-20), with a clock root "PWM1_CLK_ROOT" (Table 5-12). In i.MX 7
> > all clocks are behind a single gate, so in fact it does not matter
> > which clock we take. Given that others have peripheral access
> > behind the "pwm" gate, I guess we should take the "pwm" gate...
> 
> 
> If possible please prepare a patch. It would be the best solution.

If I might ask - are you willing to prepare patch to fix iMX7 or shall
I roll back to the ipg code already present in main line ?


Best regards,
Łukasz Majewski

> 
> Thanks in advance,
> Łukasz Majewski
> 
> > 
> > --
> > Stefan
> > 
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > > [commit message text refactored by Lukasz Majewski
> > > <l.majewski@majess.pl>] ---
> > > Changes for v3:
> > > - New patch
> > > ---
> > >  drivers/pwm/pwm-imx.c | 19 +------------------
> > >  1 file changed, 1 insertion(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > index d600fd5..70609ef2 100644
> > > --- a/drivers/pwm/pwm-imx.c
> > > +++ b/drivers/pwm/pwm-imx.c
> > > @@ -49,7 +49,6 @@
> > >  
> > >  struct imx_chip {
> > >  	struct clk	*clk_per;
> > > -	struct clk	*clk_ipg;
> > >  
> > >  	void __iomem	*mmio_base;
> > >  
> > > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip
> > > *chip, struct pwm_device *pwm, int duty_ns, int period_ns)
> > >  {
> > >  	struct imx_chip *imx = to_imx_chip(chip);
> > > -	int ret;
> > > -
> > > -	ret = clk_prepare_enable(imx->clk_ipg);
> > > -	if (ret)
> > > -		return ret;
> > >  
> > > -	ret = imx->config(chip, pwm, duty_ns, period_ns);
> > > -
> > > -	clk_disable_unprepare(imx->clk_ipg);
> > > -
> > > -	return ret;
> > > +	return imx->config(chip, pwm, duty_ns, period_ns);
> > >  }
> > >  
> > >  static int imx_pwm_enable(struct pwm_chip *chip, struct
> > > pwm_device *pwm) @@ -293,13 +283,6 @@ static int
> > > imx_pwm_probe(struct platform_device *pdev) return
> > > PTR_ERR(imx->clk_per); }
> > >  
> > > -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > > -	if (IS_ERR(imx->clk_ipg)) {
> > > -		dev_err(&pdev->dev, "getting ipg clock failed
> > > with %ld\n",
> > > -				PTR_ERR(imx->clk_ipg));
> > > -		return PTR_ERR(imx->clk_ipg);
> > > -	}
> > > -
> > >  	imx->chip.ops = &imx_pwm_ops;
> > >  	imx->chip.dev = &pdev->dev;
> > >  	imx->chip.base = -1;
> 

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

Re: [PATCH v3 RESEND 02/11] pwm: imx: remove ipg clock

[Stefan Agner]

On 2016-12-28 23:01, Lukasz Majewski wrote:
> Hi Stefan,
> 
>> Hi Stefan,
>>
>> > On 2016-12-26 23:55, Lukasz Majewski wrote:
>> > > From: Sascha Hauer <s.hauer@pengutronix.de>
>> > >
>> > > 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.
>> >
>> > Hi Lukasz,
>> >
>> > Has my concern addressed in any way with this resend?
>> > https://lkml.org/lkml/2016/11/22/729
>>
>> Unfortunately not, since I don't have iMX7 for testing.
>>
>> >
>> > Breaking hardware is usually not an option :-)
>>
>> Yes, I know, but
>>
>> Please look on the patch set from my perspective:
>>
>> I originally wanted to add polarity inversion to PWM. Then, there was
>> the request from you and Boris to go with "atomicity" support, so I
>> converted the driver to support it.
>>
>> This patch set has been resent on purpose at the end of merge window,
>> so we do have some time to fix it if it would be accepted to -next
>> tree (or any other PWM related one). Moreover, the burden for
>> preparing patches would be smaller - since we all have agreed that
>> "atomicity" is a more than welcome feature.
>>
>>
>> >
>> > I checked the i.MX 7 reference manual again, and in this case the
>> > peripheral access clock is a clock line named "ipg_clk_s" (Table
>> > 12-20), with a clock root "PWM1_CLK_ROOT" (Table 5-12). In i.MX 7
>> > all clocks are behind a single gate, so in fact it does not matter
>> > which clock we take. Given that others have peripheral access
>> > behind the "pwm" gate, I guess we should take the "pwm" gate...
>>
>>
>> If possible please prepare a patch. It would be the best solution.
> 
> If I might ask - are you willing to prepare patch to fix iMX7 or shall
> I roll back to the ipg code already present in main line ?

I doubt that just rolling back the existing code works for the new
atomic change... I guess we have to really introduce a clk enable on
each register access, also for the new atomic code.

Not sure if we should fold this change in a existing commit or create a
new patch ontop of your changes. The latter is probably easier, but
creates a "window of brokenness" for i.MX 7 in git history. But then, I
don't really mind since it currently works more or less by chance...

I can prepare a patch.

--
Stefan

> 
> 
> Best regards,
> Łukasz Majewski
> 
>>
>> Thanks in advance,
>> Łukasz Majewski
>>
>> >
>> > --
>> > Stefan
>> >
>> > >
>> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
>> > > ---
>> > > [commit message text refactored by Lukasz Majewski
>> > > <l.majewski@majess.pl>] ---
>> > > Changes for v3:
>> > > - New patch
>> > > ---
>> > >  drivers/pwm/pwm-imx.c | 19 +------------------
>> > >  1 file changed, 1 insertion(+), 18 deletions(-)
>> > >
>> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
>> > > index d600fd5..70609ef2 100644
>> > > --- a/drivers/pwm/pwm-imx.c
>> > > +++ b/drivers/pwm/pwm-imx.c
>> > > @@ -49,7 +49,6 @@
>> > >
>> > >  struct imx_chip {
>> > >  	struct clk	*clk_per;
>> > > -	struct clk	*clk_ipg;
>> > >
>> > >  	void __iomem	*mmio_base;
>> > >
>> > > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip
>> > > *chip, struct pwm_device *pwm, int duty_ns, int period_ns)
>> > >  {
>> > >  	struct imx_chip *imx = to_imx_chip(chip);
>> > > -	int ret;
>> > > -
>> > > -	ret = clk_prepare_enable(imx->clk_ipg);
>> > > -	if (ret)
>> > > -		return ret;
>> > >
>> > > -	ret = imx->config(chip, pwm, duty_ns, period_ns);
>> > > -
>> > > -	clk_disable_unprepare(imx->clk_ipg);
>> > > -
>> > > -	return ret;
>> > > +	return imx->config(chip, pwm, duty_ns, period_ns);
>> > >  }
>> > >
>> > >  static int imx_pwm_enable(struct pwm_chip *chip, struct
>> > > pwm_device *pwm) @@ -293,13 +283,6 @@ static int
>> > > imx_pwm_probe(struct platform_device *pdev) return
>> > > PTR_ERR(imx->clk_per); }
>> > >
>> > > -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
>> > > -	if (IS_ERR(imx->clk_ipg)) {
>> > > -		dev_err(&pdev->dev, "getting ipg clock failed
>> > > with %ld\n",
>> > > -				PTR_ERR(imx->clk_ipg));
>> > > -		return PTR_ERR(imx->clk_ipg);
>> > > -	}
>> > > -
>> > >  	imx->chip.ops = &imx_pwm_ops;
>> > >  	imx->chip.dev = &pdev->dev;
>> > >  	imx->chip.base = -1;
>>

Re: [PATCH v3 RESEND 02/11] pwm: imx: remove ipg clock

[Lukasz Majewski]

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

Hi Stefan,

> On 2016-12-28 23:01, Lukasz Majewski wrote:
> > Hi Stefan,
> > 
> >> Hi Stefan,
> >>
> >> > On 2016-12-26 23:55, Lukasz Majewski wrote:
> >> > > From: Sascha Hauer <s.hauer@pengutronix.de>
> >> > >
> >> > > 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.
> >> >
> >> > Hi Lukasz,
> >> >
> >> > Has my concern addressed in any way with this resend?
> >> > https://lkml.org/lkml/2016/11/22/729
> >>
> >> Unfortunately not, since I don't have iMX7 for testing.
> >>
> >> >
> >> > Breaking hardware is usually not an option :-)
> >>
> >> Yes, I know, but
> >>
> >> Please look on the patch set from my perspective:
> >>
> >> I originally wanted to add polarity inversion to PWM. Then, there
> >> was the request from you and Boris to go with "atomicity" support,
> >> so I converted the driver to support it.
> >>
> >> This patch set has been resent on purpose at the end of merge
> >> window, so we do have some time to fix it if it would be accepted
> >> to -next tree (or any other PWM related one). Moreover, the burden
> >> for preparing patches would be smaller - since we all have agreed
> >> that "atomicity" is a more than welcome feature.
> >>
> >>
> >> >
> >> > I checked the i.MX 7 reference manual again, and in this case the
> >> > peripheral access clock is a clock line named "ipg_clk_s" (Table
> >> > 12-20), with a clock root "PWM1_CLK_ROOT" (Table 5-12). In i.MX 7
> >> > all clocks are behind a single gate, so in fact it does not
> >> > matter which clock we take. Given that others have peripheral
> >> > access behind the "pwm" gate, I guess we should take the "pwm"
> >> > gate...
> >>
> >>
> >> If possible please prepare a patch. It would be the best solution.
> > 
> > If I might ask - are you willing to prepare patch to fix iMX7 or
> > shall I roll back to the ipg code already present in main line ?
> 
> I doubt that just rolling back the existing code works for the new
> atomic change... I guess we have to really introduce a clk enable on
> each register access, also for the new atomic code.

Maybe this is a way to go.

> 
> Not sure if we should fold this change in a existing commit or create
> a new patch ontop of your changes. The latter is probably easier,

I would opt for creating (adding) changes on top of my changes.

> but
> creates a "window of brokenness" for i.MX 7 in git history. But then,
> I don't really mind since it currently works more or less by chance...

Hmm... it seems like Sascha patch (included to the patch series):

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

is a good starting point (at least we removed ipg clock) for fixing
things.

> 
> I can prepare a patch.

Thank you :-)

Best regards,
Łukasz Majewski

> 
> --
> Stefan
> 
> > 
> > 
> > Best regards,
> > Łukasz Majewski
> > 
> >>
> >> Thanks in advance,
> >> Łukasz Majewski
> >>
> >> >
> >> > --
> >> > Stefan
> >> >
> >> > >
> >> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> >> > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> >> > > ---
> >> > > [commit message text refactored by Lukasz Majewski
> >> > > <l.majewski@majess.pl>] ---
> >> > > Changes for v3:
> >> > > - New patch
> >> > > ---
> >> > >  drivers/pwm/pwm-imx.c | 19 +------------------
> >> > >  1 file changed, 1 insertion(+), 18 deletions(-)
> >> > >
> >> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> >> > > index d600fd5..70609ef2 100644
> >> > > --- a/drivers/pwm/pwm-imx.c
> >> > > +++ b/drivers/pwm/pwm-imx.c
> >> > > @@ -49,7 +49,6 @@
> >> > >
> >> > >  struct imx_chip {
> >> > >  	struct clk	*clk_per;
> >> > > -	struct clk	*clk_ipg;
> >> > >
> >> > >  	void __iomem	*mmio_base;
> >> > >
> >> > > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip
> >> > > *chip, struct pwm_device *pwm, int duty_ns, int period_ns)
> >> > >  {
> >> > >  	struct imx_chip *imx = to_imx_chip(chip);
> >> > > -	int ret;
> >> > > -
> >> > > -	ret = clk_prepare_enable(imx->clk_ipg);
> >> > > -	if (ret)
> >> > > -		return ret;
> >> > >
> >> > > -	ret = imx->config(chip, pwm, duty_ns, period_ns);
> >> > > -
> >> > > -	clk_disable_unprepare(imx->clk_ipg);
> >> > > -
> >> > > -	return ret;
> >> > > +	return imx->config(chip, pwm, duty_ns, period_ns);
> >> > >  }
> >> > >
> >> > >  static int imx_pwm_enable(struct pwm_chip *chip, struct
> >> > > pwm_device *pwm) @@ -293,13 +283,6 @@ static int
> >> > > imx_pwm_probe(struct platform_device *pdev) return
> >> > > PTR_ERR(imx->clk_per); }
> >> > >
> >> > > -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> >> > > -	if (IS_ERR(imx->clk_ipg)) {
> >> > > -		dev_err(&pdev->dev, "getting ipg clock failed
> >> > > with %ld\n",
> >> > > -				PTR_ERR(imx->clk_ipg));
> >> > > -		return PTR_ERR(imx->clk_ipg);
> >> > > -	}
> >> > > -
> >> > >  	imx->chip.ops = &imx_pwm_ops;
> >> > >  	imx->chip.dev = &pdev->dev;
> >> > >  	imx->chip.base = -1;
> >>


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

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

[Lukasz Majewski]

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

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
Acked-by: Shawn Guo <shawn.guo@linaro.org>
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
---
Changes for 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 70609ef2..d594501 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -230,7 +230,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,
@@ -241,16 +248,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[] = {
@@ -272,6 +282,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;
@@ -283,7 +295,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;
@@ -294,7 +306,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 v3 RESEND 04/11] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Lukasz Majewski]

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

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

Suggested-by: Stefan Agner <stefan@agner.ch>
Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Reviewed-by: Stefan Agner <stefan@agner.ch>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
[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 d594501..8f911b9 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,
@@ -231,9 +246,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,
 };
 
@@ -252,8 +267,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 v3 RESEND 05/11] pwm: imx: Move PWMv2 software reset code to a separate function

[Lukasz Majewski]

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

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

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

---
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 8497902..b4e5803 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -119,6 +119,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)
 {
@@ -128,7 +147,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;
 
 	/*
@@ -151,15 +170,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 v3 RESEND 06/11] pwm: imx: Move PWMv2 wait for fifo slot code to a separate function

[Lukasz Majewski]

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

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

Suggested-by: Stefan Agner <stefan@agner.ch>
Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Reviewed-by: Stefan Agner <stefan@agner.ch>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
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 b4e5803..ebe9b0c 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -137,18 +137,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.
@@ -157,21 +175,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 v3 RESEND 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 v3:
- Remove ipg clock enable/disable functions

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

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

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

[Boris Brezillon]

Hi Lukasz,

On Mon, 26 Dec 2016 23:55:57 +0100
Lukasz Majewski <l.majewski@majess.pl> 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 v3:
> - Remove ipg clock enable/disable functions
> 
> Changes for v2:
> - None
> ---
>  drivers/pwm/pwm-imx.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index ebe9b0c..cd53c05 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -159,6 +159,75 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
>  	}
>  }
>  
> +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    struct pwm_state *state)
> +{
> +	unsigned long period_cycles, duty_cycles, prescale;
> +	struct imx_chip *imx = to_imx_chip(chip);
> +	struct pwm_state cstate;
> +	unsigned long long c;
> +	u32 cr = 0;
> +	int ret;
> +
> +	pwm_get_state(pwm, &cstate);
> +
> +	c = clk_get_rate(imx->clk_per);
> +	c *= state->period;
> +
> +	do_div(c, 1000000000);
> +	period_cycles = c;
> +
> +	prescale = period_cycles / 0x10000 + 1;
> +
> +	period_cycles /= prescale;
> +	c = (unsigned long long)period_cycles * state->duty_cycle;
> +	do_div(c, state->period);
> +	duty_cycles = c;
> +
> +	/*
> +	 * according to imx pwm RM, the real period value should be
> +	 * PERIOD value in PWMPR plus 2.
> +	 */
> +	if (period_cycles > 2)
> +		period_cycles -= 2;
> +	else
> +		period_cycles = 0;
> +
> +	/* Enable the clock if the PWM is being enabled. */
> +	if (state->enabled && !cstate.enabled) {
> +		ret = clk_prepare_enable(imx->clk_per);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Wait for a free FIFO slot if the PWM is already enabled, and flush
> +	 * the FIFO if the PWM was disabled and is about to be enabled.
> +	 */
> +	if (cstate.enabled)
> +		imx_pwm_wait_fifo_slot(chip, pwm);
> +	else if (state->enabled)
> +		imx_pwm_sw_reset(chip);
> +
> +	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> +	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> +
> +	cr |= MX3_PWMCR_PRESCALER(prescale) |
> +	      MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> +	      MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
> +
> +	if (state->enabled)
> +		cr |= MX3_PWMCR_EN;
> +
> +	writel(cr, imx->mmio_base + MX3_PWMCR);
> +
> +	/* Disable the clock if the PWM is being disabled. */
> +	if (!state->enabled && cstate.enabled)
> +		clk_disable_unprepare(imx->clk_per);
> +
> +	return 0;
> +}
> +

Stefan suggested to rework this function to avoid unneeded
duty/period calculation and reg write when disabling the PWM. Why
didn't you send a v4 addressing that instead of resending the exact
same v3?

Same goes for the regression introduced in patch 2: I think it's better
to keep things bisectable on all platforms (even if it appeared to work
by chance on imx7, it did work before this change).

That's just my opinion, but when you get reviews on a patch series, it's
better to address them directly (especially when issues can be easily
fixed) than provide follow-up patches.

Regards,

Boris

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

[Lukasz Majewski]

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

Hi Boris,

> Hi Lukasz,
> 
> On Mon, 26 Dec 2016 23:55:57 +0100
> Lukasz Majewski <l.majewski@majess.pl> 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 v3:
> > - Remove ipg clock enable/disable functions
> > 
> > Changes for v2:
> > - None
> > ---
> >  drivers/pwm/pwm-imx.c | 70
> > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> > 70 insertions(+)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index ebe9b0c..cd53c05 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -159,6 +159,75 @@ static void imx_pwm_wait_fifo_slot(struct
> > pwm_chip *chip, }
> >  }
> >  
> > +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct
> > pwm_device *pwm,
> > +			    struct pwm_state *state)
> > +{
> > +	unsigned long period_cycles, duty_cycles, prescale;
> > +	struct imx_chip *imx = to_imx_chip(chip);
> > +	struct pwm_state cstate;
> > +	unsigned long long c;
> > +	u32 cr = 0;
> > +	int ret;
> > +
> > +	pwm_get_state(pwm, &cstate);
> > +
> > +	c = clk_get_rate(imx->clk_per);
> > +	c *= state->period;
> > +
> > +	do_div(c, 1000000000);
> > +	period_cycles = c;
> > +
> > +	prescale = period_cycles / 0x10000 + 1;
> > +
> > +	period_cycles /= prescale;
> > +	c = (unsigned long long)period_cycles * state->duty_cycle;
> > +	do_div(c, state->period);
> > +	duty_cycles = c;
> > +
> > +	/*
> > +	 * according to imx pwm RM, the real period value should be
> > +	 * PERIOD value in PWMPR plus 2.
> > +	 */
> > +	if (period_cycles > 2)
> > +		period_cycles -= 2;
> > +	else
> > +		period_cycles = 0;
> > +
> > +	/* Enable the clock if the PWM is being enabled. */
> > +	if (state->enabled && !cstate.enabled) {
> > +		ret = clk_prepare_enable(imx->clk_per);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	/*
> > +	 * Wait for a free FIFO slot if the PWM is already
> > enabled, and flush
> > +	 * the FIFO if the PWM was disabled and is about to be
> > enabled.
> > +	 */
> > +	if (cstate.enabled)
> > +		imx_pwm_wait_fifo_slot(chip, pwm);
> > +	else if (state->enabled)
> > +		imx_pwm_sw_reset(chip);
> > +
> > +	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > +	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > +
> > +	cr |= MX3_PWMCR_PRESCALER(prescale) |
> > +	      MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> > +	      MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
> > +
> > +	if (state->enabled)
> > +		cr |= MX3_PWMCR_EN;
> > +
> > +	writel(cr, imx->mmio_base + MX3_PWMCR);
> > +
> > +	/* Disable the clock if the PWM is being disabled. */
> > +	if (!state->enabled && cstate.enabled)
> > +		clk_disable_unprepare(imx->clk_per);
> > +
> > +	return 0;
> > +}
> > +
> 
> Stefan suggested to rework this function to avoid unneeded
> duty/period calculation and reg write when disabling the PWM. Why
> didn't you send a v4 addressing that instead of resending the exact
> same v3?

The discussion between you and Stefan was in this thread:
http://patchwork.ozlabs.org/patch/689790/

Stefan proposed change, you replied with your concerns and that is all.
No clear decision what to change until today when Stefan prepared
separate (concise) patch (now I see what is the problem).


> 
> Same goes for the regression introduced in patch 2: I think it's
> better to keep things bisectable on all platforms (even if it
> appeared to work by chance on imx7, it did work before this change).

Could you be more specific about your idea to solve this problem?

> 
> That's just my opinion, but when you get reviews on a patch series,
> it's better to address them directly (especially when issues can be
> easily fixed) than provide follow-up patches.

I do not have iMX7 for testing/development, so I could not reproduce
the error and address the issue directly.

I can at best integrate Stefan's patch and hope to not introduce
regression.



Best regards,
Łukasz Majewski

> 
> Regards,
> 
> Boris


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

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

[Boris Brezillon]

Hi Lukasz,

On Thu, 29 Dec 2016 17:45:35 +0100
Lukasz Majewski <l.majewski@majess.pl> wrote:

> Hi Boris,
> 
> > Hi Lukasz,
> > 
> > On Mon, 26 Dec 2016 23:55:57 +0100
> > Lukasz Majewski <l.majewski@majess.pl> 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 v3:
> > > - Remove ipg clock enable/disable functions
> > > 
> > > Changes for v2:
> > > - None
> > > ---
> > >  drivers/pwm/pwm-imx.c | 70
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> > > 70 insertions(+)
> > > 
> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > index ebe9b0c..cd53c05 100644
> > > --- a/drivers/pwm/pwm-imx.c
> > > +++ b/drivers/pwm/pwm-imx.c
> > > @@ -159,6 +159,75 @@ static void imx_pwm_wait_fifo_slot(struct
> > > pwm_chip *chip, }
> > >  }
> > >  
> > > +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct
> > > pwm_device *pwm,
> > > +			    struct pwm_state *state)
> > > +{
> > > +	unsigned long period_cycles, duty_cycles, prescale;
> > > +	struct imx_chip *imx = to_imx_chip(chip);
> > > +	struct pwm_state cstate;
> > > +	unsigned long long c;
> > > +	u32 cr = 0;
> > > +	int ret;
> > > +
> > > +	pwm_get_state(pwm, &cstate);
> > > +
> > > +	c = clk_get_rate(imx->clk_per);
> > > +	c *= state->period;
> > > +
> > > +	do_div(c, 1000000000);
> > > +	period_cycles = c;
> > > +
> > > +	prescale = period_cycles / 0x10000 + 1;
> > > +
> > > +	period_cycles /= prescale;
> > > +	c = (unsigned long long)period_cycles * state->duty_cycle;
> > > +	do_div(c, state->period);
> > > +	duty_cycles = c;
> > > +
> > > +	/*
> > > +	 * according to imx pwm RM, the real period value should be
> > > +	 * PERIOD value in PWMPR plus 2.
> > > +	 */
> > > +	if (period_cycles > 2)
> > > +		period_cycles -= 2;
> > > +	else
> > > +		period_cycles = 0;
> > > +
> > > +	/* Enable the clock if the PWM is being enabled. */
> > > +	if (state->enabled && !cstate.enabled) {
> > > +		ret = clk_prepare_enable(imx->clk_per);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Wait for a free FIFO slot if the PWM is already
> > > enabled, and flush
> > > +	 * the FIFO if the PWM was disabled and is about to be
> > > enabled.
> > > +	 */
> > > +	if (cstate.enabled)
> > > +		imx_pwm_wait_fifo_slot(chip, pwm);
> > > +	else if (state->enabled)
> > > +		imx_pwm_sw_reset(chip);
> > > +
> > > +	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > > +	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > > +
> > > +	cr |= MX3_PWMCR_PRESCALER(prescale) |
> > > +	      MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> > > +	      MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
> > > +
> > > +	if (state->enabled)
> > > +		cr |= MX3_PWMCR_EN;
> > > +
> > > +	writel(cr, imx->mmio_base + MX3_PWMCR);
> > > +
> > > +	/* Disable the clock if the PWM is being disabled. */
> > > +	if (!state->enabled && cstate.enabled)
> > > +		clk_disable_unprepare(imx->clk_per);
> > > +
> > > +	return 0;
> > > +}
> > > +  
> > 
> > Stefan suggested to rework this function to avoid unneeded
> > duty/period calculation and reg write when disabling the PWM. Why
> > didn't you send a v4 addressing that instead of resending the exact
> > same v3?  
> 
> The discussion between you and Stefan was in this thread:
> http://patchwork.ozlabs.org/patch/689790/
> 
> Stefan proposed change, you replied with your concerns and that is all.

Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I think we
both agreed that most of the code was unneeded when all we want to do
is disable the PWM.

My concern was more about the way PWM changes are applied (->apply()
returns before the change is actually applied), but I agreed that it
could be fixed later on (if other people think it's really needed),
since the existing code already handles it this way.

> No clear decision what to change until today when Stefan prepared
> separate (concise) patch (now I see what is the problem).
> 

The patch proposed by Stefan is addressing a different problem: the
periph clock has to be enabled before accessing registers.

> 
> > 
> > Same goes for the regression introduced in patch 2: I think it's
> > better to keep things bisectable on all platforms (even if it
> > appeared to work by chance on imx7, it did work before this change).  
> 
> Could you be more specific about your idea to solve this problem?

Stefan already provided a patch, I just think it should be fixed before
patch 2 to avoid breaking bisectibility.

> 
> > 
> > That's just my opinion, but when you get reviews on a patch series,
> > it's better to address them directly (especially when issues can be
> > easily fixed) than provide follow-up patches.  
> 
> I do not have iMX7 for testing/development, so I could not reproduce
> the error and address the issue directly.

Well, the description made by Stefan seemed pretty clear to me: you
need to enable the periph clock before accessing PWM registers.

> 
> I can at best integrate Stefan's patch and hope to not introduce
> regression.

You can ask others to test your own patches. In this case, just
clearly state that the patch is untested and that you'd like people
owning a specific platform to test it.

Regards,

Boris

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

[Lukasz Majewski]

Hi Boris,

> Hi Lukasz,
> 
> On Thu, 29 Dec 2016 17:45:35 +0100
> Lukasz Majewski <l.majewski@majess.pl> wrote:
> 
> > Hi Boris,
> > 
> > > Hi Lukasz,
> > > 
> > > On Mon, 26 Dec 2016 23:55:57 +0100
> > > Lukasz Majewski <l.majewski@majess.pl> 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 v3:
> > > > - Remove ipg clock enable/disable functions
> > > > 
> > > > Changes for v2:
> > > > - None
> > > > ---
> > > >  drivers/pwm/pwm-imx.c | 70
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file
> > > > changed, 70 insertions(+)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > index ebe9b0c..cd53c05 100644
> > > > --- a/drivers/pwm/pwm-imx.c
> > > > +++ b/drivers/pwm/pwm-imx.c
> > > > @@ -159,6 +159,75 @@ static void imx_pwm_wait_fifo_slot(struct
> > > > pwm_chip *chip, }
> > > >  }
> > > >  
> > > > +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct
> > > > pwm_device *pwm,
> > > > +			    struct pwm_state *state)
> > > > +{
> > > > +	unsigned long period_cycles, duty_cycles, prescale;
> > > > +	struct imx_chip *imx = to_imx_chip(chip);
> > > > +	struct pwm_state cstate;
> > > > +	unsigned long long c;
> > > > +	u32 cr = 0;
> > > > +	int ret;
> > > > +
> > > > +	pwm_get_state(pwm, &cstate);
> > > > +
> > > > +	c = clk_get_rate(imx->clk_per);
> > > > +	c *= state->period;
> > > > +
> > > > +	do_div(c, 1000000000);
> > > > +	period_cycles = c;
> > > > +
> > > > +	prescale = period_cycles / 0x10000 + 1;
> > > > +
> > > > +	period_cycles /= prescale;
> > > > +	c = (unsigned long long)period_cycles *
> > > > state->duty_cycle;
> > > > +	do_div(c, state->period);
> > > > +	duty_cycles = c;
> > > > +
> > > > +	/*
> > > > +	 * according to imx pwm RM, the real period value
> > > > should be
> > > > +	 * PERIOD value in PWMPR plus 2.
> > > > +	 */
> > > > +	if (period_cycles > 2)
> > > > +		period_cycles -= 2;
> > > > +	else
> > > > +		period_cycles = 0;
> > > > +
> > > > +	/* Enable the clock if the PWM is being enabled. */
> > > > +	if (state->enabled && !cstate.enabled) {
> > > > +		ret = clk_prepare_enable(imx->clk_per);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Wait for a free FIFO slot if the PWM is already
> > > > enabled, and flush
> > > > +	 * the FIFO if the PWM was disabled and is about to be
> > > > enabled.
> > > > +	 */
> > > > +	if (cstate.enabled)
> > > > +		imx_pwm_wait_fifo_slot(chip, pwm);
> > > > +	else if (state->enabled)
> > > > +		imx_pwm_sw_reset(chip);
> > > > +
> > > > +	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > > > +	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > > > +
> > > > +	cr |= MX3_PWMCR_PRESCALER(prescale) |
> > > > +	      MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> > > > +	      MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
> > > > +
> > > > +	if (state->enabled)
> > > > +		cr |= MX3_PWMCR_EN;
> > > > +
> > > > +	writel(cr, imx->mmio_base + MX3_PWMCR);
> > > > +
> > > > +	/* Disable the clock if the PWM is being disabled. */
> > > > +	if (!state->enabled && cstate.enabled)
> > > > +		clk_disable_unprepare(imx->clk_per);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +  
> > > 
> > > Stefan suggested to rework this function to avoid unneeded
> > > duty/period calculation and reg write when disabling the PWM. Why
> > > didn't you send a v4 addressing that instead of resending the
> > > exact same v3?  
> > 
> > The discussion between you and Stefan was in this thread:
> > http://patchwork.ozlabs.org/patch/689790/
> > 
> > Stefan proposed change, you replied with your concerns and that is
> > all.
> 

Please correct me if I've misunderstood something :-).

> Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I think we
> both agreed that most of the code was unneeded when all we want to do
> is disable the PWM.

So for the PATCH 7/11 we fix the issue with recalculating clocks
when we want to disable PWM.
	
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;

		/*
		 * Enable the clock if the PWM is not already
		 * enabled.
		 */
		if (!cstate.enabled) {
			ret = clk_prepare_enable(imx->clk_per);
			if (ret)
			return ret;
		}

		/*
		 * Wait for a free FIFO slot if the PWM is already
		 * enabled, and flush the FIFO if the PWM was disabled
		 * and is about to be enabled.
		 */
		if (cstate.enabled)
			imx_pwm_wait_fifo_slot(chip, pwm);
		else
			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);

		/* Disable the clock if the PWM is currently enabled. */
		if (cstate.enabled)
			clk_disable_unprepare(imx->clk_per);
	}



> 
> My concern was more about the way PWM changes are applied (->apply()
> returns before the change is actually applied), but I agreed that it
> could be fixed later on (if other people think it's really needed),
> since the existing code already handles it this way.

This is the issue with FIFO setting - but for now we do not deal with
it.

> 
> > No clear decision what to change until today when Stefan prepared
> > separate (concise) patch (now I see what is the problem).
> > 
> 
> The patch proposed by Stefan is addressing a different problem: the
> periph clock has to be enabled before accessing registers.

So for this reason Stefan's patch [1] always enable the clock no matter
if PWM clock is generated or not.

> 
> > 
> > > 
> > > Same goes for the regression introduced in patch 2: I think it's
> > > better to keep things bisectable on all platforms (even if it
> > > appeared to work by chance on imx7, it did work before this
> > > change).  
> > 
> > Could you be more specific about your idea to solve this problem?
> 
> Stefan already provided a patch, I just think it should be fixed
> before patch 2 to avoid breaking bisectibility.

My idea is as follows:

I will drop patch v2 (prepared by Sasha) and then squash Stefan's patch
[1] to patch 7/11. The "old" ipg enable code will be removed with other
not needed code during conversion.

In such a way we would preserve bisectibility.

What is your opinion?



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

> 
> > 
> > > 
> > > That's just my opinion, but when you get reviews on a patch
> > > series, it's better to address them directly (especially when
> > > issues can be easily fixed) than provide follow-up patches.  
> > 
> > I do not have iMX7 for testing/development, so I could not reproduce
> > the error and address the issue directly.
> 
> Well, the description made by Stefan seemed pretty clear to me: you
> need to enable the periph clock before accessing PWM registers.
> 
> > 
> > I can at best integrate Stefan's patch and hope to not introduce
> > regression.
> 
> You can ask others to test your own patches. In this case, just
> clearly state that the patch is untested and that you'd like people
> owning a specific platform to test it.
> 
> Regards,
> 
> Boris




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 v3 RESEND 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Boris Brezillon]

Hi Lukasz,

On Tue, 3 Jan 2017 12:43:14 +0100
Lukasz Majewski <lukma@denx.de> wrote:

> Hi Boris,
> 
> > Hi Lukasz,
> > 
> > On Thu, 29 Dec 2016 17:45:35 +0100
> > Lukasz Majewski <l.majewski@majess.pl> wrote:
> >   
> > > Hi Boris,
> > >   
> > > > Hi Lukasz,
> > > > 
> > > > On Mon, 26 Dec 2016 23:55:57 +0100
> > > > Lukasz Majewski <l.majewski@majess.pl> 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 v3:
> > > > > - Remove ipg clock enable/disable functions
> > > > > 
> > > > > Changes for v2:
> > > > > - None
> > > > > ---
> > > > >  drivers/pwm/pwm-imx.c | 70
> > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file
> > > > > changed, 70 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > > index ebe9b0c..cd53c05 100644
> > > > > --- a/drivers/pwm/pwm-imx.c
> > > > > +++ b/drivers/pwm/pwm-imx.c
> > > > > @@ -159,6 +159,75 @@ static void imx_pwm_wait_fifo_slot(struct
> > > > > pwm_chip *chip, }
> > > > >  }
> > > > >  
> > > > > +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct
> > > > > pwm_device *pwm,
> > > > > +			    struct pwm_state *state)
> > > > > +{
> > > > > +	unsigned long period_cycles, duty_cycles, prescale;
> > > > > +	struct imx_chip *imx = to_imx_chip(chip);
> > > > > +	struct pwm_state cstate;
> > > > > +	unsigned long long c;
> > > > > +	u32 cr = 0;
> > > > > +	int ret;
> > > > > +
> > > > > +	pwm_get_state(pwm, &cstate);
> > > > > +
> > > > > +	c = clk_get_rate(imx->clk_per);
> > > > > +	c *= state->period;
> > > > > +
> > > > > +	do_div(c, 1000000000);
> > > > > +	period_cycles = c;
> > > > > +
> > > > > +	prescale = period_cycles / 0x10000 + 1;
> > > > > +
> > > > > +	period_cycles /= prescale;
> > > > > +	c = (unsigned long long)period_cycles *
> > > > > state->duty_cycle;
> > > > > +	do_div(c, state->period);
> > > > > +	duty_cycles = c;
> > > > > +
> > > > > +	/*
> > > > > +	 * according to imx pwm RM, the real period value
> > > > > should be
> > > > > +	 * PERIOD value in PWMPR plus 2.
> > > > > +	 */
> > > > > +	if (period_cycles > 2)
> > > > > +		period_cycles -= 2;
> > > > > +	else
> > > > > +		period_cycles = 0;
> > > > > +
> > > > > +	/* Enable the clock if the PWM is being enabled. */
> > > > > +	if (state->enabled && !cstate.enabled) {
> > > > > +		ret = clk_prepare_enable(imx->clk_per);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +	}
> > > > > +
> > > > > +	/*
> > > > > +	 * Wait for a free FIFO slot if the PWM is already
> > > > > enabled, and flush
> > > > > +	 * the FIFO if the PWM was disabled and is about to be
> > > > > enabled.
> > > > > +	 */
> > > > > +	if (cstate.enabled)
> > > > > +		imx_pwm_wait_fifo_slot(chip, pwm);
> > > > > +	else if (state->enabled)
> > > > > +		imx_pwm_sw_reset(chip);
> > > > > +
> > > > > +	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > > > > +	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > > > > +
> > > > > +	cr |= MX3_PWMCR_PRESCALER(prescale) |
> > > > > +	      MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> > > > > +	      MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
> > > > > +
> > > > > +	if (state->enabled)
> > > > > +		cr |= MX3_PWMCR_EN;
> > > > > +
> > > > > +	writel(cr, imx->mmio_base + MX3_PWMCR);
> > > > > +
> > > > > +	/* Disable the clock if the PWM is being disabled. */
> > > > > +	if (!state->enabled && cstate.enabled)
> > > > > +		clk_disable_unprepare(imx->clk_per);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +    
> > > > 
> > > > Stefan suggested to rework this function to avoid unneeded
> > > > duty/period calculation and reg write when disabling the PWM. Why
> > > > didn't you send a v4 addressing that instead of resending the
> > > > exact same v3?    
> > > 
> > > The discussion between you and Stefan was in this thread:
> > > http://patchwork.ozlabs.org/patch/689790/
> > > 
> > > Stefan proposed change, you replied with your concerns and that is
> > > all.  
> >   
> 
> Please correct me if I've misunderstood something :-).
> 
> > Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I think we
> > both agreed that most of the code was unneeded when all we want to do
> > is disable the PWM.  
> 
> So for the PATCH 7/11 we fix the issue with recalculating clocks
> when we want to disable PWM.
> 	
> 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;
> 
> 		/*
> 		 * Enable the clock if the PWM is not already
> 		 * enabled.
> 		 */
> 		if (!cstate.enabled) {
> 			ret = clk_prepare_enable(imx->clk_per);
> 			if (ret)
> 			return ret;
> 		}
> 
> 		/*
> 		 * Wait for a free FIFO slot if the PWM is already
> 		 * enabled, and flush the FIFO if the PWM was disabled
> 		 * and is about to be enabled.
> 		 */
> 		if (cstate.enabled)
> 			imx_pwm_wait_fifo_slot(chip, pwm);
> 		else
> 			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);
> 
> 		/* Disable the clock if the PWM is currently enabled. */
> 		if (cstate.enabled)
> 			clk_disable_unprepare(imx->clk_per);
> 	}
> 
> 

Yep.

> 
> > 
> > My concern was more about the way PWM changes are applied (->apply()
> > returns before the change is actually applied), but I agreed that it
> > could be fixed later on (if other people think it's really needed),
> > since the existing code already handles it this way.  
> 
> This is the issue with FIFO setting - but for now we do not deal with
> it.

Exactly.

> 
> >   
> > > No clear decision what to change until today when Stefan prepared
> > > separate (concise) patch (now I see what is the problem).
> > >   
> > 
> > The patch proposed by Stefan is addressing a different problem: the
> > periph clock has to be enabled before accessing registers.  
> 
> So for this reason Stefan's patch [1] always enable the clock no matter
> if PWM clock is generated or not.

Yes.

> 
> >   
> > >   
> > > > 
> > > > Same goes for the regression introduced in patch 2: I think it's
> > > > better to keep things bisectable on all platforms (even if it
> > > > appeared to work by chance on imx7, it did work before this
> > > > change).    
> > > 
> > > Could you be more specific about your idea to solve this problem?  
> > 
> > Stefan already provided a patch, I just think it should be fixed
> > before patch 2 to avoid breaking bisectibility.  
> 
> My idea is as follows:
> 
> I will drop patch v2 (prepared by Sasha) and then squash Stefan's patch
> [1] to patch 7/11. The "old" ipg enable code will be removed with other
> not needed code during conversion.

How about keeping patch 2 but enabling/disabling the periph clk
in imx_pwm_config() instead of completely dropping the enable/disable
clk sequence.

In patch 7 you just add the logic we talked about earlier:
unconditionally enable the periph clk when entering the
imx_pwm_apply_v2() function and disable it before leaving the function.

This way you can preserve bisectibility and still get rid of the ipg
clk.

Stefan, what's your opinion?

Regards,

Boris

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

[Stefan Agner]

On 2017-01-03 04:46, Boris Brezillon wrote:
<snip>
>> > Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I think we
>> > both agreed that most of the code was unneeded when all we want to do
>> > is disable the PWM.
>>
>> So for the PATCH 7/11 we fix the issue with recalculating clocks
>> when we want to disable PWM.
>>
>> 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;
>>
>> 		/*
>> 		 * Enable the clock if the PWM is not already
>> 		 * enabled.
>> 		 */
>> 		if (!cstate.enabled) {
>> 			ret = clk_prepare_enable(imx->clk_per);
>> 			if (ret)
>> 			return ret;
>> 		}
>>
>> 		/*
>> 		 * Wait for a free FIFO slot if the PWM is already
>> 		 * enabled, and flush the FIFO if the PWM was disabled
>> 		 * and is about to be enabled.
>> 		 */
>> 		if (cstate.enabled)
>> 			imx_pwm_wait_fifo_slot(chip, pwm);
>> 		else
>> 			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);
>>
>> 		/* Disable the clock if the PWM is currently enabled. */
>> 		if (cstate.enabled)
>> 			clk_disable_unprepare(imx->clk_per);
>> 	}
>>
>>
> 
> Yep.
> 

This looks like a good transformation of the current Patch 7, but once
you merge my patch, it will look slightly different...

>>
>> >
>> > My concern was more about the way PWM changes are applied (->apply()
>> > returns before the change is actually applied), but I agreed that it
>> > could be fixed later on (if other people think it's really needed),
>> > since the existing code already handles it this way.
>>
>> This is the issue with FIFO setting - but for now we do not deal with
>> it.
> 
> Exactly.
> 
>>
>> >
>> > > No clear decision what to change until today when Stefan prepared
>> > > separate (concise) patch (now I see what is the problem).
>> > >
>> >
>> > The patch proposed by Stefan is addressing a different problem: the
>> > periph clock has to be enabled before accessing registers.
>>
>> So for this reason Stefan's patch [1] always enable the clock no matter
>> if PWM clock is generated or not.
> 
> Yes.
> 
>>
>> >
>> > >
>> > > >
>> > > > Same goes for the regression introduced in patch 2: I think it's
>> > > > better to keep things bisectable on all platforms (even if it
>> > > > appeared to work by chance on imx7, it did work before this
>> > > > change).
>> > >
>> > > Could you be more specific about your idea to solve this problem?
>> >
>> > Stefan already provided a patch, I just think it should be fixed
>> > before patch 2 to avoid breaking bisectibility.
>>
>> My idea is as follows:
>>
>> I will drop patch v2 (prepared by Sasha) and then squash Stefan's patch
>> [1] to patch 7/11. The "old" ipg enable code will be removed with other
>> not needed code during conversion.
> 
> How about keeping patch 2 but enabling/disabling the periph clk
> in imx_pwm_config() instead of completely dropping the enable/disable
> clk sequence.
> 
> In patch 7 you just add the logic we talked about earlier:
> unconditionally enable the periph clk when entering the
> imx_pwm_apply_v2() function and disable it before leaving the function.
> 
> This way you can preserve bisectibility and still get rid of the ipg
> clk.
> 
> Stefan, what's your opinion?

We will get rid of the ipg clocks anyway in patch 8 (which removes those
functions completely).

So I think Lukasz approach should be fine, just drop patch 2 and squash
my patch into patch 7.

--
Stefan

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

[Boris Brezillon]

On Tue, 03 Jan 2017 09:29:40 -0800
Stefan Agner <stefan@agner.ch> wrote:

> On 2017-01-03 04:46, Boris Brezillon wrote:
> <snip>
> >> > Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I think we
> >> > both agreed that most of the code was unneeded when all we want to do
> >> > is disable the PWM.  
> >>
> >> So for the PATCH 7/11 we fix the issue with recalculating clocks
> >> when we want to disable PWM.
> >>
> >> 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;
> >>
> >> 		/*
> >> 		 * Enable the clock if the PWM is not already
> >> 		 * enabled.
> >> 		 */
> >> 		if (!cstate.enabled) {
> >> 			ret = clk_prepare_enable(imx->clk_per);
> >> 			if (ret)
> >> 			return ret;
> >> 		}
> >>
> >> 		/*
> >> 		 * Wait for a free FIFO slot if the PWM is already
> >> 		 * enabled, and flush the FIFO if the PWM was disabled
> >> 		 * and is about to be enabled.
> >> 		 */
> >> 		if (cstate.enabled)
> >> 			imx_pwm_wait_fifo_slot(chip, pwm);
> >> 		else
> >> 			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);
> >>
> >> 		/* Disable the clock if the PWM is currently enabled. */
> >> 		if (cstate.enabled)
> >> 			clk_disable_unprepare(imx->clk_per);
> >> 	}
> >>
> >>  
> > 
> > Yep.
> >   
> 
> This looks like a good transformation of the current Patch 7, but once
> you merge my patch, it will look slightly different...

Yes. I think we should just unconditionally enable/disable the per_clk
at function entry/exit. The prepare_enable() call is almost free
when the clk is already enabled, so it's not like we're adding a huge
overhead by doing that.

> 
> >>  
> >> >
> >> > My concern was more about the way PWM changes are applied (->apply()
> >> > returns before the change is actually applied), but I agreed that it
> >> > could be fixed later on (if other people think it's really needed),
> >> > since the existing code already handles it this way.  
> >>
> >> This is the issue with FIFO setting - but for now we do not deal with
> >> it.  
> > 
> > Exactly.
> >   
> >>  
> >> >  
> >> > > No clear decision what to change until today when Stefan prepared
> >> > > separate (concise) patch (now I see what is the problem).
> >> > >  
> >> >
> >> > The patch proposed by Stefan is addressing a different problem: the
> >> > periph clock has to be enabled before accessing registers.  
> >>
> >> So for this reason Stefan's patch [1] always enable the clock no matter
> >> if PWM clock is generated or not.  
> > 
> > Yes.
> >   
> >>  
> >> >  
> >> > >  
> >> > > >
> >> > > > Same goes for the regression introduced in patch 2: I think it's
> >> > > > better to keep things bisectable on all platforms (even if it
> >> > > > appeared to work by chance on imx7, it did work before this
> >> > > > change).  
> >> > >
> >> > > Could you be more specific about your idea to solve this problem?  
> >> >
> >> > Stefan already provided a patch, I just think it should be fixed
> >> > before patch 2 to avoid breaking bisectibility.  
> >>
> >> My idea is as follows:
> >>
> >> I will drop patch v2 (prepared by Sasha) and then squash Stefan's patch
> >> [1] to patch 7/11. The "old" ipg enable code will be removed with other
> >> not needed code during conversion.  
> > 
> > How about keeping patch 2 but enabling/disabling the periph clk
> > in imx_pwm_config() instead of completely dropping the enable/disable
> > clk sequence.
> > 
> > In patch 7 you just add the logic we talked about earlier:
> > unconditionally enable the periph clk when entering the
> > imx_pwm_apply_v2() function and disable it before leaving the function.
> > 
> > This way you can preserve bisectibility and still get rid of the ipg
> > clk.
> > 
> > Stefan, what's your opinion?  
> 
> We will get rid of the ipg clocks anyway in patch 8 (which removes those
> functions completely).
> 
> So I think Lukasz approach should be fine, just drop patch 2 and squash
> my patch into patch 7.

Well, the end result will be same (ipg_clk will be gone after patch 8),
but then it's hard to track why this clock suddenly disappeared.
I still think it's worth adding an extra commit explaining that
enabling the per_clk before accessing IP registers is needed on some
platforms (imx7), and that IPG clk is actually not required until we
start using it as a source for the PWM signal generation.

Maybe I'm the only one to think so. In this case, feel free to drop
patch 2.

Regards,

Boris

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

[Lukasz Majewski]

Hi Boris, Stefan,

> On Tue, 03 Jan 2017 09:29:40 -0800
> Stefan Agner <stefan@agner.ch> wrote:
> 
> > On 2017-01-03 04:46, Boris Brezillon wrote:
> > <snip>
> > >> > Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I
> > >> > think we both agreed that most of the code was unneeded when
> > >> > all we want to do is disable the PWM.  
> > >>
> > >> So for the PATCH 7/11 we fix the issue with recalculating clocks
> > >> when we want to disable PWM.
> > >>
> > >> 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;
> > >>
> > >> 		/*
> > >> 		 * Enable the clock if the PWM is not already
> > >> 		 * enabled.
> > >> 		 */
> > >> 		if (!cstate.enabled) {
> > >> 			ret = clk_prepare_enable(imx->clk_per);
> > >> 			if (ret)
> > >> 			return ret;
> > >> 		}
> > >>
> > >> 		/*
> > >> 		 * Wait for a free FIFO slot if the PWM is
> > >> already
> > >> 		 * enabled, and flush the FIFO if the PWM was
> > >> disabled
> > >> 		 * and is about to be enabled.
> > >> 		 */
> > >> 		if (cstate.enabled)
> > >> 			imx_pwm_wait_fifo_slot(chip, pwm);
> > >> 		else
> > >> 			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);
> > >>
> > >> 		/* Disable the clock if the PWM is currently
> > >> enabled. */ if (cstate.enabled)
> > >> 			clk_disable_unprepare(imx->clk_per);
> > >> 	}
> > >>
> > >>  
> > > 
> > > Yep.
> > >   
> > 
> > This looks like a good transformation of the current Patch 7, but
> > once you merge my patch, it will look slightly different...
> 
> Yes. I think we should just unconditionally enable/disable the per_clk
> at function entry/exit. The prepare_enable() call is almost free
> when the clk is already enabled, so it's not like we're adding a huge
> overhead by doing that.

So in the above snippet we should replace:

if (!cstate.enabled) {
	ret = clk_prepare_enable(imx->clk_per);
	if (ret)
		return ret;
}

with
	ret = clk_prepare_enable(imx->clk_per);
	if (ret)
		return ret;

And 

if (cstate.enabled)
	clk_disable_unprepare(imx->clk_per);

with 
	clk_disable_unprepare(imx->clk_per);

> 
> > 
> > >>  
> > >> >
> > >> > My concern was more about the way PWM changes are applied
> > >> > (->apply() returns before the change is actually applied), but
> > >> > I agreed that it could be fixed later on (if other people
> > >> > think it's really needed), since the existing code already
> > >> > handles it this way.  
> > >>
> > >> This is the issue with FIFO setting - but for now we do not deal
> > >> with it.  
> > > 
> > > Exactly.
> > >   
> > >>  
> > >> >  
> > >> > > No clear decision what to change until today when Stefan
> > >> > > prepared separate (concise) patch (now I see what is the
> > >> > > problem). 
> > >> >
> > >> > The patch proposed by Stefan is addressing a different
> > >> > problem: the periph clock has to be enabled before accessing
> > >> > registers.  
> > >>
> > >> So for this reason Stefan's patch [1] always enable the clock no
> > >> matter if PWM clock is generated or not.  
> > > 
> > > Yes.
> > >   
> > >>  
> > >> >  
> > >> > >  
> > >> > > >
> > >> > > > Same goes for the regression introduced in patch 2: I
> > >> > > > think it's better to keep things bisectable on all
> > >> > > > platforms (even if it appeared to work by chance on imx7,
> > >> > > > it did work before this change).  
> > >> > >
> > >> > > Could you be more specific about your idea to solve this
> > >> > > problem?  
> > >> >
> > >> > Stefan already provided a patch, I just think it should be
> > >> > fixed before patch 2 to avoid breaking bisectibility.  
> > >>
> > >> My idea is as follows:
> > >>
> > >> I will drop patch v2 (prepared by Sasha) and then squash
> > >> Stefan's patch [1] to patch 7/11. The "old" ipg enable code will
> > >> be removed with other not needed code during conversion.  
> > > 
> > > How about keeping patch 2 but enabling/disabling the periph clk
> > > in imx_pwm_config() instead of completely dropping the
> > > enable/disable clk sequence.
> > > 
> > > In patch 7 you just add the logic we talked about earlier:
> > > unconditionally enable the periph clk when entering the
> > > imx_pwm_apply_v2() function and disable it before leaving the
> > > function.
> > > 
> > > This way you can preserve bisectibility and still get rid of the
> > > ipg clk.
> > > 
> > > Stefan, what's your opinion?  
> > 
> > We will get rid of the ipg clocks anyway in patch 8 (which removes
> > those functions completely).
> > 
> > So I think Lukasz approach should be fine, just drop patch 2 and
> > squash my patch into patch 7.
> 
> Well, the end result will be same (ipg_clk will be gone after patch
> 8), but then it's hard to track why this clock suddenly disappeared.
> I still think it's worth adding an extra commit explaining that
> enabling the per_clk before accessing IP registers is needed on some
> platforms (imx7), and that IPG clk is actually not required until we
> start using it as a source for the PWM signal generation.
> 
> Maybe I'm the only one to think so. In this case, feel free to drop
> patch 2.

If you feel really bad about this issue, then we can drop patch 2 and:

reorganize patch 7/11 to 
 - keep code, which adds imx_pwm_apply_v2() function code (just moves it
as is) 
 - remove .apply = imx_pwm_apply_v2 entry from pwm_ops structure.


On top of it add patch to enable/disable unconditionally the
imx->clk_per clock to avoid problems on imx7 (and state them in commit
message).

Then we add separate patch with 
.apply = imx_pwm_apply_v2 to pwm_ops structure to enable "new" atomic
approach.

And at last we apply patch 8/11, which removes the code for old (non
atomic) behaviour.

All the issues are documented in this way on the cost of having
"dead" (I mean not used) imx_pwm_apply_v2() for two commits.



> 
> Regards,
> 
> Boris




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 v3 RESEND 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Boris Brezillon]

On Tue, 3 Jan 2017 23:01:11 +0100
Lukasz Majewski <lukma@denx.de> wrote:

> Hi Boris, Stefan,
> 
> > On Tue, 03 Jan 2017 09:29:40 -0800
> > Stefan Agner <stefan@agner.ch> wrote:
> >   
> > > On 2017-01-03 04:46, Boris Brezillon wrote:
> > > <snip>  
> > > >> > Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I
> > > >> > think we both agreed that most of the code was unneeded when
> > > >> > all we want to do is disable the PWM.    
> > > >>
> > > >> So for the PATCH 7/11 we fix the issue with recalculating clocks
> > > >> when we want to disable PWM.
> > > >>
> > > >> 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;
> > > >>
> > > >> 		/*
> > > >> 		 * Enable the clock if the PWM is not already
> > > >> 		 * enabled.
> > > >> 		 */
> > > >> 		if (!cstate.enabled) {
> > > >> 			ret = clk_prepare_enable(imx->clk_per);
> > > >> 			if (ret)
> > > >> 			return ret;
> > > >> 		}
> > > >>
> > > >> 		/*
> > > >> 		 * Wait for a free FIFO slot if the PWM is
> > > >> already
> > > >> 		 * enabled, and flush the FIFO if the PWM was
> > > >> disabled
> > > >> 		 * and is about to be enabled.
> > > >> 		 */
> > > >> 		if (cstate.enabled)
> > > >> 			imx_pwm_wait_fifo_slot(chip, pwm);
> > > >> 		else
> > > >> 			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);
> > > >>
> > > >> 		/* Disable the clock if the PWM is currently
> > > >> enabled. */ if (cstate.enabled)
> > > >> 			clk_disable_unprepare(imx->clk_per);
> > > >> 	}
> > > >>
> > > >>    
> > > > 
> > > > Yep.
> > > >     
> > > 
> > > This looks like a good transformation of the current Patch 7, but
> > > once you merge my patch, it will look slightly different...  
> > 
> > Yes. I think we should just unconditionally enable/disable the per_clk
> > at function entry/exit. The prepare_enable() call is almost free
> > when the clk is already enabled, so it's not like we're adding a huge
> > overhead by doing that.  
> 
> So in the above snippet we should replace:
> 
> if (!cstate.enabled) {
> 	ret = clk_prepare_enable(imx->clk_per);
> 	if (ret)
> 		return ret;
> }
> 
> with
> 	ret = clk_prepare_enable(imx->clk_per);
> 	if (ret)
> 		return ret;
> 
> And 
> 
> if (cstate.enabled)
> 	clk_disable_unprepare(imx->clk_per);
> 
> with 
> 	clk_disable_unprepare(imx->clk_per);

That's what I had in mind.

> 
> >   
> > >   
> > > >>    
> > > >> >
> > > >> > My concern was more about the way PWM changes are applied
> > > >> > (->apply() returns before the change is actually applied), but
> > > >> > I agreed that it could be fixed later on (if other people
> > > >> > think it's really needed), since the existing code already
> > > >> > handles it this way.    
> > > >>
> > > >> This is the issue with FIFO setting - but for now we do not deal
> > > >> with it.    
> > > > 
> > > > Exactly.
> > > >     
> > > >>    
> > > >> >    
> > > >> > > No clear decision what to change until today when Stefan
> > > >> > > prepared separate (concise) patch (now I see what is the
> > > >> > > problem).   
> > > >> >
> > > >> > The patch proposed by Stefan is addressing a different
> > > >> > problem: the periph clock has to be enabled before accessing
> > > >> > registers.    
> > > >>
> > > >> So for this reason Stefan's patch [1] always enable the clock no
> > > >> matter if PWM clock is generated or not.    
> > > > 
> > > > Yes.
> > > >     
> > > >>    
> > > >> >    
> > > >> > >    
> > > >> > > >
> > > >> > > > Same goes for the regression introduced in patch 2: I
> > > >> > > > think it's better to keep things bisectable on all
> > > >> > > > platforms (even if it appeared to work by chance on imx7,
> > > >> > > > it did work before this change).    
> > > >> > >
> > > >> > > Could you be more specific about your idea to solve this
> > > >> > > problem?    
> > > >> >
> > > >> > Stefan already provided a patch, I just think it should be
> > > >> > fixed before patch 2 to avoid breaking bisectibility.    
> > > >>
> > > >> My idea is as follows:
> > > >>
> > > >> I will drop patch v2 (prepared by Sasha) and then squash
> > > >> Stefan's patch [1] to patch 7/11. The "old" ipg enable code will
> > > >> be removed with other not needed code during conversion.    
> > > > 
> > > > How about keeping patch 2 but enabling/disabling the periph clk
> > > > in imx_pwm_config() instead of completely dropping the
> > > > enable/disable clk sequence.
> > > > 
> > > > In patch 7 you just add the logic we talked about earlier:
> > > > unconditionally enable the periph clk when entering the
> > > > imx_pwm_apply_v2() function and disable it before leaving the
> > > > function.
> > > > 
> > > > This way you can preserve bisectibility and still get rid of the
> > > > ipg clk.
> > > > 
> > > > Stefan, what's your opinion?    
> > > 
> > > We will get rid of the ipg clocks anyway in patch 8 (which removes
> > > those functions completely).
> > > 
> > > So I think Lukasz approach should be fine, just drop patch 2 and
> > > squash my patch into patch 7.  
> > 
> > Well, the end result will be same (ipg_clk will be gone after patch
> > 8), but then it's hard to track why this clock suddenly disappeared.
> > I still think it's worth adding an extra commit explaining that
> > enabling the per_clk before accessing IP registers is needed on some
> > platforms (imx7), and that IPG clk is actually not required until we
> > start using it as a source for the PWM signal generation.
> > 
> > Maybe I'm the only one to think so. In this case, feel free to drop
> > patch 2.  
> 
> If you feel really bad about this issue, then we can drop patch 2 and:
> 
> reorganize patch 7/11 to 
>  - keep code, which adds imx_pwm_apply_v2() function code (just moves it
> as is) 
>  - remove .apply = imx_pwm_apply_v2 entry from pwm_ops structure.
> 
> 
> On top of it add patch to enable/disable unconditionally the
> imx->clk_per clock to avoid problems on imx7 (and state them in commit
> message).
> 
> Then we add separate patch with 
> .apply = imx_pwm_apply_v2 to pwm_ops structure to enable "new" atomic
> approach.
> 
> And at last we apply patch 8/11, which removes the code for old (non
> atomic) behaviour.
> 
> All the issues are documented in this way on the cost of having
> "dead" (I mean not used) imx_pwm_apply_v2() for two commits.
> 

This looks even more complicated.
Sorry, but I don't see the problem with modifying patch 2 to enable
per_clk instead of ipg_clk. Can you explain what's bothering you?

If you really want to do the change after patch 7, fine, but in this
case, keep the existing logic: enable/disable ipg_clk in
imx_pwm_apply_v2() until you drop the ipg_clk and replace the ipg_clk
enable/disable sequence by the equivalent enable/disable per_clk one.

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

[Lukasz Majewski]

Hi Boris,

> On Tue, 3 Jan 2017 23:01:11 +0100
> Lukasz Majewski <lukma@denx.de> wrote:
> 
> > Hi Boris, Stefan,
> > 
> > > On Tue, 03 Jan 2017 09:29:40 -0800
> > > Stefan Agner <stefan@agner.ch> wrote:
> > >   
> > > > On 2017-01-03 04:46, Boris Brezillon wrote:
> > > > <snip>  
> > > > >> > Well, regarding the imx_pwm_apply_v2() suggested by
> > > > >> > Stefan, I think we both agreed that most of the code was
> > > > >> > unneeded when all we want to do is disable the PWM.    
> > > > >>
> > > > >> So for the PATCH 7/11 we fix the issue with recalculating
> > > > >> clocks when we want to disable PWM.
> > > > >>
> > > > >> 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;
> > > > >>
> > > > >> 		/*
> > > > >> 		 * Enable the clock if the PWM is not already
> > > > >> 		 * enabled.
> > > > >> 		 */
> > > > >> 		if (!cstate.enabled) {
> > > > >> 			ret =
> > > > >> clk_prepare_enable(imx->clk_per); if (ret)
> > > > >> 			return ret;
> > > > >> 		}
> > > > >>
> > > > >> 		/*
> > > > >> 		 * Wait for a free FIFO slot if the PWM is
> > > > >> already
> > > > >> 		 * enabled, and flush the FIFO if the PWM was
> > > > >> disabled
> > > > >> 		 * and is about to be enabled.
> > > > >> 		 */
> > > > >> 		if (cstate.enabled)
> > > > >> 			imx_pwm_wait_fifo_slot(chip, pwm);
> > > > >> 		else
> > > > >> 			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);
> > > > >>
> > > > >> 		/* Disable the clock if the PWM is currently
> > > > >> enabled. */ if (cstate.enabled)
> > > > >> 			clk_disable_unprepare(imx->clk_per);
> > > > >> 	}
> > > > >>
> > > > >>    
> > > > > 
> > > > > Yep.
> > > > >     
> > > > 
> > > > This looks like a good transformation of the current Patch 7,
> > > > but once you merge my patch, it will look slightly
> > > > different...  
> > > 
> > > Yes. I think we should just unconditionally enable/disable the
> > > per_clk at function entry/exit. The prepare_enable() call is
> > > almost free when the clk is already enabled, so it's not like
> > > we're adding a huge overhead by doing that.  
> > 
> > So in the above snippet we should replace:
> > 
> > if (!cstate.enabled) {
> > 	ret = clk_prepare_enable(imx->clk_per);
> > 	if (ret)
> > 		return ret;
> > }
> > 
> > with
> > 	ret = clk_prepare_enable(imx->clk_per);
> > 	if (ret)
> > 		return ret;
> > 
> > And 
> > 
> > if (cstate.enabled)
> > 	clk_disable_unprepare(imx->clk_per);
> > 
> > with 
> > 	clk_disable_unprepare(imx->clk_per);
> 
> That's what I had in mind.

OK.

> 
> > 
> > >   
> > > >   
> > > > >>    
> > > > >> >
> > > > >> > My concern was more about the way PWM changes are applied
> > > > >> > (->apply() returns before the change is actually applied),
> > > > >> > but I agreed that it could be fixed later on (if other
> > > > >> > people think it's really needed), since the existing code
> > > > >> > already handles it this way.    
> > > > >>
> > > > >> This is the issue with FIFO setting - but for now we do not
> > > > >> deal with it.    
> > > > > 
> > > > > Exactly.
> > > > >     
> > > > >>    
> > > > >> >    
> > > > >> > > No clear decision what to change until today when Stefan
> > > > >> > > prepared separate (concise) patch (now I see what is the
> > > > >> > > problem).   
> > > > >> >
> > > > >> > The patch proposed by Stefan is addressing a different
> > > > >> > problem: the periph clock has to be enabled before
> > > > >> > accessing registers.    
> > > > >>
> > > > >> So for this reason Stefan's patch [1] always enable the
> > > > >> clock no matter if PWM clock is generated or not.    
> > > > > 
> > > > > Yes.
> > > > >     
> > > > >>    
> > > > >> >    
> > > > >> > >    
> > > > >> > > >
> > > > >> > > > Same goes for the regression introduced in patch 2: I
> > > > >> > > > think it's better to keep things bisectable on all
> > > > >> > > > platforms (even if it appeared to work by chance on
> > > > >> > > > imx7, it did work before this change).    
> > > > >> > >
> > > > >> > > Could you be more specific about your idea to solve this
> > > > >> > > problem?    
> > > > >> >
> > > > >> > Stefan already provided a patch, I just think it should be
> > > > >> > fixed before patch 2 to avoid breaking bisectibility.    
> > > > >>
> > > > >> My idea is as follows:
> > > > >>
> > > > >> I will drop patch v2 (prepared by Sasha) and then squash
> > > > >> Stefan's patch [1] to patch 7/11. The "old" ipg enable code
> > > > >> will be removed with other not needed code during
> > > > >> conversion.    
> > > > > 
> > > > > How about keeping patch 2 but enabling/disabling the periph
> > > > > clk in imx_pwm_config() instead of completely dropping the
> > > > > enable/disable clk sequence.
> > > > > 
> > > > > In patch 7 you just add the logic we talked about earlier:
> > > > > unconditionally enable the periph clk when entering the
> > > > > imx_pwm_apply_v2() function and disable it before leaving the
> > > > > function.
> > > > > 
> > > > > This way you can preserve bisectibility and still get rid of
> > > > > the ipg clk.
> > > > > 
> > > > > Stefan, what's your opinion?    
> > > > 
> > > > We will get rid of the ipg clocks anyway in patch 8 (which
> > > > removes those functions completely).
> > > > 
> > > > So I think Lukasz approach should be fine, just drop patch 2 and
> > > > squash my patch into patch 7.  
> > > 
> > > Well, the end result will be same (ipg_clk will be gone after
> > > patch 8), but then it's hard to track why this clock suddenly
> > > disappeared. I still think it's worth adding an extra commit
> > > explaining that enabling the per_clk before accessing IP
> > > registers is needed on some platforms (imx7), and that IPG clk is
> > > actually not required until we start using it as a source for the
> > > PWM signal generation.
> > > 
> > > Maybe I'm the only one to think so. In this case, feel free to
> > > drop patch 2.  
> > 
> > If you feel really bad about this issue, then we can drop patch 2
> > and:
> > 
> > reorganize patch 7/11 to 
> >  - keep code, which adds imx_pwm_apply_v2() function code (just
> > moves it as is) 
> >  - remove .apply = imx_pwm_apply_v2 entry from pwm_ops structure.
> > 
> > 
> > On top of it add patch to enable/disable unconditionally the
> > imx->clk_per clock to avoid problems on imx7 (and state them in
> > commit message).
> > 
> > Then we add separate patch with 
> > .apply = imx_pwm_apply_v2 to pwm_ops structure to enable "new"
> > atomic approach.
> > 
> > And at last we apply patch 8/11, which removes the code for old (non
> > atomic) behaviour.
> > 
> > All the issues are documented in this way on the cost of having
> > "dead" (I mean not used) imx_pwm_apply_v2() for two commits.
> > 
> 
> This looks even more complicated.
> Sorry, but I don't see the problem with modifying patch 2 to enable
> per_clk instead of ipg_clk. Can you explain what's bothering you?

But in patch 2:
"pwm: imx: remove ipg clock"

 we _remove_ the clk_ipg from imx_pwm_config() and imx_pwm_probe(), so
 I'm quite puzzled with your above statement.

> 
> If you really want to do the change after patch 7, fine, but in this
> case, keep the existing logic: enable/disable ipg_clk in
> imx_pwm_apply_v2() until you drop the ipg_clk and replace the ipg_clk
> enable/disable sequence by the equivalent enable/disable per_clk one.
> 

Frankly, I do agree with Stefan here - we should drop patch 2, squash
all changes (including imx7 clock issues) to patch 7 (including verbose
commit message) and remove the non-atomic code in patch 8.


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 v3 RESEND 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Boris Brezillon]

On Tue, 3 Jan 2017 23:46:58 +0100
Lukasz Majewski <lukma@denx.de> wrote:

> > > > > >> > > > Same goes for the regression introduced in patch 2: I
> > > > > >> > > > think it's better to keep things bisectable on all
> > > > > >> > > > platforms (even if it appeared to work by chance on
> > > > > >> > > > imx7, it did work before this change).      
> > > > > >> > >
> > > > > >> > > Could you be more specific about your idea to solve this
> > > > > >> > > problem?      
> > > > > >> >
> > > > > >> > Stefan already provided a patch, I just think it should be
> > > > > >> > fixed before patch 2 to avoid breaking bisectibility.      
> > > > > >>
> > > > > >> My idea is as follows:
> > > > > >>
> > > > > >> I will drop patch v2 (prepared by Sasha) and then squash
> > > > > >> Stefan's patch [1] to patch 7/11. The "old" ipg enable code
> > > > > >> will be removed with other not needed code during
> > > > > >> conversion.      
> > > > > > 
> > > > > > How about keeping patch 2 but enabling/disabling the periph
> > > > > > clk in imx_pwm_config() instead of completely dropping the
> > > > > > enable/disable clk sequence.
> > > > > > 
> > > > > > In patch 7 you just add the logic we talked about earlier:
> > > > > > unconditionally enable the periph clk when entering the
> > > > > > imx_pwm_apply_v2() function and disable it before leaving the
> > > > > > function.
> > > > > > 
> > > > > > This way you can preserve bisectibility and still get rid of
> > > > > > the ipg clk.
> > > > > > 
> > > > > > Stefan, what's your opinion?      
> > > > > 
> > > > > We will get rid of the ipg clocks anyway in patch 8 (which
> > > > > removes those functions completely).
> > > > > 
> > > > > So I think Lukasz approach should be fine, just drop patch 2 and
> > > > > squash my patch into patch 7.    
> > > > 
> > > > Well, the end result will be same (ipg_clk will be gone after
> > > > patch 8), but then it's hard to track why this clock suddenly
> > > > disappeared. I still think it's worth adding an extra commit
> > > > explaining that enabling the per_clk before accessing IP
> > > > registers is needed on some platforms (imx7), and that IPG clk is
> > > > actually not required until we start using it as a source for the
> > > > PWM signal generation.
> > > > 
> > > > Maybe I'm the only one to think so. In this case, feel free to
> > > > drop patch 2.    
> > > 
> > > If you feel really bad about this issue, then we can drop patch 2
> > > and:
> > > 
> > > reorganize patch 7/11 to 
> > >  - keep code, which adds imx_pwm_apply_v2() function code (just
> > > moves it as is) 
> > >  - remove .apply = imx_pwm_apply_v2 entry from pwm_ops structure.
> > > 
> > > 
> > > On top of it add patch to enable/disable unconditionally the
> > > imx->clk_per clock to avoid problems on imx7 (and state them in
> > > commit message).
> > > 
> > > Then we add separate patch with 
> > > .apply = imx_pwm_apply_v2 to pwm_ops structure to enable "new"
> > > atomic approach.
> > > 
> > > And at last we apply patch 8/11, which removes the code for old (non
> > > atomic) behaviour.
> > > 
> > > All the issues are documented in this way on the cost of having
> > > "dead" (I mean not used) imx_pwm_apply_v2() for two commits.
> > >   
> > 
> > This looks even more complicated.
> > Sorry, but I don't see the problem with modifying patch 2 to enable
> > per_clk instead of ipg_clk. Can you explain what's bothering you?  
> 
> But in patch 2:
> "pwm: imx: remove ipg clock"
> 
>  we _remove_ the clk_ipg from imx_pwm_config() and imx_pwm_probe(), so
>  I'm quite puzzled with your above statement.

See my reworked version below.

> 
> > 
> > If you really want to do the change after patch 7, fine, but in this
> > case, keep the existing logic: enable/disable ipg_clk in
> > imx_pwm_apply_v2() until you drop the ipg_clk and replace the ipg_clk
> > enable/disable sequence by the equivalent enable/disable per_clk one.
> >   
> 
> Frankly, I do agree with Stefan here - we should drop patch 2, squash
> all changes (including imx7 clock issues) to patch 7 (including verbose
> commit message) and remove the non-atomic code in patch 8.

Hm, this is not like I'm asking something impossible here (see the
following patch).

--->8---
>From c79bb872a40b8e322fd13f33f374fb1ba085e7a9 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Mon, 26 Dec 2016 23:55:52 +0100
Subject: [PATCH v4] pwm: imx: remove ipg clock and enable per clock when required

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>
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 d600fd5cd4ba..b1d1e50d3956 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.7.4

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

[Lukasz Majewski]

Hi Boris,

> On Tue, 3 Jan 2017 23:46:58 +0100
> Lukasz Majewski <lukma@denx.de> wrote:
> 
> > > > > > >> > > > Same goes for the regression introduced in patch
> > > > > > >> > > > 2: I think it's better to keep things bisectable
> > > > > > >> > > > on all platforms (even if it appeared to work by
> > > > > > >> > > > chance on imx7, it did work before this
> > > > > > >> > > > change).      
> > > > > > >> > >
> > > > > > >> > > Could you be more specific about your idea to solve
> > > > > > >> > > this problem?      
> > > > > > >> >
> > > > > > >> > Stefan already provided a patch, I just think it
> > > > > > >> > should be fixed before patch 2 to avoid breaking
> > > > > > >> > bisectibility.      
> > > > > > >>
> > > > > > >> My idea is as follows:
> > > > > > >>
> > > > > > >> I will drop patch v2 (prepared by Sasha) and then squash
> > > > > > >> Stefan's patch [1] to patch 7/11. The "old" ipg enable
> > > > > > >> code will be removed with other not needed code during
> > > > > > >> conversion.      
> > > > > > > 
> > > > > > > How about keeping patch 2 but enabling/disabling the
> > > > > > > periph clk in imx_pwm_config() instead of completely
> > > > > > > dropping the enable/disable clk sequence.
> > > > > > > 
> > > > > > > In patch 7 you just add the logic we talked about earlier:
> > > > > > > unconditionally enable the periph clk when entering the
> > > > > > > imx_pwm_apply_v2() function and disable it before leaving
> > > > > > > the function.
> > > > > > > 
> > > > > > > This way you can preserve bisectibility and still get rid
> > > > > > > of the ipg clk.
> > > > > > > 
> > > > > > > Stefan, what's your opinion?      
> > > > > > 
> > > > > > We will get rid of the ipg clocks anyway in patch 8 (which
> > > > > > removes those functions completely).
> > > > > > 
> > > > > > So I think Lukasz approach should be fine, just drop patch
> > > > > > 2 and squash my patch into patch 7.    
> > > > > 
> > > > > Well, the end result will be same (ipg_clk will be gone after
> > > > > patch 8), but then it's hard to track why this clock suddenly
> > > > > disappeared. I still think it's worth adding an extra commit
> > > > > explaining that enabling the per_clk before accessing IP
> > > > > registers is needed on some platforms (imx7), and that IPG
> > > > > clk is actually not required until we start using it as a
> > > > > source for the PWM signal generation.
> > > > > 
> > > > > Maybe I'm the only one to think so. In this case, feel free to
> > > > > drop patch 2.    
> > > > 
> > > > If you feel really bad about this issue, then we can drop patch
> > > > 2 and:
> > > > 
> > > > reorganize patch 7/11 to 
> > > >  - keep code, which adds imx_pwm_apply_v2() function code (just
> > > > moves it as is) 
> > > >  - remove .apply = imx_pwm_apply_v2 entry from pwm_ops
> > > > structure.
> > > > 
> > > > 
> > > > On top of it add patch to enable/disable unconditionally the
> > > > imx->clk_per clock to avoid problems on imx7 (and state them in
> > > > commit message).
> > > > 
> > > > Then we add separate patch with 
> > > > .apply = imx_pwm_apply_v2 to pwm_ops structure to enable "new"
> > > > atomic approach.
> > > > 
> > > > And at last we apply patch 8/11, which removes the code for old
> > > > (non atomic) behaviour.
> > > > 
> > > > All the issues are documented in this way on the cost of having
> > > > "dead" (I mean not used) imx_pwm_apply_v2() for two commits.
> > > >   
> > > 
> > > This looks even more complicated.
> > > Sorry, but I don't see the problem with modifying patch 2 to
> > > enable per_clk instead of ipg_clk. Can you explain what's
> > > bothering you?  
> > 
> > But in patch 2:
> > "pwm: imx: remove ipg clock"
> > 
> >  we _remove_ the clk_ipg from imx_pwm_config() and imx_pwm_probe(),
> > so I'm quite puzzled with your above statement.
> 
> See my reworked version below.
> 
> > 
> > > 
> > > If you really want to do the change after patch 7, fine, but in
> > > this case, keep the existing logic: enable/disable ipg_clk in
> > > imx_pwm_apply_v2() until you drop the ipg_clk and replace the
> > > ipg_clk enable/disable sequence by the equivalent enable/disable
> > > per_clk one. 
> > 
> > Frankly, I do agree with Stefan here - we should drop patch 2,
> > squash all changes (including imx7 clock issues) to patch 7
> > (including verbose commit message) and remove the non-atomic code
> > in patch 8.
> 
> Hm, this is not like I'm asking something impossible here (see the
> following patch).
> 
> --->8---
> From c79bb872a40b8e322fd13f33f374fb1ba085e7a9 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Mon, 26 Dec 2016 23:55:52 +0100
> Subject: [PATCH v4] pwm: imx: remove ipg clock and enable per clock
> when required
> 
> 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>
> 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 d600fd5cd4ba..b1d1e50d3956 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;

Patch seems OK, so I will test it latter today including rework of
patch 7 (do not recalculate things when enabling PWM and the peripheral
clock availability.

Thanks Boris for preparing the patch.


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

[PATCH v3 RESEND 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 v3:
- None

Changes for v2:
- None
---
 drivers/pwm/pwm-imx.c | 118 --------------------------------------------------
 1 file changed, 118 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index cd53c05..b636526 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)
@@ -228,109 +224,6 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
-static int imx_pwm_config_v2(struct pwm_chip *chip,
-		struct pwm_device *pwm, int duty_ns, int period_ns)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	unsigned long long c;
-	unsigned long period_cycles, duty_cycles, prescale;
-	bool enable = pwm_is_enabled(pwm);
-	u32 cr;
-
-	/*
-	 * i.MX PWMv2 has a 4-word sample FIFO.
-	 * In order to avoid FIFO overflow issue, we do software reset
-	 * to clear all sample FIFO if the controller is disabled or
-	 * wait for a full PWM cycle to get a relinquished FIFO slot
-	 * when the controller is enabled and the FIFO is fully loaded.
-	 */
-	if (enable)
-		imx_pwm_wait_fifo_slot(chip, pwm);
-	else
-		imx_pwm_sw_reset(chip);
-
-	c = clk_get_rate(imx->clk_per);
-	c = c * period_ns;
-	do_div(c, 1000000000);
-	period_cycles = c;
-
-	prescale = period_cycles / 0x10000 + 1;
-
-	period_cycles /= prescale;
-	c = (unsigned long long)period_cycles * duty_ns;
-	do_div(c, period_ns);
-	duty_cycles = c;
-
-	/*
-	 * according to imx pwm RM, the real period value should be
-	 * PERIOD value in PWMPR plus 2.
-	 */
-	if (period_cycles > 2)
-		period_cycles -= 2;
-	else
-		period_cycles = 0;
-
-	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
-	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
-
-	cr = MX3_PWMCR_PRESCALER(prescale) |
-		MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
-		MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
-
-	if (enable)
-		cr |= MX3_PWMCR_EN;
-
-	writel(cr, imx->mmio_base + MX3_PWMCR);
-
-	return 0;
-}
-
-static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	u32 val;
-
-	val = readl(imx->mmio_base + MX3_PWMCR);
-
-	if (enable)
-		val |= MX3_PWMCR_EN;
-	else
-		val &= ~MX3_PWMCR_EN;
-
-	writel(val, imx->mmio_base + MX3_PWMCR);
-}
-
-static int imx_pwm_config(struct pwm_chip *chip,
-		struct pwm_device *pwm, int duty_ns, int period_ns)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-
-	return imx->config(chip, pwm, duty_ns, period_ns);
-}
-
-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,
@@ -339,17 +232,11 @@ static struct pwm_ops imx_pwm_ops_v1 = {
 };
 
 static struct pwm_ops imx_pwm_ops_v2 = {
-	.enable = imx_pwm_enable,
-	.disable = imx_pwm_disable,
-	.config = imx_pwm_config,
 	.apply = imx_pwm_apply_v2,
 	.owner = THIS_MODULE,
 };
 
 struct imx_pwm_data {
-	int (*config)(struct pwm_chip *chip,
-		struct pwm_device *pwm, int duty_ns, int period_ns);
-	void (*set_enable)(struct pwm_chip *chip, bool enable);
 	struct pwm_ops *pwm_ops;
 };
 
@@ -358,8 +245,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,
 };
 
@@ -406,9 +291,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 v3 RESEND 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 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 v3 RESEND 10/11] pwm: imx: doc: Update imx-pwm.txt documentation entry

[Lukasz Majewski]

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 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 v3 RESEND 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 v3:
- None

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

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index b636526..03b01a4 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)
@@ -215,6 +216,9 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (state->enabled)
 		cr |= MX3_PWMCR_EN;
 
+	if (state->polarity == PWM_POLARITY_INVERSED)
+		cr |= MX3_PWMCR_POUTC;
+
 	writel(cr, imx->mmio_base + MX3_PWMCR);
 
 	/* Disable the clock if the PWM is being disabled. */
@@ -237,6 +241,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
 };
 
 struct imx_pwm_data {
+	bool polarity_supported;
 	struct pwm_ops *pwm_ops;
 };
 
@@ -245,6 +250,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,
 };
 
@@ -285,6 +291,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