[v6] Introduce bitfield and move register macro to rust/kernel/

[PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Joel Fernandes 2 months, 2 weeks ago

Out of broad need for the register and bitfield macros in Rust, move
them out of nova into the kernel crate. Several usecases need them (Nova
is already using these and Tyr developers said they need them).

bitfield moved into kernel crate - defines bitfields in Rust.
register moved into io module - defines hardware registers and accessors.

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
 drivers/gpu/nova-core/falcon.rs               |  2 +-
 drivers/gpu/nova-core/falcon/gsp.rs           |  4 +-
 drivers/gpu/nova-core/falcon/sec2.rs          |  2 +-
 drivers/gpu/nova-core/nova_core.rs            |  3 -
 drivers/gpu/nova-core/regs.rs                 |  6 +-
 .../gpu/nova-core => rust/kernel}/bitfield.rs | 27 ++++-----
 rust/kernel/io.rs                             |  1 +
 .../macros.rs => rust/kernel/io/register.rs   | 58 ++++++++++---------
 rust/kernel/lib.rs                            |  1 +
 9 files changed, 54 insertions(+), 50 deletions(-)
 rename {drivers/gpu/nova-core => rust/kernel}/bitfield.rs (91%)
 rename drivers/gpu/nova-core/regs/macros.rs => rust/kernel/io/register.rs (93%)

diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 37e6298195e4..a15fa98c8614 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -6,6 +6,7 @@
 use hal::FalconHal;
 use kernel::device;
 use kernel::dma::DmaAddress;
+use kernel::io::register::RegisterBase;
 use kernel::prelude::*;
 use kernel::sync::aref::ARef;
 use kernel::time::Delta;
@@ -14,7 +15,6 @@
 use crate::driver::Bar0;
 use crate::gpu::Chipset;
 use crate::regs;
-use crate::regs::macros::RegisterBase;
 use crate::util;
 
 pub(crate) mod gsp;
diff --git a/drivers/gpu/nova-core/falcon/gsp.rs b/drivers/gpu/nova-core/falcon/gsp.rs
index f17599cb49fa..cd4960e997c8 100644
--- a/drivers/gpu/nova-core/falcon/gsp.rs
+++ b/drivers/gpu/nova-core/falcon/gsp.rs
@@ -1,9 +1,11 @@
 // SPDX-License-Identifier: GPL-2.0
 
+use kernel::io::register::RegisterBase;
+
 use crate::{
     driver::Bar0,
     falcon::{Falcon, FalconEngine, PFalcon2Base, PFalconBase},
-    regs::{self, macros::RegisterBase},
+    regs::self,
 };
 
 /// Type specifying the `Gsp` falcon engine. Cannot be instantiated.
diff --git a/drivers/gpu/nova-core/falcon/sec2.rs b/drivers/gpu/nova-core/falcon/sec2.rs
index 815786c8480d..81717868a8a8 100644
--- a/drivers/gpu/nova-core/falcon/sec2.rs
+++ b/drivers/gpu/nova-core/falcon/sec2.rs
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 use crate::falcon::{FalconEngine, PFalcon2Base, PFalconBase};
-use crate::regs::macros::RegisterBase;
+use kernel::io::register::RegisterBase;
 
 /// Type specifying the `Sec2` falcon engine. Cannot be instantiated.
 pub(crate) struct Sec2(());
diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
index 112277c7921e..fffcaee2249f 100644
--- a/drivers/gpu/nova-core/nova_core.rs
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -2,9 +2,6 @@
 
 //! Nova Core GPU Driver
 
-#[macro_use]
-mod bitfield;
-
 mod dma;
 mod driver;
 mod falcon;
diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
index 206dab2e1335..1f08e6d4045a 100644
--- a/drivers/gpu/nova-core/regs.rs
+++ b/drivers/gpu/nova-core/regs.rs
@@ -4,15 +4,13 @@
 // but are mapped to types.
 #![allow(non_camel_case_types)]
 
-#[macro_use]
-pub(crate) mod macros;
-
 use crate::falcon::{
     DmaTrfCmdSize, FalconCoreRev, FalconCoreRevSubversion, FalconFbifMemType, FalconFbifTarget,
     FalconModSelAlgo, FalconSecurityModel, PFalcon2Base, PFalconBase, PeregrineCoreSelect,
 };
 use crate::gpu::{Architecture, Chipset};
 use kernel::prelude::*;
+use kernel::register;
 
 // PMC
 
