[PATCH 0/6] PCI: Add legacy interrupt support in Keystone

[PATCH 0/6] PCI: Add legacy interrupt support in Keystone

[Kishon Vijay Abraham I]

Keystone driver is used by K2G and AM65 and the interrupt handling of
both of them is different. Add support to handle legacy interrupt for
both K2G and AM65 here.

Some discussions regarding this was already done here [1] and it was
around having pulse interrupt for legacy interrupt.

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

Patch series also includes converting AM65 binding to YAML and an
errata applicable for i2037.

[1] -> https://lore.kernel.org/linux-arm-kernel/20190221101518.22604-4-kishon@ti.com/

Kishon Vijay Abraham I (6):
  dt-bindings: PCI: ti,am65: Add PCIe host mode dt-bindings for TI's
    AM65 SoC
  dt-bindings: PCI: ti,am65: Add PCIe endpoint mode dt-bindings for TI's
    AM65 SoC
  irqdomain: Export of_phandle_args_to_fwspec()
  PCI: keystone: Convert to using hierarchy domain for legacy interrupts
  PCI: keystone: Add PCI legacy interrupt support for AM654
  PCI: keystone: Add workaround for Errata #i2037 (AM65x SR 1.0)

 .../bindings/pci/ti,am65-pci-ep.yaml          |  80 ++++
 .../bindings/pci/ti,am65-pci-host.yaml        | 111 ++++++
 drivers/pci/controller/dwc/pci-keystone.c     | 343 +++++++++++++-----
 include/linux/irqdomain.h                     |   2 +
 kernel/irq/irqdomain.c                        |   6 +-
 5 files changed, 440 insertions(+), 102 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/ti,am65-pci-ep.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml

-- 
2.17.1

[PATCH 1/6] dt-bindings: PCI: ti,am65: Add PCIe host mode dt-bindings for TI's AM65 SoC

[Kishon Vijay Abraham I]

Add PCIe host mode dt-bindings for TI's AM65 SoC.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../bindings/pci/ti,am65-pci-host.yaml        | 111 ++++++++++++++++++
 1 file changed, 111 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml

diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
new file mode 100644
index 000000000000..b77e492886fa
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
@@ -0,0 +1,111 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2021 Texas Instruments Incorporated - http://www.ti.com/
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: TI AM65 PCI Host
+
+maintainers:
+  - Kishon Vijay Abraham I <kishon@ti.com>
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+
+properties:
+  compatible:
+    enum:
+      - ti,am654-pcie-rc
+
+  reg:
+    maxItems: 4
+
+  reg-names:
+    items:
+      - const: app
+      - const: dbics
+      - const: config
+      - const: atu
+
+  power-domains:
+    maxItems: 1
+
+  ti,syscon-pcie-id:
+    description: Phandle to the SYSCON entry required for getting PCIe device/vendor ID
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+  ti,syscon-pcie-mode:
+    description: Phandle to the SYSCON entry required for configuring PCIe in RC or EP mode.
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+  msi-map: true
+
+  dma-coherent: true
+
+patternProperties:
+  "interrupt-controller":
+    type: object
+    description: interrupt controller to handle legacy interrupts.
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - max-link-speed
+  - num-lanes
+  - power-domains
+  - ti,syscon-pcie-id
+  - ti,syscon-pcie-mode
+  - msi-map
+  - ranges
+  - reset-gpios
+  - phys
+  - phy-names
+  - dma-coherent
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pcie0_rc: pcie@5500000 {
+                compatible = "ti,am654-pcie-rc";
+                reg =  <0x0 0x5500000 0x0 0x1000>,
+                       <0x0 0x5501000 0x0 0x1000>,
+                       <0x0 0x10000000 0x0 0x2000>,
+                       <0x0 0x5506000 0x0 0x1000>;
+                reg-names = "app", "dbics", "config", "atu";
+                power-domains = <&k3_pds 120 TI_SCI_PD_EXCLUSIVE>;
+                #address-cells = <3>;
+                #size-cells = <2>;
+                ranges = <0x81000000 0 0          0x0 0x10020000 0 0x00010000>,
+                         <0x82000000 0 0x10030000 0x0 0x10030000 0 0x07FD0000>;
+                ti,syscon-pcie-id = <&pcie_devid>;
+                ti,syscon-pcie-mode = <&pcie0_mode>;
+                bus-range = <0x0 0xff>;
+                num-viewport = <16>;
+                max-link-speed = <2>;
+                dma-coherent;
+                interrupts = <GIC_SPI 340 IRQ_TYPE_EDGE_RISING>;
+                msi-map = <0x0 &gic_its 0x0 0x10000>;
+                #interrupt-cells = <1>;
+                interrupt-map-mask = <0 0 0 7>;
+                interrupt-map = <0 0 0 1 &pcie0_intc 0>, /* INT A */
+                                <0 0 0 2 &pcie0_intc 0>, /* INT B */
+                                <0 0 0 3 &pcie0_intc 0>, /* INT C */
+                                <0 0 0 4 &pcie0_intc 0>; /* INT D */
+
+                pcie0_intc: interrupt-controller {
+                        interrupt-controller;
+                        #interrupt-cells = <1>;
+                        interrupt-parent = <&gic500>;
+                        interrupts = <GIC_SPI 328 IRQ_TYPE_EDGE_RISING>;
+                };
+        };
-- 
2.17.1

Re: [PATCH 1/6] dt-bindings: PCI: ti, am65: Add PCIe host mode dt-bindings for TI's AM65 SoC

[Rob Herring]

On Thu, 25 Mar 2021 14:30:21 +0530, Kishon Vijay Abraham I wrote:
> Add PCIe host mode dt-bindings for TI's AM65 SoC.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../bindings/pci/ti,am65-pci-host.yaml        | 111 ++++++++++++++++++
>  1 file changed, 111 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/pci/ti,am65-pci-host.example.dts:44.35-36 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:349: Documentation/devicetree/bindings/pci/ti,am65-pci-host.example.dt.yaml] Error 1
make: *** [Makefile:1380: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1458243

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 1/6] dt-bindings: PCI: ti,am65: Add PCIe host mode dt-bindings for TI's AM65 SoC

[Rob Herring]

On Thu, Mar 25, 2021 at 02:30:21PM +0530, Kishon Vijay Abraham I wrote:
> Add PCIe host mode dt-bindings for TI's AM65 SoC.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../bindings/pci/ti,am65-pci-host.yaml        | 111 ++++++++++++++++++
>  1 file changed, 111 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
> new file mode 100644
> index 000000000000..b77e492886fa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
> @@ -0,0 +1,111 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2021 Texas Instruments Incorporated - http://www.ti.com/
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: TI AM65 PCI Host
> +
> +maintainers:
> +  - Kishon Vijay Abraham I <kishon@ti.com>
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,am654-pcie-rc
> +
> +  reg:
> +    maxItems: 4
> +
> +  reg-names:
> +    items:
> +      - const: app
> +      - const: dbics

Please use 'dbi' like everyone else if this isn't shared with the other 
TI DW PCI bindings.

> +      - const: config
> +      - const: atu
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  ti,syscon-pcie-id:
> +    description: Phandle to the SYSCON entry required for getting PCIe device/vendor ID
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +
> +  ti,syscon-pcie-mode:
> +    description: Phandle to the SYSCON entry required for configuring PCIe in RC or EP mode.
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +
> +  msi-map: true
> +
> +  dma-coherent: true
> +
> +patternProperties:
> +  "interrupt-controller":

Don't need quotes.

> +    type: object
> +    description: interrupt controller to handle legacy interrupts.
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - max-link-speed
> +  - num-lanes
> +  - power-domains
> +  - ti,syscon-pcie-id
> +  - ti,syscon-pcie-mode
> +  - msi-map
> +  - ranges
> +  - reset-gpios
> +  - phys
> +  - phy-names
> +  - dma-coherent

'interrupt-controller' node is optional?

> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pcie0_rc: pcie@5500000 {
> +                compatible = "ti,am654-pcie-rc";
> +                reg =  <0x0 0x5500000 0x0 0x1000>,
> +                       <0x0 0x5501000 0x0 0x1000>,
> +                       <0x0 0x10000000 0x0 0x2000>,
> +                       <0x0 0x5506000 0x0 0x1000>;
> +                reg-names = "app", "dbics", "config", "atu";
> +                power-domains = <&k3_pds 120 TI_SCI_PD_EXCLUSIVE>;
> +                #address-cells = <3>;
> +                #size-cells = <2>;
> +                ranges = <0x81000000 0 0          0x0 0x10020000 0 0x00010000>,
> +                         <0x82000000 0 0x10030000 0x0 0x10030000 0 0x07FD0000>;
> +                ti,syscon-pcie-id = <&pcie_devid>;
> +                ti,syscon-pcie-mode = <&pcie0_mode>;
> +                bus-range = <0x0 0xff>;
> +                num-viewport = <16>;
> +                max-link-speed = <2>;
> +                dma-coherent;
> +                interrupts = <GIC_SPI 340 IRQ_TYPE_EDGE_RISING>;
> +                msi-map = <0x0 &gic_its 0x0 0x10000>;
> +                #interrupt-cells = <1>;
> +                interrupt-map-mask = <0 0 0 7>;
> +                interrupt-map = <0 0 0 1 &pcie0_intc 0>, /* INT A */
> +                                <0 0 0 2 &pcie0_intc 0>, /* INT B */
> +                                <0 0 0 3 &pcie0_intc 0>, /* INT C */
> +                                <0 0 0 4 &pcie0_intc 0>; /* INT D */
> +
> +                pcie0_intc: interrupt-controller {
> +                        interrupt-controller;
> +                        #interrupt-cells = <1>;
> +                        interrupt-parent = <&gic500>;
> +                        interrupts = <GIC_SPI 328 IRQ_TYPE_EDGE_RISING>;
> +                };
> +        };
> -- 
> 2.17.1
> 

Re: [PATCH 1/6] dt-bindings: PCI: ti,am65: Add PCIe host mode dt-bindings for TI's AM65 SoC

[Kishon Vijay Abraham I]

Hi Rob,

On 26/03/21 5:08 am, Rob Herring wrote:
> On Thu, Mar 25, 2021 at 02:30:21PM +0530, Kishon Vijay Abraham I wrote:
>> Add PCIe host mode dt-bindings for TI's AM65 SoC.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  .../bindings/pci/ti,am65-pci-host.yaml        | 111 ++++++++++++++++++
>>  1 file changed, 111 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>> new file mode 100644
>> index 000000000000..b77e492886fa
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>> @@ -0,0 +1,111 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2021 Texas Instruments Incorporated - http://www.ti.com/
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>> +
>> +title: TI AM65 PCI Host
>> +
>> +maintainers:
>> +  - Kishon Vijay Abraham I <kishon@ti.com>
>> +
>> +allOf:
>> +  - $ref: /schemas/pci/pci-bus.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ti,am654-pcie-rc
>> +
>> +  reg:
>> +    maxItems: 4
>> +
>> +  reg-names:
>> +    items:
>> +      - const: app
>> +      - const: dbics
> 
> Please use 'dbi' like everyone else if this isn't shared with the other 
> TI DW PCI bindings.

I'm just converting existing binding in pci-keystone.txt to yaml.
Documentation/devicetree/bindings/pci/pci-keystone.txt

Device tree for AM65 is also already in the upstream kernel.

I can try to remove the am65 specific part from pci-keystone.txt
> 
>> +      - const: config
>> +      - const: atu
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  ti,syscon-pcie-id:
>> +    description: Phandle to the SYSCON entry required for getting PCIe device/vendor ID
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +
>> +  ti,syscon-pcie-mode:
>> +    description: Phandle to the SYSCON entry required for configuring PCIe in RC or EP mode.
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +
>> +  msi-map: true
>> +
>> +  dma-coherent: true
>> +
>> +patternProperties:
>> +  "interrupt-controller":
> 
> Don't need quotes.

sure, will fix it.
> 
>> +    type: object
>> +    description: interrupt controller to handle legacy interrupts.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - max-link-speed
>> +  - num-lanes
>> +  - power-domains
>> +  - ti,syscon-pcie-id
>> +  - ti,syscon-pcie-mode
>> +  - msi-map
>> +  - ranges
>> +  - reset-gpios
>> +  - phys
>> +  - phy-names
>> +  - dma-coherent
> 
> 'interrupt-controller' node is optional?

yeah, upstream DT doesn't have interrupt-controller. It's added as part
of this series.

