Re: [PATCH v4 20/33] gpu: nova-core: Hopper/Blackwell: add FSP secure boot completion waiting

["Danilo Krummrich" <dakr@kernel.org>]

On Tue Feb 10, 2026 at 3:45 AM CET, John Hubbard wrote:
> +/// MCTP (Management Component Transport Protocol) header values for FSP communication.
> +pub(crate) mod mctp {
> +    pub(super) const HEADER_SOM: u32 = 1; // Start of Message
> +    pub(super) const HEADER_EOM: u32 = 1; // End of Message
> +    pub(super) const HEADER_SEID: u32 = 0; // Source Endpoint ID
> +    pub(super) const HEADER_SEQ: u32 = 0; // Sequence number

This looks like it should be a MctpMessageType enum instead.

> +
> +    pub(super) const MSG_TYPE_VENDOR_PCI: u32 = 0x7e;
> +    pub(super) const VENDOR_ID_NV: u32 = 0x10de;
> +    pub(super) const NVDM_TYPE_COT: u32 = 0x14;
> +    pub(super) const NVDM_TYPE_FSP_RESPONSE: u32 = 0x15;

This seems like it should be a different type (or even types) as they are
specific header field values.

> +}
> +
> +/// GSP FMC boot parameters structure.
> +/// This is what FSP expects to receive for booting GSP-RM.
> +/// GSP FMC initialization parameters.
> +#[repr(C)]
> +#[derive(Debug, Clone, Copy, Default)]
> +struct GspFmcInitParams {
> +    /// CC initialization "registry keys"
> +    regkeys: u32,
> +}
> +
> +// SAFETY: GspFmcInitParams is a simple C struct with only primitive types.
> +unsafe impl AsBytes for GspFmcInitParams {}
> +// SAFETY: All bit patterns are valid for the primitive fields.
> +unsafe impl FromBytes for GspFmcInitParams {}
> +
> +/// GSP ACR (Authenticated Code RAM) boot parameters.
> +#[repr(C)]
> +#[derive(Debug, Clone, Copy, Default)]
> +struct GspAcrBootGspRmParams {
> +    /// Physical memory aperture through which gspRmDescPa is accessed

For the whole series, please end them with a period. Personally, I don't care
too much, but it's a convention we have for Rust code.

> +/// FSP interface for Hopper/Blackwell GPUs.
> +pub(crate) struct Fsp;
> +
> +impl Fsp {
> +    /// Wait for FSP secure boot completion.
> +    ///
> +    /// Polls the thermal scratch register until FSP signals boot completion
> +    /// or timeout occurs.
> +    pub(crate) fn wait_secure_boot(
> +        dev: &device::Device<device::Bound>,
> +        bar: &crate::driver::Bar0,
> +        arch: crate::gpu::Architecture,
> +    ) -> Result<()> {

Please just use 'Result'.

> +        let timeout = Delta::from_millis(FSP_SECURE_BOOT_TIMEOUT_MS);
> +
> +        read_poll_timeout(
> +            || crate::regs::read_fsp_boot_complete_status(bar, arch),

Let's just import regs.

> +            |&status| {
> +                dev_dbg!(
> +                    dev,
> +                    "FSP I2CS scratch register status: {:#x} (expected: {:#x})\n",
> +                    status,
> +                    FSP_BOOT_COMPLETE_SUCCESS
> +                );

I don't think we should have print statements in the condition closure. Even for
debug prints we don't want to spam the console.

> +                status == FSP_BOOT_COMPLETE_SUCCESS
> +            },
> +            Delta::ZERO,
> +            timeout,
> +        )
> +        .map_err(|_| {
> +            let final_status =
> +                crate::regs::read_fsp_boot_complete_status(bar, arch).unwrap_or(0xDEADBEEF);

I think read_fsp_boot_complete_status() should just return an Option.

Also, do we really care about the actual value if we time out? If so, this won't
work reliably, i.e. final_status could have changed to
FSP_BOOT_COMPLETE_SUCCESS.

> +            dev_err!(
> +                dev,
> +                "FSP secure boot completion timeout - final status: {:#x}\n",
> +                final_status
> +            );
> +            ETIMEDOUT

Even if it is the wrong architecture we return ETIMEDOUT? Actually, since FSP is
for Hopper/Blackwell only/plus, we way even want to print a warning and use
debug_assert().

> +        })
> +        .map(|_| ())
> +    }
> +}