Steven Price
2025-Jul-28 07:51 UTC
[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 at nvidia.com> wrote: >>> >>> From: John Hubbard <jhubbard at 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 at nvidia.com> >>> Cc: Danilo Krummrich <dakr at kernel.org> >>> Signed-off-by: John Hubbard <jhubbard at nvidia.com> >>> Signed-off-by: Alexandre Courbot <acourbot at 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
Alexandre Courbot
2025-Jul-28 11:43 UTC
[PATCH v2 01/19] gpu: nova-core: register: minor grammar and spelling fixes
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 at nvidia.com> wrote: >>>> >>>> From: John Hubbard <jhubbard at 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 at nvidia.com> >>>> Cc: Danilo Krummrich <dakr at kernel.org> >>>> Signed-off-by: John Hubbard <jhubbard at nvidia.com> >>>> Signed-off-by: Alexandre Courbot <acourbot at 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.