Re: [PATCH v3 14/33] gpu: nova-core: add the falcon DMA and suspend helpers for r000 boot

[Alexandre Courbot]

On Fri Sep 18, 2026 at 10:07 AM JST, John Hubbard wrote:
<...>
> +    /// Transfers `len` bytes from `src_addr` into this falcon's `target_mem`.
> +    ///
> +    /// `src_addr` is a GPU physical address reached through the FBIF aperture, so the caller must
> +    /// program `NV_PFALCON_FBIF_TRANSCFG` for `ctx_dma` before calling this.
> +    ///
> +    /// # Errors
> +    ///
> +    /// - `EINVAL` if `ctx_dma` is not a context DMA slot that the falcon has, or if `src_addr` is
> +    ///   not 256-byte aligned.
> +    /// - `ERANGE` if `src_addr` does not fit the `DMATRFBASE` register pair.
> +    /// - `EOVERFLOW` if a per-block source or destination offset exceeds `u32`.
> +    #[expect(dead_code)]
> +    pub(crate) fn raw_dma_transfer(
> +        &self,
> +        ctx_dma: u32,
> +        src_addr: u64,
> +        target_mem: FalconMem,
> +        src: FalconDmaSrcOffset,
> +        dst_offset: u32,
> +        len: u32,
> +    ) -> Result {

So this method is an almost identical rewrite of `dma_wr`, except it
works from the FB instead of sysmem. That's no justification for a new
method, and `dma_wr` can be made capable of working with FB: change the
`dma_obj` parameter to an enum type describing whether the source is FB
or sysmem, with the relevant parameters for each variant as one commit,
and bring the local improvements of your version (like the use of
`checked_add` and support for DMA contexts) as separate commits.

<...>
> +    /// Returns `true` if the RISC-V core has suspended.
> +    pub(crate) fn is_processor_suspended(&self) -> bool {
> +        const INTERRUPT_PROCESSOR_SUSPENDED: u32 = bits::bit_u32(31);
> +
> +        self.read_mailbox0() & INTERRUPT_PROCESSOR_SUSPENDED != 0
> +    }
> +
> +    /// Waits until the RISC-V core has suspended.
> +    ///
> +    /// The caller must write `MAILBOX0` before starting the core, or this returns as soon as it
> +    /// reads the previous suspend.
> +    ///
> +    /// # Errors
> +    ///
> +    /// - `ETIMEDOUT` if the core has not suspended within two seconds.
> +    #[expect(dead_code)]
> +    pub(crate) fn wait_for_processor_suspend(&self) -> Result {
> +        read_poll_timeout(
> +            || Ok(self.is_processor_suspended()),
> +            |suspended| *suspended,
> +            Delta::ZERO,
> +            Delta::from_secs(2),
> +        )
> +        .map(|_| ())
> +    }

These two methods look firmware specific, and are only used in
`boot.rs`, so I'd consider moving them there to not add ad-hoc code to
this file that applies to all falcons.

> +
>      /// Start the falcon CPU.
>      pub(crate) fn start(&self) -> Result<()> {
>          match self.pfalcon.read(regs::NV_PFALCON_FALCON_CPUCTL).alias_en() {
> diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
> index 4fb1b69ac9d5..8518248c9732 100644
> --- a/drivers/gpu/nova-core/gsp/boot.rs
> +++ b/drivers/gpu/nova-core/gsp/boot.rs
> @@ -2,7 +2,6 @@
>  // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
>  
>  use kernel::{
> -    bits,
>      io::poll::read_poll_timeout,
>      prelude::*,
>      time::Delta,
> @@ -94,10 +93,9 @@ fn shutdown_gsp(
>          cmdq.send_command(commands::UnloadingGuestDriver::new(mode))?;
>  
>          // Wait until GSP signals it is suspended.
> -        const LIBOS_INTERRUPT_PROCESSOR_SUSPENDED: u32 = bits::bit_u32(31);
>          read_poll_timeout(
> -            || Ok(gsp_falcon.read_mailbox0()),
> -            |&mb0| mb0 & LIBOS_INTERRUPT_PROCESSOR_SUSPENDED != 0,
> +            || Ok(gsp_falcon.is_processor_suspended()),
> +            |suspended| *suspended,

Yup, that bit seems to confirm the comment above. :)