Attention is currently required from: Jason Nien, EricKY Cheng, 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.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68471 )
Change subject: soc/amd/common/acpi: Implement DTTS Proposal ......................................................................
Patch Set 71: Code-Review+1
(4 comments)
File src/soc/amd/common/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/048ad018_0059d2c0 PS71, Line 7: #if CONFIG(FEATURE_DYNAMIC_DPTC) : #include <soc/amd/common/acpi/dtts.asl> See my comment below. Move this `#if` into the skyrim `dtts.asl`
https://review.coreboot.org/c/coreboot/+/68471/comment/6d7e4e64_69d93b62 PS71, Line 10: External(_SB.DTTB, MethodObj) : External(_SB.DTTC, MethodObj) : External(_SB.DTTD, MethodObj) : External(_SB.DTTE, MethodObj) : External(_SB.DTTF, MethodObj) These externals should go into `dtts.asl` since that's what calls them.
File src/soc/amd/common/acpi/dtts.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/95c714ca_ddcf9a13 PS71, Line 13: // Set table A as default table after power on device : If (_SB.PRTN == 7) : { : _SB.DDEF() : Store (0, _SB.PRTN) : Return (Zero) : } Jut an FYI: The values from the "default" table will be passed into FSP and FSP will perform the initial configuration. This DTTS method will get called once the OS initialized the ACPI subsystem. Since the FSP doesn't support all the new values we will be in an inconsistent state until the OS boots and calls this ACPI method :/
https://review.coreboot.org/c/coreboot/+/68471/comment/cd85c2b3_8e3c9f66 PS71, Line 28: TIN4 >= 123 :/ so this is still board specific config. I realize adding the extra knobs to device tree, writing new ASL values is a pain. I also ack that converting all this code to acpigen is going to be a nightmare to read and maintain. I also realize that we would like this to be common code. Weighing all these options I think we just move this file into the winterhold directory. If someone else (unlikely) wants to enable this feature we can then look at doing the refactors, or just copy/paste the file and change the new values.
In the [skyrim/dsdt.asl](https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...) add the following block:
``` #if CONFIG(FEATURE_DYNAMIC_DPTC) #include <variant/acpi/dtts.asl> #endif ```
Then move this file into `src/mainboard/google/skyrim/variants/winterhold/include/variant/acpi/dtts.asl`.
This copies the pattern we had setup for `zork`'s temperature sensors: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...