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

[PATCH v2 0/3] 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.

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

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] -> http://lore.kernel.org/r/20210803074932.19820-1-kishon@ti.com

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

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

-- 
2.17.1

[PATCH v2 1/3] 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

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

[Marc Zyngier]

On Wed, 04 Aug 2021 14:29:10 +0100,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> 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>;

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?

Thanks,

	M.

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

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

[Kishon Vijay Abraham I]

Hi Marc,

On 04/08/21 8:35 pm, Marc Zyngier wrote:
> On Wed, 04 Aug 2021 14:29:10 +0100,
> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> 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>;
> 
> 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?

There is a EOI register which is used for re-triggering the interrupt.
That functionality is broken in J721E but is fixed in J7200 (the
following two patches in the series deals with that).

Thanks,
Kishon

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

[Rob Herring]

On Wed, Aug 04, 2021 at 06:59:10PM +0530, Kishon Vijay Abraham I wrote:
> 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":

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

Anything is allowed in the node?

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

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

[Kishon Vijay Abraham I]

Hi Rob,

On 13/08/21 10:47 pm, Rob Herring wrote:
> On Wed, Aug 04, 2021 at 06:59:10PM +0530, Kishon Vijay Abraham I wrote:
>> 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":
> 
> Not a pattern unless you meant for foo-interrupt-controller-bar to be 
> valid.
> 
> 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
> 
>> +    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
>>
>>

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

[Kishon Vijay Abraham I]

Hi Rob,

On 18/08/21 7:28 pm, Kishon Vijay Abraham I wrote:
> Hi Rob,
> 
> On 13/08/21 10:47 pm, Rob Herring wrote:
>> On Wed, Aug 04, 2021 at 06:59:10PM +0530, Kishon Vijay Abraham I wrote:
>>> 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":
>>
>> Not a pattern unless you meant for foo-interrupt-controller-bar to be 
>> valid.
>>
>> 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?

Thanks,
Kishon

> 
> Thanks
> Kishon
>>
>>> +    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
>>>
>>>

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

[Rob Herring]

On Wed, Sep 22, 2021 at 11:33 PM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
> Hi Rob,
>
> On 18/08/21 7:28 pm, Kishon Vijay Abraham I wrote:
> > Hi Rob,
> >
> > On 13/08/21 10:47 pm, Rob Herring wrote:
> >> On Wed, Aug 04, 2021 at 06:59:10PM +0530, Kishon Vijay Abraham I wrote:
> >>> 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":
> >>
> >> Not a pattern unless you meant for foo-interrupt-controller-bar to be
> >> valid.
> >>
> >> 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?

You don't. List the properties you are using. You need to define the
#interrupt-cells value for example. I'm sure there's already examples
in the tree doing this same child node.

Rob

[PATCH v2 2/3] PCI: j721e: Add PCI legacy interrupt support for J721E

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

[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 | 85 ++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 2ec037c43bd5..c2e7a78dc31f 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -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);
+	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;
+
+		virq = irq_find_mapping(pcie->legacy_irq_domain, 3 - i);
+		generic_handle_irq(virq);
+		j721e_pcie_intd_writel(pcie, STATUS_CLR_REG_SYS_0, INTx_EN(i));
+	}
+
+	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);
+	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;
+
+	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);
+	}
+
+	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 +514,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

Re: [PATCH v2 2/3] PCI: j721e: Add PCI legacy interrupt support for J721E

[Marc Zyngier]

On Wed, 04 Aug 2021 14:29:11 +0100,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> 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?

> 
> [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 | 85 ++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 2ec037c43bd5..c2e7a78dc31f 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -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);
> +	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?

> +
> +		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.

> +		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.

> +	}
> +
> +	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.

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

Thanks,

	M.

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

Re: [PATCH v2 2/3] PCI: j721e: Add PCI legacy interrupt support for J721E

[Kishon Vijay Abraham I]

Hi Marc,

On 04/08/21 8:43 pm, Marc Zyngier wrote:
> On Wed, 04 Aug 2021 14:29:11 +0100,
> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> 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?

