linux-kernel - Re: [PATCH v5 4/9] pwm: Add Rust driver for T-HEAD TH1520 SoC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <i2rewsouxhtbomkfd7gyxpvrxi4f6tu26visny25qilhhttcft@ufqv4p3bwcjv>
Date: Sun, 29 Jun 2025 11:08:42 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Michal Wilczynski <m.wilczynski@...sung.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, Drew Fustini <drew@...7.com>, Guo Ren <guoren@...nel.org>, 
	Fu Wei <wefu@...hat.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>, 
	Marek Szyprowski <m.szyprowski@...sung.com>, Benno Lossin <lossin@...nel.org>, 
	Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-pwm@...r.kernel.org, rust-for-linux@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	devicetree@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH v5 4/9] pwm: Add Rust driver for T-HEAD TH1520 SoC

Hello Michal,

On Sat, Jun 28, 2025 at 08:14:59PM +0200, Michal Wilczynski wrote:
> On 6/27/25 17:28, Uwe Kleine-König wrote:
> > On Mon, Jun 23, 2025 at 08:08:52PM +0200, Michal Wilczynski wrote:
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index cfddeae0eab3523f04f361fb41ccd1345c0c937b..a675b3bd68392d1b05a47a2a1390c5606647ca15 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -719,6 +719,16 @@ config PWM_TEGRA
> >>  	  To compile this driver as a module, choose M here: the module
> >>  	  will be called pwm-tegra.
> >>  
> >> +config PWM_TH1520
> >> +	tristate "TH1520 PWM support"
> >> +	depends on RUST_PWM_ABSTRACTIONS
> > 
> > RUST_PWM_ABSTRACTIONS is user selectable. Is that sensible. From a
> > user's POV it shouldn't matter if the driver is written in Rust or not.
> 
> You make an excellent point about user experience. My initial thought
> was to follow the depends on pattern that I saw in other Rust drivers.
> 
> I can see how using select would be cleaner for the end user. My only
> hesitation was that it differs from the current convention for Rust
> drivers, and I wanted to be careful about changing an established
> pattern.
> 
> If you are comfortable with setting this direction for the PWM
> subsystem, I am happy to make the change to use select
> RUST_PWM_ABSTRACTIONS (gated by depends on RUST). Please let me know.

Sounds good.

> >> +const TH1520_PWM_REG_SIZE: usize = 0xB0;
> >> +
> >> +fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
> >> +    const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
> >> +
> >> +    match ns.checked_mul(rate_hz) {
> >> +        Some(product) => product / NSEC_PER_SEC_U64,
> >> +        None => u64::MAX,
> >> +    }
> > 
> > The semantic here is: If ns * rate_hz overflows, return U64_MAX, else ns
> > * rate_hz / NSEC_PER_SEC, right?
> > 
> > If you cannot easily reproduce what mul_u64_u64_div_u64() does, I think
> > it would be more prudent do make this:
> > 
> > 	match ns.checked_mul(rate_hz) {
> > 	    Some(product) => product,
> > 	    None => u64::MAX,
> > 	} / NSEC_PER_SEC_U64
> 
> Thank you for the feedback on the calculation. I analyzed the two
> approaches and found that on overflow, my version saturates to u64::MAX,
> while the suggested version would result in u64::MAX / NSEC_PER_SEC. I
> believe my original implementation's saturation behavior is more
> predictable. With this in mind, would you be comfortable with me
> retaining the original implementation?

I'm convinced that my alternative is better. Consider the implemented
mapping: Assuming rate_hz = 160000000 you have:

	      ns     |       cycles
	-------------+---------------------
	 ...         |
	115292150452 |          18446744072
	115292150453 |          18446744072
	115292150454 |          18446744072
	115292150455 |          18446744072
	115292150456 |          18446744072
	115292150457 |          18446744073
	115292150458 |          18446744073
	115292150459 |          18446744073
	115292150460 |          18446744073
	115292150461 | 18446744073709551615
	115292150462 | 18446744073709551615
	 ...

that's strange, isn't it?

> >> +            wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
> >> +            // We can't recover the original non-zero offset, so we just set it
> >> +            // to a representative non-zero value.
> >> +            wf.duty_offset_ns = 1;
> > 
> > For an inversed polarity signal the duty_offset is polarity - duty_cycle.
> 
> I believe there was a typo in your suggestion and you meant period
> instead of polarity.

ack.

> Based on that, my understanding is that for an
> inverted signal, the generic pwm_waveform struct expects duty_offset_ns
> to represent the duration of the initial low time, while duty_length_ns
> represents the high time.

right.

> so the code would look like this:
> // For an inverted signal, `duty_length_ns` is the high time (period - low_time).
> wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
> // The offset is the initial low time, which is what the hardware register provides.
> wf.duty_offset_ns = cycles_to_ns(duty_cycles, rate_hz);

Looks correct

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)