Change in ...coreboot[master]: soc/intel/cannonlake: Update CPU Ratio base on MSR

[Lijian Zhao (Code Review)]

Lijian Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32089

Change subject: soc/intel/cannonlake: Update CPU Ratio base on MSR
......................................................................

soc/intel/cannonlake: Update CPU Ratio base on MSR

The following is logic with FSP, as long as the Cpu Ratio input in
coreboot is different with CpuStrapSet, system will force to follow
input from coreboot. But CpuStrapsetting is floating, it will 0 from
first cold boot before memory training and set to 0x1c after first
memory training.

BUG=b:129412691
TEST=Boot up sarien platform and force recovery, check there's no reset
in the path of recovery.

Signed-off-by: Lijian Zhao 
Change-Id: I959188be46343bc6f2cb3cc149097b4d449802aa
---
M src/soc/intel/cannonlake/romstage/fsp_params.c
1 file changed, 6 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/32089/1

diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c
index 3e3aa5e..b7dbac7 100644
--- a/src/soc/intel/cannonlake/romstage/fsp_params.c
+++ b/src/soc/intel/cannonlake/romstage/fsp_params.c
@@ -15,10 +15,12 @@

#include <assert.h>
 #include <chip.h>
+#include <cpu/x86/msr.h>
 #include <console/console.h>
 #include <fsp/util.h>
 #include <intelblocks/pmclib.h>
 #include <soc/iomap.h>
+#include <soc/msr.h>
 #include <soc/pci_devs.h>
 #include <soc/romstage.h>
 #include <vendorcode/google/chromeos/chromeos.h>
@@ -69,19 +71,10 @@
    m_cfg->SkipMpInit = !CONFIG_USE_INTEL_FSP_MP_INIT;
 #endif

-	/* Disable CPU Flex Ratio and SaGv in recovery mode */
-	if (vboot_recovery_mode_enabled()) {
-		struct chipset_power_state *ps = pmc_get_power_state();
-
-		/*
-		 * Only disable when coming from S5 (cold reset) otherwise
-		 * the flex ratio may be locked and FSP will return an error.
-		 */
-		if (ps && ps->prev_sleep_state == ACPI_S5) {
-			m_cfg->CpuRatio = 0;
-			m_cfg->SaGv = 0;
-		}
-	}
+	/* Set CpuRatio to be match with MSR settings */
+	msr_t flex_ratio;
+	flex_ratio = rdmsr(MSR_FLEX_RATIO);
+	m_cfg->CpuRatio = (flex_ratio.lo >> 8) & 0xff;

/* If ISH is enabled, enable ISH elements */
    if (!dev)

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32089 )

Change subject: soc/intel/cannonlake: Update CPU Ratio base on MSR
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

https://review.coreboot.org/#/c/32089/1/src/soc/intel/cannonlake/romstage/fs... 
File src/soc/intel/cannonlake/romstage/fsp_params.c:

https://review.coreboot.org/#/c/32089/1/src/soc/intel/cannonlake/romstage/fs... 
PS1, Line 74: to be match with MSR settings
"to match existing MSR value"?

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32089 )

Change subject: soc/intel/cannonlake: Update CPU Ratio base on MSR
......................................................................

Patch Set 1: Code-Review+2

[Duncan Laurie (Code Review)]

Duncan Laurie has uploaded a new patch set (#2) to the change originally created by Lijian Zhao. ( https://review.coreboot.org/c/coreboot/+/32089 )

Change subject: soc/intel/cannonlake: Update CPU Ratio base on MSR
......................................................................

soc/intel/cannonlake: Update CPU Ratio base on MSR

The following is the FSP logic: as long as the Cpu Ratio input in
coreboot is different with CpuStrapSet, system will force to follow
input from coreboot. But CpuStrapsetting is floating, it will be 0
from the first cold boot before memory training and set to 0x1c (or
max CPU ratio for the installed CPU) after first memory training.

The previous fix was attempting to ensure settings were cleared
when FSP was called in recovery mode, but only when coming from S5
which caused issues if recovery mode is requested by the OS and
is only followed by a warm reset.

BUG=b:129412691
TEST=Boot up sarien platform and force recovery, check there's no reset
in the path of recovery.

Signed-off-by: Lijian Zhao 
Change-Id: I959188be46343bc6f2cb3cc149097b4d449802aa
---
M src/soc/intel/cannonlake/romstage/fsp_params.c
1 file changed, 6 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/32089/2

[Duncan Laurie (Code Review)]

Duncan Laurie has uploaded a new patch set (#3) to the change originally created by Lijian Zhao. ( https://review.coreboot.org/c/coreboot/+/32089 )

Change subject: soc/intel/cannonlake: Update CPU Ratio base on MSR
......................................................................

soc/intel/cannonlake: Update CPU Ratio base on MSR

The following is the FSP logic: as long as the Cpu Ratio input in
coreboot is different with CpuStrapSet, system will force to follow
input from coreboot. But CpuStrapsetting is floating, it will be 0
from the first cold boot before memory training and set to 0x1c (or
max CPU ratio for the installed CPU) after first memory training.

The previous fix was attempting to ensure settings were cleared
when FSP was called in recovery mode, but only when coming from S5
which caused issues if recovery mode is requested by the OS and
is only followed by a warm reset.

BUG=b:129412691
TEST=Boot up sarien platform and force recovery, check there's no reset
in the path of recovery.

Signed-off-by: Lijian Zhao 
Change-Id: I959188be46343bc6f2cb3cc149097b4d449802aa
---
M src/soc/intel/cannonlake/romstage/fsp_params.c
1 file changed, 6 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/32089/3

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32089 )

Change subject: soc/intel/cannonlake: Update CPU Ratio base on MSR
......................................................................

Patch Set 3: Code-Review+2

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32089 )

