Change in coreboot[master]: soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU

[Sumeet R Pawnikar (Code Review)]

Sumeet R Pawnikar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43607 )

Change subject: soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU
......................................................................

soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU

Set power limits in devicetree for Tiger Lake Y-SKU varaints.

Change-Id: If4f1226473b48365e5962df9fff29910c99007fc
Signed-off-by: Sumeet R Pawnikar 
---
M src/mainboard/google/volteer/variants/baseboard/devicetree.cb
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/systemagent.c
3 files changed, 19 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/43607/1

diff --git a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb
index e0d3bea..6fdd7fc 100644
--- a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb
+++ b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb
@@ -221,6 +221,16 @@
    	.tdp_pl2_override = 38,
    	.tdp_pl4 = 71,
    }"
+	register "power_limits_config[POWER_LIMITS_Y_4_CORE]" = "{
+		.tdp_pl1_override = 9,
+		.tdp_pl2_override = 40,
+		.tdp_pl4 = 83,
+	}"
+	register "power_limits_config[POWER_LIMITS_Y_2_CORE]" = "{
+		.tdp_pl1_override = 9,
+		.tdp_pl2_override = 35,
+		.tdp_pl4 = 66,
+	}"

register "Device4Enable" = "1"

diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h
index 59dab58..403ad69 100644
--- a/src/soc/intel/tigerlake/chip.h
+++ b/src/soc/intel/tigerlake/chip.h
@@ -25,7 +25,9 @@
 /* The first two are for TGL-U */
 #define POWER_LIMITS_U_4_CORE	0
 #define POWER_LIMITS_U_2_CORE	1
-#define POWER_LIMITS_MAX	2
+#define POWER_LIMITS_Y_4_CORE	2
+#define POWER_LIMITS_Y_2_CORE	3
+#define POWER_LIMITS_MAX	4

