Change in coreboot[master]: src/cpu/intel: Add sanity check for cpu turbo mode capability

[John Zhao (Code Review)]

Hello John Zhao,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

src/cpu/intel: Add sanity check for cpu turbo mode capability

It is properly to check cpu turbo mode capability after it is intended
for enabling. If processor exhibits the presence of hardware support for
turbo, turbo global state will be updated with TURBO_ENABLE. Otherwise,
TURBO_UNAVAILABLE is applied to turbo global state.

Change-Id: Ib1bc37fb339b4a0bb6a7cdc6cd4391575b22b55a
Signed-off-by: John Zhao 
---
M src/cpu/intel/turbo/turbo.c
1 file changed, 16 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34145/1

diff --git a/src/cpu/intel/turbo/turbo.c b/src/cpu/intel/turbo/turbo.c
index 12cbfc0..e71c858 100644
--- a/src/cpu/intel/turbo/turbo.c
+++ b/src/cpu/intel/turbo/turbo.c
@@ -92,7 +92,9 @@
  */
 void enable_turbo(void)
 {
+	struct cpuid_result cpuid_regs;
    msr_t msr;
+	int turbo_cap, turbo_state;

/* Only possible if turbo is available but hidden */
    if (get_turbo_state() == TURBO_DISABLED) {
@@ -101,9 +103,21 @@
    	msr.hi &= ~H_MISC_DISABLE_TURBO;
    	wrmsr(IA32_MISC_ENABLE, msr);

+		cpuid_regs = cpuid(CPUID_LEAF_PM);
+		turbo_cap = !!(cpuid_regs.eax & PM_CAP_TURBO_MODE);
+
+		if (!turbo_cap) {
+			/* Unavailable */
+			turbo_state = TURBO_UNAVAILABLE;
+			printk(BIOS_INFO, "Turbo is unavailable\n");
+		} else {
+			/* Available */
+			turbo_state = TURBO_ENABLED;
+			printk(BIOS_INFO, "Turbo has been enabled\n");
+		}
+
    	/* Update cached turbo state */
-		set_global_turbo_state(TURBO_ENABLED);
-		printk(BIOS_INFO, "Turbo has been enabled\n");
+		set_global_turbo_state(turbo_state);
    }
 }

[John Zhao (Code Review)]

Hello Thejaswani Putta, Patrick Rudolph, Bernardo Perez Priego, John Zhao, build bot (Jenkins),

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

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

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

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

src/cpu/intel: Add sanity check for cpu turbo mode capability

It is properly to check cpu turbo mode capability after it is intended
for enabling. If processor exhibits the presence of hardware support for
turbo, turbo global state will be updated with TURBO_ENABLE. Otherwise,
TURBO_UNAVAILABLE is applied to turbo global state.

TEST=Built and boot up to kernel.

Change-Id: Ib1bc37fb339b4a0bb6a7cdc6cd4391575b22b55a
Signed-off-by: John Zhao 
---
M src/cpu/intel/turbo/turbo.c
1 file changed, 18 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34145/2

[John Zhao (Code Review)]

Hello Thejaswani Putta, Patrick Rudolph, Bernardo Perez Priego, John Zhao, build bot (Jenkins),

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

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

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

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

src/cpu/intel: Add sanity check for cpu turbo mode capability

It is properly to check cpu turbo mode capability after it is intended
for enabling. If processor exhibits the presence of hardware support for
turbo, turbo global state will be updated with TURBO_ENABLE. Otherwise,
TURBO_UNAVAILABLE is applied to turbo global state.

TEST=Built and boot up to kernel.

Change-Id: Ib1bc37fb339b4a0bb6a7cdc6cd4391575b22b55a
Signed-off-by: John Zhao 
---
M src/cpu/intel/turbo/turbo.c
1 file changed, 21 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34145/3

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34145/3/src/cpu/intel/turbo/turbo.c 
File src/cpu/intel/turbo/turbo.c:

