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

Groups > linux.kernel > #1547406 > unrolled thread

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

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 v3 RESEND 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2 Lukasz Majewski <l.majewski@majess.pl> - 2016-12-27 00:00 +0100
    Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support  for i.MX PWMv2 Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-12-29 17:30 +0100
      Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support  for i.MX PWMv2 Lukasz Majewski <l.majewski@majess.pl> - 2016-12-29 18:00 +0100
        Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support  for i.MX PWMv2 Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-12-29 18:10 +0100
          Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support  for i.MX PWMv2 Lukasz Majewski <lukma@denx.de> - 2017-01-03 12:50 +0100
            Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support  for i.MX PWMv2 Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-03 13:50 +0100
              Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support for  i.MX PWMv2 Stefan Agner <stefan@agner.ch> - 2017-01-03 18:50 +0100
                Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support  for i.MX PWMv2 Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-03 20:40 +0100
                  Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support  for i.MX PWMv2 Lukasz Majewski <lukma@denx.de> - 2017-01-03 23:10 +0100
                    Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support  for i.MX PWMv2 Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-03 23:20 +0100
                      Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support  for i.MX PWMv2 Lukasz Majewski <lukma@denx.de> - 2017-01-03 23:50 +0100
                        Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support  for i.MX PWMv2 Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-04 00:40 +0100
                          Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support  for i.MX PWMv2 Lukasz Majewski <lukma@denx.de> - 2017-01-04 12:40 +0100

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

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

[toc] | [next] | [standalone]

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

Hi Lukasz,

On Mon, 26 Dec 2016 23:55:57 +0100
Lukasz Majewski <l.majewski@majess.pl> wrote:

