Attention is currently required from: Raul Rangel, Jason Nien, Matt DeVillier, Paul Menzel, Martin Roth, Tim Van Patten, Eric Peers, LeilaCY Lin, Jason Glenesk, Caveh Jalali, Tim Wawrzynczak, Fred Reitberger, Karthikeyan Ramasubramanian, Boris Mittelberg, Felix Held.
EricKY Cheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68471 )
Change subject: common/acpi/dptc: Implement DTTS Proposal ......................................................................
Patch Set 38:
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/68471/comment/f79dc173_80669e9b PS30, Line 39: Table
table
Done
https://review.coreboot.org/c/coreboot/+/68471/comment/39d8a358_970e7bc6 PS30, Line 40: reach
reaches
Done
File src/mainboard/google/skyrim/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/73bf143d_6d92ccfc PS30, Line 18: variants/winterhold/DTTS.asl
Add the following include line here: […]
I move DTTS.asl to src/soc/amd/common/acpi/ now
File src/soc/amd/common/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/5a9a3a9b_4599ac15 PS12, Line 44: TIN4
Please don't resolve comments that are still incomplete, since it makes it difficult to see there ar […]
I created DTTS.asl and put it under src/soc/amd/common/acpi/. src/soc/amd/mendocino/acpi/soc.asl call it as below
#if CONFIG(SOC_AMD_COMMON_BLOCK_ACPI_DPTC) #if CONFIG(FEATURE_DYNAMIC_DPTC) #include <soc/amd/common/acpi/DTTS.asl> #else #include <soc/amd/common/acpi/dptc.asl> #endif #endif
File src/soc/amd/common/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/fe4da512_33de3565 PS21, Line 46: // AMB sensor trigger point
Please add a comment describing what conditions these checks are looking for and what the result sho […]
I update the commit description with table translation rule and new code change following these rules.
Please help to review that.
Follow your suggestions, I will add more comment describing what conditions these checks are looking for and what the result should be.
https://review.coreboot.org/c/coreboot/+/68471/comment/ca24f142_cbd1d08b PS21, Line 47: 123
Please add a comment stating the units of these temperature values. […]
There is an offset of temperature value stored in mapped memory. This allows reporting a temperature range of 200K to 454K = -73C to 181C. static void update_mapped_memory(void) #define EC_TEMP_SENSOR_OFFSET 200
So if the temperature is 50C, there will store 123(0x7b) in mapped memory. 50C=323K, 323-200(offset)=123(0x7b), example
https://review.coreboot.org/c/coreboot/+/68471/comment/9fd39296_30202999 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?
Follow thermal team's proposal.
A, C, E tables are triggered by AMB sensor release point. B, D, F tables are triggered by AMB sensor over point.
When mode change/Lid change happened, A, C, E (Release Temp) can switch to each other. B, D, F (Over Temp) can switch to each other. If temperature meets the new tables condition, EC will help to trigger thermal threshold event. Follow thermal threshold event, thermal table may translation to release temp table, over temp table or keep the previous table. So table translation is depended on previous table.