[PATCH v2 0/5] PCI/pwrctrl: Major rework to integrate pwrctrl devices with controller drivers

[PATCH v2 0/5] PCI/pwrctrl: Major rework to integrate pwrctrl devices with controller drivers

[Manivannan Sadhasivam]

Hi,

This series provides a major rework for the PCI power control (pwrctrl)
framework to enable the pwrctrl devices to be controlled by the PCI controller
drivers.

Problem Statement
=================

Currently, the pwrctrl framework faces two major issues:

1. Missing PERST# integration
2. Inability to properly handle bus extenders such as PCIe switch devices

First issue arises from the disconnect between the PCI controller drivers and
pwrctrl framework. At present, the pwrctrl framework just operates on its own
with the help of the PCI core. The pwrctrl devices are created by the PCI core
during initial bus scan and the pwrctrl drivers once bind, just power on the
PCI devices during their probe(). This design conflicts with the PCI Express
Card Electromechanical Specification requirements for PERST# timing. The reason
is, PERST# signals are mostly handled by the controller drivers and often
deasserted even before the pwrctrl drivers probe. According to the spec, PERST#
should be deasserted only after power and reference clock to the device are
stable, within predefined timing parameters.

The second issue stems from the PCI bus scan completing before pwrctrl drivers
probe. This poses a significant problem for PCI bus extenders like switches
because the PCI core allocates upstream bridge resources during the initial
scan. If the upstream bridge is not hotplug capable, resources are allocated
only for the number of downstream buses detected at scan time, which might be
just one if the switch was not powered and enumerated at that time. Later, when
the pwrctrl driver powers on and enumerates the switch, enumeration fails due to
insufficient upstream bridge resources.

Proposal
========

This series addresses both issues by introducing new individual APIs for pwrctrl
device creation, destruction, power on, and power off operations. Controller
drivers are expected to invoke these APIs during their probe(), remove(),
suspend(), and resume() operations. This integration allows better coordination
between controller drivers and the pwrctrl framework, enabling enhanced features
such as D3Cold support.

The original design aimed to avoid modifying controller drivers for pwrctrl
integration. However, this approach lacked scalability because different
controllers have varying requirements for when devices should be powered on. For
example, controller drivers require devices to be powered on early for
successful PHY initialization.

By using these explicit APIs, controller drivers gain fine grained control over
their associated pwrctrl devices.

This series modified the pcie-qcom driver (only consumer of pwrctrl framework)
to adopt to these APIs and also removed the old pwrctrl code from PCI core. This
could be used as a reference to add pwrctrl support for other controller drivers
also.

For example, to control the 3.3v supply to the PCI slot where the NVMe device is
connected, below modifications are required:

Devicetree
----------

	// In SoC dtsi:

	pci@1bf8000 { // controller node
		...
		pcie1_port0: pcie@0 { // PCI Root Port node
			compatible = "pciclass,0604"; // required for pwrctrl
							 driver bind
			...
		};
	};

	// In board dts:

	&pcie1_port0 {
		reset-gpios = <&tlmm 152 GPIO_ACTIVE_LOW>; // optional
		vpcie3v3-supply = <&vreg_nvme>; // NVMe power supply
	};

Controller driver
-----------------

	// Select PCI_PWRCTRL_SLOT in controller Kconfig

	probe() {
		...
		// Initialize controller resources
		pci_pwrctrl_create_devices(&pdev->dev);
		pci_pwrctrl_power_on_devices(&pdev->dev);
		// Deassert PERST# (optional)
		...
		pci_host_probe(); // Allocate host bridge and start bus scan
	}

	suspend {
		// PME_Turn_Off broadcast
		// Assert PERST# (optional)
		pci_pwrctrl_power_off_devices(&pdev->dev);
		...
	}

	resume {
		...
		pci_pwrctrl_power_on_devices(&pdev->dev);
		// Deassert PERST# (optional)
	}

I will add a documentation for the pwrctrl framework in the coming days to make
it easier to use.

Testing
=======

This series is tested on the Lenovo Thinkpad T14s laptop based on Qcom X1E
chipset and RB3Gen2 development board with TC9563 switch based on Qcom QCS6490
chipset.

**NOTE**: With this series, the controller driver may undergo multiple probe
deferral if the pwrctrl driver was not available during the controller driver
probe. This is pretty much required to avoid the resource allocation issue.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
Changes in v2:
- Exported of_pci_supply_present() API
- Demoted the -EPROBE_DEFER log to dev_dbg()
- Collected tags and rebased on top of v6.19-rc1
- Link to v1: https://lore.kernel.org/r/20251124-pci-pwrctrl-rework-v1-0-78a72627683d@oss.qualcomm.com

---
Krishna Chaitanya Chundru (1):
      PCI/pwrctrl: Add APIs for explicitly creating and destroying pwrctrl devices

Manivannan Sadhasivam (4):
      PCI: qcom: Parse PERST# from all PCIe bridge nodes
      PCI/pwrctrl: Add 'struct pci_pwrctrl::power_{on/off}' callbacks
      PCI/pwrctrl: Add APIs to power on/off the pwrctrl devices
      PCI/pwrctrl: Switch to the new pwrctrl APIs

 drivers/pci/controller/dwc/pcie-qcom.c   | 124 +++++++++++++---
 drivers/pci/of.c                         |   1 +
 drivers/pci/probe.c                      |  59 --------
 drivers/pci/pwrctrl/core.c               | 248 +++++++++++++++++++++++++++++--
 drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c |  30 +++-
 drivers/pci/pwrctrl/slot.c               |  46 ++++--
 drivers/pci/remove.c                     |  20 ---
 include/linux/pci-pwrctrl.h              |  16 +-
 8 files changed, 408 insertions(+), 136 deletions(-)
---
base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
change-id: 20251124-pci-pwrctrl-rework-c91a6e16c2f6

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

[PATCH v2 1/5] PCI: qcom: Parse PERST# from all PCIe bridge nodes

[Manivannan Sadhasivam]

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.

Tested-by: Chen-Yu Tsai <wenst@chromium.org>
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 7b92e7a1c0d9..73032449d289 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 {
@@ -291,11 +296,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);
 }
@@ -1702,18 +1710,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))
@@ -1727,7 +1775,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);
@@ -1737,9 +1790,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) {
 		if (!of_node_is_type(of_port, "pci"))
@@ -1752,7 +1806,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);
 	}
@@ -1763,6 +1819,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;
@@ -1784,19 +1841,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;
@@ -1937,7 +2003,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;
@@ -1996,7 +2062,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.48.1

[PATCH v2 2/5] PCI/pwrctrl: Add 'struct pci_pwrctrl::power_{on/off}' callbacks

[Manivannan Sadhasivam]

To allow the pwrctrl core to control the power on/off sequences of the
pwrctrl drivers, add the 'struct pci_pwrctrl::power_{on/off}' callbacks and
populate them in the respective pwrctrl drivers.

The pwrctrl drivers still power on the resources on their own now. So there
is no functional change.

Co-developed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c | 27 +++++++++++++++---
 drivers/pci/pwrctrl/slot.c               | 48 ++++++++++++++++++++++----------
 include/linux/pci-pwrctrl.h              |  4 +++
 3 files changed, 61 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c b/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
index 4e664e7b8dd2..0fb9038a1d18 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
@@ -52,11 +52,27 @@ static const struct pci_pwrctrl_pwrseq_pdata pci_pwrctrl_pwrseq_qcom_wcn_pdata =
 	.validate_device = pci_pwrctrl_pwrseq_qcm_wcn_validate_device,
 };
 
+static int pci_pwrctrl_pwrseq_power_on(struct pci_pwrctrl *ctx)
+{
+	struct pci_pwrctrl_pwrseq_data *data = container_of(ctx, struct pci_pwrctrl_pwrseq_data,
+							    ctx);
+
+	return pwrseq_power_on(data->pwrseq);
+}
+
+static void pci_pwrctrl_pwrseq_power_off(struct pci_pwrctrl *ctx)
+{
+	struct pci_pwrctrl_pwrseq_data *data = container_of(ctx, struct pci_pwrctrl_pwrseq_data,
+							    ctx);
+
+	pwrseq_power_off(data->pwrseq);
+}
+
 static void devm_pci_pwrctrl_pwrseq_power_off(void *data)
 {
-	struct pwrseq_desc *pwrseq = data;
+	struct pci_pwrctrl_pwrseq_data *pwrseq_data = data;
 
-	pwrseq_power_off(pwrseq);
+	pci_pwrctrl_pwrseq_power_off(&pwrseq_data->ctx);
 }
 
 static int pci_pwrctrl_pwrseq_probe(struct platform_device *pdev)
@@ -85,16 +101,19 @@ static int pci_pwrctrl_pwrseq_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(data->pwrseq),
 				     "Failed to get the power sequencer\n");
 
-	ret = pwrseq_power_on(data->pwrseq);
+	ret = pci_pwrctrl_pwrseq_power_on(&data->ctx);
 	if (ret)
 		return dev_err_probe(dev, ret,
 				     "Failed to power-on the device\n");
 
 	ret = devm_add_action_or_reset(dev, devm_pci_pwrctrl_pwrseq_power_off,
-				       data->pwrseq);
+				       data);
 	if (ret)
 		return ret;
 
+	data->ctx.power_on = pci_pwrctrl_pwrseq_power_on;
+	data->ctx.power_off = pci_pwrctrl_pwrseq_power_off;
+
 	pci_pwrctrl_init(&data->ctx, dev);
 
 	ret = devm_pci_pwrctrl_device_set_ready(dev, &data->ctx);
diff --git a/drivers/pci/pwrctrl/slot.c b/drivers/pci/pwrctrl/slot.c
index 3320494b62d8..14701f65f1f2 100644
--- a/drivers/pci/pwrctrl/slot.c
+++ b/drivers/pci/pwrctrl/slot.c
@@ -17,13 +17,36 @@ struct pci_pwrctrl_slot_data {
 	struct pci_pwrctrl ctx;
 	struct regulator_bulk_data *supplies;
 	int num_supplies;
+	struct clk *clk;
 };
 
-static void devm_pci_pwrctrl_slot_power_off(void *data)
+static int pci_pwrctrl_slot_power_on(struct pci_pwrctrl *ctx)
 {
-	struct pci_pwrctrl_slot_data *slot = data;
+	struct pci_pwrctrl_slot_data *slot = container_of(ctx, struct pci_pwrctrl_slot_data, ctx);
+	int ret;
+
+	ret = regulator_bulk_enable(slot->num_supplies, slot->supplies);
+	if (ret < 0) {
+		dev_err(slot->ctx.dev, "Failed to enable slot regulators\n");
+		return ret;
+	}
+
+	return clk_prepare_enable(slot->clk);
+}
+
+static void pci_pwrctrl_slot_power_off(struct pci_pwrctrl *ctx)
+{
+	struct pci_pwrctrl_slot_data *slot = container_of(ctx, struct pci_pwrctrl_slot_data, ctx);
 
 	regulator_bulk_disable(slot->num_supplies, slot->supplies);
+	clk_disable_unprepare(slot->clk);
+}
+
+static void devm_pci_pwrctrl_slot_release(void *data)
+{
+	struct pci_pwrctrl_slot_data *slot = data;
+
+	pci_pwrctrl_slot_power_off(&slot->ctx);
 	regulator_bulk_free(slot->num_supplies, slot->supplies);
 }
 
@@ -31,7 +54,6 @@ static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
 {
 	struct pci_pwrctrl_slot_data *slot;
 	struct device *dev = &pdev->dev;
-	struct clk *clk;
 	int ret;
 
 	slot = devm_kzalloc(dev, sizeof(*slot), GFP_KERNEL);
@@ -46,23 +68,21 @@ static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
 	}
 
 	slot->num_supplies = ret;
-	ret = regulator_bulk_enable(slot->num_supplies, slot->supplies);
-	if (ret < 0) {
-		dev_err_probe(dev, ret, "Failed to enable slot regulators\n");
-		regulator_bulk_free(slot->num_supplies, slot->supplies);
-		return ret;
-	}
 
-	ret = devm_add_action_or_reset(dev, devm_pci_pwrctrl_slot_power_off,
+	ret = devm_add_action_or_reset(dev, devm_pci_pwrctrl_slot_release,
 				       slot);
 	if (ret)
 		return ret;
 
-	clk = devm_clk_get_optional_enabled(dev, NULL);
-	if (IS_ERR(clk)) {
-		return dev_err_probe(dev, PTR_ERR(clk),
+	slot->clk = devm_clk_get_optional(dev, NULL);
+	if (IS_ERR(slot->clk))
+		return dev_err_probe(dev, PTR_ERR(slot->clk),
 				     "Failed to enable slot clock\n");
-	}
+
+	pci_pwrctrl_slot_power_on(&slot->ctx);
+
+	slot->ctx.power_on = pci_pwrctrl_slot_power_on;
+	slot->ctx.power_off = pci_pwrctrl_slot_power_off;
 
 	pci_pwrctrl_init(&slot->ctx, dev);
 
diff --git a/include/linux/pci-pwrctrl.h b/include/linux/pci-pwrctrl.h
index 4aefc7901cd1..bd0ee9998125 100644
--- a/include/linux/pci-pwrctrl.h
+++ b/include/linux/pci-pwrctrl.h
@@ -31,6 +31,8 @@ struct device_link;
 /**
  * struct pci_pwrctrl - PCI device power control context.
  * @dev: Address of the power controlling device.
+ * @power_on: Callback to power on the power controlling device.
+ * @power_off: Callback to power off the power controlling device.
  *
  * An object of this type must be allocated by the PCI power control device and
  * passed to the pwrctrl subsystem to trigger a bus rescan and setup a device
@@ -38,6 +40,8 @@ struct device_link;
  */
 struct pci_pwrctrl {
 	struct device *dev;
+	int (*power_on)(struct pci_pwrctrl *pwrctrl);
+	void (*power_off)(struct pci_pwrctrl *pwrctrl);
 
 	/* private: internal use only */
 	struct notifier_block nb;

-- 
2.48.1

[PATCH v2 3/5] PCI/pwrctrl: Add APIs for explicitly creating and destroying pwrctrl devices

[Manivannan Sadhasivam]

From: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

Previously, the PCI core created pwrctrl devices during pci_scan_device()
on its own and then skipped enumeration of those devices, hoping the
pwrctrl driver would power them on and trigger a bus rescan.

This approach works for endpoint devices directly connected to Root Ports,
but it fails for PCIe switches acting as bus extenders. When the switch
requires pwrctrl support, and the pwrctrl driver is not available during
the pwrctrl device creation, it's enumeration will be skipped during the
initial PCI bus scan.

This premature scan leads the PCI core to allocate resources (bridge
windows, bus numbers) for the upstream bridge based on available downstream
buses at scan time. For non-hotplug capable bridges, PCI core typically
allocates resources based on the number of buses available during the
initial bus scan, which happens to be just one if the switch is not powered
on and enumerated at that time. When the switch gets enumerated later on,
it will fail due to the lack of upstream resources.

As a result, a PCIe switch powered on by the pwrctrl driver cannot be
reliably enumerated currently. Either the switch has to be enabled in the
bootloader or the switch pwrctrl driver has to be loaded during the pwrctrl
device creation time to workaround these issues.

This commit introduces new APIs to explicitly create and destroy pwrctrl
devices from controller drivers by recursively scanning the PCI child nodes
of the controller. These APIs allow creating pwrctrl devices based on the
original criteria and are intended to be called during controller probe and
removal.

These APIs, together with the upcoming APIs for power on/off will allow the
controller drivers to power on all the devices before starting the initial
bus scan, thereby solving the resource allocation issue.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
[mani: splitted the patch, cleaned up the code, and rewrote description]
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/of.c            |   1 +
 drivers/pci/pwrctrl/core.c  | 112 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-pwrctrl.h |   8 +++-
 3 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 3579265f1198..9bb5f258759b 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -867,6 +867,7 @@ bool of_pci_supply_present(struct device_node *np)
 
 	return false;
 }
+EXPORT_SYMBOL_GPL(of_pci_supply_present);
 
 #endif /* CONFIG_PCI */
 
diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
index 6bdbfed584d6..6eca54e0d540 100644
--- a/drivers/pci/pwrctrl/core.c
+++ b/drivers/pci/pwrctrl/core.c
@@ -3,14 +3,21 @@
  * Copyright (C) 2024 Linaro Ltd.
  */
 
+#define dev_fmt(fmt) "Pwrctrl: " fmt
+
 #include <linux/device.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/pci.h>
 #include <linux/pci-pwrctrl.h>
+#include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/slab.h>
 
+#include "../pci.h"
+
 static int pci_pwrctrl_notify(struct notifier_block *nb, unsigned long action,
 			      void *data)
 {
@@ -145,6 +152,111 @@ int devm_pci_pwrctrl_device_set_ready(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_pci_pwrctrl_device_set_ready);
 
+static int pci_pwrctrl_create_device(struct device_node *np, struct device *parent)
+{
+	struct platform_device *pdev;
+	int ret;
+
+	for_each_available_child_of_node_scoped(np, child) {
+		ret = pci_pwrctrl_create_device(child, parent);
+		if (ret)
+			return ret;
+	}
+
+	/* Bail out if the platform device is already available for the node */
+	pdev = of_find_device_by_node(np);
+	if (pdev) {
+		put_device(&pdev->dev);
+		return 0;
+	}
+
+	/*
+	 * Sanity check to make sure that the node has the compatible property
+	 * to allow driver binding.
+	 */
+	if (!of_property_present(np, "compatible"))
+		return 0;
+
+	/*
+	 * Check whether the pwrctrl device really needs to be created or not.
+	 * This is decided based on at least one of the power supplies being
+	 * defined in the devicetree node of the device.
+	 */
+	if (!of_pci_supply_present(np)) {
+		dev_dbg(parent, "Skipping OF node: %s\n", np->name);
+		return 0;
+	}
+
+	/* Now create the pwrctrl device */
+	pdev = of_platform_device_create(np, NULL, parent);
+	if (!pdev) {
+		dev_err(parent, "Failed to create pwrctrl device for node: %s\n", np->name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/**
+ * pci_pwrctrl_create_devices - Create pwrctrl devices
+ *
+ * @parent: Parent PCI device for which the pwrctrl devices need to be created.
+ *
+ * This function recursively creates pwrctrl devices for the child nodes
+ * of the specified PCI parent device in a depth first manner.
+ *
+ * Returns: 0 on success, negative error number on error.
+ */
+int pci_pwrctrl_create_devices(struct device *parent)
+{
+	int ret;
+
+	for_each_available_child_of_node_scoped(parent->of_node, child) {
+		ret = pci_pwrctrl_create_device(child, parent);
+		if (ret) {
+			pci_pwrctrl_destroy_devices(parent);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_pwrctrl_create_devices);
+
+static void pci_pwrctrl_destroy_device(struct device_node *np)
+{
+	struct platform_device *pdev;
+
+	for_each_available_child_of_node_scoped(np, child)
+		pci_pwrctrl_destroy_device(child);
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev)
+		return;
+
+	of_device_unregister(pdev);
+	put_device(&pdev->dev);
+
+	of_node_clear_flag(np, OF_POPULATED);
+}
+
+/**
+ * pci_pwrctrl_destroy_devices - Destroy pwrctrl devices
+ *
+ * @parent: Parent PCI device for which the pwrctrl devices need to be destroyed.
+ *
+ * This function recursively destroys pwrctrl devices for the child nodes
+ * of the specified PCI parent device in a depth first manner.
+ */
+void pci_pwrctrl_destroy_devices(struct device *parent)
+{
+	struct device_node *np = parent->of_node;
+
+	for_each_available_child_of_node_scoped(np, child)
+		pci_pwrctrl_destroy_device(child);
+}
+EXPORT_SYMBOL_GPL(pci_pwrctrl_destroy_devices);
+
 MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@linaro.org>");
 MODULE_DESCRIPTION("PCI Device Power Control core driver");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/pci-pwrctrl.h b/include/linux/pci-pwrctrl.h
index bd0ee9998125..5590ffec0bea 100644
--- a/include/linux/pci-pwrctrl.h
+++ b/include/linux/pci-pwrctrl.h
@@ -54,5 +54,11 @@ int pci_pwrctrl_device_set_ready(struct pci_pwrctrl *pwrctrl);
 void pci_pwrctrl_device_unset_ready(struct pci_pwrctrl *pwrctrl);
 int devm_pci_pwrctrl_device_set_ready(struct device *dev,
 				     struct pci_pwrctrl *pwrctrl);
-
+#if IS_ENABLED(CONFIG_PCI_PWRCTRL)
+int pci_pwrctrl_create_devices(struct device *parent);
+void pci_pwrctrl_destroy_devices(struct device *parent);
+#else
+static inline int pci_pwrctrl_create_devices(struct device *parent) { return 0; }
+static void pci_pwrctrl_destroy_devices(struct device *parent) { }
+#endif
 #endif /* __PCI_PWRCTRL_H__ */

-- 
2.48.1

[PATCH v2 4/5] PCI/pwrctrl: Add APIs to power on/off the pwrctrl devices

[Manivannan Sadhasivam]

To fix PCIe bridge resource allocation issues when powering PCIe
switches with the pwrctrl driver, introduce APIs to explicitly power
on and off all related devices simultaneously.

Previously, the individual pwrctrl drivers powered on/off the PCIe devices
autonomously, without any control from the controller drivers. But to
enforce ordering w.r.t powering on the devices, these APIs will power
on/off all the devices at the same time.

The pci_pwrctrl_power_on_devices() API recursively scans the PCI child
nodes, makes sure that pwrctrl drivers are bind to devices, and calls their
power_on() callbacks.

Similarly, pci_pwrctrl_power_off_devices() API powers off devices
recursively via their power_off() callbacks.

These APIs are expected to be called during the controller probe and
suspend/resume time to power on/off the devices. But before calling these
APIs, the pwrctrl devices should've been created beforehand using the
pci_pwrctrl_{create/destroy}_devices() APIs.

Co-developed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/core.c  | 121 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-pwrctrl.h |   4 ++
 2 files changed, 125 insertions(+)

diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
index 6eca54e0d540..ebe1740b7c1c 100644
--- a/drivers/pci/pwrctrl/core.c
+++ b/drivers/pci/pwrctrl/core.c
@@ -65,6 +65,7 @@ void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
 {
 	pwrctrl->dev = dev;
 	INIT_WORK(&pwrctrl->work, rescan_work_func);
+	dev_set_drvdata(dev, pwrctrl);
 }
 EXPORT_SYMBOL_GPL(pci_pwrctrl_init);
 
@@ -152,6 +153,126 @@ int devm_pci_pwrctrl_device_set_ready(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_pci_pwrctrl_device_set_ready);
 
+static int __pci_pwrctrl_power_on_device(struct device *dev)
+{
+	struct pci_pwrctrl *pwrctrl = dev_get_drvdata(dev);
+
+	if (!pwrctrl)
+		return 0;
+
+	return pwrctrl->power_on(pwrctrl);
+}
+
+/*
+ * Power on the devices in a depth first manner. Before powering on the device,
+ * make sure its driver is bound.
+ */
+static int pci_pwrctrl_power_on_device(struct device_node *np)
+{
+	struct platform_device *pdev;
+	int ret;
+
+	for_each_available_child_of_node_scoped(np, child) {
+		ret = pci_pwrctrl_power_on_device(child);
+		if (ret)
+			return ret;
+	}
+
+	pdev = of_find_device_by_node(np);
+	if (pdev) {
+		if (!device_is_bound(&pdev->dev)) {
+			dev_dbg(&pdev->dev, "driver is not bound\n");
+			ret = -EPROBE_DEFER;
+		} else {
+			ret = __pci_pwrctrl_power_on_device(&pdev->dev);
+		}
+		put_device(&pdev->dev);
+
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * pci_pwrctrl_power_on_devices - Power on the pwrctrl devices
+ *
+ * @parent: Parent PCI device for which the pwrctrl devices need to be powered
+ * on.
+ *
+ * This function recursively traverses all pwrctrl devices for the child nodes
+ * of the specified PCI parent device, and powers them on in a depth first
+ * manner.
+ *
+ * Returns: 0 on success, negative error number on error.
+ */
+int pci_pwrctrl_power_on_devices(struct device *parent)
+{
+	struct device_node *np = parent->of_node;
+	int ret;
+
+	for_each_available_child_of_node_scoped(np, child) {
+		ret = pci_pwrctrl_power_on_device(child);
+		if (ret) {
+			pci_pwrctrl_power_off_devices(parent);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_pwrctrl_power_on_devices);
+
+static void __pci_pwrctrl_power_off_device(struct device *dev)
+{
+	struct pci_pwrctrl *pwrctrl = dev_get_drvdata(dev);
+
+	if (!pwrctrl)
+		return;
+
+	return pwrctrl->power_off(pwrctrl);
+}
+
+static int pci_pwrctrl_power_off_device(struct device_node *np)
+{
+	struct platform_device *pdev;
+
+	for_each_available_child_of_node_scoped(np, child)
+		pci_pwrctrl_power_off_device(child);
+
+	pdev = of_find_device_by_node(np);
+	if (pdev) {
+		if (device_is_bound(&pdev->dev))
+			__pci_pwrctrl_power_off_device(&pdev->dev);
+
+		put_device(&pdev->dev);
+	}
+
+	return 0;
+}
+
+/**
+ * pci_pwrctrl_power_off_devices - Power off the pwrctrl devices
+ *
+ * @parent: Parent PCI device for which the pwrctrl devices need to be powered
+ * off.
+ *
+ * This function recursively traverses all pwrctrl devices for the child nodes
+ * of the specified PCI parent device, and powers them off in a depth first
+ * manner.
+ *
+ * Returns: 0 on success, negative error number on error.
+ */
+void pci_pwrctrl_power_off_devices(struct device *parent)
+{
+	struct device_node *np = parent->of_node;
+
+	for_each_available_child_of_node_scoped(np, child)
+		pci_pwrctrl_power_off_device(child);
+}
+EXPORT_SYMBOL_GPL(pci_pwrctrl_power_off_devices);
+
 static int pci_pwrctrl_create_device(struct device_node *np, struct device *parent)
 {
 	struct platform_device *pdev;
diff --git a/include/linux/pci-pwrctrl.h b/include/linux/pci-pwrctrl.h
index 5590ffec0bea..1b77769eebbe 100644
--- a/include/linux/pci-pwrctrl.h
+++ b/include/linux/pci-pwrctrl.h
@@ -57,8 +57,12 @@ int devm_pci_pwrctrl_device_set_ready(struct device *dev,
 #if IS_ENABLED(CONFIG_PCI_PWRCTRL)
 int pci_pwrctrl_create_devices(struct device *parent);
 void pci_pwrctrl_destroy_devices(struct device *parent);
+int pci_pwrctrl_power_on_devices(struct device *parent);
+void pci_pwrctrl_power_off_devices(struct device *parent);
 #else
 static inline int pci_pwrctrl_create_devices(struct device *parent) { return 0; }
 static void pci_pwrctrl_destroy_devices(struct device *parent) { }
+static inline int pci_pwrctrl_power_on_devices(struct device *parent) { return 0; }
+static void pci_pwrctrl_power_off_devices(struct device *parent) { }
 #endif
 #endif /* __PCI_PWRCTRL_H__ */

-- 
2.48.1

[PATCH v2 5/5] PCI/pwrctrl: Switch to the new pwrctrl APIs

[Manivannan Sadhasivam]

Adopt the recently introduced pwrctrl APIs to create, power on, destroy,
and power off pwrctrl devices. In qcom_pcie_host_init(), call
pci_pwrctrl_create_devices() to create devices, then
pci_pwrctrl_power_on_devices() to power them on, both after controller
resource initialization. Once successful, deassert PERST# for all devices.

In qcom_pcie_host_deinit(), call pci_pwrctrl_power_off_devices() after
asserting PERST#. Note that pci_pwrctrl_destroy_devices() is not called
here, as deinit is only invoked during system suspend where device
destruction is unnecessary. If the driver becomes removable in future,
pci_pwrctrl_destroy_devices() should be called in the remove() handler.

At last, remove the old pwrctrl framework code from the PCI core, as the
new APIs are now the sole consumer of pwrctrl functionality. And also do
not power on the pwrctrl drivers during probe() as this is now handled by
the APIs.

Co-developed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c   | 22 ++++++++++--
 drivers/pci/probe.c                      | 59 --------------------------------
 drivers/pci/pwrctrl/core.c               | 15 --------
 drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c |  5 ---
 drivers/pci/pwrctrl/slot.c               |  2 --
 drivers/pci/remove.c                     | 20 -----------
 6 files changed, 20 insertions(+), 103 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 73032449d289..7c0c66480f12 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -24,6 +24,7 @@
 #include <linux/of_pci.h>
 #include <linux/pci.h>
 #include <linux/pci-ecam.h>
+#include <linux/pci-pwrctrl.h>
 #include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/platform_device.h>
@@ -1318,10 +1319,18 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 	if (ret)
 		goto err_deinit;
 
+	ret = pci_pwrctrl_create_devices(pci->dev);
+	if (ret)
+		goto err_disable_phy;
+
+	ret = pci_pwrctrl_power_on_devices(pci->dev);
+	if (ret)
+		goto err_pwrctrl_destroy;
+
 	if (pcie->cfg->ops->post_init) {
 		ret = pcie->cfg->ops->post_init(pcie);
 		if (ret)
-			goto err_disable_phy;
+			goto err_pwrctrl_power_off;
 	}
 
 	qcom_ep_reset_deassert(pcie);
@@ -1336,6 +1345,10 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 
 err_assert_reset:
 	qcom_ep_reset_assert(pcie);
+err_pwrctrl_power_off:
+	pci_pwrctrl_power_off_devices(pci->dev);
+err_pwrctrl_destroy:
+	pci_pwrctrl_destroy_devices(pci->dev);
 err_disable_phy:
 	qcom_pcie_phy_power_off(pcie);
 err_deinit:
@@ -1350,6 +1363,11 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 
 	qcom_ep_reset_assert(pcie);
+	/*
+	 * No need to destroy pwrctrl devices as this function only gets called
+	 * during system suspend as of now.
+	 */
+	pci_pwrctrl_power_off_devices(pci->dev);
 	qcom_pcie_phy_power_off(pcie);
 	pcie->cfg->ops->deinit(pcie);
 }
@@ -2027,7 +2045,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	ret = dw_pcie_host_init(pp);
 	if (ret) {
-		dev_err(dev, "cannot initialize host\n");
+		dev_err_probe(dev, ret, "cannot initialize host\n");
 		goto err_phy_exit;
 	}
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 41183aed8f5d..6e7252404b58 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2563,56 +2563,6 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
 }
 EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
 
-#if IS_ENABLED(CONFIG_PCI_PWRCTRL)
-static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
-{
-	struct pci_host_bridge *host = pci_find_host_bridge(bus);
-	struct platform_device *pdev;
-	struct device_node *np;
-
-	np = of_pci_find_child_device(dev_of_node(&bus->dev), devfn);
-	if (!np)
-		return NULL;
-
-	pdev = of_find_device_by_node(np);
-	if (pdev) {
-		put_device(&pdev->dev);
-		goto err_put_of_node;
-	}
-
-	/*
-	 * First check whether the pwrctrl device really needs to be created or
-	 * not. This is decided based on at least one of the power supplies
-	 * being defined in the devicetree node of the device.
-	 */
-	if (!of_pci_supply_present(np)) {
-		pr_debug("PCI/pwrctrl: Skipping OF node: %s\n", np->name);
-		goto err_put_of_node;
-	}
-
-	/* Now create the pwrctrl device */
-	pdev = of_platform_device_create(np, NULL, &host->dev);
-	if (!pdev) {
-		pr_err("PCI/pwrctrl: Failed to create pwrctrl device for node: %s\n", np->name);
-		goto err_put_of_node;
-	}
-
-	of_node_put(np);
-
-	return pdev;
-
-err_put_of_node:
-	of_node_put(np);
-
-	return NULL;
-}
-#else
-static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
-{
-	return NULL;
-}
-#endif
-
 /*
  * Read the config data for a PCI device, sanity-check it,
  * and fill in the dev structure.
@@ -2622,15 +2572,6 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
 	struct pci_dev *dev;
 	u32 l;
 
-	/*
-	 * Create pwrctrl device (if required) for the PCI device to handle the
-	 * power state. If the pwrctrl device is created, then skip scanning
-	 * further as the pwrctrl core will rescan the bus after powering on
-	 * the device.
-	 */
-	if (pci_pwrctrl_create_device(bus, devfn))
-		return NULL;
-
 	if (!pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
 		return NULL;
 
diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
index ebe1740b7c1c..4e2c41bc4ffe 100644
--- a/drivers/pci/pwrctrl/core.c
+++ b/drivers/pci/pwrctrl/core.c
@@ -45,16 +45,6 @@ static int pci_pwrctrl_notify(struct notifier_block *nb, unsigned long action,
 	return NOTIFY_DONE;
 }
 
-static void rescan_work_func(struct work_struct *work)
-{
-	struct pci_pwrctrl *pwrctrl = container_of(work,
-						   struct pci_pwrctrl, work);
-
-	pci_lock_rescan_remove();
-	pci_rescan_bus(to_pci_host_bridge(pwrctrl->dev->parent)->bus);
-	pci_unlock_rescan_remove();
-}
-
 /**
  * pci_pwrctrl_init() - Initialize the PCI power control context struct
  *
@@ -64,7 +54,6 @@ static void rescan_work_func(struct work_struct *work)
 void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
 {
 	pwrctrl->dev = dev;
-	INIT_WORK(&pwrctrl->work, rescan_work_func);
 	dev_set_drvdata(dev, pwrctrl);
 }
 EXPORT_SYMBOL_GPL(pci_pwrctrl_init);
@@ -95,8 +84,6 @@ int pci_pwrctrl_device_set_ready(struct pci_pwrctrl *pwrctrl)
 	if (ret)
 		return ret;
 
-	schedule_work(&pwrctrl->work);
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pci_pwrctrl_device_set_ready);
@@ -109,8 +96,6 @@ EXPORT_SYMBOL_GPL(pci_pwrctrl_device_set_ready);
  */
 void pci_pwrctrl_device_unset_ready(struct pci_pwrctrl *pwrctrl)
 {
-	cancel_work_sync(&pwrctrl->work);
-
 	/*
 	 * 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/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c b/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
index 0fb9038a1d18..7697a8a5cdde 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
@@ -101,11 +101,6 @@ static int pci_pwrctrl_pwrseq_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(data->pwrseq),
 				     "Failed to get the power sequencer\n");
 
-	ret = pci_pwrctrl_pwrseq_power_on(&data->ctx);
-	if (ret)
-		return dev_err_probe(dev, ret,
-				     "Failed to power-on the device\n");
-
 	ret = devm_add_action_or_reset(dev, devm_pci_pwrctrl_pwrseq_power_off,
 				       data);
 	if (ret)
diff --git a/drivers/pci/pwrctrl/slot.c b/drivers/pci/pwrctrl/slot.c
index 14701f65f1f2..888300aeefec 100644
--- a/drivers/pci/pwrctrl/slot.c
+++ b/drivers/pci/pwrctrl/slot.c
@@ -79,8 +79,6 @@ static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(slot->clk),
 				     "Failed to enable slot clock\n");
 
-	pci_pwrctrl_slot_power_on(&slot->ctx);
-
 	slot->ctx.power_on = pci_pwrctrl_slot_power_on;
 	slot->ctx.power_off = pci_pwrctrl_slot_power_off;
 
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 417a9ea59117..e9d519993853 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -17,25 +17,6 @@ static void pci_free_resources(struct pci_dev *dev)
 	}
 }
 
-static void pci_pwrctrl_unregister(struct device *dev)
-{
-	struct device_node *np;
-	struct platform_device *pdev;
-
-	np = dev_of_node(dev);
-	if (!np)
-		return;
-
-	pdev = of_find_device_by_node(np);
-	if (!pdev)
-		return;
-
-	of_device_unregister(pdev);
-	put_device(&pdev->dev);
-
-	of_node_clear_flag(np, OF_POPULATED);
-}
-
 static void pci_stop_dev(struct pci_dev *dev)
 {
 	pci_pme_active(dev, false);
@@ -73,7 +54,6 @@ static void pci_destroy_dev(struct pci_dev *dev)
 	pci_ide_destroy(dev);
 	pcie_aspm_exit_link_state(dev);
 	pci_bridge_d3_update(dev);
-	pci_pwrctrl_unregister(&dev->dev);
 	pci_free_resources(dev);
 	put_device(&dev->dev);
 }

-- 
2.48.1

Re: (subset) [PATCH v2 0/5] PCI/pwrctrl: Major rework to integrate pwrctrl devices with controller drivers

[Manivannan Sadhasivam]

On Tue, 16 Dec 2025 18:21:42 +0530, Manivannan Sadhasivam wrote:
> This series provides a major rework for the PCI power control (pwrctrl)
> framework to enable the pwrctrl devices to be controlled by the PCI controller
> drivers.
> 
> Problem Statement
> =================
> 
> [...]

Applied, thanks!

[1/5] PCI: qcom: Parse PERST# from all PCIe bridge nodes
      commit: 36777244652a7fe773bd7a1fe46bd3803712d3be

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

Re: [PATCH v2 0/5] PCI/pwrctrl: Major rework to integrate pwrctrl devices with controller drivers

[Sean Anderson]

Hi,

I have a few comments on the overall architecture. I did some work to
add PERST as well (sent as replies to this message).

On 12/16/25 07:51, Manivannan Sadhasivam wrote:
> Hi,
> 
> This series provides a major rework for the PCI power control (pwrctrl)
> framework to enable the pwrctrl devices to be controlled by the PCI controller
> drivers.
> 
> Problem Statement
> =================
> 
> Currently, the pwrctrl framework faces two major issues:
> 
> 1. Missing PERST# integration
> 2. Inability to properly handle bus extenders such as PCIe switch devices
> 
> First issue arises from the disconnect between the PCI controller drivers and
> pwrctrl framework. At present, the pwrctrl framework just operates on its own
> with the help of the PCI core. The pwrctrl devices are created by the PCI core
> during initial bus scan and the pwrctrl drivers once bind, just power on the
> PCI devices during their probe(). This design conflicts with the PCI Express
> Card Electromechanical Specification requirements for PERST# timing. The reason
> is, PERST# signals are mostly handled by the controller drivers and often
> deasserted even before the pwrctrl drivers probe. According to the spec, PERST#
> should be deasserted only after power and reference clock to the device are
> stable, within predefined timing parameters.
> 
> The second issue stems from the PCI bus scan completing before pwrctrl drivers
> probe. This poses a significant problem for PCI bus extenders like switches
> because the PCI core allocates upstream bridge resources during the initial
> scan. If the upstream bridge is not hotplug capable, resources are allocated
> only for the number of downstream buses detected at scan time, which might be
> just one if the switch was not powered and enumerated at that time. Later, when
> the pwrctrl driver powers on and enumerates the switch, enumeration fails due to
> insufficient upstream bridge resources.
>
>
> Proposal
> ========
> 
> This series addresses both issues by introducing new individual APIs for pwrctrl
> device creation, destruction, power on, and power off operations.

I wrote my own PCI power sequencing subsystem last year but didn't get
around to upstreaming it before the current subsystem was merged. This
approach (individual APIs for each power sequence) was how I did it. But
I think the approach to do everything in probe is rather elegant, since
it integrates with the existing device model and we don't need to modify
existing drivers.

To contrast, in U-Boot the second issue doesn't apply because driver
probing happens synchronously and config space accesses are done after
devices get probed. So you have something like

host bridge probe()
pci_scan_child_bus()
   discover root port
   root port probe()
      initialize slot resources (power supplies, clocks, etc.)
   allocate root port BARs
   discover root port children

I guess your approach is the only way to do it in Linux given the
asynchronous nature of driver binding. What is the overhead for hotplug
switches like? Maybe it makes sense to just treat all switches as
hotplug capable when PCI power sequencing is enabled?

> Controller
> drivers are expected to invoke these APIs during their probe(), remove(),
> suspend(), and resume() operations. This integration allows better coordination
> between controller drivers and the pwrctrl framework, enabling enhanced features
> such as D3Cold support.

How does PERST work? The only reference I can find to GPIOs in this
series is in the first patch. Is every driver supposed to add support
for PERST and then call these new functions? Shouldn't this be handled
by the power sequencing driver, especially as there are timing
requirements for the other resources referenced to PERST? IMO if we are
going to touch each driver, it would be much better to consolidate
things by removing the ad-hoc PERST support.

> The original design aimed to avoid modifying controller drivers for pwrctrl
> integration. However, this approach lacked scalability because different
> controllers have varying requirements for when devices should be powered on. For
> example, controller drivers require devices to be powered on early for
> successful PHY initialization.

Is this the case for qcom? The device I tested (nwl) was perfectly
happy to have the PCI device show up some time after the host bridge
got probed.

--Sean

> By using these explicit APIs, controller drivers gain fine grained control over
> their associated pwrctrl devices.
> 
> This series modified the pcie-qcom driver (only consumer of pwrctrl framework)
> to adopt to these APIs and also removed the old pwrctrl code from PCI core. This
> could be used as a reference to add pwrctrl support for other controller drivers
> also.
> 
> For example, to control the 3.3v supply to the PCI slot where the NVMe device is
> connected, below modifications are required:
> 
> Devicetree
> ----------
> 
> 	// In SoC dtsi:
> 
> 	pci@1bf8000 { // controller node
> 		...
> 		pcie1_port0: pcie@0 { // PCI Root Port node
> 			compatible = "pciclass,0604"; // required for pwrctrl
> 							 driver bind
> 			...
> 		};
> 	};
> 
> 	// In board dts:
> 
> 	&pcie1_port0 {
> 		reset-gpios = <&tlmm 152 GPIO_ACTIVE_LOW>; // optional
> 		vpcie3v3-supply = <&vreg_nvme>; // NVMe power supply
> 	};
> 
> Controller driver
> -----------------
> 
> 	// Select PCI_PWRCTRL_SLOT in controller Kconfig
> 
> 	probe() {
> 		...
> 		// Initialize controller resources
> 		pci_pwrctrl_create_devices(&pdev->dev);
> 		pci_pwrctrl_power_on_devices(&pdev->dev);
> 		// Deassert PERST# (optional)
> 		...
> 		pci_host_probe(); // Allocate host bridge and start bus scan
> 	}
> 
> 	suspend {
> 		// PME_Turn_Off broadcast
> 		// Assert PERST# (optional)
> 		pci_pwrctrl_power_off_devices(&pdev->dev);
> 		...
> 	}
> 
> 	resume {
> 		...
> 		pci_pwrctrl_power_on_devices(&pdev->dev);
> 		// Deassert PERST# (optional)
> 	}
> 
> I will add a documentation for the pwrctrl framework in the coming days to make
> it easier to use.
> 
> Testing
> =======
> 
> This series is tested on the Lenovo Thinkpad T14s laptop based on Qcom X1E
> chipset and RB3Gen2 development board with TC9563 switch based on Qcom QCS6490
> chipset.
> 
> **NOTE**: With this series, the controller driver may undergo multiple probe
> deferral if the pwrctrl driver was not available during the controller driver
> probe. This is pretty much required to avoid the resource allocation issue.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
> Changes in v2:
> - Exported of_pci_supply_present() API
> - Demoted the -EPROBE_DEFER log to dev_dbg()
> - Collected tags and rebased on top of v6.19-rc1
> - Link to v1: https://lore.kernel.org/r/20251124-pci-pwrctrl-rework-v1-0-78a72627683d@oss.qualcomm.com
> 
> ---
> Krishna Chaitanya Chundru (1):
>       PCI/pwrctrl: Add APIs for explicitly creating and destroying pwrctrl devices
> 
> Manivannan Sadhasivam (4):
>       PCI: qcom: Parse PERST# from all PCIe bridge nodes
>       PCI/pwrctrl: Add 'struct pci_pwrctrl::power_{on/off}' callbacks
>       PCI/pwrctrl: Add APIs to power on/off the pwrctrl devices
>       PCI/pwrctrl: Switch to the new pwrctrl APIs
> 
>  drivers/pci/controller/dwc/pcie-qcom.c   | 124 +++++++++++++---
>  drivers/pci/of.c                         |   1 +
>  drivers/pci/probe.c                      |  59 --------
>  drivers/pci/pwrctrl/core.c               | 248 +++++++++++++++++++++++++++++--
>  drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c |  30 +++-
>  drivers/pci/pwrctrl/slot.c               |  46 ++++--
>  drivers/pci/remove.c                     |  20 ---
>  include/linux/pci-pwrctrl.h              |  16 +-
>  8 files changed, 408 insertions(+), 136 deletions(-)
> ---
> base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
> change-id: 20251124-pci-pwrctrl-rework-c91a6e16c2f6
> 
> Best regards,

[PATCH 1/3] PCI/pwrctrl: Bind a pwrctrl device if clocks are present

[Sean Anderson]

Since commit 66db1d3cbdb0 ("PCI/pwrctrl: Add optional slot clock for PCI
slots"), power supplies are not the only resource PCI slots may create.
Also create a pwrctrl device if there are any clocks.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/pci/of.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 3579265f1198..07546a16ac86 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -847,7 +847,7 @@ void of_pci_make_host_bridge_node(struct pci_host_bridge *bridge)
  * @np: Device tree node
  *
  * Check if the power supply for the PCI device is present in the device tree
- * node or not.
+ * node or not. Clocks may also create a device.
  *
  * Return: true if at least one power supply exists; false otherwise.
  */
@@ -860,6 +860,9 @@ bool of_pci_supply_present(struct device_node *np)
 		return false;
 
 	for_each_property_of_node(np, prop) {
+		if (!strcmp(prop->name, "clocks"))
+			return true;
+
 		supply = strrchr(prop->name, '-');
 		if (supply && !strcmp(supply, "-supply"))
 			return true;
-- 
2.35.1.1320.gc452695387.dirty

[PATCH 2/3] PCI/pwrctrl: Add appropriate delays for slot power/clocks

[Sean Anderson]

Each of the PCIe electromechanical specifications requires a delay
between when power and clocks are stable and when PERST is released.
Delay for the specified time before continuing with initialization. If
there are no power supplies/clock, skip the associated delay as we
assume that they have been initialized by the bootloader (and that
booting up to this point has taken longer than the delay).

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/pci/pwrctrl/slot.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/pwrctrl/slot.c b/drivers/pci/pwrctrl/slot.c
index 3320494b62d8..1c56fcd49f2b 100644
--- a/drivers/pci/pwrctrl/slot.c
+++ b/drivers/pci/pwrctrl/slot.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -31,6 +32,7 @@ static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
 {
 	struct pci_pwrctrl_slot_data *slot;
 	struct device *dev = &pdev->dev;
+	unsigned long delay = 0;
 	struct clk *clk;
 	int ret;
 
@@ -64,6 +66,17 @@ static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
 				     "Failed to enable slot clock\n");
 	}
 
+	if (slot->num_supplies)
+		/*
+		 * Delay for T_PVPERL. This could be reduced to 1 ms/50 ms
+		 * (T_PVPGL) for Mini/M.2 slots.
+		 */
+		delay = 100000;
+	else if (clk)
+		/* Delay for T_PERST-CLK (100 us for all slot types) */
+		delay = 100;
+
+	fsleep(delay)
 	pci_pwrctrl_init(&slot->ctx, dev);
 
 	ret = devm_pci_pwrctrl_device_set_ready(dev, &slot->ctx);
-- 
2.35.1.1320.gc452695387.dirty

[PATCH 3/3] PCI/pwrctrl: Support PERST GPIO in slot driver

[Sean Anderson]

On many embedded platforms PERST is controlled by a GPIO. This was
traditionally handled by the host bridge. However, PERST may be released
before slot resources are initialized if there is a pwrctrl device. To
ensure we follow the power sequence, add support for controlling PERST
to the slot driver.

The host bridge could have already grabbed the GPIO. If this happens the
power sequence might be violated but there's really nothing we can do so
we just ignore the GPIO.

PERST must be asserted for at least T_PERST, such as when
entering/exiting S3/S4. As an optimization, we skip this delay when
PERST was already asserted, which may be the case when booting (such as
if the system has a pull-down on the line).

If the link is already up (e.g. the bootloader configured the power
supplies, clocks, and PERST) we will reset the link anyway. I don't
really know how to avoid this. I think we're OK because the root port
will be probed before we probe the endpoint so we shouldn't reset the
link while we're reading the configuration space.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/pci/pwrctrl/slot.c | 52 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pwrctrl/slot.c b/drivers/pci/pwrctrl/slot.c
index 1c56fcd49f2b..59dc92c4bc04 100644
--- a/drivers/pci/pwrctrl/slot.c
+++ b/drivers/pci/pwrctrl/slot.c
@@ -7,6 +7,7 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/gpio/consumer.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/pci-pwrctrl.h>
@@ -18,6 +19,7 @@ struct pci_pwrctrl_slot_data {
 	struct pci_pwrctrl ctx;
 	struct regulator_bulk_data *supplies;
 	int num_supplies;
+	struct gpio_desc *perst;
 };
 
 static void devm_pci_pwrctrl_slot_power_off(void *data)
@@ -28,6 +30,13 @@ static void devm_pci_pwrctrl_slot_power_off(void *data)
 	regulator_bulk_free(slot->num_supplies, slot->supplies);
 }
 
+static void devm_pci_pwrctrl_slot_assert_perst(void *data)
+{
+	struct pci_pwrctrl_slot_data *slot = data;
+
+	gpiod_set_value_cansleep(slot->perst, 1);
+}
+
 static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
 {
 	struct pci_pwrctrl_slot_data *slot;
@@ -66,6 +75,14 @@ static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
 				     "Failed to enable slot clock\n");
 	}
 
+	slot->perst = devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS);
+	if (IS_ERR(slot->perst)) {
+		/* The PCIe host bridge may have already grabbed the reset */
+		if (PTR_ERR(slot->perst) != -EBUSY)
+			return dev_err_probe(dev, ret, "failed to get PERST\n");
+		slot->perst = NULL;
+	}
+
 	if (slot->num_supplies)
 		/*
 		 * Delay for T_PVPERL. This could be reduced to 1 ms/50 ms
@@ -76,7 +93,40 @@ static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
 		/* Delay for T_PERST-CLK (100 us for all slot types) */
 		delay = 100;
 
-	fsleep(delay)
+	if (slot->perst) {
+		/*
+		 * If PERST is inactive, the following call to
+		 * gpiod_direction_output will be the first time we assert it
+		 * and we will need to delay for T_PERST.
+		 */
+		if (gpiod_get_value_cansleep(slot->perst) != 1)
+			delay = 100000;
+
+		ret = gpiod_direction_output(slot->perst, 1);
+		if (ret) {
+			dev_err(dev, "failed to assert PERST\n");
+			return ret;
+		}
+	}
+
+	fsleep(delay);
+	if (slot->perst) {
+		gpiod_set_value(slot->perst, 0);
+		ret = devm_add_action_or_reset(dev,
+					       devm_pci_pwrctrl_slot_assert_perst,
+					       slot);
+		if (ret)
+			return ret;
+
+		/*
+		 * PCIe section 6.6.1:
+		 * > ... software must wait a minimum of 100 ms before sending a
+		 * > Configuration Request to the device immediately below that
+		 * > Port.
+		 */
+		msleep(100);
+	}
+
 	pci_pwrctrl_init(&slot->ctx, dev);
 
 	ret = devm_pci_pwrctrl_device_set_ready(dev, &slot->ctx);
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH v2 5/5] PCI/pwrctrl: Switch to the new pwrctrl APIs

[Sean Anderson]

On 12/16/25 07:51, Manivannan Sadhasivam wrote:
> Adopt the recently introduced pwrctrl APIs to create, power on, destroy,
> and power off pwrctrl devices. In qcom_pcie_host_init(), call
> pci_pwrctrl_create_devices() to create devices, then
> pci_pwrctrl_power_on_devices() to power them on, both after controller
> resource initialization. Once successful, deassert PERST# for all devices.
>
> In qcom_pcie_host_deinit(), call pci_pwrctrl_power_off_devices() after
> asserting PERST#. Note that pci_pwrctrl_destroy_devices() is not called
> here, as deinit is only invoked during system suspend where device
> destruction is unnecessary. If the driver becomes removable in future,
> pci_pwrctrl_destroy_devices() should be called in the remove() handler.
>
> At last, remove the old pwrctrl framework code from the PCI core, as the
> new APIs are now the sole consumer of pwrctrl functionality. And also do
> not power on the pwrctrl drivers during probe() as this is now handled by
> the APIs.
>
> Co-developed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c   | 22 ++++++++++--
>  drivers/pci/probe.c                      | 59 --------------------------------
>  drivers/pci/pwrctrl/core.c               | 15 --------
>  drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c |  5 ---
>  drivers/pci/pwrctrl/slot.c               |  2 --
>  drivers/pci/remove.c                     | 20 -----------
>  6 files changed, 20 insertions(+), 103 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 73032449d289..7c0c66480f12 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -24,6 +24,7 @@
>  #include <linux/of_pci.h>
>  #include <linux/pci.h>
>  #include <linux/pci-ecam.h>
> +#include <linux/pci-pwrctrl.h>
>  #include <linux/pm_opp.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/platform_device.h>
> @@ -1318,10 +1319,18 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>       if (ret)
>               goto err_deinit;
>
> +     ret = pci_pwrctrl_create_devices(pci->dev);
> +     if (ret)
> +             goto err_disable_phy;
> +
> +     ret = pci_pwrctrl_power_on_devices(pci->dev);

Won't this result in a deferred probe loop if there is more than one
pwrctrl device and one has a driver loaded but the other does not?

deferred_probe_work_func()
  driver_probe_device(controller)
    qcom_pcie_probe(controller)
      pci_pwrctrl_create_devices()
        device_add(pwrctrl1)
          (probe successful)
          driver_deferred_probe_trigger()
        device_add(pwrctrl2)
          (driver not loaded)
      pci_pwrctrl_power_on_devices()
        return -EPROBE_DEFER
      pci_pwrctrl_destroy_devices()
        device_unregister(pwrctrl1)
        device_unregister(pwrctrl2)
    driver_deferred_probe_add(controller)
    // deferred_trigger_count changed, so...
    driver_deferred_probe_trigger()

And now you will continually probe the controller until all of the
drivers are loaded.

There is a non-obvious property of the deferred probe infrastructure
which is:

        Once a device creates children, it must never fail with
        EPROBE_DEFER.

So if you want to have something like this, the pwrctrl devices need to
be created before the controller is probed. Or you can use the current
system where the pwrctrl devices are probed asynchronously.

--Sean

> +     if (ret)
> +             goto err_pwrctrl_destroy;
> +
>       if (pcie->cfg->ops->post_init) {
>               ret = pcie->cfg->ops->post_init(pcie);
>               if (ret)
> -                     goto err_disable_phy;
> +                     goto err_pwrctrl_power_off;
>       }
>
>       qcom_ep_reset_deassert(pcie);
> @@ -1336,6 +1345,10 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>
>  err_assert_reset:
>       qcom_ep_reset_assert(pcie);
> +err_pwrctrl_power_off:
> +     pci_pwrctrl_power_off_devices(pci->dev);
> +err_pwrctrl_destroy:
> +     pci_pwrctrl_destroy_devices(pci->dev);
>  err_disable_phy:
>       qcom_pcie_phy_power_off(pcie);
>  err_deinit:
> @@ -1350,6 +1363,11 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
>       struct qcom_pcie *pcie = to_qcom_pcie(pci);
>
>       qcom_ep_reset_assert(pcie);
> +     /*
> +      * No need to destroy pwrctrl devices as this function only gets called
> +      * during system suspend as of now.
> +      */
> +     pci_pwrctrl_power_off_devices(pci->dev);
>       qcom_pcie_phy_power_off(pcie);
>       pcie->cfg->ops->deinit(pcie);
>  }
> @@ -2027,7 +2045,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>
>       ret = dw_pcie_host_init(pp);
>       if (ret) {
> -             dev_err(dev, "cannot initialize host\n");
> +             dev_err_probe(dev, ret, "cannot initialize host\n");
>               goto err_phy_exit;
>       }
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 41183aed8f5d..6e7252404b58 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2563,56 +2563,6 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
>  }
>  EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
>
> -#if IS_ENABLED(CONFIG_PCI_PWRCTRL)
> -static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
> -{
> -     struct pci_host_bridge *host = pci_find_host_bridge(bus);
> -     struct platform_device *pdev;
> -     struct device_node *np;
> -
> -     np = of_pci_find_child_device(dev_of_node(&bus->dev), devfn);
> -     if (!np)
> -             return NULL;
> -
> -     pdev = of_find_device_by_node(np);
> -     if (pdev) {
> -             put_device(&pdev->dev);
> -             goto err_put_of_node;
> -     }
> -
> -     /*
> -      * First check whether the pwrctrl device really needs to be created or
> -      * not. This is decided based on at least one of the power supplies
> -      * being defined in the devicetree node of the device.
> -      */
> -     if (!of_pci_supply_present(np)) {
> -             pr_debug("PCI/pwrctrl: Skipping OF node: %s\n", np->name);
> -             goto err_put_of_node;
> -     }
> -
> -     /* Now create the pwrctrl device */
> -     pdev = of_platform_device_create(np, NULL, &host->dev);
> -     if (!pdev) {
> -             pr_err("PCI/pwrctrl: Failed to create pwrctrl device for node: %s\n", np->name);
> -             goto err_put_of_node;
> -     }
> -
> -     of_node_put(np);
> -
> -     return pdev;
> -
> -err_put_of_node:
> -     of_node_put(np);
> -
> -     return NULL;
> -}
> -#else
> -static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
> -{
> -     return NULL;
> -}
> -#endif
> -
>  /*
>   * Read the config data for a PCI device, sanity-check it,
>   * and fill in the dev structure.
> @@ -2622,15 +2572,6 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
>       struct pci_dev *dev;
>       u32 l;
>
> -     /*
> -      * Create pwrctrl device (if required) for the PCI device to handle the
> -      * power state. If the pwrctrl device is created, then skip scanning
> -      * further as the pwrctrl core will rescan the bus after powering on
> -      * the device.
> -      */
> -     if (pci_pwrctrl_create_device(bus, devfn))
> -             return NULL;
> -
>       if (!pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
>               return NULL;
>
> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index ebe1740b7c1c..4e2c41bc4ffe 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c
> @@ -45,16 +45,6 @@ static int pci_pwrctrl_notify(struct notifier_block *nb, unsigned long action,
>       return NOTIFY_DONE;
>  }
>
> -static void rescan_work_func(struct work_struct *work)
> -{
> -     struct pci_pwrctrl *pwrctrl = container_of(work,
> -                                                struct pci_pwrctrl, work);
> -
> -     pci_lock_rescan_remove();
> -     pci_rescan_bus(to_pci_host_bridge(pwrctrl->dev->parent)->bus);
> -     pci_unlock_rescan_remove();
> -}
> -
>  /**
>   * pci_pwrctrl_init() - Initialize the PCI power control context struct
>   *
> @@ -64,7 +54,6 @@ static void rescan_work_func(struct work_struct *work)
>  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
>  {
>       pwrctrl->dev = dev;
> -     INIT_WORK(&pwrctrl->work, rescan_work_func);
>       dev_set_drvdata(dev, pwrctrl);
>  }
>  EXPORT_SYMBOL_GPL(pci_pwrctrl_init);
> @@ -95,8 +84,6 @@ int pci_pwrctrl_device_set_ready(struct pci_pwrctrl *pwrctrl)
>       if (ret)
>               return ret;
>
> -     schedule_work(&pwrctrl->work);
> -
>       return 0;
>  }
>  EXPORT_SYMBOL_GPL(pci_pwrctrl_device_set_ready);
> @@ -109,8 +96,6 @@ EXPORT_SYMBOL_GPL(pci_pwrctrl_device_set_ready);
>   */
>  void pci_pwrctrl_device_unset_ready(struct pci_pwrctrl *pwrctrl)
>  {
> -     cancel_work_sync(&pwrctrl->work);
> -
>       /*
>        * 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/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c b/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
> index 0fb9038a1d18..7697a8a5cdde 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
> @@ -101,11 +101,6 @@ static int pci_pwrctrl_pwrseq_probe(struct platform_device *pdev)
>               return dev_err_probe(dev, PTR_ERR(data->pwrseq),
>                                    "Failed to get the power sequencer\n");
>
> -     ret = pci_pwrctrl_pwrseq_power_on(&data->ctx);
> -     if (ret)
> -             return dev_err_probe(dev, ret,
> -                                  "Failed to power-on the device\n");
> -
>       ret = devm_add_action_or_reset(dev, devm_pci_pwrctrl_pwrseq_power_off,
>                                      data);
>       if (ret)
> diff --git a/drivers/pci/pwrctrl/slot.c b/drivers/pci/pwrctrl/slot.c
> index 14701f65f1f2..888300aeefec 100644
> --- a/drivers/pci/pwrctrl/slot.c
> +++ b/drivers/pci/pwrctrl/slot.c
> @@ -79,8 +79,6 @@ static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
>               return dev_err_probe(dev, PTR_ERR(slot->clk),
>                                    "Failed to enable slot clock\n");
>
> -     pci_pwrctrl_slot_power_on(&slot->ctx);
> -
>       slot->ctx.power_on = pci_pwrctrl_slot_power_on;
>       slot->ctx.power_off = pci_pwrctrl_slot_power_off;
>
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 417a9ea59117..e9d519993853 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -17,25 +17,6 @@ static void pci_free_resources(struct pci_dev *dev)
>       }
>  }
>
> -static void pci_pwrctrl_unregister(struct device *dev)
> -{
> -     struct device_node *np;
> -     struct platform_device *pdev;
> -
> -     np = dev_of_node(dev);
> -     if (!np)
> -             return;
> -
> -     pdev = of_find_device_by_node(np);
> -     if (!pdev)
> -             return;
> -
> -     of_device_unregister(pdev);
> -     put_device(&pdev->dev);
> -
> -     of_node_clear_flag(np, OF_POPULATED);
> -}
> -
>  static void pci_stop_dev(struct pci_dev *dev)
>  {
>       pci_pme_active(dev, false);
> @@ -73,7 +54,6 @@ static void pci_destroy_dev(struct pci_dev *dev)
>       pci_ide_destroy(dev);
>       pcie_aspm_exit_link_state(dev);
>       pci_bridge_d3_update(dev);
> -     pci_pwrctrl_unregister(&dev->dev);
>       pci_free_resources(dev);
>       put_device(&dev->dev);
>  }
>

[CES 2026, SECO]<https://exhibitors.ces.tech/8_0/exhibitor/exhibitor-details.cfm?exhid=001Pp000010EbaOIAS>

Re: [PATCH v2 0/5] PCI/pwrctrl: Major rework to integrate pwrctrl devices with controller drivers

[Manivannan Sadhasivam]

On Fri, Dec 19, 2025 at 12:19:36PM -0500, Sean Anderson wrote:
> Hi,
> 
> I have a few comments on the overall architecture. I did some work to
> add PERST as well (sent as replies to this message).
> 
> On 12/16/25 07:51, Manivannan Sadhasivam wrote:
> > Hi,
> > 
> > This series provides a major rework for the PCI power control (pwrctrl)
> > framework to enable the pwrctrl devices to be controlled by the PCI controller
> > drivers.
> > 
> > Problem Statement
> > =================
> > 
> > Currently, the pwrctrl framework faces two major issues:
> > 
> > 1. Missing PERST# integration
> > 2. Inability to properly handle bus extenders such as PCIe switch devices
> > 
> > First issue arises from the disconnect between the PCI controller drivers and
> > pwrctrl framework. At present, the pwrctrl framework just operates on its own
> > with the help of the PCI core. The pwrctrl devices are created by the PCI core
> > during initial bus scan and the pwrctrl drivers once bind, just power on the
> > PCI devices during their probe(). This design conflicts with the PCI Express
> > Card Electromechanical Specification requirements for PERST# timing. The reason
> > is, PERST# signals are mostly handled by the controller drivers and often
> > deasserted even before the pwrctrl drivers probe. According to the spec, PERST#
> > should be deasserted only after power and reference clock to the device are
> > stable, within predefined timing parameters.
> > 
> > The second issue stems from the PCI bus scan completing before pwrctrl drivers
> > probe. This poses a significant problem for PCI bus extenders like switches
> > because the PCI core allocates upstream bridge resources during the initial
> > scan. If the upstream bridge is not hotplug capable, resources are allocated
> > only for the number of downstream buses detected at scan time, which might be
> > just one if the switch was not powered and enumerated at that time. Later, when
> > the pwrctrl driver powers on and enumerates the switch, enumeration fails due to
> > insufficient upstream bridge resources.
> >
> >
> > Proposal
> > ========
> > 
> > This series addresses both issues by introducing new individual APIs for pwrctrl
> > device creation, destruction, power on, and power off operations.
> 
> I wrote my own PCI power sequencing subsystem last year but didn't get
> around to upstreaming it before the current subsystem was merged. This
> approach (individual APIs for each power sequence) was how I did it. But
> I think the approach to do everything in probe is rather elegant, since
> it integrates with the existing device model and we don't need to modify
> existing drivers.
> 
> To contrast, in U-Boot the second issue doesn't apply because driver
> probing happens synchronously and config space accesses are done after
> devices get probed. So you have something like
> 
> host bridge probe()
> pci_scan_child_bus()
>    discover root port
>    root port probe()
>       initialize slot resources (power supplies, clocks, etc.)
>    allocate root port BARs
>    discover root port children
> 
> I guess your approach is the only way to do it in Linux given the
> asynchronous nature of driver binding. What is the overhead for hotplug
> switches like? Maybe it makes sense to just treat all switches as
> hotplug capable when PCI power sequencing is enabled?
> 

Pwrctrl doesn't care if the underlying bridge is hotplug capable or not. It just
handles the power control for the device if it happens to have resource
dependency in DT. For example, if the PCIe switch requires pwrctrl and the
corresponding DT node has the resources described, pwrctrl framework will just
turn ON the switch. Then during PCI bus scan, PCI core will enumerate the switch
and check whether the downstream ports are hotplug capable or not and handles
the bus resource accordingly.

> > Controller
> > drivers are expected to invoke these APIs during their probe(), remove(),
> > suspend(), and resume() operations. This integration allows better coordination
> > between controller drivers and the pwrctrl framework, enabling enhanced features
> > such as D3Cold support.
> 
> How does PERST work? The only reference I can find to GPIOs in this
> series is in the first patch. Is every driver supposed to add support
> for PERST and then call these new functions?

Yes. We can come up with some generic controller specific APIs later to reduce
duplication, especially if GPIO is used for PERST#. But that's currently not in
scope for this series.

> Shouldn't this be handled
> by the power sequencing driver, especially as there are timing
> requirements for the other resources referenced to PERST? IMO if we are
> going to touch each driver, it would be much better to consolidate
> things by removing the ad-hoc PERST support.
> 

It is not that straightforward. Initially, my goal was to abstract pwrctrl away
from controller drivers, but then that didn't scale. Because, each controller
drivers have different requirement, some may use GPIO for PERST# and some use
MMIO. Also, some drivers do more work during the PERST# deassert, like checking
for Link up as in drivers/pci/controller/pci-tegra.c.

For sure, it would be doable to rework those drivers for pwrctrl, but that is
not practically possible and requires platform maintainer support. So to make
the pwrctrl adoption easier, I went with the explicit APIs that the drivers can
call from relevant places.

> > The original design aimed to avoid modifying controller drivers for pwrctrl
> > integration. However, this approach lacked scalability because different
> > controllers have varying requirements for when devices should be powered on. For
> > example, controller drivers require devices to be powered on early for
> > successful PHY initialization.
> 
> Is this the case for qcom? The device I tested (nwl) was perfectly
> happy to have the PCI device show up some time after the host bridge
> got probed.
> 

Not for Qcom, but some platforms do LTSSM during phy_init(), so they will fail
if the device is not powered ON at that time.

The challenge with the pwrctrl framework is that, it has to work across all
platforms and with the existing drivers without major rework. The initial design
worked for Qcom (somewhat), but I couldn't get it to scale across other
platforms.

- Mani

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

Re: [PATCH v2 5/5] PCI/pwrctrl: Switch to the new pwrctrl APIs

[Manivannan Sadhasivam]

On Fri, Dec 19, 2025 at 01:35:39PM -0500, Sean Anderson wrote:
> On 12/16/25 07:51, Manivannan Sadhasivam wrote:
> > Adopt the recently introduced pwrctrl APIs to create, power on, destroy,
> > and power off pwrctrl devices. In qcom_pcie_host_init(), call
> > pci_pwrctrl_create_devices() to create devices, then
> > pci_pwrctrl_power_on_devices() to power them on, both after controller
> > resource initialization. Once successful, deassert PERST# for all devices.
> >
> > In qcom_pcie_host_deinit(), call pci_pwrctrl_power_off_devices() after
> > asserting PERST#. Note that pci_pwrctrl_destroy_devices() is not called
> > here, as deinit is only invoked during system suspend where device
> > destruction is unnecessary. If the driver becomes removable in future,
> > pci_pwrctrl_destroy_devices() should be called in the remove() handler.
> >
> > At last, remove the old pwrctrl framework code from the PCI core, as the
> > new APIs are now the sole consumer of pwrctrl functionality. And also do
> > not power on the pwrctrl drivers during probe() as this is now handled by
> > the APIs.
> >
> > Co-developed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c   | 22 ++++++++++--
> >  drivers/pci/probe.c                      | 59 --------------------------------
> >  drivers/pci/pwrctrl/core.c               | 15 --------
> >  drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c |  5 ---
> >  drivers/pci/pwrctrl/slot.c               |  2 --
> >  drivers/pci/remove.c                     | 20 -----------
> >  6 files changed, 20 insertions(+), 103 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 73032449d289..7c0c66480f12 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/of_pci.h>
> >  #include <linux/pci.h>
> >  #include <linux/pci-ecam.h>
> > +#include <linux/pci-pwrctrl.h>
> >  #include <linux/pm_opp.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/platform_device.h>
> > @@ -1318,10 +1319,18 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> >       if (ret)
> >               goto err_deinit;
> >
> > +     ret = pci_pwrctrl_create_devices(pci->dev);
> > +     if (ret)
> > +             goto err_disable_phy;
> > +
> > +     ret = pci_pwrctrl_power_on_devices(pci->dev);
> 
> Won't this result in a deferred probe loop if there is more than one
> pwrctrl device and one has a driver loaded but the other does not?
> 
> deferred_probe_work_func()
>   driver_probe_device(controller)
>     qcom_pcie_probe(controller)
>       pci_pwrctrl_create_devices()
>         device_add(pwrctrl1)
>           (probe successful)
>           driver_deferred_probe_trigger()
>         device_add(pwrctrl2)
>           (driver not loaded)
>       pci_pwrctrl_power_on_devices()
>         return -EPROBE_DEFER
>       pci_pwrctrl_destroy_devices()
>         device_unregister(pwrctrl1)
>         device_unregister(pwrctrl2)
>     driver_deferred_probe_add(controller)
>     // deferred_trigger_count changed, so...
>     driver_deferred_probe_trigger()
> 
> And now you will continually probe the controller until all of the
> drivers are loaded.
> 
> There is a non-obvious property of the deferred probe infrastructure
> which is:
> 
>         Once a device creates children, it must never fail with
>         EPROBE_DEFER.
> 
> So if you want to have something like this, the pwrctrl devices need to
> be created before the controller is probed. Or you can use the current
> system where the pwrctrl devices are probed asynchronously.
> 

You are right and it is an oversight from me. If the pwrctrl driver is not
found, the pwrctrl devices should not be destroyed in the error path, but the
controller driver can still return -EPROBE_DEFER. This will allow the controller
driver to get reprobed later and by that time, pwrctrl device creation will be
skipped. I believe this satisfies the comment you quoted above.

I found this issue while testing the series with one of our Qcom switches and I
fixed it in yet to be submitted v3.

- Mani

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

Re: [PATCH v2 5/5] PCI/pwrctrl: Switch to the new pwrctrl APIs

[Bjorn Helgaas]

On Tue, Dec 23, 2025 at 07:41:30PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Dec 19, 2025 at 01:35:39PM -0500, Sean Anderson wrote:
> > On 12/16/25 07:51, Manivannan Sadhasivam wrote:
> > > Adopt the recently introduced pwrctrl APIs to create, power on, destroy,
> > > and power off pwrctrl devices. In qcom_pcie_host_init(), call
> > > pci_pwrctrl_create_devices() to create devices, then
> > > pci_pwrctrl_power_on_devices() to power them on, both after controller
> > > resource initialization. Once successful, deassert PERST# for all devices.

> ...
> > And now you will continually probe the controller until all of the
> > drivers are loaded.
> > 
> > There is a non-obvious property of the deferred probe infrastructure
> > which is:
> > 
> >         Once a device creates children, it must never fail with
> >         EPROBE_DEFER.
> > 
> > So if you want to have something like this, the pwrctrl devices need to
> > be created before the controller is probed. Or you can use the current
> > system where the pwrctrl devices are probed asynchronously.
> 
> You are right and it is an oversight from me. If the pwrctrl driver
> is not found, the pwrctrl devices should not be destroyed in the
> error path, but the controller driver can still return
> -EPROBE_DEFER. This will allow the controller driver to get reprobed
> later and by that time, pwrctrl device creation will be skipped. I
> believe this satisfies the comment you quoted above.
> 
> I found this issue while testing the series with one of our Qcom
> switches and I fixed it in yet to be submitted v3.

I guess I should wait for v3 before putting this in linux-next?

Re: [PATCH v2 1/5] PCI: qcom: Parse PERST# from all PCIe bridge nodes

[Bjorn Helgaas]

On Tue, Dec 16, 2025 at 06:21:43PM +0530, Manivannan Sadhasivam wrote:
> 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.

>  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);
>  }
> @@ -1702,18 +1710,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);

It looks like the perst->list will be ordered by distance from the
root, i.e., a Root Port first, followed by downstream devices?

And qcom_perst_assert() will assert/deassert PERST# in that same
order?  Intuitively I would have expected that if there are multiple
PERST# signals, we would assert them bottom-up, and deassert them
top-down.  Does the order matter?

I suppose maybe you plan to enhance pwrctrl so it can assert/deassert
individual PERST# in the hierarchy?

I'm confused about qcom_perst_assert() because it's only called from
qcom_ep_reset_assert() and qcom_ep_reset_deassert(), which are only
called from qcom_pcie_assert_perst().  Seems like a mix of host and
endpoint situation.  I assumed pwrctrl would be used on the host.
Maybe the "_ep_" names are not quite right?  Or more likely I'm just
misunderstanding the plan.

I notice you'd only applied this patch (1/5) so far on
pci/controller/dwc-qcom.  Is this patch useful by itself?

> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

Re: [PATCH v2 1/5] PCI: qcom: Parse PERST# from all PCIe bridge nodes

[Manivannan Sadhasivam]

On Fri, Dec 26, 2025 at 05:24:58PM -0600, Bjorn Helgaas wrote:
> On Tue, Dec 16, 2025 at 06:21:43PM +0530, Manivannan Sadhasivam wrote:
> > 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.
> 
> >  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);
> >  }
> > @@ -1702,18 +1710,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);
> 
> It looks like the perst->list will be ordered by distance from the
> root, i.e., a Root Port first, followed by downstream devices?
> 

Yes.

> And qcom_perst_assert() will assert/deassert PERST# in that same
> order?  Intuitively I would have expected that if there are multiple
> PERST# signals, we would assert them bottom-up, and deassert them
> top-down.  Does the order matter?
> 

I did't give much importance to the PERST# ordering since it doesn't matter,
atleast per base/electromechanical specs.

> I suppose maybe you plan to enhance pwrctrl so it can assert/deassert
> individual PERST# in the hierarchy?
> 

No, that plan has been dropped for good. For now, PERST# will be handled
entirely by the controller drivers. Sharing the PERST# handling with pwrctrl
proved to be a pain and it looks more clean (after the API introduction) to
handle PERST# in controller drivers.

> I'm confused about qcom_perst_assert() because it's only called from
> qcom_ep_reset_assert() and qcom_ep_reset_deassert(), which are only
> called from qcom_pcie_assert_perst().  Seems like a mix of host and
> endpoint situation.  I assumed pwrctrl would be used on the host.
> Maybe the "_ep_" names are not quite right?  Or more likely I'm just
> misunderstanding the plan.
> 

Yeah, it is a bit confusing now due to the introduction of the
'dw_pcie_ops::assert_perst' callback. But it will go away at the end of the
pwrctrl rework series and I'll fix those names by then.

> I notice you'd only applied this patch (1/5) so far on
> pci/controller/dwc-qcom.  Is this patch useful by itself?
> 

Yes, ofc. This patch allows the controller driver to parse and assert/deassert
PERST# from all PCI nodes, not just from the Root Port node.

- Mani

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

Re: [PATCH v2 5/5] PCI/pwrctrl: Switch to the new pwrctrl APIs

[Manivannan Sadhasivam]

On Fri, Dec 26, 2025 at 05:07:11PM -0600, Bjorn Helgaas wrote:
> On Tue, Dec 23, 2025 at 07:41:30PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Dec 19, 2025 at 01:35:39PM -0500, Sean Anderson wrote:
> > > On 12/16/25 07:51, Manivannan Sadhasivam wrote:
> > > > Adopt the recently introduced pwrctrl APIs to create, power on, destroy,
> > > > and power off pwrctrl devices. In qcom_pcie_host_init(), call
> > > > pci_pwrctrl_create_devices() to create devices, then
> > > > pci_pwrctrl_power_on_devices() to power them on, both after controller
> > > > resource initialization. Once successful, deassert PERST# for all devices.
> 
> > ...
> > > And now you will continually probe the controller until all of the
> > > drivers are loaded.
> > > 
> > > There is a non-obvious property of the deferred probe infrastructure
> > > which is:
> > > 
> > >         Once a device creates children, it must never fail with
> > >         EPROBE_DEFER.
> > > 
> > > So if you want to have something like this, the pwrctrl devices need to
> > > be created before the controller is probed. Or you can use the current
> > > system where the pwrctrl devices are probed asynchronously.
> > 
> > You are right and it is an oversight from me. If the pwrctrl driver
> > is not found, the pwrctrl devices should not be destroyed in the
> > error path, but the controller driver can still return
> > -EPROBE_DEFER. This will allow the controller driver to get reprobed
> > later and by that time, pwrctrl device creation will be skipped. I
> > believe this satisfies the comment you quoted above.
> > 
> > I found this issue while testing the series with one of our Qcom
> > switches and I fixed it in yet to be submitted v3.
> 
> I guess I should wait for v3 before putting this in linux-next?

Yes, please. I'll send v4 today or tomorrow.

- Mani

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

Re: [PATCH 1/3] PCI/pwrctrl: Bind a pwrctrl device if clocks are present

[Bartosz Golaszewski]

On Fri, Dec 19, 2025 at 6:23 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>
> Since commit 66db1d3cbdb0 ("PCI/pwrctrl: Add optional slot clock for PCI
> slots"), power supplies are not the only resource PCI slots may create.
> Also create a pwrctrl device if there are any clocks.
>
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
>

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Re: [PATCH v2 0/5] PCI/pwrctrl: Major rework to integrate pwrctrl devices with controller drivers

[Sean Anderson]

On 12/23/25 09:05, Manivannan Sadhasivam wrote:
> On Fri, Dec 19, 2025 at 12:19:36PM -0500, Sean Anderson wrote:
>> Hi,
>> 
>> I have a few comments on the overall architecture. I did some work to
>> add PERST as well (sent as replies to this message).
>> 
>> On 12/16/25 07:51, Manivannan Sadhasivam wrote:
>> > Hi,
>> > 
>> > This series provides a major rework for the PCI power control (pwrctrl)
>> > framework to enable the pwrctrl devices to be controlled by the PCI controller
>> > drivers.
>> > 
>> > Problem Statement
>> > =================
>> > 
>> > Currently, the pwrctrl framework faces two major issues:
>> > 
>> > 1. Missing PERST# integration
>> > 2. Inability to properly handle bus extenders such as PCIe switch devices
>> > 
>> > First issue arises from the disconnect between the PCI controller drivers and
>> > pwrctrl framework. At present, the pwrctrl framework just operates on its own
>> > with the help of the PCI core. The pwrctrl devices are created by the PCI core
>> > during initial bus scan and the pwrctrl drivers once bind, just power on the
>> > PCI devices during their probe(). This design conflicts with the PCI Express
>> > Card Electromechanical Specification requirements for PERST# timing. The reason
>> > is, PERST# signals are mostly handled by the controller drivers and often
>> > deasserted even before the pwrctrl drivers probe. According to the spec, PERST#
>> > should be deasserted only after power and reference clock to the device are
>> > stable, within predefined timing parameters.
>> > 
>> > The second issue stems from the PCI bus scan completing before pwrctrl drivers
>> > probe. This poses a significant problem for PCI bus extenders like switches
>> > because the PCI core allocates upstream bridge resources during the initial
>> > scan. If the upstream bridge is not hotplug capable, resources are allocated
>> > only for the number of downstream buses detected at scan time, which might be
>> > just one if the switch was not powered and enumerated at that time. Later, when
>> > the pwrctrl driver powers on and enumerates the switch, enumeration fails due to
>> > insufficient upstream bridge resources.
>> >
>> >
>> > Proposal
>> > ========
>> > 
>> > This series addresses both issues by introducing new individual APIs for pwrctrl
>> > device creation, destruction, power on, and power off operations.
>> 
>> I wrote my own PCI power sequencing subsystem last year but didn't get
>> around to upstreaming it before the current subsystem was merged. This
>> approach (individual APIs for each power sequence) was how I did it. But
>> I think the approach to do everything in probe is rather elegant, since
>> it integrates with the existing device model and we don't need to modify
>> existing drivers.
>> 
>> To contrast, in U-Boot the second issue doesn't apply because driver
>> probing happens synchronously and config space accesses are done after
>> devices get probed. So you have something like
>> 
>> host bridge probe()
>> pci_scan_child_bus()
>>    discover root port
>>    root port probe()
>>       initialize slot resources (power supplies, clocks, etc.)
>>    allocate root port BARs
>>    discover root port children
>> 
>> I guess your approach is the only way to do it in Linux given the
>> asynchronous nature of driver binding. What is the overhead for hotplug
>> switches like? Maybe it makes sense to just treat all switches as
>> hotplug capable when PCI power sequencing is enabled?
>> 
> 
> Pwrctrl doesn't care if the underlying bridge is hotplug capable or not. It just
> handles the power control for the device if it happens to have resource
> dependency in DT. For example, if the PCIe switch requires pwrctrl and the
> corresponding DT node has the resources described, pwrctrl framework will just
> turn ON the switch. Then during PCI bus scan, PCI core will enumerate the switch
> and check whether the downstream ports are hotplug capable or not and handles
> the bus resource accordingly.
> 

I'm saying that we allocate enough address space for each bridge as if
it was hotplug capable, to ensure we don't run out of space later on.

>> > Controller
>> > drivers are expected to invoke these APIs during their probe(), remove(),
>> > suspend(), and resume() operations. This integration allows better coordination
>> > between controller drivers and the pwrctrl framework, enabling enhanced features
>> > such as D3Cold support.
>> 
>> How does PERST work? The only reference I can find to GPIOs in this
>> series is in the first patch. Is every driver supposed to add support
>> for PERST and then call these new functions?
> 
> Yes. We can come up with some generic controller specific APIs later to reduce
> duplication, especially if GPIO is used for PERST#. But that's currently not in
> scope for this series.
> 
>> Shouldn't this be handled
>> by the power sequencing driver, especially as there are timing
>> requirements for the other resources referenced to PERST? IMO if we are
>> going to touch each driver, it would be much better to consolidate
>> things by removing the ad-hoc PERST support.
>> 
> 
> It is not that straightforward. Initially, my goal was to abstract pwrctrl away
> from controller drivers, but then that didn't scale. Because, each controller
> drivers have different requirement, some may use GPIO for PERST# and some use
> MMIO.

Can't you resolve that by exposing a GPIO controller for the signal?

Of course for compatibility we will want to support old devicetrees, so
maybe we should just add a method on the host bridge to control PERST.
And then pwrctrl could try to use it as a fallback.

> Also, some drivers do more work during the PERST# deassert, like checking
> for Link up as in drivers/pci/controller/pci-tegra.c.

Yeah, but is this actually necessary? We should just be able to toggle
the reset and then try to read a config register after 100 ms. If the
link is up the access will succeed. If it's down at that point then the
device doesn't follow the PCIe spec and we can't really be sure the link
will ever come up. The relevant code even has a comment

 * FIXME: If there are no PCIe cards attached, then calling this function
 * can result in the increase of the bootup time as there are big timeout
 * loops.

Which would be avoided by delaying asynchronously in the pwrseq driver.

> For sure, it would be doable to rework those drivers for pwrctrl, but that is
> not practically possible and requires platform maintainer support. So to make
> the pwrctrl adoption easier, I went with the explicit APIs that the drivers can
> call from relevant places.
> 
>> > The original design aimed to avoid modifying controller drivers for pwrctrl
>> > integration. However, this approach lacked scalability because different
>> > controllers have varying requirements for when devices should be powered on. For
>> > example, controller drivers require devices to be powered on early for
>> > successful PHY initialization.
>> 
>> Is this the case for qcom? The device I tested (nwl) was perfectly
>> happy to have the PCI device show up some time after the host bridge
>> got probed.
>> 
> 
> Not for Qcom, but some platforms do LTSSM during phy_init(), so they will fail
> if the device is not powered ON at that time.

Do you have a specific driver in mind?

> The challenge with the pwrctrl framework is that, it has to work across all
> platforms and with the existing drivers without major rework. The initial design
> worked for Qcom (somewhat), but I couldn't get it to scale across other
> platforms.

--Sean