Thanks
Kishon
> 
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
>> +    #include <dt-bindings/gpio/gpio.h>
>> +
>> +    bus {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        pcie0_rc: pcie@5500000 {
>> +                compatible = "ti,am654-pcie-rc";
>> +                reg =  <0x0 0x5500000 0x0 0x1000>,
>> +                       <0x0 0x5501000 0x0 0x1000>,
>> +                       <0x0 0x10000000 0x0 0x2000>,
>> +                       <0x0 0x5506000 0x0 0x1000>;
>> +                reg-names = "app", "dbics", "config", "atu";
>> +                power-domains = <&k3_pds 120 TI_SCI_PD_EXCLUSIVE>;
>> +                #address-cells = <3>;
>> +                #size-cells = <2>;
>> +                ranges = <0x81000000 0 0          0x0 0x10020000 0 0x00010000>,
>> +                         <0x82000000 0 0x10030000 0x0 0x10030000 0 0x07FD0000>;
>> +                ti,syscon-pcie-id = <&pcie_devid>;
>> +                ti,syscon-pcie-mode = <&pcie0_mode>;
>> +                bus-range = <0x0 0xff>;
>> +                num-viewport = <16>;
>> +                max-link-speed = <2>;
>> +                dma-coherent;
>> +                interrupts = <GIC_SPI 340 IRQ_TYPE_EDGE_RISING>;
>> +                msi-map = <0x0 &gic_its 0x0 0x10000>;
>> +                #interrupt-cells = <1>;
>> +                interrupt-map-mask = <0 0 0 7>;
>> +                interrupt-map = <0 0 0 1 &pcie0_intc 0>, /* INT A */
>> +                                <0 0 0 2 &pcie0_intc 0>, /* INT B */
>> +                                <0 0 0 3 &pcie0_intc 0>, /* INT C */
>> +                                <0 0 0 4 &pcie0_intc 0>; /* INT D */
>> +
>> +                pcie0_intc: interrupt-controller {
>> +                        interrupt-controller;
>> +                        #interrupt-cells = <1>;
>> +                        interrupt-parent = <&gic500>;
>> +                        interrupts = <GIC_SPI 328 IRQ_TYPE_EDGE_RISING>;
>> +                };
>> +        };
>> -- 
>> 2.17.1
>>

Re: [PATCH 1/6] dt-bindings: PCI: ti,am65: Add PCIe host mode dt-bindings for TI's AM65 SoC

[Rob Herring]

On Tue, Mar 30, 2021 at 4:29 AM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
> Hi Rob,
>
> On 26/03/21 5:08 am, Rob Herring wrote:
> > On Thu, Mar 25, 2021 at 02:30:21PM +0530, Kishon Vijay Abraham I wrote:
> >> Add PCIe host mode dt-bindings for TI's AM65 SoC.
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> ---
> >>  .../bindings/pci/ti,am65-pci-host.yaml        | 111 ++++++++++++++++++
> >>  1 file changed, 111 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
> >> new file mode 100644
> >> index 000000000000..b77e492886fa
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
> >> @@ -0,0 +1,111 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +# Copyright (C) 2021 Texas Instruments Incorporated - http://www.ti.com/
> >> +%YAML 1.2
> >> +---
> >> +$id: "http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml#"
> >> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> >> +
> >> +title: TI AM65 PCI Host
> >> +
> >> +maintainers:
> >> +  - Kishon Vijay Abraham I <kishon@ti.com>
> >> +
> >> +allOf:
> >> +  - $ref: /schemas/pci/pci-bus.yaml#
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - ti,am654-pcie-rc
> >> +
> >> +  reg:
> >> +    maxItems: 4
> >> +
> >> +  reg-names:
> >> +    items:
> >> +      - const: app
> >> +      - const: dbics
> >
> > Please use 'dbi' like everyone else if this isn't shared with the other
> > TI DW PCI bindings.
>
> I'm just converting existing binding in pci-keystone.txt to yaml.
> Documentation/devicetree/bindings/pci/pci-keystone.txt
>
> Device tree for AM65 is also already in the upstream kernel.
>
> I can try to remove the am65 specific part from pci-keystone.txt

Can you remove pci-keystone.txt entirely. That's what 'converting' means.

Rob

[PATCH 2/6] dt-bindings: PCI: ti,am65: Add PCIe endpoint mode dt-bindings for TI's AM65 SoC

[Kishon Vijay Abraham I]

Add PCIe endpoint mode dt-bindings for TI's AM65 SoC.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../bindings/pci/ti,am65-pci-ep.yaml          | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/ti,am65-pci-ep.yaml

diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-ep.yaml
new file mode 100644
index 000000000000..f0a5518e6331
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-ep.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2021 Texas Instruments Incorporated - http://www.ti.com/
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/pci/ti,am65-pci-ep.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: TI AM65 PCI Endpoint
+
+maintainers:
+  - Kishon Vijay Abraham I <kishon@ti.com>
+
+allOf:
+  - $ref: "pci-ep.yaml#"
+
+properties:
+  compatible:
+    enum:
+      - ti,am654-pcie-ep
+
+  reg:
+    maxItems: 4
+
+  reg-names:
+    items:
+      - const: app
+      - const: dbics
+      - const: addr_space
+      - const: atu
+
+  power-domains:
+    maxItems: 1
+
+  ti,syscon-pcie-mode:
+    description: Phandle to the SYSCON entry required for configuring PCIe in RC or EP mode.
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+  interrupts:
+    minItems: 1
+
+  dma-coherent: true
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - max-link-speed
+  - num-lanes
+  - power-domains
+  - ti,syscon-pcie-mode
+  - phys
+  - phy-names
+  - dma-coherent
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pcie0_ep: pcie-ep@5500000 {
+                compatible = "ti,am654-pcie-ep";
+                reg =  <0x0 0x5500000 0x0 0x1000>,
+                       <0x0 0x5501000 0x0 0x1000>,
+                       <0x0 0x10000000 0x0 0x8000000>,
+                       <0x0 0x5506000 0x0 0x1000>;
+                reg-names = "app", "dbics", "addr_space", "atu";
+                power-domains = <&k3_pds 120 TI_SCI_PD_EXCLUSIVE>;
+                ti,syscon-pcie-mode = <&pcie0_mode>;
+                num-ib-windows = <16>;
+                num-ob-windows = <16>;
+                max-link-speed = <2>;
+                dma-coherent;
+                interrupts = <GIC_SPI 340 IRQ_TYPE_EDGE_RISING>;
+        };
-- 
2.17.1

Re: [PATCH 2/6] dt-bindings: PCI: ti, am65: Add PCIe endpoint mode dt-bindings for TI's AM65 SoC

[Rob Herring]

On Thu, 25 Mar 2021 14:30:22 +0530, Kishon Vijay Abraham I wrote:
> Add PCIe endpoint mode dt-bindings for TI's AM65 SoC.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../bindings/pci/ti,am65-pci-ep.yaml          | 80 +++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/ti,am65-pci-ep.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/pci/ti,am65-pci-ep.example.dts:39.35-36 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:349: Documentation/devicetree/bindings/pci/ti,am65-pci-ep.example.dt.yaml] Error 1
make: *** [Makefile:1380: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1458245

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[PATCH 3/6] irqdomain: Export of_phandle_args_to_fwspec()

[Kishon Vijay Abraham I]

Export of_phandle_args_to_fwspec() to be used by drivers.
of_phandle_args_to_fwspec() can be used by drivers to get irq specifier
from device node useful while creating hierarchy domain. This was
suggested by Marc Zyngier [1].

[1] -> http://lore.kernel.org/r/20190223121143.14c1f150@why.wild-wind.fr.eu.org/
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 include/linux/irqdomain.h | 2 ++
 kernel/irq/irqdomain.c    | 6 +++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 42d196805f58..0236f508259e 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -391,6 +391,8 @@ extern void irq_domain_associate_many(struct irq_domain *domain,
 extern unsigned int irq_create_mapping_affinity(struct irq_domain *host,
 				      irq_hw_number_t hwirq,
 				      const struct irq_affinity_desc *affinity);
+void of_phandle_args_to_fwspec(struct device_node *np, const u32 *args, unsigned int count,
+			       struct irq_fwspec *fwspec);
 extern unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec);
 extern void irq_dispose_mapping(unsigned int virq);
 
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 288151393a06..70f050741ab2 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -756,9 +756,8 @@ static int irq_domain_translate(struct irq_domain *d,
 	return 0;
 }
 
-static void of_phandle_args_to_fwspec(struct device_node *np, const u32 *args,
-				      unsigned int count,
-				      struct irq_fwspec *fwspec)
+void of_phandle_args_to_fwspec(struct device_node *np, const u32 *args, unsigned int count,
+			       struct irq_fwspec *fwspec)
 {
 	int i;
 
@@ -768,6 +767,7 @@ static void of_phandle_args_to_fwspec(struct device_node *np, const u32 *args,
 	for (i = 0; i < count; i++)
 		fwspec->param[i] = args[i];
 }
+EXPORT_SYMBOL_GPL(of_phandle_args_to_fwspec);
 
 unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 {
-- 
2.17.1

[PATCH 4/6] PCI: keystone: Convert to using hierarchy domain for legacy interrupts

[Kishon Vijay Abraham I]

K2G provides separate IRQ lines for each of the four legacy interrupts.
Model this using hierarchy domain instead of linear domain with chained
IRQ handler.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 214 ++++++++++++----------
 1 file changed, 120 insertions(+), 94 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 4de8c8e5e3f2..dfa9a7fcf9b7 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -69,6 +69,7 @@
 
 #define IRQ_STATUS(n)			(0x184 + ((n) << 4))
 #define IRQ_ENABLE_SET(n)		(0x188 + ((n) << 4))
+#define IRQ_ENABLE_CLR(n)		(0x18c + ((n) << 4))
 #define INTx_EN				BIT(0)
 
 #define ERR_IRQ_STATUS			0x1c4
@@ -116,7 +117,6 @@ struct keystone_pcie {
 	struct dw_pcie		*pci;
 	/* PCI Device ID */
 	u32			device_id;
-	int			legacy_host_irqs[PCI_NUM_INTX];
 	struct			device_node *legacy_intc_np;
 
 	int			msi_host_irq;
@@ -125,7 +125,6 @@ struct keystone_pcie {
 	struct phy		**phy;
 	struct device_link	**link;
 	struct			device_node *msi_intc_np;
-	struct irq_domain	*legacy_irq_domain;
 	struct device_node	*np;
 
 	/* Application register space */
@@ -253,26 +252,6 @@ static int ks_pcie_msi_host_init(struct pcie_port *pp)
 	return dw_pcie_allocate_domains(pp);
 }
 
-static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
-				      int offset)
-{
-	struct dw_pcie *pci = ks_pcie->pci;
-	struct device *dev = pci->dev;
-	u32 pending;
-	int virq;
-
-	pending = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(offset));
-
-	if (BIT(0) & pending) {
-		virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
-		dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
-		generic_handle_irq(virq);
-	}
-
-	/* EOI the INTx interrupt */
-	ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset);
-}
-
 static void ks_pcie_enable_error_irq(struct keystone_pcie *ks_pcie)
 {
 	ks_pcie_app_writel(ks_pcie, ERR_IRQ_ENABLE_SET, ERR_IRQ_ALL);
@@ -310,39 +289,120 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
 	return IRQ_HANDLED;
 }
 
-static void ks_pcie_ack_legacy_irq(struct irq_data *d)
+void ks_pcie_irq_eoi(struct irq_data *data)
 {
+	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
+	irq_hw_number_t hwirq = data->hwirq;
+
+	ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq);
+	irq_chip_eoi_parent(data);
 }
 
-static void ks_pcie_mask_legacy_irq(struct irq_data *d)
+void ks_pcie_irq_enable(struct irq_data *data)
 {
+	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
+	irq_hw_number_t hwirq = data->hwirq;
+
+	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN);
+	irq_chip_enable_parent(data);
 }
 
-static void ks_pcie_unmask_legacy_irq(struct irq_data *d)
+void ks_pcie_irq_disable(struct irq_data *data)
 {
+	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
+	irq_hw_number_t hwirq = data->hwirq;
+
+	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN);
+	irq_chip_disable_parent(data);
 }
 
 static struct irq_chip ks_pcie_legacy_irq_chip = {
-	.name = "Keystone-PCI-Legacy-IRQ",
-	.irq_ack = ks_pcie_ack_legacy_irq,
-	.irq_mask = ks_pcie_mask_legacy_irq,
-	.irq_unmask = ks_pcie_unmask_legacy_irq,
+	.name			= "Keystone-PCI-Legacy-IRQ",
+	.irq_enable		= ks_pcie_irq_enable,
+	.irq_disable		= ks_pcie_irq_disable,
+	.irq_eoi		= ks_pcie_irq_eoi,
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
 };
 
-static int ks_pcie_init_legacy_irq_map(struct irq_domain *d,
-				       unsigned int irq,
-				       irq_hw_number_t hw_irq)
+static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+					   unsigned int nr_irqs, void *data)
 {
-	irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip,
-				 handle_level_irq);
-	irq_set_chip_data(irq, d->host_data);
+	struct keystone_pcie *ks_pcie = domain->host_data;
+	struct device_node *np = ks_pcie->legacy_intc_np;
+	struct irq_fwspec parent_fwspec, *fwspec = data;
+	struct of_phandle_args out_irq;
+	int ret;
+
+	if (nr_irqs != 1)
+		return -EINVAL;
+
+	/*
+	 * Get the correct interrupt from legacy-interrupt-controller node
+	 * corresponding to INTA/INTB/INTC/INTD (passed in fwspec->param[0])
+	 * after performing mapping specified in "interrupt-map".
+	 * interrupt-map = <0 0 0 1 &pcie_intc0 0>, INTA (4th cell in
+	 * interrupt-map) corresponds to 1st entry in "interrupts" (6th cell
+	 * in interrupt-map)
+	 */
+	ret = of_irq_parse_one(np, fwspec->param[0], &out_irq);
+	if (ret < 0) {
+		pr_err("Failed to parse interrupt node\n");
+		return ret;
+	}
+
+	of_phandle_args_to_fwspec(np, out_irq.args, out_irq.args_count, &parent_fwspec);
+
+	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
+	if (ret < 0) {
+		pr_err("Failed to allocate parent irq %u: %d\n",
+		       parent_fwspec.param[0], ret);
+		return ret;
+	}
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0],
+					    &ks_pcie_legacy_irq_chip, ks_pcie);
+	if (ret < 0) {
+		pr_err("Failed to set hwirq and chip\n");
+		goto err_set_hwirq_and_chip;
+	}
 
 	return 0;
