Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
(13 comments)
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. We may want to figure out how to surface that noun since that one is an open standard.
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 30: 0x10) These should be using the PCI macros for the registers.
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 31: 1 << 28 256 * MiB
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 33: resource->gran = 22; Where did the alignment come from?
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 51: resource->flags |= IORESOURCE_IO; It's not clear to me what the rationale is for the resource sizes being speculatively allocated under the device.
Likewise can you please explain why we have to preallocate buses as well? Are you assuming the code running after coreboot cannot allocate buses and mmio spaces?
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 59: return PCI_SLOT(dev->path.pci.devfn) == 1;
Is this defined in some standard or just happens to be the case for the […]
What does the device tree look like to support this device? I assume this is the router? Are there multiple pci functions with the same did but only one is the router?
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 69: 32 Why are we hard coding the number of buses? Shouldn't we make this configurable?
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 93: .init = 0, not needed
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. Can we not handle the special devices based on that?
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 95: .enable = 0, not needed
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 102: 0x15e7 move to pci_ids.h
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?
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.