Message-ID: <d0ffb55b-690a-4a65-98b5-b83adebfd88b@arm.com>
Date: Mon, 28 Jul 2025 08:51:38 +0100
From: Steven Price <steven.price@....com>
To: Alexandre Courbot <acourbot@...dia.com>,
 Daniel Almeida <daniel.almeida@...labora.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, John Hubbard <jhubbard@...dia.com>,
 Nouveau <nouveau-bounces@...ts.freedesktop.org>
Subject: Re: [PATCH v2 01/19] gpu: nova-core: register: minor grammar and
 spelling fixes

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@...dia.com> wrote:
>>>
>>> From: John Hubbard <jhubbard@...dia.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@...dia.com>
>>> Cc: Danilo Krummrich <dakr@...nel.org>
>>> Signed-off-by: John Hubbard <jhubbard@...dia.com>
>>> Signed-off-by: Alexandre Courbot <acourbot@...dia.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

Powered by blists - more mailing lists