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

Manivannan Sadhasivam via B4 Relay posted 5 patches 1 month ago
There is a newer version of this series
[PATCH v2 5/5] PCI: qcom: Allow pwrctrl core to toggle PERST# for new DT binding
Posted by Manivannan Sadhasivam via B4 Relay 1 month ago
From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

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

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

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

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

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 78355d12f10d263a0bb052e24c1e2d5e8f68603d..3c5c65d7d97cac186e1b671f80ba7296ad226d68 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -276,6 +276,7 @@ struct qcom_pcie_port {
 struct qcom_pcie_perst {
 	struct list_head list;
 	struct gpio_desc *desc;
+	struct device_node *np;
 };
 
 struct qcom_pcie {
@@ -298,11 +299,50 @@ struct qcom_pcie {
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
 
-static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
+static struct gpio_desc *qcom_find_perst(struct qcom_pcie *pcie, struct device_node *np)
+{
+	struct qcom_pcie_perst *perst;
+
+	list_for_each_entry(perst, &pcie->perst, list) {
+		if (np == perst->np)
+			return perst->desc;
+	}
+
+	return NULL;
+}
+
+static void qcom_toggle_perst_per_device(struct qcom_pcie *pcie,
+					 struct device_node *np, bool assert)
+{
+	int val = assert ? 1 : 0;
+	struct gpio_desc *perst;
+
+	perst = qcom_find_perst(pcie, np);
+	if (perst)
+		goto toggle_perst;
+
+	/*
+	 * If PERST# is not available in the current node, try the parent. This
+	 * fallback is needed if the current node belongs to an endpoint or
+	 * switch upstream port.
+	 */
+	if (np->parent)
+		perst = qcom_find_perst(pcie, np->parent);
+
+toggle_perst:
+	/* 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;
 	int val = assert ? 1 : 0;
 
+	if (np)
+		return qcom_toggle_perst_per_device(pcie, np, assert);
+
 	if (list_empty(&pcie->perst))
 		gpiod_set_value_cansleep(pcie->reset, val);
 
@@ -310,22 +350,34 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
 		gpiod_set_value_cansleep(perst->desc, val);
 }
 
-static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
+static void qcom_ep_reset_assert(struct qcom_pcie *pcie, struct device_node *np)
 {
-	qcom_perst_assert(pcie, true);
+	qcom_perst_reset(pcie, np, true);
 	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
 }
 
-static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
+static void qcom_ep_reset_deassert(struct qcom_pcie *pcie,
+				   struct device_node *np)
 {
 	struct dw_pcie_rp *pp = &pcie->pci->pp;
 
 	msleep(PCIE_T_PVPERL_MS);
-	qcom_perst_assert(pcie, false);
+	qcom_perst_reset(pcie, np, false);
 	if (!pp->use_linkup_irq)
 		msleep(PCIE_RESET_CONFIG_WAIT_MS);
 }
 
+static void qcom_pcie_toggle_perst(struct pci_host_bridge *bridge,
+				    struct device_node *np, bool assert)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(bridge->dev.parent);
+
+	if (assert)
+		qcom_ep_reset_assert(pcie, np);
+	else
+		qcom_ep_reset_deassert(pcie, np);
+}
+
 static int qcom_pcie_start_link(struct dw_pcie *pci)
 {
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
@@ -1320,7 +1372,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)
@@ -1336,7 +1388,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 (list_empty(&pcie->perst))
+		qcom_ep_reset_deassert(pcie, NULL);
 
 	if (pcie->cfg->ops->config_sid) {
 		ret = pcie->cfg->ops->config_sid(pcie);
@@ -1344,10 +1402,12 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 			goto err_assert_reset;
 	}
 
+	pci->pp.bridge->toggle_perst = qcom_pcie_toggle_perst;
+
 	return 0;
 
 err_assert_reset:
-	qcom_ep_reset_assert(pcie);
+	qcom_ep_reset_assert(pcie, NULL);
 err_disable_phy:
 	qcom_pcie_phy_power_off(pcie);
 err_deinit:
@@ -1361,7 +1421,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);
 }
@@ -1740,6 +1800,9 @@ static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
 		return -ENOMEM;
 
 	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, &pcie->perst);
 
 parse_child_node:
@@ -1803,8 +1866,10 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 		list_del(&port->list);
 	}
 
-	list_for_each_entry_safe(perst, tmp_perst, &pcie->perst, list)
+	list_for_each_entry_safe(perst, tmp_perst, &pcie->perst, list) {
+		of_node_put(perst->np);
 		list_del(&perst->list);
+	}
 
 	return ret;
 }
@@ -2044,8 +2109,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	qcom_pcie_phy_exit(pcie);
 	list_for_each_entry_safe(port, tmp_port, &pcie->ports, list)
 		list_del(&port->list);
-	list_for_each_entry_safe(perst, tmp_perst, &pcie->perst, list)
+	list_for_each_entry_safe(perst, tmp_perst, &pcie->perst, list) {
+		of_node_put(perst->np);
 		list_del(&perst->list);
+	}
 err_pm_runtime_put:
 	pm_runtime_put(dev);
 	pm_runtime_disable(dev);

-- 
2.45.2
Re: [PATCH v2 5/5] PCI: qcom: Allow pwrctrl core to toggle PERST# for new DT binding
Posted by Bjorn Helgaas 3 weeks, 3 days ago
On Wed, Sep 03, 2025 at 12:43:27PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> If the platform is using the new DT binding, let the pwrctrl core toggle
> PERST# for the device. This is achieved by populating the
> 'pci_host_bridge::toggle_perst' callback with qcom_pcie_toggle_perst().

Can we say something here about how to identify a "new DT binding"?
I assume there is a DT property or something that makes it "new"?

> qcom_pcie_toggle_perst() will find the PERST# GPIO descriptor associated
> with the supplied 'device_node' and toggles PERST#. If PERST# is not found
> in the supplied node, the function will look for PERST# in the parent node
> as a fallback. This is needed since PERST# won't be available in the
> endpoint node as per the DT binding.
> 
> Note that the driver still asserts PERST# during the controller
> initialization as it is needed as per the hardware documentation. Apart
> from that, the driver wouldn't touch PERST# for the new binding.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 89 +++++++++++++++++++++++++++++-----
>  1 file changed, 78 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 78355d12f10d263a0bb052e24c1e2d5e8f68603d..3c5c65d7d97cac186e1b671f80ba7296ad226d68 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -276,6 +276,7 @@ struct qcom_pcie_port {
>  struct qcom_pcie_perst {
>  	struct list_head list;
>  	struct gpio_desc *desc;
> +	struct device_node *np;
>  };
>  
>  struct qcom_pcie {
> @@ -298,11 +299,50 @@ struct qcom_pcie {
>  
>  #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
>  
> -static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> +static struct gpio_desc *qcom_find_perst(struct qcom_pcie *pcie, struct device_node *np)
> +{
> +	struct qcom_pcie_perst *perst;
> +
> +	list_for_each_entry(perst, &pcie->perst, list) {
> +		if (np == perst->np)
> +			return perst->desc;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void qcom_toggle_perst_per_device(struct qcom_pcie *pcie,
> +					 struct device_node *np, bool assert)
> +{
> +	int val = assert ? 1 : 0;
> +	struct gpio_desc *perst;
> +
> +	perst = qcom_find_perst(pcie, np);
> +	if (perst)
> +		goto toggle_perst;
> +
> +	/*
> +	 * If PERST# is not available in the current node, try the parent. This
> +	 * fallback is needed if the current node belongs to an endpoint or
> +	 * switch upstream port.
> +	 */
> +	if (np->parent)
> +		perst = qcom_find_perst(pcie, np->parent);

Ugh.  I think we need to fix the data structures here before we go
much farther.  We should be able to search for PERST# once at probe of
the Qcom controller.  Hopefully we don't need lists of things.

See https://lore.kernel.org/r/20250908183325.GA1450728@bhelgaas.

> +toggle_perst:
> +	/* 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;
>  	int val = assert ? 1 : 0;
>  
> +	if (np)
> +		return qcom_toggle_perst_per_device(pcie, np, assert);
> +
>  	if (list_empty(&pcie->perst))
>  		gpiod_set_value_cansleep(pcie->reset, val);
>  
> @@ -310,22 +350,34 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
>  		gpiod_set_value_cansleep(perst->desc, val);
>  }
>  
> -static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> +static void qcom_ep_reset_assert(struct qcom_pcie *pcie, struct device_node *np)
>  {
> -	qcom_perst_assert(pcie, true);
> +	qcom_perst_reset(pcie, np, true);
>  	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
>  }
>  
> -static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> +static void qcom_ep_reset_deassert(struct qcom_pcie *pcie,
> +				   struct device_node *np)
>  {
>  	struct dw_pcie_rp *pp = &pcie->pci->pp;
>  
>  	msleep(PCIE_T_PVPERL_MS);
> -	qcom_perst_assert(pcie, false);
> +	qcom_perst_reset(pcie, np, false);
>  	if (!pp->use_linkup_irq)
>  		msleep(PCIE_RESET_CONFIG_WAIT_MS);
>  }
>  
> +static void qcom_pcie_toggle_perst(struct pci_host_bridge *bridge,
> +				    struct device_node *np, bool assert)
> +{
> +	struct qcom_pcie *pcie = dev_get_drvdata(bridge->dev.parent);
> +
> +	if (assert)
> +		qcom_ep_reset_assert(pcie, np);
> +	else
> +		qcom_ep_reset_deassert(pcie, np);
> +}
> +
>  static int qcom_pcie_start_link(struct dw_pcie *pci)
>  {
>  	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> @@ -1320,7 +1372,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)
> @@ -1336,7 +1388,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.

Can we replace "new binding" with something explicit?  In a few
months, "new binding" won't mean anything.

> +	 */
> +	if (list_empty(&pcie->perst))
> +		qcom_ep_reset_deassert(pcie, NULL);
>  
>  	if (pcie->cfg->ops->config_sid) {
>  		ret = pcie->cfg->ops->config_sid(pcie);
> @@ -1344,10 +1402,12 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>  			goto err_assert_reset;
>  	}
>  
> +	pci->pp.bridge->toggle_perst = qcom_pcie_toggle_perst;
> +
>  	return 0;
>  
>  err_assert_reset:
> -	qcom_ep_reset_assert(pcie);
> +	qcom_ep_reset_assert(pcie, NULL);
>  err_disable_phy:
>  	qcom_pcie_phy_power_off(pcie);
>  err_deinit:
> @@ -1361,7 +1421,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);
>  }
> @@ -1740,6 +1800,9 @@ static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
>  		return -ENOMEM;
>  
>  	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, &pcie->perst);
>  
>  parse_child_node:
> @@ -1803,8 +1866,10 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  		list_del(&port->list);
>  	}
>  
> -	list_for_each_entry_safe(perst, tmp_perst, &pcie->perst, list)
> +	list_for_each_entry_safe(perst, tmp_perst, &pcie->perst, list) {
> +		of_node_put(perst->np);
>  		list_del(&perst->list);
> +	}
>  
>  	return ret;
>  }
> @@ -2044,8 +2109,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	qcom_pcie_phy_exit(pcie);
>  	list_for_each_entry_safe(port, tmp_port, &pcie->ports, list)
>  		list_del(&port->list);
> -	list_for_each_entry_safe(perst, tmp_perst, &pcie->perst, list)
> +	list_for_each_entry_safe(perst, tmp_perst, &pcie->perst, list) {
> +		of_node_put(perst->np);
>  		list_del(&perst->list);
> +	}
>  err_pm_runtime_put:
>  	pm_runtime_put(dev);
>  	pm_runtime_disable(dev);
> 
> -- 
> 2.45.2
> 
>
Re: [PATCH v2 5/5] PCI: qcom: Allow pwrctrl core to toggle PERST# for new DT binding
Posted by Manivannan Sadhasivam 3 weeks ago
On Mon, Sep 08, 2025 at 02:34:28PM GMT, Bjorn Helgaas wrote:
> On Wed, Sep 03, 2025 at 12:43:27PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > If the platform is using the new DT binding, let the pwrctrl core toggle
> > PERST# for the device. This is achieved by populating the
> > 'pci_host_bridge::toggle_perst' callback with qcom_pcie_toggle_perst().
> 
> Can we say something here about how to identify a "new DT binding"?
> I assume there is a DT property or something that makes it "new"?

This is taken care now.

> 
> > qcom_pcie_toggle_perst() will find the PERST# GPIO descriptor associated
> > with the supplied 'device_node' and toggles PERST#. If PERST# is not found
> > in the supplied node, the function will look for PERST# in the parent node
> > as a fallback. This is needed since PERST# won't be available in the
> > endpoint node as per the DT binding.
> > 
> > Note that the driver still asserts PERST# during the controller
> > initialization as it is needed as per the hardware documentation. Apart
> > from that, the driver wouldn't touch PERST# for the new binding.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 89 +++++++++++++++++++++++++++++-----
> >  1 file changed, 78 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 78355d12f10d263a0bb052e24c1e2d5e8f68603d..3c5c65d7d97cac186e1b671f80ba7296ad226d68 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -276,6 +276,7 @@ struct qcom_pcie_port {
> >  struct qcom_pcie_perst {
> >  	struct list_head list;
> >  	struct gpio_desc *desc;
> > +	struct device_node *np;
> >  };
> >  
> >  struct qcom_pcie {
> > @@ -298,11 +299,50 @@ struct qcom_pcie {
> >  
> >  #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> >  
> > -static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> > +static struct gpio_desc *qcom_find_perst(struct qcom_pcie *pcie, struct device_node *np)
> > +{
> > +	struct qcom_pcie_perst *perst;
> > +
> > +	list_for_each_entry(perst, &pcie->perst, list) {
> > +		if (np == perst->np)
> > +			return perst->desc;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static void qcom_toggle_perst_per_device(struct qcom_pcie *pcie,
> > +					 struct device_node *np, bool assert)
> > +{
> > +	int val = assert ? 1 : 0;
> > +	struct gpio_desc *perst;
> > +
> > +	perst = qcom_find_perst(pcie, np);
> > +	if (perst)
> > +		goto toggle_perst;
> > +
> > +	/*
> > +	 * If PERST# is not available in the current node, try the parent. This
> > +	 * fallback is needed if the current node belongs to an endpoint or
> > +	 * switch upstream port.
> > +	 */
> > +	if (np->parent)
> > +		perst = qcom_find_perst(pcie, np->parent);
> 
> Ugh.  I think we need to fix the data structures here before we go
> much farther.  We should be able to search for PERST# once at probe of
> the Qcom controller.  Hopefully we don't need lists of things.
> 
> See https://lore.kernel.org/r/20250908183325.GA1450728@bhelgaas.
> 

I've added a patch to fix in the next version of this series.

> > +toggle_perst:
> > +	/* 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;
> >  	int val = assert ? 1 : 0;
> >  
> > +	if (np)
> > +		return qcom_toggle_perst_per_device(pcie, np, assert);
> > +
> >  	if (list_empty(&pcie->perst))
> >  		gpiod_set_value_cansleep(pcie->reset, val);
> >  
> > @@ -310,22 +350,34 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> >  		gpiod_set_value_cansleep(perst->desc, val);
> >  }
> >  
> > -static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> > +static void qcom_ep_reset_assert(struct qcom_pcie *pcie, struct device_node *np)
> >  {
> > -	qcom_perst_assert(pcie, true);
> > +	qcom_perst_reset(pcie, np, true);
> >  	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> >  }
> >  
> > -static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> > +static void qcom_ep_reset_deassert(struct qcom_pcie *pcie,
> > +				   struct device_node *np)
> >  {
> >  	struct dw_pcie_rp *pp = &pcie->pci->pp;
> >  
> >  	msleep(PCIE_T_PVPERL_MS);
> > -	qcom_perst_assert(pcie, false);
> > +	qcom_perst_reset(pcie, np, false);
> >  	if (!pp->use_linkup_irq)
> >  		msleep(PCIE_RESET_CONFIG_WAIT_MS);
> >  }
> >  
> > +static void qcom_pcie_toggle_perst(struct pci_host_bridge *bridge,
> > +				    struct device_node *np, bool assert)
> > +{
> > +	struct qcom_pcie *pcie = dev_get_drvdata(bridge->dev.parent);
> > +
> > +	if (assert)
> > +		qcom_ep_reset_assert(pcie, np);
> > +	else
> > +		qcom_ep_reset_deassert(pcie, np);
> > +}
> > +
> >  static int qcom_pcie_start_link(struct dw_pcie *pci)
> >  {
> >  	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> > @@ -1320,7 +1372,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)
> > @@ -1336,7 +1388,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.
> 
> Can we replace "new binding" with something explicit?  In a few
> months, "new binding" won't mean anything.
> 

So I've introduced a new flag, qcom_pcie::legacy_binding, which gets set if the
driver uses qcom_pcie_parse_legacy_binding(). Based on this flag, PERST# will be
deasserted in this driver.

And I've removed references to 'new binding' term.

- Mani

-- 
மணிவண்ணன் சதாசிவம்
Re: [PATCH v2 5/5] PCI: qcom: Allow pwrctrl core to toggle PERST# for new DT binding
Posted by kernel test robot 4 weeks, 1 day ago
Hi Manivannan,

kernel test robot noticed the following build errors:

[auto build test ERROR on 8f5ae30d69d7543eee0d70083daf4de8fe15d585]

url:    https://github.com/intel-lab-lkp/linux/commits/Manivannan-Sadhasivam-via-B4-Relay/PCI-qcom-Wait-for-PCIE_RESET_CONFIG_WAIT_MS-after-PERST-deassert/20250903-151623
base:   8f5ae30d69d7543eee0d70083daf4de8fe15d585
patch link:    https://lore.kernel.org/r/20250903-pci-pwrctrl-perst-v2-5-2d461ed0e061%40oss.qualcomm.com
patch subject: [PATCH v2 5/5] PCI: qcom: Allow pwrctrl core to toggle PERST# for new DT binding
config: i386-buildonly-randconfig-002-20250904 (https://download.01.org/0day-ci/archive/20250904/202509041110.4DgQKyf1-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.4.0-5) 12.4.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250904/202509041110.4DgQKyf1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509041110.4DgQKyf1-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/pci/controller/dwc/pcie-qcom.c: In function 'qcom_pcie_host_init':
>> drivers/pci/controller/dwc/pcie-qcom.c:1405:23: error: 'struct pci_host_bridge' has no member named 'toggle_perst'
    1405 |         pci->pp.bridge->toggle_perst = qcom_pcie_toggle_perst;
         |                       ^~


vim +1405 drivers/pci/controller/dwc/pcie-qcom.c

  1368	
  1369	static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
  1370	{
  1371		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
  1372		struct qcom_pcie *pcie = to_qcom_pcie(pci);
  1373		int ret;
  1374	
  1375		qcom_ep_reset_assert(pcie, NULL);
  1376	
  1377		ret = pcie->cfg->ops->init(pcie);
  1378		if (ret)
  1379			return ret;
  1380	
  1381		ret = qcom_pcie_phy_power_on(pcie);
  1382		if (ret)
  1383			goto err_deinit;
  1384	
  1385		if (pcie->cfg->ops->post_init) {
  1386			ret = pcie->cfg->ops->post_init(pcie);
  1387			if (ret)
  1388				goto err_disable_phy;
  1389		}
  1390	
  1391		/*
  1392		 * Only deassert PERST# for all devices here if legacy binding is used.
  1393		 * For the new binding, pwrctrl driver is expected to toggle PERST# for
  1394		 * individual devices.
  1395		 */
  1396		if (list_empty(&pcie->perst))
  1397			qcom_ep_reset_deassert(pcie, NULL);
  1398	
  1399		if (pcie->cfg->ops->config_sid) {
  1400			ret = pcie->cfg->ops->config_sid(pcie);
  1401			if (ret)
  1402				goto err_assert_reset;
  1403		}
  1404	
> 1405		pci->pp.bridge->toggle_perst = qcom_pcie_toggle_perst;
  1406	
  1407		return 0;
  1408	
  1409	err_assert_reset:
  1410		qcom_ep_reset_assert(pcie, NULL);
  1411	err_disable_phy:
  1412		qcom_pcie_phy_power_off(pcie);
  1413	err_deinit:
  1414		pcie->cfg->ops->deinit(pcie);
  1415	
  1416		return ret;
  1417	}
  1418	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v2 5/5] PCI: qcom: Allow pwrctrl core to toggle PERST# for new DT binding
Posted by Manivannan Sadhasivam 4 weeks ago
On Thu, Sep 04, 2025 at 11:19:30AM GMT, kernel test robot wrote:
> Hi Manivannan,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on 8f5ae30d69d7543eee0d70083daf4de8fe15d585]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Manivannan-Sadhasivam-via-B4-Relay/PCI-qcom-Wait-for-PCIE_RESET_CONFIG_WAIT_MS-after-PERST-deassert/20250903-151623
> base:   8f5ae30d69d7543eee0d70083daf4de8fe15d585
> patch link:    https://lore.kernel.org/r/20250903-pci-pwrctrl-perst-v2-5-2d461ed0e061%40oss.qualcomm.com
> patch subject: [PATCH v2 5/5] PCI: qcom: Allow pwrctrl core to toggle PERST# for new DT binding
> config: i386-buildonly-randconfig-002-20250904 (https://download.01.org/0day-ci/archive/20250904/202509041110.4DgQKyf1-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.4.0-5) 12.4.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250904/202509041110.4DgQKyf1-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202509041110.4DgQKyf1-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/pci/controller/dwc/pcie-qcom.c: In function 'qcom_pcie_host_init':
> >> drivers/pci/controller/dwc/pcie-qcom.c:1405:23: error: 'struct pci_host_bridge' has no member named 'toggle_perst'
>     1405 |         pci->pp.bridge->toggle_perst = qcom_pcie_toggle_perst;
>          |                       ^~
> 
> 

We can ignore this error since CONFIG_PWRCTRL_SLOT will be selected by default
with this commit: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/qcom&id=add7b05aeeb417c86239e6731a168e6c46b83279

- Mani

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