PCI: Add legacy interrupt support in pci-j721e

Add bindings to specify interrupt controller for legacy interrupts.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml +7 -0

index cc900202df29..f461d7b4c0cc 100644
@@ -74,6 +74,11 @@ properties:
 
   msi-map: true
 
+patternProperties:
+  "interrupt-controller":
+    type: object
+    description: interrupt controller to handle legacy interrupts.
+
 required:
   - compatible
   - reg

@@ -97,6 +102,8 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
     #include <dt-bindings/soc/ti,sci_pm_domain.h>
     #include <dt-bindings/gpio/gpio.h>
 

Quoted 33 lines · Kishon Vijay Abraham I

Add bindings to specify interrupt controller for legacy interrupts.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml +7 -0

index cc900202df29..f461d7b4c0cc 100644
@@ -74,6 +74,11 @@ properties:
 
   msi-map: true
 
+patternProperties:
+  "interrupt-controller":
+    type: object
+    description: interrupt controller to handle legacy interrupts.
+
 required:
   - compatible
   - reg

@@ -97,6 +102,8 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
     #include <dt-bindings/soc/ti,sci_pm_domain.h>
     #include <dt-bindings/gpio/gpio.h>
 

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml examples: · line 147

Quoted 11 lines

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml +7 -0

@@ -131,5 +138,13 @@ examples:
             ranges = <0x01000000 0x0 0x10001000  0x00 0x10001000  0x0 0x0010000>,
                      <0x02000000 0x0 0x10011000  0x00 0x10011000  0x0 0x7fef000>;
             dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>;
+
+
+            pcie0_intc: interrupt-controller {
+                    interrupt-controller;
+                    #interrupt-cells = <1>;
+                    interrupt-parent = <&gic500>;
+                    interrupts = <GIC_SPI 312 IRQ_TYPE_EDGE_RISING>;

Are you sure about the edge signalling? How is the interrupt retriggered when the input is still high, which could well be the case for shared INTx?

Add bindings to specify interrupt controller for legacy interrupts.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml +0 -0

index cc900202df29..f461d7b4c0cc 100644

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml +13 -0

+    type: object
+    description: interrupt controller to handle legacy interrupts.
+
 required:
   - compatible
   - reg

@@ -97,6 +102,8 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
     #include <dt-bindings/soc/ti,sci_pm_domain.h>
     #include <dt-bindings/gpio/gpio.h>
 

@@ -131,5 +138,13 @@ examples:
             ranges = <0x01000000 0x0 0x10001000  0x00 0x10001000  0x0 0x0010000>,
                      <0x02000000 0x0 0x10011000  0x00 0x10011000  0x0 0x7fef000>;
             dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>;
+
+
+            pcie0_intc: interrupt-controller {
+                    interrupt-controller;
+                    #interrupt-cells = <1>;
+                    interrupt-parent = <&gic500>;
+                    interrupts = <GIC_SPI 312 IRQ_TYPE_EDGE_RISING>;
+            };
         };
     };

Signature · 1 line

2.17.1

Quoted 15 lines · Kishon Vijay Abraham I

Add bindings to specify interrupt controller for legacy interrupts.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml +0 -0

index cc900202df29..f461d7b4c0cc 100644
@@ -74,6 +74,11 @@ properties:
 
   msi-map: true
 

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml properties: · line 78

Quoted 2 lines

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml +2 -0

+patternProperties:
+  "interrupt-controller":

Not a pattern unless you meant for foo-interrupt-controller-bar to be valid.

Anything is allowed in the node?

Quoted 34 lines

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml +13 -0

+    type: object
+    description: interrupt controller to handle legacy interrupts.
+
 required:
   - compatible
   - reg

@@ -97,6 +102,8 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
     #include <dt-bindings/soc/ti,sci_pm_domain.h>
     #include <dt-bindings/gpio/gpio.h>
 

