[PATCH 0/6] PAXB INTx support with proper model

[PATCH 0/6] PAXB INTx support with proper model

[Ray Jui]

This patch series adds PCIe legacy interrupt (INTx) support to the iProc
PCIe driver by modeling it with its own IRQ domain. All 4 interrupts INTA,
INTB, INTC, INTD share the same interrupt line connected to the GIC
in the system. This is now modeled by using its own IRQ domain.

Also update all relevant devicetree files to adapt to the new model

This patch series is available on GIHUB:
repo: https://github.com/Broadcom/arm64-linux.git
branch: pcie-intx-v1

Ray Jui (6):
  PCI: iproc: Update iProc PCI binding for INTx support
  PCI: iproc: Add INTx support with better modeling
  arm: dts: Change PCIe INTx mapping for Cygnus
  arm: dts: Change PCIe INTx mapping for NSP
  arm: dts: Change PCIe INTx mapping for HR2
  arm64: dts: Change PCIe INTx mapping for NS2

 .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++--
 arch/arm/boot/dts/bcm-cygnus.dtsi                  | 18 +++-
 arch/arm/boot/dts/bcm-hr2.dtsi                     | 18 +++-
 arch/arm/boot/dts/bcm-nsp.dtsi                     | 27 ++++--
 arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi   | 19 +++--
 drivers/pci/host/pcie-iproc-platform.c             |  2 +
 drivers/pci/host/pcie-iproc.c                      | 95 +++++++++++++++++++++-
 drivers/pci/host/pcie-iproc.h                      |  6 ++
 8 files changed, 188 insertions(+), 28 deletions(-)

-- 
2.1.4

[PATCH 0/6] PAXB INTx support with proper model

[Ray Jui]

This patch series adds PCIe legacy interrupt (INTx) support to the iProc
PCIe driver by modeling it with its own IRQ domain. All 4 interrupts INTA,
INTB, INTC, INTD share the same interrupt line connected to the GIC
in the system. This is now modeled by using its own IRQ domain.

Also update all relevant devicetree files to adapt to the new model

This patch series is available on GIHUB:
repo: https://github.com/Broadcom/arm64-linux.git
branch: pcie-intx-v1

Ray Jui (6):
  PCI: iproc: Update iProc PCI binding for INTx support
  PCI: iproc: Add INTx support with better modeling
  arm: dts: Change PCIe INTx mapping for Cygnus
  arm: dts: Change PCIe INTx mapping for NSP
  arm: dts: Change PCIe INTx mapping for HR2
  arm64: dts: Change PCIe INTx mapping for NS2

 .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++--
 arch/arm/boot/dts/bcm-cygnus.dtsi                  | 18 +++-
 arch/arm/boot/dts/bcm-hr2.dtsi                     | 18 +++-
 arch/arm/boot/dts/bcm-nsp.dtsi                     | 27 ++++--
 arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi   | 19 +++--
 drivers/pci/host/pcie-iproc-platform.c             |  2 +
 drivers/pci/host/pcie-iproc.c                      | 95 +++++++++++++++++++++-
 drivers/pci/host/pcie-iproc.h                      |  6 ++
 8 files changed, 188 insertions(+), 28 deletions(-)

-- 
2.1.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 0/6] PAXB INTx support with proper model

[Ray Jui]

This patch series adds PCIe legacy interrupt (INTx) support to the iProc
PCIe driver by modeling it with its own IRQ domain. All 4 interrupts INTA,
INTB, INTC, INTD share the same interrupt line connected to the GIC
in the system. This is now modeled by using its own IRQ domain.

Also update all relevant devicetree files to adapt to the new model

This patch series is available on GIHUB:
repo: https://github.com/Broadcom/arm64-linux.git
branch: pcie-intx-v1

Ray Jui (6):
  PCI: iproc: Update iProc PCI binding for INTx support
  PCI: iproc: Add INTx support with better modeling
  arm: dts: Change PCIe INTx mapping for Cygnus
  arm: dts: Change PCIe INTx mapping for NSP
  arm: dts: Change PCIe INTx mapping for HR2
  arm64: dts: Change PCIe INTx mapping for NS2

 .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++--
 arch/arm/boot/dts/bcm-cygnus.dtsi                  | 18 +++-
 arch/arm/boot/dts/bcm-hr2.dtsi                     | 18 +++-
 arch/arm/boot/dts/bcm-nsp.dtsi                     | 27 ++++--
 arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi   | 19 +++--
 drivers/pci/host/pcie-iproc-platform.c             |  2 +
 drivers/pci/host/pcie-iproc.c                      | 95 +++++++++++++++++++++-
 drivers/pci/host/pcie-iproc.h                      |  6 ++
 8 files changed, 188 insertions(+), 28 deletions(-)

-- 
2.1.4

[PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Ray Jui]

Update the iProc PCIe binding document for better modeling of the legacy
interrupt (INTx) support

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
index b8e48b4..7ea24dc 100644
--- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
@@ -13,9 +13,6 @@ controller, used in Stingray
   PAXB-based root complex is used for external endpoint devices. PAXC-based
 root complex is connected to emulated endpoint devices internal to the ASIC
 - 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>
@@ -41,6 +38,16 @@ Required:
 - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
 address used by the iProc PCIe core (not the PCIe address)
 
+Legacy interrupt (INTx) support (optional):
+
+Note INTx is for PAXB only.
+
+- interrupt-controller: claims itself as an interrupt controller for INTx
+- #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
+- interrupts: interrupt line wired to the generic GIC for INTx support
+
 MSI support (optional):
 
 For older platforms without MSI integrated in the GIC, iProc PCIe core provides
@@ -77,9 +84,14 @@ Example:
 		compatible = "brcm,iproc-pcie";
 		reg = <0x18012000 0x1000>;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie0 1>,
+				<0 0 0 2 &pcie0 2>,
+				<0 0 0 3 &pcie0 3>,
+				<0 0 0 4 &pcie0 4>;
+		interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <0>;
 
@@ -115,9 +127,14 @@ Example:
 		compatible = "brcm,iproc-pcie";
 		reg = <0x18013000 0x1000>;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie1 1>,
+				<0 0 0 2 &pcie1 2>,
+				<0 0 0 3 &pcie1 3>,
+				<0 0 0 4 &pcie1 4>;
+		interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <1>;
 
-- 
2.1.4

[PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Ray Jui]

Update the iProc PCIe binding document for better modeling of the legacy
interrupt (INTx) support

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
index b8e48b4..7ea24dc 100644
--- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
@@ -13,9 +13,6 @@ controller, used in Stingray
   PAXB-based root complex is used for external endpoint devices. PAXC-based
 root complex is connected to emulated endpoint devices internal to the ASIC
 - 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>
@@ -41,6 +38,16 @@ Required:
 - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
 address used by the iProc PCIe core (not the PCIe address)
 
+Legacy interrupt (INTx) support (optional):
+
+Note INTx is for PAXB only.
+
+- interrupt-controller: claims itself as an interrupt controller for INTx
+- #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
+- interrupts: interrupt line wired to the generic GIC for INTx support
+
 MSI support (optional):
 
 For older platforms without MSI integrated in the GIC, iProc PCIe core provides
@@ -77,9 +84,14 @@ Example:
 		compatible = "brcm,iproc-pcie";
 		reg = <0x18012000 0x1000>;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie0 1>,
+				<0 0 0 2 &pcie0 2>,
+				<0 0 0 3 &pcie0 3>,
+				<0 0 0 4 &pcie0 4>;
+		interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <0>;
 
@@ -115,9 +127,14 @@ Example:
 		compatible = "brcm,iproc-pcie";
 		reg = <0x18013000 0x1000>;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie1 1>,
+				<0 0 0 2 &pcie1 2>,
+				<0 0 0 3 &pcie1 3>,
+				<0 0 0 4 &pcie1 4>;
+		interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <1>;
 
-- 
2.1.4

[PATCH 2/6] PCI: iproc: Add INTx support with better modeling

[Ray Jui]

Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
INTD share the same interrupt line connected to the GIC in the system,
while the status of each INTx can be obtained through the INTX CSR
register

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/pci/host/pcie-iproc-platform.c |  2 +
 drivers/pci/host/pcie-iproc.c          | 95 +++++++++++++++++++++++++++++++++-
 drivers/pci/host/pcie-iproc.h          |  6 +++
 3 files changed, 101 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index e764a2a..7a51e6c 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -70,6 +70,8 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
 	}
 	pcie->base_addr = reg.start;
 
+	pcie->irq = platform_get_irq(pdev, 0);
+
 	if (of_property_read_bool(np, "brcm,pcie-ob")) {
 		u32 val;
 
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 14f374d..0bd2c14 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/irqchip/arm-gic-v3.h>
+#include <linux/irqchip/chained_irq.h>
 #include <linux/platform_device.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
@@ -264,6 +265,7 @@ enum iproc_pcie_reg {
 
 	/* enable INTx */
 	IPROC_PCIE_INTX_EN,
+	IPROC_PCIE_INTX_CSR,
 
 	/* outbound address mapping */
 	IPROC_PCIE_OARR0,
@@ -305,6 +307,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = {
 	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
 	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
 	[IPROC_PCIE_INTX_EN]		= 0x330,
+	[IPROC_PCIE_INTX_CSR]		= 0x334,
 	[IPROC_PCIE_LINK_STATUS]	= 0xf0c,
 };
 
@@ -316,6 +319,7 @@ static const u16 iproc_pcie_reg_paxb[] = {
 	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
 	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
 	[IPROC_PCIE_INTX_EN]		= 0x330,
+	[IPROC_PCIE_INTX_CSR]		= 0x334,
 	[IPROC_PCIE_OARR0]		= 0xd20,
 	[IPROC_PCIE_OMAP0]		= 0xd40,
 	[IPROC_PCIE_OARR1]		= 0xd28,
@@ -332,6 +336,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = {
 	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
 	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
 	[IPROC_PCIE_INTX_EN]		= 0x330,
+	[IPROC_PCIE_INTX_CSR]		= 0x334,
 	[IPROC_PCIE_OARR0]		= 0xd20,
 	[IPROC_PCIE_OMAP0]		= 0xd40,
 	[IPROC_PCIE_OARR1]		= 0xd28,
@@ -782,9 +787,90 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie)
 	return link_is_active ? 0 : -ENODEV;
 }
 
-static void iproc_pcie_enable(struct iproc_pcie *pcie)
+static int iproc_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
+			       irq_hw_number_t hwirq)
 {
+	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops intx_domain_ops = {
+	.map = iproc_pcie_intx_map,
+};
+
+static void iproc_pcie_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct iproc_pcie *pcie;
+	struct device *dev;
+	unsigned long status;
+	u32 bit, virq;
+
+	chained_irq_enter(chip, desc);
+	pcie = irq_desc_get_handler_data(desc);
+	dev = pcie->dev;
+
+	/* go through INTx A, B, C, D until all interrupts are handled */
+	while ((status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR) &
+		SYS_RC_INTX_MASK) != 0) {
+		for_each_set_bit(bit, &status, PCI_NUM_INTX) {
+			virq = irq_find_mapping(pcie->irq_domain, bit + 1);
+			if (virq)
+				generic_handle_irq(virq);
+			else
+				dev_err(dev, "unexpected INTx%u\n", bit);
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static int iproc_pcie_intx_enable(struct iproc_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	struct device_node *node = dev->of_node;
+	int ret;
+
 	iproc_pcie_write_reg(pcie, IPROC_PCIE_INTX_EN, SYS_RC_INTX_MASK);
+
+	/*
+	 * BCMA devices do not map INTx the same way as platform devices. All
+	 * BCMA needs is the above code to enable INTx
+	 */
+	if (pcie->irq <= 0)
+		return 0;
+
+	/* set IRQ handler */
+	irq_set_chained_handler_and_data(pcie->irq, iproc_pcie_isr, pcie);
+
+	/* add IRQ domain for INTx */
+	pcie->irq_domain = irq_domain_add_linear(node, PCI_NUM_INTX + 1,
+						 &intx_domain_ops, pcie);
+	if (!pcie->irq_domain) {
+		dev_err(dev, "failed to add INTx IRQ domain\n");
+		ret = -ENOMEM;
+		goto err_rm_handler_data;
+	}
+
+	return 0;
+
+err_rm_handler_data:
+	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
+
+	return ret;
+}
+
+static void iproc_pcie_intx_disable(struct iproc_pcie *pcie)
+{
+	iproc_pcie_write_reg(pcie, IPROC_PCIE_INTX_EN, 0x0);
+
+	if (pcie->irq <= 0)
+		return;
+
+	irq_domain_remove(pcie->irq_domain);
+	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
 }
 
 static inline bool iproc_pcie_ob_is_valid(struct iproc_pcie *pcie,
@@ -1410,7 +1496,11 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 		goto err_power_off_phy;
 	}
 
-	iproc_pcie_enable(pcie);
+	ret = iproc_pcie_intx_enable(pcie);
+	if (ret) {
+		dev_err(dev, "failed to enable INTx\n");
+		goto err_power_off_phy;
+	}
 
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		if (iproc_pcie_msi_enable(pcie))
@@ -1455,6 +1545,7 @@ int iproc_pcie_remove(struct iproc_pcie *pcie)
 	pci_remove_root_bus(pcie->root_bus);
 
 	iproc_pcie_msi_disable(pcie);
+	iproc_pcie_intx_disable(pcie);
 
 	phy_power_off(pcie->phy);
 	phy_exit(pcie->phy);
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index 67081cb..cbcaf9d 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -72,6 +72,9 @@ struct iproc_msi;
  * @ib: inbound mapping related parameters
  * @ib_map: outbound mapping region related parameters
  *
+ * @irq: interrupt line wired to the generic GIC for INTx
+ * @irq_domain: IRQ domain for INTx
+ *
  * @need_msi_steer: indicates additional configuration of the iProc PCIe
  * controller is required to steer MSI writes to external interrupt controller
  * @msi: MSI data
@@ -99,6 +102,9 @@ struct iproc_pcie {
 	struct iproc_pcie_ib ib;
 	const struct iproc_pcie_ib_map *ib_map;
 
+	int irq;
+	struct irq_domain *irq_domain;
+
 	bool need_msi_steer;
 	struct iproc_msi *msi;
 };
-- 
2.1.4

[PATCH 2/6] PCI: iproc: Add INTx support with better modeling

[Ray Jui]

Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
INTD share the same interrupt line connected to the GIC in the system,
while the status of each INTx can be obtained through the INTX CSR
register

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/pci/host/pcie-iproc-platform.c |  2 +
 drivers/pci/host/pcie-iproc.c          | 95 +++++++++++++++++++++++++++++++++-
 drivers/pci/host/pcie-iproc.h          |  6 +++
 3 files changed, 101 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index e764a2a..7a51e6c 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -70,6 +70,8 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
 	}
 	pcie->base_addr = reg.start;
 
+	pcie->irq = platform_get_irq(pdev, 0);
+
 	if (of_property_read_bool(np, "brcm,pcie-ob")) {
 		u32 val;
 
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 14f374d..0bd2c14 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/irqchip/arm-gic-v3.h>
+#include <linux/irqchip/chained_irq.h>
 #include <linux/platform_device.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
@@ -264,6 +265,7 @@ enum iproc_pcie_reg {
 
 	/* enable INTx */
 	IPROC_PCIE_INTX_EN,
+	IPROC_PCIE_INTX_CSR,
 
 	/* outbound address mapping */
 	IPROC_PCIE_OARR0,
@@ -305,6 +307,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = {
 	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
 	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
 	[IPROC_PCIE_INTX_EN]		= 0x330,
+	[IPROC_PCIE_INTX_CSR]		= 0x334,
 	[IPROC_PCIE_LINK_STATUS]	= 0xf0c,
 };
 
@@ -316,6 +319,7 @@ static const u16 iproc_pcie_reg_paxb[] = {
 	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
 	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
 	[IPROC_PCIE_INTX_EN]		= 0x330,
+	[IPROC_PCIE_INTX_CSR]		= 0x334,
 	[IPROC_PCIE_OARR0]		= 0xd20,
 	[IPROC_PCIE_OMAP0]		= 0xd40,
 	[IPROC_PCIE_OARR1]		= 0xd28,
@@ -332,6 +336,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = {
 	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
 	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
 	[IPROC_PCIE_INTX_EN]		= 0x330,
+	[IPROC_PCIE_INTX_CSR]		= 0x334,
 	[IPROC_PCIE_OARR0]		= 0xd20,
 	[IPROC_PCIE_OMAP0]		= 0xd40,
 	[IPROC_PCIE_OARR1]		= 0xd28,
@@ -782,9 +787,90 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie)
 	return link_is_active ? 0 : -ENODEV;
 }
 
-static void iproc_pcie_enable(struct iproc_pcie *pcie)
+static int iproc_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
+			       irq_hw_number_t hwirq)
 {
+	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops intx_domain_ops = {
+	.map = iproc_pcie_intx_map,
+};
+
+static void iproc_pcie_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct iproc_pcie *pcie;
+	struct device *dev;
+	unsigned long status;
+	u32 bit, virq;
+
+	chained_irq_enter(chip, desc);
+	pcie = irq_desc_get_handler_data(desc);
+	dev = pcie->dev;
+
+	/* go through INTx A, B, C, D until all interrupts are handled */
+	while ((status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR) &
+		SYS_RC_INTX_MASK) != 0) {
+		for_each_set_bit(bit, &status, PCI_NUM_INTX) {
+			virq = irq_find_mapping(pcie->irq_domain, bit + 1);
+			if (virq)
+				generic_handle_irq(virq);
+			else
+				dev_err(dev, "unexpected INTx%u\n", bit);
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static int iproc_pcie_intx_enable(struct iproc_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	struct device_node *node = dev->of_node;
+	int ret;
+
 	iproc_pcie_write_reg(pcie, IPROC_PCIE_INTX_EN, SYS_RC_INTX_MASK);
+
+	/*
+	 * BCMA devices do not map INTx the same way as platform devices. All
+	 * BCMA needs is the above code to enable INTx
+	 */
+	if (pcie->irq <= 0)
+		return 0;
+
+	/* set IRQ handler */
+	irq_set_chained_handler_and_data(pcie->irq, iproc_pcie_isr, pcie);
+
+	/* add IRQ domain for INTx */
+	pcie->irq_domain = irq_domain_add_linear(node, PCI_NUM_INTX + 1,
+						 &intx_domain_ops, pcie);
+	if (!pcie->irq_domain) {
+		dev_err(dev, "failed to add INTx IRQ domain\n");
+		ret = -ENOMEM;
+		goto err_rm_handler_data;
+	}
+
+	return 0;
+
+err_rm_handler_data:
+	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
+
+	return ret;
+}
+
+static void iproc_pcie_intx_disable(struct iproc_pcie *pcie)
+{
+	iproc_pcie_write_reg(pcie, IPROC_PCIE_INTX_EN, 0x0);
+
+	if (pcie->irq <= 0)
+		return;
+
+	irq_domain_remove(pcie->irq_domain);
+	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
 }
 
 static inline bool iproc_pcie_ob_is_valid(struct iproc_pcie *pcie,
@@ -1410,7 +1496,11 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 		goto err_power_off_phy;
 	}
 
-	iproc_pcie_enable(pcie);
+	ret = iproc_pcie_intx_enable(pcie);
+	if (ret) {
+		dev_err(dev, "failed to enable INTx\n");
+		goto err_power_off_phy;
+	}
 
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		if (iproc_pcie_msi_enable(pcie))
@@ -1455,6 +1545,7 @@ int iproc_pcie_remove(struct iproc_pcie *pcie)
 	pci_remove_root_bus(pcie->root_bus);
 
 	iproc_pcie_msi_disable(pcie);
+	iproc_pcie_intx_disable(pcie);
 
 	phy_power_off(pcie->phy);
 	phy_exit(pcie->phy);
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index 67081cb..cbcaf9d 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -72,6 +72,9 @@ struct iproc_msi;
  * @ib: inbound mapping related parameters
  * @ib_map: outbound mapping region related parameters
  *
+ * @irq: interrupt line wired to the generic GIC for INTx
+ * @irq_domain: IRQ domain for INTx
+ *
  * @need_msi_steer: indicates additional configuration of the iProc PCIe
  * controller is required to steer MSI writes to external interrupt controller
  * @msi: MSI data
@@ -99,6 +102,9 @@ struct iproc_pcie {
 	struct iproc_pcie_ib ib;
 	const struct iproc_pcie_ib_map *ib_map;
 
+	int irq;
+	struct irq_domain *irq_domain;
+
 	bool need_msi_steer;
 	struct iproc_msi *msi;
 };
-- 
2.1.4

[PATCH 3/6] arm: dts: Change PCIe INTx mapping for Cygnus

[Ray Jui]

Change the PCIe INTx mapping to model the 4 INTx interrupts in the
IRQ domain of the iProc PCIe controller itself

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 arch/arm/boot/dts/bcm-cygnus.dtsi | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
index 699fdf9..6de21ef 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -254,9 +254,14 @@
 			compatible = "brcm,iproc-pcie";
 			reg = <0x18012000 0x1000>;
 
+			interrupt-controller;
 			#interrupt-cells = <1>;
-			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
+			interrupt-map-mask = <0 0 0 7>;
+			interrupt-map = <0 0 0 1 &pcie0 1>,
+					<0 0 0 2 &pcie0 2>,
+					<0 0 0 3 &pcie0 3>,
+					<0 0 0 4 &pcie0 4>;
+			interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
 
 			linux,pci-domain = <0>;
 
@@ -289,9 +294,14 @@
 			compatible = "brcm,iproc-pcie";
 			reg = <0x18013000 0x1000>;
 
+			interrupt-controller;
 			#interrupt-cells = <1>;
-			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
+			interrupt-map-mask = <0 0 0 7>;
+			interrupt-map = <0 0 0 1 &pcie1 1>,
+					<0 0 0 2 &pcie1 2>,
+					<0 0 0 3 &pcie1 3>,
+					<0 0 0 4 &pcie1 4>;
+			interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
 
 			linux,pci-domain = <1>;
 
-- 
2.1.4

[PATCH 3/6] arm: dts: Change PCIe INTx mapping for Cygnus

[Ray Jui]

Change the PCIe INTx mapping to model the 4 INTx interrupts in the
IRQ domain of the iProc PCIe controller itself

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 arch/arm/boot/dts/bcm-cygnus.dtsi | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
index 699fdf9..6de21ef 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -254,9 +254,14 @@
 			compatible = "brcm,iproc-pcie";
 			reg = <0x18012000 0x1000>;
 
+			interrupt-controller;
 			#interrupt-cells = <1>;
-			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
+			interrupt-map-mask = <0 0 0 7>;
+			interrupt-map = <0 0 0 1 &pcie0 1>,
+					<0 0 0 2 &pcie0 2>,
+					<0 0 0 3 &pcie0 3>,
+					<0 0 0 4 &pcie0 4>;
+			interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
 
 			linux,pci-domain = <0>;
 
@@ -289,9 +294,14 @@
 			compatible = "brcm,iproc-pcie";
 			reg = <0x18013000 0x1000>;
 
+			interrupt-controller;
 			#interrupt-cells = <1>;
-			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
+			interrupt-map-mask = <0 0 0 7>;
+			interrupt-map = <0 0 0 1 &pcie1 1>,
+					<0 0 0 2 &pcie1 2>,
+					<0 0 0 3 &pcie1 3>,
+					<0 0 0 4 &pcie1 4>;
+			interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
 
 			linux,pci-domain = <1>;
 
-- 
2.1.4

[PATCH 4/6] arm: dts: Change PCIe INTx mapping for NSP

[Ray Jui]

Change the PCIe INTx mapping to model the 4 INTx interrupts in the
IRQ domain of the iProc PCIe controller itself

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 arch/arm/boot/dts/bcm-nsp.dtsi | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi
index dcc55aa..8c4c8b2 100644
--- a/arch/arm/boot/dts/bcm-nsp.dtsi
+++ b/arch/arm/boot/dts/bcm-nsp.dtsi
@@ -494,9 +494,14 @@
 		compatible = "brcm,iproc-pcie";
 		reg = <0x18012000 0x1000>;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 131 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie0 1>,
+				<0 0 0 2 &pcie0 2>,
+				<0 0 0 3 &pcie0 3>,
+				<0 0 0 4 &pcie0 4>;
+		interrupts = <GIC_SPI 131 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <0>;
 
@@ -531,9 +536,14 @@
 		compatible = "brcm,iproc-pcie";
 		reg = <0x18013000 0x1000>;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 137 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie1 1>,
+				<0 0 0 2 &pcie1 2>,
+				<0 0 0 3 &pcie1 3>,
+				<0 0 0 4 &pcie1 4>;
+		interrupts = <GIC_SPI 137 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <1>;
 
@@ -568,9 +578,14 @@
 		compatible = "brcm,iproc-pcie";
 		reg = <0x18014000 0x1000>;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 143 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie2 1>,
+				<0 0 0 2 &pcie2 2>,
+				<0 0 0 3 &pcie2 3>,
+				<0 0 0 4 &pcie2 4>;
+		interrupts = <GIC_SPI 143 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <2>;
 
-- 
2.1.4

[PATCH 4/6] arm: dts: Change PCIe INTx mapping for NSP

[Ray Jui]

Change the PCIe INTx mapping to model the 4 INTx interrupts in the
IRQ domain of the iProc PCIe controller itself

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 arch/arm/boot/dts/bcm-nsp.dtsi | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi
index dcc55aa..8c4c8b2 100644
--- a/arch/arm/boot/dts/bcm-nsp.dtsi
+++ b/arch/arm/boot/dts/bcm-nsp.dtsi
@@ -494,9 +494,14 @@
 		compatible = "brcm,iproc-pcie";
 		reg = <0x18012000 0x1000>;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 131 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie0 1>,
+				<0 0 0 2 &pcie0 2>,
+				<0 0 0 3 &pcie0 3>,
+				<0 0 0 4 &pcie0 4>;
+		interrupts = <GIC_SPI 131 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <0>;
 
@@ -531,9 +536,14 @@
 		compatible = "brcm,iproc-pcie";
 		reg = <0x18013000 0x1000>;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 137 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie1 1>,
+				<0 0 0 2 &pcie1 2>,
+				<0 0 0 3 &pcie1 3>,
+				<0 0 0 4 &pcie1 4>;
+		interrupts = <GIC_SPI 137 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <1>;
 
@@ -568,9 +578,14 @@
 		compatible = "brcm,iproc-pcie";
 		reg = <0x18014000 0x1000>;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 143 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie2 1>,
+				<0 0 0 2 &pcie2 2>,
+				<0 0 0 3 &pcie2 3>,
+				<0 0 0 4 &pcie2 4>;
+		interrupts = <GIC_SPI 143 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <2>;
 
-- 
2.1.4

[PATCH 5/6] arm: dts: Change PCIe INTx mapping for HR2

[Ray Jui]

Change the PCIe INTx mapping to model the 4 INTx interrupts in the
IRQ domain of the iProc PCIe controller itself

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 arch/arm/boot/dts/bcm-hr2.dtsi | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/bcm-hr2.dtsi b/arch/arm/boot/dts/bcm-hr2.dtsi
index 3f9cedd..20e3161 100644
--- a/arch/arm/boot/dts/bcm-hr2.dtsi
+++ b/arch/arm/boot/dts/bcm-hr2.dtsi
@@ -298,9 +298,14 @@
 		compatible = "brcm,iproc-pcie";
 		reg = <0x18012000 0x1000>;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 186 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie0 1>,
+				<0 0 0 2 &pcie0 2>,
+				<0 0 0 3 &pcie0 3>,
+				<0 0 0 4 &pcie0 4>;
+		interrupts = <GIC_SPI 186 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <0>;
 
@@ -334,9 +339,14 @@
 		compatible = "brcm,iproc-pcie";
 		reg = <0x18013000 0x1000>;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 192 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie1 1>,
+				<0 0 0 2 &pcie1 2>,
+				<0 0 0 3 &pcie1 3>,
+				<0 0 0 4 &pcie1 4>;
+		interrupts = <GIC_SPI 192 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <1>;
 
-- 
2.1.4

[PATCH 5/6] arm: dts: Change PCIe INTx mapping for HR2

[Ray Jui]

Change the PCIe INTx mapping to model the 4 INTx interrupts in the
IRQ domain of the iProc PCIe controller itself

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 arch/arm/boot/dts/bcm-hr2.dtsi | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/bcm-hr2.dtsi b/arch/arm/boot/dts/bcm-hr2.dtsi
index 3f9cedd..20e3161 100644
--- a/arch/arm/boot/dts/bcm-hr2.dtsi
+++ b/arch/arm/boot/dts/bcm-hr2.dtsi
@@ -298,9 +298,14 @@
 		compatible = "brcm,iproc-pcie";
 		reg = <0x18012000 0x1000>;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 186 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie0 1>,
+				<0 0 0 2 &pcie0 2>,
+				<0 0 0 3 &pcie0 3>,
+				<0 0 0 4 &pcie0 4>;
+		interrupts = <GIC_SPI 186 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <0>;
 
@@ -334,9 +339,14 @@
 		compatible = "brcm,iproc-pcie";
 		reg = <0x18013000 0x1000>;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 192 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie1 1>,
+				<0 0 0 2 &pcie1 2>,
+				<0 0 0 3 &pcie1 3>,
+				<0 0 0 4 &pcie1 4>;
+		interrupts = <GIC_SPI 192 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <1>;
 
-- 
2.1.4

[PATCH 6/6] arm64: dts: Change PCIe INTx mapping for NS2

[Ray Jui]

Change the PCIe INTx mapping to model the 4 INTx interrupts in the
IRQ domain of the iProc PCIe controller itself

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi b/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi
index 4a2a6af..0373ebe 100644
--- a/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi
+++ b/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi
@@ -116,9 +116,14 @@
 		reg = <0 0x20020000 0 0x1000>;
 		dma-coherent;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 281 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie0 1>,
+				<0 0 0 2 &pcie0 2>,
+				<0 0 0 3 &pcie0 3>,
+				<0 0 0 4 &pcie0 4>;
+		interrupts = <GIC_SPI 281 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <0>;
 
@@ -147,9 +152,14 @@
 		reg = <0 0x50020000 0 0x1000>;
 		dma-coherent;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 305 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie4 1>,
+				<0 0 0 2 &pcie4 2>,
+				<0 0 0 3 &pcie4 3>,
+				<0 0 0 4 &pcie4 4>;
+		interrupts = <GIC_SPI 305 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <4>;
 
@@ -169,7 +179,6 @@
 
 		phys = <&pci_phy1>;
 		phy-names = "pcie-phy";
-
 		msi-parent = <&v2m0>;
 	};
 
-- 
2.1.4

[PATCH 6/6] arm64: dts: Change PCIe INTx mapping for NS2

[Ray Jui]

Change the PCIe INTx mapping to model the 4 INTx interrupts in the
IRQ domain of the iProc PCIe controller itself

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi b/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi
index 4a2a6af..0373ebe 100644
--- a/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi
+++ b/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi
@@ -116,9 +116,14 @@
 		reg = <0 0x20020000 0 0x1000>;
 		dma-coherent;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 281 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie0 1>,
+				<0 0 0 2 &pcie0 2>,
+				<0 0 0 3 &pcie0 3>,
+				<0 0 0 4 &pcie0 4>;
+		interrupts = <GIC_SPI 281 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <0>;
 
@@ -147,9 +152,14 @@
 		reg = <0 0x50020000 0 0x1000>;
 		dma-coherent;
 
+		interrupt-controller;
 		#interrupt-cells = <1>;
-		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 305 IRQ_TYPE_NONE>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie4 1>,
+				<0 0 0 2 &pcie4 2>,
+				<0 0 0 3 &pcie4 3>,
+				<0 0 0 4 &pcie4 4>;
+		interrupts = <GIC_SPI 305 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <4>;
 
@@ -169,7 +179,6 @@
 
 		phys = <&pci_phy1>;
 		phy-names = "pcie-phy";
-
 		msi-parent = <&v2m0>;
 	};
 
-- 
2.1.4

Re: [PATCH 2/6] PCI: iproc: Add INTx support with better modeling

[Andy Shevchenko]

On Wed, May 30, 2018 at 12:58 AM, Ray Jui <ray.jui@broadcom.com> wrote:
> Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
> modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
> INTD share the same interrupt line connected to the GIC in the system,
> while the status of each INTx can be obtained through the INTX CSR
> register

> +       while ((status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR) &
> +               SYS_RC_INTX_MASK) != 0) {
> +               for_each_set_bit(bit, &status, PCI_NUM_INTX) {
> +                       virq = irq_find_mapping(pcie->irq_domain, bit + 1);
> +                       if (virq)
> +                               generic_handle_irq(virq);
> +                       else
> +                               dev_err(dev, "unexpected INTx%u\n", bit);
> +               }
> +       }

do {
  status = ...;
  for_each_set_bit() {
    ...
  }
} while (status);

would look slightly better for my opinion.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/6] PCI: iproc: Add INTx support with better modeling

[Andy Shevchenko]

On Wed, May 30, 2018 at 12:58 AM, Ray Jui <ray.jui@broadcom.com> wrote:
> Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
> modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
> INTD share the same interrupt line connected to the GIC in the system,
> while the status of each INTx can be obtained through the INTX CSR
> register

> +       while ((status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR) &
> +               SYS_RC_INTX_MASK) != 0) {
> +               for_each_set_bit(bit, &status, PCI_NUM_INTX) {
> +                       virq = irq_find_mapping(pcie->irq_domain, bit + 1);
> +                       if (virq)
> +                               generic_handle_irq(virq);
> +                       else
> +                               dev_err(dev, "unexpected INTx%u\n", bit);
> +               }
> +       }

