[PATCHv2 0/2] pwm: sunxi: give the pwm IP block more time

[PATCHv2 0/2] pwm: sunxi: give the pwm IP block more time

[Olliver Schinagl]

Hi all,

last year I've sent out this patch series and there was little technical
discussion, so hopefully we can rekindle it.

This patch series fixes 2 issues. For one, we add a delay between disabling
the PWM hardware block and removing the clock, this to yield the PWM hardware
time to actually finish it's PWM and finish in the proper state. If we do not
do this, the output may stay high and may overload speakers for example.

The other issue might need a little more discussion. It also yields the PWM
hardware some extra time. When changing the config, we enable the clock
together with the parameters for the PWM. Sometimes however, the PWM IP core
has not had the time yet to actually set the ready bit. By first enabling
the gate, and then change things, the IP has time to do its thing. The PWM
block has probably done its thing even after the writel has returned. So an
extra delay (of one bus clock cycle?) is overkill.

Changes since v1:
	- Split patch series into several smaller patch series
	- Added driver author

Olliver Schinagl (2):
  pwm: sunxi: allow the pwm to finish its pulse before disable
  pwm: sunxi: Yield some time to the pwm-block to become ready

 drivers/pwm/pwm-sun4i.c | 54 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 18 deletions(-)

-- 
2.8.1

[PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Olliver Schinagl]

When we inform the PWM block to stop toggeling the output, we may end up
in a state where the output is not what we would expect (e.g. not the
low-pulse) but whatever the output was at when the clock got disabled.

To counter this we have to wait for maximally the time of one whole
period to ensure the pwm hardware was able to finish. Since we already
told the PWM hardware to disable it self, it will not continue toggling
but merly finish its current pulse.

If a whole period is considered to much, it may be contemplated to use a
half period + a little bit to ensure we get passed the transition.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/pwm/pwm-sun4i.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 03a99a5..5e97c8a 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -8,6 +8,7 @@
 
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	spin_lock(&sun4i_pwm->ctrl_lock);
 	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
+	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	spin_unlock(&sun4i_pwm->ctrl_lock);
+
+	/* Allow for the PWM hardware to finish its last toggle. The pulse
+	 * may have just started and thus we should wait a full period.
+	 */
+	ndelay(pwm_get_period(pwm));
+
+	spin_lock(&sun4i_pwm->ctrl_lock);
+	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 	val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
 	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
 	spin_unlock(&sun4i_pwm->ctrl_lock);
-- 
2.8.1

[PATCH 2/2] pwm: sunxi: Yield some time to the pwm-block to become ready

[Olliver Schinagl]

From: Olliver Schinagl <o.schinagl@ultimaker.com>

The pwm-block of some of the sunxi chips feature a 'ready' flag to
indicate the software that it is ready for new commands.

Right now, when we call pwm_config and set the period, we write the
values to the registers, and turn off the clock to the IP. Because of
this, the hardware does not have time to configure the hardware and set
the 'ready' flag.

By running the clock just before making new changes and before checking
if the hardware is ready, the hardware has time to reconfigure itself
and set the clear the flag appropriately.

Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
---
 drivers/pwm/pwm-sun4i.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 5e97c8a..dd198c3 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -105,6 +105,22 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	u64 clk_rate, div = 0;
 	unsigned int prescaler = 0;
 	int err;
+	int ret = 0;
+
+	/* Let the PWM hardware run before making any changes. We do this to
+	 * allow the hardware to have some time to clear the 'ready' flag.
+	 */
+	err = clk_prepare_enable(sun4i_pwm->clk);
+	if (err) {
+		dev_err(chip->dev, "failed to enable PWM clock\n");
+		return err;
+	}
+	spin_lock(&sun4i_pwm->ctrl_lock);
+	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	spin_unlock(&sun4i_pwm->ctrl_lock);
 
 	clk_rate = clk_get_rate(sun4i_pwm->clk);
 
@@ -137,7 +153,9 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 		if (div - 1 > PWM_PRD_MASK) {
 			dev_err(chip->dev, "period exceeds the maximum value\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			spin_lock(&sun4i_pwm->ctrl_lock);
+			goto out;
 		}
 	}
 
@@ -146,26 +164,14 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	do_div(div, period_ns);
 	dty = div;
 
-	err = clk_prepare_enable(sun4i_pwm->clk);
-	if (err) {
-		dev_err(chip->dev, "failed to enable PWM clock\n");
-		return err;
-	}
-
 	spin_lock(&sun4i_pwm->ctrl_lock);
 	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
-
 	if (sun4i_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {
-		spin_unlock(&sun4i_pwm->ctrl_lock);
-		clk_disable_unprepare(sun4i_pwm->clk);
-		return -EBUSY;
-	}
-
-	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-	if (clk_gate) {
-		val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-		sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+		ret = -EBUSY;
+		goto out;
 	}
+	val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
 
 	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 	val &= ~BIT_CH(PWM_PRESCAL_MASK, pwm->hwpwm);
@@ -175,6 +181,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	val = (dty & PWM_DTY_MASK) | PWM_PRD(prd);
 	sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
 
+out:
 	if (clk_gate) {
 		val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 		val |= clk_gate;
@@ -184,7 +191,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	spin_unlock(&sun4i_pwm->ctrl_lock);
 	clk_disable_unprepare(sun4i_pwm->clk);
 
-	return 0;
+	return ret;
 }
 
 static int sun4i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
-- 
2.8.1

Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Maxime Ripard]

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

