John Zhao 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 4:
(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
Done
https://review.coreboot.org/c/coreboot/+/42295/2//COMMIT_MSG@10 PS2, Line 10: internet : protocol
Internet Protocol
Done
https://review.coreboot.org/c/coreboot/+/42295/2//COMMIT_MSG@11 PS2, Line 11: Topogoly
Topology
Done
https://review.coreboot.org/c/coreboot/+/42295/2//COMMIT_MSG@13 PS2, Line 13: xhci
XHCI
Done
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?
Done
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
Done
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
Ack
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 138: is
are
Ack
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 146: acknolwedges
acknowledges
Ack
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: […]
Done
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?
corrected.
https://review.coreboot.org/c/coreboot/+/42295/2/src/soc/intel/tigerlake/acp... PS2, Line 158: 1
symbolic constant please (looks like a revision?)
Done
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 co […]
Done
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?
If the revision is unknown, Arg3 is returned as "Return (Arg3)".
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 ?)
Added symbolic constants.
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 c […]
Done
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.
Done
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
Done
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?
PCIe spec mandates to wait 100ms after link has been trained before sending configuration request to the device. It is conserved to keep this delay along with kernel driver. Check it late if there is need for optimization.
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 […]
It seems arguments won't be got into the package() in method.
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
Done
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 […]
It seems arguments are not got into Package in method.
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
Done