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.