Attention is currently required from: Hung-Te Lin, Rex-BC Chen, Julius Werner, Arthur Heymans, Jianjun Wang. Yu-Ping Wu 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 13:
(3 comments)
File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/63251/comment/cd6bb90b_cd7011b0 PS12, Line 151: lb_uint64_t ctrl_base;
This is the base address for those PCIe controllers that do not support ECAM. […]
Ack
File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/63251/comment/bcab00c0_03493a95 PS11, Line 36: CB_ERR
I'm not sure if this is a good idea, not every __weak function will return cb_err.
I don't understand your point. I'm just suggesting changing the return value here to another more specific error code: CB_ERR_NOT_IMPLEMENTED, to make it clear that this is actually NOT an error. No strong opinion though. Julius?
File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/63251/comment/8068cdf7_709ac8d4 PS12, Line 36: __weak enum cb_err lb_fill_pcie(struct lb_pcie *pcie) : { : return CB_ERR; : }
That might be a solution, but not every SoC will fill all of these base addresses. […]
We do want this function to be general enough to support both x86 and other platforms without ECAM support. Like Jianjun said, only the 'ctrl_base' field is for MediaTek, 'config_base' and 'atu_base' are for qualcomm (see CB:61773). TBH I'm not even sure whether 'config_base' and CONFIG_ECAM_MMCONF_BASE_ADDRESS mean the same thing. Therefore it seems difficult to have a good enough default implementation.
-
Yu-Ping Wu (Code Review)