On Wed, Jun 30, 2021 at 06:21:28PM +0200, Uwe Kleine-König wrote:
> Hello,
>
> On Wed, Jun 30, 2021 at 12:22:22PM +0200, Geert Uytterhoeven wrote:
> > Hi Uwe,
> >
> > On Wed, Jun 30, 2021 at 8:48 AM Uwe Kleine-König
> > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > > On Tue, Jun 29, 2021 at 09:44:38PM +0200, Geert Uytterhoeven wrote:
> > > > On Sat, 1 May 2021, Uwe Kleine-König wrote:
> > > > > Without this change it can happen that if changing the polarity succeeded
> > > > > but changing duty_cycle and period failed pwm->state contains a mixture
> > > > > between the old and the requested state.
> > > > >
> > > > > So remember the initial state before starting to modify the configuration
> > > > > and restore it when one of the required callback fails.
> > > > >
> > > > > Compared to the previous implementation .disable() (if necessary) is called
> > > > > earlier to prevent a glitch.
> > > > >
> > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > > >
> > > > Thanks for your patch, which is now commit d7bff84fe7ed8c3b ("pwm:
> > > > Ensure for legacy drivers that pwm->state stays consistent") in
> > > > pwm/for-next.
> > > >
> > > > This commit broke the backlight on the Atmark Techno Armadillo 800 EVA
> > > > board (arch/arm/boot/dts/r8a7740-armadillo800eva.dts), which now shows a
> > > > black screen. Reverting the commit fixes the problem.
> > > >
> > > > Do you have an idea what is wrong, and how to fix it?
> > >
> > > I starred at the patch for some time now and couldn't find a problem.
> > > Looking at drivers/pwm/pwm-renesas-tpu.c I don't see something obvious.
> > > (The .set_polarity callback is faulty as I doesn't commit the request to
> > > hardware, but that shouldn't matter here.)
> > >
> > > I guess the first request the backlight driver emits is
> > >
> > > .period = 33333,
> > > .duty_cycle = 33333,
> > > .enabled = true,
> > > .polarity = PWM_POLARITY_INVERSED,
> > >
> > > which should result into the following driver calls (with and without
> > > the breaking commit):
> > >
> > > tpu_pwm_set_polarity(chip, pwm, PWM_POLARITY_INVERSED);
> > > tpu_pwm_config(chip, pwm, 33333, 33333);
> > > tpu_pwm_enable(chip, pwm);
> > >
> > > Can you confirm that?
> >
> > tpu_pwm_config() is no longer called:
> >
> > renesas-tpu-pwm e6600000.pwm: tpu_pwm_set_polarity:334: channel
> > 2, polarity = 1
> > -renesas-tpu-pwm e6600000.pwm: tpu_pwm_config:257: channel = 2,
> > duty_ns = 0, period_ns = 33333
> > -renesas-tpu-pwm e6600000.pwm: tpu_pwm_config:257: channel = 2,
> > duty_ns = 33333, period_ns = 33333
> > renesas-tpu-pwm e6600000.pwm: tpu_pwm_enable:346: channel 2
>
> OK, I see a problem (though this doesn't explain the display staying
> off directly after boot):
>
> After doing:
>
> pwm_apply_state(pwm, { .period = 33333, .duty_cycle = 0, .enabled = false, .polarity = ..});
>
> .period and .duty_cycle are assumed to be set even though calling
> ->config was skipped because .enabled is false.
>
> So when
>
> pwm_apply_state(pwm, { .period = 33333, .duty_cycle = 0, .enabled = true, .polarity = ..});
>
> is called next, ->config isn't called because the core assumes
> .duty_cycle and .period are already setup fine.
>
> So we either must not skip calling ->config when .enabled is false:
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index ab38627bcacd..f8a5a095a410 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -558,12 +558,8 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
> pwm->state.polarity = state->polarity;
> }
>
> - if (!state->enabled) {
> - if (pwm->state.enabled)
> - chip->ops->disable(chip, pwm);
> -
> - return 0;
> - }
> + if (!state->enabled && pwm->state.enabled)
> + chip->ops->disable(chip, pwm);
>
> if (state->period != pwm->state.period ||
> state->duty_cycle != pwm->state.duty_cycle) {
> @@ -577,7 +573,7 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
> pwm->state.duty_cycle = state->duty_cycle;
> }
>
> - if (!pwm->state.enabled) {
> + if (state->enabled && !pwm->state.enabled) {
> err = chip->ops->enable(chip, pwm);
> if (err)
> goto rollback;
>
> or we have to call ->config unconditionally:
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index ab38627bcacd..05d7afe25b42 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -565,17 +565,21 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
> return 0;
> }
>
> + /*
> + * We cannot skip this even if state->period == pwm->state.period &&
> + * state->duty_cycle == pwm->state.duty_cycle because we might have
> + * exited early in the last call to pwm_apply_state because of
> + * !state->enabled and so the two values in pwm->state might not be
> + * configured in hardware.
> + */
> + err = chip->ops->config(pwm->chip, pwm,
> + state->duty_cycle,
> + state->period);
> + if (err)
> + goto rollback;
> +
> + pwm->state.period = state->period;
> + pwm->state.duty_cycle = state->duty_cycle;
> - if (state->period != pwm->state.period ||
> - state->duty_cycle != pwm->state.duty_cycle) {
> - err = chip->ops->config(pwm->chip, pwm,
> - state->duty_cycle,
> - state->period);
> - if (err)
> - goto rollback;
> -
> - pwm->state.period = state->period;
> - pwm->state.duty_cycle = state->duty_cycle;
> - }
>
> if (!pwm->state.enabled) {
> err = chip->ops->enable(chip, pwm);
>
> I slightly prefer the latter patch, but if this is indeed your problem
> both should fix it for you.
The first variant has the benefit of writing all of the state through to
hardware, which is more likely to be what a proper atomic implementation
would do. Technically it shouldn't be necessary to reconfigure *and*
disable a PWM, but it's traditionally the safer way to make sure the
hardware is really "off".
In any case, I've dropped the original patch for now. Feel free to send
an updated patch with either of the above, I don't have a strong enough
preference for either, so take your pick. But given that this is riskier
than I thought, I'll defer this to v5.15.
Thierry
Attachment:
signature.asc
Description: PGP signature
