Message-ID: <ca2ftwfducf4swuoondajiid642mctilh4lt3grgxpnup3nhag@tyuwyasfw4uj>
Date: Fri, 19 Sep 2025 09:10:08 +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>, 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>, 
	Drew Fustini <fustini@...nel.org>, Daniel Almeida <daniel.almeida@...labora.com>, 
	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
Subject: Re: [PATCH v14 4/7] pwm: Add Rust driver for T-HEAD TH1520 SoC

Hello,

actually I intended to give my feedback as a patch, but I'm not fluent
enough in Rust yet :-\, so here come my thoughts in text only.

On Wed, Aug 20, 2025 at 10:35:39AM +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, except for the Send and Sync implementations for the driver
> data, which are explained in the comments.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@...sung.com>
> ---
>  MAINTAINERS               |   1 +
>  drivers/pwm/Kconfig       |  11 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm_th1520.rs | 355 ++++++++++++++++++++++++++++++++++++++++++++++

For consistency I would prefer this file to be named pwm-th1520.rs. I
tried to fix that but then the compiler was unhappy. I guess a minus in
a filename just doesn't work for Rust?

>  4 files changed, 368 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5d7c0676c1d00a02b3d7db2de88b039c08c99c6e..d79dc21f22d143ca8cde6a06194545fbc640e695 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21741,6 +21741,7 @@ F:	drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
>  F:	drivers/pinctrl/pinctrl-th1520.c
>  F:	drivers/pmdomain/thead/
>  F:	drivers/power/sequencing/pwrseq-thead-gpu.c
> +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 2b608f4378138775ee3ba4d53f682952e1914118..dd6db01832ee985e2e588a413a13df869a029d3d 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -729,6 +729,17 @@ 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
> +	select RUST_PWM_ABSTRACTIONS
> +	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 ff4f47e5fb7a0dbac72c12de82c3773e5582db6d..5c15c95c6e49143969389198657eed0ecf4086b2 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -67,6 +67,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..5ef887b1b5dfed92c5d4b87a7d48f673352a257e
> --- /dev/null
> +++ b/drivers/pwm/pwm_th1520.rs
> @@ -0,0 +1,355 @@
> +// 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.
> +//! - Reconfiguration is glitch free - new period and duty cycle values are
> +//!   latched and take effect at the start of the next period.
> +//! - Polarity is handled via a simple hardware inversion bit; arbitrary
> +//!   duty cycle offsets are not supported.
> +//! - Disabling a channel is achieved by configuring its duty cycle to zero to
> +//!   produce a static low output. Clearing the `start` does not reliably
> +//!   force the static inactive level defined by the `INACTOUT` bit. Hence
> +//!   this method is not used in this driver.
> +//!
> +
> +use core::ops::Deref;
> +use kernel::{
> +    c_str,
> +    clk::Clk,
> +    device::{Bound, Core, Device},
> +    devres,
> +    io::mem::IoMem,
> +    of, platform,
> +    prelude::*,
> +    pwm, time,
> +};
> +
> +const TH1520_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 TH1520_PWM_START: u32 = 1 << 0;
> +const TH1520_PWM_CFG_UPDATE: u32 = 1 << 2;
> +const TH1520_PWM_CONTINUOUS_MODE: u32 = 1 << 5;
> +const TH1520_PWM_FPOUT: u32 = 1 << 8;
> +
> +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,
> +        None => u64::MAX,
> +    }) / NSEC_PER_SEC_U64
> +}
> +
> +fn cycles_to_ns(cycles: u64, rate_hz: u64) -> u64 {
> +    const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
> +
> +    // Round up
> +    let Some(numerator) = cycles
> +        .checked_mul(NSEC_PER_SEC_U64)
> +        .and_then(|p| p.checked_add(rate_hz - 1))
> +    else {
> +        return u64::MAX;
> +    };
> +
> +    numerator / rate_hz

Does cycles_to_ns(18446744074, 66000000) yield u64::MAX or u64::MAX /
66000000? Reading through the docs I found I think it's the former. I
guess we don't reach these spheres in the driver, but conceptually the
latter would be better and I think that's relevant as this is the first
Rust driver and so very likely will be the blueprint for further
drivers.

Further below you're using saturating_sub(), is there a similar variant
for multiplication that could simplify this function to

	let numerator = cyles
	  .saturating_mul(NSEC_PER_SEC_U64)
	  .saturating_add(rate_hz - 1);

	numerator / rate_hz

?

Additionally I'd add a comment that this should be replaced by
mul_u64_u64_div_u64_roundup() once that exists in Rust. (Sigh, I just
notice we still don't have that in C yet.)

> +}
> +
> +/// Hardware-specific waveform representation for TH1520.
> +#[derive(Copy, Clone, Debug, Default)]
> +struct Th1520WfHw {
> +    period_cycles: u32,
> +    duty_cycles: u32,
> +    ctrl_val: u32,
> +    enabled: bool,
> +}
> +
> +/// The driver's private data struct. It holds all necessary devres managed resources.
> +#[pin_data(PinnedDrop)]
> +struct Th1520PwmDriverData {
> +    #[pin]
> +    iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
> +    clk: Clk,
> +}
> +
> +// This `unsafe` implementation is a temporary necessity because the underlying `kernel::clk::Clk`
> +// type does not yet expose `Send` and `Sync` implementations. This block should be removed
> +// as soon as the clock abstraction provides these guarantees directly.
> +// TODO: Remove those unsafe impl's when Clk will support them itself.
> +
> +// SAFETY: The `devres` framework requires the driver's private data to be `Send` and `Sync`.
> +// We can guarantee this because the PWM core synchronizes all callbacks, preventing concurrent
> +// access to the contained `iomem` and `clk` resources.
> +unsafe impl Send for Th1520PwmDriverData {}
> +
> +// SAFETY: The same reasoning applies as for `Send`. The PWM core's synchronization
> +// guarantees that it is safe for multiple threads to have shared access (`&self`)
> +// to the driver data during callbacks.
> +unsafe impl Sync for Th1520PwmDriverData {}
> +
> +impl pwm::PwmOps for Th1520PwmDriverData {
> +    type WfHw = Th1520WfHw;
> +
> +    fn round_waveform_tohw(
> +        chip: &pwm::Chip<Self>,
> +        _pwm: &pwm::Device,
> +        wf: &pwm::Waveform,
> +    ) -> Result<pwm::RoundedWaveform<Self::WfHw>> {
> +        let data = chip.drvdata();
> +
> +        if wf.period_length_ns == 0 {

Please add a dev_dbg! here for consistency with the wf.period_length_ns
!= 0 case here.

> +            return Ok(pwm::RoundedWaveform {
> +                status: 0,
> +                hardware_waveform: 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(u64::from(u32::MAX));

What happens if you get period_cycles = 0 here?

> +        let mut duty_cycles = ns_to_cycles(wf.duty_length_ns, rate_hz).min(u64::from(u32::MAX));
> +
> +        let mut ctrl_val = TH1520_PWM_CONTINUOUS_MODE;
> +
> +        let is_inversed = wf.duty_length_ns > 0
> +            && wf.duty_offset_ns > 0
> +            && wf.duty_length_ns + wf.duty_offset_ns >= wf.period_length_ns;

For
	duty_length_ns =   0x8000000000000000
	duty_offset_ns =   0x8000000000000000
	period_length_ns = 0xffffffffffffffff

this should evaluate to true (right?) but if + overflows in the same way
in Rust as it does in C, you get is_inversed = false here.

The safe condition is

	wf.duty_offset_ns >= wf.period_length_ns - wf.duty_length_ns

here.

> +        if is_inversed {
> +            duty_cycles = period_cycles - duty_cycles;
> +        } else {
> +            ctrl_val |= TH1520_PWM_FPOUT;
> +        }
> +
> +        let wfhw = Th1520WfHw {
> +            period_cycles: period_cycles as u32,
> +            duty_cycles: duty_cycles as u32,

This casts period_cycles and duty_cycles from an u64 to an u32, right?
Can you please add a comment that shortly explains why these values fit
into an u32?

> +            ctrl_val,
> +            enabled: true,
> +        };
> +
> +        dev_dbg!(
> +            chip.device(),
> +            "clk_rate: {}Hz Requested: period {}ns, duty {}ns, offset {}ns -> HW: period {} cyc, duty {} cyc, ctrl 0x{:x}\n",

" " before the units please.

I want to establish

	"{}/{} [+{}]", duty_length_ns, period_length_ns, duty_offset_ns

as the typical way to emit a waveform. Please use that consistently.

> +            rate_hz,
> +            wf.period_length_ns,
> +            wf.duty_length_ns,
> +            wf.duty_offset_ns,
> +            wfhw.period_cycles,
> +            wfhw.duty_cycles,
> +            wfhw.ctrl_val
> +        );
> +
> +        Ok(pwm::RoundedWaveform {
> +            status: 0,
> +            hardware_waveform: wfhw,
> +        })
> +    }
> +
> +    fn round_waveform_fromhw(
> +        chip: &pwm::Chip<Self>,
> +        _pwm: &pwm::Device,
> +        wfhw: &Self::WfHw,
> +        wf: &mut pwm::Waveform,
> +    ) -> Result {
> +        let data = chip.drvdata();
> +        let rate_hz = data.clk.rate().as_hz() as u64;
> +
> +        wf.period_length_ns = cycles_to_ns(u64::from(wfhw.period_cycles), rate_hz);

Here I wonder again about wfhw.period_cycles = 0.

> +        let duty_cycles = u64::from(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 original_duty_cycles = period_cycles.saturating_sub(duty_cycles);
> +
> +            // 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);
> +        }
> +
> +        Ok(())
> +    }
> +
> +    fn read_waveform(
> +        chip: &pwm::Chip<Self>,
> +        pwm: &pwm::Device,
> +        parent_dev: &Device<Bound>,
> +    ) -> Result<Self::WfHw> {
> +        let data = chip.drvdata();
> +        let hwpwm = pwm.hwpwm();
> +        let iomem_accessor = data.iomem.access(parent_dev)?;
> +        let iomap = iomem_accessor.deref();
> +
> +        let ctrl = iomap.try_read32(th1520_pwm_ctrl(hwpwm))?;
> +        let period_cycles = iomap.try_read32(th1520_pwm_per(hwpwm))?;
> +        let duty_cycles = iomap.try_read32(th1520_pwm_fp(hwpwm))?;
> +
> +        let wfhw = Th1520WfHw {
> +            period_cycles,
> +            duty_cycles,
> +            ctrl_val: ctrl,
> +            enabled: duty_cycles != 0,
> +        };
> +
> +        dev_dbg!(
> +            chip.device(),
> +            "PWM-{}: read_waveform: Read hw state - period: {}, duty: {}, ctrl: 0x{:x}, enabled: {}",
> +            hwpwm,
> +            wfhw.period_cycles,
> +            wfhw.duty_cycles,
> +            wfhw.ctrl_val,
> +            wfhw.enabled
> +        );
> +
> +        Ok(wfhw)
> +    }
> +
> +    fn write_waveform(
> +        chip: &pwm::Chip<Self>,
> +        pwm: &pwm::Device,
> +        wfhw: &Self::WfHw,
> +        parent_dev: &Device<Bound>,
> +    ) -> Result {
> +        let data = chip.drvdata();
> +        let hwpwm = pwm.hwpwm();
> +        let iomem_accessor = data.iomem.access(parent_dev)?;
> +        let iomap = iomem_accessor.deref();
> +        let was_enabled = pwm.state().enabled();

The driver would be a tad more robust if was_enabled is determined from
the hardware registers instead. (And also it helps not to add another
dependency on pwm.state which I'd like to get rid of in the long run
with waveforms being the canonical representation internally.)

> +
> +        if !wfhw.enabled {

dev_dbg! here for consistence please.

> +            if was_enabled {
> +                iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
> +                iomap.try_write32(0, th1520_pwm_fp(hwpwm))?;
> +                iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
> +            }
> +            return Ok(());
> +        }
> +
> +        iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
> +        iomap.try_write32(wfhw.period_cycles, th1520_pwm_per(hwpwm))?;
> +        iomap.try_write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm))?;
> +        iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
> +
> +        // The `TH1520_PWM_START` bit must be written in a separate, final transaction, and
> +        // only when enabling the channel from a disabled state.
> +        if !was_enabled {
> +            iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_START, th1520_pwm_ctrl(hwpwm))?;
> +        }
> +
> +        dev_dbg!(
> +            chip.device(),
> +            "PWM-{}: Wrote (per: {}, duty: {})",
> +            hwpwm,
> +            wfhw.period_cycles,
> +            wfhw.duty_cycles,
> +        );
> +
> +        Ok(())
> +    }
> +}

Best regards
Uwe

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

Powered by blists - more mailing lists