Attention is currently required from: Hung-Te Lin, Yu-Ping Wu, Jianjun Wang. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56792 )
Change subject: soc/mediatek/mt8195: Add driver to configure PCIe ......................................................................
Patch Set 10:
(7 comments)
File src/soc/mediatek/mt8195/pcie.c:
https://review.coreboot.org/c/coreboot/+/56792/comment/f633f21a_22a29b5b PS10, Line 16: static struct mtk_pcie_mmio_res pcie_mmio_res_p0[] = { : /* Some devices still need io ranges, reserve 16MB for compatibility */ : { : .cpu_addr = 0x20000000, : .pci_addr = 0x20000000, : .size = 0x1000000, : .type = IORESOURCE_IO, : }, : { : .cpu_addr = 0x21000000, : .pci_addr = 0x21000000, : .size = 0x3000000, : .type = IORESOURCE_MEM, : }, : }; In coreboot code, these resources would be reported in the bridge device's `read_resources` device operation. This calls `pci_bus_read_resources()` by default, but it's possible to call a different function that uses the values defined in here instead.
See my comment on line 64 on how to bind specific device operations to the SoC's bridge devices.
https://review.coreboot.org/c/coreboot/+/56792/comment/6d25c52a_8c364055 PS10, Line 39: const
https://review.coreboot.org/c/coreboot/+/56792/comment/eb6c4240_c91f4585 PS10, Line 54: struct pad_func *pins = pcie_pins[port]; const
https://review.coreboot.org/c/coreboot/+/56792/comment/4c4f0c8d_ca25eba8 PS10, Line 55: size_t pin_num = ARRAY_SIZE(pcie_pins[port]); nit: local variable not needed
https://review.coreboot.org/c/coreboot/+/56792/comment/7d36d9b1_2fbe229e PS10, Line 64: void mtk_pcie_prepare(void) This code should be linked to a PCI device in devicetree.cb for the bridge device. Device operations can be bound to PCI devices by matching PCI vendor/device IDs using a PCI driver. The matching is done when scanning buses, which begins with the domain device:
pci_domain_scan_bus() ---> pci_scan_bus() ---> pci_probe_dev() ---> set_pci_ops()
The `set_pci_ops()` function first looks for a matching PCI driver in the list of PCI drivers [1], and falls back to default PCI operations if a matching PCI driver isn't found. Because the PCI bridge devices are part of the SoC, their vendor/device IDs are known, so you can use a PCI driver for them. You can use `src/northbridge/intel/haswell/pcie.c` as an example.
[1]: We use some linker magic to have an array of PCI driver entries without having to declare an array in C code. Each PCI driver entry is put in a special section (see `__pci_driver` macro definition) so that the linker places them next to each other, effectively making the linker build the array for us.
https://review.coreboot.org/c/coreboot/+/56792/comment/cc68bd84_2f84d061 PS10, Line 66: struct mtk_pcie_controller *ctrl; : : ctrl = (struct mtk_pcie_controller *)xzalloc(sizeof(*ctrl)); Looks like `ctrl` isn't used anywhere else, does it need to be dynamically allocated?
https://review.coreboot.org/c/coreboot/+/56792/comment/bd9a0230_e51aa420 PS10, Line 70: 0x112f0000 Hmmm, so this value is used in the MTK common PCIe code. It would be nice to associate this value to the `struct device` itself, but I'm not sure what the best way would be.
If the SoCs have at most one PCIe controller, this value could be a Kconfig option. However, I don't know if this is the case, as I don't have any datasheets. In any case, I'd avoid this approach as it's not very flexible.
If some SoCs can have multiple PCIe controllers, one option would be to create a `mtk_pcie_soc_get_base()` function, declared in a MTK common header and defined in SoC-specific code, that takes a `const struct device *` parameter for the bridge and returns the corresponding value. For example, by checking the PCI vendor/device IDs or the PCI bus/device/function numbers.
Also, I imagine this magic base address is SoC-specific and comes from some datasheet. It would be nice to add a comment referencing the source of this value.