Attention is currently required from: Shelley Chen, Hung-Te Lin, Angel Pons, Yu-Ping Wu. Jianjun Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56789 )
Change subject: libpayload/pci: Add support for bus mapping ......................................................................
Patch Set 40:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56789/comment/250471b6_b7912e68 PS37, Line 7: libpayload/pci: Split PCI interfaces as common and chip related
How about "Add support for bus mapping"?
Done
File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/56789/comment/56e496e7_e9bfe3c1 PS36, Line 405: PCI_COMMON
The config name should remain "PCI" in my opinion.
Done
https://review.coreboot.org/c/coreboot/+/56789/comment/eab728c5_2fb9f138 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 […]
In that case, PCI_IO_OPS should be set to "default n", I'm not sure if this can work properly on x86 platform.
https://review.coreboot.org/c/coreboot/+/56789/comment/e210f515_b8e0a916 PS36, Line 414: PCI_X86
PCI_IO_OPS
Done
https://review.coreboot.org/c/coreboot/+/56789/comment/9359427c_4fba13b3 PS36, Line 417: select PCI_COMMON
Shouldn't we use "depends on PCI" here?
PCI tag in libpayload only means that the common PCIe interface, I think we should select it instead of depends on it.
File payloads/libpayload/drivers/pci_io_ops.c:
https://review.coreboot.org/c/coreboot/+/56789/comment/6e11433d_86303ac2 PS35, Line 33: device
Take the chance to change this to "dev" for consistency.
Done
File payloads/libpayload/drivers/pci_map_bus_ops.c:
https://review.coreboot.org/c/coreboot/+/56789/comment/29b4eccb_0df02398 PS35, Line 38: addr
void *addr = ... […]
Done
File payloads/libpayload/drivers/pci_map_bus_ops.c:
https://review.coreboot.org/c/coreboot/+/56789/comment/48c427e5_9b1c4fae PS37, Line 3: 2008 Advanced Micro Devices, Inc. : * Copyright (C) 2008 coresystems GmbH
Please correct these.
Done
File payloads/libpayload/include/pci.h:
https://review.coreboot.org/c/coreboot/+/56789/comment/e6271448_07e88fd2 PS35, Line 105: u32 device
Take the chance to change this to "pcidev_t dev" for consistency.
Done
-
Jianjun Wang (Code Review)