[PATCH v7 0/2] Adding support for Versal CPM as Root Port driver

[PATCH v7 0/2] Adding support for Versal CPM as Root Port driver

[Bharat Kumar Gogada]

- Adding support for Versal CPM as Root port.
- The Versal ACAP devices include CCIX-PCIe Module (CPM). The integrated
  block for CPM along with the integrated bridge can function
  as PCIe Root Port.
- Versal CPM uses GICv3 ITS feature for assigning MSI/MSI-X
  vectors and handling MSI/MSI-X interrupts.
- Bridge error and legacy interrupts in Versal CPM are handled using
  Versal CPM specific interrupt line.

Changes for v7:
- Adding device tree documentation as schema.
- Using pci_host_probe for registering bridge.

Bharat Kumar Gogada (2):
  PCI: xilinx-cpm: Add YAML schemas for Versal CPM Root Port
  PCI: xilinx-cpm: Add Versal CPM Root Port driver

 .../devicetree/bindings/pci/xilinx-versal-cpm.yaml | 105 +++++
 drivers/pci/controller/Kconfig                     |   9 +
 drivers/pci/controller/Makefile                    |   1 +
 drivers/pci/controller/pcie-xilinx-cpm.c           | 506 +++++++++++++++++++++
 4 files changed, 621 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
 create mode 100644 drivers/pci/controller/pcie-xilinx-cpm.c

-- 
2.7.4

[PATCH v7 1/2] PCI: xilinx-cpm: Add YAML schemas for Versal CPM Root Port

[Bharat Kumar Gogada]

Add YAML schemas documentation for Versal CPM Root Port driver.

Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
---
 .../devicetree/bindings/pci/xilinx-versal-cpm.yaml | 105 +++++++++++++++++++++
 1 file changed, 105 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml

diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
new file mode 100644
index 0000000..5fc5c3f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
@@ -0,0 +1,105 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/xilinx-versal-cpm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CPM Host Controller device tree for Xilinx Versal SoCs
+
+maintainers:
+  - Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
+
+properties:
+  compatible:
+    const: xlnx,versal-cpm-host-1.00
+
+  "#address-cells":
+    const: 3
+
+  "#size-cells":
+    const: 2
+
+  reg:
+    items:
+      - description: Configuration space region and bridge registers.
+      - description: CPM system level control and status registers.
+
+  reg-names:
+    items:
+      - const: cfg
+      - const: cpm_slcr
+
+  interrupts:
+    maxItems: 1
+
+  msi-map:
+    description:
+      Maps a Requester ID to an MSI controller and associated MSI sideband data.
+
+  ranges:
+    maxItems: 2
+
+  "#interrupt-cells":
+    const: 1
+
+  interrupt-map-mask:
+    description: Standard PCI IRQ mapping properties.
+
+  interrupt-map:
+    description: Standard PCI IRQ mapping properties.
+
+  interrupt_controller:
+    description: Interrupt controller child node.
+
+  bus-range:
+    description: Range of bus numbers associated with this controller.
+
+required:
+  - compatible
+  - "#address-cells"
+  - "#size-cells"
+  - reg
+  - reg-names
+  - "#interrupt-cells"
+  - interrupts
+  - interrupt-parent
+  - interrupt-map
+  - interrupt-map-mask
+  - ranges
+  - bus-range
+  - msi-map
+
+additionalProperties: false
+
+examples:
+  - |
+
+    versal {
+               #address-cells = <2>;
+               #size-cells = <2>;
+               cpm_pcie: pci@fca10000 {
+                       compatible = "xlnx,versal-cpm-host-1.00";
+                       #address-cells = <3>;
+                       #interrupt-cells = <1>;
+                       #size-cells = <2>;
+                       interrupts = <0 72 4>;
+                       interrupt-parent = <&gic>;
+                       interrupt-map-mask = <0 0 0 7>;
+                       interrupt-map = <0 0 0 1 &pcie_intc_0 0>,
+                                       <0 0 0 2 &pcie_intc_0 1>,
+                                       <0 0 0 3 &pcie_intc_0 2>,
+                                       <0 0 0 4 &pcie_intc_0 3>;
+                       bus-range = <0x00 0xff>;
+                       ranges = <0x02000000 0x0 0xe0000000 0x0 0xe0000000 0x0 0x10000000>,
+                                <0x43000000 0x80 0x00000000 0x80 0x00000000 0x0 0x80000000>;
+                       msi-map = <0x0 &its_gic 0x0 0x10000>;
+                       reg = <0x6 0x00000000 0x0 0x10000000>,
+                             <0x0 0xfca10000 0x0 0x1000>;
+                       reg-names = "cfg", "cpm_slcr";
+                       pcie_intc_0: interrupt_controller {
+                               #address-cells = <0>;
+                               #interrupt-cells = <1>;
+                               interrupt-controller ;
+                       };
+                };
+    };
-- 
2.7.4

RE: [PATCH v7 1/2] PCI: xilinx-cpm: Add YAML schemas for Versal CPM Root Port

[Bharat Kumar Gogada]

Hi Rob,

Can you please let us know if you have any inputs on this.

Regards,
Bharat

> -----Original Message-----
> From: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> Sent: Thursday, May 7, 2020 5:29 PM
> To: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: lorenzo.pieralisi@arm.com; bhelgaas@google.com; robh@kernel.org;
> Ravikiran Gummaluri <rgummal@xilinx.com>; Bharat Kumar Gogada
> <bharatku@xilinx.com>
> Subject: [PATCH v7 1/2] PCI: xilinx-cpm: Add YAML schemas for Versal CPM
> Root Port
> 
> Add YAML schemas documentation for Versal CPM Root Port driver.
> 
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> ---
>  .../devicetree/bindings/pci/xilinx-versal-cpm.yaml | 105
> +++++++++++++++++++++
>  1 file changed, 105 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/xilinx-versal-
> cpm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> new file mode 100644
> index 0000000..5fc5c3f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> @@ -0,0 +1,105 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/xilinx-versal-cpm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CPM Host Controller device tree for Xilinx Versal SoCs
> +
> +maintainers:
> +  - Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> +
> +properties:
> +  compatible:
> +    const: xlnx,versal-cpm-host-1.00
> +
> +  "#address-cells":
> +    const: 3
> +
> +  "#size-cells":
> +    const: 2
> +
> +  reg:
> +    items:
> +      - description: Configuration space region and bridge registers.
> +      - description: CPM system level control and status registers.
> +
> +  reg-names:
> +    items:
> +      - const: cfg
> +      - const: cpm_slcr
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  msi-map:
> +    description:
> +      Maps a Requester ID to an MSI controller and associated MSI sideband
> data.
> +
> +  ranges:
> +    maxItems: 2
> +
> +  "#interrupt-cells":
> +    const: 1
> +
> +  interrupt-map-mask:
> +    description: Standard PCI IRQ mapping properties.
> +
> +  interrupt-map:
> +    description: Standard PCI IRQ mapping properties.
> +
> +  interrupt_controller:
> +    description: Interrupt controller child node.
> +
> +  bus-range:
> +    description: Range of bus numbers associated with this controller.
> +
> +required:
> +  - compatible
> +  - "#address-cells"
> +  - "#size-cells"
> +  - reg
> +  - reg-names
> +  - "#interrupt-cells"
> +  - interrupts
> +  - interrupt-parent
> +  - interrupt-map
> +  - interrupt-map-mask
> +  - ranges
> +  - bus-range
> +  - msi-map
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    versal {
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               cpm_pcie: pci@fca10000 {
> +                       compatible = "xlnx,versal-cpm-host-1.00";
> +                       #address-cells = <3>;
> +                       #interrupt-cells = <1>;
> +                       #size-cells = <2>;
> +                       interrupts = <0 72 4>;
> +                       interrupt-parent = <&gic>;
> +                       interrupt-map-mask = <0 0 0 7>;
> +                       interrupt-map = <0 0 0 1 &pcie_intc_0 0>,
> +                                       <0 0 0 2 &pcie_intc_0 1>,
> +                                       <0 0 0 3 &pcie_intc_0 2>,
> +                                       <0 0 0 4 &pcie_intc_0 3>;
> +                       bus-range = <0x00 0xff>;
> +                       ranges = <0x02000000 0x0 0xe0000000 0x0 0xe0000000 0x0
> 0x10000000>,
> +                                <0x43000000 0x80 0x00000000 0x80 0x00000000 0x0
> 0x80000000>;
> +                       msi-map = <0x0 &its_gic 0x0 0x10000>;
> +                       reg = <0x6 0x00000000 0x0 0x10000000>,
> +                             <0x0 0xfca10000 0x0 0x1000>;
> +                       reg-names = "cfg", "cpm_slcr";
> +                       pcie_intc_0: interrupt_controller {
> +                               #address-cells = <0>;
> +                               #interrupt-cells = <1>;
> +                               interrupt-controller ;
> +                       };
> +                };
> +    };
> --
> 2.7.4

Re: [PATCH v7 1/2] PCI: xilinx-cpm: Add YAML schemas for Versal CPM Root Port

[Rob Herring]

On Thu, May 07, 2020 at 05:28:35PM +0530, Bharat Kumar Gogada wrote:
> Add YAML schemas documentation for Versal CPM Root Port driver.
> 
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> ---
>  .../devicetree/bindings/pci/xilinx-versal-cpm.yaml | 105 +++++++++++++++++++++
>  1 file changed, 105 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> new file mode 100644
> index 0000000..5fc5c3f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> @@ -0,0 +1,105 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/xilinx-versal-cpm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CPM Host Controller device tree for Xilinx Versal SoCs
> +
> +maintainers:
> +  - Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> +

allOf:
  - $ref: /schemas/pci/pci-bus.yaml#

> +properties:
> +  compatible:
> +    const: xlnx,versal-cpm-host-1.00
> +

> +  "#address-cells":
> +    const: 3
> +
> +  "#size-cells":
> +    const: 2

Can drop.

> +
> +  reg:
> +    items:
> +      - description: Configuration space region and bridge registers.
> +      - description: CPM system level control and status registers.
> +
> +  reg-names:
> +    items:
> +      - const: cfg
> +      - const: cpm_slcr
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  msi-map:
> +    description:
> +      Maps a Requester ID to an MSI controller and associated MSI sideband data.
> +
> +  ranges:
> +    maxItems: 2
> +
> +  "#interrupt-cells":
> +    const: 1
> +

> +  interrupt-map-mask:
> +    description: Standard PCI IRQ mapping properties.
> +
> +  interrupt-map:
> +    description: Standard PCI IRQ mapping properties.

Can drop these 2.

> +
> +  interrupt_controller:

s/_/-/

> +    description: Interrupt controller child node.

type: object

And then need to describe all the properties under it too.

> +
> +  bus-range:
> +    description: Range of bus numbers associated with this controller.

Can drop.

> +
> +required:
> +  - compatible
> +  - "#address-cells"
> +  - "#size-cells"
> +  - reg
> +  - reg-names
> +  - "#interrupt-cells"
> +  - interrupts
> +  - interrupt-parent
> +  - interrupt-map
> +  - interrupt-map-mask
> +  - ranges
> +  - bus-range
> +  - msi-map

interrupt-controller node not required?

You can drop all the standard properties required in pci-bus.yaml (it's 
in dtschema repo).

> +
> +additionalProperties: false

This will need to be 'unevaluatedProperties: false'

> +
> +examples:
> +  - |
> +
> +    versal {
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               cpm_pcie: pci@fca10000 {

pcie@...

> +                       compatible = "xlnx,versal-cpm-host-1.00";
> +                       #address-cells = <3>;
> +                       #interrupt-cells = <1>;
> +                       #size-cells = <2>;
> +                       interrupts = <0 72 4>;
> +                       interrupt-parent = <&gic>;
> +                       interrupt-map-mask = <0 0 0 7>;
> +                       interrupt-map = <0 0 0 1 &pcie_intc_0 0>,
> +                                       <0 0 0 2 &pcie_intc_0 1>,
> +                                       <0 0 0 3 &pcie_intc_0 2>,
> +                                       <0 0 0 4 &pcie_intc_0 3>;
> +                       bus-range = <0x00 0xff>;
> +                       ranges = <0x02000000 0x0 0xe0000000 0x0 0xe0000000 0x0 0x10000000>,
> +                                <0x43000000 0x80 0x00000000 0x80 0x00000000 0x0 0x80000000>;
> +                       msi-map = <0x0 &its_gic 0x0 0x10000>;
> +                       reg = <0x6 0x00000000 0x0 0x10000000>,
> +                             <0x0 0xfca10000 0x0 0x1000>;
> +                       reg-names = "cfg", "cpm_slcr";
> +                       pcie_intc_0: interrupt_controller {
> +                               #address-cells = <0>;
> +                               #interrupt-cells = <1>;
> +                               interrupt-controller ;
> +                       };
> +                };
> +    };
> -- 
> 2.7.4
> 

RE: [PATCH v7 1/2] PCI: xilinx-cpm: Add YAML schemas for Versal CPM Root Port

[Bharat Kumar Gogada]

> Subject: Re: [PATCH v7 1/2] PCI: xilinx-cpm: Add YAML schemas for Versal
> CPM Root Port
> 
> On Thu, May 07, 2020 at 05:28:35PM +0530, Bharat Kumar Gogada wrote:
> > Add YAML schemas documentation for Versal CPM Root Port driver.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> > ---
> >  .../devicetree/bindings/pci/xilinx-versal-cpm.yaml | 105
> > +++++++++++++++++++++
> >  1 file changed, 105 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > new file mode 100644
> > index 0000000..5fc5c3f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > @@ -0,0 +1,105 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/xilinx-versal-cpm.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: CPM Host Controller device tree for Xilinx Versal SoCs
> > +
> > +maintainers:
> > +  - Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> > +
> 
> allOf:
>   - $ref: /schemas/pci/pci-bus.yaml#
> 
> > +properties:
> > +  compatible:
> > +    const: xlnx,versal-cpm-host-1.00
> > +
> 
> > +  "#address-cells":
> > +    const: 3
> > +
> > +  "#size-cells":
> > +    const: 2
> 
> Can drop.
> 
> > +
> > +  reg:
> > +    items:
> > +      - description: Configuration space region and bridge registers.
> > +      - description: CPM system level control and status registers.
> > +
> > +  reg-names:
> > +    items:
> > +      - const: cfg
> > +      - const: cpm_slcr
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  msi-map:
> > +    description:
> > +      Maps a Requester ID to an MSI controller and associated MSI
> sideband data.
> > +
> > +  ranges:
> > +    maxItems: 2
> > +
> > +  "#interrupt-cells":
> > +    const: 1
> > +
> 
> > +  interrupt-map-mask:
> > +    description: Standard PCI IRQ mapping properties.
> > +
> > +  interrupt-map:
> > +    description: Standard PCI IRQ mapping properties.
> 
> Can drop these 2.
> 
> > +
> > +  interrupt_controller:
> 
> s/_/-/
> 
> > +    description: Interrupt controller child node.
> 
> type: object
> 
> And then need to describe all the properties under it too.
Agreed, will describe properties under this child node.
> 
> > +
> > +  bus-range:
> > +    description: Range of bus numbers associated with this controller.
> 
> Can drop.
> 
> > +
> > +required:
> > +  - compatible
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +  - reg
> > +  - reg-names
> > +  - "#interrupt-cells"
> > +  - interrupts
> > +  - interrupt-parent
> > +  - interrupt-map
> > +  - interrupt-map-mask
> > +  - ranges
> > +  - bus-range
> > +  - msi-map
> 
> interrupt-controller node not required?
Yes required, will add.
> 
> You can drop all the standard properties required in pci-bus.yaml (it's in
> dtschema repo).
Agreed, will drop.
> 
> > +
> > +additionalProperties: false
> 
> This will need to be 'unevaluatedProperties: false'
Thanks Rob, will fix these in next patch.
Regards,
Bharat
> 
> > +
> > +examples:
> > +  - |
> > +
> > +    versal {
> > +               #address-cells = <2>;
> > +               #size-cells = <2>;
> > +               cpm_pcie: pci@fca10000 {
> 
> pcie@...
> 
> > +                       compatible = "xlnx,versal-cpm-host-1.00";
> > +                       #address-cells = <3>;
> > +                       #interrupt-cells = <1>;
> > +                       #size-cells = <2>;
> > +                       interrupts = <0 72 4>;
> > +                       interrupt-parent = <&gic>;
> > +                       interrupt-map-mask = <0 0 0 7>;
> > +                       interrupt-map = <0 0 0 1 &pcie_intc_0 0>,
> > +                                       <0 0 0 2 &pcie_intc_0 1>,
> > +                                       <0 0 0 3 &pcie_intc_0 2>,
> > +                                       <0 0 0 4 &pcie_intc_0 3>;
> > +                       bus-range = <0x00 0xff>;
> > +                       ranges = <0x02000000 0x0 0xe0000000 0x0 0xe0000000 0x0
> 0x10000000>,
> > +                                <0x43000000 0x80 0x00000000 0x80 0x00000000 0x0
> 0x80000000>;
> > +                       msi-map = <0x0 &its_gic 0x0 0x10000>;
> > +                       reg = <0x6 0x00000000 0x0 0x10000000>,
> > +                             <0x0 0xfca10000 0x0 0x1000>;
> > +                       reg-names = "cfg", "cpm_slcr";
> > +                       pcie_intc_0: interrupt_controller {
> > +                               #address-cells = <0>;
> > +                               #interrupt-cells = <1>;
> > +                               interrupt-controller ;
> > +                       };
> > +                };
> > +    };
> > --
> > 2.7.4
> >

[PATCH v7 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver

[Bharat Kumar Gogada]

- Add support for Versal CPM as Root Port.
- The Versal ACAP devices include CCIX-PCIe Module (CPM). The integrated
  block for CPM along with the integrated bridge can function
  as PCIe Root Port.
- Bridge error and legacy interrupts in Versal CPM are handled using
  Versal CPM specific interrupt line.

Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
---
 drivers/pci/controller/Kconfig           |   9 +
 drivers/pci/controller/Makefile          |   1 +
 drivers/pci/controller/pcie-xilinx-cpm.c | 506 +++++++++++++++++++++++++++++++
 3 files changed, 516 insertions(+)
 create mode 100644 drivers/pci/controller/pcie-xilinx-cpm.c

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 20bf00f..ca0ae24 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -81,6 +81,15 @@ config PCIE_XILINX
 	  Say 'Y' here if you want kernel to support the Xilinx AXI PCIe
 	  Host Bridge driver.
 
+config PCIE_XILINX_CPM
+	bool "Xilinx Versal CPM host bridge support"
+	depends on ARCH_ZYNQMP || COMPILE_TEST
+	select PCI_HOST_COMMON
+	help
+	  Say 'Y' here if you want kernel support for the
+	  Xilinx Versal CPM host bridge. The driver supports
+	  MSI/MSI-X interrupts using GICv3 ITS feature.
+
 config PCI_XGENE
 	bool "X-Gene PCIe controller"
 	depends on ARM64 || COMPILE_TEST
diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
index 01b2502..78dabda 100644
--- a/drivers/pci/controller/Makefile
+++ b/drivers/pci/controller/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_PCI_HOST_COMMON) += pci-host-common.o
 obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
 obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
 obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
+obj-$(CONFIG_PCIE_XILINX_CPM) += pcie-xilinx-cpm.o
 obj-$(CONFIG_PCI_V3_SEMI) += pci-v3-semi.o
 obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
 obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-cpm.c
