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

[Hans Zhang]

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

Hi Ilpo,

Thanks your for reply. Will change.


> > search for the PCIe capabilities, make use of them in the CDNS driver.
> >
> > Signed-off-by: Hans Zhang <18255117159@xxxxxxx>
> > ---
> > Changes since v5:
> > https://lore.kernel.org/linux-pci/20250321163803.391056-4-18255117159@xxxxxxx
> >
> > - 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@xxxxxxx/

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