[v6 0/5] Introduce generic capability search functions

[v6 0/5] Introduce generic capability search functions

[Hans Zhang]

1. Introduce generic capability search functions.
2. dwc/cdns use common PCI host bridge APIs for finding the capabilities.
3. Use cdns_pcie_find_*capability to avoid hardcode.
4. Add new patch for MAINTAINERS.

Changes since v5:
- If you put the helpers in drivers/pci/pci.c, they unnecessarily enlarge
  the kernel's .text section even if it's known already at compile time
  that they're never going to be used (e.g. on x86).
- Move the API for find capabilitys to a new file called
  pci-host-helpers.c.
- Add new patch for MAINTAINERS.

Changes since v4:
- Resolved [v4 1/4] compilation warning.
- The patch subject and commit message were modified.

Changes since v3:
- Resolved [v3 1/4] compilation error.
- Other patches are not modified.

Changes since v2:
- Add and split into a series of patches.

Hans Zhang (5):
  PCI: Introduce generic capability search functions
  PCI: dwc: Use common PCI host bridge APIs for finding the capabilities
  PCI: cadence: Use common PCI host bridge APIs for finding the
    capabilities
  PCI: cadence: Use cdns_pcie_find_*capability to avoid hardcode.
  MAINTAINERS: Add entry for PCI host controller helpers

 MAINTAINERS                                   |  6 ++
 drivers/pci/controller/Kconfig                | 17 ++++
 drivers/pci/controller/Makefile               |  1 +
 drivers/pci/controller/cadence/Kconfig        |  1 +
 .../pci/controller/cadence/pcie-cadence-ep.c  | 40 ++++----
 drivers/pci/controller/cadence/pcie-cadence.c | 25 +++++
 drivers/pci/controller/cadence/pcie-cadence.h |  8 +-
 drivers/pci/controller/dwc/Kconfig            |  1 +
 drivers/pci/controller/dwc/pcie-designware.c  | 71 +-------------
 drivers/pci/controller/pci-host-helpers.c     | 98 +++++++++++++++++++
 drivers/pci/pci.h                             |  7 ++
 11 files changed, 187 insertions(+), 88 deletions(-)
 create mode 100644 drivers/pci/controller/pci-host-helpers.c


base-commit: a1cffe8cc8aef85f1b07c4464f0998b9785b795a
-- 
2.25.1

[v6 1/5] PCI: Introduce generic capability search functions

[Hans Zhang]

Existing controller drivers (e.g., DWC, custom out-of-tree drivers)
duplicate logic for scanning PCI capability lists. This creates
maintenance burdens and risks inconsistencies.

To resolve this:

Add pci_host_bridge_find_*capability() in pci-host-helpers.c, accepting
controller-specific read functions and device data as parameters.

This approach:
- Centralizes critical PCI capability scanning logic
- Allows flexible adaptation to varied hardware access methods
- Reduces future maintenance overhead
- Aligns with kernel code reuse best practices

Signed-off-by: Hans Zhang <18255117159@163.com>
---
Changes since v5:
https://lore.kernel.org/linux-pci/20250321163803.391056-2-18255117159@163.com

- If you put the helpers in drivers/pci/pci.c, they unnecessarily enlarge
  the kernel's .text section even if it's known already at compile time
  that they're never going to be used (e.g. on x86).

- Move the API for find capabilitys to a new file called
  pci-host-helpers.c.

Changes since v4:
https://lore.kernel.org/linux-pci/20250321101710.371480-2-18255117159@163.com

- Resolved [v4 1/4] compilation warning.
- The patch commit message were modified.
---
 drivers/pci/controller/Kconfig            | 17 ++++
 drivers/pci/controller/Makefile           |  1 +
 drivers/pci/controller/pci-host-helpers.c | 98 +++++++++++++++++++++++
 drivers/pci/pci.h                         |  7 ++
 4 files changed, 123 insertions(+)
 create mode 100644 drivers/pci/controller/pci-host-helpers.c

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 9800b7681054..0020a892a55b 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -132,6 +132,23 @@ config PCI_HOST_GENERIC
 	  Say Y here if you want to support a simple generic PCI host
 	  controller, such as the one emulated by kvmtool.
 
+config PCI_HOST_HELPERS
+	bool
+	prompt "PCI Host Controller Helper Functions" if EXPERT
+ 	help
+	  This provides common infrastructure for PCI host controller drivers to
+	  handle PCI capability scanning and other shared operations. The helper
+	  functions eliminate code duplication across controller drivers.
+
+	  These functions are used by PCI controller drivers that need to scan
+	  PCI capabilities using controller-specific access methods (e.g. when
+	  the controller is behind a non-standard configuration space).
+
+	  If you are using any PCI host controller drivers that require these
+	  helpers (such as DesignWare, Cadence, etc), this will be
+	  automatically selected. Say N unless you are developing a custom PCI
+	  host controller driver.
+
 config PCIE_HISI_ERR
 	depends on ACPI_APEI_GHES && (ARM64 || COMPILE_TEST)
 	bool "HiSilicon HIP PCIe controller error handling driver"
diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
index 038ccbd9e3ba..e80091eb7597 100644
--- a/drivers/pci/controller/Makefile
+++ b/drivers/pci/controller/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_PCIE_RCAR_HOST) += pcie-rcar.o pcie-rcar-host.o
 obj-$(CONFIG_PCIE_RCAR_EP) += pcie-rcar.o pcie-rcar-ep.o
 obj-$(CONFIG_PCI_HOST_COMMON) += pci-host-common.o
 obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
+obj-$(CONFIG_PCI_HOST_HELPERS) += pci-host-helpers.o
 obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
 obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
 obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
diff --git a/drivers/pci/controller/pci-host-helpers.c b/drivers/pci/controller/pci-host-helpers.c
new file mode 100644
index 000000000000..cd261a281c60
--- /dev/null
+++ b/drivers/pci/controller/pci-host-helpers.c
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCI Host Controller Helper Functions
+ *
+ * Copyright (C) 2025 Hans Zhang
+ *
+ * Author: Hans Zhang <18255117159@163.com>
+ */
+
+#include <linux/pci.h>
+
+#include "../pci.h"
+
+/*
+ * 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 __pci_host_bridge_find_next_cap(void *priv,
+					  pci_host_bridge_read_cfg read_cfg,
+					  u8 cap_ptr, u8 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_host_bridge_find_next_cap(priv, read_cfg, next_cap_ptr,
+					       cap);
+}
+
+u8 pci_host_bridge_find_capability(void *priv,
+				   pci_host_bridge_read_cfg read_cfg, u8 cap)
+{
+	u8 next_cap_ptr;
+	u16 reg;
+
+	reg = read_cfg(priv, PCI_CAPABILITY_LIST, 2);
+	next_cap_ptr = (reg & 0x00ff);
+
+	return __pci_host_bridge_find_next_cap(priv, read_cfg, next_cap_ptr,
+					       cap);
+}
+EXPORT_SYMBOL_GPL(pci_host_bridge_find_capability);
+
+static u16 pci_host_bridge_find_next_ext_capability(
+	void *priv, pci_host_bridge_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_host_bridge_find_ext_capability(void *priv,
+					pci_host_bridge_read_cfg read_cfg,
+					u8 cap)
+{
+	return pci_host_bridge_find_next_ext_capability(priv, read_cfg, 0, cap);
+}
+EXPORT_SYMBOL_GPL(pci_host_bridge_find_ext_capability);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 01e51db8d285..8d1c919cbfef 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -1034,4 +1034,11 @@ void pcim_release_region(struct pci_dev *pdev, int bar);
 	(PCI_CONF1_ADDRESS(bus, dev, func, reg) | \
 	 PCI_CONF1_EXT_REG(reg))
 
+typedef u32 (*pci_host_bridge_read_cfg)(void *priv, int where, int size);
+u8 pci_host_bridge_find_capability(void *priv,
+				   pci_host_bridge_read_cfg read_cfg, u8 cap);
+u16 pci_host_bridge_find_ext_capability(void *priv,
+					pci_host_bridge_read_cfg read_cfg,
+					u8 cap);
+
 #endif /* DRIVERS_PCI_H */
-- 
2.25.1

Re: [v6 1/5] PCI: Introduce generic capability search functions

[Ilpo Järvinen]

On Mon, 24 Mar 2025, Hans Zhang wrote:

> Existing controller drivers (e.g., DWC, custom out-of-tree drivers)
> duplicate logic for scanning PCI capability lists. This creates
> maintenance burdens and risks inconsistencies.
> 
> To resolve this:
> 
> Add pci_host_bridge_find_*capability() in pci-host-helpers.c, accepting
> controller-specific read functions and device data as parameters.
> 
> This approach:
> - Centralizes critical PCI capability scanning logic
> - Allows flexible adaptation to varied hardware access methods
> - Reduces future maintenance overhead
> - Aligns with kernel code reuse best practices
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
> Changes since v5:
> https://lore.kernel.org/linux-pci/20250321163803.391056-2-18255117159@163.com
> 
> - If you put the helpers in drivers/pci/pci.c, they unnecessarily enlarge
>   the kernel's .text section even if it's known already at compile time
>   that they're never going to be used (e.g. on x86).
> 
> - Move the API for find capabilitys to a new file called
>   pci-host-helpers.c.
> 
> Changes since v4:
> https://lore.kernel.org/linux-pci/20250321101710.371480-2-18255117159@163.com
> 
> - Resolved [v4 1/4] compilation warning.
> - The patch commit message were modified.
> ---
>  drivers/pci/controller/Kconfig            | 17 ++++
>  drivers/pci/controller/Makefile           |  1 +
>  drivers/pci/controller/pci-host-helpers.c | 98 +++++++++++++++++++++++
>  drivers/pci/pci.h                         |  7 ++
>  4 files changed, 123 insertions(+)
>  create mode 100644 drivers/pci/controller/pci-host-helpers.c
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 9800b7681054..0020a892a55b 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -132,6 +132,23 @@ config PCI_HOST_GENERIC
>  	  Say Y here if you want to support a simple generic PCI host
>  	  controller, such as the one emulated by kvmtool.
>  
> +config PCI_HOST_HELPERS
> +	bool
> +	prompt "PCI Host Controller Helper Functions" if EXPERT
> + 	help
> +	  This provides common infrastructure for PCI host controller drivers to
> +	  handle PCI capability scanning and other shared operations. The helper
> +	  functions eliminate code duplication across controller drivers.
> +
> +	  These functions are used by PCI controller drivers that need to scan
> +	  PCI capabilities using controller-specific access methods (e.g. when
> +	  the controller is behind a non-standard configuration space).
> +
> +	  If you are using any PCI host controller drivers that require these
> +	  helpers (such as DesignWare, Cadence, etc), this will be
> +	  automatically selected. Say N unless you are developing a custom PCI
> +	  host controller driver.

Hi,

Does this need to be user selectable at all? What's the benefit? If 
somebody is developing a driver, they can just as well add the select 
clause in that driver to get it built.

--
 i.

> +
>  config PCIE_HISI_ERR
>  	depends on ACPI_APEI_GHES && (ARM64 || COMPILE_TEST)
>  	bool "HiSilicon HIP PCIe controller error handling driver"
> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
> index 038ccbd9e3ba..e80091eb7597 100644
> --- a/drivers/pci/controller/Makefile
> +++ b/drivers/pci/controller/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_PCIE_RCAR_HOST) += pcie-rcar.o pcie-rcar-host.o
>  obj-$(CONFIG_PCIE_RCAR_EP) += pcie-rcar.o pcie-rcar-ep.o
>  obj-$(CONFIG_PCI_HOST_COMMON) += pci-host-common.o
>  obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
> +obj-$(CONFIG_PCI_HOST_HELPERS) += pci-host-helpers.o
>  obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
>  obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
> diff --git a/drivers/pci/controller/pci-host-helpers.c b/drivers/pci/controller/pci-host-helpers.c
> new file mode 100644
> index 000000000000..cd261a281c60
> --- /dev/null
> +++ b/drivers/pci/controller/pci-host-helpers.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCI Host Controller Helper Functions
> + *
> + * Copyright (C) 2025 Hans Zhang
> + *
> + * Author: Hans Zhang <18255117159@163.com>
> + */
> +
> +#include <linux/pci.h>
> +
> +#include "../pci.h"
> +
> +/*
> + * 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 __pci_host_bridge_find_next_cap(void *priv,
> +					  pci_host_bridge_read_cfg read_cfg,
> +					  u8 cap_ptr, u8 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_host_bridge_find_next_cap(priv, read_cfg, next_cap_ptr,
> +					       cap);

This is doing (tail) recursion?? Why??

What should be done, IMO, is that code in __pci_find_next_cap_ttl() 
refactored such that it can be reused instead of duplicating it in a 
slightly different form here and the functions below.

The capability list parser should be the same?

> +}
> +
> +u8 pci_host_bridge_find_capability(void *priv,
> +				   pci_host_bridge_read_cfg read_cfg, u8 cap)
> +{
> +	u8 next_cap_ptr;
> +	u16 reg;
> +
> +	reg = read_cfg(priv, PCI_CAPABILITY_LIST, 2);
> +	next_cap_ptr = (reg & 0x00ff);
> +
> +	return __pci_host_bridge_find_next_cap(priv, read_cfg, next_cap_ptr,
> +					       cap);
> +}
> +EXPORT_SYMBOL_GPL(pci_host_bridge_find_capability);
> +
> +static u16 pci_host_bridge_find_next_ext_capability(
> +	void *priv, pci_host_bridge_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_host_bridge_find_ext_capability(void *priv,
> +					pci_host_bridge_read_cfg read_cfg,
> +					u8 cap)
> +{
> +	return pci_host_bridge_find_next_ext_capability(priv, read_cfg, 0, cap);
> +}
> +EXPORT_SYMBOL_GPL(pci_host_bridge_find_ext_capability);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 01e51db8d285..8d1c919cbfef 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -1034,4 +1034,11 @@ void pcim_release_region(struct pci_dev *pdev, int bar);
>  	(PCI_CONF1_ADDRESS(bus, dev, func, reg) | \
>  	 PCI_CONF1_EXT_REG(reg))
>  
> +typedef u32 (*pci_host_bridge_read_cfg)(void *priv, int where, int size);
> +u8 pci_host_bridge_find_capability(void *priv,
> +				   pci_host_bridge_read_cfg read_cfg, u8 cap);
> +u16 pci_host_bridge_find_ext_capability(void *priv,
> +					pci_host_bridge_read_cfg read_cfg,
> +					u8 cap);
> +
>  #endif /* DRIVERS_PCI_H */
> 

-- 
 i.

Re: [v6 1/5] PCI: Introduce generic capability search functions

[Hans Zhang]

