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

[PATCH v3 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-rc3 kernel SHA1: a909d3e636995ba7c349e2ca5dbb528154d4ac30


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

[Philipp Zabel]

Am Dienstag, den 01.11.2016, 08:10 +0100 schrieb 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>

I don't remember the details, but since I had only worked with i.MX53
and i.MX6 at the time, and Sascha now verified that the i.MX53 PWM
registers can in fact be accessed with the pwmX_ipg_clk bits gated, I
can only assume that this patch is the result of a misinterpretation of
the i.MX53 technical reference manual: Contrary to the i.MX6 TRM it does
not mention the ungated peripheral access clock (ipg_clk_s) at all, but
calls the gated ipg_clk "block interface clock" in Table 18-3.

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

regards
Philipp

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

[Stefan Agner]

On 2016-11-01 00:10, 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.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>

I have to NACK here, sorry guys.

Just tested this on a i.MX 7, the kernel freezes in imx_pwm_config, I
guess that is where the code accesses a register first.

The i.MX 7 DT (imx7s.dtsi) specifies the same clock for ipg and per, but
it seems that this clock is crucial for register access on i.MX 7:

clocks = <&clks IMX7D_PWM1_ROOT_CLK>,
         <&clks IMX7D_PWM1_ROOT_CLK>;
clock-names = "ipg", "per";          

So since the "per" clock is the same in the i.MX 7 case, imx_pwm_enable
worked...

I agree that the old code is a bit weird, especially that we get the
clock in imx_pwm_enable. It seems that all device trees specify a "ipg"
clock, so I guess we can get the clock at probe time for all variants of
this IP and just enable it on peripheral access...

--
Stefan


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

[Boris Brezillon]

On Tue, 22 Nov 2016 13:04:11 -0800
Stefan Agner <stefan@agner.ch> wrote:

> On 2016-11-01 00:10, 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.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>  
> 
> I have to NACK here, sorry guys.
> 
> Just tested this on a i.MX 7, the kernel freezes in imx_pwm_config, I
> guess that is where the code accesses a register first.
> 
> The i.MX 7 DT (imx7s.dtsi) specifies the same clock for ipg and per, but
> it seems that this clock is crucial for register access on i.MX 7:
> 
> clocks = <&clks IMX7D_PWM1_ROOT_CLK>,
>          <&clks IMX7D_PWM1_ROOT_CLK>;
> clock-names = "ipg", "per";          
> 
> So since the "per" clock is the same in the i.MX 7 case, imx_pwm_enable
> worked...
> 
> I agree that the old code is a bit weird, especially that we get the
> clock in imx_pwm_enable. It seems that all device trees specify a "ipg"
> clock, so I guess we can get the clock at probe time for all variants of
> this IP and just enable it on peripheral access...

Or, we patch the code to take the per clk before accessing PWM regs,
and release it once we're done.
AFAIU, the IPG clock is only supposed to be enabled when the PWM takes
its sources from the IPG channel, it has nothing to do we register
accesses. If this is correct, then doing what you suggest implies
abusing the IPG clk meaning.

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

[Lukasz Majewski]

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

Hi Boris, Stefan,

> On Tue, 22 Nov 2016 13:04:11 -0800
> Stefan Agner <stefan@agner.ch> wrote:
> 
> > On 2016-11-01 00:10, 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.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > Cc: Philipp Zabel <p.zabel@pengutronix.de>  
> > 
> > I have to NACK here, sorry guys.
> > 
> > Just tested this on a i.MX 7, the kernel freezes in imx_pwm_config,
> > I guess that is where the code accesses a register first.
> > 
> > The i.MX 7 DT (imx7s.dtsi) specifies the same clock for ipg and
> > per, but it seems that this clock is crucial for register access on
> > i.MX 7:
> > 
> > clocks = <&clks IMX7D_PWM1_ROOT_CLK>,
> >          <&clks IMX7D_PWM1_ROOT_CLK>;
> > clock-names = "ipg", "per";          
> > 
> > So since the "per" clock is the same in the i.MX 7 case,
> > imx_pwm_enable worked...
> > 
> > I agree that the old code is a bit weird, especially that we get the
> > clock in imx_pwm_enable. It seems that all device trees specify a
> > "ipg" clock, so I guess we can get the clock at probe time for all
> > variants of this IP and just enable it on peripheral access...
> 
> Or, we patch the code to take the per clk before accessing PWM regs,
> and release it once we're done.
> AFAIU, the IPG clock is only supposed to be enabled when the PWM takes
> its sources from the IPG channel, 

+1

That is what TRM says about this clock.

> it has nothing to do we register
> accesses. If this is correct, then doing what you suggest implies
> abusing the IPG clk meaning.

+1

Best regards,
Łukasz Majewski

> 
> > 
> > --
> > Stefan
> > 
> > 
> > > ---
> > > [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 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 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>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
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 | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index d594501..8497902 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -64,8 +64,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
@@ -83,6 +81,7 @@ 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;
 	writel(max - p, imx->mmio_base + MX1_PWMS);
@@ -90,19 +89,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 +245,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 +266,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 v3 04/11] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Stefan Agner]

