coreboot-gerrit April 2023

coreboot-gerrit@coreboot.org

1 participants
5761 discussions

[L] Change in coreboot[master]: soc/intel/cmn/cse: Handle EOP completion asynchronously
by Nick Vaccaro (Code Review) 05 Apr '23

05 Apr '23

Attention is currently required from: Subrata Banik, Jérémy Compostella, Nick Vaccaro. Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74214 ) Change subject: soc/intel/cmn/cse: Handle EOP completion asynchronously ...................................................................... Patch Set 5: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/74214 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I01a56bfe3f6c37ffb5e51a527d9fe74785441c5a Gerrit-Change-Number: 74214 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Comment-Date: Wed, 05 Apr 2023 22:25:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: 3rdparty/fsp: Update pointer from 6f2f17f3d339 to cf40b9ec6c7b
by Felix Singer (Code Review) 05 Apr '23

05 Apr '23

Attention is currently required from: Martin L Roth, Subrata Banik, Elyes Haouas. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74242 ) Change subject: 3rdparty/fsp: Update pointer from 6f2f17f3d339 to cf40b9ec6c7b ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Looks like the coreboot region for ChromeOS builds is too small with recent FSP binaries? -- To view, visit https://review.coreboot.org/c/coreboot/+/74242 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I583fc3af6eff823ef1e5ddc8bde9e042691358b6 Gerrit-Change-Number: 74242 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Comment-Date: Wed, 05 Apr 2023 22:16:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/starlabs/starbook/adl: Correct the number of NID entries
by Sean Rhodes (Code Review) 05 Apr '23

05 Apr '23

Attention is currently required from: Matt DeVillier, Paul Menzel. Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74241 ) Change subject: mb/starlabs/starbook/adl: Correct the number of NID entries ...................................................................... Patch Set 2: (2 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/74241/comment/4cb52768_ee0fe59e PS1, Line 8: > Please elaborate. What didn’t work? […] Done File src/mainboard/starlabs/starbook/variants/adl/hda_verb.c: https://review.coreboot.org/c/coreboot/+/74241/comment/23f77a6e_b56d6dd3 PS1, Line 57: 10, /* Number of 4 dword sets */ > That’s not mentioned in the commit message. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/74241 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I79825313a4801c120a0a2a321cbabab7c728aa71 Gerrit-Change-Number: 74241 Gerrit-PatchSet: 2 Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Wed, 05 Apr 2023 21:45:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/starlabs/starbook/adl: Correct the number of NID entries
by Sean Rhodes (Code Review) 05 Apr '23

05 Apr '23

Attention is currently required from: Sean Rhodes, Matt DeVillier. Hello build bot (Jenkins), Matt DeVillier, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/74241 to look at the new patch set (#2). Change subject: mb/starlabs/starbook/adl: Correct the number of NID entries ...................................................................... mb/starlabs/starbook/adl: Correct the number of NID entries The number of NID entries was too high for the Realtek and Intel sound cards, preventing the verb table from loading. Now the values are correct; it loads as intended. Signed-off-by: Sean Rhodes <sean(a)starlabs.systems> Change-Id: I79825313a4801c120a0a2a321cbabab7c728aa71 --- M src/mainboard/starlabs/starbook/variants/adl/hda_verb.c 1 file changed, 16 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/74241/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/74241 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I79825313a4801c120a0a2a321cbabab7c728aa71 Gerrit-Change-Number: 74241 Gerrit-PatchSet: 2 Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[master]: 3rdparty/fsp: Update pointer from 6f2f17f3d339 to cf40b9ec6c7b
by Felix Singer (Code Review) 05 Apr '23

05 Apr '23

Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/74242 ) Change subject: 3rdparty/fsp: Update pointer from 6f2f17f3d339 to cf40b9ec6c7b ...................................................................... 3rdparty/fsp: Update pointer from 6f2f17f3d339 to cf40b9ec6c7b Change-Id: I583fc3af6eff823ef1e5ddc8bde9e042691358b6 Signed-off-by: Felix Singer <felixsinger(a)posteo.net> --- M 3rdparty/fsp 1 file changed, 11 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/74242/1 diff --git a/3rdparty/fsp b/3rdparty/fsp index 6f2f17f..cf40b9e 160000 --- a/3rdparty/fsp +++ b/3rdparty/fsp @@ -1 +1 @@ -Subproject commit 6f2f17f3d3397cc2f00a644e218980bb33e06f66 +Subproject commit cf40b9ec6c7b0e4fc00ded7e121ea879cde94f6a -- To view, visit https://review.coreboot.org/c/coreboot/+/74242 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I583fc3af6eff823ef1e5ddc8bde9e042691358b6 Gerrit-Change-Number: 74242 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: newchange

1 0

[S] Change in coreboot[master]: soc/intel/cmn/cse: Fix unnecessary End-Of-Post cases
by Jérémy Compostella (Code Review) 05 Apr '23

05 Apr '23

Jérémy Compostella has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/74205 ) Change subject: soc/intel/cmn/cse: Fix unnecessary End-Of-Post cases ...................................................................... Abandoned -- To view, visit https://review.coreboot.org/c/coreboot/+/74205 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id2d23df668ab698213e8b1af652ef9b90ccad046 Gerrit-Change-Number: 74205 Gerrit-PatchSet: 3 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: abandon

