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 | 318 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 330 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index a575622454a2ef57ce055c8a8c4765fa4fddc490..879870471e86dcec4a0e8f5c45d2cc3409411fdd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21402,6 +21402,7 @@ F: drivers/mailbox/mailbox-th1520.c
F: drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
F: drivers/pinctrl/pinctrl-th1520.c
F: drivers/pmdomain/thead/
+F: drivers/pwm/pwm_th1520.rs
F: drivers/reset/reset-th1520.c
F: include/dt-bindings/clock/thead,th1520-clk-ap.h
F: include/dt-bindings/power/thead,th1520-power.h
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index cfddeae0eab3523f04f361fb41ccd1345c0c937b..a675b3bd68392d1b05a47a2a1390c5606647ca15 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -719,6 +719,16 @@ config PWM_TEGRA
To compile this driver as a module, choose M here: the module
will be called pwm-tegra.
+config PWM_TH1520
+ tristate "TH1520 PWM support"
+ depends on RUST_PWM_ABSTRACTIONS
+ help
+ This option enables the driver for the PWM controller found on the
+ T-HEAD TH1520 SoC.
+
+ To compile this driver as a module, choose M here; the module
+ will be called pwm-th1520. If you are unsure, say N.
+
config PWM_TIECAP
tristate "ECAP PWM support"
depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 96160f4257fcb0e0951581af0090615c0edf5260..a410747095327a315a6bcd24ae343ce7857fe323 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o
obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o
obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
+obj-$(CONFIG_PWM_TH1520) += pwm_th1520.o
obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
obj-$(CONFIG_PWM_TWL) += pwm-twl.o
diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
new file mode 100644
index 0000000000000000000000000000000000000000..a77c45cef9cf8f02a25db9d42c45cd0df565b0ec
--- /dev/null
+++ b/drivers/pwm/pwm_th1520.rs
@@ -0,0 +1,318 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2025 Samsung Electronics Co., Ltd.
+// Author: Michal Wilczynski <m.wilczynski@samsung.com>
+
+//! Rust T-HEAD TH1520 PWM driver
+//!
+//! Limitations:
+//! - The period and duty cycle are controlled by 32-bit hardware registers,
+//! limiting the maximum resolution.
+//! - The driver supports continuous output mode only; one-shot mode is not
+//! implemented.
+//! - The controller hardware provides up to 6 PWM channels.
+//!
+
+use core::ops::Deref;
+use kernel::{
+ c_str,
+ clk::Clk,
+ device::{Bound, Core, Device},
+ devres,
+ io::mem::IoMem,
+ of, platform,
+ prelude::*,
+ pwm, time,
+};
+
+const MAX_PWM_NUM: u32 = 6;
+
+// Register offsets
+const fn th1520_pwm_chn_base(n: u32) -> usize {
+ (n * 0x20) as usize
+}
+
+const fn th1520_pwm_ctrl(n: u32) -> usize {
+ th1520_pwm_chn_base(n)
+}
+
+const fn th1520_pwm_per(n: u32) -> usize {
+ th1520_pwm_chn_base(n) + 0x08
+}
+
+const fn th1520_pwm_fp(n: u32) -> usize {
+ th1520_pwm_chn_base(n) + 0x0c
+}
+
+// Control register bits
+const PWM_START: u32 = 1 << 0;
+const PWM_CFG_UPDATE: u32 = 1 << 2;
+const PWM_CONTINUOUS_MODE: u32 = 1 << 5;
+const PWM_FPOUT: u32 = 1 << 8;
+
+const TH1520_PWM_REG_SIZE: usize = 0xB0;
+
+fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
+ const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
+
+ match ns.checked_mul(rate_hz) {
+ Some(product) => product / NSEC_PER_SEC_U64,
+ None => u64::MAX,
+ }
+}
+
+fn cycles_to_ns(cycles: u64, rate_hz: u64) -> u64 {
+ const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
+
+ // Round up
+ let Some(numerator) = cycles
+ .checked_mul(NSEC_PER_SEC_U64)
+ .and_then(|p| p.checked_add(rate_hz - 1))
+ else {
+ return u64::MAX;
+ };
+
+ numerator / rate_hz
+}
+
+/// 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 round_waveform_tohw(
+ chip: &pwm::Chip,
+ _pwm: &pwm::Device,
+ wf: &pwm::Waveform,
+ ) -> Result<(c_int, Self::WfHw)> {
+ let data: &Self = chip.drvdata();
+
+ if wf.period_length_ns == 0 {
+ return Ok((
+ 0,
+ Th1520WfHw {
+ enabled: false,
+ ..Default::default()
+ },
+ ));
+ }
+
+ let rate_hz = data.clk.rate().as_hz() as u64;
+
+ let period_cycles = ns_to_cycles(wf.period_length_ns, rate_hz).min(u32::MAX as u64);
+ let mut duty_cycles = ns_to_cycles(wf.duty_length_ns, rate_hz).min(u32::MAX as u64);
+
+ let mut ctrl_val = PWM_CONTINUOUS_MODE;
+
+ if wf.duty_offset_ns == 0 {
+ ctrl_val |= PWM_FPOUT;
+ } else {
+ duty_cycles = period_cycles - duty_cycles;
+ }
+
+ let wfhw = Th1520WfHw {
+ period_cycles: period_cycles as u32,
+ duty_cycles: duty_cycles as u32,
+ ctrl_val,
+ enabled: true,
+ };
+
+ dev_dbg!(
+ chip.device(),
+ "Requested: period {}ns, duty {}ns, offset {}ns -> HW: period {} cyc, duty {} cyc, ctrl 0x{:x}\n",
+ wf.period_length_ns,
+ wf.duty_length_ns,
+ wf.duty_offset_ns,
+ wfhw.period_cycles,
+ wfhw.duty_cycles,
+ wfhw.ctrl_val
+ );
+
+ Ok((0, wfhw))
+ }
+
+ fn round_waveform_fromhw(
+ chip: &pwm::Chip,
+ _pwm: &pwm::Device,
+ wfhw: &Self::WfHw,
+ wf: &mut pwm::Waveform,
+ ) -> Result<c_int> {
+ let data: &Self = chip.drvdata();
+ let rate_hz = data.clk.rate().as_hz() as u64;
+
+ wf.period_length_ns = cycles_to_ns(wfhw.period_cycles as u64, rate_hz);
+
+ let duty_cycles = wfhw.duty_cycles as u64;
+
+ if (wfhw.ctrl_val & PWM_FPOUT) != 0 {
+ wf.duty_length_ns = cycles_to_ns(duty_cycles, rate_hz);
+ wf.duty_offset_ns = 0;
+ } else {
+ let period_cycles = wfhw.period_cycles as u64;
+ let original_duty_cycles = period_cycles.saturating_sub(duty_cycles);
+
+ wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
+ // We can't recover the original non-zero offset, so we just set it
+ // to a representative non-zero value.
+ wf.duty_offset_ns = 1;
+ }
+
+ Ok(0)
+ }
+
+ fn read_waveform(
+ chip: &pwm::Chip,
+ pwm: &pwm::Device,
+ parent_dev: &Device<Bound>,
+ ) -> Result<Self::WfHw> {
+ let data: &Self = chip.drvdata();
+ let hwpwm = pwm.hwpwm();
+ let iomem_accessor = data.iomem.access(parent_dev)?;
+ let iomap = iomem_accessor.deref();
+
+ let ctrl = iomap.try_read32(th1520_pwm_ctrl(hwpwm))?;
+ let period_cycles = iomap.try_read32(th1520_pwm_per(hwpwm))?;
+ let duty_cycles = iomap.try_read32(th1520_pwm_fp(hwpwm))?;
+
+ let wfhw = Th1520WfHw {
+ period_cycles,
+ duty_cycles,
+ ctrl_val: ctrl,
+ enabled: duty_cycles != 0,
+ };
+
+ dev_dbg!(
+ chip.device(),
+ "PWM-{}: read_waveform: Read hw state - period: {}, duty: {}, ctrl: 0x{:x}, enabled: {}",
+ hwpwm,
+ wfhw.period_cycles,
+ wfhw.duty_cycles,
+ wfhw.ctrl_val,
+ wfhw.enabled
+ );
+
+ Ok(wfhw)
+ }
+
+ fn write_waveform(
+ chip: &pwm::Chip,
+ pwm: &pwm::Device,
+ wfhw: &Self::WfHw,
+ parent_dev: &Device<Bound>,
+ ) -> Result {
+ let data: &Self = chip.drvdata();
+ let hwpwm = pwm.hwpwm();
+ let iomem_accessor = data.iomem.access(parent_dev)?;
+ let iomap = iomem_accessor.deref();
+ let was_enabled = pwm.state().enabled();
+
+ if !wfhw.enabled {
+ if was_enabled {
+ iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
+ iomap.try_write32(0, th1520_pwm_fp(hwpwm))?;
+ iomap.try_write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
+ }
+ return Ok(());
+ }
+
+ iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
+ iomap.try_write32(wfhw.period_cycles, th1520_pwm_per(hwpwm))?;
+ iomap.try_write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm))?;
+ iomap.try_write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
+
+ // The `PWM_START` bit must be written in a separate, final transaction, and
+ // only when enabling the channel from a disabled state.
+ if !was_enabled {
+ iomap.try_write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm))?;
+ }
+
+ dev_dbg!(
+ chip.device(),
+ "PWM-{}: Wrote (per: {}, duty: {})",
+ hwpwm,
+ wfhw.period_cycles,
+ wfhw.duty_cycles,
+ );
+
+ 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;
+
+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()?;
+
+ // TODO: Get exclusive ownership of the clock to prevent rate changes.
+ // The Rust equivalent of `clk_rate_exclusive_get()` is not yet available.
+ // This should be updated once it is implemented.
+ 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 drvdata = KBox::new(Th1520PwmDriverData { iomem, clk }, GFP_KERNEL)?;
+ let chip = pwm::Chip::new(dev, MAX_PWM_NUM, 0, drvdata)?;
+
+ pwm::Registration::new_foreign_owned(dev, chip, &TH1520_PWM_OPS)?;
+
+ Ok(KBox::new(Th1520PwmPlatformDriver, GFP_KERNEL)?.into())
+ }
+}
+
+kernel::module_platform_driver! {
+ type: Th1520PwmPlatformDriver,
+ name: "pwm-th1520",
+ authors: ["Michal Wilczynski <m.wilczynski@samsung.com>"],
+ description: "T-HEAD TH1520 PWM driver",
+ license: "GPL v2",
+}
--
2.34.1On Mon, Jun 23, 2025 at 08:08:52PM +0200, Michal Wilczynski wrote:
> Introduce a PWM driver for the T-HEAD TH1520 SoC, written in Rust and
> utilizing the safe PWM abstractions from the preceding commit.
>
> The driver implements the pwm::PwmOps trait using the modern waveform
> API (round_waveform_tohw, write_waveform, etc.) to support configuration
> of period, duty cycle, and polarity for the TH1520's PWM channels.
>
> Resource management is handled using idiomatic Rust patterns. The PWM
> chip object is allocated via pwm::Chip::new and its registration with
> the PWM core is managed by the pwm::Registration RAII guard. This
> ensures pwmchip_remove is always called when the driver unbinds,
> preventing resource leaks. Device managed resources are used for the
> MMIO region, and the clock lifecycle is correctly managed in the
> driver's private data Drop implementation.
>
> The driver's core logic is written entirely in safe Rust, with no unsafe
> blocks.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> MAINTAINERS | 1 +
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm_th1520.rs | 318 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 330 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a575622454a2ef57ce055c8a8c4765fa4fddc490..879870471e86dcec4a0e8f5c45d2cc3409411fdd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21402,6 +21402,7 @@ F: drivers/mailbox/mailbox-th1520.c
> F: drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> F: drivers/pinctrl/pinctrl-th1520.c
> F: drivers/pmdomain/thead/
> +F: drivers/pwm/pwm_th1520.rs
> F: drivers/reset/reset-th1520.c
> F: include/dt-bindings/clock/thead,th1520-clk-ap.h
> F: include/dt-bindings/power/thead,th1520-power.h
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index cfddeae0eab3523f04f361fb41ccd1345c0c937b..a675b3bd68392d1b05a47a2a1390c5606647ca15 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -719,6 +719,16 @@ config PWM_TEGRA
> To compile this driver as a module, choose M here: the module
> will be called pwm-tegra.
>
> +config PWM_TH1520
> + tristate "TH1520 PWM support"
> + depends on RUST_PWM_ABSTRACTIONS
RUST_PWM_ABSTRACTIONS is user selectable. Is that sensible. From a
user's POV it shouldn't matter if the driver is written in Rust or not.
> + help
> + This option enables the driver for the PWM controller found on the
> + T-HEAD TH1520 SoC.
> +
> + To compile this driver as a module, choose M here; the module
> + will be called pwm-th1520. If you are unsure, say N.
> +
> config PWM_TIECAP
> tristate "ECAP PWM support"
> depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 96160f4257fcb0e0951581af0090615c0edf5260..a410747095327a315a6bcd24ae343ce7857fe323 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o
> obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
> obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o
> obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
> +obj-$(CONFIG_PWM_TH1520) += pwm_th1520.o
> obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
> obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
> obj-$(CONFIG_PWM_TWL) += pwm-twl.o
> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..a77c45cef9cf8f02a25db9d42c45cd0df565b0ec
> --- /dev/null
> +++ b/drivers/pwm/pwm_th1520.rs
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +//! Rust T-HEAD TH1520 PWM driver
> +//!
> +//! Limitations:
> +//! - The period and duty cycle are controlled by 32-bit hardware registers,
> +//! limiting the maximum resolution.
> +//! - The driver supports continuous output mode only; one-shot mode is not
> +//! implemented.
> +//! - The controller hardware provides up to 6 PWM channels.
Important questions to answer here are:
- How does the hardware behave on disable? (Does it stop immediately
(or at all)? Does it emit a constant output? Which?)
- How does the hardware behave on reconfiguration? (Does it switch
immidiately or does it complete the current period? Can there be
glitches?
> +//!
> +
> +use core::ops::Deref;
> +use kernel::{
> + c_str,
> + clk::Clk,
> + device::{Bound, Core, Device},
> + devres,
> + io::mem::IoMem,
> + of, platform,
> + prelude::*,
> + pwm, time,
> +};
> +
> +const MAX_PWM_NUM: u32 = 6;
> +
> +// Register offsets
> +const fn th1520_pwm_chn_base(n: u32) -> usize {
> + (n * 0x20) as usize
> +}
> +
> +const fn th1520_pwm_ctrl(n: u32) -> usize {
> + th1520_pwm_chn_base(n)
> +}
> +
> +const fn th1520_pwm_per(n: u32) -> usize {
> + th1520_pwm_chn_base(n) + 0x08
> +}
> +
> +const fn th1520_pwm_fp(n: u32) -> usize {
> + th1520_pwm_chn_base(n) + 0x0c
> +}
> +
> +// Control register bits
> +const PWM_START: u32 = 1 << 0;
> +const PWM_CFG_UPDATE: u32 = 1 << 2;
> +const PWM_CONTINUOUS_MODE: u32 = 1 << 5;
> +const PWM_FPOUT: u32 = 1 << 8;
Can you please add a driver specific prefix to these?
> +const TH1520_PWM_REG_SIZE: usize = 0xB0;
> +
> +fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
> + const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
> +
> + match ns.checked_mul(rate_hz) {
> + Some(product) => product / NSEC_PER_SEC_U64,
> + None => u64::MAX,
> + }
The semantic here is: If ns * rate_hz overflows, return U64_MAX, else ns
* rate_hz / NSEC_PER_SEC, right?
If you cannot easily reproduce what mul_u64_u64_div_u64() does, I think
it would be more prudent do make this:
match ns.checked_mul(rate_hz) {
Some(product) => product,
None => u64::MAX,
} / NSEC_PER_SEC_U64
> +}
> +
> [...]
> +impl pwm::PwmOps for Th1520PwmDriverData {
> + type WfHw = Th1520WfHw;
> +
> + fn round_waveform_tohw(
> + chip: &pwm::Chip,
> + _pwm: &pwm::Device,
> + wf: &pwm::Waveform,
> + ) -> Result<(c_int, Self::WfHw)> {
> + let data: &Self = chip.drvdata();
> +
> + if wf.period_length_ns == 0 {
> + return Ok((
> + 0,
> + Th1520WfHw {
> + enabled: false,
> + ..Default::default()
> + },
> + ));
> + }
> +
> + let rate_hz = data.clk.rate().as_hz() as u64;
> +
> + let period_cycles = ns_to_cycles(wf.period_length_ns, rate_hz).min(u32::MAX as u64);
> + let mut duty_cycles = ns_to_cycles(wf.duty_length_ns, rate_hz).min(u32::MAX as u64);
> +
> + let mut ctrl_val = PWM_CONTINUOUS_MODE;
> +
> + if wf.duty_offset_ns == 0 {
> + ctrl_val |= PWM_FPOUT;
> + } else {
> + duty_cycles = period_cycles - duty_cycles;
Huh, this looks wrong. Your hardware only supports the two polarities,
right? Then configure inversed polarity if
wf->duty_length_ns && wf->duty_offset_ns && wf->duty_length_ns + wf->duty_offset_ns >= wf->period_length_ns
(i.e. how the pwm-stm32 driver does it).
> + }
> +
> + let wfhw = Th1520WfHw {
> + period_cycles: period_cycles as u32,
> + duty_cycles: duty_cycles as u32,
> + ctrl_val,
> + enabled: true,
> + };
> +
> + dev_dbg!(
> + chip.device(),
> + "Requested: period {}ns, duty {}ns, offset {}ns -> HW: period {} cyc, duty {} cyc, ctrl 0x{:x}\n",
Would it be helpful to also emit the clkrate here?
> + wf.period_length_ns,
> + wf.duty_length_ns,
> + wf.duty_offset_ns,
> + wfhw.period_cycles,
> + wfhw.duty_cycles,
> + wfhw.ctrl_val
> + );
> +
> + Ok((0, wfhw))
> + }
> +
> + fn round_waveform_fromhw(
> + chip: &pwm::Chip,
> + _pwm: &pwm::Device,
> + wfhw: &Self::WfHw,
> + wf: &mut pwm::Waveform,
> + ) -> Result<c_int> {
> + let data: &Self = chip.drvdata();
> + let rate_hz = data.clk.rate().as_hz() as u64;
> +
> + wf.period_length_ns = cycles_to_ns(wfhw.period_cycles as u64, rate_hz);
> +
> + let duty_cycles = wfhw.duty_cycles as u64;
> +
> + if (wfhw.ctrl_val & PWM_FPOUT) != 0 {
> + wf.duty_length_ns = cycles_to_ns(duty_cycles, rate_hz);
> + wf.duty_offset_ns = 0;
> + } else {
> + let period_cycles = wfhw.period_cycles as u64;
> + let original_duty_cycles = period_cycles.saturating_sub(duty_cycles);
> +
> + wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
> + // We can't recover the original non-zero offset, so we just set it
> + // to a representative non-zero value.
> + wf.duty_offset_ns = 1;
For an inversed polarity signal the duty_offset is polarity - duty_cycle.
> + }
> +
> + Ok(0)
> + }
Best regards
Uwe
On 6/27/25 17:28, Uwe Kleine-König wrote:
> On Mon, Jun 23, 2025 at 08:08:52PM +0200, Michal Wilczynski wrote:
>> Introduce a PWM driver for the T-HEAD TH1520 SoC, written in Rust and
>> utilizing the safe PWM abstractions from the preceding commit.
>>
>> The driver implements the pwm::PwmOps trait using the modern waveform
>> API (round_waveform_tohw, write_waveform, etc.) to support configuration
>> of period, duty cycle, and polarity for the TH1520's PWM channels.
>>
>> Resource management is handled using idiomatic Rust patterns. The PWM
>> chip object is allocated via pwm::Chip::new and its registration with
>> the PWM core is managed by the pwm::Registration RAII guard. This
>> ensures pwmchip_remove is always called when the driver unbinds,
>> preventing resource leaks. Device managed resources are used for the
>> MMIO region, and the clock lifecycle is correctly managed in the
>> driver's private data Drop implementation.
>>
>> The driver's core logic is written entirely in safe Rust, with no unsafe
>> blocks.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>> MAINTAINERS | 1 +
>> drivers/pwm/Kconfig | 10 ++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm_th1520.rs | 318 ++++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 330 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a575622454a2ef57ce055c8a8c4765fa4fddc490..879870471e86dcec4a0e8f5c45d2cc3409411fdd 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -21402,6 +21402,7 @@ F: drivers/mailbox/mailbox-th1520.c
>> F: drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
>> F: drivers/pinctrl/pinctrl-th1520.c
>> F: drivers/pmdomain/thead/
>> +F: drivers/pwm/pwm_th1520.rs
>> F: drivers/reset/reset-th1520.c
>> F: include/dt-bindings/clock/thead,th1520-clk-ap.h
>> F: include/dt-bindings/power/thead,th1520-power.h
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index cfddeae0eab3523f04f361fb41ccd1345c0c937b..a675b3bd68392d1b05a47a2a1390c5606647ca15 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -719,6 +719,16 @@ config PWM_TEGRA
>> To compile this driver as a module, choose M here: the module
>> will be called pwm-tegra.
>>
>> +config PWM_TH1520
>> + tristate "TH1520 PWM support"
>> + depends on RUST_PWM_ABSTRACTIONS
>
> RUST_PWM_ABSTRACTIONS is user selectable. Is that sensible. From a
> user's POV it shouldn't matter if the driver is written in Rust or not.
You make an excellent point about user experience. My initial thought
was to follow the depends on pattern that I saw in other Rust drivers.
I can see how using select would be cleaner for the end user. My only
hesitation was that it differs from the current convention for Rust
drivers, and I wanted to be careful about changing an established
pattern.
If you are comfortable with setting this direction for the PWM
subsystem, I am happy to make the change to use select
RUST_PWM_ABSTRACTIONS (gated by depends on RUST). Please let me know.
>
>> + help
>> + This option enables the driver for the PWM controller found on the
>> + T-HEAD TH1520 SoC.
>> +
>> + To compile this driver as a module, choose M here; the module
>> + will be called pwm-th1520. If you are unsure, say N.
>> +
>> config PWM_TIECAP
>> tristate "ECAP PWM support"
>> depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 96160f4257fcb0e0951581af0090615c0edf5260..a410747095327a315a6bcd24ae343ce7857fe323 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -66,6 +66,7 @@ obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o
>> obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
>> obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o
>> obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
>> +obj-$(CONFIG_PWM_TH1520) += pwm_th1520.o
>> obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
>> obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
>> obj-$(CONFIG_PWM_TWL) += pwm-twl.o
>> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..a77c45cef9cf8f02a25db9d42c45cd0df565b0ec
>> --- /dev/null
>> +++ b/drivers/pwm/pwm_th1520.rs
>> @@ -0,0 +1,318 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +//! Rust T-HEAD TH1520 PWM driver
>> +//!
>> +//! Limitations:
>> +//! - The period and duty cycle are controlled by 32-bit hardware registers,
>> +//! limiting the maximum resolution.
>> +//! - The driver supports continuous output mode only; one-shot mode is not
>> +//! implemented.
>> +//! - The controller hardware provides up to 6 PWM channels.
>
> Important questions to answer here are:
>
> - How does the hardware behave on disable? (Does it stop immediately
> (or at all)? Does it emit a constant output? Which?)
> - How does the hardware behave on reconfiguration? (Does it switch
> immidiately or does it complete the current period? Can there be
> glitches?
Sure, I have investigated and will add comments explaining that
reconfiguration is glitch free and disabling is done by setting the duty
cycle to zero
>
>> +//!
>> +
>> +use core::ops::Deref;
>> +use kernel::{
>> + c_str,
>> + clk::Clk,
>> + device::{Bound, Core, Device},
>> + devres,
>> + io::mem::IoMem,
>> + of, platform,
>> + prelude::*,
>> + pwm, time,
>> +};
>> +
>> +const MAX_PWM_NUM: u32 = 6;
>> +
>> +// Register offsets
>> +const fn th1520_pwm_chn_base(n: u32) -> usize {
>> + (n * 0x20) as usize
>> +}
>> +
>> +const fn th1520_pwm_ctrl(n: u32) -> usize {
>> + th1520_pwm_chn_base(n)
>> +}
>> +
>> +const fn th1520_pwm_per(n: u32) -> usize {
>> + th1520_pwm_chn_base(n) + 0x08
>> +}
>> +
>> +const fn th1520_pwm_fp(n: u32) -> usize {
>> + th1520_pwm_chn_base(n) + 0x0c
>> +}
>> +
>> +// Control register bits
>> +const PWM_START: u32 = 1 << 0;
>> +const PWM_CFG_UPDATE: u32 = 1 << 2;
>> +const PWM_CONTINUOUS_MODE: u32 = 1 << 5;
>> +const PWM_FPOUT: u32 = 1 << 8;
>
> Can you please add a driver specific prefix to these?
OK
>
>> +const TH1520_PWM_REG_SIZE: usize = 0xB0;
>> +
>> +fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
>> + const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
>> +
>> + match ns.checked_mul(rate_hz) {
>> + Some(product) => product / NSEC_PER_SEC_U64,
>> + None => u64::MAX,
>> + }
>
> The semantic here is: If ns * rate_hz overflows, return U64_MAX, else ns
> * rate_hz / NSEC_PER_SEC, right?
>
> If you cannot easily reproduce what mul_u64_u64_div_u64() does, I think
> it would be more prudent do make this:
>
> match ns.checked_mul(rate_hz) {
> Some(product) => product,
> None => u64::MAX,
> } / NSEC_PER_SEC_U64
Thank you for the feedback on the calculation. I analyzed the two
approaches and found that on overflow, my version saturates to u64::MAX,
while the suggested version would result in u64::MAX / NSEC_PER_SEC. I
believe my original implementation's saturation behavior is more
predictable. With this in mind, would you be comfortable with me
retaining the original implementation?
>
>> +}
>> +
>> [...]
>> +impl pwm::PwmOps for Th1520PwmDriverData {
>> + type WfHw = Th1520WfHw;
>> +
>> + fn round_waveform_tohw(
>> + chip: &pwm::Chip,
>> + _pwm: &pwm::Device,
>> + wf: &pwm::Waveform,
>> + ) -> Result<(c_int, Self::WfHw)> {
>> + let data: &Self = chip.drvdata();
>> +
>> + if wf.period_length_ns == 0 {
>> + return Ok((
>> + 0,
>> + Th1520WfHw {
>> + enabled: false,
>> + ..Default::default()
>> + },
>> + ));
>> + }
>> +
>> + let rate_hz = data.clk.rate().as_hz() as u64;
>> +
>> + let period_cycles = ns_to_cycles(wf.period_length_ns, rate_hz).min(u32::MAX as u64);
>> + let mut duty_cycles = ns_to_cycles(wf.duty_length_ns, rate_hz).min(u32::MAX as u64);
>> +
>> + let mut ctrl_val = PWM_CONTINUOUS_MODE;
>> +
>> + if wf.duty_offset_ns == 0 {
>> + ctrl_val |= PWM_FPOUT;
>> + } else {
>> + duty_cycles = period_cycles - duty_cycles;
>
> Huh, this looks wrong. Your hardware only supports the two polarities,
> right? Then configure inversed polarity if
>
> wf->duty_length_ns && wf->duty_offset_ns && wf->duty_length_ns + wf->duty_offset_ns >= wf->period_length_ns
>
> (i.e. how the pwm-stm32 driver does it).
OK, will fix
>
>> + }
>> +
>> + let wfhw = Th1520WfHw {
>> + period_cycles: period_cycles as u32,
>> + duty_cycles: duty_cycles as u32,
>> + ctrl_val,
>> + enabled: true,
>> + };
>> +
>> + dev_dbg!(
>> + chip.device(),
>> + "Requested: period {}ns, duty {}ns, offset {}ns -> HW: period {} cyc, duty {} cyc, ctrl 0x{:x}\n",
>
> Would it be helpful to also emit the clkrate here?
OK will update
>
>> + wf.period_length_ns,
>> + wf.duty_length_ns,
>> + wf.duty_offset_ns,
>> + wfhw.period_cycles,
>> + wfhw.duty_cycles,
>> + wfhw.ctrl_val
>> + );
>> +
>> + Ok((0, wfhw))
>> + }
>> +
>> + fn round_waveform_fromhw(
>> + chip: &pwm::Chip,
>> + _pwm: &pwm::Device,
>> + wfhw: &Self::WfHw,
>> + wf: &mut pwm::Waveform,
>> + ) -> Result<c_int> {
>> + let data: &Self = chip.drvdata();
>> + let rate_hz = data.clk.rate().as_hz() as u64;
>> +
>> + wf.period_length_ns = cycles_to_ns(wfhw.period_cycles as u64, rate_hz);
>> +
>> + let duty_cycles = wfhw.duty_cycles as u64;
>> +
>> + if (wfhw.ctrl_val & PWM_FPOUT) != 0 {
>> + wf.duty_length_ns = cycles_to_ns(duty_cycles, rate_hz);
>> + wf.duty_offset_ns = 0;
>> + } else {
>> + let period_cycles = wfhw.period_cycles as u64;
>> + let original_duty_cycles = period_cycles.saturating_sub(duty_cycles);
>> +
>> + wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
>> + // We can't recover the original non-zero offset, so we just set it
>> + // to a representative non-zero value.
>> + wf.duty_offset_ns = 1;
>
> For an inversed polarity signal the duty_offset is polarity - duty_cycle.
I believe there was a typo in your suggestion and you meant period
instead of polarity. Based on that, my understanding is that for an
inverted signal, the generic pwm_waveform struct expects duty_offset_ns
to represent the duration of the initial low time, while duty_length_ns
represents the high time.
so the code would look like this:
// For an inverted signal, `duty_length_ns` is the high time (period - low_time).
wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
// The offset is the initial low time, which is what the hardware register provides.
wf.duty_offset_ns = cycles_to_ns(duty_cycles, rate_hz);
>
>> + }
>> +
>> + Ok(0)
>> + }
>
> Best regards
> Uwe
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
Hello Michal,
On Sat, Jun 28, 2025 at 08:14:59PM +0200, Michal Wilczynski wrote:
> On 6/27/25 17:28, Uwe Kleine-König wrote:
> > On Mon, Jun 23, 2025 at 08:08:52PM +0200, Michal Wilczynski wrote:
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index cfddeae0eab3523f04f361fb41ccd1345c0c937b..a675b3bd68392d1b05a47a2a1390c5606647ca15 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -719,6 +719,16 @@ config PWM_TEGRA
> >> To compile this driver as a module, choose M here: the module
> >> will be called pwm-tegra.
> >>
> >> +config PWM_TH1520
> >> + tristate "TH1520 PWM support"
> >> + depends on RUST_PWM_ABSTRACTIONS
> >
> > RUST_PWM_ABSTRACTIONS is user selectable. Is that sensible. From a
> > user's POV it shouldn't matter if the driver is written in Rust or not.
>
> You make an excellent point about user experience. My initial thought
> was to follow the depends on pattern that I saw in other Rust drivers.
>
> I can see how using select would be cleaner for the end user. My only
> hesitation was that it differs from the current convention for Rust
> drivers, and I wanted to be careful about changing an established
> pattern.
>
> If you are comfortable with setting this direction for the PWM
> subsystem, I am happy to make the change to use select
> RUST_PWM_ABSTRACTIONS (gated by depends on RUST). Please let me know.
Sounds good.
> >> +const TH1520_PWM_REG_SIZE: usize = 0xB0;
> >> +
> >> +fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
> >> + const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
> >> +
> >> + match ns.checked_mul(rate_hz) {
> >> + Some(product) => product / NSEC_PER_SEC_U64,
> >> + None => u64::MAX,
> >> + }
> >
> > The semantic here is: If ns * rate_hz overflows, return U64_MAX, else ns
> > * rate_hz / NSEC_PER_SEC, right?
> >
> > If you cannot easily reproduce what mul_u64_u64_div_u64() does, I think
> > it would be more prudent do make this:
> >
> > match ns.checked_mul(rate_hz) {
> > Some(product) => product,
> > None => u64::MAX,
> > } / NSEC_PER_SEC_U64
>
> Thank you for the feedback on the calculation. I analyzed the two
> approaches and found that on overflow, my version saturates to u64::MAX,
> while the suggested version would result in u64::MAX / NSEC_PER_SEC. I
> believe my original implementation's saturation behavior is more
> predictable. With this in mind, would you be comfortable with me
> retaining the original implementation?
I'm convinced that my alternative is better. Consider the implemented
mapping: Assuming rate_hz = 160000000 you have:
ns | cycles
-------------+---------------------
... |
115292150452 | 18446744072
115292150453 | 18446744072
115292150454 | 18446744072
115292150455 | 18446744072
115292150456 | 18446744072
115292150457 | 18446744073
115292150458 | 18446744073
115292150459 | 18446744073
115292150460 | 18446744073
115292150461 | 18446744073709551615
115292150462 | 18446744073709551615
...
that's strange, isn't it?
> >> + wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
> >> + // We can't recover the original non-zero offset, so we just set it
> >> + // to a representative non-zero value.
> >> + wf.duty_offset_ns = 1;
> >
> > For an inversed polarity signal the duty_offset is polarity - duty_cycle.
>
> I believe there was a typo in your suggestion and you meant period
> instead of polarity.
ack.
> Based on that, my understanding is that for an
> inverted signal, the generic pwm_waveform struct expects duty_offset_ns
> to represent the duration of the initial low time, while duty_length_ns
> represents the high time.
right.
> so the code would look like this:
> // For an inverted signal, `duty_length_ns` is the high time (period - low_time).
> wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
> // The offset is the initial low time, which is what the hardware register provides.
> wf.duty_offset_ns = cycles_to_ns(duty_cycles, rate_hz);
Looks correct
Best regards
Uwe
© 2016 - 2025 Red Hat, Inc.