Hi,
25.04.2022 16:22, Uwe Kleine-König пишет:
> Dividing by the result of a division looses precision because the result is
> rounded twice. E.g. with clk_rate = 48000000 and period = 32760033 the
> following numbers result:
>
> rate = pc->clk_rate >> PWM_DUTY_WIDTH = 187500
> hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns) = 3052
> rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz) = 6144
>
> The exact result would be 6142.5061875 and (apart from rounding) this is
> found by using a single division. As a side effect is also a tad
> cheaper to calculate.
>
> Also using clk_rate >> PWM_DUTY_WIDTH looses precision. Consider for
> example clk_rate = 47999999 and period = 106667:
>
> mul_u64_u64_div_u64(pc->clk_rate >> PWM_DUTY_WIDTH, period_ns,
> NSEC_PER_SEC) = 19
>
> mul_u64_u64_div_u64(pc->clk_rate, period_ns,
> NSEC_PER_SEC << PWM_DUTY_WIDTH) = 20
>
> (The exact result is 20.000062083332033.)
>
> With this optimizations also switch from round-closest to round-down for
> the period calculation. Given that the calculations were non-optimal for
> quite some time now with variations in both directions which nobody
> reported as a problem, this is the opportunity to align the driver's
> behavior to the requirements of new drivers. This has several upsides:
>
> - Implementation is easier as there are no round-nearest variants of
> mul_u64_u64_div_u64().
> - Requests for too small periods are now consistently refused. This was
> kind of arbitrary before, where period_ns < min_period_ns was
> refused, but in some cases min_period_ns isn't actually implementable
> and then values between min_period_ns and the actual minimum were
> rounded up to the actual minimum.
>
> Note that the duty_cycle calculation isn't using the usual round-down
> approach yet.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> ---
> Hello,
>
> changes since (implicit) v1: Updated changelog to explain why rate = 0
> is refused now.
>
> Best regards
> Uwe
>
> drivers/pwm/pwm-tegra.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index e5a9ffef4a71..7fc03a9ec154 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -99,7 +99,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> int duty_ns, int period_ns)
> {
> struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> - unsigned long long c = duty_ns, hz;
> + unsigned long long c = duty_ns;
> unsigned long rate, required_clk_rate;
> u32 val = 0;
> int err;
> @@ -156,11 +156,9 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> pc->clk_rate = clk_get_rate(pc->clk);
> }
>
> - rate = pc->clk_rate >> PWM_DUTY_WIDTH;
> -
> /* Consider precision in PWM_SCALE_WIDTH rate calculation */
> - hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
> - rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
> + rate = mul_u64_u64_div_u64(pc->clk_rate, period_ns,
> + (u64)NSEC_PER_SEC << PWM_DUTY_WIDTH);
>
> /*
> * Since the actual PWM divider is the register's frequency divider
> @@ -169,6 +167,8 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> */
> if (rate > 0)
> rate--;
> + else
> + return -EINVAL;
This patch broke backlight on Asus Transformer tablets, they are now
getting this -EINVAL. The root of the problem is under investigation.
Should we revert this patch meantime or maybe you (Uwe/Thierry) have an
idea about what actually has gone wrong here? Thanks in advance.
> /*
> * Make sure that the rate will fit in the register's frequency
>
> base-commit: 2bf8ee0faa988b5cec3503ebf2f970a0e84d24ee