Right, this hardware (J721E) has a bug but was fixed in J7200 (Patch 3/3
handles that).
> 
>>
>> [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 | 85 ++++++++++++++++++++++
>>  1 file changed, 85 insertions(+)
>>
>> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
>> index 2ec037c43bd5..c2e7a78dc31f 100644
>> --- a/drivers/pci/controller/cadence/pci-j721e.c
>> +++ b/drivers/pci/controller/cadence/pci-j721e.c
>> @@ -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);
>> +	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.
> 
>> +
>> +		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.
> 
>> +		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.
> 
>> +	}
>> +
>> +	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.
> 
>> +	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;
>> +
>> +	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.

Thanks,
Kishon

Re: [PATCH v2 2/3] PCI: j721e: Add PCI legacy interrupt support for J721E

[Marc Zyngier]

On Mon, 09 Aug 2021 05:50:10 +0100,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> Hi Marc,
> 
> On 04/08/21 8:43 pm, Marc Zyngier wrote:
> > On Wed, 04 Aug 2021 14:29:11 +0100,
> > Kishon Vijay Abraham I <kishon@ti.com> wrote:
> >>
> >> 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?
> 
> 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?

> > 
> >>
> >> [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 | 85 ++++++++++++++++++++++
> >>  1 file changed, 85 insertions(+)
> >>
> >> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> >> index 2ec037c43bd5..c2e7a78dc31f 100644
> >> --- a/drivers/pci/controller/cadence/pci-j721e.c
> >> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> >> @@ -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);
> >> +	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.
> > 
> >> +
> >> +		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.
> > 
> >> +		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.

> > 
> >> +	}
> >> +
> >> +	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.
> > 
> >> +	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;
> >> +
> >> +	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.

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.

Thanks,

	M.

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

Re: [PATCH v2 2/3] PCI: j721e: Add PCI legacy interrupt support for J721E

[Kishon Vijay Abraham I]

Hi Marc,

On 09/08/21 3:09 pm, Marc Zyngier wrote:
> On Mon, 09 Aug 2021 05:50:10 +0100,
> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Hi Marc,
>>
>> On 04/08/21 8:43 pm, Marc Zyngier wrote:
>>> On Wed, 04 Aug 2021 14:29:11 +0100,
>>> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>>
>>>> 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?
>>
>> 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.
> 
>>>
>>>>
>>>> [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 | 85 ++++++++++++++++++++++
>>>>  1 file changed, 85 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
>>>> index 2ec037c43bd5..c2e7a78dc31f 100644
>>>> --- a/drivers/pci/controller/cadence/pci-j721e.c
>>>> +++ b/drivers/pci/controller/cadence/pci-j721e.c
>>>> @@ -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);
>>>> +	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.
>>>
>>>> +
>>>> +		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.
>>>
>>>> +		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.
> 
>>>
>>>> +	}
>>>> +
>>>> +	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.
>>>
>>>> +	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;
>>>> +
>>>> +	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.
> 
> 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.

Thanks for clarifying.

Best Regards,
Kishon

Re: [PATCH v2 2/3] PCI: j721e: Add PCI legacy interrupt support for J721E

[Marc Zyngier]

On Mon, 09 Aug 2021 15:58:38 +0100,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> Hi Marc,
> 
> On 09/08/21 3:09 pm, Marc Zyngier wrote:
> > On Mon, 09 Aug 2021 05:50:10 +0100,
> > Kishon Vijay Abraham I <kishon@ti.com> wrote:
> >>
> >> Hi Marc,
> >>
> >> On 04/08/21 8:43 pm, Marc Zyngier wrote:
> >>> On Wed, 04 Aug 2021 14:29:11 +0100,
> >>> Kishon Vijay Abraham I <kishon@ti.com> wrote:
> >>>>
> >>>> 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?
> >>
> >> 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.

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.

Thanks,

	M.

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

Re: [PATCH v2 2/3] PCI: j721e: Add PCI legacy interrupt support for J721E

[Kishon Vijay Abraham I]

Hi Marc,

On 10/08/21 6:03 pm, Marc Zyngier wrote:
> On Mon, 09 Aug 2021 15:58:38 +0100,
> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Hi Marc,
>>
>> On 09/08/21 3:09 pm, Marc Zyngier wrote:
>>> On Mon, 09 Aug 2021 05:50:10 +0100,
>>> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>>
>>>> Hi Marc,
>>>>
>>>> On 04/08/21 8:43 pm, Marc Zyngier wrote:
>>>>> On Wed, 04 Aug 2021 14:29:11 +0100,
>>>>> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>>>>
>>>>>> 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?
>>>>
>>>> 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.
> 
> 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.

Okay, will drop this patch from the series and have INTx support only
for J7200.

Thanks
Kishon

[PATCH v2 3/3] 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. (Note that the errata in J721E where EOI is not
implemented is fixed in J7200).

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pci-j721e.c | 77 ++++++++++++++++++++--
 1 file changed, 71 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index c2e7a78dc31f..be8464232be0 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -36,12 +36,24 @@
 #define STATUS_CLR_REG_SYS_0	0x700
 #define INTx_EN(num)		(1 << (num))
 
+#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,
@@ -67,6 +79,7 @@ struct j721e_pcie {
 	void __iomem		*intd_cfg_base;
 	u32			linkdown_irq_regfield;
 	struct irq_domain	*legacy_irq_domain;
+	unsigned int		is_intc_v1:1;
 };
 
 enum j721e_pcie_mode {
@@ -76,6 +89,7 @@ enum j721e_pcie_mode {
 
 struct j721e_pcie_data {
 	enum j721e_pcie_mode	mode;
+	unsigned int		is_intc_v1:1;
 	unsigned int		quirk_retrain_flag:1;
 	unsigned int		quirk_detect_quiet_flag:1;
 	u32			linkdown_irq_regfield;
@@ -129,6 +143,39 @@ 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);
+
+	for (i = 0; i < PCI_NUM_INTX; i++) {
+		reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_1);
+		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);
+}
+
+struct irq_chip j721e_pcie_irq_chip = {
+	.name		= "J721E-PCIE-INTX",
+	.irq_eoi	= j721e_pcie_irq_eoi,
+};
+
 static void j721e_pcie_v1_legacy_irq_handler(struct irq_desc *desc)
 {
 	struct j721e_pcie *pcie = irq_desc_get_handler_data(desc);
@@ -153,8 +200,14 @@ static void j721e_pcie_v1_legacy_irq_handler(struct irq_desc *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);
-	irq_set_chip_data(irq, domain->host_data);
+	struct j721e_pcie *pcie = domain->host_data;
+
+	if (pcie->is_intc_v1)
+		irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+	else
+		irq_set_chip_and_handler(irq, &j721e_pcie_irq_chip, handle_fasteoi_irq);
+
+	irq_set_chip_data(irq, pcie);
 
 	return 0;
 }
@@ -183,7 +236,11 @@ static int j721e_pcie_config_legacy_irq(struct j721e_pcie *pcie)
 		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);
+
+	if (pcie->is_intc_v1)
+		irq_set_chained_handler_and_data(irq, j721e_pcie_v1_legacy_irq_handler, pcie);
+	else
+		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);
@@ -194,9 +251,15 @@ static int j721e_pcie_config_legacy_irq(struct j721e_pcie *pcie)
 	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);
+		if (pcie->is_intc_v1) {
+			reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_0);
+			reg |= INTx_EN(i);
+			j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_0, reg);
+		} else {
+			reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_1);
+			reg |= SYS1_INTx_EN(i);
+			j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_1, reg);
+		}
 	}
 
 	return 0;
@@ -372,6 +435,7 @@ static const struct j721e_pcie_data j721e_pcie_rc_data = {
 	.quirk_retrain_flag = true,
 	.byte_access_allowed = false,
 	.linkdown_irq_regfield = LINK_DOWN,
+	.is_intc_v1 = true,
 };
 
 static const struct j721e_pcie_data j721e_pcie_ep_data = {
@@ -514,6 +578,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 			goto err_get_sync;
 		}
 
+		pcie->is_intc_v1 = data->is_intc_v1;
 		ret = j721e_pcie_config_legacy_irq(pcie);
 		if (ret < 0)
 			goto err_get_sync;
-- 
2.17.1