On Thu, Aug 25, 2016 at 07:50:10PM +0200, Olliver Schinagl wrote:
> When we inform the PWM block to stop toggeling the output, we may end up
> in a state where the output is not what we would expect (e.g. not the
> low-pulse) but whatever the output was at when the clock got disabled.
> 
> To counter this we have to wait for maximally the time of one whole
> period to ensure the pwm hardware was able to finish. Since we already
> told the PWM hardware to disable it self, it will not continue toggling
> but merly finish its current pulse.
> 
> If a whole period is considered to much, it may be contemplated to use a
> half period + a little bit to ensure we get passed the transition.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  drivers/pwm/pwm-sun4i.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 03a99a5..5e97c8a 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -8,6 +8,7 @@
>  
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> @@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	spin_lock(&sun4i_pwm->ctrl_lock);
>  	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>  	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> +	spin_unlock(&sun4i_pwm->ctrl_lock);
> +
> +	/* Allow for the PWM hardware to finish its last toggle. The pulse
> +	 * may have just started and thus we should wait a full period.
> +	 */
> +	ndelay(pwm_get_period(pwm));

Can't that use the ready bit as well?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] pwm: sunxi: Yield some time to the pwm-block to become ready

[Maxime Ripard]

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

Hi,

On Thu, Aug 25, 2016 at 07:50:11PM +0200, Olliver Schinagl wrote:
> From: Olliver Schinagl <o.schinagl@ultimaker.com>
> 
> The pwm-block of some of the sunxi chips feature a 'ready' flag to
> indicate the software that it is ready for new commands.
> 
> Right now, when we call pwm_config and set the period, we write the
> values to the registers, and turn off the clock to the IP. Because of
> this, the hardware does not have time to configure the hardware and set
> the 'ready' flag.
> 
> By running the clock just before making new changes and before checking
> if the hardware is ready, the hardware has time to reconfigure itself
> and set the clear the flag appropriately.
> 
> Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
> ---
>  drivers/pwm/pwm-sun4i.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 5e97c8a..dd198c3 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -105,6 +105,22 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	u64 clk_rate, div = 0;
>  	unsigned int prescaler = 0;
>  	int err;
> +	int ret = 0;
> +
> +	/* Let the PWM hardware run before making any changes. We do this to
> +	 * allow the hardware to have some time to clear the 'ready' flag.
> +	 */

This is not the proper comment style.

> +	err = clk_prepare_enable(sun4i_pwm->clk);
> +	if (err) {
> +		dev_err(chip->dev, "failed to enable PWM clock\n");
> +		return err;
> +	}

New line please.

> +	spin_lock(&sun4i_pwm->ctrl_lock);
> +	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> +	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> +	val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);

What are you doing here? You clear a bit, and then put the same one
back in?

> +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> +	spin_unlock(&sun4i_pwm->ctrl_lock);
>  
>  	clk_rate = clk_get_rate(sun4i_pwm->clk);
>  
> @@ -137,7 +153,9 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  		if (div - 1 > PWM_PRD_MASK) {
>  			dev_err(chip->dev, "period exceeds the maximum value\n");
> -			return -EINVAL;
> +			ret = -EINVAL;
> +			spin_lock(&sun4i_pwm->ctrl_lock);

Uh? That's really suspicious. And even if right, please don't do that,
this is just really bad for the comprehension of the workflow.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Olliver Schinagl]

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

Hi Maxime!,

On za, 2016-08-27 at 00:19 +0200, Maxime Ripard wrote:
> On Thu, Aug 25, 2016 at 07:50:10PM +0200, Olliver Schinagl wrote:
> > 
> > When we inform the PWM block to stop toggeling the output, we may
> > end up
> > in a state where the output is not what we would expect (e.g. not
> > the
> > low-pulse) but whatever the output was at when the clock got
> > disabled.
> > 
> > To counter this we have to wait for maximally the time of one whole
> > period to ensure the pwm hardware was able to finish. Since we
> > already
> > told the PWM hardware to disable it self, it will not continue
> > toggling
> > but merly finish its current pulse.
> > 
> > If a whole period is considered to much, it may be contemplated to
> > use a
> > half period + a little bit to ensure we get passed the transition.
> > 
> > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> > ---
> >  drivers/pwm/pwm-sun4i.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index 03a99a5..5e97c8a 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -8,6 +8,7 @@
> >  
> >  #include <linux/bitops.h>
> >  #include <linux/clk.h>
> > +#include <linux/delay.h>
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> > @@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct pwm_chip
> > *chip, struct pwm_device *pwm)
> >  	spin_lock(&sun4i_pwm->ctrl_lock);
> >  	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> >  	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> > +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> > +	spin_unlock(&sun4i_pwm->ctrl_lock);
> > +
> > +	/* Allow for the PWM hardware to finish its last toggle.
> > The pulse
> > +	 * may have just started and thus we should wait a full
> > period.
> > +	 */
> > +	ndelay(pwm_get_period(pwm));
> 
> Can't that use the ready bit as well?
It depends whatever is cheaper. If we disable the pwm, we have to
commit that request to hardware first. Then we have to read back the
has ready and in the strange situation it is not, wait for it to become
ready?

Also, that would mean we would loop in a spin lock, or keep
setting/clearing an additional spinlock to read the ready bit.

If that is cheaper then an ndelay, I can rewrite it of course, and
assuming the 'ready' bit gets set from disabeling the PWM. It needs to
be investigated if disabeling the PWM, the ready bit is used.q

> 
> Maxime
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Maxime Ripard]

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

On Tue, Sep 06, 2016 at 09:12:56AM +0200, Olliver Schinagl wrote:
> Hi Maxime!,
> 
> On za, 2016-08-27 at 00:19 +0200, Maxime Ripard wrote:
> > On Thu, Aug 25, 2016 at 07:50:10PM +0200, Olliver Schinagl wrote:
> > > 
> > > When we inform the PWM block to stop toggeling the output, we may
> > > end up
> > > in a state where the output is not what we would expect (e.g. not
> > > the
> > > low-pulse) but whatever the output was at when the clock got
> > > disabled.
> > > 
> > > To counter this we have to wait for maximally the time of one whole
> > > period to ensure the pwm hardware was able to finish. Since we
> > > already
> > > told the PWM hardware to disable it self, it will not continue
> > > toggling
> > > but merly finish its current pulse.
> > > 
> > > If a whole period is considered to much, it may be contemplated to
> > > use a
> > > half period + a little bit to ensure we get passed the transition.
> > > 
> > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> > > ---
> > >  drivers/pwm/pwm-sun4i.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > index 03a99a5..5e97c8a 100644
> > > --- a/drivers/pwm/pwm-sun4i.c
> > > +++ b/drivers/pwm/pwm-sun4i.c
> > > @@ -8,6 +8,7 @@
> > >  
> > >  #include <linux/bitops.h>
> > >  #include <linux/clk.h>
> > > +#include <linux/delay.h>
> > >  #include <linux/err.h>
> > >  #include <linux/io.h>
> > >  #include <linux/module.h>
> > > @@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct pwm_chip
> > > *chip, struct pwm_device *pwm)
> > >  	spin_lock(&sun4i_pwm->ctrl_lock);
> > >  	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > >  	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> > > +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> > > +	spin_unlock(&sun4i_pwm->ctrl_lock);
> > > +
> > > +	/* Allow for the PWM hardware to finish its last toggle.
> > > The pulse
> > > +	 * may have just started and thus we should wait a full
> > > period.
> > > +	 */
> > > +	ndelay(pwm_get_period(pwm));
> > 
> > Can't that use the ready bit as well?
> It depends whatever is cheaper. If we disable the pwm, we have to
> commit that request to hardware first. Then we have to read back the
> has ready and in the strange situation it is not, wait for it to become
> ready?

If it works like you were suggesting, yes.

> Also, that would mean we would loop in a spin lock, or keep
> setting/clearing an additional spinlock to read the ready bit.

You're using a spin_lock, so it's not that bad, but I was just
suggesting replacing the ndelay.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Olliver Schinagl]

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

On di, 2016-09-06 at 21:51 +0200, Maxime Ripard wrote:
> On Tue, Sep 06, 2016 at 09:12:56AM +0200, Olliver Schinagl wrote:
> > 
> > Hi Maxime!,
> > 
> > On za, 2016-08-27 at 00:19 +0200, Maxime Ripard wrote:
> > > 
> > > On Thu, Aug 25, 2016 at 07:50:10PM +0200, Olliver Schinagl wrote:
> > > > 
> > > > 
> > > > When we inform the PWM block to stop toggeling the output, we
> > > > may
> > > > end up
> > > > in a state where the output is not what we would expect (e.g.
> > > > not
> > > > the
> > > > low-pulse) but whatever the output was at when the clock got
> > > > disabled.
> > > > 
> > > > To counter this we have to wait for maximally the time of one
> > > > whole
> > > > period to ensure the pwm hardware was able to finish. Since we
> > > > already
> > > > told the PWM hardware to disable it self, it will not continue
> > > > toggling
> > > > but merly finish its current pulse.
> > > > 
> > > > If a whole period is considered to much, it may be contemplated
> > > > to
> > > > use a
> > > > half period + a little bit to ensure we get passed the
> > > > transition.
> > > > 
> > > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> > > > ---
> > > >  drivers/pwm/pwm-sun4i.c | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > > index 03a99a5..5e97c8a 100644
> > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > @@ -8,6 +8,7 @@
> > > >  
> > > >  #include <linux/bitops.h>
> > > >  #include <linux/clk.h>
> > > > +#include <linux/delay.h>
> > > >  #include <linux/err.h>
> > > >  #include <linux/io.h>
> > > >  #include <linux/module.h>
> > > > @@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct
> > > > pwm_chip
> > > > *chip, struct pwm_device *pwm)
> > > >  	spin_lock(&sun4i_pwm->ctrl_lock);
> > > >  	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > > >  	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> > > > +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> > > > +	spin_unlock(&sun4i_pwm->ctrl_lock);
> > > > +
> > > > +	/* Allow for the PWM hardware to finish its last
> > > > toggle.
> > > > The pulse
> > > > +	 * may have just started and thus we should wait a
> > > > full
> > > > period.
> > > > +	 */
> > > > +	ndelay(pwm_get_period(pwm));
> > > 
> > > Can't that use the ready bit as well?
> > It depends whatever is cheaper. If we disable the pwm, we have to
> > commit that request to hardware first. Then we have to read back
> > the
> > has ready and in the strange situation it is not, wait for it to
> > become
> > ready?
> 
> If it works like you were suggesting, yes.
> 
> > 
> > Also, that would mean we would loop in a spin lock, or keep
> > setting/clearing an additional spinlock to read the ready bit.
> 
> You're using a spin_lock, so it's not that bad, but I was just
> suggesting replacing the ndelay.
If you say the spin_lock + wait for the ready is just as expensive as
the ndelay, or the ndelay is less preferred, then I gladly make the
change; but I think we need the ndelay for the else where we do not
have the ready flag (A10 or A13 iirc?)

Olliver

> 
> Maxime
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Jonathan Liu]

On 26 August 2016 at 03:50,  <oliver@schinagl.nl> wrote:
> When we inform the PWM block to stop toggeling the output, we may end up
> in a state where the output is not what we would expect (e.g. not the
> low-pulse) but whatever the output was at when the clock got disabled.
>
> To counter this we have to wait for maximally the time of one whole
> period to ensure the pwm hardware was able to finish. Since we already
> told the PWM hardware to disable it self, it will not continue toggling
> but merly finish its current pulse.
>
> If a whole period is considered to much, it may be contemplated to use a
> half period + a little bit to ensure we get passed the transition.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  drivers/pwm/pwm-sun4i.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 03a99a5..5e97c8a 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -8,6 +8,7 @@
>
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> @@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>         spin_lock(&sun4i_pwm->ctrl_lock);
>         val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>         val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> +       sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> +       spin_unlock(&sun4i_pwm->ctrl_lock);
> +
> +       /* Allow for the PWM hardware to finish its last toggle. The pulse
> +        * may have just started and thus we should wait a full period.
> +        */
> +       ndelay(pwm_get_period(pwm));
> +
> +       spin_lock(&sun4i_pwm->ctrl_lock);
> +       val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>         val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
>         sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
>         spin_unlock(&sun4i_pwm->ctrl_lock);

I had some issues where setting the brightness for pwm-backlight to 0
was not turning off the backlight sometimes.
This patch fixes the issue for me. Thanks.

Tested-by: Jonathan Liu <net147@gmail.com>

Re: [1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Olliver Schinagl]

Hey Jonathan,


On 23-09-16 16:02, Jonathan Liu wrote:
> On 26 August 2016 at 03:50,  <oliver@schinagl.nl> wrote:
>> When we inform the PWM block to stop toggeling the output, we may end up
>> in a state where the output is not what we would expect (e.g. not the
>> low-pulse) but whatever the output was at when the clock got disabled.
>>
>> To counter this we have to wait for maximally the time of one whole
>> period to ensure the pwm hardware was able to finish. Since we already
>> told the PWM hardware to disable it self, it will not continue toggling
>> but merly finish its current pulse.
>>
>> If a whole period is considered to much, it may be contemplated to use a
>> half period + a little bit to ensure we get passed the transition.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   drivers/pwm/pwm-sun4i.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
>> index 03a99a5..5e97c8a 100644
>> --- a/drivers/pwm/pwm-sun4i.c
>> +++ b/drivers/pwm/pwm-sun4i.c
>> @@ -8,6 +8,7 @@
>>
>>   #include <linux/bitops.h>
>>   #include <linux/clk.h>
>> +#include <linux/delay.h>
>>   #include <linux/err.h>
>>   #include <linux/io.h>
>>   #include <linux/module.h>
>> @@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>>          spin_lock(&sun4i_pwm->ctrl_lock);
>>          val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>>          val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
>> +       sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
>> +       spin_unlock(&sun4i_pwm->ctrl_lock);
>> +
>> +       /* Allow for the PWM hardware to finish its last toggle. The pulse
>> +        * may have just started and thus we should wait a full period.
>> +        */
>> +       ndelay(pwm_get_period(pwm));
>> +
>> +       spin_lock(&sun4i_pwm->ctrl_lock);
>> +       val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>>          val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
>>          sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
>>          spin_unlock(&sun4i_pwm->ctrl_lock);
> I had some issues where setting the brightness for pwm-backlight to 0
> was not turning off the backlight sometimes.
> This patch fixes the issue for me. Thanks.
You are welcome, but there's some work still being done in this regard, 
or rather, i have to find time to grab a scope and properly check the 
output. So expect the proper fix to be coming to a kernel near you soon :)

Meanwhile, it seems to still confirm that this is needed. Thanks on that.

Olliver
>
> Tested-by: Jonathan Liu <net147@gmail.com>

Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Maxime Ripard]

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

Hi Oliver,

Sorry for the slow answer.

On Fri, Sep 09, 2016 at 11:01:08AM +0200, Olliver Schinagl wrote:
> > > > > *chip, struct pwm_device *pwm)
> > > > >  	spin_lock(&sun4i_pwm->ctrl_lock);
> > > > >  	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > > > >  	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> > > > > +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> > > > > +	spin_unlock(&sun4i_pwm->ctrl_lock);
> > > > > +
> > > > > +	/* Allow for the PWM hardware to finish its last
> > > > > toggle.
> > > > > The pulse
> > > > > +	 * may have just started and thus we should wait a
> > > > > full
> > > > > period.
> > > > > +	 */
> > > > > +	ndelay(pwm_get_period(pwm));
> > > > 
> > > > Can't that use the ready bit as well?
> > > It depends whatever is cheaper. If we disable the pwm, we have to
> > > commit that request to hardware first. Then we have to read back
> > > the
> > > has ready and in the strange situation it is not, wait for it to
> > > become
> > > ready?
> > 
> > If it works like you were suggesting, yes.
> > 
> > > 
> > > Also, that would mean we would loop in a spin lock, or keep
> > > setting/clearing an additional spinlock to read the ready bit.
> > 
> > You're using a spin_lock, so it's not that bad, but I was just
> > suggesting replacing the ndelay.
>
> If you say the spin_lock + wait for the ready is just as expensive as
> the ndelay, or the ndelay is less preferred, then I gladly make the
> change;

For the spin_lock part, I was just comparing it to a
spin_lock_irqsave, which is pretty expensive since it masks all the
interrupts in the system, introducing latencies.

> but I think we need the ndelay for the else where we do not
> have the ready flag (A10 or A13 iirc?)

Hmmmm, good point. But that would also apply to your second patch
then, wouldn't it?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Olliver Schinagl]

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

On za, 2016-09-24 at 22:25 +0200, Maxime Ripard wrote:
> Hi Oliver,
> 
> Sorry for the slow answer.
> 
> On Fri, Sep 09, 2016 at 11:01:08AM +0200, Olliver Schinagl wrote:
> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > *chip, struct pwm_device *pwm)
> > > > > >  	spin_lock(&sun4i_pwm->ctrl_lock);
> > > > > >  	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > > > > >  	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> > > > > > +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> > > > > > +	spin_unlock(&sun4i_pwm->ctrl_lock);
> > > > > > +
> > > > > > +	/* Allow for the PWM hardware to finish its last
> > > > > > toggle.
> > > > > > The pulse
> > > > > > +	 * may have just started and thus we should wait a
> > > > > > full
> > > > > > period.
> > > > > > +	 */
> > > > > > +	ndelay(pwm_get_period(pwm));
> > > > > 
> > > > > Can't that use the ready bit as well?
> > > > It depends whatever is cheaper. If we disable the pwm, we have
> > > > to
> > > > commit that request to hardware first. Then we have to read
> > > > back
> > > > the
> > > > has ready and in the strange situation it is not, wait for it
> > > > to
> > > > become
> > > > ready?
> > > 
> > > If it works like you were suggesting, yes.
> > > 
> > > > 
> > > > 
> > > > Also, that would mean we would loop in a spin lock, or keep
> > > > setting/clearing an additional spinlock to read the ready bit.
> > > 
> > > You're using a spin_lock, so it's not that bad, but I was just
> > > suggesting replacing the ndelay.
> > 
> > If you say the spin_lock + wait for the ready is just as expensive
> > as
> > the ndelay, or the ndelay is less preferred, then I gladly make the
> > change;
> 
> For the spin_lock part, I was just comparing it to a
> spin_lock_irqsave, which is pretty expensive since it masks all the
> interrupts in the system, introducing latencies.
so spin_lock is very expensive and we should avoid if we can?
> 
> > 
> > but I think we need the ndelay for the else where we do not
> > have the ready flag (A10 or A13 iirc?)
> 
> Hmmmm, good point. But that would also apply to your second patch
> then, wouldn't it?
yeah, you would have an if/else for the case of !hasready.

this is what i've been dabbling in the train last week, but haven't
thought it through yet, let alone tested it:


+       if (!(sun4i_pwm->data->has_rdy))
+               ndelay(pwm_get_period(pwm));
+       else
+               do {
+                       spin_lock(&sun4i_pwm->ctrl_lock);
+                       val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+                       spin_unlock(&sun4i_pwm->ctrl_lock);
+               } while (!(val & PWM_RDY(pwm->hwpwm)))

Here I assumed the spin_lock is cheap to make, expensive to hold for
long, e.g. reducing the length the spin-lock is active for. the
alternative was to remove the spin_lock here, and remove unlock-lock
before-after this block where you basically get a very long lasting
spin_lock, the alternative.
 
        spin_lock(&sun4i_pwm->ctrl_lock);
        val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+       if (sun4i_pwm->data->has_rdy && (!(val & PWM_RDY(pwm->hwpwm))))
+               dev_warn(chip->dev, "never became ready\n");

this may be useful for debugging i thought.

        val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
        sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
        spin_unlock(&sun4i_pwm->ctrl_lock);

Olliver
> 
> Maxime
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Maxime Ripard]

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

Hi,

On Mon, Sep 26, 2016 at 10:46:25AM +0200, Olliver Schinagl wrote:
> > For the spin_lock part, I was just comparing it to a
> > spin_lock_irqsave, which is pretty expensive since it masks all the
> > interrupts in the system, introducing latencies.
>
> so spin_lock is very expensive and we should avoid if we can?

spin_lock_irqsave, if possible, yes.

> > > but I think we need the ndelay for the else where we do not
> > > have the ready flag (A10 or A13 iirc?)
> > 
> > Hmmmm, good point. But that would also apply to your second patch
> > then, wouldn't it?
> yeah, you would have an if/else for the case of !hasready.
> 
> this is what i've been dabbling in the train last week, but haven't
> thought it through yet, let alone tested it:
> 
> 
> +       if (!(sun4i_pwm->data->has_rdy))
> +               ndelay(pwm_get_period(pwm));
> +       else
> +               do {
> +                       spin_lock(&sun4i_pwm->ctrl_lock);
> +                       val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> +                       spin_unlock(&sun4i_pwm->ctrl_lock);
> +               } while (!(val & PWM_RDY(pwm->hwpwm)))
> 
> Here I assumed the spin_lock is cheap to make, expensive to hold for
> long, e.g. reducing the length the spin-lock is active for. the
> alternative was to remove the spin_lock here, and remove unlock-lock
> before-after this block where you basically get a very long lasting
> spin_lock, the alternative.

If you're only reading, why do you need to take the lock?

You probbaly want to have a timeout too.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Maxime Ripard]

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

On Thu, Dec 08, 2016 at 02:23:39PM +0100, Olliver Schinagl wrote:
> Hey Maxime,
> 
> first off, also sorry for the slow delay :) (pun not intended)
> 
> On 27-08-16 00:19, Maxime Ripard wrote:
> > On Thu, Aug 25, 2016 at 07:50:10PM +0200, Olliver Schinagl wrote:
> > > When we inform the PWM block to stop toggeling the output, we may end up
> > > in a state where the output is not what we would expect (e.g. not the
> > > low-pulse) but whatever the output was at when the clock got disabled.
> > > 
> > > To counter this we have to wait for maximally the time of one whole
> > > period to ensure the pwm hardware was able to finish. Since we already
> > > told the PWM hardware to disable it self, it will not continue toggling
> > > but merly finish its current pulse.
> > > 
> > > If a whole period is considered to much, it may be contemplated to use a
> > > half period + a little bit to ensure we get passed the transition.
> > > 
> > > Signed-off-by: Olliver Schinagl<oliver@schinagl.nl>
> > > ---
> > >   drivers/pwm/pwm-sun4i.c | 11 +++++++++++
> > >   1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > index 03a99a5..5e97c8a 100644
> > > --- a/drivers/pwm/pwm-sun4i.c
> > > +++ b/drivers/pwm/pwm-sun4i.c
> > > @@ -8,6 +8,7 @@
> > >   #include <linux/bitops.h>
> > >   #include <linux/clk.h>
> > > +#include <linux/delay.h>
> > >   #include <linux/err.h>
> > >   #include <linux/io.h>
> > >   #include <linux/module.h>
> > > @@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > >   	spin_lock(&sun4i_pwm->ctrl_lock);
> > >   	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > >   	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> > > +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> > > +	spin_unlock(&sun4i_pwm->ctrl_lock);
> > > +
> > > +	/* Allow for the PWM hardware to finish its last toggle. The pulse
> > > +	 * may have just started and thus we should wait a full period.
> > > +	 */
> > > +	ndelay(pwm_get_period(pwm));
> > Can't that use the ready bit as well?
> 
> I started to implement our earlier discussed suggestions, but I do not think
> they will work. The read bit is not to let the user know it is ready with
> all of its commands, but only if the period registers are ready. I think it
> is some write lock while it copies the data into its internal control loop.
> From the manual:
> PWM0 period register ready.
> 0: PWM0 period register is ready to write,
> 1: PWM0 period register is busy.
> 
> 
> So no, I don't think i can use the ready bit here at all. The only thing we
> can do here, but I doubt it's worth it, is to read the period register,
> caluclate a time from it, and then ndelay(pwm_get_period(pwm) - ran_time)
> 
> The only 'win' then is that we could are potentially not waiting the full
> pwm period, but only a fraction of it. Since we are disabling the hardware
> (for power reasons) anyway, I don't think this is any significant win,
> except for extreme situations. E.g. we have a pwm period of 10 seconds, we
> disable it after 9.9 second, and now we have to wait for 10 seconds before
> the pwm_disable is finally done. So this could in that case be reduced to
> then only wait for 0.2 seconds since it is 'done' sooner.
> 
> However that optimization is also not 'free'. We have to read the period
> register and calculate back the time. I suggest to do that when reworking
> this driver to work with atomic mode, and merge this patch 'as is' to
> atleast fix te bug where simply not finish properly.

