Hello build bot (Jenkins), Paul Menzel, Angel Pons, Werner Zeh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41036
to look at the new patch set (#2).
Change subject: payloads/external/GRUB2: Makefile: fix check for changed files again
......................................................................
payloads/external/GRUB2: Makefile: fix check for changed files again
This fixes the missing closing brace introduced in CB:40953.
Change-Id: I295c67ab8d7596bf54cc69d088ef1df906f58d5f
Signed-off-by: Michael Niewöhner <foss(a)mniewoehner.de>
---
M payloads/external/GRUB2/Makefile
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/41036/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/41036
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I295c67ab8d7596bf54cc69d088ef1df906f58d5f
Gerrit-Change-Number: 41036
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-MessageType: newpatchset
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41036 )
Change subject: payloads/external/GRUB2: Makefile: fix check for changed files again
......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41036/1//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/41036/1//COMMIT_MSG@9
PS1, Line 9: closig
closing
--
To view, visit https://review.coreboot.org/c/coreboot/+/41036
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I295c67ab8d7596bf54cc69d088ef1df906f58d5f
Gerrit-Change-Number: 41036
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Comment-Date: Mon, 04 May 2020 20:29:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41036 )
Change subject: payloads/external/GRUB2: Makefile: fix check for changed files again
......................................................................
Patch Set 1: Code-Review+2
Tested according to irc user _3dsv
--
To view, visit https://review.coreboot.org/c/coreboot/+/41036
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I295c67ab8d7596bf54cc69d088ef1df906f58d5f
Gerrit-Change-Number: 41036
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 04 May 2020 20:29:34 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41036 )
Change subject: payloads/external/GRUB2: Makefile: fix check for changed files again
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/41036/1//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/41036/1//COMMIT_MSG@9
PS1, Line 9: cb
Mind making this 'cb' uppercase please? You can edit the commit msg from the gerrit UI
--
To view, visit https://review.coreboot.org/c/coreboot/+/41036
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I295c67ab8d7596bf54cc69d088ef1df906f58d5f
Gerrit-Change-Number: 41036
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 04 May 2020 20:23:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40775 )
Change subject: Documentation: Update vboot on lenovo
......................................................................
Documentation: Update vboot on lenovo
Update the documentation now that CB:32705 is merged.
Change-Id: I9845c0750ec4016188478154610400d1b8556793
Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com>
---
M Documentation/mainboard/lenovo/vboot.md
1 file changed, 2 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/40775/1
diff --git a/Documentation/mainboard/lenovo/vboot.md b/Documentation/mainboard/lenovo/vboot.md
index 4e1b946..24fbdad 100644
--- a/Documentation/mainboard/lenovo/vboot.md
+++ b/Documentation/mainboard/lenovo/vboot.md
@@ -18,8 +18,8 @@
partition that acts as failsafe in case the regular firmware update, that
goes to the `A` or `B` partition fails.
-**Note:** The `RO` partition isn't write-protected by default. There's a patch
-pending on gerrit [CB:32705] that write-protects the `RO` partition.
+**Note:** The `RO` partition isn't write-protected by default, as you
+have to enable the protection in the security Kconfig menu by yourself.
On *Lenovo* devices you can enable the *Fn* key as recovery mode switch, by
enabling `CONFIG_H8_FN_KEY_AS_VBOOT_RECOVERY_SW`.
@@ -35,5 +35,4 @@
[vboot] on *Lenovo* devices uses the CMOS to store configuration data, like
boot failures and the last successfully booted partition.
-[CB:32705]: https://review.coreboot.org/32705
[vboot]: ../../security/vboot/index.md
--
To view, visit https://review.coreboot.org/c/coreboot/+/40775
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I9845c0750ec4016188478154610400d1b8556793
Gerrit-Change-Number: 40775
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-MessageType: newchange
Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40950 )
Change subject: mb/purism/librem_skl: Fix CLKREQ for 15v3 NVMe
......................................................................
mb/purism/librem_skl: Fix CLKREQ for 15v3 NVMe
Per the schematics, SRCCLKREQ2# is used for the NVMe and should be enabled.
Enable CLKREQ for PCIe RP9 to match the comment, and just comment
to indicate correct value per schematic.
Test: build/boot Librem 15v3 with NVMe drive, verify drive identified
properly and no errors in boot log.
Signed-off-by: Matt DeVillier <matt.devillier(a)puri.sm>
Change-Id: I159cb7ce1f5195d95c0229490c3bbde26edbd375
---
M src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/40950/1
diff --git a/src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb b/src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb
index 308688a..d9c4f14 100644
--- a/src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb
+++ b/src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb
@@ -164,8 +164,8 @@
register "PcieRpEnable[4]" = "1"
register "PcieRpEnable[8]" = "1"
# Enable CLKREQ# for RP9
- register "PcieRpClkReqSupport[8]" = "0"
- # ClkReq for NVMe - Bruteforced (no other value works)
+ register "PcieRpClkReqSupport[8]" = "1"
+ # SRCCLKREQ2# for NVMe per schematic
register "PcieRpClkReqNumber[8]" = "2"
register "usb2_ports[0]" = "USB2_PORT_TYPE_C(OC_SKIP)" # Type-C Port
--
To view, visit https://review.coreboot.org/c/coreboot/+/40950
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I159cb7ce1f5195d95c0229490c3bbde26edbd375
Gerrit-Change-Number: 40950
Gerrit-PatchSet: 1
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-MessageType: newchange
Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40909 )
Change subject: mb/purism/librem_skl: drop SataSpeedLimit restriction
......................................................................
mb/purism/librem_skl: drop SataSpeedLimit restriction
SataSpeedLimit was set to 3Gbps to work around issues which are now
known to be the result of incorrect FSP behavior. Since SataPwrOptEnable is
now set at the SoC level and ensures the SIR registers are correctly programmed,
we can re-enable 6Gbps operation works without errors.
Test: build/boot Librem 13v2 with both m.2 and 2.5" SATA drives,
check dmesg for errors.
Change-Id: I3565dc063724ad288ef92361942fcdc14daac17e
Signed-off-by: Matt DeVillier <matt.devillier(a)puri.sm>
---
M src/mainboard/purism/librem_skl/variants/librem13v2/devicetree.cb
M src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb
2 files changed, 0 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/40909/1
diff --git a/src/mainboard/purism/librem_skl/variants/librem13v2/devicetree.cb b/src/mainboard/purism/librem_skl/variants/librem13v2/devicetree.cb
index f69c482..89e3841 100644
--- a/src/mainboard/purism/librem_skl/variants/librem13v2/devicetree.cb
+++ b/src/mainboard/purism/librem_skl/variants/librem13v2/devicetree.cb
@@ -51,7 +51,6 @@
register "SataPortsEnable[2]" = "1"
register "SataPortsDevSlp[0]" = "0"
register "SataPortsDevSlp[2]" = "0"
- register "SataSpeedLimit" = "2"
register "EnableAzalia" = "1"
register "DspEnable" = "0"
register "IoBufferOwnership" = "0"
diff --git a/src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb b/src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb
index f5b8b99..308688a 100644
--- a/src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb
+++ b/src/mainboard/purism/librem_skl/variants/librem15v3/devicetree.cb
@@ -51,7 +51,6 @@
register "SataPortsEnable[2]" = "1"
register "SataPortsDevSlp[0]" = "0"
register "SataPortsDevSlp[2]" = "0"
- register "SataSpeedLimit" = "2"
register "EnableAzalia" = "1"
register "DspEnable" = "0"
register "IoBufferOwnership" = "0"
--
To view, visit https://review.coreboot.org/c/coreboot/+/40909
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3565dc063724ad288ef92361942fcdc14daac17e
Gerrit-Change-Number: 40909
Gerrit-PatchSet: 1
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-MessageType: newchange
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40877 )
Change subject: soc/intel/skl: always enable SataPwrOptEnable
......................................................................
soc/intel/skl: always enable SataPwrOptEnable
For unknown reasons FSP skips a whole bunch of SIR (SATA Initialization
Registers) when SataPwrOptEnable=0, which currently is the default in
coreboot and FSP. Even if FSP's default was 1, coreboot would reset it.
This can lead to all sorts of problems and errors, for example:
- links get lost
- only 1.5 or 3 Gbps instead of 6 Gbps
- "unaligned write" errors in Linux
- ...
At least on two boards (supermicro/x11-lga1151-series/x11ssm-f and
purism/librem13v2) SATA is not working correctly and showing such
symptoms.
To let FSP correctly initialize the SATA controller, enable the option
SataPwrOptEnable statically. There is no valid reason to disable it,
which might break SATA, anyway.
Currently, there are no reported issues on CML and CNL, so a change
there could not be tested reliably. SKL/KBL was tested successfully
without any noticable downsides. Thus, only SKL gets changed for now.
Change-Id: I8531ba9743453a3118b389565517eb769b5e7929
Signed-off-by: Michael Niewöhner <foss(a)mniewoehner.de>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40877
Reviewed-by: Matt DeVillier <matt.devillier(a)gmail.com>
Reviewed-by: Paul Menzel <paulepanter(a)users.sourceforge.net>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M src/mainboard/51nb/x210/devicetree.cb
M src/mainboard/google/fizz/variants/baseboard/devicetree.cb
M src/mainboard/protectli/vault_kbl/devicetree.cb
M src/soc/intel/skylake/chip.c
M src/soc/intel/skylake/chip.h
5 files changed, 7 insertions(+), 7 deletions(-)
Approvals:
build bot (Jenkins): Verified
Paul Menzel: Looks good to me, but someone else must approve
Matt DeVillier: Looks good to me, approved
diff --git a/src/mainboard/51nb/x210/devicetree.cb b/src/mainboard/51nb/x210/devicetree.cb
index 82bbb1f..e453aa4 100644
--- a/src/mainboard/51nb/x210/devicetree.cb
+++ b/src/mainboard/51nb/x210/devicetree.cb
@@ -48,7 +48,6 @@
register "SataPortsDevSlp[0]" = "1"
register "SataPortsDevSlp[1]" = "1"
register "SataPortsDevSlp[2]" = "1"
- register "SataPwrOptEnable" = "1"
register "EnableAzalia" = "1"
register "DspEnable" = "0"
register "IoBufferOwnership" = "0"
diff --git a/src/mainboard/google/fizz/variants/baseboard/devicetree.cb b/src/mainboard/google/fizz/variants/baseboard/devicetree.cb
index 5da9997..f02acce 100644
--- a/src/mainboard/google/fizz/variants/baseboard/devicetree.cb
+++ b/src/mainboard/google/fizz/variants/baseboard/devicetree.cb
@@ -72,7 +72,6 @@
register "SataPortsEnable[0]" = "1"
register "SataPortsEnable[1]" = "1"
register "SataPortsDevSlp[1]" = "1"
- register "SataPwrOptEnable" = "1"
register "EnableAzalia" = "1"
register "DspEnable" = "1"
register "IoBufferOwnership" = "3"
diff --git a/src/mainboard/protectli/vault_kbl/devicetree.cb b/src/mainboard/protectli/vault_kbl/devicetree.cb
index d53e43e..d3e8b23 100644
--- a/src/mainboard/protectli/vault_kbl/devicetree.cb
+++ b/src/mainboard/protectli/vault_kbl/devicetree.cb
@@ -38,7 +38,6 @@
register "EnableSata" = "1"
register "SataSalpSupport" = "0"
register "SataMode" = "0"
- register "SataPwrOptEnable" = "1"
register "EnableAzalia" = "0"
register "DspEnable" = "0"
register "IoBufferOwnership" = "0"
diff --git a/src/soc/intel/skylake/chip.c b/src/soc/intel/skylake/chip.c
index b731baf..b24ec4f 100644
--- a/src/soc/intel/skylake/chip.c
+++ b/src/soc/intel/skylake/chip.c
@@ -258,9 +258,15 @@
params->SataEnable = config->EnableSata;
params->SataMode = config->SataMode;
params->SataSpeedLimit = config->SataSpeedLimit;
- params->SataPwrOptEnable = config->SataPwrOptEnable;
params->EnableTcoTimer = !config->PmTimerDisabled;
+ /*
+ * For unknown reasons FSP skips writing some essential SATA init registers (SIR) when
+ * SataPwrOptEnable=0. This results in link errors, "unaligned write" errors and others.
+ * Enabling this option solves these problems.
+ */
+ params->SataPwrOptEnable = 1;
+
tconfig->PchLockDownGlobalSmi = config->LockDownConfigGlobalSmi;
tconfig->PchLockDownRtcLock = config->LockDownConfigRtcLock;
tconfig->PowerLimit4 = config->PowerLimit4;
diff --git a/src/soc/intel/skylake/chip.h b/src/soc/intel/skylake/chip.h
index 387bd6f..d268eeb 100644
--- a/src/soc/intel/skylake/chip.h
+++ b/src/soc/intel/skylake/chip.h
@@ -567,9 +567,6 @@
*/
u8 IslVrCmd;
- /* Enable/Disable Sata power optimization */
- u8 SataPwrOptEnable;
-
/* Enable/Disable Sata test mode */
u8 SataTestMode;
--
To view, visit https://review.coreboot.org/c/coreboot/+/40877
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8531ba9743453a3118b389565517eb769b5e7929
Gerrit-Change-Number: 40877
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Angel Pena Galvão <mragalvao(a)gmail.com>
Gerrit-Reviewer: Jeremy Soller <jeremy(a)system76.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
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: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com>
Gerrit-Reviewer: Youness Alaoui <snifikino(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: merged
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40414 )
Change subject: lib/spd_bin: add get_spd_sn function
......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block…
File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block…
PS4, Line 85: return sn == 0x00000000, if addr is 0x0.
: return sn == 0xffffffff, if dimm is not present. */
How about just return values from the function?
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block…
PS4, Line 87: u8 *sn
This still looks weird. I understand there is the implicit length and you mentioned it above, but if it's four bytes, why not pass a u32, or a custom struct that has a u8[4] field in it, or even a length passed here too, and you could return an error on that length < SPD_SN_LEN
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block…
PS4, Line 92: /* smbus will return 0xff if addr is zero */
: if (addr == 0x00) {
: memset(sn, 0, SPD_SN_LEN);
: return;
: }
If the do_smbus_read_byte() function already handles checking for an invalid address, I don't see a good reason to do it here too. If it's an invalid address, the SMBus library should return SMBUS_WAIT_UNTIL_ACTIVE_TIMEOUT (i.e. CB_I2C_NO_DEVICE).
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block…
PS4, Line 99: dram_type
This is a u8, but do_dmbus_read_byte returns an int (where negative numbers are the error values), so this would be the place to check for an error.
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block…
PS4, Line 120: Unsupport
Unsupported
--
To view, visit https://review.coreboot.org/c/coreboot/+/40414
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I406bba7cc56debbd9851d430f069e4fb96ec937c
Gerrit-Change-Number: 40414
Gerrit-PatchSet: 4
Gerrit-Owner: Jamie Chen <jamie.chen(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: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Kane Chen <kane.chen(a)intel.com>
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-CC: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Mon, 04 May 2020 18:41:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment