Attention is currently required from: Intel coreboot Reviewers, Julius Werner, Karthik Ramasubramanian, Subrata Banik.
Jérémy Compostella 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/4ed0a6ac_0e21d28a?usp... : PS3, Line 635: __weak void platform_do_early_poweroff(void)
I'm unable to follow what is **inappropriate** here. the idea of a common code library is to allow inter-IP communication where PMC already communicating with CSE, SPI, GPIO etc. Calling the EC API is not making things very different.
The differences are substantial. Google's Embedded Controller (EC) API is not comparable to an Intel System on Chip (SoC) API when the code being discussed is sitting in `soc/intel/common/block/pmc/pmclib.c`. You have pinpointed a limitation wherein some Intel SoCs are incapable of executing an early shutdown through the standard Power Management Controller (PMC) mechanism. The `poweroff` function within the PMC must incorporate flexibility to overcome this issue. Given your intent to utilize the Google EC, it is necessary to introduce a mechanism that facilitates invoking the Google EC. In my view, a generic weak callback is the optimal solution and ought to be integrated into the Google-specific codebase. It is unclear why the code that is common across Intel SoCs should directly invoke a function specific to Google EC. It seems more fitting to use the linker to provide a board-specific early shutdown function.
Should we adopt your rationale, it would then appear reasonable to other vendors, such as XYZ, to insert their proprietary early shutdown functions directly into the `pmclib.c` file, using a construct like `if (CONFIG(XXX)) xyz_early_power_off()` in `pmclib.c`. This approach lacks scalability.
There is no value to proceed with PMC based shutdown as it will cause the system to stuck into S0 and might give a false impression to the user that system is power-off.
The behavior depends on the specific System on Chip (SoC). For SoCs that do not require special handling, your new implementation could alter the behavior from shutting down properly to entering an infinite loop.