[PATCH v5 0/4] pci: iproc: Add Broadcom iProc PCIe support

[PATCH v5 0/4] pci: iproc: Add Broadcom iProc PCIe support

[Ray Jui]

This patch series 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

Changes from v4:
 - iProc PCIe driver module support was not included in patch series v4.
   This patch series (v5) fixes it

Changes from v3:
 - Export symbols of several PCI functions so they can be used by drivers
   compiled as kernel module
 - Add additional support to the Broadcom iProc PCIe driver so it can be
   installed/uninstalled as kernel loadable module

Changes from v2:
 - Major rework of the PCIe driver to factor out common generic code from
   front-end bus interface. Support for generic platform bus interface is
   also added
 - Adapt to several new PCI APIs that have been introduced lately

Changes from v1:
 - Add standard PCI interrupt DT properties "#interrupt-cells",
   "interrupt-map-mask" and "interrupt-map" so legacy INTx interrupts can be
   supported by using standard PCI OF IRQ parsing function
 - Get rid of custom IRQ mapping function in the driver. Use
   of_irq_parse_and_map_pci instead

Ray Jui (4):
  PCI: Export symbols of PCI functions
  pci: iProc: define iProc PCIe platform bus binding
  pci: iproc: Add Broadcom iProc PCIe support
  ARM: dts: enable PCIe support for Cygnus

 .../devicetree/bindings/pci/brcm,iproc-pcie.txt    |   63 ++++
 arch/arm/boot/dts/bcm-cygnus.dtsi                  |   42 +++
 arch/arm/boot/dts/bcm958300k.dts                   |    8 +
 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 +++
 drivers/pci/pci.c                                  |    1 +
 drivers/pci/probe.c                                |    1 +
 drivers/pci/remove.c                               |    2 +
 drivers/pci/setup-bus.c                            |    1 +
 drivers/pci/setup-irq.c                            |    1 +
 13 files changed, 639 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
 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

-- 
1.7.9.5

[PATCH v5 1/4] PCI: Export symbols of PCI functions

[Ray Jui]

Export symbols of the following PCI functions so they can be referenced
by a PCI driver compiled as a kernel loadable module:

pci_common_swizzle
pci_create_root_bus
pci_stop_root_bus
pci_remove_root_bus
pci_assign_unassigned_bus_resources
pci_fixup_irqs

Signed-off-by: Ray Jui <rjui@broadcom.com>
---
 drivers/pci/pci.c       |    1 +
 drivers/pci/probe.c     |    1 +
 drivers/pci/remove.c    |    2 ++
 drivers/pci/setup-bus.c |    1 +
 drivers/pci/setup-irq.c |    1 +
 5 files changed, 6 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 81f06e8..fb4a3da 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2492,6 +2492,7 @@ u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp)
 	*pinp = pin;
 	return PCI_SLOT(dev->devfn);
 }
+EXPORT_SYMBOL(pci_common_swizzle);
 
 /**
  *	pci_release_region - Release a PCI bar
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8d2f400..eb24ef9 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1993,6 +1993,7 @@ err_out:
 	kfree(b);
 	return NULL;
 }
+EXPORT_SYMBOL(pci_create_root_bus);
 
 int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
 {
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 8bd76c9..c523159 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -139,6 +139,7 @@ void pci_stop_root_bus(struct pci_bus *bus)
 	/* stop the host bridge */
 	device_release_driver(&host_bridge->dev);
 }
+EXPORT_SYMBOL(pci_stop_root_bus);
 
 void pci_remove_root_bus(struct pci_bus *bus)
 {
@@ -158,3 +159,4 @@ void pci_remove_root_bus(struct pci_bus *bus)
 	/* remove the host bridge */
 	device_unregister(&host_bridge->dev);
 }
+EXPORT_SYMBOL(pci_remove_root_bus);
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index e3e17f3..932c9e5 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1750,3 +1750,4 @@ void pci_assign_unassigned_bus_resources(struct pci_bus *bus)
 	__pci_bus_assign_resources(bus, &add_list, NULL);
 	BUG_ON(!list_empty(&add_list));
 }
+EXPORT_SYMBOL(pci_assign_unassigned_bus_resources);
diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
index 4e2d595..9d2bbb6 100644
--- a/drivers/pci/setup-irq.c
+++ b/drivers/pci/setup-irq.c
@@ -65,3 +65,4 @@ void pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
 	for_each_pci_dev(dev)
 		pdev_fixup_irq(dev, swizzle, map_irq);
 }
+EXPORT_SYMBOL(pci_fixup_irqs);
-- 
1.7.9.5

[PATCH v5 2/4] pci: iProc: define iProc PCIe platform bus binding

[Ray Jui]

Document the Broadcom iProc PCIe platform interface device tree binding

Signed-off-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbraden@broadcom.com>
---
 .../devicetree/bindings/pci/brcm,iproc-pcie.txt    |   63 ++++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt

diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
new file mode 100644
index 0000000..f7ce50e
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
@@ -0,0 +1,63 @@
+* Broadcom iProc PCIe controller with the platform bus interface
+
+Required properties:
+- compatible: Must be "brcm,iproc-pcie"
+- reg: base address and length of the PCIe controller I/O register space
+- #interrupt-cells: set to <1>
+- interrupt-map-mask and interrupt-map, standard PCI properties to define the
+  mapping of the PCIe interface to interrupt numbers
+- linux,pci-domain: PCI domain ID. Should be unique for each host controller
+- bus-range: PCI bus numbers covered
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions
+
+Optional properties:
+- phys: phandle of the PCIe PHY device
+- phy-names: must be "pcie-phy"
+
+Example:
+	pcie0: pcie@18012000 {
+		compatible = "brcm,iproc-pcie";
+		reg = <0x18012000 0x1000>;
+
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 0>;
+		interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
+
+		linux,pci-domain = <0>;
+
+		bus-range = <0x00 0xff>;
+
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x81000000 0 0	  0x28000000 0 0x00010000
+			  0x82000000 0 0x20000000 0x20000000 0 0x04000000>;
+
+		phys = <&phy 0 5>;
+		phy-names = "pcie-phy";
+	};
+
+	pcie1: pcie@18013000 {
+		compatible = "brcm,iproc-pcie";
+		reg = <0x18013000 0x1000>;
+
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 0>;
+		interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
+
+		linux,pci-domain = <1>;
+
+		bus-range = <0x00 0xff>;
+
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x81000000 0 0	  0x48000000 0 0x00010000
+			  0x82000000 0 0x40000000 0x40000000 0 0x04000000>;
+
+		phys = <&phy 1 6>;
+		phy-names = "pcie-phy";
+	};
-- 
1.7.9.5

[PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support

[Ray Jui]

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@broadcom.com>
Reviewed-by: Scott Branden <sbraden@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
+
 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 0000000..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;
+	}
+
+	ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &iobase);
+	if (ret) {
+		dev_err(pcie->dev,
+			"unable to get PCI host bridge resources\n");
+		return ret;
+	}
+
+	pcie->resources = &res;
+
+	ret = iproc_pcie_setup(pcie);
+	if (ret) {
+		dev_err(pcie->dev, "PCIe controller setup failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
+{
+	struct iproc_pcie *pcie = platform_get_drvdata(pdev);
+
+	return iproc_pcie_remove(pcie);
+}
+
+static const struct of_device_id iproc_pcie_of_match_table[] = {
+	{ .compatible = "brcm,iproc-pcie", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, iproc_pcie_of_match_table);
+
+static struct platform_driver iproc_pcie_pltfm_driver = {
+	.driver = {
+		.name = "iproc-pcie",
+		.of_match_table = of_match_ptr(iproc_pcie_of_match_table),
+	},
+	.probe = iproc_pcie_pltfm_probe,
+	.remove = iproc_pcie_pltfm_remove,
+};
+module_platform_driver(iproc_pcie_pltfm_driver);
+
+MODULE_AUTHOR("Ray Jui <rjui@broadcom.com>");
+MODULE_DESCRIPTION("Broadcom iPROC PCIe platform driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
new file mode 100644
index 0000000..ad6c89e
--- /dev/null
+++ b/drivers/pci/host/pcie-iproc.c
@@ -0,0 +1,351 @@
+/*
+ * Copyright (C) 2014 Hauke Mehrtens <hauke@hauke-m.de>
+ * Copyright (C) 2015 Broadcom Corporatcommon ion
+ *
+ * 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/msi.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/mbus.h>
+#include <linux/slab.h>
+#include <linux/delay.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"
+
+#define CLK_CONTROL_OFFSET           0x000
+#define EP_MODE_SURVIVE_PERST_SHIFT  1
+#define EP_MODE_SURVIVE_PERST        BIT(EP_MODE_SURVIVE_PERST_SHIFT)
+#define RC_PCIE_RST_OUTPUT_SHIFT     0
+#define RC_PCIE_RST_OUTPUT           BIT(RC_PCIE_RST_OUTPUT_SHIFT)
+
+#define CFG_IND_ADDR_OFFSET          0x120
+#define CFG_IND_ADDR_MASK            0x00001ffc
+
+#define CFG_IND_DATA_OFFSET          0x124
+
+#define CFG_ADDR_OFFSET              0x1f8
+#define CFG_ADDR_BUS_NUM_SHIFT       20
+#define CFG_ADDR_BUS_NUM_MASK        0x0ff00000
+#define CFG_ADDR_DEV_NUM_SHIFT       15
+#define CFG_ADDR_DEV_NUM_MASK        0x000f8000
+#define CFG_ADDR_FUNC_NUM_SHIFT      12
+#define CFG_ADDR_FUNC_NUM_MASK       0x00007000
+#define CFG_ADDR_REG_NUM_SHIFT       2
+#define CFG_ADDR_REG_NUM_MASK        0x00000ffc
+#define CFG_ADDR_CFG_TYPE_SHIFT      0
+#define CFG_ADDR_CFG_TYPE_MASK       0x00000003
+
+#define CFG_DATA_OFFSET              0x1fc
+
+#define SYS_RC_INTX_EN               0x330
+#define SYS_RC_INTX_MASK             0xf
+
+static inline struct iproc_pcie *sys_to_pcie(struct pci_sys_data *sys)
+{
+	return sys->private_data;
+}
+
+#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;
+
+	/* root complex access */
+	if (busno == 0) {
+		if (slot >= 1)
+			return INVALID_ACCESS_OFFSET;
+		writel(where & CFG_IND_ADDR_MASK,
+		       pcie->base + CFG_IND_ADDR_OFFSET);
+		return CFG_IND_DATA_OFFSET;
+	}
+
+	if (fn > 1)
+		return INVALID_ACCESS_OFFSET;
+
+	/* EP device access */
+	val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
+		(slot << CFG_ADDR_DEV_NUM_SHIFT) |
+		(fn << CFG_ADDR_FUNC_NUM_SHIFT) |
+		(where & CFG_ADDR_REG_NUM_MASK) |
+		(1 & CFG_ADDR_CFG_TYPE_MASK);
+	writel(val, pcie->base + CFG_ADDR_OFFSET);
+
+	return CFG_DATA_OFFSET;
+}
+
+#define INVALID_CFG_RD 0xffffffff
+static int iproc_pcie_read_conf(struct iproc_pcie *pcie, int busno,
+				unsigned int devfn, int where, int size,
+				u32 *val)
+{
+	u32 offset, mask, shift;
+
+	*val = INVALID_CFG_RD;
+
+	if (size != 1 && size != 2 && size != 4)
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	else if ((size == 2) && (where & 1))
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	else if ((size == 4) && (where & 3))
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+
+	offset = iproc_pcie_cfg_base(pcie, busno, devfn, where);
+	if (offset == INVALID_ACCESS_OFFSET)
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+
+	*val = readl(pcie->base + offset);
+
+	if (size != 4) {
+		mask = (1 << (size * BITS_PER_BYTE)) - 1;
+		shift = (where % 4) * BITS_PER_BYTE;
+		*val = (*val >> shift) & mask;
+	}
+
+	dev_dbg(pcie->dev,
+		"conf rd: busn=%d devfn=%u where=%d size=%d val=0x%08x\n",
+		busno, devfn, where, size, *val);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int iproc_pcie_write_conf(struct iproc_pcie *pcie, int busno,
+				 unsigned int devfn, int where, int size,
+				 u32 val)
+{
+	u32 offset, mask, shift, data;
+
+	if (size != 1 && size != 2 && size != 4)
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	else if ((size == 2) && (where & 1))
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	else if ((size == 4) && (where & 3))
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+
+	offset = iproc_pcie_cfg_base(pcie, busno, devfn, where);
+	if (offset == INVALID_ACCESS_OFFSET)
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+
+	data = readl(pcie->base + offset);
+
+	if (size != 4) {
+		mask = (1 << (size * BITS_PER_BYTE)) - 1;
+		shift = (where % 4) * BITS_PER_BYTE;
+		data &= ~(mask << shift);
+		data |= (val & mask) << shift;
+	} else {
+		data = val;
+	}
+
+	writel(data, pcie->base + offset);
+
+	dev_dbg(pcie->dev,
+		"conf wr: busn=%d devfn=%u where=%d size=%d data=0x%08x\n",
+		busno, devfn, where, size, data);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int iproc_pcie_read(struct pci_bus *bus, unsigned int devfn,
+			   int where, int size, u32 *val)
+{
+	struct iproc_pcie *pcie = sys_to_pcie(bus->sysdata);
+
+	return iproc_pcie_read_conf(pcie, bus->number, devfn, where, size,
+				    val);
+}
+
+static int iproc_pcie_write(struct pci_bus *bus, unsigned int devfn,
+			    int where, int size, u32 val)
+{
+	struct iproc_pcie *pcie = sys_to_pcie(bus->sysdata);
+
+	return iproc_pcie_write_conf(pcie, bus->number, devfn, where, size,
+				     val);
+}
+
+static struct pci_ops iproc_pcie_ops = {
+	.read = iproc_pcie_read,
+	.write = iproc_pcie_write,
+};
+
+static void iproc_pcie_reset(struct iproc_pcie *pcie)
+{
+	u32 val;
+
+	/*
+	 * Configure the PCIe controller as root complex and send a downstream
+	 * reset
+	 */
+	val = EP_MODE_SURVIVE_PERST | RC_PCIE_RST_OUTPUT;
+	writel(val, pcie->base + CLK_CONTROL_OFFSET);
+	udelay(250);
+	val &= ~EP_MODE_SURVIVE_PERST;
+	writel(val, pcie->base + CLK_CONTROL_OFFSET);
+	msleep(250);
+}
+
+static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
+{
+	u8 hdr_type;
+	u32 link_ctrl;
+	u16 pos, link_status;
+	int link_is_active = 0;
+
+	/* make sure we are not in EP mode */
+	pci_bus_read_config_byte(bus, 0, PCI_HEADER_TYPE, &hdr_type);
+	if ((hdr_type & 0x7f) != PCI_HEADER_TYPE_BRIDGE) {
+		dev_err(pcie->dev, "in EP mode, hdr=0x08%x\n", hdr_type);
+		return -EFAULT;
+	}
+
+	/* force class to PCI_CLASS_BRIDGE_PCI (0x0604) */
+	pci_bus_write_config_word(bus, 0, PCI_CLASS_DEVICE,
+				  PCI_CLASS_BRIDGE_PCI);
+
+	/* check link status to see if link is active */
+	pos = pci_bus_find_capability(bus, 0, PCI_CAP_ID_EXP);
+	pci_bus_read_config_word(bus, 0, pos + PCI_EXP_LNKSTA, &link_status);
+	if (link_status & PCI_EXP_LNKSTA_NLW)
+		link_is_active = 1;
+
+	if (!link_is_active) {
+		/* try GEN 1 link speed */
+#define PCI_LINK_STATUS_CTRL_2_OFFSET 0x0dc
+#define PCI_TARGET_LINK_SPEED_MASK    0xf
+#define PCI_TARGET_LINK_SPEED_GEN2    0x2
+#define PCI_TARGET_LINK_SPEED_GEN1    0x1
+		pci_bus_read_config_dword(bus, 0,
+					  PCI_LINK_STATUS_CTRL_2_OFFSET,
+					  &link_ctrl);
+		if ((link_ctrl & PCI_TARGET_LINK_SPEED_MASK) ==
+		    PCI_TARGET_LINK_SPEED_GEN2) {
+			link_ctrl &= ~PCI_TARGET_LINK_SPEED_MASK;
+			link_ctrl |= PCI_TARGET_LINK_SPEED_GEN1;
+			pci_bus_write_config_dword(bus, 0,
+					   PCI_LINK_STATUS_CTRL_2_OFFSET,
+					   link_ctrl);
+			msleep(100);
+
+			pos = pci_bus_find_capability(bus, 0, PCI_CAP_ID_EXP);
+			pci_bus_read_config_word(bus, 0, pos + PCI_EXP_LNKSTA,
+						 &link_status);
+			if (link_status & PCI_EXP_LNKSTA_NLW)
+				link_is_active = 1;
+		}
+	}
+
+	dev_info(pcie->dev, "link: %s\n", link_is_active ? "UP" : "DOWN");
+
+	return link_is_active ? 0 : -ENODEV;
+}
+
+static void iproc_pcie_enable(struct iproc_pcie *pcie)
+{
+	writel(SYS_RC_INTX_MASK, pcie->base + SYS_RC_INTX_EN);
+}
+
+int iproc_pcie_setup(struct iproc_pcie *pcie)
+{
+	int ret;
+	struct pci_bus *bus;
+
+	if (!pcie || !pcie->dev || !pcie->base)
+		return -EINVAL;
+
+	if (pcie->phy) {
+		ret = phy_init(pcie->phy);
+		if (ret) {
+			dev_err(pcie->dev, "unable to initialize PCIe PHY\n");
+			return ret;
+		}
+
+		ret = phy_power_on(pcie->phy);
+		if (ret) {
+			dev_err(pcie->dev, "unable to power on PCIe PHY\n");
+			goto err_exit_phy;
+		}
+
+	}
+
+	iproc_pcie_reset(pcie);
+
+	pcie->sysdata.private_data = pcie;
+
+	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);
+
+	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);
+
+MODULE_AUTHOR("Ray Jui <rjui@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 0000000..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);
+
+#endif /* _PCIE_IPROC_H */
-- 
1.7.9.5

[PATCH v5 4/4] ARM: dts: enable PCIe support for Cygnus

[Ray Jui]

Add PCIe device nodes in bcm-cygnus.dtsi but keep them disabled there.
Only enable them for bcm958300k where PCIe interfaces are populated

Signed-off-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbraden@broadcom.com>
---
 arch/arm/boot/dts/bcm-cygnus.dtsi |   42 +++++++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/bcm958300k.dts  |    8 +++++++
 2 files changed, 50 insertions(+)

diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
index ff5fb6a..ee0c6e4 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -90,6 +90,48 @@
 		status = "disabled";
 	};
 
+	pcie0: pcie@18012000 {
+		compatible = "brcm,iproc-pcie";
+		reg = <0x18012000 0x1000>;
+
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 0>;
+		interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
+
+		linux,pci-domain = <0>;
+
+		bus-range = <0x00 0xff>;
+
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x81000000 0 0	  0x28000000 0 0x00010000
+			  0x82000000 0 0x20000000 0x20000000 0 0x04000000>;
+
+		status = "disabled";
+	};
+
+	pcie1: pcie@18013000 {
+		compatible = "brcm,iproc-pcie";
+		reg = <0x18013000 0x1000>;
+
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 0>;
+		interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
+
+		linux,pci-domain = <1>;
+
+		bus-range = <0x00 0xff>;
+
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x81000000 0 0	  0x48000000 0 0x00010000
+			  0x82000000 0 0x40000000 0x40000000 0 0x04000000>;
+
+		status = "disabled";
+	};
+
 	uart0: serial@18020000 {
 		compatible = "snps,dw-apb-uart";
 		reg = <0x18020000 0x100>;
diff --git a/arch/arm/boot/dts/bcm958300k.dts b/arch/arm/boot/dts/bcm958300k.dts
index f1bb36f..c9eb856 100644
--- a/arch/arm/boot/dts/bcm958300k.dts
+++ b/arch/arm/boot/dts/bcm958300k.dts
@@ -47,6 +47,14 @@
 		bootargs = "console=ttyS0,115200";
 	};
 
+	pcie0: pcie@18012000 {
+		status = "okay";
+	};
+
+	pcie1: pcie@18013000 {
+		status = "okay";
+	};
+
 	uart3: serial@18023000 {
 		status = "okay";
 	};
-- 
1.7.9.5

Re: [PATCH v5 1/4] PCI: Export symbols of PCI functions

[Bjorn Helgaas]

On Mon, Mar 09, 2015 at 05:38:03PM -0700, Ray Jui wrote:
> Export symbols of the following PCI functions so they can be referenced
> by a PCI driver compiled as a kernel loadable module:
> 
> pci_common_swizzle
> pci_create_root_bus
> pci_stop_root_bus
> pci_remove_root_bus
> pci_assign_unassigned_bus_resources
> pci_fixup_irqs

Have you tested your driver as a loadable module?

If it works to load/unload/reload/etc., I'm OK with exporting these
symbols.  I would prefer EXPORT_SYMBOL_GPL, though, because these don't
constitute a very coherent interface right now, and they will likely be
reworked.  For example, see Yijing Wang's work to sort out the
pci_bus_add_devices() / resource assignment mess.

Bjorn

> Signed-off-by: Ray Jui <rjui@broadcom.com>
> ---
>  drivers/pci/pci.c       |    1 +
>  drivers/pci/probe.c     |    1 +
>  drivers/pci/remove.c    |    2 ++
>  drivers/pci/setup-bus.c |    1 +
>  drivers/pci/setup-irq.c |    1 +
>  5 files changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 81f06e8..fb4a3da 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2492,6 +2492,7 @@ u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp)
>  	*pinp = pin;
>  	return PCI_SLOT(dev->devfn);
>  }
> +EXPORT_SYMBOL(pci_common_swizzle);
>  
>  /**
>   *	pci_release_region - Release a PCI bar
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8d2f400..eb24ef9 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1993,6 +1993,7 @@ err_out:
>  	kfree(b);
>  	return NULL;
>  }
> +EXPORT_SYMBOL(pci_create_root_bus);
>  
>  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
>  {
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 8bd76c9..c523159 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -139,6 +139,7 @@ void pci_stop_root_bus(struct pci_bus *bus)
>  	/* stop the host bridge */
>  	device_release_driver(&host_bridge->dev);
>  }
> +EXPORT_SYMBOL(pci_stop_root_bus);
>  
>  void pci_remove_root_bus(struct pci_bus *bus)
>  {
> @@ -158,3 +159,4 @@ void pci_remove_root_bus(struct pci_bus *bus)
>  	/* remove the host bridge */
>  	device_unregister(&host_bridge->dev);
>  }
> +EXPORT_SYMBOL(pci_remove_root_bus);
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index e3e17f3..932c9e5 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1750,3 +1750,4 @@ void pci_assign_unassigned_bus_resources(struct pci_bus *bus)
>  	__pci_bus_assign_resources(bus, &add_list, NULL);
>  	BUG_ON(!list_empty(&add_list));
>  }
> +EXPORT_SYMBOL(pci_assign_unassigned_bus_resources);
> diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
> index 4e2d595..9d2bbb6 100644
> --- a/drivers/pci/setup-irq.c
> +++ b/drivers/pci/setup-irq.c
> @@ -65,3 +65,4 @@ void pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
>  	for_each_pci_dev(dev)
>  		pdev_fixup_irq(dev, swizzle, map_irq);
>  }
> +EXPORT_SYMBOL(pci_fixup_irqs);
> -- 
> 1.7.9.5
> 

