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

[Wonkyu Kim (Code Review)]

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

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

soc/intel/tigerlake: Configure THC

Enable/Disable THCx though devicetree

BUG=None
BRANCH=None
TEST=Boot and check FSP log for THC setting

Signed-off-by: Wonkyu Kim 
Change-Id: If7683969161be67f68f441c28c80503de39079b5
---
M src/soc/intel/tigerlake/fsp_params.c
1 file changed, 13 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/41571/1

diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c
index 0c67105..1fb4e54 100644
--- a/src/soc/intel/tigerlake/fsp_params.c
+++ b/src/soc/intel/tigerlake/fsp_params.c
@@ -210,6 +210,19 @@
    else
    	params->CnviMode = 0;

+	/* THC */
+	dev = pcidev_path_on_root(PCH_DEVFN_THC0);
+	if(!dev)
+		params->ThcPort0Assignment = 0;
+	else
+		params->ThcPort0Assignment = dev->enabled;
+
+	dev =  pcidev_path_on_root(PCH_DEVFN_THC1);
+	if(!dev)
+		params->ThcPort1Assignment = 0;
+	else
+		params->ThcPort1Assignment = dev->enabled;
+
    /* Legacy 8254 timer support */
    params->Enable8254ClockGating = !CONFIG_USE_LEGACY_8254_TIMER;
    params->Enable8254ClockGatingOnS3 = !CONFIG_USE_LEGACY_8254_TIMER;

[build bot (Jenkins) (Code Review)]

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

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

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41571/1/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/41571/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 215: 	if(!dev)
space required before the open parenthesis '('

https://review.coreboot.org/c/coreboot/+/41571/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 221: 	if(!dev)
space required before the open parenthesis '('

[Wonkyu Kim (Code Review)]

Hello Patrick Rudolph,

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

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

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

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

soc/intel/tigerlake: Configure THC

Enable/Disable THCx though devicetree

BUG=None
BRANCH=None
TEST=Boot and check FSP log for THC setting

Signed-off-by: Wonkyu Kim 
Change-Id: If7683969161be67f68f441c28c80503de39079b5
---
M src/soc/intel/tigerlake/fsp_params.c
1 file changed, 13 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/41571/2

[build bot (Jenkins) (Code Review)]

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

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

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41571/2/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/41571/2/src/soc/intel/tigerlake/fsp... 
PS2, Line 221: 	ifi (!dev)
space prohibited between function name and open parenthesis '('

[Wonkyu Kim (Code Review)]

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

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

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41571/1/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/41571/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 215: 	if(!dev)
> space required before the open parenthesis '('

Done

https://review.coreboot.org/c/coreboot/+/41571/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 221: 	if(!dev)
> space required before the open parenthesis '('

Done

[Wonkyu Kim (Code Review)]

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

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

Patch Set 2: Code-Review+1

[Wonkyu Kim (Code Review)]

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

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

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

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

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

soc/intel/tigerlake: Configure THC

Enable/Disable THCx though devicetree

BUG=None
BRANCH=None
TEST=Boot and check FSP log for THC setting

Signed-off-by: Wonkyu Kim 
Change-Id: If7683969161be67f68f441c28c80503de39079b5
---
M src/soc/intel/tigerlake/fsp_params.c
1 file changed, 13 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/41571/3

[Wonkyu Kim (Code Review)]

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

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

Patch Set 3: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/41571/2/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/41571/2/src/soc/intel/tigerlake/fsp... 
PS2, Line 221: 	ifi (!dev)
> space prohibited between function name and open parenthesis '('

Done

[Tim Wawrzynczak (Code Review)]

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

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

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41571/3/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/41571/3/src/soc/intel/tigerlake/fsp... 
PS3, Line 223: 	else
             : 		params->ThcPort1Assignment = dev->enabled;
shouldn't this be:
`else if (dev->enabled)
         params->ThcPort1Assignment = 2`;

And the 2 should probably be a macro for ThcAssignmentThc1 (according to the UPD).

[Wonkyu Kim (Code Review)]

Hello build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Ravishankar Sarawadi, Dossym Nurmukhanov, Subrata Banik, Nick Vaccaro, Srinidhi N Kaushik, Patrick Rudolph,

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

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

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

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

soc/intel/tigerlake: Configure THC

Enable/Disable THCx though devicetree

BUG=None
BRANCH=None
TEST=Boot and check FSP log for THC setting

Signed-off-by: Wonkyu Kim 
Change-Id: If7683969161be67f68f441c28c80503de39079b5
---
M src/soc/intel/tigerlake/fsp_params.c
1 file changed, 13 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/41571/4

[Wonkyu Kim (Code Review)]

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

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

Patch Set 4: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/41571/3/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/41571/3/src/soc/intel/tigerlake/fsp... 
PS3, Line 223: 	else
             : 		params->ThcPort1Assignment = dev->enabled;
> shouldn't this be: […]

You're correct. Thanks for catching this.
ThcAssignmentThc0 should be ThcAssignmentThc0(1) and ThcAssignmentThc1 should be ThcAssignmentThc1(2) to be enabled.

[Wonkyu Kim (Code Review)]

Hello build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Ravishankar Sarawadi, Dossym Nurmukhanov, Subrata Banik, Nick Vaccaro, Srinidhi N Kaushik, Patrick Rudolph,

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

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

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

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

soc/intel/tigerlake: Configure THC

Enable/Disable THCx though devicetree

BUG=None
BRANCH=None
TEST=Boot and check FSP log for THC setting

Signed-off-by: Wonkyu Kim 
Change-Id: If7683969161be67f68f441c28c80503de39079b5
---
M src/soc/intel/tigerlake/fsp_params.c
1 file changed, 13 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/41571/5

[Wonkyu Kim (Code Review)]

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

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

Patch Set 5: Code-Review+1

Rebase for fixing merge conflict

[Tim Wawrzynczak (Code Review)]

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

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

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41571/5/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/41571/5/src/soc/intel/tigerlake/fsp... 
PS5, Line 235: 2
How about a symbolic constant for this?, something like
#define ThcAssignmentThc1 2 
?

[Wonkyu Kim (Code Review)]

Hello build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Ravishankar Sarawadi, Dossym Nurmukhanov, Subrata Banik, Nick Vaccaro, Srinidhi N Kaushik, Patrick Rudolph,

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

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

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

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

soc/intel/tigerlake: Configure THC

Enable/Disable THCx though devicetree

BUG=None
BRANCH=None
TEST=Boot and check FSP log for THC setting

Signed-off-by: Wonkyu Kim 
Change-Id: If7683969161be67f68f441c28c80503de39079b5
---
M src/soc/intel/tigerlake/fsp_params.c
1 file changed, 18 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/41571/6

[build bot (Jenkins) (Code Review)]

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

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

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41571/6/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/41571/6/src/soc/intel/tigerlake/fsp... 
PS6, Line 20: /* THC assignement definition */
'assignement' may be misspelled - perhaps 'assignment'?

[Wonkyu Kim (Code Review)]

Hello build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Ravishankar Sarawadi, Dossym Nurmukhanov, Subrata Banik, Nick Vaccaro, Srinidhi N Kaushik, Patrick Rudolph,

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

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

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

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

soc/intel/tigerlake: Configure THC

Enable/Disable THCx though devicetree

BUG=None
BRANCH=None
TEST=Boot and check FSP log for THC setting

Signed-off-by: Wonkyu Kim 
Change-Id: If7683969161be67f68f441c28c80503de39079b5
---
M src/soc/intel/tigerlake/fsp_params.c
1 file changed, 18 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/41571/7

[Wonkyu Kim (Code Review)]

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

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

Patch Set 7: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/41571/5/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/41571/5/src/soc/intel/tigerlake/fsp... 
PS5, Line 235: 2
> How about a symbolic constant for this?, something like […]

Done

https://review.coreboot.org/c/coreboot/+/41571/6/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/41571/6/src/soc/intel/tigerlake/fsp... 
PS6, Line 20: /* THC assignement definition */
> 'assignement' may be misspelled - perhaps 'assignment'?

Done

[Tim Wawrzynczak (Code Review)]

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

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

Patch Set 7: Code-Review+2

[Patrick Georgi (Code Review)]

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

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

soc/intel/tigerlake: Configure THC

Enable/Disable THCx though devicetree

BUG=None
BRANCH=None
TEST=Boot and check FSP log for THC setting

Signed-off-by: Wonkyu Kim 
Change-Id: If7683969161be67f68f441c28c80503de39079b5
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41571
Reviewed-by: Tim Wawrzynczak 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/tigerlake/fsp_params.c
1 file changed, 18 insertions(+), 0 deletions(-)

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

diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c
index 611a610..bdcd357 100644
--- a/src/soc/intel/tigerlake/fsp_params.c
+++ b/src/soc/intel/tigerlake/fsp_params.c
@@ -17,6 +17,11 @@
 #include <soc/soc_chip.h>
 #include <string.h>

+/* THC assignment definition */
+#define THC_NONE	0
+#define THC_0		1
+#define THC_1		2
+
 /*
  * Chip config parameter PcieRpL1Substates uses (UPD value + 1)
  * because UPD value of 0 for PcieRpL1Substates means disabled for FSP.
@@ -221,6 +226,19 @@
    else
    	params->VmdEnable = 0;

+	/* THC */
+	dev = pcidev_path_on_root(PCH_DEVFN_THC0);
+	if (!dev)
+		params->ThcPort0Assignment = 0;
+	else
+		params->ThcPort0Assignment = dev->enabled ? THC_0 : THC_NONE;
+
+	dev =  pcidev_path_on_root(PCH_DEVFN_THC1);
+	if (!dev)
+		params->ThcPort1Assignment = 0;
+	else
+		params->ThcPort1Assignment = dev->enabled ? THC_1 : THC_NONE;
+
    /* Legacy 8254 timer support */
    params->Enable8254ClockGating = !CONFIG_USE_LEGACY_8254_TIMER;
    params->Enable8254ClockGatingOnS3 = !CONFIG_USE_LEGACY_8254_TIMER;