coreboot-gerrit August 2021

coreboot-gerrit@coreboot.org

1 participants
4519 discussions

Change in coreboot[master]: MAINTAINERS: Add myself as a maintainer for System76
by Paul Menzel (Code Review) 07 Aug '21

07 Aug '21

Attention is currently required from: Tim Crawford. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52580 ) Change subject: MAINTAINERS: Add myself as a maintainer for System76 ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/52580/comment/491ff870_7992641e PS2, Line 7: myself Please use your name for people reading only the summary. -- To view, visit https://review.coreboot.org/c/coreboot/+/52580 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8aa9ee1627bf319660b193b4602d8c2d0b562424 Gerrit-Change-Number: 52580 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Crawford <tcrawford(a)system76.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Crawford <tcrawford(a)system76.com> Gerrit-Comment-Date: Sat, 07 Aug 2021 08:47:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: crossgcc: upgrade Expat to version 2.4.1
by linear (Code Review) 07 Aug '21

07 Aug '21

linear has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/56868 ) Change subject: crossgcc: upgrade Expat to version 2.4.1 ...................................................................... crossgcc: upgrade Expat to version 2.4.1 versions of expat before 2.4.0 have been renamed to prevent their use, due to some kind of vulnerability. without updating this dependency it is currently not possible to build crossgcc. Change-Id: Iec2cf560902dc556a41206d7dcd65c22cf3e1215 Signed-off-by: Mackenzie May <ky0ko(a)disroot.org> --- M util/crossgcc/buildgcc D util/crossgcc/sum/expat-2.2.9.tar.bz2.cksum A util/crossgcc/sum/expat-2.4.1.tar.bz2.cksum 3 files changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/56868/1 diff --git a/util/crossgcc/buildgcc b/util/crossgcc/buildgcc index aee55f9..7176e6b 100755 --- a/util/crossgcc/buildgcc +++ b/util/crossgcc/buildgcc @@ -41,7 +41,7 @@ GDB_VERSION=9.2 IASL_VERSION=20210331 PYTHON_VERSION=3.8.5 -EXPAT_VERSION=2.2.9 +EXPAT_VERSION=2.4.1 # CLANG version number CLANG_VERSION=12.0.0 CMAKE_VERSION=3.20.3 diff --git a/util/crossgcc/sum/expat-2.2.9.tar.bz2.cksum b/util/crossgcc/sum/expat-2.2.9.tar.bz2.cksum deleted file mode 100644 index 924b412..0000000 --- a/util/crossgcc/sum/expat-2.2.9.tar.bz2.cksum +++ /dev/null @@ -1 +0,0 @@ -ef5c1c55913a6ab18496ee99166f86269c7cdc31 tarballs/expat-2.2.9.tar.bz2 diff --git a/util/crossgcc/sum/expat-2.4.1.tar.bz2.cksum b/util/crossgcc/sum/expat-2.4.1.tar.bz2.cksum new file mode 100644 index 0000000..5b520f3 --- /dev/null +++ b/util/crossgcc/sum/expat-2.4.1.tar.bz2.cksum @@ -0,0 +1 @@ +b677b9a1cf3a1424fda183223fae2c58f50151af tarballs/expat-2.4.1.tar.bz2 -- To view, visit https://review.coreboot.org/c/coreboot/+/56868 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iec2cf560902dc556a41206d7dcd65c22cf3e1215 Gerrit-Change-Number: 56868 Gerrit-PatchSet: 1 Gerrit-Owner: linear <ky0ko(a)disroot.org> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: soc/intel/tgl: Allow setting PCIe subsystem IDs after FSP-S
by build bot (Jenkins) (Code Review) 07 Aug '21

07 Aug '21

