Kyösti Mälkki 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 3:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pci_device.c@886 PS2, Line 886: } else
else is not generally useful after a break or return
You should write this without the preprocessor #if.
I am not sure what we decided about the asymmetric use of braces
if () { stuff1; stuff2; } else stuff3;
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pci_device.c@126... PS2, Line 1269: link->subordinate = link->secondary + dev->hotplug_buses; It is too early to adjust link->subordinate here. Downstream PCI bridges will reference this as the recursive scan proceeds.
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
Does this handle BAR0? If yes you could use PCI_BASE_ADDRESS_0 instead of 0x10 her.
These are just dummy index numbers and the resources will not be written to hardware?
I think it would be cleaner to just use 'i++' here instead of fooling us to think these would end up written in any PCI hardware BARs. Also I think the dummy device we add should be considered as a pseudo PCI bridge with PCI Header Type 1 and the BAR numbers used here are for Type 0.
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 488: resource->gran = 22; I believe both .align and .gran need to conform with the requirements of the configurable .size.
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 527: pciexp_scan_bridge(dev); It should be possible to adjust dev->link_list->subordinate here to either max N or additional N PCI buses without adding hotplug_buses in struct device.
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 a minimal resource space. We cannot assume all hotpluggable devices to be disconnected for the enumeration, those connected will already claim resources and we risk running out of MMIO if we also add the reserve.
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.