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: <aot4ow37qsrehgce6vpc5m7ha5w6h4jvj7k7bokn4eo63sjk5x@iyp5ir234kx5>
Date: Fri, 27 Jun 2025 17:28:45 +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

On Mon, Jun 23, 2025 at 08:08:52PM +0200, Michal Wilczynski wrote:
> Introduce a PWM driver for the T-HEAD TH1520 SoC, written in Rust and
> utilizing the safe PWM abstractions from the preceding commit.
> 
> The driver implements the pwm::PwmOps trait using the modern waveform
> API (round_waveform_tohw, write_waveform, etc.) to support configuration
> of period, duty cycle, and polarity for the TH1520's PWM channels.
> 
> Resource management is handled using idiomatic Rust patterns. The PWM
> chip object is allocated via pwm::Chip::new and its registration with
> the PWM core is managed by the pwm::Registration RAII guard. This
> ensures pwmchip_remove is always called when the driver unbinds,
> preventing resource leaks. Device managed resources are used for the
> MMIO region, and the clock lifecycle is correctly managed in the
> driver's private data Drop implementation.
> 
> The driver's core logic is written entirely in safe Rust, with no unsafe
> blocks.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@...sung.com>
> ---
>  MAINTAINERS               |   1 +
>  drivers/pwm/Kconfig       |  10 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm_th1520.rs | 318 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 330 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a575622454a2ef57ce055c8a8c4765fa4fddc490..879870471e86dcec4a0e8f5c45d2cc3409411fdd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21402,6 +21402,7 @@ F:	drivers/mailbox/mailbox-th1520.c
>  F:	drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
>  F:	drivers/pinctrl/pinctrl-th1520.c
>  F:	drivers/pmdomain/thead/
> +F:	drivers/pwm/pwm_th1520.rs
>  F:	drivers/reset/reset-th1520.c
>  F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
>  F:	include/dt-bindings/power/thead,th1520-power.h
> 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.

> +	help
> +	  This option enables the driver for the PWM controller found on the
> +	  T-HEAD TH1520 SoC.
> +
> +	  To compile this driver as a module, choose M here; the module
> +	  will be called pwm-th1520. If you are unsure, say N.
> +
>  config PWM_TIECAP
>  	tristate "ECAP PWM support"
>  	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 96160f4257fcb0e0951581af0090615c0edf5260..a410747095327a315a6bcd24ae343ce7857fe323 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
>  obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
>  obj-$(CONFIG_PWM_SUNPLUS)	+= pwm-sunplus.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
> +obj-$(CONFIG_PWM_TH1520)	+= pwm_th1520.o
>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
>  obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
>  obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..a77c45cef9cf8f02a25db9d42c45cd0df565b0ec
> --- /dev/null
> +++ b/drivers/pwm/pwm_th1520.rs
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@...sung.com>
> +
> +//! Rust T-HEAD TH1520 PWM driver
> +//!
> +//! Limitations:
> +//! - The period and duty cycle are controlled by 32-bit hardware registers,
> +//!   limiting the maximum resolution.
> +//! - The driver supports continuous output mode only; one-shot mode is not
> +//!   implemented.
> +//! - The controller hardware provides up to 6 PWM channels.