On 2025/3/24 21:28, Ilpo Järvinen wrote:
> On Mon, 24 Mar 2025, Hans Zhang wrote:
> 
>> Existing controller drivers (e.g., DWC, custom out-of-tree drivers)
>> duplicate logic for scanning PCI capability lists. This creates
>> maintenance burdens and risks inconsistencies.
>>
>> To resolve this:
>>
>> Add pci_host_bridge_find_*capability() in pci-host-helpers.c, accepting
>> controller-specific read functions and device data as parameters.
>>
>> This approach:
>> - Centralizes critical PCI capability scanning logic
>> - Allows flexible adaptation to varied hardware access methods
>> - Reduces future maintenance overhead
>> - Aligns with kernel code reuse best practices
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>> Changes since v5:
>> https://lore.kernel.org/linux-pci/20250321163803.391056-2-18255117159@163.com
>>
>> - If you put the helpers in drivers/pci/pci.c, they unnecessarily enlarge
>>    the kernel's .text section even if it's known already at compile time
>>    that they're never going to be used (e.g. on x86).
>>
>> - Move the API for find capabilitys to a new file called
>>    pci-host-helpers.c.
>>
>> Changes since v4:
>> https://lore.kernel.org/linux-pci/20250321101710.371480-2-18255117159@163.com
>>
>> - Resolved [v4 1/4] compilation warning.
>> - The patch commit message were modified.
>> ---
>>   drivers/pci/controller/Kconfig            | 17 ++++
>>   drivers/pci/controller/Makefile           |  1 +
>>   drivers/pci/controller/pci-host-helpers.c | 98 +++++++++++++++++++++++
>>   drivers/pci/pci.h                         |  7 ++
>>   4 files changed, 123 insertions(+)
>>   create mode 100644 drivers/pci/controller/pci-host-helpers.c
>>
>> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
>> index 9800b7681054..0020a892a55b 100644
>> --- a/drivers/pci/controller/Kconfig
>> +++ b/drivers/pci/controller/Kconfig
>> @@ -132,6 +132,23 @@ config PCI_HOST_GENERIC
>>   	  Say Y here if you want to support a simple generic PCI host
>>   	  controller, such as the one emulated by kvmtool.
>>   
>> +config PCI_HOST_HELPERS
>> +	bool
>> +	prompt "PCI Host Controller Helper Functions" if EXPERT
>> + 	help
>> +	  This provides common infrastructure for PCI host controller drivers to
>> +	  handle PCI capability scanning and other shared operations. The helper
>> +	  functions eliminate code duplication across controller drivers.
>> +
>> +	  These functions are used by PCI controller drivers that need to scan
>> +	  PCI capabilities using controller-specific access methods (e.g. when
>> +	  the controller is behind a non-standard configuration space).
>> +
>> +	  If you are using any PCI host controller drivers that require these
>> +	  helpers (such as DesignWare, Cadence, etc), this will be
>> +	  automatically selected. Say N unless you are developing a custom PCI
>> +	  host controller driver.
> 
> Hi,
> 
> Does this need to be user selectable at all? What's the benefit? If
> somebody is developing a driver, they can just as well add the select
> clause in that driver to get it built.
> 

Dear Ilpo,

Thanks your for reply. Only DWC and CDNS drivers are used here, what do 
you suggest should be done?


