| Message ID | 20250323164852.430546-4-18255117159@163.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | Introduce generic capability search functions | expand |
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
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
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 >
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 >
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, >
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
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 >
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
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 >
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
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
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 >
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
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
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.
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
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.
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
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,
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(+)