> This commit provides apply() callback implementation for i.MX's PWMv2.
> 
> Suggested-by: Stefan Agner <stefan@agner.ch>
> Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> Changes for v3:
> - Remove ipg clock enable/disable functions
> 
> Changes for v2:
> - None
> ---
>  drivers/pwm/pwm-imx.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index ebe9b0c..cd53c05 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -159,6 +159,75 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
>  	}
>  }
>  
> +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    struct pwm_state *state)
> +{
> +	unsigned long period_cycles, duty_cycles, prescale;
> +	struct imx_chip *imx = to_imx_chip(chip);
> +	struct pwm_state cstate;
> +	unsigned long long c;
> +	u32 cr = 0;
> +	int ret;
> +
> +	pwm_get_state(pwm, &cstate);
> +
> +	c = clk_get_rate(imx->clk_per);
> +	c *= state->period;
> +
> +	do_div(c, 1000000000);
> +	period_cycles = c;
> +
> +	prescale = period_cycles / 0x10000 + 1;
> +
> +	period_cycles /= prescale;
> +	c = (unsigned long long)period_cycles * state->duty_cycle;
> +	do_div(c, state->period);
> +	duty_cycles = c;
> +
> +	/*
> +	 * according to imx pwm RM, the real period value should be
> +	 * PERIOD value in PWMPR plus 2.
> +	 */
> +	if (period_cycles > 2)
> +		period_cycles -= 2;
> +	else
> +		period_cycles = 0;
> +
> +	/* Enable the clock if the PWM is being enabled. */
> +	if (state->enabled && !cstate.enabled) {
> +		ret = clk_prepare_enable(imx->clk_per);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Wait for a free FIFO slot if the PWM is already enabled, and flush
> +	 * the FIFO if the PWM was disabled and is about to be enabled.
> +	 */
> +	if (cstate.enabled)
> +		imx_pwm_wait_fifo_slot(chip, pwm);
> +	else if (state->enabled)
> +		imx_pwm_sw_reset(chip);
> +
> +	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> +	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> +
> +	cr |= MX3_PWMCR_PRESCALER(prescale) |
> +	      MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> +	      MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
> +
> +	if (state->enabled)
> +		cr |= MX3_PWMCR_EN;
> +
> +	writel(cr, imx->mmio_base + MX3_PWMCR);
> +
> +	/* Disable the clock if the PWM is being disabled. */
> +	if (!state->enabled && cstate.enabled)
> +		clk_disable_unprepare(imx->clk_per);
> +
> +	return 0;
> +}
> +

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

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

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

Regards,

Boris

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

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

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

Hi Boris,

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

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

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


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

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

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

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

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



Best regards,
Łukasz Majewski

> 
> Regards,
> 
> Boris

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

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

Hi Lukasz,

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

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

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

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

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

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

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

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

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

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

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

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

Regards,

Boris

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

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

Hi Boris,

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

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

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

So for the PATCH 7/11 we fix the issue with recalculating clocks
when we want to disable PWM.
	
if (state->enabled) {
		c = clk_get_rate(imx->clk_per);
		c *= state->period;

		do_div(c, 1000000000);
		period_cycles = c;

		prescale = period_cycles / 0x10000 + 1;

		period_cycles /= prescale;
		c = (unsigned long long)period_cycles *
		    state->duty_cycle;
		do_div(c, state->period);
		duty_cycles = c;

		/*
		 * According to imx pwm RM, the real period value
		 * should be PERIOD value in PWMPR plus 2.
		 */
		if (period_cycles > 2)
			period_cycles -= 2;
		else
			period_cycles = 0;

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

		/*
		 * Wait for a free FIFO slot if the PWM is already
		 * enabled, and flush the FIFO if the PWM was disabled
		 * and is about to be enabled.
		 */
		if (cstate.enabled)
			imx_pwm_wait_fifo_slot(chip, pwm);
		else
			imx_pwm_sw_reset(chip);

		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
		writel(period_cycles, imx->mmio_base + MX3_PWMPR);

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

		writel(0, imx->mmio_base + MX3_PWMCR);

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



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

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

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

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

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

My idea is as follows:

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

In such a way we would preserve bisectibility.

What is your opinion?



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

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




Best regards,

Lukasz Majewski

--

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

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

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

Hi Lukasz,

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

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

Yep.

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

Exactly.

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

Yes.

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

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

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

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

Stefan, what's your opinion?

Regards,

Boris

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

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

On 2017-01-03 04:46, Boris Brezillon wrote:
<snip>
>> > Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I think we
>> > both agreed that most of the code was unneeded when all we want to do
>> > is disable the PWM.
>>
>> So for the PATCH 7/11 we fix the issue with recalculating clocks
>> when we want to disable PWM.
>>
>> if (state->enabled) {
>> 		c = clk_get_rate(imx->clk_per);
>> 		c *= state->period;
>>
>> 		do_div(c, 1000000000);
>> 		period_cycles = c;
>>
>> 		prescale = period_cycles / 0x10000 + 1;
>>
>> 		period_cycles /= prescale;
>> 		c = (unsigned long long)period_cycles *
>> 		    state->duty_cycle;
>> 		do_div(c, state->period);
>> 		duty_cycles = c;
>>
>> 		/*
>> 		 * According to imx pwm RM, the real period value
>> 		 * should be PERIOD value in PWMPR plus 2.
>> 		 */
>> 		if (period_cycles > 2)
>> 			period_cycles -= 2;
>> 		else
>> 			period_cycles = 0;
>>
>> 		/*
>> 		 * Enable the clock if the PWM is not already
>> 		 * enabled.
>> 		 */
>> 		if (!cstate.enabled) {
>> 			ret = clk_prepare_enable(imx->clk_per);
>> 			if (ret)
>> 			return ret;
>> 		}
>>
>> 		/*
>> 		 * Wait for a free FIFO slot if the PWM is already
>> 		 * enabled, and flush the FIFO if the PWM was disabled
>> 		 * and is about to be enabled.
>> 		 */
>> 		if (cstate.enabled)
>> 			imx_pwm_wait_fifo_slot(chip, pwm);
>> 		else
>> 			imx_pwm_sw_reset(chip);
>>
>> 		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>> 		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>>
>> 		writel(MX3_PWMCR_PRESCALER(prescale) |
>> 		       MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
>> 		       MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
>> 		       MX3_PWMCR_EN,
>> 		       imx->mmio_base + MX3_PWMCR);
>> 	} else {
>>
>> 		writel(0, imx->mmio_base + MX3_PWMCR);
>>
>> 		/* Disable the clock if the PWM is currently enabled. */
>> 		if (cstate.enabled)
>> 			clk_disable_unprepare(imx->clk_per);
>> 	}
>>
>>
> 
> Yep.
> 

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

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

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

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

--
Stefan

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

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

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

> On 2017-01-03 04:46, Boris Brezillon wrote:
> <snip>
> >> > Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I think we
> >> > both agreed that most of the code was unneeded when all we want to do
> >> > is disable the PWM.  
> >>
> >> So for the PATCH 7/11 we fix the issue with recalculating clocks
> >> when we want to disable PWM.
> >>
> >> if (state->enabled) {
> >> 		c = clk_get_rate(imx->clk_per);
> >> 		c *= state->period;
> >>
> >> 		do_div(c, 1000000000);
> >> 		period_cycles = c;
> >>
> >> 		prescale = period_cycles / 0x10000 + 1;
> >>
> >> 		period_cycles /= prescale;
> >> 		c = (unsigned long long)period_cycles *
> >> 		    state->duty_cycle;
> >> 		do_div(c, state->period);
> >> 		duty_cycles = c;
> >>
> >> 		/*
> >> 		 * According to imx pwm RM, the real period value
> >> 		 * should be PERIOD value in PWMPR plus 2.
> >> 		 */
> >> 		if (period_cycles > 2)
> >> 			period_cycles -= 2;
> >> 		else
> >> 			period_cycles = 0;
> >>
> >> 		/*
> >> 		 * Enable the clock if the PWM is not already
> >> 		 * enabled.
> >> 		 */
> >> 		if (!cstate.enabled) {
> >> 			ret = clk_prepare_enable(imx->clk_per);
> >> 			if (ret)
> >> 			return ret;
> >> 		}
> >>
> >> 		/*
> >> 		 * Wait for a free FIFO slot if the PWM is already
> >> 		 * enabled, and flush the FIFO if the PWM was disabled
> >> 		 * and is about to be enabled.
> >> 		 */
> >> 		if (cstate.enabled)
> >> 			imx_pwm_wait_fifo_slot(chip, pwm);
> >> 		else
> >> 			imx_pwm_sw_reset(chip);
> >>
> >> 		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> >> 		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> >>
> >> 		writel(MX3_PWMCR_PRESCALER(prescale) |
> >> 		       MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> >> 		       MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
> >> 		       MX3_PWMCR_EN,
> >> 		       imx->mmio_base + MX3_PWMCR);
> >> 	} else {
> >>
> >> 		writel(0, imx->mmio_base + MX3_PWMCR);
> >>
> >> 		/* Disable the clock if the PWM is currently enabled. */
> >> 		if (cstate.enabled)
> >> 			clk_disable_unprepare(imx->clk_per);
> >> 	}
> >>
> >>  
> > 
> > Yep.
> >   
> 
> This looks like a good transformation of the current Patch 7, but once
> you merge my patch, it will look slightly different...

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

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

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

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

Regards,

Boris

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

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

Hi Boris, Stefan,

> On Tue, 03 Jan 2017 09:29:40 -0800
> Stefan Agner <stefan@agner.ch> wrote:
> 
> > On 2017-01-03 04:46, Boris Brezillon wrote:
> > <snip>
> > >> > Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I
> > >> > think we both agreed that most of the code was unneeded when
> > >> > all we want to do is disable the PWM.  
> > >>
> > >> So for the PATCH 7/11 we fix the issue with recalculating clocks
> > >> when we want to disable PWM.
> > >>
> > >> if (state->enabled) {
> > >> 		c = clk_get_rate(imx->clk_per);
> > >> 		c *= state->period;
> > >>
> > >> 		do_div(c, 1000000000);
> > >> 		period_cycles = c;
> > >>
> > >> 		prescale = period_cycles / 0x10000 + 1;
> > >>
> > >> 		period_cycles /= prescale;
> > >> 		c = (unsigned long long)period_cycles *
> > >> 		    state->duty_cycle;
> > >> 		do_div(c, state->period);
> > >> 		duty_cycles = c;
> > >>
> > >> 		/*
> > >> 		 * According to imx pwm RM, the real period value
> > >> 		 * should be PERIOD value in PWMPR plus 2.
> > >> 		 */
> > >> 		if (period_cycles > 2)
> > >> 			period_cycles -= 2;
> > >> 		else
> > >> 			period_cycles = 0;
> > >>
> > >> 		/*
> > >> 		 * Enable the clock if the PWM is not already
> > >> 		 * enabled.
> > >> 		 */
> > >> 		if (!cstate.enabled) {
> > >> 			ret = clk_prepare_enable(imx->clk_per);
> > >> 			if (ret)
> > >> 			return ret;
> > >> 		}
> > >>
> > >> 		/*
> > >> 		 * Wait for a free FIFO slot if the PWM is
> > >> already
> > >> 		 * enabled, and flush the FIFO if the PWM was
> > >> disabled
> > >> 		 * and is about to be enabled.
> > >> 		 */
> > >> 		if (cstate.enabled)
> > >> 			imx_pwm_wait_fifo_slot(chip, pwm);
> > >> 		else
> > >> 			imx_pwm_sw_reset(chip);
> > >>
> > >> 		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > >> 		writel(period_cycles, imx->mmio_base +
> > >> MX3_PWMPR);
> > >>
> > >> 		writel(MX3_PWMCR_PRESCALER(prescale) |
> > >> 		       MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> > >> 		       MX3_PWMCR_DBGEN |
> > >> MX3_PWMCR_CLKSRC_IPG_HIGH | MX3_PWMCR_EN,
> > >> 		       imx->mmio_base + MX3_PWMCR);
> > >> 	} else {
> > >>
> > >> 		writel(0, imx->mmio_base + MX3_PWMCR);
> > >>
> > >> 		/* Disable the clock if the PWM is currently
> > >> enabled. */ if (cstate.enabled)
> > >> 			clk_disable_unprepare(imx->clk_per);
> > >> 	}
> > >>
> > >>  
> > > 
> > > Yep.
> > >   
> > 
> > This looks like a good transformation of the current Patch 7, but
> > once you merge my patch, it will look slightly different...
> 
> Yes. I think we should just unconditionally enable/disable the per_clk
> at function entry/exit. The prepare_enable() call is almost free
> when the clk is already enabled, so it's not like we're adding a huge
> overhead by doing that.

So in the above snippet we should replace:

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

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

And 

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

with 
	clk_disable_unprepare(imx->clk_per);

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

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

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


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

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

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

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



> 
> Regards,
> 
> Boris




Best regards,

Lukasz Majewski

--

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

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

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

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

> Hi Boris, Stefan,
> 
> > On Tue, 03 Jan 2017 09:29:40 -0800
> > Stefan Agner <stefan@agner.ch> wrote:
> >   
> > > On 2017-01-03 04:46, Boris Brezillon wrote:
> > > <snip>  
> > > >> > Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I
> > > >> > think we both agreed that most of the code was unneeded when
> > > >> > all we want to do is disable the PWM.    
> > > >>
> > > >> So for the PATCH 7/11 we fix the issue with recalculating clocks
> > > >> when we want to disable PWM.
> > > >>
> > > >> if (state->enabled) {
> > > >> 		c = clk_get_rate(imx->clk_per);
> > > >> 		c *= state->period;
> > > >>
> > > >> 		do_div(c, 1000000000);
> > > >> 		period_cycles = c;
> > > >>
> > > >> 		prescale = period_cycles / 0x10000 + 1;
> > > >>
> > > >> 		period_cycles /= prescale;
> > > >> 		c = (unsigned long long)period_cycles *
> > > >> 		    state->duty_cycle;
> > > >> 		do_div(c, state->period);
> > > >> 		duty_cycles = c;
> > > >>
> > > >> 		/*
> > > >> 		 * According to imx pwm RM, the real period value
> > > >> 		 * should be PERIOD value in PWMPR plus 2.
> > > >> 		 */
> > > >> 		if (period_cycles > 2)
> > > >> 			period_cycles -= 2;
> > > >> 		else
> > > >> 			period_cycles = 0;
> > > >>
> > > >> 		/*
> > > >> 		 * Enable the clock if the PWM is not already
> > > >> 		 * enabled.
> > > >> 		 */
> > > >> 		if (!cstate.enabled) {
> > > >> 			ret = clk_prepare_enable(imx->clk_per);
> > > >> 			if (ret)
> > > >> 			return ret;
> > > >> 		}
> > > >>
> > > >> 		/*
> > > >> 		 * Wait for a free FIFO slot if the PWM is
> > > >> already
> > > >> 		 * enabled, and flush the FIFO if the PWM was
> > > >> disabled
> > > >> 		 * and is about to be enabled.
> > > >> 		 */
> > > >> 		if (cstate.enabled)
> > > >> 			imx_pwm_wait_fifo_slot(chip, pwm);
> > > >> 		else
> > > >> 			imx_pwm_sw_reset(chip);
> > > >>
> > > >> 		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > > >> 		writel(period_cycles, imx->mmio_base +
> > > >> MX3_PWMPR);
> > > >>
> > > >> 		writel(MX3_PWMCR_PRESCALER(prescale) |
> > > >> 		       MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> > > >> 		       MX3_PWMCR_DBGEN |
> > > >> MX3_PWMCR_CLKSRC_IPG_HIGH | MX3_PWMCR_EN,
> > > >> 		       imx->mmio_base + MX3_PWMCR);
> > > >> 	} else {
> > > >>
> > > >> 		writel(0, imx->mmio_base + MX3_PWMCR);
> > > >>
> > > >> 		/* Disable the clock if the PWM is currently
> > > >> enabled. */ if (cstate.enabled)
> > > >> 			clk_disable_unprepare(imx->clk_per);
> > > >> 	}
> > > >>
> > > >>    
> > > > 
> > > > Yep.
> > > >     
> > > 
> > > This looks like a good transformation of the current Patch 7, but
> > > once you merge my patch, it will look slightly different...  
> > 
> > Yes. I think we should just unconditionally enable/disable the per_clk
> > at function entry/exit. The prepare_enable() call is almost free
> > when the clk is already enabled, so it's not like we're adding a huge
> > overhead by doing that.  
> 
> So in the above snippet we should replace:
> 
> if (!cstate.enabled) {
> 	ret = clk_prepare_enable(imx->clk_per);
> 	if (ret)
> 		return ret;
> }
> 
> with
> 	ret = clk_prepare_enable(imx->clk_per);
> 	if (ret)
> 		return ret;
> 
> And 
> 
> if (cstate.enabled)
> 	clk_disable_unprepare(imx->clk_per);
> 
> with 
> 	clk_disable_unprepare(imx->clk_per);

That's what I had in mind.

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

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

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

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

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

Hi Boris,

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

OK.

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

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

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

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

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


Best regards,

Lukasz Majewski

--

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

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

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

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

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

See my reworked version below.

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

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

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

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

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

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

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

Changes in v3:
- New patch

 drivers/pwm/pwm-imx.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

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

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

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

Hi Boris,

> On Tue, 3 Jan 2017 23:46:58 +0100
> Lukasz Majewski <lukma@denx.de> wrote:
> 
> > > > > > >> > > > Same goes for the regression introduced in patch
> > > > > > >> > > > 2: I think it's better to keep things bisectable
> > > > > > >> > > > on all platforms (even if it appeared to work by
> > > > > > >> > > > chance on imx7, it did work before this
> > > > > > >> > > > change).      
> > > > > > >> > >
> > > > > > >> > > Could you be more specific about your idea to solve
> > > > > > >> > > this problem?      
> > > > > > >> >
> > > > > > >> > Stefan already provided a patch, I just think it
> > > > > > >> > should be fixed before patch 2 to avoid breaking
> > > > > > >> > bisectibility.      
> > > > > > >>
> > > > > > >> My idea is as follows:
> > > > > > >>
> > > > > > >> I will drop patch v2 (prepared by Sasha) and then squash
> > > > > > >> Stefan's patch [1] to patch 7/11. The "old" ipg enable
> > > > > > >> code will be removed with other not needed code during
> > > > > > >> conversion.      
> > > > > > > 
> > > > > > > How about keeping patch 2 but enabling/disabling the
> > > > > > > periph clk in imx_pwm_config() instead of completely
> > > > > > > dropping the enable/disable clk sequence.
> > > > > > > 
> > > > > > > In patch 7 you just add the logic we talked about earlier:
> > > > > > > unconditionally enable the periph clk when entering the
> > > > > > > imx_pwm_apply_v2() function and disable it before leaving
> > > > > > > the function.
> > > > > > > 
> > > > > > > This way you can preserve bisectibility and still get rid
> > > > > > > of the ipg clk.
> > > > > > > 
> > > > > > > Stefan, what's your opinion?      
> > > > > > 
> > > > > > We will get rid of the ipg clocks anyway in patch 8 (which
> > > > > > removes those functions completely).
> > > > > > 
> > > > > > So I think Lukasz approach should be fine, just drop patch
> > > > > > 2 and squash my patch into patch 7.    
> > > > > 
> > > > > Well, the end result will be same (ipg_clk will be gone after
> > > > > patch 8), but then it's hard to track why this clock suddenly
> > > > > disappeared. I still think it's worth adding an extra commit
> > > > > explaining that enabling the per_clk before accessing IP
> > > > > registers is needed on some platforms (imx7), and that IPG
> > > > > clk is actually not required until we start using it as a
> > > > > source for the PWM signal generation.
> > > > > 
> > > > > Maybe I'm the only one to think so. In this case, feel free to
> > > > > drop patch 2.    
> > > > 
> > > > If you feel really bad about this issue, then we can drop patch
> > > > 2 and:
> > > > 
> > > > reorganize patch 7/11 to 
> > > >  - keep code, which adds imx_pwm_apply_v2() function code (just
> > > > moves it as is) 
> > > >  - remove .apply = imx_pwm_apply_v2 entry from pwm_ops
> > > > structure.
> > > > 
> > > > 
> > > > On top of it add patch to enable/disable unconditionally the
> > > > imx->clk_per clock to avoid problems on imx7 (and state them in
> > > > commit message).
> > > > 
> > > > Then we add separate patch with 
> > > > .apply = imx_pwm_apply_v2 to pwm_ops structure to enable "new"
> > > > atomic approach.
> > > > 
> > > > And at last we apply patch 8/11, which removes the code for old
> > > > (non atomic) behaviour.
> > > > 
> > > > All the issues are documented in this way on the cost of having
> > > > "dead" (I mean not used) imx_pwm_apply_v2() for two commits.
> > > >   
> > > 
> > > This looks even more complicated.
> > > Sorry, but I don't see the problem with modifying patch 2 to
> > > enable per_clk instead of ipg_clk. Can you explain what's
> > > bothering you?  
> > 
> > But in patch 2:
> > "pwm: imx: remove ipg clock"
> > 
> >  we _remove_ the clk_ipg from imx_pwm_config() and imx_pwm_probe(),
> > so I'm quite puzzled with your above statement.
> 
> See my reworked version below.
> 
> > 
> > > 
> > > If you really want to do the change after patch 7, fine, but in
> > > this case, keep the existing logic: enable/disable ipg_clk in
> > > imx_pwm_apply_v2() until you drop the ipg_clk and replace the
> > > ipg_clk enable/disable sequence by the equivalent enable/disable
> > > per_clk one. 
> > 
> > Frankly, I do agree with Stefan here - we should drop patch 2,
> > squash all changes (including imx7 clock issues) to patch 7
> > (including verbose commit message) and remove the non-atomic code
> > in patch 8.
> 
> Hm, this is not like I'm asking something impossible here (see the
> following patch).
> 
> --->8---
> From c79bb872a40b8e322fd13f33f374fb1ba085e7a9 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Mon, 26 Dec 2016 23:55:52 +0100
> Subject: [PATCH v4] pwm: imx: remove ipg clock and enable per clock
> when required
> 
> The use of the ipg clock was introduced with commit 7b27c160c681
> ("pwm: i.MX: fix clock lookup").
> In the commit message it was claimed that the ipg clock is enabled for
> register accesses. This is true for the ->config() callback, but not
> for the ->set_enable() callback. Given that the ipg clock is not
> consistently enabled for all register accesses we can assume that
> either it is not required at all or that the current code does not
> work. Remove the ipg clock code for now so that it's no longer in the
> way of refactoring the driver.
> 
> In the other hand, the imx7 IP requires the peripheral clock to be
> enabled before accessing its registers. Since ->config() can be called
> when the PWM is disabled (in which case, the peripheral clock is also
> disabled), we need to surround the imx->config() with
> clk_prepare_enable(per_clk)/clk_disable_unprepare(per_clk) calls.
> 
> Note that the imx7 IP was working fine so far because the ipg clock
> was actually pointing to the peripheral clock, which guaranteed
> peripheral clock activation even when ->config() was called when the
> PWM was disabled.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes in v4:
> - Enable per clk before calling imx->config()
> 
> Changes in v3:
> - New patch
> 
>  drivers/pwm/pwm-imx.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d600fd5cd4ba..b1d1e50d3956 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -49,7 +49,6 @@
>  
>  struct imx_chip {
>  	struct clk	*clk_per;
> -	struct clk	*clk_ipg;
>  
>  	void __iomem	*mmio_base;
>  
> @@ -206,13 +205,13 @@ static int imx_pwm_config(struct pwm_chip *chip,
>  	struct imx_chip *imx = to_imx_chip(chip);
>  	int ret;
>  
> -	ret = clk_prepare_enable(imx->clk_ipg);
> +	ret = clk_prepare_enable(imx->clk_per);
>  	if (ret)
>  		return ret;
>  
>  	ret = imx->config(chip, pwm, duty_ns, period_ns);
>  
> -	clk_disable_unprepare(imx->clk_ipg);
> +	clk_disable_unprepare(imx->clk_per);
>  
>  	return ret;
>  }
> @@ -293,13 +292,6 @@ static int imx_pwm_probe(struct platform_device
> *pdev) return PTR_ERR(imx->clk_per);
>  	}
>  
> -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> -	if (IS_ERR(imx->clk_ipg)) {
> -		dev_err(&pdev->dev, "getting ipg clock failed with
> %ld\n",
> -				PTR_ERR(imx->clk_ipg));
> -		return PTR_ERR(imx->clk_ipg);
> -	}
> -
>  	imx->chip.ops = &imx_pwm_ops;
>  	imx->chip.dev = &pdev->dev;
>  	imx->chip.base = -1;

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

Thanks Boris for preparing the patch.


Best regards,

Lukasz Majewski

--

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web