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

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

[Manivannan Sadhasivam via B4 Relay]

Hi,

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

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

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

Proposal
========

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

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

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

For reference, I've implemented the callback in the Qcom RC driver where it just
toggles PERST# and implements the delay as per the CEM spec (which seem to
satisfy the delay requirements of other electromechanical specs also). Since the
Qcom driver supports both legacy DT binding (all Root Port properties in host
bridge node) and new binding (Root Port properies in Root Port node), I've moved
the PERST# handling to pwrctrl driver only if new binding is used. A recently
merged patch to PCI tree [2] makes sure that the pwrctrl slot driver is selected
with the RC driver.

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

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

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

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

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

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

Testing
=======

This series is tested on Qcom X1E based T14s laptop, SM8250 based RB5 board, and
QCS6490 based RB3Gen2 board with both legacy and new DT binding.

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

Changes since RFC:

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

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
Manivannan Sadhasivam (6):
      PCI: qcom: Wait for PCIE_RESET_CONFIG_WAIT_MS after PERST# deassert
      PCI/pwrctrl: Move pci_pwrctrl_init() before turning ON the supplies
      PCI/pwrctrl: Add support for toggling PERST#
      PCI: of: Add an API to get the BDF for the device node
      PCI: qcom: Parse PERST# from all PCIe bridge nodes
      PCI: qcom: Allow pwrctrl core to toggle PERST# for new DT binding

 drivers/pci/controller/dwc/pcie-qcom.c   | 166 +++++++++++++++++++++++++------
 drivers/pci/of.c                         |  21 ++++
 drivers/pci/pwrctrl/core.c               |  27 +++++
 drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c |   4 +-
 drivers/pci/pwrctrl/slot.c               |   4 +-
 include/linux/of_pci.h                   |   6 ++
 include/linux/pci.h                      |   1 +
 7 files changed, 197 insertions(+), 32 deletions(-)
---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250818-pci-pwrctrl-perst-0bb7dd62b542

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

[PATCH 1/6] PCI: qcom: Wait for PCIE_RESET_CONFIG_WAIT_MS after PERST# deassert

[Manivannan Sadhasivam via B4 Relay]

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

PCIe spec r6.0, sec 6.6.1 mandates waiting for 100ms before deasserting
PERST# if the downstream port does not support Link speeds greater than
5.0 GT/s. But in practice, this delay seem to be required irrespective of
the supported link speed as it gives the endpoints enough time to
initialize.

Hence, add the delay by reusing the PCIE_RESET_CONFIG_WAIT_MS definition if
the linkup_irq is not supported. If the linkup_irq is supported, the driver
already waits for 100ms in the IRQ handler post link up.

Also, remove the redundant comment for PCIE_T_PVPERL_MS. Finally, the
PERST_DELAY_US sleep can be moved to PERST# assert where it should be.

Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..bcd080315d70e64eafdefd852740fe07df3dbe75 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -302,20 +302,22 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
 	else
 		list_for_each_entry(port, &pcie->ports, list)
 			gpiod_set_value_cansleep(port->reset, val);
-
-	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
 }
 
 static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
 {
 	qcom_perst_assert(pcie, true);
+	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
 }
 
 static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
 {
-	/* Ensure that PERST has been asserted for at least 100 ms */
+	struct dw_pcie_rp *pp = &pcie->pci->pp;
+
 	msleep(PCIE_T_PVPERL_MS);
 	qcom_perst_assert(pcie, false);
+	if (!pp->use_linkup_irq)
+		msleep(PCIE_RESET_CONFIG_WAIT_MS);
 }
 
 static int qcom_pcie_start_link(struct dw_pcie *pci)

-- 
2.45.2

[PATCH 2/6] PCI/pwrctrl: Move pci_pwrctrl_init() before turning ON the supplies

[Manivannan Sadhasivam via B4 Relay]

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

To allow pwrctrl core to parse the generic resources such as PERST# GPIO
before turning on the supplies.

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c | 4 ++--
 drivers/pci/pwrctrl/slot.c               | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c b/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
index 4e664e7b8dd23f592c0392efbf6728fc5bf9093f..b65955adc7bd44030593e8c49d60db0f39b03d03 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
@@ -80,6 +80,8 @@ static int pci_pwrctrl_pwrseq_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENOMEM;
 
