[M] Change in coreboot[master]: common/acpi/dptc: Dynamic Thermal Table Switching Proposal
Attention is currently required from: Jason Glenesk, Raul Rangel, Matt DeVillier, Caveh Jalali, Paul Menzel, Tim Wawrzynczak, Tim Van Patten, Eric Peers, 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: Dynamic Thermal Table Switching Proposal ......................................................................
Patch Set 21:
(15 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/68471/comment/ee232ea6_0b580960 PS10, Line 7: common/acpi/dptc: Dynamic Thermal Table Switching Proposal
Please make this a statement by adding a verb (in imperative mood) [1]. […]
Done
https://review.coreboot.org/c/coreboot/+/68471/comment/62a15150_d3a8ee28 PS10, Line 9: Thermal table switches to different modes to meet 6 : user experience.
Please use 72 characters per line. […]
Done
Patchset:
PS21: Hi all,
Update new patch based on below fixed *Space before “==” *“Lid on” change to “Lid open” *Remove debug message *Combine STTB block *Update commit title and description
And follow the suggestions, re-design the table switch method by adding PRTN in ec.asl when FEATURE_DYNAMIC_DPTC is configure (We can discuss if it should replace by TNCA)
BR, Eric KY Cheng
File src/soc/amd/common/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/5a70a7dc_4f23f510 PS10, Line 38: //Desktop + Lid on
Please use C89 comment style (as above). (A space is missing after `//` too. […]
Done
https://review.coreboot.org/c/coreboot/+/68471/comment/5725d1c1_bebf16e0 PS10, Line 38: Lid on
Does "Lid on" mean "lid open"?
Done
https://review.coreboot.org/c/coreboot/+/68471/comment/4dc1a40c_06d49799 PS10, Line 39: STTB==0
style: Add spaces around the `==`.
Done
https://review.coreboot.org/c/coreboot/+/68471/comment/bafc24cd_6bbaccbe PS10, Line 43: Printf ("EC : Desktop + Lid on")
While these are fine for debug, we don't want/need to output these types of "Desktop + Lid on" state […]
Done
https://review.coreboot.org/c/coreboot/+/68471/comment/00bb9fd6_98484115 PS10, Line 44: //over 50
After chatting with Raul, he pointed out that storing the previously applied table ID in a `Name`d o […]
Done
https://review.coreboot.org/c/coreboot/+/68471/comment/88ef6f59_b8ca0939 PS10, Line 45: 0x7B
Ah, 50C is 122F, so maybe this is off by 1?
Done
https://review.coreboot.org/c/coreboot/+/68471/comment/f4ff1511_25c60ce7 PS10, Line 47: Printf ("EC : Table B")
All of these "Table X" log statements should be moved into the `acpigen_write_alib_dptc_*` methods i […]
Done
https://review.coreboot.org/c/coreboot/+/68471/comment/3ff0b69f_f1cd507a PS10, Line 60: } : : If (_SB.PCI0.LPCB.EC0.STTB==0) : {
Combine these blocks, so we don't need to read `STTB` multiple times.
Done
https://review.coreboot.org/c/coreboot/+/68471/comment/ae6d1f8d_79d4780e PS10, Line 83: } : If (_SB.PCI0.LPCB.EC0.STTB==1) : {
Make this an `Else`.
Done
https://review.coreboot.org/c/coreboot/+/68471/comment/5b5dc9b5_c84d237a PS10, Line 108: Printf ("EC: DPTC Call End(Should not show here)")
This can be removed.
Done
File src/soc/amd/common/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/68471/comment/1fed476b_fffa57ef PS12, Line 44: TIN4
These are all device specific values. […]
Done
https://review.coreboot.org/c/coreboot/+/68471/comment/7abf33dd_8693d258 PS12, Line 51: 0x76
Please use decimal values instead of hex
Done
-
EricKY Cheng (Code Review)