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

[Steven Price <steven.price@arm.com>]

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
});

In particular that would allow a try_() call to access the 'block' 
followed by normal read()/write() calls for the members in the block.

My Rust is certainly not good enough for me to try prototyping this
yet though!

Thanks,
Steve