new file mode 100644
index 0000000..e8c0aa7
--- /dev/null
+++ b/drivers/pci/controller/pcie-xilinx-cpm.c
@@ -0,0 +1,506 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * PCIe host controller driver for Xilinx Versal CPM DMA Bridge
+ *
+ * (C) Copyright 2019 - 2020, Xilinx, Inc.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/of_irq.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/pci-ecam.h>
+
+#include "../pci.h"
+
+/* Register definitions */
+#define XILINX_CPM_PCIE_REG_IDR		0x00000E10
+#define XILINX_CPM_PCIE_REG_IMR		0x00000E14
+#define XILINX_CPM_PCIE_REG_PSCR	0x00000E1C
+#define XILINX_CPM_PCIE_REG_RPSC	0x00000E20
+#define XILINX_CPM_PCIE_REG_RPEFR	0x00000E2C
+#define XILINX_CPM_PCIE_REG_IDRN	0x00000E38
+#define XILINX_CPM_PCIE_REG_IDRN_MASK	0x00000E3C
+#define XILINX_CPM_PCIE_MISC_IR_STATUS	0x00000340
+#define XILINX_CPM_PCIE_MISC_IR_ENABLE	0x00000348
+#define XILINX_CPM_PCIE_MISC_IR_LOCAL	BIT(1)
+
+/* Interrupt registers definitions */
+#define XILINX_CPM_PCIE_INTR_LINK_DOWN		BIT(0)
+#define XILINX_CPM_PCIE_INTR_HOT_RESET		BIT(3)
+#define XILINX_CPM_PCIE_INTR_CFG_TIMEOUT	BIT(8)
+#define XILINX_CPM_PCIE_INTR_CORRECTABLE	BIT(9)
+#define XILINX_CPM_PCIE_INTR_NONFATAL		BIT(10)
+#define XILINX_CPM_PCIE_INTR_FATAL		BIT(11)
+#define XILINX_CPM_PCIE_INTR_INTX		BIT(16)
+#define XILINX_CPM_PCIE_INTR_MSI		BIT(17)
+#define XILINX_CPM_PCIE_INTR_SLV_UNSUPP		BIT(20)
+#define XILINX_CPM_PCIE_INTR_SLV_UNEXP		BIT(21)
+#define XILINX_CPM_PCIE_INTR_SLV_COMPL		BIT(22)
+#define XILINX_CPM_PCIE_INTR_SLV_ERRP		BIT(23)
+#define XILINX_CPM_PCIE_INTR_SLV_CMPABT		BIT(24)
+#define XILINX_CPM_PCIE_INTR_SLV_ILLBUR		BIT(25)
+#define XILINX_CPM_PCIE_INTR_MST_DECERR		BIT(26)
+#define XILINX_CPM_PCIE_INTR_MST_SLVERR		BIT(27)
+#define XILINX_CPM_PCIE_IMR_ALL_MASK		0x1FF39FF9
+#define XILINX_CPM_PCIE_IDR_ALL_MASK		0xFFFFFFFF
+#define XILINX_CPM_PCIE_IDRN_MASK		GENMASK(19, 16)
+#define XILINX_CPM_PCIE_INTR_CFG_PCIE_TIMEOUT	BIT(4)
+#define XILINX_CPM_PCIE_INTR_CFG_ERR_POISON	BIT(12)
+#define XILINX_CPM_PCIE_INTR_PME_TO_ACK_RCVD	BIT(15)
+#define XILINX_CPM_PCIE_INTR_PM_PME_RCVD	BIT(17)
+#define XILINX_CPM_PCIE_INTR_SLV_PCIE_TIMEOUT	BIT(28)
+#define XILINX_CPM_PCIE_IDRN_SHIFT		16
+
+/* Root Port Error FIFO Read Register definitions */
+#define XILINX_CPM_PCIE_RPEFR_ERR_VALID		BIT(18)
+#define XILINX_CPM_PCIE_RPEFR_REQ_ID		GENMASK(15, 0)
+#define XILINX_CPM_PCIE_RPEFR_ALL_MASK		0xFFFFFFFF
+
+/* Root Port Status/control Register definitions */
+#define XILINX_CPM_PCIE_REG_RPSC_BEN		BIT(0)
+
+/* Phy Status/Control Register definitions */
+#define XILINX_CPM_PCIE_REG_PSCR_LNKUP		BIT(11)
+
+/**
+ * struct xilinx_cpm_pcie_port - PCIe port information
+ * @reg_base: Bridge Register Base
+ * @cpm_base: CPM System Level Control and Status Register(SLCR) Base
+ * @dev: Device pointer
+ * @leg_domain: Legacy IRQ domain pointer
+ * @cfg: Holds mappings of config space window
+ * @irq_misc: Legacy and error interrupt number
+ * @leg_mask_lock: lock for legacy interrupts
+ */
+struct xilinx_cpm_pcie_port {
+	void __iomem *reg_base;
+	void __iomem *cpm_base;
+	struct device *dev;
+	struct irq_domain *leg_domain;
+	struct pci_config_window *cfg;
+	int irq_misc;
+	raw_spinlock_t leg_mask_lock;
+};
+
+static inline u32 pcie_read(struct xilinx_cpm_pcie_port *port, u32 reg)
+{
+	return readl(port->reg_base + reg);
+}
+
+static inline void pcie_write(struct xilinx_cpm_pcie_port *port,
+			      u32 val, u32 reg)
+{
+	writel(val, port->reg_base + reg);
+}
+
+static inline bool cpm_pcie_link_up(struct xilinx_cpm_pcie_port *port)
+{
+	return (pcie_read(port, XILINX_CPM_PCIE_REG_PSCR) &
+		XILINX_CPM_PCIE_REG_PSCR_LNKUP) ? 1 : 0;
+}
+
+/**
+ * xilinx_cpm_pcie_clear_err_interrupts - Clear Error Interrupts
+ * @port: PCIe port information
+ */
+static void cpm_pcie_clear_err_interrupts(struct xilinx_cpm_pcie_port *port)
+{
+	unsigned long val = pcie_read(port, XILINX_CPM_PCIE_REG_RPEFR);
+
+	if (val & XILINX_CPM_PCIE_RPEFR_ERR_VALID) {
+		dev_dbg(port->dev, "Requester ID %lu\n",
+			val & XILINX_CPM_PCIE_RPEFR_REQ_ID);
+		pcie_write(port, XILINX_CPM_PCIE_RPEFR_ALL_MASK,
+			   XILINX_CPM_PCIE_REG_RPEFR);
+	}
+}
+
+static void xilinx_cpm_mask_leg_irq(struct irq_data *data)
+{
+	struct irq_desc *desc = irq_to_desc(data->irq);
+	struct xilinx_cpm_pcie_port *port;
+	unsigned long flags;
+	u32 mask;
+	u32 val;
+
+	port = irq_desc_get_chip_data(desc);
+	mask = (1 << data->hwirq) << XILINX_CPM_PCIE_IDRN_SHIFT;
+	raw_spin_lock_irqsave(&port->leg_mask_lock, flags);
+	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN_MASK);
+	pcie_write(port, (val & (~mask)), XILINX_CPM_PCIE_REG_IDRN_MASK);
+	raw_spin_unlock_irqrestore(&port->leg_mask_lock, flags);
+}
+
+static void xilinx_cpm_unmask_leg_irq(struct irq_data *data)
+{
+	struct irq_desc *desc = irq_to_desc(data->irq);
+	struct xilinx_cpm_pcie_port *port;
+	unsigned long flags;
+	u32 mask;
+	u32 val;
+
+	port = irq_desc_get_chip_data(desc);
+	mask = (1 << data->hwirq) << XILINX_CPM_PCIE_IDRN_SHIFT;
+	raw_spin_lock_irqsave(&port->leg_mask_lock, flags);
+	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN_MASK);
+	pcie_write(port, (val | mask), XILINX_CPM_PCIE_REG_IDRN_MASK);
+	raw_spin_unlock_irqrestore(&port->leg_mask_lock, flags);
+}
+
+static struct irq_chip xilinx_cpm_leg_irq_chip = {
+	.name = "xilinx_cpm_pcie:legacy",
+	.irq_enable = xilinx_cpm_unmask_leg_irq,
+	.irq_disable = xilinx_cpm_mask_leg_irq,
+	.irq_mask = xilinx_cpm_mask_leg_irq,
+	.irq_unmask = xilinx_cpm_unmask_leg_irq,
+};
+
+/**
+ * xilinx_cpm_pcie_intx_map - Set the handler for the INTx and mark IRQ as valid
+ * @domain: IRQ domain
+ * @irq: Virtual IRQ number
+ * @hwirq: HW interrupt number
+ *
+ * Return: Always returns 0.
+ */
+static int xilinx_cpm_pcie_intx_map(struct irq_domain *domain,
+				    unsigned int irq, irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &xilinx_cpm_leg_irq_chip,
+				 handle_level_irq);
+	irq_set_chip_data(irq, domain->host_data);
+	irq_set_status_flags(irq, IRQ_LEVEL);
+
+	return 0;
+}
+
+/* INTx IRQ Domain operations */
+static const struct irq_domain_ops intx_domain_ops = {
+	.map = xilinx_cpm_pcie_intx_map,
+};
+
+/**
+ * xilinx_cpm_pcie_intr_handler - Interrupt Service Handler
+ * @irq: IRQ number
+ * @data: PCIe port information
+ *
+ * Return: IRQ_HANDLED on success and IRQ_NONE on failure
+ */
+static irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void *data)
+{
+	struct xilinx_cpm_pcie_port *port = data;
+	struct device *dev = port->dev;
+	u32 val, mask, status, bit;
+	unsigned long intr_val;
+
+	/* Read interrupt decode and mask registers */
+	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDR);
+	mask = pcie_read(port, XILINX_CPM_PCIE_REG_IMR);
+
+	status = val & mask;
+	if (!status)
+		return IRQ_NONE;
+
+	if (status & XILINX_CPM_PCIE_INTR_LINK_DOWN)
+		dev_warn(dev, "Link Down\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_HOT_RESET)
+		dev_info(dev, "Hot reset\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_CFG_TIMEOUT)
+		dev_warn(dev, "ECAM access timeout\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_CORRECTABLE) {
+		dev_warn(dev, "Correctable error message\n");
+		cpm_pcie_clear_err_interrupts(port);
+	}
+
+	if (status & XILINX_CPM_PCIE_INTR_NONFATAL) {
+		dev_warn(dev, "Non fatal error message\n");
+		cpm_pcie_clear_err_interrupts(port);
+	}
+
+	if (status & XILINX_CPM_PCIE_INTR_FATAL) {
+		dev_warn(dev, "Fatal error message\n");
+		cpm_pcie_clear_err_interrupts(port);
+	}
+
+	if (status & XILINX_CPM_PCIE_INTR_INTX) {
+		/* Handle INTx Interrupt */
+		intr_val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN);
+		intr_val = intr_val >> XILINX_CPM_PCIE_IDRN_SHIFT;
+
+		for_each_set_bit(bit, &intr_val, PCI_NUM_INTX)
+			generic_handle_irq(irq_find_mapping(port->leg_domain,
+							    bit));
+	}
+
+	if (status & XILINX_CPM_PCIE_INTR_SLV_UNSUPP)
+		dev_warn(dev, "Slave unsupported request\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_SLV_UNEXP)
+		dev_warn(dev, "Slave unexpected completion\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_SLV_COMPL)
+		dev_warn(dev, "Slave completion timeout\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_SLV_ERRP)
+		dev_warn(dev, "Slave Error Poison\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_SLV_CMPABT)
+		dev_warn(dev, "Slave Completer Abort\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_SLV_ILLBUR)
+		dev_warn(dev, "Slave Illegal Burst\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_MST_DECERR)
+		dev_warn(dev, "Master decode error\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_MST_SLVERR)
+		dev_warn(dev, "Master slave error\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_CFG_PCIE_TIMEOUT)
+		dev_warn(dev, "PCIe ECAM access timeout\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_CFG_ERR_POISON)
+		dev_warn(dev, "ECAM poisoned completion received\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_PME_TO_ACK_RCVD)
+		dev_warn(dev, "PME_TO_ACK message received\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_PM_PME_RCVD)
+		dev_warn(dev, "PM_PME message received\n");
+
+	if (status & XILINX_CPM_PCIE_INTR_SLV_PCIE_TIMEOUT)
+		dev_warn(dev, "PCIe completion timeout received\n");
+
+	/* Clear the Interrupt Decode register */
+	pcie_write(port, status, XILINX_CPM_PCIE_REG_IDR);
+
+	/*
+	 * XILINX_CPM_PCIE_MISC_IR_STATUS register is mapped to
+	 * CPM SLCR block.
+	 */
+	val = readl(port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS);
+	if (val)
+		writel(val, port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * xilinx_cpm_pcie_init_irq_domain - Initialize IRQ domain
+ * @port: PCIe port information
+ *
+ * Return: '0' on success and error value on failure
+ */
+static int xilinx_cpm_pcie_init_irq_domain(struct xilinx_cpm_pcie_port *port)
+{
+	struct device *dev = port->dev;
+	struct device_node *node = dev->of_node;
+	struct device_node *pcie_intc_node;
+
+	/* Setup INTx */
+	pcie_intc_node = of_get_next_child(node, NULL);
+	if (!pcie_intc_node) {
+		dev_err(dev, "No PCIe Intc node found\n");
+		return -EINVAL;
+	}
+
+	port->leg_domain = irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX,
+						 &intx_domain_ops,
+						 port);
+	of_node_put(pcie_intc_node);
+	if (!port->leg_domain) {
+		dev_err(dev, "Failed to get a INTx IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	raw_spin_lock_init(&port->leg_mask_lock);
+	return 0;
+}
+
+/**
+ * xilinx_cpm_pcie_init_port - Initialize hardware
+ * @port: PCIe port information
+ */
+static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie_port *port)
+{
+	if (cpm_pcie_link_up(port))
+		dev_info(port->dev, "PCIe Link is UP\n");
+	else
+		dev_info(port->dev, "PCIe Link is DOWN\n");
+
+	/* Disable all interrupts */
+	pcie_write(port, ~XILINX_CPM_PCIE_IDR_ALL_MASK,
+		   XILINX_CPM_PCIE_REG_IMR);
+
+	/* Clear pending interrupts */
+	pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_IDR) &
+		   XILINX_CPM_PCIE_IMR_ALL_MASK,
+		   XILINX_CPM_PCIE_REG_IDR);
+
+	/* Enable all interrupts */
+	pcie_write(port, XILINX_CPM_PCIE_IMR_ALL_MASK,
+		   XILINX_CPM_PCIE_REG_IMR);
+	pcie_write(port, XILINX_CPM_PCIE_IDRN_MASK,
+		   XILINX_CPM_PCIE_REG_IDRN_MASK);
+
+	/*
+	 * XILINX_CPM_PCIE_MISC_IR_ENABLE register is mapped to
+	 * CPM SLCR block.
+	 */
+	writel(XILINX_CPM_PCIE_MISC_IR_LOCAL,
+	       port->cpm_base + XILINX_CPM_PCIE_MISC_IR_ENABLE);
+	/* Enable the Bridge enable bit */
+	pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_RPSC) |
+		   XILINX_CPM_PCIE_REG_RPSC_BEN,
+		   XILINX_CPM_PCIE_REG_RPSC);
+}
+
+static int xilinx_cpm_request_misc_irq(struct xilinx_cpm_pcie_port *port)
+{
+	struct device *dev = port->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	int err;
+
+	port->irq_misc = platform_get_irq(pdev, 0);
+	if (port->irq_misc <= 0) {
+		dev_err(dev, "Unable to find misc IRQ line\n");
+		return port->irq_misc;
+	}
+
+	err = devm_request_irq(dev, port->irq_misc,
+			       xilinx_cpm_pcie_intr_handler,
+			       IRQF_SHARED | IRQF_NO_THREAD,
+			       "xilinx-pcie", port);
+	if (err) {
+		dev_err(dev, "unable to request misc IRQ line %d\n",
+			port->irq_misc);
+		return err;
+	}
+
+	return 0;
+}
+
+/**
+ * xilinx_cpm_pcie_parse_dt - Parse Device tree
+ * @port: PCIe port information
+ * @bus_range: Bus resource
+ *
+ * Return: '0' on success and error value on failure
+ */
+static int xilinx_cpm_pcie_parse_dt(struct xilinx_cpm_pcie_port *port,
+				    struct resource *bus_range)
+{
+	struct device *dev = port->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct resource *res;
+	int err;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
+	if (!res)
+		return -ENXIO;
+
+	port->cfg = pci_ecam_create(dev, res, bus_range,
+				    &pci_generic_ecam_ops);
+	if (IS_ERR(port->cfg))
+		return PTR_ERR(port->cfg);
+
+	port->reg_base = port->cfg->win;
+
+	port->cpm_base = devm_platform_ioremap_resource_byname(pdev,
+							       "cpm_slcr");
+	if (IS_ERR(port->cpm_base))
+		return PTR_ERR(port->cpm_base);
+
+	err = xilinx_cpm_request_misc_irq(port);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+/**
+ * xilinx_cpm_pcie_probe - Probe function
+ * @pdev: Platform device pointer
+ *
+ * Return: '0' on success and error value on failure
+ */
+static int xilinx_cpm_pcie_probe(struct platform_device *pdev)
+{
+	struct xilinx_cpm_pcie_port *port;
+	struct device *dev = &pdev->dev;
+	struct pci_host_bridge *bridge;
+	struct resource *bus_range;
+	int err;
+
+	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*port));
+	if (!bridge)
+		return -ENODEV;
+
+	port = pci_host_bridge_priv(bridge);
+
+	port->dev = dev;
+
+	err = pci_parse_request_of_pci_ranges(dev, &bridge->windows,
+					      &bridge->dma_ranges, &bus_range);
+	if (err) {
+		dev_err(dev, "Getting bridge resources failed\n");
+		return err;
+	}
+
+	err = xilinx_cpm_pcie_parse_dt(port, bus_range);
+	if (err) {
+		dev_err(dev, "Parsing DT failed\n");
+		return err;
+	}
+
+	xilinx_cpm_pcie_init_port(port);
+
+	err = xilinx_cpm_pcie_init_irq_domain(port);
+	if (err) {
+		dev_err(dev, "Failed creating IRQ Domain\n");
+		return err;
+	}
+
+	bridge->dev.parent = dev;
+	bridge->sysdata = port->cfg;
+	bridge->busnr = port->cfg->busr.start;
+	bridge->ops = &pci_generic_ecam_ops.pci_ops;
+	bridge->map_irq = of_irq_parse_and_map_pci;
+	bridge->swizzle_irq = pci_common_swizzle;
+
+	err = pci_host_probe(bridge);
+	if (err < 0) {
+		irq_domain_remove(port->leg_domain);
+		devm_free_irq(dev, port->irq_misc, port);
+		return err;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id xilinx_cpm_pcie_of_match[] = {
+	{ .compatible = "xlnx,versal-cpm-host-1.00", },
+	{}
+};
+
+static struct platform_driver xilinx_cpm_pcie_driver = {
+	.driver = {
+		.name = "xilinx-cpm-pcie",
+		.of_match_table = xilinx_cpm_pcie_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe = xilinx_cpm_pcie_probe,
+};
+
+builtin_platform_driver(xilinx_cpm_pcie_driver);
-- 
2.7.4

Re: [PATCH v7 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver

[Bjorn Helgaas]

On Thu, May 07, 2020 at 05:28:36PM +0530, Bharat Kumar Gogada wrote:
> - Add support for Versal CPM as Root Port.
> - The Versal ACAP devices include CCIX-PCIe Module (CPM). The integrated
>   block for CPM along with the integrated bridge can function
>   as PCIe Root Port.
> - Bridge error and legacy interrupts in Versal CPM are handled using
>   Versal CPM specific interrupt line.

> +static inline bool cpm_pcie_link_up(struct xilinx_cpm_pcie_port *port)
> +{
> +	return (pcie_read(port, XILINX_CPM_PCIE_REG_PSCR) &
> +		XILINX_CPM_PCIE_REG_PSCR_LNKUP) ? 1 : 0;

Almost all of the *_link_up() functions return "int".  I don't know if
there's really any benefit to using "bool", but if you do, you should
probably return "true" or "false" instead of 1/0.

> +	port->irq_misc = platform_get_irq(pdev, 0);
> +	if (port->irq_misc <= 0) {

Use:

  if (port->irq_misc < 0) {

See https://lore.kernel.org/r/20200501224042.141366-3-helgaas@kernel.org

RE: [PATCH v7 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver

[Bharat Kumar Gogada]

> Subject: Re: [PATCH v7 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver
> 
> On Thu, May 07, 2020 at 05:28:36PM +0530, Bharat Kumar Gogada wrote:
> > - Add support for Versal CPM as Root Port.
> > - The Versal ACAP devices include CCIX-PCIe Module (CPM). The integrated
> >   block for CPM along with the integrated bridge can function
> >   as PCIe Root Port.
> > - Bridge error and legacy interrupts in Versal CPM are handled using
> >   Versal CPM specific interrupt line.
> 
> > +static inline bool cpm_pcie_link_up(struct xilinx_cpm_pcie_port
> > +*port) {
> > +	return (pcie_read(port, XILINX_CPM_PCIE_REG_PSCR) &
> > +		XILINX_CPM_PCIE_REG_PSCR_LNKUP) ? 1 : 0;
> 
> Almost all of the *_link_up() functions return "int".  I don't know if there's really
> any benefit to using "bool", but if you do, you should probably return "true" or
> "false" instead of 1/0.
Hi Bjorn, thanks will fix this.
> 
> > +	port->irq_misc = platform_get_irq(pdev, 0);
> > +	if (port->irq_misc <= 0) {
> 
> Use:
> 
>   if (port->irq_misc < 0) {
> 
> See https://lore.kernel.org/r/20200501224042.141366-3-helgaas@kernel.org
Agreed, will fix this.

Regards,
Bharat

Re: [PATCH v7 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver

[Rob Herring]

On Thu, May 07, 2020 at 05:28:36PM +0530, Bharat Kumar Gogada wrote:
> - Add support for Versal CPM as Root Port.
> - The Versal ACAP devices include CCIX-PCIe Module (CPM). The integrated
>   block for CPM along with the integrated bridge can function
>   as PCIe Root Port.
> - Bridge error and legacy interrupts in Versal CPM are handled using
>   Versal CPM specific interrupt line.
> 
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> ---
>  drivers/pci/controller/Kconfig           |   9 +
>  drivers/pci/controller/Makefile          |   1 +
>  drivers/pci/controller/pcie-xilinx-cpm.c | 506 +++++++++++++++++++++++++++++++
>  3 files changed, 516 insertions(+)
>  create mode 100644 drivers/pci/controller/pcie-xilinx-cpm.c
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 20bf00f..ca0ae24 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -81,6 +81,15 @@ config PCIE_XILINX
>  	  Say 'Y' here if you want kernel to support the Xilinx AXI PCIe
>  	  Host Bridge driver.
>  
> +config PCIE_XILINX_CPM
> +	bool "Xilinx Versal CPM host bridge support"
> +	depends on ARCH_ZYNQMP || COMPILE_TEST
> +	select PCI_HOST_COMMON
> +	help
> +	  Say 'Y' here if you want kernel support for the
> +	  Xilinx Versal CPM host bridge. The driver supports
> +	  MSI/MSI-X interrupts using GICv3 ITS feature.
> +
>  config PCI_XGENE
>  	bool "X-Gene PCIe controller"
>  	depends on ARM64 || COMPILE_TEST
> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
> index 01b2502..78dabda 100644
> --- a/drivers/pci/controller/Makefile
> +++ b/drivers/pci/controller/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_PCI_HOST_COMMON) += pci-host-common.o
>  obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
>  obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
> +obj-$(CONFIG_PCIE_XILINX_CPM) += pcie-xilinx-cpm.o
>  obj-$(CONFIG_PCI_V3_SEMI) += pci-v3-semi.o
>  obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
>  obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
> diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-cpm.c
> new file mode 100644
> index 0000000..e8c0aa7
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-xilinx-cpm.c
> @@ -0,0 +1,506 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * PCIe host controller driver for Xilinx Versal CPM DMA Bridge
> + *
> + * (C) Copyright 2019 - 2020, Xilinx, Inc.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_irq.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/pci-ecam.h>
> +
> +#include "../pci.h"
> +
> +/* Register definitions */
> +#define XILINX_CPM_PCIE_REG_IDR		0x00000E10
> +#define XILINX_CPM_PCIE_REG_IMR		0x00000E14
> +#define XILINX_CPM_PCIE_REG_PSCR	0x00000E1C
> +#define XILINX_CPM_PCIE_REG_RPSC	0x00000E20
> +#define XILINX_CPM_PCIE_REG_RPEFR	0x00000E2C
> +#define XILINX_CPM_PCIE_REG_IDRN	0x00000E38
> +#define XILINX_CPM_PCIE_REG_IDRN_MASK	0x00000E3C
> +#define XILINX_CPM_PCIE_MISC_IR_STATUS	0x00000340
> +#define XILINX_CPM_PCIE_MISC_IR_ENABLE	0x00000348
> +#define XILINX_CPM_PCIE_MISC_IR_LOCAL	BIT(1)
> +
> +/* Interrupt registers definitions */
> +#define XILINX_CPM_PCIE_INTR_LINK_DOWN		BIT(0)
> +#define XILINX_CPM_PCIE_INTR_HOT_RESET		BIT(3)
> +#define XILINX_CPM_PCIE_INTR_CFG_TIMEOUT	BIT(8)
> +#define XILINX_CPM_PCIE_INTR_CORRECTABLE	BIT(9)
> +#define XILINX_CPM_PCIE_INTR_NONFATAL		BIT(10)
> +#define XILINX_CPM_PCIE_INTR_FATAL		BIT(11)
> +#define XILINX_CPM_PCIE_INTR_INTX		BIT(16)
> +#define XILINX_CPM_PCIE_INTR_MSI		BIT(17)
> +#define XILINX_CPM_PCIE_INTR_SLV_UNSUPP		BIT(20)
> +#define XILINX_CPM_PCIE_INTR_SLV_UNEXP		BIT(21)
> +#define XILINX_CPM_PCIE_INTR_SLV_COMPL		BIT(22)
> +#define XILINX_CPM_PCIE_INTR_SLV_ERRP		BIT(23)
> +#define XILINX_CPM_PCIE_INTR_SLV_CMPABT		BIT(24)
> +#define XILINX_CPM_PCIE_INTR_SLV_ILLBUR		BIT(25)
> +#define XILINX_CPM_PCIE_INTR_MST_DECERR		BIT(26)
> +#define XILINX_CPM_PCIE_INTR_MST_SLVERR		BIT(27)
> +#define XILINX_CPM_PCIE_IMR_ALL_MASK		0x1FF39FF9
> +#define XILINX_CPM_PCIE_IDR_ALL_MASK		0xFFFFFFFF
> +#define XILINX_CPM_PCIE_IDRN_MASK		GENMASK(19, 16)
> +#define XILINX_CPM_PCIE_INTR_CFG_PCIE_TIMEOUT	BIT(4)
> +#define XILINX_CPM_PCIE_INTR_CFG_ERR_POISON	BIT(12)
> +#define XILINX_CPM_PCIE_INTR_PME_TO_ACK_RCVD	BIT(15)
> +#define XILINX_CPM_PCIE_INTR_PM_PME_RCVD	BIT(17)
> +#define XILINX_CPM_PCIE_INTR_SLV_PCIE_TIMEOUT	BIT(28)
> +#define XILINX_CPM_PCIE_IDRN_SHIFT		16
> +
> +/* Root Port Error FIFO Read Register definitions */
> +#define XILINX_CPM_PCIE_RPEFR_ERR_VALID		BIT(18)
> +#define XILINX_CPM_PCIE_RPEFR_REQ_ID		GENMASK(15, 0)
> +#define XILINX_CPM_PCIE_RPEFR_ALL_MASK		0xFFFFFFFF
> +
> +/* Root Port Status/control Register definitions */
> +#define XILINX_CPM_PCIE_REG_RPSC_BEN		BIT(0)
> +
> +/* Phy Status/Control Register definitions */
> +#define XILINX_CPM_PCIE_REG_PSCR_LNKUP		BIT(11)
> +
> +/**
> + * struct xilinx_cpm_pcie_port - PCIe port information
> + * @reg_base: Bridge Register Base
> + * @cpm_base: CPM System Level Control and Status Register(SLCR) Base
> + * @dev: Device pointer
> + * @leg_domain: Legacy IRQ domain pointer
> + * @cfg: Holds mappings of config space window
> + * @irq_misc: Legacy and error interrupt number
> + * @leg_mask_lock: lock for legacy interrupts
> + */
> +struct xilinx_cpm_pcie_port {
> +	void __iomem *reg_base;
> +	void __iomem *cpm_base;
> +	struct device *dev;
> +	struct irq_domain *leg_domain;
> +	struct pci_config_window *cfg;
> +	int irq_misc;
> +	raw_spinlock_t leg_mask_lock;
> +};
> +
> +static inline u32 pcie_read(struct xilinx_cpm_pcie_port *port, u32 reg)
> +{
> +	return readl(port->reg_base + reg);
> +}
> +
> +static inline void pcie_write(struct xilinx_cpm_pcie_port *port,
> +			      u32 val, u32 reg)
> +{
> +	writel(val, port->reg_base + reg);
> +}
> +
> +static inline bool cpm_pcie_link_up(struct xilinx_cpm_pcie_port *port)
> +{
> +	return (pcie_read(port, XILINX_CPM_PCIE_REG_PSCR) &
> +		XILINX_CPM_PCIE_REG_PSCR_LNKUP) ? 1 : 0;
> +}
> +
> +/**
> + * xilinx_cpm_pcie_clear_err_interrupts - Clear Error Interrupts
> + * @port: PCIe port information
> + */
> +static void cpm_pcie_clear_err_interrupts(struct xilinx_cpm_pcie_port *port)
> +{
> +	unsigned long val = pcie_read(port, XILINX_CPM_PCIE_REG_RPEFR);
> +
> +	if (val & XILINX_CPM_PCIE_RPEFR_ERR_VALID) {
> +		dev_dbg(port->dev, "Requester ID %lu\n",
> +			val & XILINX_CPM_PCIE_RPEFR_REQ_ID);
> +		pcie_write(port, XILINX_CPM_PCIE_RPEFR_ALL_MASK,
> +			   XILINX_CPM_PCIE_REG_RPEFR);
> +	}
> +}
> +
> +static void xilinx_cpm_mask_leg_irq(struct irq_data *data)
> +{
> +	struct irq_desc *desc = irq_to_desc(data->irq);
> +	struct xilinx_cpm_pcie_port *port;
> +	unsigned long flags;
> +	u32 mask;
> +	u32 val;
> +
> +	port = irq_desc_get_chip_data(desc);
> +	mask = (1 << data->hwirq) << XILINX_CPM_PCIE_IDRN_SHIFT;
> +	raw_spin_lock_irqsave(&port->leg_mask_lock, flags);
> +	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN_MASK);
> +	pcie_write(port, (val & (~mask)), XILINX_CPM_PCIE_REG_IDRN_MASK);
> +	raw_spin_unlock_irqrestore(&port->leg_mask_lock, flags);
> +}
> +
> +static void xilinx_cpm_unmask_leg_irq(struct irq_data *data)
> +{
> +	struct irq_desc *desc = irq_to_desc(data->irq);
> +	struct xilinx_cpm_pcie_port *port;
> +	unsigned long flags;
> +	u32 mask;
> +	u32 val;
> +
> +	port = irq_desc_get_chip_data(desc);
> +	mask = (1 << data->hwirq) << XILINX_CPM_PCIE_IDRN_SHIFT;
> +	raw_spin_lock_irqsave(&port->leg_mask_lock, flags);
> +	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN_MASK);
> +	pcie_write(port, (val | mask), XILINX_CPM_PCIE_REG_IDRN_MASK);
> +	raw_spin_unlock_irqrestore(&port->leg_mask_lock, flags);
> +}
> +
> +static struct irq_chip xilinx_cpm_leg_irq_chip = {
> +	.name = "xilinx_cpm_pcie:legacy",
> +	.irq_enable = xilinx_cpm_unmask_leg_irq,
> +	.irq_disable = xilinx_cpm_mask_leg_irq,
> +	.irq_mask = xilinx_cpm_mask_leg_irq,
> +	.irq_unmask = xilinx_cpm_unmask_leg_irq,
> +};
> +
> +/**
> + * xilinx_cpm_pcie_intx_map - Set the handler for the INTx and mark IRQ as valid
> + * @domain: IRQ domain
> + * @irq: Virtual IRQ number
> + * @hwirq: HW interrupt number
> + *
> + * Return: Always returns 0.
> + */
> +static int xilinx_cpm_pcie_intx_map(struct irq_domain *domain,
> +				    unsigned int irq, irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &xilinx_cpm_leg_irq_chip,
> +				 handle_level_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +	irq_set_status_flags(irq, IRQ_LEVEL);
> +
> +	return 0;
> +}
> +
> +/* INTx IRQ Domain operations */
> +static const struct irq_domain_ops intx_domain_ops = {
> +	.map = xilinx_cpm_pcie_intx_map,
> +};
> +
> +/**
> + * xilinx_cpm_pcie_intr_handler - Interrupt Service Handler
> + * @irq: IRQ number
> + * @data: PCIe port information
> + *
> + * Return: IRQ_HANDLED on success and IRQ_NONE on failure
> + */
> +static irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void *data)
> +{
> +	struct xilinx_cpm_pcie_port *port = data;
> +	struct device *dev = port->dev;
> +	u32 val, mask, status, bit;
> +	unsigned long intr_val;
> +
> +	/* Read interrupt decode and mask registers */
> +	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDR);
> +	mask = pcie_read(port, XILINX_CPM_PCIE_REG_IMR);
> +
> +	status = val & mask;
> +	if (!status)
> +		return IRQ_NONE;
> +
> +	if (status & XILINX_CPM_PCIE_INTR_LINK_DOWN)
> +		dev_warn(dev, "Link Down\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_HOT_RESET)
> +		dev_info(dev, "Hot reset\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_CFG_TIMEOUT)
> +		dev_warn(dev, "ECAM access timeout\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_CORRECTABLE) {
> +		dev_warn(dev, "Correctable error message\n");
> +		cpm_pcie_clear_err_interrupts(port);
> +	}
> +
> +	if (status & XILINX_CPM_PCIE_INTR_NONFATAL) {
> +		dev_warn(dev, "Non fatal error message\n");
> +		cpm_pcie_clear_err_interrupts(port);
> +	}
> +
> +	if (status & XILINX_CPM_PCIE_INTR_FATAL) {
> +		dev_warn(dev, "Fatal error message\n");
> +		cpm_pcie_clear_err_interrupts(port);
> +	}
> +
> +	if (status & XILINX_CPM_PCIE_INTR_INTX) {
> +		/* Handle INTx Interrupt */
> +		intr_val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN);
> +		intr_val = intr_val >> XILINX_CPM_PCIE_IDRN_SHIFT;
> +
> +		for_each_set_bit(bit, &intr_val, PCI_NUM_INTX)
> +			generic_handle_irq(irq_find_mapping(port->leg_domain,
> +							    bit));
> +	}
> +
> +	if (status & XILINX_CPM_PCIE_INTR_SLV_UNSUPP)
> +		dev_warn(dev, "Slave unsupported request\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_SLV_UNEXP)
> +		dev_warn(dev, "Slave unexpected completion\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_SLV_COMPL)
> +		dev_warn(dev, "Slave completion timeout\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_SLV_ERRP)
> +		dev_warn(dev, "Slave Error Poison\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_SLV_CMPABT)
> +		dev_warn(dev, "Slave Completer Abort\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_SLV_ILLBUR)
> +		dev_warn(dev, "Slave Illegal Burst\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_MST_DECERR)
> +		dev_warn(dev, "Master decode error\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_MST_SLVERR)
> +		dev_warn(dev, "Master slave error\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_CFG_PCIE_TIMEOUT)
> +		dev_warn(dev, "PCIe ECAM access timeout\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_CFG_ERR_POISON)
> +		dev_warn(dev, "ECAM poisoned completion received\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_PME_TO_ACK_RCVD)
> +		dev_warn(dev, "PME_TO_ACK message received\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_PM_PME_RCVD)
> +		dev_warn(dev, "PM_PME message received\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_SLV_PCIE_TIMEOUT)
> +		dev_warn(dev, "PCIe completion timeout received\n");
> +
> +	/* Clear the Interrupt Decode register */
> +	pcie_write(port, status, XILINX_CPM_PCIE_REG_IDR);
> +
> +	/*
> +	 * XILINX_CPM_PCIE_MISC_IR_STATUS register is mapped to
> +	 * CPM SLCR block.
> +	 */
> +	val = readl(port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS);
> +	if (val)
> +		writel(val, port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/**
> + * xilinx_cpm_pcie_init_irq_domain - Initialize IRQ domain
> + * @port: PCIe port information
> + *
> + * Return: '0' on success and error value on failure
> + */
> +static int xilinx_cpm_pcie_init_irq_domain(struct xilinx_cpm_pcie_port *port)
> +{
> +	struct device *dev = port->dev;
> +	struct device_node *node = dev->of_node;
> +	struct device_node *pcie_intc_node;
> +
> +	/* Setup INTx */
> +	pcie_intc_node = of_get_next_child(node, NULL);
> +	if (!pcie_intc_node) {
> +		dev_err(dev, "No PCIe Intc node found\n");
> +		return -EINVAL;
> +	}
> +
> +	port->leg_domain = irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX,
> +						 &intx_domain_ops,
> +						 port);
> +	of_node_put(pcie_intc_node);
> +	if (!port->leg_domain) {
> +		dev_err(dev, "Failed to get a INTx IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	raw_spin_lock_init(&port->leg_mask_lock);
> +	return 0;
> +}
> +
> +/**
> + * xilinx_cpm_pcie_init_port - Initialize hardware
> + * @port: PCIe port information
> + */
> +static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie_port *port)
> +{
> +	if (cpm_pcie_link_up(port))
> +		dev_info(port->dev, "PCIe Link is UP\n");
> +	else
> +		dev_info(port->dev, "PCIe Link is DOWN\n");
> +
> +	/* Disable all interrupts */
> +	pcie_write(port, ~XILINX_CPM_PCIE_IDR_ALL_MASK,
> +		   XILINX_CPM_PCIE_REG_IMR);
> +
> +	/* Clear pending interrupts */
> +	pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_IDR) &
> +		   XILINX_CPM_PCIE_IMR_ALL_MASK,
> +		   XILINX_CPM_PCIE_REG_IDR);
> +
> +	/* Enable all interrupts */
> +	pcie_write(port, XILINX_CPM_PCIE_IMR_ALL_MASK,
> +		   XILINX_CPM_PCIE_REG_IMR);
> +	pcie_write(port, XILINX_CPM_PCIE_IDRN_MASK,
> +		   XILINX_CPM_PCIE_REG_IDRN_MASK);
> +
> +	/*
> +	 * XILINX_CPM_PCIE_MISC_IR_ENABLE register is mapped to
> +	 * CPM SLCR block.
> +	 */
> +	writel(XILINX_CPM_PCIE_MISC_IR_LOCAL,
> +	       port->cpm_base + XILINX_CPM_PCIE_MISC_IR_ENABLE);
> +	/* Enable the Bridge enable bit */
> +	pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_RPSC) |
> +		   XILINX_CPM_PCIE_REG_RPSC_BEN,
> +		   XILINX_CPM_PCIE_REG_RPSC);
> +}
> +
> +static int xilinx_cpm_request_misc_irq(struct xilinx_cpm_pcie_port *port)
> +{
> +	struct device *dev = port->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	int err;
> +
> +	port->irq_misc = platform_get_irq(pdev, 0);
> +	if (port->irq_misc <= 0) {
> +		dev_err(dev, "Unable to find misc IRQ line\n");
> +		return port->irq_misc;
> +	}
> +
> +	err = devm_request_irq(dev, port->irq_misc,
> +			       xilinx_cpm_pcie_intr_handler,
> +			       IRQF_SHARED | IRQF_NO_THREAD,
> +			       "xilinx-pcie", port);
> +	if (err) {
> +		dev_err(dev, "unable to request misc IRQ line %d\n",
> +			port->irq_misc);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * xilinx_cpm_pcie_parse_dt - Parse Device tree
> + * @port: PCIe port information
> + * @bus_range: Bus resource
> + *
> + * Return: '0' on success and error value on failure
> + */
> +static int xilinx_cpm_pcie_parse_dt(struct xilinx_cpm_pcie_port *port,
> +				    struct resource *bus_range)
> +{
> +	struct device *dev = port->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct resource *res;
> +	int err;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
> +	if (!res)
> +		return -ENXIO;
> +
> +	port->cfg = pci_ecam_create(dev, res, bus_range,
> +				    &pci_generic_ecam_ops);
> +	if (IS_ERR(port->cfg))
> +		return PTR_ERR(port->cfg);

Any errors after this point need to call pci_ecam_free(). Maybe this can 
be done later?

I think you can rework this to use pci_host_common_probe() instead. 
You'll need an .init() hook.

> +
> +	port->reg_base = port->cfg->win;
> +
> +	port->cpm_base = devm_platform_ioremap_resource_byname(pdev,
> +							       "cpm_slcr");
> +	if (IS_ERR(port->cpm_base))
> +		return PTR_ERR(port->cpm_base);
> +
> +	err = xilinx_cpm_request_misc_irq(port);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +/**
> + * xilinx_cpm_pcie_probe - Probe function
> + * @pdev: Platform device pointer
> + *
> + * Return: '0' on success and error value on failure
> + */
> +static int xilinx_cpm_pcie_probe(struct platform_device *pdev)
> +{
> +	struct xilinx_cpm_pcie_port *port;
> +	struct device *dev = &pdev->dev;
> +	struct pci_host_bridge *bridge;
> +	struct resource *bus_range;
> +	int err;
> +
> +	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*port));
> +	if (!bridge)
> +		return -ENODEV;
> +
> +	port = pci_host_bridge_priv(bridge);
> +
> +	port->dev = dev;
> +
> +	err = pci_parse_request_of_pci_ranges(dev, &bridge->windows,
> +					      &bridge->dma_ranges, &bus_range);
> +	if (err) {
> +		dev_err(dev, "Getting bridge resources failed\n");
> +		return err;
> +	}
> +
> +	err = xilinx_cpm_pcie_parse_dt(port, bus_range);
> +	if (err) {
> +		dev_err(dev, "Parsing DT failed\n");
> +		return err;
> +	}
> +
> +	xilinx_cpm_pcie_init_port(port);
> +
> +	err = xilinx_cpm_pcie_init_irq_domain(port);
> +	if (err) {
> +		dev_err(dev, "Failed creating IRQ Domain\n");
> +		return err;
> +	}
> +
> +	bridge->dev.parent = dev;
> +	bridge->sysdata = port->cfg;
> +	bridge->busnr = port->cfg->busr.start;
> +	bridge->ops = &pci_generic_ecam_ops.pci_ops;
> +	bridge->map_irq = of_irq_parse_and_map_pci;
> +	bridge->swizzle_irq = pci_common_swizzle;
> +
> +	err = pci_host_probe(bridge);
> +	if (err < 0) {
> +		irq_domain_remove(port->leg_domain);
> +		devm_free_irq(dev, port->irq_misc, port);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id xilinx_cpm_pcie_of_match[] = {
> +	{ .compatible = "xlnx,versal-cpm-host-1.00", },
> +	{}
> +};
> +
> +static struct platform_driver xilinx_cpm_pcie_driver = {
> +	.driver = {
> +		.name = "xilinx-cpm-pcie",
> +		.of_match_table = xilinx_cpm_pcie_of_match,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe = xilinx_cpm_pcie_probe,
> +};
> +
> +builtin_platform_driver(xilinx_cpm_pcie_driver);
> -- 
> 2.7.4
> 

Re: [PATCH v7 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver

[Marc Zyngier]

Bharat,

On 2020-05-07 12:58, Bharat Kumar Gogada wrote:
> - Add support for Versal CPM as Root Port.
> - The Versal ACAP devices include CCIX-PCIe Module (CPM). The 
> integrated
>   block for CPM along with the integrated bridge can function
>   as PCIe Root Port.
> - Bridge error and legacy interrupts in Versal CPM are handled using
>   Versal CPM specific interrupt line.
> 
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> ---
>  drivers/pci/controller/Kconfig           |   9 +
>  drivers/pci/controller/Makefile          |   1 +
>  drivers/pci/controller/pcie-xilinx-cpm.c | 506 
> +++++++++++++++++++++++++++++++
>  3 files changed, 516 insertions(+)
>  create mode 100644 drivers/pci/controller/pcie-xilinx-cpm.c
> 
> diff --git a/drivers/pci/controller/Kconfig 
> b/drivers/pci/controller/Kconfig
> index 20bf00f..ca0ae24 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -81,6 +81,15 @@ config PCIE_XILINX
>  	  Say 'Y' here if you want kernel to support the Xilinx AXI PCIe
>  	  Host Bridge driver.
> 
> +config PCIE_XILINX_CPM
> +	bool "Xilinx Versal CPM host bridge support"
> +	depends on ARCH_ZYNQMP || COMPILE_TEST
> +	select PCI_HOST_COMMON
> +	help
> +	  Say 'Y' here if you want kernel support for the
> +	  Xilinx Versal CPM host bridge. The driver supports
> +	  MSI/MSI-X interrupts using GICv3 ITS feature.

I don't think this last sentense makes any sense here. We usually don't 
mention things that the driver *doesn't* implement.

> +
>  config PCI_XGENE
>  	bool "X-Gene PCIe controller"
>  	depends on ARM64 || COMPILE_TEST
> diff --git a/drivers/pci/controller/Makefile 
> b/drivers/pci/controller/Makefile
> index 01b2502..78dabda 100644
> --- a/drivers/pci/controller/Makefile
> +++ b/drivers/pci/controller/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_PCI_HOST_COMMON) += pci-host-common.o
>  obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
>  obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
> +obj-$(CONFIG_PCIE_XILINX_CPM) += pcie-xilinx-cpm.o
>  obj-$(CONFIG_PCI_V3_SEMI) += pci-v3-semi.o
>  obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
>  obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
> diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c
> b/drivers/pci/controller/pcie-xilinx-cpm.c
> new file mode 100644
> index 0000000..e8c0aa7
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-xilinx-cpm.c
> @@ -0,0 +1,506 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * PCIe host controller driver for Xilinx Versal CPM DMA Bridge
> + *
> + * (C) Copyright 2019 - 2020, Xilinx, Inc.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_irq.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/pci-ecam.h>
> +
> +#include "../pci.h"
> +
> +/* Register definitions */
> +#define XILINX_CPM_PCIE_REG_IDR		0x00000E10
> +#define XILINX_CPM_PCIE_REG_IMR		0x00000E14
> +#define XILINX_CPM_PCIE_REG_PSCR	0x00000E1C
> +#define XILINX_CPM_PCIE_REG_RPSC	0x00000E20
> +#define XILINX_CPM_PCIE_REG_RPEFR	0x00000E2C
> +#define XILINX_CPM_PCIE_REG_IDRN	0x00000E38
> +#define XILINX_CPM_PCIE_REG_IDRN_MASK	0x00000E3C
> +#define XILINX_CPM_PCIE_MISC_IR_STATUS	0x00000340
> +#define XILINX_CPM_PCIE_MISC_IR_ENABLE	0x00000348
> +#define XILINX_CPM_PCIE_MISC_IR_LOCAL	BIT(1)
> +
> +/* Interrupt registers definitions */
> +#define XILINX_CPM_PCIE_INTR_LINK_DOWN		BIT(0)
> +#define XILINX_CPM_PCIE_INTR_HOT_RESET		BIT(3)
> +#define XILINX_CPM_PCIE_INTR_CFG_TIMEOUT	BIT(8)
> +#define XILINX_CPM_PCIE_INTR_CORRECTABLE	BIT(9)
> +#define XILINX_CPM_PCIE_INTR_NONFATAL		BIT(10)
> +#define XILINX_CPM_PCIE_INTR_FATAL		BIT(11)
> +#define XILINX_CPM_PCIE_INTR_INTX		BIT(16)
> +#define XILINX_CPM_PCIE_INTR_MSI		BIT(17)
> +#define XILINX_CPM_PCIE_INTR_SLV_UNSUPP		BIT(20)
> +#define XILINX_CPM_PCIE_INTR_SLV_UNEXP		BIT(21)
> +#define XILINX_CPM_PCIE_INTR_SLV_COMPL		BIT(22)
> +#define XILINX_CPM_PCIE_INTR_SLV_ERRP		BIT(23)
> +#define XILINX_CPM_PCIE_INTR_SLV_CMPABT		BIT(24)
> +#define XILINX_CPM_PCIE_INTR_SLV_ILLBUR		BIT(25)
> +#define XILINX_CPM_PCIE_INTR_MST_DECERR		BIT(26)
> +#define XILINX_CPM_PCIE_INTR_MST_SLVERR		BIT(27)
> +#define XILINX_CPM_PCIE_IMR_ALL_MASK		0x1FF39FF9

I assume that this is the logical OR of all the XILINX_CPM_PCIE_INTR_* 
bits. Please express it as such.

> +#define XILINX_CPM_PCIE_IDR_ALL_MASK		0xFFFFFFFF
> +#define XILINX_CPM_PCIE_IDRN_MASK		GENMASK(19, 16)
> +#define XILINX_CPM_PCIE_INTR_CFG_PCIE_TIMEOUT	BIT(4)
> +#define XILINX_CPM_PCIE_INTR_CFG_ERR_POISON	BIT(12)
> +#define XILINX_CPM_PCIE_INTR_PME_TO_ACK_RCVD	BIT(15)
> +#define XILINX_CPM_PCIE_INTR_PM_PME_RCVD	BIT(17)

So we have two definitions for bit 17... Given that nothing uses the MSI 
version, I assume it is useless...

> +#define XILINX_CPM_PCIE_INTR_SLV_PCIE_TIMEOUT	BIT(28)

Please group all the XILINX_CPM_PCIE_INTR_* together.

> +#define XILINX_CPM_PCIE_IDRN_SHIFT		16
> +
> +/* Root Port Error FIFO Read Register definitions */
> +#define XILINX_CPM_PCIE_RPEFR_ERR_VALID		BIT(18)
> +#define XILINX_CPM_PCIE_RPEFR_REQ_ID		GENMASK(15, 0)
> +#define XILINX_CPM_PCIE_RPEFR_ALL_MASK		0xFFFFFFFF
> +
> +/* Root Port Status/control Register definitions */
> +#define XILINX_CPM_PCIE_REG_RPSC_BEN		BIT(0)
> +
> +/* Phy Status/Control Register definitions */
> +#define XILINX_CPM_PCIE_REG_PSCR_LNKUP		BIT(11)
> +
> +/**
> + * struct xilinx_cpm_pcie_port - PCIe port information
> + * @reg_base: Bridge Register Base
> + * @cpm_base: CPM System Level Control and Status Register(SLCR) Base
> + * @dev: Device pointer
> + * @leg_domain: Legacy IRQ domain pointer

leg, arm, thumb, limb... Given that oyu use the 'intx' description all 
over the driver, please be consistent and name this intx_domain.

> + * @cfg: Holds mappings of config space window
> + * @irq_misc: Legacy and error interrupt number

Let's face it, this is the *only* interrupt this thing as, so the 'misc' 
is supperfluous.

> + * @leg_mask_lock: lock for legacy interrupts
> + */
> +struct xilinx_cpm_pcie_port {
> +	void __iomem *reg_base;
> +	void __iomem *cpm_base;
> +	struct device *dev;
> +	struct irq_domain *leg_domain;
> +	struct pci_config_window *cfg;
> +	int irq_misc;
> +	raw_spinlock_t leg_mask_lock;
> +};
> +
> +static inline u32 pcie_read(struct xilinx_cpm_pcie_port *port, u32 
> reg)
> +{
> +	return readl(port->reg_base + reg);

There is no need for enforced ordering with non-device writes, so you 
can turn this into readl_relaxed. You can also drop the inline, as the 
compiler will do the right thing for you.

> +}
> +
> +static inline void pcie_write(struct xilinx_cpm_pcie_port *port,
> +			      u32 val, u32 reg)
> +{
> +	writel(val, port->reg_base + reg);

Same thing.

> +}
> +
> +static inline bool cpm_pcie_link_up(struct xilinx_cpm_pcie_port *port)
> +{
> +	return (pcie_read(port, XILINX_CPM_PCIE_REG_PSCR) &
> +		XILINX_CPM_PCIE_REG_PSCR_LNKUP) ? 1 : 0;

If you are making the return value a bool, you might as well return 
true/false. But that's a cumbersome way to write:

         return pcie_read(port, XILINX_CPM_PCIE_REG_PSCR) &
                XILINX_CPM_PCIE_REG_PSCR_LNKUP;

> +}
> +
> +/**
> + * xilinx_cpm_pcie_clear_err_interrupts - Clear Error Interrupts
> + * @port: PCIe port information
> + */
> +static void cpm_pcie_clear_err_interrupts(struct xilinx_cpm_pcie_port 
> *port)
> +{
> +	unsigned long val = pcie_read(port, XILINX_CPM_PCIE_REG_RPEFR);
> +
> +	if (val & XILINX_CPM_PCIE_RPEFR_ERR_VALID) {
> +		dev_dbg(port->dev, "Requester ID %lu\n",
> +			val & XILINX_CPM_PCIE_RPEFR_REQ_ID);
> +		pcie_write(port, XILINX_CPM_PCIE_RPEFR_ALL_MASK,
> +			   XILINX_CPM_PCIE_REG_RPEFR);
> +	}
> +}
> +
> +static void xilinx_cpm_mask_leg_irq(struct irq_data *data)
> +{
> +	struct irq_desc *desc = irq_to_desc(data->irq);
> +	struct xilinx_cpm_pcie_port *port;
> +	unsigned long flags;
> +	u32 mask;
> +	u32 val;
> +
> +	port = irq_desc_get_chip_data(desc);

port = irq_data_get_irq_chip_data(data);

You should never have to get the irq_desc (and using irq_to_desc() is a 
prettyodd way to get it when you already have the irq_data).

> +	mask = (1 << data->hwirq) << XILINX_CPM_PCIE_IDRN_SHIFT;

Also known as BIT(data->hwirq + XILINX_CPM_PCIE_IDRN_SHIFT).

> +	raw_spin_lock_irqsave(&port->leg_mask_lock, flags);
> +	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN_MASK);
> +	pcie_write(port, (val & (~mask)), XILINX_CPM_PCIE_REG_IDRN_MASK);
> +	raw_spin_unlock_irqrestore(&port->leg_mask_lock, flags);
> +}
> +
> +static void xilinx_cpm_unmask_leg_irq(struct irq_data *data)
> +{
> +	struct irq_desc *desc = irq_to_desc(data->irq);
> +	struct xilinx_cpm_pcie_port *port;
> +	unsigned long flags;
> +	u32 mask;
> +	u32 val;
> +
> +	port = irq_desc_get_chip_data(desc);
> +	mask = (1 << data->hwirq) << XILINX_CPM_PCIE_IDRN_SHIFT;
> +	raw_spin_lock_irqsave(&port->leg_mask_lock, flags);
> +	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN_MASK);
> +	pcie_write(port, (val | mask), XILINX_CPM_PCIE_REG_IDRN_MASK);
> +	raw_spin_unlock_irqrestore(&port->leg_mask_lock, flags);
> +}
> +
> +static struct irq_chip xilinx_cpm_leg_irq_chip = {
> +	.name = "xilinx_cpm_pcie:legacy",

"INTx" is a good enough description.

> +	.irq_enable = xilinx_cpm_unmask_leg_irq,
> +	.irq_disable = xilinx_cpm_mask_leg_irq,
> +	.irq_mask = xilinx_cpm_mask_leg_irq,
> +	.irq_unmask = xilinx_cpm_unmask_leg_irq,

This makes no sense. If enable/disable have the same implementation as 
unmask/mask, then enable/disable is pretty useless. Please drop them.

> +};
> +
> +/**
> + * xilinx_cpm_pcie_intx_map - Set the handler for the INTx and mark
> IRQ as valid
> + * @domain: IRQ domain
> + * @irq: Virtual IRQ number
> + * @hwirq: HW interrupt number
> + *
> + * Return: Always returns 0.
> + */
> +static int xilinx_cpm_pcie_intx_map(struct irq_domain *domain,
> +				    unsigned int irq, irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &xilinx_cpm_leg_irq_chip,
> +				 handle_level_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +	irq_set_status_flags(irq, IRQ_LEVEL);
> +
> +	return 0;
> +}
> +
> +/* INTx IRQ Domain operations */
> +static const struct irq_domain_ops intx_domain_ops = {
> +	.map = xilinx_cpm_pcie_intx_map,
> +};
> +
> +/**
> + * xilinx_cpm_pcie_intr_handler - Interrupt Service Handler
> + * @irq: IRQ number
> + * @data: PCIe port information
> + *
> + * Return: IRQ_HANDLED on success and IRQ_NONE on failure
> + */
> +static irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void *data)
> +{
> +	struct xilinx_cpm_pcie_port *port = data;
> +	struct device *dev = port->dev;
> +	u32 val, mask, status, bit;
> +	unsigned long intr_val;
> +
> +	/* Read interrupt decode and mask registers */
> +	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDR);
> +	mask = pcie_read(port, XILINX_CPM_PCIE_REG_IMR);
> +
> +	status = val & mask;
> +	if (!status)
> +		return IRQ_NONE;
> +
> +	if (status & XILINX_CPM_PCIE_INTR_LINK_DOWN)
> +		dev_warn(dev, "Link Down\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_HOT_RESET)
> +		dev_info(dev, "Hot reset\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_CFG_TIMEOUT)
> +		dev_warn(dev, "ECAM access timeout\n");

There is a certain sense of repetition here. It really begs the question 
of *why* you want to take these interrupts if all you do is spam the 
console with warnings, not taking any action to potentially remedy the 
problem.

> +
> +	if (status & XILINX_CPM_PCIE_INTR_CORRECTABLE) {
> +		dev_warn(dev, "Correctable error message\n");
> +		cpm_pcie_clear_err_interrupts(port);
> +	}
> +
> +	if (status & XILINX_CPM_PCIE_INTR_NONFATAL) {
> +		dev_warn(dev, "Non fatal error message\n");
> +		cpm_pcie_clear_err_interrupts(port);
> +	}
> +
> +	if (status & XILINX_CPM_PCIE_INTR_FATAL) {
> +		dev_warn(dev, "Fatal error message\n");
> +		cpm_pcie_clear_err_interrupts(port);
> +	}
> +
> +	if (status & XILINX_CPM_PCIE_INTR_INTX) {
> +		/* Handle INTx Interrupt */
> +		intr_val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN);
> +		intr_val = intr_val >> XILINX_CPM_PCIE_IDRN_SHIFT;
> +
> +		for_each_set_bit(bit, &intr_val, PCI_NUM_INTX)
> +			generic_handle_irq(irq_find_mapping(port->leg_domain,
> +							    bit));

That's a firm no. We don't demux chained handlers in an interrupt 
handler. It may work for now, but it will eventually break. And to be 
clear, this whole function needs to die. By the look of it, this PCie 
controller implements *TWO* interrupt multiplexers:

- one that muxes all the ILINX_CPM_PCIE_INTR_* events onto a single 
top-level IRQ
- another one that muxes all INTX lines onto XILINX_CPM_PCIE_INTR_INTX

Please implement the whole thing as described above.

> +	}
> +
> +	if (status & XILINX_CPM_PCIE_INTR_SLV_UNSUPP)
> +		dev_warn(dev, "Slave unsupported request\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_SLV_UNEXP)
> +		dev_warn(dev, "Slave unexpected completion\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_SLV_COMPL)
> +		dev_warn(dev, "Slave completion timeout\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_SLV_ERRP)
> +		dev_warn(dev, "Slave Error Poison\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_SLV_CMPABT)
> +		dev_warn(dev, "Slave Completer Abort\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_SLV_ILLBUR)
> +		dev_warn(dev, "Slave Illegal Burst\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_MST_DECERR)
> +		dev_warn(dev, "Master decode error\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_MST_SLVERR)
> +		dev_warn(dev, "Master slave error\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_CFG_PCIE_TIMEOUT)
> +		dev_warn(dev, "PCIe ECAM access timeout\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_CFG_ERR_POISON)
> +		dev_warn(dev, "ECAM poisoned completion received\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_PME_TO_ACK_RCVD)
> +		dev_warn(dev, "PME_TO_ACK message received\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_PM_PME_RCVD)
> +		dev_warn(dev, "PM_PME message received\n");
> +
> +	if (status & XILINX_CPM_PCIE_INTR_SLV_PCIE_TIMEOUT)
> +		dev_warn(dev, "PCIe completion timeout received\n");

I'm pretty sure there is a slightly better way to write this...

> +
> +	/* Clear the Interrupt Decode register */
> +	pcie_write(port, status, XILINX_CPM_PCIE_REG_IDR);
> +
> +	/*
> +	 * XILINX_CPM_PCIE_MISC_IR_STATUS register is mapped to
> +	 * CPM SLCR block.
> +	 */
> +	val = readl(port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS);
> +	if (val)
> +		writel(val, port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/**
> + * xilinx_cpm_pcie_init_irq_domain - Initialize IRQ domain
> + * @port: PCIe port information
> + *
> + * Return: '0' on success and error value on failure
> + */
> +static int xilinx_cpm_pcie_init_irq_domain(struct xilinx_cpm_pcie_port 
> *port)
> +{
> +	struct device *dev = port->dev;
> +	struct device_node *node = dev->of_node;
> +	struct device_node *pcie_intc_node;
> +
> +	/* Setup INTx */
> +	pcie_intc_node = of_get_next_child(node, NULL);
> +	if (!pcie_intc_node) {
> +		dev_err(dev, "No PCIe Intc node found\n");
> +		return -EINVAL;
> +	}
> +
> +	port->leg_domain = irq_domain_add_linear(pcie_intc_node, 
> PCI_NUM_INTX,
> +						 &intx_domain_ops,
> +						 port);
> +	of_node_put(pcie_intc_node);
> +	if (!port->leg_domain) {
> +		dev_err(dev, "Failed to get a INTx IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	raw_spin_lock_init(&port->leg_mask_lock);
> +	return 0;
> +}
> +
> +/**
> + * xilinx_cpm_pcie_init_port - Initialize hardware
> + * @port: PCIe port information
> + */
> +static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie_port 
> *port)
> +{
> +	if (cpm_pcie_link_up(port))
> +		dev_info(port->dev, "PCIe Link is UP\n");
> +	else
> +		dev_info(port->dev, "PCIe Link is DOWN\n");
> +
> +	/* Disable all interrupts */
> +	pcie_write(port, ~XILINX_CPM_PCIE_IDR_ALL_MASK,
> +		   XILINX_CPM_PCIE_REG_IMR);
> +
> +	/* Clear pending interrupts */
> +	pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_IDR) &
> +		   XILINX_CPM_PCIE_IMR_ALL_MASK,
> +		   XILINX_CPM_PCIE_REG_IDR);
> +
> +	/* Enable all interrupts */
> +	pcie_write(port, XILINX_CPM_PCIE_IMR_ALL_MASK,
> +		   XILINX_CPM_PCIE_REG_IMR);
> +	pcie_write(port, XILINX_CPM_PCIE_IDRN_MASK,
> +		   XILINX_CPM_PCIE_REG_IDRN_MASK);

No. We don't enable interrupts randomly. They need a handler registered 
with the core IRq subsystem *first*, which is why this needs to be 
hooked in as a proper irqchip, and each event handled individualy.

> +
> +	/*
> +	 * XILINX_CPM_PCIE_MISC_IR_ENABLE register is mapped to
> +	 * CPM SLCR block.
> +	 */
> +	writel(XILINX_CPM_PCIE_MISC_IR_LOCAL,
> +	       port->cpm_base + XILINX_CPM_PCIE_MISC_IR_ENABLE);
> +	/* Enable the Bridge enable bit */
> +	pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_RPSC) |
> +		   XILINX_CPM_PCIE_REG_RPSC_BEN,
> +		   XILINX_CPM_PCIE_REG_RPSC);
> +}
> +
> +static int xilinx_cpm_request_misc_irq(struct xilinx_cpm_pcie_port 
> *port)
> +{
> +	struct device *dev = port->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	int err;
> +
> +	port->irq_misc = platform_get_irq(pdev, 0);
> +	if (port->irq_misc <= 0) {

If 0 is an error, how do you distinguish it from the non-error case?

> +		dev_err(dev, "Unable to find misc IRQ line\n");
> +		return port->irq_misc;
> +	}
> +
> +	err = devm_request_irq(dev, port->irq_misc,
> +			       xilinx_cpm_pcie_intr_handler,
> +			       IRQF_SHARED | IRQF_NO_THREAD,
> +			       "xilinx-pcie", port);
> +	if (err) {
> +		dev_err(dev, "unable to request misc IRQ line %d\n",
> +			port->irq_misc);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * xilinx_cpm_pcie_parse_dt - Parse Device tree
> + * @port: PCIe port information
> + * @bus_range: Bus resource
> + *
> + * Return: '0' on success and error value on failure
> + */
> +static int xilinx_cpm_pcie_parse_dt(struct xilinx_cpm_pcie_port *port,
> +				    struct resource *bus_range)
> +{
> +	struct device *dev = port->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct resource *res;
> +	int err;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
> +	if (!res)
> +		return -ENXIO;
> +
> +	port->cfg = pci_ecam_create(dev, res, bus_range,
> +				    &pci_generic_ecam_ops);
> +	if (IS_ERR(port->cfg))
> +		return PTR_ERR(port->cfg);
> +
> +	port->reg_base = port->cfg->win;
> +
> +	port->cpm_base = devm_platform_ioremap_resource_byname(pdev,
> +							       "cpm_slcr");
> +	if (IS_ERR(port->cpm_base))
> +		return PTR_ERR(port->cpm_base);
> +
> +	err = xilinx_cpm_request_misc_irq(port);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +/**
> + * xilinx_cpm_pcie_probe - Probe function
> + * @pdev: Platform device pointer
> + *
> + * Return: '0' on success and error value on failure
> + */
> +static int xilinx_cpm_pcie_probe(struct platform_device *pdev)
> +{
> +	struct xilinx_cpm_pcie_port *port;
> +	struct device *dev = &pdev->dev;
> +	struct pci_host_bridge *bridge;
> +	struct resource *bus_range;
> +	int err;
> +
> +	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*port));
> +	if (!bridge)
> +		return -ENODEV;
> +
> +	port = pci_host_bridge_priv(bridge);
> +
> +	port->dev = dev;
> +
> +	err = pci_parse_request_of_pci_ranges(dev, &bridge->windows,
> +					      &bridge->dma_ranges, &bus_range);
> +	if (err) {
> +		dev_err(dev, "Getting bridge resources failed\n");
> +		return err;
> +	}
> +
> +	err = xilinx_cpm_pcie_parse_dt(port, bus_range);
> +	if (err) {
> +		dev_err(dev, "Parsing DT failed\n");
> +		return err;
> +	}
> +
> +	xilinx_cpm_pcie_init_port(port);
> +
> +	err = xilinx_cpm_pcie_init_irq_domain(port);
> +	if (err) {
> +		dev_err(dev, "Failed creating IRQ Domain\n");
> +		return err;
> +	}
> +
> +	bridge->dev.parent = dev;
> +	bridge->sysdata = port->cfg;
> +	bridge->busnr = port->cfg->busr.start;
> +	bridge->ops = &pci_generic_ecam_ops.pci_ops;
> +	bridge->map_irq = of_irq_parse_and_map_pci;
> +	bridge->swizzle_irq = pci_common_swizzle;
> +
> +	err = pci_host_probe(bridge);
> +	if (err < 0) {
> +		irq_domain_remove(port->leg_domain);
> +		devm_free_irq(dev, port->irq_misc, port);

Why calling dem_free_irq()? The whole point of using devm* is not to 
have to manage the error handling.

> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id xilinx_cpm_pcie_of_match[] = {
> +	{ .compatible = "xlnx,versal-cpm-host-1.00", },
> +	{}
> +};
> +
> +static struct platform_driver xilinx_cpm_pcie_driver = {
> +	.driver = {
> +		.name = "xilinx-cpm-pcie",
> +		.of_match_table = xilinx_cpm_pcie_of_match,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe = xilinx_cpm_pcie_probe,
> +};
> +
> +builtin_platform_driver(xilinx_cpm_pcie_driver);

I don't think this driver is in a state where it can be merged. Given 
that we're already at v7 and to save everyone a bit of time, I've 
written the patch that implements the above comments. It compiles, but 
is probably broken. Hopefully you can test it and figure things out.

         M.

 From 8b5d158374e59edf26e61c512eeb00ca7c9d891d Mon Sep 17 00:00:00 2001
 From: Marc Zyngier <maz@kernel.org>
Date: Fri, 22 May 2020 16:33:32 +0100
Subject: [PATCH] PCI: xilinx-cpm: Revamp irq handling

The xilinx-cpm driver missuses the IRQ layer in creative ways.
It implements a chained interrupt demultiplexer inside a normal
handler, enables interrupts randomly, and could overall do with
a good cleanup.

Instead, implement the IRQ support as two nested chained irqchips,
the outer one dealing with all the PCIe events, the inner one
namaging the INTx signals.

Additionally, the whole driver is cleanup-up so that it can make
a bit more sense to me. YMMV.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
  drivers/pci/controller/Kconfig           |   3 +-
  drivers/pci/controller/pcie-xilinx-cpm.c | 427 ++++++++++++++---------
  2 files changed, 263 insertions(+), 167 deletions(-)

diff --git a/drivers/pci/controller/Kconfig 
b/drivers/pci/controller/Kconfig
index 1e0f63865775..d0abd671a7b6 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -87,8 +87,7 @@ config PCIE_XILINX_CPM
  	select PCI_HOST_COMMON
  	help
  	  Say 'Y' here if you want kernel support for the
-	  Xilinx Versal CPM host bridge. The driver supports
-	  MSI/MSI-X interrupts using GICv3 ITS feature.
+	  Xilinx Versal CPM host bridge.

  config PCI_XGENE
  	bool "X-Gene PCIe controller"
diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c 
b/drivers/pci/controller/pcie-xilinx-cpm.c
index e8c0aa757f72..32ed9e7e2676 100644
--- a/drivers/pci/controller/pcie-xilinx-cpm.c
+++ b/drivers/pci/controller/pcie-xilinx-cpm.c
@@ -5,8 +5,11 @@
   * (C) Copyright 2019 - 2020, Xilinx, Inc.
   */

+#include <linux/bitfield.h>
  #include <linux/interrupt.h>
  #include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
  #include <linux/irqdomain.h>
  #include <linux/kernel.h>
  #include <linux/module.h>
@@ -33,30 +36,55 @@
  #define XILINX_CPM_PCIE_MISC_IR_LOCAL	BIT(1)

  /* Interrupt registers definitions */
-#define XILINX_CPM_PCIE_INTR_LINK_DOWN		BIT(0)
-#define XILINX_CPM_PCIE_INTR_HOT_RESET		BIT(3)
-#define XILINX_CPM_PCIE_INTR_CFG_TIMEOUT	BIT(8)
-#define XILINX_CPM_PCIE_INTR_CORRECTABLE	BIT(9)
-#define XILINX_CPM_PCIE_INTR_NONFATAL		BIT(10)
-#define XILINX_CPM_PCIE_INTR_FATAL		BIT(11)
-#define XILINX_CPM_PCIE_INTR_INTX		BIT(16)
-#define XILINX_CPM_PCIE_INTR_MSI		BIT(17)
-#define XILINX_CPM_PCIE_INTR_SLV_UNSUPP		BIT(20)
-#define XILINX_CPM_PCIE_INTR_SLV_UNEXP		BIT(21)
-#define XILINX_CPM_PCIE_INTR_SLV_COMPL		BIT(22)
-#define XILINX_CPM_PCIE_INTR_SLV_ERRP		BIT(23)
-#define XILINX_CPM_PCIE_INTR_SLV_CMPABT		BIT(24)
-#define XILINX_CPM_PCIE_INTR_SLV_ILLBUR		BIT(25)
-#define XILINX_CPM_PCIE_INTR_MST_DECERR		BIT(26)
-#define XILINX_CPM_PCIE_INTR_MST_SLVERR		BIT(27)
-#define XILINX_CPM_PCIE_IMR_ALL_MASK		0x1FF39FF9
+#define XILINX_CPM_PCIE_INTR_LINK_DOWN		0
+#define XILINX_CPM_PCIE_INTR_HOT_RESET		3
+#define XILINX_CPM_PCIE_INTR_CFG_PCIE_TIMEOUT	4
+#define XILINX_CPM_PCIE_INTR_CFG_TIMEOUT	8
+#define XILINX_CPM_PCIE_INTR_CORRECTABLE	9
+#define XILINX_CPM_PCIE_INTR_NONFATAL		10
+#define XILINX_CPM_PCIE_INTR_FATAL		11
+#define XILINX_CPM_PCIE_INTR_CFG_ERR_POISON	12
+#define XILINX_CPM_PCIE_INTR_PME_TO_ACK_RCVD	15
+#define XILINX_CPM_PCIE_INTR_INTX		16
+#define XILINX_CPM_PCIE_INTR_PM_PME_RCVD	17
+#define XILINX_CPM_PCIE_INTR_SLV_UNSUPP		20
+#define XILINX_CPM_PCIE_INTR_SLV_UNEXP		21
+#define XILINX_CPM_PCIE_INTR_SLV_COMPL		22
+#define XILINX_CPM_PCIE_INTR_SLV_ERRP		23
+#define XILINX_CPM_PCIE_INTR_SLV_CMPABT		24
+#define XILINX_CPM_PCIE_INTR_SLV_ILLBUR		25
+#define XILINX_CPM_PCIE_INTR_MST_DECERR		26
+#define XILINX_CPM_PCIE_INTR_MST_SLVERR		27
+#define XILINX_CPM_PCIE_INTR_SLV_PCIE_TIMEOUT	28
+
+#define IMR(x)	BIT(XILINX_CPM_PCIE_INTR_ ##x)
+
+#define XILINX_CPM_PCIE_IMR_ALL_MASK			\
+	(						\
+		IMR(LINK_DOWN)		|		\
+		IMR(HOT_RESET)		|		\
+		IMR(CFG_PCIE_TIMEOUT)	|		\
+		IMR(CFG_TIMEOUT)	|		\
+		IMR(CORRECTABLE)	|		\
+		IMR(NONFATAL)		|		\
+		IMR(FATAL)		|		\
+		IMR(CFG_ERR_POISON)	|		\
+		IMR(PME_TO_ACK_RCVD)	|		\
+		IMR(INTX)		|		\
+		IMR(PM_PME_RCVD)	|		\
+		IMR(SLV_UNSUPP)		|		\
+		IMR(SLV_UNEXP)		|		\
+		IMR(SLV_COMPL)		|		\
+		IMR(SLV_ERRP)		|		\
+		IMR(SLV_CMPABT)		|		\
+		IMR(SLV_ILLBUR)		|		\
+		IMR(MST_DECERR)		|		\
+		IMR(MST_SLVERR)		|		\
+		IMR(SLV_PCIE_TIMEOUT)			\
+	)
+
  #define XILINX_CPM_PCIE_IDR_ALL_MASK		0xFFFFFFFF
  #define XILINX_CPM_PCIE_IDRN_MASK		GENMASK(19, 16)
-#define XILINX_CPM_PCIE_INTR_CFG_PCIE_TIMEOUT	BIT(4)
-#define XILINX_CPM_PCIE_INTR_CFG_ERR_POISON	BIT(12)
-#define XILINX_CPM_PCIE_INTR_PME_TO_ACK_RCVD	BIT(15)
-#define XILINX_CPM_PCIE_INTR_PM_PME_RCVD	BIT(17)
-#define XILINX_CPM_PCIE_INTR_SLV_PCIE_TIMEOUT	BIT(28)
  #define XILINX_CPM_PCIE_IDRN_SHIFT		16

  /* Root Port Error FIFO Read Register definitions */
@@ -75,36 +103,37 @@
   * @reg_base: Bridge Register Base
   * @cpm_base: CPM System Level Control and Status Register(SLCR) Base
   * @dev: Device pointer
- * @leg_domain: Legacy IRQ domain pointer
+ * @intx_domain: Legacy IRQ domain pointer
   * @cfg: Holds mappings of config space window
- * @irq_misc: Legacy and error interrupt number
- * @leg_mask_lock: lock for legacy interrupts
+ * @irq: INTx and error interrupt number
+ * @lock: lock protecting shared register access
   */
  struct xilinx_cpm_pcie_port {
-	void __iomem *reg_base;
-	void __iomem *cpm_base;
-	struct device *dev;
-	struct irq_domain *leg_domain;
-	struct pci_config_window *cfg;
-	int irq_misc;
-	raw_spinlock_t leg_mask_lock;
+	void __iomem			*reg_base;
+	void __iomem			*cpm_base;
+	struct device			*dev;
+	struct irq_domain		*intx_domain;
+	struct irq_domain		*cpm_domain;
+	struct pci_config_window	*cfg;
+	int				irq;
+	raw_spinlock_t			lock;
  };

  static inline u32 pcie_read(struct xilinx_cpm_pcie_port *port, u32 reg)
  {
-	return readl(port->reg_base + reg);
+	return readl_relaxed(port->reg_base + reg);
  }

  static inline void pcie_write(struct xilinx_cpm_pcie_port *port,
  			      u32 val, u32 reg)
  {
-	writel(val, port->reg_base + reg);
+	writel_relaxed(val, port->reg_base + reg);
  }

  static inline bool cpm_pcie_link_up(struct xilinx_cpm_pcie_port *port)
  {
  	return (pcie_read(port, XILINX_CPM_PCIE_REG_PSCR) &
-		XILINX_CPM_PCIE_REG_PSCR_LNKUP) ? 1 : 0;
+		XILINX_CPM_PCIE_REG_PSCR_LNKUP);
  }

  /**
@@ -125,44 +154,56 @@ static void cpm_pcie_clear_err_interrupts(struct 
xilinx_cpm_pcie_port *port)

  static void xilinx_cpm_mask_leg_irq(struct irq_data *data)
  {
-	struct irq_desc *desc = irq_to_desc(data->irq);
-	struct xilinx_cpm_pcie_port *port;
+	struct xilinx_cpm_pcie_port *port = irq_data_get_irq_chip_data(data);
  	unsigned long flags;
  	u32 mask;
  	u32 val;

-	port = irq_desc_get_chip_data(desc);
-	mask = (1 << data->hwirq) << XILINX_CPM_PCIE_IDRN_SHIFT;
-	raw_spin_lock_irqsave(&port->leg_mask_lock, flags);
+	mask = BIT(data->hwirq + XILINX_CPM_PCIE_IDRN_SHIFT);
+	raw_spin_lock_irqsave(&port->lock, flags);
  	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN_MASK);
  	pcie_write(port, (val & (~mask)), XILINX_CPM_PCIE_REG_IDRN_MASK);
-	raw_spin_unlock_irqrestore(&port->leg_mask_lock, flags);
+	raw_spin_unlock_irqrestore(&port->lock, flags);
  }

  static void xilinx_cpm_unmask_leg_irq(struct irq_data *data)
  {
-	struct irq_desc *desc = irq_to_desc(data->irq);
-	struct xilinx_cpm_pcie_port *port;
+	struct xilinx_cpm_pcie_port *port = irq_data_get_irq_chip_data(data);
  	unsigned long flags;
  	u32 mask;
  	u32 val;

-	port = irq_desc_get_chip_data(desc);
-	mask = (1 << data->hwirq) << XILINX_CPM_PCIE_IDRN_SHIFT;
-	raw_spin_lock_irqsave(&port->leg_mask_lock, flags);
+	mask = BIT(data->hwirq + XILINX_CPM_PCIE_IDRN_SHIFT);
+	raw_spin_lock_irqsave(&port->lock, flags);
  	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN_MASK);
  	pcie_write(port, (val | mask), XILINX_CPM_PCIE_REG_IDRN_MASK);
-	raw_spin_unlock_irqrestore(&port->leg_mask_lock, flags);
+	raw_spin_unlock_irqrestore(&port->lock, flags);
  }

  static struct irq_chip xilinx_cpm_leg_irq_chip = {
-	.name = "xilinx_cpm_pcie:legacy",
-	.irq_enable = xilinx_cpm_unmask_leg_irq,
-	.irq_disable = xilinx_cpm_mask_leg_irq,
-	.irq_mask = xilinx_cpm_mask_leg_irq,
-	.irq_unmask = xilinx_cpm_unmask_leg_irq,
+	.name		= "INTx",
+	.irq_mask	= xilinx_cpm_mask_leg_irq,
+	.irq_unmask	= xilinx_cpm_unmask_leg_irq,
  };

+static void xilinx_cpm_pcie_intx_flow(struct irq_desc *desc)
+{
+	struct xilinx_cpm_pcie_port *port = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long val;
+	int i;
+
+	chained_irq_enter(chip, desc);
+
+	val = FIELD_GET(XILINX_CPM_PCIE_IDRN_MASK,
+			pcie_read(port, XILINX_CPM_PCIE_REG_IDRN));
+
+	for_each_set_bit(i, &val, PCI_NUM_INTX)
+		generic_handle_irq(irq_find_mapping(port->intx_domain, i));
+
+	chained_irq_exit(chip, desc);
+}
+
  /**
   * xilinx_cpm_pcie_intx_map - Set the handler for the INTx and mark IRQ 
as valid
   * @domain: IRQ domain
@@ -187,111 +228,130 @@ static const struct irq_domain_ops 
intx_domain_ops = {
  	.map = xilinx_cpm_pcie_intx_map,
  };

-/**
- * xilinx_cpm_pcie_intr_handler - Interrupt Service Handler
- * @irq: IRQ number
- * @data: PCIe port information
- *
- * Return: IRQ_HANDLED on success and IRQ_NONE on failure
- */
-static irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void *data)
+static void xilinx_cpm_mask_event_irq(struct irq_data *d)
  {
-	struct xilinx_cpm_pcie_port *port = data;
-	struct device *dev = port->dev;
-	u32 val, mask, status, bit;
-	unsigned long intr_val;
-
-	/* Read interrupt decode and mask registers */
-	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDR);
-	mask = pcie_read(port, XILINX_CPM_PCIE_REG_IMR);
-
-	status = val & mask;
-	if (!status)
-		return IRQ_NONE;
-
-	if (status & XILINX_CPM_PCIE_INTR_LINK_DOWN)
-		dev_warn(dev, "Link Down\n");
-
-	if (status & XILINX_CPM_PCIE_INTR_HOT_RESET)
-		dev_info(dev, "Hot reset\n");
-
-	if (status & XILINX_CPM_PCIE_INTR_CFG_TIMEOUT)
-		dev_warn(dev, "ECAM access timeout\n");
-
-	if (status & XILINX_CPM_PCIE_INTR_CORRECTABLE) {
-		dev_warn(dev, "Correctable error message\n");
-		cpm_pcie_clear_err_interrupts(port);
-	}
-
-	if (status & XILINX_CPM_PCIE_INTR_NONFATAL) {
-		dev_warn(dev, "Non fatal error message\n");
-		cpm_pcie_clear_err_interrupts(port);
-	}
-
-	if (status & XILINX_CPM_PCIE_INTR_FATAL) {
-		dev_warn(dev, "Fatal error message\n");
-		cpm_pcie_clear_err_interrupts(port);
-	}
-
-	if (status & XILINX_CPM_PCIE_INTR_INTX) {
-		/* Handle INTx Interrupt */
-		intr_val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN);
-		intr_val = intr_val >> XILINX_CPM_PCIE_IDRN_SHIFT;
-
-		for_each_set_bit(bit, &intr_val, PCI_NUM_INTX)
-			generic_handle_irq(irq_find_mapping(port->leg_domain,
-							    bit));
-	}
-
-	if (status & XILINX_CPM_PCIE_INTR_SLV_UNSUPP)
-		dev_warn(dev, "Slave unsupported request\n");

-	if (status & XILINX_CPM_PCIE_INTR_SLV_UNEXP)
-		dev_warn(dev, "Slave unexpected completion\n");
+	struct xilinx_cpm_pcie_port *port = irq_data_get_irq_chip_data(d);
+	u32 val;

-	if (status & XILINX_CPM_PCIE_INTR_SLV_COMPL)
-		dev_warn(dev, "Slave completion timeout\n");
+	raw_spin_lock(&port->lock);
+	val = pcie_read(port, XILINX_CPM_PCIE_REG_IMR);
+	val &= ~d->hwirq;
+	pcie_write(port, val, XILINX_CPM_PCIE_REG_IMR);
+	raw_spin_unlock(&port->lock);
+}

-	if (status & XILINX_CPM_PCIE_INTR_SLV_ERRP)
-		dev_warn(dev, "Slave Error Poison\n");
+static void xilinx_cpm_unmask_event_irq(struct irq_data *d)
+{
+	struct xilinx_cpm_pcie_port *port = irq_data_get_irq_chip_data(d);
+	u32 val;

-	if (status & XILINX_CPM_PCIE_INTR_SLV_CMPABT)
-		dev_warn(dev, "Slave Completer Abort\n");
+	raw_spin_lock(&port->lock);
+	val = pcie_read(port, XILINX_CPM_PCIE_REG_IMR);
+	val |= d->hwirq;
+	pcie_write(port, val, XILINX_CPM_PCIE_REG_IMR);
+	raw_spin_unlock(&port->lock);
+}

-	if (status & XILINX_CPM_PCIE_INTR_SLV_ILLBUR)
-		dev_warn(dev, "Slave Illegal Burst\n");
+static struct irq_chip xilinx_cpm_event_irq_chip = {
+	.name		= "RC-Event",
+	.irq_mask	= xilinx_cpm_mask_event_irq,
+	.irq_unmask	= xilinx_cpm_unmask_event_irq,
+};

-	if (status & XILINX_CPM_PCIE_INTR_MST_DECERR)
-		dev_warn(dev, "Master decode error\n");
+static int xilinx_cpm_pcie_event_map(struct irq_domain *domain,
+				    unsigned int irq, irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &xilinx_cpm_event_irq_chip,
+				 handle_level_irq);
+	irq_set_chip_data(irq, domain->host_data);
+	irq_set_status_flags(irq, IRQ_LEVEL);

-	if (status & XILINX_CPM_PCIE_INTR_MST_SLVERR)
-		dev_warn(dev, "Master slave error\n");
+	return 0;
+}

-	if (status & XILINX_CPM_PCIE_INTR_CFG_PCIE_TIMEOUT)
-		dev_warn(dev, "PCIe ECAM access timeout\n");
+static const struct irq_domain_ops event_domain_ops = {
+	.map = xilinx_cpm_pcie_event_map,
+};

-	if (status & XILINX_CPM_PCIE_INTR_CFG_ERR_POISON)
-		dev_warn(dev, "ECAM poisoned completion received\n");
+static void xilinx_cpm_pcie_event_flow(struct irq_desc *desc)
+{
+	struct xilinx_cpm_pcie_port *port = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long val;
+	int i;

-	if (status & XILINX_CPM_PCIE_INTR_PME_TO_ACK_RCVD)
-		dev_warn(dev, "PME_TO_ACK message received\n");
+	chained_irq_enter(chip, desc);

-	if (status & XILINX_CPM_PCIE_INTR_PM_PME_RCVD)
-		dev_warn(dev, "PM_PME message received\n");
+	val =  pcie_read(port, XILINX_CPM_PCIE_REG_IDR);
+	val &= pcie_read(port, XILINX_CPM_PCIE_REG_IMR);

-	if (status & XILINX_CPM_PCIE_INTR_SLV_PCIE_TIMEOUT)
-		dev_warn(dev, "PCIe completion timeout received\n");
+	for_each_set_bit(i, &val, 32)
+		generic_handle_irq(irq_find_mapping(port->cpm_domain, i));

  	/* Clear the Interrupt Decode register */
-	pcie_write(port, status, XILINX_CPM_PCIE_REG_IDR);
+	pcie_write(port, val, XILINX_CPM_PCIE_REG_IDR);

  	/*
  	 * XILINX_CPM_PCIE_MISC_IR_STATUS register is mapped to
  	 * CPM SLCR block.
  	 */
-	val = readl(port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS);
+	val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS);
  	if (val)
-		writel(val, port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS);
+		writel_relaxed(val, port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS);
+
+	chained_irq_exit(chip, desc);
+}
+
+#define _IC(x, s)				\
+	[XILINX_CPM_PCIE_INTR_ ## x] = { __stringify(x), s }
+
+static const struct {
+	const char	*sym;
+	const char	*str;
+} intr_cause[32] = {
+	_IC(LINK_DOWN,		"Link Down"),
+	_IC(HOT_RESET,		"Hot reset"),
+	_IC(CFG_TIMEOUT,	"ECAM access timeout"),
+	_IC(CORRECTABLE,	"Correctable error message"),
+	_IC(NONFATAL,		"Non fatal error message"),
+	_IC(FATAL,		"Fatal error message"),
+	_IC(SLV_UNSUPP,		"Slave unsupported request"),
+	_IC(SLV_UNEXP,		"Slave unexpected completion"),
+	_IC(SLV_COMPL,		"Slave completion timeout"),
+	_IC(SLV_ERRP,		"Slave Error Poison"),
+	_IC(SLV_CMPABT,		"Slave Completer Abort"),
+	_IC(SLV_ILLBUR,		"Slave Illegal Burst"),
+	_IC(MST_DECERR,		"Master decode error"),
+	_IC(MST_SLVERR,		"Master slave error"),
+	_IC(CFG_PCIE_TIMEOUT,	"PCIe ECAM access timeout"),
+	_IC(CFG_ERR_POISON,	"ECAM poisoned completion received"),
+	_IC(PME_TO_ACK_RCVD,	"PME_TO_ACK message received"),
+	_IC(PM_PME_RCVD,	"PM_PME message received"),
+	_IC(SLV_PCIE_TIMEOUT,	"PCIe completion timeout received"),
+};
+
+static irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void *dev_id)
+{
+	struct xilinx_cpm_pcie_port *port = dev_id;
+	struct device *dev = port->dev;
+	struct irq_data *d;
+
+	d = irq_domain_get_irq_data(port->cpm_domain, irq);
+
+	switch(d->hwirq) {
+	case XILINX_CPM_PCIE_INTR_CORRECTABLE:
+	case XILINX_CPM_PCIE_INTR_NONFATAL:
+	case XILINX_CPM_PCIE_INTR_FATAL:
+		cpm_pcie_clear_err_interrupts(port);
+		fallthrough;
+
+	default:
+		if (intr_cause[d->hwirq].str)
+			dev_warn(dev, "%s\n", intr_cause[d->hwirq].str);
+		else
+			dev_warn(dev, "Unknown interrupt\n");
+	}

  	return IRQ_HANDLED;
  }
@@ -315,17 +375,41 @@ static int xilinx_cpm_pcie_init_irq_domain(struct 
xilinx_cpm_pcie_port *port)
  		return -EINVAL;
  	}

-	port->leg_domain = irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX,
+	port->cpm_domain = irq_domain_add_linear(pcie_intc_node, 32,
+						 &event_domain_ops,
+						 port);
+	if (!port->cpm_domain)
+		goto out;
+
+	irq_domain_update_bus_token(port->cpm_domain, DOMAIN_BUS_NEXUS);
+
+	port->intx_domain = irq_domain_add_linear(pcie_intc_node, 
PCI_NUM_INTX,
  						 &intx_domain_ops,
  						 port);
+	if (!port->intx_domain)
+		goto out;
+
+	irq_domain_update_bus_token(port->intx_domain, DOMAIN_BUS_WIRED);
+
  	of_node_put(pcie_intc_node);
-	if (!port->leg_domain) {
-		dev_err(dev, "Failed to get a INTx IRQ domain\n");
-		return -ENOMEM;
-	}
+	raw_spin_lock_init(&port->lock);

-	raw_spin_lock_init(&port->leg_mask_lock);
  	return 0;
+
+out:
+	of_node_put(pcie_intc_node);
+	if (port->intx_domain) {
+		irq_domain_remove(port->intx_domain);
+		port->intx_domain = NULL;
+	}
+
+	if (port->cpm_domain) {
+		irq_domain_remove(port->cpm_domain);
+		port->cpm_domain = NULL;
+	}
+
+	dev_err(dev, "Failed to allocate IRQ domains\n");
+	return -ENOMEM;
  }

  /**
@@ -348,12 +432,6 @@ static void xilinx_cpm_pcie_init_port(struct 
xilinx_cpm_pcie_port *port)
  		   XILINX_CPM_PCIE_IMR_ALL_MASK,
  		   XILINX_CPM_PCIE_REG_IDR);

-	/* Enable all interrupts */
-	pcie_write(port, XILINX_CPM_PCIE_IMR_ALL_MASK,
-		   XILINX_CPM_PCIE_REG_IMR);
-	pcie_write(port, XILINX_CPM_PCIE_IDRN_MASK,
-		   XILINX_CPM_PCIE_REG_IDRN_MASK);
-
  	/*
  	 * XILINX_CPM_PCIE_MISC_IR_ENABLE register is mapped to
  	 * CPM SLCR block.
@@ -366,28 +444,45 @@ static void xilinx_cpm_pcie_init_port(struct 
xilinx_cpm_pcie_port *port)
  		   XILINX_CPM_PCIE_REG_RPSC);
  }

-static int xilinx_cpm_request_misc_irq(struct xilinx_cpm_pcie_port 
*port)
+static int xilinx_cpm_setup_irq(struct xilinx_cpm_pcie_port *port)
  {
  	struct device *dev = port->dev;
  	struct platform_device *pdev = to_platform_device(dev);
-	int err;
+	int i, irq;

-	port->irq_misc = platform_get_irq(pdev, 0);
-	if (port->irq_misc <= 0) {
-		dev_err(dev, "Unable to find misc IRQ line\n");
-		return port->irq_misc;
+	port->irq = platform_get_irq(pdev, 0);
+	if (port->irq < 0) {
+		dev_err(dev, "Unable to find IRQ line\n");
+		return port->irq;
  	}

-	err = devm_request_irq(dev, port->irq_misc,
-			       xilinx_cpm_pcie_intr_handler,
-			       IRQF_SHARED | IRQF_NO_THREAD,
-			       "xilinx-pcie", port);
-	if (err) {
-		dev_err(dev, "unable to request misc IRQ line %d\n",
-			port->irq_misc);
-		return err;
+	for (i = 0; i < ARRAY_SIZE(intr_cause); i++) {
+		int err;
+
+		if (!intr_cause[i].str)
+			continue;
+
+		irq = irq_create_mapping(port->cpm_domain, i);
+		if (WARN_ON(irq <= 0))
+			return -ENXIO;
+
+		err = devm_request_irq(dev, irq, xilinx_cpm_pcie_intr_handler,
+				       0, intr_cause[i].sym, port);
+		if (WARN_ON(err))
+			return err;
  	}

+	irq = irq_create_mapping(port->cpm_domain, XILINX_CPM_PCIE_INTR_INTX);
+	if (WARN_ON(irq <= 0))
+		return -ENXIO;
+
+	/* Plug the INTx chained handler */
+	irq_set_chained_handler_and_data(irq, xilinx_cpm_pcie_intx_flow, 
port);
+
+	/* Plug the main event chained handler */
+	irq_set_chained_handler_and_data(port->irq, 
xilinx_cpm_pcie_event_flow,
+					 port);
+
  	return 0;
  }

@@ -422,7 +517,7 @@ static int xilinx_cpm_pcie_parse_dt(struct 
xilinx_cpm_pcie_port *port,
  	if (IS_ERR(port->cpm_base))
  		return PTR_ERR(port->cpm_base);

-	err = xilinx_cpm_request_misc_irq(port);
+	err = xilinx_cpm_setup_irq(port);
  	if (err)
  		return err;

@@ -481,8 +576,10 @@ static int xilinx_cpm_pcie_probe(struct 
platform_device *pdev)

  	err = pci_host_probe(bridge);
  	if (err < 0) {
-		irq_domain_remove(port->leg_domain);
-		devm_free_irq(dev, port->irq_misc, port);
+		if (port->intx_domain)
+			irq_domain_remove(port->intx_domain);
+		if (port->cpm_domain)
+			irq_domain_remove(port->cpm_domain);
  		return err;
  	}

-- 
2.26.2


-- 
Jazz is not dead. It just smells funny...

RE: [PATCH v7 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver

[Bharat Kumar Gogada]

> Bharat,
> 
> On 2020-05-07 12:58, Bharat Kumar Gogada wrote:
> > - Add support for Versal CPM as Root Port.
> > - The Versal ACAP devices include CCIX-PCIe Module (CPM). The
> > integrated
> >   block for CPM along with the integrated bridge can function
> >   as PCIe Root Port.
> > - Bridge error and legacy interrupts in Versal CPM are handled using
> >   Versal CPM specific interrupt line.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> > ---
> >  drivers/pci/controller/Kconfig           |   9 +
> >  drivers/pci/controller/Makefile          |   1 +
> >  drivers/pci/controller/pcie-xilinx-cpm.c | 506
> > +++++++++++++++++++++++++++++++
> >  3 files changed, 516 insertions(+)
> >  create mode 100644 drivers/pci/controller/pcie-xilinx-cpm.c
> >
> > diff --git a/drivers/pci/controller/Kconfig
> > b/drivers/pci/controller/Kconfig index 20bf00f..ca0ae24 100644
> > --- a/drivers/pci/controller/Kconfig
> > +++ b/drivers/pci/controller/Kconfig
> > @@ -81,6 +81,15 @@ config PCIE_XILINX
> >  	  Say 'Y' here if you want kernel to support the Xilinx AXI PCIe
> >  	  Host Bridge driver.
> >
> > +config PCIE_XILINX_CPM
> > +	bool "Xilinx Versal CPM host bridge support"
> > +	depends on ARCH_ZYNQMP || COMPILE_TEST
> > +	select PCI_HOST_COMMON
> > +	help
> > +	  Say 'Y' here if you want kernel support for the
> > +	  Xilinx Versal CPM host bridge. The driver supports
> > +	  MSI/MSI-X interrupts using GICv3 ITS feature.
> 
> I don't think this last sentense makes any sense here. We usually don't
> mention things that the driver *doesn't* implement.
Agreed, will remove these details.
> 
> > +
> >  config PCI_XGENE
> >  	bool "X-Gene PCIe controller"
> >  	depends on ARM64 || COMPILE_TEST
> > diff --git a/drivers/pci/controller/Makefile
> > b/drivers/pci/controller/Makefile index 01b2502..78dabda 100644
> > --- a/drivers/pci/controller/Makefile
> > +++ b/drivers/pci/controller/Makefile
> > @@ -12,6 +12,7 @@ obj-$(CONFIG_PCI_HOST_COMMON) += pci-host-
> common.o
> >  obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
> >  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
> >  obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
> > +obj-$(CONFIG_PCIE_XILINX_CPM) += pcie-xilinx-cpm.o
> >  obj-$(CONFIG_PCI_V3_SEMI) += pci-v3-semi.o
> >  obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
> >  obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o diff --git
> > a/drivers/pci/controller/pcie-xilinx-cpm.c
> > b/drivers/pci/controller/pcie-xilinx-cpm.c
> > new file mode 100644
> > index 0000000..e8c0aa7
> > --- /dev/null
> > +++ b/drivers/pci/controller/pcie-xilinx-cpm.c
> > @@ -0,0 +1,506 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * PCIe host controller driver for Xilinx Versal CPM DMA Bridge
> > + *
> > + * (C) Copyright 2019 - 2020, Xilinx, Inc.
> > + */
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_pci.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pci-ecam.h>
> > +
> > +#include "../pci.h"
> > +
> > +/* Register definitions */
> > +#define XILINX_CPM_PCIE_REG_IDR		0x00000E10
> > +#define XILINX_CPM_PCIE_REG_IMR		0x00000E14
> > +#define XILINX_CPM_PCIE_REG_PSCR	0x00000E1C
> > +#define XILINX_CPM_PCIE_REG_RPSC	0x00000E20
> > +#define XILINX_CPM_PCIE_REG_RPEFR	0x00000E2C
> > +#define XILINX_CPM_PCIE_REG_IDRN	0x00000E38
> > +#define XILINX_CPM_PCIE_REG_IDRN_MASK	0x00000E3C
> > +#define XILINX_CPM_PCIE_MISC_IR_STATUS	0x00000340
> > +#define XILINX_CPM_PCIE_MISC_IR_ENABLE	0x00000348
> > +#define XILINX_CPM_PCIE_MISC_IR_LOCAL	BIT(1)
> > +
> > +/* Interrupt registers definitions */
> > +#define XILINX_CPM_PCIE_INTR_LINK_DOWN		BIT(0)
> > +#define XILINX_CPM_PCIE_INTR_HOT_RESET		BIT(3)
> > +#define XILINX_CPM_PCIE_INTR_CFG_TIMEOUT	BIT(8)
> > +#define XILINX_CPM_PCIE_INTR_CORRECTABLE	BIT(9)
> > +#define XILINX_CPM_PCIE_INTR_NONFATAL		BIT(10)
> > +#define XILINX_CPM_PCIE_INTR_FATAL		BIT(11)
> > +#define XILINX_CPM_PCIE_INTR_INTX		BIT(16)
> > +#define XILINX_CPM_PCIE_INTR_MSI		BIT(17)
> > +#define XILINX_CPM_PCIE_INTR_SLV_UNSUPP		BIT(20)
> > +#define XILINX_CPM_PCIE_INTR_SLV_UNEXP		BIT(21)
> > +#define XILINX_CPM_PCIE_INTR_SLV_COMPL		BIT(22)
> > +#define XILINX_CPM_PCIE_INTR_SLV_ERRP		BIT(23)
> > +#define XILINX_CPM_PCIE_INTR_SLV_CMPABT		BIT(24)
> > +#define XILINX_CPM_PCIE_INTR_SLV_ILLBUR		BIT(25)
> > +#define XILINX_CPM_PCIE_INTR_MST_DECERR		BIT(26)
> > +#define XILINX_CPM_PCIE_INTR_MST_SLVERR		BIT(27)
> > +#define XILINX_CPM_PCIE_IMR_ALL_MASK		0x1FF39FF9
> 
> I assume that this is the logical OR of all the XILINX_CPM_PCIE_INTR_* bits.
> Please express it as such.
Agreed, will change this.
> 
> > +#define XILINX_CPM_PCIE_IDR_ALL_MASK		0xFFFFFFFF
> > +#define XILINX_CPM_PCIE_IDRN_MASK		GENMASK(19, 16)
> > +#define XILINX_CPM_PCIE_INTR_CFG_PCIE_TIMEOUT	BIT(4)
> > +#define XILINX_CPM_PCIE_INTR_CFG_ERR_POISON	BIT(12)
> > +#define XILINX_CPM_PCIE_INTR_PME_TO_ACK_RCVD	BIT(15)
> > +#define XILINX_CPM_PCIE_INTR_PM_PME_RCVD	BIT(17)
> 
> So we have two definitions for bit 17... Given that nothing uses the MSI
> version, I assume it is useless...
> 
> > +#define XILINX_CPM_PCIE_INTR_SLV_PCIE_TIMEOUT	BIT(28)
> 
> Please group all the XILINX_CPM_PCIE_INTR_* together.
> 
Agreed, will change this.
> > +#define XILINX_CPM_PCIE_IDRN_SHIFT		16
> > +
> > +/* Root Port Error FIFO Read Register definitions */
> > +#define XILINX_CPM_PCIE_RPEFR_ERR_VALID		BIT(18)
> > +#define XILINX_CPM_PCIE_RPEFR_REQ_ID		GENMASK(15, 0)
> > +#define XILINX_CPM_PCIE_RPEFR_ALL_MASK		0xFFFFFFFF
> > +
> > +/* Root Port Status/control Register definitions */
> > +#define XILINX_CPM_PCIE_REG_RPSC_BEN		BIT(0)
> > +
> > +/* Phy Status/Control Register definitions */
> > +#define XILINX_CPM_PCIE_REG_PSCR_LNKUP		BIT(11)
> > +
> > +/**
> > + * struct xilinx_cpm_pcie_port - PCIe port information
> > + * @reg_base: Bridge Register Base
> > + * @cpm_base: CPM System Level Control and Status Register(SLCR) Base
> > + * @dev: Device pointer
> > + * @leg_domain: Legacy IRQ domain pointer
> 
> leg, arm, thumb, limb... Given that oyu use the 'intx' description all over the
> driver, please be consistent and name this intx_domain.
> 
> > + * @cfg: Holds mappings of config space window
> > + * @irq_misc: Legacy and error interrupt number
> 
> Let's face it, this is the *only* interrupt this thing as, so the 'misc'
> is supperfluous.
> 
> > + * @leg_mask_lock: lock for legacy interrupts  */ struct
> > +xilinx_cpm_pcie_port {
> > +	void __iomem *reg_base;
> > +	void __iomem *cpm_base;
> > +	struct device *dev;
> > +	struct irq_domain *leg_domain;
> > +	struct pci_config_window *cfg;
> > +	int irq_misc;
> > +	raw_spinlock_t leg_mask_lock;
> > +};
> > +
> > +static inline u32 pcie_read(struct xilinx_cpm_pcie_port *port, u32
> > reg)
> > +{
> > +	return readl(port->reg_base + reg);
> 
> There is no need for enforced ordering with non-device writes, so you can
> turn this into readl_relaxed. You can also drop the inline, as the compiler
> will do the right thing for you.
>
Agreed, will change to readl_relaxed.
 
> > +}
> > +
> > +static inline void pcie_write(struct xilinx_cpm_pcie_port *port,
> > +			      u32 val, u32 reg)
> > +{
> > +	writel(val, port->reg_base + reg);
> 
> Same thing.
> 
> > +}
> > +
> > +static inline bool cpm_pcie_link_up(struct xilinx_cpm_pcie_port
> > +*port) {
> > +	return (pcie_read(port, XILINX_CPM_PCIE_REG_PSCR) &
> > +		XILINX_CPM_PCIE_REG_PSCR_LNKUP) ? 1 : 0;
> 
> If you are making the return value a bool, you might as well return
> true/false. But that's a cumbersome way to write:
> 
>          return pcie_read(port, XILINX_CPM_PCIE_REG_PSCR) &
>                 XILINX_CPM_PCIE_REG_PSCR_LNKUP;
> 
> > +}
> > +
> > +/**
> > + * xilinx_cpm_pcie_clear_err_interrupts - Clear Error Interrupts
> > + * @port: PCIe port information
> > + */
> > +static void cpm_pcie_clear_err_interrupts(struct xilinx_cpm_pcie_port
> > *port)
> > +{
> > +	unsigned long val = pcie_read(port, XILINX_CPM_PCIE_REG_RPEFR);
> > +
> > +	if (val & XILINX_CPM_PCIE_RPEFR_ERR_VALID) {
> > +		dev_dbg(port->dev, "Requester ID %lu\n",
> > +			val & XILINX_CPM_PCIE_RPEFR_REQ_ID);
> > +		pcie_write(port, XILINX_CPM_PCIE_RPEFR_ALL_MASK,
> > +			   XILINX_CPM_PCIE_REG_RPEFR);
> > +	}
> > +}
> > +
> > +static void xilinx_cpm_mask_leg_irq(struct irq_data *data) {
> > +	struct irq_desc *desc = irq_to_desc(data->irq);
> > +	struct xilinx_cpm_pcie_port *port;
> > +	unsigned long flags;
> > +	u32 mask;
> > +	u32 val;
> > +
> > +	port = irq_desc_get_chip_data(desc);
> 
> port = irq_data_get_irq_chip_data(data);
> 
> You should never have to get the irq_desc (and using irq_to_desc() is a
> prettyodd way to get it when you already have the irq_data).
> 
> > +	mask = (1 << data->hwirq) << XILINX_CPM_PCIE_IDRN_SHIFT;
> 
> Also known as BIT(data->hwirq + XILINX_CPM_PCIE_IDRN_SHIFT).
> 
> > +	raw_spin_lock_irqsave(&port->leg_mask_lock, flags);
> > +	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN_MASK);
> > +	pcie_write(port, (val & (~mask)),
> XILINX_CPM_PCIE_REG_IDRN_MASK);
> > +	raw_spin_unlock_irqrestore(&port->leg_mask_lock, flags); }
> > +
> > +static void xilinx_cpm_unmask_leg_irq(struct irq_data *data) {
> > +	struct irq_desc *desc = irq_to_desc(data->irq);
> > +	struct xilinx_cpm_pcie_port *port;
> > +	unsigned long flags;
> > +	u32 mask;
> > +	u32 val;
> > +
> > +	port = irq_desc_get_chip_data(desc);
> > +	mask = (1 << data->hwirq) << XILINX_CPM_PCIE_IDRN_SHIFT;
> > +	raw_spin_lock_irqsave(&port->leg_mask_lock, flags);
> > +	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN_MASK);
> > +	pcie_write(port, (val | mask), XILINX_CPM_PCIE_REG_IDRN_MASK);
> > +	raw_spin_unlock_irqrestore(&port->leg_mask_lock, flags); }
> > +
> > +static struct irq_chip xilinx_cpm_leg_irq_chip = {
> > +	.name = "xilinx_cpm_pcie:legacy",
> 
> "INTx" is a good enough description.
> 
> > +	.irq_enable = xilinx_cpm_unmask_leg_irq,
> > +	.irq_disable = xilinx_cpm_mask_leg_irq,
> > +	.irq_mask = xilinx_cpm_mask_leg_irq,
> > +	.irq_unmask = xilinx_cpm_unmask_leg_irq,
> 
> This makes no sense. If enable/disable have the same implementation as
> unmask/mask, then enable/disable is pretty useless. Please drop them.
> 
Agreed, will drop these methods.
> > +};
> > +
> > +/**
> > + * xilinx_cpm_pcie_intx_map - Set the handler for the INTx and mark
> > IRQ as valid
> > + * @domain: IRQ domain
> > + * @irq: Virtual IRQ number
> > + * @hwirq: HW interrupt number
> > + *
> > + * Return: Always returns 0.
> > + */
> > +static int xilinx_cpm_pcie_intx_map(struct irq_domain *domain,
> > +				    unsigned int irq, irq_hw_number_t hwirq)
> {
> > +	irq_set_chip_and_handler(irq, &xilinx_cpm_leg_irq_chip,
> > +				 handle_level_irq);
> > +	irq_set_chip_data(irq, domain->host_data);
> > +	irq_set_status_flags(irq, IRQ_LEVEL);
> > +
> > +	return 0;
> > +}
> > +
> > +/* INTx IRQ Domain operations */
> > +static const struct irq_domain_ops intx_domain_ops = {
> > +	.map = xilinx_cpm_pcie_intx_map,
> > +};
> > +
> > +/**
> > + * xilinx_cpm_pcie_intr_handler - Interrupt Service Handler
> > + * @irq: IRQ number
> > + * @data: PCIe port information
> > + *
> > + * Return: IRQ_HANDLED on success and IRQ_NONE on failure  */ static
> > +irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void *data) {
> > +	struct xilinx_cpm_pcie_port *port = data;
> > +	struct device *dev = port->dev;
> > +	u32 val, mask, status, bit;
> > +	unsigned long intr_val;
> > +
> > +	/* Read interrupt decode and mask registers */
> > +	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDR);
> > +	mask = pcie_read(port, XILINX_CPM_PCIE_REG_IMR);
> > +
> > +	status = val & mask;
> > +	if (!status)
> > +		return IRQ_NONE;
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_LINK_DOWN)
> > +		dev_warn(dev, "Link Down\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_HOT_RESET)
> > +		dev_info(dev, "Hot reset\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_CFG_TIMEOUT)
> > +		dev_warn(dev, "ECAM access timeout\n");
> 
> There is a certain sense of repetition here. It really begs the question of
> *why* you want to take these interrupts if all you do is spam the console
> with warnings, not taking any action to potentially remedy the problem.
> 
We use this information if user faces an issue to track down possible cause. 
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_CORRECTABLE) {
> > +		dev_warn(dev, "Correctable error message\n");
> > +		cpm_pcie_clear_err_interrupts(port);
> > +	}
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_NONFATAL) {
> > +		dev_warn(dev, "Non fatal error message\n");
> > +		cpm_pcie_clear_err_interrupts(port);
> > +	}
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_FATAL) {
> > +		dev_warn(dev, "Fatal error message\n");
> > +		cpm_pcie_clear_err_interrupts(port);
> > +	}
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_INTX) {
> > +		/* Handle INTx Interrupt */
> > +		intr_val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN);
> > +		intr_val = intr_val >> XILINX_CPM_PCIE_IDRN_SHIFT;
> > +
> > +		for_each_set_bit(bit, &intr_val, PCI_NUM_INTX)
> > +			generic_handle_irq(irq_find_mapping(port-
> >leg_domain,
> > +							    bit));
> 
> That's a firm no. We don't demux chained handlers in an interrupt handler.
> It may work for now, but it will eventually break. And to be clear, this whole
> function needs to die. By the look of it, this PCie controller implements
> *TWO* interrupt multiplexers:
> 
> - one that muxes all the ILINX_CPM_PCIE_INTR_* events onto a single top-
> level IRQ
> - another one that muxes all INTX lines onto XILINX_CPM_PCIE_INTR_INTX
> 
> Please implement the whole thing as described above.
Agreed, thanks for resolving this. 
> 
> > +	}
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_SLV_UNSUPP)
> > +		dev_warn(dev, "Slave unsupported request\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_SLV_UNEXP)
> > +		dev_warn(dev, "Slave unexpected completion\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_SLV_COMPL)
> > +		dev_warn(dev, "Slave completion timeout\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_SLV_ERRP)
> > +		dev_warn(dev, "Slave Error Poison\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_SLV_CMPABT)
> > +		dev_warn(dev, "Slave Completer Abort\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_SLV_ILLBUR)
> > +		dev_warn(dev, "Slave Illegal Burst\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_MST_DECERR)
> > +		dev_warn(dev, "Master decode error\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_MST_SLVERR)
> > +		dev_warn(dev, "Master slave error\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_CFG_PCIE_TIMEOUT)
> > +		dev_warn(dev, "PCIe ECAM access timeout\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_CFG_ERR_POISON)
> > +		dev_warn(dev, "ECAM poisoned completion received\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_PME_TO_ACK_RCVD)
> > +		dev_warn(dev, "PME_TO_ACK message received\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_PM_PME_RCVD)
> > +		dev_warn(dev, "PM_PME message received\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_SLV_PCIE_TIMEOUT)
> > +		dev_warn(dev, "PCIe completion timeout received\n");
> 
> I'm pretty sure there is a slightly better way to write this...
> 
> > +
> > +	/* Clear the Interrupt Decode register */
> > +	pcie_write(port, status, XILINX_CPM_PCIE_REG_IDR);
> > +
> > +	/*
> > +	 * XILINX_CPM_PCIE_MISC_IR_STATUS register is mapped to
> > +	 * CPM SLCR block.
> > +	 */
> > +	val = readl(port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS);
> > +	if (val)
> > +		writel(val, port->cpm_base +
> XILINX_CPM_PCIE_MISC_IR_STATUS);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +/**
> > + * xilinx_cpm_pcie_init_irq_domain - Initialize IRQ domain
> > + * @port: PCIe port information
> > + *
> > + * Return: '0' on success and error value on failure  */ static int
> > +xilinx_cpm_pcie_init_irq_domain(struct xilinx_cpm_pcie_port
> > *port)
> > +{
> > +	struct device *dev = port->dev;
> > +	struct device_node *node = dev->of_node;
> > +	struct device_node *pcie_intc_node;
> > +
> > +	/* Setup INTx */
> > +	pcie_intc_node = of_get_next_child(node, NULL);
> > +	if (!pcie_intc_node) {
> > +		dev_err(dev, "No PCIe Intc node found\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	port->leg_domain = irq_domain_add_linear(pcie_intc_node,
> > PCI_NUM_INTX,
> > +						 &intx_domain_ops,
> > +						 port);
> > +	of_node_put(pcie_intc_node);
> > +	if (!port->leg_domain) {
> > +		dev_err(dev, "Failed to get a INTx IRQ domain\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	raw_spin_lock_init(&port->leg_mask_lock);
> > +	return 0;
> > +}
> > +
> > +/**
> > + * xilinx_cpm_pcie_init_port - Initialize hardware
> > + * @port: PCIe port information
> > + */
> > +static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie_port
> > *port)
> > +{
> > +	if (cpm_pcie_link_up(port))
> > +		dev_info(port->dev, "PCIe Link is UP\n");
> > +	else
> > +		dev_info(port->dev, "PCIe Link is DOWN\n");
> > +
> > +	/* Disable all interrupts */
> > +	pcie_write(port, ~XILINX_CPM_PCIE_IDR_ALL_MASK,
> > +		   XILINX_CPM_PCIE_REG_IMR);
> > +
> > +	/* Clear pending interrupts */
> > +	pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_IDR) &
> > +		   XILINX_CPM_PCIE_IMR_ALL_MASK,
> > +		   XILINX_CPM_PCIE_REG_IDR);
> > +
> > +	/* Enable all interrupts */
> > +	pcie_write(port, XILINX_CPM_PCIE_IMR_ALL_MASK,
> > +		   XILINX_CPM_PCIE_REG_IMR);
> > +	pcie_write(port, XILINX_CPM_PCIE_IDRN_MASK,
> > +		   XILINX_CPM_PCIE_REG_IDRN_MASK);
> 
> No. We don't enable interrupts randomly. They need a handler registered
> with the core IRq subsystem *first*, which is why this needs to be hooked in
> as a proper irqchip, and each event handled individualy.
> 
> > +
> > +	/*
> > +	 * XILINX_CPM_PCIE_MISC_IR_ENABLE register is mapped to
> > +	 * CPM SLCR block.
> > +	 */
> > +	writel(XILINX_CPM_PCIE_MISC_IR_LOCAL,
> > +	       port->cpm_base + XILINX_CPM_PCIE_MISC_IR_ENABLE);
> > +	/* Enable the Bridge enable bit */
> > +	pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_RPSC) |
> > +		   XILINX_CPM_PCIE_REG_RPSC_BEN,
> > +		   XILINX_CPM_PCIE_REG_RPSC);
> > +}
> > +
> > +static int xilinx_cpm_request_misc_irq(struct xilinx_cpm_pcie_port
> > *port)
> > +{
> > +	struct device *dev = port->dev;
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	int err;
> > +
> > +	port->irq_misc = platform_get_irq(pdev, 0);
> > +	if (port->irq_misc <= 0) {
> 
> If 0 is an error, how do you distinguish it from the non-error case?
> 
> > +		dev_err(dev, "Unable to find misc IRQ line\n");
> > +		return port->irq_misc;
> > +	}
> > +
> > +	err = devm_request_irq(dev, port->irq_misc,
> > +			       xilinx_cpm_pcie_intr_handler,
> > +			       IRQF_SHARED | IRQF_NO_THREAD,
> > +			       "xilinx-pcie", port);
> > +	if (err) {
> > +		dev_err(dev, "unable to request misc IRQ line %d\n",
> > +			port->irq_misc);
> > +		return err;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * xilinx_cpm_pcie_parse_dt - Parse Device tree
> > + * @port: PCIe port information
> > + * @bus_range: Bus resource
> > + *
> > + * Return: '0' on success and error value on failure  */ static int
> > +xilinx_cpm_pcie_parse_dt(struct xilinx_cpm_pcie_port *port,
> > +				    struct resource *bus_range)
> > +{
> > +	struct device *dev = port->dev;
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct resource *res;
> > +	int err;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "cfg");
> > +	if (!res)
> > +		return -ENXIO;
> > +
> > +	port->cfg = pci_ecam_create(dev, res, bus_range,
> > +				    &pci_generic_ecam_ops);
> > +	if (IS_ERR(port->cfg))
> > +		return PTR_ERR(port->cfg);
> > +
> > +	port->reg_base = port->cfg->win;
> > +
> > +	port->cpm_base = devm_platform_ioremap_resource_byname(pdev,
> > +							       "cpm_slcr");
> > +	if (IS_ERR(port->cpm_base))
> > +		return PTR_ERR(port->cpm_base);
> > +
> > +	err = xilinx_cpm_request_misc_irq(port);
> > +	if (err)
> > +		return err;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * xilinx_cpm_pcie_probe - Probe function
> > + * @pdev: Platform device pointer
> > + *
> > + * Return: '0' on success and error value on failure  */ static int
> > +xilinx_cpm_pcie_probe(struct platform_device *pdev) {
> > +	struct xilinx_cpm_pcie_port *port;
> > +	struct device *dev = &pdev->dev;
> > +	struct pci_host_bridge *bridge;
> > +	struct resource *bus_range;
> > +	int err;
> > +
> > +	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*port));
> > +	if (!bridge)
> > +		return -ENODEV;
> > +
> > +	port = pci_host_bridge_priv(bridge);
> > +
> > +	port->dev = dev;
> > +
> > +	err = pci_parse_request_of_pci_ranges(dev, &bridge->windows,
> > +					      &bridge->dma_ranges,
> &bus_range);
> > +	if (err) {
> > +		dev_err(dev, "Getting bridge resources failed\n");
> > +		return err;
> > +	}
> > +
> > +	err = xilinx_cpm_pcie_parse_dt(port, bus_range);
> > +	if (err) {
> > +		dev_err(dev, "Parsing DT failed\n");
> > +		return err;
> > +	}
> > +
> > +	xilinx_cpm_pcie_init_port(port);
> > +
> > +	err = xilinx_cpm_pcie_init_irq_domain(port);
> > +	if (err) {
> > +		dev_err(dev, "Failed creating IRQ Domain\n");
> > +		return err;
> > +	}
> > +
> > +	bridge->dev.parent = dev;
> > +	bridge->sysdata = port->cfg;
> > +	bridge->busnr = port->cfg->busr.start;
> > +	bridge->ops = &pci_generic_ecam_ops.pci_ops;
> > +	bridge->map_irq = of_irq_parse_and_map_pci;
> > +	bridge->swizzle_irq = pci_common_swizzle;
> > +
> > +	err = pci_host_probe(bridge);
> > +	if (err < 0) {
> > +		irq_domain_remove(port->leg_domain);
> > +		devm_free_irq(dev, port->irq_misc, port);
> 
> Why calling dem_free_irq()? The whole point of using devm* is not to have
> to manage the error handling.
> 
> > +		return err;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id xilinx_cpm_pcie_of_match[] = {
> > +	{ .compatible = "xlnx,versal-cpm-host-1.00", },
> > +	{}
> > +};
> > +
> > +static struct platform_driver xilinx_cpm_pcie_driver = {
> > +	.driver = {
> > +		.name = "xilinx-cpm-pcie",
> > +		.of_match_table = xilinx_cpm_pcie_of_match,
> > +		.suppress_bind_attrs = true,
> > +	},
> > +	.probe = xilinx_cpm_pcie_probe,
> > +};
> > +
> > +builtin_platform_driver(xilinx_cpm_pcie_driver);
> 
> I don't think this driver is in a state where it can be merged. Given that we're
> already at v7 and to save everyone a bit of time, I've written the patch that
> implements the above comments. It compiles, but is probably broken.
> Hopefully you can test it and figure things out.
Hi Marc,
Thanks for your time and inputs.

Regards,
Bharat


> 
>          M.
> 
>  From 8b5d158374e59edf26e61c512eeb00ca7c9d891d Mon Sep 17 00:00:00
> 2001
>  From: Marc Zyngier <maz@kernel.org>
> Date: Fri, 22 May 2020 16:33:32 +0100
> Subject: [PATCH] PCI: xilinx-cpm: Revamp irq handling
> 
> The xilinx-cpm driver missuses the IRQ layer in creative ways.
> It implements a chained interrupt demultiplexer inside a normal handler,
> enables interrupts randomly, and could overall do with a good cleanup.
> 
> Instead, implement the IRQ support as two nested chained irqchips, the
> outer one dealing with all the PCIe events, the inner one namaging the INTx
> signals.
> 
> Additionally, the whole driver is cleanup-up so that it can make a bit more
> sense to me. YMMV.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>   drivers/pci/controller/Kconfig           |   3 +-
>   drivers/pci/controller/pcie-xilinx-cpm.c | 427 ++++++++++++++---------
>   2 files changed, 263 insertions(+), 167 deletions(-)
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 1e0f63865775..d0abd671a7b6 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -87,8 +87,7 @@ config PCIE_XILINX_CPM
>   	select PCI_HOST_COMMON
>   	help
>   	  Say 'Y' here if you want kernel support for the
> -	  Xilinx Versal CPM host bridge. The driver supports
> -	  MSI/MSI-X interrupts using GICv3 ITS feature.
> +	  Xilinx Versal CPM host bridge.
> 
>   config PCI_XGENE
>   	bool "X-Gene PCIe controller"
> diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c
> b/drivers/pci/controller/pcie-xilinx-cpm.c
> index e8c0aa757f72..32ed9e7e2676 100644
> --- a/drivers/pci/controller/pcie-xilinx-cpm.c
> +++ b/drivers/pci/controller/pcie-xilinx-cpm.c
> @@ -5,8 +5,11 @@
>    * (C) Copyright 2019 - 2020, Xilinx, Inc.
>    */
> 
> +#include <linux/bitfield.h>
>   #include <linux/interrupt.h>
>   #include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
>   #include <linux/irqdomain.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
> @@ -33,30 +36,55 @@
>   #define XILINX_CPM_PCIE_MISC_IR_LOCAL	BIT(1)
> 
>   /* Interrupt registers definitions */
> -#define XILINX_CPM_PCIE_INTR_LINK_DOWN		BIT(0)
> -#define XILINX_CPM_PCIE_INTR_HOT_RESET		BIT(3)
> -#define XILINX_CPM_PCIE_INTR_CFG_TIMEOUT	BIT(8)
> -#define XILINX_CPM_PCIE_INTR_CORRECTABLE	BIT(9)
> -#define XILINX_CPM_PCIE_INTR_NONFATAL		BIT(10)
> -#define XILINX_CPM_PCIE_INTR_FATAL		BIT(11)
> -#define XILINX_CPM_PCIE_INTR_INTX		BIT(16)
> -#define XILINX_CPM_PCIE_INTR_MSI		BIT(17)
> -#define XILINX_CPM_PCIE_INTR_SLV_UNSUPP		BIT(20)
> -#define XILINX_CPM_PCIE_INTR_SLV_UNEXP		BIT(21)
> -#define XILINX_CPM_PCIE_INTR_SLV_COMPL		BIT(22)
> -#define XILINX_CPM_PCIE_INTR_SLV_ERRP		BIT(23)
> -#define XILINX_CPM_PCIE_INTR_SLV_CMPABT		BIT(24)
> -#define XILINX_CPM_PCIE_INTR_SLV_ILLBUR		BIT(25)
> -#define XILINX_CPM_PCIE_INTR_MST_DECERR		BIT(26)
> -#define XILINX_CPM_PCIE_INTR_MST_SLVERR		BIT(27)
> -#define XILINX_CPM_PCIE_IMR_ALL_MASK		0x1FF39FF9
> +#define XILINX_CPM_PCIE_INTR_LINK_DOWN		0
> +#define XILINX_CPM_PCIE_INTR_HOT_RESET		3
> +#define XILINX_CPM_PCIE_INTR_CFG_PCIE_TIMEOUT	4
> +#define XILINX_CPM_PCIE_INTR_CFG_TIMEOUT	8
> +#define XILINX_CPM_PCIE_INTR_CORRECTABLE	9
> +#define XILINX_CPM_PCIE_INTR_NONFATAL		10
> +#define XILINX_CPM_PCIE_INTR_FATAL		11
> +#define XILINX_CPM_PCIE_INTR_CFG_ERR_POISON	12
> +#define XILINX_CPM_PCIE_INTR_PME_TO_ACK_RCVD	15
> +#define XILINX_CPM_PCIE_INTR_INTX		16
> +#define XILINX_CPM_PCIE_INTR_PM_PME_RCVD	17
> +#define XILINX_CPM_PCIE_INTR_SLV_UNSUPP		20
> +#define XILINX_CPM_PCIE_INTR_SLV_UNEXP		21
> +#define XILINX_CPM_PCIE_INTR_SLV_COMPL		22
> +#define XILINX_CPM_PCIE_INTR_SLV_ERRP		23
> +#define XILINX_CPM_PCIE_INTR_SLV_CMPABT		24
> +#define XILINX_CPM_PCIE_INTR_SLV_ILLBUR		25
> +#define XILINX_CPM_PCIE_INTR_MST_DECERR		26
> +#define XILINX_CPM_PCIE_INTR_MST_SLVERR		27
> +#define XILINX_CPM_PCIE_INTR_SLV_PCIE_TIMEOUT	28
> +
> +#define IMR(x)	BIT(XILINX_CPM_PCIE_INTR_ ##x)
> +
> +#define XILINX_CPM_PCIE_IMR_ALL_MASK			\
> +	(						\
> +		IMR(LINK_DOWN)		|		\
> +		IMR(HOT_RESET)		|		\
> +		IMR(CFG_PCIE_TIMEOUT)	|		\
> +		IMR(CFG_TIMEOUT)	|		\
> +		IMR(CORRECTABLE)	|		\
> +		IMR(NONFATAL)		|		\
> +		IMR(FATAL)		|		\
> +		IMR(CFG_ERR_POISON)	|		\
> +		IMR(PME_TO_ACK_RCVD)	|		\
> +		IMR(INTX)		|		\
> +		IMR(PM_PME_RCVD)	|		\
> +		IMR(SLV_UNSUPP)		|		\
> +		IMR(SLV_UNEXP)		|		\
> +		IMR(SLV_COMPL)		|		\
> +		IMR(SLV_ERRP)		|		\
> +		IMR(SLV_CMPABT)		|		\
> +		IMR(SLV_ILLBUR)		|		\
> +		IMR(MST_DECERR)		|		\
> +		IMR(MST_SLVERR)		|		\
> +		IMR(SLV_PCIE_TIMEOUT)			\
> +	)
> +
>   #define XILINX_CPM_PCIE_IDR_ALL_MASK		0xFFFFFFFF
>   #define XILINX_CPM_PCIE_IDRN_MASK		GENMASK(19, 16)
> -#define XILINX_CPM_PCIE_INTR_CFG_PCIE_TIMEOUT	BIT(4)
> -#define XILINX_CPM_PCIE_INTR_CFG_ERR_POISON	BIT(12)
> -#define XILINX_CPM_PCIE_INTR_PME_TO_ACK_RCVD	BIT(15)
> -#define XILINX_CPM_PCIE_INTR_PM_PME_RCVD	BIT(17)
> -#define XILINX_CPM_PCIE_INTR_SLV_PCIE_TIMEOUT	BIT(28)
>   #define XILINX_CPM_PCIE_IDRN_SHIFT		16
> 
>   /* Root Port Error FIFO Read Register definitions */ @@ -75,36 +103,37 @@
>    * @reg_base: Bridge Register Base
>    * @cpm_base: CPM System Level Control and Status Register(SLCR) Base
>    * @dev: Device pointer
> - * @leg_domain: Legacy IRQ domain pointer
> + * @intx_domain: Legacy IRQ domain pointer
>    * @cfg: Holds mappings of config space window
> - * @irq_misc: Legacy and error interrupt number
> - * @leg_mask_lock: lock for legacy interrupts
> + * @irq: INTx and error interrupt number
> + * @lock: lock protecting shared register access
>    */
>   struct xilinx_cpm_pcie_port {
> -	void __iomem *reg_base;
> -	void __iomem *cpm_base;
> -	struct device *dev;
> -	struct irq_domain *leg_domain;
> -	struct pci_config_window *cfg;
> -	int irq_misc;
> -	raw_spinlock_t leg_mask_lock;
> +	void __iomem			*reg_base;
> +	void __iomem			*cpm_base;
> +	struct device			*dev;
> +	struct irq_domain		*intx_domain;
> +	struct irq_domain		*cpm_domain;
> +	struct pci_config_window	*cfg;
> +	int				irq;
> +	raw_spinlock_t			lock;
>   };
> 
>   static inline u32 pcie_read(struct xilinx_cpm_pcie_port *port, u32 reg)
>   {
> -	return readl(port->reg_base + reg);
> +	return readl_relaxed(port->reg_base + reg);
>   }
> 
>   static inline void pcie_write(struct xilinx_cpm_pcie_port *port,
>   			      u32 val, u32 reg)
>   {
> -	writel(val, port->reg_base + reg);
> +	writel_relaxed(val, port->reg_base + reg);
>   }
> 
>   static inline bool cpm_pcie_link_up(struct xilinx_cpm_pcie_port *port)
>   {
>   	return (pcie_read(port, XILINX_CPM_PCIE_REG_PSCR) &
> -		XILINX_CPM_PCIE_REG_PSCR_LNKUP) ? 1 : 0;
> +		XILINX_CPM_PCIE_REG_PSCR_LNKUP);
>   }
> 
>   /**
> @@ -125,44 +154,56 @@ static void cpm_pcie_clear_err_interrupts(struct
> xilinx_cpm_pcie_port *port)
> 
>   static void xilinx_cpm_mask_leg_irq(struct irq_data *data)
>   {
> -	struct irq_desc *desc = irq_to_desc(data->irq);
> -	struct xilinx_cpm_pcie_port *port;
> +	struct xilinx_cpm_pcie_port *port =
> irq_data_get_irq_chip_data(data);
>   	unsigned long flags;
>   	u32 mask;
>   	u32 val;
> 
> -	port = irq_desc_get_chip_data(desc);
> -	mask = (1 << data->hwirq) << XILINX_CPM_PCIE_IDRN_SHIFT;
> -	raw_spin_lock_irqsave(&port->leg_mask_lock, flags);
> +	mask = BIT(data->hwirq + XILINX_CPM_PCIE_IDRN_SHIFT);
> +	raw_spin_lock_irqsave(&port->lock, flags);
>   	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN_MASK);
>   	pcie_write(port, (val & (~mask)),
> XILINX_CPM_PCIE_REG_IDRN_MASK);
> -	raw_spin_unlock_irqrestore(&port->leg_mask_lock, flags);
> +	raw_spin_unlock_irqrestore(&port->lock, flags);
>   }
> 
>   static void xilinx_cpm_unmask_leg_irq(struct irq_data *data)
>   {
> -	struct irq_desc *desc = irq_to_desc(data->irq);
> -	struct xilinx_cpm_pcie_port *port;
> +	struct xilinx_cpm_pcie_port *port =
> irq_data_get_irq_chip_data(data);
>   	unsigned long flags;
>   	u32 mask;
>   	u32 val;
> 
> -	port = irq_desc_get_chip_data(desc);
> -	mask = (1 << data->hwirq) << XILINX_CPM_PCIE_IDRN_SHIFT;
> -	raw_spin_lock_irqsave(&port->leg_mask_lock, flags);
> +	mask = BIT(data->hwirq + XILINX_CPM_PCIE_IDRN_SHIFT);
> +	raw_spin_lock_irqsave(&port->lock, flags);
>   	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN_MASK);
>   	pcie_write(port, (val | mask), XILINX_CPM_PCIE_REG_IDRN_MASK);
> -	raw_spin_unlock_irqrestore(&port->leg_mask_lock, flags);
> +	raw_spin_unlock_irqrestore(&port->lock, flags);
>   }
> 
>   static struct irq_chip xilinx_cpm_leg_irq_chip = {
> -	.name = "xilinx_cpm_pcie:legacy",
> -	.irq_enable = xilinx_cpm_unmask_leg_irq,
> -	.irq_disable = xilinx_cpm_mask_leg_irq,
> -	.irq_mask = xilinx_cpm_mask_leg_irq,
> -	.irq_unmask = xilinx_cpm_unmask_leg_irq,
> +	.name		= "INTx",
> +	.irq_mask	= xilinx_cpm_mask_leg_irq,
> +	.irq_unmask	= xilinx_cpm_unmask_leg_irq,
>   };
> 
> +static void xilinx_cpm_pcie_intx_flow(struct irq_desc *desc) {
> +	struct xilinx_cpm_pcie_port *port =
> irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned long val;
> +	int i;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	val = FIELD_GET(XILINX_CPM_PCIE_IDRN_MASK,
> +			pcie_read(port, XILINX_CPM_PCIE_REG_IDRN));
> +
> +	for_each_set_bit(i, &val, PCI_NUM_INTX)
> +		generic_handle_irq(irq_find_mapping(port->intx_domain, i));
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
>   /**
>    * xilinx_cpm_pcie_intx_map - Set the handler for the INTx and mark IRQ as
> valid
>    * @domain: IRQ domain
> @@ -187,111 +228,130 @@ static const struct irq_domain_ops
> intx_domain_ops = {
>   	.map = xilinx_cpm_pcie_intx_map,
>   };
> 
> -/**
> - * xilinx_cpm_pcie_intr_handler - Interrupt Service Handler
> - * @irq: IRQ number
> - * @data: PCIe port information
> - *
> - * Return: IRQ_HANDLED on success and IRQ_NONE on failure
> - */
> -static irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void *data)
> +static void xilinx_cpm_mask_event_irq(struct irq_data *d)
>   {
> -	struct xilinx_cpm_pcie_port *port = data;
> -	struct device *dev = port->dev;
> -	u32 val, mask, status, bit;
> -	unsigned long intr_val;
> -
> -	/* Read interrupt decode and mask registers */
> -	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDR);
> -	mask = pcie_read(port, XILINX_CPM_PCIE_REG_IMR);
> -
> -	status = val & mask;
> -	if (!status)
> -		return IRQ_NONE;
> -
> -	if (status & XILINX_CPM_PCIE_INTR_LINK_DOWN)
> -		dev_warn(dev, "Link Down\n");
> -
> -	if (status & XILINX_CPM_PCIE_INTR_HOT_RESET)
> -		dev_info(dev, "Hot reset\n");
> -
> -	if (status & XILINX_CPM_PCIE_INTR_CFG_TIMEOUT)
> -		dev_warn(dev, "ECAM access timeout\n");
> -
> -	if (status & XILINX_CPM_PCIE_INTR_CORRECTABLE) {
> -		dev_warn(dev, "Correctable error message\n");
> -		cpm_pcie_clear_err_interrupts(port);
> -	}
> -
> -	if (status & XILINX_CPM_PCIE_INTR_NONFATAL) {
> -		dev_warn(dev, "Non fatal error message\n");
> -		cpm_pcie_clear_err_interrupts(port);
> -	}
> -
> -	if (status & XILINX_CPM_PCIE_INTR_FATAL) {
> -		dev_warn(dev, "Fatal error message\n");
> -		cpm_pcie_clear_err_interrupts(port);
> -	}
> -
> -	if (status & XILINX_CPM_PCIE_INTR_INTX) {
> -		/* Handle INTx Interrupt */
> -		intr_val = pcie_read(port, XILINX_CPM_PCIE_REG_IDRN);
> -		intr_val = intr_val >> XILINX_CPM_PCIE_IDRN_SHIFT;
> -
> -		for_each_set_bit(bit, &intr_val, PCI_NUM_INTX)
> -			generic_handle_irq(irq_find_mapping(port-
> >leg_domain,
> -							    bit));
> -	}
> -
> -	if (status & XILINX_CPM_PCIE_INTR_SLV_UNSUPP)
> -		dev_warn(dev, "Slave unsupported request\n");
> 
> -	if (status & XILINX_CPM_PCIE_INTR_SLV_UNEXP)
> -		dev_warn(dev, "Slave unexpected completion\n");
> +	struct xilinx_cpm_pcie_port *port = irq_data_get_irq_chip_data(d);
> +	u32 val;
> 
> -	if (status & XILINX_CPM_PCIE_INTR_SLV_COMPL)
> -		dev_warn(dev, "Slave completion timeout\n");
> +	raw_spin_lock(&port->lock);
> +	val = pcie_read(port, XILINX_CPM_PCIE_REG_IMR);
> +	val &= ~d->hwirq;
> +	pcie_write(port, val, XILINX_CPM_PCIE_REG_IMR);
> +	raw_spin_unlock(&port->lock);
> +}
> 
> -	if (status & XILINX_CPM_PCIE_INTR_SLV_ERRP)
> -		dev_warn(dev, "Slave Error Poison\n");
> +static void xilinx_cpm_unmask_event_irq(struct irq_data *d) {
> +	struct xilinx_cpm_pcie_port *port = irq_data_get_irq_chip_data(d);
> +	u32 val;
> 
> -	if (status & XILINX_CPM_PCIE_INTR_SLV_CMPABT)
> -		dev_warn(dev, "Slave Completer Abort\n");
> +	raw_spin_lock(&port->lock);
> +	val = pcie_read(port, XILINX_CPM_PCIE_REG_IMR);
> +	val |= d->hwirq;
> +	pcie_write(port, val, XILINX_CPM_PCIE_REG_IMR);
> +	raw_spin_unlock(&port->lock);
> +}
> 
> -	if (status & XILINX_CPM_PCIE_INTR_SLV_ILLBUR)
> -		dev_warn(dev, "Slave Illegal Burst\n");
> +static struct irq_chip xilinx_cpm_event_irq_chip = {
> +	.name		= "RC-Event",
> +	.irq_mask	= xilinx_cpm_mask_event_irq,
> +	.irq_unmask	= xilinx_cpm_unmask_event_irq,
> +};
> 
> -	if (status & XILINX_CPM_PCIE_INTR_MST_DECERR)
> -		dev_warn(dev, "Master decode error\n");
> +static int xilinx_cpm_pcie_event_map(struct irq_domain *domain,
> +				    unsigned int irq, irq_hw_number_t hwirq)
> {
> +	irq_set_chip_and_handler(irq, &xilinx_cpm_event_irq_chip,
> +				 handle_level_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +	irq_set_status_flags(irq, IRQ_LEVEL);
> 
> -	if (status & XILINX_CPM_PCIE_INTR_MST_SLVERR)
> -		dev_warn(dev, "Master slave error\n");
> +	return 0;
> +}
> 
> -	if (status & XILINX_CPM_PCIE_INTR_CFG_PCIE_TIMEOUT)
> -		dev_warn(dev, "PCIe ECAM access timeout\n");
> +static const struct irq_domain_ops event_domain_ops = {
> +	.map = xilinx_cpm_pcie_event_map,
> +};
> 
> -	if (status & XILINX_CPM_PCIE_INTR_CFG_ERR_POISON)
> -		dev_warn(dev, "ECAM poisoned completion received\n");
> +static void xilinx_cpm_pcie_event_flow(struct irq_desc *desc) {
> +	struct xilinx_cpm_pcie_port *port =
> irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned long val;
> +	int i;
> 
> -	if (status & XILINX_CPM_PCIE_INTR_PME_TO_ACK_RCVD)
> -		dev_warn(dev, "PME_TO_ACK message received\n");
> +	chained_irq_enter(chip, desc);
> 
> -	if (status & XILINX_CPM_PCIE_INTR_PM_PME_RCVD)
> -		dev_warn(dev, "PM_PME message received\n");
> +	val =  pcie_read(port, XILINX_CPM_PCIE_REG_IDR);
> +	val &= pcie_read(port, XILINX_CPM_PCIE_REG_IMR);
> 
> -	if (status & XILINX_CPM_PCIE_INTR_SLV_PCIE_TIMEOUT)
> -		dev_warn(dev, "PCIe completion timeout received\n");
> +	for_each_set_bit(i, &val, 32)
> +		generic_handle_irq(irq_find_mapping(port->cpm_domain,
> i));
> 
>   	/* Clear the Interrupt Decode register */
> -	pcie_write(port, status, XILINX_CPM_PCIE_REG_IDR);
> +	pcie_write(port, val, XILINX_CPM_PCIE_REG_IDR);
> 
>   	/*
>   	 * XILINX_CPM_PCIE_MISC_IR_STATUS register is mapped to
>   	 * CPM SLCR block.
>   	 */
> -	val = readl(port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS);
> +	val = readl_relaxed(port->cpm_base +
> XILINX_CPM_PCIE_MISC_IR_STATUS);
>   	if (val)
> -		writel(val, port->cpm_base +
> XILINX_CPM_PCIE_MISC_IR_STATUS);
> +		writel_relaxed(val, port->cpm_base +
> XILINX_CPM_PCIE_MISC_IR_STATUS);
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +#define _IC(x, s)				\
> +	[XILINX_CPM_PCIE_INTR_ ## x] = { __stringify(x), s }
> +
> +static const struct {
> +	const char	*sym;
> +	const char	*str;
> +} intr_cause[32] = {
> +	_IC(LINK_DOWN,		"Link Down"),
> +	_IC(HOT_RESET,		"Hot reset"),
> +	_IC(CFG_TIMEOUT,	"ECAM access timeout"),
> +	_IC(CORRECTABLE,	"Correctable error message"),
> +	_IC(NONFATAL,		"Non fatal error message"),
> +	_IC(FATAL,		"Fatal error message"),
> +	_IC(SLV_UNSUPP,		"Slave unsupported request"),
> +	_IC(SLV_UNEXP,		"Slave unexpected completion"),
> +	_IC(SLV_COMPL,		"Slave completion timeout"),
> +	_IC(SLV_ERRP,		"Slave Error Poison"),
> +	_IC(SLV_CMPABT,		"Slave Completer Abort"),
> +	_IC(SLV_ILLBUR,		"Slave Illegal Burst"),
> +	_IC(MST_DECERR,		"Master decode error"),
> +	_IC(MST_SLVERR,		"Master slave error"),
> +	_IC(CFG_PCIE_TIMEOUT,	"PCIe ECAM access timeout"),
> +	_IC(CFG_ERR_POISON,	"ECAM poisoned completion received"),
> +	_IC(PME_TO_ACK_RCVD,	"PME_TO_ACK message received"),
> +	_IC(PM_PME_RCVD,	"PM_PME message received"),
> +	_IC(SLV_PCIE_TIMEOUT,	"PCIe completion timeout received"),
> +};
> +
> +static irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void *dev_id)
> +{
> +	struct xilinx_cpm_pcie_port *port = dev_id;
> +	struct device *dev = port->dev;
> +	struct irq_data *d;
> +
> +	d = irq_domain_get_irq_data(port->cpm_domain, irq);
> +
> +	switch(d->hwirq) {
> +	case XILINX_CPM_PCIE_INTR_CORRECTABLE:
> +	case XILINX_CPM_PCIE_INTR_NONFATAL:
> +	case XILINX_CPM_PCIE_INTR_FATAL:
> +		cpm_pcie_clear_err_interrupts(port);
> +		fallthrough;
> +
> +	default:
> +		if (intr_cause[d->hwirq].str)
> +			dev_warn(dev, "%s\n", intr_cause[d->hwirq].str);
> +		else
> +			dev_warn(dev, "Unknown interrupt\n");
> +	}
> 
>   	return IRQ_HANDLED;
>   }
> @@ -315,17 +375,41 @@ static int xilinx_cpm_pcie_init_irq_domain(struct
> xilinx_cpm_pcie_port *port)
>   		return -EINVAL;
>   	}
> 
> -	port->leg_domain = irq_domain_add_linear(pcie_intc_node,
> PCI_NUM_INTX,
> +	port->cpm_domain = irq_domain_add_linear(pcie_intc_node, 32,
> +						 &event_domain_ops,
> +						 port);
> +	if (!port->cpm_domain)
> +		goto out;
> +
> +	irq_domain_update_bus_token(port->cpm_domain,
> DOMAIN_BUS_NEXUS);
> +
> +	port->intx_domain = irq_domain_add_linear(pcie_intc_node,
> PCI_NUM_INTX,
>   						 &intx_domain_ops,
>   						 port);
> +	if (!port->intx_domain)
> +		goto out;
> +
> +	irq_domain_update_bus_token(port->intx_domain,
> DOMAIN_BUS_WIRED);
> +
>   	of_node_put(pcie_intc_node);
> -	if (!port->leg_domain) {
> -		dev_err(dev, "Failed to get a INTx IRQ domain\n");
> -		return -ENOMEM;
> -	}
> +	raw_spin_lock_init(&port->lock);
> 
> -	raw_spin_lock_init(&port->leg_mask_lock);
>   	return 0;
> +
> +out:
> +	of_node_put(pcie_intc_node);
> +	if (port->intx_domain) {
> +		irq_domain_remove(port->intx_domain);
> +		port->intx_domain = NULL;
> +	}
> +
> +	if (port->cpm_domain) {
> +		irq_domain_remove(port->cpm_domain);
> +		port->cpm_domain = NULL;
> +	}
> +
> +	dev_err(dev, "Failed to allocate IRQ domains\n");
> +	return -ENOMEM;
>   }
> 
>   /**
> @@ -348,12 +432,6 @@ static void xilinx_cpm_pcie_init_port(struct
> xilinx_cpm_pcie_port *port)
>   		   XILINX_CPM_PCIE_IMR_ALL_MASK,
>   		   XILINX_CPM_PCIE_REG_IDR);
> 
> -	/* Enable all interrupts */
> -	pcie_write(port, XILINX_CPM_PCIE_IMR_ALL_MASK,
> -		   XILINX_CPM_PCIE_REG_IMR);
> -	pcie_write(port, XILINX_CPM_PCIE_IDRN_MASK,
> -		   XILINX_CPM_PCIE_REG_IDRN_MASK);
> -
>   	/*
>   	 * XILINX_CPM_PCIE_MISC_IR_ENABLE register is mapped to
>   	 * CPM SLCR block.
> @@ -366,28 +444,45 @@ static void xilinx_cpm_pcie_init_port(struct
> xilinx_cpm_pcie_port *port)
>   		   XILINX_CPM_PCIE_REG_RPSC);
>   }
> 
> -static int xilinx_cpm_request_misc_irq(struct xilinx_cpm_pcie_port
> *port)
> +static int xilinx_cpm_setup_irq(struct xilinx_cpm_pcie_port *port)
>   {
>   	struct device *dev = port->dev;
>   	struct platform_device *pdev = to_platform_device(dev);
> -	int err;
> +	int i, irq;
> 
> -	port->irq_misc = platform_get_irq(pdev, 0);
> -	if (port->irq_misc <= 0) {
> -		dev_err(dev, "Unable to find misc IRQ line\n");
> -		return port->irq_misc;
> +	port->irq = platform_get_irq(pdev, 0);
> +	if (port->irq < 0) {
> +		dev_err(dev, "Unable to find IRQ line\n");
> +		return port->irq;
>   	}
> 
> -	err = devm_request_irq(dev, port->irq_misc,
> -			       xilinx_cpm_pcie_intr_handler,
> -			       IRQF_SHARED | IRQF_NO_THREAD,
> -			       "xilinx-pcie", port);
> -	if (err) {
> -		dev_err(dev, "unable to request misc IRQ line %d\n",
> -			port->irq_misc);
> -		return err;
> +	for (i = 0; i < ARRAY_SIZE(intr_cause); i++) {
> +		int err;
> +
> +		if (!intr_cause[i].str)
> +			continue;
> +
> +		irq = irq_create_mapping(port->cpm_domain, i);
> +		if (WARN_ON(irq <= 0))
> +			return -ENXIO;
> +
> +		err = devm_request_irq(dev, irq,
> xilinx_cpm_pcie_intr_handler,
> +				       0, intr_cause[i].sym, port);
> +		if (WARN_ON(err))
> +			return err;
>   	}
> 
> +	irq = irq_create_mapping(port->cpm_domain,
> XILINX_CPM_PCIE_INTR_INTX);
> +	if (WARN_ON(irq <= 0))
> +		return -ENXIO;
> +
> +	/* Plug the INTx chained handler */
> +	irq_set_chained_handler_and_data(irq, xilinx_cpm_pcie_intx_flow,
> port);
> +
> +	/* Plug the main event chained handler */
> +	irq_set_chained_handler_and_data(port->irq,
> xilinx_cpm_pcie_event_flow,
> +					 port);
> +
>   	return 0;
>   }
> 
> @@ -422,7 +517,7 @@ static int xilinx_cpm_pcie_parse_dt(struct
> xilinx_cpm_pcie_port *port,
>   	if (IS_ERR(port->cpm_base))
>   		return PTR_ERR(port->cpm_base);
> 
> -	err = xilinx_cpm_request_misc_irq(port);
> +	err = xilinx_cpm_setup_irq(port);
>   	if (err)
>   		return err;
> 
> @@ -481,8 +576,10 @@ static int xilinx_cpm_pcie_probe(struct
> platform_device *pdev)
> 
>   	err = pci_host_probe(bridge);
>   	if (err < 0) {
> -		irq_domain_remove(port->leg_domain);
> -		devm_free_irq(dev, port->irq_misc, port);
> +		if (port->intx_domain)
> +			irq_domain_remove(port->intx_domain);
> +		if (port->cpm_domain)
> +			irq_domain_remove(port->cpm_domain);
>   		return err;
>   	}
> 
> --
> 2.26.2
> 
> 
> --
> Jazz is not dead. It just smells funny...