Re: [v2] PCI: cadence: Add configuration space capability search API

From: Hans Zhang
Date: Fri Mar 14 2025 - 20:06:24 EST

Next message: Josh Poimboeuf: "Re: [PATCH 04/20] x86/cpu: Use named asm operands in clflushopt()"
Previous message: Josh Poimboeuf: "Re: [PATCH 14/20] x86/barrier: Use alternative_io() in 32-bit barrier functions"
In reply to: Hans Zhang: "Re: [v2] PCI: cadence: Add configuration space capability search API"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2025/3/14 21:05, Manivannan Sadhasivam wrote:

On Mon, Mar 10, 2025 at 11:09:54PM +0800, Hans Zhang wrote:

On 2025/3/9 18:02, Siddharth Vadapalli wrote:

Hi Siddharth,

Prior to this patch, I don't think hard-coded is that reasonable. Because
the SOC design of each company does not guarantee that the offset of each
capability is the same. This parameter can be configured when selecting PCIe
configuration options. The current code that just happens to hit the offset
address is the same.

1. You are modifying the driver for the Cadence PCIe Controller IP and
not the one for your SoC (a.k.a the application/glue/wrapper driver).
2. The offsets are tied to the Controller IP and not to your SoC. Any
differences that arise due to IP Integration into your SoC should be
handled in the Glue driver (the one which you haven't upstreamed yet).
3. If the offsets in the Controller IP itself have changed, this
indicates a different IP version which has nothing to do with the SoC
that you are using.

Please clarify whether you are facing problems with the offsets due to a
difference in the IP Controller Version, or due to the way in which the IP
was integrated into your SoC.

Hi Siddharth,

I have consulted several PCIe RTL designers in the past two days. They told
me that the controller IP of Synopsys or Cadence can be configured with the
offset address of the capability. I don't think it has anything to do with
SOC, it's just a feature of PCIe controller IP. In particular, the number of
extended capability is relatively large. When RTL is generated, one more
configuration may cause the offset addresses of extended capability to be
different. Therefore, it is unreasonable to assign all the offset addresses
in advance.

Here, I want to make Cadence PCIe common driver more general. When we keep
developing new SoCs, the capability or extended capability offset address
may eventually be inconsistent.

Thank you very much Siddharth for discussing this patch with me. I would
like to know what other maintainers have to say about this.

Even though this patch is mostly for an out of tree controller driver which is
not going to be upstreamed, the patch itself is serving some purpose. I really
like to avoid the hardcoded offsets wherever possible. So I'm in favor of this
patch.

However, these newly introduced functions are a duplicated version of DWC
functions. So we will end up with duplicated functions in multiple places. I'd
like them to be moved (both this and DWC) to drivers/pci/pci.c if possible. The
generic function *_find_capability() can accept the controller specific readl/
readw APIs and the controller specific private data.

Hi Mani,

Thanks Mani for the idea. Please check whether the following patches are OK. If yes, I will submit a series of patches again.

I have tested on RK3588 development board, as well as cdns platform is OK.

diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
index 204e045aed8c..e1ce3ad612c9 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.c
+++ b/drivers/pci/controller/cadence/pcie-cadence.c
@@ -8,6 +8,31 @@

#include "pcie-cadence.h"

+static u32 cdns_pcie_read_cfg(void *priv, int where, int size)
+{
+ struct cdns_pcie *pcie = priv;
+ u32 val;
+
+ if (size == 4)
+ val = readl(pcie->reg_base + where);
+ else if (size == 2)
+ val = readw(pcie->reg_base + where);
+ else if (size == 1)
+ val = readb(pcie->reg_base + where);
+
+ return val;
+}
+
+u8 cdns_find_cap(struct cdns_pcie *pcie, int cap)
+{
+ return pci_generic_find_capability(pcie, cdns_pcie_read_cfg, cap);
+}
+
+u16 cdns_find_ext_cap(struct cdns_pcie *pcie, int cap)
+{
+ return pci_generic_find_ext_capability(pcie, cdns_pcie_read_cfg, 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..e90464c4a660 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -557,6 +557,9 @@ static inline int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
}
#endif

+u8 cdns_find_cap(struct cdns_pcie *pcie, int cap);
+u16 cdns_find_ext_cap(struct cdns_pcie *pcie, int 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..4b645ad58b35 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -283,6 +283,22 @@ u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
}
EXPORT_SYMBOL_GPL(dw_pcie_find_ext_capability);

+static u32 dwc_pcie_read_cfg(void *priv, int where, int size)
+{
+ struct dw_pcie *pcie = priv;
+ return dw_pcie_read_dbi(pcie, where, size);
+}
+
+u8 dwc_find_cap(struct dw_pcie *pcie, int cap)
+{
+ return pci_generic_find_capability(pcie, dwc_pcie_read_cfg, cap);
+}
+
+u16 dwc_find_ext_cap(struct dw_pcie *pcie, int cap)
+{
+ return pci_generic_find_ext_capability(pcie, dwc_pcie_read_cfg, cap);
+}
+
int dw_pcie_read(void __iomem *addr, int size, u32 *val)
{
if (!IS_ALIGNED((uintptr_t)addr, size)) {
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 501d9ddfea16..260c6672a4f5 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -479,6 +479,9 @@ void dw_pcie_version_detect(struct dw_pcie *pci);
u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap);

+u8 dwc_find_cap(struct dw_pcie *pcie, int cap);
+u16 dwc_find_ext_cap(struct dw_pcie *pcie, int cap);
+
int dw_pcie_read(void __iomem *addr, int size, u32 *val);
int dw_pcie_write(void __iomem *addr, int size, u32 val);

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 869d204a70a3..4376bee65cc6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -612,6 +612,84 @@ u16 pci_find_ext_capability(struct pci_dev *dev, int cap)
}
EXPORT_SYMBOL_GPL(pci_find_ext_capability);

+static u8 __pci_generic_find_next_cap(void *priv, pci_generic_read_cfg read_cfg,
+ u8 cap_ptr, int cap)
+{
+ u8 cap_id, next_cap_ptr;
+ u16 reg;
+
+ if (!cap_ptr)
+ return 0;
+
+ reg = read_cfg(priv, cap_ptr, 2);
+ cap_id = (reg & 0x00ff);
+
+ if (cap_id > PCI_CAP_ID_MAX)
+ return 0;
+
+ if (cap_id == cap)
+ return cap_ptr;
+
+ next_cap_ptr = (reg & 0xff00) >> 8;
+ return __pci_generic_find_next_cap(priv, read_cfg, next_cap_ptr, cap);
+}
+
+u8 pci_generic_find_capability(void *priv, pci_generic_read_cfg read_cfg,
+ int cap)
+{
+ u8 next_cap_ptr;
+ u16 reg;
+
+ reg = read_cfg(priv, PCI_CAPABILITY_LIST, 2);
+ next_cap_ptr = (reg & 0x00ff);
+
+ return __pci_generic_find_next_cap(priv, read_cfg, next_cap_ptr, cap);
+}
+EXPORT_SYMBOL_GPL(pci_generic_find_capability);
+
+static u16 pci_generic_find_next_ext_capability(void *priv,
+ pci_generic_read_cfg read_cfg,
+ 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 = read_cfg(priv, pos, 4);
+ /*
+ * 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 = read_cfg(priv, pos, 4);
+ }
+
+ return 0;
+}
+
+u16 pci_generic_find_ext_capability(void *priv, pci_generic_read_cfg read_cfg,
+ int cap)
+{
+ return pci_generic_find_next_ext_capability(priv, read_cfg, 0, cap);
+}
+EXPORT_SYMBOL_GPL(pci_generic_find_ext_capability);
+
/**
* pci_get_dsn - Read and return the 8-byte Device Serial Number
* @dev: PCI device to query
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 47b31ad724fa..63744f3de3a4 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1205,6 +1205,11 @@ u8 pci_find_ht_capability(struct pci_dev *dev, int ht_cap);
u8 pci_find_next_ht_capability(struct pci_dev *dev, u8 pos, int ht_cap);
u16 pci_find_ext_capability(struct pci_dev *dev, int cap);
u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 pos, int cap);
+typedef u32 (*pci_generic_read_cfg)(void *priv, int where, int size);
+u8 pci_generic_find_capability(void *priv, pci_generic_read_cfg read_cfg,
+ int cap);
+u16 pci_generic_find_ext_capability(void *priv, pci_generic_read_cfg read_cfg,
+ int cap);
struct pci_bus *pci_find_next_bus(const struct pci_bus *from);
u16 pci_find_vsec_capability(struct pci_dev *dev, u16 vendor, int cap);
u16 pci_find_dvsec_capability(struct pci_dev *dev, u16 vendor, u16 dvsec);
@@ -2012,7 +2017,13 @@ static inline u8 pci_find_next_capability(struct pci_dev *dev, u8 post, int cap)
{ return 0; }
static inline u16 pci_find_ext_capability(struct pci_dev *dev, int cap)
{ return 0; }
-
+typedef u32 (*pci_generic_read_cfg)(void *priv, int where, int size);
+u8 pci_generic_find_capability(void *priv, pci_generic_read_cfg read_cfg,
+ int cap)
+{ return 0; }
+u16 pci_generic_find_ext_capability(void *priv, pci_generic_read_cfg read_cfg,
+ int cap)
+{ return 0; }
static inline u64 pci_get_dsn(struct pci_dev *dev)
{ return 0; }

Best regards,
Hans

Next message: Josh Poimboeuf: "Re: [PATCH 04/20] x86/cpu: Use named asm operands in clflushopt()"
Previous message: Josh Poimboeuf: "Re: [PATCH 14/20] x86/barrier: Use alternative_io() in 32-bit barrier functions"
In reply to: Hans Zhang: "Re: [v2] PCI: cadence: Add configuration space capability search API"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]