Attention is currently required from: Shelley Chen, Ravi Kumar Bokka.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63085 )
Change subject: Performance improvement by removing delays in cpucp init
......................................................................
Patch Set 3:
(4 comments)
File src/mainboard/google/herobrine/romstage.c:
https://review.coreboot.org/c/coreboot/+/63085/comment/8e52c5d3_a540de31
PS3, Line 19: void cpucp_prepare(void)
You should keep this function in SoC code and just link that file into romstage, then call it from here.
https://review.coreboot.org/c/coreboot/+/63085/comment/76000d5f_1834543e
PS3, Line 33: cpucp_prepare();
Does this need to run after QcLib? If so, please explain why in a comment. If not, probably better to move it before QcLib because that's the biggest time sink here in romstage.
File src/soc/qualcomm/sc7280/cpucp_load_reset.c:
https://review.coreboot.org/c/coreboot/+/63085/comment/b5b453e0_08a82c1f
PS3, Line 12:
Should be no more than one blank line here.
https://review.coreboot.org/c/coreboot/+/63085/comment/77756b5f_6a2f1e05
PS3, Line 15: {
You still need to keep the wait_ms(...) here because there's no guarantee that enough time passed between romstage and now.
--
To view, visit https://review.coreboot.org/c/coreboot/+/63085
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1a727514810a505cd1005ae7f52e5215e404b3bb
Gerrit-Change-Number: 63085
Gerrit-PatchSet: 3
Gerrit-Owner: Ravi Kumar Bokka <rbokka(a)codeaurora.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sudheer Amrabadi <samrabad(a)codeaurora.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Ravi Kumar Bokka <rbokka(a)codeaurora.org>
Gerrit-Comment-Date: Wed, 06 Apr 2022 02:42:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Zheng Bao, Felix Held.
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63344 )
Change subject: amdfwtool: Add a flag to record the second gen instead of romsig
......................................................................
Patch Set 4:
(1 comment)
File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/63344/comment/6930eecc_d7c6c472
PS3, Line 1387: if (cb_config->second_gen) {
> i think this has the logic backwards since EFS_SECOND_GEN is defined as 0. […]
Done.
A stupid mistake.
--
To view, visit https://review.coreboot.org/c/coreboot/+/63344
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id50311034b46aa1791dcc10b107de4af6c86b927
Gerrit-Change-Number: 63344
Gerrit-PatchSet: 4
Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Zheng Bao
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Attention: Zheng Bao
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Wed, 06 Apr 2022 02:27:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-MessageType: comment
Attention is currently required from: Bao Zheng, Zheng Bao.
Hello build bot (Jenkins), Zheng Bao,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/63344
to look at the new patch set (#4).
Change subject: amdfwtool: Add a flag to record the second gen instead of romsig
......................................................................
amdfwtool: Add a flag to record the second gen instead of romsig
This is for future feature combo, which gets the soc id from fw.cfg in
a loop instead of the command line, and the romsig is not set until
fw.cfg is processed.
Change-Id: Id50311034b46aa1791dcc10b107de4af6c86b927
Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com>
---
M util/amdfwtool/amdfwtool.c
M util/amdfwtool/amdfwtool.h
2 files changed, 38 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/63344/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/63344
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id50311034b46aa1791dcc10b107de4af6c86b927
Gerrit-Change-Number: 63344
Gerrit-PatchSet: 4
Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Zheng Bao
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Attention: Zheng Bao
Gerrit-MessageType: newpatchset
Attention is currently required from: Reka Norman, Tim Wawrzynczak.
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63368 )
Change subject: mb/google/brya/var/nereid: Add WLAN power sequence
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/63368
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1ca46d9649eff3f96a0e77db594d87288b29a83a
Gerrit-Change-Number: 63368
Gerrit-PatchSet: 3
Gerrit-Owner: Reka Norman <rekanorman(a)chromium.org>
Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Reka Norman <rekanorman(a)google.com>
Gerrit-Attention: Reka Norman <rekanorman(a)chromium.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Comment-Date: Wed, 06 Apr 2022 01:18:36 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Reka Norman, Kangheui Won, Tim Wawrzynczak.
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63370 )
Change subject: mb/google/brya/var/nereid: Enable pen garage
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> Not yet. Sam, could you test this? (The pen garage on my nereid seems broken. […]
Can confirm pen eject wakes from S0ix with the expected wake source in the eventlog:
Wake Source | GPE # | 79
--
To view, visit https://review.coreboot.org/c/coreboot/+/63370
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I0d5134737fc758a43e1fff95e9f2a20200991bb1
Gerrit-Change-Number: 63370
Gerrit-PatchSet: 1
Gerrit-Owner: Reka Norman <rekanorman(a)chromium.org>
Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org>
Gerrit-Reviewer: Reka Norman <rekanorman(a)google.com>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Reka Norman <rekanorman(a)google.com>
Gerrit-Attention: Kangheui Won <khwon(a)chromium.org>
Gerrit-Attention: Reka Norman <rekanorman(a)chromium.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Comment-Date: Wed, 06 Apr 2022 01:17:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Reka Norman <rekanorman(a)chromium.org>
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Hung-Te Lin, Jason Glenesk, Raul Rangel, Marshall Dawson, Tim Wawrzynczak, Christian Walter, Arthur Heymans, Kyösti Mälkki, Andrey Petrov, Fred Reitberger, Yu-Ping Wu, Felix Held.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63375 )
Change subject: [RFC] CBMEM: Have INIT_HOOKS in every stage
......................................................................
Patch Set 1:
(4 comments)
Patchset:
PS1:
Not a fan of the extra binary size impact that comes from this. Registering and running through a no-op function hook is not quite as efficient as not registering it at all.
If you want more flexibility to do things like !ENV_ROMSTAGE, how about changing this to CBMEM_INIT_HOOK(function, condition)? `condition` could only be something the preprocessor can evaluate, but that's good enough for ENV_xxx and Kconfigs. (Universal hooks can just use `true` as the condition.)
Also, we don't want to link hook functions into stages that don't even have CBMEM, so it would be good to still do something with __PRE_RAM__ and ENV_STAGE_CREATES_CBMEM to exclude that.
File src/soc/amd/stoneyridge/romstage.c:
https://review.coreboot.org/c/coreboot/+/63375/comment/1e5a966f_103fe282
PS1, Line 218: CBMEM_INIT_HOOK(migrate_power_state)
This looks like a functional change? Intentional? Maybe still put it in a separate commit in that case, for clarity.
File src/soc/intel/baytrail/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/63375/comment/4f963b9c_6454f9ec
PS1, Line 35: CBMEM_INIT_HOOK(migrate_power_state)
Same? This doesn't look like something you'd want to run more than once?
File src/soc/intel/braswell/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/63375/comment/b58603c3_63f59935
PS1, Line 29: CBMEM_INIT_HOOK(migrate_power_state);
Same.
--
To view, visit https://review.coreboot.org/c/coreboot/+/63375
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie24bf4e818ca69f539196c3a814f3c52d4103d7e
Gerrit-Change-Number: 63375
Gerrit-PatchSet: 1
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Wed, 06 Apr 2022 01:11:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Jes Klinke, Philipp Deppenwiese, Patrick Rudolph, Christian Walter, Werner Zeh.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63285 )
Change subject: Factor TI50/CR50 config
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> Currently, I see both CRB_TPM and MAINBOARD_HAS_CRB_TPM in Kconfig. I am not sure how they are used differently, maybe the latter declares that the support exist, and the former can be used to actually enable it in coreboot.
Right, but I don't think there's really a point in those being split (same as for I2C and SPI). The original intent for these splits was that MAINBOARD_HAS_xxx describes the hardware truth of the mainboard, and the other option describes whether the driver is actually enabled. But this only makes sense when the Kconfigs are built to allow for a difference in those choices. You can see this work correctly in MAINBOARD_HAS_TPM2 vs TPM2/NO_TPM... the former describes what the board has and is not user-visible in menuconfig, and the latter can be independently set by the user to decide whether they actually want to enable it. But then this pattern seems to have been copied incorrectly to other areas without really understanding the logic behind this distinction... SPI_TPM/I2C_TPM/CRB_TPM are not menuconfig visible, and they are directly and solely decided by the respective MAINBOARD_HAS Kconfigs. Having two Kconfigs with a direct 1:1 relationship to each other is pointless, that might as well be a single config. So I believe merging them makes sense. (+Adding some people who might have different opinions.)
Since the MAINBOARD_HAS_xxx symbols appear a lot in many board Kconfigs, if you don't want to go through the churn of updating all of those, we could also say that we only keep the MAINBOARD_HAS_I2C_TPM/MAINBOARD_HAS_SPI_TPM and get rid of the I2C_TPM/SPI_TPM options instead? That should make for a smaller patch overall. (Although I'm not sure how much sense this makes to be honest, the option we select when we have a Winbond flash chip on the board is also just called SPI_FLASH_WINBOND, not MAINBOARD_HAS_SPI_FLASH_WINBOND. I think the MAINBOAR_HAS only makes sense if a meaningfully separate option without that exists as well.)
> It will be a major refactoring, and I an worried that it will be hard for me to verify that I am not accidentally changing something unintended for an existing board.
You can never test all boards with large refactorings like this but that's no reason not to make them. This is mostly an "either it builds or it doesn't" issue, so I think with careful reviews and Jenkins we should be fine.
--
To view, visit https://review.coreboot.org/c/coreboot/+/63285
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I037b1b717a16c468e2f97a912da99f125b61e1ce
Gerrit-Change-Number: 63285
Gerrit-PatchSet: 2
Gerrit-Owner: Jes Klinke <jbk(a)chromium.org>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Jes Klinke <jbk(a)chromium.org>
Gerrit-Attention: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Comment-Date: Wed, 06 Apr 2022 00:48:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Jes Klinke <jbk(a)chromium.org>
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Tim Wawrzynczak.
Reka Norman has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63368 )
Change subject: mb/google/brya/var/nereid: Add WLAN power sequence
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
> We actually need EN to be pulled low for longer to fully discharge the rail, so instead drive EN hig […]
Tested that WLAN still enumerates and works even though RST isn't released until ramstage.
--
To view, visit https://review.coreboot.org/c/coreboot/+/63368
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1ca46d9649eff3f96a0e77db594d87288b29a83a
Gerrit-Change-Number: 63368
Gerrit-PatchSet: 3
Gerrit-Owner: Reka Norman <rekanorman(a)chromium.org>
Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Reka Norman <rekanorman(a)google.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Comment-Date: Wed, 06 Apr 2022 00:34:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Reka Norman <rekanorman(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Reka Norman, Tim Wawrzynczak.
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63368 )
Change subject: mb/google/brya/var/nereid: Add WLAN power sequence
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/63368
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1ca46d9649eff3f96a0e77db594d87288b29a83a
Gerrit-Change-Number: 63368
Gerrit-PatchSet: 3
Gerrit-Owner: Reka Norman <rekanorman(a)chromium.org>
Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Reka Norman <rekanorman(a)google.com>
Gerrit-Attention: Reka Norman <rekanorman(a)chromium.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Comment-Date: Wed, 06 Apr 2022 00:19:12 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment