Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1506778 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation Lukasz Majewski <l.majewski@majess.pl> - 2016-10-24 00:00 +0200
    Re: [PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate  switch to atomic pwm operation Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-10-24 17:30 +0200
    Re: [PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate  switch to atomic pwm operation Sascha Hauer <s.hauer@pengutronix.de> - 2016-10-25 08:00 +0200
      Re: [PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate  switch to atomic pwm operation Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-10-25 08:30 +0200
        Re: [PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate  switch to atomic pwm operation Sascha Hauer <s.hauer@pengutronix.de> - 2016-10-25 08:40 +0200
          Re: [PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate  switch to atomic pwm operation Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-10-25 08:50 +0200
      Re: [PATCH 1/6] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate  switch to atomic pwm operation Lukasz Majewski <l.majewski@majess.pl> - 2016-10-25 09:00 +0200

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

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

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

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

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

[toc] | [next] | [standalone]

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

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

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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

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

Sascha

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

[toc] | [prev] | [next] | [standalone]

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

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

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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

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

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

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

Sascha


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

[toc] | [prev] | [next] | [standalone]

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

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

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

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

Hi Sascha,

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

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

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

Thanks for feedback,

Łukasz Majewski

> 
> Sascha
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web