diff mbox series

[RFC,V3,8/8] cpufreq: Add Rust based cpufreq-dt driver

Message ID b7df0c75cc07a451243b554fb2272c91cbe42dfe.1719990273.git.viresh.kumar@linaro.org (mailing list archive)
State Superseded, archived
Headers show
Series Rust bindings for cpufreq and OPP core + sample driver | expand

Commit Message

Viresh Kumar July 3, 2024, 7:14 a.m. UTC
This commit adds a Rust based cpufreq-dt driver, which covers most of
the functionality of the existing C based driver. Only a handful of
things are left, like fetching platform data from cpufreq-dt-platdev.c.

This is tested with the help of QEMU for now and switching of
frequencies work as expected.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/Kconfig        |  12 ++
 drivers/cpufreq/Makefile       |   1 +
 drivers/cpufreq/rcpufreq_dt.rs | 225 +++++++++++++++++++++++++++++++++
 3 files changed, 238 insertions(+)
 create mode 100644 drivers/cpufreq/rcpufreq_dt.rs

Comments

Danilo Krummrich July 5, 2024, 11:32 a.m. UTC | #1
Hi Viresh,

On 7/3/24 09:14, Viresh Kumar wrote:
> This commit adds a Rust based cpufreq-dt driver, which covers most of
> the functionality of the existing C based driver. Only a handful of
> things are left, like fetching platform data from cpufreq-dt-platdev.c.
> 
> This is tested with the help of QEMU for now and switching of
> frequencies work as expected.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/cpufreq/Kconfig        |  12 ++
>   drivers/cpufreq/Makefile       |   1 +
>   drivers/cpufreq/rcpufreq_dt.rs | 225 +++++++++++++++++++++++++++++++++
>   3 files changed, 238 insertions(+)
>   create mode 100644 drivers/cpufreq/rcpufreq_dt.rs
> 
> diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
> new file mode 100644
> index 000000000000..652458e7a3b9
> --- /dev/null
> +++ b/drivers/cpufreq/rcpufreq_dt.rs

<snip>

