[coreboot-gerrit] [M] Change in coreboot[main]: soc/intel/xeon_sp: Initial support for PCI multi segment groups

[Nico Huber (Code Review)]

Attention is currently required from: Angel Pons, Annie Chen, Arthur Heymans, Arthur Heymans, Chen, Gang C, Christian Walter, David Hendricks, Felix Held, Jincheng Li, Johnny Lin, Naresh Solanki, Nill Ge, Patrick Rudolph, Shuo Liu, TangYiwei, Tim Chu.

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79878?usp=email )

Change subject: soc/intel/xeon_sp: Initial support for PCI multi segment groups
......................................................................

Patch Set 18: Code-Review+1

(1 comment)

File src/soc/intel/xeon_sp/chip_common.c:

https://review.coreboot.org/c/coreboot/+/79878/comment/7b10484c_914a62be :
PS15, Line 276:
> Done

Ah, just read above message after I wrote the below. Still going to post
it for completeness...

Maybe a misunderstanding of `struct device_path`. It doesn't describe a full
path but just one element of the path, e.g. on this 'host bus' (expressed by
the upstream pointer) this 'domain id' (expressed by struct device_path).

We added some more information like the socket into the 'domain id' and later
the secondary bus. While that's a convenient way to create unique ids, I don't
think we should use that to retrieve information like socket or bus numbers
from the domain path element. If such information is needed, we should get it
from the device tree (e.g. the secondary bus is `dev->downstream->secondary`
and not `dev->path.domain.domain & 0xff`; and the PCI segment group is
`dev->downstream->segment_group` as correctly described in this patch).

(If there is a need to retrieve the socket number, maybe it is time to introduce
a socket node to the devicetree? or if that's too much overhead, make it official
and add a `socket` field to `struct domain_path`?)