[PATCH 0/2] PCI: Add legacy interrupt support in pci-j721e

[PATCH 0/2] PCI: Add legacy interrupt support in pci-j721e

[Kishon Vijay Abraham I]

Patch series adds support for legacy interrupt in pci-j721e. There are
two HW implementations of legacy interrupt controller, one specific to
J721E and the other for J7200/AM64.

In both these implementations, the legacy interrupt is connect to pulse
interrupt of GIC and level to pulse is handled by configuring EOI
register. EOI to convert level to pulse is broken in J721E due to an
errata but is functional in J7200. Hence legacy interrupt support is not
added for J721E

v1 of the patch series can be found @ [1]
Patch series is created on top of [2]

Changes from v2:
1) Dropped legacy interrupt support for J721e
2) Enable legacy interrupts registers in .irq_enable callback

Changes from v1:
1) Only the legacy interrupt specific part is sent as part of this
series. Rest are split and sent as a separate series [2]
2) Created irq_chip for legacy interrupt and used it's ops for enabling,
disabling the interrupts.

[1] -> http://lore.kernel.org/r/20210325090936.9306-1-kishon@ti.com
[2] -> https://lore.kernel.org/linux-omap/20210811123336.31357-1-kishon@ti.com/

Kishon Vijay Abraham I (2):
  dt-bindings: PCI: ti,j721e: Add bindings to specify legacy interrupts
  PCI: j721e: Add PCI legacy interrupt support for J7200

 .../bindings/pci/ti,j721e-pci-host.yaml       |  15 +++
 drivers/pci/controller/cadence/pci-j721e.c    | 119 ++++++++++++++++++
 2 files changed, 134 insertions(+)

-- 
2.17.1

[PATCH 0/2] PCI: Add legacy interrupt support in pci-j721e

[Kishon Vijay Abraham I]

Patch series adds support for legacy interrupt in pci-j721e. There are
two HW implementations of legacy interrupt controller, one specific to
J721E and the other for J7200/AM64.

In both these implementations, the legacy interrupt is connect to pulse
interrupt of GIC and level to pulse is handled by configuring EOI
register. EOI to convert level to pulse is broken in J721E due to an
errata but is functional in J7200. Hence legacy interrupt support is not
added for J721E

v1 of the patch series can be found @ [1]
Patch series is created on top of [2]

Changes from v2:
1) Dropped legacy interrupt support for J721e
2) Enable legacy interrupts registers in .irq_enable callback

Changes from v1:
1) Only the legacy interrupt specific part is sent as part of this
series. Rest are split and sent as a separate series [2]
2) Created irq_chip for legacy interrupt and used it's ops for enabling,
disabling the interrupts.

[1] -> http://lore.kernel.org/r/20210325090936.9306-1-kishon@ti.com
[2] -> https://lore.kernel.org/linux-omap/20210811123336.31357-1-kishon@ti.com/

Kishon Vijay Abraham I (2):
  dt-bindings: PCI: ti,j721e: Add bindings to specify legacy interrupts
  PCI: j721e: Add PCI legacy interrupt support for J7200

 .../bindings/pci/ti,j721e-pci-host.yaml       |  15 +++
 drivers/pci/controller/cadence/pci-j721e.c    | 119 ++++++++++++++++++
 2 files changed, 134 insertions(+)

-- 
2.17.1


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

[PATCH 1/2] dt-bindings: PCI: ti,j721e: Add bindings to specify legacy interrupts

[Kishon Vijay Abraham I]

Add bindings to specify interrupt controller for legacy interrupts.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
index cc900202df29..f461d7b4c0cc 100644
--- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
+++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
@@ -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>
 
@@ -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>;
+            };
         };
     };
-- 
2.17.1

[PATCH 1/2] dt-bindings: PCI: ti, j721e: Add bindings to specify legacy interrupts

[Kishon Vijay Abraham I]

