Attention is currently required from: Wilson Chou, Marc Jones, Jonathan Zhang, Ryback Hung, Johnny Lin, Paul Menzel, Tim Wawrzynczak, Shuming Chu (Shuming).
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/67264 )
Change subject: device: Clear lane error status ......................................................................
Patch Set 3:
(6 comments)
Patchset:
PS2:
We need to clear lane error status after (last) link training triggered by coreboot. […]
Thanks. Could we make it part of pciexp_retrain_link() then?
File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/67264/comment/39a03b74_7c0a83db PS2, Line 546: if (reg32 != 0) { You can avoid the indentation with early returns, e.g. ``` pos = ... if (pos == 0) return;
reg32 = ... if (reg32 == 0) return; ```
https://review.coreboot.org/c/coreboot/+/67264/comment/a564db0a_11dfbea9 PS2, Line 548: int bus_num, device_num; : bus_num = dev->bus->secondary; : device_num = dev->path.pci.devfn; : printk(BIOS_DEBUG, "bus = 0x%x dev = 0x%x\n", bus_num, device_num); Please use dev_path().
https://review.coreboot.org/c/coreboot/+/67264/comment/b6258a80_2bb8d4c0 PS2, Line 552: printk(BIOS_DEBUG, "pciexp_find_extended_cap = 0x%x\n", pos); This is probably something to put inside pciexp_find_extended_cap(), if we want to keep it.
File src/include/device/pci_def.h:
https://review.coreboot.org/c/coreboot/+/67264/comment/628c64fa_c6cb0a38 PS2, Line 104: #define PCI_SECONDARY_BUS 0x19 /* Secondary bus number */ This is (as the comment above says) an offset in type 1 configuration headers. It only shares the number by coincidence.
https://review.coreboot.org/c/coreboot/+/67264/comment/a2651f3b_b866d1d3 PS2, Line 465: #define PCI_EXT_CAP_ID_VNDR 0x0b Please add a new define here. See /usr/include/pci/header.h for reference. The offsets within the capability should have there own block below.
-
Nico Huber (Code Review)