do {
  status = ...;
  for_each_set_bit() {
    ...
  }
} while (status);

would look slightly better for my opinion.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/6] PCI: iproc: Add INTx support with better modeling

[Andy Shevchenko]

On Wed, May 30, 2018 at 12:58 AM, Ray Jui <ray.jui@broadcom.com> wrote:
> Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
> modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
> INTD share the same interrupt line connected to the GIC in the system,
> while the status of each INTx can be obtained through the INTX CSR
> register

> +       while ((status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR) &
> +               SYS_RC_INTX_MASK) != 0) {
> +               for_each_set_bit(bit, &status, PCI_NUM_INTX) {
> +                       virq = irq_find_mapping(pcie->irq_domain, bit + 1);
> +                       if (virq)
> +                               generic_handle_irq(virq);
> +                       else
> +                               dev_err(dev, "unexpected INTx%u\n", bit);
> +               }
> +       }

do {
  status = ...;
  for_each_set_bit() {
    ...
  }
} while (status);

would look slightly better for my opinion.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/6] PCI: iproc: Add INTx support with better modeling

[Ray Jui]

Hi Andy,

On 5/29/2018 5:59 PM, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 12:58 AM, Ray Jui <ray.jui@broadcom.com> wrote:
>> Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
>> modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
>> INTD share the same interrupt line connected to the GIC in the system,
>> while the status of each INTx can be obtained through the INTX CSR
>> register
> 
>> +       while ((status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR) &
>> +               SYS_RC_INTX_MASK) != 0) {
>> +               for_each_set_bit(bit, &status, PCI_NUM_INTX) {
>> +                       virq = irq_find_mapping(pcie->irq_domain, bit + 1);
>> +                       if (virq)
>> +                               generic_handle_irq(virq);
>> +                       else
>> +                               dev_err(dev, "unexpected INTx%u\n", bit);
>> +               }
>> +       }
> 
> do {
>    status = ...;
>    for_each_set_bit() {
>      ...
>    }
> } while (status);
> 
> would look slightly better for my opinion.
> 

Indeed. I agree with you. I'll wait for comments before sending out v2 
which will include this improvement.

Thanks,

Ray

[PATCH 2/6] PCI: iproc: Add INTx support with better modeling

[Ray Jui]

Hi Andy,

On 5/29/2018 5:59 PM, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 12:58 AM, Ray Jui <ray.jui@broadcom.com> wrote:
>> Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
>> modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
>> INTD share the same interrupt line connected to the GIC in the system,
>> while the status of each INTx can be obtained through the INTX CSR
>> register
> 
>> +       while ((status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR) &
>> +               SYS_RC_INTX_MASK) != 0) {
>> +               for_each_set_bit(bit, &status, PCI_NUM_INTX) {
>> +                       virq = irq_find_mapping(pcie->irq_domain, bit + 1);
>> +                       if (virq)
>> +                               generic_handle_irq(virq);
>> +                       else
>> +                               dev_err(dev, "unexpected INTx%u\n", bit);
>> +               }
>> +       }
> 
> do {
>    status = ...;
>    for_each_set_bit() {
>      ...
>    }
> } while (status);
> 
> would look slightly better for my opinion.
> 

Indeed. I agree with you. I'll wait for comments before sending out v2 
which will include this improvement.

Thanks,

Ray

Re: [PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Rob Herring]

+Arnd

On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> Update the iProc PCIe binding document for better modeling of the legacy
> interrupt (INTx) support
>
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> ---
>  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
>  1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> index b8e48b4..7ea24dc 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> @@ -13,9 +13,6 @@ controller, used in Stingray
>    PAXB-based root complex is used for external endpoint devices. PAXC-based
>  root complex is connected to emulated endpoint devices internal to the ASIC
>  - 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>
> @@ -41,6 +38,16 @@ Required:
>  - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
>  address used by the iProc PCIe core (not the PCIe address)
>
> +Legacy interrupt (INTx) support (optional):
> +
> +Note INTx is for PAXB only.
> +
> +- interrupt-controller: claims itself as an interrupt controller for INTx
> +- #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
> +- interrupts: interrupt line wired to the generic GIC for INTx support
> +
>  MSI support (optional):
>
>  For older platforms without MSI integrated in the GIC, iProc PCIe core provides
> @@ -77,9 +84,14 @@ Example:
>                 compatible = "brcm,iproc-pcie";
>                 reg = <0x18012000 0x1000>;
>
> +               interrupt-controller;
>                 #interrupt-cells = <1>;
> -               interrupt-map-mask = <0 0 0 0>;
> -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> +               interrupt-map-mask = <0 0 0 7>;
> +               interrupt-map = <0 0 0 1 &pcie0 1>,

Are you sure this works? The irq parsing code will ignore
interrupt-map if interrupt-controller is found. In other words, you
should have one or the other, but not both.

Maybe it happens to work because "pcie0" is this node and your irq
numbers are the same.

Arnd, any thoughts on this?

> +                               <0 0 0 2 &pcie0 2>,
> +                               <0 0 0 3 &pcie0 3>,
> +                               <0 0 0 4 &pcie0 4>;
> +               interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
>
>                 linux,pci-domain = <0>;
>
> @@ -115,9 +127,14 @@ Example:
>                 compatible = "brcm,iproc-pcie";
>                 reg = <0x18013000 0x1000>;
>
> +               interrupt-controller;
>                 #interrupt-cells = <1>;
> -               interrupt-map-mask = <0 0 0 0>;
> -               interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
> +               interrupt-map-mask = <0 0 0 7>;
> +               interrupt-map = <0 0 0 1 &pcie1 1>,
> +                               <0 0 0 2 &pcie1 2>,
> +                               <0 0 0 3 &pcie1 3>,
> +                               <0 0 0 4 &pcie1 4>;
> +               interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
>
>                 linux,pci-domain = <1>;
>
> --
> 2.1.4
>

[PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Rob Herring]

+Arnd

On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> Update the iProc PCIe binding document for better modeling of the legacy
> interrupt (INTx) support
>
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> ---
>  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
>  1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> index b8e48b4..7ea24dc 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> @@ -13,9 +13,6 @@ controller, used in Stingray
>    PAXB-based root complex is used for external endpoint devices. PAXC-based
>  root complex is connected to emulated endpoint devices internal to the ASIC
>  - 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>
> @@ -41,6 +38,16 @@ Required:
>  - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
>  address used by the iProc PCIe core (not the PCIe address)
>
> +Legacy interrupt (INTx) support (optional):
> +
> +Note INTx is for PAXB only.
> +
> +- interrupt-controller: claims itself as an interrupt controller for INTx
> +- #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
> +- interrupts: interrupt line wired to the generic GIC for INTx support
> +
>  MSI support (optional):
>
>  For older platforms without MSI integrated in the GIC, iProc PCIe core provides
> @@ -77,9 +84,14 @@ Example:
>                 compatible = "brcm,iproc-pcie";
>                 reg = <0x18012000 0x1000>;
>
> +               interrupt-controller;
>                 #interrupt-cells = <1>;
> -               interrupt-map-mask = <0 0 0 0>;
> -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> +               interrupt-map-mask = <0 0 0 7>;
> +               interrupt-map = <0 0 0 1 &pcie0 1>,

Are you sure this works? The irq parsing code will ignore
interrupt-map if interrupt-controller is found. In other words, you
should have one or the other, but not both.

Maybe it happens to work because "pcie0" is this node and your irq
numbers are the same.

Arnd, any thoughts on this?

> +                               <0 0 0 2 &pcie0 2>,
> +                               <0 0 0 3 &pcie0 3>,
> +                               <0 0 0 4 &pcie0 4>;
> +               interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
>
>                 linux,pci-domain = <0>;
>
> @@ -115,9 +127,14 @@ Example:
>                 compatible = "brcm,iproc-pcie";
>                 reg = <0x18013000 0x1000>;
>
> +               interrupt-controller;
>                 #interrupt-cells = <1>;
> -               interrupt-map-mask = <0 0 0 0>;
> -               interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
> +               interrupt-map-mask = <0 0 0 7>;
> +               interrupt-map = <0 0 0 1 &pcie1 1>,
> +                               <0 0 0 2 &pcie1 2>,
> +                               <0 0 0 3 &pcie1 3>,
> +                               <0 0 0 4 &pcie1 4>;
> +               interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
>
>                 linux,pci-domain = <1>;
>
> --
> 2.1.4
>

Re: [PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Ray Jui]

Hi Rob/Arnd,

On 6/4/2018 7:17 AM, Rob Herring wrote:
> +Arnd
> 
> On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
>> Update the iProc PCIe binding document for better modeling of the legacy
>> interrupt (INTx) support
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> ---
>>   .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
>>   1 file changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> index b8e48b4..7ea24dc 100644
>> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> @@ -13,9 +13,6 @@ controller, used in Stingray
>>     PAXB-based root complex is used for external endpoint devices. PAXC-based
>>   root complex is connected to emulated endpoint devices internal to the ASIC
>>   - 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>
>> @@ -41,6 +38,16 @@ Required:
>>   - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
>>   address used by the iProc PCIe core (not the PCIe address)
>>
>> +Legacy interrupt (INTx) support (optional):
>> +
>> +Note INTx is for PAXB only.
>> +
>> +- interrupt-controller: claims itself as an interrupt controller for INTx
>> +- #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
>> +- interrupts: interrupt line wired to the generic GIC for INTx support
>> +
>>   MSI support (optional):
>>
>>   For older platforms without MSI integrated in the GIC, iProc PCIe core provides
>> @@ -77,9 +84,14 @@ Example:
>>                  compatible = "brcm,iproc-pcie";
>>                  reg = <0x18012000 0x1000>;
>>
>> +               interrupt-controller;
>>                  #interrupt-cells = <1>;
>> -               interrupt-map-mask = <0 0 0 0>;
>> -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
>> +               interrupt-map-mask = <0 0 0 7>;
>> +               interrupt-map = <0 0 0 1 &pcie0 1>,
> 
> Are you sure this works? The irq parsing code will ignore
> interrupt-map if interrupt-controller is found. In other words, you
> should have one or the other, but not both.

Yes, it does work. I tested this by using an Intel E1000E PCIe NIC card 
installed in our system and have it fall back to INTx.

