coreboot-gerrit February 2025

coreboot-gerrit@coreboot.org

1 participants
2886 discussions

[XS] Change in coreboot[main]: mb/emulation/spike-riscv/uart.c: Update UART address
by Maximilian Brune (Code Review) 10 Feb '25

10 Feb '25

Attention is currently required from: Maximilian Brune, Philipp Hug, ron minnich. Hello Nicholas Chin, Philipp Hug, build bot (Jenkins), ron minnich, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/85852?usp=email to look at the new patch set (#2). Change subject: mb/emulation/spike-riscv/uart.c: Update UART address ...................................................................... mb/emulation/spike-riscv/uart.c: Update UART address Spike Simulator commit 191634d2854d implemented a ns16550 serial device which puts the base address at 0x10000000. Tested: Start Spike Simulator and see that coreboot prints onto the UART. Signed-off-by: Maximilian Brune <maximilian.brune(a)9elements.com> Change-Id: I0e3db9d8b141c733bf609f906018096e3594ce83 --- M src/mainboard/emulation/spike-riscv/uart.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/85852/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/85852?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I0e3db9d8b141c733bf609f906018096e3594ce83 Gerrit-Change-Number: 85852 Gerrit-PatchSet: 2 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: ron minnich <rminnich(a)gmail.com> Gerrit-Attention: Philipp Hug <philipp(a)hug.cx> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: ron minnich <rminnich(a)gmail.com>

1 0

[M] Change in coreboot[main]: mb/amd/birman_plus: Update devicetree
by Maximilian Brune (Code Review) 10 Feb '25

10 Feb '25

Attention is currently required from: Ana Carolina Cabral, Paul Menzel. Maximilian Brune has posted comments on this change by Maximilian Brune. ( https://review.coreboot.org/c/coreboot/+/84500?usp=email ) Change subject: mb/amd/birman_plus: Update devicetree ...................................................................... Patch Set 4: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/84500/comment/c47e9352_20494e00?us… : PS3, Line 8: > Some details would be nice. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/84500?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I1cc2e4c8f722048b24d84cf782855ae7a8d64c42 Gerrit-Change-Number: 84500 Gerrit-PatchSet: 4 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Ana Carolina Cabral <ana.cpmelo95(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Ana Carolina Cabral <ana.cpmelo95(a)gmail.com> Gerrit-Comment-Date: Mon, 10 Feb 2025 21:09:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>

1 0

[M] Change in coreboot[main]: mb/amd/birman_plus: Update devicetree
by Maximilian Brune (Code Review) 10 Feb '25

10 Feb '25

Attention is currently required from: Ana Carolina Cabral, Maximilian Brune. Hello Ana Carolina Cabral, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/84500?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Code-Review+1 by Ana Carolina Cabral, Verified+1 by build bot (Jenkins) Change subject: mb/amd/birman_plus: Update devicetree ...................................................................... mb/amd/birman_plus: Update devicetree The devicetree was still a copy of a previous mainboard. This patch updates the devicetree for the birman_plus mainboard. Birman plus is an AMD reference board. sources: - document #58168 Rev 1.01 "Birman+ User Guide" - birman+ schematic Change-Id: I1cc2e4c8f722048b24d84cf782855ae7a8d64c42 Signed-off-by: Maximilian Brune <maximilian.brune(a)9elements.com> --- M src/mainboard/amd/birman_plus/Kconfig M src/mainboard/amd/birman_plus/devicetree_glinda.cb 2 files changed, 40 insertions(+), 12 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/84500/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/84500?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I1cc2e4c8f722048b24d84cf782855ae7a8d64c42 Gerrit-Change-Number: 84500 Gerrit-PatchSet: 4 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Ana Carolina Cabral <ana.cpmelo95(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Ana Carolina Cabral <ana.cpmelo95(a)gmail.com>

1 0

[M] Change in coreboot[main]: soc/amd/glinda/pcie_gpp.c: Add PCI routing table
by Maximilian Brune (Code Review) 10 Feb '25

10 Feb '25

Maximilian Brune has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/85019?usp=email ) Change subject: soc/amd/glinda/pcie_gpp.c: Add PCI routing table ...................................................................... Abandoned not needed -- To view, visit https://review.coreboot.org/c/coreboot/+/85019?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: abandon Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I44c11205ecc1554451a5e9d8f05102fd0d4d25ad Gerrit-Change-Number: 85019 Gerrit-PatchSet: 1 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>

1 0

[S] Change in coreboot[main]: soc/intel/cmn/pmc: Add support for early power off
by Jérémy Compostella (Code Review) 10 Feb '25

10 Feb '25

Attention is currently required from: Angel Pons, 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/2ca6c15e_a1d5b117?us… : PS3, Line 635: __weak void platform_do_early_poweroff(void) > > > Can you tell me the name of the Intel SoC that supports early shutdown using PMC? Based on my kn […] I appreciate your suggestion @th3fanbus@gmail.com; it addresses my concern regarding the direct invocation of a Chrome EC-specific function from `pmclib.c`. -- To view, visit https://review.coreboot.org/c/coreboot/+/86336?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I39f516640b3f75ab4c6a09826922289c0533f79b Gerrit-Change-Number: 86336 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Mon, 10 Feb 2025 19:55:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com>

1 0

[S] Change in coreboot[main]: soc/amd/common/block/graphics: Support non VGA IGDs
by Felix Held (Code Review) 10 Feb '25

10 Feb '25

Attention is currently required from: Ana Carolina Cabral, Fred Reitberger, Jason Glenesk, Matt DeVillier, Patrick Rudolph. Felix Held has posted comments on this change by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/86300?usp=email ) Change subject: soc/amd/common/block/graphics: Support non VGA IGDs ...................................................................... Patch Set 2: (2 comments) Patchset: PS2: > Will update the commit message that more work is to be done to get the amdgpu driver working. sounds good to me File src/soc/amd/common/block/graphics/Kconfig: https://review.coreboot.org/c/coreboot/+/86300/comment/be530bfc_ff7e9cd3?us… : PS2, Line 39: SOC_AMD_COMMON_BLOCK_GRAPHICS_VGA i'd invert this option and have boards having a non-VGA-compatible iGPU select that option. maybe SOC_AMD_COMMON_BLOCK_GRAPHICS_NO_VGA, SOC_AMD_COMMON_BLOCK_GRAPHICS_NO_VGA_RANGES or SOC_AMD_COMMON_BLOCK_GRAPHICS_NON_VGA ? the VGA-compatible case is the default case in coreboot, so we should select the kconfig option in the case where the VGA-compatibility is missing -- To view, visit https://review.coreboot.org/c/coreboot/+/86300?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I6ab28aab74f3169d45d7d852a37ddfcfc75b7c88 Gerrit-Change-Number: 86300 Gerrit-PatchSet: 2 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Ana Carolina Cabral <ana.cpmelo95(a)gmail.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Ana Carolina Cabral <ana.cpmelo95(a)gmail.com> Gerrit-Comment-Date: Mon, 10 Feb 2025 19:31:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com> Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>