@@ -131,5 +138,13 @@ examples:
             ranges = <0x01000000 0x0 0x10001000  0x00 0x10001000  0x0 0x0010000>,
                      <0x02000000 0x0 0x10011000  0x00 0x10011000  0x0 0x7fef000>;
             dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>;
+
+
+            pcie0_intc: interrupt-controller {
+                    interrupt-controller;
+                    #interrupt-cells = <1>;
+                    interrupt-parent = <&gic500>;
+                    interrupts = <GIC_SPI 312 IRQ_TYPE_EDGE_RISING>;
+            };
         };
     };

Signature · 1 line

2.17.1

Hi Rob,

Quoted 22 lines · Rob Herring

Quoted 12 lines · Kishon Vijay Abraham I

Add bindings to specify interrupt controller for legacy interrupts.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml +0 -0

index cc900202df29..f461d7b4c0cc 100644

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml properties: · line 78

Quoted 5 lines

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml +2 -0

@@ -74,6 +74,11 @@ properties:
 
   msi-map: true
 
+patternProperties:
+  "interrupt-controller":

Not a pattern unless you meant for foo-interrupt-controller-bar to be valid.

Quoted 1 line

Anything is allowed in the node?

It's same as whatever is defined in schemas/interrupt-controller.yaml, just that it should be a subnode of pcie@. Should I add whatever is present in schemas/interrupt-controller.yaml here?

Thanks
Kishon

Quoted 34 lines

Quoted 34 lines

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml +13 -0

+    type: object
+    description: interrupt controller to handle legacy interrupts.
+
 required:
   - compatible
   - reg

@@ -97,6 +102,8 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
     #include <dt-bindings/soc/ti,sci_pm_domain.h>
     #include <dt-bindings/gpio/gpio.h>
 

@@ -131,5 +138,13 @@ examples:
             ranges = <0x01000000 0x0 0x10001000  0x00 0x10001000  0x0 0x0010000>,
                      <0x02000000 0x0 0x10011000  0x00 0x10011000  0x0 0x7fef000>;
             dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>;
+
+
+            pcie0_intc: interrupt-controller {
+                    interrupt-controller;
+                    #interrupt-cells = <1>;
+                    interrupt-parent = <&gic500>;
+                    interrupts = <GIC_SPI 312 IRQ_TYPE_EDGE_RISING>;
+            };
         };
     };

Signature · 1 line

2.17.1

Hi Rob,

Quoted 25 lines · Kishon Vijay Abraham I

Hi Rob,

Quoted 22 lines · Rob Herring

Quoted 12 lines · Kishon Vijay Abraham I

Add bindings to specify interrupt controller for legacy interrupts.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml +0 -0

index cc900202df29..f461d7b4c0cc 100644

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml properties: · line 78

Quoted 5 lines

Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml +2 -0

@@ -74,6 +74,11 @@ properties:
 
   msi-map: true
 
+patternProperties:
+  "interrupt-controller":

Not a pattern unless you meant for foo-interrupt-controller-bar to be valid.

Quoted 5 lines

Quoted 1 line

Anything is allowed in the node?

It's same as whatever is defined in schemas/interrupt-controller.yaml, just that it should be a subnode of pcie@. Should I add whatever is present in schemas/interrupt-controller.yaml here?

Can you suggest how to include this?

Add PCI legacy interrupt support for J721E. J721E has a single HW interrupt line for all the four legacy interrupts INTA/INTB/INTC/INTD. The HW interrupt line connected to GIC is a pulse interrupt whereas the legacy interrupts by definition is level interrupt. In order to provide level interrupt functionality to edge interrupt line, PCIe in J721E has provided IRQ_EOI register.

However due to Errata ID #i2094 ([1]), EOI feature is not enabled in HW and only a single pulse interrupt will be generated for every ASSERT_INTx/DEASSERT_INTx.