> 
>> +
>>   config PCIE_HISI_ERR
>>   	depends on ACPI_APEI_GHES && (ARM64 || COMPILE_TEST)
>>   	bool "HiSilicon HIP PCIe controller error handling driver"
>> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
>> index 038ccbd9e3ba..e80091eb7597 100644
>> --- a/drivers/pci/controller/Makefile
>> +++ b/drivers/pci/controller/Makefile
>> @@ -12,6 +12,7 @@ obj-$(CONFIG_PCIE_RCAR_HOST) += pcie-rcar.o pcie-rcar-host.o
>>   obj-$(CONFIG_PCIE_RCAR_EP) += pcie-rcar.o pcie-rcar-ep.o
>>   obj-$(CONFIG_PCI_HOST_COMMON) += pci-host-common.o
>>   obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
>> +obj-$(CONFIG_PCI_HOST_HELPERS) += pci-host-helpers.o
>>   obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
>>   obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
>>   obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
>> diff --git a/drivers/pci/controller/pci-host-helpers.c b/drivers/pci/controller/pci-host-helpers.c
>> new file mode 100644
>> index 000000000000..cd261a281c60
>> --- /dev/null
>> +++ b/drivers/pci/controller/pci-host-helpers.c
>> @@ -0,0 +1,98 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * PCI Host Controller Helper Functions
>> + *
>> + * Copyright (C) 2025 Hans Zhang
>> + *
>> + * Author: Hans Zhang <18255117159@163.com>
>> + */
>> +
>> +#include <linux/pci.h>
>> +
>> +#include "../pci.h"
>> +
>> +/*
>> + * 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 __pci_host_bridge_find_next_cap(void *priv,
>> +					  pci_host_bridge_read_cfg read_cfg,
>> +					  u8 cap_ptr, u8 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_host_bridge_find_next_cap(priv, read_cfg, next_cap_ptr,
>> +					       cap);
> 
> This is doing (tail) recursion?? Why??
> 
> What should be done, IMO, is that code in __pci_find_next_cap_ttl()
> refactored such that it can be reused instead of duplicating it in a
> slightly different form here and the functions below.
> 
> The capability list parser should be the same?
> 

The original function is in the following file:
drivers/pci/controller/dwc/pcie-designware.c
u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)

CDNS has the same need to find the offset of the capability.

We don't have pci_dev before calling pci_host_probe, but we want to get 
the offset of the capability and configure some registers to initialize 
the root port. Therefore, the __pci_find_next_cap_ttl function cannot be 
used. This is also the purpose of dw_pcie_find_*capability.


The CDNS driver does not have a cdns_pcie_find_*capability function. 
Therefore, separate the find capability, and then DWC and CDNS can be 
used at the same time to reduce duplicate code.


Communication history:

Bjorn HelgaasMarch 14, 2025, 8:31 p.m. UTC | #8
On Fri, Mar 14, 2025 at 06:35:11PM +0530, Manivannan Sadhasivam wrote:
 > ...

 > 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.

I agree, it would be really nice to share this code.

It looks a little messy to deal with passing around pointers to
controller read ops, and we'll still end up with a lot of duplicated
code between __pci_find_next_cap() and __cdns_pcie_find_next_cap(),
etc.

Maybe someday we'll make a generic way to access non-PCI "config"
space like this host controller space and PCIe RCRBs.

Or if you add interfaces that accept read/write ops, maybe the
existing pci_find_capability() etc could be refactored on top of them
by passing in pci_bus_read_config_word() as the accessor.


>> +}
>> +
>> +u8 pci_host_bridge_find_capability(void *priv,
>> +				   pci_host_bridge_read_cfg read_cfg, u8 cap)
>> +{
>> +	u8 next_cap_ptr;
>> +	u16 reg;
>> +
>> +	reg = read_cfg(priv, PCI_CAPABILITY_LIST, 2);
>> +	next_cap_ptr = (reg & 0x00ff);
>> +
>> +	return __pci_host_bridge_find_next_cap(priv, read_cfg, next_cap_ptr,
>> +					       cap);
>> +}
>> +EXPORT_SYMBOL_GPL(pci_host_bridge_find_capability);


Best regards,
Hans

Re: [v6 1/5] PCI: Introduce generic capability search functions

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 9561 bytes --]

On Mon, 24 Mar 2025, Hans Zhang wrote:
> On 2025/3/24 21:28, Ilpo Järvinen wrote:
> > On Mon, 24 Mar 2025, Hans Zhang wrote:
> > 
> > > Existing controller drivers (e.g., DWC, custom out-of-tree drivers)
> > > duplicate logic for scanning PCI capability lists. This creates
> > > maintenance burdens and risks inconsistencies.
> > > 
> > > To resolve this:
> > > 
> > > Add pci_host_bridge_find_*capability() in pci-host-helpers.c, accepting
> > > controller-specific read functions and device data as parameters.
> > > 
> > > This approach:
> > > - Centralizes critical PCI capability scanning logic
> > > - Allows flexible adaptation to varied hardware access methods
> > > - Reduces future maintenance overhead
> > > - Aligns with kernel code reuse best practices
> > > 
> > > Signed-off-by: Hans Zhang <18255117159@163.com>
> > > ---
> > > Changes since v5:
> > > https://lore.kernel.org/linux-pci/20250321163803.391056-2-18255117159@163.com
> > > 
> > > - If you put the helpers in drivers/pci/pci.c, they unnecessarily enlarge
> > >    the kernel's .text section even if it's known already at compile time
> > >    that they're never going to be used (e.g. on x86).
> > > 
> > > - Move the API for find capabilitys to a new file called
> > >    pci-host-helpers.c.
> > > 
> > > Changes since v4:
> > > https://lore.kernel.org/linux-pci/20250321101710.371480-2-18255117159@163.com
> > > 
> > > - Resolved [v4 1/4] compilation warning.
> > > - The patch commit message were modified.
> > > ---
> > >   drivers/pci/controller/Kconfig            | 17 ++++
> > >   drivers/pci/controller/Makefile           |  1 +
> > >   drivers/pci/controller/pci-host-helpers.c | 98 +++++++++++++++++++++++
> > >   drivers/pci/pci.h                         |  7 ++
> > >   4 files changed, 123 insertions(+)
> > >   create mode 100644 drivers/pci/controller/pci-host-helpers.c
> > > 
> > > diff --git a/drivers/pci/controller/Kconfig
> > > b/drivers/pci/controller/Kconfig
> > > index 9800b7681054..0020a892a55b 100644
> > > --- a/drivers/pci/controller/Kconfig
> > > +++ b/drivers/pci/controller/Kconfig
> > > @@ -132,6 +132,23 @@ config PCI_HOST_GENERIC
> > >   	  Say Y here if you want to support a simple generic PCI host
> > >   	  controller, such as the one emulated by kvmtool.
> > >   +config PCI_HOST_HELPERS
> > > +	bool
> > > +	prompt "PCI Host Controller Helper Functions" if EXPERT
> > > + 	help
> > > +	  This provides common infrastructure for PCI host controller drivers
> > > to
> > > +	  handle PCI capability scanning and other shared operations. The
> > > helper
> > > +	  functions eliminate code duplication across controller drivers.
> > > +
> > > +	  These functions are used by PCI controller drivers that need to scan
> > > +	  PCI capabilities using controller-specific access methods (e.g. when
> > > +	  the controller is behind a non-standard configuration space).
> > > +
> > > +	  If you are using any PCI host controller drivers that require these
> > > +	  helpers (such as DesignWare, Cadence, etc), this will be
> > > +	  automatically selected. Say N unless you are developing a custom PCI
> > > +	  host controller driver.
> > 
> > Hi,
> > 
> > Does this need to be user selectable at all? What's the benefit? If
> > somebody is developing a driver, they can just as well add the select
> > clause in that driver to get it built.
> > 
> 
> Dear Ilpo,
> 
> Thanks your for reply. Only DWC and CDNS drivers are used here, what do you
> suggest should be done?

Just make it only Kconfig select'able and not user selectable at all.

> > > +
> > >   config PCIE_HISI_ERR
> > >   	depends on ACPI_APEI_GHES && (ARM64 || COMPILE_TEST)
> > >   	bool "HiSilicon HIP PCIe controller error handling driver"
> > > diff --git a/drivers/pci/controller/Makefile
> > > b/drivers/pci/controller/Makefile
> > > index 038ccbd9e3ba..e80091eb7597 100644
> > > --- a/drivers/pci/controller/Makefile
> > > +++ b/drivers/pci/controller/Makefile
> > > @@ -12,6 +12,7 @@ obj-$(CONFIG_PCIE_RCAR_HOST) += pcie-rcar.o
> > > pcie-rcar-host.o
> > >   obj-$(CONFIG_PCIE_RCAR_EP) += pcie-rcar.o pcie-rcar-ep.o
> > >   obj-$(CONFIG_PCI_HOST_COMMON) += pci-host-common.o
> > >   obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
> > > +obj-$(CONFIG_PCI_HOST_HELPERS) += pci-host-helpers.o
> > >   obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
> > >   obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
> > >   obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
> > > diff --git a/drivers/pci/controller/pci-host-helpers.c
> > > b/drivers/pci/controller/pci-host-helpers.c
> > > new file mode 100644
> > > index 000000000000..cd261a281c60
> > > --- /dev/null
> > > +++ b/drivers/pci/controller/pci-host-helpers.c
> > > @@ -0,0 +1,98 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * PCI Host Controller Helper Functions
> > > + *
> > > + * Copyright (C) 2025 Hans Zhang
> > > + *
> > > + * Author: Hans Zhang <18255117159@163.com>
> > > + */
> > > +
> > > +#include <linux/pci.h>
> > > +
> > > +#include "../pci.h"
> > > +
> > > +/*
> > > + * 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 __pci_host_bridge_find_next_cap(void *priv,
> > > +					  pci_host_bridge_read_cfg read_cfg,
> > > +					  u8 cap_ptr, u8 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_host_bridge_find_next_cap(priv, read_cfg, next_cap_ptr,
> > > +					       cap);
> > 
> > This is doing (tail) recursion?? Why??
> > 
> > What should be done, IMO, is that code in __pci_find_next_cap_ttl()
> > refactored such that it can be reused instead of duplicating it in a
> > slightly different form here and the functions below.
> > 
> > The capability list parser should be the same?
> > 
> 
> The original function is in the following file:
> drivers/pci/controller/dwc/pcie-designware.c
> u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
> u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
> 
> CDNS has the same need to find the offset of the capability.
> 
> We don't have pci_dev before calling pci_host_probe, but we want to get the
> offset of the capability and configure some registers to initialize the root
> port. Therefore, the __pci_find_next_cap_ttl function cannot be used. This is
> also the purpose of dw_pcie_find_*capability.

__pci_find_next_cap_ttl() does not take pci_dev so I'm unsure if the 
problem is real or not?!?

> The CDNS driver does not have a cdns_pcie_find_*capability function.
> Therefore, separate the find capability, and then DWC and CDNS can be used at
> the same time to reduce duplicate code.
> 
> 
> Communication history:
> 
> Bjorn HelgaasMarch 14, 2025, 8:31 p.m. UTC | #8
> On Fri, Mar 14, 2025 at 06:35:11PM +0530, Manivannan Sadhasivam wrote:
> > ...
> 
> > 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.
> 
> I agree, it would be really nice to share this code.
> 
> It looks a little messy to deal with passing around pointers to
> controller read ops, and we'll still end up with a lot of duplicated
> code between __pci_find_next_cap() and __cdns_pcie_find_next_cap(),
> etc.
> 
> Maybe someday we'll make a generic way to access non-PCI "config"
> space like this host controller space and PCIe RCRBs.
> 
> Or if you add interfaces that accept read/write ops, maybe the
> existing pci_find_capability() etc could be refactored on top of them
> by passing in pci_bus_read_config_word() as the accessor.

At minimum, the loop in __pci_find_next_cap_ttl() could be turned into a 
macro similar to eg. read_poll_timeout() that takes the read function as 
an argument (read_poll_timeout() looks messy because it doesn't align 
backslashed to far right). That would avoid duplicating the parsing logic
on C code level.

> > > +}
> > > +
> > > +u8 pci_host_bridge_find_capability(void *priv,
> > > +				   pci_host_bridge_read_cfg read_cfg, u8 cap)
> > > +{
> > > +	u8 next_cap_ptr;
> > > +	u16 reg;
> > > +
> > > +	reg = read_cfg(priv, PCI_CAPABILITY_LIST, 2);
> > > +	next_cap_ptr = (reg & 0x00ff);
> > > +
> > > +	return __pci_host_bridge_find_next_cap(priv, read_cfg, next_cap_ptr,
> > > +					       cap);
> > > +}
> > > +EXPORT_SYMBOL_GPL(pci_host_bridge_find_capability);
> 
> 
> Best regards,
> Hans
> 

-- 
 i.

Re: [v6 1/5] PCI: Introduce generic capability search functions

[Hans Zhang]

On 2025/3/24 22:52, Ilpo Järvinen wrote:
>>>> --- a/drivers/pci/controller/Kconfig
>>>> +++ b/drivers/pci/controller/Kconfig
>>>> @@ -132,6 +132,23 @@ config PCI_HOST_GENERIC
>>>>    	  Say Y here if you want to support a simple generic PCI host
>>>>    	  controller, such as the one emulated by kvmtool.
>>>>    +config PCI_HOST_HELPERS
>>>> +	bool
>>>> +	prompt "PCI Host Controller Helper Functions" if EXPERT
>>>> + 	help
>>>> +	  This provides common infrastructure for PCI host controller drivers
>>>> to
>>>> +	  handle PCI capability scanning and other shared operations. The
>>>> helper
>>>> +	  functions eliminate code duplication across controller drivers.
>>>> +
>>>> +	  These functions are used by PCI controller drivers that need to scan
>>>> +	  PCI capabilities using controller-specific access methods (e.g. when
>>>> +	  the controller is behind a non-standard configuration space).
>>>> +
>>>> +	  If you are using any PCI host controller drivers that require these
>>>> +	  helpers (such as DesignWare, Cadence, etc), this will be
>>>> +	  automatically selected. Say N unless you are developing a custom PCI
>>>> +	  host controller driver.
>>>
>>> Hi,
>>>
>>> Does this need to be user selectable at all? What's the benefit? If
>>> somebody is developing a driver, they can just as well add the select
>>> clause in that driver to get it built.
>>>
>>
>> Dear Ilpo,
>>
>> Thanks your for reply. Only DWC and CDNS drivers are used here, what do you
>> suggest should be done?
> 
> Just make it only Kconfig select'able and not user selectable at all.
> 

Hi Ilpo,

Thanks your for reply. Will change.

Will delete it.
prompt "PCI Host Controller Helper Functions" if EXPERT

>>>> + * 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 __pci_host_bridge_find_next_cap(void *priv,
>>>> +					  pci_host_bridge_read_cfg read_cfg,
>>>> +					  u8 cap_ptr, u8 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_host_bridge_find_next_cap(priv, read_cfg, next_cap_ptr,
>>>> +					       cap);
>>>
>>> This is doing (tail) recursion?? Why??
>>>
>>> What should be done, IMO, is that code in __pci_find_next_cap_ttl()
>>> refactored such that it can be reused instead of duplicating it in a
>>> slightly different form here and the functions below.
>>>
>>> The capability list parser should be the same?
>>>
>>
>> The original function is in the following file:
>> drivers/pci/controller/dwc/pcie-designware.c
>> u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
>> u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
>>
>> CDNS has the same need to find the offset of the capability.
>>
>> We don't have pci_dev before calling pci_host_probe, but we want to get the
>> offset of the capability and configure some registers to initialize the root
>> port. Therefore, the __pci_find_next_cap_ttl function cannot be used. This is
>> also the purpose of dw_pcie_find_*capability.
> 
> __pci_find_next_cap_ttl() does not take pci_dev so I'm unsure if the
> problem is real or not?!?

__pci_find_next_cap_ttl uses pci_bus as the first argument, and other 
functions take pci_dev->bus as its first argument. Either way, either 
pci_bus or pci_dev is required, and before pcie enumeration, there was 
no pci_bus or pci_dev.

I replied to you in the patch email [v6 3/5], if I wasn't clear enough, 
please remind me and we'll discuss it again.

> 
>> The CDNS driver does not have a cdns_pcie_find_*capability function.
>> Therefore, separate the find capability, and then DWC and CDNS can be used at
>> the same time to reduce duplicate code.
>>
>>
>> Communication history:
>>
>> Bjorn HelgaasMarch 14, 2025, 8:31 p.m. UTC | #8
>> On Fri, Mar 14, 2025 at 06:35:11PM +0530, Manivannan Sadhasivam wrote:
>>> ...
>>
>>> 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.
>>
>> I agree, it would be really nice to share this code.
>>
>> It looks a little messy to deal with passing around pointers to
>> controller read ops, and we'll still end up with a lot of duplicated
>> code between __pci_find_next_cap() and __cdns_pcie_find_next_cap(),
>> etc.
>>
>> Maybe someday we'll make a generic way to access non-PCI "config"
>> space like this host controller space and PCIe RCRBs.
>>
>> Or if you add interfaces that accept read/write ops, maybe the
>> existing pci_find_capability() etc could be refactored on top of them
>> by passing in pci_bus_read_config_word() as the accessor.
> 
> At minimum, the loop in __pci_find_next_cap_ttl() could be turned into a
> macro similar to eg. read_poll_timeout() that takes the read function as
> an argument (read_poll_timeout() looks messy because it doesn't align
> backslashed to far right). That would avoid duplicating the parsing logic
> on C code level.
> 

The config space register cannot be read before PCIe enumeration. Only 
the read and write functions of the root port driver can be used.

Best regards,
Hans

Re: [v6 1/5] PCI: Introduce generic capability search functions

[Manivannan Sadhasivam]

On Mon, Mar 24, 2025 at 12:48:48AM +0800, Hans Zhang wrote:
> Existing controller drivers (e.g., DWC, custom out-of-tree drivers)
> duplicate logic for scanning PCI capability lists. This creates
> maintenance burdens and risks inconsistencies.
> 
> To resolve this:
> 
> Add pci_host_bridge_find_*capability() in pci-host-helpers.c, accepting
> controller-specific read functions and device data as parameters.
> 
> This approach:
> - Centralizes critical PCI capability scanning logic
> - Allows flexible adaptation to varied hardware access methods
> - Reduces future maintenance overhead
> - Aligns with kernel code reuse best practices
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
> Changes since v5:
> https://lore.kernel.org/linux-pci/20250321163803.391056-2-18255117159@163.com
> 
> - If you put the helpers in drivers/pci/pci.c, they unnecessarily enlarge
>   the kernel's .text section even if it's known already at compile time
>   that they're never going to be used (e.g. on x86).
> 
> - Move the API for find capabilitys to a new file called
>   pci-host-helpers.c.
> 
> Changes since v4:
> https://lore.kernel.org/linux-pci/20250321101710.371480-2-18255117159@163.com
> 
> - Resolved [v4 1/4] compilation warning.
> - The patch commit message were modified.
> ---
>  drivers/pci/controller/Kconfig            | 17 ++++
>  drivers/pci/controller/Makefile           |  1 +
>  drivers/pci/controller/pci-host-helpers.c | 98 +++++++++++++++++++++++
>  drivers/pci/pci.h                         |  7 ++
>  4 files changed, 123 insertions(+)
>  create mode 100644 drivers/pci/controller/pci-host-helpers.c
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 9800b7681054..0020a892a55b 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -132,6 +132,23 @@ config PCI_HOST_GENERIC
>  	  Say Y here if you want to support a simple generic PCI host
>  	  controller, such as the one emulated by kvmtool.
>  
> +config PCI_HOST_HELPERS
> +	bool
> +	prompt "PCI Host Controller Helper Functions" if EXPERT

User is not required to select this Kconfig option so that his driver can build.
Please make this symbol invisible to user and make it selected by the required
controller drivers only.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [v6 1/5] PCI: Introduce generic capability search functions

[Hans Zhang]

On 2025/3/28 00:57, Manivannan Sadhasivam wrote:
>> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
>> index 9800b7681054..0020a892a55b 100644
>> --- a/drivers/pci/controller/Kconfig
>> +++ b/drivers/pci/controller/Kconfig
>> @@ -132,6 +132,23 @@ config PCI_HOST_GENERIC
>>   	  Say Y here if you want to support a simple generic PCI host
>>   	  controller, such as the one emulated by kvmtool.
>>   
>> +config PCI_HOST_HELPERS
>> +	bool
>> +	prompt "PCI Host Controller Helper Functions" if EXPERT
> 
> User is not required to select this Kconfig option so that his driver can build.
> Please make this symbol invisible to user and make it selected by the required
> controller drivers only.
> 

Hi Mani,

Thanks your for reply. Will change.

Best regards,
Hans

Re: [v6 1/5] PCI: Introduce generic capability search functions

[Manivannan Sadhasivam]

On Mon, Mar 24, 2025 at 12:48:48AM +0800, Hans Zhang wrote:
> Existing controller drivers (e.g., DWC, custom out-of-tree drivers)

Oh, you should not mention 'out-of-tree drivers' as this patch is not anyway
intented to benefit them. We certainly do not care about out of tree drivers.

- Mani

> duplicate logic for scanning PCI capability lists. This creates
> maintenance burdens and risks inconsistencies.
> 
> To resolve this:
> 
> Add pci_host_bridge_find_*capability() in pci-host-helpers.c, accepting
> controller-specific read functions and device data as parameters.
> 
> This approach:
> - Centralizes critical PCI capability scanning logic
> - Allows flexible adaptation to varied hardware access methods
> - Reduces future maintenance overhead
> - Aligns with kernel code reuse best practices
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
> Changes since v5:
> https://lore.kernel.org/linux-pci/20250321163803.391056-2-18255117159@163.com
> 
> - If you put the helpers in drivers/pci/pci.c, they unnecessarily enlarge
>   the kernel's .text section even if it's known already at compile time
>   that they're never going to be used (e.g. on x86).
> 
> - Move the API for find capabilitys to a new file called
>   pci-host-helpers.c.
> 
> Changes since v4:
> https://lore.kernel.org/linux-pci/20250321101710.371480-2-18255117159@163.com
> 
> - Resolved [v4 1/4] compilation warning.
> - The patch commit message were modified.
> ---
>  drivers/pci/controller/Kconfig            | 17 ++++
>  drivers/pci/controller/Makefile           |  1 +
>  drivers/pci/controller/pci-host-helpers.c | 98 +++++++++++++++++++++++
>  drivers/pci/pci.h                         |  7 ++
>  4 files changed, 123 insertions(+)
>  create mode 100644 drivers/pci/controller/pci-host-helpers.c
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 9800b7681054..0020a892a55b 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -132,6 +132,23 @@ config PCI_HOST_GENERIC
>  	  Say Y here if you want to support a simple generic PCI host
>  	  controller, such as the one emulated by kvmtool.
>  
> +config PCI_HOST_HELPERS
> +	bool
> +	prompt "PCI Host Controller Helper Functions" if EXPERT
> + 	help
> +	  This provides common infrastructure for PCI host controller drivers to
> +	  handle PCI capability scanning and other shared operations. The helper
> +	  functions eliminate code duplication across controller drivers.
> +
> +	  These functions are used by PCI controller drivers that need to scan
> +	  PCI capabilities using controller-specific access methods (e.g. when
> +	  the controller is behind a non-standard configuration space).
> +
> +	  If you are using any PCI host controller drivers that require these
> +	  helpers (such as DesignWare, Cadence, etc), this will be
> +	  automatically selected. Say N unless you are developing a custom PCI
> +	  host controller driver.
> +
>  config PCIE_HISI_ERR
>  	depends on ACPI_APEI_GHES && (ARM64 || COMPILE_TEST)
>  	bool "HiSilicon HIP PCIe controller error handling driver"
> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
> index 038ccbd9e3ba..e80091eb7597 100644
> --- a/drivers/pci/controller/Makefile
> +++ b/drivers/pci/controller/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_PCIE_RCAR_HOST) += pcie-rcar.o pcie-rcar-host.o
>  obj-$(CONFIG_PCIE_RCAR_EP) += pcie-rcar.o pcie-rcar-ep.o
>  obj-$(CONFIG_PCI_HOST_COMMON) += pci-host-common.o
>  obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
> +obj-$(CONFIG_PCI_HOST_HELPERS) += pci-host-helpers.o
>  obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
>  obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
> diff --git a/drivers/pci/controller/pci-host-helpers.c b/drivers/pci/controller/pci-host-helpers.c
> new file mode 100644
> index 000000000000..cd261a281c60
> --- /dev/null
> +++ b/drivers/pci/controller/pci-host-helpers.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCI Host Controller Helper Functions
> + *
> + * Copyright (C) 2025 Hans Zhang
> + *
> + * Author: Hans Zhang <18255117159@163.com>
> + */
> +
> +#include <linux/pci.h>
> +
> +#include "../pci.h"
> +
> +/*
> + * 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 __pci_host_bridge_find_next_cap(void *priv,
> +					  pci_host_bridge_read_cfg read_cfg,
> +					  u8 cap_ptr, u8 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_host_bridge_find_next_cap(priv, read_cfg, next_cap_ptr,
> +					       cap);
> +}
> +
> +u8 pci_host_bridge_find_capability(void *priv,
> +				   pci_host_bridge_read_cfg read_cfg, u8 cap)
> +{
> +	u8 next_cap_ptr;
> +	u16 reg;
> +
> +	reg = read_cfg(priv, PCI_CAPABILITY_LIST, 2);
> +	next_cap_ptr = (reg & 0x00ff);
> +
> +	return __pci_host_bridge_find_next_cap(priv, read_cfg, next_cap_ptr,
> +					       cap);
> +}
> +EXPORT_SYMBOL_GPL(pci_host_bridge_find_capability);
> +
> +static u16 pci_host_bridge_find_next_ext_capability(
> +	void *priv, pci_host_bridge_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_host_bridge_find_ext_capability(void *priv,
> +					pci_host_bridge_read_cfg read_cfg,
> +					u8 cap)
> +{
> +	return pci_host_bridge_find_next_ext_capability(priv, read_cfg, 0, cap);
> +}
> +EXPORT_SYMBOL_GPL(pci_host_bridge_find_ext_capability);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 01e51db8d285..8d1c919cbfef 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -1034,4 +1034,11 @@ void pcim_release_region(struct pci_dev *pdev, int bar);
>  	(PCI_CONF1_ADDRESS(bus, dev, func, reg) | \
>  	 PCI_CONF1_EXT_REG(reg))
>  
> +typedef u32 (*pci_host_bridge_read_cfg)(void *priv, int where, int size);
> +u8 pci_host_bridge_find_capability(void *priv,
> +				   pci_host_bridge_read_cfg read_cfg, u8 cap);
> +u16 pci_host_bridge_find_ext_capability(void *priv,
> +					pci_host_bridge_read_cfg read_cfg,
> +					u8 cap);
> +
>  #endif /* DRIVERS_PCI_H */
> -- 
> 2.25.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [v6 1/5] PCI: Introduce generic capability search functions

