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/+/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/051fd309_9610c21c?us… :
PS3, Line 427: CONFIG_FSP2_0_LOGO_FILE_NAME
> > Actually, why is this an FSP2_0 option? This should probably also move to `src/lib/Kconfig` and be […]
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.
--
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: 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 00:58:22 +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>
Attention is currently required from: Dinesh Gehlot, Intel coreboot Reviewers, Jayvik Desai, Jérémy Compostella, Kapil Porwal, Karthik Ramasubramanian, Nick Vaccaro, Subrata Banik.
Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86278?usp=email )
Change subject: soc/intel/alderlake: Use helper for UX messages
......................................................................
Patch Set 18:
(1 comment)
File src/soc/intel/alderlake/romstage/ux.c:
https://review.coreboot.org/c/coreboot/+/86278/comment/aa09a5d4_16046538?us… :
PS18, Line 31: bool ux_inform_user_of_update_operation(const char *name)
> Do we need this to be an extra function, though? Why not just have the caller pass the ID to `ux_inf […]
Oh, I see I asked this in CB:86224 already. Okay, fine, doesn't really matter much either way.
--
To view, visit https://review.coreboot.org/c/coreboot/+/86278?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: Ib31f7633e7b3f84122419e4ce39e2b5044cb9a96
Gerrit-Change-Number: 86278
Gerrit-PatchSet: 18
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: Julius Werner <jwerner(a)chromium.org>
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-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: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Fri, 14 Feb 2025 00:55:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Attention is currently required from: Dinesh Gehlot, Intel coreboot Reviewers, Jayvik Desai, Jérémy Compostella, Kapil Porwal, Karthik Ramasubramanian, Nick Vaccaro, Subrata Banik.
Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86278?usp=email )
Change subject: soc/intel/alderlake: Use helper for UX messages
......................................................................
Patch Set 18: Code-Review+2
(2 comments)
File src/soc/intel/alderlake/romstage/ux.c:
https://review.coreboot.org/c/coreboot/+/86278/comment/af6b083d_37d98f77?us… :
PS17, Line 11: const char *name
> > Debug statement will be human-readable/comprehensible with name over id. […]
Oh... okay, I see now that this wasn't used for anything else before either, I thought it was a leftover from the string-based message IDs. Okay, fine.
File src/soc/intel/alderlake/romstage/ux.c:
https://review.coreboot.org/c/coreboot/+/86278/comment/b2a5aaa8_559dbf18?us… :
PS18, Line 31: bool ux_inform_user_of_update_operation(const char *name)
Do we need this to be an extra function, though? Why not just have the caller pass the ID to `ux_inform_user_of_operation()` directly?
--
To view, visit https://review.coreboot.org/c/coreboot/+/86278?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: Ib31f7633e7b3f84122419e4ce39e2b5044cb9a96
Gerrit-Change-Number: 86278
Gerrit-PatchSet: 18
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: Julius Werner <jwerner(a)chromium.org>
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-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: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Fri, 14 Feb 2025 00:51:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
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>
Attention is currently required from: Angel Pons, Intel coreboot Reviewers, Jérémy Compostella, Karthik Ramasubramanian, Subrata Banik.
Julius Werner 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/fc56a014_74d91e37?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.)
--
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: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Fri, 14 Feb 2025 00:46:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Boris Mittelberg, Caveh Jalali, Subrata Banik.
Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86379?usp=email )
Change subject: ec/google/chromeec: Add early power off support
......................................................................
Patch Set 1:
(1 comment)
File src/ec/google/chromeec/reset.c:
https://review.coreboot.org/c/coreboot/+/86379/comment/0d0d05e2_cfe75603?us… :
PS1, Line 8: google_chromeec_do_early_poweroff();
> I'm not really sure why this is a separate function now. […]
Actually, not sure this needs to be an extra file either. If you move the Kconfig `select` into `chromeec/Kconfig` as suggested on CB:86356, this will always be enabled anyway, so you might as well just put it in the main ec.c.
--
To view, visit https://review.coreboot.org/c/coreboot/+/86379?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: I0c634d69de36fe8bdb6a61c121e321d3626ac3ff
Gerrit-Change-Number: 86379
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
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-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Fri, 14 Feb 2025 00:42:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Attention is currently required from: Dinesh Gehlot, Intel coreboot Reviewers, Jayvik Desai, Jérémy Compostella, Kapil Porwal, Nick Vaccaro, Subrata Banik.
Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86356?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: mb/google/brox: Use early power off support
......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS4:
> Acknowledged
So what's "Acknowledged" here now, are we moving this `select` into `chromeec/Kconfig` or not?
--
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 00:40:26 +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>
Attention is currently required from: Boris Mittelberg, Caveh Jalali, Subrata Banik.
Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86379?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: ec/google/chromeec: Add early power off support
......................................................................
Patch Set 1:
(1 comment)
File src/ec/google/chromeec/reset.c:
https://review.coreboot.org/c/coreboot/+/86379/comment/44335126_17c184d1?us… :
PS1, Line 8: google_chromeec_do_early_poweroff();
I'm not really sure why this is a separate function now. Just get rid of that again and put the two lines from there into here.
--
To view, visit https://review.coreboot.org/c/coreboot/+/86379?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: I0c634d69de36fe8bdb6a61c121e321d3626ac3ff
Gerrit-Change-Number: 86379
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
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-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Fri, 14 Feb 2025 00:38:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No