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.