Attention is currently required from: Shelley Chen, Douglas Anderson.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56901 )
Change subject: trogdor: Fix "TPM interrupt" lb_gpio to be ACTIVE_HIGH
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> > Pardon my ignorance, but what exactly makes this a "latched" GPIO? Is it because it's a GPIO that' […]
"latched" means the line is edge triggered and the hardware will set an interrupt flag as soon as it sees the appropriate edge which is cleared the next time software checks for it. In coreboot it's basically a totally different API -- gpio_input_irq() vs. gpio_input() and gpio_irq_status() vs. gpio_get(). But we're using the same table to pass both kinds of GPIOs to depthcharge, and in depthcharge they're wrapped in the same GpioOps API despite having different underpinnings (new_gpio_latched_from_coreboot() vs. new_gpio_input_from_coreboot()). The problem here is that the parsing of this table is handled by the same code for both kinds of GPIOs (sysinfo_lookup_gpio()), and as soon as that sees an ACTIVE_LOW flag it will automatically wrap the whole thing in a new_gpio_not() in depthcharge. So ACTIVE_LOW means "negate this in depthcharge", which makes sense for level-trigged GPIOs (because in our depthcharge code we always assume asserted = high for everything and use this negation mechanism for active low lines), but it doesn't make sense for latched GPIOs because the decision which edge to trigger on was already made by the initial coreboot code that programmed the controller to do this latching, and from then on out it can only return 1 for "yes I saw the edge" or 0 for "didn't see a new edge yet since the last time I was called". Putting a negation wrapper around that is always wrong.
(GPIO_AP_EC_INT is fine because it's not using the latched mechanism, it's a normal level-triggered GPIO and the EC only takes it high again after the triggering condition has been removed. If you ask me they should have programmed H1 to just do the same thing because it's a ton easier to deal with, but nobody asked me about that...)
--
To view, visit https://review.coreboot.org/c/coreboot/+/56901
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie1586b0e10b64df0712e28552411c4d540a7e457
Gerrit-Change-Number: 56901
Gerrit-PatchSet: 1
Gerrit-Owner: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Douglas Anderson <dianders(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Douglas Anderson <dianders(a)chromium.org>
Gerrit-Comment-Date: Mon, 16 Aug 2021 22:25:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Douglas Anderson <dianders(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Diana Zigterman, Paul Menzel, Jason Glenesk, Nikolai Vyssotski.
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55434 )
Change subject: mb/google/guybrush: Enable STT in device tree
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/55434
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I37b5da1b56586ef75ad17f6766cd00ddac87aa5a
Gerrit-Change-Number: 55434
Gerrit-PatchSet: 3
Gerrit-Owner: Jason Glenesk <jason.glenesk(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Diana Zigterman <dzigterman(a)google.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Matt Papageorge <matthewpapa07(a)gmail.com>
Gerrit-Reviewer: Nikolai Vyssotski <nikolai.vyssotski(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Diana Zigterman <dzigterman(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)amd.corp-partner.google.com>
Gerrit-Attention: Nikolai Vyssotski <nikolai.vyssotski(a)amd.corp-partner.google.com>
Gerrit-Comment-Date: Mon, 16 Aug 2021 22:08:30 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Shelley Chen, Julius Werner.
Douglas Anderson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56901 )
Change subject: trogdor: Fix "TPM interrupt" lb_gpio to be ACTIVE_HIGH
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> Pardon my ignorance, but what exactly makes this a "latched" GPIO? Is it because it's a GPIO that's exposed from coreboot to depthcharge and thus virtualized? If so, why would the "GPIO_AP_EC_INT" right above it be any different?
Any answer to this question?
--
To view, visit https://review.coreboot.org/c/coreboot/+/56901
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie1586b0e10b64df0712e28552411c4d540a7e457
Gerrit-Change-Number: 56901
Gerrit-PatchSet: 1
Gerrit-Owner: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Douglas Anderson <dianders(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Mon, 16 Aug 2021 21:54:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Douglas Anderson <dianders(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Martin Roth, Furquan Shaikh, Marshall Dawson, Karthik Ramasubramanian, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56960 )
Change subject: soc/amd/common: Show current SPI speeds and modes
......................................................................
Patch Set 1:
(1 comment)
File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/56960/comment/e1939af6_1c0f5b98
PS1, Line 40: printk(BIOS_DEBUG,"SPI normal read speed: %s\n",
> space required after that ',' (ctx:VxV)
Please fix.
--
To view, visit https://review.coreboot.org/c/coreboot/+/56960
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I7825a9337474c147b803c85c9af7f9dc24670459
Gerrit-Change-Number: 56960
Gerrit-PatchSet: 1
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: 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-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Mon, 16 Aug 2021 21:51:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Martin Roth, Furquan Shaikh, Marshall Dawson, Karthik Ramasubramanian, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56959 )
Change subject: soc/amd/common: Update SPI based on Kconfig & EFS instead of devtree
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/56959/comment/b90c4c86_60780a0f
PS1, Line 42: !
When would this fail?
--
To view, visit https://review.coreboot.org/c/coreboot/+/56959
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8a24f637b2a0061f60a8f736121d224d4c4ba69b
Gerrit-Change-Number: 56959
Gerrit-PatchSet: 1
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: 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-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Mon, 16 Aug 2021 21:49:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Martin Roth, Furquan Shaikh, Marshall Dawson, Karthik Ramasubramanian, Felix Held.
Raul Rangel 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:
(3 comments)
File src/mainboard/google/guybrush/Kconfig:
https://review.coreboot.org/c/coreboot/+/56885/comment/a9e413d9_04975aeb
PS5, Line 107: ALT_SPI_SPEED
When is the ALT used?
https://review.coreboot.org/c/coreboot/+/56885/comment/1acabd1d_6f5f8853
PS5, Line 110: config TPM_SPI_SPEED
: default 0 # 66MHz
:
Can we remove this since we don't use the TPM SPI interface on guybrush/zork?
File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/56885/comment/e3b0d968_058035d5
PS5, Line 250: 4
Can you split this out into its own CL. Curious what the boot time improvement is, if any...
--
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: 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-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Mon, 16 Aug 2021 21:44:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Jason Glenesk, Martin Roth, Furquan Shaikh, Marshall Dawson, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56815 )
Change subject: mb/amd/majolica/Kconfig: add EFS SPI settings
......................................................................
Patch Set 3:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56815/comment/07f570f3_1a368dbc
PS3, Line 12: EM100
Does the em100 work on Majolica?
--
To view, visit https://review.coreboot.org/c/coreboot/+/56815
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8996c2bf606ccd21686092beac8d96b22c0b7869
Gerrit-Change-Number: 56815
Gerrit-PatchSet: 3
Gerrit-Owner: 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: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Martin Roth <martinroth(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
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: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Mon, 16 Aug 2021 21:41:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Caveh Jalali, Tim Wawrzynczak, Nick Vaccaro, Zhuohao Lee.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56984 )
Change subject: mb/google/volteer: reduce RW_MRC_CACHE size to 64K
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56984/comment/035bfee9_ca69693e
PS2, Line 7: reduce RW_MRC_CACHE size to 64K
FMAP cannot be changed once a device has shipped. Since some volteer devices have already qual'ed and shipped, I think it is better to just leave the MRC cache at the current size. The space being recovered isn't really being used for anything else anyways i.e. lives as unused region in RW_MISC.
You can however check if these savings can be applied to newer platforms like brya.
--
To view, visit https://review.coreboot.org/c/coreboot/+/56984
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I91544b7b825161b09992c972bd7c3d3732719735
Gerrit-Change-Number: 56984
Gerrit-PatchSet: 2
Gerrit-Owner: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Zhuohao Lee <zhuohao(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Furquan Shaikh <furquan(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: Zhuohao Lee <zhuohao(a)google.com>
Gerrit-Comment-Date: Mon, 16 Aug 2021 21:32:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Tim Wawrzynczak.
Hello Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/56984
to look at the new patch set (#2).
Change subject: mb/google/volteer: reduce RW_MRC_CACHE size to 64K
......................................................................
mb/google/volteer: reduce RW_MRC_CACHE size to 64K
BUG=b:162015973
TEST="emerge-volteer coreboot chromeos-bootimage", flash and boot
volteer2 to kernel and verify the firmware_CorruptRecoveryCache and
firmware_CorruptRecoveryCache faft tests pass.
Change-Id: I91544b7b825161b09992c972bd7c3d3732719735
Signed-off-by: Nick Vaccaro <nvaccaro(a)google.com>
---
M src/mainboard/google/volteer/chromeos.fmd
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/56984/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/56984
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I91544b7b825161b09992c972bd7c3d3732719735
Gerrit-Change-Number: 56984
Gerrit-PatchSet: 2
Gerrit-Owner: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: newpatchset