[coreboot-gerrit] [M] Change in coreboot[main]: drivers/intel/fsp2_0: Add early low-battery shutdown during memory init

[Subrata Banik (Code Review)]

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

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

Change subject: drivers/intel/fsp2_0: Add early low-battery shutdown during memory init
......................................................................

Patch Set 7:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/86452/comment/91f452ef_28e1e370?usp... :
PS6, Line 351: 		platform_display_early_shutdown_notification(NULL);
> > I am wondering if ux.h can be moved from soc/intel/${soc}/romstage/ into either src/drivers/intel/ux/ux.h or src/vendorcode/intel/ux/ux.h. It can export the following APIs:
> > bool ux_inform_user_of_update_operation(const char *name, void *arg);
> > bool ux_inform_user_of_poweroff_operation(const char *name, void *arg, bool *defer_poweroff);
> >
> > Then ux_libgfxinit.c and ux_ugop.c can be added. ux_libgfxinit.c re-uses the code as is from soc/intel/alderlake/romstage/ux.c. arg is unused in this driver. ux_ugop.c gathers all the common codes for meteorlake, pantherlake and uses arg as fspm_upd. At most one of them gets enabled depending on the SoC.
> >
> > With that this part can be simplified as:
> > if (CONFIG(PLATFORM_HAS_EARLY_LOW_BATTERY_INDICATOR)) {
> >         ux_inform_user_of_poweroff_operation("low-battery-shutdown", fspm_upd, defer_shutdown);
> >         if (!defer_shutdown)
> >                 do_low_battery_poweroff();
> > }
> >
> > That way we don't need platform_display_early_shutdown_notification hook. This will also address Julius's comment here: https://review.coreboot.org/c/coreboot/+/85454/comment/3280809d_e258c525/
> >
> > I am suggesting this since it is getting harder to wrap up the flow with too many if(CONFIG(...)) checks and platform_specific hooks.
>
> I guess the only motivation is to keep this code inside SoC local due to the number of UPDs (using fspm_upd) that it's aiming to use. Some discussion is here https://review.coreboot.org/c/coreboot/+/85454/comment/3280809d_e258c525/ where we are discussing if moving UPD handing into common code is a good idea (knowing there is no standard spec for UPD naming and usage)

marking resolved based on b/398872610.