[Hans Zhang]

On 2025/3/28 00:58, Manivannan Sadhasivam wrote:
> On Mon, Mar 24, 2025 at 12:48:48AM +0800, Hans Zhang wrote:
>> Existing controller drivers (e.g., DWC, custom out-of-tree drivers)
> 
> Oh, you should not mention 'out-of-tree drivers' as this patch is not anyway
> intented to benefit them. We certainly do not care about out of tree drivers.
> 


Hi Mani,

Thanks your for reply. Will delete.

Best regards,
Hans

[v6 2/5] PCI: dwc: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang]

Since the PCI core is now exposing generic APIs for the host bridges to
search for the PCIe capabilities, make use of them in the DWC driver.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
Changes since v5:
https://lore.kernel.org/linux-pci/20250321163803.391056-3-18255117159@163.com/

- Kconfig add "select PCI_HOST_HELPERS"
---
 drivers/pci/controller/dwc/Kconfig           |  1 +
 drivers/pci/controller/dwc/pcie-designware.c | 71 ++------------------
 2 files changed, 6 insertions(+), 66 deletions(-)

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index b6d6778b0698..c71b3ad44f3e 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -5,6 +5,7 @@ menu "DesignWare-based PCIe controllers"
 
 config PCIE_DW
 	bool
+	select PCI_HOST_HELPERS
 
 config PCIE_DW_HOST
 	bool
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 145e7f579072..0329f233cf11 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -203,83 +203,22 @@ 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 u32 dwc_pcie_read_cfg(void *priv, int where, int size)
 {
-	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 *pci = priv;
 
-	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 dw_pcie_read_dbi(pci, where, size);
 }
 
 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_host_bridge_find_capability(pci, dwc_pcie_read_cfg, 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_host_bridge_find_ext_capability(pci, dwc_pcie_read_cfg, cap);
 }
 EXPORT_SYMBOL_GPL(dw_pcie_find_ext_capability);
 
-- 
2.25.1

[v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang]

Since the PCI core is now exposing generic APIs for the host bridges to
search for the PCIe capabilities, make use of them in the CDNS driver.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
Changes since v5:
https://lore.kernel.org/linux-pci/20250321163803.391056-4-18255117159@163.com

- Kconfig add "select PCI_HOST_HELPERS"
---
 drivers/pci/controller/cadence/Kconfig        |  1 +
 drivers/pci/controller/cadence/pcie-cadence.c | 25 +++++++++++++++++++
 drivers/pci/controller/cadence/pcie-cadence.h |  3 +++
 3 files changed, 29 insertions(+)

diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
index 8a0044bb3989..0a4f245bbeb0 100644
--- a/drivers/pci/controller/cadence/Kconfig
+++ b/drivers/pci/controller/cadence/Kconfig
@@ -5,6 +5,7 @@ menu "Cadence-based PCIe controllers"
 
 config PCIE_CADENCE
 	bool
+	select PCI_HOST_HELPERS
 
 config PCIE_CADENCE_HOST
 	bool
diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
index 204e045aed8c..329dab4ff813 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_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
+{
+	return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
+}
+
+u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
+{
+	return pci_host_bridge_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..6f4981fccb94 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_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,
-- 
2.25.1

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[kernel test robot]

Hi Hans,

kernel test robot noticed the following build errors:

[auto build test ERROR on a1cffe8cc8aef85f1b07c4464f0998b9785b795a]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-Zhang/PCI-Introduce-generic-capability-search-functions/20250324-005300
base:   a1cffe8cc8aef85f1b07c4464f0998b9785b795a
patch link:    https://lore.kernel.org/r/20250323164852.430546-4-18255117159%40163.com
patch subject: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities
config: csky-randconfig-001-20250324 (https://download.01.org/0day-ci/archive/20250324/202503240212.GtZsXgoK-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250324/202503240212.GtZsXgoK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503240212.GtZsXgoK-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/pci/controller/cadence/pcie-cadence.c: In function 'cdns_pcie_find_capability':
>> drivers/pci/controller/cadence/pcie-cadence.c:28:16: error: implicit declaration of function 'pci_host_bridge_find_capability'; did you mean 'cdns_pcie_find_capability'? [-Wimplicit-function-declaration]
      28 |         return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
         |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                cdns_pcie_find_capability
   drivers/pci/controller/cadence/pcie-cadence.c: In function 'cdns_pcie_find_ext_capability':
>> drivers/pci/controller/cadence/pcie-cadence.c:33:16: error: implicit declaration of function 'pci_host_bridge_find_ext_capability'; did you mean 'cdns_pcie_find_ext_capability'? [-Wimplicit-function-declaration]
      33 |         return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg, cap);
         |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                cdns_pcie_find_ext_capability


vim +28 drivers/pci/controller/cadence/pcie-cadence.c

    25	
    26	u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
    27	{
  > 28		return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
    29	}
    30	
    31	u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
    32	{
  > 33		return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg, cap);
    34	}
    35	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang]

On 2025/3/24 02:33, kernel test robot wrote:
> Hi Hans,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on a1cffe8cc8aef85f1b07c4464f0998b9785b795a]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Hans-Zhang/PCI-Introduce-generic-capability-search-functions/20250324-005300
> base:   a1cffe8cc8aef85f1b07c4464f0998b9785b795a
> patch link:    https://lore.kernel.org/r/20250323164852.430546-4-18255117159%40163.com
> patch subject: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities
> config: csky-randconfig-001-20250324 (https://download.01.org/0day-ci/archive/20250324/202503240212.GtZsXgoK-lkp@intel.com/config)
> compiler: csky-linux-gcc (GCC) 14.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250324/202503240212.GtZsXgoK-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202503240212.GtZsXgoK-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>     drivers/pci/controller/cadence/pcie-cadence.c: In function 'cdns_pcie_find_capability':
>>> drivers/pci/controller/cadence/pcie-cadence.c:28:16: error: implicit declaration of function 'pci_host_bridge_find_capability'; did you mean 'cdns_pcie_find_capability'? [-Wimplicit-function-declaration]
>        28 |         return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
>           |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>           |                cdns_pcie_find_capability
>     drivers/pci/controller/cadence/pcie-cadence.c: In function 'cdns_pcie_find_ext_capability':
>>> drivers/pci/controller/cadence/pcie-cadence.c:33:16: error: implicit declaration of function 'pci_host_bridge_find_ext_capability'; did you mean 'cdns_pcie_find_ext_capability'? [-Wimplicit-function-declaration]
>        33 |         return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg, cap);
>           |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>           |                cdns_pcie_find_ext_capability
> 

Missing header file: #include "... /.. /pci.h". Will change.

Best regards,
Hans

> vim +28 drivers/pci/controller/cadence/pcie-cadence.c
> 
>      25	
>      26	u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
>      27	{
>    > 28		return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
>      29	}
>      30	
>      31	u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
>      32	{
>    > 33		return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg, cap);
>      34	}
>      35	
> 

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[kernel test robot]

Hi Hans,

kernel test robot noticed the following build errors:

[auto build test ERROR on a1cffe8cc8aef85f1b07c4464f0998b9785b795a]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-Zhang/PCI-Introduce-generic-capability-search-functions/20250324-005300
base:   a1cffe8cc8aef85f1b07c4464f0998b9785b795a
patch link:    https://lore.kernel.org/r/20250323164852.430546-4-18255117159%40163.com
patch subject: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities
config: riscv-randconfig-001-20250324 (https://download.01.org/0day-ci/archive/20250324/202503240338.N37HXlm9-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project c2692afc0a92cd5da140dfcdfff7818a5b8ce997)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250324/202503240338.N37HXlm9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503240338.N37HXlm9-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/pci/controller/cadence/pcie-cadence.c:20:11: warning: variable 'val' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
      20 |         else if (size == 1)
         |                  ^~~~~~~~~
   drivers/pci/controller/cadence/pcie-cadence.c:23:9: note: uninitialized use occurs here
      23 |         return val;
         |                ^~~
   drivers/pci/controller/cadence/pcie-cadence.c:20:7: note: remove the 'if' if its condition is always true
      20 |         else if (size == 1)
         |              ^~~~~~~~~~~~~~
      21 |                 val = readb(pcie->reg_base + where);
   drivers/pci/controller/cadence/pcie-cadence.c:14:9: note: initialize the variable 'val' to silence this warning
      14 |         u32 val;
         |                ^
         |                 = 0
>> drivers/pci/controller/cadence/pcie-cadence.c:28:9: error: call to undeclared function 'pci_host_bridge_find_capability'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      28 |         return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
         |                ^
>> drivers/pci/controller/cadence/pcie-cadence.c:33:9: error: call to undeclared function 'pci_host_bridge_find_ext_capability'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      33 |         return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg, cap);
         |                ^
   drivers/pci/controller/cadence/pcie-cadence.c:33:9: note: did you mean 'cdns_pcie_find_ext_capability'?
   drivers/pci/controller/cadence/pcie-cadence.c:31:5: note: 'cdns_pcie_find_ext_capability' declared here
      31 | u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
         |     ^
      32 | {
      33 |         return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg, cap);
         |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                cdns_pcie_find_ext_capability
   1 warning and 2 errors generated.


vim +/pci_host_bridge_find_capability +28 drivers/pci/controller/cadence/pcie-cadence.c

    25	
    26	u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
    27	{
  > 28		return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
    29	}
    30	
    31	u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
    32	{
  > 33		return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg, cap);
    34	}
    35	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang]

On 2025/3/24 03:26, kernel test robot wrote:
> Hi Hans,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on a1cffe8cc8aef85f1b07c4464f0998b9785b795a]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Hans-Zhang/PCI-Introduce-generic-capability-search-functions/20250324-005300
> base:   a1cffe8cc8aef85f1b07c4464f0998b9785b795a
> patch link:    https://lore.kernel.org/r/20250323164852.430546-4-18255117159%40163.com
> patch subject: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities
> config: riscv-randconfig-001-20250324 (https://download.01.org/0day-ci/archive/20250324/202503240338.N37HXlm9-lkp@intel.com/config)
> compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project c2692afc0a92cd5da140dfcdfff7818a5b8ce997)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250324/202503240338.N37HXlm9-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202503240338.N37HXlm9-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>     drivers/pci/controller/cadence/pcie-cadence.c:20:11: warning: variable 'val' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
>        20 |         else if (size == 1)
>           |                  ^~~~~~~~~
>     drivers/pci/controller/cadence/pcie-cadence.c:23:9: note: uninitialized use occurs here
>        23 |         return val;
>           |                ^~~
>     drivers/pci/controller/cadence/pcie-cadence.c:20:7: note: remove the 'if' if its condition is always true
>        20 |         else if (size == 1)
>           |              ^~~~~~~~~~~~~~
>        21 |                 val = readb(pcie->reg_base + where);
>     drivers/pci/controller/cadence/pcie-cadence.c:14:9: note: initialize the variable 'val' to silence this warning
>        14 |         u32 val;
>           |                ^
>           |                 = 0

Will change. u32 val = 0;

>>> drivers/pci/controller/cadence/pcie-cadence.c:28:9: error: call to undeclared function 'pci_host_bridge_find_capability'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>        28 |         return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
>           |                ^
>>> drivers/pci/controller/cadence/pcie-cadence.c:33:9: error: call to undeclared function 'pci_host_bridge_find_ext_capability'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>        33 |         return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg, cap);
>           |                ^
>     drivers/pci/controller/cadence/pcie-cadence.c:33:9: note: did you mean 'cdns_pcie_find_ext_capability'?
>     drivers/pci/controller/cadence/pcie-cadence.c:31:5: note: 'cdns_pcie_find_ext_capability' declared here
>        31 | u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
>           |     ^
>        32 | {
>        33 |         return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg, cap);
>           |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>           |                cdns_pcie_find_ext_capability
>     1 warning and 2 errors generated.
> 
> 
> vim +/pci_host_bridge_find_capability +28 drivers/pci/controller/cadence/pcie-cadence.c
> 
>      25	
>      26	u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
>      27	{
>    > 28		return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
>      29	}
>      30	
>      31	u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
>      32	{
>    > 33		return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg, cap);
>      34	}
>      35	
> 

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Ilpo Järvinen]

On Mon, 24 Mar 2025, Hans Zhang wrote:

> Since the PCI core is now exposing generic APIs for the host bridges to

No need to say "since ... is now exposing". Just say "Use ..." as if the 
API has always existed even if you just added it.

> search for the PCIe capabilities, make use of them in the CDNS driver.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
> Changes since v5:
> https://lore.kernel.org/linux-pci/20250321163803.391056-4-18255117159@163.com
> 
> - Kconfig add "select PCI_HOST_HELPERS"
> ---
>  drivers/pci/controller/cadence/Kconfig        |  1 +
>  drivers/pci/controller/cadence/pcie-cadence.c | 25 +++++++++++++++++++
>  drivers/pci/controller/cadence/pcie-cadence.h |  3 +++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
> index 8a0044bb3989..0a4f245bbeb0 100644
> --- a/drivers/pci/controller/cadence/Kconfig
> +++ b/drivers/pci/controller/cadence/Kconfig
> @@ -5,6 +5,7 @@ menu "Cadence-based PCIe controllers"
>  
>  config PCIE_CADENCE
>  	bool
> +	select PCI_HOST_HELPERS
>  
>  config PCIE_CADENCE_HOST
>  	bool
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
> index 204e045aed8c..329dab4ff813 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);

Should this use cdns_pcie_readl() ?

> +	else if (size == 2)
> +		val = readw(pcie->reg_base + where);
> +	else if (size == 1)
> +		val = readb(pcie->reg_base + where);
> +
> +	return val;
> +}
> +
> +u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
> +{
> +	return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
> +}
> +
> +u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
> +{
> +	return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg, cap);
> +}

I'm really wondering why the read config function is provided directly as 
an argument. Shouldn't struct pci_host_bridge have some ops that can read 
config so wouldn't it make much more sense to pass it and use the func 
from there? There seems to ops in pci_host_bridge that has read(), does 
that work? If not, why?

> +
>  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..6f4981fccb94 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_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,
> 

-- 
 i.

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang]

On 2025/3/24 21:44, Ilpo Järvinen wrote:
> On Mon, 24 Mar 2025, Hans Zhang wrote:
> 
>> Since the PCI core is now exposing generic APIs for the host bridges to
> 
> No need to say "since ... is now exposing". Just say "Use ..." as if the
> API has always existed even if you just added it.
> 