On 2016-11-01 00:10, 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>
> ---
> 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 | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d594501..8497902 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -64,8 +64,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
> @@ -83,6 +81,7 @@ 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);

This change is unnecessary.

It is also common to have declarations at the beginning of the function
and separated with a newline from code.

But otherwise looks good to me:

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

--
Stefan

>  	u32 max = readl(imx->mmio_base + MX1_PWMP);
>  	u32 p = max * duty_ns / period_ns;
>  	writel(max - p, imx->mmio_base + MX1_PWMS);
> @@ -90,19 +89,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 +245,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 +266,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,
>  };

[PATCH v3 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>
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

Re: [PATCH v3 05/11] pwm: imx: Move PWMv2 software reset code to a separate function

[Stefan Agner]

On 2016-11-01 00:10, Lukasz Majewski 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>
> 

Looks good to me:

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

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

[PATCH v3 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>
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

Re: [PATCH v3 06/11] pwm: imx: Move PWMv2 wait for fifo slot code to a separate function

[Stefan Agner]

On 2016-11-01 00:10, Lukasz Majewski 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: Stefan Agner <stefan@agner.ch>

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

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

[Stefan Agner]

On 2016-11-01 00:10, Lukasz Majewski 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);
> +

Couldn't we do:

if (cstate.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 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;

} else if (state->enabled) {
	imx_pwm_sw_reset(chip);
}

and get rid of the if (state->enabled) in between? This would safe us
useless recalculation when disabling the controller...

--
Stefan

> +
> +	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,
>  };

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

[Boris Brezillon]

On Tue, 22 Nov 2016 13:55:33 -0800
Stefan Agner <stefan@agner.ch> wrote:

> On 2016-11-01 00:10, Lukasz Majewski 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);
> > +  
> 
> Couldn't we do:
> 
> if (cstate.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 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;  
> 
> } else if (state->enabled) {
> 	imx_pwm_sw_reset(chip);
> }
> 
> and get rid of the if (state->enabled) in between? This would safe us
> useless recalculation when disabling the controller...

I get your point, but I'm pretty sure your proposal does not do what
you want (remember that cstate is the current state, and state is the
new state to apply).

Something like that would work better:

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


This being said, I'm a bit concerned by the way this driver handles PWM
config requests.
It seems that the new config request is queued, and nothing guarantees
that it is actually applied when the pwm_apply/config/enable/disable()
functions return.

This approach has several flaws IMO:

1/ I'm not sure this is what the PWM users expect. Getting your request
   queued with no guarantees that it is applied can be weird in some
   cases (especially when the user changes the PWM config several times
   in a short period of time).
2/ In the disable path, you queue a "stop PWM" request, but you're not
   sure that it's actually dequeued before the per clk is disabled.
   What happens in that case? And more importantly, what happens when
   the PWM is re-enabled to apply a new config? AFAICS, there might be
   a short period of time where the re-enabled PWM is actually running
   with the old config until we flush the command queue and queue a new
   command.
