Change in ...coreboot[master]: soc/intel/cannonlake: Set correct temperature threshold for PCH Therm...
Sumeet R Pawnikar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33129
Change subject: soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor ......................................................................
soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFA.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/cannonlake/Makefile.inc M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/finalize.c M src/soc/intel/cannonlake/include/soc/iomap.h A src/soc/intel/cannonlake/include/soc/thermal.h A src/soc/intel/cannonlake/thermal.c 6 files changed, 147 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/1
diff --git a/src/soc/intel/cannonlake/Makefile.inc b/src/soc/intel/cannonlake/Makefile.inc index 1328944..03faa3c 100644 --- a/src/soc/intel/cannonlake/Makefile.inc +++ b/src/soc/intel/cannonlake/Makefile.inc @@ -53,6 +53,7 @@ ramstage-y += smmrelocate.c ramstage-y += spi.c ramstage-y += systemagent.c +ramstage-y += thermal.c ramstage-y += uart.c ramstage-y += vr_config.c ramstage-y += sd.c diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index 0d51c1c..39f808b 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -421,6 +421,9 @@ * Bit 0: MISCCFG_GPDLCGEN */ uint8_t gpio_pm[TOTAL_GPIO_COMM]; + + /* PCH Trip Temperature */ + uint8_t pch_trip_temp; };
typedef struct soc_intel_cannonlake_config config_t; diff --git a/src/soc/intel/cannonlake/finalize.c b/src/soc/intel/cannonlake/finalize.c index 4dfd15b..d277658 100644 --- a/src/soc/intel/cannonlake/finalize.c +++ b/src/soc/intel/cannonlake/finalize.c @@ -32,6 +32,7 @@ #include <soc/pm.h> #include <soc/smbus.h> #include <soc/systemagent.h> +#include <soc/thermal.h> #include <stdlib.h>
#include "chip.h" @@ -63,6 +64,16 @@ uint8_t reg8;
tco_lockdown(); + + /* + * Set low maximum temp value used for dynamic thermal sensor + * shutdown consideration. + * + * If Dynamic Thermal Shutdown is enabled then PMC logic shuts down the + * thermal sensor when CPU is in a C-state and DTS Temp <= LTT. + */ + pch_thermal_configuration(); + /* * Disable ACPI PM timer based on dt policy * diff --git a/src/soc/intel/cannonlake/include/soc/iomap.h b/src/soc/intel/cannonlake/include/soc/iomap.h index 100bd11..488d938 100644 --- a/src/soc/intel/cannonlake/include/soc/iomap.h +++ b/src/soc/intel/cannonlake/include/soc/iomap.h @@ -76,6 +76,8 @@
#define HECI1_BASE_ADDRESS 0xfeda2000
+#define THERMAL_BASE_ADDRESS 0xfe600000 + #define VTD_BASE_ADDRESS 0xFED90000 #define VTD_BASE_SIZE 0x00004000 /* diff --git a/src/soc/intel/cannonlake/include/soc/thermal.h b/src/soc/intel/cannonlake/include/soc/thermal.h new file mode 100644 index 0000000..1f4085c --- /dev/null +++ b/src/soc/intel/cannonlake/include/soc/thermal.h @@ -0,0 +1,24 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Intel Corp. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _SOC_THERMAL_H_ +#define _SOC_THERMAL_H_ + +#define THERMAL_SENSOR_POWER_MANAGEMENT 0x1c + +/* Enable thermal sensor power management */ +void pch_thermal_configuration(void); + +#endif diff --git a/src/soc/intel/cannonlake/thermal.c b/src/soc/intel/cannonlake/thermal.c new file mode 100644 index 0000000..9d62843 --- /dev/null +++ b/src/soc/intel/cannonlake/thermal.c @@ -0,0 +1,106 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Intel Corp. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <device/mmio.h> +#include <device/pci_ops.h> +#include <console/console.h> +#include <device/device.h> +#include <device/pci.h> +#include <soc/iomap.h> +#include <soc/pci_devs.h> +#include <soc/thermal.h> + +#include "chip.h" + +#define MAX_TRIP_TEMP 205 +#define DEFAULT_TRIP_TEMP 50 + +static void *pch_thermal_get_bar(struct device *dev) +{ + uintptr_t bar; + + bar = pci_read_config32(dev, PCI_BASE_ADDRESS_0); + /* + * Bits [31:12] are the base address as per EDS for Thermal Device, + * Don't care about [11:0] bits + */ + return (void *)(bar & ~PCI_BASE_ADDRESS_MEM_ATTR_MASK); +} + +static void pch_thermal_set_bar(struct device *dev, uintptr_t tempbar) +{ + uint8_t pcireg; + + /* Assign Resources to Thermal Device */ + /* Clear BIT 1-2 of Command Register */ + pcireg = pci_read_config8(dev, PCI_COMMAND); + pcireg &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY); + pci_write_config8(dev, PCI_COMMAND, pcireg); + + /* Program Temporary BAR for Thermal Device */ + pci_write_config32(dev, PCI_BASE_ADDRESS_0, tempbar); + pci_write_config32(dev, PCI_BASE_ADDRESS_1, 0x0); + + /* Enable Bus Master and MMIO Space */ + pcireg = pci_read_config8(dev, PCI_COMMAND); + pcireg |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY; + pci_write_config8(dev, PCI_COMMAND, pcireg); +} + +/* PCH Low Temp Threshold (LTT) */ +static uint16_t pch_get_ltt_value(struct device *dev) +{ + static struct soc_intel_skylake_config *config; + uint16_t ltt_value; + uint16_t trip_temp = DEFAULT_TRIP_TEMP; + + config = dev->chip_info; + + if (config->pch_trip_temp) + trip_temp = config->pch_trip_temp; + + if (trip_temp > MAX_TRIP_TEMP) + die("Input PCH temp trip is higher than allowed range!"); + + /* Trip Point Temp = (LTT / 2 - 50 degree C) */ + ltt_value = (trip_temp + 50) * 2; + + return ltt_value; +} + +/* Enable thermal sensor power management */ +void pch_thermal_configuration(void) +{ + uint16_t reg16; + struct device *dev = PCH_DEV_THERMAL; + if (!dev) { + printk(BIOS_ERR, "PCH_DEV_THERMAL device not found!\n"); + return; + } + void *thermalbar = pch_thermal_get_bar(dev); + + /* Use default pre-ram bar */ + if (!thermalbar) { + pch_thermal_set_bar(dev, THERMAL_BASE_ADDRESS); + thermalbar = (void *)THERMAL_BASE_ADDRESS; + } + + /* Set Low Temp Threshold (LTT) at TSPM offset 0x1c[8:0] */ + reg16 = read16(thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT); + reg16 &= ~0x1ff; + /* Low Temp Threshold (LTT) */ + reg16 |= pch_get_ltt_value(dev); + write16(thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT, reg16); +}
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor ......................................................................
soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFA.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/cannonlake/Makefile.inc M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/finalize.c M src/soc/intel/cannonlake/include/soc/iomap.h A src/soc/intel/cannonlake/include/soc/thermal.h A src/soc/intel/cannonlake/thermal.c 6 files changed, 147 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 3:
(7 comments)
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/chip.h@425 PS3, Line 425: /* PCH Trip Temperature */ What are the units?
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/iomap.h:
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/include/soc... PS3, Line 79: Just use a space to keep it consistent with other definitions.
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/thermal.h:
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/include/soc... PS3, Line 19: #define THERMAL_SENSOR_POWER_MANAGEMENT 0x1c Why does this have to be in the .h file? Its used only within thermal.c
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/thermal.c File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/thermal.c@2... PS3, Line 27: #define MAX_TRIP_TEMP 205 It would be good to add a comment indicating that this value follows the same formula you mentioned below -- 0x1ff / 2 - 50 = ~205
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/thermal.c@2... PS3, Line 28: #define DEFAULT_TRIP_TEMP 50 How is this default value determined?
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/thermal.c@8... PS3, Line 87: struct const
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/thermal.c@1... PS3, Line 104: pch_get_ltt_value(dev); Shouldn't this also be enabling bits 13 and 14? i.e. dynamic thermal sensor shutdown?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/chip.h@425 PS3, Line 425: /* PCH Trip Temperature */
What are the units?
Agreed. When a variable contains a value that is in specific units, I often add a '_C' for example to indicate that the unit.
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/thermal.c File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/thermal.c@7... PS3, Line 78: ltt_value = (trip_temp + 50) * 2; Macros to calculate this would be helpful; you could also then use them to define MAX_TRIP_TEMP, and perhaps DEFAULT_TRIP_TEMp as well.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 3:
We are working on addressing above review comments. We will share next version. Thanks.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 3:
Patch Set 3:
We are working on addressing above review comments. We will share next version. Thanks.
Any updates?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33129/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33129/3//COMMIT_MSG@7 PS3, Line 7: soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor s/Set correct/correct/ is shorter.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/chip.h@425 PS3, Line 425: /* PCH Trip Temperature */
Agreed. […]
Does it make sense to add unit in this comment part ?
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/iomap.h:
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/include/soc... PS3, Line 79:
Just use a space to keep it consistent with other definitions.
Ack
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/thermal.c File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/thermal.c@2... PS3, Line 27: #define MAX_TRIP_TEMP 205
It would be good to add a comment indicating that this value follows the same formula you mentioned […]
I will add below comment for this define. /* Based on formula calculated Max Trip Temp=(0x1ff / 2 - 50 degree C) */
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/thermal.c@2... PS3, Line 28: #define DEFAULT_TRIP_TEMP 50
How is this default value determined?
This is the safest Trip Temp value.
https://review.coreboot.org/#/c/33129/3/src/soc/intel/cannonlake/thermal.c@8... PS3, Line 87: struct
const
We are modifying the register values below for this structure. Please, share your comment on why it should be const structure ?
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 3:
We are working on other comments.
Hello Aaron Durbin, Rajat Jain, Paul Fagerburg, Tim Wawrzynczak, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#4).
Change subject: soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor ......................................................................
soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFA.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/cannonlake/Makefile.inc M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/finalize.c M src/soc/intel/cannonlake/include/soc/iomap.h A src/soc/intel/cannonlake/include/soc/thermal.h A src/soc/intel/cannonlake/thermal.c 6 files changed, 148 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/4
Hello Patrick Rudolph, Aaron Durbin, Rajat Jain, Paul Fagerburg, Tim Wawrzynczak, Duncan Laurie, Shelley Chen, build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#5).
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFA.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/cannonlake/Makefile.inc M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/finalize.c M src/soc/intel/cannonlake/include/soc/iomap.h A src/soc/intel/cannonlake/include/soc/thermal.h A src/soc/intel/cannonlake/thermal.c 6 files changed, 148 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/5
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 5: Code-Review-1
(7 comments)
Is there a reason why it is done in the finalize function?
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/in... PS5, Line 71: #define THERMAL_BASE_ADDRESS 0xfe600000 tab
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 28: #define MAX_TRIP_TEMP 205 make use of GET_LTT_VALUE
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 37: bar = pci_read_config32(dev, PCI_BASE_ADDRESS_0); use find_resource(dev, PCI_BASE_ADDRESS_0); instead
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 74: if (config->pch_trip_temp) possible NULL pointer dereference
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 78: die("Input PCH temp trip is higher than allowed range!"); I guess printk(BIOS_ERR is sufficient if you limit the trip_temp to MAX_TRIP_TEMP
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 81: ltt_value = GET_LTT_VALUE; please use the define in correct manner
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 98: if (!thermalbar) { shouldn't happen. Printing an error should be sufficient.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 5:
(7 comments)
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/in... PS5, Line 71: #define THERMAL_BASE_ADDRESS 0xfe600000
tab
Ack
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 28: #define MAX_TRIP_TEMP 205
make use of GET_LTT_VALUE
This is the worst case value writing 1 to all [8:0] bits and calculated based on below formula, Max Trip Temp=(0x1ff / 2 - 50 degree C). I would think to add this as a comment for this value.
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 37: bar = pci_read_config32(dev, PCI_BASE_ADDRESS_0);
use find_resource(dev, PCI_BASE_ADDRESS_0); instead
“pci_read_config32” reads the 32-bit address for thermal register on CML-Cometlake. It's definition is in payloads/libpayload/drivers/pci.c file. If we replace this with “bar = find_resource(dev, PCI_BASE_ADDRESS_0);” where find_resource returns structure of the base address availability, which we might not be able to use directly below in next statement. Definition of this find_resource is in src/device/device_util.c file
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 74: if (config->pch_trip_temp)
possible NULL pointer dereference
This "pch_trip_temp" has value defined in devicetree.cb file. If not, this config structure is static as per above line 68, which would have default value as 0 and trip_temp will have DEFAULT_TRIP_TEMP from line 70.
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 78: die("Input PCH temp trip is higher than allowed range!");
I guess printk(BIOS_ERR is sufficient if you limit the trip_temp to MAX_TRIP_TEMP
There is a reason for using die here because this is some fatal error kind of situation where trip_temp is higher than the worst case MAX_TRIP_TEMP 205 deg C.
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 81: ltt_value = GET_LTT_VALUE;
please use the define in correct manner
As per the comment on line 80 mentioned above, here we have defined the GET_LTT_VALUE macro as ((trip_temp + 50) * 2) where we get trip_temp from config. Let's know if you have any other suggestion for this.
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 98: if (!thermalbar) {
shouldn't happen. Printing an error should be sufficient.
We need this because on CML (cometlake) observed this might be 0. So, need to set appropriate thermal base address with below statements.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 5: Code-Review-2
(5 comments)
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 37: bar = pci_read_config32(dev, PCI_BASE_ADDRESS_0);
“pci_read_config32” reads the 32-bit address for thermal register on CML-Cometlake. […]
I don't see how this file affects libpayload. Maybe you need to fix your IDE?
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 74: if (config->pch_trip_temp)
This "pch_trip_temp" has value defined in devicetree.cb file. […]
you seem not to know what a NULL pointer dereference is.
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 78: die("Input PCH temp trip is higher than allowed range!");
There is a reason for using die here because this is some fatal error kind of situation where trip_t […]
why is 205 deg C fatal, but 204 deg C is not?
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 81: ltt_value = GET_LTT_VALUE;
As per the comment on line 80 mentioned above, here we have defined the GET_LTT_VALUE macro as ((tri […]
the correct way of using defines for formular is #define GET_LTT_VALUE(x) ((x + 50) * 2)
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 98: if (!thermalbar) {
We need this because on CML (cometlake) observed this might be 0. […]
You either don't understand why it's 0 or you don't care. In both cases this code should never made it into master. The reason why it is 0 is that you likely have disabled the thermal device in devicetree.cb.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 5: -Code-Review
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 31: trip_temp This macro should not make assumptions about variable name. It would be better to define this : #define GET_LTT_VALUE(v) ((v) + 50) * 2)
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 98: if (!thermalbar) {
You either don't understand why it's 0 or you don't care. […]
You will need something like this for hatch:
diff --git a/src/mainboard/google/hatch/variants/baseboard/devicetree.cb b/src/mainboard/google/hatch/variants/baseboard/devicetree.cb index 9a9125396c3..dffb3d66c78 100644 --- a/src/mainboard/google/hatch/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/hatch/variants/baseboard/devicetree.cb @@ -112,7 +112,7 @@ chip soc/intel/cannonlake device pci 02.0 on end # Integrated Graphics Device device pci 04.0 off end # SA Thermal device device pci 05.0 off end # SA IPU - device pci 12.0 off end # Thermal Subsystem + device pci 12.0 on end # Thermal Subsystem device pci 12.5 off end # UFS SCS device pci 12.6 off end # GSPI #2 device pci 14.0 on
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 5:
(6 comments)
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 31: trip_temp
This macro should not make assumptions about variable name. […]
Sure, we will use this way. Thanks for suggestion.
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 37: bar = pci_read_config32(dev, PCI_BASE_ADDRESS_0);
I don't see how this file affects libpayload. […]
To use find_resource(), we are planning to use below code struct resource *res; res = find_resource(dev, PCI_BASE_ADDRESS_0); /* Assign the base address of the resource */ bar = res->base;
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 74: if (config->pch_trip_temp)
you seem not to know what a NULL pointer dereference is.
We will check this condition and print an error message.
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 78: die("Input PCH temp trip is higher than allowed range!");
why is 205 deg C fatal, but 204 deg C is not?
worst case (MAX) value writing 1 to all [8:0] bits ie 0x1ff. MAX_TRIP_VALUE=(0x1ff / 2 - 50 deg C) = 205 deg C. Even for 204 deg C is not valid case. But, we have considered here the worst case of all 1s.
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 81: ltt_value = GET_LTT_VALUE;
the correct way of using defines for formular is […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 98: if (!thermalbar) {
You will need something like this for hatch: […]
Sure, we will implement this way. I have tested and it works.
Hello Patrick Rudolph, Aaron Durbin, Rajat Jain, Patrick Rudolph, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Paul Fagerburg, Tim Wawrzynczak, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#6).
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFA.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/cannonlake/Makefile.inc M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/finalize.c M src/soc/intel/cannonlake/include/soc/iomap.h A src/soc/intel/cannonlake/include/soc/thermal.h A src/soc/intel/cannonlake/thermal.c 6 files changed, 138 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/6
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 31: #define GET_LTT_VALUE(x) ((x + 50) * 2) Macro parameters should be parenthesized: (((x) + 50) * 2)
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 27: #define THERMAL_SENSOR_POWER_MANAGEMENT 0x1c : #define MAX_TRIP_TEMP 205 : /* This is the safest default Trip Temp value */ : #define DEFAULT_TRIP_TEMP 50 : #define GET_LTT_VALUE(x) ((x + 50) * 2) Can you line these up together?
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 95: ~0x1ff Please make this a #define'd MASK instead of magic numbers.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 6: Code-Review-1
(3 comments)
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 47: return (void *)(bar & ~PCI_BASE_ADDRESS_MEM_ATTR_MASK); PCI_BASE_ADDRESS_MEM_ATTR_MASK isn't necessary, the resource base doesn't contain attributes.
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 59: if (!config->pch_trip_temp) { config might be NULL
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 90: if (!thermalbar) return of thermalbar not found. Otherwise you might corrupt low memory.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 47: return (void *)(bar & ~PCI_BASE_ADDRESS_MEM_ATTR_MASK);
PCI_BASE_ADDRESS_MEM_ATTR_MASK isn't necessary, the resource base doesn't contain attributes.
Ack
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 59: if (!config->pch_trip_temp) {
config might be NULL
We will add one more if to check is config NULL or not. If yes, print error message and return.
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 90: if (!thermalbar)
return of thermalbar not found. […]
Sure. Thanks for this finding.
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 95: ~0x1ff
Please make this a #define'd MASK instead of magic numbers.
Ack
Hello Patrick Rudolph, Aaron Durbin, Rajat Jain, Patrick Rudolph, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Paul Fagerburg, Tim Wawrzynczak, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#7).
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFA.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/cannonlake/Makefile.inc M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/finalize.c M src/soc/intel/cannonlake/include/soc/iomap.h A src/soc/intel/cannonlake/include/soc/thermal.h A src/soc/intel/cannonlake/thermal.c 6 files changed, 142 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/7
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/in... PS7, Line 71: nit: Please use tabs like the other macros.
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... PS7, Line 32: ( nit: use tab like the other macros?
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... PS7, Line 54: = dev->chip_info; : : if (!config) { : printk(BIOS_ERR, "PCH thermal config not found!\n"); : return 0; : } : It would be good to use config_of() which will be going in here: https://review.coreboot.org/c/coreboot/+/34328
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... PS7, Line 62: printk(BIOS_ERR, "PCH trip temp does not contain valid value!\n"); Also add --> "Using default"?
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... PS7, Line 84: PCH_DEV_THERMAL pcidev_path_on_root(PCH_DEVFN_THERMAL);
Hello Patrick Rudolph, Aaron Durbin, Rajat Jain, Patrick Rudolph, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Paul Fagerburg, Tim Wawrzynczak, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#8).
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFA.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/cannonlake/Makefile.inc M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/finalize.c M src/soc/intel/cannonlake/include/soc/iomap.h A src/soc/intel/cannonlake/include/soc/thermal.h A src/soc/intel/cannonlake/thermal.c 6 files changed, 138 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/8
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 8:
(5 comments)
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/in... PS7, Line 71:
nit: Please use tabs like the other macros.
Ack
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... PS7, Line 32: (
nit: use tab like the other macros?
Ack
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... PS7, Line 54: = dev->chip_info; : : if (!config) { : printk(BIOS_ERR, "PCH thermal config not found!\n"); : return 0; : } :
It would be good to use config_of() which will be going in here: https://review.coreboot. […]
Sure. ACK.
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... PS7, Line 62: printk(BIOS_ERR, "PCH trip temp does not contain valid value!\n");
Also add --> "Using default"?
I will add this.
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... PS7, Line 84: PCH_DEV_THERMAL
pcidev_path_on_root(PCH_DEVFN_THERMAL);
I will update with this function call. Thanks.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 8:
As per Jenkins build bot error is for config_of() function and it looks like this depends on CL https://review.coreboot.org/c/coreboot/+/34298/4 and it seems this CL got merged recently. Is it possible to give one more Jenkins build with this recent CL merged ? Thanks.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 9:
I have rebased it on top of master branch. Let's see if jenkins kicks again.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 9:
Patch Set 9:
I have rebased it on top of master branch. Let's see if jenkins kicks again.
Thanks Furquan for rebasing this. Now, jenkins build is successful.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 9: Code-Review+1
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 9: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 9: Code-Review+1
(4 comments)
Overall LGTM. Some minor nits.
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/in... PS9, Line 71: #define THERMAL_BASE_ADDRESS 0xfe600000 Not used anymore.
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... PS9, Line 40: nit: if (!res) return NULL;
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... PS9, Line 87: /* Use default pre-ram bar */ Comment is not true anymore.
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... PS9, Line 89: PCH ERROR: PCH thermalbar not found!
So that it gets caught by test scripts.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 9:
(4 comments)
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/in... PS9, Line 71: #define THERMAL_BASE_ADDRESS 0xfe600000
Not used anymore.
Ack
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... PS9, Line 40:
nit: […]
Sure. Thanks.
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... PS9, Line 87: /* Use default pre-ram bar */
Comment is not true anymore.
Yes, Thanks.
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... PS9, Line 89: PCH
ERROR: PCH thermalbar not found! […]
Sure.
Hello Patrick Rudolph, Aaron Durbin, Rajat Jain, Patrick Rudolph, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Paul Fagerburg, Tim Wawrzynczak, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#10).
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFE.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/cannonlake/Makefile.inc M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/finalize.c A src/soc/intel/cannonlake/include/soc/thermal.h A src/soc/intel/cannonlake/thermal.c 5 files changed, 137 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/10
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 10: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 10: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 10:
(5 comments)
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 34: void * I think returning uintptr_t would be appropriate here.
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 46: (void *) See above.
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 52: static Why is this static? It gets reassigned to the same pointer every time it's called.
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 87: void *thermalbar = pch_thermal_get_bar(dev); See above about uintptr_t
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 96: CATASTROPHIC_TRIP_POINT_VALUE This is used as a mask, could you rename it CATASTROPHIC_TRIP_POINT_MASK ?
Furquan Shaikh has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Removed Code-Review+2 by Furquan Shaikh furquan@google.com
Paul Fagerburg has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Removed Code-Review+1 by Paul Fagerburg pfagerburg@chromium.org
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/cannonlake: set temperature threshold for PCH Thermal Sensor ......................................................................
Patch Set 10:
general suggestion, looks like this CL is similar to what we has in SKL/KBL here https://review.coreboot.org/22419
now it we decided to have such files in CNL and may be going forward every core design then why don't we move generic implementation into "src/soc/intel/common/pch/thermal". this code block meant for all common PCH started from gen6
Hello Patrick Rudolph, Aaron Durbin, Rajat Jain, Patrick Rudolph, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Paul Fagerburg, Tim Wawrzynczak, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#11).
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFE.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h M src/soc/intel/common/pch/Kconfig A src/soc/intel/common/pch/include/thermal.h A src/soc/intel/common/pch/thermal/Kconfig A src/soc/intel/common/pch/thermal/Makefile.inc A src/soc/intel/common/pch/thermal/thermal.c 6 files changed, 132 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/11
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 11:
(5 comments)
Patch Set 10:
general suggestion, looks like this CL is similar to what we has in SKL/KBL here https://review.coreboot.org/22419
now it we decided to have such files in CNL and may be going forward every core design then why don't we move generic implementation into "src/soc/intel/common/pch/thermal". this code block meant for all common PCH started from gen6
Uploaded new patch set with this common code change.
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 34: void *
I think returning uintptr_t would be appropriate here.
Ack. Incorporated in latest patch set.
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 46: (void *)
See above.
Ack
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 52: static
Why is this static? It gets reassigned to the same pointer every time it's called.
Done
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 87: void *thermalbar = pch_thermal_get_bar(dev);
See above about uintptr_t
Ack
https://review.coreboot.org/c/coreboot/+/33129/10/src/soc/intel/cannonlake/t... PS10, Line 96: CATASTROPHIC_TRIP_POINT_VALUE
This is used as a mask, could you rename it CATASTROPHIC_TRIP_POINT_MASK ?
Done
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 11:
in function `pch_thermal_configuration': src/soc/intel/common/pch/thermal/thermal.c:80: multiple definition of `pch_thermal_configuration'
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 11:
(11 comments)
Patch Set 11:
in function `pch_thermal_configuration': src/soc/intel/common/pch/thermal/thermal.c:80: multiple definition of `pch_thermal_configuration'
Note, the moment you have added this thermal kconfig into PCH common kconfig list, your SKL code also started looking into it.
hence we avoid compilation error, you should have remove thermal.c references from SKL code as well in same patch
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/chip.h:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/block... PS11, Line 36: remove line break
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/block... PS11, Line 38: pch_trip_temp pch_thermal_trip ?
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/i... File src/soc/intel/common/pch/include/thermal.h:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/i... PS11, Line 1: /* shouldn't this file move into soc/intel/common/pch/include/intelpch/thermal.h ?
i believe you wish to refer this file from soc directory, if yes then its has to be in include/intelpch.
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/Kconfig:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 5: This option allows to configure PCH thermal registers. append at end of this line "for supported PCH"
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 25: #include "thermal.h" #include <intelblocks/thermal.h>
should be
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 27: either use tab or space, don't mix please
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 58: common_config->pch_trip_temp hold this value into trip_temp
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 62: trip_temp pass it here DEFAULT_TRIP_TEMP directly
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 67: trip_temp = common_config->pch_trip_temp; you can avoid this assignment here
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 74: i guess simpler would be
/* * This function will get pch_thermal_trip config specific to soc. * */ int get_thermal_config(void) { const struct soc_intel_common_config *common_config; common_config = chip_get_common_soc_structure();
return common_config->pch_thermal_trip; }
static uint16_t pch_get_ltt_value(struct device *dev) { uint16_t thermal_config = get_thermal_config(); if (!thermal_config) thermal_config = DEFAULT_TRIP_TEMP;
if (thermal_config > MAX_TRIP_TEMP) die("Input PCH temp trip is higher than allowed range!");
ltt_value = GET_LTT_VALUE(thermal_config);
return ltt_value ; }
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 90: printk(BIOS_ERR, "ERROR: PCH thermalbar not found!\n"); : return; How you are making sure that BAR would have been allocated already ? if you don;t have plan to allocate temp bar then why you have added #include <soc/iomap.h> in include section.
Ideally you should check if BAR there then okay, else assign temp bar and continue rather just return.
Atleast in hatch devicetree.cb i don't see thermal device 00:0x12.0 been listed hence i'm not sure who allocates the BAR here ? if no one assign BAR then this code won't solve your purpose.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 74:
i guess simpler would be […]
make get_thermal_config one static as its been local already
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 11:
(7 comments)
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/chip.h:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/block... PS11, Line 38: pch_trip_temp
pch_thermal_trip ?
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/i... File src/soc/intel/common/pch/include/thermal.h:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/i... PS11, Line 1: /*
shouldn't this file move into soc/intel/common/pch/include/intelpch/thermal.h ? […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/Kconfig:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 5: This option allows to configure PCH thermal registers.
append at end of this line "for supported PCH"
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 25: #include "thermal.h"
#include <intelblocks/thermal.h> […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 27:
either use tab or space, don't mix please
After define string I use space and after the macro name I use tab in this file.
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 74:
make get_thermal_config one static as its been local already
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 90: printk(BIOS_ERR, "ERROR: PCH thermalbar not found!\n"); : return;
How you are making sure that BAR would have been allocated already ? if you don;t have plan to alloc […]
00:0x12.0 being used in https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/...
Hello Patrick Rudolph, Aaron Durbin, Rajat Jain, Subrata Banik, Patrick Rudolph, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Paul Fagerburg, Tim Wawrzynczak, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#12).
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFE.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h A src/soc/intel/common/pch/include/intelpch/thermal.h A src/soc/intel/common/pch/thermal/Kconfig A src/soc/intel/common/pch/thermal/Makefile.inc A src/soc/intel/common/pch/thermal/thermal.c 5 files changed, 126 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/12
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 12: Code-Review-1
(6 comments)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 33: int pch_thermal_trip is defined as uint8_t. Is there a good reason to promote it to an int?
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 45: uint16_t thermal_config = get_thermal_config(); And here you're casting the uint8_t to an int to a uint16_t
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 78: struct device *dev = pcidev_path_on_root(PCH_DEVFN_THERMAL); Prefer separating declaration from assignment, please.
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 84: uintptr_t Prefer variable definitions at the top of the scope, please.
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 91: void *)thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT); Pointer arithmetic on void* pointers is illegal in C.
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 95: (void *)thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT Pointer arithmetic on void* pointers is illegal in C.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 90: printk(BIOS_ERR, "ERROR: PCH thermalbar not found!\n"); : return;
00:0x12.0 being used in https://review.coreboot. […]
i still prefers to have temp bar assignment rather expecting mb user will always enbale thermal device
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 90: printk(BIOS_ERR, "ERROR: PCH thermalbar not found!\n"); : return;
i still prefers to have temp bar assignment rather expecting mb user will always enbale thermal devi […]
A proper solution would be valid devicetree.cbs and pci ops for the thermal device, instead of adding more hacks like temp bar assignment.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 90: printk(BIOS_ERR, "ERROR: PCH thermalbar not found!\n"); : return;
A proper solution would be valid devicetree. […]
IMO, in bios world tempbar is not the hack. Why BIOS should do another PCI device enumeration when it doesn't have any business with it. For sure BIOS is not going to use this thermal device during POST. Its just to configure a device register(THERMAL_SENSOR_POWER_MANAGEMENT) and leave that device.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12: I am curious to know the reasoning why this belongs under common/pch and not common/block? I would think of the thermal device as another IP block.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
I am curious to know the reasoning why this belongs under common/pch and not common/block? I would t […]
i don't know if this feature already existed for atom families. As sumeet was porting SKL/KBL code into CNL/CML hence i have asked to move into common/pch because common/pch actually meant to keep anything common between all core family PCH.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
i don't know if this feature already existed for atom families. […]
That does not make sense to me nor was I aware that was the distinction in the directory structure. That's the first time I've seen this explicitly stated. PCH are shared across lines, and I don't think it's helpful to create an artificial difference there.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
That does not make sense to me nor was I aware that was the distinction in the directory structure. […]
PCH terminology typically doesn't exist for atom line. idea behind PCH directory structure to make exclusive common code based on PCH alignment like SPT, CNP, CMP, ICP, TGP etc.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
PCH terminology typically doesn't exist for atom line. […]
Even if an IP block is not used by a particular family, it should be part of common/block and the SoC can decide whether or not that block is present on the SoC which is controlled by the Kconfig. Isn't that right?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
Even if an IP block is not used by a particular family, it should be part of common/block and the So […]
thats the correct expectation Furquan but we were also assessing how do we make more common code based on PCH common alignment, if you see pch/kconfig is now selecting all those common IP from single place rather duplicating those into core platform (SKL, CNL, ICL etc) kconfig like atom socs. It minimizes the maintain headache and also avoid unnecessary common approach which is not common between core and atom. (that was the key idea for pch directory)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
thats the correct expectation Furquan but we were also assessing how do we make more common code bas […]
I don't completely agree that adding this IP to common/block would be a maintenance issue. It keeps all IPs together. Again, you can always select SOC_INTEL_COMMON_BLOCK_THERMAL in pch/Kconfig like you have been doing for all the other IPs in there.
In my opinion, splitting IPs to be under common/block and common/pch would end up creating more confusion.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 12:
(8 comments)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
I don't completely agree that adding this IP to common/block would be a maintenance issue. […]
yes Furquan, i agree on logical splitting part, but here thermal is not treated as in general IP like other existed into common/block. And taking part into PCI emueration, its kind of chipset programming specific to thermal register.
I'm okay if you are advocating about moving this module from common/pch to common/block. But my point is that if you like to actually try to make further common code based on PCH alignment which is reasonable might exist inside common/pch. In you see long back, intel CPU used to get common code based on southbridge common.
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 33: int
pch_thermal_trip is defined as uint8_t. […]
Make it to uint8_t as pch_thermal_trip static uint8_t get_thermal_config(void)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 45: uint16_t thermal_config = get_thermal_config();
And here you're casting the uint8_t to an int to a uint16_t
uint8_t thermal_config = get_thermal_config();
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 78: struct device *dev = pcidev_path_on_root(PCH_DEVFN_THERMAL);
Prefer separating declaration from assignment, please.
struct device *dev; uintptr_t thermalbar;
dev = pcidev_path_on_root(PCH_DEVFN_THERMAL); if (!dev) { printk(BIOS_ERR, "ERROR: PCH_DEVFN_THERMAL device not found!\n"); return; }
thermalbar = pch_thermal_get_bar(dev);
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 84: uintptr_t
Prefer variable definitions at the top of the scope, please.
look at above
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 89: or you can also create a helper function like below
static uint16_t read16p(uintptr_t addr) { return read16((void *)addr); }
reg16 = read16p(thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT);
in that way you can avoid creating temp variable as thermalbar_pm
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 91: void *)thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT);
Pointer arithmetic on void* pointers is illegal in C.
you can create a local variable for thermal_pm
uintptr_t thermalbar_pm = thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT; reg16 = read16((void *)thermalbar_pm);
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 95: (void *)thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT
Pointer arithmetic on void* pointers is illegal in C.
write16((void *)thermalbar_pm, reg16);
Hello Aaron Durbin, Patrick Rudolph, Rajat Jain, Subrata Banik, Patrick Rudolph, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Paul Fagerburg, Tim Wawrzynczak, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#13).
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFE.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h A src/soc/intel/common/pch/include/intelpch/thermal.h A src/soc/intel/common/pch/thermal/Kconfig A src/soc/intel/common/pch/thermal/Makefile.inc A src/soc/intel/common/pch/thermal/thermal.c 5 files changed, 139 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/13
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
But my point is that if you like to actually try to make further common code based on PCH alignment which is reasonable might exist inside common/pch.
What kind of common code? Why does having it in block make it difficult?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
But my point is that if you like to actually try to make further common code based on PCH alignment which is reasonable might exist inside common/pch.
What kind of common code?
might be building soc/intel code further around PCH, like surrounding icl pch may be if we wish to do to accommodate future soc?
Why does having it in block make it difficult?
common/block is dedicated to IPs and inside soc/intel directory we might have some common stage files as well like finalize.c, elog.c, smihandler.c, smmrelocation.c across all core family ? just a initial thought! similar effort might help us to land newer soc code.
Hello Aaron Durbin, Patrick Rudolph, Rajat Jain, Subrata Banik, Patrick Rudolph, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Paul Fagerburg, Tim Wawrzynczak, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#14).
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFE.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h A src/soc/intel/common/pch/include/intelpch/thermal.h A src/soc/intel/common/pch/thermal/Kconfig A src/soc/intel/common/pch/thermal/Makefile.inc A src/soc/intel/common/pch/thermal/thermal.c 5 files changed, 144 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/14
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
PCH terminology typically doesn't exist for atom line. idea behind PCH directory structure to make exclusive common code based on PCH alignment like SPT, CNP, CMP, ICP, TGP etc.
I don't believe it matters if some specific terminology is used or not. The IP block reuse is what matters. Moreover, monolithic dies in atom line are things of the past. There is inherently a pch in the past 2 parts on the atom line. But even if there wasn't -- the ip block reuse is the key bit and we should be organizing code in that manner.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 14:
(6 comments)
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 33: int
Make it to uint8_t as pch_thermal_trip […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 45: uint16_t thermal_config = get_thermal_config();
uint8_t thermal_config = get_thermal_config();
Done
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 78: struct device *dev = pcidev_path_on_root(PCH_DEVFN_THERMAL);
struct device *dev; […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 84: uintptr_t
look at above
Done
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 91: void *)thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT);
you can create a local variable for thermal_pm […]
Done
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 95: (void *)thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT
write16((void *)thermalbar_pm, reg16);
Done
Hello Aaron Durbin, Patrick Rudolph, Rajat Jain, Subrata Banik, Patrick Rudolph, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Paul Fagerburg, Tim Wawrzynczak, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#15).
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFE.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h A src/soc/intel/common/pch/include/intelpch/thermal.h A src/soc/intel/common/pch/thermal/Kconfig A src/soc/intel/common/pch/thermal/Makefile.inc A src/soc/intel/common/pch/thermal/thermal.c 5 files changed, 140 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/15
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/33129/15/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/15/src/soc/intel/common/pch/t... PS15, Line 29: static uint16_t read16p(uintptr_t addr) : { : return read16((void *)addr); : } : : static void write16p(uintptr_t addr, uint16_t val) : { : write16((void *)addr, val); : } These are not necessary. A void pointer is allowed to alias any type, so below you can just call read16(...), and write16(...).
https://review.coreboot.org/c/coreboot/+/33129/15/src/soc/intel/common/pch/t... PS15, Line 105: read16p See above.
https://review.coreboot.org/c/coreboot/+/33129/15/src/soc/intel/common/pch/t... PS15, Line 109: write16p See above.
Hello Aaron Durbin, Patrick Rudolph, Rajat Jain, Subrata Banik, Patrick Rudolph, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Paul Fagerburg, Tim Wawrzynczak, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#16).
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFE.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h A src/soc/intel/common/pch/include/intelpch/thermal.h A src/soc/intel/common/pch/thermal/Kconfig A src/soc/intel/common/pch/thermal/Makefile.inc A src/soc/intel/common/pch/thermal/thermal.c 5 files changed, 130 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/16
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
PCH terminology typically doesn't exist for atom line. […]
I agree with what Aaron said above. Let's think of this as IP block reuse. It should provide the same level of sharing for current and future SoCs by just having to enable a single Kconfig.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
I agree with what Aaron said above. Let's think of this as IP block reuse. […]
got it. lets move this code into common/block from common/pch
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33129/15/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/15/src/soc/intel/common/pch/t... PS15, Line 105: read16p
See above.
yes, Tim's point is valid as now you already has thermalbar_pm then we can get rid of read16p and directly use read16 with void* cast
Hello Aaron Durbin, Patrick Rudolph, Rajat Jain, Subrata Banik, Patrick Rudolph, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Paul Fagerburg, Tim Wawrzynczak, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#17).
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFE.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h A src/soc/intel/common/block/include/intelblocks/thermal.h A src/soc/intel/common/block/thermal/Kconfig A src/soc/intel/common/block/thermal/Makefile.inc A src/soc/intel/common/block/thermal/thermal.c 5 files changed, 130 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/17
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 17: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 17:
(5 comments)
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... File src/soc/intel/common/block/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 29: get_thermal_config Maybe rename this to get_thermal_trip_temp ?
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 41: thermal_config; : : thermal_config = get_thermal_config(); see above, maybe rename variable to thermal_trip_temp ?
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 56: pch_thermal_get_bar Since this function is local to this file, and its only purpose is to get the BAR of the thermal device, does it really need to be passed the device struct? It could take no parameters, and just get 'dev = pcidev_path_on_root(PCH_DEVFN_THERMAL)' itself.
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 95: void nit: consider uint16_t*
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 99: void nit: consider uint16_t*
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 17:
(6 comments)
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... File src/soc/intel/common/block/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 29: get_thermal_config
Maybe rename this to get_thermal_trip_temp ?
Ack
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 41: thermal_config; : : thermal_config = get_thermal_config();
see above, maybe rename variable to thermal_trip_temp ?
Ack
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 56: pch_thermal_get_bar
Since this function is local to this file, and its only purpose is to get the BAR of the thermal dev […]
Looking at further optimization, thinking of removing this function pch_thermal_get_bar(). We can directly call 'res = find_resource(dev, PCI_BASE_ADDRESS_0)' in function pch_thermal_configuration() at below line 85.
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 84: : thermalbar = pch_thermal_get_bar(dev); : if (!thermalbar) { : printk(BIOS_ERR, "ERROR: PCH thermalbar not found!\n"); : return; : } Adding below code snippet as discussed in above comment at line 56.
+ struct resource *res; - thermalbar = pch_thermal_get_bar(); - if (!thermalbar) { + res = find_resource(dev, PCI_BASE_ADDRESS_0); + if (!res) { printk(BIOS_ERR, "ERROR: PCH thermalbar not found!\n"); return; } + /* Get the base address of the resource */ + thermalbar = res->base;
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 95: void
nit: consider uint16_t*
Ack
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 99: void
nit: consider uint16_t*
Ack
Hello Aaron Durbin, Patrick Rudolph, Rajat Jain, Subrata Banik, Patrick Rudolph, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Paul Fagerburg, Tim Wawrzynczak, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33129
to look at the new patch set (#18).
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFE.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h A src/soc/intel/common/block/include/intelblocks/thermal.h A src/soc/intel/common/block/thermal/Kconfig A src/soc/intel/common/block/thermal/Makefile.inc A src/soc/intel/common/block/thermal/thermal.c 5 files changed, 119 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/33129/18
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 18: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 18: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 18: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 18:
Sumeet - you will have to mark all unresolved comments on all patchsets as resolved in order for gerrit UI to show the Submit button.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 18:
(7 comments)
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... File src/soc/intel/common/block/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 29: get_thermal_config
Ack
Done
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 41: thermal_config; : : thermal_config = get_thermal_config();
Ack
Done
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 56: pch_thermal_get_bar
Looking at further optimization, thinking of removing this function pch_thermal_get_bar(). […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/17/src/soc/intel/common/block... PS17, Line 84: : thermalbar = pch_thermal_get_bar(dev); : if (!thermalbar) { : printk(BIOS_ERR, "ERROR: PCH thermalbar not found!\n"); : return; : }
Adding below code snippet as discussed in above comment at line 56. […]
Done
https://review.coreboot.org/c/coreboot/+/33129/15/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/15/src/soc/intel/common/pch/t... PS15, Line 29: static uint16_t read16p(uintptr_t addr) : { : return read16((void *)addr); : } : : static void write16p(uintptr_t addr, uint16_t val) : { : write16((void *)addr, val); : }
These are not necessary. […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/15/src/soc/intel/common/pch/t... PS15, Line 105: read16p
yes, Tim's point is valid as now you already has thermalbar_pm then we can get rid of read16p and d […]
Done
https://review.coreboot.org/c/coreboot/+/33129/15/src/soc/intel/common/pch/t... PS15, Line 109: write16p
See above.
Done
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 18:
(26 comments)
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/ch... PS3, Line 425: /* PCH Trip Temperature */
Does it make sense to add unit in this comment part ?
Done
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/thermal.h:
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/in... PS3, Line 19: #define THERMAL_SENSOR_POWER_MANAGEMENT 0x1c
Why does this have to be in the .h file? Its used only within thermal. […]
Done
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/th... PS3, Line 78: ltt_value = (trip_temp + 50) * 2;
Macros to calculate this would be helpful; you could also then use them to define MAX_TRIP_TEMP, and […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/th... PS3, Line 87: struct
We are modifying the register values below for this structure. […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/th... PS3, Line 104: pch_get_ltt_value(dev);
Shouldn't this also be enabling bits 13 and 14? i.e. […]
Done
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 28: #define MAX_TRIP_TEMP 205
This is the worst case value writing 1 to all [8:0] bits and calculated based on below formula, […]
Done
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 31: trip_temp
Sure, we will use this way. Thanks for suggestion.
Done
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 37: bar = pci_read_config32(dev, PCI_BASE_ADDRESS_0);
To use find_resource(), we are planning to use below code […]
Done
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 74: if (config->pch_trip_temp)
We will check this condition and print an error message.
Done
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 78: die("Input PCH temp trip is higher than allowed range!");
worst case (MAX) value writing 1 to all [8:0] bits ie 0x1ff. […]
Updated accordingly.
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 98: if (!thermalbar) {
Sure, we will implement this way. I have tested and it works.
Ack
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 27: #define THERMAL_SENSOR_POWER_MANAGEMENT 0x1c : #define MAX_TRIP_TEMP 205 : /* This is the safest default Trip Temp value */ : #define DEFAULT_TRIP_TEMP 50 : #define GET_LTT_VALUE(x) ((x + 50) * 2)
Can you line these up together?
Done
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 31: #define GET_LTT_VALUE(x) ((x + 50) * 2)
Macro parameters should be parenthesized: (((x) + 50) * 2)
Done
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 59: if (!config->pch_trip_temp) {
We will add one more if to check is config NULL or not. If yes, print error message and return.
Ack
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 90: if (!thermalbar)
Sure. Thanks for this finding.
Ack
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... PS7, Line 62: printk(BIOS_ERR, "PCH trip temp does not contain valid value!\n");
I will add this.
Done
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... PS7, Line 84: PCH_DEV_THERMAL
I will update with this function call. Thanks.
Ack
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... PS9, Line 40:
Sure. Thanks.
Ack
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... PS9, Line 87: /* Use default pre-ram bar */
Yes, Thanks.
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/chip.h:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/block... PS11, Line 36:
remove line break
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 27:
After define string I use space and after the macro name I use tab in this file.
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 58: common_config->pch_trip_temp
hold this value into trip_temp
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 62: trip_temp
pass it here DEFAULT_TRIP_TEMP directly
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 67: trip_temp = common_config->pch_trip_temp;
you can avoid this assignment here
Done
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 90: printk(BIOS_ERR, "ERROR: PCH thermalbar not found!\n"); : return;
IMO, in bios world tempbar is not the hack. […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
got it. […]
Done
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 18:
(6 comments)
https://review.coreboot.org/c/coreboot/+/33129/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33129/3//COMMIT_MSG@7 PS3, Line 7: soc/intel/cannonlake: Set correct temperature threshold for PCH Thermal Sensor
s/Set correct/correct/ is shorter.
Done
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/th... PS3, Line 27: #define MAX_TRIP_TEMP 205
I will add below comment for this define. […]
Done
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/th... PS3, Line 28: #define DEFAULT_TRIP_TEMP 50
This is the safest Trip Temp value.
Done
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... PS7, Line 54: = dev->chip_info; : : if (!config) { : printk(BIOS_ERR, "PCH thermal config not found!\n"); : return 0; : } :
Sure. ACK.
Done
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... PS9, Line 89: PCH
Sure.
Done
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... PS12, Line 89:
or you can also create a helper function like below […]
Done
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 18:
Patch Set 18:
Sumeet - you will have to mark all unresolved comments on all patchsets as resolved in order for gerrit UI to show the Submit button.
Done. Thanks.
Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal shutdown when S0ix is enabled.
BUG=None BRANCH=None TEST=Verified Thermal Device (B0: D18: F0) TSPM offset 0x1c [LTT (8:0)] value is 0xFE.
Change-Id: Ibd1e669fcbfe8dc6e6e5556aa5b1373ed19c3685 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33129 Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Subrata Banik subrata.banik@intel.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/common/block/include/intelblocks/chip.h A src/soc/intel/common/block/include/intelblocks/thermal.h A src/soc/intel/common/block/thermal/Kconfig A src/soc/intel/common/block/thermal/Makefile.inc A src/soc/intel/common/block/thermal/thermal.c 5 files changed, 119 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Subrata Banik: Looks good to me, approved Tim Wawrzynczak: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/common/block/include/intelblocks/chip.h b/src/soc/intel/common/block/include/intelblocks/chip.h index 555bdaa..9fe165e 100644 --- a/src/soc/intel/common/block/include/intelblocks/chip.h +++ b/src/soc/intel/common/block/include/intelblocks/chip.h @@ -33,6 +33,8 @@ int chipset_lockdown; struct gspi_cfg gspi[CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX]; struct dw_i2c_bus_config i2c[CONFIG_SOC_INTEL_I2C_DEV_MAX]; + /* PCH Thermal Trip Temperature in deg C */ + uint8_t pch_thermal_trip; };
/* This function to retrieve soc config structure required by common code */ diff --git a/src/soc/intel/common/block/include/intelblocks/thermal.h b/src/soc/intel/common/block/include/intelblocks/thermal.h new file mode 100644 index 0000000..ab18eb6 --- /dev/null +++ b/src/soc/intel/common/block/include/intelblocks/thermal.h @@ -0,0 +1,22 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Intel Corp. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _SOC_INTEL_COMMON_BLOCK_THERMAL_H_ +#define _SOC_INTEL_COMMON_BLOCK_THERMAL_H_ + +/* Enable thermal sensor power management */ +void pch_thermal_configuration(void); + +#endif diff --git a/src/soc/intel/common/block/thermal/Kconfig b/src/soc/intel/common/block/thermal/Kconfig new file mode 100644 index 0000000..0605176 --- /dev/null +++ b/src/soc/intel/common/block/thermal/Kconfig @@ -0,0 +1,5 @@ +config SOC_INTEL_COMMON_BLOCK_THERMAL + bool + default n + help + This option allows to configure PCH thermal registers for supported PCH. diff --git a/src/soc/intel/common/block/thermal/Makefile.inc b/src/soc/intel/common/block/thermal/Makefile.inc new file mode 100644 index 0000000..6f216b3 --- /dev/null +++ b/src/soc/intel/common/block/thermal/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_THERMAL) += thermal.c diff --git a/src/soc/intel/common/block/thermal/thermal.c b/src/soc/intel/common/block/thermal/thermal.c new file mode 100644 index 0000000..39a98a4 --- /dev/null +++ b/src/soc/intel/common/block/thermal/thermal.c @@ -0,0 +1,89 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Intel Corp. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <console/console.h> +#include <device/mmio.h> +#include <intelblocks/chip.h> +#include <intelblocks/thermal.h> +#include <soc/pci_devs.h> + +#define THERMAL_SENSOR_POWER_MANAGEMENT 0x1c +#define CATASTROPHIC_TRIP_POINT_MASK 0x1ff +#define MAX_TRIP_TEMP 205 +/* This is the safest default Trip Temp value */ +#define DEFAULT_TRIP_TEMP 50 +#define GET_LTT_VALUE(x) (((x) + 50) * (2)) + +static uint8_t get_thermal_trip_temp(void) +{ + const struct soc_intel_common_config *common_config; + common_config = chip_get_common_soc_structure(); + + return common_config->pch_thermal_trip; +} + +/* PCH Low Temp Threshold (LTT) */ +static uint16_t pch_get_ltt_value(struct device *dev) +{ + uint16_t ltt_value; + uint8_t thermal_config; + + thermal_config = get_thermal_trip_temp(); + if (!thermal_config) + thermal_config = DEFAULT_TRIP_TEMP; + + if (thermal_config > MAX_TRIP_TEMP) + die("Input PCH temp trip is higher than allowed range!"); + + /* Trip Point Temp = (LTT / 2 - 50 degree C) */ + ltt_value = GET_LTT_VALUE(thermal_config); + + return ltt_value; +} + +/* Enable thermal sensor power management */ +void pch_thermal_configuration(void) +{ + uint16_t reg16; + uintptr_t thermalbar; + uintptr_t thermalbar_pm; + struct device *dev; + struct resource *res; + + dev = pcidev_path_on_root(PCH_DEVFN_THERMAL); + if (!dev) { + printk(BIOS_ERR, "ERROR: PCH_DEVFN_THERMAL device not found!\n"); + return; + } + + res = find_resource(dev, PCI_BASE_ADDRESS_0); + if (!res) { + printk(BIOS_ERR, "ERROR: PCH thermal device not found!\n"); + return; + } + + /* Get the base address of the resource */ + thermalbar = res->base; + + /* Get the required thermal address to write the register value */ + thermalbar_pm = thermalbar + THERMAL_SENSOR_POWER_MANAGEMENT; + + /* Set Low Temp Threshold (LTT) at TSPM offset 0x1c[8:0] */ + reg16 = read16((uint16_t *)thermalbar_pm); + reg16 &= ~CATASTROPHIC_TRIP_POINT_MASK; + /* Low Temp Threshold (LTT) */ + reg16 |= pch_get_ltt_value(dev); + write16((uint16_t *)thermalbar_pm, reg16); +}
Roy Mingi Park has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 19: Code-Review+1
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Martin Roth (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Fagerburg (Code Review)
-
Paul Menzel (Code Review)
-
Roy Mingi Park (Code Review)
-
Subrata Banik (Code Review)
-
Sumeet R Pawnikar (Code Review)
-
Tim Wawrzynczak (Code Review)