Attention is currently required from: Keith Hui, Tim Wawrzynczak, Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52642 )
Change subject: cpu/x86/mtrr: Prefer keeping WRCOMB requests to reserving MTRRs for OS
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> True, this turns it fully into an "advisory" in terms of coreboot, it will now happily keep WC reque […]
It just seems really weird to add a Kconfig in one commit and cripple it
in the very next. Either we feel safe that Slot1 platforms won't need that
many MTRRs (or the OS wouldn't know how to do better anyway), or we need
that Kconfig option. It doesn't seem that there were new revelations since
the parent commit?
Should we just try to find somebody to look into it? My preference would be
no Kconfig at all and always printing the message at level info. But for
that we'd need to know how things are on the old platforms.
--
To view, visit https://review.coreboot.org/c/coreboot/+/52642
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I108c743d25baaeae1860732601cc7abda2f05932
Gerrit-Change-Number: 52642
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Keith Hui <buurin(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Attention: Keith Hui <buurin(a)gmail.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Fri, 30 Apr 2021 15:21:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Nico Huber, Furquan Shaikh, Matt DeVillier, Angel Pons, Michael Niewöhner, Patrick Rudolph, Felix Held.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52493 )
Change subject: [RFC] soc/intel/skylake: Introduce new method for setting device states
......................................................................
Patch Set 9:
(1 comment)
File src/soc/intel/skylake/fspdevmap.h:
https://review.coreboot.org/c/coreboot/+/52493/comment/5a58bfc4_a882d294
PS9, Line 9: const uint8_t inverted;
> Only reason I brought it up is that in the list you have in the CL, you have 0's added. […]
Yes, I understood that. Very appreciated :)
So, I'll move the FSP option to the first position and the indicator to the last one.
--
To view, visit https://review.coreboot.org/c/coreboot/+/52493
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I70fde306c65a8881f565c5f923be20f380ea64d3
Gerrit-Change-Number: 52493
Gerrit-PatchSet: 9
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
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-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 30 Apr 2021 15:10:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Attention is currently required from: Michael Niewöhner.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52627 )
Change subject: soc/intel/skylake: Clean up root port structs
......................................................................
Patch Set 3:
(1 comment)
File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/52627/comment/ad648ba3_668d66d6
PS2, Line 35: static const struct pcie_rp_group pch_rp_groups[] = {
: #if CONFIG(SKYLAKE_SOC_PCH_H)
: { .slot = PCH_DEV_SLOT_PCIE, .count = 8 },
: { .slot = PCH_DEV_SLOT_PCIE_1, .count = 8 },
: /* Sunrise Point PCH-H actually only has 4 ports in the
: third group. But that would require a runtime check
: and probing 4 non-existent ports shouldn't hurt. */
: { .slot = PCH_DEV_SLOT_PCIE_2, .count = 8 },
: #else
: { .slot = PCH_DEV_SLOT_PCIE, .count = 8 },
: { .slot = PCH_DEV_SLOT_PCIE_1, .count = 4 },
: #endif
:
> why confusing? the third controller is only there on PCH-H
I am not talking about the third controller (the count is untouched there) but the second. In your example, you moved it out of the if-else-condition. Sure it wouldn't hurt probing just 4 ports more, but it makes me feel we are misusing it. So we would have to add a comment there too (in addition to the existing one), because we want to save two lines of code. Workarounds are confusing. Then why even care about the root port count?
However, as Patrick mentioned, the compiler should optimize it anyway. So maybe the code is just fine as it is.
--
To view, visit https://review.coreboot.org/c/coreboot/+/52627
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I0d5155356bd302ef938c76eb60688276fec67502
Gerrit-Change-Number: 52627
Gerrit-PatchSet: 3
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Comment-Date: Fri, 30 Apr 2021 14:51:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Martin Roth, Marshall Dawson, Karthik Ramasubramanian, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52771 )
Change subject: soc/amd/common: Move external oscillator config away from common
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> It is weird that the patch could not be verified for more than 12 hours.
What happens if you rebase?
--
To view, visit https://review.coreboot.org/c/coreboot/+/52771
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8c5d98addfba750f9ddb87a846599541b4a8340a
Gerrit-Change-Number: 52771
Gerrit-PatchSet: 1
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: 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: 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: Fri, 30 Apr 2021 14:51:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Nico Huber, Furquan Shaikh, Matt DeVillier, Angel Pons, Michael Niewöhner, Patrick Rudolph, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52493 )
Change subject: [RFC] soc/intel/skylake: Introduce new method for setting device states
......................................................................
Patch Set 9:
(1 comment)
File src/soc/intel/skylake/fspdevmap.h:
https://review.coreboot.org/c/coreboot/+/52493/comment/1f1d934b_3a1dd057
PS9, Line 9: const uint8_t inverted;
> Let's just avoid the discussion by providing a completely different […]
Only reason I brought it up is that in the list you have in the CL, you have 0's added. I was proposing you move it so you can drop the 0 from the list:
i.e.,
const struct device_fspoption_map devmap[] = {
{ SA_DEVFN_TS, ¶ms->Device4Enable },
{ SA_DEVFN_GMM, ¶ms->GmmEnable },
{ SA_DEVFN_IMGU, ¶ms->SaImguEnable },
{ SA_DEVFN_CHAP, &tconfig->ChapDeviceEnable },
{ PCH_DEVFN_CIO, ¶ms->PchCio2Enable },
{ PCH_DEVFN_GBE, ¶ms->PchLanEnable },
{ PCH_DEVFN_ISH, ¶ms->PchIshEnable },
{ PCH_DEVFN_HDA, ¶ms->PchHdaEnable },
{ PCH_DEVFN_SPI, ¶ms->ShowSpiController },
{ PCH_DEVFN_EMMC, ¶ms->ScsEmmcEnabled },
{ PCH_DEVFN_SATA, ¶ms->SataEnable },
{ PCH_DEVFN_USBOTG, ¶ms->XdciEnable },
{ PCH_DEVFN_SDCARD, ¶ms->ScsSdCardEnabled },
{ PCH_DEVFN_THERMAL, ¶ms->PchThermalDeviceEnable },
};
--
To view, visit https://review.coreboot.org/c/coreboot/+/52493
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I70fde306c65a8881f565c5f923be20f380ea64d3
Gerrit-Change-Number: 52493
Gerrit-PatchSet: 9
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
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-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 30 Apr 2021 14:50:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Attention is currently required from: Star Labs, Patrick Rudolph.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52767 )
Change subject: Added LabTop series Added ITE 8987E Added LOCKDIS to mark SPI as writable in SKL Added CMOS setting to disable ME
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/52767/comment/84e00246_1f25cd74
PS1, Line 6:
: Added LabTop series
: Added ITE 8987E
: Added LOCKDIS to mark SPI as writable in SKL
: Added CMOS setting to disable ME
Ideally this should be split into separate patches
--
To view, visit https://review.coreboot.org/c/coreboot/+/52767
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I71d099d6dad529fff79c1ccf30082152a92a284d
Gerrit-Change-Number: 52767
Gerrit-PatchSet: 1
Gerrit-Owner: Star Labs <admin(a)starlabs.systems>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Star Labs <admin(a)starlabs.systems>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Fri, 30 Apr 2021 14:47:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52642 )
Change subject: cpu/x86/mtrr: Prefer keeping WRCOMB requests to reserving MTRRs for OS
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> The change seems harmless but it leaves config RESERVE_MTRRS_FOR_OS […]
True, this turns it fully into an "advisory" in terms of coreboot, it will now happily keep WC requests in favor of dropping them for the OS to use... thoughts? CPU_INTEL_SLOT_1 is the only user of this Kconfig, I have no idea what the memory map might look like for something like that.
--
To view, visit https://review.coreboot.org/c/coreboot/+/52642
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I108c743d25baaeae1860732601cc7abda2f05932
Gerrit-Change-Number: 52642
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Comment-Date: Fri, 30 Apr 2021 14:31:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Martin Roth, Marshall Dawson, Felix Held.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52771 )
Change subject: soc/amd/common: Move external oscillator config away from common
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
It is weird that the patch could not be verified for more than 12 hours.
--
To view, visit https://review.coreboot.org/c/coreboot/+/52771
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8c5d98addfba750f9ddb87a846599541b4a8340a
Gerrit-Change-Number: 52771
Gerrit-PatchSet: 1
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: 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: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 30 Apr 2021 14:26:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Marshall Dawson, Karthik Ramasubramanian, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52771 )
Change subject: soc/amd/common: Move external oscillator config away from common
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/52771
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8c5d98addfba750f9ddb87a846599541b4a8340a
Gerrit-Change-Number: 52771
Gerrit-PatchSet: 1
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
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-Attention: Jason Glenesk <jason.glenesk(a)gmail.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: Fri, 30 Apr 2021 14:24:11 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment