Joel Fernandes
2025-Oct-16 15:13 UTC
[PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro
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 at weathered-steel.dev>
Reviewed-by: Alexandre Courbot <acourbot at nvidia.com>
Reviewed-by: Edwin Peer <epeer at nvidia.com>
Signed-off-by: Joel Fernandes <joelagnelf at 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
Yury Norov
2025-Oct-16 17:48 UTC
[PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro
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 at weathered-steel.dev> > Reviewed-by: Alexandre Courbot <acourbot at nvidia.com> > Reviewed-by: Edwin Peer <epeer at nvidia.com> > Signed-off-by: Joel Fernandes <joelagnelf at 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 at 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
Joel Fernandes
2025-Oct-16 19:28 UTC
[PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro
> On Oct 16, 2025, at 1:48?PM, Yury Norov <yury.norov at 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 103That 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 at weathered-steel.dev> >> Reviewed-by: Alexandre Courbot <acourbot at nvidia.com> >> Reviewed-by: Edwin Peer <epeer at nvidia.com> >> Signed-off-by: Joel Fernandes <joelagnelf at 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 at 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