Re: [PATCH v5 15/23] gpu: nova-core: add falcon register definitions and base code

[Danilo Krummrich <dakr@kernel.org>]

On Thu, Jun 12, 2025 at 11:01:43PM +0900, Alexandre Courbot wrote:
> +    /// Perform a DMA write according to `load_offsets` from `dma_handle` into the falcon's
> +    /// `target_mem`.
> +    ///
> +    /// `sec` is set if the loaded firmware is expected to run in secure mode.
> +    fn dma_wr(
> +        &self,
> +        bar: &Bar0,
> +        dma_handle: bindings::dma_addr_t,

I think we should pass &F from dma_load() rather than the raw handle.

<snip>

> +fn select_core_ga102<E: FalconEngine>(bar: &Bar0) -> Result {
> +    let bcr_ctrl = regs::NV_PRISCV_RISCV_BCR_CTRL::read(bar, E::BASE);
> +    if bcr_ctrl.core_select() != PeregrineCoreSelect::Falcon {
> +        regs::NV_PRISCV_RISCV_BCR_CTRL::default()
> +            .set_core_select(PeregrineCoreSelect::Falcon)
> +            .write(bar, E::BASE);
> +
> +        util::wait_on(Duration::from_millis(10), || {

As agreed, can you please add a brief comment to justify the timeout?

> +            let r = regs::NV_PRISCV_RISCV_BCR_CTRL::read(bar, E::BASE);
> +            if r.valid() {
> +                Some(())
> +            } else {
> +                None
> +            }
> +        })?;
> +    }
> +
> +    Ok(())
> +}
> +
> +fn signature_reg_fuse_version_ga102(
> +    dev: &device::Device,
> +    bar: &Bar0,
> +    engine_id_mask: u16,
> +    ucode_id: u8,
> +) -> Result<u32> {
> +    // The ucode fuse versions are contained in the FUSE_OPT_FPF_<ENGINE>_UCODE<X>_VERSION
> +    // registers, which are an array. Our register definition macros do not allow us to manage them
> +    // properly, so we need to hardcode their addresses for now.

Sounds like a TODO?

> +
> +    // Each engine has 16 ucode version registers numbered from 1 to 16.
> +    if ucode_id == 0 || ucode_id > 16 {
> +        dev_err!(dev, "invalid ucode id {:#x}", ucode_id);
> +        return Err(EINVAL);
> +    }
> +
> +    // Base address of the FUSE registers array corresponding to the engine.
> +    let reg_fuse_base = if engine_id_mask & 0x0001 != 0 {
> +        regs::NV_FUSE_OPT_FPF_SEC2_UCODE1_VERSION::OFFSET
> +    } else if engine_id_mask & 0x0004 != 0 {
> +        regs::NV_FUSE_OPT_FPF_NVDEC_UCODE1_VERSION::OFFSET
> +    } else if engine_id_mask & 0x0400 != 0 {
> +        regs::NV_FUSE_OPT_FPF_GSP_UCODE1_VERSION::OFFSET
> +    } else {
> +        dev_err!(dev, "unexpected engine_id_mask {:#x}", engine_id_mask);
> +        return Err(EINVAL);
> +    };
> +
> +    // Read `reg_fuse_base[ucode_id - 1]`.
> +    let reg_fuse_version =
> +        bar.read32(reg_fuse_base + ((ucode_id - 1) as usize * core::mem::size_of::<u32>()));
> +
> +    Ok(fls_u32(reg_fuse_version))
> +}
> +
> +fn program_brom_ga102<E: FalconEngine>(bar: &Bar0, params: &FalconBromParams) -> Result {
> +    regs::NV_PFALCON2_FALCON_BROM_PARAADDR::default()
> +        .set_value(params.pkc_data_offset)
> +        .write(bar, E::BASE);
> +    regs::NV_PFALCON2_FALCON_BROM_ENGIDMASK::default()
> +        .set_value(params.engine_id_mask as u32)
> +        .write(bar, E::BASE);
> +    regs::NV_PFALCON2_FALCON_BROM_CURR_UCODE_ID::default()
> +        .set_ucode_id(params.ucode_id)
> +        .write(bar, E::BASE);
> +    regs::NV_PFALCON2_FALCON_MOD_SEL::default()
> +        .set_algo(FalconModSelAlgo::Rsa3k)
> +        .write(bar, E::BASE);
> +
> +    Ok(())
> +}
> +
> +pub(super) struct Ga102<E: FalconEngine>(PhantomData<E>);
> +
> +impl<E: FalconEngine> Ga102<E> {
> +    pub(super) fn new() -> Self {
> +        Self(PhantomData)
> +    }
> +}
> +
> +impl<E: FalconEngine> FalconHal<E> for Ga102<E> {
> +    fn select_core(&self, _falcon: &Falcon<E>, bar: &Bar0) -> Result {
> +        select_core_ga102::<E>(bar)
> +    }
> +
> +    fn signature_reg_fuse_version(
> +        &self,
> +        falcon: &Falcon<E>,
> +        bar: &Bar0,
> +        engine_id_mask: u16,
> +        ucode_id: u8,
> +    ) -> Result<u32> {
> +        signature_reg_fuse_version_ga102(&falcon.dev, bar, engine_id_mask, ucode_id)
> +    }
> +
> +    fn program_brom(&self, _falcon: &Falcon<E>, bar: &Bar0, params: &FalconBromParams) -> Result {
> +        program_brom_ga102::<E>(bar, params)
> +    }

Why are those two separate functions?