[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 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 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 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

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

[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
>

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 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>;
>>   
>>
> 
> 

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

[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

[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 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

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

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

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
>