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

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

[Manivannan Sadhasivam]

Hi,

This series is an RFC to propose pwrctrl framework to control the PERST# GPIO
instead of letting the controller drivers to do so (which is a mistake btw).

Right now, the pwrctrl framework is controlling the power supplies to the
components (endpoints and such), but it is not controlling PERST#. This was
pointed out by Brian during a related conversation [1]. But we cannot just move
the PERST# control from controller drivers due to the following reasons:

1. Most of the controller drivers need to assert PERST# during the controller
initialization sequence. This is mostly as per their hardware reference manual
and should not be changed.

2. Controller drivers still need to toggle PERST# when pwrctrl is not used i.e.,
when the power supplies are not accurately described in PCI DT node. This can
happen on unsupported platforms and also for platforms with legacy DTs.

For this reason, I've kept the PERST# retrieval logic in the controller drivers
and just passed the gpio descriptors (for each slot) to the pwrctrl framework.
This will allow both the controller drivers and pwrctrl framework to share the
PERST# (which is ugly but can't be avoided). But care must be taken to ensure
that the controller drivers only assert PERST# and not deassert when pwrctrl is
used. I've added the change for the Qcom driver as a reference. The Qcom driver
is a slight mess because, it now has to support both new DT binding (PERST# and
PHY in Root Port node) and legacy (both in Host Bridge node). So I've allowed
the PERST# control only for the new binding (which is always going to use
pwrctrl framework to control the component supplies).

Testing
=======

This series is tested on Lenovo Thinkpad T14s laptop (with out-of-tree patch
enabling PCIe WLAN card) and on RB3 Gen2 with TC9563 switch (also with the not
yet merged series [2]). A big take away from this series is that, it is now
possible to get rid of the controversial {start/stop}_link() callback proposed
in the above mentioned switch pwrctrl driver [3].

- Mani

[1] https://lore.kernel.org/linux-pci/Z_6kZ7x7gnoH-P7x@google.com/
[2] https://lore.kernel.org/linux-pci/20250412-qps615_v4_1-v5-0-5b6a06132fec@oss.qualcomm.com/ 
[3] https://lore.kernel.org/linux-pci/20250412-qps615_v4_1-v5-4-5b6a06132fec@oss.qualcomm.com/

Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
---
Manivannan Sadhasivam (3):
      PCI/pwrctrl: Move pci_pwrctrl_init() before turning ON the supplies
      PCI/pwrctrl: Allow pwrctrl core to control PERST# GPIO if available
      PCI: qcom: Allow pwrctrl framework to control PERST#

 drivers/pci/controller/dwc/pcie-designware-host.c |  1 +
 drivers/pci/controller/dwc/pcie-designware.h      |  1 +
 drivers/pci/controller/dwc/pcie-qcom.c            | 26 ++++++++++++++-
 drivers/pci/pwrctrl/core.c                        | 39 +++++++++++++++++++++++
 drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c          |  4 +--
 drivers/pci/pwrctrl/slot.c                        |  4 +--
 include/linux/pci-pwrctrl.h                       |  2 ++
 include/linux/pci.h                               |  2 ++
 8 files changed, 74 insertions(+), 5 deletions(-)
---
base-commit: 00f0defc332be94b7f1fdc56ce7dcb6528cdf002
change-id: 20250707-pci-pwrctrl-perst-bdc6e36a335c

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

[PATCH RFC 1/3] PCI/pwrctrl: Move pci_pwrctrl_init() before turning ON the supplies

[Manivannan Sadhasivam]

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

Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
---
 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 18becc144913e04709783be43efe09c33ed2b502..97170c85d6f58f0812321716cb57e1fd8856572f 100644
--- a/drivers/pci/pwrctrl/slot.c
+++ b/drivers/pci/pwrctrl/slot.c
@@ -36,6 +36,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) {
@@ -55,8 +57,6 @@ static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_regulator_disable;
 
-	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 RFC 2/3] PCI/pwrctrl: Allow pwrctrl core to control PERST# GPIO if available

[Manivannan Sadhasivam]

PERST# is an (optional) auxiliary signal provided by the PCIe host to
components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
sec 6.6.1.

If PERST# is available, it's state will be toggled during the component
power-up and power-down scenarios as per the PCI Express Card
Electromechanical Spec v4.0, sec 2.2.

Historically, the PCIe controller drivers were directly controlling the
PERST# signal together with the power supplies. But with the advent of the
pwrctrl framework, the power supply control is now moved to the pwrctrl,
but controller drivers still ended up toggling the PERST# signal. This only
happens on Qcom platforms where pwrctrl framework is being used. But
nevertheseless, it is wrong to toggle PERST# (especially deassert) without
controlling the power supplies.

So allow the pwrctrl core to control the PERST# GPIO is available. The
controller drivers still need to parse them and populate the
'host_bridge->perst' GPIO descriptor array based on the available slots.
Unfortunately, we cannot just move the PERST# handling from controller
drivers as most of the controller drivers need to assert PERST# during the
controller initialization.

Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/pci/pwrctrl/core.c  | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/pci-pwrctrl.h |  2 ++
 include/linux/pci.h         |  2 ++
 3 files changed, 43 insertions(+)

diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
index 6bdbfed584d6d79ce28ba9e384a596b065ca69a4..abdb46399a96c8281916f971329d5460fcff3f6e 100644
--- a/drivers/pci/pwrctrl/core.c
+++ b/drivers/pci/pwrctrl/core.c
@@ -3,14 +3,19 @@
  * Copyright (C) 2024 Linaro Ltd.
  */
 
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/export.h>
+#include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
+#include <linux/of_pci.h>
 #include <linux/pci.h>
 #include <linux/pci-pwrctrl.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)
 {
@@ -56,11 +61,42 @@ static void rescan_work_func(struct work_struct *work)
  */
 void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
 {
+	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
+	int devfn;
+
 	pwrctrl->dev = dev;
 	INIT_WORK(&pwrctrl->work, rescan_work_func);
+
+	if (!host_bridge->perst)
+		return;
+
+	devfn = of_pci_get_devfn(dev_of_node(dev));
+	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
+		pwrctrl->perst = host_bridge->perst[PCI_SLOT(devfn)];
 }
 EXPORT_SYMBOL_GPL(pci_pwrctrl_init);
 
+static void pci_pwrctrl_perst_deassert(struct pci_pwrctrl *pwrctrl)
+{
+	/* Bail out early to avoid the delay if PERST# is not available */
+	if (!pwrctrl->perst)
+		return;
+
+	msleep(PCIE_T_PVPERL_MS);
+	gpiod_set_value_cansleep(pwrctrl->perst, 0);
+	/*
+	 * FIXME: The following delay is only required for downstream ports not
+	 * supporting link speed greater than 5.0 GT/s.
+	 */
+	msleep(PCIE_RESET_CONFIG_DEVICE_WAIT_MS);
+}
+
+static void pci_pwrctrl_perst_assert(struct pci_pwrctrl *pwrctrl)
+{
+	/* No need to validate desc here as gpiod APIs handle it for us */
+	gpiod_set_value_cansleep(pwrctrl->perst, 1);
+}
+
 /**
  * pci_pwrctrl_device_set_ready() - Notify the pwrctrl subsystem that the PCI
  * device is powered-up and ready to be detected.
@@ -82,6 +118,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 +141,7 @@ 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-pwrctrl.h b/include/linux/pci-pwrctrl.h
index 7d439b0675e91e920737287eaf1937f38e47f2ce..1ce6aec343fea1b77a146682f499ece791be70dc 100644
--- a/include/linux/pci-pwrctrl.h
+++ b/include/linux/pci-pwrctrl.h
@@ -31,6 +31,7 @@ struct device_link;
 /**
  * struct pci_pwrctrl - PCI device power control context.
  * @dev: Address of the power controlling device.
+ * @perst: PERST# GPIO connected to the PCI 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 +39,7 @@ struct device_link;
  */
 struct pci_pwrctrl {
 	struct device *dev;
+	struct gpio_desc *perst;
 
 	/* Private: don't use. */
 	struct notifier_block nb;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 05e68f35f39238f8b9ce08df97b384d1c1e89bbe..fcce106c7e2985ee1dd79bfcd241f133e5599fe1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -39,6 +39,7 @@
 #include <linux/io.h>
 #include <linux/resource_ext.h>
 #include <linux/msi_api.h>
+#include <linux/gpio/consumer.h>
 #include <uapi/linux/pci.h>
 
 #include <linux/pci_ids.h>
@@ -600,6 +601,7 @@ 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		*release_data;
+	struct gpio_desc **perst;
 	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
 	unsigned int	no_ext_tags:1;		/* No Extended Tags */
 	unsigned int	no_inc_mrrs:1;		/* No Increase MRRS */

-- 
2.45.2

[PATCH RFC 3/3] PCI: qcom: Allow pwrctrl framework to control PERST#

[Manivannan Sadhasivam]

Since the Qcom platforms rely on pwrctrl framework to control the power
supplies, allow it to control PERST# also. PERST# should be toggled during
the power-on and power-off scenarios.

But the controller driver still need to assert PERST# during the controller
initialization. So only skip the deassert if pwrctrl usage is detected. The
pwrctrl framework will deassert PERST# after turning on the supplies.

The usage of pwrctrl framework is detected based on the new DT binding
i.e., with the presence of PERST# and PHY properties in the Root Port node
instead of the host bridge node.

When the legacy binding is used, PERST# is only controlled by the
controller driver since it is not reliable to detect whether pwrctrl is
used or not. So the legacy platforms are untouched by this commit.

Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/pci/controller/dwc/pcie-designware-host.c |  1 +
 drivers/pci/controller/dwc/pcie-designware.h      |  1 +
 drivers/pci/controller/dwc/pcie-qcom.c            | 26 ++++++++++++++++++++++-
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index af6c91ec7312bab6c6e5ad35b051d0f452fe7b8d..e45f53bb135a75963318666a479eb6d9582f30eb 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -492,6 +492,7 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 		return -ENOMEM;
 
 	pp->bridge = bridge;
+	bridge->perst = pp->perst;
 
 	ret = dw_pcie_host_get_resources(pp);
 	if (ret)
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 4165c49a0a5059cab92dee3c47f8024af9d840bd..7b28f76ebf6a2de8781746eba43a8e3ad9a5cbb2 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -430,6 +430,7 @@ struct dw_pcie_rp {
 	struct resource		*msg_res;
 	bool			use_linkup_irq;
 	struct pci_eq_presets	presets;
+	struct gpio_desc	**perst;
 };
 
 struct dw_pcie_ep_ops {
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 620ac7cf09472b84c37e83ee3ce40e94a1d9d878..61e1d0d6469030c549328ab4d8c65d5377d525e3 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -313,6 +313,11 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
 
 static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
 {
+	struct dw_pcie_rp *pp = &pcie->pci->pp;
+
+	if (pp->perst)
+		return;
+
 	/* Ensure that PERST has been asserted for at least 100 ms */
 	msleep(PCIE_T_PVPERL_MS);
 	qcom_perst_assert(pcie, false);
@@ -1701,11 +1706,12 @@ static const struct pci_ecam_ops pci_qcom_ecam_ops = {
 
 static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node)
 {
+	struct dw_pcie_rp *pp = &pcie->pci->pp;
 	struct device *dev = pcie->pci->dev;
 	struct qcom_pcie_port *port;
 	struct gpio_desc *reset;
 	struct phy *phy;
-	int ret;
+	int ret, devfn;
 
 	reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
 				      "reset", GPIOD_OUT_HIGH, "PERST#");
@@ -1724,6 +1730,12 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
 	if (ret)
 		return ret;
 
+	devfn = of_pci_get_devfn(node);
+	if (devfn < 0)
+		return -ENOENT;
+
+	pp->perst[PCI_SLOT(devfn)] = reset;
+
 	port->reset = reset;
 	port->phy = phy;
 	INIT_LIST_HEAD(&port->list);
@@ -1734,10 +1746,20 @@ 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 dw_pcie_rp *pp = &pcie->pci->pp;
 	struct device *dev = pcie->pci->dev;
 	struct qcom_pcie_port *port, *tmp;
+	int child_cnt;
 	int ret = -ENOENT;
 
+	child_cnt = of_get_available_child_count(dev->of_node);
+	if (!child_cnt)
+		return ret;
+
+	pp->perst = kcalloc(child_cnt, sizeof(struct gpio_desc *), GFP_KERNEL);
+	if (!pp->perst)
+		return -ENOMEM;
+
 	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
 		ret = qcom_pcie_parse_port(pcie, of_port);
 		if (ret)
@@ -1747,6 +1769,7 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 	return ret;
 
 err_port_del:
+	kfree(pp->perst);
 	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
 		list_del(&port->list);
 
@@ -1984,6 +2007,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	dw_pcie_host_deinit(pp);
 err_phy_exit:
 	qcom_pcie_phy_exit(pcie);
+	kfree(pp->perst);
 	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
 		list_del(&port->list);
 err_pm_runtime_put:

-- 
2.45.2

Re: [PATCH RFC 0/3] PCI/pwrctrl: Allow pwrctrl framework to control PERST# GPIO if available

[Brian Norris]

Hi Manivannan,

Thanks for tackling this!

On Mon, Jul 07, 2025 at 11:48:37PM +0530, Manivannan Sadhasivam wrote:
> Hi,
> 
> This series is an RFC to propose pwrctrl framework to control the PERST# GPIO
> instead of letting the controller drivers to do so (which is a mistake btw).
> 
> Right now, the pwrctrl framework is controlling the power supplies to the
> components (endpoints and such), but it is not controlling PERST#. This was
> pointed out by Brian during a related conversation [1]. But we cannot just move
> the PERST# control from controller drivers due to the following reasons:
> 
> 1. Most of the controller drivers need to assert PERST# during the controller
> initialization sequence. This is mostly as per their hardware reference manual
> and should not be changed.
> 
> 2. Controller drivers still need to toggle PERST# when pwrctrl is not used i.e.,
> when the power supplies are not accurately described in PCI DT node. This can
> happen on unsupported platforms and also for platforms with legacy DTs.
> 
> For this reason, I've kept the PERST# retrieval logic in the controller drivers
> and just passed the gpio descriptors (for each slot) to the pwrctrl framework.

How sure are we that GPIOs (and *only* GPIOs) are sufficient for this
feature? I've seen a few drivers that pair a GPIO with some kind of
"internal" reset too, and it's not always clear that they can/should be
operated separately.

For example, drivers/pci/controller/dwc/pci-imx6.c /
imx_pcie_{,de}assert_core_reset(), and pcie-tegra194.c's
APPL_PINMUX_PEX_RST. The tegra case especially seems pretty clear that
its non-GPIO "pex_rst" is resetting an endpoint.

> This will allow both the controller drivers and pwrctrl framework to share the
> PERST# (which is ugly but can't be avoided). But care must be taken to ensure
> that the controller drivers only assert PERST# and not deassert when pwrctrl is
> used. I've added the change for the Qcom driver as a reference. The Qcom driver
> is a slight mess because, it now has to support both new DT binding (PERST# and
> PHY in Root Port node) and legacy (both in Host Bridge node). So I've allowed
> the PERST# control only for the new binding (which is always going to use
> pwrctrl framework to control the component supplies).
> 
> Testing
> =======
> 
> This series is tested on Lenovo Thinkpad T14s laptop (with out-of-tree patch
> enabling PCIe WLAN card) and on RB3 Gen2 with TC9563 switch (also with the not
> yet merged series [2]). A big take away from this series is that, it is now
> possible to get rid of the controversial {start/stop}_link() callback proposed
> in the above mentioned switch pwrctrl driver [3].

This is a tiny bit tangential to the PERST# discussion, but I believe
there are other controller driver features that don't fit into the
sequence model of:

1. start LTSSM (controller driver)
2. pwrctrl eventually turns on power + delay per spec
3. pwrctrl deasserts PERST#
4. pwrctrl delays a fixed amount of time, per the CEM spec
5. pwrctrl rescans bus

For example, tegra_pcie_dw_start_link() notes some cases where it needs
to take action and retry when the link doesn't come up. Similarly, I've
seen drivers with retry loops for cases where the link comes up, but not
at the expected link rate. None of this is possible if the controller
driver only gets to take care of #1, and has no involvement in between
#3 and #5.

(Yes, I acknowledge that DWC's .start_link() is being abused in
tegra_pcie_dw_start_link(). But it's still reality, with a
seemingly-good use case.)

Brian

> 
> - Mani
> 
> [1] https://lore.kernel.org/linux-pci/Z_6kZ7x7gnoH-P7x@google.com/
> [2] https://lore.kernel.org/linux-pci/20250412-qps615_v4_1-v5-0-5b6a06132fec@oss.qualcomm.com/ 
> [3] https://lore.kernel.org/linux-pci/20250412-qps615_v4_1-v5-4-5b6a06132fec@oss.qualcomm.com/
> 
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
> Manivannan Sadhasivam (3):
>       PCI/pwrctrl: Move pci_pwrctrl_init() before turning ON the supplies
>       PCI/pwrctrl: Allow pwrctrl core to control PERST# GPIO if available
>       PCI: qcom: Allow pwrctrl framework to control PERST#
> 
>  drivers/pci/controller/dwc/pcie-designware-host.c |  1 +
>  drivers/pci/controller/dwc/pcie-designware.h      |  1 +
>  drivers/pci/controller/dwc/pcie-qcom.c            | 26 ++++++++++++++-
>  drivers/pci/pwrctrl/core.c                        | 39 +++++++++++++++++++++++
>  drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c          |  4 +--
>  drivers/pci/pwrctrl/slot.c                        |  4 +--
>  include/linux/pci-pwrctrl.h                       |  2 ++
>  include/linux/pci.h                               |  2 ++
>  8 files changed, 74 insertions(+), 5 deletions(-)
> ---
> base-commit: 00f0defc332be94b7f1fdc56ce7dcb6528cdf002
> change-id: 20250707-pci-pwrctrl-perst-bdc6e36a335c
> 
> Best regards,
> -- 
> Manivannan Sadhasivam <mani@kernel.org>
> 

Re: [PATCH RFC 2/3] PCI/pwrctrl: Allow pwrctrl core to control PERST# GPIO if available

[Brian Norris]

Hi Manivannan,

On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> PERST# is an (optional) auxiliary signal provided by the PCIe host to
> components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> sec 6.6.1.
> 
> If PERST# is available, it's state will be toggled during the component
> power-up and power-down scenarios as per the PCI Express Card
> Electromechanical Spec v4.0, sec 2.2.
> 
> Historically, the PCIe controller drivers were directly controlling the
> PERST# signal together with the power supplies. But with the advent of the
> pwrctrl framework, the power supply control is now moved to the pwrctrl,
> but controller drivers still ended up toggling the PERST# signal.

[reflowed:]
> This only happens on Qcom platforms where pwrctrl framework is being
> used.

What do you mean by this sentence? That this problem only occurs on Qcom
platforms? (I believe that's false.) Or that the problem doesn't occur
if the platform is not using pwrctrl? (i.e., it maintained power in some
other way, before the controller driver gets involved. I believe this
variation is correct.)

> But
> nevertheseless, it is wrong to toggle PERST# (especially deassert) without
> controlling the power supplies.
> 
> So allow the pwrctrl core to control the PERST# GPIO is available. The

s/is/if/

?

> controller drivers still need to parse them and populate the
> 'host_bridge->perst' GPIO descriptor array based on the available slots.
> Unfortunately, we cannot just move the PERST# handling from controller
> drivers as most of the controller drivers need to assert PERST# during the
> controller initialization.
> 
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>  drivers/pci/pwrctrl/core.c  | 39 +++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-pwrctrl.h |  2 ++
>  include/linux/pci.h         |  2 ++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index 6bdbfed584d6d79ce28ba9e384a596b065ca69a4..abdb46399a96c8281916f971329d5460fcff3f6e 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c

>  static int pci_pwrctrl_notify(struct notifier_block *nb, unsigned long action,
>  			      void *data)
>  {
> @@ -56,11 +61,42 @@ static void rescan_work_func(struct work_struct *work)
>   */
>  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
>  {
> +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> +	int devfn;
> +
>  	pwrctrl->dev = dev;
>  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> +
> +	if (!host_bridge->perst)
> +		return;
> +
> +	devfn = of_pci_get_devfn(dev_of_node(dev));
> +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
> +		pwrctrl->perst = host_bridge->perst[PCI_SLOT(devfn)];

It seems a little suspect that we trust the device tree slot
specification to not overflow the perst[] array. I think we can
reasonably mitigate that in the controller driver (so, patch 3 in this
series), but I want to call that out, in case there's something we can
do here too.

>  }
>  EXPORT_SYMBOL_GPL(pci_pwrctrl_init);
>  
> +static void pci_pwrctrl_perst_deassert(struct pci_pwrctrl *pwrctrl)
> +{
> +	/* Bail out early to avoid the delay if PERST# is not available */
> +	if (!pwrctrl->perst)
> +		return;
> +
> +	msleep(PCIE_T_PVPERL_MS);
> +	gpiod_set_value_cansleep(pwrctrl->perst, 0);

What if PERST# was already deasserted? On one hand, we're wasting time
here if so. On the other, you're not accomplishing your spec-compliance
goal if it was.

> +	/*
> +	 * FIXME: The following delay is only required for downstream ports not
> +	 * supporting link speed greater than 5.0 GT/s.
> +	 */
> +	msleep(PCIE_RESET_CONFIG_DEVICE_WAIT_MS);

Should this be PCIE_RESET_CONFIG_DEVICE_WAIT_MS or PCIE_T_RRS_READY_MS?
Or are those describing the same thing? It seems like they were added
within a month or two of each other, so maybe they're just duplicates.

BTW, I see you have a FIXME here, but anyway, I wonder if both of the
msleep() delays in this function will need some kind of override (e.g.,
via Device Tree), since there's room for implementation or form factor
differences, if I'm reading the spec correctly. Maybe that's a question
for another time, with actual proof / use case.

> +}
> +

Brian

Re: [PATCH RFC 3/3] PCI: qcom: Allow pwrctrl framework to control PERST#

[Brian Norris]

Hi,

On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote:
> Since the Qcom platforms rely on pwrctrl framework to control the power
> supplies, allow it to control PERST# also. PERST# should be toggled during
> the power-on and power-off scenarios.
> 
> But the controller driver still need to assert PERST# during the controller
> initialization. So only skip the deassert if pwrctrl usage is detected. The
> pwrctrl framework will deassert PERST# after turning on the supplies.
> 
> The usage of pwrctrl framework is detected based on the new DT binding
> i.e., with the presence of PERST# and PHY properties in the Root Port node
> instead of the host bridge node.
> 
> When the legacy binding is used, PERST# is only controlled by the
> controller driver since it is not reliable to detect whether pwrctrl is
> used or not. So the legacy platforms are untouched by this commit.
> 
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c |  1 +
>  drivers/pci/controller/dwc/pcie-designware.h      |  1 +
>  drivers/pci/controller/dwc/pcie-qcom.c            | 26 ++++++++++++++++++++++-
>  3 files changed, 27 insertions(+), 1 deletion(-)

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 620ac7cf09472b84c37e83ee3ce40e94a1d9d878..61e1d0d6469030c549328ab4d8c65d5377d525e3 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c

> @@ -1724,6 +1730,12 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
>  	if (ret)
>  		return ret;
>  
> +	devfn = of_pci_get_devfn(node);
> +	if (devfn < 0)
> +		return -ENOENT;
> +
> +	pp->perst[PCI_SLOT(devfn)] = reset;

It seems like you assume a well-written device tree, such that this
PCI_SLOT(devfn) doesn't overflow the perst[] array. It seems like we
should guard against that somehow.

Also see my comment below, where I believe even a well-written device
tree could trip this up.

> +
>  	port->reset = reset;
>  	port->phy = phy;
>  	INIT_LIST_HEAD(&port->list);
> @@ -1734,10 +1746,20 @@ 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 dw_pcie_rp *pp = &pcie->pci->pp;
>  	struct device *dev = pcie->pci->dev;
>  	struct qcom_pcie_port *port, *tmp;
> +	int child_cnt;
>  	int ret = -ENOENT;
>  
> +	child_cnt = of_get_available_child_count(dev->of_node);

I think you're assuming "available children" correlate precisely with a
0-indexed array of ports. But what if, e.g., port 0 is disabled in the
device tree, and only port 1 is available? Then you'll overflow.

> +	if (!child_cnt)
> +		return ret;
> +
> +	pp->perst = kcalloc(child_cnt, sizeof(struct gpio_desc *), GFP_KERNEL);

IIUC, you kfree() this on error, but otherwise, you never free it. I
also see that this driver can't actually be unbound (commit f9a666008338
("PCI: qcom: Make explicitly non-modular")), so technically there's no
way to "leak" this other than by probe errors...
...but it still seems like devm_*() would fit better.

(NB: I'm not sure I agree with commit f9a666008338 that "[driver unbind]
doesn't have a sensible use case anyway". That just sounds like
laziness. And it *can* have a useful purpose for testing.)

Brian

> +	if (!pp->perst)
> +		return -ENOMEM;
> +
>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>  		ret = qcom_pcie_parse_port(pcie, of_port);
>  		if (ret)
 

Re: [PATCH RFC 0/3] PCI/pwrctrl: Allow pwrctrl framework to control PERST# GPIO if available

[Manivannan Sadhasivam]

On Tue, Jul 08, 2025 at 06:39:37PM GMT, Brian Norris wrote:
> Hi Manivannan,
> 
> Thanks for tackling this!
> 
> On Mon, Jul 07, 2025 at 11:48:37PM +0530, Manivannan Sadhasivam wrote:
> > Hi,
> > 
> > This series is an RFC to propose pwrctrl framework to control the PERST# GPIO
> > instead of letting the controller drivers to do so (which is a mistake btw).
> > 
> > Right now, the pwrctrl framework is controlling the power supplies to the
> > components (endpoints and such), but it is not controlling PERST#. This was
> > pointed out by Brian during a related conversation [1]. But we cannot just move
> > the PERST# control from controller drivers due to the following reasons:
> > 
> > 1. Most of the controller drivers need to assert PERST# during the controller
> > initialization sequence. This is mostly as per their hardware reference manual
> > and should not be changed.
> > 
> > 2. Controller drivers still need to toggle PERST# when pwrctrl is not used i.e.,
> > when the power supplies are not accurately described in PCI DT node. This can
> > happen on unsupported platforms and also for platforms with legacy DTs.
> > 
> > For this reason, I've kept the PERST# retrieval logic in the controller drivers
> > and just passed the gpio descriptors (for each slot) to the pwrctrl framework.
> 
> How sure are we that GPIOs (and *only* GPIOs) are sufficient for this
> feature? I've seen a few drivers that pair a GPIO with some kind of
> "internal" reset too, and it's not always clear that they can/should be
> operated separately.
> 
> For example, drivers/pci/controller/dwc/pci-imx6.c /
> imx_pcie_{,de}assert_core_reset(), and pcie-tegra194.c's
> APPL_PINMUX_PEX_RST. The tegra case especially seems pretty clear that
> its non-GPIO "pex_rst" is resetting an endpoint.
> 

Right. But GPIO is the most commonly used approach for implementing PERST# and
it is the one supported on the platform I'm testing with. So I just went with
that. For sure there are other methods exist and the PCIe spec itself doesn't
define how PERST# should be implemented in a form factor. It merely defines
PERST# as an 'auxiliary signal'. So yes, other form of PERST# can exist. But for
the sake of keeping this proposal simple, I'm considering only GPIO based
PERST# atm.

Also, Tegra platforms are not converted to use pwrctrl framework and I don't
know if the platform maintainers are interested in it or not. But if they start
using it, we can tackle this situation by introducing a callback that
asserts/deasserts PERST# (yes, callbacks are evil, but I don't know any other
sensible way to support vendor specific PERST# implementations).

Oh and do take a look at pcie-brcmstb driver, which I promised to move to
pwrctrl framework for another reason. It uses multiple callbacks per SoC
revisions for toggling PERST#. So for these usecases, having a callback in
'struct pci_host_bridge' would be a good fit and I may introduce it after this
series gets in.

> > This will allow both the controller drivers and pwrctrl framework to share the
> > PERST# (which is ugly but can't be avoided). But care must be taken to ensure
> > that the controller drivers only assert PERST# and not deassert when pwrctrl is
> > used. I've added the change for the Qcom driver as a reference. The Qcom driver
> > is a slight mess because, it now has to support both new DT binding (PERST# and
> > PHY in Root Port node) and legacy (both in Host Bridge node). So I've allowed
> > the PERST# control only for the new binding (which is always going to use
> > pwrctrl framework to control the component supplies).
> > 
> > Testing
> > =======
> > 
> > This series is tested on Lenovo Thinkpad T14s laptop (with out-of-tree patch
> > enabling PCIe WLAN card) and on RB3 Gen2 with TC9563 switch (also with the not
> > yet merged series [2]). A big take away from this series is that, it is now
> > possible to get rid of the controversial {start/stop}_link() callback proposed
> > in the above mentioned switch pwrctrl driver [3].
> 
> This is a tiny bit tangential to the PERST# discussion, but I believe
> there are other controller driver features that don't fit into the
> sequence model of:
> 
> 1. start LTSSM (controller driver)
> 2. pwrctrl eventually turns on power + delay per spec
> 3. pwrctrl deasserts PERST#
> 4. pwrctrl delays a fixed amount of time, per the CEM spec
> 5. pwrctrl rescans bus
> 
> For example, tegra_pcie_dw_start_link() notes some cases where it needs
> to take action and retry when the link doesn't come up. Similarly, I've
> seen drivers with retry loops for cases where the link comes up, but not
> at the expected link rate. None of this is possible if the controller
> driver only gets to take care of #1, and has no involvement in between
> #3 and #5.
> 

Having this back and forth communication would make the pwrctrl driver a lot
messier. But I believe, we could teach pwrctrl driver to detect link up (similar
to dw_pcie_wait_for_link()) and if link didn't come up, it could do retry and
other steps with help from controller drivers. But these things should be
implemented only when platforms like Tegra start to show some love towards
pwrctrl.

- Mani

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

Re: [PATCH RFC 2/3] PCI/pwrctrl: Allow pwrctrl core to control PERST# GPIO if available

[Manivannan Sadhasivam]

On Tue, Jul 08, 2025 at 08:15:39PM GMT, Brian Norris wrote:
> Hi Manivannan,
> 
> On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> > PERST# is an (optional) auxiliary signal provided by the PCIe host to
> > components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> > sec 6.6.1.
> > 
> > If PERST# is available, it's state will be toggled during the component
> > power-up and power-down scenarios as per the PCI Express Card
> > Electromechanical Spec v4.0, sec 2.2.
> > 
> > Historically, the PCIe controller drivers were directly controlling the
> > PERST# signal together with the power supplies. But with the advent of the
> > pwrctrl framework, the power supply control is now moved to the pwrctrl,
> > but controller drivers still ended up toggling the PERST# signal.
> 
> [reflowed:]
> > This only happens on Qcom platforms where pwrctrl framework is being
> > used.
> 
> What do you mean by this sentence? That this problem only occurs on Qcom
> platforms? (I believe that's false.) Or that the problem doesn't occur
> if the platform is not using pwrctrl? (i.e., it maintained power in some
> other way, before the controller driver gets involved. I believe this
> variation is correct.)
> 

The latter one. I will rephrase this sentence in next version.

> > But
> > nevertheseless, it is wrong to toggle PERST# (especially deassert) without
> > controlling the power supplies.
> > 
> > So allow the pwrctrl core to control the PERST# GPIO is available. The
> 
> s/is/if/
> 
> ?
> 
> > controller drivers still need to parse them and populate the
> > 'host_bridge->perst' GPIO descriptor array based on the available slots.
> > Unfortunately, we cannot just move the PERST# handling from controller
> > drivers as most of the controller drivers need to assert PERST# during the
> > controller initialization.
> > 
> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> > ---
> >  drivers/pci/pwrctrl/core.c  | 39 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci-pwrctrl.h |  2 ++
> >  include/linux/pci.h         |  2 ++
> >  3 files changed, 43 insertions(+)
> > 
> > diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> > index 6bdbfed584d6d79ce28ba9e384a596b065ca69a4..abdb46399a96c8281916f971329d5460fcff3f6e 100644
> > --- a/drivers/pci/pwrctrl/core.c
> > +++ b/drivers/pci/pwrctrl/core.c
> 
> >  static int pci_pwrctrl_notify(struct notifier_block *nb, unsigned long action,
> >  			      void *data)
> >  {
> > @@ -56,11 +61,42 @@ static void rescan_work_func(struct work_struct *work)
> >   */
> >  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
> >  {
> > +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> > +	int devfn;
> > +
> >  	pwrctrl->dev = dev;
> >  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> > +
> > +	if (!host_bridge->perst)
> > +		return;
> > +
> > +	devfn = of_pci_get_devfn(dev_of_node(dev));
> > +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
> > +		pwrctrl->perst = host_bridge->perst[PCI_SLOT(devfn)];
> 
> It seems a little suspect that we trust the device tree slot
> specification to not overflow the perst[] array. I think we can
> reasonably mitigate that in the controller driver (so, patch 3 in this
> series), but I want to call that out, in case there's something we can
> do here too.
> 
> >  }
> >  EXPORT_SYMBOL_GPL(pci_pwrctrl_init);
> >  
> > +static void pci_pwrctrl_perst_deassert(struct pci_pwrctrl *pwrctrl)
> > +{
> > +	/* Bail out early to avoid the delay if PERST# is not available */
> > +	if (!pwrctrl->perst)
> > +		return;
> > +
> > +	msleep(PCIE_T_PVPERL_MS);
> > +	gpiod_set_value_cansleep(pwrctrl->perst, 0);
> 
> What if PERST# was already deasserted? On one hand, we're wasting time
> here if so. On the other, you're not accomplishing your spec-compliance
> goal if it was.
> 

If controller drivers populate 'pci_host_bridge::perst', then they should not
deassert PERST# as they don't control the supplies. I've mentioned it in the
cover letter, but I will mention it in commit message also.

> > +	/*
> > +	 * FIXME: The following delay is only required for downstream ports not
> > +	 * supporting link speed greater than 5.0 GT/s.
> > +	 */
> > +	msleep(PCIE_RESET_CONFIG_DEVICE_WAIT_MS);
> 
> Should this be PCIE_RESET_CONFIG_DEVICE_WAIT_MS or PCIE_T_RRS_READY_MS?
> Or are those describing the same thing? It seems like they were added
> within a month or two of each other, so maybe they're just duplicates.
> 

You are right. This is already taken care in:
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/linkup-fix&id=bbc6a829ad3f054181d24a56944f944002e68898

I will rebase the next version on top of pci/next to make use of it.

> BTW, I see you have a FIXME here, but anyway, I wonder if both of the
> msleep() delays in this function will need some kind of override (e.g.,
> via Device Tree), since there's room for implementation or form factor
> differences, if I'm reading the spec correctly. Maybe that's a question
> for another time, with actual proof / use case.
> 

First delay cannot be skipped as both PCIe CEM and M.2 FF, mandates this delay.
Though, M.2 mandates only a min delay of 50ms, and leaves the value to be
defined by the vendor. So a common 100ms would be on the safe side.

For the second delay, it comes from the PCIe spec itself. Refer r6.0, sec
6.6.1. So we cannot skip that, though we should only need it if the link is
operating at <= 5.0 GT/s. Right now, I haven't implemented the logic to detect
the link speed, hence the TODO.

- Mani

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

Re: [PATCH RFC 3/3] PCI: qcom: Allow pwrctrl framework to control PERST#

[Manivannan Sadhasivam]

On Tue, Jul 08, 2025 at 08:18:06PM GMT, Brian Norris wrote:
> Hi,
> 
> On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote:
> > Since the Qcom platforms rely on pwrctrl framework to control the power
> > supplies, allow it to control PERST# also. PERST# should be toggled during
> > the power-on and power-off scenarios.
> > 
> > But the controller driver still need to assert PERST# during the controller
> > initialization. So only skip the deassert if pwrctrl usage is detected. The
> > pwrctrl framework will deassert PERST# after turning on the supplies.
> > 
> > The usage of pwrctrl framework is detected based on the new DT binding
> > i.e., with the presence of PERST# and PHY properties in the Root Port node
> > instead of the host bridge node.
> > 
> > When the legacy binding is used, PERST# is only controlled by the
> > controller driver since it is not reliable to detect whether pwrctrl is
> > used or not. So the legacy platforms are untouched by this commit.
> > 
> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c |  1 +
> >  drivers/pci/controller/dwc/pcie-designware.h      |  1 +
> >  drivers/pci/controller/dwc/pcie-qcom.c            | 26 ++++++++++++++++++++++-
> >  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 620ac7cf09472b84c37e83ee3ce40e94a1d9d878..61e1d0d6469030c549328ab4d8c65d5377d525e3 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> 
> > @@ -1724,6 +1730,12 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
> >  	if (ret)
> >  		return ret;
> >  
> > +	devfn = of_pci_get_devfn(node);
> > +	if (devfn < 0)
> > +		return -ENOENT;
> > +
> > +	pp->perst[PCI_SLOT(devfn)] = reset;
> 
> It seems like you assume a well-written device tree, such that this
> PCI_SLOT(devfn) doesn't overflow the perst[] array. It seems like we
> should guard against that somehow.
> 

Sure. I will add a check.

> Also see my comment below, where I believe even a well-written device
> tree could trip this up.
> 
> > +
> >  	port->reset = reset;
> >  	port->phy = phy;
> >  	INIT_LIST_HEAD(&port->list);
> > @@ -1734,10 +1746,20 @@ 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 dw_pcie_rp *pp = &pcie->pci->pp;
> >  	struct device *dev = pcie->pci->dev;
> >  	struct qcom_pcie_port *port, *tmp;
> > +	int child_cnt;
> >  	int ret = -ENOENT;
> >  
> > +	child_cnt = of_get_available_child_count(dev->of_node);
> 
> I think you're assuming "available children" correlate precisely with a
> 0-indexed array of ports. But what if, e.g., port 0 is disabled in the
> device tree, and only port 1 is available? Then you'll overflow.
> 

Right. I will take care it in next version.

> > +	if (!child_cnt)
> > +		return ret;
> > +
> > +	pp->perst = kcalloc(child_cnt, sizeof(struct gpio_desc *), GFP_KERNEL);
> 
> IIUC, you kfree() this on error, but otherwise, you never free it. I
> also see that this driver can't actually be unbound (commit f9a666008338
> ("PCI: qcom: Make explicitly non-modular")), so technically there's no
> way to "leak" this other than by probe errors...
> ...but it still seems like devm_*() would fit better.
> 

Even if we use devm_(), we need to free the array when qcom_pcie_parse_port()
fails. And as you spotted, we don't remove the driver currently. So I decided to
use kfree(). Someone would argue that if we manually free the memory, then it
defeats the purpose of devm_() variants.

> (NB: I'm not sure I agree with commit f9a666008338 that "[driver unbind]
> doesn't have a sensible use case anyway". That just sounds like
> laziness. And it *can* have a useful purpose for testing.)
> 

There was a whole debate on it. It is mostly due to the fact that this driver
implements the MSI controller and the IRQCHIP drivers are not expected to go
away during runtime. But atleast, I would like to build this driver as a module
and not remove it. The patch is pending for some time.

- Mani

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

Re: [PATCH RFC 1/3] PCI/pwrctrl: Move pci_pwrctrl_init() before turning ON the supplies

[Bartosz Golaszewski]

On Mon, Jul 7, 2025 at 8:18 PM Manivannan Sadhasivam <mani@kernel.org> wrote:
>
> To allow pwrctrl core to parse the generic resources such as PERST# GPIO
> before turning on the supplies.
>
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> ---

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH RFC 3/3] PCI: qcom: Allow pwrctrl framework to control PERST#

[Brian Norris]

Hi,

On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote:
> Since the Qcom platforms rely on pwrctrl framework to control the power
> supplies, allow it to control PERST# also. PERST# should be toggled during
> the power-on and power-off scenarios.
> 
> But the controller driver still need to assert PERST# during the controller
> initialization. So only skip the deassert if pwrctrl usage is detected. The
> pwrctrl framework will deassert PERST# after turning on the supplies.
> 
> The usage of pwrctrl framework is detected based on the new DT binding
> i.e., with the presence of PERST# and PHY properties in the Root Port node
> instead of the host bridge node.

I just noticed what this paragraph means. IIUC, this implies that in
your new binding, one *must* describe one or more *-supply in the port
or endpoint device(s). Otherwise, no pwrctrl devices will be created,
and no one will deassert PERST# for you. My understanding is that
*-supply is optional, and so this is a poor requirement.

And even if all QCOM device trees manage to have external regulators
described in their device trees, there are certainly other systems where
the driver might (optionally) use pwrctrl for some devices, but others
will establish power on their own (e.g., PCIe tied to some other system
power rail).

I think you either need the PCI core to tell you whether pwrctrl is in
use, or else you need to disconnect this PERST# support from pwrctrl.

> When the legacy binding is used, PERST# is only controlled by the
> controller driver since it is not reliable to detect whether pwrctrl is
> used or not. So the legacy platforms are untouched by this commit.
> 
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c |  1 +
>  drivers/pci/controller/dwc/pcie-designware.h      |  1 +
>  drivers/pci/controller/dwc/pcie-qcom.c            | 26 ++++++++++++++++++++++-
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index af6c91ec7312bab6c6e5ad35b051d0f452fe7b8d..e45f53bb135a75963318666a479eb6d9582f30eb 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -492,6 +492,7 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  		return -ENOMEM;
>  
>  	pp->bridge = bridge;
> +	bridge->perst = pp->perst;
>  
>  	ret = dw_pcie_host_get_resources(pp);
>  	if (ret)
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 4165c49a0a5059cab92dee3c47f8024af9d840bd..7b28f76ebf6a2de8781746eba43a8e3ad9a5cbb2 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -430,6 +430,7 @@ struct dw_pcie_rp {
>  	struct resource		*msg_res;
>  	bool			use_linkup_irq;
>  	struct pci_eq_presets	presets;
> +	struct gpio_desc	**perst;
>  };
>  
>  struct dw_pcie_ep_ops {
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 620ac7cf09472b84c37e83ee3ce40e94a1d9d878..61e1d0d6469030c549328ab4d8c65d5377d525e3 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -313,6 +313,11 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
>  
>  static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
>  {
> +	struct dw_pcie_rp *pp = &pcie->pci->pp;
> +
> +	if (pp->perst)

You're doing a non-NULL check here, but you forgot to reinitialize it to
NULL in some cases below (qcom_pcie_parse_ports()).

This is also apparently where you assume |perst| implies pwrctrl. Per
above, I don't think that's a good assumption.

> +		return;
> +
>  	/* Ensure that PERST has been asserted for at least 100 ms */
>  	msleep(PCIE_T_PVPERL_MS);
>  	qcom_perst_assert(pcie, false);
...
>  static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  {
> +	struct dw_pcie_rp *pp = &pcie->pci->pp;
>  	struct device *dev = pcie->pci->dev;
>  	struct qcom_pcie_port *port, *tmp;
> +	int child_cnt;
>  	int ret = -ENOENT;
>  
> +	child_cnt = of_get_available_child_count(dev->of_node);
> +	if (!child_cnt)
> +		return ret;
> +
> +	pp->perst = kcalloc(child_cnt, sizeof(struct gpio_desc *), GFP_KERNEL);
> +	if (!pp->perst)
> +		return -ENOMEM;
> +
>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>  		ret = qcom_pcie_parse_port(pcie, of_port);
>  		if (ret)
> @@ -1747,6 +1769,7 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  	return ret;
>  
>  err_port_del:
> +	kfree(pp->perst);

You need this too:

	pp->perst = NULL;

Otherwise, you leave a non-NULL (invalid) pointer for cases that
qcom_pcie_parse_legacy_binding() might handle.

Brian

>  	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
>  		list_del(&port->list);
>  

Re: [PATCH RFC 2/3] PCI/pwrctrl: Allow pwrctrl core to control PERST# GPIO if available

[Brian Norris]

On Wed, Jul 09, 2025 at 01:35:08PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jul 08, 2025 at 08:15:39PM GMT, Brian Norris wrote:
> > On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> > > --- a/drivers/pci/pwrctrl/core.c
> > > +++ b/drivers/pci/pwrctrl/core.c
> > > +static void pci_pwrctrl_perst_deassert(struct pci_pwrctrl *pwrctrl)
> > > +{
> > > +	/* Bail out early to avoid the delay if PERST# is not available */
> > > +	if (!pwrctrl->perst)
> > > +		return;
> > > +
> > > +	msleep(PCIE_T_PVPERL_MS);
> > > +	gpiod_set_value_cansleep(pwrctrl->perst, 0);
> > 
> > What if PERST# was already deasserted? On one hand, we're wasting time
> > here if so. On the other, you're not accomplishing your spec-compliance
> > goal if it was.
> > 
> 
> If controller drivers populate 'pci_host_bridge::perst', then they should not
> deassert PERST# as they don't control the supplies. I've mentioned it in the
> cover letter, but I will mention it in commit message also.

Sorry, I think I partially read that, but didn't really grasp how it fit
in here.

I commented on patch 3 where you try to implement this. IIUC, you're
making excessive assumptions about the use of pwrctrl.

Brian

Re: [PATCH RFC 0/3] PCI/pwrctrl: Allow pwrctrl framework to control PERST# GPIO if available

[Brian Norris]

Hi,

On Wed, Jul 09, 2025 at 12:18:29PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jul 08, 2025 at 06:39:37PM GMT, Brian Norris wrote:
> > On Mon, Jul 07, 2025 at 11:48:37PM +0530, Manivannan Sadhasivam wrote:
> > > Hi,
> > > 
> > > This series is an RFC to propose pwrctrl framework to control the PERST# GPIO
> > > instead of letting the controller drivers to do so (which is a mistake btw).
> > > 
> > > Right now, the pwrctrl framework is controlling the power supplies to the
> > > components (endpoints and such), but it is not controlling PERST#. This was
> > > pointed out by Brian during a related conversation [1]. But we cannot just move
> > > the PERST# control from controller drivers due to the following reasons:
> > > 
> > > 1. Most of the controller drivers need to assert PERST# during the controller
> > > initialization sequence. This is mostly as per their hardware reference manual
> > > and should not be changed.
> > > 
> > > 2. Controller drivers still need to toggle PERST# when pwrctrl is not used i.e.,
> > > when the power supplies are not accurately described in PCI DT node. This can
> > > happen on unsupported platforms and also for platforms with legacy DTs.
> > > 
> > > For this reason, I've kept the PERST# retrieval logic in the controller drivers
> > > and just passed the gpio descriptors (for each slot) to the pwrctrl framework.
> > 
> > How sure are we that GPIOs (and *only* GPIOs) are sufficient for this
> > feature? I've seen a few drivers that pair a GPIO with some kind of
> > "internal" reset too, and it's not always clear that they can/should be
> > operated separately.
> > 
> > For example, drivers/pci/controller/dwc/pci-imx6.c /
> > imx_pcie_{,de}assert_core_reset(), and pcie-tegra194.c's
> > APPL_PINMUX_PEX_RST. The tegra case especially seems pretty clear that
> > its non-GPIO "pex_rst" is resetting an endpoint.
> > 
> 
> Right. But GPIO is the most commonly used approach for implementing PERST# and
> it is the one supported on the platform I'm testing with. So I just went with
> that. For sure there are other methods exist and the PCIe spec itself doesn't
> define how PERST# should be implemented in a form factor. It merely defines
> PERST# as an 'auxiliary signal'. So yes, other form of PERST# can exist. But for
> the sake of keeping this proposal simple, I'm considering only GPIO based
> PERST# atm.

Hmm, OK. A simple start is fine, but I'm pointing out this will quickly
show its limitations.

> Also, Tegra platforms are not converted to use pwrctrl framework and I don't
> know if the platform maintainers are interested in it or not. But if they start
> using it, we can tackle this situation by introducing a callback that
> asserts/deasserts PERST# (yes, callbacks are evil, but I don't know any other
> sensible way to support vendor specific PERST# implementations).

IMO, it's pretty fair game to at least account for things people are
doing in upstream drivers today, even if they aren't wholly ready to
adopt the new thing. It's harder to gain new users when you actively
don't support things you know the users need.

> Oh and do take a look at pcie-brcmstb driver, which I promised to move to
> pwrctrl framework for another reason. It uses multiple callbacks per SoC
> revisions for toggling PERST#. So for these usecases, having a callback in
> 'struct pci_host_bridge' would be a good fit and I may introduce it after this
> series gets in.

Sure. I think there are plenty of drivers that will need it. And that's
why I brought it up.

But if that's a "phase 2" thing, so be it.

> > > This will allow both the controller drivers and pwrctrl framework to share the
> > > PERST# (which is ugly but can't be avoided). But care must be taken to ensure
> > > that the controller drivers only assert PERST# and not deassert when pwrctrl is
> > > used. I've added the change for the Qcom driver as a reference. The Qcom driver
> > > is a slight mess because, it now has to support both new DT binding (PERST# and
> > > PHY in Root Port node) and legacy (both in Host Bridge node). So I've allowed
> > > the PERST# control only for the new binding (which is always going to use
> > > pwrctrl framework to control the component supplies).
> > > 
> > > Testing
> > > =======
> > > 
> > > This series is tested on Lenovo Thinkpad T14s laptop (with out-of-tree patch
> > > enabling PCIe WLAN card) and on RB3 Gen2 with TC9563 switch (also with the not
> > > yet merged series [2]). A big take away from this series is that, it is now
> > > possible to get rid of the controversial {start/stop}_link() callback proposed
> > > in the above mentioned switch pwrctrl driver [3].
> > 
> > This is a tiny bit tangential to the PERST# discussion, but I believe
> > there are other controller driver features that don't fit into the
> > sequence model of:
> > 
> > 1. start LTSSM (controller driver)
> > 2. pwrctrl eventually turns on power + delay per spec
> > 3. pwrctrl deasserts PERST#
> > 4. pwrctrl delays a fixed amount of time, per the CEM spec
> > 5. pwrctrl rescans bus
> > 
> > For example, tegra_pcie_dw_start_link() notes some cases where it needs
> > to take action and retry when the link doesn't come up. Similarly, I've
> > seen drivers with retry loops for cases where the link comes up, but not
> > at the expected link rate. None of this is possible if the controller
> > driver only gets to take care of #1, and has no involvement in between
> > #3 and #5.
> > 
> 
> Having this back and forth communication would make the pwrctrl driver a lot
> messier. But I believe, we could teach pwrctrl driver to detect link up (similar
> to dw_pcie_wait_for_link()) and if link didn't come up, it could do retry and
> other steps with help from controller drivers. But these things should be
> implemented only when platforms like Tegra start to show some love towards
> pwrctrl.

Never mind the lack of love you feel here :)
But I'm actively looking at drivers that don't yet fit into what pwrctrl
supports, and I'd like them to use pwrctrl someday instead of
reinventing the wheel.

You're arguing against more callbacks, and start_link()-like
functionality, but I'm pretty sure some of these things are necessities,
if you're trying to abstract power control away from controller drivers.

Again, maybe this is a problem to be solved later. But I think you're
kidding yourself that pwrctrl is ready as-is, and that you can avoid
these kinds of callbacks.

Regards,
Brian

Re: [PATCH RFC 2/3] PCI/pwrctrl: Allow pwrctrl core to control PERST# GPIO if available

[Brian Norris]

Sorry for so many individual reviews, but I've passed over this a few
times and had new questions/comments several times:

On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> PERST# is an (optional) auxiliary signal provided by the PCIe host to
> components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> sec 6.6.1.

>  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
>  {
> +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> +	int devfn;
> +
>  	pwrctrl->dev = dev;
>  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> +
> +	if (!host_bridge->perst)
> +		return;
> +
> +	devfn = of_pci_get_devfn(dev_of_node(dev));
> +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])

This seems to imply a 1:1 correlation between slots and pwrctrl devices,
almost as if you expect everyone is using drivers/pci/pwrctrl/slot.c.
But there is also endpoint-specific pwrctrl support, and there's quite
a bit of flexibility around what these hierarchies can look like.

How do you account for that?

For example, couldn't you have both a "port" and an "endpoint" pwrctrl? Would
they both grab the same PERST# GPIO here? And might that incur excessive
resets, possibly even clobbering each other?

Or what if multiple slots are governed by a single GPIO? Do you expect
the bridge perst[] array to contain redundant GPIOs?

Brian

> +		pwrctrl->perst = host_bridge->perst[PCI_SLOT(devfn)];
>  }
>  EXPORT_SYMBOL_GPL(pci_pwrctrl_init);

Re: [PATCH RFC 0/3] PCI/pwrctrl: Allow pwrctrl framework to control PERST# GPIO if available

[Manivannan Sadhasivam]

On Fri, Jul 11, 2025 at 05:04:38PM GMT, Brian Norris wrote:
> Hi,
> 
> On Wed, Jul 09, 2025 at 12:18:29PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Jul 08, 2025 at 06:39:37PM GMT, Brian Norris wrote:
> > > On Mon, Jul 07, 2025 at 11:48:37PM +0530, Manivannan Sadhasivam wrote:
> > > > Hi,
> > > > 
> > > > This series is an RFC to propose pwrctrl framework to control the PERST# GPIO
> > > > instead of letting the controller drivers to do so (which is a mistake btw).
> > > > 
> > > > Right now, the pwrctrl framework is controlling the power supplies to the
> > > > components (endpoints and such), but it is not controlling PERST#. This was
> > > > pointed out by Brian during a related conversation [1]. But we cannot just move
> > > > the PERST# control from controller drivers due to the following reasons:
> > > > 
> > > > 1. Most of the controller drivers need to assert PERST# during the controller
> > > > initialization sequence. This is mostly as per their hardware reference manual
> > > > and should not be changed.
> > > > 
> > > > 2. Controller drivers still need to toggle PERST# when pwrctrl is not used i.e.,
> > > > when the power supplies are not accurately described in PCI DT node. This can
> > > > happen on unsupported platforms and also for platforms with legacy DTs.
> > > > 
> > > > For this reason, I've kept the PERST# retrieval logic in the controller drivers
> > > > and just passed the gpio descriptors (for each slot) to the pwrctrl framework.
> > > 
> > > How sure are we that GPIOs (and *only* GPIOs) are sufficient for this
> > > feature? I've seen a few drivers that pair a GPIO with some kind of
> > > "internal" reset too, and it's not always clear that they can/should be
> > > operated separately.
> > > 
> > > For example, drivers/pci/controller/dwc/pci-imx6.c /
> > > imx_pcie_{,de}assert_core_reset(), and pcie-tegra194.c's
> > > APPL_PINMUX_PEX_RST. The tegra case especially seems pretty clear that
> > > its non-GPIO "pex_rst" is resetting an endpoint.
> > > 
> > 
> > Right. But GPIO is the most commonly used approach for implementing PERST# and
> > it is the one supported on the platform I'm testing with. So I just went with
> > that. For sure there are other methods exist and the PCIe spec itself doesn't
> > define how PERST# should be implemented in a form factor. It merely defines
> > PERST# as an 'auxiliary signal'. So yes, other form of PERST# can exist. But for
> > the sake of keeping this proposal simple, I'm considering only GPIO based
> > PERST# atm.
> 
> Hmm, OK. A simple start is fine, but I'm pointing out this will quickly
> show its limitations.
> 

I don't disagree :)

> > Also, Tegra platforms are not converted to use pwrctrl framework and I don't
> > know if the platform maintainers are interested in it or not. But if they start
> > using it, we can tackle this situation by introducing a callback that
> > asserts/deasserts PERST# (yes, callbacks are evil, but I don't know any other
> > sensible way to support vendor specific PERST# implementations).
> 
> IMO, it's pretty fair game to at least account for things people are
> doing in upstream drivers today, even if they aren't wholly ready to
> adopt the new thing. It's harder to gain new users when you actively
> don't support things you know the users need.
> 
> > Oh and do take a look at pcie-brcmstb driver, which I promised to move to
> > pwrctrl framework for another reason. It uses multiple callbacks per SoC
> > revisions for toggling PERST#. So for these usecases, having a callback in
> > 'struct pci_host_bridge' would be a good fit and I may introduce it after this
> > series gets in.
> 
> Sure. I think there are plenty of drivers that will need it. And that's
> why I brought it up.
> 
> But if that's a "phase 2" thing, so be it.
> 
> > > > This will allow both the controller drivers and pwrctrl framework to share the
> > > > PERST# (which is ugly but can't be avoided). But care must be taken to ensure
> > > > that the controller drivers only assert PERST# and not deassert when pwrctrl is
> > > > used. I've added the change for the Qcom driver as a reference. The Qcom driver
> > > > is a slight mess because, it now has to support both new DT binding (PERST# and
> > > > PHY in Root Port node) and legacy (both in Host Bridge node). So I've allowed
> > > > the PERST# control only for the new binding (which is always going to use
> > > > pwrctrl framework to control the component supplies).
> > > > 
> > > > Testing
> > > > =======
> > > > 
> > > > This series is tested on Lenovo Thinkpad T14s laptop (with out-of-tree patch
> > > > enabling PCIe WLAN card) and on RB3 Gen2 with TC9563 switch (also with the not
> > > > yet merged series [2]). A big take away from this series is that, it is now
> > > > possible to get rid of the controversial {start/stop}_link() callback proposed
> > > > in the above mentioned switch pwrctrl driver [3].
> > > 
> > > This is a tiny bit tangential to the PERST# discussion, but I believe
> > > there are other controller driver features that don't fit into the
> > > sequence model of:
> > > 
> > > 1. start LTSSM (controller driver)
> > > 2. pwrctrl eventually turns on power + delay per spec
> > > 3. pwrctrl deasserts PERST#
> > > 4. pwrctrl delays a fixed amount of time, per the CEM spec
> > > 5. pwrctrl rescans bus
> > > 
> > > For example, tegra_pcie_dw_start_link() notes some cases where it needs
> > > to take action and retry when the link doesn't come up. Similarly, I've
> > > seen drivers with retry loops for cases where the link comes up, but not
> > > at the expected link rate. None of this is possible if the controller
> > > driver only gets to take care of #1, and has no involvement in between
> > > #3 and #5.
> > > 
> > 
> > Having this back and forth communication would make the pwrctrl driver a lot
> > messier. But I believe, we could teach pwrctrl driver to detect link up (similar
> > to dw_pcie_wait_for_link()) and if link didn't come up, it could do retry and
> > other steps with help from controller drivers. But these things should be
> > implemented only when platforms like Tegra start to show some love towards
> > pwrctrl.
> 
> Never mind the lack of love you feel here :)
> But I'm actively looking at drivers that don't yet fit into what pwrctrl
> supports, and I'd like them to use pwrctrl someday instead of
> reinventing the wheel.
> 

I'm not against supporting these controller drivers/platforms in pwrctrl. It's
just that I do not want to implement solutions now without having users. But I'm
glad that you are bringing these up. I'm adding these to my pwrctrl/todo list.

> You're arguing against more callbacks, and start_link()-like
> functionality, but I'm pretty sure some of these things are necessities,
> if you're trying to abstract power control away from controller drivers.
> 

Well, that's my personal preference. These days I feel (mostly after spending my
time as a maintainer of PCI controller drivers) that callbacks are evil and they
pay way for 'midlayers'. But having said that, I myself implemented callbacks
whereever I felt that no other options were possible. And here also, I'm just
claiming that this series avoids the '{start/stop}_link' callbacks which was
hated by many (including me) as it ties the Qcom switch driver implementing
these callbacks to be tied to a specific platform.

> Again, maybe this is a problem to be solved later. But I think you're
> kidding yourself that pwrctrl is ready as-is, and that you can avoid
> these kinds of callbacks.
> 

No, I never claimed that pwrctrl is perfect and it would solve all the PCI power
issues. But I'm happy that it atleast solves a couple of issues and allows me to
address the rest of them. We had been talking about a framework like this for
several years without any upstream solution. But now we finally have one and I'm
merely trying to make use of it to address issues.

- Mani

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

Re: [PATCH RFC 3/3] PCI: qcom: Allow pwrctrl framework to control PERST#

[Manivannan Sadhasivam]

On Fri, Jul 11, 2025 at 04:42:47PM GMT, Brian Norris wrote:
> Hi,
> 
> On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote:
> > Since the Qcom platforms rely on pwrctrl framework to control the power
> > supplies, allow it to control PERST# also. PERST# should be toggled during
> > the power-on and power-off scenarios.
> > 
> > But the controller driver still need to assert PERST# during the controller
> > initialization. So only skip the deassert if pwrctrl usage is detected. The
> > pwrctrl framework will deassert PERST# after turning on the supplies.
> > 
> > The usage of pwrctrl framework is detected based on the new DT binding
> > i.e., with the presence of PERST# and PHY properties in the Root Port node
> > instead of the host bridge node.
> 
> I just noticed what this paragraph means. IIUC, this implies that in
> your new binding, one *must* describe one or more *-supply in the port
> or endpoint device(s). Otherwise, no pwrctrl devices will be created,
> and no one will deassert PERST# for you. My understanding is that
> *-supply is optional, and so this is a poor requirement.
> 

Your understanding is correct. But the problem is, you thought that pwrctrl
would work across all platforms without any modifications, which unfortunately
is not true and is the main source of confusion. And I never claim anywhere that
pwrctrl is ready for all platforms. I just want platforms to start showing
interest towards it and we will collectively solve the issues. Or I'll be happy
to solve the issues if platform maintainers show interest towards it. This is
what currently happening with brcmstb. I signed up for the transition to
pwrctrl as their out-of-tree is breaking with pwrctrl.

Right now, we indeed create pwrctrl device based on the presence of power
supplies as that's how the sole user of pwrctrl (Qcom platforms) behave. But
sure, for some other platforms we might have only 'reset-gpios'. When we have to
support those platforms, we will extend the logic.

> And even if all QCOM device trees manage to have external regulators
> described in their device trees, there are certainly other systems where
> the driver might (optionally) use pwrctrl for some devices, but others
> will establish power on their own (e.g., PCIe tied to some other system
> power rail).
> 
> I think you either need the PCI core to tell you whether pwrctrl is in
> use, or else you need to disconnect this PERST# support from pwrctrl.
> 

It is not straightforward for the PCI core to tell whether pwrctrl is in use or
not. pwrctrl has no devicetree representation as it is not a separate hardware
entity. It just reuses the PCI device DT node. So I used the -supply properties
to assume that the pwrctrl device will be used. And almost none of the upstream
DTS has -supply properties in the PCI child node (only exception is brcmstb
where they define -supply properties in the PCI child node, but only in the DT
binding). So that added up.

> > When the legacy binding is used, PERST# is only controlled by the
> > controller driver since it is not reliable to detect whether pwrctrl is
> > used or not. So the legacy platforms are untouched by this commit.
> > 
> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c |  1 +
> >  drivers/pci/controller/dwc/pcie-designware.h      |  1 +
> >  drivers/pci/controller/dwc/pcie-qcom.c            | 26 ++++++++++++++++++++++-
> >  3 files changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index af6c91ec7312bab6c6e5ad35b051d0f452fe7b8d..e45f53bb135a75963318666a479eb6d9582f30eb 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -492,6 +492,7 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >  		return -ENOMEM;
> >  
> >  	pp->bridge = bridge;
> > +	bridge->perst = pp->perst;
> >  
> >  	ret = dw_pcie_host_get_resources(pp);
> >  	if (ret)
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 4165c49a0a5059cab92dee3c47f8024af9d840bd..7b28f76ebf6a2de8781746eba43a8e3ad9a5cbb2 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -430,6 +430,7 @@ struct dw_pcie_rp {
> >  	struct resource		*msg_res;
> >  	bool			use_linkup_irq;
> >  	struct pci_eq_presets	presets;
> > +	struct gpio_desc	**perst;
> >  };
> >  
> >  struct dw_pcie_ep_ops {
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 620ac7cf09472b84c37e83ee3ce40e94a1d9d878..61e1d0d6469030c549328ab4d8c65d5377d525e3 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -313,6 +313,11 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> >  
> >  static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> >  {
> > +	struct dw_pcie_rp *pp = &pcie->pci->pp;
> > +
> > +	if (pp->perst)
> 
> You're doing a non-NULL check here, but you forgot to reinitialize it to
> NULL in some cases below (qcom_pcie_parse_ports()).
> 
> This is also apparently where you assume |perst| implies pwrctrl. Per
> above, I don't think that's a good assumption.
> 

Atleast this assumption holds true for now. Otherwise, I'd have to implement
callbacks without any users.

> > +		return;
> > +
> >  	/* Ensure that PERST has been asserted for at least 100 ms */
> >  	msleep(PCIE_T_PVPERL_MS);
> >  	qcom_perst_assert(pcie, false);
> ...
> >  static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
> >  {
> > +	struct dw_pcie_rp *pp = &pcie->pci->pp;
> >  	struct device *dev = pcie->pci->dev;
> >  	struct qcom_pcie_port *port, *tmp;
> > +	int child_cnt;
> >  	int ret = -ENOENT;
> >  
> > +	child_cnt = of_get_available_child_count(dev->of_node);
> > +	if (!child_cnt)
> > +		return ret;
> > +
> > +	pp->perst = kcalloc(child_cnt, sizeof(struct gpio_desc *), GFP_KERNEL);
> > +	if (!pp->perst)
> > +		return -ENOMEM;
> > +
> >  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> >  		ret = qcom_pcie_parse_port(pcie, of_port);
> >  		if (ret)
> > @@ -1747,6 +1769,7 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
> >  	return ret;
> >  
> >  err_port_del:
> > +	kfree(pp->perst);
> 
> You need this too:
> 
> 	pp->perst = NULL;
> 
> Otherwise, you leave a non-NULL (invalid) pointer for cases that
> qcom_pcie_parse_legacy_binding() might handle.
> 

Right, thanks for spotting!

- Mani

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

Re: [PATCH RFC 2/3] PCI/pwrctrl: Allow pwrctrl core to control PERST# GPIO if available

[Manivannan Sadhasivam]

On Fri, Jul 11, 2025 at 05:38:16PM GMT, Brian Norris wrote:
> Sorry for so many individual reviews, but I've passed over this a few
> times and had new questions/comments several times:
> 

That's fine. I'm happy to answer as someone other than me is interested in
pwrctrl :)

> On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> > PERST# is an (optional) auxiliary signal provided by the PCIe host to
> > components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> > sec 6.6.1.
> 
> >  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
> >  {
> > +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> > +	int devfn;
> > +
> >  	pwrctrl->dev = dev;
> >  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> > +
> > +	if (!host_bridge->perst)
> > +		return;
> > +
> > +	devfn = of_pci_get_devfn(dev_of_node(dev));
> > +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
> 
> This seems to imply a 1:1 correlation between slots and pwrctrl devices,
> almost as if you expect everyone is using drivers/pci/pwrctrl/slot.c.
> But there is also endpoint-specific pwrctrl support, and there's quite
> a bit of flexibility around what these hierarchies can look like.
> 
> How do you account for that?
> 
> For example, couldn't you have both a "port" and an "endpoint" pwrctrl? Would
> they both grab the same PERST# GPIO here? And might that incur excessive
> resets, possibly even clobbering each other?
> 

If both port and endpoint nodes are present, then only one will contain
'reset-gpios'. Right now, the DT binding only supports PERST#, WAKE#, CLKREQ#
properties in RP node, but that won't work if we have multiple lines per slot/
controller. Ideally, we would want the properties to be present in endpoint node
if available. But if we have only standard expansion slots, then it makes sense
to define them in the port node. But doing so, we can only expect the slot to
have only one instance of these properties as we cannot reliably map which
property corresponds to the endpoint.

I've opened a dtschema issue for this:
https://github.com/devicetree-org/dt-schema/issues/168

This is what I have in my mind:

1) For expansion slots:

pcie_port0{
	device_type = "pci";
	reg = <0x0 0x0 0x0 0x0 0x0>;
	bus-range = <0x01 0xff>

	reset-gpios = <>;
	wake-gpios = <>;
};

2) For endpoint nodes:

pcie_port0{
	device_type = "pci";
	reg = <0x0 0x0 0x0 0x0 0x0>;
	bus-range = <0x01 0xff>

	pcie@0 {
		reg = <0x10000 0x0 0x0 0x0 0x0>;
		reset-gpios = <>;
		wake-gpios = <>;
	};

	pcie@1 {
		reg = <0x10800 0x0 0x0 0x0 0x0>;
		reset-gpios = <>;
		wake-gpios = <>;
	};

	...
};

I don't think there is any usecase to have both slot and endpoint nodes defining
these properties.

But for your initial question on what if the platform doesn't define any supply
and uses non-GPIO based PERST#/WAKE#/CLKREQ#, I don't know how to let PCI core
create the pwrctrl device. This is something we need to brainstorm.

> Or what if multiple slots are governed by a single GPIO? Do you expect
> the bridge perst[] array to contain redundant GPIOs?
> 

It is common for devices within a slot/hierarchy to share these GPIOs (with some
drawbacks), but I'm not sure how all slots can share a single GPIO. That would
mean, if the host wants to reset one endpoint in a slot, it has to reset all the
endpoints connected to all slots. Sounds like a bizarre design to me.

- Mani

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

Re: [PATCH RFC 2/3] PCI/pwrctrl: Allow pwrctrl core to control PERST# GPIO if available

[Manivannan Sadhasivam]

On Sat, Jul 12, 2025 at 01:59:34PM GMT, Manivannan Sadhasivam wrote:
> On Fri, Jul 11, 2025 at 05:38:16PM GMT, Brian Norris wrote:
> > Sorry for so many individual reviews, but I've passed over this a few
> > times and had new questions/comments several times:
> > 
> 
> That's fine. I'm happy to answer as someone other than me is interested in
> pwrctrl :)
> 
> > On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> > > PERST# is an (optional) auxiliary signal provided by the PCIe host to
> > > components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> > > sec 6.6.1.
> > 
> > >  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
> > >  {
> > > +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> > > +	int devfn;
> > > +
> > >  	pwrctrl->dev = dev;
> > >  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> > > +
> > > +	if (!host_bridge->perst)
> > > +		return;
> > > +
> > > +	devfn = of_pci_get_devfn(dev_of_node(dev));
> > > +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
> > 
> > This seems to imply a 1:1 correlation between slots and pwrctrl devices,
> > almost as if you expect everyone is using drivers/pci/pwrctrl/slot.c.
> > But there is also endpoint-specific pwrctrl support, and there's quite
> > a bit of flexibility around what these hierarchies can look like.
> > 
> > How do you account for that?
> > 
> > For example, couldn't you have both a "port" and an "endpoint" pwrctrl? Would
> > they both grab the same PERST# GPIO here? And might that incur excessive
> > resets, possibly even clobbering each other?
> > 
> 
> If both port and endpoint nodes are present, then only one will contain
> 'reset-gpios'. Right now, the DT binding only supports PERST#, WAKE#, CLKREQ#
> properties in RP node, but that won't work if we have multiple lines per slot/
> controller. Ideally, we would want the properties to be present in endpoint node
> if available. But if we have only standard expansion slots, then it makes sense
> to define them in the port node. But doing so, we can only expect the slot to
> have only one instance of these properties as we cannot reliably map which
> property corresponds to the endpoint.
> 
> I've opened a dtschema issue for this:
> https://github.com/devicetree-org/dt-schema/issues/168
> 

I realized that there is no need to define these properties (PERST#, WAKE#,
CLKREQ#) in the endpoint node (the DT binding also doesn't allow now anyway).
These properties should just exist in the Root Port node as there can be only
one set per hierarchy i.e., Root Complex would only use one set of these GPIOs
per Root Port and the endpoint need to share them.

So I closed the dtschema issue.

- Mani

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

Re: [PATCH RFC 3/3] PCI: qcom: Allow pwrctrl framework to control PERST#

[Brian Norris]

Hi Manivannan,

Sorry for some delay. Things get busy, and I don't get the time for
proper review/reply sometimes...

On Sat, Jul 12, 2025 at 11:50:43AM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jul 11, 2025 at 04:42:47PM GMT, Brian Norris wrote:
> > Hi,
> > 
> > On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote:
> > > Since the Qcom platforms rely on pwrctrl framework to control the power
> > > supplies, allow it to control PERST# also. PERST# should be toggled during
> > > the power-on and power-off scenarios.
> > > 
> > > But the controller driver still need to assert PERST# during the controller
> > > initialization. So only skip the deassert if pwrctrl usage is detected. The
> > > pwrctrl framework will deassert PERST# after turning on the supplies.
> > > 
> > > The usage of pwrctrl framework is detected based on the new DT binding
> > > i.e., with the presence of PERST# and PHY properties in the Root Port node
> > > instead of the host bridge node.
> > 
> > I just noticed what this paragraph means. IIUC, this implies that in
> > your new binding, one *must* describe one or more *-supply in the port
> > or endpoint device(s). Otherwise, no pwrctrl devices will be created,
> > and no one will deassert PERST# for you. My understanding is that
> > *-supply is optional, and so this is a poor requirement.
> > 
> 
> Your understanding is correct. But the problem is, you thought that pwrctrl
> would work across all platforms without any modifications, which unfortunately

I do not think this. Of course there's some modification needed on
occasion, especially when drivers assume they can poll for the link to
come up when power isn't ready, or if they want to get PERST# right
(i.e., $subject).

OTOH, I don't think you can claim that platforms *don't* support
pwrctrl. If a driver has a well-behaved start_link() behavior and
doesn't otherwise manage slot/endpoint *-supply properties (a la
pcie-brcmstb), it should mostly work without further involvement.

But crucially, that changes with PERST#. And I think you're making
very narrow assumptions when you do that.

> is not true and is the main source of confusion. And I never claim anywhere that
> pwrctrl is ready for all platforms. I just want platforms to start showing
> interest towards it and we will collectively solve the issues. Or I'll be happy
> to solve the issues if platform maintainers show interest towards it. This is
> what currently happening with brcmstb. I signed up for the transition to
> pwrctrl as their out-of-tree is breaking with pwrctrl.
> 
> Right now, we indeed create pwrctrl device based on the presence of power
> supplies as that's how the sole user of pwrctrl (Qcom platforms) behave. But

I don't see how this is really Qualcomm specific, unless you simply
require that all new Qcom DTs specify external *-supply. I don't see
that in your Documentation/devicetree/bindings/pci/qcom*.yaml though,
and I don't think that's reasonable.

> sure, for some other platforms we might have only 'reset-gpios'. When we have to
> support those platforms, we will extend the logic.

The thing is, you don't have 100% control over this. You sound like you
only want to support device trees that are shipped in the upstream
kernel, but that's not how they work -- it's totally valid to ship
non-upstream device trees, if you follow the DT bindings. And you've
already hit that pitfall with brcmstb.

Suppose you have a Qcom platform today, with pwrctrl support, and:

 1. it has GPIO PERST#
 2. some boards have external power controls for the endpoint. *-supply
    nodes are described for the endpoint, and pwrctrl is in use.
 3. some boards have hardwired power that is always-on / on at boot (no
    *-supply node, no pwrctrl).

As you've written it today, #3 will no longer work, since you're
deferring PERST# to pwrctrl, but pwrctrl never gets involved.

Crucially, you can't read the driver source to tell the difference
between #2 and #3, and it's not even in the binding schema. Now magnify
this across other drivers that might support this.

I have boards like #2 and #3, and I don't know how I'm supposed to
develop my driver.

> > And even if all QCOM device trees manage to have external regulators
> > described in their device trees, there are certainly other systems where
> > the driver might (optionally) use pwrctrl for some devices, but others
> > will establish power on their own (e.g., PCIe tied to some other system
> > power rail).
> > 
> > I think you either need the PCI core to tell you whether pwrctrl is in
> > use, or else you need to disconnect this PERST# support from pwrctrl.
> > 
> 
> It is not straightforward for the PCI core to tell whether pwrctrl is in use or
> not.

Yes, well this seems like a fundamental recurring problem at the root
here. This agnostic design just causes more problems, IMO.

> pwrctrl has no devicetree representation as it is not a separate hardware
> entity. It just reuses the PCI device DT node. So I used the -supply properties
> to assume that the pwrctrl device will be used. And almost none of the upstream
> DTS has -supply properties in the PCI child node (only exception is brcmstb
> where they define -supply properties in the PCI child node, but only in the DT
> binding). So that added up.

You gotta work off DT bindings and schema to make assertions. You can't
just guess based on in-tree device trees, and so you can't prove
non-existence, if it's not explicit in the bindings.

Brian

Re: [PATCH RFC 2/3] PCI/pwrctrl: Allow pwrctrl core to control PERST# GPIO if available

[Brian Norris]

Thanks for clearing up some confusion. I was misled on some aspects. But
I think there's still a problem in here:

On Thu, Jul 24, 2025 at 07:43:38PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Jul 12, 2025 at 01:59:34PM GMT, Manivannan Sadhasivam wrote:
> > On Fri, Jul 11, 2025 at 05:38:16PM GMT, Brian Norris wrote:
> > > On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> > > > PERST# is an (optional) auxiliary signal provided by the PCIe host to
> > > > components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> > > > sec 6.6.1.
> > > 
> > > >  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
> > > >  {
> > > > +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> > > > +	int devfn;
> > > > +
> > > >  	pwrctrl->dev = dev;
> > > >  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> > > > +
> > > > +	if (!host_bridge->perst)
> > > > +		return;
> > > > +
> > > > +	devfn = of_pci_get_devfn(dev_of_node(dev));
> > > > +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
> > > 
> > > This seems to imply a 1:1 correlation between slots and pwrctrl devices,
> > > almost as if you expect everyone is using drivers/pci/pwrctrl/slot.c.
> > > But there is also endpoint-specific pwrctrl support, and there's quite
> > > a bit of flexibility around what these hierarchies can look like.
> > > 
> > > How do you account for that?
> > > 
> > > For example, couldn't you have both a "port" and an "endpoint" pwrctrl? Would
> > > they both grab the same PERST# GPIO here? And might that incur excessive
> > > resets, possibly even clobbering each other?
...
> I realized that there is no need to define these properties (PERST#, WAKE#,
> CLKREQ#) in the endpoint node (the DT binding also doesn't allow now anyway).
> These properties should just exist in the Root Port node as there can be only
> one set per hierarchy i.e., Root Complex would only use one set of these GPIOs
> per Root Port and the endpoint need to share them.

That implies it's not a 1:1 correlation between PERST# GPIO and pwrctrl
device. Multiple endpoints might need powered up, but they may share a
PERST#. I don't think this patch solves this properly, as it allows the
first one to deassert PERST# before the other(s) are powered.

Or maybe I'm missing something yet again :)

Brian

Re: [PATCH RFC 2/3] PCI/pwrctrl: Allow pwrctrl core to control PERST# GPIO if available

[Manivannan Sadhasivam]

On Fri, Jul 25, 2025 at 02:04:28PM GMT, Brian Norris wrote:
> Thanks for clearing up some confusion. I was misled on some aspects. But
> I think there's still a problem in here:
> 
> On Thu, Jul 24, 2025 at 07:43:38PM +0530, Manivannan Sadhasivam wrote:
> > On Sat, Jul 12, 2025 at 01:59:34PM GMT, Manivannan Sadhasivam wrote:
> > > On Fri, Jul 11, 2025 at 05:38:16PM GMT, Brian Norris wrote:
> > > > On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> > > > > PERST# is an (optional) auxiliary signal provided by the PCIe host to
> > > > > components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> > > > > sec 6.6.1.
> > > > 
> > > > >  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
> > > > >  {
> > > > > +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> > > > > +	int devfn;
> > > > > +
> > > > >  	pwrctrl->dev = dev;
> > > > >  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> > > > > +
> > > > > +	if (!host_bridge->perst)
> > > > > +		return;
> > > > > +
> > > > > +	devfn = of_pci_get_devfn(dev_of_node(dev));
> > > > > +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
> > > > 
> > > > This seems to imply a 1:1 correlation between slots and pwrctrl devices,
> > > > almost as if you expect everyone is using drivers/pci/pwrctrl/slot.c.
> > > > But there is also endpoint-specific pwrctrl support, and there's quite
> > > > a bit of flexibility around what these hierarchies can look like.
> > > > 
> > > > How do you account for that?
> > > > 
> > > > For example, couldn't you have both a "port" and an "endpoint" pwrctrl? Would
> > > > they both grab the same PERST# GPIO here? And might that incur excessive
> > > > resets, possibly even clobbering each other?
> ...
> > I realized that there is no need to define these properties (PERST#, WAKE#,
> > CLKREQ#) in the endpoint node (the DT binding also doesn't allow now anyway).
> > These properties should just exist in the Root Port node as there can be only
> > one set per hierarchy i.e., Root Complex would only use one set of these GPIOs
> > per Root Port and the endpoint need to share them.
> 
> That implies it's not a 1:1 correlation between PERST# GPIO and pwrctrl
> device. Multiple endpoints might need powered up, but they may share a
> PERST#. I don't think this patch solves this properly, as it allows the
> first one to deassert PERST# before the other(s) are powered.
> 

You are right. This series doesn't take account of this configuration and I plan
to incorporate it in the next one. It might take some time as supporting such
configuration (and others that we discussed in this thread) is not
straightforward.

- Mani

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

Re: [PATCH RFC 3/3] PCI: qcom: Allow pwrctrl framework to control PERST#

[Manivannan Sadhasivam]

On Fri, Jul 25, 2025 at 01:53:02PM GMT, Brian Norris wrote:
> Hi Manivannan,
> 
> Sorry for some delay. Things get busy, and I don't get the time for
> proper review/reply sometimes...
> 
> On Sat, Jul 12, 2025 at 11:50:43AM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Jul 11, 2025 at 04:42:47PM GMT, Brian Norris wrote:
> > > Hi,
> > > 
> > > On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote:
> > > > Since the Qcom platforms rely on pwrctrl framework to control the power
> > > > supplies, allow it to control PERST# also. PERST# should be toggled during
> > > > the power-on and power-off scenarios.
> > > > 
> > > > But the controller driver still need to assert PERST# during the controller
> > > > initialization. So only skip the deassert if pwrctrl usage is detected. The
> > > > pwrctrl framework will deassert PERST# after turning on the supplies.
> > > > 
> > > > The usage of pwrctrl framework is detected based on the new DT binding
> > > > i.e., with the presence of PERST# and PHY properties in the Root Port node
> > > > instead of the host bridge node.
> > > 
> > > I just noticed what this paragraph means. IIUC, this implies that in
> > > your new binding, one *must* describe one or more *-supply in the port
> > > or endpoint device(s). Otherwise, no pwrctrl devices will be created,
> > > and no one will deassert PERST# for you. My understanding is that
> > > *-supply is optional, and so this is a poor requirement.
> > > 
> > 
> > Your understanding is correct. But the problem is, you thought that pwrctrl
> > would work across all platforms without any modifications, which unfortunately
> 
> I do not think this. Of course there's some modification needed on
> occasion, especially when drivers assume they can poll for the link to
> come up when power isn't ready, or if they want to get PERST# right
> (i.e., $subject).
> 
> OTOH, I don't think you can claim that platforms *don't* support
> pwrctrl. If a driver has a well-behaved start_link() behavior and
> doesn't otherwise manage slot/endpoint *-supply properties (a la
> pcie-brcmstb), it should mostly work without further involvement.
> 

I agree. As I dig more, there seems to be a gazellion of these combinations
exist. In the next version, I'll try to accomodate most of them.

> But crucially, that changes with PERST#. And I think you're making
> very narrow assumptions when you do that.
> 

This series does indeed. I wanted to start small, but I realized that going too
simplistic won't work for everyone.

> > is not true and is the main source of confusion. And I never claim anywhere that
> > pwrctrl is ready for all platforms. I just want platforms to start showing
> > interest towards it and we will collectively solve the issues. Or I'll be happy
> > to solve the issues if platform maintainers show interest towards it. This is
> > what currently happening with brcmstb. I signed up for the transition to
> > pwrctrl as their out-of-tree is breaking with pwrctrl.
> > 
> > Right now, we indeed create pwrctrl device based on the presence of power
> > supplies as that's how the sole user of pwrctrl (Qcom platforms) behave. But
> 
> I don't see how this is really Qualcomm specific, unless you simply
> require that all new Qcom DTs specify external *-supply. I don't see
> that in your Documentation/devicetree/bindings/pci/qcom*.yaml though,
> and I don't think that's reasonable.
> 
> > sure, for some other platforms we might have only 'reset-gpios'. When we have to
> > support those platforms, we will extend the logic.
> 
> The thing is, you don't have 100% control over this. You sound like you
> only want to support device trees that are shipped in the upstream
> kernel, but that's not how they work -- it's totally valid to ship
> non-upstream device trees, if you follow the DT bindings. And you've
> already hit that pitfall with brcmstb.
> 
> Suppose you have a Qcom platform today, with pwrctrl support, and:
> 
>  1. it has GPIO PERST#
>  2. some boards have external power controls for the endpoint. *-supply
>     nodes are described for the endpoint, and pwrctrl is in use.
>  3. some boards have hardwired power that is always-on / on at boot (no
>     *-supply node, no pwrctrl).
> 
> As you've written it today, #3 will no longer work, since you're
> deferring PERST# to pwrctrl, but pwrctrl never gets involved.
> 
> Crucially, you can't read the driver source to tell the difference
> between #2 and #3, and it's not even in the binding schema. Now magnify
> this across other drivers that might support this.
> 
> I have boards like #2 and #3, and I don't know how I'm supposed to
> develop my driver.
> 

pci_pwrctrl_create_device() should be really smart to figure out these
combinations. Let me see how smart it becomes.

> > > And even if all QCOM device trees manage to have external regulators
> > > described in their device trees, there are certainly other systems where
> > > the driver might (optionally) use pwrctrl for some devices, but others
> > > will establish power on their own (e.g., PCIe tied to some other system
> > > power rail).
> > > 
> > > I think you either need the PCI core to tell you whether pwrctrl is in
> > > use, or else you need to disconnect this PERST# support from pwrctrl.
> > > 
> > 
> > It is not straightforward for the PCI core to tell whether pwrctrl is in use or
> > not.
> 
> Yes, well this seems like a fundamental recurring problem at the root
> here. This agnostic design just causes more problems, IMO.
> 
> > pwrctrl has no devicetree representation as it is not a separate hardware
> > entity. It just reuses the PCI device DT node. So I used the -supply properties
> > to assume that the pwrctrl device will be used. And almost none of the upstream
> > DTS has -supply properties in the PCI child node (only exception is brcmstb
> > where they define -supply properties in the PCI child node, but only in the DT
> > binding). So that added up.
> 
> You gotta work off DT bindings and schema to make assertions. You can't
> just guess based on in-tree device trees, and so you can't prove
> non-existence, if it's not explicit in the bindings.
> 

This is the actual problem here. We cannot introduce any changes in the binding
for the sake of pwrctrl. Pwrctrl is just a kernel framework which is supposed to
work with the existing bindings and DTS. For sure we can ammend the binding to
make it strict. Like, mandating the -supply property even if it is always on as
the endpoint definitely needs atleast one supply from the host (well from the
system PMIC atleast). But nothing more.

- Mani

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