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.
Joel Fernandes
2025-Sep-09 18:55 UTC
[PATCH v2 1/4] nova-core: bitstruct: Move bitfield-specific code from register! into new macro
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 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?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
Alexandre Courbot
2025-Sep-10 13:25 UTC
[PATCH v2 1/4] nova-core: bitstruct: Move bitfield-specific code from register! into new macro
On Wed Sep 10, 2025 at 3:55 AM JST, Joel Fernandes wrote:> 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 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? > > 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 was about to write "but it not a struct", and then I remembered that the body of the macro does this: pub(crate) struct $name(u32); ... so there goes my argument. :') Just one more thing below.> >> 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.When you introduce the types, can you change the syntax from `: u32` to `(u32)`? That way the declaration becomes bitfield! { struct ControlReg(u32) { ... } } ... which at least looks like a valid declaration for a Rust struct that wraps a primitive type. Same for registers, if possible.