Change in ...coreboot[master]: soc/intel/apl: Call mca_configure() on cold boots only

[Nico Huber (Code Review)]

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31200

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

soc/intel/apl: Call mca_configure() on cold boots only

By BIOS Spec we must not do this on warm boots. It also addresses half
of the TODO comment.

TEST=Confirmed that warm boots on Kontron/mAL10 don't hang any more.

Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Signed-off-by: Nico Huber 
---
M src/soc/intel/apollolake/cpu.c
1 file changed, 5 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/31200/1

diff --git a/src/soc/intel/apollolake/cpu.c b/src/soc/intel/apollolake/cpu.c
index bcec28e..c3b689a 100644
--- a/src/soc/intel/apollolake/cpu.c
+++ b/src/soc/intel/apollolake/cpu.c
@@ -17,6 +17,7 @@
  * GNU General Public License for more details.
  */

+#include <arch/acpi.h>
 #include <assert.h>
 #include <console/console.h>
 #include "chip.h"
@@ -70,10 +71,10 @@
 void soc_core_init(struct device *cpu)
 {
    /* Clear out pending MCEs */
-	/* TODO(adurbin): This should only be done on a cold boot. Also, some
-	 * of these banks are core vs package scope. For now every CPU clears
-	 * every bank. */
-	mca_configure(NULL);
+	/* TODO(adurbin): Some of these banks are core vs package
+			  scope. For now every CPU clears every bank. */
+	if (acpi_get_sleep_type() != ACPI_S0)
+		mca_configure(NULL);

/* Set core MSRs */
    reg_script_run(core_msr_script);

[Nico Huber (Code Review)]

Hello Werner Zeh, Aaron Durbin, Patrick Rudolph, Subrata Banik, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31200

to look at the new patch set (#2).

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

soc/intel/apl: Call mca_configure() on cold boots only

By APL BIOS Spec, we must not do this on warm boots. Also, an
x5-E3940 hangs in tests after touching these MSRs on a warm boot.

The TODO comment seems stale and copied over. So the actual
requirements for SGX are unknown and we add a guard for that
case.

TEST=Confirmed that warm boots on Kontron/mAL10 don't hang any more.

Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Signed-off-by: Nico Huber 
---
M src/soc/intel/apollolake/cpu.c
1 file changed, 6 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/31200/2

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31200 )

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

Patch Set 2: Code-Review+2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31200 )

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

Patch Set 2: Code-Review+1

[Nico Huber (Code Review)]

Hello Werner Zeh, Aaron Durbin, Patrick Rudolph, Subrata Banik, Paul Menzel, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31200

to look at the new patch set (#3).

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

soc/intel/apl: Call mca_configure() on cold boots only

By APL BIOS Spec, we must not do this on warm boots.

The TODO comment seems stale and copied over. So the actual
requirements for SGX are unknown and we add a guard for that
case.

Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Signed-off-by: Nico Huber 
---
M src/soc/intel/apollolake/cpu.c
1 file changed, 6 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/31200/3

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31200 )

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

Patch Set 3:

My testing was flawed. When I tried to actually check
MCEs from the OS after a warm boot, it hanged in the OS.

Still, this change makes sense, separate from my problem.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31200 )

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

Patch Set 3: Code-Review+1

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31200 )

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/31200/3/src/soc/intel/apollolake/cpu.c 
File src/soc/intel/apollolake/cpu.c:

https://review.coreboot.org/#/c/31200/3/src/soc/intel/apollolake/cpu.c@76 
PS3, Line 76: IS_ENABLED(SOC_INTEL_COMMON_BLOCK_SGX
What is the dependency to SGX here? If I get it right we are dealing with the internal machine check logic here which is capable of reporting different kinds of internal system errors. It makes sense to clear this errors on every cold boot and not touch this registers on warm boot so that an OS based driver could report these errors. But why clearing them always if SGX is enabled?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31200 )

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31200/3/src/soc/intel/apollolake/cpu.c 
File src/soc/intel/apollolake/cpu.c:

https://review.coreboot.org/#/c/31200/3/src/soc/intel/apollolake/cpu.c@76 
PS3, Line 76: IS_ENABLED(SOC_INTEL_COMMON_BLOCK_SGX
> What is the dependency to SGX here? If I get it right we are dealing with the internal machine check […]

The call was added for SGX support in dc194e2bc4e. I don't
know the details nor where to look them up. Will add Pratik,
maybe he can tell us more.

[Pratikkumar V Prajapati (Code Review)]

Pratikkumar V Prajapati has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31200 )

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31200/3/src/soc/intel/apollolake/cpu.c 
File src/soc/intel/apollolake/cpu.c:

https://review.coreboot.org/#/c/31200/3/src/soc/intel/apollolake/cpu.c@76 
PS3, Line 76: IS_ENABLED(SOC_INTEL_COMMON_BLOCK_SGX
> The call was added for SGX support in dc194e2bc4e. I don't […]

SGX configuration needs microcode reload and for that we need to have MCA regs cleared otherwise SGX wont get enabled.

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31200 )

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

Patch Set 3: Code-Review+2
> Patch Set 3:
>
> (1 comment)

Ah, thank you for the clarification.

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31200 )

Change subject: soc/intel/apl: Call mca_configure() on cold boots only
......................................................................

soc/intel/apl: Call mca_configure() on cold boots only

By APL BIOS Spec, we must not do this on warm boots.

The TODO comment seems stale and copied over. So the actual
requirements for SGX are unknown and we add a guard for that
case.

Change-Id: I09b4a2fe22267d7318951aac20a3ea566403492e
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/31200
Reviewed-by: Angel Pons 
Reviewed-by: Werner Zeh 
Reviewed-by: Aaron Durbin 
Reviewed-by: Paul Menzel 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/apollolake/cpu.c
1 file changed, 6 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Aaron Durbin: Looks good to me, approved
  Werner Zeh: Looks good to me, approved
  Angel Pons: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/apollolake/cpu.c b/src/soc/intel/apollolake/cpu.c
index bcec28e..d1c5f6f 100644
--- a/src/soc/intel/apollolake/cpu.c
+++ b/src/soc/intel/apollolake/cpu.c
@@ -17,6 +17,7 @@
  * GNU General Public License for more details.
  */

+#include <arch/acpi.h>
 #include <assert.h>
 #include <console/console.h>
 #include "chip.h"
@@ -70,10 +71,11 @@
 void soc_core_init(struct device *cpu)
 {
    /* Clear out pending MCEs */
-	/* TODO(adurbin): This should only be done on a cold boot. Also, some
-	 * of these banks are core vs package scope. For now every CPU clears
-	 * every bank. */
-	mca_configure(NULL);
+	/* TODO(adurbin): Some of these banks are core vs package
+			  scope. For now every CPU clears every bank. */
+	if (IS_ENABLED(SOC_INTEL_COMMON_BLOCK_SGX) ||
+	    acpi_get_sleep_type() == ACPI_S5)
+		mca_configure(NULL);

/* Set core MSRs */
    reg_script_run(core_msr_script);