Attention is currently required from: Angel Pons, 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 4:
(2 comments)
File src/soc/intel/common/block/pmc/Kconfig:
https://review.coreboot.org/c/coreboot/+/86336/comment/4e244b32_387f32ba?us… :
PS4, Line 93:
When searching through the entire Coreboot project using grep, it appears that the convention is to use the `HAVE_` prefix. Should the term `HAVE_EARLY_POWEROFF_SUPPORT` be used to adhere to this convention?
File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/86336/comment/f379f18b_df7632ab?us… :
PS4, Line 640: printk(BIOS_EMERG, "This platform doesn't know how to power off before ramstage,"
I believe that the "before ramstage" part is inaccurate. According to my earlier testing on Panther Lake, it also isn't working before FSP-S in ramstage. The exclamation point seems unnecessary. Could we use a message such as:
"This platform cannot be powered off until the silicon initialization is complete; entering an infinite loop."
Also, how does this code addresses the before FSP-S powering off in ramstage ?
--
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: 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: 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: Tue, 11 Feb 2025 17:02:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Arthur Heymans, David Hendricks, Felix Held, Matt DeVillier.
Alicja Michalska has posted comments on this change by Alicja Michalska. ( https://review.coreboot.org/c/coreboot/+/86131?usp=email )
Change subject: device/Kconfig: Make option to allocate above 4G appear in Kconfig
......................................................................
Patch Set 5:
(1 comment)
File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/86131/comment/c83d9a3b_8861cc31?us… :
PS5, Line 1013: bool "Extend resource window for PCIe devices"
> ```suggestion […]
Good idea, thanks!
--
To view, visit https://review.coreboot.org/c/coreboot/+/86131?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: Ia17b3312016409d8fd6bcce4321481a7b7e35ce5
Gerrit-Change-Number: 86131
Gerrit-PatchSet: 5
Gerrit-Owner: Alicja Michalska <ahplka19(a)gmail.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Tue, 11 Feb 2025 16:55:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com>
Attention is currently required from: Dinesh Gehlot, Intel coreboot Reviewers, Jayvik Desai, Kapil Porwal, Karthik Ramasubramanian, Nick Vaccaro, Subrata Banik.
Jérémy Compostella has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86356?usp=email )
Change subject: soc/intel/alderlake: Add early power off support
......................................................................
Patch Set 1:
(1 comment)
File src/soc/intel/alderlake/reset.c:
https://review.coreboot.org/c/coreboot/+/86356/comment/2aa9b213_44fe50a1?us… :
PS1, Line 20: #if CONFIG(PLATFORM_SUPPORTS_EARLY_POWEROFF)
I have been thinking about the location of this code. You are binding a function to another upon on a platform dependency.
Wouldn't it make more sense to have it in `mainboard/google/brox` for example. I do not think this is justified to have `CONFIG(EC_GOOGLE_CHROMEEC)` in `soc/intel/alderlake` while this is implicitly available in `mainboard/google/brox`.
--
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: 1
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: 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: Subrata Banik <subratabanik(a)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-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Tue, 11 Feb 2025 16:54:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Dinesh Gehlot, John Su, Kapil Porwal, Nick Vaccaro, Subrata Banik.
Matt DeVillier has posted comments on this change by John Su. ( https://review.coreboot.org/c/coreboot/+/86338?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: mb/google/trulo/var/uldrenite: Add fw_config probe for USB-C ports
......................................................................
Patch Set 1:
(1 comment)
File src/mainboard/google/brya/variants/uldrenite/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/86338/comment/025849be_0d88e81b?us… :
PS1, Line 12: option MB_C_ONE 1
so this option isn't being used yet?
--
To view, visit https://review.coreboot.org/c/coreboot/+/86338?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: Ic0161e8b85c5a2afe6ec8473497d21a5737f4f70
Gerrit-Change-Number: 86338
Gerrit-PatchSet: 1
Gerrit-Owner: John Su <john_su(a)compal.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: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Dtrain Hsu <dtrain_hsu(a)compal.corp-partner.google.com>
Gerrit-CC: Jamie Chen <jamie_chen(a)compal.corp-partner.google.com>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-CC: Van Chen <van_chen(a)compal.corp-partner.google.com>
Gerrit-Attention: John Su <john_su(a)compal.corp-partner.google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)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: Tue, 11 Feb 2025 16:30:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Alicja Michalska, Arthur Heymans, David Hendricks, Felix Held.
Matt DeVillier has posted comments on this change by Alicja Michalska. ( https://review.coreboot.org/c/coreboot/+/86131?usp=email )
Change subject: device/Kconfig: Make option to allocate above 4G appear in Kconfig
......................................................................
Patch Set 5:
(1 comment)
File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/86131/comment/b77cbe5d_22631676?us… :
PS5, Line 1013: bool "Extend resource window for PCIe devices"
```suggestion
bool "Extend resource window for PCIe devices above 4G"
```
--
To view, visit https://review.coreboot.org/c/coreboot/+/86131?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: Ia17b3312016409d8fd6bcce4321481a7b7e35ce5
Gerrit-Change-Number: 86131
Gerrit-PatchSet: 5
Gerrit-Owner: Alicja Michalska <ahplka19(a)gmail.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Alicja Michalska <ahplka19(a)gmail.com>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Tue, 11 Feb 2025 16:09:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Fred Reitberger, Jason Glenesk, Martin L Roth, Matt DeVillier, Nick Kochlowski, Varshit Pandya.
Felix Held has posted comments on this change by Nick Kochlowski. ( https://review.coreboot.org/c/coreboot/+/85634?usp=email )
Change subject: drivers/amd/opensil/memmap.c: Factor out common memmap code to driver
......................................................................
Patch Set 21:
(2 comments)
File src/drivers/amd/opensil/memmap.c:
https://review.coreboot.org/c/coreboot/+/85634/comment/f70f7e2a_9d87d25a?us… :
PS21, Line 50: strcmp
shouldn't this be 'if (!strcmp(...))'? strcmp return 0 on match and non-zero on mismatch and the existing code did a continue when this is a MMIO range
https://review.coreboot.org/c/coreboot/+/85634/comment/774da3eb_25bbf2fe?us… :
PS21, Line 56: if (strcmp(opensil_get_hole_info_type(holes[hole].type), "UMA"))
probably same here
--
To view, visit https://review.coreboot.org/c/coreboot/+/85634?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: I80b9bdd7fd633c7b12d695ced5d4b9b518570d80
Gerrit-Change-Number: 85634
Gerrit-PatchSet: 21
Gerrit-Owner: Nick Kochlowski <nickkochlowski(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Varshit Pandya <pandyavarshit(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Varshit Pandya <pandyavarshit(a)gmail.com>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Nick Kochlowski <nickkochlowski(a)gmail.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Comment-Date: Tue, 11 Feb 2025 16:06:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Bao Zheng, Felix Held, Julius Werner, Marshall Dawson, Zheng Bao, ritul guru.
Maximilian Brune has posted comments on this change by Bao Zheng. ( https://review.coreboot.org/c/coreboot/+/85466?usp=email )
Change subject: amdfwtool: Set address mode of BIOS binary as context defines
......................................................................
Patch Set 9: Code-Review+2
(1 comment)
Patchset:
PS9:
I tested on birman-plus: The binary changed as expected, but the size of all CBFS files is the same (build using BUILD_TIMELESS). birman-plus also still boots fine with A/B Recovery layout.
--
To view, visit https://review.coreboot.org/c/coreboot/+/85466?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: I9c2f2983d03c913b28fbd87aa0925a32a4649d62
Gerrit-Change-Number: 85466
Gerrit-PatchSet: 9
Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Reviewer: Zheng Bao
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: ritul guru <ritul.bits(a)gmail.com>
Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: ritul guru <ritul.bits(a)gmail.com>
Gerrit-Attention: Zheng Bao
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Tue, 11 Feb 2025 16:06:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
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.
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/+/86227?usp=email
to look at the new patch set (#10).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: soc/intel/adl: Handle critical low battery early in romstage
......................................................................
soc/intel/adl: Handle critical low battery early in romstage
This commit implements early handling of critical low battery
conditions in the romstage for Alder Lake platforms.
A message is displayed to the user via
ux_inform_user_of_poweroff_operation. A short delay is introduced to
allow the user to see the message. A low battery event is logged.
The system is shut down via the Chrome EC.
This early handling prevents the system from proceeding with
boot (while performing firmware update) if the battery is critically
low and ensures a clean shutdown. This is particularly important for
ChromeOS devices.
BUG=b:339673254
TEST=Verified low battery boot event logging and controlled shutdown.
Change-Id: Ib4be86ed17818ee05b7bec0337a90f80017183c2
Signed-off-by: Subrata Banik <subratabanik(a)google.com>
---
M src/soc/intel/alderlake/romstage/fsp_params.c
1 file changed, 24 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/86227/10
--
To view, visit https://review.coreboot.org/c/coreboot/+/86227?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: Ib4be86ed17818ee05b7bec0337a90f80017183c2
Gerrit-Change-Number: 86227
Gerrit-PatchSet: 10
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: 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-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Attention is currently required from: Julius Werner, Karthik Ramasubramanian.
Hello Julius Werner, Karthik Ramasubramanian, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/86228?usp=email
to look at the new patch set (#10).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: vc/google/chromeos: Implement platform callback for critical shutdown
......................................................................
vc/google/chromeos: Implement platform callback for critical shutdown
This commit implements `platform_is_low_battery_shutdown_needed` and
callback for ChromeOS.
- platform_is_low_battery_shutdown_needed: API to check if low battery
shutdown is needed.
BUG=b:339673254
TEST=Verified low battery boot event logging and controlled shutdown.
Change-Id: I119f80a45c045a6095cae98f179c755a2e948e9c
Signed-off-by: Subrata Banik <subratabanik(a)google.com>
---
M src/vendorcode/google/chromeos/Makefile.mk
A src/vendorcode/google/chromeos/battery.c
2 files changed, 37 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/86228/10
--
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: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I119f80a45c045a6095cae98f179c755a2e948e9c
Gerrit-Change-Number: 86228
Gerrit-PatchSet: 10
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>