Re: [PATCH v5 1/4] PCI: Export symbols of PCI functions

[Ray Jui]

Hi Bjorn,

On 3/10/2015 1:56 PM, Bjorn Helgaas wrote:
> On Mon, Mar 09, 2015 at 05:38:03PM -0700, Ray Jui wrote:
>> Export symbols of the following PCI functions so they can be referenced
>> by a PCI driver compiled as a kernel loadable module:
>>
>> pci_common_swizzle
>> pci_create_root_bus
>> pci_stop_root_bus
>> pci_remove_root_bus
>> pci_assign_unassigned_bus_resources
>> pci_fixup_irqs
> 
> Have you tested your driver as a loadable module?
> 
> If it works to load/unload/reload/etc., I'm OK with exporting these
> symbols.  I would prefer EXPORT_SYMBOL_GPL, though, because these don't
> constitute a very coherent interface right now, and they will likely be
> reworked.  For example, see Yijing Wang's work to sort out the
> pci_bus_add_devices() / resource assignment mess.
> 
> Bjorn

Yes, I tested the driver with load/unload/reload on Cygnus. Console logs
are included in my reply to Arnd's email from another email thread.

I'll wait for more feedback from you on the iProc PCIe driver itself.
But if you have reviewed them and think they are okay then I can go
ahead and submit v6 with changes to use EXPORT_SYMBOL_GPL.

