Attention is currently required from: Martin Roth, Subrata Banik, Andrey Petrov, Patrick Rudolph. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52866 )
Change subject: drivers/intel/fsp2_0: Add mb hooks before & after FSP calls ......................................................................
Patch Set 1:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/52866/comment/b2617892_e9afda7f PS1, Line 9: There are currently various callbacks and hooks for chipsets and : mainboards in various places around the FSP calls, but I need some : mainboard hooks immediately before and after the FSP calls. From the follow-up changes, it looks like you are using these callbacks for configuring pads before the FSP call. This is currently achieved by using the calls:
`platform_fsp_memory_init_params_cb` for FSP-M `platform_fsp_silicon_init_params_cb` for FSP-S
What is the significance of performing the GPIO initialization just before the jump to FSP-M/S? Not completely against having the hooks, but currently there is one function call into SoC (which in turn can call mainboard) that allows each to perform any initialization that is required before the FSP calls. It also ensures that proper ordering is maintained between SoC and mainboard w.r.t. initializing UPD params.
https://review.coreboot.org/c/coreboot/+/52866/comment/03fa8d3c_dc488d85 PS1, Line 13: This allows for GPIO initialization before the calls as required That can be achieved by using the call chain for platform_fsp_memory_init_params_cb and platform_fsp_silicon_init_params_cb
https://review.coreboot.org/c/coreboot/+/52866/comment/7c069d98_873671aa PS1, Line 14: easy analysis and updates of register changes within the FSP. If this is the intent, I think a better place to put the callbacks would be fsp_debug_{before|after}_{memory|silicon}_init(). It will allow you to dump or check any registers or configurations before and after FSP calls. Example: Recently the debug path was updated to snapshot and verify any GPIO changes across FSP calls: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src.... Similarly, other hooks can be added as required.
File src/drivers/intel/fsp2_0/include/fsp/api.h:
https://review.coreboot.org/c/coreboot/+/52866/comment/03b8c29e_7f78ac43 PS1, Line 75: print values This is better suited for the debug path in my opinion.
https://review.coreboot.org/c/coreboot/+/52866/comment/2175e625_b9b139d2 PS1, Line 75: save and restore registers This seems like a problem with the FSP implementation itself. If there are certain things FSP must not touch, then we should be pushing the SoC vendor to fix that rather than adding permanent workarounds in coreboot. I understand that we have to deal with such situations and we have intentionally kept this outside the FSP driver to ensure that those actually get addressed rather than accepting it as the right solution. Example: CB:52248
https://review.coreboot.org/c/coreboot/+/52866/comment/e0ce83da_9e29667a PS1, Line 75: other initialization Other initialization can already be handled via the callback path we already have.