Skip to content

L
linux
Switch branch/tag
  1. 06 Sep, 2020 4 commits
    • 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
  3. 04 Sep, 2020 4 commits
  5. 03 Sep, 2020 1 commit
  7. 01 Sep, 2020 17 commits
  9. 31 Aug, 2020 1 commit
  11. 29 Aug, 2020 1 commit
  13. 28 Aug, 2020 4 commits
  15. 27 Aug, 2020 4 commits
  17. 26 Aug, 2020 3 commits
  19. 24 Aug, 2020 1 commit
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7