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 segment groups ......................................................................
Patch Set 2:
(5 comments)
Patchset:
PS2: had a look at my local branch and it was in a worse shape than i remembered, so i'll try to integrate some of the things i did into this patch and push it with a new change-id so that you can have a look
File src/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/79877/comment/e0d9cf5b_d9d13405 : PS1, Line 153: for (int i = 0; i < CONFIG_ECAM_SEGMENT_COUNT; i++) {
another option that i'm planning to look into is to loop over the domains here and get the secondary […]
looks like i misread that part of the spec. this likely won't work; see one of my other comments
https://review.coreboot.org/c/coreboot/+/79877/comment/8cfca0bb_d1b95a21 : PS1, Line 157: i * CONFIG_ECAM_MMCONF_BUS_NUMBER
I assume that would alter how things are numbered in Linux (i.e. more PCI domains) […]
in a multi segment group system, each pci root needs to have _SEG which tells the host to which segment it belongs to, but the spec explicitly says that the mcfg table mustn't contain multiple entries corresponding to a single pci segment group. either misread or misremember that, so iterating over all domains and use the secondary, max_subordinate and segment from it won't be within the spec
File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/79877/comment/99fcb4ab_6800a409 : PS2, Line 428: DEVTREE_CONST struct device *pcidev_path_on_bus(unsigned int bus, pci_devfn_t devfn); since this function isn't used that widely in the tree, i wonder if we should just add the segment_group parameter to it and and add a 0 as parameter on the call sites
File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/79877/comment/9c322bc1_6e9207f5 : PS1, Line 89: uint8_t segment; /* PCI segment */
i think i used a macro to get the bus and segment number from the secondary/max_subordinate. […]
hmm, having looked at both approaches a bit more in detail, your approach might be the better one that also has a bit less possibilities of things becoming inconsistent. i know, i'm going a bit back and forward on this one, but it's probably good that we both looked at this. oh and i hope that i didn't sound too grumpy yesterday; was already quite late when i saw this patch and replied
i'd rename this to segment_group, so that it's in line with the spec; a segment might also refer to a pci bus
-
Felix Held (Code Review)