+	pci_pwrctrl_init(&data->ctx, dev);
+
 	data->pwrseq = devm_pwrseq_get(dev, pdata->target);
 	if (IS_ERR(data->pwrseq))
 		return dev_err_probe(dev, PTR_ERR(data->pwrseq),
@@ -95,8 +97,6 @@ static int pci_pwrctrl_pwrseq_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	pci_pwrctrl_init(&data->ctx, dev);
-
 	ret = devm_pci_pwrctrl_device_set_ready(dev, &data->ctx);
 	if (ret)
 		return dev_err_probe(dev, ret,
diff --git a/drivers/pci/pwrctrl/slot.c b/drivers/pci/pwrctrl/slot.c
index 6e138310b45b9f7e930b6814e0a24f7111d25fee..b68406a6b027e4d9f853e86d4340e0ab267b6126 100644
--- a/drivers/pci/pwrctrl/slot.c
+++ b/drivers/pci/pwrctrl/slot.c
@@ -38,6 +38,8 @@ static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
 	if (!slot)
 		return -ENOMEM;
 
+	pci_pwrctrl_init(&slot->ctx, dev);
+
 	ret = of_regulator_bulk_get_all(dev, dev_of_node(dev),
 					&slot->supplies);
 	if (ret < 0) {
@@ -63,8 +65,6 @@ static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
 				     "Failed to enable slot clock\n");
 	}
 
-	pci_pwrctrl_init(&slot->ctx, dev);
-
 	ret = devm_pci_pwrctrl_device_set_ready(dev, &slot->ctx);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to register pwrctrl driver\n");

-- 
2.45.2

[PATCH 3/6] PCI/pwrctrl: Add support for toggling PERST#

[Manivannan Sadhasivam via B4 Relay]

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

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

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

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

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

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

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

-- 
2.45.2

[PATCH 4/6] PCI: of: Add an API to get the BDF for the device node

[Manivannan Sadhasivam via B4 Relay]

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

Bus:Device:Function (BDF) numbers are used to uniquely identify a
device/function on a PCI bus. Hence, add an API to get the BDF from the
devicetree node of a device.

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

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 3579265f119845637e163d9051437c89662762f8..5e584d25434291430145e510b1d49a371dce9165 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -183,6 +183,27 @@ int of_pci_get_devfn(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(of_pci_get_devfn);
 
+/**
+ * of_pci_get_bdf() - Get Bus:Device:Function (BDF) numbers for a device node
+ * @np: device node
+ *
+ * Parses a standard 5-cell PCI resource and returns an 16-bit value that
+ * corresponds to the BDF of the node. On error, a negative error code is
+ * returned.
+ */
+int of_pci_get_bdf(struct device_node *np)
+{
+	u32 reg[5];
+	int error;
+
+	error = of_property_read_u32_array(np, "reg", reg, ARRAY_SIZE(reg));
+	if (error)
+		return error;
+
+	return (reg[0] >> 8) & 0xffff;
+}
+EXPORT_SYMBOL_GPL(of_pci_get_bdf);
+
 /**
  * of_pci_parse_bus_range() - parse the bus-range property of a PCI device
  * @node: device node
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index 29658c0ee71ff10122760214d04ee2bab01709fd..b36ac10653c82f4efdbb57cea56d2ec9d581212f 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -12,6 +12,7 @@ struct device_node;
 struct device_node *of_pci_find_child_device(struct device_node *parent,
 					     unsigned int devfn);
 int of_pci_get_devfn(struct device_node *np);
+int of_pci_get_bdf(struct device_node *np);
 void of_pci_check_probe_only(void);
 #else
 static inline struct device_node *of_pci_find_child_device(struct device_node *parent,
@@ -25,6 +26,11 @@ static inline int of_pci_get_devfn(struct device_node *np)
 	return -EINVAL;
 }
 
+static inline int of_pci_get_bdf(struct device_node *np)
+{
+	return -EINVAL;
+}
+
 static inline void of_pci_check_probe_only(void) { }
 #endif
 

-- 
2.45.2

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

[Manivannan Sadhasivam via B4 Relay]

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

Devicetree schema allows the PERST# GPIO to be present in all PCIe bridge
nodes, not just in Root Port node. But the current logic parses PERST# only
from the Root Port node. 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 Root Port node. If the 'reset-gpios' property is found for a node, the
GPIO descriptor will be stored in IDR structure with node BDF as the ID.

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

So for now, PERST# sharing is not supported.

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

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index bcd080315d70e64eafdefd852740fe07df3dbe75..5d73c46095af3219687ff77e5922f08bb41e43a9 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -19,6 +19,7 @@
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/limits.h>
+#include <linux/idr.h>
 #include <linux/init.h>
 #include <linux/of.h>
 #include <linux/of_pci.h>
@@ -286,6 +287,7 @@ struct qcom_pcie {
 	const struct qcom_pcie_cfg *cfg;
 	struct dentry *debugfs;
 	struct list_head ports;
+	struct idr perst;
 	bool suspended;
 	bool use_pm_opp;
 };
@@ -294,14 +296,15 @@ struct qcom_pcie {
 
 static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
 {
-	struct qcom_pcie_port *port;
 	int val = assert ? 1 : 0;
+	struct gpio_desc *perst;
+	int bdf;
 
-	if (list_empty(&pcie->ports))
+	if (idr_is_empty(&pcie->perst))
 		gpiod_set_value_cansleep(pcie->reset, val);
-	else
-		list_for_each_entry(port, &pcie->ports, list)
-			gpiod_set_value_cansleep(port->reset, val);
+
+	idr_for_each_entry(&pcie->perst, perst, bdf)
+		gpiod_set_value_cansleep(perst, val);
 }
 
 static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
@@ -1702,20 +1705,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 device_node *np)
 {
 	struct device *dev = pcie->pci->dev;
-	struct qcom_pcie_port *port;
 	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;
+
+	ret = of_pci_get_bdf(np);
+	if (ret < 0)
+		return ret;
+
+	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);
+	}
+
+	ret = idr_alloc(&pcie->perst, reset, ret, 0, GFP_KERNEL);
+	if (ret < 0)
+		return ret;
+
+parse_child_node:
+	for_each_available_child_of_node_scoped(np, child) {
+		ret = qcom_pcie_parse_perst(pcie, child);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
 
-	phy = devm_of_phy_get(dev, node, NULL);
+static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *np)
+{
+	struct device *dev = pcie->pci->dev;
+	struct qcom_pcie_port *port;
+	struct phy *phy;
+	int ret;
+
+	phy = devm_of_phy_get(dev, np, NULL);
 	if (IS_ERR(phy))
 		return PTR_ERR(phy);
 
@@ -1727,7 +1768,10 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
 	if (ret)
 		return ret;
 
-	port->reset = reset;
+	ret = qcom_pcie_parse_perst(pcie, np);
+	if (ret)
+		return ret;
+
 	port->phy = phy;
 	INIT_LIST_HEAD(&port->list);
 	list_add_tail(&port->list, &pcie->ports);
@@ -1739,7 +1783,11 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 {
 	struct device *dev = pcie->pci->dev;
 	struct qcom_pcie_port *port, *tmp;
-	int ret = -ENOENT;
+	struct gpio_desc *perst;
+	int ret = -ENODEV;
+	int bdf;
+
+	idr_init(&pcie->perst);
 
 	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
 		ret = qcom_pcie_parse_port(pcie, of_port);
@@ -1750,8 +1798,13 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 	return ret;
 
 err_port_del:
-	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		phy_exit(port->phy);
 		list_del(&port->list);
+	}
+
+	idr_for_each_entry(&pcie->perst, perst, bdf)
+		idr_remove(&pcie->perst, bdf);
 
 	return ret;
 }
@@ -1782,12 +1835,13 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	unsigned long max_freq = ULONG_MAX;
 	struct qcom_pcie_port *port, *tmp;
 	struct device *dev = &pdev->dev;
+	struct gpio_desc *perst;
 	struct dev_pm_opp *opp;
 	struct qcom_pcie *pcie;
 	struct dw_pcie_rp *pp;
 	struct resource *res;
 	struct dw_pcie *pci;
-	int ret, irq;
+	int ret, irq, bdf;
 	char *name;
 
 	pcie_cfg = of_device_get_match_data(dev);
@@ -1927,7 +1981,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;
@@ -1989,6 +2043,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	qcom_pcie_phy_exit(pcie);
 	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
 		list_del(&port->list);
+	idr_for_each_entry(&pcie->perst, perst, bdf)
+		idr_remove(&pcie->perst, bdf);
 err_pm_runtime_put:
 	pm_runtime_put(dev);
 	pm_runtime_disable(dev);

-- 
2.45.2

[PATCH 6/6] PCI: qcom: Allow pwrctrl core to toggle PERST# for new DT binding

[Manivannan Sadhasivam via B4 Relay]

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

If the platform is using the new DT binding, let the pwrctrl core toggle
PERST# for the device. This is achieved by populating the
'pci_host_bridge::toggle_perst' callback with qcom_pcie_toggle_perst().

qcom_pcie_toggle_perst() will find the PERST# GPIO descriptor associated
with the supplied 'device_node' and toggle PERST#. If PERST# is not found
in the supplied node, the function will look for PERST# in the parent node
as a fallback. This is needed since PERST# won't be available in the
endpoint node as per the DT binding.

Note that the driver still asserts PERST# during the controller
initialization as it is needed as per the hardware documentation. Apart
from that, the driver wouldn't touch PERST# for the new binding.

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

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 5d73c46095af3219687ff77e5922f08bb41e43a9..fd07cd493f9fb974acfc924778c7a5e980630ae6 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -294,12 +294,44 @@ struct qcom_pcie {
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
 
-static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
+static void qcom_toggle_perst_per_device(struct qcom_pcie *pcie,
+					 struct device_node *np, int val)
+{
+	struct gpio_desc *perst;
+	int bdf;
+
+	bdf = of_pci_get_bdf(np);
+	if (bdf < 0)
+		return;
+
+	perst = idr_find(&pcie->perst, bdf);
+	if (perst)
+		goto toggle_perst;
+
+	/*
+	 * If PERST# is not available in the current node, try the parent. This
+	 * fallback is needed if the current node belongs to an endpoint.
+	 */
+	bdf = of_pci_get_bdf(np->parent);
+	if (bdf < 0)
+		return;
+
+	perst = idr_find(&pcie->perst, bdf);
+toggle_perst:
+	/* gpiod* APIs handle NULL gpio_desc gracefully. So no need to check. */
+	gpiod_set_value_cansleep(perst, val);
+}
+
+static void qcom_perst_assert(struct qcom_pcie *pcie, struct device_node *np,
+			      bool assert)
 {
 	int val = assert ? 1 : 0;
 	struct gpio_desc *perst;
 	int bdf;
 
+	if (np)
+		return qcom_toggle_perst_per_device(pcie, np, val);
+
 	if (idr_is_empty(&pcie->perst))
 		gpiod_set_value_cansleep(pcie->reset, val);
 
@@ -307,22 +339,34 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
 		gpiod_set_value_cansleep(perst, val);
 }
 
-static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
+static void qcom_ep_reset_assert(struct qcom_pcie *pcie, struct device_node *np)
 {
-	qcom_perst_assert(pcie, true);
+	qcom_perst_assert(pcie, np, true);
 	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
 }
 
-static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
+static void qcom_ep_reset_deassert(struct qcom_pcie *pcie,
+				   struct device_node *np)
 {
 	struct dw_pcie_rp *pp = &pcie->pci->pp;
 
 	msleep(PCIE_T_PVPERL_MS);
-	qcom_perst_assert(pcie, false);
+	qcom_perst_assert(pcie, np, false);
 	if (!pp->use_linkup_irq)
 		msleep(PCIE_RESET_CONFIG_WAIT_MS);
 }
 
+static void qcom_pcie_toggle_perst(struct pci_host_bridge *bridge,
+				    struct device_node *np, bool assert)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(bridge->dev.parent);
+
+	if (assert)
+		qcom_ep_reset_assert(pcie, np);
+	else
+		qcom_ep_reset_deassert(pcie, np);
+}
+
 static int qcom_pcie_start_link(struct dw_pcie *pci)
 {
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
@@ -1317,7 +1361,7 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 	int ret;
 
-	qcom_ep_reset_assert(pcie);
+	qcom_ep_reset_assert(pcie, NULL);
 
 	ret = pcie->cfg->ops->init(pcie);
 	if (ret)
@@ -1333,7 +1377,13 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 			goto err_disable_phy;
 	}
 
-	qcom_ep_reset_deassert(pcie);
+	/*
+	 * Only deassert PERST# for all devices here if legacy binding is used.
+	 * For the new binding, pwrctrl driver is expected to toggle PERST# for
+	 * individual devices.
+	 */
+	if (idr_is_empty(&pcie->perst))
+		qcom_ep_reset_deassert(pcie, NULL);
 
 	if (pcie->cfg->ops->config_sid) {
 		ret = pcie->cfg->ops->config_sid(pcie);
@@ -1341,10 +1391,12 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 			goto err_assert_reset;
 	}
 
+	pci->pp.bridge->toggle_perst = qcom_pcie_toggle_perst;
+
 	return 0;
 
 err_assert_reset:
-	qcom_ep_reset_assert(pcie);
+	qcom_ep_reset_assert(pcie, NULL);
 err_disable_phy:
 	qcom_pcie_phy_power_off(pcie);
 err_deinit:
@@ -1358,7 +1410,7 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 
-	qcom_ep_reset_assert(pcie);
+	qcom_ep_reset_assert(pcie, NULL);
 	qcom_pcie_phy_power_off(pcie);
 	pcie->cfg->ops->deinit(pcie);
 }

-- 
2.45.2

Re: [PATCH 4/6] PCI: of: Add an API to get the BDF for the device node

[Rob Herring]

On Tue, Aug 19, 2025 at 12:44:53PM +0530, Manivannan Sadhasivam wrote:
> Bus:Device:Function (BDF) numbers are used to uniquely identify a
> device/function on a PCI bus. Hence, add an API to get the BDF from the
> devicetree node of a device.

For FDT, the bus should always be 0. It doesn't make sense for FDT. The 
bus number in DT reflects how firmware configured the PCI buses, but 
there's no firmware configuration of PCI for FDT.

Re: [PATCH 4/6] PCI: of: Add an API to get the BDF for the device node

[Manivannan Sadhasivam]

On Fri, Aug 22, 2025 at 08:51:47AM GMT, Rob Herring wrote:
> On Tue, Aug 19, 2025 at 12:44:53PM +0530, Manivannan Sadhasivam wrote:
> > Bus:Device:Function (BDF) numbers are used to uniquely identify a
> > device/function on a PCI bus. Hence, add an API to get the BDF from the
> > devicetree node of a device.
> 
> For FDT, the bus should always be 0. It doesn't make sense for FDT. The 
> bus number in DT reflects how firmware configured the PCI buses, but 
> there's no firmware configuration of PCI for FDT.

This API is targeted for DT platforms only, where it is used to uniquely
identify a devfn. What should I do to make it DT specific and not FDT?

- Mani

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

Re: [PATCH 4/6] PCI: of: Add an API to get the BDF for the device node

[Rob Herring]

On Fri, Aug 22, 2025 at 07:57:41PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Aug 22, 2025 at 08:51:47AM GMT, Rob Herring wrote:
> > On Tue, Aug 19, 2025 at 12:44:53PM +0530, Manivannan Sadhasivam wrote:
> > > Bus:Device:Function (BDF) numbers are used to uniquely identify a
> > > device/function on a PCI bus. Hence, add an API to get the BDF from the
> > > devicetree node of a device.
> > 
> > For FDT, the bus should always be 0. It doesn't make sense for FDT. The 
> > bus number in DT reflects how firmware configured the PCI buses, but 
> > there's no firmware configuration of PCI for FDT.
> 
> This API is targeted for DT platforms only, where it is used to uniquely
> identify a devfn. What should I do to make it DT specific and not FDT?

I don't understand. There are FDT and OF (actual OpenFirmware) 
platforms. I'm pretty sure you don't care about the latter.

Rob

Re: [PATCH 4/6] PCI: of: Add an API to get the BDF for the device node

[Manivannan Sadhasivam]

On Mon, Aug 25, 2025 at 05:43:15PM GMT, Rob Herring wrote:
> On Fri, Aug 22, 2025 at 07:57:41PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Aug 22, 2025 at 08:51:47AM GMT, Rob Herring wrote:
> > > On Tue, Aug 19, 2025 at 12:44:53PM +0530, Manivannan Sadhasivam wrote:
> > > > Bus:Device:Function (BDF) numbers are used to uniquely identify a
> > > > device/function on a PCI bus. Hence, add an API to get the BDF from the
> > > > devicetree node of a device.
> > > 
> > > For FDT, the bus should always be 0. It doesn't make sense for FDT. The 
> > > bus number in DT reflects how firmware configured the PCI buses, but 
> > > there's no firmware configuration of PCI for FDT.
> > 
> > This API is targeted for DT platforms only, where it is used to uniquely
> > identify a devfn. What should I do to make it DT specific and not FDT?
> 
> I don't understand. There are FDT and OF (actual OpenFirmware) 
> platforms. I'm pretty sure you don't care about the latter.
> 

Sorry, I mixed the terminologies. Yes, I did refer the platforms making use of
the FDT binary and not OF platforms.

In the DTS, we do use bus number to differentiate between devices, not just
devfn. But I get your point, bus no other than 0 are not fixed and allocated by
the OS during runtime or by the firmware.

So how should we uniquely identify a PCIe node here, if not by BDF?

- Mani

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

Re: [PATCH 4/6] PCI: of: Add an API to get the BDF for the device node

[Rob Herring]

On Tue, Aug 26, 2025 at 2:15 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
>
> On Mon, Aug 25, 2025 at 05:43:15PM GMT, Rob Herring wrote:
> > On Fri, Aug 22, 2025 at 07:57:41PM +0530, Manivannan Sadhasivam wrote:
> > > On Fri, Aug 22, 2025 at 08:51:47AM GMT, Rob Herring wrote:
> > > > On Tue, Aug 19, 2025 at 12:44:53PM +0530, Manivannan Sadhasivam wrote:
> > > > > Bus:Device:Function (BDF) numbers are used to uniquely identify a
> > > > > device/function on a PCI bus. Hence, add an API to get the BDF from the
> > > > > devicetree node of a device.
> > > >
> > > > For FDT, the bus should always be 0. It doesn't make sense for FDT. The
> > > > bus number in DT reflects how firmware configured the PCI buses, but
> > > > there's no firmware configuration of PCI for FDT.
> > >
> > > This API is targeted for DT platforms only, where it is used to uniquely
> > > identify a devfn. What should I do to make it DT specific and not FDT?
> >
> > I don't understand. There are FDT and OF (actual OpenFirmware)
> > platforms. I'm pretty sure you don't care about the latter.
> >
>
> Sorry, I mixed the terminologies. Yes, I did refer the platforms making use of
> the FDT binary and not OF platforms.
>
> In the DTS, we do use bus number to differentiate between devices, not just
> devfn. But I get your point, bus no other than 0 are not fixed and allocated by
> the OS during runtime or by the firmware.
>
> So how should we uniquely identify a PCIe node here, if not by BDF?

By path. Which is consistent since there is also no bus num in the unit-address.

Rob

Re: [PATCH 3/6] PCI/pwrctrl: Add support for toggling PERST#

[Bartosz Golaszewski]

On Tue, Aug 19, 2025 at 9:15 AM Manivannan Sadhasivam via B4 Relay
<devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> wrote:
>
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>
> As per PCIe spec r6.0, sec 6.6.1, PERST# is an auxiliary signal provided by
> the system to a component as a Fundamental Reset. This signal if available,
> should conform to the rules defined by the electromechanical form factor
> specifications like PCIe CEM spec r4.0, sec 2.2.
>
> Since pwrctrl driver is meant to control the power supplies, it should also
> control the PERST# signal if available. But traditionally, the host bridge
> (controller) drivers are the ones parsing and controlling the PERST#
> signal. They also sometimes need to assert PERST# during their own hardware
> initialization. So it is not possible to move the PERST# control away from
> the controller drivers and it must be shared logically.
>
> Hence, add a new callback 'pci_host_bridge::toggle_perst', that allows the
> pwrctrl core to toggle PERST# with the help of the controller drivers. But
> care must be taken care by the controller drivers to not deassert the
> PERST# signal if this callback is populated.
>
> This callback if available, will be called by the pwrctrl core during the
> device power up and power down scenarios. Controller drivers should
> identify the device using the 'struct device_node' passed during the
> callback and toggle PERST# accordingly.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/pwrctrl/core.c | 27 +++++++++++++++++++++++++++
>  include/linux/pci.h        |  1 +
>  2 files changed, 28 insertions(+)
>
> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index 6bdbfed584d6d79ce28ba9e384a596b065ca69a4..8a26f432436d064acb7ebbbc9ce8fc339909fbe9 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c
> @@ -6,6 +6,7 @@
>  #include <linux/device.h>
>  #include <linux/export.h>
>  #include <linux/kernel.h>
> +#include <linux/of.h>
>  #include <linux/pci.h>
>  #include <linux/pci-pwrctrl.h>
>  #include <linux/property.h>
> @@ -61,6 +62,28 @@ void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(pci_pwrctrl_init);
>
> +static void pci_pwrctrl_perst_deassert(struct pci_pwrctrl *pwrctrl)
> +{
> +       struct pci_host_bridge *host_bridge = to_pci_host_bridge(pwrctrl->dev->parent);
> +       struct device_node *np = dev_of_node(pwrctrl->dev);
> +
> +       if (!host_bridge->toggle_perst)
> +               return;
> +
> +       host_bridge->toggle_perst(host_bridge, np, false);
> +}
> +
> +static void pci_pwrctrl_perst_assert(struct pci_pwrctrl *pwrctrl)
> +{
> +       struct pci_host_bridge *host_bridge = to_pci_host_bridge(pwrctrl->dev->parent);
> +       struct device_node *np = dev_of_node(pwrctrl->dev);
> +
> +       if (!host_bridge->toggle_perst)
> +               return;
> +
> +       host_bridge->toggle_perst(host_bridge, np, true);
> +}
> +
>  /**
>   * pci_pwrctrl_device_set_ready() - Notify the pwrctrl subsystem that the PCI
>   * device is powered-up and ready to be detected.
> @@ -82,6 +105,8 @@ int pci_pwrctrl_device_set_ready(struct pci_pwrctrl *pwrctrl)
>         if (!pwrctrl->dev)
>                 return -ENODEV;
>
> +       pci_pwrctrl_perst_deassert(pwrctrl);
> +
>         pwrctrl->nb.notifier_call = pci_pwrctrl_notify;
>         ret = bus_register_notifier(&pci_bus_type, &pwrctrl->nb);
>         if (ret)
> @@ -103,6 +128,8 @@ void pci_pwrctrl_device_unset_ready(struct pci_pwrctrl *pwrctrl)
>  {
>         cancel_work_sync(&pwrctrl->work);
>
> +       pci_pwrctrl_perst_assert(pwrctrl);
> +
>         /*
>          * We don't have to delete the link here. Typically, this function
>          * is only called when the power control device is being detached. If
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 59876de13860dbe50ee6c207cd57e54f51a11079..9eeee84d550bb9f15a90b5db9da03fccef8097ee 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -605,6 +605,7 @@ struct pci_host_bridge {
>         void (*release_fn)(struct pci_host_bridge *);
>         int (*enable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
>         void (*disable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
> +       void (*toggle_perst)(struct pci_host_bridge *bridge, struct device_node *np, bool assert);

Shouldn't this be wrapped in an #if IS_ENABLED(PCI_PWRCTL)?

Bart

>         void            *release_data;
>         unsigned int    ignore_reset_delay:1;   /* For entire hierarchy */
>         unsigned int    no_ext_tags:1;          /* No Extended Tags */
>
> --
> 2.45.2
>
>

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

[Bartosz Golaszewski]

On Tue, Aug 19, 2025 at 9:15 AM Manivannan Sadhasivam via B4 Relay
<devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> wrote:
>
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>
> Devicetree schema allows the PERST# GPIO to be present in all PCIe bridge
> nodes, not just in Root Port node. But the current logic parses PERST# only
> from the Root Port node. 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 Root Port node. If the 'reset-gpios' property is found for a node, the
> GPIO descriptor will be stored in IDR structure with node BDF as the ID.
>
> It should be noted that if more than one bridge node has the same GPIO for
> PERST# (shared PERST#), the driver will error out. This is due to the
> limitation in the GPIOLIB subsystem that allows only exclusive (non-shared)
> access to GPIOs from consumers. But this is soon going to get fixed. Once
> that happens, it will get incorporated in this driver.
>
> So for now, PERST# sharing is not supported.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 90 +++++++++++++++++++++++++++-------
>  1 file changed, 73 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index bcd080315d70e64eafdefd852740fe07df3dbe75..5d73c46095af3219687ff77e5922f08bb41e43a9 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -19,6 +19,7 @@
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
>  #include <linux/limits.h>
> +#include <linux/idr.h>
>  #include <linux/init.h>
>  #include <linux/of.h>
>  #include <linux/of_pci.h>
> @@ -286,6 +287,7 @@ struct qcom_pcie {
>         const struct qcom_pcie_cfg *cfg;
>         struct dentry *debugfs;
>         struct list_head ports;
> +       struct idr perst;
>         bool suspended;
>         bool use_pm_opp;
>  };
> @@ -294,14 +296,15 @@ struct qcom_pcie {
>
>  static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
>  {
> -       struct qcom_pcie_port *port;
>         int val = assert ? 1 : 0;
> +       struct gpio_desc *perst;
> +       int bdf;
>
> -       if (list_empty(&pcie->ports))
> +       if (idr_is_empty(&pcie->perst))
>                 gpiod_set_value_cansleep(pcie->reset, val);
> -       else
> -               list_for_each_entry(port, &pcie->ports, list)
> -                       gpiod_set_value_cansleep(port->reset, val);
> +
> +       idr_for_each_entry(&pcie->perst, perst, bdf)
> +               gpiod_set_value_cansleep(perst, val);
>  }
>
>  static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> @@ -1702,20 +1705,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 device_node *np)
>  {
>         struct device *dev = pcie->pci->dev;
> -       struct qcom_pcie_port *port;
>         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;
> +
> +       ret = of_pci_get_bdf(np);
> +       if (ret < 0)
> +               return ret;
> +
> +       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");

Then maybe just use the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag for now and
don't bail-out - it will make it easier to spot it when converting to
the new solution?

Bart

> +
>                 return PTR_ERR(reset);
> +       }
> +
> +       ret = idr_alloc(&pcie->perst, reset, ret, 0, GFP_KERNEL);
> +       if (ret < 0)
> +               return ret;
> +
> +parse_child_node:
> +       for_each_available_child_of_node_scoped(np, child) {
> +               ret = qcom_pcie_parse_perst(pcie, child);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
>
> -       phy = devm_of_phy_get(dev, node, NULL);
> +static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *np)
> +{
> +       struct device *dev = pcie->pci->dev;
> +       struct qcom_pcie_port *port;
> +       struct phy *phy;
> +       int ret;
> +
> +       phy = devm_of_phy_get(dev, np, NULL);
>         if (IS_ERR(phy))
>                 return PTR_ERR(phy);
>
> @@ -1727,7 +1768,10 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
>         if (ret)
>                 return ret;
>
> -       port->reset = reset;
> +       ret = qcom_pcie_parse_perst(pcie, np);
> +       if (ret)
> +               return ret;
> +
>         port->phy = phy;
>         INIT_LIST_HEAD(&port->list);
>         list_add_tail(&port->list, &pcie->ports);
> @@ -1739,7 +1783,11 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  {
>         struct device *dev = pcie->pci->dev;
>         struct qcom_pcie_port *port, *tmp;
> -       int ret = -ENOENT;
> +       struct gpio_desc *perst;
> +       int ret = -ENODEV;
> +       int bdf;
> +
> +       idr_init(&pcie->perst);
>
>         for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>                 ret = qcom_pcie_parse_port(pcie, of_port);
> @@ -1750,8 +1798,13 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>         return ret;
>
>  err_port_del:
> -       list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +       list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> +               phy_exit(port->phy);
>                 list_del(&port->list);
> +       }
> +
> +       idr_for_each_entry(&pcie->perst, perst, bdf)
> +               idr_remove(&pcie->perst, bdf);
>
>         return ret;
>  }
> @@ -1782,12 +1835,13 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>         unsigned long max_freq = ULONG_MAX;
>         struct qcom_pcie_port *port, *tmp;
>         struct device *dev = &pdev->dev;
> +       struct gpio_desc *perst;
>         struct dev_pm_opp *opp;
>         struct qcom_pcie *pcie;
>         struct dw_pcie_rp *pp;
>         struct resource *res;
>         struct dw_pcie *pci;
> -       int ret, irq;
> +       int ret, irq, bdf;
>         char *name;
>
>         pcie_cfg = of_device_get_match_data(dev);
> @@ -1927,7 +1981,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;
> @@ -1989,6 +2043,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>         qcom_pcie_phy_exit(pcie);
>         list_for_each_entry_safe(port, tmp, &pcie->ports, list)
>                 list_del(&port->list);
> +       idr_for_each_entry(&pcie->perst, perst, bdf)
> +               idr_remove(&pcie->perst, bdf);
>  err_pm_runtime_put:
>         pm_runtime_put(dev);
>         pm_runtime_disable(dev);
>
> --
> 2.45.2
>
>

