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

["Eliot Courtney" <ecourtney@nvidia.com>]

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?

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