That whole discussion made me realise something that is really
bad. AFAIK, pwm_get_period returns a 32 bits register, which means a
theorical period of 4s. Busy looping during 4 seconds is already very
bad, as you basically kill one CPU during that time, but doing so in a
(potentially) atomic context is even worse.

NACK.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Olliver Schinagl]

Hey Maxime,

Happy new year! I'm sorry that I missed your previous mail! I completely 
looked over it. Sorry!

On 12-12-16 13:24, Maxime Ripard wrote:
> On Thu, Dec 08, 2016 at 02:23:39PM +0100, Olliver Schinagl wrote:
>> Hey Maxime,
>>
>> first off, also sorry for the slow delay :) (pun not intended)
>>
>> On 27-08-16 00:19, Maxime Ripard wrote:
>>> On Thu, Aug 25, 2016 at 07:50:10PM +0200, Olliver Schinagl wrote:
>>>> When we inform the PWM block to stop toggeling the output, we may end up
>>>> in a state where the output is not what we would expect (e.g. not the
>>>> low-pulse) but whatever the output was at when the clock got disabled.
>>>>
>>>> To counter this we have to wait for maximally the time of one whole
>>>> period to ensure the pwm hardware was able to finish. Since we already
>>>> told the PWM hardware to disable it self, it will not continue toggling
>>>> but merly finish its current pulse.
>>>>
>>>> If a whole period is considered to much, it may be contemplated to use a
>>>> half period + a little bit to ensure we get passed the transition.
>>>>
>>>> Signed-off-by: Olliver Schinagl<oliver@schinagl.nl>
>>>> ---
>>>>   drivers/pwm/pwm-sun4i.c | 11 +++++++++++
>>>>   1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
>>>> index 03a99a5..5e97c8a 100644
>>>> --- a/drivers/pwm/pwm-sun4i.c
>>>> +++ b/drivers/pwm/pwm-sun4i.c
>>>> @@ -8,6 +8,7 @@
>>>>   #include <linux/bitops.h>
>>>>   #include <linux/clk.h>
>>>> +#include <linux/delay.h>
>>>>   #include <linux/err.h>
>>>>   #include <linux/io.h>
>>>>   #include <linux/module.h>
>>>> @@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>>>>   	spin_lock(&sun4i_pwm->ctrl_lock);
>>>>   	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>>>>   	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
>>>> +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
>>>> +	spin_unlock(&sun4i_pwm->ctrl_lock);
>>>> +
>>>> +	/* Allow for the PWM hardware to finish its last toggle. The pulse
>>>> +	 * may have just started and thus we should wait a full period.
>>>> +	 */
>>>> +	ndelay(pwm_get_period(pwm));
>>> Can't that use the ready bit as well?
>>
>> I started to implement our earlier discussed suggestions, but I do not think
>> they will work. The read bit is not to let the user know it is ready with
>> all of its commands, but only if the period registers are ready. I think it
>> is some write lock while it copies the data into its internal control loop.
>> From the manual:
>> PWM0 period register ready.
>> 0: PWM0 period register is ready to write,
>> 1: PWM0 period register is busy.
>>
>>
>> So no, I don't think i can use the ready bit here at all. The only thing we
>> can do here, but I doubt it's worth it, is to read the period register,
>> caluclate a time from it, and then ndelay(pwm_get_period(pwm) - ran_time)
>>
>> The only 'win' then is that we could are potentially not waiting the full
>> pwm period, but only a fraction of it. Since we are disabling the hardware
>> (for power reasons) anyway, I don't think this is any significant win,
>> except for extreme situations. E.g. we have a pwm period of 10 seconds, we
>> disable it after 9.9 second, and now we have to wait for 10 seconds before
>> the pwm_disable is finally done. So this could in that case be reduced to
>> then only wait for 0.2 seconds since it is 'done' sooner.
>>
>> However that optimization is also not 'free'. We have to read the period
>> register and calculate back the time. I suggest to do that when reworking
>> this driver to work with atomic mode, and merge this patch 'as is' to
>> atleast fix te bug where simply not finish properly.
>
> That whole discussion made me realise something that is really
> bad. AFAIK, pwm_get_period returns a 32 bits register, which means a
> theorical period of 4s. Busy looping during 4 seconds is already very
> bad, as you basically kill one CPU during that time, but doing so in a
> (potentially) atomic context is even worse.
Well technically, isn't it a 16 bit register? (half for the period, 
other half for the duty cycle?) Anyway, I think the delay can be far 
exceeding 4 seconds (though I haven't checked what the PWM delay max 
option is).

