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.