Attention is currently required from: Cliff Huang, Fred Reitberger, Jason Glenesk, Lance Zhao, Matt DeVillier, Nico Huber, Patrick Rudolph, Tim Wawrzynczak.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79877?usp=email )
Change subject: device: Add support for multiple PCI segments ......................................................................
Patch Set 1:
(4 comments)
File src/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/79877/comment/8a895c69_33d863c1 : PS1, Line 157: i * CONFIG_ECAM_MMCONF_BUS_NUMBER
Shouldn't this still be 0? In each segment group we start from 0, I guess.
that's also my understanding here
https://review.coreboot.org/c/coreboot/+/79877/comment/0e6ee3ab_9b60ce2d : PS1, Line 158: CONFIG_ECAM_MMCONF_BUS_NUMBER hm, CONFIG_ECAM_SEGMENT_COUNT > 1 and CONFIG_ECAM_MMCONF_BUS_NUMBER != 256 will be odd, but still valid, right?
File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/79877/comment/3ebe8b44_61836c4f : PS1, Line 559: config ECAM_SEGMENT_COUNT i first also added a kconfig option for this, but ended up removing it again, since just using 512 or 1024 for ECAM_MMCONF_BUS_NUMBER both needs one kconfig symbol less and makes the code a bit simpler. that requires the assumption that the ecam mmio regions are continuous, but that's like an assumption that can be made
File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/79877/comment/be77d2f1_e1c36735 : PS1, Line 89: uint8_t segment; /* PCI segment */
We also discussed lately if it wouldn't be easier to make it part of […]
Nico and i discussed this topic on irc some weeks ago and we ended up with the conclusion that using the upper 8 bits of secondary and subordinate would probably be the better option. sure, it requires some macros and also some sanity checks that all busses from the same pci root need to have the same segment number, but requires less changes that either affect a larger part of the tree or would introduce additional functions that do similar things as existing functions, but with an additional segment parameter
i have local patches that implement this as discussed, but haven't gotten around to clean up, test and push those; mainly waited with that for the xeon sp resource patch to finally get ready and into upstream and now we have duplicate efforts :/