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/3c481e3b_8f471700?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.
let me know if you have any concerns with the proposal https://review.coreboot.org/c/coreboot/+/86336/comments/9034fea3_b9e2a170?
if Intel EC has an existing implementation to power-off chipset then can you add that in the SoC specific override under respective Kconfig implementation.
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.
Override is always an option with meaningful implementation.
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,
Can you tell me the name of the Intel SoC that supports early shutdown using PMC? Based on my knowledge, there are currently no Intel SoCs that support common code with early shutdown capabilities. However, we can consider implementing a SoC-specific override if any future SoC enables early shutdown.
your new implementation could alter the behavior from shutting down properly to entering an infinite loop.
-
Subrata Banik (Code Review)