[1] -> J721E DRA829/TDA4VM Processors Silicon Revision 1.1/1.0 SPRZ455A – DECEMBER 2020 – REVISED AUGUST 2021
(https://www.ti.com/lit/er/sprz455a/sprz455a.pdf)

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/pci/controller/cadence/pci-j721e.c | 85 ++++++++++++++++++++++
 1 file changed, 85 insertions(+)

drivers/pci/controller/cadence/pci-j721e.c +11 -0

index 2ec037c43bd5..c2e7a78dc31f 100644
@@ -29,6 +29,13 @@
 #define LINK_DOWN		BIT(1)
 #define J7200_LINK_DOWN		BIT(10)
 
+#define EOI_REG			0x10
+
+#define ENABLE_REG_SYS_0	0x100
+#define STATUS_REG_SYS_0	0x500
+#define STATUS_CLR_REG_SYS_0	0x700
+#define INTx_EN(num)		(1 << (num))
+
 #define J721E_PCIE_USER_CMD_STATUS	0x4
 #define LINK_TRAINING_ENABLE		BIT(0)
 

@@ -59,6 +66,7 @@ struct j721e_pcie {
 	void __iomem		*user_cfg_base;
 	void __iomem		*intd_cfg_base;
 	u32			linkdown_irq_regfield;
+	struct irq_domain	*legacy_irq_domain;
 };
 
 enum j721e_pcie_mode {

@@ -121,6 +129,79 @@ static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie)
 	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_2, reg);
 }
 
