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

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

[Karl Mehltretter]

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");

base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.39.5 (Apple Git-154)

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

[Uwe Kleine-König]

[-- 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 --]