[PATCH v3 0/4] PCI/pwrctrl: Allow pwrctrl framework to control PERST# if available

[PATCH v3 0/4] PCI/pwrctrl: Allow pwrctrl framework to control PERST# if available

[Manivannan Sadhasivam via B4 Relay]

Hi,

This series is the proper version for toggling PERST# from the pwrctrl
framework after the initial RFC posted earlier [1].

Problem statement
=================

Pwrctrl framework is intented to control the supplies to the components on the
PCI bus. However, if the platform supports the PERST# signal, it should be
toggled as per the requirements in the electromechanical specifications like
PCIe CEM, Mini, and M.2. Since the pwrctrl framework is controlling the power
supplies, it should also toggle PERST# as per the requirements in the above
mentioned specifications. Right now, it is just controlling the power to the
components and rely on controller drivers to toggle PERST#, which goes against
the specs. For instance, controller drivers will deassert PERST# even before the
pwrctrl driver enables the supplies. This causes the device to see PERST#
deassert immediately after power on, thereby violating the delay requirements in
the PCI Electromechanical specs.

Proposal
========

To fix this issue, the pwrctrl framework has to control the PERST# signal. But
unfortunately, it is not straightforward. This is mostly due to controller
drivers still need to assert PERST# as a part of their own initialization
sequence. The controller drivers will parse PERST# from the devicetree nodes
even before the pwrctrl drivers get probed. So the PERST# control needs to be
shared between both drivers in a logical manner.

This is achieved by adding a new callback, 'pci_host_bridge::perst_assert'. This
callback if available, will be called by the pwrctrl framework during the power
on and power off scenarios. The callback implementation in the controller driver
has to take care of asserting/deasserting PERST# in an implementation specific
way i.e., if the PERST# signal is a GPIO, then it should be toggled using gpiod
APIs, or if the signal is implemented as a CSR, then the relevant registers
should be written.

Ideally, the PERST# delay requirements should be implemented in the pwrctrl
framework (before/after calling the callback), but some controller drivers
perform some post-link_up operations requiring them to control the delay within
the driver. Those drivers may use this callback to assert/deassert PERST# and
perform post-link_up operations.

For reference, I've implemented the callback in the Qcom RC driver where it just
asserts/deasserts PERST# and handles delay. Since the Qcom driver supports both
legacy DT binding (all Root Port properties in host bridge node) and new binding
(Root Port properies in Root Port node), I've moved the PERST# handling to
pwrctrl driver only if the newly introduced 'qcom_pcie::legacy_binding' flag is
not set. This flag if set, implies that the platform is using the legacy DT
binding, so the controller driver has to control PERST#. 

DT binding requirement
======================

This series has some assumptions on the DT binding. But some of them are not
enforced right now:

1. Pwrctrl driver requires the PCIe device node to have atleast one -supply
property. Those supplies are already documented in the dtschema [2], but they
are optional. So if those supplies are not present, pwrctrl driver will not get
probed. For platforms having a fixed power supply to the components, they should
describe those fixed supplies in DT. Otherwise, they cannot use pwrctrl drivers.
(NOT ENFROCED)

2. Optional PERST# GPIO (reset-gpios property) is only allowed in the bridge
node in the DT binding [3]. So for looking up the PERST# for an endpoint node,
the controller driver has to look up the parent node where PERST# would be
available. (ENFORCED)

3. If shared PERST# is implemented, all the bridge nodes (Root port and switch
downstream) should have the same 'reset-gpios' property. This way, the
controller drivers parsing PERST# could know if it is shared and invoke relevant
gpiod APIs/flags. (NOT ENFORCED)

I don't know how we can make sure DT binding enforces option 1 and 3.

Testing
=======

This series is tested on Qcom X1E based T14s laptop, SM8250 based RB5 board, and
QCS6490 based RB3Gen2 board with DTS specifying Root Port properties in host
bridge node and in Root Port node.

[1] https://lore.kernel.org/linux-pci/20250707-pci-pwrctrl-perst-v1-0-c3c7e513e312@kernel.org/
[2] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-bus-common.yaml#L173
[3] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-bus-common.yaml#L141

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
Changes in v3:
- Dropped the pci_pwrctrl_init() move patch as it is no longer required
- Added a patch to cleanup the usage of 'phy' and 'reset' pointers
- Renamed the callback from 'toggle_perst' to 'perst_assert'
- Reworded the patch descriptions
- Link to v2: https://lore.kernel.org/r/20250903-pci-pwrctrl-perst-v2-0-2d461ed0e061@oss.qualcomm.com

Changes in v2:
- Reworked the PERST# lookup logic to use the node pointer instead of BDF
- Added PWRCTRL guard to the toggle_perst callback
- Link to v1: https://lore.kernel.org/r/20250819-pci-pwrctrl-perst-v1-0-4b74978d2007@oss.qualcomm.com

Changes since RFC:
* Implemented PERST# toggling using a callback since GPIO based PERST# is not
  available on all platforms. This also moves all PERST# handling to the
  controller drivers allowing them to add any additional post-link_up logic.

---
Manivannan Sadhasivam (4):
      PCI/pwrctrl: Add support for asserting/deasserting PERST#
      PCI: qcom: Move host bridge 'phy' and 'reset' pointers to struct qcom_pcie_port
      PCI: qcom: Parse PERST# from all PCIe bridge nodes
      PCI: qcom: Allow pwrctrl core to control PERST# if 'reset-gpios' property is available

 drivers/pci/controller/dwc/pcie-qcom.c | 269 ++++++++++++++++++++++++---------
 drivers/pci/pwrctrl/core.c             |  27 ++++
 include/linux/pci.h                    |   3 +
 3 files changed, 227 insertions(+), 72 deletions(-)
---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250818-pci-pwrctrl-perst-0bb7dd62b542

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

[PATCH v3 1/4] PCI/pwrctrl: Add support for asserting/deasserting PERST#

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

As per PCIe spec r6.0, sec 6.6.1, PERST# is an auxiliary signal provided by
the system to a component as a Fundamental Reset. This signal if available,
should conform to the rules defined by the Electromechanical form factor
specifications like PCIe CEM spec r4.0, sec 2.2.

Since pwrctrl driver is meant to control the power supplies to the PCI
components, it should also control the PERST# signal if available. But
traditionally, the host bridge (controller) drivers are the ones parsing
and controlling the PERST# signal. They also sometimes need to assert
PERST# during their own hardware initialization. So it is not possible to
move the PERST# control away from the controller drivers and it must be
shared logically.

Hence, add a new callback 'pci_host_bridge::perst_assert', that allows the
pwrctrl core to assert/deassert PERST# with the help of the controller
drivers. But care must be taken care by the controller drivers to not
deassert the PERST# signal if this callback is populated.

This callback if available, will be called by the pwrctrl core during the
device power up and power down scenarios. Controller drivers should
identify the device using the 'struct device_node' passed during the
callback and assert/deassert PERST# accordingly.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/core.c | 27 +++++++++++++++++++++++++++
 include/linux/pci.h        |  3 +++
 2 files changed, 30 insertions(+)

diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
index 6bdbfed584d6d79ce28ba9e384a596b065ca69a4..54d3dbc24020e979f668bb294448e8429cd8bdd3 100644
--- a/drivers/pci/pwrctrl/core.c
+++ b/drivers/pci/pwrctrl/core.c
@@ -6,6 +6,7 @@
 #include <linux/device.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
+#include <linux/of.h>
 #include <linux/pci.h>
 #include <linux/pci-pwrctrl.h>
 #include <linux/property.h>
@@ -61,6 +62,28 @@ void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(pci_pwrctrl_init);
 
+static void pci_pwrctrl_perst_deassert(struct pci_pwrctrl *pwrctrl)
+{
+	struct pci_host_bridge *host_bridge = to_pci_host_bridge(pwrctrl->dev->parent);
+	struct device_node *np = dev_of_node(pwrctrl->dev);
+
+	if (!host_bridge->perst_assert)
+		return;
+
+	host_bridge->perst_assert(host_bridge, np, false);
+}
+
+static void pci_pwrctrl_perst_assert(struct pci_pwrctrl *pwrctrl)
+{
+	struct pci_host_bridge *host_bridge = to_pci_host_bridge(pwrctrl->dev->parent);
+	struct device_node *np = dev_of_node(pwrctrl->dev);
+
+	if (!host_bridge->perst_assert)
+		return;
+
+	host_bridge->perst_assert(host_bridge, np, true);
+}
+
 /**
  * pci_pwrctrl_device_set_ready() - Notify the pwrctrl subsystem that the PCI
  * device is powered-up and ready to be detected.
@@ -82,6 +105,8 @@ int pci_pwrctrl_device_set_ready(struct pci_pwrctrl *pwrctrl)
 	if (!pwrctrl->dev)
 		return -ENODEV;
 
+	pci_pwrctrl_perst_deassert(pwrctrl);
+
 	pwrctrl->nb.notifier_call = pci_pwrctrl_notify;
 	ret = bus_register_notifier(&pci_bus_type, &pwrctrl->nb);
 	if (ret)
@@ -103,6 +128,8 @@ void pci_pwrctrl_device_unset_ready(struct pci_pwrctrl *pwrctrl)
 {
 	cancel_work_sync(&pwrctrl->work);
 
+	pci_pwrctrl_perst_assert(pwrctrl);
+
 	/*
 	 * We don't have to delete the link here. Typically, this function
 	 * is only called when the power control device is being detached. If
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 59876de13860dbe50ee6c207cd57e54f51a11079..08007ef244c0a8bc2be27496bea7249ce3e00935 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -605,6 +605,9 @@ struct pci_host_bridge {
 	void (*release_fn)(struct pci_host_bridge *);
 	int (*enable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
 	void (*disable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
+#if IS_ENABLED(CONFIG_PCI_PWRCTRL)
+	void (*perst_assert)(struct pci_host_bridge *bridge, struct device_node *np, bool assert);
+#endif
 	void		*release_data;
 	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
 	unsigned int	no_ext_tags:1;		/* No Extended Tags */

-- 
2.45.2

[PATCH v3 2/4] PCI: qcom: Move host bridge 'phy' and 'reset' pointers to struct qcom_pcie_port

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

DT binding allows specifying 'phy' and 'reset' properties in both host
bridge and Root Port nodes, though specifying in the host bridge node is
marked as deprecated. Still, the pcie-qcom driver should support both
combinations for maintaining the DT backwards compatibility. For this
purpose, the driver is holding the relevant pointers of these properties in
two structs: struct qcom_pcie_port and struct qcom_pcie.

However, this causes confusion and increases the driver complexity. Hence,
move the pointers from struct qcom_pcie to struct qcom_pcie_port. As a
result, even if these properties are specified in the host bridge node,
the pointers will be stored in struct qcom_pcie_port as if the properties
are specified in a single Root Port node. This logic simplifies the driver
a lot.

Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 87 ++++++++++++++--------------------
 1 file changed, 36 insertions(+), 51 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..6170c86f465f43f980f5b2f88bd8799c3c152e68 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -279,8 +279,6 @@ struct qcom_pcie {
 	void __iomem *elbi;			/* DT elbi */
 	void __iomem *mhi;
 	union qcom_pcie_resources res;
-	struct phy *phy;
-	struct gpio_desc *reset;
 	struct icc_path *icc_mem;
 	struct icc_path *icc_cpu;
 	const struct qcom_pcie_cfg *cfg;
@@ -297,11 +295,8 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
 	struct qcom_pcie_port *port;
 	int val = assert ? 1 : 0;
 
-	if (list_empty(&pcie->ports))
-		gpiod_set_value_cansleep(pcie->reset, val);
-	else
-		list_for_each_entry(port, &pcie->ports, list)
-			gpiod_set_value_cansleep(port->reset, val);
+	list_for_each_entry(port, &pcie->ports, list)
+		gpiod_set_value_cansleep(port->reset, val);
 
 	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
 }
@@ -1253,57 +1248,32 @@ static bool qcom_pcie_link_up(struct dw_pcie *pci)
 	return val & PCI_EXP_LNKSTA_DLLLA;
 }
 
-static void qcom_pcie_phy_exit(struct qcom_pcie *pcie)
-{
-	struct qcom_pcie_port *port;
-
-	if (list_empty(&pcie->ports))
-		phy_exit(pcie->phy);
-	else
-		list_for_each_entry(port, &pcie->ports, list)
-			phy_exit(port->phy);
-}
-
 static void qcom_pcie_phy_power_off(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_port *port;
 
-	if (list_empty(&pcie->ports)) {
-		phy_power_off(pcie->phy);
-	} else {
-		list_for_each_entry(port, &pcie->ports, list)
-			phy_power_off(port->phy);
-	}
+	list_for_each_entry(port, &pcie->ports, list)
+		phy_power_off(port->phy);
 }
 
 static int qcom_pcie_phy_power_on(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_port *port;
-	int ret = 0;
+	int ret;
 
-	if (list_empty(&pcie->ports)) {
-		ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
+	list_for_each_entry(port, &pcie->ports, list) {
+		ret = phy_set_mode_ext(port->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
 		if (ret)
 			return ret;
 
-		ret = phy_power_on(pcie->phy);
-		if (ret)
+		ret = phy_power_on(port->phy);
+		if (ret) {
+			qcom_pcie_phy_power_off(pcie);
 			return ret;
-	} else {
-		list_for_each_entry(port, &pcie->ports, list) {
-			ret = phy_set_mode_ext(port->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
-			if (ret)
-				return ret;
-
-			ret = phy_power_on(port->phy);
-			if (ret) {
-				qcom_pcie_phy_power_off(pcie);
-				return ret;
-			}
 		}
 	}
 
-	return ret;
+	return 0;
 }
 
 static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
@@ -1748,8 +1718,10 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 	return ret;
 
 err_port_del:
-	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		phy_exit(port->phy);
 		list_del(&port->list);
+	}
 
 	return ret;
 }
@@ -1757,20 +1729,32 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
 {
 	struct device *dev = pcie->pci->dev;
+	struct qcom_pcie_port *port;
+	struct gpio_desc *reset;
+	struct phy *phy;
 	int ret;
 
-	pcie->phy = devm_phy_optional_get(dev, "pciephy");
-	if (IS_ERR(pcie->phy))
-		return PTR_ERR(pcie->phy);
+	phy = devm_phy_optional_get(dev, "pciephy");
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
 
-	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
-	if (IS_ERR(pcie->reset))
-		return PTR_ERR(pcie->reset);
+	reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset))
+		return PTR_ERR(reset);
 
-	ret = phy_init(pcie->phy);
+	ret = phy_init(phy);
 	if (ret)
 		return ret;
 
+	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	port->reset = reset;
+	port->phy = phy;
+	INIT_LIST_HEAD(&port->list);
+	list_add_tail(&port->list, &pcie->ports);
+
 	return 0;
 }
 
@@ -1984,9 +1968,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 err_host_deinit:
 	dw_pcie_host_deinit(pp);
 err_phy_exit:
-	qcom_pcie_phy_exit(pcie);
-	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		phy_exit(port->phy);
 		list_del(&port->list);
+	}
 err_pm_runtime_put:
 	pm_runtime_put(dev);
 	pm_runtime_disable(dev);

-- 
2.45.2

[PATCH v3 3/4] PCI: qcom: Parse PERST# from all PCIe bridge nodes

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

Devicetree schema allows the PERST# GPIO to be present in all PCIe bridge
nodes, not just in Root Port node. But the current logic parses PERST# only
from the Root Port nodes. Though it is not causing any issue on the current
platforms, the upcoming platforms will have PERST# in PCIe switch
downstream ports also. So this requires parsing all the PCIe bridge nodes
for the PERST# GPIO.

Hence, rework the parsing logic to extend to all PCIe bridge nodes starting
from the Root Port node. If the 'reset-gpios' property is found for a PCI
bridge node, the GPIO descriptor will be stored in qcom_pcie_perst::desc
and added to the qcom_pcie_port::perst list.

It should be noted that if more than one bridge node has the same GPIO for
PERST# (shared PERST#), the driver will error out. This is due to the
limitation in the GPIOLIB subsystem that allows only exclusive (non-shared)
access to GPIOs from consumers. But this is soon going to get fixed. Once
that happens, it will get incorporated in this driver.

So for now, PERST# sharing is not supported.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 102 +++++++++++++++++++++++++++------
 1 file changed, 85 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 6170c86f465f43f980f5b2f88bd8799c3c152e68..ccff01c31898cdbc5634221e7f8ef7e86469f5fd 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -267,10 +267,15 @@ struct qcom_pcie_cfg {
 	bool no_l0s;
 };
 
+struct qcom_pcie_perst {
+	struct list_head list;
+	struct gpio_desc *desc;
+};
+
 struct qcom_pcie_port {
 	struct list_head list;
-	struct gpio_desc *reset;
 	struct phy *phy;
+	struct list_head perst;
 };
 
 struct qcom_pcie {
@@ -292,11 +297,14 @@ struct qcom_pcie {
 
 static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
 {
+	struct qcom_pcie_perst *perst;
 	struct qcom_pcie_port *port;
 	int val = assert ? 1 : 0;
 
-	list_for_each_entry(port, &pcie->ports, list)
-		gpiod_set_value_cansleep(port->reset, val);
+	list_for_each_entry(port, &pcie->ports, list) {
+		list_for_each_entry(perst, &port->perst, list)
+			gpiod_set_value_cansleep(perst->desc, val);
+	}
 
 	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
 }
@@ -1670,18 +1678,58 @@ static const struct pci_ecam_ops pci_qcom_ecam_ops = {
 	}
 };
 
-static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node)
+/* Parse PERST# from all nodes in depth first manner starting from @np */
+static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
+				 struct qcom_pcie_port *port,
+				 struct device_node *np)
 {
 	struct device *dev = pcie->pci->dev;
-	struct qcom_pcie_port *port;
+	struct qcom_pcie_perst *perst;
 	struct gpio_desc *reset;
-	struct phy *phy;
 	int ret;
 
-	reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
-				      "reset", GPIOD_OUT_HIGH, "PERST#");
-	if (IS_ERR(reset))
+	if (!of_find_property(np, "reset-gpios", NULL))
+		goto parse_child_node;
+
+	reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(np), "reset",
+				      GPIOD_OUT_HIGH, "PERST#");
+	if (IS_ERR(reset)) {
+		/*
+		 * FIXME: GPIOLIB currently supports exclusive GPIO access only.
+		 * Non exclusive access is broken. But shared PERST# requires
+		 * non-exclusive access. So once GPIOLIB properly supports it,
+		 * implement it here.
+		 */
+		if (PTR_ERR(reset) == -EBUSY)
+			dev_err(dev, "Shared PERST# is not supported\n");
+
 		return PTR_ERR(reset);
+	}
+
+	perst = devm_kzalloc(dev, sizeof(*perst), GFP_KERNEL);
+	if (!perst)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&perst->list);
+	perst->desc = reset;
+	list_add_tail(&perst->list, &port->perst);
+
+parse_child_node:
+	for_each_available_child_of_node_scoped(np, child) {
+		ret = qcom_pcie_parse_perst(pcie, port, child);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node)
+{
+	struct device *dev = pcie->pci->dev;
+	struct qcom_pcie_port *port;
+	struct phy *phy;
+	int ret;
 
 	phy = devm_of_phy_get(dev, node, NULL);
 	if (IS_ERR(phy))
@@ -1695,7 +1743,12 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
 	if (ret)
 		return ret;
 
-	port->reset = reset;
+	INIT_LIST_HEAD(&port->perst);
+
+	ret = qcom_pcie_parse_perst(pcie, port, node);
+	if (ret)
+		return ret;
+
 	port->phy = phy;
 	INIT_LIST_HEAD(&port->list);
 	list_add_tail(&port->list, &pcie->ports);
@@ -1705,9 +1758,10 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
 
 static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 {
+	struct qcom_pcie_perst *perst, *tmp_perst;
+	struct qcom_pcie_port *port, *tmp_port;
 	struct device *dev = pcie->pci->dev;
-	struct qcom_pcie_port *port, *tmp;
-	int ret = -ENOENT;
+	int ret = -ENODEV;
 
 	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
 		ret = qcom_pcie_parse_port(pcie, of_port);
@@ -1718,7 +1772,9 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 	return ret;
 
 err_port_del:
-	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+	list_for_each_entry_safe(port, tmp_port, &pcie->ports, list) {
+		list_for_each_entry_safe(perst, tmp_perst, &port->perst, list)
+			list_del(&perst->list);
 		phy_exit(port->phy);
 		list_del(&port->list);
 	}
@@ -1729,6 +1785,7 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
 {
 	struct device *dev = pcie->pci->dev;
+	struct qcom_pcie_perst *perst;
 	struct qcom_pcie_port *port;
 	struct gpio_desc *reset;
 	struct phy *phy;
@@ -1750,19 +1807,28 @@ static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
 	if (!port)
 		return -ENOMEM;
 
-	port->reset = reset;
+	perst = devm_kzalloc(dev, sizeof(*perst), GFP_KERNEL);
+	if (!perst)
+		return -ENOMEM;
+
 	port->phy = phy;
 	INIT_LIST_HEAD(&port->list);
 	list_add_tail(&port->list, &pcie->ports);
 
+	perst->desc = reset;
+	INIT_LIST_HEAD(&port->perst);
+	INIT_LIST_HEAD(&perst->list);
+	list_add_tail(&perst->list, &port->perst);
+
 	return 0;
 }
 
 static int qcom_pcie_probe(struct platform_device *pdev)
 {
+	struct qcom_pcie_perst *perst, *tmp_perst;
+	struct qcom_pcie_port *port, *tmp_port;
 	const struct qcom_pcie_cfg *pcie_cfg;
 	unsigned long max_freq = ULONG_MAX;
-	struct qcom_pcie_port *port, *tmp;
 	struct device *dev = &pdev->dev;
 	struct dev_pm_opp *opp;
 	struct qcom_pcie *pcie;
@@ -1909,7 +1975,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	ret = qcom_pcie_parse_ports(pcie);
 	if (ret) {
-		if (ret != -ENOENT) {
+		if (ret != -ENODEV) {
 			dev_err_probe(pci->dev, ret,
 				      "Failed to parse Root Port: %d\n", ret);
 			goto err_pm_runtime_put;
@@ -1968,7 +2034,9 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 err_host_deinit:
 	dw_pcie_host_deinit(pp);
 err_phy_exit:
-	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+	list_for_each_entry_safe(port, tmp_port, &pcie->ports, list) {
+		list_for_each_entry_safe(perst, tmp_perst, &port->perst, list)
+			list_del(&perst->list);
 		phy_exit(port->phy);
 		list_del(&port->list);
 	}

-- 
2.45.2

[PATCH v3 4/4] PCI: qcom: Allow pwrctrl core to control PERST# if 'reset-gpios' property is available

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

For historic reasons, the pcie-qcom driver was controlling the power supply
and PERST# GPIO of the PCIe slot. This turned out to be an issue as the
power supply requirements differ between components. For instance, some of
the WLAN chipsets used in Qualcomm systems were connected to the Root Port
in a non-standard way using their own connectors. This requires specific
power sequencing mechanisms for controlling the WLAN chipsets. So the
pwrctrl framework (CONFIG_PWRCTRL) was introduced to handle these custom
and complex power supply requirements for components.

Sooner, we realized that it would be best to let the pwrctrl driver control
the supplies to the PCIe slots also. As it will allow us to consolidate all
the power supply handling in one place instead of doing it in two. So the
CONFIG_PWRCTRL_SLOT driver was introduced, that just parses the Root Port
nodes representing slots and controls the standard power supplies like
3.3v, 3.3VAux etc...

However, the control of the PERST# GPIOs was still within the controller
drivers like pcie-qcom. So the controller drivers continued to assert/
deassert PERST# GPIOs independent of the power supplies to the components.
This mostly went unnoticed as the components tolerated this non-standard
PERST# assertion/deassertion. But this behavior completely goes against the
PCIe Electromechanical specs like CEM, M.2, as these specs enforce strict
control of PERST# signal together with the power supplies.

So conform to these specs, allow the pwrctrl core to control PERST# for the
slots if the 'reset-gpios' property is specified in the DT bridge nodes.
This is achieved by populating the 'pci_host_bridge::perst_assert' callback
with qcom_pcie_perst_assert() function, so that the pwrctrl core can
control PERST# through this callback.

qcom_pcie_perst_assert() will find the PERST# GPIO descriptor associated
with the supplied 'device_node' of the component and asserts/deasserts
PERST# as requested by the 'assert' parameter. If PERST# is not found in
the supplied node of the component, the function will look for PERST# in
the parent node as a fallback. This is needed since PERST# won't be
available in the endpoint node as per the DT binding.

Note that the driver still asserts PERST# during the controller
initialization as it is needed as per the hardware documentation.

For preserving the backward compatibility with older DTs that still
specifies the Root Port resources in the host bridge DT node, the
controller driver still controls power supplies and PERST# for them. For
those cases, the 'qcom_pcie::legacy_binding' flag will be set and the
driver will continue to control PERST# exclusively. If this flag is not
set, then the pwrctrl driver will control PERST# through the callback.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 94 ++++++++++++++++++++++++++++++----
 1 file changed, 83 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index ccff01c31898cdbc5634221e7f8ef7e86469f5fd..f9a8908d6e5dc3e8dd9ab1c210bfbc5cca1e5be9 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -270,6 +270,7 @@ struct qcom_pcie_cfg {
 struct qcom_pcie_perst {
 	struct list_head list;
 	struct gpio_desc *desc;
+	struct device_node *np;
 };
 
 struct qcom_pcie_port {
@@ -291,34 +292,90 @@ struct qcom_pcie {
 	struct list_head ports;
 	bool suspended;
 	bool use_pm_opp;
+	bool legacy_binding;
 };
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
 
-static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
+static struct gpio_desc *qcom_find_perst(struct qcom_pcie *pcie, struct device_node *np)
+{
+	struct qcom_pcie_perst *perst;
+	struct qcom_pcie_port *port;
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		list_for_each_entry(perst, &port->perst, list)
+			if (np == perst->np)
+				return perst->desc;
+	}
+
+	return NULL;
+}
+
+static void qcom_perst_reset_per_device(struct qcom_pcie *pcie,
+					 struct device_node *np, int val)
+{
+	struct gpio_desc *perst;
+
+	perst = qcom_find_perst(pcie, np);
+	if (perst)
+		goto perst_assert;
+
+	/*
+	 * If PERST# is not available in the current node, try the parent. This
+	 * fallback is needed if the current node belongs to an endpoint or
+	 * switch upstream port.
+	 */
+	if (np->parent)
+		perst = qcom_find_perst(pcie, np->parent);
+
+perst_assert:
+	/* gpiod* APIs handle NULL gpio_desc gracefully. So no need to check. */
+	gpiod_set_value_cansleep(perst, val);
+}
+
+static void qcom_perst_reset(struct qcom_pcie *pcie, struct device_node *np,
+			      bool assert)
 {
 	struct qcom_pcie_perst *perst;
 	struct qcom_pcie_port *port;
 	int val = assert ? 1 : 0;
 
+	if (np) {
+		qcom_perst_reset_per_device(pcie, np, val);
+		goto perst_delay;
+	}
+
 	list_for_each_entry(port, &pcie->ports, list) {
 		list_for_each_entry(perst, &port->perst, list)
 			gpiod_set_value_cansleep(perst->desc, val);
 	}
 
+perst_delay:
 	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
 }
 
-static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
+static void qcom_ep_reset_assert(struct qcom_pcie *pcie, struct device_node *np)
 {
-	qcom_perst_assert(pcie, true);
+	qcom_perst_reset(pcie, np, true);
 }
 
-static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
+static void qcom_ep_reset_deassert(struct qcom_pcie *pcie,
+				   struct device_node *np)
 {
 	/* Ensure that PERST has been asserted for at least 100 ms */
 	msleep(PCIE_T_PVPERL_MS);
-	qcom_perst_assert(pcie, false);
+	qcom_perst_reset(pcie, np, false);
+}
+
+static void qcom_pcie_perst_assert(struct pci_host_bridge *bridge,
+				    struct device_node *np, bool assert)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(bridge->dev.parent);
+
+	if (assert)
+		qcom_ep_reset_assert(pcie, np);
+	else
+		qcom_ep_reset_deassert(pcie, np);
 }
 
 static int qcom_pcie_start_link(struct dw_pcie *pci)
@@ -1290,7 +1347,7 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 	int ret;
 
-	qcom_ep_reset_assert(pcie);
+	qcom_ep_reset_assert(pcie, NULL);
 
 	ret = pcie->cfg->ops->init(pcie);
 	if (ret)
@@ -1306,7 +1363,13 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 			goto err_disable_phy;
 	}
 
-	qcom_ep_reset_deassert(pcie);
+	/*
+	 * Only deassert PERST# for all devices here if legacy binding is used.
+	 * For the new binding, pwrctrl driver is expected to toggle PERST# for
+	 * individual devices.
+	 */
+	if (pcie->legacy_binding)
+		qcom_ep_reset_deassert(pcie, NULL);
 
 	if (pcie->cfg->ops->config_sid) {
 		ret = pcie->cfg->ops->config_sid(pcie);
@@ -1314,10 +1377,12 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 			goto err_assert_reset;
 	}
 
+	pci->pp.bridge->perst_assert = qcom_pcie_perst_assert;
+
 	return 0;
 
 err_assert_reset:
-	qcom_ep_reset_assert(pcie);
+	qcom_ep_reset_assert(pcie, NULL);
 err_disable_phy:
 	qcom_pcie_phy_power_off(pcie);
 err_deinit:
@@ -1331,7 +1396,7 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 
-	qcom_ep_reset_assert(pcie);
+	qcom_ep_reset_assert(pcie, NULL);
 	qcom_pcie_phy_power_off(pcie);
 	pcie->cfg->ops->deinit(pcie);
 }
@@ -1712,6 +1777,9 @@ static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
 
 	INIT_LIST_HEAD(&perst->list);
 	perst->desc = reset;
+	/* Increase the refcount to make sure 'np' is valid till it is stored */
+	of_node_get(np);
+	perst->np = np;
 	list_add_tail(&perst->list, &port->perst);
 
 parse_child_node:
@@ -1773,8 +1841,10 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 
 err_port_del:
 	list_for_each_entry_safe(port, tmp_port, &pcie->ports, list) {
-		list_for_each_entry_safe(perst, tmp_perst, &port->perst, list)
+		list_for_each_entry_safe(perst, tmp_perst, &port->perst, list) {
+			of_node_put(perst->np);
 			list_del(&perst->list);
+		}
 		phy_exit(port->phy);
 		list_del(&port->list);
 	}
@@ -2035,8 +2105,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	dw_pcie_host_deinit(pp);
 err_phy_exit:
 	list_for_each_entry_safe(port, tmp_port, &pcie->ports, list) {
-		list_for_each_entry_safe(perst, tmp_perst, &port->perst, list)
+		list_for_each_entry_safe(perst, tmp_perst, &port->perst, list) {
+			of_node_put(perst->np);
 			list_del(&perst->list);
+		}
 		phy_exit(port->phy);
 		list_del(&port->list);
 	}

-- 
2.45.2

Re: [PATCH v3 2/4] PCI: qcom: Move host bridge 'phy' and 'reset' pointers to struct qcom_pcie_port

[Bjorn Helgaas]

On Fri, Sep 12, 2025 at 02:05:02PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> DT binding allows specifying 'phy' and 'reset' properties in both host
> bridge and Root Port nodes, though specifying in the host bridge node is
> marked as deprecated. Still, the pcie-qcom driver should support both
> combinations for maintaining the DT backwards compatibility. For this
> purpose, the driver is holding the relevant pointers of these properties in
> two structs: struct qcom_pcie_port and struct qcom_pcie.
> 
> However, this causes confusion and increases the driver complexity. Hence,
> move the pointers from struct qcom_pcie to struct qcom_pcie_port. As a
> result, even if these properties are specified in the host bridge node,
> the pointers will be stored in struct qcom_pcie_port as if the properties
> are specified in a single Root Port node. This logic simplifies the driver
> a lot.

> @@ -297,11 +295,8 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
>  	struct qcom_pcie_port *port;
>  	int val = assert ? 1 : 0;
>  
> -	if (list_empty(&pcie->ports))
> -		gpiod_set_value_cansleep(pcie->reset, val);
> -	else
> -		list_for_each_entry(port, &pcie->ports, list)
> -			gpiod_set_value_cansleep(port->reset, val);
> +	list_for_each_entry(port, &pcie->ports, list)
> +		gpiod_set_value_cansleep(port->reset, val);

This is so much nicer, thanks for doing this!

>  static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
>  {
>  	struct device *dev = pcie->pci->dev;
> +	struct qcom_pcie_port *port;
> +	struct gpio_desc *reset;
> +	struct phy *phy;
>  	int ret;
>  
> -	pcie->phy = devm_phy_optional_get(dev, "pciephy");
> -	if (IS_ERR(pcie->phy))
> -		return PTR_ERR(pcie->phy);
> +	phy = devm_phy_optional_get(dev, "pciephy");
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);

Seems like it would be easier to integrate this fallback into
qcom_pcie_parse_port() instead if separating it into
qcom_pcie_parse_legacy_binding().

What if you did something like this in qcom_pcie_parse_port():

  qcom_pcie_parse_port
  {
      reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
				    "reset",  GPIOD_OUT_HIGH, "PERST#");
      if (IS_ERR(reset)) {
	  reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
	  if (IS_ERR(reset))
	      return PTR_ERR(reset);
      }
      ...

Then you could share all the port kzalloc and port list management.

Could do the same with the PHY stuff.

> -	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> -	if (IS_ERR(pcie->reset))
> -		return PTR_ERR(pcie->reset);
> +	reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset))
> +		return PTR_ERR(reset);
>  
> -	ret = phy_init(pcie->phy);
> +	ret = phy_init(phy);
>  	if (ret)
>  		return ret;
>  
> +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->reset = reset;
> +	port->phy = phy;
> +	INIT_LIST_HEAD(&port->list);
> +	list_add_tail(&port->list, &pcie->ports);
> +
>  	return 0;
>  }

Re: [PATCH v3 3/4] PCI: qcom: Parse PERST# from all PCIe bridge nodes

[Bjorn Helgaas]

On Fri, Sep 12, 2025 at 02:05:03PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> Devicetree schema allows the PERST# GPIO to be present in all PCIe bridge
> nodes, not just in Root Port node. But the current logic parses PERST# only
> from the Root Port nodes. Though it is not causing any issue on the current
> platforms, the upcoming platforms will have PERST# in PCIe switch
> downstream ports also. So this requires parsing all the PCIe bridge nodes
> for the PERST# GPIO.
> 
> Hence, rework the parsing logic to extend to all PCIe bridge nodes starting
> from the Root Port node. If the 'reset-gpios' property is found for a PCI
> bridge node, the GPIO descriptor will be stored in qcom_pcie_perst::desc
> and added to the qcom_pcie_port::perst list.

The switch part doesn't seem qcom specific.  Aren't we going to end up
with lots of drivers reimplementing something like the
qcom_pcie_port.perst list?

Re: [PATCH v3 3/4] PCI: qcom: Parse PERST# from all PCIe bridge nodes

[Manivannan Sadhasivam]

On Fri, Sep 12, 2025 at 06:28:11PM GMT, Bjorn Helgaas wrote:
> On Fri, Sep 12, 2025 at 02:05:03PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> > Devicetree schema allows the PERST# GPIO to be present in all PCIe bridge
> > nodes, not just in Root Port node. But the current logic parses PERST# only
> > from the Root Port nodes. Though it is not causing any issue on the current
> > platforms, the upcoming platforms will have PERST# in PCIe switch
> > downstream ports also. So this requires parsing all the PCIe bridge nodes
> > for the PERST# GPIO.
> > 
> > Hence, rework the parsing logic to extend to all PCIe bridge nodes starting
> > from the Root Port node. If the 'reset-gpios' property is found for a PCI
> > bridge node, the GPIO descriptor will be stored in qcom_pcie_perst::desc
> > and added to the qcom_pcie_port::perst list.
> 
> The switch part doesn't seem qcom specific.  Aren't we going to end up
> with lots of drivers reimplementing something like the
> qcom_pcie_port.perst list?

If this kind of switch is attached to other platforms, then yes. Right now, Qcom
host is the only known DT based host platform that has seen this requirement.

If other drivers also start showing this pattern, I may try to create a common
code somewhere to avoid code duplication. But now, common code is not warranted.

- Mani

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

Re: [PATCH v3 2/4] PCI: qcom: Move host bridge 'phy' and 'reset' pointers to struct qcom_pcie_port

[Manivannan Sadhasivam]

On Fri, Sep 12, 2025 at 06:23:48PM GMT, Bjorn Helgaas wrote:
> On Fri, Sep 12, 2025 at 02:05:02PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> > DT binding allows specifying 'phy' and 'reset' properties in both host
> > bridge and Root Port nodes, though specifying in the host bridge node is
> > marked as deprecated. Still, the pcie-qcom driver should support both
> > combinations for maintaining the DT backwards compatibility. For this
> > purpose, the driver is holding the relevant pointers of these properties in
> > two structs: struct qcom_pcie_port and struct qcom_pcie.
> > 
> > However, this causes confusion and increases the driver complexity. Hence,
> > move the pointers from struct qcom_pcie to struct qcom_pcie_port. As a
> > result, even if these properties are specified in the host bridge node,
> > the pointers will be stored in struct qcom_pcie_port as if the properties
> > are specified in a single Root Port node. This logic simplifies the driver
> > a lot.
> 
> > @@ -297,11 +295,8 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> >  	struct qcom_pcie_port *port;
> >  	int val = assert ? 1 : 0;
> >  
> > -	if (list_empty(&pcie->ports))
> > -		gpiod_set_value_cansleep(pcie->reset, val);
> > -	else
> > -		list_for_each_entry(port, &pcie->ports, list)
> > -			gpiod_set_value_cansleep(port->reset, val);
> > +	list_for_each_entry(port, &pcie->ports, list)
> > +		gpiod_set_value_cansleep(port->reset, val);
> 
> This is so much nicer, thanks for doing this!
> 
> >  static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
> >  {
> >  	struct device *dev = pcie->pci->dev;
> > +	struct qcom_pcie_port *port;
> > +	struct gpio_desc *reset;
> > +	struct phy *phy;
> >  	int ret;
> >  
> > -	pcie->phy = devm_phy_optional_get(dev, "pciephy");
> > -	if (IS_ERR(pcie->phy))
> > -		return PTR_ERR(pcie->phy);
> > +	phy = devm_phy_optional_get(dev, "pciephy");
> > +	if (IS_ERR(phy))
> > +		return PTR_ERR(phy);
> 
> Seems like it would be easier to integrate this fallback into
> qcom_pcie_parse_port() instead if separating it into
> qcom_pcie_parse_legacy_binding().
> 
> What if you did something like this in qcom_pcie_parse_port():
> 
>   qcom_pcie_parse_port
>   {
>       reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
> 				    "reset",  GPIOD_OUT_HIGH, "PERST#");
>       if (IS_ERR(reset)) {
> 	  reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> 	  if (IS_ERR(reset))
> 	      return PTR_ERR(reset);
>       }
>       ...
> 
> Then you could share all the port kzalloc and port list management.
> 
> Could do the same with the PHY stuff.
> 

Yeah, we could do something like this, but this will unnecessarily do a lookup
for 'perst-gpios' for every bridge node where there will only be 'reset-gpios'.
It is not a big deal though, since this code is only called in the probe path,
but I find the qcom_pcie_parse_legacy_binding() function to be visually
appealing as it separates legacy binding handling from the Root Port binding
and also makes it easy to drop the support for it in the future.

- Mani

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

Re: [PATCH v3 3/4] PCI: qcom: Parse PERST# from all PCIe bridge nodes

[Bjorn Helgaas]

On Mon, Sep 15, 2025 at 06:23:45PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Sep 12, 2025 at 06:28:11PM GMT, Bjorn Helgaas wrote:
> > On Fri, Sep 12, 2025 at 02:05:03PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > 
> > > Devicetree schema allows the PERST# GPIO to be present in all
> > > PCIe bridge nodes, not just in Root Port node. But the current
> > > logic parses PERST# only from the Root Port nodes. Though it is
> > > not causing any issue on the current platforms, the upcoming
> > > platforms will have PERST# in PCIe switch downstream ports also.
> > > So this requires parsing all the PCIe bridge nodes for the
> > > PERST# GPIO.
> > > 
> > > Hence, rework the parsing logic to extend to all PCIe bridge
> > > nodes starting from the Root Port node. If the 'reset-gpios'
> > > property is found for a PCI bridge node, the GPIO descriptor
> > > will be stored in qcom_pcie_perst::desc and added to the
> > > qcom_pcie_port::perst list.
> > 
> > The switch part doesn't seem qcom specific.  Aren't we going to
> > end up with lots of drivers reimplementing something like the
> > qcom_pcie_port.perst list?
> 
> If this kind of switch is attached to other platforms, then yes.
> Right now, Qcom host is the only known DT based host platform that
> has seen this requirement.

So I guess the issue here is that pwrctrl controls power to a slot
below a Switch Downstream Port, and we want pwrctrl to also control
PERST# to that same slot so that pwrctrl can power up the slot and
then deassert PERST# to the slot later, e.g., after a T_PVPERL delay?

Seems like whatever parses the devicetree power regulator information
for the slot should also parse the PERST# GPIO for the slot.

Bjorn

Re: [PATCH v3 2/4] PCI: qcom: Move host bridge 'phy' and 'reset' pointers to struct qcom_pcie_port

[Bjorn Helgaas]

On Fri, Sep 12, 2025 at 02:05:02PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> DT binding allows specifying 'phy' and 'reset' properties in both host
> bridge and Root Port nodes, though specifying in the host bridge node is
> marked as deprecated. Still, the pcie-qcom driver should support both
> combinations for maintaining the DT backwards compatibility. For this
> purpose, the driver is holding the relevant pointers of these properties in
> two structs: struct qcom_pcie_port and struct qcom_pcie.
> 
> However, this causes confusion and increases the driver complexity. Hence,
> move the pointers from struct qcom_pcie to struct qcom_pcie_port. As a
> result, even if these properties are specified in the host bridge node,
> the pointers will be stored in struct qcom_pcie_port as if the properties
> are specified in a single Root Port node. This logic simplifies the driver
> a lot.
> 
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

Reviewed-by: Bjorn Helgaas <bhelgaas@google.com> 

I would put this patch by itself on pci/controller/qcom immediately
because it's not related to the rest of the series, and we should make
sure it's in v6.18 regardless of the rest.

> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 87 ++++++++++++++--------------------
>  1 file changed, 36 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..6170c86f465f43f980f5b2f88bd8799c3c152e68 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -279,8 +279,6 @@ struct qcom_pcie {
>  	void __iomem *elbi;			/* DT elbi */
>  	void __iomem *mhi;
>  	union qcom_pcie_resources res;
> -	struct phy *phy;
> -	struct gpio_desc *reset;
>  	struct icc_path *icc_mem;
>  	struct icc_path *icc_cpu;
>  	const struct qcom_pcie_cfg *cfg;
> @@ -297,11 +295,8 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
>  	struct qcom_pcie_port *port;
>  	int val = assert ? 1 : 0;
>  
> -	if (list_empty(&pcie->ports))
> -		gpiod_set_value_cansleep(pcie->reset, val);
> -	else
> -		list_for_each_entry(port, &pcie->ports, list)
> -			gpiod_set_value_cansleep(port->reset, val);
> +	list_for_each_entry(port, &pcie->ports, list)
> +		gpiod_set_value_cansleep(port->reset, val);
>  
>  	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
>  }
> @@ -1253,57 +1248,32 @@ static bool qcom_pcie_link_up(struct dw_pcie *pci)
>  	return val & PCI_EXP_LNKSTA_DLLLA;
>  }
>  
> -static void qcom_pcie_phy_exit(struct qcom_pcie *pcie)
> -{
> -	struct qcom_pcie_port *port;
> -
> -	if (list_empty(&pcie->ports))
> -		phy_exit(pcie->phy);
> -	else
> -		list_for_each_entry(port, &pcie->ports, list)
> -			phy_exit(port->phy);
> -}
> -
>  static void qcom_pcie_phy_power_off(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_port *port;
>  
> -	if (list_empty(&pcie->ports)) {
> -		phy_power_off(pcie->phy);
> -	} else {
> -		list_for_each_entry(port, &pcie->ports, list)
> -			phy_power_off(port->phy);
> -	}
> +	list_for_each_entry(port, &pcie->ports, list)
> +		phy_power_off(port->phy);
>  }
>  
>  static int qcom_pcie_phy_power_on(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_port *port;
> -	int ret = 0;
> +	int ret;
>  
> -	if (list_empty(&pcie->ports)) {
> -		ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> +	list_for_each_entry(port, &pcie->ports, list) {
> +		ret = phy_set_mode_ext(port->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
>  		if (ret)
>  			return ret;
>  
> -		ret = phy_power_on(pcie->phy);
> -		if (ret)
> +		ret = phy_power_on(port->phy);
> +		if (ret) {
> +			qcom_pcie_phy_power_off(pcie);
>  			return ret;
> -	} else {
> -		list_for_each_entry(port, &pcie->ports, list) {
> -			ret = phy_set_mode_ext(port->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> -			if (ret)
> -				return ret;
> -
> -			ret = phy_power_on(port->phy);
> -			if (ret) {
> -				qcom_pcie_phy_power_off(pcie);
> -				return ret;
> -			}
>  		}
>  	}
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> @@ -1748,8 +1718,10 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  	return ret;
>  
>  err_port_del:
> -	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> +		phy_exit(port->phy);
>  		list_del(&port->list);
> +	}
>  
>  	return ret;
>  }
> @@ -1757,20 +1729,32 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
>  {
>  	struct device *dev = pcie->pci->dev;
> +	struct qcom_pcie_port *port;
> +	struct gpio_desc *reset;
> +	struct phy *phy;
>  	int ret;
>  
> -	pcie->phy = devm_phy_optional_get(dev, "pciephy");
> -	if (IS_ERR(pcie->phy))
> -		return PTR_ERR(pcie->phy);
> +	phy = devm_phy_optional_get(dev, "pciephy");
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);
>  
> -	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> -	if (IS_ERR(pcie->reset))
> -		return PTR_ERR(pcie->reset);
> +	reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset))
> +		return PTR_ERR(reset);
>  
> -	ret = phy_init(pcie->phy);
> +	ret = phy_init(phy);
>  	if (ret)
>  		return ret;
>  
> +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->reset = reset;
> +	port->phy = phy;
> +	INIT_LIST_HEAD(&port->list);
> +	list_add_tail(&port->list, &pcie->ports);
> +
>  	return 0;
>  }
>  
> @@ -1984,9 +1968,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  err_host_deinit:
>  	dw_pcie_host_deinit(pp);
>  err_phy_exit:
> -	qcom_pcie_phy_exit(pcie);
> -	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> +		phy_exit(port->phy);
>  		list_del(&port->list);
> +	}
>  err_pm_runtime_put:
>  	pm_runtime_put(dev);
>  	pm_runtime_disable(dev);
> 
> -- 
> 2.45.2
> 
> 

Re: [PATCH v3 4/4] PCI: qcom: Allow pwrctrl core to control PERST# if 'reset-gpios' property is available

[Bjorn Helgaas]

On Fri, Sep 12, 2025 at 02:05:04PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> For historic reasons, the pcie-qcom driver was controlling the power supply
> and PERST# GPIO of the PCIe slot.

> This turned out to be an issue as the power supply requirements
> differ between components. For instance, some of the WLAN chipsets
> used in Qualcomm systems were connected to the Root Port in a
> non-standard way using their own connectors.

This is kind of hand-wavy.  I don't know what a non-standard connector
has to do with this.  I assume there's still a PCIe link from Root
Port to WLAN, and there's still a PERST# signal to the WLAN device and
a Root Port GPIO that asserts/deasserts it.

> This requires specific power sequencing mechanisms for controlling
> the WLAN chipsets. So the pwrctrl framework (CONFIG_PWRCTRL) was
> introduced to handle these custom and complex power supply
> requirements for components.
> 
> Sooner, we realized that it would be best to let the pwrctrl driver control
> the supplies to the PCIe slots also. As it will allow us to consolidate all
> the power supply handling in one place instead of doing it in two. So the
> CONFIG_PWRCTRL_SLOT driver was introduced, that just parses the Root Port
> nodes representing slots and controls the standard power supplies like
> 3.3v, 3.3VAux etc...
> 
> However, the control of the PERST# GPIOs was still within the controller
> drivers like pcie-qcom. So the controller drivers continued to assert/
> deassert PERST# GPIOs independent of the power supplies to the components.
> This mostly went unnoticed as the components tolerated this non-standard
> PERST# assertion/deassertion. But this behavior completely goes against the
> PCIe Electromechanical specs like CEM, M.2, as these specs enforce strict
> control of PERST# signal together with the power supplies.
> 
> So conform to these specs, allow the pwrctrl core to control PERST# for the
> slots if the 'reset-gpios' property is specified in the DT bridge nodes.
> This is achieved by populating the 'pci_host_bridge::perst_assert' callback
> with qcom_pcie_perst_assert() function, so that the pwrctrl core can
> control PERST# through this callback.
> 
> qcom_pcie_perst_assert() will find the PERST# GPIO descriptor associated
> with the supplied 'device_node' of the component and asserts/deasserts
> PERST# as requested by the 'assert' parameter. If PERST# is not found in
> the supplied node of the component, the function will look for PERST# in
> the parent node as a fallback. This is needed since PERST# won't be
> available in the endpoint node as per the DT binding.
> 
> Note that the driver still asserts PERST# during the controller
> initialization as it is needed as per the hardware documentation.
> 
> For preserving the backward compatibility with older DTs that still
> specifies the Root Port resources in the host bridge DT node, the
> controller driver still controls power supplies and PERST# for them. For
> those cases, the 'qcom_pcie::legacy_binding' flag will be set and the
> driver will continue to control PERST# exclusively. If this flag is not
> set, then the pwrctrl driver will control PERST# through the callback.

Re: [PATCH v3 3/4] PCI: qcom: Parse PERST# from all PCIe bridge nodes

[Manivannan Sadhasivam]

On Tue, Sep 16, 2025 at 02:49:06PM GMT, Bjorn Helgaas wrote:
> On Mon, Sep 15, 2025 at 06:23:45PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Sep 12, 2025 at 06:28:11PM GMT, Bjorn Helgaas wrote:
> > > On Fri, Sep 12, 2025 at 02:05:03PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > > 
> > > > Devicetree schema allows the PERST# GPIO to be present in all
> > > > PCIe bridge nodes, not just in Root Port node. But the current
> > > > logic parses PERST# only from the Root Port nodes. Though it is
> > > > not causing any issue on the current platforms, the upcoming
> > > > platforms will have PERST# in PCIe switch downstream ports also.
> > > > So this requires parsing all the PCIe bridge nodes for the
> > > > PERST# GPIO.
> > > > 
> > > > Hence, rework the parsing logic to extend to all PCIe bridge
> > > > nodes starting from the Root Port node. If the 'reset-gpios'
> > > > property is found for a PCI bridge node, the GPIO descriptor
> > > > will be stored in qcom_pcie_perst::desc and added to the
> > > > qcom_pcie_port::perst list.
> > > 
> > > The switch part doesn't seem qcom specific.  Aren't we going to
> > > end up with lots of drivers reimplementing something like the
> > > qcom_pcie_port.perst list?
> > 
> > If this kind of switch is attached to other platforms, then yes.
> > Right now, Qcom host is the only known DT based host platform that
> > has seen this requirement.
> 
> So I guess the issue here is that pwrctrl controls power to a slot
> below a Switch Downstream Port, and we want pwrctrl to also control
> PERST# to that same slot so that pwrctrl can power up the slot and
> then deassert PERST# to the slot later, e.g., after a T_PVPERL delay?
> 
> Seems like whatever parses the devicetree power regulator information
> for the slot should also parse the PERST# GPIO for the slot.
> 

Problem is, the controller driver also asserts PERST# during controller
initialization. So even if the power control driver parses PERST#, it still need
to share it with the controller driver somehow. I tried it in previous
iteration, but it turned out to be too complex. So settled with this version
where the controller driver parses PERST# as it used to, but not just from Root
Port and then asserts/deasserts PERST# through the callback.

- Mani

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

Re: (subset) [PATCH v3 0/4] PCI/pwrctrl: Allow pwrctrl framework to control PERST# if available

[Manivannan Sadhasivam]

On Fri, 12 Sep 2025 14:05:00 +0530, Manivannan Sadhasivam wrote:
> This series is the proper version for toggling PERST# from the pwrctrl
> framework after the initial RFC posted earlier [1].
> 
> Problem statement
> =================
> 
> Pwrctrl framework is intented to control the supplies to the components on the
> PCI bus. However, if the platform supports the PERST# signal, it should be
> toggled as per the requirements in the electromechanical specifications like
> PCIe CEM, Mini, and M.2. Since the pwrctrl framework is controlling the power
> supplies, it should also toggle PERST# as per the requirements in the above
> mentioned specifications. Right now, it is just controlling the power to the
> components and rely on controller drivers to toggle PERST#, which goes against
> the specs. For instance, controller drivers will deassert PERST# even before the
> pwrctrl driver enables the supplies. This causes the device to see PERST#
> deassert immediately after power on, thereby violating the delay requirements in
> the PCI Electromechanical specs.
> 
> [...]

Applied, thanks!

[2/4] PCI: qcom: Move host bridge 'phy' and 'reset' pointers to struct qcom_pcie_port
      commit: af8df709bf365f5583d31091280354e1ef0b201f

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

Re: [PATCH v3 2/4] PCI: qcom: Move host bridge 'phy' and 'reset' pointers to struct qcom_pcie_port

[Manivannan Sadhasivam]

On Tue, Sep 16, 2025 at 03:08:17PM GMT, Bjorn Helgaas wrote:
> On Fri, Sep 12, 2025 at 02:05:02PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> > DT binding allows specifying 'phy' and 'reset' properties in both host
> > bridge and Root Port nodes, though specifying in the host bridge node is
> > marked as deprecated. Still, the pcie-qcom driver should support both
> > combinations for maintaining the DT backwards compatibility. For this
> > purpose, the driver is holding the relevant pointers of these properties in
> > two structs: struct qcom_pcie_port and struct qcom_pcie.
> > 
> > However, this causes confusion and increases the driver complexity. Hence,
> > move the pointers from struct qcom_pcie to struct qcom_pcie_port. As a
> > result, even if these properties are specified in the host bridge node,
> > the pointers will be stored in struct qcom_pcie_port as if the properties
> > are specified in a single Root Port node. This logic simplifies the driver
> > a lot.
> > 
> > Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com> 
> 
> I would put this patch by itself on pci/controller/qcom immediately
> because it's not related to the rest of the series, and we should make
> sure it's in v6.18 regardless of the rest.
> 

Done!

- Mani

> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 87 ++++++++++++++--------------------
> >  1 file changed, 36 insertions(+), 51 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..6170c86f465f43f980f5b2f88bd8799c3c152e68 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -279,8 +279,6 @@ struct qcom_pcie {
> >  	void __iomem *elbi;			/* DT elbi */
> >  	void __iomem *mhi;
> >  	union qcom_pcie_resources res;
> > -	struct phy *phy;
> > -	struct gpio_desc *reset;
> >  	struct icc_path *icc_mem;
> >  	struct icc_path *icc_cpu;
> >  	const struct qcom_pcie_cfg *cfg;
> > @@ -297,11 +295,8 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> >  	struct qcom_pcie_port *port;
> >  	int val = assert ? 1 : 0;
> >  
> > -	if (list_empty(&pcie->ports))
> > -		gpiod_set_value_cansleep(pcie->reset, val);
> > -	else
> > -		list_for_each_entry(port, &pcie->ports, list)
> > -			gpiod_set_value_cansleep(port->reset, val);
> > +	list_for_each_entry(port, &pcie->ports, list)
> > +		gpiod_set_value_cansleep(port->reset, val);
> >  
> >  	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> >  }
> > @@ -1253,57 +1248,32 @@ static bool qcom_pcie_link_up(struct dw_pcie *pci)
> >  	return val & PCI_EXP_LNKSTA_DLLLA;
> >  }
> >  
> > -static void qcom_pcie_phy_exit(struct qcom_pcie *pcie)
> > -{
> > -	struct qcom_pcie_port *port;
> > -
> > -	if (list_empty(&pcie->ports))
> > -		phy_exit(pcie->phy);
> > -	else
> > -		list_for_each_entry(port, &pcie->ports, list)
> > -			phy_exit(port->phy);
> > -}
> > -
> >  static void qcom_pcie_phy_power_off(struct qcom_pcie *pcie)
> >  {
> >  	struct qcom_pcie_port *port;
> >  
> > -	if (list_empty(&pcie->ports)) {
> > -		phy_power_off(pcie->phy);
> > -	} else {
> > -		list_for_each_entry(port, &pcie->ports, list)
> > -			phy_power_off(port->phy);
> > -	}
> > +	list_for_each_entry(port, &pcie->ports, list)
> > +		phy_power_off(port->phy);
> >  }
> >  
> >  static int qcom_pcie_phy_power_on(struct qcom_pcie *pcie)
> >  {
> >  	struct qcom_pcie_port *port;
> > -	int ret = 0;
> > +	int ret;
> >  
> > -	if (list_empty(&pcie->ports)) {
> > -		ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> > +	list_for_each_entry(port, &pcie->ports, list) {
> > +		ret = phy_set_mode_ext(port->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> >  		if (ret)
> >  			return ret;
> >  
> > -		ret = phy_power_on(pcie->phy);
> > -		if (ret)
> > +		ret = phy_power_on(port->phy);
> > +		if (ret) {
> > +			qcom_pcie_phy_power_off(pcie);
> >  			return ret;
> > -	} else {
> > -		list_for_each_entry(port, &pcie->ports, list) {
> > -			ret = phy_set_mode_ext(port->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> > -			if (ret)
> > -				return ret;
> > -
> > -			ret = phy_power_on(port->phy);
> > -			if (ret) {
> > -				qcom_pcie_phy_power_off(pcie);
> > -				return ret;
> > -			}
> >  		}
> >  	}
> >  
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> > @@ -1748,8 +1718,10 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
> >  	return ret;
> >  
> >  err_port_del:
> > -	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> > +		phy_exit(port->phy);
> >  		list_del(&port->list);
> > +	}
> >  
> >  	return ret;
> >  }
> > @@ -1757,20 +1729,32 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
> >  static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
> >  {
> >  	struct device *dev = pcie->pci->dev;
> > +	struct qcom_pcie_port *port;
> > +	struct gpio_desc *reset;
> > +	struct phy *phy;
> >  	int ret;
> >  
> > -	pcie->phy = devm_phy_optional_get(dev, "pciephy");
> > -	if (IS_ERR(pcie->phy))
> > -		return PTR_ERR(pcie->phy);
> > +	phy = devm_phy_optional_get(dev, "pciephy");
> > +	if (IS_ERR(phy))
> > +		return PTR_ERR(phy);
> >  
> > -	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> > -	if (IS_ERR(pcie->reset))
> > -		return PTR_ERR(pcie->reset);
> > +	reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(reset))
> > +		return PTR_ERR(reset);
> >  
> > -	ret = phy_init(pcie->phy);
> > +	ret = phy_init(phy);
> >  	if (ret)
> >  		return ret;
> >  
> > +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > +	if (!port)
> > +		return -ENOMEM;
> > +
> > +	port->reset = reset;
> > +	port->phy = phy;
> > +	INIT_LIST_HEAD(&port->list);
> > +	list_add_tail(&port->list, &pcie->ports);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -1984,9 +1968,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> >  err_host_deinit:
> >  	dw_pcie_host_deinit(pp);
> >  err_phy_exit:
> > -	qcom_pcie_phy_exit(pcie);
> > -	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> > +		phy_exit(port->phy);
> >  		list_del(&port->list);
> > +	}
> >  err_pm_runtime_put:
> >  	pm_runtime_put(dev);
> >  	pm_runtime_disable(dev);
> > 
> > -- 
> > 2.45.2
> > 
> > 

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

Re: [PATCH v3 4/4] PCI: qcom: Allow pwrctrl core to control PERST# if 'reset-gpios' property is available

[Manivannan Sadhasivam]

On Tue, Sep 16, 2025 at 03:48:10PM GMT, Bjorn Helgaas wrote:
> On Fri, Sep 12, 2025 at 02:05:04PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> > For historic reasons, the pcie-qcom driver was controlling the power supply
> > and PERST# GPIO of the PCIe slot.
> 
> > This turned out to be an issue as the power supply requirements
> > differ between components. For instance, some of the WLAN chipsets
> > used in Qualcomm systems were connected to the Root Port in a
> > non-standard way using their own connectors.
> 
> This is kind of hand-wavy.  I don't know what a non-standard connector
> has to do with this.  I assume there's still a PCIe link from Root
> Port to WLAN, and there's still a PERST# signal to the WLAN device and
> a Root Port GPIO that asserts/deasserts it.
> 

If we have a non-standard connector, then the power supply requirements change.
There is no longer the standard 3.3v, 3.3Vaux, 1.8v supplies, but plenty more.
For instance, take a look at the WCN6855 WiFi/BT combo chip in the Lenovo X13s
laptop:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts#n414

These supplies directly go from the host PMIC to the WCN6855 chip integrated
in the PCB itself. And these supplies need to be turned on/off in a sequence
also, together with the EN/SWCTRL GPIOs, while sharing with the Bluetooth
driver.

To handle this complexity, pwrctrl framework was introduced.

- Mani

> > This requires specific power sequencing mechanisms for controlling
> > the WLAN chipsets. So the pwrctrl framework (CONFIG_PWRCTRL) was
> > introduced to handle these custom and complex power supply
> > requirements for components.
> > 
> > Sooner, we realized that it would be best to let the pwrctrl driver control
> > the supplies to the PCIe slots also. As it will allow us to consolidate all
> > the power supply handling in one place instead of doing it in two. So the
> > CONFIG_PWRCTRL_SLOT driver was introduced, that just parses the Root Port
> > nodes representing slots and controls the standard power supplies like
> > 3.3v, 3.3VAux etc...
> > 
> > However, the control of the PERST# GPIOs was still within the controller
> > drivers like pcie-qcom. So the controller drivers continued to assert/
> > deassert PERST# GPIOs independent of the power supplies to the components.
> > This mostly went unnoticed as the components tolerated this non-standard
> > PERST# assertion/deassertion. But this behavior completely goes against the
> > PCIe Electromechanical specs like CEM, M.2, as these specs enforce strict
> > control of PERST# signal together with the power supplies.
> > 
> > So conform to these specs, allow the pwrctrl core to control PERST# for the
> > slots if the 'reset-gpios' property is specified in the DT bridge nodes.
> > This is achieved by populating the 'pci_host_bridge::perst_assert' callback
> > with qcom_pcie_perst_assert() function, so that the pwrctrl core can
> > control PERST# through this callback.
> > 
> > qcom_pcie_perst_assert() will find the PERST# GPIO descriptor associated
> > with the supplied 'device_node' of the component and asserts/deasserts
> > PERST# as requested by the 'assert' parameter. If PERST# is not found in
> > the supplied node of the component, the function will look for PERST# in
> > the parent node as a fallback. This is needed since PERST# won't be
> > available in the endpoint node as per the DT binding.
> > 
> > Note that the driver still asserts PERST# during the controller
> > initialization as it is needed as per the hardware documentation.
> > 
> > For preserving the backward compatibility with older DTs that still
> > specifies the Root Port resources in the host bridge DT node, the
> > controller driver still controls power supplies and PERST# for them. For
> > those cases, the 'qcom_pcie::legacy_binding' flag will be set and the
> > driver will continue to control PERST# exclusively. If this flag is not
> > set, then the pwrctrl driver will control PERST# through the callback.

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

Re: [PATCH v3 4/4] PCI: qcom: Allow pwrctrl core to control PERST# if 'reset-gpios' property is available

[Bjorn Helgaas]

On Wed, Sep 17, 2025 at 03:53:25PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Sep 16, 2025 at 03:48:10PM GMT, Bjorn Helgaas wrote:
> > On Fri, Sep 12, 2025 at 02:05:04PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > 
> > > For historic reasons, the pcie-qcom driver was controlling the
> > > power supply and PERST# GPIO of the PCIe slot.
> > 
> > > This turned out to be an issue as the power supply requirements
> > > differ between components. For instance, some of the WLAN
> > > chipsets used in Qualcomm systems were connected to the Root
> > > Port in a non-standard way using their own connectors.
> > 
> > This is kind of hand-wavy.  I don't know what a non-standard
> > connector has to do with this.  I assume there's still a PCIe link
> > from Root Port to WLAN, and there's still a PERST# signal to the
> > WLAN device and a Root Port GPIO that asserts/deasserts it.
> 
> If we have a non-standard connector, then the power supply
> requirements change.  There is no longer the standard 3.3v, 3.3Vaux,
> 1.8v supplies, but plenty more.  For instance, take a look at the
> WCN6855 WiFi/BT combo chip in the Lenovo X13s laptop:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts#n414
> 
> These supplies directly go from the host PMIC to the WCN6855 chip
> integrated in the PCB itself. And these supplies need to be turned
> on/off in a sequence also, together with the EN/SWCTRL GPIOs, while
> sharing with the Bluetooth driver.

It sounds like the WCN6855 power supplies have nothing to do with the
qcom PCIe controller, the Root Port, or any switches leading to the
WCN6855.  And I guess the same for the wlan-enable, bt-enable, and
swctrl GPIOs?

  wcn6855-pmu {
          compatible = "qcom,wcn6855-pmu";
          wlan-enable-gpios = <&tlmm 134 GPIO_ACTIVE_HIGH>;
          bt-enable-gpios = <&tlmm 133 GPIO_ACTIVE_HIGH>;
          swctrl-gpios = <&tlmm 132 GPIO_ACTIVE_HIGH>;
          regulators {
                  vreg_pmu_rfa_cmn_0p8: ldo0 {
                          regulator-name = "vreg_pmu_rfa_cmn_0p8";
                  ...

  &pcie4_port0 {
          wifi@0 {
                  compatible = "pci17cb,1103";
                  vddrfacmn-supply = <&vreg_pmu_rfa_cmn_0p8>;
                  ...

But I guess PERST# isn't described in the same place (not in
wcn6855-pmu)?  Looks like maybe it's this, which IIUC is part of the
pcie4 host bridge?

  &pcie4 {
          max-link-speed = <2>;
          perst-gpios = <&tlmm 141 GPIO_ACTIVE_LOW>;
          wake-gpios = <&tlmm 139 GPIO_ACTIVE_LOW>;

Does that mean this PERST# signal is driven by a GPIO and routed
directly to the WCN6855?  Seems like there's some affinity between the
WCN6855 power supplies and the WCN6855 PERST# signal, and maybe they
would be better described together?

Re: [PATCH v3 4/4] PCI: qcom: Allow pwrctrl core to control PERST# if 'reset-gpios' property is available

[Manivannan Sadhasivam]

On Thu, Sep 18, 2025 at 01:53:56PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 17, 2025 at 03:53:25PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Sep 16, 2025 at 03:48:10PM GMT, Bjorn Helgaas wrote:
> > > On Fri, Sep 12, 2025 at 02:05:04PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > > 
> > > > For historic reasons, the pcie-qcom driver was controlling the
> > > > power supply and PERST# GPIO of the PCIe slot.
> > > 
> > > > This turned out to be an issue as the power supply requirements
> > > > differ between components. For instance, some of the WLAN
> > > > chipsets used in Qualcomm systems were connected to the Root
> > > > Port in a non-standard way using their own connectors.
> > > 
> > > This is kind of hand-wavy.  I don't know what a non-standard
> > > connector has to do with this.  I assume there's still a PCIe link
> > > from Root Port to WLAN, and there's still a PERST# signal to the
> > > WLAN device and a Root Port GPIO that asserts/deasserts it.
> > 
> > If we have a non-standard connector, then the power supply
> > requirements change.  There is no longer the standard 3.3v, 3.3Vaux,
> > 1.8v supplies, but plenty more.  For instance, take a look at the
> > WCN6855 WiFi/BT combo chip in the Lenovo X13s laptop:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts#n414
> > 
> > These supplies directly go from the host PMIC to the WCN6855 chip
> > integrated in the PCB itself. And these supplies need to be turned
> > on/off in a sequence also, together with the EN/SWCTRL GPIOs, while
> > sharing with the Bluetooth driver.
> 
> It sounds like the WCN6855 power supplies have nothing to do with the
> qcom PCIe controller, the Root Port, or any switches leading to the
> WCN6855.  And I guess the same for the wlan-enable, bt-enable, and
> swctrl GPIOs?
> 
>   wcn6855-pmu {
>           compatible = "qcom,wcn6855-pmu";
>           wlan-enable-gpios = <&tlmm 134 GPIO_ACTIVE_HIGH>;
>           bt-enable-gpios = <&tlmm 133 GPIO_ACTIVE_HIGH>;
>           swctrl-gpios = <&tlmm 132 GPIO_ACTIVE_HIGH>;
>           regulators {
>                   vreg_pmu_rfa_cmn_0p8: ldo0 {
>                           regulator-name = "vreg_pmu_rfa_cmn_0p8";
>                   ...
> 
>   &pcie4_port0 {
>           wifi@0 {
>                   compatible = "pci17cb,1103";
>                   vddrfacmn-supply = <&vreg_pmu_rfa_cmn_0p8>;
>                   ...
> 
> But I guess PERST# isn't described in the same place (not in
> wcn6855-pmu)?  Looks like maybe it's this, which IIUC is part of the
> pcie4 host bridge?
> 
>   &pcie4 {
>           max-link-speed = <2>;
>           perst-gpios = <&tlmm 141 GPIO_ACTIVE_LOW>;
>           wake-gpios = <&tlmm 139 GPIO_ACTIVE_LOW>;
> 
> Does that mean this PERST# signal is driven by a GPIO and routed
> directly to the WCN6855?  Seems like there's some affinity between the
> WCN6855 power supplies and the WCN6855 PERST# signal, and maybe they
> would be better described together?

Yes, 'perst-gpios' is the PERST# signal that goes from the host system to the
WCN6855 chip. But we cannot define this signal in the WCN6855 node as the DT
binding only allows to define it in the PCI bridge nodes. This is why it is
currently defined in the host bridge node. But when this platform switches to
the per-Root Port binding, this property will be moved to the Root Port node as
'reset-gpios'.

Because of this reason, the host controller driver has to parse PERST# from all
PCI bridge nodes (like if there is a switch connected, there might be PERST# per
downstream port) and share them with the pwrctrl framework.

- Mani

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

Re: [PATCH v3 4/4] PCI: qcom: Allow pwrctrl core to control PERST# if 'reset-gpios' property is available

[Bjorn Helgaas]

On Fri, Sep 19, 2025 at 01:45:51PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Sep 18, 2025 at 01:53:56PM -0500, Bjorn Helgaas wrote:
> > On Wed, Sep 17, 2025 at 03:53:25PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Sep 16, 2025 at 03:48:10PM GMT, Bjorn Helgaas wrote:
> > > > On Fri, Sep 12, 2025 at 02:05:04PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > > > 
> > > > > For historic reasons, the pcie-qcom driver was controlling the
> > > > > power supply and PERST# GPIO of the PCIe slot.
> > > > 
> > > > > This turned out to be an issue as the power supply requirements
> > > > > differ between components. For instance, some of the WLAN
> > > > > chipsets used in Qualcomm systems were connected to the Root
> > > > > Port in a non-standard way using their own connectors.
> > > > 
> > > > This is kind of hand-wavy.  I don't know what a non-standard
> > > > connector has to do with this.  I assume there's still a PCIe link
> > > > from Root Port to WLAN, and there's still a PERST# signal to the
> > > > WLAN device and a Root Port GPIO that asserts/deasserts it.
> > > 
> > > If we have a non-standard connector, then the power supply
> > > requirements change.  There is no longer the standard 3.3v, 3.3Vaux,
> > > 1.8v supplies, but plenty more.  For instance, take a look at the
> > > WCN6855 WiFi/BT combo chip in the Lenovo X13s laptop:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts#n414
> > > 
> > > These supplies directly go from the host PMIC to the WCN6855 chip
> > > integrated in the PCB itself. And these supplies need to be turned
> > > on/off in a sequence also, together with the EN/SWCTRL GPIOs, while
> > > sharing with the Bluetooth driver.
> > 
> > It sounds like the WCN6855 power supplies have nothing to do with the
> > qcom PCIe controller, the Root Port, or any switches leading to the
> > WCN6855.  And I guess the same for the wlan-enable, bt-enable, and
> > swctrl GPIOs?
> > 
> >   wcn6855-pmu {
> >           compatible = "qcom,wcn6855-pmu";
> >           wlan-enable-gpios = <&tlmm 134 GPIO_ACTIVE_HIGH>;
> >           bt-enable-gpios = <&tlmm 133 GPIO_ACTIVE_HIGH>;
> >           swctrl-gpios = <&tlmm 132 GPIO_ACTIVE_HIGH>;
> >           regulators {
> >                   vreg_pmu_rfa_cmn_0p8: ldo0 {
> >                           regulator-name = "vreg_pmu_rfa_cmn_0p8";
> >                   ...
> > 
> >   &pcie4_port0 {
> >           wifi@0 {
> >                   compatible = "pci17cb,1103";
> >                   vddrfacmn-supply = <&vreg_pmu_rfa_cmn_0p8>;
> >                   ...
> > 
> > But I guess PERST# isn't described in the same place (not in
> > wcn6855-pmu)?  Looks like maybe it's this, which IIUC is part of the
> > pcie4 host bridge?
> > 
> >   &pcie4 {
> >           max-link-speed = <2>;
> >           perst-gpios = <&tlmm 141 GPIO_ACTIVE_LOW>;
> >           wake-gpios = <&tlmm 139 GPIO_ACTIVE_LOW>;
> > 
> > Does that mean this PERST# signal is driven by a GPIO and routed
> > directly to the WCN6855?  Seems like there's some affinity between the
> > WCN6855 power supplies and the WCN6855 PERST# signal, and maybe they
> > would be better described together?
> 
> Yes, 'perst-gpios' is the PERST# signal that goes from the host
> system to the WCN6855 chip. But we cannot define this signal in the
> WCN6855 node as the DT binding only allows to define it in the PCI
> bridge nodes. This is why it is currently defined in the host bridge
> node. But when this platform switches to the per-Root Port binding,
> this property will be moved to the Root Port node as 'reset-gpios'.

I'm questioning what the right place is to describe PERST#.  Neither
the host bridge/Root Complex nor the Root Port has any architected
support for asserting PERST#, so we can't write generic code to handle
it.

The PERST# signal is defined by the CEM specs, so can be physically
included in a standard connector or cable that carries the Link.  The
Link is originated by a Downstream Port, and the PCIe spec tells us
how to operate the Link using the DP's Link Control, Link Status, etc.

But PERST# might not originate in the Downstream Port, and the spec
doesn't tell us how to assert/deassert it, so I'm not sure it really
fits in the same class as things like 'max-link-speed' and
'num-lanes'.  Maybe it doesn't need to be in either the host bridge or
the Root Port?

> Because of this reason, the host controller driver has to parse
> PERST# from all PCI bridge nodes (like if there is a switch
> connected, there might be PERST# per downstream port) and share them
> with the pwrctrl framework.

Re: [PATCH v3 4/4] PCI: qcom: Allow pwrctrl core to control PERST# if 'reset-gpios' property is available

[Manivannan Sadhasivam]

On Mon, Sep 22, 2025 at 11:00:41AM -0500, Bjorn Helgaas wrote:
> On Fri, Sep 19, 2025 at 01:45:51PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Sep 18, 2025 at 01:53:56PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Sep 17, 2025 at 03:53:25PM +0530, Manivannan Sadhasivam wrote:
> > > > On Tue, Sep 16, 2025 at 03:48:10PM GMT, Bjorn Helgaas wrote:
> > > > > On Fri, Sep 12, 2025 at 02:05:04PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > > > > 
> > > > > > For historic reasons, the pcie-qcom driver was controlling the
> > > > > > power supply and PERST# GPIO of the PCIe slot.
> > > > > 
> > > > > > This turned out to be an issue as the power supply requirements
> > > > > > differ between components. For instance, some of the WLAN
> > > > > > chipsets used in Qualcomm systems were connected to the Root
> > > > > > Port in a non-standard way using their own connectors.
> > > > > 
> > > > > This is kind of hand-wavy.  I don't know what a non-standard
> > > > > connector has to do with this.  I assume there's still a PCIe link
> > > > > from Root Port to WLAN, and there's still a PERST# signal to the
> > > > > WLAN device and a Root Port GPIO that asserts/deasserts it.
> > > > 
> > > > If we have a non-standard connector, then the power supply
> > > > requirements change.  There is no longer the standard 3.3v, 3.3Vaux,
> > > > 1.8v supplies, but plenty more.  For instance, take a look at the
> > > > WCN6855 WiFi/BT combo chip in the Lenovo X13s laptop:
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts#n414
> > > > 
> > > > These supplies directly go from the host PMIC to the WCN6855 chip
> > > > integrated in the PCB itself. And these supplies need to be turned
> > > > on/off in a sequence also, together with the EN/SWCTRL GPIOs, while
> > > > sharing with the Bluetooth driver.
> > > 
> > > It sounds like the WCN6855 power supplies have nothing to do with the
> > > qcom PCIe controller, the Root Port, or any switches leading to the
> > > WCN6855.  And I guess the same for the wlan-enable, bt-enable, and
> > > swctrl GPIOs?
> > > 
> > >   wcn6855-pmu {
> > >           compatible = "qcom,wcn6855-pmu";
> > >           wlan-enable-gpios = <&tlmm 134 GPIO_ACTIVE_HIGH>;
> > >           bt-enable-gpios = <&tlmm 133 GPIO_ACTIVE_HIGH>;
> > >           swctrl-gpios = <&tlmm 132 GPIO_ACTIVE_HIGH>;
> > >           regulators {
> > >                   vreg_pmu_rfa_cmn_0p8: ldo0 {
> > >                           regulator-name = "vreg_pmu_rfa_cmn_0p8";
> > >                   ...
> > > 
> > >   &pcie4_port0 {
> > >           wifi@0 {
> > >                   compatible = "pci17cb,1103";
> > >                   vddrfacmn-supply = <&vreg_pmu_rfa_cmn_0p8>;
> > >                   ...
> > > 
> > > But I guess PERST# isn't described in the same place (not in
> > > wcn6855-pmu)?  Looks like maybe it's this, which IIUC is part of the
> > > pcie4 host bridge?
> > > 
> > >   &pcie4 {
> > >           max-link-speed = <2>;
> > >           perst-gpios = <&tlmm 141 GPIO_ACTIVE_LOW>;
> > >           wake-gpios = <&tlmm 139 GPIO_ACTIVE_LOW>;
> > > 
> > > Does that mean this PERST# signal is driven by a GPIO and routed
> > > directly to the WCN6855?  Seems like there's some affinity between the
> > > WCN6855 power supplies and the WCN6855 PERST# signal, and maybe they
> > > would be better described together?
> > 
> > Yes, 'perst-gpios' is the PERST# signal that goes from the host
> > system to the WCN6855 chip. But we cannot define this signal in the
> > WCN6855 node as the DT binding only allows to define it in the PCI
> > bridge nodes. This is why it is currently defined in the host bridge
> > node. But when this platform switches to the per-Root Port binding,
> > this property will be moved to the Root Port node as 'reset-gpios'.
> 
> I'm questioning what the right place is to describe PERST#.  Neither
> the host bridge/Root Complex nor the Root Port has any architected
> support for asserting PERST#, so we can't write generic code to handle
> it.
> 

True.

> The PERST# signal is defined by the CEM specs, so can be physically
> included in a standard connector or cable that carries the Link.  The
> Link is originated by a Downstream Port, and the PCIe spec tells us
> how to operate the Link using the DP's Link Control, Link Status, etc.
> 
> But PERST# might not originate in the Downstream Port, and the spec
> doesn't tell us how to assert/deassert it, so I'm not sure it really
> fits in the same class as things like 'max-link-speed' and
> 'num-lanes'.  Maybe it doesn't need to be in either the host bridge or
> the Root Port?
> 

While I agree that PERST# has nothing to do with the Downstream Port, we don't
have any better way to represent it in devicetree. Either this has to be defined
in Host Bridge or Root Port/Bridge or Endpoint node. Currently, the devicetree
spec allows it to be defined in both Host Bridge and Root Port nodes, but not in
the Endpoint node. AFAIU, this is due to the fact that PERST# is a host
controlled signal, not device (unlike WAKE#). So we cannot put it in the
Endpoint node.

Moreover, if it is defined in the Host Bridge node, then we cannot do
PERST#<->device mapping in the case of multiple PERST# signals. So defining it
in the Root Port/Bridge node seemed to be the ideal place (till when there is a
single PERST# per slot/downstream port).

Maybe Rob could share more of the insight.

- Mani

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