+static void j721e_pcie_v1_legacy_irq_handler(struct irq_desc *desc)
+{
+	struct j721e_pcie *pcie = irq_desc_get_handler_data(desc);

drivers/pci/controller/cadence/pci-j721e.c +34 -0

+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops j721e_pcie_intx_domain_ops = {
+	.map = j721e_pcie_intx_map,
+};
+
+static int j721e_pcie_config_legacy_irq(struct j721e_pcie *pcie)
+{
+	struct irq_domain *legacy_irq_domain;
+	struct device *dev = pcie->dev;
+	struct device_node *node = dev->of_node;
+	struct device_node *intc_node;
+	int irq, i;
+	u32 reg;
+
+	intc_node = of_get_child_by_name(node, "interrupt-controller");
+	if (!intc_node) {
+		dev_dbg(dev, "interrupt-controller node is absent. Legacy INTR not supported\n");
+		return 0;
+	}
+
+	irq = irq_of_parse_and_map(intc_node, 0);
+	if (!irq) {
+		dev_err(dev, "Failed to parse and map legacy irq\n");
+		return -EINVAL;
+	}
+	irq_set_chained_handler_and_data(irq, j721e_pcie_v1_legacy_irq_handler, pcie);
+
+	legacy_irq_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX,
+						  &j721e_pcie_intx_domain_ops, pcie);
+	if (!legacy_irq_domain) {

Quoted 8 lines · Kishon Vijay Abraham I

Add PCI legacy interrupt support for J721E. J721E has a single HW interrupt line for all the four legacy interrupts INTA/INTB/INTC/INTD. The HW interrupt line connected to GIC is a pulse interrupt whereas the legacy interrupts by definition is level interrupt. In order to provide level interrupt functionality to edge interrupt line, PCIe in J721E has provided IRQ_EOI register.

Quoted 3 lines

However due to Errata ID #i2094 ([1]), EOI feature is not enabled in HW and only a single pulse interrupt will be generated for every ASSERT_INTx/DEASSERT_INTx.

So my earlier remark stands. If you get a single edge, how do you handle a level that is still high after having handled the interrupt on hardware that has this bug?

Quoted 41 lines

[1] -> J721E DRA829/TDA4VM Processors Silicon Revision 1.1/1.0 SPRZ455A – DECEMBER 2020 – REVISED AUGUST 2021
(https://www.ti.com/lit/er/sprz455a/sprz455a.pdf)

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/pci/controller/cadence/pci-j721e.c | 85 ++++++++++++++++++++++
 1 file changed, 85 insertions(+)

drivers/pci/controller/cadence/pci-j721e.c +8 -0

index 2ec037c43bd5..c2e7a78dc31f 100644
@@ -29,6 +29,13 @@
 #define LINK_DOWN		BIT(1)
 #define J7200_LINK_DOWN		BIT(10)
 
+#define EOI_REG			0x10
+
+#define ENABLE_REG_SYS_0	0x100
+#define STATUS_REG_SYS_0	0x500
+#define STATUS_CLR_REG_SYS_0	0x700
+#define INTx_EN(num)		(1 << (num))
+
 #define J721E_PCIE_USER_CMD_STATUS	0x4
 #define LINK_TRAINING_ENABLE		BIT(0)
 

@@ -59,6 +66,7 @@ struct j721e_pcie {
 	void __iomem		*user_cfg_base;
 	void __iomem		*intd_cfg_base;
 	u32			linkdown_irq_regfield;
+	struct irq_domain	*legacy_irq_domain;
 };
 
 enum j721e_pcie_mode {

@@ -121,6 +129,79 @@ static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie)
 	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_2, reg);
 }
 

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 144

Quoted 13 lines

drivers/pci/controller/cadence/pci-j721e.c +13 -0

+static void j721e_pcie_v1_legacy_irq_handler(struct irq_desc *desc)
+{
+	struct j721e_pcie *pcie = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	int i, virq;
+	u32 reg;
+
+	chained_irq_enter(chip, desc);
+
+	for (i = 0; i < PCI_NUM_INTX; i++) {
+		reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_0);
+		if (!(reg & INTx_EN(i)))
+			continue;

Why do you need to perform multiple reads? Surely reg contains all the bits you need, doesn't it?

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 147

Quoted 4 lines

drivers/pci/controller/cadence/pci-j721e.c +3 -0

+
+		virq = irq_find_mapping(pcie->legacy_irq_domain, 3 - i);
+		generic_handle_irq(virq);

Please combine both lines into a single generic_handle_domain_irq() call.

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 148

Quoted 2 lines

drivers/pci/controller/cadence/pci-j721e.c +1 -0

+		j721e_pcie_intd_writel(pcie, STATUS_CLR_REG_SYS_0, INTx_EN(i));

What is the purpose of this write? It feels like this should be a irq_eoi callback.

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 156

Quoted 9 lines

drivers/pci/controller/cadence/pci-j721e.c +8 -0

+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static int j721e_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);

An INTx interrupt is a level interrupt. Please use the corresponding flow.

Quoted 39 lines

drivers/pci/controller/cadence/pci-j721e.c +38 -0

+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops j721e_pcie_intx_domain_ops = {
+	.map = j721e_pcie_intx_map,
+};
+
+static int j721e_pcie_config_legacy_irq(struct j721e_pcie *pcie)
+{
+	struct irq_domain *legacy_irq_domain;
+	struct device *dev = pcie->dev;
+	struct device_node *node = dev->of_node;
+	struct device_node *intc_node;
+	int irq, i;
+	u32 reg;
+
+	intc_node = of_get_child_by_name(node, "interrupt-controller");
+	if (!intc_node) {
+		dev_dbg(dev, "interrupt-controller node is absent. Legacy INTR not supported\n");
+		return 0;
+	}
+
+	irq = irq_of_parse_and_map(intc_node, 0);
+	if (!irq) {
+		dev_err(dev, "Failed to parse and map legacy irq\n");
+		return -EINVAL;
+	}
+	irq_set_chained_handler_and_data(irq, j721e_pcie_v1_legacy_irq_handler, pcie);
+
+	legacy_irq_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX,
+						  &j721e_pcie_intx_domain_ops, pcie);
+	if (!legacy_irq_domain) {
+		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
+		return -EINVAL;
+	}
+	pcie->legacy_irq_domain = legacy_irq_domain;

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 200

Quoted 6 lines

drivers/pci/controller/cadence/pci-j721e.c +6 -0

+
+	for (i = 0; i < PCI_NUM_INTX; i++) {
+		reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_0);
+		reg |= INTx_EN(i);
+		j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_0, reg);
+	}

