| 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 |
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", > +}
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.
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 >
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 --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", +}
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