Attention is currently required from: Maulik V Vaghela, Subrata Banik, Patrick Rudolph. 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 8:
(3 comments)
File src/soc/intel/common/block/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/59209/comment/443102ab_5ed4d8e5 PS8, Line 57: static void thermal_or32(uintptr_t addr, uint32_t ordata) : { : uint32_t data32; : : data32 = read32p(addr); : data32 |= ordata; : write32p(addr, data32); : } `setbits32` ?
https://review.coreboot.org/c/coreboot/+/59209/comment/24286a3e_163eceb3 PS8, Line 112: 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, : }, : }; Is this really necessary for ~10 10 lines of code ?
``` setbits32(pmc_bar + PMC_PWRM_THERMAL_CTEN, PMC_PWRM_THERMAL_CTEN_CPDEN | PMC_PWRM_THERMAL_CTEN_CTENLOCK); setbits32(pmc_bar + PMC_PWRM_THERMAL_ECRPTEN, PMC_PWRM_THERMAL_ECRPTEN_EN_RPT | PMC_PWRM_THERMAL_ECRPTEN_ECRPTENLOCK); ``` etc. ?
Also should any others of these settings be configurable, e.g. EC thermal reporting?
https://review.coreboot.org/c/coreboot/+/59209/comment/0dd56b87_162f942e 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); : } : just thinking out loud, should this go in src/soc/intel/common/block/pmclib.c? then alderlake & tigerlake can just call it from their `soc_pmc_init()` function? Since there is no separate thermal device anymore? WDYT?