Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39785 )
Change subject: soc/intel/tigerlake: Configure TCSS xHCI power management ......................................................................
Patch Set 8:
(19 comments)
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/tcss.asl:
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 46: , 8, : PBSS, 1, // Power Button Status : Offset(0x40), // 0x40, General Purpose Event Control : , 17, : GPEC, 1, // Software GPE Control this looks unused?
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 74: ToInteger comment above says this should be an integer?
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 135: _SB.PCI0. already in _SB.PCI0 scope
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 141: MBAR does this work to re-use the same field name as above? can they just be merged or is the ByteAcc vs DWordAcc important?
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 263: LEqual (Local0, 100) Local0 == 100
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 283: And (^RBAR, 0xFFFFFFFFFFFFFFFE) Does (^RBAR & ~1) work?
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 287: dynamically Nothing seems dynamic about this method
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 312: { nit: CR before {
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 314: LEqual (TD3C, 1) TD3C == 1
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 317: Store (0, TD3C) TD3C = 0
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 318: Store (0, Local0) Local0 = 0
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 318: / nit: space after //
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 321: ///TCSS_IOM_ACK_TIMEOUT_IN_MS))) { left over?
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 330: LEqual maybe us LNotEqual so the if clause isn't empty? (or the ASL2.0 variant)
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 348: } Else { : /* Drop this method due to it is already exit D3 cold */ : Return : } this is unnecessary
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 379: PowerResource this PowerResource is for _SB.PCI0 itself?
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 408: ( nit: space before (
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 415: nit: extra tab
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/tcss_xhci.asl:
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 31: ( nit: space after (