coreboot-gerrit February 2025

coreboot-gerrit@coreboot.org

1 participants
2886 discussions

[S] Change in coreboot[main]: lib: Centralize logo.bmp inclusion in lib/Makefile.mk
by Subrata Banik (Code Review) 13 Feb '25

13 Feb '25

Attention is currently required from: Andrey Petrov, Intel coreboot Reviewers, Karthik Ramasubramanian, Ronak Kanabar. Subrata Banik has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86367?usp=email ) Change subject: lib: Centralize logo.bmp inclusion in lib/Makefile.mk ...................................................................... Patch Set 3: (1 comment) File src/lib/Makefile.mk: https://review.coreboot.org/c/coreboot/+/86367/comment/3fabc73d_71bc4ab8?us… : PS3, Line 427: CONFIG_FSP2_0_LOGO_FILE_NAME > Oh, I see, *all* the BMP_LOGO Kconfigs are still in the FSP folder even if most of them aren't prefixed "FSP" anymore. Yeah that's not great, if the code is in a platform-independent directory the Kconfigs should be too. > > You don't need to do it now but it would be good if it got done eventually. agree with you. will submit refactor cl later -- To view, visit https://review.coreboot.org/c/coreboot/+/86367?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: I16ed1cf29b839c25b6ea1c2f10faf3d99dd707c9 Gerrit-Change-Number: 86367 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(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: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Fri, 14 Feb 2025 02:56:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>

1 0

[S] Change in coreboot[main]: soc/intel/common: Add low battery shutdown function
by Subrata Banik (Code Review) 13 Feb '25

13 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/+/86361?usp=email ) Change subject: soc/intel/common: Add low battery shutdown function ...................................................................... Patch Set 4: (1 comment) File src/soc/intel/common/reset.c: https://review.coreboot.org/c/coreboot/+/86361/comment/959ac88d_5f933f4d?us… : PS4, Line 27: poweroff(); > Wait, where did the rest of this function go? Who is doing the delay and the elog entry now? (If this function does nothing more than `poweroff()`, it doesn't need to exist, the caller could just call `poweroff()` directly.) I will add delay and elog in this same function alongwith CL that introduces PLATFORM_HAS_LOW_BATTERY_INDICATOR Kconfig, therefore, this function will grow to support platform with PLATFORM_HAS_LOW_BATTERY_INDICATOR Kconfig enabled. This is just a skeleton for now -- To view, visit https://review.coreboot.org/c/coreboot/+/86361?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: I92e9003c70c2608770972f1a302f954ebdf17bc4 Gerrit-Change-Number: 86361 Gerrit-PatchSet: 4 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Fri, 14 Feb 2025 02:54:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>

1 0

[XS] Change in coreboot[main]: src/Kconfig: Add config SYSTEM_TYPE_BOX
by Eric Lai (Code Review) 13 Feb '25

13 Feb '25

