Attention is currently required from: Hung-Te Lin, 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 15:
(2 comments)
Patchset:
PS9:
LGTM from a general coding point of view now, someone who knows the PCIe stuff should review for +2.
Some of the fields seem to target specific implementations and not generic PCIe. So here are some basic questions: * Do we need all the fields in one struct? Looking ahead in the queue it seems for Mediatek only `ctrl_base` is used and the fields for ECAM `config_base`/`config_size` are not. Should they be mutually exclusive? * If we go for a single struct/table entry, should it be further tagged so the consumer knows what driver to use, what fields are valid? * I couldn't find a patch that makes use of `atu_base`. Is this part of a generic concept or will this also be used in an SoC specific way?
File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/63251/comment/3d3ea401_dc41e6db PS15, Line 153: uint32_t config_size; I'd make this 64-bit wide too.
There seem to be implementations that cover more than one PCIe segment group, and hence are greater than 256MiB. So while 2GiB (2048 buses) may seem very large, it's not impossible that we will see bigger config spaces.
-
Nico Huber (Code Review)