> 
> Maybe it happens to work because "pcie0" is this node and your irq
> numbers are the same.

Perhaps it works because we are claiming "pcie0" as an interrupt 
controller by itself and the INTx is modeled under that.

> 
> Arnd, any thoughts on this?
> 

Please let me know if the above model makes sense or not.

Thanks,

Ray

>> +                               <0 0 0 2 &pcie0 2>,
>> +                               <0 0 0 3 &pcie0 3>,
>> +                               <0 0 0 4 &pcie0 4>;
>> +               interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
>>
>>                  linux,pci-domain = <0>;
>>
>> @@ -115,9 +127,14 @@ Example:
>>                  compatible = "brcm,iproc-pcie";
>>                  reg = <0x18013000 0x1000>;
>>
>> +               interrupt-controller;
>>                  #interrupt-cells = <1>;
>> -               interrupt-map-mask = <0 0 0 0>;
>> -               interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
>> +               interrupt-map-mask = <0 0 0 7>;
>> +               interrupt-map = <0 0 0 1 &pcie1 1>,
>> +                               <0 0 0 2 &pcie1 2>,
>> +                               <0 0 0 3 &pcie1 3>,
>> +                               <0 0 0 4 &pcie1 4>;
>> +               interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
>>
>>                  linux,pci-domain = <1>;
>>
>> --
>> 2.1.4
>>

Re: [PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Ray Jui]

Hi Rob/Arnd,

On 6/4/2018 7:17 AM, Rob Herring wrote:
> +Arnd
> 
> On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
>> Update the iProc PCIe binding document for better modeling of the legacy
>> interrupt (INTx) support
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> ---
>>   .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
>>   1 file changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> index b8e48b4..7ea24dc 100644
>> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> @@ -13,9 +13,6 @@ controller, used in Stingray
>>     PAXB-based root complex is used for external endpoint devices. PAXC-based
>>   root complex is connected to emulated endpoint devices internal to the ASIC
>>   - 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>
>> @@ -41,6 +38,16 @@ Required:
>>   - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
>>   address used by the iProc PCIe core (not the PCIe address)
>>
>> +Legacy interrupt (INTx) support (optional):
>> +
>> +Note INTx is for PAXB only.
>> +
>> +- interrupt-controller: claims itself as an interrupt controller for INTx
>> +- #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
>> +- interrupts: interrupt line wired to the generic GIC for INTx support
>> +
>>   MSI support (optional):
>>
>>   For older platforms without MSI integrated in the GIC, iProc PCIe core provides
>> @@ -77,9 +84,14 @@ Example:
>>                  compatible = "brcm,iproc-pcie";
>>                  reg = <0x18012000 0x1000>;
>>
>> +               interrupt-controller;
>>                  #interrupt-cells = <1>;
>> -               interrupt-map-mask = <0 0 0 0>;
>> -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
>> +               interrupt-map-mask = <0 0 0 7>;
>> +               interrupt-map = <0 0 0 1 &pcie0 1>,
> 
> Are you sure this works? The irq parsing code will ignore
> interrupt-map if interrupt-controller is found. In other words, you
> should have one or the other, but not both.

Yes, it does work. I tested this by using an Intel E1000E PCIe NIC card 
installed in our system and have it fall back to INTx.

> 
> Maybe it happens to work because "pcie0" is this node and your irq
> numbers are the same.

Perhaps it works because we are claiming "pcie0" as an interrupt 
controller by itself and the INTx is modeled under that.

> 
> Arnd, any thoughts on this?
> 

Please let me know if the above model makes sense or not.

Thanks,

Ray

>> +                               <0 0 0 2 &pcie0 2>,
>> +                               <0 0 0 3 &pcie0 3>,
>> +                               <0 0 0 4 &pcie0 4>;
>> +               interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
>>
>>                  linux,pci-domain = <0>;
>>
>> @@ -115,9 +127,14 @@ Example:
>>                  compatible = "brcm,iproc-pcie";
>>                  reg = <0x18013000 0x1000>;
>>
>> +               interrupt-controller;
>>                  #interrupt-cells = <1>;
>> -               interrupt-map-mask = <0 0 0 0>;
>> -               interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
>> +               interrupt-map-mask = <0 0 0 7>;
>> +               interrupt-map = <0 0 0 1 &pcie1 1>,
>> +                               <0 0 0 2 &pcie1 2>,
>> +                               <0 0 0 3 &pcie1 3>,
>> +                               <0 0 0 4 &pcie1 4>;
>> +               interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
>>
>>                  linux,pci-domain = <1>;
>>
>> --
>> 2.1.4
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Ray Jui]

Hi Rob/Arnd,

On 6/4/2018 7:17 AM, Rob Herring wrote:
> +Arnd
> 
> On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
>> Update the iProc PCIe binding document for better modeling of the legacy
>> interrupt (INTx) support
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> ---
>>   .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
>>   1 file changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> index b8e48b4..7ea24dc 100644
>> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> @@ -13,9 +13,6 @@ controller, used in Stingray
>>     PAXB-based root complex is used for external endpoint devices. PAXC-based
>>   root complex is connected to emulated endpoint devices internal to the ASIC
>>   - 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>
>> @@ -41,6 +38,16 @@ Required:
>>   - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
>>   address used by the iProc PCIe core (not the PCIe address)
>>
>> +Legacy interrupt (INTx) support (optional):
>> +
>> +Note INTx is for PAXB only.
>> +
>> +- interrupt-controller: claims itself as an interrupt controller for INTx
>> +- #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
>> +- interrupts: interrupt line wired to the generic GIC for INTx support
>> +
>>   MSI support (optional):
>>
>>   For older platforms without MSI integrated in the GIC, iProc PCIe core provides
>> @@ -77,9 +84,14 @@ Example:
>>                  compatible = "brcm,iproc-pcie";
>>                  reg = <0x18012000 0x1000>;
>>
>> +               interrupt-controller;
>>                  #interrupt-cells = <1>;
>> -               interrupt-map-mask = <0 0 0 0>;
>> -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
>> +               interrupt-map-mask = <0 0 0 7>;
>> +               interrupt-map = <0 0 0 1 &pcie0 1>,
> 
> Are you sure this works? The irq parsing code will ignore
> interrupt-map if interrupt-controller is found. In other words, you
> should have one or the other, but not both.

Yes, it does work. I tested this by using an Intel E1000E PCIe NIC card 
installed in our system and have it fall back to INTx.

> 
> Maybe it happens to work because "pcie0" is this node and your irq
> numbers are the same.

Perhaps it works because we are claiming "pcie0" as an interrupt 
controller by itself and the INTx is modeled under that.

> 
> Arnd, any thoughts on this?
> 

Please let me know if the above model makes sense or not.

Thanks,

Ray

>> +                               <0 0 0 2 &pcie0 2>,
>> +                               <0 0 0 3 &pcie0 3>,
>> +                               <0 0 0 4 &pcie0 4>;
>> +               interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
>>
>>                  linux,pci-domain = <0>;
>>
>> @@ -115,9 +127,14 @@ Example:
>>                  compatible = "brcm,iproc-pcie";
>>                  reg = <0x18013000 0x1000>;
>>
>> +               interrupt-controller;
>>                  #interrupt-cells = <1>;
>> -               interrupt-map-mask = <0 0 0 0>;
>> -               interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
>> +               interrupt-map-mask = <0 0 0 7>;
>> +               interrupt-map = <0 0 0 1 &pcie1 1>,
>> +                               <0 0 0 2 &pcie1 2>,
>> +                               <0 0 0 3 &pcie1 3>,
>> +                               <0 0 0 4 &pcie1 4>;
>> +               interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
>>
>>                  linux,pci-domain = <1>;
>>
>> --
>> 2.1.4
>>

Re: [PATCH 3/6] arm: dts: Change PCIe INTx mapping for Cygnus

[Florian Fainelli]

On 05/29/2018 02:58 PM, Ray Jui wrote:
> Change the PCIe INTx mapping to model the 4 INTx interrupts in the
> IRQ domain of the iProc PCIe controller itself
> 
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> ---
>  arch/arm/boot/dts/bcm-cygnus.dtsi | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
> index 699fdf9..6de21ef 100644
> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
> @@ -254,9 +254,14 @@
>  			compatible = "brcm,iproc-pcie";
>  			reg = <0x18012000 0x1000>;
>  
> +			interrupt-controller;
>  			#interrupt-cells = <1>;
> -			interrupt-map-mask = <0 0 0 0>;
> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> +			interrupt-map-mask = <0 0 0 7>;
> +			interrupt-map = <0 0 0 1 &pcie0 1>,
> +					<0 0 0 2 &pcie0 2>,
> +					<0 0 0 3 &pcie0 3>,
> +					<0 0 0 4 &pcie0 4>;
> +			interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;