1 0

[S] Change in coreboot[master]: mb/google/brya: Compile gpio.c in SMM
by Matt DeVillier (Code Review) 05 Apr '23

05 Apr '23

Attention is currently required from: Tarun Tuli. Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74233 ) Change subject: mb/google/brya: Compile gpio.c in SMM ...................................................................... Patch Set 2: (1 comment) File src/mainboard/google/brya/variants/baseboard/brask/Makefile.inc: https://review.coreboot.org/c/coreboot/+/74233/comment/25751c66_9e54f7d0 PS1, Line 9: smm-y += gpio.c > would it make sense to make this conditional on the config option? potentially. I'm not sure why it was done unconditionally on previous boards, since SMMSTORE support wasn't really a consideration when adding the board. I don't have any problem with making it conditional here -- To view, visit https://review.coreboot.org/c/coreboot/+/74233 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If049cba98f13f060807058029306dcad2ada2d49 Gerrit-Change-Number: 74233 Gerrit-PatchSet: 2 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Comment-Date: Wed, 05 Apr 2023 21:31:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tarun Tuli <taruntuli(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/google/brya: Enable asynchronous End-Of-Post
by Jérémy Compostella (Code Review) 05 Apr '23

05 Apr '23

Attention is currently required from: Subrata Banik, Nick Vaccaro. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74215 ) Change subject: mb/google/brya: Enable asynchronous End-Of-Post ...................................................................... Patch Set 5: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/74215 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib850330fbb9e84839eb1093db054332cbcb59b41 Gerrit-Change-Number: 74215 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Comment-Date: Wed, 05 Apr 2023 21:31:23 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in coreboot[master]: soc/intel/cmn/cse: Handle EOP completion asynchronously
by Jérémy Compostella (Code Review) 05 Apr '23

05 Apr '23

Attention is currently required from: Subrata Banik, Nick Vaccaro. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74214 ) Change subject: soc/intel/cmn/cse: Handle EOP completion asynchronously ...................................................................... Patch Set 5: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/74214 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I01a56bfe3f6c37ffb5e51a527d9fe74785441c5a Gerrit-Change-Number: 74214 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Comment-Date: Wed, 05 Apr 2023 21:31:18 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in coreboot[master]: soc/intel/cmn/cse: Handle EOP completion asynchronously
by Jérémy Compostella (Code Review) 05 Apr '23

05 Apr '23