@@ -331,6 +329,7 @@ pub(crate) fn mem_scrubbing_done(self) -> bool {
 
 pub(crate) mod gm107 {
     // FUSE
+    use kernel::register;
 
     register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00021c04 {
         0:0     display_disabled as bool;
@@ -339,6 +338,7 @@ pub(crate) mod gm107 {
 
 pub(crate) mod ga100 {
     // FUSE
+    use kernel::register;
 
     register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00820c04 {
         0:0     display_disabled as bool;
diff --git a/drivers/gpu/nova-core/bitfield.rs b/rust/kernel/bitfield.rs
similarity index 91%
rename from drivers/gpu/nova-core/bitfield.rs
rename to rust/kernel/bitfield.rs
index cbedbb0078f6..09cd5741598c 100644
--- a/drivers/gpu/nova-core/bitfield.rs
+++ b/rust/kernel/bitfield.rs
@@ -9,7 +9,7 @@
 /// # Syntax
 ///
 /// ```rust
-/// use nova_core::bitfield;
+/// use kernel::bitfield;
 ///
 /// #[derive(Debug, Clone, Copy, Default)]
 /// enum Mode {
@@ -82,10 +82,11 @@
 ///   the result.
 /// - `as <type> ?=> <try_into_type>` calls `<try_into_type>`'s `TryFrom::<<type>>` implementation
 ///   and returns the result. This is useful with fields for which not all values are valid.
+#[macro_export]
 macro_rules! bitfield {
     // Main entry point - defines the bitfield struct with fields
     ($vis:vis struct $name:ident($storage:ty) $(, $comment:literal)? { $($fields:tt)* }) => {
-        bitfield!(@core $vis $name $storage $(, $comment)? { $($fields)* });
+        ::kernel::bitfield!(@core $vis $name $storage $(, $comment)? { $($fields)* });
     };
 
     // All rules below are helpers.
@@ -114,7 +115,7 @@ fn from(val: $name) -> $storage {
             }
         }
 
-        bitfield!(@fields_dispatcher $vis $name $storage { $($fields)* });
+        ::kernel::bitfield!(@fields_dispatcher $vis $name $storage { $($fields)* });
     };
 
     // Captures the fields and passes them to all the implementers that require field information.
@@ -130,7 +131,7 @@ fn from(val: $name) -> $storage {
         )*
     }
     ) => {
-        bitfield!(@field_accessors $vis $name $storage {
+        ::kernel::bitfield!(@field_accessors $vis $name $storage {
             $(
                 $hi:$lo $field as $type
                 $(?=> $try_into_type)?
@@ -139,8 +140,8 @@ fn from(val: $name) -> $storage {
             ;
             )*
         });
-        bitfield!(@debug $name { $($field;)* });
-        bitfield!(@default $name { $($field;)* });
+        ::kernel::bitfield!(@debug $name { $($field;)* });
+        ::kernel::bitfield!(@default $name { $($field;)* });
     };
 
     // Defines all the field getter/setter methods for `$name`.
@@ -155,13 +156,13 @@ fn from(val: $name) -> $storage {
         }
     ) => {
         $(
-            bitfield!(@check_field_bounds $hi:$lo $field as $type);
+            ::kernel::bitfield!(@check_field_bounds $hi:$lo $field as $type);
         )*
 
         #[allow(dead_code)]
         impl $name {
             $(
-            bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type
+            ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type
                 $(?=> $try_into_type)?
                 $(=> $into_type)?
                 $(, $comment)?
@@ -198,7 +199,7 @@ impl $name {
         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
             $(, $comment:literal)?;
     ) => {
-        bitfield!(
+        ::kernel::bitfield!(
             @leaf_accessor $vis $name $storage, $hi:$lo $field
             { |f| <$into_type>::from(if f != 0 { true } else { false }) }
             $into_type => $into_type $(, $comment)?;
@@ -209,7 +210,7 @@ impl $name {
     (
         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
     ) => {
-        bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
+        ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
     };
 
     // Catches the `?=>` syntax for non-boolean fields.
@@ -217,7 +218,7 @@ impl $name {
         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
             $(, $comment:literal)?;
     ) => {
-        bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
+        ::kernel::bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
             { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
             ::core::result::Result<
                 $try_into_type,
@@ -231,7 +232,7 @@ impl $name {
         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
             $(, $comment:literal)?;
     ) => {
-        bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
+        ::kernel::bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
             { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
     };
 
@@ -240,7 +241,7 @@ impl $name {
         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt
             $(, $comment:literal)?;
     ) => {
-        bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
+        ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
     };
 
     // Generates the accessor methods for a single field.
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 03b467722b86..a79b603604b1 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -8,6 +8,7 @@
 use crate::{bindings, build_assert, ffi::c_void};
 
 pub mod mem;
+pub mod register;
 pub mod resource;
 
 pub use resource::Resource;
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/rust/kernel/io/register.rs
similarity index 93%
rename from drivers/gpu/nova-core/regs/macros.rs
rename to rust/kernel/io/register.rs
index c0a5194e8d97..c24d956f122f 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/rust/kernel/io/register.rs
@@ -17,7 +17,8 @@
 /// The `T` generic argument is used to distinguish which base to use, in case a type provides
 /// several bases. It is given to the `register!` macro to restrict the use of the register to
 /// implementors of this particular variant.
-pub(crate) trait RegisterBase<T> {
+pub trait RegisterBase<T> {
+    /// The base address for the register.
     const BASE: usize;
 }
 
@@ -26,7 +27,7 @@ pub(crate) trait RegisterBase<T> {
 ///
 /// Example:
 ///
-/// ```no_run
+/// ```ignore
 /// register!(BOOT_0 @ 0x00000100, "Basic revision information about the GPU" {
 ///    3:0     minor_revision as u8, "Minor revision of the chip";
 ///    7:4     major_revision as u8, "Major revision of the chip";
@@ -39,7 +40,7 @@ pub(crate) trait RegisterBase<T> {
 /// significant bits of the register. Each field can be accessed and modified using accessor
 /// methods:
 ///
-/// ```no_run
+/// ```ignore
 /// // Read from the register's defined offset (0x100).
 /// let boot0 = BOOT_0::read(&bar);
 /// pr_info!("chip revision: {}.{}", boot0.major_revision(), boot0.minor_revision());
@@ -61,7 +62,7 @@ pub(crate) trait RegisterBase<T> {
 /// It is also possible to create a alias register by using the `=> ALIAS` syntax. This is useful
 /// for cases where a register's interpretation depends on the context:
 ///
-/// ```no_run
+/// ```ignore
 /// register!(SCRATCH @ 0x00000200, "Scratch register" {
 ///    31:0     value as u32, "Raw value";
 /// });
@@ -111,7 +112,7 @@ pub(crate) trait RegisterBase<T> {
 /// this register needs to implement `RegisterBase<Base>`. Here is the above example translated
 /// into code:
 ///
-/// ```no_run
+/// ```ignore
 /// // Type used to identify the base.
 /// pub(crate) struct CpuCtlBase;
 ///
@@ -162,7 +163,7 @@ pub(crate) trait RegisterBase<T> {
 /// compile-time or runtime bound checking. Simply define their address as `Address[Size]`, and add
 /// an `idx` parameter to their `read`, `write` and `alter` methods:
 ///
-/// ```no_run
+/// ```ignore
 /// # fn no_run() -> Result<(), Error> {
 /// # fn get_scratch_idx() -> usize {
 /// #   0x15
@@ -211,7 +212,7 @@ pub(crate) trait RegisterBase<T> {
 /// Combining the two features described in the sections above, arrays of registers accessible from
 /// a base can also be defined:
 ///
-/// ```no_run
+/// ```ignore
 /// # fn no_run() -> Result<(), Error> {
 /// # fn get_scratch_idx() -> usize {
 /// #   0x15
@@ -273,28 +274,29 @@ pub(crate) trait RegisterBase<T> {
 /// # Ok(())
 /// # }
 /// ```
+#[macro_export]
 macro_rules! register {
     // Creates a register at a fixed offset of the MMIO space.
     ($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* } ) => {
-        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
+        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_fixed $name @ $offset);
     };
 
     // Creates an alias register of fixed offset register `alias` with its own fields.
     ($name:ident => $alias:ident $(, $comment:literal)? { $($fields:tt)* } ) => {
-        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
+        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_fixed $name @ $alias::OFFSET);
     };
 
     // Creates a register at a relative offset from a base address provider.
     ($name:ident @ $base:ty [ $offset:literal ] $(, $comment:literal)? { $($fields:tt)* } ) => {
-        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
+        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_relative $name @ $base [ $offset ]);
     };
 
     // Creates an alias register of relative offset register `alias` with its own fields.
     ($name:ident => $base:ty [ $alias:ident ] $(, $comment:literal)? { $($fields:tt)* }) => {
-        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
+        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_relative $name @ $base [ $alias::OFFSET ]);
     };
 
@@ -305,7 +307,7 @@ macro_rules! register {
         }
     ) => {
         static_assert!(::core::mem::size_of::<u32>() <= $stride);
-        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
+        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_array $name @ $offset [ $size ; $stride ]);
     };
 
@@ -326,7 +328,7 @@ macro_rules! register {
             $(, $comment:literal)? { $($fields:tt)* }
     ) => {
         static_assert!(::core::mem::size_of::<u32>() <= $stride);
-        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
+        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_relative_array $name @ $base [ $offset [ $size ; $stride ] ]);
     };
 
@@ -348,7 +350,7 @@ macro_rules! register {
         }
     ) => {
         static_assert!($idx < $alias::SIZE);
-        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
+        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_relative $name @ $base [ $alias::OFFSET + $idx * $alias::STRIDE ] );
     };
 
@@ -357,7 +359,7 @@ macro_rules! register {
     // to avoid it being interpreted in place of the relative register array alias rule.
     ($name:ident => $alias:ident [ $idx:expr ] $(, $comment:literal)? { $($fields:tt)* }) => {
         static_assert!($idx < $alias::SIZE);
-        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
+        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );
     };
 
@@ -414,12 +416,12 @@ pub(crate) fn read<const SIZE: usize, T, B>(
                 base: &B,
             ) -> Self where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
-                B: crate::regs::macros::RegisterBase<$base>,
+                B: ::kernel::io::register::RegisterBase<$base>,
             {
                 const OFFSET: usize = $name::OFFSET;
 
                 let value = io.read32(
-                    <B as crate::regs::macros::RegisterBase<$base>>::BASE + OFFSET
+                    <B as ::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
                 );
 
                 Self(value)
@@ -435,13 +437,13 @@ pub(crate) fn write<const SIZE: usize, T, B>(
                 base: &B,
             ) where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
-                B: crate::regs::macros::RegisterBase<$base>,
+                B: ::kernel::io::register::RegisterBase<$base>,
             {
                 const OFFSET: usize = $name::OFFSET;
 
                 io.write32(
                     self.0,
-                    <B as crate::regs::macros::RegisterBase<$base>>::BASE + OFFSET
+                    <B as ::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
                 );
             }
 
@@ -455,7 +457,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
                 f: F,
             ) where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
-                B: crate::regs::macros::RegisterBase<$base>,
+                B: ::kernel::io::register::RegisterBase<$base>,
                 F: ::core::ops::FnOnce(Self) -> Self,
             {
                 let reg = f(Self::read(io, base));
@@ -600,11 +602,11 @@ pub(crate) fn read<const SIZE: usize, T, B>(
                 idx: usize,
             ) -> Self where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
-                B: crate::regs::macros::RegisterBase<$base>,
+                B: ::kernel::io::register::RegisterBase<$base>,
             {
                 build_assert!(idx < Self::SIZE);
 
-                let offset = <B as crate::regs::macros::RegisterBase<$base>>::BASE +
+                let offset = <B as ::kernel::io::register::RegisterBase<$base>>::BASE +
                     Self::OFFSET + (idx * Self::STRIDE);
                 let value = io.read32(offset);
 
@@ -622,11 +624,11 @@ pub(crate) fn write<const SIZE: usize, T, B>(
                 idx: usize
             ) where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
-                B: crate::regs::macros::RegisterBase<$base>,
+                B: ::kernel::io::register::RegisterBase<$base>,
             {
                 build_assert!(idx < Self::SIZE);
 
-                let offset = <B as crate::regs::macros::RegisterBase<$base>>::BASE +
+                let offset = <B as ::kernel::io::register::RegisterBase<$base>>::BASE +
                     Self::OFFSET + (idx * Self::STRIDE);
 
                 io.write32(self.0, offset);
@@ -643,7 +645,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
                 f: F,
             ) where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
-                B: crate::regs::macros::RegisterBase<$base>,
+                B: ::kernel::io::register::RegisterBase<$base>,
                 F: ::core::ops::FnOnce(Self) -> Self,
             {
                 let reg = f(Self::read(io, base, idx));
@@ -662,7 +664,7 @@ pub(crate) fn try_read<const SIZE: usize, T, B>(
                 idx: usize,
             ) -> ::kernel::error::Result<Self> where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
-                B: crate::regs::macros::RegisterBase<$base>,
+                B: ::kernel::io::register::RegisterBase<$base>,
             {
                 if idx < Self::SIZE {
                     Ok(Self::read(io, base, idx))
@@ -684,7 +686,7 @@ pub(crate) fn try_write<const SIZE: usize, T, B>(
                 idx: usize,
             ) -> ::kernel::error::Result where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
-                B: crate::regs::macros::RegisterBase<$base>,
+                B: ::kernel::io::register::RegisterBase<$base>,
             {
                 if idx < Self::SIZE {
                     Ok(self.write(io, base, idx))
@@ -707,7 +709,7 @@ pub(crate) fn try_alter<const SIZE: usize, T, B, F>(
                 f: F,
             ) -> ::kernel::error::Result where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
-                B: crate::regs::macros::RegisterBase<$base>,
+                B: ::kernel::io::register::RegisterBase<$base>,
                 F: ::core::ops::FnOnce(Self) -> Self,
             {
                 if idx < Self::SIZE {
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index fcffc3988a90..8f8260090c02 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -63,6 +63,7 @@
 pub mod alloc;
 #[cfg(CONFIG_AUXILIARY_BUS)]
 pub mod auxiliary;
+pub mod bitfield;
 pub mod bits;
 #[cfg(CONFIG_BLOCK)]
 pub mod block;
-- 
2.34.1

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Beata Michalska 1 month, 3 weeks ago

Hi Joel,

I know I'm chiming in a bit late, so apologies for that.

The register! macro does seem to be a solid foundation for MMIO register
definitions, thought there are few points that could be potentially
[re]considered.

The current design assumes a fixed, compile-time-known MMIO region size.
It does not cover cases when the region size is known only at runtime.
I do appreciate that in cases like that, we are loosing all the deliberate
compile-time checks but it might be necessary to provide support for those as
well (at some point at least).

On the (potential) improvement side:

Allowing offsets to be expressions rather than literals would make the macro
easier to use for regions defined at a fixed base offset, where subsequent
offsets are derived from that base, i.e:

REG_1_BASE 	-> 0x100
REG_1_STATUS 	-> REG_1_BASE + 0x0
REG_1_CONTROL	-> REG_1_BASE + 0x04
...

The alias mechanism is a nice touch. It might be worth allowing arrays of
registers with explicit aliases to be defined in a single macro invocation,
instead of repeating similar definitions, smth along the lines of:

  register!(
      REG_STATUS @ 0x300[8; STRIDE] {
          0:0 enabled as bool;
          3:1 mode as u8;
          7:4 flags as u8;
      }
      aliases {
          REG_STATUS_ENABLED[0] {
              0:0 enabled as bool;
          }
          REG_STATUS_MODE[0] {
              3:1 mode as u8;
          }
          REG_STATUS_FLAGS[4] {
              7:4 flags as u8;
          }
      }
  );


Finally, for runtime values such as indexes, it could be useful to verify once
and then allow infallible reads/writes through some kind access token.
That might make runtime-safe access patterns simpler and more efficient.
I'm still pondering on how that could look like though (implementation-wise)

---
BR
Beata

On Fri, Oct 03, 2025 at 11:47:47AM -0400, Joel Fernandes wrote:
> Out of broad need for the register and bitfield macros in Rust, move
> them out of nova into the kernel crate. Several usecases need them (Nova
> is already using these and Tyr developers said they need them).
> 
> bitfield moved into kernel crate - defines bitfields in Rust.
> register moved into io module - defines hardware registers and accessors.
> 
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> ---
>  drivers/gpu/nova-core/falcon.rs               |  2 +-
>  drivers/gpu/nova-core/falcon/gsp.rs           |  4 +-
>  drivers/gpu/nova-core/falcon/sec2.rs          |  2 +-
>  drivers/gpu/nova-core/nova_core.rs            |  3 -
>  drivers/gpu/nova-core/regs.rs                 |  6 +-
>  .../gpu/nova-core => rust/kernel}/bitfield.rs | 27 ++++-----
>  rust/kernel/io.rs                             |  1 +
>  .../macros.rs => rust/kernel/io/register.rs   | 58 ++++++++++---------
>  rust/kernel/lib.rs                            |  1 +
>  9 files changed, 54 insertions(+), 50 deletions(-)
>  rename {drivers/gpu/nova-core => rust/kernel}/bitfield.rs (91%)
>  rename drivers/gpu/nova-core/regs/macros.rs => rust/kernel/io/register.rs (93%)
> 
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index 37e6298195e4..a15fa98c8614 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -6,6 +6,7 @@
>  use hal::FalconHal;
>  use kernel::device;
>  use kernel::dma::DmaAddress;
> +use kernel::io::register::RegisterBase;
>  use kernel::prelude::*;
>  use kernel::sync::aref::ARef;
>  use kernel::time::Delta;
> @@ -14,7 +15,6 @@
>  use crate::driver::Bar0;
>  use crate::gpu::Chipset;
>  use crate::regs;
> -use crate::regs::macros::RegisterBase;
>  use crate::util;
>  
>  pub(crate) mod gsp;
> diff --git a/drivers/gpu/nova-core/falcon/gsp.rs b/drivers/gpu/nova-core/falcon/gsp.rs
> index f17599cb49fa..cd4960e997c8 100644
> --- a/drivers/gpu/nova-core/falcon/gsp.rs
> +++ b/drivers/gpu/nova-core/falcon/gsp.rs
> @@ -1,9 +1,11 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> +use kernel::io::register::RegisterBase;
> +
>  use crate::{
>      driver::Bar0,
>      falcon::{Falcon, FalconEngine, PFalcon2Base, PFalconBase},
> -    regs::{self, macros::RegisterBase},
> +    regs::self,
>  };
>  
>  /// Type specifying the `Gsp` falcon engine. Cannot be instantiated.
> diff --git a/drivers/gpu/nova-core/falcon/sec2.rs b/drivers/gpu/nova-core/falcon/sec2.rs
> index 815786c8480d..81717868a8a8 100644
> --- a/drivers/gpu/nova-core/falcon/sec2.rs
> +++ b/drivers/gpu/nova-core/falcon/sec2.rs
> @@ -1,7 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
>  use crate::falcon::{FalconEngine, PFalcon2Base, PFalconBase};
> -use crate::regs::macros::RegisterBase;
> +use kernel::io::register::RegisterBase;
>  
>  /// Type specifying the `Sec2` falcon engine. Cannot be instantiated.
>  pub(crate) struct Sec2(());
> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
> index 112277c7921e..fffcaee2249f 100644
> --- a/drivers/gpu/nova-core/nova_core.rs
> +++ b/drivers/gpu/nova-core/nova_core.rs
> @@ -2,9 +2,6 @@
>  
>  //! Nova Core GPU Driver
>  
> -#[macro_use]
> -mod bitfield;
> -
>  mod dma;
>  mod driver;
>  mod falcon;
> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
> index 206dab2e1335..1f08e6d4045a 100644
> --- a/drivers/gpu/nova-core/regs.rs
> +++ b/drivers/gpu/nova-core/regs.rs
> @@ -4,15 +4,13 @@
>  // but are mapped to types.
>  #![allow(non_camel_case_types)]
>  
> -#[macro_use]
> -pub(crate) mod macros;
> -
>  use crate::falcon::{
>      DmaTrfCmdSize, FalconCoreRev, FalconCoreRevSubversion, FalconFbifMemType, FalconFbifTarget,
>      FalconModSelAlgo, FalconSecurityModel, PFalcon2Base, PFalconBase, PeregrineCoreSelect,
>  };
>  use crate::gpu::{Architecture, Chipset};
>  use kernel::prelude::*;
> +use kernel::register;
>  
>  // PMC
>  
> @@ -331,6 +329,7 @@ pub(crate) fn mem_scrubbing_done(self) -> bool {
>  
>  pub(crate) mod gm107 {
>      // FUSE
> +    use kernel::register;
>  
>      register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00021c04 {
>          0:0     display_disabled as bool;
> @@ -339,6 +338,7 @@ pub(crate) mod gm107 {
>  
>  pub(crate) mod ga100 {
>      // FUSE
> +    use kernel::register;
>  
>      register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00820c04 {
>          0:0     display_disabled as bool;
> diff --git a/drivers/gpu/nova-core/bitfield.rs b/rust/kernel/bitfield.rs
> similarity index 91%
> rename from drivers/gpu/nova-core/bitfield.rs
> rename to rust/kernel/bitfield.rs
> index cbedbb0078f6..09cd5741598c 100644
> --- a/drivers/gpu/nova-core/bitfield.rs
> +++ b/rust/kernel/bitfield.rs
> @@ -9,7 +9,7 @@
>  /// # Syntax
>  ///
>  /// ```rust
> -/// use nova_core::bitfield;
> +/// use kernel::bitfield;
>  ///
>  /// #[derive(Debug, Clone, Copy, Default)]
>  /// enum Mode {
> @@ -82,10 +82,11 @@
>  ///   the result.
>  /// - `as <type> ?=> <try_into_type>` calls `<try_into_type>`'s `TryFrom::<<type>>` implementation
>  ///   and returns the result. This is useful with fields for which not all values are valid.
> +#[macro_export]
>  macro_rules! bitfield {
>      // Main entry point - defines the bitfield struct with fields
>      ($vis:vis struct $name:ident($storage:ty) $(, $comment:literal)? { $($fields:tt)* }) => {
> -        bitfield!(@core $vis $name $storage $(, $comment)? { $($fields)* });
> +        ::kernel::bitfield!(@core $vis $name $storage $(, $comment)? { $($fields)* });
>      };
>  
>      // All rules below are helpers.
> @@ -114,7 +115,7 @@ fn from(val: $name) -> $storage {
>              }
>          }
>  
> -        bitfield!(@fields_dispatcher $vis $name $storage { $($fields)* });
> +        ::kernel::bitfield!(@fields_dispatcher $vis $name $storage { $($fields)* });
>      };
>  
>      // Captures the fields and passes them to all the implementers that require field information.
> @@ -130,7 +131,7 @@ fn from(val: $name) -> $storage {
>          )*
>      }
>      ) => {
> -        bitfield!(@field_accessors $vis $name $storage {
> +        ::kernel::bitfield!(@field_accessors $vis $name $storage {
>              $(
>                  $hi:$lo $field as $type
>                  $(?=> $try_into_type)?
> @@ -139,8 +140,8 @@ fn from(val: $name) -> $storage {
>              ;
>              )*
>          });
> -        bitfield!(@debug $name { $($field;)* });
> -        bitfield!(@default $name { $($field;)* });
> +        ::kernel::bitfield!(@debug $name { $($field;)* });
> +        ::kernel::bitfield!(@default $name { $($field;)* });
>      };
>  
>      // Defines all the field getter/setter methods for `$name`.
> @@ -155,13 +156,13 @@ fn from(val: $name) -> $storage {
>          }
>      ) => {
>          $(
> -            bitfield!(@check_field_bounds $hi:$lo $field as $type);
> +            ::kernel::bitfield!(@check_field_bounds $hi:$lo $field as $type);
>          )*
>  
>          #[allow(dead_code)]
>          impl $name {
>              $(
> -            bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type
> +            ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type
>                  $(?=> $try_into_type)?
>                  $(=> $into_type)?
>                  $(, $comment)?
> @@ -198,7 +199,7 @@ impl $name {
>          @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
>              $(, $comment:literal)?;
>      ) => {
> -        bitfield!(
> +        ::kernel::bitfield!(
>              @leaf_accessor $vis $name $storage, $hi:$lo $field
>              { |f| <$into_type>::from(if f != 0 { true } else { false }) }
>              $into_type => $into_type $(, $comment)?;
> @@ -209,7 +210,7 @@ impl $name {
>      (
>          @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
>      ) => {
> -        bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
> +        ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
>      };
>  
>      // Catches the `?=>` syntax for non-boolean fields.
> @@ -217,7 +218,7 @@ impl $name {
>          @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
>              $(, $comment:literal)?;
>      ) => {
> -        bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
> +        ::kernel::bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
>              { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
>              ::core::result::Result<
>                  $try_into_type,
> @@ -231,7 +232,7 @@ impl $name {
>          @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
>              $(, $comment:literal)?;
>      ) => {
> -        bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
> +        ::kernel::bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
>              { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
>      };
>  
> @@ -240,7 +241,7 @@ impl $name {
>          @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt
>              $(, $comment:literal)?;
>      ) => {
> -        bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
> +        ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
>      };
>  
>      // Generates the accessor methods for a single field.
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 03b467722b86..a79b603604b1 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -8,6 +8,7 @@
>  use crate::{bindings, build_assert, ffi::c_void};
>  
>  pub mod mem;
> +pub mod register;
>  pub mod resource;
>  
>  pub use resource::Resource;
> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/rust/kernel/io/register.rs
> similarity index 93%
> rename from drivers/gpu/nova-core/regs/macros.rs
> rename to rust/kernel/io/register.rs
> index c0a5194e8d97..c24d956f122f 100644
> --- a/drivers/gpu/nova-core/regs/macros.rs
> +++ b/rust/kernel/io/register.rs
> @@ -17,7 +17,8 @@
>  /// The `T` generic argument is used to distinguish which base to use, in case a type provides
>  /// several bases. It is given to the `register!` macro to restrict the use of the register to
>  /// implementors of this particular variant.
> -pub(crate) trait RegisterBase<T> {
> +pub trait RegisterBase<T> {
> +    /// The base address for the register.
>      const BASE: usize;
>  }
>  
> @@ -26,7 +27,7 @@ pub(crate) trait RegisterBase<T> {
>  ///
>  /// Example:
>  ///
> -/// ```no_run
> +/// ```ignore
>  /// register!(BOOT_0 @ 0x00000100, "Basic revision information about the GPU" {
>  ///    3:0     minor_revision as u8, "Minor revision of the chip";
>  ///    7:4     major_revision as u8, "Major revision of the chip";
> @@ -39,7 +40,7 @@ pub(crate) trait RegisterBase<T> {
>  /// significant bits of the register. Each field can be accessed and modified using accessor
>  /// methods:
>  ///
> -/// ```no_run
> +/// ```ignore
>  /// // Read from the register's defined offset (0x100).
>  /// let boot0 = BOOT_0::read(&bar);
>  /// pr_info!("chip revision: {}.{}", boot0.major_revision(), boot0.minor_revision());
> @@ -61,7 +62,7 @@ pub(crate) trait RegisterBase<T> {
>  /// It is also possible to create a alias register by using the `=> ALIAS` syntax. This is useful
>  /// for cases where a register's interpretation depends on the context:
>  ///
> -/// ```no_run
> +/// ```ignore
>  /// register!(SCRATCH @ 0x00000200, "Scratch register" {
>  ///    31:0     value as u32, "Raw value";
>  /// });
> @@ -111,7 +112,7 @@ pub(crate) trait RegisterBase<T> {
>  /// this register needs to implement `RegisterBase<Base>`. Here is the above example translated
>  /// into code:
>  ///
> -/// ```no_run
> +/// ```ignore
>  /// // Type used to identify the base.
>  /// pub(crate) struct CpuCtlBase;
>  ///
> @@ -162,7 +163,7 @@ pub(crate) trait RegisterBase<T> {
>  /// compile-time or runtime bound checking. Simply define their address as `Address[Size]`, and add
>  /// an `idx` parameter to their `read`, `write` and `alter` methods:
>  ///
> -/// ```no_run
> +/// ```ignore
>  /// # fn no_run() -> Result<(), Error> {
>  /// # fn get_scratch_idx() -> usize {
>  /// #   0x15
> @@ -211,7 +212,7 @@ pub(crate) trait RegisterBase<T> {
>  /// Combining the two features described in the sections above, arrays of registers accessible from
>  /// a base can also be defined:
>  ///
> -/// ```no_run
> +/// ```ignore
>  /// # fn no_run() -> Result<(), Error> {
>  /// # fn get_scratch_idx() -> usize {
>  /// #   0x15
> @@ -273,28 +274,29 @@ pub(crate) trait RegisterBase<T> {
>  /// # Ok(())
>  /// # }
>  /// ```
> +#[macro_export]
>  macro_rules! register {
>      // Creates a register at a fixed offset of the MMIO space.
>      ($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* } ) => {
> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>          register!(@io_fixed $name @ $offset);
>      };
>  
>      // Creates an alias register of fixed offset register `alias` with its own fields.
>      ($name:ident => $alias:ident $(, $comment:literal)? { $($fields:tt)* } ) => {
> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>          register!(@io_fixed $name @ $alias::OFFSET);
>      };
>  
>      // Creates a register at a relative offset from a base address provider.
>      ($name:ident @ $base:ty [ $offset:literal ] $(, $comment:literal)? { $($fields:tt)* } ) => {
> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>          register!(@io_relative $name @ $base [ $offset ]);
>      };
>  
>      // Creates an alias register of relative offset register `alias` with its own fields.
>      ($name:ident => $base:ty [ $alias:ident ] $(, $comment:literal)? { $($fields:tt)* }) => {
> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>          register!(@io_relative $name @ $base [ $alias::OFFSET ]);
>      };
>  
> @@ -305,7 +307,7 @@ macro_rules! register {
>          }
>      ) => {
>          static_assert!(::core::mem::size_of::<u32>() <= $stride);
> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>          register!(@io_array $name @ $offset [ $size ; $stride ]);
>      };
>  
> @@ -326,7 +328,7 @@ macro_rules! register {
>              $(, $comment:literal)? { $($fields:tt)* }
>      ) => {
>          static_assert!(::core::mem::size_of::<u32>() <= $stride);
> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>          register!(@io_relative_array $name @ $base [ $offset [ $size ; $stride ] ]);
>      };
>  
> @@ -348,7 +350,7 @@ macro_rules! register {
>          }
>      ) => {
>          static_assert!($idx < $alias::SIZE);
> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>          register!(@io_relative $name @ $base [ $alias::OFFSET + $idx * $alias::STRIDE ] );
>      };
>  
> @@ -357,7 +359,7 @@ macro_rules! register {
>      // to avoid it being interpreted in place of the relative register array alias rule.
>      ($name:ident => $alias:ident [ $idx:expr ] $(, $comment:literal)? { $($fields:tt)* }) => {
>          static_assert!($idx < $alias::SIZE);
> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>          register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );
>      };
>  
> @@ -414,12 +416,12 @@ pub(crate) fn read<const SIZE: usize, T, B>(
>                  base: &B,
>              ) -> Self where
>                  T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> -                B: crate::regs::macros::RegisterBase<$base>,
> +                B: ::kernel::io::register::RegisterBase<$base>,
>              {
>                  const OFFSET: usize = $name::OFFSET;
>  
>                  let value = io.read32(
> -                    <B as crate::regs::macros::RegisterBase<$base>>::BASE + OFFSET
> +                    <B as ::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
>                  );
>  
>                  Self(value)
> @@ -435,13 +437,13 @@ pub(crate) fn write<const SIZE: usize, T, B>(
>                  base: &B,
>              ) where
>                  T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> -                B: crate::regs::macros::RegisterBase<$base>,
> +                B: ::kernel::io::register::RegisterBase<$base>,
>              {
>                  const OFFSET: usize = $name::OFFSET;
>  
>                  io.write32(
>                      self.0,
> -                    <B as crate::regs::macros::RegisterBase<$base>>::BASE + OFFSET
> +                    <B as ::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
>                  );
>              }
>  
> @@ -455,7 +457,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
>                  f: F,
>              ) where
>                  T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> -                B: crate::regs::macros::RegisterBase<$base>,
> +                B: ::kernel::io::register::RegisterBase<$base>,
>                  F: ::core::ops::FnOnce(Self) -> Self,
>              {
>                  let reg = f(Self::read(io, base));
> @@ -600,11 +602,11 @@ pub(crate) fn read<const SIZE: usize, T, B>(
>                  idx: usize,
>              ) -> Self where
>                  T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> -                B: crate::regs::macros::RegisterBase<$base>,
> +                B: ::kernel::io::register::RegisterBase<$base>,
>              {
>                  build_assert!(idx < Self::SIZE);
>  
> -                let offset = <B as crate::regs::macros::RegisterBase<$base>>::BASE +
> +                let offset = <B as ::kernel::io::register::RegisterBase<$base>>::BASE +
>                      Self::OFFSET + (idx * Self::STRIDE);
>                  let value = io.read32(offset);
>  
> @@ -622,11 +624,11 @@ pub(crate) fn write<const SIZE: usize, T, B>(
>                  idx: usize
>              ) where
>                  T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> -                B: crate::regs::macros::RegisterBase<$base>,
> +                B: ::kernel::io::register::RegisterBase<$base>,
>              {
>                  build_assert!(idx < Self::SIZE);
>  
> -                let offset = <B as crate::regs::macros::RegisterBase<$base>>::BASE +
> +                let offset = <B as ::kernel::io::register::RegisterBase<$base>>::BASE +
>                      Self::OFFSET + (idx * Self::STRIDE);
>  
>                  io.write32(self.0, offset);
> @@ -643,7 +645,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
>                  f: F,
>              ) where
>                  T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> -                B: crate::regs::macros::RegisterBase<$base>,
> +                B: ::kernel::io::register::RegisterBase<$base>,
>                  F: ::core::ops::FnOnce(Self) -> Self,
>              {
>                  let reg = f(Self::read(io, base, idx));
> @@ -662,7 +664,7 @@ pub(crate) fn try_read<const SIZE: usize, T, B>(
>                  idx: usize,
>              ) -> ::kernel::error::Result<Self> where
>                  T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> -                B: crate::regs::macros::RegisterBase<$base>,
> +                B: ::kernel::io::register::RegisterBase<$base>,
>              {
>                  if idx < Self::SIZE {
>                      Ok(Self::read(io, base, idx))
> @@ -684,7 +686,7 @@ pub(crate) fn try_write<const SIZE: usize, T, B>(
>                  idx: usize,
>              ) -> ::kernel::error::Result where
>                  T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> -                B: crate::regs::macros::RegisterBase<$base>,
> +                B: ::kernel::io::register::RegisterBase<$base>,
>              {
>                  if idx < Self::SIZE {
>                      Ok(self.write(io, base, idx))
> @@ -707,7 +709,7 @@ pub(crate) fn try_alter<const SIZE: usize, T, B, F>(
>                  f: F,
>              ) -> ::kernel::error::Result where
>                  T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> -                B: crate::regs::macros::RegisterBase<$base>,
> +                B: ::kernel::io::register::RegisterBase<$base>,
>                  F: ::core::ops::FnOnce(Self) -> Self,
>              {
>                  if idx < Self::SIZE {
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index fcffc3988a90..8f8260090c02 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -63,6 +63,7 @@
>  pub mod alloc;
>  #[cfg(CONFIG_AUXILIARY_BUS)]
>  pub mod auxiliary;
> +pub mod bitfield;
>  pub mod bits;
>  #[cfg(CONFIG_BLOCK)]
>  pub mod block;
> -- 
> 2.34.1
> 
>

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Joel Fernandes 1 month, 3 weeks ago

Hi Beata,

> On Oct 22, 2025, at 2:41 PM, Beata Michalska <beata.michalska@arm.com> wrote:
> 
> Hi Joel,
> 
> I know I'm chiming in a bit late, so apologies for that.

No problem.

> 
> The register! macro does seem to be a solid foundation for MMIO register
> definitions, thought there are few points that could be potentially
> [re]considered.

I agree. Just to clarify, Alexandre is the main developer of the register macro. I just
attempted to move the code and made some improvements :). I replied below:

> 
> The current design assumes a fixed, compile-time-known MMIO region size.
> It does not cover cases when the region size is known only at runtime.
> I do appreciate that in cases like that, we are loosing all the deliberate
> compile-time checks but it might be necessary to provide support for those as
> well (at some point at least).

Sure that could be useful if you have a use case. 

> 
> On the (potential) improvement side:
> 
> Allowing offsets to be expressions rather than literals would make the macro
> easier to use for regions defined at a fixed base offset, where subsequent
> offsets are derived from that base, i.e:
> 
> REG_1_BASE    -> 0x100
> REG_1_STATUS    -> REG_1_BASE + 0x0
> REG_1_CONTROL    -> REG_1_BASE + 0x04

This is already possible with the register macro using relative-registers (RegisterBase) right?

> ...
> 
> The alias mechanism is a nice touch. It might be worth allowing arrays of
> registers with explicit aliases to be defined in a single macro invocation,
> instead of repeating similar definitions, smth along the lines of:
> 
>  register!(
>      REG_STATUS @ 0x300[8; STRIDE] {
>          0:0 enabled as bool;
>          3:1 mode as u8;
>          7:4 flags as u8;
>      }
>      aliases {
>          REG_STATUS_ENABLED[0] {
>              0:0 enabled as bool;
>          }
>          REG_STATUS_MODE[0] {
>              3:1 mode as u8;
>          }
>          REG_STATUS_FLAGS[4] {
>              7:4 flags as u8;
>          }
>      }

The aliasing might be better do embed as syntax in the Bitfield itself,
instead of additional aliases{} blocks.
By the way, array of registers is also supported already as you may know.

>  );
> 
> 
> Finally, for runtime values such as indexes, it could be useful to verify once
> and then allow infallible reads/writes through some kind access token.

Why? The verification is already done at compile-time AFAICS.

> That might make runtime-safe access patterns simpler and more efficient.

Because it is compile-time, it is already runtime efficient :)

> I'm still pondering on how that could look like though (implementation-wise)

Patches welcomed! For now this still lives in nova-core and Alex is working
on adding support for BoundedInt after which we can move it out.

Thanks,

 - Joel


> ---
> BR
> Beata
> 
>> On Fri, Oct 03, 2025 at 11:47:47AM -0400, Joel Fernandes wrote:
>> Out of broad need for the register and bitfield macros in Rust, move
>> them out of nova into the kernel crate. Several usecases need them (Nova
>> is already using these and Tyr developers said they need them).
>> 
>> bitfield moved into kernel crate - defines bitfields in Rust.
>> register moved into io module - defines hardware registers and accessors.
>> 
>> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
>> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>> drivers/gpu/nova-core/falcon.rs               |  2 +-
>> drivers/gpu/nova-core/falcon/gsp.rs           |  4 +-
>> drivers/gpu/nova-core/falcon/sec2.rs          |  2 +-
>> drivers/gpu/nova-core/nova_core.rs            |  3 -
>> drivers/gpu/nova-core/regs.rs                 |  6 +-
>> .../gpu/nova-core => rust/kernel}/bitfield.rs | 27 ++++-----
>> rust/kernel/io.rs                             |  1 +
>> .../macros.rs => rust/kernel/io/register.rs   | 58 ++++++++++---------
>> rust/kernel/lib.rs                            |  1 +
>> 9 files changed, 54 insertions(+), 50 deletions(-)
>> rename {drivers/gpu/nova-core => rust/kernel}/bitfield.rs (91%)
>> rename drivers/gpu/nova-core/regs/macros.rs => rust/kernel/io/register.rs (93%)
>> 
>> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
>> index 37e6298195e4..a15fa98c8614 100644
>> --- a/drivers/gpu/nova-core/falcon.rs
>> +++ b/drivers/gpu/nova-core/falcon.rs
>> @@ -6,6 +6,7 @@
>> use hal::FalconHal;
>> use kernel::device;
>> use kernel::dma::DmaAddress;
>> +use kernel::io::register::RegisterBase;
>> use kernel::prelude::*;
>> use kernel::sync::aref::ARef;
>> use kernel::time::Delta;
>> @@ -14,7 +15,6 @@
>> use crate::driver::Bar0;
>> use crate::gpu::Chipset;
>> use crate::regs;
>> -use crate::regs::macros::RegisterBase;
>> use crate::util;
>> 
>> pub(crate) mod gsp;
>> diff --git a/drivers/gpu/nova-core/falcon/gsp.rs b/drivers/gpu/nova-core/falcon/gsp.rs
>> index f17599cb49fa..cd4960e997c8 100644
>> --- a/drivers/gpu/nova-core/falcon/gsp.rs
>> +++ b/drivers/gpu/nova-core/falcon/gsp.rs
>> @@ -1,9 +1,11 @@
>> // SPDX-License-Identifier: GPL-2.0
>> 
>> +use kernel::io::register::RegisterBase;
>> +
>> use crate::{
>>     driver::Bar0,
>>     falcon::{Falcon, FalconEngine, PFalcon2Base, PFalconBase},
>> -    regs::{self, macros::RegisterBase},
>> +    regs::self,
>> };
>> 
>> /// Type specifying the `Gsp` falcon engine. Cannot be instantiated.
>> diff --git a/drivers/gpu/nova-core/falcon/sec2.rs b/drivers/gpu/nova-core/falcon/sec2.rs
>> index 815786c8480d..81717868a8a8 100644
>> --- a/drivers/gpu/nova-core/falcon/sec2.rs
>> +++ b/drivers/gpu/nova-core/falcon/sec2.rs
>> @@ -1,7 +1,7 @@
>> // SPDX-License-Identifier: GPL-2.0
>> 
>> use crate::falcon::{FalconEngine, PFalcon2Base, PFalconBase};
>> -use crate::regs::macros::RegisterBase;
>> +use kernel::io::register::RegisterBase;
>> 
>> /// Type specifying the `Sec2` falcon engine. Cannot be instantiated.
>> pub(crate) struct Sec2(());
>> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
>> index 112277c7921e..fffcaee2249f 100644
>> --- a/drivers/gpu/nova-core/nova_core.rs
>> +++ b/drivers/gpu/nova-core/nova_core.rs
>> @@ -2,9 +2,6 @@
>> 
>> //! Nova Core GPU Driver
>> 
>> -#[macro_use]
>> -mod bitfield;
>> -
>> mod dma;
>> mod driver;
>> mod falcon;
>> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
>> index 206dab2e1335..1f08e6d4045a 100644
>> --- a/drivers/gpu/nova-core/regs.rs
>> +++ b/drivers/gpu/nova-core/regs.rs
>> @@ -4,15 +4,13 @@
>> // but are mapped to types.
>> #![allow(non_camel_case_types)]
>> 
>> -#[macro_use]
>> -pub(crate) mod macros;
>> -
>> use crate::falcon::{
>>     DmaTrfCmdSize, FalconCoreRev, FalconCoreRevSubversion, FalconFbifMemType, FalconFbifTarget,
>>     FalconModSelAlgo, FalconSecurityModel, PFalcon2Base, PFalconBase, PeregrineCoreSelect,
>> };
>> use crate::gpu::{Architecture, Chipset};
>> use kernel::prelude::*;
>> +use kernel::register;
>> 
>> // PMC
>> 
>> @@ -331,6 +329,7 @@ pub(crate) fn mem_scrubbing_done(self) -> bool {
>> 
>> pub(crate) mod gm107 {
>>     // FUSE
>> +    use kernel::register;
>> 
>>     register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00021c04 {
>>         0:0     display_disabled as bool;
>> @@ -339,6 +338,7 @@ pub(crate) mod gm107 {
>> 
>> pub(crate) mod ga100 {
>>     // FUSE
>> +    use kernel::register;
>> 
>>     register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00820c04 {
>>         0:0     display_disabled as bool;
>> diff --git a/drivers/gpu/nova-core/bitfield.rs b/rust/kernel/bitfield.rs
>> similarity index 91%
>> rename from drivers/gpu/nova-core/bitfield.rs
>> rename to rust/kernel/bitfield.rs
>> index cbedbb0078f6..09cd5741598c 100644
>> --- a/drivers/gpu/nova-core/bitfield.rs
>> +++ b/rust/kernel/bitfield.rs
>> @@ -9,7 +9,7 @@
>> /// # Syntax
>> ///
>> /// ```rust
>> -/// use nova_core::bitfield;
>> +/// use kernel::bitfield;
>> ///
>> /// #[derive(Debug, Clone, Copy, Default)]
>> /// enum Mode {
>> @@ -82,10 +82,11 @@
>> ///   the result.
>> /// - `as <type> ?=> <try_into_type>` calls `<try_into_type>`'s `TryFrom::<<type>>` implementation
>> ///   and returns the result. This is useful with fields for which not all values are valid.
>> +#[macro_export]
>> macro_rules! bitfield {
>>     // Main entry point - defines the bitfield struct with fields
>>     ($vis:vis struct $name:ident($storage:ty) $(, $comment:literal)? { $($fields:tt)* }) => {
>> -        bitfield!(@core $vis $name $storage $(, $comment)? { $($fields)* });
>> +        ::kernel::bitfield!(@core $vis $name $storage $(, $comment)? { $($fields)* });
>>     };
>> 
>>     // All rules below are helpers.
>> @@ -114,7 +115,7 @@ fn from(val: $name) -> $storage {
>>             }
>>         }
>> 
>> -        bitfield!(@fields_dispatcher $vis $name $storage { $($fields)* });
>> +        ::kernel::bitfield!(@fields_dispatcher $vis $name $storage { $($fields)* });
>>     };
>> 
>>     // Captures the fields and passes them to all the implementers that require field information.
>> @@ -130,7 +131,7 @@ fn from(val: $name) -> $storage {
>>         )*
>>     }
>>     ) => {
>> -        bitfield!(@field_accessors $vis $name $storage {
>> +        ::kernel::bitfield!(@field_accessors $vis $name $storage {
>>             $(
>>                 $hi:$lo $field as $type
>>                 $(?=> $try_into_type)?
>> @@ -139,8 +140,8 @@ fn from(val: $name) -> $storage {
>>             ;
>>             )*
>>         });
>> -        bitfield!(@debug $name { $($field;)* });
>> -        bitfield!(@default $name { $($field;)* });
>> +        ::kernel::bitfield!(@debug $name { $($field;)* });
>> +        ::kernel::bitfield!(@default $name { $($field;)* });
>>     };
>> 
>>     // Defines all the field getter/setter methods for `$name`.
>> @@ -155,13 +156,13 @@ fn from(val: $name) -> $storage {
>>         }
>>     ) => {
>>         $(
>> -            bitfield!(@check_field_bounds $hi:$lo $field as $type);
>> +            ::kernel::bitfield!(@check_field_bounds $hi:$lo $field as $type);
>>         )*
>> 
>>         #[allow(dead_code)]
>>         impl $name {
>>             $(
>> -            bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type
>> +            ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type
>>                 $(?=> $try_into_type)?
>>                 $(=> $into_type)?
>>                 $(, $comment)?
>> @@ -198,7 +199,7 @@ impl $name {
>>         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
>>             $(, $comment:literal)?;
>>     ) => {
>> -        bitfield!(
>> +        ::kernel::bitfield!(
>>             @leaf_accessor $vis $name $storage, $hi:$lo $field
>>             { |f| <$into_type>::from(if f != 0 { true } else { false }) }
>>             $into_type => $into_type $(, $comment)?;
>> @@ -209,7 +210,7 @@ impl $name {
>>     (
>>         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
>>     ) => {
>> -        bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
>> +        ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
>>     };
>> 
>>     // Catches the `?=>` syntax for non-boolean fields.
>> @@ -217,7 +218,7 @@ impl $name {
>>         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
>>             $(, $comment:literal)?;
>>     ) => {
>> -        bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
>> +        ::kernel::bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
>>             { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
>>             ::core::result::Result<
>>                 $try_into_type,
>> @@ -231,7 +232,7 @@ impl $name {
>>         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
>>             $(, $comment:literal)?;
>>     ) => {
>> -        bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
>> +        ::kernel::bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
>>             { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
>>     };
>> 
>> @@ -240,7 +241,7 @@ impl $name {
>>         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt
>>             $(, $comment:literal)?;
>>     ) => {
>> -        bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
>> +        ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
>>     };
>> 
>>     // Generates the accessor methods for a single field.
>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
>> index 03b467722b86..a79b603604b1 100644
>> --- a/rust/kernel/io.rs
>> +++ b/rust/kernel/io.rs
>> @@ -8,6 +8,7 @@
>> use crate::{bindings, build_assert, ffi::c_void};
>> 
>> pub mod mem;
>> +pub mod register;
>> pub mod resource;
>> 
>> pub use resource::Resource;
>> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/rust/kernel/io/register.rs
>> similarity index 93%
>> rename from drivers/gpu/nova-core/regs/macros.rs
>> rename to rust/kernel/io/register.rs
>> index c0a5194e8d97..c24d956f122f 100644
>> --- a/drivers/gpu/nova-core/regs/macros.rs
>> +++ b/rust/kernel/io/register.rs
>> @@ -17,7 +17,8 @@
>> /// The `T` generic argument is used to distinguish which base to use, in case a type provides
>> /// several bases. It is given to the `register!` macro to restrict the use of the register to
>> /// implementors of this particular variant.
>> -pub(crate) trait RegisterBase<T> {
>> +pub trait RegisterBase<T> {
>> +    /// The base address for the register.
>>     const BASE: usize;
>> }
>> 
>> @@ -26,7 +27,7 @@ pub(crate) trait RegisterBase<T> {
>> ///
>> /// Example:
>> ///
>> -/// ```no_run
>> +/// ```ignore
>> /// register!(BOOT_0 @ 0x00000100, "Basic revision information about the GPU" {
>> ///    3:0     minor_revision as u8, "Minor revision of the chip";
>> ///    7:4     major_revision as u8, "Major revision of the chip";
>> @@ -39,7 +40,7 @@ pub(crate) trait RegisterBase<T> {
>> /// significant bits of the register. Each field can be accessed and modified using accessor
>> /// methods:
>> ///
>> -/// ```no_run
>> +/// ```ignore
>> /// // Read from the register's defined offset (0x100).
>> /// let boot0 = BOOT_0::read(&bar);
>> /// pr_info!("chip revision: {}.{}", boot0.major_revision(), boot0.minor_revision());
>> @@ -61,7 +62,7 @@ pub(crate) trait RegisterBase<T> {
>> /// It is also possible to create a alias register by using the `=> ALIAS` syntax. This is useful
>> /// for cases where a register's interpretation depends on the context:
>> ///
>> -/// ```no_run
>> +/// ```ignore
>> /// register!(SCRATCH @ 0x00000200, "Scratch register" {
>> ///    31:0     value as u32, "Raw value";
>> /// });
>> @@ -111,7 +112,7 @@ pub(crate) trait RegisterBase<T> {
>> /// this register needs to implement `RegisterBase<Base>`. Here is the above example translated
>> /// into code:
>> ///
>> -/// ```no_run
>> +/// ```ignore
>> /// // Type used to identify the base.
>> /// pub(crate) struct CpuCtlBase;
>> ///
>> @@ -162,7 +163,7 @@ pub(crate) trait RegisterBase<T> {
>> /// compile-time or runtime bound checking. Simply define their address as `Address[Size]`, and add
>> /// an `idx` parameter to their `read`, `write` and `alter` methods:
>> ///
>> -/// ```no_run
>> +/// ```ignore
>> /// # fn no_run() -> Result<(), Error> {
>> /// # fn get_scratch_idx() -> usize {
>> /// #   0x15
>> @@ -211,7 +212,7 @@ pub(crate) trait RegisterBase<T> {
>> /// Combining the two features described in the sections above, arrays of registers accessible from
>> /// a base can also be defined:
>> ///
>> -/// ```no_run
>> +/// ```ignore
>> /// # fn no_run() -> Result<(), Error> {
>> /// # fn get_scratch_idx() -> usize {
>> /// #   0x15
>> @@ -273,28 +274,29 @@ pub(crate) trait RegisterBase<T> {
>> /// # Ok(())
>> /// # }
>> /// ```
>> +#[macro_export]
>> macro_rules! register {
>>     // Creates a register at a fixed offset of the MMIO space.
>>     ($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* } ) => {
>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>>         register!(@io_fixed $name @ $offset);
>>     };
>> 
>>     // Creates an alias register of fixed offset register `alias` with its own fields.
>>     ($name:ident => $alias:ident $(, $comment:literal)? { $($fields:tt)* } ) => {
>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>>         register!(@io_fixed $name @ $alias::OFFSET);
>>     };
>> 
>>     // Creates a register at a relative offset from a base address provider.
>>     ($name:ident @ $base:ty [ $offset:literal ] $(, $comment:literal)? { $($fields:tt)* } ) => {
>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>>         register!(@io_relative $name @ $base [ $offset ]);
>>     };
>> 
>>     // Creates an alias register of relative offset register `alias` with its own fields.
>>     ($name:ident => $base:ty [ $alias:ident ] $(, $comment:literal)? { $($fields:tt)* }) => {
>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>>         register!(@io_relative $name @ $base [ $alias::OFFSET ]);
>>     };
>> 
>> @@ -305,7 +307,7 @@ macro_rules! register {
>>         }
>>     ) => {
>>         static_assert!(::core::mem::size_of::<u32>() <= $stride);
>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>>         register!(@io_array $name @ $offset [ $size ; $stride ]);
>>     };
>> 
>> @@ -326,7 +328,7 @@ macro_rules! register {
>>             $(, $comment:literal)? { $($fields:tt)* }
>>     ) => {
>>         static_assert!(::core::mem::size_of::<u32>() <= $stride);
>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>>         register!(@io_relative_array $name @ $base [ $offset [ $size ; $stride ] ]);
>>     };
>> 
>> @@ -348,7 +350,7 @@ macro_rules! register {
>>         }
>>     ) => {
>>         static_assert!($idx < $alias::SIZE);
>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>>         register!(@io_relative $name @ $base [ $alias::OFFSET + $idx * $alias::STRIDE ] );
>>     };
>> 
>> @@ -357,7 +359,7 @@ macro_rules! register {
>>     // to avoid it being interpreted in place of the relative register array alias rule.
>>     ($name:ident => $alias:ident [ $idx:expr ] $(, $comment:literal)? { $($fields:tt)* }) => {
>>         static_assert!($idx < $alias::SIZE);
>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
>>         register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );
>>     };
>> 
>> @@ -414,12 +416,12 @@ pub(crate) fn read<const SIZE: usize, T, B>(
>>                 base: &B,
>>             ) -> Self where
>>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>> -                B: crate::regs::macros::RegisterBase<$base>,
>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>             {
>>                 const OFFSET: usize = $name::OFFSET;
>> 
>>                 let value = io.read32(
>> -                    <B as crate::regs::macros::RegisterBase<$base>>::BASE + OFFSET
>> +                    <B as ::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
>>                 );
>> 
>>                 Self(value)
>> @@ -435,13 +437,13 @@ pub(crate) fn write<const SIZE: usize, T, B>(
>>                 base: &B,
>>             ) where
>>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>> -                B: crate::regs::macros::RegisterBase<$base>,
>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>             {
>>                 const OFFSET: usize = $name::OFFSET;
>> 
>>                 io.write32(
>>                     self.0,
>> -                    <B as crate::regs::macros::RegisterBase<$base>>::BASE + OFFSET
>> +                    <B as ::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
>>                 );
>>             }
>> 
>> @@ -455,7 +457,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
>>                 f: F,
>>             ) where
>>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>> -                B: crate::regs::macros::RegisterBase<$base>,
>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>                 F: ::core::ops::FnOnce(Self) -> Self,
>>             {
>>                 let reg = f(Self::read(io, base));
>> @@ -600,11 +602,11 @@ pub(crate) fn read<const SIZE: usize, T, B>(
>>                 idx: usize,
>>             ) -> Self where
>>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>> -                B: crate::regs::macros::RegisterBase<$base>,
>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>             {
>>                 build_assert!(idx < Self::SIZE);
>> 
>> -                let offset = <B as crate::regs::macros::RegisterBase<$base>>::BASE +
>> +                let offset = <B as ::kernel::io::register::RegisterBase<$base>>::BASE +
>>                     Self::OFFSET + (idx * Self::STRIDE);
>>                 let value = io.read32(offset);
>> 
>> @@ -622,11 +624,11 @@ pub(crate) fn write<const SIZE: usize, T, B>(
>>                 idx: usize
>>             ) where
>>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>> -                B: crate::regs::macros::RegisterBase<$base>,
>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>             {
>>                 build_assert!(idx < Self::SIZE);
>> 
>> -                let offset = <B as crate::regs::macros::RegisterBase<$base>>::BASE +
>> +                let offset = <B as ::kernel::io::register::RegisterBase<$base>>::BASE +
>>                     Self::OFFSET + (idx * Self::STRIDE);
>> 
>>                 io.write32(self.0, offset);
>> @@ -643,7 +645,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
>>                 f: F,
>>             ) where
>>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>> -                B: crate::regs::macros::RegisterBase<$base>,
>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>                 F: ::core::ops::FnOnce(Self) -> Self,
>>             {
>>                 let reg = f(Self::read(io, base, idx));
>> @@ -662,7 +664,7 @@ pub(crate) fn try_read<const SIZE: usize, T, B>(
>>                 idx: usize,
>>             ) -> ::kernel::error::Result<Self> where
>>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>> -                B: crate::regs::macros::RegisterBase<$base>,
>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>             {
>>                 if idx < Self::SIZE {
>>                     Ok(Self::read(io, base, idx))
>> @@ -684,7 +686,7 @@ pub(crate) fn try_write<const SIZE: usize, T, B>(
>>                 idx: usize,
>>             ) -> ::kernel::error::Result where
>>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>> -                B: crate::regs::macros::RegisterBase<$base>,
>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>             {
>>                 if idx < Self::SIZE {
>>                     Ok(self.write(io, base, idx))
>> @@ -707,7 +709,7 @@ pub(crate) fn try_alter<const SIZE: usize, T, B, F>(
>>                 f: F,
>>             ) -> ::kernel::error::Result where
>>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>> -                B: crate::regs::macros::RegisterBase<$base>,
>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>                 F: ::core::ops::FnOnce(Self) -> Self,
>>             {
>>                 if idx < Self::SIZE {
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index fcffc3988a90..8f8260090c02 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -63,6 +63,7 @@
>> pub mod alloc;
>> #[cfg(CONFIG_AUXILIARY_BUS)]
>> pub mod auxiliary;
>> +pub mod bitfield;
>> pub mod bits;
>> #[cfg(CONFIG_BLOCK)]
>> pub mod block;
>> --
>> 2.34.1
>> 
>>

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Beata Michalska 1 month, 3 weeks ago

On Wed, Oct 22, 2025 at 07:37:55PM +0000, Joel Fernandes wrote:
> Hi Beata,
> 
> > On Oct 22, 2025, at 2:41 PM, Beata Michalska <beata.michalska@arm.com> wrote:
> > 
> > Hi Joel,
> > 
> > I know I'm chiming in a bit late, so apologies for that.
> 
> No problem.
> 
> > 
> > The register! macro does seem to be a solid foundation for MMIO register
> > definitions, thought there are few points that could be potentially
> > [re]considered.
> 
> I agree. Just to clarify, Alexandre is the main developer of the register macro. I just
> attempted to move the code and made some improvements :). I replied below:
> 
> > 
> > The current design assumes a fixed, compile-time-known MMIO region size.
> > It does not cover cases when the region size is known only at runtime.
> > I do appreciate that in cases like that, we are loosing all the deliberate
> > compile-time checks but it might be necessary to provide support for those as
> > well (at some point at least).
> 
> Sure that could be useful if you have a use case.
I guess everything that would use IoRequest::iomap(self), which generates
Io<SIZE=0> which is a game over for the macro.
> 
> > 
> > On the (potential) improvement side:
> > 
> > Allowing offsets to be expressions rather than literals would make the macro
> > easier to use for regions defined at a fixed base offset, where subsequent
> > offsets are derived from that base, i.e:
> > 
> > REG_1_BASE    -> 0x100
> > REG_1_STATUS    -> REG_1_BASE + 0x0
> > REG_1_CONTROL    -> REG_1_BASE + 0x04
> 
> This is already possible with the register macro using relative-registers (RegisterBase) right?
Probably though the use case I had in mind is relative array of registers.
It's fine to use the macro as is for few registers, having a significant number
of those gets cumbersome though. Unless I am misreading things.
> 
> > ...
> > 
> > The alias mechanism is a nice touch. It might be worth allowing arrays of
> > registers with explicit aliases to be defined in a single macro invocation,
> > instead of repeating similar definitions, smth along the lines of:
> > 
> >  register!(
> >      REG_STATUS @ 0x300[8; STRIDE] {
> >          0:0 enabled as bool;
> >          3:1 mode as u8;
> >          7:4 flags as u8;
> >      }
> >      aliases {
> >          REG_STATUS_ENABLED[0] {
> >              0:0 enabled as bool;
> >          }
> >          REG_STATUS_MODE[0] {
> >              3:1 mode as u8;
> >          }
> >          REG_STATUS_FLAGS[4] {
> >              7:4 flags as u8;
> >          }
> >      }
> 
> The aliasing might be better do embed as syntax in the Bitfield itself,
> instead of additional aliases{} blocks.
> By the way, array of registers is also supported already as you may know.
I was referring to aliasing having in mind array of registers.

> 
> >  );
> > 
> > 
> > Finally, for runtime values such as indexes, it could be useful to verify once
> > and then allow infallible reads/writes through some kind access token.
> 
> Why? The verification is already done at compile-time AFAICS.
Well, that's the point. Those are runtime values, and as of now, the only
support for those is for arrays of registers when one, when using try_xxx
methods, ends up with check being performed each time the method is called.

---
BR
Beata
> 
> > That might make runtime-safe access patterns simpler and more efficient.
> 
> Because it is compile-time, it is already runtime efficient :)
> 
> > I'm still pondering on how that could look like though (implementation-wise)
> 
> Patches welcomed! For now this still lives in nova-core and Alex is working
> on adding support for BoundedInt after which we can move it out.
> 
> Thanks,
> 
>  - Joel
> 
> 
> > ---
> > BR
> > Beata
> > 
> >> On Fri, Oct 03, 2025 at 11:47:47AM -0400, Joel Fernandes wrote:
> >> Out of broad need for the register and bitfield macros in Rust, move
> >> them out of nova into the kernel crate. Several usecases need them (Nova
> >> is already using these and Tyr developers said they need them).
> >> 
> >> bitfield moved into kernel crate - defines bitfields in Rust.
> >> register moved into io module - defines hardware registers and accessors.
> >> 
> >> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> >> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> >> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> >> ---
> >> drivers/gpu/nova-core/falcon.rs               |  2 +-
> >> drivers/gpu/nova-core/falcon/gsp.rs           |  4 +-
> >> drivers/gpu/nova-core/falcon/sec2.rs          |  2 +-
> >> drivers/gpu/nova-core/nova_core.rs            |  3 -
> >> drivers/gpu/nova-core/regs.rs                 |  6 +-
> >> .../gpu/nova-core => rust/kernel}/bitfield.rs | 27 ++++-----
> >> rust/kernel/io.rs                             |  1 +
> >> .../macros.rs => rust/kernel/io/register.rs   | 58 ++++++++++---------
> >> rust/kernel/lib.rs                            |  1 +
> >> 9 files changed, 54 insertions(+), 50 deletions(-)
> >> rename {drivers/gpu/nova-core => rust/kernel}/bitfield.rs (91%)
> >> rename drivers/gpu/nova-core/regs/macros.rs => rust/kernel/io/register.rs (93%)
> >> 
> >> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> >> index 37e6298195e4..a15fa98c8614 100644
> >> --- a/drivers/gpu/nova-core/falcon.rs
> >> +++ b/drivers/gpu/nova-core/falcon.rs
> >> @@ -6,6 +6,7 @@
> >> use hal::FalconHal;
> >> use kernel::device;
> >> use kernel::dma::DmaAddress;
> >> +use kernel::io::register::RegisterBase;
> >> use kernel::prelude::*;
> >> use kernel::sync::aref::ARef;
> >> use kernel::time::Delta;
> >> @@ -14,7 +15,6 @@
> >> use crate::driver::Bar0;
> >> use crate::gpu::Chipset;
> >> use crate::regs;
> >> -use crate::regs::macros::RegisterBase;
> >> use crate::util;
> >> 
> >> pub(crate) mod gsp;
> >> diff --git a/drivers/gpu/nova-core/falcon/gsp.rs b/drivers/gpu/nova-core/falcon/gsp.rs
> >> index f17599cb49fa..cd4960e997c8 100644
> >> --- a/drivers/gpu/nova-core/falcon/gsp.rs
> >> +++ b/drivers/gpu/nova-core/falcon/gsp.rs
> >> @@ -1,9 +1,11 @@
> >> // SPDX-License-Identifier: GPL-2.0
> >> 
> >> +use kernel::io::register::RegisterBase;
> >> +
> >> use crate::{
> >>     driver::Bar0,
> >>     falcon::{Falcon, FalconEngine, PFalcon2Base, PFalconBase},
> >> -    regs::{self, macros::RegisterBase},
> >> +    regs::self,
> >> };
> >> 
> >> /// Type specifying the `Gsp` falcon engine. Cannot be instantiated.
> >> diff --git a/drivers/gpu/nova-core/falcon/sec2.rs b/drivers/gpu/nova-core/falcon/sec2.rs
> >> index 815786c8480d..81717868a8a8 100644
> >> --- a/drivers/gpu/nova-core/falcon/sec2.rs
> >> +++ b/drivers/gpu/nova-core/falcon/sec2.rs
> >> @@ -1,7 +1,7 @@
> >> // SPDX-License-Identifier: GPL-2.0
> >> 
> >> use crate::falcon::{FalconEngine, PFalcon2Base, PFalconBase};
> >> -use crate::regs::macros::RegisterBase;
> >> +use kernel::io::register::RegisterBase;
> >> 
> >> /// Type specifying the `Sec2` falcon engine. Cannot be instantiated.
> >> pub(crate) struct Sec2(());
> >> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
> >> index 112277c7921e..fffcaee2249f 100644
> >> --- a/drivers/gpu/nova-core/nova_core.rs
> >> +++ b/drivers/gpu/nova-core/nova_core.rs
> >> @@ -2,9 +2,6 @@
> >> 
> >> //! Nova Core GPU Driver
> >> 
> >> -#[macro_use]
> >> -mod bitfield;
> >> -
> >> mod dma;
> >> mod driver;
> >> mod falcon;
> >> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
> >> index 206dab2e1335..1f08e6d4045a 100644
> >> --- a/drivers/gpu/nova-core/regs.rs
> >> +++ b/drivers/gpu/nova-core/regs.rs
> >> @@ -4,15 +4,13 @@
> >> // but are mapped to types.
> >> #![allow(non_camel_case_types)]
> >> 
> >> -#[macro_use]
> >> -pub(crate) mod macros;
> >> -
> >> use crate::falcon::{
> >>     DmaTrfCmdSize, FalconCoreRev, FalconCoreRevSubversion, FalconFbifMemType, FalconFbifTarget,
> >>     FalconModSelAlgo, FalconSecurityModel, PFalcon2Base, PFalconBase, PeregrineCoreSelect,
> >> };
> >> use crate::gpu::{Architecture, Chipset};
> >> use kernel::prelude::*;
> >> +use kernel::register;
> >> 
> >> // PMC
> >> 
> >> @@ -331,6 +329,7 @@ pub(crate) fn mem_scrubbing_done(self) -> bool {
> >> 
> >> pub(crate) mod gm107 {
> >>     // FUSE
> >> +    use kernel::register;
> >> 
> >>     register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00021c04 {
> >>         0:0     display_disabled as bool;
> >> @@ -339,6 +338,7 @@ pub(crate) mod gm107 {
> >> 
> >> pub(crate) mod ga100 {
> >>     // FUSE
> >> +    use kernel::register;
> >> 
> >>     register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00820c04 {
> >>         0:0     display_disabled as bool;
> >> diff --git a/drivers/gpu/nova-core/bitfield.rs b/rust/kernel/bitfield.rs
> >> similarity index 91%
> >> rename from drivers/gpu/nova-core/bitfield.rs
> >> rename to rust/kernel/bitfield.rs
> >> index cbedbb0078f6..09cd5741598c 100644
> >> --- a/drivers/gpu/nova-core/bitfield.rs
> >> +++ b/rust/kernel/bitfield.rs
> >> @@ -9,7 +9,7 @@
> >> /// # Syntax
> >> ///
> >> /// ```rust
> >> -/// use nova_core::bitfield;
> >> +/// use kernel::bitfield;
> >> ///
> >> /// #[derive(Debug, Clone, Copy, Default)]
> >> /// enum Mode {
> >> @@ -82,10 +82,11 @@
> >> ///   the result.
> >> /// - `as <type> ?=> <try_into_type>` calls `<try_into_type>`'s `TryFrom::<<type>>` implementation
> >> ///   and returns the result. This is useful with fields for which not all values are valid.
> >> +#[macro_export]
> >> macro_rules! bitfield {
> >>     // Main entry point - defines the bitfield struct with fields
> >>     ($vis:vis struct $name:ident($storage:ty) $(, $comment:literal)? { $($fields:tt)* }) => {
> >> -        bitfield!(@core $vis $name $storage $(, $comment)? { $($fields)* });
> >> +        ::kernel::bitfield!(@core $vis $name $storage $(, $comment)? { $($fields)* });
> >>     };
> >> 
> >>     // All rules below are helpers.
> >> @@ -114,7 +115,7 @@ fn from(val: $name) -> $storage {
> >>             }
> >>         }
> >> 
> >> -        bitfield!(@fields_dispatcher $vis $name $storage { $($fields)* });
> >> +        ::kernel::bitfield!(@fields_dispatcher $vis $name $storage { $($fields)* });
> >>     };
> >> 
> >>     // Captures the fields and passes them to all the implementers that require field information.
> >> @@ -130,7 +131,7 @@ fn from(val: $name) -> $storage {
> >>         )*
> >>     }
> >>     ) => {
> >> -        bitfield!(@field_accessors $vis $name $storage {
> >> +        ::kernel::bitfield!(@field_accessors $vis $name $storage {
> >>             $(
> >>                 $hi:$lo $field as $type
> >>                 $(?=> $try_into_type)?
> >> @@ -139,8 +140,8 @@ fn from(val: $name) -> $storage {
> >>             ;
> >>             )*
> >>         });
> >> -        bitfield!(@debug $name { $($field;)* });
> >> -        bitfield!(@default $name { $($field;)* });
> >> +        ::kernel::bitfield!(@debug $name { $($field;)* });
> >> +        ::kernel::bitfield!(@default $name { $($field;)* });
> >>     };
> >> 
> >>     // Defines all the field getter/setter methods for `$name`.
> >> @@ -155,13 +156,13 @@ fn from(val: $name) -> $storage {
> >>         }
> >>     ) => {
> >>         $(
> >> -            bitfield!(@check_field_bounds $hi:$lo $field as $type);
> >> +            ::kernel::bitfield!(@check_field_bounds $hi:$lo $field as $type);
> >>         )*
> >> 
> >>         #[allow(dead_code)]
> >>         impl $name {
> >>             $(
> >> -            bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type
> >> +            ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type
> >>                 $(?=> $try_into_type)?
> >>                 $(=> $into_type)?
> >>                 $(, $comment)?
> >> @@ -198,7 +199,7 @@ impl $name {
> >>         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
> >>             $(, $comment:literal)?;
> >>     ) => {
> >> -        bitfield!(
> >> +        ::kernel::bitfield!(
> >>             @leaf_accessor $vis $name $storage, $hi:$lo $field
> >>             { |f| <$into_type>::from(if f != 0 { true } else { false }) }
> >>             $into_type => $into_type $(, $comment)?;
> >> @@ -209,7 +210,7 @@ impl $name {
> >>     (
> >>         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
> >>     ) => {
> >> -        bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
> >> +        ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
> >>     };
> >> 
> >>     // Catches the `?=>` syntax for non-boolean fields.
> >> @@ -217,7 +218,7 @@ impl $name {
> >>         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
> >>             $(, $comment:literal)?;
> >>     ) => {
> >> -        bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
> >> +        ::kernel::bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
> >>             { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
> >>             ::core::result::Result<
> >>                 $try_into_type,
> >> @@ -231,7 +232,7 @@ impl $name {
> >>         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
> >>             $(, $comment:literal)?;
> >>     ) => {
> >> -        bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
> >> +        ::kernel::bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
> >>             { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
> >>     };
> >> 
> >> @@ -240,7 +241,7 @@ impl $name {
> >>         @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt
> >>             $(, $comment:literal)?;
> >>     ) => {
> >> -        bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
> >> +        ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
> >>     };
> >> 
> >>     // Generates the accessor methods for a single field.
> >> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> >> index 03b467722b86..a79b603604b1 100644
> >> --- a/rust/kernel/io.rs
> >> +++ b/rust/kernel/io.rs
> >> @@ -8,6 +8,7 @@
> >> use crate::{bindings, build_assert, ffi::c_void};
> >> 
> >> pub mod mem;
> >> +pub mod register;
> >> pub mod resource;
> >> 
> >> pub use resource::Resource;
> >> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/rust/kernel/io/register.rs
> >> similarity index 93%
> >> rename from drivers/gpu/nova-core/regs/macros.rs
> >> rename to rust/kernel/io/register.rs
> >> index c0a5194e8d97..c24d956f122f 100644
> >> --- a/drivers/gpu/nova-core/regs/macros.rs
> >> +++ b/rust/kernel/io/register.rs
> >> @@ -17,7 +17,8 @@
> >> /// The `T` generic argument is used to distinguish which base to use, in case a type provides
> >> /// several bases. It is given to the `register!` macro to restrict the use of the register to
> >> /// implementors of this particular variant.
> >> -pub(crate) trait RegisterBase<T> {
> >> +pub trait RegisterBase<T> {
> >> +    /// The base address for the register.
> >>     const BASE: usize;
> >> }
> >> 
> >> @@ -26,7 +27,7 @@ pub(crate) trait RegisterBase<T> {
> >> ///
> >> /// Example:
> >> ///
> >> -/// ```no_run
> >> +/// ```ignore
> >> /// register!(BOOT_0 @ 0x00000100, "Basic revision information about the GPU" {
> >> ///    3:0     minor_revision as u8, "Minor revision of the chip";
> >> ///    7:4     major_revision as u8, "Major revision of the chip";
> >> @@ -39,7 +40,7 @@ pub(crate) trait RegisterBase<T> {
> >> /// significant bits of the register. Each field can be accessed and modified using accessor
> >> /// methods:
> >> ///
> >> -/// ```no_run
> >> +/// ```ignore
> >> /// // Read from the register's defined offset (0x100).
> >> /// let boot0 = BOOT_0::read(&bar);
> >> /// pr_info!("chip revision: {}.{}", boot0.major_revision(), boot0.minor_revision());
> >> @@ -61,7 +62,7 @@ pub(crate) trait RegisterBase<T> {
> >> /// It is also possible to create a alias register by using the `=> ALIAS` syntax. This is useful
> >> /// for cases where a register's interpretation depends on the context:
> >> ///
> >> -/// ```no_run
> >> +/// ```ignore
> >> /// register!(SCRATCH @ 0x00000200, "Scratch register" {
> >> ///    31:0     value as u32, "Raw value";
> >> /// });
> >> @@ -111,7 +112,7 @@ pub(crate) trait RegisterBase<T> {
> >> /// this register needs to implement `RegisterBase<Base>`. Here is the above example translated
> >> /// into code:
> >> ///
> >> -/// ```no_run
> >> +/// ```ignore
> >> /// // Type used to identify the base.
> >> /// pub(crate) struct CpuCtlBase;
> >> ///
> >> @@ -162,7 +163,7 @@ pub(crate) trait RegisterBase<T> {
> >> /// compile-time or runtime bound checking. Simply define their address as `Address[Size]`, and add
> >> /// an `idx` parameter to their `read`, `write` and `alter` methods:
> >> ///
> >> -/// ```no_run
> >> +/// ```ignore
> >> /// # fn no_run() -> Result<(), Error> {
> >> /// # fn get_scratch_idx() -> usize {
> >> /// #   0x15
> >> @@ -211,7 +212,7 @@ pub(crate) trait RegisterBase<T> {
> >> /// Combining the two features described in the sections above, arrays of registers accessible from
> >> /// a base can also be defined:
> >> ///
> >> -/// ```no_run
> >> +/// ```ignore
> >> /// # fn no_run() -> Result<(), Error> {
> >> /// # fn get_scratch_idx() -> usize {
> >> /// #   0x15
> >> @@ -273,28 +274,29 @@ pub(crate) trait RegisterBase<T> {
> >> /// # Ok(())
> >> /// # }
> >> /// ```
> >> +#[macro_export]
> >> macro_rules! register {
> >>     // Creates a register at a fixed offset of the MMIO space.
> >>     ($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* } ) => {
> >> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >>         register!(@io_fixed $name @ $offset);
> >>     };
> >> 
> >>     // Creates an alias register of fixed offset register `alias` with its own fields.
> >>     ($name:ident => $alias:ident $(, $comment:literal)? { $($fields:tt)* } ) => {
> >> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >>         register!(@io_fixed $name @ $alias::OFFSET);
> >>     };
> >> 
> >>     // Creates a register at a relative offset from a base address provider.
> >>     ($name:ident @ $base:ty [ $offset:literal ] $(, $comment:literal)? { $($fields:tt)* } ) => {
> >> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >>         register!(@io_relative $name @ $base [ $offset ]);
> >>     };
> >> 
> >>     // Creates an alias register of relative offset register `alias` with its own fields.
> >>     ($name:ident => $base:ty [ $alias:ident ] $(, $comment:literal)? { $($fields:tt)* }) => {
> >> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >>         register!(@io_relative $name @ $base [ $alias::OFFSET ]);
> >>     };
> >> 
> >> @@ -305,7 +307,7 @@ macro_rules! register {
> >>         }
> >>     ) => {
> >>         static_assert!(::core::mem::size_of::<u32>() <= $stride);
> >> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >>         register!(@io_array $name @ $offset [ $size ; $stride ]);
> >>     };
> >> 
> >> @@ -326,7 +328,7 @@ macro_rules! register {
> >>             $(, $comment:literal)? { $($fields:tt)* }
> >>     ) => {
> >>         static_assert!(::core::mem::size_of::<u32>() <= $stride);
> >> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >>         register!(@io_relative_array $name @ $base [ $offset [ $size ; $stride ] ]);
> >>     };
> >> 
> >> @@ -348,7 +350,7 @@ macro_rules! register {
> >>         }
> >>     ) => {
> >>         static_assert!($idx < $alias::SIZE);
> >> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >>         register!(@io_relative $name @ $base [ $alias::OFFSET + $idx * $alias::STRIDE ] );
> >>     };
> >> 
> >> @@ -357,7 +359,7 @@ macro_rules! register {
> >>     // to avoid it being interpreted in place of the relative register array alias rule.
> >>     ($name:ident => $alias:ident [ $idx:expr ] $(, $comment:literal)? { $($fields:tt)* }) => {
> >>         static_assert!($idx < $alias::SIZE);
> >> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
> >>         register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );
> >>     };
> >> 
> >> @@ -414,12 +416,12 @@ pub(crate) fn read<const SIZE: usize, T, B>(
> >>                 base: &B,
> >>             ) -> Self where
> >>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >> -                B: crate::regs::macros::RegisterBase<$base>,
> >> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>             {
> >>                 const OFFSET: usize = $name::OFFSET;
> >> 
> >>                 let value = io.read32(
> >> -                    <B as crate::regs::macros::RegisterBase<$base>>::BASE + OFFSET
> >> +                    <B as ::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
> >>                 );
> >> 
> >>                 Self(value)
> >> @@ -435,13 +437,13 @@ pub(crate) fn write<const SIZE: usize, T, B>(
> >>                 base: &B,
> >>             ) where
> >>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >> -                B: crate::regs::macros::RegisterBase<$base>,
> >> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>             {
> >>                 const OFFSET: usize = $name::OFFSET;
> >> 
> >>                 io.write32(
> >>                     self.0,
> >> -                    <B as crate::regs::macros::RegisterBase<$base>>::BASE + OFFSET
> >> +                    <B as ::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
> >>                 );
> >>             }
> >> 
> >> @@ -455,7 +457,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
> >>                 f: F,
> >>             ) where
> >>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >> -                B: crate::regs::macros::RegisterBase<$base>,
> >> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>                 F: ::core::ops::FnOnce(Self) -> Self,
> >>             {
> >>                 let reg = f(Self::read(io, base));
> >> @@ -600,11 +602,11 @@ pub(crate) fn read<const SIZE: usize, T, B>(
> >>                 idx: usize,
> >>             ) -> Self where
> >>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >> -                B: crate::regs::macros::RegisterBase<$base>,
> >> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>             {
> >>                 build_assert!(idx < Self::SIZE);
> >> 
> >> -                let offset = <B as crate::regs::macros::RegisterBase<$base>>::BASE +
> >> +                let offset = <B as ::kernel::io::register::RegisterBase<$base>>::BASE +
> >>                     Self::OFFSET + (idx * Self::STRIDE);
> >>                 let value = io.read32(offset);
> >> 
> >> @@ -622,11 +624,11 @@ pub(crate) fn write<const SIZE: usize, T, B>(
> >>                 idx: usize
> >>             ) where
> >>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >> -                B: crate::regs::macros::RegisterBase<$base>,
> >> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>             {
> >>                 build_assert!(idx < Self::SIZE);
> >> 
> >> -                let offset = <B as crate::regs::macros::RegisterBase<$base>>::BASE +
> >> +                let offset = <B as ::kernel::io::register::RegisterBase<$base>>::BASE +
> >>                     Self::OFFSET + (idx * Self::STRIDE);
> >> 
> >>                 io.write32(self.0, offset);
> >> @@ -643,7 +645,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
> >>                 f: F,
> >>             ) where
> >>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >> -                B: crate::regs::macros::RegisterBase<$base>,
> >> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>                 F: ::core::ops::FnOnce(Self) -> Self,
> >>             {
> >>                 let reg = f(Self::read(io, base, idx));
> >> @@ -662,7 +664,7 @@ pub(crate) fn try_read<const SIZE: usize, T, B>(
> >>                 idx: usize,
> >>             ) -> ::kernel::error::Result<Self> where
> >>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >> -                B: crate::regs::macros::RegisterBase<$base>,
> >> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>             {
> >>                 if idx < Self::SIZE {
> >>                     Ok(Self::read(io, base, idx))
> >> @@ -684,7 +686,7 @@ pub(crate) fn try_write<const SIZE: usize, T, B>(
> >>                 idx: usize,
> >>             ) -> ::kernel::error::Result where
> >>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >> -                B: crate::regs::macros::RegisterBase<$base>,
> >> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>             {
> >>                 if idx < Self::SIZE {
> >>                     Ok(self.write(io, base, idx))
> >> @@ -707,7 +709,7 @@ pub(crate) fn try_alter<const SIZE: usize, T, B, F>(
> >>                 f: F,
> >>             ) -> ::kernel::error::Result where
> >>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >> -                B: crate::regs::macros::RegisterBase<$base>,
> >> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>                 F: ::core::ops::FnOnce(Self) -> Self,
> >>             {
> >>                 if idx < Self::SIZE {
> >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> >> index fcffc3988a90..8f8260090c02 100644
> >> --- a/rust/kernel/lib.rs
> >> +++ b/rust/kernel/lib.rs
> >> @@ -63,6 +63,7 @@
> >> pub mod alloc;
> >> #[cfg(CONFIG_AUXILIARY_BUS)]
> >> pub mod auxiliary;
> >> +pub mod bitfield;
> >> pub mod bits;
> >> #[cfg(CONFIG_BLOCK)]
> >> pub mod block;
> >> --
> >> 2.34.1
> >> 
> >>

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Joel Fernandes 1 month, 3 weeks ago

Hi Beata,

> On Oct 23, 2025, at 9:55 AM, Beata Michalska <beata.michalska@arm.com> wrote:
[...]
>>>
>>> The current design assumes a fixed, compile-time-known MMIO region size.
>>> It does not cover cases when the region size is known only at runtime.
>>> I do appreciate that in cases like that, we are loosing all the deliberate
>>> compile-time checks but it might be necessary to provide support for those
>>> as well (at some point at least).
>>
>> Sure that could be useful if you have a use case.
> I guess everything that would use IoRequest::iomap(self), which generates
> Io<SIZE=0> which is a game over for the macro.

I am curious what is your usecase for this such that size of the IO region
cannot be know until runtime, can you share? It should also be possible at
runtime to use the correct type, based on the IO region size IMO. The correct
type can encode the required size.

>>
>>>
>>> On the (potential) improvement side:
>>>
>>> Allowing offsets to be expressions rather than literals would make the macro
>>> easier to use for regions defined at a fixed base offset, where subsequent
>>> offsets are derived from that base, i.e:
>>>
>>> REG_1_BASE    -> 0x100
>>> REG_1_STATUS    -> REG_1_BASE + 0x0
>>> REG_1_CONTROL    -> REG_1_BASE + 0x04
>>
>> This is already possible with the register macro using relative-registers
>> (RegisterBase) right?
>
> Probably though the use case I had in mind is relative array of registers.
> It's fine to use the macro as is for few registers, having a significant
> number of those gets cumbersome though. Unless I am misreading things.

I am not sure it is cumbersome. The relative register syntax should be able to
support a larger number of registers. Could you share an example to describe the
issue with RegisterBase vs with your proposed syntax?

>>> The alias mechanism is a nice touch. It might be worth allowing arrays of
>>> registers with explicit aliases to be defined in a single macro invocation,
>>> instead of repeating similar definitions, smth along the lines of:
>>>
>>> register!(
>>>     REG_STATUS @ 0x300[8; STRIDE] {
>>>         0:0 enabled as bool;
>>>         3:1 mode as u8;
>>>         7:4 flags as u8;
>>>     }
>>>     aliases {
>>>         REG_STATUS_ENABLED[0] {
>>>             0:0 enabled as bool;
>>>         }
>>>         REG_STATUS_MODE[0] {
>>>             3:1 mode as u8;
>>>         }
>>>         REG_STATUS_FLAGS[4] {
>>>             7:4 flags as u8;
>>>         }
>>>     }
>>
>> The aliasing might be better do embed as syntax in the Bitfield itself,
>> instead of additional aliases{} blocks.
>> By the way, array of registers is also supported already as you may know.
> I was referring to aliasing having in mind array of registers.
>

AFAICS, either way you will still have a large number of alias definitions.
It might be better to invoke register macro explicitly for each alias IMO.

By the way, I do not follow your example because the names of the aliases in it
have the names of specific fields embedded in them. Whereas the register macro
aliases feature aliases the whole register, not specific fields, right?

>>> Finally, for runtime values such as indexes, it could be useful to verify
>>> once and then allow infallible reads/writes through some kind access token.
>>
>> Why? The verification is already done at compile-time AFAICS.
>
> Well, that's the point. Those are runtime values, and as of now, the only
> support for those is for arrays of registers when one, when using try_xxx
> methods, ends up with check being performed each time the method is called.

Ah for this part of your email, you are referring to try accessors. For the
fixed sizes regions at least, to avoid the runtime check, it will be possible to
accept BoundedInt [1] in the future. That type actually came up for the exact
same reason (keeping the checking light). This cleverly moves the checking to
the caller side which could be done in a slow path. If the size of the IO region
is fixed, then you don’t need to use try accessors at all if you use BoundedInt
whenever we have it.

thanks,

 - Joel

[1] https://lore.kernel.org/all/20251009-bounded_ints-v2-0-ff3d7fee3ffd@nvidia.com/

>
> ---
> BR
> Beata
>>
>>> That might make runtime-safe access patterns simpler and more efficient.
>>
>> Because it is compile-time, it is already runtime efficient :)
>>
>>> I'm still pondering on how that could look like though (implementation-wise)
>>
>> Patches welcomed! For now this still lives in nova-core and Alex is working
>> on adding support for BoundedInt after which we can move it out.
>>
>> Thanks,
>>
>> - Joel
>>
>>
>>> ---
>>> BR
>>> Beata
>>>
>>>> On Fri, Oct 03, 2025 at 11:47:47AM -0400, Joel Fernandes wrote:
>>>> Out of broad need for the register and bitfield macros in Rust, move
>>>> them out of nova into the kernel crate. Several usecases need them (Nova
>>>> is already using these and Tyr developers said they need them).
>>>>
>>>> bitfield moved into kernel crate - defines bitfields in Rust.
>>>> register moved into io module - defines hardware registers and accessors.
>>>>
>>>> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
>>>> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
>>>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>>>> ---
>>>> drivers/gpu/nova-core/falcon.rs               |  2 +-
>>>> drivers/gpu/nova-core/falcon/gsp.rs           |  4 +-
>>>> drivers/gpu/nova-core/falcon/sec2.rs          |  2 +-
>>>> drivers/gpu/nova-core/nova_core.rs            |  3 -
>>>> drivers/gpu/nova-core/regs.rs                 |  6 +-
>>>> .../gpu/nova-core => rust/kernel}/bitfield.rs | 27 ++++-----
>>>> rust/kernel/io.rs                             |  1 +
>>>> .../macros.rs => rust/kernel/io/register.rs   | 58 ++++++++++---------
>>>> rust/kernel/lib.rs                            |  1 +
>>>> 9 files changed, 54 insertions(+), 50 deletions(-)
>>>> rename {drivers/gpu/nova-core => rust/kernel}/bitfield.rs (91%)
>>>> rename drivers/gpu/nova-core/regs/macros.rs => rust/kernel/io/register.rs (93%)
>>>>
>>>> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
>>>> index 37e6298195e4..a15fa98c8614 100644
>>>> --- a/drivers/gpu/nova-core/falcon.rs
>>>> +++ b/drivers/gpu/nova-core/falcon.rs
>>>> @@ -6,6 +6,7 @@
>>>> use hal::FalconHal;
>>>> use kernel::device;
>>>> use kernel::dma::DmaAddress;
>>>> +use kernel::io::register::RegisterBase;
>>>> use kernel::prelude::*;
>>>> use kernel::sync::aref::ARef;
>>>> use kernel::time::Delta;
>>>> @@ -14,7 +15,6 @@
>>>> use crate::driver::Bar0;
>>>> use crate::gpu::Chipset;
>>>> use crate::regs;
>>>> -use crate::regs::macros::RegisterBase;
>>>> use crate::util;
>>>>
>>>> pub(crate) mod gsp;
>>>> diff --git a/drivers/gpu/nova-core/falcon/gsp.rs
b/drivers/gpu/nova-core/falcon/gsp.rs
>>>> index f17599cb49fa..cd4960e997c8 100644
>>>> --- a/drivers/gpu/nova-core/falcon/gsp.rs
>>>> +++ b/drivers/gpu/nova-core/falcon/gsp.rs
>>>> @@ -1,9 +1,11 @@
>>>> // SPDX-License-Identifier: GPL-2.0
>>>>
>>>> +use kernel::io::register::RegisterBase;
>>>> +
>>>> use crate::{
>>>>    driver::Bar0,
>>>>    falcon::{Falcon, FalconEngine, PFalcon2Base, PFalconBase},
>>>> -    regs::{self, macros::RegisterBase},
>>>> +    regs::self,
>>>> };
>>>>
>>>> /// Type specifying the `Gsp` falcon engine. Cannot be instantiated.
>>>> diff --git a/drivers/gpu/nova-core/falcon/sec2.rs
b/drivers/gpu/nova-core/falcon/sec2.rs
>>>> index 815786c8480d..81717868a8a8 100644
>>>> --- a/drivers/gpu/nova-core/falcon/sec2.rs
>>>> +++ b/drivers/gpu/nova-core/falcon/sec2.rs
>>>> @@ -1,7 +1,7 @@
>>>> // SPDX-License-Identifier: GPL-2.0
>>>>
>>>> use crate::falcon::{FalconEngine, PFalcon2Base, PFalconBase};
>>>> -use crate::regs::macros::RegisterBase;
>>>> +use kernel::io::register::RegisterBase;
>>>>
>>>> /// Type specifying the `Sec2` falcon engine. Cannot be instantiated.
>>>> pub(crate) struct Sec2(());
>>>> diff --git a/drivers/gpu/nova-core/nova_core.rs
b/drivers/gpu/nova-core/nova_core.rs
>>>> index 112277c7921e..fffcaee2249f 100644
>>>> --- a/drivers/gpu/nova-core/nova_core.rs
>>>> +++ b/drivers/gpu/nova-core/nova_core.rs
>>>> @@ -2,9 +2,6 @@
>>>>
>>>> //! Nova Core GPU Driver
>>>>
>>>> -#[macro_use]
>>>> -mod bitfield;
>>>> -
>>>> mod dma;
>>>> mod driver;
>>>> mod falcon;
>>>> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
>>>> index 206dab2e1335..1f08e6d4045a 100644
>>>> --- a/drivers/gpu/nova-core/regs.rs
>>>> +++ b/drivers/gpu/nova-core/regs.rs
>>>> @@ -4,15 +4,13 @@
>>>> // but are mapped to types.
>>>> #![allow(non_camel_case_types)]
>>>>
>>>> -#[macro_use]
>>>> -pub(crate) mod macros;
>>>> -
>>>> use crate::falcon::{
>>>>    DmaTrfCmdSize, FalconCoreRev, FalconCoreRevSubversion,
FalconFbifMemType, FalconFbifTarget,
>>>>    FalconModSelAlgo, FalconSecurityModel, PFalcon2Base, PFalconBase,
PeregrineCoreSelect,
>>>> };
>>>> use crate::gpu::{Architecture, Chipset};
>>>> use kernel::prelude::*;
>>>> +use kernel::register;
>>>>
>>>> // PMC
>>>>
>>>> @@ -331,6 +329,7 @@ pub(crate) fn mem_scrubbing_done(self) -> bool {
>>>>
>>>> pub(crate) mod gm107 {
>>>>    // FUSE
>>>> +    use kernel::register;
>>>>
>>>>    register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00021c04 {
>>>>        0:0     display_disabled as bool;
>>>> @@ -339,6 +338,7 @@ pub(crate) mod gm107 {
>>>>
>>>> pub(crate) mod ga100 {
>>>>    // FUSE
>>>> +    use kernel::register;
>>>>
>>>>    register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00820c04 {
>>>>        0:0     display_disabled as bool;
>>>> diff --git a/drivers/gpu/nova-core/bitfield.rs b/rust/kernel/bitfield.rs
>>>> similarity index 91%
>>>> rename from drivers/gpu/nova-core/bitfield.rs
>>>> rename to rust/kernel/bitfield.rs
>>>> index cbedbb0078f6..09cd5741598c 100644
>>>> --- a/drivers/gpu/nova-core/bitfield.rs
>>>> +++ b/rust/kernel/bitfield.rs
>>>> @@ -9,7 +9,7 @@
>>>> /// # Syntax
>>>> ///
>>>> /// ```rust
>>>> -/// use nova_core::bitfield;
>>>> +/// use kernel::bitfield;
>>>> ///
>>>> /// #[derive(Debug, Clone, Copy, Default)]
>>>> /// enum Mode {
>>>> @@ -82,10 +82,11 @@
>>>> ///   the result.
>>>> /// - `as <type> ?=> <try_into_type>` calls `<try_into_type>`'s
`TryFrom::<<type>>` implementation
>>>> ///   and returns the result. This is useful with fields for which not all
values are valid.
>>>> +#[macro_export]
>>>> macro_rules! bitfield {
>>>>    // Main entry point - defines the bitfield struct with fields
>>>>    ($vis:vis struct $name:ident($storage:ty) $(, $comment:literal)? {
$($fields:tt)* }) => {
>>>> -        bitfield!(@core $vis $name $storage $(, $comment)? { $($fields)* });
>>>> +        ::kernel::bitfield!(@core $vis $name $storage $(, $comment)? {
$($fields)* });
>>>>    };
>>>>
>>>>    // All rules below are helpers.
>>>> @@ -114,7 +115,7 @@ fn from(val: $name) -> $storage {
>>>>            }
>>>>        }
>>>>
>>>> -        bitfield!(@fields_dispatcher $vis $name $storage { $($fields)* });
>>>> +        ::kernel::bitfield!(@fields_dispatcher $vis $name $storage {
$($fields)* });
>>>>    };
>>>>
>>>>    // Captures the fields and passes them to all the implementers that
require field information.
>>>> @@ -130,7 +131,7 @@ fn from(val: $name) -> $storage {
>>>>        )*
>>>>    }
>>>>    ) => {
>>>> -        bitfield!(@field_accessors $vis $name $storage {
>>>> +        ::kernel::bitfield!(@field_accessors $vis $name $storage {
>>>>            $(
>>>>                $hi:$lo $field as $type
>>>>                $(?=> $try_into_type)?
>>>> @@ -139,8 +140,8 @@ fn from(val: $name) -> $storage {
>>>>            ;
>>>>            )*
>>>>        });
>>>> -        bitfield!(@debug $name { $($field;)* });
>>>> -        bitfield!(@default $name { $($field;)* });
>>>> +        ::kernel::bitfield!(@debug $name { $($field;)* });
>>>> +        ::kernel::bitfield!(@default $name { $($field;)* });
>>>>    };
>>>>
>>>>    // Defines all the field getter/setter methods for `$name`.
>>>> @@ -155,13 +156,13 @@ fn from(val: $name) -> $storage {
>>>>        }
>>>>    ) => {
>>>>        $(
>>>> -            bitfield!(@check_field_bounds $hi:$lo $field as $type);
>>>> +            ::kernel::bitfield!(@check_field_bounds $hi:$lo $field as $type);
>>>>        )*
>>>>
>>>>        #[allow(dead_code)]
>>>>        impl $name {
>>>>            $(
>>>> -            bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field
as $type
>>>> +            ::kernel::bitfield!(@field_accessor $vis $name $storage,
$hi:$lo $field as $type
>>>>                $(?=> $try_into_type)?
>>>>                $(=> $into_type)?
>>>>                $(, $comment)?
>>>> @@ -198,7 +199,7 @@ impl $name {
>>>>        @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt
$field:ident as bool => $into_type:ty
>>>>            $(, $comment:literal)?;
>>>>    ) => {
>>>> -        bitfield!(
>>>> +        ::kernel::bitfield!(
>>>>            @leaf_accessor $vis $name $storage, $hi:$lo $field
>>>>            { |f| <$into_type>::from(if f != 0 { true } else { false }) }
>>>>            $into_type => $into_type $(, $comment)?;
>>>> @@ -209,7 +210,7 @@ impl $name {
>>>>    (
>>>>        @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt
$field:ident as bool $(, $comment:literal)?;
>>>>    ) => {
>>>> -        bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as
bool => bool $(, $comment)?;);
>>>> +        ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo
$field as bool => bool $(, $comment)?;);
>>>>    };
>>>>
>>>>    // Catches the `?=>` syntax for non-boolean fields.
>>>> @@ -217,7 +218,7 @@ impl $name {
>>>>        @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt
$field:ident as $type:tt ?=> $try_into_type:ty
>>>>            $(, $comment:literal)?;
>>>>    ) => {
>>>> -        bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
>>>> +        ::kernel::bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
>>>>            { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
>>>>            ::core::result::Result<
>>>>                $try_into_type,
>>>> @@ -231,7 +232,7 @@ impl $name {
>>>>        @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt
$field:ident as $type:tt => $into_type:ty
>>>>            $(, $comment:literal)?;
>>>>    ) => {
>>>> -        bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
>>>> +        ::kernel::bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
>>>>            { |f| <$into_type>::from(f as $type) } $into_type => $into_type
$(, $comment)?;);
>>>>    };
>>>>
>>>> @@ -240,7 +241,7 @@ impl $name {
>>>>        @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt
$field:ident as $type:tt
>>>>            $(, $comment:literal)?;
>>>>    ) => {
>>>> -        bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as
$type => $type $(, $comment)?;);
>>>> +        ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo
$field as $type => $type $(, $comment)?;);
>>>>    };
>>>>
>>>>    // Generates the accessor methods for a single field.
>>>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
>>>> index 03b467722b86..a79b603604b1 100644
>>>> --- a/rust/kernel/io.rs
>>>> +++ b/rust/kernel/io.rs
>>>> @@ -8,6 +8,7 @@
>>>> use crate::{bindings, build_assert, ffi::c_void};
>>>>
>>>> pub mod mem;
>>>> +pub mod register;
>>>> pub mod resource;
>>>>
>>>> pub use resource::Resource;
>>>> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/rust/kernel/io/register.rs
>>>> similarity index 93%
>>>> rename from drivers/gpu/nova-core/regs/macros.rs
>>>> rename to rust/kernel/io/register.rs
>>>> index c0a5194e8d97..c24d956f122f 100644
>>>> --- a/drivers/gpu/nova-core/regs/macros.rs
>>>> +++ b/rust/kernel/io/register.rs
>>>> @@ -17,7 +17,8 @@
>>>> /// The `T` generic argument is used to distinguish which base to use, in
case a type provides
>>>> /// several bases. It is given to the `register!` macro to restrict the use
of the register to
>>>> /// implementors of this particular variant.
>>>> -pub(crate) trait RegisterBase<T> {
>>>> +pub trait RegisterBase<T> {
>>>> +    /// The base address for the register.
>>>>    const BASE: usize;
>>>> }
>>>>
>>>> @@ -26,7 +27,7 @@ pub(crate) trait RegisterBase<T> {
>>>> ///
>>>> /// Example:
>>>> ///
>>>> -/// ```no_run
>>>> +/// ```ignore
>>>> /// register!(BOOT_0 @ 0x00000100, "Basic revision information about the GPU" {
>>>> ///    3:0     minor_revision as u8, "Minor revision of the chip";
>>>> ///    7:4     major_revision as u8, "Major revision of the chip";
>>>> @@ -39,7 +40,7 @@ pub(crate) trait RegisterBase<T> {
>>>> /// significant bits of the register. Each field can be accessed and
modified using accessor
>>>> /// methods:
>>>> ///
>>>> -/// ```no_run
>>>> +/// ```ignore
>>>> /// // Read from the register's defined offset (0x100).
>>>> /// let boot0 = BOOT_0::read(&bar);
>>>> /// pr_info!("chip revision: {}.{}", boot0.major_revision(),
boot0.minor_revision());
>>>> @@ -61,7 +62,7 @@ pub(crate) trait RegisterBase<T> {
>>>> /// It is also possible to create a alias register by using the `=> ALIAS`
syntax. This is useful
>>>> /// for cases where a register's interpretation depends on the context:
>>>> ///
>>>> -/// ```no_run
>>>> +/// ```ignore
>>>> /// register!(SCRATCH @ 0x00000200, "Scratch register" {
>>>> ///    31:0     value as u32, "Raw value";
>>>> /// });
>>>> @@ -111,7 +112,7 @@ pub(crate) trait RegisterBase<T> {
>>>> /// this register needs to implement `RegisterBase<Base>`. Here is the
above example translated
>>>> /// into code:
>>>> ///
>>>> -/// ```no_run
>>>> +/// ```ignore
>>>> /// // Type used to identify the base.
>>>> /// pub(crate) struct CpuCtlBase;
>>>> ///
>>>> @@ -162,7 +163,7 @@ pub(crate) trait RegisterBase<T> {
>>>> /// compile-time or runtime bound checking. Simply define their address as
`Address[Size]`, and add
>>>> /// an `idx` parameter to their `read`, `write` and `alter` methods:
>>>> ///
>>>> -/// ```no_run
>>>> +/// ```ignore
>>>> /// # fn no_run() -> Result<(), Error> {
>>>> /// # fn get_scratch_idx() -> usize {
>>>> /// #   0x15
>>>> @@ -211,7 +212,7 @@ pub(crate) trait RegisterBase<T> {
>>>> /// Combining the two features described in the sections above, arrays of
registers accessible from
>>>> /// a base can also be defined:
>>>> ///
>>>> -/// ```no_run
>>>> +/// ```ignore
>>>> /// # fn no_run() -> Result<(), Error> {
>>>> /// # fn get_scratch_idx() -> usize {
>>>> /// #   0x15
>>>> @@ -273,28 +274,29 @@ pub(crate) trait RegisterBase<T> {
>>>> /// # Ok(())
>>>> /// # }
>>>> /// ```
>>>> +#[macro_export]
>>>> macro_rules! register {
>>>>    // Creates a register at a fixed offset of the MMIO space.
>>>>    ($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* }
) => {
>>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>>        register!(@io_fixed $name @ $offset);
>>>>    };
>>>>
>>>>    // Creates an alias register of fixed offset register `alias` with its
own fields.
>>>>    ($name:ident => $alias:ident $(, $comment:literal)? { $($fields:tt)* } )
=> {
>>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>>        register!(@io_fixed $name @ $alias::OFFSET);
>>>>    };
>>>>
>>>>    // Creates a register at a relative offset from a base address provider.
>>>>    ($name:ident @ $base:ty [ $offset:literal ] $(, $comment:literal)? {
$($fields:tt)* } ) => {
>>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>>        register!(@io_relative $name @ $base [ $offset ]);
>>>>    };
>>>>
>>>>    // Creates an alias register of relative offset register `alias` with
its own fields.
>>>>    ($name:ident => $base:ty [ $alias:ident ] $(, $comment:literal)? {
$($fields:tt)* }) => {
>>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>>        register!(@io_relative $name @ $base [ $alias::OFFSET ]);
>>>>    };
>>>>
>>>> @@ -305,7 +307,7 @@ macro_rules! register {
>>>>        }
>>>>    ) => {
>>>>        static_assert!(::core::mem::size_of::<u32>() <= $stride);
>>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>>        register!(@io_array $name @ $offset [ $size ; $stride ]);
>>>>    };
>>>>
>>>> @@ -326,7 +328,7 @@ macro_rules! register {
>>>>            $(, $comment:literal)? { $($fields:tt)* }
>>>>    ) => {
>>>>        static_assert!(::core::mem::size_of::<u32>() <= $stride);
>>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>>        register!(@io_relative_array $name @ $base [ $offset [ $size ;
$stride ] ]);
>>>>    };
>>>>
>>>> @@ -348,7 +350,7 @@ macro_rules! register {
>>>>        }
>>>>    ) => {
>>>>        static_assert!($idx < $alias::SIZE);
>>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>>        register!(@io_relative $name @ $base [ $alias::OFFSET + $idx *
$alias::STRIDE ] );
>>>>    };
>>>>
>>>> @@ -357,7 +359,7 @@ macro_rules! register {
>>>>    // to avoid it being interpreted in place of the relative register array
alias rule.
>>>>    ($name:ident => $alias:ident [ $idx:expr ] $(, $comment:literal)? {
$($fields:tt)* }) => {
>>>>        static_assert!($idx < $alias::SIZE);
>>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
>>>>        register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );
>>>>    };
>>>>
>>>> @@ -414,12 +416,12 @@ pub(crate) fn read<const SIZE: usize, T, B>(
>>>>                base: &B,
>>>>            ) -> Self where
>>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>>>> -                B: crate::regs::macros::RegisterBase<$base>,
>>>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>>>            {
>>>>                const OFFSET: usize = $name::OFFSET;
>>>>
>>>>                let value = io.read32(
>>>> -                    <B as crate::regs::macros::RegisterBase<$base>>::BASE
+ OFFSET
>>>> +                    <B as
::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
>>>>                );
>>>>
>>>>                Self(value)
>>>> @@ -435,13 +437,13 @@ pub(crate) fn write<const SIZE: usize, T, B>(
>>>>                base: &B,
>>>>            ) where
>>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>>>> -                B: crate::regs::macros::RegisterBase<$base>,
>>>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>>>            {
>>>>                const OFFSET: usize = $name::OFFSET;
>>>>
>>>>                io.write32(
>>>>                    self.0,
>>>> -                    <B as crate::regs::macros::RegisterBase<$base>>::BASE
+ OFFSET
>>>> +                    <B as
::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
>>>>                );
>>>>            }
>>>>
>>>> @@ -455,7 +457,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
>>>>                f: F,
>>>>            ) where
>>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>>>> -                B: crate::regs::macros::RegisterBase<$base>,
>>>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>>>                F: ::core::ops::FnOnce(Self) -> Self,
>>>>            {
>>>>                let reg = f(Self::read(io, base));
>>>> @@ -600,11 +602,11 @@ pub(crate) fn read<const SIZE: usize, T, B>(
>>>>                idx: usize,
>>>>            ) -> Self where
>>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>>>> -                B: crate::regs::macros::RegisterBase<$base>,
>>>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>>>            {
>>>>                build_assert!(idx < Self::SIZE);
>>>>
>>>> -                let offset = <B as
crate::regs::macros::RegisterBase<$base>>::BASE +
>>>> +                let offset = <B as
::kernel::io::register::RegisterBase<$base>>::BASE +
>>>>                    Self::OFFSET + (idx * Self::STRIDE);
>>>>                let value = io.read32(offset);
>>>>
>>>> @@ -622,11 +624,11 @@ pub(crate) fn write<const SIZE: usize, T, B>(
>>>>                idx: usize
>>>>            ) where
>>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>>>> -                B: crate::regs::macros::RegisterBase<$base>,
>>>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>>>            {
>>>>                build_assert!(idx < Self::SIZE);
>>>>
>>>> -                let offset = <B as
crate::regs::macros::RegisterBase<$base>>::BASE +
>>>> +                let offset = <B as
::kernel::io::register::RegisterBase<$base>>::BASE +
>>>>                    Self::OFFSET + (idx * Self::STRIDE);
>>>>
>>>>                io.write32(self.0, offset);
>>>> @@ -643,7 +645,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
>>>>                f: F,
>>>>            ) where
>>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>>>> -                B: crate::regs::macros::RegisterBase<$base>,
>>>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>>>                F: ::core::ops::FnOnce(Self) -> Self,
>>>>            {
>>>>                let reg = f(Self::read(io, base, idx));
>>>> @@ -662,7 +664,7 @@ pub(crate) fn try_read<const SIZE: usize, T, B>(
>>>>                idx: usize,
>>>>            ) -> ::kernel::error::Result<Self> where
>>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>>>> -                B: crate::regs::macros::RegisterBase<$base>,
>>>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>>>            {
>>>>                if idx < Self::SIZE {
>>>>                    Ok(Self::read(io, base, idx))
>>>> @@ -684,7 +686,7 @@ pub(crate) fn try_write<const SIZE: usize, T, B>(
>>>>                idx: usize,
>>>>            ) -> ::kernel::error::Result where
>>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>>>> -                B: crate::regs::macros::RegisterBase<$base>,
>>>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>>>            {
>>>>                if idx < Self::SIZE {
>>>>                    Ok(self.write(io, base, idx))
>>>> @@ -707,7 +709,7 @@ pub(crate) fn try_alter<const SIZE: usize, T, B, F>(
>>>>                f: F,
>>>>            ) -> ::kernel::error::Result where
>>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>>>> -                B: crate::regs::macros::RegisterBase<$base>,
>>>> +                B: ::kernel::io::register::RegisterBase<$base>,
>>>>                F: ::core::ops::FnOnce(Self) -> Self,
>>>>            {
>>>>                if idx < Self::SIZE {
>>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>>>> index fcffc3988a90..8f8260090c02 100644
>>>> --- a/rust/kernel/lib.rs
>>>> +++ b/rust/kernel/lib.rs
>>>> @@ -63,6 +63,7 @@
>>>> pub mod alloc;
>>>> #[cfg(CONFIG_AUXILIARY_BUS)]
>>>> pub mod auxiliary;
>>>> +pub mod bitfield;
>>>> pub mod bits;
>>>> #[cfg(CONFIG_BLOCK)]
>>>> pub mod block;
>>>> --
>>>> 2.34.1
>>>>
>>>>

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Beata Michalska 1 month, 2 weeks ago

On Thu, Oct 23, 2025 at 05:47:11PM -0400, Joel Fernandes wrote:
> Hi Beata,
> 
> > On Oct 23, 2025, at 9:55 AM, Beata Michalska <beata.michalska@arm.com> wrote:
> [...]
> >>>
> >>> The current design assumes a fixed, compile-time-known MMIO region size.
> >>> It does not cover cases when the region size is known only at runtime.
> >>> I do appreciate that in cases like that, we are loosing all the deliberate
> >>> compile-time checks but it might be necessary to provide support for those
> >>> as well (at some point at least).
> >>
> >> Sure that could be useful if you have a use case.
> > I guess everything that would use IoRequest::iomap(self), which generates
> > Io<SIZE=0> which is a game over for the macro.
> 
> I am curious what is your usecase for this such that size of the IO region
> cannot be know until runtime, can you share? It should also be possible at
> runtime to use the correct type, based on the IO region size IMO. The correct
> type can encode the required size.
It's more theoretical at this point, but there are drivers that do rely on
information from either DT or ACPI tables for the base address and size of the
MMIO region: anything that uses devm_platform_ioremap_resource() or
devm_platform_ioremap_resource_byname() I guess.
I'm not sure I follow your reference to the 'correct type' though.
Anyways, as it has been stated [1], this should not be a blocker for
getting the changes in.

> 
> >>
> >>>
> >>> On the (potential) improvement side:
> >>>
> >>> Allowing offsets to be expressions rather than literals would make the macro
> >>> easier to use for regions defined at a fixed base offset, where subsequent
> >>> offsets are derived from that base, i.e:
> >>>
> >>> REG_1_BASE    -> 0x100
> >>> REG_1_STATUS    -> REG_1_BASE + 0x0
> >>> REG_1_CONTROL    -> REG_1_BASE + 0x04
> >>
> >> This is already possible with the register macro using relative-registers
> >> (RegisterBase) right?
> >
> > Probably though the use case I had in mind is relative array of registers.
> > It's fine to use the macro as is for few registers, having a significant
> > number of those gets cumbersome though. Unless I am misreading things.
> 
> I am not sure it is cumbersome. The relative register syntax should be able to
> support a larger number of registers. Could you share an example to describe the
> issue with RegisterBase vs with your proposed syntax?
Let's assume there is an array of arrays of registers:

+-------------------------------+
|	       ...		|
|				|
+-------------------------------+ -> SECTION_1 = BASE
|	     ENTRY_1		|
+-------------------------------+
|	     ENTRY_2		|
+-------------------------------+
|	       ...		|
+-------------------------------+
|	     ENTRY_N		|
+-------------------------------+ -> SECTION_2 = BASE + SIZEOF(SECTION)
|	     ENTRY_1		|
+-------------------------------+
|	     ENTRY_2		|
+-------------------------------+
|	       ...		|
+-------------------------------+
|	     ENTRY_N		|
+-------------------------------+ -> SECTION_M = BASE + SIZEOF(SECTION) *(M-1)
|	     ENTRY_1		|
+-------------------------------+
|	     ENTRY_2		|
+-------------------------------+
|	       ...		|
+-------------------------------+
|	     ENTRY_N		|
+-------------------------------+

Each section has the same layout and is composed on N entries.
Access pattern for thise is: get ENTRY_x for a section ID, where the ID is
provided at runtime, entries are fixed.
The most straightforward approach (for me at least) was to use register! macro
for each ENTRY, making each of them an array whose stride skips from one section
to the same filed in the next section. With the stride being the full section
size, index section walks through all sections for that entry, i.e.:


register!(ENTRY_1 @ (BASE + 0x00) [SECTION_COUNT; SECTION_SIZE] {
	31:0 value as u32;
});

register!(ENTRY_2 @ (BASE + SECTION_SIZE) [SECTION_COUNT; SECTION_SIZE] {
	31:0 value as u32;
});

...

Pretty straightforward and here is where the expression of the macro's offset
could be handy.

Now, with the relative registers approach ..., unless I am not mistaken,
it would look like:

pub(crate) struct SectionBase;

pub(crate) struct Section<const ID: usize>;

impl<const ID: usize> RegisterBase<SetionBase> for Section<ID> {
	const BASE: usize = BASE + ID + SECTION_SIZE;
}

or have a type per each Section explicitely.

register!(SECTION @ BASE[SECTION_COUNT; SECTION_SIZE] {
	31:0 value as u32;
});

register!(ENTRY @ SectionBase[0x0000[ENTRY_COUNT; ENTRY_SIZE]] {
	31:0 value as u32;
});

Now alias per each entry: mapping ENTRY_n => ENTRY_name so that things are more
descriptive and it's easier to code accessing functional regs by name rather
than by index:

register!(ENTRY_1 => SectionBase[ENTRY[0]] { 31:0 value as u32; });
...

Reading say ENTRY_4 in Sction 18:
ENTRY::try_read(io, &Section::<18>, 4)?;

So far so good (though still more verbose approach).
Because Section is being indexed based on a runtime value this will not work
as we need compile-time BASE for relative registers.
This would require mapping between runtime idx and Section::<n>:
not ideal I would say.

Please correct me if I am wrong.
> 
> >>> The alias mechanism is a nice touch. It might be worth allowing arrays of
> >>> registers with explicit aliases to be defined in a single macro invocation,
> >>> instead of repeating similar definitions, smth along the lines of:
> >>>
> >>> register!(
> >>>     REG_STATUS @ 0x300[8; STRIDE] {
> >>>         0:0 enabled as bool;
> >>>         3:1 mode as u8;
> >>>         7:4 flags as u8;
> >>>     }
> >>>     aliases {
> >>>         REG_STATUS_ENABLED[0] {
> >>>             0:0 enabled as bool;
> >>>         }
> >>>         REG_STATUS_MODE[0] {
> >>>             3:1 mode as u8;
> >>>         }
> >>>         REG_STATUS_FLAGS[4] {
> >>>             7:4 flags as u8;
> >>>         }
> >>>     }
> >>
> >> The aliasing might be better do embed as syntax in the Bitfield itself,
> >> instead of additional aliases{} blocks.
> >> By the way, array of registers is also supported already as you may know.
> > I was referring to aliasing having in mind array of registers.
> >
> 
> AFAICS, either way you will still have a large number of alias definitions.
> It might be better to invoke register macro explicitly for each alias IMO.
For each alias you do have to have an entry: whether that would be embedded in
the macro syntax or explicitly defined via separate register! calls.
I just think it's easier to have it embedded otherwise you need to repeat the
same pattern for each alias , as of:

register!(ALIAS_1 => BASE_REG ...);
register!(ALIAS_2 => BASE_REG ...);
register!(ALIAS_3 => BASE_REG ...);

Call me lazy (which I am) , but I find it redundant.
Anyways it's nothing major, was only a suggestion for potential improvement.
> 
> By the way, I do not follow your example because the names of the aliases in it
> have the names of specific fields embedded in them. Whereas the register macro
> aliases feature aliases the whole register, not specific fields, right?
Actually you can alias a register as whole  or with explicit fields.
It just gives you a different view of the same register (memory).
> 
> >>> Finally, for runtime values such as indexes, it could be useful to verify
> >>> once and then allow infallible reads/writes through some kind access token.
> >>
> >> Why? The verification is already done at compile-time AFAICS.
> >
> > Well, that's the point. Those are runtime values, and as of now, the only
> > support for those is for arrays of registers when one, when using try_xxx
> > methods, ends up with check being performed each time the method is called.
> 
> Ah for this part of your email, you are referring to try accessors. For the
> fixed sizes regions at least, to avoid the runtime check, it will be possible to
> accept BoundedInt [1] in the future. That type actually came up for the exact
> same reason (keeping the checking light). This cleverly moves the checking to
> the caller side which could be done in a slow path. If the size of the IO region
> is fixed, then you don’t need to use try accessors at all if you use BoundedInt
> whenever we have it.
> 
All right - my bad. Guess I need to have a look at BoundedInt then.
Thanks for the info.


---
[1] https://lore.kernel.org/rust-for-linux/DDPRDKFEK3H3.2CE3YMXRTLGTI@kernel.org/
---

BR
Beata
> thanks,
> 
>  - Joel
> 
> [1] https://lore.kernel.org/all/20251009-bounded_ints-v2-0-ff3d7fee3ffd@nvidia.com/
>
> >
> > ---
> > BR
> > Beata
> >>
> >>> That might make runtime-safe access patterns simpler and more efficient.
> >>
> >> Because it is compile-time, it is already runtime efficient :)
> >>
> >>> I'm still pondering on how that could look like though (implementation-wise)
> >>
> >> Patches welcomed! For now this still lives in nova-core and Alex is working
> >> on adding support for BoundedInt after which we can move it out.
> >>
> >> Thanks,
> >>
> >> - Joel
> >>
> >>
> >>> ---
> >>> BR
> >>> Beata
> >>>
> >>>> On Fri, Oct 03, 2025 at 11:47:47AM -0400, Joel Fernandes wrote:
> >>>> Out of broad need for the register and bitfield macros in Rust, move
> >>>> them out of nova into the kernel crate. Several usecases need them (Nova
> >>>> is already using these and Tyr developers said they need them).
> >>>>
> >>>> bitfield moved into kernel crate - defines bitfields in Rust.
> >>>> register moved into io module - defines hardware registers and accessors.
> >>>>
> >>>> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> >>>> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> >>>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> >>>> ---
> >>>> drivers/gpu/nova-core/falcon.rs               |  2 +-
> >>>> drivers/gpu/nova-core/falcon/gsp.rs           |  4 +-
> >>>> drivers/gpu/nova-core/falcon/sec2.rs          |  2 +-
> >>>> drivers/gpu/nova-core/nova_core.rs            |  3 -
> >>>> drivers/gpu/nova-core/regs.rs                 |  6 +-
> >>>> .../gpu/nova-core => rust/kernel}/bitfield.rs | 27 ++++-----
> >>>> rust/kernel/io.rs                             |  1 +
> >>>> .../macros.rs => rust/kernel/io/register.rs   | 58 ++++++++++---------
> >>>> rust/kernel/lib.rs                            |  1 +
> >>>> 9 files changed, 54 insertions(+), 50 deletions(-)
> >>>> rename {drivers/gpu/nova-core => rust/kernel}/bitfield.rs (91%)
> >>>> rename drivers/gpu/nova-core/regs/macros.rs => rust/kernel/io/register.rs (93%)
> >>>>
> >>>> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> >>>> index 37e6298195e4..a15fa98c8614 100644
> >>>> --- a/drivers/gpu/nova-core/falcon.rs
> >>>> +++ b/drivers/gpu/nova-core/falcon.rs
> >>>> @@ -6,6 +6,7 @@
> >>>> use hal::FalconHal;
> >>>> use kernel::device;
> >>>> use kernel::dma::DmaAddress;
> >>>> +use kernel::io::register::RegisterBase;
> >>>> use kernel::prelude::*;
> >>>> use kernel::sync::aref::ARef;
> >>>> use kernel::time::Delta;
> >>>> @@ -14,7 +15,6 @@
> >>>> use crate::driver::Bar0;
> >>>> use crate::gpu::Chipset;
> >>>> use crate::regs;
> >>>> -use crate::regs::macros::RegisterBase;
> >>>> use crate::util;
> >>>>
> >>>> pub(crate) mod gsp;
> >>>> diff --git a/drivers/gpu/nova-core/falcon/gsp.rs
> b/drivers/gpu/nova-core/falcon/gsp.rs
> >>>> index f17599cb49fa..cd4960e997c8 100644
> >>>> --- a/drivers/gpu/nova-core/falcon/gsp.rs
> >>>> +++ b/drivers/gpu/nova-core/falcon/gsp.rs
> >>>> @@ -1,9 +1,11 @@
> >>>> // SPDX-License-Identifier: GPL-2.0
> >>>>
> >>>> +use kernel::io::register::RegisterBase;
> >>>> +
> >>>> use crate::{
> >>>>    driver::Bar0,
> >>>>    falcon::{Falcon, FalconEngine, PFalcon2Base, PFalconBase},
> >>>> -    regs::{self, macros::RegisterBase},
> >>>> +    regs::self,
> >>>> };
> >>>>
> >>>> /// Type specifying the `Gsp` falcon engine. Cannot be instantiated.
> >>>> diff --git a/drivers/gpu/nova-core/falcon/sec2.rs
> b/drivers/gpu/nova-core/falcon/sec2.rs
> >>>> index 815786c8480d..81717868a8a8 100644
> >>>> --- a/drivers/gpu/nova-core/falcon/sec2.rs
> >>>> +++ b/drivers/gpu/nova-core/falcon/sec2.rs
> >>>> @@ -1,7 +1,7 @@
> >>>> // SPDX-License-Identifier: GPL-2.0
> >>>>
> >>>> use crate::falcon::{FalconEngine, PFalcon2Base, PFalconBase};
> >>>> -use crate::regs::macros::RegisterBase;
> >>>> +use kernel::io::register::RegisterBase;
> >>>>
> >>>> /// Type specifying the `Sec2` falcon engine. Cannot be instantiated.
> >>>> pub(crate) struct Sec2(());
> >>>> diff --git a/drivers/gpu/nova-core/nova_core.rs
> b/drivers/gpu/nova-core/nova_core.rs
> >>>> index 112277c7921e..fffcaee2249f 100644
> >>>> --- a/drivers/gpu/nova-core/nova_core.rs
> >>>> +++ b/drivers/gpu/nova-core/nova_core.rs
> >>>> @@ -2,9 +2,6 @@
> >>>>
> >>>> //! Nova Core GPU Driver
> >>>>
> >>>> -#[macro_use]
> >>>> -mod bitfield;
> >>>> -
> >>>> mod dma;
> >>>> mod driver;
> >>>> mod falcon;
> >>>> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
> >>>> index 206dab2e1335..1f08e6d4045a 100644
> >>>> --- a/drivers/gpu/nova-core/regs.rs
> >>>> +++ b/drivers/gpu/nova-core/regs.rs
> >>>> @@ -4,15 +4,13 @@
> >>>> // but are mapped to types.
> >>>> #![allow(non_camel_case_types)]
> >>>>
> >>>> -#[macro_use]
> >>>> -pub(crate) mod macros;
> >>>> -
> >>>> use crate::falcon::{
> >>>>    DmaTrfCmdSize, FalconCoreRev, FalconCoreRevSubversion,
> FalconFbifMemType, FalconFbifTarget,
> >>>>    FalconModSelAlgo, FalconSecurityModel, PFalcon2Base, PFalconBase,
> PeregrineCoreSelect,
> >>>> };
> >>>> use crate::gpu::{Architecture, Chipset};
> >>>> use kernel::prelude::*;
> >>>> +use kernel::register;
> >>>>
> >>>> // PMC
> >>>>
> >>>> @@ -331,6 +329,7 @@ pub(crate) fn mem_scrubbing_done(self) -> bool {
> >>>>
> >>>> pub(crate) mod gm107 {
> >>>>    // FUSE
> >>>> +    use kernel::register;
> >>>>
> >>>>    register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00021c04 {
> >>>>        0:0     display_disabled as bool;
> >>>> @@ -339,6 +338,7 @@ pub(crate) mod gm107 {
> >>>>
> >>>> pub(crate) mod ga100 {
> >>>>    // FUSE
> >>>> +    use kernel::register;
> >>>>
> >>>>    register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00820c04 {
> >>>>        0:0     display_disabled as bool;
> >>>> diff --git a/drivers/gpu/nova-core/bitfield.rs b/rust/kernel/bitfield.rs
> >>>> similarity index 91%
> >>>> rename from drivers/gpu/nova-core/bitfield.rs
> >>>> rename to rust/kernel/bitfield.rs
> >>>> index cbedbb0078f6..09cd5741598c 100644
> >>>> --- a/drivers/gpu/nova-core/bitfield.rs
> >>>> +++ b/rust/kernel/bitfield.rs
> >>>> @@ -9,7 +9,7 @@
> >>>> /// # Syntax
> >>>> ///
> >>>> /// ```rust
> >>>> -/// use nova_core::bitfield;
> >>>> +/// use kernel::bitfield;
> >>>> ///
> >>>> /// #[derive(Debug, Clone, Copy, Default)]
> >>>> /// enum Mode {
> >>>> @@ -82,10 +82,11 @@
> >>>> ///   the result.
> >>>> /// - `as <type> ?=> <try_into_type>` calls `<try_into_type>`'s
> `TryFrom::<<type>>` implementation
> >>>> ///   and returns the result. This is useful with fields for which not all
> values are valid.
> >>>> +#[macro_export]
> >>>> macro_rules! bitfield {
> >>>>    // Main entry point - defines the bitfield struct with fields
> >>>>    ($vis:vis struct $name:ident($storage:ty) $(, $comment:literal)? {
> $($fields:tt)* }) => {
> >>>> -        bitfield!(@core $vis $name $storage $(, $comment)? { $($fields)* });
> >>>> +        ::kernel::bitfield!(@core $vis $name $storage $(, $comment)? {
> $($fields)* });
> >>>>    };
> >>>>
> >>>>    // All rules below are helpers.
> >>>> @@ -114,7 +115,7 @@ fn from(val: $name) -> $storage {
> >>>>            }
> >>>>        }
> >>>>
> >>>> -        bitfield!(@fields_dispatcher $vis $name $storage { $($fields)* });
> >>>> +        ::kernel::bitfield!(@fields_dispatcher $vis $name $storage {
> $($fields)* });
> >>>>    };
> >>>>
> >>>>    // Captures the fields and passes them to all the implementers that
> require field information.
> >>>> @@ -130,7 +131,7 @@ fn from(val: $name) -> $storage {
> >>>>        )*
> >>>>    }
> >>>>    ) => {
> >>>> -        bitfield!(@field_accessors $vis $name $storage {
> >>>> +        ::kernel::bitfield!(@field_accessors $vis $name $storage {
> >>>>            $(
> >>>>                $hi:$lo $field as $type
> >>>>                $(?=> $try_into_type)?
> >>>> @@ -139,8 +140,8 @@ fn from(val: $name) -> $storage {
> >>>>            ;
> >>>>            )*
> >>>>        });
> >>>> -        bitfield!(@debug $name { $($field;)* });
> >>>> -        bitfield!(@default $name { $($field;)* });
> >>>> +        ::kernel::bitfield!(@debug $name { $($field;)* });
> >>>> +        ::kernel::bitfield!(@default $name { $($field;)* });
> >>>>    };
> >>>>
> >>>>    // Defines all the field getter/setter methods for `$name`.
> >>>> @@ -155,13 +156,13 @@ fn from(val: $name) -> $storage {
> >>>>        }
> >>>>    ) => {
> >>>>        $(
> >>>> -            bitfield!(@check_field_bounds $hi:$lo $field as $type);
> >>>> +            ::kernel::bitfield!(@check_field_bounds $hi:$lo $field as $type);
> >>>>        )*
> >>>>
> >>>>        #[allow(dead_code)]
> >>>>        impl $name {
> >>>>            $(
> >>>> -            bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field
> as $type
> >>>> +            ::kernel::bitfield!(@field_accessor $vis $name $storage,
> $hi:$lo $field as $type
> >>>>                $(?=> $try_into_type)?
> >>>>                $(=> $into_type)?
> >>>>                $(, $comment)?
> >>>> @@ -198,7 +199,7 @@ impl $name {
> >>>>        @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt
> $field:ident as bool => $into_type:ty
> >>>>            $(, $comment:literal)?;
> >>>>    ) => {
> >>>> -        bitfield!(
> >>>> +        ::kernel::bitfield!(
> >>>>            @leaf_accessor $vis $name $storage, $hi:$lo $field
> >>>>            { |f| <$into_type>::from(if f != 0 { true } else { false }) }
> >>>>            $into_type => $into_type $(, $comment)?;
> >>>> @@ -209,7 +210,7 @@ impl $name {
> >>>>    (
> >>>>        @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt
> $field:ident as bool $(, $comment:literal)?;
> >>>>    ) => {
> >>>> -        bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as
> bool => bool $(, $comment)?;);
> >>>> +        ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo
> $field as bool => bool $(, $comment)?;);
> >>>>    };
> >>>>
> >>>>    // Catches the `?=>` syntax for non-boolean fields.
> >>>> @@ -217,7 +218,7 @@ impl $name {
> >>>>        @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt
> $field:ident as $type:tt ?=> $try_into_type:ty
> >>>>            $(, $comment:literal)?;
> >>>>    ) => {
> >>>> -        bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
> >>>> +        ::kernel::bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
> >>>>            { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
> >>>>            ::core::result::Result<
> >>>>                $try_into_type,
> >>>> @@ -231,7 +232,7 @@ impl $name {
> >>>>        @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt
> $field:ident as $type:tt => $into_type:ty
> >>>>            $(, $comment:literal)?;
> >>>>    ) => {
> >>>> -        bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
> >>>> +        ::kernel::bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
> >>>>            { |f| <$into_type>::from(f as $type) } $into_type => $into_type
> $(, $comment)?;);
> >>>>    };
> >>>>
> >>>> @@ -240,7 +241,7 @@ impl $name {
> >>>>        @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt
> $field:ident as $type:tt
> >>>>            $(, $comment:literal)?;
> >>>>    ) => {
> >>>> -        bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as
> $type => $type $(, $comment)?;);
> >>>> +        ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo
> $field as $type => $type $(, $comment)?;);
> >>>>    };
> >>>>
> >>>>    // Generates the accessor methods for a single field.
> >>>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> >>>> index 03b467722b86..a79b603604b1 100644
> >>>> --- a/rust/kernel/io.rs
> >>>> +++ b/rust/kernel/io.rs
> >>>> @@ -8,6 +8,7 @@
> >>>> use crate::{bindings, build_assert, ffi::c_void};
> >>>>
> >>>> pub mod mem;
> >>>> +pub mod register;
> >>>> pub mod resource;
> >>>>
> >>>> pub use resource::Resource;
> >>>> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/rust/kernel/io/register.rs
> >>>> similarity index 93%
> >>>> rename from drivers/gpu/nova-core/regs/macros.rs
> >>>> rename to rust/kernel/io/register.rs
> >>>> index c0a5194e8d97..c24d956f122f 100644
> >>>> --- a/drivers/gpu/nova-core/regs/macros.rs
> >>>> +++ b/rust/kernel/io/register.rs
> >>>> @@ -17,7 +17,8 @@
> >>>> /// The `T` generic argument is used to distinguish which base to use, in
> case a type provides
> >>>> /// several bases. It is given to the `register!` macro to restrict the use
> of the register to
> >>>> /// implementors of this particular variant.
> >>>> -pub(crate) trait RegisterBase<T> {
> >>>> +pub trait RegisterBase<T> {
> >>>> +    /// The base address for the register.
> >>>>    const BASE: usize;
> >>>> }
> >>>>
> >>>> @@ -26,7 +27,7 @@ pub(crate) trait RegisterBase<T> {
> >>>> ///
> >>>> /// Example:
> >>>> ///
> >>>> -/// ```no_run
> >>>> +/// ```ignore
> >>>> /// register!(BOOT_0 @ 0x00000100, "Basic revision information about the GPU" {
> >>>> ///    3:0     minor_revision as u8, "Minor revision of the chip";
> >>>> ///    7:4     major_revision as u8, "Major revision of the chip";
> >>>> @@ -39,7 +40,7 @@ pub(crate) trait RegisterBase<T> {
> >>>> /// significant bits of the register. Each field can be accessed and
> modified using accessor
> >>>> /// methods:
> >>>> ///
> >>>> -/// ```no_run
> >>>> +/// ```ignore
> >>>> /// // Read from the register's defined offset (0x100).
> >>>> /// let boot0 = BOOT_0::read(&bar);
> >>>> /// pr_info!("chip revision: {}.{}", boot0.major_revision(),
> boot0.minor_revision());
> >>>> @@ -61,7 +62,7 @@ pub(crate) trait RegisterBase<T> {
> >>>> /// It is also possible to create a alias register by using the `=> ALIAS`
> syntax. This is useful
> >>>> /// for cases where a register's interpretation depends on the context:
> >>>> ///
> >>>> -/// ```no_run
> >>>> +/// ```ignore
> >>>> /// register!(SCRATCH @ 0x00000200, "Scratch register" {
> >>>> ///    31:0     value as u32, "Raw value";
> >>>> /// });
> >>>> @@ -111,7 +112,7 @@ pub(crate) trait RegisterBase<T> {
> >>>> /// this register needs to implement `RegisterBase<Base>`. Here is the
> above example translated
> >>>> /// into code:
> >>>> ///
> >>>> -/// ```no_run
> >>>> +/// ```ignore
> >>>> /// // Type used to identify the base.
> >>>> /// pub(crate) struct CpuCtlBase;
> >>>> ///
> >>>> @@ -162,7 +163,7 @@ pub(crate) trait RegisterBase<T> {
> >>>> /// compile-time or runtime bound checking. Simply define their address as
> `Address[Size]`, and add
> >>>> /// an `idx` parameter to their `read`, `write` and `alter` methods:
> >>>> ///
> >>>> -/// ```no_run
> >>>> +/// ```ignore
> >>>> /// # fn no_run() -> Result<(), Error> {
> >>>> /// # fn get_scratch_idx() -> usize {
> >>>> /// #   0x15
> >>>> @@ -211,7 +212,7 @@ pub(crate) trait RegisterBase<T> {
> >>>> /// Combining the two features described in the sections above, arrays of
> registers accessible from
> >>>> /// a base can also be defined:
> >>>> ///
> >>>> -/// ```no_run
> >>>> +/// ```ignore
> >>>> /// # fn no_run() -> Result<(), Error> {
> >>>> /// # fn get_scratch_idx() -> usize {
> >>>> /// #   0x15
> >>>> @@ -273,28 +274,29 @@ pub(crate) trait RegisterBase<T> {
> >>>> /// # Ok(())
> >>>> /// # }
> >>>> /// ```
> >>>> +#[macro_export]
> >>>> macro_rules! register {
> >>>>    // Creates a register at a fixed offset of the MMIO space.
> >>>>    ($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* }
> ) => {
> >>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>>        register!(@io_fixed $name @ $offset);
> >>>>    };
> >>>>
> >>>>    // Creates an alias register of fixed offset register `alias` with its
> own fields.
> >>>>    ($name:ident => $alias:ident $(, $comment:literal)? { $($fields:tt)* } )
> => {
> >>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>>        register!(@io_fixed $name @ $alias::OFFSET);
> >>>>    };
> >>>>
> >>>>    // Creates a register at a relative offset from a base address provider.
> >>>>    ($name:ident @ $base:ty [ $offset:literal ] $(, $comment:literal)? {
> $($fields:tt)* } ) => {
> >>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>>        register!(@io_relative $name @ $base [ $offset ]);
> >>>>    };
> >>>>
> >>>>    // Creates an alias register of relative offset register `alias` with
> its own fields.
> >>>>    ($name:ident => $base:ty [ $alias:ident ] $(, $comment:literal)? {
> $($fields:tt)* }) => {
> >>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>>        register!(@io_relative $name @ $base [ $alias::OFFSET ]);
> >>>>    };
> >>>>
> >>>> @@ -305,7 +307,7 @@ macro_rules! register {
> >>>>        }
> >>>>    ) => {
> >>>>        static_assert!(::core::mem::size_of::<u32>() <= $stride);
> >>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>>        register!(@io_array $name @ $offset [ $size ; $stride ]);
> >>>>    };
> >>>>
> >>>> @@ -326,7 +328,7 @@ macro_rules! register {
> >>>>            $(, $comment:literal)? { $($fields:tt)* }
> >>>>    ) => {
> >>>>        static_assert!(::core::mem::size_of::<u32>() <= $stride);
> >>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>>        register!(@io_relative_array $name @ $base [ $offset [ $size ;
> $stride ] ]);
> >>>>    };
> >>>>
> >>>> @@ -348,7 +350,7 @@ macro_rules! register {
> >>>>        }
> >>>>    ) => {
> >>>>        static_assert!($idx < $alias::SIZE);
> >>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>>        register!(@io_relative $name @ $base [ $alias::OFFSET + $idx *
> $alias::STRIDE ] );
> >>>>    };
> >>>>
> >>>> @@ -357,7 +359,7 @@ macro_rules! register {
> >>>>    // to avoid it being interpreted in place of the relative register array
> alias rule.
> >>>>    ($name:ident => $alias:ident [ $idx:expr ] $(, $comment:literal)? {
> $($fields:tt)* }) => {
> >>>>        static_assert!($idx < $alias::SIZE);
> >>>> -        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>> +        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
> $($fields)* } );
> >>>>        register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );
> >>>>    };
> >>>>
> >>>> @@ -414,12 +416,12 @@ pub(crate) fn read<const SIZE: usize, T, B>(
> >>>>                base: &B,
> >>>>            ) -> Self where
> >>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >>>> -                B: crate::regs::macros::RegisterBase<$base>,
> >>>> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>>>            {
> >>>>                const OFFSET: usize = $name::OFFSET;
> >>>>
> >>>>                let value = io.read32(
> >>>> -                    <B as crate::regs::macros::RegisterBase<$base>>::BASE
> + OFFSET
> >>>> +                    <B as
> ::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
> >>>>                );
> >>>>
> >>>>                Self(value)
> >>>> @@ -435,13 +437,13 @@ pub(crate) fn write<const SIZE: usize, T, B>(
> >>>>                base: &B,
> >>>>            ) where
> >>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >>>> -                B: crate::regs::macros::RegisterBase<$base>,
> >>>> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>>>            {
> >>>>                const OFFSET: usize = $name::OFFSET;
> >>>>
> >>>>                io.write32(
> >>>>                    self.0,
> >>>> -                    <B as crate::regs::macros::RegisterBase<$base>>::BASE
> + OFFSET
> >>>> +                    <B as
> ::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
> >>>>                );
> >>>>            }
> >>>>
> >>>> @@ -455,7 +457,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
> >>>>                f: F,
> >>>>            ) where
> >>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >>>> -                B: crate::regs::macros::RegisterBase<$base>,
> >>>> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>>>                F: ::core::ops::FnOnce(Self) -> Self,
> >>>>            {
> >>>>                let reg = f(Self::read(io, base));
> >>>> @@ -600,11 +602,11 @@ pub(crate) fn read<const SIZE: usize, T, B>(
> >>>>                idx: usize,
> >>>>            ) -> Self where
> >>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >>>> -                B: crate::regs::macros::RegisterBase<$base>,
> >>>> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>>>            {
> >>>>                build_assert!(idx < Self::SIZE);
> >>>>
> >>>> -                let offset = <B as
> crate::regs::macros::RegisterBase<$base>>::BASE +
> >>>> +                let offset = <B as
> ::kernel::io::register::RegisterBase<$base>>::BASE +
> >>>>                    Self::OFFSET + (idx * Self::STRIDE);
> >>>>                let value = io.read32(offset);
> >>>>
> >>>> @@ -622,11 +624,11 @@ pub(crate) fn write<const SIZE: usize, T, B>(
> >>>>                idx: usize
> >>>>            ) where
> >>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >>>> -                B: crate::regs::macros::RegisterBase<$base>,
> >>>> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>>>            {
> >>>>                build_assert!(idx < Self::SIZE);
> >>>>
> >>>> -                let offset = <B as
> crate::regs::macros::RegisterBase<$base>>::BASE +
> >>>> +                let offset = <B as
> ::kernel::io::register::RegisterBase<$base>>::BASE +
> >>>>                    Self::OFFSET + (idx * Self::STRIDE);
> >>>>
> >>>>                io.write32(self.0, offset);
> >>>> @@ -643,7 +645,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
> >>>>                f: F,
> >>>>            ) where
> >>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >>>> -                B: crate::regs::macros::RegisterBase<$base>,
> >>>> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>>>                F: ::core::ops::FnOnce(Self) -> Self,
> >>>>            {
> >>>>                let reg = f(Self::read(io, base, idx));
> >>>> @@ -662,7 +664,7 @@ pub(crate) fn try_read<const SIZE: usize, T, B>(
> >>>>                idx: usize,
> >>>>            ) -> ::kernel::error::Result<Self> where
> >>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >>>> -                B: crate::regs::macros::RegisterBase<$base>,
> >>>> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>>>            {
> >>>>                if idx < Self::SIZE {
> >>>>                    Ok(Self::read(io, base, idx))
> >>>> @@ -684,7 +686,7 @@ pub(crate) fn try_write<const SIZE: usize, T, B>(
> >>>>                idx: usize,
> >>>>            ) -> ::kernel::error::Result where
> >>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >>>> -                B: crate::regs::macros::RegisterBase<$base>,
> >>>> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>>>            {
> >>>>                if idx < Self::SIZE {
> >>>>                    Ok(self.write(io, base, idx))
> >>>> @@ -707,7 +709,7 @@ pub(crate) fn try_alter<const SIZE: usize, T, B, F>(
> >>>>                f: F,
> >>>>            ) -> ::kernel::error::Result where
> >>>>                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> >>>> -                B: crate::regs::macros::RegisterBase<$base>,
> >>>> +                B: ::kernel::io::register::RegisterBase<$base>,
> >>>>                F: ::core::ops::FnOnce(Self) -> Self,
> >>>>            {
> >>>>                if idx < Self::SIZE {
> >>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> >>>> index fcffc3988a90..8f8260090c02 100644
> >>>> --- a/rust/kernel/lib.rs
> >>>> +++ b/rust/kernel/lib.rs
> >>>> @@ -63,6 +63,7 @@
> >>>> pub mod alloc;
> >>>> #[cfg(CONFIG_AUXILIARY_BUS)]
> >>>> pub mod auxiliary;
> >>>> +pub mod bitfield;
> >>>> pub mod bits;
> >>>> #[cfg(CONFIG_BLOCK)]
> >>>> pub mod block;
> >>>> --
> >>>> 2.34.1
> >>>>
> >>>>

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Danilo Krummrich 1 month, 2 weeks ago

On Mon Oct 27, 2025 at 10:06 AM CET, Beata Michalska wrote:
> It's more theoretical at this point, but there are drivers that do rely on
> information from either DT or ACPI tables for the base address and size of the
> MMIO region: anything that uses devm_platform_ioremap_resource() or
> devm_platform_ioremap_resource_byname() I guess.

Don't get confused, those are two different things: The size of the MMIO region
(or a PCI BAR) and the const SIZE generic in Io<SIZE> are two different things.

The former is the actual size of an MMIO region, whereas the latter is the
minimum size requested by a driver for proper operation.

For instance, let's assume your driver requests ten contiguous 32-bit registers
starting at offset zero of an MMIO region.

In this case you can call req.iomap_sized<0x28>(), because you know that your
driver is not able to properly work without an MMIO region with at least a width
of 0x28 bytes.

The actual size of the MMIO region returned by req.iomap_sized<0x28>() may
indeed be smaller or larger than that, depending on what is defined in the DT,
ACPI or PCI BAR.

If smaller than the const SIZE generic, the call to req.iomap_sized<0x28>() will
fail, otherwise it will be successful. The actual size of the MMIO region is not
influenced by the const SIZE generic.

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Beata Michalska 1 month, 2 weeks ago

On Mon, Oct 27, 2025 at 10:56:41AM +0100, Danilo Krummrich wrote:
> On Mon Oct 27, 2025 at 10:06 AM CET, Beata Michalska wrote:
> > It's more theoretical at this point, but there are drivers that do rely on
> > information from either DT or ACPI tables for the base address and size of the
> > MMIO region: anything that uses devm_platform_ioremap_resource() or
> > devm_platform_ioremap_resource_byname() I guess.
> 
> Don't get confused, those are two different things: The size of the MMIO region
> (or a PCI BAR) and the const SIZE generic in Io<SIZE> are two different things.
> 
> The former is the actual size of an MMIO region, whereas the latter is the
> minimum size requested by a driver for proper operation.
> 
> For instance, let's assume your driver requests ten contiguous 32-bit registers
> starting at offset zero of an MMIO region.
> 
> In this case you can call req.iomap_sized<0x28>(), because you know that your
> driver is not able to properly work without an MMIO region with at least a width
> of 0x28 bytes.
> 
> The actual size of the MMIO region returned by req.iomap_sized<0x28>() may
> indeed be smaller or larger than that, depending on what is defined in the DT,
> ACPI or PCI BAR.
> 
> If smaller than the const SIZE generic, the call to req.iomap_sized<0x28>() will
> fail, otherwise it will be successful. The actual size of the MMIO region is not
> influenced by the const SIZE generic.
I appreciate the explanation.
I think my confusion here comes from the fact that I was assuming there is an
intent to accommodate different MMIO regions sizes for various device revisions,
and not expecting all drivers to explicitly call the iomap_sized in all cases.
My bad then.
Again, thanks for clarifying that.


---
Best Regards
Beata

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Joel Fernandes 1 month, 3 weeks ago


On 10/23/2025 5:47 PM, Joel Fernandes wrote:
>>>> Finally, for runtime values such as indexes, it could be useful to verify
>>>> once and then allow infallible reads/writes through some kind access token.
>>> Why? The verification is already done at compile-time AFAICS.
>> Well, that's the point. Those are runtime values, and as of now, the only
>> support for those is for arrays of registers when one, when using try_xxx
>> methods, ends up with check being performed each time the method is called.
>> Ah for this part of your email, you are referring to try accessors. For the
> fixed sizes regions at least, to avoid the runtime check, it will be possible to
> accept BoundedInt [1] in the future. That type actually came up for the exact
> same reason (keeping the checking light). This cleverly moves the checking to
> the caller side which could be done in a slow path. If the size of the IO region
> is fixed, then you don’t need to use try accessors at all if you use BoundedInt
> whenever we have it.

To clarify, BoundedInt is supposed to bound the values passed to the APIs, not
the address. Perhaps we can add additional types for the offsets as well.

thanks,

 - Joel

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Danilo Krummrich 1 month, 3 weeks ago

On Thu Oct 23, 2025 at 3:55 PM CEST, Beata Michalska wrote:
> I guess everything that would use IoRequest::iomap(self), which generates
> Io<SIZE=0> which is a game over for the macro.

To be honest, we may just want to remove this API. A driver that requires a zero
sized MMIO region with only optional runtime derived access offsets seems a bit
too exotic.

That doesn't mean that occasionally we'll have drivers needing runtime offsets,
so eventually we want to support that with the register!() macro, but it's not
*that* urgent.

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Dirk Behme 2 months, 1 week ago

On 03/10/2025 17:47, Joel Fernandes wrote:
> Out of broad need for the register and bitfield macros in Rust, move
> them out of nova into the kernel crate. Several usecases need them (Nova
> is already using these and Tyr developers said they need them).
> 
> bitfield moved into kernel crate - defines bitfields in Rust.
> register moved into io module - defines hardware registers and accessors.
> 
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> ---
>   drivers/gpu/nova-core/falcon.rs               |  2 +-
>   drivers/gpu/nova-core/falcon/gsp.rs           |  4 +-
>   drivers/gpu/nova-core/falcon/sec2.rs          |  2 +-
>   drivers/gpu/nova-core/nova_core.rs            |  3 -
>   drivers/gpu/nova-core/regs.rs                 |  6 +-
>   .../gpu/nova-core => rust/kernel}/bitfield.rs | 27 ++++-----
>   rust/kernel/io.rs                             |  1 +
>   .../macros.rs => rust/kernel/io/register.rs   | 58 ++++++++++---------
>   rust/kernel/lib.rs                            |  1 +
>   9 files changed, 54 insertions(+), 50 deletions(-)
>   rename {drivers/gpu/nova-core => rust/kernel}/bitfield.rs (91%)
>   rename drivers/gpu/nova-core/regs/macros.rs => rust/kernel/io/register.rs (93%)
> 
...
> index c0a5194e8d97..c24d956f122f 100644
> --- a/drivers/gpu/nova-core/regs/macros.rs
> +++ b/rust/kernel/io/register.rs

Assuming that register.rs is supposed to become the "generic" way to 
access hardware registers I started to have a look to it. Some weeks 
back testing interrupts I used some quite simple timer with 4 registers 
[1]. Now, thinking about converting it to register!() I have three 
understanding / usage questions:

* At the moment register!() is for 32-bit registers, only? So it can't 
be used for my example having 8-bit and 16-bit registers as well?

* In my example I used io.try_write*() and io.try_read*() for the 
register access. What is the relationship between these and the 
register!() accessors (e.g. from the examples BOOT_0::read(&bar);)? Will 
both stay? When to use which?

Note: Due to the file move obviously not the full content of the "new" 
file register.rs is shown in this patch. Therefore, let me try it this 
way, citing from register.rs:

-- cut --
...
/// This defines a `BOOT_0` type which can be read or written from 
offset `0x100` of an `Io`
/// region
....
/// ```ignore
/// // Read from the register's defined offset (0x100).
/// let boot0 = BOOT_0::read(&bar);
-- cut --

* What is "&bar" in this example? Is it the `Io` region the explanation 
talks about?

Thanks!

Dirk

[1] 
https://lore.kernel.org/rust-for-linux/dd34e5f4-5027-4096-9f32-129c8a067d0a@de.bosch.com/

The registers:

const TSTR: usize =  0x4; //  8 Bit register
const TCOR: usize =  0x8; // 32 Bit register
const TCNT: usize =  0xC; // 32 Bit register
const TCR:  usize = 0x10; // 16 Bit register

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Danilo Krummrich 2 months, 1 week ago

On Thu Oct 9, 2025 at 8:59 AM CEST, Dirk Behme wrote:
> Assuming that register.rs is supposed to become the "generic" way to 
> access hardware registers I started to have a look to it. Some weeks 
> back testing interrupts I used some quite simple timer with 4 registers 
> [1]. Now, thinking about converting it to register!() I have three 
> understanding / usage questions:
>
> * At the moment register!() is for 32-bit registers, only? So it can't 
> be used for my example having 8-bit and 16-bit registers as well?

Yes, currently the register!() macro always generates a 32-bit register type
(mainly because nova-core did not need anything else). However, this will of
course be generalized (which should be pretty straight forward).

Having a brief look at the TMU datasheet it looks like you should be able to
treat TSTR and TCR as 32-bit registers without any issues for testing the
register!() macro today. I.e. you can just define it as:

	register!(TSTR @ 0x04, "Timer Start Register" {
	    2:2    str2 as bool, "Specifies whether TCNT2 is operated or stopped.";
	    1:1    str1 as bool, "Specifies whether TCNT1 is operated or stopped.";
	    0:0    str0 as bool, "Specifies whether TCNT0 is operated or stopped.";
	});

Same for TCR.

> * In my example I used io.try_write*() and io.try_read*() for the 
> register access. What is the relationship between these and the 
> register!() accessors (e.g. from the examples BOOT_0::read(&bar);)? Will 
> both stay? When to use which?

The try_*() variants should only be used of the offset of the register is not
known at compile time.

If it is known at compile time (such as in your case) you should use the
infallible variants that perform a range check at compile time.

For this to work you need to specify the minimum MMIO range your driver expects,
i.e. instead of

	let iomem = Arc::pin_init(request.iomap()?, GFP_KERNEL)?;

you call

	let iomem = Arc::pin_init(request.iomap_sized::<TMU_MMIO_SIZE>()?, GFP_KERNEL)?;

where TMU_MMIO_SIZE is the minimum MMIO region size your driver is able to
operate on. In your case this would be 0x12, given that TCR has a width of
16-bit. However, if you treat it as 32-bit register it would be 0x14.

Even without the register!() macro this would be a huge improvement. For
instance, your IRQ handler from [1] can do

	let tcr = io.read16_relaxed(TCR);
	if tcr & (0x1 << 8) != 0 {
	    io.write16_relaxed(tcr & !(0x1 << 8), TCR);
	}

instead of

	let tcr = io.try_read16_relaxed(TCR).unwrap_or(0);
	if tcr & (0x1 << 8) != 0 {
	    io.try_write16_relaxed(tcr & !(0x1 << 8), TCR).unwrap_or(());
	}

And with the register macro it becomes

	let tcr = TCR::read(io);
	if tcr.underflow() {
		tcr.set_underflow(false);
		tcr.write(io);
	}

Note that you can also extend the generated TCR type accordingly, for instance:

	impl TCR {
	    fn handle_underflow<const SIZE: usize, T>(io: &T)
	    where
	        T: Deref<Target = Io<SIZE>>,
	    {
	        let this = Self::read(io);
	        if this.underflow() {
	            this.set_underflow(false);
	            this.write(io);
	        }
	    }
	}

and then from your IRQ handler you can just call

	TCR::handle_underflow();

> Note: Due to the file move obviously not the full content of the "new" 
> file register.rs is shown in this patch. Therefore, let me try it this 
> way, citing from register.rs:
>
> -- cut --
> ...
> /// This defines a `BOOT_0` type which can be read or written from 
> offset `0x100` of an `Io`
> /// region
> ....
> /// ```ignore
> /// // Read from the register's defined offset (0x100).
> /// let boot0 = BOOT_0::read(&bar);
> -- cut --
>
> * What is "&bar" in this example? Is it the `Io` region the explanation 
> talks about?

Yes, it's the I/O backend (a pci::Bar in this case). However, we should probably
use a more generic name in the examples.

> [1] 
> https://lore.kernel.org/rust-for-linux/dd34e5f4-5027-4096-9f32-129c8a067d0a@de.bosch.com/
>
> The registers:
>
> const TSTR: usize =  0x4; //  8 Bit register
> const TCOR: usize =  0x8; // 32 Bit register
> const TCNT: usize =  0xC; // 32 Bit register
> const TCR:  usize = 0x10; // 16 Bit register

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Dirk Behme 2 months, 1 week ago

On 09/10/2025 13:16, Danilo Krummrich wrote:
> On Thu Oct 9, 2025 at 8:59 AM CEST, Dirk Behme wrote:
>> Assuming that register.rs is supposed to become the "generic" way to
>> access hardware registers I started to have a look to it. Some weeks
>> back testing interrupts I used some quite simple timer with 4 registers
>> [1]. Now, thinking about converting it to register!() I have three
>> understanding / usage questions:
>>
>> * At the moment register!() is for 32-bit registers, only? So it can't
>> be used for my example having 8-bit and 16-bit registers as well?
> 
> Yes, currently the register!() macro always generates a 32-bit register type
> (mainly because nova-core did not need anything else). However, this will of
> course be generalized (which should be pretty straight forward).
> 
> Having a brief look at the TMU datasheet it looks like you should be able to
> treat TSTR and TCR as 32-bit registers without any issues for testing the
> register!() macro today. I.e. you can just define it as:
> 
> 	register!(TSTR @ 0x04, "Timer Start Register" {
> 	    2:2    str2 as bool, "Specifies whether TCNT2 is operated or stopped.";
> 	    1:1    str1 as bool, "Specifies whether TCNT1 is operated or stopped.";
> 	    0:0    str0 as bool, "Specifies whether TCNT0 is operated or stopped.";
> 	});
> 
> Same for TCR.
> 
>> * In my example I used io.try_write*() and io.try_read*() for the
>> register access. What is the relationship between these and the
>> register!() accessors (e.g. from the examples BOOT_0::read(&bar);)? Will
>> both stay? When to use which?
> 
> The try_*() variants should only be used of the offset of the register is not
> known at compile time.
> 
> If it is known at compile time (such as in your case) you should use the
> infallible variants that perform a range check at compile time.
> 
> For this to work you need to specify the minimum MMIO range your driver expects,
> i.e. instead of
> 
> 	let iomem = Arc::pin_init(request.iomap()?, GFP_KERNEL)?;
> 
> you call
> 
> 	let iomem = Arc::pin_init(request.iomap_sized::<TMU_MMIO_SIZE>()?, GFP_KERNEL)?;
> 
> where TMU_MMIO_SIZE is the minimum MMIO region size your driver is able to
> operate on. In your case this would be 0x12, given that TCR has a width of
> 16-bit. However, if you treat it as 32-bit register it would be 0x14.
> 
> Even without the register!() macro this would be a huge improvement. For
> instance, your IRQ handler from [1] can do
> 
> 	let tcr = io.read16_relaxed(TCR);
> 	if tcr & (0x1 << 8) != 0 {
> 	    io.write16_relaxed(tcr & !(0x1 << 8), TCR);
> 	}
> 
> instead of
> 
> 	let tcr = io.try_read16_relaxed(TCR).unwrap_or(0);
> 	if tcr & (0x1 << 8) != 0 {
> 	    io.try_write16_relaxed(tcr & !(0x1 << 8), TCR).unwrap_or(());
> 	}
> 
> And with the register macro it becomes
> 
> 	let tcr = TCR::read(io);
> 	if tcr.underflow() {
> 		tcr.set_underflow(false);
> 		tcr.write(io);
> 	}
> 
> Note that you can also extend the generated TCR type accordingly, for instance:
> 
> 	impl TCR {
> 	    fn handle_underflow<const SIZE: usize, T>(io: &T)
> 	    where
> 	        T: Deref<Target = Io<SIZE>>,
> 	    {
> 	        let this = Self::read(io);
> 	        if this.underflow() {
> 	            this.set_underflow(false);
> 	            this.write(io);
> 	        }
> 	    }
> 	}
> 
> and then from your IRQ handler you can just call
> 
> 	TCR::handle_underflow();


Many thanks! I think such verbose explanation helps a lot!


>> Note: Due to the file move obviously not the full content of the "new"
>> file register.rs is shown in this patch. Therefore, let me try it this
>> way, citing from register.rs:
>>
>> -- cut --
>> ...
>> /// This defines a `BOOT_0` type which can be read or written from
>> offset `0x100` of an `Io`
>> /// region
>> ....
>> /// ```ignore
>> /// // Read from the register's defined offset (0x100).
>> /// let boot0 = BOOT_0::read(&bar);
>> -- cut --
>>
>> * What is "&bar" in this example? Is it the `Io` region the explanation
>> talks about?
> 
> Yes, it's the I/O backend (a pci::Bar in this case). However, we should probably
> use a more generic name in the examples.

Intuitively for this example I was thinking about &io ;)

Thanks again!

Dirk

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Alexandre Courbot 2 months, 1 week ago

On Thu Oct 9, 2025 at 8:16 PM JST, Danilo Krummrich wrote:
> On Thu Oct 9, 2025 at 8:59 AM CEST, Dirk Behme wrote:
>> Assuming that register.rs is supposed to become the "generic" way to 
>> access hardware registers I started to have a look to it. Some weeks 
>> back testing interrupts I used some quite simple timer with 4 registers 
>> [1]. Now, thinking about converting it to register!() I have three 
>> understanding / usage questions:
>>
>> * At the moment register!() is for 32-bit registers, only? So it can't 
>> be used for my example having 8-bit and 16-bit registers as well?
>
> Yes, currently the register!() macro always generates a 32-bit register type
> (mainly because nova-core did not need anything else). However, this will of
> course be generalized (which should be pretty straight forward).
>
> Having a brief look at the TMU datasheet it looks like you should be able to
> treat TSTR and TCR as 32-bit registers without any issues for testing the
> register!() macro today. I.e. you can just define it as:
>
> 	register!(TSTR @ 0x04, "Timer Start Register" {
> 	    2:2    str2 as bool, "Specifies whether TCNT2 is operated or stopped.";
> 	    1:1    str1 as bool, "Specifies whether TCNT1 is operated or stopped.";
> 	    0:0    str0 as bool, "Specifies whether TCNT0 is operated or stopped.";
> 	});
>
> Same for TCR.

Patch 2 of this series actually adds support for 16 and 8 bit register
storage.

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Dirk Behme 1 month, 2 weeks ago

On 09.10.25 13:28, Alexandre Courbot wrote:
> On Thu Oct 9, 2025 at 8:16 PM JST, Danilo Krummrich wrote:
>> On Thu Oct 9, 2025 at 8:59 AM CEST, Dirk Behme wrote:
>>> Assuming that register.rs is supposed to become the "generic" way to 
>>> access hardware registers I started to have a look to it. Some weeks 
>>> back testing interrupts I used some quite simple timer with 4 registers 
>>> [1]. Now, thinking about converting it to register!() I have three 
>>> understanding / usage questions:
>>>
>>> * At the moment register!() is for 32-bit registers, only? So it can't 
>>> be used for my example having 8-bit and 16-bit registers as well?
>>
>> Yes, currently the register!() macro always generates a 32-bit register type
>> (mainly because nova-core did not need anything else). However, this will of
>> course be generalized (which should be pretty straight forward).
>>
>> Having a brief look at the TMU datasheet it looks like you should be able to
>> treat TSTR and TCR as 32-bit registers without any issues for testing the
>> register!() macro today. I.e. you can just define it as:
>>
>> 	register!(TSTR @ 0x04, "Timer Start Register" {
>> 	    2:2    str2 as bool, "Specifies whether TCNT2 is operated or stopped.";
>> 	    1:1    str1 as bool, "Specifies whether TCNT1 is operated or stopped.";
>> 	    0:0    str0 as bool, "Specifies whether TCNT0 is operated or stopped.";
>> 	});
>>
>> Same for TCR.
> 
> Patch 2 of this series actually adds support for 16 and 8 bit register
> storage.

Hmm, how to use that with the register!() macro? I mean patch 2 adds
support for different storage widths for *bitfields*. But looking at
patch 4 [2] it looks like *register!()* still uses $name(u32)? With
that it looks like that the register!() macro still just supports 32
bit registers? Or what have I missed?

What I'm looking for is a way to specify if a register is 8, 16 or 32
bit. Using the example from above something like

register!(TSTR<u8> @ ....

Thanks

Dirk

[2]

https://lore.kernel.org/rust-for-linux/20251003154748.1687160-5-joelagnelf@nvidia.com/

...
+#[macro_export]
 macro_rules! register {
     // Creates a register at a fixed offset of the MMIO space.
     ($name:ident @ $offset:literal $(, $comment:literal)? {
$($fields:tt)* } ) => {
-        bitfield!(pub(crate) struct $name(u32) $(, $comment)? {
$($fields)* } );
+        ::kernel::bitfield!(pub(crate) struct $name(u32) $(,
$comment)? { $($fields)* } );
         register!(@io_fixed $name @ $offset);
     };
...

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Alexandre Courbot 1 month, 2 weeks ago

On Sun Nov 2, 2025 at 3:51 AM JST, Dirk Behme wrote:
> On 09.10.25 13:28, Alexandre Courbot wrote:
>> On Thu Oct 9, 2025 at 8:16 PM JST, Danilo Krummrich wrote:
>>> On Thu Oct 9, 2025 at 8:59 AM CEST, Dirk Behme wrote:
>>>> Assuming that register.rs is supposed to become the "generic" way to 
>>>> access hardware registers I started to have a look to it. Some weeks 
>>>> back testing interrupts I used some quite simple timer with 4 registers 
>>>> [1]. Now, thinking about converting it to register!() I have three 
>>>> understanding / usage questions:
>>>>
>>>> * At the moment register!() is for 32-bit registers, only? So it can't 
>>>> be used for my example having 8-bit and 16-bit registers as well?
>>>
>>> Yes, currently the register!() macro always generates a 32-bit register type
>>> (mainly because nova-core did not need anything else). However, this will of
>>> course be generalized (which should be pretty straight forward).
>>>
>>> Having a brief look at the TMU datasheet it looks like you should be able to
>>> treat TSTR and TCR as 32-bit registers without any issues for testing the
>>> register!() macro today. I.e. you can just define it as:
>>>
>>> 	register!(TSTR @ 0x04, "Timer Start Register" {
>>> 	    2:2    str2 as bool, "Specifies whether TCNT2 is operated or stopped.";
>>> 	    1:1    str1 as bool, "Specifies whether TCNT1 is operated or stopped.";
>>> 	    0:0    str0 as bool, "Specifies whether TCNT0 is operated or stopped.";
>>> 	});
>>>
>>> Same for TCR.
>> 
>> Patch 2 of this series actually adds support for 16 and 8 bit register
>> storage.
>
> Hmm, how to use that with the register!() macro? I mean patch 2 adds
> support for different storage widths for *bitfields*. But looking at
> patch 4 [2] it looks like *register!()* still uses $name(u32)? With
> that it looks like that the register!() macro still just supports 32
> bit registers? Or what have I missed?
>
> What I'm looking for is a way to specify if a register is 8, 16 or 32
> bit. Using the example from above something like
>
> register!(TSTR<u8> @ ....

Errr indeed, you are correct. The `register` macro's syntax has not been
updated to take advantage of `bitfield`'s storage types, and `u32` is
still hardcoded as of this series.

This looks like an oversight - a register is basically a bitfield with
some I/O, so making it support storage types should be trivial. I guess
this hasn't been done yet because Nova is the only user so far, and we
don't need/want to explicitly specify a type for each register since
they are invariably `u32`.

But it wouldn't look good to change the syntax of `register` after
moving it out, so I agree this should take place before the move. The
same applies to the visiblity feature.

One way to avoid a update all the declarations so far would be to give
Nova its own `register` macro that invokes the one in `kernel` with
the relevant parameters hardcoded.

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Dirk Behme 1 month ago

On 02/11/2025 04:00, Alexandre Courbot wrote:
> On Sun Nov 2, 2025 at 3:51 AM JST, Dirk Behme wrote:
>> On 09.10.25 13:28, Alexandre Courbot wrote:
>>> On Thu Oct 9, 2025 at 8:16 PM JST, Danilo Krummrich wrote:
>>>> On Thu Oct 9, 2025 at 8:59 AM CEST, Dirk Behme wrote:
>>>>> Assuming that register.rs is supposed to become the "generic" way to
>>>>> access hardware registers I started to have a look to it. Some weeks
>>>>> back testing interrupts I used some quite simple timer with 4 registers
>>>>> [1]. Now, thinking about converting it to register!() I have three
>>>>> understanding / usage questions:
>>>>>
>>>>> * At the moment register!() is for 32-bit registers, only? So it can't
>>>>> be used for my example having 8-bit and 16-bit registers as well?
>>>>
>>>> Yes, currently the register!() macro always generates a 32-bit register type
>>>> (mainly because nova-core did not need anything else). However, this will of
>>>> course be generalized (which should be pretty straight forward).
>>>>
>>>> Having a brief look at the TMU datasheet it looks like you should be able to
>>>> treat TSTR and TCR as 32-bit registers without any issues for testing the
>>>> register!() macro today. I.e. you can just define it as:
>>>>
>>>> 	register!(TSTR @ 0x04, "Timer Start Register" {
>>>> 	    2:2    str2 as bool, "Specifies whether TCNT2 is operated or stopped.";
>>>> 	    1:1    str1 as bool, "Specifies whether TCNT1 is operated or stopped.";
>>>> 	    0:0    str0 as bool, "Specifies whether TCNT0 is operated or stopped.";
>>>> 	});
>>>>
>>>> Same for TCR.
>>>
>>> Patch 2 of this series actually adds support for 16 and 8 bit register
>>> storage.
>>
>> Hmm, how to use that with the register!() macro? I mean patch 2 adds
>> support for different storage widths for *bitfields*. But looking at
>> patch 4 [2] it looks like *register!()* still uses $name(u32)? With
>> that it looks like that the register!() macro still just supports 32
>> bit registers? Or what have I missed?
>>
>> What I'm looking for is a way to specify if a register is 8, 16 or 32
>> bit. Using the example from above something like
>>
>> register!(TSTR<u8> @ ....
> 
> Errr indeed, you are correct. The `register` macro's syntax has not been
> updated to take advantage of `bitfield`'s storage types, and `u32` is
> still hardcoded as of this series.
> 
> This looks like an oversight - a register is basically a bitfield with
> some I/O, so making it support storage types should be trivial. I guess
> this hasn't been done yet because Nova is the only user so far, and we
> don't need/want to explicitly specify a type for each register since
> they are invariably `u32`.
> 
> But it wouldn't look good to change the syntax of `register` after
> moving it out, so I agree this should take place before the move. The
> same applies to the visiblity feature.
> 
> One way to avoid a update all the declarations so far would be to give
> Nova its own `register` macro that invokes the one in `kernel` with
> the relevant parameters hardcoded.


Just fyi, hacking something like [1] below does work for my (very 
limited) use case. And it defaults `register!` without type to <u32>.

Thanks!

Dirk

[1]

--- a/rust/kernel/io/register.rs
+++ b/rust/kernel/io/register.rs
@@ -276,33 +276,38 @@ pub trait RegisterBase<T> {
  /// ```
  #[macro_export]
  macro_rules! register {
-    // Creates a register at a fixed offset of the MMIO space.
+    // Creates a register at a fixed offset of the MMIO space, defaults 
to u32 if no type is specified.
      ($name:ident @ $offset:literal $(, $comment:literal)? { 
$($fields:tt)* } ) => {
-        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? 
{ $($fields)* } );
-        register!(@io_fixed $name @ $offset);
+        register!($name<u32> @ $offset $(, $comment)? { $($fields)* });
+    };
+
+    // Creates a register at a fixed offset of the MMIO space, explicit 
type required.
+    ($name:ident<$ty:ty> @ $offset:literal $(, $comment:literal)? { 
$($fields:tt)* } ) => {
+        ::kernel::bitfield!(pub(crate) struct $name($ty) $(, $comment)? 
{ $($fields)* } );
+        register!(@io_fixed<$ty> $name @ $offset);
      };

      // Creates an alias register of fixed offset register `alias` with 
its own fields.
-    ($name:ident => $alias:ident $(, $comment:literal)? { 
$($fields:tt)* } ) => {
-        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? 
{ $($fields)* } );
-        register!(@io_fixed $name @ $alias::OFFSET);
+    ($name:ident<$ty:ty> => $alias:ident $(, $comment:literal)? { 
$($fields:tt)* } ) => {
+        ::kernel::bitfield!(pub(crate) struct $name($ty) $(, $comment)? 
{ $($fields)* } );
+        register!(@io_fixed<$ty> $name @ $alias::OFFSET);
      };

      // Creates a register at a relative offset from a base address 
provider.
-    ($name:ident @ $base:ty [ $offset:literal ] $(, $comment:literal)? 
{ $($fields:tt)* } ) => {
-        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? 
{ $($fields)* } );
-        register!(@io_relative $name @ $base [ $offset ]);
+    ($name:ident<$ty:ty> @ $base:ty [ $offset:literal ] $(, 
$comment:literal)? { $($fields:tt)* } ) => {
+        ::kernel::bitfield!(pub(crate) struct $name($ty) $(, $comment)? 
{ $($fields)* } );
+        register!(@io_relative<$ty> $name @ $base [ $offset ]);
      };

      // Creates an alias register of relative offset register `alias` 
with its own fields.
-    ($name:ident => $base:ty [ $alias:ident ] $(, $comment:literal)? { 
$($fields:tt)* }) => {
-        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? 
{ $($fields)* } );
-        register!(@io_relative $name @ $base [ $alias::OFFSET ]);
+    ($name:ident<$ty:ty> => $base:ty [ $alias:ident ] $(, 
$comment:literal)? { $($fields:tt)* }) => {
+        ::kernel::bitfield!(pub(crate) struct $name($ty) $(, $comment)? 
{ $($fields)* } );
+        register!(@io_relative<$ty> $name @ $base [ $alias::OFFSET ]);
      };

-    // Creates an array of registers at a fixed offset of the MMIO space.
+    // Creates an array of registers at a fixed offset of the MMIO 
space. (u32 only for now)
      (
-        $name:ident @ $offset:literal [ $size:expr ; $stride:expr ] $(, 
$comment:literal)? {
+        $name:ident<u32> @ $offset:literal [ $size:expr ; $stride:expr 
] $(, $comment:literal)? {
              $($fields:tt)*
          }
      ) => {
@@ -311,20 +316,20 @@ macro_rules! register {
          register!(@io_array $name @ $offset [ $size ; $stride ]);
      };

-    // Shortcut for contiguous array of registers (stride == size of 
element).
+    // Shortcut for contiguous array of registers (stride == size of 
element). (u32 only for now)
      (
-        $name:ident @ $offset:literal [ $size:expr ] $(, 
$comment:literal)? {
+        $name:ident<u32> @ $offset:literal [ $size:expr ] $(, 
$comment:literal)? {
              $($fields:tt)*
          }
      ) => {
-        register!($name @ $offset [ $size ; 
::core::mem::size_of::<u32>() ] $(, $comment)? {
+        register!($name<u32> @ $offset [ $size ; 
::core::mem::size_of::<u32>() ] $(, $comment)? {
              $($fields)*
          } );
      };

-    // Creates an array of registers at a relative offset from a base 
address provider.
+    // Creates an array of registers at a relative offset from a base 
address provider. (u32 only for now)
      (
-        $name:ident @ $base:ty [ $offset:literal [ $size:expr ; 
$stride:expr ] ]
+        $name:ident<u32> @ $base:ty [ $offset:literal [ $size:expr ; 
$stride:expr ] ]
              $(, $comment:literal)? { $($fields:tt)* }
      ) => {
          static_assert!(::core::mem::size_of::<u32>() <= $stride);
@@ -332,20 +337,19 @@ macro_rules! register {
          register!(@io_relative_array $name @ $base [ $offset [ $size ; 
$stride ] ]);
      };

-    // Shortcut for contiguous array of relative registers (stride == 
size of element).
+    // Shortcut for contiguous array of relative registers (stride == 
size of element). (u32 only for now)
      (
-        $name:ident @ $base:ty [ $offset:literal [ $size:expr ] ] $(, 
$comment:literal)? {
+        $name:ident<u32> @ $base:ty [ $offset:literal [ $size:expr ] ] 
$(, $comment:literal)? {
              $($fields:tt)*
          }
      ) => {
-        register!($name @ $base [ $offset [ $size ; 
::core::mem::size_of::<u32>() ] ]
+        register!($name<u32> @ $base:ty [ $offset:literal [ $size:expr 
; ::core::mem::size_of::<u32>() ] ]
              $(, $comment)? { $($fields)* } );
      };

-    // Creates an alias of register `idx` of relative array of 
registers `alias` with its own
-    // fields.
+    // Creates an alias of register `idx` of relative array of 
registers `alias` with its own fields. (u32 only for now)
      (
-        $name:ident => $base:ty [ $alias:ident [ $idx:expr ] ] $(, 
$comment:literal)? {
+        $name:ident<u32> => $base:ty [ $alias:ident [ $idx:expr ] ] $(, 
$comment:literal)? {
              $($fields:tt)*
          }
      ) => {
@@ -354,17 +358,15 @@ macro_rules! register {
          register!(@io_relative $name @ $base [ $alias::OFFSET + $idx * 
$alias::STRIDE ] );
      };

-    // Creates an alias of register `idx` of array of registers `alias` 
with its own fields.
-    // This rule belongs to the (non-relative) register arrays set, but 
needs to be put last
-    // to avoid it being interpreted in place of the relative register 
array alias rule.
-    ($name:ident => $alias:ident [ $idx:expr ] $(, $comment:literal)? { 
$($fields:tt)* }) => {
+    // Creates an alias of register `idx` of array of registers `alias` 
with its own fields. (u32 only for now)
+    ($name:ident<u32> => $alias:ident [ $idx:expr ] $(, 
$comment:literal)? { $($fields:tt)* }) => {
          static_assert!($idx < $alias::SIZE);
          ::kernel::bitfield!(pub(crate) struct $name(u32) $(, 
$comment)? { $($fields)* } );
          register!(@io_fixed $name @ $alias::OFFSET + $idx * 
$alias::STRIDE );
      };

-    // Generates the IO accessors for a fixed offset register.
-    (@io_fixed $name:ident @ $offset:expr) => {
+    // Generates the IO accessors for a fixed offset register, using 
the type parameter.
+    (@io_fixed<$ty:ty> $name:ident @ $offset:expr) => {
          #[allow(dead_code)]
          impl $name {
              pub(crate) const OFFSET: usize = $offset;
@@ -374,7 +376,15 @@ impl $name {
              pub(crate) fn read<const SIZE: usize, T>(io: &T) -> Self where
                  T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
              {
-                Self(io.read32($offset))
+                Self(
+                    if core::any::TypeId::of::<$ty>() == 
core::any::TypeId::of::<u8>() {
+                        io.read8($offset) as $ty
+                    } else if core::any::TypeId::of::<$ty>() == 
core::any::TypeId::of::<u16>() {
+                        io.read16($offset) as $ty
+                    } else {
+                        io.read32($offset) as $ty
+                    }
+                )
              }

              /// Write the value contained in `self` to the register 
address in `io`.
@@ -382,7 +392,13 @@ pub(crate) fn read<const SIZE: usize, T>(io: &T) -> 
Self where
              pub(crate) fn write<const SIZE: usize, T>(self, io: &T) where
                  T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
              {
-                io.write32(self.0, $offset)
+                if core::any::TypeId::of::<$ty>() == 
core::any::TypeId::of::<u8>() {
+                    io.write8(self.0 as u8, $offset)
+                } else if core::any::TypeId::of::<$ty>() == 
core::any::TypeId::of::<u16>() {
+                    io.write16(self.0 as u16, $offset)
+                } else {
+                    io.write32(self.0 as u32, $offset)
+                }
              }

              /// Read the register from its address in `io` and run `f` 
on its value to obtain a new
@@ -401,8 +417,8 @@ pub(crate) fn alter<const SIZE: usize, T, F>(
          }
      };

-    // Generates the IO accessors for a relative offset register.
-    (@io_relative $name:ident @ $base:ty [ $offset:expr ]) => {
+    // Generates the IO accessors for a relative offset register, using 
the type parameter.
+    (@io_relative<$ty:ty> $name:ident @ $base:ty [ $offset:expr ]) => {
          #[allow(dead_code)]
          impl $name {
              pub(crate) const OFFSET: usize = $offset;
@@ -420,9 +436,7 @@ pub(crate) fn read<const SIZE: usize, T, B>(
              {
                  const OFFSET: usize = $name::OFFSET;

-                let value = io.read32(
-                    <B as 
::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
-                );
+                let value = io.read::<$ty>(<B as 
::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET);

                  Self(value)
              }
@@ -441,10 +455,7 @@ pub(crate) fn write<const SIZE: usize, T, B>(
              {
                  const OFFSET: usize = $name::OFFSET;

-                io.write32(
-                    self.0,
-                    <B as 
::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
-                );
+                io.write::<$ty>(self.0, <B as 
::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET);
              }

              /// Read the register from `io`, using the base address 
provided by `base` and adding
-- 
2.48.0

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Danilo Krummrich 2 months, 1 week ago

On 10/9/25 1:28 PM, Alexandre Courbot wrote:
> On Thu Oct 9, 2025 at 8:16 PM JST, Danilo Krummrich wrote:
>> On Thu Oct 9, 2025 at 8:59 AM CEST, Dirk Behme wrote:
>>> Assuming that register.rs is supposed to become the "generic" way to 
>>> access hardware registers I started to have a look to it. Some weeks 
>>> back testing interrupts I used some quite simple timer with 4 registers 
>>> [1]. Now, thinking about converting it to register!() I have three 
>>> understanding / usage questions:
>>>
>>> * At the moment register!() is for 32-bit registers, only? So it can't 
>>> be used for my example having 8-bit and 16-bit registers as well?
>>
>> Yes, currently the register!() macro always generates a 32-bit register type
>> (mainly because nova-core did not need anything else). However, this will of
>> course be generalized (which should be pretty straight forward).
>>
>> Having a brief look at the TMU datasheet it looks like you should be able to
>> treat TSTR and TCR as 32-bit registers without any issues for testing the
>> register!() macro today. I.e. you can just define it as:
>>
>> 	register!(TSTR @ 0x04, "Timer Start Register" {
>> 	    2:2    str2 as bool, "Specifies whether TCNT2 is operated or stopped.";
>> 	    1:1    str1 as bool, "Specifies whether TCNT1 is operated or stopped.";
>> 	    0:0    str0 as bool, "Specifies whether TCNT0 is operated or stopped.";
>> 	});
>>
>> Same for TCR.
> 
> Patch 2 of this series actually adds support for 16 and 8 bit register
> storage.

Heh! I knew I saw a patch for this already somewhere, seems like I missed the
forest for the trees. :)

Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

Posted by Alexandre Courbot 2 months, 1 week ago

On Sat Oct 4, 2025 at 12:47 AM JST, Joel Fernandes wrote:
> Out of broad need for the register and bitfield macros in Rust, move
> them out of nova into the kernel crate. Several usecases need them (Nova
> is already using these and Tyr developers said they need them).
>
> bitfield moved into kernel crate - defines bitfields in Rust.
> register moved into io module - defines hardware registers and accessors.
>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> ---
>  drivers/gpu/nova-core/falcon.rs               |  2 +-
>  drivers/gpu/nova-core/falcon/gsp.rs           |  4 +-
>  drivers/gpu/nova-core/falcon/sec2.rs          |  2 +-
>  drivers/gpu/nova-core/nova_core.rs            |  3 -
>  drivers/gpu/nova-core/regs.rs                 |  6 +-
>  .../gpu/nova-core => rust/kernel}/bitfield.rs | 27 ++++-----
>  rust/kernel/io.rs                             |  1 +
>  .../macros.rs => rust/kernel/io/register.rs   | 58 ++++++++++---------
>  rust/kernel/lib.rs                            |  1 +
>  9 files changed, 54 insertions(+), 50 deletions(-)
>  rename {drivers/gpu/nova-core => rust/kernel}/bitfield.rs (91%)
>  rename drivers/gpu/nova-core/regs/macros.rs => rust/kernel/io/register.rs (93%)
>
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index 37e6298195e4..a15fa98c8614 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -6,6 +6,7 @@
>  use hal::FalconHal;
>  use kernel::device;
>  use kernel::dma::DmaAddress;
> +use kernel::io::register::RegisterBase;
>  use kernel::prelude::*;
>  use kernel::sync::aref::ARef;
>  use kernel::time::Delta;
> @@ -14,7 +15,6 @@
>  use crate::driver::Bar0;
>  use crate::gpu::Chipset;
>  use crate::regs;
> -use crate::regs::macros::RegisterBase;
>  use crate::util;
>  
>  pub(crate) mod gsp;
> diff --git a/drivers/gpu/nova-core/falcon/gsp.rs b/drivers/gpu/nova-core/falcon/gsp.rs
> index f17599cb49fa..cd4960e997c8 100644
> --- a/drivers/gpu/nova-core/falcon/gsp.rs
> +++ b/drivers/gpu/nova-core/falcon/gsp.rs
> @@ -1,9 +1,11 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> +use kernel::io::register::RegisterBase;
> +
>  use crate::{
>      driver::Bar0,
>      falcon::{Falcon, FalconEngine, PFalcon2Base, PFalconBase},
> -    regs::{self, macros::RegisterBase},
> +    regs::self,

`rustfmt` was not happy with this one:

--- a/drivers/gpu/nova-core/falcon/gsp.rs
+++ b/drivers/gpu/nova-core/falcon/gsp.rs
@@ -5,7 +5,7 @@
 use crate::{
     driver::Bar0,
     falcon::{Falcon, FalconEngine, PFalcon2Base, PFalconBase},
-    regs::self,
+    regs,
 };

No need to resend just for this though.

[PATCH v6 1/5] nova-core: bitfield: Move bitfield-specific code from register! into new macro
[PATCH v6 2/5] nova-core: bitfield: Add support for different storage widths
[PATCH v6 3/5] nova-core: bitfield: Add support for custom visiblity
[PATCH v6 4/5] rust: Move register and bitfield macros out of Nova
[PATCH v6 5/5] rust: bitfield: Add KUNIT tests for bitfield