Re: [PATCH] leds: rgb: leds-qcom-lpg: Fix pwm resolution for Hi-Res PWMs

[Abel Vesa <abel.vesa@linaro.org>]

On 25-02-25 01:09:00, Sebastian Reichel wrote:
> Hi,
> 
> 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 * refclk
> > > > prediv_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 * refclk
> > > > pwm_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 * refclk
> > prediv_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 * refclk
> > pwm_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:
> 
> 1. PWM backlight driver requests PWM with 5 MHz period
> 2. leds-qcom-lpg uses 4.2666 MHz period instead due to HW limits
> 3. PWM backlight driver is unaware and requests a duty cycle
>    expecting the period to be 5 MHz, so the duty cycle can
>    exceed 100%

Yes, exactly.

> 
> Then the question is: Why is the PWM backlight driver not aware of
> the reduced period? It runs pwm_get_state(), so leds-qcom-lpg can
> actually report back that it is using 4.2 MHz instead of 5 MHz.

That's a good point. Will try to do that instead.

> 
> I guess that also means the bug could be avoided by requesting a
> period of 4266666 in DT in the first place. Might be an option to
> unblock the T14s upstreaming.

Haven't tried yet. But yes, it should work. Will try soon.

> 
> Greetings,
> 
> -- Sebastian
> 
> > Something like this:
> > 
> > step = period / (1 << resolution)
> > 
> > So: 
> > 
> > 5000000 / ((1 << 8) - 1) = 19607
> > 
> > and then:
> > 
> > pwm_value = duty / step;

As for this, it's all wrong, because if the user will expect an exact
duty cycle, this will not give that. And I think it's obvious why.

So your suggestion of reporting the "actual" period should be the way to
go.

> > 
> > Hope this makes sense.
> > 
> > Will try this out and respin the patch.
> > 
> > > 
> > > I'm asking because the max value is only used for capping, so with
> > > this patch the maximum brightness will be reached at around 80% duty
> > > cycle (i.e. when the wrap over happens without this patch).
> > > 
> > > Locally I'm currently remapping the duty cycle range to the PWM
> > > value range, which means the display brightness increases
> > > step-by-step until reaching 100% "duty cycle":
> > > 
> > > 		val = (duty * 255) / chan->period;
> > > 		chan->pwm_value = min(val, 255);
> > > 
> > > But for the backlight control the absolute numbers do not really
> > > matter and I have zero knowledge about the chip. So it might be
> > > that the controller really can only go up to ~80% duty cycle at
> > > these settings?
> > > 
> > > Greetings,
> > > 
> > > -- Sebastian
> > > 
> > > >  drivers/leds/rgb/leds-qcom-lpg.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> > > > index f3c9ef2bfa572f9ee86c8b8aa37deb8231965490..146cd9b447787bf170310321e939022dfb176e9f 100644
> > > > --- a/drivers/leds/rgb/leds-qcom-lpg.c
> > > > +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> > > > @@ -529,7 +529,7 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty)
> > > >  	unsigned int clk_rate;
> > > >  
> > > >  	if (chan->subtype == LPG_SUBTYPE_HI_RES_PWM) {
> > > > -		max = LPG_RESOLUTION_15BIT - 1;
> > > > +		max = BIT(lpg_pwm_resolution_hi_res[chan->pwm_resolution_sel]) - 1;
> > > >  		clk_rate = lpg_clk_rates_hi_res[chan->clk_sel];
> > > >  	} else {
> > > >  		max = LPG_RESOLUTION_9BIT - 1;
> > > > 
> > > > ---
> > > > base-commit: 50a0c754714aa3ea0b0e62f3765eb666a1579f24
> > > > change-id: 20250220-leds-qcom-lpg-fix-max-pwm-on-hi-res-067e8782a79b
> > > > 
> > > > Best regards,
> > > > -- 
> > > > Abel Vesa <abel.vesa@linaro.org>
> > > > 
> > 
> >