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 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?
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.h> and use ACPI_BASE_ADDRESS
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 ..." here?
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?
same for all the other DBG statements that are commented out. I don't think there is a downside to printing this, by default the kernel will ignore but it may be useful for 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 the file with the others?
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 it is not found.
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 595: extra space
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
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 serialized method.
Should WACT be in the parent device? Maybe use a mutex instead?
edit: I see it these manipulated in the pcierp _PS0 method now. Can you expand on the comments here so this is not so confusing?
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?
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
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?
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 299: ( space
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
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 134: extra space
https://review.coreboot.org/c/coreboot/+/39785/18/src/soc/intel/tigerlake/ac... PS18, Line 135: extra space