3/ The queueing approach complicates the whole logic. You have to
   flush the FIFO in some cases, or wait for an empty slots if too many
   commands are queued.
   Forcing imx_pwm_apply_v2() to wait for the config request to be
   applied should simplify the whole thing, because you will always be
   guaranteed that the FIFO is empty, and that the current
   configuration is the last requested one.

Regards,

Boris

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

[Stefan Agner]

On 2016-11-23 00:38, Boris Brezillon wrote:
> On Tue, 22 Nov 2016 13:55:33 -0800
> Stefan Agner <stefan@agner.ch> wrote:
> 
>> On 2016-11-01 00:10, Lukasz Majewski 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);
>> > +
>>
>> Couldn't we do:
>>
>> if (cstate.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 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;
>>
>> } else if (state->enabled) {
>> 	imx_pwm_sw_reset(chip);
>> }
>>
>> and get rid of the if (state->enabled) in between? This would safe us
>> useless recalculation when disabling the controller...
> 
> I get your point, but I'm pretty sure your proposal does not do what
> you want (remember that cstate is the current state, and state is the
> new state to apply).
> 
> Something like that would work better:
> 
> 	if (state->enabled) {

Oops, yes, got that wrong. state->enabled is what I meant.

> 		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);
> 	}
> 
> 
> This being said, I'm a bit concerned by the way this driver handles PWM
> config requests.
> It seems that the new config request is queued, and nothing guarantees

Not sure if that is true. The RM says: "A change in the period value due
to a write in PWM_PWMPR results in the counter being reset to zero and
the start of a new count period."

And for PWMSAR: "When a new value is written, the duty cycle changes
after the current period is over."

So I guess writing the period basically makes sure the next value from
PWMSAR will be active immediately...


> that it is actually applied when the pwm_apply/config/enable/disable()
> functions return.


Given that the driver did it like that since quite some time, I am
assuming it mostly works in practice. 

I would rather prefer to do that conversion to atomic PWM API now, and
fix that in a second step...

> 
> This approach has several flaws IMO:
> 
> 1/ I'm not sure this is what the PWM users expect. Getting your request
>    queued with no guarantees that it is applied can be weird in some
>    cases (especially when the user changes the PWM config several times
>    in a short period of time).
> 2/ In the disable path, you queue a "stop PWM" request, but you're not
>    sure that it's actually dequeued before the per clk is disabled.
>    What happens in that case? And more importantly, what happens when
>    the PWM is re-enabled to apply a new config? AFAICS, there might be
>    a short period of time where the re-enabled PWM is actually running
>    with the old config until we flush the command queue and queue a new
>    command.
> 3/ The queueing approach complicates the whole logic. You have to
>    flush the FIFO in some cases, or wait for an empty slots if too many
>    commands are queued.
>    Forcing imx_pwm_apply_v2() to wait for the config request to be
>    applied should simplify the whole thing, because you will always be
>    guaranteed that the FIFO is empty, and that the current
>    configuration is the last requested one.
> 

--
Stefan

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

[Lukasz Majewski]

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

Dear Stefan, Boris,

