[PATCH v3 0/4] Add Broadcom North Star 2 support

[PATCH v3 0/4] Add Broadcom North Star 2 support

[Ray Jui]

This patch series adds Broadcom North Star 2 (NS2) SoC support. NS2 is an ARMv8
based SoC and under the Broadcom iProc family.

Sorry for tying this with the Broadcom iProc PCIe driver fixes for ARM64. I
have to tie them together because iProc PCIe support is enabled by default
when ARCH_BCM_IPROC is enabled. Without the fixes in the iProc PCIe driver,
enabling CONFIG_ARCH_BCM_IPROC would break the build for arm64 defconfig. Let
me know if there's a better way to handle this.

This patch series is generated based on v4.2-rc2 and tested on Broadcom NS2 SVK

Code available on GITHUB: https://github.com/Broadcom/arm64-linux.git
branch is ns2-core-v3

Changes from V2:
- Drop hardcoded earlycon kernel command line paramter in NS2 SVK dts file
because 1) earlycon is a debugging feature that can be enabled in the
bootloader and should not be enabled by default in the board dts file and 2)
of_earlycon should be used and support should be added to 8250 DW driver

Changes from V1:
- Took Arnd's advice to tweak the location of struct pci_sys_data within
struct iproc_pcie. This helps to get rid of most of the CONFIG_ARM wrap in
iProc PCIe core driver
- Use stdout-path and alias for serial console in NS2 SVK dts
- Add all 4 CPU descriptions in NS2 dtsi
- Remove "clock-frequency" property in the armv8 timer node so timer frequency
can be determined based on readings from CNTFRQ_EL0
- Remove config flag ARCH_BCM_NS2. Leave only ARCH_BCM_IPROC for all Broadcom
arm64 SoCs as advised

Ray Jui (4):
  PCI: iproc: enable arm64 support for iProc PCIe
  PCI: iproc: Fix ARM64 dependency in Kconfig
  arm64: Add Broadcom iProc family support
  arm64: dts: Add Broadcom North Star 2 support

 Documentation/devicetree/bindings/arm/bcm/ns2.txt |    9 ++
 arch/arm64/Kconfig                                |    5 +
 arch/arm64/boot/dts/Makefile                      |    1 +
 arch/arm64/boot/dts/broadcom/Makefile             |    5 +
 arch/arm64/boot/dts/broadcom/ns2-svk.dts          |   59 +++++++++++
 arch/arm64/boot/dts/broadcom/ns2.dtsi             |  118 +++++++++++++++++++++
 arch/arm64/configs/defconfig                      |    2 +
 drivers/pci/host/Kconfig                          |    2 +-
 drivers/pci/host/pcie-iproc.c                     |   15 +--
 drivers/pci/host/pcie-iproc.h                     |    8 +-
 10 files changed, 210 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/bcm/ns2.txt
 create mode 100644 arch/arm64/boot/dts/broadcom/Makefile
 create mode 100644 arch/arm64/boot/dts/broadcom/ns2-svk.dts
 create mode 100644 arch/arm64/boot/dts/broadcom/ns2.dtsi

-- 
1.7.9.5

[PATCH v3 1/4] PCI: iproc: enable arm64 support for iProc PCIe

[Ray Jui]

This patch enables arm64 support to the iProc PCIe driver

Signed-off-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/pci/host/pcie-iproc.c |   15 ++++-----------
 drivers/pci/host/pcie-iproc.h |    8 ++++++--
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index d77481e..8a556d5 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -58,11 +58,6 @@
 #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;
