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

["Alexandre Courbot" <acourbot@nvidia.com>]

On Wed Jun 18, 2025 at 1:33 AM JST, Danilo Krummrich wrote:
> 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.

Agreed, done.

>
> <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?

Oops, for some reason I haven't addressed that part of your comment last
time, sorry about that. Added `// TIMEOUT:` statements above all calls
to `wait_on`. Note that sometimes the justification for these cannot be
more than "arbitrarily high value indicating something went wrong".

(similarly, I have added a `dma_handle_with_offset` method to
`CoherentAllocation` as I said I would in v4).

>
>> +            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?

Yes, although it is addressed in the next iteration of the register
macro (which I will send after this series), which supports register
arrays. Marked this as a TODO nonetheless.

>
>> +
>> +    // 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?

Do you mean why does `program_brom` calls `program_brom_ga102`? This is
so HAL methods can be re-used in other architectures. For instance,
Hopper's HAL be identical to Ampere save for `select_core`, so having
everything in separate functions allows the Hopper HAL to just call
`program_brom_ga102`. It's a sane convention to have IMHO, maybe we
should codify it via a HAL paragraph in the guidelines document?