Attention is currently required from: Bao Zheng, Jason Nien, Paul Menzel, Jason Nein, Rob Barnes.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63722 )
Change subject: mb/google/nipperkin: Fix WLAN to GEN2 speed
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63722/comment/1d5342fa_07b8df61
PS1, Line 10: during S0ix suspend
> We suspect a PCIe link speed negotiation issue and are capturing PCIE analyzer traces, ADS runs, and […]
Ack. Also the location of the hang is documented in the commit message.
--
To view, visit https://review.coreboot.org/c/coreboot/+/63722
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I988365e51aca0d6515c5605b3032521cf59d8d30
Gerrit-Change-Number: 63722
Gerrit-PatchSet: 4
Gerrit-Owner: Rob Barnes <robbarnes(a)google.com>
Gerrit-Reviewer: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Kevin Chiu <kevin.chiu.17802(a)gmail.com>
Gerrit-Reviewer: Patrick Huang <patrick.huang(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jason Nein <finaljason(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jason Nein <finaljason(a)gmail.com>
Gerrit-Attention: Rob Barnes <robbarnes(a)google.com>
Gerrit-Comment-Date: Thu, 21 Apr 2022 17:00:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Comment-In-Reply-To: Jason Nein <finaljason(a)gmail.com>
Comment-In-Reply-To: Rob Barnes <robbarnes(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63763 )
Change subject: device/Kconfig: Change ON_DEVICE_ROM_LOAD default
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Should we add something to configs/ to keep the path build tested?
Currently there are Galileo configs enabling it, but they might be
gone soon.
--
To view, visit https://review.coreboot.org/c/coreboot/+/63763
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I70b4ca6bf83f2d2fa591e30967e481d67e1b9f87
Gerrit-Change-Number: 63763
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Thu, 21 Apr 2022 16:57:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63763 )
Change subject: device/Kconfig: Change ON_DEVICE_ROM_LOAD default
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/63763
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I70b4ca6bf83f2d2fa591e30967e481d67e1b9f87
Gerrit-Change-Number: 63763
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Thu, 21 Apr 2022 16:47:36 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Subrata Banik, Tim Wawrzynczak, Angel Pons, Arthur Heymans, Nick Vaccaro, Lean Sheng Tan, Eric Lai, Werner Zeh.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63640 )
Change subject: soc/intel/common/smbus: Add `finalize` operation for smbus
......................................................................
Patch Set 6:
(1 comment)
File src/soc/intel/common/block/smbus/smbus.c:
https://review.coreboot.org/c/coreboot/+/63640/comment/d6eba21e_ecdb5bb4
PS5, Line 77: tco_lockdown();
> > > t's important to ensure that TCO base address is locked before running 3rd party code
> >
> > Then it's already too late? running option ROMs happens during PCI device initialization (.init), so by .final, it's already happened.
>
> Valid point Tim, IMO, we should bind this with a bootstate which is equivalent to (.enable) entry. WDYT ?
That would be an ubiquitous problem in coreboot... Option roms from external PCI devices are just an insecure setup and should be disabled by default. CB:63763
--
To view, visit https://review.coreboot.org/c/coreboot/+/63640
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie945680049514e6c5d797790a381a6946e836926
Gerrit-Change-Number: 63640
Gerrit-PatchSet: 6
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Comment-Date: Thu, 21 Apr 2022 16:44:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Comment-In-Reply-To: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-MessageType: comment
Attention is currently required from: Jamie Chen, Tim Wawrzynczak.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63675 )
Change subject: soc/intel/jasperlake: CNVi: Enable fewer wakeups to reduce SoC power consumption
......................................................................
Patch Set 5: Code-Review+2
(2 comments)
File src/soc/intel/jasperlake/chip.h:
https://review.coreboot.org/c/coreboot/+/63675/comment/282ed3af_e18fc1b9
PS5, Line 428: * Setting this on a system that supports S0i3 (set xtalsdqdis [Bit 22] in
: * cppmvric1 register to 0) will break CNVI timing.
> The xtalsdqdis bit can be set or clear by ASL code or OS code. […]
Ack
File src/soc/intel/jasperlake/finalize.c:
https://review.coreboot.org/c/coreboot/+/63675/comment/cacba913_21b1b413
PS5, Line 63: if (config->cnvi_reduce_s0ix_pwr_usage) {
> This is only for system with CNVi module to work around and keeping this enabled will not impact the […]
Ack
--
To view, visit https://review.coreboot.org/c/coreboot/+/63675
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I56439a406547e2ee1e47d34be14ecc9a8df04693
Gerrit-Change-Number: 63675
Gerrit-PatchSet: 5
Gerrit-Owner: Jamie Chen <jamie.chen(a)intel.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Kane Chen <kane.chen(a)intel.com>
Gerrit-CC: Simon Yang <simon1.yang(a)intel.com>
Gerrit-Attention: Jamie Chen <jamie.chen(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)google.com>
Gerrit-Comment-Date: Thu, 21 Apr 2022 16:41:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Jamie Chen <jamie.chen(a)intel.com>
Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: comment
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/63763 )
Change subject: device/Kconfig: Change ON_DEVICE_ROM_LOAD default
......................................................................
device/Kconfig: Change ON_DEVICE_ROM_LOAD default
Defaulting to 'y' to load optional blobs from PCI cards is not a good
idea in an open source project. On top of that PCI option roms are a
security issue.
This would affect desktop use cases that expect graphic output in the
payload on an external GPU.
Change-Id: I70b4ca6bf83f2d2fa591e30967e481d67e1b9f87
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/device/Kconfig
1 file changed, 1 insertion(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/63763/1
diff --git a/src/device/Kconfig b/src/device/Kconfig
index 388972c..1899a95 100644
--- a/src/device/Kconfig
+++ b/src/device/Kconfig
@@ -171,8 +171,7 @@
config ON_DEVICE_ROM_LOAD
bool "Load Option ROMs on PCI devices"
- default n if PAYLOAD_SEABIOS
- default y if !PAYLOAD_SEABIOS
+ default n
depends on VGA_ROM_RUN
help
Load Option ROMs stored on PCI/PCIe/AGP VGA devices in coreboot.
@@ -186,8 +185,6 @@
more complete BIOS interrupt services available than coreboot,
which some option ROMs require in order to function correctly.
- If unsure, say N when using SeaBIOS as payload, Y otherwise.
-
choice
prompt "Option ROM execution type"
default PCI_OPTION_ROM_RUN_YABEL if !ARCH_X86
--
To view, visit https://review.coreboot.org/c/coreboot/+/63763
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I70b4ca6bf83f2d2fa591e30967e481d67e1b9f87
Gerrit-Change-Number: 63763
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: newchange
Attention is currently required from: Wonkyu Kim, Tim Wawrzynczak, Paul Menzel, Michael Niewöhner.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63615 )
Change subject: intel/common/../systemagent: Add sa_get_mchbar_base api
......................................................................
Patch Set 12: Code-Review+1
(1 comment)
Patchset:
PS12:
LGTM, @angel, any feedback ?
--
To view, visit https://review.coreboot.org/c/coreboot/+/63615
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie4c7af3ea8c2b2b6afcc76e1165fadbe15e0bceb
Gerrit-Change-Number: 63615
Gerrit-PatchSet: 12
Gerrit-Owner: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Subrata Banik <subratabanik(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: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Comment-Date: Thu, 21 Apr 2022 16:30:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Tim Wawrzynczak, Angel Pons, Arthur Heymans, Nick Vaccaro, Lean Sheng Tan, Eric Lai, Werner Zeh.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63640 )
Change subject: soc/intel/common/smbus: Add `finalize` operation for smbus
......................................................................
Patch Set 6:
(1 comment)
File src/soc/intel/common/block/smbus/smbus.c:
https://review.coreboot.org/c/coreboot/+/63640/comment/fe82a45a_fc57e73d
PS5, Line 77: tco_lockdown();
> > t's important to ensure that TCO base address is locked before running 3rd party code
>
> Then it's already too late? running option ROMs happens during PCI device initialization (.init), so by .final, it's already happened.
Valid point Tim, IMO, we should bind this with a bootstate which is equivalent to (.enable) entry. WDYT ?
--
To view, visit https://review.coreboot.org/c/coreboot/+/63640
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie945680049514e6c5d797790a381a6946e836926
Gerrit-Change-Number: 63640
Gerrit-PatchSet: 6
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.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-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Comment-Date: Thu, 21 Apr 2022 16:29:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Comment-In-Reply-To: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-MessageType: comment
Attention is currently required from: Subrata Banik, Tim Wawrzynczak, Paul Menzel, Michael Niewöhner.
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63615 )
Change subject: intel/common/../systemagent: Add sa_get_mchbar_base api
......................................................................
Patch Set 12:
(2 comments)
File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/63615/comment/3b6d8fe4_29de1818
PS11, Line 89: if(mchbar_lo & 0x1)
> one space between if and brace ?
Done
https://review.coreboot.org/c/coreboot/+/63615/comment/fcbaf1ff_1fc8805d
PS11, Line 94: if(mchbar_base == 0) {
> one space between if and brace ?
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/63615
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie4c7af3ea8c2b2b6afcc76e1165fadbe15e0bceb
Gerrit-Change-Number: 63615
Gerrit-PatchSet: 12
Gerrit-Owner: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Subrata Banik <subratabanik(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: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Comment-Date: Thu, 21 Apr 2022 16:14:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Fred Reitberger, Felix Held.
Jon Murphy has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63703 )
Change subject: soc/amd/sabrina: Disable lpc ldrq function
......................................................................
Patch Set 2:
(1 comment)
File src/soc/amd/sabrina/include/soc/espi.h:
https://review.coreboot.org/c/coreboot/+/63703/comment/487af82d_7bbe1715
PS1, Line 6: define
> Why move them? We don't need to make them public for others to consume.
Discussed offline, thought it may be useful to expose, but agreed it's YAGNI. It can be moved if/when needed.
--
To view, visit https://review.coreboot.org/c/coreboot/+/63703
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I680c10dc8916c23ce6934d87aee5e484d4a84b78
Gerrit-Change-Number: 63703
Gerrit-PatchSet: 2
Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.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: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Thu, 21 Apr 2022 16:12:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org>
Gerrit-MessageType: comment