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 ?