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

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

[Andrey Lebedev]

Hello,

I think I'm experiencing problem with pwm-sun4i module. I'll describe
the symptoms first.

I have a device, based on Allwinner A20 (Cubieboard 2) with LVDS display
that has a PWM-based backlight. The problem is: when linux shuts down,
the backlight stays on. I expect it to be turned off. This used to work
as expected on kernel 5.2-rc2, but after upgrade to 5.8 the backlight
does not turn off anymore (most of the times, see below).

The backlight is configured in the device tree [1]. The brightness can
be changed by writing to "brightness" file on sysfs. So, linux can
control the PWM line. Backlight sysfs directory also has a "bl_power"
file, which can accept "0" to power on or "4" to power off the backlight
(according to [2]).

Now, writing "4" to bl_power sometimes turns the backlight off and
sometimes not. I've found that the probability of backlight turning off
pretty much correlates with the current screen brightness: on 100%
brightness it will never turn off, on 50% brightness it will turn off on
about half of the times. When backlight does not turn off, it goes on
full brightness. It feels like the line, controlled by pwm stays in
whatever state it was the moment backlight was powered down - either
full 1 or 0.

The pwm backlight device driver (pwm_bl) requests to set the duty cycle
to 0 and disable the pwm with the same request [3], but I suspect the
implementation driver (pwm-sun4i) does not actually set the duty cycle
to 0 before disabling the pulse width modulation.

Is there anything that can be done to fix this?


[1]
https://github.com/Openvario/meta-openvario/blob/warrior/recipes-kernel/linux/linux-mainline/openvario-common.dts#L21-L27


[2]
https://www.kernel.org/doc/Documentation/ABI/stable/sysfs-class-backlight

[3]
https://github.com/torvalds/linux/blob/master/drivers/video/backlight/pwm_bl.c#L81-L83

-- 
Andrey Lebedev aka -.- . -.. -.. . .-.
Software engineer
Homepage: http://lebedev.lt/

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

[Daniel Thompson]

On Thu, Aug 27, 2020 at 10:55:28PM +0300, Andrey Lebedev wrote:
> Hello,
> 
> I think I'm experiencing problem with pwm-sun4i module. I'll describe
> the symptoms first.
> 
> I have a device, based on Allwinner A20 (Cubieboard 2) with LVDS display
> that has a PWM-based backlight. The problem is: when linux shuts down,
> the backlight stays on. I expect it to be turned off. This used to work
> as expected on kernel 5.2-rc2, but after upgrade to 5.8 the backlight
> does not turn off anymore (most of the times, see below).
> 
> The backlight is configured in the device tree [1]. The brightness can
> be changed by writing to "brightness" file on sysfs. So, linux can
> control the PWM line. Backlight sysfs directory also has a "bl_power"
> file, which can accept "0" to power on or "4" to power off the backlight
> (according to [2]).
> 
> Now, writing "4" to bl_power sometimes turns the backlight off and
> sometimes not. I've found that the probability of backlight turning off
> pretty much correlates with the current screen brightness: on 100%
> brightness it will never turn off, on 50% brightness it will turn off on
> about half of the times. When backlight does not turn off, it goes on
> full brightness. It feels like the line, controlled by pwm stays in
> whatever state it was the moment backlight was powered down - either
> full 1 or 0.
> 
> The pwm backlight device driver (pwm_bl) requests to set the duty cycle
> to 0 and disable the pwm with the same request [3], but I suspect the
> implementation driver (pwm-sun4i) does not actually set the duty cycle
> to 0 before disabling the pulse width modulation.
> 
> Is there anything that can be done to fix this?

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.

I suspect disabling the initial disable would solve your issue... but it
might provoke some new ones!

Anyhow, try removing the else clause starting at line 299 and see what
happens:
https://elixir.bootlin.com/linux/v5.8/source/drivers/pwm/pwm-sun4i.c#L299


Daniel.

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

[Andrey Lebedev]

On 9/2/20 12:54 PM, Daniel Thompson wrote:
> On Thu, Aug 27, 2020 at 10:55:28PM +0300, Andrey Lebedev wrote:
>> I think I'm experiencing problem with pwm-sun4i module. I'll describe
>> the symptoms first.
>> 
>> I have a device, based on Allwinner A20 (Cubieboard 2) with LVDS display
>> that has a PWM-based backlight. The problem is: when linux shuts down,
>> the backlight stays on. I expect it to be turned off. This used to work
>> as expected on kernel 5.2-rc2, but after upgrade to 5.8 the backlight
>> does not turn off anymore (most of the times, see below).
>> 
>> The backlight is configured in the device tree [1]. The brightness can
>> be changed by writing to "brightness" file on sysfs. So, linux can
>> control the PWM line. Backlight sysfs directory also has a "bl_power"
>> file, which can accept "0" to power on or "4" to power off the backlight
>> (according to [2]).
>> 
>> Now, writing "4" to bl_power sometimes turns the backlight off and
>> sometimes not. I've found that the probability of backlight turning off
>> pretty much correlates with the current screen brightness: on 100%
>> brightness it will never turn off, on 50% brightness it will turn off on
>> about half of the times. When backlight does not turn off, it goes on
>> full brightness. It feels like the line, controlled by pwm stays in
>> whatever state it was the moment backlight was powered down - either
>> full 1 or 0.>>
>> The pwm backlight device driver (pwm_bl) requests to set the duty cycle
>> to 0 and disable the pwm with the same request [3], but I suspect the
>> implementation driver (pwm-sun4i) does not actually set the duty cycle
>> to 0 before disabling the pulse width modulation.
>>
>> Is there anything that can be done to fix this?
> 
> 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.
> 
> I suspect disabling the initial disable would solve your issue... but it
> might provoke some new ones!
> 
> Anyhow, try removing the else clause starting at line 299 and see what
> happens:
> https://elixir.bootlin.com/linux/v5.8/source/drivers/pwm/pwm-sun4i.c#L299
> 

Thanks Daniel,

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.

I'm not completely sure, but this might be a regression after
d3817a647059a3e5f8791e9b7225d194985aa75f. So, adding Pascal and
Alexandre to the loop.

We'll add this as custom patch to our kernel, will let you know if
something will go wrong because of this.



diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 5c677c563349..4b0b9aed9bb9 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -296,9 +296,6 @@ static int sun4i_pwm_apply(struct pwm_chip *chip,
struct pwm_device *pwm,

        if (state->enabled) {
                ctrl |= BIT_CH(PWM_EN, pwm->hwpwm);
-       } else {
-               ctrl &= ~BIT_CH(PWM_EN, pwm->hwpwm);
-               ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
        }

        sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG);



-- 
Andrey Lebedev aka -.- . -.. -.. . .-.
Software engineer
Homepage: http://lebedev.lt/

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

[Pascal Roeleven]

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.

Regards,
Pascal

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@pascalroeleven.nl>
> 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@pascalroeleven.nl>
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>  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);