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

[Hans Zhang]

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