Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Nick Vaccaro, EricR Lai.
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58180 )
Change subject: mb/google/brya: Clear SLP_S0_GATE_L on ACPI sleep entry
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> 10 seconds for S5->G3 is expected because that is the timeout used by EC (https://source.chromium. […]
good to know, but i also don't see SYS_SLP_S0IX_L dropping for
10 seconds. shouldn't that respond very quickly with the
SLP_S0_GATE_L fix?
--
To view, visit https://review.coreboot.org/c/coreboot/+/58180
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5500e701ac8731d141b51dc381609c80047dc1f8
Gerrit-Change-Number: 58180
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
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: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Comment-Date: Fri, 08 Oct 2021 01:16:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Comment-In-Reply-To: Caveh Jalali <caveh(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Caveh Jalali, Tim Wawrzynczak, Nick Vaccaro, EricR Lai.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58180 )
Change subject: mb/google/brya: Clear SLP_S0_GATE_L on ACPI sleep entry
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> i'm getting different behavior on brya than described in the […]
10 seconds for S5->G3 is expected because that is the timeout used by EC (https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/pla…) before it starts transitioning out of S5 into G3.
--
To view, visit https://review.coreboot.org/c/coreboot/+/58180
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5500e701ac8731d141b51dc381609c80047dc1f8
Gerrit-Change-Number: 58180
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
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: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Comment-Date: Fri, 08 Oct 2021 00:47:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Caveh Jalali <caveh(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Tim Wawrzynczak, Nick Vaccaro, EricR Lai.
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58180 )
Change subject: mb/google/brya: Clear SLP_S0_GATE_L on ACPI sleep entry
......................................................................
Patch Set 2: Code-Review-1
(1 comment)
Patchset:
PS2:
i'm getting different behavior on brya than described in the
patch description and the 10 second S5 -> G3 delay is still
there. let's hold off on this until we get to the bottom of
the discrepancy.
--
To view, visit https://review.coreboot.org/c/coreboot/+/58180
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5500e701ac8731d141b51dc381609c80047dc1f8
Gerrit-Change-Number: 58180
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
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: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Comment-Date: Fri, 08 Oct 2021 00:11:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
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/+/57555 )
Change subject: tests: Add lib/lzma-test test case
......................................................................
Patch Set 7:
(9 comments)
Patchset:
PS7:
Sorry for the long review delay.
File tests/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/57555/comment/69bcf91f_9bc78c25
PS7, Line 240: lzma-test-cflags += -I "$(src)/lib"
Why not just write `#include <lib/lzmadecode.h> in the code? I think that would make it easier to follow where that file is found.
https://review.coreboot.org/c/coreboot/+/57555/comment/6a188a3b_2817baa3
PS7, Line 241: lzma-test-cflags += -DLZMA_TEST_DATA_PATH=\"$(testsrc)/data/lib/lzma-test/\"
Maybe just define a `-DTEST_DATA_DIR=\"$(testsrc)/data\" in the base tests/Makefile.inc and use that for all tests that need this kind of thing? (I'd like to reduce the amount of special magic that is hidden in test-specific cflags to the minimum necessary.)
File tests/lib/lzma-test.c:
https://review.coreboot.org/c/coreboot/+/57555/comment/e89e2ca3_748d0810
PS7, Line 52: goto finish;
Weird way to write `return 1`?
https://review.coreboot.org/c/coreboot/+/57555/comment/9443c5f0_5248a00b
PS7, Line 57: s->raw_filename = test_malloc(raw_filename_size);
nit: you can just use asprintf() to make all of this a lot easier.
https://review.coreboot.org/c/coreboot/+/57555/comment/b74ac85e_a3ff3b31
PS7, Line 65: goto error;
Nope. teardown() expects *state to point to s, but at this point it still points to fname_base. (In general, I don't think you need to worry about allocation cleanup in error cases that much, the program is gonna exit soon after anyway... I think it's fine to just return here. Just don't make it segfault.)
https://review.coreboot.org/c/coreboot/+/57555/comment/9b5a14f0_0563f1d9
PS7, Line 88: goto finish;
nit: just write `return 0`?
https://review.coreboot.org/c/coreboot/+/57555/comment/6b28f991_f4d2eded
PS7, Line 154: (
Do these parens do anything? (Also, in general please prefer struct initializers with designated member names rather than just relying on order, as it's easier to read... e.g. .name = "test...(...)", .test_func = test_ulzman_correct_file, etc.)
https://review.coreboot.org/c/coreboot/+/57555/comment/0c09ec9d_a6ad3726
PS7, Line 161: ULZMAN_CORRECT_FILE_TEST("data.1"),
It would be good if somewhere (e.g. maybe here, with line comments?) it was documented what these 4 test files each represent.
--
To view, visit https://review.coreboot.org/c/coreboot/+/57555
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id75e0b41991382d4c391b031862106de58eacdf7
Gerrit-Change-Number: 57555
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: Werner Zeh <werner.zeh(a)siemens.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-Attention: Jan Dabros <jsd(a)semihalf.com>
Gerrit-Comment-Date: Fri, 08 Oct 2021 00:04:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Patrick Rudolph.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58188 )
Change subject: nb/sandybridge:add CBMEM_MEMINFO table when initing RAM
......................................................................
Patch Set 1:
(6 comments)
File src/northbridge/intel/sandybridge/raminit_mrc.c:
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-130019):
https://review.coreboot.org/c/coreboot/+/58188/comment/7c1147bd_c57e27e1
PS1, Line 392: struct memory_info* mem_info;
"foo* bar" should be "foo *bar"
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-130019):
https://review.coreboot.org/c/coreboot/+/58188/comment/3e4802d1_45af275a
PS1, Line 422: memcpy(dimm->serial, // bytes 122-125 in SPD
please, no space before tabs
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-130019):
https://review.coreboot.org/c/coreboot/+/58188/comment/b334da75_8236466d
PS1, Line 428: dimm->mod_id = // bytes 117/118 (LSB/MSB)
line over 96 characters
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-130019):
https://review.coreboot.org/c/coreboot/+/58188/comment/6f7cfd16_86babe49
PS1, Line 446: memcpy(dimm->serial, // bytes 122-125 in SPD
please, no space before tabs
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-130019):
https://review.coreboot.org/c/coreboot/+/58188/comment/a14f9a4c_d62b7c20
PS1, Line 452: dimm->mod_id = // bytes 117/118 (LSB/MSB)
line over 96 characters
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-130019):
https://review.coreboot.org/c/coreboot/+/58188/comment/92b85c81_e60e6923
PS1, Line 452: dimm->mod_id = // bytes 117/118 (LSB/MSB)
please, no space before tabs
--
To view, visit https://review.coreboot.org/c/coreboot/+/58188
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I15e00a01121150b778cfa684b9147d0cac97beb8
Gerrit-Change-Number: 58188
Gerrit-PatchSet: 1
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Thu, 07 Oct 2021 23:59:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/58186 )
Change subject: mb/google/caroline: Update _HID for digitizer
......................................................................
mb/google/caroline: Update _HID for digitizer
Caroline uses a Wacom digitizer, so adjust the ACPI HID
so that the proper drivers attach under Windows/Linux.
Change-Id: I732b09001dc41a91a32a5f9260abdab435b28b8a
Signed-off-by: Matt DeVillier <matt.devillier(a)gmail.com>
---
M src/mainboard/google/glados/variants/caroline/include/variant/acpi/mainboard.asl
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/58186/1
diff --git a/src/mainboard/google/glados/variants/caroline/include/variant/acpi/mainboard.asl b/src/mainboard/google/glados/variants/caroline/include/variant/acpi/mainboard.asl
index f382e08..7d1196e 100644
--- a/src/mainboard/google/glados/variants/caroline/include/variant/acpi/mainboard.asl
+++ b/src/mainboard/google/glados/variants/caroline/include/variant/acpi/mainboard.asl
@@ -55,7 +55,7 @@
{
Device (DIGI)
{
- Name (_HID, "ACPI0C50")
+ Name (_HID, "WCOM005C")
Name (_CID, "PNP0C50")
Name (_UID, 1)
Name (_S0W, 4)
--
To view, visit https://review.coreboot.org/c/coreboot/+/58186
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I732b09001dc41a91a32a5f9260abdab435b28b8a
Gerrit-Change-Number: 58186
Gerrit-PatchSet: 1
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-MessageType: newchange