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

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

On Thu Mar 19, 2026 at 2:35 PM JST, Eliot Courtney wrote:
> On Wed Mar 18, 2026 at 5:06 PM JST, Alexandre Courbot wrote:
>> Convert all PFALCON, PFALCON2 and PRISCV registers to use the kernel's
>> register macro and update the code accordingly.
>>
>> Because they rely on the same types to implement relative registers,
>> they need to be updated in lockstep.
>>
>> nova-core's local register macro is now unused, so remove it.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>>  drivers/gpu/nova-core/falcon.rs                    | 333 +++++-----
>>  drivers/gpu/nova-core/falcon/gsp.rs                |  22 +-
>>  drivers/gpu/nova-core/falcon/hal/ga102.rs          |  55 +-
>>  drivers/gpu/nova-core/falcon/hal/tu102.rs          |  12 +-
>>  drivers/gpu/nova-core/falcon/sec2.rs               |  17 +-
>>  drivers/gpu/nova-core/firmware/fwsec/bootloader.rs |  19 +-
>>  drivers/gpu/nova-core/regs.rs                      | 350 +++++-----
>>  drivers/gpu/nova-core/regs/macros.rs               | 739 ---------------------
>>  8 files changed, 421 insertions(+), 1126 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
>> index 4721865f59d9..90afef40acd0 100644
>> --- a/drivers/gpu/nova-core/falcon.rs
>> +++ b/drivers/gpu/nova-core/falcon.rs
>> @@ -14,9 +14,14 @@
>>          DmaMask, //
>>      },
>>      io::{
>> -        poll::read_poll_timeout, //
>> +        poll::read_poll_timeout,
>> +        register::{
>> +            RegisterBase,
>> +            WithBase, //
>> +        },
>>          Io,
>>      },
>> +    num::Bounded,
>>      prelude::*,
>>      sync::aref::ARef,
>>      time::Delta,
>> @@ -33,7 +38,6 @@
>>          IntoSafeCast, //
>>      },
>>      regs,
>> -    regs::macros::RegisterBase, //
>>  };
>>  
>>  pub(crate) mod gsp;
>> @@ -44,11 +48,14 @@
>>  pub(crate) const MEM_BLOCK_ALIGNMENT: usize = 256;
>>  
>>  // TODO[FPRI]: Replace with `ToPrimitive`.
>> -macro_rules! impl_from_enum_to_u8 {
>> -    ($enum_type:ty) => {
>> -        impl From<$enum_type> for u8 {
>> +macro_rules! impl_from_enum_to_bounded {
>> +    ($enum_type:ty, $length:literal) => {
>> +        impl From<$enum_type> for Bounded<u32, $length> {
>>              fn from(value: $enum_type) -> Self {
>> -                value as u8
>> +                // Shift the value left by the number of unused bits.
>> +                let b = Bounded::<u32, 32>::from((value as u32) << (32 - $length));
>> +                // Shift back right to create a `Bounded` of the expected width.
>> +                b.shr::<{ 32 - $length }, $length>()
>>              }
>>          }
>>      };
>
> This can silently truncate stuff if we typo the wrong bounded size.
> Any reason not to use `Bounded::from_expr(value as u32)` for this?

`from_expr` is tricky to use because it assumes the compiler optimizer
has enough information to guarantee that the set of possible values will
fit into the `Bounded` - and drops a very obscure build-time error if
the proof cannot be established. So it is really for obvious cases like
`if x < 0x10 { Bounded::<u8, 4>::new(x) }`.

Here we are converting from an enum, and in my experience `from_expr`
does work, but I still prefer to avoid it if we can. The bit-shake
method is another way of obtaining the right `Bounded` but in this case
you are right we can lose data - although the use is purely local, and
temporary until the `TryFrom` and `Into` derive macros [1] are
available.

The "correct" way to do this meanwhile would be to generate a match
statement handling all valid values, but this is a bit more intrusive
for something that is temporary.

[1] https://lore.kernel.org/all/20260129-try-from-into-macro-v5-0-dd011008118c@gmail.com/

>
>> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
>> index 4ac4e9126db8..08d9a9697adc 100644
>> --- a/drivers/gpu/nova-core/regs.rs
>> +++ b/drivers/gpu/nova-core/regs.rs
>> @@ -1,13 +1,10 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  
>> -// Required to retain the original register names used by OpenRM, which are all capital snake case
>> -// but are mapped to types.
>> -#![allow(non_camel_case_types)]
>> -
>> -#[macro_use]
>> -pub(crate) mod macros;
>> -
>>  use kernel::{
>> +    io::{
>> +        register::WithBase,
>> +        Io, //
>> +    },
>>      prelude::*,
>>      time, //
>>  };
>> @@ -314,60 +311,147 @@ pub(crate) fn vga_workspace_addr(self) -> Option<u64> {
>>  
>>  // PFALCON
>>  
>> -register!(NV_PFALCON_FALCON_IRQSCLR @ PFalconBase[0x00000004] {
>> -    4:4     halt as bool;
>> -    6:6     swgen0 as bool;
>> -});
>> +nv_reg! {
>> +    NV_PFALCON_FALCON_IRQSCLR @ PFalconBase + 0x00000004 {
>> +        4:4     halt => bool;
>> +        6:6     swgen0 => bool;
>> +    }
>>  
>> -register!(NV_PFALCON_FALCON_MAILBOX0 @ PFalconBase[0x00000040] {
>> -    31:0    value as u32;
>> -});
>> +    NV_PFALCON_FALCON_MAILBOX0 @ PFalconBase + 0x00000040 {
>> +        31:0    value => u32;
>> +    }
>>  
>> -register!(NV_PFALCON_FALCON_MAILBOX1 @ PFalconBase[0x00000044] {
>> -    31:0    value as u32;
>> -});
>> +    NV_PFALCON_FALCON_MAILBOX1 @ PFalconBase + 0x00000044 {
>> +        31:0    value => u32;
>> +    }
>>  
>> -// Used to store version information about the firmware running
>> -// on the Falcon processor.
>> -register!(NV_PFALCON_FALCON_OS @ PFalconBase[0x00000080] {
>> -    31:0    value as u32;
>> -});
>> +    /// Used to store version information about the firmware running
>> +    /// on the Falcon processor.
>> +    NV_PFALCON_FALCON_OS @ PFalconBase + 0x00000080 {
>> +        31:0    value => u32;
>> +    }
>>  
>> -register!(NV_PFALCON_FALCON_RM @ PFalconBase[0x00000084] {
>> -    31:0    value as u32;
>> -});
>> +    NV_PFALCON_FALCON_RM @ PFalconBase + 0x00000084 {
>> +        31:0    value => u32;
>> +    }
>>  
>> -register!(NV_PFALCON_FALCON_HWCFG2 @ PFalconBase[0x000000f4] {
>> -    10:10   riscv as bool;
>> -    12:12   mem_scrubbing as bool, "Set to 0 after memory scrubbing is completed";
>> -    31:31   reset_ready as bool, "Signal indicating that reset is completed (GA102+)";
>> -});
>> +    NV_PFALCON_FALCON_HWCFG2 @ PFalconBase + 0x000000f4 {
>> +        10:10   riscv => bool;
>> +        /// Set to 0 after memory scrubbing is completed.
>> +        12:12   mem_scrubbing => bool;
>> +        /// Signal indicating that reset is completed (GA102+).
>> +        31:31   reset_ready => bool;
>> +    }
>>  
>> -impl NV_PFALCON_FALCON_HWCFG2 {
>> -    /// Returns `true` if memory scrubbing is completed.
>> -    pub(crate) fn mem_scrubbing_done(self) -> bool {
>> -        !self.mem_scrubbing()
>> +    NV_PFALCON_FALCON_CPUCTL @ PFalconBase + 0x00000100 {
>> +        1:1     startcpu => bool;
>> +        4:4     halted => bool;
>> +        6:6     alias_en => bool;
>> +    }
>> +
>> +    NV_PFALCON_FALCON_BOOTVEC @ PFalconBase + 0x00000104 {
>> +        31:0    value => u32;
>> +    }
>> +
>> +    NV_PFALCON_FALCON_DMACTL @ PFalconBase + 0x0000010c {
>> +        0:0     require_ctx => bool;
>> +        1:1     dmem_scrubbing => bool;
>> +        2:2     imem_scrubbing => bool;
>> +        6:3     dmaq_num;
>> +        7:7     secure_stat => bool;
>> +    }
>> +
>> +    NV_PFALCON_FALCON_DMATRFBASE @ PFalconBase + 0x00000110 {
>> +        31:0    base => u32;
>> +    }
>> +
>> +    NV_PFALCON_FALCON_DMATRFMOFFS @ PFalconBase + 0x00000114 {
>> +        23:0    offs;
>> +    }
>> +
>> +    NV_PFALCON_FALCON_DMATRFCMD @ PFalconBase + 0x00000118 {
>> +        0:0     full => bool;
>> +        1:1     idle => bool;
>> +        3:2     sec;
>> +        4:4     imem => bool;
>> +        5:5     is_write => bool;
>> +        10:8    size ?=> DmaTrfCmdSize;
>> +        14:12   ctxdma;
>> +        16:16   set_dmtag;
>> +    }
>> +
>> +    NV_PFALCON_FALCON_DMATRFFBOFFS @ PFalconBase + 0x0000011c {
>> +        31:0    offs => u32;
>> +    }
>> +
>> +    NV_PFALCON_FALCON_DMATRFBASE1 @ PFalconBase + 0x00000128 {
>> +        8:0     base;
>> +    }
>> +
>> +    NV_PFALCON_FALCON_HWCFG1 @ PFalconBase + 0x0000012c {
>> +        /// Core revision.
>> +        3:0     core_rev ?=> FalconCoreRev;
>> +        /// Security model.
>> +        5:4     security_model ?=> FalconSecurityModel;
>> +        /// Core revision subversion.
>> +        7:6     core_rev_subversion => FalconCoreRevSubversion;
>> +    }
>> +
>> +    NV_PFALCON_FALCON_CPUCTL_ALIAS @ PFalconBase + 0x00000130 {
>> +        1:1     startcpu => bool;
>> +    }
>> +
>> +    /// IMEM access control register. Up to 4 ports are available for IMEM access.
>> +    NV_PFALCON_FALCON_IMEMC[4, stride = 16] @ PFalconBase + 0x00000180 {
>> +        /// IMEM block and word offset.
>> +        15:0      offs;
>> +        /// Auto-increment on write.
>> +        24:24     aincw => bool;
>> +        /// Access secure IMEM.
>> +        28:28     secure => bool;
>> +    }
>> +
>> +    /// IMEM data register. Reading/writing this register accesses IMEM at the address
>> +    /// specified by the corresponding IMEMC register.
>> +    NV_PFALCON_FALCON_IMEMD[4, stride = 16] @ PFalconBase + 0x00000184 {
>> +        31:0      data;
>> +    }
>> +
>> +    /// IMEM tag register. Used to set the tag for the current IMEM block.
>> +    NV_PFALCON_FALCON_IMEMT[4, stride = 16] @ PFalconBase + 0x00000188 {
>> +        15:0      tag;
>> +    }
>> +
>> +    /// DMEM access control register. Up to 8 ports are available for DMEM access.
>> +    NV_PFALCON_FALCON_DMEMC[8, stride = 8] @ PFalconBase + 0x000001c0 {
>> +        /// DMEM block and word offset.
>> +        15:0      offs;
>> +        /// Auto-increment on write.
>> +        24:24     aincw => bool;
>> +    }
>> +
>> +    /// DMEM data register. Reading/writing this register accesses DMEM at the address
>> +    /// specified by the corresponding DMEMC register.
>> +    NV_PFALCON_FALCON_DMEMD[8, stride = 8] @ PFalconBase + 0x000001c4 {
>> +        31:0      data;
>> +    }
>> +
>> +    /// Actually known as `NV_PSEC_FALCON_ENGINE` and `NV_PGSP_FALCON_ENGINE` depending on the
>> +    /// falcon instance.
>> +    NV_PFALCON_FALCON_ENGINE @ PFalconBase + 0x000003c0 {
>> +        0:0     reset => bool;
>> +    }
>> +
>> +    NV_PFALCON_FBIF_TRANSCFG[8] @ PFalconBase + 0x00000600 {
>> +        1:0     target ?=> FalconFbifTarget;
>> +        2:2     mem_type => FalconFbifMemType;
>> +    }
>> +
>> +    NV_PFALCON_FBIF_CTL @ PFalconBase + 0x00000624 {
>> +        7:7     allow_phys_no_ctx => bool;
>>      }
>>  }
>>  
>> -register!(NV_PFALCON_FALCON_CPUCTL @ PFalconBase[0x00000100] {
>> -    1:1     startcpu as bool;
>> -    4:4     halted as bool;
>> -    6:6     alias_en as bool;
>> -});
>> -
>> -register!(NV_PFALCON_FALCON_BOOTVEC @ PFalconBase[0x00000104] {
>> -    31:0    value as u32;
>> -});
>> -
>> -register!(NV_PFALCON_FALCON_DMACTL @ PFalconBase[0x0000010c] {
>> -    0:0     require_ctx as bool;
>> -    1:1     dmem_scrubbing as bool;
>> -    2:2     imem_scrubbing as bool;
>> -    6:3     dmaq_num as u8;
>> -    7:7     secure_stat as bool;
>> -});
>> -
>>  impl NV_PFALCON_FALCON_DMACTL {
>>      /// Returns `true` if memory scrubbing is completed.
>>      pub(crate) fn mem_scrubbing_done(self) -> bool {
>> @@ -375,147 +459,81 @@ pub(crate) fn mem_scrubbing_done(self) -> bool {
>>      }
>>  }
>>  
>> -register!(NV_PFALCON_FALCON_DMATRFBASE @ PFalconBase[0x00000110] {
>> -    31:0    base as u32;
>> -});
>> -
>> -register!(NV_PFALCON_FALCON_DMATRFMOFFS @ PFalconBase[0x00000114] {
>> -    23:0    offs as u32;
>> -});
>> -
>> -register!(NV_PFALCON_FALCON_DMATRFCMD @ PFalconBase[0x00000118] {
>> -    0:0     full as bool;
>> -    1:1     idle as bool;
>> -    3:2     sec as u8;
>> -    4:4     imem as bool;
>> -    5:5     is_write as bool;
>> -    10:8    size as u8 ?=> DmaTrfCmdSize;
>> -    14:12   ctxdma as u8;
>> -    16:16   set_dmtag as u8;
>> -});
>> -
>>  impl NV_PFALCON_FALCON_DMATRFCMD {
>>      /// Programs the `imem` and `sec` fields for the given FalconMem
>>      pub(crate) fn with_falcon_mem(self, mem: FalconMem) -> Self {
>> -        self.set_imem(mem != FalconMem::Dmem)
>> -            .set_sec(if mem == FalconMem::ImemSecure { 1 } else { 0 })
>> +        let this = self.with_imem(mem != FalconMem::Dmem);
>> +
>> +        match mem {
>> +            FalconMem::ImemSecure => this.with_const_sec::<1>(),
>> +            _ => this.with_const_sec::<0>(),
>> +        }
>>      }
>>  }
>>  
>> -register!(NV_PFALCON_FALCON_DMATRFFBOFFS @ PFalconBase[0x0000011c] {
>> -    31:0    offs as u32;
>> -});
>> -
>> -register!(NV_PFALCON_FALCON_DMATRFBASE1 @ PFalconBase[0x00000128] {
>> -    8:0     base as u16;
>> -});
>> -
>> -register!(NV_PFALCON_FALCON_HWCFG1 @ PFalconBase[0x0000012c] {
>> -    3:0     core_rev as u8 ?=> FalconCoreRev, "Core revision";
>> -    5:4     security_model as u8 ?=> FalconSecurityModel, "Security model";
>> -    7:6     core_rev_subversion as u8 ?=> FalconCoreRevSubversion, "Core revision subversion";
>> -});
>> -
>> -register!(NV_PFALCON_FALCON_CPUCTL_ALIAS @ PFalconBase[0x00000130] {
>> -    1:1     startcpu as bool;
>> -});
>> -
>> -// IMEM access control register. Up to 4 ports are available for IMEM access.
>> -register!(NV_PFALCON_FALCON_IMEMC @ PFalconBase[0x00000180[4; 16]] {
>> -    15:0      offs as u16, "IMEM block and word offset";
>> -    24:24     aincw as bool, "Auto-increment on write";
>> -    28:28     secure as bool, "Access secure IMEM";
>> -});
>> -
>> -// IMEM data register. Reading/writing this register accesses IMEM at the address
>> -// specified by the corresponding IMEMC register.
>> -register!(NV_PFALCON_FALCON_IMEMD @ PFalconBase[0x00000184[4; 16]] {
>> -    31:0      data as u32;
>> -});
>> -
>> -// IMEM tag register. Used to set the tag for the current IMEM block.
>> -register!(NV_PFALCON_FALCON_IMEMT @ PFalconBase[0x00000188[4; 16]] {
>> -    15:0      tag as u16;
>> -});
>> -
>> -// DMEM access control register. Up to 8 ports are available for DMEM access.
>> -register!(NV_PFALCON_FALCON_DMEMC @ PFalconBase[0x000001c0[8; 8]] {
>> -    15:0      offs as u16, "DMEM block and word offset";
>> -    24:24     aincw as bool, "Auto-increment on write";
>> -});
>> -
>> -// DMEM data register. Reading/writing this register accesses DMEM at the address
>> -// specified by the corresponding DMEMC register.
>> -register!(NV_PFALCON_FALCON_DMEMD @ PFalconBase[0x000001c4[8; 8]] {
>> -    31:0      data as u32;
>> -});
>> -
>> -// Actually known as `NV_PSEC_FALCON_ENGINE` and `NV_PGSP_FALCON_ENGINE` depending on the falcon
>> -// instance.
>> -register!(NV_PFALCON_FALCON_ENGINE @ PFalconBase[0x000003c0] {
>> -    0:0     reset as bool;
>> -});
>> -
>>  impl NV_PFALCON_FALCON_ENGINE {
>>      /// Resets the falcon
>>      pub(crate) fn reset_engine<E: FalconEngine>(bar: &Bar0) {
>> -        Self::read(bar, &E::ID).set_reset(true).write(bar, &E::ID);
>> +        bar.update(Self::of::<E>(), |r| r.with_reset(true));
>>  
>>          // TIMEOUT: falcon engine should not take more than 10us to reset.
>>          time::delay::fsleep(time::Delta::from_micros(10));
>>  
>> -        Self::read(bar, &E::ID).set_reset(false).write(bar, &E::ID);
>> +        bar.update(Self::of::<E>(), |r| r.with_reset(false));
>>      }
>>  }
>>  
>> -register!(NV_PFALCON_FBIF_TRANSCFG @ PFalconBase[0x00000600[8]] {
>> -    1:0     target as u8 ?=> FalconFbifTarget;
>> -    2:2     mem_type as bool => FalconFbifMemType;
>> -});
>> -
>> -register!(NV_PFALCON_FBIF_CTL @ PFalconBase[0x00000624] {
>> -    7:7     allow_phys_no_ctx as bool;
>> -});
>> +impl NV_PFALCON_FALCON_HWCFG2 {
>> +    /// Returns `true` if memory scrubbing is completed.
>> +    pub(crate) fn mem_scrubbing_done(self) -> bool {
>> +        !self.mem_scrubbing()
>> +    }
>> +}
>>  
>>  /* PFALCON2 */
>>  
>> -register!(NV_PFALCON2_FALCON_MOD_SEL @ PFalcon2Base[0x00000180] {
>> -    7:0     algo as u8 ?=> FalconModSelAlgo;
>> -});
>> +nv_reg! {
>> +    NV_PFALCON2_FALCON_MOD_SEL @ PFalcon2Base + 0x00000180 {
>> +        7:0     algo ?=> FalconModSelAlgo;
>> +    }
>>  
>> -register!(NV_PFALCON2_FALCON_BROM_CURR_UCODE_ID @ PFalcon2Base[0x00000198] {
>> -    7:0    ucode_id as u8;
>> -});
>> +    NV_PFALCON2_FALCON_BROM_CURR_UCODE_ID @ PFalcon2Base + 0x00000198 {
>> +        7:0    ucode_id => u8;
>> +    }
>>  
>> -register!(NV_PFALCON2_FALCON_BROM_ENGIDMASK @ PFalcon2Base[0x0000019c] {
>> -    31:0    value as u32;
>> -});
>> +    NV_PFALCON2_FALCON_BROM_ENGIDMASK @ PFalcon2Base + 0x0000019c {
>> +        31:0    value => u32;
>> +    }
>>  
>> -// OpenRM defines this as a register array, but doesn't specify its size and only uses its first
>> -// element. Be conservative until we know the actual size or need to use more registers.
>> -register!(NV_PFALCON2_FALCON_BROM_PARAADDR @ PFalcon2Base[0x00000210[1]] {
>> -    31:0    value as u32;
>> -});
>> +    /// OpenRM defines this as a register array, but doesn't specify its size and only uses its
>> +    /// first element. Be conservative until we know the actual size or need to use more registers.
>> +    NV_PFALCON2_FALCON_BROM_PARAADDR[1] @ PFalcon2Base + 0x00000210 {
>> +        31:0    value => u32;
>> +    }
>> +}
>>  
>>  // PRISCV
>>  
>> -// RISC-V status register for debug (Turing and GA100 only).
>> -// Reflects current RISC-V core status.
>> -register!(NV_PRISCV_RISCV_CORE_SWITCH_RISCV_STATUS @ PFalcon2Base[0x00000240] {
>> -    0:0     active_stat as bool, "RISC-V core active/inactive status";
>> -});
>> -
>>  // GA102 and later
>> -register!(NV_PRISCV_RISCV_CPUCTL @ PFalcon2Base[0x00000388] {
>> -    0:0     halted as bool;
>> -    7:7     active_stat as bool;
>> -});
>> +nv_reg! {
>> +    /// RISC-V status register for debug (Turing and GA100 only).
>> +    /// Reflects current RISC-V core status.
>> +    NV_PRISCV_RISCV_CORE_SWITCH_RISCV_STATUS @ PFalcon2Base + 0x00000240 {
>> +        /// RISC-V core active/inactive status.
>> +        0:0     active_stat => bool;
>> +    }
>
> The above comment says "GA102 and later" but right after it has
> "Turing and GA100 only" which seems incongruous.

Right, this comment was for `NV_PRISCV_RISCV_CPUCTL` but it likely had a
copy/paste accident.