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 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 (#4) 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/4 -- 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: 4 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

[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 (#3) 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(+), 30 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/74214/3 -- 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: 3 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

[S] Change in coreboot[master]: util/testing/Makefile.inc: Fix typo in parameter
by Felix Singer (Code Review) 05 Apr '23

05 Apr '23

Attention is currently required from: Martin L Roth, Paul Menzel. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74240 ) Change subject: util/testing/Makefile.inc: Fix typo in parameter ...................................................................... Patch Set 1: (2 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/74240/comment/3bbd97d6_77085f58 PS1, Line 7: Fix typo in parameter > More specific: Add missing - to scan-build switch +1 Patchset: PS1: Need to check again. I tried building the Docker containers and it failed without this. -- To view, visit https://review.coreboot.org/c/coreboot/+/74240 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iae10f639c43fed7709698e620e732cddce5658d8 Gerrit-Change-Number: 74240 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Wed, 05 Apr 2023 21:19:36 +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 Paul Menzel (Code Review) 05 Apr '23

05 Apr '23

Attention is currently required from: Sean Rhodes, Matt DeVillier. Paul Menzel 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 1: (2 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/74241/comment/12b8a5ba_e4c1501f PS1, Line 8: Please elaborate. What didn’t work? Why are the new values correct? What is the source? File src/mainboard/starlabs/starbook/variants/adl/hda_verb.c: https://review.coreboot.org/c/coreboot/+/74241/comment/5e78f9ec_0ff834d9 PS1, Line 57: 10, /* Number of 4 dword sets */ That’s not mentioned in the commit message. -- 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: 1 Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Comment-Date: Wed, 05 Apr 2023 21:19:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: util/testing/Makefile.inc: Fix typo in parameter
by Paul Menzel (Code Review) 05 Apr '23

05 Apr '23

Attention is currently required from: Felix Singer, Martin L Roth. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74240 ) Change subject: util/testing/Makefile.inc: Fix typo in parameter ...................................................................... Patch Set 1: Code-Review+1 (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/74240/comment/a3cfc0a4_f42ded01 PS1, Line 7: Fix typo in parameter More specific: Add missing - to scan-build switch -- To view, visit https://review.coreboot.org/c/coreboot/+/74240 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iae10f639c43fed7709698e620e732cddce5658d8 Gerrit-Change-Number: 74240 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Comment-Date: Wed, 05 Apr 2023 21:18:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: mb/google/myst: First pass GPIO configuration for Myst
by Paul Menzel (Code Review) 05 Apr '23

05 Apr '23

Attention is currently required from: Raul Rangel, Jon Murphy, Tim Van Patten, Karthik Ramasubramanian, Mark Hasemeyer. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74095 ) Change subject: mb/google/myst: First pass GPIO configuration for Myst ...................................................................... Patch Set 14: (2 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/74095/comment/918dd27d_8f69caf1 PS14, Line 7: First pass GPIO configuration for Myst Please make it a statement by adding a verb (in imperative mood): > Add GPIO initial configuration (“for Myst” is also redundant.) https://review.coreboot.org/c/coreboot/+/74095/comment/b7be3070_10e78d67 PS14, Line 9: Initial GPIO configuration for Myst. What source did you use? -- To view, visit https://review.coreboot.org/c/coreboot/+/74095 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia019704c7b027f14d46281e0de0ffdbc4906a20b Gerrit-Change-Number: 74095 Gerrit-PatchSet: 14 Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Tim Van Patten <timvp(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: Jon Murphy <jpmurphy(a)google.com> Gerrit-Attention: Tim Van Patten <timvp(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com> Gerrit-Comment-Date: Wed, 05 Apr 2023 21:13:09 +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

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 ...................................................................... Set Ready For Review -- 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: 1 Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Comment-Date: Wed, 05 Apr 2023 21:10:02 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

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

05 Apr '23

Attention is currently required from: Matt DeVillier. Tarun Tuli 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 1: (1 comment) File src/mainboard/google/brya/variants/baseboard/brask/Makefile.inc: https://review.coreboot.org/c/coreboot/+/74233/comment/f28cd0c5_df6aa299 PS1, Line 9: smm-y += gpio.c would it make sense to make this conditional on the config option? -- 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: 1 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Comment-Date: Wed, 05 Apr 2023 20:34:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in coreboot[master]: util/inteltool: Add support for Jasper Lake
by Maciej Pijanowski (Code Review) 05 Apr '23

05 Apr '23

Attention is currently required from: Michał Żygowski, Jakub Czapiga, Karol Zmyslowski, Stefan Reinauer, Michal Zygowski. Maciej Pijanowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73934 ) Change subject: util/inteltool: Add support for Jasper Lake ...................................................................... Patch Set 12: Code-Review-1 (23 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/73934/comment/4f1e42f2_543e885d PS9, Line 10: Documents: Intel Datasheet: 633935, rev. 006 > Done Done Patchset: PS12: Still, not all old comments were resolved. New fixes and improvements came with new bugs. I confirmed with datashseet, and found quite a lot of errors, or at least something to explain. Please do. File util/inteltool/gpio_names/jasperlake.h: https://review.coreboot.org/c/coreboot/+/73934/comment/6dc47560_35c824d4 PS12, Line 8: const char *const jasperlake_pch_group_a_names[] = { This group is not used in any of the communities. https://review.coreboot.org/c/coreboot/+/73934/comment/bbfee5cc_d775712b PS12, Line 28: "GPP_A19", "PCHHOT_N", "n/a", After A19, we have ESPI_CLK_LOOPBK in datasheet https://review.coreboot.org/c/coreboot/+/73934/comment/0377aeaf_ddd9114b PS12, Line 63: Please remove empty line https://review.coreboot.org/c/coreboot/+/73934/comment/abba133d_02526e92 PS12, Line 64: "GSPI0_CLK_LOOPBK", "GSPI0_CLK_LOOPBK", "n/a", "n/a", Inconsistent indents in this group after adding LOOBPKG entries https://review.coreboot.org/c/coreboot/+/73934/comment/3f7cc3b8_7e2a48db PS12, Line 214: "GPP_F04", "CNV_RF_RESET_N", Can you explain, why do we start from F04? In datasheet I can see it starts from F00. https://review.coreboot.org/c/coreboot/+/73934/comment/87c901a3_3b96dfc0 PS12, Line 226: "GPP_F18", "EMMC_RESET_N", Can you explain, why do we stop at F18? In datasheet I can see F19 as well. https://review.coreboot.org/c/coreboot/+/73934/comment/ee7ac0fc_65b4bdac PS12, Line 274: "GPP_H19", "n/a", "n/a", "n/a", Don't we have up to H23 in datasheet? https://review.coreboot.org/c/coreboot/+/73934/comment/3d2c502d_f444aa14 PS12, Line 308: const struct gpio_group jasperlake_pch_group_spi0 = { We have "jasperlake_pch_group_spi_names` but group is called: "jasperlake_pch_group_spi0"... Why? Please be consistent and use "jasperlake_pch_group_spi" https://review.coreboot.org/c/coreboot/+/73934/comment/9b19c998_b026e0fe PS12, Line 458: const struct gpio_group jasperlake_pch_group_sys_pcie = { Again, lack of consistency... if we have "jasperlake_pch_group_pcie_vgpio_names", we should have: "jasperlake_pch_group_pcie_vgpio" https://review.coreboot.org/c/coreboot/+/73934/comment/445fed79_b3726de6 PS12, Line 480: const struct gpio_group jasperlake_pch_group_sys_vusb = { Change to: "jasperlake_pch_group_vgpio_usb" https://review.coreboot.org/c/coreboot/+/73934/comment/61bd3222_8d3e0deb PS12, Line 539: // Deep Sleep Well group Not sure how relevant this comment is. Definitely empty line is missing here. https://review.coreboot.org/c/coreboot/+/73934/comment/17e44080_e320263c PS12, Line 552: Why empty line? https://review.coreboot.org/c/coreboot/+/73934/comment/7c1220e8_9d124d1d PS12, Line 553: "GPD_INPUT3VSEL", "GPD_INPUT3VSEL", You broke indents in this group. https://review.coreboot.org/c/coreboot/+/73934/comment/e8bef4ba_1304e5d7 PS12, Line 584: Sometimes 0, sometimes 1, sometimes 2 empty lines after this comment... This is mess. Please consistent. https://review.coreboot.org/c/coreboot/+/73934/comment/230407fe_afeed922 PS12, Line 657: &jasperlake_pch_group_b, group_a is missing after group_b in this community https://review.coreboot.org/c/coreboot/+/73934/comment/4c33b4b5_e676fc49 PS12, Line 660: empty line? File util/inteltool/gpio_names/jasperlake.h: https://review.coreboot.org/c/coreboot/+/73934/comment/d2e21bab_12d697e6 PS9, Line 140: const char *const jasperlake_pch_community1_sys_names[] = { > For example, ADL-P defines almost the same group as: "alderlake_pch_p_group_hvcmos_names". […] Done https://review.coreboot.org/c/coreboot/+/73934/comment/23f4acec_2037af63 PS9, Line 281: "GSPI2_CLK_LOOPBK", "GSPI2_CLK_LOOPBK", > I still don't see this being changed Again, it is still not changed. It should go to group D, if I see correctly. https://review.coreboot.org/c/coreboot/+/73934/comment/7c6232a7_438940d5 PS9, Line 312: "GSPI0_CLK_LOOPBK", "GSPI0_CLK_LOOPBK", > Done Done https://review.coreboot.org/c/coreboot/+/73934/comment/6fbb3ad1_102aefba PS9, Line 317: .display = "------- GPIO Group GPP_B -------", > Done Done https://review.coreboot.org/c/coreboot/+/73934/comment/cfa332ef_b36ebb8e PS9, Line 359: const char *const jasperlake_pch_group_sys_comm2_names[] = { > Done Done -- To view, visit https://review.coreboot.org/c/coreboot/+/73934 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If4134bd03f5544b5845cde998ee526e5ddd5b51d Gerrit-Change-Number: 73934 Gerrit-PatchSet: 12 Gerrit-Owner: Karol Zmyslowski Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Reviewer: Michal Zygowski <miczyg94(a)gmail.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Attention: Karol Zmyslowski Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Michal Zygowski <miczyg94(a)gmail.com> Gerrit-Comment-Date: Wed, 05 Apr 2023 20:30:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Comment-In-Reply-To: Karol Zmyslowski Gerrit-MessageType: comment

1 0

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

05 Apr '23

Attention is currently required from: Subrata Banik, Jérémy Compostella, Nick Vaccaro. Tarun Tuli 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 2: 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: 2 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-Comment-Date: Wed, 05 Apr 2023 20:27:58 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2023