+
+err_set_hwirq_and_chip:
+	irq_domain_free_irqs_parent(domain, virq, 1);
+
+	return ret;
+}
+
+static int ks_pcie_irq_domain_translate(struct irq_domain *domain,
+					struct irq_fwspec *fwspec,
+					unsigned long *hwirq,
+					unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count != 2)
+			return -EINVAL;
+
+		if (fwspec->param[0] >= PCI_NUM_INTX)
+			return -EINVAL;
+
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[1];
+
+		return 0;
+	}
+
+	return -EINVAL;
 }
 
 static const struct irq_domain_ops ks_pcie_legacy_irq_domain_ops = {
-	.map = ks_pcie_init_legacy_irq_map,
-	.xlate = irq_domain_xlate_onetwocell,
+	.alloc		= ks_pcie_legacy_irq_domain_alloc,
+	.free		= irq_domain_free_irqs_common,
+	.translate	= ks_pcie_irq_domain_translate,
 };
 
 /**
@@ -614,35 +674,6 @@ static void ks_pcie_msi_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-/**
- * ks_pcie_legacy_irq_handler() - Handle legacy interrupt
- * @irq: IRQ line for legacy interrupts
- * @desc: Pointer to irq descriptor
- *
- * Traverse through pending legacy interrupts and invoke handler for each. Also
- * takes care of interrupt controller level mask/ack operation.
- */
-static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
-{
-	unsigned int irq = irq_desc_get_irq(desc);
-	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
-	struct dw_pcie *pci = ks_pcie->pci;
-	struct device *dev = pci->dev;
-	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-
-	dev_dbg(dev, ": Handling legacy irq %d\n", irq);
-
-	/*
-	 * The chained irq handler installation would have replaced normal
-	 * interrupt driver handler so we need to take care of mask/unmask and
-	 * ack operation.
-	 */
-	chained_irq_enter(chip, desc);
-	ks_pcie_handle_legacy_irq(ks_pcie, irq_offset);
-	chained_irq_exit(chip, desc);
-}
-
 static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
 {
 	struct device *dev = ks_pcie->pci->dev;
@@ -702,20 +733,33 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
 	struct device *dev = ks_pcie->pci->dev;
 	struct irq_domain *legacy_irq_domain;
 	struct device_node *np = ks_pcie->np;
+	struct irq_domain *parent_domain;
+	struct device_node *parent_node;
 	struct device_node *intc_np;
-	int irq_count, irq, ret = 0, i;
+	int irq_count, ret = 0;
 
-	intc_np = of_get_child_by_name(np, "legacy-interrupt-controller");
+	intc_np = of_get_child_by_name(np, "interrupt-controller");
 	if (!intc_np) {
-		/*
-		 * Since legacy interrupts are modeled as edge-interrupts in
-		 * AM6, keep it disabled for now.
-		 */
-		if (ks_pcie->is_am6)
-			return 0;
 		dev_warn(dev, "legacy-interrupt-controller node is absent\n");
 		return -EINVAL;
 	}
+	ks_pcie->legacy_intc_np = intc_np;
+
+	parent_node = of_irq_find_parent(intc_np);
+	if (!parent_node) {
+		dev_err(dev, "unable to obtain parent node\n");
+		ret = -ENXIO;
+		goto err;
+	}
+
+	parent_domain = irq_find_host(parent_node);
+	if (!parent_domain) {
+		dev_err(dev, "unable to obtain parent domain\n");
+		ret = -ENXIO;
+		goto err;
+	}
+
+	of_node_put(parent_node);
 
 	irq_count = of_irq_count(intc_np);
 	if (!irq_count) {
@@ -724,31 +768,13 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
 		goto err;
 	}
 
-	for (i = 0; i < irq_count; i++) {
-		irq = irq_of_parse_and_map(intc_np, i);
-		if (!irq) {
-			ret = -EINVAL;
-			goto err;
-		}
-		ks_pcie->legacy_host_irqs[i] = irq;
-
-		irq_set_chained_handler_and_data(irq,
-						 ks_pcie_legacy_irq_handler,
-						 ks_pcie);
-	}
-
-	legacy_irq_domain =
-		irq_domain_add_linear(intc_np, PCI_NUM_INTX,
-				      &ks_pcie_legacy_irq_domain_ops, NULL);
+	legacy_irq_domain = irq_domain_add_hierarchy(parent_domain, 0, PCI_NUM_INTX, intc_np,
+						     &ks_pcie_legacy_irq_domain_ops, ks_pcie);
 	if (!legacy_irq_domain) {
 		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
 		ret = -EINVAL;
 		goto err;
 	}
-	ks_pcie->legacy_irq_domain = legacy_irq_domain;
-
-	for (i = 0; i < PCI_NUM_INTX; i++)
-		ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(i), INTx_EN);
 
 err:
 	of_node_put(intc_np);