Add bindings to specify interrupt controller for legacy interrupts.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
index cc900202df29..f461d7b4c0cc 100644
--- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
+++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
@@ -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>
 
@@ -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>;
+            };
         };
     };
-- 
2.17.1


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

[PATCH 2/2] PCI: j721e: Add PCI legacy interrupt support for J7200

[Kishon Vijay Abraham I]

Add PCI legacy interrupt support for J7200. J7200 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 J7200 has provided USER_EOI_REG register. When the SW writes to
USER_EOI_REG register after handling the interrupt, the IP checks the
state of legacy interrupt and re-triggers pulse interrupt invoking
the handler again.

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

[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 <kishon@ti.com>
---
 drivers/pci/controller/cadence/pci-j721e.c | 119 +++++++++++++++++++++
 1 file changed, 119 insertions(+)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index ffb176d288cd..4e786d6b89e0 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -29,12 +29,24 @@
 #define LINK_DOWN		BIT(1)
 #define J7200_LINK_DOWN		BIT(10)
 
+#define ENABLE_REG_SYS_1	0x104
+#define STATUS_REG_SYS_1	0x504
+#define SYS1_INTx_EN(num)	(1 << (22 + (num)))
+
 #define J721E_PCIE_USER_CMD_STATUS	0x4
 #define LINK_TRAINING_ENABLE		BIT(0)
 
 #define J721E_PCIE_USER_LINKSTATUS	0x14
 #define LINK_STATUS			GENMASK(1, 0)
 
+#define USER_EOI_REG		0xC8
+enum eoi_reg {
+	EOI_DOWNSTREAM_INTERRUPT,
+	EOI_FLR_INTERRUPT,
+	EOI_LEGACY_INTERRUPT,
+	EOI_POWER_STATE_INTERRUPT,
+};
+
 enum link_status {
 	NO_RECEIVERS_DETECTED,
 	LINK_TRAINING_IN_PROGRESS,
@@ -59,6 +71,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 +134,108 @@ 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_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);
+
+	reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_1);
+	for (i = 0; i < PCI_NUM_INTX; i++) {
+		if (!(reg & SYS1_INTx_EN(i)))
+			continue;
+
+		virq = irq_find_mapping(pcie->legacy_irq_domain, i);
+		generic_handle_irq(virq);
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void j721e_pcie_irq_eoi(struct irq_data *data)
+{
+	struct j721e_pcie *pcie = irq_data_get_irq_chip_data(data);
+
+	j721e_pcie_user_writel(pcie, USER_EOI_REG, EOI_LEGACY_INTERRUPT);
+}
+
+static void j721e_pcie_irq_enable(struct irq_data *data)
+{
+	struct j721e_pcie *pcie = irq_data_get_irq_chip_data(data);
+	u32 reg;
+
+	reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_1);
+	reg |= SYS1_INTx_EN(irqd_to_hwirq(data));
+	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_1, reg);
+}
+
+static void j721e_pcie_irq_disable(struct irq_data *data)
+{
+	struct j721e_pcie *pcie = irq_data_get_irq_chip_data(data);
+	u32 reg;
+
+	reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_1);
+	reg &= ~SYS1_INTx_EN(irqd_to_hwirq(data));
+	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_1, reg);
+}
+
+struct irq_chip j721e_pcie_irq_chip = {
+	.name		= "J721E-PCIE-INTX",
+	.irq_eoi	= j721e_pcie_irq_eoi,
+	.irq_enable	= j721e_pcie_irq_enable,
+	.irq_disable	= j721e_pcie_irq_disable,
+};
+
+static int j721e_pcie_intx_map(struct irq_domain *domain, unsigned int irq, irq_hw_number_t hwirq)
+{
+	struct j721e_pcie *pcie = domain->host_data;
+
+	irq_set_chip_and_handler(irq, &j721e_pcie_irq_chip, handle_fasteoi_irq);
+	irq_set_chip_data(irq, pcie);
+
+	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;
+
+	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_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;
+
+	return 0;
+}
+
 static int j721e_pcie_start_link(struct cdns_pcie *cdns_pcie)
 {
 	struct j721e_pcie *pcie = dev_get_drvdata(cdns_pcie->dev);
@@ -433,6 +548,10 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 			goto err_get_sync;
 		}
 
