Re: pwm-sun4i: PWM backlight is not turned off on shutdown

[Daniel Thompson]

On Wed, Sep 02, 2020 at 09:42:49PM +0200, Pascal Roeleven wrote:
> Thank you for adding me. Emil (also added now) and I spent a while on trying
> to figure out how to solve this. The Allwinner PWM controller has some
> quirks.
> 
> Unfortunately I never got around to perform some more tests and fix it
> indefinitely. It's still on my todo list..
> 
> > On 9/2/20 12:54 PM, Daniel Thompson wrote:
> > > There's some rather odd logic in sun4i_pwm_apply() that results in the
> > > PWM being disabled twice... once when it applies the initial config
> > > and again after waiting for a duty_cycle.
> 
> That's true. To properly turn off the controller you have to turn the
> controller off first and keep the gate on for at least two full clock
> cycles. Then the gate must be turned off. Otherwise it might get stuck.
> That's probably what is trying to be done here.
> 
> On 2020-09-02 21:05, Andrey Lebedev wrote:
> > Indeed, this fixes the issue for me. The display goes dark reliably on
> > writing 4 to "/sys/.../bl_power" as well as when system is halted. I did
> > not notice any negative side effects so far.
> 
> Problems start to arise when combining bl_power and brightness setting in a
> particular order or at the same time (with for example a backlight driver
> which sets both bl_power and brightness). I can't recall exactly what caused
> problems and when, but one thing I was sure of is that timing was of the
> essence. Once I added some delays here and there it started to work.
> 
> If this patch works for you then that's great, but unfortunately it isn't a
> complete solution.

Forgive my poking but it does look to me like Andrey may have a point
about d3817a647059 ("pwm: sun4i: Remove redundant needs_delay").

I've not got this hardware so I can't comment on whether the current
code is correct or not. However, after reviewing d3817a647059, it is
certainly looks like the patch does not actually implement what the
patch description says it does. In fact, by activating previously
unreachable code, it appears to introduces exactly the regression
described by Andrey.


> From d3817a647059a3e5f8791e9b7225d194985aa75f Mon Sep 17 00:00:00 2001
> From: Pascal Roeleven <dev@xxxxxxxxxxxxxxxxx>
> Date: Tue, 17 Mar 2020 16:59:03 +0100
> Subject: [PATCH] pwm: sun4i: Remove redundant needs_delay
> 
> 'needs_delay' does now always evaluate to true, so remove all
> occurrences.

In other words, all paths that test !needs_delay[pwm->hwpwm] are
unreachable...


> Signed-off-by: Pascal Roeleven <dev@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Thierry Reding <thierry.reding@xxxxxxxxx>
> ---
>  drivers/pwm/pwm-sun4i.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 3e3efa6c768f..5c677c563349 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -90,7 +90,6 @@ struct sun4i_pwm_chip {
>  	spinlock_t ctrl_lock;
>  	const struct sun4i_pwm_data *data;
>  	unsigned long next_period[2];
> -	bool needs_delay[2];
>  };
>  
>  static inline struct sun4i_pwm_chip *to_sun4i_pwm_chip(struct pwm_chip *chip)
> @@ -287,7 +286,6 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
>  	sun4i_pwm->next_period[pwm->hwpwm] = jiffies +
>  		usecs_to_jiffies(cstate.period / 1000 + 1);
> -	sun4i_pwm->needs_delay[pwm->hwpwm] = true;
>  
>  	if (state->polarity != PWM_POLARITY_NORMAL)
>  		ctrl &= ~BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
> @@ -298,7 +296,7 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	if (state->enabled) {
>  		ctrl |= BIT_CH(PWM_EN, pwm->hwpwm);
> -	} else if (!sun4i_pwm->needs_delay[pwm->hwpwm]) {
> +	} else {

... but this previously unreachable path will now be executed
if state->enabled is false.

>  		ctrl &= ~BIT_CH(PWM_EN, pwm->hwpwm);
>  		ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
>  	}
> @@ -310,15 +308,9 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (state->enabled)
>  		return 0;
>  
> -	if (!sun4i_pwm->needs_delay[pwm->hwpwm]) {
> -		clk_disable_unprepare(sun4i_pwm->clk);
> -		return 0;
> -	}
> -

This unreachable path is correctly removed.

>  	/* We need a full period to elapse before disabling the channel. */
>  	now = jiffies;
> -	if (sun4i_pwm->needs_delay[pwm->hwpwm] &&

This unconditionally true expression is also correctly removed.

In short this patch changes behaviour in a manner that could not be
predicted from the patch description.


Daniel.


> -	    time_before(now, sun4i_pwm->next_period[pwm->hwpwm])) {
> +	if (time_before(now, sun4i_pwm->next_period[pwm->hwpwm])) {

>  		delay_us = jiffies_to_usecs(sun4i_pwm->next_period[pwm->hwpwm] -
>  					   now);
>  		if ((delay_us / 500) > MAX_UDELAY_MS)
> @@ -326,7 +318,6 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  		else
>  			usleep_range(delay_us, delay_us * 2);
>  	}
> -	sun4i_pwm->needs_delay[pwm->hwpwm] = false;
>  
>  	spin_lock(&sun4i_pwm->ctrl_lock);
>  	ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);