Attention is currently required from: Tim Wawrzynczak, Patrick Rudolph. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56867 ) Change subject: soc/intel/tgl: Allow setting PCIe subsystem IDs after FSP-S ...................................................................... Patch Set 2: (2 comments) File src/soc/intel/tigerlake/fsp_params.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125904): https://review.coreboot.org/c/coreboot/+/56867/comment/df7f5e24_e9ad153c PS2, Line 640: uint64_t :4; space prohibited before that ':' (ctx:WxV) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125904): https://review.coreboot.org/c/coreboot/+/56867/comment/d57b1e7e_0ed3695b PS2, Line 642: uint64_t :16; space prohibited before that ':' (ctx:WxV) -- To view, visit https://review.coreboot.org/c/coreboot/+/56867 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I391b9fd0dc9dda925c1c8fe52bff153fe044d73e Gerrit-Change-Number: 56867 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Crawford <tcrawford(a)system76.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: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Sat, 07 Aug 2021 07:07:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/tgl: Allow setting PCIe subsystem IDs after FSP-S
by Tim Crawford (Code Review) 07 Aug '21

07 Aug '21

Attention is currently required from: Tim Crawford, Tim Wawrzynczak, Patrick Rudolph. Hello build bot (Jenkins), Tim Wawrzynczak, Patrick Rudolph, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/56867 to look at the new patch set (#2). Change subject: soc/intel/tgl: Allow setting PCIe subsystem IDs after FSP-S ...................................................................... soc/intel/tgl: Allow setting PCIe subsystem IDs after FSP-S Prevent the FSP from writing its default SVID SDID values of 8086:7270 for internal devices as this locks most of the registers. Allows the subsystemid values set in devicetree to be used. A description of this SSID table override behavior, along with example code, is provided in the TigerLake FSP Integration Guide, section 15.178 ("SI_CONFIG Struct Reference"). The xHCI and HDA devices have RW/L registers rather than RW/O registers. They can be written to multiple times but cannot be modified after being locked, which happens during FspSiliconInit. Because coreboot populates subsystem IDs after SiliconInit, these devices specifically must be written beforehand or will otherwise be locked with their default values of 0:0. TGL also introduces parameters for customizing the default SVID:SSID. These must be set or it will still use the FSP defaults. Tested by checking lspci output on System76 darp7 (TGL-U). References: - b1fa231d76a ("soc/intel/cnl: Allow setting PCIe subsystem IDs after FSP-S") - TigerLake FSP Integration Guide - Intel Document #631120-001 Change-Id: I391b9fd0dc9dda925c1c8fe52bff153fe044d73e Signed-off-by: Tim Crawford <tcrawford(a)system76.com> --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 55 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/56867/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/56867 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I391b9fd0dc9dda925c1c8fe52bff153fe044d73e Gerrit-Change-Number: 56867 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Crawford <tcrawford(a)system76.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: Tim Crawford <tcrawford(a)system76.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/tgl: Allow setting PCIe subsystem IDs after FSP-S
by build bot (Jenkins) (Code Review) 07 Aug '21

07 Aug '21

Attention is currently required from: Tim Wawrzynczak, Patrick Rudolph. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56867 ) Change subject: soc/intel/tgl: Allow setting PCIe subsystem IDs after FSP-S ...................................................................... Patch Set 1: (2 comments) File src/soc/intel/tigerlake/fsp_params.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125903): https://review.coreboot.org/c/coreboot/+/56867/comment/985da1fa_7e555b94 PS1, Line 640: uint64_t :4; space prohibited before that ':' (ctx:WxV) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125903): https://review.coreboot.org/c/coreboot/+/56867/comment/c01c7548_83fc3dd2 PS1, Line 642: uint64_t :16; space prohibited before that ':' (ctx:WxV) -- To view, visit https://review.coreboot.org/c/coreboot/+/56867 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I391b9fd0dc9dda925c1c8fe52bff153fe044d73e Gerrit-Change-Number: 56867 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Crawford <tcrawford(a)system76.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Sat, 07 Aug 2021 06:50:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/tgl: Allow setting PCIe subsystem IDs after FSP-S
by Tim Crawford (Code Review) 07 Aug '21

