Attention is currently required from: Paul Menzel, Nick Vaccaro, Scott Chao.
Hello build bot (Jenkins), Tim Wawrzynczak, Nick Vaccaro,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/60270
to look at the new patch set (#4).
Change subject: mb/google/brya/var/gimble: Decrease touchscreen T3 timing to 200ms
......................................................................
mb/google/brya/var/gimble: Decrease touchscreen T3 timing to 200ms
We set T3 as 300ms to meet Elan's spec, but the resume/suspend times
are greater than 500ms, which is the spec for Chromebooks.
The actual kernel timing has been measured, and given the ACPI delay
after deasserting reset in addition to the delay until the kernel
driver accesses the device, delaying only 200ms in the ACPI method is
also sufficient to meet the 300ms requirement.
BUG=b:210772498
BRANCH=none
TEST=build and test touchscreen function on DUT.
TEST=suspend, wake DUT and check touchscreen function.
Signed-off-by: Scott Chao <scott_chao(a)wistron.corp-partner.google.com>
Change-Id: I4bb4eda09686cb59b6e19c741aa2b78d84332d2a
---
M src/mainboard/google/brya/variants/gimble/overridetree.cb
M src/mainboard/google/brya/variants/gimble4es/overridetree.cb
2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/60270/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/60270
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4bb4eda09686cb59b6e19c741aa2b78d84332d2a
Gerrit-Change-Number: 60270
Gerrit-PatchSet: 4
Gerrit-Owner: Scott Chao <scott_chao(a)wistron.corp-partner.google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anfernee Chen <anfernee_chen(a)wistron.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Scott Chao <scott_chao(a)wistron.corp-partner.google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Hsuan-ting Chen, Edward O'Callaghan, Nikolai Artemiev.
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59714 )
Change subject: util/cbfstool: Port elogtool to libflashrom
......................................................................
Patch Set 10:
(6 comments)
File util/cbfstool/uflashrom.c:
https://review.coreboot.org/c/coreboot/+/59714/comment/63fa3005_251ee898
PS10, Line 25: if (level != FLASHROM_MSG_SPEW)
This condition will never be false given the early return above. I'm not sure the fflush() is needed given only stderr is used.
https://review.coreboot.org/c/coreboot/+/59714/comment/6951a3fc_129f471b
PS10, Line 38: size_t new_size = size - offset;
I suspect not finding the end of the region will confuse the parts of elogtool attempting to manage the elog size.
https://review.coreboot.org/c/coreboot/+/59714/comment/17703a29_382843f5
PS10, Line 73: /* empty region causes seg fault in API. */
This comment seems a bit lost. Perhaps hoist it to the if (region) above, or remove it.
https://review.coreboot.org/c/coreboot/+/59714/comment/af43ac37_217fb141
PS10, Line 90: /* FIXME: The flashrom_image_read() API starts the write into the buffer
This seems like a slightly strange place to opine about the libflashrom API. That said, this expectation doesn't scale nicely to more than one region - at least, not by default - a "packed" option might be reasonable to request such treatment.
In my view, the critical problem is that the offset and length of a region cannot be retrieved; a packed option (with corresponding offset query adjustments) would be more of a nice convenience.
https://review.coreboot.org/c/coreboot/+/59714/comment/2883f50b_e4ff12c0
PS10, Line 145: r |= flashrom_layout_include_region(layout, region);
This has the inverse problem of the read path: libflashrom expects the region in the buffer to be at its offset in the layout, but the buffer provided by elog_write() is just the contents of that region.
I guess the flashrom_layout_read_fmap_from_buffer() call above could never be expected to succeed unless the region happens to be the FMAP region.
https://review.coreboot.org/c/coreboot/+/59714/comment/de3d00e7_f2e7f19a
PS10, Line 154: flashrom_flag_set(flashctx, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, true);
I think this would be false on the old implementation.
--
To view, visit https://review.coreboot.org/c/coreboot/+/59714
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I79df2934b9b0492a554a4fecdd533a0abe1df231
Gerrit-Change-Number: 59714
Gerrit-PatchSet: 10
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: Jack Rosenthal <jrosenth(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Ricardo Quesada <ricardoq(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Thu, 06 Jan 2022 01:47:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Shelley Chen, Angel Pons, Julius Werner.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60795 )
Change subject: mb/google/herobrine: Ensure board ID GPIOs are set
......................................................................
Patch Set 1:
(1 comment)
File src/mainboard/google/herobrine/boardid.c:
https://review.coreboot.org/c/coreboot/+/60795/comment/214213b5_a913902a
PS1, Line 25: dead_code();
> Not related to this patch, but does QC SoC have extra ADCs? We've been using them for board IDs on M […]
Ack
--
To view, visit https://review.coreboot.org/c/coreboot/+/60795
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I863cd7acc31442333d8f73d1bd3a77f5c9978020
Gerrit-Change-Number: 60795
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Thu, 06 Jan 2022 01:44:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Hung-Te Lin <hungte(a)chromium.org>
Comment-In-Reply-To: Shelley Chen <shchen(a)google.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Shelley Chen, Angel Pons, Julius Werner.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60795 )
Change subject: mb/google/herobrine: Ensure board ID GPIOs are set
......................................................................
Patch Set 1:
(1 comment)
File src/mainboard/google/herobrine/boardid.c:
https://review.coreboot.org/c/coreboot/+/60795/comment/404a4742_ad047267
PS1, Line 25: dead_code();
> Sorry about that. […]
Not related to this patch, but does QC SoC have extra ADCs? We've been using them for board IDs on MTK platforms for a long time (before we moved to CBI) and never had to move the GPIOs between revisions and models...
--
To view, visit https://review.coreboot.org/c/coreboot/+/60795
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I863cd7acc31442333d8f73d1bd3a77f5c9978020
Gerrit-Change-Number: 60795
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Thu, 06 Jan 2022 01:43:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Shelley Chen <shchen(a)google.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Hsuan-ting Chen, Edward O'Callaghan, Nikolai Artemiev.
Ricardo Quesada has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59714 )
Change subject: util/cbfstool: Port elogtool to libflashrom
......................................................................
Patch Set 10: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/59714
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I79df2934b9b0492a554a4fecdd533a0abe1df231
Gerrit-Change-Number: 59714
Gerrit-PatchSet: 10
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: Jack Rosenthal <jrosenth(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Ricardo Quesada <ricardoq(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Thu, 06 Jan 2022 01:41:27 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Hsuan-ting Chen, Edward O'Callaghan, Nikolai Artemiev.
Ricardo Quesada has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59714 )
Change subject: util/cbfstool: Port elogtool to libflashrom
......................................................................
Patch Set 10:
(2 comments)
File util/cbfstool/uflashrom.c:
https://review.coreboot.org/c/coreboot/+/59714/comment/fc5b05d8_94477656
PS10, Line 108: flashrom_write
nit: I'd add comment for flashrom_write() and flashrom_read() describing what they do and the return values. In particular whether it returns positive or negative values as error.
(see comment below)
https://review.coreboot.org/c/coreboot/+/59714/comment/e20a13a4_0297dbd3
PS10, Line 160: r |=
nit: is this needed ?
context:
flashrom_programmer_shutdown always return 0.
But it seems that libflashrom() uses positives values for errors (e.g: flashrom_programmer_init).
so, if in the future flashrom_programmer_shutdown() gets updated and returns an error, it will be a positive value.
But at this point r could be -1.
So, most probably the `r |= ` in `r |= flashrom_programmer_shutdown` is not needed (?)
--
To view, visit https://review.coreboot.org/c/coreboot/+/59714
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I79df2934b9b0492a554a4fecdd533a0abe1df231
Gerrit-Change-Number: 59714
Gerrit-PatchSet: 10
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: Jack Rosenthal <jrosenth(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Ricardo Quesada <ricardoq(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Thu, 06 Jan 2022 01:40:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Ravi kumar, mturney mturney, Julius Werner, Rajesh Patil.
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50581 )
Change subject: mb/google/herobrine: Initialize EC and TPM devices
......................................................................
Patch Set 97:
(1 comment)
File src/mainboard/google/herobrine/chromeos.c:
https://review.coreboot.org/c/coreboot/+/50581/comment/96d4327d_f303d73f
PS97, Line 31: #if CONFIG(EC_GOOGLE_CHROMEEC)
I couldn't figure out a better way to do as variable length arrays are not allowd in C. It seems like in trogdor we just define the EC and TPM gpios for bubs even though they are not used, but that doesn't seem correct. Would really appreciate it if others have any suggestions.
--
To view, visit https://review.coreboot.org/c/coreboot/+/50581
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8cbdd1d59a0166688d52d61646db1b6764879a7c
Gerrit-Change-Number: 50581
Gerrit-PatchSet: 97
Gerrit-Owner: Ravi kumar <rbokka(a)codeaurora.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: mturney mturney <mturney(a)codeaurora.org>
Gerrit-CC: Mukesh Savaliya <msavaliy(a)qualcomm.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Rajesh Patil <rajpat(a)qualcomm.corp-partner.google.com>
Gerrit-CC: Ravi Kumar Bokka <c_rbokka(a)qualcomm.corp-partner.google.com>
Gerrit-CC: Sudheer Amrabadi <samrabad(a)codeaurora.org>
Gerrit-Attention: Ravi kumar <rbokka(a)codeaurora.org>
Gerrit-Attention: mturney mturney <mturney(a)codeaurora.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Rajesh Patil <rajpat(a)qualcomm.corp-partner.google.com>
Gerrit-Comment-Date: Thu, 06 Jan 2022 01:36:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Shelley Chen, Ravi kumar, mturney mturney, Julius Werner, Rajesh Patil.
Shelley Chen has uploaded a new patch set (#97) to the change originally created by Ravi kumar. ( https://review.coreboot.org/c/coreboot/+/50581 )
Change subject: mb/google/herobrine: Initialize EC and TPM devices
......................................................................
mb/google/herobrine: Initialize EC and TPM devices
Initialize EC and H1/TPM instances on herobrine devices.
BUG=b:182963902
BRANCH=None
TEST=Validated on qualcomm sc7280 development board
and verified booting on herobrine.
Change-Id: I8cbdd1d59a0166688d52d61646db1b6764879a7c
Signed-off-by: Roja Rani Yarubandi <rojay(a)codeaurora.org>
---
M src/mainboard/google/herobrine/Kconfig
M src/mainboard/google/herobrine/board.h
M src/mainboard/google/herobrine/bootblock.c
M src/mainboard/google/herobrine/chromeos.c
4 files changed, 81 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/50581/97
--
To view, visit https://review.coreboot.org/c/coreboot/+/50581
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8cbdd1d59a0166688d52d61646db1b6764879a7c
Gerrit-Change-Number: 50581
Gerrit-PatchSet: 97
Gerrit-Owner: Ravi kumar <rbokka(a)codeaurora.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: mturney mturney <mturney(a)codeaurora.org>
Gerrit-CC: Mukesh Savaliya <msavaliy(a)qualcomm.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Rajesh Patil <rajpat(a)qualcomm.corp-partner.google.com>
Gerrit-CC: Ravi Kumar Bokka <c_rbokka(a)qualcomm.corp-partner.google.com>
Gerrit-CC: Sudheer Amrabadi <samrabad(a)codeaurora.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Ravi kumar <rbokka(a)codeaurora.org>
Gerrit-Attention: mturney mturney <mturney(a)codeaurora.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Rajesh Patil <rajpat(a)qualcomm.corp-partner.google.com>
Gerrit-MessageType: newpatchset