[PATCH v7.1 0/4] bitfield initial refactor within nova-core (RESEND)

[PATCH v7.1 0/4] bitfield initial refactor within nova-core (RESEND)

[Joel Fernandes]

(Resending due to some commit message mistakes (missing SOB etc). Thanks!).

These patches implement the initial refactoring and few improvements to the
register and bitfield macros. Rebased on drm-rust-next.

Main difference from the previous series [1] is dropped the moving out of
nova-core pending BoundedInt changes:
https://lore.kernel.org/all/20251003154748.1687160-1-joelagnelf@nvidia.com/
Other than that, added tags, resolved conflict with kernel::fmt changes and
rebased on drm-rust-next.

Alexandre Courbot (1):
  gpu: nova-core: register: use field type for Into implementation

Joel Fernandes (3):
  gpu: nova-core: bitfield: Move bitfield-specific code from register!
    into new macro
  gpu: nova-core: bitfield: Add support for different storage widths
  gpu: nova-core: bitfield: Add support for custom visiblity

 drivers/gpu/nova-core/bitfield.rs    | 333 +++++++++++++++++++++++++++
 drivers/gpu/nova-core/falcon.rs      |  38 ++-
 drivers/gpu/nova-core/nova_core.rs   |   3 +
 drivers/gpu/nova-core/regs/macros.rs | 259 +--------------------
 4 files changed, 373 insertions(+), 260 deletions(-)
 create mode 100644 drivers/gpu/nova-core/bitfield.rs


base-commit: 1d5cffebd930d61588c32198f85fbe541ab97b8f
-- 
2.34.1

[PATCH v7.1 1/4] gpu: nova-core: register: use field type for Into implementation

[Joel Fernandes]

From: Alexandre Courbot <acourbot@nvidia.com>

The getter method of a field works with the field type, but its setter
expects the type of the register. This leads to an asymmetry in the
From/Into implementations required for a field with a dedicated type.
For instance, a field declared as

    pub struct ControlReg(u32) {
        3:0 mode as u8 ?=> Mode;
        ...
    }

currently requires the following implementations:

    impl TryFrom<u8> for Mode {
      ...
    }

    impl From<Mode> for u32 {
      ...
    }

Change this so the `From<Mode>` now needs to be implemented for `u8`,
i.e. the primitive type of the field. This is more consistent, and will
become a requirement once we start using the TryFrom/Into derive macros
to implement these automatically.

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
 drivers/gpu/nova-core/falcon.rs      | 38 ++++++++++++++++++++--------
 drivers/gpu/nova-core/regs/macros.rs | 10 ++++----
 2 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 37e6298195e4..3f505b870601 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -22,11 +22,11 @@
 pub(crate) mod sec2;
 
 // TODO[FPRI]: Replace with `ToPrimitive`.
