Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/block: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 18:
(26 comments)
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/ch... PS3, Line 425: /* PCH Trip Temperature */
Does it make sense to add unit in this comment part ?
Done
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/thermal.h:
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/in... PS3, Line 19: #define THERMAL_SENSOR_POWER_MANAGEMENT 0x1c
Why does this have to be in the .h file? Its used only within thermal. […]
Done
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/th... PS3, Line 78: ltt_value = (trip_temp + 50) * 2;
Macros to calculate this would be helpful; you could also then use them to define MAX_TRIP_TEMP, and […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/th... PS3, Line 87: struct
We are modifying the register values below for this structure. […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/3/src/soc/intel/cannonlake/th... PS3, Line 104: pch_get_ltt_value(dev);
Shouldn't this also be enabling bits 13 and 14? i.e. […]
Done
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
This is the worst case value writing 1 to all [8:0] bits and calculated based on below formula, […]
Done
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 31: trip_temp
Sure, we will use this way. Thanks for suggestion.
Done
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);
To use find_resource(), we are planning to use below code […]
Done
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 74: if (config->pch_trip_temp)
We will check this condition and print an error message.
Done
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!");
worst case (MAX) value writing 1 to all [8:0] bits ie 0x1ff. […]
Updated accordingly.
https://review.coreboot.org/c/coreboot/+/33129/5/src/soc/intel/cannonlake/th... PS5, Line 98: if (!thermalbar) {
Sure, we will implement this way. I have tested and it works.
Ack
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 27: #define THERMAL_SENSOR_POWER_MANAGEMENT 0x1c : #define MAX_TRIP_TEMP 205 : /* This is the safest default Trip Temp value */ : #define DEFAULT_TRIP_TEMP 50 : #define GET_LTT_VALUE(x) ((x + 50) * 2)
Can you line these up together?
Done
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 31: #define GET_LTT_VALUE(x) ((x + 50) * 2)
Macro parameters should be parenthesized: (((x) + 50) * 2)
Done
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 59: if (!config->pch_trip_temp) {
We will add one more if to check is config NULL or not. If yes, print error message and return.
Ack
https://review.coreboot.org/c/coreboot/+/33129/6/src/soc/intel/cannonlake/th... PS6, Line 90: if (!thermalbar)
Sure. Thanks for this finding.
Ack
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... PS7, Line 62: printk(BIOS_ERR, "PCH trip temp does not contain valid value!\n");
I will add this.
Done
https://review.coreboot.org/c/coreboot/+/33129/7/src/soc/intel/cannonlake/th... PS7, Line 84: PCH_DEV_THERMAL
I will update with this function call. Thanks.
Ack
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... File src/soc/intel/cannonlake/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... PS9, Line 40:
Sure. Thanks.
Ack
https://review.coreboot.org/c/coreboot/+/33129/9/src/soc/intel/cannonlake/th... PS9, Line 87: /* Use default pre-ram bar */
Yes, Thanks.
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/chip.h:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/block... PS11, Line 36:
remove line break
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 27:
After define string I use space and after the macro name I use tab in this file.
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 58: common_config->pch_trip_temp
hold this value into trip_temp
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 62: trip_temp
pass it here DEFAULT_TRIP_TEMP directly
Ack
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 67: trip_temp = common_config->pch_trip_temp;
you can avoid this assignment here
Done
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 90: printk(BIOS_ERR, "ERROR: PCH thermalbar not found!\n"); : return;
IMO, in bios world tempbar is not the hack. […]
Ack
https://review.coreboot.org/c/coreboot/+/33129/12/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/thermal.c:
PS12:
got it. […]
Done