https://review.coreboot.org/c/coreboot/+/34145/3/src/cpu/intel/turbo/turbo.c... 
PS3, Line 120: 	else {
else should follow close brace '}'

https://review.coreboot.org/c/coreboot/+/34145/3/src/cpu/intel/turbo/turbo.c... 
PS3, Line 120: 	else {
braces {} are not necessary for single statement blocks

[John Zhao (Code Review)]

Hello Thejaswani Putta, Patrick Rudolph, Bernardo Perez Priego, John Zhao, build bot (Jenkins),

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

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

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

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

src/cpu/intel: Add sanity check for cpu turbo mode capability

It is properly to check cpu turbo mode capability after it is intended
for enabling. If processor exhibits the presence of hardware support for
turbo, turbo global state will be updated with TURBO_ENABLE. Otherwise,
TURBO_UNAVAILABLE is applied to turbo global state.

TEST=Built and boot up to kernel.

Change-Id: Ib1bc37fb339b4a0bb6a7cdc6cd4391575b22b55a
Signed-off-by: John Zhao 
---
M src/cpu/intel/turbo/turbo.c
1 file changed, 20 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34145/4

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34145/3/src/cpu/intel/turbo/turbo.c 
File src/cpu/intel/turbo/turbo.c:

https://review.coreboot.org/c/coreboot/+/34145/3/src/cpu/intel/turbo/turbo.c... 
PS3, Line 120: 	else {
> else should follow close brace '}'

Ack

https://review.coreboot.org/c/coreboot/+/34145/3/src/cpu/intel/turbo/turbo.c... 
PS3, Line 120: 	else {
> braces {} are not necessary for single statement blocks

Ack

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34145/4/src/cpu/intel/turbo/turbo.c 
File src/cpu/intel/turbo/turbo.c:

https://review.coreboot.org/c/coreboot/+/34145/4/src/cpu/intel/turbo/turbo.c... 
PS4, Line 120: 	else
else should follow close brace '}'

[John Zhao (Code Review)]

Hello Thejaswani Putta, Patrick Rudolph, Bernardo Perez Priego, John Zhao, build bot (Jenkins),

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

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

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

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

src/cpu/intel: Add sanity check for cpu turbo mode capability

It is properly to check cpu turbo mode capability after it is intended
for enabling. If processor exhibits the presence of hardware support for
turbo, turbo global state will be updated with TURBO_ENABLE. Otherwise,
TURBO_UNAVAILABLE is applied to turbo global state.

TEST=Built and boot up to kernel.

Change-Id: Ib1bc37fb339b4a0bb6a7cdc6cd4391575b22b55a
Signed-off-by: John Zhao 
---
M src/cpu/intel/turbo/turbo.c
1 file changed, 20 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34145/5

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34145/4/src/cpu/intel/turbo/turbo.c 
File src/cpu/intel/turbo/turbo.c:

https://review.coreboot.org/c/coreboot/+/34145/4/src/cpu/intel/turbo/turbo.c... 
PS4, Line 120: 	else
> else should follow close brace '}'

Ack

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 5:

+Patrick, did current build bot already implement the latest coding style suggestion to use braces even for single statement?

[Bernardo Perez Priego (Code Review)]

Bernardo Perez Priego has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 5: Code-Review+1

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 5: Code-Review+2

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 5:

(2 comments)

Sorry, just some minor changes to the commit message.

https://review.coreboot.org/c/coreboot/+/34145/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34145/5//COMMIT_MSG@9 
PS5, Line 9: properly
proper

https://review.coreboot.org/c/coreboot/+/34145/5//COMMIT_MSG@9 
PS5, Line 9: intended
           : for enabling. 
selected to be enabled

[Furquan Shaikh (Code Review)]

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

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 5:

(3 comments)

https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c 
File src/cpu/intel/turbo/turbo.c:

https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c... 
PS5, Line 107: cpuid_regs = cpuid(CPUID_LEAF_PM);
             : 		turbo_cap = !!(cpuid_regs.eax & PM_CAP_TURBO_MODE);
Why is this function checking cap again since get_turbo_state() already does that?

https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c... 
PS5, Line 112: turbo_state = TURBO_UNAVAILABLE;
get_turbo_state() in line 101 ensures that turbo_state is not TURBO_UNAVAILABLE. Isn't this check redundant?

https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c... 
PS5, Line 120: turbo_state = get_global_turbo_state();
Else case doesn't make much sense. It gets global state to turbo_state and writes back turbo_state to global state in the next line?

[John Zhao (Code Review)]

John Zhao has uploaded a new patch set (#6) to the change originally created by John Zhao. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

src/cpu/intel: Add sanity check for cpu turbo mode capability

It is proper to check cpu turbo mode capability after it is selected
to be enabled. If processor exhibits the presence of hardware support for
turbo, turbo global state will be updated with TURBO_ENABLE. Otherwise,
TURBO_UNAVAILABLE is applied to turbo global state.

TEST=Built and boot up to kernel.

Change-Id: Ib1bc37fb339b4a0bb6a7cdc6cd4391575b22b55a
Signed-off-by: John Zhao 
---
M src/cpu/intel/turbo/turbo.c
1 file changed, 17 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34145/6

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 6:

(5 comments)

https://review.coreboot.org/c/coreboot/+/34145/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34145/5//COMMIT_MSG@9 
PS5, Line 9: properly
> proper

Ack

https://review.coreboot.org/c/coreboot/+/34145/5//COMMIT_MSG@9 
PS5, Line 9: intended
           : for enabling.
> selected to be enabled

Ack

https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c 
File src/cpu/intel/turbo/turbo.c:

https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c... 
PS5, Line 107: cpuid_regs = cpuid(CPUID_LEAF_PM);
             : 		turbo_cap = !!(cpuid_regs.eax & PM_CAP_TURBO_MODE);
> Why is this function checking cap again since get_turbo_state() already does that?

It is intended if get_turbo_state() results in TURBO_DISABLED as "if (!turbo_cap && !turbo_en)" scenario. enable_turbo() then intends to enable turbo and check its status.

https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c... 
PS5, Line 112: turbo_state = TURBO_UNAVAILABLE;
> get_turbo_state() in line 101 ensures that turbo_state is not TURBO_UNAVAILABLE. […]

If get_turbo_state() in line 101 ensures that turbo_state is NOT TURBO_UNAVAILABLE, get_turbo_state then updates turbo_state to either TURBO_ENABLED (fsp already enabled turbo) or TURBO_DISABLED (fsp does not enable turbo). Only in the case of TURBO_DISABLED, enable_turbo() tries to enable turbo, checks its status and updates turbo_state properly.

https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c... 
PS5, Line 120: turbo_state = get_global_turbo_state();
> Else case doesn't make much sense. […]

Ack

[Furquan Shaikh (Code Review)]

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

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 6:

(4 comments)

https://review.coreboot.org/c/coreboot/+/34145/6/src/cpu/intel/turbo/turbo.c 
File src/cpu/intel/turbo/turbo.c:

https://review.coreboot.org/c/coreboot/+/34145/6/src/cpu/intel/turbo/turbo.c... 
PS6, Line 57: int get_turbo_state(void)
I am wondering if we should add a function:

void update_turbo_state(void)
{

}

This function basically does the same things as lines 68-86 of this file.

And then have:

int get_turbo_state(void)
{
    if (turbo_state == TURBO_UNKNOWN)
         update_turbo_state();

return turbo_state;
}

https://review.coreboot.org/c/coreboot/+/34145/6/src/cpu/intel/turbo/turbo.c... 
PS6, Line 107: cpuid_regs = cpuid(CPUID_LEAF_PM);
             : 		turbo_cap = !!(cpuid_regs.eax & PM_CAP_TURBO_MODE);
             : 
             : 		if (!turbo_cap) {
             : 			/* Unavailable */
             : 			turbo_state = TURBO_UNAVAILABLE;
             : 			printk(BIOS_INFO, "Turbo is unavailable\n");
             : 		} else {
             : 			/* Available */
             : 			turbo_state = TURBO_ENABLED;
             : 			printk(BIOS_INFO, "Turbo has been enabled\n");
             : 		}
             : 
             : 		/* Update cached turbo state */
             : 		set_global_turbo_state(turbo_state);
And then all of this can be changed to:
update_turbo_state();

https://review.coreboot.org/c/coreboot/+/34145/6/src/cpu/intel/turbo/turbo.c... 
PS6, Line 138: TURBO_UNAVAILABLE
Shouldn't this be TURBO_DISABLED?

https://review.coreboot.org/c/coreboot/+/34145/6/src/cpu/intel/turbo/turbo.c... 
PS6, Line 137: /* Update cached turbo state */
             : 	set_global_turbo_state(TURBO_UNAVAILABLE);
             : 	printk(BIOS_INFO, "Turbo has been disabled\n");
And this can be changed to do the same i.e. update_turbo_state();

[John Zhao (Code Review)]

John Zhao has uploaded a new patch set (#7) to the change originally created by John Zhao. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

src/cpu/intel: Add sanity check for cpu turbo mode capability

It is proper to check cpu turbo mode capability after it is selected
to be enabled. If processor exhibits the presence of hardware support for
turbo, turbo global state will be updated with TURBO_ENABLE. Otherwise,
TURBO_UNAVAILABLE is applied to turbo global state.

TEST=Built and boot up to kernel.

Change-Id: Ib1bc37fb339b4a0bb6a7cdc6cd4391575b22b55a
Signed-off-by: John Zhao 
---
M src/cpu/intel/turbo/turbo.c
M src/include/cpu/intel/turbo.h
2 files changed, 29 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34145/7

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 7:

(4 comments)

https://review.coreboot.org/c/coreboot/+/34145/6/src/cpu/intel/turbo/turbo.c 
File src/cpu/intel/turbo/turbo.c:

https://review.coreboot.org/c/coreboot/+/34145/6/src/cpu/intel/turbo/turbo.c... 
PS6, Line 57: int get_turbo_state(void)
> I am wondering if we should add a function: […]

Done

https://review.coreboot.org/c/coreboot/+/34145/6/src/cpu/intel/turbo/turbo.c... 
PS6, Line 107: cpuid_regs = cpuid(CPUID_LEAF_PM);
             : 		turbo_cap = !!(cpuid_regs.eax & PM_CAP_TURBO_MODE);
             : 
             : 		if (!turbo_cap) {
             : 			/* Unavailable */
             : 			turbo_state = TURBO_UNAVAILABLE;
             : 			printk(BIOS_INFO, "Turbo is unavailable\n");
             : 		} else {
             : 			/* Available */
             : 			turbo_state = TURBO_ENABLED;
             : 			printk(BIOS_INFO, "Turbo has been enabled\n");
             : 		}
             : 
             : 		/* Update cached turbo state */
             : 		set_global_turbo_state(turbo_state);
> And then all of this can be changed to: […]

Done

https://review.coreboot.org/c/coreboot/+/34145/6/src/cpu/intel/turbo/turbo.c... 
PS6, Line 138: TURBO_UNAVAILABLE
> Shouldn't this be TURBO_DISABLED?

Done

https://review.coreboot.org/c/coreboot/+/34145/6/src/cpu/intel/turbo/turbo.c... 
PS6, Line 137: /* Update cached turbo state */
             : 	set_global_turbo_state(TURBO_UNAVAILABLE);
             : 	printk(BIOS_INFO, "Turbo has been disabled\n");
> And this can be changed to do the same i.e. […]

Done

[John Zhao (Code Review)]

John Zhao has uploaded a new patch set (#8) to the change originally created by John Zhao. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

src/cpu/intel: Add sanity check for cpu turbo mode capability

It is proper to check cpu turbo mode capability after it is selected
to be enabled. If processor exhibits the presence of hardware support for
turbo, turbo global state will be updated with TURBO_ENABLE. Otherwise,
TURBO_UNAVAILABLE is applied to turbo global state.

TEST=Built and boot up to kernel.

Change-Id: Ib1bc37fb339b4a0bb6a7cdc6cd4391575b22b55a
Signed-off-by: John Zhao 
---
M src/cpu/intel/turbo/turbo.c
M src/include/cpu/intel/turbo.h
2 files changed, 30 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34145/8

[Furquan Shaikh (Code Review)]

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

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 8:

(4 comments)

https://review.coreboot.org/c/coreboot/+/34145/8/src/cpu/intel/turbo/turbo.c 
File src/cpu/intel/turbo/turbo.c:

https://review.coreboot.org/c/coreboot/+/34145/8/src/cpu/intel/turbo/turbo.c... 
PS8, Line 55: void
This can just return the new turbo_state.

https://review.coreboot.org/c/coreboot/+/34145/8/src/cpu/intel/turbo/turbo.c... 
PS8, Line 94: update_turbo_state
... then this can be:
turbo_state = update_turbo_state();

https://review.coreboot.org/c/coreboot/+/34145/8/src/cpu/intel/turbo/turbo.c... 
PS8, Line 96: turbo_state
Else, this will return stale turbo_state when it is TURBO_UNKNOWN.

https://review.coreboot.org/c/coreboot/+/34145/8/src/include/cpu/intel/turbo... 
File src/include/cpu/intel/turbo.h:

https://review.coreboot.org/c/coreboot/+/34145/8/src/include/cpu/intel/turbo... 
PS8, Line 44: /* Update turbo state */
            : void update_turbo_state(void);
Do we need to expose this? Currently, there are no users. I think its better to just keep it static.

[John Zhao (Code Review)]

John Zhao has uploaded a new patch set (#9) to the change originally created by John Zhao. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

src/cpu/intel: Add sanity check for cpu turbo mode capability

It is proper to check cpu turbo mode capability after it is selected
to be enabled. If processor exhibits the presence of hardware support for
turbo, turbo global state will be updated with TURBO_ENABLE. Otherwise,
TURBO_UNAVAILABLE is applied to turbo global state.

TEST=Built and boot up to kernel.

Change-Id: Ib1bc37fb339b4a0bb6a7cdc6cd4391575b22b55a
Signed-off-by: John Zhao 
---
M src/cpu/intel/turbo/turbo.c
1 file changed, 29 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34145/9

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 9:

(4 comments)

https://review.coreboot.org/c/coreboot/+/34145/8/src/cpu/intel/turbo/turbo.c 
File src/cpu/intel/turbo/turbo.c:

https://review.coreboot.org/c/coreboot/+/34145/8/src/cpu/intel/turbo/turbo.c... 
PS8, Line 55: void
> This can just return the new turbo_state.

Done

https://review.coreboot.org/c/coreboot/+/34145/8/src/cpu/intel/turbo/turbo.c... 
PS8, Line 94: update_turbo_state
> ... then this can be: […]

Done

https://review.coreboot.org/c/coreboot/+/34145/8/src/cpu/intel/turbo/turbo.c... 
PS8, Line 96: turbo_state
> Else, this will return stale turbo_state when it is TURBO_UNKNOWN.

Done

https://review.coreboot.org/c/coreboot/+/34145/8/src/include/cpu/intel/turbo... 
File src/include/cpu/intel/turbo.h:

https://review.coreboot.org/c/coreboot/+/34145/8/src/include/cpu/intel/turbo... 
PS8, Line 44: /* Update turbo state */
            : void update_turbo_state(void);
> Do we need to expose this? Currently, there are no users. I think its better to just keep it static.

Done

[Furquan Shaikh (Code Review)]

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

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 9: Code-Review+2

[Paul Menzel (Code Review)]

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

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 9:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34145/9//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34145/9//COMMIT_MSG@14 
PS9, Line 14: TEST=Built and boot up to kernel.
On what device?

https://review.coreboot.org/c/coreboot/+/34145/9/src/cpu/intel/turbo/turbo.c 
File src/cpu/intel/turbo/turbo.c:

https://review.coreboot.org/c/coreboot/+/34145/9/src/cpu/intel/turbo/turbo.c... 
PS9, Line 86:  * Determine the current state of Turbo and cache it for later.
            :  * Turbo is a package level config so it does not need to be
            :  * enabled on every core.
Please use the full text width.

[John Zhao (Code Review)]

John Zhao has uploaded a new patch set (#10) to the change originally created by John Zhao. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

src/cpu/intel: Add sanity check for cpu turbo mode capability

It is proper to check cpu turbo mode capability after it is selected
to be enabled. If processor exhibits the presence of hardware support for
turbo, turbo global state will be updated with TURBO_ENABLE. Otherwise,
TURBO_UNAVAILABLE is applied to turbo global state.

TEST=Validated turbo state on GLK and WHL devices.

Change-Id: Ib1bc37fb339b4a0bb6a7cdc6cd4391575b22b55a
Signed-off-by: John Zhao 
---
M src/cpu/intel/turbo/turbo.c
1 file changed, 28 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34145/10

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 10:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34145/9//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34145/9//COMMIT_MSG@14 
PS9, Line 14: TEST=Built and boot up to kernel.
> On what device?

Done

https://review.coreboot.org/c/coreboot/+/34145/9/src/cpu/intel/turbo/turbo.c 
File src/cpu/intel/turbo/turbo.c:

https://review.coreboot.org/c/coreboot/+/34145/9/src/cpu/intel/turbo/turbo.c... 
PS9, Line 86:  * Determine the current state of Turbo and cache it for later.
            :  * Turbo is a package level config so it does not need to be
            :  * enabled on every core.
> Please use the full text width.

Done

[Furquan Shaikh (Code Review)]

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

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 10: Code-Review+2

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

Patch Set 10:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c 
File src/cpu/intel/turbo/turbo.c:

https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c... 
PS5, Line 107: cpuid_regs = cpuid(CPUID_LEAF_PM);
             : 		turbo_cap = !!(cpuid_regs.eax & PM_CAP_TURBO_MODE);
> It is intended if get_turbo_state() results in TURBO_DISABLED as "if (!turbo_cap && !turbo_en)" scen […]

Done

https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c... 
PS5, Line 112: turbo_state = TURBO_UNAVAILABLE;
> If get_turbo_state() in line 101 ensures that turbo_state is NOT TURBO_UNAVAILABLE, get_turbo_state  […]

Done

[Martin Roth (Code Review)]

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34145 )

Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability
......................................................................

src/cpu/intel: Add sanity check for cpu turbo mode capability

It is proper to check cpu turbo mode capability after it is selected
to be enabled. If processor exhibits the presence of hardware support for
turbo, turbo global state will be updated with TURBO_ENABLE. Otherwise,
TURBO_UNAVAILABLE is applied to turbo global state.

TEST=Validated turbo state on GLK and WHL devices.

Change-Id: Ib1bc37fb339b4a0bb6a7cdc6cd4391575b22b55a
Signed-off-by: John Zhao 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34145
Reviewed-by: Furquan Shaikh 
Tested-by: build bot (Jenkins) 
---
M src/cpu/intel/turbo/turbo.c
1 file changed, 28 insertions(+), 17 deletions(-)

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

diff --git a/src/cpu/intel/turbo/turbo.c b/src/cpu/intel/turbo/turbo.c
index 12cbfc0..ae97f9a 100644
--- a/src/cpu/intel/turbo/turbo.c
+++ b/src/cpu/intel/turbo/turbo.c
@@ -50,21 +50,15 @@
 };

/*
- * Determine the current state of Turbo and cache it for later.
- * Turbo is a package level config so it does not need to be
- * enabled on every core.
+ * Try to update the global Turbo state.
  */
-int get_turbo_state(void)
+static int update_turbo_state(void)
 {
    struct cpuid_result cpuid_regs;
    int turbo_en, turbo_cap;
    msr_t msr;
    int turbo_state = get_global_turbo_state();

-	/* Return cached state if available */
-	if (turbo_state != TURBO_UNKNOWN)
-		return turbo_state;
-
    cpuid_regs = cpuid(CPUID_LEAF_PM);
    turbo_cap = !!(cpuid_regs.eax & PM_CAP_TURBO_MODE);

@@ -84,6 +78,22 @@

set_global_turbo_state(turbo_state);
    printk(BIOS_INFO, "Turbo is %s\n", turbo_state_desc[turbo_state]);
+
+	return turbo_state;
+}
+
+/*
+ * Determine the current state of Turbo and cache it for later. Turbo is package
+ * level config so it does not need to be enabled on every core.
+ */
+int get_turbo_state(void)
+{
+	int turbo_state = get_global_turbo_state();
+
+	/* Return cached state if available */
+	if (turbo_state == TURBO_UNKNOWN)
+		turbo_state = update_turbo_state();
+
    return turbo_state;
 }

@@ -102,8 +112,7 @@
    	wrmsr(IA32_MISC_ENABLE, msr);

/* Update cached turbo state */
-		set_global_turbo_state(TURBO_ENABLED);
-		printk(BIOS_INFO, "Turbo has been enabled\n");
+		update_turbo_state();
    }
 }

@@ -114,12 +123,14 @@
 {
    msr_t msr;

-	/* Set Turbo Disable bit in Misc Enables */
-	msr = rdmsr(IA32_MISC_ENABLE);
-	msr.hi |= H_MISC_DISABLE_TURBO;
-	wrmsr(IA32_MISC_ENABLE, msr);
+	/* Only possible if turbo is available and visible */
+	if (get_turbo_state() == TURBO_ENABLED) {
+		/* Set Turbo Disable bit in Misc Enables */
+		msr = rdmsr(IA32_MISC_ENABLE);
+		msr.hi |= H_MISC_DISABLE_TURBO;
+		wrmsr(IA32_MISC_ENABLE, msr);

-	/* Update cached turbo state */
-	set_global_turbo_state(TURBO_UNAVAILABLE);
-	printk(BIOS_INFO, "Turbo has been disabled\n");
+		/* Update cached turbo state */
+		update_turbo_state();
+	}
 }