Attention is currently required from: Shelley Chen, Hung-Te Lin, Paul Menzel, Rex-BC Chen, Julius Werner, Arthur Heymans, Yu-Ping Wu, Jianjun Wang. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63251 )
Change subject: coreboot_tables: Add PCIe info to coreboot table ......................................................................
Patch Set 19:
(1 comment)
Patchset:
PS9:
As far as I'm aware this is their current draft patch: CB:57615. Does someone who understands PCIe better than I do follow what's going on there
It really seems to be just a draft. I doubt this code works (it's more than just whitespace errors). It's not safe to derive anything about the hardware from that code. Is there public QC documentation by now? If not, I suggest to make it a requirement before they waste another 6 months ;)
(is it really normal to run that much code for each register access)?
No, and it's not necessary for every access in this case. If you look at the mapping, it doesn't change unless the accessed device/function changes. So one could at least cache the function used and only re-run the mapping procedure if that changes (the same applies to Mediatek btw. but as it's a single MMIO write to change the mapping, probably not worth it).
Oh, the respective coreboot code looks different, it restores another mapping after every access. So something is fishy...
Another thing I've noticed: in Linux, the root bus is treated very differently. After all, it looks like tested code, i.e. code that was tested for one specific setup and not written for PCIe in general.
Also, as far as I can see the mmio_size is set to 1MB in CB:61773, so I guess it's not always 4K?
It's hard to tell if this is a hardware limit or a choice made for coreboot that could change and should be passed via cbtables. AFAICS, the libpayload code only makes use of 4K. However, the bus/device/function is encoded from bit 16 up, so that _might_ suggest the mapping requires 64K per function. If it's freely configurable, I guess one could map enough to cover all buses like ECAM? (if the CPU side supports enough physical address bits)
-
Nico Huber (Code Review)