Attention is currently required from: Shelley Chen, Hung-Te Lin, Furquan Shaikh, Paul Menzel, Angel Pons, Yu-Ping Wu. Jianjun Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56791 )
Change subject: soc/mediatek: Add PCIe support ......................................................................
Patch Set 7:
(11 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56791/comment/8f905163_abd95494 PS6, Line 18: SSD
Which one exactly? On what board? […]
Sorry for the late response, updated the board info and boot log, thanks.
File src/soc/mediatek/common/pcie.c:
https://review.coreboot.org/c/coreboot/+/56791/comment/76b7937a_e1d8fde9 PS2, Line 329: mtk_pcie_gen3_probe
Hi Jianjun, thanks for putting this together. […]
Sorry for the late response, the device_operations has been used by soc_ops, can we still add this pci_domain_ops?
File src/soc/mediatek/common/pcie.c:
https://review.coreboot.org/c/coreboot/+/56791/comment/3533ed27_744e1727 PS6, Line 106: setctions
sections
Done
https://review.coreboot.org/c/coreboot/+/56791/comment/26c4f50c_53d5057f PS6, Line 111: mdelay(100);
One way to avoid this delay would be to assert PERST# earlier, and do something else in the meantime […]
Yes, it's truly a long time, but it defined by PCIe spec, remove it directly will cause some compatibility issues, we can make this driver as user configurable first, and try to found a way to assert PERST# earlier, thanks.
https://review.coreboot.org/c/coreboot/+/56791/comment/2f809e0c_e56fb118 PS6, Line 122: __func__, val);
Error messages should be user understandable. Please elaborate, and also document the effects.
Done
https://review.coreboot.org/c/coreboot/+/56791/comment/6f02e355_146fbbbc PS6, Line 159: res->cpu_addr | PCIE_ATR_SIZE(fls(res->size)));
Should fit in one line.
Done
https://review.coreboot.org/c/coreboot/+/56791/comment/27fbc25e_47778c52 PS6, Line 174: static int mtk_pcie_dev_assign_resource(struct device *dev,
Please excuse my ignorance, but what is special for assigning resources on MediaTek, that no common […]
Thanks for your review, the PCIe module in arm platform seems different from the X86 platform, we have a 64MBytes MMIO space starting from 0x2000_0000, but I can't found any common function that can assign this start address to the PCIe resources(e.g. BAR, Bridge window), so I add these functions for: 1. Use mtk_pcie_dev_assign_resource to maintain the usage of the MMIO space; 2. Use mtk_pcie_bridge_assign_resources to update the resource base/size information of the bridge, let the config space of the root port can get the correct memory/io base/limit.
https://review.coreboot.org/c/coreboot/+/56791/comment/03a2f7c2_23b15681 PS6, Line 180: printk(BIOS_INFO, "res->index = %#lx\n", res->index);
Looks more like debugging or spew?
Done
https://review.coreboot.org/c/coreboot/+/56791/comment/27631b49_10378538 PS6, Line 194: __func__, ctrl->mmio_io_size);
Plesae add the values to error message. […]
Done
https://review.coreboot.org/c/coreboot/+/56791/comment/26abe9ac_3d0742e8 PS6, Line 285: ret = mtk_pcie_startup_port(ctrl); : if (ret) : return;
Rewrite as […]
Done
https://review.coreboot.org/c/coreboot/+/56791/comment/b0cb8a91_540d2458 PS6, Line 289: printk(BIOS_INFO, "%s: Try to probe PCIe bus\n", __func__);
- Trying … […]
This messages seems redundant, it has been removed, thanks.
-
Jianjun Wang (Code Review)