Attention is currently required from: Shelley Chen, Ravi kumar, Furquan Shaikh, Paul Menzel, Julius Werner, Kyösti Mälkki, mturney mturney. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/53903 )
Change subject: libpayload: Add MMIO support in PCI lib ......................................................................
Patch Set 30: -Code-Review
(5 comments)
File payloads/libpayload/drivers/pci_ops.c:
https://review.coreboot.org/c/coreboot/+/53903/comment/a2d3910a_43dc71b8 PS30, Line 31: ui use static?
https://review.coreboot.org/c/coreboot/+/53903/comment/a3bbca00_32231bdd PS30, Line 33: /* Get PCI MMCONFIG BASE address */ : uintptr_t get_pci_mmio_cfgbase(pcidev_t dev) : { : uintptr_t mmconf_base = (uintptr_t)lib_sysinfo.pci_mmio_cfg_base; : u32 devfn = ((dev >> 3) & 0x1f) | (dev & 0x07); : : return (mmconf_base | devfn); : } : : u8 pci_read_config8(pcidev_t dev, u16 reg) : { : void *addr; : uintptr_t mmio_base = get_pci_mmio_cfgbase(dev); : : addr = (void *)(mmio_base | reg); : return read8(addr); : } Did you have a look at include/device/pci_mmio_cfg.h ? It's pretty clever and compact on how to deal with pci mmio. It uses an union to avoid all the castings. Maybe you can reuse some concepts? The licensing is different but maybe Kyösti is willing to allow it under the libpayload license.
File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/53903/comment/c9f8dfeb_6860e04a PS30, Line 247: void *mmio_config_base; You also want to expose the mmcfg size or number of busses (CONFIG_MMCONF_BUS_NUMBER)
File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/53903/comment/fbf2d5d6_a4bbba53 PS30, Line 95: void lb_add_pci(struct lb_header *header) why not static? where else is this called?
https://review.coreboot.org/c/coreboot/+/53903/comment/8af914c9_f027dde9 PS30, Line 499: lb_add_pci(head); guard with if (CONFIG(MMCONF_SUPPORT))