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

DT binding allows specifying 'phy' and 'reset' properties in both host
bridge and Root Port nodes, though specifying in the host bridge node is
marked as deprecated. Still, the pcie-qcom driver should support both
combinations for maintaining the DT backwards compatibility. For this
purpose, the driver is holding the relevant pointers of these properties in
two structs: struct qcom_pcie_port and struct qcom_pcie.

However, this causes confusion and increases the driver complexity. Hence,
move the pointers from struct qcom_pcie to struct qcom_pcie_port. As a
result, even if these properties are specified in the host bridge node,
the pointers will be stored in struct qcom_pcie_port as if the properties
are specified in a single Root Port node. This logic simplifies the driver
a lot.

Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 87 ++++++++++++++--------------------
 1 file changed, 36 insertions(+), 51 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..6170c86f465f43f980f5b2f88bd8799c3c152e68 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -279,8 +279,6 @@ struct qcom_pcie {
 	void __iomem *elbi;			/* DT elbi */
 	void __iomem *mhi;
 	union qcom_pcie_resources res;
-	struct phy *phy;
-	struct gpio_desc *reset;
 	struct icc_path *icc_mem;
 	struct icc_path *icc_cpu;
 	const struct qcom_pcie_cfg *cfg;
@@ -297,11 +295,8 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
 	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);
+	list_for_each_entry(port, &pcie->ports, list)
+		gpiod_set_value_cansleep(port->reset, val);
 
 	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
 }
@@ -1253,57 +1248,32 @@ 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);
-	}
+	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;
+	int ret;
 
-	if (list_empty(&pcie->ports)) {
-		ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
+	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(pcie->phy);
-		if (ret)
+		ret = phy_power_on(port->phy);
+		if (ret) {
+			qcom_pcie_phy_power_off(pcie);
 			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;
+	return 0;
 }
 
 static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
@@ -1748,8 +1718,10 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 	return ret;
 
 err_port_del:
-	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		phy_exit(port->phy);
 		list_del(&port->list);
+	}
 
 	return ret;
 }
@@ -1757,20 +1729,32 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
 {
 	struct device *dev = pcie->pci->dev;
+	struct qcom_pcie_port *port;
+	struct gpio_desc *reset;
+	struct phy *phy;
 	int ret;
 
-	pcie->phy = devm_phy_optional_get(dev, "pciephy");
-	if (IS_ERR(pcie->phy))
-		return PTR_ERR(pcie->phy);
+	phy = devm_phy_optional_get(dev, "pciephy");
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
 
-	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
-	if (IS_ERR(pcie->reset))
-		return PTR_ERR(pcie->reset);
+	reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset))
+		return PTR_ERR(reset);
 
-	ret = phy_init(pcie->phy);
+	ret = phy_init(phy);
 	if (ret)
 		return ret;
 
+	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;
 }
 
@@ -1984,9 +1968,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 err_host_deinit:
 	dw_pcie_host_deinit(pp);
 err_phy_exit:
-	qcom_pcie_phy_exit(pcie);
-	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		phy_exit(port->phy);
 		list_del(&port->list);
+	}
 err_pm_runtime_put:
 	pm_runtime_put(dev);
 	pm_runtime_disable(dev);

-- 
2.45.2

On Fri, Sep 12, 2025 at 02:05:02PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> DT binding allows specifying 'phy' and 'reset' properties in both host
> bridge and Root Port nodes, though specifying in the host bridge node is
> marked as deprecated. Still, the pcie-qcom driver should support both
> combinations for maintaining the DT backwards compatibility. For this
> purpose, the driver is holding the relevant pointers of these properties in
> two structs: struct qcom_pcie_port and struct qcom_pcie.
> 
> However, this causes confusion and increases the driver complexity. Hence,
> move the pointers from struct qcom_pcie to struct qcom_pcie_port. As a
> result, even if these properties are specified in the host bridge node,
> the pointers will be stored in struct qcom_pcie_port as if the properties
> are specified in a single Root Port node. This logic simplifies the driver
> a lot.
> 
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

Reviewed-by: Bjorn Helgaas <bhelgaas@google.com> 