Hi Ilpo,

Thanks your for reply. Will change.


>> search for the PCIe capabilities, make use of them in the CDNS driver.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>> Changes since v5:
>> https://lore.kernel.org/linux-pci/20250321163803.391056-4-18255117159@163.com
>>
>> - Kconfig add "select PCI_HOST_HELPERS"
>> ---
>>   drivers/pci/controller/cadence/Kconfig        |  1 +
>>   drivers/pci/controller/cadence/pcie-cadence.c | 25 +++++++++++++++++++
>>   drivers/pci/controller/cadence/pcie-cadence.h |  3 +++
>>   3 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
>> index 8a0044bb3989..0a4f245bbeb0 100644
>> --- a/drivers/pci/controller/cadence/Kconfig
>> +++ b/drivers/pci/controller/cadence/Kconfig
>> @@ -5,6 +5,7 @@ menu "Cadence-based PCIe controllers"
>>   
>>   config PCIE_CADENCE
>>   	bool
>> +	select PCI_HOST_HELPERS
>>   
>>   config PCIE_CADENCE_HOST
>>   	bool
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
>> index 204e045aed8c..329dab4ff813 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);
> 
> Should this use cdns_pcie_readl() ?

pci_host_bridge_find_*capability required to read two or four bytes.

reg = read_cfg(priv, cap_ptr, 2);
or
header = read_cfg(priv, pos, 4);

Here I mainly want to write it the same way as size == 2 and size == 1.
Or size == 4 should I write it as cdns_pcie_readl() ?

> 
>> +	else if (size == 2)
>> +		val = readw(pcie->reg_base + where);
>> +	else if (size == 1)
>> +		val = readb(pcie->reg_base + where);
>> +
>> +	return val;
>> +}
>> +
>> +u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
>> +{
>> +	return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
>> +}
>> +
>> +u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
>> +{
>> +	return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg, cap);
>> +}
> 
> I'm really wondering why the read config function is provided directly as
> an argument. Shouldn't struct pci_host_bridge have some ops that can read
> config so wouldn't it make much more sense to pass it and use the func
> from there? There seems to ops in pci_host_bridge that has read(), does
> that work? If not, why?
> 

No effect. Because we need to get the offset of the capability before 
PCIe enumerates the device. I originally added a separate find 
capability related function for CDNS in the following patch. It's also 
copied directly from DWC. Mani felt there was too much duplicate code 
and also suggested passing a callback function that could manipulate the 
registers of the root port of DWC or CDNS.

https://patchwork.kernel.org/project/linux-pci/patch/20250308133903.322216-1-18255117159@163.com/

The original function is in the following file:
drivers/pci/controller/dwc/pcie-designware.c
u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)

CDNS has the same need to find the offset of the capability.

>> +
>>   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..6f4981fccb94 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_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,
>>
> 

Best regards,
Hans

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5841 bytes --]

On Mon, 24 Mar 2025, Hans Zhang wrote:
> On 2025/3/24 21:44, Ilpo Järvinen wrote:
> > On Mon, 24 Mar 2025, Hans Zhang wrote:
> > 
> > > Since the PCI core is now exposing generic APIs for the host bridges to
> > 
> > No need to say "since ... is now exposing". Just say "Use ..." as if the
> > API has always existed even if you just added it.
> > 
> 
> Hi Ilpo,
> 
> Thanks your for reply. Will change.
> 
> 
> > > search for the PCIe capabilities, make use of them in the CDNS driver.
> > > 
> > > Signed-off-by: Hans Zhang <18255117159@163.com>
> > > ---
> > > Changes since v5:
> > > https://lore.kernel.org/linux-pci/20250321163803.391056-4-18255117159@163.com
> > > 
> > > - Kconfig add "select PCI_HOST_HELPERS"
> > > ---
> > >   drivers/pci/controller/cadence/Kconfig        |  1 +
> > >   drivers/pci/controller/cadence/pcie-cadence.c | 25 +++++++++++++++++++
> > >   drivers/pci/controller/cadence/pcie-cadence.h |  3 +++
> > >   3 files changed, 29 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/cadence/Kconfig
> > > b/drivers/pci/controller/cadence/Kconfig
> > > index 8a0044bb3989..0a4f245bbeb0 100644
> > > --- a/drivers/pci/controller/cadence/Kconfig
> > > +++ b/drivers/pci/controller/cadence/Kconfig
> > > @@ -5,6 +5,7 @@ menu "Cadence-based PCIe controllers"
> > >     config PCIE_CADENCE
> > >   	bool
> > > +	select PCI_HOST_HELPERS
> > >     config PCIE_CADENCE_HOST
> > >   	bool
> > > diff --git a/drivers/pci/controller/cadence/pcie-cadence.c
> > > b/drivers/pci/controller/cadence/pcie-cadence.c
> > > index 204e045aed8c..329dab4ff813 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);
> > 
> > Should this use cdns_pcie_readl() ?
> 
> pci_host_bridge_find_*capability required to read two or four bytes.
> 
> reg = read_cfg(priv, cap_ptr, 2);
> or
> header = read_cfg(priv, pos, 4);
> 
> Here I mainly want to write it the same way as size == 2 and size == 1.
> Or size == 4 should I write it as cdns_pcie_readl() ?

As is, it seems two functions are added for the same thing for the case 
with size == 4 with different names which feels duplication. One could add 
cdns_pcie_readw() and cdns_pcie_readb() too but perhaps cdns_pcie_readl() 
should just call this new function instead?

> > > +	else if (size == 2)
> > > +		val = readw(pcie->reg_base + where);
> > > +	else if (size == 1)
> > > +		val = readb(pcie->reg_base + where);
> > > +
> > > +	return val;
> > > +}
> > > +
> > > +u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
> > > +{
> > > +	return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
> > > +}
> > > +
> > > +u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
> > > +{
> > > +	return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg,
> > > cap);
> > > +}
> > 
> > I'm really wondering why the read config function is provided directly as
> > an argument. Shouldn't struct pci_host_bridge have some ops that can read
> > config so wouldn't it make much more sense to pass it and use the func
> > from there? There seems to ops in pci_host_bridge that has read(), does
> > that work? If not, why?
> > 
> 
> No effect. 

I'm not sure what you meant?

> Because we need to get the offset of the capability before PCIe
> enumerates the device. 

Is this to say it is needed before the struct pci_host_bridge is created?

> I originally added a separate find capability related
> function for CDNS in the following patch. It's also copied directly from DWC.
> Mani felt there was too much duplicate code and also suggested passing a
> callback function that could manipulate the registers of the root port of DWC
> or CDNS.

I very much like the direction this patchset is moving (moving shared 
part of controllers code to core), I just feel this doesn't go far enough 
when it's passing function pointer to the read function.

I admit I've never written a controller driver so perhaps there's 
something detail I lack knowledge of but I'd want to understand why
struct pci_ops (which exists both in pci_host_bridge and pci_bus) cannot 
be used?

> https://patchwork.kernel.org/project/linux-pci/patch/20250308133903.322216-1-18255117159@163.com/
> 
> The original function is in the following file:
> drivers/pci/controller/dwc/pcie-designware.c
> u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
> u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
> 
> CDNS has the same need to find the offset of the capability.
> 
> > > +
> > >   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..6f4981fccb94 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_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,
> > > 
> > 
> 
> Best regards,
> Hans
> 

-- 
 i.

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang]

On 2025/3/24 23:02, Ilpo Järvinen wrote:
>>>>    +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);
>>>
>>> Should this use cdns_pcie_readl() ?
>>
>> pci_host_bridge_find_*capability required to read two or four bytes.
>>
>> reg = read_cfg(priv, cap_ptr, 2);
>> or
>> header = read_cfg(priv, pos, 4);
>>
>> Here I mainly want to write it the same way as size == 2 and size == 1.
>> Or size == 4 should I write it as cdns_pcie_readl() ?
> 
> As is, it seems two functions are added for the same thing for the case
> with size == 4 with different names which feels duplication. One could add
> cdns_pcie_readw() and cdns_pcie_readb() too but perhaps cdns_pcie_readl()
> should just call this new function instead?

Hi Ilpo,

Redefine a function with reference to DWC?

u32 dw_pcie_read_dbi(struct dw_pcie *pci, u32 reg, size_t size)
   dw_pcie_read(pci->dbi_base + reg, size, &val);
     dw_pcie_read

int dw_pcie_read(void __iomem *addr, int size, u32 *val)
{
	if (!IS_ALIGNED((uintptr_t)addr, size)) {
		*val = 0;
		return PCIBIOS_BAD_REGISTER_NUMBER;
	}

	if (size == 4) {
		*val = readl(addr);
	} else if (size == 2) {
		*val = readw(addr);
	} else if (size == 1) {
		*val = readb(addr);
	} else {
		*val = 0;
		return PCIBIOS_BAD_REGISTER_NUMBER;
	}

	return PCIBIOS_SUCCESSFUL;
}
EXPORT_SYMBOL_GPL(dw_pcie_read);

> 
>>>> +	else if (size == 2)
>>>> +		val = readw(pcie->reg_base + where);
>>>> +	else if (size == 1)
>>>> +		val = readb(pcie->reg_base + where);
>>>> +
>>>> +	return val;
>>>> +}
>>>> +
>>>> +u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
>>>> +{
>>>> +	return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
>>>> +}
>>>> +
>>>> +u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
>>>> +{
>>>> +	return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg,
>>>> cap);
>>>> +}
>>>
>>> I'm really wondering why the read config function is provided directly as
>>> an argument. Shouldn't struct pci_host_bridge have some ops that can read
>>> config so wouldn't it make much more sense to pass it and use the func
>>> from there? There seems to ops in pci_host_bridge that has read(), does
>>> that work? If not, why?
>>>
>>
>> No effect.
> 
> I'm not sure what you meant?
> 
>> Because we need to get the offset of the capability before PCIe
>> enumerates the device.
> 
> Is this to say it is needed before the struct pci_host_bridge is created?
> 
>> I originally added a separate find capability related
>> function for CDNS in the following patch. It's also copied directly from DWC.
>> Mani felt there was too much duplicate code and also suggested passing a
>> callback function that could manipulate the registers of the root port of DWC
>> or CDNS.
> 
> I very much like the direction this patchset is moving (moving shared
> part of controllers code to core), I just feel this doesn't go far enough
> when it's passing function pointer to the read function.
> 
> I admit I've never written a controller driver so perhaps there's
> something detail I lack knowledge of but I'd want to understand why
> struct pci_ops (which exists both in pci_host_bridge and pci_bus) cannot
> be used?
> 


I don't know if the following code can make it clear to you.

static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
	.host_init	= qcom_pcie_host_init,
                   pcie->cfg->ops->post_init(pcie);
                     qcom_pcie_post_init_2_3_3
                       dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
};

int dw_pcie_host_init(struct dw_pcie_rp *pp)
   bridge = devm_pci_alloc_host_bridge(dev, 0);
   if (pp->ops->host_init)
     pp->ops = &qcom_pcie_dw_ops;  // qcom here needs to find capability

   pci_host_probe(bridge); // pcie enumerate flow
     pci_scan_root_bus_bridge(bridge);
       pci_register_host_bridge(bridge);
         bus->ops = bridge->ops;   // Only pci bus ops can be used


Best regards,
Hans

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5483 bytes --]

On Tue, 25 Mar 2025, Hans Zhang wrote:
> On 2025/3/24 23:02, Ilpo Järvinen wrote:
> > > > >    +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);
> > > > 
> > > > Should this use cdns_pcie_readl() ?
> > > 
> > > pci_host_bridge_find_*capability required to read two or four bytes.
> > > 
> > > reg = read_cfg(priv, cap_ptr, 2);
> > > or
> > > header = read_cfg(priv, pos, 4);
> > > 
> > > Here I mainly want to write it the same way as size == 2 and size == 1.
> > > Or size == 4 should I write it as cdns_pcie_readl() ?
> > 
> > As is, it seems two functions are added for the same thing for the case
> > with size == 4 with different names which feels duplication. One could add
> > cdns_pcie_readw() and cdns_pcie_readb() too but perhaps cdns_pcie_readl()
> > should just call this new function instead?
> 
> Hi Ilpo,
> 
> Redefine a function with reference to DWC?

This patch was about cadence so my comment above what related to that.

> u32 dw_pcie_read_dbi(struct dw_pcie *pci, u32 reg, size_t size)
>   dw_pcie_read(pci->dbi_base + reg, size, &val);
>     dw_pcie_read
> 
> int dw_pcie_read(void __iomem *addr, int size, u32 *val)
> {
> 	if (!IS_ALIGNED((uintptr_t)addr, size)) {
> 		*val = 0;
> 		return PCIBIOS_BAD_REGISTER_NUMBER;
> 	}
> 
> 	if (size == 4) {
> 		*val = readl(addr);
> 	} else if (size == 2) {
> 		*val = readw(addr);
> 	} else if (size == 1) {
> 		*val = readb(addr);
> 	} else {
> 		*val = 0;
> 		return PCIBIOS_BAD_REGISTER_NUMBER;
> 	}
> 
> 	return PCIBIOS_SUCCESSFUL;
> }
> EXPORT_SYMBOL_GPL(dw_pcie_read);
> 
> > 
> > > > > +	else if (size == 2)
> > > > > +		val = readw(pcie->reg_base + where);
> > > > > +	else if (size == 1)
> > > > > +		val = readb(pcie->reg_base + where);
> > > > > +
> > > > > +	return val;
> > > > > +}
> > > > > +
> > > > > +u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
> > > > > +{
> > > > > +	return pci_host_bridge_find_capability(pcie,
> > > > > cdns_pcie_read_cfg, cap);
> > > > > +}
> > > > > +
> > > > > +u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
> > > > > +{
> > > > > +	return pci_host_bridge_find_ext_capability(pcie,
> > > > > cdns_pcie_read_cfg,
> > > > > cap);
> > > > > +}
> > > > 
> > > > I'm really wondering why the read config function is provided directly
> > > > as
> > > > an argument. Shouldn't struct pci_host_bridge have some ops that can
> > > > read
> > > > config so wouldn't it make much more sense to pass it and use the func
> > > > from there? There seems to ops in pci_host_bridge that has read(), does
> > > > that work? If not, why?
> > > > 
> > > 
> > > No effect.
> > 
> > I'm not sure what you meant?
> > 
> > > Because we need to get the offset of the capability before PCIe
> > > enumerates the device.
> > 
> > Is this to say it is needed before the struct pci_host_bridge is created?
> > 
> > > I originally added a separate find capability related
> > > function for CDNS in the following patch. It's also copied directly from
> > > DWC.
> > > Mani felt there was too much duplicate code and also suggested passing a
> > > callback function that could manipulate the registers of the root port of
> > > DWC
> > > or CDNS.
> > 
> > I very much like the direction this patchset is moving (moving shared
> > part of controllers code to core), I just feel this doesn't go far enough
> > when it's passing function pointer to the read function.
> > 
> > I admit I've never written a controller driver so perhaps there's
> > something detail I lack knowledge of but I'd want to understand why
> > struct pci_ops (which exists both in pci_host_bridge and pci_bus) cannot
> > be used?
> > 
> 
> 
> I don't know if the following code can make it clear to you.
> 
> static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> 	.host_init	= qcom_pcie_host_init,
>                   pcie->cfg->ops->post_init(pcie);
>                     qcom_pcie_post_init_2_3_3
>                       dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> };
> 
> int dw_pcie_host_init(struct dw_pcie_rp *pp)
>   bridge = devm_pci_alloc_host_bridge(dev, 0);

