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(|_| ())
> + }
> +}
