[PATCH] backlight: pwm_bl: configure pwm only once per backlight toggle

[Adam Ford]

On Thu, Oct 17, 2019 at 6:11 AM Thierry Reding <thierry.reding at gmail.com> wrote:
>
> On Thu, Oct 17, 2019 at 12:11:16PM +0200, Uwe Kleine-König wrote:
> > On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote:
> > > On 17. 10. 19 10:10, Uwe Kleine-König wrote:
> > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let
> > > > pwm_get_state() return the last implemented state")) changed the
> > > > semantic of pwm_get_state() and disclosed an (as it seems) common
> > > > problem in lowlevel PWM drivers. By not relying on the period and duty
> > > > cycle being retrievable from a disabled PWM this type of problem is
> > > > worked around.
> > > >
> > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state
> > > > combo once is also more effective.
> > > >
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig at pengutronix.de>
> > > > ---
> > > > Hello,
> > > >
> > > > There are now two reports about 01ccf903edd6 breaking a backlight. As
> > > > far as I understand the problem this is a combination of the backend pwm
> > > > driver yielding surprising results and the pwm-bl driver doing things
> > > > more complicated than necessary.
> > > >
> > > > So I guess this patch works around these problems. Still it would be
> > > > interesting to find out the details in the imx driver that triggers the
> > > > problem. So Adam, can you please instrument the pwm-imx27 driver to
> > > > print *state at the beginning of pwm_imx27_apply() and the end of
> > > > pwm_imx27_get_state() and provide the results?
> > > >
> > > > Note I only compile tested this change.
> > >
> > > Hi Uwe,
> > > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+"
> > > thread that I have a similar problem when you submitted this patch.
> > >
> > > So here are my few cents:
> > >
> > > My setup is as follows:
> > >  - imx6dl-yapp4-draco with i.MX6Solo
> > >  - backlight is controlled with inverted PWM signal
> > >  - max brightness level = 32, default brightness level set to 32 in DT.
> > >
> > > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let
> > >    pwm_get_state() return the last implemented state):
> > >
> > >  - System boots to userspace and backlight is enabled all the time from
> > >    power up.
> > >
> > >    $ dmesg | grep state
> > >    [    1.763381] get state end: -1811360608, enabled: 0
> >
> > What is -1811360608? When I wrote "print *state" above, I thought about
> > something like:
> >
> >       pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d",
> >               __func__, state->period, state->duty_cycle, state->polarity, state->enabled);
> >
> > A quick look into drivers/pwm/pwm-imx27.c shows that this is another
> > driver that yields duty_cycle = 0 when the hardware is off.
>
> It seems to me like the best recourse to fix this for now would be to
> patch up the drivers that return 0 when the hardware is off by caching
> the currently configured duty cycle.
>
> How about the patch below?
>
> Thierry
>
> --- >8 ---
> From 15a52a7f1b910804fabd74a5882befd3f9d6bb37 Mon Sep 17 00:00:00 2001
> From: Thierry Reding <thierry.reding at gmail.com>
> Date: Thu, 17 Oct 2019 12:56:00 +0200
> Subject: [PATCH] pwm: imx27: Cache duty cycle register value
>
> The hardware register containing the duty cycle value cannot be accessed
> when the PWM is disabled. This causes the ->get_state() callback to read
> back a duty cycle value of 0, which can confuse consumer drivers.
>
> Signed-off-by: Thierry Reding <thierry.reding at gmail.com>
> ---
>  drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index ae11d8577f18..4113d5cd4c62 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -85,6 +85,13 @@ struct pwm_imx27_chip {
>         struct clk      *clk_per;
>         void __iomem    *mmio_base;
>         struct pwm_chip chip;
> +
> +       /*
> +        * The driver cannot read the current duty cycle from the hardware if
> +        * the hardware is disabled. Cache the last programmed duty cycle
> +        * value to return in that case.
> +        */
> +       unsigned int duty_cycle;
>  };
>
>  #define to_pwm_imx27_chip(chip)        container_of(chip, struct pwm_imx27_chip, chip)
> @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip,
>         tmp = NSEC_PER_SEC * (u64)(period + 2);
>         state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
>
> -       /* PWMSAR can be read only if PWM is enabled */
> -       if (state->enabled) {
> +       /*
> +        * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
> +        * use the cached value.
> +        */
> +       if (state->enabled)
>                 val = readl(imx->mmio_base + MX3_PWMSAR);
> -               tmp = NSEC_PER_SEC * (u64)(val);
> -               state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
> -       } else {
> -               state->duty_cycle = 0;
> -       }
> +       else
> +               val = imx->duty_cycle;
> +
> +       tmp = NSEC_PER_SEC * (u64)(val);
> +       state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);

Is this right?  It seems like the tmp  and state->duty_cycle
caltulations should be kept inside "if (state->enabled)" because if we
set val to the duty_cycle in the else, I would think it is going to
calculate this again.

I think the 'else' should be 'state->duty_cycle = imx->duty_cycle'
because we shouldn't need to recalculate this again.

Am I missing something?

adam
>
>         if (!state->enabled)
>                 pwm_imx27_clk_disable_unprepare(chip);
> @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>                 writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>                 writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>
> +               /*
> +                * Store the duty cycle for future reference in cases where
> +                * the MX3_PWMSAR register can't be read (i.e. when the PWM
> +                * is disabled).
> +                */
> +               imx->duty_cycle = duty_cycles;
> +
>                 cr = MX3_PWMCR_PRESCALER_SET(prescale) |
>                      MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
>                      FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
> --
> 2.23.0
>