Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36622 )
Change subject: drivers/fsp2_0: drop support for FSP-T
......................................................................
Patch Set 4:
> Patch Set 4:
>
> > Patch Set 4: Code-Review+1
> >
> > (1 comment)
> >
> > > Patch Set 4: Code-Review-1
> > >
> > > (1 comment)
> > >
> > > Is this really necessary, FSP-T is optional right?
> >
> > coreboot is an open source project, blobs that replace open source code have no place here. The reason for its removal is exactly that it is optional.
>
> Further we already have a superior open source version. Why try to mainting something that is not working for most people?
coreboot is an open-source project and is used by both businesses and the community. There may be a reason that this is needed for some business applications. I don't know.
I think we need to evaluate the trade-offs. Intel should also be allowed to weigh-in before it's summarily deleted.
Please start a discussion on the mailing list about this deletion. That's the official communication channel as it's easy to miss individual patches.
Finally, if the decision ultimately is to delete this, it should happen after the next release and be documented in the release notes.
Also, please note that I really have no stake in it staying or going - I just want to make sure that it's properly discussed and decided upon before being removed.
--
To view, visit https://review.coreboot.org/c/coreboot/+/36622
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib791b30b621730f4b7c0a5f668a3b6559245daf5
Gerrit-Change-Number: 36622
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Guckian
Gerrit-Reviewer: David Guckian <david.guckian(a)intel.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Michael Niewöhner
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Vanessa Eusebio <vanessa.f.eusebio(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Tue, 05 Nov 2019 18:46:38 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36622 )
Change subject: drivers/fsp2_0: drop support for FSP-T
......................................................................
Patch Set 4:
> Patch Set 4: Code-Review+1
>
> (1 comment)
>
> > Patch Set 4: Code-Review-1
> >
> > (1 comment)
> >
> > Is this really necessary, FSP-T is optional right?
>
> coreboot is an open source project, blobs that replace open source code have no place here. The reason for its removal is exactly that it is optional.
Further we already have a superior open source version. Why try to mainting something that is not working for most people?
--
To view, visit https://review.coreboot.org/c/coreboot/+/36622
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib791b30b621730f4b7c0a5f668a3b6559245daf5
Gerrit-Change-Number: 36622
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Guckian
Gerrit-Reviewer: David Guckian <david.guckian(a)intel.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Michael Niewöhner
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Vanessa Eusebio <vanessa.f.eusebio(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Tue, 05 Nov 2019 17:15:12 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36622 )
Change subject: drivers/fsp2_0: drop support for FSP-T
......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36622/4//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/36622/4//COMMIT_MSG@9
PS4, Line 9: FSP-T is only working in very few cases
> Is this the only justification for removing FSP-T from fsp2_0 driver?
It doesn't make sense to maintain some binary blob we know nothing about, while we have a open source implementation.
--
To view, visit https://review.coreboot.org/c/coreboot/+/36622
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib791b30b621730f4b7c0a5f668a3b6559245daf5
Gerrit-Change-Number: 36622
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Guckian
Gerrit-Reviewer: David Guckian <david.guckian(a)intel.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Michael Niewöhner
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Vanessa Eusebio <vanessa.f.eusebio(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Tue, 05 Nov 2019 17:13:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: David Guckian
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Hello Aaron Durbin, Karthik Ramasubramanian, Julius Werner, build bot (Jenkins), Joel Kitching, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36341
to look at the new patch set (#18).
Change subject: security/vboot: Build vboot library with same .a that depthcharge uses
......................................................................
security/vboot: Build vboot library with same .a that depthcharge uses
Currently, depthcharge and coreboot are using two different vboot libraries.
coreboot is using "fwlib20", while depthcharge uses "fwlib". The only
difference between the two libraries is the inclusion of vboot1-only
compilation units in fwlib, which are now deprecated. Therefore, coreboot
may as well use fwlib too. Vboot is expected to converge on a single firmware
library soon.
BUG=none
BRANCH=none
TEST=compiles and runs verstage correctly
Change-Id: I905b781c3596965ec7ef45a2a7eafe15fdd4d9cc
Signed-off-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
---
M src/security/vboot/Makefile.inc
1 file changed, 5 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/36341/18
--
To view, visit https://review.coreboot.org/c/coreboot/+/36341
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I905b781c3596965ec7ef45a2a7eafe15fdd4d9cc
Gerrit-Change-Number: 36341
Gerrit-PatchSet: 18
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Joel Kitching <kitching(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(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)users.sourceforge.net>
Gerrit-MessageType: newpatchset
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36211 )
Change subject: cpu/intel/car: Add EC software sync to Intel romstage
......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36211/1//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/36211/1//COMMIT_MSG@9
PS1, Line 9: Perform EC software sync in romstage, before memory training is started.
> Explain why.
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/36211
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I31f3407a2afcbf288461fab1397f965f025bc07c
Gerrit-Change-Number: 36211
Gerrit-PatchSet: 19
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Joel Kitching <kitching(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Tue, 05 Nov 2019 16:50:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment
Hello Aaron Durbin, Patrick Rudolph, Karthik Ramasubramanian, Julius Werner, Arthur Heymans, Duncan Laurie, Shelley Chen, build bot (Jenkins), Joel Kitching, Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36211
to look at the new patch set (#19).
Change subject: cpu/intel/car: Add EC software sync to Intel romstage
......................................................................
cpu/intel/car: Add EC software sync to Intel romstage
Perform EC software sync in romstage, before memory training is started.
Because the ChromeOS EC will not currently perform USB-PD negotiation
until it jumps to running its RW code, this allows the system to get
access to more power earlier in the boot flow.
This is guarded by CONFIG_VBOOT_EARLY_EC_SYNC.
BUG=b:112198832
BRANCH=none
TEST=EC software sync works in update and non-update case.
No significant effect on boot time (~6 ms).
Change-Id: I31f3407a2afcbf288461fab1397f965f025bc07c
Signed-off-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
---
M src/cpu/intel/car/romstage.c
1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/36211/19
--
To view, visit https://review.coreboot.org/c/coreboot/+/36211
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I31f3407a2afcbf288461fab1397f965f025bc07c
Gerrit-Change-Number: 36211
Gerrit-PatchSet: 19
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Joel Kitching <kitching(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: newpatchset
Nathaniel L Desimone has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36243 )
Change subject: vendorcode/intel/fsp/tigerlake: Add tigerlake FSP header file template
......................................................................
Patch Set 4: Code-Review+1
@Patrick, since it will still be a while before TigerLake comes out, I'd consider Subrata's submission to fall in the "early" UPD category. While I think this patch is reasonable, I fully agree that some cleanup in the vendorcode/intel directory is needed.
Looking at the vendorcode directory, these were the ones that looked like potential deletion candidates that should be investigated:
• cannonlake (I’m not 100% sure on this one, is that what we use for CoffeeLake/WhiskeyLake?)
• cometlake
• icelake
@Subrata, can you or someone else from Balaji's team take a look?
--
To view, visit https://review.coreboot.org/c/coreboot/+/36243
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia1d064ea60e3b7c2d1114771b07cf7ec7f0e2b37
Gerrit-Change-Number: 36243
Gerrit-PatchSet: 4
Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: John Zhao <john.zhao(a)intel.com>
Gerrit-Reviewer: Nathaniel L Desimone <nathaniel.l.desimone(a)intel.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com>
Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-CC: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Comment-Date: Tue, 05 Nov 2019 16:46:24 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36553 )
Change subject: soc/intel/tigerlake/acpi: Copy acpi directory from icelake
......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36553/1/src/soc/intel/tigerlake/ac…
File src/soc/intel/tigerlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/36553/1/src/soc/intel/tigerlake/ac…
PS1, Line 85: LAnd (LGreaterEqual (Arg0, GPP_G0), LLessEqual (Arg0, GPP_A23)
> It's much harder to review all this code in the old syntax. You don't have to fix all other instances of old ACPI syntax,
Did you mean to change this file alone with new syntax or all .asl file exist inside soc/intel/tigerlake/acpi/*.asl ?
> but for good review to happen on TGL ACPI code it helps greatly.
--
To view, visit https://review.coreboot.org/c/coreboot/+/36553
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: If967cb5904f543ce21eb6e89421df0e5673d2238
Gerrit-Change-Number: 36553
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com>
Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Tue, 05 Nov 2019 16:36:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment