On the LPSS PWM controller found on Bay Trail (BYT) and Cherry Trail (CHT) platforms, the following sequence results in an output duty-cycle of 100% independent of what the duty-cycle requested in the ctrl-reg is:
1. Clear ENABLE bit in ctrl register 2. Let the machine reach a S0i3 low power state 3. Set the ENABLE bit in the ctrl register
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. Note on BYT and CHT the ENABLE bit must be set before waiting for the SW_UPDATE bit to clear, otherwise the SW_UPDATE bit will never clear (this is indicated in the pwm-lpss.c code by lpwm->info->bypass being false).
My theory about why this is happening is that when we hit S0i3 the part which holds the latched values gets turned off and when its turned back on again at least the on-time-div value has been lost and gets reset to 0 which corresponds to an output duty-cycle of 100%. Testing has shown that setting the SW_UPDATE bit to request latching the ctrl-register values into the actual registers (again) fixes this, confirming this theory.
In the past there have been issues where setting the SW_UPDATE bit when nothing has changed would lead to the next ctrl register changing being ignored, see commit 2153bbc12f77 ("pwm: lpss: Only set update bit if we are actually changing the settings"), so we should only set the SW_UPDATE bit when actually changing the ENABLE bit from 0 to 1.
When looking into how to fix this I noticed that on platforms where lpwm->info->bypass == false we unnecessarily do 2 read-modify-write cycles of the ctrl register, one to set the base-unit and on-time-div, immediately followed by another to set the ENABLE bit.
This commit fixes the 100% duty cycle issue by folding the setting of the ENABLE bit into pwm_lpss_prepare(), which already checks if any bits in the ctrl-register have actually changed and if that is the case then sets the SW_UPDATE bit.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/pwm/pwm-lpss.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index a764e062103b..2cb0e2a9c08c 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -80,7 +80,7 @@ static inline int pwm_lpss_is_updating(struct pwm_device *pwm) }
static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm, - int duty_ns, int period_ns) + int duty_ns, int period_ns, bool enable) { unsigned long long on_time_div; unsigned long c = lpwm->info->clk_rate, base_unit_range; @@ -115,6 +115,8 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm, ctrl &= ~((base_unit_range - 1) << PWM_BASE_UNIT_SHIFT); ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT; ctrl |= on_time_div; + if (enable) + ctrl |= PWM_ENABLE;
if (orig_ctrl != ctrl) { pwm_lpss_write(pwm, ctrl); @@ -142,8 +144,9 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, pm_runtime_put(chip->dev); return ret; } - pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period); - pwm_lpss_cond_enable(pwm, lpwm->info->bypass == false); + pwm_lpss_prepare(lpwm, pwm, + state->duty_cycle, state->period, + lpwm->info->bypass == false); ret = pwm_lpss_wait_for_update(pwm); if (ret) { pm_runtime_put(chip->dev); @@ -154,7 +157,8 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, ret = pwm_lpss_is_updating(pwm); if (ret) return ret; - pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period); + pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, + state->period, false); return pwm_lpss_wait_for_update(pwm); } } else if (pwm_is_enabled(pwm)) {