Attention is currently required from: Arthur Heymans, Felix Singer, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Lean Sheng Tan, Werner Zeh, Patrick Rudolph, EricR Lai. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60405 )
Change subject: soc/intel/common/cse: Add `finalize` operation for CSE ......................................................................
Patch Set 24:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/60405/comment/512fa710_cceb1c4f PS19, Line 7: soc/intel/common/block/notify: Implement coreboot notify native driver
Thanks for views.. my worry part is, if you switch off the FSP Notify code, and enables coreboot native code in this regard, then we might end-up loose changes if SOC team adds in the FSP notify API though the changes are optional. My concern is with respect to maintenance.
Valid concern and we can have few strategies to ensure robustness
Refer to the SoC platform FAS security chapter and implement those recommended programming (ideally those are meant for OEM/ODM to implement, all IBVs do refer to the same document).
Intel can own this piece like what is being done YoY for Cache-as-RAM native code in `soc/intel/common/block/cpu/car`, eNEM as replacement for FSP-T for each SoC program. (on ADL it took several months to validate eNEM and enable finally) In the past we didn't pick FSP-T just to avoid extra enabling time for CAR/eNEM, right?
Please share your thoughts on this.
Ping! @Sridhar?
Do you have some concern or we can mark this closed ?
File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/60405/comment/8a39d957_932f4a36 PS24, Line 1195: static void cse_final(struct device *dev) : { : if (CONFIG(SKIP_FSP_NOTIFY_PHASE_READY_TO_BOOT)) { : cse_send_end_of_post(); : : cse_control_global_reset_lock(); : : if (CONFIG(DISABLE_HECI1_AT_PRE_BOOT)) { : cse_set_to_d0i3(); : heci1_disable(); : } : } : : if (CONFIG(SKIP_FSP_NOTIFY_PHASE_END_OF_FIRMWARE)) : heci_set_to_d0i3(); : }
What if we aggregate both operations covered by SKIP_FSP_NOTIFY_PHASE_READY_TO_BOOT & SKIP_FSP_NOTIFY_PHASE_END_OF_FIRMWARE under single macro (like CB_INTEL_SOC_RUN_OPS_BEFORE_PAYLOAD_LAUNCH?)?
Few thoughts: 1. Additional maintenance of adding new Kconfig CB_INTEL_SOC_RUN_OPS_BEFORE_PAYLOAD_LAUNCH and doing forward who enables it?
2. Having separate operation under specific Kconfig would make these operations still in context of FSP i.e. a SoC user feel free to pick, skip FSP notify 2a (ready to boot) but let FSP to perform 2b (end of firmware). combing under bigger umbrella would take away the freedom.
3. From SoC recommendation side, Ready to Boot and End of Post are event which has meaning and silicon prefer to perform some operation under specific hoods, hence, coreboot would allow the same flexibility for SoC vendor enggs to add the required code inside specific `if` clause without mixing it.