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

From: Hans Zhang
Date: Tue Mar 25 2025 - 08:19:03 EST



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