Attention is currently required from: Arthur Heymans, Christian Walter, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Patrick Rudolph, Shuo Liu, Tim Chu.
Nico Huber has posted comments on this change by Shuo Liu. ( https://review.coreboot.org/c/coreboot/+/83538?usp=email )
Change subject: soc/intel/xeon_sp: Reserve FSP MMIO high window ......................................................................
Patch Set 2: Code-Review+1
(5 comments)
Patchset:
PS2: Side note: This whole situation is becoming too messy for my taste. Especially as there seems still no progress in documenting the FSP (at least none that I've heard of). I think we should make it a requirement for future FSP versions (GNR + 1) that FSP does *not* do any ressource assignments, and otherwise not accept it upstream. I'm sure this would have saved us all a huge amount of time by now.
That's just my opinion. I'm losing faith here because it feels like I'm not giving the review the necessary attention. Attention only necessary because of a botched blob design. Only option out seems to not review.
Commit Message:
https://review.coreboot.org/c/coreboot/+/83538/comment/7b281765_37d6b0db?usp... : PS1, Line 7: soc/intel/xeon_sp: Reserve FSP MMIO high window
GNR and later SoC also fit.
I don't understand. I meant they are not covered by this change, so the commit summary should mention SPR.
There will be MMIO ranges not for PCI domain usage, and thus forming holes.
What does that mean "not for PCI domain usage"? Is it used for anything else?
https://review.coreboot.org/c/coreboot/+/83538/comment/650dbb46_e0d481c6?usp... : PS1, Line 19: is especially important : on systems with 2 or more sockets, where each socket has multiple : domains.
On multiple socket system, each socket will have IIO stacks and PCI domains under them, and thus the […]
Well, you could also tell it to use WB default instead. But I get it, Linux relies too much on the BIOS there (I very much would have expected it to use PAT right away).
That Linux complains would be nice to have in the commit message (that's more informative than "it's important").
File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/83538/comment/8a529726_b1e4003c?usp... : PS1, Line 375: index++;
Yes, mmapvtd will have both anonymous static resources (starting from 0x0) and PCI resources (0x180, […]
No, I was not referring to any 0x180, but to the standard PCI BARs that pci_dev_read_resources() would add if they exist (if not, why call it?). Solution looks ok, though.
File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/83538/comment/55147469_e0bbca29?usp... : PS2, Line 371: * MMIO high window has to be added in set_resources instead of read_resources : * due to that adding in read_resources will cause the whole window reserved : * and cannot be used for resource allocation. ```suggestion * The MMIO high window has to be added in set_resources() instead of * read_resources(). Because adding in read_resources() would cause the * whole window to be reserved, and it couldn't be used for resource * allocation. ```