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

[PATCH v8 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     | 476 ++++++++++++++++++
 5 files changed, 611 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.44.1

[PATCH v8 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>
Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.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.44.1

[PATCH v8 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>
Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 .../bindings/pci/amd,versal2-mdb-host.yaml    | 121 ++++++++++++++++++
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.
Changes in v7:
--------------
- Add reviewed-by
---

 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..43dc2585c237
--- /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 0xa0000000 0x00 0xa0000000 0x00 0x10000000>,
+                     <0x43000000 0x1100 0x00 0x1100 0x00 0x00 0x1000000>;
+            interrupts = <GIC_SPI 165 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.44.1

[PATCH v8 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.
Changes in v7:
--------------
- None.
Changes in v8:
--------------
- Remove inline keyword.
- Fix indentations.
- Add AMD MDB prefix to interrupt names.
- Remove Kernel doc.
- Fix return types.
- Modify dev_warn to dev_warn_once.
- Add Intx handler & callbacks.
---
 drivers/pci/controller/dwc/Kconfig        |  11 +
 drivers/pci/controller/dwc/Makefile       |   1 +
 drivers/pci/controller/dwc/pcie-amd-mdb.c | 476 ++++++++++++++++++++++
 3 files changed, 488 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..94b83fa649ae
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
@@ -0,0 +1,476 @@
+// 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_INTA_ASSERT		16
+#define AMD_MDB_PCIE_INTR_INTB_ASSERT		18
+#define AMD_MDB_PCIE_INTR_INTC_ASSERT		20
+#define AMD_MDB_PCIE_INTR_INTD_ASSERT		22
+#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(INTA_ASSERT)	|		\
+		IMR(INTB_ASSERT)	|		\
+		IMR(INTC_ASSERT)	|		\
+		IMR(INTD_ASSERT)	|		\
+		IMR(PM_PME_RCVD)	|		\
+		IMR(PME_TO_ACK_RCVD)	|		\
+		IMR(MISC_CORRECTABLE)	|		\
+		IMR(NONFATAL)		|		\
+		IMR(FATAL)				\
+	)
+
+#define AMD_MDB_TLP_PCIE_INTX_MASK	GENMASK(23, 16)
+
+/**
+ * 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;
+	int				intx_irq;
+};
+
+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_MASK_MISC);
+	pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_ENABLE_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_MASK_MISC);
+	pcie_write(pcie, (val | mask), AMD_MDB_TLP_IR_ENABLE_MISC);
+	raw_spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static struct irq_chip amd_mdb_intx_irq_chip = {
+	.name		= "AMD MDB 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,
+};
+
+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		= "AMD MDB 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 -ENODATA;
+	}
+
+	pcie->mdb_domain = irq_domain_add_linear(pcie_intc_node, 32,
+						 &event_domain_ops, pcie);
+	if (!pcie->mdb_domain) {
+		dev_err(dev, "Failed to add mdb_domain\n");
+		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) {
+		dev_err(dev, "Failed to add intx_domain\n");
+		goto mdb_out;
+	}
+
+	of_node_put(pcie_intc_node);
+	irq_domain_update_bus_token(pcie->intx_domain, DOMAIN_BUS_WIRED);
+
+	raw_spin_lock_init(&pp->lock);
+
+	return 0;
+mdb_out:
+	amd_mdb_pcie_free_irq_domains(pcie);
+out:
+	of_node_put(pcie_intc_node);
+
+	return -ENOMEM;
+}
+
+static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args)
+{
+	struct amd_mdb_pcie *pcie = args;
+	unsigned long val;
+	int i;
+
+	val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
+			pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
+
+	for_each_set_bit(i, &val, 4)
+		generic_handle_domain_irq(pcie->intx_domain, i);
+
+	return IRQ_HANDLED;
+}
+
+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_once(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 -ENOMEM;
+		}
+		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;
+		}
+	}
+
+	pcie->intx_irq = irq_create_mapping(pcie->mdb_domain,
+					    AMD_MDB_PCIE_INTR_INTA_ASSERT);
+	if (!pcie->intx_irq) {
+		dev_err(dev, "Failed to map INTx interrupt\n");
+		return -ENXIO;
+	}
+
+	/* Plug the INTx handler */
+	err = devm_request_irq(dev, pcie->intx_irq,
+			       dw_pcie_rp_intx_flow,
+			       IRQF_SHARED | IRQF_NO_THREAD, NULL, pcie);
+	if (err) {
+		dev_err(dev, "Failed to request INTx IRQ %d\n", irq);
+		return err;
+	}
+
+	/* Plug the main event handler */
+	err = devm_request_irq(dev, pp->irq, amd_mdb_pcie_event_flow,
+			       IRQF_SHARED | IRQF_NO_THREAD, "amd_mdb 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.44.1

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

[Havalige, Thippeswamy]

Hi mani,

Please is there update on this patch.

Regards,
Thippeswamy H

> -----Original Message-----
> From: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
> Sent: Wednesday, January 29, 2025 5:00 PM
> To: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> manivannan.sadhasivam@linaro.org; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org
> Cc: 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>; Havalige, Thippeswamy
> <thippeswamy.havalige@amd.com>
> Subject: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
> 
> 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.
> Changes in v7:
> --------------
> - None.
> Changes in v8:
> --------------
> - Remove inline keyword.
> - Fix indentations.
> - Add AMD MDB prefix to interrupt names.
> - Remove Kernel doc.
> - Fix return types.
> - Modify dev_warn to dev_warn_once.
> - Add Intx handler & callbacks.
> ---
>  drivers/pci/controller/dwc/Kconfig        |  11 +
>  drivers/pci/controller/dwc/Makefile       |   1 +
>  drivers/pci/controller/dwc/pcie-amd-mdb.c | 476 ++++++++++++++++++++++
>  3 files changed, 488 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..94b83fa649ae
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> @@ -0,0 +1,476 @@
> +// 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_INTA_ASSERT		16
> +#define AMD_MDB_PCIE_INTR_INTB_ASSERT		18
> +#define AMD_MDB_PCIE_INTR_INTC_ASSERT		20
> +#define AMD_MDB_PCIE_INTR_INTD_ASSERT		22
> +#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(INTA_ASSERT)	|		\
> +		IMR(INTB_ASSERT)	|		\
> +		IMR(INTC_ASSERT)	|		\
> +		IMR(INTD_ASSERT)	|		\
> +		IMR(PM_PME_RCVD)	|		\
> +		IMR(PME_TO_ACK_RCVD)	|		\
> +		IMR(MISC_CORRECTABLE)	|		\
> +		IMR(NONFATAL)		|		\
> +		IMR(FATAL)				\
> +	)
> +
> +#define AMD_MDB_TLP_PCIE_INTX_MASK	GENMASK(23, 16)
> +
> +/**
> + * 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;
> +	int				intx_irq;
> +};
> +
> +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_MASK_MISC);
> +	pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_ENABLE_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_MASK_MISC);
> +	pcie_write(pcie, (val | mask), AMD_MDB_TLP_IR_ENABLE_MISC);
> +	raw_spin_unlock_irqrestore(&port->lock, flags);
> +}
> +
> +static struct irq_chip amd_mdb_intx_irq_chip = {
> +	.name		= "AMD MDB 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,
> +};
> +
> +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		= "AMD MDB 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 -ENODATA;
> +	}
> +
> +	pcie->mdb_domain = irq_domain_add_linear(pcie_intc_node, 32,
> +						 &event_domain_ops, pcie);
> +	if (!pcie->mdb_domain) {
> +		dev_err(dev, "Failed to add mdb_domain\n");
> +		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) {
> +		dev_err(dev, "Failed to add intx_domain\n");
> +		goto mdb_out;
> +	}
> +
> +	of_node_put(pcie_intc_node);
> +	irq_domain_update_bus_token(pcie->intx_domain, DOMAIN_BUS_WIRED);
> +
> +	raw_spin_lock_init(&pp->lock);
> +
> +	return 0;
> +mdb_out:
> +	amd_mdb_pcie_free_irq_domains(pcie);
> +out:
> +	of_node_put(pcie_intc_node);
> +
> +	return -ENOMEM;
> +}
> +
> +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args)
> +{
> +	struct amd_mdb_pcie *pcie = args;
> +	unsigned long val;
> +	int i;
> +
> +	val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> +			pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> +
> +	for_each_set_bit(i, &val, 4)
> +		generic_handle_domain_irq(pcie->intx_domain, i);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +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_once(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 -ENOMEM;
> +		}
> +		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;
> +		}
> +	}
> +
> +	pcie->intx_irq = irq_create_mapping(pcie->mdb_domain,
> +					    AMD_MDB_PCIE_INTR_INTA_ASSERT);
> +	if (!pcie->intx_irq) {
> +		dev_err(dev, "Failed to map INTx interrupt\n");
> +		return -ENXIO;
> +	}
> +
> +	/* Plug the INTx handler */
> +	err = devm_request_irq(dev, pcie->intx_irq,
> +			       dw_pcie_rp_intx_flow,
> +			       IRQF_SHARED | IRQF_NO_THREAD, NULL, pcie);
> +	if (err) {
> +		dev_err(dev, "Failed to request INTx IRQ %d\n", irq);
> +		return err;
> +	}
> +
> +	/* Plug the main event handler */
> +	err = devm_request_irq(dev, pp->irq, amd_mdb_pcie_event_flow,
> +			       IRQF_SHARED | IRQF_NO_THREAD, "amd_mdb
> 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.44.1

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

[Manivannan Sadhasivam]

On Wed, Jan 29, 2025 at 05:00:29PM +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.
> Changes in v7:
> --------------
> - None.
> Changes in v8:
> --------------
> - Remove inline keyword.
> - Fix indentations.
> - Add AMD MDB prefix to interrupt names.
> - Remove Kernel doc.
> - Fix return types.
> - Modify dev_warn to dev_warn_once.
> - Add Intx handler & callbacks.
> ---
>  drivers/pci/controller/dwc/Kconfig        |  11 +
>  drivers/pci/controller/dwc/Makefile       |   1 +
>  drivers/pci/controller/dwc/pcie-amd-mdb.c | 476 ++++++++++++++++++++++
>  3 files changed, 488 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..94b83fa649ae
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> @@ -0,0 +1,476 @@
> +// 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_INTA_ASSERT		16
> +#define AMD_MDB_PCIE_INTR_INTB_ASSERT		18
> +#define AMD_MDB_PCIE_INTR_INTC_ASSERT		20
> +#define AMD_MDB_PCIE_INTR_INTD_ASSERT		22
> +#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(INTA_ASSERT)	|		\
> +		IMR(INTB_ASSERT)	|		\
> +		IMR(INTC_ASSERT)	|		\
> +		IMR(INTD_ASSERT)	|		\
> +		IMR(PM_PME_RCVD)	|		\
> +		IMR(PME_TO_ACK_RCVD)	|		\
> +		IMR(MISC_CORRECTABLE)	|		\
> +		IMR(NONFATAL)		|		\
> +		IMR(FATAL)				\
> +	)
> +
> +#define AMD_MDB_TLP_PCIE_INTX_MASK	GENMASK(23, 16)
> +
> +/**
> + * 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

intx_irq is not defined.

> + */
> +struct amd_mdb_pcie {
> +	struct dw_pcie			pci;
> +	void __iomem			*slcr;
> +	struct irq_domain		*intx_domain;
> +	struct irq_domain		*mdb_domain;
> +	int				intx_irq;
> +};
> +
> +static const struct dw_pcie_host_ops amd_mdb_pcie_host_ops = {
> +};
> +
> +static inline u32 pcie_read(struct amd_mdb_pcie *pcie, u32 reg)

'inline' keyword is not removed.

Also, why are you insisting of keeping this wrapper? It literally adds 0 value.

> +{
> +	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_MASK_MISC);
> +	pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_ENABLE_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_MASK_MISC);
> +	pcie_write(pcie, (val | mask), AMD_MDB_TLP_IR_ENABLE_MISC);
> +	raw_spin_unlock_irqrestore(&port->lock, flags);
> +}
> +
> +static struct irq_chip amd_mdb_intx_irq_chip = {
> +	.name		= "AMD MDB 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

Make use of 80 column width.

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

Why can't you just do,

	pcie_write(pcie, 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);

Again, it is a good practice to clear the interrupts before processing them. If
there are any technical reason to not do so, please explain. Your reply in the
previous version didn't add much value.

> +
> +	return IRQ_HANDLED;
> +}
> +

[...]

> +static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie,

add_pcie_port() is not logically correct as you are not adding any port (root
port). Maybe reword it to amd_mdb_pcie_init()?

- Mani

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

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

[Bjorn Helgaas]

On Wed, Jan 29, 2025 at 05:00:29PM +0530, Thippeswamy Havalige wrote:
> Add support for AMD MDB (Multimedia DMA Bridge) IP core as Root Port.

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

Remove this _SHIFT #define and use something like this instead:

  #define AMD_MDB_PCIE_INTX_BIT(x) FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK, BIT(x))

I don't know what exactly the right name for that is; it looks like
maybe these bits apply to all the above registers
(AMD_MDB_TLP_IR_STATUS_MISC, AMD_MDB_TLP_IR_MASK_MISC,
AMD_MDB_TLP_IR_ENABLE_MISC)

> +#define AMD_MDB_PCIE_INTR_INTA_ASSERT		16
> +#define AMD_MDB_PCIE_INTR_INTB_ASSERT		18
> +#define AMD_MDB_PCIE_INTR_INTC_ASSERT		20
> +#define AMD_MDB_PCIE_INTR_INTD_ASSERT		22

It's kind of weird that these skip the odd-numbered bits, since
dw_pcie_rp_intx_flow(), amd_mdb_mask_intx_irq(),
amd_mdb_unmask_intx_irq() only use bits 19:16.  Something seems wrong
and needs either a fix or a comment about why this is the way it is.

> +#define IMR(x) BIT(AMD_MDB_PCIE_INTR_ ##x)
> +#define AMD_MDB_PCIE_IMR_ALL_MASK			\
> +	(						\
> +		IMR(CMPL_TIMEOUT)	|		\
> +		IMR(INTA_ASSERT)	|		\
> +		IMR(INTB_ASSERT)	|		\
> +		IMR(INTC_ASSERT)	|		\
> +		IMR(INTD_ASSERT)	|		\
> +		IMR(PM_PME_RCVD)	|		\
> +		IMR(PME_TO_ACK_RCVD)	|		\
> +		IMR(MISC_CORRECTABLE)	|		\
> +		IMR(NONFATAL)		|		\
> +		IMR(FATAL)				\
> +	)
> +
> +#define AMD_MDB_TLP_PCIE_INTX_MASK	GENMASK(23, 16)

I would drop AMD_MDB_PCIE_INTR_INTA_ASSERT, etc, and just use
AMD_MDB_TLP_PCIE_INTX_MASK in the AMD_MDB_PCIE_IMR_ALL_MASK
definition.

If there are really eight bits of INTx-related things here for the
four INTx interrupts, I think you should make two #defines to separate
them out.

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

  val &= ~AMD_MDB_PCIE_INTX_BIT(data->hwirq);
  pcie_write(pcie, val, AMD_MDB_TLP_IR_ENABLE_MISC);

> +	pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_ENABLE_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_MASK_MISC);

  val |= AMD_MDB_PCIE_INTX_BIT(data->hwirq);

