Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42295 )
Change subject: soc/intel/tigerlake: Update TCSS for SW CM support ......................................................................
Patch Set 2:
(23 comments)
https://review.coreboot.org/c/coreboot/+/42295/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42295/2//COMMIT_MSG@10 PS2, Line 10: doman domain
https://review.coreboot.org/c/coreboot/+/42295/2//COMMIT_MSG@10 PS2, Line 10: internet : protocol Internet Protocol
https://review.coreboot.org/c/coreboot/+/42295/2//COMMIT_MSG@11 PS2, Line 11: Topogoly Topology
https://review.coreboot.org/c/coreboot/+/42295/2//COMMIT_MSG@13 PS2, Line 13: xhci XHCI
https://review.coreboot.org/c/coreboot/+/42295/2//COMMIT_MSG@16 PS2, Line 16: TEST=Check Type C device connection/enumeration with SW CM. all ports, all orientations?
https://review.coreboot.org/c/coreboot/+/42295/2//COMMIT_MSG@9 PS2, Line 9: This change adds support for SW CM. Add Operating System Capabilities : (_OSC) method to enable USB/DisplayPort/Inter-doman USB4 internet : protocol tunneling and disable PCIe tunneling. Remove Connect Topogoly : (CNTP) command because kernel driver directly works with SW CM Thunderbolt : firmware. Update _DSD method for USB4 support across xhci and PCIe root ports. : : BUG=b:140645231 : TEST=Check Type C device connection/enumeration with SW CM. 72 characters wide for commit msg please
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/tcss.asl:
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 138: definiton definition
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 138: is are
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 146: acknolwedges acknowledges
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 147: Preserved bits in the control : * field from the platform to OSPM, while masked/cleared bits in the : * control field indicate that the platform does not permit OSPM control : * of the respective capabilities or features I think you can just simplify this by saying: "Masked/Cleared bits in the control field indicate that the platform does not permit OSPM control of the respective capabilities or features."
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 155: CreateDWordField(Arg3, 8, CDW3) I'm confused, doesn't Arg3 only contain 3 DWords? So this index (8) would be out of bounds?
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 158: 1 symbolic constant please (looks like a revision?)
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 159: 0x8 /* Unknown revision */ if you made a symbolic constant for 8 meaning UNKNOWN_REVISION (or similar) you wouldn't need the comment.
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 160: } Also don't you want to Return (Arg3) if the revision is unknown?
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 161: 0xb symbolic constant please (maybe USB4_TUNNELS_ALL_EXCEPT_PCIE ?)
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 165: 0x4 /* Unrecognized UUID */ if you made a symbolic constant for 8 meaning UNRECOGNIZED_UUID (or similar) you wouldn't need the comment
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/tcss_dma.asl:
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 30: : Method (_PS0, 0, Serialized) : { : } : : Method (_PS3, 0, Serialized) : { : } _PS0 and _PS3 are now empty, please remove them.
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/tcss_pcierp.asl:
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 67: return didn't need to change the case here
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 199: Sleep(100) /* Wait for 100ms before return to OS starts any DS activities. */ If all this Method is doing is disabling SCIs, does the 100ms sleep still need to be here?
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 224: Return ( Package() { : /* acpi_pci_bridge_d3 at ../drivers/pci/pci-acpi.c */ : ToUUID("6211E2C0-58A3-4AF3-90E1-927A4E0C55A4"), : Package () : { : Package (2) { "HotPlugSupportInD3", 1 }, : }, : : /* pci_acpi_set_untrusted at ../drivers/pci/pci-acpi.c */ : ToUUID("EFCC06CC-73AC-4BC3-BFF0-76143807C389"), : Package () { : Package (2) { "ExternalFacingPort", 1 }, /* TBT/CIO port */ : /* : * UID of the TBT RP on platform, range is: 0, 1 ..., : * (NumOfTBTRP - 1). : */ : Package (2) { "UID", TUID }, : }, : ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), : Package () { : Package (2) { "usb4-host-interface", _SB.PCI0.TDM0 }, : Package (2) { "usb4-port-number", TUID }, : } : }) : } ElseIf (TUID == 2) { : Return ( Package () { : /* acpi_pci_bridge_d3 at ../drivers/pci/pci-acpi.c */ : ToUUID("6211E2C0-58A3-4AF3-90E1-927A4E0C55A4"), : Package () : { : Package (2) { "HotPlugSupportInD3", 1 }, : }, : : /* pci_acpi_set_untrusted at ../drivers/pci/pci-acpi.c */ : ToUUID("EFCC06CC-73AC-4BC3-BFF0-76143807C389"), : Package () { : Package (2) { "ExternalFacingPort", 1 }, /* TBT/CIO port */ : /* : * UID of the TBT RP on platform, range is: 0, 1 ..., : * (NumOfTBTRP - 1). : */ : Package (2) { "UID", TUID }, : }, : ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), : Package () { : Package (2) { "usb4-host-interface", _SB.PCI0.TDM1 }, : Package (2) { "usb4-port-number", 0 }, : } : }) : } Else { /* TUID == 3 */ : Return ( Package () { : /* acpi_pci_bridge_d3 at ../drivers/pci/pci-acpi.c */ : ToUUID("6211E2C0-58A3-4AF3-90E1-927A4E0C55A4"), : Package () : { : Package (2) { "HotPlugSupportInD3", 1 }, : }, : : /* pci_acpi_set_untrusted at ../drivers/pci/pci-acpi.c */ : ToUUID("EFCC06CC-73AC-4BC3-BFF0-76143807C389"), : Package () { : Package (2) { "ExternalFacingPort", 1 }, /* TBT/CIO port */ : /* : * UID of the TBT RP on platform, range is: 0, 1 ..., : * (NumOfTBTRP - 1). : */ : Package (2) { "UID", TUID }, : }, : ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), : Package () { : Package (2) { "usb4-host-interface", _SB.PCI0.TDM1 }, : Package (2) { "usb4-port-number", 1 }, : } This is extremely repetitive, could you refactor out the outer Package() definition from each branch of the conditional into a Method? Method (DSDH, 2) { Return Package () { .... Package() { "usb4-host-interface", Arg0 }, Package() { "usb4-port-number", Arg1 } .... } }
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 223: If ((TUID == 0) || (TUID == 1)) { : Return ( Package() { : /* acpi_pci_bridge_d3 at ../drivers/pci/pci-acpi.c */ : ToUUID("6211E2C0-58A3-4AF3-90E1-927A4E0C55A4"), : Package () : { : Package (2) { "HotPlugSupportInD3", 1 }, : }, : : /* pci_acpi_set_untrusted at ../drivers/pci/pci-acpi.c */ : ToUUID("EFCC06CC-73AC-4BC3-BFF0-76143807C389"), : Package () { : Package (2) { "ExternalFacingPort", 1 }, /* TBT/CIO port */ : /* : * UID of the TBT RP on platform, range is: 0, 1 ..., : * (NumOfTBTRP - 1). : */ : Package (2) { "UID", TUID }, : }, : ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), : Package () { : Package (2) { "usb4-host-interface", _SB.PCI0.TDM0 }, : Package (2) { "usb4-port-number", TUID }, : } : }) : } ElseIf (TUID == 2) { : Return ( Package () { : /* acpi_pci_bridge_d3 at ../drivers/pci/pci-acpi.c */ : ToUUID("6211E2C0-58A3-4AF3-90E1-927A4E0C55A4"), : Package () : { : Package (2) { "HotPlugSupportInD3", 1 }, : }, : : /* pci_acpi_set_untrusted at ../drivers/pci/pci-acpi.c */ : ToUUID("EFCC06CC-73AC-4BC3-BFF0-76143807C389"), : Package () { : Package (2) { "ExternalFacingPort", 1 }, /* TBT/CIO port */ : /* : * UID of the TBT RP on platform, range is: 0, 1 ..., : * (NumOfTBTRP - 1). : */ : Package (2) { "UID", TUID }, : }, : ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), : Package () { : Package (2) { "usb4-host-interface", _SB.PCI0.TDM1 }, : Package (2) { "usb4-port-number", 0 }, : } : }) : } Else { /* TUID == 3 */ : Return ( Package () { : /* acpi_pci_bridge_d3 at ../drivers/pci/pci-acpi.c */ : ToUUID("6211E2C0-58A3-4AF3-90E1-927A4E0C55A4"), : Package () : { : Package (2) { "HotPlugSupportInD3", 1 }, : }, : : /* pci_acpi_set_untrusted at ../drivers/pci/pci-acpi.c */ : ToUUID("EFCC06CC-73AC-4BC3-BFF0-76143807C389"), : Package () { : Package (2) { "ExternalFacingPort", 1 }, /* TBT/CIO port */ : /* : * UID of the TBT RP on platform, range is: 0, 1 ..., : * (NumOfTBTRP - 1). : */ : Package (2) { "UID", TUID }, : }, : ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), : Package () { : Package (2) { "usb4-host-interface", _SB.PCI0.TDM1 }, : Package (2) { "usb4-port-number", 1 }, : } : }) : } tabs please
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/tcss_xhci.asl:
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 125: ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), : Package () : { : Package (2) { "usb4-host-interface", _SB.PCI0.TDM0 }, : Package (2) { "usb4-port-number", 0 } : } Same thing here, could you refactor out this inner part of each of these 4 methods _DSD methods into a helper like above?
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 118: Device (SS01) : { : Name (_ADR, 0x02) : Method (_DSD, 0, NotSerialized) : { : Return( Package () : { : ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), : Package () : { : Package (2) { "usb4-host-interface", _SB.PCI0.TDM0 }, : Package (2) { "usb4-port-number", 0 } : } : }) : } : } : : Device (SS02) : { : Name (_ADR, 0x03) : Method (_DSD, 0, NotSerialized) : { : Return( Package () : { : ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), : Package () : { : Package (2) { "usb4-host-interface", _SB.PCI0.TDM0 }, : Package (2) { "usb4-port-number", 1 } : } : }) : } : } : : Device (SS03) : { : Name (_ADR, 0x04) : Method (_DSD, 0, NotSerialized) : { : Return( Package () : { : ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), : Package () : { : Package (2) { "usb4-host-interface", _SB.PCI0.TDM1 }, : Package (2) { "usb4-port-number", 0 } : } : }) : } : } : : Device (SS04) : { : Name (_ADR, 0x05) : Method (_DSD, 0, NotSerialized) : { : Return( Package () : { : ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), : Package () : { : Package (2) { "usb4-host-interface", _SB.PCI0.TDM1 }, : Package (2) { "usb4-port-number", 1 } : } : }) : } : } tabs please