Change in coreboot[master]: soc/intel/common/cpu: Don't set any TCC settings if offset is 0

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42879 )

Change subject: soc/intel/common/cpu: Don't set any TCC settings if offset is 0
......................................................................

soc/intel/common/cpu: Don't set any TCC settings if offset is 0

Many previous versions of this function would return early if tcc_offset
is 0. This adds that logic back in.

Change-Id: Ibc529520a4e74608cb5d20e5a6e8fc2c727c903c
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/common/block/cpu/cpulib.c
1 file changed, 6 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/42879/1

diff --git a/src/soc/intel/common/block/cpu/cpulib.c b/src/soc/intel/common/block/cpu/cpulib.c
index 0ac8dda..e4ab664 100644
--- a/src/soc/intel/common/block/cpu/cpulib.c
+++ b/src/soc/intel/common/block/cpu/cpulib.c
@@ -260,15 +260,20 @@
    const config_t *conf = config_of_soc();
    msr_t msr;

+	if (!conf->tcc_offset)
+		return;
+
    /* Set TCC activation offset */
    msr = rdmsr(MSR_PLATFORM_INFO);
-	if ((msr.lo & BIT(30)) && conf->tcc_offset) {
+	if ((msr.lo & BIT(30))) {
    	msr = rdmsr(MSR_TEMPERATURE_TARGET);
    	msr.lo &= ~(0xf << 24);
    	msr.lo |= (conf->tcc_offset & 0xf) << 24;
    	wrmsr(MSR_TEMPERATURE_TARGET, msr);
    }
+
    msr = rdmsr(MSR_TEMPERATURE_TARGET);
+
    /* Time Window Tau Bits [6:0] */
    msr.lo &= ~0x7f;
    msr.lo |= 0xe6; /* setting 100ms thermal time window */

[Mario Scheithauer (Code Review)]

Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42879 )

Change subject: soc/intel/common/cpu: Don't set any TCC settings if offset is 0
......................................................................

Patch Set 1: Code-Review+2

It is now like in the old function and our boards run again. The patch looks good to me. Thanks Tim!

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42879 )

Change subject: soc/intel/common/cpu: Don't set any TCC settings if offset is 0
......................................................................

Patch Set 1: Code-Review+1

[Sumeet R Pawnikar (Code Review)]

Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42879 )

Change subject: soc/intel/common/cpu: Don't set any TCC settings if offset is 0
......................................................................

Patch Set 1: Code-Review+2

Good to have this change.

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42879 )

Change subject: soc/intel/common/cpu: Don't set any TCC settings if offset is 0
......................................................................

soc/intel/common/cpu: Don't set any TCC settings if offset is 0

Many previous versions of this function would return early if tcc_offset
is 0. This adds that logic back in.

Change-Id: Ibc529520a4e74608cb5d20e5a6e8fc2c727c903c
Signed-off-by: Tim Wawrzynczak 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42879
Tested-by: build bot (Jenkins) 
Reviewed-by: Mario Scheithauer 
Reviewed-by: Christian Walter 
Reviewed-by: Sumeet R Pawnikar 
---
M src/soc/intel/common/block/cpu/cpulib.c
1 file changed, 6 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Mario Scheithauer: Looks good to me, approved
  Sumeet R Pawnikar: Looks good to me, approved
  Christian Walter: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/common/block/cpu/cpulib.c b/src/soc/intel/common/block/cpu/cpulib.c
index 0ac8dda..e4ab664 100644
--- a/src/soc/intel/common/block/cpu/cpulib.c
+++ b/src/soc/intel/common/block/cpu/cpulib.c
@@ -260,15 +260,20 @@
    const config_t *conf = config_of_soc();
    msr_t msr;

+	if (!conf->tcc_offset)
+		return;
+
    /* Set TCC activation offset */
    msr = rdmsr(MSR_PLATFORM_INFO);
-	if ((msr.lo & BIT(30)) && conf->tcc_offset) {
+	if ((msr.lo & BIT(30))) {
    	msr = rdmsr(MSR_TEMPERATURE_TARGET);
    	msr.lo &= ~(0xf << 24);
    	msr.lo |= (conf->tcc_offset & 0xf) << 24;
    	wrmsr(MSR_TEMPERATURE_TARGET, msr);
    }
+
    msr = rdmsr(MSR_TEMPERATURE_TARGET);
+
    /* Time Window Tau Bits [6:0] */
    msr.lo &= ~0x7f;
    msr.lo |= 0xe6; /* setting 100ms thermal time window */

[Werner Zeh (Code Review)]

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

Change subject: soc/intel/common/cpu: Don't set any TCC settings if offset is 0
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42879/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cpu/cpulib.c:

https://review.coreboot.org/c/coreboot/+/42879/2/src/soc/intel/common/block/... 
PS2, Line 277: /* Time Window Tau Bits [6:0] */
             : 	msr.lo &= ~0x7f;
             : 	msr.lo |= 0xe6; /* setting 100ms thermal time window */
             : 	wrmsr(MSR_TEMPERATURE_TARGET, msr);
If I have a look at the MSR documentation of Apollo Lake then I will find that bits 0..7 in MSR 0x1A2 are marked as reserved. With this code one could potentially still hit the error if for some reason tcc_offset is set in devicetree. I would rather see a platform differentiation choice here and execute this only on platforms that do support these bits (like Tiger Lake).

if (CONFIG(SOC_INTEL_TIGERLAKE) || CONFIG(WHAT_EVER_PLATFORM_SUPPORTS_THESE_BITS)) {
    msr = rdmsr(MSR_TEMPERATURE_TARGET);
    /* Time Window Tau Bits [6:0] */
    msr.lo &= ~0x7f;
    msr.lo |= 0xe6; /* setting 100ms thermal time window */
    wrmsr(MSR_TEMPERATURE_TARGET, msr);
}

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42879 )

Change subject: soc/intel/common/cpu: Don't set any TCC settings if offset is 0
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42879/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cpu/cpulib.c:

https://review.coreboot.org/c/coreboot/+/42879/2/src/soc/intel/common/block/... 
PS2, Line 277: /* Time Window Tau Bits [6:0] */
             : 	msr.lo &= ~0x7f;
             : 	msr.lo |= 0xe6; /* setting 100ms thermal time window */
             : 	wrmsr(MSR_TEMPERATURE_TARGET, msr);
> If I have a look at the MSR documentation of Apollo Lake then I will find that bits 0.. […]

That's fair. It looks like from cannonlake onwards, those bits are as expected here (time window bits). I'll supplement this with a guard to exclude APL and before, since from the EDSs I have, all SoC's after that appear to have the bits defined that way.