Message-Id: <DFY76VCL7KO7.29FCRAOOY7FVN@nvidia.com>
Date: Mon, 26 Jan 2026 12:24:11 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Gary Guo" <gary@...yguo.net>
Cc: "Danilo Krummrich" <dakr@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Daniel Almeida" <daniel.almeida@...labora.com>,
 "Miguel Ojeda" <ojeda@...nel.org>, "Boqun Feng" <boqun.feng@...il.com>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Trevor
 Gross" <tmgross@...ch.edu>, "Yury Norov" <yury.norov@...il.com>, "John
 Hubbard" <jhubbard@...dia.com>, "Alistair Popple" <apopple@...dia.com>,
 "Joel Fernandes" <joelagnelf@...dia.com>, "Timur Tabi" <ttabi@...dia.com>,
 "Edwin Peer" <epeer@...dia.com>, "Eliot Courtney" <ecourtney@...dia.com>,
 "Dirk Behme" <dirk.behme@...bosch.com>, "Steven Price"
 <steven.price@....com>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 4/5] rust: io: add `register!` macro

On Wed Jan 21, 2026 at 11:50 PM JST, Gary Guo wrote:
> On Wed Jan 21, 2026 at 7:23 AM GMT, Alexandre Courbot wrote:
>> Add a macro for defining hardware register types with I/O accessors.
>>
>> Each register field is represented as a `Bounded` of the appropriate bit
>> width, ensuring field values are never silently truncated.
>>
>> Fields can optionally be converted to/from custom types, either fallibly
>> or infallibly.
>>
>> The address of registers can be direct, relative, or indexed, supporting
>> most of the patterns in which registers are arranged.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
>> ---
>>  rust/kernel/io.rs          |    1 +
>>  rust/kernel/io/register.rs | 1198 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 1199 insertions(+)
>>
>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
>> index a97eb44a9a87..eccaa176b6b9 100644
>> --- a/rust/kernel/io.rs
>> +++ b/rust/kernel/io.rs
>> @@ -11,6 +11,7 @@
>>  
>>  pub mod mem;
>>  pub mod poll;
>> +pub mod register;
>>  pub mod resource;
>>  
>>  pub use resource::Resource;
>> diff --git a/rust/kernel/io/register.rs b/rust/kernel/io/register.rs
>> new file mode 100644
>> index 000000000000..e414aebe4c86
>> --- /dev/null
>> +++ b/rust/kernel/io/register.rs
>> @@ -0,0 +1,1198 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +/// Defines a dedicated type for a register with an absolute offset, including getter and setter
>> +/// methods for its fields and methods to read and write it from an `Io` region.
>> +///
>> +/// A register is essentially a [`bitfield!`] with I/O capabilities. The syntax of the `register!`
>> +/// macro reflects that fact, being essentially identical to that of [`bitfield!`] with the
>> +/// addition of addressing information after the `@` token.
>> +///
>> +/// Example:
>> +///
>> +/// ```
>> +/// use kernel::register;
>> +///
>> +/// register!(pub BOOT_0(u32) @ 0x00000100, "Basic revision information about the chip" {
>> +///     7:4 major_revision, "Major revision of the chip";
>> +///     3:0 minor_revision, "Minor revision of the chip";
>> +/// });
>
> The comment is inserted as doc comment, but it uses the string syntax.
>
> I guess the idea is that you want write everything in a single line so you can
> visually align the fields? I think it
> looks fine on the fields, but the same-line documentation of the type itself
> looks a bit off.
>
> Something like this will definitely feel much more Rusty:
>
> register!(
>     /// Basic revision information about the chip.
>     pub struct BOOT_0(u32) @ 0x00000100 {
>         /// Major revision of the chip.
>         major_version: [7:4],
>         /// Minor revision of the chip.
>         ///
>         /// This would also allow you easily expand the documentation into
>         /// multiple lines!
>         ///
>         /// Perhaps useful to document some quirks about the register!
>         /// I know currently registers and their fields are very underdocumented
>         /// and they probably don't need multiple lines, but I hope that'll not
>         /// true in the future and we would have detailed docs in the driver --
>         /// in which case visually aligning becomes impossible anyway.
>         minor_version: [3:0],
>         //           ^~ closer to the variable syntax in Rust
>         //             ^~ I keep the hi:lo syntax which I suppose is to reflect Verilog.
>     }
> )

