Re: [PATCH 1/8] gpu: nova-core: convert PMC registers to kernel register macro

["Alexandre Courbot" <acourbot@nvidia.com>]

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@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).