-- 
2.17.1

Re: [PATCH 4/6] PCI: keystone: Convert to using hierarchy domain for legacy interrupts

[Krzysztof Wilczyński]

Hi Kishon,

Thank you for sending the series over!

A few small nitpick, so feel free to ignore it.

[...]
> +	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
> +	if (ret < 0) {
> +		pr_err("Failed to allocate parent irq %u: %d\n",
> +		       parent_fwspec.param[0], ret);
> +		return ret;
[...]

Use "IRQ" in the message above.

Also, the error messages with both starting with upper- and lower- case
letter, not sure if this is because of dev_err() vs pr_err(), but if
there is no significance between these two methods, then it might be
nice to keep the style consistent.

Krzysztof

[PATCH 5/6] PCI: keystone: Add PCI legacy interrupt support for AM654

[Kishon Vijay Abraham I]

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

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 87 +++++++++++++++++++++--
 1 file changed, 82 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index dfa9a7fcf9b7..84a25207d0d3 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -118,6 +118,7 @@ struct keystone_pcie {
 	/* PCI Device ID */
 	u32			device_id;
 	struct			device_node *legacy_intc_np;
+	struct irq_domain	*legacy_irq_domain;
 
 	int			msi_host_irq;
 	int			num_lanes;
@@ -289,6 +290,29 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
 	return IRQ_HANDLED;
 }
 
+static void ks_pcie_am654_legacy_irq_handler(struct irq_desc *desc)
+{
+	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	int virq, i;
+	u32 reg;
+
+	chained_irq_enter(chip, desc);
+
+	for (i = 0; i < PCI_NUM_INTX; i++) {
+		reg = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(i));
+		if (!(reg & INTx_EN))
+			continue;
+
+		virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, i);
+		generic_handle_irq(virq);
+		ks_pcie_app_writel(ks_pcie, IRQ_STATUS(i), INTx_EN);
+		ks_pcie_app_writel(ks_pcie, IRQ_EOI, i);
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
 void ks_pcie_irq_eoi(struct irq_data *data)
 {
 	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
@@ -728,6 +752,54 @@ static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
 	return ret;
 }
 
+static int ks_pcie_am654_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 ks_pcie_am654_irq_domain_ops = {
+	.map = ks_pcie_am654_intx_map,
+};
+
+static int ks_pcie_am654_config_legacy_irq(struct keystone_pcie *ks_pcie)
+{
+	struct device *dev = ks_pcie->pci->dev;
+	struct irq_domain *legacy_irq_domain;
+	struct device_node *np = ks_pcie->np;
+	struct device_node *intc_np;
+	int ret = 0;
+	int irq;
+	int i;
+
+	intc_np = of_get_child_by_name(np, "interrupt-controller");
+	if (!intc_np) {
+		dev_warn(dev, "legacy interrupt-controller node is absent\n");
+		return -EINVAL;
+	}
+
+	irq = irq_of_parse_and_map(intc_np, 0);
+	if (!irq)
+		return -EINVAL;
+
+	irq_set_chained_handler_and_data(irq, ks_pcie_am654_legacy_irq_handler, ks_pcie);
+	legacy_irq_domain = irq_domain_add_linear(intc_np, PCI_NUM_INTX,
+						  &ks_pcie_am654_irq_domain_ops, ks_pcie);
+	if (!legacy_irq_domain) {
+		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
+		return -EINVAL;
+	}
+	ks_pcie->legacy_irq_domain = legacy_irq_domain;
+
+	for (i = 0; i < PCI_NUM_INTX; i++)
+		ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(i), INTx_EN);
+
+	return ret;
+}
+
 static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
 {
 	struct device *dev = ks_pcie->pci->dev;
@@ -835,12 +907,17 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
 	int ret;
 
 	pp->bridge->ops = &ks_pcie_ops;
-	if (!ks_pcie->is_am6)
-		pp->bridge->child_ops = &ks_child_pcie_ops;
 
-	ret = ks_pcie_config_legacy_irq(ks_pcie);
-	if (ret)
-		return ret;
+	if (!ks_pcie->is_am6) {
+		pp->bridge->child_ops = &ks_child_pcie_ops;
+		ret = ks_pcie_config_legacy_irq(ks_pcie);
+		if (ret)
+			return ret;
+	} else {
+		ret = ks_pcie_am654_config_legacy_irq(ks_pcie);
+		if (ret)
+			return ret;
+	}
 
 	ret = ks_pcie_config_msi_irq(ks_pcie);
 	if (ret)
-- 
2.17.1

Re: [PATCH 5/6] PCI: keystone: Add PCI legacy interrupt support for AM654

[Krzysztof Wilczyński]

Hi Kishon,

[...]
> +	if (!legacy_irq_domain) {
> +		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
> +		return -EINVAL;
> +	}
[...]

It would be "IRQ" and "IRQs" in the message above.

[...]
> -	ret = ks_pcie_config_legacy_irq(ks_pcie);
> -	if (ret)
> -		return ret;
> +	if (!ks_pcie->is_am6) {
> +		pp->bridge->child_ops = &ks_child_pcie_ops;
> +		ret = ks_pcie_config_legacy_irq(ks_pcie);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = ks_pcie_am654_config_legacy_irq(ks_pcie);
> +		if (ret)
> +			return ret;
> +	}
[...]

What if we change this to the following:

	if (!ks_pcie->is_am6) {
		pp->bridge->child_ops = &ks_child_pcie_ops;
		ret = ks_pcie_config_legacy_irq(ks_pcie);
	} else {
		ret = ks_pcie_am654_config_legacy_irq(ks_pcie);
	}
	
	if (ret)
	  	return ret;

Not sure if this is something you would prefer, but it seems that either
of the functions can set "ret", so checking immediately after would be
the same as checking in either of the branches.  But, this is a matter
of style, so it would be up to you - not sure what do you prefer.

Krzysztof

Re: [PATCH 5/6] PCI: keystone: Add PCI legacy interrupt support for AM654

[Marc Zyngier]

On Thu, 25 Mar 2021 09:00:25 +0000,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> Add PCI legacy interrupt support for AM654. AM654 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 AM654 has provided IRQ_EOI register. When the SW writes to IRQ_EOI
> register after handling the interrupt, the IP checks the state of
> legacy interrupt and re-triggers pulse interrupt invoking the handler
> again.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 87 +++++++++++++++++++++--
>  1 file changed, 82 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index dfa9a7fcf9b7..84a25207d0d3 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -118,6 +118,7 @@ struct keystone_pcie {
>  	/* PCI Device ID */
>  	u32			device_id;
>  	struct			device_node *legacy_intc_np;
> +	struct irq_domain	*legacy_irq_domain;
>  
>  	int			msi_host_irq;
>  	int			num_lanes;
> @@ -289,6 +290,29 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
>  	return IRQ_HANDLED;
>  }
>  
> +static void ks_pcie_am654_legacy_irq_handler(struct irq_desc *desc)
> +{
> +	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	int virq, i;
> +	u32 reg;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	for (i = 0; i < PCI_NUM_INTX; i++) {
> +		reg = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(i));
> +		if (!(reg & INTx_EN))
> +			continue;
> +
> +		virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, i);
> +		generic_handle_irq(virq);

I'm on my way to kill irq_linear_revmap(), so I'd rather you didn't
add more instances. Consider using irq_find_mapping() instead.

> +		ks_pcie_app_writel(ks_pcie, IRQ_STATUS(i), INTx_EN);
> +		ks_pcie_app_writel(ks_pcie, IRQ_EOI, i);

What are these writes for? The first one feels like an Ack, and the
second one has EOI written over it.

If that's what they are, llease move these to a proper irq_chip
structure and use the appropriate flow handler, instead of
dummy_irq_chip and handle_simple_irq.

Thanks,

	M.

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

[PATCH 6/6] PCI: keystone: Add workaround for Errata #i2037 (AM65x SR 1.0)

[Kishon Vijay Abraham I]

Errata #i2037 in AM65x/DRA80xM Processors Silicon Revision 1.0
(SPRZ452D–July 2018–Revised December 2019 [1]) mentions when an
inbound PCIe TLP spans more than two internal AXI 128-byte bursts,
the bus may corrupt the packet payload and the corrupt data may
cause associated applications or the processor to hang.

The workaround for Errata #i2037 is to limit the maximum read
request size and maximum payload size to 128 Bytes. Add workaround
for Errata #i2037 here. The errata and workaround is applicable
only to AM65x SR 1.0 and later versions of the silicon will have
this fixed.

[1] -> http://www.ti.com/lit/er/sprz452d/sprz452d.pdf

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 42 +++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 84a25207d0d3..80b6e874199d 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -35,6 +35,11 @@
 #define PCIE_DEVICEID_SHIFT	16
 
 /* Application registers */
+#define PID				0x000
+#define RTL				GENMASK(15, 11)
+#define RTL_SHIFT			11
+#define AM6_PCI_PG1_RTL_VER		0x15
+
 #define CMD_STATUS			0x004
 #define LTSSM_EN_VAL		        BIT(0)
 #define OB_XLAT_EN_VAL		        BIT(1)
@@ -106,6 +111,8 @@
 
 #define to_keystone_pcie(x)		dev_get_drvdata((x)->dev)
 
+#define PCI_DEVICE_ID_TI_AM654X		0xb00c
+
 struct ks_pcie_of_data {
 	enum dw_pcie_device_mode mode;
 	const struct dw_pcie_host_ops *host_ops;
@@ -619,7 +626,11 @@ static int ks_pcie_start_link(struct dw_pcie *pci)
 static void ks_pcie_quirk(struct pci_dev *dev)
 {
 	struct pci_bus *bus = dev->bus;
+	struct keystone_pcie *ks_pcie;
+	struct device *bridge_dev;
 	struct pci_dev *bridge;
+	u32 val;
+
 	static const struct pci_device_id rc_pci_devids[] = {
 		{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCIE_RC_K2HK),
 		 .class = PCI_CLASS_BRIDGE_PCI << 8, .class_mask = ~0, },
@@ -631,6 +642,11 @@ static void ks_pcie_quirk(struct pci_dev *dev)
 		 .class = PCI_CLASS_BRIDGE_PCI << 8, .class_mask = ~0, },
 		{ 0, },
 	};
+	static const struct pci_device_id am6_pci_devids[] = {
+		{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_AM654X),
+		 .class = PCI_CLASS_BRIDGE_PCI << 8, .class_mask = ~0, },
+		{ 0, },
+	};
 
 	if (pci_is_root_bus(bus))
 		bridge = dev;