07 Aug '21

Tim Crawford has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/56867 ) Change subject: soc/intel/tgl: Allow setting PCIe subsystem IDs after FSP-S ...................................................................... soc/intel/tgl: Allow setting PCIe subsystem IDs after FSP-S Prevent the FSP from writing its default SVID SDID values of 8086:7270 for internal devices as this locks most of the registers. Allows the subsystemid values set in devicetree to be used. A description of this SSID table override behavior, along with example code, is provided in the TigerLake FSP Integration Guide, section 15.178 ("SI_CONFIG Struct Reference"). The xHCI and HDA devices have RW/L registers rather than RW/O registers. They can be written to multiple times but cannot be modified after being locked, which happens during FspSiliconInit. Because coreboot populates subsystem IDs after SiliconInit, these devices specifically must be written beforehand or will otherwise be locked with their default values of 0:0. TGL also introduces parameters for customizing the default SVID:SSID. These must be set or it will still use the FSP defaults. Tested by checking lspci output on System76 darp7 (TGL-U). References: - b1fa231d76a ("soc/intel/cnl: Allow setting PCIe subsystem IDs after FSP-S") - TigerLake FSP Integration Guide - Intel Document #631120-001 Change-Id: I391b9fd0dc9dda925c1c8fe52bff153fe044d73e Signed-off-by: Tim Crawford <tcrawford(a)system76.com> --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 55 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/56867/1 diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c index 010ff02..0de6dfc 100644 --- a/src/soc/intel/tigerlake/fsp_params.c +++ b/src/soc/intel/tigerlake/fsp_params.c @@ -625,6 +625,61 @@ /* USB2 Phy Sus power gating setting override */ params->PmcUsb2PhySusPgEnable = !config->usb2_phy_sus_pg_disable; + /* + * Prevent FSP from programming write-once subsystem IDs by providing + * a custom SSID table. Must have at least one entry for the FSP to + * use the table. + */ + struct svid_ssid_init_entry { + union { + struct { + uint64_t reg:12; /* Register offset */ + uint64_t function:3; + uint64_t device:5; + uint64_t bus:8; + uint64_t :4; + uint64_t segment:16; + uint64_t :16; + }; + uint64_t segbusdevfuncregister; + }; + struct { + uint16_t svid; + uint16_t ssid; + }; + uint32_t reserved; + }; + + /* + * The xHCI and HDA devices have RW/L rather than RW/O registers for + * subsystem IDs and so must be written before FspSiliconInit locks + * them with their default values. + */ + const pci_devfn_t devfn_table[] = { PCH_DEVFN_XHCI, PCH_DEVFN_HDA }; + static struct svid_ssid_init_entry ssid_table[ARRAY_SIZE(devfn_table)]; + + for (i = 0; i < ARRAY_SIZE(devfn_table); i++) { + ssid_table[i].reg = PCI_SUBSYSTEM_VENDOR_ID; + ssid_table[i].device = PCI_SLOT(devfn_table[i]); + ssid_table[i].function = PCI_FUNC(devfn_table[i]); + dev = pcidev_path_on_root(devfn_table[i]); + if (dev) { + ssid_table[i].svid = dev->subsystem_vendor; + ssid_table[i].ssid = dev->subsystem_device; + } + } + + params->SiSsidTablePtr = (uintptr_t)ssid_table; + params->SiNumberOfSsidTableEntry = ARRAY_SIZE(ssid_table); + + /* + * Replace the default SVID:SSID value with the values specified in + * the devicetree for the root device. + */ + dev = pcidev_path_on_root(SA_DEVFN_ROOT); + params->SiCustomizedSvid = dev->subsystem_vendor; + params->SiCustomizedSsid = dev->subsystem_device + mainboard_silicon_init_params(params); } -- To view, visit https://review.coreboot.org/c/coreboot/+/56867 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I391b9fd0dc9dda925c1c8fe52bff153fe044d73e Gerrit-Change-Number: 56867 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Crawford <tcrawford(a)system76.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: tests: Add lib/cbfs-verification-test test case
by Julius Werner (Code Review) 06 Aug '21