> +	pcie_write(pcie, (val | mask), AMD_MDB_TLP_IR_ENABLE_MISC);
> +	raw_spin_unlock_irqrestore(&port->lock, flags);
> +}
> +
> +static struct irq_chip amd_mdb_intx_irq_chip = {
> +	.name		= "AMD MDB INTx",
> +	.irq_mask	= amd_mdb_mask_intx_irq,
> +	.irq_unmask	= amd_mdb_unmask_intx_irq,

Prefer

  .irq_mask       = amd_mdb_intx_irq_mask,
  .irq_unmask     = amd_mdb_intx_irq_unmask,

so the function names match the grep pattern of the function pointers
(".*_irq_mask").

> +static struct irq_chip amd_mdb_event_irq_chip = {
> +	.name		= "AMD MDB RC-Event",
> +	.irq_mask	= amd_mdb_mask_event_irq,
> +	.irq_unmask	= amd_mdb_unmask_event_irq,

Same function name comment.

> +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args)
> +{
> +	struct amd_mdb_pcie *pcie = args;
> +	unsigned long val;
> +	int i;
> +
> +	val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> +			pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> +
> +	for_each_set_bit(i, &val, 4)

  for_each_set_bit(..., PCI_NUM_INTX)

> +		generic_handle_domain_irq(pcie->intx_domain, i);
> +
> +	return IRQ_HANDLED;
> +}

> +
> +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_once(dev, "Unknown IRQ %ld\n", d->hwirq);

What's the point of an interrupt handler that only logs it?

> +	return IRQ_HANDLED;
> +}

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

