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
This patch enables arm64 support to the iProc PCIe driver
Signed-off-by: Ray Jui <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
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
Allow Broadcom iProc PCIe core driver to be compiled for ARM64
Signed-off-by: Ray Jui <[email protected]>
Reviewed-by: Vikram Prakash <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Ray Jui <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
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
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 <[email protected]>
> Reviewed-by: Scott Branden <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> Reviewed-by: Scott Branden <[email protected]>
>> ---
>> 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
>>
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
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 <[email protected]>
> >> Reviewed-by: Scott Branden <[email protected]>
> >> ---
> >> 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
> >>
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