Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41363 )
Change subject: device: avoid fixed resources hanging off pci domain device ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41363/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41363/2//COMMIT_MSG@9 PS2, Line 9: Some chipsets are adding resources to the pci domain device. As
Isn't this the right place for platform resources? How do other chipsets do it?
Great question. I've been asking that myself. I've typically seen resources hanging off of 'real' devices.
I'm going to meander a bit because these topics are related. The xeon_sp devices have this notion of pstacks which are bridges, but these don't show up as probeable devices. The parameters are provided by FSP, but we need to group the pci devices that do show up under these virtual devices. The reason is that the windows for decode mmio, io, and buses follow these virtual devices. There's a lot of nasty code that needs to be removed, but we need to be able to handle probing grouped under these virtual devices with pre assigned bus numbers as well as resource windows.
Next up is the multiple 'link's added to struct bus. Seemingly this is for handling the hypertransport topologies more 1:1. The code handling all this is pretty gnarly, and I haven't dug into it in a while. I bring it up because a linked list of buses sounds like it could be a possible solution to the pstack thing I mentioned above. However, struct bus doesn't have resources -- only a struct device. Because of that we can't provide a window/bridge using struct bus. We need a struct device.
That's for some background, but what I don't have my head wrapped around is this: what is a pci domain? And what does the root device represent? Can there and when would there be multiple pci domains? Similarly where do we land the notion of address space and where should those resources be reported? Why would fixed resources be added to pci domain device? Why not on the root itself?
For the cpu cluster we have mainly nothing being reported resource-wise of the cpu cluster device.
$ git grep -A 2 PATH_CPU | grep ops |awk '{print $NF }' |sort | uniq | sed -e 's/&//' -e 's/;//' | while read f; do git grep -A 3 $f | grep read_resources; done | awk '{ print $NF }' | sort | uniq -c 32 noop_read_resources, 10 soc_read_resources,
And only the ARM devices supply soc_read_resources:
$ git grep -A 2 PATH_CPU | grep ops |awk '{print $NF }' |sort | uniq | sed -e 's/&//' -e 's/;//' | while read f; do git grep -A 3 $f | grep read_resources; done | grep -v noop_read_resources | cut -d- -f1 src/soc/cavium/cn81xx/soc.c src/soc/mediatek/mt8173/soc.c src/soc/mediatek/mt8183/soc.c src/soc/nvidia/tegra210/soc.c src/soc/qualcomm/ipq40xx/soc.c src/soc/qualcomm/ipq806x/soc.c src/soc/qualcomm/qcs405/soc.c src/soc/qualcomm/sc7180/soc.c src/soc/qualcomm/sdm845/soc.c src/soc/rockchip/rk3399/soc.c
That's for reporting dram resources because there's no other device to hang the resources off of.
Something needs to give semantically for how to proper model all this in code. pci domain is not the entire address space for decoding memory and io, but we've smashed those concepts into it. Back to my 'real' device comment: mainly the registers associated with the device that provides those decode windows houses the resources. Many times the registers just so happen to show up on a pci device under the pci domain.
I'd love to hear your thoughts on what constructs we need to provide. Orienting around address space: we have a global root space providing the full addressing, but then there are decode windows (both positive and subtractive) which route those transactions. struct bus isn't sufficient for topology grouping alone as it really needs a struct device. But we have some pretty loose semantics around IORESOURCE_* and DEVICE_PATH_* where we don't really have the concept of orienting around address space windows. We do have the hierarchy ability, but I think how we currently struct that isn't conducive to reality in how chips are designed and glued together.
https://review.coreboot.org/c/coreboot/+/41363/2/src/device/device.c File src/device/device.c:
https://review.coreboot.org/c/coreboot/+/41363/2/src/device/device.c@935 PS2, Line 935: if (dev->path.type == DEVICE_PATH_DOMAIN) { : initialize_domain_memranges(ranges, res, type); : constrain_domain_resources(dev, ranges, type); : } else { : initialize_bridge_memranges(ranges, res, type); : }
I've had only a quick look at the callers. But it looks like they always know if we deal with a domain or not (and then need a non-const pointer or not). So alternatively to removing all `const`, one could split this function up, I guess. But it would probably only help us with a single `const`...
I had to peel the const off because of update_constraints() and search_bus_resources() expecting the call back to pass a non-const struct device. I can see if I can fix that if you want.
Haven't yet figured out, why this is a two-in-one function.
pci domain is device that houses the global decode of the address space. For assigning resources one only needs to do depth first accumulation of resources. Then at assignment time one only needs to supply the bridges below it their designated windows -- a very flat view of the address space. i.e. domain doesn't care what's below those bridges. The bridges provide the decode windows.