[V3,2/2] rust: Add initial clk abstractions

Commit Message

Viresh Kumar March 3, 2025, 9:58 a.m. UTC

Add initial abstractions for the clk APIs. These provide the minimal
functionality needed for common use cases, making them straightforward
to introduce in the first iteration.

These will be used by Rust based cpufreq / OPP layers to begin with.

Tested-by: Daniel Almeida <[e-mail]>
Reviewed-by: Daniel Almeida <[e-mail]>
Signed-off-by: Viresh Kumar <[e-mail]>
---
 MAINTAINERS        |   1 +
 rust/kernel/clk.rs | 134 +++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs |   1 +
 3 files changed, 136 insertions(+)
 create mode 100644 rust/kernel/clk.rs

Comments

Alice Ryhl March 3, 2025, 10:04 a.m. UTC | #1

On Mon, Mar 3, 2025 at 11:00 AM Viresh Kumar <[e-mail]> wrote:
>
> Add initial abstractions for the clk APIs. These provide the minimal
> functionality needed for common use cases, making them straightforward
> to introduce in the first iteration.
>
> These will be used by Rust based cpufreq / OPP layers to begin with.
>
> Tested-by: Daniel Almeida <[e-mail]>
> Reviewed-by: Daniel Almeida <[e-mail]>
> Signed-off-by: Viresh Kumar <[e-mail]>

Every function in this patch could be #[inline]. Otherwise rustc will
generate a bunch of wrapper functions that just forward a call into C.

>  MAINTAINERS        |   1 +
>  rust/kernel/clk.rs | 134 +++++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs |   1 +
>  3 files changed, 136 insertions(+)
>  create mode 100644 rust/kernel/clk.rs
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 726110d3c988..96e2574f41c0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5779,6 +5779,7 @@ F:        include/linux/clk-pr*
>  F:     include/linux/clk/
>  F:     include/linux/of_clk.h
>  F:     rust/helpers/clk.c
> +F:     rust/kernel/clk.rs
>  X:     drivers/clk/clkdev.c
>
>  COMMON INTERNET FILE SYSTEM CLIENT (CIFS and SMB3)
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> new file mode 100644
> index 000000000000..1fa5b7298373
> --- /dev/null
> +++ b/rust/kernel/clk.rs
> @@ -0,0 +1,134 @@
> +// 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, to_result, Result},
> +    prelude::*,
> +};
> +
> +use core::{ops::Deref, ptr};
> +
> +/// Frequency unit.
> +pub type Hertz = crate::ffi::c_ulong;
> +
> +/// A simple implementation of `struct clk` from the C code.
> +#[repr(transparent)]
> +pub struct Clk(*mut bindings::clk);

This needs an invariants section.

