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

From: Hans Zhang
Date: Tue Mar 25 2025 - 10:48:53 EST




On 2025/3/25 20:16, Hans Zhang wrote:
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.


Hi Ilpo,

Another question comes to mind:
If working in EP mode, devm_pci_alloc_host_bridge will not be executed and there will be no struct pci_host_bridge.

Don't know if you have anything to add?

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