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?