Re: [PATCH v2] pwm: atmel: Fix prescaler for periods of 2^32 clock cycles or more

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

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

Hello Karl,

On Fri, Oct 16, 2026 at 06:12:21PM +0200, Karl Mehltretter wrote:
> atmel_pwm_calculate_cprd_and_pres() keeps the period in clock cycles in
> an unsigned long long but derives the prescaler from fls(cycles). fls()
> takes an unsigned int, so the upper 32 bits are ignored.
> 
> On controllers that the driver configures with a 32-bit period register
> (SAM9X60, SAM9X7) a request of 2^32 clock cycles or more therefore gets
> prescaler 0 and a truncated CPRD. The request succeeds and sysfs
> reports the requested period.
> 
> On a SAM9X75 (PWM clock 266.67 MHz) this affects periods from about
> 16.1 s on. Requesting 20 s with a 10 s duty cycle
> 
>   # echo 2 > /sys/class/pwm/pwmchip0/export
>   # echo 20000000000 > /sys/class/pwm/pwmchip0/pwm2/period
>   # echo 10000000000 > /sys/class/pwm/pwmchip0/pwm2/duty_cycle
>   # echo 1 > /sys/class/pwm/pwmchip0/pwm2/enable
> 
> programs CPRE=0, CPRD=0x3de4355c and CDTY=0x9ef21aae. The channel runs
> with a period of 3.9 s, measured from the period end flag in PWM_ISR.
> With this duty cycle CDTY is above CPRD, so the output stays inactive
> and an LED on it stays dark.
> 
> Use fls64() so that the prescaler is derived from the full 64-bit
> value. The driver then programs CPRE=1, CPRD=0x9ef21aae and
> CDTY=0x4f790d57. The measured period is 20.0 s and the LED is on for
> 10 s of every 20 s.
> 
> Controllers configured with a 16-bit period register are unaffected
> for all representable periods. Some oversized requests of 2^32 clock
> cycles or more now fail with -EINVAL instead of being programmed with
> truncated values. Clamping such requests to the maximum possible
> configuration is a pre-existing issue best addressed when converting
> the driver to the waveform API.
> 
> The truncation was not reachable when it was introduced. The period
> was an unsigned int in nanoseconds then, which limits it to about
> 4.29 s. Commit a9d887dc1c60 ("pwm: Convert period and duty cycle to
> u64") made longer periods possible.
> 
> Fixes: a9d887dc1c60 ("pwm: Convert period and duty cycle to u64")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
> ---
> Changes in v2:
> - Keep only the Fixes tag for the commit that exposed the bug (Uwe).
> - Clarify the pre-existing handling of oversized requests (Uwe).
> - Add Microchip's confirmation of the data sheet error.
> - Rebase onto v7.3-rc7.
> - No code changes from v1.
> 
> v1: https://lore.kernel.org/r/20261003083036.21584-1-kmehltretter@gmail.com/
> 
> Notes:
>     Found while testing a Rust port of this driver on a SAM9X75 Curiosity
>     board. The port uses a 64-bit fls. Its registers differed from the C
>     driver for long periods.
> 
>     Tested on that board with a v7.3-rc5-based kernel built with clang
>     22.1.8. PC20 was routed to PWM2 (peripheral function C) in a local
>     device tree. The blue LED is on that pin.
> 
>     - Without the patch there are 23 period ends in 90 s, 3.88 s to 3.93 s
>       apart. The LED stays dark.
>     - With the patch the period ends are 19.98 s to 20.01 s apart. The LED
>       is on for 10 s of every 20 s.
> 
>     The SAM9X60 and SAM9X7 data sheets describe the channel counter as
>     16 bit. That does not match the hardware.
> 
>     - The 20.0 s result needs CPRD=0x9ef21aae. All 32 bits are used.
>     - Commit 0285827d546d ("pwm: atmel: Add support for controllers with
>       32 bit counters") says 32 bit.
>     - Microchip's Harmony PWM example for this board programs
>       CPRD=133333333.
> 
>     Microchip support has confirmed that the data sheets are incorrect and
>     will be corrected.
> 
>  drivers/pwm/pwm-atmel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 86918523d821..574020d90a3b 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -195,7 +195,7 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
>  	 * So for each bit the number of clock cycles is wider divide the input
>  	 * clock frequency by two using pres and shift cprd accordingly.
>  	 */
> -	shift = fls(cycles) - atmel_pwm->data->cfg.period_bits;
> +	shift = fls64(cycles) - atmel_pwm->data->cfg.period_bits;
>  
>  	if (shift > PWM_MAX_PRES) {
>  		dev_err(pwmchip_parent(chip), "pres exceeds the maximum value\n");

just above this code snippet there is:

	unsigned long long cycles = state->period;

        /* Calculate the period cycles and prescale value */
        cycles *= clkrate;
        do_div(cycles, NSEC_PER_SEC);

there is another overflow happening here if state->period * clkrate
doesn't fit into an u64. A complete fix converts that to:

	cycles = mul_u64_u64_div_u64(state->period, clkrate, NSEC_PER_SEC);

which prevents an overflow if clkrate ≤ NSEC_PER_SEC.
The latter can be asserted by checking that at probe time after calling
clk_rate_exclusive_get().

The options now are I apply your patch anyhow and the above can be
addressed later, or you rework your patch to a complete fix. What is
your preference?

Best regards
Uwe

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