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