Patrick Georgi submitted this change.

View Change

Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Matt DeVillier: Looks good to me, approved
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@mniewoehner.de>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40877
Reviewed-by: Matt DeVillier <matt.devillier@gmail.com>
Reviewed-by: Paul Menzel <paulepanter@users.sourceforge.net>
Tested-by: build bot (Jenkins) <no-reply@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(-)

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 change 40877. To unsubscribe, or for help writing mail filters, visit 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@intel.com>
Gerrit-Reviewer: Angel Pena Galvão <mragalvao@gmail.com>
Gerrit-Reviewer: Jeremy Soller <jeremy@system76.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com>
Gerrit-Reviewer: Nico Huber <nico.h@gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com>
Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net>
Gerrit-Reviewer: Piotr Król <piotr.krol@3mdeb.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com>
Gerrit-Reviewer: Swift Geek (Sebastian Grzywna) <swiftgeek@gmail.com>
Gerrit-Reviewer: Youness Alaoui <snifikino@gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus@gmail.com>
Gerrit-MessageType: merged