linux-pwm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API
@ 2020-07-08 21:14 Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 01/16] ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase Hans de Goede
                   ` (16 more replies)
  0 siblings, 17 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi

Hi All,

Here is v4 of my patch series converting the i915 driver's code for
controlling the panel's backlight with an external PWM controller to
use the atomic PWM API. See below for the changelog.

Initially the plan was for this series to consist of 2 parts:
1. convert the pwm-crc driver to support the atomic PWM API and
2. convert the i915 driver's PWM code to use the atomic PWM API.

But during testing I've found a number of bugs in the pwm-lpss and I
found that the acpi_lpss code needs some special handling because of
some ugliness found in most Cherry Trail DSDTs.

So now this series has grown somewhat large and consists of 4 parts:

1. acpi_lpss fixes workarounds for Cherry Trail DSTD nastiness
2. various fixes to the pwm-lpss driver
3. convert the pwm-crc driver to support the atomic PWM API and
4. convert the i915 driver's PWM code to use the atomic PWM API

The involved acpi_lpss and pwm drivers do not see a whole lot of churn,
so the plan is to merge this all through drm-intel-next-queued (dinq)
once all the patches are reviewed / have acks.

In v4 the ACPI patches have been Acked by Rafael and the i915 patches
have been acked by Jani. So that just leaves the PWM patches.

Uwe can I get your ok / ack for merging this through the dinq branch
once you have acked al the PWM patches ?

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 + CRC-PMIC PWM
-GPD Pocket  CHT + CRC-PMIC PWM

Changelog:

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

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 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

Regards,

Hans

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v4 01/16] ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 02/16] ACPI / LPSS: Save Cherry Trail PWM ctx registers only once (at activation) Hans de Goede
                   ` (15 subsequent siblings)
  16 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi, Rafael J . Wysocki

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 = {
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 02/16] ACPI / LPSS: Save Cherry Trail PWM ctx registers only once (at activation)
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 01/16] ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 03/16] pwm: lpss: Fix off by one error in base_unit math in pwm_lpss_prepare() Hans de Goede
                   ` (14 subsequent siblings)
  16 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi, Rafael J . Wysocki

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;
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 03/16] pwm: lpss: Fix off by one error in base_unit math in pwm_lpss_prepare()
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 01/16] ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 02/16] ACPI / LPSS: Save Cherry Trail PWM ctx registers only once (at activation) Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 04/16] pwm: lpss: Add range limit check for the base_unit register value Hans de Goede
                   ` (13 subsequent siblings)
  16 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi

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;
 
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 04/16] pwm: lpss: Add range limit check for the base_unit register value
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
                   ` (2 preceding siblings ...)
  2020-07-08 21:14 ` [PATCH v4 03/16] pwm: lpss: Fix off by one error in base_unit math in pwm_lpss_prepare() Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-09 12:53   ` Andy Shevchenko
  2020-07-08 21:14 ` [PATCH v4 05/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume Hans de Goede
                   ` (12 subsequent siblings)
  16 siblings, 1 reply; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi

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")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- Change upper limit of clamp to (base_unit_range - 1)
- Add Fixes tag
---
 drivers/pwm/pwm-lpss.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 43b1fc634af1..80d0f9c64f9d 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -97,6 +97,9 @@ 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_t(unsigned long long, base_unit, 1,
+			    base_unit_range - 1);
 
 	on_time_div = 255ULL * duty_ns;
 	do_div(on_time_div, period_ns);
@@ -105,7 +108,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;
 
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 05/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
                   ` (3 preceding siblings ...)
  2020-07-08 21:14 ` [PATCH v4 04/16] pwm: lpss: Add range limit check for the base_unit register value Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-09 13:36   ` Andy Shevchenko
  2020-07-08 21:14 ` [PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0 Hans de Goede
                   ` (11 subsequent siblings)
  16 siblings, 1 reply; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi

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 makes the resume handler use pwm_lpss_apply() to
restore the settings when necessary. This fixes the output-freq and
duty-cycle being reset to their defaults on resume.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
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 | 62 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 53 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 80d0f9c64f9d..4f3d60ce9929 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -123,25 +123,31 @@ 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_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			  const struct pwm_state *state)
+static int __pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			    const struct pwm_state *state, bool from_resume)
 {
 	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
 	int ret;
 
 	if (state->enabled) {
 		if (!pwm_is_enabled(pwm)) {
-			pm_runtime_get_sync(chip->dev);
+			if (!from_resume)
+				pm_runtime_get_sync(chip->dev);
+
 			ret = pwm_lpss_is_updating(pwm);
 			if (ret) {
-				pm_runtime_put(chip->dev);
+				if (!from_resume)
+					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) {
-				pm_runtime_put(chip->dev);
+				if (!from_resume)
+					pm_runtime_put(chip->dev);
+
 				return ret;
 			}
 			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == true);
@@ -154,12 +160,20 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		}
 	} else if (pwm_is_enabled(pwm)) {
 		pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
-		pm_runtime_put(chip->dev);
+
+		if (!from_resume)
+			pm_runtime_put(chip->dev);
 	}
 
 	return 0;
 }
 
+static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			  const struct pwm_state *state)
+{
+	return __pwm_lpss_apply(chip, pwm, state, false);
+}
+
 static void pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 			       struct pwm_state *state)
 {
@@ -272,10 +286,40 @@ 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_state saved_state;
+	struct pwm_device *pwm;
+	int i, ret;
+	u32 ctrl;
 
-	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];
+
+		ctrl = pwm_lpss_read(pwm);
+		/* If we did not reach S0i3/S3 the controller keeps its state */
+		if (ctrl == lpwm->saved_ctrl[i])
+			continue;
+
+		/*
+		 * 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.
+		 * So instead of reproducing all the code from pwm_apply() here,
+		 * we just reapply the state as stored in pwm->state.
+		 */
+		saved_state = pwm->state;
+		/*
+		 * Update enabled to its actual setting for the
+		 * enabled<->disabled transitions inside apply().
+		 */
+		pwm->state.enabled = !!(ctrl & PWM_ENABLE);
+		ret = __pwm_lpss_apply(&lpwm->chip, pwm, &saved_state, true);
+		if (ret)
+			dev_err(dev, "Error restoring state on resume\n");
+	}
 
 	return 0;
 }
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
                   ` (4 preceding siblings ...)
  2020-07-08 21:14 ` [PATCH v4 05/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-09 14:50   ` Andy Shevchenko
  2020-07-08 21:14 ` [PATCH v4 07/16] pwm: crc: Fix period / duty_cycle times being off by a factor of 256 Hans de Goede
                   ` (10 subsequent siblings)
  16 siblings, 1 reply; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi

The datasheet specifies that programming the base_unit part of the
ctrl register to 0 results in a contineous low signal.

Adjust the get_state method to reflect this by setting pwm_state.period
to 1 and duty_cycle to 0.

Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- This is a new patch in v4 of this patchset
---
 drivers/pwm/pwm-lpss.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 4f3d60ce9929..4d4de45cf99b 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -192,14 +192,16 @@ static void pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	freq = base_unit * lpwm->info->clk_rate;
 	do_div(freq, base_unit_range);
-	if (freq == 0)
-		state->period = NSEC_PER_SEC;
-	else
+	if (freq == 0) {
+		/* In this case the PWM outputs a continous low signal */
+		state->period = 1;
+		state->duty_cycle = 0;
+	} else {
 		state->period = NSEC_PER_SEC / (unsigned long)freq;
-
-	on_time_div *= state->period;
-	do_div(on_time_div, 255);
-	state->duty_cycle = on_time_div;
+		on_time_div *= state->period;
+		do_div(on_time_div, 255);
+		state->duty_cycle = on_time_div;
+	}
 
 	state->polarity = PWM_POLARITY_NORMAL;
 	state->enabled = !!(ctrl & PWM_ENABLE);
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 07/16] pwm: crc: Fix period / duty_cycle times being off by a factor of 256
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
                   ` (5 preceding siblings ...)
  2020-07-08 21:14 ` [PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0 Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 08/16] pwm: crc: Fix off-by-one error in the clock-divider calculations Hans de Goede
                   ` (9 subsequent siblings)
  16 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi

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.

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);
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 08/16] pwm: crc: Fix off-by-one error in the clock-divider calculations
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
                   ` (6 preceding siblings ...)
  2020-07-08 21:14 ` [PATCH v4 07/16] pwm: crc: Fix period / duty_cycle times being off by a factor of 256 Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 09/16] pwm: crc: Fix period changes not having any effect Hans de Goede
                   ` (8 subsequent siblings)
  16 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi

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).

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);
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 09/16] pwm: crc: Fix period changes not having any effect
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
                   ` (7 preceding siblings ...)
  2020-07-08 21:14 ` [PATCH v4 08/16] pwm: crc: Fix off-by-one error in the clock-divider calculations Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 10/16] pwm: crc: Enable/disable PWM output on enable/disable Hans de Goede
                   ` (7 subsequent siblings)
  16 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi

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

