[PATCH RESEND] pwm: th1520: Remove requirement for mul_u64_u64_div_u64_roundup

[PATCH RESEND] pwm: th1520: Remove requirement for mul_u64_u64_div_u64_roundup

[Maurice Hieronymus via B4 Relay]

From: Maurice Hieronymus <mhi@mailbox.org>

The cycle register is always u32, so cycles_to_ns() can take a u32
instead of a u64. With that narrowing, cycles * NSEC_PER_SEC is at most
u32::MAX * 1e9 (~4.3e18), which fits in u64 without overflow. The
saturating arithmetic is therefore no longer needed, and the ceiling
division can use Rust's u64::div_ceil() directly instead of the
open-coded numerator/denominator form.

This also drops the TODO referring to a future
mul_u64_u64_div_u64_roundup kernel helper, which is no longer required.

Signed-off-by: Maurice Hieronymus <mhi@mailbox.org>
---
Note: Resending v1 because my mail server (mailbox.org) was
unable to deliver the original submission to @kernerl.org
recipients. Going through the b4 web submission endpoint this
time. No changes to the patch content.
---
 drivers/pwm/pwm_th1520.rs | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
index ddd44a5ce497..933c1ec59c2a 100644
--- a/drivers/pwm/pwm_th1520.rs
+++ b/drivers/pwm/pwm_th1520.rs
@@ -67,16 +67,10 @@ fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
     ns.saturating_mul(rate_hz) / NSEC_PER_SEC_U64
 }
 