> On 2016-11-23 00:38, Boris Brezillon wrote:
> > On Tue, 22 Nov 2016 13:55:33 -0800
> > Stefan Agner <stefan@agner.ch> wrote:
> > 
> >> On 2016-11-01 00:10, Lukasz Majewski 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);
> >> > +
> >>
> >> Couldn't we do:
> >>
> >> if (cstate.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 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;
> >>
> >> } else if (state->enabled) {
> >> 	imx_pwm_sw_reset(chip);
> >> }
> >>
> >> and get rid of the if (state->enabled) in between? This would safe
> >> us useless recalculation when disabling the controller...
> > 
> > I get your point, but I'm pretty sure your proposal does not do what
> > you want (remember that cstate is the current state, and state is
> > the new state to apply).
> > 
> > Something like that would work better:
> > 
> > 	if (state->enabled) {
> 
> Oops, yes, got that wrong. state->enabled is what I meant.
> 
> > 		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);
> > 	}
> > 
> > 
> > This being said, I'm a bit concerned by the way this driver handles
> > PWM config requests.
> > It seems that the new config request is queued, and nothing
> > guarantees
> 
> Not sure if that is true. The RM says: "A change in the period value
> due to a write in PWM_PWMPR results in the counter being reset to
> zero and the start of a new count period."
> 
> And for PWMSAR: "When a new value is written, the duty cycle changes
> after the current period is over."
> 
> So I guess writing the period basically makes sure the next value from
> PWMSAR will be active immediately...
> 
> 
> > that it is actually applied when the
> > pwm_apply/config/enable/disable() functions return.
> 
> 
> Given that the driver did it like that since quite some time, I am
> assuming it mostly works in practice. 
> 
> I would rather prefer to do that conversion to atomic PWM API now, and
> fix that in a second step...

I'm also for fixing one problem in a time. The "PWM ->apply()" set of
patches now tries to fix all problems in IMX PWM driver.

Could we agree on the scope of this work? I mean what should be
included to "->apply()" rework and what will be fixed latter?

Frankly, I think that this patch series comes to the point where it is
not manageable anymore.

Please also keep in mind that I do have iMX6q system, Stefan has imx7
and Sasha has HW with PWMv1 working.

Changing the driver in N different places not related to the
"->apply()" atomicity support (the ipg clock, FIFO) requires far more
work and testing.


Best regards,
Łukasz Majewski

> 
> > 
> > This approach has several flaws IMO:
> > 
> > 1/ I'm not sure this is what the PWM users expect. Getting your
> > request queued with no guarantees that it is applied can be weird
> > in some cases (especially when the user changes the PWM config
> > several times in a short period of time).
> > 2/ In the disable path, you queue a "stop PWM" request, but you're
> > not sure that it's actually dequeued before the per clk is disabled.
> >    What happens in that case? And more importantly, what happens
> > when the PWM is re-enabled to apply a new config? AFAICS, there
> > might be a short period of time where the re-enabled PWM is
> > actually running with the old config until we flush the command
> > queue and queue a new command.
> > 3/ The queueing approach complicates the whole logic. You have to
> >    flush the FIFO in some cases, or wait for an empty slots if too
> > many commands are queued.
> >    Forcing imx_pwm_apply_v2() to wait for the config request to be
> >    applied should simplify the whole thing, because you will always
> > be guaranteed that the FIFO is empty, and that the current
> >    configuration is the last requested one.
> > 
> 
> --
> Stefan


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

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

[Boris Brezillon]

On Mon, 28 Nov 2016 06:50:31 +0100
Lukasz Majewski <l.majewski@majess.pl> wrote:

> Dear Stefan, Boris,
> 
> > On 2016-11-23 00:38, Boris Brezillon wrote:  
> > > On Tue, 22 Nov 2016 13:55:33 -0800
> > > Stefan Agner <stefan@agner.ch> wrote:
> > >   
> > >> On 2016-11-01 00:10, Lukasz Majewski 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);
> > >> > +  
> > >>
> > >> Couldn't we do:
> > >>
> > >> if (cstate.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 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;  
> > >>
> > >> } else if (state->enabled) {
> > >> 	imx_pwm_sw_reset(chip);
> > >> }
> > >>
> > >> and get rid of the if (state->enabled) in between? This would safe
> > >> us useless recalculation when disabling the controller...  
> > > 
> > > I get your point, but I'm pretty sure your proposal does not do what
> > > you want (remember that cstate is the current state, and state is
> > > the new state to apply).
> > > 
> > > Something like that would work better:
> > > 
> > > 	if (state->enabled) {  
> > 
> > Oops, yes, got that wrong. state->enabled is what I meant.
> >   
> > > 		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);
> > > 	}
> > > 
> > > 
> > > This being said, I'm a bit concerned by the way this driver handles
> > > PWM config requests.
> > > It seems that the new config request is queued, and nothing
> > > guarantees  
> > 
> > Not sure if that is true. The RM says: "A change in the period value
> > due to a write in PWM_PWMPR results in the counter being reset to
> > zero and the start of a new count period."
> > 
> > And for PWMSAR: "When a new value is written, the duty cycle changes
> > after the current period is over."
> > 
> > So I guess writing the period basically makes sure the next value from
> > PWMSAR will be active immediately...
> > 
> >   
> > > that it is actually applied when the
> > > pwm_apply/config/enable/disable() functions return.  
> > 
> > 
> > Given that the driver did it like that since quite some time, I am
> > assuming it mostly works in practice. 
> > 
> > I would rather prefer to do that conversion to atomic PWM API now, and
> > fix that in a second step...  
> 
> I'm also for fixing one problem in a time. The "PWM ->apply()" set of
> patches now tries to fix all problems in IMX PWM driver.
> 
> Could we agree on the scope of this work? I mean what should be
> included to "->apply()" rework and what will be fixed latter?