-}
-
 /**
  * Note access to the configuration registers are protected at the higher layer
  * by 'pci_lock' in drivers/pci/access.c
@@ -71,8 +66,7 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
 					    unsigned int devfn,
 					    int where)
 {
-	struct pci_sys_data *sys = bus->sysdata;
-	struct iproc_pcie *pcie = sys_to_pcie(sys);
+	struct iproc_pcie *pcie = bus->sysdata;
 	unsigned slot = PCI_SLOT(devfn);
 	unsigned fn = PCI_FUNC(devfn);
 	unsigned busno = bus->number;
@@ -208,10 +202,7 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 
 	iproc_pcie_reset(pcie);
 
-	pcie->sysdata.private_data = pcie;
-
-	bus = pci_create_root_bus(pcie->dev, 0, &iproc_pcie_ops,
-				  &pcie->sysdata, res);
+	bus = pci_create_root_bus(pcie->dev, 0, &iproc_pcie_ops, pcie, res);
 	if (!bus) {
 		dev_err(pcie->dev, "unable to create PCI root bus\n");
 		ret = -ENOMEM;
@@ -229,7 +220,9 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 
 	pci_scan_child_bus(bus);
 	pci_assign_unassigned_bus_resources(bus);
+#ifdef CONFIG_ARM
 	pci_fixup_irqs(pci_common_swizzle, pcie->map_irq);
+#endif
 	pci_bus_add_devices(bus);
 
 	return 0;
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index ba0a108..0ee9673 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -18,18 +18,22 @@
 
 /**
  * iProc PCIe device
+ * @sysdata: Per PCI controller data. This needs to be kept at the beginning of
+ * struct iproc_pcie, to enable support of both ARM32 and ARM64 platforms with
+ * minimal changes in the iProc PCIe core driver
  * @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 {
+#ifdef CONFIG_ARM
+	struct pci_sys_data sysdata;
+#endif
 	struct device *dev;
 	void __iomem *base;
-	struct pci_sys_data sysdata;
 	struct pci_bus *root_bus;
 	struct phy *phy;
 	int irqs[IPROC_PCIE_MAX_NUM_IRQS];
-- 
1.7.9.5

[PATCH v3 2/4] PCI: iproc: Fix ARM64 dependency in Kconfig

[Ray Jui]

Allow Broadcom iProc PCIe core driver to be compiled for ARM64

Signed-off-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Vikram Prakash <vikramp@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/pci/host/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index c132bdd..d2c6144 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -117,7 +117,7 @@ config PCI_VERSATILE
 
 config PCIE_IPROC
 	tristate "Broadcom iProc PCIe controller"
-	depends on OF && ARM
+	depends on OF && (ARM || ARM64)
 	default n
 	help
 	  This enables the iProc PCIe core controller support for Broadcom's
-- 
1.7.9.5

[PATCH v3 3/4] arm64: Add Broadcom iProc family support

[Ray Jui]

This patch adds support to Broadcom's iProc family of arm64 based SoCs
in the arm64 Kconfig and defconfig files

Signed-off-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 arch/arm64/Kconfig           |    5 +++++
 arch/arm64/configs/defconfig |    2 ++
 2 files changed, 7 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 318175f..969ef4a 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -162,6 +162,11 @@ source "kernel/Kconfig.freezer"
 
 menu "Platform selection"
 
+config ARCH_BCM_IPROC
+	bool "Broadcom iProc SoC Family"
+	help
+	  This enables support for Broadcom iProc based SoCs
+
 config ARCH_EXYNOS
 	bool
 	help
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 4e17e7e..c83d51f 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -31,6 +31,7 @@ CONFIG_MODULES=y
 CONFIG_MODULE_UNLOAD=y
 # CONFIG_BLK_DEV_BSG is not set
 # CONFIG_IOSCHED_DEADLINE is not set
+CONFIG_ARCH_BCM_IPROC=y
 CONFIG_ARCH_EXYNOS7=y
 CONFIG_ARCH_FSL_LS2085A=y
 CONFIG_ARCH_HISI=y
@@ -102,6 +103,7 @@ CONFIG_SERIO_AMBAKMI=y
 CONFIG_LEGACY_PTY_COUNT=16
 CONFIG_SERIAL_8250=y
 CONFIG_SERIAL_8250_CONSOLE=y
+CONFIG_SERIAL_8250_DW=y
 CONFIG_SERIAL_8250_MT6577=y
 CONFIG_SERIAL_AMBA_PL011=y
 CONFIG_SERIAL_AMBA_PL011_CONSOLE=y
-- 
1.7.9.5

[PATCH v3 4/4] arm64: dts: Add Broadcom North Star 2 support

[Ray Jui]

Add Broadcom NS2 device tree binding document. Also add initial device
tree dtsi for Broadcom North Star 2 (NS2) SoC and board support for NS2
SVK board

Signed-off-by: Jon Mason <jonmason@broadcom.com>
Signed-off-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 Documentation/devicetree/bindings/arm/bcm/ns2.txt |    9 ++
 arch/arm64/boot/dts/Makefile                      |    1 +
 arch/arm64/boot/dts/broadcom/Makefile             |    5 +
 arch/arm64/boot/dts/broadcom/ns2-svk.dts          |   59 +++++++++++
 arch/arm64/boot/dts/broadcom/ns2.dtsi             |  118 +++++++++++++++++++++
 5 files changed, 192 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/bcm/ns2.txt
 create mode 100644 arch/arm64/boot/dts/broadcom/Makefile
 create mode 100644 arch/arm64/boot/dts/broadcom/ns2-svk.dts
 create mode 100644 arch/arm64/boot/dts/broadcom/ns2.dtsi

diff --git a/Documentation/devicetree/bindings/arm/bcm/ns2.txt b/Documentation/devicetree/bindings/arm/bcm/ns2.txt
new file mode 100644
index 0000000..35f056f
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/bcm/ns2.txt
@@ -0,0 +1,9 @@
+Broadcom North Star 2 (NS2) device tree bindings
+------------------------------------------------
+
+Boards with NS2 shall have the following properties:
+
+Required root node property:
+
+NS2 SVK board
+compatible = "brcm,ns2-svk", "brcm,ns2";
diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
index 38913be..9f95941 100644
--- a/arch/arm64/boot/dts/Makefile
+++ b/arch/arm64/boot/dts/Makefile
@@ -1,6 +1,7 @@
 dts-dirs += amd
 dts-dirs += apm
 dts-dirs += arm
+dts-dirs += broadcom
 dts-dirs += cavium
 dts-dirs += exynos
 dts-dirs += freescale
diff --git a/arch/arm64/boot/dts/broadcom/Makefile b/arch/arm64/boot/dts/broadcom/Makefile
new file mode 100644
index 0000000..e21fe66
--- /dev/null
+++ b/arch/arm64/boot/dts/broadcom/Makefile
@@ -0,0 +1,5 @@
+dtb-$(CONFIG_ARCH_BCM_IPROC) += ns2-svk.dtb
+
+always		:= $(dtb-y)
+subdir-y	:= $(dts-dirs)
+clean-files	:= *.dtb
diff --git a/arch/arm64/boot/dts/broadcom/ns2-svk.dts b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
new file mode 100644
index 0000000..244baf8
--- /dev/null
+++ b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
@@ -0,0 +1,59 @@
+/*
+ *  BSD LICENSE
+ *
+ *  Copyright(c) 2015 Broadcom Corporation.  All rights reserved.
+ *
+ *  Redistribution and use in source and binary forms, with or without
+ *  modification, are permitted provided that the following conditions
+ *  are met:
+ *
+ *    * Redistributions of source code must retain the above copyright
+ *      notice, this list of conditions and the following disclaimer.
+ *    * Redistributions in binary form must reproduce the above copyright
+ *      notice, this list of conditions and the following disclaimer in
+ *      the documentation and/or other materials provided with the
+ *      distribution.
+ *    * Neither the name of Broadcom Corporation nor the names of its
+ *      contributors may be used to endorse or promote products derived
+ *      from this software without specific prior written permission.
+ *
+ *  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *  OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *  LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *  DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *  THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+/dts-v1/;
+
+#include "ns2.dtsi"
+
+/ {
+	model = "Broadcom NS2 SVK";
+	compatible = "brcm,ns2-svk", "brcm,ns2";
+
+	aliases {
+		serial0 = &uart3;
+	};
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	memory {
+		device_type = "memory";
+		reg = <0x000000000 0x80000000 0x00000000 0x40000000>;
+	};
+
+	soc: soc {
+		uart3: serial@66130000 {
+			status = "ok";
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi
new file mode 100644
index 0000000..3c92d92
--- /dev/null
+++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
@@ -0,0 +1,118 @@
+/*
+ *  BSD LICENSE
+ *
+ *  Copyright(c) 2015 Broadcom Corporation.  All rights reserved.
+ *
+ *  Redistribution and use in source and binary forms, with or without
+ *  modification, are permitted provided that the following conditions
+ *  are met:
+ *
+ *    * Redistributions of source code must retain the above copyright
+ *      notice, this list of conditions and the following disclaimer.
+ *    * Redistributions in binary form must reproduce the above copyright
+ *      notice, this list of conditions and the following disclaimer in
+ *      the documentation and/or other materials provided with the
+ *      distribution.
+ *    * Neither the name of Broadcom Corporation nor the names of its
+ *      contributors may be used to endorse or promote products derived
+ *      from this software without specific prior written permission.
+ *
+ *  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *  OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *  LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *  DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *  THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+/memreserve/ 0x84b00000 0x00000008;
+
+/ {
+	compatible = "brcm,ns2";
+	interrupt-parent = <&gic>;
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	cpus {
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a57", "arm,armv8";
+			reg = <0 0>;
+			enable-method = "spin-table";
+			cpu-release-addr = <0 0x84b00000>;
+		};
+
+		cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a57", "arm,armv8";
+			reg = <0 1>;
+			enable-method = "spin-table";
+			cpu-release-addr = <0 0x84b00000>;
+		};
+
+		cpu@2 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a57", "arm,armv8";
+			reg = <0 2>;
+			enable-method = "spin-table";
+			cpu-release-addr = <0 0x84b00000>;
+		};
+
+		cpu@3 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a57", "arm,armv8";
+			reg = <0 3>;
+			enable-method = "spin-table";
+			cpu-release-addr = <0 0x84b00000>;
+		};
+	};
+
+	timer {
+		compatible = "arm,armv8-timer";
+		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_RAW(0xff) |
+			      IRQ_TYPE_EDGE_RISING)>,
+			     <GIC_PPI 14 (GIC_CPU_MASK_RAW(0xff) |
+			      IRQ_TYPE_EDGE_RISING)>,
+			     <GIC_PPI 11 (GIC_CPU_MASK_RAW(0xff) |
+			      IRQ_TYPE_EDGE_RISING)>,
+			     <GIC_PPI 10 (GIC_CPU_MASK_RAW(0xff) |
+			      IRQ_TYPE_EDGE_RISING)>;
+	};
+
+	soc: soc {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0 0 0 0xffffffff>;
+
+		gic: interrupt-controller@65210000 {
+			compatible = "arm,gic-400";
+			#interrupt-cells = <3>;
+			interrupt-controller;
+			reg = <0x65210000 0x1000>,
+			      <0x65220000 0x1000>,
+			      <0x65240000 0x2000>,
+			      <0x65260000 0x1000>;
+		};
+
+		uart3: serial@66130000 {
+			compatible = "snps,dw-apb-uart";
+			reg = <0x66130000 0x100>;
+			interrupts = <GIC_SPI 393 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clock-frequency = <23961600>;
+			status = "disabled";
+		};
+	};
+};
-- 
1.7.9.5

Re: [PATCH v3 1/4] PCI: iproc: enable arm64 support for iProc PCIe

[Bjorn Helgaas]

On Wed, Jul 15, 2015 at 09:39:20PM -0700, Ray Jui wrote:
> This patch enables arm64 support to the iProc PCIe driver

This needs a little more explanation: ARM has a common struct pci_sys_data
but ARM64 does not, and ARM needs pci_fixup_irqs() but ARM64 does not (why
not?), ARM uses the common pci_sys_data for the PCI sysdata while ARM64
uses a driver-specific sysdata, etc.

> Signed-off-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
>  drivers/pci/host/pcie-iproc.c |   15 ++++-----------
>  drivers/pci/host/pcie-iproc.h |    8 ++++++--
>  2 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index d77481e..8a556d5 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -58,11 +58,6 @@
>  #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;
> -}
> -
>  /**
>   * Note access to the configuration registers are protected at the higher layer
>   * by 'pci_lock' in drivers/pci/access.c
> @@ -71,8 +66,7 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
>  					    unsigned int devfn,
>  					    int where)
>  {
> -	struct pci_sys_data *sys = bus->sysdata;
> -	struct iproc_pcie *pcie = sys_to_pcie(sys);
> +	struct iproc_pcie *pcie = bus->sysdata;
>  	unsigned slot = PCI_SLOT(devfn);
>  	unsigned fn = PCI_FUNC(devfn);
>  	unsigned busno = bus->number;
> @@ -208,10 +202,7 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  
>  	iproc_pcie_reset(pcie);
>  
> -	pcie->sysdata.private_data = pcie;
> -
> -	bus = pci_create_root_bus(pcie->dev, 0, &iproc_pcie_ops,
> -				  &pcie->sysdata, res);
> +	bus = pci_create_root_bus(pcie->dev, 0, &iproc_pcie_ops, pcie, res);
>  	if (!bus) {
>  		dev_err(pcie->dev, "unable to create PCI root bus\n");
>  		ret = -ENOMEM;
> @@ -229,7 +220,9 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  
>  	pci_scan_child_bus(bus);
>  	pci_assign_unassigned_bus_resources(bus);
> +#ifdef CONFIG_ARM
>  	pci_fixup_irqs(pci_common_swizzle, pcie->map_irq);
> +#endif
>  	pci_bus_add_devices(bus);
>  
>  	return 0;
> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
> index ba0a108..0ee9673 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -18,18 +18,22 @@
>  
>  /**
>   * iProc PCIe device
> + * @sysdata: Per PCI controller data. This needs to be kept at the beginning of
> + * struct iproc_pcie, to enable support of both ARM32 and ARM64 platforms with
> + * minimal changes in the iProc PCIe core driver
>   * @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 {
> +#ifdef CONFIG_ARM
> +	struct pci_sys_data sysdata;
> +#endif

I'm OK with adding #ifdefs to make this work on both ARM and ARM64.  We can
at least see the ifdefs and know what needs to be fixed.  I'm a little more
hesitant about adding code that depends on the position of sysdata within
struct iproc_pcie.  I'd rather have something ugly and robust that cries
out for fixing than something minimal and fragile.

I see that your v1 patch added #ifdef CONFIG_ARM around sysdata at its
original location below, and you mentioned that you took Arnd's advice to
move sysdata to the beginning of the structure, but I don't see Arnd's
email on the list.

>  	struct device *dev;
>  	void __iomem *base;
> -	struct pci_sys_data sysdata;
>  	struct pci_bus *root_bus;
>  	struct phy *phy;
>  	int irqs[IPROC_PCIE_MAX_NUM_IRQS];
> -- 
> 1.7.9.5
> 

Re: [PATCH v3 1/4] PCI: iproc: enable arm64 support for iProc PCIe

[Ray Jui]

On 7/21/2015 1:30 PM, Bjorn Helgaas wrote:
> On Wed, Jul 15, 2015 at 09:39:20PM -0700, Ray Jui wrote:
>> This patch enables arm64 support to the iProc PCIe driver
> 
> This needs a little more explanation: ARM has a common struct pci_sys_data
> but ARM64 does not,

Correct, and according to Arnd, there's already work in process of
removing the need for pci_sys_data on arm32. Before that is done, we
need this in the driver for it to work on both arm32 and arm64.

and ARM needs pci_fixup_irqs() but ARM64 does not (why
> not?),

under arch/arm64/kernel/pci.c:

 41 /*
 42  * Try to assign the IRQ number from DT when adding a new device
 43  */
 44 int pcibios_add_device(struct pci_dev *dev)
 45 {
 46         dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
 47
 48         return 0;
 49 }

interrupt is automatically parsed and mapped when adding a new device
for arm64.

ARM uses the common pci_sys_data for the PCI sysdata while ARM64
> uses a driver-specific sysdata, etc.
> 

Correct. pci_sys_data for arm32 will eventually be removed, so all arm32
based PCie host should only need to carry driver specific sysdata.

>> Signed-off-by: Ray Jui <rjui@broadcom.com>
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>> ---
>>  drivers/pci/host/pcie-iproc.c |   15 ++++-----------
>>  drivers/pci/host/pcie-iproc.h |    8 ++++++--
>>  2 files changed, 10 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> index d77481e..8a556d5 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -58,11 +58,6 @@
>>  #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;
>> -}
>> -
>>  /**
>>   * Note access to the configuration registers are protected at the higher layer
>>   * by 'pci_lock' in drivers/pci/access.c
>> @@ -71,8 +66,7 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
>>  					    unsigned int devfn,
>>  					    int where)
>>  {
>> -	struct pci_sys_data *sys = bus->sysdata;
>> -	struct iproc_pcie *pcie = sys_to_pcie(sys);
>> +	struct iproc_pcie *pcie = bus->sysdata;
>>  	unsigned slot = PCI_SLOT(devfn);
>>  	unsigned fn = PCI_FUNC(devfn);
>>  	unsigned busno = bus->number;
>> @@ -208,10 +202,7 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>>  
>>  	iproc_pcie_reset(pcie);
>>  
>> -	pcie->sysdata.private_data = pcie;
>> -
>> -	bus = pci_create_root_bus(pcie->dev, 0, &iproc_pcie_ops,
>> -				  &pcie->sysdata, res);
>> +	bus = pci_create_root_bus(pcie->dev, 0, &iproc_pcie_ops, pcie, res);
>>  	if (!bus) {
>>  		dev_err(pcie->dev, "unable to create PCI root bus\n");
>>  		ret = -ENOMEM;
>> @@ -229,7 +220,9 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>>  
>>  	pci_scan_child_bus(bus);
>>  	pci_assign_unassigned_bus_resources(bus);
>> +#ifdef CONFIG_ARM
>>  	pci_fixup_irqs(pci_common_swizzle, pcie->map_irq);
>> +#endif
>>  	pci_bus_add_devices(bus);
>>  
>>  	return 0;
>> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
>> index ba0a108..0ee9673 100644
>> --- a/drivers/pci/host/pcie-iproc.h
>> +++ b/drivers/pci/host/pcie-iproc.h
>> @@ -18,18 +18,22 @@
>>  
>>  /**
>>   * iProc PCIe device
>> + * @sysdata: Per PCI controller data. This needs to be kept at the beginning of
>> + * struct iproc_pcie, to enable support of both ARM32 and ARM64 platforms with
>> + * minimal changes in the iProc PCIe core driver
>>   * @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 {
>> +#ifdef CONFIG_ARM
>> +	struct pci_sys_data sysdata;
>> +#endif
> 
> I'm OK with adding #ifdefs to make this work on both ARM and ARM64.  We can
> at least see the ifdefs and know what needs to be fixed.  I'm a little more
> hesitant about adding code that depends on the position of sysdata within
> struct iproc_pcie.  I'd rather have something ugly and robust that cries
> out for fixing than something minimal and fragile.
> 

Yes that was my original code and that was a bit ugly. Arnd proposed
this and it does indeed make the look a lot cleaner. But yeah, it now
depends on the location of struct pci_sys_data in memory and I see your
concern. In fact, I asked exactly the same question to Arnd.

Are you okay with living with this for a little while until struct
pci_sys_data is eventually removed from arm32?

> I see that your v1 patch added #ifdef CONFIG_ARM around sysdata at its
> original location below, and you mentioned that you took Arnd's advice to
> move sysdata to the beginning of the structure, but I don't see Arnd's
> email on the list.
> 

Sorry maybe you need to elaborate here. Am I supposed to add Arnd's name
in the commit message? Other than that, Arnd is on this email thread.

>>  	struct device *dev;
>>  	void __iomem *base;
>> -	struct pci_sys_data sysdata;
>>  	struct pci_bus *root_bus;
>>  	struct phy *phy;
>>  	int irqs[IPROC_PCIE_MAX_NUM_IRQS];
>> -- 
>> 1.7.9.5
>>

Re: [PATCH v3 1/4] PCI: iproc: enable arm64 support for iProc PCIe

[Ray Jui]

On 7/21/2015 1:50 PM, Ray Jui wrote:
> 
> 
> On 7/21/2015 1:30 PM, Bjorn Helgaas wrote:
>> On Wed, Jul 15, 2015 at 09:39:20PM -0700, Ray Jui wrote:

>> I see that your v1 patch added #ifdef CONFIG_ARM around sysdata at its
>> original location below, and you mentioned that you took Arnd's advice to
>> move sysdata to the beginning of the structure, but I don't see Arnd's
>> email on the list.
>>
> 
> Sorry maybe you need to elaborate here. Am I supposed to add Arnd's name
> in the commit message? Other than that, Arnd is on this email thread.
> 

For your previous question: when Arnd replied, he only replied to me but
did not reply to all, that's why you cannot find his email....

Ray

Re: [PATCH v3 1/4] PCI: iproc: enable arm64 support for iProc PCIe

[Bjorn Helgaas]

On Tue, Jul 21, 2015 at 01:50:28PM -0700, Ray Jui wrote:
> 
> 
> On 7/21/2015 1:30 PM, Bjorn Helgaas wrote:
> > On Wed, Jul 15, 2015 at 09:39:20PM -0700, Ray Jui wrote:
> >> This patch enables arm64 support to the iProc PCIe driver
> > 
> > This needs a little more explanation: ARM has a common struct pci_sys_data
> > but ARM64 does not,
> 
> Correct, and according to Arnd, there's already work in process of
> removing the need for pci_sys_data on arm32. Before that is done, we
> need this in the driver for it to work on both arm32 and arm64.
> 
> and ARM needs pci_fixup_irqs() but ARM64 does not (why
> > not?),
> 
> under arch/arm64/kernel/pci.c:
> 
>  41 /*
>  42  * Try to assign the IRQ number from DT when adding a new device
>  43  */
>  44 int pcibios_add_device(struct pci_dev *dev)
>  45 {
>  46         dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>  47
>  48         return 0;
>  49 }
> 
> interrupt is automatically parsed and mapped when adding a new device
> for arm64.
> 
> ARM uses the common pci_sys_data for the PCI sysdata while ARM64
> > uses a driver-specific sysdata, etc.
> 
> Correct. pci_sys_data for arm32 will eventually be removed, so all arm32
> based PCie host should only need to carry driver specific sysdata.

