pwm: sunxi: wait for the READY bit

Most of the call sites in the kernel are not really prepared to handle
-EBUSY when calling pwm_config(). This means that they will either fail
silently or fail without letting the user retry at a later time.

This can be seen for example when using pwm-backlight (the most common use
case for this driver). It will first call pwm_config() with a 0 duty cycle
and disable the pwm. Then it will call pwm_config() that fails because the
pwm had no chance to update its period internally and
pwm_enable() ending up with a duty cycle of 0.

Instead, actually wait for the RDY bit to go low before continuing.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/pwm/pwm-sun4i.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Hey Alexandre,

I've sent several patches regarding pwm a while ago, sadly you never 
responded [0]. So I guess this is a follow up from that?

I couldn't quickly find the resubmitted version however.

Anyway, see below for my comments.

On 03-01-17 15:57, Alexandre Belloni wrote:
> Most of the call sites in the kernel are not really prepared to handle
> -EBUSY when calling pwm_config(). This means that they will either fail
> silently or fail without letting the user retry at a later time.
>
> This can be seen for example when using pwm-backlight (the most common use
> case for this driver). It will first call pwm_config() with a 0 duty cycle
> and disable the pwm. Then it will call pwm_config() that fails because the
> pwm had no chance to update its period internally and
> pwm_enable() ending up with a duty cycle of 0.
>
> Instead, actually wait for the RDY bit to go low before continuing.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  drivers/pwm/pwm-sun4i.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index b0803f6c64d9..be489388e006 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -10,6 +10,7 @@
>  #include <linux/clk.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -103,7 +104,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	u32 prd, dty, val, clk_gate;
>  	u64 clk_rate, div = 0;
>  	unsigned int prescaler = 0;
> -	int err;
> +	int err = 0;
>
>  	clk_rate = clk_get_rate(sun4i_pwm->clk);
>
> @@ -154,18 +155,22 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	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);
> +
> +	if (sun4i_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {
> +		val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
>  		sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> +
> +		err = readl_poll_timeout(sun4i_pwm->base + PWM_CTRL_REG, val,
> +					 !(val & PWM_RDY(pwm->hwpwm)), 400,
> +					 500000);
> +		if (err)
> +			goto finish;
>  	}
>
What happens on sun4i here? sun4i does not have the RDY flag, but it 
does need the PWM_CLK_GATING to be active.

maybe only the readl_poll_timeout() should be guarded by the has_rdy, 
where you poll the register as you do now, and in the else just have a 
'known safe delay' to emulate the has_rdy behavior? I'm guessing a few 
clock cycles of the PWM block. I don't think the documentation states 
how long this could/should be.

With my 'wait before disable' patch [1] I run into the same issue, I 
think. We do not know how long to wait before the hardware is ready.

> +	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);
>  	val |= BIT_CH(prescaler, pwm->hwpwm);
> @@ -174,6 +179,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));
>
> +finish:
>  	if (clk_gate) {
>  		val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>  		val |= clk_gate;
> @@ -183,7 +189,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 err;
>  }
>
>  static int sun4i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>

[0] https://patchwork.kernel.org/patch/9299635/
[1] https://lkml.org/lkml/2016/9/26/91
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 03/01/2017 at 16:56:16 +0100, Olliver Schinagl wrote :
> Hey Alexandre,
> 
> I've sent several patches regarding pwm a while ago, sadly you never
> responded [0]. So I guess this is a follow up from that?
> 

Well, we had the issue and I just had a bit of time to look at it. As I
remembered you kind of had the same issue, I chose to Cc you.

> I couldn't quickly find the resubmitted version however.
> 

Was there a new version?