Attention is currently required from: Subrata Banik, Jérémy Compostella, Nick Vaccaro. Jérémy Compostella has uploaded a new patch set (#5) to the change originally created by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/74214 ) Change subject: soc/intel/cmn/cse: Handle EOP completion asynchronously ...................................................................... soc/intel/cmn/cse: Handle EOP completion asynchronously coreboot supports three instances of sending EOP: 1. At CSE `.final' device operation 2. Early as with Alder Lake in chip_operations.init if `SOC_INTEL_CSE_SEND_EOP_EARLY' is selected 3. At BS_PAYLOAD_BOOT as designed for Meteor Lake if `SOC_INTEL_CSE_SEND_EOP_LATE' is selected Currently, Alder Lake uses #3 as it results in better and more stable boot time. However, what would deliver even better result is to not actively wait for CSE completion. This patch introduces a new `SOC_INTEL_CSE_SEND_EOP_ASYNC' Kconfig which split the action of sending EOP request and receiving EOP completion response from the CSE. This patch used in conjunction with #1 can significantly improves the overall boot time on a Raptor Lake design. For example `SOC_INTEL_CSE_SEND_EOP_ASYNC' on a skolas board can deliver up to 36 ms boot time improvement as illustrated below. | # | Late EOP | Async EOP | |----------+----------+-----------| | 1 | 1020.052 | 971.272 | | 2 | 1015.911 | 971.821 | | 3 | 1038.415 | 1021.841 | | 4 | 1020.657 | 993.751 | | 5 | 1065.128 | 1020.951 | | 6 | 1037.859 | 1023.326 | | 7 | 1042.010 | 984.412 | |----------+----------+-----------| | Mean | 1034.29 | 998.20 | | Variance | 4.76 % | 5.21 % | The improvement is not stable but comparing coreboot and FSP performance timestamps demonstrate that the slowness is caused by a lower memory frequency (SaGv point) at early boot which is not an issue addressed by this patch. We also observe some improvement on an Alder Lake design. For example, the same configuration on a kano board can deliver up to 10 ms boot time improvement as illustrated below. | # | Late EOP | Async EOP | |----------+----------+-----------| | 0 | 1067.719 | 1050.106 | | 1 | 1058.263 | 1056.836 | | 2 | 1064.091 | 1056.709 | | 3 | 1068.614 | 1055.042 | | 4 | 1065.749 | 1056.732 | | 5 | 1069.838 | 1057.846 | | 6 | 1066.897 | 1053.548 | | 7 | 1060.850 | 1051.911 | |----------+----------+-----------| | Mean | 1065.25 | 1054.84 | The improvement is more limited on kano because a longer PCIe initialization delays EOP in the Late EOP configuration which make it faster to complete. CSME team confirms that: 1. End-Of-Post is a blocking command in the sense that BIOS is requested to wait for the command completion before loading the OS or second stage bootloader. 2. The BIOS is not required to actively wait for completion of the command and can perform other operations in the meantime as long as they do not involve HECI commands. On Raptor Lake, coreboot does not send any HECI command after End-Of-Post. FSP-s code review did not reveal any HECI command being sent as part of the `AFTER_PCI_ENUM', `READY_TO_BOOT' or `END_OF_FIRMWARE' notifications. If any HECI send and receive command has been sent the extra code added in `cse_receive_eop()' should catch it. According to commit 387ec919d9f7 ("soc/intel/alderlake: Select SOC_INTEL_CSE_SEND_EOP_LATE"), FSP-silicon can sometimes (on the first boot after flashing of a Marasov board for instance) request coreboot to perform a global request out of AFTER_PCI_ENUM notification. Global request relies on a HECI command. Even though, we tested that it does not create any issue, `SOC_INTEL_CSE_SEND_EOP_ASYNC' flag should not be associated to the `SOC_INTEL_CSE_SEND_EOP_EARLY' flag to prevent potential a global reset command to "conflict" with the EOP command. This patch also introduces a new code logic to detect if CSE is in the right state to handle the EOP command. Otherwise, it uses the prescribed method to make the CSE function disable. The typical scenario is the ChromeOS recovery boot where CSE stays in RO partition and therefore EOP command should be avoided. [DEBUG] BS: BS_PAYLOAD_LOAD exit times (exec / console): 0 / 14 ms [INFO ] HECI: coreboot in recovery mode; found CSE in expected SOFT TEMP DISABLE state, skipping EOP [INFO ] Disabling Heci using PMC IPC [WARN ] HECI: CSE device 16.0 is hidden [WARN ] HECI: CSE device 16.1 is disabled [WARN ] HECI: CSE device 16.2 is disabled [WARN ] HECI: CSE device 16.3 is disabled [WARN ] HECI: CSE device 16.4 is disabled [WARN ] HECI: CSE device 16.5 is disabled BUG=b:276339544 BRANCH=firmware-brya-14505.B TEST=Tests on brya0 with and `SOC_INTEL_CSE_SEND_EOP_ASYNC' show End-Of-Post sent soon after FSP-s and EOP message receive at `BS_PAYLOAD_BOOT'. Verify robustness by injecting a `GET_BOOT_STATE' HECI command with or without `heci_reset'. The implementation always successfully completed the EOP before moving to the payload. As expected, the boot time benefit of the asynchronous solution was under some injection scenario undermined by this unexpected HECI command. Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com> Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: I01a56bfe3f6c37ffb5e51a527d9fe74785441c5a --- M src/soc/intel/common/block/cse/Kconfig M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/cse/cse_eop.c 3 files changed, 256 insertions(+), 45 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/74214/5 -- To view, visit https://review.coreboot.org/c/coreboot/+/74214 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I01a56bfe3f6c37ffb5e51a527d9fe74785441c5a Gerrit-Change-Number: 74214 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-MessageType: newpatchset

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2023