Attention is currently required from: Maulik V Vaghela, Tim Wawrzynczak, Patrick Rudolph, EricR Lai. Subrata Banik 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:
(2 comments)
File src/soc/intel/common/block/thermal/thermal.c:
https://review.coreboot.org/c/coreboot/+/59209/comment/00fe669b_2d11d850 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. ?\
This can be done for sure, even at first patch set I have done the same but for few reason, I have prefer to create a structure as below:
1. More than one bit fields are getting set and with such bigger macro names, its difficult to follow what all bits we are programming into which register?
2. Making register offset clearly visible to know which offset we are programming.
3. Although there are majority of programing to set/reset certain bits but we might need to perform thermal threshold level write into the register hence wish to make it clear the underlying operations.
TL.T2L[28:20]: Program to the Throttle 2 temperature threshold level. TL.T1L[18:10]: Program to the Throttle 1 temperature threshold level. TL.T0L[8:0]: Program to the Throttle 0 temperature threshold level.
looks like we don't need helper function thermal_rmw32/thermal_or32 as we could do the same using clrsetbits32 and setbits32. Please let me know if you still prefers to program as suggested by you. I don't mind to change the way its being done, as all does the same register programing with even or little improvement of code readability đ
Also should any others of these settings be configurable, e.g. EC thermal reporting?
Please take a look into https://review.coreboot.org/c/coreboot/+/59209/8/src/soc/intel/common/block/...
// Enable thermal reporting to an EC over SMBus or eSPI and PMC. // Set bit[0] of ECRPTEN register to 1.
https://review.coreboot.org/c/coreboot/+/59209/comment/0ce69d68_1a63a7be 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); : } :
That could work, if this will be part of PMC in the future. [âŚ]
I kind of agree with both of you. @Tim, you are right that there is no dedicated thermal device from TGL so maybe move this code into PMC and call it separately. @Eric, for now what visibility I have in the codebase, looks like thermal configuration going to stay behind PMC for longer đ
The only reason, I have thought prior to moving the thermal configuration for latest SOC even there is no thermal device are:
1. Maintain a programming parity with its purpose rather than the actual device. While debugging if we are seeing any thermal related issue, it would be easy to just grep "*thermal*.c" rather pmc.c đ
2. Also, you can understand the amount of code reusability we could achieve irrespective of whether we have a dedicated thermal device or device register behind PMC. The underlying thermal threshold logic or getting the thermal configuration value from different mb code, we could easily reuse from here example: get_thermal_trip_temp() and pch_get_ltt_value() etc.
3. Making a unified call irrespective of different SoC can land into pch_thermal_configuration() would help to make us understand the change in thermal configuration from SOC side over generation and we could share the feedback to the SoC team about good or bad in this change.
The only downside is the W/A that we have here: https://review.coreboot.org/c/coreboot/+/59209/8/src/soc/intel/common/block/...
I believe for the benefit of making things common (meaningfully) we can pay that much. WDYT?