[PATCH v7 0/2] Add support for AMD Versal Gen 2 MDB PCIe RP PERST#

[PATCH v7 0/2] Add support for AMD Versal Gen 2 MDB PCIe RP PERST#

[Sai Krishna Musham]

Add example usage of reset-gpios for PCIe RP PERST#

Add support for PCIe Root Port PERST# signal handling

Sai Krishna Musham (2):
  dt-bindings: PCI: amd-mdb: Add example usage of reset-gpios for PCIe
    RP PERST#
  PCI: amd-mdb: Add support for PCIe RP PERST# signal handling

 .../bindings/pci/amd,versal2-mdb-host.yaml    | 22 ++++++++
 drivers/pci/controller/dwc/pcie-amd-mdb.c     | 52 ++++++++++++++++++-
 2 files changed, 73 insertions(+), 1 deletion(-)


base-commit: 0bd0a41a5120f78685a132834865b0a631b9026a
-- 
2.43.0

[PATCH v7 1/2] dt-bindings: PCI: amd-mdb: Add example usage of reset-gpios for PCIe RP PERST#

[Sai Krishna Musham]

Update the device tree binding example to include usage of the
`reset-gpios` property in PCIe Root Port (RP) bridge node for PERST#
signal handling.

Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
Changes in v6 & v7:
- None

Changes in v5:
- Add Reviewed-by tag.

Changes in v4:
- Remove reset-gpios define as it is already part of pci-bus-common.yaml.

Changes in v3:
- Move reset-gpios to PCI bridge node.

Changes in v2:
- Update commit message

v6 https://lore.kernel.org/all/20250719030951.3616385-1-sai.krishna.musham@amd.com/
v5 https://lore.kernel.org/all/20250711052357.3859719-1-sai.krishna.musham@amd.com/
v4 https://lore.kernel.org/all/20250626054906.3277029-1-sai.krishna.musham@amd.com/
v3 https://lore.kernel.org/r/20250618080931.2472366-1-sai.krishna.musham@amd.com/
v2 https://lore.kernel.org/r/20250429090046.1512000-1-sai.krishna.musham@amd.com/
v1 https://lore.kernel.org/r/20250326041507.98232-1-sai.krishna.musham@amd.com/
---
 .../bindings/pci/amd,versal2-mdb-host.yaml    | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml b/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
index 43dc2585c237..421e1116ae7e 100644
--- a/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
+++ b/Documentation/devicetree/bindings/pci/amd,versal2-mdb-host.yaml
@@ -71,6 +71,17 @@ properties:
       - "#address-cells"
       - "#interrupt-cells"
 
+patternProperties:
+  '^pcie@[0-2],0$':
+    type: object
+    $ref: /schemas/pci/pci-pci-bridge.yaml#
+
+    properties:
+      reg:
+        maxItems: 1
+
+    unevaluatedProperties: false
+
 required:
   - reg
   - reg-names
@@ -87,6 +98,7 @@ examples:
   - |
     #include <dt-bindings/interrupt-controller/arm-gic.h>
     #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
 
     soc {
         #address-cells = <2>;
@@ -112,6 +124,16 @@ examples:
             #size-cells = <2>;
             #interrupt-cells = <1>;
             device_type = "pci";
+
+            pcie@0,0 {
+                device_type = "pci";
+                reg = <0x0 0x0 0x0 0x0 0x0>;
+                reset-gpios = <&tca6416_u37 7 GPIO_ACTIVE_LOW>;
+                #address-cells = <3>;
+                #size-cells = <2>;
+                ranges;
+            };
+
             pcie_intc_0: interrupt-controller {
                 #address-cells = <0>;
                 #interrupt-cells = <1>;
-- 
2.43.0

[PATCH v7 2/2] PCI: amd-mdb: Add support for PCIe RP PERST# signal handling

[Sai Krishna Musham]

Add support for handling the AMD Versal Gen 2 MDB PCIe Root Port PERST#
signal via a GPIO by parsing the new PCIe bridge node to acquire the
reset GPIO. If the bridge node is not found, fall back to acquiring it
from the PCIe host bridge node.

As part of this, update the interrupt controller node parsing to use
of_get_child_by_name() instead of of_get_next_child(), since the PCIe
host bridge node now has multiple children. This ensures the correct
node is selected during initialization.

Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
---
Changes in v7:
- Use for_each_child_of_node_with_prefix() to iterate through PCIe
  Bridge nodes.

Changes in v6:
- Simplified error checking condition logic.
- Removed unnecessary fallback message.

Changes in v5:
- Add fall back mechanism to acquire reset GPIO from PCIe node when PCIe Bridge
node is not present.

Changes in v4:
- Resolve kernel test robot warning.
https://lore.kernel.org/oe-kbuild-all/202506241020.rPD1a2Vr-lkp@intel.com/
- Update commit message.

Changes in v3:
- Implement amd_mdb_parse_pcie_port to parse bridge node for reset-gpios property.

Changes in v2:
- Change delay to PCIE_T_PVPERL_MS

v6 https://lore.kernel.org/all/20250719030951.3616385-1-sai.krishna.musham@amd.com/
v5 https://lore.kernel.org/all/20250711052357.3859719-1-sai.krishna.musham@amd.com/
v4 https://lore.kernel.org/all/20250626054906.3277029-1-sai.krishna.musham@amd.com/
v3 https://lore.kernel.org/r/20250618080931.2472366-1-sai.krishna.musham@amd.com/
v2 https://lore.kernel.org/r/20250429090046.1512000-1-sai.krishna.musham@amd.com/
v1 https://lore.kernel.org/r/20250326041507.98232-1-sai.krishna.musham@amd.com/
---
 drivers/pci/controller/dwc/pcie-amd-mdb.c | 52 ++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-amd-mdb.c b/drivers/pci/controller/dwc/pcie-amd-mdb.c
index 9f7251a16d32..3c6e837465bb 100644
--- a/drivers/pci/controller/dwc/pcie-amd-mdb.c
+++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
@@ -18,6 +18,7 @@
 #include <linux/resource.h>
 #include <linux/types.h>
 
+#include "../../pci.h"
 #include "pcie-designware.h"
 
 #define AMD_MDB_TLP_IR_STATUS_MISC		0x4C0
@@ -56,6 +57,7 @@
  * @slcr: MDB System Level Control and Status Register (SLCR) base
  * @intx_domain: INTx IRQ domain pointer
  * @mdb_domain: MDB IRQ domain pointer
+ * @perst_gpio: GPIO descriptor for PERST# signal handling
  * @intx_irq: INTx IRQ interrupt number
  */
 struct amd_mdb_pcie {
@@ -63,6 +65,7 @@ struct amd_mdb_pcie {
 	void __iomem			*slcr;
 	struct irq_domain		*intx_domain;
 	struct irq_domain		*mdb_domain;
+	struct gpio_desc		*perst_gpio;
 	int				intx_irq;
 };
 
@@ -284,7 +287,7 @@ static int amd_mdb_pcie_init_irq_domains(struct amd_mdb_pcie *pcie,
 	struct device_node *pcie_intc_node;
 	int err;
 
-	pcie_intc_node = of_get_next_child(node, NULL);
+	pcie_intc_node = of_get_child_by_name(node, "interrupt-controller");
 	if (!pcie_intc_node) {
 		dev_err(dev, "No PCIe Intc node found\n");
 		return -ENODEV;
@@ -402,6 +405,28 @@ static int amd_mdb_setup_irq(struct amd_mdb_pcie *pcie,
 	return 0;
 }
 
+static int amd_mdb_parse_pcie_port(struct amd_mdb_pcie *pcie)
+{
+	struct device *dev = pcie->pci.dev;
+	struct device_node *pcie_port_node __maybe_unused;
+
+	/*
+	 * This platform currently supports only one Root Port, so the loop
+	 * will execute only once.
+	 * TODO: Enhance the driver to handle multiple Root Ports in the future.
+	 */
+	for_each_child_of_node_with_prefix(dev->of_node, pcie_port_node, "pcie") {
+		pcie->perst_gpio = devm_fwnode_gpiod_get(dev, of_fwnode_handle(pcie_port_node),
+							 "reset", GPIOD_OUT_HIGH, NULL);
+		if (IS_ERR(pcie->perst_gpio))
+			return dev_err_probe(dev, PTR_ERR(pcie->perst_gpio),
+					     "Failed to request reset GPIO\n");
+		return 0;
+	}
+
+	return -ENODEV;
+}
+
 static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie,
 				 struct platform_device *pdev)
 {
@@ -426,6 +451,12 @@ static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie,
 
 	pp->ops = &amd_mdb_pcie_host_ops;
 
+	if (pcie->perst_gpio) {
+		mdelay(PCIE_T_PVPERL_MS);
+		gpiod_set_value_cansleep(pcie->perst_gpio, 0);
+		mdelay(PCIE_RESET_CONFIG_WAIT_MS);
+	}
+
 	err = dw_pcie_host_init(pp);
 	if (err) {
 		dev_err(dev, "Failed to initialize host, err=%d\n", err);
@@ -444,6 +475,7 @@ static int amd_mdb_pcie_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct amd_mdb_pcie *pcie;
 	struct dw_pcie *pci;
+	int ret;
 
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
 	if (!pcie)
@@ -454,6 +486,24 @@ static int amd_mdb_pcie_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pcie);
 
+	ret = amd_mdb_parse_pcie_port(pcie);
+	/*
+	 * If amd_mdb_parse_pcie_port returns -ENODEV, it indicates that the
+	 * PCIe Bridge node was not found in the device tree. This is not
+	 * considered a fatal error and will trigger a fallback where the
+	 * reset GPIO is acquired directly from the PCIe Host Bridge node.
+	 */
+	if (ret) {
+		if (ret != -ENODEV)
+			return ret;
+
+		pcie->perst_gpio = devm_gpiod_get_optional(dev, "reset",
+							   GPIOD_OUT_HIGH);
+		if (IS_ERR(pcie->perst_gpio))
+			return dev_err_probe(dev, PTR_ERR(pcie->perst_gpio),
+					     "Failed to request reset GPIO\n");
+	}
+
 	return amd_mdb_add_pcie_port(pcie, pdev);
 }
 
-- 
2.43.0

Re: [PATCH v7 0/2] Add support for AMD Versal Gen 2 MDB PCIe RP PERST#

[Manivannan Sadhasivam]

On Thu, 07 Aug 2025 13:10:17 +0530, Sai Krishna Musham wrote:
> Add example usage of reset-gpios for PCIe RP PERST#
> 
> Add support for PCIe Root Port PERST# signal handling
> 
> Sai Krishna Musham (2):
>   dt-bindings: PCI: amd-mdb: Add example usage of reset-gpios for PCIe
>     RP PERST#
>   PCI: amd-mdb: Add support for PCIe RP PERST# signal handling
> 
> [...]

Applied, thanks!

[1/2] dt-bindings: PCI: amd-mdb: Add example usage of reset-gpios for PCIe RP PERST#
      commit: 0b9275edc3543d0d2d03313a7c8de5157d61b189
[2/2] PCI: amd-mdb: Add support for PCIe RP PERST# signal handling
      commit: 1d0156c8b230ca74272708a3206684e6d6157302

Best regards,
-- 
Manivannan Sadhasivam <mani@kernel.org>

RE: [PATCH v7 0/2] Add support for AMD Versal Gen 2 MDB PCIe RP PERST#

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> Sent: Monday, August 11, 2025 4:13 PM
> To: bhelgaas@google.com; lpieralisi@kernel.org; robh@kernel.org;
> krzk+dt@kernel.org; conor+dt@kernel.org; cassel@kernel.org; Krzysztof Wilczyński
> <kwilczynski@kernel.org>; Musham, Sai Krishna <sai.krishna.musham@amd.com>
> Cc: linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>; Gogada, Bharat
> Kumar <bharat.kumar.gogada@amd.com>; Havalige, Thippeswamy
> <thippeswamy.havalige@amd.com>
> Subject: Re: [PATCH v7 0/2] Add support for AMD Versal Gen 2 MDB PCIe RP
> PERST#
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Thu, 07 Aug 2025 13:10:17 +0530, Sai Krishna Musham wrote:
> > Add example usage of reset-gpios for PCIe RP PERST#
> >
> > Add support for PCIe Root Port PERST# signal handling
> >
> > Sai Krishna Musham (2):
> >   dt-bindings: PCI: amd-mdb: Add example usage of reset-gpios for PCIe
> >     RP PERST#
> >   PCI: amd-mdb: Add support for PCIe RP PERST# signal handling
> >
> > [...]
>
> Applied, thanks!
>
> [1/2] dt-bindings: PCI: amd-mdb: Add example usage of reset-gpios for PCIe RP
> PERST#
>       commit: 0b9275edc3543d0d2d03313a7c8de5157d61b189
> [2/2] PCI: amd-mdb: Add support for PCIe RP PERST# signal handling
>       commit: 1d0156c8b230ca74272708a3206684e6d6157302
>

This was my first patch—I really appreciate the guidance and support from everyone. Thank you Manivannan for applying the patch.

Best Regards,
Sai Krishna

> Best regards,
> --
> Manivannan Sadhasivam <mani@kernel.org>

Re: [PATCH v7 2/2] PCI: amd-mdb: Add support for PCIe RP PERST# signal handling

[Bjorn Helgaas]

On Thu, Aug 07, 2025 at 01:10:19PM +0530, Sai Krishna Musham wrote:
> Add support for handling the AMD Versal Gen 2 MDB PCIe Root Port PERST#
> signal via a GPIO by parsing the new PCIe bridge node to acquire the
> reset GPIO. If the bridge node is not found, fall back to acquiring it
> from the PCIe host bridge node.
> 
> As part of this, update the interrupt controller node parsing to use
> of_get_child_by_name() instead of of_get_next_child(), since the PCIe
> host bridge node now has multiple children. This ensures the correct
> node is selected during initialization.
> 
> Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> ---
> Changes in v7:
> - Use for_each_child_of_node_with_prefix() to iterate through PCIe
>   Bridge nodes.
> 
> Changes in v6:
> - Simplified error checking condition logic.
> - Removed unnecessary fallback message.
> 
> Changes in v5:
> - Add fall back mechanism to acquire reset GPIO from PCIe node when PCIe Bridge
> node is not present.
> 
> Changes in v4:
> - Resolve kernel test robot warning.
> https://lore.kernel.org/oe-kbuild-all/202506241020.rPD1a2Vr-lkp@intel.com/
> - Update commit message.
> 
> Changes in v3:
> - Implement amd_mdb_parse_pcie_port to parse bridge node for reset-gpios property.
> 
> Changes in v2:
> - Change delay to PCIE_T_PVPERL_MS
> 
> v6 https://lore.kernel.org/all/20250719030951.3616385-1-sai.krishna.musham@amd.com/
> v5 https://lore.kernel.org/all/20250711052357.3859719-1-sai.krishna.musham@amd.com/
> v4 https://lore.kernel.org/all/20250626054906.3277029-1-sai.krishna.musham@amd.com/
> v3 https://lore.kernel.org/r/20250618080931.2472366-1-sai.krishna.musham@amd.com/
> v2 https://lore.kernel.org/r/20250429090046.1512000-1-sai.krishna.musham@amd.com/
> v1 https://lore.kernel.org/r/20250326041507.98232-1-sai.krishna.musham@amd.com/
> ---
>  drivers/pci/controller/dwc/pcie-amd-mdb.c | 52 ++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-amd-mdb.c b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> index 9f7251a16d32..3c6e837465bb 100644
> --- a/drivers/pci/controller/dwc/pcie-amd-mdb.c
> +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> @@ -18,6 +18,7 @@
>  #include <linux/resource.h>
>  #include <linux/types.h>
>  
> +#include "../../pci.h"
>  #include "pcie-designware.h"
>  
>  #define AMD_MDB_TLP_IR_STATUS_MISC		0x4C0
> @@ -56,6 +57,7 @@
>   * @slcr: MDB System Level Control and Status Register (SLCR) base
>   * @intx_domain: INTx IRQ domain pointer
>   * @mdb_domain: MDB IRQ domain pointer
> + * @perst_gpio: GPIO descriptor for PERST# signal handling
>   * @intx_irq: INTx IRQ interrupt number
>   */
>  struct amd_mdb_pcie {
> @@ -63,6 +65,7 @@ struct amd_mdb_pcie {
>  	void __iomem			*slcr;
>  	struct irq_domain		*intx_domain;
>  	struct irq_domain		*mdb_domain;
> +	struct gpio_desc		*perst_gpio;
>  	int				intx_irq;
>  };
>  
> @@ -284,7 +287,7 @@ static int amd_mdb_pcie_init_irq_domains(struct amd_mdb_pcie *pcie,
>  	struct device_node *pcie_intc_node;
>  	int err;
>  
> -	pcie_intc_node = of_get_next_child(node, NULL);
> +	pcie_intc_node = of_get_child_by_name(node, "interrupt-controller");
>  	if (!pcie_intc_node) {
>  		dev_err(dev, "No PCIe Intc node found\n");
>  		return -ENODEV;
> @@ -402,6 +405,28 @@ static int amd_mdb_setup_irq(struct amd_mdb_pcie *pcie,
>  	return 0;
>  }
>  
> +static int amd_mdb_parse_pcie_port(struct amd_mdb_pcie *pcie)
> +{
> +	struct device *dev = pcie->pci.dev;
> +	struct device_node *pcie_port_node __maybe_unused;
> +
> +	/*
> +	 * This platform currently supports only one Root Port, so the loop
> +	 * will execute only once.
> +	 * TODO: Enhance the driver to handle multiple Root Ports in the future.
> +	 */
> +	for_each_child_of_node_with_prefix(dev->of_node, pcie_port_node, "pcie") {

This is only the second user of for_each_child_of_node_with_prefix()
in the whole tree.  Also the only use of "__maybe_unused" in
drivers/pci/controller/.

Most of the PCI controller drivers use
for_each_available_child_of_node_scoped(); can we do the same here?

The apple, kirin, mt7621, mtk, and qcom drivers are examples.  I think
the qcom structure is pretty good, and it has a similar fallback path
for DTs without Root Port nodes (qcom_pcie_parse_legacy_binding()):

  qcom_pcie_probe
    ret = qcom_pcie_parse_ports
      for_each_available_child_of_node_scoped(dev->of_node, of_port)
        qcom_pcie_parse_port(of_port)
          reset = devm_fwnode_gpiod_get(..., "reset", ...)
    if (ret)
      qcom_pcie_parse_legacy_binding

IIUC the current amd-mdb hardware only supports a single Root Port, so
I don't think you need a TODO, since there's no point in that
enhancement until hardware supports multiple RPs.

But I probably *would* add a check here so that if we run the current
driver on future hardware that does have multiple Root Ports with
separate resets for each RP, there's at least a chance that the first
RP will work.  E.g.,

  amd_mdb_parse_pcie_port(...)
  {
    if (pcie->perst_gpio) {
      dev_warn("Ignoring extra Root Port\n");
      return 0;
    }

    pcie->perst_gpio = devm_fwnode_gpiod_get(...);

> +		pcie->perst_gpio = devm_fwnode_gpiod_get(dev, of_fwnode_handle(pcie_port_node),
> +							 "reset", GPIOD_OUT_HIGH, NULL);
> +		if (IS_ERR(pcie->perst_gpio))
> +			return dev_err_probe(dev, PTR_ERR(pcie->perst_gpio),
> +					     "Failed to request reset GPIO\n");
> +		return 0;
> +	}
> +
> +	return -ENODEV;
> +}
> +
>  static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie,
>  				 struct platform_device *pdev)
>  {
> @@ -426,6 +451,12 @@ static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie,
>  
>  	pp->ops = &amd_mdb_pcie_host_ops;
>  
> +	if (pcie->perst_gpio) {
> +		mdelay(PCIE_T_PVPERL_MS);
> +		gpiod_set_value_cansleep(pcie->perst_gpio, 0);
> +		mdelay(PCIE_RESET_CONFIG_WAIT_MS);
> +	}
> +
>  	err = dw_pcie_host_init(pp);
>  	if (err) {
>  		dev_err(dev, "Failed to initialize host, err=%d\n", err);
> @@ -444,6 +475,7 @@ static int amd_mdb_pcie_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct amd_mdb_pcie *pcie;
>  	struct dw_pcie *pci;
> +	int ret;
>  
>  	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>  	if (!pcie)
> @@ -454,6 +486,24 @@ static int amd_mdb_pcie_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, pcie);
>  
> +	ret = amd_mdb_parse_pcie_port(pcie);
> +	/*
> +	 * If amd_mdb_parse_pcie_port returns -ENODEV, it indicates that the
> +	 * PCIe Bridge node was not found in the device tree. This is not
> +	 * considered a fatal error and will trigger a fallback where the
> +	 * reset GPIO is acquired directly from the PCIe Host Bridge node.
> +	 */
> +	if (ret) {
> +		if (ret != -ENODEV)
> +			return ret;
> +
> +		pcie->perst_gpio = devm_gpiod_get_optional(dev, "reset",
> +							   GPIOD_OUT_HIGH);
> +		if (IS_ERR(pcie->perst_gpio))
> +			return dev_err_probe(dev, PTR_ERR(pcie->perst_gpio),
> +					     "Failed to request reset GPIO\n");
> +	}
> +
>  	return amd_mdb_add_pcie_port(pcie, pdev);
>  }
>  
> -- 
> 2.43.0
> 

Re: [PATCH v7 2/2] PCI: amd-mdb: Add support for PCIe RP PERST# signal handling

[Bjorn Helgaas]

On Tue, Aug 12, 2025 at 02:41:15PM -0500, Bjorn Helgaas wrote:
> On Thu, Aug 07, 2025 at 01:10:19PM +0530, Sai Krishna Musham wrote:
> > Add support for handling the AMD Versal Gen 2 MDB PCIe Root Port PERST#
> > signal via a GPIO by parsing the new PCIe bridge node to acquire the
> > reset GPIO. If the bridge node is not found, fall back to acquiring it
> > from the PCIe host bridge node.
> > 
> > As part of this, update the interrupt controller node parsing to use
> > of_get_child_by_name() instead of of_get_next_child(), since the PCIe
> > host bridge node now has multiple children. This ensures the correct
> > node is selected during initialization.
> ...

> >   * @slcr: MDB System Level Control and Status Register (SLCR) base
> >   * @intx_domain: INTx IRQ domain pointer
> >   * @mdb_domain: MDB IRQ domain pointer
> > + * @perst_gpio: GPIO descriptor for PERST# signal handling
> >   * @intx_irq: INTx IRQ interrupt number
> >   */
> >  struct amd_mdb_pcie {
> > @@ -63,6 +65,7 @@ struct amd_mdb_pcie {
> >  	void __iomem			*slcr;
> >  	struct irq_domain		*intx_domain;
> >  	struct irq_domain		*mdb_domain;
> > +	struct gpio_desc		*perst_gpio;
> >  	int				intx_irq;
> >  };
> >  
> > @@ -284,7 +287,7 @@ static int amd_mdb_pcie_init_irq_domains(struct amd_mdb_pcie *pcie,
> >  	struct device_node *pcie_intc_node;
> >  	int err;
> >  
> > -	pcie_intc_node = of_get_next_child(node, NULL);
> > +	pcie_intc_node = of_get_child_by_name(node, "interrupt-controller");
> >  	if (!pcie_intc_node) {
> >  		dev_err(dev, "No PCIe Intc node found\n");
> >  		return -ENODEV;
> > @@ -402,6 +405,28 @@ static int amd_mdb_setup_irq(struct amd_mdb_pcie *pcie,
> >  	return 0;
> >  }
> >  
> > +static int amd_mdb_parse_pcie_port(struct amd_mdb_pcie *pcie)
> > +{
> > +	struct device *dev = pcie->pci.dev;
> > +	struct device_node *pcie_port_node __maybe_unused;
> > +
> > +	/*
> > +	 * This platform currently supports only one Root Port, so the loop
> > +	 * will execute only once.
> > +	 * TODO: Enhance the driver to handle multiple Root Ports in the future.
> > +	 */
> > +	for_each_child_of_node_with_prefix(dev->of_node, pcie_port_node, "pcie") {
> 
> This is only the second user of for_each_child_of_node_with_prefix()
> in the whole tree.  Also the only use of "__maybe_unused" in
> drivers/pci/controller/.
> 
> Most of the PCI controller drivers use
> for_each_available_child_of_node_scoped(); can we do the same here?

I suppose you used for_each_child_of_node_with_prefix() because, as
you mentioned in the commit log, there may be multiple children
(interrupt controller and Root Port(s))?

I think of_irq_parse_and_map_pci() takes care of much of the INTx
setup based on interrupt-map, so many drivers don't contain any
explicit INTx stuff.  But amd-mdb must be an exception for some
reason.

Most bindings don't include an interrupt-controller child in the pcie@
stanza.  I don't know enough about device tree to understand why
amd-mdb needs it.

Bjorn

RE: [PATCH v7 2/2] PCI: amd-mdb: Add support for PCIe RP PERST# signal handling

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, August 13, 2025 1:11 AM
> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>
> Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com; mani@kernel.org;
> robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; cassel@kernel.org;
> linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>; Gogada, Bharat
> Kumar <bharat.kumar.gogada@amd.com>; Havalige, Thippeswamy
> <thippeswamy.havalige@amd.com>
> Subject: Re: [PATCH v7 2/2] PCI: amd-mdb: Add support for PCIe RP PERST#
> signal handling
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Thu, Aug 07, 2025 at 01:10:19PM +0530, Sai Krishna Musham wrote:
> > Add support for handling the AMD Versal Gen 2 MDB PCIe Root Port PERST#
> > signal via a GPIO by parsing the new PCIe bridge node to acquire the
> > reset GPIO. If the bridge node is not found, fall back to acquiring it
> > from the PCIe host bridge node.
> >
> > As part of this, update the interrupt controller node parsing to use
> > of_get_child_by_name() instead of of_get_next_child(), since the PCIe
> > host bridge node now has multiple children. This ensures the correct
> > node is selected during initialization.
> >
> > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> > ---
> > Changes in v7:
> > - Use for_each_child_of_node_with_prefix() to iterate through PCIe
> >   Bridge nodes.
> >
> > Changes in v6:
> > - Simplified error checking condition logic.
> > - Removed unnecessary fallback message.
> >
> > Changes in v5:
> > - Add fall back mechanism to acquire reset GPIO from PCIe node when PCIe
> Bridge
> > node is not present.
> >
> > Changes in v4:
> > - Resolve kernel test robot warning.
> > https://lore.kernel.org/oe-kbuild-all/202506241020.rPD1a2Vr-lkp@intel.com/
> > - Update commit message.
> >
> > Changes in v3:
> > - Implement amd_mdb_parse_pcie_port to parse bridge node for reset-gpios
> property.
> >
> > Changes in v2:
> > - Change delay to PCIE_T_PVPERL_MS
> >
> > v6 https://lore.kernel.org/all/20250719030951.3616385-1-
> sai.krishna.musham@amd.com/
> > v5 https://lore.kernel.org/all/20250711052357.3859719-1-
> sai.krishna.musham@amd.com/
> > v4 https://lore.kernel.org/all/20250626054906.3277029-1-
> sai.krishna.musham@amd.com/
> > v3 https://lore.kernel.org/r/20250618080931.2472366-1-
> sai.krishna.musham@amd.com/
> > v2 https://lore.kernel.org/r/20250429090046.1512000-1-
> sai.krishna.musham@amd.com/
> > v1 https://lore.kernel.org/r/20250326041507.98232-1-
> sai.krishna.musham@amd.com/
> > ---
> >  drivers/pci/controller/dwc/pcie-amd-mdb.c | 52 ++++++++++++++++++++++-
> >  1 file changed, 51 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-amd-mdb.c
> b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> > index 9f7251a16d32..3c6e837465bb 100644
> > --- a/drivers/pci/controller/dwc/pcie-amd-mdb.c
> > +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/resource.h>
> >  #include <linux/types.h>
> >
> > +#include "../../pci.h"
> >  #include "pcie-designware.h"
> >
> >  #define AMD_MDB_TLP_IR_STATUS_MISC           0x4C0
> > @@ -56,6 +57,7 @@
> >   * @slcr: MDB System Level Control and Status Register (SLCR) base
> >   * @intx_domain: INTx IRQ domain pointer
> >   * @mdb_domain: MDB IRQ domain pointer
> > + * @perst_gpio: GPIO descriptor for PERST# signal handling
> >   * @intx_irq: INTx IRQ interrupt number
> >   */
> >  struct amd_mdb_pcie {
> > @@ -63,6 +65,7 @@ struct amd_mdb_pcie {
> >       void __iomem                    *slcr;
> >       struct irq_domain               *intx_domain;
> >       struct irq_domain               *mdb_domain;
> > +     struct gpio_desc                *perst_gpio;
> >       int                             intx_irq;
> >  };
> >
> > @@ -284,7 +287,7 @@ static int amd_mdb_pcie_init_irq_domains(struct
> amd_mdb_pcie *pcie,
> >       struct device_node *pcie_intc_node;
> >       int err;
> >
> > -     pcie_intc_node = of_get_next_child(node, NULL);
> > +     pcie_intc_node = of_get_child_by_name(node, "interrupt-controller");
> >       if (!pcie_intc_node) {
> >               dev_err(dev, "No PCIe Intc node found\n");
> >               return -ENODEV;
> > @@ -402,6 +405,28 @@ static int amd_mdb_setup_irq(struct amd_mdb_pcie
> *pcie,
> >       return 0;
> >  }
> >
> > +static int amd_mdb_parse_pcie_port(struct amd_mdb_pcie *pcie)
> > +{
> > +     struct device *dev = pcie->pci.dev;
> > +     struct device_node *pcie_port_node __maybe_unused;
> > +
> > +     /*
> > +      * This platform currently supports only one Root Port, so the loop
> > +      * will execute only once.
> > +      * TODO: Enhance the driver to handle multiple Root Ports in the future.
> > +      */
> > +     for_each_child_of_node_with_prefix(dev->of_node, pcie_port_node, "pcie") {
>
> This is only the second user of for_each_child_of_node_with_prefix()
> in the whole tree.  Also the only use of "__maybe_unused" in
> drivers/pci/controller/.
>
> Most of the PCI controller drivers use
> for_each_available_child_of_node_scoped(); can we do the same here?
>
> The apple, kirin, mt7621, mtk, and qcom drivers are examples.  I think
> the qcom structure is pretty good, and it has a similar fallback path
> for DTs without Root Port nodes (qcom_pcie_parse_legacy_binding()):
>
>   qcom_pcie_probe
>     ret = qcom_pcie_parse_ports
>       for_each_available_child_of_node_scoped(dev->of_node, of_port)
>         qcom_pcie_parse_port(of_port)
>           reset = devm_fwnode_gpiod_get(..., "reset", ...)
>     if (ret)
>       qcom_pcie_parse_legacy_binding
>
> IIUC the current amd-mdb hardware only supports a single Root Port, so
> I don't think you need a TODO, since there's no point in that
> enhancement until hardware supports multiple RPs.
>
> But I probably *would* add a check here so that if we run the current
> driver on future hardware that does have multiple Root Ports with
> separate resets for each RP, there's at least a chance that the first
> RP will work.  E.g.,
>
>   amd_mdb_parse_pcie_port(...)
>   {
>     if (pcie->perst_gpio) {
>       dev_warn("Ignoring extra Root Port\n");
>       return 0;
>     }
>
>     pcie->perst_gpio = devm_fwnode_gpiod_get(...);
>

Thanks for the feedback. I'll switch to using for_each_available_child_of_node_scoped()
and send it as a separate patch.

> > +             pcie->perst_gpio = devm_fwnode_gpiod_get(dev,
> of_fwnode_handle(pcie_port_node),
> > +                                                      "reset", GPIOD_OUT_HIGH, NULL);
> > +             if (IS_ERR(pcie->perst_gpio))
> > +                     return dev_err_probe(dev, PTR_ERR(pcie->perst_gpio),
> > +                                          "Failed to request reset GPIO\n");
> > +             return 0;
> > +     }
> > +
> > +     return -ENODEV;
> > +}
> > +
> >  static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie,
> >                                struct platform_device *pdev)
> >  {
> > @@ -426,6 +451,12 @@ static int amd_mdb_add_pcie_port(struct
> amd_mdb_pcie *pcie,
> >
> >       pp->ops = &amd_mdb_pcie_host_ops;
> >
> > +     if (pcie->perst_gpio) {
> > +             mdelay(PCIE_T_PVPERL_MS);
> > +             gpiod_set_value_cansleep(pcie->perst_gpio, 0);
> > +             mdelay(PCIE_RESET_CONFIG_WAIT_MS);
> > +     }
> > +
> >       err = dw_pcie_host_init(pp);
> >       if (err) {
> >               dev_err(dev, "Failed to initialize host, err=%d\n", err);
> > @@ -444,6 +475,7 @@ static int amd_mdb_pcie_probe(struct platform_device
> *pdev)
> >       struct device *dev = &pdev->dev;
> >       struct amd_mdb_pcie *pcie;
> >       struct dw_pcie *pci;
> > +     int ret;
> >
> >       pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> >       if (!pcie)
> > @@ -454,6 +486,24 @@ static int amd_mdb_pcie_probe(struct platform_device
> *pdev)
> >
> >       platform_set_drvdata(pdev, pcie);
> >
> > +     ret = amd_mdb_parse_pcie_port(pcie);
> > +     /*
> > +      * If amd_mdb_parse_pcie_port returns -ENODEV, it indicates that the
> > +      * PCIe Bridge node was not found in the device tree. This is not
> > +      * considered a fatal error and will trigger a fallback where the
> > +      * reset GPIO is acquired directly from the PCIe Host Bridge node.
> > +      */
> > +     if (ret) {
> > +             if (ret != -ENODEV)
> > +                     return ret;
> > +
> > +             pcie->perst_gpio = devm_gpiod_get_optional(dev, "reset",
> > +                                                        GPIOD_OUT_HIGH);
> > +             if (IS_ERR(pcie->perst_gpio))
> > +                     return dev_err_probe(dev, PTR_ERR(pcie->perst_gpio),
> > +                                          "Failed to request reset GPIO\n");
> > +     }
> > +
> >       return amd_mdb_add_pcie_port(pcie, pdev);
> >  }
> >
> > --
> > 2.43.0
> >

RE: [PATCH v7 2/2] PCI: amd-mdb: Add support for PCIe RP PERST# signal handling

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, August 13, 2025 4:31 AM
> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>
> Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com; mani@kernel.org;
> robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; cassel@kernel.org;
> linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>; Gogada, Bharat
> Kumar <bharat.kumar.gogada@amd.com>; Havalige, Thippeswamy
> <thippeswamy.havalige@amd.com>
> Subject: Re: [PATCH v7 2/2] PCI: amd-mdb: Add support for PCIe RP PERST#
> signal handling
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Tue, Aug 12, 2025 at 02:41:15PM -0500, Bjorn Helgaas wrote:
> > On Thu, Aug 07, 2025 at 01:10:19PM +0530, Sai Krishna Musham wrote:
> > > Add support for handling the AMD Versal Gen 2 MDB PCIe Root Port PERST#
> > > signal via a GPIO by parsing the new PCIe bridge node to acquire the
> > > reset GPIO. If the bridge node is not found, fall back to acquiring it
> > > from the PCIe host bridge node.
> > >
> > > As part of this, update the interrupt controller node parsing to use
> > > of_get_child_by_name() instead of of_get_next_child(), since the PCIe
> > > host bridge node now has multiple children. This ensures the correct
> > > node is selected during initialization.
> > ...
>
> > >   * @slcr: MDB System Level Control and Status Register (SLCR) base
> > >   * @intx_domain: INTx IRQ domain pointer
> > >   * @mdb_domain: MDB IRQ domain pointer
> > > + * @perst_gpio: GPIO descriptor for PERST# signal handling
> > >   * @intx_irq: INTx IRQ interrupt number
> > >   */
> > >  struct amd_mdb_pcie {
> > > @@ -63,6 +65,7 @@ struct amd_mdb_pcie {
> > >     void __iomem                    *slcr;
> > >     struct irq_domain               *intx_domain;
> > >     struct irq_domain               *mdb_domain;
> > > +   struct gpio_desc                *perst_gpio;
> > >     int                             intx_irq;
> > >  };
> > >
> > > @@ -284,7 +287,7 @@ static int amd_mdb_pcie_init_irq_domains(struct
> amd_mdb_pcie *pcie,
> > >     struct device_node *pcie_intc_node;
> > >     int err;
> > >
> > > -   pcie_intc_node = of_get_next_child(node, NULL);
> > > +   pcie_intc_node = of_get_child_by_name(node, "interrupt-controller");
> > >     if (!pcie_intc_node) {
> > >             dev_err(dev, "No PCIe Intc node found\n");
> > >             return -ENODEV;
> > > @@ -402,6 +405,28 @@ static int amd_mdb_setup_irq(struct amd_mdb_pcie
> *pcie,
> > >     return 0;
> > >  }
> > >
> > > +static int amd_mdb_parse_pcie_port(struct amd_mdb_pcie *pcie)
> > > +{
> > > +   struct device *dev = pcie->pci.dev;
> > > +   struct device_node *pcie_port_node __maybe_unused;
> > > +
> > > +   /*
> > > +    * This platform currently supports only one Root Port, so the loop
> > > +    * will execute only once.
> > > +    * TODO: Enhance the driver to handle multiple Root Ports in the future.
> > > +    */
> > > +   for_each_child_of_node_with_prefix(dev->of_node, pcie_port_node, "pcie") {
> >
> > This is only the second user of for_each_child_of_node_with_prefix()
> > in the whole tree.  Also the only use of "__maybe_unused" in
> > drivers/pci/controller/.
> >
> > Most of the PCI controller drivers use
> > for_each_available_child_of_node_scoped(); can we do the same here?
>
> I suppose you used for_each_child_of_node_with_prefix() because, as
> you mentioned in the commit log, there may be multiple children
> (interrupt controller and Root Port(s))?
>
> I think of_irq_parse_and_map_pci() takes care of much of the INTx
> setup based on interrupt-map, so many drivers don't contain any
> explicit INTx stuff.  But amd-mdb must be an exception for some
> reason.
>
> Most bindings don't include an interrupt-controller child in the pcie@
> stanza.  I don't know enough about device tree to understand why
> amd-mdb needs it.
>

Thanks Bjorn for pointing out the of_irq_parse_and_map_pci() API. I'll update the driver
to use it for INTx setup based on the interrupt-map, and will explore removing the
interrupt-controller node if it's no longer needed.

> Bjorn

Regards,
Sai Krishna