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 <[email protected]> wrote:
>>>
>>> From: John Hubbard <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Cc: Danilo Krummrich <[email protected]>
>>> Signed-off-by: John Hubbard <[email protected]>
>>> Signed-off-by: Alexandre Courbot <[email protected]>
>>> ---
>>> 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
