Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 2:
(20 comments)
A large number of the comments made before were solved with the latest changeset (3). Please review this changeset, it is no longer specific to thunderbolt 3 devices but encompases any PCI express hotplug bridges.
https://review.coreboot.org/c/coreboot/+/35946/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35946/1//COMMIT_MSG@7 PS1, Line 7: driver/thunderbolt: Driver for allocating hotplug resources
Please make it a statement by adding a verb (in imperative mood): […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1//COMMIT_MSG@16 PS1, Line 16: was
is
Done
https://review.coreboot.org/c/coreboot/+/35946/1//COMMIT_MSG@16 PS1, Line 16: was
is
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/Kco... File src/drivers/thunderbolt/Kconfig:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/Kco... PS1, Line 1: config DRIVERS_THUNDERBOLT
USB4 is the new hotness. […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... File src/drivers/thunderbolt/thunderbolt.c:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 25: static void slot_dev_read_resources(struct device *dev)
that code should be shared with the existing southbridge code.
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 30: 0x10)
These should be using the PCI macros for the registers.
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 31: 1 << 28
256 * MiB
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 33: resource->gran = 22;
Where did the alignment come from?
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 51: resource->flags |= IORESOURCE_IO;
sounds good to me
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 59: return PCI_SLOT(dev->path.pci.devfn) == 1;
What does the device tree look like to support this device? I assume this is the router? Are there m […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 69: dev->hotplug_buses = 32;
I also like the hotplug_buses approach and it's rather noninvasive
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 69: 32
might be worth putting this into the device tree and only defaulting to 32 when there's no setting i […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 73: pciexp_scan_bridge(dev);
That's assuming thunderbolt links are already provisioned and pcie tunneling is happening. […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 80: slot = alloc_dev(dev->link_list, &slot_path);
IIRC the 256MiB was what the vendor firmware does; not sure if this is mandated by the TB spec. […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 93: .init = 0,
not needed
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 94: .scan_bus = tbt_pciexp_scan_bridge,
I believe thunderbolt and usb4 controllers need to export capabilities in pci config space. […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 95: .enable = 0,
not needed
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 102: 0x15e7
move to pci_ids. […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/include/device/device.h File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/35946/1/src/include/device/device.h... PS1, Line 132: hotplug buses
number?
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/include/device/device.h... PS1, Line 132: uint16_t
This doesn't need to be fixed width.
Done