amd_mdb_pcie_init_port() doesn't initialize anything other than
disabling/clearing/enabling interrupts.  Seems like it could be
squashed into amd_mdb_setup_irq() or called from there so it's
obvious that it's interrupt-related.

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

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

[Havalige, Thippeswamy]

Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Monday, February 3, 2025 11: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: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
> 
> On Wed, Jan 29, 2025 at 05:00:29PM +0530, Thippeswamy Havalige wrote:
> > Add support for AMD MDB (Multimedia DMA Bridge) IP core as Root Port.
> 
> > +#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
> 
> Remove this _SHIFT #define and use something like this instead:
> 
>   #define AMD_MDB_PCIE_INTX_BIT(x)
> FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK, BIT(x))
Thanks, will update this in next patch.
> 
> I don't know what exactly the right name for that is; it looks like maybe these
> bits apply to all the above registers (AMD_MDB_TLP_IR_STATUS_MISC,
> AMD_MDB_TLP_IR_MASK_MISC,
> AMD_MDB_TLP_IR_ENABLE_MISC)
> 
> > +#define AMD_MDB_PCIE_INTR_INTA_ASSERT		16
> > +#define AMD_MDB_PCIE_INTR_INTB_ASSERT		18
> > +#define AMD_MDB_PCIE_INTR_INTC_ASSERT		20
> > +#define AMD_MDB_PCIE_INTR_INTD_ASSERT		22
> 
> It's kind of weird that these skip the odd-numbered bits, since
> dw_pcie_rp_intx_flow(), amd_mdb_mask_intx_irq(),
> amd_mdb_unmask_intx_irq() only use bits 19:16.  Something seems wrong
> and needs either a fix or a comment about why this is the way it is.
- Thanks for review comments, the odd bits are meant for deasserting inta, intb intc & intd
I ll include this in my next patch 
> 
> > +#define IMR(x) BIT(AMD_MDB_PCIE_INTR_ ##x)
> > +#define AMD_MDB_PCIE_IMR_ALL_MASK			\
> > +	(						\
> > +		IMR(CMPL_TIMEOUT)	|		\
> > +		IMR(INTA_ASSERT)	|		\
> > +		IMR(INTB_ASSERT)	|		\
> > +		IMR(INTC_ASSERT)	|		\
> > +		IMR(INTD_ASSERT)	|		\
> > +		IMR(PM_PME_RCVD)	|		\
> > +		IMR(PME_TO_ACK_RCVD)	|		\
> > +		IMR(MISC_CORRECTABLE)	|		\
> > +		IMR(NONFATAL)		|		\
> > +		IMR(FATAL)				\
> > +	)
> > +
> > +#define AMD_MDB_TLP_PCIE_INTX_MASK	GENMASK(23, 16)
> 
> I would drop AMD_MDB_PCIE_INTR_INTA_ASSERT, etc, and just use
> AMD_MDB_TLP_PCIE_INTX_MASK in the AMD_MDB_PCIE_IMR_ALL_MASK
> definition.
> 
> If there are really eight bits of INTx-related things here for the four INTx
> interrupts, I think you should make two #defines to separate them out.
Thanks for review comments. Yes, there are 8 intx related bits I ll define them in
my next patch. I was in confusion here regarding "PCI_NUM_INTX " since this macro
indicates INTA INTB INTC INTD bits so I discarded deassert bits here.
> 
> > +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_MASK_MISC);
> 
>   val &= ~AMD_MDB_PCIE_INTX_BIT(data->hwirq);
>   pcie_write(pcie, val, AMD_MDB_TLP_IR_ENABLE_MISC);
- Thanks for review comments, Will update this in our next patch.
> 
> > +	pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_ENABLE_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_MASK_MISC);
> 
>   val |= AMD_MDB_PCIE_INTX_BIT(data->hwirq);
- Thanks for review comments, Will update this in our next patch.
> 
> > +	pcie_write(pcie, (val | mask), AMD_MDB_TLP_IR_ENABLE_MISC);
> > +	raw_spin_unlock_irqrestore(&port->lock, flags); }
> > +
> > +static struct irq_chip amd_mdb_intx_irq_chip = {
> > +	.name		= "AMD MDB INTx",
> > +	.irq_mask	= amd_mdb_mask_intx_irq,
> > +	.irq_unmask	= amd_mdb_unmask_intx_irq,
> 
> Prefer
> 
>   .irq_mask       = amd_mdb_intx_irq_mask,
>   .irq_unmask     = amd_mdb_intx_irq_unmask,
> 
> so the function names match the grep pattern of the function pointers
> (".*_irq_mask").
- Thanks for review comments, Will update this in our next patch.
> 
> > +static struct irq_chip amd_mdb_event_irq_chip = {
> > +	.name		= "AMD MDB RC-Event",
> > +	.irq_mask	= amd_mdb_mask_event_irq,
> > +	.irq_unmask	= amd_mdb_unmask_event_irq,
> 
> Same function name comment.
- Thanks for review comments, Will update this in our next patch.
> 
> > +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args) {
> > +	struct amd_mdb_pcie *pcie = args;
> > +	unsigned long val;
> > +	int i;
> > +
> > +	val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> > +			pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> > +
> > +	for_each_set_bit(i, &val, 4)
> 
>   for_each_set_bit(..., PCI_NUM_INTX)
- Thanks for review comments, In next patch I will update value to 8 here.
> 
> > +		generic_handle_domain_irq(pcie->intx_domain, i);
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> > +
> > +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_once(dev, "Unknown IRQ %ld\n", d->hwirq);
> 
> What's the point of an interrupt handler that only logs it?
- Thank you for your valuable review comments. At this stage, our objective is to notify the
user of the occurrence of an event. While we intend to integrate these events with the AER 
subsystem in the future, for the time being, we will limit the functionality to notifying the user.
> 
> > +	return IRQ_HANDLED;
> > +}
> 
> > +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);
> 
> amd_mdb_pcie_init_port() doesn't initialize anything other than
> disabling/clearing/enabling interrupts.  Seems like it could be squashed into
> amd_mdb_setup_irq() or called from there so it's obvious that it's interrupt-
> related.
Thanks for review comment, I will update this in next patch.
> 
> > +	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;
> > +}

Regards,
Thippeswamy H

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

[Bjorn Helgaas]

On Tue, Feb 04, 2025 at 09:37:51AM +0000, Havalige, Thippeswamy wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> ...
> > On Wed, Jan 29, 2025 at 05:00:29PM +0530, Thippeswamy Havalige wrote:
> > > Add support for AMD MDB (Multimedia DMA Bridge) IP core as Root Port.

> > > +#define AMD_MDB_PCIE_INTR_INTA_ASSERT		16
> > > +#define AMD_MDB_PCIE_INTR_INTB_ASSERT		18
> > > +#define AMD_MDB_PCIE_INTR_INTC_ASSERT		20
> > > +#define AMD_MDB_PCIE_INTR_INTD_ASSERT		22
> > 
> > It's kind of weird that these skip the odd-numbered bits, since
> > dw_pcie_rp_intx_flow(), amd_mdb_mask_intx_irq(),
> > amd_mdb_unmask_intx_irq() only use bits 19:16.  Something seems wrong
> > and needs either a fix or a comment about why this is the way it is.
>
> ... the odd bits are meant for deasserting inta, intb intc & intd I
> ll include this in my next patch 

> > > +#define IMR(x) BIT(AMD_MDB_PCIE_INTR_ ##x)
> > > +#define AMD_MDB_PCIE_IMR_ALL_MASK			\
> > > +	(						\
> > > +		IMR(CMPL_TIMEOUT)	|		\
> > > +		IMR(INTA_ASSERT)	|		\
> > > +		IMR(INTB_ASSERT)	|		\
> > > +		IMR(INTC_ASSERT)	|		\
> > > +		IMR(INTD_ASSERT)	|		\
> > > +		IMR(PM_PME_RCVD)	|		\
> > > +		IMR(PME_TO_ACK_RCVD)	|		\
> > > +		IMR(MISC_CORRECTABLE)	|		\
> > > +		IMR(NONFATAL)		|		\
> > > +		IMR(FATAL)				\
> > > +	)
> > > +
> > > +#define AMD_MDB_TLP_PCIE_INTX_MASK	GENMASK(23, 16)
> > 
> > I would drop AMD_MDB_PCIE_INTR_INTA_ASSERT, etc, and just use
> > AMD_MDB_TLP_PCIE_INTX_MASK in the AMD_MDB_PCIE_IMR_ALL_MASK
> > definition.
> > 
> > If there are really eight bits of INTx-related things here for the
> > four INTx interrupts, I think you should make two #defines to
> > separate them out.

> Yes, there are 8 intx related bits I ll define them in my next
> patch. I was in confusion here regarding "PCI_NUM_INTX " since this
> macro indicates INTA INTB INTC INTD bits so I discarded deassert
> bits here.

It seems like what you have is a single 8-bit field that contains both
assert and deassert info, interspersed.  GENMASK()/FIELD_GET() isn't
enough to really separate them.  Maybe you can do something like this:

  #define AMD_MDB_TLP_PCIE_INTX_MASK          GENMASK(23, 16)

  #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(1 << x)

If you don't need the deassert bits, a comment would be useful, but
there's no point in adding a #define for them.  If you do need them,
maybe this:

  #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)  BIT((1 << x) + 1)

> > > +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args) {
> > > +	struct amd_mdb_pcie *pcie = args;
> > > +	unsigned long val;
> > > +	int i;
> > > +
> > > +	val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> > > +			pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> > > +
> > > +	for_each_set_bit(i, &val, 4)
> > 
> >   for_each_set_bit(..., PCI_NUM_INTX)

> In next patch I will update value to 8 here.

And here you could do:

  val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
                  pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));

  for (i = 0; i < PCI_NUM_INTX; i++) {
    if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))
      generic_handle_domain_irq(pcie->intx_domain, i);

> > > +		generic_handle_domain_irq(pcie->intx_domain, i);

> > > +	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_once(dev, "Unknown IRQ %ld\n", d->hwirq);
> > 
> > What's the point of an interrupt handler that only logs it?
>
> At this stage, our objective is to notify the user of the occurrence
> of an event. While we intend to integrate these events with the AER
> subsystem in the future, for the time being, we will limit the
> functionality to notifying the user.

OK, just add a comment to that effect here.

Bjorn

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

[Havalige, Thippeswamy]

Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, February 5, 2025 3:42 AM
> 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: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
> 
> On Tue, Feb 04, 2025 at 09:37:51AM +0000, Havalige, Thippeswamy wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <helgaas@kernel.org>
> > ...
> > > On Wed, Jan 29, 2025 at 05:00:29PM +0530, Thippeswamy Havalige wrote:
> > > > Add support for AMD MDB (Multimedia DMA Bridge) IP core as Root
> Port.
> 
> > > > +#define AMD_MDB_PCIE_INTR_INTA_ASSERT		16
> > > > +#define AMD_MDB_PCIE_INTR_INTB_ASSERT		18
> > > > +#define AMD_MDB_PCIE_INTR_INTC_ASSERT		20
> > > > +#define AMD_MDB_PCIE_INTR_INTD_ASSERT		22
> > >
> > > It's kind of weird that these skip the odd-numbered bits, since
> > > dw_pcie_rp_intx_flow(), amd_mdb_mask_intx_irq(),
> > > amd_mdb_unmask_intx_irq() only use bits 19:16.  Something seems
> > > wrong and needs either a fix or a comment about why this is the way it is.
> >
> > ... the odd bits are meant for deasserting inta, intb intc & intd I ll
> > include this in my next patch
> 
> > > > +#define IMR(x) BIT(AMD_MDB_PCIE_INTR_ ##x)
> > > > +#define AMD_MDB_PCIE_IMR_ALL_MASK			\
> > > > +	(						\
> > > > +		IMR(CMPL_TIMEOUT)	|		\
> > > > +		IMR(INTA_ASSERT)	|		\
> > > > +		IMR(INTB_ASSERT)	|		\
> > > > +		IMR(INTC_ASSERT)	|		\
> > > > +		IMR(INTD_ASSERT)	|		\
> > > > +		IMR(PM_PME_RCVD)	|		\
> > > > +		IMR(PME_TO_ACK_RCVD)	|		\
> > > > +		IMR(MISC_CORRECTABLE)	|		\
> > > > +		IMR(NONFATAL)		|		\
> > > > +		IMR(FATAL)				\
> > > > +	)
> > > > +
> > > > +#define AMD_MDB_TLP_PCIE_INTX_MASK	GENMASK(23, 16)
> > >
> > > I would drop AMD_MDB_PCIE_INTR_INTA_ASSERT, etc, and just use
> > > AMD_MDB_TLP_PCIE_INTX_MASK in the
> AMD_MDB_PCIE_IMR_ALL_MASK
> > > definition.
> > >
> > > If there are really eight bits of INTx-related things here for the
> > > four INTx interrupts, I think you should make two #defines to
> > > separate them out.
> 
> > Yes, there are 8 intx related bits I ll define them in my next patch.
> > I was in confusion here regarding "PCI_NUM_INTX " since this macro
> > indicates INTA INTB INTC INTD bits so I discarded deassert bits here.
> 
> It seems like what you have is a single 8-bit field that contains both assert and
> deassert info, interspersed.  GENMASK()/FIELD_GET() isn't enough to really
> separate them.  Maybe you can do something like this:
> 
>   #define AMD_MDB_TLP_PCIE_INTX_MASK          GENMASK(23, 16)
> 
>   #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(1 << x)
> 
> If you don't need the deassert bits, a comment would be useful, but there's
> no point in adding a #define for them.  If you do need them, maybe this:
> 
>   #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)  BIT((1 << x) + 1)
> 
> > > > +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args) {
> > > > +	struct amd_mdb_pcie *pcie = args;
> > > > +	unsigned long val;
> > > > +	int i;
> > > > +
> > > > +	val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> > > > +			pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> > > > +
> > > > +	for_each_set_bit(i, &val, 4)
> > >
> > >   for_each_set_bit(..., PCI_NUM_INTX)
> 
> > In next patch I will update value to 8 here.
> 
> And here you could do:
> 
>   val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
>                   pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> 
>   for (i = 0; i < PCI_NUM_INTX; i++) {
>     if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))
- Thanks for reviewing, This condition never met observing zero here.
>       generic_handle_domain_irq(pcie->intx_domain, i);
> 
> > > > +		generic_handle_domain_irq(pcie->intx_domain, i);
> 
> > > > +	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_once(dev, "Unknown IRQ %ld\n", d->hwirq);
> > >
> > > What's the point of an interrupt handler that only logs it?
> >
> > At this stage, our objective is to notify the user of the occurrence
> > of an event. While we intend to integrate these events with the AER
> > subsystem in the future, for the time being, we will limit the
> > functionality to notifying the user.
> 
> OK, just add a comment to that effect here.
- Thanks for reviewing, will add this in next patch.
> 
> Bjorn

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

[Havalige, Thippeswamy]

Hi Bjorn,

> -----Original Message-----
> From: Havalige, Thippeswamy
> Sent: Wednesday, February 5, 2025 5:08 PM
> To: Bjorn Helgaas <helgaas@kernel.org>
> 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: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
> 
> Hi Bjorn,
> > > > It's kind of weird that these skip the odd-numbered bits, since
> > > > dw_pcie_rp_intx_flow(), amd_mdb_mask_intx_irq(),
> > > > amd_mdb_unmask_intx_irq() only use bits 19:16.  Something seems
> > > > wrong and needs either a fix or a comment about why this is the way it
> is.
> > >
> > > ... the odd bits are meant for deasserting inta, intb intc & intd I
> > > ll include this in my next patch
> >
> > > > > +#define IMR(x) BIT(AMD_MDB_PCIE_INTR_ ##x)
> > > > > +#define AMD_MDB_PCIE_IMR_ALL_MASK			\
> > > > > +	(						\
> > > > > +		IMR(CMPL_TIMEOUT)	|		\
> > > > > +		IMR(INTA_ASSERT)	|		\
> > > > > +		IMR(INTB_ASSERT)	|		\
> > > > > +		IMR(INTC_ASSERT)	|		\
> > > > > +		IMR(INTD_ASSERT)	|		\
> > > > > +		IMR(PM_PME_RCVD)	|		\
> > > > > +		IMR(PME_TO_ACK_RCVD)	|		\
> > > > > +		IMR(MISC_CORRECTABLE)	|		\
> > > > > +		IMR(NONFATAL)		|		\
> > > > > +		IMR(FATAL)				\
> > > > > +	)
> > > > > +
> > > > > +#define AMD_MDB_TLP_PCIE_INTX_MASK	GENMASK(23, 16)
> > > >
> > > > I would drop AMD_MDB_PCIE_INTR_INTA_ASSERT, etc, and just use
> > > > AMD_MDB_TLP_PCIE_INTX_MASK in the
> > AMD_MDB_PCIE_IMR_ALL_MASK
> > > > definition.
> > > >
> > > > If there are really eight bits of INTx-related things here for the
> > > > four INTx interrupts, I think you should make two #defines to
> > > > separate them out.
> >
> > > Yes, there are 8 intx related bits I ll define them in my next patch.
> > > I was in confusion here regarding "PCI_NUM_INTX " since this macro
> > > indicates INTA INTB INTC INTD bits so I discarded deassert bits here.
> >
> > It seems like what you have is a single 8-bit field that contains both
> > assert and deassert info, interspersed.  GENMASK()/FIELD_GET() isn't
> > enough to really separate them.  Maybe you can do something like this:
> >
> >   #define AMD_MDB_TLP_PCIE_INTX_MASK          GENMASK(23, 16)
> >
> >   #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(1 << x)
> >
> > If you don't need the deassert bits, a comment would be useful, but
> > there's no point in adding a #define for them.  If you do need them, maybe
> this:
> >
> >   #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)  BIT((1 << x) + 1)
> >
> > > > > +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args) {
> > > > > +	struct amd_mdb_pcie *pcie = args;
> > > > > +	unsigned long val;
> > > > > +	int i;
> > > > > +
> > > > > +	val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> > > > > +			pcie_read(pcie,
> AMD_MDB_TLP_IR_STATUS_MISC));
> > > > > +
> > > > > +	for_each_set_bit(i, &val, 4)
> > > >
> > > >   for_each_set_bit(..., PCI_NUM_INTX)
> >
> > > In next patch I will update value to 8 here.
> >
> > And here you could do:
> >
> >   val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> >                   pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> >
> >   for (i = 0; i < PCI_NUM_INTX; i++) {
> >     if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))
> - Thanks for reviewing, This condition never met observing zero here.
To satisfy this condition need to modify macros as following.
#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(x)
#define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)    BIT(x+1)

> >       generic_handle_domain_irq(pcie->intx_domain, i);
> >
> > > > > +		generic_handle_domain_irq(pcie->intx_domain, i);
> >
> > > > > +	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_once(dev, "Unknown IRQ %ld\n", d-
> >hwirq);
> > > >
> > > > What's the point of an interrupt handler that only logs it?
> > >
> > > At this stage, our objective is to notify the user of the occurrence
> > > of an event. While we intend to integrate these events with the AER
> > > subsystem in the future, for the time being, we will limit the
> > > functionality to notifying the user.
> >
> > OK, just add a comment to that effect here.
> - Thanks for reviewing, will add this in next patch.
> >
> > Bjorn

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

[Bjorn Helgaas]

On Wed, Feb 05, 2025 at 11:53:53AM +0000, Havalige, Thippeswamy wrote:
> > -----Original Message-----
> > From: Havalige, Thippeswamy
> > Sent: Wednesday, February 5, 2025 5:08 PM
> > To: Bjorn Helgaas <helgaas@kernel.org>
> > 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: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver

> > > > > It's kind of weird that these skip the odd-numbered bits,
> > > > > since dw_pcie_rp_intx_flow(), amd_mdb_mask_intx_irq(),
> > > > > amd_mdb_unmask_intx_irq() only use bits 19:16.  Something
> > > > > seems wrong and needs either a fix or a comment about why
> > > > > this is the way it is.
> > > >
> > > > ... the odd bits are meant for deasserting inta, intb intc &
> > > > intd I ll include this in my next patch

Tangent: I don't know what "deassert" would mean here, since INTx is
an *incoming* interrupt and the Root Port is the receiver and can mask
or acknowledge the interrupt but not really deassert it.