+		ret = j721e_pcie_config_legacy_irq(pcie);
+		if (ret < 0)
+			goto err_get_sync;
+
 		bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
 		if (!bridge) {
 			ret = -ENOMEM;
-- 
2.17.1

[PATCH 2/2] PCI: j721e: Add PCI legacy interrupt support for J7200

[Kishon Vijay Abraham I]

Add PCI legacy interrupt support for J7200. J7200 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 J7200 has provided USER_EOI_REG register. When the SW writes to
USER_EOI_REG register after handling the interrupt, the IP checks the
state of legacy interrupt and re-triggers pulse interrupt invoking
the handler again.

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

[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 <kishon@ti.com>
---
 drivers/pci/controller/cadence/pci-j721e.c | 119 +++++++++++++++++++++
 1 file changed, 119 insertions(+)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index ffb176d288cd..4e786d6b89e0 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -29,12 +29,24 @@
 #define LINK_DOWN		BIT(1)
 #define J7200_LINK_DOWN		BIT(10)
 
+#define ENABLE_REG_SYS_1	0x104
+#define STATUS_REG_SYS_1	0x504
+#define SYS1_INTx_EN(num)	(1 << (22 + (num)))
+
 #define J721E_PCIE_USER_CMD_STATUS	0x4
 #define LINK_TRAINING_ENABLE		BIT(0)
 
 #define J721E_PCIE_USER_LINKSTATUS	0x14
 #define LINK_STATUS			GENMASK(1, 0)
 
+#define USER_EOI_REG		0xC8
+enum eoi_reg {
+	EOI_DOWNSTREAM_INTERRUPT,
+	EOI_FLR_INTERRUPT,
+	EOI_LEGACY_INTERRUPT,
+	EOI_POWER_STATE_INTERRUPT,
+};
+
 enum link_status {
 	NO_RECEIVERS_DETECTED,
 	LINK_TRAINING_IN_PROGRESS,
@@ -59,6 +71,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 +134,108 @@ 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_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);
+
+	reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_1);
+	for (i = 0; i < PCI_NUM_INTX; i++) {
+		if (!(reg & SYS1_INTx_EN(i)))
+			continue;
+
+		virq = irq_find_mapping(pcie->legacy_irq_domain, i);
+		generic_handle_irq(virq);
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void j721e_pcie_irq_eoi(struct irq_data *data)
+{
+	struct j721e_pcie *pcie = irq_data_get_irq_chip_data(data);
+
+	j721e_pcie_user_writel(pcie, USER_EOI_REG, EOI_LEGACY_INTERRUPT);
+}
+
+static void j721e_pcie_irq_enable(struct irq_data *data)
+{
+	struct j721e_pcie *pcie = irq_data_get_irq_chip_data(data);
+	u32 reg;
+
+	reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_1);
+	reg |= SYS1_INTx_EN(irqd_to_hwirq(data));
+	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_1, reg);
+}
+
+static void j721e_pcie_irq_disable(struct irq_data *data)
+{
+	struct j721e_pcie *pcie = irq_data_get_irq_chip_data(data);
+	u32 reg;
+
+	reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_1);
+	reg &= ~SYS1_INTx_EN(irqd_to_hwirq(data));
+	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_1, reg);
+}
+
+struct irq_chip j721e_pcie_irq_chip = {
+	.name		= "J721E-PCIE-INTX",
+	.irq_eoi	= j721e_pcie_irq_eoi,
+	.irq_enable	= j721e_pcie_irq_enable,
+	.irq_disable	= j721e_pcie_irq_disable,
+};
+
+static int j721e_pcie_intx_map(struct irq_domain *domain, unsigned int irq, irq_hw_number_t hwirq)
+{
+	struct j721e_pcie *pcie = domain->host_data;
+
+	irq_set_chip_and_handler(irq, &j721e_pcie_irq_chip, handle_fasteoi_irq);
+	irq_set_chip_data(irq, pcie);
+
+	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;
+
+	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_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;
+
+	return 0;
+}
+
 static int j721e_pcie_start_link(struct cdns_pcie *cdns_pcie)
 {
 	struct j721e_pcie *pcie = dev_get_drvdata(cdns_pcie->dev);
@@ -433,6 +548,10 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 			goto err_get_sync;
 		}
 
+		ret = j721e_pcie_config_legacy_irq(pcie);
+		if (ret < 0)
+			goto err_get_sync;
+
 		bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
 		if (!bridge) {
 			ret = -ENOMEM;
-- 
2.17.1


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

Re: [PATCH 2/2] PCI: j721e: Add PCI legacy interrupt support for J7200

[Marc Zyngier]

On Wed, 11 Aug 2021 13:38:46 +0100,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> Add PCI legacy interrupt support for J7200. J7200 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 J7200 has provided USER_EOI_REG register. When the SW writes to
> USER_EOI_REG register after handling the interrupt, the IP checks the
> state of legacy interrupt and re-triggers pulse interrupt invoking
> the handler again.
> 
> Due to Errata ID #i2094 ([1]), EOI feature is not enabled in J721E
> and only a single pulse interrupt will be generated for every
> ASSERT_INTx/DEASSERT_INTx. Hence legacy interrupt is not enabled in
> J721E.

How do you prevent this from being enabled on an affected platform?
Just by virtue of not having the interrupt-controller property in DT?
In which case, it may be useful to clarify this in the DT binding and
say that it is only valid on J7200.

> 
> [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 <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pci-j721e.c | 119 +++++++++++++++++++++
>  1 file changed, 119 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index ffb176d288cd..4e786d6b89e0 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -29,12 +29,24 @@
>  #define LINK_DOWN		BIT(1)
>  #define J7200_LINK_DOWN		BIT(10)
>  
> +#define ENABLE_REG_SYS_1	0x104
> +#define STATUS_REG_SYS_1	0x504
> +#define SYS1_INTx_EN(num)	(1 << (22 + (num)))
> +
>  #define J721E_PCIE_USER_CMD_STATUS	0x4
>  #define LINK_TRAINING_ENABLE		BIT(0)
>  
>  #define J721E_PCIE_USER_LINKSTATUS	0x14
>  #define LINK_STATUS			GENMASK(1, 0)
>  
> +#define USER_EOI_REG		0xC8
> +enum eoi_reg {
> +	EOI_DOWNSTREAM_INTERRUPT,
> +	EOI_FLR_INTERRUPT,
> +	EOI_LEGACY_INTERRUPT,
> +	EOI_POWER_STATE_INTERRUPT,
> +};
> +
>  enum link_status {
>  	NO_RECEIVERS_DETECTED,
>  	LINK_TRAINING_IN_PROGRESS,
> @@ -59,6 +71,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 +134,108 @@ 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_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);
> +
> +	reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_1);
> +	for (i = 0; i < PCI_NUM_INTX; i++) {
> +		if (!(reg & SYS1_INTx_EN(i)))
> +			continue;
> +
> +		virq = irq_find_mapping(pcie->legacy_irq_domain, i);
> +		generic_handle_irq(virq);

Please use generic_handle_domain_irq().

> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void j721e_pcie_irq_eoi(struct irq_data *data)
> +{
> +	struct j721e_pcie *pcie = irq_data_get_irq_chip_data(data);
> +
> +	j721e_pcie_user_writel(pcie, USER_EOI_REG, EOI_LEGACY_INTERRUPT);
> +}
> +
> +static void j721e_pcie_irq_enable(struct irq_data *data)
> +{
> +	struct j721e_pcie *pcie = irq_data_get_irq_chip_data(data);
> +	u32 reg;
> +
> +	reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_1);
> +	reg |= SYS1_INTx_EN(irqd_to_hwirq(data));
> +	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_1, reg);

RMW of a register shared between interrupts without a lock. It will
eventually end badly.

> +}
> +
> +static void j721e_pcie_irq_disable(struct irq_data *data)
> +{
> +	struct j721e_pcie *pcie = irq_data_get_irq_chip_data(data);
> +	u32 reg;
> +
> +	reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_1);
> +	reg &= ~SYS1_INTx_EN(irqd_to_hwirq(data));
> +	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_1, reg);

