Attention is currently required from: Jason Glenesk, Raul Rangel, EricKY Cheng, Matt DeVillier, Caveh Jalali, Paul Menzel, Tim Wawrzynczak, Eric Peers, Fred Reitberger, Karthikeyan Ramasubramanian, Boris Mittelberg, Felix Held.
Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68471 )
Change subject: common/acpi/dptc: Dynamic Thermal Table Switching Proposal ......................................................................
Patch Set 21:
(11 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/68471/comment/3f6ca69e_e7d5b0de PS21, Line 9: Thermal table switches to different modes to meet 6 : user experience. nit: Implement dynamic thermal table switching flow chart.
This commit message should also describe the various states, so we have a human-readable version of the algorithm which we can validate the code against.
File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/82170415_19b99c88 PS21, Line 44: #if CONFIG(FEATURE_DYNAMIC_DPTC) : Name (PRTN, 0) // Previous thermal table Label A(0) to F(5) : #endif Move this to `src/soc/amd/common/acpi/dptc.asl`, where it's used.
File src/soc/amd/common/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/e901cca8_8914882c PS12, Line 44: TIN4
Done
This was not done. Raul is asking for this code to be moved out of the common `dptc.asl` that all AMD devices use into something that's specific to winterhold devices.
File src/soc/amd/common/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/24919593_6f853ff8 PS21, Line 46: // AMB sensor trigger point Please add a comment describing what conditions these checks are looking for and what the result should be.
Specifically, there is no ">50C" transition state in the diagram here: https://docs.google.com/presentation/d/1Ifjyx_Fu39WgLjJRsUBPxXcHW0dA706Jbx5b...
Instead, there are only >=45C and <50 transition points.
https://review.coreboot.org/c/coreboot/+/68471/comment/b4f7edd6_326aedc7 PS21, Line 47: 123 50C = 122F
https://review.coreboot.org/c/coreboot/+/68471/comment/ef3c5f81_d1172da6 PS21, Line 47: 123 Please add a comment stating the units of these temperature values. The documentation all uses Celsius, but these values are Fahrenheit.
https://review.coreboot.org/c/coreboot/+/68471/comment/b61c72a8_65f809aa PS21, Line 48: (_SB.PCI0.LPCB.EC0.PRTN == 3) || // Previous table is D : (_SB.PCI0.LPCB.EC0.PRTN == 5)) // Previous table is F Why is this checking if the previous table was D or F, when neither of those are related to desktop+lid-open? I'd expect this to check table A.
https://review.coreboot.org/c/coreboot/+/68471/comment/74999180_73c83b15 PS21, Line 56: 118 118F = 47.8C. What state is this checking?
https://review.coreboot.org/c/coreboot/+/68471/comment/c7f901eb_d1c8c486 PS21, Line 57: (_SB.PCI0.LPCB.EC0.PRTN == 2) || // Previous table is C : (_SB.PCI0.LPCB.EC0.PRTN == 4)) // Previous table is E Similarly, why is this state dependent on tables C/E?
https://review.coreboot.org/c/coreboot/+/68471/comment/1b81e61e_05e2d9f0 PS21, Line 64: }Else{ nit: Add whitespace around `Else` and before `//`.
https://review.coreboot.org/c/coreboot/+/68471/comment/7c46eb44_9a215c27 PS21, Line 84: }Else{// Laptop Same