I would put this patch by itself on pci/controller/qcom immediately
because it's not related to the rest of the series, and we should make
sure it's in v6.18 regardless of the rest.

> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 87 ++++++++++++++--------------------
>  1 file changed, 36 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..6170c86f465f43f980f5b2f88bd8799c3c152e68 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -279,8 +279,6 @@ struct qcom_pcie {
>  	void __iomem *elbi;			/* DT elbi */
>  	void __iomem *mhi;
>  	union qcom_pcie_resources res;
> -	struct phy *phy;
> -	struct gpio_desc *reset;
>  	struct icc_path *icc_mem;
>  	struct icc_path *icc_cpu;
>  	const struct qcom_pcie_cfg *cfg;
> @@ -297,11 +295,8 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
>  	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);
> +	list_for_each_entry(port, &pcie->ports, list)
> +		gpiod_set_value_cansleep(port->reset, val);
>  
>  	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
>  }
> @@ -1253,57 +1248,32 @@ 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);
> -	}
> +	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;
> +	int ret;
>  
> -	if (list_empty(&pcie->ports)) {
> -		ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> +	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(pcie->phy);
> -		if (ret)
> +		ret = phy_power_on(port->phy);
> +		if (ret) {
> +			qcom_pcie_phy_power_off(pcie);
>  			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;
> +	return 0;
>  }
>  
>  static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> @@ -1748,8 +1718,10 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  	return ret;
>  
>  err_port_del:
> -	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> +		phy_exit(port->phy);
>  		list_del(&port->list);
> +	}
>  
>  	return ret;
>  }
> @@ -1757,20 +1729,32 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
>  {
>  	struct device *dev = pcie->pci->dev;
> +	struct qcom_pcie_port *port;
> +	struct gpio_desc *reset;
> +	struct phy *phy;
>  	int ret;
>  
> -	pcie->phy = devm_phy_optional_get(dev, "pciephy");
> -	if (IS_ERR(pcie->phy))
> -		return PTR_ERR(pcie->phy);
> +	phy = devm_phy_optional_get(dev, "pciephy");
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);
>  
> -	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> -	if (IS_ERR(pcie->reset))
> -		return PTR_ERR(pcie->reset);
> +	reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset))
> +		return PTR_ERR(reset);
>  
> -	ret = phy_init(pcie->phy);
> +	ret = phy_init(phy);
>  	if (ret)
>  		return ret;
>  
> +	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;
>  }
>  
> @@ -1984,9 +1968,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  err_host_deinit:
>  	dw_pcie_host_deinit(pp);
>  err_phy_exit:
> -	qcom_pcie_phy_exit(pcie);
> -	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> +		phy_exit(port->phy);
>  		list_del(&port->list);
> +	}
>  err_pm_runtime_put:
>  	pm_runtime_put(dev);
>  	pm_runtime_disable(dev);
> 
> -- 
> 2.45.2
> 
>

On Tue, Sep 16, 2025 at 03:08:17PM GMT, Bjorn Helgaas wrote:
> On Fri, Sep 12, 2025 at 02:05:02PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> > DT binding allows specifying 'phy' and 'reset' properties in both host
> > bridge and Root Port nodes, though specifying in the host bridge node is
> > marked as deprecated. Still, the pcie-qcom driver should support both
> > combinations for maintaining the DT backwards compatibility. For this
> > purpose, the driver is holding the relevant pointers of these properties in
> > two structs: struct qcom_pcie_port and struct qcom_pcie.
> > 
> > However, this causes confusion and increases the driver complexity. Hence,
> > move the pointers from struct qcom_pcie to struct qcom_pcie_port. As a
> > result, even if these properties are specified in the host bridge node,
> > the pointers will be stored in struct qcom_pcie_port as if the properties
> > are specified in a single Root Port node. This logic simplifies the driver
> > a lot.
> > 
> > Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com> 
> 
> I would put this patch by itself on pci/controller/qcom immediately
> because it's not related to the rest of the series, and we should make
> sure it's in v6.18 regardless of the rest.
> 

Done!

- Mani

> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 87 ++++++++++++++--------------------
> >  1 file changed, 36 insertions(+), 51 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..6170c86f465f43f980f5b2f88bd8799c3c152e68 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -279,8 +279,6 @@ struct qcom_pcie {
> >  	void __iomem *elbi;			/* DT elbi */
> >  	void __iomem *mhi;
> >  	union qcom_pcie_resources res;
> > -	struct phy *phy;
> > -	struct gpio_desc *reset;
> >  	struct icc_path *icc_mem;
> >  	struct icc_path *icc_cpu;
> >  	const struct qcom_pcie_cfg *cfg;
> > @@ -297,11 +295,8 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> >  	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);
> > +	list_for_each_entry(port, &pcie->ports, list)
> > +		gpiod_set_value_cansleep(port->reset, val);
> >  
> >  	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> >  }
> > @@ -1253,57 +1248,32 @@ 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);
> > -	}
> > +	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;
> > +	int ret;
> >  
> > -	if (list_empty(&pcie->ports)) {
> > -		ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> > +	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(pcie->phy);
> > -		if (ret)
> > +		ret = phy_power_on(port->phy);
> > +		if (ret) {
> > +			qcom_pcie_phy_power_off(pcie);
> >  			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;
> > +	return 0;
> >  }
> >  
> >  static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> > @@ -1748,8 +1718,10 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
> >  	return ret;
> >  
> >  err_port_del:
> > -	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> > +		phy_exit(port->phy);
> >  		list_del(&port->list);
> > +	}
> >  
> >  	return ret;
> >  }
> > @@ -1757,20 +1729,32 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
> >  static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
> >  {
> >  	struct device *dev = pcie->pci->dev;
> > +	struct qcom_pcie_port *port;
> > +	struct gpio_desc *reset;
> > +	struct phy *phy;
> >  	int ret;
> >  
> > -	pcie->phy = devm_phy_optional_get(dev, "pciephy");
> > -	if (IS_ERR(pcie->phy))
> > -		return PTR_ERR(pcie->phy);
> > +	phy = devm_phy_optional_get(dev, "pciephy");
> > +	if (IS_ERR(phy))
> > +		return PTR_ERR(phy);
> >  
> > -	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> > -	if (IS_ERR(pcie->reset))
> > -		return PTR_ERR(pcie->reset);
> > +	reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(reset))
> > +		return PTR_ERR(reset);
> >  
> > -	ret = phy_init(pcie->phy);
> > +	ret = phy_init(phy);
> >  	if (ret)
> >  		return ret;
> >  
> > +	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;
> >  }
> >  
> > @@ -1984,9 +1968,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> >  err_host_deinit:
> >  	dw_pcie_host_deinit(pp);
> >  err_phy_exit:
> > -	qcom_pcie_phy_exit(pcie);
> > -	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> > +		phy_exit(port->phy);
> >  		list_del(&port->list);
> > +	}
> >  err_pm_runtime_put:
> >  	pm_runtime_put(dev);
> >  	pm_runtime_disable(dev);
> > 
> > -- 
> > 2.45.2
> > 
> > 

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

On Fri, Sep 12, 2025 at 02:05:02PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> DT binding allows specifying 'phy' and 'reset' properties in both host
> bridge and Root Port nodes, though specifying in the host bridge node is
> marked as deprecated. Still, the pcie-qcom driver should support both
> combinations for maintaining the DT backwards compatibility. For this
> purpose, the driver is holding the relevant pointers of these properties in
> two structs: struct qcom_pcie_port and struct qcom_pcie.
> 
> However, this causes confusion and increases the driver complexity. Hence,
> move the pointers from struct qcom_pcie to struct qcom_pcie_port. As a
> result, even if these properties are specified in the host bridge node,
> the pointers will be stored in struct qcom_pcie_port as if the properties
> are specified in a single Root Port node. This logic simplifies the driver
> a lot.

> @@ -297,11 +295,8 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
>  	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);
> +	list_for_each_entry(port, &pcie->ports, list)
> +		gpiod_set_value_cansleep(port->reset, val);

This is so much nicer, thanks for doing this!

>  static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
>  {
>  	struct device *dev = pcie->pci->dev;
> +	struct qcom_pcie_port *port;
> +	struct gpio_desc *reset;
> +	struct phy *phy;
>  	int ret;
>  
> -	pcie->phy = devm_phy_optional_get(dev, "pciephy");
> -	if (IS_ERR(pcie->phy))
> -		return PTR_ERR(pcie->phy);
> +	phy = devm_phy_optional_get(dev, "pciephy");
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);

Seems like it would be easier to integrate this fallback into
qcom_pcie_parse_port() instead if separating it into
qcom_pcie_parse_legacy_binding().

What if you did something like this in qcom_pcie_parse_port():

  qcom_pcie_parse_port
  {
      reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
				    "reset",  GPIOD_OUT_HIGH, "PERST#");
      if (IS_ERR(reset)) {
	  reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
	  if (IS_ERR(reset))
	      return PTR_ERR(reset);
      }
      ...

Then you could share all the port kzalloc and port list management.

Could do the same with the PHY stuff.

> -	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> -	if (IS_ERR(pcie->reset))
> -		return PTR_ERR(pcie->reset);
> +	reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset))
> +		return PTR_ERR(reset);
>  
> -	ret = phy_init(pcie->phy);
> +	ret = phy_init(phy);
>  	if (ret)
>  		return ret;
>  
> +	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;
>  }

On Fri, Sep 12, 2025 at 06:23:48PM GMT, Bjorn Helgaas wrote:
> On Fri, Sep 12, 2025 at 02:05:02PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> > DT binding allows specifying 'phy' and 'reset' properties in both host
> > bridge and Root Port nodes, though specifying in the host bridge node is
> > marked as deprecated. Still, the pcie-qcom driver should support both
> > combinations for maintaining the DT backwards compatibility. For this
> > purpose, the driver is holding the relevant pointers of these properties in
> > two structs: struct qcom_pcie_port and struct qcom_pcie.
> > 
> > However, this causes confusion and increases the driver complexity. Hence,
> > move the pointers from struct qcom_pcie to struct qcom_pcie_port. As a
> > result, even if these properties are specified in the host bridge node,
> > the pointers will be stored in struct qcom_pcie_port as if the properties
> > are specified in a single Root Port node. This logic simplifies the driver
> > a lot.
> 
> > @@ -297,11 +295,8 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> >  	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);
> > +	list_for_each_entry(port, &pcie->ports, list)
> > +		gpiod_set_value_cansleep(port->reset, val);
> 
> This is so much nicer, thanks for doing this!
> 
> >  static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
> >  {
> >  	struct device *dev = pcie->pci->dev;
> > +	struct qcom_pcie_port *port;
> > +	struct gpio_desc *reset;
> > +	struct phy *phy;
> >  	int ret;
> >  
> > -	pcie->phy = devm_phy_optional_get(dev, "pciephy");
> > -	if (IS_ERR(pcie->phy))
> > -		return PTR_ERR(pcie->phy);
> > +	phy = devm_phy_optional_get(dev, "pciephy");
> > +	if (IS_ERR(phy))
> > +		return PTR_ERR(phy);
> 
> Seems like it would be easier to integrate this fallback into
> qcom_pcie_parse_port() instead if separating it into
> qcom_pcie_parse_legacy_binding().
> 
> What if you did something like this in qcom_pcie_parse_port():
> 
>   qcom_pcie_parse_port
>   {
>       reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
> 				    "reset",  GPIOD_OUT_HIGH, "PERST#");
>       if (IS_ERR(reset)) {
> 	  reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> 	  if (IS_ERR(reset))
> 	      return PTR_ERR(reset);
>       }
>       ...
> 
> Then you could share all the port kzalloc and port list management.
> 
> Could do the same with the PHY stuff.
> 

Yeah, we could do something like this, but this will unnecessarily do a lookup
for 'perst-gpios' for every bridge node where there will only be 'reset-gpios'.
It is not a big deal though, since this code is only called in the probe path,
but I find the qcom_pcie_parse_legacy_binding() function to be visually
appealing as it separates legacy binding handling from the Root Port binding
and also makes it easy to drop the support for it in the future.

- Mani

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