This should be moved to the irq_unmask() callback.

Hi Marc,

Quoted 14 lines · Marc Zyngier

Quoted 11 lines · Kishon Vijay Abraham I

Add PCI legacy interrupt support for J721E. J721E has a single HW interrupt line for all the four legacy interrupts INTA/INTB/INTC/INTD. The HW interrupt line connected to GIC is a pulse interrupt whereas the legacy interrupts by definition is level interrupt. In order to provide level interrupt functionality to edge interrupt line, PCIe in J721E has provided IRQ_EOI register.

However due to Errata ID #i2094 ([1]), EOI feature is not enabled in HW and only a single pulse interrupt will be generated for every ASSERT_INTx/DEASSERT_INTx.

Quoted 3 lines

So my earlier remark stands. If you get a single edge, how do you handle a level that is still high after having handled the interrupt on hardware that has this bug?

Right, this hardware (J721E) has a bug but was fixed in J7200 (Patch 3/3 handles that).

Quoted 41 lines

Quoted 41 lines

[1] -> J721E DRA829/TDA4VM Processors Silicon Revision 1.1/1.0 SPRZ455A – DECEMBER 2020 – REVISED AUGUST 2021
(https://www.ti.com/lit/er/sprz455a/sprz455a.pdf)

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/pci/controller/cadence/pci-j721e.c | 85 ++++++++++++++++++++++
 1 file changed, 85 insertions(+)

drivers/pci/controller/cadence/pci-j721e.c +8 -0

index 2ec037c43bd5..c2e7a78dc31f 100644
@@ -29,6 +29,13 @@
 #define LINK_DOWN		BIT(1)
 #define J7200_LINK_DOWN		BIT(10)
 
+#define EOI_REG			0x10
+
+#define ENABLE_REG_SYS_0	0x100
+#define STATUS_REG_SYS_0	0x500
+#define STATUS_CLR_REG_SYS_0	0x700
+#define INTx_EN(num)		(1 << (num))
+
 #define J721E_PCIE_USER_CMD_STATUS	0x4
 #define LINK_TRAINING_ENABLE		BIT(0)
 

@@ -59,6 +66,7 @@ struct j721e_pcie {
 	void __iomem		*user_cfg_base;
 	void __iomem		*intd_cfg_base;
 	u32			linkdown_irq_regfield;
+	struct irq_domain	*legacy_irq_domain;
 };
 
 enum j721e_pcie_mode {

@@ -121,6 +129,79 @@ static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie)
 	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_2, reg);
 }
 

Quoted 16 lines

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 144

Quoted 13 lines

drivers/pci/controller/cadence/pci-j721e.c +13 -0

+static void j721e_pcie_v1_legacy_irq_handler(struct irq_desc *desc)
+{
+	struct j721e_pcie *pcie = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	int i, virq;
+	u32 reg;
+
+	chained_irq_enter(chip, desc);
+
+	for (i = 0; i < PCI_NUM_INTX; i++) {
+		reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_0);
+		if (!(reg & INTx_EN(i)))
+			continue;

Why do you need to perform multiple reads? Surely reg contains all the bits you need, doesn't it?

Right, will fix it up.

Quoted 7 lines

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 147

Quoted 4 lines

drivers/pci/controller/cadence/pci-j721e.c +3 -0

+
+		virq = irq_find_mapping(pcie->legacy_irq_domain, 3 - i);
+		generic_handle_irq(virq);

Please combine both lines into a single generic_handle_domain_irq() call.

Okay.

Quoted 1 line

Quoted 4 lines

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 148

Quoted 1 line

drivers/pci/controller/cadence/pci-j721e.c +1 -0

+		j721e_pcie_intd_writel(pcie, STATUS_CLR_REG_SYS_0, INTx_EN(i));

What is the purpose of this write? It feels like this should be a irq_eoi callback.

It's an IRQ ACK, since in this platform the level to edge is not implemented properly in HW.

Quoted 11 lines

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 156

Quoted 9 lines

drivers/pci/controller/cadence/pci-j721e.c +8 -0

+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static int j721e_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);

An INTx interrupt is a level interrupt. Please use the corresponding flow.

Okay.

Quoted 39 lines

Quoted 39 lines

drivers/pci/controller/cadence/pci-j721e.c +38 -0

+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops j721e_pcie_intx_domain_ops = {
+	.map = j721e_pcie_intx_map,
+};
+
+static int j721e_pcie_config_legacy_irq(struct j721e_pcie *pcie)
+{
+	struct irq_domain *legacy_irq_domain;
+	struct device *dev = pcie->dev;
+	struct device_node *node = dev->of_node;
+	struct device_node *intc_node;
+	int irq, i;
+	u32 reg;
+
+	intc_node = of_get_child_by_name(node, "interrupt-controller");
+	if (!intc_node) {
+		dev_dbg(dev, "interrupt-controller node is absent. Legacy INTR not supported\n");
+		return 0;
+	}
+
+	irq = irq_of_parse_and_map(intc_node, 0);
+	if (!irq) {
+		dev_err(dev, "Failed to parse and map legacy irq\n");
+		return -EINVAL;
+	}
+	irq_set_chained_handler_and_data(irq, j721e_pcie_v1_legacy_irq_handler, pcie);
+
+	legacy_irq_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX,
+						  &j721e_pcie_intx_domain_ops, pcie);
+	if (!legacy_irq_domain) {
+		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
+		return -EINVAL;
+	}
+	pcie->legacy_irq_domain = legacy_irq_domain;

Quoted 8 lines

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 200

Quoted 6 lines

drivers/pci/controller/cadence/pci-j721e.c +6 -0

+
+	for (i = 0; i < PCI_NUM_INTX; i++) {
+		reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_0);
+		reg |= INTx_EN(i);
+		j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_0, reg);
+	}

This should be moved to the irq_unmask() callback.

Should we also have a corresponding irq_mask()? Then would require us implement reference counting since legacy interrupts are shared.

Quoted 22 lines · Kishon Vijay Abraham I

Hi Marc,

Quoted 17 lines · Marc Zyngier

Quoted 11 lines · Kishon Vijay Abraham I

Add PCI legacy interrupt support for J721E. J721E has a single HW interrupt line for all the four legacy interrupts INTA/INTB/INTC/INTD. The HW interrupt line connected to GIC is a pulse interrupt whereas the legacy interrupts by definition is level interrupt. In order to provide level interrupt functionality to edge interrupt line, PCIe in J721E has provided IRQ_EOI register.

However due to Errata ID #i2094 ([1]), EOI feature is not enabled in HW and only a single pulse interrupt will be generated for every ASSERT_INTx/DEASSERT_INTx.

So my earlier remark stands. If you get a single edge, how do you handle a level that is still high after having handled the interrupt on hardware that has this bug?

Quoted 2 lines

Right, this hardware (J721E) has a bug but was fixed in J7200 (Patch 3/3 handles that).

But how do you make it work with J721E? Is it even worth supporting if (as I expect) it is unreliable?

Quoted 70 lines

Quoted 42 lines

Quoted 42 lines