I strongly suspect that the BACKLIGHT_EN register at address 0x51 really
controls a separate output-only GPIO which is connected to the LCD panels
backlight-enable input. Like how the PANEL_EN register at address 0x52
controls an output-only GPIO which is earmarked for the LCD panel's
enable pin. If this is correct then this GPIO should really be added to
the gpio-crystalcove.c driver and the PWM driver should stop poking it.
But I've been unable to come up with a definitive answer here, so I'm
keeping this as is for now.

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.

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 */
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 10/16] pwm: crc: Enable/disable PWM output on enable/disable
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
                   ` (8 preceding siblings ...)
  2020-07-08 21:14 ` [PATCH v4 09/16] pwm: crc: Fix period changes not having any effect Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 11/16] pwm: crc: Implement apply() method to support the new atomic PWM API Hans de Goede
                   ` (6 subsequent siblings)
  16 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi

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>
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,
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 11/16] pwm: crc: Implement apply() method to support the new atomic PWM API
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
                   ` (9 preceding siblings ...)
  2020-07-08 21:14 ` [PATCH v4 10/16] pwm: crc: Enable/disable PWM output on enable/disable Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 12/16] pwm: crc: Implement get_state() method Hans de Goede
                   ` (5 subsequent siblings)
  16 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi

Replace the enable, disable and config pwm_ops with an apply op,
to support the new atomic PWM API.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- Keep crc_pwm_calc_clk_div() helper to avoid needless churn
---
 drivers/pwm/pwm-crc.c | 89 ++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 36 deletions(-)

diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
index b72008c9b072..8a7f4707279c 100644
--- a/drivers/pwm/pwm-crc.c
+++ b/drivers/pwm/pwm-crc.c
@@ -51,59 +51,76 @@ 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));
-
-	regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, div | PWM_OUTPUT_ENABLE);
-	regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 1);
-
-	return 0;
-}
-
-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,
-			  int duty_ns, int period_ns)
-{
-	struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
+	struct crystalcove_pwm *crc_pwm = to_crc_pwm(chip);
 	struct device *dev = crc_pwm->chip.dev;
-	int level;
+	int err;
 
-	if (period_ns > PWM_MAX_PERIOD_NS) {
+	if (state->period > PWM_MAX_PERIOD_NS) {
 		dev_err(dev, "un-supported period_ns\n");
 		return -EINVAL;
 	}
 
-	if (pwm_get_period(pwm) != period_ns) {
-		int clk_div = crc_pwm_calc_clk_div(period_ns);
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EOPNOTSUPP;
+
+	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;
+		}
+	}
+
+	if (pwm_get_duty_cycle(pwm) != state->duty_cycle ||
+	    pwm_get_period(pwm) != state->period) {
+		int level = state->duty_cycle * PWM_MAX_LEVEL / state->period;
 
+		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_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)
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 12/16] pwm: crc: Implement get_state() method
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
                   ` (10 preceding siblings ...)
  2020-07-08 21:14 ` [PATCH v4 11/16] pwm: crc: Implement apply() method to support the new atomic PWM API Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 13/16] drm/i915: panel: Add get_vbt_pwm_freq() helper Hans de Goede
                   ` (4 subsequent siblings)
  16 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi

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 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 8a7f4707279c..e58b0979d708 100644
--- a/drivers/pwm/pwm-crc.c
+++ b/drivers/pwm/pwm-crc.c
@@ -119,8 +119,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(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)
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 13/16] drm/i915: panel: Add get_vbt_pwm_freq() helper
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
                   ` (11 preceding siblings ...)
  2020-07-08 21:14 ` [PATCH v4 12/16] pwm: crc: Implement get_state() method Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 14/16] drm/i915: panel: Honor the VBT PWM frequency for devs with an external PWM controller Hans de Goede
                   ` (3 subsequent siblings)
  16 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi, Jani Nikula

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 3c5056dbf607..8efdd9f08a08 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,
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 14/16] drm/i915: panel: Honor the VBT PWM frequency for devs with an external PWM controller
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
                   ` (12 preceding siblings ...)
  2020-07-08 21:14 ` [PATCH v4 13/16] drm/i915: panel: Add get_vbt_pwm_freq() helper Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 15/16] drm/i915: panel: Honor the VBT PWM min setting " Hans de Goede
                   ` (2 subsequent siblings)
  16 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi, Jani Nikula

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 2bf3d4cb4ea9..de32f9efb120 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 8efdd9f08a08..14e611c92194 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(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;
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 15/16] drm/i915: panel: Honor the VBT PWM min setting for devs with an external PWM controller
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
                   ` (13 preceding siblings ...)
  2020-07-08 21:14 ` [PATCH v4 14/16] drm/i915: panel: Honor the VBT PWM frequency for devs with an external PWM controller Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-08 21:14 ` [PATCH v4 16/16] drm/i915: panel: Use atomic PWM API " Hans de Goede
  2020-07-09 14:14 ` [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Sam Ravnborg
  16 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: linux-pwm, linux-acpi, Jani Nikula, intel-gfx, dri-devel,
	Andy Shevchenko, Mika Westerberg

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 14e611c92194..cb28b9908ca4 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(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",
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH v4 16/16] drm/i915: panel: Use atomic PWM API for devs with an external PWM controller
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
                   ` (14 preceding siblings ...)
  2020-07-08 21:14 ` [PATCH v4 15/16] drm/i915: panel: Honor the VBT PWM min setting " Hans de Goede
@ 2020-07-08 21:14 ` Hans de Goede
  2020-07-11  6:32   ` Uwe Kleine-König
  2020-07-09 14:14 ` [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Sam Ravnborg
  16 siblings, 1 reply; 35+ messages in thread
From: Hans de Goede @ 2020-07-08 21:14 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, intel-gfx, dri-devel, Andy Shevchenko,
	Mika Westerberg, linux-acpi, Jani Nikula

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 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    | 71 +++++++++----------
 2 files changed, 34 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 de32f9efb120..4bd9981e70a1 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 cb28b9908ca4..3d97267c8238 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,36 +1915,30 @@ 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_get_period(panel->backlight.pwm)) {
+		/* PWM is already enabled, use existing settings */
+		pwm_get_state(panel->backlight.pwm, &panel->backlight.pwm_state);
+
+		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 / 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);
 	}
 