That would definitely be better, unfortunately since this is a
declarative macro it cannot match against comments, hence the current
syntax.

>
> Another top-level question I have is whether we should define multiple registers
> (perhaps all) in one macro invocation. That's the strategy of `tock-registers`
> crate which is widely used in embedded Rust.

I agree that would be nice, especially as it opens the way for better
syntax for e.g. register blocks with a base.

I am having trouble adding that to the current design though. Some rules
use the tt muncher pattern and these will greedily consume all tokens,
including those for the next register. I am a bit at my wit's end here,
but if you have an idea for how to do it nicely then by all means. :)

>
> (Although, they use a completely different strategy in generating register
> mapping; all registers are becoming fields in the same struct).
>
>> +/// ```
>> +///
>> +/// This defines a `BOOT_0` type which can be read or written from offset `0x100` of an `Io`
>> +/// region. For instance, `minor_revision` is made of the 4 least significant bits of the
>> +/// register. Each field can be accessed and modified using accessor
>> +/// methods:
>> +///
>> +/// ```no_run
>> +/// use kernel::register;
>> +/// use kernel::num::Bounded;
>> +///
>> +/// # register!(pub BOOT_0(u32) @ 0x00000100, "Basic revision information about the chip" {
>> +/// #     7:4 major_revision, "Major revision of the chip";
>> +/// #     3:0 minor_revision, "Minor revision of the chip";
>> +/// # });
>> +/// # fn test(bar: &kernel::io::Io) {
>> +/// // Read from the register's defined offset (0x100).
>> +/// let boot0 = BOOT_0::read(&bar);
>> +/// pr_info!("chip revision: {}.{}", boot0.major_revision().get(), boot0.minor_revision().get());
>> +///
>> +/// // Update some fields and write the value back.
>> +/// boot0
>> +///     .set_major_revision(Bounded::<u32, _>::new::<3>())
>> +///     .set_minor_revision(Bounded::<u32, _>::new::<10>())
>
> This looks very verbose...

Yes, it is a bit unfortunate (although real-life code is unlikely to be
that verbose if you look at the top patch).

And a bit frustrating as one could expect the first generic parameter to
be inferred, so we could write `Bounded::new<3>()`. But due to how the
`new` constructor of `Bounded` is declared (using one dedicated
impl block per dedicated type as we cannot use generics with const
code), the compiler requires the backing type to be explicitly
specified. :/

Once we have the ability to use const trait methods this can be improved
though.

>
>> +///     .write(&bar);
>> +///
>> +/// // Or, just read and update the register in a single step:
>> +/// BOOT_0::update(&bar, |r| r
>> +///     .set_major_revision(Bounded::<u32, _>::new::<3>())
>> +///     .set_minor_revision(Bounded::<u32, _>::new::<10>())
>> +/// );
>> +/// # }
>> +/// ```
>> +///
>> +/// 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.
>> +///
>> +/// Attributes can be applied to the generated struct. The `#[allow(non_camel_case_types)]`
>> +/// attribute is automatically added since register names typically use SCREAMING_CASE:
>> +///
>> +/// ```
>> +/// use kernel::register;
>> +///
>> +/// register! {
>> +///     pub STATUS(u32) @ 0x00000000, "Status register" {
>> +///         0:0 ready, "Device ready flag";
>> +///     }
>> +/// }
>> +/// ```
>> +///
>> +/// It is also possible to create an alias register by using the `=> ALIAS` syntax. This is useful
>> +/// for cases where a register's interpretation depends on the context:
>> +///
>> +/// ```
>> +/// use kernel::register;
>> +///
>> +/// register!(pub SCRATCH(u32) @ 0x00000200, "Scratch register" {
>> +///     31:0 value, "Raw value";
>> +/// });
>> +///
>> +/// register!(pub SCRATCH_BOOT_STATUS(u32) => SCRATCH, "Boot status of the firmware" {
>
> How about "as"?
>
> register!(pub SCRATCH_BOOT_STATUS(u32) as SCRATCH);

I'd rather not, this could be interpreted as the conversion being done
using the `as` keyword while we are using `Into`.

>
>> +///     0:0 completed, "Whether the firmware has completed booting";
>> +/// });
>> +/// ```
>> +///
>> +/// In this example, `SCRATCH_BOOT_STATUS` uses the same I/O address as `SCRATCH`, while also
>> +/// providing its own `completed` field.
>> +///
>> +/// ## Relative registers
>> +///
>> +/// A register can be defined as being accessible from a fixed offset of a provided base. For
>> +/// instance, imagine the following I/O space:
>> +///
>> +/// ```text
>> +///           +-----------------------------+
>> +///           |             ...             |
>> +///           |                             |
>> +///  0x100--->+------------CPU0-------------+
>> +///           |                             |
>> +///  0x110--->+-----------------------------+
>> +///           |           CPU_CTL           |
>> +///           +-----------------------------+
>> +///           |             ...             |
>> +///           |                             |
>> +///           |                             |
>> +///  0x200--->+------------CPU1-------------+
>> +///           |                             |
>> +///  0x210--->+-----------------------------+
>> +///           |           CPU_CTL           |
>> +///           +-----------------------------+
>> +///           |             ...             |
>> +///           +-----------------------------+
>> +/// ```
>> +///
>> +/// `CPU0` and `CPU1` both have a `CPU_CTL` register that starts at offset `0x10` of their I/O
>> +/// space segment. Since both instances of `CPU_CTL` share the same layout, we don't want to define
>> +/// them twice and would prefer a way to select which one to use from a single definition
>> +///
>> +/// This can be done using the `Base[Offset]` syntax when specifying the register's address.
>> +///
>> +/// `Base` is an arbitrary type (typically a ZST) to be used as a generic parameter of the
>> +/// [`RegisterBase`] trait to provide the base as a constant, i.e. each type providing a base for
>> +/// this register needs to implement `RegisterBase<Base>`. Here is the above example translated
>> +/// into code:
>> +///
>> +/// ```no_run
>> +/// use kernel::register;
>> +/// use kernel::io::register::RegisterBase;
>> +///
>> +/// // Type used to identify the base.
>> +/// pub struct CpuCtlBase;
>> +///
>> +/// // ZST describing `CPU0`.
>> +/// struct Cpu0;
>> +/// impl RegisterBase<CpuCtlBase> for Cpu0 {
>> +///     const BASE: usize = 0x100;
>> +/// }
>> +/// // Singleton of `CPU0` used to identify it.
>> +/// const CPU0: Cpu0 = Cpu0;
>> +///
>> +/// // ZST describing `CPU1`.
>> +/// struct Cpu1;
>> +/// impl RegisterBase<CpuCtlBase> for Cpu1 {
>> +///     const BASE: usize = 0x200;
>> +/// }
>> +/// // Singleton of `CPU1` used to identify it.
>> +/// const CPU1: Cpu1 = Cpu1;
>> +///
>> +/// # fn test(bar: &kernel::io::Io) {
>> +/// // This makes `CPU_CTL` accessible from all implementors of `RegisterBase<CpuCtlBase>`.
>> +/// register!(pub CPU_CTL(u32) @ CpuCtlBase[0x10], "CPU core control" {
>
> Maybe `CpuCtlBase + 0x10`? I think this means the offset is going to be differ
> by 0x10, not that it's going to be the 17th register?

Yup, I tend to agree with you on the readability side. This requires
changing some captures from `ty` to `ident` as `+` is not allowed after
`ty` though, which will forbid namespaces from being specified in the
type's path. Do you think this is an acceptable compromise?

>
>> +///     0:0 start, "Start the CPU core";
>> +/// });
>> +///
>> +/// // The `read`, `write` and `update` methods of relative registers take an extra `base` argument
>> +/// // that is used to resolve its final address by adding its `BASE` to the offset of the
>> +/// // register.
>> +///
>> +/// // Start `CPU0`.
>> +/// CPU_CTL::update(&bar, &CPU0, |r| r.set_start(true));
>> +///
>> +/// // Start `CPU1`.
>> +/// CPU_CTL::update(&bar, &CPU1, |r| r.set_start(true));
>> +///
>> +/// // Aliases can also be defined for relative register.
>> +/// register!(pub CPU_CTL_ALIAS(u32) => CpuCtlBase[CPU_CTL], "Alias to CPU core control" {
>> +///     1:1 alias_start, "Start the aliased CPU core";
>> +/// });
>> +///
>> +/// // Start the aliased `CPU0`.
>> +/// CPU_CTL_ALIAS::update(&bar, &CPU0, |r| r.set_alias_start(true));
>> +/// # }
>> +/// ```
>> +///
>> +/// ## Arrays of registers
>> +///
>> +/// Some I/O areas contain consecutive values that can be interpreted in the same way. These areas
>> +/// can be defined as an array of identical registers, allowing them to be accessed by index with
>> +/// compile-time or runtime bound checking. Simply define their address as `Address[Size]`, and add
>> +/// an `idx` parameter to their `read`, `write` and `update` methods:
>> +///
>> +/// ```no_run
>> +/// use kernel::register;
>> +///
>> +/// # fn no_run(bar: &kernel::io::Io) -> Result<(), Error> {
>> +/// # fn get_scratch_idx() -> usize {
>> +/// #   0x15
>> +/// # }
>> +/// // Array of 64 consecutive registers with the same layout starting at offset `0x80`.
>> +/// register!(pub SCRATCH(u32) @ 0x00000080[64], "Scratch registers" {
>
> This syntax is way to close to the base syntax above.
>
> Perhaps `pub SCRATCH([u32; 64]) @ 0x00000080` would make more sense?

So we also need to specify the optional stride of the array, which adds
one more parameter. We can go with the following though:

  pub SCRATCH(u32)[64] @ 0x00000080

or with the stride:

  pub SCRATCH(u32)[64; 16] @ 0x00000080

How does it look?

>
>> +///     31:0 value;
>> +/// });
>> +///
>> +/// // Read scratch register 0, i.e. I/O address `0x80`.
>> +/// let scratch_0 = SCRATCH::read(&bar, 0).value();
>> +/// // Read scratch register 15, i.e. I/O address `0x80 + (15 * 4)`.
>> +/// let scratch_15 = SCRATCH::read(&bar, 15).value();
>> +///
>> +/// // This is out of bounds and won't build.
>> +/// // let scratch_128 = SCRATCH::read(&bar, 128).value();
>> +///
>> +/// // Runtime-obtained array index.
>> +/// let scratch_idx = get_scratch_idx();
>> +/// // Access on a runtime index returns an error if it is out-of-bounds.
>> +/// let some_scratch = SCRATCH::try_read(&bar, scratch_idx)?.value();
>> +///
>> +/// // Alias to a particular register in an array.
>> +/// // Here `SCRATCH[8]` is used to convey the firmware exit code.
>> +/// register!(pub FIRMWARE_STATUS(u32) => SCRATCH[8], "Firmware exit status code" {
>> +///     7:0 status;
>> +/// });
>> +///
>> +/// let status = FIRMWARE_STATUS::read(&bar).status();
>> +///
>> +/// // Non-contiguous register arrays can be defined by adding a stride parameter.
>> +/// // Here, each of the 16 registers of the array are separated by 8 bytes, meaning that the
>> +/// // registers of the two declarations below are interleaved.
>> +/// register!(pub SCRATCH_INTERLEAVED_0(u32) @ 0x000000c0[16 ; 8], "Scratch registers bank 0" {
>> +///     31:0 value;
>> +/// });
>> +/// register!(pub SCRATCH_INTERLEAVED_1(u32) @ 0x000000c4[16 ; 8], "Scratch registers bank 1" {
>> +///     31:0 value;
>> +/// });
>> +/// # Ok(())
>> +/// # }
>> +/// ```
>> +///
>> +/// ## Relative arrays of registers
>> +///
>> +/// Combining the two features described in the sections above, arrays of registers accessible from
>> +/// a base can also be defined:
>> +///
>> +/// ```no_run
>> +/// use kernel::register;
>> +/// use kernel::io::register::RegisterBase;
>> +///
>> +/// # fn no_run(bar: &kernel::io::Io) -> Result<(), Error> {
>> +/// # fn get_scratch_idx() -> usize {
>> +/// #   0x15
>> +/// # }
>> +/// // Type used as parameter of `RegisterBase` to specify the base.
>> +/// pub struct CpuCtlBase;
>> +///
>> +/// // ZST describing `CPU0`.
>> +/// struct Cpu0;
>> +/// impl RegisterBase<CpuCtlBase> for Cpu0 {
>> +///     const BASE: usize = 0x100;
>> +/// }
>> +/// // Singleton of `CPU0` used to identify it.
>> +/// const CPU0: Cpu0 = Cpu0;
>> +///
>> +/// // ZST describing `CPU1`.
>> +/// struct Cpu1;
>> +/// impl RegisterBase<CpuCtlBase> for Cpu1 {
>> +///     const BASE: usize = 0x200;
>> +/// }
>> +/// // Singleton of `CPU1` used to identify it.
>> +/// const CPU1: Cpu1 = Cpu1;
>> +///
>> +/// // 64 per-cpu scratch registers, arranged as a contiguous array.
>> +/// register!(pub CPU_SCRATCH(u32) @ CpuCtlBase[0x00000080[64]], "Per-CPU scratch registers" {
>
> Ah... I don't like this.

