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

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

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

I'm not sure what you meant?

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

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

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

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

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

> https://patchwork.kernel.org/project/linux-pci/patch/20250308133903.322216-1-18255117159@163.com/
> 
> The original function is in the following file:
> drivers/pci/controller/dwc/pcie-designware.c
> u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
> u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
> 
> CDNS has the same need to find the offset of the capability.
> 
> > > +
> > >   void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie)
> > >   {
> > >   	u32 delay = 0x3;
> > > diff --git a/drivers/pci/controller/cadence/pcie-cadence.h
> > > b/drivers/pci/controller/cadence/pcie-cadence.h
> > > index f5eeff834ec1..6f4981fccb94 100644
> > > --- a/drivers/pci/controller/cadence/pcie-cadence.h
> > > +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> > > @@ -557,6 +557,9 @@ static inline int cdns_pcie_ep_setup(struct
> > > cdns_pcie_ep *ep)
> > >   }
> > >   #endif
> > >   +u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap);
> > > +u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap);
> > > +
> > >   void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie);
> > >     void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 busnr,
> > > u8 fn,
> > > 
> > 
> 
> Best regards,
> Hans
> 

-- 
 i.