| Message ID | 20200717133753.127282-7-hdegoede@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API | expand |
On Fri, Jul 17, 2020 at 03:37:43PM +0200, Hans de Goede wrote: > Before this commit a suspend + resume of the LPSS PWM controller > would result in the controller being reset to its defaults of > output-freq = clock/256, duty-cycle=100%, until someone changes > to the output-freq and/or duty-cycle are made. > > This problem has been masked so far because the main consumer > (the i915 driver) was always making duty-cycle changes on resume. > With the conversion of the i915 driver to the atomic PWM API the > driver now only disables/enables the PWM on suspend/resume leaving > the output-freq and duty as is, triggering this problem. > > The LPSS PWM controller has a mechanism where the ctrl register value > and the actual base-unit and on-time-div values used are latched. When > software sets the SW_UPDATE bit then at the end of the current PWM cycle, > the new values from the ctrl-register will be latched into the actual > registers, and the SW_UPDATE bit will be cleared. > > The problem is that before this commit our suspend/resume handling > consisted of simply saving the PWM ctrl register on suspend and > restoring it on resume, without setting the PWM_SW_UPDATE bit. > When the controller has lost its state over a suspend/resume and thus > has been reset to the defaults, just restoring the register is not > enough. We must also set the SW_UPDATE bit to tell the controller to > latch the restored values into the actual registers. > > Fixing this problem is not as simple as just or-ing in the value which > is being restored with SW_UPDATE. If the PWM was enabled before we must > write the new settings + PWM_SW_UPDATE before setting PWM_ENABLE. > We must also wait for PWM_SW_UPDATE to become 0 again and depending on the > model we must do this either before or after the setting of PWM_ENABLE. > > All the necessary logic for doing this is already present inside > pwm_lpss_apply(), so instead of duplicating this inside the resume > handler, this commit makes the resume handler use pwm_lpss_apply() to > restore the settings when necessary. This fixes the output-freq and > duty-cycle being reset to their defaults on resume. ... > -static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, > - const struct pwm_state *state) > +static int __pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state, bool from_resume) > { > struct pwm_lpss_chip *lpwm = to_lpwm(chip); > int ret = 0; > > if (state->enabled) { > if (!pwm_is_enabled(pwm)) { > - pm_runtime_get_sync(chip->dev); > + if (!from_resume) > + pm_runtime_get_sync(chip->dev); > + > ret = pwm_lpss_prepare_enable(lpwm, pwm, state, true); > - if (ret) > + if (ret && !from_resume) > pm_runtime_put(chip->dev); > } else { > ret = pwm_lpss_prepare_enable(lpwm, pwm, state, false); > } > } else if (pwm_is_enabled(pwm)) { > pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); > - pm_runtime_put(chip->dev); > + > + if (!from_resume) > + pm_runtime_put(chip->dev); > } > > return ret; > } Maybe I'm too picky, but I would go even further and split apply to two versions static int pwm_lpss_apply_on_resume(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) > { > struct pwm_lpss_chip *lpwm = to_lpwm(chip); > > if (state->enabled) > return pwm_lpss_prepare_enable(lpwm, pwm, state, !pwm_is_enabled(pwm)); > if (pwm_is_enabled(pwm)) { > pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); > return 0; > } and another one for !from_resume. > +static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + return __pwm_lpss_apply(chip, pwm, state, false); > +} ... > + ret = __pwm_lpss_apply(&lpwm->chip, pwm, &saved_state, true); > + if (ret) > + dev_err(dev, "Error restoring state on resume\n"); I'm wondering if it's a real error why we do not bail out? Otherwise dev_warn() ?
Hi, On 7/28/20 8:57 PM, Andy Shevchenko wrote: > On Fri, Jul 17, 2020 at 03:37:43PM +0200, Hans de Goede wrote: >> Before this commit a suspend + resume of the LPSS PWM controller >> would result in the controller being reset to its defaults of >> output-freq = clock/256, duty-cycle=100%, until someone changes >> to the output-freq and/or duty-cycle are made. >> >> This problem has been masked so far because the main consumer >> (the i915 driver) was always making duty-cycle changes on resume. >> With the conversion of the i915 driver to the atomic PWM API the >> driver now only disables/enables the PWM on suspend/resume leaving >> the output-freq and duty as is, triggering this problem. >> >> The LPSS PWM controller has a mechanism where the ctrl register value >> and the actual base-unit and on-time-div values used are latched. When >> software sets the SW_UPDATE bit then at the end of the current PWM cycle, >> the new values from the ctrl-register will be latched into the actual >> registers, and the SW_UPDATE bit will be cleared. >> >> The problem is that before this commit our suspend/resume handling >> consisted of simply saving the PWM ctrl register on suspend and >> restoring it on resume, without setting the PWM_SW_UPDATE bit. >> When the controller has lost its state over a suspend/resume and thus >> has been reset to the defaults, just restoring the register is not >> enough. We must also set the SW_UPDATE bit to tell the controller to >> latch the restored values into the actual registers. >> >> Fixing this problem is not as simple as just or-ing in the value which >> is being restored with SW_UPDATE. If the PWM was enabled before we must >> write the new settings + PWM_SW_UPDATE before setting PWM_ENABLE. >> We must also wait for PWM_SW_UPDATE to become 0 again and depending on the >> model we must do this either before or after the setting of PWM_ENABLE. >> >> All the necessary logic for doing this is already present inside >> pwm_lpss_apply(), so instead of duplicating this inside the resume >> handler, this commit makes the resume handler use pwm_lpss_apply() to >> restore the settings when necessary. This fixes the output-freq and >> duty-cycle being reset to their defaults on resume. > > ... > >> -static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, >> - const struct pwm_state *state) >> +static int __pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, >> + const struct pwm_state *state, bool from_resume) >> { >> struct pwm_lpss_chip *lpwm = to_lpwm(chip); >> int ret = 0; >> >> if (state->enabled) { >> if (!pwm_is_enabled(pwm)) { >> - pm_runtime_get_sync(chip->dev); >> + if (!from_resume) >> + pm_runtime_get_sync(chip->dev); >> + >> ret = pwm_lpss_prepare_enable(lpwm, pwm, state, true); >> - if (ret) >> + if (ret && !from_resume) >> pm_runtime_put(chip->dev); >> } else { >> ret = pwm_lpss_prepare_enable(lpwm, pwm, state, false); >> } >> } else if (pwm_is_enabled(pwm)) { >> pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); >> - pm_runtime_put(chip->dev); >> + >> + if (!from_resume) >> + pm_runtime_put(chip->dev); >> } >> >> return ret; >> } > > Maybe I'm too picky, but I would go even further and split apply to two versions > > static int pwm_lpss_apply_on_resume(struct pwm_chip *chip, struct pwm_device *pwm, > const struct pwm_state *state) >> { >> struct pwm_lpss_chip *lpwm = to_lpwm(chip); >> >> if (state->enabled) >> return pwm_lpss_prepare_enable(lpwm, pwm, state, !pwm_is_enabled(pwm)); >> if (pwm_is_enabled(pwm)) { >> pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); >> return 0; >> } > > and another one for !from_resume. It is a bit picky :) But that is actually not a bad idea, although I would write it like this for more symmetry with the normal (not on_resume) apply version, while at it I also renamed the function: /* * This is a mirror of pwm_lpss_apply() without pm_runtime reference handling * for restoring the PWM state on resume. */ static int pwm_lpss_restore_state(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) { struct pwm_lpss_chip *lpwm = to_lpwm(chip); int ret = 0; if (state->enabled) ret = pwm_lpss_prepare_enable(lpwm, pwm, state, !pwm_is_enabled(pwm)); else if (pwm_is_enabled(pwm)) pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); return ret; } Would that work for you? >> +static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, >> + const struct pwm_state *state) >> +{ >> + return __pwm_lpss_apply(chip, pwm, state, false); >> +} > > ... > >> + ret = __pwm_lpss_apply(&lpwm->chip, pwm, &saved_state, true); >> + if (ret) >> + dev_err(dev, "Error restoring state on resume\n"); > > I'm wondering if it's a real error why we do not bail out? > Otherwise dev_warn() ? It is a real error, but a single PWM chip might have multiple controllers and bailing out early would mean not even trying to restore the state on the other controllers. As for propagating the error, AFAIK the pm framework does not do anything with resume errors other then log an extra error. Regards, Hans
On Tue, Jul 28, 2020 at 09:55:22PM +0200, Hans de Goede wrote: > On 7/28/20 8:57 PM, Andy Shevchenko wrote: > > On Fri, Jul 17, 2020 at 03:37:43PM +0200, Hans de Goede wrote: ... > > Maybe I'm too picky, but I would go even further and split apply to two versions > > > > static int pwm_lpss_apply_on_resume(struct pwm_chip *chip, struct pwm_device *pwm, > > const struct pwm_state *state) > > > { > > > struct pwm_lpss_chip *lpwm = to_lpwm(chip); > > > if (state->enabled) > > > return pwm_lpss_prepare_enable(lpwm, pwm, state, !pwm_is_enabled(pwm)); > > > if (pwm_is_enabled(pwm)) { > > > pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); > > > return 0; > > > } > > > > and another one for !from_resume. > > It is a bit picky :) But that is actually not a bad idea, although I would write > it like this for more symmetry with the normal (not on_resume) apply version, > while at it I also renamed the function: > > /* > * This is a mirror of pwm_lpss_apply() without pm_runtime reference handling > * for restoring the PWM state on resume. > */ > static int pwm_lpss_restore_state(struct pwm_chip *chip, struct pwm_device *pwm, > const struct pwm_state *state) > { > struct pwm_lpss_chip *lpwm = to_lpwm(chip); > int ret = 0; > > if (state->enabled) > ret = pwm_lpss_prepare_enable(lpwm, pwm, state, !pwm_is_enabled(pwm)); > else if (pwm_is_enabled(pwm)) > pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); > > return ret; > } > > Would that work for you? Yes. ... > > > + ret = __pwm_lpss_apply(&lpwm->chip, pwm, &saved_state, true); > > > + if (ret) > > > + dev_err(dev, "Error restoring state on resume\n"); > > > > I'm wondering if it's a real error why we do not bail out? > > Otherwise dev_warn() ? > > It is a real error, but a single PWM chip might have multiple controllers > and bailing out early would mean not even trying to restore the state on > the other controllers. As for propagating the error, AFAIK the pm framework > does not do anything with resume errors other then log an extra error. OK.
Hi, On 7/29/20 10:12 AM, Andy Shevchenko wrote: > On Tue, Jul 28, 2020 at 09:55:22PM +0200, Hans de Goede wrote: >> On 7/28/20 8:57 PM, Andy Shevchenko wrote: >>> On Fri, Jul 17, 2020 at 03:37:43PM +0200, Hans de Goede wrote: > > ... > >>> Maybe I'm too picky, but I would go even further and split apply to two versions >>> >>> static int pwm_lpss_apply_on_resume(struct pwm_chip *chip, struct pwm_device *pwm, >>> const struct pwm_state *state) >>>> { >>>> struct pwm_lpss_chip *lpwm = to_lpwm(chip); >>>> if (state->enabled) >>>> return pwm_lpss_prepare_enable(lpwm, pwm, state, !pwm_is_enabled(pwm)); >>>> if (pwm_is_enabled(pwm)) { >>>> pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); >>>> return 0; >>>> } >>> >>> and another one for !from_resume. >> >> It is a bit picky :) But that is actually not a bad idea, although I would write >> it like this for more symmetry with the normal (not on_resume) apply version, >> while at it I also renamed the function: >> >> /* >> * This is a mirror of pwm_lpss_apply() without pm_runtime reference handling >> * for restoring the PWM state on resume. >> */ >> static int pwm_lpss_restore_state(struct pwm_chip *chip, struct pwm_device *pwm, >> const struct pwm_state *state) >> { >> struct pwm_lpss_chip *lpwm = to_lpwm(chip); >> int ret = 0; >> >> if (state->enabled) >> ret = pwm_lpss_prepare_enable(lpwm, pwm, state, !pwm_is_enabled(pwm)); >> else if (pwm_is_enabled(pwm)) >> pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); >> >> return ret; >> } >> >> Would that work for you? > > Yes. Ok, I've added the suggested/discussed helper in my personal tree. Is it ok if I add your Reviewed-by with that change in place. This is the last unreviewed bit, so I would rather not respin the series just for this (there will be one more respin when I rebase it on 5.9-rc1). If you want to check out what the patch looks like now, the new version from my personal tree is here: https://github.com/jwrdegoede/linux-sunxi/commit/e4869830d88bb8cb8251718e0086ac189abc0f56 Regards, Hans
On Sun, Aug 02, 2020 at 10:51:34PM +0200, Hans de Goede wrote: > On 7/29/20 10:12 AM, Andy Shevchenko wrote: ... > Ok, I've added the suggested/discussed helper in my personal tree. Is it ok > if I add your Reviewed-by with that change in place. Yes, go ahead! > This is the last unreviewed > bit, so I would rather not respin the series just for this (there will be one > more respin when I rebase it on 5.9-rc1). > > If you want to check out what the patch looks like now, the new version from > my personal tree is here: > > https://github.com/jwrdegoede/linux-sunxi/commit/e4869830d88bb8cb8251718e0086ac189abc0f56 Thanks, looks good to me.
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index 8a136ba2a583..cf4eaf7ef2a2 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -143,29 +143,39 @@ static int pwm_lpss_prepare_enable(struct pwm_lpss_chip *lpwm, return 0; } -static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, - const struct pwm_state *state) +static int __pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state, bool from_resume) { struct pwm_lpss_chip *lpwm = to_lpwm(chip); int ret = 0; if (state->enabled) { if (!pwm_is_enabled(pwm)) { - pm_runtime_get_sync(chip->dev); + if (!from_resume) + pm_runtime_get_sync(chip->dev); + ret = pwm_lpss_prepare_enable(lpwm, pwm, state, true); - if (ret) + if (ret && !from_resume) pm_runtime_put(chip->dev); } else { ret = pwm_lpss_prepare_enable(lpwm, pwm, state, false); } } else if (pwm_is_enabled(pwm)) { pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); - pm_runtime_put(chip->dev); + + if (!from_resume) + pm_runtime_put(chip->dev); } return ret; } +static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + return __pwm_lpss_apply(chip, pwm, state, false); +} + static void pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_state *state) { @@ -278,10 +288,40 @@ EXPORT_SYMBOL_GPL(pwm_lpss_suspend); int pwm_lpss_resume(struct device *dev) { struct pwm_lpss_chip *lpwm = dev_get_drvdata(dev); - int i; + struct pwm_state saved_state; + struct pwm_device *pwm; + int i, ret; + u32 ctrl; - for (i = 0; i < lpwm->info->npwm; i++) - writel(lpwm->saved_ctrl[i], lpwm->regs + i * PWM_SIZE + PWM); + for (i = 0; i < lpwm->info->npwm; i++) { + pwm = &lpwm->chip.pwms[i]; + + ctrl = pwm_lpss_read(pwm); + /* If we did not reach S0i3/S3 the controller keeps its state */ + if (ctrl == lpwm->saved_ctrl[i]) + continue; + + /* + * We cannot just blindly restore the old value here. Since we + * are changing the settings we must set SW_UPDATE and if the + * PWM was enabled before we must write the new settings + + * PWM_SW_UPDATE before setting PWM_ENABLE. We must also wait + * for PWM_SW_UPDATE to become 0 again and depending on the + * model we must do this either before or after the setting of + * PWM_ENABLE. + * So instead of reproducing all the code from pwm_apply() here, + * we just reapply the state as stored in pwm->state. + */ + saved_state = pwm->state; + /* + * Update enabled to its actual setting for the + * enabled<->disabled transitions inside apply(). + */ + pwm->state.enabled = !!(ctrl & PWM_ENABLE); + ret = __pwm_lpss_apply(&lpwm->chip, pwm, &saved_state, true); + if (ret) + dev_err(dev, "Error restoring state on resume\n"); + } return 0; }
Before this commit a suspend + resume of the LPSS PWM controller would result in the controller being reset to its defaults of output-freq = clock/256, duty-cycle=100%, until someone changes to the output-freq and/or duty-cycle are made. This problem has been masked so far because the main consumer (the i915 driver) was always making duty-cycle changes on resume. With the conversion of the i915 driver to the atomic PWM API the driver now only disables/enables the PWM on suspend/resume leaving the output-freq and duty as is, triggering this problem. The LPSS PWM controller has a mechanism where the ctrl register value and the actual base-unit and on-time-div values used are latched. When software sets the SW_UPDATE bit then at the end of the current PWM cycle, the new values from the ctrl-register will be latched into the actual registers, and the SW_UPDATE bit will be cleared. The problem is that before this commit our suspend/resume handling consisted of simply saving the PWM ctrl register on suspend and restoring it on resume, without setting the PWM_SW_UPDATE bit. When the controller has lost its state over a suspend/resume and thus has been reset to the defaults, just restoring the register is not enough. We must also set the SW_UPDATE bit to tell the controller to latch the restored values into the actual registers. Fixing this problem is not as simple as just or-ing in the value which is being restored with SW_UPDATE. If the PWM was enabled before we must write the new settings + PWM_SW_UPDATE before setting PWM_ENABLE. We must also wait for PWM_SW_UPDATE to become 0 again and depending on the model we must do this either before or after the setting of PWM_ENABLE. All the necessary logic for doing this is already present inside pwm_lpss_apply(), so instead of duplicating this inside the resume handler, this commit makes the resume handler use pwm_lpss_apply() to restore the settings when necessary. This fixes the output-freq and duty-cycle being reset to their defaults on resume. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v5: - The changes to pwm_lpss_apply() are much cleaner now thanks to the new pwm_lpss_prepare_enable() helper. Changes in v3: - This replaces the "pwm: lpss: Set SW_UPDATE bit when enabling the PWM" patch from previous versions of this patch-set, which really was a hack working around the resume issue which this patch fixes properly. --- drivers/pwm/pwm-lpss.c | 56 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 8 deletions(-)