Attention is currently required from: Karthik Ramasubramanian, Subrata Banik.
Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86228?usp=email )
Change subject: vc/google/chromeos: Implement platform callback for critical shutdown
......................................................................
Patch Set 7:
(1 comment)
File src/vendorcode/google/chromeos/battery.c:
https://review.coreboot.org/c/coreboot/+/86228/comment/3ac502ff_4ac0695c?us… :
PS7, Line 59: google_chromeec_reboot(EC_REBOOT_COLD_AP_OFF, 0);
> Well then, like I said, I think the better solution would be to make sure the `poweroff()` function […]
(edit: Actually that override would probably fit better in `src/ec/google/chromeec` than in vendorcode.)
--
To view, visit https://review.coreboot.org/c/coreboot/+/86228?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: I119f80a45c045a6095cae98f179c755a2e948e9c
Gerrit-Change-Number: 86228
Gerrit-PatchSet: 7
Gerrit-Owner: Subrata Banik <subratabanik(a)google.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-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Sat, 08 Feb 2025 02:20:08 +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: Karthik Ramasubramanian, Subrata Banik.
Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86228?usp=email )
Change subject: vc/google/chromeos: Implement platform callback for critical shutdown
......................................................................
Patch Set 7:
(1 comment)
File src/vendorcode/google/chromeos/battery.c:
https://review.coreboot.org/c/coreboot/+/86228/comment/d9e36e45_05dc603b?us… :
PS7, Line 59: google_chromeec_reboot(EC_REBOOT_COLD_AP_OFF, 0);
> > I'm still not really clear why this needs to be an EC command rather than a normal `poweroff()`? I […]
Well then, like I said, I think the better solution would be to make sure the `poweroff()` function implemented by the platform does the right thing, rather than forcing platform-independent code to deal with this quirk.
For example, you could add something like this to the top of the `poweroff()` implementation in `intel/common/block/pmc/pmclib.c`:
```
if (ENV_ROMSTAGE_OR_BEFORE) {
platform_do_intel_early_poweroff();
} else {
pmc_enable_...
if (!ENV_SMM)
halt();
}
```
and then elsewhere in that file
```
__weak void platform_do_intel_early_poweroff(void) {
printk(BIOS_EMERG, "This platform doesn't know how to power off before ramstage, hanging!\n");
halt();
}
```
and then somewhere in `vendorcode/google/chromeos` you can override that function (in a file that gets compiled for Intel platforms when CONFIG_EC_GOOGLE_CHROMEEC is set)
```
void platform_do_intel_early_poweroff(void) {
google_chromeec_reboot(EC_REBOOT_COLD_AP_OFF, 0);
halt();
}
```
And then other parts of coreboot can just call `poweroff()` whenever they need to power off the system without having to know about these platform-specific quirks.
--
To view, visit https://review.coreboot.org/c/coreboot/+/86228?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: I119f80a45c045a6095cae98f179c755a2e948e9c
Gerrit-Change-Number: 86228
Gerrit-PatchSet: 7
Gerrit-Owner: Subrata Banik <subratabanik(a)google.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-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Sat, 08 Feb 2025 02:19:02 +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: Julius Werner, Karthik Ramasubramanian.
Subrata Banik has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86228?usp=email )
Change subject: vc/google/chromeos: Implement platform callback for critical shutdown
......................................................................
Patch Set 7:
(1 comment)
File src/vendorcode/google/chromeos/battery.c:
https://review.coreboot.org/c/coreboot/+/86228/comment/94644268_23bdda24?us… :
PS7, Line 59: google_chromeec_reboot(EC_REBOOT_COLD_AP_OFF, 0);
> I'm still not really clear why this needs to be an EC command rather than a normal `poweroff()`? If this was about there being some sort of bug with `poweroff()` ― I think I read that somewhere(?) ― then perhaps the answer is to change the `poweroff()` implementation for the ChromeOS platforms to use an EC command instead, rather than making generic code avoid using the API entirely. If coreboot has a global API like `poweroff()` that's implemented by the platforms, the assumption is that the platform should implement it in a way that works correctly, and the generic code should not need to be aware of any platform-specific quirks with it.
>
> This plays into the question whether this function needs to be platform-specific at all. If you need to use an EC command then I guess it needs to, but if you could just use `poweroff()` you could make it generic.
Please refer to b/383044280.
In summary, there is a chipset limitation that `poweroff()` using PMC register doesn't work at an early stage on Intel chipset so, we need to use EC driven method.
In future Intel might add that chipset capability
--
To view, visit https://review.coreboot.org/c/coreboot/+/86228?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: I119f80a45c045a6095cae98f179c755a2e948e9c
Gerrit-Change-Number: 86228
Gerrit-PatchSet: 7
Gerrit-Owner: Subrata Banik <subratabanik(a)google.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-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Sat, 08 Feb 2025 01:59:15 +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, Eran Mitrani, Intel coreboot Reviewers, Jakub "Kuba" Czapiga, Jayvik Desai, Julius Werner, Jérémy Compostella, Kapil Porwal, Nick Vaccaro, Tarun.
Subrata Banik has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86283?usp=email )
Change subject: lib: Refactor ux_locales_get_text API
......................................................................
Patch Set 8:
(1 comment)
File src/include/ux_locales.h:
https://review.coreboot.org/c/coreboot/+/86283/comment/98cab73c_6199d60b?us… :
PS5, Line 23: const char *ux_locales_get_text(const char *name, const char *fallback_text);
> Well, then change the test, of course. Tests do not have a purpose by themselves, they only exist to validate the underlying coreboot code. If the code changes, you can remove or rewrite the tests that checked cases which cannot exist anymore. If we now only have an API that takes an enum ID, the tests only need to check the behavior when passing those IDs (including edge cases like passing an illegal ID, or passing a valid ID but the resulting CBFS filename not existing).
The test tries to check multiple things at once, and we can only change things when we're ready.
1. Passing a valid ID without a supported language ID should return NULL.
2. Passing a valid ID with a different language ID should return the same message w/ language ID on it.
I need more time to understand how to keep similar tests after using enum IDs. Should we handle them separately in a CL and make the necessary adjustments? This CL just ensures that unit tests don't fail due to recent refactoring of ux-locales.
I'd prefer to keep all required unit tests and add more based on enum IDs to test `ux_locales_get_text_by_id`, because `ux_locales_get_text` is an important helper function for `ux_locales_get_text_by_id`.
WDYT ?
--
To view, visit https://review.coreboot.org/c/coreboot/+/86283?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: I4952802396265b9ee8d164d6e43a7f2b3599d6c0
Gerrit-Change-Number: 86283
Gerrit-PatchSet: 8
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com>
Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com>
Gerrit-Reviewer: Jakub "Kuba" Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Tarun <tstuli(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.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: Eran Mitrani <mitrani(a)google.com>
Gerrit-Attention: Jakub "Kuba" Czapiga <czapiga(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-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Tarun <tstuli(a)gmail.com>
Gerrit-Comment-Date: Sat, 08 Feb 2025 01:56:00 +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: Karthik Ramasubramanian, Subrata Banik.
Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86228?usp=email )
Change subject: vc/google/chromeos: Implement platform callback for critical shutdown
......................................................................
Patch Set 7:
(1 comment)
File src/vendorcode/google/chromeos/battery.c:
https://review.coreboot.org/c/coreboot/+/86228/comment/6672f47e_f23467bb?us… :
PS7, Line 59: google_chromeec_reboot(EC_REBOOT_COLD_AP_OFF, 0);
I'm still not really clear why this needs to be an EC command rather than a normal `poweroff()`? If this was about there being some sort of bug with `poweroff()` ― I think I read that somewhere(?) ― then perhaps the answer is to change the `poweroff()` implementation for the ChromeOS platforms to use an EC command instead, rather than making generic code avoid using the API entirely. If coreboot has a global API like `poweroff()` that's implemented by the platforms, the assumption is that the platform should implement it in a way that works correctly, and the generic code should not need to be aware of any platform-specific quirks with it.
This plays into the question whether this function needs to be platform-specific at all. If you need to use an EC command then I guess it needs to, but if you could just use `poweroff()` you could make it generic.
--
To view, visit https://review.coreboot.org/c/coreboot/+/86228?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: I119f80a45c045a6095cae98f179c755a2e948e9c
Gerrit-Change-Number: 86228
Gerrit-PatchSet: 7
Gerrit-Owner: Subrata Banik <subratabanik(a)google.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-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Sat, 08 Feb 2025 01:17:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Andrey Petrov, Intel coreboot Reviewers, Jérémy Compostella, Ronak Kanabar, Subrata Banik.
Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86225?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: drivers/intel/fsp2_0: Add low battery indicator screen
......................................................................
Patch Set 8:
(2 comments)
File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/86225/comment/cf070376_290ea4a7?us… :
PS8, Line 527: select BMP_LOGO
This should probably be a `depends on` instead?
File src/vendorcode/google/chromeos/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/86225/comment/23ac7108_0bd5eaf8?us… :
PS8, Line 45: low_battery.bmp,CONFIG_PLATFORM_LOW_BATTERY_INDICATOR_LOGO_PATH))
This should go either in `fsp2_0/Makefile.mk` or in `src/lib/Makefile.mk` (because `bmp_logo.c` is there... actually, it's quite weird that we don't have a rule adding a `logo.bmp` file when `HAVE_CUSTOM_BMP_LOGO` is false, that should probably go in there too).
--
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: 8
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: 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: Julius Werner <jwerner(a)chromium.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-Comment-Date: Sat, 08 Feb 2025 01:10:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Dinesh Gehlot, Eran Mitrani, Intel coreboot Reviewers, Jakub "Kuba" Czapiga, Jayvik Desai, Jérémy Compostella, Kapil Porwal, Nick Vaccaro, Subrata Banik, Tarun.
Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86283?usp=email )
Change subject: lib: Refactor ux_locales_get_text API
......................................................................
Patch Set 8:
(2 comments)
File src/include/ux_locales.h:
https://review.coreboot.org/c/coreboot/+/86283/comment/a994de87_53607b74?us… :
PS5, Line 23: const char *ux_locales_get_text(const char *name, const char *fallback_text);
> > > Why should we still keep this API around? I think we should switch everything to using IDs. […]
Well, then change the test, of course. Tests do not have a purpose by themselves, they only exist to validate the underlying coreboot code. If the code changes, you can remove or rewrite the tests that checked cases which cannot exist anymore. If we now only have an API that takes an enum ID, the tests only need to check the behavior when passing those IDs (including edge cases like passing an illegal ID, or passing a valid ID but the resulting CBFS filename not existing).
File src/lib/ux_locales.c:
https://review.coreboot.org/c/coreboot/+/86283/comment/0b3cbe04_848034b0?us… :
PS8, Line 27: ux_locale_text_msg
nit: should probably call this `ux_locale_fallback_text` to clarify that it is only used when the actual locale file cannot be found.
--
To view, visit https://review.coreboot.org/c/coreboot/+/86283?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: I4952802396265b9ee8d164d6e43a7f2b3599d6c0
Gerrit-Change-Number: 86283
Gerrit-PatchSet: 8
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com>
Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com>
Gerrit-Reviewer: Jakub "Kuba" Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Tarun <tstuli(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.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: Eran Mitrani <mitrani(a)google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Jakub "Kuba" Czapiga <czapiga(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-Attention: Tarun <tstuli(a)gmail.com>
Gerrit-Comment-Date: Sat, 08 Feb 2025 01:06:08 +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>