If we adopt your syntax suggestions this should thankfully look much
better.

>
>> +///     31:0 value;
>> +/// });
>> +///
>> +/// let cpu0_scratch_0 = CPU_SCRATCH::read(&bar, &Cpu0, 0).value();
>> +/// let cpu1_scratch_15 = CPU_SCRATCH::read(&bar, &Cpu1, 15).value();
>> +///
>> +/// // This won't build.
>> +/// // let cpu0_scratch_128 = CPU_SCRATCH::read(&bar, &Cpu0, 128).value();
>> +///
>> +/// // Runtime-obtained array index.
>> +/// let scratch_idx = get_scratch_idx();
>> +/// // Access on a runtime value returns an error if it is out-of-bounds.
>> +/// let cpu0_some_scratch = CPU_SCRATCH::try_read(&bar, &Cpu0, scratch_idx)?.value();
>> +///
>> +/// // `SCRATCH[8]` is used to convey the firmware exit code.
>> +/// register!(pub CPU_FIRMWARE_STATUS(u32) => CpuCtlBase[CPU_SCRATCH[8]],
>> +///     "Per-CPU firmware exit status code" {
>> +///     7:0 status;
>> +/// });
>> +///
>> +/// let cpu0_status = CPU_FIRMWARE_STATUS::read(&bar, &Cpu0).status();
>> +///
>> +/// // Non-contiguous register arrays can be defined by adding a stride parameter.
>> +/// // Here, each of the 16 registers of the array are separated by 8 bytes, meaning that the
>> +/// // registers of the two declarations below are interleaved.
>> +/// register!(pub CPU_SCRATCH_INTERLEAVED_0(u32) @ CpuCtlBase[0x00000d00[16 ; 8]],
>
> For striding, SystemRDL uses syntax like this:
>
>     type name[len] @ addr += stride;
>
> which I think is better as this won't be confused with array length syntax in
> Rust.
>
> Crazy idea: is it possible to just use SystemRDL (maybe via a proc macro).

I wasn't familiar with SystemRDL, your comment made me take a look.
Interestingly some of the syntax is already close to that of this macro
(notably the use of `@`). That makes me think that we should try to
align when possible/relevant.

There are also lots of ideas that we could probably integrate (like
register blocks and address maps), but parsing the syntax as-is seems
overkill to me, and would delay this work quite a bit as we would
probably reach the limits of what we can do with declarative macros.

Powered by blists - more mailing lists