06 Aug '21

Attention is currently required from: Jakub Czapiga, Jan Dabros. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56601 ) Change subject: tests: Add lib/cbfs-verification-test test case ...................................................................... Patch Set 7: (13 comments) File tests/lib/Makefile.inc: https://review.coreboot.org/c/coreboot/+/56601/comment/6249186e_387fefce PS2, Line 199: CONFIG_NO_CBFS_MCACHE=0 \ > I disabled MCache for this test. It will be tested in another one. […] Well I think file verification and metadata verification are certainly different enough things that you could have separate tests for them if you want (especially because they would likely need different mock functions and/or data). And if you do something like that more integrated overall verification test I described above, that would probably also need to be its own test file. I don't think you need to put everything "verification" into a single file -- rather, you probably want to put the things that can use the same mocks into the same file, and have stuff that needs a very different mock setup in another file. File tests/lib/Makefile.inc: https://review.coreboot.org/c/coreboot/+/56601/comment/a970dfb1_d37fc54a PS7, Line 35: tests-y += cbfs-verification-no-sha512-test Should explain the rationale for testing both cases in a comment somewhere. https://review.coreboot.org/c/coreboot/+/56601/comment/fce9d39a_6a24443e PS7, Line 188: cbfs-verification-sources := tests/lib/cbfs-verification-test.c \ I wonder if it makes sense to define a copy_test function for this sort of stuff, so that you could just do $(call copy_test,cbfs-verification-no-sha512,cbfs-verification-has-sha512) cbfs-verification-has-sha512-config += VB2_SUPPORT_SHA512=1 to allow writing these variations with less duplication. https://review.coreboot.org/c/coreboot/+/56601/comment/b81c125d_47b90ab1 PS7, Line 198: 3rdparty/vboot/firmware/include Shouldn't we just put these into the default include path for tests? They are in the default include path for normal coreboot, and I think tests should just use the same. https://review.coreboot.org/c/coreboot/+/56601/comment/110d662f_d29516cb PS7, Line 219: CONFIG_CBFS_VERIFICATION=1 Wasn't the point that this should be 0 here? File tests/lib/cbfs-verification-test.c: https://review.coreboot.org/c/coreboot/+/56601/comment/27433d5c_d491794a PS2, Line 167: assert_null(mapping); > Done, I think. […] Ack https://review.coreboot.org/c/coreboot/+/56601/comment/6a9f34f0_1c407298 PS2, Line 181: assert_memory_equal(mapping, &test_data, TEST_DATA_SIZE); > Assuming we do not use cache, we can. […] That's not how this works -- CBFS cache is used by the specific rdev's mmap() implementation, not by the core CBFS framework (at least not for this). If you're providing a mem_rdev it will just return a pointer for mmap(), not use any cache. https://review.coreboot.org/c/coreboot/+/56601/comment/96746d5b_8a505712 PS2, Line 244: NULL > I have seen cbfs_init_boot_device(cbd, NULL) in ./src/security/vboot/vboot_loader.c, so I tought tat it would be nice to include it in the test as well. Yeah that's a stopgap, that's why it says // TODO. It basically means CBFS_VERIFICATION is force-disabled for RW CBFSes for now. Once we implement that part, it will pass a valid hash there. If you want to have a test for cbfs_walk() where all the vb2_digest...() stuff is mocked out you can certainly do that. But I would at some point definitely like to have a test that tests the whole verification system with real hashes and real hash functions. Call it a higher level "integration test" if you want (but I think it can use this same test framework), it's just good to test that all these things can work together to fulfill their intended purpose and don't just work in isolation (when you mock too many things, there's always a danger that you mocked them wrong and they don't represent the real case). I think I'd consider that test more important than a completely bare-bones, fully mocked-out cbfs_walk() test, but I certainly wouldn't mind having both. File tests/lib/cbfs-verification-test.c: https://review.coreboot.org/c/coreboot/+/56601/comment/ec7c8b23_61dd7ba2 PS7, Line 147: { Actually, I think it would be better to properly mock this too (comparing buf and size to expected values). Otherwise you're not really testing much verification, you're just kinda pretending that you always get the right result. https://review.coreboot.org/c/coreboot/+/56601/comment/cf7f0c44_54c1bdaa PS7, Line 216: &file_valid_hash.attrs_and_data[HASH_ATTR_SIZE], HASH_ATTR_SIZE); Uhhh... wait a second... does this test succeed? Because it shouldn't succeed. When CONFIG_CBFS_VERIFICATION=0, vb2_hash_verify() should never be called. Does cmocka not check at the end of every test whether there were pending expect()s that didn't get hit (I thought it did)? https://review.coreboot.org/c/coreboot/+/56601/comment/d6060f4c_72fe1e35 PS7, Line 243: assert_null(mapping); And this should return a valid mapping because verification is disabled. I guess you just never ran this because of that typo in the Makefile? https://review.coreboot.org/c/coreboot/+/56601/comment/a4e74919_27dace2e PS7, Line 283: /* No cache, so no validation required */ (nit: comment still makes no sense here) https://review.coreboot.org/c/coreboot/+/56601/comment/9e238193_659e5500 PS7, Line 288: has_mcache Well, you can either define NO_CBFS_MCACHE, or you can test mcache stuff here. Not both. -- To view, visit https://review.coreboot.org/c/coreboot/+/56601 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1d8cbb1c2d0a9db3236de065428b70a9c2a66330 Gerrit-Change-Number: 56601 Gerrit-PatchSet: 7 Gerrit-Owner: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Jan Dabros <jsd(a)semihalf.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Attention: Jan Dabros <jsd(a)semihalf.com> Gerrit-Comment-Date: Sat, 07 Aug 2021 01:22:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jakub Czapiga <jacz(a)semihalf.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/guybrush: Switch from 33MHz to 66MHz SPI Speed
by Furquan Shaikh (Code Review) 06 Aug '21

