Duncan Laurie 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) for later debug.
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?
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 makese sense. Can you add a comment here? Will the definition of _L61 be added separately?
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
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"
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 is doing?
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... PS16, Line 182: spaces before tab
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.
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 authoritative: https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-r...
https://review.coreboot.org/c/coreboot/+/39785/16/src/soc/intel/tigerlake/ac... PS16, Line 267: - 1 long line
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?
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