Change in coreboot[master]: mb/google/volteer: Override power limits with SKU-specific limits

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

mb/google/volteer: Override power limits with SKU-specific limits

Using guidance from Intel, a new set of power limits (PL1, PL2 & PL4)
are available from. They are dependent upon the SKU of the CPU that the
mainboard is running on. This is distinguished via System Agent PCI ID
and the appropriate limits specified in devtree are overriden.

BUG=152639350
TEST=On a Volteer SKU4, verified the following console output:
CPU TDP = 28 Watts
CPU PL1 = 15 Watts
CPU PL2 = 60 Watts
CPU PL4 = 105 Watts

Signed-off-by: Tim Wawrzynczak 
Change-Id: I18a66fc3aacbb3ab594b2e3d6e2a4ad84c10d8f0
---
M src/mainboard/google/volteer/mainboard.c
1 file changed, 41 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/42436/1

diff --git a/src/mainboard/google/volteer/mainboard.c b/src/mainboard/google/volteer/mainboard.c
index 1ede7f2..b24e97a 100644
--- a/src/mainboard/google/volteer/mainboard.c
+++ b/src/mainboard/google/volteer/mainboard.c
@@ -1,16 +1,16 @@
-/*
- *
- *
- * SPDX-License-Identifier: GPL-2.0-or-later
- */
+/* SPDX-License-Identifier: GPL-2.0-or-later */

#include <console/console.h>
 #include <acpi/acpi.h>
 #include <baseboard/variants.h>
 #include <device/device.h>
+#include <device/pci_ids.h>
+#include <device/pci_ops.h>
 #include <ec/ec.h>
 #include <ec/google/chromeec/ec.h>
 #include <soc/gpio.h>
+#include <soc/pci_devs.h>
+#include <soc/soc_chip.h>
 #include <vendorcode/google/chromeos/chromeos.h>
 #include <variant/gpio.h>

@@ -25,6 +25,40 @@
    dev->ops->acpi_inject_dsdt = chromeos_dsdt_generator;
 }

+static void override_power_limits(void)
+{
+	struct soc_intel_tigerlake_config *config;
+	struct device *sa;
+	uint16_t sa_pci_id;
+
+	sa = pcidev_path_on_root(SA_DEVFN_ROOT);
+	sa_pci_id = sa ? pci_read_config16(sa, PCI_DEVICE_ID) : 0xFFFF;
+	config = config_of_soc();
+
+	/*
+	 * Reconfigure power limits depending on which processor SKU is on this particular
+	 * mainboard. This is detected via System Agent PCI ID.
+	 */
+	switch (sa_pci_id) {
+	case PCI_DEVICE_ID_INTEL_TGL_ID_U:
+	case PCI_DEVICE_ID_INTEL_TGL_ID_U_1:
+		/* 4 cores, allow for higher power limits (in W) */
+		config->power_limits_config.tdp_pl1_override = 15;
+		config->power_limits_config.tdp_pl2_override = 60;
+		config->power_limits_config.tdp_pl4 = 105;
+		break;
+	case PCI_DEVICE_ID_INTEL_TGL_ID_U_2_2:
+		/* 2 cores, allow for lower power limits (in W) */
+		config->power_limits_config.tdp_pl1_override = 15;
+		config->power_limits_config.tdp_pl2_override = 38;
+		config->power_limits_config.tdp_pl4 = 71;
+		break;
+	default:
+		printk(BIOS_ERR, "Volteer: unknown SA ID: 0x%4x", sa_pci_id);
+		break;
+	}
+}
+
 static void mainboard_chip_init(void *chip_info)
 {
    const struct pad_config *base_pads;
@@ -36,6 +70,8 @@

gpio_configure_pads_with_override(base_pads, base_num,
    	override_pads, override_num);
+
+	override_power_limits();
 }

struct chip_operations mainboard_ops = {

[Nick Vaccaro (Code Review)]

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42436 )

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

Assuming power limits are correct, LGTM after NIT fix in commit comment.

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

https://review.coreboot.org/c/coreboot/+/42436/1//COMMIT_MSG@10 
PS1, Line 10: from.
?

[Nick Vaccaro (Code Review)]

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42436 )

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