-	level = DIV_ROUND_UP(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;
-
 	drm_info(&dev_priv->drm, "Using %s PWM for LCD backlight control\n",
 		 desc);
 	return 0;
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 04/16] pwm: lpss: Add range limit check for the base_unit register value
  2020-07-08 21:14 ` [PATCH v4 04/16] pwm: lpss: Add range limit check for the base_unit register value Hans de Goede
@ 2020-07-09 12:53   ` Andy Shevchenko
  2020-07-09 13:23     ` Hans de Goede
  0 siblings, 1 reply; 35+ messages in thread
From: Andy Shevchenko @ 2020-07-09 12:53 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, intel-gfx, dri-devel,
	Mika Westerberg, linux-acpi

On Wed, Jul 08, 2020 at 11:14:20PM +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.

And I don't see how you can get duty 100% / 0% (I don't remember which one is
equivalent to 0 in base unit) after this change. IIRC the problem here that
base unit when non-zero is always being added to the counter and it will
trigger the change of output at some point which is not what we want for 100% /
0% cases.

> 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.

It would be nice to have an example of calculus here.

> This commit fixes both issues by clamping the base_unit value to be
> between 1 and (base_unit_range - 1).

Eventually I sat and wrote all this on paper. I see now that the problem
is in out of range of the period. And strongly we should clamp rather period
to the supported range, but your solution is an equivalent.

Only question is about the 100% / 0% duty cycle.

> Fixes: 684309e5043e ("pwm: lpss: Avoid potential overflow of base_unit")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v3:
> - Change upper limit of clamp to (base_unit_range - 1)
> - Add Fixes tag
> ---
>  drivers/pwm/pwm-lpss.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index 43b1fc634af1..80d0f9c64f9d 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -97,6 +97,9 @@ 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_t(unsigned long long, base_unit, 1,
> +			    base_unit_range - 1);

A nit: one line.

>  	on_time_div = 255ULL * duty_ns;
>  	do_div(on_time_div, period_ns);
> @@ -105,7 +108,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;
>  
> -- 
> 2.26.2
> 

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 04/16] pwm: lpss: Add range limit check for the base_unit register value
  2020-07-09 12:53   ` Andy Shevchenko
@ 2020-07-09 13:23     ` Hans de Goede
  2020-07-09 14:21       ` Andy Shevchenko
  0 siblings, 1 reply; 35+ messages in thread
From: Hans de Goede @ 2020-07-09 13:23 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, intel-gfx, dri-devel,
	Mika Westerberg, linux-acpi

Hi,

On 7/9/20 2:53 PM, Andy Shevchenko wrote:
> On Wed, Jul 08, 2020 at 11:14:20PM +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.
> 
> And I don't see how you can get duty 100% / 0% (I don't remember which one is
> equivalent to 0 in base unit) after this change. IIRC the problem here that
> base unit when non-zero is always being added to the counter and it will
> trigger the change of output at some point which is not what we want for 100% /
> 0% cases.

The base_unit controls the output frequency, not the duty-cycle. So clamping
the base_unit, as calculated from the period here, which also only configures
output-frequency does not impact the duty-cycle at all.

note that AFAICT currently no (in kernel) users actually try to set a period value
which would hit the clamp, so for existing users the clamp is a no-op. I just
added it to this patch-set for correctness sake and because userspace
(sysfs interface) users could in theory set out of range values.

As for the duty-cycle thing, first of all let me say that that is a
question / issue which is completely orthogonal to this patch, this
patch only impacts the period/output frequency NOT the duty-cycle,

With that said, the documentation is not really helpful here,
we need to set the on_time_div to 255 to get a duty-cycle close to 0
(and to 0 to get a duty cycle of 100%) but if setting this to 255 gives
us a duty-cycle of really really 0%, or just close to 0% is uncleaer.

We could do a separate patch add ing a hack where if the user asks for
0% duty-cycle we program the base_unit to 0, but that seems like a bad
idea for 2 reasons:

1. If the user really wants the output to be constantly 0 the user should
just disable the pwm

2. New base_unit values are latched and not applied until the counter
overflows, with a base_unit of 0 the counter never overflows. I have
not tested this but I would not be surprised if after programming a
base_unit value of 0, we are unable to ever change the value again
through any other means then power-cycling the PWM controller.
Even if I could test this on some revisions, we already know that
not all revisions work the same wrt the latching. So it is best to
just never set base_unit to 0, that is just a recipe asking for trouble.

>> 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.
> 
> It would be nice to have an example of calculus here.
> 
>> This commit fixes both issues by clamping the base_unit value to be
>> between 1 and (base_unit_range - 1).
> 
> Eventually I sat and wrote all this on paper. I see now that the problem
> is in out of range of the period. And strongly we should clamp rather period
> to the supported range, but your solution is an equivalent.

Right, the advantage of doing the clamping on the register value is that we
avoid some tricky math with possible rounding errors and which is different
per controller revision because the number of bits in the base unit being
different per controller revision.

> Only question is about the 100% / 0% duty cycle.

See my answer to that above.

>> Fixes: 684309e5043e ("pwm: lpss: Avoid potential overflow of base_unit")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v3:
>> - Change upper limit of clamp to (base_unit_range - 1)
>> - Add Fixes tag
>> ---
>>   drivers/pwm/pwm-lpss.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
>> index 43b1fc634af1..80d0f9c64f9d 100644
>> --- a/drivers/pwm/pwm-lpss.c
>> +++ b/drivers/pwm/pwm-lpss.c
>> @@ -97,6 +97,9 @@ 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_t(unsigned long long, base_unit, 1,
>> +			    base_unit_range - 1);
> 
> A nit: one line.

Doesn't fit in 80 chars, I guess we could make this one line now with the new 100 chars
limit, but that does make it harder to read for people using standard terminal widths
and a terminal based editors. So I would prefer to keep this as is.

Regards,

Hans


> 
>>   	on_time_div = 255ULL * duty_ns;
>>   	do_div(on_time_div, period_ns);
>> @@ -105,7 +108,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;
>>   
>> -- 
>> 2.26.2
>>
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 05/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume
  2020-07-08 21:14 ` [PATCH v4 05/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume Hans de Goede
@ 2020-07-09 13:36   ` Andy Shevchenko
  2020-07-09 13:48     ` Hans de Goede
  0 siblings, 1 reply; 35+ messages in thread
From: Andy Shevchenko @ 2020-07-09 13:36 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, intel-gfx, dri-devel,
	Mika Westerberg, linux-acpi

On Wed, Jul 08, 2020 at 11:14:21PM +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.
> 
> 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 makes the resume handler use pwm_lpss_apply() to
> restore the settings when necessary. This fixes the output-freq and
> duty-cycle being reset to their defaults on resume.

...

> +static int __pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    const struct pwm_state *state, bool from_resume)
>  {
>  	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
>  	int ret;
>  
>  	if (state->enabled) {
>  		if (!pwm_is_enabled(pwm)) {
> -			pm_runtime_get_sync(chip->dev);
> +			if (!from_resume)
> +				pm_runtime_get_sync(chip->dev);
> +
>  			ret = pwm_lpss_is_updating(pwm);
>  			if (ret) {
> -				pm_runtime_put(chip->dev);
> +				if (!from_resume)
> +					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) {
> -				pm_runtime_put(chip->dev);
> +				if (!from_resume)
> +					pm_runtime_put(chip->dev);
> +
>  				return ret;
>  			}
>  			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == true);

>  		}
>  	} else if (pwm_is_enabled(pwm)) {
>  		pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
> -		pm_runtime_put(chip->dev);
> +
> +		if (!from_resume)
> +			pm_runtime_put(chip->dev);
>  	}

I'm wondering if splitting more will make this look better, like:

	...
	if (from_resume) {
		ret = pwm_lpss_prepare_enable(...); // whatever name you think suits better
	} else {
		pm_runtime_get_sync(...);
		ret = pwm_lpss_prepare_enable(...);
		if (ret)
			pm_runtime_put(...);
	}
	...

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 05/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume
  2020-07-09 13:36   ` Andy Shevchenko
