On Thu, Jun 29, 2023 at 05:56:31PM -0500, Bjorn Helgaas wrote:
> On Thu, Jun 29, 2023 at 10:19:47AM -0700, Lizhi Hou wrote:
> > The PCI endpoint device such as Xilinx Alveo PCI card maps the register
> > spaces from multiple hardware peripherals to its PCI BAR. Normally,
> > the PCI core discovers devices and BARs using the PCI enumeration process.
> > There is no infrastructure to discover the hardware peripherals that are
> > present in a PCI device, and which can be accessed through the PCI BARs.
>
> IIUC this is basically a multi-function device except that instead of
> each device being a separate PCI Function, they all appear in a single
> Function. That would mean all the devices share the same config space
> so a single PCI Command register controls all of them, they all share
> the same IRQs (either INTx or MSI/MSI-X), any MMIO registers are likely
> in a shared BAR, etc., right?
>
> Obviously PCI enumeration only sees the single Function and binds a
> single driver to it. But IIUC, you want to use existing drivers for
> each of these sub-devices, so this series adds a DT node for the
> single Function (using the quirks that call of_pci_make_dev_node()).
> And I assume that when the PCI driver claims the single Function, it
> will use that DT node to add platform devices, and those existing
> drivers can claim those?
>
> I don't see the PCI driver for the single Function in this series. Is
> that coming? Is this series useful without it?
>
> > Apparently, the device tree framework requires a device tree node for the
> > PCI device. Thus, it can generate the device tree nodes for hardware
> > peripherals underneath. Because PCI is self discoverable bus, there might
> > not be a device tree node created for PCI devices. Furthermore, if the PCI
> > device is hot pluggable, when it is plugged in, the device tree nodes for
> > its parent bridges are required. Add support to generate device tree node
> > for PCI bridges.
>
> Can you remind me why hot-adding a PCI device requires DT nodes for
> parent bridges? I don't think we have those today, so maybe the DT
> node for the PCI device requires a DT parent? How far up does that
> go? From this patch, I guess a Root Port would be the top DT node on
> a PCIe system, since that's the top PCI-to-PCI bridge?
>
> This patch adds a DT node for *every* PCI bridge in the system. We
> only actually need that node for these unusual devices. Is there some
> way the driver for the single PCI Function could add that node when it
> is needed? Sorry if you've answered this in the past; maybe the
> answer could be in the commit log or a code comment in case somebody
> else wonders.
>
> > @@ -340,6 +340,8 @@ void pci_bus_add_device(struct pci_dev *dev)
> > */
> > pcibios_bus_add_device(dev);
> > pci_fixup_device(pci_fixup_final, dev);
> > + if (pci_is_bridge(dev))
> > + of_pci_make_dev_node(dev);
>
> It'd be nice to have a clue here about why we need this, since this is
> executed for *every* system, even ACPI platforms that typically don't
> use OF things.
>
> > pci_create_sysfs_dev_files(dev);
> > pci_proc_attach_device(dev);
> > pci_bridge_d3_update(dev);
> > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > index 2c25f4fa0225..9786ae407948 100644
> > --- a/drivers/pci/of.c
> > +++ b/drivers/pci/of.c
> > @@ -487,6 +487,15 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *
> > } else {
> > /* We found a P2P bridge, check if it has a node */
> > ppnode = pci_device_to_OF_node(ppdev);
> > +#if IS_ENABLED(CONFIG_PCI_DYNAMIC_OF_NODES)
>
> I would use plain #ifdef here instead of IS_ENABLED(), as you did in
> pci.h below. IS_ENABLED() is true if the Kconfig symbol is set to
> either "y" or "m".
Actually, IS_ENABLED() with a C 'if' rather than a preprocessor #if
would work here and is preferred.
But this code and the "dynamic" property needs more discussion.
Rob