@@ -656,6 +672,32 @@ static void ks_pcie_quirk(struct pci_dev *dev)
 			pcie_set_readrq(dev, 256);
 		}
 	}
+
+	/*
+	 * Memory transactions fail with PCI controller in AM654 PG1.0
+	 * when MRRS is set to more than 128 Bytes. Force the MRRS to
+	 * 128 Bytes in all downstream devices.
+	 */
+	if (pci_match_id(am6_pci_devids, bridge)) {
+		bridge_dev = pci_get_host_bridge_device(dev);
+		if (!bridge_dev && !bridge_dev->parent)
+			return;
+
+		ks_pcie = dev_get_drvdata(bridge_dev->parent);
+		if (!ks_pcie)
+			return;
+
+		val = ks_pcie_app_readl(ks_pcie, PID);
+		val &= RTL;
+		val >>= RTL_SHIFT;
+		if (val != AM6_PCI_PG1_RTL_VER)
+			return;
+
+		if (pcie_get_readrq(dev) > 128) {
+			dev_info(&dev->dev, "limiting MRRS to 128\n");
+			pcie_set_readrq(dev, 128);
+		}
+	}
 }
 DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, ks_pcie_quirk);
 
-- 
2.17.1

Re: [PATCH 6/6] PCI: keystone: Add workaround for Errata #i2037 (AM65x SR 1.0)

