Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33129 )
Change subject: soc/intel/common/pch: Enable PCH Thermal Sensor for threshold configuration ......................................................................
Patch Set 11:
(11 comments)
Patch Set 11:
in function `pch_thermal_configuration': src/soc/intel/common/pch/thermal/thermal.c:80: multiple definition of `pch_thermal_configuration'
Note, the moment you have added this thermal kconfig into PCH common kconfig list, your SKL code also started looking into it.
hence we avoid compilation error, you should have remove thermal.c references from SKL code as well in same patch
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
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/block... PS11, Line 38: pch_trip_temp pch_thermal_trip ?
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/i... File src/soc/intel/common/pch/include/thermal.h:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/i... PS11, Line 1: /* shouldn't this file move into soc/intel/common/pch/include/intelpch/thermal.h ?
i believe you wish to refer this file from soc directory, if yes then its has to be in include/intelpch.
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... File src/soc/intel/common/pch/thermal/Kconfig:
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 5: This option allows to configure PCH thermal registers. append at end of this line "for supported PCH"
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 25: #include "thermal.h" #include <intelblocks/thermal.h>
should be
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 27: either use tab or space, don't mix please
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
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
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
https://review.coreboot.org/c/coreboot/+/33129/11/src/soc/intel/common/pch/t... PS11, Line 74: i guess simpler would be
/* * This function will get pch_thermal_trip config specific to soc. * */ int get_thermal_config(void) { const struct soc_intel_common_config *common_config; common_config = chip_get_common_soc_structure();
return common_config->pch_thermal_trip; }
static uint16_t pch_get_ltt_value(struct device *dev) { uint16_t thermal_config = get_thermal_config(); if (!thermal_config) thermal_config = DEFAULT_TRIP_TEMP;
if (thermal_config > MAX_TRIP_TEMP) die("Input PCH temp trip is higher than allowed range!");
ltt_value = GET_LTT_VALUE(thermal_config);
return ltt_value ; }
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; How you are making sure that BAR would have been allocated already ? if you don;t have plan to allocate temp bar then why you have added #include <soc/iomap.h> in include section.
Ideally you should check if BAR there then okay, else assign temp bar and continue rather just return.
Atleast in hatch devicetree.cb i don't see thermal device 00:0x12.0 been listed hence i'm not sure who allocates the BAR here ? if no one assign BAR then this code won't solve your purpose.