Re: [PATCH 3/6] PCI/pwrctrl: Add support for toggling PERST#

[Manivannan Sadhasivam]

On Wed, Aug 27, 2025 at 06:32:30PM GMT, Bartosz Golaszewski wrote:
> On Tue, Aug 19, 2025 at 9:15 AM Manivannan Sadhasivam via B4 Relay
> <devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> wrote:
> >
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> >
> > As per PCIe spec r6.0, sec 6.6.1, PERST# is an auxiliary signal provided by
> > the system to a component as a Fundamental Reset. This signal if available,
> > should conform to the rules defined by the electromechanical form factor
> > specifications like PCIe CEM spec r4.0, sec 2.2.
> >
> > Since pwrctrl driver is meant to control the power supplies, it should also
> > control the PERST# signal if available. But traditionally, the host bridge
> > (controller) drivers are the ones parsing and controlling the PERST#
> > signal. They also sometimes need to assert PERST# during their own hardware
> > initialization. So it is not possible to move the PERST# control away from
> > the controller drivers and it must be shared logically.
> >
> > Hence, add a new callback 'pci_host_bridge::toggle_perst', that allows the
> > pwrctrl core to toggle PERST# with the help of the controller drivers. But
> > care must be taken care by the controller drivers to not deassert the
> > PERST# signal if this callback is populated.
> >
> > This callback if available, will be called by the pwrctrl core during the
> > device power up and power down scenarios. Controller drivers should
> > identify the device using the 'struct device_node' passed during the
> > callback and toggle PERST# accordingly.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> >  drivers/pci/pwrctrl/core.c | 27 +++++++++++++++++++++++++++
> >  include/linux/pci.h        |  1 +
> >  2 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> > index 6bdbfed584d6d79ce28ba9e384a596b065ca69a4..8a26f432436d064acb7ebbbc9ce8fc339909fbe9 100644
> > --- a/drivers/pci/pwrctrl/core.c
> > +++ b/drivers/pci/pwrctrl/core.c
> > @@ -6,6 +6,7 @@
> >  #include <linux/device.h>
> >  #include <linux/export.h>
> >  #include <linux/kernel.h>
> > +#include <linux/of.h>
> >  #include <linux/pci.h>
> >  #include <linux/pci-pwrctrl.h>
> >  #include <linux/property.h>
> > @@ -61,6 +62,28 @@ void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(pci_pwrctrl_init);
> >
> > +static void pci_pwrctrl_perst_deassert(struct pci_pwrctrl *pwrctrl)
> > +{
> > +       struct pci_host_bridge *host_bridge = to_pci_host_bridge(pwrctrl->dev->parent);
> > +       struct device_node *np = dev_of_node(pwrctrl->dev);
> > +
> > +       if (!host_bridge->toggle_perst)
> > +               return;
> > +
> > +       host_bridge->toggle_perst(host_bridge, np, false);
> > +}
> > +
> > +static void pci_pwrctrl_perst_assert(struct pci_pwrctrl *pwrctrl)
> > +{
> > +       struct pci_host_bridge *host_bridge = to_pci_host_bridge(pwrctrl->dev->parent);
> > +       struct device_node *np = dev_of_node(pwrctrl->dev);
> > +
> > +       if (!host_bridge->toggle_perst)
> > +               return;
> > +
> > +       host_bridge->toggle_perst(host_bridge, np, true);
> > +}
> > +
> >  /**
> >   * pci_pwrctrl_device_set_ready() - Notify the pwrctrl subsystem that the PCI
> >   * device is powered-up and ready to be detected.
> > @@ -82,6 +105,8 @@ int pci_pwrctrl_device_set_ready(struct pci_pwrctrl *pwrctrl)
> >         if (!pwrctrl->dev)
> >                 return -ENODEV;
> >
> > +       pci_pwrctrl_perst_deassert(pwrctrl);
> > +
> >         pwrctrl->nb.notifier_call = pci_pwrctrl_notify;
> >         ret = bus_register_notifier(&pci_bus_type, &pwrctrl->nb);
> >         if (ret)
> > @@ -103,6 +128,8 @@ void pci_pwrctrl_device_unset_ready(struct pci_pwrctrl *pwrctrl)
> >  {
> >         cancel_work_sync(&pwrctrl->work);
> >
> > +       pci_pwrctrl_perst_assert(pwrctrl);
> > +
> >         /*
> >          * We don't have to delete the link here. Typically, this function
> >          * is only called when the power control device is being detached. If
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 59876de13860dbe50ee6c207cd57e54f51a11079..9eeee84d550bb9f15a90b5db9da03fccef8097ee 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -605,6 +605,7 @@ struct pci_host_bridge {
> >         void (*release_fn)(struct pci_host_bridge *);
> >         int (*enable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
> >         void (*disable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
> > +       void (*toggle_perst)(struct pci_host_bridge *bridge, struct device_node *np, bool assert);
> 
> Shouldn't this be wrapped in an #if IS_ENABLED(PCI_PWRCTL)?
> 

Ack.

- Mani

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

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

[Manivannan Sadhasivam]

On Wed, Aug 27, 2025 at 06:34:38PM GMT, Bartosz Golaszewski wrote:
> On Tue, Aug 19, 2025 at 9:15 AM Manivannan Sadhasivam via B4 Relay
> <devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> wrote:
> >
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> >
> > Devicetree schema allows the PERST# GPIO to be present in all PCIe bridge
> > nodes, not just in Root Port node. But the current logic parses PERST# only
> > from the Root Port node. 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 Root Port node. If the 'reset-gpios' property is found for a node, the
> > GPIO descriptor will be stored in IDR structure with node BDF as the ID.
> >
> > It should be noted that if more than one bridge node has the same GPIO for
> > PERST# (shared PERST#), the driver will error out. This is due to the
> > limitation in the GPIOLIB subsystem that allows only exclusive (non-shared)
> > access to GPIOs from consumers. But this is soon going to get fixed. Once
> > that happens, it will get incorporated in this driver.
> >
> > So for now, PERST# sharing is not supported.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 90 +++++++++++++++++++++++++++-------
> >  1 file changed, 73 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index bcd080315d70e64eafdefd852740fe07df3dbe75..5d73c46095af3219687ff77e5922f08bb41e43a9 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/iopoll.h>
> >  #include <linux/kernel.h>
> >  #include <linux/limits.h>
> > +#include <linux/idr.h>
> >  #include <linux/init.h>
> >  #include <linux/of.h>
> >  #include <linux/of_pci.h>
> > @@ -286,6 +287,7 @@ struct qcom_pcie {
> >         const struct qcom_pcie_cfg *cfg;
> >         struct dentry *debugfs;
> >         struct list_head ports;
> > +       struct idr perst;
> >         bool suspended;
> >         bool use_pm_opp;
> >  };
> > @@ -294,14 +296,15 @@ struct qcom_pcie {
> >
> >  static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> >  {
> > -       struct qcom_pcie_port *port;
> >         int val = assert ? 1 : 0;
> > +       struct gpio_desc *perst;
> > +       int bdf;
> >
> > -       if (list_empty(&pcie->ports))
> > +       if (idr_is_empty(&pcie->perst))
> >                 gpiod_set_value_cansleep(pcie->reset, val);
> > -       else
> > -               list_for_each_entry(port, &pcie->ports, list)
> > -                       gpiod_set_value_cansleep(port->reset, val);
> > +
> > +       idr_for_each_entry(&pcie->perst, perst, bdf)
> > +               gpiod_set_value_cansleep(perst, val);
> >  }
> >
> >  static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> > @@ -1702,20 +1705,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 device_node *np)
> >  {
> >         struct device *dev = pcie->pci->dev;
> > -       struct qcom_pcie_port *port;
> >         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;
> > +
> > +       ret = of_pci_get_bdf(np);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       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");
> 
> Then maybe just use the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag for now and
> don't bail-out - it will make it easier to spot it when converting to
> the new solution?
> 

But that gives the impression that shared PERST# is supported, but in reality it
is not.

- Mani

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

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

[Bartosz Golaszewski]

On Wed, Aug 27, 2025 at 7:28 PM Manivannan Sadhasivam <mani@kernel.org> wrote:
>
> On Wed, Aug 27, 2025 at 06:34:38PM GMT, Bartosz Golaszewski wrote:
> > On Tue, Aug 19, 2025 at 9:15 AM Manivannan Sadhasivam via B4 Relay
> > <devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> wrote:
> >
> > Then maybe just use the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag for now and
> > don't bail-out - it will make it easier to spot it when converting to
> > the new solution?
> >
>
> But that gives the impression that shared PERST# is supported, but in reality it
> is not.
>

Ok, nevermind then, I'll write this down as a candidate for testing
once I have the shared-gpio driver functional. What platform could I
potentially test this one BTW?

Bart

Re: [PATCH 4/6] PCI: of: Add an API to get the BDF for the device node

[Manivannan Sadhasivam]

On Tue, Aug 26, 2025 at 08:12:53AM GMT, Rob Herring wrote:
> On Tue, Aug 26, 2025 at 2:15 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
> >
> > On Mon, Aug 25, 2025 at 05:43:15PM GMT, Rob Herring wrote:
> > > On Fri, Aug 22, 2025 at 07:57:41PM +0530, Manivannan Sadhasivam wrote:
> > > > On Fri, Aug 22, 2025 at 08:51:47AM GMT, Rob Herring wrote:
> > > > > On Tue, Aug 19, 2025 at 12:44:53PM +0530, Manivannan Sadhasivam wrote:
> > > > > > Bus:Device:Function (BDF) numbers are used to uniquely identify a
> > > > > > device/function on a PCI bus. Hence, add an API to get the BDF from the
> > > > > > devicetree node of a device.
> > > > >
> > > > > For FDT, the bus should always be 0. It doesn't make sense for FDT. The
> > > > > bus number in DT reflects how firmware configured the PCI buses, but
> > > > > there's no firmware configuration of PCI for FDT.
> > > >
> > > > This API is targeted for DT platforms only, where it is used to uniquely
> > > > identify a devfn. What should I do to make it DT specific and not FDT?
> > >
> > > I don't understand. There are FDT and OF (actual OpenFirmware)
> > > platforms. I'm pretty sure you don't care about the latter.
> > >
> >
> > Sorry, I mixed the terminologies. Yes, I did refer the platforms making use of
> > the FDT binary and not OF platforms.
> >
> > In the DTS, we do use bus number to differentiate between devices, not just
> > devfn. But I get your point, bus no other than 0 are not fixed and allocated by
> > the OS during runtime or by the firmware.
> >
> > So how should we uniquely identify a PCIe node here, if not by BDF?
> 
> By path. Which is consistent since there is also no bus num in the unit-address.
> 

But there is no straightforward way to know the full path, isn't it? Anyway, for
simplicity, I'll just use the node pointer itself to identify the node.

- Mani

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