On Sun, Aug 30, 2020 at 02:57:43PM +0200, Hans de Goede wrote:
This commit removes a check where we would skip writing the ctrl register and then setting the update bit in case the ctrl register already contains the correct values.
In a perfect world skipping the update should be fine in these cases, but on Cherry Trail devices the AML code in the GFX0 devices' PS0 and PS3 methods messes with the PWM controller.
The "ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase" patch earlier in this series stops the GFX0._PS0 method from messing with the PWM controller and on the DSDT-s inspected sofar the _PS3 method only reads from the PWM controller (and turns it off before we get a change to do so):
{ PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */ PSAT |= 0x03 Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */ }The PWM controller getting turning off before we do this ourselves is a bit annoying but not really an issue.
The problem this patch fixes comes from a new variant of the GFX0._PS3 code messing with the PWM controller found on the Acer One 10 S1003 (1):
{ PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */ PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */ PWMT &= 0xFF0000FF PWMT |= 0xC0000000 PWMC = PWMT /* \_SB_.PCI0.GFX0.PWMT */ PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */ Sleep (0x64) PWMB &= 0x3FFFFFFF PWMC = PWMB /* \_SB_.PCI0.GFX0.PWMB */ PSAT |= 0x03 Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */ }This "beautiful" piece of code clears the base-unit part of the ctrl-reg, which effectively disables the controller, and it sets the update flag to apply this change. Then after this it restores the original ctrl-reg value, so we do not see it has mucked with the controller.
*But* it does not set the update flag when restoring the original value. So the check to see if we can skip writing the ctrl register succeeds but since the update flag was not set, the old base-unit value of 0 is still in use and the PWM controller is effectively disabled.
IOW this PWM controller poking means that we cannot trust the base-unit / on-time-div value we read back from the PWM controller since it may not have been applied/committed. Thus we must always update the ctrl-register and set the update bit.
- And once I knew what to look for also in a bunch of other devices
including the popular Lenovo Ideapad Miix 310 and 320 models and various Medion models.
Despite the above mentioned issue I'm always in favour of not micro-optimizing I/O. Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
Signed-off-by: Hans de Goede hdegoede@redhat.com
Changes in v8:
- New patch in v8 of this patch-set
drivers/pwm/pwm-lpss.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index 9a7400c6fb6e..20f6b6d6f874 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -85,7 +85,7 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm, unsigned long long on_time_div; unsigned long c = lpwm->info->clk_rate, base_unit_range; unsigned long long base_unit, freq = NSEC_PER_SEC;
- u32 orig_ctrl, ctrl;
u32 ctrl;
do_div(freq, period_ns);
@@ -104,16 +104,14 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm, do_div(on_time_div, period_ns); on_time_div = 255ULL - on_time_div;
- orig_ctrl = ctrl = pwm_lpss_read(pwm);
- ctrl = pwm_lpss_read(pwm); ctrl &= ~PWM_ON_TIME_DIV_MASK; ctrl &= ~((base_unit_range - 1) << PWM_BASE_UNIT_SHIFT); ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT; ctrl |= on_time_div;
- if (orig_ctrl != ctrl) {
pwm_lpss_write(pwm, ctrl);pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);- }
- pwm_lpss_write(pwm, ctrl);
- pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
}
static inline void pwm_lpss_cond_enable(struct pwm_device *pwm, bool cond)
2.28.0