Alexandre Courbot
2025-Sep-08 03:12 UTC
[PATCH v2 1/4] nova-core: bitstruct: Move bitfield-specific code from register! into new macro
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 at 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 {` ?> +/// 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).> +/// > +/// This generates a struct with: > +/// - Field accessors: `mode()`, `state()`, etc. > +/// - Field setters: `set_mode()`, `set_state()`, etc. (supports builder pattern). > +/// - Debug and Default implementations > +/// > +/// The field setters can be used with the builder pattern, example: > +/// ControlReg::default().set_mode(mode).set_state(state); > +/// > +/// 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.Can you remove the related documentation from `register!` and replace it with a sentence like "Please look at the [`bitstruct`] macro for the complete syntax of fields definitions"? This will ensure we don't have to update the documentation twice if/when the syntax gets updated. The rest of the patch is a perfect move (with a few renames) of the internal rules from one macro to the other, which makes it really easy to review. Thanks for doing this!
Joel Fernandes
2025-Sep-08 17:16 UTC
[PATCH v2 1/4] nova-core: bitstruct: Move bitfield-specific code from register! into new macro
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 at 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.> >> +/// 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.> >> +/// >> +/// This generates a struct with: >> +/// - Field accessors: `mode()`, `state()`, etc. >> +/// - Field setters: `set_mode()`, `set_state()`, etc. (supports builder pattern). >> +/// - Debug and Default implementations >> +/// >> +/// The field setters can be used with the builder pattern, example: >> +/// ControlReg::default().set_mode(mode).set_state(state); >> +/// >> +/// 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. > > Can you remove the related documentation from `register!` and replace it > with a sentence like "Please look at the [`bitstruct`] macro for the > complete syntax of fields definitions"? This will ensure we don't have > to update the documentation twice if/when the syntax gets updated.Sure!> > The rest of the patch is a perfect move (with a few renames) of the > internal rules from one macro to the other, which makes it really easy > to review. Thanks for doing this!My pleasure, thanks for the suggestion! - Joel
Alexandre Courbot
2025-Sep-09 02:37 UTC
[PATCH v2 1/4] nova-core: bitstruct: Move bitfield-specific code from register! into new macro
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 at 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? 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.> >> >>> +/// 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.