Change subject: soc/intel/cannonlake: Update CPU Ratio base on MSR
......................................................................

Patch Set 3: Code-Review+2

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32089 )

Change subject: soc/intel/cannonlake: Update CPU Ratio base on MSR
......................................................................

Patch Set 3:

I am going to submit this because it is blocking lab deployment and about to become a schedule emergency for me.

[Duncan Laurie (Code Review)]

Duncan Laurie has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32089 )

Change subject: soc/intel/cannonlake: Update CPU Ratio base on MSR
......................................................................

soc/intel/cannonlake: Update CPU Ratio base on MSR

The following is the FSP logic: as long as the Cpu Ratio input in
coreboot is different with CpuStrapSet, system will force to follow
input from coreboot. But CpuStrapsetting is floating, it will be 0
from the first cold boot before memory training and set to 0x1c (or
max CPU ratio for the installed CPU) after first memory training.

The previous fix was attempting to ensure settings were cleared
when FSP was called in recovery mode, but only when coming from S5
which caused issues if recovery mode is requested by the OS and
is only followed by a warm reset.

BUG=b:129412691
TEST=Boot up sarien platform and force recovery, check there's no reset
in the path of recovery.

Signed-off-by: Lijian Zhao 
Change-Id: I959188be46343bc6f2cb3cc149097b4d449802aa
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32089
Reviewed-by: Furquan Shaikh 
Reviewed-by: Duncan Laurie 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/cannonlake/romstage/fsp_params.c
1 file changed, 6 insertions(+), 13 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Duncan Laurie: Looks good to me, approved
  Furquan Shaikh: Looks good to me, approved

diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c
index 3e3aa5e..ffdcee4 100644
--- a/src/soc/intel/cannonlake/romstage/fsp_params.c
+++ b/src/soc/intel/cannonlake/romstage/fsp_params.c
@@ -15,10 +15,12 @@

#include <assert.h>
 #include <chip.h>
+#include <cpu/x86/msr.h>
 #include <console/console.h>
 #include <fsp/util.h>
 #include <intelblocks/pmclib.h>
 #include <soc/iomap.h>
+#include <soc/msr.h>
 #include <soc/pci_devs.h>
 #include <soc/romstage.h>
 #include <vendorcode/google/chromeos/chromeos.h>
@@ -69,19 +71,10 @@
    m_cfg->SkipMpInit = !CONFIG_USE_INTEL_FSP_MP_INIT;
 #endif

-	/* Disable CPU Flex Ratio and SaGv in recovery mode */
-	if (vboot_recovery_mode_enabled()) {
-		struct chipset_power_state *ps = pmc_get_power_state();
-
-		/*
-		 * Only disable when coming from S5 (cold reset) otherwise
-		 * the flex ratio may be locked and FSP will return an error.
-		 */
-		if (ps && ps->prev_sleep_state == ACPI_S5) {
-			m_cfg->CpuRatio = 0;
-			m_cfg->SaGv = 0;
-		}
-	}
+	/* Set CpuRatio to match existing MSR value */
+	msr_t flex_ratio;
+	flex_ratio = rdmsr(MSR_FLEX_RATIO);
+	m_cfg->CpuRatio = (flex_ratio.lo >> 8) & 0xff;

/* If ISH is enabled, enable ISH elements */
    if (!dev)