Message-ID: <26dcba54-93c1-dda4-c5e2-e324e9d50b09@linux.intel.com>
Date: Tue, 25 Mar 2025 13:15:49 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Hans Zhang <18255117159@....com>
cc: lpieralisi@...nel.org, kw@...ux.com, manivannan.sadhasivam@...aro.org, 
    robh@...nel.org, bhelgaas@...gle.com, jingoohan1@...il.com, 
    thomas.richard@...tlin.com, linux-pci@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for
 finding the capabilities

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
> 

-- 
 i.

Powered by blists - more mailing lists