Felix Held 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:
(5 comments)
only had a brief look, but i like where this is going. splitting the patch into the (pci_)device part and the TB-specific part would be a good, since those two are separate changes
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 51: resource->flags |= IORESOURCE_IO;
It's not clear to me what the rationale is for the resource sizes being speculatively allocated unde […]
In order for the hot-plugging of devices and busses to work, we need to reserve MMIO and IO space for the potential devices behind this bridge for the allocator, so a big enough MMIO window gets reserved for the hot-pluggeable devices and get routed through the bridges between this one and the PCIe root. Since we don't know what devices will get hot-plugged after coreboot configured things, we'll just have to reserve enough resources here. The MMIO decode windows in the bridges between this bridge and the root port is the big issue that is addressed here. If this isn't done here, the system would have to move around MMIO BARs of other PCIe devices to be able to map the device's MMIO behind the TB bridge while the other devices still need to work, but you'll probably need to unload the corresponding drivers, move the BARs around and then reload the device drivers which isn't very practical and might even not be implemented on operating systems.
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 69: dev->hotplug_buses = 32;
While I like this idea. The `hotplug_buses` implementation seems clean […]
I also like the hotplug_buses approach and it's rather noninvasive
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?
might be worth putting this into the device tree and only defaulting to 32 when there's no setting in the device tree
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 73: pciexp_scan_bridge(dev); haven't looked very closely at the code, but what does happen if there are devices already connected behind the TB bridge at boot time? do the 256MiB MMIO space get allocated additionally?
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 80: slot = alloc_dev(dev->link_list, &slot_path);
I don't follow, are you suggesting to allocate a total of 2x 8GiB? […]
IIRC the 256MiB was what the vendor firmware does; not sure if this is mandated by the TB spec. 256MiB prefetchable MMIO space sounds reasonable to me; 256MiB non-prefetchable MMIO sounds rather big to me though (and the non-prefetchable can't be mapped above 4G)