[PATCH v8 00/17] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API
Hi All,
Unfortunately while testing some unrelated things I found another issue with this series related to the CHT ACPI GFX0._PS3 code poking the PWM controller in unexpected ways.
This new version contains a new patch: "[PATCH v8 07/17] pwm: lpss: Always update state and set update bit" fixing this. It also contains the same change in: "PATCH v8 06/17] pwm: lpss: Use pwm_lpss_restore() when restoring state on resume". Rather then adding the below check and then dropping it in the new patch, I've squashed the dropping of these lines:
/* If we did not reach S0i3/S3 the controller keeps its state */ if (ctrl == lpwm->saved_ctrl[i]) continue;
Directly into the original commit leading to a cleaner history.
As discussed before, because of interdependencies of the patches I plan to push the entire series to drm-intel-next-queued once the series has passed CI.
I still plan to do this as soon as I get a Reviewed-by or Acked-by for the new pwm-lpss.c patch.
Thierry, I believe from our previous discussion that you are ok with pushing the pwm-crc and pwm-lpss patches through the drm-intel tree, but you have not given your Acked-by for this. If you are not ok with me pushing these out this way please let me now ASAP. If you are ok with this an Acked-by would be appreciated.
This series has been tested (and re-tested after adding various bug-fixes) extensively. It has been tested on the following devices:
-Asus T100TA BYT + CRC-PMIC PWM -Toshiba WT8-A BYT + CRC-PMIC PWM -Thundersoft TS178 BYT + CRC-PMIC PWM, inverse PWM -Asus T100HA CHT + CRC-PMIC PWM -Terra Pad 1061 BYT + LPSS PWM -Trekstor Twin 10.1 BYT + LPSS PWM -Asus T101HA CHT + LPSS PWM -GPD Pocket CHT + LPSS PWM -Acer One S1003 CHT + LPSS PWM
Regards,
Hans
Changelog:
Changes in v8: - Add a new patch dealing with the ACPI/DSDT GFX0._PS3 code poking the PWM controller in unexpected ways on some Cherry Trail devices
Changes in v7: - Fix a u64 divide leading to undefined reference to `__udivdi3' errors on 32 bit platforms by casting the divisor to an unsigned long
Changes in v6: - Rebase on v5.9-rc1 - Adjust pwm-crc patches for pwm_state.period and .duty_cycle now being u64
Changes in v5: - Dropped the "pwm: lpss: Correct get_state result for base_unit == 0" patch. The base_unit == 0 condition should never happen and sofar it is unclear what the proper behavior / correct values to store in the pwm_state should be when this does happen. Since this patch was added as an extra pwm-lpss fix in v4 of this patch-set and otherwise is orthogonal to the of this patch-set just drop it (again). - "[PATCH 04/16] pwm: lpss: Add range limit check for the base_unit register value" - Use clamp_val(... instead of clam_t(unsigned long long, ... - "[PATCH 05/16] pwm: lpss: Add pwm_lpss_prepare_enable() helper" - This is a new patch in v5 of this patchset - [PATCH 06/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume - Use the new pwm_lpss_prepare_enable() helper
Changes in v4: - "[PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0" - This is a new patch in v4 of this patchset - "[PATCH v4 12/16] pwm: crc: Implement get_state() method" - Use DIV_ROUND_UP when calculating the period and duty_cycle values - "[PATCH v4 16/16] drm/i915: panel: Use atomic PWM API for devs with an external PWM controller" - Add a note to the commit message about the changes in pwm_disable_backlight() - Use the pwm_set/get_relative_duty_cycle() helpers
Changes in v3: - "[PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value" - Use base_unit_range - 1 as maximum value for the clamp() - "[PATCH v3 05/15] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume" - This replaces the "pwm: lpss: Set SW_UPDATE bit when enabling the PWM" patch from previous versions of this patch-set, which really was a hack working around the resume issue which this patch fixes properly. - PATCH v3 6 - 11 pwm-crc changes: - Various small changes resulting from the reviews by Andy and Uwe, including some refactoring of the patches to reduce the amount of churn in the patch-set
Changes in v2: - Fix coverletter subject - Drop accidentally included debugging patch - "[PATCH v3 02/15] ACPI / LPSS: Save Cherry Trail PWM ctx registers only once ( - Move #define LPSS_SAVE_CTX_ONCE define to group it with LPSS_SAVE_CTX
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: Rafael J. Wysocki rafael.j.wysocki@intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/acpi/acpi_lpss.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index 5e2bfbcf526f..67892fc0b822 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -257,6 +257,7 @@ static const struct lpss_device_desc bsw_pwm_dev_desc = { .flags = LPSS_SAVE_CTX | LPSS_NO_D3_DELAY, .prv_offset = 0x800, .setup = bsw_pwm_setup, + .resume_from_noirq = true, };
static const struct lpss_device_desc byt_uart_dev_desc = {
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: Rafael J. Wysocki rafael.j.wysocki@intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- Changes in v2: - Move #define LPSS_SAVE_CTX_ONCE define to group it with LPSS_SAVE_CTX --- drivers/acpi/acpi_lpss.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index 67892fc0b822..a8d7d83ac761 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -67,7 +67,15 @@ ACPI_MODULE_NAME("acpi_lpss"); #define LPSS_CLK_DIVIDER BIT(2) #define LPSS_LTR BIT(3) #define LPSS_SAVE_CTX BIT(4) -#define LPSS_NO_D3_DELAY BIT(5) +/* + * For some devices the DSDT AML code for another device turns off the device + * before our suspend handler runs, causing us to read/save all 1-s (0xffffffff) + * as ctx register values. + * Luckily these devices always use the same ctx register values, so we can + * work around this by saving the ctx registers once on activation. + */ +#define LPSS_SAVE_CTX_ONCE BIT(5) +#define LPSS_NO_D3_DELAY BIT(6)
struct lpss_private_data;
@@ -254,7 +262,7 @@ static const struct lpss_device_desc byt_pwm_dev_desc = { };
static const struct lpss_device_desc bsw_pwm_dev_desc = { - .flags = LPSS_SAVE_CTX | LPSS_NO_D3_DELAY, + .flags = LPSS_SAVE_CTX_ONCE | LPSS_NO_D3_DELAY, .prv_offset = 0x800, .setup = bsw_pwm_setup, .resume_from_noirq = true, @@ -885,9 +893,14 @@ static int acpi_lpss_activate(struct device *dev) * we have to deassert reset line to be sure that ->probe() will * recognize the device. */ - if (pdata->dev_desc->flags & LPSS_SAVE_CTX) + if (pdata->dev_desc->flags & (LPSS_SAVE_CTX | LPSS_SAVE_CTX_ONCE)) lpss_deassert_reset(pdata);
+#ifdef CONFIG_PM + if (pdata->dev_desc->flags & LPSS_SAVE_CTX_ONCE) + acpi_lpss_save_ctx(dev, pdata); +#endif + return 0; }
@@ -1031,7 +1044,7 @@ static int acpi_lpss_resume(struct device *dev)
acpi_lpss_d3_to_d0_delay(pdata);
- if (pdata->dev_desc->flags & LPSS_SAVE_CTX) + if (pdata->dev_desc->flags & (LPSS_SAVE_CTX | LPSS_SAVE_CTX_ONCE)) acpi_lpss_restore_ctx(dev, pdata);
return 0;
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 684309e5043e ("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: 684309e5043e ("pwm: lpss: Avoid potential overflow of base_unit") Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Acked-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de Signed-off-by: Hans de Goede hdegoede@redhat.com --- Changes in v3: - Add Fixes tag - Add Reviewed-by: Andy Shevchenko tag --- drivers/pwm/pwm-lpss.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index 9d965ffe66d1..43b1fc634af1 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -93,7 +93,7 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm, * The equation is: * base_unit = round(base_unit_range * freq / c) */ - base_unit_range = BIT(lpwm->info->base_unit_bits) - 1; + base_unit_range = BIT(lpwm->info->base_unit_bits); freq *= base_unit_range;
base_unit = DIV_ROUND_CLOSEST_ULL(freq, c); @@ -104,8 +104,8 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm,
orig_ctrl = ctrl = pwm_lpss_read(pwm); ctrl &= ~PWM_ON_TIME_DIV_MASK; - ctrl &= ~(base_unit_range << PWM_BASE_UNIT_SHIFT); - base_unit &= base_unit_range; + ctrl &= ~((base_unit_range - 1) << PWM_BASE_UNIT_SHIFT); + base_unit &= (base_unit_range - 1); ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT; ctrl |= on_time_div;
On Sun, Aug 30, 2020 at 02:57:39PM +0200, Hans de Goede wrote:
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 684309e5043e ("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: 684309e5043e ("pwm: lpss: Avoid potential overflow of base_unit") Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Acked-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de Signed-off-by: Hans de Goede hdegoede@redhat.com
Changes in v3:
- Add Fixes tag
- Add Reviewed-by: Andy Shevchenko tag
drivers/pwm/pwm-lpss.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Acked-by: Thierry Reding thierry.reding@gmail.com
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: 684309e5043e ("pwm: lpss: Avoid potential overflow of base_unit") Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- Changes in v5: - Use clamp_val(... instead of clam_t(unsigned long long, ...
Changes in v3: - Change upper limit of clamp to (base_unit_range - 1) - Add Fixes tag --- drivers/pwm/pwm-lpss.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index 43b1fc634af1..da9bc3d10104 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -97,6 +97,8 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm, freq *= base_unit_range;
base_unit = DIV_ROUND_CLOSEST_ULL(freq, c); + /* base_unit must not be 0 and we also want to avoid overflowing it */ + base_unit = clamp_val(base_unit, 1, base_unit_range - 1);
on_time_div = 255ULL * duty_ns; do_div(on_time_div, period_ns); @@ -105,7 +107,6 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm, orig_ctrl = ctrl = pwm_lpss_read(pwm); ctrl &= ~PWM_ON_TIME_DIV_MASK; ctrl &= ~((base_unit_range - 1) << PWM_BASE_UNIT_SHIFT); - base_unit &= (base_unit_range - 1); ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT; ctrl |= on_time_div;
On Sun, Aug 30, 2020 at 02:57:40PM +0200, Hans de Goede wrote:
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: 684309e5043e ("pwm: lpss: Avoid potential overflow of base_unit") Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com
Changes in v5:
- Use clamp_val(... instead of clam_t(unsigned long long, ...
Changes in v3:
- Change upper limit of clamp to (base_unit_range - 1)
- Add Fixes tag
drivers/pwm/pwm-lpss.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Acked-by: Thierry Reding thierry.reding@gmail.com
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: Andy Shevchenko andriy.shevchenko@linux.intel.com Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/pwm/pwm-lpss.c | 45 ++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index da9bc3d10104..8a136ba2a583 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -122,41 +122,48 @@ static inline void pwm_lpss_cond_enable(struct pwm_device *pwm, bool cond) pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE); }
+static int pwm_lpss_prepare_enable(struct pwm_lpss_chip *lpwm, + struct pwm_device *pwm, + const struct pwm_state *state, + bool enable) +{ + int ret; + + ret = pwm_lpss_is_updating(pwm); + if (ret) + return ret; + + pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period); + pwm_lpss_cond_enable(pwm, enable && lpwm->info->bypass == false); + ret = pwm_lpss_wait_for_update(pwm); + if (ret) + return ret; + + pwm_lpss_cond_enable(pwm, enable && lpwm->info->bypass == true); + return 0; +} + static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) { struct pwm_lpss_chip *lpwm = to_lpwm(chip); - int ret; + int ret = 0;
if (state->enabled) { if (!pwm_is_enabled(pwm)) { pm_runtime_get_sync(chip->dev); - ret = pwm_lpss_is_updating(pwm); - if (ret) { - 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); - ret = pwm_lpss_wait_for_update(pwm); - if (ret) { + ret = pwm_lpss_prepare_enable(lpwm, pwm, state, true); + if (ret) pm_runtime_put(chip->dev); - return ret; - } - pwm_lpss_cond_enable(pwm, lpwm->info->bypass == true); } else { - ret = pwm_lpss_is_updating(pwm); - if (ret) - return ret; - pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period); - return pwm_lpss_wait_for_update(pwm); + ret = pwm_lpss_prepare_enable(lpwm, pwm, state, false); } } else if (pwm_is_enabled(pwm)) { pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); pm_runtime_put(chip->dev); }
- return 0; + return ret; }
static void pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
On Sun, Aug 30, 2020 at 02:57:41PM +0200, Hans de Goede wrote:
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: Andy Shevchenko andriy.shevchenko@linux.intel.com Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/pwm/pwm-lpss.c | 45 ++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 19 deletions(-)
Acked-by: Thierry Reding thierry.reding@gmail.com
Before this commit a suspend + resume of the LPSS PWM controller would result in the controller being reset to its defaults of output-freq = clock/256, duty-cycle=100%, until someone changes to the output-freq and/or duty-cycle are made.
This problem has been masked so far because the main consumer (the i915 driver) was always making duty-cycle changes on resume. With the conversion of the i915 driver to the atomic PWM API the driver now only disables/enables the PWM on suspend/resume leaving the output-freq and duty as is, triggering this problem.
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.
The problem is that before this commit our suspend/resume handling consisted of simply saving the PWM ctrl register on suspend and restoring it on resume, without setting the PWM_SW_UPDATE bit. When the controller has lost its state over a suspend/resume and thus has been reset to the defaults, just restoring the register is not enough. We must also set the SW_UPDATE bit to tell the controller to latch the restored values into the actual registers.
Fixing this problem is not as simple as just or-ing in the value which is being restored with SW_UPDATE. If the PWM was enabled before we must write the new settings + PWM_SW_UPDATE before setting PWM_ENABLE. We must also wait for PWM_SW_UPDATE to become 0 again and depending on the model we must do this either before or after the setting of PWM_ENABLE.
All the necessary logic for doing this is already present inside pwm_lpss_apply(), so instead of duplicating this inside the resume handler, this commit adds a new pwm_lpss_restore() helper which mirrors pwm_lpss_apply() minus the runtime-pm reference handling (which we should not change on resume).
This fixes the output-freq and duty-cycle being reset to their defaults on resume.
Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- Changes in v8: - Drop optimization to skip restore if current ctrl reg is the same as our saved ctrl reg value (because this causes issues on some devices) - Simplify pwm_lpss_restore_state() to not rely on the current state - Modify commit message to mention the new pwm_lpss_restore_state() helper
Changes in v6: - Add a pwm_lpss_restore_state() helper for re-applying the PWM state on resume
Changes in v5: - The changes to pwm_lpss_apply() are much cleaner now thanks to the new pwm_lpss_prepare_enable() helper.
Changes in v3: - This replaces the "pwm: lpss: Set SW_UPDATE bit when enabling the PWM" patch from previous versions of this patch-set, which really was a hack working around the resume issue which this patch fixes properly. --- drivers/pwm/pwm-lpss.c | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index 8a136ba2a583..9a7400c6fb6e 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -166,6 +166,25 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, return ret; }
+/* + * This is a mirror of pwm_lpss_apply() without relying on the current state + * (no pwm_is_enabled() calls) and without pm_runtime reference handling, + * for restoring the PWM state on resume. + */ +static int pwm_lpss_restore_state(struct pwm_lpss_chip *lpwm, + struct pwm_device *pwm, + const struct pwm_state *state) +{ + int ret = 0; + + if (state->enabled) + ret = pwm_lpss_prepare_enable(lpwm, pwm, state, true); + else + pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); + + return ret; +} + static void pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_state *state) { @@ -278,10 +297,25 @@ EXPORT_SYMBOL_GPL(pwm_lpss_suspend); int pwm_lpss_resume(struct device *dev) { struct pwm_lpss_chip *lpwm = dev_get_drvdata(dev); - int i; + struct pwm_device *pwm; + int i, ret;
- for (i = 0; i < lpwm->info->npwm; i++) - writel(lpwm->saved_ctrl[i], lpwm->regs + i * PWM_SIZE + PWM); + for (i = 0; i < lpwm->info->npwm; i++) { + pwm = &lpwm->chip.pwms[i]; + + /* + * We cannot just blindly restore the old value here. Since we + * are changing the settings we must set SW_UPDATE and if the + * PWM was enabled before we must write the new settings + + * PWM_SW_UPDATE before setting PWM_ENABLE. We must also wait + * for PWM_SW_UPDATE to become 0 again and depending on the + * model we must do this either before or after the setting of + * PWM_ENABLE. + */ + ret = pwm_lpss_restore_state(lpwm, pwm, &pwm->state); + if (ret) + dev_err(dev, "Error restoring state on resume\n"); + }
return 0; }
On Sun, Aug 30, 2020 at 02:57:42PM +0200, Hans de Goede wrote:
Before this commit a suspend + resume of the LPSS PWM controller would result in the controller being reset to its defaults of output-freq = clock/256, duty-cycle=100%, until someone changes to the output-freq and/or duty-cycle are made.
This problem has been masked so far because the main consumer (the i915 driver) was always making duty-cycle changes on resume. With the conversion of the i915 driver to the atomic PWM API the driver now only disables/enables the PWM on suspend/resume leaving the output-freq and duty as is, triggering this problem.
Doesn't this imply that there's another bug at play here? At the PWM API level you're applying a state and it's up to the driver to ensure that the hardware state after ->apply() is what the software has requested.
If you only switch the enable state and that doesn't cause period and duty cycle to be updated it means that your driver isn't writing those registers when it should be.
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.
The problem is that before this commit our suspend/resume handling consisted of simply saving the PWM ctrl register on suspend and restoring it on resume, without setting the PWM_SW_UPDATE bit. When the controller has lost its state over a suspend/resume and thus has been reset to the defaults, just restoring the register is not enough. We must also set the SW_UPDATE bit to tell the controller to latch the restored values into the actual registers.
Fixing this problem is not as simple as just or-ing in the value which is being restored with SW_UPDATE. If the PWM was enabled before we must write the new settings + PWM_SW_UPDATE before setting PWM_ENABLE. We must also wait for PWM_SW_UPDATE to become 0 again and depending on the model we must do this either before or after the setting of PWM_ENABLE.
All the necessary logic for doing this is already present inside pwm_lpss_apply(), so instead of duplicating this inside the resume handler, this commit adds a new pwm_lpss_restore() helper which mirrors pwm_lpss_apply() minus the runtime-pm reference handling (which we should not change on resume).
If this is all already implemented in pwm_lpss_apply(), why isn't it working for the suspend/resume case? It shouldn't matter that the consumer only changes the enable/disable state. After ->apply() successfully returns your hardware should be programmed with exactly the state that the consumer requested.
Thierry
Hi,
On 8/31/20 1:10 PM, Thierry Reding wrote:
On Sun, Aug 30, 2020 at 02:57:42PM +0200, Hans de Goede wrote:
Before this commit a suspend + resume of the LPSS PWM controller would result in the controller being reset to its defaults of output-freq = clock/256, duty-cycle=100%, until someone changes to the output-freq and/or duty-cycle are made.
This problem has been masked so far because the main consumer (the i915 driver) was always making duty-cycle changes on resume. With the conversion of the i915 driver to the atomic PWM API the driver now only disables/enables the PWM on suspend/resume leaving the output-freq and duty as is, triggering this problem.
Doesn't this imply that there's another bug at play here? At the PWM API level you're applying a state and it's up to the driver to ensure that the hardware state after ->apply() is what the software has requested.
If you only switch the enable state and that doesn't cause period and duty cycle to be updated it means that your driver isn't writing those registers when it should be.
Right, the driver was not committing those as it should *on resume*, that and it skips setting the update bit on the subsequent enable, which is an optimization which gets removed in 7/17.
Before switching the i915 driver over to atomic, when the LPSS-PWM was used for the backlight we got the following order on suspend/resume
1. Set duty-cycle to 0% 2. Set enabled to 0 3. Save ctrl reg 4. Power-off PWM controller, it now looses all its state 5. Power-on PWM ctrl 6. Restore ctrl reg (as a single reg write) 7. Set enabled to 1, at this point one would expect the duty/freq from the restored ctrl-reg to apply, but: a) The resume code never sets the update bit (which this commit fixes); and b) On applying the pwm_state with enabled=1 the code applying the state does this (before setting the enabled bit in the ctrl reg):
if (orig_ctrl != ctrl) { pwm_lpss_write(pwm, ctrl); pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE); } and since the restore of the ctrl reg set the old duty/freq the writes are skipped, so the update bit never gets set.
8. Set duty-cycle to the pre-suspend value (which is not 0) this does cause a change in the ctrl-reg, so now the update flag does get set.
Note that 1-2 and 7-8 are both done by the non atomic i915 code, when moving the i915 code to atomic I decided that having these 2 separate steps here is non-sense, so the new i915 code just toggles the enable bit. So in essence the new atomic PWM i915 code drops step 1 and 8.
Dropping steps 8 means that the update bit never gets set and we end up with the PWM running at its power-on-reset duty cycle.
You are correct in your remark to patch 7/17 that since that removes the if (orig_ctrl != ctrl) for the writes that now step 7 will be sufficient to get the PWM to work again. But that only takes the i915 usage into account.
What if the PWM is used through the sysfs userspace API? Then only steps 3-6 will happen on suspend-resume and without fixing step 6 to properly restore the PWM controller in its pre-resume state (this patch) it will once again be running at its power-on-reset defaults instead of the values from the restored control register.
So at step 6, if the PWM was enabled before, we must set the update bit, and then wait for it to clear again so the controller is ready for subsequent updates. The waiting for it to clear again needs to happen before or after setting the enable bit depending on the hw generation, which leads to this patch.
I hope that helps explain why this patch is the correct thing to do.
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.
The problem is that before this commit our suspend/resume handling consisted of simply saving the PWM ctrl register on suspend and restoring it on resume, without setting the PWM_SW_UPDATE bit. When the controller has lost its state over a suspend/resume and thus has been reset to the defaults, just restoring the register is not enough. We must also set the SW_UPDATE bit to tell the controller to latch the restored values into the actual registers.
Fixing this problem is not as simple as just or-ing in the value which is being restored with SW_UPDATE. If the PWM was enabled before we must write the new settings + PWM_SW_UPDATE before setting PWM_ENABLE. We must also wait for PWM_SW_UPDATE to become 0 again and depending on the model we must do this either before or after the setting of PWM_ENABLE.
All the necessary logic for doing this is already present inside pwm_lpss_apply(), so instead of duplicating this inside the resume handler, this commit adds a new pwm_lpss_restore() helper which mirrors pwm_lpss_apply() minus the runtime-pm reference handling (which we should not change on resume).
If this is all already implemented in pwm_lpss_apply(), why isn't it working for the suspend/resume case? It shouldn't matter that the consumer only changes the enable/disable state. After ->apply() successfully returns your hardware should be programmed with exactly the state that the consumer requested.
See above, apply() was trying to be smart but the restore of ctrl on resume without setting the update bit was tricking it. That being too smart for its own good is removed in 7/16 as you rightfully point out. But this patch is still necessary for the PWM controller to be in the expected state between resume and the first apply() after resume (which may be quite a long time in the future when using e.g. the sysfs API).
Regards,
Hans
On Mon, Aug 31, 2020 at 01:46:28PM +0200, Hans de Goede wrote:
Hi,
On 8/31/20 1:10 PM, Thierry Reding wrote:
On Sun, Aug 30, 2020 at 02:57:42PM +0200, Hans de Goede wrote:
Before this commit a suspend + resume of the LPSS PWM controller would result in the controller being reset to its defaults of output-freq = clock/256, duty-cycle=100%, until someone changes to the output-freq and/or duty-cycle are made.
This problem has been masked so far because the main consumer (the i915 driver) was always making duty-cycle changes on resume. With the conversion of the i915 driver to the atomic PWM API the driver now only disables/enables the PWM on suspend/resume leaving the output-freq and duty as is, triggering this problem.
Doesn't this imply that there's another bug at play here? At the PWM API level you're applying a state and it's up to the driver to ensure that the hardware state after ->apply() is what the software has requested.
If you only switch the enable state and that doesn't cause period and duty cycle to be updated it means that your driver isn't writing those registers when it should be.
Right, the driver was not committing those as it should *on resume*, that and it skips setting the update bit on the subsequent enable, which is an optimization which gets removed in 7/17.
Before switching the i915 driver over to atomic, when the LPSS-PWM was used for the backlight we got the following order on suspend/resume
- Set duty-cycle to 0%
- Set enabled to 0
- Save ctrl reg
- Power-off PWM controller, it now looses all its state
- Power-on PWM ctrl
- Restore ctrl reg (as a single reg write)
- Set enabled to 1, at this point one would expect the
duty/freq from the restored ctrl-reg to apply, but: a) The resume code never sets the update bit (which this commit fixes); and b) On applying the pwm_state with enabled=1 the code applying the state does this (before setting the enabled bit in the ctrl reg):
if (orig_ctrl != ctrl) { pwm_lpss_write(pwm, ctrl); pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE); } and since the restore of the ctrl reg set the old duty/freq the writes are skipped, so the update bit never gets set.
- Set duty-cycle to the pre-suspend value (which is not 0)
this does cause a change in the ctrl-reg, so now the update flag does get set.
Note that 1-2 and 7-8 are both done by the non atomic i915 code, when moving the i915 code to atomic I decided that having these 2 separate steps here is non-sense, so the new i915 code just toggles the enable bit. So in essence the new atomic PWM i915 code drops step 1 and 8.
Dropping steps 8 means that the update bit never gets set and we end up with the PWM running at its power-on-reset duty cycle.
You are correct in your remark to patch 7/17 that since that removes the if (orig_ctrl != ctrl) for the writes that now step 7 will be sufficient to get the PWM to work again. But that only takes the i915 usage into account.
What if the PWM is used through the sysfs userspace API? Then only steps 3-6 will happen on suspend-resume and without fixing step 6 to properly restore the PWM controller in its pre-resume state (this patch) it will once again be running at its power-on-reset defaults instead of the values from the restored control register.
Actually PWM's sysfs code has suspend/resume callbacks that basically make sysfs just a regular consumer of PWMs. So they do end up doing a pwm_apply_state() on the PWM as well on suspend and restore the state from before suspend on resume.
This was done very specifically because the suspend/resume order can be unexpected under some circumstances, so for PWM we really want for the consumer to always have ultimate control over when precisely the PWM is restored on resume.
The reason why we did this was because people observed weird glitches on suspend/resume with different severity. In some cases a backlight would be resumed before the display controller had had a chance to start sending frames, causing on-screen corruption in some cases (such as smart displays) and in other cases a PWM-controller regulator would be resumed too late or too early, which I think was causing some issue with the CPUs not working properly on resume.
So I'd prefer not to have any PWM driver save and restore its own context on suspend/resume, because that's inevitably going to cause unexpected behaviour at some point. If it's absolutely necessary we can of course still do that, but I think in that case we need to at least add a comment in the code about why context save/restore is needed in this particular case and make it clear that this is not something that other drivers should copy because they most likely won't be needing it.
Given the above it also doesn't sound to me like there's a real problem, or at least that the bug is somewhere else. A consumer should always be responsible for applying the pre-suspend state upon resume and it sounds like that would be true after patch 7. Since sysfs is just a regular consumer, the same should apply for sysfs-controlled PWMs as well.
So at step 6, if the PWM was enabled before, we must set the update bit, and then wait for it to clear again so the controller is ready for subsequent updates. The waiting for it to clear again needs to happen before or after setting the enable bit depending on the hw generation, which leads to this patch.
But all of that should be happening as part of the call to pwm_apply_state(), right? That path should be taken for all consumers on resume, including sysfs.
I hope that helps explain why this patch is the correct thing to do.
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.
The problem is that before this commit our suspend/resume handling consisted of simply saving the PWM ctrl register on suspend and restoring it on resume, without setting the PWM_SW_UPDATE bit. When the controller has lost its state over a suspend/resume and thus has been reset to the defaults, just restoring the register is not enough. We must also set the SW_UPDATE bit to tell the controller to latch the restored values into the actual registers.
Fixing this problem is not as simple as just or-ing in the value which is being restored with SW_UPDATE. If the PWM was enabled before we must write the new settings + PWM_SW_UPDATE before setting PWM_ENABLE. We must also wait for PWM_SW_UPDATE to become 0 again and depending on the model we must do this either before or after the setting of PWM_ENABLE.
All the necessary logic for doing this is already present inside pwm_lpss_apply(), so instead of duplicating this inside the resume handler, this commit adds a new pwm_lpss_restore() helper which mirrors pwm_lpss_apply() minus the runtime-pm reference handling (which we should not change on resume).
If this is all already implemented in pwm_lpss_apply(), why isn't it working for the suspend/resume case? It shouldn't matter that the consumer only changes the enable/disable state. After ->apply() successfully returns your hardware should be programmed with exactly the state that the consumer requested.
See above, apply() was trying to be smart but the restore of ctrl on resume without setting the update bit was tricking it. That being too smart for its own good is removed in 7/16 as you rightfully point out. But this patch is still necessary for the PWM controller to be in the expected state between resume and the first apply() after resume (which may be quite a long time in the future when using e.g. the sysfs API).
Like I said, the sysfs code should be resuming any exported PWMs on resume just like any other consumer.
Obviously it's always up to the consumer to call pwm_apply_state() at the right time. If that's "too late" for some reason, then that's a bug in the consumer driver. But as I explained above there are a number of cases where restoring context in the PWM driver itself doesn't work because it can cause sequencing issues.
Thierry
Hi,
On 8/31/20 3:15 PM, Thierry Reding wrote:
On Mon, Aug 31, 2020 at 01:46:28PM +0200, Hans de Goede wrote:
Hi,
On 8/31/20 1:10 PM, Thierry Reding wrote:
On Sun, Aug 30, 2020 at 02:57:42PM +0200, Hans de Goede wrote:
Before this commit a suspend + resume of the LPSS PWM controller would result in the controller being reset to its defaults of output-freq = clock/256, duty-cycle=100%, until someone changes to the output-freq and/or duty-cycle are made.
This problem has been masked so far because the main consumer (the i915 driver) was always making duty-cycle changes on resume. With the conversion of the i915 driver to the atomic PWM API the driver now only disables/enables the PWM on suspend/resume leaving the output-freq and duty as is, triggering this problem.
Doesn't this imply that there's another bug at play here? At the PWM API level you're applying a state and it's up to the driver to ensure that the hardware state after ->apply() is what the software has requested.
If you only switch the enable state and that doesn't cause period and duty cycle to be updated it means that your driver isn't writing those registers when it should be.
Right, the driver was not committing those as it should *on resume*, that and it skips setting the update bit on the subsequent enable, which is an optimization which gets removed in 7/17.
Before switching the i915 driver over to atomic, when the LPSS-PWM was used for the backlight we got the following order on suspend/resume
- Set duty-cycle to 0%
- Set enabled to 0
- Save ctrl reg
- Power-off PWM controller, it now looses all its state
- Power-on PWM ctrl
- Restore ctrl reg (as a single reg write)
- Set enabled to 1, at this point one would expect the
duty/freq from the restored ctrl-reg to apply, but: a) The resume code never sets the update bit (which this commit fixes); and b) On applying the pwm_state with enabled=1 the code applying the state does this (before setting the enabled bit in the ctrl reg):
if (orig_ctrl != ctrl) { pwm_lpss_write(pwm, ctrl); pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE); } and since the restore of the ctrl reg set the old duty/freq the writes are skipped, so the update bit never gets set.
- Set duty-cycle to the pre-suspend value (which is not 0)
this does cause a change in the ctrl-reg, so now the update flag does get set.
Note that 1-2 and 7-8 are both done by the non atomic i915 code, when moving the i915 code to atomic I decided that having these 2 separate steps here is non-sense, so the new i915 code just toggles the enable bit. So in essence the new atomic PWM i915 code drops step 1 and 8.
Dropping steps 8 means that the update bit never gets set and we end up with the PWM running at its power-on-reset duty cycle.
You are correct in your remark to patch 7/17 that since that removes the if (orig_ctrl != ctrl) for the writes that now step 7 will be sufficient to get the PWM to work again. But that only takes the i915 usage into account.
What if the PWM is used through the sysfs userspace API? Then only steps 3-6 will happen on suspend-resume and without fixing step 6 to properly restore the PWM controller in its pre-resume state (this patch) it will once again be running at its power-on-reset defaults instead of the values from the restored control register.
Actually PWM's sysfs code has suspend/resume callbacks that basically make sysfs just a regular consumer of PWMs. So they do end up doing a pwm_apply_state() on the PWM as well on suspend and restore the state from before suspend on resume.
This was done very specifically because the suspend/resume order can be unexpected under some circumstances, so for PWM we really want for the consumer to always have ultimate control over when precisely the PWM is restored on resume.
The reason why we did this was because people observed weird glitches on suspend/resume with different severity. In some cases a backlight would be resumed before the display controller had had a chance to start sending frames, causing on-screen corruption in some cases (such as smart displays) and in other cases a PWM-controller regulator would be resumed too late or too early, which I think was causing some issue with the CPUs not working properly on resume.
So I'd prefer not to have any PWM driver save and restore its own context on suspend/resume, because that's inevitably going to cause unexpected behaviour at some point. If it's absolutely necessary we can of course still do that, but I think in that case we need to at least add a comment in the code about why context save/restore is needed in this particular case and make it clear that this is not something that other drivers should copy because they most likely won't be needing it.
Given the above it also doesn't sound to me like there's a real problem, or at least that the bug is somewhere else. A consumer should always be responsible for applying the pre-suspend state upon resume and it sounds like that would be true after patch 7. Since sysfs is just a regular consumer, the same should apply for sysfs-controlled PWMs as well.
Ok, I was not aware that for PWM the consumer is supposed to always be the one to restore the state. If that is the rule then we should probably just drop the save/restore suspend/resume code from pwm-lpss.
It seems that I'm actually responsible for adding that suspend/resume code in the first place, see commit 1d375b58c12f ("pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume") although the ctrl-register was already being saved/restored before that commit but then by the acpi/acpi_lpss.c code.
One worry after dropping the suspend/resume save/restore code is what happens if the controller was enabled at the moment the system suspends and the consumers first post resume apply() call has pwm_state.enabled set too.
Currently pwm_lpss_apply() looks like this:
if (state->enabled) { if (!pwm_is_enabled(pwm)) { pm_runtime_get_sync(chip->dev); ret = pwm_lpss_prepare_enable(lpwm, pwm, state, true); if (ret) pm_runtime_put(chip->dev); } else { ret = pwm_lpss_prepare_enable(lpwm, pwm, state, false); } } else if (pwm_is_enabled(pwm)) {
Where the true / false parameter to pwm_lpss_prepare_enable() decides if pwm_lpss_prepare_enable() sets the enable bit in the controllers ctrl register, or if it skips that.
If we then come from a full system suspend (controller loses state, comes up with enable bit cleared) we will still enter the:
ret = pwm_lpss_prepare_enable(lpwm, pwm, state, false);
Path since the !pwm_is_enabled(pwm) check checks the pwm_state struct, not the actual hw-enabled bit and then we do not (re)set the enabled after resume as we should when apply() is called with pwm_state.enabled set.
Fixing this is easy though, we still need to check for the disabled -> enabled transition for runtime pm refcounting, but we can also tell pwm_lpss_prepare_enable() to set the enable bit in the other path, this will be a no-op in case it is already set.
So then the new apply() code would become:
if (state->enabled) { if (!pwm_is_enabled(pwm)) { pm_runtime_get_sync(chip->dev); ret = pwm_lpss_prepare_enable(lpwm, pwm, state, true); if (ret) pm_runtime_put(chip->dev); } else { ret = pwm_lpss_prepare_enable(lpwm, pwm, state, true); } } else if (pwm_is_enabled(pwm)) {
(and we can even optimize out the enable parameter to pwm_lpss_prepare_enable then and always make it set the enable bit).
Together with patch 07/16 will make apply() always work independent of the state the controller was in before it got called. Which in light of all the subtle issues we have seen surrounding this is likely a good thing.
And with the fix to make apply() fully independent of the previous state of the controller, I'm all for dropping the suspend/resume state save/restore code. Doing that makes things more KISS so I like it :)
So at step 6, if the PWM was enabled before, we must set the update bit, and then wait for it to clear again so the controller is ready for subsequent updates. The waiting for it to clear again needs to happen before or after setting the enable bit depending on the hw generation, which leads to this patch.
But all of that should be happening as part of the call to pwm_apply_state(), right? That path should be taken for all consumers on resume, including sysfs.
Ack.
<snip>
See above, apply() was trying to be smart but the restore of ctrl on resume without setting the update bit was tricking it. That being too smart for its own good is removed in 7/16 as you rightfully point out. But this patch is still necessary for the PWM controller to be in the expected state between resume and the first apply() after resume (which may be quite a long time in the future when using e.g. the sysfs API).
Like I said, the sysfs code should be resuming any exported PWMs on resume just like any other consumer.
Obviously it's always up to the consumer to call pwm_apply_state() at the right time. If that's "too late" for some reason, then that's a bug in the consumer driver. But as I explained above there are a number of cases where restoring context in the PWM driver itself doesn't work because it can cause sequencing issues.
Ack, I was not aware that PWM consumers are responsible for restoring their own state on resume. If that is the case then:
TL;DR:
1. We (I) should make apply() work independent of the current hardware state, instead of having it make various assumptions about that state as it now does.
2. We (I) should drop the suspend/resume save/restore state handlers from pwm-lpss completely.
So I believe that I should prepare yet another version of this patch-set replacing 06/17 and 07/17 with 2 patches doing these 2 things.
Thierry, Andy, does that sound good to you ?
Regards,
Hans
On Mon, Aug 31, 2020 at 07:57:30PM +0200, Hans de Goede wrote:
On 8/31/20 3:15 PM, Thierry Reding wrote:
On Mon, Aug 31, 2020 at 01:46:28PM +0200, Hans de Goede wrote:
On 8/31/20 1:10 PM, Thierry Reding wrote:
On Sun, Aug 30, 2020 at 02:57:42PM +0200, Hans de Goede wrote:
Before this commit a suspend + resume of the LPSS PWM controller would result in the controller being reset to its defaults of output-freq = clock/256, duty-cycle=100%, until someone changes to the output-freq and/or duty-cycle are made.
This problem has been masked so far because the main consumer (the i915 driver) was always making duty-cycle changes on resume. With the conversion of the i915 driver to the atomic PWM API the driver now only disables/enables the PWM on suspend/resume leaving the output-freq and duty as is, triggering this problem.
Doesn't this imply that there's another bug at play here? At the PWM API level you're applying a state and it's up to the driver to ensure that the hardware state after ->apply() is what the software has requested.
If you only switch the enable state and that doesn't cause period and duty cycle to be updated it means that your driver isn't writing those registers when it should be.
Right, the driver was not committing those as it should *on resume*, that and it skips setting the update bit on the subsequent enable, which is an optimization which gets removed in 7/17.
Before switching the i915 driver over to atomic, when the LPSS-PWM was used for the backlight we got the following order on suspend/resume
- Set duty-cycle to 0%
- Set enabled to 0
- Save ctrl reg
- Power-off PWM controller, it now looses all its state
- Power-on PWM ctrl
- Restore ctrl reg (as a single reg write)
- Set enabled to 1, at this point one would expect the
duty/freq from the restored ctrl-reg to apply, but: a) The resume code never sets the update bit (which this commit fixes); and b) On applying the pwm_state with enabled=1 the code applying the state does this (before setting the enabled bit in the ctrl reg):
if (orig_ctrl != ctrl) { pwm_lpss_write(pwm, ctrl); pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE); } and since the restore of the ctrl reg set the old duty/freq the writes are skipped, so the update bit never gets set.
- Set duty-cycle to the pre-suspend value (which is not 0)
this does cause a change in the ctrl-reg, so now the update flag does get set.
Note that 1-2 and 7-8 are both done by the non atomic i915 code, when moving the i915 code to atomic I decided that having these 2 separate steps here is non-sense, so the new i915 code just toggles the enable bit. So in essence the new atomic PWM i915 code drops step 1 and 8.
Dropping steps 8 means that the update bit never gets set and we end up with the PWM running at its power-on-reset duty cycle.
You are correct in your remark to patch 7/17 that since that removes the if (orig_ctrl != ctrl) for the writes that now step 7 will be sufficient to get the PWM to work again. But that only takes the i915 usage into account.
What if the PWM is used through the sysfs userspace API? Then only steps 3-6 will happen on suspend-resume and without fixing step 6 to properly restore the PWM controller in its pre-resume state (this patch) it will once again be running at its power-on-reset defaults instead of the values from the restored control register.
Actually PWM's sysfs code has suspend/resume callbacks that basically make sysfs just a regular consumer of PWMs. So they do end up doing a pwm_apply_state() on the PWM as well on suspend and restore the state from before suspend on resume.
This was done very specifically because the suspend/resume order can be unexpected under some circumstances, so for PWM we really want for the consumer to always have ultimate control over when precisely the PWM is restored on resume.
The reason why we did this was because people observed weird glitches on suspend/resume with different severity. In some cases a backlight would be resumed before the display controller had had a chance to start sending frames, causing on-screen corruption in some cases (such as smart displays) and in other cases a PWM-controller regulator would be resumed too late or too early, which I think was causing some issue with the CPUs not working properly on resume.
So I'd prefer not to have any PWM driver save and restore its own context on suspend/resume, because that's inevitably going to cause unexpected behaviour at some point. If it's absolutely necessary we can of course still do that, but I think in that case we need to at least add a comment in the code about why context save/restore is needed in this particular case and make it clear that this is not something that other drivers should copy because they most likely won't be needing it.
Given the above it also doesn't sound to me like there's a real problem, or at least that the bug is somewhere else. A consumer should always be responsible for applying the pre-suspend state upon resume and it sounds like that would be true after patch 7. Since sysfs is just a regular consumer, the same should apply for sysfs-controlled PWMs as well.
Ok, I was not aware that for PWM the consumer is supposed to always be the one to restore the state. If that is the rule then we should probably just drop the save/restore suspend/resume code from pwm-lpss.
It seems that I'm actually responsible for adding that suspend/resume code in the first place, see commit 1d375b58c12f ("pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume") although the ctrl-register was already being saved/restored before that commit but then by the acpi/acpi_lpss.c code.
One worry after dropping the suspend/resume save/restore code is what happens if the controller was enabled at the moment the system suspends and the consumers first post resume apply() call has pwm_state.enabled set too.
Currently pwm_lpss_apply() looks like this:
if (state->enabled) { if (!pwm_is_enabled(pwm)) { pm_runtime_get_sync(chip->dev); ret = pwm_lpss_prepare_enable(lpwm, pwm, state, true); if (ret) pm_runtime_put(chip->dev); } else { ret = pwm_lpss_prepare_enable(lpwm, pwm, state, false); } } else if (pwm_is_enabled(pwm)) {Where the true / false parameter to pwm_lpss_prepare_enable() decides if pwm_lpss_prepare_enable() sets the enable bit in the controllers ctrl register, or if it skips that.
If we then come from a full system suspend (controller loses state, comes up with enable bit cleared) we will still enter the:
ret = pwm_lpss_prepare_enable(lpwm, pwm, state, false);Path since the !pwm_is_enabled(pwm) check checks the pwm_state struct, not the actual hw-enabled bit and then we do not (re)set the enabled after resume as we should when apply() is called with pwm_state.enabled set.
Fixing this is easy though, we still need to check for the disabled -> enabled transition for runtime pm refcounting, but we can also tell pwm_lpss_prepare_enable() to set the enable bit in the other path, this will be a no-op in case it is already set.
So then the new apply() code would become:
if (state->enabled) { if (!pwm_is_enabled(pwm)) { pm_runtime_get_sync(chip->dev); ret = pwm_lpss_prepare_enable(lpwm, pwm, state, true); if (ret) pm_runtime_put(chip->dev); } else { ret = pwm_lpss_prepare_enable(lpwm, pwm, state, true); } } else if (pwm_is_enabled(pwm)) {(and we can even optimize out the enable parameter to pwm_lpss_prepare_enable then and always make it set the enable bit).
Together with patch 07/16 will make apply() always work independent of the state the controller was in before it got called. Which in light of all the subtle issues we have seen surrounding this is likely a good thing.
And with the fix to make apply() fully independent of the previous state of the controller, I'm all for dropping the suspend/resume state save/restore code. Doing that makes things more KISS so I like it :)
So at step 6, if the PWM was enabled before, we must set the update bit, and then wait for it to clear again so the controller is ready for subsequent updates. The waiting for it to clear again needs to happen before or after setting the enable bit depending on the hw generation, which leads to this patch.
But all of that should be happening as part of the call to pwm_apply_state(), right? That path should be taken for all consumers on resume, including sysfs.
Ack.
<snip>
See above, apply() was trying to be smart but the restore of ctrl on resume without setting the update bit was tricking it. That being too smart for its own good is removed in 7/16 as you rightfully point out. But this patch is still necessary for the PWM controller to be in the expected state between resume and the first apply() after resume (which may be quite a long time in the future when using e.g. the sysfs API).
Like I said, the sysfs code should be resuming any exported PWMs on resume just like any other consumer.
Obviously it's always up to the consumer to call pwm_apply_state() at the right time. If that's "too late" for some reason, then that's a bug in the consumer driver. But as I explained above there are a number of cases where restoring context in the PWM driver itself doesn't work because it can cause sequencing issues.
Ack, I was not aware that PWM consumers are responsible for restoring their own state on resume. If that is the case then:
TL;DR:
- We (I) should make apply() work independent of the current
hardware state, instead of having it make various assumptions about that state as it now does.
- We (I) should drop the suspend/resume save/restore state
handlers from pwm-lpss completely.
So I believe that I should prepare yet another version of this patch-set replacing 06/17 and 07/17 with 2 patches doing these 2 things.
Thierry, Andy, does that sound good to you ?
Good to me, thanks!
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.
1) 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.
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)
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
Hi,
On 8/31/20 10:56 AM, Andy Shevchenko wrote:
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
Thank you.
Regards,
Hans
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
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.
Doesn't this now make patch 6/17 obsolete?
Thierry
Hi,
On 8/31/20 1:13 PM, Thierry Reding wrote:
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.
Doesn't this now make patch 6/17 obsolete?
No, there is no guarantee we will get any changes soon after resume, so we must restore the state properly on resume, before 5.17 we were just blindly restoring the old ctrl reg state, but if either the freq-div or the duty-cycle changes, we should also set the update bit in that case to apply the new freq-div/ duty-cycle.
This actually also helps with that case since patch 6/17 uses pwm_lpss_prepare and this makes pwm_lpss_prepare set the update but unconditionally.
Also on resume we most do the set the enable bit vs set the update bit in the right order, depending on the generation of the SoC in which the PWM controller is embedded. 6/17 fixes all this by resume, by treating resume as a special case of apply() taking all the steps apply does.
Regards,
Hans
On Mon, Aug 31, 2020 at 01:26:46PM +0200, Hans de Goede wrote:
Hi,
On 8/31/20 1:13 PM, Thierry Reding wrote:
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.
Doesn't this now make patch 6/17 obsolete?
No, there is no guarantee we will get any changes soon after resume, so we must restore the state properly on resume, before 5.17 we were just blindly restoring the old ctrl reg state, but if either the freq-div or the duty-cycle changes, we should also set the update bit in that case to apply the new freq-div/ duty-cycle.
Hm... I didn't realize the driver was already saving/restoring context before this. And from a quick look through the subsystem it looks like I've done a pretty poor job of enforcing the "no context save/restore from PWM drivers" rule. There are some cases that have had this support since before we realized that this is problematic, but I think at least pwm-img is newer than that and should never have had that code either.
This actually also helps with that case since patch 6/17 uses pwm_lpss_prepare and this makes pwm_lpss_prepare set the update but unconditionally.
Also on resume we most do the set the enable bit vs set the update bit in the right order, depending on the generation of the SoC in which the PWM controller is embedded. 6/17 fixes all this by resume, by treating resume as a special case of apply() taking all the steps apply does.
As I mentioned earlier this works only under the assumption that the suspend/resume order is correct. And that's possibly true for LPSS. It won't work in the general case, though, because a backlight could end up suspending/resuming completely out of sync with the rest of the display pipeline and that's not something that we want.
I would expect that on i915 you also do have a controlled call sequence that LPSS is part of, so I would expect that some consumer would eventually call pwm_apply_state() and then any new settings would get applied. Yes, that may perhaps be not immediately at the point where the LPSS resumes, but it should be exactly at the point where the consumer wants to enable it and therefore the only point where you can expect it to make sense to enable the PWM.
Anyway, if this really turns out to be the only way to make this work I can't object to it. But if you do rely on this, perhaps just make a mental note that this can lead to sequencing problems that you may potentially run into at some point.
Thierry
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: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- Changes in v3: - Use NSEC_PER_USEC instead of adding a new (non-sensical) NSEC_PER_MHZ define --- drivers/pwm/pwm-crc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index 272eeb071147..c056eb9b858c 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -21,8 +21,8 @@
#define PWM_MAX_LEVEL 0xFF
-#define PWM_BASE_CLK 6000000 /* 6 MHz */ -#define PWM_MAX_PERIOD_NS 21333 /* 46.875KHz */ +#define PWM_BASE_CLK_MHZ 6 /* 6 MHz */ +#define PWM_MAX_PERIOD_NS 5461333 /* 183 Hz */
/** * struct crystalcove_pwm - Crystal Cove PWM controller @@ -72,7 +72,7 @@ static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm,
/* changing the clk divisor, need to disable fisrt */ crc_pwm_disable(c, pwm); - clk_div = PWM_BASE_CLK * period_ns / NSEC_PER_SEC; + clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_USEC);
regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, clk_div | PWM_OUTPUT_ENABLE);
On Sun, Aug 30, 2020 at 02:57:44PM +0200, Hans de Goede wrote:
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: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com
Changes in v3:
- Use NSEC_PER_USEC instead of adding a new (non-sensical) NSEC_PER_MHZ define
drivers/pwm/pwm-crc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Acked-by: Thierry Reding treding@nvidia.com
On Sun, Aug 30, 2020 at 02:57:44PM +0200, Hans de Goede wrote:
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: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com
Changes in v3:
- Use NSEC_PER_USEC instead of adding a new (non-sensical) NSEC_PER_MHZ define
drivers/pwm/pwm-crc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Acked-by: Thierry Reding thierry.reding@gmail.com
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: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- Changes in v3: - Introduce crc_pwm_calc_clk_div() here instead of later in the patch-set to reduce the amount of churn in the patch-set a bit --- drivers/pwm/pwm-crc.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index c056eb9b858c..44ec7d5b63e1 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -22,7 +22,7 @@ #define PWM_MAX_LEVEL 0xFF
#define PWM_BASE_CLK_MHZ 6 /* 6 MHz */ -#define PWM_MAX_PERIOD_NS 5461333 /* 183 Hz */ +#define PWM_MAX_PERIOD_NS 5461334 /* 183 Hz */
/** * struct crystalcove_pwm - Crystal Cove PWM controller @@ -39,6 +39,18 @@ static inline struct crystalcove_pwm *to_crc_pwm(struct pwm_chip *pc) return container_of(pc, struct crystalcove_pwm, chip); }
+static int crc_pwm_calc_clk_div(int period_ns) +{ + int clk_div; + + clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_USEC); + /* clk_div 1 - 128, maps to register values 0-127 */ + if (clk_div > 0) + clk_div--; + + return clk_div; +} + static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm) { struct crystalcove_pwm *crc_pwm = to_crc_pwm(c); @@ -68,11 +80,10 @@ static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm, }
if (pwm_get_period(pwm) != period_ns) { - int clk_div; + int clk_div = crc_pwm_calc_clk_div(period_ns);
/* changing the clk divisor, need to disable fisrt */ crc_pwm_disable(c, pwm); - clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_USEC);
regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, clk_div | PWM_OUTPUT_ENABLE);
On Sun, Aug 30, 2020 at 02:57:45PM +0200, Hans de Goede wrote:
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: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com
Changes in v3:
- Introduce crc_pwm_calc_clk_div() here instead of later in the patch-set to reduce the amount of churn in the patch-set a bit
drivers/pwm/pwm-crc.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
Acked-by: Thierry Reding thierry.reding@gmail.com
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: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/pwm/pwm-crc.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index 44ec7d5b63e1..81232da0c767 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -82,14 +82,11 @@ static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm, if (pwm_get_period(pwm) != period_ns) { int clk_div = crc_pwm_calc_clk_div(period_ns);
- /* changing the clk divisor, need to disable fisrt */ - crc_pwm_disable(c, pwm); + /* changing the clk divisor, clear PWM_OUTPUT_ENABLE first */ + regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, 0);
regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, clk_div | PWM_OUTPUT_ENABLE); - - /* enable back */ - crc_pwm_enable(c, pwm); }
/* change the pwm duty cycle */
On Sun, Aug 30, 2020 at 02:57:46PM +0200, Hans de Goede wrote:
The pwm-crc code is using 2 different enable bits:
- bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE)
- 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: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/pwm/pwm-crc.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
Acked-by: Thierry Reding thierry.reding@gmail.com
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: Uwe Kleine-König u.kleine-koenig@pengutronix.de Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- Changes in v3: - Remove paragraph about tri-stating the output from the commit message, we don't have a datasheet so this was just an unfounded guess --- drivers/pwm/pwm-crc.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index 81232da0c767..b72008c9b072 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -54,7 +54,9 @@ static int crc_pwm_calc_clk_div(int period_ns) static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm) { struct crystalcove_pwm *crc_pwm = to_crc_pwm(c); + int div = crc_pwm_calc_clk_div(pwm_get_period(pwm));
+ regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, div | PWM_OUTPUT_ENABLE); regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 1);
return 0; @@ -63,8 +65,10 @@ static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm) static void crc_pwm_disable(struct pwm_chip *c, struct pwm_device *pwm) { struct crystalcove_pwm *crc_pwm = to_crc_pwm(c); + int div = crc_pwm_calc_clk_div(pwm_get_period(pwm));
regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 0); + regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, div); }
static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm,
On Sun, Aug 30, 2020 at 02:57:47PM +0200, Hans de Goede wrote:
The pwm-crc code is using 2 different enable bits:
- bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE)
- 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: Uwe Kleine-König u.kleine-koenig@pengutronix.de Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com
Changes in v3:
- Remove paragraph about tri-stating the output from the commit message, we don't have a datasheet so this was just an unfounded guess
drivers/pwm/pwm-crc.c | 4 ++++ 1 file changed, 4 insertions(+)
Acked-by: Thierry Reding thierry.reding@gmail.com
Replace the enable, disable and config pwm_ops with an apply op, to support the new atomic PWM API.
Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- Changes in v6: - Rebase on 5.9-rc1 - Use do_div when calculating level because pwm_state.period and .duty_cycle are now u64
Changes in v3: - Keep crc_pwm_calc_clk_div() helper to avoid needless churn --- drivers/pwm/pwm-crc.c | 89 ++++++++++++++++++++++++++----------------- 1 file changed, 54 insertions(+), 35 deletions(-)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index b72008c9b072..27dc30882424 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -51,59 +51,78 @@ static int crc_pwm_calc_clk_div(int period_ns) return clk_div; }
-static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm) +static int crc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) { - struct crystalcove_pwm *crc_pwm = to_crc_pwm(c); - int div = crc_pwm_calc_clk_div(pwm_get_period(pwm)); + struct crystalcove_pwm *crc_pwm = to_crc_pwm(chip); + struct device *dev = crc_pwm->chip.dev; + int err;
- regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, div | PWM_OUTPUT_ENABLE); - regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 1); + if (state->period > PWM_MAX_PERIOD_NS) { + dev_err(dev, "un-supported period_ns\n"); + return -EINVAL; + }
- return 0; -} + if (state->polarity != PWM_POLARITY_NORMAL) + return -EOPNOTSUPP;
-static void crc_pwm_disable(struct pwm_chip *c, struct pwm_device *pwm) -{ - struct crystalcove_pwm *crc_pwm = to_crc_pwm(c); - int div = crc_pwm_calc_clk_div(pwm_get_period(pwm)); + if (pwm_is_enabled(pwm) && !state->enabled) { + err = regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 0); + if (err) { + dev_err(dev, "Error writing BACKLIGHT_EN %d\n", err); + return err; + } + }
- regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 0); - regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, div); -} + if (pwm_get_duty_cycle(pwm) != state->duty_cycle || + pwm_get_period(pwm) != state->period) { + u64 level = state->duty_cycle * PWM_MAX_LEVEL;
-static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm, - int duty_ns, int period_ns) -{ - struct crystalcove_pwm *crc_pwm = to_crc_pwm(c); - struct device *dev = crc_pwm->chip.dev; - int level; + do_div(level, state->period);
- if (period_ns > PWM_MAX_PERIOD_NS) { - dev_err(dev, "un-supported period_ns\n"); - return -EINVAL; + err = regmap_write(crc_pwm->regmap, PWM0_DUTY_CYCLE, level); + if (err) { + dev_err(dev, "Error writing PWM0_DUTY_CYCLE %d\n", err); + return err; + } }
- if (pwm_get_period(pwm) != period_ns) { - int clk_div = crc_pwm_calc_clk_div(period_ns); - + if (pwm_is_enabled(pwm) && state->enabled && + pwm_get_period(pwm) != state->period) { /* changing the clk divisor, clear PWM_OUTPUT_ENABLE first */ - regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, 0); + err = regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, 0); + if (err) { + dev_err(dev, "Error writing PWM0_CLK_DIV %d\n", err); + return err; + } + }
- regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, - clk_div | PWM_OUTPUT_ENABLE); + if (pwm_get_period(pwm) != state->period || + pwm_is_enabled(pwm) != state->enabled) { + int clk_div = crc_pwm_calc_clk_div(state->period); + int pwm_output_enable = state->enabled ? PWM_OUTPUT_ENABLE : 0; + + err = regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, + clk_div | pwm_output_enable); + if (err) { + dev_err(dev, "Error writing PWM0_CLK_DIV %d\n", err); + return err; + } }
- /* change the pwm duty cycle */ - level = duty_ns * PWM_MAX_LEVEL / period_ns; - regmap_write(crc_pwm->regmap, PWM0_DUTY_CYCLE, level); + if (!pwm_is_enabled(pwm) && state->enabled) { + err = regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 1); + if (err) { + dev_err(dev, "Error writing BACKLIGHT_EN %d\n", err); + return err; + } + }
return 0; }
static const struct pwm_ops crc_pwm_ops = { - .config = crc_pwm_config, - .enable = crc_pwm_enable, - .disable = crc_pwm_disable, + .apply = crc_pwm_apply, };
static int crystalcove_pwm_probe(struct platform_device *pdev)
On Sun, Aug 30, 2020 at 02:57:48PM +0200, Hans de Goede wrote:
Replace the enable, disable and config pwm_ops with an apply op, to support the new atomic PWM API.
Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com
Changes in v6:
- Rebase on 5.9-rc1
- Use do_div when calculating level because pwm_state.period and .duty_cycle are now u64
Changes in v3:
- Keep crc_pwm_calc_clk_div() helper to avoid needless churn
drivers/pwm/pwm-crc.c | 89 ++++++++++++++++++++++++++----------------- 1 file changed, 54 insertions(+), 35 deletions(-)
Acked-by: Thierry Reding thierry.reding@gmail.com
Implement the pwm_ops.get_state() method to complete the support for the new atomic PWM API.
Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- Changes in v6: - Rebase on 5.9-rc1 - Use DIV_ROUND_UP_ULL because pwm_state.period and .duty_cycle are now u64
Changes in v5: - Fix an indentation issue
Changes in v4: - Use DIV_ROUND_UP when calculating the period and duty_cycle from the controller's register values
Changes in v3: - Add Andy's Reviewed-by tag - Remove extra whitespace to align some code after assignments (requested by Uwe Kleine-König) --- drivers/pwm/pwm-crc.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index 27dc30882424..ecfdfac0c2d9 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -121,8 +121,39 @@ static int crc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, return 0; }
+static void crc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct crystalcove_pwm *crc_pwm = to_crc_pwm(chip); + struct device *dev = crc_pwm->chip.dev; + unsigned int clk_div, clk_div_reg, duty_cycle_reg; + int error; + + error = regmap_read(crc_pwm->regmap, PWM0_CLK_DIV, &clk_div_reg); + if (error) { + dev_err(dev, "Error reading PWM0_CLK_DIV %d\n", error); + return; + } + + error = regmap_read(crc_pwm->regmap, PWM0_DUTY_CYCLE, &duty_cycle_reg); + if (error) { + dev_err(dev, "Error reading PWM0_DUTY_CYCLE %d\n", error); + return; + } + + clk_div = (clk_div_reg & ~PWM_OUTPUT_ENABLE) + 1; + + state->period = + DIV_ROUND_UP(clk_div * NSEC_PER_USEC * 256, PWM_BASE_CLK_MHZ); + state->duty_cycle = + DIV_ROUND_UP_ULL(duty_cycle_reg * state->period, PWM_MAX_LEVEL); + state->polarity = PWM_POLARITY_NORMAL; + state->enabled = !!(clk_div_reg & PWM_OUTPUT_ENABLE); +} + static const struct pwm_ops crc_pwm_ops = { .apply = crc_pwm_apply, + .get_state = crc_pwm_get_state, };
static int crystalcove_pwm_probe(struct platform_device *pdev)
On Sun, Aug 30, 2020 at 02:57:49PM +0200, Hans de Goede wrote:
Implement the pwm_ops.get_state() method to complete the support for the new atomic PWM API.
Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com
Changes in v6:
- Rebase on 5.9-rc1
- Use DIV_ROUND_UP_ULL because pwm_state.period and .duty_cycle are now u64
Changes in v5:
- Fix an indentation issue
Changes in v4:
- Use DIV_ROUND_UP when calculating the period and duty_cycle from the controller's register values
Changes in v3:
- Add Andy's Reviewed-by tag
- Remove extra whitespace to align some code after assignments (requested by Uwe Kleine-König)
drivers/pwm/pwm-crc.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
Acked-by: Thierry Reding thierry.reding@gmail.com
Factor the code which checks and drm_dbg_kms-s the VBT PWM frequency out of get_backlight_max_vbt().
This is a preparation patch for honering the VBT PWM frequency for devices which use an external PWM controller (devices using pwm_setup_backlight()).
Acked-by: Jani Nikula jani.nikula@intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/i915/display/intel_panel.c | 27 ++++++++++++++-------- 1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c index 4072d7062efd..df7472a3b9f8 100644 --- a/drivers/gpu/drm/i915/display/intel_panel.c +++ b/drivers/gpu/drm/i915/display/intel_panel.c @@ -1543,18 +1543,9 @@ static u32 vlv_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz) return DIV_ROUND_CLOSEST(clock, pwm_freq_hz * mul); }
-static u32 get_backlight_max_vbt(struct intel_connector *connector) +static u16 get_vbt_pwm_freq(struct drm_i915_private *dev_priv) { - struct drm_i915_private *dev_priv = to_i915(connector->base.dev); - struct intel_panel *panel = &connector->panel; u16 pwm_freq_hz = dev_priv->vbt.backlight.pwm_freq_hz; - u32 pwm; - - if (!panel->backlight.hz_to_pwm) { - drm_dbg_kms(&dev_priv->drm, - "backlight frequency conversion not supported\n"); - return 0; - }
if (pwm_freq_hz) { drm_dbg_kms(&dev_priv->drm, @@ -1567,6 +1558,22 @@ static u32 get_backlight_max_vbt(struct intel_connector *connector) pwm_freq_hz); }
+ return pwm_freq_hz; +} + +static u32 get_backlight_max_vbt(struct intel_connector *connector) +{ + struct drm_i915_private *dev_priv = to_i915(connector->base.dev); + struct intel_panel *panel = &connector->panel; + u16 pwm_freq_hz = get_vbt_pwm_freq(dev_priv); + u32 pwm; + + if (!panel->backlight.hz_to_pwm) { + drm_dbg_kms(&dev_priv->drm, + "backlight frequency conversion not supported\n"); + return 0; + } + pwm = panel->backlight.hz_to_pwm(connector, pwm_freq_hz); if (!pwm) { drm_dbg_kms(&dev_priv->drm,
So far for devices using an external PWM controller (devices using pwm_setup_backlight()), we have been hardcoding the period-time passed to pwm_config() to 21333 ns.
I suspect this was done because many VBTs set the PWM frequency to 200 which corresponds to a period-time of 5000000 ns, which greatly exceeds the PWM_MAX_PERIOD_NS define in the Crystal Cove PMIC PWM driver, which used to be 21333.
This PWM_MAX_PERIOD_NS define was actually based on a bug in the PWM driver where its period and duty-cycle times where off by a factor of 256.
Due to this bug the hardcoded CRC_PMIC_PWM_PERIOD_NS value of 21333 would result in the PWM driver using its divider of 128, which would result in a PWM output frequency of 6000000 Hz / 256 / 128 = 183 Hz. So actually pretty close to the default VBT value of 200 Hz.
Now that this bug in the pwm-crc driver is fixed, we can actually use the VBT defined frequency.
This is important because:
a) With the pwm-crc driver fixed it will now translate the hardcoded CRC_PMIC_PWM_PERIOD_NS value of 21333 ns / 46 Khz to a PWM output frequency of 23 KHz (the max it can do).
b) The pwm-lpss driver used on many models has always honored the 21333 ns / 46 Khz request
Some panels do not like such high output frequencies. E.g. on a Terra Pad 1061 tablet, using the LPSS PWM controller, the backlight would go from off to max, when changing the sysfs backlight brightness value from 90-100%, anything under aprox. 90% would turn the backlight fully off.
Honoring the VBT specified PWM frequency will also hopefully fix the various bug reports which we have received about users perceiving the backlight to flicker after a suspend/resume cycle.
Acked-by: Jani Nikula jani.nikula@intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- .../drm/i915/display/intel_display_types.h | 1 + drivers/gpu/drm/i915/display/intel_panel.c | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index e8f809161c75..7171e7c8d928 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -223,6 +223,7 @@ struct intel_panel { bool util_pin_active_low; /* bxt+ */ u8 controller; /* bxt+ only */ struct pwm_device *pwm; + int pwm_period_ns;
/* DPCD backlight */ u8 pwmgen_bit_count; diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c index df7472a3b9f8..5a13089d2fc0 100644 --- a/drivers/gpu/drm/i915/display/intel_panel.c +++ b/drivers/gpu/drm/i915/display/intel_panel.c @@ -40,8 +40,6 @@ #include "intel_dsi_dcs_backlight.h" #include "intel_panel.h"
-#define CRC_PMIC_PWM_PERIOD_NS 21333 - void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -597,7 +595,7 @@ static u32 pwm_get_backlight(struct intel_connector *connector) int duty_ns;
duty_ns = pwm_get_duty_cycle(panel->backlight.pwm); - return DIV_ROUND_UP(duty_ns * 100, CRC_PMIC_PWM_PERIOD_NS); + return DIV_ROUND_UP(duty_ns * 100, panel->backlight.pwm_period_ns); }
static void lpt_set_backlight(const struct drm_connector_state *conn_state, u32 level) @@ -671,9 +669,10 @@ static void bxt_set_backlight(const struct drm_connector_state *conn_state, u32 static void pwm_set_backlight(const struct drm_connector_state *conn_state, u32 level) { struct intel_panel *panel = &to_intel_connector(conn_state->connector)->panel; - int duty_ns = DIV_ROUND_UP(level * CRC_PMIC_PWM_PERIOD_NS, 100); + int duty_ns = DIV_ROUND_UP(level * panel->backlight.pwm_period_ns, 100);
- pwm_config(panel->backlight.pwm, duty_ns, CRC_PMIC_PWM_PERIOD_NS); + pwm_config(panel->backlight.pwm, duty_ns, + panel->backlight.pwm_period_ns); }
static void @@ -1917,6 +1916,9 @@ static int pwm_setup_backlight(struct intel_connector *connector, return -ENODEV; }
+ panel->backlight.pwm_period_ns = NSEC_PER_SEC / + get_vbt_pwm_freq(dev_priv); + /* * FIXME: pwm_apply_args() should be removed when switching to * the atomic PWM API. @@ -1926,9 +1928,10 @@ static int pwm_setup_backlight(struct intel_connector *connector, panel->backlight.min = 0; /* 0% */ panel->backlight.max = 100; /* 100% */ level = intel_panel_compute_brightness(connector, 100); - ns = DIV_ROUND_UP(level * CRC_PMIC_PWM_PERIOD_NS, 100); + ns = DIV_ROUND_UP(level * panel->backlight.pwm_period_ns, 100);
- retval = pwm_config(panel->backlight.pwm, ns, CRC_PMIC_PWM_PERIOD_NS); + retval = pwm_config(panel->backlight.pwm, ns, + panel->backlight.pwm_period_ns); if (retval < 0) { drm_err(&dev_priv->drm, "Failed to configure the pwm chip\n"); pwm_put(panel->backlight.pwm); @@ -1937,7 +1940,7 @@ static int pwm_setup_backlight(struct intel_connector *connector, }
level = DIV_ROUND_UP_ULL(pwm_get_duty_cycle(panel->backlight.pwm) * 100, - CRC_PMIC_PWM_PERIOD_NS); + panel->backlight.pwm_period_ns); panel->backlight.level = intel_panel_compute_brightness(connector, level); panel->backlight.enabled = panel->backlight.level != 0;
So far for devices using an external PWM controller (devices using pwm_setup_backlight()), we have been hardcoding the minimum allowed PWM level to 0. But several of these devices specify a non 0 minimum setting in their VBT.
Change pwm_setup_backlight() to use get_backlight_min_vbt() to get the minimum level.
Acked-by: Jani Nikula jani.nikula@intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/i915/display/intel_panel.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c index 5a13089d2fc0..2b27f9b07403 100644 --- a/drivers/gpu/drm/i915/display/intel_panel.c +++ b/drivers/gpu/drm/i915/display/intel_panel.c @@ -1925,8 +1925,8 @@ static int pwm_setup_backlight(struct intel_connector *connector, */ pwm_apply_args(panel->backlight.pwm);
- panel->backlight.min = 0; /* 0% */ panel->backlight.max = 100; /* 100% */ + panel->backlight.min = get_backlight_min_vbt(connector); level = intel_panel_compute_brightness(connector, 100); ns = DIV_ROUND_UP(level * panel->backlight.pwm_period_ns, 100);
@@ -1941,8 +1941,9 @@ static int pwm_setup_backlight(struct intel_connector *connector,
level = DIV_ROUND_UP_ULL(pwm_get_duty_cycle(panel->backlight.pwm) * 100, panel->backlight.pwm_period_ns); - panel->backlight.level = - intel_panel_compute_brightness(connector, level); + level = intel_panel_compute_brightness(connector, level); + panel->backlight.level = clamp(level, panel->backlight.min, + panel->backlight.max); panel->backlight.enabled = panel->backlight.level != 0;
drm_info(&dev_priv->drm, "Using %s PWM for LCD backlight control\n",
Now that the PWM drivers which we use have been converted to the atomic PWM API, we can move the i915 panel code over to using the atomic PWM API.
The removes a long standing FIXME and this removes a flicker where the backlight brightness would jump to 100% when i915 loads even if using the fastset path.
Note that this commit also simplifies pwm_disable_backlight(), by dropping the intel_panel_actually_set_backlight(..., 0) call. This call sets the PWM to 0% duty-cycle. I believe that this call was only present as a workaround for a bug in the pwm-crc.c driver where it failed to clear the PWM_OUTPUT_ENABLE bit. This is fixed by an earlier patch in this series.
After the dropping of this workaround, the usleep call, which seems unnecessary to begin with, has no useful effect anymore, so drop that too.
Acked-by: Jani Nikula jani.nikula@intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- Changes in v7: - Fix a u64 divide leading to undefined reference to `__udivdi3' errors on 32 bit platforms by casting the divisor to an unsigned long
Changes in v6: - Drop the pwm_get_period() check in pwm_setup(), it is now longer needed now that we use pwm_get_relative_duty_cycle()
Changes in v4: - Add a note to the commit message about the dropping of the intel_panel_actually_set_backlight() and usleep() calls from pwm_disable_backlight() - Use the pwm_set/get_relative_duty_cycle() helpers instead of using DIY code for this --- .../drm/i915/display/intel_display_types.h | 3 +- drivers/gpu/drm/i915/display/intel_panel.c | 70 ++++++++----------- 2 files changed, 33 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 7171e7c8d928..e3ebe7c313ba 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -28,6 +28,7 @@
#include <linux/async.h> #include <linux/i2c.h> +#include <linux/pwm.h> #include <linux/sched/clock.h>
#include <drm/drm_atomic.h> @@ -223,7 +224,7 @@ struct intel_panel { bool util_pin_active_low; /* bxt+ */ u8 controller; /* bxt+ only */ struct pwm_device *pwm; - int pwm_period_ns; + struct pwm_state pwm_state;
/* DPCD backlight */ u8 pwmgen_bit_count; diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c index 2b27f9b07403..2eb1a3a93df1 100644 --- a/drivers/gpu/drm/i915/display/intel_panel.c +++ b/drivers/gpu/drm/i915/display/intel_panel.c @@ -592,10 +592,10 @@ static u32 bxt_get_backlight(struct intel_connector *connector) static u32 pwm_get_backlight(struct intel_connector *connector) { struct intel_panel *panel = &connector->panel; - int duty_ns; + struct pwm_state state;
- duty_ns = pwm_get_duty_cycle(panel->backlight.pwm); - return DIV_ROUND_UP(duty_ns * 100, panel->backlight.pwm_period_ns); + pwm_get_state(panel->backlight.pwm, &state); + return pwm_get_relative_duty_cycle(&state, 100); }
static void lpt_set_backlight(const struct drm_connector_state *conn_state, u32 level) @@ -669,10 +669,9 @@ static void bxt_set_backlight(const struct drm_connector_state *conn_state, u32 static void pwm_set_backlight(const struct drm_connector_state *conn_state, u32 level) { struct intel_panel *panel = &to_intel_connector(conn_state->connector)->panel; - int duty_ns = DIV_ROUND_UP(level * panel->backlight.pwm_period_ns, 100);
- pwm_config(panel->backlight.pwm, duty_ns, - panel->backlight.pwm_period_ns); + pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100); + pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state); }
static void @@ -841,10 +840,8 @@ static void pwm_disable_backlight(const struct drm_connector_state *old_conn_sta struct intel_connector *connector = to_intel_connector(old_conn_state->connector); struct intel_panel *panel = &connector->panel;
- /* Disable the backlight */ - intel_panel_actually_set_backlight(old_conn_state, 0); - usleep_range(2000, 3000); - pwm_disable(panel->backlight.pwm); + panel->backlight.pwm_state.enabled = false; + pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state); }
void intel_panel_disable_backlight(const struct drm_connector_state *old_conn_state) @@ -1176,9 +1173,12 @@ static void pwm_enable_backlight(const struct intel_crtc_state *crtc_state, { struct intel_connector *connector = to_intel_connector(conn_state->connector); struct intel_panel *panel = &connector->panel; + int level = panel->backlight.level;
- pwm_enable(panel->backlight.pwm); - intel_panel_actually_set_backlight(conn_state, panel->backlight.level); + level = intel_panel_compute_brightness(connector, level); + pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100); + panel->backlight.pwm_state.enabled = true; + pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state); }
static void __intel_panel_enable_backlight(const struct intel_crtc_state *crtc_state, @@ -1897,8 +1897,7 @@ static int pwm_setup_backlight(struct intel_connector *connector, struct drm_i915_private *dev_priv = to_i915(dev); struct intel_panel *panel = &connector->panel; const char *desc; - u32 level, ns; - int retval; + u32 level;
/* Get the right PWM chip for DSI backlight according to VBT */ if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) { @@ -1916,35 +1915,28 @@ static int pwm_setup_backlight(struct intel_connector *connector, return -ENODEV; }
- panel->backlight.pwm_period_ns = NSEC_PER_SEC / - get_vbt_pwm_freq(dev_priv); - - /* - * FIXME: pwm_apply_args() should be removed when switching to - * the atomic PWM API. - */ - pwm_apply_args(panel->backlight.pwm); - panel->backlight.max = 100; /* 100% */ panel->backlight.min = get_backlight_min_vbt(connector); - level = intel_panel_compute_brightness(connector, 100); - ns = DIV_ROUND_UP(level * panel->backlight.pwm_period_ns, 100);
- retval = pwm_config(panel->backlight.pwm, ns, - panel->backlight.pwm_period_ns); - if (retval < 0) { - drm_err(&dev_priv->drm, "Failed to configure the pwm chip\n"); - pwm_put(panel->backlight.pwm); - panel->backlight.pwm = NULL; - return retval; - } + if (pwm_is_enabled(panel->backlight.pwm)) { + /* PWM is already enabled, use existing settings */ + pwm_get_state(panel->backlight.pwm, &panel->backlight.pwm_state);
- level = DIV_ROUND_UP_ULL(pwm_get_duty_cycle(panel->backlight.pwm) * 100, - panel->backlight.pwm_period_ns); - level = intel_panel_compute_brightness(connector, level); - panel->backlight.level = clamp(level, panel->backlight.min, - panel->backlight.max); - panel->backlight.enabled = panel->backlight.level != 0; + level = pwm_get_relative_duty_cycle(&panel->backlight.pwm_state, + 100); + level = intel_panel_compute_brightness(connector, level); + panel->backlight.level = clamp(level, panel->backlight.min, + panel->backlight.max); + panel->backlight.enabled = true; + + drm_dbg_kms(&dev_priv->drm, "PWM already enabled at freq %ld, VBT freq %d, level %d\n", + NSEC_PER_SEC / (unsigned long)panel->backlight.pwm_state.period, + get_vbt_pwm_freq(dev_priv), level); + } else { + /* Set period from VBT frequency, leave other settings at 0. */ + panel->backlight.pwm_state.period = + NSEC_PER_SEC / get_vbt_pwm_freq(dev_priv); + }
drm_info(&dev_priv->drm, "Using %s PWM for LCD backlight control\n", desc);
dri-devel@lists.freedesktop.org
-
Andy Shevchenko -
Hans de Goede -
Thierry Reding