Message-Id: <D0DDB132-68E8-43D0-B7EB-AA607C9BB72F@collabora.com>
Date: Fri, 25 Jul 2025 16:12:27 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Danilo Krummrich <dakr@...nel.org>,
 David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>,
 Beata Michalska <beata.michalska@....com>,
 nouveau@...ts.freedesktop.org,
 dri-devel@...ts.freedesktop.org,
 rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 17/19] gpu: nova-core: register: add support for
 register arrays

Hi Alex,

> On 18 Jul 2025, at 04:26, Alexandre Courbot <acourbot@...dia.com> wrote:
> 
> Having registers that can be interpreted identically in a contiguous I/O
> area (or at least, following a given stride) is a common way to organize
> registers, and is used by NVIDIA hardware. Thus, add a way to simply and
> safely declare such a layout using the register!() macro.
> 
> Build-time bound-checking is effective for array accesses performed with
> a constant. For cases where the index cannot be known at compile time,
> `try_` variants of the accessors are also made available that return
> `EINVAL` if the access is out-of-bounds.
> 
> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
> ---
> drivers/gpu/nova-core/gpu.rs         |   2 +-
> drivers/gpu/nova-core/regs.rs        |  15 +--
> drivers/gpu/nova-core/regs/macros.rs | 195 +++++++++++++++++++++++++++++++++++
> 3 files changed, 204 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 72d40b0124f0c1a2a381484172c289af523511df..325484ecdaf03d4dcdc4ac2aecc10ca763f442db 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -221,7 +221,7 @@ fn run_fwsec_frts(
>         fwsec_frts.run(dev, falcon, bar)?;
> 
>         // SCRATCH_E contains the error code for FWSEC-FRTS.
> -        let frts_status = regs::NV_PBUS_SW_SCRATCH_0E::read(bar).frts_err_code();
> +        let frts_status = regs::NV_PBUS_SW_SCRATCH_0E_FRTS_ERR::read(bar).frts_err_code();
>         if frts_status != 0 {
>             dev_err!(
>                 dev,
> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
> index 35d796b744e933ad70245b50e6eff861b429c519..0c857842b31f9ca5d842ee5b1e5841de480d1f1f 100644
> --- a/drivers/gpu/nova-core/regs.rs
> +++ b/drivers/gpu/nova-core/regs.rs
> @@ -44,8 +44,10 @@ pub(crate) fn chipset(self) -> Result<Chipset> {
> 
> // PBUS
> 
> -// TODO[REGA]: this is an array of registers.
> -register!(NV_PBUS_SW_SCRATCH_0E@...0001438  {
> +register!(NV_PBUS_SW_SCRATCH @ 0x00001400[64]  {});
> +
> +register!(NV_PBUS_SW_SCRATCH_0E_FRTS_ERR => NV_PBUS_SW_SCRATCH[0xe],
> +    "scratch register 0xe used as FRTS firmware error code" {
>     31:16   frts_err_code as u16;
> });
> 
> @@ -123,13 +125,12 @@ pub(crate) fn higher_bound(self) -> u64 {
>     0:0     read_protection_level0 as bool, "Set after FWSEC lowers its protection level";
> });
> 
> -// TODO[REGA]: This is an array of registers.
> -register!(NV_PGC6_AON_SECURE_SCRATCH_GROUP_05 @ 0x00118234 {
> -    31:0    value as u32;
> -});
> +// OpenRM defines this as a register array, but doesn't specify its size and only uses its first
> +// element. Be conservative until we know the actual size or need to use more registers.
> +register!(NV_PGC6_AON_SECURE_SCRATCH_GROUP_05 @ 0x00118234[1] {});
> 
> register!(
> -    NV_PGC6_AON_SECURE_SCRATCH_GROUP_05_0_GFW_BOOT => NV_PGC6_AON_SECURE_SCRATCH_GROUP_05,
> +    NV_PGC6_AON_SECURE_SCRATCH_GROUP_05_0_GFW_BOOT => NV_PGC6_AON_SECURE_SCRATCH_GROUP_05[0],
>     "Scratch group 05 register 0 used as GFW boot progress indicator" {
>         7:0    progress as u8, "Progress of GFW boot (0xff means completed)";
>     }
> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
> index 3465fb302ce921ca995ecbb71b83efe1c9a62a1d..0b5ccc50967b1deb02cf927142d5f422141e780d 100644
> --- a/drivers/gpu/nova-core/regs/macros.rs
> +++ b/drivers/gpu/nova-core/regs/macros.rs
> @@ -162,6 +162,57 @@ pub(crate) trait RegisterBase<T> {
> /// // Start the aliased `CPU0`.
> /// CPU_CTL_ALIAS::alter(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 `alter` methods:
> +///
> +/// ```no_run
> +/// # fn no_run() -> Result<(), Error> {
> +/// # fn get_scratch_idx() -> usize {
> +/// #   0x15
> +/// # }
> +/// // Array of 64 consecutive registers with the same layout starting at offset `0x80`.
> +/// register!(SCRATCH @ 0x00000080[64], "Scratch registers" {
> +///     31:0    value as u32;
> +/// });
> +///
> +/// // 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();

Ahhhhh, maybe this is what I have been looking for all along.

Alright, this is great! :)

> +///
> +/// // 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();

Awesome.

> +///
> +/// // Alias to a particular register in an array.
> +/// // Here `SCRATCH[8]` is used to convey the firmware exit code.
> +/// register!(FIRMWARE_STATUS => SCRATCH[8], "Firmware exit status code" {
> +///     7:0     status as u8;
> +/// });
> +///
> +/// 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!(SCRATCH_INTERLEAVED_0 @ 0x000000c0[16 ; 8], "Scratch registers bank 0" {
> +///     31:0    value as u32;
> +/// });
> +/// register!(SCRATCH_INTERLEAVED_1 @ 0x000000c4[16 ; 8], "Scratch registers bank 1" {
> +///     31:0    value as u32;
> +/// });
> +/// # Ok(())
> +/// # }
> +/// ```
> macro_rules! register {
>     // Creates a register at a fixed offset of the MMIO space.
>     ($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* } ) => {
> @@ -187,6 +238,35 @@ macro_rules! register {
>         register!(@io_relative $name @ $base [ $alias::OFFSET ]);
>     };
> 
> +    // Creates an array of registers at a fixed offset of the MMIO space.
> +    (
> +        $name:ident @ $offset:literal [ $size:expr ; $stride:expr ] $(, $comment:literal)? {
> +            $($fields:tt)*
> +        }
> +    ) => {
> +        static_assert!(::core::mem::size_of::<u32>() <= $stride);

Perhaps a TODO here would be nice, since you’ll want to change it when/if
this macros get to support non-u32 types (which is apparently on the roadmap
IIUC).

> +        register!(@core $name $(, $comment)? { $($fields)* } );
> +        register!(@io_array $name @ $offset [ $size ; $stride ]);
> +    };
> +
> +    // Shortcut for contiguous array of registers (stride == size of element).
> +    (
> +        $name:ident @ $offset:literal [ $size:expr ] $(, $comment:literal)? {
> +            $($fields:tt)*
> +        }
> +    ) => {
> +        register!($name @ $offset [ $size ; ::core::mem::size_of::<u32>() ] $(, $comment)? {

Same here.

> +            $($fields)*
> +        } );
> +    };
> +
> +    // Creates an alias of register `idx` of array of registers `alias` with its own fields.
> +    ($name:ident => $alias:ident [ $idx:expr ] $(, $comment:literal)? { $($fields:tt)* }) => {
> +        static_assert!($idx < $alias::SIZE);
> +        register!(@core $name $(, $comment)? { $($fields)* } );
> +        register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );

Why is this @io_fixed?

> +    };
> +
>     // All rules below are helpers.
> 
>     // Defines the wrapper `$name` type, as well as its relevant implementations (`Debug`,
> @@ -520,4 +600,119 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
>             }
>         }
>     };
> +
> +    // Generates the IO accessors for an array of registers.
> +    (@io_array $name:ident @ $offset:literal [ $size:expr ; $stride:expr ]) => {
> +        #[allow(dead_code)]
> +        impl $name {
> +            pub(crate) const OFFSET: usize = $offset;
> +            pub(crate) const SIZE: usize = $size;
> +            pub(crate) const STRIDE: usize = $stride;
> +
> +            /// Read the array register at index `idx` from its address in `io`.
> +            #[inline(always)]
> +            pub(crate) fn read<const SIZE: usize, T>(
> +                io: &T,
> +                idx: usize,
> +            ) -> Self where
> +                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> +            {
> +                build_assert!(idx < Self::SIZE);
> +
> +                let offset = Self::OFFSET + (idx * Self::STRIDE);
> +                let value = io.read32(offset);
> +
> +                Self(value)
> +            }
> +
> +            /// Write the value contained in `self` to the array register with index `idx` in `io`.
> +            #[inline(always)]
> +            pub(crate) fn write<const SIZE: usize, T>(
> +                self,
> +                io: &T,
> +                idx: usize
> +            ) where
> +                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> +            {
> +                build_assert!(idx < Self::SIZE);
> +
> +                let offset = Self::OFFSET + (idx * Self::STRIDE);
> +
> +                io.write32(self.0, offset);
> +            }
> +
> +            /// Read the array register at index `idx` in `io` and run `f` on its value to obtain a
> +            /// new value to write back.
> +            #[inline(always)]
> +            pub(crate) fn alter<const SIZE: usize, T, F>(
> +                io: &T,
> +                idx: usize,
> +                f: F,
> +            ) where
> +                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> +                F: ::core::ops::FnOnce(Self) -> Self,
> +            {
> +                let reg = f(Self::read(io, idx));
> +                reg.write(io, idx);
> +            }
> +
> +            /// Read the array register at index `idx` from its address in `io`.
> +            ///
> +            /// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
> +            /// access was out-of-bounds.
> +            #[inline(always)]
> +            pub(crate) fn try_read<const SIZE: usize, T>(
> +                io: &T,
> +                idx: usize,
> +            ) -> ::kernel::error::Result<Self> where
> +                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> +            {
> +                if idx < Self::SIZE {
> +                    Ok(Self::read(io, idx))
> +                } else {
> +                    Err(EINVAL)
> +                }
> +            }
> +
> +            /// Write the value contained in `self` to the array register with index `idx` in `io`.
> +            ///
> +            /// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
> +            /// access was out-of-bounds.
> +            #[inline(always)]
> +            pub(crate) fn try_write<const SIZE: usize, T>(
> +                self,
> +                io: &T,
> +                idx: usize,
> +            ) -> ::kernel::error::Result where
> +                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> +            {
> +                if idx < Self::SIZE {
> +                    Ok(self.write(io, idx))
> +                } else {
> +                    Err(EINVAL)
> +                }
> +            }
> +
> +            /// Read the array register at index `idx` in `io` and run `f` on its value to obtain a
> +            /// new value to write back.
> +            ///
> +            /// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
> +            /// access was out-of-bounds.
> +            #[inline(always)]
> +            pub(crate) fn try_alter<const SIZE: usize, T, F>(
> +                io: &T,
> +                idx: usize,
> +                f: F,
> +            ) -> ::kernel::error::Result where
> +                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
> +                F: ::core::ops::FnOnce(Self) -> Self,
> +            {
> +                if idx < Self::SIZE {
> +                    Ok(Self::alter(io, idx, f))
> +                } else {
> +                    Err(EINVAL)
> +                }
> +            }
> +        }
> +    };
> }
> 
> -- 
> 2.50.1
> 

Assuming that the @io_fixed stuff is correct:

Reviewed-by: Daniel Almeida <daniel.almeida@...labora.com>

— Daniel

Powered by blists - more mailing lists