Attention is currently required from: Shelley Chen, Ravi kumar, Furquan Shaikh, Paul Menzel, Julius Werner, Arthur Heymans, Kyösti Mälkki, mturney mturney. Prasad Malisetty 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:
(6 comments)
Patchset:
PS30: Hi Furquan/ Arthur,
Thanks for the review.
I have modified the patch-set as per the latest comments. validation is in progress.
Hi Furquan, I have some questions on few of the comments. Could you please help me to confirm, so that I will update the changes and submit in next version patch set.
Thanks -Prasad
File payloads/libpayload/drivers/pci_ops.c:
https://review.coreboot.org/c/coreboot/+/53903/comment/ff631722_7dc4b874 PS30, Line 31: ui
use static?
Hi Arthur,
Sure, we will update in next version.
Thanks -Prasad
https://review.coreboot.org/c/coreboot/+/53903/comment/e45e8cbb_1eaeadf8 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. […]
Yes, true. I will reuse and update the changes in next patch set version.
Thanks -Prasad
File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/53903/comment/faa37ac2_193ec49e PS30, Line 247: void *mmio_config_base;
You also want to expose the mmcfg size or number of busses (CONFIG_MMCONF_BUS_NUMBER)
I will check internally and update exact BUS number.
File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/53903/comment/a253f792_04fa211e PS30, Line 95: void lb_add_pci(struct lb_header *header)
why not static? where else is this called?
Yes static only. no where else called. Will update the change in next patch set.
https://review.coreboot.org/c/coreboot/+/53903/comment/fc553f81_30d5db33 PS30, Line 499: lb_add_pci(head);
guard with if (CONFIG(MMCONF_SUPPORT))
Sure, I will incorporate changes in next patch set.