Attention is currently required from: Subrata Banik, Maulik V Vaghela, Tim Wawrzynczak, EricR Lai. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61936 )
Change subject: soc/intel/common/block: Add variable to correct pcie_rp indexing ......................................................................
Patch Set 7: Code-Review+1
(3 comments)
File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/61936/comment/4121a7d5_ecd63b26 PS3, Line 49: port_num = port_num - 1; Thanks!
Also, before TBT PCIe init, if we read this root ports value is 0xFFFFFFFF.
Ah, that makes it hard to figure out the reset state.
Anyway, the resulting state is as expected (if I translated the DID correctly).
File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/61936/comment/9bf72f4e_5e5d0eb3 PS7, Line 35: /* Read 1-based absolute port number. This reflects the numbering : scheme that Intel uses in their documentation and what we use : as index (0-based, though) in our mapping. */ : const unsigned int port_num = (lcap & PCI_EXP_LNKCAP_PORT) >> 24; : : /* While checking port number from LCAP registers, coreboot assumes : that port number is 1 based. This might not be true for all the : cases. In case of ADL TBT, LCAP port number starts from index 2 : which might break this logic. : Thus subtracting port_num_start from port number will allow us : to return correct index(0 based) to caller. : */ Please merge these comments or drop them entirely. If we first say one thing just to correct it in the next comment that's worse, IMO, than not saying anything at all.
(And please use one of the official comment styles.)
https://review.coreboot.org/c/coreboot/+/61936/comment/4f7685b2_589cdf4e PS7, Line 47: port_idx When I named it `port_idx` I had something 0-based in mind. And the original `port_num` was actually only kept 1-based because that was how the hardware provided the value. Now that this is not generally true anymore, I see little value to work with a 1-based value. The whole code could be more self-explanatory and would need less comments if we don't enforce this outdated encoding.