Re: [PATCH RFC 2/6] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Michal Wilczynski <m.wilczynski@samsung.com>]

On 6/5/25 12:39, Uwe Kleine-König wrote:
> Hello,
> 
> I don't speak Rust, so please double-check all my feedback if it really
> applies.

No problem, appreciate your time and experience with PWM subsystem !

> 
> On Sat, May 24, 2025 at 11:14:56PM +0200, Michal Wilczynski wrote:
>> Introduce a PWM driver for the T-HEAD TH1520 SoC written in Rust. It
>> utilizes the Rust PWM abstractions added in the previous commit.
>>
>> The driver implements the standard PwmOps for the PWM framework,
>> supporting configuration of period, duty cycle, and polarity for the
>> TH1520's PWM channels. It uses devm managed resources for the PWM chip
>> itself and Rust DevRes for I/O memory. Clock management is handled using
>> Rust's RAII pattern.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  MAINTAINERS               |   1 +
>>  drivers/pwm/Kconfig       |   6 +
>>  drivers/pwm/Makefile      |   1 +
>>  drivers/pwm/pwm_th1520.rs | 272 ++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 280 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2b080e8f3d873b1e401b3a2fe1207c224c4591fc..0cfac73aea65076c5ccb50a25ea686fb86b472b8 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -20986,6 +20986,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:	include/dt-bindings/clock/thead,th1520-clk-ap.h
>>  F:	include/dt-bindings/power/thead,th1520-power.h
>>  F:	include/linux/firmware/thead/thead,th1520-aon.h
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index b5bd5c13b3a5e5a575a0fbfb2e285f5665b7a671..796fcd8343b7c8e30f62edc2e0fecf0e9b1ae20e 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -684,6 +684,12 @@ 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
>> +	  Generic PWM framework driver for TH1520 SoC.
>> +
>>  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 539e0def3f82fcb866ab83a0346a15f7efdd7127..6890f860ada6f1a6ed43dd3a3a9584cd2fa877f3 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -70,3 +70,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..4665e293e8d0bdc1a62a4e295cdaf4d47b3dd134
>> --- /dev/null
>> +++ b/drivers/pwm/pwm_th1520.rs
>> @@ -0,0 +1,272 @@
>> +// 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 kernel::{c_str, clk::Clk, device, io::mem::IoMem, of, platform, prelude::*, pwm, time};
>> +
>> +const MAX_PWM_NUM: u32 = 6;
>> +
>> +const fn th1520_pwm_chn_base(n: u32) -> u32 {
>> +    n * 0x20
>> +}
>> +const fn th1520_pwm_ctrl(n: u32) -> u32 {
>> +    th1520_pwm_chn_base(n) + 0x00
>> +}
>> +const fn th1520_pwm_per(n: u32) -> u32 {
>> +    th1520_pwm_chn_base(n) + 0x08
>> +}
>> +const fn th1520_pwm_fp(n: u32) -> u32 {
>> +    th1520_pwm_chn_base(n) + 0x0c
>> +}
>> +
>> +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 PWM_INFACTOUT: u32 = 1 << 9;
>> +
>> +struct Th1520PwmChipData {
>> +    clk: Clk,
>> +    iomem: kernel::devres::Devres<IoMem<0>>,
>> +}
>> +
>> +impl Th1520PwmChipData {
>> +    fn _config(
>> +        &self,
>> +        hwpwm: u32,
>> +        duty_ns: u64,
>> +        period_ns: u64,
>> +        target_polarity: pwm::Polarity,
> 
> Why "target_polarity"? duty_ns and period_ns also don't contain
> "target_"?

You're right, that was an inconsistent name. I've since re-worked the
driver to use the modern waveform API as you suggested in the other
thread. In the new version, this function no longer exists, and the new
round_waveform_tohw op takes a &pwm::Waveform parameter, which resolves
this naming issue. Thank you for pointing it out.

> 
>> +    ) -> Result<u32> {
>> +        let regs = self.iomem.try_access().ok_or_else(|| {
>> +            pr_err!("PWM-{}: Failed to access I/O memory in _config\n", hwpwm);
>> +            EBUSY
>> +        })?;
>> +
>> +        // Calculate cycle values
>> +        let rate_hz_u64 = self.clk.rate().as_hz() as u64;
>> +
>> +        if duty_ns > period_ns {
>> +            pr_err!(
>> +                "PWM-{}: Duty {}ns > period {}ns\n",
>> +                hwpwm,
>> +                duty_ns,
>> +                period_ns
>> +            );
>> +            return Err(EINVAL);
>> +        }
>> +        if period_ns == 0 {
>> +            pr_err!("PWM-{}: Period is zero\n", hwpwm);
>> +            return Err(EINVAL);
>> +        }
> 
> You don't need to check for period_ns == 0 explicitly. This case then
> hits period_cycle == 0 below.
> 
>> +
>> +        let mut period_cycle = mul_div_u64(period_ns, rate_hz_u64, time::NSEC_PER_SEC as u64);
> 
> if period_ns is big and rate_hz_u64 > NSEC_PER_SEC this might overflow.
> 
> Typically refuse to probe if rate_hz_u64 > NSEC_PER_SEC and call
> clk_rate_exclusive_get().
> 
>> +        if period_cycle > u32::MAX as u64 {
>> +            period_cycle = u32::MAX as u64;
>> +        }
>> +        if period_cycle == 0 {
>> +            pr_err!(
>> +                "PWM-{}: Calculated period_cycle is zero, not allowed by HW\n",
>> +                hwpwm
>> +            );
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        let mut duty_cycle = mul_div_u64(duty_ns, rate_hz_u64, time::NSEC_PER_SEC as u64);
>> +        if duty_cycle > u32::MAX as u64 {
>> +            duty_cycle = u32::MAX as u64;
>> +        }
>> +
>> +        let mut base_ctrl_val = PWM_INFACTOUT | PWM_CONTINUOUS_MODE;
>> +        if target_polarity == pwm::Polarity::Normal {
>> +            // FPOUT=1 for Normal
>> +            base_ctrl_val |= PWM_FPOUT;
>> +        } else {
>> +            // Inversed, FPOUT=0
>> +            base_ctrl_val &= !PWM_FPOUT;
>> +        }
>> +        regs.try_write32(base_ctrl_val, th1520_pwm_ctrl(hwpwm) as usize)?;
>> +        pr_debug!(
>> +            "PWM-{}: _config: Initial CTRL write (polarity, mode): 0x{:x}\n",
>> +            hwpwm,
>> +            base_ctrl_val
>> +        );
>> +
>> +        // Write period and duty registers
>> +        regs.try_write32(period_cycle as u32, th1520_pwm_per(hwpwm) as usize)?;
>> +        regs.try_write32(duty_cycle as u32, th1520_pwm_fp(hwpwm) as usize)?;
>> +        pr_debug!(
>> +            "PWM-{}: _config: Period_cyc={}, Duty_cyc={}\n",
>> +            hwpwm,
>> +            period_cycle,
>> +            duty_cycle
>> +        );
>> +
>> +        // Apply period/duty by toggling CFG_UPDATE from 0 to 1.
>> +        // The `base_ctrl_val` (just written to HW) has CFG_UPDATE=0. Now set it.
>> +        let ctrl_val_for_update = base_ctrl_val | PWM_CFG_UPDATE;
>> +        regs.try_write32(ctrl_val_for_update, th1520_pwm_ctrl(hwpwm) as usize)?;
>> +        pr_debug!(
>> +            "PWM-{}: _config: CTRL write with CFG_UPDATE: 0x{:x}\n",
>> +            hwpwm,
>> +            ctrl_val_for_update
>> +        );
>> +
>> +        Ok(ctrl_val_for_update)
>> +    }
>> +
>> +    fn _enable(&self, hwpwm: u32, ctrl_val_after_config: u32) -> Result {
>> +        let regs = self.iomem.try_access().ok_or_else(|| {
>> +            pr_err!("PWM-{}: Failed to access I/O memory in _enable\n", hwpwm);
>> +            EBUSY
>> +        })?;
>> +
>> +        // ctrl_val_after_config already has mode, polarity, and CFG_UPDATE correctly set.
>> +        // Now add the START bit. START bit auto-clears.
>> +        let ctrl_to_start = ctrl_val_after_config | PWM_START;
>> +        regs.try_write32(ctrl_to_start, th1520_pwm_ctrl(hwpwm) as usize)?;
>> +        pr_debug!(
>> +            "PWM-{}: _enable: CTRL write with START: 0x{:x}\n",
>> +            hwpwm,
>> +            ctrl_to_start
>> +        );
>> +        Ok(())
>> +    }
>> +
>> +    fn _disable(&self, hwpwm: u32) -> Result<()> {
>> +        let regs = self.iomem.try_access().ok_or_else(|| {
>> +            pr_err!("PWM-{}: Failed to access I/O memory in _disable\n", hwpwm);
>> +            EINVAL
>> +        })?;
>> +
>> +        let mut ctrl_val = regs.try_read32(th1520_pwm_ctrl(hwpwm) as usize)?;
>> +        pr_debug!("PWM-{}: _disable: Read CTRL: 0x{:x}\n", hwpwm, ctrl_val);
>> +
>> +        // Ensure CFG_UPDATE is 0 before updating duty (Limitation #4)
>> +        if (ctrl_val & PWM_CFG_UPDATE) != 0 {
>> +            ctrl_val &= !PWM_CFG_UPDATE;
>> +            regs.try_write32(ctrl_val, th1520_pwm_ctrl(hwpwm) as usize)?;
>> +            pr_debug!(
>> +                "PWM-{}: _disable: Cleared CFG_UPDATE, wrote CTRL: 0x{:x}\n",
>> +                hwpwm,
>> +                ctrl_val
>> +            );
>> +        }
>> +
>> +        // Set duty cycle to 0
>> +        regs.try_write32(0, th1520_pwm_fp(hwpwm) as usize)?;
>> +        pr_debug!("PWM-{}: _disable: Wrote 0 to DUTY (FP) register\n", hwpwm);
>> +
>> +        // Apply the 0% duty by toggling CFG_UPDATE from 0 to 1
>> +        // Use the ctrl_val that has CFG_UPDATE cleared (or was already clear)
>> +        ctrl_val |= PWM_CFG_UPDATE;
>> +        regs.try_write32(ctrl_val, th1520_pwm_ctrl(hwpwm) as usize)?;
>> +        pr_debug!(
>> +            "PWM-{}: _disable: Set CFG_UPDATE, wrote CTRL: 0x{:x}\n",
>> +            hwpwm,
>> +            ctrl_val
>> +        );
>> +
>> +        Ok(())
>> +    }
>> +}
>> +
>> +impl pwm::PwmOps for Th1520PwmChipData {
>> +    // This driver implements get_state
> 
> I don't spot the get_state implementation?!

That's a good catch, thank you. You are correct, the comment was ahead
of the implementation in this RFC version. My intention was to keep the
initial patch focused. I have now added the full get_state
implementation for v2.

> 
>> +    fn apply(
>> +        pwm_chip_ref: &mut pwm::Chip,
>> +        pwm_dev: &mut pwm::Device,
>> +        target_state: &pwm::State,
> 
> In C code I like these variables be called "chip", "pwm" and "state"
> respectively. Is that possible here, too?

Absolutely. That makes sense for consistency with the C codebase. In the
new waveform based version, I've ensured the PwmOps parameters are named
simply chip, pwm to align with standard practice.

> 
>> +    ) -> Result {
>> +        let data: &Th1520PwmChipData = pwm_chip_ref.get_drvdata().ok_or(EINVAL)?;
>> +        let hwpwm = pwm_dev.hwpwm();
>> +
>> +        if !target_state.enabled() {
>> +            if pwm_dev.state().enabled() {
>> +                data._disable(hwpwm)?;
>> +            }
>> +
>> +            return Ok(());
>> +        }
>> +
>> +        // Configure period, duty, and polarity.
>> +        // This function also latches period/duty with CFG_UPDATE.
>> +        // It returns the control value that was written with CFG_UPDATE set.
>> +        let ctrl_val_after_config = data._config(
>> +            hwpwm,
>> +            target_state.duty_cycle(),
>> +            target_state.period(),
>> +            target_state.polarity(),
>> +        )?;
>> +
>> +        // Enable by setting START bit if it wasn't enabled before this apply call
>> +        if !pwm_dev.state().enabled() {
>> +            data._enable(hwpwm, ctrl_val_after_config)?;
>> +        }
>> +
>> +        Ok(())
>> +    }
>> +}
>> +
>> +impl Drop for Th1520PwmChipData {
>> +    fn drop(&mut self) {
>> +        self.clk.disable_unprepare();
>> +    }
>> +}
>> +
>> +fn mul_div_u64(a: u64, b: u64, c: u64) -> u64 {
>> +    if c == 0 {
>> +        return 0;
>> +    }
>> +    a.wrapping_mul(b) / c
>> +}
> 
> Is this save if a * b > U64_MAX? I would have expected such a function
> to already exist in generic code.

You're right, thank you. The wrapping_mul is unsafe due to the overflow
risk you pointed out.

The ideal solution would be to use the kernel's own mul_u64_u64_div_u64
helper function.

Rust maintainers: This binding doesn't seem to be available yet. Would a
preparatory patch introducing a minimal rust/kernel/math.rs with  only
this binding be the best way to proceed? It seems like a useful utility
for more than just this driver.

> 
>> +static TH1520_PWM_OPS: pwm::PwmOpsVTable = pwm::create_pwm_ops::<Th1520PwmChipData>();
>> +
>> +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<device::Core>,
>> +        _id_info: Option<&Self::IdInfo>,
>> +    ) -> Result<Pin<KBox<Self>>> {
>> +        let resource = pdev.resource(0).ok_or(ENODEV)?;
>> +        let iomem = pdev.ioremap_resource(&resource)?;
>> +
>> +        let clk = Clk::get(pdev.as_ref(), None)?;
>> +
>> +        clk.prepare_enable()?;
> 
> Is there something like devm_clk_get_enabled() such that the drop
> function becomes redundant?

I've reviewed the current clk.rs abstractions that were recently merged,
and while they provide Clk::get() (which is refcounted), a combined
devm_clk_get_enabled() doesn't exist yet. Because of this, the driver
must manually call enable and disable. I've achieved it through Rust
RAII.

> 
>> +        let driver_data = KBox::new(Th1520PwmChipData { clk, iomem }, GFP_KERNEL)?;
>> +        let pwm_chip = pwm::devm_chip_alloc(pdev.as_ref(), MAX_PWM_NUM, 0)?;
>> +
>> +        let result = pwm::devm_chip_add(pdev.as_ref(), pwm_chip, &TH1520_PWM_OPS);
>> +        if result.is_err() {
>> +            pr_err!("Failed to add PWM chip: {:?}\n", result);
>> +            return Err(EIO);
>> +        }
>> +
>> +        pwm_chip.set_drvdata(driver_data);
>> +        pr_info!("T-HEAD TH1520 PWM probed correctly\n");
> 
> Please degrade to pr_debug. Each driver emitting a message is an
> annoyance.
> 
>> +        Ok(KBox::new(Self, GFP_KERNEL)?.into())
>> +    }
>> +}
>> +
>> +kernel::module_platform_driver! {
>> +    type: Th1520PwmPlatformDriver,
>> +    name: "pwm_th1520",
>> +    author: "Michal Wilczynski",
>> +    description: "T-HEAD TH1520 PWM driver",
>> +    license: "GPL v2",
>> +}
> 
> Best regards
> Uwe

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>