Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities
From: Hans Zhang
Date: Fri Mar 28 2025 - 06:34:22 EST
On 2025/3/25 23:37, Hans Zhang wrote:
On 2025/3/25 23:18, Ilpo Järvinen wrote:>>
Hi Ilpo,
Another question comes to mind:
If working in EP mode, devm_pci_alloc_host_bridge will not be
executed and
there will be no struct pci_host_bridge.
Don't know if you have anything to add?
Hi Hans,
No, I don't have further ideas at this point, sorry. It seems it isn't
realistic without something more substantial that currently isn't there.
This lack of way to have a generic way to read the config before the main
struct are instanciated by the PCI core seems to be the limitation that
hinders sharing code between controller drivers and it would have been
nice to address it.
But please still make the capability list parsing code common, it should
be relatively straightforward using a macro which can take different read
functions similar to read_poll_timeout. That will avoid at least some
amount of code duplication.
Thanks for trying to come up with a solution (or thinking enough to say
it doesn't work)!
Hi Ilpo,
It's okay. It's what I'm supposed to do. Thank you very much for your
discussion with me. I'll try a macro definition like read_poll_timeout.
Will share the revised patches soon for your feedback.
Best regards,
Hans
Dear Ilpo, Mani and Bjorn,
The following is my new idea, and the following is patch. Please help to
check whether it is reasonable.
I successfully tested the CDNS and DWC drivers locally. If there are
other risks, please point them out, and we'll discuss them. Please give
your opinion. Thank you very much.
Or is the submitted patch reasonable? I'd like to ask for your advice.
Patchs:
diff --git a/drivers/pci/controller/cadence/pcie-cadence.c
b/drivers/pci/controller/cadence/pcie-cadence.c
index 204e045aed8c..92aea42a18d0 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.c
+++ b/drivers/pci/controller/cadence/pcie-cadence.c
@@ -5,9 +5,37 @@
#include <linux/kernel.h>
#include <linux/of.h>
+#include <linux/pci.h>
#include "pcie-cadence.h"
+static int cdns_pcie_read_cfg(void *priv, unsigned int devfn, int where,
+ int size, u32 *val)
+{
+ struct cdns_pcie *pcie = priv;
+
+ if (size == 4)
+ *val = cdns_pcie_readl(pcie, where);
+ else if (size == 2)
+ *val = cdns_pcie_readw(pcie, where);
+ else if (size == 1)
+ *val = cdns_pcie_readb(pcie, where);
+
+ return PCIBIOS_SUCCESSFUL;
+}
+
+u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
+{
+ return PCI_FIND_NEXT_CAP_TTL(pcie, 0, cdns_pcie_read_cfg,
+ PCI_CAPABILITY_LIST, cap);
+}
+
+u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
+{
+ return PCI_FIND_NEXT_EXT_CAPABILITY(pcie, 0, cdns_pcie_read_cfg, 0,
+ cap);
+}
+
void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie)
{
u32 delay = 0x3;
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h
b/drivers/pci/controller/cadence/pcie-cadence.h
index f5eeff834ec1..dd7cb6b6b291 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -398,6 +398,16 @@ static inline u32 cdns_pcie_readl(struct cdns_pcie
*pcie, u32 reg)
return readl(pcie->reg_base + reg);
}
+static inline u32 cdns_pcie_readw(struct cdns_pcie *pcie, u32 reg)
+{
+ return readw(pcie->reg_base + reg);
+}
+
+static inline u32 cdns_pcie_readb(struct cdns_pcie *pcie, u32 reg)
+{
+ return readb(pcie->reg_base + reg);
+}
+
static inline u32 cdns_pcie_read_sz(void __iomem *addr, int size)
{
void __iomem *aligned_addr = PTR_ALIGN_DOWN(addr, 0x4);
@@ -557,6 +567,9 @@ static inline int cdns_pcie_ep_setup(struct
cdns_pcie_ep *ep)
}
#endif
+u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap);
+u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap);
+
void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie);
void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 busnr,
u8 fn,
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index 145e7f579072..1b579dbc5cb1 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -203,83 +203,26 @@ void dw_pcie_version_detect(struct dw_pcie *pci)
pci->type = ver;
}
-/*
- * These interfaces resemble the pci_find_*capability() interfaces, but
these
- * are for configuring host controllers, which are bridges *to* PCI
devices but
- * are not PCI devices themselves.
- */
-static u8 __dw_pcie_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
- u8 cap)
+static int dw_pcie_read_cfg(void *priv, unsigned int devfn, int where,
int size,
+ u32 *val)
{
- u8 cap_id, next_cap_ptr;
- u16 reg;
-
- if (!cap_ptr)
- return 0;
-
- reg = dw_pcie_readw_dbi(pci, cap_ptr);
- cap_id = (reg & 0x00ff);
-
- if (cap_id > PCI_CAP_ID_MAX)
- return 0;
+ struct dw_pcie *pcie = priv;
+ *val = dw_pcie_read_dbi(pcie, where, size);
- if (cap_id == cap)
- return cap_ptr;
-
- next_cap_ptr = (reg & 0xff00) >> 8;
- return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
+ return PCIBIOS_SUCCESSFUL;
}
u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
{
- u8 next_cap_ptr;
- u16 reg;
-
- reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
- next_cap_ptr = (reg & 0x00ff);
-
- return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
+ return PCI_FIND_NEXT_CAP_TTL(pcie, 0, dw_pcie_read_cfg,
+ PCI_CAPABILITY_LIST, cap);
}
EXPORT_SYMBOL_GPL(dw_pcie_find_capability);
-static u16 dw_pcie_find_next_ext_capability(struct dw_pcie *pci, u16 start,
- u8 cap)
-{
- u32 header;
- int ttl;
- int pos = PCI_CFG_SPACE_SIZE;
-
- /* minimum 8 bytes per capability */
- ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
-
- if (start)
- pos = start;
-
- header = dw_pcie_readl_dbi(pci, pos);
- /*
- * If we have no capabilities, this is indicated by cap ID,
- * cap version and next pointer all being 0.
- */
- if (header == 0)
- return 0;
-
- while (ttl-- > 0) {
- if (PCI_EXT_CAP_ID(header) == cap && pos != start)
- return pos;
-
- pos = PCI_EXT_CAP_NEXT(header);
- if (pos < PCI_CFG_SPACE_SIZE)
- break;
-
- header = dw_pcie_readl_dbi(pci, pos);
- }
-
- return 0;
-}
-
u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
{
- return dw_pcie_find_next_ext_capability(pci, 0, cap);
+ return PCI_FIND_NEXT_EXT_CAPABILITY(pcie, 0, dw_pcie_read_cfg, 0,
+ cap);
}
EXPORT_SYMBOL_GPL(dw_pcie_find_ext_capability);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 869d204a70a3..af7467c3143d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -423,28 +423,27 @@ static int pci_dev_str_match(struct pci_dev *dev,
const char *p,
return 1;
}
-static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
- u8 pos, int cap, int *ttl)
+static int __pci_bus_read_config(void *priv, unsigned int devfn, int where,
+ u32 size, u32 *val)
{
- u8 id;
- u16 ent;
+ struct pci_bus *bus = priv;
+ int ret;
- pci_bus_read_config_byte(bus, devfn, pos, &pos);
+ if (size == 1)
+ ret = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
+ else if (size == 2)
+ ret = pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
+ else
+ ret = pci_bus_read_config_dword(bus, devfn, where, val);
- while ((*ttl)--) {
- if (pos < 0x40)
- break;
- pos &= ~3;
- pci_bus_read_config_word(bus, devfn, pos, &ent);
+ return ret;
+}
- id = ent & 0xff;
- if (id == 0xff)
- break;
- if (id == cap)
- return pos;
- pos = (ent >> 8);
- }
- return 0;
+static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
+ u8 pos, int cap, int *ttl)
+{
+ return PCI_FIND_NEXT_CAP_TTL(bus, devfn, __pci_bus_read_config, pos,
+ cap);
}
static u8 __pci_find_next_cap(struct pci_bus *bus, unsigned int devfn,
@@ -553,42 +552,11 @@ EXPORT_SYMBOL(pci_bus_find_capability);
*/
u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 start, int cap)
{
- u32 header;
- int ttl;
- u16 pos = PCI_CFG_SPACE_SIZE;
-
- /* minimum 8 bytes per capability */
- ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
-
if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
return 0;
- if (start)
- pos = start;
-
- if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
- return 0;
-
- /*
- * If we have no capabilities, this is indicated by cap ID,
- * cap version and next pointer all being 0.
- */
- if (header == 0)
- return 0;
-
- while (ttl-- > 0) {
- if (PCI_EXT_CAP_ID(header) == cap && pos != start)
- return pos;
-
- pos = PCI_EXT_CAP_NEXT(header);
- if (pos < PCI_CFG_SPACE_SIZE)
- break;
-
- if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
- break;
- }
-
- return 0;
+ return PCI_FIND_NEXT_EXT_CAPABILITY(dev->bus, dev->devfn,
+ __pci_bus_read_config, start, cap);
}
EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 47b31ad724fa..0d5dfd010a01 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1198,6 +1198,64 @@ void pci_sort_breadthfirst(void);
/* Generic PCI functions exported to card drivers */
+/* Extended capability finder */
+#define PCI_FIND_NEXT_CAP_TTL(priv, devfn, read_cfg, start, cap)
\
+ ({ \
+ typeof(start) __pos = (start); \
+ int __ttl = PCI_FIND_CAP_TTL; \
+ u16 __ent = 0; \
+ u8 __found_pos = 0; \
+ u8 __id; \
+
\
+ read_cfg((priv), (devfn), __pos, 1, (u32 *)&__pos); \
+
\
+ while (__ttl--) { \
+ if (__pos < 0x40) \
+ break; \
+ __pos &= ~3; \
+ read_cfg((priv), (devfn), __pos, 2, (u32 *)&__ent); \
+
\
+ __id = __ent & 0xff; \
+ if (__id == 0xff) \
+ break; \
+ if (__id == (cap)) { \
+ __found_pos = __pos; \
+ break; \
+ } \
+ __pos = (__ent >> 8); \
+ } \
+ __found_pos; \
+ })
+
+/* Extended capability finder */
+#define PCI_FIND_NEXT_EXT_CAPABILITY(priv, devfn, read_cfg, start, cap)
\
+ ({ \
+ u16 __pos = (start) ?: PCI_CFG_SPACE_SIZE; \
+ u16 __found_pos = 0; \
+ int __ttl, __ret; \
+ u32 __header; \
+
\
+ __ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8; \
+ while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) { \
+ __ret = read_cfg((priv), (devfn), __pos, 4, \
+ &__header); \
+ if (__ret != PCIBIOS_SUCCESSFUL) \
+ break; \
+
\
+ if (__header == 0) \
+ break; \
+
\
+ if (PCI_EXT_CAP_ID(__header) == (cap) && \
+ __pos != start) { \
+ __found_pos = __pos; \
+ break; \
+ } \
+
\
+ __pos = PCI_EXT_CAP_NEXT(__header); \
+ } \
+ __found_pos; \
+ })
+
u8 pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn,
int cap);
u8 pci_find_capability(struct pci_dev *dev, int cap);
u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap);
Best regards,
Hans