On Wed, Oct 25, 2017 at 4:16 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > On Wed, Oct 25, 2017 at 11:40:47AM -0700, Scott Branden wrote: >> Hi Bjorn, >> >> >> On 17-10-25 10:23 AM, Bjorn Helgaas wrote: >> >[+cc Ray, Scott, Jon] >> > >> >On Wed, Oct 25, 2017 at 11:28:07AM -0400, Jim Quinlan wrote: >> >>On Tue, Oct 24, 2017 at 2:57 PM, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote: >> >>>Hi Jim, >> >>> >> >>>On 10/24/2017 11:15 AM, Jim Quinlan wrote: >> >>>>This commit adds MSI to the Broadcom STB PCIe host controller. It does >> >>>>not add MSIX since that functionality is not in the HW. The MSI >> >>>>controller is physically located within the PCIe block, however, there >> >>>>is no reason why the MSI controller could not be moved elsewhere in >> >>>>the future. >> >>>> >> >>>>Since the internal Brcmstb MSI controller is intertwined with the PCIe >> >>>>controller, it is not its own platform device but rather part of the >> >>>>PCIe platform device. >> >>>> >> >>>>Signed-off-by: Jim Quinlan <jim2101024@xxxxxxxxx> >> >>>>--- >> >>>> drivers/pci/host/Kconfig | 12 ++ >> >>>> drivers/pci/host/Makefile | 1 + >> >>>> drivers/pci/host/pci-brcmstb-msi.c | 318 +++++++++++++++++++++++++++++++++++++ >> >>>> drivers/pci/host/pci-brcmstb.c | 72 +++++++-- >> >>>> drivers/pci/host/pci-brcmstb.h | 26 +++ >> >>>> 5 files changed, 419 insertions(+), 10 deletions(-) >> >>>> create mode 100644 drivers/pci/host/pci-brcmstb-msi.c >> >>>> >> >>>>diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig >> >>>>index b9b4f11..54aa5d2 100644 >> >>>>--- a/drivers/pci/host/Kconfig >> >>>>+++ b/drivers/pci/host/Kconfig >> >>>>@@ -228,4 +228,16 @@ config PCI_BRCMSTB >> >>>> default ARCH_BRCMSTB || BMIPS_GENERIC >> >>>> help >> >>>> Adds support for Broadcom Settop Box PCIe host controller. >> >>>>+ To compile this driver as a module, choose m here. >> >>>>+ >> >>>>+config PCI_BRCMSTB_MSI >> >>>>+ bool "Broadcom Brcmstb PCIe MSI support" >> >>>>+ depends on ARCH_BRCMSTB || BMIPS_GENERIC >> >>>This could probably be depends on PCI_BRCMSTB, which would imply these >> >>>two conditions. PCI_BRCMSTB_MSI on its own is probably not very useful >> >>>without the parent RC driver. >> >>> >> >>>>+ depends on OF >> >>>>+ depends on PCI_MSI >> >>>>+ default PCI_BRCMSTB >> >>>>+ help >> >>>>+ Say Y here if you want to enable MSI support for Broadcom's iProc >> >>>>+ PCIe controller >> >>>>+ >> >>>> endmenu >> >>>>diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile >> >>>>index c283321..1026d6f 100644 >> >>>>--- a/drivers/pci/host/Makefile >> >>>>+++ b/drivers/pci/host/Makefile >> >>>>@@ -23,6 +23,7 @@ obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o >> >>>> obj-$(CONFIG_VMD) += vmd.o >> >>>> obj-$(CONFIG_PCI_BRCMSTB) += brcmstb-pci.o >> >>>> brcmstb-pci-objs := pci-brcmstb.o pci-brcmstb-dma.o >> >>>>+obj-$(CONFIG_PCI_BRCMSTB_MSI) += pci-brcmstb-msi.o >> >>>Should we combine this file with the brcmstb-pci.o? There is probably no >> >>>functional difference, except that pci-brcmstb-msi.ko needs to be loaded >> >>>first, right? >> >>>-- >> >>>Florian >> >>If you look at the pci/host/Kconfig you will see that other drivers >> >>also have a separate MSI config (eg iproc, altera, xgene) so there is >> >>precedent. The reason that pci-brcmstb-msi.c is its own file is >> >>because it depends on an irq function that is not exported. That is >> >>why CONFIG_PCI_BRCMSTB_MSI is bool, and CONFIG_PCI_BRCMSTB is >> >>tristate. -- Jim >> >There is precedent, but that doesn't mean I like it :) >> >I would strongly prefer one file per driver when possible. >> > >> >Take iproc for example. iproc-msi.c is enabled by a Kconfig bool. It >> >contains a bunch of code with the only external entry points being >> >iproc_msi_init() and iproc_msi_exit(). These are only called via >> >iproc_pcie_bcma_probe() or iproc_pcie_pltfm_probe(), both of which are >> >tristate. So iproc-msi.c is only compiled if CONFIG_IPROC_BCMA or >> >CONFIG_IPROC_PLATFORM are enabled, but all that text is loaded even if >> >neither module is loaded, which seems suboptimal. >> > >> >I don't care if you have several config options to enable the BCMA >> >probe and the platform probe (although these could probably be >> >replaced in the code by a simple "#ifdef CONFIG_BCMA" and "#ifdef >> >CONFIG_OF"), and making CONFIG_PCIE_IPROC tristate so it can be a >> >module makes sense. But I think it would be better to put all the >> >code in one file instead of five, and probably remove >> >CONFIG_PCIE_IPROC_MSI. Maybe this requires exporting some IRQ >> >function that currently isn't exported. But that seems like a simpler >> >solution than what we currently have. >> Placing pcie-iproc-bcma.c in its own file is useful in being able to >> read the code that is actually used. BCMA is really unnecessary if >> a few platforms stopped using BCMA and declared everything via >> devicetree or ACPI. Same with pcie-iproc-platform.c. Both keep the >> mess out of pcie-iproc.c. > > Maybe. Both pcie-iproc-bcma.c and pcie-iproc-platform.c are small > (280 lines combined) relative to pcie-iproc.c + pcie-iproc-msi.c (2150 > lines combined), and keeping them separate requires pcie-iproc.h, > which could otherwise be folded into pcie-iproc.c. So I'm still a > little skeptical. You think of combining them as a mess, but I think > of it as a big convenience because I could see all the iproc-related > code in one place :) > >> It looks like pcie-iproc-msi.c followed existing pci drivers in >> place. So if msi was cleaned up through the entire pci drivers then >> yes it would make sense to remove CONFIG_PCIE_IPROC_MSI and combine >> code in pcie-iproc.c. But I think leaving the bcma and platform >> code in their own files makes it easier for us to work with the code >> rather than placing unused code in ifdefs in the same file. > > But I do object *more* to putting MSI in a separate file, especially > given the fact that all PCIe devices that generate interrupts must > support MSI. So any reasonable PCIe root complex must also support > MSI, and I don't really see the benefit of configuring and building it > separately. > > Anyway, iproc is already in the tree, and we *could* maybe change > something eventually. > > The question we need to decide now is about how brcmstb should be > structured. Somebody mentioned an IRQ function that would have to be > exported for the brcmstb main driver & MSI pieces to be combined. I > don't know what specifically that is, but I thought maybe iproc had > the same issue. I was one the one who said an export was needed but I just recompiled the MSI code as a module and had no problem. I was probably using an older tree when I concluded this. So I'm fine with combining the MSI code in the same file as the driver. > > Bjorn
