1. 06 Sep, 2020 7 commits
    • Hans de Goede's avatar
      pwm: lpss: Remove suspend/resume handlers · 547d9e92
      Hans de Goede authored
      PWM controller drivers should not restore the PWM state on resume. The
      convention is that PWM consumers do this by calling pwm_apply_state(),
      so that it can be done at the exact moment when the consumer needs
      the state to be stored, avoiding e.g. backlight flickering.
      
      The only in kernel consumers of the pwm-lpss code, the i915 driver
      and the pwm-class sysfs interface code both correctly restore the
      state on resume, so there is no need to do this in the pwm-lpss code.
      
      More-over the removed resume handler is buggy, since it blindly
      restores the ctrl-register contents without setting the update
      bit, which is necessary to get the controller to actually use/apply
      the restored base-unit and on-time-div values.
      Acked-by: default avatarThierry Reding <thierry.reding@gmail.com>
      Signed-off-by: default avatarHans de Goede <hdegoede@redhat.com>
      Reviewed-by: default avatarAndy Shevchenko <andriy.shevchenko@linux.intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20200903112337.4113-8-hdegoede@redhat.com
      547d9e92
    • Hans de Goede's avatar
      pwm: lpss: Make pwm_lpss_apply() not rely on existing hardware state · d6d54bac
      Hans de Goede authored
      Before this commit pwm_lpss_apply() was assuming 2 pre-conditions
      were met by the existing hardware state:
      
      1. That the base-unit and on-time-div read back from the
      control register are those actually in use, so that it
      can skip setting the update bit if the read-back value
      matches the desired values.
      
      2. That the controller is enabled when the cached
      pwm_state.enabled says that the controller is enabled.
      
      As the long history of fixes for subtle (often suspend/resume)
      lpss-pwm issues shows, these assumptions are not necessary
      always true.
      
      1. Specifically is not true on some (*) Cherry Trail devices
      with a nasty GFX0._PS3 method which: a. saves the ctrl reg value.
      b. sets the base-unit to 0 and writes the update bit to apply/commit
      c. restores the original ctrl value without setting the update bit,
      so that the 0 base-unit value is still in use.
      
      2. Assumption 2. currently is true, but only because of the code which
      saves/restores the state on suspend/resume. By convention restoring the
      PWM state should be done by the PWM consumer and the presence of this
      code in the pmw-lpss driver is a bug. Therefor the save/restore code will
      be dropped in the next patch in this series, after which this assumption
      also is no longer true.
      
      This commit changes the pwm_lpss_apply() to not make any assumptions about
      the state the hardware is in. Instead it makes pwm_lpss_apply() always
      fully program the PWM controller, making it much less fragile.
      
      *) Seen on the Acer One 10 S1003, Lenovo Ideapad Miix 310 and 320 models
      and various Medion models.
      Acked-by: default avatarThierry Reding <thierry.reding@gmail.com>
      Signed-off-by: default avatarHans de Goede <hdegoede@redhat.com>
      Reviewed-by: default avatarAndy Shevchenko <andriy.shevchenko@linux.intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20200903112337.4113-7-hdegoede@redhat.com
      d6d54bac
    • Hans de Goede's avatar
      pwm: lpss: Add pwm_lpss_prepare_enable() helper · 092d83e3
      Hans de Goede authored
      In the not-enabled -> enabled path pwm_lpss_apply() needs to get a
      runtime-pm reference; and then on any errors it needs to release it
      again.
      
      This leads to somewhat hard to read code. This commit introduces a new
      pwm_lpss_prepare_enable() helper and moves all the steps necessary for
      the not-enabled -> enabled transition there, so that we can error check
      the entire transition in a single place and only have one pm_runtime_put()
      on failure call site.
      
      While working on this I noticed that the enabled -> enabled (update
      settings) path was quite similar, so I've added an enable parameter to
      the new pwm_lpss_prepare_enable() helper, which allows using it in that
      path too.
      Suggested-by: default avatarAndy Shevchenko <andriy.shevchenko@linux.intel.com>
      Reviewed-by: default avatarAndy Shevchenko <andriy.shevchenko@linux.intel.com>
      Acked-by: default avatarThierry Reding <thierry.reding@gmail.com>
      Signed-off-by: default avatarHans de Goede <hdegoede@redhat.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20200903112337.4113-6-hdegoede@redhat.com
      092d83e3
    • Hans de Goede's avatar
      pwm: lpss: Add range limit check for the base_unit register value · ef9f60da
      Hans de Goede authored
      When the user requests a high enough period ns value, then the
      calculations in pwm_lpss_prepare() might result in a base_unit value of 0.
      
      But according to the data-sheet the way the PWM controller works is that
      each input clock-cycle the base_unit gets added to a N bit counter and
      that counter overflowing determines the PWM output frequency. Adding 0
      to the counter is a no-op. The data-sheet even explicitly states that
      writing 0 to the base_unit bits will result in the PWM outputting a
      continuous 0 signal.
      
      When the user requestes a low enough period ns value, then the
      calculations in pwm_lpss_prepare() might result in a base_unit value
      which is bigger then base_unit_range - 1. Currently the codes for this
      deals with this by applying a mask:
      
      	base_unit &= (base_unit_range - 1);
      
      But this means that we let the value overflow the range, we throw away the
      higher bits and store whatever value is left in the lower bits into the
      register leading to a random output frequency, rather then clamping the
      output frequency to the highest frequency which the hardware can do.
      
      This commit fixes both issues by clamping the base_unit value to be
      between 1 and (base_unit_range - 1).
      
      Fixes: 684309e5 ("pwm: lpss: Avoid potential overflow of base_unit")
      Reviewed-by: default avatarAndy Shevchenko <andriy.shevchenko@linux.intel.com>
      Acked-by: default avatarThierry Reding <thierry.reding@gmail.com>
      Signed-off-by: default avatarHans de Goede <hdegoede@redhat.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20200903112337.4113-5-hdegoede@redhat.com
      ef9f60da
    • Hans de Goede's avatar
      pwm: lpss: Fix off by one error in base_unit math in pwm_lpss_prepare() · 181f4d2f
      Hans de Goede authored
      According to the data-sheet the way the PWM controller works is that
      each input clock-cycle the base_unit gets added to a N bit counter and
      that counter overflowing determines the PWM output frequency.
      
      So assuming e.g. a 16 bit counter this means that if base_unit is set to 1,
      after 65535 input clock-cycles the counter has been increased from 0 to
      65535 and it will overflow on the next cycle, so it will overflow after
      every 65536 clock cycles and thus the calculations done in
      pwm_lpss_prepare() should use 65536 and not 65535.
      
      This commit fixes this. Note this also aligns the calculations in
      pwm_lpss_prepare() with those in pwm_lpss_get_state().
      
      Note this effectively reverts commit 684309e5 ("pwm: lpss: Avoid
      potential overflow of base_unit"). The next patch in this series really
      fixes the potential overflow of the base_unit value.
      
      Fixes: 684309e5 ("pwm: lpss: Avoid potential overflow of base_unit")
      Reviewed-by: default avatarAndy Shevchenko <andriy.shevchenko@linux.intel.com>
      Acked-by: default avatarUwe Kleine-König <u.kleine-koenig@pengutronix.de>
      Acked-by: default avatarThierry Reding <thierry.reding@gmail.com>
      Signed-off-by: default avatarHans de Goede <hdegoede@redhat.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20200903112337.4113-4-hdegoede@redhat.com
      181f4d2f
    • Hans de Goede's avatar
      ACPI / LPSS: Save Cherry Trail PWM ctx registers only once (at activation) · 15aa5e4c
      Hans de Goede authored
      The DSDTs on most Cherry Trail devices have an ugly clutch where the PWM
      controller gets turned off from the _PS3 method of the graphics-card dev:
      
                  Method (_PS3, 0, Serialized)  // _PS3: Power State 3
                  {
                      ...
                                  PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
                                  PSAT |= 0x03
                                  Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
                      ...
                  }
      
      Where PSAT is the power-status register of the PWM controller.
      
      Since the i915 driver will do a pwm_get on the pwm device as it uses it to
      control the LCD panel backlight, there is a device-link marking the i915
      device as a consumer of the pwm device. So that the PWM controller will
      always be suspended after the i915 driver suspends (which is the right
      thing to do). This causes the above GFX0 PS3 AML code to run before
      acpi_lpss.c calls acpi_lpss_save_ctx().
      
      So on these devices the PWM controller will already be off when
      acpi_lpss_save_ctx() runs. This causes it to read/save all 1-s (0xffffffff)
      as ctx register values.
      
      When these bogus values get restored on resume the PWM controller actually
      keeps working, since most bits are reserved, but this does set bit 3 of
      the LPSS General purpose register, which for the PWM controller has the
      following function: "This bit is re-used to support 32kHz slow mode.
      Default is 19.2MHz as PWM source clock".
      
      This causes the clock of the PWM controller to switch from 19.2MHz to
      32KHz, which is a slow-down of a factor 600. Surprisingly enough so far
      there have been few bug reports about this. This is likely because the
      i915 driver was hardcoding the PWM frequency to 46 KHz, which divided
      by 600 would result in a PWM frequency of approx. 78 Hz, which mostly
      still works fine. There are some bug reports about the LCD backlight
      flickering after suspend/resume which are likely caused by this issue.
      
      But with the upcoming patch-series to finally switch the i915 drivers
      code for external PWM controllers to use the atomic API and to honor
      the PWM frequency specified in the video BIOS (VBT), this becomes a much
      bigger problem. On most cases the VBT specifies either 200 Hz or 20
      KHz as PWM frequency, which with the mentioned issue ends up being either
      1/3 Hz, where the backlight actually visible blinks on and off every 3s,
      or in 33 Hz and horrible flickering of the backlight.
      
      There are a number of possible solutions to this problem:
      
      1. Make acpi_lpss_save_ctx() run before GFX0._PS3
       Pro: Clean solution from pov of not medling with save/restore ctx code
       Con: As mentioned the current ordering is the right thing to do
       Con: Requires assymmetry in at what suspend/resume phase we do the save vs
            restore, requiring more suspend/resume ordering hacks in already
            convoluted acpi_lpss.c suspend/resume code.
      2. Do some sort of save once mode for the LPSS ctx
       Pro: Reasonably clean
       Con: Needs a new LPSS flag + code changes to handle the flag
      3. Detect we have failed to save the ctx registers and do not restore them
       Pro: Not PWM specific, might help with issues on other LPSS devices too
       Con: If we can get away with not restoring the ctx why bother with it at
            all?
      4. Do not save the ctx for CHT PWM controllers
       Pro: Clean, as simple as dropping a flag?
       Con: Not so simple as dropping a flag, needs a new flag to ensure that
            we still do lpss_deassert_reset() on device activation.
      5. Make the pwm-lpss code fixup the LPSS-context registers
       Pro: Keeps acpi_lpss.c code clean
       Con: Moves knowledge of LPSS-context into the pwm-lpss.c code
      
      1 and 5 both do not seem to be a desirable way forward.
      
      3 and 4 seem ok, but they both assume that restoring the LPSS-context
      registers is not necessary. I have done a couple of test and those do
      show that restoring the LPSS-context indeed does not seem to be necessary
      on devices using s2idle suspend (and successfully reaching S0i3). But I
      have no hardware to test deep / S3 suspend. So I'm not sure that not
      restoring the context is safe.
      
      That leaves solution 2, which is about as simple / clean as 3 and 4,
      so this commit fixes the described problem by implementing a new
      LPSS_SAVE_CTX_ONCE flag and setting that for the CHT PWM controllers.
      Acked-by: default avatarRafael J. Wysocki <rafael.j.wysocki@intel.com>
      Signed-off-by: default avatarHans de Goede <hdegoede@redhat.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20200903112337.4113-3-hdegoede@redhat.com
      15aa5e4c
    • Hans de Goede's avatar
      ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase · 5e31ee84
      Hans de Goede authored
      The DSDTs on most Cherry Trail devices have an ugly clutch where the PWM
      controller gets poked from the _PS0 method of the graphics-card device:
      
      	Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
      	If (((Local0 & 0x03) == 0x03))
      	{
      	    PSAT &= 0xFFFFFFFC
      	    Local1 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
      	    RSTA = Zero
      	    RSTF = Zero
      	    RSTA = One
      	    RSTF = One
      	    PWMB |= 0xC0000000
      	    PWMC = PWMB /* \_SB_.PCI0.GFX0.PWMB */
      	}
      
      Where PSAT is the power-status register of the PWM controller, so if it
      is in D3 when the GFX0 device's PS0 method runs then it will turn it on
      and restore the PWM ctrl register value it saved from its PS3 handler.
      Note not only does it restore it, it ors it with 0xC0000000 turning it
      on at a time where we may not want it to get turned on at all.
      
      The pwm_get call which the i915 driver does to get a reference to the
      PWM controller, already adds a device-link making the GFX0 device a
      consumer of the PWM device. So it should already have been resumed when
      the above AML runs and the AML should thus not do its undesirable poking
      of the PWM controller register.
      
      But the PCI core powers on PCI devices in the no-irq resume phase and
      thus calls the troublesome PS0 method in the no-irq resume phase.
      Where as LPSS devices by default are resumed in the early resume phase.
      
      This commit sets the resume_from_noirq flag in the bsw_pwm_dev_desc
      struct, so that Cherry Trail PWM controllers will be resumed in the
      no-irq phase. Together with the device-link added by the pwm-get this
      ensures that the PWM controller will be on when the troublesome PS0
      method runs, which stops it from poking the PWM controller.
      Acked-by: default avatarRafael J. Wysocki <rafael.j.wysocki@intel.com>
      Signed-off-by: default avatarHans de Goede <hdegoede@redhat.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20200903112337.4113-2-hdegoede@redhat.com
      5e31ee84
  2. 04 Sep, 2020 4 commits
  3. 03 Sep, 2020 1 commit
  4. 01 Sep, 2020 17 commits
  5. 31 Aug, 2020 1 commit
  6. 29 Aug, 2020 1 commit
  7. 28 Aug, 2020 4 commits
  8. 27 Aug, 2020 4 commits
  9. 26 Aug, 2020 1 commit