| Message ID | c68081e18d939aefc7f6dac798df6b72e81bba4b.1738832118.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | Rust bindings for cpufreq and OPP core + sample driver | expand |
On Thu, Feb 06, 2025 at 02:58:27PM +0530, Viresh Kumar wrote: > This adds very basic bindings for the clk framework, implements only > clk_get() and clk_put(). These are the bare minimum bindings required > for many users and are simple enough to add in the first attempt. > > These will be used by Rust based cpufreq / OPP core to begin with. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > MAINTAINERS | 1 + > rust/bindings/bindings_helper.h | 1 + > rust/kernel/clk.rs | 48 +++++++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 2 ++ > 4 files changed, 52 insertions(+) > create mode 100644 rust/kernel/clk.rs > > diff --git a/MAINTAINERS b/MAINTAINERS > index ff4511914e0a..604717065476 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5780,6 +5780,7 @@ F: include/dt-bindings/clock/ > F: include/linux/clk-pr* > F: include/linux/clk/ > F: include/linux/of_clk.h > +F: rust/kernel/clk.rs > X: drivers/clk/clkdev.c > > COMMON INTERNET FILE SYSTEM CLIENT (CIFS and SMB3) > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index 59b4bc49d039..4eadcf645df0 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -10,6 +10,7 @@ > #include <linux/blk-mq.h> > #include <linux/blk_types.h> > #include <linux/blkdev.h> > +#include <linux/clk.h> > #include <linux/cpu.h> > #include <linux/cpumask.h> > #include <linux/cred.h> > diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs > new file mode 100644 > index 000000000000..123cdb43b115 > --- /dev/null > +++ b/rust/kernel/clk.rs > @@ -0,0 +1,48 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Clock abstractions. > +//! > +//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h) > + > +use crate::{ > + bindings, > + device::Device, > + error::{from_err_ptr, Result}, > + prelude::*, > +}; > + > +use core::ptr; > + > +/// A simple implementation of `struct clk` from the C code. > +#[repr(transparent)] > +pub struct Clk(*mut bindings::clk); Guess this should be Opaque<bindings::clk>. > + > +impl Clk { > + /// Creates `Clk` instance for a device and a connection id. > + pub fn new(dev: &Device, name: Option<&CStr>) -> Result<Self> { > + let con_id = if let Some(name) = name { > + name.as_ptr() as *const _ > + } else { > + ptr::null() > + }; > + > + // SAFETY: It is safe to call `clk_get()`, on a device pointer earlier received from the C > + // code. > + Ok(Self(from_err_ptr(unsafe { > + bindings::clk_get(dev.as_raw(), con_id) > + })?)) > + } > + > + /// Obtain the raw `struct clk *`. > + pub fn as_raw(&self) -> *mut bindings::clk { > + self.0 > + } > +} > + > +impl Drop for Clk { > + fn drop(&mut self) { > + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to > + // relinquish it now. > + unsafe { bindings::clk_put(self.0) }; > + } > +} > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index ccbf7fa087a0..77d3b1f82154 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -40,6 +40,8 @@ > pub mod block; > #[doc(hidden)] > pub mod build_assert; > +#[cfg(CONFIG_COMMON_CLK)] > +pub mod clk; > pub mod cpu; > pub mod cpumask; > pub mod cred; > -- > 2.31.1.272.g89b43f80a514 > >
On Thu, Feb 06, 2025 at 12:49:14PM +0100, Danilo Krummrich wrote: > On Thu, Feb 06, 2025 at 02:58:27PM +0530, Viresh Kumar wrote: > > This adds very basic bindings for the clk framework, implements only > > clk_get() and clk_put(). These are the bare minimum bindings required > > for many users and are simple enough to add in the first attempt. > > > > These will be used by Rust based cpufreq / OPP core to begin with. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > MAINTAINERS | 1 + > > rust/bindings/bindings_helper.h | 1 + > > rust/kernel/clk.rs | 48 +++++++++++++++++++++++++++++++++ > > rust/kernel/lib.rs | 2 ++ > > 4 files changed, 52 insertions(+) > > create mode 100644 rust/kernel/clk.rs > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index ff4511914e0a..604717065476 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -5780,6 +5780,7 @@ F: include/dt-bindings/clock/ > > F: include/linux/clk-pr* > > F: include/linux/clk/ > > F: include/linux/of_clk.h > > +F: rust/kernel/clk.rs > > X: drivers/clk/clkdev.c > > > > COMMON INTERNET FILE SYSTEM CLIENT (CIFS and SMB3) > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > > index 59b4bc49d039..4eadcf645df0 100644 > > --- a/rust/bindings/bindings_helper.h > > +++ b/rust/bindings/bindings_helper.h > > @@ -10,6 +10,7 @@ > > #include <linux/blk-mq.h> > > #include <linux/blk_types.h> > > #include <linux/blkdev.h> > > +#include <linux/clk.h> > > #include <linux/cpu.h> > > #include <linux/cpumask.h> > > #include <linux/cred.h> > > diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs > > new file mode 100644 > > index 000000000000..123cdb43b115 > > --- /dev/null > > +++ b/rust/kernel/clk.rs > > @@ -0,0 +1,48 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +//! Clock abstractions. > > +//! > > +//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h) > > + > > +use crate::{ > > + bindings, > > + device::Device, > > + error::{from_err_ptr, Result}, > > + prelude::*, > > +}; > > + > > +use core::ptr; > > + > > +/// A simple implementation of `struct clk` from the C code. > > +#[repr(transparent)] > > +pub struct Clk(*mut bindings::clk); > > Guess this should be Opaque<bindings::clk>. Sorry, I meant NonNull<bindings::clk>. > > > + > > +impl Clk { > > + /// Creates `Clk` instance for a device and a connection id. > > + pub fn new(dev: &Device, name: Option<&CStr>) -> Result<Self> { > > + let con_id = if let Some(name) = name { > > + name.as_ptr() as *const _ > > + } else { > > + ptr::null() > > + }; > > + > > + // SAFETY: It is safe to call `clk_get()`, on a device pointer earlier received from the C > > + // code. > > + Ok(Self(from_err_ptr(unsafe { > > + bindings::clk_get(dev.as_raw(), con_id) > > + })?)) > > + } > > + > > + /// Obtain the raw `struct clk *`. > > + pub fn as_raw(&self) -> *mut bindings::clk { > > + self.0 > > + } > > +} > > + > > +impl Drop for Clk { > > + fn drop(&mut self) { > > + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to > > + // relinquish it now. > > + unsafe { bindings::clk_put(self.0) }; > > + } > > +} > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > > index ccbf7fa087a0..77d3b1f82154 100644 > > --- a/rust/kernel/lib.rs > > +++ b/rust/kernel/lib.rs > > @@ -40,6 +40,8 @@ > > pub mod block; > > #[doc(hidden)] > > pub mod build_assert; > > +#[cfg(CONFIG_COMMON_CLK)] > > +pub mod clk; > > pub mod cpu; > > pub mod cpumask; > > pub mod cred; > > -- > > 2.31.1.272.g89b43f80a514 > > > >
Quoting Danilo Krummrich (2025-02-06 03:52:41) > On Thu, Feb 06, 2025 at 12:49:14PM +0100, Danilo Krummrich wrote: > > On Thu, Feb 06, 2025 at 02:58:27PM +0530, Viresh Kumar wrote: > > > diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs > > > new file mode 100644 > > > index 000000000000..123cdb43b115 > > > --- /dev/null > > > +++ b/rust/kernel/clk.rs > > > @@ -0,0 +1,48 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > + > > > +//! Clock abstractions. > > > +//! > > > +//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h) > > > + > > > +use crate::{ > > > + bindings, > > > + device::Device, > > > + error::{from_err_ptr, Result}, > > > + prelude::*, > > > +}; > > > + > > > +use core::ptr; > > > + > > > +/// A simple implementation of `struct clk` from the C code. > > > +#[repr(transparent)] > > > +pub struct Clk(*mut bindings::clk); > > > > Guess this should be Opaque<bindings::clk>. > > Sorry, I meant NonNull<bindings::clk>. NULL is a valid clk. It's like "don't care" in the common clk framework as most clk consumer operations bail out early in that case.
On Thu, Feb 06, 2025 at 12:05:59PM -0800, Stephen Boyd wrote: > Quoting Danilo Krummrich (2025-02-06 03:52:41) > > On Thu, Feb 06, 2025 at 12:49:14PM +0100, Danilo Krummrich wrote: > > > On Thu, Feb 06, 2025 at 02:58:27PM +0530, Viresh Kumar wrote: > > > > diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs > > > > new file mode 100644 > > > > index 000000000000..123cdb43b115 > > > > --- /dev/null > > > > +++ b/rust/kernel/clk.rs > > > > @@ -0,0 +1,48 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > + > > > > +//! Clock abstractions. > > > > +//! > > > > +//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h) > > > > + > > > > +use crate::{ > > > > + bindings, > > > > + device::Device, > > > > + error::{from_err_ptr, Result}, > > > > + prelude::*, > > > > +}; > > > > + > > > > +use core::ptr; > > > > + > > > > +/// A simple implementation of `struct clk` from the C code. > > > > +#[repr(transparent)] > > > > +pub struct Clk(*mut bindings::clk); > > > > > > Guess this should be Opaque<bindings::clk>. > > > > Sorry, I meant NonNull<bindings::clk>. > > NULL is a valid clk. It's like "don't care" in the common clk framework Thanks for clarifying! > as most clk consumer operations bail out early in that case. Most? Does that mean NULL isn't *always* valid?
On 07-02-25, 00:11, Danilo Krummrich wrote: > On Thu, Feb 06, 2025 at 12:05:59PM -0800, Stephen Boyd wrote: > > Quoting Danilo Krummrich (2025-02-06 03:52:41) > > > On Thu, Feb 06, 2025 at 12:49:14PM +0100, Danilo Krummrich wrote: > > > > On Thu, Feb 06, 2025 at 02:58:27PM +0530, Viresh Kumar wrote: > > > > > +/// A simple implementation of `struct clk` from the C code. > > > > > +#[repr(transparent)] > > > > > +pub struct Clk(*mut bindings::clk); > > > > > > > > Guess this should be Opaque<bindings::clk>. > > > > > > Sorry, I meant NonNull<bindings::clk>. > > > > NULL is a valid clk. It's like "don't care" in the common clk framework > > Thanks for clarifying! > Guess this should be Opaque<bindings::clk>. So it should be this now ?
On 07-02-25, 14:54, Viresh Kumar wrote: > On 07-02-25, 00:11, Danilo Krummrich wrote: > > Guess this should be Opaque<bindings::clk>. > > So it should be this now ? Also, I should be using ARef and AlwaysRefCounted along with that ? I am not sure if I can use those with the clk API. Yes, it is refcounted but there is no direct way of incrementing the refcount unlike other frameworks. clk_get() accepts a dev pointer and a char pointer, whereas clk_put() accepts the clk pointer itself.
On 2/7/25 10:24 AM, Viresh Kumar wrote: > On 07-02-25, 00:11, Danilo Krummrich wrote: >> On Thu, Feb 06, 2025 at 12:05:59PM -0800, Stephen Boyd wrote: >>> Quoting Danilo Krummrich (2025-02-06 03:52:41) >>>> On Thu, Feb 06, 2025 at 12:49:14PM +0100, Danilo Krummrich wrote: >>>>> On Thu, Feb 06, 2025 at 02:58:27PM +0530, Viresh Kumar wrote: > >>>>>> +/// A simple implementation of `struct clk` from the C code. >>>>>> +#[repr(transparent)] >>>>>> +pub struct Clk(*mut bindings::clk); >>>>> >>>>> Guess this should be Opaque<bindings::clk>. >>>> >>>> Sorry, I meant NonNull<bindings::clk>. >>> >>> NULL is a valid clk. It's like "don't care" in the common clk framework >> >> Thanks for clarifying! > >> Guess this should be Opaque<bindings::clk>. > > So it should be this now ? I actually meant NonNull<bindings::clk>, which I corrected in a subsequent mail, where Stephen pointed out that NULL is a valid value for a struct clk.
On 07-02-25, 18:19, Danilo Krummrich wrote: > On 2/7/25 10:24 AM, Viresh Kumar wrote: > > On 07-02-25, 00:11, Danilo Krummrich wrote: > > > On Thu, Feb 06, 2025 at 12:05:59PM -0800, Stephen Boyd wrote: > > > > Quoting Danilo Krummrich (2025-02-06 03:52:41) > > > > > On Thu, Feb 06, 2025 at 12:49:14PM +0100, Danilo Krummrich wrote: > > > > > > On Thu, Feb 06, 2025 at 02:58:27PM +0530, Viresh Kumar wrote: > > > > > > > > > +/// A simple implementation of `struct clk` from the C code. > > > > > > > +#[repr(transparent)] > > > > > > > +pub struct Clk(*mut bindings::clk); > > > > > > > > > > > > Guess this should be Opaque<bindings::clk>. > > > > > > > > > > Sorry, I meant NonNull<bindings::clk>. > > > > > > > > NULL is a valid clk. It's like "don't care" in the common clk framework > > > > > > Thanks for clarifying! > > > > > Guess this should be Opaque<bindings::clk>. > > > > So it should be this now ? > > I actually meant NonNull<bindings::clk>, which I corrected in a subsequent mail, > where Stephen pointed out that NULL is a valid value for a struct clk. Ahh okay, so no changes required now. Thanks.
Quoting Danilo Krummrich (2025-02-06 15:11:31) > On Thu, Feb 06, 2025 at 12:05:59PM -0800, Stephen Boyd wrote: > > > as most clk consumer operations bail out early in that case. > > Most? Does that mean NULL isn't *always* valid? I'm hedging because the common clk framework is just one implementation of the clk.h API in the kernel. We still have a couple other implementations (sadly) where I haven't checked to see what they do if a NULL pointer is passed in. But NULL should always be a valid clk handle per the clk.h API documentation, so an implementation that fails at that is broken.
Hi Viresh > On 6 Feb 2025, at 06:28, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > This adds very basic bindings for the clk framework, implements only > clk_get() and clk_put(). These are the bare minimum bindings required > for many users and are simple enough to add in the first attempt. I am missing clk_prepare_enable/clk_disable_unprepare. Otherwise I see no way of enabling and disabling clks. IMHO I would also consider these as “bare minimum”. — Daniel
diff --git a/MAINTAINERS b/MAINTAINERS index ff4511914e0a..604717065476 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5780,6 +5780,7 @@ F: include/dt-bindings/clock/ F: include/linux/clk-pr* F: include/linux/clk/ F: include/linux/of_clk.h +F: rust/kernel/clk.rs X: drivers/clk/clkdev.c COMMON INTERNET FILE SYSTEM CLIENT (CIFS and SMB3) diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 59b4bc49d039..4eadcf645df0 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -10,6 +10,7 @@ #include <linux/blk-mq.h> #include <linux/blk_types.h> #include <linux/blkdev.h> +#include <linux/clk.h> #include <linux/cpu.h> #include <linux/cpumask.h> #include <linux/cred.h> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs new file mode 100644 index 000000000000..123cdb43b115 --- /dev/null +++ b/rust/kernel/clk.rs @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Clock abstractions. +//! +//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h) + +use crate::{ + bindings, + device::Device, + error::{from_err_ptr, Result}, + prelude::*, +}; + +use core::ptr; + +/// A simple implementation of `struct clk` from the C code. +#[repr(transparent)] +pub struct Clk(*mut bindings::clk); + +impl Clk { + /// Creates `Clk` instance for a device and a connection id. + pub fn new(dev: &Device, name: Option<&CStr>) -> Result<Self> { + let con_id = if let Some(name) = name { + name.as_ptr() as *const _ + } else { + ptr::null() + }; + + // SAFETY: It is safe to call `clk_get()`, on a device pointer earlier received from the C + // code. + Ok(Self(from_err_ptr(unsafe { + bindings::clk_get(dev.as_raw(), con_id) + })?)) + } + + /// Obtain the raw `struct clk *`. + pub fn as_raw(&self) -> *mut bindings::clk { + self.0 + } +} + +impl Drop for Clk { + fn drop(&mut self) { + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to + // relinquish it now. + unsafe { bindings::clk_put(self.0) }; + } +} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index ccbf7fa087a0..77d3b1f82154 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -40,6 +40,8 @@ pub mod block; #[doc(hidden)] pub mod build_assert; +#[cfg(CONFIG_COMMON_CLK)] +pub mod clk; pub mod cpu; pub mod cpumask; pub mod cred;
This adds very basic bindings for the clk framework, implements only clk_get() and clk_put(). These are the bare minimum bindings required for many users and are simple enough to add in the first attempt. These will be used by Rust based cpufreq / OPP core to begin with. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- MAINTAINERS | 1 + rust/bindings/bindings_helper.h | 1 + rust/kernel/clk.rs | 48 +++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 2 ++ 4 files changed, 52 insertions(+) create mode 100644 rust/kernel/clk.rs