Re: [PATCH v2 12/16] gpu: nova-core: use projection for PFALCON and PFALCON2 registers

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

On Thu Aug 6, 2026 at 1:35 AM JST, Gary Guo wrote:
> Add fixed size region types `PFalconRegisters` and `PFalcon2Registers` and
> update PFALCON and PFALCON registers to be fixed register on them and not

nit: second `PFALCON` should be `PFALCON2`.

> relative registers on `NovaRegisters`.
>
> Update `Falcon` struct to store projected views when constructing and
> access with `self.pfalcon` and `self.pfalcon2`.
>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---
>  drivers/gpu/nova-core/falcon.rs                    | 157 +++++++++------------
>  drivers/gpu/nova-core/falcon/fsp.rs                |  63 +++++----
>  drivers/gpu/nova-core/falcon/gsp.rs                |  51 ++++---
>  drivers/gpu/nova-core/falcon/hal/ga102.rs          |  62 ++++----
>  drivers/gpu/nova-core/falcon/hal/tu102.rs          |   9 +-
>  drivers/gpu/nova-core/falcon/sec2.rs               |  37 +++--
>  drivers/gpu/nova-core/firmware/fwsec/bootloader.rs |  18 +--
>  drivers/gpu/nova-core/gsp/hal/tu102.rs             |   7 +-
>  drivers/gpu/nova-core/regs.rs                      |  91 ++++++------
>  9 files changed, 238 insertions(+), 257 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index a91cbdd5d636..ed52572690ff 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -14,13 +14,12 @@
>      },
>      io::{
>          poll::read_poll_timeout,
> -        register::{
> -            RegisterBase,
> -            WithBase, //
> -        },
> +        register::Array,
>          Io,
> +        Mmio, //
>      },
>      prelude::*,
> +    sizes::SZ_4K,
>      time::Delta,
>  };
>  
> @@ -165,18 +164,22 @@ pub(crate) enum FalconFbifMemType with From<Bounded<u32, 1>> {
>      }
>  }
>  
> -/// Type used to represent the `PFALCON` registers address base for a given falcon engine.
> -pub(crate) struct PFalconBase(());
> +/// Type used to represent the `PFALCON` registers.
> +#[repr(align(4))]
> +#[derive(FromBytes, IntoBytes)]
> +pub(crate) struct PFalconRegisters([u8; SZ_4K]);
>  
> -/// Type used to represent the `PFALCON2` registers address base for a given falcon engine.
> -pub(crate) struct PFalcon2Base(());
> +/// Type used to represent the `PFALCON2` registers.
> +#[repr(align(4))]
> +#[derive(FromBytes, IntoBytes)]
> +pub(crate) struct PFalcon2Registers([u8; SZ_4K]);
>  
>  /// Trait defining the parameters of a given Falcon engine.
>  ///
>  /// Each engine provides one base for `PFALCON` and `PFALCON2` registers.
> -pub(crate) trait FalconEngine:
> -    Send + Sync + RegisterBase<PFalconBase> + RegisterBase<PFalcon2Base> + Sized
> -{
> +pub(crate) trait FalconEngine: Send + Sync + Sized {
> +    fn pfalcon(io: Bar0<'_>) -> Mmio<'_, PFalconRegisters>;
> +    fn pfalcon2(io: Bar0<'_>) -> Mmio<'_, PFalcon2Registers>;

Remember on v1 when we contemplated using associated consts? Turns out
we can with this version:

    // Need a better name, but you get the idea.
    pub(crate) type PFalconRegs = OffsetLoc<NovaRegisters, PFalconRegisters>;
    pub(crate) type PFalcon2Regs = OffsetLoc<NovaRegisters, PFalcon2Registers>;

    pub(crate) trait FalconEngine: Send + Sync + Sized {
        const PFALCON: PFalconRegs;
        const PFALCON2: PFalcon2Regs;
    }

... and make `Falcon::new` call `io_project` directly, and it works! At
the cost of importing `OffsetLoc` in `falcon.rs`, but that removes ~30
LoCs in total, and I'm not sure `OffsetLoc` should be hidden anyway.

>  }
>  
>  /// Represents a portion of the firmware to be loaded into a particular memory (e.g. IMEM or DMEM)
> @@ -358,6 +361,8 @@ pub(crate) struct Falcon<'a, E: FalconEngine> {
>      hal: KBox<dyn FalconHal<E>>,
>      dev: &'a device::Device<device::Bound>,
>      bar: Bar0<'a>,
> +    pub(crate) pfalcon: Mmio<'a, PFalconRegisters>,
> +    pfalcon2: Mmio<'a, PFalcon2Registers>,
>  }
>  
>  impl<'a, E: FalconEngine + 'static> Falcon<'a, E> {
> @@ -371,19 +376,19 @@ pub(crate) fn new(
>              hal: hal::falcon_hal(chipset)?,
>              dev,
>              bar,
> +            pfalcon: E::pfalcon(bar),
> +            pfalcon2: E::pfalcon2(bar),
>          })
>      }
>  
>      /// Resets DMA-related registers.
>      pub(crate) fn dma_reset(&self) {
> -        self.bar.update(regs::NV_PFALCON_FBIF_CTL::of::<E>(), |v| {
> +        self.pfalcon.update(regs::NV_PFALCON_FBIF_CTL, |v| {
>              v.with_allow_phys_no_ctx(true)
>          });
>  
> -        self.bar.write(
> -            WithBase::of::<E>(),
> -            regs::NV_PFALCON_FALCON_DMACTL::zeroed(),
> -        );
> +        self.pfalcon
> +            .write_reg(regs::NV_PFALCON_FALCON_DMACTL::zeroed());

Remembering the debates we had over how to address relative registers
when we ported registers to the new I/O scheme, I guess this new syntax
which should make everyone happy! :)

<...>
> diff --git a/drivers/gpu/nova-core/falcon/gsp.rs b/drivers/gpu/nova-core/falcon/gsp.rs
> index ae32f401aeb0..cbea6d7b49d3 100644
> --- a/drivers/gpu/nova-core/falcon/gsp.rs
> +++ b/drivers/gpu/nova-core/falcon/gsp.rs
> @@ -2,23 +2,24 @@
>  
>  use kernel::{
>      io::{
> +        io_project,
>          poll::read_poll_timeout,
> -        register::{
> -            RegisterBase,
> -            WithBase, //
> -        },
> +        register,
>          Io,
> +        Mmio, //
>      },
>      prelude::*,
>      time::Delta, //
>  };
>  
>  use crate::{
> +    driver::{
> +        Bar0,
> +        NovaRegisters, //
> +    },
>      falcon::{
>          Falcon,
> -        FalconEngine,
> -        PFalcon2Base,
> -        PFalconBase, //
> +        FalconEngine, //
>      },
>      regs,
>  };
> @@ -26,24 +27,31 @@
>  /// Type specifying the `Gsp` falcon engine. Cannot be instantiated.
>  pub(crate) struct Gsp(());
>  
> -impl RegisterBase<PFalconBase> for Gsp {
> -    const BASE: usize = 0x00110000;
> -}
> +register! {
> +    base: NovaRegisters;
>  
> -impl RegisterBase<PFalcon2Base> for Gsp {
> -    const BASE: usize = 0x00111000;
> +    PFALCON: super::PFalconRegisters @ 0x00110000;
> +    PFALCON2: super::PFalcon2Registers @ 0x00111000;
>  }
>  
> -impl FalconEngine for Gsp {}
> +impl FalconEngine for Gsp {
> +    #[inline]
> +    fn pfalcon<'a>(io: Bar0<'a>) -> Mmio<'a, super::PFalconRegisters> {
> +        io_project!(io, build: PFALCON)
> +    }
> +
> +    #[inline]
> +    fn pfalcon2<'a>(io: Bar0<'a>) -> Mmio<'a, super::PFalcon2Registers> {

The lifetime `'a` is elided in `fsp.rs` and `sec2.rs`, so we can also do
it here. But the point is moot if we switch to associated consts anyway.

> +        io_project!(io, build: PFALCON2)
> +    }
> +}
>  
>  impl<'a> Falcon<'a, Gsp> {
>      /// Clears the SWGEN0 bit in the Falcon's IRQ status clear register to
>      /// allow GSP to signal CPU for processing new messages in message queue.
>      pub(crate) fn clear_swgen0_intr(&self) {
> -        self.bar.write(
> -            WithBase::of::<Gsp>(),
> -            regs::NV_PFALCON_FALCON_IRQSCLR::zeroed().with_swgen0(true),
> -        );
> +        self.pfalcon
> +            .write_reg(regs::NV_PFALCON_FALCON_IRQSCLR::zeroed().with_swgen0(true));
>      }
>  
>      /// Checks if GSP reload/resume has completed during the boot process.
> @@ -59,8 +67,8 @@ pub(crate) fn check_reload_completed(&self, timeout: Delta) -> Result<bool> {
>  
>      /// Returns whether the RISC-V branch privilege lockdown bit is set.
>      pub(crate) fn riscv_branch_privilege_lockdown(&self) -> bool {
> -        self.bar
> -            .read(regs::NV_PFALCON_FALCON_HWCFG2::of::<Gsp>())
> +        self.pfalcon
> +            .read(regs::NV_PFALCON_FALCON_HWCFG2)
>              .riscv_br_priv_lockdown()
>      }
>  
> @@ -71,10 +79,7 @@ pub(crate) fn priv_target_mask_released(&self) -> bool {
>          const LOCKED_PATTERN: u32 = 0xbadf_4100;
>          const LOCKED_MASK: u32 = 0xffff_ff00;
>  
> -        let hwcfg2 = self
> -            .bar
> -            .read(regs::NV_PFALCON_FALCON_HWCFG2::of::<Gsp>())
> -            .into_raw();
> +        let hwcfg2 = self.pfalcon.read(regs::NV_PFALCON_FALCON_HWCFG2).into_raw();
>  
>          hwcfg2 != 0 && (hwcfg2 & LOCKED_MASK) != LOCKED_PATTERN
>      }
> diff --git a/drivers/gpu/nova-core/falcon/hal/ga102.rs b/drivers/gpu/nova-core/falcon/hal/ga102.rs
> index 7600ee07ca2e..ebfaff3d960f 100644
> --- a/drivers/gpu/nova-core/falcon/hal/ga102.rs
> +++ b/drivers/gpu/nova-core/falcon/hal/ga102.rs
> @@ -6,11 +6,9 @@
>      device,
>      io::{
>          poll::read_poll_timeout,
> -        register::{
> -            Array,
> -            WithBase, //
> -        },
> -        Io, //
> +        register::Array,
> +        Io,
> +        Mmio, //
>      },
>      prelude::*,
>      time::Delta, //
> @@ -24,6 +22,7 @@
>          FalconBromParams,
>          FalconEngine,
>          FalconModSelAlgo,
> +        PFalcon2Registers,
>          PeregrineCoreSelect, //
>      },
>      regs,
> @@ -31,17 +30,16 @@
>  
>  use super::FalconHal;
>  
> -fn select_core_ga102<E: FalconEngine>(bar: Bar0<'_>) -> Result {
> -    let bcr_ctrl = bar.read(regs::NV_PRISCV_RISCV_BCR_CTRL::of::<E>());
> +fn select_core_ga102<E: FalconEngine>(pfalcon2: Mmio<'_, PFalcon2Registers>) -> Result {

The generic parameter `E` is now unused and can be removed.

> +    let bcr_ctrl = pfalcon2.read(regs::NV_PRISCV_RISCV_BCR_CTRL);
>      if bcr_ctrl.core_select() != PeregrineCoreSelect::Falcon {
> -        bar.write(
> -            WithBase::of::<E>(),
> +        pfalcon2.write_reg(
>              regs::NV_PRISCV_RISCV_BCR_CTRL::zeroed().with_core_select(PeregrineCoreSelect::Falcon),
>          );
>  
>          // TIMEOUT: falcon core should take less than 10ms to report being enabled.
>          read_poll_timeout(
> -            || Ok(bar.read(regs::NV_PRISCV_RISCV_BCR_CTRL::of::<E>())),
> +            || Ok(pfalcon2.read(regs::NV_PRISCV_RISCV_BCR_CTRL)),
>              |r| r.valid(),
>              Delta::ZERO,
>              Delta::from_millis(10),
> @@ -86,24 +84,23 @@ fn signature_reg_fuse_version_ga102(
>      Ok(u16::BITS - reg_fuse_version.leading_zeros())
>  }
>  
> -fn program_brom_ga102<E: FalconEngine>(bar: Bar0<'_>, params: &FalconBromParams) {
> -    bar.write(
> -        WithBase::of::<E>().at(0),
> +fn program_brom_ga102<E: FalconEngine>(

Same here.

<...>
> @@ -359,7 +356,7 @@ pub(crate) fn usable_fb_size(self) -> u64 {
>          25:25   aincr => bool;
>      }
>  
> -    pub(crate) NV_PFALCON_FALCON_EMEMD(u32) @ PFalconBase + 0x00000ac4 {
> +    pub(crate) NV_PFALCON_FALCON_EMEMD(u32) @ 0x00000ac4 {
>          31:0    data => u32;
>      }
>  }
> @@ -385,13 +382,13 @@ pub(crate) fn with_falcon_mem(self, mem: FalconMem) -> Self {
>  
>  impl NV_PFALCON_FALCON_ENGINE {
>      /// Resets the falcon
> -    pub(crate) fn reset_engine<E: FalconEngine>(bar: Bar0<'_>) {
> -        bar.update(Self::of::<E>(), |r| r.with_reset(true));
> +    pub(crate) fn reset_engine<E: FalconEngine>(pfalcon: Mmio<'_, PFalconRegisters>) {

Here as well `E` can be dropped.