[M] Change in coreboot[master]: drivers/intel/dtbt: Add discrete Thunderbolt driver

[Kyösti Mälkki (Code Review)]

Attention is currently required from: Jeremy Soller, Martin L Roth, Michał Żygowski, Paul Menzel, Tim Crawford, Tim Wawrzynczak.

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75286?usp=email )

Change subject: drivers/intel/dtbt: Add discrete Thunderbolt driver
......................................................................

Patch Set 7:

(6 comments)

File src/drivers/intel/dtbt/dtbt.c:

https://review.coreboot.org/c/coreboot/+/75286/comment/d6e641d7_677c818b :
PS6, Line 13: #define PCIE2TBT 0x54C
Referring to older datasheet: Alpine-Ridge DP, Thunderbolt Interface Controller, Revision 1.03, October 2015. Can you confirm these registers 0x548 and 0x54c come from "Vendor Specific Enhanced Capability" with VS_CAP_1 0x05011234 ?

Could you add a comment about that and define 0x500 as a common base. Some other TBT host controller could have the same register interface, just have the capability appear at a different offset in PCIE config space.

Ideally, the code would search for the location of said capability in PCIE config space, but let's not do that yet.

https://review.coreboot.org/c/coreboot/+/75286/comment/e1da7d58_fd6c3abb :
PS6, Line 24: #define TIMEOUT_MS 1000
Is there a reason why you don't use this timeout with acpigen_write_delay_until_namestr_int() calls? It's literal 600 (ms) below.

https://review.coreboot.org/c/coreboot/+/75286/comment/b6c20477_0eb71fe4 :
PS6, Line 32: 	if (!wait_ms(TIMEOUT_MS, pci_read_config32(dev, TBT2PCIE) & TBT2PCIE_DONE)) {
In my old reference DONE means command acknowledged, but not necessarily completed successfully. You actually had logged the read status in previous patchset, but dropped it.

If there was no timeout on waiting DONE, a non-zero return code in the error field should be logged with BIOS_ERR.

I cannot say if some form of retry or recovery should be attempted wrt sending of SET/GET_SECURITY_LEVEL commands. TBD later, if you encounter any logged errors.

https://review.coreboot.org/c/coreboot/+/75286/comment/d6fc98a5_a4655178 :
PS6, Line 37: 	if (!wait_ms(TIMEOUT_MS, !(pci_read_config32(dev, TBT2PCIE) & TBT2PCIE_DONE))) {
My old reference for PCIE2TBT_GO2SX command says clearing VALID would not clear DONE. Timeout and error should be omitted in that case. Please check if (current) specs says anything about this.

I would have assumed that same would apply to PCIE2TBT_GO2SX_NO_WAKE and my datasheet described 0x03 status in TBT2PCIE as reserved. Maybe command 0x03 gives back status 0x02?

https://review.coreboot.org/c/coreboot/+/75286/comment/15f65b34_25cc48f5 :
PS6, Line 83: 		{
Does this type of indentation to mimic ASL blocks appear somewhere else in our tree already? I understand the purpose, but it's wasting 1/4th of the recommended linewidth of 96.

https://review.coreboot.org/c/coreboot/+/75286/comment/cafed3ac_b02d95df :
PS6, Line 105: 				acpigen_write_delay_until_namestr_int(600, "TB2P", PCIE2TBT_GO2SX_NO_WAKE);
> linewidth, possibly

As you decided to remove shift from PCIE2TBT_GO2SX_NO_WAKE define, you need to add it here now.