Change in coreboot[master]: libpayload/pci: Split PCI interfaces as common and chip related
Attention is currently required from: Shelley Chen, Hung-Te Lin, Angel Pons, Jianjun Wang. Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56789 )
Change subject: libpayload/pci: Split PCI interfaces as common and chip related ......................................................................
Patch Set 37:
(11 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56789/comment/faa111ac_f71bfaf2 PS2, Line 7: Split PCI interfaces as common and chip related Shelly, could you take over to review this?
In CB:53903, Furquan suggested using a pci_ops struct handle the differences between PCI_IO (for x86) and PCI_MMIO (for qc & mtk). This patch does a (slightly) different thing (by having different implementations of pci_{read,write}_config{8,16,32} functions), but I don't see significant problems with this approach.
Normally for embedded platform, the base address of PCIe hardware might be different, so I also provide a 'pci_update_hw_base()' function for users(e.g. depthcharge) to set its hardware base address, is that ok?
Let's discuss that in CB:56794.
File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/56789/comment/1c30a501_01745c1d PS12, Line 418:
Sorry, wrong CL link, it should be: […]
Ack
File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/56789/comment/dcf0abf1_a917f721 PS36, Line 405: PCI_COMMON The config name should remain "PCI" in my opinion.
https://review.coreboot.org/c/coreboot/+/56789/comment/3c69ee14_3568d2b8 PS36, Line 411: select PCI_COMMON : default n If one of PCI_MAP_BUS and PCI_IO_OPS must be set, then we don't need this extra config. We can use
ifeq ($(CONFIG_XXX),y)
in Makefile if needed.
https://review.coreboot.org/c/coreboot/+/56789/comment/555a0879_63c1179b PS36, Line 414: PCI_X86 PCI_IO_OPS
https://review.coreboot.org/c/coreboot/+/56789/comment/08911ed5_ebeabd7a PS36, Line 417: select PCI_COMMON Shouldn't we use "depends on PCI" here?
File payloads/libpayload/drivers/pci_io_ops.c:
https://review.coreboot.org/c/coreboot/+/56789/comment/e6690196_461aaee8 PS35, Line 33: device Take the chance to change this to "dev" for consistency.
File payloads/libpayload/drivers/pci_map_bus_ops.c:
PS12:
I think `pci_mmio_ops.c` corresponds to ECAM.
Yes ECAM is mmio, but IIUC pci_map_bus() is also mmio with custom mapping. Anyway, since ECAM is not supported in libpayload (yet?), I'm fine with the current file name.
File payloads/libpayload/drivers/pci_map_bus_ops.c:
https://review.coreboot.org/c/coreboot/+/56789/comment/3c2b6359_faea6901 PS35, Line 38: | Is the address always aligned? I mean, should we write "+" here?
https://review.coreboot.org/c/coreboot/+/56789/comment/0ccab213_644ed8eb PS35, Line 38: addr void *addr = ...
And no blank line above. We can even simplify this to
return read8((void *)(...));
File payloads/libpayload/include/pci.h:
https://review.coreboot.org/c/coreboot/+/56789/comment/37b4270a_cffcf378 PS35, Line 105: u32 device Take the chance to change this to "pcidev_t dev" for consistency.
-
Yu-Ping Wu (Code Review)