I never asked to fix that in this series ;-), I was just pointing the
weird behavior of the existing code.

Let's focus on the atomic support for now.

> 
> Frankly, I think that this patch series comes to the point where it is
> not manageable anymore.
> 
> Please also keep in mind that I do have iMX6q system, Stefan has imx7
> and Sasha has HW with PWMv1 working.
> 
> Changing the driver in N different places not related to the
> "->apply()" atomicity support (the ipg clock, FIFO) requires far more
> work and testing.
> 
> 
> Best regards,
> Łukasz Majewski
> 
> >   
> > > 
> > > This approach has several flaws IMO:
> > > 
> > > 1/ I'm not sure this is what the PWM users expect. Getting your
> > > request queued with no guarantees that it is applied can be weird
> > > in some cases (especially when the user changes the PWM config
> > > several times in a short period of time).
> > > 2/ In the disable path, you queue a "stop PWM" request, but you're
> > > not sure that it's actually dequeued before the per clk is disabled.
> > >    What happens in that case? And more importantly, what happens
> > > when the PWM is re-enabled to apply a new config? AFAICS, there
> > > might be a short period of time where the re-enabled PWM is
> > > actually running with the old config until we flush the command
> > > queue and queue a new command.
> > > 3/ The queueing approach complicates the whole logic. You have to
> > >    flush the FIFO in some cases, or wait for an empty slots if too
> > > many commands are queued.
> > >    Forcing imx_pwm_apply_v2() to wait for the config request to be
> > >    applied should simplify the whole thing, because you will always
> > > be guaranteed that the FIFO is empty, and that the current
> > >    configuration is the last requested one.
> > >   
> > 
> > --
> > Stefan  
> 

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

[Lukasz Majewski]

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

Dear Boris, Stefan,

