coreboot-gerrit February 2025

coreboot-gerrit@coreboot.org

1 participants
2886 discussions

[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: (1 comment) File src/soc/intel/common/block/pmc/pmclib.c: https://review.coreboot.org/c/coreboot/+/86336/comment/4ed0a6ac_0e21d28a?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. 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. -- 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 18:32:33 +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

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/ef02298c_90304077?us… : PS3, Line 635: __weak void platform_do_early_poweroff(void) > The `pmc.c` file is intended to be a generic module, primarily interacting with PMC-related APIs. Including any calls to Chrome EC-specific functions appears inappropriate within this context. The objective is to assign the early power-off sequence management to the Embedded Controller (EC), and it is suggested that the weak function mechanism be utilized directly for this purpose, rather than introducing an indirect call within `pmclib.c`. 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 concern should be if we are implementing an API which is EC specific inside PMClib.c which is not the case. I don't see a point to implement `platform_do_early_poweroff` as override inside EC code which is only meant to implement an EC specific API. If `platform_do_early_poweroff` doesn't fit here to call the EC API "google_chromeec_do_early_poweroff", then probably, we should move this into `reset.c` as it meant to implement reset code. > > Additionally, scalability should be considered. For instance, if a different vendor desires to implement their own early shutdown process, the current design would necessitate further modifications to the `pmclib.c` file. > > > 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. > > In my opinion, the goal is twofold: > > 1. Maintaining the default behavior when no specific early shutdown routine is provided. This approach allows platforms that do not require specialized early shutdown management to operate as intended. Not sure what you meant by intended here ? while below message gives an impression that device doesn't have right implementation to allow early power-off and halting. The display might be still active and one can sense that the system is at hang stage. ``` printk(BIOS_EMERG, "This platform doesn't know how to power off before ramstage," " hanging!\n"); halt(); ``` The proposed msg gives an impression that system will use some magic implemention using PMC which can mitigate the first statement "Early power off is not supported on this platform. Proceeding with the standard PMC shutdown procedure." 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. ``` __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(); } ``` > 2. Including a disclaimer to indicate that the generic PMC solution may not be effective. -- 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:08: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 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: (1 comment) File src/soc/intel/common/block/pmc/pmclib.c: https://review.coreboot.org/c/coreboot/+/86336/comment/88873401_4439f76d?us… : PS3, Line 635: __weak void platform_do_early_poweroff(void) The `pmc.c` file is intended to be a generic module, primarily interacting with PMC-related APIs. Including any calls to Chrome EC-specific functions appears inappropriate within this context. The objective is to assign the early power-off sequence management to the Embedded Controller (EC), and it is suggested that the weak function mechanism be utilized directly for this purpose, rather than introducing an indirect call within `pmclib.c`. Additionally, scalability should be considered. For instance, if a different vendor desires to implement their own early shutdown process, the current design would necessitate further modifications to the `pmclib.c` file. > 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. In my opinion, the goal is twofold: 1. Maintaining the default behavior when no specific early shutdown routine is provided. This approach allows platforms that do not require specialized early shutdown management to operate as intended. 2. Including a disclaimer to indicate that the generic PMC solution may not be effective. -- 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 17:55:11 +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

1 0

[S] Change in coreboot[main]: soc/intel/pantherlake: Display Sign-of-Life during memory training
by Jérémy Compostella (Code Review) 10 Feb '25

10 Feb '25

Attention is currently required from: Alok Agarwal, Intel coreboot Reviewers, Jayvik Desai, Jérémy Compostella, Kapil Porwal, Paul Menzel, Pranava Y N, Vikrant L Jadeja. Hello Alok Agarwal, Anil Kumar K, Intel coreboot Reviewers, Jayvik Desai, Kapil Porwal, Pranava Y N, Subrata Banik, Vikrant L Jadeja, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/85454?usp=email to look at the new patch set (#16). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: soc/intel/pantherlake: Display Sign-of-Life during memory training ...................................................................... soc/intel/pantherlake: Display Sign-of-Life during memory training This commit activates the Firmware Support Package (FSP) Memory Sign-of-Life feature (FSP_UGOP_EARLY_SIGN_OF_LIFE), which allows for the display of a user-configurable text message on-screen during memory initialization. This feature enhances the user experience by providing reassurance that the memory training process is underway and may take some time. The following FSP-M UPDs (Updateable Product Data) are utilized: - VgaInitControl (boolean): Initializes graphics, establishes VGA text mode, and centers the VgaMessage text on the screen. It clears the screen, disables VGA text mode, and deactivates graphics upon exiting the FSP-M (Firmware Support Package - Memory Initialization). - VbtPtr (address): This is a pointer to the VBT (Video BIOS Table) binary. - VbtSize (unsigned integer): Indicates the size of the VBT binary. - LidStatus (boolean): Given the limited resources available at early boot stages, the text message is shown on a single monitor. The lid status determines the most appropriate display to use: - 0: If the lid is closed, display the text message on an external display if one is available; otherwise, display nothing. - 1: If the lid is open, display the message on the internal display; if unavailable, default to an external display. - VgaMessage (string): Specifies the text message to be displayed. When the FSP_UGOP_EARLY_SIGN_OF_LIFE flag is set, coreboot is configured to use the UPDs mentioned above to show a text message during the memory training phase. This text message can be customized through the locale text mechanism using the identifier memory_training_desc. TEST="Enabling FSP-M Sign-of-Life" message is present in the log upon the first boot, and a message is displayed on the screen while the FSP performs MRC training. Signed-off-by: Anil Kumar <anil.kumar.k(a)intel.com> Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com> Change-Id: I993eb0d59cd01fa62f35a77f84e262e389efb367 --- M src/soc/intel/pantherlake/Kconfig M src/soc/intel/pantherlake/romstage/fsp_params.c 2 files changed, 46 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/85454/16 -- To view, visit https://review.coreboot.org/c/coreboot/+/85454?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: I993eb0d59cd01fa62f35a77f84e262e389efb367 Gerrit-Change-Number: 85454 Gerrit-PatchSet: 16 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Alok Agarwal <alok.agarwal(a)intel.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Pranava Y N <pranavayn(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Vikrant L Jadeja <vikrant.l.jadeja(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jayvik Desai <jayvik(a)google.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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Vikrant L Jadeja <vikrant.l.jadeja(a)intel.com> Gerrit-Attention: Pranava Y N <pranavayn(a)google.com> Gerrit-Attention: Alok Agarwal <alok.agarwal(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

1 0

[M] Change in coreboot[main]: mb/starlabs/*: Correct PAD ownership
by Matt DeVillier (Code Review) 10 Feb '25

10 Feb '25

Attention is currently required from: Sean Rhodes. Matt DeVillier has posted comments on this change by Sean Rhodes. ( https://review.coreboot.org/c/coreboot/+/86344?usp=email ) Change subject: mb/starlabs/*: Correct PAD ownership ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/86344?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: I7207d4d00e810c15d071eca0bea83796989e3735 Gerrit-Change-Number: 86344 Gerrit-PatchSet: 2 Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Comment-Date: Mon, 10 Feb 2025 16:38:14 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[S] Change in coreboot[main]: mb/starlabs/starbook/adl_n: Disconnect unused GPIOs
by Matt DeVillier (Code Review) 10 Feb '25

10 Feb '25

Attention is currently required from: Sean Rhodes. Matt DeVillier has posted comments on this change by Sean Rhodes. ( https://review.coreboot.org/c/coreboot/+/86347?usp=email ) Change subject: mb/starlabs/starbook/adl_n: Disconnect unused GPIOs ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/86347?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: I5dfeb5c1503ca85baf3641f1f5803519ec517b81 Gerrit-Change-Number: 86347 Gerrit-PatchSet: 2 Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Comment-Date: Mon, 10 Feb 2025 16:37:37 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[S] Change in coreboot[main]: mb/starlabs/starbook/adl_n: Disconnect unused GPIOs
by Sean Rhodes (Code Review) 10 Feb '25

10 Feb '25

Sean Rhodes has posted comments on this change by Sean Rhodes. ( https://review.coreboot.org/c/coreboot/+/86347?usp=email ) Change subject: mb/starlabs/starbook/adl_n: Disconnect unused GPIOs ...................................................................... Set Ready For Review -- To view, visit https://review.coreboot.org/c/coreboot/+/86347?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: I5dfeb5c1503ca85baf3641f1f5803519ec517b81 Gerrit-Change-Number: 86347 Gerrit-PatchSet: 2 Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Comment-Date: Mon, 10 Feb 2025 16:36:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit February 2025