That all makes sense.  I'm just looking for a condensed version of it in
the changelog because it takes some digging to figure it out, and in a
couple months even the implicit context of "somebody's working to combine
arm32 and arm64" will be gone.  So we need a changelog that motivates this
patch as it is.

> >> Signed-off-by: Ray Jui <rjui@broadcom.com>
> >> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> >> ---
> >>  drivers/pci/host/pcie-iproc.c |   15 ++++-----------
> >>  drivers/pci/host/pcie-iproc.h |    8 ++++++--
> >>  2 files changed, 10 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> >> index d77481e..8a556d5 100644
> >> --- a/drivers/pci/host/pcie-iproc.c
> >> +++ b/drivers/pci/host/pcie-iproc.c
> >> @@ -58,11 +58,6 @@
> >>  #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;
> >> -}
> >> -
> >>  /**
> >>   * Note access to the configuration registers are protected at the higher layer
> >>   * by 'pci_lock' in drivers/pci/access.c
> >> @@ -71,8 +66,7 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
> >>  					    unsigned int devfn,
> >>  					    int where)
> >>  {
> >> -	struct pci_sys_data *sys = bus->sysdata;
> >> -	struct iproc_pcie *pcie = sys_to_pcie(sys);
> >> +	struct iproc_pcie *pcie = bus->sysdata;
> >>  	unsigned slot = PCI_SLOT(devfn);
> >>  	unsigned fn = PCI_FUNC(devfn);
> >>  	unsigned busno = bus->number;
> >> @@ -208,10 +202,7 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> >>  
> >>  	iproc_pcie_reset(pcie);
> >>  
> >> -	pcie->sysdata.private_data = pcie;
> >> -
> >> -	bus = pci_create_root_bus(pcie->dev, 0, &iproc_pcie_ops,
> >> -				  &pcie->sysdata, res);
> >> +	bus = pci_create_root_bus(pcie->dev, 0, &iproc_pcie_ops, pcie, res);
> >>  	if (!bus) {
> >>  		dev_err(pcie->dev, "unable to create PCI root bus\n");
> >>  		ret = -ENOMEM;
> >> @@ -229,7 +220,9 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> >>  
> >>  	pci_scan_child_bus(bus);
> >>  	pci_assign_unassigned_bus_resources(bus);
> >> +#ifdef CONFIG_ARM
> >>  	pci_fixup_irqs(pci_common_swizzle, pcie->map_irq);
> >> +#endif
> >>  	pci_bus_add_devices(bus);
> >>  
> >>  	return 0;
> >> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
> >> index ba0a108..0ee9673 100644
> >> --- a/drivers/pci/host/pcie-iproc.h
> >> +++ b/drivers/pci/host/pcie-iproc.h
> >> @@ -18,18 +18,22 @@
> >>  
> >>  /**
> >>   * iProc PCIe device
> >> + * @sysdata: Per PCI controller data. This needs to be kept at the beginning of
> >> + * struct iproc_pcie, to enable support of both ARM32 and ARM64 platforms with
> >> + * minimal changes in the iProc PCIe core driver
> >>   * @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 {
> >> +#ifdef CONFIG_ARM
> >> +	struct pci_sys_data sysdata;
> >> +#endif
> > 
> > I'm OK with adding #ifdefs to make this work on both ARM and ARM64.  We can
> > at least see the ifdefs and know what needs to be fixed.  I'm a little more
> > hesitant about adding code that depends on the position of sysdata within
> > struct iproc_pcie.  I'd rather have something ugly and robust that cries
> > out for fixing than something minimal and fragile.
> 
> Yes that was my original code and that was a bit ugly. Arnd proposed
> this and it does indeed make the look a lot cleaner. But yeah, it now
> depends on the location of struct pci_sys_data in memory and I see your
> concern. In fact, I asked exactly the same question to Arnd.
> 
> Are you okay with living with this for a little while until struct
> pci_sys_data is eventually removed from arm32?
> 
> > I see that your v1 patch added #ifdef CONFIG_ARM around sysdata at its
> > original location below, and you mentioned that you took Arnd's advice to
> > move sysdata to the beginning of the structure, but I don't see Arnd's
> > email on the list.
> 
> Sorry maybe you need to elaborate here. Am I supposed to add Arnd's name
> in the commit message? Other than that, Arnd is on this email thread.

No, I wasn't looking for Arnd's name in the changelog; I was just hoping to
read that discussion because it could save me from the embarrassment of
suggesting something different than Arnd did :)

Personally I'd rather have ugly ifdefs because they make it obvious where
the potholes are.  But again, I'm sure Arnd has very good reasons if he
thinks this is better.

> >>  	struct device *dev;
> >>  	void __iomem *base;
> >> -	struct pci_sys_data sysdata;
> >>  	struct pci_bus *root_bus;
> >>  	struct phy *phy;
> >>  	int irqs[IPROC_PCIE_MAX_NUM_IRQS];
> >> -- 
> >> 1.7.9.5
> >>

Re: [PATCH v3 1/4] PCI: iproc: enable arm64 support for iProc PCIe

[Ray Jui]

On 7/21/2015 3:02 PM, Bjorn Helgaas wrote:
> On Tue, Jul 21, 2015 at 01:50:28PM -0700, Ray Jui wrote:
>>
>>
>> On 7/21/2015 1:30 PM, Bjorn Helgaas wrote:
>>> On Wed, Jul 15, 2015 at 09:39:20PM -0700, Ray Jui wrote:
>>>> This patch enables arm64 support to the iProc PCIe driver
>>>
>>> This needs a little more explanation: ARM has a common struct pci_sys_data
>>> but ARM64 does not,
>>
>> Correct, and according to Arnd, there's already work in process of
>> removing the need for pci_sys_data on arm32. Before that is done, we
>> need this in the driver for it to work on both arm32 and arm64.
>>
>> and ARM needs pci_fixup_irqs() but ARM64 does not (why
>>> not?),
>>
>> under arch/arm64/kernel/pci.c:
>>
>>  41 /*
>>  42  * Try to assign the IRQ number from DT when adding a new device
>>  43  */
>>  44 int pcibios_add_device(struct pci_dev *dev)
>>  45 {
>>  46         dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>>  47
>>  48         return 0;
>>  49 }
>>
>> interrupt is automatically parsed and mapped when adding a new device
>> for arm64.
>>
>> ARM uses the common pci_sys_data for the PCI sysdata while ARM64
>>> uses a driver-specific sysdata, etc.
>>
>> Correct. pci_sys_data for arm32 will eventually be removed, so all arm32
>> based PCie host should only need to carry driver specific sysdata.
> 
> That all makes sense.  I'm just looking for a condensed version of it in
> the changelog because it takes some digging to figure it out, and in a
> couple months even the implicit context of "somebody's working to combine
> arm32 and arm64" will be gone.  So we need a changelog that motivates this
> patch as it is.
> 

Okay I will re-submit a new patch with a commit message that explains
the change in more details.

Thanks,

Ray