Hello Sumeet Pawnikar,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42513
to review the following change.
Change subject: tigerlake: enable tcc_offset functionality ......................................................................
tigerlake: enable tcc_offset functionality
This enables Thermal Control Circuit (TCC) activation feature to set tcc_offset value to new value as 10 degree Celcius in devicetree.
BUG=None BRANCH=None TEST=Built for volteer platform and verified the MSR value.
Change-Id: I36b0d6aad4be8a9cbb145dcd66d65235d3f6ac35 Signed-off-by: Sumeet R Pawnikar sumeet.r.pawnikar@intel.corp-partner.google.com --- M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/soc/intel/tigerlake/cpu.c M src/soc/intel/tigerlake/fsp_params.c 3 files changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/42513/1
diff --git a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb index 8cd926c..18b436d 100644 --- a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb @@ -205,6 +205,8 @@
register "Device4Enable" = "1"
+ register "tcc_offset" = "10" # TCC of 90C + # Intel Common SoC Config #+-------------------+---------------------------+ #| Field | Value | diff --git a/src/soc/intel/tigerlake/cpu.c b/src/soc/intel/tigerlake/cpu.c index ec78d15..b95fdc6 100644 --- a/src/soc/intel/tigerlake/cpu.c +++ b/src/soc/intel/tigerlake/cpu.c @@ -216,4 +216,7 @@ { if (mp_init_with_smm(cpu_bus, &mp_ops)) printk(BIOS_ERR, "MP initialization failure.\n"); + + /* Thermal throttle activation offset */ + configure_thermal_target(); } diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c index 3fbb89a..a612427 100644 --- a/src/soc/intel/tigerlake/fsp_params.c +++ b/src/soc/intel/tigerlake/fsp_params.c @@ -213,6 +213,9 @@ /* Enable TCPU for processor thermal control */ params->Device4Enable = config->Device4Enable;
+ /* Set TccActivationOffset */ + params->TccActivationOffset = config->tcc_offset; + /* LAN */ dev = pcidev_path_on_root(PCH_DEVFN_GBE); if (!dev)
Hello Sumeet Pawnikar, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Tim Wawrzynczak, Nick Vaccaro, Aaron Durbin, Patrick Rudolph, Karthikeyan Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42513
to look at the new patch set (#3).
Change subject: tigerlake: enable tcc_offset functionality ......................................................................
tigerlake: enable tcc_offset functionality
This enables Thermal Control Circuit (TCC) activation feature to set tcc_offset value to new value as 10 degree Celcius in devicetree.
BUG=None BRANCH=None TEST=Built for volteer platform and verified the MSR value.
Change-Id: I36b0d6aad4be8a9cbb145dcd66d65235d3f6ac35 Signed-off-by: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com --- M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/soc/intel/tigerlake/cpu.c M src/soc/intel/tigerlake/fsp_params.c 3 files changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/42513/3
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42513 )
Change subject: tigerlake: enable tcc_offset functionality ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42513/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/42513/3/src/mainboard/google/voltee... PS3, Line 208: 90C What is "90C"?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42513 )
Change subject: tigerlake: enable tcc_offset functionality ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42513/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/42513/3/src/mainboard/google/voltee... PS3, Line 208: 90C
What is "90C"?
90 degrees C. This adds 10 degrees C to the default TCC setting of 80 deg C. "Thermal Control Circuit (TCC): Specified maximum temperature limit at which processor power will be automatically reduced."
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42513 )
Change subject: tigerlake: enable tcc_offset functionality ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42513/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/42513/3/src/mainboard/google/voltee... PS3, Line 208: 10 How much testing has this change had? Given the thermal issues we've seen with TGL, will this not make those worse if we allow it to get even hotter before reducing power?
https://review.coreboot.org/c/coreboot/+/42513/3/src/mainboard/google/voltee... PS3, Line 208: 90C
90 degrees C. This adds 10 degrees C to the default TCC setting of 80 deg C. […]
Thanks Tim, now it makes sense.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42513 )
Change subject: tigerlake: enable tcc_offset functionality ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42513/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/42513/3/src/mainboard/google/voltee... PS3, Line 208: 10
How much testing has this change had? Given the thermal issues we've seen with TGL, will this not m […]
Or was the issue not the actual heat itself causing SoC hang issues but the fact that power was being reduced?
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42513 )
Change subject: tigerlake: enable tcc_offset functionality ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42513/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/42513/3/src/mainboard/google/voltee... PS3, Line 208: 10
Or was the issue not the actual heat itself causing SoC hang issues but the fact that power was bein […]
Current Temperature Target (TjMax) is 100 degree C as per bits [23:16]. Above we are setting TCC Target Offset to 10 degree C [bits 30:24]. So, Pcode FW will start taking throttling action at 90 degree C [i.e. Temperature Target - Offset]. If there is no Target Offset (i.e. suppose 0) than FW will start throttling action at 100 degree C which is very high temperature and might be late to take any thermal throttling action to control the temperature. Here, by setting Target Offset to 10 degree C will prevent SoC temperature to rise above 90 degree C and help to control thermal heat up issues by taking early thorttling action.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42513 )
Change subject: tigerlake: enable tcc_offset functionality ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42513/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/42513/3/src/mainboard/google/voltee... PS3, Line 208: 10
Current Temperature Target (TjMax) is 100 degree C as per bits [23:16]. […]
Ack
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42513 )
Change subject: tigerlake: enable tcc_offset functionality ......................................................................
Patch Set 4:
Sumeet, do you think you could split this up into two patches, one to add the fields to tiger lake, and the other to set TCC offset to 10 for volteer?
Hello Sumeet Pawnikar, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Tim Wawrzynczak, Nick Vaccaro, Aaron Durbin, Patrick Rudolph, Karthikeyan Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42513
to look at the new patch set (#5).
Change subject: tigerlake: enable tcc_offset functionality ......................................................................
tigerlake: enable tcc_offset functionality
This enables Thermal Control Circuit (TCC) activation feature to set tcc_offset to new value in devicetree.
BUG=None BRANCH=None TEST=Built for volteer platform and verified the MSR value.
Change-Id: I36b0d6aad4be8a9cbb145dcd66d65235d3f6ac35 Signed-off-by: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/tigerlake/cpu.c M src/soc/intel/tigerlake/fsp_params.c 2 files changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/42513/5
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42513 )
Change subject: tigerlake: enable tcc_offset functionality ......................................................................
Patch Set 5:
Patch Set 4:
Sumeet, do you think you could split this up into two patches, one to add the fields to tiger lake, and the other to set TCC offset to 10 for volteer?
Sure, submitted two separate patches as per above comment.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42513 )
Change subject: tigerlake: enable tcc_offset functionality ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42513 )
Change subject: tigerlake: enable tcc_offset functionality ......................................................................
tigerlake: enable tcc_offset functionality
This enables Thermal Control Circuit (TCC) activation feature to set tcc_offset to new value in devicetree.
BUG=None BRANCH=None TEST=Built for volteer platform and verified the MSR value.
Change-Id: I36b0d6aad4be8a9cbb145dcd66d65235d3f6ac35 Signed-off-by: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42513 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/tigerlake/cpu.c M src/soc/intel/tigerlake/fsp_params.c 2 files changed, 6 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/soc/intel/tigerlake/cpu.c b/src/soc/intel/tigerlake/cpu.c index ec78d15..9a96f8f 100644 --- a/src/soc/intel/tigerlake/cpu.c +++ b/src/soc/intel/tigerlake/cpu.c @@ -216,4 +216,7 @@ { if (mp_init_with_smm(cpu_bus, &mp_ops)) printk(BIOS_ERR, "MP initialization failure.\n"); + + /* Thermal throttle activation offset */ + configure_tcc_thermal_target(); } diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c index 3fbb89a..a612427 100644 --- a/src/soc/intel/tigerlake/fsp_params.c +++ b/src/soc/intel/tigerlake/fsp_params.c @@ -213,6 +213,9 @@ /* Enable TCPU for processor thermal control */ params->Device4Enable = config->Device4Enable;
+ /* Set TccActivationOffset */ + params->TccActivationOffset = config->tcc_offset; + /* LAN */ dev = pcidev_path_on_root(PCH_DEVFN_GBE); if (!dev)
-
Nick Vaccaro (Code Review) -
Patrick Georgi (Code Review)
-
Sumeet R Pawnikar (Code Review)
-
Tim Wawrzynczak (Code Review)