Anyway, you are right, we should absolutely not do this!

>
> NACK.
Absolutely! But what do you suggest? Would usleep (or msleep) instead of 
the ndelay work properly?

>
> Maxime
>

Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Alexandre Belloni]

On 03/01/2017 at 16:59:57 +0100, Olliver Schinagl wrote :
> On 12-12-16 13:24, Maxime Ripard wrote:
> > On Thu, Dec 08, 2016 at 02:23:39PM +0100, Olliver Schinagl wrote:
> > > Hey Maxime,
> > > 
> > > first off, also sorry for the slow delay :) (pun not intended)
> > > 
> > > On 27-08-16 00:19, Maxime Ripard wrote:
> > > > On Thu, Aug 25, 2016 at 07:50:10PM +0200, Olliver Schinagl wrote:
> > > > > When we inform the PWM block to stop toggeling the output, we may end up
> > > > > in a state where the output is not what we would expect (e.g. not the
> > > > > low-pulse) but whatever the output was at when the clock got disabled.
> > > > > 
> > > > > To counter this we have to wait for maximally the time of one whole
> > > > > period to ensure the pwm hardware was able to finish. Since we already
> > > > > told the PWM hardware to disable it self, it will not continue toggling
> > > > > but merly finish its current pulse.
> > > > > 
> > > > > If a whole period is considered to much, it may be contemplated to use a
> > > > > half period + a little bit to ensure we get passed the transition.
> > > > > 
> > > > > Signed-off-by: Olliver Schinagl<oliver@schinagl.nl>
> > > > > ---
> > > > >   drivers/pwm/pwm-sun4i.c | 11 +++++++++++
> > > > >   1 file changed, 11 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > > > index 03a99a5..5e97c8a 100644
> > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > @@ -8,6 +8,7 @@
> > > > >   #include <linux/bitops.h>
> > > > >   #include <linux/clk.h>
> > > > > +#include <linux/delay.h>
> > > > >   #include <linux/err.h>
> > > > >   #include <linux/io.h>
> > > > >   #include <linux/module.h>
> > > > > @@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > > >   	spin_lock(&sun4i_pwm->ctrl_lock);
> > > > >   	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > > > >   	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> > > > > +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> > > > > +	spin_unlock(&sun4i_pwm->ctrl_lock);
> > > > > +
> > > > > +	/* Allow for the PWM hardware to finish its last toggle. The pulse
> > > > > +	 * may have just started and thus we should wait a full period.
> > > > > +	 */
> > > > > +	ndelay(pwm_get_period(pwm));
> > > > Can't that use the ready bit as well?
> > > 
> > > I started to implement our earlier discussed suggestions, but I do not think
> > > they will work. The read bit is not to let the user know it is ready with
> > > all of its commands, but only if the period registers are ready. I think it
> > > is some write lock while it copies the data into its internal control loop.
> > > From the manual:
> > > PWM0 period register ready.
> > > 0: PWM0 period register is ready to write,
> > > 1: PWM0 period register is busy.
> > > 
> > > 
> > > So no, I don't think i can use the ready bit here at all. The only thing we
> > > can do here, but I doubt it's worth it, is to read the period register,
> > > caluclate a time from it, and then ndelay(pwm_get_period(pwm) - ran_time)
> > > 
> > > The only 'win' then is that we could are potentially not waiting the full
> > > pwm period, but only a fraction of it. Since we are disabling the hardware
> > > (for power reasons) anyway, I don't think this is any significant win,
> > > except for extreme situations. E.g. we have a pwm period of 10 seconds, we
> > > disable it after 9.9 second, and now we have to wait for 10 seconds before
> > > the pwm_disable is finally done. So this could in that case be reduced to
> > > then only wait for 0.2 seconds since it is 'done' sooner.
> > > 
> > > However that optimization is also not 'free'. We have to read the period
> > > register and calculate back the time. I suggest to do that when reworking
> > > this driver to work with atomic mode, and merge this patch 'as is' to
> > > atleast fix te bug where simply not finish properly.
> > 
> > That whole discussion made me realise something that is really
> > bad. AFAIK, pwm_get_period returns a 32 bits register, which means a
> > theorical period of 4s. Busy looping during 4 seconds is already very
> > bad, as you basically kill one CPU during that time, but doing so in a
> > (potentially) atomic context is even worse.
> Well technically, isn't it a 16 bit register? (half for the period, other
> half for the duty cycle?) Anyway, I think the delay can be far exceeding 4
> seconds (though I haven't checked what the PWM delay max option is).
> 

That's 196.8s.

But you can rely on the RDY bit because what you want to
achieve actually relies on the fact that the duty cycle will be set to 0
before disabling the channel.


> Anyway, you are right, we should absolutely not do this!
> 
> > 
> > NACK.
> Absolutely! But what do you suggest? Would usleep (or msleep) instead of the
> ndelay work properly?
> 

We can probably set up a timer and disable the channel when it expires.
Obviously it will be necessary to cancel the timer if the channel is
reenabled in the mean time.

Or maybe we can make all the functions blocking and forget about
synchronizing both channels.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Thierry Reding]

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