/*
  * Enable External V1P05 Rail in: BIT0:S0i1/S0i2,
diff --git a/src/soc/intel/tigerlake/systemagent.c b/src/soc/intel/tigerlake/systemagent.c
index 7b99ef1..f1356e2 100644
--- a/src/soc/intel/tigerlake/systemagent.c
+++ b/src/soc/intel/tigerlake/systemagent.c
@@ -84,6 +84,12 @@
    case PCI_DEVICE_ID_INTEL_TGL_ID_U_2_2:
    	soc_config = &config->power_limits_config[POWER_LIMITS_U_2_CORE];
    	break;
+	case PCI_DEVICE_ID_INTEL_TGL_ID_Y_4_2:
+		soc_config = &config->power_limits_config[POWER_LIMITS_Y_4_CORE];
+		break;
+	case PCI_DEVICE_ID_INTEL_TGL_ID_Y_2_2:
+		soc_config = &config->power_limits_config[POWER_LIMITS_Y_2_CORE];
+		break;
    default:
    	printk(BIOS_ERR, "TGL: unknown SA ID: 0x%4x, skipping power limits "
    	       "configuration\n", sa_pci_id);

[Tim Wawrzynczak (Code Review)]

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

Change subject: soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43607/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43607/1//COMMIT_MSG@10 
PS1, Line 10: 
Can you mention something about setting the limits for volteer too?

[Caveh Jalali (Code Review)]

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

Change subject: soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43607/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43607/1//COMMIT_MSG@10 
PS1, Line 10: 
also, add:

BUG=b:152639350

[Sumeet R Pawnikar (Code Review)]

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

Change subject: soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/43607/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43607/1//COMMIT_MSG@10 
PS1, Line 10:
> also, add: […]

Ack

https://review.coreboot.org/c/coreboot/+/43607/1//COMMIT_MSG@10 
PS1, Line 10:
> Can you mention something about setting the limits for volteer too?

Done

[Sumeet R Pawnikar (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Caveh Jalali, Tim Wawrzynczak, Derek Huang, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU
......................................................................

soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU

Set power limits in devicetree for Tiger Lake Y-SKU varaints.

BUG=b:152639350
BRANCH=None
TEST=Built and tested power limits on volteer variant board.

Change-Id: If4f1226473b48365e5962df9fff29910c99007fc
Signed-off-by: Sumeet R Pawnikar 
---
M src/mainboard/google/volteer/variants/baseboard/devicetree.cb
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/systemagent.c
3 files changed, 25 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/43607/2

[Caveh Jalali (Code Review)]

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

Change subject: soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU
......................................................................

Patch Set 2: Code-Review+1

[Tim Wawrzynczak (Code Review)]

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

Change subject: soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/43607/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43607/2//COMMIT_MSG@9 
PS2, Line 9: varaints
variants

https://review.coreboot.org/c/coreboot/+/43607/2//COMMIT_MSG@10 
PS2, Line 10: 
Can you also mention that you set up power limits for Volteer boards that use the -Y SKU?

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43607/2/src/mainboard/google/voltee... 
File src/mainboard/google/volteer/variants/baseboard/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/43607/2/src/mainboard/google/voltee... 
PS2, Line 214: 	register "power_limits_config[POWER_LIMITS_U_2_CORE]" = "{
How are these power limits determined?

i) Are they same for all mainboards using a particular CPU?
ii) Are these dependent on mainboard design?

If it is i), why not put these values in SoC directly. If it is ii), shouldn't this go in overridetree.cb for that variant?

From the details on the bug it looks like it is probably (i). Or are you putting this here because they need to be tuned later on based on each OEM design?

[Tim Wawrzynczak (Code Review)]

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

Change subject: soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43607/2/src/mainboard/google/voltee... 
File src/mainboard/google/volteer/variants/baseboard/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/43607/2/src/mainboard/google/voltee... 
PS2, Line 214: 	register "power_limits_config[POWER_LIMITS_U_2_CORE]" = "{
> How are these power limits determined? […]

My understanding is that these limits were derived for the Volteer platform, one set of limits for each type (-U or -Y) and # of cores (2 or 4). So they are at least baseline applicable to all variants, although some may require further tuning. Sumeet, is that accurate?

[Sumeet R Pawnikar (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Caveh Jalali, Tim Wawrzynczak, Derek Huang, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU
......................................................................

soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU

Set power limits in devicetree for Tiger Lake Y-SKU based volteer
variant boards.

BUG=b:152639350
BRANCH=None
TEST=Built and tested power limits on volteer variant board.

Change-Id: If4f1226473b48365e5962df9fff29910c99007fc
Signed-off-by: Sumeet R Pawnikar 
---
M src/mainboard/google/volteer/variants/baseboard/devicetree.cb
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/systemagent.c
3 files changed, 25 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/43607/3

[Sumeet R Pawnikar (Code Review)]

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

Change subject: soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU
......................................................................

Patch Set 3:

(3 comments)

https://review.coreboot.org/c/coreboot/+/43607/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43607/2//COMMIT_MSG@9 
PS2, Line 9: varaints
> variants

Done

https://review.coreboot.org/c/coreboot/+/43607/2//COMMIT_MSG@10 
PS2, Line 10:
> Can you also mention that you set up power limits for Volteer boards that use the -Y SKU?

Done

https://review.coreboot.org/c/coreboot/+/43607/2/src/mainboard/google/voltee... 
File src/mainboard/google/volteer/variants/baseboard/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/43607/2/src/mainboard/google/voltee... 
PS2, Line 214: 	register "power_limits_config[POWER_LIMITS_U_2_CORE]" = "{
> My understanding is that these limits were derived for the Volteer platform, one set of limits for e […]

Yes these are baseline values for U/Y SKU type variants. We might need tuning later based on system design.

[Tim Wawrzynczak (Code Review)]

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

Change subject: soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU
......................................................................

Patch Set 3: Code-Review+2

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43607 )

Change subject: soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU
......................................................................

soc/intel/tigerlake: Set power limits for Tiger Lake Y-SKU

Set power limits in devicetree for Tiger Lake Y-SKU based volteer
variant boards.

BUG=b:152639350
BRANCH=None
TEST=Built and tested power limits on volteer variant board.

Change-Id: If4f1226473b48365e5962df9fff29910c99007fc
Signed-off-by: Sumeet R Pawnikar 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/43607
Reviewed-by: Tim Wawrzynczak 
Reviewed-by: Caveh Jalali 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/google/volteer/variants/baseboard/devicetree.cb
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/systemagent.c
3 files changed, 25 insertions(+), 7 deletions(-)

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

diff --git a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb
index e0d3bea..0e8ad3e 100644
--- a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb
+++ b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb
@@ -211,15 +211,25 @@
    # Enable DPTF
    register "dptf_enable" = "1"

+	register "power_limits_config[POWER_LIMITS_U_2_CORE]" = "{
+		.tdp_pl1_override = 15,
+		.tdp_pl2_override = 38,
+		.tdp_pl4 = 71,
+	}"
    register "power_limits_config[POWER_LIMITS_U_4_CORE]" = "{
    	.tdp_pl1_override = 15,
    	.tdp_pl2_override = 60,
    	.tdp_pl4 = 105,
    }"
-	register "power_limits_config[POWER_LIMITS_U_2_CORE]" = "{
-		.tdp_pl1_override = 15,
-		.tdp_pl2_override = 38,
-		.tdp_pl4 = 71,
+	register "power_limits_config[POWER_LIMITS_Y_2_CORE]" = "{
+		.tdp_pl1_override = 9,
+		.tdp_pl2_override = 35,
+		.tdp_pl4 = 66,
+	}"
+	register "power_limits_config[POWER_LIMITS_Y_4_CORE]" = "{
+		.tdp_pl1_override = 9,
+		.tdp_pl2_override = 40,
+		.tdp_pl4 = 83,
    }"

register "Device4Enable" = "1"
diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h
index 59dab58..3d910ce 100644
--- a/src/soc/intel/tigerlake/chip.h
+++ b/src/soc/intel/tigerlake/chip.h
@@ -23,9 +23,11 @@
 #define MAX_HD_AUDIO_SSP_LINKS  6

/* The first two are for TGL-U */
-#define POWER_LIMITS_U_4_CORE	0
-#define POWER_LIMITS_U_2_CORE	1
-#define POWER_LIMITS_MAX	2
+#define POWER_LIMITS_U_2_CORE	0
+#define POWER_LIMITS_U_4_CORE	1
+#define POWER_LIMITS_Y_2_CORE	2
+#define POWER_LIMITS_Y_4_CORE	3
+#define POWER_LIMITS_MAX	4

/*
  * Enable External V1P05 Rail in: BIT0:S0i1/S0i2,
diff --git a/src/soc/intel/tigerlake/systemagent.c b/src/soc/intel/tigerlake/systemagent.c
index fd611bb..29487a8 100644
--- a/src/soc/intel/tigerlake/systemagent.c
+++ b/src/soc/intel/tigerlake/systemagent.c
@@ -84,6 +84,12 @@
    case PCI_DEVICE_ID_INTEL_TGL_ID_U_4_2:
    	soc_config = &config->power_limits_config[POWER_LIMITS_U_4_CORE];
    	break;
+	case PCI_DEVICE_ID_INTEL_TGL_ID_Y_2_2:
+		soc_config = &config->power_limits_config[POWER_LIMITS_Y_2_CORE];
+		break;
+	case PCI_DEVICE_ID_INTEL_TGL_ID_Y_4_2:
+		soc_config = &config->power_limits_config[POWER_LIMITS_Y_4_CORE];
+		break;
    default:
    	printk(BIOS_ERR, "TGL: unknown SA ID: 0x%4x, skipping power limits "
    	       "configuration\n", sa_pci_id);