Message ID | 20170220201657.24801-2-hdegoede@redhat.com |
---|---|
State | Superseded |
Headers | show |
+Andy Hi, Thanks for the patch! On Mon, Feb 20, 2017 at 10:16:55PM +0200, Hans de Goede wrote: > At least on cherrytrail, the update bit will never go low when the > enabled bit is not set. > > This causes the backlight on my cube iwork8 air tablet to never go on > again after being turned off, because the enable path does: > > pwm_lpss_prepare(); > ret = pwm_lpss_update(pwm); > if (ret) > return ret; > pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE); > > And the pwm_lpss_update() call fails, as the setting of the > UPDATE bit never gets acked, because the ENABLE bit is not set. > > Subsequent calls then all fail because of the pwm_lpss_is_updating() > check done by pwm_lpss_apply(). > > This commit fixes this by setting the enable bit before calling > pwm_lpss_update(). > > Fixes: 10d56a4cb1c6 ("pwm: lpss: Avoid reconfiguring while UPDATE bit...") > Cc: Ilkka Koskinen <ilkka.koskinen@intel.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/pwm/pwm-lpss.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c > index 689d2c1..6c99abc 100644 > --- a/drivers/pwm/pwm-lpss.c > +++ b/drivers/pwm/pwm-lpss.c > @@ -137,12 +137,12 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, > return ret; > } > pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period); > + pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE); > ret = pwm_lpss_update(pwm); The BXT documentation that I have recommends to set update bit before enable one. However, based on your experiment on Cherryview, we still have to set it before read_poll_timeout(). Andy, should we indeed remove the return value from apply() and just print a warning like Mika initially suggested? --Ilkka > if (ret) { > pm_runtime_put(chip->dev); > return ret; > } > - pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE); > } else { > ret = pwm_lpss_is_updating(pwm); > if (ret) > -- > 2.9.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2017-02-23 at 00:06 -0800, Ilkka Koskinen wrote: > > At least on cherrytrail, the update bit will never go low when the > > enabled bit is not set. > > > > This causes the backlight on my cube iwork8 air tablet to never go > > on > > again after being turned off, because the enable path does: > > > > pwm_lpss_prepare(); > > ret = pwm_lpss_update(pwm); > > if (ret) > > return ret; > > pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE); > > > > And the pwm_lpss_update() call fails, as the setting of the > > UPDATE bit never gets acked, because the ENABLE bit is not set. > > > > Subsequent calls then all fail because of the pwm_lpss_is_updating() > > check done by pwm_lpss_apply(). > > > > This commit fixes this by setting the enable bit before calling > > pwm_lpss_update(). This might break other systems. > > @@ -137,12 +137,12 @@ static int pwm_lpss_apply(struct pwm_chip > > *chip, struct pwm_device *pwm, > > return ret; > > } > > pwm_lpss_prepare(lpwm, pwm, state- > > >duty_cycle, state->period); > > + pwm_lpss_write(pwm, pwm_lpss_read(pwm) | > > PWM_ENABLE); > > ret = pwm_lpss_update(pwm); > > The BXT documentation that I have recommends to set update bit before > enable > one. We have the same work flow for all SoCs that have this PWM IP. I suspect it might be a silicon bug somewhere, but I have never heard of it. > However, based on your experiment on Cherryview, we still have to set > it > before read_poll_timeout(). I would test and confirm the issue on our machines to see that the fix doesn't break the rest. Seems like we have several subtle different implementation of it: BYT, CHT, MRFLD, BXT. > Andy, should we indeed remove the return value from apply() and just > print a warning > like Mika initially suggested? I definitely consider it better than Hans' initial proposal. Hans, can you just replace return ret; by dev_warn(..., "UPDATE bit is not cleared!\n"); and test it?
Hi, On 28-02-17 10:46, Andy Shevchenko wrote: > On Thu, 2017-02-23 at 00:06 -0800, Ilkka Koskinen wrote: > >>> At least on cherrytrail, the update bit will never go low when the >>> enabled bit is not set. >>> >>> This causes the backlight on my cube iwork8 air tablet to never go >>> on >>> again after being turned off, because the enable path does: >>> >>> pwm_lpss_prepare(); >>> ret = pwm_lpss_update(pwm); >>> if (ret) >>> return ret; >>> pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE); >>> >>> And the pwm_lpss_update() call fails, as the setting of the >>> UPDATE bit never gets acked, because the ENABLE bit is not set. >>> >>> Subsequent calls then all fail because of the pwm_lpss_is_updating() >>> check done by pwm_lpss_apply(). >>> > >>> This commit fixes this by setting the enable bit before calling >>> pwm_lpss_update(). > > This might break other systems. > >>> @@ -137,12 +137,12 @@ static int pwm_lpss_apply(struct pwm_chip >>> *chip, struct pwm_device *pwm, >>> return ret; >>> } >>> pwm_lpss_prepare(lpwm, pwm, state- >>>> duty_cycle, state->period); >>> + pwm_lpss_write(pwm, pwm_lpss_read(pwm) | >>> PWM_ENABLE); >>> ret = pwm_lpss_update(pwm); >> >> The BXT documentation that I have recommends to set update bit before >> enable >> one. > > We have the same work flow for all SoCs that have this PWM IP. I suspect > it might be a silicon bug somewhere, but I have never heard of it. > >> However, based on your experiment on Cherryview, we still have to set >> it >> before read_poll_timeout(). > > I would test and confirm the issue on our machines to see that the fix > doesn't break the rest. Seems like we have several subtle different > implementation of it: BYT, CHT, MRFLD, BXT. > >> Andy, should we indeed remove the return value from apply() and just >> print a warning >> like Mika initially suggested? > > I definitely consider it better than Hans' initial proposal. > > Hans, can you just replace > > return ret; > > by > > dev_warn(..., "UPDATE bit is not cleared!\n"); > > and test it? Sure, done, as expected that fixes it, but not really in a pretty way, now at boot and every time the screen goes into DPMS / off I get: [ 9.461180] pwm-lpss 80862288:00: PWM_SW_UPDATE was not cleared [ 9.461190] pwm-lpss 80862288:00: UPDATE bit is not cleared! [ 124.642002] pwm-lpss 80862288:00: PWM_SW_UPDATE was not cleared [ 124.642007] pwm-lpss 80862288:00: UPDATE bit is not cleared! I believe it would be better to split pwm_lpss_update into setting the update-bit and pwm_lpss_wait_for_update, and move the pwm_lpss_wait_for_update call in the enable path to after setting the enable-bit. I've just finished testing a patch which does that, probably easier to discuss it when you can see the code, so I will send that out as a v2 of this one. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index 689d2c1..6c99abc 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -137,12 +137,12 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, return ret; } pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period); + pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE); ret = pwm_lpss_update(pwm); if (ret) { pm_runtime_put(chip->dev); return ret; } - pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE); } else { ret = pwm_lpss_is_updating(pwm); if (ret)
At least on cherrytrail, the update bit will never go low when the enabled bit is not set. This causes the backlight on my cube iwork8 air tablet to never go on again after being turned off, because the enable path does: pwm_lpss_prepare(); ret = pwm_lpss_update(pwm); if (ret) return ret; pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE); And the pwm_lpss_update() call fails, as the setting of the UPDATE bit never gets acked, because the ENABLE bit is not set. Subsequent calls then all fail because of the pwm_lpss_is_updating() check done by pwm_lpss_apply(). This commit fixes this by setting the enable bit before calling pwm_lpss_update(). Fixes: 10d56a4cb1c6 ("pwm: lpss: Avoid reconfiguring while UPDATE bit...") Cc: Ilkka Koskinen <ilkka.koskinen@intel.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/pwm/pwm-lpss.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)