Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support
...
>> +bus = pci_create_root_bus(pcie->dev, 0, _pcie_ops,
>> + >sysdata, pcie->resources);
>> +if (!bus) {
>> +dev_err(pcie->dev, "unable to create PCI root bus\n");
>> +ret = -ENOMEM;
>> +goto err_power_off_phy;
>> +}
>> +pcie->root_bus = bus;
>> +
>> +ret = iproc_pcie_check_link(pcie, bus);
>> +if (ret) {
>> +dev_err(pcie->dev, "no PCIe EP device detected\n");
>> +goto err_rm_root_bus;
>> +}
>> +
>> +iproc_pcie_enable(pcie);
>> +
>> +pci_scan_child_bus(bus);
>> +pci_assign_unassigned_bus_resources(bus);
>> +pci_bus_add_devices(bus);
>> +
>> +pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
>
> I think the IRQ fixup should be before pci_bus_add_devices(). CC'd Yijing,
> who's been fixing similar issues.
>
> pci_bus_add_devices() is the point where drivers can claim these devices,
> and we don't want to change things after a driver claims the device.
Yes, we should call pci_bus_add_devices() at last, after all
resources(mem/io/irq) are
configured properly. Otherwise, this code could not be run normally in non
system boot up path.
Thanks!
Yijing.
>
>> +
>> +return 0;
>> +
>> +err_rm_root_bus:
>> +pci_stop_root_bus(bus);
>> +pci_remove_root_bus(bus);
>> +
>> +err_power_off_phy:
>> +if (pcie->phy)
>> +phy_power_off(pcie->phy);
>> +err_exit_phy:
>> +if (pcie->phy)
>> +phy_exit(pcie->phy);
>> +
>> +return ret;
>> +}
>> +EXPORT_SYMBOL(iproc_pcie_setup);
>> +
>> +int iproc_pcie_remove(struct iproc_pcie *pcie)
>> +{
>> +pci_stop_root_bus(pcie->root_bus);
>> +pci_remove_root_bus(pcie->root_bus);
>> +
>> +if (pcie->phy) {
>> +phy_power_off(pcie->phy);
>> +phy_exit(pcie->phy);
>> +}
>> +
>> +return 0;
>> +}
>> +EXPORT_SYMBOL(iproc_pcie_remove);
>
> These exports wouldn't be needed if this were all squashed into one file.
>
>> +
>> +MODULE_AUTHOR("Ray Jui ");
>> +MODULE_DESCRIPTION("Broadcom iPROC PCIe common driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
>> new file mode 100644
>> index 000..569bc04
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-iproc.h
>> @@ -0,0 +1,42 @@
>> +/*
>> + * Copyright (C) 2014-2015 Broadcom Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#ifndef _PCIE_IPROC_H
>> +#define _PCIE_IPROC_H
>> +
>> +#define IPROC_PCIE_MAX_NUM_IRQS 6
>> +
>> +/**
>> + * iProc PCIe device
>> + * @dev: pointer to device data structure
>> + * @base: PCIe host controller I/O register base
>> + * @resources: linked list of all PCI resources
>> + * @sysdata: Per PCI controller data
>> + * @root_bus: pointer to root bus
>> + * @phy: optional PHY device that controls the Serdes
>> + * @irqs: interrupt IDs
>> + */
>> +struct iproc_pcie {
>> +struct device *dev;
>> +void __iomem *base;
>> +struct list_head *resources;
>> +struct pci_sys_data sysdata;
>> +struct pci_bus *root_bus;
>> +struct phy *phy;
>> +int irqs[IPROC_PCIE_MAX_NUM_IRQS];
>> +};
>> +
>> +extern int iproc_pcie_setup(struct iproc_pcie *pcie);
>> +extern int iproc_pcie_remove(struct iproc_pcie *pcie);
>
> Hopefully you can squash this all into one file, but if you can't, my
> preference is to omit "extern" on function declarations.
>
>> +
>> +#endif /* _PCIE_IPROC_H */
>> --
>> 1.7.9.5
>>
>
> .
>
--
Thanks!
Yijing
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support
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
> >> Reviewed-by: Scott Branden
> >> ---
> >> 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 0x
> >> +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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support
On Tuesday 10 March 2015 15:22:36 Ray Jui wrote: > > 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. > I was the one that initially suggested splitting the driver into files like this, and I still think it's the right strategy with the BCMA driver coming up. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support
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
>> Reviewed-by: Scott Branden
>> ---
>> 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.
>> +
>> endmenu
>> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
>> index e61d91c..2e02d20 100644
>> --- a/drivers/pci/host/Makefile
>> +++ b/drivers/pci/host/Makefile
>> @@ -13,3 +13,5 @@ obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
>> obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
>> obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
>> obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
>> +obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
>> +obj-$(CONFIG_PCIE_IPROC_PLTFM) += pcie-iproc-pltfm.o
>> diff --git a/drivers/pci/host/pcie-iproc-pltfm.c
>> b/drivers/pci/host/pcie-iproc-pltfm.c
>> new file mode 100644
>> index 000..af935ea
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-iproc-pltfm.c
>> @@ -0,0 +1,108 @@
>> +/*
>> + * Copyright (C) 2015 Broadcom Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include "pcie-iproc.h"
>> +
>> +static int __init iproc_pcie_pltfm_probe(struct platform_device *pdev)
>> +{
>> +struct iproc_pcie *pcie;
>> +struct device_node *np = pdev->dev.of_node;
>> +struct resource reg;
>> +resource_size_t iobase = 0;
>> +LIST_HEAD(res);
>> +int ret;
>> +
>> +pcie = devm_kzalloc(>dev, sizeof(struct iproc_pcie), GFP_KERNEL);
>> +if (!pcie)
>> +return -ENOMEM;
>> +
>> +pcie->dev = >dev;
>> +platform_set_drvdata(pdev, pcie);
>> +
>> +ret = of_address_to_resource(np, 0, );
>> +if (ret < 0) {
>> +dev_err(pcie->dev, "unable to obtain controller resources\n");
>> +return ret;
>> +}
>> +
>> +pcie->base = devm_ioremap(pcie->dev, reg.start, resource_size());
>>
Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support
[+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
> Reviewed-by: Scott Branden
> ---
> 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.
> +
> endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index e61d91c..2e02d20 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -13,3 +13,5 @@ obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
> obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
> obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
> obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
> +obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
> +obj-$(CONFIG_PCIE_IPROC_PLTFM) += pcie-iproc-pltfm.o
> diff --git a/drivers/pci/host/pcie-iproc-pltfm.c
> b/drivers/pci/host/pcie-iproc-pltfm.c
> new file mode 100644
> index 000..af935ea
> --- /dev/null
> +++ b/drivers/pci/host/pcie-iproc-pltfm.c
> @@ -0,0 +1,108 @@
> +/*
> + * Copyright (C) 2015 Broadcom Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "pcie-iproc.h"
> +
> +static int __init iproc_pcie_pltfm_probe(struct platform_device *pdev)
> +{
> + struct iproc_pcie *pcie;
> + struct device_node *np = pdev->dev.of_node;
> + struct resource reg;
> + resource_size_t iobase = 0;
> + LIST_HEAD(res);
> + int ret;
> +
> + pcie = devm_kzalloc(>dev, sizeof(struct iproc_pcie), GFP_KERNEL);
> + if (!pcie)
> + return -ENOMEM;
> +
> + pcie->dev = >dev;
> + platform_set_drvdata(pdev, pcie);
> +
> + ret = of_address_to_resource(np, 0, );
> + if (ret < 0) {
> + dev_err(pcie->dev, "unable to obtain controller resources\n");
> + return ret;
> + }
> +
> + pcie->base = devm_ioremap(pcie->dev, reg.start, resource_size());
> + if (!pcie->base) {
> + dev_err(pcie->dev, "unable to map controller registers\n");
> + return -ENOMEM;
> + }
> +
> + /* PHY use is optional */
> + pcie->phy = devm_phy_get(>dev, "pcie-phy");
> + if (IS_ERR(pcie->phy)) {
> + if (PTR_ERR(pcie->phy) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + pcie->phy = NULL;
> + }
> +
> + ret = of_pci_get_host_bridge_resources(np, 0, 0xff, , );
> +
Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support
...
+bus = pci_create_root_bus(pcie-dev, 0, iproc_pcie_ops,
+ pcie-sysdata, pcie-resources);
+if (!bus) {
+dev_err(pcie-dev, unable to create PCI root bus\n);
+ret = -ENOMEM;
+goto err_power_off_phy;
+}
+pcie-root_bus = bus;
+
+ret = iproc_pcie_check_link(pcie, bus);
+if (ret) {
+dev_err(pcie-dev, no PCIe EP device detected\n);
+goto err_rm_root_bus;
+}
+
+iproc_pcie_enable(pcie);
+
+pci_scan_child_bus(bus);
+pci_assign_unassigned_bus_resources(bus);
+pci_bus_add_devices(bus);
+
+pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
I think the IRQ fixup should be before pci_bus_add_devices(). CC'd Yijing,
who's been fixing similar issues.
pci_bus_add_devices() is the point where drivers can claim these devices,
and we don't want to change things after a driver claims the device.
Yes, we should call pci_bus_add_devices() at last, after all
resources(mem/io/irq) are
configured properly. Otherwise, this code could not be run normally in non
system boot up path.
Thanks!
Yijing.
+
+return 0;
+
+err_rm_root_bus:
+pci_stop_root_bus(bus);
+pci_remove_root_bus(bus);
+
+err_power_off_phy:
+if (pcie-phy)
+phy_power_off(pcie-phy);
+err_exit_phy:
+if (pcie-phy)
+phy_exit(pcie-phy);
+
+return ret;
+}
+EXPORT_SYMBOL(iproc_pcie_setup);
+
+int iproc_pcie_remove(struct iproc_pcie *pcie)
+{
+pci_stop_root_bus(pcie-root_bus);
+pci_remove_root_bus(pcie-root_bus);
+
+if (pcie-phy) {
+phy_power_off(pcie-phy);
+phy_exit(pcie-phy);
+}
+
+return 0;
+}
+EXPORT_SYMBOL(iproc_pcie_remove);
These exports wouldn't be needed if this were all squashed into one file.
+
+MODULE_AUTHOR(Ray Jui r...@broadcom.com);
+MODULE_DESCRIPTION(Broadcom iPROC PCIe common driver);
+MODULE_LICENSE(GPL v2);
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
new file mode 100644
index 000..569bc04
--- /dev/null
+++ b/drivers/pci/host/pcie-iproc.h
@@ -0,0 +1,42 @@
+/*
+ * Copyright (C) 2014-2015 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed as is WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _PCIE_IPROC_H
+#define _PCIE_IPROC_H
+
+#define IPROC_PCIE_MAX_NUM_IRQS 6
+
+/**
+ * iProc PCIe device
+ * @dev: pointer to device data structure
+ * @base: PCIe host controller I/O register base
+ * @resources: linked list of all PCI resources
+ * @sysdata: Per PCI controller data
+ * @root_bus: pointer to root bus
+ * @phy: optional PHY device that controls the Serdes
+ * @irqs: interrupt IDs
+ */
+struct iproc_pcie {
+struct device *dev;
+void __iomem *base;
+struct list_head *resources;
+struct pci_sys_data sysdata;
+struct pci_bus *root_bus;
+struct phy *phy;
+int irqs[IPROC_PCIE_MAX_NUM_IRQS];
+};
+
+extern int iproc_pcie_setup(struct iproc_pcie *pcie);
+extern int iproc_pcie_remove(struct iproc_pcie *pcie);
Hopefully you can squash this all into one file, but if you can't, my
preference is to omit extern on function declarations.
+
+#endif /* _PCIE_IPROC_H */
--
1.7.9.5
.
--
Thanks!
Yijing
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support
[+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 r...@broadcom.com
Reviewed-by: Scott Branden sbra...@broadcom.com
---
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.
+
endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index e61d91c..2e02d20 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -13,3 +13,5 @@ obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
+obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
+obj-$(CONFIG_PCIE_IPROC_PLTFM) += pcie-iproc-pltfm.o
diff --git a/drivers/pci/host/pcie-iproc-pltfm.c
b/drivers/pci/host/pcie-iproc-pltfm.c
new file mode 100644
index 000..af935ea
--- /dev/null
+++ b/drivers/pci/host/pcie-iproc-pltfm.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright (C) 2015 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed as is WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/kernel.h
+#include linux/pci.h
+#include linux/clk.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/interrupt.h
+#include linux/platform_device.h
+#include linux/of_address.h
+#include linux/of_pci.h
+#include linux/of_irq.h
+#include linux/of_platform.h
+#include linux/phy/phy.h
+
+#include pcie-iproc.h
+
+static int __init iproc_pcie_pltfm_probe(struct platform_device *pdev)
+{
+ struct iproc_pcie *pcie;
+ struct device_node *np = pdev-dev.of_node;
+ struct resource reg;
+ resource_size_t iobase = 0;
+ LIST_HEAD(res);
+ int ret;
+
+ pcie = devm_kzalloc(pdev-dev, sizeof(struct iproc_pcie), GFP_KERNEL);
+ if (!pcie)
+ return -ENOMEM;
+
+ pcie-dev = pdev-dev;
+ platform_set_drvdata(pdev, pcie);
+
+ ret = of_address_to_resource(np, 0, reg);
+ if (ret 0) {
+ dev_err(pcie-dev, unable to obtain controller resources\n);
+ return ret;
+ }
+
+ pcie-base = devm_ioremap(pcie-dev, reg.start, resource_size(reg));
+ if (!pcie-base) {
+ dev_err(pcie-dev, unable to map controller registers\n);
+ return -ENOMEM;
+ }
+
+ /* PHY use is optional */
+ pcie-phy = devm_phy_get(pdev-dev, pcie-phy);
+ if (IS_ERR(pcie-phy)) {
+ if (PTR_ERR(pcie-phy) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ pcie-phy = NULL;
Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support
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 r...@broadcom.com
Reviewed-by: Scott Branden sbra...@broadcom.com
---
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.
+
endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index e61d91c..2e02d20 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -13,3 +13,5 @@ obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
+obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
+obj-$(CONFIG_PCIE_IPROC_PLTFM) += pcie-iproc-pltfm.o
diff --git a/drivers/pci/host/pcie-iproc-pltfm.c
b/drivers/pci/host/pcie-iproc-pltfm.c
new file mode 100644
index 000..af935ea
--- /dev/null
+++ b/drivers/pci/host/pcie-iproc-pltfm.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright (C) 2015 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed as is WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/kernel.h
+#include linux/pci.h
+#include linux/clk.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/interrupt.h
+#include linux/platform_device.h
+#include linux/of_address.h
+#include linux/of_pci.h
+#include linux/of_irq.h
+#include linux/of_platform.h
+#include linux/phy/phy.h
+
+#include pcie-iproc.h
+
+static int __init iproc_pcie_pltfm_probe(struct platform_device *pdev)
+{
+struct iproc_pcie *pcie;
+struct device_node *np = pdev-dev.of_node;
+struct resource reg;
+resource_size_t iobase = 0;
+LIST_HEAD(res);
+int ret;
+
+pcie = devm_kzalloc(pdev-dev, sizeof(struct iproc_pcie), GFP_KERNEL);
+if (!pcie)
+return -ENOMEM;
+
+pcie-dev = pdev-dev;
+platform_set_drvdata(pdev, pcie);
+
+ret = of_address_to_resource(np, 0, reg);
+if (ret 0) {
+dev_err(pcie-dev, unable to obtain controller resources\n);
+return ret;
+}
+
+pcie-base = devm_ioremap(pcie-dev, reg.start, resource_size(reg));
+if (!pcie-base) {
+
Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support
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 r...@broadcom.com
Reviewed-by: Scott Branden sbra...@broadcom.com
---
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 0x
+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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support
On Tuesday 10 March 2015 15:22:36 Ray Jui wrote: 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. I was the one that initially suggested splitting the driver into files like this, and I still think it's the right strategy with the BCMA driver coming up. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
