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

Commit Message

Hans Zhang March 23, 2025, 4:48 p.m. UTC

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 <[e-mail]>
---
Changes since v5:
https://lore.kernel.org/linux-pci/[e-mail]

- 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/[e-mail]

- 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

Comments

Ilpo Järvinen March 24, 2025, 1:28 p.m. UTC | #1

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 <[e-mail]>
> ---
> Changes since v5:
> https://lore.kernel.org/linux-pci/[e-mail]
> 
> - 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/[e-mail]
> 
> - 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 <[e-mail]>
> + */
> +
> +#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 */
>

Hans Zhang March 24, 2025, 2:39 p.m. UTC | #2

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 <[e-mail]>
>> ---
>> Changes since v5:
>> https://lore.kernel.org/linux-pci/[e-mail]
>>
>> - 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/[e-mail]
>>
>> - 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 <[e-mail]>
>> + */
>> +
>> +#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

Ilpo Järvinen March 24, 2025, 2:52 p.m. UTC | #3

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 <[e-mail]>
> > > ---
> > > Changes since v5:
> > > https://lore.kernel.org/linux-pci/[e-mail]
> > > 
> > > - 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/[e-mail]
> > > 
> > > - 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 <[e-mail]>
> > > + */
> > > +
> > > +#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
>

Hans Zhang March 25, 2025, 2:58 a.m. UTC | #4

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

Manivannan Sadhasivam March 27, 2025, 4:57 p.m. UTC | #5

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 <[e-mail]>
> ---
> Changes since v5:
> https://lore.kernel.org/linux-pci/[e-mail]
> 
> - 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/[e-mail]
> 
> - 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

Manivannan Sadhasivam March 27, 2025, 4:58 p.m. UTC | #6

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 <[e-mail]>
> ---
> Changes since v5:
> https://lore.kernel.org/linux-pci/[e-mail]
> 
> - 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/[e-mail]
> 
> - 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 <[e-mail]>
> + */
> +
> +#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
>

Hans Zhang March 28, 2025, 9:41 a.m. UTC | #7

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

Hans Zhang March 28, 2025, 9:42 a.m. UTC | #8

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

Patch

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 <[e-mail]>
+ */
+
+#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 */