Attention is currently required from: Chen Wisley, Wisley Chen.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59269 )
Change subject: mb/google/brya/var/redrix: De-assert SSD PERST# in romstage
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/59269/comment/2062d5a2_f58b4d72
PS1, Line 11:
suggestion: add something like
```
The reason for this is to give enough time after PERST#
deassertion so that the SSD has enough time to initialize before the FSP scans the RPs for downstream devices.
```
--
To view, visit https://review.coreboot.org/c/coreboot/+/59269
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I242cb1517f564d9d135d523b1e7f95ac34d601f8
Gerrit-Change-Number: 59269
Gerrit-PatchSet: 1
Gerrit-Owner: Chen Wisley <wisley.chen(a)quantatw.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Wisley Chen <wisley.chen(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Chen Wisley <wisley.chen(a)quantatw.com>
Gerrit-Attention: Wisley Chen <wisley.chen(a)quanta.corp-partner.google.com>
Gerrit-Comment-Date: Fri, 12 Nov 2021 21:05:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Chen Wisley.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59267 )
Change subject: mb/google/brya/var/redrix: Correct WWAN power sequence
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
File src/mainboard/google/brya/variants/redrix/gpio.c:
https://review.coreboot.org/c/coreboot/+/59267/comment/41447a61_588145ac
PS2, Line 71: /* E0 : SATAXPCIE0 ==> WWAN_PERST_L */
suggestion: add a comment here explaining that E0 is sequenced out-of-order here is so that PERST_L is sequenced after RST_L
--
To view, visit https://review.coreboot.org/c/coreboot/+/59267
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ibba1ecc04b563ae4eedd7596594f33812cbac150
Gerrit-Change-Number: 59267
Gerrit-PatchSet: 2
Gerrit-Owner: Chen Wisley <wisley.chen(a)quantatw.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Wisley Chen <wisley.chen(a)quanta.corp-partner.google.com>
Gerrit-Attention: Chen Wisley <wisley.chen(a)quantatw.com>
Gerrit-Comment-Date: Fri, 12 Nov 2021 21:03:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nick Vaccaro, EricR Lai.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59236 )
Change subject: mb/google/brya/var/felwinter: Remove USB2 port 0
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/59236/comment/bb56dd0c_4fcb4adf
PS1, Line 9: shcematics
`as per schematics`
--
To view, visit https://review.coreboot.org/c/coreboot/+/59236
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I45d467a80c23d82dc33dcbed176430a758eea403
Gerrit-Change-Number: 59236
Gerrit-PatchSet: 1
Gerrit-Owner: EricR Lai <ericr_lai(a)compal.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-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Comment-Date: Fri, 12 Nov 2021 21:00:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nick Vaccaro, EricR Lai.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59234 )
Change subject: mb/google/brya/var/felwinter: Enable garage pen detection
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Patchset:
PS1:
Looks like wake should work too
--
To view, visit https://review.coreboot.org/c/coreboot/+/59234
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib5929c876d1a0da34dadd7997a61ab8e75acbbb6
Gerrit-Change-Number: 59234
Gerrit-PatchSet: 1
Gerrit-Owner: EricR Lai <ericr_lai(a)compal.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-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Comment-Date: Fri, 12 Nov 2021 20:55:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: zanxi chen.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59044 )
Change subject: mb/google/trogdor: Adjust mipi panel backlight gpio
......................................................................
Patch Set 5:
(3 comments)
File src/mainboard/google/trogdor/board.h:
https://review.coreboot.org/c/coreboot/+/59044/comment/e3c58dc6_877bc52d
PS5, Line 16: #define GPIO_TP_EN GPIO(85)
This should be under the #if below, and set to dead_code_t(gpio_t) for eDP boards.
File src/mainboard/google/trogdor/chromeos.c:
https://review.coreboot.org/c/coreboot/+/59044/comment/aa893d37_20807e42
PS5, Line 18: gpio_output(GPIO_TP_EN, 0);
Is this necessary? This pin should already be a pull-down on reset.
File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/59044/comment/dc8864c0_148d7f3e
PS5, Line 145: if (CONFIG(BOARD_GOOGLE_MRBLAND) || CONFIG(BOARD_GOOGLE_WORMDINGLER))
Why not TROGDOR_HAS_MIPI_PANEL? I hope this part is the same across all MIPI variants?
--
To view, visit https://review.coreboot.org/c/coreboot/+/59044
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie9920e5366f6b1ea9e0da228bd211317516b390a
Gerrit-Change-Number: 59044
Gerrit-PatchSet: 5
Gerrit-Owner: zanxi chen <chenzanxi(a)huaqin.corp-partner.google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: xuxinxiong <xuxinxiong(a)huaqin.corp-partner.google.com>
Gerrit-CC: Weimin Wu <wuweimin(a)huaqin.corp-partner.google.com>
Gerrit-Attention: zanxi chen <chenzanxi(a)huaqin.corp-partner.google.com>
Gerrit-Comment-Date: Fri, 12 Nov 2021 20:47:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Julius Werner, Kyösti Mälkki, Rob Barnes, Karthik Ramasubramanian.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59021 )
Change subject: arch/x86/smp/spinlock: Fix threading when !STAGE_HAS_SPINLOCKS
......................................................................
Patch Set 2:
(1 comment)
File src/arch/x86/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/59021/comment/89dd8068_dac5b3e7
PS2, Line 50: thread_coop_disable();
> Well, if threading is disabled then thread_mutex_lock() is a no-op right now. […]
So I've been playing around with the mutex idea locally. One thing I realized is that regardless of spin_lock or mutex, we still need to disable coop multi-threading in vprintk. printk is the primary debugging mechanism, so if printk is off yielding every character, it makes it very very difficult to debug the threading code. e.g., if the threading code hits an assert and tries to printk during a printk yield, we end up in a deadlock.
I've audited the places where spin_lock is used, and printk is the only one that causes problems because it calls `udelay` in the UART driver.
So here are the two things I'm going to do first:
1) Remove thread_coop_enable/disable from spin_lock/unlock.
2) Add a thread_coop_disable/enable to vprintk
This will solve the immediate problem of thread coop not working in romstage or earlier.
The second thing I can do is implement a multi-cpu mutex and migrate all spin_lock use cases over to mutex. This will give us a single synchronization primitive that works for both coop and multi-cpus. Having this will reduce the chances of things breaking down the road a udelay() gets added somehow into a critical section. I have the WIP code for this in my local work tree already. It's written in C instead of asm, so we won't need arch specific behavior.
--
To view, visit https://review.coreboot.org/c/coreboot/+/59021
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iea621fcdad8f0367acce4f70be42a4e9a68da938
Gerrit-Change-Number: 59021
Gerrit-PatchSet: 2
Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan.m.shaikh(a)gmail.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Attention: Furquan Shaikh <furquan.m.shaikh(a)gmail.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Attention: Rob Barnes <robbarnes(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Fri, 12 Nov 2021 20:43:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org>
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Miriam Polzer, Andrey Pronin, Yu-Ping Wu.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59097 )
Change subject: security/vboot: Add NVRAM counter for TPM 2.0
......................................................................
Patch Set 3:
(2 comments)
File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/59097/comment/9bfb2a3b_ec23fbf4
PS3, Line 150: .TPMA_NV_NO_DA = 1,
> NV_BITS makes it a bit index which is bit different from a counter. […]
I don't know, maybe Andrey does. Maybe there are other ways to trigger a DA lockout (like aborting transactions half-way through or something)?
https://review.coreboot.org/c/coreboot/+/59097/comment/917ef303_4bd47d80
PS3, Line 438: NULL, 0)
> I think recreating the counter wont help an attacker, it will start again at a higher ot the same va […]
Oh... yeah, okay, I guess that's already designed for that purpose. Then I guess this should be good as it is.
--
To view, visit https://review.coreboot.org/c/coreboot/+/59097
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I511dba3b3461713ce20fb2bda9fced0fee6517e1
Gerrit-Change-Number: 59097
Gerrit-PatchSet: 3
Gerrit-Owner: Miriam Polzer <mpolzer(a)google.com>
Gerrit-Reviewer: Andrey Pronin <apronin(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Miriam Polzer <mpolzer(a)google.com>
Gerrit-Attention: Andrey Pronin <apronin(a)chromium.org>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Comment-Date: Fri, 12 Nov 2021 20:33:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Miriam Polzer <mpolzer(a)google.com>
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Andrey Pronin, Christian Walter, Julius Werner, Tim Wawrzynczak.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59134 )
Change subject: security/tpm/tcg-2.0: Handle TPM_RC_NV_RANGE return code
......................................................................
Patch Set 2:
(1 comment)
File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/59134/comment/896b290b_0c7824c4
PS2, Line 246: return TPM_E_READ_EMPTY;
> Don't we want to define a new code for this? I don't think READ_EMPTY really fits.
How about TPM_E_READ_TOO_LARGE or TPM_E_REQUEST_TOO_LARGE?
--
To view, visit https://review.coreboot.org/c/coreboot/+/59134
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8b403e2f33cc1368065cc21f73df1102695f73eb
Gerrit-Change-Number: 59134
Gerrit-PatchSet: 2
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Andrey Pronin <apronin(a)google.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Andrey Pronin <apronin(a)google.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)google.com>
Gerrit-Comment-Date: Fri, 12 Nov 2021 20:20:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Tim Wawrzynczak, Sugnan Prabhu S, Ravindra, Michael Niewöhner, Patrick Rudolph, Ravindra N.
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57783 )
Change subject: cpu/intel/common: Update CPPCv3 Nominal Frequency entry
......................................................................
Patch Set 7:
(1 comment)
File src/cpu/intel/common/common_init.c:
https://review.coreboot.org/c/coreboot/+/57783/comment/13b201a7_a1d3e6e7
PS7, Line 136: CPPC_DWORD(0);
> The difference is: CPPC_UNSUPPORTED resulting in `{Register {(SystemMemory, 0, 0, 0, 0)}` which the […]
@Tim/Michael,
Correct, here I am just initializing the element to ZERO.
In my follow-up patches, I will be updating the element with Nominal Frequency in the file#src/soc/intel/common/block/acpi/acpi.c.
As these changes are from 2 difference code directories, with this patch, I am setting the CPPC_NOMINAL_FREQ element to ZERO. In other patch, I will be updating the element with Nominal Frequency. I will expose all my patch train for review.
--
To view, visit https://review.coreboot.org/c/coreboot/+/57783
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: If39e5e4ac77a71b143cdb5f043fb398abb398ee3
Gerrit-Change-Number: 57783
Gerrit-PatchSet: 7
Gerrit-Owner: Ravindra <ravindra(a)intel.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Maulik V Vaghela <maulik.v.vaghela(a)intel.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Ravindra N <ravindra(a)intel.corp-partner.google.com>
Gerrit-CC: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com>
Gerrit-Attention: Ravindra <ravindra(a)intel.com>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: Ravindra N <ravindra(a)intel.corp-partner.google.com>
Gerrit-Comment-Date: Fri, 12 Nov 2021 20:10:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Marshall Dawson, Tim Wawrzynczak, Kyösti Mälkki, Karthikeyan Ramasubramanian, Felix Held.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59094 )
Change subject: Add ENV_STAGE_SUPPORTS_SMP to clean up spinlock stubs
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/59094
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iba52febdeee78294f916775ee9ce8a82d6203570
Gerrit-Change-Number: 59094
Gerrit-PatchSet: 3
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Karthikeyan Ramasubramanian <kramasub(a)chromium.org>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Attention: Karthikeyan Ramasubramanian <kramasub(a)chromium.org>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 12 Nov 2021 20:00:12 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment