Change in coreboot[master]: soc/intel/tigerlake: Configure Hyperthreading

[Wonkyu Kim (Code Review)]

Wonkyu Kim has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39683 )

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

soc/intel/tigerlake: Configure Hyperthreading

Configure Hyperthreading based on devicetree

BUG=none
TEST= Build and boot with FSP log and check Hyperthread setting

Signed-off-by: Wonkyu Kim 
Change-Id: Idc94e6b8ecd59a43be60bf60dc7dd0811ac0350b
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c
2 files changed, 3 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/39683/1

diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h
index f82f13d..7bee20c 100644
--- a/src/soc/intel/tigerlake/chip.h
+++ b/src/soc/intel/tigerlake/chip.h
@@ -292,6 +292,8 @@
     */
    uint8_t cpu_ratio_override;

+	/* HyperThreading : enable (1) / disable (0) */
+	uint8_t HyperThreading;
 };

typedef struct soc_intel_tigerlake_config config_t;
diff --git a/src/soc/intel/tigerlake/romstage/fsp_params_tgl.c b/src/soc/intel/tigerlake/romstage/fsp_params_tgl.c
index 95f637e..c762d3d 100644
--- a/src/soc/intel/tigerlake/romstage/fsp_params_tgl.c
+++ b/src/soc/intel/tigerlake/romstage/fsp_params_tgl.c
@@ -144,7 +144,7 @@
    	m_cfg->TcssItbtPcie3En = 0;

/* Enable Hyper Threading */
-	m_cfg->HyperThreading = 1;
+	m_cfg->HyperThreading = config->HyperThreading;
    /* Disable Lock PCU Thermal Management registers */
    m_cfg->LockPTMregs = 0;
    /* Channel Hash Mask:0x0001=BIT6 set(Minimal), 0x3FFF=BIT[19:6] set(Maximum) */

[Wonkyu Kim (Code Review)]

Hello Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

soc/intel/tigerlake: Configure Hyperthreading

Configure Hyperthreading based on devicetree

BUG=none
TEST=Build and boot up with FSP log enabled and check Hyperthread setting
from FSP log.

Signed-off-by: Wonkyu Kim 
Change-Id: Idc94e6b8ecd59a43be60bf60dc7dd0811ac0350b
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c
2 files changed, 3 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/39683/2

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39683 )

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

Patch Set 2: Code-Review+1

[Srinidhi N Kaushik (Code Review)]

Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39683 )

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

Patch Set 2: Code-Review+2

[Wonkyu Kim (Code Review)]

Hello build bot (Jenkins), Shaunak Saha, Caveh Jalali, Ravishankar Sarawadi, Nick Vaccaro, Furquan Shaikh, Srinidhi N Kaushik, Raj Astekar, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

soc/intel/tigerlake: Configure Hyperthreading

Configure Hyperthreading based on devicetree

BUG=none
TEST= Build and boot with FSP log and check Hyperthread setting

Signed-off-by: Wonkyu Kim 
Change-Id: Idc94e6b8ecd59a43be60bf60dc7dd0811ac0350b
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c
2 files changed, 4 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/39683/3

[Caveh Jalali (Code Review)]

Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39683 )

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

Patch Set 3: Code-Review+1

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39683/3/src/soc/intel/tigerlake/chi... 
File src/soc/intel/tigerlake/chip.h:

https://review.coreboot.org/c/coreboot/+/39683/3/src/soc/intel/tigerlake/chi... 
PS3, Line 296: HyperThreading
I think it might be better to have a negative option i.e. HyperThreadingDisable. In general, I believe most mainboards would want to keep this enabled by default.

[Wonkyu Kim (Code Review)]

Hello build bot (Jenkins), Shaunak Saha, Caveh Jalali, Ravishankar Sarawadi, Nick Vaccaro, Furquan Shaikh, Srinidhi N Kaushik, Raj Astekar, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

soc/intel/tigerlake: Configure Hyperthreading

Configure Hyperthreading based on Kconfig

BUG=none
TEST= Build and boot with FSP log and check Hyperthread setting

Signed-off-by: Wonkyu Kim 
Change-Id: Idc94e6b8ecd59a43be60bf60dc7dd0811ac0350b
---
M src/soc/intel/tigerlake/Kconfig
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c
3 files changed, 9 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/39683/4

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39683 )

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39683/3/src/soc/intel/tigerlake/chi... 
File src/soc/intel/tigerlake/chip.h:

https://review.coreboot.org/c/coreboot/+/39683/3/src/soc/intel/tigerlake/chi... 
PS3, Line 296: HyperThreading
> I think it might be better to have a negative option i.e. HyperThreadingDisable. […]

yes.  I think it's better use Kconfig file rather than config variable.

[Wonkyu Kim (Code Review)]

Hello build bot (Jenkins), Shaunak Saha, Caveh Jalali, Ravishankar Sarawadi, Nick Vaccaro, Furquan Shaikh, Srinidhi N Kaushik, Raj Astekar, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

soc/intel/tigerlake: Configure Hyperthreading

Configure Hyperthreading based on Kconfig

BUG=none
TEST= Build and boot with FSP log and check Hyperthread setting

Signed-off-by: Wonkyu Kim 
Change-Id: Idc94e6b8ecd59a43be60bf60dc7dd0811ac0350b
---
M src/soc/intel/tigerlake/Kconfig
M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c
2 files changed, 7 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/39683/5

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39683/3/src/soc/intel/tigerlake/chi... 
File src/soc/intel/tigerlake/chip.h:

https://review.coreboot.org/c/coreboot/+/39683/3/src/soc/intel/tigerlake/chi... 
PS3, Line 296: HyperThreading
> yes.  I think it's better use Kconfig file rather than config variable.

I did not mean using a Kconfig. Basically, I meant changing this option to be inverted i.e.
uint8_t HyperThreadingDisable;

I don't think you need a Kconfig for this.

[Paul Menzel (Code Review)]

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

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

Patch Set 5:

Why is this Tiger Lake specific, and not common code?

[Wonkyu Kim (Code Review)]

Hello build bot (Jenkins), Shaunak Saha, Caveh Jalali, Ravishankar Sarawadi, Nick Vaccaro, Furquan Shaikh, Srinidhi N Kaushik, Raj Astekar, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

soc/intel/tigerlake: Configure Hyperthreading

Configure Hyperthreading based on devicetree

BUG=none
TEST= Build and boot with FSP log and check Hyperthread setting

Signed-off-by: Wonkyu Kim 
Change-Id: Idc94e6b8ecd59a43be60bf60dc7dd0811ac0350b
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c
2 files changed, 5 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/39683/6

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39683 )

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

Patch Set 6: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/39683/3/src/soc/intel/tigerlake/chi... 
File src/soc/intel/tigerlake/chip.h:

https://review.coreboot.org/c/coreboot/+/39683/3/src/soc/intel/tigerlake/chi... 
PS3, Line 296: HyperThreading
> I did not mean using a Kconfig. Basically, I meant changing this option to be inverted i.e. […]

Done

[Srinidhi N Kaushik (Code Review)]

Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39683 )

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

Patch Set 6: Code-Review+2

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

Patch Set 6: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/39683/6/src/soc/intel/tigerlake/rom... 
File src/soc/intel/tigerlake/romstage/fsp_params_tgl.c:

https://review.coreboot.org/c/coreboot/+/39683/6/src/soc/intel/tigerlake/rom... 
PS6, Line 147: config->HyperThreadingDisable ? 0 : 1
nit: !config->HyperThreadingDisable

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39683 )

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39683/6/src/soc/intel/tigerlake/rom... 
File src/soc/intel/tigerlake/romstage/fsp_params_tgl.c:

