| Message ID | 20150804215457.9189.27595.stgit@bhelgaas-glaptop2.roam.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Bjorn, On 04/08/15 22:54, Bjorn Helgaas wrote: > Previously there was no way to specify the MSI controller when creating a > new PCI root bus, so we had to create the bus, set its MSI controller, then > scan the bus. With the new pci_scan_root_bus_msi() interface, we can > specify the MSI controller up front and get rid of that intermediate step. > > Look up the MSI controller first, then use pci_scan_root_bus_msi() to > create and scan the root PCI bus. I'm wondering about these XGene patches. With the code that is queued for v4.3 in tip/irq/core, the X-Gene MSI driver doesn't export a struct msi_controller anymore, and entirely relies on IRQ domains to identify to be matched with the actual PCI driver. Do you intend this as a cleanup until everything lands in mainline? At that point, we'd be able to remove all traces of struct msi_controller from this driver. Alternatively, we could ask tglx to add an extra patch to the existing queue in order to clean up pci-xgene.c (nuking the whole xgene_pcie_msi_enable function). Thoughts? M.
On Thu, Aug 6, 2015 at 11:26 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > Hi Bjorn, > > On 04/08/15 22:54, Bjorn Helgaas wrote: >> Previously there was no way to specify the MSI controller when creating a >> new PCI root bus, so we had to create the bus, set its MSI controller, then >> scan the bus. With the new pci_scan_root_bus_msi() interface, we can >> specify the MSI controller up front and get rid of that intermediate step. >> >> Look up the MSI controller first, then use pci_scan_root_bus_msi() to >> create and scan the root PCI bus. > > I'm wondering about these XGene patches. > > With the code that is queued for v4.3 in tip/irq/core, the X-Gene MSI > driver doesn't export a struct msi_controller anymore, and entirely > relies on IRQ domains to identify to be matched with the actual PCI driver. > > Do you intend this as a cleanup until everything lands in mainline? At > that point, we'd be able to remove all traces of struct msi_controller > from this driver. > > Alternatively, we could ask tglx to add an extra patch to the existing > queue in order to clean up pci-xgene.c (nuking the whole > xgene_pcie_msi_enable function). > > Thoughts? In pcie-altera driver, it uses pci_scan_root_bus() and it doesn't call to of_pci_find_msi_chip_by_node() to retrieve msi_controller struct as well. But, the msi domain stuff is still working. Should I change it to pci_scan_root_bus_msi()? Regards Ley Foon
On 06/08/15 17:41, Ley Foon Tan wrote: > On Thu, Aug 6, 2015 at 11:26 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> Hi Bjorn, >> >> On 04/08/15 22:54, Bjorn Helgaas wrote: >>> Previously there was no way to specify the MSI controller when creating a >>> new PCI root bus, so we had to create the bus, set its MSI controller, then >>> scan the bus. With the new pci_scan_root_bus_msi() interface, we can >>> specify the MSI controller up front and get rid of that intermediate step. >>> >>> Look up the MSI controller first, then use pci_scan_root_bus_msi() to >>> create and scan the root PCI bus. >> >> I'm wondering about these XGene patches. >> >> With the code that is queued for v4.3 in tip/irq/core, the X-Gene MSI >> driver doesn't export a struct msi_controller anymore, and entirely >> relies on IRQ domains to identify to be matched with the actual PCI driver. >> >> Do you intend this as a cleanup until everything lands in mainline? At >> that point, we'd be able to remove all traces of struct msi_controller >> from this driver. >> >> Alternatively, we could ask tglx to add an extra patch to the existing >> queue in order to clean up pci-xgene.c (nuking the whole >> xgene_pcie_msi_enable function). >> >> Thoughts? > > In pcie-altera driver, it uses pci_scan_root_bus() and it doesn't call > to of_pci_find_msi_chip_by_node() to retrieve msi_controller struct as > well. > But, the msi domain stuff is still working. Should I change it to > pci_scan_root_bus_msi()? If you're not using struct msi_controller, I don't see a reason to use pci_scan_root_bus_msi(). The MSI irq domain patches are queued for 4.3, and I don't believe your driver will be merged before that. Eventually, I'd like to kill msi_controller entirely. It is completely redundant with the use of irq domains. Thanks, M.
On Fri, Aug 7, 2015 at 12:53 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 06/08/15 17:41, Ley Foon Tan wrote: >> On Thu, Aug 6, 2015 at 11:26 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >>> Hi Bjorn, >>> >>> On 04/08/15 22:54, Bjorn Helgaas wrote: >>>> Previously there was no way to specify the MSI controller when creating a >>>> new PCI root bus, so we had to create the bus, set its MSI controller, then >>>> scan the bus. With the new pci_scan_root_bus_msi() interface, we can >>>> specify the MSI controller up front and get rid of that intermediate step. >>>> >>>> Look up the MSI controller first, then use pci_scan_root_bus_msi() to >>>> create and scan the root PCI bus. >>> >>> I'm wondering about these XGene patches. >>> >>> With the code that is queued for v4.3 in tip/irq/core, the X-Gene MSI >>> driver doesn't export a struct msi_controller anymore, and entirely >>> relies on IRQ domains to identify to be matched with the actual PCI driver. >>> >>> Do you intend this as a cleanup until everything lands in mainline? At >>> that point, we'd be able to remove all traces of struct msi_controller >>> from this driver. >>> >>> Alternatively, we could ask tglx to add an extra patch to the existing >>> queue in order to clean up pci-xgene.c (nuking the whole >>> xgene_pcie_msi_enable function). >>> >>> Thoughts? >> >> In pcie-altera driver, it uses pci_scan_root_bus() and it doesn't call >> to of_pci_find_msi_chip_by_node() to retrieve msi_controller struct as >> well. >> But, the msi domain stuff is still working. Should I change it to >> pci_scan_root_bus_msi()? > > If you're not using struct msi_controller, I don't see a reason to use > pci_scan_root_bus_msi(). The MSI irq domain patches are queued for 4.3, > and I don't believe your driver will be merged before that. > > Eventually, I'd like to kill msi_controller entirely. It is completely > redundant with the use of irq domains. > Got it. It doesn't require struct msi_controller. Thanks. Regards Ley Foon
Hi Marc, On Thu, Aug 06, 2015 at 04:26:10PM +0100, Marc Zyngier wrote: > On 04/08/15 22:54, Bjorn Helgaas wrote: > > Previously there was no way to specify the MSI controller when creating a > > new PCI root bus, so we had to create the bus, set its MSI controller, then > > scan the bus. With the new pci_scan_root_bus_msi() interface, we can > > specify the MSI controller up front and get rid of that intermediate step. > > > > Look up the MSI controller first, then use pci_scan_root_bus_msi() to > > create and scan the root PCI bus. > > I'm wondering about these XGene patches. > > With the code that is queued for v4.3 in tip/irq/core, the X-Gene MSI > driver doesn't export a struct msi_controller anymore, and entirely > relies on IRQ domains to identify to be matched with the actual PCI driver. > > Do you intend this as a cleanup until everything lands in mainline? At > that point, we'd be able to remove all traces of struct msi_controller > from this driver. I haven't been following the IRQ domain stuff or tip/irq/core, so I really don't know how this relates to that. I took a look, and I see 8d63bc7beaee ("PCI/MSI: pci-xgene-msi: Get rid of struct msi_controller"), which removes an msi_controller pointer from pci-xgene-msi.c, but I don't see any pci-xgene.c changes in tip/irq/core. Are you saying that xgene_pcie_msi_enable() will go away eventually? And the OF "msi-parent" lookup will go away, or at least be moved elsewhere? We currently have: xgene_pcie_probe_bridge(...) { ... bus = pci_create_root_bus(...); xgene_pcie_msi_enable(...); pci_scan_child_bus(bus); ... } I'd like to get rid of that arch-specific MSI enable stuff because then we can use a higher level interface, e.g., pci_scan_root_bus(), and make the X-Gene code slightly simpler. > Alternatively, we could ask tglx to add an extra patch to the existing > queue in order to clean up pci-xgene.c (nuking the whole > xgene_pcie_msi_enable function). Ah, I guess you *are* saying that xgene_pcie_msi_enable() will go away eventually. I don't know how to do that, but apparently you do, so I'll just drop these X-Gene-related patches for now. Bjorn
On Mon, Aug 10, 2015 at 3:04 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > Hi Marc, > > On Thu, Aug 06, 2015 at 04:26:10PM +0100, Marc Zyngier wrote: >> On 04/08/15 22:54, Bjorn Helgaas wrote: >> > Previously there was no way to specify the MSI controller when creating a >> > new PCI root bus, so we had to create the bus, set its MSI controller, then >> > scan the bus. With the new pci_scan_root_bus_msi() interface, we can >> > specify the MSI controller up front and get rid of that intermediate step. >> > >> > Look up the MSI controller first, then use pci_scan_root_bus_msi() to >> > create and scan the root PCI bus. >> >> I'm wondering about these XGene patches. >> >> With the code that is queued for v4.3 in tip/irq/core, the X-Gene MSI >> driver doesn't export a struct msi_controller anymore, and entirely >> relies on IRQ domains to identify to be matched with the actual PCI driver. >> >> Do you intend this as a cleanup until everything lands in mainline? At >> that point, we'd be able to remove all traces of struct msi_controller >> from this driver. > > I haven't been following the IRQ domain stuff or tip/irq/core, so I > really don't know how this relates to that. I took a look, and I see > 8d63bc7beaee ("PCI/MSI: pci-xgene-msi: Get rid of struct > msi_controller"), which removes an msi_controller pointer from > pci-xgene-msi.c, but I don't see any pci-xgene.c changes in > tip/irq/core. > > Are you saying that xgene_pcie_msi_enable() will go away eventually? > And the OF "msi-parent" lookup will go away, or at least be moved > elsewhere? We currently have: > > xgene_pcie_probe_bridge(...) > { > ... > bus = pci_create_root_bus(...); > xgene_pcie_msi_enable(...); > pci_scan_child_bus(bus); > ... > } > > I'd like to get rid of that arch-specific MSI enable stuff because > then we can use a higher level interface, e.g., pci_scan_root_bus(), > and make the X-Gene code slightly simpler. > >> Alternatively, we could ask tglx to add an extra patch to the existing >> queue in order to clean up pci-xgene.c (nuking the whole >> xgene_pcie_msi_enable function). > > Ah, I guess you *are* saying that xgene_pcie_msi_enable() will go away > eventually. I don't know how to do that, but apparently you do, so > I'll just drop these X-Gene-related patches for now. Hi Bjorn, I tested PCIe MSI with Marc's patch on X-Gene platform and it was working fine. xgene_pcie_msi_enable will go away. I will post clean up patch to remove these msi_controller traces for X-Gene. > > Bjorn
diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c index 3cb58a3..514f41b 100644 --- a/drivers/pci/host/pci-xgene.c +++ b/drivers/pci/host/pci-xgene.c @@ -68,6 +68,7 @@ struct xgene_pcie_port { struct device_node *node; struct device *dev; + struct msi_controller *msi; struct clk *clk; void __iomem *csr_base; void __iomem *cfg_base; @@ -501,8 +502,7 @@ static int xgene_pcie_setup(struct xgene_pcie_port *port, return 0; } -static int xgene_pcie_msi_enable(struct xgene_pcie_port *port, - struct pci_bus *bus) +static int xgene_pcie_msi_enable(struct xgene_pcie_port *port) { struct device_node *msi_node; @@ -510,12 +510,12 @@ static int xgene_pcie_msi_enable(struct xgene_pcie_port *port, if (!msi_node) return -ENODEV; - bus->msi = of_pci_find_msi_chip_by_node(msi_node); - if (!bus->msi) + port->msi = of_pci_find_msi_chip_by_node(msi_node); + if (!port->msi) return -ENODEV; of_node_put(msi_node); - bus->msi->dev = &port->dev; + port->msi->dev = &port->dev; return 0; } @@ -554,16 +554,15 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev) if (ret) return ret; - bus = pci_create_root_bus(&pdev->dev, 0, - &xgene_pcie_ops, port, &res); - if (!bus) - return -ENOMEM; - if (IS_ENABLED(CONFIG_PCI_MSI)) - if (xgene_pcie_msi_enable(port, bus)) + if (xgene_pcie_msi_enable(port)) dev_info(port->dev, "failed to enable MSI\n"); - pci_scan_child_bus(bus); + bus = pci_scan_root_bus_msi(&pdev->dev, 0, &xgene_pcie_ops, port, + &res, port->msi); + if (!bus) + return -ENOMEM; + pci_assign_unassigned_bus_resources(bus); pci_bus_add_devices(bus);
Previously there was no way to specify the MSI controller when creating a new PCI root bus, so we had to create the bus, set its MSI controller, then scan the bus. With the new pci_scan_root_bus_msi() interface, we can specify the MSI controller up front and get rid of that intermediate step. Look up the MSI controller first, then use pci_scan_root_bus_msi() to create and scan the root PCI bus. Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/host/pci-xgene.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)