Re: [PATCH v2 01/19] gpu: nova-core: register: minor grammar and spelling fixes

["Alexandre Courbot" <acourbot@nvidia.com>]

On Mon Jul 28, 2025 at 4:51 PM JST, Steven Price wrote:
> On 28/07/2025 05:59, Alexandre Courbot wrote:
>> Hi Daniel, thanks for the review!
>> 
>> On Sat Jul 26, 2025 at 1:14 AM JST, Daniel Almeida wrote:
>>> Hi Alex. Thank you and John for working on this in general. It will be useful
>>> for the whole ecosystem! :) 
>>>
>>>> On 18 Jul 2025, at 04:26, Alexandre Courbot <acourbot@nvidia.com> wrote:
>>>>
>>>> From: John Hubbard <jhubbard@nvidia.com>
>>>>
>>>> There is only one top-level macro in this file at the moment, but the
>>>> "macros.rs" file name allows for more. Change the wording so that it
>>>> will remain valid even if additional macros are added to the file.
>>>>
>>>> Fix a couple of spelling errors and grammatical errors, and break up a
>>>> run-on sentence, for clarity.
>>>>
>>>> Cc: Alexandre Courbot <acourbot@nvidia.com>
>>>> Cc: Danilo Krummrich <dakr@kernel.org>
>>>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>>> ---
>>>> drivers/gpu/nova-core/regs/macros.rs | 14 +++++++-------
>>>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
>>>> index cdf668073480ed703c89ffa8628f5c9de6494687..864d1e83bed2979f5661e038f4c9fd87d33f69a7 100644
>>>> --- a/drivers/gpu/nova-core/regs/macros.rs
>>>> +++ b/drivers/gpu/nova-core/regs/macros.rs
>>>> @@ -1,17 +1,17 @@
>>>> // SPDX-License-Identifier: GPL-2.0
>>>>
>>>> -//! Macro to define register layout and accessors.
>>>> +//! `register!` macro to define register layout and accessors.
>>>
>>> I would have kept this line as-is. Users will most likely know the name of the
>>> macro already. At this point, they will be looking for what it does, so
>>> mentioning "register" here is a bit redundant IMHO.
>>>
>>>> //!
>>>> //! A single register typically includes several fields, which are accessed through a combination
>>>> //! of bit-shift and mask operations that introduce a class of potential mistakes, notably because
>>>> //! not all possible field values are necessarily valid.
>>>> //!
>>>> -//! The macro in this module allow to define, using an intruitive and readable syntax, a dedicated
>>>> -//! type for each register with its own field accessors that can return an error is a field's value
>>>> -//! is invalid.
>>>> +//! The `register!` macro in this module provides an intuitive and readable syntax for defining a
>>>> +//! dedicated type for each register. Each such type comes with its own field accessors that can
>>>> +//! return an error if a field's value is invalid.
>>>>
>>>> -/// Defines a dedicated type for a register with an absolute offset, alongside with getter and
>>>> -/// setter methods for its fields and methods to read and write it from an `Io` region.
>>>> +/// 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.
>>>
>>> +cc Steven Price,
>>>
>>> Sorry for hijacking this patch, but I think that we should be more flexible and
>>> allow for non-literal offsets in the macro.
>>>
>>> In Tyr, for example, some of the offsets need to be computed at runtime, i.e.:
>>>
>>> +pub(crate) struct AsRegister(usize);
>>> +
>>> +impl AsRegister {
>>> +    fn new(as_nr: usize, offset: usize) -> Result<Self> {
>>> +        if as_nr >= 32 {
>>> +            Err(EINVAL)
>>> +        } else {
>>> +            Ok(AsRegister(mmu_as(as_nr) + offset))
>>> +        }
>>> +    }
>>>
>>> Or:
>>>
>>> +pub(crate) struct Doorbell(usize);
>>> +
>>> +impl Doorbell {
>>> +    pub(crate) fn new(doorbell_id: usize) -> Self {
>>> +        Doorbell(0x80000 + (doorbell_id * 0x10000))
>>> +    }
>>>
>>> I don't think this will work with the current macro, JFYI.
>> 
>> IIUC from the comments on the next patches, your need is covered with
>> the relative and array registers definitions, is that correct?
>
> My Rust is somewhat shaky, but I believe "non-contiguous register 
> arrays" will do what we want. Although I'll admit it would be neater for 
> the likes of the AS registers if there was a way to define a "block" of 
> registers and then use an array of blocks. Something vaguely like this 
> (excuse the poor Rust):
>
> register_block!(MMU_AS_CONTROL @ 0x2400[16 ; 64], "MMU Address Space registers" {
> 	register!(TRANSTAB @ 0x0000, "Translation table base address" {
> 		31:0	base as u32;
> 	});
> 	register!(MEMATTR @ 0x0008, "Memory attributes" {
> 		7:0	attr0 as u8;
> 		7:0	attr1 as u8;
> 		// ...
> 	});
> 	// More registers
> });

I can think of two ways to achieve something similar using the current
patchset:

- As you mentioned, a set of non-contiguous register arrays. This should
  work rather well, as you could just do
  `MMU_AS_CONTROL_MEMATTR::read(bar, 4)` to read the `MMU_AS_CONTROL_MEMATTR`
  register of the 5th instance, with compile-time bound validation. It's
  not what register arrays are for originally, but it does the job.

- As a set of relative offset registers sharing the same group. This is
  more in line with the idea of a register block, but it also means that
  each instance needs to have its own type declared, which is a bit
  cumbersome but can be mitigated with a macro. More inconvenient if the
  fact that you cannot address using a simple number anymore...

The idea of register blocks is interesting. I wonder how that would
translate in terms of access to invididual registers, i.e. does the
block end up just being a prefix into the full register name, or is it
something else? From your example declaration I picture that accesses
would look something like `MMU_AS_CONTROL[4]::MEMATTR::read(bar)`, which
ngl looks great, but I also cannot think of a construct that would allow
such a syntax... Happy to think more about it though.