Same thing.

> +}
> +
> +struct irq_chip j721e_pcie_irq_chip = {
> +	.name		= "J721E-PCIE-INTX",
> +	.irq_eoi	= j721e_pcie_irq_eoi,
> +	.irq_enable	= j721e_pcie_irq_enable,
> +	.irq_disable	= j721e_pcie_irq_disable,
> +};
> +
> +static int j721e_pcie_intx_map(struct irq_domain *domain, unsigned int irq, irq_hw_number_t hwirq)
> +{
> +	struct j721e_pcie *pcie = domain->host_data;
> +
> +	irq_set_chip_and_handler(irq, &j721e_pcie_irq_chip, handle_fasteoi_irq);
> +	irq_set_chip_data(irq, pcie);
> +
> +	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;
> +
> +	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_legacy_irq_handler, pcie);

At this stage, you now have enabled the mux interrupt, and it can fire
at will (who knows what state the enabled bits are, given that you
don't initialise them?). However, you still haven't allocated the
domain. What could possibly go wrong?

Please initialise all the interrupts to their disabled state, and only
register the handler once all the data structures have been populated.

> +
> +	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;
> +
> +	return 0;
> +}
> +
>  static int j721e_pcie_start_link(struct cdns_pcie *cdns_pcie)
>  {
>  	struct j721e_pcie *pcie = dev_get_drvdata(cdns_pcie->dev);
> @@ -433,6 +548,10 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>  			goto err_get_sync;
>  		}
>  
> +		ret = j721e_pcie_config_legacy_irq(pcie);
> +		if (ret < 0)
> +			goto err_get_sync;
> +
>  		bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
>  		if (!bridge) {
>  			ret = -ENOMEM;

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 2/2] PCI: j721e: Add PCI legacy interrupt support for J7200

[Marc Zyngier]

On Wed, 11 Aug 2021 13:38:46 +0100,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> Add PCI legacy interrupt support for J7200. J7200 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 J7200 has provided USER_EOI_REG register. When the SW writes to
> USER_EOI_REG register after handling the interrupt, the IP checks the
> state of legacy interrupt and re-triggers pulse interrupt invoking
> the handler again.
> 
> Due to Errata ID #i2094 ([1]), EOI feature is not enabled in J721E
> and only a single pulse interrupt will be generated for every
> ASSERT_INTx/DEASSERT_INTx. Hence legacy interrupt is not enabled in
> J721E.

How do you prevent this from being enabled on an affected platform?
Just by virtue of not having the interrupt-controller property in DT?
In which case, it may be useful to clarify this in the DT binding and
say that it is only valid on J7200.

