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 <[email protected]> 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.
