Re: [PATCH] gpu: nova-core: falcon: store bar and dev in falcon

[Alexandre Courbot]

Hi Tim, thanks for the patch!

On Thu Jun 25, 2026 at 12:51 AM JST, Tim Kovalenko wrote:
> Store the bound device and `BAR0` mapping in `Falcon` instead of passing
> them through every `Falcon` operation. This simplifies the `Falcon` API and
> removes repeated `dev`/`bar` plumbing from reset, load, boot, mailbox, DMA,
> and GSP/FSP-specific Falcon helpers.
>
> Add a named helper for configuring Falcon FBIF transaction slots for
> physical coherent system memory, avoiding direct `BAR0` access from the
> FWSEC bootloader path without exposing BAR0 publicly.
>
> Future work / questions:

Questions are not something we want to appear in the final git log, so
please move such comments to after the `---` mark. If you use b4, this
is where the cover letter text will be placed on single-patch series.

>
>     - Focused only on the Falcon for this patch - more to follow.
>
>     - Not sure about the FalconHal and if I should modify the Trait
>         - Could be part of the next patch
>         - But it could definitively be simplified

Every HAL method takes the `Falcon` as a parameter, so it is able
to access its `bar` and `dev`. I think it makes sense to remove the
parameters there as well when possible.

>
>     - Also, how far should the refactor go and to what extend add new
>       methods to avoid passing `bar` or `dev` but also not exposing them as
>       pub. For `dev`, there's a lot of `dev_err` usage that requires us to
>       pass it as a param.
>         - I've created the `set_fbif_transcfg_phys_sysmem` method to
>           address such issue and remove some code duplication but that
>           method could be made a bit more generic.

If you add new methods, these should come as separate patches, and be
thoroughly thought - `set_fbif_transcfg_phys_sysmem` seems a bit too
ad-hoc to me. It looks like the proper fix for this access to
`NV_PFALCON_FBIF_TRANSCFG` is more something like moving it to
`Falcon::pio_load` or something, but this is a separate problem.

Let's keep the patch focused on `Falcon` itself, and keep the `bar`
argument of `FwsecFirmwareWithBl::run` until we figure out the right way
to address this. We don't need to clean up everything in one go.

>
>     - Also, is there a reason why we are not passing the `GspBootContext`
>       when we need stuff like `bar`, `dev` and both falcons?

`GspBootContext` was recently introduced, and yes we want to use it
more, but there are plans to make it generic and until we know what it
will look like I would like to limit its use. So for this patch, better
to keep things simple and mechanical.

>
> Reported-by: Alexandre Courbot <acourbot@xxxxxxxxxx>

This is not a bug, so `Suggested-by:` is more accurate I think.

> Closes: https://rust-for-linux.zulipchat.com/#narrow/channel/509436-Nova/topic/Storing.20driver-bound.20references.20into.20sub-devices/near/599137882

This can be `Link:`, as this patch by itself doesn't fix them all yet.

The patch itself clearly makes things more readable, and looks like what
I had in mind - modulo the addition of new methods that I would prefer
to see taken care of separately. If you can send a v2 that focuses
strictly on falcon and does not introduce new methods, I think that
would be perfect.