John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39785 )
Change subject: soc/intel/tigerlake: Configure TCSS power management ......................................................................
Patch Set 18:
(15 comments)
Patch Set 18:
(15 comments)
This would really benefit from some sort of overview indicating how it these pieces fit together. I'm not expecting a formal Intel IBL doc, but could you put something in a comment at the top of tcss.asl describing the different pieces?
I added the following brief description. Please let us know whether we need to add more. Thanks. /* * Type C Subsystem(TCSS) topology provides Runtime D3 support for USB host controller(xHCI), * USB device controller(xDCI), Thunderbolt DMA devices and Thunderbolt PCIe controlers. * PCIe RP0/RP1 is grouped with DMA0 and PCIe RP2/RP3 is grouped with DMA1. */
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/tcss.asl:
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 45: 0x1800
should be able to include <soc/iomap. […]
Ack
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 55: Set Default value 0 to
this comment isn't very readable, maybe "set default value to 0 for ... […]
Ack
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 172: * DBG("Timeout occurred.") */
leave as 'Printf ("pCode Mailbox timeout occurred")' for debugging? […]
DBG is the pseudo name. APRT is the actual method for acpi debug.
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 282: Method
This only seems to get used once, maybe just make a #define for the 0xc10000 constant at the top of […]
Ack
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 324: !=
you could reduce the indent level on all of these by changing this to an == and returning early if i […]
Ack
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 595:
extra space
Ack
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 686: Name (LTEN, 0) /* Latency Tolerance Reporting Mechanism, 0:Disable, 1:Enable */ : Name (LMSL, 0x88C8) /* PCIE LTR max snoop Latency */ : Name (LNSL, 0x88C8) /* PCIE LTR max no snoop Latency */
I don't see any of these values getting used
Ack
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/tcss_dma.asl:
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 122: other thread's _PS0
I don't understand how this works, WACT variable is local to this DMA device, and this is a serializ […]
Updated with comments.
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 195: _DSD
can you add a comment with a pointer to the location in the kernel where this is used?
Done
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/tcss_pcierp.asl:
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 232: If (PDCX == 1) { : If (DLSC == 0) { : /* Clear PDC since it is not a hotplug. */ : PDCX = 1 : } : }
tab indent
Ack
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 249: _DSD
can you add a comment with a pointer to the location in the kernel where this is used?
Done
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 299: (
space
Ack
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/tcss_xhci.asl:
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 129:
extra space
Done
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 134:
extra space
Done
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 135:
extra space
Done