> +
> +impl Clk {
> +    /// Gets clock corresponding to a device and a connection id and returns `Clk`.
> +    pub fn get(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()` for a valid device pointer.
> +        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
> +    }
> +
> +    /// Clock enable.
> +    pub fn enable(&self) -> Result<()> {
> +        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
> +        // by `clk_get()`.
> +        to_result(unsafe { bindings::clk_enable(self.as_raw()) })
> +    }
> +
> +    /// Clock disable.
> +    pub fn disable(&self) {
> +        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
> +        // by `clk_get()`.
> +        unsafe { bindings::clk_disable(self.as_raw()) };
> +    }
> +
> +    /// Clock prepare.
> +    pub fn prepare(&self) -> Result<()> {
> +        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
> +        // by `clk_get()`.
> +        to_result(unsafe { bindings::clk_prepare(self.as_raw()) })
> +    }
> +
> +    /// Clock unprepare.
> +    pub fn unprepare(&self) {
> +        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
> +        // by `clk_get()`.
> +        unsafe { bindings::clk_unprepare(self.as_raw()) };
> +    }
> +
> +    /// Clock prepare enable.
> +    pub fn prepare_enable(&self) -> Result<()> {
> +        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
> +        // by `clk_get()`.
> +        to_result(unsafe { bindings::clk_prepare_enable(self.as_raw()) })
> +    }
> +
> +    /// Clock disable unprepare.
> +    pub fn disable_unprepare(&self) {
> +        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
> +        // by `clk_get()`.
> +        unsafe { bindings::clk_disable_unprepare(self.as_raw()) };
> +    }
> +
> +    /// Clock get rate.
> +    pub fn rate(&self) -> Hertz {
> +        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
> +        // by `clk_get()`.
> +        unsafe { bindings::clk_get_rate(self.as_raw()) }
> +    }
> +
> +    /// Clock set rate.
> +    pub fn set_rate(&self, rate: Hertz) -> Result<()> {
> +        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
> +        // by `clk_get()`.
> +        to_result(unsafe { bindings::clk_set_rate(self.as_raw(), rate) })
> +    }
> +}
> +
> +impl Drop for Clk {
> +    fn drop(&mut self) {
> +        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
> +        // by `clk_get()`.
> +        unsafe { bindings::clk_put(self.as_raw()) };
> +    }
> +}
> +
> +/// A simple implementation of optional `Clk`.
> +pub struct OptionalClk(Clk);

What is this?

> +impl OptionalClk {
> +    /// Gets optional clock corresponding to a device and a connection id and returns `Clk`.
> +    pub fn get(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_optional()` for a valid device pointer.
> +        Ok(Self(Clk(from_err_ptr(unsafe {
> +            bindings::clk_get_optional(dev.as_raw(), con_id)
> +        })?)))
> +    }
> +}
> +
> +// Make `OptionalClk` behave like `Clk`.
> +impl Deref for OptionalClk {
> +    type Target = Clk;
> +
> +    fn deref(&self) -> &Clk {
> +        &self.0
> +    }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0911..324b86f127a0 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -40,6 +40,7 @@
>  pub mod block;
>  #[doc(hidden)]
>  pub mod build_assert;
> +pub mod clk;
>  pub mod cred;
>  pub mod device;
>  pub mod device_id;
> --
> 2.31.1.272.g89b43f80a514
>

Miguel Ojeda March 3, 2025, 10:16 a.m. UTC | #2

On Mon, Mar 3, 2025 at 11:00 AM Viresh Kumar <[e-mail]> wrote:
>
> +/// Frequency unit.
> +pub type Hertz = crate::ffi::c_ulong;

Do we want this to be an alias or would it make sense to take the
chance to make this a newtype?

> +/// A simple implementation of `struct clk` from the C code.

In general, please try to provide some documentation and/or examples
where possible/reasonable.

> +    /// Gets clock corresponding to a device and a connection id and returns `Clk`.

Please use intra-doc links wherever they may work.

> +    /// Clock enable.

Should these be e.g. "Enable the clock." or similar?

Moreover, I see quite a lot of documentation about some of these
functions in the C side. I think we should not regress on that. Should
we link to the C docs, too?

> +    pub fn enable(&self) -> Result<()> {

Can this simply use `Result`?

> +pub mod clk;

Just to double check, do we need any `cfg`? I see some functions exist
even without e.g. `CONFIG_COMMON_CLK`, but I wanted to ask if you
tried to build it without it enabled.

Thanks!

Cheers,
Miguel

Viresh Kumar March 3, 2025, 11:32 a.m. UTC | #3

On 03-03-25, 11:04, Alice Ryhl wrote:
> > +/// A simple implementation of optional `Clk`.
> > +pub struct OptionalClk(Clk);
> 
> What is this?

This came up during review of the previous version [1]. A resource (clk in this
case) can be optional for a driver to work and such drivers call
clk_get_optional(). Similar APIs are present in other frameworks as well.

I was not sure if this should be implemented as a separate method in struct Clk
itself or like this.

> > +impl OptionalClk {
> > +    /// Gets optional clock corresponding to a device and a connection id and returns `Clk`.
> > +    pub fn get(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_optional()` for a valid device pointer.
> > +        Ok(Self(Clk(from_err_ptr(unsafe {
> > +            bindings::clk_get_optional(dev.as_raw(), con_id)
> > +        })?)))
> > +    }
> > +}

Viresh Kumar March 3, 2025, 11:44 a.m. UTC | #4

On 03-03-25, 11:16, Miguel Ojeda wrote:
> On Mon, Mar 3, 2025 at 11:00 AM Viresh Kumar <[e-mail]> wrote:
> >
> > +/// Frequency unit.
> > +pub type Hertz = crate::ffi::c_ulong;
> 
> Do we want this to be an alias or would it make sense to take the
> chance to make this a newtype?

Actually Daneil did suggest to make this "Struct Hertz(c_ulong)", but then I
looked at rust/kernel/time.rs:

pub type Jiffies = crate::ffi::c_ulong;

And I thought this is probably what everyone would have agreed to and did it
this way.

> > +    /// Clock enable.
> 
> Should these be e.g. "Enable the clock." or similar?
> 
> Moreover, I see quite a lot of documentation about some of these
> functions in the C side. I think we should not regress on that. Should
> we link to the C docs, too?

Something like this (from print.rs) ?

/// [`pr_debug`]: https://docs.kernel.org/core-api/printk-basics.html#c.pr_debug

> > +pub mod clk;
> 
> Just to double check, do we need any `cfg`? I see some functions exist
> even without e.g. `CONFIG_COMMON_CLK`, but I wanted to ask if you
> tried to build it without it enabled.

Yes, I was using this under `cfg` earlier, but removed that recently after
testing this without CONFIG_HAVE_CLK. clk.h provides wrappers for cases where
the config option isn't available.

Miguel Ojeda March 3, 2025, 3:50 p.m. UTC | #5

On Mon, Mar 3, 2025 at 12:44 PM Viresh Kumar <[e-mail]> wrote:
>
> Actually Daneil did suggest to make this "Struct Hertz(c_ulong)", but then I
> looked at rust/kernel/time.rs:
>
> pub type Jiffies = crate::ffi::c_ulong;
>
> And I thought this is probably what everyone would have agreed to and did it
> this way.

I see, thanks!

Personally, I would prefer that we consider using "strong typedefs"
wherever possible, as early as possible -- otherwise it will be more
painful later on.

> Something like this (from print.rs) ?
>
> /// [`pr_debug`]: https://docs.kernel.org/core-api/printk-basics.html#c.pr_debug

Yeah. The docs in the C side may or may not make perfect sense on the
Rust side though, i.e. it it may make sense to adapt them, add
examples, etc., but it may be that they are exactly what you want, in
which case only linking may be OK (i.e. normally linking is something
to do on top of the new docs).

> Yes, I was using this under `cfg` earlier, but removed that recently after
> testing this without CONFIG_HAVE_CLK. clk.h provides wrappers for cases where
> the config option isn't available.

Great, thanks a lot for testing that!

Cheers,
Miguel

Viresh Kumar March 4, 2025, 8:53 a.m. UTC | #6

On 03-03-25, 11:16, Miguel Ojeda wrote:
> On Mon, Mar 3, 2025 at 11:00 AM Viresh Kumar <[e-mail]> wrote:
> >
> > +/// Frequency unit.
> > +pub type Hertz = crate::ffi::c_ulong;
> 
> Do we want this to be an alias or would it make sense to take the
> chance to make this a newtype?

I have tried some improvements based on your (and Alice's comments), please see
if it looks any better now.

Miguel Ojeda March 4, 2025, 9:37 a.m. UTC | #7

On Tue, Mar 4, 2025 at 9:53 AM Viresh Kumar <[e-mail]> wrote:
>
> I have tried some improvements based on your (and Alice's comments), please see
> if it looks any better now.

That looks much, much better, thanks!

> +/// Frequency unit.
> +#[derive(Copy, Clone, PartialEq, Eq, Debug)]
> +pub struct Hertz(c_ulong);

Please add a quick example for this one, e.g. constructing it and
comparing the value with an `assert_eq!` and another line comparing
two different `Hertz` objects for instance. After all, this one we can
even run it easily!

> +/// This structure represents the Rust abstraction for a C [`struct clk`].

Nit: the usual style is e.g.:

    /// An instance of a PHY device.
    ///
    /// Wraps the kernel's [`struct phy_device`].

i.e. the first line does not need to say "This structure" ... "Rust
abstraction" etc.

> +/// Instances of this type are reference-counted. Calling `get` ensures that the allocation remains

Please use intra-doc links (also for `OptionalClk` etc.).

> +/// ## Example

Nit: plural (even if there is a single example).

> +///     clk.disable_unprepare();

Looking at the example, a question that one may have is: should we
have something like a scope guard or a closure-passing API for this,
or does it not make sense in general?

> +    /// Enable the clock.
> +    #[inline]
> +    pub fn enable(&self) -> Result {

Should the users of these methods consult the C API side for the
comments/docs? e.g. should they read
https://docs.kernel.org/driver-api/clk.html#locking?

If so, please at least provide a link to the C API or the relevant
docs. e.g. https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable.
Otherwise, if there is something there that should be mentioned here
directly, please do so.

In other words, in general, the goal is that you can find everything
you need in the Rust docs, even if those docs may sometimes rely on a
link there to the C side or a Doc/ document to avoid duplication. But
the information or the way to find that information should be there,
if that makes sense.

> +        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
> +        // by `clk_get()`.

We should probably say why we know that, i.e. due to the invariant,
unless I am missing something.

By the way, in the constructor, you should add/use an `// INVARIANT:`
comment (please grep to see how others do it).

> +///     let expected_rate = Hertz::new(1_000_000_000);

Would it be useful for users to have constructors for a few SI
prefixes, e.g. `Hertz::from_giga`? I see some big constants used for
e.g. `set_rate` in the C side, so I guess it could.

On top of that, would any other kind of operation make sense? For
instance, `.inverse()` to/from time or things like that -- we don't
need to do any of this now, of course, but it may be worth taking a
minute to investigate how we could improve the type now that we have
it.

Thanks!

Cheers,
Miguel

Viresh Kumar March 5, 2025, 11:46 a.m. UTC | #8

On 04-03-25, 10:37, Miguel Ojeda wrote:
> On Tue, Mar 4, 2025 at 9:53 AM Viresh Kumar <[e-mail]> wrote:
> > +///     clk.disable_unprepare();
> 
> Looking at the example, a question that one may have is: should we
> have something like a scope guard or a closure-passing API for this,
> or does it not make sense in general?

Something like this (untested) ?

+/// Runs a cleanup function/closure when dropped.
+///
+/// The [`ClkGuard::dismiss`] function prevents the cleanup function from running.
+///
+pub type ClkGuard<'a> = ScopeGuard<&'a Clk, fn(&Clk)>;
+
 /// A reference-counted clock.
 ///
 /// This represents the Rust abstraction for the C [`struct clk`].
@@ -139,10 +146,12 @@ pub fn as_raw(&self) -> *mut bindings::clk {
     ///
     /// [`clk_enable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable
     #[inline]
-    pub fn enable(&self) -> Result {
+    pub fn enable(&self) -> Result<ClkGuard<'_>> {
         // SAFETY: By the type invariants, it is safe to call clk APIs of the C code for a clock
         // pointer earlier returned by [`clk_get`].
-        to_result(unsafe { bindings::clk_enable(self.as_raw()) })
+        to_result(unsafe { bindings::clk_enable(self.as_raw()) })?;
+
+        Ok(ClkGuard::new_with_data(self, Clk::disable))
     }
 
     /// Disable the clock.
@@ -163,10 +172,12 @@ pub fn disable(&self) {
     ///
     /// [`clk_prepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_prepare
     #[inline]
-    pub fn prepare(&self) -> Result {
+    pub fn prepare(&self) -> Result<ClkGuard<'_>> {
         // SAFETY: By the type invariants, it is safe to call clk APIs of the C code for a clock
         // pointer earlier returned by [`clk_get`].
-        to_result(unsafe { bindings::clk_prepare(self.as_raw()) })
+        to_result(unsafe { bindings::clk_prepare(self.as_raw()) })?;
+
+        Ok(ClkGuard::new_with_data(self, Clk::unprepare))
     }
 
     /// Unprepare the clock.
@@ -185,10 +196,12 @@ pub fn unprepare(&self) {
     ///
     /// Equivalent to calling [`Clk::prepare`] followed by [`Clk::enable`].
     #[inline]
-    pub fn prepare_enable(&self) -> Result {
+    pub fn prepare_enable(&self) -> Result<ClkGuard<'_>> {
         // SAFETY: By the type invariants, it is safe to call clk APIs of the C code for a clock
         // pointer earlier returned by [`clk_get`].
-        to_result(unsafe { bindings::clk_prepare_enable(self.as_raw()) })
+        to_result(unsafe { bindings::clk_prepare_enable(self.as_raw()) })?;
+
+        Ok(ClkGuard::new_with_data(self, Clk::disable_unprepare))
     }
 
> > +///     let expected_rate = Hertz::new(1_000_000_000);
> 
> On top of that, would any other kind of operation make sense? For
> instance, `.inverse()` to/from time or things like that -- we don't
> need to do any of this now, of course, but it may be worth taking a
> minute to investigate how we could improve the type now that we have
> it.

I am not sure if we should be implementing them right away, I agree
about from_hz/khz/mhz/ghz though. Let some users come for that and
then we can consider it ?

Stephen Boyd March 5, 2025, 10:31 p.m. UTC | #9

Quoting Viresh Kumar (2025-03-05 03:46:59)
> On 04-03-25, 10:37, Miguel Ojeda wrote:
> > On Tue, Mar 4, 2025 at 9:53 AM Viresh Kumar <[e-mail]> wrote:
> > > +///     clk.disable_unprepare();
> > 
> > Looking at the example, a question that one may have is: should we
> > have something like a scope guard or a closure-passing API for this,
> > or does it not make sense in general?
> 
> Something like this (untested) ?
> 
> +/// Runs a cleanup function/closure when dropped.
> +///
> +/// The [`ClkGuard::dismiss`] function prevents the cleanup function from running.
> +///
> +pub type ClkGuard<'a> = ScopeGuard<&'a Clk, fn(&Clk)>;
> +
>  /// A reference-counted clock.
>  ///
>  /// This represents the Rust abstraction for the C [`struct clk`].
> @@ -139,10 +146,12 @@ pub fn as_raw(&self) -> *mut bindings::clk {
>      ///
>      /// [`clk_enable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable
>      #[inline]
> -    pub fn enable(&self) -> Result {
> +    pub fn enable(&self) -> Result<ClkGuard<'_>> {
>          // SAFETY: By the type invariants, it is safe to call clk APIs of the C code for a clock
>          // pointer earlier returned by [`clk_get`].
> -        to_result(unsafe { bindings::clk_enable(self.as_raw()) })
> +        to_result(unsafe { bindings::clk_enable(self.as_raw()) })?;
> +
> +        Ok(ClkGuard::new_with_data(self, Clk::disable))

Does this mean that a clk consumer has to keep the Result returned from
enable() in scope until they want to disable the clk? I don't see how
that makes sense, because most of the time a consumer will enable a clk
during probe and leave it enabled until system suspend or runtime PM
suspend time. At that point, they would disable the clk explicitly with
disable(), but now they would need to drop a reference to do that?

Viresh Kumar March 6, 2025, 4:40 a.m. UTC | #10

On 05-03-25, 14:31, Stephen Boyd wrote:
> Does this mean that a clk consumer has to keep the Result returned from
> enable() in scope until they want to disable the clk?

Yes and no.

> I don't see how
> that makes sense, because most of the time a consumer will enable a clk
> during probe and leave it enabled until system suspend or runtime PM
> suspend time. At that point, they would disable the clk explicitly with
> disable(), but now they would need to drop a reference to do that?

Broadly there are two type of clk users I believe:

1. clk is enabled / disabled from same routine:

   In this case the result can be kept in a local variable and the matching
   cleanup fn will be called at exit.

   fn transfer_data(...) -> Result {
        let _guard = clk.enable()?;

        ...
        transfer-data here
        ...
        // clk.disable() will be called automatically as soon as _guard goes out
        // of scope.
   }

2. clk is enabled / disabled from different routines:

   In this case the caller needs to call dismiss to avoid the automatic freeing
   of resource. Alternatively the returned value can be stored too somewhere,
   but I am not sure if it what users will end up doing.

   fn probe(...) -> Result {
        clk.enable()?.dismiss();

        ...
   }

   fn remove (...) {
        clk.disable();

        ...
   }

Danilo Krummrich March 6, 2025, 12:33 p.m. UTC | #11

On Tue, Mar 04, 2025 at 02:23:51PM +0530, Viresh Kumar wrote:
> +/// This structure represents the Rust abstraction for a C [`struct clk`].
> +///
> +/// # Invariants
> +///
> +/// A [`Clk`] instance always corresponds to a valid [`struct clk`] created by the C portion of the
> +/// kernel.
> +///
> +/// Instances of this type are reference-counted. Calling `get` ensures that the allocation remains
> +/// valid for the lifetime of the [`Clk`].
> +///
> +/// ## Example
> +///
> +/// The following example demonstrates how to obtain and configure a clock for a device.
> +///
> +/// ```
> +/// use kernel::clk::{Clk, Hertz};
> +/// use kernel::device::Device;
> +/// use kernel::error::Result;
> +///
> +/// fn configure_clk(dev: &Device) -> Result {
> +///     let clk = Clk::get(dev, "apb_clk")?;
> +///
> +///     clk.prepare_enable()?;
> +///
> +///     let expected_rate = Hertz::new(1_000_000_000);
> +///
> +///     if clk.rate() != expected_rate {
> +///         clk.set_rate(expected_rate)?;
> +///     }
> +///
> +///     clk.disable_unprepare();
> +///     Ok(())
> +/// }
> +/// ```
> +///
> +/// [`struct clk`]: https://docs.kernel.org/driver-api/clk.html
> +#[repr(transparent)]
> +pub struct Clk(*mut bindings::clk);
> +
> +impl Clk {
> +    /// Gets `Clk` corresponding to a [`Device`] and a connection id.
> +    pub fn get(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()` for a valid device pointer.
> +        Ok(Self(from_err_ptr(unsafe {
> +            bindings::clk_get(dev.as_raw(), con_id)
> +        })?))
> +    }
> +
> +    /// Obtain the raw `struct clk *`.
> +    #[inline]
> +    pub fn as_raw(&self) -> *mut bindings::clk {
> +        self.0
> +    }
> +
> +    /// Enable the clock.
> +    #[inline]
> +    pub fn enable(&self) -> Result {
> +        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
> +        // by `clk_get()`.

You may want to add an invariant for this, i.e. something along the lines of
"Clk always holds either a pointer to a valid struct clk or a NULL pointer".

In this safety comment you can then say that by the type invariant of Clk
self.as_raw() is a valid argument for $func.

Not that your type invariant needs the NULL case, since OptionalClk may set Clk
to hold a NULL pointer.

I still think that a new type MaybeNull<T> would be nice to encapsulate this
invariant, but we can also wait until we get another use-case for it.

Stephen Boyd March 6, 2025, 8:58 p.m. UTC | #12

Quoting Viresh Kumar (2025-03-05 20:40:28)
> On 05-03-25, 14:31, Stephen Boyd wrote:
> > Does this mean that a clk consumer has to keep the Result returned from
> > enable() in scope until they want to disable the clk?
> 
> Yes and no.
> 
> > I don't see how
> > that makes sense, because most of the time a consumer will enable a clk
> > during probe and leave it enabled until system suspend or runtime PM
> > suspend time. At that point, they would disable the clk explicitly with
> > disable(), but now they would need to drop a reference to do that?
> 
> Broadly there are two type of clk users I believe:
> 
> 1. clk is enabled / disabled from same routine:
> 
>    In this case the result can be kept in a local variable and the matching
>    cleanup fn will be called at exit.

This is almost never the case. Listing these as two types of clk users
tries to make the two equal, when the vast majority of users are the
second. Please don't.

> 
>    fn transfer_data(...) -> Result {
>         let _guard = clk.enable()?;
> 
>         ...
>         transfer-data here
>         ...
>         // clk.disable() will be called automatically as soon as _guard goes out
>         // of scope.
>    }
> 
> 2. clk is enabled / disabled from different routines:
> 
>    In this case the caller needs to call dismiss to avoid the automatic freeing
>    of resource. Alternatively the returned value can be stored too somewhere,
>    but I am not sure if it what users will end up doing.
> 
>    fn probe(...) -> Result {
>         clk.enable()?.dismiss();

Yuck. Can't we tie the lifetime of the clk to the consumer device driver
so that when the driver is unbound the clk is dropped and it decrements
all the enables/prepares and puts the clk with clk_put()? A ScopeGuard
could probably be used for that on the struct Clk itself, but we would
want to track the enables and prepares in the rust wrapper code until
the struct clk can be inspected directly.

The problem is we don't know how a platform may implement the clk API,
and CCF hasn't taken over the entire kernel yet so we can't rely on some
private API between the CCF and the rust wrapper to know how many
clk_disable()s to call, or even rely on clk_put() to do the work for us.
Can the rust wrappers depend on CONFIG_COMMON_CLK? If they did then we
could have some private API between rust and CCF. We probably don't want
rust code to _not_ use COMMON_CLK underneath so we can encourage the
last few holdouts to migrate to CCF. I'd lean towards depending on
COMMON_CLK for the rust wrappers in this case.

Viresh Kumar March 7, 2025, 7:23 a.m. UTC | #13

On 06-03-25, 12:58, Stephen Boyd wrote:
> Quoting Viresh Kumar (2025-03-05 20:40:28)
> > 2. clk is enabled / disabled from different routines:
> > 
> >    In this case the caller needs to call dismiss to avoid the automatic freeing
> >    of resource. Alternatively the returned value can be stored too somewhere,
> >    but I am not sure if it what users will end up doing.
> > 
> >    fn probe(...) -> Result {
> >         clk.enable()?.dismiss();
> 
> Yuck. Can't we tie the lifetime of the clk to the consumer device driver
> so that when the driver is unbound the clk is dropped

Yes, that is how it would work right now, the driver needs to store the clk
instance locally. As soon as Clk would be dropped, clk_put() will be called.

> and it decrements all the enables/prepares and puts the clk with clk_put()?

Miguel, how do you suggest we do this ?

> A ScopeGuard could probably be used for that on the struct Clk itself,

Not sure if I misunderstood that, but as soon as Clk goes out of scope,
clk_put() will be called from drop() anyway.

> but we would
> want to track the enables and prepares in the rust wrapper code until
> the struct clk can be inspected directly.

So Rust abstraction needs to do some sort of refcounting for this I believe. Not
sure if we want to do it and if yes, then how.

> The problem is we don't know how a platform may implement the clk API,
> and CCF hasn't taken over the entire kernel yet so we can't rely on some
> private API between the CCF and the rust wrapper to know how many
> clk_disable()s to call, or even rely on clk_put() to do the work for us.
> Can the rust wrappers depend on CONFIG_COMMON_CLK? If they did then we
> could have some private API between rust and CCF. We probably don't want
> rust code to _not_ use COMMON_CLK underneath so we can encourage the
> last few holdouts to migrate to CCF. I'd lean towards depending on
> COMMON_CLK for the rust wrappers in this case.

Sure.

Daniel Almeida March 8, 2025, 2:03 a.m. UTC | #14

Hi Viresh,

> On 4 Mar 2025, at 05:53, Viresh Kumar <[e-mail]> wrote:
> 
> On 03-03-25, 11:16, Miguel Ojeda wrote:
>> On Mon, Mar 3, 2025 at 11:00 AM Viresh Kumar <[e-mail]> wrote:
>>> 
>>> +/// Frequency unit.
>>> +pub type Hertz = crate::ffi::c_ulong;
>> 
>> Do we want this to be an alias or would it make sense to take the
>> chance to make this a newtype?
> 
> I have tried some improvements based on your (and Alice's comments), please see
> if it looks any better now.
> 
> -- 
> viresh
> 
> -------------------------8<-------------------------
> 
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> new file mode 100644
> index 000000000000..fc3cb0f5f332
> --- /dev/null
> +++ b/rust/kernel/clk.rs
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Clock abstractions.
> +//!
> +//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h)
> +//!
> +//! Reference: <https://docs.kernel.org/driver-api/clk.html>
> +
> +use crate::{
> +    bindings,
> +    device::Device,
> +    error::{from_err_ptr, to_result, Result},
> +    ffi::c_ulong,
> +    prelude::*,
> +};
> +
> +use core::{ops::Deref, ptr};
> +
> +/// Frequency unit.
> +#[derive(Copy, Clone, PartialEq, Eq, Debug)]
> +pub struct Hertz(c_ulong);

Maybe make self.0 pub too?

> +
> +impl Hertz {
> +    /// Creates a new `Hertz` value.
> +    pub fn new(freq: c_ulong) -> Self {
> +        Hertz(freq)

I don’t think we need a `new` function.  IMHO, the only thing that matters is
that the name Hertz shows up in the calling code, i.e.:

```
fn foo() {
    let clk = …;
    let some_val = …;
    clk.set_rate(Hertz(some_val)); // Ok: crystal clear this is Hertz
}
```

A  impl From<Hertz> for c_ulong would also be helpful, so that we don’t have to
manually define all the arithmetic operations on this.

```
fn foo() {
    let clk = …;
    let double = u32::from(clk.rate()) * 2;
    clk.set_rate(Hertz(double)); // Ok: crystal clear this is Hertz
}
```

I need more time to look at the rest of the patch, so feel free to carry on with the
feedback from others. Sorry for the delay!

— Daniel

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 726110d3c988..96e2574f41c0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5779,6 +5779,7 @@  F:	include/linux/clk-pr*
 F:	include/linux/clk/
 F:	include/linux/of_clk.h
 F:	rust/helpers/clk.c
+F:	rust/kernel/clk.rs
 X:	drivers/clk/clkdev.c
 
 COMMON INTERNET FILE SYSTEM CLIENT (CIFS and SMB3)
diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
new file mode 100644
index 000000000000..1fa5b7298373
--- /dev/null
+++ b/rust/kernel/clk.rs
@@ -0,0 +1,134 @@ 
+// 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, to_result, Result},
+    prelude::*,
+};
+
+use core::{ops::Deref, ptr};
+
+/// Frequency unit.
+pub type Hertz = crate::ffi::c_ulong;
+
+/// A simple implementation of `struct clk` from the C code.
+#[repr(transparent)]
+pub struct Clk(*mut bindings::clk);
+
+impl Clk {
+    /// Gets clock corresponding to a device and a connection id and returns `Clk`.
+    pub fn get(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()` for a valid device pointer.
+        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
+    }
+
+    /// Clock enable.
+    pub fn enable(&self) -> Result<()> {
+        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
+        // by `clk_get()`.
+        to_result(unsafe { bindings::clk_enable(self.as_raw()) })
+    }
+
+    /// Clock disable.
+    pub fn disable(&self) {
+        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
+        // by `clk_get()`.
+        unsafe { bindings::clk_disable(self.as_raw()) };
+    }
+
+    /// Clock prepare.
+    pub fn prepare(&self) -> Result<()> {
+        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
+        // by `clk_get()`.
+        to_result(unsafe { bindings::clk_prepare(self.as_raw()) })
+    }
+
+    /// Clock unprepare.
+    pub fn unprepare(&self) {
+        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
+        // by `clk_get()`.
+        unsafe { bindings::clk_unprepare(self.as_raw()) };
+    }
+
+    /// Clock prepare enable.
+    pub fn prepare_enable(&self) -> Result<()> {
+        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
+        // by `clk_get()`.
+        to_result(unsafe { bindings::clk_prepare_enable(self.as_raw()) })
+    }
+
+    /// Clock disable unprepare.
+    pub fn disable_unprepare(&self) {
+        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
+        // by `clk_get()`.
+        unsafe { bindings::clk_disable_unprepare(self.as_raw()) };
+    }
+
+    /// Clock get rate.
+    pub fn rate(&self) -> Hertz {
+        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
+        // by `clk_get()`.
+        unsafe { bindings::clk_get_rate(self.as_raw()) }
+    }
+
+    /// Clock set rate.
+    pub fn set_rate(&self, rate: Hertz) -> Result<()> {
+        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
+        // by `clk_get()`.
+        to_result(unsafe { bindings::clk_set_rate(self.as_raw(), rate) })
+    }
+}
+
+impl Drop for Clk {
+    fn drop(&mut self) {
+        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
+        // by `clk_get()`.
+        unsafe { bindings::clk_put(self.as_raw()) };
+    }
+}
+
+/// A simple implementation of optional `Clk`.
+pub struct OptionalClk(Clk);
+
+impl OptionalClk {
+    /// Gets optional clock corresponding to a device and a connection id and returns `Clk`.
+    pub fn get(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_optional()` for a valid device pointer.
+        Ok(Self(Clk(from_err_ptr(unsafe {
+            bindings::clk_get_optional(dev.as_raw(), con_id)
+        })?)))
+    }
+}
+
+// Make `OptionalClk` behave like `Clk`.
+impl Deref for OptionalClk {
+    type Target = Clk;
+
+    fn deref(&self) -> &Clk {
+        &self.0
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0911..324b86f127a0 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -40,6 +40,7 @@ 
 pub mod block;
 #[doc(hidden)]
 pub mod build_assert;
+pub mod clk;
 pub mod cred;
 pub mod device;
 pub mod device_id;