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

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

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

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

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

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

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

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

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

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

-- 
2.45.2

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

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

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

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

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

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

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

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

To handle this complexity, pwrctrl framework was introduced.

- Mani

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

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

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

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

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

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

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

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

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

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

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

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

- Mani

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

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

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

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

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

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

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

True.

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

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

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

Maybe Rob could share more of the insight.

- Mani

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