[PATCH] pwm: tegra: Fix value discarding for duty and period

[PATCH] pwm: tegra: Fix value discarding for duty and period

[Uwe Kleine-König]

tegra_pwm_config() is called with u64 values passed for duty_ns and
period_ns in tegra_pwm_apply(). Make these parameters u64 instead of int
to not lose the upper bits for bigger values. If such a discard happens
the values that are written to hardware become wrong, or (if the
discarded period value becomes 0) a division by zero happens.

Adapt the math functions accordingly to not let the now potentially
bigger values result in overflows.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/bug/linux-5c5b97d6-4da6-4d06-9dc7-957419dcd832
Fixes: fd3ddd4355c0 ("pwm: tegra: Implement .apply() callback")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
Hello,

actually the Fixes: line isn't quite accurate. The problem that the
arguments are cropped to int from u64 exists since v4.7-rc1 (commit
5ec803edcb70 ("pwm: Add core infrastructure to allow atomic updates"))
in the pwm core, but the blamed commit above is the commit where this
problem was moved to the pwm-tegra driver and thus it's fixable there.
The problem in the core doesn't exist any more since commit 0829c35dc534
("pwm: Drop support for legacy drivers") in v6.0-rc1.

Note this patch changes the rounding of the duty cycle (from somewhat
round closest to somewhat round down). It would not be hard to keep the
behaviour by introducing a variant of mul_u64_u64_div_u64() that rounds
closest, but the rounding is spoiled anyhow by later rounding of the
period. To actually fix that it would be best to convert the driver to
the waveform callbacks.

Example calculation: Let's assume an input clock (pc->clk_rate) of
100000 Hz and a request with period_ns = 230399744 and
duty_ns = 128249857. To translate that into register settings the
DUTY_SHIFT values will be round down with both approaches and result in
an actual duty_cycle of 127658000 ns (Δ = -591857 ns) while 128557000 ns
(Δ = 307143 ns) would also be possible.

Best regards
Uwe

 drivers/pwm/pwm-tegra.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 5cdbe120ba2d..8b5ed7998fdd 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -138,22 +138,21 @@ static inline void tegra_pwm_writel(struct pwm_device *pwm, unsigned int offset,
 }
 
 static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			    int duty_ns, int period_ns)
+			    u64 duty_ns, u64 period_ns)
 {
 	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
-	unsigned long long c = duty_ns;
 	unsigned long rate, required_clk_rate;
 	u32 val = 0;
 	int err;
+	u64 c;
 
 	/*
 	 * Convert from duty_ns / period_ns to a fixed number of duty ticks
-	 * per TEGRA_PWM_DEPTH cycles and make sure to round to the
-	 * nearest integer during division.
+	 * per TEGRA_PWM_DEPTH cycles.
 	 */
-	c *= TEGRA_PWM_DEPTH;
-	c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
+	c = mul_u64_u64_div_u64(duty_ns, TEGRA_PWM_DEPTH, period_ns);
 
+	/* The cast here is fine as c <= TEGRA_PWM_DEPTH */
 	val = (u32)c << TEGRA_PWM_DUTY_SHIFT;
 
 	/*

base-commit: 00e917df050d79ac26ec700717680ad6421d2808
-- 
2.56.0.rc1

Re: [PATCH] pwm: tegra: Fix value discarding for duty and period

[Mikko Perttunen]

On Thursday, October 15, 2026 5:53 PM Uwe Kleine-König wrote:
> tegra_pwm_config() is called with u64 values passed for duty_ns and
> period_ns in tegra_pwm_apply(). Make these parameters u64 instead of int
> to not lose the upper bits for bigger values. If such a discard happens
> the values that are written to hardware become wrong, or (if the
> discarded period value becomes 0) a division by zero happens.
> 
> Adapt the math functions accordingly to not let the now potentially
> bigger values result in overflows.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://sashiko.dev/#/bug/linux-5c5b97d6-4da6-4d06-9dc7-957419dcd832
> Fixes: fd3ddd4355c0 ("pwm: tegra: Implement .apply() callback")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
> Hello,
> 
> actually the Fixes: line isn't quite accurate. The problem that the
> arguments are cropped to int from u64 exists since v4.7-rc1 (commit
> 5ec803edcb70 ("pwm: Add core infrastructure to allow atomic updates"))
> in the pwm core, but the blamed commit above is the commit where this
> problem was moved to the pwm-tegra driver and thus it's fixable there.
> The problem in the core doesn't exist any more since commit 0829c35dc534
> ("pwm: Drop support for legacy drivers") in v6.0-rc1.
> 
> Note this patch changes the rounding of the duty cycle (from somewhat
> round closest to somewhat round down). It would not be hard to keep the
> behaviour by introducing a variant of mul_u64_u64_div_u64() that rounds
> closest, but the rounding is spoiled anyhow by later rounding of the
> period. To actually fix that it would be best to convert the driver to
> the waveform callbacks.
> 
> Example calculation: Let's assume an input clock (pc->clk_rate) of
> 100000 Hz and a request with period_ns = 230399744 and
> duty_ns = 128249857. To translate that into register settings the
> DUTY_SHIFT values will be round down with both approaches and result in
> an actual duty_cycle of 127658000 ns (Δ = -591857 ns) while 128557000 ns
> (Δ = 307143 ns) would also be possible.
> 
> Best regards
> Uwe
> 
>  drivers/pwm/pwm-tegra.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 5cdbe120ba2d..8b5ed7998fdd 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -138,22 +138,21 @@ static inline void tegra_pwm_writel(struct pwm_device *pwm, unsigned int offset,
>  }
>  
>  static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> -			    int duty_ns, int period_ns)
> +			    u64 duty_ns, u64 period_ns)
>  {
>  	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> -	unsigned long long c = duty_ns;
>  	unsigned long rate, required_clk_rate;
>  	u32 val = 0;
>  	int err;
> +	u64 c;
>  
>  	/*
>  	 * Convert from duty_ns / period_ns to a fixed number of duty ticks
> -	 * per TEGRA_PWM_DEPTH cycles and make sure to round to the
> -	 * nearest integer during division.
> +	 * per TEGRA_PWM_DEPTH cycles.
>  	 */
> -	c *= TEGRA_PWM_DEPTH;
> -	c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
> +	c = mul_u64_u64_div_u64(duty_ns, TEGRA_PWM_DEPTH, period_ns);
>  
> +	/* The cast here is fine as c <= TEGRA_PWM_DEPTH */
>  	val = (u32)c << TEGRA_PWM_DUTY_SHIFT;

The change looks good to me, but the required clock rate calculation 
further below should also be updated accordingly.

Thanks
Mikko

>  
>  	/*
> 
> base-commit: 00e917df050d79ac26ec700717680ad6421d2808
> -- 
> 2.56.0.rc1
> 
>