It does this almost immediately:

    bridge->ops = &dw_pcie_ops;

Can we like add some function into those ops such that the necessary read 
can be performed? Like .early_root_config_read or something like that?

Then the host bridge capability finder can input struct pci_host_bridge 
*host_bridge and can do host_bridge->ops->early_root_cfg_read(host_bridge, 
...). That would already be a big win over passing the read function 
itself as a pointer.

Hopefully having such a function in the ops would allow moving other 
common controller driver functionality into PCI core as well as it would 
abstract the per controller read function (for the time before everything 
is fully instanciated).

Is that a workable approach?

>   if (pp->ops->host_init)
>     pp->ops = &qcom_pcie_dw_ops;  // qcom here needs to find capability
>
>   pci_host_probe(bridge); // pcie enumerate flow
>     pci_scan_root_bus_bridge(bridge);
>       pci_register_host_bridge(bridge);
>         bus->ops = bridge->ops;   // Only pci bus ops can be used
> 
> 
> Best regards,
> Hans
> 

-- 
 i.

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang]

On 2025/3/25 19:15, Ilpo Järvinen wrote:
> On Tue, 25 Mar 2025, Hans Zhang wrote:
>> On 2025/3/24 23:02, Ilpo Järvinen wrote:
>>>>>>     +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);
>>>>>
>>>>> Should this use cdns_pcie_readl() ?
>>>>
>>>> pci_host_bridge_find_*capability required to read two or four bytes.
>>>>
>>>> reg = read_cfg(priv, cap_ptr, 2);
>>>> or
>>>> header = read_cfg(priv, pos, 4);
>>>>
>>>> Here I mainly want to write it the same way as size == 2 and size == 1.
>>>> Or size == 4 should I write it as cdns_pcie_readl() ?
>>>
>>> As is, it seems two functions are added for the same thing for the case
>>> with size == 4 with different names which feels duplication. One could add
>>> cdns_pcie_readw() and cdns_pcie_readb() too but perhaps cdns_pcie_readl()
>>> should just call this new function instead?
>>
>> Hi Ilpo,
>>
>> Redefine a function with reference to DWC?
> 
> This patch was about cadence so my comment above what related to that.
> 

Hi Ilpo,

Thanks your for reply. Let's look at the main problem first.

>> u32 dw_pcie_read_dbi(struct dw_pcie *pci, u32 reg, size_t size)
>>    dw_pcie_read(pci->dbi_base + reg, size, &val);
>>      dw_pcie_read
>>
>> int dw_pcie_read(void __iomem *addr, int size, u32 *val)
>> {
>> 	if (!IS_ALIGNED((uintptr_t)addr, size)) {
>> 		*val = 0;
>> 		return PCIBIOS_BAD_REGISTER_NUMBER;
>> 	}
>>
>> 	if (size == 4) {
>> 		*val = readl(addr);
>> 	} else if (size == 2) {
>> 		*val = readw(addr);
>> 	} else if (size == 1) {
>> 		*val = readb(addr);
>> 	} else {
>> 		*val = 0;
>> 		return PCIBIOS_BAD_REGISTER_NUMBER;
>> 	}
>>
>> 	return PCIBIOS_SUCCESSFUL;
>> }
>> EXPORT_SYMBOL_GPL(dw_pcie_read);
>>
>>>
>>>>>> +	else if (size == 2)
>>>>>> +		val = readw(pcie->reg_base + where);
>>>>>> +	else if (size == 1)
>>>>>> +		val = readb(pcie->reg_base + where);
>>>>>> +
>>>>>> +	return val;
>>>>>> +}
>>>>>> +
>>>>>> +u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
>>>>>> +{
>>>>>> +	return pci_host_bridge_find_capability(pcie,
>>>>>> cdns_pcie_read_cfg, cap);
>>>>>> +}
>>>>>> +
>>>>>> +u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
>>>>>> +{
>>>>>> +	return pci_host_bridge_find_ext_capability(pcie,
>>>>>> cdns_pcie_read_cfg,
>>>>>> cap);
>>>>>> +}
>>>>>
>>>>> I'm really wondering why the read config function is provided directly
>>>>> as
>>>>> an argument. Shouldn't struct pci_host_bridge have some ops that can
>>>>> read
>>>>> config so wouldn't it make much more sense to pass it and use the func
>>>>> from there? There seems to ops in pci_host_bridge that has read(), does
>>>>> that work? If not, why?
>>>>>
>>>>
>>>> No effect.
>>>
>>> I'm not sure what you meant?
>>>
>>>> Because we need to get the offset of the capability before PCIe
>>>> enumerates the device.
>>>
>>> Is this to say it is needed before the struct pci_host_bridge is created?
>>>
>>>> I originally added a separate find capability related
>>>> function for CDNS in the following patch. It's also copied directly from
>>>> DWC.
>>>> Mani felt there was too much duplicate code and also suggested passing a
>>>> callback function that could manipulate the registers of the root port of
>>>> DWC
>>>> or CDNS.
>>>
>>> I very much like the direction this patchset is moving (moving shared
>>> part of controllers code to core), I just feel this doesn't go far enough
>>> when it's passing function pointer to the read function.
>>>
>>> I admit I've never written a controller driver so perhaps there's
>>> something detail I lack knowledge of but I'd want to understand why
>>> struct pci_ops (which exists both in pci_host_bridge and pci_bus) cannot
>>> be used?
>>>
>>
>>
>> I don't know if the following code can make it clear to you.
>>
>> static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
>> 	.host_init	= qcom_pcie_host_init,
>>                    pcie->cfg->ops->post_init(pcie);
>>                      qcom_pcie_post_init_2_3_3
>>                        dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>> };
>>
>> int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>    bridge = devm_pci_alloc_host_bridge(dev, 0);
> 
> It does this almost immediately:
> 
>      bridge->ops = &dw_pcie_ops;
> 
> Can we like add some function into those ops such that the necessary read
> can be performed? Like .early_root_config_read or something like that?
> 
> Then the host bridge capability finder can input struct pci_host_bridge
> *host_bridge and can do host_bridge->ops->early_root_cfg_read(host_bridge,
> ...). That would already be a big win over passing the read function
> itself as a pointer.
> 
> Hopefully having such a function in the ops would allow moving other
> common controller driver functionality into PCI core as well as it would
> abstract the per controller read function (for the time before everything
> is fully instanciated).
> 
> Is that a workable approach?
>

I'll try to add and test it in your way first.

Another problem here is that I've seen some drivers invoke 
dw_pcie_find_*capability before if (pp->ops->init) {. When I confirm it, 
or I'll see if I can cover all the issues.

If I pass the test, I will provide the temporary patch here, please 
check whether it is OK, and then submit the next version. If not, we'll 
discuss it.

Thank you very much for your advice.

>>    if (pp->ops->host_init)
>>      pp->ops = &qcom_pcie_dw_ops;  // qcom here needs to find capability
>>
>>    pci_host_probe(bridge); // pcie enumerate flow
>>      pci_scan_root_bus_bridge(bridge);
>>        pci_register_host_bridge(bridge);
>>          bus->ops = bridge->ops;   // Only pci bus ops can be used
>>
>>


Best regards,
Hans

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang]

On 2025/3/25 20:16, Hans Zhang wrote:
>>>>>> I'm really wondering why the read config function is provided 
>>>>>> directly
>>>>>> as
>>>>>> an argument. Shouldn't struct pci_host_bridge have some ops that can
>>>>>> read
>>>>>> config so wouldn't it make much more sense to pass it and use the 
>>>>>> func
>>>>>> from there? There seems to ops in pci_host_bridge that has read(), 
>>>>>> does
>>>>>> that work? If not, why?
>>>>>>
>>>>>
>>>>> No effect.
>>>>
>>>> I'm not sure what you meant?
>>>>
>>>>> Because we need to get the offset of the capability before PCIe
>>>>> enumerates the device.
>>>>
>>>> Is this to say it is needed before the struct pci_host_bridge is 
>>>> created?
>>>>
>>>>> I originally added a separate find capability related
>>>>> function for CDNS in the following patch. It's also copied directly 
>>>>> from
>>>>> DWC.
>>>>> Mani felt there was too much duplicate code and also suggested 
>>>>> passing a
>>>>> callback function that could manipulate the registers of the root 
>>>>> port of
>>>>> DWC
>>>>> or CDNS.
>>>>
>>>> I very much like the direction this patchset is moving (moving shared
>>>> part of controllers code to core), I just feel this doesn't go far 
>>>> enough
>>>> when it's passing function pointer to the read function.
>>>>
>>>> I admit I've never written a controller driver so perhaps there's
>>>> something detail I lack knowledge of but I'd want to understand why
>>>> struct pci_ops (which exists both in pci_host_bridge and pci_bus) 
>>>> cannot
>>>> be used?
>>>>
>>>
>>>
>>> I don't know if the following code can make it clear to you.
>>>
>>> static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
>>>     .host_init    = qcom_pcie_host_init,
>>>                    pcie->cfg->ops->post_init(pcie);
>>>                      qcom_pcie_post_init_2_3_3
>>>                        dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>>> };
>>>
>>> int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>>    bridge = devm_pci_alloc_host_bridge(dev, 0);
>>
>> It does this almost immediately:
>>
>>      bridge->ops = &dw_pcie_ops;
>>
>> Can we like add some function into those ops such that the necessary read
>> can be performed? Like .early_root_config_read or something like that?
>>
>> Then the host bridge capability finder can input struct pci_host_bridge
>> *host_bridge and can do 
>> host_bridge->ops->early_root_cfg_read(host_bridge,
>> ...). That would already be a big win over passing the read function
>> itself as a pointer.
>>
>> Hopefully having such a function in the ops would allow moving other
>> common controller driver functionality into PCI core as well as it would
>> abstract the per controller read function (for the time before everything
>> is fully instanciated).
>>
>> Is that a workable approach?
>>
> 
> I'll try to add and test it in your way first.
> 
> Another problem here is that I've seen some drivers invoke 
> dw_pcie_find_*capability before if (pp->ops->init) {. When I confirm it, 
> or I'll see if I can cover all the issues.
> 
> If I pass the test, I will provide the temporary patch here, please 
> check whether it is OK, and then submit the next version. If not, we'll 
> discuss it.
> 

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?

> Thank you very much for your advice.
> 
>>>    if (pp->ops->host_init)
>>>      pp->ops = &qcom_pcie_dw_ops;  // qcom here needs to find capability
>>>
>>>    pci_host_probe(bridge); // pcie enumerate flow
>>>      pci_scan_root_bus_bridge(bridge);
>>>        pci_register_host_bridge(bridge);
>>>          bus->ops = bridge->ops;   // Only pci bus ops can be used
>>>
>>>

Best regards,
Hans

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5286 bytes --]

On Tue, 25 Mar 2025, Hans Zhang wrote:
> On 2025/3/25 20:16, Hans Zhang wrote:
> > > > > > > I'm really wondering why the read config function is provided
> > > > > > > directly
> > > > > > > as
> > > > > > > an argument. Shouldn't struct pci_host_bridge have some ops that
> > > > > > > can
> > > > > > > read
> > > > > > > config so wouldn't it make much more sense to pass it and use the
> > > > > > > func
> > > > > > > from there? There seems to ops in pci_host_bridge that has read(),
> > > > > > > does
> > > > > > > that work? If not, why?
> > > > > > > 
> > > > > > 
> > > > > > No effect.
> > > > > 
> > > > > I'm not sure what you meant?
> > > > > 
> > > > > > Because we need to get the offset of the capability before PCIe
> > > > > > enumerates the device.
> > > > > 
> > > > > Is this to say it is needed before the struct pci_host_bridge is
> > > > > created?
> > > > > 
> > > > > > I originally added a separate find capability related
> > > > > > function for CDNS in the following patch. It's also copied directly
> > > > > > from
> > > > > > DWC.
> > > > > > Mani felt there was too much duplicate code and also suggested
> > > > > > passing a
> > > > > > callback function that could manipulate the registers of the root
> > > > > > port of
> > > > > > DWC
> > > > > > or CDNS.
> > > > > 
> > > > > I very much like the direction this patchset is moving (moving shared
> > > > > part of controllers code to core), I just feel this doesn't go far
> > > > > enough
> > > > > when it's passing function pointer to the read function.
> > > > > 
> > > > > I admit I've never written a controller driver so perhaps there's
> > > > > something detail I lack knowledge of but I'd want to understand why
> > > > > struct pci_ops (which exists both in pci_host_bridge and pci_bus)
> > > > > cannot
> > > > > be used?
> > > > > 
> > > > 
> > > > 
> > > > I don't know if the following code can make it clear to you.
> > > > 
> > > > static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> > > >     .host_init    = qcom_pcie_host_init,
> > > >                    pcie->cfg->ops->post_init(pcie);
> > > >                      qcom_pcie_post_init_2_3_3
> > > >                        dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > > > };
> > > > 
> > > > int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > > >    bridge = devm_pci_alloc_host_bridge(dev, 0);
> > > 
> > > It does this almost immediately:
> > > 
> > >      bridge->ops = &dw_pcie_ops;
> > > 
> > > Can we like add some function into those ops such that the necessary read
> > > can be performed? Like .early_root_config_read or something like that?
> > > 
> > > Then the host bridge capability finder can input struct pci_host_bridge
> > > *host_bridge and can do host_bridge->ops->early_root_cfg_read(host_bridge,
> > > ...). That would already be a big win over passing the read function
> > > itself as a pointer.
> > > 
> > > Hopefully having such a function in the ops would allow moving other
> > > common controller driver functionality into PCI core as well as it would
> > > abstract the per controller read function (for the time before everything
> > > is fully instanciated).
> > > 
> > > Is that a workable approach?
> > > 
> > 
> > I'll try to add and test it in your way first.
> > 
> > Another problem here is that I've seen some drivers invoke
> > dw_pcie_find_*capability before if (pp->ops->init) {. When I confirm it, or
> > I'll see if I can cover all the issues.
> > 
> > If I pass the test, I will provide the temporary patch here, please check
> > whether it is OK, and then submit the next version. If not, we'll discuss
> > it.
> > 
> 
> 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)!

