[PATCH v2 00/15] pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API
Hi All,
Here is v2 dropping a debugging-patch which I accidentally kept for v1 and addressing a minor review remark from Andy for the 2nd patch.
This patch series converts the i915 driver's code for controlling the panel's backlight with an external PWM controller to use the atomic PWM API.
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
So we need to discuss how to merge this (once it passes review). Although the inter-dependencies are only runtime I still think we should make sure that 1-3 are in the drm-intel-next-queued (dinq) tree before merging the i915 changes. Both to make sure that the intel-gfx CI system does not become unhappy and for bisecting reasons.
The involved acpi_lpss and pwm drivers do not see a whole lot of churn, so we could just merge everything through dinq, or we could use immutable branch and merge those into dinq.
So Rafael and Thierry, can I either get your Acked-by for directly merging this into dinq, or can you provide an immutable branch with these 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
Regards,
Hans
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.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/acpi/acpi_lpss.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index 5e2bfbcf526f..67892fc0b822 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -257,6 +257,7 @@ static const struct lpss_device_desc bsw_pwm_dev_desc = { .flags = LPSS_SAVE_CTX | LPSS_NO_D3_DELAY, .prv_offset = 0x800, .setup = bsw_pwm_setup, + .resume_from_noirq = true, };
static const struct lpss_device_desc byt_uart_dev_desc = {
The DSDTs on most Cherry Trail devices have an ugly clutch where the PWM controller gets turned off from the _PS3 method of the graphics-card dev:
Method (_PS3, 0, Serialized) // _PS3: Power State 3 { ... PWMB = PWMC /* _SB_.PCI0.GFX0.PWMC */ PSAT |= 0x03 Local0 = PSAT /* _SB_.PCI0.GFX0.PSAT */ ... }
Where PSAT is the power-status register of the PWM controller.
Since the i915 driver will do a pwm_get on the pwm device as it uses it to control the LCD panel backlight, there is a device-link marking the i915 device as a consumer of the pwm device. So that the PWM controller will always be suspended after the i915 driver suspends (which is the right thing to do). This causes the above GFX0 PS3 AML code to run before acpi_lpss.c calls acpi_lpss_save_ctx().
So on these devices the PWM controller will already be off when acpi_lpss_save_ctx() runs. This causes it to read/save all 1-s (0xffffffff) as ctx register values.
When these bogus values get restored on resume the PWM controller actually keeps working, since most bits are reserved, but this does set bit 3 of the LPSS General purpose register, which for the PWM controller has the following function: "This bit is re-used to support 32kHz slow mode. Default is 19.2MHz as PWM source clock".
This causes the clock of the PWM controller to switch from 19.2MHz to 32KHz, which is a slow-down of a factor 600. Surprisingly enough so far there have been few bug reports about this. This is likely because the i915 driver was hardcoding the PWM frequency to 46 KHz, which divided by 600 would result in a PWM frequency of approx. 78 Hz, which mostly still works fine. There are some bug reports about the LCD backlight flickering after suspend/resume which are likely caused by this issue.
But with the upcoming patch-series to finally switch the i915 drivers code for external PWM controllers to use the atomic API and to honor the PWM frequency specified in the video BIOS (VBT), this becomes a much bigger problem. On most cases the VBT specifies either 200 Hz or 20 KHz as PWM frequency, which with the mentioned issue ends up being either 1/3 Hz, where the backlight actually visible blinks on and off every 3s, or in 33 Hz and horrible flickering of the backlight.
There are a number of possible solutions to this problem:
1. Make acpi_lpss_save_ctx() run before GFX0._PS3 Pro: Clean solution from pov of not medling with save/restore ctx code Con: As mentioned the current ordering is the right thing to do Con: Requires assymmetry in at what suspend/resume phase we do the save vs restore, requiring more suspend/resume ordering hacks in already convoluted acpi_lpss.c suspend/resume code. 2. Do some sort of save once mode for the LPSS ctx Pro: Reasonably clean Con: Needs a new LPSS flag + code changes to handle the flag 3. Detect we have failed to save the ctx registers and do not restore them Pro: Not PWM specific, might help with issues on other LPSS devices too Con: If we can get away with not restoring the ctx why bother with it at all? 4. Do not save the ctx for CHT PWM controllers Pro: Clean, as simple as dropping a flag? Con: Not so simple as dropping a flag, needs a new flag to ensure that we still do lpss_deassert_reset() on device activation. 5. Make the pwm-lpss code fixup the LPSS-context registers Pro: Keeps acpi_lpss.c code clean Con: Moves knowledge of LPSS-context into the pwm-lpss.c code
1 and 5 both do not seem to be a desirable way forward.
3 and 4 seem ok, but they both assume that restoring the LPSS-context registers is not necessary. I have done a couple of test and those do show that restoring the LPSS-context indeed does not seem to be necessary on devices using s2idle suspend (and successfully reaching S0i3). But I have no hardware to test deep / S3 suspend. So I'm not sure that not restoring the context is safe.
That leaves solution 2, which is about as simple / clean as 3 and 4, so this commit fixes the described problem by implementing a new LPSS_SAVE_CTX_ONCE flag and setting that for the CHT PWM controllers.
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;
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.
base_unit values > (base_unit_range / 256), or iow base_unit values using the 8 most significant bits, cause loss of resolution of the duty-cycle. E.g. assuming a base_unit_range of 65536 steps, then a base_unit value of 768 (256 * 3), limits the duty-cycle resolution to 65536 / 768 = 85 steps. Clamp the max base_unit value to base_unit_range / 32 to ensure a duty-cycle resolution of at least 32 steps. This limits the maximum output frequency to 600 KHz / 780 KHz depending on the base clock.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/pwm/pwm-lpss.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index 9d965ffe66d1..cae74ce61654 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -97,6 +97,14 @@ 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 for values > (base_unit_range / 256) + * (values using the 8 most significant bits) the duty-cycle resolution + * degrades. Clamp the maximum value to base_unit_range / 32 which + * leaves a duty-cycle resolution of 32 steps. + */ + base_unit = clamp_t(unsigned long long, base_unit, 1, + base_unit_range / 32);
on_time_div = 255ULL * duty_ns; do_div(on_time_div, period_ns); @@ -105,7 +113,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 << PWM_BASE_UNIT_SHIFT); - base_unit &= base_unit_range; ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT; ctrl |= on_time_div;
On Sun, Jun 07, 2020 at 08:18:28PM +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.
So, and why it's a problem?
base_unit values > (base_unit_range / 256), or iow base_unit values using the 8 most significant bits, cause loss of resolution of the duty-cycle. E.g. assuming a base_unit_range of 65536 steps, then a base_unit value of 768 (256 * 3), limits the duty-cycle resolution to 65536 / 768 = 85 steps. Clamp the max base_unit value to base_unit_range / 32 to ensure a duty-cycle resolution of at least 32 steps. This limits the maximum output frequency to 600 KHz / 780 KHz depending on the base clock.
This part I don't understand. Why we limiting base unit? I seems like a deliberate regression.
Hi,
On 6/8/20 5:50 AM, Andy Shevchenko wrote:
On Sun, Jun 07, 2020 at 08:18:28PM +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.
So, and why it's a problem?
Lets sya the user requests a PWM output frequency of 100Hz on Cherry Trail which has a 19200000 Hz clock this will result in 100 * 65536 / 19200000 = 0.3 -> 0 as base-unit value. So instead of getting 100 Hz the user will now get a pin which is always outputting low.
OTOH if we clamp to 1 as lowest value, the user will get 192000000 / 65536 = 292 Hz as output frequency which is as close to the requested value as we can get while actually still working as a PWM controller.
base_unit values > (base_unit_range / 256), or iow base_unit values using the 8 most significant bits, cause loss of resolution of the duty-cycle. E.g. assuming a base_unit_range of 65536 steps, then a base_unit value of 768 (256 * 3), limits the duty-cycle resolution to 65536 / 768 = 85 steps. Clamp the max base_unit value to base_unit_range / 32 to ensure a duty-cycle resolution of at least 32 steps. This limits the maximum output frequency to 600 KHz / 780 KHz depending on the base clock.
This part I don't understand. Why we limiting base unit? I seems like a deliberate regression.
The way the PWM controller works is that the base-unit gets added to say a 16 bit (on CHT) counter each input clock and then the highest 8 bits of that counter get compared to the value programmed into the ON_TIME_DIV bits.
Lets say we do not clamp and allow any value and lets say the user selects an output frequency of half the input clock, so base-unit value is 32768, then the counter will only have 2 values: 0 and 32768 after that it will wrap around again. So any on time-div value < 128 will result in the output being always high and any value > 128 will result in the output being high/low 50% of the time and a value of 255 will make the output always low.
So in essence we now only have 3 duty cycle levels, which seems like a bad idea to me / not what a pwm controller is supposed to do.
So I decided to put a cut of at having at least 32 steps.
The mean reason I wrote this patch though is to avoid a base-unit value of 0 which really results in a completely non working PWM output. I personally believe clamping on the high side is a good idea too. But if you are against that I can drop that part.
Note that the clamping on the high side will not affect the primary user of the LPSS-pwm driver which is the i915 backlight code, that never asks for such high frequencies. But it could help to avoid an user shooting themselves in the foot when using the PWM on a dev board through the sysfs interface.
Regards,
Hans
On Mon, Jun 08, 2020 at 01:07:12PM +0200, Hans de Goede wrote:
On 6/8/20 5:50 AM, Andy Shevchenko wrote:
On Sun, Jun 07, 2020 at 08:18:28PM +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.
So, and why it's a problem?
Lets sya the user requests a PWM output frequency of 100Hz on Cherry Trail which has a 19200000 Hz clock this will result in 100 * 65536 / 19200000 = 0.3 -> 0 as base-unit value. So instead of getting 100 Hz the user will now get a pin which is always outputting low.
OTOH if we clamp to 1 as lowest value, the user will get 192000000 / 65536 = 292 Hz as output frequency which is as close to the requested value as we can get while actually still working as a PWM controller.
So, we should basically divide and round up, no?
At least for 0 we will get 0.
base_unit values > (base_unit_range / 256), or iow base_unit values using the 8 most significant bits, cause loss of resolution of the duty-cycle. E.g. assuming a base_unit_range of 65536 steps, then a base_unit value of 768 (256 * 3), limits the duty-cycle resolution to 65536 / 768 = 85 steps. Clamp the max base_unit value to base_unit_range / 32 to ensure a duty-cycle resolution of at least 32 steps. This limits the maximum output frequency to 600 KHz / 780 KHz depending on the base clock.
This part I don't understand. Why we limiting base unit? I seems like a deliberate regression.
The way the PWM controller works is that the base-unit gets added to say a 16 bit (on CHT) counter each input clock and then the highest 8 bits of that counter get compared to the value programmed into the ON_TIME_DIV bits.
Lets say we do not clamp and allow any value and lets say the user selects an output frequency of half the input clock, so base-unit value is 32768, then the counter will only have 2 values: 0 and 32768 after that it will wrap around again. So any on time-div value < 128 will result in the output being always high and any value > 128 will result in the output being high/low 50% of the time and a value of 255 will make the output always low.
So in essence we now only have 3 duty cycle levels, which seems like a bad idea to me / not what a pwm controller is supposed to do.
It's exactly what is written in the documentation. I can't buy base unit clamp. Though, I can buy, perhaps, on time divisor granularity, i.e. 1/ 0% - 25%-1 (0%) 2/ 25% - 50% - 75% (50%) 3/ 75%+1 - 100% (100%) And so on till we got a maximum resolution (8 bits).
Hi,
On 6/8/20 2:51 PM, Andy Shevchenko wrote:
On Mon, Jun 08, 2020 at 01:07:12PM +0200, Hans de Goede wrote:
On 6/8/20 5:50 AM, Andy Shevchenko wrote:
On Sun, Jun 07, 2020 at 08:18:28PM +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.
So, and why it's a problem?
Lets sya the user requests a PWM output frequency of 100Hz on Cherry Trail which has a 19200000 Hz clock this will result in 100 * 65536 / 19200000 = 0.3 -> 0 as base-unit value. So instead of getting 100 Hz the user will now get a pin which is always outputting low.
OTOH if we clamp to 1 as lowest value, the user will get 192000000 / 65536 = 292 Hz as output frequency which is as close to the requested value as we can get while actually still working as a PWM controller.
So, we should basically divide and round up, no?
Yes, that will work for the low limit of base_unit but it will make all the other requested period values less accurate.
At least for 0 we will get 0.
We're dealing with frequency here, but the API is dealing with period, so to get 0 HZ the API user would have to request a period of > 1s e.g. request 2s / 0.5 Hz but then the user is still not really requesting 0Hz (that would correspond with a period of infinity which integers cannot represent.
base_unit values > (base_unit_range / 256), or iow base_unit values using the 8 most significant bits, cause loss of resolution of the duty-cycle. E.g. assuming a base_unit_range of 65536 steps, then a base_unit value of 768 (256 * 3), limits the duty-cycle resolution to 65536 / 768 = 85 steps. Clamp the max base_unit value to base_unit_range / 32 to ensure a duty-cycle resolution of at least 32 steps. This limits the maximum output frequency to 600 KHz / 780 KHz depending on the base clock.
This part I don't understand. Why we limiting base unit? I seems like a deliberate regression.
The way the PWM controller works is that the base-unit gets added to say a 16 bit (on CHT) counter each input clock and then the highest 8 bits of that counter get compared to the value programmed into the ON_TIME_DIV bits.
Lets say we do not clamp and allow any value and lets say the user selects an output frequency of half the input clock, so base-unit value is 32768, then the counter will only have 2 values: 0 and 32768 after that it will wrap around again. So any on time-div value < 128 will result in the output being always high and any value > 128 will result in the output being high/low 50% of the time and a value of 255 will make the output always low.
So in essence we now only have 3 duty cycle levels, which seems like a bad idea to me / not what a pwm controller is supposed to do.
It's exactly what is written in the documentation. I can't buy base unit clamp. Though, I can buy, perhaps, on time divisor granularity, i.e. 1/ 0% - 25%-1 (0%) 2/ 25% - 50% - 75% (50%) 3/ 75%+1 - 100% (100%) And so on till we got a maximum resolution (8 bits).
Note that the PWM API does not expose the granularity to the API user, which is why I went with just putting a minimum on it of 32 steps.
Anyways I don't have a strong opinion on this, so I'm fine with not clamping the base-unit to preserve granularity. We should still clamp it to avoid overflow if the user us requesting a really high frequency though!
The old code had:
base_unit &= base_unit_range;
Which means that if the user requests a too high value, then we first overflow base_unit and then truncate it to fit leading to a random frequency.
So if we forget my minimal granularity argument, then at a minimum we need to replace the above line with:
base_unit = clamp_t(unsigned long long, base_unit, 1, base_unit_range - 1);
And since we need the clamp anyways we can then keep the current round-closest behavior.
Regards,
Hans
On Mon, Jun 08, 2020 at 01:07:12PM +0200, Hans de Goede wrote:
Hi,
On 6/8/20 5:50 AM, Andy Shevchenko wrote:
On Sun, Jun 07, 2020 at 08:18:28PM +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.
So, and why it's a problem?
Lets sya the user requests a PWM output frequency of 100Hz on Cherry Trail which has a 19200000 Hz clock this will result in 100 * 65536 / 19200000 = 0.3 -> 0 as base-unit value. So instead of getting 100 Hz the user will now get a pin which is always outputting low.
I didn't follow the complete discussion but note that the general rule is:
round period down to the next possible implementable period round duty_cycle down to the next possible implementable duty_cycle
so if a small enough period (and so a small duty_cycle) is requested it is expected that duty_cycle will be zero.
Best regards Uwe
On Fri, Jun 12, 2020 at 12:12:42AM +0200, Uwe Kleine-König wrote:
On Mon, Jun 08, 2020 at 01:07:12PM +0200, Hans de Goede wrote:
On 6/8/20 5:50 AM, Andy Shevchenko wrote:
On Sun, Jun 07, 2020 at 08:18:28PM +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.
So, and why it's a problem?
Lets sya the user requests a PWM output frequency of 100Hz on Cherry Trail which has a 19200000 Hz clock this will result in 100 * 65536 / 19200000 = 0.3 -> 0 as base-unit value. So instead of getting 100 Hz the user will now get a pin which is always outputting low.
I didn't follow the complete discussion but note that the general rule is:
round period down to the next possible implementable period round duty_cycle down to the next possible implementable duty_cycle
so if a small enough period (and so a small duty_cycle) is requested it is expected that duty_cycle will be zero.
...which brings me an idea that PWM framework should expose API to get a capabilities, like DMA Engine has.
In such capabilities, in particular, caller can get ranges of the correct frequencies of the underneath hardware.
Hello Andy,
On Fri, Jun 12, 2020 at 02:57:32PM +0300, Andy Shevchenko wrote:
On Fri, Jun 12, 2020 at 12:12:42AM +0200, Uwe Kleine-König wrote:
I didn't follow the complete discussion but note that the general rule is:
round period down to the next possible implementable period round duty_cycle down to the next possible implementable duty_cycle
so if a small enough period (and so a small duty_cycle) is requested it is expected that duty_cycle will be zero.
...which brings me an idea that PWM framework should expose API to get a capabilities, like DMA Engine has.
In such capabilities, in particular, caller can get ranges of the correct frequencies of the underneath hardware.
my idea is to introduce a function pwm_round_state() that has a similar semantic to clk_round_rate(). But this is only one of several thoughts I have for the pwm framework. And as there is (AFAIK) no user who would benefit from pwm_round_state() this is further down on my todo list.
Best regards Uwe
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().
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/pwm/pwm-lpss.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index cae74ce61654..a764e062103b 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); @@ -112,7 +112,7 @@ 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); + ctrl &= ~((base_unit_range - 1) << PWM_BASE_UNIT_SHIFT); ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT; ctrl |= on_time_div;
On Sun, Jun 07, 2020 at 08:18:29PM +0200, Hans de Goede wrote:
According to the data-sheet the way the PWM controller works is that each input clock-cycle the base_unit gets added to a N bit counter and that counter overflowing determines the PWM output frequency.
So assuming e.g. a 16 bit counter this means that if base_unit is set to 1, after 65535 input clock-cycles the counter has been increased from 0 to 65535 and it will overflow on the next cycle, so it will overflow after every 65536 clock cycles and thus the calculations done in pwm_lpss_prepare() should use 65536 and not 65535.
This commit fixes this. Note this also aligns the calculations in pwm_lpss_prepare() with those in pwm_lpss_get_state().
This one sounds like a bug which I have noticed on Broxton (but thought as a hardware issue). In any case it has to be tested on various platforms to see how it affects on them.
Hi,
On 6/8/20 5:55 AM, Andy Shevchenko wrote:
On Sun, Jun 07, 2020 at 08:18:29PM +0200, Hans de Goede wrote:
According to the data-sheet the way the PWM controller works is that each input clock-cycle the base_unit gets added to a N bit counter and that counter overflowing determines the PWM output frequency.
So assuming e.g. a 16 bit counter this means that if base_unit is set to 1, after 65535 input clock-cycles the counter has been increased from 0 to 65535 and it will overflow on the next cycle, so it will overflow after every 65536 clock cycles and thus the calculations done in pwm_lpss_prepare() should use 65536 and not 65535.
This commit fixes this. Note this also aligns the calculations in pwm_lpss_prepare() with those in pwm_lpss_get_state().
This one sounds like a bug which I have noticed on Broxton (but thought as a hardware issue). In any case it has to be tested on various platforms to see how it affects on them.
If you like at the datasheet / read my commit description then it becomes obvious that because of the way the PWM controller works that it takes the full 2^(base-unit-bits) for the counter to overflow, not 2^(base-unit-bits) - 1. This will make a difference of a factor 65535/65536 in the output frequency which will be tricky to measure.
IOW I'm not sure we can really test if this helps, but it is obviously the right thing to do and it aligns the pwm_apply code with the pwm_get_state code which already does not have the - 1.
Regards,
Hans
On Mon, Jun 08, 2020 at 01:13:01PM +0200, Hans de Goede wrote:
On 6/8/20 5:55 AM, Andy Shevchenko wrote:
On Sun, Jun 07, 2020 at 08:18:29PM +0200, Hans de Goede wrote:
According to the data-sheet the way the PWM controller works is that each input clock-cycle the base_unit gets added to a N bit counter and that counter overflowing determines the PWM output frequency.
So assuming e.g. a 16 bit counter this means that if base_unit is set to 1, after 65535 input clock-cycles the counter has been increased from 0 to 65535 and it will overflow on the next cycle, so it will overflow after every 65536 clock cycles and thus the calculations done in pwm_lpss_prepare() should use 65536 and not 65535.
This commit fixes this. Note this also aligns the calculations in pwm_lpss_prepare() with those in pwm_lpss_get_state().
This one sounds like a bug which I have noticed on Broxton (but thought as a hardware issue). In any case it has to be tested on various platforms to see how it affects on them.
If you like at the datasheet / read my commit description then it becomes obvious that because of the way the PWM controller works that it takes the full 2^(base-unit-bits) for the counter to overflow, not 2^(base-unit-bits) - 1. This will make a difference of a factor 65535/65536 in the output frequency which will be tricky to measure.
IOW I'm not sure we can really test if this helps, but it is obviously the right thing to do and it aligns the pwm_apply code with the pwm_get_state code which already does not have the - 1.
Yes. It seems I did a mistake in the commit 684309e5043e ("pwm: lpss: Avoid potential overflow of base_unit") when missed multiplication.
For this one
Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
On the LPSS PWM controller found on Bay Trail (BYT) and Cherry Trail (CHT) platforms, the following sequence results in an output duty-cycle of 100% independent of what the duty-cycle requested in the ctrl-reg is:
1. Clear ENABLE bit in ctrl register 2. Let the machine reach a S0i3 low power state 3. Set the ENABLE bit in the ctrl register
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. Note on BYT and CHT the ENABLE bit must be set before waiting for the SW_UPDATE bit to clear, otherwise the SW_UPDATE bit will never clear (this is indicated in the pwm-lpss.c code by lpwm->info->bypass being false).
My theory about why this is happening is that when we hit S0i3 the part which holds the latched values gets turned off and when its turned back on again at least the on-time-div value has been lost and gets reset to 0 which corresponds to an output duty-cycle of 100%. Testing has shown that setting the SW_UPDATE bit to request latching the ctrl-register values into the actual registers (again) fixes this, confirming this theory.
In the past there have been issues where setting the SW_UPDATE bit when nothing has changed would lead to the next ctrl register changing being ignored, see commit 2153bbc12f77 ("pwm: lpss: Only set update bit if we are actually changing the settings"), so we should only set the SW_UPDATE bit when actually changing the ENABLE bit from 0 to 1.
When looking into how to fix this I noticed that on platforms where lpwm->info->bypass == false we unnecessarily do 2 read-modify-write cycles of the ctrl register, one to set the base-unit and on-time-div, immediately followed by another to set the ENABLE bit.
This commit fixes the 100% duty cycle issue by folding the setting of the ENABLE bit into pwm_lpss_prepare(), which already checks if any bits in the ctrl-register have actually changed and if that is the case then sets the SW_UPDATE bit.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/pwm/pwm-lpss.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index a764e062103b..2cb0e2a9c08c 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -80,7 +80,7 @@ static inline int pwm_lpss_is_updating(struct pwm_device *pwm) }
static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm, - int duty_ns, int period_ns) + int duty_ns, int period_ns, bool enable) { unsigned long long on_time_div; unsigned long c = lpwm->info->clk_rate, base_unit_range; @@ -115,6 +115,8 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm, ctrl &= ~((base_unit_range - 1) << PWM_BASE_UNIT_SHIFT); ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT; ctrl |= on_time_div; + if (enable) + ctrl |= PWM_ENABLE;
if (orig_ctrl != ctrl) { pwm_lpss_write(pwm, ctrl); @@ -142,8 +144,9 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, 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); + pwm_lpss_prepare(lpwm, pwm, + state->duty_cycle, state->period, + lpwm->info->bypass == false); ret = pwm_lpss_wait_for_update(pwm); if (ret) { pm_runtime_put(chip->dev); @@ -154,7 +157,8 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, ret = pwm_lpss_is_updating(pwm); if (ret) return ret; - pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period); + pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, + state->period, false); return pwm_lpss_wait_for_update(pwm); } } else if (pwm_is_enabled(pwm)) {
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 --- drivers/pwm/pwm-crc.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index 272eeb071147..43fc912c1fe9 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -21,8 +21,10 @@
#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 */ + +#define NSEC_PER_MHZ 1000
/** * struct crystalcove_pwm - Crystal Cove PWM controller @@ -72,7 +74,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_MHZ);
regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, clk_div | PWM_OUTPUT_ENABLE);
On Sun, Jun 07, 2020 at 08:18:31PM +0200, Hans de Goede wrote:
While looking into adding atomic-pwm support to the pwm-crc driver I noticed something odd, there is a PWM_BASE_CLK define of 6 MHz and there is a clock-divider which divides this with a value between 1-128, and there are 256 duty-cycle steps.
The pwm-crc code before this commit assumed that a clock-divider setting of 1 means that the PWM output is running at 6 MHZ, if that is true, where do these 256 duty-cycle steps come from?
This would require an internal frequency of 256 * 6 MHz = 1.5 GHz, that seems unlikely for a PMIC which is using a silicon process optimized for power-switching transistors. It is way more likely that there is an 8 bit counter for the duty cycle which acts as an extra fixed divider wrt the PWM output frequency.
The main user of the pwm-crc driver is the i915 GPU driver which uses it for backlight control. Lets compare the PWM register values set by the video-BIOS (the GOP), assuming the extra fixed divider is present versus the PWM frequency specified in the Video-BIOS-Tables:
Device: PWM Hz set by BIOS PWM Hz specified in VBT Asus T100TA 200 200 Asus T100HA 200 200 Lenovo Miix 2 8 23437 20000 Toshiba WT8-A 23437 20000
So as we can see if we assume the extra division by 256 then the register values set by the GOP are an exact match for the VBT values, where as otherwise the values would be of by a factor of 256.
This commit fixes the period / duty_cycle calculations to take the extra division by 256 into account.
...
+#define NSEC_PER_MHZ 1000
This is against physics. What this cryptic name means actually? Existing NSEC_PER_USEC ?
Hi,
On 6/9/20 1:29 PM, Andy Shevchenko wrote:
On Sun, Jun 07, 2020 at 08:18:31PM +0200, Hans de Goede wrote:
While looking into adding atomic-pwm support to the pwm-crc driver I noticed something odd, there is a PWM_BASE_CLK define of 6 MHz and there is a clock-divider which divides this with a value between 1-128, and there are 256 duty-cycle steps.
The pwm-crc code before this commit assumed that a clock-divider setting of 1 means that the PWM output is running at 6 MHZ, if that is true, where do these 256 duty-cycle steps come from?
This would require an internal frequency of 256 * 6 MHz = 1.5 GHz, that seems unlikely for a PMIC which is using a silicon process optimized for power-switching transistors. It is way more likely that there is an 8 bit counter for the duty cycle which acts as an extra fixed divider wrt the PWM output frequency.
The main user of the pwm-crc driver is the i915 GPU driver which uses it for backlight control. Lets compare the PWM register values set by the video-BIOS (the GOP), assuming the extra fixed divider is present versus the PWM frequency specified in the Video-BIOS-Tables:
Device: PWM Hz set by BIOS PWM Hz specified in VBT Asus T100TA 200 200 Asus T100HA 200 200 Lenovo Miix 2 8 23437 20000 Toshiba WT8-A 23437 20000
So as we can see if we assume the extra division by 256 then the register values set by the GOP are an exact match for the VBT values, where as otherwise the values would be of by a factor of 256.
This commit fixes the period / duty_cycle calculations to take the extra division by 256 into account.
...
+#define NSEC_PER_MHZ 1000
This is against physics. What this cryptic name means actually? Existing NSEC_PER_USEC ?
Yes, using existing NSEC_PER_USEC is better I will use that for the next version.
Regards,
Hans
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 --- drivers/pwm/pwm-crc.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index 43fc912c1fe9..5ba2a65c524c 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 */
#define NSEC_PER_MHZ 1000
@@ -75,6 +75,9 @@ 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_MHZ * period_ns / (256 * NSEC_PER_MHZ); + /* clk_div 1 - 128, maps to register values 0-127 */ + if (clk_div > 0) + clk_div--;
regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, clk_div | PWM_OUTPUT_ENABLE);
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 | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index 5ba2a65c524c..ef49a6e3c4d6 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -72,8 +72,9 @@ static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm, if (pwm_get_period(pwm) != period_ns) { int clk_div;
- /* 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); + clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_MHZ); /* clk_div 1 - 128, maps to register values 0-127 */ if (clk_div > 0) @@ -81,9 +82,6 @@ static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm,
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 */
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.
This should disable the internal (divided) PWM clock and tri-state the PWM output pin when disabled, saving some power.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/pwm/pwm-crc.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index ef49a6e3c4d6..53734bcf67e1 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -41,10 +41,24 @@ 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_MHZ); + /* 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); + 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; @@ -53,8 +67,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, @@ -70,16 +86,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, clear PWM_OUTPUT_ENABLE first */ regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, 0); - - clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_MHZ); - /* clk_div 1 - 128, maps to register values 0-127 */ - if (clk_div > 0) - clk_div--; - regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, clk_div | PWM_OUTPUT_ENABLE); }
On Sun, Jun 07, 2020 at 08:18:34PM +0200, Hans de Goede wrote:
The pwm-crc code is using 2 different enable bits:
- bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE)
- bit 0 of the BACKLIGHT_EN register
So far we've kept the PWM_OUTPUT_ENABLE bit set when disabling the PWM, this commit makes crc_pwm_disable() clear it on disable and makes crc_pwm_enable() set it again on re-enable.
This should disable the internal (divided) PWM clock and tri-state the PWM output pin when disabled, saving some power.
...
+static int crc_pwm_calc_clk_div(int period_ns) +{
- int clk_div;
- clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_MHZ);
- /* clk_div 1 - 128, maps to register values 0-127 */
- if (clk_div > 0)
clk_div--;- return clk_div;
+}
You can reduce ping-pong format of the series if you introduced this helper in the patch that adds -1 to clock divisor.
On Sun, Jun 07, 2020 at 08:18:34PM +0200, Hans de Goede wrote:
The pwm-crc code is using 2 different enable bits:
- bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE)
- bit 0 of the BACKLIGHT_EN register
So far we've kept the PWM_OUTPUT_ENABLE bit set when disabling the PWM, this commit makes crc_pwm_disable() clear it on disable and makes crc_pwm_enable() set it again on re-enable.
This should disable the internal (divided) PWM clock and tri-state the PWM output pin when disabled, saving some power.
It would be great if you could also document that disabling the PWM makes the output tri-state. There are a few drivers that have a "Limitations" section at their top. Describing that there (in the same format) would be the right place.
Also note that according to Thierry's conception getting a (driven) inactive output is the right thing for a disabled PWM.
Best regards Uwe
Hi,
On 6/12/20 12:20 AM, Uwe Kleine-König wrote:
On Sun, Jun 07, 2020 at 08:18:34PM +0200, Hans de Goede wrote:
The pwm-crc code is using 2 different enable bits:
- bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE)
- bit 0 of the BACKLIGHT_EN register
So far we've kept the PWM_OUTPUT_ENABLE bit set when disabling the PWM, this commit makes crc_pwm_disable() clear it on disable and makes crc_pwm_enable() set it again on re-enable.
This should disable the internal (divided) PWM clock and tri-state the PWM output pin when disabled, saving some power.
It would be great if you could also document that disabling the PWM makes the output tri-state. There are a few drivers that have a "Limitations" section at their top. Describing that there (in the same format) would be the right place.
Also note that according to Thierry's conception getting a (driven) inactive output is the right thing for a disabled PWM.
Hmm, the tri-state thing is an assumption from my side and we don't have any docs for this PWM controller, so I'm not sure at all if that is true. So I think it will be better to just drop the tri-state bit from the commit msg for the next version.
Regards,
Hans
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 --- drivers/pwm/pwm-crc.c | 107 +++++++++++++++++++++++------------------- 1 file changed, 59 insertions(+), 48 deletions(-)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index 53734bcf67e1..58c7e9ef7278 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -41,70 +41,81 @@ 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) +static int crc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) { - int clk_div; - - clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_MHZ); - /* 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); - 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); + int err, clk_div, level, pwm_output_enable; struct device *dev = crc_pwm->chip.dev; - int level;
- 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 -ENOTSUPP; + + 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) { + 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); - regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, - clk_div | PWM_OUTPUT_ENABLE); + 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; + } }
- /* change the pwm duty cycle */ - level = duty_ns * PWM_MAX_LEVEL / period_ns; - regmap_write(crc_pwm->regmap, PWM0_DUTY_CYCLE, level); + if (pwm_get_period(pwm) != state->period || + pwm_is_enabled(pwm) != state->enabled) { + clk_div = PWM_BASE_CLK_MHZ * state->period / + (256 * NSEC_PER_MHZ); + /* clk_div 1 - 128, maps to register values 0-127 */ + if (clk_div > 0) + clk_div--; + + 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; + } + } + + if (!pwm_is_enabled(pwm) && state->enabled) { + err = regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 1); + if (err) { + dev_err(dev, "Error writing BACKLIGHT_EN %d\n", err); + return err; + } + }
return 0; }
static const struct pwm_ops crc_pwm_ops = { - .config = crc_pwm_config, - .enable = crc_pwm_enable, - .disable = crc_pwm_disable, + .apply = crc_pwm_apply, };
static int crystalcove_pwm_probe(struct platform_device *pdev)
On Sun, Jun 07, 2020 at 08:18:35PM +0200, Hans de Goede wrote:
Replace the enable, disable and config pwm_ops with an apply op, to support the new atomic PWM API.
...
-static int crc_pwm_calc_clk_div(int period_ns) +static int crc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
const struct pwm_state *state){
- int clk_div;
- clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_MHZ);
- /* clk_div 1 - 128, maps to register values 0-127 */
- if (clk_div > 0)
clk_div--;- return clk_div;
-}
...
clk_div = PWM_BASE_CLK_MHZ * state->period /(256 * NSEC_PER_MHZ);/* clk_div 1 - 128, maps to register values 0-127 */if (clk_div > 0)clk_div--;
And again... :-(
Hi,
On 6/9/20 1:32 PM, Andy Shevchenko wrote:
On Sun, Jun 07, 2020 at 08:18:35PM +0200, Hans de Goede wrote:
Replace the enable, disable and config pwm_ops with an apply op, to support the new atomic PWM API.
...
-static int crc_pwm_calc_clk_div(int period_ns) +static int crc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
const struct pwm_state *state)
- int clk_div;
- clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_MHZ);
- /* clk_div 1 - 128, maps to register values 0-127 */
- if (clk_div > 0)
clk_div--;- return clk_div;
-}
...
clk_div = PWM_BASE_CLK_MHZ * state->period /(256 * NSEC_PER_MHZ);/* clk_div 1 - 128, maps to register values 0-127 */if (clk_div > 0)clk_div--;And again... :-(
Well yes I cannot help it that the original code, as submitted by Intel, was of very questionable quality, so instead of just converting it to the atomic PWM API I had to do a ton of bugfixes first... I tried to do this all in small bits rather then in a single big rewrite the buggy <beep> commit to make life easier for reviewers.
I can introduce the crc_pwm_calc_clk_div helper earlier as you suggested in an earlier mail. I guess I could also keep the helper here, and then fold it into the function in a later commit (*).
Would that work for you ?
Regards,
Hans
*) Because having a helper for 3 lines of code when it is used only once is not helpful IMHO, it only makes it harder to figure out what the code is exactly doing when readin the code.
On Tue, Jun 09, 2020 at 03:44:18PM +0200, Hans de Goede wrote:
On 6/9/20 1:32 PM, Andy Shevchenko wrote:
On Sun, Jun 07, 2020 at 08:18:35PM +0200, Hans de Goede wrote:
...
And again... :-(
Well yes I cannot help it that the original code, as submitted by Intel, was of very questionable quality, so instead of just converting it to the atomic PWM API I had to do a ton of bugfixes first... I tried to do this all in small bits rather then in a single big rewrite the buggy <beep> commit to make life easier for reviewers.
Yes, I know about that old code quality, sorry, we were not at Intel that time (or were just right-less newbies).
I can introduce the crc_pwm_calc_clk_div helper earlier as you suggested in an earlier mail. I guess I could also keep the helper here, and then fold it into the function in a later commit (*).
Would that work for you ?
Definitely.
*) Because having a helper for 3 lines of code when it is used only once is not helpful IMHO, it only makes it harder to figure out what the code is exactly doing when readin the code.
At least it will reduce churn to just
1) introduce foo(); 2) do many changes with foo() being used; 3) drop foo() *if* it's not needed / makes little sense.
Implement the pwm_ops.get_state() method to complete the support for the new atomic PWM API.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/pwm/pwm-crc.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index 58c7e9ef7278..6c75a3470bc8 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -114,8 +114,37 @@ 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 = clk_div * NSEC_PER_MHZ * 256 / PWM_BASE_CLK_MHZ; + state->duty_cycle = duty_cycle_reg * state->period / PWM_MAX_LEVEL; + state->polarity = PWM_POLARITY_NORMAL; + state->enabled = !!(clk_div_reg & PWM_OUTPUT_ENABLE); +} + static const struct pwm_ops crc_pwm_ops = { .apply = crc_pwm_apply, + .get_state = crc_pwm_get_state, };
static int crystalcove_pwm_probe(struct platform_device *pdev)
On Sun, Jun 07, 2020 at 08:18:36PM +0200, Hans de Goede wrote:
Implement the pwm_ops.get_state() method to complete the support for the new atomic PWM API.
This one is good.
Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/pwm/pwm-crc.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index 58c7e9ef7278..6c75a3470bc8 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -114,8 +114,37 @@ 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 = clk_div * NSEC_PER_MHZ * 256 / PWM_BASE_CLK_MHZ;
- state->duty_cycle = 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
Hello,
On Sun, Jun 07, 2020 at 08:18:36PM +0200, Hans de Goede wrote:
Implement the pwm_ops.get_state() method to complete the support for the new atomic PWM API.
Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/pwm/pwm-crc.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index 58c7e9ef7278..6c75a3470bc8 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -114,8 +114,37 @@ 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;- }
I assume that duty_cycle_reg cannot be bigger than 0xff? Would it make sense to mask the value accordingly to get more robust code?
- clk_div = (clk_div_reg & ~PWM_OUTPUT_ENABLE) + 1;
- state->period = clk_div * NSEC_PER_MHZ * 256 / PWM_BASE_CLK_MHZ;
- state->duty_cycle = duty_cycle_reg * state->period / PWM_MAX_LEVEL;
- state->polarity = PWM_POLARITY_NORMAL;
- state->enabled = !!(clk_div_reg & PWM_OUTPUT_ENABLE);
These aligned = look strange (IMHO). If you don't feel strong here I'd like to see a single space before a =.
Unrelated to your series I think we should change .get_state() to return an error indication.
Best regards Uwe
Hi,
On 6/11/20 11:37 PM, Uwe Kleine-König wrote:
Hello,
On Sun, Jun 07, 2020 at 08:18:36PM +0200, Hans de Goede wrote:
Implement the pwm_ops.get_state() method to complete the support for the new atomic PWM API.
Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/pwm/pwm-crc.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index 58c7e9ef7278..6c75a3470bc8 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -114,8 +114,37 @@ 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;- }
I assume that duty_cycle_reg cannot be bigger than 0xff? Would it make sense to mask the value accordingly to get more robust code?
- clk_div = (clk_div_reg & ~PWM_OUTPUT_ENABLE) + 1;
- state->period = clk_div * NSEC_PER_MHZ * 256 / PWM_BASE_CLK_MHZ;
- state->duty_cycle = duty_cycle_reg * state->period / PWM_MAX_LEVEL;
- state->polarity = PWM_POLARITY_NORMAL;
- state->enabled = !!(clk_div_reg & PWM_OUTPUT_ENABLE);
These aligned = look strange (IMHO). If you don't feel strong here I'd like to see a single space before a =.
Ok, will change for the next version.
Regards,
Hans
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()).
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,
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.
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 b24266c624fa..24ea4a7b6dde 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;
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.
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",
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.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- .../drm/i915/display/intel_display_types.h | 3 +- drivers/gpu/drm/i915/display/intel_panel.c | 73 +++++++++---------- 2 files changed, 37 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 24ea4a7b6dde..48afb2925271 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..a0f76343f381 100644 --- a/drivers/gpu/drm/i915/display/intel_panel.c +++ b/drivers/gpu/drm/i915/display/intel_panel.c @@ -592,10 +592,11 @@ 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; + int duty_ns, period_ns;
duty_ns = pwm_get_duty_cycle(panel->backlight.pwm); - return DIV_ROUND_UP(duty_ns * 100, panel->backlight.pwm_period_ns); + period_ns = pwm_get_period(panel->backlight.pwm); + return DIV_ROUND_UP(duty_ns * 100, period_ns); }
static void lpt_set_backlight(const struct drm_connector_state *conn_state, u32 level) @@ -669,10 +670,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 * panel->backlight.pwm_period_ns, 100);
- pwm_config(panel->backlight.pwm, duty_ns, - panel->backlight.pwm_period_ns); + panel->backlight.pwm_state.duty_cycle = + DIV_ROUND_UP(level * panel->backlight.pwm_state.period, 100); + pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state); }
static void @@ -841,10 +842,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 +1175,14 @@ 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); + + panel->backlight.pwm_state.duty_cycle = + DIV_ROUND_UP(level * panel->backlight.pwm_state.period, 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 +1901,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 +1919,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 = DIV_ROUND_UP(panel->backlight.pwm_state.duty_cycle * + 100, panel->backlight.pwm_state.period); + 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 setting 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;
dri-devel@lists.freedesktop.org
-
Andy Shevchenko -
Hans de Goede -
Uwe Kleine-König