> On Mon, 28 Nov 2016 06:50:31 +0100
> Lukasz Majewski <l.majewski@majess.pl> wrote:
> 
> > Dear Stefan, Boris,
> > 
> > > On 2016-11-23 00:38, Boris Brezillon wrote:  
> > > > On Tue, 22 Nov 2016 13:55:33 -0800
> > > > Stefan Agner <stefan@agner.ch> wrote:
> > > >   
> > > >> On 2016-11-01 00:10, Lukasz Majewski 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);
> > > >> > +  
> > > >>
> > > >> Couldn't we do:
> > > >>
> > > >> if (cstate.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 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;  
> > > >>
> > > >> } else if (state->enabled) {
> > > >> 	imx_pwm_sw_reset(chip);
> > > >> }
> > > >>
> > > >> and get rid of the if (state->enabled) in between? This would
> > > >> safe us useless recalculation when disabling the
> > > >> controller...  
> > > > 
> > > > I get your point, but I'm pretty sure your proposal does not do
> > > > what you want (remember that cstate is the current state, and
> > > > state is the new state to apply).
> > > > 
> > > > Something like that would work better:
> > > > 
> > > > 	if (state->enabled) {  
> > > 
> > > Oops, yes, got that wrong. state->enabled is what I meant.
> > >   
> > > > 		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);
> > > > 	}
> > > > 
> > > > 
> > > > This being said, I'm a bit concerned by the way this driver
> > > > handles PWM config requests.
> > > > It seems that the new config request is queued, and nothing
> > > > guarantees  
> > > 
> > > Not sure if that is true. The RM says: "A change in the period
> > > value due to a write in PWM_PWMPR results in the counter being
> > > reset to zero and the start of a new count period."
> > > 
> > > And for PWMSAR: "When a new value is written, the duty cycle
> > > changes after the current period is over."
> > > 
> > > So I guess writing the period basically makes sure the next value
> > > from PWMSAR will be active immediately...
> > > 
> > >   
> > > > that it is actually applied when the
> > > > pwm_apply/config/enable/disable() functions return.  
> > > 
> > > 
> > > Given that the driver did it like that since quite some time, I am
> > > assuming it mostly works in practice. 
> > > 
> > > I would rather prefer to do that conversion to atomic PWM API
> > > now, and fix that in a second step...  
> > 
> > I'm also for fixing one problem in a time. The "PWM ->apply()" set
> > of patches now tries to fix all problems in IMX PWM driver.
> > 
> > Could we agree on the scope of this work? I mean what should be
> > included to "->apply()" rework and what will be fixed latter?
> 
> I never asked to fix that in this series ;-), I was just pointing the
> weird behavior of the existing code.
> 
> Let's focus on the atomic support for now.

So Boris, you don't have any comments to the atomic support patches? :-)

Stefan, do you require to change the ipg stuff in the atomic series or
could it be done as a subsequent patch?

If you don't have any more questions, I will prepare next patch
iteration according to Stefan comments.

Best regards,
Łukasz Majewski

> 
> > 
> > Frankly, I think that this patch series comes to the point where it
> > is not manageable anymore.
> > 
> > Please also keep in mind that I do have iMX6q system, Stefan has
> > imx7 and Sasha has HW with PWMv1 working.
> > 
> > Changing the driver in N different places not related to the
> > "->apply()" atomicity support (the ipg clock, FIFO) requires far
> > more work and testing.
> > 
> > 
> > Best regards,
> > Łukasz Majewski
> > 
> > >   
> > > > 
> > > > This approach has several flaws IMO:
> > > > 
> > > > 1/ I'm not sure this is what the PWM users expect. Getting your
> > > > request queued with no guarantees that it is applied can be
> > > > weird in some cases (especially when the user changes the PWM
> > > > config several times in a short period of time).
> > > > 2/ In the disable path, you queue a "stop PWM" request, but
> > > > you're not sure that it's actually dequeued before the per clk
> > > > is disabled. What happens in that case? And more importantly,
> > > > what happens when the PWM is re-enabled to apply a new config?
> > > > AFAICS, there might be a short period of time where the
> > > > re-enabled PWM is actually running with the old config until we
> > > > flush the command queue and queue a new command.
> > > > 3/ The queueing approach complicates the whole logic. You have
> > > > to flush the FIFO in some cases, or wait for an empty slots if
> > > > too many commands are queued.
> > > >    Forcing imx_pwm_apply_v2() to wait for the config request to
> > > > be applied should simplify the whole thing, because you will
> > > > always be guaranteed that the FIFO is empty, and that the
> > > > current configuration is the last requested one.
> > > >   
> > > 
> > > --
> > > Stefan  
> > 
> 


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

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

[Boris Brezillon]

On Mon, 28 Nov 2016 21:48:57 +0100
Lukasz Majewski <l.majewski@majess.pl> wrote:

