Attention is currently required from: Intel coreboot Reviewers, Julius Werner, Jérémy Compostella, Karthik Ramasubramanian.
Subrata Banik has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86336?usp=email )
Change subject: soc/intel/cmn/pmc: Add support for early power off ......................................................................
Patch Set 3:
(1 comment)
File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/86336/comment/e64b0f72_6b4a4c24?usp... : PS3, Line 635: __weak void platform_do_early_poweroff(void)
I propose two enhancements as follows:
- The implementation of the subordinate function should be placed within the `src/ec/google/chromeec/ec.c` file, encapsulated by an appropriate Kconfig conditional compilation block.
`ec.c` is a generic code and should only deal with EC related APIs. Doesn't make any sense to me to implement platform specific implementation there and guard with additional Kconfig which might be intel specific.
I don;t see any problem with current schema of things. if you see, please share with me.
- The default function can be defined using the following code snippet:
__weak void platform_do_early_poweroff(void) { printk(BIOS_EMERG, "Early power off is not supported on this platform. " "Proceeding with the standard PMC shutdown procedure.\n"); pmc_control_poweroff(); }
Calling `pmc_control_poweroff` in the early stage doesn't add any value. system might eventually hang in S0 with display being off and system burn all fuels w/o any meaningful purpose.
The `platform_do_early_poweroff` function serves as a generic fallback that issues an emergency print statement and invokes the `pmc_control_poweroff` function to initiate a power-off sequence.