Attention is currently required from: Hung-Te Lin, Rex-BC Chen, Julius Werner, Yu-Ping Wu, Jianjun Wang. Arthur Heymans 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 12:
(2 comments)
File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/63251/comment/ed96177d_c3fe5303 PS12, Line 151: lb_uint64_t ctrl_base;
This is the base address for those PCIe controllers that do not support ECAM. For MediaTek's PCIe controller, we need to setting some registers of PCIe hardware to send the TLP, so pass its base address for payloads to access these registers.
Got it. Thanks for explaining.
File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/63251/comment/65f34473_4eab7915 PS12, Line 36: __weak enum cb_err lb_fill_pcie(struct lb_pcie *pcie) : { : return CB_ERR; : }
I think we can get the information from device tree directly by define this function, so we don't need to define another macro with same value.
I'm a bit confused. I guess I'm mostly thinking in x86 terms where CONFIG_ECAM_MMCONF_BASE_ADDRESS is known at build time. I was suggesting that adding it as such to the '__weak' function would be a good idea?
Maybe we can also get rid of the __weak linking but have a call 'get_pcie_ctrl_base()' to always fill in this struct depending on some Kconfig params. Does if(CONFIG(ECAM_MMCONF_SUPPORT)) lb_pcie.cntr_base = get_pcie_ctrl_base() make sense and otherwise just fill in the config_base and confi_size?
-
Arthur Heymans (Code Review)