Message-ID: <ddabf340-a00f-75b1-2b6b-d9ab550a984f@linux.intel.com>
Date: Tue, 25 Mar 2025 17:18:29 +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/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?

Hi Hans,

No, I don't have further ideas at this point, sorry. It seems it isn't 
realistic without something more substantial that currently isn't there.

This lack of way to have a generic way to read the config before the main 
struct are instanciated by the PCI core seems to be the limitation that 
hinders sharing code between controller drivers and it would have been 
nice to address it.

But please still make the capability list parsing code common, it should 
be relatively straightforward using a macro which can take different read 
functions similar to read_poll_timeout. That will avoid at least some 
amount of code duplication.

Thanks for trying to come up with a solution (or thinking enough to say 
it doesn't work)!

> > 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
> 

-- 
 i.

Powered by blists - more mailing lists