[PATCH 1/8] gpu: nova-core: convert PMC registers to kernel register macro
Alexandre Courbot
acourbot at nvidia.com
Thu Mar 19 07:18:08 PDT 2026
On Thu Mar 19, 2026 at 11:16 AM JST, Eliot Courtney wrote:
> On Thu Mar 19, 2026 at 11:07 AM JST, Alexandre Courbot wrote:
>> On Thu Mar 19, 2026 at 10:42 AM JST, Eliot Courtney wrote:
>>> On Wed Mar 18, 2026 at 5:05 PM JST, Alexandre Courbot wrote:
>>>> Convert all PMC registers to use the kernel's register macro and update
>>>> the code accordingly.
>>>>
>>>> nova-core's registers have some constant properties (like a 32-bit size
>>>> and a crate visibility), so introduce the `nv_reg` macro to shorten
>>>> their declaration.
>>>>
>>>> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
>>>> ---
>>>> drivers/gpu/nova-core/falcon.rs | 7 ++--
>>>> drivers/gpu/nova-core/gpu.rs | 37 ++++++++++-----------
>>>> drivers/gpu/nova-core/regs.rs | 73 +++++++++++++++++++++++++++++++----------
>>>> 3 files changed, 78 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
>>>> index 7097a206ec3c..4721865f59d9 100644
>>>> --- a/drivers/gpu/nova-core/falcon.rs
>>>> +++ b/drivers/gpu/nova-core/falcon.rs
>>>> @@ -13,7 +13,10 @@
>>>> DmaAddress,
>>>> DmaMask, //
>>>> },
>>>> - io::poll::read_poll_timeout,
>>>> + io::{
>>>> + poll::read_poll_timeout, //
>>>> + Io,
>>>> + },
>>>
>>> nit: // should be on the last import?
>>
>> It should, thanks.
>>
>>>
>>>> prelude::*,
>>>> sync::aref::ARef,
>>>> time::Delta,
>>>> @@ -532,7 +535,7 @@ pub(crate) fn reset(&self, bar: &Bar0) -> Result {
>>>> self.hal.reset_wait_mem_scrubbing(bar)?;
>>>>
>>>> regs::NV_PFALCON_FALCON_RM::default()
>>>> - .set_value(regs::NV_PMC_BOOT_0::read(bar).into())
>>>> + .set_value(bar.read(regs::NV_PMC_BOOT_0).into())
>>>> .write(bar, &E::ID);
>>>>
>>>> Ok(())
>>>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>>>> index 8579d632e717..d81abc7de3d7 100644
>>>> --- a/drivers/gpu/nova-core/gpu.rs
>>>> +++ b/drivers/gpu/nova-core/gpu.rs
>>>> @@ -4,6 +4,8 @@
>>>> device,
>>>> devres::Devres,
>>>> fmt,
>>>> + io::Io,
>>>> + num::Bounded,
>>>> pci,
>>>> prelude::*,
>>>> sync::Arc, //
>>>> @@ -129,24 +131,18 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>>>> }
>>>>
>>>> /// Enum representation of the GPU generation.
>>>> -///
>>>> -/// TODO: remove the `Default` trait implementation, and the `#[default]`
>>>> -/// attribute, once the register!() macro (which creates Architecture items) no
>>>> -/// longer requires it for read-only fields.
>>>> -#[derive(fmt::Debug, Default, Copy, Clone)]
>>>> -#[repr(u8)]
>>>> +#[derive(fmt::Debug, Copy, Clone)]
>>>> pub(crate) enum Architecture {
>>>> - #[default]
>>>> Turing = 0x16,
>>>> Ampere = 0x17,
>>>> Ada = 0x19,
>>>> }
>>>>
>>>> -impl TryFrom<u8> for Architecture {
>>>> +impl TryFrom<Bounded<u32, 6>> for Architecture {
>>>> type Error = Error;
>>>>
>>>> - fn try_from(value: u8) -> Result<Self> {
>>>> - match value {
>>>> + fn try_from(value: Bounded<u32, 6>) -> Result<Self> {
>>>> + match u8::from(value) {
>>>> 0x16 => Ok(Self::Turing),
>>>> 0x17 => Ok(Self::Ampere),
>>>> 0x19 => Ok(Self::Ada),
>>>> @@ -155,23 +151,26 @@ fn try_from(value: u8) -> Result<Self> {
>>>> }
>>>> }
>>>>
>>>> -impl From<Architecture> for u8 {
>>>> +impl From<Architecture> for Bounded<u32, 6> {
>>>> fn from(value: Architecture) -> Self {
>>>> - // CAST: `Architecture` is `repr(u8)`, so this cast is always lossless.
>>>> - value as u8
>>>> + match value {
>>>> + Architecture::Turing => Bounded::<u32, 6>::new::<0x16>(),
>>>> + Architecture::Ampere => Bounded::<u32, 6>::new::<0x17>(),
>>>> + Architecture::Ada => Bounded::<u32, 6>::new::<0x19>(),
>>>> + }
>>>> }
>>>> }
>>>>
>>>> pub(crate) struct Revision {
>>>> - major: u8,
>>>> - minor: u8,
>>>> + major: Bounded<u8, 4>,
>>>> + minor: Bounded<u8, 4>,
>>>> }
>>>>
>>>> impl From<regs::NV_PMC_BOOT_42> for Revision {
>>>> fn from(boot0: regs::NV_PMC_BOOT_42) -> Self {
>>>> Self {
>>>> - major: boot0.major_revision(),
>>>> - minor: boot0.minor_revision(),
>>>> + major: boot0.major_revision().cast(),
>>>> + minor: boot0.minor_revision().cast(),
>>>> }
>>>> }
>>>> }
>>>> @@ -208,13 +207,13 @@ fn new(dev: &device::Device, bar: &Bar0) -> Result<Spec> {
>>>> // from an earlier (pre-Fermi) era, and then using boot42 to precisely identify the GPU.
>>>> // Somewhere in the Rubin timeframe, boot0 will no longer have space to add new GPU IDs.
>>>>
>>>> - let boot0 = regs::NV_PMC_BOOT_0::read(bar);
>>>> + let boot0 = bar.read(regs::NV_PMC_BOOT_0);
>>>>
>>>> if boot0.is_older_than_fermi() {
>>>> return Err(ENODEV);
>>>> }
>>>>
>>>> - let boot42 = regs::NV_PMC_BOOT_42::read(bar);
>>>> + let boot42 = bar.read(regs::NV_PMC_BOOT_42);
>>>> Spec::try_from(boot42).inspect_err(|_| {
>>>> dev_err!(dev, "Unsupported chipset: {}\n", boot42);
>>>> })
>>>> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
>>>> index 53f412f0ca32..62c2065e63ef 100644
>>>> --- a/drivers/gpu/nova-core/regs.rs
>>>> +++ b/drivers/gpu/nova-core/regs.rs
>>>> @@ -35,20 +35,64 @@
>>>> num::FromSafeCast,
>>>> };
>>>>
>>>> +// All nova-core registers are 32-bit and `pub(crate)`. Wrap the `register!` macro to avoid
>>>> +// repeating this information for every register.
>>>> +macro_rules! nv_reg {
>>>> + (
>>>> + $(
>>>> + $(#[$attr:meta])* $name:ident $([ $size:expr $(, stride = $stride:expr)? ])?
>>>> + $(@ $offset:literal)?
>>>> + $(@ $base:ident + $base_offset:literal)?
>>>> + $(=> $alias:ident $(+ $alias_offset:ident)? $([$alias_idx:expr])? )?
>>>> + $(, $comment:literal)? { $($fields:tt)* }
>>>> + )*
>>>> + )=> {
>>>> + $(
>>>> + ::kernel::io::register!(
>>>> + @reg $(#[$attr])* pub(crate) $name(u32) $([$size $(, stride = $stride)?])?
>>>> + $(@ $offset)?
>>>> + $(@ $base + $base_offset)?
>>>> + $(=> $alias $(+ $alias_offset)? $([$alias_idx])? )?
>>>> + $(, $comment)? { $($fields)* }
>>>> + );
>>>> + )*
>>>> + };
>>>> +}
>>>> +
>>>
>>> Is it really worth introducing this macro to save pub(crate) and (u32)?
>>> Are we definitely going to always be using pub(crate) and u32?
>>
>> So far we are. I'm not particularly passionate about it, but I think
>> it's nice not having to repeat ourselves (and potentially introduce
>> typos).
>
> One downside is that the nested macros make it harder to check the
> implementation since you have to go to this definition, then to the
> register macro definition if you want to check something. And I also
> feel like I need to read this intermediate macro definition to see if
> it's doing anything special if I run into some error.
>
> Personally I would probably go with just using the register macro
> directly, and then we can tighten the visibility later if it's useful
> without having to change all of these. But not a super strong opinion,
> so up to you. Just doesn't feel like it saves us that much in typing the
> extra tens of characters.
IIRC Danilo also had reservations about `nv_reg` when we chatted about
it - I'll remove it from the next revision to see what it looks like.
More information about the linux-riscv
mailing list