Re: [PATCH] pwm: tegra: fix doubled output frequency due to divider truncation

["Uwe Kleine-König" <ukleinek@kernel.org>]

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

Hello Ola,

On Mon, Jul 13, 2026 at 01:15:41PM +0200, Ola Chr. Vaage wrote:
> From: "Ola Chr. Vaage" <ola.christoffer.vage@scoutdi.com>
> 
> The PWM_SCALE frequency divider is computed by rounding down
> 
> 	clk_rate * period_ns / (NSEC_PER_SEC << PWM_DUTY_WIDTH)
> 
> With dynamic clock scaling (Tegra186 and later), the driver doubles its
> clock-rate request when the provider cannot meet it. The provider then
> typically grants slightly less than the doubled request, the exact
> divider lands just below 2, and the round-down truncates it to 1: the
> output runs at double the requested frequency. On Tegra234 the BPMP
> grants PWM clock rates as 408 MHz / N, so nearly every requested period
> is affected: requesting 40000 ns produces 20078 ns, 100000 ns produces
> 50195 ns. The duty ratio is computed independently and stays correct,
> which hides the problem from duty-only consumers such as backlights;
> frequency-sensitive loads break.
> 
> Measured on a Jetson Orin NX board with the 45334 ns fan period used in
> NVIDIA device trees:
> 
> 	required_clk_rate = ceil((1e9 << 8) / 45334) = 5646977
> 	clk_round_rate() = 5589041 (408 MHz / 73) -> request doubled
> 	dev_pm_opp_set_rate(11293954) grants 11027028 (408 MHz / 37)
> 	divider = trunc(1.953) = 1 -> output period 23217 ns, 43.07 kHz
> 
> 43 kHz is outside the 21-28 kHz band that 4-wire fan PWM inputs are
> designed for. The fan on this board cannot start below duty 110/255 and
> stalls below 88/255, so closed-loop fan control oscillates between a
> stalled fan and full speed.
> 
> Round the divider to the closest integer instead. This bounds the
> period error to half a divider step rather than a full one, and
> restores what this configuration did before commit 8c193f4714df
> ("pwm: tegra: Optimize period calculation") switched the rounding from
> closest to down: divider round(0.99) = 1 on the undoubled 5589041 Hz
> grant, period 45802 ns, +1.0%. Between that commit and commit
> 5eccd0d9fabc ("pwm: tegra: Ensure the clock rate is not less than
> needed") the same request failed with -EINVAL instead. Verified on the
> Orin NX with this patch applied: PWM_SCALE reads 1 (21.54 kHz, +2.4%
> period), the fan starts at duty <= 50/255 and sustains 80/255, and
> closed-loop control is stable. There is no round-closest variant of
> mul_u64_u64_div_u64(), so compute twice the quotient and round up the
> halving.
> 
> Fixes: 5eccd0d9fabc ("pwm: tegra: Ensure the clock rate is not less than needed")
> Signed-off-by: Ola Chr. Vaage <ola.christoffer.vage@scoutdi.com>
> ---
>  drivers/pwm/pwm-tegra.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 172063b51d4..a5adc4f3ce6 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -163,9 +163,15 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  		pc->clk_rate = clk_get_rate(pc->clk);
>  	}
>  
> -	/* Consider precision in PWM_SCALE_WIDTH rate calculation */
> -	rate = mul_u64_u64_div_u64(pc->clk_rate, period_ns,
> +	/*
> +	 * Consider precision in PWM_SCALE_WIDTH rate calculation. Round to
> +	 * the closest integer: there is no round-closest variant of
> +	 * mul_u64_u64_div_u64(), so compute twice the quotient and round up
> +	 * the halving.
> +	 */
> +	rate = mul_u64_u64_div_u64(pc->clk_rate, 2 * (u64)period_ns,
>  				   (u64)NSEC_PER_SEC << PWM_DUTY_WIDTH);
> +	rate = DIV_ROUND_UP_ULL(rate, 2);
>  
>  	/*
>  	 * Since the actual PWM divider is the register's frequency divider

The usual behaviour for .apply() is to pick the largest possible period
not larger than the requested period (and similar for duty_cycle). The
tegra PWM driver doesn't do that and instead trys to pick a nearest
match. If you change how the configuration happens, the only acceptable
way is to migrate to the usual behaviour, still better, convert to the
waveform callbacks.

The driver also lacks a .get_state() callback and should not use
pwm_is_enabled(). Also calling tegra_pwm_config() discards bits from
state->duty_cycle and state->period if they are bigger than INT_MAX.

So there are some things to work on in that driver, but don't make the
clock selection not more complicated as it already is and as is
necessary.

Best regards
Uwe

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