Attention is currently required from: Shelley Chen, Ravi kumar, Julius Werner, Prasad Malisetty, mturney mturney. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/53903 )
Change subject: libpayload: Add MMIO support in PCI lib ......................................................................
Patch Set 22:
(25 comments)
Patchset:
PS22: This change should be split into at least the following 2 CLs:
1. Add support for passing PCI MMCONF info from coreboot to libpayload (i.e. adding PCI MMCONF base to coreboot tables in coreboot and parsing that info and saving that info in lib_sysinfo in libpayload). 2. Add support for performing PCI MMCONF operations in libpayload (This is the comment I posted in drivers/pci.c)
File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/53903/comment/88b902da_12d0da67 PS18, Line 401: MMCONF_SUPPORT This config is not really required in libpayload. Coreboot already has this config and it should expose the coreboot table entry for `MMCONF_BASE_ADDRESS` only when `MMCONF_SUPPORT` is enabled in coreboot. Libpayload needs to basically check if the coreboot table entry is exposed and use pci mmio ops if MMCONF_BASE_ADDRESS is available, else fall back to pci io ops.
File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/53903/comment/e4befd85_af62cdf3 PS22, Line 401: MMCONF_SUPPORT This config is not required in libpayload. Reasoning: We already have `MMCONF_SUPPORT` in coreboot which can be used to fill in pci_mmconf_info record. If that record is present, then libpayload knows that the platform supports PCI MMCONF ops.
We need special handling for PCI IO ops which are supported only on x86 platforms. That will require a Kconfig of its own.
File payloads/libpayload/configs/config.herobrine:
https://review.coreboot.org/c/coreboot/+/53903/comment/8b7ecda2_5574fdb9 PS18, Line 1: CONFIG_LP_CHROMEOS=y Changes in this file should be pushed as a separate CL. Let's use this CL to basically enable support for pci mmio ops in libpayload.
File payloads/libpayload/configs/config.herobrine:
https://review.coreboot.org/c/coreboot/+/53903/comment/6ea27b16_b0c0515a PS22, Line 7: CONFIG_LP_PCI=y This is not really required. It is always set to 'y' in Kconfig.
https://review.coreboot.org/c/coreboot/+/53903/comment/f8649851_dbb7981f PS22, Line 8: CONFIG_LP_PCIE_NVME This config doesn't exist and is not required.
File payloads/libpayload/drivers/pci.c:
https://review.coreboot.org/c/coreboot/+/53903/comment/b3c82754_532845d5 PS22, Line 35: #if CONFIG(LP_MMCONF_SUPPORT) : void *cfg_addr = lib_sysinfo.pci_ep_cfg_base + reg; : return read8(cfg_addr); : #endif : outl(device | (reg & ~3), 0xCF8); : return inb(0xCFC + (reg & 3)); Rather than forcing all architectures to define dummy in/out operations, it would be better to split this into two separate handlers. Something like this should work:
1. Create a file pci_io_ops.c and move all the current pci_{read|write}_config{8|16|32} definitions to it with names pci_io_{read|write}_config{8|16|32}. Define a structure `pci_ops` which pci_io_ops.c can initialize to the moved functions. Add a new Kconfig `PCI_IO_OPS` which will default to 'y' only for ARCH_X86.
2. Create a file pci_mmio_ops.c and add pci_mmio_{read|write}_config{8|16|32} functions and similarly initialize pci_mmio_ops structure.
3. In pci.c, provide a global `pci_ops` which can be set to appropriate ops structure and then used by the pci_{read|write}_config{8|16|32} functions. Following changes would be required here: a. pci_set_ops: Set `pci_ops` to `pci_mmio_ops` if non-zero `pci_mmconf_base` is passed in coreboot tables. Else set to `pci_io_ops` if `PCI_IO_OPS` is selected. b. pci_{read|write}_config{8|16|32}: These will have to add a `die_if(pci_ops == NULL, ""); to ensure pci_ops is properly initialized and call appropriate op using the ops structure: pci_ops->{read|write}{8|16|32}(...);
Finally, you will have to call `pci_set_ops()` for each architecture after `lib_get_sysinfo()` is done (and before pci_init for x86).
File payloads/libpayload/include/arm64/arch/io.h:
PS22: I don't think we need to add dummy in/out operations to arm/arm64 files. See my comments in pci.c
File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/53903/comment/ca298e90_e49d1cde PS22, Line 90: CB_TAG_PCI_EP_CONFIG `CB_TAG_PCI_MMCONF`
https://review.coreboot.org/c/coreboot/+/53903/comment/ae89efcc_8219b766 PS22, Line 90: Use tabs instead of spaces like other enums.
https://review.coreboot.org/c/coreboot/+/53903/comment/0d087180_b1ddfbe3 PS22, Line 243: EP Drop EP and instead use MMCONF. Actually the structure name is self-descriptive. Comment isn't really required.
https://review.coreboot.org/c/coreboot/+/53903/comment/025905c3_d918cfbf PS22, Line 244: cb_pci_config_info `cb_pci_mmconf_info`
https://review.coreboot.org/c/coreboot/+/53903/comment/3c8ea995_bef21a62 PS22, Line 247: config_base mmconf_base
File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/53903/comment/4431cc7e_9e25216b PS22, Line 150: pci_ep_cfg_base `uintptr_t pci_mmconf_base;`
File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/c/coreboot/+/53903/comment/50596a13_930ef72b PS22, Line 122: pci_ep_cfg_base pci_mmconf_base
https://review.coreboot.org/c/coreboot/+/53903/comment/764474c4_ade864d6 PS22, Line 431: cb_parse_pci_ep_config_base `cb_parse_pci_mmconf_info`
File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/53903/comment/5d6d2ed2_9f99217f PS22, Line 93: 0x00cd Can you please use 0x0042 to avoid mixing up with CMOS-related options?
https://review.coreboot.org/c/coreboot/+/53903/comment/bbd18722_62c23054 PS22, Line 93: = Please use tabs after the enum name to align `= <val>,` just like the other enums.
https://review.coreboot.org/c/coreboot/+/53903/comment/0359221b_12a75ad6 PS22, Line 93: LB_TAG_PCI_EP_CONFIG Why EP? I think it would be better to name this `LB_TAG_PCI_MMCONF` or `LB_TAG_PCI_MMCONFIG` to indicate that this record provides PCI MMCONFIG information.
https://review.coreboot.org/c/coreboot/+/53903/comment/6715ea3b_9f180996 PS22, Line 332: lb_pci_config_info Probably rename as `lb_pci_mmconfig_info`
https://review.coreboot.org/c/coreboot/+/53903/comment/ce14da32_12d64e8c PS22, Line 335: void *config_base; `uint64_t mmconf_base;`
File src/include/boot/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/53903/comment/2b5b949f_64754f61 PS22, Line 24: /*Fill PCIe endpoint config window base address*/ : void lb_fill_ep_config_window(struct lb_pci_config_info *pci); : void lb_add_pci(struct lb_header *header); : void lb_add_pci_config(struct lb_pci_config_info *info, : struct lb_pci_config_info *data); None of these functions are required for back and forth with SoC code. You only need a single `lb_add_pci_mmconf()` and its scope can be limited to coreboot_tables.c.
There are already device Kconfigs for MMCONF base and length that can be used. Please see: * MMCONF_BASE_ADDRESS * MMCONF_LENGTH
File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/53903/comment/0d06bcc2_53e0e0fa PS22, Line 108: lb_fill_ep_config_window(pci); There is no need to call back into SoC code. There are Kconfigs that already provide this information and utilized within other common code:
``` pci->mmconf_base = CONFIG_MMCONF_BASE_ADDRESS; ```
https://review.coreboot.org/c/coreboot/+/53903/comment/89132618_b437ccfe PS22, Line 505: if (CONFIG(SC7280_PCIE_NVME)) You don't need SoC specific check here. There is already config `MMCONF_SUPPORT` that can be used:
``` if (CONFIG(MMCONF_SUPPORT)) lb_add_pci_mmconf(head); ```
File src/soc/qualcomm/sc7280/Kconfig:
https://review.coreboot.org/c/coreboot/+/53903/comment/d97209e7_80516146 PS22, Line 41: config SC7280_PCIE_NVME This is not really required. Please see comment in `coreboot_table.c`.
-
Furquan Shaikh (Code Review)