Hello,
I was dragged into the discussion by the patch that Abel Vesa created in reply to this mail, i.e. https://lore.kernel.org/linux-pwm/20250226-pwm-bl-read-back-period-from-hw-v...
On Tue, Feb 25, 2025 at 10:09:57AM +0200, Abel Vesa wrote:
On 25-02-25 01:09:00, Sebastian Reichel wrote:
On Mon, Feb 24, 2025 at 10:24:33PM +0200, Abel Vesa wrote:
On 25-02-21 00:35:08, Sebastian Reichel wrote:
On Thu, Feb 20, 2025 at 12:31:00PM +0200, Abel Vesa wrote:
Currently, for the high resolution PWMs, the resolution, clock, pre-divider and exponent are being selected based on period. Basically, the implementation loops over each one of these and tries to find the closest (higher) period based on the following formula:
period * refclkprediv_exp = log2 ------------------------------------- NSEC_PER_SEC * pre_div * resolution
Since the resolution is power of 2, the actual period resulting is usually higher than what the resolution allows. That's why the duty cycle requested needs to be capped to the maximum value allowed by the resolution (known as PWM size).
Here is an example of how this can happen:
For a requested period of 5000000, the best clock is 19.2MHz, the best prediv is 5, the best exponent is 6 and the best resolution is 256.
Then, the pwm value is determined based on requested period and duty cycle, best prediv, best exponent and best clock, using the following formula:
duty * refclkpwm_value = ---------------------------------------------- NSEC_PER_SEC * prediv * (1 << prediv_exp)
So in this specific scenario:
(5000000 * 19200000) / (1000000000 * 5 * (1 << 64)) = 300
With a resolution of 8 bits, this pwm value obviously goes over.
Therefore, the max pwm value allowed needs to be 255.
If not, the PMIC internal logic will only value that is under the set PWM size, resulting in a wrapped around PWM value.
This has been observed on Lenovo Thinkpad T14s Gen6 (LCD panel version) which uses one of the PMK8550 to control the LCD backlight.
Fix the value of the PWM by capping to a max based on the chosen resolution (PWM size).
Cc: stable@vger.kernel.org # 6.4 Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM") Signed-off-by: Abel Vesa abel.vesa@linaro.org
Note: This fix is blocking backlight support on Lenovo Thinkpad T14s Gen6 (LCD version), for which I have patches ready to send once this patch is agreed on (review) and merged.
Do you know if the pwm duty cycle to pwm value calculation is correct otherwise?
Sorry for the late reply.
No worries, I understand this takes time.
Here is my understanding of the calculation of the pwm value currently implemented.
First, find the best pre_div, refclk, resolution and prediv_exp by looping through all refclk, resolution and prediv possible values, for the following formula:
period * refclkprediv_exp = log2 ------------------------------------- NSEC_PER_SEC * pre_div * (1 << resolution)
So in DT we set the period to 50000000. For this, as I mentioned in the commit message the best refclk is 19.2MHz, the best prediv is 5, the best exponent is 6 and the best resolution is 255.
So if you use these to compute the period following this formula:
NSEC_PER_SEC * prediv * (1 << resolution)best_period = ------------------------------------------- refclk
So in our case:
(1000000000 * 5 * (1 << 8) * (1 << 6)) / 19200000 = 4266666.6666...
So here is where the things go wrong. Bjorn helped me figure this out today (off-list). Basically, the pwm framework will allow values up to 5000000, as specified in the DT, but for then pwm value will go over 255 when computing the actual pwm value by the following formula:
duty * refclkpwm_value = ---------------------------------------------- NSEC_PER_SEC * prediv * (1 << prediv_exp)
So here is how the value 300 is reached (I messed up this next formula in the commit message):
(5000000 * 19200000) / (1000000000 * 5 * (1 << 8)) = 300
But if we were to use the best_period determined:
(4266666 * 19200000) / (1000000000 * 5 * (1 << 8)) = 255
So I guess the process of determining the best parameters is correct. What I think is missing is we need to divide the requested period (5000000) to the resolution (255) and make sure the duty cycle is a multiple of the result.
Let me try to summarize that:
- PWM backlight driver requests PWM with 5 MHz period
- leds-qcom-lpg uses 4.2666 MHz period instead due to HW limits
- PWM backlight driver is unaware and requests a duty cycle expecting the period to be 5 MHz, so the duty cycle can exceed 100%
Can you please enable CONFIG_PWM_DEBUG, enable pwm tracing (
echo 1 > /sys/kernel/debug/tracing/events/pwm/enable
) then reproduce the problem and provide the output of
cat /sys/kernel/debug/tracing/trace
.
I didn't take a deeper dive in this driver combination, but here is a description about what *should* happen:
You're talking about period in MHz, the PWM abstraction uses nanoseconds. So your summary translated to the PWM wording is (to the best of my understanding):
1. PWM backlight driver requests PWM with .period = 200 ns and .duty_cycle = 200 ns.
2. leds-qcom-lpg cannot pick 200 ns exactly and then chooses .period = 1000000000 / 4.26666 MHz = 234.375 ns
3. leds-qcom-lpg then determines setting for requested .duty_cycle based on .period = 200 ns which then ends up with something bogus.
Right?
There is a problem in 2. already: The PWM hardware driver is supposed to pick the highest period (in ns) not bigger than the requested value. So it must not pick 234.375 ns. (Enabling CONFIG_PWM_DEBUG on that is supposed to wail about that.) It should instead pick (say) 187 ns. In the next step the hw driver should pick the highest duty_cycle (again in ns) not exceeding the requested value (and physics). That will be (I guess) also 187 ns in the constructed example. So you should get your requested 100 % relative duty cycle at least.
So the problem about now knowing the resulting PWM waveform is somewhat real. I think if leds-qcom-lpg behaved as expected from a PWM driver, it would be a tad better than your report suggests. I might miss something though.
Best regards Uwe