1 0

[S] Change in coreboot[main]: soc/intel/cmn/pmc: Add support for early power off
by Subrata Banik (Code Review) 10 Feb '25

10 Feb '25

Attention is currently required from: Angel Pons, 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: (2 comments) Patchset: PS3: > I disagree with #1 as detailed in the discussion found at the following link: [comment 21c234e0_b1965b7b](https://review.coreboot.org/c/coreboot/+/86336/comment/2…. > > Regarding point #2, it appears to be a more reasonable approach than your previous proposal. Given that the limitation is specific to the System on Chip (SoC), it is sensible to manage it within the SoC-specific codebase. Sure, there is no valid reason for calling PMC register as is from early power off as we know it doesn't work and would cause same effect/or worse like what halt() does. I like the suggestion coming from Angel, which would even allow me to keep early power-off implementation inside ChromeOS vendor code. File src/soc/intel/common/block/pmc/pmclib.c: https://review.coreboot.org/c/coreboot/+/86336/comment/b48e9593_54fd7608?us… : PS3, Line 635: __weak void platform_do_early_poweroff(void) > > 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. > > I cannot provide you with a 100% guaranteed list of Intel SoCs where early shutdown is confirmed to be effective, as this scenario has not been formally tested, and I lack the resources to conduct such tests personally. However, based on my experience in the automotive industry working with SoCs like Apollo Lake, I believe that early shutdown was operational at early boot. Based on my testing on all devices starting with SKL till latest PTL, no Intel chipset supports early power-off before MRC init is done using vanilla PMC register programming (it needs special handling). Therefore, there is no additional point of agreeing or disagreeing here. I understood your point and will modify the code. -- To view, visit https://review.coreboot.org/c/coreboot/+/86336?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I39f516640b3f75ab4c6a09826922289c0533f79b Gerrit-Change-Number: 86336 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Mon, 10 Feb 2025 19:17:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com>

