[coreboot-gerrit] [S] Change in coreboot[main]: drivers/intel/fsp2_0: Add platform callback for critical shutdown

[Julius Werner (Code Review)]

Attention is currently required from: Andrey Petrov, Intel coreboot Reviewers, Karthik Ramasubramanian, Ronak Kanabar, Subrata Banik.

Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86226?usp=email )

Change subject: drivers/intel/fsp2_0: Add platform callback for critical shutdown
......................................................................

Patch Set 2:

(1 comment)

File src/drivers/intel/fsp2_0/silicon_init.c:

https://review.coreboot.org/c/coreboot/+/86226/comment/7534644f_eac9ecff?usp... :
PS2, Line 263: 		platform_is_critical_shutdown_needed();
Having a function that sounds like a question hang the boot flow is not very intuitive. Also, it's not good that `google_chromeec_is_below_critical_threshold()` is called twice (once from this and once from `bmp_logo_filename()`) — if the EC should return different answers to the two host commands for some reason, you're gonna end up in a very weird state.

I don't think we need to tie this into `HAVE_CUSTOM_BMP_LOGO`, since the only real purpose for that option was to allow SKU differentiation in the splash screen, and we probably won't need SKU differentiation in our low battery screens. So I would say just hardcode the filename for the low battery case in `bmp_load_logo()` and don't even call `bmp_logo_filename()` for that case.

I would suggest you make the `platform_is_critical_shutdown_needed()` function a function that only returns true or false without hanging the system itself, but caches that result so that it can't suddenly change when you call it a second time. (Maybe also name it something that explicitly says "low battery" because it's otherwise not quite clear that it's related to that.)
```
#if CONFIG(...)
bool platform_is_low_battery_shutdown_needed(void)
{
  static bool result = false;
  static bool checked = false;

if (!checked) {
    if (google_chromeec_is_below_critical_threshold())
      result = true;
    checked = true;
  };

return result;
}
#endif
```
Then, in `bmp_load_logo()`, you do something like
```
const char *logo_name;
if (platform_is_low_battery_shutdown_needed())
  logo_name = "low_battery.bmp";
else
  logo_name = bmp_logo_filename();

*logo_size = cbfs_load(logo_name, ...);
```
And then here, you do the
```
if (platform_is_low_battery_shutdown_needed()) {
  elog_add_event_byte(ELOG_TYPE_LOW_BATTERY_INDICATOR, ELOG_FW_ISSUE_SHUTDOWN);
  delay(3);
  poweroff();
}
```