@ 2020-07-09 13:48     ` Hans de Goede
  0 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-09 13:48 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, intel-gfx, dri-devel,
	Mika Westerberg, linux-acpi

Hi,

On 7/9/20 3:36 PM, Andy Shevchenko wrote:
> On Wed, Jul 08, 2020 at 11:14:21PM +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.
>>
>> 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 makes the resume handler use pwm_lpss_apply() to
>> restore the settings when necessary. This fixes the output-freq and
>> duty-cycle being reset to their defaults on resume.
> 
> ...
> 
>> +static int __pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			    const struct pwm_state *state, bool from_resume)
>>   {
>>   	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
>>   	int ret;
>>   
>>   	if (state->enabled) {
>>   		if (!pwm_is_enabled(pwm)) {
>> -			pm_runtime_get_sync(chip->dev);
>> +			if (!from_resume)
>> +				pm_runtime_get_sync(chip->dev);
>> +
>>   			ret = pwm_lpss_is_updating(pwm);
>>   			if (ret) {
>> -				pm_runtime_put(chip->dev);
>> +				if (!from_resume)
>> +					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) {
>> -				pm_runtime_put(chip->dev);
>> +				if (!from_resume)
>> +					pm_runtime_put(chip->dev);
>> +
>>   				return ret;
>>   			}
>>   			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == true);
> 
>>   		}
>>   	} else if (pwm_is_enabled(pwm)) {
>>   		pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
>> -		pm_runtime_put(chip->dev);
>> +
>> +		if (!from_resume)
>> +			pm_runtime_put(chip->dev);
>>   	}
> 
> I'm wondering if splitting more will make this look better, like:
> 
> 	...
> 	if (from_resume) {
> 		ret = pwm_lpss_prepare_enable(...); // whatever name you think suits better
> 	} else {
> 		pm_runtime_get_sync(...);
> 		ret = pwm_lpss_prepare_enable(...);
> 		if (ret)
> 			pm_runtime_put(...);
> 	}
> 	...
> 

That is a good idea, I like it. We already had multiple pm_runtime_put() calls
before for the error handlig and this patch did not make it any better.

So adding a pwm_lpss_prepare_enable() helper (the name works for)
will also cleanup the original code. I will add this helper as
a separate preparation patch for this one in v5 of the patch-set.

Regards,

Hans

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API
  2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
                   ` (15 preceding siblings ...)
  2020-07-08 21:14 ` [PATCH v4 16/16] drm/i915: panel: Use atomic PWM API " Hans de Goede
@ 2020-07-09 14:14 ` Sam Ravnborg
  2020-07-09 14:40   ` Hans de Goede
  16 siblings, 1 reply; 35+ messages in thread
From: Sam Ravnborg @ 2020-07-09 14:14 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, linux-acpi, intel-gfx,
	dri-devel, Andy Shevchenko, Mika Westerberg

Hi Hans.

On Wed, Jul 08, 2020 at 11:14:16PM +0200, Hans de Goede wrote:
> Hi All,
> 
> Here is v4 of my patch series converting the i915 driver's code for
> controlling the panel's backlight with an external PWM controller to
> use the atomic PWM API. See below for the changelog.

Why is it that i915 cannot use the pwm_bl driver for backlight?
I have not studied the code - just wondering.

	Sam

> 
> Initially the plan was for this series to consist of 2 parts:
> 1. convert the pwm-crc driver to support the atomic PWM API and
> 2. convert the i915 driver's PWM code to use the atomic PWM API.
> 
> But during testing I've found a number of bugs in the pwm-lpss and I
> found that the acpi_lpss code needs some special handling because of
> some ugliness found in most Cherry Trail DSDTs.
> 
> So now this series has grown somewhat large and consists of 4 parts:
> 
> 1. acpi_lpss fixes workarounds for Cherry Trail DSTD nastiness
> 2. various fixes to the pwm-lpss driver
> 3. convert the pwm-crc driver to support the atomic PWM API and
> 4. convert the i915 driver's PWM code to use the atomic PWM API
> 
> The involved acpi_lpss and pwm drivers do not see a whole lot of churn,
> so the plan is to merge this all through drm-intel-next-queued (dinq)
> once all the patches are reviewed / have acks.
> 
> In v4 the ACPI patches have been Acked by Rafael and the i915 patches
> have been acked by Jani. So that just leaves the PWM patches.
> 
> Uwe can I get your ok / ack for merging this through the dinq branch
> once you have acked al the PWM patches ?
> 
> 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 + CRC-PMIC PWM
> -GPD Pocket  CHT + CRC-PMIC PWM
> 
> Changelog:
> 
> 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
> 
> 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 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
> 
> Regards,
> 
> Hans
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 04/16] pwm: lpss: Add range limit check for the base_unit register value
  2020-07-09 13:23     ` Hans de Goede
@ 2020-07-09 14:21       ` Andy Shevchenko
  2020-07-09 14:33         ` Hans de Goede
  0 siblings, 1 reply; 35+ messages in thread
From: Andy Shevchenko @ 2020-07-09 14:21 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, intel-gfx, dri-devel,
	Mika Westerberg, linux-acpi

On Thu, Jul 09, 2020 at 03:23:13PM +0200, Hans de Goede wrote:
> On 7/9/20 2:53 PM, Andy Shevchenko wrote:
> > On Wed, Jul 08, 2020 at 11:14:20PM +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.
> > 
> > And I don't see how you can get duty 100% / 0% (I don't remember which one is
> > equivalent to 0 in base unit) after this change. IIRC the problem here that
> > base unit when non-zero is always being added to the counter and it will
> > trigger the change of output at some point which is not what we want for 100% /
> > 0% cases.
> 
> The base_unit controls the output frequency, not the duty-cycle. So clamping
> the base_unit, as calculated from the period here, which also only configures
> output-frequency does not impact the duty-cycle at all.
> 
> note that AFAICT currently no (in kernel) users actually try to set a period value
> which would hit the clamp, so for existing users the clamp is a no-op. I just
> added it to this patch-set for correctness sake and because userspace
> (sysfs interface) users could in theory set out of range values.
> 
> As for the duty-cycle thing, first of all let me say that that is a
> question / issue which is completely orthogonal to this patch, this
> patch only impacts the period/output frequency NOT the duty-cycle,

Unfortunately the base unit settings affects duty cycle.

