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 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 485: 0x10
These are just dummy index numbers and the resources will not be written to hardware? […]
This came from src/southbridge/intel/common/pciehp.c
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 494: 0x14
Same question here with BAR1
This came from src/southbridge/intel/common/pciehp.c
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 538: dummy->ops = &pciexp_hotplug_dummy_ops;
I like the dummy device approach, however it does make an additional reserve instead of guaranteeing […]
With Thunderbolt 3 currently coreboot does not set a security settings so it defaults to having no PCIe capability, meaning the bridges will not have any extra allocation by accident.
https://review.coreboot.org/c/coreboot/+/35946/2/src/include/device/device.h File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/35946/2/src/include/device/device.h... PS2, Line 133: /* hotplug buses to allocate */
Maybe better: Number of hotplug buses to allocate?
Done
https://review.coreboot.org/c/coreboot/+/35946/2/src/include/device/pciexp.h File src/include/device/pciexp.h:
https://review.coreboot.org/c/coreboot/+/35946/2/src/include/device/pciexp.h... PS2, Line 29: #if CONFIG(PCIEXP_HOTPLUG)
No quard needed.
I think it is better to not declare the items inside this guard if they are not defined