Attention is currently required from: Shelley Chen, Hung-Te Lin, Furquan Shaikh, Yu-Ping Wu, Jianjun Wang. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56791 )
Change subject: soc/mediatek: Add PCIe support ......................................................................
Patch Set 9:
(6 comments)
File src/soc/mediatek/common/include/soc/pcie_common.h:
https://review.coreboot.org/c/coreboot/+/56791/comment/a57c12cc_ac6e70e5 PS9, Line 6: #include <device/device.h> Please add:
#include <types.h>
File src/soc/mediatek/common/pcie.c:
https://review.coreboot.org/c/coreboot/+/56791/comment/c7175f66_80c49ff6 PS9, Line 37: val please add parentheses around macro parameters
https://review.coreboot.org/c/coreboot/+/56791/comment/5309f2a8_28a9ac77 PS9, Line 69: remove space after `*` unary operator
https://review.coreboot.org/c/coreboot/+/56791/comment/0357fdb5_3e30bf54 PS9, Line 69: static const char * const ltssm_str[] = { Is this Mediatek-specific?
https://review.coreboot.org/c/coreboot/+/56791/comment/c4d365d1_fcfacc8b PS9, Line 103: devfn = (dev >> 12) & 0xf; : val = PCIE_CFG_HEADER((dev >> 20) & 0xf, devfn); This seems unnecessarily complex. Looks like this is equivalent:
val = (dev >> 12) & 0x0f0f;
I don't know where the 0xf masks come from. Maybe it's because the register fields are only 4 bits wide, but I don't have any datasheets to check.
https://review.coreboot.org/c/coreboot/+/56791/comment/26e0fd6f_ac87fdae PS9, Line 218: 0x100000 What is this magic number?