Attention is currently required from: Intel coreboot Reviewers, Matt DeVillier.
Jérémy Compostella has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/coreboot/+/86279?usp=email )
Change subject: drivers/intel/gma: Drop unused MAILBOXES_DESKTOP
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/86279?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: Ia06fe75702887aa6953bf17bd4bc14af4038bec5
Gerrit-Change-Number: 86279
Gerrit-PatchSet: 1
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Comment-Date: Tue, 04 Feb 2025 19:57:52 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Julius Werner, Karthik Ramasubramanian.
Subrata Banik has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86225?usp=email )
Change subject: vc/google/chromeos: Add low battery indicator screen
......................................................................
Patch Set 2:
(1 comment)
File src/vendorcode/google/chromeos/Kconfig:
https://review.coreboot.org/c/coreboot/+/86225/comment/4952cb4d_ede95390?us… :
PS2, Line 124: and defer the firmware update.
> This refers to deferring the CSE FW Sync that happens in romstage on certain platforms. This also refers to deferring the memory training when FSP-M is updated as part of firmware update.
thanks for the explanation
--
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: 2
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-CC: 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: Tue, 04 Feb 2025 19:52:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
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: Dinesh Gehlot, Intel coreboot Reviewers, Jayvik Desai, Julius Werner, Jérémy Compostella, Kapil Porwal, Karthik Ramasubramanian, Nick Vaccaro.
Hello Dinesh Gehlot, Intel coreboot Reviewers, Jayvik Desai, Kapil Porwal, Karthik Ramasubramanian, Nick Vaccaro, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/86224?usp=email
to look at the new patch set (#3).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: soc/intel/alderlake: Display low battery message on screen
......................................................................
soc/intel/alderlake: Display low battery message on screen
This commit adds a function ux_inform_user_of_poweroff_operation
to display a message on the screen when the system is powering off due
to critically low battery. The message is centered on the screen and
displays a localized string "Battery critically low. Shutting down.".
If no localized string is found, a default English message is displayed.
This implementation relies on CHROMEOS_ENABLE_ESOL Kconfig which is used
to render text message for early sign-off life.
BUG=b:339673254
TEST=Able to capture the eventlog for low battery boot event.
Change-Id: I3b24d2c89ade8cc62b7e47c487d52d47b7f3376d
Signed-off-by: Subrata Banik <subratabanik(a)google.com>
---
M src/soc/intel/alderlake/romstage/ux.c
M src/soc/intel/alderlake/romstage/ux.h
2 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/86224/3
--
To view, visit https://review.coreboot.org/c/coreboot/+/86224?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: I3b24d2c89ade8cc62b7e47c487d52d47b7f3376d
Gerrit-Change-Number: 86224
Gerrit-PatchSet: 3
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: 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-CC: Jérémy Compostella <jeremy.compostella(a)intel.com>
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: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Attention is currently required from: Dinesh Gehlot, Intel coreboot Reviewers, Jayvik Desai, Julius Werner, Jérémy Compostella, Kapil Porwal, Karthik Ramasubramanian, Nick Vaccaro.
Subrata Banik has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86224?usp=email )
Change subject: soc/intel/adl: Display low battery message on screen
......................................................................
Patch Set 2:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/86224/comment/ee3f7725_353df5fa?us… :
PS1, Line 7: soc/intel/adl: Display low battery message on screen
> Looks like you have room to spell alderlake.
Acknowledged
https://review.coreboot.org/c/coreboot/+/86224/comment/5134868a_7545f04c?us… :
PS1, Line 9: This commit adds a function ux_inform_user_of_poweroff_operation
> > This is 75. Cf. https://doc.coreboot.org/contributing/gerrit_guidelines. […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/86224/comment/155f69ca_4ce587a1?us… :
PS1, Line 11:
> Nit: Seems there is an extra space.
Acknowledged
https://review.coreboot.org/c/coreboot/+/86224/comment/47ded586_08675613?us… :
PS1, Line 16: use
> Nit: used
Acknowledged
https://review.coreboot.org/c/coreboot/+/86224/comment/11b8e740_76ef144f?us… :
PS1, Line 16: rely
> Nit: relies
Acknowledged
File src/soc/intel/alderlake/romstage/ux.c:
https://review.coreboot.org/c/coreboot/+/86224/comment/0465f578_84c159f3?us… :
PS1, Line 42: if (!CONFIG(CHROMEOS_ENABLE_ESOL) ||
> should fit on one line.
Acknowledged
https://review.coreboot.org/c/coreboot/+/86224/comment/e930521f_1e572826?us… :
PS1, Line 50: UX_MEMORY_TRAINING_DESC
> Should it be a different locale name here instead of relying on memory training?
Acknowledged
https://review.coreboot.org/c/coreboot/+/86224/comment/66d9910f_d7c44a72?us… :
PS1, Line 53: text = "Battery critically low. Shutting down";
> nit: a dot at the end?
Acknowledged
https://review.coreboot.org/c/coreboot/+/86224/comment/683c4478_aa70bdd1?us… :
PS1, Line 59: }
> Looks like both `ux_inform_user_of_update_operation` and `user_inform_user_of_poweroff_operation` ha […]
Acknowledged
--
To view, visit https://review.coreboot.org/c/coreboot/+/86224?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: I3b24d2c89ade8cc62b7e47c487d52d47b7f3376d
Gerrit-Change-Number: 86224
Gerrit-PatchSet: 2
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: 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-CC: Jérémy Compostella <jeremy.compostella(a)intel.com>
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: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Tue, 04 Feb 2025 19:50:50 +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>
Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>
Attention is currently required from: Dinesh Gehlot, Intel coreboot Reviewers, Jayvik Desai, Julius Werner, Jérémy Compostella, Kapil Porwal, Karthik Ramasubramanian, Nick Vaccaro.
Subrata Banik has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86224?usp=email )
Change subject: soc/intel/adl: Display low battery message on screen
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> > Okay, fair enough... if Alderlake will remain the only platform that uses the libgfxinit version I guess we can keep the code in there.
> >
> > There still seems to be some duplication here regarding the fallback text when the locale-specific text wasn't found. Maybe we should move that hardcoded fallback into `ux_locales_get_text()` to avoid that.
>
>
> thanks for the explanation @kramasub@google.com
> Maybe we should move that hardcoded fallback into `ux_locales_get_text()` to avoid that.
The fallback language_id has already supported https://github.com/coreboot/coreboot/blob/main/src/lib/ux_locales.c#L165
As the hardcode text message is not generic enough for different usecase, i assume we won't be able to move the fallback option text inside `ux_locales_get_text`.
For example: the text msg for MRC update would be different with low-battery power-off msg.
--
To view, visit https://review.coreboot.org/c/coreboot/+/86224?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: I3b24d2c89ade8cc62b7e47c487d52d47b7f3376d
Gerrit-Change-Number: 86224
Gerrit-PatchSet: 2
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: 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-CC: Jérémy Compostella <jeremy.compostella(a)intel.com>
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: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Tue, 04 Feb 2025 19:42:07 +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: Julius Werner, Karthik Ramasubramanian, Subrata Banik.
Dinesh Gehlot has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86223?usp=email )
Change subject: util/cbfstool/eventlog: Add low battery event type
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/86223?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: I5cc5e5f540657c7dfd174a4928e697a272da813a
Gerrit-Change-Number: 86223
Gerrit-PatchSet: 3
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
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: Tue, 04 Feb 2025 19:41:45 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes