[1/2] pwm: pxa: Enable clock before applying config

The clock has to be on before we can apply the config anyway, so there
is no need to turn the clock on, off, and back on again.

This fixes an issue discovered on the PXA168 where sometimes the PWM
output doesn't activate properly if the clock is turned off and back on
too quickly. Removing the on/off/on sequence eliminates the problem.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/pwm/pwm-pxa.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

Hello Doug,

On Sat, Oct 01, 2022 at 11:15:51PM -0700, Doug Brown wrote:
> The clock has to be on before we can apply the config anyway, so there
> is no need to turn the clock on, off, and back on again.
> 
> This fixes an issue discovered on the PXA168 where sometimes the PWM
> output doesn't activate properly if the clock is turned off and back on
> too quickly. Removing the on/off/on sequence eliminates the problem.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
>  drivers/pwm/pwm-pxa.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
> index 0bcaa58c6a91..208c32c79453 100644
> --- a/drivers/pwm/pwm-pxa.c
> +++ b/drivers/pwm/pwm-pxa.c
> @@ -64,7 +64,6 @@ static int pxa_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	unsigned long long c;
>  	unsigned long period_cycles, prescale, pv, dc;
>  	unsigned long offset;
> -	int rc;
>  
>  	offset = pwm->hwpwm ? 0x10 : 0;
>  
> @@ -86,18 +85,11 @@ static int pxa_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	else
>  		dc = mul_u64_u64_div_u64(pv + 1, duty_ns, period_ns);
>  
> -	/* NOTE: the clock to PWM has to be enabled first
> -	 * before writing to the registers
> -	 */
> -	rc = clk_prepare_enable(pc->clk);
> -	if (rc < 0)
> -		return rc;
> -
> +	/* Clock is required here. We can assume it's already on. */
>  	writel(prescale, pc->mmio_base + offset + PWMCR);
>  	writel(dc, pc->mmio_base + offset + PWMDCR);
>  	writel(pv, pc->mmio_base + offset + PWMPCR);
>  
> -	clk_disable_unprepare(pc->clk);
>  	return 0;
>  }
>  
> @@ -130,12 +122,17 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  		return 0;
>  	}
>  
> +	if (!pwm->state.enabled) {
> +		err = pxa_pwm_enable(chip, pwm);
> +		if (err)
> +			return err;
> +	}

First, would you mind adding a first patch that just replaces
pxa_pwm_enable() by clk_prepare_enable(pc->clk) and pxa_pwm_disable() by
clk_disable_unprepare(pc->clk)? Given that the dedicated .enable() and
.disable() callbacks are not needed any more, this would simplify the
driver a bit.

Then I think to understand that you need the clk enabled for operating
the output and for register access, right? I'm not sure, but after the
above change it might be simpler/more efficient to enable the clk here
unconditionally (and adapt the end of the function accordingly).

>  	err = pxa_pwm_config(chip, pwm, state->duty_cycle, state->period);
> -	if (err)
> +	if (err) {
> +		pxa_pwm_disable(chip, pwm);
>  		return err;
> -
> -	if (!pwm->state.enabled)
> -		return pxa_pwm_enable(chip, pwm);
> +	}

If there is a transistion say from:

	.period = A, .duty_cycle = B, .enabled = false

to

	.period = C, .duty_cycle = D, .enabled = true

can it happen that you (shortly) see A and B on the wire? (I think this
is orthogonal to this patch and happens with and without it.) If so it
might be prudent do configure duty_cycle = 0 for the .enabled = false
case.

Best regards
Uwe

Hi Uwe,

On 10/2/2022 8:04 AM, Uwe Kleine-König wrote:

> First, would you mind adding a first patch that just replaces
> pxa_pwm_enable() by clk_prepare_enable(pc->clk) and pxa_pwm_disable() by
> clk_disable_unprepare(pc->clk)? Given that the dedicated .enable() and
> .disable() callbacks are not needed any more, this would simplify the
> driver a bit.

Will do -- makes sense.

> Then I think to understand that you need the clk enabled for operating
> the output and for register access, right? I'm not sure, but after the
> above change it might be simpler/more efficient to enable the clk here
> unconditionally (and adapt the end of the function accordingly).

Right. It has to be enabled in order to access the registers, and it
also simultaneously enables the actual output. What you are suggesting
makes perfect sense to me. I will play with that approach for V2.

> If there is a transistion say from:
> 
> 	.period = A, .duty_cycle = B, .enabled = false
> 
> to
> 
> 	.period = C, .duty_cycle = D, .enabled = true
> 
> can it happen that you (shortly) see A and B on the wire? (I think this
> is orthogonal to this patch and happens with and without it.) If so it
> might be prudent do configure duty_cycle = 0 for the .enabled = false
> case.

You are absolutely correct. I had noticed that case was possible, but it
didn't click in my head that setting duty_cycle to 0 when disabling
would work around it. I need to set duty_cycle to 0 when disabling the
PWM anyway, because it seems to have a tendency to get stuck high when
stopping the clock.

I also realized I forgot to submit one other patch to allow this driver
to be selected with ARCH_MMP (which the PXA168 belongs to). I will
include that with V2 as well.

Thanks for reviewing this so quickly and providing great feedback!

Doug