1. 06 Sep, 2020 11 commits
    • Hans de Goede's avatar
      pwm: crc: Enable/disable PWM output on enable/disable · 6fdefe60
      Hans de Goede authored
      The pwm-crc code is using 2 different enable bits:
      1. bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE)
      2. bit 0 of the BACKLIGHT_EN register
      
      So far we've kept the PWM_OUTPUT_ENABLE bit set when disabling the PWM,
      this commit makes crc_pwm_disable() clear it on disable and makes
      crc_pwm_enable() set it again on re-enable.
      Acked-by: default avatarUwe Kleine-König <u.kleine-koenig@pengutronix.de>
      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-12-hdegoede@redhat.com
      6fdefe60
    • Hans de Goede's avatar
      pwm: crc: Fix period changes not having any effect · 6158231a
      Hans de Goede authored
      The pwm-crc code is using 2 different enable bits:
      1. bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE)
      2. bit 0 of the BACKLIGHT_EN register
      
      The BACKLIGHT_EN register at address 0x51 really controls a separate
      output-only GPIO which is earmarked to be used as output connected to the
      backlight-enable pin for LCD panels, this GPO is part of the PMIC's
      "Display Panel Control Block." . This pin should probably be moved over
      to a GPIO provider driver (and consumers modified accordingly), but that
      is something for an(other) patch.
      
      Enabling / disabling the actual PWM output is controlled by the
      PWM_OUTPUT_ENABLE bit of the PWM0_CLK_DIV register.
      
      As the comment in the old code already indicates we must disable the PWM
      before we can change the clock divider. But the crc_pwm_disable() and
      crc_pwm_enable() calls the old code make for this only change the
      BACKLIGHT_EN register; and the value of that register does not matter for
      changing the period / the divider. What does matter is that the
      PWM_OUTPUT_ENABLE bit must be cleared before a new value can be written.
      
      This commit modifies crc_pwm_config() to clear PWM_OUTPUT_ENABLE instead
      when changing the period, so that period changes actually work.
      
      Note this fix will cause a significant behavior change on some devices
      using the CRC PWM output to drive their backlight. Before the PWM would
      always run with the output frequency configured by the BIOS at boot, now
      the period time specified by the i915 driver will actually be honored.
      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-11-hdegoede@redhat.com
      6158231a
    • Hans de Goede's avatar
      pwm: crc: Fix off-by-one error in the clock-divider calculations · a05af71f
      Hans de Goede authored
      The CRC PWM controller has a clock-divider which divides the clock with
      a value between 1-128. But as can seen from the PWM_DIV_CLK_xxx
      defines, this range maps to a register value of 0-127.
      
      So after calculating the clock-divider we must subtract 1 to get the
      register value, unless the requested frequency was so high that the
      calculation has already resulted in a (rounded) divider value of 0.
      
      Note that before this fix, setting a period of PWM_MAX_PERIOD_NS which
      corresponds to the max. divider value of 128 could have resulted in a
      bug where the code would use 128 as divider-register value which would
      have resulted in an actual divider value of 0 (and the enable bit being
      set). A rounding error stopped this bug from actually happen. This
      same rounding error means that after the subtraction of 1 it is impossible
      to set the divider to 128. Also bump PWM_MAX_PERIOD_NS by 1 ns to allow
      setting a divider of 128 (register-value 127).
      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-10-hdegoede@redhat.com
      a05af71f
    • Hans de Goede's avatar
      pwm: crc: Fix period / duty_cycle times being off by a factor of 256 · 79e08992
      Hans de Goede authored
      While looking into adding atomic-pwm support to the pwm-crc driver I
      noticed something odd, there is a PWM_BASE_CLK define of 6 MHz and
      there is a clock-divider which divides this with a value between 1-128,
      and there are 256 duty-cycle steps.
      
      The pwm-crc code before this commit assumed that a clock-divider
      setting of 1 means that the PWM output is running at 6 MHZ, if that
      is true, where do these 256 duty-cycle steps come from?
      
      This would require an internal frequency of 256 * 6 MHz = 1.5 GHz, that
      seems unlikely for a PMIC which is using a silicon process optimized for
      power-switching transistors. It is way more likely that there is an 8
      bit counter for the duty cycle which acts as an extra fixed divider
      wrt the PWM output frequency.
      
      The main user of the pwm-crc driver is the i915 GPU driver which uses it
      for backlight control. Lets compare the PWM register values set by the
      video-BIOS (the GOP), assuming the extra fixed divider is present versus
      the PWM frequency specified in the Video-BIOS-Tables:
      
      Device:		PWM Hz set by BIOS	PWM Hz specified in VBT
      Asus T100TA 	200			200
      Asus T100HA 	200			200
      Lenovo Miix 2 8	23437			20000
      Toshiba WT8-A	23437			20000
      
      So as we can see if we assume the extra division by 256 then the register
      values set by the GOP are an exact match for the VBT values, where as
      otherwise the values would be of by a factor of 256.
      
      This commit fixes the period / duty_cycle calculations to take the
      extra division by 256 into account.
      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-9-hdegoede@redhat.com
      79e08992
    • 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 1 commit