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. -- To view, visit https://review.coreboot.org/c/coreboot/+/35946 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I500191626584b83e6a8ae38417fd324b5e803afc Gerrit-Change-Number: 35946 Gerrit-PatchSet: 3 Gerrit-Owner: Jeremy Soller <jeremy@system76.com> Gerrit-Reviewer: Jeremy Soller <jeremy@system76.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Werner Zeh <werner.zeh@siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Aaron Durbin <adurbin@chromium.org> Gerrit-CC: Brandon Breitenstein <brandon.breitenstein@intel.com> Gerrit-CC: Divya S Sasidharan <divya.s.sasidharan@intel.com> Gerrit-CC: Duncan Laurie <dlaurie@chromium.org> Gerrit-CC: Felix Held <felix-coreboot@felixheld.de> Gerrit-CC: Furquan Shaikh <furquan@google.com> Gerrit-CC: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-CC: Matt DeVillier <matt.devillier@gmail.com> Gerrit-CC: Mimoja <coreboot@mimoja.de> Gerrit-CC: Nathaniel L Desimone <nathaniel.l.desimone@intel.com> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-CC: Subrata Banik <subrata.banik@intel.com> Gerrit-Comment-Date: Fri, 06 Dec 2019 08:22:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Werner Zeh <werner.zeh@siemens.com> Comment-In-Reply-To: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: comment