Attention is currently required from: Dinesh Gehlot, Ivy Jian, Jayvik Desai, Kapil Porwal. Eric Lai has posted comments on this change by Ivy Jian. ( https://review.coreboot.org/c/coreboot/+/86396?usp=email ) Change subject: src/Kconfig: Add config SYSTEM_TYPE_BOX ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/86396?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: I887413cbc09fb0725b2ffd621fe10991b7dbcf6d Gerrit-Change-Number: 86396 Gerrit-PatchSet: 1 Gerrit-Owner: Ivy Jian <ivy.jian(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jayvik Desai <jayvik(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Ivy Jian <ivy.jian(a)quanta.corp-partner.google.com> Gerrit-Comment-Date: Fri, 14 Feb 2025 02:38:11 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[XS] Change in coreboot[main]: mb/google/brya/Kconfig: Select SYSTEM_TYPE_BOX for Dirks
by Eric Lai (Code Review) 13 Feb '25

13 Feb '25

Attention is currently required from: David Wu, Derek Huang, Dinesh Gehlot, Ivy Jian, Jayvik Desai, Kapil Porwal, Nick Vaccaro. Eric Lai has posted comments on this change by Ivy Jian. ( https://review.coreboot.org/c/coreboot/+/86392?usp=email ) Change subject: mb/google/brya/Kconfig: Select SYSTEM_TYPE_BOX for Dirks ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/86392?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: Iabc9afdfdb07d4d6cb4d3fb4b43bfdc3cf2aa383 Gerrit-Change-Number: 86392 Gerrit-PatchSet: 3 Gerrit-Owner: Ivy Jian <ivy.jian(a)quanta.corp-partner.google.com> Gerrit-Reviewer: David Wu <david_wu(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Derek Huang <derekhuang(a)google.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jayvik Desai <jayvik(a)google.com> Gerrit-Attention: David Wu <david_wu(a)quanta.corp-partner.google.com> Gerrit-Attention: Derek Huang <derekhuang(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Ivy Jian <ivy.jian(a)quanta.corp-partner.google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Comment-Date: Fri, 14 Feb 2025 02:37:57 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[M] Change in coreboot[main]: mb/google/nissa/var/pujjoniru: Update DTT settings for thermal control
by Eric Lai (Code Review) 13 Feb '25

13 Feb '25

Attention is currently required from: Dinesh Gehlot, Jayvik Desai, Kapil Porwal, Nick Vaccaro, hualin wei. Eric Lai has posted comments on this change by hualin wei. ( https://review.coreboot.org/c/coreboot/+/86404?usp=email ) Change subject: mb/google/nissa/var/pujjoniru: Update DTT settings for thermal control ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/86404?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: Ia32911488464af4e5070543e2ec630c339ab1925 Gerrit-Change-Number: 86404 Gerrit-PatchSet: 1 Gerrit-Owner: hualin wei <weihualin(a)huaqin.corp-partner.google.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Qinghong Zeng <zengqinghong(a)huaqin.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Cindy Lu <luyi8(a)huaqin.corp-partner.google.com> Gerrit-CC: Dolan Liu <liuyong5(a)huaqin.corp-partner.google.com> Gerrit-Attention: Jayvik Desai <jayvik(a)google.com> Gerrit-Attention: hualin wei <weihualin(a)huaqin.corp-partner.google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Comment-Date: Fri, 14 Feb 2025 02:37:21 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[XS] Change in coreboot[main]: mb/google/brox: Use early power off support
by Karthik Ramasubramanian (Code Review) 13 Feb '25

13 Feb '25

Attention is currently required from: Dinesh Gehlot, Intel coreboot Reviewers, Jayvik Desai, Jérémy Compostella, Kapil Porwal, Nick Vaccaro, Subrata Banik. Karthik Ramasubramanian has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86356?usp=email ) Change subject: mb/google/brox: Use early power off support ...................................................................... Patch Set 5: (1 comment) Patchset: PS4: > Does that not select HAVE_EARLY_POWEROFF_SUPPORT for all platforms that use ChromeEC including Intel […] Never mind, I get your idea to HAVE_EARLY_POWEROFF_SUPPORT for all boards that use ChromeEC. Since poweroff() is called from romstage only on platforms with Early Sign of Life, it should be fine. -- To view, visit https://review.coreboot.org/c/coreboot/+/86356?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: Ib748cb4d540c631afd0f6bd5bff23ec17601f3ed Gerrit-Change-Number: 86356 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Jayvik Desai <jayvik(a)google.com> Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Comment-Date: Fri, 14 Feb 2025 02:37:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>

1 0

[M] Change in coreboot[main]: sb/intel/common: Add GPIO serial blink console support
by Keith Hui (Code Review) 13 Feb '25

13 Feb '25

Attention is currently required from: Intel coreboot Reviewers, Jérémy Compostella. Keith Hui has posted comments on this change by Keith Hui. ( https://review.coreboot.org/c/coreboot/+/86390?usp=email ) Change subject: sb/intel/common: Add GPIO serial blink console support ...................................................................... Patch Set 1: (1 comment) File src/southbridge/intel/common/gpio.c: https://review.coreboot.org/c/coreboot/+/86390/comment/f91b111f_60598c8f?us… : PS1, Line 195: CONSOLE_INTEL_GPSB) > Couldn't in its own C file instead ? Because static get_gpio_base(). To put this code in its own file I'll need to make it not static and set up headers accordingly. If that's ok I'll do it. Also, once factored out should it live in console/ or sb/intel/common? -- To view, visit https://review.coreboot.org/c/coreboot/+/86390?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: I0741142fe21eba4989d28b96e795d3bfa3085466 Gerrit-Change-Number: 86390 Gerrit-PatchSet: 1 Gerrit-Owner: Keith Hui <buurin(a)gmail.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.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-Comment-Date: Fri, 14 Feb 2025 02:27:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com>

1 0

[XS] Change in coreboot[main]: mb/google/brox: Use early power off support
by Karthik Ramasubramanian (Code Review) 13 Feb '25

13 Feb '25

Attention is currently required from: Dinesh Gehlot, Intel coreboot Reviewers, Jayvik Desai, Jérémy Compostella, Kapil Porwal, Nick Vaccaro, Subrata Banik. Karthik Ramasubramanian has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86356?usp=email ) Change subject: mb/google/brox: Use early power off support ...................................................................... Patch Set 5: (1 comment) Patchset: PS4: > So what's "Acknowledged" here now, are we moving this `select` into `chromeec/Kconfig` or not? Does that not select HAVE_EARLY_POWEROFF_SUPPORT for all platforms that use ChromeEC including Intel, ARM and AMD platforms? Also not all Intel platforms that use ChromeEC need this support - only more recent ones like Brox, Fatcat and probably Brya & Rex. Other mainboards like Dedede, Volteer and prior ones don't need this since Early Sign of Life is not supported in those boards. -- To view, visit https://review.coreboot.org/c/coreboot/+/86356?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: Ib748cb4d540c631afd0f6bd5bff23ec17601f3ed Gerrit-Change-Number: 86356 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Jayvik Desai <jayvik(a)google.com> Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Comment-Date: Fri, 14 Feb 2025 02:19:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>

1 0

[S] Change in coreboot[main]: drivers/intel/fsp2_0: Add platform callback for critical shutdown
by Julius Werner (Code Review) 13 Feb '25

13 Feb '25

Attention is currently required from: Andrey Petrov, Intel coreboot Reviewers, Karthik Ramasubramanian, Ronak Kanabar, Subrata Banik. Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86226?usp=email ) Change subject: drivers/intel/fsp2_0: Add platform callback for critical shutdown ...................................................................... Patch Set 12: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/86226?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: I2d6677d70dea3d24f5a19d70608fd21229a271a0 Gerrit-Change-Number: 86226 Gerrit-PatchSet: 12 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(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: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Fri, 14 Feb 2025 01:12:45 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[S] Change in coreboot[main]: drivers/intel/fsp2_0: Add low battery indicator screen
by Julius Werner (Code Review) 13 Feb '25

13 Feb '25

Attention is currently required from: Andrey Petrov, Intel coreboot Reviewers, Jérémy Compostella, Karthik Ramasubramanian, Ronak Kanabar, Subrata Banik. Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86225?usp=email ) Change subject: drivers/intel/fsp2_0: Add low battery indicator screen ...................................................................... Patch Set 13: (2 comments) File src/drivers/intel/fsp2_0/Kconfig: https://review.coreboot.org/c/coreboot/+/86225/comment/15f8590a_16182560?us… : PS13, Line 525: config PLATFORM_HAS_LOW_BATTERY_INDICATOR > Does it still make sense that these options are in the FSP directory? Maybe they should also be in ` […] Kconfigs should all be cleaned up and moved out of FSP directory in later patch, as discussed in CB:86367. https://review.coreboot.org/c/coreboot/+/86225/comment/9d407bc7_c5e4fe24?us… : PS13, Line 535: config HAVE_ESOL_SUPPORT_FOR_LOW_BATTERY_INDICATOR > Allow rendering low-battery shutdown msg is a platform choice, so we can;t force a platform to show the eSOL msg or UI msg depending upon mainboard supports uGOP/libgfxinit Well, that is a choice by the person compiling coreboot. Mainboards should only `select` things that are actually required by the hardware, not policy choices by whoever ships that board. Those choices should be made in menuconfig (in the ChromeOS case, that means in our ebuilds). In this case, I think that is already done by the `PLATFORM_HAS_EARLY_LOW_BATTERY_INDICATOR` option itself (because that is menuconfig-configurable). > low battery rendering would use a portion of eSOL technology. take a case for PTL, where eSOL feature is ready but not low-battery handing inside uGOP therefore, we can't enforce to enable this feature if platform supports either uGOP or libgfxinit Okay, that's a fair point. But that is determined by the SoC, not the mainboard. So if you want this to guard against someone selecting the option on platforms where it doesn't do anything, then you'd have to put the `select HAVE_ESOL_SUPPORT_FOR_LOW_BATTERY_INDICATOR` into `src/soc/intel/alderlake/Kconfig`. So I guess this patch is good to merge, but CB:86316 should be changed to target the SoC instead. > To maintain platform independence from ChromeOS, this feature allows overrides for configurable parameters like APIs for logo display, text messages, poweroff control, and low-battery indicators. I mean, I don't see anything there that's ChromeOS-specific other than the splash screen image itself, and that is already handled by the image file Kconfig. All the other things here are generic features that any OS could choose to use. So I don't think `CHROMEOS_ENABLE_ESOL` should be a ChromeOS option, it should probably move to a more generic place (but doesn't need to be done in this patch). -- To view, visit https://review.coreboot.org/c/coreboot/+/86225?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: I711c53455639b449fe85903139bbc06cdab08d09 Gerrit-Change-Number: 86225 Gerrit-PatchSet: 13 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(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: Ronak Kanabar <ronak.kanabar(a)intel.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: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Fri, 14 Feb 2025 01:11:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit February 2025