> > Thank you very much for your advice.
> > 
> > > >    if (pp->ops->host_init)
> > > >      pp->ops = &qcom_pcie_dw_ops;  // qcom here needs to find capability
> > > > 
> > > >    pci_host_probe(bridge); // pcie enumerate flow
> > > >      pci_scan_root_bus_bridge(bridge);
> > > >        pci_register_host_bridge(bridge);
> > > >          bus->ops = bridge->ops;   // Only pci bus ops can be used
> > > > 
> > > > 
> 
> Best regards,
> Hans
> 

-- 
 i.

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang]

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

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang]

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

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 14985 bytes --]

On Fri, 28 Mar 2025, Hans Zhang wrote:
> 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)     \

Please put it into drivers/pci/pci.h but other than that this is certainly 
to the direction I was suggesting.

You don't need to have those priv and devfn there like that but you can 
use the args... trick (see linux/iopoll.h) and put them as last parameters 
to the macro so they can wary based on the caller needs, assuming args 
will work like this, I've not tested:

		read_cfg(args, __pos, &val)

The size parameter is the most annoying one as it's in between where and 
*val arguments so args... trick won't work for it. I suggest just 
providing a function with the same signature as the related pci/access.c 
function for now.

A few nits related to the existing code quality of the cap parser function 
which should be addressed while we touch this function (probably in own 
patches indepedent of this code move/rearrange patch itself).

> +	({                                                                  \
> +		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)                                   \

I started to wonder if there's something for this and it turn out we've 
PCI_STD_HEADER_SIZEOF.

> +				break;                                      \
> +			__pos &= ~3;                                        \

This could use some align helper.

> +			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);                               \

I'd add these into uapi/linux/pci_regs.h:

#define PCI_CAP_ID_MASK		0x00ff
#define PCI_CAP_LIST_NEXT_MASK	0xff00

And then use FIELD_GET() to extract the fields.

> +		}                                                           \
> +		__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);


I started to wonder though if the controller drivers could simply create 
an "early" struct pci_dev & pci_bus just so they can use the normal 
accessors while the real structs are not yet created. It looks not 
much is needed from those structs to let the accessors to work.

-- 
 i.

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang]

On 2025/3/28 19:42, Ilpo Järvinen wrote:
>> 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)     \
> 
> Please put it into drivers/pci/pci.h but other than that this is certainly
> to the direction I was suggesting.

Hi Ilpo,

I'm sorry for not replying to you in time. I tried to understand your 
suggestion, modify it, and then experiment on the actual SOC platform. 
Thank you very much for your reply and patient advice.

I'm going to put it in the drivers/pci/pci.h file.

> 
> You don't need to have those priv and devfn there like that but you can
> use the args... trick (see linux/iopoll.h) and put them as last parameters
> to the macro so they can wary based on the caller needs, assuming args
> will work like this, I've not tested:
> 
> 		read_cfg(args, __pos, &val)
> 

I have modified it in the following reply, but I only modify it like 
this at present: read_cfg(args, __pos, size, &val)

> The size parameter is the most annoying one as it's in between where and
> *val arguments so args... trick won't work for it. I suggest just
> providing a function with the same signature as the related pci/access.c
> function for now.
> 

Currently DWC, CDNS, and pci.c seem to be unable to unify a common 
function to read config.

I don't quite understand what you mean here. Is the current 
dw_pcie_read_cfg, cdns_pcie_read_cfg, __pci_bus_read_config correct? 
Please look at my latest modification. If it is not correct, please 
explain it again. Thank you very much.

> A few nits related to the existing code quality of the cap parser function
> which should be addressed while we touch this function (probably in own
> patches indepedent of this code move/rearrange patch itself).
> 

I will revise it in my final submission. The following reply has been 
modified.

>> +	({                                                                  \
>> +		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)                                   \
> 
> I started to wonder if there's something for this and it turn out we've
> PCI_STD_HEADER_SIZEOF.

It has been modified.

> 
>> +				break;                                      \
>> +			__pos &= ~3;                                        \
> 
> This could use some align helper.

It has been modified.

> 
>> +			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);                               \
> 
> I'd add these into uapi/linux/pci_regs.h:

This means that you will submit, and I will submit after you?
Or should I submit this series of patches together?

> 
> #define PCI_CAP_ID_MASK		0x00ff
> #define PCI_CAP_LIST_NEXT_MASK	0xff00
> 
> And then use FIELD_GET() to extract the fields.

It has been modified.

> 
>> +		}                                                           \
>> +		__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);
> 
> 
> I started to wonder though if the controller drivers could simply create
> an "early" struct pci_dev & pci_bus just so they can use the normal
> accessors while the real structs are not yet created. It looks not
> much is needed from those structs to let the accessors to work.
> 

Here are a few questions:
1. We need to initialize some variables for pci_dev. For example, 
dev->cfg_size needs to be initialized to 4K for PCIe.

u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 start, int cap)
{
	......
	if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
		return 0;
	......


2. Create an "early" struct pci_dev & pci_bus for each SOC vendor (Qcom, 
Rockchip, etc). It leads to a lot of code that feels weird.

I still prefer the approach we are discussing now.


This is a modified patchs based on your suggestion:

diff --git a/drivers/pci/controller/cadence/pcie-cadence.c 
b/drivers/pci/controller/cadence/pcie-cadence.c
index 204e045aed8c..3991cc4c58d6 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.c
+++ b/drivers/pci/controller/cadence/pcie-cadence.c
@@ -7,6 +7,32 @@
  #include <linux/of.h>

  #include "pcie-cadence.h"
+#include "../../pci.h"
+
+static int cdns_pcie_read_cfg(void *priv, 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(cdns_pcie_read_cfg, PCI_CAPABILITY_LIST,
+				     cap, pcie);
+}
+
+u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
+{
+	return PCI_FIND_NEXT_EXT_CAPABILITY(cdns_pcie_read_cfg, 0, cap, pcie);
+}

  void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie)
  {
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..e9a9a80b1085 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -203,83 +203,25 @@ 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, 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;

-	if (cap_id == cap)
-		return cap_ptr;
+	*val = dw_pcie_read_dbi(pcie, where, size);

-	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(dw_pcie_read_cfg, PCI_CAPABILITY_LIST, cap,
+				     pcie);
  }
  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(dw_pcie_read_cfg, 0, cap, pcie);
  }
  EXPORT_SYMBOL_GPL(dw_pcie_find_ext_capability);

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 869d204a70a3..778e366ea72e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -423,28 +423,28 @@ 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)
+{
+	// If accepted, I will remove all use of "int *ttl" in a future patch.
+	return PCI_FIND_NEXT_CAP_TTL(__pci_bus_read_config, pos, cap, bus,
+				     devfn);
  }

  static u8 __pci_find_next_cap(struct pci_bus *bus, unsigned int devfn,
@@ -553,42 +553,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(__pci_bus_read_config, start, cap,
+					    dev->bus, dev->devfn);
  }
  EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2e9cf26a9ee9..68c111be521d 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -4,6 +4,65 @@

  #include <linux/pci.h>

+/* Ilpo: I'd add these into uapi/linux/pci_regs.h: */
+#define PCI_CAP_ID_MASK		0x00ff
+#define PCI_CAP_LIST_NEXT_MASK	0xff00
+
+/* Standard capability finder */
+#define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...)		\
+({									\
+	u8 __pos = (start);						\
+	int __ttl = PCI_FIND_CAP_TTL;					\
+	u16 __ent;							\
+	u8 __found_pos = 0;						\
+	u8 __id;							\
+									\
+	read_cfg(args, __pos, 1, (u32 *)&__pos);			\
+									\
+	while (__ttl--) {						\
+		if (__pos < PCI_STD_HEADER_SIZEOF)			\
+			break;						\
+		__pos = ALIGN_DOWN(__pos, 4);				\
+		read_cfg(args, __pos, 2, (u32 *)&__ent);		\
+		__id = FIELD_GET(PCI_CAP_ID_MASK, __ent);		\
+		if (__id == 0xff)					\
+			break;						\
+		if (__id == (cap)) {					\
+			__found_pos = __pos;				\
+			break;						\
+		}							\
+		__pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, __ent);	\
+	}								\
+	__found_pos;							\
+})
+
+/* Extended capability finder */
+#define PCI_FIND_NEXT_EXT_CAPABILITY(read_cfg, start, cap, args...)	\
+({									\
+	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(args, __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;							\
+})
+
  struct pcie_tlp_log;

  /* Number of possible devfns: 0.0 to 1f.7 inclusive */


Looking forward to your latest suggestions.

Best regards,
Hans

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 18088 bytes --]

On Sun, 30 Mar 2025, Hans Zhang wrote:
> On 2025/3/28 19:42, Ilpo Järvinen wrote:
> > > 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)     \
> > 
> > Please put it into drivers/pci/pci.h but other than that this is certainly
> > to the direction I was suggesting.
> 
> Hi Ilpo,
> 
> I'm sorry for not replying to you in time. I tried to understand your
> suggestion, modify it, and then experiment on the actual SOC platform. Thank
> you very much for your reply and patient advice.
> 
> I'm going to put it in the drivers/pci/pci.h file.
> 
> > 
> > You don't need to have those priv and devfn there like that but you can
> > use the args... trick (see linux/iopoll.h) and put them as last parameters
> > to the macro so they can wary based on the caller needs, assuming args
> > will work like this, I've not tested:
> > 
> > 		read_cfg(args, __pos, &val)
> > 
> 
> I have modified it in the following reply, but I only modify it like this at
> present: read_cfg(args, __pos, size, &val)
> 
> > The size parameter is the most annoying one as it's in between where and
> > *val arguments so args... trick won't work for it. I suggest just
> > providing a function with the same signature as the related pci/access.c
> > function for now.
> > 
> 
> Currently DWC, CDNS, and pci.c seem to be unable to unify a common function to
> read config.
> 
> I don't quite understand what you mean here. Is the current dw_pcie_read_cfg,
> cdns_pcie_read_cfg, __pci_bus_read_config correct? Please look at my latest
> modification. If it is not correct, please explain it again. Thank you very
> much.

This was mostly me lamenting over the parameter order which makes the args 
trick less efficient than it could be if the parameters would be in a 
different order. The patch below looked okay to me.

> > A few nits related to the existing code quality of the cap parser function
> > which should be addressed while we touch this function (probably in own
> > patches indepedent of this code move/rearrange patch itself).
> > 
> 
> I will revise it in my final submission. The following reply has been
> modified.


> > > +			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);                               \
> > 
> > I'd add these into uapi/linux/pci_regs.h:
> 
> This means that you will submit, and I will submit after you?
> Or should I submit this series of patches together?

I commented these cleanup opportunities so that you could add them to 
your series. If I'd immediately start working on area/lines you're working 
with, it would just trigger conflicts so it's better the original author 
does the improvements within the series he/she is working with. It's a lot 
less work for the maintainer that way :-).

> > #define PCI_CAP_ID_MASK		0x00ff
> > #define PCI_CAP_LIST_NEXT_MASK	0xff00
> > 
> > And then use FIELD_GET() to extract the fields.
> 
> It has been modified.
> 
> > 
> > > +		}                                                           \
> > > +		__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);
> > 
> > 
> > I started to wonder though if the controller drivers could simply create
> > an "early" struct pci_dev & pci_bus just so they can use the normal
> > accessors while the real structs are not yet created. It looks not
> > much is needed from those structs to let the accessors to work.
> > 
> 
> Here are a few questions:
> 1. We need to initialize some variables for pci_dev. For example,
> dev->cfg_size needs to be initialized to 4K for PCIe.
> 
> u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 start, int cap)
> {
> 	......
> 	if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
> 		return 0;
> 	......
> 

Sure, it would require some initialization of the struct (but not 
full init like the probe path does that does lots of setup too).

> 2. Create an "early" struct pci_dev & pci_bus for each SOC vendor (Qcom,
> Rockchip, etc). It leads to a lot of code that feels weird.

The early pci_dev+pci_bus would be created by a helper in PCI core that 
initializes what is necessary for the supported set of early core 
functionality to work. The controller drivers themselves would just call 
that function.

> I still prefer the approach we are discussing now.

I'm not saying we should immediately head toward this new idea within this 
series because it's going to be relatively big change. But it's certainly 
something that looks worth exploring so that the current chicken-egg 
problem with controller drivers could be solved.

> This is a modified patchs based on your suggestion:
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.c
> b/drivers/pci/controller/cadence/pcie-cadence.c
> index 204e045aed8c..3991cc4c58d6 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence.c
> @@ -7,6 +7,32 @@
>  #include <linux/of.h>
> 
>  #include "pcie-cadence.h"
> +#include "../../pci.h"
> +
> +static int cdns_pcie_read_cfg(void *priv, 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(cdns_pcie_read_cfg, PCI_CAPABILITY_LIST,
> +				     cap, pcie);
> +}
> +
> +u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
> +{
> +	return PCI_FIND_NEXT_EXT_CAPABILITY(cdns_pcie_read_cfg, 0, cap, pcie);
> +}
> 
>  void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie)
>  {
> 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..e9a9a80b1085 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -203,83 +203,25 @@ 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, 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;
> 
> -	if (cap_id == cap)
> -		return cap_ptr;
> +	*val = dw_pcie_read_dbi(pcie, where, size);
> 
> -	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(dw_pcie_read_cfg, PCI_CAPABILITY_LIST,
> cap,
> +				     pcie);
>  }
>  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(dw_pcie_read_cfg, 0, cap, pcie);
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_find_ext_capability);
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a3..778e366ea72e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -423,28 +423,28 @@ 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)
> +{
> +	// If accepted, I will remove all use of "int *ttl" in a future patch.
> +	return PCI_FIND_NEXT_CAP_TTL(__pci_bus_read_config, pos, cap, bus,
> +				     devfn);
>  }
> 
>  static u8 __pci_find_next_cap(struct pci_bus *bus, unsigned int devfn,
> @@ -553,42 +553,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(__pci_bus_read_config, start, cap,
> +					    dev->bus, dev->devfn);
>  }
>  EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 2e9cf26a9ee9..68c111be521d 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -4,6 +4,65 @@
> 
>  #include <linux/pci.h>

Make sure to add the necessary headers for the function/macros you're 
using so that things won't depend on the #include order in the .c file.

> 
> +/* Ilpo: I'd add these into uapi/linux/pci_regs.h: */
> +#define PCI_CAP_ID_MASK		0x00ff
> +#define PCI_CAP_LIST_NEXT_MASK	0xff00
> +
> +/* Standard capability finder */

Capability

Always use the same capitalization as the specs do.

You should probably write a kernel doc for this macro too.

I'd put these macro around where pcie_cap_has_*() forward declarations 
are so that the initial define block is not split.

