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.1On 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
> 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
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
> 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 >
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
> 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
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
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
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/
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/
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/
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/ >
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.
© 2016 - 2026 Red Hat, Inc.