Move phy, PERST# handling to root port and provide a way to have multi-port
logic.

Currently, QCOM controllers only support single port, and all properties
are present in the host bridge node itself. This is incorrect, as
properties like phys, perst-gpios, etc.. can vary per port and should be
present in the root port node.

To maintain DT backwards compatibility, fallback to the legacy method of
parsing the host bridge node if the port parsing fails.

pci-bus-common.yaml uses reset-gpios property for representing PERST#, use
same property instead of perst-gpios.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 178 ++++++++++++++++++++++++++++-----
 1 file changed, 151 insertions(+), 27 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index f7ed1e010eb6607b2e98a42f0051c47e4de2af93..56d04a15edf8f99f6d3b9bfaa037ff922b521888 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -267,6 +267,12 @@ struct qcom_pcie_cfg {
 	bool no_l0s;
 };
 
+struct qcom_pcie_port {
+	struct list_head list;
+	struct gpio_desc *reset;
+	struct phy *phy;
+};
+
 struct qcom_pcie {
 	struct dw_pcie *pci;
 	void __iomem *parf;			/* DT parf */
@@ -279,24 +285,37 @@ struct qcom_pcie {
 	struct icc_path *icc_cpu;
 	const struct qcom_pcie_cfg *cfg;
 	struct dentry *debugfs;
+	struct list_head ports;
 	bool suspended;
 	bool use_pm_opp;
 };
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
 
-static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
+static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
 {
-	gpiod_set_value_cansleep(pcie->reset, 1);
+	struct qcom_pcie_port *port;
+	int val = assert ? 1 : 0;
+
+	if (list_empty(&pcie->ports))
+		gpiod_set_value_cansleep(pcie->reset, val);
+	else
+		list_for_each_entry(port, &pcie->ports, list)
+			gpiod_set_value_cansleep(port->reset, val);
+
 	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
 }
 
+static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
+{
+	qcom_perst_assert(pcie, true);
+}
+
 static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
 {
 	/* Ensure that PERST has been asserted for at least 100 ms */
 	msleep(PCIE_T_PVPERL_MS);
-	gpiod_set_value_cansleep(pcie->reset, 0);
-	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
+	qcom_perst_assert(pcie, false);
 }
 
 static int qcom_pcie_start_link(struct dw_pcie *pci)
@@ -1234,6 +1253,59 @@ static bool qcom_pcie_link_up(struct dw_pcie *pci)
 	return val & PCI_EXP_LNKSTA_DLLLA;
 }
 
+static void qcom_pcie_phy_exit(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_port *port;
+
+	if (list_empty(&pcie->ports))
+		phy_exit(pcie->phy);
+	else
+		list_for_each_entry(port, &pcie->ports, list)
+			phy_exit(port->phy);
+}
+
+static void qcom_pcie_phy_power_off(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_port *port;
+
+	if (list_empty(&pcie->ports)) {
+		phy_power_off(pcie->phy);
+	} else {
+		list_for_each_entry(port, &pcie->ports, list)
+			phy_power_off(port->phy);
+	}
+}
+
+static int qcom_pcie_phy_power_on(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_port *port;
+	int ret = 0;
+
+	if (list_empty(&pcie->ports)) {
+		ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
+		if (ret)
+			return ret;
+
+		ret = phy_power_on(pcie->phy);
+		if (ret)
+			return ret;
+	} else {
+		list_for_each_entry(port, &pcie->ports, list) {
+			ret = phy_set_mode_ext(port->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
+			if (ret)
+				return ret;
+
+			ret = phy_power_on(port->phy);
+			if (ret) {
+				qcom_pcie_phy_power_off(pcie);
+				return ret;
+			}
+		}
+	}
+
+	return ret;
+}
+
 static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -1246,11 +1318,7 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 	if (ret)
 		return ret;
 
-	ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
-	if (ret)
-		goto err_deinit;
-
-	ret = phy_power_on(pcie->phy);
+	ret = qcom_pcie_phy_power_on(pcie);
 	if (ret)
 		goto err_deinit;
 
@@ -1273,7 +1341,7 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 err_assert_reset:
 	qcom_ep_reset_assert(pcie);
 err_disable_phy:
-	phy_power_off(pcie->phy);
+	qcom_pcie_phy_power_off(pcie);
 err_deinit:
 	pcie->cfg->ops->deinit(pcie);
 
@@ -1286,7 +1354,7 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 
 	qcom_ep_reset_assert(pcie);
-	phy_power_off(pcie->phy);
+	qcom_pcie_phy_power_off(pcie);
 	pcie->cfg->ops->deinit(pcie);
 }
 
