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