> 
> [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 <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pci-j721e.c | 119 +++++++++++++++++++++
>  1 file changed, 119 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index ffb176d288cd..4e786d6b89e0 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -29,12 +29,24 @@
>  #define LINK_DOWN		BIT(1)
>  #define J7200_LINK_DOWN		BIT(10)
>  
> +#define ENABLE_REG_SYS_1	0x104
> +#define STATUS_REG_SYS_1	0x504
> +#define SYS1_INTx_EN(num)	(1 << (22 + (num)))
> +
>  #define J721E_PCIE_USER_CMD_STATUS	0x4
>  #define LINK_TRAINING_ENABLE		BIT(0)
>  
>  #define J721E_PCIE_USER_LINKSTATUS	0x14
>  #define LINK_STATUS			GENMASK(1, 0)
>  
> +#define USER_EOI_REG		0xC8
> +enum eoi_reg {
> +	EOI_DOWNSTREAM_INTERRUPT,
> +	EOI_FLR_INTERRUPT,
> +	EOI_LEGACY_INTERRUPT,
> +	EOI_POWER_STATE_INTERRUPT,
> +};
> +
>  enum link_status {
>  	NO_RECEIVERS_DETECTED,
>  	LINK_TRAINING_IN_PROGRESS,
> @@ -59,6 +71,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 +134,108 @@ 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_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);
> +
> +	reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_1);
> +	for (i = 0; i < PCI_NUM_INTX; i++) {
> +		if (!(reg & SYS1_INTx_EN(i)))
> +			continue;
> +
> +		virq = irq_find_mapping(pcie->legacy_irq_domain, i);
> +		generic_handle_irq(virq);

Please use generic_handle_domain_irq().

> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void j721e_pcie_irq_eoi(struct irq_data *data)
> +{
> +	struct j721e_pcie *pcie = irq_data_get_irq_chip_data(data);
> +
> +	j721e_pcie_user_writel(pcie, USER_EOI_REG, EOI_LEGACY_INTERRUPT);
> +}
> +
> +static void j721e_pcie_irq_enable(struct irq_data *data)
> +{
> +	struct j721e_pcie *pcie = irq_data_get_irq_chip_data(data);
> +	u32 reg;
> +
> +	reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_1);
> +	reg |= SYS1_INTx_EN(irqd_to_hwirq(data));
> +	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_1, reg);

RMW of a register shared between interrupts without a lock. It will
eventually end badly.

> +}
> +
> +static void j721e_pcie_irq_disable(struct irq_data *data)
> +{
> +	struct j721e_pcie *pcie = irq_data_get_irq_chip_data(data);
> +	u32 reg;
> +
> +	reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_1);
> +	reg &= ~SYS1_INTx_EN(irqd_to_hwirq(data));
> +	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_1, reg);

Same thing.

> +}
> +
> +struct irq_chip j721e_pcie_irq_chip = {
> +	.name		= "J721E-PCIE-INTX",
> +	.irq_eoi	= j721e_pcie_irq_eoi,
> +	.irq_enable	= j721e_pcie_irq_enable,
> +	.irq_disable	= j721e_pcie_irq_disable,
> +};
> +
> +static int j721e_pcie_intx_map(struct irq_domain *domain, unsigned int irq, irq_hw_number_t hwirq)
> +{
> +	struct j721e_pcie *pcie = domain->host_data;
> +
> +	irq_set_chip_and_handler(irq, &j721e_pcie_irq_chip, handle_fasteoi_irq);
> +	irq_set_chip_data(irq, pcie);
> +
> +	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;
> +
> +	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_legacy_irq_handler, pcie);

At this stage, you now have enabled the mux interrupt, and it can fire
at will (who knows what state the enabled bits are, given that you
don't initialise them?). However, you still haven't allocated the
domain. What could possibly go wrong?

Please initialise all the interrupts to their disabled state, and only
register the handler once all the data structures have been populated.

> +
> +	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;
> +
> +	return 0;
> +}
> +
>  static int j721e_pcie_start_link(struct cdns_pcie *cdns_pcie)
>  {
>  	struct j721e_pcie *pcie = dev_get_drvdata(cdns_pcie->dev);
> @@ -433,6 +548,10 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>  			goto err_get_sync;
>  		}
>  
> +		ret = j721e_pcie_config_legacy_irq(pcie);
> +		if (ret < 0)
> +			goto err_get_sync;
> +
>  		bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
>  		if (!bridge) {
>  			ret = -ENOMEM;

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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