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 16:
(12 comments)
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/tcss.asl:
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... PS16, Line 532: /* DBG("Error: Error: Timeout occurred.") */
It does not seem like a bad idea to leave these Printf() in place (and add them to other error paths […]
Ack
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/tcss_pcierp.asl:
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... PS16, Line 12: L0SE, 1, /* 0, L0s Entry Enabled */ : , 3, : LDIS, 1, : , 3,
some of these are not used, can you scrub for the ones that are?
For debug purpose, it seems ok to keep them here.
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... PS16, Line 96: _L61
I was confused as to why it was specifically _L61, but GPE 0x61 looks like GPE0_HOT_PLUG so that mak […]
The _L61 is GPE_L61. The sub-method HPEV of _L61 checks and handles Hot Plug SCI status.
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... PS16, Line 102: all
this doesn't seem to clear all status bits, just a couple
This method handles Hot-Plug event. It seems both of PDCX(Presence Detect Changed) and HPSX(Hot Plug SCI status) clearing are all for this event.
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... PS16, Line 107: ^
is this legal? the spec defines ^ as a prefix that is followed by "ParentPrefix or Name"
It seems legal as '^' is defined to be parentprefix. It notifies OS with object like _SB.PCI0.TRP0,_SB.PCI0.TRP1,_SB.PCI0.TRP2, or _SB.PCI0.TRP3.
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... PS16, Line 107: 0
0 is "bus check" to force a re-enumerate? can you add a comment indicating that is what this notify […]
You are right. Once the bus check notification value '0' is received, bus re-enumeration is required (starting at the target object): Walk the bus from the target device down. Remove devices that are not present any more. Add devices that has just appeared.
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... PS16, Line 182:
spaces before tab
Ack
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... PS16, Line 245: ethod (CHKH, 0) { : If (PDCX == 1) { : If (DLSC == 0) { : /* Clear PDC since it is not a hotplug. */ : PDCX = 1 : } : } : }
this only seems to be used once, might as well fold it into _PS3 so it is easier to follow the code.
Ack
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... PS16, Line 260: HotPlugSupportInD3
can you add a comment with a pointer to where these are defined? i assume the microsoft doc is auth […]
Sorry, I did not have documents. I was informed Chromeos/linux/Windows share the same tcss pcie root ports device specification data(_DSD).
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... PS16, Line 267: - 1
long line
Ack
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... PS16, Line 311: Consume one pending PME notification to prevent it from blocking the queue.
is this to work around an OS behavior?
When PME Status(PSPX) is set, it indicates that PME was asserted. Subsequent PMEs are kept pending until this bit is cleared.
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/tcss_xhci.asl:
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... PS16, Line 10: VDID, 32,
this doesn't look used
xhci VDID is used in TCOF method as "While (_SB.PCI0.TXHC.VDID != 0xFFFFFFFF) {".