> +
> +type DeviceData = Box<cpufreq::Registration<CPUFreqDTDriver>>;
> +
> +impl platform::Driver for CPUFreqDTDriver {
> +    type Data = Arc<DeviceData>;
> +
> +    define_of_id_table! {(), [
> +        (of::DeviceId(b_str!("operating-points-v2")), None),
> +    ]}
> +
> +    fn probe(_dev: &mut platform::Device, _id_info: Option<&Self::IdInfo>) -> Result<Self::Data> {
> +        let drv = Arc::new(
> +            cpufreq::Registration::<CPUFreqDTDriver>::register(
> +                c_str!("cpufreq-dt"),
> +                (),
> +                cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
> +                true,
> +            )?,
> +            GFP_KERNEL,
> +        )?;

Putting the `cpufreq::Registration` into `Arc<DeviceData>` is unsafe from a
lifetime point of view. Nothing prevents this `Arc` to out-live the
`platform::Driver`.

Instead, you should wrap `cpufreq::Registration` into `Devres`. See
`drm::drv::Registration` for an example [1].

[1] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/drm/drv.rs?ref_type=heads#L173

> +
> +        pr_info!("CPUFreq DT driver registered\n");
> +
> +        Ok(drv)
> +    }
> +}
> +
> +module_platform_driver! {
> +    type: CPUFreqDTDriver,
> +    name: "cpufreq_dt",
> +    author: "Viresh Kumar <viresh.kumar@linaro.org>",
> +    description: "Generic CPUFreq DT driver",
> +    license: "GPL v2",
> +}
Viresh Kumar July 10, 2024, 8:56 a.m. UTC | #2
On 05-07-24, 13:32, Danilo Krummrich wrote:
> On 7/3/24 09:14, Viresh Kumar wrote:
> > +    fn probe(_dev: &mut platform::Device, _id_info: Option<&Self::IdInfo>) -> Result<Self::Data> {
> > +        let drv = Arc::new(
> > +            cpufreq::Registration::<CPUFreqDTDriver>::register(
> > +                c_str!("cpufreq-dt"),
> > +                (),
> > +                cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
> > +                true,
> > +            )?,
> > +            GFP_KERNEL,
> > +        )?;
> 
> Putting the `cpufreq::Registration` into `Arc<DeviceData>` is unsafe from a
> lifetime point of view. Nothing prevents this `Arc` to out-live the
> `platform::Driver`.

Hmm, the platform driver layer (in Rust) should guarantee that the
data will be freed from the driver removal path. Isn't it ?

> Instead, you should wrap `cpufreq::Registration` into `Devres`. See
> `drm::drv::Registration` for an example [1].
> 
> [1] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/drm/drv.rs?ref_type=heads#L173

I can convert to that too, will do it anyway.
Danilo Krummrich July 10, 2024, 3:28 p.m. UTC | #3
On Wed, Jul 10, 2024 at 02:26:52PM +0530, Viresh Kumar wrote:
> On 05-07-24, 13:32, Danilo Krummrich wrote:
> > On 7/3/24 09:14, Viresh Kumar wrote:
> > > +    fn probe(_dev: &mut platform::Device, _id_info: Option<&Self::IdInfo>) -> Result<Self::Data> {
> > > +        let drv = Arc::new(
> > > +            cpufreq::Registration::<CPUFreqDTDriver>::register(
> > > +                c_str!("cpufreq-dt"),
> > > +                (),
> > > +                cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
> > > +                true,
> > > +            )?,
> > > +            GFP_KERNEL,
> > > +        )?;
> > 
> > Putting the `cpufreq::Registration` into `Arc<DeviceData>` is unsafe from a
> > lifetime point of view. Nothing prevents this `Arc` to out-live the
> > `platform::Driver`.
> 
> Hmm, the platform driver layer (in Rust) should guarantee that the
> data will be freed from the driver removal path. Isn't it ?

No, the platform driver layer will only guarantee that it decreses the reference
count of the `Arc` by one, that doesn't guarantee a free. If something else
still holds a reference to the `Arc` it will keep the `Registration` alive,
unless it's wrapped by `Devres`.

> 
> > Instead, you should wrap `cpufreq::Registration` into `Devres`. See
> > `drm::drv::Registration` for an example [1].
> > 
> > [1] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/drm/drv.rs?ref_type=heads#L173
> 
> I can convert to that too, will do it anyway.
> 
> -- 
> viresh
>
Viresh Kumar July 11, 2024, 6:06 a.m. UTC | #4
On 10-07-24, 17:28, Danilo Krummrich wrote:
> No, the platform driver layer will only guarantee that it decreses the reference
> count of the `Arc` by one, that doesn't guarantee a free. If something else
> still holds a reference to the `Arc` it will keep the `Registration` alive,
> unless it's wrapped by `Devres`.

I see. Thanks.

There is one problem that I haven't found a solution to yet. If I make
the following change to the driver:

diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
index 315adca2a747..052ea2db095a 100644
--- a/drivers/cpufreq/rcpufreq_dt.rs
+++ b/drivers/cpufreq/rcpufreq_dt.rs
@@ -236,7 +236,7 @@ fn probe(dev: &mut platform::Device, _id_info: Option<&Self::IdInfo>) -> Result<

 module_platform_driver! {
     type: CPUFreqDTDriver,
-    name: "cpufreq_dt",
+    name: "cpufreq-dt",
     author: "Viresh Kumar <viresh.kumar@linaro.org>",
     description: "Generic CPUFreq DT driver",
     license: "GPL v2",

then I get this error:

  CLIPPY     drivers/cpufreq/rcpufreq_dt.o
error: expected one of `:`, `;`, or `=`, found `-`
   --> /mnt/ssd/all/work/repos/kernel/linux/drivers/cpufreq/rcpufreq_dt.rs:237:1
    |
237 | / module_platform_driver! {
238 | |     type: CPUFreqDTDriver,
239 | |     name: "cpufreq-dt",
240 | |     author: "Viresh Kumar <viresh.kumar@linaro.org>",
241 | |     description: "Generic CPUFreq DT driver",
242 | |     license: "GPL v2",
243 | | }
    | |_^ expected one of `:`, `;`, or `=`
    |
    = note: this error originates in the macro
    `$crate::prelude::module` which comes from the expansion of the
    macro `module_platform_driver` (in Nightly builds, run with -Z
    macro-backtrace for more info)


And because of that I had to change the name of the platform device
too in the existing kernel.
diff mbox series

Patch

diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index 94e55c40970a..eb9359bd3c5c 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -217,6 +217,18 @@  config CPUFREQ_DT
 
 	  If in doubt, say N.
 
+config CPUFREQ_DT_RUST
+	tristate "Rust based Generic DT based cpufreq driver"
+	depends on HAVE_CLK && OF && RUST
+	select CPUFREQ_DT_PLATDEV
+	select PM_OPP
+	help
+	  This adds a Rust based generic DT based cpufreq driver for frequency
+	  management.  It supports both uniprocessor (UP) and symmetric
+	  multiprocessor (SMP) systems.
+
+	  If in doubt, say N.
+
 config CPUFREQ_DT_PLATDEV
 	tristate "Generic DT based cpufreq platdev driver"
 	depends on OF
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 8d141c71b016..4981d908b803 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -15,6 +15,7 @@  obj-$(CONFIG_CPU_FREQ_GOV_COMMON)		+= cpufreq_governor.o
 obj-$(CONFIG_CPU_FREQ_GOV_ATTR_SET)	+= cpufreq_governor_attr_set.o
 
 obj-$(CONFIG_CPUFREQ_DT)		+= cpufreq-dt.o
+obj-$(CONFIG_CPUFREQ_DT_RUST)		+= rcpufreq_dt.o
 obj-$(CONFIG_CPUFREQ_DT_PLATDEV)	+= cpufreq-dt-platdev.o
 
 # Traces
diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
new file mode 100644
index 000000000000..652458e7a3b9
--- /dev/null
+++ b/drivers/cpufreq/rcpufreq_dt.rs
@@ -0,0 +1,225 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust based implementation of the cpufreq-dt driver.
+
+use core::format_args;
+
+use kernel::{
+    b_str, c_str, clk, cpufreq, cpumask::Cpumask, define_of_id_table, device::Device,
+    error::code::*, fmt, macros::vtable, module_platform_driver, of, opp, platform, prelude::*,
+    str::CString, sync::Arc,
+};
+
+// Finds exact supply name from the OF node.
+fn find_supply_name_exact(np: &of::DeviceNode, name: &str) -> Option<CString> {
+    let name_cstr = CString::try_from_fmt(fmt!("{}-supply", name)).ok()?;
+
+    np.find_property(&name_cstr).ok()?;
+    CString::try_from_fmt(fmt!("{}", name)).ok()
+}
+
+// Finds supply name for the CPU from DT.
+fn find_supply_names(dev: &Device, cpu: u32) -> Option<Vec<CString>> {
+    let np = of::DeviceNode::from_dev(dev).ok()?;
+
+    // Try "cpu0" for older DTs.
+    let name = match cpu {
+        0 => find_supply_name_exact(&np, "cpu0"),
+        _ => None,
+    }
+    .or(find_supply_name_exact(&np, "cpu"))?;
+
+    let mut list = Vec::with_capacity(1, GFP_KERNEL).ok()?;
+    list.push(name, GFP_KERNEL).ok()?;
+
+    Some(list)
+}
+
+// Represents the cpufreq dt device.
+struct CPUFreqDTDevice {
+    opp_table: opp::Table,
+    freq_table: opp::FreqTable,
+    #[allow(dead_code)]
+    mask: Cpumask,
+    #[allow(dead_code)]
+    token: Option<opp::ConfigToken>,
+    #[allow(dead_code)]
+    clk: clk::Clk,
+}
+
+struct CPUFreqDTDriver;
+
+#[vtable]
+impl opp::ConfigOps for CPUFreqDTDriver {}
+
+#[vtable]
+impl cpufreq::Driver for CPUFreqDTDriver {
+    type Data = ();
+    type PData = Arc<CPUFreqDTDevice>;
+
+    fn init(policy: &mut cpufreq::Policy) -> Result<Self::PData> {
+        let cpu = policy.cpu();
+        let dev = Device::from_cpu(cpu)?;
+        let mut mask = Cpumask::new()?;
+
+        mask.set(cpu);
+
+        let token = match find_supply_names(&dev, cpu) {
+            Some(names) => Some(
+                opp::Config::<Self>::new()
+                    .set_regulator_names(names)?
+                    .set(&dev)?,
+            ),
+            _ => None,
+        };
+
+        // Get OPP-sharing information from "operating-points-v2" bindings.
+        let fallback = match opp::Table::of_sharing_cpus(&dev, &mut mask) {
+            Ok(_) => false,
+            Err(e) => {
+                if e != ENOENT {
+                    return Err(e);
+                }
+
+                // "operating-points-v2" not supported. If the platform hasn't
+                // set sharing CPUs, fallback to all CPUs share the `Policy`
+                // for backward compatibility.
+                opp::Table::sharing_cpus(&dev, &mut mask).is_err()
+            }
+        };
+
+        // Initialize OPP tables for all policy cpus.
+        //
+        // For platforms not using "operating-points-v2" bindings, we do this
+        // before updating policy cpus. Otherwise, we will end up creating
+        // duplicate OPPs for the CPUs.
+        //
+        // OPPs might be populated at runtime, don't fail for error here unless
+        // it is -EPROBE_DEFER.
+        let mut opp_table = match opp::Table::from_of_cpumask(&dev, &mask) {
+            Ok(table) => table,
+            Err(e) => {
+                if e == EPROBE_DEFER {
+                    return Err(e);
+                }
+
+                // The table is added dynamically ?
+                opp::Table::from_dev(&dev)?
+            }
+        };
+
+        // The OPP table must be initialized, statically or dynamically, by this point.
+        opp_table.opp_count()?;
+
+        // Set sharing cpus for fallback scenario.
+        if fallback {
+            mask.set_all();
+            opp_table.set_sharing_cpus(&mask)?;
+        }
+
+        let mut transition_latency = opp_table.max_transition_latency() as u32;
+        if transition_latency == 0 {
+            transition_latency = cpufreq::ETERNAL_LATENCY;
+        }
+
+        let freq_table = opp_table.to_cpufreq_table()?;
+        let clk = policy
+            .set_freq_table(freq_table.table())
+            .set_dvfs_possible_from_any_cpu()
+            .set_suspend_freq((opp_table.suspend_freq() / 1000) as u32)
+            .set_transition_latency(transition_latency)
+            .set_clk(&dev, None)?;
+
+        mask.copy(policy.cpus());
+
+        Ok(Arc::new(
+            CPUFreqDTDevice {
+                opp_table,
+                freq_table,
+                mask,
+                token,
+                clk,
+            },
+            GFP_KERNEL,
+        )?)
+    }
+
+    fn exit(_policy: &mut cpufreq::Policy, _data: Option<Self::PData>) -> Result<()> {
+        Ok(())
+    }
+
+    fn online(_policy: &mut cpufreq::Policy) -> Result<()> {
+        // We did light-weight tear down earlier, nothing to do here.
+        Ok(())
+    }
+
+    fn offline(_policy: &mut cpufreq::Policy) -> Result<()> {
+        // Preserve policy->data and don't free resources on light-weight
+        // tear down.
+        Ok(())
+    }
+
+    fn suspend(policy: &mut cpufreq::Policy) -> Result<()> {
+        policy.generic_suspend()
+    }
+
+    fn verify(data: &mut cpufreq::PolicyData) -> Result<()> {
+        data.generic_verify()
+    }
+
+    fn target_index(policy: &mut cpufreq::Policy, index: u32) -> Result<()> {
+        let data = match policy.data::<Self::PData>() {
+            Some(data) => data,
+            None => return Err(ENOENT),
+        };
+
+        let freq = data.freq_table.freq(index.try_into().unwrap())? as u64;
+        data.opp_table.set_rate(freq * 1000)
+    }
+
+    fn get(policy: &mut cpufreq::Policy) -> Result<u32> {
+        policy.generic_get()
+    }
+
+    fn set_boost(_policy: &mut cpufreq::Policy, _state: i32) -> Result<()> {
+        Ok(())
+    }
+
+    fn register_em(policy: &mut cpufreq::Policy) {
+        policy.register_em_opp()
+    }
+}
+
+type DeviceData = Box<cpufreq::Registration<CPUFreqDTDriver>>;
+
+impl platform::Driver for CPUFreqDTDriver {
+    type Data = Arc<DeviceData>;
+
+    define_of_id_table! {(), [
+        (of::DeviceId(b_str!("operating-points-v2")), None),
+    ]}
+
+    fn probe(_dev: &mut platform::Device, _id_info: Option<&Self::IdInfo>) -> Result<Self::Data> {
+        let drv = Arc::new(
+            cpufreq::Registration::<CPUFreqDTDriver>::register(
+                c_str!("cpufreq-dt"),
+                (),
+                cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
+                true,
+            )?,
+            GFP_KERNEL,
+        )?;
+
+        pr_info!("CPUFreq DT driver registered\n");
+
+        Ok(drv)
+    }
+}
+
+module_platform_driver! {
+    type: CPUFreqDTDriver,
+    name: "cpufreq_dt",
+    author: "Viresh Kumar <viresh.kumar@linaro.org>",
+    description: "Generic CPUFreq DT driver",
+    license: "GPL v2",
+}