From f70ecc10995434b8de494f1f2cc0129becfb5c12 Mon Sep 17 00:00:00 2001 From: Tomasz Bursztyka Date: Wed, 18 Aug 2021 13:20:59 +0200 Subject: [PATCH] drivers/pcie: Improve and fix MBAR retrieval depending on use cases So far pcie_get_mbar() has been the only way to retrieve a MBAR. But it's logic does not fit all uses cases as we will see further. The meaning of its parameter "index" is not about BAR index but about a valid Base Address count instead. It's an arbitrary way to index MBARs unrelated to the actual BAR index. While this has proven to be just the function we needed so far, this has not been the case for MSI-X, which one (through BIR info) needs to access the BAR by their actual index. Same as ivshmem in fact, though that one did not generate any bug since it never has IO BARs nor 64bits BARs (so far?). So: - renaming existing pcie_get_mbar() to pcie_probe_mbar(), which is a more relevant name as it indeed probes the BARs to find the nth valid one. - Introducing a new pcie_get_mbar() which this time really asks for the BAR index. - Applying the change where relevant. So all use pcie_probe_mbar() now but MSI-X and ivshmem. Fixes #37444 Signed-off-by: Tomasz Bursztyka --- drivers/ethernet/eth_e1000.c | 2 +- drivers/i2c/i2c_dw.c | 2 +- drivers/pcie/host/pcie.c | 39 ++++++++++++++++++++++++----------- drivers/serial/uart_ns16550.c | 2 +- include/drivers/pcie/pcie.h | 19 +++++++++++++---- 5 files changed, 45 insertions(+), 19 deletions(-) diff --git a/drivers/ethernet/eth_e1000.c b/drivers/ethernet/eth_e1000.c index c387e242fa2..7ff56861ebe 100644 --- a/drivers/ethernet/eth_e1000.c +++ b/drivers/ethernet/eth_e1000.c @@ -244,7 +244,7 @@ int e1000_probe(const struct device *ddev) return -ENODEV; } - pcie_get_mbar(bdf, 0, &mbar); + pcie_probe_mbar(bdf, 0, &mbar); pcie_set_cmd(bdf, PCIE_CONF_CMDSTAT_MEM | PCIE_CONF_CMDSTAT_MASTER, true); diff --git a/drivers/i2c/i2c_dw.c b/drivers/i2c/i2c_dw.c index 224c7261ae7..b0227965f9e 100644 --- a/drivers/i2c/i2c_dw.c +++ b/drivers/i2c/i2c_dw.c @@ -623,7 +623,7 @@ static int i2c_dw_initialize(const struct device *dev) return -EINVAL; } - pcie_get_mbar(rom->pcie_bdf, 0, &mbar); + pcie_probe_mbar(rom->pcie_bdf, 0, &mbar); pcie_set_cmd(rom->pcie_bdf, PCIE_CONF_CMDSTAT_MEM, true); device_map(DEVICE_MMIO_RAM_PTR(dev), mbar.phys_addr, diff --git a/drivers/pcie/host/pcie.c b/drivers/pcie/host/pcie.c index 3bb576a40eb..c115b477569 100644 --- a/drivers/pcie/host/pcie.c +++ b/drivers/pcie/host/pcie.c @@ -71,22 +71,15 @@ uint32_t pcie_get_cap(pcie_bdf_t bdf, uint32_t cap_id) return reg; } -bool pcie_get_mbar(pcie_bdf_t bdf, unsigned int index, struct pcie_mbar *mbar) +bool pcie_get_mbar(pcie_bdf_t bdf, + unsigned int bar_index, + struct pcie_mbar *mbar) { + uint32_t reg = bar_index + PCIE_CONF_BAR0; uintptr_t phys_addr; - uint32_t reg; size_t size; - for (reg = PCIE_CONF_BAR0; - index > 0 && reg <= PCIE_CONF_BAR5; reg++, index--) { - uintptr_t addr = pcie_conf_read(bdf, reg); - - if (PCIE_CONF_BAR_MEM(addr) && PCIE_CONF_BAR_64(addr)) { - reg++; - } - } - - if (index != 0 || reg > PCIE_CONF_BAR5) { + if (reg > PCIE_CONF_BAR5) { return false; } @@ -136,6 +129,28 @@ bool pcie_get_mbar(pcie_bdf_t bdf, unsigned int index, struct pcie_mbar *mbar) return true; } +bool pcie_probe_mbar(pcie_bdf_t bdf, + unsigned int index, + struct pcie_mbar *mbar) +{ + uint32_t reg; + + for (reg = PCIE_CONF_BAR0; + index > 0 && reg <= PCIE_CONF_BAR5; reg++, index--) { + uintptr_t addr = pcie_conf_read(bdf, reg); + + if (PCIE_CONF_BAR_MEM(addr) && PCIE_CONF_BAR_64(addr)) { + reg++; + } + } + + if (index != 0) { + return false; + } + + return pcie_get_mbar(bdf, reg - PCIE_CONF_BAR0, mbar); +} + /* The first bit is used to indicate whether the list of reserved interrupts * have been initialized based on content stored in the irq_alloc linker * section in ROM. diff --git a/drivers/serial/uart_ns16550.c b/drivers/serial/uart_ns16550.c index d9ecd2c8c60..2d80d552db3 100644 --- a/drivers/serial/uart_ns16550.c +++ b/drivers/serial/uart_ns16550.c @@ -360,7 +360,7 @@ static int uart_ns16550_configure(const struct device *dev, goto out; } - pcie_get_mbar(dev_cfg->pcie_bdf, 0, &mbar); + pcie_probe_mbar(dev_cfg->pcie_bdf, 0, &mbar); pcie_set_cmd(dev_cfg->pcie_bdf, PCIE_CONF_CMDSTAT_MEM, true); device_map(DEVICE_MMIO_RAM_PTR(dev), mbar.phys_addr, mbar.size, diff --git a/include/drivers/pcie/pcie.h b/include/drivers/pcie/pcie.h index 12b6cafe269..327702af6f0 100644 --- a/include/drivers/pcie/pcie.h +++ b/include/drivers/pcie/pcie.h @@ -88,7 +88,18 @@ extern void pcie_conf_write(pcie_bdf_t bdf, unsigned int reg, uint32_t data); extern bool pcie_probe(pcie_bdf_t bdf, pcie_id_t id); /** - * @brief Get the nth MMIO address assigned to an endpoint. + * @brief Get the MBAR at a specific BAR index + * @param bdf the PCI(e) endpoint + * @param bar_index 0-based BAR index + * @param mbar Pointer to struct pcie_mbar + * @return true if the mbar was found and is valid, false otherwise + */ +extern bool pcie_get_mbar(pcie_bdf_t bdf, + unsigned int bar_index, + struct pcie_mbar *mbar); + +/** + * @brief Probe the nth MMIO address assigned to an endpoint. * @param bdf the PCI(e) endpoint * @param index (0-based) index * @param mbar Pointer to struct pcie_mbar @@ -100,9 +111,9 @@ extern bool pcie_probe(pcie_bdf_t bdf, pcie_id_t id); * are order-preserving with respect to the endpoint BARs: e.g., index 0 * will return the lowest-numbered memory BAR on the endpoint. */ -extern bool pcie_get_mbar(pcie_bdf_t bdf, - unsigned int index, - struct pcie_mbar *mbar); +extern bool pcie_probe_mbar(pcie_bdf_t bdf, + unsigned int index, + struct pcie_mbar *mbar); /** * @brief Set or reset bits in the endpoint command/status register.