https://review.coreboot.org/c/coreboot/+/39683/6/src/soc/intel/tigerlake/rom... 
PS6, Line 147: config->HyperThreadingDisable ? 0 : 1
> nit: !config->HyperThreadingDisable

Current code also resolve the case which user provides non 0 or 1.
0 is disable Hyperthreading and non 0 is enabling Hyperthreading.

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39683 )

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39683/6/src/soc/intel/tigerlake/rom... 
File src/soc/intel/tigerlake/romstage/fsp_params_tgl.c:

https://review.coreboot.org/c/coreboot/+/39683/6/src/soc/intel/tigerlake/rom... 
PS6, Line 147: config->HyperThreadingDisable ? 0 : 1
> Current code also resolve the case which user provides non 0 or 1. […]

same for !val:

#include <stdio.h>

#define val(A) "!" #A " = %d\n", !A
void main() {
        printf(val(0));
        printf(val(1));
        printf(val(2));
        printf(val(-1));
}

!0 = 1
!1 = 0
!2 = 0
!-1 = 0

[Wonkyu Kim (Code Review)]

Hello build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Ravishankar Sarawadi, Nick Vaccaro, Furquan Shaikh, Srinidhi N Kaushik, Raj Astekar, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

soc/intel/tigerlake: Configure Hyperthreading

Configure Hyperthreading based on devicetree

BUG=none
TEST= Build and boot with FSP log and check Hyperthread setting

Signed-off-by: Wonkyu Kim 
Change-Id: Idc94e6b8ecd59a43be60bf60dc7dd0811ac0350b
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c
2 files changed, 5 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/39683/7

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39683 )

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

Patch Set 7: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/39683/6/src/soc/intel/tigerlake/rom... 
File src/soc/intel/tigerlake/romstage/fsp_params_tgl.c:

https://review.coreboot.org/c/coreboot/+/39683/6/src/soc/intel/tigerlake/rom... 
PS6, Line 147: config->HyperThreadingDisable ? 0 : 1
> same for !val: […]

Ack

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

Patch Set 7: Code-Review+2

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/tigerlake: Configure Hyperthreading
......................................................................

soc/intel/tigerlake: Configure Hyperthreading

Configure Hyperthreading based on devicetree

BUG=none
TEST= Build and boot with FSP log and check Hyperthread setting

Signed-off-by: Wonkyu Kim 
Change-Id: Idc94e6b8ecd59a43be60bf60dc7dd0811ac0350b
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39683
Reviewed-by: Furquan Shaikh 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c
2 files changed, 5 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Wonkyu Kim: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h
index f82f13d..1d4bd5f 100644
--- a/src/soc/intel/tigerlake/chip.h
+++ b/src/soc/intel/tigerlake/chip.h
@@ -292,6 +292,8 @@
     */
    uint8_t cpu_ratio_override;

+	/* HyperThreadingDisable : Yes (1) / No (0) */
+	uint8_t HyperThreadingDisable;
 };

typedef struct soc_intel_tigerlake_config config_t;
diff --git a/src/soc/intel/tigerlake/romstage/fsp_params_tgl.c b/src/soc/intel/tigerlake/romstage/fsp_params_tgl.c
index 95f637e..32f1b03 100644
--- a/src/soc/intel/tigerlake/romstage/fsp_params_tgl.c
+++ b/src/soc/intel/tigerlake/romstage/fsp_params_tgl.c
@@ -143,8 +143,9 @@
    else
    	m_cfg->TcssItbtPcie3En = 0;

-	/* Enable Hyper Threading */
-	m_cfg->HyperThreading = 1;
+	/* Hyper Threading */
+	m_cfg->HyperThreading = !config->HyperThreadingDisable;
+
    /* Disable Lock PCU Thermal Management registers */
    m_cfg->LockPTMregs = 0;
    /* Channel Hash Mask:0x0001=BIT6 set(Minimal), 0x3FFF=BIT[19:6] set(Maximum) */