> > > > > > +#define IMR(x) BIT(AMD_MDB_PCIE_INTR_ ##x)
> > > > > > +#define AMD_MDB_PCIE_IMR_ALL_MASK			\
> > > > > > +	(						\
> > > > > > +		IMR(CMPL_TIMEOUT)	|		\
> > > > > > +		IMR(INTA_ASSERT)	|		\
> > > > > > +		IMR(INTB_ASSERT)	|		\
> > > > > > +		IMR(INTC_ASSERT)	|		\
> > > > > > +		IMR(INTD_ASSERT)	|		\
> > > > > > +		IMR(PM_PME_RCVD)	|		\
> > > > > > +		IMR(PME_TO_ACK_RCVD)	|		\
> > > > > > +		IMR(MISC_CORRECTABLE)	|		\
> > > > > > +		IMR(NONFATAL)		|		\
> > > > > > +		IMR(FATAL)				\
> > > > > > +	)
> > > > > > +
> > > > > > +#define AMD_MDB_TLP_PCIE_INTX_MASK	GENMASK(23, 16)
> > > > >
> > > > > I would drop AMD_MDB_PCIE_INTR_INTA_ASSERT, etc, and just
> > > > > use AMD_MDB_TLP_PCIE_INTX_MASK in the
> > > > > AMD_MDB_PCIE_IMR_ALL_MASK definition.
> > > > >
> > > > > If there are really eight bits of INTx-related things here
> > > > > for the four INTx interrupts, I think you should make two
> > > > > #defines to separate them out.
> > >
> > > > Yes, there are 8 intx related bits I ll define them in my next
> > > > patch.  I was in confusion here regarding "PCI_NUM_INTX "
> > > > since this macro indicates INTA INTB INTC INTD bits so I
> > > > discarded deassert bits here.
> > >
> > > It seems like what you have is a single 8-bit field that
> > > contains both assert and deassert info, interspersed.
> > > GENMASK()/FIELD_GET() isn't enough to really separate them.
> > > Maybe you can do something like this:
> > >
> > >   #define AMD_MDB_TLP_PCIE_INTX_MASK          GENMASK(23, 16)
> > >
> > >   #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(1 << x)
> > >
> > > If you don't need the deassert bits, a comment would be useful,
> > > but there's no point in adding a #define for them.  If you do
> > > need them, maybe this:
> > >
> > >   #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)  BIT((1 << x) + 1)
> > >
> > > > > > +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args) {
> > > > > > +	struct amd_mdb_pcie *pcie = args;
> > > > > > +	unsigned long val;
> > > > > > +	int i;
> > > > > > +
> > > > > > +	val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> > > > > > +			pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> > > > > > +
> > > > > > +	for_each_set_bit(i, &val, 4)
> > > > >
> > > > >   for_each_set_bit(..., PCI_NUM_INTX)
> > >
> > > > In next patch I will update value to 8 here.
> > >
> > > And here you could do:
> > >
> > >   val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> > >                   pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> > >
> > >   for (i = 0; i < PCI_NUM_INTX; i++) {
> > >     if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))

> > This condition never met observing zero here.

> To satisfy this condition need to modify macros as following.
> #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(x)
> #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)    BIT(x+1)

Maybe I don't understand how the assert/deassert bits are laid out in
the register.

The original patch has this:

  +#define AMD_MDB_PCIE_INTR_INTA_ASSERT    16
  +#define AMD_MDB_PCIE_INTR_INTB_ASSERT    18
  +#define AMD_MDB_PCIE_INTR_INTC_ASSERT    20
  +#define AMD_MDB_PCIE_INTR_INTD_ASSERT    22

and if the odd bits are for deassert I thought that meant they would
look like this:

   #define AMD_MDB_PCIE_INTR_INTA_DEASSERT  17
   #define AMD_MDB_PCIE_INTR_INTB_DEASSERT  19
   #define AMD_MDB_PCIE_INTR_INTC_DEASSERT  21
   #define AMD_MDB_PCIE_INTR_INTD_DEASSERT  23

  +#define AMD_MDB_TLP_PCIE_INTX_MASK     GENMASK(23, 16)

If we extract AMD_MDB_TLP_PCIE_INTX_MASK with FIELD_GET(),
the field gets shifted right by 16, so we should end up with
something like this:

  INTA assert     0000 0001  ==  BIT(0)
  INTA deassert   0000 0010  ==  BIT(1)
  INTB assert     0000 0100  ==  BIT(2)
  INTB deassert   0000 1000  ==  BIT(3)
  INTC assert     0001 0000  ==  BIT(4)
  INTC deassert   0010 0000  ==  BIT(5)
  INTD assert     0100 0000  ==  BIT(6)
  INTD deassert   1000 0000  ==  BIT(7)

But maybe that's not how they're actually laid out?

I think the argument to AMD_MDB_PCIE_INTR_INTX_ASSERT() should
be the hwirq (0..3 for INTA..INTD), so if we use

  #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(x)
  #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)  BIT(x+1)

as you propose, don't the assert/deassert bits collide?

  AMD_MDB_PCIE_INTR_INTX_ASSERT(0)   == BIT(0) for INTA assert
  AMD_MDB_PCIE_INTR_INTX_ASSERT(1)   == BIT(1) for INTB assert

  AMD_MDB_PCIE_INTR_INTX_DEASSERT(0) == BIT(1) for INTA deassert

> > >       generic_handle_domain_irq(pcie->intx_domain, i);
> > >
> > > > > > +		generic_handle_domain_irq(pcie->intx_domain, i);

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

[Markus Elfring]

…
> +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> @@ -0,0 +1,476 @@
…
> +static void amd_mdb_mask_intx_irq(struct irq_data *data)
> +{
…
> +	raw_spin_lock_irqsave(&port->lock, flags);
> +	val = pcie_read(pcie, AMD_MDB_TLP_IR_MASK_MISC);
> +	pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_ENABLE_MISC);
> +	raw_spin_unlock_irqrestore(&port->lock, flags);
> +}
…

Under which circumstances would you become interested to apply a statement
like “guard(raw_spinlock_irqsave)(&port->lock);”?
https://elixir.bootlin.com/linux/v6.13.1/source/include/linux/spinlock.h#L551

Regards,
Markus

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

[Havalige, Thippeswamy]

Hi @Markus Elfring

> -----Original Message-----
> From: Markus Elfring <Markus.Elfring@web.de>
> Sent: Wednesday, February 5, 2025 8:40 PM
> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>; linux-
> pci@vger.kernel.org; devicetree@vger.kernel.org; Bjorn Helgaas
> <bhelgaas@google.com>; Conor Dooley <conor+dt@kernel.org>; Krzysztof
> Kozlowski <krzk+dt@kernel.org>; Krzysztof Wilczyński <kw@linux.com>;
> Lorenzo Pieralisi <lpieralisi@kernel.org>; Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org>; Rob Herring <robh@kernel.org>
> Cc: LKML <linux-kernel@vger.kernel.org>; Gogada, Bharat Kumar
> <bharat.kumar.gogada@amd.com>; Jingoo Han <jingoohan1@gmail.com>;
> Simek, Michal <michal.simek@amd.com>
> Subject: Re: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
> 
> …
> > +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> > @@ -0,0 +1,476 @@
> …
> > +static void amd_mdb_mask_intx_irq(struct irq_data *data)
> > +{
> …
> > +	raw_spin_lock_irqsave(&port->lock, flags);
> > +	val = pcie_read(pcie, AMD_MDB_TLP_IR_MASK_MISC);
> > +	pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_ENABLE_MISC);
> > +	raw_spin_unlock_irqrestore(&port->lock, flags);
> > +}
> …
> 
> Under which circumstances would you become interested to apply a
> statement
> like “guard(raw_spinlock_irqsave)(&port->lock);”?
> https://elixir.bootlin.com/linux/v6.13.1/source/include/linux/spinlock.h#L551
-Thanks for review comments, raw_spin_lock_irqsave is lightweight and performance oriented. 
Achieves it by not performing few checks related to preemption, lock deprecation that was originally in spin_lock_irqsave.
If you add guard around guard around the raw_spin_lock_irqsave then it should provide those additional safety checks.
Its need is based on the environment, if you needs those checks/features.
We need to check the implementation/code to exactly see what are those. So choose to prevent my interrupt handler from
 being affected by latency pruning

> 
> Regards,
> Markus

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

[Bjorn Helgaas]

On Thu, Feb 06, 2025 at 04:07:23PM +0000, Havalige, Thippeswamy wrote:
> > -----Original Message-----
> > From: Markus Elfring <Markus.Elfring@web.de>
> > Sent: Wednesday, February 5, 2025 8:40 PM
> > To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>; linux-
> > pci@vger.kernel.org; devicetree@vger.kernel.org; Bjorn Helgaas
> > <bhelgaas@google.com>; Conor Dooley <conor+dt@kernel.org>; Krzysztof
> > Kozlowski <krzk+dt@kernel.org>; Krzysztof Wilczyński <kw@linux.com>;
> > Lorenzo Pieralisi <lpieralisi@kernel.org>; Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org>; Rob Herring <robh@kernel.org>
> > Cc: LKML <linux-kernel@vger.kernel.org>; Gogada, Bharat Kumar
> > <bharat.kumar.gogada@amd.com>; Jingoo Han <jingoohan1@gmail.com>;
> > Simek, Michal <michal.simek@amd.com>
> > Subject: Re: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
> > 
> > …
> > > +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> > > @@ -0,0 +1,476 @@
> > …
> > > +static void amd_mdb_mask_intx_irq(struct irq_data *data)
> > > +{
> > …
> > > +	raw_spin_lock_irqsave(&port->lock, flags);
> > > +	val = pcie_read(pcie, AMD_MDB_TLP_IR_MASK_MISC);
> > > +	pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_ENABLE_MISC);
> > > +	raw_spin_unlock_irqrestore(&port->lock, flags);
> > > +}
> > …
> > 
> > Under which circumstances would you become interested to apply a
> > statement like “guard(raw_spinlock_irqsave)(&port->lock);”?
> > https://elixir.bootlin.com/linux/v6.13.1/source/include/linux/spinlock.h#L551
>
> -Thanks for review comments, raw_spin_lock_irqsave is lightweight
> and performance oriented. 
>
> Achieves it by not performing few checks related to preemption, lock
> deprecation that was originally in spin_lock_irqsave.
>
> If you add guard around guard around the raw_spin_lock_irqsave then
> it should provide those additional safety checks.
>
> Its need is based on the environment, if you needs those
> checks/features.  We need to check the implementation/code to
> exactly see what are those. So choose to prevent my interrupt
> handler from being affected by latency pruning

IIUC, using guard() would not add any additional checks; it only helps
prevent errors like returning from the function without releasing the
lock.

That makes sense in larger functions with multiple exits, but IMO
there's no real benefit in a case like this where the function is so
short, there's only one exit, and it's completely obvious that we
lock, do something, and unlock.

I don't *really* like guard() anyway because it's kind of magic in
that the unlock doesn't actually appear in the code, and it's kind of
hard to unravel what guard() is and how it works.  But I guess that's
mostly because it's just a new idiom that takes time to internalize.

Bjorn

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

[Markus Elfring]

> I don't *really* like guard() anyway because it's kind of magic in
> that the unlock doesn't actually appear in the code, and it's kind of
> hard to unravel what guard() is and how it works.  But I guess that's
> mostly because it's just a new idiom that takes time to internalize.
How will the circumstances evolve further for growing applications of
scope-based resource management?

Regards,
Markus

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

[Manivannan Sadhasivam]

On Fri, Feb 07, 2025 at 07:39:03AM +0100, Markus Elfring wrote:
> > I don't *really* like guard() anyway because it's kind of magic in
> > that the unlock doesn't actually appear in the code, and it's kind of
> > hard to unravel what guard() is and how it works.  But I guess that's
> > mostly because it's just a new idiom that takes time to internalize.
> How will the circumstances evolve further for growing applications of
> scope-based resource management?
> 

Please ignore the comments from Markus.

Reference: https://lkml.org/lkml/2024/5/20/724

- Mani

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

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

[Markus Elfring]

> Please ignore the comments from Markus.
I hope that patch review and software development discussions can become more constructive again
also for related topics.

Regards,
Markus

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

[Havalige, Thippeswamy]

Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, February 5, 2025 7:51 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: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
> 
> On Wed, Feb 05, 2025 at 11:53:53AM +0000, Havalige, Thippeswamy wrote:
> > > -----Original Message-----
> > > From: Havalige, Thippeswamy
> > > Sent: Wednesday, February 5, 2025 5:08 PM
> > > To: Bjorn Helgaas <helgaas@kernel.org>
> > > 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: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port
> driver
> 
> > > > > > It's kind of weird that these skip the odd-numbered bits,
> > > > > > since dw_pcie_rp_intx_flow(), amd_mdb_mask_intx_irq(),
> > > > > > amd_mdb_unmask_intx_irq() only use bits 19:16.  Something
> > > > > > seems wrong and needs either a fix or a comment about why
> > > > > > this is the way it is.
> > > > >
> > > > > ... the odd bits are meant for deasserting inta, intb intc &
> > > > > intd I ll include this in my next patch
> 
> Tangent: I don't know what "deassert" would mean here, since INTx is
> an *incoming* interrupt and the Root Port is the receiver and can mask
> or acknowledge the interrupt but not really deassert it.
>

Yes, I agree but our controller, however, provides explicit tracking of both transitions using internal registers
(TLP_IR_STATUS_MISC).

> > > > > > > +#define IMR(x) BIT(AMD_MDB_PCIE_INTR_ ##x)
> > > > > > > +#define AMD_MDB_PCIE_IMR_ALL_MASK			\
> > > > > > > +	(						\
> > > > > > > +		IMR(CMPL_TIMEOUT)	|		\
> > > > > > > +		IMR(INTA_ASSERT)	|		\
> > > > > > > +		IMR(INTB_ASSERT)	|		\
> > > > > > > +		IMR(INTC_ASSERT)	|		\
> > > > > > > +		IMR(INTD_ASSERT)	|		\
> > > > > > > +		IMR(PM_PME_RCVD)	|		\
> > > > > > > +		IMR(PME_TO_ACK_RCVD)	|		\
> > > > > > > +		IMR(MISC_CORRECTABLE)	|		\
> > > > > > > +		IMR(NONFATAL)		|		\
> > > > > > > +		IMR(FATAL)				\
> > > > > > > +	)
> > > > > > > +
> > > > > > > +#define AMD_MDB_TLP_PCIE_INTX_MASK	GENMASK(23, 16)
> > > > > >
> > > > > > I would drop AMD_MDB_PCIE_INTR_INTA_ASSERT, etc, and just
> > > > > > use AMD_MDB_TLP_PCIE_INTX_MASK in the
> > > > > > AMD_MDB_PCIE_IMR_ALL_MASK definition.
> > > > > >
> > > > > > If there are really eight bits of INTx-related things here
> > > > > > for the four INTx interrupts, I think you should make two
> > > > > > #defines to separate them out.
> > > >
> > > > > Yes, there are 8 intx related bits I ll define them in my next
> > > > > patch.  I was in confusion here regarding "PCI_NUM_INTX "
> > > > > since this macro indicates INTA INTB INTC INTD bits so I
> > > > > discarded deassert bits here.
> > > >
> > > > It seems like what you have is a single 8-bit field that
> > > > contains both assert and deassert info, interspersed.
> > > > GENMASK()/FIELD_GET() isn't enough to really separate them.
> > > > Maybe you can do something like this:
> > > >
> > > >   #define AMD_MDB_TLP_PCIE_INTX_MASK          GENMASK(23, 16)
> > > >
> > > >   #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(1 << x)
> > > >
> > > > If you don't need the deassert bits, a comment would be useful,
> > > > but there's no point in adding a #define for them.  If you do
> > > > need them, maybe this:
> > > >
> > > >   #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)  BIT((1 << x) + 1)
> > > >
> > > > > > > +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args) {
> > > > > > > +	struct amd_mdb_pcie *pcie = args;
> > > > > > > +	unsigned long val;
> > > > > > > +	int i;
> > > > > > > +
> > > > > > > +	val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> > > > > > > +			pcie_read(pcie,
> AMD_MDB_TLP_IR_STATUS_MISC));
> > > > > > > +
> > > > > > > +	for_each_set_bit(i, &val, 4)
> > > > > >
> > > > > >   for_each_set_bit(..., PCI_NUM_INTX)
> > > >
> > > > > In next patch I will update value to 8 here.
> > > >
> > > > And here you could do:
> > > >
> > > >   val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> > > >                   pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> > > >
> > > >   for (i = 0; i < PCI_NUM_INTX; i++) {
> > > >     if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))
> 
> > > This condition never met observing zero here.
> 

The val variable will hold the bits that are currently set. For example, if the INTA bit is set (10000 in binary),
then val will have a value of 1 after applying FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK, 
pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC)).

Issue with these Macros:
#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(x << 1)
#define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)  BIT((x << 1) + 1)
When x = 0, the condition inside the loop:

if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))

expands to:
if (val & BIT(1 << 0))
Since 1 << 0 evaluates to 1, this becomes:

if (val & BIT(1))
If val = 1, this results in 1 & BIT(1), which evaluates to 0, meaning the condition is never satisfied.

> > To satisfy this condition need to modify macros as following.
> > #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(x)
> > #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)    BIT(x+1)
> 
> Maybe I don't understand how the assert/deassert bits are laid out in
> the register.
> 
> The original patch has this:
> 
>   +#define AMD_MDB_PCIE_INTR_INTA_ASSERT    16
>   +#define AMD_MDB_PCIE_INTR_INTB_ASSERT    18
>   +#define AMD_MDB_PCIE_INTR_INTC_ASSERT    20
>   +#define AMD_MDB_PCIE_INTR_INTD_ASSERT    22
> 
> and if the odd bits are for deassert I thought that meant they would
> look like this:
> 
>    #define AMD_MDB_PCIE_INTR_INTA_DEASSERT  17
>    #define AMD_MDB_PCIE_INTR_INTB_DEASSERT  19
>    #define AMD_MDB_PCIE_INTR_INTC_DEASSERT  21
>    #define AMD_MDB_PCIE_INTR_INTD_DEASSERT  23
> 

Yes, your correct. ASSERT & DEASSERT bits in the register are laid in the same way.

>   +#define AMD_MDB_TLP_PCIE_INTX_MASK     GENMASK(23, 16)
> 
> If we extract AMD_MDB_TLP_PCIE_INTX_MASK with FIELD_GET(),
> the field gets shifted right by 16, so we should end up with
> something like this:
> 
>   INTA assert     0000 0001  ==  BIT(0)
>   INTA deassert   0000 0010  ==  BIT(1)
>   INTB assert     0000 0100  ==  BIT(2)
>   INTB deassert   0000 1000  ==  BIT(3)
>   INTC assert     0001 0000  ==  BIT(4)
>   INTC deassert   0010 0000  ==  BIT(5)
>   INTD assert     0100 0000  ==  BIT(6)
>   INTD deassert   1000 0000  ==  BIT(7)
> 
> But maybe that's not how they're actually laid out?
> 
> I think the argument to AMD_MDB_PCIE_INTR_INTX_ASSERT() should
> be the hwirq (0..3 for INTA..INTD), so if we use
> 
>   #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(x)
>   #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)  BIT(x+1)
> 
> as you propose, don't the assert/deassert bits collide?
> 
>   AMD_MDB_PCIE_INTR_INTX_ASSERT(0)   == BIT(0) for INTA assert
>   AMD_MDB_PCIE_INTR_INTX_ASSERT(1)   == BIT(1) for INTB assert

Your analysis is correct-using above macros does not yield the expected results

> 
>   AMD_MDB_PCIE_INTR_INTX_DEASSERT(0) == BIT(1) for INTA deassert
> 
> > > >       generic_handle_domain_irq(pcie->intx_domain, i);
> > > >
> > > > > > > +		generic_handle_domain_irq(pcie->intx_domain, i);

To ensure proper handling of INTx interrupts, I plan to use the following macros:

#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(x * 2)
#define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)  BIT(x * 2 + 1)

With this approach, the conditions will be evaluated as follows:

For an assert bit:
if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))
Example:
AMD_MDB_PCIE_INTR_INTX_ASSERT(0) -> BIT(0 * 2) -> BIT(0),
If val = 1, then 1 & BIT(0) is true, and the interrupt will be handled.

For a deassert bit:
if (val & AMD_MDB_PCIE_INTR_INTX_DEASSERT(i))
Example:
AMD_MDB_PCIE_INTR_INTX_DEASSERT(0) -> BIT((0 * 2) + 1) -> BIT(1),
If val = 2, then 2 & BIT(1) is true, indicating that the deassert condition is met.

To incorporate this logic, I propose updating the loop as follows:
for_each_set_bit(i, &val, 8) {  
    if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))  
        generic_handle_domain_irq(pcie->intx_domain, i);  

    if (val & AMD_MDB_PCIE_INTR_INTX_DEASSERT(i))  
        generic_handle_domain_irq(pcie->intx_domain, i);  
}
This ensures that both assert and deassert conditions are handled correctly.
Let me know if you have any suggestions or correct me if am missing anything.

Regards,
Thippeswamy H

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

[Bjorn Helgaas]

On Fri, Feb 07, 2025 at 07:39:03AM +0100, Markus Elfring wrote:
> > I don't *really* like guard() anyway because it's kind of magic in
> > that the unlock doesn't actually appear in the code, and it's kind of
> > hard to unravel what guard() is and how it works.  But I guess that's
> > mostly because it's just a new idiom that takes time to internalize.
>
> How will the circumstances evolve further for growing applications of
> scope-based resource management?

I'm sure it will evolve to become the typical style.  Right now, it's
not quite there yet, as evidenced by the fact that the only reference
to them in Documentation/ is this somewhat ambivalent note:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-netdev.rst?id=v6.13#n380

We do already have a few uses of guard() and scoped_guard() in
drivers/pci, and I don't really object to more, including in this
amd-mdb case.  Whatever we do, I *would* want to do it consistently
throughout the file.

Bjorn

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

[Krzysztof Kozlowski]

On 12/02/2025 00:16, Bjorn Helgaas wrote:
> On Fri, Feb 07, 2025 at 07:39:03AM +0100, Markus Elfring wrote:
>>> I don't *really* like guard() anyway because it's kind of magic in
>>> that the unlock doesn't actually appear in the code, and it's kind of
>>> hard to unravel what guard() is and how it works.  But I guess that's
>>> mostly because it's just a new idiom that takes time to internalize.
>>
>> How will the circumstances evolve further for growing applications of
>> scope-based resource management?
> 
> I'm sure it will evolve to become the typical style.  Right now, it's
> not quite there yet, as evidenced by the fact that the only reference
> to them in Documentation/ is this somewhat ambivalent note:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-netdev.rst?id=v6.13#n380
> 
> We do already have a few uses of guard() and scoped_guard() in
> drivers/pci, and I don't really object to more, including in this
> amd-mdb case.  Whatever we do, I *would* want to do it consistently
> throughout the file.


Bjorn,
There is little point in discussing anything with Markus. It's a person
known to waste our time.

Best regards,
Krzysztof