Re: [PATCH v2 1/4] nova-core: bitstruct: Move bitfield-specific code from register! into new macro

[Joel Fernandes <joelagnelf@nvidia.com>]

On Tue, Sep 09, 2025 at 11:37:15AM +0900, Alexandre Courbot wrote:
> On Tue Sep 9, 2025 at 2:16 AM JST, Joel Fernandes wrote:
> > Hi Alex,
> >
> > On 9/7/2025 11:12 PM, Alexandre Courbot wrote:
> >> On Thu Sep 4, 2025 at 6:54 AM JST, Joel Fernandes wrote:
> >>> The bitfield-specific into new macro. This will be used to define
> >>> structs with bitfields, similar to C language.
> >>>
> >>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> >>> ---
> >>>  drivers/gpu/nova-core/bitstruct.rs   | 271 +++++++++++++++++++++++++++
> >>>  drivers/gpu/nova-core/nova_core.rs   |   3 +
> >>>  drivers/gpu/nova-core/regs/macros.rs | 247 +-----------------------
> >>>  3 files changed, 282 insertions(+), 239 deletions(-)
> >>>  create mode 100644 drivers/gpu/nova-core/bitstruct.rs
> >>>
> >>> diff --git a/drivers/gpu/nova-core/bitstruct.rs b/drivers/gpu/nova-core/bitstruct.rs
> >>> new file mode 100644
> >>> index 000000000000..1dd9edab7d07
> >>> --- /dev/null
> >>> +++ b/drivers/gpu/nova-core/bitstruct.rs
> >>> @@ -0,0 +1,271 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +//
> >>> +// bitstruct.rs — Bitfield library for Rust structures
> >>> +//
> >>> +// A library that provides support for defining bit fields in Rust
> >>> +// structures. Also used from things that need bitfields like register macro.
> >>> +///
> >>> +/// # Syntax
> >>> +///
> >>> +/// ```rust
> >>> +/// bitstruct! {
> >>> +///     struct ControlReg {
> >> 
> >> The `struct` naming here looks a bit confusing to me - as of this patch,
> >> this is a u32, right? And eventually these types will be limited to primitive types,
> >> so why not just `ControlReg: u32 {` ?
> >
> > This is done in a later patch. This patch is only code movement, in later patch
> > we add precisely the syntax you're describing when we add storage types, and
> > update the register! macro. In this patch bitstruct is only u32.
> 
> My point was, is the `struct` keyword needed at all? Isn't it a bit
> confusing since these types are technically not Rust structs?

Now that bitstruct has changed to bitfield, I would really insist on leaving
'struct' in there.

So it will look like this:

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

Sounds reasonable?

> I agree the `: u32` can be introduced later, the original `register!`
> macro did not specify any type information so there is indeed no reason
> to add it in this patch.

Yep.

> >
> >> 
> >>> +///         3:0       mode        as u8 ?=> Mode;
> >>> +///         7:4       state       as u8 => State;
> >>> +///     }
> >>> +/// }
> >>> +/// ```
> >> 
> >> As this will move to the kernel crate, it is particularly important to
> >> make sure that this example can compile and run - so please provide
> >> simple definitions for `Mode` and `State` to make sure the kunit tests
> >> will pass after patch 4 (in the current state I'm pretty sure they won't).
> >
> > Good catch. This will blow up the example though. I will change it to no_run
> > like the register! macro did if that's Ok.
> 
> If you reduce `State` to 1 bit and change its type to `bool`, and limit
> `Mode` to two or three variants, the example should remain short. I
> think it is valuable to provide a complete working example here as the
> syntax is not obvious at first sight.

Ok, so it will look like this, still about 40 lines more, but that works for me.

@@ -7,11 +7,54 @@
 //!
 //! # Syntax
 //!
-//! ```no_run
+//! ```rust
+//! #[derive(Debug, Clone, Copy)]
+//! enum Mode {
+//!     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 u32 {
+//!     fn from(mode: Mode) -> u32 {
+//!         mode as u32
+//!     }
+//! }
+//!
+//! #[derive(Debug, Clone, Copy)]
+//! enum State {
+//!     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 u32 {
+//!     fn from(state: State) -> u32 {
+//!         state as u32
+//!     }
+//! }
+//!
 //! bitfield! {
 //!     struct ControlReg {
 //!         3:0       mode        as u8 ?=> Mode;
-//!         7:4       state       as u8 => State;
+//!         7         state       as bool => State;
 //!     }
 //! }
 //! ```

 thanks,

  - Joel