1 0

[S] Change in coreboot[main]: soc/intel/cmn/pmc: Add support for early power off
by Angel Pons (Code Review) 10 Feb '25

10 Feb '25

Attention is currently required from: Intel coreboot Reviewers, Julius Werner, Karthik Ramasubramanian, Subrata Banik. Angel Pons 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/58cfaed8_f38cb763?us… : PS3, Line 635: __weak void platform_do_early_poweroff(void) > > > I'm unable to follow what is **inappropriate** here. […] How about guarding `platform_do_early_poweroff()` behind a generic `PLATFORM_HAS_EARLY_POWEROFF` Kconfig option? The idea is that whoever selects this Kconfig has to provide a definition of `platform_do_early_poweroff()`, which could be mainboard code, EC code or whatever. ``` void poweroff(void) { if (!ENV_ROMSTAGE_OR_BEFORE) { pmc_control_poweroff(); } else if (CONFIG(PLATFORM_HAS_EARLY_POWEROFF)) { platform_do_early_poweroff(); } else { printk(BIOS_EMERG, "This platform doesn't know how to power off before ramstage, hanging!\n"); halt(); } } ``` As for making the chipset behave, I know I was able to power off Haswell during romstage (before raminit), but I had to perform a few extra steps for it to work. I don't remember where I put that code but I can look for it tomorrow (it's part of an interactive command-line I use for testing, I never published it). -- To view, visit https://review.coreboot.org/c/coreboot/+/86336?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I39f516640b3f75ab4c6a09826922289c0533f79b Gerrit-Change-Number: 86336 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Mon, 10 Feb 2025 19:11:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com>

1 0

[S] Change in coreboot[main]: soc/intel/cmn/pmc: Add support for early power off
by Jérémy Compostella (Code Review) 10 Feb '25

10 Feb '25

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: (2 comments) Patchset: PS3: > Please hold review in this CL as I will refactor this CL […] I disagree with #1 as detailed in the discussion found at the following link: [comment 21c234e0_b1965b7b](https://review.coreboot.org/c/coreboot/+/86336/comment/2…. Regarding point #2, it appears to be a more reasonable approach than your previous proposal. Given that the limitation is specific to the System on Chip (SoC), it is sensible to manage it within the SoC-specific codebase. File src/soc/intel/common/block/pmc/pmclib.c: https://review.coreboot.org/c/coreboot/+/86336/comment/9bacfd9a_0bc8a136?us… : PS3, Line 635: __weak void platform_do_early_poweroff(void) > 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. I cannot provide you with a 100% guaranteed list of Intel SoCs where early shutdown is confirmed to be effective, as this scenario has not been formally tested, and I lack the resources to conduct such tests personally. However, based on my experience in the automotive industry working with SoCs like Apollo Lake, I believe that early shutdown was operational at early boot. -- To view, visit https://review.coreboot.org/c/coreboot/+/86336?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I39f516640b3f75ab4c6a09826922289c0533f79b Gerrit-Change-Number: 86336 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Mon, 10 Feb 2025 19:07:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com>

1 0

[S] Change in coreboot[main]: soc/intel/cmn/pmc: Add support for early power off
by Subrata Banik (Code Review) 10 Feb '25

10 Feb '25

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?us… : 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. -- To view, visit https://review.coreboot.org/c/coreboot/+/86336?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I39f516640b3f75ab4c6a09826922289c0533f79b Gerrit-Change-Number: 86336 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Mon, 10 Feb 2025 18:41:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com>

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit February 2025