[1] -> J721E DRA829/TDA4VM Processors Silicon Revision 1.1/1.0 SPRZ455A – DECEMBER 2020 – REVISED AUGUST 2021
(https://www.ti.com/lit/er/sprz455a/sprz455a.pdf)

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/pci/controller/cadence/pci-j721e.c | 85 ++++++++++++++++++++++
 1 file changed, 85 insertions(+)

drivers/pci/controller/cadence/pci-j721e.c +8 -0

index 2ec037c43bd5..c2e7a78dc31f 100644
@@ -29,6 +29,13 @@
 #define LINK_DOWN		BIT(1)
 #define J7200_LINK_DOWN		BIT(10)
 
+#define EOI_REG			0x10
+
+#define ENABLE_REG_SYS_0	0x100
+#define STATUS_REG_SYS_0	0x500
+#define STATUS_CLR_REG_SYS_0	0x700
+#define INTx_EN(num)		(1 << (num))
+
 #define J721E_PCIE_USER_CMD_STATUS	0x4
 #define LINK_TRAINING_ENABLE		BIT(0)
 

@@ -59,6 +66,7 @@ struct j721e_pcie {
 	void __iomem		*user_cfg_base;
 	void __iomem		*intd_cfg_base;
 	u32			linkdown_irq_regfield;
+	struct irq_domain	*legacy_irq_domain;
 };
 
 enum j721e_pcie_mode {

@@ -121,6 +129,79 @@ static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie)
 	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_2, reg);
 }
 

Quoted 16 lines

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 144

Quoted 13 lines

drivers/pci/controller/cadence/pci-j721e.c +13 -0

+static void j721e_pcie_v1_legacy_irq_handler(struct irq_desc *desc)
+{
+	struct j721e_pcie *pcie = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	int i, virq;
+	u32 reg;
+
+	chained_irq_enter(chip, desc);
+
+	for (i = 0; i < PCI_NUM_INTX; i++) {
+		reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_0);
+		if (!(reg & INTx_EN(i)))
+			continue;

Why do you need to perform multiple reads? Surely reg contains all the bits you need, doesn't it?

Right, will fix it up.

Quoted 7 lines

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 147

Quoted 4 lines

drivers/pci/controller/cadence/pci-j721e.c +3 -0

+
+		virq = irq_find_mapping(pcie->legacy_irq_domain, 3 - i);
+		generic_handle_irq(virq);

Please combine both lines into a single generic_handle_domain_irq() call.

Okay.

Quoted 1 line

Quoted 7 lines

Quoted 4 lines

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 148

Quoted 1 line

drivers/pci/controller/cadence/pci-j721e.c +1 -0

+		j721e_pcie_intd_writel(pcie, STATUS_CLR_REG_SYS_0, INTx_EN(i));

What is the purpose of this write? It feels like this should be a irq_eoi callback.

It's an IRQ ACK, since in this platform the level to edge is not implemented properly in HW.

An Ack for an edge interrupt would need to happen before you handle the interrupt, or you'd wrongly acknowledge edges that fire between the handling of the interrupt and the Ack, and that would never be handled.

If it really is an Ack, it needs to be moved *before* the handling, preferably in an irq_ack callback. Otherwise, it is an EOI, and it belongs to irq_eoi.

Quoted 61 lines

Quoted 11 lines

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 156

Quoted 9 lines

drivers/pci/controller/cadence/pci-j721e.c +8 -0

+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static int j721e_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);

An INTx interrupt is a level interrupt. Please use the corresponding flow.

Okay.

Quoted 47 lines

Quoted 35 lines

drivers/pci/controller/cadence/pci-j721e.c +34 -0

+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops j721e_pcie_intx_domain_ops = {
+	.map = j721e_pcie_intx_map,
+};
+
+static int j721e_pcie_config_legacy_irq(struct j721e_pcie *pcie)
+{
+	struct irq_domain *legacy_irq_domain;
+	struct device *dev = pcie->dev;
+	struct device_node *node = dev->of_node;
+	struct device_node *intc_node;
+	int irq, i;
+	u32 reg;
+
+	intc_node = of_get_child_by_name(node, "interrupt-controller");
+	if (!intc_node) {
+		dev_dbg(dev, "interrupt-controller node is absent. Legacy INTR not supported\n");
+		return 0;
+	}
+
+	irq = irq_of_parse_and_map(intc_node, 0);
+	if (!irq) {
+		dev_err(dev, "Failed to parse and map legacy irq\n");
+		return -EINVAL;
+	}
+	irq_set_chained_handler_and_data(irq, j721e_pcie_v1_legacy_irq_handler, pcie);
+
+	legacy_irq_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX,
+						  &j721e_pcie_intx_domain_ops, pcie);
+	if (!legacy_irq_domain) {

drivers/pci/controller/cadence/pci-j721e.c static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) · line 200