> Dear Boris, Stefan,
> 
> > On Mon, 28 Nov 2016 06:50:31 +0100
> > Lukasz Majewski <l.majewski@majess.pl> wrote:
> >   
> > > Dear Stefan, Boris,
> > >   
> > > > On 2016-11-23 00:38, Boris Brezillon wrote:    
> > > > > On Tue, 22 Nov 2016 13:55:33 -0800
> > > > > Stefan Agner <stefan@agner.ch> wrote:
> > > > >     
> > > > >> On 2016-11-01 00:10, Lukasz Majewski 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);
> > > > >> > +    
> > > > >>
> > > > >> Couldn't we do:
> > > > >>
> > > > >> if (cstate.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 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;    
> > > > >>
> > > > >> } else if (state->enabled) {
> > > > >> 	imx_pwm_sw_reset(chip);
> > > > >> }
> > > > >>
> > > > >> and get rid of the if (state->enabled) in between? This would
> > > > >> safe us useless recalculation when disabling the
> > > > >> controller...    
> > > > > 
> > > > > I get your point, but I'm pretty sure your proposal does not do
> > > > > what you want (remember that cstate is the current state, and
> > > > > state is the new state to apply).
> > > > > 
> > > > > Something like that would work better:
> > > > > 
> > > > > 	if (state->enabled) {    
> > > > 
> > > > Oops, yes, got that wrong. state->enabled is what I meant.
> > > >     
> > > > > 		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);
> > > > > 	}
> > > > > 
> > > > > 
> > > > > This being said, I'm a bit concerned by the way this driver
> > > > > handles PWM config requests.
> > > > > It seems that the new config request is queued, and nothing
> > > > > guarantees    
> > > > 
> > > > Not sure if that is true. The RM says: "A change in the period
> > > > value due to a write in PWM_PWMPR results in the counter being
> > > > reset to zero and the start of a new count period."
> > > > 
> > > > And for PWMSAR: "When a new value is written, the duty cycle
> > > > changes after the current period is over."
> > > > 
> > > > So I guess writing the period basically makes sure the next value
> > > > from PWMSAR will be active immediately...
> > > > 
> > > >     
> > > > > that it is actually applied when the
> > > > > pwm_apply/config/enable/disable() functions return.    
> > > > 
> > > > 
> > > > Given that the driver did it like that since quite some time, I am
> > > > assuming it mostly works in practice. 
> > > > 
> > > > I would rather prefer to do that conversion to atomic PWM API
> > > > now, and fix that in a second step...    
> > > 
> > > I'm also for fixing one problem in a time. The "PWM ->apply()" set
> > > of patches now tries to fix all problems in IMX PWM driver.
> > > 
> > > Could we agree on the scope of this work? I mean what should be
> > > included to "->apply()" rework and what will be fixed latter?  
> > 
> > I never asked to fix that in this series ;-), I was just pointing the
> > weird behavior of the existing code.
> > 
> > Let's focus on the atomic support for now.  
> 
> So Boris, you don't have any comments to the atomic support patches? :-)

Nope.

> 
> Stefan, do you require to change the ipg stuff in the atomic series or
> could it be done as a subsequent patch?
> 
> If you don't have any more questions, I will prepare next patch
> iteration according to Stefan comments.
> 
> Best regards,
> Łukasz Majewski

[PATCH v3 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 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 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 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

Re: [PATCH v3 11/11] pwm: imx: Add polarity inversion support to i.MX's PWMv2

[Stefan Agner]

On 2016-11-01 00:10, Lukasz Majewski wrote:
> With this patch the polarity settings for i.MX's PWMv2 is now supported
> on top of atomic PWM setting

Nit: Try to use imperative, e.g. "Update the driver to support...." (see
also Documentation/SubmittingPatches).

I prefer to have the documentation and driver update in one commit so it
is obvious that the driver support has been added at the same time the
documentation has been updated.

--
Stefan

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

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

[Lukasz Majewski]

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

Dear All,

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

Are there any more comments regarding this patch set?

Best regards,
Łukasz Majewski

> 
> 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-rc3 kernel SHA1: a909d3e636995ba7c349e2ca5dbb528154d4ac30
> 
> 
> 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(-)
> 


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