You would want to fix those IRQ_TYPE_NONE values as well because since
commit 83a86fbb5b56b5eed8a476cc3fe214077d7c4f49 ("irqchip/gic: Loudly
complain about the use of IRQ_TYPE_NONE") this is going to create some
nice warnings on boot.

I am about to send fixes for NSP and HR2 since that's what I have access
to at the moment, but it would be good if you could send updates to the
Cygnus and NS2 DTS files?

Thanks

>  
>  			linux,pci-domain = <0>;
>  
> @@ -289,9 +294,14 @@
>  			compatible = "brcm,iproc-pcie";
>  			reg = <0x18013000 0x1000>;
>  
> +			interrupt-controller;
>  			#interrupt-cells = <1>;
> -			interrupt-map-mask = <0 0 0 0>;
> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
> +			interrupt-map-mask = <0 0 0 7>;
> +			interrupt-map = <0 0 0 1 &pcie1 1>,
> +					<0 0 0 2 &pcie1 2>,
> +					<0 0 0 3 &pcie1 3>,
> +					<0 0 0 4 &pcie1 4>;
> +			interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
>  
>  			linux,pci-domain = <1>;
>  
> 


-- 
Florian

Re: [PATCH 3/6] arm: dts: Change PCIe INTx mapping for Cygnus

[Florian Fainelli]

On 05/29/2018 02:58 PM, Ray Jui wrote:
> Change the PCIe INTx mapping to model the 4 INTx interrupts in the
> IRQ domain of the iProc PCIe controller itself
> 
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> ---
>  arch/arm/boot/dts/bcm-cygnus.dtsi | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
> index 699fdf9..6de21ef 100644
> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
> @@ -254,9 +254,14 @@
>  			compatible = "brcm,iproc-pcie";
>  			reg = <0x18012000 0x1000>;
>  
> +			interrupt-controller;
>  			#interrupt-cells = <1>;
> -			interrupt-map-mask = <0 0 0 0>;
> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> +			interrupt-map-mask = <0 0 0 7>;
> +			interrupt-map = <0 0 0 1 &pcie0 1>,
> +					<0 0 0 2 &pcie0 2>,
> +					<0 0 0 3 &pcie0 3>,
> +					<0 0 0 4 &pcie0 4>;
> +			interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;

You would want to fix those IRQ_TYPE_NONE values as well because since
commit 83a86fbb5b56b5eed8a476cc3fe214077d7c4f49 ("irqchip/gic: Loudly
complain about the use of IRQ_TYPE_NONE") this is going to create some
nice warnings on boot.

I am about to send fixes for NSP and HR2 since that's what I have access
to at the moment, but it would be good if you could send updates to the
Cygnus and NS2 DTS files?

Thanks

>  
>  			linux,pci-domain = <0>;
>  
> @@ -289,9 +294,14 @@
>  			compatible = "brcm,iproc-pcie";
>  			reg = <0x18013000 0x1000>;
>  
> +			interrupt-controller;
>  			#interrupt-cells = <1>;
> -			interrupt-map-mask = <0 0 0 0>;
> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
> +			interrupt-map-mask = <0 0 0 7>;
> +			interrupt-map = <0 0 0 1 &pcie1 1>,
> +					<0 0 0 2 &pcie1 2>,
> +					<0 0 0 3 &pcie1 3>,
> +					<0 0 0 4 &pcie1 4>;
> +			interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
>  
>  			linux,pci-domain = <1>;
>  
> 


-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/6] arm: dts: Change PCIe INTx mapping for Cygnus

[Florian Fainelli]

On 05/29/2018 02:58 PM, Ray Jui wrote:
> Change the PCIe INTx mapping to model the 4 INTx interrupts in the
> IRQ domain of the iProc PCIe controller itself
> 
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> ---
>  arch/arm/boot/dts/bcm-cygnus.dtsi | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
> index 699fdf9..6de21ef 100644
> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
> @@ -254,9 +254,14 @@
>  			compatible = "brcm,iproc-pcie";
>  			reg = <0x18012000 0x1000>;
>  
> +			interrupt-controller;
>  			#interrupt-cells = <1>;
> -			interrupt-map-mask = <0 0 0 0>;
> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> +			interrupt-map-mask = <0 0 0 7>;
> +			interrupt-map = <0 0 0 1 &pcie0 1>,
> +					<0 0 0 2 &pcie0 2>,
> +					<0 0 0 3 &pcie0 3>,
> +					<0 0 0 4 &pcie0 4>;
> +			interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;

You would want to fix those IRQ_TYPE_NONE values as well because since
commit 83a86fbb5b56b5eed8a476cc3fe214077d7c4f49 ("irqchip/gic: Loudly
complain about the use of IRQ_TYPE_NONE") this is going to create some
nice warnings on boot.

I am about to send fixes for NSP and HR2 since that's what I have access
to at the moment, but it would be good if you could send updates to the
Cygnus and NS2 DTS files?

Thanks

>  
>  			linux,pci-domain = <0>;
>  
> @@ -289,9 +294,14 @@
>  			compatible = "brcm,iproc-pcie";
>  			reg = <0x18013000 0x1000>;
>  
> +			interrupt-controller;
>  			#interrupt-cells = <1>;
> -			interrupt-map-mask = <0 0 0 0>;
> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
> +			interrupt-map-mask = <0 0 0 7>;
> +			interrupt-map = <0 0 0 1 &pcie1 1>,
> +					<0 0 0 2 &pcie1 2>,
> +					<0 0 0 3 &pcie1 3>,
> +					<0 0 0 4 &pcie1 4>;
> +			interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
>  
>  			linux,pci-domain = <1>;
>  
> 


-- 
Florian

Re: [PATCH 3/6] arm: dts: Change PCIe INTx mapping for Cygnus

[Ray Jui]

On 6/11/2018 3:36 PM, Florian Fainelli wrote:
> On 05/29/2018 02:58 PM, Ray Jui wrote:
>> Change the PCIe INTx mapping to model the 4 INTx interrupts in the
>> IRQ domain of the iProc PCIe controller itself
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> ---
>>   arch/arm/boot/dts/bcm-cygnus.dtsi | 18 ++++++++++++++----
>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
>> index 699fdf9..6de21ef 100644
>> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
>> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
>> @@ -254,9 +254,14 @@
>>   			compatible = "brcm,iproc-pcie";
>>   			reg = <0x18012000 0x1000>;
>>   
>> +			interrupt-controller;
>>   			#interrupt-cells = <1>;
>> -			interrupt-map-mask = <0 0 0 0>;
>> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
>> +			interrupt-map-mask = <0 0 0 7>;
>> +			interrupt-map = <0 0 0 1 &pcie0 1>,
>> +					<0 0 0 2 &pcie0 2>,
>> +					<0 0 0 3 &pcie0 3>,
>> +					<0 0 0 4 &pcie0 4>;
>> +			interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
> 
> You would want to fix those IRQ_TYPE_NONE values as well because since
> commit 83a86fbb5b56b5eed8a476cc3fe214077d7c4f49 ("irqchip/gic: Loudly
> complain about the use of IRQ_TYPE_NONE") this is going to create some
> nice warnings on boot.
> 
> I am about to send fixes for NSP and HR2 since that's what I have access
> to at the moment, but it would be good if you could send updates to the
> Cygnus and NS2 DTS files?
> 
> Thanks
> 

Okay. Thanks for letting me know. How do you want this to be done for 
Cygnus and NS2?

I guess I should have the fix patches to DTS done and sent out first, 
and then rebase this INTx patch series against the patches with the fix.

Does that make sense to you?

Thanks,

Ray

>>   
>>   			linux,pci-domain = <0>;
>>   
>> @@ -289,9 +294,14 @@
>>   			compatible = "brcm,iproc-pcie";
>>   			reg = <0x18013000 0x1000>;
>>   
>> +			interrupt-controller;
>>   			#interrupt-cells = <1>;
>> -			interrupt-map-mask = <0 0 0 0>;
>> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
>> +			interrupt-map-mask = <0 0 0 7>;
>> +			interrupt-map = <0 0 0 1 &pcie1 1>,
>> +					<0 0 0 2 &pcie1 2>,
>> +					<0 0 0 3 &pcie1 3>,
>> +					<0 0 0 4 &pcie1 4>;
>> +			interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
>>   
>>   			linux,pci-domain = <1>;
>>   
>>
> 
> 

[PATCH 3/6] arm: dts: Change PCIe INTx mapping for Cygnus

[Ray Jui]

On 6/11/2018 3:36 PM, Florian Fainelli wrote:
> On 05/29/2018 02:58 PM, Ray Jui wrote:
>> Change the PCIe INTx mapping to model the 4 INTx interrupts in the
>> IRQ domain of the iProc PCIe controller itself
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> ---
>>   arch/arm/boot/dts/bcm-cygnus.dtsi | 18 ++++++++++++++----
>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
>> index 699fdf9..6de21ef 100644
>> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
>> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
>> @@ -254,9 +254,14 @@
>>   			compatible = "brcm,iproc-pcie";
>>   			reg = <0x18012000 0x1000>;
>>   
>> +			interrupt-controller;
>>   			#interrupt-cells = <1>;
>> -			interrupt-map-mask = <0 0 0 0>;
>> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
>> +			interrupt-map-mask = <0 0 0 7>;
>> +			interrupt-map = <0 0 0 1 &pcie0 1>,
>> +					<0 0 0 2 &pcie0 2>,
>> +					<0 0 0 3 &pcie0 3>,
>> +					<0 0 0 4 &pcie0 4>;
>> +			interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
> 
> You would want to fix those IRQ_TYPE_NONE values as well because since
> commit 83a86fbb5b56b5eed8a476cc3fe214077d7c4f49 ("irqchip/gic: Loudly
> complain about the use of IRQ_TYPE_NONE") this is going to create some
> nice warnings on boot.
> 
> I am about to send fixes for NSP and HR2 since that's what I have access
> to at the moment, but it would be good if you could send updates to the
> Cygnus and NS2 DTS files?
> 
> Thanks
> 

Okay. Thanks for letting me know. How do you want this to be done for 
Cygnus and NS2?

I guess I should have the fix patches to DTS done and sent out first, 
and then rebase this INTx patch series against the patches with the fix.

Does that make sense to you?

Thanks,

Ray

>>   
>>   			linux,pci-domain = <0>;
>>   
>> @@ -289,9 +294,14 @@
>>   			compatible = "brcm,iproc-pcie";
>>   			reg = <0x18013000 0x1000>;
>>   
>> +			interrupt-controller;
>>   			#interrupt-cells = <1>;
>> -			interrupt-map-mask = <0 0 0 0>;
>> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
>> +			interrupt-map-mask = <0 0 0 7>;
>> +			interrupt-map = <0 0 0 1 &pcie1 1>,
>> +					<0 0 0 2 &pcie1 2>,
>> +					<0 0 0 3 &pcie1 3>,
>> +					<0 0 0 4 &pcie1 4>;
>> +			interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
>>   
>>   			linux,pci-domain = <1>;
>>   
>>
> 
> 

Re: [PATCH 3/6] arm: dts: Change PCIe INTx mapping for Cygnus

[Florian Fainelli]

On June 11, 2018 5:27:20 PM PDT, Ray Jui <ray.jui@broadcom.com> wrote:
>
>
>On 6/11/2018 3:36 PM, Florian Fainelli wrote:
>> On 05/29/2018 02:58 PM, Ray Jui wrote:
>>> Change the PCIe INTx mapping to model the 4 INTx interrupts in the
>>> IRQ domain of the iProc PCIe controller itself
>>>
>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>> ---
>>>   arch/arm/boot/dts/bcm-cygnus.dtsi | 18 ++++++++++++++----
>>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
>b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> index 699fdf9..6de21ef 100644
>>> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> @@ -254,9 +254,14 @@
>>>   			compatible = "brcm,iproc-pcie";
>>>   			reg = <0x18012000 0x1000>;
>>>   
>>> +			interrupt-controller;
>>>   			#interrupt-cells = <1>;
>>> -			interrupt-map-mask = <0 0 0 0>;
>>> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
>>> +			interrupt-map-mask = <0 0 0 7>;
>>> +			interrupt-map = <0 0 0 1 &pcie0 1>,
>>> +					<0 0 0 2 &pcie0 2>,
>>> +					<0 0 0 3 &pcie0 3>,
>>> +					<0 0 0 4 &pcie0 4>;
>>> +			interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
>> 
>> You would want to fix those IRQ_TYPE_NONE values as well because
>since
>> commit 83a86fbb5b56b5eed8a476cc3fe214077d7c4f49 ("irqchip/gic: Loudly
>> complain about the use of IRQ_TYPE_NONE") this is going to create
>some
>> nice warnings on boot.
>> 
>> I am about to send fixes for NSP and HR2 since that's what I have
>access
>> to at the moment, but it would be good if you could send updates to
>the
>> Cygnus and NS2 DTS files?
>> 
>> Thanks
>> 
>
>Okay. Thanks for letting me know. How do you want this to be done for 
>Cygnus and NS2?
>
>I guess I should have the fix patches to DTS done and sent out first, 
>and then rebase this INTx patch series against the patches with the
>fix.
>
>Does that make sense to you?

Yes it does. As soon as v4.18-rc1 is tagged I will send the Device Tree fixes pull request, if I can have yours for Cygnus and NS2 in the next few days I could lump those together. Then we can either base your changes off v4.18-rc2 or a later tag that already has the DTS fixes.

-- 
Florian

Re: [PATCH 3/6] arm: dts: Change PCIe INTx mapping for Cygnus

[Florian Fainelli]

On June 11, 2018 5:27:20 PM PDT, Ray Jui <ray.jui@broadcom.com> wrote:
>
>
>On 6/11/2018 3:36 PM, Florian Fainelli wrote:
>> On 05/29/2018 02:58 PM, Ray Jui wrote:
>>> Change the PCIe INTx mapping to model the 4 INTx interrupts in the
>>> IRQ domain of the iProc PCIe controller itself
>>>
>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>> ---
>>>   arch/arm/boot/dts/bcm-cygnus.dtsi | 18 ++++++++++++++----
>>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
>b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> index 699fdf9..6de21ef 100644
>>> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> @@ -254,9 +254,14 @@
>>>   			compatible = "brcm,iproc-pcie";
>>>   			reg = <0x18012000 0x1000>;
>>>   
>>> +			interrupt-controller;
>>>   			#interrupt-cells = <1>;
>>> -			interrupt-map-mask = <0 0 0 0>;
>>> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
>>> +			interrupt-map-mask = <0 0 0 7>;
>>> +			interrupt-map = <0 0 0 1 &pcie0 1>,
>>> +					<0 0 0 2 &pcie0 2>,
>>> +					<0 0 0 3 &pcie0 3>,
>>> +					<0 0 0 4 &pcie0 4>;
>>> +			interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
>> 
>> You would want to fix those IRQ_TYPE_NONE values as well because
>since
>> commit 83a86fbb5b56b5eed8a476cc3fe214077d7c4f49 ("irqchip/gic: Loudly
>> complain about the use of IRQ_TYPE_NONE") this is going to create
>some
>> nice warnings on boot.
>> 
>> I am about to send fixes for NSP and HR2 since that's what I have
>access
>> to at the moment, but it would be good if you could send updates to
>the
>> Cygnus and NS2 DTS files?
>> 
>> Thanks
>> 
>
>Okay. Thanks for letting me know. How do you want this to be done for 
>Cygnus and NS2?
>
>I guess I should have the fix patches to DTS done and sent out first, 
>and then rebase this INTx patch series against the patches with the
>fix.
>
>Does that make sense to you?

Yes it does. As soon as v4.18-rc1 is tagged I will send the Device Tree fixes pull request, if I can have yours for Cygnus and NS2 in the next few days I could lump those together. Then we can either base your changes off v4.18-rc2 or a later tag that already has the DTS fixes.

-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/6] arm: dts: Change PCIe INTx mapping for Cygnus

[Florian Fainelli]

On June 11, 2018 5:27:20 PM PDT, Ray Jui <ray.jui@broadcom.com> wrote:
>
>
>On 6/11/2018 3:36 PM, Florian Fainelli wrote:
>> On 05/29/2018 02:58 PM, Ray Jui wrote:
>>> Change the PCIe INTx mapping to model the 4 INTx interrupts in the
>>> IRQ domain of the iProc PCIe controller itself
>>>
>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>> ---
>>>   arch/arm/boot/dts/bcm-cygnus.dtsi | 18 ++++++++++++++----
>>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
>b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> index 699fdf9..6de21ef 100644
>>> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> @@ -254,9 +254,14 @@
>>>   			compatible = "brcm,iproc-pcie";
>>>   			reg = <0x18012000 0x1000>;
>>>   
>>> +			interrupt-controller;
>>>   			#interrupt-cells = <1>;
>>> -			interrupt-map-mask = <0 0 0 0>;
>>> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
>>> +			interrupt-map-mask = <0 0 0 7>;
>>> +			interrupt-map = <0 0 0 1 &pcie0 1>,
>>> +					<0 0 0 2 &pcie0 2>,
>>> +					<0 0 0 3 &pcie0 3>,
>>> +					<0 0 0 4 &pcie0 4>;
>>> +			interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
>> 
>> You would want to fix those IRQ_TYPE_NONE values as well because
>since
>> commit 83a86fbb5b56b5eed8a476cc3fe214077d7c4f49 ("irqchip/gic: Loudly
>> complain about the use of IRQ_TYPE_NONE") this is going to create
>some
>> nice warnings on boot.
>> 
>> I am about to send fixes for NSP and HR2 since that's what I have
>access
>> to at the moment, but it would be good if you could send updates to
>the
>> Cygnus and NS2 DTS files?
>> 
>> Thanks
>> 
>
>Okay. Thanks for letting me know. How do you want this to be done for 
>Cygnus and NS2?
>
>I guess I should have the fix patches to DTS done and sent out first, 
>and then rebase this INTx patch series against the patches with the
>fix.
>
>Does that make sense to you?

Yes it does. As soon as v4.18-rc1 is tagged I will send the Device Tree fixes pull request, if I can have yours for Cygnus and NS2 in the next few days I could lump those together. Then we can either base your changes off v4.18-rc2 or a later tag that already has the DTS fixes.

-- 
Florian

Re: [PATCH 3/6] arm: dts: Change PCIe INTx mapping for Cygnus

[Ray Jui]

On 6/11/2018 5:55 PM, Florian Fainelli wrote:
> On June 11, 2018 5:27:20 PM PDT, Ray Jui <ray.jui@broadcom.com> wrote:
>>
>>
>> On 6/11/2018 3:36 PM, Florian Fainelli wrote:
>>> On 05/29/2018 02:58 PM, Ray Jui wrote:
>>>> Change the PCIe INTx mapping to model the 4 INTx interrupts in the
>>>> IRQ domain of the iProc PCIe controller itself
>>>>
>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>>> ---
>>>>    arch/arm/boot/dts/bcm-cygnus.dtsi | 18 ++++++++++++++----
>>>>    1 file changed, 14 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
>> b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>>> index 699fdf9..6de21ef 100644
>>>> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
>>>> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>>> @@ -254,9 +254,14 @@
>>>>    			compatible = "brcm,iproc-pcie";
>>>>    			reg = <0x18012000 0x1000>;
>>>>    
>>>> +			interrupt-controller;
>>>>    			#interrupt-cells = <1>;
>>>> -			interrupt-map-mask = <0 0 0 0>;
>>>> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
>>>> +			interrupt-map-mask = <0 0 0 7>;
>>>> +			interrupt-map = <0 0 0 1 &pcie0 1>,
>>>> +					<0 0 0 2 &pcie0 2>,
>>>> +					<0 0 0 3 &pcie0 3>,
>>>> +					<0 0 0 4 &pcie0 4>;
>>>> +			interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
>>>
>>> You would want to fix those IRQ_TYPE_NONE values as well because
>> since
>>> commit 83a86fbb5b56b5eed8a476cc3fe214077d7c4f49 ("irqchip/gic: Loudly
>>> complain about the use of IRQ_TYPE_NONE") this is going to create
>> some
>>> nice warnings on boot.
>>>
>>> I am about to send fixes for NSP and HR2 since that's what I have
>> access
>>> to at the moment, but it would be good if you could send updates to
>> the
>>> Cygnus and NS2 DTS files?
>>>
>>> Thanks
>>>
>>
>> Okay. Thanks for letting me know. How do you want this to be done for
>> Cygnus and NS2?
>>
>> I guess I should have the fix patches to DTS done and sent out first,
>> and then rebase this INTx patch series against the patches with the
>> fix.
>>
>> Does that make sense to you?
> 
> Yes it does. As soon as v4.18-rc1 is tagged I will send the Device Tree fixes pull request, if I can have yours for Cygnus and NS2 in the next few days I could lump those together. Then we can either base your changes off v4.18-rc2 or a later tag that already has the DTS fixes.
> 

Okay that sounds good. I'll try to get those fixes out for you within 
the next few days.

Thanks!

Ray

[PATCH 3/6] arm: dts: Change PCIe INTx mapping for Cygnus

[Ray Jui]

On 6/11/2018 5:55 PM, Florian Fainelli wrote:
> On June 11, 2018 5:27:20 PM PDT, Ray Jui <ray.jui@broadcom.com> wrote:
>>
>>
>> On 6/11/2018 3:36 PM, Florian Fainelli wrote:
>>> On 05/29/2018 02:58 PM, Ray Jui wrote:
>>>> Change the PCIe INTx mapping to model the 4 INTx interrupts in the
>>>> IRQ domain of the iProc PCIe controller itself
>>>>
>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>>> ---
>>>>    arch/arm/boot/dts/bcm-cygnus.dtsi | 18 ++++++++++++++----
>>>>    1 file changed, 14 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
>> b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>>> index 699fdf9..6de21ef 100644
>>>> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
>>>> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>>> @@ -254,9 +254,14 @@
>>>>    			compatible = "brcm,iproc-pcie";
>>>>    			reg = <0x18012000 0x1000>;
>>>>    
>>>> +			interrupt-controller;
>>>>    			#interrupt-cells = <1>;
>>>> -			interrupt-map-mask = <0 0 0 0>;
>>>> -			interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
>>>> +			interrupt-map-mask = <0 0 0 7>;
>>>> +			interrupt-map = <0 0 0 1 &pcie0 1>,
>>>> +					<0 0 0 2 &pcie0 2>,
>>>> +					<0 0 0 3 &pcie0 3>,
>>>> +					<0 0 0 4 &pcie0 4>;
>>>> +			interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
>>>
>>> You would want to fix those IRQ_TYPE_NONE values as well because
>> since
>>> commit 83a86fbb5b56b5eed8a476cc3fe214077d7c4f49 ("irqchip/gic: Loudly
>>> complain about the use of IRQ_TYPE_NONE") this is going to create
>> some
>>> nice warnings on boot.
>>>
>>> I am about to send fixes for NSP and HR2 since that's what I have
>> access
>>> to at the moment, but it would be good if you could send updates to
>> the
>>> Cygnus and NS2 DTS files?
>>>
>>> Thanks
>>>
>>
>> Okay. Thanks for letting me know. How do you want this to be done for
>> Cygnus and NS2?
>>
>> I guess I should have the fix patches to DTS done and sent out first,
>> and then rebase this INTx patch series against the patches with the
>> fix.
>>
>> Does that make sense to you?
> 
> Yes it does. As soon as v4.18-rc1 is tagged I will send the Device Tree fixes pull request, if I can have yours for Cygnus and NS2 in the next few days I could lump those together. Then we can either base your changes off v4.18-rc2 or a later tag that already has the DTS fixes.
> 

Okay that sounds good. I'll try to get those fixes out for you within 
the next few days.

Thanks!

Ray

Re: [PATCH 2/6] PCI: iproc: Add INTx support with better modeling

[poza]

On 2018-05-30 03:28, Ray Jui wrote:
> Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
> modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
> INTD share the same interrupt line connected to the GIC in the system,
> while the status of each INTx can be obtained through the INTX CSR
> register
> 
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> ---
>  drivers/pci/host/pcie-iproc-platform.c |  2 +
>  drivers/pci/host/pcie-iproc.c          | 95 
> +++++++++++++++++++++++++++++++++-
>  drivers/pci/host/pcie-iproc.h          |  6 +++
>  3 files changed, 101 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc-platform.c
> b/drivers/pci/host/pcie-iproc-platform.c
> index e764a2a..7a51e6c 100644
> --- a/drivers/pci/host/pcie-iproc-platform.c
> +++ b/drivers/pci/host/pcie-iproc-platform.c
> @@ -70,6 +70,8 @@ static int iproc_pcie_pltfm_probe(struct
> platform_device *pdev)
>  	}
>  	pcie->base_addr = reg.start;
> 
> +	pcie->irq = platform_get_irq(pdev, 0);
> +
>  	if (of_property_read_bool(np, "brcm,pcie-ob")) {
>  		u32 val;
> 
> diff --git a/drivers/pci/host/pcie-iproc.c 
> b/drivers/pci/host/pcie-iproc.c
> index 14f374d..0bd2c14 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -14,6 +14,7 @@
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
>  #include <linux/irqchip/arm-gic-v3.h>
> +#include <linux/irqchip/chained_irq.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_address.h>
>  #include <linux/of_pci.h>
> @@ -264,6 +265,7 @@ enum iproc_pcie_reg {
> 
>  	/* enable INTx */
>  	IPROC_PCIE_INTX_EN,
> +	IPROC_PCIE_INTX_CSR,
> 
>  	/* outbound address mapping */
>  	IPROC_PCIE_OARR0,
> @@ -305,6 +307,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = {
>  	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
>  	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
>  	[IPROC_PCIE_INTX_EN]		= 0x330,
> +	[IPROC_PCIE_INTX_CSR]		= 0x334,
>  	[IPROC_PCIE_LINK_STATUS]	= 0xf0c,
>  };
> 
> @@ -316,6 +319,7 @@ static const u16 iproc_pcie_reg_paxb[] = {
>  	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
>  	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
>  	[IPROC_PCIE_INTX_EN]		= 0x330,
> +	[IPROC_PCIE_INTX_CSR]		= 0x334,
>  	[IPROC_PCIE_OARR0]		= 0xd20,
>  	[IPROC_PCIE_OMAP0]		= 0xd40,
>  	[IPROC_PCIE_OARR1]		= 0xd28,
> @@ -332,6 +336,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = {
>  	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
>  	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
>  	[IPROC_PCIE_INTX_EN]		= 0x330,
> +	[IPROC_PCIE_INTX_CSR]		= 0x334,
>  	[IPROC_PCIE_OARR0]		= 0xd20,
>  	[IPROC_PCIE_OMAP0]		= 0xd40,
>  	[IPROC_PCIE_OARR1]		= 0xd28,
> @@ -782,9 +787,90 @@ static int iproc_pcie_check_link(struct iproc_pcie 
> *pcie)
>  	return link_is_active ? 0 : -ENODEV;
>  }
> 
> -static void iproc_pcie_enable(struct iproc_pcie *pcie)
> +static int iproc_pcie_intx_map(struct irq_domain *domain, unsigned int 
> irq,
> +			       irq_hw_number_t hwirq)
>  {
> +	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops intx_domain_ops = {
> +	.map = iproc_pcie_intx_map,
> +};
> +
> +static void iproc_pcie_isr(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct iproc_pcie *pcie;
> +	struct device *dev;
> +	unsigned long status;
> +	u32 bit, virq;
> +
> +	chained_irq_enter(chip, desc);
> +	pcie = irq_desc_get_handler_data(desc);
> +	dev = pcie->dev;
> +
> +	/* go through INTx A, B, C, D until all interrupts are handled */
> +	while ((status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR) &
> +		SYS_RC_INTX_MASK) != 0) {
> +		for_each_set_bit(bit, &status, PCI_NUM_INTX) {
> +			virq = irq_find_mapping(pcie->irq_domain, bit + 1);
> +			if (virq)
> +				generic_handle_irq(virq);
> +			else
> +				dev_err(dev, "unexpected INTx%u\n", bit);
> +		}
> +	}
> +

Are these level or edge interrupts ? although I see DT says: 
IRQ_TYPE_NONE
do you not need to clear interrupt status bits in IPROC_PCIE_INTX_CSR ?

> +	chained_irq_exit(chip, desc);
> +}
> +
> +static int iproc_pcie_intx_enable(struct iproc_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	struct device_node *node = dev->of_node;
> +	int ret;
> +
>  	iproc_pcie_write_reg(pcie, IPROC_PCIE_INTX_EN, SYS_RC_INTX_MASK);
> +
> +	/*
> +	 * BCMA devices do not map INTx the same way as platform devices. All
> +	 * BCMA needs is the above code to enable INTx
> +	 */
> +	if (pcie->irq <= 0)
> +		return 0;
> +
> +	/* set IRQ handler */
> +	irq_set_chained_handler_and_data(pcie->irq, iproc_pcie_isr, pcie);
> +
> +	/* add IRQ domain for INTx */
> +	pcie->irq_domain = irq_domain_add_linear(node, PCI_NUM_INTX + 1,
> +						 &intx_domain_ops, pcie);
> +	if (!pcie->irq_domain) {
> +		dev_err(dev, "failed to add INTx IRQ domain\n");
> +		ret = -ENOMEM;
> +		goto err_rm_handler_data;
> +	}
> +
> +	return 0;
> +
> +err_rm_handler_data:
> +	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
> +
> +	return ret;
> +}
> +
> +static void iproc_pcie_intx_disable(struct iproc_pcie *pcie)
> +{
> +	iproc_pcie_write_reg(pcie, IPROC_PCIE_INTX_EN, 0x0);
> +
> +	if (pcie->irq <= 0)
> +		return;
> +
> +	irq_domain_remove(pcie->irq_domain);
> +	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
>  }
> 
>  static inline bool iproc_pcie_ob_is_valid(struct iproc_pcie *pcie,
> @@ -1410,7 +1496,11 @@ int iproc_pcie_setup(struct iproc_pcie *pcie,
> struct list_head *res)
>  		goto err_power_off_phy;
>  	}
> 
> -	iproc_pcie_enable(pcie);
> +	ret = iproc_pcie_intx_enable(pcie);
> +	if (ret) {
> +		dev_err(dev, "failed to enable INTx\n");
> +		goto err_power_off_phy;
> +	}
> 
>  	if (IS_ENABLED(CONFIG_PCI_MSI))
>  		if (iproc_pcie_msi_enable(pcie))
> @@ -1455,6 +1545,7 @@ int iproc_pcie_remove(struct iproc_pcie *pcie)
>  	pci_remove_root_bus(pcie->root_bus);
> 
>  	iproc_pcie_msi_disable(pcie);
> +	iproc_pcie_intx_disable(pcie);
> 
>  	phy_power_off(pcie->phy);
>  	phy_exit(pcie->phy);
> diff --git a/drivers/pci/host/pcie-iproc.h 
> b/drivers/pci/host/pcie-iproc.h
> index 67081cb..cbcaf9d 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -72,6 +72,9 @@ struct iproc_msi;
>   * @ib: inbound mapping related parameters
>   * @ib_map: outbound mapping region related parameters
>   *
> + * @irq: interrupt line wired to the generic GIC for INTx
> + * @irq_domain: IRQ domain for INTx
> + *
>   * @need_msi_steer: indicates additional configuration of the iProc 
> PCIe
>   * controller is required to steer MSI writes to external interrupt 
> controller
>   * @msi: MSI data
> @@ -99,6 +102,9 @@ struct iproc_pcie {
>  	struct iproc_pcie_ib ib;
>  	const struct iproc_pcie_ib_map *ib_map;
> 
> +	int irq;
> +	struct irq_domain *irq_domain;
> +
>  	bool need_msi_steer;
>  	struct iproc_msi *msi;
>  };

Re: [PATCH 2/6] PCI: iproc: Add INTx support with better modeling

[poza]

On 2018-05-30 03:28, Ray Jui wrote:
> Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
> modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
> INTD share the same interrupt line connected to the GIC in the system,
> while the status of each INTx can be obtained through the INTX CSR
> register
> 
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> ---
>  drivers/pci/host/pcie-iproc-platform.c |  2 +
>  drivers/pci/host/pcie-iproc.c          | 95 
> +++++++++++++++++++++++++++++++++-
>  drivers/pci/host/pcie-iproc.h          |  6 +++
>  3 files changed, 101 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc-platform.c
> b/drivers/pci/host/pcie-iproc-platform.c
> index e764a2a..7a51e6c 100644
> --- a/drivers/pci/host/pcie-iproc-platform.c
> +++ b/drivers/pci/host/pcie-iproc-platform.c
> @@ -70,6 +70,8 @@ static int iproc_pcie_pltfm_probe(struct
> platform_device *pdev)
>  	}
>  	pcie->base_addr = reg.start;
> 
> +	pcie->irq = platform_get_irq(pdev, 0);
> +
>  	if (of_property_read_bool(np, "brcm,pcie-ob")) {
>  		u32 val;
> 
> diff --git a/drivers/pci/host/pcie-iproc.c 
> b/drivers/pci/host/pcie-iproc.c
> index 14f374d..0bd2c14 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -14,6 +14,7 @@
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
>  #include <linux/irqchip/arm-gic-v3.h>
> +#include <linux/irqchip/chained_irq.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_address.h>
>  #include <linux/of_pci.h>
> @@ -264,6 +265,7 @@ enum iproc_pcie_reg {
> 
>  	/* enable INTx */
>  	IPROC_PCIE_INTX_EN,
> +	IPROC_PCIE_INTX_CSR,
> 
>  	/* outbound address mapping */
>  	IPROC_PCIE_OARR0,
> @@ -305,6 +307,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = {
>  	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
>  	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
>  	[IPROC_PCIE_INTX_EN]		= 0x330,
> +	[IPROC_PCIE_INTX_CSR]		= 0x334,
>  	[IPROC_PCIE_LINK_STATUS]	= 0xf0c,
>  };
> 
> @@ -316,6 +319,7 @@ static const u16 iproc_pcie_reg_paxb[] = {
>  	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
>  	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
>  	[IPROC_PCIE_INTX_EN]		= 0x330,
> +	[IPROC_PCIE_INTX_CSR]		= 0x334,
>  	[IPROC_PCIE_OARR0]		= 0xd20,
>  	[IPROC_PCIE_OMAP0]		= 0xd40,
>  	[IPROC_PCIE_OARR1]		= 0xd28,
> @@ -332,6 +336,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = {
>  	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
>  	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
>  	[IPROC_PCIE_INTX_EN]		= 0x330,
> +	[IPROC_PCIE_INTX_CSR]		= 0x334,
>  	[IPROC_PCIE_OARR0]		= 0xd20,
>  	[IPROC_PCIE_OMAP0]		= 0xd40,
>  	[IPROC_PCIE_OARR1]		= 0xd28,
> @@ -782,9 +787,90 @@ static int iproc_pcie_check_link(struct iproc_pcie 
> *pcie)
>  	return link_is_active ? 0 : -ENODEV;
>  }
> 
> -static void iproc_pcie_enable(struct iproc_pcie *pcie)
> +static int iproc_pcie_intx_map(struct irq_domain *domain, unsigned int 
> irq,
> +			       irq_hw_number_t hwirq)
>  {
> +	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops intx_domain_ops = {
> +	.map = iproc_pcie_intx_map,
> +};
> +
> +static void iproc_pcie_isr(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct iproc_pcie *pcie;
> +	struct device *dev;
> +	unsigned long status;
> +	u32 bit, virq;
> +
> +	chained_irq_enter(chip, desc);
> +	pcie = irq_desc_get_handler_data(desc);
> +	dev = pcie->dev;
> +
> +	/* go through INTx A, B, C, D until all interrupts are handled */
> +	while ((status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR) &
> +		SYS_RC_INTX_MASK) != 0) {
> +		for_each_set_bit(bit, &status, PCI_NUM_INTX) {
> +			virq = irq_find_mapping(pcie->irq_domain, bit + 1);
> +			if (virq)
> +				generic_handle_irq(virq);
> +			else
> +				dev_err(dev, "unexpected INTx%u\n", bit);
> +		}
> +	}
> +

Are these level or edge interrupts ? although I see DT says: 
IRQ_TYPE_NONE
do you not need to clear interrupt status bits in IPROC_PCIE_INTX_CSR ?

> +	chained_irq_exit(chip, desc);
> +}
> +
> +static int iproc_pcie_intx_enable(struct iproc_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	struct device_node *node = dev->of_node;
> +	int ret;
> +
>  	iproc_pcie_write_reg(pcie, IPROC_PCIE_INTX_EN, SYS_RC_INTX_MASK);
> +
> +	/*
> +	 * BCMA devices do not map INTx the same way as platform devices. All
> +	 * BCMA needs is the above code to enable INTx
> +	 */
> +	if (pcie->irq <= 0)
> +		return 0;
> +
> +	/* set IRQ handler */
> +	irq_set_chained_handler_and_data(pcie->irq, iproc_pcie_isr, pcie);
> +
> +	/* add IRQ domain for INTx */
> +	pcie->irq_domain = irq_domain_add_linear(node, PCI_NUM_INTX + 1,
> +						 &intx_domain_ops, pcie);
> +	if (!pcie->irq_domain) {
> +		dev_err(dev, "failed to add INTx IRQ domain\n");
> +		ret = -ENOMEM;
> +		goto err_rm_handler_data;
> +	}
> +
> +	return 0;
> +
> +err_rm_handler_data:
> +	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
> +
> +	return ret;
> +}
> +
> +static void iproc_pcie_intx_disable(struct iproc_pcie *pcie)
> +{
> +	iproc_pcie_write_reg(pcie, IPROC_PCIE_INTX_EN, 0x0);
> +
> +	if (pcie->irq <= 0)
> +		return;
> +
> +	irq_domain_remove(pcie->irq_domain);
> +	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
>  }
> 
>  static inline bool iproc_pcie_ob_is_valid(struct iproc_pcie *pcie,
> @@ -1410,7 +1496,11 @@ int iproc_pcie_setup(struct iproc_pcie *pcie,
> struct list_head *res)
>  		goto err_power_off_phy;
>  	}
> 
> -	iproc_pcie_enable(pcie);
> +	ret = iproc_pcie_intx_enable(pcie);
> +	if (ret) {
> +		dev_err(dev, "failed to enable INTx\n");
> +		goto err_power_off_phy;
> +	}
> 
>  	if (IS_ENABLED(CONFIG_PCI_MSI))
>  		if (iproc_pcie_msi_enable(pcie))
> @@ -1455,6 +1545,7 @@ int iproc_pcie_remove(struct iproc_pcie *pcie)
>  	pci_remove_root_bus(pcie->root_bus);
> 
>  	iproc_pcie_msi_disable(pcie);
> +	iproc_pcie_intx_disable(pcie);
> 
>  	phy_power_off(pcie->phy);
>  	phy_exit(pcie->phy);
> diff --git a/drivers/pci/host/pcie-iproc.h 
> b/drivers/pci/host/pcie-iproc.h
> index 67081cb..cbcaf9d 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -72,6 +72,9 @@ struct iproc_msi;
>   * @ib: inbound mapping related parameters
>   * @ib_map: outbound mapping region related parameters
>   *
> + * @irq: interrupt line wired to the generic GIC for INTx
> + * @irq_domain: IRQ domain for INTx
> + *
>   * @need_msi_steer: indicates additional configuration of the iProc 
> PCIe
>   * controller is required to steer MSI writes to external interrupt 
> controller
>   * @msi: MSI data
> @@ -99,6 +102,9 @@ struct iproc_pcie {
>  	struct iproc_pcie_ib ib;
>  	const struct iproc_pcie_ib_map *ib_map;
> 
> +	int irq;
> +	struct irq_domain *irq_domain;
> +
>  	bool need_msi_steer;
>  	struct iproc_msi *msi;
>  };

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/6] PCI: iproc: Add INTx support with better modeling

[poza at codeaurora.org]

On 2018-05-30 03:28, Ray Jui wrote:
> Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
> modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
> INTD share the same interrupt line connected to the GIC in the system,
> while the status of each INTx can be obtained through the INTX CSR
> register
> 
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> ---
>  drivers/pci/host/pcie-iproc-platform.c |  2 +
>  drivers/pci/host/pcie-iproc.c          | 95 
> +++++++++++++++++++++++++++++++++-
>  drivers/pci/host/pcie-iproc.h          |  6 +++
>  3 files changed, 101 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc-platform.c
> b/drivers/pci/host/pcie-iproc-platform.c
> index e764a2a..7a51e6c 100644
> --- a/drivers/pci/host/pcie-iproc-platform.c
> +++ b/drivers/pci/host/pcie-iproc-platform.c
> @@ -70,6 +70,8 @@ static int iproc_pcie_pltfm_probe(struct
> platform_device *pdev)
>  	}
>  	pcie->base_addr = reg.start;
> 
> +	pcie->irq = platform_get_irq(pdev, 0);
> +
>  	if (of_property_read_bool(np, "brcm,pcie-ob")) {
>  		u32 val;
> 
> diff --git a/drivers/pci/host/pcie-iproc.c 
> b/drivers/pci/host/pcie-iproc.c
> index 14f374d..0bd2c14 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -14,6 +14,7 @@
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
>  #include <linux/irqchip/arm-gic-v3.h>
> +#include <linux/irqchip/chained_irq.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_address.h>
>  #include <linux/of_pci.h>
> @@ -264,6 +265,7 @@ enum iproc_pcie_reg {
> 
>  	/* enable INTx */
>  	IPROC_PCIE_INTX_EN,
> +	IPROC_PCIE_INTX_CSR,
> 
>  	/* outbound address mapping */
>  	IPROC_PCIE_OARR0,
> @@ -305,6 +307,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = {
>  	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
>  	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
>  	[IPROC_PCIE_INTX_EN]		= 0x330,
> +	[IPROC_PCIE_INTX_CSR]		= 0x334,
>  	[IPROC_PCIE_LINK_STATUS]	= 0xf0c,
>  };
> 
> @@ -316,6 +319,7 @@ static const u16 iproc_pcie_reg_paxb[] = {
>  	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
>  	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
>  	[IPROC_PCIE_INTX_EN]		= 0x330,
> +	[IPROC_PCIE_INTX_CSR]		= 0x334,
>  	[IPROC_PCIE_OARR0]		= 0xd20,
>  	[IPROC_PCIE_OMAP0]		= 0xd40,
>  	[IPROC_PCIE_OARR1]		= 0xd28,
> @@ -332,6 +336,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = {
>  	[IPROC_PCIE_CFG_ADDR]		= 0x1f8,
>  	[IPROC_PCIE_CFG_DATA]		= 0x1fc,
>  	[IPROC_PCIE_INTX_EN]		= 0x330,
> +	[IPROC_PCIE_INTX_CSR]		= 0x334,
>  	[IPROC_PCIE_OARR0]		= 0xd20,
>  	[IPROC_PCIE_OMAP0]		= 0xd40,
>  	[IPROC_PCIE_OARR1]		= 0xd28,
> @@ -782,9 +787,90 @@ static int iproc_pcie_check_link(struct iproc_pcie 
> *pcie)
>  	return link_is_active ? 0 : -ENODEV;
>  }
> 
> -static void iproc_pcie_enable(struct iproc_pcie *pcie)
> +static int iproc_pcie_intx_map(struct irq_domain *domain, unsigned int 
> irq,
> +			       irq_hw_number_t hwirq)
>  {
> +	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops intx_domain_ops = {
> +	.map = iproc_pcie_intx_map,
> +};
> +
> +static void iproc_pcie_isr(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct iproc_pcie *pcie;
> +	struct device *dev;
> +	unsigned long status;
> +	u32 bit, virq;
> +
> +	chained_irq_enter(chip, desc);
> +	pcie = irq_desc_get_handler_data(desc);
> +	dev = pcie->dev;
> +
> +	/* go through INTx A, B, C, D until all interrupts are handled */
> +	while ((status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR) &
> +		SYS_RC_INTX_MASK) != 0) {
> +		for_each_set_bit(bit, &status, PCI_NUM_INTX) {
> +			virq = irq_find_mapping(pcie->irq_domain, bit + 1);
> +			if (virq)
> +				generic_handle_irq(virq);
> +			else
> +				dev_err(dev, "unexpected INTx%u\n", bit);
> +		}
> +	}
> +

Are these level or edge interrupts ? although I see DT says: 
IRQ_TYPE_NONE
do you not need to clear interrupt status bits in IPROC_PCIE_INTX_CSR ?

> +	chained_irq_exit(chip, desc);
> +}
> +
> +static int iproc_pcie_intx_enable(struct iproc_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	struct device_node *node = dev->of_node;
> +	int ret;
> +
>  	iproc_pcie_write_reg(pcie, IPROC_PCIE_INTX_EN, SYS_RC_INTX_MASK);
> +
> +	/*
> +	 * BCMA devices do not map INTx the same way as platform devices. All
> +	 * BCMA needs is the above code to enable INTx
> +	 */
> +	if (pcie->irq <= 0)
> +		return 0;
> +
> +	/* set IRQ handler */
> +	irq_set_chained_handler_and_data(pcie->irq, iproc_pcie_isr, pcie);
> +
> +	/* add IRQ domain for INTx */
> +	pcie->irq_domain = irq_domain_add_linear(node, PCI_NUM_INTX + 1,
> +						 &intx_domain_ops, pcie);
> +	if (!pcie->irq_domain) {
> +		dev_err(dev, "failed to add INTx IRQ domain\n");
> +		ret = -ENOMEM;
> +		goto err_rm_handler_data;
> +	}
> +
> +	return 0;
> +
> +err_rm_handler_data:
> +	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
> +
> +	return ret;
> +}
> +
> +static void iproc_pcie_intx_disable(struct iproc_pcie *pcie)
> +{
> +	iproc_pcie_write_reg(pcie, IPROC_PCIE_INTX_EN, 0x0);
> +
> +	if (pcie->irq <= 0)
> +		return;
> +
> +	irq_domain_remove(pcie->irq_domain);
> +	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
>  }
> 
>  static inline bool iproc_pcie_ob_is_valid(struct iproc_pcie *pcie,
> @@ -1410,7 +1496,11 @@ int iproc_pcie_setup(struct iproc_pcie *pcie,
> struct list_head *res)
>  		goto err_power_off_phy;
>  	}
> 
> -	iproc_pcie_enable(pcie);
> +	ret = iproc_pcie_intx_enable(pcie);
> +	if (ret) {
> +		dev_err(dev, "failed to enable INTx\n");
> +		goto err_power_off_phy;
> +	}
> 
>  	if (IS_ENABLED(CONFIG_PCI_MSI))
>  		if (iproc_pcie_msi_enable(pcie))
> @@ -1455,6 +1545,7 @@ int iproc_pcie_remove(struct iproc_pcie *pcie)
>  	pci_remove_root_bus(pcie->root_bus);
> 
>  	iproc_pcie_msi_disable(pcie);
> +	iproc_pcie_intx_disable(pcie);
> 
>  	phy_power_off(pcie->phy);
>  	phy_exit(pcie->phy);
> diff --git a/drivers/pci/host/pcie-iproc.h 
> b/drivers/pci/host/pcie-iproc.h
> index 67081cb..cbcaf9d 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -72,6 +72,9 @@ struct iproc_msi;
>   * @ib: inbound mapping related parameters
>   * @ib_map: outbound mapping region related parameters
>   *
> + * @irq: interrupt line wired to the generic GIC for INTx
> + * @irq_domain: IRQ domain for INTx
> + *
>   * @need_msi_steer: indicates additional configuration of the iProc 
> PCIe
>   * controller is required to steer MSI writes to external interrupt 
> controller
>   * @msi: MSI data
> @@ -99,6 +102,9 @@ struct iproc_pcie {
>  	struct iproc_pcie_ib ib;
>  	const struct iproc_pcie_ib_map *ib_map;
> 
> +	int irq;
> +	struct irq_domain *irq_domain;
> +
>  	bool need_msi_steer;
>  	struct iproc_msi *msi;
>  };

Re: [PATCH 2/6] PCI: iproc: Add INTx support with better modeling

[Ray Jui]

On 6/12/2018 1:52 AM, poza@codeaurora.org wrote:
> On 2018-05-30 03:28, Ray Jui wrote:
>> Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
>> modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
>> INTD share the same interrupt line connected to the GIC in the system,
>> while the status of each INTx can be obtained through the INTX CSR
>> register
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> ---
>>  drivers/pci/host/pcie-iproc-platform.c |  2 +
>>  drivers/pci/host/pcie-iproc.c          | 95 
>> +++++++++++++++++++++++++++++++++-
>>  drivers/pci/host/pcie-iproc.h          |  6 +++
>>  3 files changed, 101 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-iproc-platform.c
>> b/drivers/pci/host/pcie-iproc-platform.c
>> index e764a2a..7a51e6c 100644
>> --- a/drivers/pci/host/pcie-iproc-platform.c
>> +++ b/drivers/pci/host/pcie-iproc-platform.c
>> @@ -70,6 +70,8 @@ static int iproc_pcie_pltfm_probe(struct
>> platform_device *pdev)
>>      }
>>      pcie->base_addr = reg.start;
>>
>> +    pcie->irq = platform_get_irq(pdev, 0);
>> +
>>      if (of_property_read_bool(np, "brcm,pcie-ob")) {
>>          u32 val;
>>
>> diff --git a/drivers/pci/host/pcie-iproc.c 
>> b/drivers/pci/host/pcie-iproc.c
>> index 14f374d..0bd2c14 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -14,6 +14,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/irqchip/arm-gic-v3.h>
>> +#include <linux/irqchip/chained_irq.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/of_address.h>
>>  #include <linux/of_pci.h>
>> @@ -264,6 +265,7 @@ enum iproc_pcie_reg {
>>
>>      /* enable INTx */
>>      IPROC_PCIE_INTX_EN,
>> +    IPROC_PCIE_INTX_CSR,
>>
>>      /* outbound address mapping */
>>      IPROC_PCIE_OARR0,
>> @@ -305,6 +307,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = {
>>      [IPROC_PCIE_CFG_ADDR]        = 0x1f8,
>>      [IPROC_PCIE_CFG_DATA]        = 0x1fc,
>>      [IPROC_PCIE_INTX_EN]        = 0x330,
>> +    [IPROC_PCIE_INTX_CSR]        = 0x334,
>>      [IPROC_PCIE_LINK_STATUS]    = 0xf0c,
>>  };
>>
>> @@ -316,6 +319,7 @@ static const u16 iproc_pcie_reg_paxb[] = {
>>      [IPROC_PCIE_CFG_ADDR]        = 0x1f8,
>>      [IPROC_PCIE_CFG_DATA]        = 0x1fc,
>>      [IPROC_PCIE_INTX_EN]        = 0x330,
>> +    [IPROC_PCIE_INTX_CSR]        = 0x334,
>>      [IPROC_PCIE_OARR0]        = 0xd20,
>>      [IPROC_PCIE_OMAP0]        = 0xd40,
>>      [IPROC_PCIE_OARR1]        = 0xd28,
>> @@ -332,6 +336,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = {
>>      [IPROC_PCIE_CFG_ADDR]        = 0x1f8,
>>      [IPROC_PCIE_CFG_DATA]        = 0x1fc,
>>      [IPROC_PCIE_INTX_EN]        = 0x330,
>> +    [IPROC_PCIE_INTX_CSR]        = 0x334,
>>      [IPROC_PCIE_OARR0]        = 0xd20,
>>      [IPROC_PCIE_OMAP0]        = 0xd40,
>>      [IPROC_PCIE_OARR1]        = 0xd28,
>> @@ -782,9 +787,90 @@ static int iproc_pcie_check_link(struct 
>> iproc_pcie *pcie)
>>      return link_is_active ? 0 : -ENODEV;
>>  }
>>
>> -static void iproc_pcie_enable(struct iproc_pcie *pcie)
>> +static int iproc_pcie_intx_map(struct irq_domain *domain, unsigned 
>> int irq,
>> +                   irq_hw_number_t hwirq)
>>  {
>> +    irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
>> +    irq_set_chip_data(irq, domain->host_data);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct irq_domain_ops intx_domain_ops = {
>> +    .map = iproc_pcie_intx_map,
>> +};
>> +
>> +static void iproc_pcie_isr(struct irq_desc *desc)
>> +{
>> +    struct irq_chip *chip = irq_desc_get_chip(desc);
>> +    struct iproc_pcie *pcie;
>> +    struct device *dev;
>> +    unsigned long status;
>> +    u32 bit, virq;
>> +
>> +    chained_irq_enter(chip, desc);
>> +    pcie = irq_desc_get_handler_data(desc);
>> +    dev = pcie->dev;
>> +
>> +    /* go through INTx A, B, C, D until all interrupts are handled */
>> +    while ((status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR) &
>> +        SYS_RC_INTX_MASK) != 0) {
>> +        for_each_set_bit(bit, &status, PCI_NUM_INTX) {
>> +            virq = irq_find_mapping(pcie->irq_domain, bit + 1);
>> +            if (virq)
>> +                generic_handle_irq(virq);
>> +            else
>> +                dev_err(dev, "unexpected INTx%u\n", bit);
>> +        }
>> +    }
>> +
> 
> Are these level or edge interrupts ? although I see DT says: IRQ_TYPE_NONE

DT entries should be fixed to trigger on level HIGH like Florian and I 
discussed on the mailing list yesterday. It looks like you are missing a 
lot of discussions on the mailing list. Could you please help to make 
sure you read all the existing discussions when commenting on a patch?

> do you not need to clear interrupt status bits in IPROC_PCIE_INTX_CSR ?
> 

RO

Re: [PATCH 2/6] PCI: iproc: Add INTx support with better modeling

[Ray Jui]

CgpPbiA2LzEyLzIwMTggMTo1MiBBTSwgcG96YUBjb2RlYXVyb3JhLm9yZyB3cm90ZToKPiBPbiAy
MDE4LTA1LTMwIDAzOjI4LCBSYXkgSnVpIHdyb3RlOgo+PiBBZGQgUENJZSBsZWdhY3kgaW50ZXJy
dXB0IElOVHggc3VwcG9ydCB0byB0aGUgaVByb2MgUENJZSBkcml2ZXIgYnkKPj4gbW9kZWxpbmcg
aXQgd2l0aCBpdHMgb3duIElSUSBkb21haW4uIEFsbCA0IGludGVycnVwdHMgSU5UQSwgSU5UQiwg
SU5UQywKPj4gSU5URCBzaGFyZSB0aGUgc2FtZSBpbnRlcnJ1cHQgbGluZSBjb25uZWN0ZWQgdG8g
dGhlIEdJQyBpbiB0aGUgc3lzdGVtLAo+PiB3aGlsZSB0aGUgc3RhdHVzIG9mIGVhY2ggSU5UeCBj
YW4gYmUgb2J0YWluZWQgdGhyb3VnaCB0aGUgSU5UWCBDU1IKPj4gcmVnaXN0ZXIKPj4KPj4gU2ln
bmVkLW9mZi1ieTogUmF5IEp1aSA8cmF5Lmp1aUBicm9hZGNvbS5jb20+Cj4+IC0tLQo+PiDCoGRy
aXZlcnMvcGNpL2hvc3QvcGNpZS1pcHJvYy1wbGF0Zm9ybS5jIHzCoCAyICsKPj4gwqBkcml2ZXJz
L3BjaS9ob3N0L3BjaWUtaXByb2MuY8KgwqDCoMKgwqDCoMKgwqDCoCB8IDk1IAo+PiArKysrKysr
KysrKysrKysrKysrKysrKysrKysrKysrKystCj4+IMKgZHJpdmVycy9wY2kvaG9zdC9wY2llLWlw
cm9jLmjCoMKgwqDCoMKgwqDCoMKgwqAgfMKgIDYgKysrCj4+IMKgMyBmaWxlcyBjaGFuZ2VkLCAx
MDEgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkKPj4KPj4gZGlmZiAtLWdpdCBhL2RyaXZl
cnMvcGNpL2hvc3QvcGNpZS1pcHJvYy1wbGF0Zm9ybS5jCj4+IGIvZHJpdmVycy9wY2kvaG9zdC9w
Y2llLWlwcm9jLXBsYXRmb3JtLmMKPj4gaW5kZXggZTc2NGEyYS4uN2E1MWU2YyAxMDA2NDQKPj4g
LS0tIGEvZHJpdmVycy9wY2kvaG9zdC9wY2llLWlwcm9jLXBsYXRmb3JtLmMKPj4gKysrIGIvZHJp
dmVycy9wY2kvaG9zdC9wY2llLWlwcm9jLXBsYXRmb3JtLmMKPj4gQEAgLTcwLDYgKzcwLDggQEAg
c3RhdGljIGludCBpcHJvY19wY2llX3BsdGZtX3Byb2JlKHN0cnVjdAo+PiBwbGF0Zm9ybV9kZXZp
Y2UgKnBkZXYpCj4+IMKgwqDCoMKgIH0KPj4gwqDCoMKgwqAgcGNpZS0+YmFzZV9hZGRyID0gcmVn
LnN0YXJ0Owo+Pgo+PiArwqDCoMKgIHBjaWUtPmlycSA9IHBsYXRmb3JtX2dldF9pcnEocGRldiwg
MCk7Cj4+ICsKPj4gwqDCoMKgwqAgaWYgKG9mX3Byb3BlcnR5X3JlYWRfYm9vbChucCwgImJyY20s
cGNpZS1vYiIpKSB7Cj4+IMKgwqDCoMKgwqDCoMKgwqAgdTMyIHZhbDsKPj4KPj4gZGlmZiAtLWdp
dCBhL2RyaXZlcnMvcGNpL2hvc3QvcGNpZS1pcHJvYy5jIAo+PiBiL2RyaXZlcnMvcGNpL2hvc3Qv
cGNpZS1pcHJvYy5jCj4+IGluZGV4IDE0ZjM3NGQuLjBiZDJjMTQgMTAwNjQ0Cj4+IC0tLSBhL2Ry
aXZlcnMvcGNpL2hvc3QvcGNpZS1pcHJvYy5jCj4+ICsrKyBiL2RyaXZlcnMvcGNpL2hvc3QvcGNp
ZS1pcHJvYy5jCj4+IEBAIC0xNCw2ICsxNCw3IEBACj4+IMKgI2luY2x1ZGUgPGxpbnV4L2RlbGF5
Lmg+Cj4+IMKgI2luY2x1ZGUgPGxpbnV4L2ludGVycnVwdC5oPgo+PiDCoCNpbmNsdWRlIDxsaW51
eC9pcnFjaGlwL2FybS1naWMtdjMuaD4KPj4gKyNpbmNsdWRlIDxsaW51eC9pcnFjaGlwL2NoYWlu
ZWRfaXJxLmg+Cj4+IMKgI2luY2x1ZGUgPGxpbnV4L3BsYXRmb3JtX2RldmljZS5oPgo+PiDCoCNp
bmNsdWRlIDxsaW51eC9vZl9hZGRyZXNzLmg+Cj4+IMKgI2luY2x1ZGUgPGxpbnV4L29mX3BjaS5o
Pgo+PiBAQCAtMjY0LDYgKzI2NSw3IEBAIGVudW0gaXByb2NfcGNpZV9yZWcgewo+Pgo+PiDCoMKg
wqDCoCAvKiBlbmFibGUgSU5UeCAqLwo+PiDCoMKgwqDCoCBJUFJPQ19QQ0lFX0lOVFhfRU4sCj4+
ICvCoMKgwqAgSVBST0NfUENJRV9JTlRYX0NTUiwKPj4KPj4gwqDCoMKgwqAgLyogb3V0Ym91bmQg
YWRkcmVzcyBtYXBwaW5nICovCj4+IMKgwqDCoMKgIElQUk9DX1BDSUVfT0FSUjAsCj4+IEBAIC0z
MDUsNiArMzA3LDcgQEAgc3RhdGljIGNvbnN0IHUxNiBpcHJvY19wY2llX3JlZ19wYXhiX2JjbWFb
XSA9IHsKPj4gwqDCoMKgwqAgW0lQUk9DX1BDSUVfQ0ZHX0FERFJdwqDCoMKgwqDCoMKgwqAgPSAw
eDFmOCwKPj4gwqDCoMKgwqAgW0lQUk9DX1BDSUVfQ0ZHX0RBVEFdwqDCoMKgwqDCoMKgwqAgPSAw
eDFmYywKPj4gwqDCoMKgwqAgW0lQUk9DX1BDSUVfSU5UWF9FTl3CoMKgwqDCoMKgwqDCoCA9IDB4
MzMwLAo+PiArwqDCoMKgIFtJUFJPQ19QQ0lFX0lOVFhfQ1NSXcKgwqDCoMKgwqDCoMKgID0gMHgz
MzQsCj4+IMKgwqDCoMKgIFtJUFJPQ19QQ0lFX0xJTktfU1RBVFVTXcKgwqDCoCA9IDB4ZjBjLAo+
PiDCoH07Cj4+Cj4+IEBAIC0zMTYsNiArMzE5LDcgQEAgc3RhdGljIGNvbnN0IHUxNiBpcHJvY19w
Y2llX3JlZ19wYXhiW10gPSB7Cj4+IMKgwqDCoMKgIFtJUFJPQ19QQ0lFX0NGR19BRERSXcKgwqDC
oMKgwqDCoMKgID0gMHgxZjgsCj4+IMKgwqDCoMKgIFtJUFJPQ19QQ0lFX0NGR19EQVRBXcKgwqDC
oMKgwqDCoMKgID0gMHgxZmMsCj4+IMKgwqDCoMKgIFtJUFJPQ19QQ0lFX0lOVFhfRU5dwqDCoMKg
wqDCoMKgwqAgPSAweDMzMCwKPj4gK8KgwqDCoCBbSVBST0NfUENJRV9JTlRYX0NTUl3CoMKgwqDC
oMKgwqDCoCA9IDB4MzM0LAo+PiDCoMKgwqDCoCBbSVBST0NfUENJRV9PQVJSMF3CoMKgwqDCoMKg
wqDCoCA9IDB4ZDIwLAo+PiDCoMKgwqDCoCBbSVBST0NfUENJRV9PTUFQMF3CoMKgwqDCoMKgwqDC
oCA9IDB4ZDQwLAo+PiDCoMKgwqDCoCBbSVBST0NfUENJRV9PQVJSMV3CoMKgwqDCoMKgwqDCoCA9
IDB4ZDI4LAo+PiBAQCAtMzMyLDYgKzMzNiw3IEBAIHN0YXRpYyBjb25zdCB1MTYgaXByb2NfcGNp
ZV9yZWdfcGF4Yl92MltdID0gewo+PiDCoMKgwqDCoCBbSVBST0NfUENJRV9DRkdfQUREUl3CoMKg
wqDCoMKgwqDCoCA9IDB4MWY4LAo+PiDCoMKgwqDCoCBbSVBST0NfUENJRV9DRkdfREFUQV3CoMKg
wqDCoMKgwqDCoCA9IDB4MWZjLAo+PiDCoMKgwqDCoCBbSVBST0NfUENJRV9JTlRYX0VOXcKgwqDC
oMKgwqDCoMKgID0gMHgzMzAsCj4+ICvCoMKgwqAgW0lQUk9DX1BDSUVfSU5UWF9DU1JdwqDCoMKg
wqDCoMKgwqAgPSAweDMzNCwKPj4gwqDCoMKgwqAgW0lQUk9DX1BDSUVfT0FSUjBdwqDCoMKgwqDC
oMKgwqAgPSAweGQyMCwKPj4gwqDCoMKgwqAgW0lQUk9DX1BDSUVfT01BUDBdwqDCoMKgwqDCoMKg
wqAgPSAweGQ0MCwKPj4gwqDCoMKgwqAgW0lQUk9DX1BDSUVfT0FSUjFdwqDCoMKgwqDCoMKgwqAg
PSAweGQyOCwKPj4gQEAgLTc4Miw5ICs3ODcsOTAgQEAgc3RhdGljIGludCBpcHJvY19wY2llX2No
ZWNrX2xpbmsoc3RydWN0IAo+PiBpcHJvY19wY2llICpwY2llKQo+PiDCoMKgwqDCoCByZXR1cm4g
bGlua19pc19hY3RpdmUgPyAwIDogLUVOT0RFVjsKPj4gwqB9Cj4+Cj4+IC1zdGF0aWMgdm9pZCBp
cHJvY19wY2llX2VuYWJsZShzdHJ1Y3QgaXByb2NfcGNpZSAqcGNpZSkKPj4gK3N0YXRpYyBpbnQg
aXByb2NfcGNpZV9pbnR4X21hcChzdHJ1Y3QgaXJxX2RvbWFpbiAqZG9tYWluLCB1bnNpZ25lZCAK
Pj4gaW50IGlycSwKPj4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCBpcnFf
aHdfbnVtYmVyX3QgaHdpcnEpCj4+IMKgewo+PiArwqDCoMKgIGlycV9zZXRfY2hpcF9hbmRfaGFu
ZGxlcihpcnEsICZkdW1teV9pcnFfY2hpcCwgaGFuZGxlX3NpbXBsZV9pcnEpOwo+PiArwqDCoMKg
IGlycV9zZXRfY2hpcF9kYXRhKGlycSwgZG9tYWluLT5ob3N0X2RhdGEpOwo+PiArCj4+ICvCoMKg
wqAgcmV0dXJuIDA7Cj4+ICt9Cj4+ICsKPj4gK3N0YXRpYyBjb25zdCBzdHJ1Y3QgaXJxX2RvbWFp
bl9vcHMgaW50eF9kb21haW5fb3BzID0gewo+PiArwqDCoMKgIC5tYXAgPSBpcHJvY19wY2llX2lu
dHhfbWFwLAo+PiArfTsKPj4gKwo+PiArc3RhdGljIHZvaWQgaXByb2NfcGNpZV9pc3Ioc3RydWN0
IGlycV9kZXNjICpkZXNjKQo+PiArewo+PiArwqDCoMKgIHN0cnVjdCBpcnFfY2hpcCAqY2hpcCA9
IGlycV9kZXNjX2dldF9jaGlwKGRlc2MpOwo+PiArwqDCoMKgIHN0cnVjdCBpcHJvY19wY2llICpw
Y2llOwo+PiArwqDCoMKgIHN0cnVjdCBkZXZpY2UgKmRldjsKPj4gK8KgwqDCoCB1bnNpZ25lZCBs
b25nIHN0YXR1czsKPj4gK8KgwqDCoCB1MzIgYml0LCB2aXJxOwo+PiArCj4+ICvCoMKgwqAgY2hh
aW5lZF9pcnFfZW50ZXIoY2hpcCwgZGVzYyk7Cj4+ICvCoMKgwqAgcGNpZSA9IGlycV9kZXNjX2dl
dF9oYW5kbGVyX2RhdGEoZGVzYyk7Cj4+ICvCoMKgwqAgZGV2ID0gcGNpZS0+ZGV2Owo+PiArCj4+
ICvCoMKgwqAgLyogZ28gdGhyb3VnaCBJTlR4IEEsIEIsIEMsIEQgdW50aWwgYWxsIGludGVycnVw
dHMgYXJlIGhhbmRsZWQgKi8KPj4gK8KgwqDCoCB3aGlsZSAoKHN0YXR1cyA9IGlwcm9jX3BjaWVf
cmVhZF9yZWcocGNpZSwgSVBST0NfUENJRV9JTlRYX0NTUikgJgo+PiArwqDCoMKgwqDCoMKgwqAg
U1lTX1JDX0lOVFhfTUFTSykgIT0gMCkgewo+PiArwqDCoMKgwqDCoMKgwqAgZm9yX2VhY2hfc2V0
X2JpdChiaXQsICZzdGF0dXMsIFBDSV9OVU1fSU5UWCkgewo+PiArwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoCB2aXJxID0gaXJxX2ZpbmRfbWFwcGluZyhwY2llLT5pcnFfZG9tYWluLCBiaXQgKyAxKTsK
Pj4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqAgaWYgKHZpcnEpCj4+ICvCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqAgZ2VuZXJpY19oYW5kbGVfaXJxKHZpcnEpOwo+PiArwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoCBlbHNlCj4+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAgZGV2X2Vy
cihkZXYsICJ1bmV4cGVjdGVkIElOVHgldVxuIiwgYml0KTsKPj4gK8KgwqDCoMKgwqDCoMKgIH0K
Pj4gK8KgwqDCoCB9Cj4+ICsKPiAKPiBBcmUgdGhlc2UgbGV2ZWwgb3IgZWRnZSBpbnRlcnJ1cHRz
ID8gYWx0aG91Z2ggSSBzZWUgRFQgc2F5czogSVJRX1RZUEVfTk9ORQoKRFQgZW50cmllcyBzaG91
bGQgYmUgZml4ZWQgdG8gdHJpZ2dlciBvbiBsZXZlbCBISUdIIGxpa2UgRmxvcmlhbiBhbmQgSSAK
ZGlzY3Vzc2VkIG9uIHRoZSBtYWlsaW5nIGxpc3QgeWVzdGVyZGF5LiBJdCBsb29rcyBsaWtlIHlv
dSBhcmUgbWlzc2luZyBhIApsb3Qgb2YgZGlzY3Vzc2lvbnMgb24gdGhlIG1haWxpbmcgbGlzdC4g
Q291bGQgeW91IHBsZWFzZSBoZWxwIHRvIG1ha2UgCnN1cmUgeW91IHJlYWQgYWxsIHRoZSBleGlz
dGluZyBkaXNjdXNzaW9ucyB3aGVuIGNvbW1lbnRpbmcgb24gYSBwYXRjaD8KCj4gZG8geW91IG5v
dCBuZWVkIHRvIGNsZWFyIGludGVycnVwdCBzdGF0dXMgYml0cyBpbiBJUFJPQ19QQ0lFX0lOVFhf
Q1NSID8KPiAKClJPCgoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX18KbGludXgtYXJtLWtlcm5lbCBtYWlsaW5nIGxpc3QKbGludXgtYXJtLWtlcm5lbEBsaXN0
cy5pbmZyYWRlYWQub3JnCmh0dHA6Ly9saXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGlu
Zm8vbGludXgtYXJtLWtlcm5lbAo=

[PATCH 2/6] PCI: iproc: Add INTx support with better modeling

[Ray Jui]

On 6/12/2018 1:52 AM, poza at codeaurora.org wrote:
> On 2018-05-30 03:28, Ray Jui wrote:
>> Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
>> modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
>> INTD share the same interrupt line connected to the GIC in the system,
>> while the status of each INTx can be obtained through the INTX CSR
>> register
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> ---
>> ?drivers/pci/host/pcie-iproc-platform.c |? 2 +
>> ?drivers/pci/host/pcie-iproc.c????????? | 95 
>> +++++++++++++++++++++++++++++++++-
>> ?drivers/pci/host/pcie-iproc.h????????? |? 6 +++
>> ?3 files changed, 101 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-iproc-platform.c
>> b/drivers/pci/host/pcie-iproc-platform.c
>> index e764a2a..7a51e6c 100644
>> --- a/drivers/pci/host/pcie-iproc-platform.c
>> +++ b/drivers/pci/host/pcie-iproc-platform.c
>> @@ -70,6 +70,8 @@ static int iproc_pcie_pltfm_probe(struct
>> platform_device *pdev)
>> ???? }
>> ???? pcie->base_addr = reg.start;
>>
>> +??? pcie->irq = platform_get_irq(pdev, 0);
>> +
>> ???? if (of_property_read_bool(np, "brcm,pcie-ob")) {
>> ???????? u32 val;
>>
>> diff --git a/drivers/pci/host/pcie-iproc.c 
>> b/drivers/pci/host/pcie-iproc.c
>> index 14f374d..0bd2c14 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -14,6 +14,7 @@
>> ?#include <linux/delay.h>
>> ?#include <linux/interrupt.h>
>> ?#include <linux/irqchip/arm-gic-v3.h>
>> +#include <linux/irqchip/chained_irq.h>
>> ?#include <linux/platform_device.h>
>> ?#include <linux/of_address.h>
>> ?#include <linux/of_pci.h>
>> @@ -264,6 +265,7 @@ enum iproc_pcie_reg {
>>
>> ???? /* enable INTx */
>> ???? IPROC_PCIE_INTX_EN,
>> +??? IPROC_PCIE_INTX_CSR,
>>
>> ???? /* outbound address mapping */
>> ???? IPROC_PCIE_OARR0,
>> @@ -305,6 +307,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = {
>> ???? [IPROC_PCIE_CFG_ADDR]??????? = 0x1f8,
>> ???? [IPROC_PCIE_CFG_DATA]??????? = 0x1fc,
>> ???? [IPROC_PCIE_INTX_EN]??????? = 0x330,
>> +??? [IPROC_PCIE_INTX_CSR]??????? = 0x334,
>> ???? [IPROC_PCIE_LINK_STATUS]??? = 0xf0c,
>> ?};
>>
>> @@ -316,6 +319,7 @@ static const u16 iproc_pcie_reg_paxb[] = {
>> ???? [IPROC_PCIE_CFG_ADDR]??????? = 0x1f8,
>> ???? [IPROC_PCIE_CFG_DATA]??????? = 0x1fc,
>> ???? [IPROC_PCIE_INTX_EN]??????? = 0x330,
>> +??? [IPROC_PCIE_INTX_CSR]??????? = 0x334,
>> ???? [IPROC_PCIE_OARR0]??????? = 0xd20,
>> ???? [IPROC_PCIE_OMAP0]??????? = 0xd40,
>> ???? [IPROC_PCIE_OARR1]??????? = 0xd28,
>> @@ -332,6 +336,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = {
>> ???? [IPROC_PCIE_CFG_ADDR]??????? = 0x1f8,
>> ???? [IPROC_PCIE_CFG_DATA]??????? = 0x1fc,
>> ???? [IPROC_PCIE_INTX_EN]??????? = 0x330,
>> +??? [IPROC_PCIE_INTX_CSR]??????? = 0x334,
>> ???? [IPROC_PCIE_OARR0]??????? = 0xd20,
>> ???? [IPROC_PCIE_OMAP0]??????? = 0xd40,
>> ???? [IPROC_PCIE_OARR1]??????? = 0xd28,
>> @@ -782,9 +787,90 @@ static int iproc_pcie_check_link(struct 
>> iproc_pcie *pcie)
>> ???? return link_is_active ? 0 : -ENODEV;
>> ?}
>>
>> -static void iproc_pcie_enable(struct iproc_pcie *pcie)
>> +static int iproc_pcie_intx_map(struct irq_domain *domain, unsigned 
>> int irq,
>> +?????????????????? irq_hw_number_t hwirq)
>> ?{
>> +??? irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
>> +??? irq_set_chip_data(irq, domain->host_data);
>> +
>> +??? return 0;
>> +}
>> +
>> +static const struct irq_domain_ops intx_domain_ops = {
>> +??? .map = iproc_pcie_intx_map,
>> +};
>> +
>> +static void iproc_pcie_isr(struct irq_desc *desc)
>> +{
>> +??? struct irq_chip *chip = irq_desc_get_chip(desc);
>> +??? struct iproc_pcie *pcie;
>> +??? struct device *dev;
>> +??? unsigned long status;
>> +??? u32 bit, virq;
>> +
>> +??? chained_irq_enter(chip, desc);
>> +??? pcie = irq_desc_get_handler_data(desc);
>> +??? dev = pcie->dev;
>> +
>> +??? /* go through INTx A, B, C, D until all interrupts are handled */
>> +??? while ((status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR) &
>> +??????? SYS_RC_INTX_MASK) != 0) {
>> +??????? for_each_set_bit(bit, &status, PCI_NUM_INTX) {
>> +??????????? virq = irq_find_mapping(pcie->irq_domain, bit + 1);
>> +??????????? if (virq)
>> +??????????????? generic_handle_irq(virq);
>> +??????????? else
>> +??????????????? dev_err(dev, "unexpected INTx%u\n", bit);
>> +??????? }
>> +??? }
>> +
> 
> Are these level or edge interrupts ? although I see DT says: IRQ_TYPE_NONE

DT entries should be fixed to trigger on level HIGH like Florian and I 
discussed on the mailing list yesterday. It looks like you are missing a 
lot of discussions on the mailing list. Could you please help to make 
sure you read all the existing discussions when commenting on a patch?

> do you not need to clear interrupt status bits in IPROC_PCIE_INTX_CSR ?
> 

RO

Re: [PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Lorenzo Pieralisi]

On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote:
> +Arnd
> 
> On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> > Update the iProc PCIe binding document for better modeling of the legacy
> > interrupt (INTx) support
> >
> > Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> > ---
> >  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
> >  1 file changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > index b8e48b4..7ea24dc 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > @@ -13,9 +13,6 @@ controller, used in Stingray
> >    PAXB-based root complex is used for external endpoint devices. PAXC-based
> >  root complex is connected to emulated endpoint devices internal to the ASIC
> >  - 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>
> > @@ -41,6 +38,16 @@ Required:
> >  - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
> >  address used by the iProc PCIe core (not the PCIe address)
> >
> > +Legacy interrupt (INTx) support (optional):
> > +
> > +Note INTx is for PAXB only.
> > +
> > +- interrupt-controller: claims itself as an interrupt controller for INTx
> > +- #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
> > +- interrupts: interrupt line wired to the generic GIC for INTx support
> > +
> >  MSI support (optional):
> >
> >  For older platforms without MSI integrated in the GIC, iProc PCIe core provides
> > @@ -77,9 +84,14 @@ Example:
> >                 compatible = "brcm,iproc-pcie";
> >                 reg = <0x18012000 0x1000>;
> >
> > +               interrupt-controller;
> >                 #interrupt-cells = <1>;
> > -               interrupt-map-mask = <0 0 0 0>;
> > -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> > +               interrupt-map-mask = <0 0 0 7>;
> > +               interrupt-map = <0 0 0 1 &pcie0 1>,
> 
> Are you sure this works? The irq parsing code will ignore
> interrupt-map if interrupt-controller is found. In other words, you
> should have one or the other, but not both.
> 
> Maybe it happens to work because "pcie0" is this node and your irq
> numbers are the same.
> 
> Arnd, any thoughts on this?

To start with, I think the destination IRQ number is wrong, what the
mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB,
#INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo)
interrupt controller.

I really want to clean this up since currently there are different
DT bindings defining this in different ways which resulted in
non-consistent kernel code.

AFAICS, the Aardvark PCIe controller bindings define the mapping
as I expect:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4

but I would like to get Rob and Arnd viewpoint on this so that
we can close this topic once for all.


Cheers,
Lorenzo

> 
> > +                               <0 0 0 2 &pcie0 2>,
> > +                               <0 0 0 3 &pcie0 3>,
> > +                               <0 0 0 4 &pcie0 4>;
> > +               interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
> >
> >                 linux,pci-domain = <0>;
> >
> > @@ -115,9 +127,14 @@ Example:
> >                 compatible = "brcm,iproc-pcie";
> >                 reg = <0x18013000 0x1000>;
> >
> > +               interrupt-controller;
> >                 #interrupt-cells = <1>;
> > -               interrupt-map-mask = <0 0 0 0>;
> > -               interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
> > +               interrupt-map-mask = <0 0 0 7>;
> > +               interrupt-map = <0 0 0 1 &pcie1 1>,
> > +                               <0 0 0 2 &pcie1 2>,
> > +                               <0 0 0 3 &pcie1 3>,
> > +                               <0 0 0 4 &pcie1 4>;
> > +               interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
> >
> >                 linux,pci-domain = <1>;
> >
> > --
> > 2.1.4
> >

Re: [PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Lorenzo Pieralisi]

On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote:
> +Arnd
> 
> On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> > Update the iProc PCIe binding document for better modeling of the legacy
> > interrupt (INTx) support
> >
> > Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> > ---
> >  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
> >  1 file changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > index b8e48b4..7ea24dc 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > @@ -13,9 +13,6 @@ controller, used in Stingray
> >    PAXB-based root complex is used for external endpoint devices. PAXC-based
> >  root complex is connected to emulated endpoint devices internal to the ASIC
> >  - 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>
> > @@ -41,6 +38,16 @@ Required:
> >  - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
> >  address used by the iProc PCIe core (not the PCIe address)
> >
> > +Legacy interrupt (INTx) support (optional):
> > +
> > +Note INTx is for PAXB only.
> > +
> > +- interrupt-controller: claims itself as an interrupt controller for INTx
> > +- #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
> > +- interrupts: interrupt line wired to the generic GIC for INTx support
> > +
> >  MSI support (optional):
> >
> >  For older platforms without MSI integrated in the GIC, iProc PCIe core provides
> > @@ -77,9 +84,14 @@ Example:
> >                 compatible = "brcm,iproc-pcie";
> >                 reg = <0x18012000 0x1000>;
> >
> > +               interrupt-controller;
> >                 #interrupt-cells = <1>;
> > -               interrupt-map-mask = <0 0 0 0>;
> > -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> > +               interrupt-map-mask = <0 0 0 7>;
> > +               interrupt-map = <0 0 0 1 &pcie0 1>,
> 
> Are you sure this works? The irq parsing code will ignore
> interrupt-map if interrupt-controller is found. In other words, you
> should have one or the other, but not both.
> 
> Maybe it happens to work because "pcie0" is this node and your irq
> numbers are the same.
> 
> Arnd, any thoughts on this?

To start with, I think the destination IRQ number is wrong, what the
mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB,
#INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo)
interrupt controller.

I really want to clean this up since currently there are different
DT bindings defining this in different ways which resulted in
non-consistent kernel code.

AFAICS, the Aardvark PCIe controller bindings define the mapping
as I expect:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4

but I would like to get Rob and Arnd viewpoint on this so that
we can close this topic once for all.


Cheers,
Lorenzo

> 
> > +                               <0 0 0 2 &pcie0 2>,
> > +                               <0 0 0 3 &pcie0 3>,
> > +                               <0 0 0 4 &pcie0 4>;
> > +               interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
> >
> >                 linux,pci-domain = <0>;
> >
> > @@ -115,9 +127,14 @@ Example:
> >                 compatible = "brcm,iproc-pcie";
> >                 reg = <0x18013000 0x1000>;
> >
> > +               interrupt-controller;
> >                 #interrupt-cells = <1>;
> > -               interrupt-map-mask = <0 0 0 0>;
> > -               interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
> > +               interrupt-map-mask = <0 0 0 7>;
> > +               interrupt-map = <0 0 0 1 &pcie1 1>,
> > +                               <0 0 0 2 &pcie1 2>,
> > +                               <0 0 0 3 &pcie1 3>,
> > +                               <0 0 0 4 &pcie1 4>;
> > +               interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
> >
> >                 linux,pci-domain = <1>;
> >
> > --
> > 2.1.4
> >

Re: [PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Lorenzo Pieralisi]

On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote:
> +Arnd
> 
> On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> > Update the iProc PCIe binding document for better modeling of the legacy
> > interrupt (INTx) support
> >
> > Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> > ---
> >  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
> >  1 file changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > index b8e48b4..7ea24dc 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > @@ -13,9 +13,6 @@ controller, used in Stingray
> >    PAXB-based root complex is used for external endpoint devices. PAXC-based
> >  root complex is connected to emulated endpoint devices internal to the ASIC
> >  - 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>
> > @@ -41,6 +38,16 @@ Required:
> >  - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
> >  address used by the iProc PCIe core (not the PCIe address)
> >
> > +Legacy interrupt (INTx) support (optional):
> > +
> > +Note INTx is for PAXB only.
> > +
> > +- interrupt-controller: claims itself as an interrupt controller for INTx
> > +- #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
> > +- interrupts: interrupt line wired to the generic GIC for INTx support
> > +
> >  MSI support (optional):
> >
> >  For older platforms without MSI integrated in the GIC, iProc PCIe core provides
> > @@ -77,9 +84,14 @@ Example:
> >                 compatible = "brcm,iproc-pcie";
> >                 reg = <0x18012000 0x1000>;
> >
> > +               interrupt-controller;
> >                 #interrupt-cells = <1>;
> > -               interrupt-map-mask = <0 0 0 0>;
> > -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> > +               interrupt-map-mask = <0 0 0 7>;
> > +               interrupt-map = <0 0 0 1 &pcie0 1>,
> 
> Are you sure this works? The irq parsing code will ignore
> interrupt-map if interrupt-controller is found. In other words, you
> should have one or the other, but not both.
> 
> Maybe it happens to work because "pcie0" is this node and your irq
> numbers are the same.
> 
> Arnd, any thoughts on this?

To start with, I think the destination IRQ number is wrong, what the
mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB,
#INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo)
interrupt controller.

I really want to clean this up since currently there are different
DT bindings defining this in different ways which resulted in
non-consistent kernel code.

AFAICS, the Aardvark PCIe controller bindings define the mapping
as I expect:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4

but I would like to get Rob and Arnd viewpoint on this so that
we can close this topic once for all.


Cheers,
Lorenzo

> 
> > +                               <0 0 0 2 &pcie0 2>,
> > +                               <0 0 0 3 &pcie0 3>,
> > +                               <0 0 0 4 &pcie0 4>;
> > +               interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
> >
> >                 linux,pci-domain = <0>;
> >
> > @@ -115,9 +127,14 @@ Example:
> >                 compatible = "brcm,iproc-pcie";
> >                 reg = <0x18013000 0x1000>;
> >
> > +               interrupt-controller;
> >                 #interrupt-cells = <1>;
> > -               interrupt-map-mask = <0 0 0 0>;
> > -               interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
> > +               interrupt-map-mask = <0 0 0 7>;
> > +               interrupt-map = <0 0 0 1 &pcie1 1>,
> > +                               <0 0 0 2 &pcie1 2>,
> > +                               <0 0 0 3 &pcie1 3>,
> > +                               <0 0 0 4 &pcie1 4>;
> > +               interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
> >
> >                 linux,pci-domain = <1>;
> >
> > --
> > 2.1.4
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Lorenzo Pieralisi]

On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote:
> +Arnd
> 
> On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> > Update the iProc PCIe binding document for better modeling of the legacy
> > interrupt (INTx) support
> >
> > Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> > ---
> >  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
> >  1 file changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > index b8e48b4..7ea24dc 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > @@ -13,9 +13,6 @@ controller, used in Stingray
> >    PAXB-based root complex is used for external endpoint devices. PAXC-based
> >  root complex is connected to emulated endpoint devices internal to the ASIC
> >  - 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>
> > @@ -41,6 +38,16 @@ Required:
> >  - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
> >  address used by the iProc PCIe core (not the PCIe address)
> >
> > +Legacy interrupt (INTx) support (optional):
> > +
> > +Note INTx is for PAXB only.
> > +
> > +- interrupt-controller: claims itself as an interrupt controller for INTx
> > +- #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
> > +- interrupts: interrupt line wired to the generic GIC for INTx support
> > +
> >  MSI support (optional):
> >
> >  For older platforms without MSI integrated in the GIC, iProc PCIe core provides
> > @@ -77,9 +84,14 @@ Example:
> >                 compatible = "brcm,iproc-pcie";
> >                 reg = <0x18012000 0x1000>;
> >
> > +               interrupt-controller;
> >                 #interrupt-cells = <1>;
> > -               interrupt-map-mask = <0 0 0 0>;
> > -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> > +               interrupt-map-mask = <0 0 0 7>;
> > +               interrupt-map = <0 0 0 1 &pcie0 1>,
> 
> Are you sure this works? The irq parsing code will ignore
> interrupt-map if interrupt-controller is found. In other words, you
> should have one or the other, but not both.
> 
> Maybe it happens to work because "pcie0" is this node and your irq
> numbers are the same.
> 
> Arnd, any thoughts on this?

To start with, I think the destination IRQ number is wrong, what the
mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB,
#INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo)
interrupt controller.

I really want to clean this up since currently there are different
DT bindings defining this in different ways which resulted in
non-consistent kernel code.

AFAICS, the Aardvark PCIe controller bindings define the mapping
as I expect:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4

but I would like to get Rob and Arnd viewpoint on this so that
we can close this topic once for all.


Cheers,
Lorenzo

> 
> > +                               <0 0 0 2 &pcie0 2>,
> > +                               <0 0 0 3 &pcie0 3>,
> > +                               <0 0 0 4 &pcie0 4>;
> > +               interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
> >
> >                 linux,pci-domain = <0>;
> >
> > @@ -115,9 +127,14 @@ Example:
> >                 compatible = "brcm,iproc-pcie";
> >                 reg = <0x18013000 0x1000>;
> >
> > +               interrupt-controller;
> >                 #interrupt-cells = <1>;
> > -               interrupt-map-mask = <0 0 0 0>;
> > -               interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
> > +               interrupt-map-mask = <0 0 0 7>;
> > +               interrupt-map = <0 0 0 1 &pcie1 1>,
> > +                               <0 0 0 2 &pcie1 2>,
> > +                               <0 0 0 3 &pcie1 3>,
> > +                               <0 0 0 4 &pcie1 4>;
> > +               interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
> >
> >                 linux,pci-domain = <1>;
> >
> > --
> > 2.1.4
> >

Re: [PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Arnd Bergmann]

On Tue, Sep 18, 2018 at 3:42 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote:
> > +Arnd
> >
> > On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> > > Update the iProc PCIe binding document for better modeling of the legacy
> > > interrupt (INTx) support
> > >
> > > Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> > > ---
> > >  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
> > >  1 file changed, 24 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > index b8e48b4..7ea24dc 100644
> > > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > @@ -13,9 +13,6 @@ controller, used in Stingray
> > >    PAXB-based root complex is used for external endpoint devices. PAXC-based
> > >  root complex is connected to emulated endpoint devices internal to the ASIC
> > >  - 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>
> > > @@ -41,6 +38,16 @@ Required:
> > >  - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
> > >  address used by the iProc PCIe core (not the PCIe address)
> > >
> > > +Legacy interrupt (INTx) support (optional):
> > > +
> > > +Note INTx is for PAXB only.
> > > +
> > > +- interrupt-controller: claims itself as an interrupt controller for INTx
> > > +- #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
> > > +- interrupts: interrupt line wired to the generic GIC for INTx support
> > > +
> > >  MSI support (optional):
> > >
> > >  For older platforms without MSI integrated in the GIC, iProc PCIe core provides
> > > @@ -77,9 +84,14 @@ Example:
> > >                 compatible = "brcm,iproc-pcie";
> > >                 reg = <0x18012000 0x1000>;
> > >
> > > +               interrupt-controller;
> > >                 #interrupt-cells = <1>;
> > > -               interrupt-map-mask = <0 0 0 0>;
> > > -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> > > +               interrupt-map-mask = <0 0 0 7>;
> > > +               interrupt-map = <0 0 0 1 &pcie0 1>,
> >
> > Are you sure this works? The irq parsing code will ignore
> > interrupt-map if interrupt-controller is found. In other words, you
> > should have one or the other, but not both.
> >
> > Maybe it happens to work because "pcie0" is this node and your irq
> > numbers are the same.
> >
> > Arnd, any thoughts on this?
>
> To start with, I think the destination IRQ number is wrong, what the
> mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB,
> #INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo)
> interrupt controller.
>
> I really want to clean this up since currently there are different
> DT bindings defining this in different ways which resulted in
> non-consistent kernel code.
>
> AFAICS, the Aardvark PCIe controller bindings define the mapping
> as I expect:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4
>
> but I would like to get Rob and Arnd viewpoint on this so that
> we can close this topic once for all.

It seems ambiguous at best, as Rob suggested it may only
work by accident. Since there is only one upstream interrupt,
could we simply list <GIC_SPI 100 IRQ_TYPE_NONE> as
the destination for any IntX?

        Arnd

[PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Arnd Bergmann]

On Tue, Sep 18, 2018 at 3:42 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote:
> > +Arnd
> >
> > On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> > > Update the iProc PCIe binding document for better modeling of the legacy
> > > interrupt (INTx) support
> > >
> > > Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> > > ---
> > >  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
> > >  1 file changed, 24 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > index b8e48b4..7ea24dc 100644
> > > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > @@ -13,9 +13,6 @@ controller, used in Stingray
> > >    PAXB-based root complex is used for external endpoint devices. PAXC-based
> > >  root complex is connected to emulated endpoint devices internal to the ASIC
> > >  - 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>
> > > @@ -41,6 +38,16 @@ Required:
> > >  - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
> > >  address used by the iProc PCIe core (not the PCIe address)
> > >
> > > +Legacy interrupt (INTx) support (optional):
> > > +
> > > +Note INTx is for PAXB only.
> > > +
> > > +- interrupt-controller: claims itself as an interrupt controller for INTx
> > > +- #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
> > > +- interrupts: interrupt line wired to the generic GIC for INTx support
> > > +
> > >  MSI support (optional):
> > >
> > >  For older platforms without MSI integrated in the GIC, iProc PCIe core provides
> > > @@ -77,9 +84,14 @@ Example:
> > >                 compatible = "brcm,iproc-pcie";
> > >                 reg = <0x18012000 0x1000>;
> > >
> > > +               interrupt-controller;
> > >                 #interrupt-cells = <1>;
> > > -               interrupt-map-mask = <0 0 0 0>;
> > > -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> > > +               interrupt-map-mask = <0 0 0 7>;
> > > +               interrupt-map = <0 0 0 1 &pcie0 1>,
> >
> > Are you sure this works? The irq parsing code will ignore
> > interrupt-map if interrupt-controller is found. In other words, you
> > should have one or the other, but not both.
> >
> > Maybe it happens to work because "pcie0" is this node and your irq
> > numbers are the same.
> >
> > Arnd, any thoughts on this?
>
> To start with, I think the destination IRQ number is wrong, what the
> mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB,
> #INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo)
> interrupt controller.
>
> I really want to clean this up since currently there are different
> DT bindings defining this in different ways which resulted in
> non-consistent kernel code.
>
> AFAICS, the Aardvark PCIe controller bindings define the mapping
> as I expect:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4
>
> but I would like to get Rob and Arnd viewpoint on this so that
> we can close this topic once for all.

It seems ambiguous at best, as Rob suggested it may only
work by accident. Since there is only one upstream interrupt,
could we simply list <GIC_SPI 100 IRQ_TYPE_NONE> as
the destination for any IntX?

        Arnd

Re: [PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Lorenzo Pieralisi]

On Mon, Sep 24, 2018 at 10:53:13PM +0200, Arnd Bergmann wrote:
> On Tue, Sep 18, 2018 at 3:42 PM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote:
> > > +Arnd
> > >
> > > On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> > > > Update the iProc PCIe binding document for better modeling of the legacy
> > > > interrupt (INTx) support
> > > >
> > > > Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> > > > ---
> > > >  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
> > > >  1 file changed, 24 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > > index b8e48b4..7ea24dc 100644
> > > > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > > @@ -13,9 +13,6 @@ controller, used in Stingray
> > > >    PAXB-based root complex is used for external endpoint devices. PAXC-based
> > > >  root complex is connected to emulated endpoint devices internal to the ASIC
> > > >  - 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>
> > > > @@ -41,6 +38,16 @@ Required:
> > > >  - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
> > > >  address used by the iProc PCIe core (not the PCIe address)
> > > >
> > > > +Legacy interrupt (INTx) support (optional):
> > > > +
> > > > +Note INTx is for PAXB only.
> > > > +
> > > > +- interrupt-controller: claims itself as an interrupt controller for INTx
> > > > +- #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
> > > > +- interrupts: interrupt line wired to the generic GIC for INTx support
> > > > +
> > > >  MSI support (optional):
> > > >
> > > >  For older platforms without MSI integrated in the GIC, iProc PCIe core provides
> > > > @@ -77,9 +84,14 @@ Example:
> > > >                 compatible = "brcm,iproc-pcie";
> > > >                 reg = <0x18012000 0x1000>;
> > > >
> > > > +               interrupt-controller;
> > > >                 #interrupt-cells = <1>;
> > > > -               interrupt-map-mask = <0 0 0 0>;
> > > > -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> > > > +               interrupt-map-mask = <0 0 0 7>;
> > > > +               interrupt-map = <0 0 0 1 &pcie0 1>,
> > >
> > > Are you sure this works? The irq parsing code will ignore
> > > interrupt-map if interrupt-controller is found. In other words, you
> > > should have one or the other, but not both.
> > >
> > > Maybe it happens to work because "pcie0" is this node and your irq
> > > numbers are the same.
> > >
> > > Arnd, any thoughts on this?
> >
> > To start with, I think the destination IRQ number is wrong, what the
> > mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB,
> > #INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo)
> > interrupt controller.
> >
> > I really want to clean this up since currently there are different
> > DT bindings defining this in different ways which resulted in
> > non-consistent kernel code.
> >
> > AFAICS, the Aardvark PCIe controller bindings define the mapping
> > as I expect:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4
> >
> > but I would like to get Rob and Arnd viewpoint on this so that
> > we can close this topic once for all.
> 
> It seems ambiguous at best, as Rob suggested it may only
> work by accident. Since there is only one upstream interrupt,
> could we simply list <GIC_SPI 100 IRQ_TYPE_NONE> as
> the destination for any IntX?

I think that would not be correct from an HW description standpoint
since there is some logic in the host bridge that behaves as an
interrupt controller (eg registers to ack/mask IRQs).

AFAICS the aardvark (it is an example) bindings below should be correct,
with an interrupt controller node within the PCI host bridge:

	pcie0: pcie@d0070000 {
		compatible = "marvell,armada-3700-pcie";
		device_type = "pci";
		reg = <0 0xd0070000 0 0x20000>;
		#address-cells = <3>;
		#size-cells = <2>;
		bus-range = <0x00 0xff>;
		interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
		#interrupt-cells = <1>;
		msi-controller;
		msi-parent = <&pcie0>;
		ranges = <0x82000000 0 0xe8000000   0 0xe8000000 0 0x1000000 /* Port 0 MEM */
			  0x81000000 0 0xe9000000   0 0xe9000000 0 0x10000>; /* Port 0 IO*/
		interrupt-map-mask = <0 0 0 7>;
		interrupt-map = <0 0 0 1 &pcie_intc 0>,
				<0 0 0 2 &pcie_intc 1>,
				<0 0 0 3 &pcie_intc 2>,
				<0 0 0 4 &pcie_intc 3>;
		pcie_intc: interrupt-controller {
			interrupt-controller;
			#interrupt-cells = <1>;
		};
	};

Thoughts ?

Lorenzo

[PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Lorenzo Pieralisi]

On Mon, Sep 24, 2018 at 10:53:13PM +0200, Arnd Bergmann wrote:
> On Tue, Sep 18, 2018 at 3:42 PM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote:
> > > +Arnd
> > >
> > > On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> > > > Update the iProc PCIe binding document for better modeling of the legacy
> > > > interrupt (INTx) support
> > > >
> > > > Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> > > > ---
> > > >  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
> > > >  1 file changed, 24 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > > index b8e48b4..7ea24dc 100644
> > > > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > > @@ -13,9 +13,6 @@ controller, used in Stingray
> > > >    PAXB-based root complex is used for external endpoint devices. PAXC-based
> > > >  root complex is connected to emulated endpoint devices internal to the ASIC
> > > >  - 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>
> > > > @@ -41,6 +38,16 @@ Required:
> > > >  - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
> > > >  address used by the iProc PCIe core (not the PCIe address)
> > > >
> > > > +Legacy interrupt (INTx) support (optional):
> > > > +
> > > > +Note INTx is for PAXB only.
> > > > +
> > > > +- interrupt-controller: claims itself as an interrupt controller for INTx
> > > > +- #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
> > > > +- interrupts: interrupt line wired to the generic GIC for INTx support
> > > > +
> > > >  MSI support (optional):
> > > >
> > > >  For older platforms without MSI integrated in the GIC, iProc PCIe core provides
> > > > @@ -77,9 +84,14 @@ Example:
> > > >                 compatible = "brcm,iproc-pcie";
> > > >                 reg = <0x18012000 0x1000>;
> > > >
> > > > +               interrupt-controller;
> > > >                 #interrupt-cells = <1>;
> > > > -               interrupt-map-mask = <0 0 0 0>;
> > > > -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> > > > +               interrupt-map-mask = <0 0 0 7>;
> > > > +               interrupt-map = <0 0 0 1 &pcie0 1>,
> > >
> > > Are you sure this works? The irq parsing code will ignore
> > > interrupt-map if interrupt-controller is found. In other words, you
> > > should have one or the other, but not both.
> > >
> > > Maybe it happens to work because "pcie0" is this node and your irq
> > > numbers are the same.
> > >
> > > Arnd, any thoughts on this?
> >
> > To start with, I think the destination IRQ number is wrong, what the
> > mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB,
> > #INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo)
> > interrupt controller.
> >
> > I really want to clean this up since currently there are different
> > DT bindings defining this in different ways which resulted in
> > non-consistent kernel code.
> >
> > AFAICS, the Aardvark PCIe controller bindings define the mapping
> > as I expect:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4
> >
> > but I would like to get Rob and Arnd viewpoint on this so that
> > we can close this topic once for all.
> 
> It seems ambiguous at best, as Rob suggested it may only
> work by accident. Since there is only one upstream interrupt,
> could we simply list <GIC_SPI 100 IRQ_TYPE_NONE> as
> the destination for any IntX?

I think that would not be correct from an HW description standpoint
since there is some logic in the host bridge that behaves as an
interrupt controller (eg registers to ack/mask IRQs).

AFAICS the aardvark (it is an example) bindings below should be correct,
with an interrupt controller node within the PCI host bridge:

	pcie0: pcie at d0070000 {
		compatible = "marvell,armada-3700-pcie";
		device_type = "pci";
		reg = <0 0xd0070000 0 0x20000>;
		#address-cells = <3>;
		#size-cells = <2>;
		bus-range = <0x00 0xff>;
		interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
		#interrupt-cells = <1>;
		msi-controller;
		msi-parent = <&pcie0>;
		ranges = <0x82000000 0 0xe8000000   0 0xe8000000 0 0x1000000 /* Port 0 MEM */
			  0x81000000 0 0xe9000000   0 0xe9000000 0 0x10000>; /* Port 0 IO*/
		interrupt-map-mask = <0 0 0 7>;
		interrupt-map = <0 0 0 1 &pcie_intc 0>,
				<0 0 0 2 &pcie_intc 1>,
				<0 0 0 3 &pcie_intc 2>,
				<0 0 0 4 &pcie_intc 3>;
		pcie_intc: interrupt-controller {
			interrupt-controller;
			#interrupt-cells = <1>;
		};
	};

Thoughts ?

Lorenzo

Re: [PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Arnd Bergmann]

On Tue, Sep 25, 2018 at 12:49 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Mon, Sep 24, 2018 at 10:53:13PM +0200, Arnd Bergmann wrote:
> > On Tue, Sep 18, 2018 at 3:42 PM Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> > >
> > > On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote:
> > > > +Arnd
> > > >
> > > > On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> > > > > Update the iProc PCIe binding document for better modeling of the legacy
> > > > > interrupt (INTx) support
> > > > >
> > > > > Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> > > > > ---
> > > > >  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
> > > > >  1 file changed, 24 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > > > index b8e48b4..7ea24dc 100644
> > > > > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > > > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > > > @@ -13,9 +13,6 @@ controller, used in Stingray
> > > > >    PAXB-based root complex is used for external endpoint devices. PAXC-based
> > > > >  root complex is connected to emulated endpoint devices internal to the ASIC
> > > > >  - 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>
> > > > > @@ -41,6 +38,16 @@ Required:
> > > > >  - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
> > > > >  address used by the iProc PCIe core (not the PCIe address)
> > > > >
> > > > > +Legacy interrupt (INTx) support (optional):
> > > > > +
> > > > > +Note INTx is for PAXB only.
> > > > > +
> > > > > +- interrupt-controller: claims itself as an interrupt controller for INTx
> > > > > +- #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
> > > > > +- interrupts: interrupt line wired to the generic GIC for INTx support
> > > > > +
> > > > >  MSI support (optional):
> > > > >
> > > > >  For older platforms without MSI integrated in the GIC, iProc PCIe core provides
> > > > > @@ -77,9 +84,14 @@ Example:
> > > > >                 compatible = "brcm,iproc-pcie";
> > > > >                 reg = <0x18012000 0x1000>;
> > > > >
> > > > > +               interrupt-controller;
> > > > >                 #interrupt-cells = <1>;
> > > > > -               interrupt-map-mask = <0 0 0 0>;
> > > > > -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> > > > > +               interrupt-map-mask = <0 0 0 7>;
> > > > > +               interrupt-map = <0 0 0 1 &pcie0 1>,
> > > >
> > > > Are you sure this works? The irq parsing code will ignore
> > > > interrupt-map if interrupt-controller is found. In other words, you
> > > > should have one or the other, but not both.
> > > >
> > > > Maybe it happens to work because "pcie0" is this node and your irq
> > > > numbers are the same.
> > > >
> > > > Arnd, any thoughts on this?
> > >
> > > To start with, I think the destination IRQ number is wrong, what the
> > > mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB,
> > > #INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo)
> > > interrupt controller.
> > >
> > > I really want to clean this up since currently there are different
> > > DT bindings defining this in different ways which resulted in
> > > non-consistent kernel code.
> > >
> > > AFAICS, the Aardvark PCIe controller bindings define the mapping
> > > as I expect:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4
> > >
> > > but I would like to get Rob and Arnd viewpoint on this so that
> > > we can close this topic once for all.
> >
> > It seems ambiguous at best, as Rob suggested it may only
> > work by accident. Since there is only one upstream interrupt,
> > could we simply list <GIC_SPI 100 IRQ_TYPE_NONE> as
> > the destination for any IntX?
>
> I think that would not be correct from an HW description standpoint
> since there is some logic in the host bridge that behaves as an
> interrupt controller (eg registers to ack/mask IRQs).
>
> AFAICS the aardvark (it is an example) bindings below should be correct,
> with an interrupt controller node within the PCI host bridge:
>
>         pcie0: pcie@d0070000 {
>                 compatible = "marvell,armada-3700-pcie";
>                 device_type = "pci";
>                 reg = <0 0xd0070000 0 0x20000>;
>                 #address-cells = <3>;
>                 #size-cells = <2>;
>                 bus-range = <0x00 0xff>;
>                 interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
>                 #interrupt-cells = <1>;
>                 msi-controller;
>                 msi-parent = <&pcie0>;
>                 ranges = <0x82000000 0 0xe8000000   0 0xe8000000 0 0x1000000 /* Port 0 MEM */
>                           0x81000000 0 0xe9000000   0 0xe9000000 0 0x10000>; /* Port 0 IO*/
>                 interrupt-map-mask = <0 0 0 7>;
>                 interrupt-map = <0 0 0 1 &pcie_intc 0>,
>                                 <0 0 0 2 &pcie_intc 1>,
>                                 <0 0 0 3 &pcie_intc 2>,
>                                 <0 0 0 4 &pcie_intc 3>;
>                 pcie_intc: interrupt-controller {
>                         interrupt-controller;
>                         #interrupt-cells = <1>;
>                 };
>         };
>
> Thoughts ?

Yes, I think that's better. We probably still need to move the
interrupts, msi-controller, msi-parent and interrupt-parent
properties into the child node.

        Arnd

[PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Arnd Bergmann]

On Tue, Sep 25, 2018 at 12:49 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Mon, Sep 24, 2018 at 10:53:13PM +0200, Arnd Bergmann wrote:
> > On Tue, Sep 18, 2018 at 3:42 PM Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> > >
> > > On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote:
> > > > +Arnd
> > > >
> > > > On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> > > > > Update the iProc PCIe binding document for better modeling of the legacy
> > > > > interrupt (INTx) support
> > > > >
> > > > > Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> > > > > ---
> > > > >  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
> > > > >  1 file changed, 24 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > > > index b8e48b4..7ea24dc 100644
> > > > > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > > > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> > > > > @@ -13,9 +13,6 @@ controller, used in Stingray
> > > > >    PAXB-based root complex is used for external endpoint devices. PAXC-based
> > > > >  root complex is connected to emulated endpoint devices internal to the ASIC
> > > > >  - 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>
> > > > > @@ -41,6 +38,16 @@ Required:
> > > > >  - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
> > > > >  address used by the iProc PCIe core (not the PCIe address)
> > > > >
> > > > > +Legacy interrupt (INTx) support (optional):
> > > > > +
> > > > > +Note INTx is for PAXB only.
> > > > > +
> > > > > +- interrupt-controller: claims itself as an interrupt controller for INTx
> > > > > +- #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
> > > > > +- interrupts: interrupt line wired to the generic GIC for INTx support
> > > > > +
> > > > >  MSI support (optional):
> > > > >
> > > > >  For older platforms without MSI integrated in the GIC, iProc PCIe core provides
> > > > > @@ -77,9 +84,14 @@ Example:
> > > > >                 compatible = "brcm,iproc-pcie";
> > > > >                 reg = <0x18012000 0x1000>;
> > > > >
> > > > > +               interrupt-controller;
> > > > >                 #interrupt-cells = <1>;
> > > > > -               interrupt-map-mask = <0 0 0 0>;
> > > > > -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> > > > > +               interrupt-map-mask = <0 0 0 7>;
> > > > > +               interrupt-map = <0 0 0 1 &pcie0 1>,
> > > >
> > > > Are you sure this works? The irq parsing code will ignore
> > > > interrupt-map if interrupt-controller is found. In other words, you
> > > > should have one or the other, but not both.
> > > >
> > > > Maybe it happens to work because "pcie0" is this node and your irq
> > > > numbers are the same.
> > > >
> > > > Arnd, any thoughts on this?
> > >
> > > To start with, I think the destination IRQ number is wrong, what the
> > > mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB,
> > > #INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo)
> > > interrupt controller.
> > >
> > > I really want to clean this up since currently there are different
> > > DT bindings defining this in different ways which resulted in
> > > non-consistent kernel code.
> > >
> > > AFAICS, the Aardvark PCIe controller bindings define the mapping
> > > as I expect:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4
> > >
> > > but I would like to get Rob and Arnd viewpoint on this so that
> > > we can close this topic once for all.
> >
> > It seems ambiguous at best, as Rob suggested it may only
> > work by accident. Since there is only one upstream interrupt,
> > could we simply list <GIC_SPI 100 IRQ_TYPE_NONE> as
> > the destination for any IntX?
>
> I think that would not be correct from an HW description standpoint
> since there is some logic in the host bridge that behaves as an
> interrupt controller (eg registers to ack/mask IRQs).
>
> AFAICS the aardvark (it is an example) bindings below should be correct,
> with an interrupt controller node within the PCI host bridge:
>
>         pcie0: pcie at d0070000 {
>                 compatible = "marvell,armada-3700-pcie";
>                 device_type = "pci";
>                 reg = <0 0xd0070000 0 0x20000>;
>                 #address-cells = <3>;
>                 #size-cells = <2>;
>                 bus-range = <0x00 0xff>;
>                 interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
>                 #interrupt-cells = <1>;
>                 msi-controller;
>                 msi-parent = <&pcie0>;
>                 ranges = <0x82000000 0 0xe8000000   0 0xe8000000 0 0x1000000 /* Port 0 MEM */
>                           0x81000000 0 0xe9000000   0 0xe9000000 0 0x10000>; /* Port 0 IO*/
>                 interrupt-map-mask = <0 0 0 7>;
>                 interrupt-map = <0 0 0 1 &pcie_intc 0>,
>                                 <0 0 0 2 &pcie_intc 1>,
>                                 <0 0 0 3 &pcie_intc 2>,
>                                 <0 0 0 4 &pcie_intc 3>;
>                 pcie_intc: interrupt-controller {
>                         interrupt-controller;
>                         #interrupt-cells = <1>;
>                 };
>         };
>
> Thoughts ?

Yes, I think that's better. We probably still need to move the
interrupts, msi-controller, msi-parent and interrupt-parent
properties into the child node.

        Arnd

Re: [PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Ray Jui]

On 9/25/2018 3:55 AM, Arnd Bergmann wrote:
> On Tue, Sep 25, 2018 at 12:49 PM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
>>
>> On Mon, Sep 24, 2018 at 10:53:13PM +0200, Arnd Bergmann wrote:
>>> On Tue, Sep 18, 2018 at 3:42 PM Lorenzo Pieralisi
>>> <lorenzo.pieralisi@arm.com> wrote:
>>>>
>>>> On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote:
>>>>> +Arnd
>>>>>
>>>>> On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
>>>>>> Update the iProc PCIe binding document for better modeling of the legacy
>>>>>> interrupt (INTx) support
>>>>>>
>>>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>>>>> ---
>>>>>>  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
>>>>>>  1 file changed, 24 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>>>> index b8e48b4..7ea24dc 100644
>>>>>> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>>>> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>>>> @@ -13,9 +13,6 @@ controller, used in Stingray
>>>>>>    PAXB-based root complex is used for external endpoint devices. PAXC-based
>>>>>>  root complex is connected to emulated endpoint devices internal to the ASIC
>>>>>>  - 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>
>>>>>> @@ -41,6 +38,16 @@ Required:
>>>>>>  - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
>>>>>>  address used by the iProc PCIe core (not the PCIe address)
>>>>>>
>>>>>> +Legacy interrupt (INTx) support (optional):
>>>>>> +
>>>>>> +Note INTx is for PAXB only.
>>>>>> +
>>>>>> +- interrupt-controller: claims itself as an interrupt controller for INTx
>>>>>> +- #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
>>>>>> +- interrupts: interrupt line wired to the generic GIC for INTx support
>>>>>> +
>>>>>>  MSI support (optional):
>>>>>>
>>>>>>  For older platforms without MSI integrated in the GIC, iProc PCIe core provides
>>>>>> @@ -77,9 +84,14 @@ Example:
>>>>>>                 compatible = "brcm,iproc-pcie";
>>>>>>                 reg = <0x18012000 0x1000>;
>>>>>>
>>>>>> +               interrupt-controller;
>>>>>>                 #interrupt-cells = <1>;
>>>>>> -               interrupt-map-mask = <0 0 0 0>;
>>>>>> -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
>>>>>> +               interrupt-map-mask = <0 0 0 7>;
>>>>>> +               interrupt-map = <0 0 0 1 &pcie0 1>,
>>>>>
>>>>> Are you sure this works? The irq parsing code will ignore
>>>>> interrupt-map if interrupt-controller is found. In other words, you
>>>>> should have one or the other, but not both.
>>>>>
>>>>> Maybe it happens to work because "pcie0" is this node and your irq
>>>>> numbers are the same.
>>>>>
>>>>> Arnd, any thoughts on this?
>>>>
>>>> To start with, I think the destination IRQ number is wrong, what the
>>>> mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB,
>>>> #INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo)
>>>> interrupt controller.
>>>>
>>>> I really want to clean this up since currently there are different
>>>> DT bindings defining this in different ways which resulted in
>>>> non-consistent kernel code.
>>>>
>>>> AFAICS, the Aardvark PCIe controller bindings define the mapping
>>>> as I expect:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4
>>>>
>>>> but I would like to get Rob and Arnd viewpoint on this so that
>>>> we can close this topic once for all.
>>>
>>> It seems ambiguous at best, as Rob suggested it may only
>>> work by accident. Since there is only one upstream interrupt,
>>> could we simply list <GIC_SPI 100 IRQ_TYPE_NONE> as
>>> the destination for any IntX?
>>
>> I think that would not be correct from an HW description standpoint
>> since there is some logic in the host bridge that behaves as an
>> interrupt controller (eg registers to ack/mask IRQs).
>>
>> AFAICS the aardvark (it is an example) bindings below should be correct,
>> with an interrupt controller node within the PCI host bridge:
>>
>>         pcie0: pcie@d0070000 {
>>                 compatible = "marvell,armada-3700-pcie";
>>                 device_type = "pci";
>>                 reg = <0 0xd0070000 0 0x20000>;
>>                 #address-cells = <3>;
>>                 #size-cells = <2>;
>>                 bus-range = <0x00 0xff>;
>>                 interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
>>                 #interrupt-cells = <1>;
>>                 msi-controller;
>>                 msi-parent = <&pcie0>;
>>                 ranges = <0x82000000 0 0xe8000000   0 0xe8000000 0 0x1000000 /* Port 0 MEM */
>>                           0x81000000 0 0xe9000000   0 0xe9000000 0 0x10000>; /* Port 0 IO*/
>>                 interrupt-map-mask = <0 0 0 7>;
>>                 interrupt-map = <0 0 0 1 &pcie_intc 0>,
>>                                 <0 0 0 2 &pcie_intc 1>,
>>                                 <0 0 0 3 &pcie_intc 2>,
>>                                 <0 0 0 4 &pcie_intc 3>;
>>                 pcie_intc: interrupt-controller {
>>                         interrupt-controller;
>>                         #interrupt-cells = <1>;
>>                 };
>>         };
>>
>> Thoughts ?
> 
> Yes, I think that's better. We probably still need to move the
> interrupts, msi-controller, msi-parent and interrupt-parent
> properties into the child node.

Okay thanks for all the feedback. In my case, I think I just to need
create a dummy 'intc' subnode under the pcie node and declare it as a
(dummy) interrupt controller).

I'll make the change in my next revision.

> 
>         Arnd
> 

Re: [PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support

[Ray Jui]

On 9/25/2018 3:55 AM, Arnd Bergmann wrote:
> On Tue, Sep 25, 2018 at 12:49 PM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
>>
>> On Mon, Sep 24, 2018 at 10:53:13PM +0200, Arnd Bergmann wrote:
>>> On Tue, Sep 18, 2018 at 3:42 PM Lorenzo Pieralisi
>>> <lorenzo.pieralisi@arm.com> wrote:
>>>>
>>>> On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote:
>>>>> +Arnd
>>>>>
>>>>> On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
>>>>>> Update the iProc PCIe binding document for better modeling of the legacy
>>>>>> interrupt (INTx) support
>>>>>>
>>>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>>>>> ---
>>>>>>  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    | 31 +++++++++++++++++-----
>>>>>>  1 file changed, 24 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>>>> index b8e48b4..7ea24dc 100644
>>>>>> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>>>> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>>>> @@ -13,9 +13,6 @@ controller, used in Stingray
>>>>>>    PAXB-based root complex is used for external endpoint devices. PAXC-based
>>>>>>  root complex is connected to emulated endpoint devices internal to the ASIC
>>>>>>  - 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>
>>>>>> @@ -41,6 +38,16 @@ Required:
>>>>>>  - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
>>>>>>  address used by the iProc PCIe core (not the PCIe address)
>>>>>>
>>>>>> +Legacy interrupt (INTx) support (optional):
>>>>>> +
>>>>>> +Note INTx is for PAXB only.
>>>>>> +
>>>>>> +- interrupt-controller: claims itself as an interrupt controller for INTx
>>>>>> +- #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
>>>>>> +- interrupts: interrupt line wired to the generic GIC for INTx support
>>>>>> +
>>>>>>  MSI support (optional):
>>>>>>
>>>>>>  For older platforms without MSI integrated in the GIC, iProc PCIe core provides
>>>>>> @@ -77,9 +84,14 @@ Example:
>>>>>>                 compatible = "brcm,iproc-pcie";
>>>>>>                 reg = <0x18012000 0x1000>;
>>>>>>
>>>>>> +               interrupt-controller;
>>>>>>                 #interrupt-cells = <1>;
>>>>>> -               interrupt-map-mask = <0 0 0 0>;
>>>>>> -               interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
>>>>>> +               interrupt-map-mask = <0 0 0 7>;
>>>>>> +               interrupt-map = <0 0 0 1 &pcie0 1>,
>>>>>
>>>>> Are you sure this works? The irq parsing code will ignore
>>>>> interrupt-map if interrupt-controller is found. In other words, you
>>>>> should have one or the other, but not both.
>>>>>
>>>>> Maybe it happens to work because "pcie0" is this node and your irq
>>>>> numbers are the same.
>>>>>
>>>>> Arnd, any thoughts on this?
>>>>
>>>> To start with, I think the destination IRQ number is wrong, what the
>>>> mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB,
>>>> #INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo)
>>>> interrupt controller.
>>>>
>>>> I really want to clean this up since currently there are different
>>>> DT bindings defining this in different ways which resulted in
>>>> non-consistent kernel code.
>>>>
>>>> AFAICS, the Aardvark PCIe controller bindings define the mapping
>>>> as I expect:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4
>>>>
>>>> but I would like to get Rob and Arnd viewpoint on this so that
>>>> we can close this topic once for all.
>>>
>>> It seems ambiguous at best, as Rob suggested it may only
>>> work by accident. Since there is only one upstream interrupt,
>>> could we simply list <GIC_SPI 100 IRQ_TYPE_NONE> as
>>> the destination for any IntX?
>>
>> I think that would not be correct from an HW description standpoint
>> since there is some logic in the host bridge that behaves as an
>> interrupt controller (eg registers to ack/mask IRQs).
>>
>> AFAICS the aardvark (it is an example) bindings below should be correct,
>> with an interrupt controller node within the PCI host bridge:
>>
>>         pcie0: pcie@d0070000 {
>>                 compatible = "marvell,armada-3700-pcie";
>>                 device_type = "pci";
>>                 reg = <0 0xd0070000 0 0x20000>;
>>                 #address-cells = <3>;
>>                 #size-cells = <2>;
>>                 bus-range = <0x00 0xff>;
>>                 interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
>>                 #interrupt-cells = <1>;
>>                 msi-controller;
>>                 msi-parent = <&pcie0>;
>>                 ranges = <0x82000000 0 0xe8000000   0 0xe8000000 0 0x1000000 /* Port 0 MEM */
>>                           0x81000000 0 0xe9000000   0 0xe9000000 0 0x10000>; /* Port 0 IO*/
>>                 interrupt-map-mask = <0 0 0 7>;
>>                 interrupt-map = <0 0 0 1 &pcie_intc 0>,
>>                                 <0 0 0 2 &pcie_intc 1>,
>>                                 <0 0 0 3 &pcie_intc 2>,
>>                                 <0 0 0 4 &pcie_intc 3>;
>>                 pcie_intc: interrupt-controller {
>>                         interrupt-controller;
>>                         #interrupt-cells = <1>;
>>                 };
>>         };
>>
>> Thoughts ?
> 
> Yes, I think that's better. We probably still need to move the
> interrupts, msi-controller, msi-parent and interrupt-parent
> properties into the child node.

Okay thanks for all the feedback. In my case, I think I just to need
create a dummy 'intc' subnode under the pcie node and declare it as a
(dummy) interrupt controller).

I'll make the change in my next revision.

> 
>         Arnd
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel