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

[PATCH v7 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            |  11 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pcie-amd-mdb.c     | 436 ++++++++++++++++++
 5 files changed, 571 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

[PATCH v7 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>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
Changes in v3:
-------------
- Introduced below changes in dwc yaml schema.
Changes in v5:
-------------
- Modify mdb_pcie_slcr as constant.
Changes in v6:
-------------
-Modify slcr 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 205326fb2d75..fdecfe6ad5f1 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: slcr
     allOf:
       - contains:
           const: dbi
-- 
2.34.1

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

[Manivannan Sadhasivam]

On Mon, Jan 20, 2025 at 04:13:03AM +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>

Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> Changes in v3:
> -------------
> - Introduced below changes in dwc yaml schema.
> Changes in v5:
> -------------
> - Modify mdb_pcie_slcr as constant.
> Changes in v6:
> -------------
> -Modify slcr 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 205326fb2d75..fdecfe6ad5f1 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: slcr
>      allOf:
>        - contains:
>            const: dbi
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH v7 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>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
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.
Changes in v6:
--------------
- Reduce dbi size to 4k.
- update register name to slcr.
---
 .../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..db751a51e63c
--- /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 System Level Control and Status Register (SLCR) Base
+      - description: configuration region
+      - description: data bus interface
+      - description: address translation unit register
+
+  reg-names:
+    items:
+      - const: 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 0x1000>,
+                  <0x0 0xed860000 0x0 0x2000>;
+            reg-names = "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: [PATCH v7 2/3] dt-bindings: PCI: amd-mdb: Add AMD Versal2 MDB PCIe Root Port Bridge

[Manivannan Sadhasivam]

On Mon, Jan 20, 2025 at 04:13:04AM +0530, Thippeswamy Havalige wrote:
> Add AMD Versal2 MDB (Multimedia DMA Bridge) PCIe Root Port Bridge.
> 
> Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>

Couple of comments below. With those fixed,

Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> 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.
> Changes in v6:
> --------------
> - Reduce dbi size to 4k.
> - update register name to slcr.
> ---
>  .../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..db751a51e63c
> --- /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 System Level Control and Status Register (SLCR) Base
> +      - description: configuration region
> +      - description: data bus interface
> +      - description: address translation unit register
> +
> +  reg-names:
> +    items:
> +      - const: 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 0x1000>,
> +                  <0x0 0xed860000 0x0 0x2000>;
> +            reg-names = "slcr", "config", "dbi", "atu";
> +            ranges = <0x2000000 0x00 0xa8000000 0x00 0xa8000000 0x00 0x10000000>,

I/O PCI address starts from 0.

Also use 0x0 instead of 0x00 for consistency.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

[PATCH v7 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.
Changes in v6:
--------------
- Remove pdev automatic variable.
- Update register name to slcr.
- Fix whitespace.
- remove Spurious extra line.
- Update Legacy to INTx.
- Add space before (SLCR).
- Update menuconfig description.
---
 drivers/pci/controller/dwc/Kconfig        |  11 +
 drivers/pci/controller/dwc/Makefile       |   1 +
 drivers/pci/controller/dwc/pcie-amd-mdb.c | 436 ++++++++++++++++++++++
 3 files changed, 448 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..61d119646749 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -27,6 +27,17 @@ 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 MDB Versal2 PCIe Host controller"
+	depends on OF || COMPILE_TEST
+	depends on PCI && PCI_MSI
+	select PCIE_DW_HOST
+	help
+	  Say Y here if you want to enable PCIe controller support on AMD
+	  Versal2 SoCs. The AMD MDB Versal2 PCIe controller is based on DesignWare
+	  IP and therefore the driver re-uses the Designware core functions to
+	  implement the driver.
+
 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..ef1ac757d41c
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
@@ -0,0 +1,436 @@
+// 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
+ * @slcr: MDB System Level Control and Status Register (SLCR) Base
+ * @intx_domain: INTx IRQ domain pointer
+ * @mdb_domain: MDB IRQ domain pointer
+ */
+struct amd_mdb_pcie {
+	struct dw_pcie			pci;
+	void __iomem			*slcr;
+	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->slcr + reg);
+}
+
+static inline void pcie_write(struct amd_mdb_pcie *pcie,
+			      u32 val, u32 reg)
+{
+	writel_relaxed(val, pcie->slcr + 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
+ */
+static int amd_mdb_pcie_init_port(struct amd_mdb_pcie *pcie)
+{
+	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->slcr = devm_platform_ioremap_resource_byname(pdev, "slcr");
+	if (IS_ERR(pcie->slcr))
+		return PTR_ERR(pcie->slcr);
+
+	ret = amd_mdb_pcie_init_irq_domains(pcie, pdev);
+	if (ret)
+		return ret;
+
+	amd_mdb_pcie_init_port(pcie);
+
+	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: [PATCH v7 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver

[Manivannan Sadhasivam]

On Mon, Jan 20, 2025 at 04:13:05AM +0530, Thippeswamy Havalige wrote:
> 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.
> Changes in v6:
> --------------
> - Remove pdev automatic variable.
> - Update register name to slcr.
> - Fix whitespace.
> - remove Spurious extra line.
> - Update Legacy to INTx.
> - Add space before (SLCR).
> - Update menuconfig description.
> ---
>  drivers/pci/controller/dwc/Kconfig        |  11 +
>  drivers/pci/controller/dwc/Makefile       |   1 +
>  drivers/pci/controller/dwc/pcie-amd-mdb.c | 436 ++++++++++++++++++++++
>  3 files changed, 448 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..61d119646749 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -27,6 +27,17 @@ 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 MDB Versal2 PCIe Host controller"
> +	depends on OF || COMPILE_TEST
> +	depends on PCI && PCI_MSI
> +	select PCIE_DW_HOST
> +	help
> +	  Say Y here if you want to enable PCIe controller support on AMD
> +	  Versal2 SoCs. The AMD MDB Versal2 PCIe controller is based on DesignWare
> +	  IP and therefore the driver re-uses the Designware core functions to
> +	  implement the driver.
> +
>  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..ef1ac757d41c
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> @@ -0,0 +1,436 @@
> +// 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
> + * @slcr: MDB System Level Control and Status Register (SLCR) Base
> + * @intx_domain: INTx IRQ domain pointer
> + * @mdb_domain: MDB IRQ domain pointer
> + */
> +struct amd_mdb_pcie {
> +	struct dw_pcie			pci;
> +	void __iomem			*slcr;
> +	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)

No need of 'inline' keyword in .c files. Compiler will inline the function on
its own when required.

But I'd prefer to use the {readl/writel}_relaxed directly instead of a wrapper.

> +{
> +	return readl_relaxed(pcie->slcr + reg);
> +}
> +
> +static inline void pcie_write(struct amd_mdb_pcie *pcie,
> +			      u32 val, u32 reg)
> +{
> +	writel_relaxed(val, pcie->slcr + 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);
> +

Remove the newline here and add it before raw_spin_lock_irqsave().

> +	val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);
> +	pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_STATUS_MISC);
> +

Remove the newline here.

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

Same as above.

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

Add the 'AMD MDB' prefix.

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

This kernel doc doesn't add any value. So you can drop it.

> +static int amd_mdb_pcie_init_port(struct amd_mdb_pcie *pcie)
> +{
> +	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);

Don't you need port lock here? What if the IRQ handler gets executed in a
different CPU?

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

Clear the IRQs before handling them.

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

Why this lock is non-irq variant compared to amd_mdb_mask_intx_irq()?

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

Same here.

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

Add the 'AMD MDB' prefix.

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

newline here.

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

-ENODATA

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

Move this before irq_domain_update_bus_token().

> +	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");

It is not a good practice to print error logs and return a fixed error no in
goto labels. If one more condition gets added to this function later, these all
should be moved to the respective error check. So please do so now itself even
if it adds a bit of redundancy.

The error log could be improved by stating which domain actually failed.

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

Move this to dev_dbg().

> +	else
> +		dev_warn(dev, "Unknown IRQ %ld\n", d->hwirq);

dev_WARN_ONCE() to avoid spamming the log.

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

-ENOMEM

> +		}
> +		err = devm_request_irq(dev, irq, amd_mdb_pcie_intr_handler,
> +				       IRQF_SHARED | IRQF_NO_THREAD,
> +				       intr_cause[i].sym, pcie);

IRQ name should have the 'amd_mdb' prefix and also the domain number to uniquely
identify the interrupt belonging to each host bridge.

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

Same as above.

> +	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->slcr = devm_platform_ioremap_resource_byname(pdev, "slcr");
> +	if (IS_ERR(pcie->slcr))
> +		return PTR_ERR(pcie->slcr);
> +
> +	ret = amd_mdb_pcie_init_irq_domains(pcie, pdev);
> +	if (ret)
> +		return ret;
> +
> +	amd_mdb_pcie_init_port(pcie);
> +
> +	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,

Consider using PROBE_PREFER_ASYNCHRONOUS to speed up the probe.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Havalige, Thippeswamy]

Hi Sadhasivam,

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: Tuesday, January 21, 2025 10:18 AM
> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>
> Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com; 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: [PATCH v7 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
> 
> On Mon, Jan 20, 2025 at 04:13:05AM +0530, Thippeswamy Havalige wrote:
> > 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.
> > Changes in v6:
> > --------------
> > - Remove pdev automatic variable.
> > - Update register name to slcr.
> > - Fix whitespace.
> > - remove Spurious extra line.
> > - Update Legacy to INTx.
> > - Add space before (SLCR).
> > - Update menuconfig description.
> > ---
> >  drivers/pci/controller/dwc/Kconfig        |  11 +
> >  drivers/pci/controller/dwc/Makefile       |   1 +
> >  drivers/pci/controller/dwc/pcie-amd-mdb.c | 436
> > ++++++++++++++++++++++
> >  3 files changed, 448 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..61d119646749 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -27,6 +27,17 @@ 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 MDB Versal2 PCIe Host controller"
> > +	depends on OF || COMPILE_TEST
> > +	depends on PCI && PCI_MSI
> > +	select PCIE_DW_HOST
> > +	help
> > +	  Say Y here if you want to enable PCIe controller support on AMD
> > +	  Versal2 SoCs. The AMD MDB Versal2 PCIe controller is based on
> DesignWare
> > +	  IP and therefore the driver re-uses the Designware core functions to
> > +	  implement the driver.
> > +
> >  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..ef1ac757d41c
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> > @@ -0,0 +1,436 @@
> > +// 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
> > + * @slcr: MDB System Level Control and Status Register (SLCR) Base
> > + * @intx_domain: INTx IRQ domain pointer
> > + * @mdb_domain: MDB IRQ domain pointer  */ struct amd_mdb_pcie {
> > +	struct dw_pcie			pci;
> > +	void __iomem			*slcr;
> > +	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)
> 
> No need of 'inline' keyword in .c files. Compiler will inline the function on its own
> when required.
> 
> But I'd prefer to use the {readl/writel}_relaxed directly instead of a wrapper.
- Thank you for reviewing, Will remove inline keyword & like to keep wrapper function.
> 
> > +{
> > +	return readl_relaxed(pcie->slcr + reg); }
> > +
> > +static inline void pcie_write(struct amd_mdb_pcie *pcie,
> > +			      u32 val, u32 reg)
> > +{
> > +	writel_relaxed(val, pcie->slcr + 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);
> > +
> 
> Remove the newline here and add it before raw_spin_lock_irqsave().
- Thank you for review comments, Will remove this piece of call back function.
> 
> > +	val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);
> > +	pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_STATUS_MISC);
> > +
> 
> Remove the newline here.
- Thank you for review comments, Will remove this piece of call back function.
> 
> > +	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);
> > +
> 
> Same as above.
> 
> > +	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",
> 
> Add the 'AMD MDB' prefix.
- Thank you for review comments, Will remove this piece of call back function.
> 
> > +	.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
> > + */
> 
> This kernel doc doesn't add any value. So you can drop it.
- Thank you for review comments, Will remove this kernel doc in next patch.
> 
> > +static int amd_mdb_pcie_init_port(struct amd_mdb_pcie *pcie) {
> > +	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);
> 
> Don't you need port lock here? What if the IRQ handler gets executed in a different
> CPU?
- Thank you for reviewing. Since we are using a single IRQ line, the IRQ line remains in a disabled (masked) state until
the interrupts are handled. Therefore, there is no need for a locking mechanism in this case. 
> > +	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);
> 
> Clear the IRQs before handling them.
- Thank you for reviewing, This interrupt handling patch is authored by Marc zingier. 
This is intentionally done this way so that IRQ's are masked before they are handled.
> 
> > +
> > +	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);
> 
> Why this lock is non-irq variant compared to amd_mdb_mask_intx_irq()?
> 
> > +	val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);
- Here I need to mask the interrupts before handling. 
Refer https://gitenterprise.xilinx.com/Linux/linux-xlnx/blob/xlnx_rebase_v6.1-next/drivers/pci/controller/pcie-xilinx-cpm.c#L255
> > +	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);
> 
> Same here.
> 
> > +	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",
> 
> Add the 'AMD MDB' prefix.
- Thank you for reviewing, will add this in next patch.
> 
> > +	.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);
> 
> newline here.
- Thank you for reviewing, will update this in next patch.
> 
> > +	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;
> 
> -ENODATA
> 
> > +	}
> > +
> > +	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);
> 
> Move this before irq_domain_update_bus_token().
> 
> > +	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");
> 
> It is not a good practice to print error logs and return a fixed error no in goto labels. If
> one more condition gets added to this function later, these all should be moved to
> the respective error check. So please do so now itself even if it adds a bit of
> redundancy.
> 
> The error log could be improved by stating which domain failed.
Thank you for reviewing, Will update this in next patch.
> 
> > +
> > +	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);
> 
> Move this to dev_dbg().
- Thank you for the feedback. This message is intended as a warning for the end user to indicate that an event has occurred,
and not just for debugging purposes. Hence, we believe dev_warn() is the appropriate log level for this scenario. 
Please let us know if you have further concerns.
> > +	else
> > +		dev_warn(dev, "Unknown IRQ %ld\n", d->hwirq);
> 
> dev_WARN_ONCE() to avoid spamming the log.
- Thank you reviewing, I ll change it in next patch.
> 
> > +
> > +	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;
> 
> -ENOMEM
> 
> > +		}
> > +		err = devm_request_irq(dev, irq, amd_mdb_pcie_intr_handler,
> > +				       IRQF_SHARED | IRQF_NO_THREAD,
> > +				       intr_cause[i].sym, pcie);
> 
> IRQ name should have the 'amd_mdb' prefix and also the domain number to
> uniquely identify the interrupt belonging to each host bridge.
- Thank you for the feedback. We will update the IRQ name to include the amd_mdb
prefix in the next patch. However, including the domain number may not be necessary
in this case since both controllers are not expected to be present simultaneously.  
> > +		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);
> 
> Same as above.
- Thank you for feedback, will update in next patch.
> 
> > +	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->slcr = devm_platform_ioremap_resource_byname(pdev, "slcr");
> > +	if (IS_ERR(pcie->slcr))
> > +		return PTR_ERR(pcie->slcr);
> > +
> > +	ret = amd_mdb_pcie_init_irq_domains(pcie, pdev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	amd_mdb_pcie_init_port(pcie);
> > +
> > +	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,
> 
> Consider using PROBE_PREFER_ASYNCHRONOUS to speed up the probe.

- Thank you for the suggestion. However, in future probe function involves handling the PCIe PERST# signal, 
which may require synchronous execution to ensure proper initialization of the device and its dependencies.
> 
> - Mani
> 
> --
> மணிவண்ணன் சதாசிவம்

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

[Havalige, Thippeswamy]

Hi Sadhasivam,

> -----Original Message-----
> From: Havalige, Thippeswamy
> Sent: Tuesday, January 28, 2025 2:52 PM
> To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com; 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: [PATCH v7 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
> 
> Hi Sadhasivam,
> 
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: Tuesday, January 21, 2025 10:18 AM
> > To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>
> > Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> 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: [PATCH v7 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
> >
> > On Mon, Jan 20, 2025 at 04:13:05AM +0530, Thippeswamy Havalige wrote:
> > > 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.
> > > Changes in v6:
> > > --------------
> > > - Remove pdev automatic variable.
> > > - Update register name to slcr.
> > > - Fix whitespace.
> > > - remove Spurious extra line.
> > > - Update Legacy to INTx.
> > > - Add space before (SLCR).
> > > - Update menuconfig description.
> > > ---
> > >  drivers/pci/controller/dwc/Kconfig        |  11 +
> > >  drivers/pci/controller/dwc/Makefile       |   1 +
> > >  drivers/pci/controller/dwc/pcie-amd-mdb.c | 436
> > > ++++++++++++++++++++++
> > >  3 files changed, 448 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..61d119646749 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -27,6 +27,17 @@ 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 MDB Versal2 PCIe Host controller"
> > > +	depends on OF || COMPILE_TEST
> > > +	depends on PCI && PCI_MSI
> > > +	select PCIE_DW_HOST
> > > +	help
> > > +	  Say Y here if you want to enable PCIe controller support on AMD
> > > +	  Versal2 SoCs. The AMD MDB Versal2 PCIe controller is based on
> > DesignWare
> > > +	  IP and therefore the driver re-uses the Designware core functions to
> > > +	  implement the driver.
> > > +
> > >  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..ef1ac757d41c
> > > --- /dev/null
> > > +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> > > @@ -0,0 +1,436 @@
> > > +// 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
> > > + * @slcr: MDB System Level Control and Status Register (SLCR) Base
> > > + * @intx_domain: INTx IRQ domain pointer
> > > + * @mdb_domain: MDB IRQ domain pointer  */ struct amd_mdb_pcie {
> > > +	struct dw_pcie			pci;
> > > +	void __iomem			*slcr;
> > > +	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)
> >
> > No need of 'inline' keyword in .c files. Compiler will inline the function on its own
> > when required.
> >
> > But I'd prefer to use the {readl/writel}_relaxed directly instead of a wrapper.
> - Thank you for reviewing, Will remove inline keyword & like to keep wrapper
> function.
> >
> > > +{
> > > +	return readl_relaxed(pcie->slcr + reg); }
> > > +
> > > +static inline void pcie_write(struct amd_mdb_pcie *pcie,
> > > +			      u32 val, u32 reg)
> > > +{
> > > +	writel_relaxed(val, pcie->slcr + 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);
> > > +
> >
> > Remove the newline here and add it before raw_spin_lock_irqsave().
> - Thank you for review comments, Will remove this piece of call back function.
- Will take your review comments and keep this callback function.
> >
> > > +	val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);
> > > +	pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_STATUS_MISC);
> > > +
> >
> > Remove the newline here.
> - Thank you for review comments, Will remove this piece of call back function.
- Will take your review comments and keep this callback function.
> >
> > > +	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);
> > > +
> >
> > Same as above.
> >
> > > +	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",
> >
> > Add the 'AMD MDB' prefix.
> - Thank you for review comments, Will remove this piece of call back function.
> >
> > > +	.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
> > > + */
> >
> > This kernel doc doesn't add any value. So you can drop it.
> - Thank you for review comments, Will remove this kernel doc in next patch.
> >
> > > +static int amd_mdb_pcie_init_port(struct amd_mdb_pcie *pcie) {
> > > +	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);
> >
> > Don't you need port lock here? What if the IRQ handler gets executed in a different
> > CPU?
> - Thank you for reviewing. Since we are using a single IRQ line, the IRQ line
> remains in a disabled (masked) state until
> the interrupts are handled. Therefore, there is no need for a locking mechanism in
> this case.
> > > +	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);
> >
> > Clear the IRQs before handling them.
> - Thank you for reviewing, This interrupt handling patch is authored by Marc zingier.
> This is intentionally done this way so that IRQ's are masked before they are handled.
> >
> > > +
> > > +	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);
> >
> > Why this lock is non-irq variant compared to amd_mdb_mask_intx_irq()?
> >
> > > +	val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);
> - Here I need to mask the interrupts before handling.
> Refer https://gitenterprise.xilinx.com/Linux/linux-xlnx/blob/xlnx_rebase_v6.1-
> next/drivers/pci/controller/pcie-xilinx-cpm.c#L255
> > > +	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);
> >
> > Same here.
> >
> > > +	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",
> >
> > Add the 'AMD MDB' prefix.
> - Thank you for reviewing, will add this in next patch.
> >
> > > +	.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);
> >
> > newline here.
> - Thank you for reviewing, will update this in next patch.
> >
> > > +	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;
> >
> > -ENODATA
> >
> > > +	}
> > > +
> > > +	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);
> >
> > Move this before irq_domain_update_bus_token().
> >
> > > +	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");
> >
> > It is not a good practice to print error logs and return a fixed error no in goto labels.
> If
> > one more condition gets added to this function later, these all should be moved to
> > the respective error check. So please do so now itself even if it adds a bit of
> > redundancy.
> >
> > The error log could be improved by stating which domain failed.
> Thank you for reviewing, Will update this in next patch.
> >
> > > +
> > > +	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);
> >
> > Move this to dev_dbg().
> - Thank you for the feedback. This message is intended as a warning for the end
> user to indicate that an event has occurred,
> and not just for debugging purposes. Hence, we believe dev_warn() is the
> appropriate log level for this scenario.
> Please let us know if you have further concerns.
> > > +	else
> > > +		dev_warn(dev, "Unknown IRQ %ld\n", d->hwirq);
> >
> > dev_WARN_ONCE() to avoid spamming the log.
> - Thank you reviewing, I ll change it in next patch.
> >
> > > +
> > > +	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;
> >
> > -ENOMEM
> >
> > > +		}
> > > +		err = devm_request_irq(dev, irq, amd_mdb_pcie_intr_handler,
> > > +				       IRQF_SHARED | IRQF_NO_THREAD,
> > > +				       intr_cause[i].sym, pcie);
> >
> > IRQ name should have the 'amd_mdb' prefix and also the domain number to
> > uniquely identify the interrupt belonging to each host bridge.
> - Thank you for the feedback. We will update the IRQ name to include the amd_mdb
> prefix in the next patch. However, including the domain number may not be
> necessary
> in this case since both controllers are not expected to be present simultaneously.
> > > +		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);
> >
> > Same as above.
> - Thank you for feedback, will update in next patch.
> >
> > > +	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->slcr = devm_platform_ioremap_resource_byname(pdev, "slcr");
> > > +	if (IS_ERR(pcie->slcr))
> > > +		return PTR_ERR(pcie->slcr);
> > > +
> > > +	ret = amd_mdb_pcie_init_irq_domains(pcie, pdev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	amd_mdb_pcie_init_port(pcie);
> > > +
> > > +	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,
> >
> > Consider using PROBE_PREFER_ASYNCHRONOUS to speed up the probe.
> 
> - Thank you for the suggestion. However, in future probe function involves handling
> the PCIe PERST# signal,
> which may require synchronous execution to ensure proper initialization of the
> device and its dependencies.
> >
> > - Mani
> >
> > --
> > மணிவண்ணன் சதாசிவம்