[RESEND PATCH v5 0/3] Add support for AMD MDB IP as Root Port

[RESEND PATCH v5 0/3] Add support for AMD MDB IP as Root Port

[Thippeswamy Havalige]

This series of patch add support for AMD MDB IP as Root Port.

The AMD MDB IP support's 32 bit and 64bit BAR's at Gen5 speed.
As Root Port it supports MSI and legacy interrupts.

Thippeswamy Havalige (3):
  dt-bindings: PCI: dwc: Add AMD Versal2 mdb slcr support
  dt-bindings: PCI: amd-mdb: Add AMD Versal2 MDB PCIe Root Port Bridge
  PCI: amd-mdb: Add AMD MDB Root Port driver

 .../bindings/pci/amd,versal2-mdb-host.yaml    | 121 +++++
 .../devicetree/bindings/pci/snps,dw-pcie.yaml |   2 +
 drivers/pci/controller/dwc/Kconfig            |  10 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pcie-amd-mdb.c     | 439 ++++++++++++++++++
 5 files changed, 573 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
 create mode 100644 drivers/pci/controller/dwc/pcie-amd-mdb.c

-- 
2.34.1

[RESEND PATCH v5 1/3] dt-bindings: PCI: dwc: Add AMD Versal2 mdb slcr support

[Thippeswamy Havalige]

Add support for mdb slcr aperture that is only supported for AMD Versal2
devices.

Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
---
Changes in v3:
-------------
- Introduced below changes in dwc yaml schema.
Changes in v5:
-------------
- Modify mdb_pcie_slcr as constant.
---
 Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
index 548f59d76ef2..696568e81cfc 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
@@ -113,6 +113,8 @@ properties:
               enum: [ smu, mpu ]
             - description: Tegra234 aperture
               enum: [ ecam ]
+            - description: AMD MDB PCIe slcr region
+              const: mdb_pcie_slcr
     allOf:
       - contains:
           const: dbi
-- 
2.34.1

Re: [RESEND PATCH v5 1/3] dt-bindings: PCI: dwc: Add AMD Versal2 mdb slcr support

[Rob Herring]

On Fri, Dec 13, 2024 at 12:10:33PM +0530, Thippeswamy Havalige wrote:
> Add support for mdb slcr aperture that is only supported for AMD Versal2
> devices.
> 
> Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
> ---
> Changes in v3:
> -------------
> - Introduced below changes in dwc yaml schema.
> Changes in v5:
> -------------
> - Modify mdb_pcie_slcr as constant.
> ---
>  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> index 548f59d76ef2..696568e81cfc 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> @@ -113,6 +113,8 @@ properties:
>                enum: [ smu, mpu ]
>              - description: Tegra234 aperture
>                enum: [ ecam ]
> +            - description: AMD MDB PCIe slcr region
> +              const: mdb_pcie_slcr

Including the block name is redundant. Just 'slcr' is sufficient.

>      allOf:
>        - contains:
>            const: dbi
> -- 
> 2.34.1
> 

RE: [RESEND PATCH v5 1/3] dt-bindings: PCI: dwc: Add AMD Versal2 mdb slcr support

[Havalige, Thippeswamy]

Hi Rob,
> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Tuesday, December 17, 2024 8:31 PM
> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>
> Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> manivannan.sadhasivam@linaro.org; krzk+dt@kernel.org; conor+dt@kernel.org;
> linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; jingoohan1@gmail.com; Simek, Michal
> <michal.simek@amd.com>; Gogada, Bharat Kumar
> <bharat.kumar.gogada@amd.com>
> Subject: Re: [RESEND PATCH v5 1/3] dt-bindings: PCI: dwc: Add AMD Versal2 mdb
> slcr support
> 
> On Fri, Dec 13, 2024 at 12:10:33PM +0530, Thippeswamy Havalige wrote:
> > Add support for mdb slcr aperture that is only supported for AMD
> > Versal2 devices.
> >
> > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
> > ---
> > Changes in v3:
> > -------------
> > - Introduced below changes in dwc yaml schema.
> > Changes in v5:
> > -------------
> > - Modify mdb_pcie_slcr as constant.
> > ---
> >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > index 548f59d76ef2..696568e81cfc 100644
> > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > @@ -113,6 +113,8 @@ properties:
> >                enum: [ smu, mpu ]
> >              - description: Tegra234 aperture
> >                enum: [ ecam ]
> > +            - description: AMD MDB PCIe slcr region
> > +              const: mdb_pcie_slcr
> 
> Including the block name is redundant. Just 'slcr' is sufficient.
Thank you for your comments, I'll update this in next patch.
> 
> >      allOf:
> >        - contains:
> >            const: dbi
> > --
> > 2.34.1
> >

[RESEND PATCH v5 2/3] dt-bindings: PCI: amd-mdb: Add AMD Versal2 MDB PCIe Root Port Bridge

[Thippeswamy Havalige]

Add AMD Versal2 MDB (Multimedia DMA Bridge) PCIe Root Port Bridge.

Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
---
Changes in v2:
-------------
- Modify patch subject.
- Add pcie host bridge reference.
- Modify filename as per compatible string.
- Remove standard PCI properties.
- Modify interrupt controller description.
- Indentation

Changes in v3:
-------------
- Modified SLCR to lower case.
- Add dwc schemas.
- Remove common properties.
- Move additionalProperties below properties.
- Remove ranges property from required properties.
- Drop blank line.
- Modify pci@ to pcie@

Changes in v4:
-------------
- None.

Changes in v5:
-------------
- None.
---
 .../bindings/pci/amd,versal2-mdb-host.yaml    | 121 ++++++++++++++++++
---
 .../bindings/pci/amd,versal2-mdb-host.yaml    | 121 ++++++++++++++++++
 1 file changed, 121 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml

diff --git a/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml b/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
new file mode 100644
index 000000000000..c319adeeee66
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
@@ -0,0 +1,121 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/amd,versal2-mdb-host.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMD Versal2 MDB(Multimedia DMA Bridge) Host Controller
+
+maintainers:
+  - Thippeswamy Havalige <thippeswamy.havalige@amd.com>
+
+allOf:
+  - $ref: /schemas/pci/pci-host-bridge.yaml#
+  - $ref: /schemas/pci/snps,dw-pcie.yaml#
+
+properties:
+  compatible:
+    const: amd,versal2-mdb-host
+
+  reg:
+    items:
+      - description: MDB PCIe controller 0 SLCR
+      - description: configuration region
+      - description: data bus interface
+      - description: address translation unit register
+
+  reg-names:
+    items:
+      - const: mdb_pcie_slcr
+      - const: config
+      - const: dbi
+      - const: atu
+
+  ranges:
+    maxItems: 2
+
+  msi-map:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-map-mask:
+    items:
+      - const: 0
+      - const: 0
+      - const: 0
+      - const: 7
+
+  interrupt-map:
+    maxItems: 4
+
+  "#interrupt-cells":
+    const: 1
+
+  interrupt-controller:
+    description: identifies the node as an interrupt controller
+    type: object
+    additionalProperties: false
+    properties:
+      interrupt-controller: true
+
+      "#address-cells":
+        const: 0
+
+      "#interrupt-cells":
+        const: 1
+
+    required:
+      - interrupt-controller
+      - "#address-cells"
+      - "#interrupt-cells"
+
+required:
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-map
+  - interrupt-map-mask
+  - msi-map
+  - "#interrupt-cells"
+  - interrupt-controller
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+        pcie@ed931000 {
+            compatible = "amd,versal2-mdb-host";
+            reg = <0x0 0xed931000 0x0 0x2000>,
+                  <0x1000 0x100000 0x0 0xff00000>,
+                  <0x1000 0x0 0x0 0x100000>,
+                  <0x0 0xed860000 0x0 0x2000>;
+            reg-names = "mdb_pcie_slcr", "config", "dbi", "atu";
+            ranges = <0x2000000 0x00 0xa8000000 0x00 0xa8000000 0x00 0x10000000>,
+                     <0x43000000 0x1100 0x00 0x1100 0x00 0x00 0x1000000>;
+            interrupts = <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>;
+            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>;
+            msi-map = <0x0 &gic_its 0x00 0x10000>;
+            #address-cells = <3>;
+            #size-cells = <2>;
+            #interrupt-cells = <1>;
+            device_type = "pci";
+            pcie_intc_0: interrupt-controller {
+                #address-cells = <0>;
+                #interrupt-cells = <1>;
+                interrupt-controller;
+           };
+        };
+    };
-- 
2.34.1

Re: [RESEND PATCH v5 2/3] dt-bindings: PCI: amd-mdb: Add AMD Versal2 MDB PCIe Root Port Bridge

[Rob Herring]

On Fri, Dec 13, 2024 at 12:10:34PM +0530, Thippeswamy Havalige wrote:
> Add AMD Versal2 MDB (Multimedia DMA Bridge) PCIe Root Port Bridge.
> 
> Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
> ---
> Changes in v2:
> -------------
> - Modify patch subject.
> - Add pcie host bridge reference.
> - Modify filename as per compatible string.
> - Remove standard PCI properties.
> - Modify interrupt controller description.
> - Indentation
> 
> Changes in v3:
> -------------
> - Modified SLCR to lower case.
> - Add dwc schemas.
> - Remove common properties.
> - Move additionalProperties below properties.
> - Remove ranges property from required properties.
> - Drop blank line.
> - Modify pci@ to pcie@
> 
> Changes in v4:
> -------------
> - None.
> 
> Changes in v5:
> -------------
> - None.
> ---
>  .../bindings/pci/amd,versal2-mdb-host.yaml    | 121 ++++++++++++++++++
> ---
>  .../bindings/pci/amd,versal2-mdb-host.yaml    | 121 ++++++++++++++++++
>  1 file changed, 121 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml b/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
> new file mode 100644
> index 000000000000..c319adeeee66
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
> @@ -0,0 +1,121 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/amd,versal2-mdb-host.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMD Versal2 MDB(Multimedia DMA Bridge) Host Controller
> +
> +maintainers:
> +  - Thippeswamy Havalige <thippeswamy.havalige@amd.com>
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-host-bridge.yaml#
> +  - $ref: /schemas/pci/snps,dw-pcie.yaml#
> +
> +properties:
> +  compatible:
> +    const: amd,versal2-mdb-host
> +
> +  reg:
> +    items:
> +      - description: MDB PCIe controller 0 SLCR
> +      - description: configuration region
> +      - description: data bus interface
> +      - description: address translation unit register
> +
> +  reg-names:
> +    items:
> +      - const: mdb_pcie_slcr
> +      - const: config
> +      - const: dbi
> +      - const: atu
> +
> +  ranges:
> +    maxItems: 2
> +
> +  msi-map:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-map-mask:
> +    items:
> +      - const: 0
> +      - const: 0
> +      - const: 0
> +      - const: 7
> +
> +  interrupt-map:
> +    maxItems: 4
> +
> +  "#interrupt-cells":
> +    const: 1
> +
> +  interrupt-controller:
> +    description: identifies the node as an interrupt controller
> +    type: object
> +    additionalProperties: false
> +    properties:
> +      interrupt-controller: true
> +
> +      "#address-cells":
> +        const: 0
> +
> +      "#interrupt-cells":
> +        const: 1
> +
> +    required:
> +      - interrupt-controller
> +      - "#address-cells"
> +      - "#interrupt-cells"
> +
> +required:
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - interrupt-map
> +  - interrupt-map-mask
> +  - msi-map
> +  - "#interrupt-cells"
> +  - interrupt-controller
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +        pcie@ed931000 {
> +            compatible = "amd,versal2-mdb-host";
> +            reg = <0x0 0xed931000 0x0 0x2000>,
> +                  <0x1000 0x100000 0x0 0xff00000>,
> +                  <0x1000 0x0 0x0 0x100000>,

DBI space is 1MB? Last I checked, there's less than 4KB worth of 
registers.

The address looks odd. The config space is purely iATU configuration. 
Really, we should have described the entire address space (like the 
endpoint) available to the ATU. So the 1MB offset in the base 
address seems like just that. Most h/w design to cut down signal 
routing would put the base address for the ATU input at something 
aligned greater than the size of the address space. 

> +                  <0x0 0xed860000 0x0 0x2000>;

And then the DBI and ATU registers are nowhere near each other? 
Possible, but seems odd.

> +            reg-names = "mdb_pcie_slcr", "config", "dbi", "atu";
> +            ranges = <0x2000000 0x00 0xa8000000 0x00 0xa8000000 0x00 0x10000000>,
> +                     <0x43000000 0x1100 0x00 0x1100 0x00 0x00 0x1000000>;
> +            interrupts = <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>;
> +            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>;
> +            msi-map = <0x0 &gic_its 0x00 0x10000>;
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +            #interrupt-cells = <1>;
> +            device_type = "pci";
> +            pcie_intc_0: interrupt-controller {
> +                #address-cells = <0>;
> +                #interrupt-cells = <1>;
> +                interrupt-controller;
> +           };
> +        };
> +    };
> -- 
> 2.34.1
> 

RE: [RESEND PATCH v5 2/3] dt-bindings: PCI: amd-mdb: Add AMD Versal2 MDB PCIe Root Port Bridge

[Havalige, Thippeswamy]

Hi Rob,
> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Tuesday, December 17, 2024 8:41 PM
> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>
> Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> manivannan.sadhasivam@linaro.org; krzk+dt@kernel.org; conor+dt@kernel.org;
> linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; jingoohan1@gmail.com; Simek, Michal
> <michal.simek@amd.com>; Gogada, Bharat Kumar
> <bharat.kumar.gogada@amd.com>
> Subject: Re: [RESEND PATCH v5 2/3] dt-bindings: PCI: amd-mdb: Add AMD
> Versal2 MDB PCIe Root Port Bridge
> 
> On Fri, Dec 13, 2024 at 12:10:34PM +0530, Thippeswamy Havalige wrote:
> > Add AMD Versal2 MDB (Multimedia DMA Bridge) PCIe Root Port Bridge.
> >
> > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
> > ---
> > Changes in v2:
> > -------------
> > - Modify patch subject.
> > - Add pcie host bridge reference.
> > - Modify filename as per compatible string.
> > - Remove standard PCI properties.
> > - Modify interrupt controller description.
> > - Indentation
> >
> > Changes in v3:
> > -------------
> > - Modified SLCR to lower case.
> > - Add dwc schemas.
> > - Remove common properties.
> > - Move additionalProperties below properties.
> > - Remove ranges property from required properties.
> > - Drop blank line.
> > - Modify pci@ to pcie@
> >
> > Changes in v4:
> > -------------
> > - None.
> >
> > Changes in v5:
> > -------------
> > - None.
> > ---
> >  .../bindings/pci/amd,versal2-mdb-host.yaml    | 121 ++++++++++++++++++
> > ---
> >  .../bindings/pci/amd,versal2-mdb-host.yaml    | 121 ++++++++++++++++++
> >  1 file changed, 121 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/amd,versal2-mdb-
> host.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
> b/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
> > new file mode 100644
> > index 000000000000..c319adeeee66
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
> > @@ -0,0 +1,121 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/amd,versal2-mdb-host.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: AMD Versal2 MDB(Multimedia DMA Bridge) Host Controller
> > +
> > +maintainers:
> > +  - Thippeswamy Havalige <thippeswamy.havalige@amd.com>
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/pci-host-bridge.yaml#
> > +  - $ref: /schemas/pci/snps,dw-pcie.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: amd,versal2-mdb-host
> > +
> > +  reg:
> > +    items:
> > +      - description: MDB PCIe controller 0 SLCR
> > +      - description: configuration region
> > +      - description: data bus interface
> > +      - description: address translation unit register
> > +
> > +  reg-names:
> > +    items:
> > +      - const: mdb_pcie_slcr
> > +      - const: config
> > +      - const: dbi
> > +      - const: atu
> > +
> > +  ranges:
> > +    maxItems: 2
> > +
> > +  msi-map:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-map-mask:
> > +    items:
> > +      - const: 0
> > +      - const: 0
> > +      - const: 0
> > +      - const: 7
> > +
> > +  interrupt-map:
> > +    maxItems: 4
> > +
> > +  "#interrupt-cells":
> > +    const: 1
> > +
> > +  interrupt-controller:
> > +    description: identifies the node as an interrupt controller
> > +    type: object
> > +    additionalProperties: false
> > +    properties:
> > +      interrupt-controller: true
> > +
> > +      "#address-cells":
> > +        const: 0
> > +
> > +      "#interrupt-cells":
> > +        const: 1
> > +
> > +    required:
> > +      - interrupt-controller
> > +      - "#address-cells"
> > +      - "#interrupt-cells"
> > +
> > +required:
> > +  - reg
> > +  - reg-names
> > +  - interrupts
> > +  - interrupt-map
> > +  - interrupt-map-mask
> > +  - msi-map
> > +  - "#interrupt-cells"
> > +  - interrupt-controller
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +        pcie@ed931000 {
> > +            compatible = "amd,versal2-mdb-host";
> > +            reg = <0x0 0xed931000 0x0 0x2000>,
> > +                  <0x1000 0x100000 0x0 0xff00000>,
> > +                  <0x1000 0x0 0x0 0x100000>,
> 
> DBI space is 1MB? Last I checked, there's less than 4KB worth of
> registers.
Thank you for your feedback. I will reduce the size to 4KB, as the DBI space primarily
uses less than 4KB for its registers. Beyond this, the space includes port logic registers,
which can be safely ignored in this context.
> The address looks odd. The config space is purely iATU configuration.
> Really, we should have described the entire address space (like the
> endpoint) available to the ATU. So the 1MB offset in the base
> address seems like just that. Most h/w design to cut down signal
> routing would put the base address for the ATU input at something
> aligned greater than the size of the address space.
In AMD (Xilinx) PCIe IPs, the configuration space for the endpoint typically starts after 1MB.
To align with this, I initially set the DBI size to 1MB. However, considering the actual utilization of less
than 4KB for DBI registers, this allocation I'll update in the next patch.
> 
> > +                  <0x0 0xed860000 0x0 0x2000>;
> 
> And then the DBI and ATU registers are nowhere near each other?
> Possible, but seems odd.

Thank you for your feedback. The DBI address provided in the device tree serves as
one way to access the local ECAM space, which corresponds to a relative address
of 0xED840000.

The internal IP uses the 0xED840000 address to configure all PCIe attributes.
When accessing the address 0x1000_0000_0000, PCIe internally translates and
implicitly accesses the 0xED840000 address.

> > +            reg-names = "mdb_pcie_slcr", "config", "dbi", "atu";
> > +            ranges = <0x2000000 0x00 0xa8000000 0x00 0xa8000000 0x00
> 0x10000000>,
> > +                     <0x43000000 0x1100 0x00 0x1100 0x00 0x00 0x1000000>;
> > +            interrupts = <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>;
> > +            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>;
> > +            msi-map = <0x0 &gic_its 0x00 0x10000>;
> > +            #address-cells = <3>;
> > +            #size-cells = <2>;
> > +            #interrupt-cells = <1>;
> > +            device_type = "pci";
> > +            pcie_intc_0: interrupt-controller {
> > +                #address-cells = <0>;
> > +                #interrupt-cells = <1>;
> > +                interrupt-controller;
> > +           };
> > +        };
> > +    };
> > --
> > 2.34.1
> >

Regards,
Thippeswamy H

Re: [RESEND PATCH v5 2/3] dt-bindings: PCI: amd-mdb: Add AMD Versal2 MDB PCIe Root Port Bridge

[Rob Herring]

On Wed, Dec 18, 2024 at 3:28 AM Havalige, Thippeswamy
<thippeswamy.havalige@amd.com> wrote:
>
> Hi Rob,
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > Sent: Tuesday, December 17, 2024 8:41 PM
> > To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>
> > Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> > manivannan.sadhasivam@linaro.org; krzk+dt@kernel.org; conor+dt@kernel.org;
> > linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > kernel@vger.kernel.org; jingoohan1@gmail.com; Simek, Michal
> > <michal.simek@amd.com>; Gogada, Bharat Kumar
> > <bharat.kumar.gogada@amd.com>
> > Subject: Re: [RESEND PATCH v5 2/3] dt-bindings: PCI: amd-mdb: Add AMD
> > Versal2 MDB PCIe Root Port Bridge
> >
> > On Fri, Dec 13, 2024 at 12:10:34PM +0530, Thippeswamy Havalige wrote:
> > > Add AMD Versal2 MDB (Multimedia DMA Bridge) PCIe Root Port Bridge.
> > >
> > > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
> > > ---
> > > Changes in v2:
> > > -------------
> > > - Modify patch subject.
> > > - Add pcie host bridge reference.
> > > - Modify filename as per compatible string.
> > > - Remove standard PCI properties.
> > > - Modify interrupt controller description.
> > > - Indentation
> > >
> > > Changes in v3:
> > > -------------
> > > - Modified SLCR to lower case.
> > > - Add dwc schemas.
> > > - Remove common properties.
> > > - Move additionalProperties below properties.
> > > - Remove ranges property from required properties.
> > > - Drop blank line.
> > > - Modify pci@ to pcie@
> > >
> > > Changes in v4:
> > > -------------
> > > - None.
> > >
> > > Changes in v5:
> > > -------------
> > > - None.
> > > ---
> > >  .../bindings/pci/amd,versal2-mdb-host.yaml    | 121 ++++++++++++++++++
> > > ---
> > >  .../bindings/pci/amd,versal2-mdb-host.yaml    | 121 ++++++++++++++++++
> > >  1 file changed, 121 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/pci/amd,versal2-mdb-
> > host.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
> > b/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
> > > new file mode 100644
> > > index 000000000000..c319adeeee66
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
> > > @@ -0,0 +1,121 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/pci/amd,versal2-mdb-host.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: AMD Versal2 MDB(Multimedia DMA Bridge) Host Controller
> > > +
> > > +maintainers:
> > > +  - Thippeswamy Havalige <thippeswamy.havalige@amd.com>
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/pci/pci-host-bridge.yaml#
> > > +  - $ref: /schemas/pci/snps,dw-pcie.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: amd,versal2-mdb-host
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: MDB PCIe controller 0 SLCR
> > > +      - description: configuration region
> > > +      - description: data bus interface
> > > +      - description: address translation unit register
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: mdb_pcie_slcr
> > > +      - const: config
> > > +      - const: dbi
> > > +      - const: atu
> > > +
> > > +  ranges:
> > > +    maxItems: 2
> > > +
> > > +  msi-map:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  interrupt-map-mask:
> > > +    items:
> > > +      - const: 0
> > > +      - const: 0
> > > +      - const: 0
> > > +      - const: 7
> > > +
> > > +  interrupt-map:
> > > +    maxItems: 4
> > > +
> > > +  "#interrupt-cells":
> > > +    const: 1
> > > +
> > > +  interrupt-controller:
> > > +    description: identifies the node as an interrupt controller
> > > +    type: object
> > > +    additionalProperties: false
> > > +    properties:
> > > +      interrupt-controller: true
> > > +
> > > +      "#address-cells":
> > > +        const: 0
> > > +
> > > +      "#interrupt-cells":
> > > +        const: 1
> > > +
> > > +    required:
> > > +      - interrupt-controller
> > > +      - "#address-cells"
> > > +      - "#interrupt-cells"
> > > +
> > > +required:
> > > +  - reg
> > > +  - reg-names
> > > +  - interrupts
> > > +  - interrupt-map
> > > +  - interrupt-map-mask
> > > +  - msi-map
> > > +  - "#interrupt-cells"
> > > +  - interrupt-controller
> > > +
> > > +unevaluatedProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +
> > > +    soc {
> > > +        #address-cells = <2>;
> > > +        #size-cells = <2>;
> > > +        pcie@ed931000 {
> > > +            compatible = "amd,versal2-mdb-host";
> > > +            reg = <0x0 0xed931000 0x0 0x2000>,
> > > +                  <0x1000 0x100000 0x0 0xff00000>,
> > > +                  <0x1000 0x0 0x0 0x100000>,
> >
> > DBI space is 1MB? Last I checked, there's less than 4KB worth of
> > registers.
> Thank you for your feedback. I will reduce the size to 4KB, as the DBI space primarily
> uses less than 4KB for its registers. Beyond this, the space includes port logic registers,
> which can be safely ignored in this context.
> > The address looks odd. The config space is purely iATU configuration.
> > Really, we should have described the entire address space (like the
> > endpoint) available to the ATU. So the 1MB offset in the base
> > address seems like just that. Most h/w design to cut down signal
> > routing would put the base address for the ATU input at something
> > aligned greater than the size of the address space.
> In AMD (Xilinx) PCIe IPs, the configuration space for the endpoint typically starts after 1MB.
> To align with this, I initially set the DBI size to 1MB. However, considering the actual utilization of less
> than 4KB for DBI registers, this allocation I'll update in the next patch.
> >
> > > +                  <0x0 0xed860000 0x0 0x2000>;
> >
> > And then the DBI and ATU registers are nowhere near each other?
> > Possible, but seems odd.
>
> Thank you for your feedback. The DBI address provided in the device tree serves as
> one way to access the local ECAM space, which corresponds to a relative address
> of 0xED840000.
>
> The internal IP uses the 0xED840000 address to configure all PCIe attributes.
> When accessing the address 0x1000_0000_0000, PCIe internally translates and
> implicitly accesses the 0xED840000 address.

Using the 0xED840000 address would be more aligned with how everyone
else accesses the DBI. If you support ECAM, then doesn't
0x1000_0010_0000 also correspond to the same registers in the DBI
space? Though I thought only the generic PCI config space registers
would be exposed there.

Speaking of ECAM, if that's supported, it would be better if the
driver used that (there's not any support in the driver for that). The
advantage would be you could skip reconfiguring the iATU on every
access. Ideally, the result would be utilizing what's in
drivers/pci/ecam.c. Not something that needs to be done in this
series, but you need to make sure the DT correctly describes the ECAM
region (seems like it should be large enough). The hisi driver does
this though it suffers from not handling devfn>0 on the root bus
correctly (also a quirk in the generic ECAM based driver for DWC based
implementations) and also needs 32-bit accesses to the root bus.

Rob

[RESEND PATCH v5 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver

[Thippeswamy Havalige]

Add support for AMD MDB(Multimedia DMA Bridge) IP core as Root Port.

The Versal2 devices include MDB Module. The integrated block for MDB along
with the integrated bridge can function as PCIe Root Port controller at
Gen5 32-Gb/s operation per lane.

Bridge supports error and legacy interrupts and are handled using platform
specific interrupt line in Versal2.

Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
---
changes in v2:
-------------
- Update Gen5 speed in the patch description.
- Modify Kconfig file.
- Update string _leg_ to intx.
- Get platform structure through automic variables.
- Remove _rp_ in function.
Changes in v3:
--------------
-None.
Changes in v4:
--------------
-None.
Changes in v5:
--------------
-None.
---
 drivers/pci/controller/dwc/Kconfig        |  10 +
 drivers/pci/controller/dwc/Makefile       |   1 +
 drivers/pci/controller/dwc/pcie-amd-mdb.c | 439 ++++++++++++++++++++++
 3 files changed, 450 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-amd-mdb.c

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index b6d6778b0698..30d0a3eadf1d 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -27,6 +27,16 @@ config PCIE_AL
 	  required only for DT-based platforms. ACPI platforms with the
 	  Annapurna Labs PCIe controller don't need to enable this.
 
+config PCIE_AMD_MDB
+	bool "AMD PCIe controller (host mode)"
+	depends on OF || COMPILE_TEST
+	depends on PCI && PCI_MSI
+	select PCIE_DW_HOST
+	help
+	  Say Y here to enable PCIe controller support on AMD SoCs. The
+	  PCIe controller is based on DesignWare Hardware and uses AMD
+	  hardware wrappers.
+
 config PCI_MESON
 	tristate "Amlogic Meson PCIe controller"
 	default m if ARCH_MESON
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index a8308d9ea986..ae27eda6ec5e 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
 obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
 obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
 obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
+obj-$(CONFIG_PCIE_AMD_MDB) += pcie-amd-mdb.o
 obj-$(CONFIG_PCIE_BT1) += pcie-bt1.o
 obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
 obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
diff --git a/drivers/pci/controller/dwc/pcie-amd-mdb.c b/drivers/pci/controller/dwc/pcie-amd-mdb.c
new file mode 100644
index 000000000000..3947aad13ea0
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
@@ -0,0 +1,439 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe host controller driver for AMD MDB PCIe Bridge
+ *
+ * Copyright (C) 2024-2025, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/of_device.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/types.h>
+
+#include "pcie-designware.h"
+
+#define AMD_MDB_TLP_IR_STATUS_MISC		0x4C0
+#define AMD_MDB_TLP_IR_MASK_MISC		0x4C4
+#define AMD_MDB_TLP_IR_ENABLE_MISC		0x4C8
+
+#define AMD_MDB_PCIE_IDRN_SHIFT			16
+
+/* Interrupt registers definitions */
+#define AMD_MDB_PCIE_INTR_CMPL_TIMEOUT		15
+#define AMD_MDB_PCIE_INTR_PM_PME_RCVD		24
+#define AMD_MDB_PCIE_INTR_PME_TO_ACK_RCVD	25
+#define AMD_MDB_PCIE_INTR_MISC_CORRECTABLE	26
+#define AMD_MDB_PCIE_INTR_NONFATAL		27
+#define AMD_MDB_PCIE_INTR_FATAL			28
+
+#define IMR(x) BIT(AMD_MDB_PCIE_INTR_ ##x)
+#define AMD_MDB_PCIE_IMR_ALL_MASK			\
+	(						\
+		IMR(CMPL_TIMEOUT)	|		\
+		IMR(PM_PME_RCVD)	|		\
+		IMR(PME_TO_ACK_RCVD)	|		\
+		IMR(MISC_CORRECTABLE)	|		\
+		IMR(NONFATAL)		|		\
+		IMR(FATAL)				\
+	)
+
+/**
+ * struct amd_mdb_pcie - PCIe port information
+ * @pci: DesignWare PCIe controller structure
+ * @mdb_base: MDB System Level Control and Status Register(SLCR) Base
+ * @intx_domain: Legacy IRQ domain pointer
+ * @mdb_domain: MDB IRQ domain pointer
+ */
+struct amd_mdb_pcie {
+	struct dw_pcie			pci;
+	void __iomem			*mdb_base;
+	struct irq_domain		*intx_domain;
+	struct irq_domain		*mdb_domain;
+};
+
+static const struct dw_pcie_host_ops amd_mdb_pcie_host_ops = {
+};
+
+static inline u32 pcie_read(struct amd_mdb_pcie *pcie, u32 reg)
+{
+	return readl_relaxed(pcie->mdb_base + reg);
+}
+
+static inline void pcie_write(struct amd_mdb_pcie *pcie,
+			      u32 val, u32 reg)
+{
+	writel_relaxed(val, pcie->mdb_base + reg);
+}
+
+static void amd_mdb_mask_intx_irq(struct irq_data *data)
+{
+	struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(data);
+	struct dw_pcie *pci = &pcie->pci;
+	struct dw_pcie_rp *port = &pci->pp;
+	unsigned long flags;
+	u32 mask, val;
+
+	mask = BIT(data->hwirq + AMD_MDB_PCIE_IDRN_SHIFT);
+	raw_spin_lock_irqsave(&port->lock, flags);
+
+	val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);
+	pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_STATUS_MISC);
+
+	raw_spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static void amd_mdb_unmask_intx_irq(struct irq_data *data)
+{
+	struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(data);
+	struct dw_pcie *pci = &pcie->pci;
+	struct dw_pcie_rp *port = &pci->pp;
+	unsigned long flags;
+	u32 mask;
+	u32 val;
+
+	mask = BIT(data->hwirq + AMD_MDB_PCIE_IDRN_SHIFT);
+	raw_spin_lock_irqsave(&port->lock, flags);
+
+	val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);
+	pcie_write(pcie, (val | mask), AMD_MDB_TLP_IR_STATUS_MISC);
+
+	raw_spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static struct irq_chip amd_mdb_intx_irq_chip = {
+	.name		= "INTx",
+	.irq_mask	= amd_mdb_mask_intx_irq,
+	.irq_unmask	= amd_mdb_unmask_intx_irq,
+};
+
+/**
+ * amd_mdb_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 amd_mdb_pcie_intx_map(struct irq_domain *domain,
+				 unsigned int irq, irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &amd_mdb_intx_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 amd_intx_domain_ops = {
+	.map = amd_mdb_pcie_intx_map,
+};
+
+/**
+ * amd_mdb_pcie_init_port - Initialize hardware
+ * @pcie: PCIe port information
+ * @pdev: platform device
+ */
+static int amd_mdb_pcie_init_port(struct amd_mdb_pcie *pcie,
+				  struct platform_device *pdev)
+{
+	int val;
+
+	/* Disable all TLP Interrupts */
+	pcie_write(pcie, pcie_read(pcie, AMD_MDB_TLP_IR_ENABLE_MISC) &
+		   ~AMD_MDB_PCIE_IMR_ALL_MASK,
+		   AMD_MDB_TLP_IR_ENABLE_MISC);
+
+	/* Clear pending TLP interrupts */
+	pcie_write(pcie, pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC) &
+		   AMD_MDB_PCIE_IMR_ALL_MASK,
+		   AMD_MDB_TLP_IR_STATUS_MISC);
+
+	/* Enable all TLP Interrupts */
+	val = pcie_read(pcie, AMD_MDB_TLP_IR_ENABLE_MISC);
+	pcie_write(pcie, (val | AMD_MDB_PCIE_IMR_ALL_MASK),
+		   AMD_MDB_TLP_IR_ENABLE_MISC);
+
+	return 0;
+}
+
+static irqreturn_t amd_mdb_pcie_event_flow(int irq, void *args)
+{
+	struct amd_mdb_pcie *pcie = args;
+	unsigned long val;
+	int i;
+
+	val =  pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);
+	val &= ~pcie_read(pcie, AMD_MDB_TLP_IR_MASK_MISC);
+	for_each_set_bit(i, &val, 32)
+		generic_handle_domain_irq(pcie->mdb_domain, i);
+	pcie_write(pcie, val, AMD_MDB_TLP_IR_STATUS_MISC);
+
+	return IRQ_HANDLED;
+}
+
+#define _IC(x, s)[AMD_MDB_PCIE_INTR_ ## x] = { __stringify(x), s }
+
+static const struct {
+	const char	*sym;
+	const char	*str;
+} intr_cause[32] = {
+	_IC(CMPL_TIMEOUT,	"completion timeout"),
+	_IC(PM_PME_RCVD,	"PM_PME message received"),
+	_IC(PME_TO_ACK_RCVD,	"PME_TO_ACK message received"),
+	_IC(MISC_CORRECTABLE,	"Correctable error message"),
+	_IC(NONFATAL,		"Non fatal error message"),
+	_IC(FATAL,		"Fatal error message"),
+};
+
+static void amd_mdb_mask_event_irq(struct irq_data *d)
+{
+	struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(d);
+	struct dw_pcie *pci = &pcie->pci;
+	struct dw_pcie_rp *port = &pci->pp;
+	u32 val;
+
+	raw_spin_lock(&port->lock);
+	val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);
+	val &= ~BIT(d->hwirq);
+	pcie_write(pcie, val, AMD_MDB_TLP_IR_STATUS_MISC);
+	raw_spin_unlock(&port->lock);
+}
+
+static void amd_mdb_unmask_event_irq(struct irq_data *d)
+{
+	struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(d);
+	struct dw_pcie *pci = &pcie->pci;
+	struct dw_pcie_rp *port = &pci->pp;
+	u32 val;
+
+	raw_spin_lock(&port->lock);
+	val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);
+	val |= BIT(d->hwirq);
+	pcie_write(pcie, val, AMD_MDB_TLP_IR_STATUS_MISC);
+	raw_spin_unlock(&port->lock);
+}
+
+static struct irq_chip amd_mdb_event_irq_chip = {
+	.name		= "RC-Event",
+	.irq_mask	= amd_mdb_mask_event_irq,
+	.irq_unmask	= amd_mdb_unmask_event_irq,
+};
+
+static int amd_mdb_pcie_event_map(struct irq_domain *domain,
+				  unsigned int irq, irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &amd_mdb_event_irq_chip,
+				 handle_level_irq);
+	irq_set_chip_data(irq, domain->host_data);
+	irq_set_status_flags(irq, IRQ_LEVEL);
+	return 0;
+}
+
+static const struct irq_domain_ops event_domain_ops = {
+	.map = amd_mdb_pcie_event_map,
+};
+
+static void amd_mdb_pcie_free_irq_domains(struct amd_mdb_pcie *pcie)
+{
+	if (pcie->intx_domain) {
+		irq_domain_remove(pcie->intx_domain);
+		pcie->intx_domain = NULL;
+	}
+
+	if (pcie->mdb_domain) {
+		irq_domain_remove(pcie->mdb_domain);
+		pcie->mdb_domain = NULL;
+	}
+}
+
+/**
+ * amd_mdb_pcie_init_irq_domains - Initialize IRQ domain
+ * @pcie: PCIe port information
+ * @pdev: platform device
+ * Return: '0' on success and error value on failure
+ */
+static int amd_mdb_pcie_init_irq_domains(struct amd_mdb_pcie *pcie,
+					 struct platform_device *pdev)
+{
+	struct dw_pcie *pci = &pcie->pci;
+	struct dw_pcie_rp *pp = &pci->pp;
+	struct device *dev = &pdev->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;
+	}
+
+	pcie->mdb_domain = irq_domain_add_linear(pcie_intc_node, 32,
+						 &event_domain_ops,
+					       pcie);
+	if (!pcie->mdb_domain)
+		goto out;
+
+	irq_domain_update_bus_token(pcie->mdb_domain, DOMAIN_BUS_NEXUS);
+
+	pcie->intx_domain = irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX,
+						  &amd_intx_domain_ops, pcie);
+	if (!pcie->intx_domain)
+		goto mdb_out;
+
+	irq_domain_update_bus_token(pcie->intx_domain, DOMAIN_BUS_WIRED);
+
+	of_node_put(pcie_intc_node);
+	raw_spin_lock_init(&pp->lock);
+
+	return 0;
+mdb_out:
+	amd_mdb_pcie_free_irq_domains(pcie);
+out:
+	of_node_put(pcie_intc_node);
+	dev_err(dev, "Failed to allocate IRQ domains\n");
+
+	return -ENOMEM;
+}
+
+static irqreturn_t amd_mdb_pcie_intr_handler(int irq, void *args)
+{
+	struct amd_mdb_pcie *pcie = args;
+	struct device *dev;
+	struct irq_data *d;
+
+	dev = pcie->pci.dev;
+
+	d = irq_domain_get_irq_data(pcie->mdb_domain, irq);
+	if (intr_cause[d->hwirq].str)
+		dev_warn(dev, "%s\n", intr_cause[d->hwirq].str);
+	else
+		dev_warn(dev, "Unknown IRQ %ld\n", d->hwirq);
+
+	return IRQ_HANDLED;
+}
+
+static int amd_mdb_setup_irq(struct amd_mdb_pcie *pcie,
+			     struct platform_device *pdev)
+{
+	struct dw_pcie *pci = &pcie->pci;
+	struct dw_pcie_rp *pp = &pci->pp;
+	struct device *dev = &pdev->dev;
+	int i, irq, err;
+
+	pp->irq = platform_get_irq(pdev, 0);
+	if (pp->irq < 0)
+		return pp->irq;
+
+	for (i = 0; i < ARRAY_SIZE(intr_cause); i++) {
+		if (!intr_cause[i].str)
+			continue;
+		irq = irq_create_mapping(pcie->mdb_domain, i);
+		if (!irq) {
+			dev_err(dev, "Failed to map mdb domain interrupt\n");
+			return -ENXIO;
+		}
+		err = devm_request_irq(dev, irq, amd_mdb_pcie_intr_handler,
+				       IRQF_SHARED | IRQF_NO_THREAD,
+				       intr_cause[i].sym, pcie);
+		if (err) {
+			dev_err(dev, "Failed to request IRQ %d\n", irq);
+			return err;
+		}
+	}
+
+	/* Plug the main event chained handler */
+	err = devm_request_irq(dev, pp->irq, amd_mdb_pcie_event_flow,
+			       IRQF_SHARED | IRQF_NO_THREAD, "pcie_irq", pcie);
+	if (err) {
+		dev_err(dev, "Failed to request event IRQ %d\n", pp->irq);
+		return err;
+	}
+
+	return 0;
+}
+
+static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie,
+				 struct platform_device *pdev)
+{
+	struct dw_pcie *pci = &pcie->pci;
+	struct dw_pcie_rp *pp = &pci->pp;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	pcie->mdb_base = devm_platform_ioremap_resource_byname(pdev, "mdb_pcie_slcr");
+	if (IS_ERR(pcie->mdb_base))
+		return PTR_ERR(pcie->mdb_base);
+
+	ret = amd_mdb_pcie_init_irq_domains(pcie, pdev);
+	if (ret)
+		return ret;
+
+	amd_mdb_pcie_init_port(pcie, pdev);
+
+	ret = amd_mdb_setup_irq(pcie, pdev);
+	if (ret) {
+		dev_err(dev, "Failed to set up interrupts\n");
+		goto out;
+	}
+
+	pp->ops = &amd_mdb_pcie_host_ops;
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(dev, "Failed to initialize host\n");
+		goto out;
+	}
+
+	return 0;
+
+out:
+	amd_mdb_pcie_free_irq_domains(pcie);
+	return ret;
+}
+
+static int amd_mdb_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct amd_mdb_pcie *pcie;
+	struct dw_pcie *pci;
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pci = &pcie->pci;
+	pci->dev = dev;
+
+	platform_set_drvdata(pdev, pcie);
+
+	return amd_mdb_add_pcie_port(pcie, pdev);
+}
+
+static const struct of_device_id amd_mdb_pcie_of_match[] = {
+	{
+		.compatible = "amd,versal2-mdb-host",
+	},
+	{},
+};
+
+static struct platform_driver amd_mdb_pcie_driver = {
+	.driver = {
+		.name	= "amd-mdb-pcie",
+		.of_match_table = amd_mdb_pcie_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe = amd_mdb_pcie_probe,
+};
+builtin_platform_driver(amd_mdb_pcie_driver);
-- 
2.34.1

Re: [RESEND PATCH v5 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver

[Bjorn Helgaas]

On Fri, Dec 13, 2024 at 12:10:35PM +0530, Thippeswamy Havalige wrote:
> Add support for AMD MDB(Multimedia DMA Bridge) IP core as Root Port.

Add space before "(".

> +config PCIE_AMD_MDB
> +	bool "AMD PCIe controller (host mode)"

Seems too generic to describe this as "the AMD PCIe controller".
Perhaps at least "AMD MDB PCIe controller"?  And/or include "Versal2"?

> +	depends on OF || COMPILE_TEST
> +	depends on PCI && PCI_MSI
> +	select PCIE_DW_HOST
> +	help
> +	  Say Y here to enable PCIe controller support on AMD SoCs. The
> +	  PCIe controller is based on DesignWare Hardware and uses AMD
> +	  hardware wrappers.

Make this help text a little more specific, too.

> + * struct amd_mdb_pcie - PCIe port information
> + * @pci: DesignWare PCIe controller structure
> + * @mdb_base: MDB System Level Control and Status Register(SLCR) Base

Add space before "(".

Thanks for expanding this initialism.  Capitalize it in the text of
other patches so it's obvious that it's an initialism, not a word.

> + * @intx_domain: Legacy IRQ domain pointer

Just say "INTx IRQ domain pointer".  No point in using two terms when
we use "INTx" everywhere else.

> + * @mdb_domain: MDB IRQ domain pointer
> + */
> +struct amd_mdb_pcie {
> +	struct dw_pcie			pci;
> +	void __iomem			*mdb_base;
> +	struct irq_domain		*intx_domain;
> +	struct irq_domain		*mdb_domain;
> +};

> + * amd_mdb_pcie_init_port - Initialize hardware
> + * @pcie: PCIe port information
> + * @pdev: platform device
> + */
> +static int amd_mdb_pcie_init_port(struct amd_mdb_pcie *pcie,
> +				  struct platform_device *pdev)

"pdev" is unused, why include it?

> +static irqreturn_t amd_mdb_pcie_event_flow(int irq, void *args)
> +{
> +	struct amd_mdb_pcie *pcie = args;
> +	unsigned long val;
> +	int i;
> +
> +	val =  pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);

Spurious extra space.

> +static int amd_mdb_pcie_init_irq_domains(struct amd_mdb_pcie *pcie,
> +					 struct platform_device *pdev)
> +{
> +	struct dw_pcie *pci = &pcie->pci;
> +	struct dw_pcie_rp *pp = &pci->pp;
> +	struct device *dev = &pdev->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;
> +	}
> +
> +	pcie->mdb_domain = irq_domain_add_linear(pcie_intc_node, 32,
> +						 &event_domain_ops,
> +					       pcie);

Fix whitespace.  "pcie" would fit on the previous line.

Bjorn

RE: [RESEND PATCH v5 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver

[Havalige, Thippeswamy]

Hi Bjorn,

Thanks for review comments, will fix all below comments in next patch.

Regards,
Thippeswamy H

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Friday, December 13, 2024 10:58 PM
> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>
> Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> manivannan.sadhasivam@linaro.org; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; jingoohan1@gmail.com; Simek, Michal
> <michal.simek@amd.com>; Gogada, Bharat Kumar
> <bharat.kumar.gogada@amd.com>
> Subject: Re: [RESEND PATCH v5 3/3] PCI: amd-mdb: Add AMD MDB Root Port
> driver
> 
> On Fri, Dec 13, 2024 at 12:10:35PM +0530, Thippeswamy Havalige wrote:
> > Add support for AMD MDB(Multimedia DMA Bridge) IP core as Root Port.
> 
> Add space before "(".
> 
> > +config PCIE_AMD_MDB
> > +	bool "AMD PCIe controller (host mode)"
> 
> Seems too generic to describe this as "the AMD PCIe controller".
> Perhaps at least "AMD MDB PCIe controller"?  And/or include "Versal2"?
> 
> > +	depends on OF || COMPILE_TEST
> > +	depends on PCI && PCI_MSI
> > +	select PCIE_DW_HOST
> > +	help
> > +	  Say Y here to enable PCIe controller support on AMD SoCs. The
> > +	  PCIe controller is based on DesignWare Hardware and uses AMD
> > +	  hardware wrappers.
> 
> Make this help text a little more specific, too.
> 
> > + * struct amd_mdb_pcie - PCIe port information
> > + * @pci: DesignWare PCIe controller structure
> > + * @mdb_base: MDB System Level Control and Status Register(SLCR) Base
> 
> Add space before "(".
> 
> Thanks for expanding this initialism.  Capitalize it in the text of other patches so it's
> obvious that it's an initialism, not a word.
> 
> > + * @intx_domain: Legacy IRQ domain pointer
> 
> Just say "INTx IRQ domain pointer".  No point in using two terms when we use
> "INTx" everywhere else.
> 
> > + * @mdb_domain: MDB IRQ domain pointer  */ struct amd_mdb_pcie {
> > +	struct dw_pcie			pci;
> > +	void __iomem			*mdb_base;
> > +	struct irq_domain		*intx_domain;
> > +	struct irq_domain		*mdb_domain;
> > +};
> 
> > + * amd_mdb_pcie_init_port - Initialize hardware
> > + * @pcie: PCIe port information
> > + * @pdev: platform device
> > + */
> > +static int amd_mdb_pcie_init_port(struct amd_mdb_pcie *pcie,
> > +				  struct platform_device *pdev)
> 
> "pdev" is unused, why include it?
> 
> > +static irqreturn_t amd_mdb_pcie_event_flow(int irq, void *args) {
> > +	struct amd_mdb_pcie *pcie = args;
> > +	unsigned long val;
> > +	int i;
> > +
> > +	val =  pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);
> 
> Spurious extra space.
> 
> > +static int amd_mdb_pcie_init_irq_domains(struct amd_mdb_pcie *pcie,
> > +					 struct platform_device *pdev)
> > +{
> > +	struct dw_pcie *pci = &pcie->pci;
> > +	struct dw_pcie_rp *pp = &pci->pp;
> > +	struct device *dev = &pdev->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;
> > +	}
> > +
> > +	pcie->mdb_domain = irq_domain_add_linear(pcie_intc_node, 32,
> > +						 &event_domain_ops,
> > +					       pcie);
> 
> Fix whitespace.  "pcie" would fit on the previous line.
> 
> Bjorn

Re: [RESEND PATCH v5 0/3] Add support for AMD MDB IP as Root Port

[Krzysztof Kozlowski]

On Fri, Dec 13, 2024 at 12:10:32PM +0530, Thippeswamy Havalige wrote:
> This series of patch add support for AMD MDB IP as Root Port.
> 
> The AMD MDB IP support's 32 bit and 64bit BAR's at Gen5 speed.
> As Root Port it supports MSI and legacy interrupts.

Why are you resending patches after 5 days? Your cover letter should
explain this.

Read submitting patches about timeframes so you won't annoy/spam
maintainers.

Best regards,
Krzysztof

RE: [RESEND PATCH v5 0/3] Add support for AMD MDB IP as Root Port

[Havalige, Thippeswamy]

Hi Krzysztof Kozlowski,

Thanks for your inputs, Sorry Will take care of this next patch.

Regards,
Thippeswamy H
> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Friday, December 13, 2024 2:26 PM
> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>
> Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> manivannan.sadhasivam@linaro.org; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; jingoohan1@gmail.com; Simek, Michal
> <michal.simek@amd.com>; Gogada, Bharat Kumar
> <bharat.kumar.gogada@amd.com>
> Subject: Re: [RESEND PATCH v5 0/3] Add support for AMD MDB IP as Root Port
> 
> On Fri, Dec 13, 2024 at 12:10:32PM +0530, Thippeswamy Havalige wrote:
> > This series of patch add support for AMD MDB IP as Root Port.
> >
> > The AMD MDB IP support's 32 bit and 64bit BAR's at Gen5 speed.
> > As Root Port it supports MSI and legacy interrupts.
> 
> Why are you resending patches after 5 days? Your cover letter should
> explain this.
> 
> Read submitting patches about timeframes so you won't annoy/spam
> maintainers.
> 
> Best regards,
> Krzysztof