Important questions to answer here are:

 - How does the hardware behave on disable? (Does it stop immediately
   (or at all)? Does it emit a constant output? Which?) 
 - How does the hardware behave on reconfiguration? (Does it switch
   immidiately or does it complete the current period? Can there be
   glitches?

> +//!
> +
> +use core::ops::Deref;
> +use kernel::{
> +    c_str,
> +    clk::Clk,
> +    device::{Bound, Core, Device},
> +    devres,
> +    io::mem::IoMem,
> +    of, platform,
> +    prelude::*,
> +    pwm, time,
> +};
> +
> +const MAX_PWM_NUM: u32 = 6;
> +
> +// Register offsets
> +const fn th1520_pwm_chn_base(n: u32) -> usize {
> +    (n * 0x20) as usize
> +}
> +
> +const fn th1520_pwm_ctrl(n: u32) -> usize {
> +    th1520_pwm_chn_base(n)
> +}
> +
> +const fn th1520_pwm_per(n: u32) -> usize {
> +    th1520_pwm_chn_base(n) + 0x08
> +}
> +
> +const fn th1520_pwm_fp(n: u32) -> usize {
> +    th1520_pwm_chn_base(n) + 0x0c
> +}
> +
> +// Control register bits
> +const PWM_START: u32 = 1 << 0;
> +const PWM_CFG_UPDATE: u32 = 1 << 2;
> +const PWM_CONTINUOUS_MODE: u32 = 1 << 5;
> +const PWM_FPOUT: u32 = 1 << 8;

Can you please add a driver specific prefix to these?

> +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

> +}
> +
> [...]
> +impl pwm::PwmOps for Th1520PwmDriverData {
> +    type WfHw = Th1520WfHw;
> +
> +    fn round_waveform_tohw(
> +        chip: &pwm::Chip,
> +        _pwm: &pwm::Device,
> +        wf: &pwm::Waveform,
> +    ) -> Result<(c_int, Self::WfHw)> {
> +        let data: &Self = chip.drvdata();
> +
> +        if wf.period_length_ns == 0 {
> +            return Ok((
> +                0,
> +                Th1520WfHw {
> +                    enabled: false,
> +                    ..Default::default()
> +                },
> +            ));
> +        }
> +
> +        let rate_hz = data.clk.rate().as_hz() as u64;
> +
> +        let period_cycles = ns_to_cycles(wf.period_length_ns, rate_hz).min(u32::MAX as u64);
> +        let mut duty_cycles = ns_to_cycles(wf.duty_length_ns, rate_hz).min(u32::MAX as u64);
> +
> +        let mut ctrl_val = PWM_CONTINUOUS_MODE;
> +
> +        if wf.duty_offset_ns == 0 {
> +            ctrl_val |= PWM_FPOUT;
> +        } else {
> +            duty_cycles = period_cycles - duty_cycles;

Huh, this looks wrong. Your hardware only supports the two polarities,
right? Then configure inversed polarity if

	wf->duty_length_ns && wf->duty_offset_ns && wf->duty_length_ns + wf->duty_offset_ns >= wf->period_length_ns

(i.e. how the pwm-stm32 driver does it).

> +        }
> +
> +        let wfhw = Th1520WfHw {
> +            period_cycles: period_cycles as u32,
> +            duty_cycles: duty_cycles as u32,
> +            ctrl_val,
> +            enabled: true,
> +        };
> +
> +        dev_dbg!(
> +            chip.device(),
> +            "Requested: period {}ns, duty {}ns, offset {}ns -> HW: period {} cyc, duty {} cyc, ctrl 0x{:x}\n",

Would it be helpful to also emit the clkrate here?

> +            wf.period_length_ns,
> +            wf.duty_length_ns,
> +            wf.duty_offset_ns,
> +            wfhw.period_cycles,
> +            wfhw.duty_cycles,
> +            wfhw.ctrl_val
> +        );
> +
> +        Ok((0, wfhw))
> +    }
> +
> +    fn round_waveform_fromhw(
> +        chip: &pwm::Chip,
> +        _pwm: &pwm::Device,
> +        wfhw: &Self::WfHw,
> +        wf: &mut pwm::Waveform,
> +    ) -> Result<c_int> {
> +        let data: &Self = chip.drvdata();
> +        let rate_hz = data.clk.rate().as_hz() as u64;
> +
> +        wf.period_length_ns = cycles_to_ns(wfhw.period_cycles as u64, rate_hz);
> +
> +        let duty_cycles = wfhw.duty_cycles as u64;
> +
> +        if (wfhw.ctrl_val & PWM_FPOUT) != 0 {
> +            wf.duty_length_ns = cycles_to_ns(duty_cycles, rate_hz);
> +            wf.duty_offset_ns = 0;
> +        } else {
> +            let period_cycles = wfhw.period_cycles as u64;
> +            let original_duty_cycles = period_cycles.saturating_sub(duty_cycles);
> +
> +            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.

> +        }
> +
> +        Ok(0)
> +    }

Best regards
Uwe

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