Attention is currently required from: Hou-hsun Lee, Paul Menzel, Nick Vaccaro, Zhuohao Lee, Alan Huang.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58241 )
Change subject: mb/google/brya/var/baseboard/brask: Add power limits functions
......................................................................
Patch Set 14:
(7 comments)
File src/mainboard/google/brya/variants/baseboard/brask/ramstage.c:
https://review.coreboot.org/c/coreboot/+/58241/comment/e5e248e1_b91f5860
PS12, Line 20: bool get_sku_index(const struct cpu_power_limits *limits, size_t num_entries,
> Fixed.
Done
https://review.coreboot.org/c/coreboot/+/58241/comment/b16d3cb2_079d6133
PS12, Line 121:
> Fixed.
Done
File src/mainboard/google/brya/variants/brask/ramstage.c:
PS12:
> Separate to here https://review.coreboot. […]
Done
https://review.coreboot.org/c/coreboot/+/58241/comment/3586bd4a_e7aaa981
PS12, Line 21: *
> Fixed.
Done
https://review.coreboot.org/c/coreboot/+/58241/comment/e62c8d45_91030e66
PS12, Line 21: *
> Fixed.
Done
https://review.coreboot.org/c/coreboot/+/58241/comment/5a538692_964dc0c1
PS12, Line 41: barral
> Fixed.
Done
https://review.coreboot.org/c/coreboot/+/58241/comment/883d1211_1b42f641
PS12, Line 41: barral
> Fixed.
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/58241
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I183017068e9c78acb9fa7073c53593d304ba9248
Gerrit-Change-Number: 58241
Gerrit-PatchSet: 14
Gerrit-Owner: Alan Huang <alan-huang(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Zhuohao Lee <zhuohao(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: David Wu <david_wu(a)quanta.corp-partner.google.com>
Gerrit-CC: Hou-hsun Lee <hou-hsun.lee(a)intel.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Hou-hsun Lee <hou-hsun.lee(a)intel.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Zhuohao Lee <zhuohao(a)google.com>
Gerrit-Attention: Alan Huang <alan-huang(a)quanta.corp-partner.google.com>
Gerrit-Comment-Date: Tue, 23 Nov 2021 22:54:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Comment-In-Reply-To: Alan Huang <alan-huang(a)quanta.corp-partner.google.com>
Gerrit-MessageType: comment
Attention is currently required from: Kane Chen, Nick Vaccaro, Patrick Rudolph.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59562 )
Change subject: soc/intel/adl: Modify SOC_INTEL_ALDERLAKE_DEBUG_CONSENT default value
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/59562
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie6fbf3cdcf5dcd1a11a895ea83f55157a2ac4eb9
Gerrit-Change-Number: 59562
Gerrit-PatchSet: 4
Gerrit-Owner: Kane Chen <kane.chen(a)intel.corp-partner.google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Kane Chen <kane.chen(a)intel.corp-partner.google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Tue, 23 Nov 2021 22:52:29 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Karthik Ramasubramanian.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58954 )
Change subject: lib/cbfs: Use a work queue for cbfs_preload
......................................................................
Patch Set 1:
(4 comments)
File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/58954/comment/9d5d4b44_bcad46ab
PS1, Line 276: /* Only valid when done == true */
nit: maybe easier to just add a CB_IN_PROGRESS error type to model this?
https://review.coreboot.org/c/coreboot/+/58954/comment/f23bbfa3_8d255086
PS1, Line 313: list_for_each(context, cbfs_preload_context_list, list_node) {
Would be good to add a comment here (or somewhere) explaining why we don't need a mutex to prevent concurrent access to the list.
https://review.coreboot.org/c/coreboot/+/58954/comment/1073bbd3_04566a32
PS1, Line 328: DEBUG("%s: cbfs preload thread done\n", __func__);
Hmmm... so the one thing that concerns me a bit is that there are a lot of implicit assumptions about what constitutes an automatic critical section in our concurrency model here. For example, you rely on the fact that there can be no yield() in the middle of a list_append() call to make sure the list is always in a consistent state for the reader thread. Or here, you rely on the fact that there's no yield() between the last check in the list_for_each() loop (where it decides it has reached the end) and the end of the thread, to ensure that no new context may have been enqueued after we were done processing the list but before the thread state changed to THREAD_DONE.
Right now it's pretty easy to guarantee all these are true (you are guaranteeing that there can be no yield() in a printk(), right? Or did I misunderstand that?), but it just seems a bit "wrong" to the normal concurrency-aware programmer's mind. Dunno what the right solution here is... to just keep it this way and accept that coreboot concurrency is "special" and you have to manually pay attention to things that can udelay(), or to splatter the code with a ton of extra mutexes (or at least disable_coop()) that we know we don't really need. Maybe at least add comments warning about the various implicit assumptions?
(Maybe in this case it would be good enough to just have this function disable_coop() right at the start, and then only enable it inside the loop right before calling rdev_readat(), and disable it again before the end of the loop. That way all the list accesses and other areas of concern would be in the guaranteed coop-free zone, and disable_coop() is a bit cheaper than a mutex variable.)
https://review.coreboot.org/c/coreboot/+/58954/comment/8ae23d5a_e8e08acf
PS1, Line 337: switch (cbfs_preload_handle.state) {
nit: bit complicated way of saying
if (cbfs_preload_handle.state == THREAD_DONE)
return CB_SUCCESS;
--
To view, visit https://review.coreboot.org/c/coreboot/+/58954
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I965385e70793557cb14e4b925ae9afbe66a12c0e
Gerrit-Change-Number: 58954
Gerrit-PatchSet: 1
Gerrit-Owner: Raul Rangel <rrangel(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-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Tue, 23 Nov 2021 22:25:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Martin Roth.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59607 )
Change subject: Makefile: Add util/kconfig/Makefile.real to nocompile list
......................................................................
Patch Set 2: Code-Review+1
(2 comments)
File Makefile:
https://review.coreboot.org/c/coreboot/+/59607/comment/b3a15aa1_ef9a4a89
PS2, Line 108: Four
nit: How about "The cases" (or "Enumeration of cases"), so we won't have to edit that line anymore?
https://review.coreboot.org/c/coreboot/+/59607/comment/74db688f_55a91ccd
PS2, Line 110: and the make target tools
I don't understand that part of the sentence.
--
To view, visit https://review.coreboot.org/c/coreboot/+/59607
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: If29c3a84c7c82ea099ef9610f4ecaa599f0d8649
Gerrit-Change-Number: 59607
Gerrit-PatchSet: 2
Gerrit-Owner: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Comment-Date: Tue, 23 Nov 2021 21:56:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59550 )
Change subject: rules.h, thread.h, lib/cbfs: Add ENV_STAGE_SUPPORTS_COOP
......................................................................
Patch Set 1:
(1 comment)
File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/59550/comment/ff50f4b8_4cb2cb3f
PS1, Line 306: ENV_ROMSTAGE
I just realized there may be a bit of an issue with preloading in romstage in combination with VBOOT_STARTS_IN_ROMSTAGE... in that case, the vboot code is linked into the romstage and runs as part of the run_ramstage() call. As a result, the behavior of cbfs_boot_lookup() changes. So files preloaded before the vboot code runs would no longer be valid afterwards.
No platforms anybody still cares about today really use VBOOT_STARTS_IN_ROMSTAGE, but we shouldn't just leave a potential bug lying around... how about just preventing the issue by making this (ENV_ROMSTAGE && !CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) here?
--
To view, visit https://review.coreboot.org/c/coreboot/+/59550
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1197406d1d36391998b08e3076146bb2fff59d00
Gerrit-Change-Number: 59550
Gerrit-PatchSet: 1
Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Comment-Date: Tue, 23 Nov 2021 21:55:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Hung-Te Lin, Furquan Shaikh, Mathew King, Paul Menzel, Julius Werner.
Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57864 )
Change subject: guybrush: add RO_GSCVD area to FMAP
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
File src/mainboard/google/guybrush/chromeos.fmd:
https://review.coreboot.org/c/coreboot/+/57864/comment/10e55afd_233e5373
PS1, Line 29: RO_GSCVD 8K
> > I thing RO_GSCVD should be part of the RO section, and should be updated only if WP is disabled an […]
ah, I am confusing RO_SECTION and WP_RO, moved to WP_RO, put it after VPD which is not supposed to be changing with firmware updates.
--
To view, visit https://review.coreboot.org/c/coreboot/+/57864
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ifa24d5a6271a8bcbf737d4580ec85b9cfdd9af01
Gerrit-Change-Number: 57864
Gerrit-PatchSet: 2
Gerrit-Owner: Vadim Bendebury <vbendeb(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan.m.shaikh(a)gmail.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Mathew King <mathewk(a)chromium.org>
Gerrit-Reviewer: Vadim Bendebury <vbendeb(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Furquan Shaikh <furquan.m.shaikh(a)gmail.com>
Gerrit-Attention: Mathew King <mathewk(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Tue, 23 Nov 2021 21:34:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Hung-Te Lin <hungte(a)chromium.org>
Comment-In-Reply-To: Vadim Bendebury <vbendeb(a)chromium.org>
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Hung-Te Lin, Vadim Bendebury, Furquan Shaikh, Mathew King, Paul Menzel.
Hello Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Mathew King, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/57864
to look at the new patch set (#2).
Change subject: guybrush: add RO_GSCVD area to FMAP
......................................................................
guybrush: add RO_GSCVD area to FMAP
This area is used for storing AP RO verification information.
BRANCH=none
BUG=b:141191727
TEST=built a guybrush firmware image and verified that the RO_GSCVD
area was indeed added:
$ dump_fmap /build/guybrush/firmware/image-guybrush.bin | \
grep -B3 RO_GSCVD
area: 25
area_offset: 0x00808000
area_size: 0x00002000 (8192)
area_name: RO_GSCVD
$
- verified that guybrush device boots fine with the new image.
Change-Id: Ifa24d5a6271a8bcbf737d4580ec85b9cfdd9af01
Signed-off-by: Vadim Bendebury <vbendeb(a)chromium.org>
---
M src/mainboard/google/guybrush/chromeos.fmd
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/57864/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/57864
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ifa24d5a6271a8bcbf737d4580ec85b9cfdd9af01
Gerrit-Change-Number: 57864
Gerrit-PatchSet: 2
Gerrit-Owner: Vadim Bendebury <vbendeb(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan.m.shaikh(a)gmail.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Mathew King <mathewk(a)chromium.org>
Gerrit-Reviewer: Vadim Bendebury <vbendeb(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Vadim Bendebury <vbendeb(a)chromium.org>
Gerrit-Attention: Furquan Shaikh <furquan.m.shaikh(a)gmail.com>
Gerrit-Attention: Mathew King <mathewk(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Raul Rangel.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59550 )
Change subject: rules.h, thread.h, lib/cbfs: Add ENV_STAGE_SUPPORTS_COOP
......................................................................
Patch Set 1:
(1 comment)
File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/59550/comment/4d9803f1_2a900706
PS1, Line 306: ENV_X86
Why check this explicitly? It currently only works on x86 but that's already modeled by the Kconfig, and we'll hopefully expand it some day.
--
To view, visit https://review.coreboot.org/c/coreboot/+/59550
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1197406d1d36391998b08e3076146bb2fff59d00
Gerrit-Change-Number: 59550
Gerrit-PatchSet: 1
Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Comment-Date: Tue, 23 Nov 2021 21:29:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Patrick Rudolph, Christian Walter, Arthur Heymans.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59545 )
Change subject: mb/prodrive/hermes: Update r04 front audio config
......................................................................
Patch Set 2:
(1 comment)
File src/mainboard/prodrive/hermes/variants/r04/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/59545/comment/da9047a2_37577778
PS1, Line 48: board_cfg->
> drop this
Oops, silly me. Thanks!
--
To view, visit https://review.coreboot.org/c/coreboot/+/59545
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3760f0a25e964cf0eba99d180fd6f3e8488af868
Gerrit-Change-Number: 59545
Gerrit-PatchSet: 2
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Tue, 23 Nov 2021 21:26:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Philipp Hug, Ron Minnich.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59546 )
Change subject: arch/{arm,arm64,ppc64,riscv}: Add noop cpu_relax
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/59546
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ifcb4546ceb2894eeb37589d0282b7e076d7a4747
Gerrit-Change-Number: 59546
Gerrit-PatchSet: 2
Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx>
Gerrit-Reviewer: Ron Minnich <rminnich(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Philipp Hug <philipp(a)hug.cx>
Gerrit-Attention: Ron Minnich <rminnich(a)gmail.com>
Gerrit-Comment-Date: Tue, 23 Nov 2021 21:20:18 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment