From: Thierry Reding <thierry.reding@gmail.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>,
Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
Linux PWM List <linux-pwm@vger.kernel.org>,
Lee Jones <lee.jones@linaro.org>,
Sascha Hauer <kernel@pengutronix.de>
Subject: Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent
Date: Wed, 30 Jun 2021 19:18:22 +0200 [thread overview]
Message-ID: <YNynXgD2jhUFLZig@orome.fritz.box> (raw)
In-Reply-To: <20210630162128.ufmul6euxkwnou25@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 6419 bytes --]
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@pengutronix.de> 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@pengutronix.de>
> > > >
> > > > 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2021-06-30 17:16 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-11 16:04 [PATCH] pwm: Ensure for legacy drivers that pwm->state stays consistent Uwe Kleine-König
2021-05-01 16:09 ` [PATCH v2] " Uwe Kleine-König
2021-06-28 11:46 ` Thierry Reding
2021-06-28 17:24 ` Uwe Kleine-König
2021-06-29 19:44 ` Geert Uytterhoeven
2021-06-30 6:48 ` Uwe Kleine-König
2021-06-30 10:22 ` Geert Uytterhoeven
2021-06-30 16:21 ` Uwe Kleine-König
2021-06-30 17:08 ` Geert Uytterhoeven
2021-06-30 17:18 ` Thierry Reding [this message]
2021-06-29 19:52 ` Geert Uytterhoeven
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YNynXgD2jhUFLZig@orome.fritz.box \
--to=thierry.reding@gmail.com \
--cc=geert@linux-m68k.org \
--cc=kernel@pengutronix.de \
--cc=lee.jones@linaro.org \
--cc=linux-pwm@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=u.kleine-koenig@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).