[PATCH v4 14/20] gpu: nova-core: add falcon register definitions and base code

[Alexandre Courbot]

Hi Lyude, thanks for the review!

On Sat May 31, 2025 at 7:22 AM JST, Lyude Paul wrote:
<snip>
>> +    /// `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,
>> +        target_mem: FalconMem,
>> +        load_offsets: FalconLoadTarget,
>> +        sec: bool,
>> +    ) -> Result {
>> +        const DMA_LEN: u32 = 256;
>> +
>> +        // For IMEM, we want to use the start offset as a virtual address tag for each page, since
>> +        // code addresses in the firmware (and the boot vector) are virtual.
>> +        //
>> +        // For DMEM we can fold the start offset into the DMA handle.
>> +        let (src_start, dma_start) = match target_mem {
>> +            FalconMem::Imem => (load_offsets.src_start, dma_handle),
>> +            FalconMem::Dmem => (
>> +                0,
>> +                dma_handle + load_offsets.src_start as bindings::dma_addr_t,
>
> I wonder if maybe we shouldn't use dma_addr_t directly from bindings and add a
> proper type alias for it somewhere?

I guess so, let me see if I can easily change CoherentAllocation to
support that (and address Danilo's related comment).

>> +pub(crate) trait FalconHal<E: FalconEngine>: Sync {
>> +    // Activates the Falcon core if the engine is a risvc/falcon dual engine.
>> +    fn select_core(&self, _falcon: &Falcon<E>, _bar: &Bar0) -> Result<()> {
>
> Could just be Result
>
>> +        Ok(())
>> +    }
>> +
>> +    /// Returns the fused version of the signature to use in order to run a HS firmware on this
>> +    /// falcon instance. `engine_id_mask` and `ucode_id` are obtained from the firmware header.
>> +    fn get_signature_reg_fuse_version(
>> +        &self,
>> +        falcon: &Falcon<E>,
>> +        bar: &Bar0,
>> +        engine_id_mask: u16,
>> +        ucode_id: u8,
>> +    ) -> Result<u32>;
>> +
>> +    // Program the boot ROM registers prior to starting a secure firmware.
>> +    fn program_brom(&self, falcon: &Falcon<E>, bar: &Bar0, params: &FalconBromParams)
>> +        -> Result<()>;
>
> Same here

Urrk I've done this throughout the code. Fixed them all, thanks.