06 Aug '21

Attention is currently required from: Raul Rangel, Karthik Ramasubramanian, Felix Held. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56645 ) Change subject: mb/google/guybrush: Switch from 33MHz to 66MHz SPI Speed ...................................................................... Patch Set 3: (1 comment) File src/mainboard/google/guybrush/variants/baseboard/devicetree.cb: https://review.coreboot.org/c/coreboot/+/56645/comment/e418ebcc_cd6bec91 PS3, Line 13: 33M > yes, i was referring to the 8 dummy cycles for the fast read command that gives the flash more time […] Makes sense. Thanks Felix! I think most parts support upto 50MHz for normal speed. It would be good to understand what operations fall under altio to ensure the speed is set correctly for that as well. -- To view, visit https://review.coreboot.org/c/coreboot/+/56645 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc5c0580ed0d19f1fffce59df3888dd7963255a1 Gerrit-Change-Number: 56645 Gerrit-PatchSet: 3 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Sat, 07 Aug 2021 01:08:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd/common/block/spi: Don't update spi speed if EFS is changed
by Furquan Shaikh (Code Review) 06 Aug '21

06 Aug '21

Attention is currently required from: Martin Roth, Karthik Ramasubramanian, Felix Held. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56818 ) Change subject: soc/amd/common/block/spi: Don't update spi speed if EFS is changed ...................................................................... Patch Set 2: (1 comment) File src/soc/amd/common/block/spi/fch_spi.c: https://review.coreboot.org/c/coreboot/+/56818/comment/06ad04b4_528805da PS2, Line 84: if (CONFIG(EM100)) : fch_spi_config_em100_modes(); : else : fch_spi_config_mb_modes(); > the mainboard's Kconfig options have separate cases for the EM100 mode Separate cases yes. But they set the same Kconfig option. Basically, we need options for READ_MODE, FAST_SPI_SPEED, TPM_SPEED, NORMAL_SPEED and ALTIO_SPEED. -- To view, visit https://review.coreboot.org/c/coreboot/+/56818 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I278768e361499919666d07e1dd92d83c2390035e Gerrit-Change-Number: 56818 Gerrit-PatchSet: 2 Gerrit-Owner: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Furquan Shaikh <furquan(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Sat, 07 Aug 2021 01:04:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Comment-In-Reply-To: Martin Roth <martinroth(a)google.com> Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: elogtool: add "clear" command
by build bot (Jenkins) (Code Review) 06 Aug '21