@@ -1631,11 +1699,41 @@ 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 device *dev = pcie->pci->dev;
+	struct qcom_pcie_port *port;
+	struct gpio_desc *reset;
+	struct phy *phy;
+
+	reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
+				      "reset", GPIOD_OUT_HIGH, "PERST#");
+	if (IS_ERR(reset))
+		return PTR_ERR(reset);
+
+	phy = devm_of_phy_get(dev, node, NULL);
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
+
+	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	port->reset = reset;
+	port->phy = phy;
+	INIT_LIST_HEAD(&port->list);
+	list_add_tail(&port->list, &pcie->ports);
+
+	return 0;
+}
+
 static int qcom_pcie_probe(struct platform_device *pdev)
 {
 	const struct qcom_pcie_cfg *pcie_cfg;
 	unsigned long max_freq = ULONG_MAX;
+	struct qcom_pcie_port *port, *tmp;
 	struct device *dev = &pdev->dev;
+	struct device_node *of_port;
 	struct dev_pm_opp *opp;
 	struct qcom_pcie *pcie;
 	struct dw_pcie_rp *pp;
@@ -1701,6 +1799,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		goto err_pm_runtime_put;
 	}
 
+	INIT_LIST_HEAD(&pcie->ports);
+
 	pci->dev = dev;
 	pci->ops = &dw_pcie_ops;
 	pp = &pci->pp;
@@ -1709,12 +1809,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	pcie->cfg = pcie_cfg;
 
-	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
-	if (IS_ERR(pcie->reset)) {
-		ret = PTR_ERR(pcie->reset);
-		goto err_pm_runtime_put;
-	}
-
 	pcie->parf = devm_platform_ioremap_resource_byname(pdev, "parf");
 	if (IS_ERR(pcie->parf)) {
 		ret = PTR_ERR(pcie->parf);
@@ -1737,12 +1831,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		}
 	}
 
-	pcie->phy = devm_phy_optional_get(dev, "pciephy");
-	if (IS_ERR(pcie->phy)) {
-		ret = PTR_ERR(pcie->phy);
-		goto err_pm_runtime_put;
-	}
-
 	/* OPP table is optional */
 	ret = devm_pm_opp_of_add_table(dev);
 	if (ret && ret != -ENODEV) {
@@ -1789,9 +1877,42 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	pp->ops = &qcom_pcie_dw_ops;
 
-	ret = phy_init(pcie->phy);
-	if (ret)
-		goto err_pm_runtime_put;
+	for_each_available_child_of_node(dev->of_node, of_port) {
+		ret = qcom_pcie_parse_port(pcie, of_port);
+		of_node_put(of_port);
+		if (ret) {
+			if (ret != -ENOENT) {
+				dev_err_probe(pci->dev, ret,
+					      "Failed to parse port nodes %d\n",
+					      ret);
+				goto err_port_del;
+			}
+			break;
+		}
+	}
+
+	/*
+	 * In the case of properties not populated in root port, fallback to the
+	 * legacy method of parsing the host bridge node. This is to maintain DT
+	 * backwards compatibility.
+	 */
+	if (ret) {
+		pcie->phy = devm_phy_optional_get(dev, "pciephy");
+		if (IS_ERR(pcie->phy)) {
+			ret = PTR_ERR(pcie->phy);
+			goto err_pm_runtime_put;
+		}
+
+		pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
+		if (IS_ERR(pcie->reset)) {
+			ret = PTR_ERR(pcie->reset);
+			goto err_pm_runtime_put;
+		}
+
+		ret = phy_init(pcie->phy);
+		if (ret)
+			goto err_pm_runtime_put;
+	}
 
 	platform_set_drvdata(pdev, pcie);
 
@@ -1836,7 +1957,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 err_host_deinit:
 	dw_pcie_host_deinit(pp);
 err_phy_exit:
-	phy_exit(pcie->phy);
+	qcom_pcie_phy_exit(pcie);
+err_port_del:
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		list_del(&port->list);
 err_pm_runtime_put:
 	pm_runtime_put(dev);
 	pm_runtime_disable(dev);

-- 
2.34.1

On Wed, Jul 02, 2025 at 04:50:42PM +0530, Krishna Chaitanya Chundru wrote:
> Move phy, PERST# handling to root port and provide a way to have multi-port
> logic.
> 
> Currently, QCOM controllers only support single port, and all properties
> are present in the host bridge node itself. This is incorrect, as
> properties like phys, perst-gpios, etc.. can vary per port and should be
> present in the root port node.
> 
> To maintain DT backwards compatibility, fallback to the legacy method of
> parsing the host bridge node if the port parsing fails.
> 
> pci-bus-common.yaml uses reset-gpios property for representing PERST#, use
> same property instead of perst-gpios.
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 178 ++++++++++++++++++++++++++++-----
>  1 file changed, 151 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index f7ed1e010eb6607b2e98a42f0051c47e4de2af93..56d04a15edf8f99f6d3b9bfaa037ff922b521888 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -267,6 +267,12 @@ struct qcom_pcie_cfg {
>  	bool no_l0s;
>  };
>  
> +struct qcom_pcie_port {
> +	struct list_head list;
> +	struct gpio_desc *reset;
> +	struct phy *phy;

This change is already upstream (a2fbecdbbb9d ("PCI: qcom: Add support
for parsing the new Root Port binding")), but it seems wrong to me to
have "phy" and "reset" in both struct qcom_pcie and struct
qcom_pcie_port.  

I know we need *find* those things in different places (either a
per-Root Port DT stanza or the top-level qcom host bridge), but why
can't we always put them in struct qcom_pcie_port and drop them from
struct qcom_pcie?

Having them in both places means all the users need to worry about
that DT difference and look in both places instead of always looking
at qcom_pcie_port.

> +};
> +
>  struct qcom_pcie {
>  	struct dw_pcie *pci;
>  	void __iomem *parf;			/* DT parf */
> @@ -279,24 +285,37 @@ struct qcom_pcie {
>  	struct icc_path *icc_cpu;
>  	const struct qcom_pcie_cfg *cfg;
>  	struct dentry *debugfs;
> +	struct list_head ports;
>  	bool suspended;
>  	bool use_pm_opp;
>  };

> +static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
>  {
> -	gpiod_set_value_cansleep(pcie->reset, 1);
> +	struct qcom_pcie_port *port;
> +	int val = assert ? 1 : 0;
> +
> +	if (list_empty(&pcie->ports))
> +		gpiod_set_value_cansleep(pcie->reset, val);
> +	else
> +		list_for_each_entry(port, &pcie->ports, list)
> +			gpiod_set_value_cansleep(port->reset, val);

This is the kind of complication I think we should avoid.

> +static void qcom_pcie_phy_exit(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_port *port;
> +
> +	if (list_empty(&pcie->ports))
> +		phy_exit(pcie->phy);
> +	else
> +		list_for_each_entry(port, &pcie->ports, list)
> +			phy_exit(port->phy);

And this.

> +}
> +
> +static void qcom_pcie_phy_power_off(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_port *port;
> +
> +	if (list_empty(&pcie->ports)) {
> +		phy_power_off(pcie->phy);
> +	} else {
> +		list_for_each_entry(port, &pcie->ports, list)
> +			phy_power_off(port->phy);

And this.  And there's more.

Bjorn

On Wed, Jul 02, 2025 at 04:50:42PM GMT, Krishna Chaitanya Chundru wrote:

[...]

> -	ret = phy_init(pcie->phy);
> -	if (ret)
> -		goto err_pm_runtime_put;
> +	for_each_available_child_of_node(dev->of_node, of_port) {
> +		ret = qcom_pcie_parse_port(pcie, of_port);
> +		of_node_put(of_port);
> +		if (ret) {
> +			if (ret != -ENOENT) {
> +				dev_err_probe(pci->dev, ret,
> +					      "Failed to parse port nodes %d\n",
> +					      ret);
> +				goto err_port_del;
> +			}
> +			break;
> +		}
> +	}
> +
> +	/*
> +	 * In the case of properties not populated in root port, fallback to the
> +	 * legacy method of parsing the host bridge node. This is to maintain DT
> +	 * backwards compatibility.
> +	 */
> +	if (ret) {
> +		pcie->phy = devm_phy_optional_get(dev, "pciephy");
> +		if (IS_ERR(pcie->phy)) {
> +			ret = PTR_ERR(pcie->phy);
> +			goto err_pm_runtime_put;

Shouldn't this and below be err_port_del?

- Mani

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

On 7/2/2025 9:03 PM, Manivannan Sadhasivam wrote:
> On Wed, Jul 02, 2025 at 04:50:42PM GMT, Krishna Chaitanya Chundru wrote:
> 
> [...]
> 
>> -	ret = phy_init(pcie->phy);
>> -	if (ret)
>> -		goto err_pm_runtime_put;
>> +	for_each_available_child_of_node(dev->of_node, of_port) {
>> +		ret = qcom_pcie_parse_port(pcie, of_port);
>> +		of_node_put(of_port);
>> +		if (ret) {
>> +			if (ret != -ENOENT) {
>> +				dev_err_probe(pci->dev, ret,
>> +					      "Failed to parse port nodes %d\n",
>> +					      ret);
>> +				goto err_port_del;
>> +			}
>> +			break;
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * In the case of properties not populated in root port, fallback to the
>> +	 * legacy method of parsing the host bridge node. This is to maintain DT
>> +	 * backwards compatibility.
>> +	 */
>> +	if (ret) {
>> +		pcie->phy = devm_phy_optional_get(dev, "pciephy");
>> +		if (IS_ERR(pcie->phy)) {
>> +			ret = PTR_ERR(pcie->phy);
>> +			goto err_pm_runtime_put;
> 
> Shouldn't this and below be err_port_del?
> 
This is a legacy way of parsing property, if the execution
comes here means the port parsing has failed and ports are not created.
so err_port_del will not have any effect.

- Krishna Chaitanya.
> - Mani
>

On Thu, Jul 03, 2025 at 09:48:17AM GMT, Krishna Chaitanya Chundru wrote:
> 
> 
> On 7/2/2025 9:03 PM, Manivannan Sadhasivam wrote:
> > On Wed, Jul 02, 2025 at 04:50:42PM GMT, Krishna Chaitanya Chundru wrote:
> > 
> > [...]
> > 
> > > -	ret = phy_init(pcie->phy);
> > > -	if (ret)
> > > -		goto err_pm_runtime_put;
> > > +	for_each_available_child_of_node(dev->of_node, of_port) {
> > > +		ret = qcom_pcie_parse_port(pcie, of_port);
> > > +		of_node_put(of_port);
> > > +		if (ret) {
> > > +			if (ret != -ENOENT) {
> > > +				dev_err_probe(pci->dev, ret,
> > > +					      "Failed to parse port nodes %d\n",
> > > +					      ret);
> > > +				goto err_port_del;
> > > +			}
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	/*
> > > +	 * In the case of properties not populated in root port, fallback to the
> > > +	 * legacy method of parsing the host bridge node. This is to maintain DT
> > > +	 * backwards compatibility.
> > > +	 */
> > > +	if (ret) {
> > > +		pcie->phy = devm_phy_optional_get(dev, "pciephy");
> > > +		if (IS_ERR(pcie->phy)) {
> > > +			ret = PTR_ERR(pcie->phy);
> > > +			goto err_pm_runtime_put;
> > 
> > Shouldn't this and below be err_port_del?
> > 
> This is a legacy way of parsing property, if the execution
> comes here means the port parsing has failed and ports are not created.
> so err_port_del will not have any effect.
> 

Oops. I got confused by the if (ret) flow. It would be more clear if a goto is
used to indicate that the legacy codeblock is skipped. I'll just incorporate it
while applying.

- Mani

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