> >  	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> > -	if (clk_gate) {
> > -		val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> > +
> > +	if (sun4i_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {
> > +		val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> >  		sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> > +
> > +		err = readl_poll_timeout(sun4i_pwm->base + PWM_CTRL_REG, val,
> > +					 !(val & PWM_RDY(pwm->hwpwm)), 400,
> > +					 500000);
> > +		if (err)
> > +			goto finish;
> >  	}
> > 
> What happens on sun4i here? sun4i does not have the RDY flag, but it does
> need the PWM_CLK_GATING to be active.
> 

Does it actually need it? The datasheet doesn't say anything about it.
I'm actually wondering what happens if the period is written twice in a
row without waiting. If the latest period is used, maybe we don't
actually care.

> maybe only the readl_poll_timeout() should be guarded by the has_rdy, where
> you poll the register as you do now, and in the else just have a 'known safe
> delay' to emulate the has_rdy behavior? I'm guessing a few clock cycles of
> the PWM block. I don't think the documentation states how long this
> could/should be.
> 

My guess is that the IP is waiting for the current period to finish
before updating the period internally. That would be the sane way to do it but
maybe I'm an optimist.

> With my 'wait before disable' patch [1] I run into the same issue, I think.
> We do not know how long to wait before the hardware is ready.
> 

Up to 196.8s if I'm right...

Hey Alexandre,

Sorry for the very slow reply. We just bought a house so have been 
offline for 6+ weeks!


On 03-01-17 17:44, Alexandre Belloni wrote:
> On 03/01/2017 at 16:56:16 +0100, Olliver Schinagl wrote :
>> Hey Alexandre,
>>
>> I've sent several patches regarding pwm a while ago, sadly you never
>> responded [0]. So I guess this is a follow up from that?
>>
>
> Well, we had the issue and I just had a bit of time to look at it. As I
> remembered you kind of had the same issue, I chose to Cc you.
>
>> I couldn't quickly find the resubmitted version however.
>>
>
> Was there a new version?
I believe there was, but I think this is almost 18 - 24 months ago when 
I started these patches.

Anyhow,

>
>>>  	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
>>> -	if (clk_gate) {
>>> -		val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
>>> +
>>> +	if (sun4i_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {
>>> +		val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
>>>  		sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
>>> +
>>> +		err = readl_poll_timeout(sun4i_pwm->base + PWM_CTRL_REG, val,
>>> +					 !(val & PWM_RDY(pwm->hwpwm)), 400,
>>> +					 500000);
>>> +		if (err)
>>> +			goto finish;
>>>  	}
>>>
>> What happens on sun4i here? sun4i does not have the RDY flag, but it does
>> need the PWM_CLK_GATING to be active.
>>
>
> Does it actually need it? The datasheet doesn't say anything about it.

I'm fairly certain it does, everything needs the gate enabled to run. 
E.g. no clock gate enabled, the entire IP is unable to do anything.

> I'm actually wondering what happens if the period is written twice in a
> row without waiting. If the latest period is used, maybe we don't
> actually care.

That approach sounds a little hack-ish, (and I have forgot almost all 
context here, so forgive me) but basically you are suggesting to just 
spam the period register until it sticks?

Anyway, as I said, I'm fairly certain also the A10 needs the gate 
enabled to be able to 'eat' the data, so it looks like this would break 
things on A10.

>
>> maybe only the readl_poll_timeout() should be guarded by the has_rdy, where
>> you poll the register as you do now, and in the else just have a 'known safe
>> delay' to emulate the has_rdy behavior? I'm guessing a few clock cycles of
>> the PWM block. I don't think the documentation states how long this
>> could/should be.
>>
>
> My guess is that the IP is waiting for the current period to finish
> before updating the period internally. That would be the sane way to do it but
> maybe I'm an optimist.

Well that does sound logically; i'm guessing in pseudo code the vhdl 
likley looks like this

if (clk > HIGH) {
	if (counter < period) {
		counter++;
	} else {
		counter = 0;
		period = period_reg;
	}
}

where the clock only ticks if the clk_gate is active, otherwise clk 
never toggles from HIGH to LOW.

I'll give some more thought into this ...

Olliver

>
>> With my 'wait before disable' patch [1] I run into the same issue, I think.
>> We do not know how long to wait before the hardware is ready.
>>
>
> Up to 196.8s if I'm right...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html