-fn cycles_to_ns(cycles: u64, rate_hz: u64) -> u64 {
+fn cycles_to_ns(cycles: u32, rate_hz: u64) -> u64 {
     const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
 
-    // TODO: Replace with a kernel helper like `mul_u64_u64_div_u64_roundup`
-    // once available in Rust.
-    let numerator = cycles
-        .saturating_mul(NSEC_PER_SEC_U64)
-        .saturating_add(rate_hz - 1);
-
-    numerator / rate_hz
+    (u64::from(cycles) * NSEC_PER_SEC_U64).div_ceil(rate_hz)
 }
 
 /// Hardware-specific waveform representation for TH1520.
@@ -192,15 +186,15 @@ fn round_waveform_fromhw(
             return Ok(());
         }
 
-        wf.period_length_ns = cycles_to_ns(u64::from(wfhw.period_cycles), rate_hz);
+        wf.period_length_ns = cycles_to_ns(wfhw.period_cycles, rate_hz);
 
-        let duty_cycles = u64::from(wfhw.duty_cycles);
+        let duty_cycles = wfhw.duty_cycles;
 
         if (wfhw.ctrl_val & TH1520_PWM_FPOUT) != 0 {
             wf.duty_length_ns = cycles_to_ns(duty_cycles, rate_hz);
             wf.duty_offset_ns = 0;
         } else {
-            let period_cycles = u64::from(wfhw.period_cycles);
+            let period_cycles = wfhw.period_cycles;
             let original_duty_cycles = period_cycles.saturating_sub(duty_cycles);
 
             // For an inverted signal, `duty_length_ns` is the high time (period - low_time).

---
base-commit: 3936b25815ee686a273ca7bbdc9ae19af5e608a3
change-id: 20260521-pwm-th1520-fix-8e45558bbd31

Best regards,
-- 
Maurice Hieronymus <mhi@mailbox.org>

Re: [PATCH RESEND] pwm: th1520: Remove requirement for mul_u64_u64_div_u64_roundup

[Michal Wilczynski]

On 5/25/26 15:11, Maurice Hieronymus via B4 Relay wrote:
> From: Maurice Hieronymus <mhi@mailbox.org>
> 
> The cycle register is always u32, so cycles_to_ns() can take a u32
> instead of a u64. With that narrowing, cycles * NSEC_PER_SEC is at most
> u32::MAX * 1e9 (~4.3e18), which fits in u64 without overflow. The
> saturating arithmetic is therefore no longer needed, and the ceiling
> division can use Rust's u64::div_ceil() directly instead of the
> open-coded numerator/denominator form.
> 
> This also drops the TODO referring to a future
> mul_u64_u64_div_u64_roundup kernel helper, which is no longer required.
> 
> Signed-off-by: Maurice Hieronymus <mhi@mailbox.org>

Hi Maurice,

Thanks for sending this.

I agree that this optimization makes perfect sense when looking at the
TH1520 driver in isolation. However, since this is currently the only
Rust PWM driver in the tree, it serves as the reference implementation
for future drivers.

We need cycles_to_ns to remain a generic blueprint. Many PWM controllers
have 64-bit registers, and in the C API, it is standard practice to use
helpers like DIV64_U64_ROUND_UP regardless of the register size (e.g.
axi_pwmgen). Resolving the TODO by assuming 32-bit registers removes
that generic capability.

At the time this was merged, the equivalent Rust math helpers didn't
exist yet, which is why the open-coded saturating math is there as a
placeholder.

To move forward, we should fix this by implementing the proper generic
64-bit math helper in the Rust abstractions, rather than narrowing the
types here. Are you open to looking into adding the Rust equivalent for
mul_u64_u64_div_u64_roundup instead?

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>

Re: [PATCH RESEND] pwm: th1520: Remove requirement for mul_u64_u64_div_u64_roundup

[Maurice Hieronymus]

On Mon, 2026-05-25 at 16:36 +0200, Michal Wilczynski wrote:
> 
> To move forward, we should fix this by implementing the proper
> generic
> 64-bit math helper in the Rust abstractions, rather than narrowing
> the
> types here. Are you open to looking into adding the Rust equivalent
> for
> mul_u64_u64_div_u64_roundup instead?

I would be definitely down, to help with that.

Before I start working on it, a couple of clarification questions.

Should I implement this as a thin FFI wrapper calling the C-Functions?
Or do you prefer to have a re-implementation in Rust.

Since mul_u64_u64_div_u64_roundup is a macro: Is it okay if this will
be a small Rust function or should it be a macro as well?

Thanks a lot,

Maurice

Re: [PATCH RESEND] pwm: th1520: Remove requirement for mul_u64_u64_div_u64_roundup

[Gary Guo]

On Mon May 25, 2026 at 4:51 PM BST, Maurice Hieronymus wrote:
> On Mon, 2026-05-25 at 16:36 +0200, Michal Wilczynski wrote:
>> 
>> To move forward, we should fix this by implementing the proper
>> generic
>> 64-bit math helper in the Rust abstractions, rather than narrowing
>> the
>> types here. Are you open to looking into adding the Rust equivalent
>> for
>> mul_u64_u64_div_u64_roundup instead?
>
> I would be definitely down, to help with that.
>
> Before I start working on it, a couple of clarification questions.
>
> Should I implement this as a thin FFI wrapper calling the C-Functions?
> Or do you prefer to have a re-implementation in Rust.
>
> Since mul_u64_u64_div_u64_roundup is a macro: Is it okay if this will
> be a small Rust function or should it be a macro as well?

This should definitely not be a macro. You should only use Rust macros when it
needs to be a macro. This function should just be a method of `u64` added via
an extension trait, so you could use `u64::mul_div_ceil` to invoke it.

The implementation of `mul_u64_add_u64_div_u64`is non-trivial, and thus you
should defer to the C function to avoid re-implementing the same code. However,
I think it'll be fine to first wrap the `mul_u64_add_u64_div_u64` as
`u64::mul_add_div()` and then have `u64::mul_div_ceil()` be

    #[inline]
    fn mul_div_ceil(self, mul: Self, div: Self) -> Self {
        self.mul_add_div(mul, div - 1, div)
    }

Best,
Gary

Re: [PATCH RESEND] pwm: th1520: Remove requirement for mul_u64_u64_div_u64_roundup

[Michal Wilczynski]

On 5/25/26 17:51, Maurice Hieronymus wrote:
> On Mon, 2026-05-25 at 16:36 +0200, Michal Wilczynski wrote:
>>
>> To move forward, we should fix this by implementing the proper
>> generic
>> 64-bit math helper in the Rust abstractions, rather than narrowing
>> the
>> types here. Are you open to looking into adding the Rust equivalent
>> for
>> mul_u64_u64_div_u64_roundup instead?
> 
> I would be definitely down, to help with that.
> 
> Before I start working on it, a couple of clarification questions.
> 
> Should I implement this as a thin FFI wrapper calling the C-Functions?
> Or do you prefer to have a re-implementation in Rust.
> 
> Since mul_u64_u64_div_u64_roundup is a macro: Is it okay if this will
> be a small Rust function or should it be a macro as well?
> 
> Thanks a lot,

Apologies, I have to correct my statement from before after discussing
with Uwe. The reason the macro was used in the mentioned driver is NOT
that PWM controllers have 64-bit registers - Uwe isn't aware of that
being a real pattern either, and I was inferring it from the code rather
than from hardware reality. The actual reason the macros get used isn't
unified across drivers and we should pin that down separately rather
than block your patch on it.

So please hold off on the Rust helper for now - your initial patch might
be the right direction.

> 
> Maurice
> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>

Re: [PATCH RESEND] pwm: th1520: Remove requirement for mul_u64_u64_div_u64_roundup

[Maurice Hieronymus]

> 
> Apologies, I have to correct my statement from before after
> discussing
No worries!
> with Uwe. The reason the macro was used in the mentioned driver is
> NOT
> that PWM controllers have 64-bit registers - Uwe isn't aware of that
> being a real pattern either, and I was inferring it from the code
> rather
> than from hardware reality. The actual reason the macros get used
> isn't
> unified across drivers and we should pin that down separately rather
> than block your patch on it.
> 
> So please hold off on the Rust helper for now - your initial patch
> might
> be the right direction.
> 
Is there any further conclusion which direction is right? Can we merge
my patch, does it need some change or are we going down the road with
the Rust helpers?

Thanks!

Maurice
> > 
> > Maurice
> > 
> 
> Best regards,

Re: [PATCH RESEND] pwm: th1520: Remove requirement for mul_u64_u64_div_u64_roundup

[Michal Wilczynski]

On 5/30/26 12:41, Maurice Hieronymus wrote:
>>
>> Apologies, I have to correct my statement from before after
>> discussing
> No worries!
>> with Uwe. The reason the macro was used in the mentioned driver is
>> NOT
>> that PWM controllers have 64-bit registers - Uwe isn't aware of that
>> being a real pattern either, and I was inferring it from the code
>> rather
>> than from hardware reality. The actual reason the macros get used
>> isn't
>> unified across drivers and we should pin that down separately rather
>> than block your patch on it.
>>
>> So please hold off on the Rust helper for now - your initial patch
>> might
>> be the right direction.
>>
> Is there any further conclusion which direction is right? Can we merge
> my patch, does it need some change or are we going down the road with
> the Rust helpers?

I think this patch is fine to move forward.

We were wondering if there would be any noticeable difference in
execution speed between the 64-bit computations and the mixed
32-bit/64-bit computations. I ran a profile on the platform, and the
performance with and without the patch is identical:

Here's the test I ran

WITH PATCH
# hyperfine --warmup 2 --runs 40 './pwmtestperf'                                                                                                                    
  Time (mean ± σ):      3.370 s ±  0.069 s    [User: 0.069 s, System: 3.313 s]
  Range (min … max):    3.271 s …  3.547 s    40 runs
 
WITHOUT PATCH
# hyperfine --warmup 2 --runs 40 './pwmtestperf
  Time (mean ± σ):      3.368 s ±  0.051 s    [User: 0.058 s, System: 3.326 s]
  Range (min … max):    3.278 s …  3.516 s    40 runs

The delta is completely within the margin of error, confirming that
64-bit math is just as fast as 32-bit math on this architecture under
standard loads. It is worth mentioning that to get a more meaningful
performance measurement, pwmtestperf would probably need to be modified
to stress this specific code path much more heavily.

That said, fitting it into 32-bit where appropriate makes perfect sense
from a code quality perspective.

Reviewed-by: Michal Wilczynski <m.wilczynski@samsung.com>

> 
> Thanks!
> 
> Maurice
>>>
>>> Maurice
>>>
>>
>> Best regards,
>