Attention is currently required from: Jakub Czapiga.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57009 )
Change subject: tests: Fix function mocking for clang
......................................................................
Patch Set 1:
(1 comment)
File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/57009/comment/a1d2de9b_90bab087
PS1, Line 139: printf '#ifdef __TEST_SRCOBJ__\n' >> $$@;
> I found a somehow-working solution. […]
Ooohhh, great find! Thanks!
--
To view, visit https://review.coreboot.org/c/coreboot/+/57009
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1f9011f444248544de7a71bbefc54edc006ae0cd
Gerrit-Change-Number: 57009
Gerrit-PatchSet: 1
Gerrit-Owner: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com>
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-Comment-Date: Fri, 20 Aug 2021 21:12:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-MessageType: comment
Attention is currently required from: Julius Werner.
Hello build bot (Jenkins), Jakub Czapiga,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/57009
to look at the new patch set (#2).
Change subject: tests: Fix function mocking for clang
......................................................................
tests: Fix function mocking for clang
clang seems to like to do some aggressive optimizations that break our
approach of mocking functions for test by using objcopy to turn them
weak after the fact on individual compiled object files. For example, in
CB:56601 the function cbfs_get_boot_device() is mocked this way. When
compiling the cbfs_boot_lookup() function in src/lib/cbfs.c with clang,
it will generate a normal callq instruction to a relocation for
cbfs_boot_lookup(), which can then later be pointed to the mocked
version of that function. However, it will also somehow infer that the
version of cbfs_boot_lookup() in that file can only ever return a
pointer to the static local `ro` variable (because CONFIG_VBOOT is
disabled in the environment for that particular test), and instead
generate instructions that directly load the address of a relocation for
that variable into %rdi for the following call to cbfs_lookup(), rather
than using the real function return value. (Why it would do that is
anyone's guess because this seems unlikely to be faster than just moving
the function return value from %rax into %rdi like a normal compiler.)
Long story short, this optimization breaks our tests because
cbfs_lookup() will be called with the wrong pointer. clang doesn't
provide many options to disable individual optimizations, so the only
solution seems to be to make clang aware that the function is weak
during the compilation stage already, so it can be aware that it may get
replaced. This patch implements that by marking the mocked functions
weak via #pragma weak lines in the per-test autogenerated config header.
Signed-off-by: Julius Werner <jwerner(a)chromium.org>
Change-Id: I1f9011f444248544de7a71bbefc54edc006ae0cd
---
M tests/Makefile.inc
1 file changed, 19 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/57009/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/57009
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1f9011f444248544de7a71bbefc54edc006ae0cd
Gerrit-Change-Number: 57009
Gerrit-PatchSet: 2
Gerrit-Owner: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Sumeet R Pawnikar.
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54724 )
Change subject: mb/google/brya: Add two sensors for DPTF functionality
......................................................................
Patch Set 2:
(1 comment)
File src/mainboard/google/brya/variants/brya0/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/54724/comment/dcc5fa7b_ba2cd1d1
PS2, Line 72: TEMP_SENSOR_0
> As per b:181271666 comment#20, there are total 4 on-board thermal sensors. […]
so, does "CPU" on line 71 map to an integrated SoC temp sensor as a target?
--
To view, visit https://review.coreboot.org/c/coreboot/+/54724
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idc9bd6040c9bb316ec7e314f5e9c937c75cfc95a
Gerrit-Change-Number: 54724
Gerrit-PatchSet: 2
Gerrit-Owner: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
Gerrit-Comment-Date: Fri, 20 Aug 2021 20:47:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Caveh Jalali <caveh(a)chromium.org>
Comment-In-Reply-To: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
Gerrit-MessageType: comment
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 9:
(4 comments)
File tests/include/tests/lib/cbfs_util.h:
https://review.coreboot.org/c/coreboot/+/56601/comment/6d356f74_3ddfcb0d
PS8, Line 74: TEST_REGION(cbfs_cache, TEST_CBFS_CACHE_SIZE);
> It is almost the same as for test data files. […]
I mean the definitions for the file_no_hash, file_valid_hash, etc. globals. You define five TEST_DATA blocks in this header, and the only useful thing one can do with them is define CBFS file structures like that (with various attributes), so might make sense to do that in the common mock file already.
File tests/lib/cbfs-verification-test.c:
https://review.coreboot.org/c/coreboot/+/56601/comment/2473d3c9_3c269c7b
PS2, Line 4: #define __noreturn
> I think so. die() from tests/stubs/die. […]
I mean as long as it works like this without needing weird hacks to redefine the macro, that should be fine.
https://review.coreboot.org/c/coreboot/+/56601/comment/4605193d_38c16442
PS2, Line 181: assert_memory_equal(mapping, &test_data, TEST_DATA_SIZE);
> Done
...so are you changing them to pointer comparisons?
File tests/lib/cbfs-verification-test.c:
https://review.coreboot.org/c/coreboot/+/56601/comment/df9fbd91_3afa832a
PS8, Line 244: return cmocka_run_group_tests(cbfs_verification_tests, NULL, NULL);
> How about adding TEST_NAME define and using it instead? And we could redefine cmocka_run_group_tests […]
Yeah, that should also work
--
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: 9
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: Fri, 20 Aug 2021 20:41:38 +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
Attention is currently required from: Shelley Chen.
Douglas Anderson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57065 )
Change subject: WIP: Herobrine: Initialize Trackpad as MIXED mode
......................................................................
Patch Set 1:
(1 comment)
File src/mainboard/google/herobrine/mainboard.c:
https://review.coreboot.org/c/coreboot/+/57065/comment/de3a8956_fe835517
PS1, Line 65: Touch
trackpad to distinguish it from "touch" below.
--
To view, visit https://review.coreboot.org/c/coreboot/+/57065
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib3a406408d049e1b30db0297dc9843024c316a0c
Gerrit-Change-Number: 57065
Gerrit-PatchSet: 1
Gerrit-Owner: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Douglas Anderson <dianders(a)chromium.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Comment-Date: Fri, 20 Aug 2021 20:06:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Shelley Chen.
Douglas Anderson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57057 )
Change subject: WIP: Herobrine: Initialize Pen and Audio i2c devices
......................................................................
Patch Set 2:
(4 comments)
File src/mainboard/google/herobrine/mainboard.c:
https://review.coreboot.org/c/coreboot/+/57057/comment/6ba182ae_a4b3b121
PS2, Line 74: qupv3_se_fw_load_and_init(QUPV3_1_SE4, SE_PROTOCOL_SPI, MIXED); /* ESIM SPI */
Drop this line. The eSIM was connected to SPI in a really early version of trogdor but (as far as I know) isn't now.
QUPV3_1_SE4 = 8 + 4 = 12. ...and QUP12 is actually supposed to be configured for i2c for talking to our TPM. This might be the source of some of our TPM problems that we were blaming on blobs?
https://review.coreboot.org/c/coreboot/+/57057/comment/ab2a163d_ee6b7a04
PS2, Line 76: QUPV3_1_SE6
1_SE6 = 8 + 6 = 14. spi14 pins are "gpio56", "gpio57", "gpio58", "gpio59". Schematic shows FP SPI as 44 - 47. That's spi11. 11 - 8 = 3 so this should be QUPV3_1_SE3.
https://review.coreboot.org/c/coreboot/+/57057/comment/4d9da488_63dd6177
PS2, Line 78: i2c_init(QUPV3_0_SE0, I2C_SPEED_FAST); /* Audio - QUP 0 */
Can you re-order? Seems like things are mostly sorted by QUP so this should be at the beginning?
Also, why this instead of `qupv3_se_fw_load_and_init()` like everything else in this file?
https://review.coreboot.org/c/coreboot/+/57057/comment/2b572cdf_4379a640
PS2, Line 79: i2c_init(QUPV3_1_SE7, I2C_SPEED_FAST); /* PEN - QUP 15 */
Let's drop this line. I was wrong, it's actually 13 and handled above as "Touch I2C"
--
To view, visit https://review.coreboot.org/c/coreboot/+/57057
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic6f9dfd2ed79e094b37d004106003fedff8d1d45
Gerrit-Change-Number: 57057
Gerrit-PatchSet: 2
Gerrit-Owner: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Douglas Anderson <dianders(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Comment-Date: Fri, 20 Aug 2021 20:05:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Raul Rangel, Martin Roth, Furquan Shaikh, Marshall Dawson, Karthik Ramasubramanian.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56885 )
Change subject: mb/(amd,google): Update SPI Kconfig settings based on devicetree
......................................................................
Patch Set 5:
(1 comment)
File src/mainboard/google/guybrush/Kconfig:
https://review.coreboot.org/c/coreboot/+/56885/comment/fd26c700_d32a2a59
PS5, Line 110: config TPM_SPI_SPEED
: default 0 # 66MHz
:
> All 4 different SPI speeds are configured using a single register - each 4 bits. […]
when the chipset's Kconfig provides a default that works for all boards, we don't need to override the setting here when we don't use this feature
--
To view, visit https://review.coreboot.org/c/coreboot/+/56885
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Icce1d57761465ae8255e5d9ce8679f3fdcb0ceed
Gerrit-Change-Number: 56885
Gerrit-PatchSet: 5
Gerrit-Owner: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Fri, 20 Aug 2021 20:01:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org>
Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Raul Rangel, Martin Roth, Furquan Shaikh, Marshall Dawson, Karthik Ramasubramanian.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56884 )
Change subject: soc/amd/common/block/spi: Add SPI config to Kconfig
......................................................................
Patch Set 3:
(2 comments)
File src/soc/amd/common/block/spi/Kconfig:
https://review.coreboot.org/c/coreboot/+/56884/comment/2caacaa4_b5abea06
PS3, Line 20: 0
> HOLD line alone needs to be floating. Otherwise I believe it should support Dual mode.
i'd use some of the normal single bit spi modes here, since that's what will work in all cases. the boards can select a wider interface if the flash chip can support that on those
https://review.coreboot.org/c/coreboot/+/56884/comment/77f37eb0_b3eb27da
PS3, Line 40: 0
i'd use 3 as default for this and the other speeds as well, since 16 MHz will be working in all configurations and if a mainboard can support faster speeds, it can override the Kconfig option. 66MHz won't work on all possible mainboards and i'd like to have working defaults in the chipset's Kconfig
--
To view, visit https://review.coreboot.org/c/coreboot/+/56884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8f29e49e886bd99b39172905e21bfd392c6c10e2
Gerrit-Change-Number: 56884
Gerrit-PatchSet: 3
Gerrit-Owner: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Fri, 20 Aug 2021 19:59:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org>
Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: comment