* [PATCH v2 1/7] rust: Add basic PWM abstractions
2025-06-10 12:52 ` [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
@ 2025-06-10 12:52 ` Michal Wilczynski
2025-06-11 13:35 ` Miguel Ojeda
2025-06-12 9:12 ` Benno Lossin
2025-06-10 12:52 ` [PATCH v2 2/7] pwm: Add Rust driver for T-HEAD TH1520 SoC Michal Wilczynski
` (6 subsequent siblings)
7 siblings, 2 replies; 27+ messages in thread
From: Michal Wilczynski @ 2025-06-10 12:52 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Introduce safe Rust abstractions for the Linux PWM subsystem. These
abstractions provide ergonomic, lifetime managed wrappers around the
core C data structures and functions, enabling the development of PWM
chip drivers in safe Rust.
This initial version provides the core building blocks for writing a PWM
chip provider driver, with a focus on safety, resource management, and
idiomatic Rust patterns.
The main components are:
Ownership and Lifetime Management:
- The pwm::Chip type, an ARef managed wrapper for struct pwm_chip,
correctly handles the object's lifetime by using the embedded struct
device's reference counter.
- A pwm::Registration RAII guard ensures that a call to register a
chip (pwmchip_add) is always paired with a call to unregister it
(pwmchip_remove), preventing resource leaks.
Safe Type Wrappers:
- Safe, idiomatic Rust types (Polarity, Waveform, State, Args,
Device) are provided to abstract away the raw C structs and enums.
The State wrapper holds its data by value, avoiding unnecessary
heap allocations.
Driver Operations (PwmOps):
- A generic PwmOps trait allows drivers to implement the standard
PWM operations. It uses an associated type (WfHw) for the driver's
hardware specific waveform data, moving unsafe serialization logic into
the abstraction layer.
The trait exposes the modern waveform API (round_waveform_tohw,
write_waveform, etc.) as well as the other standard kernel callbacks
(get_state, request, apply).
- A create_pwm_ops function generates a C-compatible vtable from a
PwmOps implementor.
This foundational layer is designed to be used by subsequent patches to
implement specific PWM chip drivers in Rust.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
MAINTAINERS | 6 +
drivers/pwm/Kconfig | 13 +
rust/bindings/bindings_helper.h | 1 +
rust/helpers/helpers.c | 1 +
rust/helpers/pwm.c | 20 +
rust/kernel/lib.rs | 2 +
rust/kernel/pwm.rs | 864 ++++++++++++++++++++++++++++++++++++++++
7 files changed, 907 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index f2668b81115cb85bc94275e9554330969989fabf..5589c0d2253bcb04e78d7b89ef6ef0ed41121d77 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19990,6 +19990,12 @@ F: include/linux/pwm.h
F: include/linux/pwm_backlight.h
K: pwm_(config|apply_might_sleep|apply_atomic|ops)
+PWM SUBSYSTEM BINDINGS [RUST]
+M: Michal Wilczynski <m.wilczynski@samsung.com>
+S: Maintained
+F: rust/helpers/pwm.c
+F: rust/kernel/pwm.rs
+
PXA GPIO DRIVER
M: Robert Jarzmik <robert.jarzmik@free.fr>
L: linux-gpio@vger.kernel.org
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index d9bcd1e8413eaed1602d6686873e263767c58f5f..03c5a100a03e2acdccf8a46b9c70b736b630bd3a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -790,4 +790,17 @@ config PWM_XILINX
To compile this driver as a module, choose M here: the module
will be called pwm-xilinx.
+ config RUST_PWM_ABSTRACTIONS
+ bool "Rust PWM abstractions support"
+ depends on RUST
+ depends on PWM=y
+ help
+ This option enables the safe Rust abstraction layer for the PWM
+ subsystem. It provides idiomatic wrappers and traits necessary for
+ writing PWM controller drivers in Rust.
+
+ The abstractions handle resource management (like memory and reference
+ counting) and provide safe interfaces to the underlying C core,
+ allowing driver logic to be written in safe Rust.
+
endif
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 693cdd01f9290fa01375cf78cac0e5a90df74c6c..6fe7dd529577952bf7adb4fe0526b0d5fbd6f3bd 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -64,6 +64,7 @@
#include <linux/pm_opp.h>
#include <linux/poll.h>
#include <linux/property.h>
+#include <linux/pwm.h>
#include <linux/refcount.h>
#include <linux/sched.h>
#include <linux/security.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 0f1b5d11598591bc62bb6439747211af164b76d6..73902d8bd87e93cb3bc3c501360c37e29e8dde19 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -30,6 +30,7 @@
#include "platform.c"
#include "pci.c"
#include "pid_namespace.c"
+#include "pwm.c"
#include "rbtree.c"
#include "rcu.c"
#include "refcount.c"
diff --git a/rust/helpers/pwm.c b/rust/helpers/pwm.c
new file mode 100644
index 0000000000000000000000000000000000000000..d75c588863685d3990b525bb1b84aa4bc35ac397
--- /dev/null
+++ b/rust/helpers/pwm.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2025 Samsung Electronics Co., Ltd.
+// Author: Michal Wilczynski <m.wilczynski@samsung.com>
+
+#include <linux/pwm.h>
+
+struct device *rust_helper_pwmchip_parent(const struct pwm_chip *chip)
+{
+ return pwmchip_parent(chip);
+}
+
+void *rust_helper_pwmchip_get_drvdata(struct pwm_chip *chip)
+{
+ return pwmchip_get_drvdata(chip);
+}
+
+void rust_helper_pwmchip_set_drvdata(struct pwm_chip *chip, void *data)
+{
+ pwmchip_set_drvdata(chip, data);
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index d652c92633b82525f37e5cd8a040d268e0c191d1..d634593d5e8084049cc22daadb5019de139599fe 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -106,6 +106,8 @@
pub mod seq_file;
pub mod sizes;
mod static_assert;
+#[cfg(CONFIG_RUST_PWM_ABSTRACTIONS)]
+pub mod pwm;
#[doc(hidden)]
pub mod std_vendor;
pub mod str;
diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
new file mode 100644
index 0000000000000000000000000000000000000000..b5839703c49ed7b9aa61723a7e506f5cb4dd665d
--- /dev/null
+++ b/rust/kernel/pwm.rs
@@ -0,0 +1,864 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2025 Samsung Electronics Co., Ltd.
+// Author: Michal Wilczynski <m.wilczynski@samsung.com>
+
+//! PWM (Pulse Width Modulator) abstractions.
+//!
+//! This module provides safe Rust abstractions for working with the Linux
+//! kernel's PWM subsystem, leveraging types generated by `bindgen`
+//! from `<linux/pwm.h>` and `drivers/pwm/core.c`.
+
+use crate::{
+ bindings,
+ device::{self, Bound},
+ error::{self, to_result, Result},
+ prelude::*,
+ str::CStr,
+ types::{ARef, AlwaysRefCounted, ForeignOwnable, Opaque},
+};
+use core::{marker::PhantomData, mem::ManuallyDrop, ptr::NonNull};
+
+/// Maximum size for the hardware-specific waveform representation buffer.
+/// From C: #define WFHWSIZE 20
+pub const WFHW_MAX_SIZE: usize = 20;
+
+/// PWM polarity. Mirrors `enum pwm_polarity`.
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+pub enum Polarity {
+ /// Normal polarity (duty cycle defines the high period of the signal)
+ Normal,
+ /// Inversed polarity (duty cycle defines the low period of the signal)
+ Inversed,
+}
+
+impl From<bindings::pwm_polarity> for Polarity {
+ fn from(polarity: bindings::pwm_polarity) -> Self {
+ match polarity {
+ bindings::pwm_polarity_PWM_POLARITY_NORMAL => Polarity::Normal,
+ bindings::pwm_polarity_PWM_POLARITY_INVERSED => Polarity::Inversed,
+ _ => Polarity::Normal,
+ }
+ }
+}
+
+impl From<Polarity> for bindings::pwm_polarity {
+ fn from(polarity: Polarity) -> Self {
+ match polarity {
+ Polarity::Normal => bindings::pwm_polarity_PWM_POLARITY_NORMAL,
+ Polarity::Inversed => bindings::pwm_polarity_PWM_POLARITY_INVERSED,
+ }
+ }
+}
+
+/// Represents a PWM waveform configuration. Mirrors struct pwm_waveform.
+#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
+pub struct Waveform {
+ /// Total duration of one complete PWM cycle, in nanoseconds.
+ pub period_length_ns: u64,
+
+ /// Duration the PWM signal is in its "active" state during one period,
+ /// in nanoseconds. For a typical "normal" polarity configuration where active is high,
+ /// this represents the high time of the signal.
+ pub duty_length_ns: u64,
+
+ /// Time delay from the start of the period to the first active edge
+ /// of the duty cycle, in nanoseconds. For many simpler PWM configurations,
+ /// this is 0, meaning the duty cycle's active phase starts at the beginning
+ /// of the period.
+ pub duty_offset_ns: u64,
+}
+
+impl From<bindings::pwm_waveform> for Waveform {
+ fn from(wf: bindings::pwm_waveform) -> Self {
+ Waveform {
+ period_length_ns: wf.period_length_ns,
+ duty_length_ns: wf.duty_length_ns,
+ duty_offset_ns: wf.duty_offset_ns,
+ }
+ }
+}
+
+impl From<Waveform> for bindings::pwm_waveform {
+ fn from(wf: Waveform) -> Self {
+ bindings::pwm_waveform {
+ period_length_ns: wf.period_length_ns,
+ duty_length_ns: wf.duty_length_ns,
+ duty_offset_ns: wf.duty_offset_ns,
+ }
+ }
+}
+
+/// Wrapper for board-dependent PWM arguments (`struct pwm_args`).
+#[repr(transparent)]
+pub struct Args(Opaque<bindings::pwm_args>);
+
+impl Args {
+ /// Creates an `Args` wrapper from a C struct pointer.
+ ///
+ /// # Safety
+ /// The caller must ensure that `c_args_ptr` is a valid, non-null pointer
+ /// to `bindings::pwm_args` and that the pointed-to data is valid
+ /// for the duration of this function call (as data is copied).
+ unsafe fn from_c_ptr(c_args_ptr: *const bindings::pwm_args) -> Self {
+ // SAFETY: Caller guarantees `c_args_ptr` is valid. We dereference it to copy.
+ Args(Opaque::new(unsafe { *c_args_ptr }))
+ }
+
+ /// Returns the period of the PWM signal in nanoseconds.
+ pub fn period(&self) -> u64 {
+ // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
+ // managed by the `Opaque` wrapper. This pointer is guaranteed to be
+ // valid and aligned for the lifetime of `self` because `Opaque` owns a copy.
+ unsafe { (*self.0.get()).period }
+ }
+
+ /// Returns the polarity of the PWM signal.
+ pub fn polarity(&self) -> Polarity {
+ // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
+ // managed by the `Opaque` wrapper. This pointer is guaranteed to be
+ // valid and aligned for the lifetime of `self`.
+ let raw_polarity = unsafe { (*self.0.get()).polarity };
+ Polarity::from(raw_polarity)
+ }
+}
+
+/// Wrapper for PWM state (`struct pwm_state`).
+#[repr(transparent)]
+pub struct State(bindings::pwm_state);
+
+impl Default for State {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl State {
+ /// Creates a new zeroed `State`.
+ pub fn new() -> Self {
+ State(bindings::pwm_state::default())
+ }
+
+ /// Creates a `State` wrapper by taking ownership of a C `pwm_state` value.
+ pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
+ State(c_state)
+ }
+
+ /// Gets the period of the PWM signal in nanoseconds.
+ pub fn period(&self) -> u64 {
+ self.0.period
+ }
+
+ /// Sets the period of the PWM signal in nanoseconds.
+ pub fn set_period(&mut self, period_ns: u64) {
+ self.0.period = period_ns;
+ }
+
+ /// Gets the duty cycle of the PWM signal in nanoseconds.
+ pub fn duty_cycle(&self) -> u64 {
+ self.0.duty_cycle
+ }
+
+ /// Sets the duty cycle of the PWM signal in nanoseconds.
+ pub fn set_duty_cycle(&mut self, duty_ns: u64) {
+ self.0.duty_cycle = duty_ns;
+ }
+
+ /// Returns `true` if the PWM signal is enabled.
+ pub fn enabled(&self) -> bool {
+ self.0.enabled
+ }
+
+ /// Sets the enabled state of the PWM signal.
+ pub fn set_enabled(&mut self, enabled: bool) {
+ self.0.enabled = enabled;
+ }
+
+ /// Gets the polarity of the PWM signal.
+ pub fn polarity(&self) -> Polarity {
+ Polarity::from(self.0.polarity)
+ }
+
+ /// Sets the polarity of the PWM signal.
+ pub fn set_polarity(&mut self, polarity: Polarity) {
+ self.0.polarity = polarity.into();
+ }
+
+ /// Returns `true` if the PWM signal is configured for power usage hint.
+ pub fn usage_power(&self) -> bool {
+ self.0.usage_power
+ }
+
+ /// Sets the power usage hint for the PWM signal.
+ pub fn set_usage_power(&mut self, usage_power: bool) {
+ self.0.usage_power = usage_power;
+ }
+}
+
+/// Wrapper for a PWM device/channel (`struct pwm_device`).
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::pwm_device>);
+
+impl Device {
+ /// Creates a temporary `&mut Device` from a raw C pointer for use in callbacks.
+ ///
+ /// It returns a mutable reference (`&mut Self`) because the underlying C APIs
+ /// for PWM operations use non-const pointers (`struct pwm_device *`). This
+ /// signals that the functions in the vtable are permitted to mutate the
+ /// device's state (e.g., by writing to hardware registers). Using `&mut`
+ /// allows the `PwmOps` trait to accurately model this behavior and leverage
+ /// Rust's aliasing rules for greater safety.
+ ///
+ /// # Safety
+ /// The caller must ensure that `ptr` is a valid, non-null pointer to
+ /// `bindings::pwm_device` that is properly initialized.
+ /// The `pwm_device` must remain valid for the lifetime `'a`.
+ /// The caller must also ensure that Rust's aliasing rules are upheld.
+ pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_device) -> &'a mut Self {
+ // SAFETY: Caller guarantees `ptr` is valid and meets lifetime/aliasing.
+ // `Self` is `#[repr(transparent)]`, so casting is valid.
+ unsafe { &mut *ptr.cast::<Self>() }
+ }
+
+ /// Returns a raw pointer to the underlying `pwm_device`.
+ fn as_raw(&self) -> *mut bindings::pwm_device {
+ self.0.get()
+ }
+
+ /// Gets the hardware PWM index for this device within its chip.
+ pub fn hwpwm(&self) -> u32 {
+ // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime.
+ unsafe { (*self.as_raw()).hwpwm }
+ }
+
+ /// Gets a reference to the parent `Chip` that this device belongs to.
+ pub fn chip(&self) -> &Chip {
+ // SAFETY: `self.as_raw()` provides a valid pointer. (*self.as_raw()).chip
+ // is assumed to be a valid pointer to `pwm_chip` managed by the kernel.
+ // Chip::from_ptr's safety conditions must be met.
+ unsafe { Chip::from_ptr((*self.as_raw()).chip) }
+ }
+
+ /// Gets the label for this PWM device, if any.
+ pub fn label(&self) -> Option<&CStr> {
+ // SAFETY: self.as_raw() provides a valid pointer.
+ let label_ptr = unsafe { (*self.as_raw()).label };
+ if label_ptr.is_null() {
+ None
+ } else {
+ // SAFETY: label_ptr is non-null and points to a C string
+ // managed by the kernel, valid for the lifetime of the PWM device.
+ Some(unsafe { CStr::from_char_ptr(label_ptr) })
+ }
+ }
+
+ /// Gets a copy of the board-dependent arguments for this PWM device.
+ pub fn args(&self) -> Args {
+ // SAFETY: self.as_raw() gives a valid pointer to `pwm_device`.
+ // The `args` field is a valid `pwm_args` struct embedded within `pwm_device`.
+ // `Args::from_c_ptr`'s safety conditions are met by providing this pointer.
+ unsafe { Args::from_c_ptr(&(*self.as_raw()).args) }
+ }
+
+ /// Gets a copy of the current state of this PWM device.
+ pub fn state(&self) -> State {
+ // SAFETY: `self.as_raw()` gives a valid pointer. `(*self.as_raw()).state`
+ // is a valid `pwm_state` struct. `State::from_c` copies this data.
+ State::from_c(unsafe { (*self.as_raw()).state })
+ }
+
+ /// Returns `true` if the PWM signal is currently enabled based on its state.
+ pub fn is_enabled(&self) -> bool {
+ self.state().enabled()
+ }
+}
+
+/// Wrapper for a PWM chip/controller (`struct pwm_chip`).
+#[repr(transparent)]
+pub struct Chip(Opaque<bindings::pwm_chip>);
+
+impl Chip {
+ /// Creates a temporary `&mut Chip` from a raw C pointer for use in callbacks.
+ ///
+ /// It returns a mutable reference (`&mut Self`) because the underlying C APIs
+ /// for PWM operations use non-const pointers (`struct pwm_chip *`). This
+ /// signals that the functions in the vtable are permitted to mutate the
+ /// chip's state (e.g., by calling `set_drvdata` or through operations that
+ /// modify hardware registers). Using `&mut` is essential for these cases.
+ ///
+ /// # Safety
+ /// The caller must ensure that `ptr` is a valid, non-null pointer to
+ /// `bindings::pwm_chip` that is properly initialized.
+ /// The `pwm_chip` must remain valid for the lifetime `'a`.
+ /// The caller must also ensure that Rust's aliasing rules are upheld.
+ pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_chip) -> &'a mut Self {
+ // SAFETY: Caller guarantees `ptr` is valid and meets lifetime/aliasing.
+ // `Self` is `#[repr(transparent)]`, so casting is valid.
+ unsafe { &mut *ptr.cast::<Self>() }
+ }
+
+ /// Returns a raw pointer to the underlying `pwm_chip`.
+ pub(crate) fn as_raw(&self) -> *mut bindings::pwm_chip {
+ self.0.get()
+ }
+
+ /// Gets the number of PWM channels (hardware PWMs) on this chip.
+ pub fn npwm(&self) -> u32 {
+ // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime.
+ unsafe { (*self.as_raw()).npwm }
+ }
+
+ /// Returns `true` if the chip supports atomic operations for configuration.
+ pub fn is_atomic(&self) -> bool {
+ // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime.
+ unsafe { (*self.as_raw()).atomic }
+ }
+
+ /// Returns a reference to the embedded `struct device` abstraction.
+ pub fn device(&self) -> &device::Device {
+ // SAFETY: `self.as_raw()` provides a valid pointer to `bindings::pwm_chip`.
+ // The `dev` field is an instance of `bindings::device` embedded within `pwm_chip`.
+ // Taking a pointer to this embedded field is valid.
+ // `device::Device` is `#[repr(transparent)]`.
+ // The lifetime of the returned reference is tied to `self`.
+ let dev_field_ptr = unsafe { core::ptr::addr_of!((*self.as_raw()).dev) };
+ // SAFETY: `dev_field_ptr` is a valid pointer to `bindings::device`.
+ // Casting and dereferencing is safe due to `repr(transparent)` and lifetime.
+ unsafe { &*(dev_field_ptr.cast::<device::Device>()) }
+ }
+
+ /// Returns a reference to the parent device of this PWM chip's device.
+ pub fn parent_device(&self) -> Option<&device::Device> {
+ self.device().parent()
+ }
+
+ /// Gets the *typed* driver-specific data associated with this chip's embedded device.
+ pub fn drvdata<T: 'static>(&self) -> Option<&T> {
+ // SAFETY: `self.as_raw()` gives a valid pwm_chip pointer.
+ // `bindings::pwmchip_get_drvdata` is the C function to retrieve driver data.
+ let ptr = unsafe { bindings::pwmchip_get_drvdata(self.as_raw()) };
+ if ptr.is_null() {
+ None
+ } else {
+ // SAFETY: `ptr` is non-null. Caller ensures `T` is the correct type.
+ // Lifetime of data is managed by the driver that set it.
+ unsafe { Some(&*(ptr.cast::<T>())) }
+ }
+ }
+
+ /// Sets the *typed* driver-specific data associated with this chip's embedded device.
+ pub fn set_drvdata<T: 'static + ForeignOwnable>(&self, data: T) {
+ // SAFETY: `self.as_raw()` gives a valid pwm_chip pointer.
+ // `bindings::pwmchip_set_drvdata` is the C function to set driver data.
+ // `data.into_foreign()` provides a valid `*mut c_void`.
+ unsafe { bindings::pwmchip_set_drvdata(self.as_raw(), data.into_foreign().cast()) }
+ }
+
+ /// Allocates and wraps a PWM chip using `bindings::pwmchip_alloc`.
+ ///
+ /// Returns an `ARef<Chip>` managing the chip's lifetime via refcounting
+ /// on its embedded `struct device`.
+ pub fn new(parent_dev: &device::Device, npwm: u32, sizeof_priv: usize) -> Result<ARef<Self>> {
+ // SAFETY: `parent_device_for_dev_field.as_raw()` is valid.
+ // `bindings::pwmchip_alloc` returns a valid `*mut bindings::pwm_chip` (refcount 1)
+ // or an ERR_PTR.
+ let c_chip_ptr_raw =
+ unsafe { bindings::pwmchip_alloc(parent_dev.as_raw(), npwm, sizeof_priv) };
+
+ let c_chip_ptr: *mut bindings::pwm_chip = error::from_err_ptr(c_chip_ptr_raw)?;
+
+ // Cast the `*mut bindings::pwm_chip` to `*mut Chip`. This is valid because
+ // `Chip` is `repr(transparent)` over `Opaque<bindings::pwm_chip>`, and
+ // `Opaque<T>` is `repr(transparent)` over `T`.
+ let chip_ptr_as_self = c_chip_ptr.cast::<Self>();
+
+ // SAFETY: `chip_ptr_as_self` points to a valid `Chip` (layout-compatible with
+ // `bindings::pwm_chip`) whose embedded device has refcount 1.
+ // `ARef::from_raw` takes this pointer and manages it via `AlwaysRefCounted`.
+ Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(chip_ptr_as_self)) })
+ }
+}
+
+// SAFETY: Implements refcounting for `Chip` using the embedded `struct device`.
+unsafe impl AlwaysRefCounted for Chip {
+ #[inline]
+ fn inc_ref(&self) {
+ // SAFETY: `self.0.get()` points to a valid `pwm_chip` because `self` exists.
+ // The embedded `dev` is valid. `get_device` increments its refcount.
+ unsafe {
+ bindings::get_device(core::ptr::addr_of_mut!((*self.0.get()).dev));
+ }
+ }
+
+ #[inline]
+ unsafe fn dec_ref(obj: NonNull<Chip>) {
+ let c_chip_ptr = obj.cast::<bindings::pwm_chip>().as_ptr();
+
+ // SAFETY: `obj` is a valid pointer to a `Chip` (and thus `bindings::pwm_chip`)
+ // with a non-zero refcount. `put_device` handles decrement and final release.
+ unsafe {
+ bindings::put_device(core::ptr::addr_of_mut!((*c_chip_ptr).dev));
+ }
+ }
+}
+
+// SAFETY: `Chip` is a wrapper around `*mut bindings::pwm_chip`. The underlying C
+// structure's state is managed and synchronized by the kernel's device model
+// and PWM core locking mechanisms. Therefore, it is safe to move the `Chip`
+// wrapper (and the pointer it contains) across threads.
+unsafe impl Send for Chip {}
+
+// SAFETY: It is safe for multiple threads to have shared access (`&Chip`) because
+// the `Chip` data is immutable from the Rust side without holding the appropriate
+// kernel locks, which the C core is responsible for. Any interior mutability is
+// handled and synchronized by the C kernel code.
+unsafe impl Sync for Chip {}
+
+/// Manages the registration of a PWM chip, ensuring `pwmchip_remove` is called on drop.
+pub struct Registration {
+ chip: ManuallyDrop<ARef<Chip>>,
+}
+
+impl Registration {
+ /// Registers a PWM chip (obtained via `Chip::new`) with the PWM subsystem.
+ ///
+ /// Takes an `ARef<Chip>`. On `Drop` of the returned `Registration` object,
+ /// `pwmchip_remove` is called for the chip.
+ pub fn new(chip: ARef<Chip>, ops_vtable: &'static PwmOpsVTable) -> Result<Self> {
+ // Get the raw C pointer from ARef<Chip>.
+ let c_chip_ptr = chip.as_raw().cast::<bindings::pwm_chip>();
+
+ // SAFETY: `c_chip_ptr` is valid (guaranteed by ARef existing).
+ // `ops_vtable.as_raw()` provides a valid `*const bindings::pwm_ops`.
+ // `bindings::__pwmchip_add` preconditions (valid pointers, ops set on chip) are met.
+ unsafe {
+ (*c_chip_ptr).ops = ops_vtable.as_raw();
+ to_result(bindings::__pwmchip_add(c_chip_ptr, core::ptr::null_mut()))?;
+ }
+ Ok(Registration {
+ chip: ManuallyDrop::new(chip),
+ })
+ }
+}
+
+impl Drop for Registration {
+ fn drop(&mut self) {
+ let chip = &**self.chip;
+ let chip_raw: *mut bindings::pwm_chip = chip.as_raw();
+
+ // SAFETY: `chip_raw` points to a chip that was successfully registered via `Self::new`.
+ // `bindings::pwmchip_remove` is the correct C function to unregister it.
+ unsafe {
+ bindings::pwmchip_remove(chip_raw);
+ ManuallyDrop::drop(&mut self.chip); // Drops the ARef<Chip>
+ }
+ }
+}
+
+/// Trait defining the operations for a PWM driver.
+pub trait PwmOps: 'static + Sized {
+ /// The driver-specific hardware representation of a waveform.
+ /// This type must be `Copy`, `Default`, and fit within `WFHW_MAX_SIZE`.
+ type WfHw: Copy + Default;
+
+ /// Optional hook to atomically apply a new PWM config.
+ fn apply(
+ _chip: &mut Chip,
+ _pwm: &mut Device,
+ _state: &State,
+ _parent_dev: &device::Device<Bound>,
+ ) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Optional hook for when a PWM device is requested.
+ fn request(_chip: &mut Chip, _pwm: &mut Device, _parent_dev: &device::Device<Bound>) -> Result {
+ Ok(())
+ }
+
+ /// Optional hook for when a PWM device is freed.
+ fn free(_chip: &mut Chip, _pwm: &mut Device, _parent_dev: &device::Device<Bound>) {}
+
+ /// Optional hook for capturing a PWM signal.
+ fn capture(
+ _chip: &mut Chip,
+ _pwm: &mut Device,
+ _result: &mut bindings::pwm_capture,
+ _timeout: usize,
+ _parent_dev: &device::Device<Bound>,
+ ) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Optional hook to get the current hardware state.
+ fn get_state(
+ _chip: &mut Chip,
+ _pwm: &mut Device,
+ _state: &mut State,
+ _parent_dev: &device::Device<Bound>,
+ ) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Convert a generic waveform to the hardware-specific representation.
+ /// This is typically a pure calculation and does not perform I/O.
+ fn round_waveform_tohw(
+ _chip: &mut Chip,
+ _pwm: &mut Device,
+ _wf: &Waveform,
+ ) -> Result<(i32, Self::WfHw)> {
+ Err(ENOTSUPP)
+ }
+
+ /// Convert a hardware-specific representation back to a generic waveform.
+ /// This is typically a pure calculation and does not perform I/O.
+ fn round_waveform_fromhw(
+ _chip: &mut Chip,
+ _pwm: &Device,
+ _wfhw: &Self::WfHw,
+ _wf: &mut Waveform,
+ ) -> Result<i32> {
+ Err(ENOTSUPP)
+ }
+
+ /// Read the current hardware configuration into the hardware-specific representation.
+ fn read_waveform(
+ _chip: &mut Chip,
+ _pwm: &mut Device,
+ _parent_dev: &device::Device<Bound>,
+ ) -> Result<Self::WfHw> {
+ Err(ENOTSUPP)
+ }
+
+ /// Write a hardware-specific waveform configuration to the hardware.
+ fn write_waveform(
+ _chip: &mut Chip,
+ _pwm: &mut Device,
+ _wfhw: &Self::WfHw,
+ _parent_dev: &device::Device<Bound>,
+ ) -> Result {
+ Err(ENOTSUPP)
+ }
+}
+/// Bridges Rust `PwmOps` to the C `pwm_ops` vtable.
+struct Adapter<T: PwmOps> {
+ _p: PhantomData<T>,
+}
+
+impl<T: PwmOps> Adapter<T> {
+ /// # Safety
+ /// `wfhw_ptr` must be valid for writes of `size_of::<T::WfHw>()` bytes.
+ unsafe fn serialize_wfhw(wfhw: &T::WfHw, wfhw_ptr: *mut core::ffi::c_void) -> Result {
+ let size = core::mem::size_of::<T::WfHw>();
+ if size > WFHW_MAX_SIZE {
+ return Err(EINVAL);
+ }
+
+ // SAFETY: The caller ensures `wfhw_ptr` is valid for `size` bytes.
+ unsafe {
+ core::ptr::copy_nonoverlapping(wfhw as *const _ as *const u8, wfhw_ptr.cast(), size);
+ }
+
+ Ok(())
+ }
+
+ /// # Safety
+ /// `wfhw_ptr` must be valid for reads of `size_of::<T::WfHw>()` bytes.
+ unsafe fn deserialize_wfhw(wfhw_ptr: *const core::ffi::c_void) -> Result<T::WfHw> {
+ let size = core::mem::size_of::<T::WfHw>();
+ if size > WFHW_MAX_SIZE {
+ return Err(EINVAL);
+ }
+
+ let mut wfhw = T::WfHw::default();
+ // SAFETY: The caller ensures `wfhw_ptr` is valid for `size` bytes.
+ unsafe {
+ core::ptr::copy_nonoverlapping(wfhw_ptr.cast(), &mut wfhw as *mut _ as *mut u8, size);
+ }
+
+ Ok(wfhw)
+ }
+
+ /// # Safety
+ /// C-callback. Pointers from C must be valid.
+ unsafe extern "C" fn apply_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ s: *const bindings::pwm_state,
+ ) -> i32 {
+ // SAFETY: This block relies on the function's safety contract: the C caller
+ // provides valid pointers. `Chip::from_ptr` and `Device::from_ptr` are `unsafe fn`
+ // whose preconditions are met by this contract.
+ let (chip, pwm) = unsafe { (Chip::from_ptr(c), Device::from_ptr(p)) };
+ let parent_dev = match chip.parent_device() {
+ Some(dev) => dev,
+ None => {
+ return EINVAL.to_errno();
+ }
+ };
+
+ // SAFETY: The PWM core guarantees callbacks only happen on a live, bound device.
+ let bound_parent =
+ unsafe { &*(parent_dev as *const device::Device as *const device::Device<Bound>) };
+
+ // SAFETY: The state provided by the callback is guaranteed to be valid
+ let state = State::from_c(unsafe { *s });
+ match T::apply(chip, pwm, &state, bound_parent) {
+ Ok(()) => 0,
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ /// C-callback. Pointers from C must be valid.
+ unsafe extern "C" fn request_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ ) -> i32 {
+ // SAFETY: PWM core guarentees `c` and `p` are valid pointers.
+ let (chip, pwm) = unsafe { (Chip::from_ptr(c), Device::from_ptr(p)) };
+ let parent_dev = match chip.parent_device() {
+ Some(dev) => dev,
+ None => {
+ return EINVAL.to_errno();
+ }
+ };
+
+ let bound_parent =
+ // SAFETY: The PWM core guarantees the device is bound during callbacks.
+ unsafe { &*(parent_dev as *const device::Device as *const device::Device<Bound>) };
+ match T::request(chip, pwm, bound_parent) {
+ Ok(()) => 0,
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ /// C-callback. Pointers from C must be valid.
+ unsafe extern "C" fn free_callback(c: *mut bindings::pwm_chip, p: *mut bindings::pwm_device) {
+ // SAFETY: Relies on the function's contract that `c` and `p` are valid pointers.
+ let (chip, pwm) = unsafe { (Chip::from_ptr(c), Device::from_ptr(p)) };
+ let parent_dev = match chip.parent_device() {
+ Some(dev) => dev,
+ None => {
+ return;
+ }
+ };
+
+ let bound_parent =
+ // SAFETY: The PWM core guarantees the device is bound during callbacks.
+ unsafe { &*(parent_dev as *const device::Device as *const device::Device<Bound>) };
+ T::free(chip, pwm, bound_parent);
+ }
+
+ /// # Safety
+ /// C-callback. Pointers from C must be valid.
+ unsafe extern "C" fn capture_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ res: *mut bindings::pwm_capture,
+ timeout: usize,
+ ) -> i32 {
+ // SAFETY: Relies on the function's contract that `c` and `p` are valid pointers.
+ let (chip, pwm, result) = unsafe { (Chip::from_ptr(c), Device::from_ptr(p), &mut *res) };
+ let parent_dev = match chip.parent_device() {
+ Some(dev) => dev,
+ None => {
+ return EINVAL.to_errno();
+ }
+ };
+
+ let bound_parent =
+ // SAFETY: The PWM core guarantees the device is bound during callbacks.
+ unsafe { &*(parent_dev as *const device::Device as *const device::Device<Bound>) };
+ match T::capture(chip, pwm, result, timeout, bound_parent) {
+ Ok(()) => 0,
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ /// C-callback. Pointers from C must be valid.
+ unsafe extern "C" fn get_state_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ s: *mut bindings::pwm_state,
+ ) -> i32 {
+ // SAFETY: Relies on the function's contract that `c` and `p` are valid pointers.
+ let (chip, pwm) = unsafe { (Chip::from_ptr(c), Device::from_ptr(p)) };
+ let parent_dev = match chip.parent_device() {
+ Some(dev) => dev,
+ None => {
+ return EINVAL.to_errno();
+ }
+ };
+ let bound_parent =
+ // SAFETY: The PWM core guarantees the device is bound during callbacks.
+ unsafe { &*(parent_dev as *const device::Device as *const device::Device<Bound>) };
+ let mut rust_state = State::new();
+ match T::get_state(chip, pwm, &mut rust_state, bound_parent) {
+ Ok(()) => {
+ // SAFETY: `s` is guaranteed valid by the C caller.
+ unsafe {
+ *s = rust_state.0;
+ };
+ 0
+ }
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ /// C-callback. Pointers from C must be valid.
+ unsafe extern "C" fn round_waveform_tohw_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ w: *const bindings::pwm_waveform,
+ wh: *mut core::ffi::c_void,
+ ) -> i32 {
+ // SAFETY: Relies on the function's contract that `c` and `p` are valid pointers.
+ let (chip, pwm, wf) =
+ unsafe { (Chip::from_ptr(c), Device::from_ptr(p), Waveform::from(*w)) };
+ match T::round_waveform_tohw(chip, pwm, &wf) {
+ Ok((status, wfhw)) => {
+ // SAFETY: `wh` is valid per this function's safety contract.
+ if unsafe { Self::serialize_wfhw(&wfhw, wh) }.is_err() {
+ return EINVAL.to_errno();
+ }
+ status
+ }
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ /// C-callback. Pointers from C must be valid.
+ unsafe extern "C" fn round_waveform_fromhw_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ wh: *const core::ffi::c_void,
+ w: *mut bindings::pwm_waveform,
+ ) -> i32 {
+ // SAFETY: Relies on the function's contract that `c` and `p` are valid pointers.
+ let (chip, pwm) = unsafe { (Chip::from_ptr(c), Device::from_ptr(p)) };
+ // SAFETY: `deserialize_wfhw`'s safety contract is met by this function's contract.
+ let wfhw = match unsafe { Self::deserialize_wfhw(wh) } {
+ Ok(v) => v,
+ Err(e) => return e.to_errno(),
+ };
+
+ let mut rust_wf = Waveform::default();
+ match T::round_waveform_fromhw(chip, pwm, &wfhw, &mut rust_wf) {
+ Ok(ret) => {
+ // SAFETY: `w` is guaranteed valid by the C caller.
+ unsafe {
+ *w = rust_wf.into();
+ };
+ ret
+ }
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ /// C-callback. Pointers from C must be valid.
+ unsafe extern "C" fn read_waveform_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ wh: *mut core::ffi::c_void,
+ ) -> i32 {
+ // SAFETY: Relies on the function's contract that `c` and `p` are valid pointers.
+ let (chip, pwm) = unsafe { (Chip::from_ptr(c), Device::from_ptr(p)) };
+ let parent_dev = match chip.parent_device() {
+ Some(dev) => dev,
+ None => {
+ return EINVAL.to_errno();
+ }
+ };
+
+ let bound_parent =
+ // SAFETY: The PWM core guarantees the device is bound during callbacks.
+ unsafe { &*(parent_dev as *const device::Device as *const device::Device<Bound>) };
+ match T::read_waveform(chip, pwm, bound_parent) {
+ // SAFETY: `wh` is valid per this function's safety contract.
+ Ok(wfhw) => match unsafe { Self::serialize_wfhw(&wfhw, wh) } {
+ Ok(()) => 0,
+ Err(e) => e.to_errno(),
+ },
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ /// C-callback. Pointers from C must be valid.
+ unsafe extern "C" fn write_waveform_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ wh: *const core::ffi::c_void,
+ ) -> i32 {
+ // SAFETY: Relies on the function's contract that `c` and `p` are valid pointers.
+ let (chip, pwm) = unsafe { (Chip::from_ptr(c), Device::from_ptr(p)) };
+ let parent_dev = match chip.parent_device() {
+ Some(dev) => dev,
+ None => {
+ return EINVAL.to_errno();
+ }
+ };
+
+ let bound_parent =
+ // SAFETY: The PWM core guarantees the device is bound during callbacks.
+ unsafe { &*(parent_dev as *const device::Device as *const device::Device<Bound>) };
+ // SAFETY: `wh` is valid per this function's safety contract.
+ let wfhw = match unsafe { Self::deserialize_wfhw(wh) } {
+ Ok(v) => v,
+ Err(e) => return e.to_errno(),
+ };
+ match T::write_waveform(chip, pwm, &wfhw, bound_parent) {
+ Ok(()) => 0,
+ Err(e) => e.to_errno(),
+ }
+ }
+}
+/// VTable structure wrapper for PWM operations. Mirrors `struct pwm_ops`.
+#[repr(transparent)]
+pub struct PwmOpsVTable(Opaque<bindings::pwm_ops>);
+
+// SAFETY: PwmOpsVTable is Send. The vtable contains only function pointers
+// and a size, which are simple data types that can be safely moved across
+// threads. The thread-safety of calling these functions is handled by the
+// kernel's locking mechanisms.
+unsafe impl Send for PwmOpsVTable {}
+// SAFETY: PwmOpsVTable is Sync. The vtable is immutable after it is created,
+// so it can be safely referenced and accessed concurrently by multiple threads
+// e.g. to read the function pointers.
+unsafe impl Sync for PwmOpsVTable {}
+
+impl PwmOpsVTable {
+ /// Returns a raw pointer to the underlying `pwm_ops` struct.
+ pub(crate) fn as_raw(&self) -> *const bindings::pwm_ops {
+ self.0.get()
+ }
+}
+
+/// Creates a PWM operations vtable for a type `T` that implements `PwmOps`.
+///
+/// This is used to bridge Rust trait implementations to the C `struct pwm_ops`
+/// expected by the kernel.
+pub const fn create_pwm_ops<T: PwmOps>() -> PwmOpsVTable {
+ // SAFETY: `core::mem::zeroed()` is unsafe. For `pwm_ops`, all fields are
+ // `Option<extern "C" fn(...)>` or data, so a zeroed pattern (None/0) is valid initially.
+ let mut ops: bindings::pwm_ops = unsafe { core::mem::zeroed() };
+
+ ops.apply = Some(Adapter::<T>::apply_callback);
+ ops.request = Some(Adapter::<T>::request_callback);
+ ops.free = Some(Adapter::<T>::free_callback);
+ ops.capture = Some(Adapter::<T>::capture_callback);
+ ops.get_state = Some(Adapter::<T>::get_state_callback);
+
+ ops.round_waveform_tohw = Some(Adapter::<T>::round_waveform_tohw_callback);
+ ops.round_waveform_fromhw = Some(Adapter::<T>::round_waveform_fromhw_callback);
+ ops.read_waveform = Some(Adapter::<T>::read_waveform_callback);
+ ops.write_waveform = Some(Adapter::<T>::write_waveform_callback);
+ ops.sizeof_wfhw = core::mem::size_of::<T::WfHw>();
+
+ PwmOpsVTable(Opaque::new(ops))
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 1/7] rust: Add basic PWM abstractions
2025-06-10 12:52 ` [PATCH v2 1/7] rust: Add basic PWM abstractions Michal Wilczynski
@ 2025-06-11 13:35 ` Miguel Ojeda
2025-06-12 9:12 ` Benno Lossin
1 sibling, 0 replies; 27+ messages in thread
From: Miguel Ojeda @ 2025-06-11 13:35 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Alexandre Ghiti, Marek Szyprowski,
Benno Lossin, Michael Turquette, Stephen Boyd, linux-kernel,
linux-pwm, rust-for-linux, linux-riscv, devicetree, linux-clk
Hi Michal,
Some docs-only/nits quick review for future versions and other
patches. Some may apply multiple times.
On Tue, Jun 10, 2025 at 2:54 PM Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
> +//! This module provides safe Rust abstractions for working with the Linux
> +//! kernel's PWM subsystem, leveraging types generated by `bindgen`
> +//! from `<linux/pwm.h>` and `drivers/pwm/core.c`.
The second part we typically do with a "C header" reference to
`srctree/...` which will get rendered as clickable link to the file
(please check other files to see how it is usually done).
I would also simplify, e.g. typically abstractions try to be safe, and
the bindings typically come from `bindgen`, so I would just say e.g.
//! PWM subsystem abstractions.
//!
//! C header: ...
What types are you using from `drivers/pwm/core.c`, by the way?
> +use crate::{
> + bindings,
> + device::{self, Bound},
> + error::{self, to_result, Result},
> + prelude::*,
> + str::CStr,
> + types::{ARef, AlwaysRefCounted, ForeignOwnable, Opaque},
> +};
At least a couple of these already come from the prelude.
> +/// Maximum size for the hardware-specific waveform representation buffer.
> +/// From C: #define WFHWSIZE 20
This would be rendered as a single paragraph, so I would split it into
two by adding a newline in between.
In addition, please use code spans (i.e. backquotes) where possible.
> +/// PWM polarity. Mirrors `enum pwm_polarity`.
We don't link consistently C types, but if you wanted, you could link
them (either `srctree/` link to a file or a docs.kernel.org to a
concrete C item if it is rendered there -- either is fine).
(Eventually we want to have an automatic way to do so, similar to
intra-doc links)
> + /// Normal polarity (duty cycle defines the high period of the signal)
Please end sentences/docs with a period.
> + Normal,
> + /// Inversed polarity (duty cycle defines the low period of the signal)
I suggest a newline between these.
> +impl From<bindings::pwm_polarity> for Polarity {
> + fn from(polarity: bindings::pwm_polarity) -> Self {
> + match polarity {
> + bindings::pwm_polarity_PWM_POLARITY_NORMAL => Polarity::Normal,
> + bindings::pwm_polarity_PWM_POLARITY_INVERSED => Polarity::Inversed,
> + _ => Polarity::Normal,
> + }
> + }
> +}
Should this be fallible? i.e. should the default case be an error / is
the C enum expected to have any other value ?
(I have no context, so this may be all expected, of course)
> +/// Represents a PWM waveform configuration. Mirrors struct pwm_waveform.
Code span.
> + /// Duration the PWM signal is in its "active" state during one period,
> + /// in nanoseconds. For a typical "normal" polarity configuration where active is high,
> + /// this represents the high time of the signal.
> + pub duty_length_ns: u64,
The first paragraph of an item is the "short description", i.e. it
acts as a title when it gets rendered in item lists. So it should be
short if possible. For instance, here I would add a new paragraph
between the two sentences.
(Ditto for other cases).
> + /// This type must be `Copy`, `Default`, and fit within `WFHW_MAX_SIZE`.
Please use intra-doc links wherever possible, e.g. [`Copy`] and
[`WFHW_MAX_SIZE`] (assuming they work).
> + /// # Safety
> + /// C-callback. Pointers from C must be valid.
Please add a newline between these to match the usual style. Also,
"C-callback" is not a precondition, so I would move it outside the
safety section (above), unless you want to restrict C to be the only
one calling this or things like that (in which case I would clarify).
> + unsafe extern "C" fn read_waveform_callback(
> + c: *mut bindings::pwm_chip,
> + p: *mut bindings::pwm_device,
> + wh: *mut core::ffi::c_void,
Please avoid `core::ffi::` -- nowadays you should be able to just
write e.g. `c_void`, and that will get you the `kernel::ffi::` one
(https://docs.kernel.org/rust/coding-guidelines.html#c-ffi-types).
> + ) -> i32 {
Unless the C side uses an explicitly `s32`, which as far as I can see
it doesn't in this case, please use `c_int` instead (i.e. please match
the C signatures in callbacks, even if they happen to resolve to that
type).
Finally, in another patch I noticed the `author` key -- we are trying
to move to the `authors` (plural) one, so please use that one.
Thanks!
Cheers,
Miguel
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 1/7] rust: Add basic PWM abstractions
2025-06-10 12:52 ` [PATCH v2 1/7] rust: Add basic PWM abstractions Michal Wilczynski
2025-06-11 13:35 ` Miguel Ojeda
@ 2025-06-12 9:12 ` Benno Lossin
1 sibling, 0 replies; 27+ messages in thread
From: Benno Lossin @ 2025-06-12 9:12 UTC (permalink / raw)
To: Michal Wilczynski, Uwe Kleine-König, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
Drew Fustini, Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, Marek Szyprowski, Michael Turquette,
Stephen Boyd
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
On Tue Jun 10, 2025 at 2:52 PM CEST, Michal Wilczynski wrote:
> Introduce safe Rust abstractions for the Linux PWM subsystem. These
> abstractions provide ergonomic, lifetime managed wrappers around the
> core C data structures and functions, enabling the development of PWM
> chip drivers in safe Rust.
>
> This initial version provides the core building blocks for writing a PWM
> chip provider driver, with a focus on safety, resource management, and
> idiomatic Rust patterns.
>
> The main components are:
>
> Ownership and Lifetime Management:
> - The pwm::Chip type, an ARef managed wrapper for struct pwm_chip,
> correctly handles the object's lifetime by using the embedded struct
> device's reference counter.
> - A pwm::Registration RAII guard ensures that a call to register a
> chip (pwmchip_add) is always paired with a call to unregister it
> (pwmchip_remove), preventing resource leaks.
>
> Safe Type Wrappers:
> - Safe, idiomatic Rust types (Polarity, Waveform, State, Args,
> Device) are provided to abstract away the raw C structs and enums.
> The State wrapper holds its data by value, avoiding unnecessary
> heap allocations.
>
> Driver Operations (PwmOps):
> - A generic PwmOps trait allows drivers to implement the standard
> PWM operations. It uses an associated type (WfHw) for the driver's
> hardware specific waveform data, moving unsafe serialization logic into
> the abstraction layer.
> The trait exposes the modern waveform API (round_waveform_tohw,
> write_waveform, etc.) as well as the other standard kernel callbacks
> (get_state, request, apply).
> - A create_pwm_ops function generates a C-compatible vtable from a
> PwmOps implementor.
>
> This foundational layer is designed to be used by subsequent patches to
> implement specific PWM chip drivers in Rust.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> MAINTAINERS | 6 +
> drivers/pwm/Kconfig | 13 +
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers/helpers.c | 1 +
> rust/helpers/pwm.c | 20 +
> rust/kernel/lib.rs | 2 +
> rust/kernel/pwm.rs | 864 ++++++++++++++++++++++++++++++++++++++++
> 7 files changed, 907 insertions(+)
Do you mind splitting this into smaller commits to make review easier?
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 2/7] pwm: Add Rust driver for T-HEAD TH1520 SoC
2025-06-10 12:52 ` [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
2025-06-10 12:52 ` [PATCH v2 1/7] rust: Add basic PWM abstractions Michal Wilczynski
@ 2025-06-10 12:52 ` Michal Wilczynski
2025-06-11 6:58 ` Uwe Kleine-König
2025-06-10 12:52 ` [PATCH v2 3/7] clk: thead: Mark essential bus clocks as CLK_IGNORE_UNUSED Michal Wilczynski
` (5 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Michal Wilczynski @ 2025-06-10 12:52 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
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@samsung.com>
---
MAINTAINERS | 1 +
drivers/pwm/Kconfig | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm_th1520.rs | 287 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 299 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 5589c0d2253bcb04e78d7b89ef6ef0ed41121d77..966ce515c8bfefdff1975bb716a267435ec0feae 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21319,6 +21319,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 03c5a100a03e2acdccf8a46b9c70b736b630bd3a..be05658a568cb9156ef623caf54ff1aaba898d01 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_RUST
+ tristate "TH1520 PWM support (Rust)"
+ depends on RUST_PWM_ABSTRACTIONS
+ help
+ This option enables the driver for the PWM controller found on the
+ T-HEAD TH1520 SoC. This driver is written in Rust.
+
+ 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..d41b1940df903ba2036d8e3ed93efcd66834b7ab 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -73,3 +73,4 @@ obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o
obj-$(CONFIG_PWM_VISCONTI) += pwm-visconti.o
obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o
obj-$(CONFIG_PWM_XILINX) += pwm-xilinx.o
+obj-$(CONFIG_PWM_TH1520_RUST) += pwm_th1520.o
diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
new file mode 100644
index 0000000000000000000000000000000000000000..9e43474f5123b51c49035d71219303a606c20a5a
--- /dev/null
+++ b/drivers/pwm/pwm_th1520.rs
@@ -0,0 +1,287 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2025 Samsung Electronics Co., Ltd.
+// Author: Michal Wilczynski <m.wilczynski@samsung.com>
+
+//! Rust T-HEAD TH1520 PWM driver
+
+use core::ops::Deref;
+use kernel::{
+ c_str,
+ clk::Clk,
+ device::{Bound, Core, Device},
+ devres,
+ error::{code::*, Result},
+ io::mem::IoMem,
+ math::KernelMathExt,
+ 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;
+
+const TH1520_PWM_REG_SIZE: usize = 0xB0;
+
+/// 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.
+struct Th1520PwmDriverData {
+ iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
+ clk: Clk,
+}
+
+impl pwm::PwmOps for Th1520PwmDriverData {
+ type WfHw = Th1520WfHw;
+
+ fn get_state(
+ chip: &mut pwm::Chip,
+ pwm: &mut pwm::Device,
+ state: &mut pwm::State,
+ parent_dev: &Device<Bound>,
+ ) -> Result {
+ let data: &Self = chip.drvdata().ok_or(EINVAL)?;
+ let hwpwm = pwm.hwpwm();
+ let iomem_guard = data.iomem.access(parent_dev)?;
+ let iomap = iomem_guard.deref();
+ let ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
+ let period_cycles = iomap.read32(th1520_pwm_per(hwpwm));
+ let duty_cycles = iomap.read32(th1520_pwm_fp(hwpwm));
+
+ state.set_enabled(duty_cycles != 0);
+
+ let rate_hz = data.clk.rate().as_hz();
+ let period_ns = (period_cycles as u64)
+ .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
+ .unwrap_or(0);
+ state.set_period(period_ns);
+
+ let duty_ns = (duty_cycles as u64)
+ .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
+ .unwrap_or(0);
+ state.set_duty_cycle(duty_ns);
+
+ if (ctrl & PWM_FPOUT) != 0 {
+ state.set_polarity(pwm::Polarity::Normal);
+ } else {
+ state.set_polarity(pwm::Polarity::Inversed);
+ }
+
+ Ok(())
+ }
+
+ fn round_waveform_tohw(
+ chip: &mut pwm::Chip,
+ pwm: &mut pwm::Device,
+ wf: &pwm::Waveform,
+ ) -> Result<(i32, Self::WfHw)> {
+ let data: &Self = chip.drvdata().ok_or(EINVAL)?;
+ let hwpwm = pwm.hwpwm();
+
+ if wf.duty_offset_ns != 0 {
+ dev_err!(chip.device(), "PWM-{}: Duty offset not supported\n", hwpwm);
+ return Err(ENOTSUPP);
+ }
+
+ if wf.period_length_ns == 0 {
+ return Ok((
+ 0,
+ Th1520WfHw {
+ enabled: false,
+ ..Default::default()
+ },
+ ));
+ }
+
+ let rate_hz = data.clk.rate().as_hz();
+
+ let period_cycles = wf
+ .period_length_ns
+ .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
+ .ok_or(EINVAL)?;
+ if period_cycles > u32::MAX as u64 {
+ dev_err!(
+ chip.device(),
+ "PWM-{}: Calculated period {} cycles is out of range\n",
+ hwpwm,
+ period_cycles
+ );
+ return Err(EINVAL);
+ }
+
+ let duty_cycles = wf
+ .duty_length_ns
+ .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
+ .ok_or(EINVAL)?;
+ if duty_cycles > period_cycles {
+ dev_err!(
+ chip.device(),
+ "PWM-{}: Duty {}ns > period {}ns\n",
+ hwpwm,
+ wf.duty_length_ns,
+ wf.period_length_ns
+ );
+ return Err(EINVAL);
+ }
+
+ let mut ctrl_val = PWM_CONTINUOUS_MODE;
+ if pwm.state().polarity() == pwm::Polarity::Normal {
+ ctrl_val |= PWM_FPOUT;
+ }
+
+ let wfhw = Th1520WfHw {
+ period_cycles: period_cycles as u32,
+ duty_cycles: duty_cycles as u32,
+ ctrl_val,
+ enabled: true,
+ };
+
+ dev_dbg!(
+ chip.device(),
+ "wfhw -- Period: {}, Duty: {}, Ctrl: 0x{:x}\n",
+ wfhw.period_cycles,
+ wfhw.duty_cycles,
+ wfhw.ctrl_val
+ );
+ Ok((0, wfhw))
+ }
+
+ fn write_waveform(
+ chip: &mut pwm::Chip,
+ pwm: &mut pwm::Device,
+ wfhw: &Self::WfHw,
+ parent_dev: &Device<Bound>,
+ ) -> Result {
+ let data: &Self = chip.drvdata().ok_or(EINVAL)?;
+ let hwpwm = pwm.hwpwm();
+ let iomem_guard = data.iomem.access(parent_dev)?;
+ let iomap = iomem_guard.deref();
+ let was_enabled = pwm.state().enabled();
+
+ if !wfhw.enabled {
+ if was_enabled {
+ let mut ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
+
+ ctrl &= !PWM_CFG_UPDATE;
+
+ iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
+ iomap.write32(0, th1520_pwm_fp(hwpwm));
+ iomap.write32(ctrl | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
+ }
+ return Ok(());
+ }
+
+ let ctrl = wfhw.ctrl_val & !PWM_CFG_UPDATE;
+
+ iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
+ iomap.write32(wfhw.period_cycles, th1520_pwm_per(hwpwm));
+ iomap.write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm));
+ iomap.write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
+
+ if !was_enabled {
+ iomap.write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm));
+ }
+
+ Ok(())
+ }
+}
+
+impl Drop for Th1520PwmDriverData {
+ fn drop(&mut self) {
+ self.clk.disable_unprepare();
+ }
+}
+
+static TH1520_PWM_OPS: pwm::PwmOpsVTable = pwm::create_pwm_ops::<Th1520PwmDriverData>();
+
+struct Th1520PwmPlatformDriver {
+ _registration: pwm::Registration,
+}
+
+kernel::of_device_table!(
+ OF_TABLE,
+ MODULE_OF_TABLE,
+ <Th1520PwmPlatformDriver as platform::Driver>::IdInfo,
+ [(of::DeviceId::new(c_str!("thead,th1520-pwm")), ())]
+);
+
+impl platform::Driver for Th1520PwmPlatformDriver {
+ type IdInfo = ();
+ const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+
+ fn probe(
+ pdev: &platform::Device<Core>,
+ _id_info: Option<&Self::IdInfo>,
+ ) -> Result<Pin<KBox<Self>>> {
+ let dev = pdev.as_ref();
+ let resource = pdev.resource(0).ok_or(ENODEV)?;
+ let iomem = pdev.ioremap_resource_sized::<TH1520_PWM_REG_SIZE>(resource)?;
+ let clk = Clk::get(pdev.as_ref(), None)?;
+
+ clk.prepare_enable()?;
+
+ let rate_hz = clk.rate().as_hz();
+ if rate_hz == 0 {
+ dev_err!(dev, "Clock rate is zero\n");
+ return Err(EINVAL);
+ }
+
+ if rate_hz > time::NSEC_PER_SEC as usize {
+ dev_err!(
+ dev,
+ "Clock rate {} Hz is too high, not supported.\n",
+ rate_hz
+ );
+ return Err(ERANGE);
+ }
+
+ let chip = pwm::Chip::new(dev, MAX_PWM_NUM, 0)?;
+
+ let drvdata = KBox::new(Th1520PwmDriverData { iomem, clk }, GFP_KERNEL)?;
+ chip.set_drvdata(drvdata);
+
+ let registration = pwm::Registration::new(chip, &TH1520_PWM_OPS)?;
+
+ Ok(KBox::new(
+ Th1520PwmPlatformDriver {
+ _registration: registration,
+ },
+ GFP_KERNEL,
+ )?
+ .into())
+ }
+}
+
+kernel::module_platform_driver! {
+ type: Th1520PwmPlatformDriver,
+ name: "pwm-th1520",
+ author: "Michal Wilczynski <m.wilczynski@samsung.com>",
+ description: "T-HEAD TH1520 PWM driver",
+ license: "GPL v2",
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 2/7] pwm: Add Rust driver for T-HEAD TH1520 SoC
2025-06-10 12:52 ` [PATCH v2 2/7] pwm: Add Rust driver for T-HEAD TH1520 SoC Michal Wilczynski
@ 2025-06-11 6:58 ` Uwe Kleine-König
2025-06-11 19:52 ` Michal Wilczynski
` (2 more replies)
0 siblings, 3 replies; 27+ messages in thread
From: Uwe Kleine-König @ 2025-06-11 6:58 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
[-- Attachment #1: Type: text/plain, Size: 14575 bytes --]
Hello,
On Tue, Jun 10, 2025 at 02:52:50PM +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@samsung.com>
> ---
> MAINTAINERS | 1 +
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm_th1520.rs | 287 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 299 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5589c0d2253bcb04e78d7b89ef6ef0ed41121d77..966ce515c8bfefdff1975bb716a267435ec0feae 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21319,6 +21319,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 03c5a100a03e2acdccf8a46b9c70b736b630bd3a..be05658a568cb9156ef623caf54ff1aaba898d01 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_RUST
Is "_RUST" relevant here? I'd drop that.
> + tristate "TH1520 PWM support (Rust)"
Also while having drivers is rust is a great step forward, it's not
relevant to the user selecting support for the TH1520 device.
> + depends on RUST_PWM_ABSTRACTIONS
> + help
> + This option enables the driver for the PWM controller found on the
> + T-HEAD TH1520 SoC. This driver is written in Rust.
> +
> + 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..d41b1940df903ba2036d8e3ed93efcd66834b7ab 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -73,3 +73,4 @@ obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o
> obj-$(CONFIG_PWM_VISCONTI) += pwm-visconti.o
> obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o
> obj-$(CONFIG_PWM_XILINX) += pwm-xilinx.o
> +obj-$(CONFIG_PWM_TH1520_RUST) += pwm_th1520.o
Alphabetic ordering please
> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..9e43474f5123b51c49035d71219303a606c20a5a
> --- /dev/null
> +++ b/drivers/pwm/pwm_th1520.rs
> @@ -0,0 +1,287 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +//! Rust T-HEAD TH1520 PWM driver
A short paragraph describing the hardware limitations of that driver
here would be nice. While you probably cannot stick to the exact format
used in newer C drivers such that
sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
emits the info for your driver, I'd appreciate you sticking to mostly
this format.
> +use core::ops::Deref;
> +use kernel::{
> + c_str,
> + clk::Clk,
> + device::{Bound, Core, Device},
> + devres,
> + error::{code::*, Result},
> + io::mem::IoMem,
> + math::KernelMathExt,
> + 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
> +}
empty line here between these functions?
> +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;
> +
> +const TH1520_PWM_REG_SIZE: usize = 0xB0;
> +
> +/// Hardware-specific waveform representation for TH1520.
Some comments use 2 and other 3 slashes. Does this have any semantic?
> +#[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.
> +struct Th1520PwmDriverData {
> + iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
> + clk: Clk,
> +}
> +
> +impl pwm::PwmOps for Th1520PwmDriverData {
> + type WfHw = Th1520WfHw;
> +
> + fn get_state(
> + chip: &mut pwm::Chip,
> + pwm: &mut pwm::Device,
> + state: &mut pwm::State,
> + parent_dev: &Device<Bound>,
> + ) -> Result {
Huh, if you do the newstyle stuff, .get_state() is wrong. It's either
.round_waveform_tohw() + .round_waveform_fromhw() + .read_waveform() +
.write_waveform() or .apply() + .get_state(), but don't mix these.
> + let data: &Self = chip.drvdata().ok_or(EINVAL)?;
> + let hwpwm = pwm.hwpwm();
> + let iomem_guard = data.iomem.access(parent_dev)?;
> + let iomap = iomem_guard.deref();
> + let ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
> + let period_cycles = iomap.read32(th1520_pwm_per(hwpwm));
> + let duty_cycles = iomap.read32(th1520_pwm_fp(hwpwm));
> +
> + state.set_enabled(duty_cycles != 0);
> +
> + let rate_hz = data.clk.rate().as_hz();
> + let period_ns = (period_cycles as u64)
> + .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
> + .unwrap_or(0);
What does .unwrap_or(0) do? You need to round up in this mul_div
operation.
> + state.set_period(period_ns);
> +
> + let duty_ns = (duty_cycles as u64)
> + .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
> + .unwrap_or(0);
> + state.set_duty_cycle(duty_ns);
> +
> + if (ctrl & PWM_FPOUT) != 0 {
> + state.set_polarity(pwm::Polarity::Normal);
> + } else {
> + state.set_polarity(pwm::Polarity::Inversed);
> + }
> +
> + Ok(())
> + }
> +
> + fn round_waveform_tohw(
> + chip: &mut pwm::Chip,
> + pwm: &mut pwm::Device,
> + wf: &pwm::Waveform,
> + ) -> Result<(i32, Self::WfHw)> {
> + let data: &Self = chip.drvdata().ok_or(EINVAL)?;
> + let hwpwm = pwm.hwpwm();
> +
> + if wf.duty_offset_ns != 0 {
> + dev_err!(chip.device(), "PWM-{}: Duty offset not supported\n", hwpwm);
That's wrong, pick the biggest offset value that is possible to
implement and not bigger than the requested value.
Your hardware can do inversed polarity, so offset is either 0 or
period-duty.
> + return Err(ENOTSUPP);
> + }
> +
> + if wf.period_length_ns == 0 {
> + return Ok((
> + 0,
> + Th1520WfHw {
> + enabled: false,
> + ..Default::default()
> + },
> + ));
> + }
> +
> + let rate_hz = data.clk.rate().as_hz();
> +
> + let period_cycles = wf
> + .period_length_ns
> + .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
> + .ok_or(EINVAL)?;
If period_length_ns is BIG, pick the biggest possible period_cycles
value, not EINVAL.
> + if period_cycles > u32::MAX as u64 {
> + dev_err!(
> + chip.device(),
> + "PWM-{}: Calculated period {} cycles is out of range\n",
> + hwpwm,
> + period_cycles
> + );
> + return Err(EINVAL);
> + }
ditto.
> + let duty_cycles = wf
> + .duty_length_ns
> + .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
> + .ok_or(EINVAL)?;
> + if duty_cycles > period_cycles {
You can assume this won't happen.
> + dev_err!(
> + chip.device(),
> + "PWM-{}: Duty {}ns > period {}ns\n",
> + hwpwm,
> + wf.duty_length_ns,
> + wf.period_length_ns
> + );
> + return Err(EINVAL);
> + }
> +
> + let mut ctrl_val = PWM_CONTINUOUS_MODE;
> + if pwm.state().polarity() == pwm::Polarity::Normal {
> + ctrl_val |= PWM_FPOUT;
What is pwm.state()? If that's similar to pwm->state in C this is
irrelevant here. It describes the current state, not the new request.
> + }
> +
> + let wfhw = Th1520WfHw {
> + period_cycles: period_cycles as u32,
> + duty_cycles: duty_cycles as u32,
> + ctrl_val,
> + enabled: true,
> + };
> +
> + dev_dbg!(
> + chip.device(),
> + "wfhw -- Period: {}, Duty: {}, Ctrl: 0x{:x}\n",
> + wfhw.period_cycles,
> + wfhw.duty_cycles,
> + wfhw.ctrl_val
> + );
This would be much more helpful if it also contained the values from wf.
> + Ok((0, wfhw))
> + }
> +
> + fn write_waveform(
> + chip: &mut pwm::Chip,
> + pwm: &mut pwm::Device,
> + wfhw: &Self::WfHw,
> + parent_dev: &Device<Bound>,
> + ) -> Result {
> + let data: &Self = chip.drvdata().ok_or(EINVAL)?;
> + let hwpwm = pwm.hwpwm();
> + let iomem_guard = data.iomem.access(parent_dev)?;
> + let iomap = iomem_guard.deref();
> + let was_enabled = pwm.state().enabled();
> +
> + if !wfhw.enabled {
> + if was_enabled {
> + let mut ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
Do you need that read? Isn't is clear what the value is?
> + ctrl &= !PWM_CFG_UPDATE;
> +
> + iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
> + iomap.write32(0, th1520_pwm_fp(hwpwm));
> + iomap.write32(ctrl | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
> + }
> + return Ok(());
> + }
> +
> + let ctrl = wfhw.ctrl_val & !PWM_CFG_UPDATE;
wfhw.ctrl_val never has PWM_CFG_UPDATE set.
> + iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
> + iomap.write32(wfhw.period_cycles, th1520_pwm_per(hwpwm));
> + iomap.write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm));
> + iomap.write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
> +
> + if !was_enabled {
> + iomap.write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm));
Can this be combined with the above write?
> + }
> +
> + Ok(())
> + }
> +}
> +
> +impl Drop for Th1520PwmDriverData {
> + fn drop(&mut self) {
> + self.clk.disable_unprepare();
> + }
> +}
> +
> +static TH1520_PWM_OPS: pwm::PwmOpsVTable = pwm::create_pwm_ops::<Th1520PwmDriverData>();
> +
> +struct Th1520PwmPlatformDriver {
> + _registration: pwm::Registration,
> +}
> +
> +kernel::of_device_table!(
> + OF_TABLE,
> + MODULE_OF_TABLE,
> + <Th1520PwmPlatformDriver as platform::Driver>::IdInfo,
> + [(of::DeviceId::new(c_str!("thead,th1520-pwm")), ())]
> +);
> +
> +impl platform::Driver for Th1520PwmPlatformDriver {
> + type IdInfo = ();
> + const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +
> + fn probe(
> + pdev: &platform::Device<Core>,
> + _id_info: Option<&Self::IdInfo>,
> + ) -> Result<Pin<KBox<Self>>> {
> + let dev = pdev.as_ref();
> + let resource = pdev.resource(0).ok_or(ENODEV)?;
> + let iomem = pdev.ioremap_resource_sized::<TH1520_PWM_REG_SIZE>(resource)?;
> + let clk = Clk::get(pdev.as_ref(), None)?;
> +
> + clk.prepare_enable()?;
We don't have clk_rate_get_exclusive() yet, right? Then please add a
comment here that this needs to be added here when it became available.
> +
> + let rate_hz = clk.rate().as_hz();
> + if rate_hz == 0 {
> + dev_err!(dev, "Clock rate is zero\n");
> + return Err(EINVAL);
> + }
> +
> + if rate_hz > time::NSEC_PER_SEC as usize {
> + dev_err!(
> + dev,
> + "Clock rate {} Hz is too high, not supported.\n",
> + rate_hz
> + );
> + return Err(ERANGE);
> + }
> +
> + let chip = pwm::Chip::new(dev, MAX_PWM_NUM, 0)?;
> +
> + let drvdata = KBox::new(Th1520PwmDriverData { iomem, clk }, GFP_KERNEL)?;
> + chip.set_drvdata(drvdata);
> +
> + let registration = pwm::Registration::new(chip, &TH1520_PWM_OPS)?;
> +
> + Ok(KBox::new(
> + Th1520PwmPlatformDriver {
> + _registration: registration,
> + },
> + GFP_KERNEL,
> + )?
> + .into())
> + }
> +}
> +
> +kernel::module_platform_driver! {
> + type: Th1520PwmPlatformDriver,
> + name: "pwm-th1520",
> + author: "Michal Wilczynski <m.wilczynski@samsung.com>",
> + description: "T-HEAD TH1520 PWM driver",
> + license: "GPL v2",
> +}
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 2/7] pwm: Add Rust driver for T-HEAD TH1520 SoC
2025-06-11 6:58 ` Uwe Kleine-König
@ 2025-06-11 19:52 ` Michal Wilczynski
2025-06-11 20:04 ` Michal Wilczynski
2025-06-12 8:14 ` Michal Wilczynski
2 siblings, 0 replies; 27+ messages in thread
From: Michal Wilczynski @ 2025-06-11 19:52 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On 6/11/25 08:58, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Jun 10, 2025 at 02:52:50PM +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@samsung.com>
>> ---
>> MAINTAINERS | 1 +
>> drivers/pwm/Kconfig | 10 ++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm_th1520.rs | 287 ++++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 299 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5589c0d2253bcb04e78d7b89ef6ef0ed41121d77..966ce515c8bfefdff1975bb716a267435ec0feae 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -21319,6 +21319,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 03c5a100a03e2acdccf8a46b9c70b736b630bd3a..be05658a568cb9156ef623caf54ff1aaba898d01 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_RUST
>
> Is "_RUST" relevant here? I'd drop that.
Yeah I think it's a good idea to drop.
>
>> + tristate "TH1520 PWM support (Rust)"
>
> Also while having drivers is rust is a great step forward, it's not
> relevant to the user selecting support for the TH1520 device.
>
>> + depends on RUST_PWM_ABSTRACTIONS
>> + help
>> + This option enables the driver for the PWM controller found on the
>> + T-HEAD TH1520 SoC. This driver is written in Rust.
>> +
>> + 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..d41b1940df903ba2036d8e3ed93efcd66834b7ab 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -73,3 +73,4 @@ obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o
>> obj-$(CONFIG_PWM_VISCONTI) += pwm-visconti.o
>> obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o
>> obj-$(CONFIG_PWM_XILINX) += pwm-xilinx.o
>> +obj-$(CONFIG_PWM_TH1520_RUST) += pwm_th1520.o
>
> Alphabetic ordering please
>
>> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..9e43474f5123b51c49035d71219303a606c20a5a
>> --- /dev/null
>> +++ b/drivers/pwm/pwm_th1520.rs
>> @@ -0,0 +1,287 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +//! Rust T-HEAD TH1520 PWM driver
>
> A short paragraph describing the hardware limitations of that driver
> here would be nice. While you probably cannot stick to the exact format
> used in newer C drivers such that
>
> sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
>
> emits the info for your driver, I'd appreciate you sticking to mostly
> this format.
Good point. I will add a documentation comment block at the top of the
file outlining the hardware limitations, keeping the format as close as
possible to what is used in the C drivers.
>
>> +use core::ops::Deref;
>> +use kernel::{
>> + c_str,
>> + clk::Clk,
>> + device::{Bound, Core, Device},
>> + devres,
>> + error::{code::*, Result},
>> + io::mem::IoMem,
>> + math::KernelMathExt,
>> + 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
>> +}
>
> empty line here between these functions?
>
>> +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;
>> +
>> +const TH1520_PWM_REG_SIZE: usize = 0xB0;
>> +
>> +/// Hardware-specific waveform representation for TH1520.
>
> Some comments use 2 and other 3 slashes. Does this have any semantic?
Yes, they have a semantic difference. /// denotes a documentation
comment that is processed by rustdoc to generate documentation, while //
is a regular implementation comment. The compiler is configured to
require documentation for public items (like structs and functions),
which is why /// is used on the struct definition.
>
>> +#[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.
>> +struct Th1520PwmDriverData {
>> + iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
>> + clk: Clk,
>> +}
>> +
>> +impl pwm::PwmOps for Th1520PwmDriverData {
>> + type WfHw = Th1520WfHw;
>> +
>> + fn get_state(
>> + chip: &mut pwm::Chip,
>> + pwm: &mut pwm::Device,
>> + state: &mut pwm::State,
>> + parent_dev: &Device<Bound>,
>> + ) -> Result {
>
> Huh, if you do the newstyle stuff, .get_state() is wrong. It's either
> .round_waveform_tohw() + .round_waveform_fromhw() + .read_waveform() +
> .write_waveform() or .apply() + .get_state(), but don't mix these.
You are absolutely right. This was a misunderstanding of the new API on
my part. I will remove the .get_state implementation entirely in the
next version and rely solely on the waveform operations.
>
>> + let data: &Self = chip.drvdata().ok_or(EINVAL)?;
>> + let hwpwm = pwm.hwpwm();
>> + let iomem_guard = data.iomem.access(parent_dev)?;
>> + let iomap = iomem_guard.deref();
>> + let ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
>> + let period_cycles = iomap.read32(th1520_pwm_per(hwpwm));
>> + let duty_cycles = iomap.read32(th1520_pwm_fp(hwpwm));
>> +
>> + state.set_enabled(duty_cycles != 0);
>> +
>> + let rate_hz = data.clk.rate().as_hz();
>> + let period_ns = (period_cycles as u64)
>> + .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
>> + .unwrap_or(0);
>
> What does .unwrap_or(0) do? You need to round up in this mul_div
> operation.
The .unwrap_or(0) is to handle the case where the mul_div helper returns
None, which can happen if the divisor (rate_hz) is zero. In that case,
the period becomes 0. The mul_diver helper is introduced in this commit
[1].
[1] - https://lore.kernel.org/all/20250609-math-rust-v1-v1-1-285fac00031f@samsung.com/
>
>> + state.set_period(period_ns);
>> +
>> + let duty_ns = (duty_cycles as u64)
>> + .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
>> + .unwrap_or(0);
>> + state.set_duty_cycle(duty_ns);
>> +
>> + if (ctrl & PWM_FPOUT) != 0 {
>> + state.set_polarity(pwm::Polarity::Normal);
>> + } else {
>> + state.set_polarity(pwm::Polarity::Inversed);
>> + }
>> +
>> + Ok(())
>> + }
>> +
>> + fn round_waveform_tohw(
>> + chip: &mut pwm::Chip,
>> + pwm: &mut pwm::Device,
>> + wf: &pwm::Waveform,
>> + ) -> Result<(i32, Self::WfHw)> {
>> + let data: &Self = chip.drvdata().ok_or(EINVAL)?;
>> + let hwpwm = pwm.hwpwm();
>> +
>> + if wf.duty_offset_ns != 0 {
>> + dev_err!(chip.device(), "PWM-{}: Duty offset not supported\n", hwpwm);
>
> That's wrong, pick the biggest offset value that is possible to
> implement and not bigger than the requested value.
> Your hardware can do inversed polarity, so offset is either 0 or
> period-duty.
Addressed below with the pwm.state() comment
>
>> + return Err(ENOTSUPP);
>> + }
>> +
>> + if wf.period_length_ns == 0 {
>> + return Ok((
>> + 0,
>> + Th1520WfHw {
>> + enabled: false,
>> + ..Default::default()
>> + },
>> + ));
>> + }
>> +
>> + let rate_hz = data.clk.rate().as_hz();
>> +
>> + let period_cycles = wf
>> + .period_length_ns
>> + .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
>> + .ok_or(EINVAL)?;
>
> If period_length_ns is BIG, pick the biggest possible period_cycles
> value, not EINVAL.
In this case EINVAL mean the function would return EINVAL not
'period_cycles' = EINVAL. This won't happen here since
time::NSEC_PER_SEC is a constant, so this won't return None. This is not
checking for overflow.
>
>> + if period_cycles > u32::MAX as u64 {
In here I could pick period_cycles = u32::MAX.
>> + dev_err!(
>> + chip.device(),
>> + "PWM-{}: Calculated period {} cycles is out of range\n",
>> + hwpwm,
>> + period_cycles
>> + );
>> + return Err(EINVAL);
>> + }
>
> ditto.
>
>> + let duty_cycles = wf
>> + .duty_length_ns
>> + .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
>> + .ok_or(EINVAL)?;
>> + if duty_cycles > period_cycles {
>
> You can assume this won't happen.
>
>> + dev_err!(
>> + chip.device(),
>> + "PWM-{}: Duty {}ns > period {}ns\n",
>> + hwpwm,
>> + wf.duty_length_ns,
>> + wf.period_length_ns
>> + );
>> + return Err(EINVAL);
>> + }
>> +
>> + let mut ctrl_val = PWM_CONTINUOUS_MODE;
>> + if pwm.state().polarity() == pwm::Polarity::Normal {
>> + ctrl_val |= PWM_FPOUT;
>
> What is pwm.state()? If that's similar to pwm->state in C this is
> irrelevant here. It describes the current state, not the new request.
Yes, you are correct. I should derive the polarity from the requested
waveform arguments, not from the current state. I see now how to handle
both the duty_offset and the polarity based on the arguments in wf. I
will implement the correct logic.
>
>> + }
>> +
>> + let wfhw = Th1520WfHw {
>> + period_cycles: period_cycles as u32,
>> + duty_cycles: duty_cycles as u32,
>> + ctrl_val,
>> + enabled: true,
>> + };
>> +
>> + dev_dbg!(
>> + chip.device(),
>> + "wfhw -- Period: {}, Duty: {}, Ctrl: 0x{:x}\n",
>> + wfhw.period_cycles,
>> + wfhw.duty_cycles,
>> + wfhw.ctrl_val
>> + );
>
> This would be much more helpful if it also contained the values from wf.
>
>> + Ok((0, wfhw))
>> + }
>> +
>> + fn write_waveform(
>> + chip: &mut pwm::Chip,
>> + pwm: &mut pwm::Device,
>> + wfhw: &Self::WfHw,
>> + parent_dev: &Device<Bound>,
>> + ) -> Result {
>> + let data: &Self = chip.drvdata().ok_or(EINVAL)?;
>> + let hwpwm = pwm.hwpwm();
>> + let iomem_guard = data.iomem.access(parent_dev)?;
>> + let iomap = iomem_guard.deref();
>> + let was_enabled = pwm.state().enabled();
>> +
>> + if !wfhw.enabled {
>> + if was_enabled {
>> + let mut ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
>
> Do you need that read? Isn't is clear what the value is?
>
>> + ctrl &= !PWM_CFG_UPDATE;
>> +
>> + iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
>> + iomap.write32(0, th1520_pwm_fp(hwpwm));
>> + iomap.write32(ctrl | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
>> + }
>> + return Ok(());
>> + }
>> +
>> + let ctrl = wfhw.ctrl_val & !PWM_CFG_UPDATE;
>
> wfhw.ctrl_val never has PWM_CFG_UPDATE set.
You're right about the redundant read and the logic with PWM_CFG_UPDATE.
These are leftovers from a frustrating debug session related to a clock
gating issue. I will refactor this section to be cleaner and more
direct.
>
>> + iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
>> + iomap.write32(wfhw.period_cycles, th1520_pwm_per(hwpwm));
>> + iomap.write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm));
>> + iomap.write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
>> +
>> + if !was_enabled {
>> + iomap.write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm));
>
> Can this be combined with the above write?
Per the TH1520 peripheral manual [2] (chapter 6.6.2.1), the PWM_START bit
should only be asserted when enabling the PWM for the first time, not
during a reconfiguration of an alreadyrunning channel. The separate if
statement is there to handle this specific hardware requirement.
[2] - https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/blob/6b56e2d69485c375c5912eaa2791f79f1d089c07/docs/TH1520%20Peripheral%20Interface%20User%2
>
>> + }
>> +
>> + Ok(())
>> + }
>> +}
>> +
>> +impl Drop for Th1520PwmDriverData {
>> + fn drop(&mut self) {
>> + self.clk.disable_unprepare();
>> + }
>> +}
>> +
>> +static TH1520_PWM_OPS: pwm::PwmOpsVTable = pwm::create_pwm_ops::<Th1520PwmDriverData>();
>> +
>> +struct Th1520PwmPlatformDriver {
>> + _registration: pwm::Registration,
>> +}
>> +
>> +kernel::of_device_table!(
>> + OF_TABLE,
>> + MODULE_OF_TABLE,
>> + <Th1520PwmPlatformDriver as platform::Driver>::IdInfo,
>> + [(of::DeviceId::new(c_str!("thead,th1520-pwm")), ())]
>> +);
>> +
>> +impl platform::Driver for Th1520PwmPlatformDriver {
>> + type IdInfo = ();
>> + const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
>> +
>> + fn probe(
>> + pdev: &platform::Device<Core>,
>> + _id_info: Option<&Self::IdInfo>,
>> + ) -> Result<Pin<KBox<Self>>> {
>> + let dev = pdev.as_ref();
>> + let resource = pdev.resource(0).ok_or(ENODEV)?;
>> + let iomem = pdev.ioremap_resource_sized::<TH1520_PWM_REG_SIZE>(resource)?;
>> + let clk = Clk::get(pdev.as_ref(), None)?;
>> +
>> + clk.prepare_enable()?;
>
> We don't have clk_rate_get_exclusive() yet, right? Then please add a
> comment here that this needs to be added here when it became available.
Yeah sadly we don't have that abstraction yet.
>
>> +
>> + let rate_hz = clk.rate().as_hz();
>> + if rate_hz == 0 {
>> + dev_err!(dev, "Clock rate is zero\n");
>> + return Err(EINVAL);
>> + }
>> +
>> + if rate_hz > time::NSEC_PER_SEC as usize {
>> + dev_err!(
>> + dev,
>> + "Clock rate {} Hz is too high, not supported.\n",
>> + rate_hz
>> + );
>> + return Err(ERANGE);
>> + }
>> +
>> + let chip = pwm::Chip::new(dev, MAX_PWM_NUM, 0)?;
>> +
>> + let drvdata = KBox::new(Th1520PwmDriverData { iomem, clk }, GFP_KERNEL)?;
>> + chip.set_drvdata(drvdata);
>> +
>> + let registration = pwm::Registration::new(chip, &TH1520_PWM_OPS)?;
>> +
>> + Ok(KBox::new(
>> + Th1520PwmPlatformDriver {
>> + _registration: registration,
>> + },
>> + GFP_KERNEL,
>> + )?
>> + .into())
>> + }
>> +}
>> +
>> +kernel::module_platform_driver! {
>> + type: Th1520PwmPlatformDriver,
>> + name: "pwm-th1520",
>> + author: "Michal Wilczynski <m.wilczynski@samsung.com>",
>> + description: "T-HEAD TH1520 PWM driver",
>> + license: "GPL v2",
>> +}
>
> Best regards
> Uwe
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 2/7] pwm: Add Rust driver for T-HEAD TH1520 SoC
2025-06-11 6:58 ` Uwe Kleine-König
2025-06-11 19:52 ` Michal Wilczynski
@ 2025-06-11 20:04 ` Michal Wilczynski
2025-06-11 21:15 ` Michal Wilczynski
2025-06-11 21:40 ` Uwe Kleine-König
2025-06-12 8:14 ` Michal Wilczynski
2 siblings, 2 replies; 27+ messages in thread
From: Michal Wilczynski @ 2025-06-11 20:04 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On 6/11/25 08:58, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Jun 10, 2025 at 02:52:50PM +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@samsung.com>
>> ---
>> MAINTAINERS | 1 +
>> drivers/pwm/Kconfig | 10 ++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm_th1520.rs | 287 ++++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 299 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5589c0d2253bcb04e78d7b89ef6ef0ed41121d77..966ce515c8bfefdff1975bb716a267435ec0feae 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -21319,6 +21319,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 03c5a100a03e2acdccf8a46b9c70b736b630bd3a..be05658a568cb9156ef623caf54ff1aaba898d01 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_RUST
>
> Is "_RUST" relevant here? I'd drop that.
Yeah I think it's a good idea to drop.
>
>> + tristate "TH1520 PWM support (Rust)"
>
> Also while having drivers is rust is a great step forward, it's not
> relevant to the user selecting support for the TH1520 device.
>
>> + depends on RUST_PWM_ABSTRACTIONS
>> + help
>> + This option enables the driver for the PWM controller found on the
>> + T-HEAD TH1520 SoC. This driver is written in Rust.
>> +
>> + 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..d41b1940df903ba2036d8e3ed93efcd66834b7ab 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -73,3 +73,4 @@ obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o
>> obj-$(CONFIG_PWM_VISCONTI) += pwm-visconti.o
>> obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o
>> obj-$(CONFIG_PWM_XILINX) += pwm-xilinx.o
>> +obj-$(CONFIG_PWM_TH1520_RUST) += pwm_th1520.o
>
> Alphabetic ordering please
>
>> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..9e43474f5123b51c49035d71219303a606c20a5a
>> --- /dev/null
>> +++ b/drivers/pwm/pwm_th1520.rs
>> @@ -0,0 +1,287 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +//! Rust T-HEAD TH1520 PWM driver
>
> A short paragraph describing the hardware limitations of that driver
> here would be nice. While you probably cannot stick to the exact format
> used in newer C drivers such that
>
> sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
>
> emits the info for your driver, I'd appreciate you sticking to mostly
> this format.
Good point. I will add a documentation comment block at the top of the
file outlining the hardware limitations, keeping the format as close as
possible to what is used in the C drivers.
>
>> +use core::ops::Deref;
>> +use kernel::{
>> + c_str,
>> + clk::Clk,
>> + device::{Bound, Core, Device},
>> + devres,
>> + error::{code::*, Result},
>> + io::mem::IoMem,
>> + math::KernelMathExt,
>> + 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
>> +}
>
> empty line here between these functions?
>
>> +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;
>> +
>> +const TH1520_PWM_REG_SIZE: usize = 0xB0;
>> +
>> +/// Hardware-specific waveform representation for TH1520.
>
> Some comments use 2 and other 3 slashes. Does this have any semantic?
Yes, they have a semantic difference. /// denotes a documentation
comment that is processed by rustdoc to generate documentation, while //
is a regular implementation comment. The compiler is configured to
require documentation for public items (like structs and functions),
which is why /// is used on the struct definition.
>
>> +#[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.
>> +struct Th1520PwmDriverData {
>> + iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
>> + clk: Clk,
>> +}
>> +
>> +impl pwm::PwmOps for Th1520PwmDriverData {
>> + type WfHw = Th1520WfHw;
>> +
>> + fn get_state(
>> + chip: &mut pwm::Chip,
>> + pwm: &mut pwm::Device,
>> + state: &mut pwm::State,
>> + parent_dev: &Device<Bound>,
>> + ) -> Result {
>
> Huh, if you do the newstyle stuff, .get_state() is wrong. It's either
> .round_waveform_tohw() + .round_waveform_fromhw() + .read_waveform() +
> .write_waveform() or .apply() + .get_state(), but don't mix these.
You are absolutely right. This was a misunderstanding of the new API on
my part. I will remove the .get_state implementation entirely in the
next version and rely solely on the waveform operations.
>
>> + let data: &Self = chip.drvdata().ok_or(EINVAL)?;
>> + let hwpwm = pwm.hwpwm();
>> + let iomem_guard = data.iomem.access(parent_dev)?;
>> + let iomap = iomem_guard.deref();
>> + let ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
>> + let period_cycles = iomap.read32(th1520_pwm_per(hwpwm));
>> + let duty_cycles = iomap.read32(th1520_pwm_fp(hwpwm));
>> +
>> + state.set_enabled(duty_cycles != 0);
>> +
>> + let rate_hz = data.clk.rate().as_hz();
>> + let period_ns = (period_cycles as u64)
>> + .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
>> + .unwrap_or(0);
>
> What does .unwrap_or(0) do? You need to round up in this mul_div
> operation.
The .unwrap_or(0) is to handle the case where the mul_div helper returns
None, which can happen if the divisor (rate_hz) is zero. In that case,
the period becomes 0. The mul_div helper is introduced in this commit
[1].
[1] - https://lore.kernel.org/all/20250609-math-rust-v1-v1-1-285fac00031f@samsung.com/
>
>> + state.set_period(period_ns);
>> +
>> + let duty_ns = (duty_cycles as u64)
>> + .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
>> + .unwrap_or(0);
>> + state.set_duty_cycle(duty_ns);
>> +
>> + if (ctrl & PWM_FPOUT) != 0 {
>> + state.set_polarity(pwm::Polarity::Normal);
>> + } else {
>> + state.set_polarity(pwm::Polarity::Inversed);
>> + }
>> +
>> + Ok(())
>> + }
>> +
>> + fn round_waveform_tohw(
>> + chip: &mut pwm::Chip,
>> + pwm: &mut pwm::Device,
>> + wf: &pwm::Waveform,
>> + ) -> Result<(i32, Self::WfHw)> {
>> + let data: &Self = chip.drvdata().ok_or(EINVAL)?;
>> + let hwpwm = pwm.hwpwm();
>> +
>> + if wf.duty_offset_ns != 0 {
>> + dev_err!(chip.device(), "PWM-{}: Duty offset not supported\n", hwpwm);
>
> That's wrong, pick the biggest offset value that is possible to
> implement and not bigger than the requested value.
> Your hardware can do inversed polarity, so offset is either 0 or
> period-duty.
Addressed below with the pwm.state() comment
>
>> + return Err(ENOTSUPP);
>> + }
>> +
>> + if wf.period_length_ns == 0 {
>> + return Ok((
>> + 0,
>> + Th1520WfHw {
>> + enabled: false,
>> + ..Default::default()
>> + },
>> + ));
>> + }
>> +
>> + let rate_hz = data.clk.rate().as_hz();
>> +
>> + let period_cycles = wf
>> + .period_length_ns
>> + .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
>> + .ok_or(EINVAL)?;
>
> If period_length_ns is BIG, pick the biggest possible period_cycles
> value, not EINVAL.
In this case EINVAL mean the function would return EINVAL not
'period_cycles' = EINVAL. This won't happen here since
time::NSEC_PER_SEC is a constant, so this won't return None. This is not
checking for overflow.
>
>> + if period_cycles > u32::MAX as u64 {
In here I could pick period_cycles = u32::MAX.
>> + dev_err!(
>> + chip.device(),
>> + "PWM-{}: Calculated period {} cycles is out of range\n",
>> + hwpwm,
>> + period_cycles
>> + );
>> + return Err(EINVAL);
>> + }
>
> ditto.
>
>> + let duty_cycles = wf
>> + .duty_length_ns
>> + .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
>> + .ok_or(EINVAL)?;
>> + if duty_cycles > period_cycles {
>
> You can assume this won't happen.
>
>> + dev_err!(
>> + chip.device(),
>> + "PWM-{}: Duty {}ns > period {}ns\n",
>> + hwpwm,
>> + wf.duty_length_ns,
>> + wf.period_length_ns
>> + );
>> + return Err(EINVAL);
>> + }
>> +
>> + let mut ctrl_val = PWM_CONTINUOUS_MODE;
>> + if pwm.state().polarity() == pwm::Polarity::Normal {
>> + ctrl_val |= PWM_FPOUT;
>
> What is pwm.state()? If that's similar to pwm->state in C this is
> irrelevant here. It describes the current state, not the new request.
Yes, you are correct. I should derive the polarity from the requested
waveform arguments, not from the current state. I see now how to handle
both the duty_offset and the polarity based on the arguments in wf. I
will implement the correct logic.
>
>> + }
>> +
>> + let wfhw = Th1520WfHw {
>> + period_cycles: period_cycles as u32,
>> + duty_cycles: duty_cycles as u32,
>> + ctrl_val,
>> + enabled: true,
>> + };
>> +
>> + dev_dbg!(
>> + chip.device(),
>> + "wfhw -- Period: {}, Duty: {}, Ctrl: 0x{:x}\n",
>> + wfhw.period_cycles,
>> + wfhw.duty_cycles,
>> + wfhw.ctrl_val
>> + );
>
> This would be much more helpful if it also contained the values from wf.
>
>> + Ok((0, wfhw))
>> + }
>> +
>> + fn write_waveform(
>> + chip: &mut pwm::Chip,
>> + pwm: &mut pwm::Device,
>> + wfhw: &Self::WfHw,
>> + parent_dev: &Device<Bound>,
>> + ) -> Result {
>> + let data: &Self = chip.drvdata().ok_or(EINVAL)?;
>> + let hwpwm = pwm.hwpwm();
>> + let iomem_guard = data.iomem.access(parent_dev)?;
>> + let iomap = iomem_guard.deref();
>> + let was_enabled = pwm.state().enabled();
>> +
>> + if !wfhw.enabled {
>> + if was_enabled {
>> + let mut ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
>
> Do you need that read? Isn't is clear what the value is?
>
>> + ctrl &= !PWM_CFG_UPDATE;
>> +
>> + iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
>> + iomap.write32(0, th1520_pwm_fp(hwpwm));
>> + iomap.write32(ctrl | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
>> + }
>> + return Ok(());
>> + }
>> +
>> + let ctrl = wfhw.ctrl_val & !PWM_CFG_UPDATE;
>
> wfhw.ctrl_val never has PWM_CFG_UPDATE set.
You're right about the redundant read and the logic with PWM_CFG_UPDATE.
These are leftovers from a frustrating debug session related to a clock
gating issue. I will refactor this section to be cleaner and more
direct.
>
>> + iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
>> + iomap.write32(wfhw.period_cycles, th1520_pwm_per(hwpwm));
>> + iomap.write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm));
>> + iomap.write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
>> +
>> + if !was_enabled {
>> + iomap.write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm));
>
> Can this be combined with the above write?
Per the TH1520 peripheral manual [2] (chapter 6.6.2.1), the PWM_START bit
should only be asserted when enabling the PWM for the first time, not
during a reconfiguration of an alreadyrunning channel. The separate if
statement is there to handle this specific hardware requirement.
[2] - https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/blob/6b56e2d69485c375c5912eaa2791f79f1d089c07/docs/TH1520%20Peripheral%20Interface%20User%2
>
>> + }
>> +
>> + Ok(())
>> + }
>> +}
>> +
>> +impl Drop for Th1520PwmDriverData {
>> + fn drop(&mut self) {
>> + self.clk.disable_unprepare();
>> + }
>> +}
>> +
>> +static TH1520_PWM_OPS: pwm::PwmOpsVTable = pwm::create_pwm_ops::<Th1520PwmDriverData>();
>> +
>> +struct Th1520PwmPlatformDriver {
>> + _registration: pwm::Registration,
>> +}
>> +
>> +kernel::of_device_table!(
>> + OF_TABLE,
>> + MODULE_OF_TABLE,
>> + <Th1520PwmPlatformDriver as platform::Driver>::IdInfo,
>> + [(of::DeviceId::new(c_str!("thead,th1520-pwm")), ())]
>> +);
>> +
>> +impl platform::Driver for Th1520PwmPlatformDriver {
>> + type IdInfo = ();
>> + const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
>> +
>> + fn probe(
>> + pdev: &platform::Device<Core>,
>> + _id_info: Option<&Self::IdInfo>,
>> + ) -> Result<Pin<KBox<Self>>> {
>> + let dev = pdev.as_ref();
>> + let resource = pdev.resource(0).ok_or(ENODEV)?;
>> + let iomem = pdev.ioremap_resource_sized::<TH1520_PWM_REG_SIZE>(resource)?;
>> + let clk = Clk::get(pdev.as_ref(), None)?;
>> +
>> + clk.prepare_enable()?;
>
> We don't have clk_rate_get_exclusive() yet, right? Then please add a
> comment here that this needs to be added here when it became available.
Yeah sadly we don't have that abstraction yet.
>
>> +
>> + let rate_hz = clk.rate().as_hz();
>> + if rate_hz == 0 {
>> + dev_err!(dev, "Clock rate is zero\n");
>> + return Err(EINVAL);
>> + }
>> +
>> + if rate_hz > time::NSEC_PER_SEC as usize {
>> + dev_err!(
>> + dev,
>> + "Clock rate {} Hz is too high, not supported.\n",
>> + rate_hz
>> + );
>> + return Err(ERANGE);
>> + }
>> +
>> + let chip = pwm::Chip::new(dev, MAX_PWM_NUM, 0)?;
>> +
>> + let drvdata = KBox::new(Th1520PwmDriverData { iomem, clk }, GFP_KERNEL)?;
>> + chip.set_drvdata(drvdata);
>> +
>> + let registration = pwm::Registration::new(chip, &TH1520_PWM_OPS)?;
>> +
>> + Ok(KBox::new(
>> + Th1520PwmPlatformDriver {
>> + _registration: registration,
>> + },
>> + GFP_KERNEL,
>> + )?
>> + .into())
>> + }
>> +}
>> +
>> +kernel::module_platform_driver! {
>> + type: Th1520PwmPlatformDriver,
>> + name: "pwm-th1520",
>> + author: "Michal Wilczynski <m.wilczynski@samsung.com>",
>> + description: "T-HEAD TH1520 PWM driver",
>> + license: "GPL v2",
>> +}
>
> Best regards
> Uwe
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 2/7] pwm: Add Rust driver for T-HEAD TH1520 SoC
2025-06-11 20:04 ` Michal Wilczynski
@ 2025-06-11 21:15 ` Michal Wilczynski
2025-06-11 21:40 ` Uwe Kleine-König
1 sibling, 0 replies; 27+ messages in thread
From: Michal Wilczynski @ 2025-06-11 21:15 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On 6/11/25 22:04, Michal Wilczynski wrote:
>
>
> On 6/11/25 08:58, Uwe Kleine-König wrote:
>> Hello,
>>
>>
>> What does .unwrap_or(0) do? You need to round up in this mul_div
>> operation.
Yeah and I'm thinking that the helper needs to be updated or new one
added like mul_div_round_up, to do the rounding
>
> The .unwrap_or(0) is to handle the case where the mul_div helper returns
> None, which can happen if the divisor (rate_hz) is zero. In that case,
> the period becomes 0. The mul_div helper is introduced in this commit
> [1].
>
> [1] - https://lore.kernel.org/all/20250609-math-rust-v1-v1-1-285fac00031f@samsung.com/
>
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/7] pwm: Add Rust driver for T-HEAD TH1520 SoC
2025-06-11 20:04 ` Michal Wilczynski
2025-06-11 21:15 ` Michal Wilczynski
@ 2025-06-11 21:40 ` Uwe Kleine-König
1 sibling, 0 replies; 27+ messages in thread
From: Uwe Kleine-König @ 2025-06-11 21:40 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
[-- Attachment #1: Type: text/plain, Size: 3116 bytes --]
Hello Michal,
On Wed, Jun 11, 2025 at 10:04:54PM +0200, Michal Wilczynski wrote:
> On 6/11/25 08:58, Uwe Kleine-König wrote:
> > On Tue, Jun 10, 2025 at 02:52:50PM +0200, Michal Wilczynski wrote:
> > Some comments use 2 and other 3 slashes. Does this have any semantic?
>
> Yes, they have a semantic difference. /// denotes a documentation
> comment that is processed by rustdoc to generate documentation, while //
> is a regular implementation comment. The compiler is configured to
> require documentation for public items (like structs and functions),
> which is why /// is used on the struct definition.
ok, thanks.
> >> + let period_ns = (period_cycles as u64)
> >> + .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
> >> + .unwrap_or(0);
> >
> > What does .unwrap_or(0) do? You need to round up in this mul_div
> > operation.
>
> The .unwrap_or(0) is to handle the case where the mul_div helper returns
> None, which can happen if the divisor (rate_hz) is zero. In that case,
It can *only* happen if rate_hz is zero? If we had
clk_rate_exclusive_get() we could rely on rate_hz being non-zero.
> the period becomes 0. The mul_div helper is introduced in this commit
> [1].
>
> [1] - https://lore.kernel.org/all/20250609-math-rust-v1-v1-1-285fac00031f@samsung.com/
>
> >
> >> + let period_cycles = wf
> >> + .period_length_ns
> >> + .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
> >> + .ok_or(EINVAL)?;
> >
> > If period_length_ns is BIG, pick the biggest possible period_cycles
> > value, not EINVAL.
>
> In this case EINVAL mean the function would return EINVAL not
> 'period_cycles' = EINVAL. This won't happen here since
> time::NSEC_PER_SEC is a constant, so this won't return None. This is not
> checking for overflow.
OK, so this is only there to please the rust compiler, right?
> >
> >> + if period_cycles > u32::MAX as u64 {
>
> In here I could pick period_cycles = u32::MAX.
indeed.
> >
> >> + iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
> >> + iomap.write32(wfhw.period_cycles, th1520_pwm_per(hwpwm));
> >> + iomap.write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm));
> >> + iomap.write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
> >> +
> >> + if !was_enabled {
> >> + iomap.write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm));
> >
> > Can this be combined with the above write?
>
> Per the TH1520 peripheral manual [2] (chapter 6.6.2.1), the PWM_START bit
> should only be asserted when enabling the PWM for the first time, not
> during a reconfiguration of an alreadyrunning channel. The separate if
> statement is there to handle this specific hardware requirement.
Yeah, I wondered if you could simplify that to (well, or make it a bit
faster at least) using:
ctrl_val = wfhw.ctrl_val | PWM_CFG_UPDATE;
if !was_enabled {
ctrl_val |= PWM_START;
}
iomap.write32(ctrl_val, th1520_pwm_ctrl(hwpwm));
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/7] pwm: Add Rust driver for T-HEAD TH1520 SoC
2025-06-11 6:58 ` Uwe Kleine-König
2025-06-11 19:52 ` Michal Wilczynski
2025-06-11 20:04 ` Michal Wilczynski
@ 2025-06-12 8:14 ` Michal Wilczynski
2025-06-12 20:36 ` Uwe Kleine-König
2 siblings, 1 reply; 27+ messages in thread
From: Michal Wilczynski @ 2025-06-12 8:14 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On 6/11/25 08:58, Uwe Kleine-König wrote:
> Hello,
>
> Huh, if you do the newstyle stuff, .get_state() is wrong. It's either
> .round_waveform_tohw() + .round_waveform_fromhw() + .read_waveform() +
> .write_waveform() or .apply() + .get_state(), but don't mix these.
In the process of implementing the full "newstyle" waveform API as you
suggested, I discovered a hardware limitation. After writing new values
to the period and duty cycle registers, reading them back does not
return the programmed values, which makes it impossible to reliably
report the current hardware state.
This appears to be a known quirk of the hardware, as the reference C
driver from T-HEAD [1] also omits the .get_state callback, likely for
the same reason.
Given this, would it be acceptable to provide a write-only driver? My
proposed solution would be to omit the .read_waveform() and
.round_waveform_fromhw() implementations from my PwmOps trait. This
would mean the driver can correctly set the PWM state, but attempting to
read it back via sysfs would fail (e.g., with -EOPNOTSUPP), reflecting
the hardware's capability.
Does this sound like a reasonable approach to you?
[1] - https://github.com/revyos/thead-kernel/blob/lpi4a/drivers/pwm/pwm-light.c
>
>> + let data: &Self = chip.drvdata().ok_or(EINVAL)?;
>> + let hwpwm = pwm.hwpwm();
>> + let iomem_guard = data.iomem.access(parent_dev)?;
>> + let iomap = iomem_guard.deref();
>> + let ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
>> + let period_cycles = iomap.read32(th1520_pwm_per(hwpwm));
>> + let duty_cycles = iomap.read32(th1520_pwm_fp(hwpwm));
>> +
>> + state.set_enabled(duty_cycles != 0);
>> +
>> + let rate_hz = data.clk.rate().as_hz();
>> + let period_ns = (period_cycles as u64)
>> + .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
>> + .unwrap_or(0);
> Best regards
> Uwe
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/7] pwm: Add Rust driver for T-HEAD TH1520 SoC
2025-06-12 8:14 ` Michal Wilczynski
@ 2025-06-12 20:36 ` Uwe Kleine-König
2025-06-17 11:55 ` Michal Wilczynski
0 siblings, 1 reply; 27+ messages in thread
From: Uwe Kleine-König @ 2025-06-12 20:36 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]
Hello Michael,
On Thu, Jun 12, 2025 at 10:14:13AM +0200, Michal Wilczynski wrote:
> On 6/11/25 08:58, Uwe Kleine-König wrote:
> > Huh, if you do the newstyle stuff, .get_state() is wrong. It's either
> > .round_waveform_tohw() + .round_waveform_fromhw() + .read_waveform() +
> > .write_waveform() or .apply() + .get_state(), but don't mix these.
>
> In the process of implementing the full "newstyle" waveform API as you
> suggested, I discovered a hardware limitation. After writing new values
> to the period and duty cycle registers, reading them back does not
> return the programmed values, which makes it impossible to reliably
> report the current hardware state.
>
> This appears to be a known quirk of the hardware, as the reference C
> driver from T-HEAD [1] also omits the .get_state callback, likely for
> the same reason.
Do you read complete non-sense or e.g. the old configuration until
the current period ends?
I guess would be that .get_state wasn't implemented because this is an
oldoldstyle driver and it works also without that function.
> Given this, would it be acceptable to provide a write-only driver? My
> proposed solution would be to omit the .read_waveform() and
> .round_waveform_fromhw() implementations from my PwmOps trait. This
Please don't skip .round_waveform_fromhw(), that one is needed for
pwm_round_waveform_might_sleep().
I don't like it, but given that the hardware doesn't play along there is
no alternative.
> would mean the driver can correctly set the PWM state, but attempting to
> read it back via sysfs would fail (e.g., with -EOPNOTSUPP), reflecting
> the hardware's capability.
I think there might be another patch opportunity then to make PWM_DEBUG
work with that.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/7] pwm: Add Rust driver for T-HEAD TH1520 SoC
2025-06-12 20:36 ` Uwe Kleine-König
@ 2025-06-17 11:55 ` Michal Wilczynski
0 siblings, 0 replies; 27+ messages in thread
From: Michal Wilczynski @ 2025-06-17 11:55 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On 6/12/25 22:36, Uwe Kleine-König wrote:
> Hello Michael,
>
> On Thu, Jun 12, 2025 at 10:14:13AM +0200, Michal Wilczynski wrote:
>> On 6/11/25 08:58, Uwe Kleine-König wrote:
>>> Huh, if you do the newstyle stuff, .get_state() is wrong. It's either
>>> .round_waveform_tohw() + .round_waveform_fromhw() + .read_waveform() +
>>> .write_waveform() or .apply() + .get_state(), but don't mix these.
>>
>> In the process of implementing the full "newstyle" waveform API as you
>> suggested, I discovered a hardware limitation. After writing new values
>> to the period and duty cycle registers, reading them back does not
>> return the programmed values, which makes it impossible to reliably
>> report the current hardware state.
>>
>> This appears to be a known quirk of the hardware, as the reference C
>> driver from T-HEAD [1] also omits the .get_state callback, likely for
>> the same reason.
>
> Do you read complete non-sense or e.g. the old configuration until
> the current period ends?
>
> I guess would be that .get_state wasn't implemented because this is an
> oldoldstyle driver and it works also without that function.
Hi Uwe,
My apologies for the confusion. After further testing, it appears I was
mistaken, and the hardware reads are working correctly. I must have made
an error when testing via sysfs.
I'll be submitting a v3 that implements the full round_waveform_tohw(),
round_waveform_fromhw(), read_waveform(), and write_waveform()
combination as you initially suggested.
>
>> Given this, would it be acceptable to provide a write-only driver? My
>> proposed solution would be to omit the .read_waveform() and
>> .round_waveform_fromhw() implementations from my PwmOps trait. This
>
> Please don't skip .round_waveform_fromhw(), that one is needed for
> pwm_round_waveform_might_sleep().
>
> I don't like it, but given that the hardware doesn't play along there is
> no alternative.
>
>> would mean the driver can correctly set the PWM state, but attempting to
>> read it back via sysfs would fail (e.g., with -EOPNOTSUPP), reflecting
>> the hardware's capability.
>
> I think there might be another patch opportunity then to make PWM_DEBUG
> work with that.
>
> Best regards
> Uwe
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 3/7] clk: thead: Mark essential bus clocks as CLK_IGNORE_UNUSED
2025-06-10 12:52 ` [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
2025-06-10 12:52 ` [PATCH v2 1/7] rust: Add basic PWM abstractions Michal Wilczynski
2025-06-10 12:52 ` [PATCH v2 2/7] pwm: Add Rust driver for T-HEAD TH1520 SoC Michal Wilczynski
@ 2025-06-10 12:52 ` Michal Wilczynski
2025-06-15 18:03 ` Drew Fustini
2025-06-10 12:52 ` [PATCH v2 4/7] dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller Michal Wilczynski
` (4 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Michal Wilczynski @ 2025-06-10 12:52 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Probing peripherals in the AON and PERI domains, such as the PVT thermal
sensor and the PWM controller, can lead to boot hangs or unresponsive
devices on the LPi4A board. The root cause is that their parent bus
clocks ('CLK_CPU2AON_X2H' and the 'CLK_PERISYS_APB' clocks) are
automatically gated by the kernel's power-saving mechanisms when the bus
is perceived as idle.
Alternative solutions were investigated, including modeling the parent
bus in the Device Tree with 'simple-pm-bus' or refactoring the clock
driver's parentage. The 'simple-pm-bus' approach is not viable due to
the lack of defined bus address ranges in the hardware manual and its
creation of improper dependencies on the 'pm_runtime' API for consumer
drivers.
Therefore, applying the'`CLK_IGNORE_UNUSED' flag directly to the
essential bus clocks is the most direct and targeted fix. This prevents
the kernel from auto-gating these buses and ensures peripherals remain
accessible.
This change fixes the boot hang associated with the PVT sensor and
resolves the functional issues with the PWM controller.
[1] - https://lore.kernel.org/all/9e8a12db-236d-474c-b110-b3be96edf057@samsung.com/
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
drivers/clk/thead/clk-th1520-ap.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
index ebfb1d59401d05443716eb0029403b01775e8f73..cf7f6bd428a0faa4611b3fc61edbbc6690e565d9 100644
--- a/drivers/clk/thead/clk-th1520-ap.c
+++ b/drivers/clk/thead/clk-th1520-ap.c
@@ -792,11 +792,12 @@ static CCU_GATE(CLK_AON2CPU_A2X, aon2cpu_a2x_clk, "aon2cpu-a2x", axi4_cpusys2_ac
0x134, BIT(8), 0);
static CCU_GATE(CLK_X2X_CPUSYS, x2x_cpusys_clk, "x2x-cpusys", axi4_cpusys2_aclk_pd,
0x134, BIT(7), 0);
-static CCU_GATE(CLK_CPU2AON_X2H, cpu2aon_x2h_clk, "cpu2aon-x2h", axi_aclk_pd, 0x138, BIT(8), 0);
+static CCU_GATE(CLK_CPU2AON_X2H, cpu2aon_x2h_clk, "cpu2aon-x2h", axi_aclk_pd,
+ 0x138, BIT(8), CLK_IGNORE_UNUSED);
static CCU_GATE(CLK_CPU2PERI_X2H, cpu2peri_x2h_clk, "cpu2peri-x2h", axi4_cpusys2_aclk_pd,
0x140, BIT(9), CLK_IGNORE_UNUSED);
static CCU_GATE(CLK_PERISYS_APB1_HCLK, perisys_apb1_hclk, "perisys-apb1-hclk", perisys_ahb_hclk_pd,
- 0x150, BIT(9), 0);
+ 0x150, BIT(9), CLK_IGNORE_UNUSED);
static CCU_GATE(CLK_PERISYS_APB2_HCLK, perisys_apb2_hclk, "perisys-apb2-hclk", perisys_ahb_hclk_pd,
0x150, BIT(10), CLK_IGNORE_UNUSED);
static CCU_GATE(CLK_PERISYS_APB3_HCLK, perisys_apb3_hclk, "perisys-apb3-hclk", perisys_ahb_hclk_pd,
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 3/7] clk: thead: Mark essential bus clocks as CLK_IGNORE_UNUSED
2025-06-10 12:52 ` [PATCH v2 3/7] clk: thead: Mark essential bus clocks as CLK_IGNORE_UNUSED Michal Wilczynski
@ 2025-06-15 18:03 ` Drew Fustini
0 siblings, 0 replies; 27+ messages in thread
From: Drew Fustini @ 2025-06-15 18:03 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On Tue, Jun 10, 2025 at 02:52:51PM +0200, Michal Wilczynski wrote:
> Probing peripherals in the AON and PERI domains, such as the PVT thermal
> sensor and the PWM controller, can lead to boot hangs or unresponsive
> devices on the LPi4A board. The root cause is that their parent bus
> clocks ('CLK_CPU2AON_X2H' and the 'CLK_PERISYS_APB' clocks) are
> automatically gated by the kernel's power-saving mechanisms when the bus
> is perceived as idle.
>
> Alternative solutions were investigated, including modeling the parent
> bus in the Device Tree with 'simple-pm-bus' or refactoring the clock
> driver's parentage. The 'simple-pm-bus' approach is not viable due to
> the lack of defined bus address ranges in the hardware manual and its
> creation of improper dependencies on the 'pm_runtime' API for consumer
> drivers.
>
> Therefore, applying the'`CLK_IGNORE_UNUSED' flag directly to the
> essential bus clocks is the most direct and targeted fix. This prevents
> the kernel from auto-gating these buses and ensures peripherals remain
> accessible.
>
> This change fixes the boot hang associated with the PVT sensor and
> resolves the functional issues with the PWM controller.
>
> [1] - https://lore.kernel.org/all/9e8a12db-236d-474c-b110-b3be96edf057@samsung.com/
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> drivers/clk/thead/clk-th1520-ap.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
> index ebfb1d59401d05443716eb0029403b01775e8f73..cf7f6bd428a0faa4611b3fc61edbbc6690e565d9 100644
> --- a/drivers/clk/thead/clk-th1520-ap.c
> +++ b/drivers/clk/thead/clk-th1520-ap.c
> @@ -792,11 +792,12 @@ static CCU_GATE(CLK_AON2CPU_A2X, aon2cpu_a2x_clk, "aon2cpu-a2x", axi4_cpusys2_ac
> 0x134, BIT(8), 0);
> static CCU_GATE(CLK_X2X_CPUSYS, x2x_cpusys_clk, "x2x-cpusys", axi4_cpusys2_aclk_pd,
> 0x134, BIT(7), 0);
> -static CCU_GATE(CLK_CPU2AON_X2H, cpu2aon_x2h_clk, "cpu2aon-x2h", axi_aclk_pd, 0x138, BIT(8), 0);
> +static CCU_GATE(CLK_CPU2AON_X2H, cpu2aon_x2h_clk, "cpu2aon-x2h", axi_aclk_pd,
> + 0x138, BIT(8), CLK_IGNORE_UNUSED);
> static CCU_GATE(CLK_CPU2PERI_X2H, cpu2peri_x2h_clk, "cpu2peri-x2h", axi4_cpusys2_aclk_pd,
> 0x140, BIT(9), CLK_IGNORE_UNUSED);
> static CCU_GATE(CLK_PERISYS_APB1_HCLK, perisys_apb1_hclk, "perisys-apb1-hclk", perisys_ahb_hclk_pd,
> - 0x150, BIT(9), 0);
> + 0x150, BIT(9), CLK_IGNORE_UNUSED);
> static CCU_GATE(CLK_PERISYS_APB2_HCLK, perisys_apb2_hclk, "perisys-apb2-hclk", perisys_ahb_hclk_pd,
> 0x150, BIT(10), CLK_IGNORE_UNUSED);
> static CCU_GATE(CLK_PERISYS_APB3_HCLK, perisys_apb3_hclk, "perisys-apb3-hclk", perisys_ahb_hclk_pd,
>
> --
> 2.34.1
>
I'm okay with fixing it this way for now and revisiting the parent
relationships later.
Reviewed-by: Drew Fustini <drew@pdp7.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 4/7] dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller
2025-06-10 12:52 ` [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
` (2 preceding siblings ...)
2025-06-10 12:52 ` [PATCH v2 3/7] clk: thead: Mark essential bus clocks as CLK_IGNORE_UNUSED Michal Wilczynski
@ 2025-06-10 12:52 ` Michal Wilczynski
2025-06-16 6:53 ` Krzysztof Kozlowski
2025-06-10 12:52 ` [PATCH v2 5/7] riscv: dts: thead: Add PWM controller node Michal Wilczynski
` (3 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Michal Wilczynski @ 2025-06-10 12:52 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Add the Device Tree binding documentation for the T-HEAD
TH1520 SoC PWM controller.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
.../devicetree/bindings/pwm/thead,th1520-pwm.yaml | 48 ++++++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 49 insertions(+)
diff --git a/Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml b/Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..855aec59ac53c430adc849271235686e87b10e6c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/thead,th1520-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: T-HEAD TH1520 PWM controller
+
+maintainers:
+ - Michal Wilczynski <m.wilczynski@samsung.com>
+
+allOf:
+ - $ref: pwm.yaml#
+
+properties:
+ compatible:
+ const: thead,th1520-pwm
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: SoC PWM clock
+
+ "#pwm-cells":
+ const: 3
+
+required:
+ - compatible
+ - reg
+ - clocks
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/thead,th1520-clk-ap.h>
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ pwm@ffec01c000 {
+ compatible = "thead,th1520-pwm";
+ reg = <0xff 0xec01c000 0x0 0x4000>;
+ clocks = <&clk CLK_PWM>;
+ #pwm-cells = <3>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 966ce515c8bfefdff1975bb716a267435ec0feae..c8a4a2d4f55af213fa6ad2911f972990210ec950 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21311,6 +21311,7 @@ F: Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
F: Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
F: Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
F: Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
+F: Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml
F: Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
F: arch/riscv/boot/dts/thead/
F: drivers/clk/thead/clk-th1520-ap.c
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 4/7] dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller
2025-06-10 12:52 ` [PATCH v2 4/7] dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller Michal Wilczynski
@ 2025-06-16 6:53 ` Krzysztof Kozlowski
0 siblings, 0 replies; 27+ messages in thread
From: Krzysztof Kozlowski @ 2025-06-16 6:53 UTC (permalink / raw)
To: Michal Wilczynski, Uwe Kleine-König, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
Drew Fustini, Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
On 10/06/2025 14:52, Michal Wilczynski wrote:
> Add the Device Tree binding documentation for the T-HEAD
> TH1520 SoC PWM controller.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 5/7] riscv: dts: thead: Add PWM controller node
2025-06-10 12:52 ` [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
` (3 preceding siblings ...)
2025-06-10 12:52 ` [PATCH v2 4/7] dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller Michal Wilczynski
@ 2025-06-10 12:52 ` Michal Wilczynski
2025-06-10 12:52 ` [PATCH v2 6/7] riscv: dts: thead: Add PVT node Michal Wilczynski
` (2 subsequent siblings)
7 siblings, 0 replies; 27+ messages in thread
From: Michal Wilczynski @ 2025-06-10 12:52 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Add the Device Tree node for the T-HEAD TH1520 SoC's PWM controller.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
arch/riscv/boot/dts/thead/th1520.dtsi | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 1db0054c4e093400e9dbebcee5fcfa5b5cae6e32..26996422e1efe5d2dde68819c2cec1c3fa782a23 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -490,6 +490,13 @@ uart2: serial@ffec010000 {
status = "disabled";
};
+ pwm: pwm@ffec01c000 {
+ compatible = "thead,th1520-pwm";
+ reg = <0xff 0xec01c000 0x0 0x4000>;
+ clocks = <&clk CLK_PWM>;
+ #pwm-cells = <3>;
+ };
+
clk: clock-controller@ffef010000 {
compatible = "thead,th1520-clk-ap";
reg = <0xff 0xef010000 0x0 0x1000>;
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v2 6/7] riscv: dts: thead: Add PVT node
2025-06-10 12:52 ` [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
` (4 preceding siblings ...)
2025-06-10 12:52 ` [PATCH v2 5/7] riscv: dts: thead: Add PWM controller node Michal Wilczynski
@ 2025-06-10 12:52 ` Michal Wilczynski
2025-06-10 12:52 ` [PATCH v2 7/7] riscv: dts: thead: Add PWM fan and thermal control Michal Wilczynski
2025-06-10 21:10 ` [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver Drew Fustini
7 siblings, 0 replies; 27+ messages in thread
From: Michal Wilczynski @ 2025-06-10 12:52 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Add PVT DT node for thermal sensor.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 26996422e1efe5d2dde68819c2cec1c3fa782a23..bef30780034e06b07aa29b27b0225ea891a4b531 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -669,6 +669,17 @@ padctrl_aosys: pinctrl@fffff4a000 {
thead,pad-group = <1>;
};
+ pvt: pvt@fffff4e000 {
+ compatible = "moortec,mr75203";
+ reg = <0xff 0xfff4e000 0x0 0x80>,
+ <0xff 0xfff4e080 0x0 0x100>,
+ <0xff 0xfff4e180 0x0 0x680>,
+ <0xff 0xfff4e800 0x0 0x600>;
+ reg-names = "common", "ts", "pd", "vm";
+ clocks = <&aonsys_clk>;
+ #thermal-sensor-cells = <1>;
+ };
+
gpio@fffff52000 {
compatible = "snps,dw-apb-gpio";
reg = <0xff 0xfff52000 0x0 0x1000>;
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v2 7/7] riscv: dts: thead: Add PWM fan and thermal control
2025-06-10 12:52 ` [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
` (5 preceding siblings ...)
2025-06-10 12:52 ` [PATCH v2 6/7] riscv: dts: thead: Add PVT node Michal Wilczynski
@ 2025-06-10 12:52 ` Michal Wilczynski
2025-06-10 21:10 ` [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver Drew Fustini
7 siblings, 0 replies; 27+ messages in thread
From: Michal Wilczynski @ 2025-06-10 12:52 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Add Device Tree nodes to enable a PWM controlled fan and it's associated
thermal management for the Lichee Pi 4A board.
This enables temperature-controlled active cooling for the Lichee Pi 4A
board based on SoC temperature.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts | 67 +++++++++++++++++++++++
1 file changed, 67 insertions(+)
diff --git a/arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts b/arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts
index 4020c727f09e8e2286fdc7fecd79dbd8eba69556..c58c2085ca92a3234f1350500cedae4157f0c35f 100644
--- a/arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts
+++ b/arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts
@@ -28,9 +28,76 @@ aliases {
chosen {
stdout-path = "serial0:115200n8";
};
+
+ thermal-zones {
+ cpu-thermal {
+ polling-delay = <1000>;
+ polling-delay-passive = <1000>;
+ thermal-sensors = <&pvt 0>;
+
+ trips {
+ fan_config0: fan-trip0 {
+ temperature = <39000>;
+ hysteresis = <5000>;
+ type = "active";
+ };
+
+ fan_config1: fan-trip1 {
+ temperature = <50000>;
+ hysteresis = <5000>;
+ type = "active";
+ };
+
+ fan_config2: fan-trip2 {
+ temperature = <60000>;
+ hysteresis = <5000>;
+ type = "active";
+ };
+ };
+
+ cooling-maps {
+ map-active-0 {
+ cooling-device = <&fan 1 1>;
+ trip = <&fan_config0>;
+ };
+
+ map-active-1 {
+ cooling-device = <&fan 2 2>;
+ trip = <&fan_config1>;
+ };
+
+ map-active-2 {
+ cooling-device = <&fan 3 3>;
+ trip = <&fan_config2>;
+ };
+ };
+ };
+ };
+
+ fan: pwm-fan {
+ pinctrl-names = "default";
+ pinctrl-0 = <&fan_pins>;
+ compatible = "pwm-fan";
+ #cooling-cells = <2>;
+ pwms = <&pwm 1 10000000 0>;
+ cooling-levels = <0 66 196 255>;
+ };
+
};
&padctrl0_apsys {
+ fan_pins: fan-0 {
+ pwm1-pins {
+ pins = "GPIO3_3"; /* PWM1 */
+ function = "pwm";
+ bias-disable;
+ drive-strength = <25>;
+ input-disable;
+ input-schmitt-disable;
+ slew-rate = <0>;
+ };
+ };
+
uart0_pins: uart0-0 {
tx-pins {
pins = "UART0_TXD";
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver
2025-06-10 12:52 ` [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
` (6 preceding siblings ...)
2025-06-10 12:52 ` [PATCH v2 7/7] riscv: dts: thead: Add PWM fan and thermal control Michal Wilczynski
@ 2025-06-10 21:10 ` Drew Fustini
2025-06-11 15:14 ` Michal Wilczynski
7 siblings, 1 reply; 27+ messages in thread
From: Drew Fustini @ 2025-06-10 21:10 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On Tue, Jun 10, 2025 at 02:52:48PM +0200, Michal Wilczynski wrote:
> This patch series introduces Rust support for the T-HEAD TH1520 PWM
> controller and demonstrates its use for fan control on the Sipeed Lichee
> Pi 4A board.
>
> The primary goal of this patch series is to introduce a basic set of
> Rust abstractions for the Linux PWM subsystem. As a first user and
> practical demonstration of these abstractions, the series also provides
> a functional PWM driver for the T-HEAD TH1520 SoC. This allows control
> of its PWM channels and ultimately enables temperature controlled fan
> support for the Lichee Pi 4A board. This work aims to explore the use of
> Rust for PWM drivers and lay a foundation for potential future
> Rust based PWM drivers.
>
> The core of this series is a new rust/kernel/pwm.rs module that provides
> abstractions for writing PWM chip provider drivers in Rust. This has
> been significantly reworked from v1 based on extensive feedback. The key
> features of the new abstraction layer include:
>
> - Ownership and Lifetime Management: The pwm::Chip wrapper is managed
> by ARef, correctly tying its lifetime to its embedded struct device
> reference counter. Chip registration is handled by a pwm::Registration
> RAII guard, which guarantees that pwmchip_add is always paired with
> pwmchip_remove, preventing resource leaks.
>
> - Modern and Safe API: The PwmOps trait is now based on the modern
> waveform API (round_waveform_tohw, write_waveform, etc.) as recommended
> by the subsystem maintainer. It is generic over a driver's
> hardware specific data structure, moving all unsafe serialization logic
> into the abstraction layer and allowing drivers to be written in 100%
> safe Rust.
>
> - Ergonomics: The API provides safe, idiomatic wrappers for other PWM
> types (State, Args, Device, etc.) and uses standard kernel error
> handling patterns.
>
> The series is structured as follows:
> - Rust PWM Abstractions: The new safe abstraction layer.
> - TH1520 PWM Driver: A new Rust driver for the TH1520 SoC, built on
> top of the new abstractions.
> - Clock Fix: A necessary fix to the TH1520 clock driver to ensure bus
> clocks remain enabled.
> - Device Tree Bindings & Nodes: The remaining patches add the necessary
> DT bindings and nodes for the TH1520 PWM controller, a thermal
> sensor, and the PWM fan configuration for the Lichee Pi 4A board.
>
> Testing:
> Tested on the TH1520 SoC. The fan works correctly. The duty/period
> calculaties are correct. Fan starts slow when the chip is not hot and
> gradually increases the speed when PVT reports higher temperatures.
>
> The patches are based on mainline, with some dependencies which are not
> merged yet - platform Io support [1] and math wrapper [2].
>
> Reference repository with all the patches together can be found on
> github [3].
I'm trying to build your rust-next-pwm-working-fan-for-sending-v4 branch
but I get this error:
$ make W=1 LLVM=1 ARCH=riscv -j16
CALL scripts/checksyscalls.sh
.pylintrc: warning: ignored by one of the .gitignore files
UPD include/generated/utsversion.h
CC init/version-timestamp.o
KSYMS .tmp_vmlinux0.kallsyms.S
AS .tmp_vmlinux0.kallsyms.o
LD .tmp_vmlinux1
ld.lld: error: undefined symbol: rust_build_error
referenced by pwm_th1520.4789668fc0b4e501-cgu.0
drivers/pwm/pwm_th1520.o:(<pwm_th1520::Th1520PwmDriverData as kernel::pwm::PwmOps>::get_state) in archive vmlinux.a
referenced by pwm_th1520.4789668fc0b4e501-cgu.0
drivers/pwm/pwm_th1520.o:(<pwm_th1520::Th1520PwmDriverData as kernel::pwm::PwmOps>::write_waveform) in archive vmlinux.a
referenced by pwm_th1520.4789668fc0b4e501-cgu.0
drivers/pwm/pwm_th1520.o:(<pwm_th1520::Th1520PwmDriverData as kernel::pwm::PwmOps>::write_waveform) in archive vmlinux.a
make[2]: *** [scripts/Makefile.vmlinux:91: vmlinux] Error 1
make[1]: *** [/home/pdp7/linux/Makefile:1241: vmlinux] Error 2
make: *** [Makefile:248: __sub-make] Error 2
I've uploaded the config to:
https://gist.github.com/pdp7/e2c34dd7e4349a54bd67b53254bd3a22
Thanks,
Drew
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver
2025-06-10 21:10 ` [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver Drew Fustini
@ 2025-06-11 15:14 ` Michal Wilczynski
2025-06-11 16:55 ` Miguel Ojeda
2025-06-11 23:52 ` Drew Fustini
0 siblings, 2 replies; 27+ messages in thread
From: Michal Wilczynski @ 2025-06-11 15:14 UTC (permalink / raw)
To: Drew Fustini
Cc: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On 6/10/25 23:10, Drew Fustini wrote:
> On Tue, Jun 10, 2025 at 02:52:48PM +0200, Michal Wilczynski wrote:
>> This patch series introduces Rust support for the T-HEAD TH1520 PWM
>> controller and demonstrates its use for fan control on the Sipeed Lichee
>> Pi 4A board.
>>
>> The primary goal of this patch series is to introduce a basic set of
>> Rust abstractions for the Linux PWM subsystem. As a first user and
>> practical demonstration of these abstractions, the series also provides
>> a functional PWM driver for the T-HEAD TH1520 SoC. This allows control
>> of its PWM channels and ultimately enables temperature controlled fan
>> support for the Lichee Pi 4A board. This work aims to explore the use of
>> Rust for PWM drivers and lay a foundation for potential future
>> Rust based PWM drivers.
>>
>> The core of this series is a new rust/kernel/pwm.rs module that provides
>> abstractions for writing PWM chip provider drivers in Rust. This has
>> been significantly reworked from v1 based on extensive feedback. The key
>> features of the new abstraction layer include:
>>
>> - Ownership and Lifetime Management: The pwm::Chip wrapper is managed
>> by ARef, correctly tying its lifetime to its embedded struct device
>> reference counter. Chip registration is handled by a pwm::Registration
>> RAII guard, which guarantees that pwmchip_add is always paired with
>> pwmchip_remove, preventing resource leaks.
>>
>> - Modern and Safe API: The PwmOps trait is now based on the modern
>> waveform API (round_waveform_tohw, write_waveform, etc.) as recommended
>> by the subsystem maintainer. It is generic over a driver's
>> hardware specific data structure, moving all unsafe serialization logic
>> into the abstraction layer and allowing drivers to be written in 100%
>> safe Rust.
>>
>> - Ergonomics: The API provides safe, idiomatic wrappers for other PWM
>> types (State, Args, Device, etc.) and uses standard kernel error
>> handling patterns.
>>
>> The series is structured as follows:
>> - Rust PWM Abstractions: The new safe abstraction layer.
>> - TH1520 PWM Driver: A new Rust driver for the TH1520 SoC, built on
>> top of the new abstractions.
>> - Clock Fix: A necessary fix to the TH1520 clock driver to ensure bus
>> clocks remain enabled.
>> - Device Tree Bindings & Nodes: The remaining patches add the necessary
>> DT bindings and nodes for the TH1520 PWM controller, a thermal
>> sensor, and the PWM fan configuration for the Lichee Pi 4A board.
>>
>> Testing:
>> Tested on the TH1520 SoC. The fan works correctly. The duty/period
>> calculaties are correct. Fan starts slow when the chip is not hot and
>> gradually increases the speed when PVT reports higher temperatures.
>>
>> The patches are based on mainline, with some dependencies which are not
>> merged yet - platform Io support [1] and math wrapper [2].
>>
>> Reference repository with all the patches together can be found on
>> github [3].
>
> I'm trying to build your rust-next-pwm-working-fan-for-sending-v4 branch
> but I get this error:
>
> $ make W=1 LLVM=1 ARCH=riscv -j16
> CALL scripts/checksyscalls.sh
> .pylintrc: warning: ignored by one of the .gitignore files
> UPD include/generated/utsversion.h
> CC init/version-timestamp.o
> KSYMS .tmp_vmlinux0.kallsyms.S
> AS .tmp_vmlinux0.kallsyms.o
> LD .tmp_vmlinux1
> ld.lld: error: undefined symbol: rust_build_error
> referenced by pwm_th1520.4789668fc0b4e501-cgu.0
> drivers/pwm/pwm_th1520.o:(<pwm_th1520::Th1520PwmDriverData as kernel::pwm::PwmOps>::get_state) in archive vmlinux.a
> referenced by pwm_th1520.4789668fc0b4e501-cgu.0
> drivers/pwm/pwm_th1520.o:(<pwm_th1520::Th1520PwmDriverData as kernel::pwm::PwmOps>::write_waveform) in archive vmlinux.a
> referenced by pwm_th1520.4789668fc0b4e501-cgu.0
> drivers/pwm/pwm_th1520.o:(<pwm_th1520::Th1520PwmDriverData as kernel::pwm::PwmOps>::write_waveform) in archive vmlinux.a
> make[2]: *** [scripts/Makefile.vmlinux:91: vmlinux] Error 1
> make[1]: *** [/home/pdp7/linux/Makefile:1241: vmlinux] Error 2
> make: *** [Makefile:248: __sub-make] Error 2
Hi,
Thanks for testing !
I can reproduce the issue with your config.
The root of the problem was a failing compile time assertion
(build_assert!) in the underlying Rust abstracions, I think IoMem since
get_state and write_waveform functions are impacted. My development
configuration was accidentally hiding this issue, but your configuration
correctly exposed it.
The kernel config option that is different on my setup is:
CONFIG_RUST_BUILD_ASSERT_ALLOW=y
Now I have to take a look at the IoMem abstractions, I think there is
a new revision [1]. Will apply it and check why exactly compile
assertions are triggering.
[1] - https://lore.kernel.org/all/20250603-topics-tyr-platform_iomem-v9-0-a27e04157e3e@collabora.com/
>
> I've uploaded the config to:
> https://protect2.fireeye.com/v1/url?k=0cc1a518-535a9c14-0cc02e57-000babff3563-8df2dfc535042c2a&q=1&e=eaf92127-0b5a-4559-9796-3264da753ae3&u=https%3A%2F%2Fgist.github.com%2Fpdp7%2Fe2c34dd7e4349a54bd67b53254bd3a22
>
> Thanks,
> Drew
>
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver
2025-06-11 15:14 ` Michal Wilczynski
@ 2025-06-11 16:55 ` Miguel Ojeda
2025-06-11 23:52 ` Drew Fustini
1 sibling, 0 replies; 27+ messages in thread
From: Miguel Ojeda @ 2025-06-11 16:55 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Drew Fustini, Uwe Kleine-König, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich, Guo Ren, Fu Wei,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Alexandre Ghiti, Marek Szyprowski,
Benno Lossin, Michael Turquette, Stephen Boyd, linux-kernel,
linux-pwm, rust-for-linux, linux-riscv, devicetree, linux-clk
On Wed, Jun 11, 2025 at 5:14 PM Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
> The kernel config option that is different on my setup is:
> CONFIG_RUST_BUILD_ASSERT_ALLOW=y
Yeah, code must work with `CONFIG_RUST_BUILD_ASSERT_ALLOW=n` -- the
config is an escape hatch just in case a user toolchain cannot build
the code for some reason.
In other words, if a `build_assert!` is triggered, then that is a bug
(likely in new callers misusing an API, but it could also be in the
callees, of course).
We may eventually remove it, or perhaps invert its meaning so that
`allmodconfig` doesn't enable it, which is how I guess you ended up
with it, right?
I hope that helps.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver
2025-06-11 15:14 ` Michal Wilczynski
2025-06-11 16:55 ` Miguel Ojeda
@ 2025-06-11 23:52 ` Drew Fustini
2025-06-12 5:01 ` Uwe Kleine-König
1 sibling, 1 reply; 27+ messages in thread
From: Drew Fustini @ 2025-06-11 23:52 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On Wed, Jun 11, 2025 at 05:14:40PM +0200, Michal Wilczynski wrote:
>
>
> On 6/10/25 23:10, Drew Fustini wrote:
> > On Tue, Jun 10, 2025 at 02:52:48PM +0200, Michal Wilczynski wrote:
> >> This patch series introduces Rust support for the T-HEAD TH1520 PWM
> >> controller and demonstrates its use for fan control on the Sipeed Lichee
> >> Pi 4A board.
> >>
> >> The primary goal of this patch series is to introduce a basic set of
> >> Rust abstractions for the Linux PWM subsystem. As a first user and
> >> practical demonstration of these abstractions, the series also provides
> >> a functional PWM driver for the T-HEAD TH1520 SoC. This allows control
> >> of its PWM channels and ultimately enables temperature controlled fan
> >> support for the Lichee Pi 4A board. This work aims to explore the use of
> >> Rust for PWM drivers and lay a foundation for potential future
> >> Rust based PWM drivers.
> >>
> >> The core of this series is a new rust/kernel/pwm.rs module that provides
> >> abstractions for writing PWM chip provider drivers in Rust. This has
> >> been significantly reworked from v1 based on extensive feedback. The key
> >> features of the new abstraction layer include:
> >>
> >> - Ownership and Lifetime Management: The pwm::Chip wrapper is managed
> >> by ARef, correctly tying its lifetime to its embedded struct device
> >> reference counter. Chip registration is handled by a pwm::Registration
> >> RAII guard, which guarantees that pwmchip_add is always paired with
> >> pwmchip_remove, preventing resource leaks.
> >>
> >> - Modern and Safe API: The PwmOps trait is now based on the modern
> >> waveform API (round_waveform_tohw, write_waveform, etc.) as recommended
> >> by the subsystem maintainer. It is generic over a driver's
> >> hardware specific data structure, moving all unsafe serialization logic
> >> into the abstraction layer and allowing drivers to be written in 100%
> >> safe Rust.
> >>
> >> - Ergonomics: The API provides safe, idiomatic wrappers for other PWM
> >> types (State, Args, Device, etc.) and uses standard kernel error
> >> handling patterns.
> >>
> >> The series is structured as follows:
> >> - Rust PWM Abstractions: The new safe abstraction layer.
> >> - TH1520 PWM Driver: A new Rust driver for the TH1520 SoC, built on
> >> top of the new abstractions.
> >> - Clock Fix: A necessary fix to the TH1520 clock driver to ensure bus
> >> clocks remain enabled.
> >> - Device Tree Bindings & Nodes: The remaining patches add the necessary
> >> DT bindings and nodes for the TH1520 PWM controller, a thermal
> >> sensor, and the PWM fan configuration for the Lichee Pi 4A board.
> >>
> >> Testing:
> >> Tested on the TH1520 SoC. The fan works correctly. The duty/period
> >> calculaties are correct. Fan starts slow when the chip is not hot and
> >> gradually increases the speed when PVT reports higher temperatures.
> >>
> >> The patches are based on mainline, with some dependencies which are not
> >> merged yet - platform Io support [1] and math wrapper [2].
> >>
> >> Reference repository with all the patches together can be found on
> >> github [3].
> >
> > I'm trying to build your rust-next-pwm-working-fan-for-sending-v4 branch
> > but I get this error:
> >
> > $ make W=1 LLVM=1 ARCH=riscv -j16
> > CALL scripts/checksyscalls.sh
> > .pylintrc: warning: ignored by one of the .gitignore files
> > UPD include/generated/utsversion.h
> > CC init/version-timestamp.o
> > KSYMS .tmp_vmlinux0.kallsyms.S
> > AS .tmp_vmlinux0.kallsyms.o
> > LD .tmp_vmlinux1
> > ld.lld: error: undefined symbol: rust_build_error
> > referenced by pwm_th1520.4789668fc0b4e501-cgu.0
> > drivers/pwm/pwm_th1520.o:(<pwm_th1520::Th1520PwmDriverData as kernel::pwm::PwmOps>::get_state) in archive vmlinux.a
> > referenced by pwm_th1520.4789668fc0b4e501-cgu.0
> > drivers/pwm/pwm_th1520.o:(<pwm_th1520::Th1520PwmDriverData as kernel::pwm::PwmOps>::write_waveform) in archive vmlinux.a
> > referenced by pwm_th1520.4789668fc0b4e501-cgu.0
> > drivers/pwm/pwm_th1520.o:(<pwm_th1520::Th1520PwmDriverData as kernel::pwm::PwmOps>::write_waveform) in archive vmlinux.a
> > make[2]: *** [scripts/Makefile.vmlinux:91: vmlinux] Error 1
> > make[1]: *** [/home/pdp7/linux/Makefile:1241: vmlinux] Error 2
> > make: *** [Makefile:248: __sub-make] Error 2
>
> Hi,
>
> Thanks for testing !
> I can reproduce the issue with your config.
>
> The root of the problem was a failing compile time assertion
> (build_assert!) in the underlying Rust abstracions, I think IoMem since
> get_state and write_waveform functions are impacted. My development
> configuration was accidentally hiding this issue, but your configuration
> correctly exposed it.
>
> The kernel config option that is different on my setup is:
> CONFIG_RUST_BUILD_ASSERT_ALLOW=y
Thanks for the explanation. I wanted to see how far I could get so I
also have set CONFIG_RUST_BUILD_ASSERT_ALLOW=y for now.
I also enabled the pwm fan driver. However, there is a probe failure:
[ 1.250921] pwm-fan pwm-fan: Failed to configure PWM: -524
[ 1.256546] pwm-fan pwm-fan: probe with driver pwm-fan failed with error -524
This seems to be the result `set_pwm(ctx, initial_pwm)` failing.
It seems like the TH1520 PWM driver loaded okay:
# cat /sys/class/pwm/pwmchip0/npwm
6
# ls -l /sys/class/pwm/pwmchip0/device
lrwxrwxrwx 1 root root 0 Jun 12 07:37 /sys/class/pwm/pwmchip0/device -> ../../../ffec01c000.pwm
# ls -l /sys/class/pwm/pwmchip0/device/driver
lrwxrwxrwx 1 root root 0 Feb 28 2023 /sys/class/pwm/pwmchip0/device/driver -> ../../../../bus/platform/drivers/pwm-th1520
I'm using your mwilczy/rust-next-pwm-working-fan-for-sending-v4 branch:
7ec07c93dbac riscv: dts: thead: Add PWM fan and thermal control
c8a6138b2a13 riscv: dts: thead: Add PVT node
14e2f1bfd26b riscv: dts: thead: Add PWM controller node
afe06057030e dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller
fe75d1ab60c9 clk: thead: Mark essential bus clocks as CLK_IGNORE_UNUSED
47dc6a551376 pwm: Add Rust driver for T-HEAD TH1520 SoC
9370bdd31cdc rust: Add basic PWM abstractions
f077d5bf0be8 Rust Abstractions for PWM subsystem with TH1520 PWM driver
f153d0d0221f rust: math: Add KernelMathExt trait with a mul_div helper
51c4a2e7d48a Fix for Device<Bound>
4847fa4f7ac8 rust: platform: allow ioremap of platform resources
929c56df82e5 rust: io: mem: add a generic iomem abstraction
09dfabb4677c rust: io: add resource abstraction
I uploaded the kconfig [1] and boot log [2]. Any ideas?
Thanks,
Drew
[1] https://gist.github.com/pdp7/58ca1f543898daeb281c013facb79aed
[2] https://gist.github.com/pdp7/e263b1b928a17d499f94fa5be7b3a7f8
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver
2025-06-11 23:52 ` Drew Fustini
@ 2025-06-12 5:01 ` Uwe Kleine-König
2025-06-12 13:27 ` Michal Wilczynski
0 siblings, 1 reply; 27+ messages in thread
From: Uwe Kleine-König @ 2025-06-12 5:01 UTC (permalink / raw)
To: Drew Fustini
Cc: Michal Wilczynski, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
[-- Attachment #1: Type: text/plain, Size: 641 bytes --]
Hello Drew,
On Wed, Jun 11, 2025 at 04:52:22PM -0700, Drew Fustini wrote:
> I also enabled the pwm fan driver. However, there is a probe failure:
>
> [ 1.250921] pwm-fan pwm-fan: Failed to configure PWM: -524
> [ 1.256546] pwm-fan pwm-fan: probe with driver pwm-fan failed with error -524
524 = ENOTSUPP, so it seems the request had duty_offset > 0. Does your
fan use PWM_POLARITY_INVERTED? If so, try without that flag. If your fan
really needs an inverted PWM this of course makes fan control buggy.
With the next revision it should work fine (as a duty_offset > 0 should
get rounded down to 0).
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver
2025-06-12 5:01 ` Uwe Kleine-König
@ 2025-06-12 13:27 ` Michal Wilczynski
2025-06-12 17:49 ` Drew Fustini
0 siblings, 1 reply; 27+ messages in thread
From: Michal Wilczynski @ 2025-06-12 13:27 UTC (permalink / raw)
To: Uwe Kleine-König, Drew Fustini
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On 6/12/25 07:01, Uwe Kleine-König wrote:
> Hello Drew,
>
> On Wed, Jun 11, 2025 at 04:52:22PM -0700, Drew Fustini wrote:
>> I also enabled the pwm fan driver. However, there is a probe failure:
>>
>> [ 1.250921] pwm-fan pwm-fan: Failed to configure PWM: -524
>> [ 1.256546] pwm-fan pwm-fan: probe with driver pwm-fan failed with error -524
>
> 524 = ENOTSUPP, so it seems the request had duty_offset > 0. Does your
> fan use PWM_POLARITY_INVERTED? If so, try without that flag. If your fan
> really needs an inverted PWM this of course makes fan control buggy.
> With the next revision it should work fine (as a duty_offset > 0 should
> get rounded down to 0).
Since we're running the same DT, the polarity shouldn't be inverted. I
see you have CONFIG_PWM_DEBUG=y enabled, which is most likely the reason
the probe fails.
With that option, the following check is performed in __pwm_apply:
if (IS_ENABLED(CONFIG_PWM_DEBUG)) {
struct pwm_waveform wf_rounded;
err = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf_rounded);
if (err)
return err;
In this revision of the driver, I have not implemented the read-waveform
callbacks, so the Rust PWM abstractions correctly return -ENOTSUPP.
Uwe, this poses a problem, as reading from the duty and period registers
on the TH1520 SoC's PWM controller appears to be broken.
>
> Best regards
> Uwe
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver
2025-06-12 13:27 ` Michal Wilczynski
@ 2025-06-12 17:49 ` Drew Fustini
0 siblings, 0 replies; 27+ messages in thread
From: Drew Fustini @ 2025-06-12 17:49 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On Thu, Jun 12, 2025 at 03:27:09PM +0200, Michal Wilczynski wrote:
>
>
> On 6/12/25 07:01, Uwe Kleine-König wrote:
> > Hello Drew,
> >
> > On Wed, Jun 11, 2025 at 04:52:22PM -0700, Drew Fustini wrote:
> >> I also enabled the pwm fan driver. However, there is a probe failure:
> >>
> >> [ 1.250921] pwm-fan pwm-fan: Failed to configure PWM: -524
> >> [ 1.256546] pwm-fan pwm-fan: probe with driver pwm-fan failed with error -524
> >
> > 524 = ENOTSUPP, so it seems the request had duty_offset > 0. Does your
> > fan use PWM_POLARITY_INVERTED? If so, try without that flag. If your fan
> > really needs an inverted PWM this of course makes fan control buggy.
> > With the next revision it should work fine (as a duty_offset > 0 should
> > get rounded down to 0).
>
> Since we're running the same DT, the polarity shouldn't be inverted. I
> see you have CONFIG_PWM_DEBUG=y enabled, which is most likely the reason
> the probe fails.
Thanks, I've disabled that options and the probe completes okay. The fan
is now spinning :)
Drew
^ permalink raw reply [flat|nested] 27+ messages in thread