Quoted 10 lines

drivers/pci/controller/cadence/pci-j721e.c +10 -0

+		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
+		return -EINVAL;
+	}
+	pcie->legacy_irq_domain = legacy_irq_domain;
+
+	for (i = 0; i < PCI_NUM_INTX; i++) {
+		reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_0);
+		reg |= INTx_EN(i);
+		j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_0, reg);
+	}

This should be moved to the irq_unmask() callback.

Quoted 2 lines

Should we also have a corresponding irq_mask()? Then would require us implement reference counting since legacy interrupts are shared.

The core code should already deal with this, I expect. It isn't like shared interrupts are something new. And yes, you should have both mask and unmask.

Hi Marc,

Quoted 24 lines · Marc Zyngier

Quoted 22 lines · Kishon Vijay Abraham I

Hi Marc,

Quoted 17 lines · Marc Zyngier

Quoted 11 lines · Kishon Vijay Abraham I

Add PCI legacy interrupt support for J721E. J721E has a single HW interrupt line for all the four legacy interrupts INTA/INTB/INTC/INTD. The HW interrupt line connected to GIC is a pulse interrupt whereas the legacy interrupts by definition is level interrupt. In order to provide level interrupt functionality to edge interrupt line, PCIe in J721E has provided IRQ_EOI register.

However due to Errata ID #i2094 ([1]), EOI feature is not enabled in HW and only a single pulse interrupt will be generated for every ASSERT_INTx/DEASSERT_INTx.

So my earlier remark stands. If you get a single edge, how do you handle a level that is still high after having handled the interrupt on hardware that has this bug?

Quoted 5 lines

Quoted 2 lines

Right, this hardware (J721E) has a bug but was fixed in J7200 (Patch 3/3 handles that).

But how do you make it work with J721E? Is it even worth supporting if (as I expect) it is unreliable?

I've seen at-least the NVMe devices triggers the interrupts again and the data transfer completes. But I agree, this is unreliable.

Quoted 34 lines · Kishon Vijay Abraham I

Hi Marc,

Quoted 29 lines · Marc Zyngier

Quoted 18 lines · Kishon Vijay Abraham I

Hi Marc,

Quoted 14 lines · Marc Zyngier

Quoted 11 lines · Kishon Vijay Abraham I

Add PCI legacy interrupt support for J721E. J721E has a single HW interrupt line for all the four legacy interrupts INTA/INTB/INTC/INTD. The HW interrupt line connected to GIC is a pulse interrupt whereas the legacy interrupts by definition is level interrupt. In order to provide level interrupt functionality to edge interrupt line, PCIe in J721E has provided IRQ_EOI register.

However due to Errata ID #i2094 ([1]), EOI feature is not enabled in HW and only a single pulse interrupt will be generated for every ASSERT_INTx/DEASSERT_INTx.

Quoted 6 lines

Quoted 3 lines

So my earlier remark stands. If you get a single edge, how do you handle a level that is still high after having handled the interrupt on hardware that has this bug?

Right, this hardware (J721E) has a bug but was fixed in J7200 (Patch 3/3 handles that).

But how do you make it work with J721E? Is it even worth supporting if (as I expect) it is unreliable?

Quoted 2 lines

I've seen at-least the NVMe devices triggers the interrupts again and the data transfer completes. But I agree, this is unreliable.

Then I don't think you should add INTx support for this system. It is bound to be a support burden, and will reflect badly on the whole platform. Focusing on J7200 is probably the best thing to do.