Documentation says about integer part and fractional, where integer is
8 bit and this what's being compared to on time divisor. Thus, if on time
divisor is 255 and base unit is 1 (in integer part) or 0.25, we can't get 0%.
(It looks like if 'on time divisor MOD base unit == 0' we won't get 0%)

> With that said, the documentation is not really helpful here,
> we need to set the on_time_div to 255 to get a duty-cycle close to 0
> (and to 0 to get a duty cycle of 100%) but if setting this to 255 gives
> us a duty-cycle of really really 0%, or just close to 0% is uncleaer.

It depends on base unit value.

> We could do a separate patch add ing a hack where if the user asks for
> 0% duty-cycle we program the base_unit to 0, but that seems like a bad
> idea for 2 reasons:

> 1. If the user really wants the output to be constantly 0 the user should
> just disable the pwm

I can't take this as an argument. Disabling PWM is orthogonal to what duty cycle is.

> 2. New base_unit values are latched and not applied until the counter
> overflows, with a base_unit of 0 the counter never overflows. I have
> not tested this but I would not be surprised if after programming a
> base_unit value of 0, we are unable to ever change the value again
> through any other means then power-cycling the PWM controller.
> Even if I could test this on some revisions, we already know that
> not all revisions work the same wrt the latching. So it is best to
> just never set base_unit to 0, that is just a recipe asking for trouble.

This what doc says about zeros:
• Programming either the PWM_base_unit value or the PWM_on_time_divisor to ‘0’
will generate an always zero output.

So, what I'm talking seems about correlation between base unit and on time
divisor rather than zeros.

I agree with this patch.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> > > 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.
> > 
> > It would be nice to have an example of calculus here.
> > 
> > > This commit fixes both issues by clamping the base_unit value to be
> > > between 1 and (base_unit_range - 1).
> > 
> > Eventually I sat and wrote all this on paper. I see now that the problem
> > is in out of range of the period. And strongly we should clamp rather period
> > to the supported range, but your solution is an equivalent.
> 
> Right, the advantage of doing the clamping on the register value is that we
> avoid some tricky math with possible rounding errors and which is different
> per controller revision because the number of bits in the base unit being
> different per controller revision.

...

> > > +	base_unit = clamp_t(unsigned long long, base_unit, 1,
> > > +			    base_unit_range - 1);
> > 
> > A nit: one line.
> 
> Doesn't fit in 80 chars, I guess we could make this one line now with the new 100 chars
> limit, but that does make it harder to read for people using standard terminal widths
> and a terminal based editors. So I would prefer to keep this as is.

You can use clamp_val().

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 04/16] pwm: lpss: Add range limit check for the base_unit register value
  2020-07-09 14:21       ` Andy Shevchenko
@ 2020-07-09 14:33         ` Hans de Goede
  2020-07-09 14:51           ` Andy Shevchenko
  0 siblings, 1 reply; 35+ messages in thread
From: Hans de Goede @ 2020-07-09 14:33 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, intel-gfx, dri-devel,
	Mika Westerberg, linux-acpi

Hi,

On 7/9/20 4:21 PM, Andy Shevchenko wrote:
> On Thu, Jul 09, 2020 at 03:23:13PM +0200, Hans de Goede wrote:
>> On 7/9/20 2:53 PM, Andy Shevchenko wrote:
>>> On Wed, Jul 08, 2020 at 11:14:20PM +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.
>>>
>>> And I don't see how you can get duty 100% / 0% (I don't remember which one is
>>> equivalent to 0 in base unit) after this change. IIRC the problem here that
>>> base unit when non-zero is always being added to the counter and it will
>>> trigger the change of output at some point which is not what we want for 100% /
>>> 0% cases.
>>
>> The base_unit controls the output frequency, not the duty-cycle. So clamping
>> the base_unit, as calculated from the period here, which also only configures
>> output-frequency does not impact the duty-cycle at all.
>>
>> note that AFAICT currently no (in kernel) users actually try to set a period value
>> which would hit the clamp, so for existing users the clamp is a no-op. I just
>> added it to this patch-set for correctness sake and because userspace
>> (sysfs interface) users could in theory set out of range values.
>>
>> As for the duty-cycle thing, first of all let me say that that is a
>> question / issue which is completely orthogonal to this patch, this
>> patch only impacts the period/output frequency NOT the duty-cycle,
> 
> Unfortunately the base unit settings affects duty cycle.
> 
> Documentation says about integer part and fractional, where integer is
> 8 bit and this what's being compared to on time divisor. Thus, if on time
> divisor is 255 and base unit is 1 (in integer part) or 0.25, we can't get 0%.
> (It looks like if 'on time divisor MOD base unit == 0' we won't get 0%)
> 
>> With that said, the documentation is not really helpful here,
>> we need to set the on_time_div to 255 to get a duty-cycle close to 0
>> (and to 0 to get a duty cycle of 100%) but if setting this to 255 gives
>> us a duty-cycle of really really 0%, or just close to 0% is uncleaer.
> 
> It depends on base unit value.
> 
>> We could do a separate patch add ing a hack where if the user asks for
>> 0% duty-cycle we program the base_unit to 0, but that seems like a bad
>> idea for 2 reasons:
> 
>> 1. If the user really wants the output to be constantly 0 the user should
>> just disable the pwm
> 
> I can't take this as an argument. Disabling PWM is orthogonal to what duty cycle is.
> 
>> 2. New base_unit values are latched and not applied until the counter
>> overflows, with a base_unit of 0 the counter never overflows. I have
>> not tested this but I would not be surprised if after programming a
>> base_unit value of 0, we are unable to ever change the value again
>> through any other means then power-cycling the PWM controller.
>> Even if I could test this on some revisions, we already know that
>> not all revisions work the same wrt the latching. So it is best to
>> just never set base_unit to 0, that is just a recipe asking for trouble.
> 
> This what doc says about zeros:
> • Programming either the PWM_base_unit value or the PWM_on_time_divisor to ‘0’
> will generate an always zero output.
> 
> So, what I'm talking seems about correlation between base unit and on time
> divisor rather than zeros.
> 
> I agree with this patch.
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thank you.

>>>> 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.
>>>
>>> It would be nice to have an example of calculus here.
>>>
>>>> This commit fixes both issues by clamping the base_unit value to be
>>>> between 1 and (base_unit_range - 1).
>>>
>>> Eventually I sat and wrote all this on paper. I see now that the problem
>>> is in out of range of the period. And strongly we should clamp rather period
>>> to the supported range, but your solution is an equivalent.
>>
>> Right, the advantage of doing the clamping on the register value is that we
>> avoid some tricky math with possible rounding errors and which is different
>> per controller revision because the number of bits in the base unit being
>> different per controller revision.
> 
> ...
> 
>>>> +	base_unit = clamp_t(unsigned long long, base_unit, 1,
>>>> +			    base_unit_range - 1);
>>>
>>> A nit: one line.
>>
>> Doesn't fit in 80 chars, I guess we could make this one line now with the new 100 chars
>> limit, but that does make it harder to read for people using standard terminal widths
>> and a terminal based editors. So I would prefer to keep this as is.
> 
> You can use clamp_val().

I did not know about that, that will work nicely I will switch to clamp_val
for the next version. I assume it is ok to keep your Reviewed-by with this
very minor change?

Regards,

Hans

> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API
  2020-07-09 14:14 ` [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Sam Ravnborg
@ 2020-07-09 14:40   ` Hans de Goede
  2020-07-09 15:23     ` Sam Ravnborg
  2020-07-11  6:19     ` Uwe Kleine-König
  0 siblings, 2 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-09 14:40 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, linux-acpi, intel-gfx,
	dri-devel, Andy Shevchenko, Mika Westerberg

Hi,

On 7/9/20 4:14 PM, Sam Ravnborg wrote:
> Hi Hans.
> 
> On Wed, Jul 08, 2020 at 11:14:16PM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> Here is v4 of my patch series converting the i915 driver's code for
>> controlling the panel's backlight with an external PWM controller to
>> use the atomic PWM API. See below for the changelog.
> 
> Why is it that i915 cannot use the pwm_bl driver for backlight?
> I have not studied the code - just wondering.

The intel_panel.c code deals with 7 different types of PWM controllers
which are built into the GPU + support for external PWM controllers
through the kernel's PWM subsystem.

pwm_bl will work for the external PWM controller case, but not for
the others. On top of that the intel_panel code integrates which
the video BIOS, getting things like frequency, minimum value
and if the range is inverted (0% duty == backlight brightness max).
I'm not even sure if pwm_bl supports all of this, but even if it
does the intel_panel code handles this in a unified manner for
all supported PWM controllers, including the ones which are
an integral part of the GPU.

Regards,

Hans



>> Initially the plan was for this series to consist of 2 parts:
>> 1. convert the pwm-crc driver to support the atomic PWM API and
>> 2. convert the i915 driver's PWM code to use the atomic PWM API.
>>
>> But during testing I've found a number of bugs in the pwm-lpss and I
>> found that the acpi_lpss code needs some special handling because of
>> some ugliness found in most Cherry Trail DSDTs.
>>
>> So now this series has grown somewhat large and consists of 4 parts:
>>
>> 1. acpi_lpss fixes workarounds for Cherry Trail DSTD nastiness
>> 2. various fixes to the pwm-lpss driver
>> 3. convert the pwm-crc driver to support the atomic PWM API and
>> 4. convert the i915 driver's PWM code to use the atomic PWM API
>>
>> The involved acpi_lpss and pwm drivers do not see a whole lot of churn,
>> so the plan is to merge this all through drm-intel-next-queued (dinq)
>> once all the patches are reviewed / have acks.
>>
>> In v4 the ACPI patches have been Acked by Rafael and the i915 patches
>> have been acked by Jani. So that just leaves the PWM patches.
>>
>> Uwe can I get your ok / ack for merging this through the dinq branch
>> once you have acked al the PWM patches ?
>>
>> 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 + CRC-PMIC PWM
>> -GPD Pocket  CHT + CRC-PMIC PWM
>>
>> Changelog:
>>
>> 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
>>
>> 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 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
>>
>> Regards,
>>
>> Hans
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0
  2020-07-08 21:14 ` [PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0 Hans de Goede
@ 2020-07-09 14:50   ` Andy Shevchenko
  2020-07-09 15:47     ` Hans de Goede
  0 siblings, 1 reply; 35+ messages in thread
From: Andy Shevchenko @ 2020-07-09 14:50 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, intel-gfx, dri-devel,
	Mika Westerberg, linux-acpi

On Wed, Jul 08, 2020 at 11:14:22PM +0200, Hans de Goede wrote:
> The datasheet specifies that programming the base_unit part of the
> ctrl register to 0 results in a contineous low signal.
> 
> Adjust the get_state method to reflect this by setting pwm_state.period
> to 1 and duty_cycle to 0.

...

> +	if (freq == 0) {
> +		/* In this case the PWM outputs a continous low signal */

> +		state->period = 1;

I guess this should be something like half of the range (so base unit calc
will give 128). Because with period = 1 (too small) it will give too small
base unit (if apply) and as a result we get high frequency pulses.

> +		state->duty_cycle = 0;
> +	} else {
>  		state->period = NSEC_PER_SEC / (unsigned long)freq;
> +		on_time_div *= state->period;
> +		do_div(on_time_div, 255);
> +		state->duty_cycle = on_time_div;
> +	}

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 04/16] pwm: lpss: Add range limit check for the base_unit register value
  2020-07-09 14:33         ` Hans de Goede
@ 2020-07-09 14:51           ` Andy Shevchenko
  0 siblings, 0 replies; 35+ messages in thread
From: Andy Shevchenko @ 2020-07-09 14:51 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, intel-gfx, dri-devel,
	Mika Westerberg, linux-acpi

On Thu, Jul 09, 2020 at 04:33:50PM +0200, Hans de Goede wrote:
> On 7/9/20 4:21 PM, Andy Shevchenko wrote:
> > On Thu, Jul 09, 2020 at 03:23:13PM +0200, Hans de Goede wrote:

...

> > You can use clamp_val().
> 
> I did not know about that, that will work nicely I will switch to clamp_val
> for the next version. I assume it is ok to keep your Reviewed-by with this
> very minor change?

Sure.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API
  2020-07-09 14:40   ` Hans de Goede
@ 2020-07-09 15:23     ` Sam Ravnborg
  2020-07-11  6:19     ` Uwe Kleine-König
  1 sibling, 0 replies; 35+ messages in thread
From: Sam Ravnborg @ 2020-07-09 15:23 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, linux-acpi, intel-gfx,
	dri-devel, Andy Shevchenko, Mika Westerberg

Hi,
On Thu, Jul 09, 2020 at 04:40:56PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/9/20 4:14 PM, Sam Ravnborg wrote:
> > Hi Hans.
> > 
> > On Wed, Jul 08, 2020 at 11:14:16PM +0200, Hans de Goede wrote:
> > > Hi All,
> > > 
> > > Here is v4 of my patch series converting the i915 driver's code for
> > > controlling the panel's backlight with an external PWM controller to
> > > use the atomic PWM API. See below for the changelog.
> > 
> > Why is it that i915 cannot use the pwm_bl driver for backlight?
> > I have not studied the code - just wondering.
> 
> The intel_panel.c code deals with 7 different types of PWM controllers
> which are built into the GPU + support for external PWM controllers
> through the kernel's PWM subsystem.
> 
> pwm_bl will work for the external PWM controller case, but not for
> the others. On top of that the intel_panel code integrates which
> the video BIOS, getting things like frequency, minimum value
> and if the range is inverted (0% duty == backlight brightness max).
> I'm not even sure if pwm_bl supports all of this, but even if it
> does the intel_panel code handles this in a unified manner for
> all supported PWM controllers, including the ones which are
> an integral part of the GPU.

Thanks for the explanation.
This is a more complicated world than the usual embedded case with a
single pwm, no BIOS etc. So it makes sense.

	Sam

> 
> Regards,
> 
> Hans
> 
> 
> 
> > > Initially the plan was for this series to consist of 2 parts:
> > > 1. convert the pwm-crc driver to support the atomic PWM API and
> > > 2. convert the i915 driver's PWM code to use the atomic PWM API.
> > > 
> > > But during testing I've found a number of bugs in the pwm-lpss and I
> > > found that the acpi_lpss code needs some special handling because of
> > > some ugliness found in most Cherry Trail DSDTs.
> > > 
> > > So now this series has grown somewhat large and consists of 4 parts:
> > > 
> > > 1. acpi_lpss fixes workarounds for Cherry Trail DSTD nastiness
> > > 2. various fixes to the pwm-lpss driver
> > > 3. convert the pwm-crc driver to support the atomic PWM API and
> > > 4. convert the i915 driver's PWM code to use the atomic PWM API
> > > 
> > > The involved acpi_lpss and pwm drivers do not see a whole lot of churn,
> > > so the plan is to merge this all through drm-intel-next-queued (dinq)
> > > once all the patches are reviewed / have acks.
> > > 
> > > In v4 the ACPI patches have been Acked by Rafael and the i915 patches
> > > have been acked by Jani. So that just leaves the PWM patches.
> > > 
> > > Uwe can I get your ok / ack for merging this through the dinq branch
> > > once you have acked al the PWM patches ?
> > > 
> > > 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 + CRC-PMIC PWM
> > > -GPD Pocket  CHT + CRC-PMIC PWM
> > > 
> > > Changelog:
> > > 
> > > 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
> > > 
> > > 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 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
> > > 
> > > Regards,
> > > 
> > > Hans
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0
  2020-07-09 14:50   ` Andy Shevchenko
@ 2020-07-09 15:47     ` Hans de Goede
  2020-07-11  6:11       ` Uwe Kleine-König
  0 siblings, 1 reply; 35+ messages in thread
From: Hans de Goede @ 2020-07-09 15:47 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Thierry Reding, Uwe Kleine-König, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, intel-gfx, dri-devel,
	Mika Westerberg, linux-acpi

Hi,

On 7/9/20 4:50 PM, Andy Shevchenko wrote:
> On Wed, Jul 08, 2020 at 11:14:22PM +0200, Hans de Goede wrote:
>> The datasheet specifies that programming the base_unit part of the
>> ctrl register to 0 results in a contineous low signal.
>>
>> Adjust the get_state method to reflect this by setting pwm_state.period
>> to 1 and duty_cycle to 0.
> 
> ...
> 
>> +	if (freq == 0) {
>> +		/* In this case the PWM outputs a continous low signal */
> 
>> +		state->period = 1;
> 
> I guess this should be something like half of the range (so base unit calc
> will give 128). Because with period = 1 (too small) it will give too small
> base unit (if apply) and as a result we get high frequency pulses.

You are right, that if after this the user only changes the duty-cycle
things will work very poorly, we will end up with a base_unit value of
e.g 65535 and then have almost no duty-cycle resolution at all.

How about using a value here which results in a base_unit value of
256 (for 16 bit base-unit registers), that is the highest frequency we
can do while still having full duty-cycle resolution and it also
is the power-on-reset value, so using a higher period which translates
to a base_unit value of 256 (the por calue) seems like a sensible thing to do.

Uwe what do you think about this?

Regards,

Hans



> 
>> +		state->duty_cycle = 0;
>> +	} else {
>>   		state->period = NSEC_PER_SEC / (unsigned long)freq;
>> +		on_time_div *= state->period;
>> +		do_div(on_time_div, 255);
>> +		state->duty_cycle = on_time_div;
>> +	}
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0
  2020-07-09 15:47     ` Hans de Goede
@ 2020-07-11  6:11       ` Uwe Kleine-König
  2020-07-11 13:58         ` Hans de Goede
  0 siblings, 1 reply; 35+ messages in thread
From: Uwe Kleine-König @ 2020-07-11  6:11 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Andy Shevchenko, Thierry Reding, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, intel-gfx, dri-devel,
	Mika Westerberg, linux-acpi

[-- Attachment #1: Type: text/plain, Size: 1439 bytes --]

On Thu, Jul 09, 2020 at 05:47:59PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/9/20 4:50 PM, Andy Shevchenko wrote:
> > On Wed, Jul 08, 2020 at 11:14:22PM +0200, Hans de Goede wrote:
> > > The datasheet specifies that programming the base_unit part of the
> > > ctrl register to 0 results in a contineous low signal.
> > > 
> > > Adjust the get_state method to reflect this by setting pwm_state.period
> > > to 1 and duty_cycle to 0.
> > 
> > ...
> > 
> > > +	if (freq == 0) {
> > > +		/* In this case the PWM outputs a continous low signal */
> > 
> > > +		state->period = 1;
> > 
> > I guess this should be something like half of the range (so base unit calc
> > will give 128). Because with period = 1 (too small) it will give too small
> > base unit (if apply) and as a result we get high frequency pulses.
> 
> You are right, that if after this the user only changes the duty-cycle
> things will work very poorly, we will end up with a base_unit value of
> e.g 65535 and then have almost no duty-cycle resolution at all.

Is this a problem of the consumer that we don't need to solve? Are there
known consumers running into this problem?

pwm_lpss_prepare() is buggy here, a request for a too low period should be
refused.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API
  2020-07-09 14:40   ` Hans de Goede
  2020-07-09 15:23     ` Sam Ravnborg
@ 2020-07-11  6:19     ` Uwe Kleine-König
  2020-07-11 13:46       ` Hans de Goede
  1 sibling, 1 reply; 35+ messages in thread
From: Uwe Kleine-König @ 2020-07-11  6:19 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Sam Ravnborg, Thierry Reding, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, linux-acpi, intel-gfx,
	dri-devel, Andy Shevchenko, Mika Westerberg

[-- Attachment #1: Type: text/plain, Size: 1827 bytes --]

Hi Hans,

On Thu, Jul 09, 2020 at 04:40:56PM +0200, Hans de Goede wrote:
> On 7/9/20 4:14 PM, Sam Ravnborg wrote:
> > On Wed, Jul 08, 2020 at 11:14:16PM +0200, Hans de Goede wrote:
> > > Here is v4 of my patch series converting the i915 driver's code for
> > > controlling the panel's backlight with an external PWM controller to
> > > use the atomic PWM API. See below for the changelog.
> > 
> > Why is it that i915 cannot use the pwm_bl driver for backlight?
> > I have not studied the code - just wondering.
> 
> The intel_panel.c code deals with 7 different types of PWM controllers
> which are built into the GPU + support for external PWM controllers
> through the kernel's PWM subsystem.
> 
> pwm_bl will work for the external PWM controller case, but not for
> the others. On top of that the intel_panel code integrates which
> the video BIOS, getting things like frequency, minimum value
> and if the range is inverted (0% duty == backlight brightness max).
> I'm not even sure if pwm_bl supports all of this, but even if it
> does the intel_panel code handles this in a unified manner for
> all supported PWM controllers, including the ones which are
> an integral part of the GPU.

pwm_bl handles inverted PWM just fine. I'm unsure what "integrates which
the video BIOS" means, but I don't see how "handling 7 different types
of PWM controllers explicitly and others using the PWM API" can be seen
as "unified manner" compared to "provide a pwm driver for whatever might
be in the GPU and then use generic code (PWM API, pwm_bl) to drive it".

Maybe I'm not understanding some involved complexity here?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 16/16] drm/i915: panel: Use atomic PWM API for devs with an external PWM controller
  2020-07-08 21:14 ` [PATCH v4 16/16] drm/i915: panel: Use atomic PWM API " Hans de Goede
@ 2020-07-11  6:32   ` Uwe Kleine-König
  2020-07-11 13:51     ` Hans de Goede
  0 siblings, 1 reply; 35+ messages in thread
From: Uwe Kleine-König @ 2020-07-11  6:32 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Thierry Reding, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, intel-gfx, dri-devel,
	Andy Shevchenko, Mika Westerberg, linux-acpi, Jani Nikula

[-- Attachment #1: Type: text/plain, Size: 8465 bytes --]

On Wed, Jul 08, 2020 at 11:14:32PM +0200, Hans de Goede wrote:
> 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 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    | 71 +++++++++----------
>  2 files changed, 34 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 de32f9efb120..4bd9981e70a1 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 cb28b9908ca4..3d97267c8238 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);

Here you introduce a slight difference: pwm_get_relative_duty_cycle uses
round-closest while you replace a round-up. Is this relevant?

>  }
>  
>  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);

Similar here: The function used to use round-up but
pwm_set_relative_duty_cycle() used round-closest.

>  }
>  
>  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,36 +1915,30 @@ 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_get_period(panel->backlight.pwm)) {

What would pwm_is_enabled(panel->backlight.pwm) == true &&
pwm_get_period(panel->backlight.pwm) == 0 mean? I hope this doesn't
happen?!

> +		/* PWM is already enabled, use existing settings */
> +		pwm_get_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> +
> +		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 / panel->backlight.pwm_state.period,

.period becomes a u64 soon, so be prepared to fixup here.

> +			    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);
>  	}
>  
> -	level = DIV_ROUND_UP(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;
> -
>  	drm_info(&dev_priv->drm, "Using %s PWM for LCD backlight control\n",
>  		 desc);
>  	return 0;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API
  2020-07-11  6:19     ` Uwe Kleine-König
@ 2020-07-11 13:46       ` Hans de Goede
  0 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-11 13:46 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Sam Ravnborg, Thierry Reding, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, linux-acpi, intel-gfx,
	dri-devel, Andy Shevchenko, Mika Westerberg

Hi,

On 7/11/20 8:19 AM, Uwe Kleine-König wrote:
> Hi Hans,
> 
> On Thu, Jul 09, 2020 at 04:40:56PM +0200, Hans de Goede wrote:
>> On 7/9/20 4:14 PM, Sam Ravnborg wrote:
>>> On Wed, Jul 08, 2020 at 11:14:16PM +0200, Hans de Goede wrote:
>>>> Here is v4 of my patch series converting the i915 driver's code for
>>>> controlling the panel's backlight with an external PWM controller to
>>>> use the atomic PWM API. See below for the changelog.
>>>
>>> Why is it that i915 cannot use the pwm_bl driver for backlight?
>>> I have not studied the code - just wondering.
>>
>> The intel_panel.c code deals with 7 different types of PWM controllers
>> which are built into the GPU + support for external PWM controllers
>> through the kernel's PWM subsystem.
>>
>> pwm_bl will work for the external PWM controller case, but not for
>> the others. On top of that the intel_panel code integrates which
>> the video BIOS, getting things like frequency, minimum value
>> and if the range is inverted (0% duty == backlight brightness max).
>> I'm not even sure if pwm_bl supports all of this, but even if it
>> does the intel_panel code handles this in a unified manner for
>> all supported PWM controllers, including the ones which are
>> an integral part of the GPU.
> 
> pwm_bl handles inverted PWM just fine. I'm unsure what "integrates which
> the video BIOS" means,

Integrating with the video BIOS means reading the VBT (Video BIOS Tables)
and extracting info about which PWM controller to use, what frequency
to program the output at, minimum allowed duty-cycle and if the scale
is inverted.

> but I don't see how "handling 7 different types
> of PWM controllers explicitly and others using the PWM API" can be seen
> as "unified manner" compared to "provide a pwm driver for whatever might
> be in the GPU and then use generic code (PWM API, pwm_bl) to drive it".

Part of this is historical, the main x86 GPU drivers have always treated
backlight control as integral part of the display pipeline and in some
cases it really is, e.g. for eDP panels in some cases the backlight
is controlled through the DP aux channel, there is no PWM controller
(visible to the kernel involved). So the intel_panel.c code really
is a backlight-control de-multiplexer, picking the right "plugin"
to control the backlight, which may also be the eDP backlight control
code. Using a PWM controller supported by the PWM-core/class is just
one of the many supported "plugins".

Also the GPU currently is treated as a single device, not as a MFD
device, so we cannot have an isolated PWM driver. We could have code
inside the GPU driver which exports a PWM-controller to the PWM-core,
to then get a reference to the exported PWM-controller but that would
be very roundabout.

The devices which are using an external PWM controller are actually
the exception here, 99.9% of all devices use the GPU integrated PWM
controller.

Anyways changing over the other PWM-like controllers support by
the intel-panel code falls way outside of the scope of this
patch-set.

Regards,

Hans

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 16/16] drm/i915: panel: Use atomic PWM API for devs with an external PWM controller
  2020-07-11  6:32   ` Uwe Kleine-König
@ 2020-07-11 13:51     ` Hans de Goede
  0 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-11 13:51 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Thierry Reding, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, intel-gfx, dri-devel,
	Andy Shevchenko, Mika Westerberg, linux-acpi, Jani Nikula

Hi,

On 7/11/20 8:32 AM, Uwe Kleine-König wrote:
> On Wed, Jul 08, 2020 at 11:14:32PM +0200, Hans de Goede wrote:
>> 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 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    | 71 +++++++++----------
>>   2 files changed, 34 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 de32f9efb120..4bd9981e70a1 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 cb28b9908ca4..3d97267c8238 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);
> 
> Here you introduce a slight difference: pwm_get_relative_duty_cycle uses
> round-closest while you replace a round-up. Is this relevant?

Yes I'm aware of the change in rounding and I do not believe that it is
relevant. One of the advantages of switching to the helpers is not having
to worry about the rounding and letting the helpers figure that out :)

>>   }
>>   
>>   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);
> 
> Similar here: The function used to use round-up but
> pwm_set_relative_duty_cycle() used round-closest.

Idem.


>>   }
>>   
>>   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,36 +1915,30 @@ 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_get_period(panel->backlight.pwm)) {
> 
> What would pwm_is_enabled(panel->backlight.pwm) == true &&
> pwm_get_period(panel->backlight.pwm) == 0 mean? I hope this doesn't
> happen?!

It shouldn't happen this code uses only 2 PWM controller drivers,
pwm-crc and pwm-lpss and the get_state of neither ever sets
period tto 0. This check is just here for extra safety, since getting it
wrong would lead to a divide by 0. Which I see has been fixed by the
helper now (which does its own period==0 check). So I guess I can
(and I will) just drop this extra check for the next version.

>> +		/* PWM is already enabled, use existing settings */
>> +		pwm_get_state(panel->backlight.pwm, &panel->backlight.pwm_state);
>> +
>> +		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 / panel->backlight.pwm_state.period,
> 
> .period becomes a u64 soon, so be prepared to fixup here.
> 
>> +			    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);
>>   	}
>>   
>> -	level = DIV_ROUND_UP(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;
>> -
>>   	drm_info(&dev_priv->drm, "Using %s PWM for LCD backlight control\n",
>>   		 desc);
>>   	return 0;
> 
> Best regards
> Uwe



Regards,

Hans

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0
  2020-07-11  6:11       ` Uwe Kleine-König
@ 2020-07-11 13:58         ` Hans de Goede
  0 siblings, 0 replies; 35+ messages in thread
From: Hans de Goede @ 2020-07-11 13:58 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Andy Shevchenko, Thierry Reding, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Ville Syrjälä,
	Rafael J . Wysocki, Len Brown, linux-pwm, intel-gfx, dri-devel,
	Mika Westerberg, linux-acpi

Hi,

On 7/11/20 8:11 AM, Uwe Kleine-König wrote:
> On Thu, Jul 09, 2020 at 05:47:59PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 7/9/20 4:50 PM, Andy Shevchenko wrote:
>>> On Wed, Jul 08, 2020 at 11:14:22PM +0200, Hans de Goede wrote:
>>>> The datasheet specifies that programming the base_unit part of the
>>>> ctrl register to 0 results in a contineous low signal.
>>>>
>>>> Adjust the get_state method to reflect this by setting pwm_state.period
>>>> to 1 and duty_cycle to 0.
>>>
>>> ...
>>>
>>>> +	if (freq == 0) {
>>>> +		/* In this case the PWM outputs a continous low signal */
>>>
>>>> +		state->period = 1;
>>>
>>> I guess this should be something like half of the range (so base unit calc
>>> will give 128). Because with period = 1 (too small) it will give too small
>>> base unit (if apply) and as a result we get high frequency pulses.
>>
>> You are right, that if after this the user only changes the duty-cycle
>> things will work very poorly, we will end up with a base_unit value of
>> e.g 65535 and then have almost no duty-cycle resolution at all.
> 
> Is this a problem of the consumer that we don't need to solve? Are there
> known consumers running into this problem?

AFAICT we never ever actually see freq == 0 here, this is just a code-path
to avoid a divide by 0 in case we somehow mysteriously do get freq == 0
here.

On boot the PWM controller is either not used and then the default freq =
input-clock / 256, or it is used and programmed to same sane value.

> pwm_lpss_prepare() is buggy here, a request for a too low period should be
> refused.

So instead of clamping as is done in an earlier patch, we should return
-EINVAL ?  Only for too low periods, or also for too high periods ?

I must say this does worry me a bit, the VBT may request 200Hz output
frequency and some revisions of the PWM controller can do 283Hz as
lowest output freq. ATM we just give the i915 code the 283 Hz if it
request 200, that seems more sane then to give it -EINVAL, since -EINVAL
would require the i915 driver to know the exact limits of each PWM
controller and then to clamp the VBT value before passing it to the
PWM driver, that means moving knowledge out of the PWM driver into
the i915 code.

I believe that without first amending the PWM API too allow a consumer
to query the period min/max values, returning -EINVAL is not the right
thing to do here.

Regards,

Hans

^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, other threads:[~2020-07-11 13:58 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
2020-07-08 21:14 ` [PATCH v4 01/16] ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase Hans de Goede
2020-07-08 21:14 ` [PATCH v4 02/16] ACPI / LPSS: Save Cherry Trail PWM ctx registers only once (at activation) Hans de Goede
2020-07-08 21:14 ` [PATCH v4 03/16] pwm: lpss: Fix off by one error in base_unit math in pwm_lpss_prepare() Hans de Goede
2020-07-08 21:14 ` [PATCH v4 04/16] pwm: lpss: Add range limit check for the base_unit register value Hans de Goede
2020-07-09 12:53   ` Andy Shevchenko
2020-07-09 13:23     ` Hans de Goede
2020-07-09 14:21       ` Andy Shevchenko
2020-07-09 14:33         ` Hans de Goede
2020-07-09 14:51           ` Andy Shevchenko
2020-07-08 21:14 ` [PATCH v4 05/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume Hans de Goede
2020-07-09 13:36   ` Andy Shevchenko
2020-07-09 13:48     ` Hans de Goede
2020-07-08 21:14 ` [PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0 Hans de Goede
2020-07-09 14:50   ` Andy Shevchenko
2020-07-09 15:47     ` Hans de Goede
2020-07-11  6:11       ` Uwe Kleine-König
2020-07-11 13:58         ` Hans de Goede
2020-07-08 21:14 ` [PATCH v4 07/16] pwm: crc: Fix period / duty_cycle times being off by a factor of 256 Hans de Goede
2020-07-08 21:14 ` [PATCH v4 08/16] pwm: crc: Fix off-by-one error in the clock-divider calculations Hans de Goede
2020-07-08 21:14 ` [PATCH v4 09/16] pwm: crc: Fix period changes not having any effect Hans de Goede
2020-07-08 21:14 ` [PATCH v4 10/16] pwm: crc: Enable/disable PWM output on enable/disable Hans de Goede
2020-07-08 21:14 ` [PATCH v4 11/16] pwm: crc: Implement apply() method to support the new atomic PWM API Hans de Goede
2020-07-08 21:14 ` [PATCH v4 12/16] pwm: crc: Implement get_state() method Hans de Goede
2020-07-08 21:14 ` [PATCH v4 13/16] drm/i915: panel: Add get_vbt_pwm_freq() helper Hans de Goede
2020-07-08 21:14 ` [PATCH v4 14/16] drm/i915: panel: Honor the VBT PWM frequency for devs with an external PWM controller Hans de Goede
2020-07-08 21:14 ` [PATCH v4 15/16] drm/i915: panel: Honor the VBT PWM min setting " Hans de Goede
2020-07-08 21:14 ` [PATCH v4 16/16] drm/i915: panel: Use atomic PWM API " Hans de Goede
2020-07-11  6:32   ` Uwe Kleine-König
2020-07-11 13:51     ` Hans de Goede
2020-07-09 14:14 ` [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Sam Ravnborg
2020-07-09 14:40   ` Hans de Goede
2020-07-09 15:23     ` Sam Ravnborg
2020-07-11  6:19     ` Uwe Kleine-König
2020-07-11 13:46       ` Hans de Goede

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).