Let me know.

Thanks,

Ray

Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support

[Bjorn Helgaas]

[+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@broadcom.com>
> Reviewed-by: Scott Branden <sbraden@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 0000000..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;
> +	}
> +
> +	ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &iobase);
> +	if (ret) {
> +		dev_err(pcie->dev,
> +			"unable to get PCI host bridge resources\n");
> +		return ret;
> +	}
> +
> +	pcie->resources = &res;
> +
> +	ret = iproc_pcie_setup(pcie);
> +	if (ret) {
> +		dev_err(pcie->dev, "PCIe controller setup failed\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
> +{
> +	struct iproc_pcie *pcie = platform_get_drvdata(pdev);
> +
> +	return iproc_pcie_remove(pcie);
> +}
> +
> +static const struct of_device_id iproc_pcie_of_match_table[] = {
> +	{ .compatible = "brcm,iproc-pcie", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, iproc_pcie_of_match_table);
> +
> +static struct platform_driver iproc_pcie_pltfm_driver = {
> +	.driver = {
> +		.name = "iproc-pcie",
> +		.of_match_table = of_match_ptr(iproc_pcie_of_match_table),
> +	},
> +	.probe = iproc_pcie_pltfm_probe,
> +	.remove = iproc_pcie_pltfm_remove,
> +};
> +module_platform_driver(iproc_pcie_pltfm_driver);
> +
> +MODULE_AUTHOR("Ray Jui <rjui@broadcom.com>");
> +MODULE_DESCRIPTION("Broadcom iPROC PCIe platform driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> new file mode 100644
> index 0000000..ad6c89e
> --- /dev/null
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -0,0 +1,351 @@
> +/*
> + * Copyright (C) 2014 Hauke Mehrtens <hauke@hauke-m.de>
> + * Copyright (C) 2015 Broadcom Corporatcommon ion
> + *
> + * 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/msi.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/mbus.h>
> +#include <linux/slab.h>
> +#include <linux/delay.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"
> +
> +#define CLK_CONTROL_OFFSET           0x000
> +#define EP_MODE_SURVIVE_PERST_SHIFT  1
> +#define EP_MODE_SURVIVE_PERST        BIT(EP_MODE_SURVIVE_PERST_SHIFT)
> +#define RC_PCIE_RST_OUTPUT_SHIFT     0
> +#define RC_PCIE_RST_OUTPUT           BIT(RC_PCIE_RST_OUTPUT_SHIFT)
> +
> +#define CFG_IND_ADDR_OFFSET          0x120
> +#define CFG_IND_ADDR_MASK            0x00001ffc
> +
> +#define CFG_IND_DATA_OFFSET          0x124
> +
> +#define CFG_ADDR_OFFSET              0x1f8
> +#define CFG_ADDR_BUS_NUM_SHIFT       20
> +#define CFG_ADDR_BUS_NUM_MASK        0x0ff00000
> +#define CFG_ADDR_DEV_NUM_SHIFT       15
> +#define CFG_ADDR_DEV_NUM_MASK        0x000f8000
> +#define CFG_ADDR_FUNC_NUM_SHIFT      12
> +#define CFG_ADDR_FUNC_NUM_MASK       0x00007000
> +#define CFG_ADDR_REG_NUM_SHIFT       2
> +#define CFG_ADDR_REG_NUM_MASK        0x00000ffc
> +#define CFG_ADDR_CFG_TYPE_SHIFT      0
> +#define CFG_ADDR_CFG_TYPE_MASK       0x00000003
> +
> +#define CFG_DATA_OFFSET              0x1fc
> +
> +#define SYS_RC_INTX_EN               0x330
> +#define SYS_RC_INTX_MASK             0xf
> +
> +static inline struct iproc_pcie *sys_to_pcie(struct pci_sys_data *sys)
> +{
> +	return sys->private_data;
> +}
> +
> +#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.

> +	/* root complex access */
> +	if (busno == 0) {
> +		if (slot >= 1)
> +			return INVALID_ACCESS_OFFSET;
> +		writel(where & CFG_IND_ADDR_MASK,
> +		       pcie->base + CFG_IND_ADDR_OFFSET);
> +		return CFG_IND_DATA_OFFSET;
> +	}
> +
> +	if (fn > 1)
> +		return INVALID_ACCESS_OFFSET;
> +
> +	/* EP device access */
> +	val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
> +		(slot << CFG_ADDR_DEV_NUM_SHIFT) |
> +		(fn << CFG_ADDR_FUNC_NUM_SHIFT) |
> +		(where & CFG_ADDR_REG_NUM_MASK) |
> +		(1 & CFG_ADDR_CFG_TYPE_MASK);
> +	writel(val, pcie->base + CFG_ADDR_OFFSET);
> +
> +	return CFG_DATA_OFFSET;
> +}
> +
> +#define INVALID_CFG_RD 0xffffffff
> +static int iproc_pcie_read_conf(struct iproc_pcie *pcie, int busno,
> +				unsigned int devfn, int where, int size,
> +				u32 *val)
> +{
> +	u32 offset, mask, shift;
> +
> +	*val = INVALID_CFG_RD;
> +
> +	if (size != 1 && size != 2 && size != 4)
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	else if ((size == 2) && (where & 1))
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	else if ((size == 4) && (where & 3))
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +
> +	offset = iproc_pcie_cfg_base(pcie, busno, devfn, where);
> +	if (offset == INVALID_ACCESS_OFFSET)
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +
> +	*val = readl(pcie->base + offset);
> +
> +	if (size != 4) {
> +		mask = (1 << (size * BITS_PER_BYTE)) - 1;
> +		shift = (where % 4) * BITS_PER_BYTE;
> +		*val = (*val >> shift) & mask;
> +	}
> +
> +	dev_dbg(pcie->dev,
> +		"conf rd: busn=%d devfn=%u where=%d size=%d val=0x%08x\n",
> +		busno, devfn, where, size, *val);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}

This looks like maybe it could use Rob Herring's (CC'd) new generic config
accessors?  For example, see these:

  1f94a94f67e1 ("PCI: Add generic config accessors")
  21186728865e ("PCI: generic: Convert to use generic config accessors")

> +
> +static int iproc_pcie_write_conf(struct iproc_pcie *pcie, int busno,
> +				 unsigned int devfn, int where, int size,
> +				 u32 val)
> +{
> +	u32 offset, mask, shift, data;
> +
> +	if (size != 1 && size != 2 && size != 4)
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	else if ((size == 2) && (where & 1))
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	else if ((size == 4) && (where & 3))
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +
> +	offset = iproc_pcie_cfg_base(pcie, busno, devfn, where);
> +	if (offset == INVALID_ACCESS_OFFSET)
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +
> +	data = readl(pcie->base + offset);
> +
> +	if (size != 4) {
> +		mask = (1 << (size * BITS_PER_BYTE)) - 1;
> +		shift = (where % 4) * BITS_PER_BYTE;
> +		data &= ~(mask << shift);
> +		data |= (val & mask) << shift;
> +	} else {
> +		data = val;
> +	}
> +
> +	writel(data, pcie->base + offset);
> +
> +	dev_dbg(pcie->dev,
> +		"conf wr: busn=%d devfn=%u where=%d size=%d data=0x%08x\n",
> +		busno, devfn, where, size, data);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int iproc_pcie_read(struct pci_bus *bus, unsigned int devfn,
> +			   int where, int size, u32 *val)
> +{
> +	struct iproc_pcie *pcie = sys_to_pcie(bus->sysdata);
> +
> +	return iproc_pcie_read_conf(pcie, bus->number, devfn, where, size,
> +				    val);
> +}
> +
> +static int iproc_pcie_write(struct pci_bus *bus, unsigned int devfn,
> +			    int where, int size, u32 val)
> +{
> +	struct iproc_pcie *pcie = sys_to_pcie(bus->sysdata);
> +
> +	return iproc_pcie_write_conf(pcie, bus->number, devfn, where, size,
> +				     val);
> +}
> +
> +static struct pci_ops iproc_pcie_ops = {
> +	.read = iproc_pcie_read,
> +	.write = iproc_pcie_write,
> +};
> +
> +static void iproc_pcie_reset(struct iproc_pcie *pcie)
> +{
> +	u32 val;
> +
> +	/*
> +	 * Configure the PCIe controller as root complex and send a downstream
> +	 * reset
> +	 */
> +	val = EP_MODE_SURVIVE_PERST | RC_PCIE_RST_OUTPUT;
> +	writel(val, pcie->base + CLK_CONTROL_OFFSET);
> +	udelay(250);
> +	val &= ~EP_MODE_SURVIVE_PERST;
> +	writel(val, pcie->base + CLK_CONTROL_OFFSET);
> +	msleep(250);
> +}
> +
> +static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
> +{
> +	u8 hdr_type;
> +	u32 link_ctrl;
> +	u16 pos, link_status;
> +	int link_is_active = 0;
> +
> +	/* make sure we are not in EP mode */
> +	pci_bus_read_config_byte(bus, 0, PCI_HEADER_TYPE, &hdr_type);
> +	if ((hdr_type & 0x7f) != PCI_HEADER_TYPE_BRIDGE) {
> +		dev_err(pcie->dev, "in EP mode, hdr=0x08%x\n", hdr_type);
> +		return -EFAULT;
> +	}
> +
> +	/* force class to PCI_CLASS_BRIDGE_PCI (0x0604) */
> +	pci_bus_write_config_word(bus, 0, PCI_CLASS_DEVICE,
> +				  PCI_CLASS_BRIDGE_PCI);
> +
> +	/* check link status to see if link is active */
> +	pos = pci_bus_find_capability(bus, 0, PCI_CAP_ID_EXP);
> +	pci_bus_read_config_word(bus, 0, pos + PCI_EXP_LNKSTA, &link_status);
> +	if (link_status & PCI_EXP_LNKSTA_NLW)
> +		link_is_active = 1;
> +
> +	if (!link_is_active) {
> +		/* try GEN 1 link speed */
> +#define PCI_LINK_STATUS_CTRL_2_OFFSET 0x0dc
> +#define PCI_TARGET_LINK_SPEED_MASK    0xf
> +#define PCI_TARGET_LINK_SPEED_GEN2    0x2
> +#define PCI_TARGET_LINK_SPEED_GEN1    0x1
> +		pci_bus_read_config_dword(bus, 0,
> +					  PCI_LINK_STATUS_CTRL_2_OFFSET,
> +					  &link_ctrl);
> +		if ((link_ctrl & PCI_TARGET_LINK_SPEED_MASK) ==
> +		    PCI_TARGET_LINK_SPEED_GEN2) {
> +			link_ctrl &= ~PCI_TARGET_LINK_SPEED_MASK;
> +			link_ctrl |= PCI_TARGET_LINK_SPEED_GEN1;
> +			pci_bus_write_config_dword(bus, 0,
> +					   PCI_LINK_STATUS_CTRL_2_OFFSET,
> +					   link_ctrl);
> +			msleep(100);
> +
> +			pos = pci_bus_find_capability(bus, 0, PCI_CAP_ID_EXP);
> +			pci_bus_read_config_word(bus, 0, pos + PCI_EXP_LNKSTA,
> +						 &link_status);
> +			if (link_status & PCI_EXP_LNKSTA_NLW)
> +				link_is_active = 1;
> +		}
> +	}
> +
> +	dev_info(pcie->dev, "link: %s\n", link_is_active ? "UP" : "DOWN");
> +
> +	return link_is_active ? 0 : -ENODEV;
> +}
> +
> +static void iproc_pcie_enable(struct iproc_pcie *pcie)
> +{
> +	writel(SYS_RC_INTX_MASK, pcie->base + SYS_RC_INTX_EN);
> +}
> +
> +int iproc_pcie_setup(struct iproc_pcie *pcie)
> +{
> +	int ret;
> +	struct pci_bus *bus;
> +
> +	if (!pcie || !pcie->dev || !pcie->base)
> +		return -EINVAL;
> +
> +	if (pcie->phy) {
> +		ret = phy_init(pcie->phy);
> +		if (ret) {
> +			dev_err(pcie->dev, "unable to initialize PCIe PHY\n");
> +			return ret;
> +		}
> +
> +		ret = phy_power_on(pcie->phy);
> +		if (ret) {
> +			dev_err(pcie->dev, "unable to power on PCIe PHY\n");
> +			goto err_exit_phy;
> +		}
> +
> +	}
> +
> +	iproc_pcie_reset(pcie);
> +
> +	pcie->sysdata.private_data = pcie;
> +
> +	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.

> +
> +	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 <rjui@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 0000000..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
> 

Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support

[Ray Jui]

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@broadcom.com>
>> Reviewed-by: Scott Branden <sbraden@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 0000000..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;
>> +	}
>> +
>> +	ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &iobase);
>> +	if (ret) {
>> +		dev_err(pcie->dev,
>> +			"unable to get PCI host bridge resources\n");
>> +		return ret;
>> +	}
>> +
>> +	pcie->resources = &res;
>> +
>> +	ret = iproc_pcie_setup(pcie);
>> +	if (ret) {
>> +		dev_err(pcie->dev, "PCIe controller setup failed\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
>> +{
>> +	struct iproc_pcie *pcie = platform_get_drvdata(pdev);
>> +
>> +	return iproc_pcie_remove(pcie);
>> +}
>> +
>> +static const struct of_device_id iproc_pcie_of_match_table[] = {
>> +	{ .compatible = "brcm,iproc-pcie", },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, iproc_pcie_of_match_table);
>> +
>> +static struct platform_driver iproc_pcie_pltfm_driver = {
>> +	.driver = {
>> +		.name = "iproc-pcie",
>> +		.of_match_table = of_match_ptr(iproc_pcie_of_match_table),
>> +	},
>> +	.probe = iproc_pcie_pltfm_probe,
>> +	.remove = iproc_pcie_pltfm_remove,
>> +};
>> +module_platform_driver(iproc_pcie_pltfm_driver);
>> +
>> +MODULE_AUTHOR("Ray Jui <rjui@broadcom.com>");
>> +MODULE_DESCRIPTION("Broadcom iPROC PCIe platform driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> new file mode 100644
>> index 0000000..ad6c89e
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -0,0 +1,351 @@
>> +/*
>> + * Copyright (C) 2014 Hauke Mehrtens <hauke@hauke-m.de>
>> + * Copyright (C) 2015 Broadcom Corporatcommon ion
>> + *
>> + * 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/msi.h>
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/mbus.h>
>> +#include <linux/slab.h>
>> +#include <linux/delay.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"
>> +
>> +#define CLK_CONTROL_OFFSET           0x000
>> +#define EP_MODE_SURVIVE_PERST_SHIFT  1
>> +#define EP_MODE_SURVIVE_PERST        BIT(EP_MODE_SURVIVE_PERST_SHIFT)
>> +#define RC_PCIE_RST_OUTPUT_SHIFT     0
>> +#define RC_PCIE_RST_OUTPUT           BIT(RC_PCIE_RST_OUTPUT_SHIFT)
>> +
>> +#define CFG_IND_ADDR_OFFSET          0x120
>> +#define CFG_IND_ADDR_MASK            0x00001ffc
>> +
>> +#define CFG_IND_DATA_OFFSET          0x124
>> +
>> +#define CFG_ADDR_OFFSET              0x1f8
>> +#define CFG_ADDR_BUS_NUM_SHIFT       20
>> +#define CFG_ADDR_BUS_NUM_MASK        0x0ff00000
>> +#define CFG_ADDR_DEV_NUM_SHIFT       15
>> +#define CFG_ADDR_DEV_NUM_MASK        0x000f8000
>> +#define CFG_ADDR_FUNC_NUM_SHIFT      12
>> +#define CFG_ADDR_FUNC_NUM_MASK       0x00007000
>> +#define CFG_ADDR_REG_NUM_SHIFT       2
>> +#define CFG_ADDR_REG_NUM_MASK        0x00000ffc
>> +#define CFG_ADDR_CFG_TYPE_SHIFT      0
>> +#define CFG_ADDR_CFG_TYPE_MASK       0x00000003
>> +
>> +#define CFG_DATA_OFFSET              0x1fc
>> +
>> +#define SYS_RC_INTX_EN               0x330
>> +#define SYS_RC_INTX_MASK             0xf
>> +
>> +static inline struct iproc_pcie *sys_to_pcie(struct pci_sys_data *sys)
>> +{
>> +	return sys->private_data;
>> +}
>> +
>> +#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?

>> +	/* root complex access */
>> +	if (busno == 0) {
>> +		if (slot >= 1)
>> +			return INVALID_ACCESS_OFFSET;
>> +		writel(where & CFG_IND_ADDR_MASK,
>> +		       pcie->base + CFG_IND_ADDR_OFFSET);
>> +		return CFG_IND_DATA_OFFSET;
>> +	}
>> +
>> +	if (fn > 1)
>> +		return INVALID_ACCESS_OFFSET;
>> +
>> +	/* EP device access */
>> +	val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
>> +		(slot << CFG_ADDR_DEV_NUM_SHIFT) |
>> +		(fn << CFG_ADDR_FUNC_NUM_SHIFT) |
>> +		(where & CFG_ADDR_REG_NUM_MASK) |
>> +		(1 & CFG_ADDR_CFG_TYPE_MASK);
>> +	writel(val, pcie->base + CFG_ADDR_OFFSET);
>> +
>> +	return CFG_DATA_OFFSET;
>> +}
>> +
>> +#define INVALID_CFG_RD 0xffffffff
>> +static int iproc_pcie_read_conf(struct iproc_pcie *pcie, int busno,
>> +				unsigned int devfn, int where, int size,
>> +				u32 *val)
>> +{
>> +	u32 offset, mask, shift;
>> +
>> +	*val = INVALID_CFG_RD;
>> +
>> +	if (size != 1 && size != 2 && size != 4)
>> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>> +	else if ((size == 2) && (where & 1))
>> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>> +	else if ((size == 4) && (where & 3))
>> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>> +
>> +	offset = iproc_pcie_cfg_base(pcie, busno, devfn, where);
>> +	if (offset == INVALID_ACCESS_OFFSET)
>> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>> +
>> +	*val = readl(pcie->base + offset);
>> +
>> +	if (size != 4) {
>> +		mask = (1 << (size * BITS_PER_BYTE)) - 1;
>> +		shift = (where % 4) * BITS_PER_BYTE;
>> +		*val = (*val >> shift) & mask;
>> +	}
>> +
>> +	dev_dbg(pcie->dev,
>> +		"conf rd: busn=%d devfn=%u where=%d size=%d val=0x%08x\n",
>> +		busno, devfn, where, size, *val);
>> +
>> +	return PCIBIOS_SUCCESSFUL;
>> +}
> 
> This looks like maybe it could use Rob Herring's (CC'd) new generic config
> accessors?  For example, see these:
> 
>   1f94a94f67e1 ("PCI: Add generic config accessors")
>   21186728865e ("PCI: generic: Convert to use generic config accessors")
> 

Okay I will switch to use the generic code for config space access. I'll
add map_bus callback for address calculation.

>> +
>> +static int iproc_pcie_write_conf(struct iproc_pcie *pcie, int busno,
>> +				 unsigned int devfn, int where, int size,
>> +				 u32 val)
>> +{
>> +	u32 offset, mask, shift, data;
>> +
>> +	if (size != 1 && size != 2 && size != 4)
>> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>> +	else if ((size == 2) && (where & 1))
>> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>> +	else if ((size == 4) && (where & 3))
>> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>> +
>> +	offset = iproc_pcie_cfg_base(pcie, busno, devfn, where);
>> +	if (offset == INVALID_ACCESS_OFFSET)
>> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>> +
>> +	data = readl(pcie->base + offset);
>> +
>> +	if (size != 4) {
>> +		mask = (1 << (size * BITS_PER_BYTE)) - 1;
>> +		shift = (where % 4) * BITS_PER_BYTE;
>> +		data &= ~(mask << shift);
>> +		data |= (val & mask) << shift;
>> +	} else {
>> +		data = val;
>> +	}
>> +
>> +	writel(data, pcie->base + offset);
>> +
>> +	dev_dbg(pcie->dev,
>> +		"conf wr: busn=%d devfn=%u where=%d size=%d data=0x%08x\n",
>> +		busno, devfn, where, size, data);
>> +
>> +	return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>> +static int iproc_pcie_read(struct pci_bus *bus, unsigned int devfn,
>> +			   int where, int size, u32 *val)
>> +{
>> +	struct iproc_pcie *pcie = sys_to_pcie(bus->sysdata);
>> +
>> +	return iproc_pcie_read_conf(pcie, bus->number, devfn, where, size,
>> +				    val);
>> +}
>> +
>> +static int iproc_pcie_write(struct pci_bus *bus, unsigned int devfn,
>> +			    int where, int size, u32 val)
>> +{
>> +	struct iproc_pcie *pcie = sys_to_pcie(bus->sysdata);
>> +
>> +	return iproc_pcie_write_conf(pcie, bus->number, devfn, where, size,
>> +				     val);
>> +}
>> +
>> +static struct pci_ops iproc_pcie_ops = {
>> +	.read = iproc_pcie_read,
>> +	.write = iproc_pcie_write,
>> +};
>> +
>> +static void iproc_pcie_reset(struct iproc_pcie *pcie)
>> +{
>> +	u32 val;
>> +
>> +	/*
>> +	 * Configure the PCIe controller as root complex and send a downstream
>> +	 * reset
>> +	 */
>> +	val = EP_MODE_SURVIVE_PERST | RC_PCIE_RST_OUTPUT;
>> +	writel(val, pcie->base + CLK_CONTROL_OFFSET);
>> +	udelay(250);
>> +	val &= ~EP_MODE_SURVIVE_PERST;
>> +	writel(val, pcie->base + CLK_CONTROL_OFFSET);
>> +	msleep(250);
>> +}
>> +
>> +static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
>> +{
>> +	u8 hdr_type;
>> +	u32 link_ctrl;
>> +	u16 pos, link_status;
>> +	int link_is_active = 0;
>> +
>> +	/* make sure we are not in EP mode */
>> +	pci_bus_read_config_byte(bus, 0, PCI_HEADER_TYPE, &hdr_type);
>> +	if ((hdr_type & 0x7f) != PCI_HEADER_TYPE_BRIDGE) {
>> +		dev_err(pcie->dev, "in EP mode, hdr=0x08%x\n", hdr_type);
>> +		return -EFAULT;
>> +	}
>> +
>> +	/* force class to PCI_CLASS_BRIDGE_PCI (0x0604) */
>> +	pci_bus_write_config_word(bus, 0, PCI_CLASS_DEVICE,
>> +				  PCI_CLASS_BRIDGE_PCI);
>> +
>> +	/* check link status to see if link is active */
>> +	pos = pci_bus_find_capability(bus, 0, PCI_CAP_ID_EXP);
>> +	pci_bus_read_config_word(bus, 0, pos + PCI_EXP_LNKSTA, &link_status);
>> +	if (link_status & PCI_EXP_LNKSTA_NLW)
>> +		link_is_active = 1;
>> +
>> +	if (!link_is_active) {
>> +		/* try GEN 1 link speed */
>> +#define PCI_LINK_STATUS_CTRL_2_OFFSET 0x0dc
>> +#define PCI_TARGET_LINK_SPEED_MASK    0xf
>> +#define PCI_TARGET_LINK_SPEED_GEN2    0x2
>> +#define PCI_TARGET_LINK_SPEED_GEN1    0x1
>> +		pci_bus_read_config_dword(bus, 0,
>> +					  PCI_LINK_STATUS_CTRL_2_OFFSET,
>> +					  &link_ctrl);
>> +		if ((link_ctrl & PCI_TARGET_LINK_SPEED_MASK) ==
>> +		    PCI_TARGET_LINK_SPEED_GEN2) {
>> +			link_ctrl &= ~PCI_TARGET_LINK_SPEED_MASK;
>> +			link_ctrl |= PCI_TARGET_LINK_SPEED_GEN1;
>> +			pci_bus_write_config_dword(bus, 0,
>> +					   PCI_LINK_STATUS_CTRL_2_OFFSET,
>> +					   link_ctrl);
>> +			msleep(100);
>> +
>> +			pos = pci_bus_find_capability(bus, 0, PCI_CAP_ID_EXP);
>> +			pci_bus_read_config_word(bus, 0, pos + PCI_EXP_LNKSTA,
>> +						 &link_status);
>> +			if (link_status & PCI_EXP_LNKSTA_NLW)
>> +				link_is_active = 1;
>> +		}
>> +	}
>> +
>> +	dev_info(pcie->dev, "link: %s\n", link_is_active ? "UP" : "DOWN");
>> +
>> +	return link_is_active ? 0 : -ENODEV;
>> +}
>> +
>> +static void iproc_pcie_enable(struct iproc_pcie *pcie)
>> +{
>> +	writel(SYS_RC_INTX_MASK, pcie->base + SYS_RC_INTX_EN);
>> +}
>> +
>> +int iproc_pcie_setup(struct iproc_pcie *pcie)
>> +{
>> +	int ret;
>> +	struct pci_bus *bus;
>> +
>> +	if (!pcie || !pcie->dev || !pcie->base)
>> +		return -EINVAL;
>> +
>> +	if (pcie->phy) {
>> +		ret = phy_init(pcie->phy);
>> +		if (ret) {
>> +			dev_err(pcie->dev, "unable to initialize PCIe PHY\n");
>> +			return ret;
>> +		}
>> +
>> +		ret = phy_power_on(pcie->phy);
>> +		if (ret) {
>> +			dev_err(pcie->dev, "unable to power on PCIe PHY\n");
>> +			goto err_exit_phy;
>> +		}
>> +
>> +	}
>> +
>> +	iproc_pcie_reset(pcie);
>> +
>> +	pcie->sysdata.private_data = pcie;
>> +
>> +	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.
> 

Right. I will fix this.

>> +
>> +	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.
> 

Understood. Let's decide whether or not to keep everything in one file.

>> +
>> +MODULE_AUTHOR("Ray Jui <rjui@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 0000000..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 for the review!

Ray

Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support

[Arnd Bergmann]

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

Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support

[Bjorn Helgaas]

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@broadcom.com>
> >> Reviewed-by: Scott Branden <sbraden@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 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

Re: [PATCH v5 3/4] pci: iproc: Add Broadcom iProc PCIe support

[Yijing Wang]

...
>> +	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 <rjui@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 0000000..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