Hi Fabrice, On Fri, 2026-10-30 at 15:58 +0100, Fabrice Gasnier wrote: > On 10/30/26 11:31, Thomas Perrot wrote: > > stm32_pwm_set_config() derives the prescaler with > > do_div(div, priv->max_arr + 1). max_arr is a u32, so on a 32-bit > > timer > > (max_arr = 0xFFFFFFFF, e.g. TIM5 on STM32MP15) the sum wraps to 0 > > and > > do_div() divides by zero. Configuring a PWM on such a timer, for > > instance for a backlight, resets the board. > > Hi Thomas, > > Ouch :-( > > Thanks for spotting !
Thanks for the review and the tags.
>
> >
> > do_div() takes a 32-bit divisor, so widening the operand is not
> > enough.
> > Use div64_u64() with a 64-bit divisor. The prescaler then comes out
> > as
> > 0, which is right since the period fits in ARR without prescaling.
> >
> > Fixes: be5523d38296 ("pwm: stm32: add driver to support pwm with
> > timer")
> > Signed-off-by: Thomas Perrot <[email protected]>
> > ---
> > On a 32-bit timer such as TIM5 on STM32MP15, stm32_pwm_set_config()
> > divides by max_arr + 1, which wraps to 0 in a u32. Seen when a
> > backlight on TIM5 is configured from U-Boot.
> >
> > The fix is ported from a change carried in ST's U-Boot v2023.10-
> > stm32mp
> > tree.
>
> Do you mean this is applicable to ST's downstream too ?
> (I don't find such fix there)
I wrote this fix on top of ST's v2023.10-stm32mp tree and I believe it
applies there too.
>
> > ---
> > drivers/pwm/pwm-stm32.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > index a691f75e4a71..3f2b593477b1 100644
> > --- a/drivers/pwm/pwm-stm32.c
> > +++ b/drivers/pwm/pwm-stm32.c
> > @@ -13,6 +13,7 @@
> > #include <asm/arch/timers.h>
> > #include <dm/device_compat.h>
> > #include <linux/bitfield.h>
> > +#include <linux/math64.h>
> > #include <linux/time.h>
> >
> > #define CCMR_CHANNEL_SHIFT 8
> > @@ -64,7 +65,7 @@ static int stm32_pwm_set_config(struct udevice
> > *dev, uint channel,
> > do_div(div, NSEC_PER_SEC);
> > prd = div;
> >
> > - do_div(div, priv->max_arr + 1);
> > + div = div64_u64(div, (u64)priv->max_arr + 1);
>
> I'm fine with the patch. Just suggesting here: div isn't recycled
> later.
> So could directly set:
> prescaler = div64_u64(prd, (u64)priv->max_arr + 1)
>
> > prescaler = div;
>
> By doing this, the div variable may be dropped completely to end-up
> with simply:
> prd = (unsigned long long)priv->rate * period_ns;
> do_div(prd, NSEC_PER_SEC);
>
> prescaler = div64_u64(prd, (u64)priv->max_arr + 1);
>
> Up to you to update...
I’ll send a v2 to remove the div.
Kind regards,
Thomas
>
> Both case you can add my:
> Tested-by: Fabrice Gasnier <[email protected]>
> Reviewed-by: Fabrice Gasnier <[email protected]>
>
> Best Regards,
> Fabrice
>
> > if (prescaler > MAX_TIM_PSC)
> > return -EINVAL;
> >
> > ---
> > base-commit: 03e45557e1f38a39a0c80fe30483130ff376cf92
> > change-id: 20261029-tprrt-pwm-stm32-32bit-timer-div-eeb070ead061
> >
> > Best regards,
signature.asc
Description: This is a digitally signed message part
