On Tue, Mar 10, 2015 at 03:22:36PM -0700, Ray Jui wrote:
> Hi Bjorn,
>
> On 3/10/2015 2:40 PM, Bjorn Helgaas wrote:
> > [+cc Rob, Yijing]
> >
> > On Mon, Mar 09, 2015 at 05:38:05PM -0700, Ray Jui wrote:
> >> This adds the support for Broadcom iProc PCIe controller
> >>
> >> pcie-iproc.c servers as the common core driver, and front-end bus
> >> interface needs to be added to support different bus interfaces
> >>
> >> pcie-iproc-pltfm.c contains the support for the platform bus interface
> >>
> >> Signed-off-by: Ray Jui <rjui@xxxxxxxxxxxx>
> >> Reviewed-by: Scott Branden <sbraden@xxxxxxxxxxxx>
> >> ---
> >> drivers/pci/host/Kconfig | 17 ++
> >> drivers/pci/host/Makefile | 2 +
> >> drivers/pci/host/pcie-iproc-pltfm.c | 108 +++++++++++
> >> drivers/pci/host/pcie-iproc.c | 351 +++++++++++++++++++++++++++++++++++
> >> drivers/pci/host/pcie-iproc.h | 42 +++++
> >> 5 files changed, 520 insertions(+)
> >> create mode 100644 drivers/pci/host/pcie-iproc-pltfm.c
> >> create mode 100644 drivers/pci/host/pcie-iproc.c
> >> create mode 100644 drivers/pci/host/pcie-iproc.h
> >>
> >> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> >> index 7b892a9..f4d9c90 100644
> >> --- a/drivers/pci/host/Kconfig
> >> +++ b/drivers/pci/host/Kconfig
> >> @@ -106,4 +106,21 @@ config PCI_VERSATILE
> >> bool "ARM Versatile PB PCI controller"
> >> depends on ARCH_VERSATILE
> >>
> >> +config PCIE_IPROC
> >> + tristate "Broadcom iProc PCIe controller"
> >> + help
> >> + This enables the iProc PCIe core controller support for Broadcom's
> >> + iProc family of SoCs. An appropriate bus interface driver also needs
> >> + to be enabled
> >> +
> >> +config PCIE_IPROC_PLTFM
> >> + tristate "Broadcom iProc PCIe platform bus driver"
> >> + depends on ARCH_BCM_IPROC || COMPILE_TEST
> >> + depends on OF
> >> + select PCIE_IPROC
> >> + default ARCH_BCM_IPROC
> >> + help
> >> + Say Y here if you want to use the Broadcom iProc PCIe controller
> >> + through the generic platform bus interface
> >
> > Do you anticipate additional front-end bus interfaces? If not, and maybe
> > even if you do, you might squash everything into pcie-iproc.c. Then you
> > only need one file (no .h file needed) and the package is a little
> > simpler. I think it's pretty common to have multiple driver registration
> > methods in the same file (OF, PCI, ACPI, etc.) And I think it's common to
> > have those methods guarded by the generic config symbol (CONFIG_PCI,
> > CONFIG_OF, etc.) rather than defining new ones specific to the driver.
>
> Yes I do expect Hauke (CCed) to add BCMA bus front end support later.
>
> I still think having the front end bus driver separated is cleaner and
> may be less troublesome for Hauke to add BCMA support in the future. But
> if you strongly favor having everything stuffed in one single file, I
> can make that change. Please let me know.
OK, just leave it as-is.
> >> +#define INVALID_ACCESS_OFFSET 0xffffffff
> >> +static u32 iproc_pcie_cfg_base(struct iproc_pcie *pcie, int busno,
> >> + unsigned int devfn, int where)
> >> +{
> >> + int slot = PCI_SLOT(devfn);
> >> + int fn = PCI_FUNC(devfn);
> >> + u32 val;
> >> +
> >
> > Would you mind adding a comment to the effect that
> > CFG_IND_ADDR_OFFSET/CFG_IND_DATA_OFFSET and
> > CFG_ADDR_OFFSET/CFG_DATA_OFFSET are protected by pci_lock?
> >
> > They obviously need a mutex, and while I don't have any plans to
> > change it, I'm not completely 100% sure that pci_lock is the best
> > place for it.
>
> Sorry I don't get what you want me to do here. Do you want me to add
> some comment to explain that the struct pci_ops read/write callbacks are
> already protected at the upper layer by the pci_lock spinlock and
> therefore no lock is required in this driver?
Nothing fancy; something like this that "git grep pci_lock" will find:
/* addr/data must used atomically and are protected by pci_lock */
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
