Attention is currently required from: Raul Rangel, Jason Nien, Matt DeVillier, Paul Menzel, Eric Lai, Martin Roth, Eric Peers, LeilaCY Lin, Jason Glenesk, Caveh Jalali, Tim Wawrzynczak, 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: soc/amd/common/acpi: Implement DTTS Proposal ......................................................................
Patch Set 53:
(10 comments)
Patchset:
PS53: Thanks for bearing with us. I think this is getting pretty close now.
File src/soc/amd/common/acpi/DTTS.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/8bc003d9_e4aac501 PS39, Line 15: /* If _SB.DDEF is not present, DPTC is not enabled so return early. */ : If (!CondRefOf (_SB.DDEF)) : { : Return (Zero) : }
Based on your suggestion, I need your help to clarify the design thinking as below. […]
Yes, I think what you have is close to what I was describing. I do have a comment above about your implementation though. I'm going to close this comment, and we can discuss the details further in the other comment.
https://review.coreboot.org/c/coreboot/+/68471/comment/4876def3_36fedc58 PS39, Line 21: //Set table A as default table after power on SUT : If (_SB.PRTN == 7) : { : _SB.DDEF() : Store(0,_SB.PRTN) : Return (Zero) : }
As thermal team's request, table A would be the first table during power on. […]
Is it possible to power on with the lid closed, in laptop mode, or with the sensor >45C? If so, the only signal that would be received again (while still in that state) is the temperature changing, since the "lid closed" or "laptop mode" values won't be sent again if they aren't changing. It's also unknown how long it would take for the temperature to change (though it may not matter).
For example, that would mean the device could possibly be running with Table A when powered on with the lid closed until the temperature changes, in which case Table C will be loaded regardless of what the current temperature is since the previous table was A.
Since the design is for the EC to always send a signal when the temperate changes by 1 deg (as for as I know, anyway), this may be a non-issue because the DPTC values will be updated before anything gets too hot.
Do you know how the EC temperature signals working? Are there CLs to enable this signalling to the AP, or is that unnecessary?
File src/soc/amd/common/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/36e3ede9_42e21c57 PS12, Line 44: TIN4
I created DTTS.asl and put it under src/soc/amd/common/acpi/. […]
Done
File src/soc/amd/common/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/60640f38_78394a59 PS21, Line 47: 123
There is an offset of temperature value stored in mapped memory. […]
Ack
https://review.coreboot.org/c/coreboot/+/68471/comment/eb9cd23d_03095be5 PS21, Line 47: 123
There is an offset of temperature value stored in mapped memory. […]
Ack
https://review.coreboot.org/c/coreboot/+/68471/comment/1f7db687_c993a3e2 PS21, Line 48: (_SB.PCI0.LPCB.EC0.PRTN == 3) || // Previous table is D : (_SB.PCI0.LPCB.EC0.PRTN == 5)) // Previous table is F
Follow thermal team's proposal, A C E are temperature release table, B D E are temperature over tabl […]
Ack
https://review.coreboot.org/c/coreboot/+/68471/comment/2148384d_aacbe59d PS21, Line 56: 118
Add a comment indicating this offset is how the temperature values here are determined on the first […]
Ack
https://review.coreboot.org/c/coreboot/+/68471/comment/356f5ab8_40899581 PS21, Line 57: (_SB.PCI0.LPCB.EC0.PRTN == 2) || // Previous table is C : (_SB.PCI0.LPCB.EC0.PRTN == 4)) // Previous table is E
Follow thermal team's proposal. […]
Ack
File src/soc/amd/common/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/17ddc4f6_60a3da2f PS53, Line 42: #if CONFIG(FEATURE_DYNAMIC_DPTC) : #include <soc/amd/common/acpi/DTTS.asl> : #endif Can this `#include` be moved to the top of the file? I would then expect this to just be:
``` If (CondRefOf (_SB.DTTS)) { _SB.DTTS() Return (Zero) } ```