> +#define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...)		\
> +({									\
> +	u8 __pos = (start);						\
> +	int __ttl = PCI_FIND_CAP_TTL;					\
> +	u16 __ent;							\
> +	u8 __found_pos = 0;						\
> +	u8 __id;							\
> +									\
> +	read_cfg(args, __pos, 1, (u32 *)&__pos);			\
> +									\
> +	while (__ttl--) {						\
> +		if (__pos < PCI_STD_HEADER_SIZEOF)			\
> +			break;						\
> +		__pos = ALIGN_DOWN(__pos, 4);				\
> +		read_cfg(args, __pos, 2, (u32 *)&__ent);		\
> +		__id = FIELD_GET(PCI_CAP_ID_MASK, __ent);		\
> +		if (__id == 0xff)					\
> +			break;						\
> +		if (__id == (cap)) {					\
> +			__found_pos = __pos;				\
> +			break;						\
> +		}							\
> +		__pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, __ent);	\
> +	}								\
> +	__found_pos;							\
> +})
> +
> +/* Extended capability finder */
> +#define PCI_FIND_NEXT_EXT_CAPABILITY(read_cfg, start, cap, args...)	\
> +({									\
> +	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(args, __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;							\
> +})
> +
>  struct pcie_tlp_log;
> 
>  /* Number of possible devfns: 0.0 to 1f.7 inclusive */
> 
> 
> Looking forward to your latest suggestions.

This generally looked good, I didn't read with a very fine comb but just 
focused on the important bits. I'll take a more detailed look once you 
make the official submission.

-- 
 i.

Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang]

On 2025/4/1 00:39, Ilpo Järvinen wrote:
>>>> +			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);                               \
>>>
>>> I'd add these into uapi/linux/pci_regs.h:
>>
>> This means that you will submit, and I will submit after you?
>> Or should I submit this series of patches together?
> 
> I commented these cleanup opportunities so that you could add them to
> your series. If I'd immediately start working on area/lines you're working
> with, it would just trigger conflicts so it's better the original author
> does the improvements within the series he/she is working with. It's a lot
> less work for the maintainer that way :-).
> 

Hi Ilpo,

Thanks your for reply. Thank you so much for your comments.

>>> I started to wonder though if the controller drivers could simply create
>>> an "early" struct pci_dev & pci_bus just so they can use the normal
>>> accessors while the real structs are not yet created. It looks not
>>> much is needed from those structs to let the accessors to work.
>>>
>>
>> Here are a few questions:
>> 1. We need to initialize some variables for pci_dev. For example,
>> dev->cfg_size needs to be initialized to 4K for PCIe.
>>
>> u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 start, int cap)
>> {
>> 	......
>> 	if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
>> 		return 0;
>> 	......
>>
> 
> Sure, it would require some initialization of the struct (but not
> full init like the probe path does that does lots of setup too).
> 
>> 2. Create an "early" struct pci_dev & pci_bus for each SOC vendor (Qcom,
>> Rockchip, etc). It leads to a lot of code that feels weird.
> 
> The early pci_dev+pci_bus would be created by a helper in PCI core that
> initializes what is necessary for the supported set of early core
> functionality to work. The controller drivers themselves would just call
> that function.
> 

Ok, got it.

>> I still prefer the approach we are discussing now.
> 
> I'm not saying we should immediately head toward this new idea within this
> series because it's going to be relatively big change. But it's certainly
> something that looks worth exploring so that the current chicken-egg
> problem with controller drivers could be solved.
> 

Ok, I hope to have the opportunity to participate in the discussion 
together in the future.

>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 2e9cf26a9ee9..68c111be521d 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -4,6 +4,65 @@
>>
>>   #include <linux/pci.h>
> 
> Make sure to add the necessary headers for the function/macros you're
> using so that things won't depend on the #include order in the .c file.
> 

Will do.

>>
>> +/* Ilpo: I'd add these into uapi/linux/pci_regs.h: */
>> +#define PCI_CAP_ID_MASK		0x00ff
>> +#define PCI_CAP_LIST_NEXT_MASK	0xff00
>> +
>> +/* Standard capability finder */
> 
> Capability
> 
> Always use the same capitalization as the specs do.
> 

Will change.

> You should probably write a kernel doc for this macro too.
> 

Will do.

> I'd put these macro around where pcie_cap_has_*() forward declarations
> are so that the initial define block is not split.
> 

Will change.

>> +#define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...)		\
>> +({									\
>> +	u8 __pos = (start);						\
>> +	int __ttl = PCI_FIND_CAP_TTL;					\
>> +	u16 __ent;							\
>> +	u8 __found_pos = 0;						\
>> +	u8 __id;							\
>> +									\
>> +	read_cfg(args, __pos, 1, (u32 *)&__pos);			\
>> +									\
>> +	while (__ttl--) {						\
>> +		if (__pos < PCI_STD_HEADER_SIZEOF)			\
>> +			break;						\
>> +		__pos = ALIGN_DOWN(__pos, 4);				\
>> +		read_cfg(args, __pos, 2, (u32 *)&__ent);		\
>> +		__id = FIELD_GET(PCI_CAP_ID_MASK, __ent);		\
>> +		if (__id == 0xff)					\
>> +			break;						\
>> +		if (__id == (cap)) {					\
>> +			__found_pos = __pos;				\
>> +			break;						\
>> +		}							\
>> +		__pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, __ent);	\
>> +	}								\
>> +	__found_pos;							\
>> +})
>> +
>> +/* Extended capability finder */
>> +#define PCI_FIND_NEXT_EXT_CAPABILITY(read_cfg, start, cap, args...)	\
>> +({									\
>> +	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(args, __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;							\
>> +})
>> +
>>   struct pcie_tlp_log;
>>
>>   /* Number of possible devfns: 0.0 to 1f.7 inclusive */
>>
>>
>> Looking forward to your latest suggestions.
> 
> This generally looked good, I didn't read with a very fine comb but just
> focused on the important bits. I'll take a more detailed look once you
> make the official submission.

Ok, I'm going to prepare the next version of patch. I hope you can 
review it again. Thank you very much


Best regards,
Hans

[v6 4/5] PCI: cadence: Use cdns_pcie_find_*capability to avoid hardcode.

[Hans Zhang]

The PCIe capability/extended capability offsets are not guaranteed to be
the same across all SoCs integrating the Cadence PCIe IP. Hence, use the
cdns_pcie_find_{ext}_capability() APIs for finding them.

This avoids hardcoding the offsets in the driver.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
Changes since v5:

- None

Changes since v4:
https://lore.kernel.org/linux-pci/20250321101710.371480-5-18255117159@163.com/

- The patch subject and commit message were modified.
---
 .../pci/controller/cadence/pcie-cadence-ep.c  | 40 +++++++++++--------
 drivers/pci/controller/cadence/pcie-cadence.h |  5 ---
 2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
index e0cc4560dfde..aea53ddcaf9b 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
@@ -19,12 +19,13 @@
 
 static u8 cdns_pcie_get_fn_from_vfn(struct cdns_pcie *pcie, u8 fn, u8 vfn)
 {
-	u32 cap = CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET;
 	u32 first_vf_offset, stride;
+	u16 cap;
 
 	if (vfn == 0)
 		return fn;
 
+	cap = cdns_pcie_find_ext_capability(pcie, PCI_EXT_CAP_ID_SRIOV);
 	first_vf_offset = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_SRIOV_VF_OFFSET);
 	stride = cdns_pcie_ep_fn_readw(pcie, fn, cap +  PCI_SRIOV_VF_STRIDE);
 	fn = fn + first_vf_offset + ((vfn - 1) * stride);
@@ -36,10 +37,11 @@ static int cdns_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn,
 				     struct pci_epf_header *hdr)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
-	u32 cap = CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET;
 	struct cdns_pcie *pcie = &ep->pcie;
 	u32 reg;
+	u16 cap;
 
+	cap = cdns_pcie_find_ext_capability(pcie, PCI_EXT_CAP_ID_SRIOV);
 	if (vfn > 1) {
 		dev_err(&epc->dev, "Only Virtual Function #1 has deviceID\n");
 		return -EINVAL;
@@ -224,9 +226,10 @@ static int cdns_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, u8 mmc)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
-	u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
 	u16 flags;
+	u8 cap;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
 	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
 
 	/*
@@ -246,9 +249,10 @@ static int cdns_pcie_ep_get_msi(struct pci_epc *epc, u8 fn, u8 vfn)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
-	u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
 	u16 flags, mme;
+	u8 cap;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
 	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
 
 	/* Validate that the MSI feature is actually enabled. */
@@ -269,9 +273,10 @@ static int cdns_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
-	u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
 	u32 val, reg;
+	u8 cap;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
 	func_no = cdns_pcie_get_fn_from_vfn(pcie, func_no, vfunc_no);
 
 	reg = cap + PCI_MSIX_FLAGS;
@@ -290,9 +295,10 @@ static int cdns_pcie_ep_set_msix(struct pci_epc *epc, u8 fn, u8 vfn,
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
-	u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
 	u32 val, reg;
+	u8 cap;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
 	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
 
 	reg = cap + PCI_MSIX_FLAGS;
@@ -379,11 +385,11 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn, u8 vfn,
 				     u8 interrupt_num)
 {
 	struct cdns_pcie *pcie = &ep->pcie;
-	u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
 	u16 flags, mme, data, data_mask;
-	u8 msi_count;
 	u64 pci_addr, pci_addr_mask = 0xff;
+	u8 msi_count, cap;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
 	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
 
 	/* Check whether the MSI feature has been enabled by the PCI host. */
@@ -431,14 +437,14 @@ static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, u8 vfn,
 				    u32 *msi_addr_offset)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
-	u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
 	struct cdns_pcie *pcie = &ep->pcie;
 	u64 pci_addr, pci_addr_mask = 0xff;
 	u16 flags, mme, data, data_mask;
-	u8 msi_count;
+	u8 msi_count, cap;
 	int ret;
 	int i;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
 	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
 
 	/* Check whether the MSI feature has been enabled by the PCI host. */
@@ -481,16 +487,16 @@ static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, u8 vfn,
 static int cdns_pcie_ep_send_msix_irq(struct cdns_pcie_ep *ep, u8 fn, u8 vfn,
 				      u16 interrupt_num)
 {
-	u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
 	u32 tbl_offset, msg_data, reg;
 	struct cdns_pcie *pcie = &ep->pcie;
 	struct pci_epf_msix_tbl *msix_tbl;
 	struct cdns_pcie_epf *epf;
 	u64 pci_addr_mask = 0xff;
 	u64 msg_addr;
+	u8 bir, cap;
 	u16 flags;
-	u8 bir;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
 	epf = &ep->epf[fn];
 	if (vfn > 0)
 		epf = &epf->epf[vfn - 1];
@@ -564,7 +570,9 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
 	int max_epfs = sizeof(epc->function_num_map) * 8;
 	int ret, epf, last_fn;
 	u32 reg, value;
+	u8 cap;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_EXP);
 	/*
 	 * BIT(0) is hardwired to 1, hence function 0 is always enabled
 	 * and can't be disabled anyway.
@@ -588,12 +596,10 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
 				continue;
 
 			value = cdns_pcie_ep_fn_readl(pcie, epf,
-					CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET +
-					PCI_EXP_DEVCAP);
+						      cap + PCI_EXP_DEVCAP);
 			value &= ~PCI_EXP_DEVCAP_FLR;
-			cdns_pcie_ep_fn_writel(pcie, epf,
-					CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET +
-					PCI_EXP_DEVCAP, value);
+			cdns_pcie_ep_fn_writel(pcie, epf, cap + PCI_EXP_DEVCAP,
+					       value);
 		}
 	}
 
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 6f4981fccb94..d0fcf1b3549c 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -125,11 +125,6 @@
  */
 #define CDNS_PCIE_EP_FUNC_BASE(fn)	(((fn) << 12) & GENMASK(19, 12))
 
-#define CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET	0x90
-#define CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET	0xb0
-#define CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET	0xc0
-#define CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET	0x200
-
 /*
  * Endpoint PF Registers
  */
-- 
2.25.1

[v6 5/5] MAINTAINERS: Add entry for PCI host controller helpers

[Hans Zhang]

Add maintenance entry for the newly introduced PCI host controller helper
functions. These functions provide common infrastructure for capability
scanning and other shared operations across PCI host controller drivers.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 00e94bec401e..9b3236704b83 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18119,6 +18119,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/pci/intel,ixp4xx-pci.yaml
 F:	drivers/pci/controller/pci-ixp4xx.c
 
+PCI DRIVER FOR HELPER FUNCTIONS
+M:	Hans Zhang <18255117159@163.com>
+L:	linux-pci@vger.kernel.org
+S:	Maintained
+F:	drivers/pci/controller/pci-host-helpers.c
+
 PCI DRIVER FOR INTEL VOLUME MANAGEMENT DEVICE (VMD)
 M:	Nirmal Patel <nirmal.patel@linux.intel.com>
 R:	Jonathan Derrick <jonathan.derrick@linux.dev>
-- 
2.25.1

Re: [v6 5/5] MAINTAINERS: Add entry for PCI host controller helpers

[Manivannan Sadhasivam]

On Mon, Mar 24, 2025 at 12:48:52AM +0800, Hans Zhang wrote:
> Add maintenance entry for the newly introduced PCI host controller helper
> functions. These functions provide common infrastructure for capability
> scanning and other shared operations across PCI host controller drivers.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>  MAINTAINERS | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 00e94bec401e..9b3236704b83 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18119,6 +18119,12 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/pci/intel,ixp4xx-pci.yaml
>  F:	drivers/pci/controller/pci-ixp4xx.c
>  
> +PCI DRIVER FOR HELPER FUNCTIONS
> +M:	Hans Zhang <18255117159@163.com>
> +L:	linux-pci@vger.kernel.org
> +S:	Maintained
> +F:	drivers/pci/controller/pci-host-helpers.c

I don't see much value in maintaining this file separately as these helpers are
not going to evolve much, thus not needing separate maintenance.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [v6 5/5] MAINTAINERS: Add entry for PCI host controller helpers

[Hans Zhang]

On 2025/3/28 01:01, Manivannan Sadhasivam wrote:
> On Mon, Mar 24, 2025 at 12:48:52AM +0800, Hans Zhang wrote:
>> Add maintenance entry for the newly introduced PCI host controller helper
>> functions. These functions provide common infrastructure for capability
>> scanning and other shared operations across PCI host controller drivers.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>>   MAINTAINERS | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 00e94bec401e..9b3236704b83 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -18119,6 +18119,12 @@ S:	Maintained
>>   F:	Documentation/devicetree/bindings/pci/intel,ixp4xx-pci.yaml
>>   F:	drivers/pci/controller/pci-ixp4xx.c
>>   
>> +PCI DRIVER FOR HELPER FUNCTIONS
>> +M:	Hans Zhang <18255117159@163.com>
>> +L:	linux-pci@vger.kernel.org
>> +S:	Maintained
>> +F:	drivers/pci/controller/pci-host-helpers.c
> 
> I don't see much value in maintaining this file separately as these helpers are
> not going to evolve much, thus not needing separate maintenance.
> 

Hi Mani,

Thanks your for reply. Will delete.

Best regards,
Hans