On Tue, Jan 03, 2017 at 05:55:54PM +0100, Alexandre Belloni wrote:
> On 03/01/2017 at 16:59:57 +0100, Olliver Schinagl wrote :
> > On 12-12-16 13:24, Maxime Ripard wrote:
> > > On Thu, Dec 08, 2016 at 02:23:39PM +0100, Olliver Schinagl wrote:
> > > > Hey Maxime,
> > > > 
> > > > first off, also sorry for the slow delay :) (pun not intended)
> > > > 
> > > > On 27-08-16 00:19, Maxime Ripard wrote:
> > > > > On Thu, Aug 25, 2016 at 07:50:10PM +0200, Olliver Schinagl wrote:
> > > > > > When we inform the PWM block to stop toggeling the output, we may end up
> > > > > > in a state where the output is not what we would expect (e.g. not the
> > > > > > low-pulse) but whatever the output was at when the clock got disabled.
> > > > > > 
> > > > > > To counter this we have to wait for maximally the time of one whole
> > > > > > period to ensure the pwm hardware was able to finish. Since we already
> > > > > > told the PWM hardware to disable it self, it will not continue toggling
> > > > > > but merly finish its current pulse.
> > > > > > 
> > > > > > If a whole period is considered to much, it may be contemplated to use a
> > > > > > half period + a little bit to ensure we get passed the transition.
> > > > > > 
> > > > > > Signed-off-by: Olliver Schinagl<oliver@schinagl.nl>
> > > > > > ---
> > > > > >   drivers/pwm/pwm-sun4i.c | 11 +++++++++++
> > > > > >   1 file changed, 11 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > > > > index 03a99a5..5e97c8a 100644
> > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > @@ -8,6 +8,7 @@
> > > > > >   #include <linux/bitops.h>
> > > > > >   #include <linux/clk.h>
> > > > > > +#include <linux/delay.h>
> > > > > >   #include <linux/err.h>
> > > > > >   #include <linux/io.h>
> > > > > >   #include <linux/module.h>
> > > > > > @@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > > > >   	spin_lock(&sun4i_pwm->ctrl_lock);
> > > > > >   	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > > > > >   	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> > > > > > +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> > > > > > +	spin_unlock(&sun4i_pwm->ctrl_lock);
> > > > > > +
> > > > > > +	/* Allow for the PWM hardware to finish its last toggle. The pulse
> > > > > > +	 * may have just started and thus we should wait a full period.
> > > > > > +	 */
> > > > > > +	ndelay(pwm_get_period(pwm));
> > > > > Can't that use the ready bit as well?
> > > > 
> > > > I started to implement our earlier discussed suggestions, but I do not think
> > > > they will work. The read bit is not to let the user know it is ready with
> > > > all of its commands, but only if the period registers are ready. I think it
> > > > is some write lock while it copies the data into its internal control loop.
> > > > From the manual:
> > > > PWM0 period register ready.
> > > > 0: PWM0 period register is ready to write,
> > > > 1: PWM0 period register is busy.
> > > > 
> > > > 
> > > > So no, I don't think i can use the ready bit here at all. The only thing we
> > > > can do here, but I doubt it's worth it, is to read the period register,
> > > > caluclate a time from it, and then ndelay(pwm_get_period(pwm) - ran_time)
> > > > 
> > > > The only 'win' then is that we could are potentially not waiting the full
> > > > pwm period, but only a fraction of it. Since we are disabling the hardware
> > > > (for power reasons) anyway, I don't think this is any significant win,
> > > > except for extreme situations. E.g. we have a pwm period of 10 seconds, we
> > > > disable it after 9.9 second, and now we have to wait for 10 seconds before
> > > > the pwm_disable is finally done. So this could in that case be reduced to
> > > > then only wait for 0.2 seconds since it is 'done' sooner.
> > > > 
> > > > However that optimization is also not 'free'. We have to read the period
> > > > register and calculate back the time. I suggest to do that when reworking
> > > > this driver to work with atomic mode, and merge this patch 'as is' to
> > > > atleast fix te bug where simply not finish properly.
> > > 
> > > That whole discussion made me realise something that is really
> > > bad. AFAIK, pwm_get_period returns a 32 bits register, which means a
> > > theorical period of 4s. Busy looping during 4 seconds is already very
> > > bad, as you basically kill one CPU during that time, but doing so in a
> > > (potentially) atomic context is even worse.
> > Well technically, isn't it a 16 bit register? (half for the period, other
> > half for the duty cycle?) Anyway, I think the delay can be far exceeding 4
> > seconds (though I haven't checked what the PWM delay max option is).
> > 
> 
> That's 196.8s.
> 
> But you can rely on the RDY bit because what you want to
> achieve actually relies on the fact that the duty cycle will be set to 0
> before disabling the channel.
> 
> 
> > Anyway, you are right, we should absolutely not do this!
> > 
> > > 
> > > NACK.
> > Absolutely! But what do you suggest? Would usleep (or msleep) instead of the
> > ndelay work properly?
> > 
> 
> We can probably set up a timer and disable the channel when it expires.
> Obviously it will be necessary to cancel the timer if the channel is
> reenabled in the mean time.
> 
> Or maybe we can make all the functions blocking and forget about
> synchronizing both channels.

Yes, I think blocking is fine. All drivers report "might sleep" since
v4.5 anyway, so by now all users must have gained support for blocking
drivers.

I'm also not aware of any users that couldn't cope with blocking PWM
devices. The only case that I'm aware of is the LED class that uses it
for blink support and it needs to support the blocking case anyway for
devices that sit on a slow bus, for example.

I'll post a patch to remove pwm_can_sleep() altogether, it's long been
redundant. Because of that I also think it's safe for every driver to
block during any of the operations, which is true both for legacy ones
as well as atomic ones.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Jonathan Liu]

On 24 September 2016 at 00:03, Olliver Schinagl <oliver@schinagl.nl> wrote:
> You are welcome, but there's some work still being done in this regard, or
> rather, i have to find time to grab a scope and properly check the output.
> So expect the proper fix to be coming to a kernel near you soon :)
>
> Meanwhile, it seems to still confirm that this is needed. Thanks on that.
>
> Olliver

Ping. I see pwm_can_sleep() has been removed in 4.11.
Any updates on this?

Regards,
Jonathan