Re: [v6 1/5] PCI: Introduce generic capability search functions
From: Hans Zhang
Date: Mon Mar 24 2025 - 22:59:33 EST
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