Patch Set 1: Code-Review+1

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42436 )

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42436/1/src/mainboard/google/voltee... 
File src/mainboard/google/volteer/mainboard.c:

https://review.coreboot.org/c/coreboot/+/42436/1/src/mainboard/google/voltee... 
PS1, Line 46: power_limits_config
you could make this into an array and put all the values in devicetree itself

[Caveh Jalali (Code Review)]

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

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

Patch Set 1:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/42436/1//COMMIT_MSG@14 
PS1, Line 14: 152639350
b:...

https://review.coreboot.org/c/coreboot/+/42436/1/src/mainboard/google/voltee... 
File src/mainboard/google/volteer/mainboard.c:

https://review.coreboot.org/c/coreboot/+/42436/1/src/mainboard/google/voltee... 
PS1, Line 28: override_power_limits
i think we need checks here to never increase the limits beyond
what is specified in the device tree. the way i think of this is
that the device tree specifies the system design limits and the
CPU-SKU limits represent the SoC limits. the final limits
should be the MIN(a,b).

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/42436/1/src/mainboard/google/voltee... 
File src/mainboard/google/volteer/mainboard.c:

https://review.coreboot.org/c/coreboot/+/42436/1/src/mainboard/google/voltee... 
PS1, Line 28: override_power_limits
> i think we need checks here to never increase the limits beyond […]

You're right, the PLn limits are SoC-specific (the PsysPLn ones are mainboard/system-specific). Intel came up with these numbers from our reference board's thermal & VR solution. I don't believe we are allowing partners that much latitude with those aspects of system design, so I think these numbers should apply to all of them. LMK if you disagree.

https://review.coreboot.org/c/coreboot/+/42436/1/src/mainboard/google/voltee... 
PS1, Line 46: power_limits_config
> you could make this into an array and put all the values in devicetree itself

That's an interesting thought. With nice symbolic constants for the indexes, that may be more readable even.
#define POWER_LIMITS_4_CORE 0
#define POWER_LIMITS_2_CORE 1
then use those in devicetree. let me give that a try.

[Tim Wawrzynczak (Code Review)]

Hello build bot (Jenkins), Caveh Jalali, Duncan Laurie, Sumeet R Pawnikar, Nick Vaccaro,

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

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

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

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

mb/google/volteer: Override power limits with SKU-specific limits

Using guidance from Intel, a new set of power limits (PL1, PL2 & PL4)
are available. They are dependent upon the SKU of the CPU that the
mainboard is running on. Volteer is updated here to use these new
limits.

To accomplish this, the SoC chip config's power_limits_config member
was expanded to an array, which can be indexed by POWER_LIMITS_*_CORE
macros. Just before power limits are applied, the correct set of them
is chosen from the array based on System Agent PCI ID. Therefore, a
TGL board should have two sets of power limits available in the
devicetree.

BUG=152639350
TEST=On a Volteer SKU4 (4-core), verified the following console output:
CPU PL1 = 15 Watts
CPU PL2 = 60 Watts
CPU PL4 = 105 Watts

Signed-off-by: Tim Wawrzynczak 
Change-Id: I18a66fc3aacbb3ab594b2e3d6e2a4ad84c10d8f0
Signed-off-by: Tim Wawrzynczak 
---
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, 39 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/42436/2

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42436/1/src/mainboard/google/voltee... 
File src/mainboard/google/volteer/mainboard.c:

https://review.coreboot.org/c/coreboot/+/42436/1/src/mainboard/google/voltee... 
PS1, Line 28: override_power_limits
> You're right, the PLn limits are SoC-specific (the PsysPLn ones are mainboard/system-specific). […]

Also with the new patch set, that concern is no longer there.

[Caveh Jalali (Code Review)]

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

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

Patch Set 2:

(3 comments)

ya, this

https://review.coreboot.org/c/coreboot/+/42436/1/src/mainboard/google/voltee... 
File src/mainboard/google/volteer/mainboard.c:

https://review.coreboot.org/c/coreboot/+/42436/1/src/mainboard/google/voltee... 
PS1, Line 28: override_power_limits
> Also with the new patch set, that concern is no longer there.

cool, ya the new patchset does provide better control over the values.

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

https://review.coreboot.org/c/coreboot/+/42436/2/src/soc/intel/tigerlake/chi... 
PS2, Line 25: #define POWER_LIMITS_4_CORE	0
            : #define POWER_LIMITS_2_CORE	1
we may want to use more descriptive naming here, specially since
this is exposed in the device tree. there'll be a tier of values
for TGL-U and a tier for TGL-Y. 2-core vs. 4-core is likely going to be
ambiguous.

https://review.coreboot.org/c/coreboot/+/42436/2/src/soc/intel/tigerlake/sys... 
File src/soc/intel/tigerlake/systemagent.c:

https://review.coreboot.org/c/coreboot/+/42436/2/src/soc/intel/tigerlake/sys... 
PS2, Line 80: 	switch (sa_pci_id) {
we'll need to include PCI_DEVICE_ID_INTEL_TGL_ID_Y at some point.

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

Patch Set 2:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/42436/2/src/soc/intel/tigerlake/chi... 
PS2, Line 25: #define POWER_LIMITS_4_CORE	0
            : #define POWER_LIMITS_2_CORE	1
> we may want to use more descriptive naming here, specially since […]

Ack

https://review.coreboot.org/c/coreboot/+/42436/2/src/soc/intel/tigerlake/sys... 
File src/soc/intel/tigerlake/systemagent.c:

https://review.coreboot.org/c/coreboot/+/42436/2/src/soc/intel/tigerlake/sys... 
PS2, Line 80: 	switch (sa_pci_id) {
> we'll need to include PCI_DEVICE_ID_INTEL_TGL_ID_Y at some point.

Sure, I just haven't seen anything yet. LMK if you do 😊
I also don't see 9A10 (TGL-Y) called out in the stepping ID guide...

[Tim Wawrzynczak (Code Review)]

Hello build bot (Jenkins), Caveh Jalali, Duncan Laurie, Sumeet R Pawnikar, Nick Vaccaro, Patrick Rudolph,

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

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

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

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

mb/google/volteer: Override power limits with SKU-specific limits

Using guidance from Intel, a new set of power limits (PL1, PL2 & PL4)
are available for TGL-U. They are dependent upon the SKU of the CPU
that the mainboard is running on. Volteer is updated here to use these
new limits.

To accomplish this, the SoC chip config's power_limits_config member
was expanded to an array, which can be indexed by POWER_LIMITS_*_CORE
macros. Just before power limits are applied, the correct set of them
is chosen from the array based on System Agent PCI ID. Therefore, a
TGL board should have two sets of power limits available in the
devicetree.

BUG=b:152639350
TEST=On a Volteer SKU4 (4-core), verified the following console output:
CPU PL1 = 15 Watts
CPU PL2 = 60 Watts
CPU PL4 = 105 Watts

Change-Id: I18a66fc3aacbb3ab594b2e3d6e2a4ad84c10d8f0
Signed-off-by: Tim Wawrzynczak 
---
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, 40 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/42436/3

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

Patch Set 3:

(3 comments)

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

https://review.coreboot.org/c/coreboot/+/42436/1//COMMIT_MSG@10 
PS1, Line 10: from.
> ?

Ack

https://review.coreboot.org/c/coreboot/+/42436/1//COMMIT_MSG@14 
PS1, Line 14: 152639350
> b:...

Ack

https://review.coreboot.org/c/coreboot/+/42436/1/src/mainboard/google/voltee... 
File src/mainboard/google/volteer/mainboard.c:

https://review.coreboot.org/c/coreboot/+/42436/1/src/mainboard/google/voltee... 
PS1, Line 46: power_limits_config
> That's an interesting thought. […]

Ack

[Nick Vaccaro (Code Review)]

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42436 )

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

Patch Set 3: Code-Review+1

[Caveh Jalali (Code Review)]

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

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

Patch Set 3: Code-Review+2

[Patrick Georgi (Code Review)]

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

Change subject: mb/google/volteer: Override power limits with SKU-specific limits
......................................................................

mb/google/volteer: Override power limits with SKU-specific limits

Using guidance from Intel, a new set of power limits (PL1, PL2 & PL4)
are available for TGL-U. They are dependent upon the SKU of the CPU
that the mainboard is running on. Volteer is updated here to use these
new limits.

To accomplish this, the SoC chip config's power_limits_config member
was expanded to an array, which can be indexed by POWER_LIMITS_*_CORE
macros. Just before power limits are applied, the correct set of them
is chosen from the array based on System Agent PCI ID. Therefore, a
TGL board should have two sets of power limits available in the
devicetree.

BUG=b:152639350
TEST=On a Volteer SKU4 (4-core), verified the following console output:
CPU PL1 = 15 Watts
CPU PL2 = 60 Watts
CPU PL4 = 105 Watts

Change-Id: I18a66fc3aacbb3ab594b2e3d6e2a4ad84c10d8f0
Signed-off-by: Tim Wawrzynczak 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42436
Tested-by: build bot (Jenkins) 
Reviewed-by: Nick Vaccaro 
Reviewed-by: Caveh Jalali 
---
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, 40 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nick Vaccaro: Looks good to me, but someone else must approve
  Caveh Jalali: 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 8cd926c..f2e427f 100644
--- a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb
+++ b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb
@@ -198,9 +198,15 @@
    # Enable DPTF
    register "dptf_enable" = "1"

-	register "power_limits_config" = "{
+	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 "Device4Enable" = "1"
diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h
index 30377aa..8b1fe2d 100644
--- a/src/soc/intel/tigerlake/chip.h
+++ b/src/soc/intel/tigerlake/chip.h
@@ -22,6 +22,11 @@
 #define MAX_HD_AUDIO_SNDW_LINKS 4
 #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
+
 /*
  * Enable External V1P05 Rail in: BIT0:S0i1/S0i2,
  * BIT1:S0i3, BIT2:S3, BIT3:S4, BIT4:S5
@@ -55,7 +60,7 @@
    struct soc_intel_common_config common_soc_config;

/* Common struct containing power limits configuration information */
-	struct soc_power_limits_config power_limits_config;
+	struct soc_power_limits_config power_limits_config[POWER_LIMITS_MAX];

/* Gpio group routed to each dword of the GPE0 block. Values are
     * of the form PMC_GPP_[A:U] or GPD. */
diff --git a/src/soc/intel/tigerlake/systemagent.c b/src/soc/intel/tigerlake/systemagent.c
index 08d1ef3..e428365 100644
--- a/src/soc/intel/tigerlake/systemagent.c
+++ b/src/soc/intel/tigerlake/systemagent.c
@@ -6,9 +6,11 @@
  * Chapter number: 3
  */

+#include <console/console.h>
 #include <device/device.h>
 #include <delay.h>
 #include <device/pci.h>
+#include <device/pci_ids.h>
 #include <device/pci_ops.h>
 #include <intelblocks/power_limit.h>
 #include <intelblocks/systemagent.h>
@@ -53,8 +55,14 @@
 void soc_systemagent_init(struct device *dev)
 {
    struct soc_power_limits_config *soc_config;
+	struct device *sa;
+	uint16_t sa_pci_id;
    config_t *config;

+	/* Get System Agent PCI ID */
+	sa = pcidev_path_on_root(SA_DEVFN_ROOT);
+	sa_pci_id = sa ? pci_read_config16(sa, PCI_DEVICE_ID) : 0xFFFF;
+
    /* Enable Power Aware Interrupt Routing */
    enable_power_aware_intr();

@@ -64,7 +72,25 @@
    /* Configure turbo power limits 1ms after reset complete bit */
    mdelay(1);
    config = config_of_soc();
-	soc_config = &config->power_limits_config;
+
+	/*
+	 * Choose a power limits configuration based on the SoC SKU,
+	 * differentiated here based on SA PCI ID.
+	 */
+	switch (sa_pci_id) {
+	case PCI_DEVICE_ID_INTEL_TGL_ID_U:
+	case PCI_DEVICE_ID_INTEL_TGL_ID_U_1:
+		soc_config = &config->power_limits_config[POWER_LIMITS_U_4_CORE];
+		break;
+	case PCI_DEVICE_ID_INTEL_TGL_ID_U_2_2:
+		soc_config = &config->power_limits_config[POWER_LIMITS_U_2_CORE];
+		break;
+	default:
+		printk(BIOS_ERR, "TGL: unknown SA ID: 0x%4x, skipping power limits "
+		       "configuration\n", sa_pci_id);
+		return;
+	}
+
    set_power_limits(MOBILE_SKU_PL1_TIME_SEC, soc_config);
 }