Attention is currently required from: Maulik V Vaghela, Subrata Banik, Patrick Rudolph, EricR Lai. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59209 )
Change subject: soc/intel/../thermal: Add support for thermal config behind PMC device ......................................................................
Patch Set 10:
(1 comment)
File src/soc/intel/common/block/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/59209/comment/401eced1_72fdd1c2 PS8, Line 106: /* Enable thermal sensor power management using PMC PCH device */ : static void pch_pmc_thermal_configuration(void) : { : struct device *dev = pcidev_path_on_root(SA_DEVFN_ROOT); : uintptr_t pmc_bar = soc_read_pmc_base(); : : struct pmc_thermal_config { : void (*func)(uintptr_t addr, uint32_t data); : uint16_t offset; : uint32_t value; : } config[] = { : { : .func = thermal_or32, : .offset = PMC_PWRM_THERMAL_CTEN, : .value = PMC_PWRM_THERMAL_CTEN_CPDEN | PMC_PWRM_THERMAL_CTEN_CTENLOCK, : }, : { : .func = thermal_or32, : .offset = PMC_PWRM_THERMAL_ECRPTEN, : .value = PMC_PWRM_THERMAL_ECRPTEN_EN_RPT : | PMC_PWRM_THERMAL_ECRPTEN_ECRPTENLOCK, : }, : { : .func = write32p, : .offset = PMC_PWRM_THERMAL_TL, : .value = pch_get_ltt_value(dev) | PMC_PWRM_THERMAL_TL_TTEN : | PMC_PWRM_THERMAL_TL_TLLOCK, : }, : { : .func = thermal_or32, : .offset = PMC_PWRM_THERMAL_PHLC, : .value = PMC_PWRM_THERMAL_PHLC_PHLCLOCK, : }, : { : .func = thermal_or32, : .offset = PMC_PWRM_THERMAL_TLEN, : .value = PMC_PWRM_THERMAL_TLEN_TLENLOCK, : }, : }; : : for (int i = 0; i < ARRAY_SIZE(config); i++) : config[i].func(pmc_bar + config[i].offset, config[i].value); : } :
@Tim, any further thoughts ?
I am really not liking the #define PCH_DEVFN_THERMAL DEADCODE thing...
Here's my suggestion: 1) Add a new file thermal-pmc.c here in this folder 2) thermal.c and thermal-pmc.c both provide the same public interface, i.e., the functions exported in intelblocks/thermal.h, but their implementation can be different, and then the Makefile.inc just builds the correct one depending on SOC_INTEL_COMMON_BLOCK_THERMAL_BEHIND_PMC. They have different register layouts for LTT, etc. so instead of just using the preprocessor everywhere, a separate file would be cleaner.