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 <[email protected]> > --- > 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.