[Krzysztof Wilczyński]

Hi Kishon,

A few small nitpicks.

> Errata #i2037 in AM65x/DRA80xM Processors Silicon Revision 1.0
> (SPRZ452D–July 2018–Revised December 2019 [1]) mentions when an
> inbound PCIe TLP spans more than two internal AXI 128-byte bursts,
> the bus may corrupt the packet payload and the corrupt data may
> cause associated applications or the processor to hang.
> 
> The workaround for Errata #i2037 is to limit the maximum read
> request size and maximum payload size to 128 Bytes. Add workaround
> for Errata #i2037 here. The errata and workaround is applicable
> only to AM65x SR 1.0 and later versions of the silicon will have
> this fixed.

I think it would be either "128 B" or "128 bytes", there is no need to
capitalise bytes.

[...]
> +	/*
> +	 * Memory transactions fail with PCI controller in AM654 PG1.0
> +	 * when MRRS is set to more than 128 Bytes. Force the MRRS to
> +	 * 128 Bytes in all downstream devices.
> +	 */

Same here, it would be "128 bytes" in the comment above.

[...]
> +		if (pcie_get_readrq(dev) > 128) {
> +			dev_info(&dev->dev, "limiting MRRS to 128\n");
> +			pcie_set_readrq(dev, 128);
> +		}
[...]

Might be nice to add unit here, so "128 bytes".

Krzysztof

Re: [PATCH 0/6] PCI: Add legacy interrupt support in Keystone

[Christian Gmeiner]

Hi

Am Do., 25. März 2021 um 10:04 Uhr schrieb Kishon Vijay Abraham I
<kishon@ti.com>:
>
> Keystone driver is used by K2G and AM65 and the interrupt handling of
> both of them is different. Add support to handle legacy interrupt for
> both K2G and AM65 here.
>
> Some discussions regarding this was already done here [1] and it was
> around having pulse interrupt for legacy interrupt.
>
> 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 AM654 has provided IRQ_EOI register. When the SW writes to IRQ_EOI
> register after handling the interrupt, the IP checks the state of
> legacy interrupt and re-triggers pulse interrupt invoking the handler
> again.
>
> Patch series also includes converting AM65 binding to YAML and an
> errata applicable for i2037.
>
> [1] -> https://lore.kernel.org/linux-arm-kernel/20190221101518.22604-4-kishon@ti.com/
>
> Kishon Vijay Abraham I (6):
>   dt-bindings: PCI: ti,am65: Add PCIe host mode dt-bindings for TI's
>     AM65 SoC
>   dt-bindings: PCI: ti,am65: Add PCIe endpoint mode dt-bindings for TI's
>     AM65 SoC
>   irqdomain: Export of_phandle_args_to_fwspec()
>   PCI: keystone: Convert to using hierarchy domain for legacy interrupts
>   PCI: keystone: Add PCI legacy interrupt support for AM654
>   PCI: keystone: Add workaround for Errata #i2037 (AM65x SR 1.0)
>
>  .../bindings/pci/ti,am65-pci-ep.yaml          |  80 ++++
>  .../bindings/pci/ti,am65-pci-host.yaml        | 111 ++++++
>  drivers/pci/controller/dwc/pci-keystone.c     | 343 +++++++++++++-----
>  include/linux/irqdomain.h                     |   2 +
>  kernel/irq/irqdomain.c                        |   6 +-
>  5 files changed, 440 insertions(+), 102 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/ti,am65-pci-ep.yaml
>  create mode 100644 Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>
> --
> 2.17.1
>

Is there somewhere an updated version of this patch series?

-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy

Re: [PATCH 0/6] PCI: Add legacy interrupt support in Keystone

[Kishon Vijay Abraham I]

Hi Christian,

On 17/05/21 6:45 pm, Christian Gmeiner wrote:
> Hi
> 
> Am Do., 25. März 2021 um 10:04 Uhr schrieb Kishon Vijay Abraham I
> <kishon@ti.com>:
>>
>> Keystone driver is used by K2G and AM65 and the interrupt handling of
>> both of them is different. Add support to handle legacy interrupt for
>> both K2G and AM65 here.
>>
>> Some discussions regarding this was already done here [1] and it was
>> around having pulse interrupt for legacy interrupt.
>>
>> 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 AM654 has provided IRQ_EOI register. When the SW writes to IRQ_EOI
>> register after handling the interrupt, the IP checks the state of
>> legacy interrupt and re-triggers pulse interrupt invoking the handler
>> again.
>>
>> Patch series also includes converting AM65 binding to YAML and an
>> errata applicable for i2037.
>>
>> [1] -> https://lore.kernel.org/linux-arm-kernel/20190221101518.22604-4-kishon@ti.com/
>>
>> Kishon Vijay Abraham I (6):
>>   dt-bindings: PCI: ti,am65: Add PCIe host mode dt-bindings for TI's
>>     AM65 SoC
>>   dt-bindings: PCI: ti,am65: Add PCIe endpoint mode dt-bindings for TI's
>>     AM65 SoC
>>   irqdomain: Export of_phandle_args_to_fwspec()
>>   PCI: keystone: Convert to using hierarchy domain for legacy interrupts
>>   PCI: keystone: Add PCI legacy interrupt support for AM654
>>   PCI: keystone: Add workaround for Errata #i2037 (AM65x SR 1.0)
>>
>>  .../bindings/pci/ti,am65-pci-ep.yaml          |  80 ++++
>>  .../bindings/pci/ti,am65-pci-host.yaml        | 111 ++++++
>>  drivers/pci/controller/dwc/pci-keystone.c     | 343 +++++++++++++-----
>>  include/linux/irqdomain.h                     |   2 +
>>  kernel/irq/irqdomain.c                        |   6 +-
>>  5 files changed, 440 insertions(+), 102 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/pci/ti,am65-pci-ep.yaml
>>  create mode 100644 Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>
>> --
>> 2.17.1
>>
> 
> Is there somewhere an updated version of this patch series?

I haven't posted an updated version yet. My plan was to re-work and post
it by early June.

Thanks
Kishon