06 Aug '21

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56866 ) Change subject: elogtool: add "clear" command ...................................................................... Patch Set 1: (15 comments) File src/commonlib/bsd/elog.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/87a5bd6f_41ffeef3 PS1, Line 67: if (event->month > 0x12 || event->day > 0x31 || event->hour > 0x23 || that open brace { should be on the previous line Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/807416a9_a04614ee PS1, Line 99: } adding a line without newline at end of file File util/cbfstool/elogtool.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/938f03f8_1d7b6c93 PS1, Line 120: static int elog_clear_events(struct buffer* buf) "foo* bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/f5ee200b_b0f10ad2 PS1, Line 124: void* data_offset; "foo* bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/daafe519_07efc9ea PS1, Line 135: * We calcualte the size of the "used" buffer, needed for ELOG_TYPE_LOG_CLEAR. 'calcualte' may be misspelled - perhaps 'calculate'? Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/d4de4862_f609fd3d PS1, Line 141: while ((const char*)(event) < buf->data + buf->size) { "(foo*)" should be "(foo *)" File util/cbfstool/eventlog.h: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/f4470a62_92a9d606 PS1, Line 11: void eventlog_init_event(struct event_header* event, uint8_t type, const void* data, int data_size); line over 96 characters Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/1c49376c_cb40e3c4 PS1, Line 11: void eventlog_init_event(struct event_header* event, uint8_t type, const void* data, int data_size); "foo* bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/6835e0c5_8bb85ac0 PS1, Line 11: void eventlog_init_event(struct event_header* event, uint8_t type, const void* data, int data_size); "foo* bar" should be "foo *bar" File util/cbfstool/eventlog.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/ee9ceac4_7a7c40b2 PS1, Line 638: void eventlog_init_event(struct event_header* event, uint8_t type, const void* data, int data_size) line over 96 characters Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/c0012e6f_34718503 PS1, Line 638: void eventlog_init_event(struct event_header* event, uint8_t type, const void* data, int data_size) "foo* bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/f3a7f88d_22b3f15c PS1, Line 638: void eventlog_init_event(struct event_header* event, uint8_t type, const void* data, int data_size) "foo* bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/a2a567a0_63932956 PS1, Line 645: elog_fill_timestamp(event, tm.tm_sec, tm.tm_min, tm.tm_hour, tm.tm_mday, tm.tm_mon, tm.tm_year); line over 96 characters Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/6e854695_30e24648 PS1, Line 648: void* ptr = (uint32_t*) &event[1]; "(foo*)" should be "(foo *)" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-125890): https://review.coreboot.org/c/coreboot/+/56866/comment/ce275707_b410b83b PS1, Line 648: void* ptr = (uint32_t*) &event[1]; "foo* bar" should be "foo *bar" -- To view, visit https://review.coreboot.org/c/coreboot/+/56866 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7ea64ea748ee4c3647b0256be51203464a54c2dd Gerrit-Change-Number: 56866 Gerrit-PatchSet: 1 Gerrit-Owner: Ricardo Quesada <ricardoq(a)google.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 07 Aug 2021 00:59:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit August 2021