Attention is currently required from: Dinesh Gehlot, Intel coreboot Reviewers, Jayvik Desai, Jérémy Compostella, Kapil Porwal, Nick Vaccaro, Subrata Banik.
Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86227?usp=email )
Change subject: soc/intel/adl: Handle critical low battery early in romstage ......................................................................
Patch Set 3:
(2 comments)
File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/86227/comment/fd870af4_7068bc5a?usp... : PS3, Line 425: elog_add_event_byte(ELOG_TYPE_LOW_BATTERY_INDICATOR, ELOG_FW_ISSUE_SHUTDOWN); You should log the event before you delay, in case the system browns out in those 3 seconds.
https://review.coreboot.org/c/coreboot/+/86227/comment/470476dd_43d2123d?usp... : PS3, Line 446: early_shutdown_if_battery_below_critical_threshold(); Why is this inserted here? Isn't this the code path that only happens when MRC cache data was not found? Do you only want to shut down on low battery if you need to do memory training? (The commit message doesn't really explain it that way...)
In general, I don't understand why this would be in the FSP code at all. What does this have to do with FSP? I would rather expect this to just be at the start of `mainboard_romstage_entry()` somewhere, or even directly called from `romstage_main()`.