-macro_rules! impl_from_enum_to_u32 {
+macro_rules! impl_from_enum_to_u8 {
     ($enum_type:ty) => {
-        impl From<$enum_type> for u32 {
+        impl From<$enum_type> for u8 {
             fn from(value: $enum_type) -> Self {
-                value as u32
+                value as u8
             }
         }
     };
@@ -46,7 +46,7 @@ pub(crate) enum FalconCoreRev {
     Rev6 = 6,
     Rev7 = 7,
 }
-impl_from_enum_to_u32!(FalconCoreRev);
+impl_from_enum_to_u8!(FalconCoreRev);
 
 // TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconCoreRev {
@@ -81,7 +81,7 @@ pub(crate) enum FalconCoreRevSubversion {
     Subversion2 = 2,
     Subversion3 = 3,
 }
-impl_from_enum_to_u32!(FalconCoreRevSubversion);
+impl_from_enum_to_u8!(FalconCoreRevSubversion);
 
 // TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconCoreRevSubversion {
@@ -125,7 +125,7 @@ pub(crate) enum FalconSecurityModel {
     /// Also known as High-Secure, Privilege Level 3 or PL3.
     Heavy = 3,
 }
-impl_from_enum_to_u32!(FalconSecurityModel);
+impl_from_enum_to_u8!(FalconSecurityModel);
 
 // TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconSecurityModel {
@@ -157,7 +157,7 @@ pub(crate) enum FalconModSelAlgo {
     #[default]
     Rsa3k = 1,
 }
-impl_from_enum_to_u32!(FalconModSelAlgo);
+impl_from_enum_to_u8!(FalconModSelAlgo);
 
 // TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconModSelAlgo {
@@ -179,7 +179,7 @@ pub(crate) enum DmaTrfCmdSize {
     #[default]
     Size256B = 0x6,
 }
-impl_from_enum_to_u32!(DmaTrfCmdSize);
+impl_from_enum_to_u8!(DmaTrfCmdSize);
 
 // TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for DmaTrfCmdSize {
@@ -202,7 +202,6 @@ pub(crate) enum PeregrineCoreSelect {
     /// RISC-V core is active.
     Riscv = 1,
 }
-impl_from_enum_to_u32!(PeregrineCoreSelect);
 
 impl From<bool> for PeregrineCoreSelect {
     fn from(value: bool) -> Self {
@@ -213,6 +212,15 @@ fn from(value: bool) -> Self {
     }
 }
 
+impl From<PeregrineCoreSelect> for bool {
+    fn from(value: PeregrineCoreSelect) -> Self {
+        match value {
+            PeregrineCoreSelect::Falcon => false,
+            PeregrineCoreSelect::Riscv => true,
+        }
+    }
+}
+
 /// Different types of memory present in a falcon core.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub(crate) enum FalconMem {
@@ -236,7 +244,7 @@ pub(crate) enum FalconFbifTarget {
     /// Non-coherent system memory (System DRAM).
     NoncoherentSysmem = 2,
 }
-impl_from_enum_to_u32!(FalconFbifTarget);
+impl_from_enum_to_u8!(FalconFbifTarget);
 
 // TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconFbifTarget {
@@ -263,7 +271,6 @@ pub(crate) enum FalconFbifMemType {
     /// Physical memory addresses.
     Physical = 1,
 }
-impl_from_enum_to_u32!(FalconFbifMemType);
 
 /// Conversion from a single-bit register field.
 impl From<bool> for FalconFbifMemType {
@@ -275,6 +282,15 @@ fn from(value: bool) -> Self {
     }
 }
 
+impl From<FalconFbifMemType> for bool {
+    fn from(value: FalconFbifMemType) -> Self {
+        match value {
+            FalconFbifMemType::Virtual => false,
+            FalconFbifMemType::Physical => true,
+        }
+    }
+}
+
 /// Type used to represent the `PFALCON` registers address base for a given falcon engine.
 pub(crate) struct PFalconBase(());
 
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index 8058e1696df9..1c54a4533822 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -482,7 +482,7 @@ impl $name {
         register!(
             @leaf_accessor $name $hi:$lo $field
             { |f| <$into_type>::from(if f != 0 { true } else { false }) }
-            $into_type => $into_type $(, $comment)?;
+            bool $into_type => $into_type $(, $comment)?;
         );
     };
 
@@ -499,7 +499,7 @@ impl $name {
             $(, $comment:literal)?;
     ) => {
         register!(@leaf_accessor $name $hi:$lo $field
-            { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
+            { |f| <$try_into_type>::try_from(f as $type) } $type $try_into_type =>
             ::core::result::Result<
                 $try_into_type,
                 <$try_into_type as ::core::convert::TryFrom<$type>>::Error
@@ -513,7 +513,7 @@ impl $name {
             $(, $comment:literal)?;
     ) => {
         register!(@leaf_accessor $name $hi:$lo $field
-            { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
+            { |f| <$into_type>::from(f as $type) } $type $into_type => $into_type $(, $comment)?;);
     };
 
     // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
@@ -527,7 +527,7 @@ impl $name {
     // Generates the accessor methods for a single field.
     (
         @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
-            { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
+            { $process:expr } $prim_type:tt $to_type:ty => $res_type:ty $(, $comment:literal)?;
     ) => {
         ::kernel::macros::paste!(
         const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
@@ -559,7 +559,7 @@ pub(crate) fn $field(self) -> $res_type {
         pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
             const MASK: u32 = $name::[<$field:upper _MASK>];
             const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
-            let value = (u32::from(value) << SHIFT) & MASK;
+            let value = (u32::from($prim_type::from(value)) << SHIFT) & MASK;
             self.0 = (self.0 & !MASK) | value;
 
             self
-- 
2.34.1

[PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[Joel Fernandes]

Move the bitfield-specific code from the register macro into a new macro
called bitfield. This will be used to define structs with bitfields,
similar to C language.

Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Reviewed-by: Edwin Peer <epeer@nvidia.com>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
 drivers/gpu/nova-core/bitfield.rs    | 319 +++++++++++++++++++++++++++
 drivers/gpu/nova-core/nova_core.rs   |   3 +
 drivers/gpu/nova-core/regs/macros.rs | 259 +---------------------
 3 files changed, 332 insertions(+), 249 deletions(-)
 create mode 100644 drivers/gpu/nova-core/bitfield.rs

diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
new file mode 100644
index 000000000000..98ccb1bd3289
--- /dev/null
+++ b/drivers/gpu/nova-core/bitfield.rs
@@ -0,0 +1,319 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Bitfield library for Rust structures
+//!
+//! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro.
+
+/// Defines a struct with accessors to access bits within an inner unsigned integer.
+///
+/// # Syntax
+///
+/// ```rust
+/// use nova_core::bitfield;
+///
+/// #[derive(Debug, Clone, Copy, Default)]
+/// enum Mode {
+///     #[default]
+///     Low = 0,
+///     High = 1,
+///     Auto = 2,
+/// }
+///
+/// impl TryFrom<u8> for Mode {
+///     type Error = u8;
+///     fn try_from(value: u8) -> Result<Self, Self::Error> {
+///         match value {
+///             0 => Ok(Mode::Low),
+///             1 => Ok(Mode::High),
+///             2 => Ok(Mode::Auto),
+///             _ => Err(value),
+///         }
+///     }
+/// }
+///
+/// impl From<Mode> for u8 {
+///     fn from(mode: Mode) -> u8 {
+///         mode as u8
+///     }
+/// }
+///
+/// #[derive(Debug, Clone, Copy, Default)]
+/// enum State {
+///     #[default]
+///     Inactive = 0,
+///     Active = 1,
+/// }
+///
+/// impl From<bool> for State {
+///     fn from(value: bool) -> Self {
+///         if value { State::Active } else { State::Inactive }
+///     }
+/// }
+///
+/// impl From<State> for bool {
+///     fn from(state: State) -> bool {
+///         match state {
+///             State::Inactive => false,
+///             State::Active => true,
+///         }
+///     }
+/// }
+///
+/// bitfield! {
+///     struct ControlReg {
+///         3:0 mode as u8 ?=> Mode;
+///         7:7 state as bool => State;
+///     }
+/// }
+/// ```
+///
+/// This generates a struct with:
+/// - Field accessors: `mode()`, `state()`, etc.
+/// - Field setters: `set_mode()`, `set_state()`, etc. (supports chaining with builder pattern).
+/// - Debug and Default implementations.
+///
+/// Fields are defined as follows:
+///
+/// - `as <type>` simply returns the field value casted to <type>, typically `u32`, `u16`, `u8` or
+///   `bool`. Note that `bool` fields must have a range of 1 bit.
+/// - `as <type> => <into_type>` calls `<into_type>`'s `From::<<type>>` implementation and returns
+///   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_rules! bitfield {
+    // Main entry point - defines the bitfield struct with fields
+    (struct $name:ident $(, $comment:literal)? { $($fields:tt)* }) => {
+        bitfield!(@core $name $(, $comment)? { $($fields)* });
+    };
+
+    // All rules below are helpers.
+
+    // Defines the wrapper `$name` type, as well as its relevant implementations (`Debug`,
+    // `Default`, `BitOr`, and conversion to the value type) and field accessor methods.
+    (@core $name:ident $(, $comment:literal)? { $($fields:tt)* }) => {
+        $(
+        #[doc=$comment]
+        )?
+        #[repr(transparent)]
+        #[derive(Clone, Copy)]
+        pub(crate) struct $name(u32);
+
+        impl ::core::ops::BitOr for $name {
+            type Output = Self;
+
+            fn bitor(self, rhs: Self) -> Self::Output {
+                Self(self.0 | rhs.0)
+            }
+        }
+
+        impl ::core::convert::From<$name> for u32 {
+            fn from(val: $name) -> u32 {
+                val.0
+            }
+        }
+
+        bitfield!(@fields_dispatcher $name { $($fields)* });
+    };
+
+    // Captures the fields and passes them to all the implementers that require field information.
+    //
+    // Used to simplify the matching rules for implementers, so they don't need to match the entire
+    // complex fields rule even though they only make use of part of it.
+    (@fields_dispatcher $name:ident {
+        $($hi:tt:$lo:tt $field:ident as $type:tt
+            $(?=> $try_into_type:ty)?
+            $(=> $into_type:ty)?
+            $(, $comment:literal)?
+        ;
+        )*
+    }
+    ) => {
+        bitfield!(@field_accessors $name {
+            $(
+                $hi:$lo $field as $type
+                $(?=> $try_into_type)?
+                $(=> $into_type)?
+                $(, $comment)?
+            ;
+            )*
+        });
+        bitfield!(@debug $name { $($field;)* });
+        bitfield!(@default $name { $($field;)* });
+    };
+
+    // Defines all the field getter/setter methods for `$name`.
+    (
+        @field_accessors $name:ident {
+        $($hi:tt:$lo:tt $field:ident as $type:tt
+            $(?=> $try_into_type:ty)?
+            $(=> $into_type:ty)?
+            $(, $comment:literal)?
+        ;
+        )*
+        }
+    ) => {
+        $(
+            bitfield!(@check_field_bounds $hi:$lo $field as $type);
+        )*
+
+        #[allow(dead_code)]
+        impl $name {
+            $(
+            bitfield!(@field_accessor $name $hi:$lo $field as $type
+                $(?=> $try_into_type)?
+                $(=> $into_type)?
+                $(, $comment)?
+                ;
+            );
+            )*
+        }
+    };
+
+    // Boolean fields must have `$hi == $lo`.
+    (@check_field_bounds $hi:tt:$lo:tt $field:ident as bool) => {
+        #[allow(clippy::eq_op)]
+        const _: () = {
+            ::kernel::build_assert!(
+                $hi == $lo,
+                concat!("boolean field `", stringify!($field), "` covers more than one bit")
+            );
+        };
+    };
+
+    // Non-boolean fields must have `$hi >= $lo`.
+    (@check_field_bounds $hi:tt:$lo:tt $field:ident as $type:tt) => {
+        #[allow(clippy::eq_op)]
+        const _: () = {
+            ::kernel::build_assert!(
+                $hi >= $lo,
+                concat!("field `", stringify!($field), "`'s MSB is smaller than its LSB")
+            );
+        };
+    };
+
+    // Catches fields defined as `bool` and convert them into a boolean value.
+    (
+        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
+            $(, $comment:literal)?;
+    ) => {
+        bitfield!(
+            @leaf_accessor $name $hi:$lo $field
+            { |f| <$into_type>::from(if f != 0 { true } else { false }) }
+            bool $into_type => $into_type $(, $comment)?;
+        );
+    };
+
+    // Shortcut for fields defined as `bool` without the `=>` syntax.
+    (
+        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
+    ) => {
+        bitfield!(@field_accessor $name $hi:$lo $field as bool => bool $(, $comment)?;);
+    };
+
+    // Catches the `?=>` syntax for non-boolean fields.
+    (
+        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
+            $(, $comment:literal)?;
+    ) => {
+        bitfield!(@leaf_accessor $name $hi:$lo $field
+            { |f| <$try_into_type>::try_from(f as $type) } $type $try_into_type =>
+            ::core::result::Result<
+                $try_into_type,
+                <$try_into_type as ::core::convert::TryFrom<$type>>::Error
+            >
+            $(, $comment)?;);
+    };
+
+    // Catches the `=>` syntax for non-boolean fields.
+    (
+        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
+            $(, $comment:literal)?;
+    ) => {
+        bitfield!(@leaf_accessor $name $hi:$lo $field
+            { |f| <$into_type>::from(f as $type) } $type $into_type => $into_type $(, $comment)?;);
+    };
+
+    // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
+    (
+        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt
+            $(, $comment:literal)?;
+    ) => {
+        bitfield!(@field_accessor $name $hi:$lo $field as $type => $type $(, $comment)?;);
+    };
+
+    // Generates the accessor methods for a single field.
+    (
+        @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
+            { $process:expr } $prim_type:tt $to_type:ty => $res_type:ty $(, $comment:literal)?;
+    ) => {
+        ::kernel::macros::paste!(
+        const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
+        const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
+        const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
+        );
+
+        $(
+        #[doc="Returns the value of this field:"]
+        #[doc=$comment]
+        )?
+        #[inline(always)]
+        pub(crate) fn $field(self) -> $res_type {
+            ::kernel::macros::paste!(
+            const MASK: u32 = $name::[<$field:upper _MASK>];
+            const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
+            );
+            let field = ((self.0 & MASK) >> SHIFT);
+
+            $process(field)
+        }
+
+        ::kernel::macros::paste!(
+        $(
+        #[doc="Sets the value of this field:"]
+        #[doc=$comment]
+        )?
+        #[inline(always)]
+        pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
+            const MASK: u32 = $name::[<$field:upper _MASK>];
+            const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
+            let value = (u32::from($prim_type::from(value)) << SHIFT) & MASK;
+            self.0 = (self.0 & !MASK) | value;
+
+            self
+        }
+        );
+    };
+
+    // Generates the `Debug` implementation for `$name`.
+    (@debug $name:ident { $($field:ident;)* }) => {
+        impl ::kernel::fmt::Debug for $name {
+            fn fmt(&self, f: &mut ::kernel::fmt::Formatter<'_>) -> ::kernel::fmt::Result {
+                f.debug_struct(stringify!($name))
+                    .field("<raw>", &::kernel::prelude::fmt!("{:#x}", &self.0))
+                $(
+                    .field(stringify!($field), &self.$field())
+                )*
+                    .finish()
+            }
+        }
+    };
+
+    // Generates the `Default` implementation for `$name`.
+    (@default $name:ident { $($field:ident;)* }) => {
+        /// Returns a value for the bitfield where all fields are set to their default value.
+        impl ::core::default::Default for $name {
+            fn default() -> Self {
+                #[allow(unused_mut)]
+                let mut value = Self(Default::default());
+
+                ::kernel::macros::paste!(
+                $(
+                value.[<set_ $field>](Default::default());
+                )*
+                );
+
+                value
+            }
+        }
+    };
+}
diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
index fffcaee2249f..112277c7921e 100644
--- a/drivers/gpu/nova-core/nova_core.rs
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -2,6 +2,9 @@
 
 //! Nova Core GPU Driver
 
+#[macro_use]
+mod bitfield;
+
 mod dma;
 mod driver;
 mod falcon;
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index 1c54a4533822..945d15a2c529 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -8,7 +8,8 @@
 //!
 //! The `register!` macro in this module provides an intuitive and readable syntax for defining a
 //! dedicated type for each register. Each such type comes with its own field accessors that can
-//! return an error if a field's value is invalid.
+//! return an error if a field's value is invalid. Please look at the [`bitfield`] macro for the
+//! complete syntax of fields definitions.
 
 /// Trait providing a base address to be added to the offset of a relative register to obtain
 /// its actual offset.
@@ -54,15 +55,6 @@ pub(crate) trait RegisterBase<T> {
 /// BOOT_0::alter(&bar, |r| r.set_major_revision(3).set_minor_revision(10));
 /// ```
 ///
-/// Fields are defined as follows:
-///
-/// - `as <type>` simply returns the field value casted to <type>, typically `u32`, `u16`, `u8` or
-///   `bool`. Note that `bool` fields must have a range of 1 bit.
-/// - `as <type> => <into_type>` calls `<into_type>`'s `From::<<type>>` implementation and returns
-///   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.
-///
 /// The documentation strings are optional. If present, they will be added to the type's
 /// definition, or the field getter and setter methods they are attached to.
 ///
@@ -284,25 +276,25 @@ pub(crate) trait RegisterBase<T> {
 macro_rules! register {
     // Creates a register at a fixed offset of the MMIO space.
     ($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* } ) => {
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitfield!(struct $name $(, $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)* } ) => {
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitfield!(struct $name $(, $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)* } ) => {
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitfield!(struct $name $(, $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)* }) => {
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitfield!(struct $name $(, $comment)? { $($fields)* } );
         register!(@io_relative $name @ $base [ $alias::OFFSET ]);
     };
 
@@ -313,7 +305,7 @@ macro_rules! register {
         }
     ) => {
         static_assert!(::core::mem::size_of::<u32>() <= $stride);
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitfield!(struct $name $(, $comment)? { $($fields)* } );
         register!(@io_array $name @ $offset [ $size ; $stride ]);
     };
 
@@ -334,7 +326,7 @@ macro_rules! register {
             $(, $comment:literal)? { $($fields:tt)* }
     ) => {
         static_assert!(::core::mem::size_of::<u32>() <= $stride);
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitfield!(struct $name $(, $comment)? { $($fields)* } );
         register!(@io_relative_array $name @ $base [ $offset [ $size ; $stride ] ]);
     };
 
@@ -356,7 +348,7 @@ macro_rules! register {
         }
     ) => {
         static_assert!($idx < $alias::SIZE);
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitfield!(struct $name $(, $comment)? { $($fields)* } );
         register!(@io_relative $name @ $base [ $alias::OFFSET + $idx * $alias::STRIDE ] );
     };
 
@@ -365,241 +357,10 @@ 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);
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitfield!(struct $name $(, $comment)? { $($fields)* } );
         register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );
     };
 
-    // All rules below are helpers.
-
-    // Defines the wrapper `$name` type, as well as its relevant implementations (`Debug`,
-    // `Default`, `BitOr`, and conversion to the value type) and field accessor methods.
-    (@core $name:ident $(, $comment:literal)? { $($fields:tt)* }) => {
-        $(
-        #[doc=$comment]
-        )?
-        #[repr(transparent)]
-        #[derive(Clone, Copy)]
-        pub(crate) struct $name(u32);
-
-        impl ::core::ops::BitOr for $name {
-            type Output = Self;
-
-            fn bitor(self, rhs: Self) -> Self::Output {
-                Self(self.0 | rhs.0)
-            }
-        }
-
-        impl ::core::convert::From<$name> for u32 {
-            fn from(reg: $name) -> u32 {
-                reg.0
-            }
-        }
-
-        register!(@fields_dispatcher $name { $($fields)* });
-    };
-
-    // Captures the fields and passes them to all the implementers that require field information.
-    //
-    // Used to simplify the matching rules for implementers, so they don't need to match the entire
-    // complex fields rule even though they only make use of part of it.
-    (@fields_dispatcher $name:ident {
-        $($hi:tt:$lo:tt $field:ident as $type:tt
-            $(?=> $try_into_type:ty)?
-            $(=> $into_type:ty)?
-            $(, $comment:literal)?
-        ;
-        )*
-    }
-    ) => {
-        register!(@field_accessors $name {
-            $(
-                $hi:$lo $field as $type
-                $(?=> $try_into_type)?
-                $(=> $into_type)?
-                $(, $comment)?
-            ;
-            )*
-        });
-        register!(@debug $name { $($field;)* });
-        register!(@default $name { $($field;)* });
-    };
-
-    // Defines all the field getter/methods methods for `$name`.
-    (
-        @field_accessors $name:ident {
-        $($hi:tt:$lo:tt $field:ident as $type:tt
-            $(?=> $try_into_type:ty)?
-            $(=> $into_type:ty)?
-            $(, $comment:literal)?
-        ;
-        )*
-        }
-    ) => {
-        $(
-            register!(@check_field_bounds $hi:$lo $field as $type);
-        )*
-
-        #[allow(dead_code)]
-        impl $name {
-            $(
-            register!(@field_accessor $name $hi:$lo $field as $type
-                $(?=> $try_into_type)?
-                $(=> $into_type)?
-                $(, $comment)?
-                ;
-            );
-            )*
-        }
-    };
-
-    // Boolean fields must have `$hi == $lo`.
-    (@check_field_bounds $hi:tt:$lo:tt $field:ident as bool) => {
-        #[allow(clippy::eq_op)]
-        const _: () = {
-            ::kernel::build_assert!(
-                $hi == $lo,
-                concat!("boolean field `", stringify!($field), "` covers more than one bit")
-            );
-        };
-    };
-
-    // Non-boolean fields must have `$hi >= $lo`.
-    (@check_field_bounds $hi:tt:$lo:tt $field:ident as $type:tt) => {
-        #[allow(clippy::eq_op)]
-        const _: () = {
-            ::kernel::build_assert!(
-                $hi >= $lo,
-                concat!("field `", stringify!($field), "`'s MSB is smaller than its LSB")
-            );
-        };
-    };
-
-    // Catches fields defined as `bool` and convert them into a boolean value.
-    (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
-            $(, $comment:literal)?;
-    ) => {
-        register!(
-            @leaf_accessor $name $hi:$lo $field
-            { |f| <$into_type>::from(if f != 0 { true } else { false }) }
-            bool $into_type => $into_type $(, $comment)?;
-        );
-    };
-
-    // Shortcut for fields defined as `bool` without the `=>` syntax.
-    (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
-    ) => {
-        register!(@field_accessor $name $hi:$lo $field as bool => bool $(, $comment)?;);
-    };
-
-    // Catches the `?=>` syntax for non-boolean fields.
-    (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
-            $(, $comment:literal)?;
-    ) => {
-        register!(@leaf_accessor $name $hi:$lo $field
-            { |f| <$try_into_type>::try_from(f as $type) } $type $try_into_type =>
-            ::core::result::Result<
-                $try_into_type,
-                <$try_into_type as ::core::convert::TryFrom<$type>>::Error
-            >
-            $(, $comment)?;);
-    };
-
-    // Catches the `=>` syntax for non-boolean fields.
-    (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
-            $(, $comment:literal)?;
-    ) => {
-        register!(@leaf_accessor $name $hi:$lo $field
-            { |f| <$into_type>::from(f as $type) } $type $into_type => $into_type $(, $comment)?;);
-    };
-
-    // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
-    (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt
-            $(, $comment:literal)?;
-    ) => {
-        register!(@field_accessor $name $hi:$lo $field as $type => $type $(, $comment)?;);
-    };
-
-    // Generates the accessor methods for a single field.
-    (
-        @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
-            { $process:expr } $prim_type:tt $to_type:ty => $res_type:ty $(, $comment:literal)?;
-    ) => {
-        ::kernel::macros::paste!(
-        const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
-        const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
-        const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
-        );
-
-        $(
-        #[doc="Returns the value of this field:"]
-        #[doc=$comment]
-        )?
-        #[inline(always)]
-        pub(crate) fn $field(self) -> $res_type {
-            ::kernel::macros::paste!(
-            const MASK: u32 = $name::[<$field:upper _MASK>];
-            const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
-            );
-            let field = ((self.0 & MASK) >> SHIFT);
-
-            $process(field)
-        }
-
-        ::kernel::macros::paste!(
-        $(
-        #[doc="Sets the value of this field:"]
-        #[doc=$comment]
-        )?
-        #[inline(always)]
-        pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
-            const MASK: u32 = $name::[<$field:upper _MASK>];
-            const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
-            let value = (u32::from($prim_type::from(value)) << SHIFT) & MASK;
-            self.0 = (self.0 & !MASK) | value;
-
-            self
-        }
-        );
-    };
-
-    // Generates the `Debug` implementation for `$name`.
-    (@debug $name:ident { $($field:ident;)* }) => {
-        impl ::kernel::fmt::Debug for $name {
-            fn fmt(&self, f: &mut ::kernel::fmt::Formatter<'_>) -> ::kernel::fmt::Result {
-                f.debug_struct(stringify!($name))
-                    .field("<raw>", &::kernel::prelude::fmt!("{:#x}", &self.0))
-                $(
-                    .field(stringify!($field), &self.$field())
-                )*
-                    .finish()
-            }
-        }
-    };
-
-    // Generates the `Default` implementation for `$name`.
-    (@default $name:ident { $($field:ident;)* }) => {
-        /// Returns a value for the register where all fields are set to their default value.
-        impl ::core::default::Default for $name {
-            fn default() -> Self {
-                #[allow(unused_mut)]
-                let mut value = Self(Default::default());
-
-                ::kernel::macros::paste!(
-                $(
-                value.[<set_ $field>](Default::default());
-                )*
-                );
-
-                value
-            }
-        }
-    };
-
     // Generates the IO accessors for a fixed offset register.
     (@io_fixed $name:ident @ $offset:expr) => {
         #[allow(dead_code)]
-- 
2.34.1

[PATCH v7.1 3/4] gpu: nova-core: bitfield: Add support for different storage widths

[Joel Fernandes]

Previously, bitfields were hardcoded to use u32 as the underlying
storage type.  Add support for different storage types (u8, u16, u32,
u64) to the bitfield macro.

New syntax is: struct Name(<type ex., u32>) { ... }

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
Reviewed-by: Edwin Peer <epeer@nvidia.com>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
 drivers/gpu/nova-core/bitfield.rs    | 65 ++++++++++++++++------------
 drivers/gpu/nova-core/regs/macros.rs | 16 +++----
 2 files changed, 46 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
index 98ccb1bd3289..998970ff0bbc 100644
--- a/drivers/gpu/nova-core/bitfield.rs
+++ b/drivers/gpu/nova-core/bitfield.rs
@@ -60,7 +60,7 @@
 /// }
 ///
 /// bitfield! {
-///     struct ControlReg {
+///     struct ControlReg(u32) {
 ///         3:0 mode as u8 ?=> Mode;
 ///         7:7 state as bool => State;
 ///     }
@@ -70,6 +70,8 @@
 /// This generates a struct with:
 /// - Field accessors: `mode()`, `state()`, etc.
 /// - Field setters: `set_mode()`, `set_state()`, etc. (supports chaining with builder pattern).
+///   Note that the compiler will error out if the size of the setter's arg exceeds the
+///   struct's storage size.
 /// - Debug and Default implementations.
 ///
 /// Fields are defined as follows:
@@ -82,21 +84,21 @@
 ///   and returns the result. This is useful with fields for which not all values are valid.
 macro_rules! bitfield {
     // Main entry point - defines the bitfield struct with fields
-    (struct $name:ident $(, $comment:literal)? { $($fields:tt)* }) => {
-        bitfield!(@core $name $(, $comment)? { $($fields)* });
+    (struct $name:ident($storage:ty) $(, $comment:literal)? { $($fields:tt)* }) => {
+        bitfield!(@core $name $storage $(, $comment)? { $($fields)* });
     };
 
     // All rules below are helpers.
 
     // Defines the wrapper `$name` type, as well as its relevant implementations (`Debug`,
     // `Default`, `BitOr`, and conversion to the value type) and field accessor methods.
-    (@core $name:ident $(, $comment:literal)? { $($fields:tt)* }) => {
+    (@core $name:ident $storage:ty $(, $comment:literal)? { $($fields:tt)* }) => {
         $(
         #[doc=$comment]
         )?
         #[repr(transparent)]
         #[derive(Clone, Copy)]
-        pub(crate) struct $name(u32);
+        pub(crate) struct $name($storage);
 
         impl ::core::ops::BitOr for $name {
             type Output = Self;
@@ -106,20 +108,20 @@ fn bitor(self, rhs: Self) -> Self::Output {
             }
         }
 
-        impl ::core::convert::From<$name> for u32 {
-            fn from(val: $name) -> u32 {
+        impl ::core::convert::From<$name> for $storage {
+            fn from(val: $name) -> $storage {
                 val.0
             }
         }
 
-        bitfield!(@fields_dispatcher $name { $($fields)* });
+        bitfield!(@fields_dispatcher $name $storage { $($fields)* });
     };
 
     // Captures the fields and passes them to all the implementers that require field information.
     //
     // Used to simplify the matching rules for implementers, so they don't need to match the entire
     // complex fields rule even though they only make use of part of it.
-    (@fields_dispatcher $name:ident {
+    (@fields_dispatcher $name:ident $storage:ty {
         $($hi:tt:$lo:tt $field:ident as $type:tt
             $(?=> $try_into_type:ty)?
             $(=> $into_type:ty)?
@@ -128,7 +130,7 @@ fn from(val: $name) -> u32 {
         )*
     }
     ) => {
-        bitfield!(@field_accessors $name {
+        bitfield!(@field_accessors $name $storage {
             $(
                 $hi:$lo $field as $type
                 $(?=> $try_into_type)?
@@ -143,7 +145,7 @@ fn from(val: $name) -> u32 {
 
     // Defines all the field getter/setter methods for `$name`.
     (
-        @field_accessors $name:ident {
+        @field_accessors $name:ident $storage:ty {
         $($hi:tt:$lo:tt $field:ident as $type:tt
             $(?=> $try_into_type:ty)?
             $(=> $into_type:ty)?
@@ -159,7 +161,7 @@ fn from(val: $name) -> u32 {
         #[allow(dead_code)]
         impl $name {
             $(
-            bitfield!(@field_accessor $name $hi:$lo $field as $type
+            bitfield!(@field_accessor $name $storage, $hi:$lo $field as $type
                 $(?=> $try_into_type)?
                 $(=> $into_type)?
                 $(, $comment)?
@@ -193,11 +195,11 @@ impl $name {
 
     // Catches fields defined as `bool` and convert them into a boolean value.
     (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
+        @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
             $(, $comment:literal)?;
     ) => {
         bitfield!(
-            @leaf_accessor $name $hi:$lo $field
+            @leaf_accessor $name $storage, $hi:$lo $field
             { |f| <$into_type>::from(if f != 0 { true } else { false }) }
             bool $into_type => $into_type $(, $comment)?;
         );
@@ -205,17 +207,17 @@ impl $name {
 
     // Shortcut for fields defined as `bool` without the `=>` syntax.
     (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
+        @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
     ) => {
-        bitfield!(@field_accessor $name $hi:$lo $field as bool => bool $(, $comment)?;);
+        bitfield!(@field_accessor $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
     };
 
     // Catches the `?=>` syntax for non-boolean fields.
     (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
+        @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
             $(, $comment:literal)?;
     ) => {
-        bitfield!(@leaf_accessor $name $hi:$lo $field
+        bitfield!(@leaf_accessor $name $storage, $hi:$lo $field
             { |f| <$try_into_type>::try_from(f as $type) } $type $try_into_type =>
             ::core::result::Result<
                 $try_into_type,
@@ -226,29 +228,38 @@ impl $name {
 
     // Catches the `=>` syntax for non-boolean fields.
     (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
+        @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
             $(, $comment:literal)?;
     ) => {
-        bitfield!(@leaf_accessor $name $hi:$lo $field
+        bitfield!(@leaf_accessor $name $storage, $hi:$lo $field
             { |f| <$into_type>::from(f as $type) } $type $into_type => $into_type $(, $comment)?;);
     };
 
     // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
     (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt
+        @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt
             $(, $comment:literal)?;
     ) => {
-        bitfield!(@field_accessor $name $hi:$lo $field as $type => $type $(, $comment)?;);
+        bitfield!(@field_accessor $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
     };
 
     // Generates the accessor methods for a single field.
     (
-        @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
+        @leaf_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident
             { $process:expr } $prim_type:tt $to_type:ty => $res_type:ty $(, $comment:literal)?;
     ) => {
         ::kernel::macros::paste!(
         const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
-        const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
+        const [<$field:upper _MASK>]: $storage = {
+            // Generate mask for shifting
+            match ::core::mem::size_of::<$storage>() {
+                1 => ::kernel::bits::genmask_u8($lo..=$hi) as $storage,
+                2 => ::kernel::bits::genmask_u16($lo..=$hi) as $storage,
+                4 => ::kernel::bits::genmask_u32($lo..=$hi) as $storage,
+                8 => ::kernel::bits::genmask_u64($lo..=$hi) as $storage,
+                _ => ::kernel::build_error!("Unsupported storage type size")
+            }
+        };
         const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
         );
 
@@ -259,7 +270,7 @@ impl $name {
         #[inline(always)]
         pub(crate) fn $field(self) -> $res_type {
             ::kernel::macros::paste!(
-            const MASK: u32 = $name::[<$field:upper _MASK>];
+            const MASK: $storage = $name::[<$field:upper _MASK>];
             const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
             );
             let field = ((self.0 & MASK) >> SHIFT);
@@ -274,9 +285,9 @@ pub(crate) fn $field(self) -> $res_type {
         )?
         #[inline(always)]
         pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
-            const MASK: u32 = $name::[<$field:upper _MASK>];
+            const MASK: $storage = $name::[<$field:upper _MASK>];
             const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
-            let value = (u32::from($prim_type::from(value)) << SHIFT) & MASK;
+            let value = ($storage::from($prim_type::from(value)) << SHIFT) & MASK;
             self.0 = (self.0 & !MASK) | value;
 
             self
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index 945d15a2c529..ffd7d5cb73bb 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -276,25 +276,25 @@ pub(crate) trait RegisterBase<T> {
 macro_rules! register {
     // Creates a register at a fixed offset of the MMIO space.
     ($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* } ) => {
-        bitfield!(struct $name $(, $comment)? { $($fields)* } );
+        bitfield!(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!(struct $name $(, $comment)? { $($fields)* } );
+        bitfield!(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!(struct $name $(, $comment)? { $($fields)* } );
+        bitfield!(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!(struct $name $(, $comment)? { $($fields)* } );
+        bitfield!(struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_relative $name @ $base [ $alias::OFFSET ]);
     };
 
@@ -305,7 +305,7 @@ macro_rules! register {
         }
     ) => {
         static_assert!(::core::mem::size_of::<u32>() <= $stride);
-        bitfield!(struct $name $(, $comment)? { $($fields)* } );
+        bitfield!(struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_array $name @ $offset [ $size ; $stride ]);
     };
 
@@ -326,7 +326,7 @@ macro_rules! register {
             $(, $comment:literal)? { $($fields:tt)* }
     ) => {
         static_assert!(::core::mem::size_of::<u32>() <= $stride);
-        bitfield!(struct $name $(, $comment)? { $($fields)* } );
+        bitfield!(struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_relative_array $name @ $base [ $offset [ $size ; $stride ] ]);
     };
 
@@ -348,7 +348,7 @@ macro_rules! register {
         }
     ) => {
         static_assert!($idx < $alias::SIZE);
-        bitfield!(struct $name $(, $comment)? { $($fields)* } );
+        bitfield!(struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_relative $name @ $base [ $alias::OFFSET + $idx * $alias::STRIDE ] );
     };
 
@@ -357,7 +357,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!(struct $name $(, $comment)? { $($fields)* } );
+        bitfield!(struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );
     };
 
-- 
2.34.1

[PATCH v7.1 4/4] gpu: nova-core: bitfield: Add support for custom visiblity

[Joel Fernandes]

Add support for custom visiblity to allow for users to control
visibility of the structure and helpers.

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
Reviewed-by: Edwin Peer <epeer@nvidia.com>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
 drivers/gpu/nova-core/bitfield.rs    | 49 +++++++++++++++-------------
 drivers/gpu/nova-core/regs/macros.rs | 16 ++++-----
 2 files changed, 34 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
index 998970ff0bbc..0994505393dd 100644
--- a/drivers/gpu/nova-core/bitfield.rs
+++ b/drivers/gpu/nova-core/bitfield.rs
@@ -60,7 +60,7 @@
 /// }
 ///
 /// bitfield! {
-///     struct ControlReg(u32) {
+///     pub struct ControlReg(u32) {
 ///         3:0 mode as u8 ?=> Mode;
 ///         7:7 state as bool => State;
 ///     }
@@ -74,6 +74,9 @@
 ///   struct's storage size.
 /// - Debug and Default implementations.
 ///
+/// Note: Field accessors and setters inherit the same visibility as the struct itself.
+/// In the example above, both `mode()` and `set_mode()` methods will be `pub`.
+///
 /// Fields are defined as follows:
 ///
 /// - `as <type>` simply returns the field value casted to <type>, typically `u32`, `u16`, `u8` or
@@ -84,21 +87,21 @@
 ///   and returns the result. This is useful with fields for which not all values are valid.
 macro_rules! bitfield {
     // Main entry point - defines the bitfield struct with fields
-    (struct $name:ident($storage:ty) $(, $comment:literal)? { $($fields:tt)* }) => {
-        bitfield!(@core $name $storage $(, $comment)? { $($fields)* });
+    ($vis:vis struct $name:ident($storage:ty) $(, $comment:literal)? { $($fields:tt)* }) => {
+        bitfield!(@core $vis $name $storage $(, $comment)? { $($fields)* });
     };
 
     // All rules below are helpers.
 
     // Defines the wrapper `$name` type, as well as its relevant implementations (`Debug`,
     // `Default`, `BitOr`, and conversion to the value type) and field accessor methods.
-    (@core $name:ident $storage:ty $(, $comment:literal)? { $($fields:tt)* }) => {
+    (@core $vis:vis $name:ident $storage:ty $(, $comment:literal)? { $($fields:tt)* }) => {
         $(
         #[doc=$comment]
         )?
         #[repr(transparent)]
         #[derive(Clone, Copy)]
-        pub(crate) struct $name($storage);
+        $vis struct $name($storage);
 
         impl ::core::ops::BitOr for $name {
             type Output = Self;
@@ -114,14 +117,14 @@ fn from(val: $name) -> $storage {
             }
         }
 
-        bitfield!(@fields_dispatcher $name $storage { $($fields)* });
+        bitfield!(@fields_dispatcher $vis $name $storage { $($fields)* });
     };
 
     // Captures the fields and passes them to all the implementers that require field information.
     //
     // Used to simplify the matching rules for implementers, so they don't need to match the entire
     // complex fields rule even though they only make use of part of it.
-    (@fields_dispatcher $name:ident $storage:ty {
+    (@fields_dispatcher $vis:vis $name:ident $storage:ty {
         $($hi:tt:$lo:tt $field:ident as $type:tt
             $(?=> $try_into_type:ty)?
             $(=> $into_type:ty)?
@@ -130,7 +133,7 @@ fn from(val: $name) -> $storage {
         )*
     }
     ) => {
-        bitfield!(@field_accessors $name $storage {
+        bitfield!(@field_accessors $vis $name $storage {
             $(
                 $hi:$lo $field as $type
                 $(?=> $try_into_type)?
@@ -145,7 +148,7 @@ fn from(val: $name) -> $storage {
 
     // Defines all the field getter/setter methods for `$name`.
     (
-        @field_accessors $name:ident $storage:ty {
+        @field_accessors $vis:vis $name:ident $storage:ty {
         $($hi:tt:$lo:tt $field:ident as $type:tt
             $(?=> $try_into_type:ty)?
             $(=> $into_type:ty)?
@@ -161,7 +164,7 @@ fn from(val: $name) -> $storage {
         #[allow(dead_code)]
         impl $name {
             $(
-            bitfield!(@field_accessor $name $storage, $hi:$lo $field as $type
+            bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type
                 $(?=> $try_into_type)?
                 $(=> $into_type)?
                 $(, $comment)?
@@ -195,11 +198,11 @@ impl $name {
 
     // Catches fields defined as `bool` and convert them into a boolean value.
     (
-        @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
+        @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
             $(, $comment:literal)?;
     ) => {
         bitfield!(
-            @leaf_accessor $name $storage, $hi:$lo $field
+            @leaf_accessor $vis $name $storage, $hi:$lo $field
             { |f| <$into_type>::from(if f != 0 { true } else { false }) }
             bool $into_type => $into_type $(, $comment)?;
         );
@@ -207,17 +210,17 @@ impl $name {
 
     // Shortcut for fields defined as `bool` without the `=>` syntax.
     (
-        @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
+        @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
     ) => {
-        bitfield!(@field_accessor $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
+        bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
     };
 
     // Catches the `?=>` syntax for non-boolean fields.
     (
-        @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
+        @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 $name $storage, $hi:$lo $field
+        bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
             { |f| <$try_into_type>::try_from(f as $type) } $type $try_into_type =>
             ::core::result::Result<
                 $try_into_type,
@@ -228,24 +231,24 @@ impl $name {
 
     // Catches the `=>` syntax for non-boolean fields.
     (
-        @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
+        @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 $name $storage, $hi:$lo $field
+        bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
             { |f| <$into_type>::from(f as $type) } $type $into_type => $into_type $(, $comment)?;);
     };
 
     // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
     (
-        @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt
+        @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt
             $(, $comment:literal)?;
     ) => {
-        bitfield!(@field_accessor $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
+        bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
     };
 
     // Generates the accessor methods for a single field.
     (
-        @leaf_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident
+        @leaf_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident
             { $process:expr } $prim_type:tt $to_type:ty => $res_type:ty $(, $comment:literal)?;
     ) => {
         ::kernel::macros::paste!(
@@ -268,7 +271,7 @@ impl $name {
         #[doc=$comment]
         )?
         #[inline(always)]
-        pub(crate) fn $field(self) -> $res_type {
+        $vis fn $field(self) -> $res_type {
             ::kernel::macros::paste!(
             const MASK: $storage = $name::[<$field:upper _MASK>];
             const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
@@ -284,7 +287,7 @@ pub(crate) fn $field(self) -> $res_type {
         #[doc=$comment]
         )?
         #[inline(always)]
-        pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
+        $vis fn [<set_ $field>](mut self, value: $to_type) -> Self {
             const MASK: $storage = $name::[<$field:upper _MASK>];
             const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
             let value = ($storage::from($prim_type::from(value)) << SHIFT) & MASK;
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index ffd7d5cb73bb..c0a5194e8d97 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -276,25 +276,25 @@ pub(crate) trait RegisterBase<T> {
 macro_rules! register {
     // Creates a register at a fixed offset of the MMIO space.
     ($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* } ) => {
-        bitfield!(struct $name(u32) $(, $comment)? { $($fields)* } );
+        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!(struct $name(u32) $(, $comment)? { $($fields)* } );
+        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!(struct $name(u32) $(, $comment)? { $($fields)* } );
+        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!(struct $name(u32) $(, $comment)? { $($fields)* } );
+        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_relative $name @ $base [ $alias::OFFSET ]);
     };
 
@@ -305,7 +305,7 @@ macro_rules! register {
         }
     ) => {
         static_assert!(::core::mem::size_of::<u32>() <= $stride);
-        bitfield!(struct $name(u32) $(, $comment)? { $($fields)* } );
+        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_array $name @ $offset [ $size ; $stride ]);
     };
 
@@ -326,7 +326,7 @@ macro_rules! register {
             $(, $comment:literal)? { $($fields:tt)* }
     ) => {
         static_assert!(::core::mem::size_of::<u32>() <= $stride);
-        bitfield!(struct $name(u32) $(, $comment)? { $($fields)* } );
+        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_relative_array $name @ $base [ $offset [ $size ; $stride ] ]);
     };
 
@@ -348,7 +348,7 @@ macro_rules! register {
         }
     ) => {
         static_assert!($idx < $alias::SIZE);
-        bitfield!(struct $name(u32) $(, $comment)? { $($fields)* } );
+        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_relative $name @ $base [ $alias::OFFSET + $idx * $alias::STRIDE ] );
     };
 
@@ -357,7 +357,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!(struct $name(u32) $(, $comment)? { $($fields)* } );
+        bitfield!(pub(crate) struct $name(u32) $(, $comment)? { $($fields)* } );
         register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );
     };
 
-- 
2.34.1

Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[Yury Norov]

On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
> Move the bitfield-specific code from the register macro into a new macro
> called bitfield. This will be used to define structs with bitfields,
> similar to C language.

Can you please fix line length issues before v8?

$ awk '{print length}' drivers/gpu/nova-core/bitfield.rs | sort -rn | uniq -c
      1 118
      1 116
      1 113
      1 109
      1 105
      1 103
      ...
 
> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> Reviewed-by: Edwin Peer <epeer@nvidia.com>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> ---
>  drivers/gpu/nova-core/bitfield.rs    | 319 +++++++++++++++++++++++++++
>  drivers/gpu/nova-core/nova_core.rs   |   3 +
>  drivers/gpu/nova-core/regs/macros.rs | 259 +---------------------
>  3 files changed, 332 insertions(+), 249 deletions(-)
>  create mode 100644 drivers/gpu/nova-core/bitfield.rs
 
...

> +///
> +/// bitfield! {
> +///     struct ControlReg {
> +///         3:0 mode as u8 ?=> Mode;
> +///         7:7 state as bool => State;
> +///     }
> +/// }

This notation is really unwelcome this days. It may be OK for a random
macro in some local driver, but doesn't really work for a global basic
data type:

https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/

I've already shared this link with you, and shared my concern.

I realize that rust/bitfield derives the GENMASK(hi, lo) notation here,
and GENMASK() derives verilog or hardware specs popular notations. But
software people prefer lo:hi. I'm probably OK if you choose C-style
start:nbits, if you prefer. But let's stop this hi:lo early, please.

Let me quote Linus from the link above:

  It does "high, low", which is often very unintuitive, and in fact the
  very commit that introduced this thing from hell had to convert the
  sane "low,high" cases to the other way around.
  
  See commit 10ef6b0dffe4 ("bitops: Introduce a more generic BITMASK
  macro"), and notice how ALMOST ALL use cases were switched around to
  the illogical "high,low" format by that introductory phase.
  
  And yes, I understand why that person did it: many datasheets show
  bits in a register graphically, and then you see that "high .. low"
  thing in a rectangle that describes the register, and that ordering
  them makes 100% sense IN THAT CONTEXT.
  
  But it damn well does not make sense in most other contexts.

  In fact, even in the context of generating mask #defines, it actually
  reads oddly, because you end up having things like

    /* Status register (SR) */
    #define I2C_SR_OP               GENMASK(1, 0)   /* Operation */
    #define I2C_SR_STATUS           GENMASK(3, 2)   /* controller status */
    #define I2C_SR_CAUSE            GENMASK(6, 4)   /* Abort cause */
    #define I2C_SR_TYPE             GENMASK(8, 7)   /* Receive type */
    #define I2C_SR_LENGTH           GENMASK(19, 9)  /* Transfer length */

  ...

Now compare it to what we've got in nova right now:

  register!(NV_PMC_BOOT_0 @ 0x00000000, "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";
      8:8     architecture_1 as u8, "MSB of the architecture";
      23:20   implementation as u8, "Implementation version of the architecture";
      28:24   architecture_0 as u8, "Lower bits of the architecture";
  });

There's so far 36 thousands of GENMASK()s in the kernel, and only 67
register!()s. It's a separate topic what to do with the GENMASK()
codebase. But now that you do this massive refactoring for the
register!() macro, let's convert those 67 users to the lo:hi notation.

As a side note, for GENMASKs, I tried this trick:

        #define GENMASK(a, b) UNSAFE_GENMASK(MIN(a, b), MAX(a, b))

It works, but bloats defconfig kernel for another 1K. I don't think it
would add to readability on both C and rust sides.

Thanks,
Yury

Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[Joel Fernandes]

> On Oct 16, 2025, at 1:48 PM, Yury Norov <yury.norov@gmail.com> wrote:
> 
> On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
>> Move the bitfield-specific code from the register macro into a new macro
>> called bitfield. This will be used to define structs with bitfields,
>> similar to C language.
> 
> Can you please fix line length issues before v8?
> 
> $ awk '{print length}' drivers/gpu/nova-core/bitfield.rs | sort -rn | uniq -c
>      1 118
>      1 116
>      1 113
>      1 109
>      1 105
>      1 103

That is intentional. I will look again but long lines can be a matter of style
and if wrapping effects readability then we do not want to do that. That is
why it is a checkpatch warning not an error. We have to look it case by case.

>      ...
> 
>> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
>> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
>> Reviewed-by: Edwin Peer <epeer@nvidia.com>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>> drivers/gpu/nova-core/bitfield.rs    | 319 +++++++++++++++++++++++++++
>> drivers/gpu/nova-core/nova_core.rs   |   3 +
>> drivers/gpu/nova-core/regs/macros.rs | 259 +---------------------
>> 3 files changed, 332 insertions(+), 249 deletions(-)
>> create mode 100644 drivers/gpu/nova-core/bitfield.rs
> 
> ...
> 
>> +///
>> +/// bitfield! {
>> +///     struct ControlReg {
>> +///         3:0 mode as u8 ?=> Mode;
>> +///         7:7 state as bool => State;
>> +///     }
>> +/// }
> 
> This notation is really unwelcome this days. It may be OK for a random
> macro in some local driver, but doesn't really work for a global basic
> data type:
> 
> https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/
> 
> I've already shared this link with you, and shared my concern.
> 
> I realize that rust/bitfield derives the GENMASK(hi, lo) notation here,
> and GENMASK() derives verilog or hardware specs popular notations. But
> software people prefer lo:hi. I'm probably OK if you choose C-style
> start:nbits, if you prefer. But let's stop this hi:lo early, please.
> 
> Let me quote Linus from the link above:
> 
>  It does "high, low", which is often very unintuitive, and in fact the
>  very commit that introduced this thing from hell had to convert the
>  sane "low,high" cases to the other way around.

I agree with Linus but I disagree with comparing it with these macros.
I agree with Linus it is oddly unreadable when used as function parameters.
But that is a different syntax. Over here we are using colons with sufficient whitespace around hi:lo.

> 
>  See commit 10ef6b0dffe4 ("bitops: Introduce a more generic BITMASK
>  macro"), and notice how ALMOST ALL use cases were switched around to
>  the illogical "high,low" format by that introductory phase.

Again this is different syntax so assuming that Linus will not be ok with it is a stretch IMO.

The rust macros here have their own syntax and are not function parameters.

> 
>  And yes, I understand why that person did it: many datasheets show
>  bits in a register graphically, and then you see that "high .. low"
>  thing in a rectangle that describes the register, and that ordering
>  them makes 100% sense IN THAT CONTEXT.
> 
>  But it damn well does not make sense in most other contexts.
> 
>  In fact, even in the context of generating mask #defines, it actually
>  reads oddly, because you end up having things like
> 
>    /* Status register (SR) */
>    #define I2C_SR_OP               GENMASK(1, 0)   /* Operation */
>    #define I2C_SR_STATUS           GENMASK(3, 2)   /* controller status */
>    #define I2C_SR_CAUSE            GENMASK(6, 4)   /* Abort cause */
>    #define I2C_SR_TYPE             GENMASK(8, 7)   /* Receive type */
>    #define I2C_SR_LENGTH           GENMASK(19, 9)  /* Transfer length */

Sure this is super odd indeed. But again not apples to apples here.

thanks,

 - Joel


> 
>  ...
> 
> Now compare it to what we've got in nova right now:
> 
>  register!(NV_PMC_BOOT_0 @ 0x00000000, "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";
>      8:8     architecture_1 as u8, "MSB of the architecture";
>      23:20   implementation as u8, "Implementation version of the architecture";
>      28:24   architecture_0 as u8, "Lower bits of the architecture";
>  });
> 
> There's so far 36 thousands of GENMASK()s in the kernel, and only 67
> register!()s. It's a separate topic what to do with the GENMASK()
> codebase. But now that you do this massive refactoring for the
> register!() macro, let's convert those 67 users to the lo:hi notation.
> 
> As a side note, for GENMASKs, I tried this trick:
> 
>        #define GENMASK(a, b) UNSAFE_GENMASK(MIN(a, b), MAX(a, b))
> 
> It works, but bloats defconfig kernel for another 1K. I don't think it
> would add to readability on both C and rust sides.
> 
> Thanks,
> Yury

Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[Danilo Krummrich]

On Thu Oct 16, 2025 at 9:28 PM CEST, Joel Fernandes wrote:
>> On Oct 16, 2025, at 1:48 PM, Yury Norov <yury.norov@gmail.com> wrote:
>> On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
>>> +///
>>> +/// bitfield! {
>>> +///     struct ControlReg {
>>> +///         3:0 mode as u8 ?=> Mode;
>>> +///         7:7 state as bool => State;
>>> +///     }
>>> +/// }
>> 
>> This notation is really unwelcome this days. It may be OK for a random
>> macro in some local driver, but doesn't really work for a global basic
>> data type:
>> 
>> https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/
>> 
>> I've already shared this link with you, and shared my concern.
>> 
>> I realize that rust/bitfield derives the GENMASK(hi, lo) notation here,
>> and GENMASK() derives verilog or hardware specs popular notations. But
>> software people prefer lo:hi. I'm probably OK if you choose C-style
>> start:nbits, if you prefer. But let's stop this hi:lo early, please.
>> 
>> Let me quote Linus from the link above:
>> 
>>  It does "high, low", which is often very unintuitive, and in fact the
>>  very commit that introduced this thing from hell had to convert the
>>  sane "low,high" cases to the other way around.
>
> I agree with Linus but I disagree with comparing it with these macros.
> I agree with Linus it is oddly unreadable when used as function parameters.
> But that is a different syntax. Over here we are using colons with sufficient whitespace around hi:lo.

I agree with Joel here.

While I'm not super opinionated for general bitfields, for the register!()
infrastructure I very much prefer the hi:lo notation, as this is the common
notation in datasheets and TRMs.

However, if we use hi:lo, we should use it decending, i.e.:

	bitfield! {
	    struct ControlReg {
	        7:5 state as u8 => State;
	        3:0 mode as u8 ?=> Mode;
	    }
	}

- Danilo

Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[John Hubbard]

On 10/16/25 12:34 PM, Danilo Krummrich wrote:
> On Thu Oct 16, 2025 at 9:28 PM CEST, Joel Fernandes wrote:
>>> On Oct 16, 2025, at 1:48 PM, Yury Norov <yury.norov@gmail.com> wrote:
>>> On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
...
> While I'm not super opinionated for general bitfields, for the register!()
> infrastructure I very much prefer the hi:lo notation, as this is the common
> notation in datasheets and TRMs.
> 
> However, if we use hi:lo, we should use it decending, i.e.:
> 

Sure, descending works.

> 	bitfield! {
> 	    struct ControlReg {
> 	        7:5 state as u8 => State;
> 	        3:0 mode as u8 ?=> Mode;

And hi:lo matches our HW reference manuals. And if you're dealing
with bitfields, you are often also dealing with HW, so this is
a reasonable place in the SW to use hi:lo.

Looks good to me.


thanks,
-- 
John Hubbard

Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[John Hubbard]

On 10/16/25 12:28 PM, Joel Fernandes wrote:
>> On Oct 16, 2025, at 1:48 PM, Yury Norov <yury.norov@gmail.com> wrote:
>> On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
...
>> Can you please fix line length issues before v8?
>>
>> $ awk '{print length}' drivers/gpu/nova-core/bitfield.rs | sort -rn | uniq -c
>>      1 118
>>      1 116
>>      1 113
>>      1 109
>>      1 105
>>      1 103
> 
> That is intentional. I will look again but long lines can be a matter of style
> and if wrapping effects readability then we do not want to do that. That is

This is true, but let's be very mindful about pushing that *subjective*
readability thing--and strongly prefer Rust's 100 line length convention.

The larger Rust for Linux has been quite diligent about trying to stay
within that length.

In this case, most or even all of these can stay under 100 lines, I suspect.

> why it is a checkpatch warning not an error. We have to look it case by case.
> 


thanks,
-- 
John Hubbard

Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[Joel Fernandes]

> On Oct 16, 2025, at 3:42 PM, John Hubbard <jhubbard@nvidia.com> wrote:
> 
> In this case, most or even all of these can stay under 100 lines, I suspect.

Sure, 100 line limit sounds reasonable. It was not clear what limit Yury
was referring to (ex, 80 lines).

I will revise it.  Thanks,

 - Joel


> 
>> why it is a checkpatch warning not an error. We have to look it case by case.
>> 
> 
> 
> thanks,
> --
> John Hubbard
> 

Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[Joel Fernandes]

> On Oct 16, 2025, at 3:34 PM, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Thu Oct 16, 2025 at 9:28 PM CEST, Joel Fernandes wrote:
>>>> On Oct 16, 2025, at 1:48 PM, Yury Norov <yury.norov@gmail.com> wrote:
>>> On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
>>>> +///
>>>> +/// bitfield! {
>>>> +///     struct ControlReg {
>>>> +///         3:0 mode as u8 ?=> Mode;
>>>> +///         7:7 state as bool => State;
>>>> +///     }
>>>> +/// }
>>> 
>>> This notation is really unwelcome this days. It may be OK for a random
>>> macro in some local driver, but doesn't really work for a global basic
>>> data type:
>>> 
>>> https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/
>>> 
>>> I've already shared this link with you, and shared my concern.
>>> 
>>> I realize that rust/bitfield derives the GENMASK(hi, lo) notation here,
>>> and GENMASK() derives verilog or hardware specs popular notations. But
>>> software people prefer lo:hi. I'm probably OK if you choose C-style
>>> start:nbits, if you prefer. But let's stop this hi:lo early, please.
>>> 
>>> Let me quote Linus from the link above:
>>> 
>>> It does "high, low", which is often very unintuitive, and in fact the
>>> very commit that introduced this thing from hell had to convert the
>>> sane "low,high" cases to the other way around.
>> 
>> I agree with Linus but I disagree with comparing it with these macros.
>> I agree with Linus it is oddly unreadable when used as function parameters.
>> But that is a different syntax. Over here we are using colons with sufficient whitespace around hi:lo.
> 
> I agree with Joel here.
> 
> While I'm not super opinionated for general bitfields, for the register!()
> infrastructure I very much prefer the hi:lo notation, as this is the common
> notation in datasheets and TRMs.
> 
> However, if we use hi:lo, we should use it decending, i.e.:
> 
>    bitfield! {
>        struct ControlReg {
>            7:5 state as u8 => State;
>            3:0 mode as u8 ?=> Mode;
>        }
>    }

Makes sense. The macro currently supports both. I will fix the example docs.

thanks,

 - Joel


> 
> - Danilo

Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[Alexandre Courbot]

On Fri Oct 17, 2025 at 4:39 AM JST, John Hubbard wrote:
> On 10/16/25 12:34 PM, Danilo Krummrich wrote:
>> On Thu Oct 16, 2025 at 9:28 PM CEST, Joel Fernandes wrote:
>>>> On Oct 16, 2025, at 1:48 PM, Yury Norov <yury.norov@gmail.com> wrote:
>>>> On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
> ...
>> While I'm not super opinionated for general bitfields, for the register!()
>> infrastructure I very much prefer the hi:lo notation, as this is the common
>> notation in datasheets and TRMs.
>> 
>> However, if we use hi:lo, we should use it decending, i.e.:
>> 
>
> Sure, descending works.
>
>> 	bitfield! {
>> 	    struct ControlReg {
>> 	        7:5 state as u8 => State;
>> 	        3:0 mode as u8 ?=> Mode;
>
> And hi:lo matches our HW reference manuals. And if you're dealing
> with bitfields, you are often also dealing with HW, so this is
> a reasonable place in the SW to use hi:lo.

Definitely agree here. The use of `:` is what makes the difference with
the GENMASK macro, which separates its argument with a regular comma.
There is no room for mistaking these with anything else.

Re: [PATCH v7.1 0/4] bitfield initial refactor within nova-core (RESEND)

[Alexandre Courbot]

On Fri Oct 17, 2025 at 12:13 AM JST, Joel Fernandes wrote:
> (Resending due to some commit message mistakes (missing SOB etc). Thanks!).
>
> These patches implement the initial refactoring and few improvements to the
> register and bitfield macros. Rebased on drm-rust-next.
>
> Main difference from the previous series [1] is dropped the moving out of
> nova-core pending BoundedInt changes:
> https://lore.kernel.org/all/20251003154748.1687160-1-joelagnelf@nvidia.com/
> Other than that, added tags, resolved conflict with kernel::fmt changes and
> rebased on drm-rust-next.

Thanks, this version is looking pretty good, and works as intended.

I plan on pushing these 4 patches soonish after fixing the line length
issues and the other few problems reported by checkpatch.

Danilo, please let me know if you think this is premature, but imho it
is good to set this part in stone to avoid merge conflicts with future
patches that will want to modify the register macro.

Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[John Hubbard]

On 10/16/25 12:39 PM, John Hubbard wrote:
> On 10/16/25 12:34 PM, Danilo Krummrich wrote:
>> On Thu Oct 16, 2025 at 9:28 PM CEST, Joel Fernandes wrote:
>>>> On Oct 16, 2025, at 1:48 PM, Yury Norov <yury.norov@gmail.com> wrote:
>>>> On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
> ...
>> While I'm not super opinionated for general bitfields, for the register!()
>> infrastructure I very much prefer the hi:lo notation, as this is the common
>> notation in datasheets and TRMs.
>>
>> However, if we use hi:lo, we should use it decending, i.e.:
(restored from the email thread):

	bitfield! {
	    struct ControlReg {
	        7:5 state as u8 => State;
	        3:0 mode as u8 ?=> Mode;
	    }
	}>>
> 
> Sure, descending works.

Oops! I need to correct myself. After reviewing most of Joel Fernandes'
latest patchset ([PATCH 0/7] Pre-requisite patches for mm and irq in
nova-core) [1], I remember that the HW documentation is written in
ascending order.

For one example (out of countless hundreds or thousands), please see [2].
Considering that I actually pushed this file up to github just a few
years ago, it's rather silly of me to forget this basic truth. :)

We really want to stay close to the HW documentation, and so, all other
things being (nearly) equal, this means that we should prefer ascending
field order, if that's OK with everyone.


[1] https://lore.kernel.org/20251020185539.49986-1-joelagnelf@nvidia.com
[2] https://github.com/NVIDIA/open-gpu-doc/blob/master/manuals/ampere/ga102/dev_ce.ref.txt

thanks,
-- 
John Hubbard

Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[Danilo Krummrich]

On Tue Oct 21, 2025 at 12:50 AM CEST, John Hubbard wrote:
> On 10/16/25 12:39 PM, John Hubbard wrote:
>> On 10/16/25 12:34 PM, Danilo Krummrich wrote:
>>> On Thu Oct 16, 2025 at 9:28 PM CEST, Joel Fernandes wrote:
>>>>> On Oct 16, 2025, at 1:48 PM, Yury Norov <yury.norov@gmail.com> wrote:
>>>>> On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
>> ...
>>> While I'm not super opinionated for general bitfields, for the register!()
>>> infrastructure I very much prefer the hi:lo notation, as this is the common
>>> notation in datasheets and TRMs.
>>>
>>> However, if we use hi:lo, we should use it decending, i.e.:
> (restored from the email thread):
>
> 	bitfield! {
> 	    struct ControlReg {
> 	        7:5 state as u8 => State;
> 	        3:0 mode as u8 ?=> Mode;
> 	    }
> 	}>>
>> 
>> Sure, descending works.
>
> Oops! I need to correct myself. After reviewing most of Joel Fernandes'
> latest patchset ([PATCH 0/7] Pre-requisite patches for mm and irq in
> nova-core) [1], I remember that the HW documentation is written in
> ascending order.
>
> For one example (out of countless hundreds or thousands), please see [2].
> Considering that I actually pushed this file up to github just a few
> years ago, it's rather silly of me to forget this basic truth. :)
>
> We really want to stay close to the HW documentation, and so, all other
> things being (nearly) equal, this means that we should prefer ascending
> field order, if that's OK with everyone.

But that's OpenRM specific, I'm pretty sure when you look at internal datasheets
and TRMs you will find hi:lo with decending order, for instance [3] page 1672
(clicked a random location in the scroll bar. :).

Besides, I think that hi:lo with ascending order is confusing. It should either
be hi:lo decending or lo:hi ascending.

For registers the common one is the former.

> [1] https://lore.kernel.org/20251020185539.49986-1-joelagnelf@nvidia.com
> [2] https://github.com/NVIDIA/open-gpu-doc/blob/master/manuals/ampere/ga102/dev_ce.ref.txt
[3] https://developer.nvidia.com/downloads/orin-series-soc-technical-reference-manual/

Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[John Hubbard]

On 10/20/25 4:07 PM, Danilo Krummrich wrote:
> On Tue Oct 21, 2025 at 12:50 AM CEST, John Hubbard wrote:
>> On 10/16/25 12:39 PM, John Hubbard wrote:
>>> On 10/16/25 12:34 PM, Danilo Krummrich wrote:
>>>> On Thu Oct 16, 2025 at 9:28 PM CEST, Joel Fernandes wrote:
>>>>>> On Oct 16, 2025, at 1:48 PM, Yury Norov <yury.norov@gmail.com> wrote:
>>>>>> On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
>>> ...
>>>> While I'm not super opinionated for general bitfields, for the register!()
>>>> infrastructure I very much prefer the hi:lo notation, as this is the common
>>>> notation in datasheets and TRMs.
>>>>
>>>> However, if we use hi:lo, we should use it decending, i.e.:
>> (restored from the email thread):
>>
>> 	bitfield! {
>> 	    struct ControlReg {
>> 	        7:5 state as u8 => State;
>> 	        3:0 mode as u8 ?=> Mode;
>> 	    }
>> 	}>>
>>>
>>> Sure, descending works.
>>
>> Oops! I need to correct myself. After reviewing most of Joel Fernandes'
>> latest patchset ([PATCH 0/7] Pre-requisite patches for mm and irq in
>> nova-core) [1], I remember that the HW documentation is written in
>> ascending order.
>>
>> For one example (out of countless hundreds or thousands), please see [2].
>> Considering that I actually pushed this file up to github just a few
>> years ago, it's rather silly of me to forget this basic truth. :)
>>
>> We really want to stay close to the HW documentation, and so, all other
>> things being (nearly) equal, this means that we should prefer ascending
>> field order, if that's OK with everyone.
> 
> But that's OpenRM specific, I'm pretty sure when you look at internal datasheets
> and TRMs you will find hi:lo with decending order, for instance [3] page 1672

TRM is Tegra. This is gradually going away, from our point of view, as
the larger, older RM (Open RM) driver subsumes things.

Open RM follows the main dGPU ref manuals, and we have piles of those
and they all apply to Nova.

None of the TRM stuff applies to Nova.

> (clicked a random location in the scroll bar. :).
> 
> Besides, I think that hi:lo with ascending order is confusing. It should either
> be hi:lo decending or lo:hi ascending.
> 
> For registers the common one is the former.
> 
>> [1] https://lore.kernel.org/20251020185539.49986-1-joelagnelf@nvidia.com
>> [2] https://github.com/NVIDIA/open-gpu-doc/blob/master/manuals/ampere/ga102/dev_ce.ref.txt
> [3] https://developer.nvidia.com/downloads/orin-series-soc-technical-reference-manual/

Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[Danilo Krummrich]

On Tue Oct 21, 2025 at 1:16 AM CEST, John Hubbard wrote:
> On 10/20/25 4:07 PM, Danilo Krummrich wrote:
>> On Tue Oct 21, 2025 at 12:50 AM CEST, John Hubbard wrote:
>>> On 10/16/25 12:39 PM, John Hubbard wrote:
>>>> On 10/16/25 12:34 PM, Danilo Krummrich wrote:
>>>>> On Thu Oct 16, 2025 at 9:28 PM CEST, Joel Fernandes wrote:
>>>>>>> On Oct 16, 2025, at 1:48 PM, Yury Norov <yury.norov@gmail.com> wrote:
>>>>>>> On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
>>>> ...
>>>>> While I'm not super opinionated for general bitfields, for the register!()
>>>>> infrastructure I very much prefer the hi:lo notation, as this is the common
>>>>> notation in datasheets and TRMs.
>>>>>
>>>>> However, if we use hi:lo, we should use it decending, i.e.:
>>> (restored from the email thread):
>>>
>>> 	bitfield! {
>>> 	    struct ControlReg {
>>> 	        7:5 state as u8 => State;
>>> 	        3:0 mode as u8 ?=> Mode;
>>> 	    }
>>> 	}>>
>>>>
>>>> Sure, descending works.
>>>
>>> Oops! I need to correct myself. After reviewing most of Joel Fernandes'
>>> latest patchset ([PATCH 0/7] Pre-requisite patches for mm and irq in
>>> nova-core) [1], I remember that the HW documentation is written in
>>> ascending order.
>>>
>>> For one example (out of countless hundreds or thousands), please see [2].
>>> Considering that I actually pushed this file up to github just a few
>>> years ago, it's rather silly of me to forget this basic truth. :)
>>>
>>> We really want to stay close to the HW documentation, and so, all other
>>> things being (nearly) equal, this means that we should prefer ascending
>>> field order, if that's OK with everyone.
>> 
>> But that's OpenRM specific, I'm pretty sure when you look at internal datasheets
>> and TRMs you will find hi:lo with decending order, for instance [3] page 1672
>
> TRM is Tegra. This is gradually going away, from our point of view, as
> the larger, older RM (Open RM) driver subsumes things.
>
> Open RM follows the main dGPU ref manuals, and we have piles of those
> and they all apply to Nova.
>
> None of the TRM stuff applies to Nova.

My point is less about NVIDIA TRMs, it's about that this is uncommon in general,
OpenRM is the one being special here.

So, the question for me is do we care more about consistency throughout the
kernel, or do we care about consistency between a driver and it's uncommon
reference manual.

I think consistency throughout the kernel is more important.

>> (clicked a random location in the scroll bar. :).
>> 
>> Besides, I think that hi:lo with ascending order is confusing. It should either
>> be hi:lo decending or lo:hi ascending.
>> 
>> For registers the common one is the former.
>> 
>>> [1] https://lore.kernel.org/20251020185539.49986-1-joelagnelf@nvidia.com
>>> [2] https://github.com/NVIDIA/open-gpu-doc/blob/master/manuals/ampere/ga102/dev_ce.ref.txt
>> [3] https://developer.nvidia.com/downloads/orin-series-soc-technical-reference-manual/

Re: [PATCH v7.1 0/4] bitfield initial refactor within nova-core (RESEND)

[Danilo Krummrich]

On 10/18/25 3:41 PM, Alexandre Courbot wrote:
> On Fri Oct 17, 2025 at 12:13 AM JST, Joel Fernandes wrote:
>> (Resending due to some commit message mistakes (missing SOB etc). Thanks!).
>>
>> These patches implement the initial refactoring and few improvements to the
>> register and bitfield macros. Rebased on drm-rust-next.
>>
>> Main difference from the previous series [1] is dropped the moving out of
>> nova-core pending BoundedInt changes:
>> https://lore.kernel.org/all/20251003154748.1687160-1-joelagnelf@nvidia.com/
>> Other than that, added tags, resolved conflict with kernel::fmt changes and
>> rebased on drm-rust-next.
> 
> Thanks, this version is looking pretty good, and works as intended.
> 
> I plan on pushing these 4 patches soonish after fixing the line length
> issues and the other few problems reported by checkpatch.
> 
> Danilo, please let me know if you think this is premature, but imho it
> is good to set this part in stone to avoid merge conflicts with future
> patches that will want to modify the register macro.

SGTM, we can keep discussing the hi:lo ascending / descending topic for
nova-core independently.

However, for the sample code that, eventually, we'll move out of nova-core, we
should stick to what's common.

With that,

Acked-by: Danilo Krummrich <dakr@kernel.org>

Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[John Hubbard]

On 10/20/25 4:22 PM, Danilo Krummrich wrote:
> On Tue Oct 21, 2025 at 1:16 AM CEST, John Hubbard wrote:
>> On 10/20/25 4:07 PM, Danilo Krummrich wrote:
>>> On Tue Oct 21, 2025 at 12:50 AM CEST, John Hubbard wrote:
>>>> On 10/16/25 12:39 PM, John Hubbard wrote:
>>>>> On 10/16/25 12:34 PM, Danilo Krummrich wrote:
>>>>>> On Thu Oct 16, 2025 at 9:28 PM CEST, Joel Fernandes wrote:
>>>>>>>> On Oct 16, 2025, at 1:48 PM, Yury Norov <yury.norov@gmail.com> wrote:
>>>>>>>> On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
>>>>> ...
>>> But that's OpenRM specific, I'm pretty sure when you look at internal datasheets
>>> and TRMs you will find hi:lo with decending order, for instance [3] page 1672
>>
>> TRM is Tegra. This is gradually going away, from our point of view, as
>> the larger, older RM (Open RM) driver subsumes things.
>>
>> Open RM follows the main dGPU ref manuals, and we have piles of those
>> and they all apply to Nova.
>>
>> None of the TRM stuff applies to Nova.
> 
> My point is less about NVIDIA TRMs, it's about that this is uncommon in general,
> OpenRM is the one being special here.
> 
> So, the question for me is do we care more about consistency throughout the
> kernel, or do we care about consistency between a driver and it's uncommon
> reference manual.

Yes, I think that is the key point.

> 
> I think consistency throughout the kernel is more important.
> 

Perhaps, but may I point out that there are countless thousands of lines
of HW ref manuals to deal with? GPUs are uncommonly complicated devices,
and I've spent a lot of time in this area, being awed at the sheer volume
of HW documentation.

And there are hundreds of engineers who today work on GSP and Open RM,
some of whom I expect to eventually end up working on Nova.

So looking ahead, I'd be much more comfortable saying something like
this:

"Nova and its associated GPU HW manuals are special snowflakes that
use a particular documenation style. We'll allow them to deviate from
other kernel conventions in this area."

Yes? Please? :)


thanks,
-- 
John Hubbard


>>> (clicked a random location in the scroll bar. :).
>>>
>>> Besides, I think that hi:lo with ascending order is confusing. It should either
>>> be hi:lo decending or lo:hi ascending.
>>>
>>> For registers the common one is the former.
>>>
>>>> [1] https://lore.kernel.org/20251020185539.49986-1-joelagnelf@nvidia.com
>>>> [2] https://github.com/NVIDIA/open-gpu-doc/blob/master/manuals/ampere/ga102/dev_ce.ref.txt
>>> [3] https://developer.nvidia.com/downloads/orin-series-soc-technical-reference-manual/
> 

Re: [PATCH v7.1 0/4] bitfield initial refactor within nova-core (RESEND)

[Alexandre Courbot]

On Tue Oct 21, 2025 at 8:44 AM JST, Danilo Krummrich wrote:
> On 10/18/25 3:41 PM, Alexandre Courbot wrote:
>> On Fri Oct 17, 2025 at 12:13 AM JST, Joel Fernandes wrote:
>>> (Resending due to some commit message mistakes (missing SOB etc). Thanks!).
>>>
>>> These patches implement the initial refactoring and few improvements to the
>>> register and bitfield macros. Rebased on drm-rust-next.
>>>
>>> Main difference from the previous series [1] is dropped the moving out of
>>> nova-core pending BoundedInt changes:
>>> https://lore.kernel.org/all/20251003154748.1687160-1-joelagnelf@nvidia.com/
>>> Other than that, added tags, resolved conflict with kernel::fmt changes and
>>> rebased on drm-rust-next.
>> 
>> Thanks, this version is looking pretty good, and works as intended.
>> 
>> I plan on pushing these 4 patches soonish after fixing the line length
>> issues and the other few problems reported by checkpatch.
>> 
>> Danilo, please let me know if you think this is premature, but imho it
>> is good to set this part in stone to avoid merge conflicts with future
>> patches that will want to modify the register macro.
>
> SGTM, we can keep discussing the hi:lo ascending / descending topic for
> nova-core independently.
>
> However, for the sample code that, eventually, we'll move out of nova-core, we
> should stick to what's common.
>
> With that,
>
> Acked-by: Danilo Krummrich <dakr@kernel.org>

Pushed to drm-rust-next after fixing the checkpatch errors and
reordering the sample code in descending order.

... and as dim was pushing, I noticed I forgot to add your Acked-by. >_<
Apologies for that.

Re: [PATCH v7.1 0/4] bitfield initial refactor within nova-core (RESEND)

[Danilo Krummrich]

On 10/21/25 3:46 PM, Alexandre Courbot wrote:
> ... and as dim was pushing, I noticed I forgot to add your Acked-by. >_<
> Apologies for that.

No worries at all, it happens. :)