[PATCH v3 0/3] PCI: qcom: Move PERST# GPIO & phy retrieval from controller to PCIe bridge node

[PATCH v3 0/3] PCI: qcom: Move PERST# GPIO & phy retrieval from controller to PCIe bridge node

[Krishna Chaitanya Chundru]

The main intention of this series is to move wake# to the root port node.
After this series we will come up with a patch which registers for wake IRQ
from the pcieport driver. The wake IRQ is needed for the endpoint to wakeup
the host from D3cold. The driver change for wake IRQ is posted here[1].

There are many places we agreed to move the wake and perst gpio's
and phy etc to the pcie root port node instead of bridge node[2] as the
these properties are root port specific and does not belongs to
bridge node.

So move the phy, phy-names, wake-gpio's in the root port.
There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
start using that property instead of perst-gpio.

For backward compatibility, don't remove any existing properties in the
bridge node.

There are some other properties like num-lanes, max-link-speed which
needs to be moved to the root port nodes, but in this series we are
excluding them for now as this requires more changes in dwc layer and
can complicate the things.

Once this series gets merged all other platforms also will be updated
to use this new way.

Note:- The driver change needs to be merged first before dts changes.
Krzysztof Wilczyński or Mani can you provide the immutable branch with
these PCIe changes.

[1] https://lore.kernel.org/all/20250401-wake_irq_support-v1-0-d2e22f4a0efd@oss.qualcomm.com/ 
[2] https://lore.kernel.org/linux-pci/20241211192014.GA3302752@bhelgaas/

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
Changes in v3:
- Make old properties as deprecated, update commit message (Dmitry)
- Add helper functions wherever both multiport and legacy methods are used. (Mani)
- Link to v2: https://lore.kernel.org/r/20250414-perst-v2-0-89247746d755@oss.qualcomm.com

Changes in v2:
- Remove phy-names property and change the driver, dtsi accordingly (Rob)
- Link to v1: https://lore.kernel.org/r/20250322-perst-v1-0-e5e4da74a204@oss.qualcomm.com

---
Krishna Chaitanya Chundru (3):
      dt-bindings: PCI: qcom: Move phy, wake & reset gpio's to root port
      PCI: qcom: Add support for multi-root port
      arm64: qcom: sc7280: Move phy, perst to root port node

 .../devicetree/bindings/pci/qcom,pcie-common.yaml  |  36 ++++-
 .../devicetree/bindings/pci/qcom,pcie-sc7280.yaml  |  16 +-
 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts       |   5 +-
 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi     |   5 +-
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi           |   5 +-
 arch/arm64/boot/dts/qcom/sc7280.dtsi               |   6 +-
 drivers/pci/controller/dwc/pcie-qcom.c             | 169 +++++++++++++++++----
 7 files changed, 202 insertions(+), 40 deletions(-)
---
base-commit: cfb2e2c57aef75a414c0f18445c7441df5bc13be
change-id: 20250101-perst-cb885b5a6129

Best regards,
-- 
Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

[PATCH v3 1/3] dt-bindings: PCI: qcom: Move phy, wake & reset gpio's to root port

[Krishna Chaitanya Chundru]

Move the phy, phy-names, wake-gpio's to the pcie root port node instead of
the bridge node, as agreed upon in multiple places one instance is[1].

Update the qcom,pcie-common.yaml to include the phy, phy-names, and
wake-gpios properties in the root port node. There is already reset-gpios
defined for PERST# in pci-bus-common.yaml, start using that property
instead of perst-gpio.

For backward compatibility, do not remove any existing properties in the
bridge node.

[1] https://lore.kernel.org/linux-pci/20241211192014.GA3302752@bhelgaas/

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 .../devicetree/bindings/pci/qcom,pcie-common.yaml  | 36 ++++++++++++++++++++--
 .../devicetree/bindings/pci/qcom,pcie-sc7280.yaml  | 16 +++++++---
 2 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
index 0480c58f7d998adbac4c6de20cdaec945b3bab21..e5f60faa18ad68a29900a66fbfcba3d4f8e88e7b 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
@@ -51,10 +51,18 @@ properties:
 
   phys:
     maxItems: 1
+    deprecated: true
+    description:
+      This property is deprecated, instead of referencing this property from
+      the controller node, use the property from the PCIe root port node.
 
   phy-names:
     items:
       - const: pciephy
+    deprecated: true
+    description:
+      Phandle to the register map node. This property is deprecated, and not
+      required to add in the root port also, as the root port has only one phy.
 
   power-domains:
     maxItems: 1
@@ -71,12 +79,18 @@ properties:
     maxItems: 12
 
   perst-gpios:
-    description: GPIO controlled connection to PERST# signal
+    description: GPIO controlled connection to PERST# signal. This property is
+      deprecated, instead of referencing this property from the controller node,
+      use the reset-gpios property from the root port node.
     maxItems: 1
+    deprecated: true
 
   wake-gpios:
-    description: GPIO controlled connection to WAKE# signal
+    description: GPIO controlled connection to WAKE# signal. This property is
+      deprecated, instead of referencing this property from the controller node,
+      use the property from the PCIe root port node.
     maxItems: 1
+    deprecated: true
 
   vddpe-3v3-supply:
     description: PCIe endpoint power supply
@@ -85,6 +99,24 @@ properties:
   opp-table:
     type: object
 
+patternProperties:
+  "^pcie@":
+    type: object
+    $ref: /schemas/pci/pci-pci-bridge.yaml#
+
+    properties:
+      reg:
+        maxItems: 1
+
+      phys:
+        maxItems: 1
+
+      wake-gpios:
+        description: GPIO controlled connection to WAKE# signal
+        maxItems: 1
+
+    unevaluatedProperties: false
+
 required:
   - reg
   - reg-names
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml
index 76cb9fbfd476fb0412217c68bd8db44a51c7d236..eb70cc6b6618af43fb03e124db20e2ade26a95ae 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml
@@ -162,9 +162,6 @@ examples:
             iommu-map = <0x0 &apps_smmu 0x1c80 0x1>,
                         <0x100 &apps_smmu 0x1c81 0x1>;
 
-            phys = <&pcie1_phy>;
-            phy-names = "pciephy";
-
             pinctrl-names = "default";
             pinctrl-0 = <&pcie1_clkreq_n>;
 
@@ -173,7 +170,18 @@ examples:
             resets = <&gcc GCC_PCIE_1_BCR>;
             reset-names = "pci";
 
-            perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
             vddpe-3v3-supply = <&pp3300_ssd>;
+            pcie1_port0: pcie@0 {
+                device_type = "pci";
+                reg = <0x0 0x0 0x0 0x0 0x0>;
+                bus-range = <0x01 0xff>;
+
+                #address-cells = <3>;
+                #size-cells = <2>;
+                ranges;
+                phys = <&pcie1_phy>;
+
+                reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
+            };
         };
     };

-- 
2.34.1

[PATCH v3 2/3] PCI: qcom: Add support for multi-root port

[Krishna Chaitanya Chundru]

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 controller node itself. This is incorrect, as
properties like phy, perst, wake, 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 controller 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 | 169 +++++++++++++++++++++++++++------
 1 file changed, 142 insertions(+), 27 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index dc98ae63362db0422384b1879a2b9a7dc564d091..e97e5076f5f77acbbdfb982af7acc69daf9bf307 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -262,6 +262,11 @@ 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 */
@@ -276,22 +281,35 @@ struct qcom_pcie {
 	struct dentry *debugfs;
 	bool suspended;
 	bool use_pm_opp;
+	struct list_head ports;
 };
 
 #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_deassert(struct qcom_pcie *pcie, bool assert)
 {
-	gpiod_set_value_cansleep(pcie->reset, 1);
+	struct qcom_pcie_port *port, *tmp;
+	int val = assert ? 1 : 0;
+
+	if (list_empty(&pcie->ports))
+		gpiod_set_value_cansleep(pcie->reset, val);
+	else
+		list_for_each_entry_safe(port, tmp, &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_deassert(pcie, true);
+}
+
 static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
 {
 	/* Ensure that PERST has been asserted for at least 100 ms */
 	msleep(100);
-	gpiod_set_value_cansleep(pcie->reset, 0);
-	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
+	qcom_perst_assert_deassert(pcie, false);
 }
 
 static int qcom_pcie_start_link(struct dw_pcie *pci)
@@ -1229,6 +1247,59 @@ static int 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, *tmp;
+
+	if (list_empty(&pcie->ports))
+		phy_exit(pcie->phy);
+	else
+		list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+			phy_exit(port->phy);
+}
+
+static void qcom_pcie_phy_off(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_port *port, *tmp;
+
+	if (list_empty(&pcie->ports)) {
+		phy_power_off(pcie->phy);
+	} else {
+		list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+			phy_power_off(port->phy);
+	}
+}
+
+static int qcom_pcie_phy_power_on(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_port *port, *tmp;
+	int ret = 0;
+
+	if (list_empty(&pcie->ports)) {
+		ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
+		if (ret)
+			goto out;
+
+		ret = phy_power_on(pcie->phy);
+		if (ret)
+			goto out;
+	} else {
+		list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+			ret = phy_set_mode_ext(port->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
+			if (ret)
+				goto out;
+
+			ret = phy_power_on(port->phy);
+			if (ret) {
+				qcom_pcie_phy_off(pcie);
+				goto out;
+			}
+		}
+	}
+out:
+	return ret;
+}
+
 static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -1241,11 +1312,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;
 
@@ -1268,7 +1335,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_off(pcie);
 err_deinit:
 	pcie->cfg->ops->deinit(pcie);
 
@@ -1281,7 +1348,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_off(pcie);
 	pcie->cfg->ops->deinit(pcie);
 }
 
@@ -1579,11 +1646,41 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+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;
@@ -1611,6 +1708,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_pm_runtime_put;
 
+	INIT_LIST_HEAD(&pcie->ports);
+
 	pci->dev = dev;
 	pci->ops = &dw_pcie_ops;
 	pp = &pci->pp;
@@ -1619,12 +1718,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);
@@ -1647,12 +1740,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) {
@@ -1699,9 +1786,35 @@ 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)
+			break;
+	}
+
+	/*
+	 * In the case of failure in parsing the port nodes, fallback to the
+	 * legacy method of parsing the controller 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);
 
@@ -1746,10 +1859,12 @@ 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_pm_runtime_put:
 	pm_runtime_put(dev);
 	pm_runtime_disable(dev);
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		list_del(&port->list);
 
 	return ret;
 }

-- 
2.34.1

[PATCH v3 3/3] arm64: qcom: sc7280: Move phy, perst to root port node

[Krishna Chaitanya Chundru]

There are many places we agreed to move the wake and perst gpio's
and phy etc to the pcie root port node instead of bridge node[1].

So move the phy, phy-names, wake-gpio's in the root port.
There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
start using that property instead of perst-gpio.

[1] https://lore.kernel.org/linux-pci/20241211192014.GA3302752@bhelgaas/

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts   | 5 ++++-
 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++-
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 5 ++++-
 arch/arm64/boot/dts/qcom/sc7280.dtsi           | 6 ++----
 4 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
index 7a36c90ad4ec8b52f30b22b1621404857d6ef336..3dd58986ad5da0f898537a51715bb5d0fecbe100 100644
--- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
+++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
@@ -709,8 +709,11 @@ &mdss_edp_phy {
 	status = "okay";
 };
 
+&pcie1_port0 {
+	reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
+};
+
 &pcie1 {
-	perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
 
 	pinctrl-0 = <&pcie1_reset_n>, <&pcie1_wake_n>;
 	pinctrl-names = "default";
diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index 2ba4ea60cb14736c9cfbf9f4a9048f20a4c921f2..ff11d85d015bdab6a90bd8a0eb9113a339866953 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -472,10 +472,13 @@ &pcie1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie1_clkreq_n>, <&ssd_rst_l>, <&pe_wake_odl>;
 
-	perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
 	vddpe-3v3-supply = <&pp3300_ssd>;
 };
 
+&pcie1_port0 {
+	reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
+};
+
 &pm8350c_pwm {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 7370aa0dbf0e3f9e7a3e38c3f00686e1d3dcbc9f..3209bb15dfec36299cabae07d34f3dc82db6de77 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -414,9 +414,12 @@ &lpass_va_macro {
 	vdd-micb-supply = <&vreg_bob>;
 };
 
+&pcie1_port0 {
+	reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
+};
+
 &pcie1 {
 	status = "okay";
-	perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
 
 	vddpe-3v3-supply = <&nvme_3v3_regulator>;
 
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 0f2caf36910b65c398c9e03800a8ce0a8a1f8fc7..376fabf3b4eac34d75bb79ef902c9d83490c45f7 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2271,9 +2271,6 @@ pcie1: pcie@1c08000 {
 
 			power-domains = <&gcc GCC_PCIE_1_GDSC>;
 
-			phys = <&pcie1_phy>;
-			phy-names = "pciephy";
-
 			pinctrl-names = "default";
 			pinctrl-0 = <&pcie1_clkreq_n>;
 
@@ -2284,7 +2281,7 @@ pcie1: pcie@1c08000 {
 
 			status = "disabled";
 
-			pcie@0 {
+			pcie1_port0: pcie@0 {
 				device_type = "pci";
 				reg = <0x0 0x0 0x0 0x0 0x0>;
 				bus-range = <0x01 0xff>;
@@ -2292,6 +2289,7 @@ pcie@0 {
 				#address-cells = <3>;
 				#size-cells = <2>;
 				ranges;
+				phys = <&pcie1_phy>;
 			};
 		};
 

-- 
2.34.1

Re: [PATCH v3 2/3] PCI: qcom: Add support for multi-root port

[Konrad Dybcio]

On 4/19/25 7:19 AM, 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 controller node itself. This is incorrect, as
> properties like phy, perst, wake, 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 controller 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>
> ---

[...]

> -static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> +static void qcom_perst_assert_deassert(struct qcom_pcie *pcie, bool assert)
>  {
> -	gpiod_set_value_cansleep(pcie->reset, 1);
> +	struct qcom_pcie_port *port, *tmp;
> +	int val = assert ? 1 : 0;

assert is already a boolean - are some checkers complaining?

[...]

> +	/*
> +	 * In the case of failure in parsing the port nodes, fallback to the
> +	 * legacy method of parsing the controller node. This is to maintain DT
> +	 * backwards compatibility.

It'd be simpler to call qcom_pcie_parse_port on the PCIe controller's
OF node, removing the need for the if-else-s throughout the patch

Konrad

Re: [PATCH v3 2/3] PCI: qcom: Add support for multi-root port

[Krishna Chaitanya Chundru]

On 4/23/2025 2:15 AM, Konrad Dybcio wrote:
> On 4/19/25 7:19 AM, 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 controller node itself. This is incorrect, as
>> properties like phy, perst, wake, 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 controller 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>
>> ---
> 
> [...]
> 
>> -static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
>> +static void qcom_perst_assert_deassert(struct qcom_pcie *pcie, bool assert)
>>   {
>> -	gpiod_set_value_cansleep(pcie->reset, 1);
>> +	struct qcom_pcie_port *port, *tmp;
>> +	int val = assert ? 1 : 0;
> 
> assert is already a boolean - are some checkers complaining?
Ack, I will remove this in next patch.
> 
> [...]
> 
>> +	/*
>> +	 * In the case of failure in parsing the port nodes, fallback to the
>> +	 * legacy method of parsing the controller node. This is to maintain DT
>> +	 * backwards compatibility.
> 
> It'd be simpler to call qcom_pcie_parse_port on the PCIe controller's
> OF node, removing the need for the if-else-s throughout the patch
> 
There is difference in perst property name for controller's OF node and
the root port OF node. controller use perst-gpios, where as the root
port node uses the pci-bus-common.yaml defined way of perst i.e
reset-gpios.

It's better to have this way then having if else condition in the
qcom_pcie_parse_port.

- Krishna Chaitanya.
> Konrad
> 
> 

Re: [PATCH v3 3/3] arm64: qcom: sc7280: Move phy, perst to root port node

[Rob Herring]

On Sat, Apr 19, 2025 at 10:49:26AM +0530, Krishna Chaitanya Chundru wrote:
> There are many places we agreed to move the wake and perst gpio's
> and phy etc to the pcie root port node instead of bridge node[1].
> 
> So move the phy, phy-names, wake-gpio's in the root port.
> There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
> start using that property instead of perst-gpio.

Moving the properties will break existing kernels. If that doesn't 
matter for these platforms, say so in the commit msg.

> 
> [1] https://lore.kernel.org/linux-pci/20241211192014.GA3302752@bhelgaas/
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts   | 5 ++++-
>  arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++-
>  arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 5 ++++-
>  arch/arm64/boot/dts/qcom/sc7280.dtsi           | 6 ++----
>  4 files changed, 14 insertions(+), 7 deletions(-)

Re: [PATCH v3 1/3] dt-bindings: PCI: qcom: Move phy, wake & reset gpio's to root port

[Rob Herring (Arm)]

On Sat, 19 Apr 2025 10:49:24 +0530, Krishna Chaitanya Chundru wrote:
> Move the phy, phy-names, wake-gpio's to the pcie root port node instead of
> the bridge node, as agreed upon in multiple places one instance is[1].
> 
> Update the qcom,pcie-common.yaml to include the phy, phy-names, and
> wake-gpios properties in the root port node. There is already reset-gpios
> defined for PERST# in pci-bus-common.yaml, start using that property
> instead of perst-gpio.
> 
> For backward compatibility, do not remove any existing properties in the
> bridge node.
> 
> [1] https://lore.kernel.org/linux-pci/20241211192014.GA3302752@bhelgaas/
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  .../devicetree/bindings/pci/qcom,pcie-common.yaml  | 36 ++++++++++++++++++++--
>  .../devicetree/bindings/pci/qcom,pcie-sc7280.yaml  | 16 +++++++---
>  2 files changed, 46 insertions(+), 6 deletions(-)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v3 3/3] arm64: qcom: sc7280: Move phy, perst to root port node

[Konrad Dybcio]

On 4/23/25 5:37 PM, Rob Herring wrote:
> On Sat, Apr 19, 2025 at 10:49:26AM +0530, Krishna Chaitanya Chundru wrote:
>> There are many places we agreed to move the wake and perst gpio's
>> and phy etc to the pcie root port node instead of bridge node[1].
>>
>> So move the phy, phy-names, wake-gpio's in the root port.
>> There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
>> start using that property instead of perst-gpio.
> 
> Moving the properties will break existing kernels. If that doesn't 
> matter for these platforms, say so in the commit msg.

I don't think we generally guarantee *forward* dt compatibility though, no?

Konrad

Re: [PATCH v3 1/3] dt-bindings: PCI: qcom: Move phy, wake & reset gpio's to root port

[Manivannan Sadhasivam]

On Sat, Apr 19, 2025 at 10:49:24AM +0530, Krishna Chaitanya Chundru wrote:
> Move the phy, phy-names, wake-gpio's to the pcie root port node instead of
> the bridge node, as agreed upon in multiple places one instance is[1].

s/instead of the bridge node/from host bridge node/g

> 
> Update the qcom,pcie-common.yaml to include the phy, phy-names, and
> wake-gpios properties in the root port node. There is already reset-gpios
> defined for PERST# in pci-bus-common.yaml, start using that property
> instead of perst-gpio.
> 
> For backward compatibility, do not remove any existing properties in the
> bridge node.

... Hence mark them as 'deprecated'.

> 
> [1] https://lore.kernel.org/linux-pci/20241211192014.GA3302752@bhelgaas/
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  .../devicetree/bindings/pci/qcom,pcie-common.yaml  | 36 ++++++++++++++++++++--
>  .../devicetree/bindings/pci/qcom,pcie-sc7280.yaml  | 16 +++++++---
>  2 files changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
> index 0480c58f7d998adbac4c6de20cdaec945b3bab21..e5f60faa18ad68a29900a66fbfcba3d4f8e88e7b 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
> @@ -51,10 +51,18 @@ properties:
>  
>    phys:
>      maxItems: 1
> +    deprecated: true
> +    description:
> +      This property is deprecated, instead of referencing this property from
> +      the controller node, use the property from the PCIe root port node.

s/controller/host bridge

Here and below.

>  
>    phy-names:
>      items:
>        - const: pciephy
> +    deprecated: true
> +    description:
> +      Phandle to the register map node. This property is deprecated, and not
> +      required to add in the root port also, as the root port has only one phy.
>  
>    power-domains:
>      maxItems: 1
> @@ -71,12 +79,18 @@ properties:
>      maxItems: 12
>  
>    perst-gpios:
> -    description: GPIO controlled connection to PERST# signal
> +    description: GPIO controlled connection to PERST# signal. This property is
> +      deprecated, instead of referencing this property from the controller node,
> +      use the reset-gpios property from the root port node.
>      maxItems: 1
> +    deprecated: true
>  
>    wake-gpios:
> -    description: GPIO controlled connection to WAKE# signal
> +    description: GPIO controlled connection to WAKE# signal. This property is
> +      deprecated, instead of referencing this property from the controller node,
> +      use the property from the PCIe root port node.
>      maxItems: 1
> +    deprecated: true
>  
>    vddpe-3v3-supply:
>      description: PCIe endpoint power supply
> @@ -85,6 +99,24 @@ properties:
>    opp-table:
>      type: object
>  
> +patternProperties:
> +  "^pcie@":
> +    type: object
> +    $ref: /schemas/pci/pci-pci-bridge.yaml#
> +
> +    properties:
> +      reg:
> +        maxItems: 1
> +
> +      phys:
> +        maxItems: 1
> +
> +      wake-gpios:
> +        description: GPIO controlled connection to WAKE# signal
> +        maxItems: 1

Shouldn't 'wake-gpios' be part of the pci-bus-common.yaml?

- Mani

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

Re: [PATCH v3 2/3] PCI: qcom: Add support for multi-root port

[Manivannan Sadhasivam]

On Sat, Apr 19, 2025 at 10:49:25AM +0530, Krishna Chaitanya Chundru wrote:
> Move phy, perst handling to root port and provide a way to have multi-port

s/perst/PERST#

> logic.
> 
> Currently, qcom controllers only support single port, and all properties

s/qcom/Qcom

> are present in the controller node itself. This is incorrect, as
> properties like phy, perst, wake, etc. can vary per port and should be

If you are referring to properties, specify them as it is. Like, phys,
perst-gpios, etc...

> present in the root port node.
> 
> To maintain DT backwards compatibility, fallback to the legacy method of
> parsing the controller node if the port parsing fails.
> 
> pci-bus-common.yaml uses reset-gpios property for representing PERST, use

s/PERST/PERST#

> same property instead of perst-gpios.
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 169 +++++++++++++++++++++++++++------
>  1 file changed, 142 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index dc98ae63362db0422384b1879a2b9a7dc564d091..e97e5076f5f77acbbdfb982af7acc69daf9bf307 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -262,6 +262,11 @@ struct qcom_pcie_cfg {
>  	bool no_l0s;
>  };
>  
> +struct qcom_pcie_port {
> +	struct list_head list;
> +	struct gpio_desc *reset;
> +	struct phy *phy;
> +};

Insert a newline.

>  struct qcom_pcie {
>  	struct dw_pcie *pci;
>  	void __iomem *parf;			/* DT parf */
> @@ -276,22 +281,35 @@ struct qcom_pcie {
>  	struct dentry *debugfs;
>  	bool suspended;
>  	bool use_pm_opp;
> +	struct list_head ports;

Move it above the boolean.

>  };
>  
>  #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_deassert(struct qcom_pcie *pcie, bool assert)

qcom_perst_assert(..., bool assert)

>  {
> -	gpiod_set_value_cansleep(pcie->reset, 1);
> +	struct qcom_pcie_port *port, *tmp;
> +	int val = assert ? 1 : 0;
> +
> +	if (list_empty(&pcie->ports))
> +		gpiod_set_value_cansleep(pcie->reset, val);
> +	else
> +		list_for_each_entry_safe(port, tmp, &pcie->ports, list)

_safe loop should be used only when the entry is removed.

> +			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_deassert(pcie, true);
> +}
> +
>  static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
>  {
>  	/* Ensure that PERST has been asserted for at least 100 ms */
>  	msleep(100);
> -	gpiod_set_value_cansleep(pcie->reset, 0);
> -	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> +	qcom_perst_assert_deassert(pcie, false);
>  }
>  
>  static int qcom_pcie_start_link(struct dw_pcie *pci)
> @@ -1229,6 +1247,59 @@ static int 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, *tmp;
> +
> +	if (list_empty(&pcie->ports))
> +		phy_exit(pcie->phy);
> +	else
> +		list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +			phy_exit(port->phy);
> +}
> +
> +static void qcom_pcie_phy_off(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_port *port, *tmp;
> +
> +	if (list_empty(&pcie->ports)) {
> +		phy_power_off(pcie->phy);
> +	} else {
> +		list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +			phy_power_off(port->phy);
> +	}
> +}
> +
> +static int qcom_pcie_phy_power_on(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_port *port, *tmp;
> +	int ret = 0;
> +
> +	if (list_empty(&pcie->ports)) {
> +		ret = phy_set_mode_ext(pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> +		if (ret)
> +			goto out;
> +
> +		ret = phy_power_on(pcie->phy);
> +		if (ret)
> +			goto out;
> +	} else {
> +		list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> +			ret = phy_set_mode_ext(port->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> +			if (ret)
> +				goto out;
> +
> +			ret = phy_power_on(port->phy);
> +			if (ret) {
> +				qcom_pcie_phy_off(pcie);
> +				goto out;
> +			}
> +		}
> +	}
> +out:

I would prefer returning directly above instead of the err label.

> +	return ret;
> +}
> +
>  static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> @@ -1241,11 +1312,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;
>  
> @@ -1268,7 +1335,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_off(pcie);

Use 'qcom_pcie_phy_power_off' for consistency.

>  err_deinit:
>  	pcie->cfg->ops->deinit(pcie);
>  
> @@ -1281,7 +1348,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_off(pcie);
>  	pcie->cfg->ops->deinit(pcie);
>  }
>  
> @@ -1579,11 +1646,41 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +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;
> @@ -1611,6 +1708,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err_pm_runtime_put;
>  
> +	INIT_LIST_HEAD(&pcie->ports);
> +
>  	pci->dev = dev;
>  	pci->ops = &dw_pcie_ops;
>  	pp = &pci->pp;
> @@ -1619,12 +1718,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);
> @@ -1647,12 +1740,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) {
> @@ -1699,9 +1786,35 @@ 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)

We should only skip the loop if ENOENT is returned. Any other errors should
be treated as a hard error and probe should fail.

> +			break;
> +	}
> +
> +	/*
> +	 * In the case of failure in parsing the port nodes, fallback to the

Driver cannot fallback to the legacy binding if the root port node parsing
fails. It can only do so if root the port node properties are not populated.

> +	 * legacy method of parsing the controller node. This is to maintain DT
> +	 * backwards compatibility.
> +	 */
> +	if (ret) {

Add a warning to let users/developers switch to new binding:

	dev_warn(dev, "Using legacy device tree binding\n");

- Mani

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

Re: [PATCH v3 3/3] arm64: qcom: sc7280: Move phy, perst to root port node

[Manivannan Sadhasivam]

On Sat, Apr 19, 2025 at 10:49:26AM +0530, Krishna Chaitanya Chundru wrote:
> There are many places we agreed to move the wake and perst gpio's
> and phy etc to the pcie root port node instead of bridge node[1].

Same comment as binding patch applies here.

> 
> So move the phy, phy-names, wake-gpio's in the root port.

You are not moving any 'wake-gpios' property.

> There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
> start using that property instead of perst-gpio.
> 
> [1] https://lore.kernel.org/linux-pci/20241211192014.GA3302752@bhelgaas/
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts   | 5 ++++-
>  arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++-
>  arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 5 ++++-
>  arch/arm64/boot/dts/qcom/sc7280.dtsi           | 6 ++----
>  4 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> index 7a36c90ad4ec8b52f30b22b1621404857d6ef336..3dd58986ad5da0f898537a51715bb5d0fecbe100 100644
> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> @@ -709,8 +709,11 @@ &mdss_edp_phy {
>  	status = "okay";
>  };
>  
> +&pcie1_port0 {
> +	reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
> +};
> +
>  &pcie1 {
> -	perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
>  
>  	pinctrl-0 = <&pcie1_reset_n>, <&pcie1_wake_n>;
>  	pinctrl-names = "default";

What about the pinctrl properties? They should also be moved.

- Mani

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

Re: [PATCH v3 0/3] PCI: qcom: Move PERST# GPIO & phy retrieval from controller to PCIe bridge node

[Manivannan Sadhasivam]

On Sat, Apr 19, 2025 at 10:49:23AM +0530, Krishna Chaitanya Chundru wrote:
> The main intention of this series is to move wake# to the root port node.
> After this series we will come up with a patch which registers for wake IRQ
> from the pcieport driver. The wake IRQ is needed for the endpoint to wakeup
> the host from D3cold. The driver change for wake IRQ is posted here[1].
> 
> There are many places we agreed to move the wake and perst gpio's
> and phy etc to the pcie root port node instead of bridge node[2] as the
> these properties are root port specific and does not belongs to
> bridge node.
> 
> So move the phy, phy-names, wake-gpio's in the root port.
> There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
> start using that property instead of perst-gpio.
> 
> For backward compatibility, don't remove any existing properties in the
> bridge node.
> 
> There are some other properties like num-lanes, max-link-speed which
> needs to be moved to the root port nodes, but in this series we are
> excluding them for now as this requires more changes in dwc layer and
> can complicate the things.
> 
> Once this series gets merged all other platforms also will be updated
> to use this new way.
> 
> Note:- The driver change needs to be merged first before dts changes.
> Krzysztof Wilczyński or Mani can you provide the immutable branch with
> these PCIe changes.
> 

Since there could be other patches for Qcom driver in the PCI tree, I don't
prefer immutable branch. Let's first merge the driver and binding patches
through PCI tree and you can submit the dts changes for rest of the platforms
for the next cycle.

- Mani

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

Re: [PATCH v3 3/3] arm64: qcom: sc7280: Move phy, perst to root port node

[Krzysztof Kozlowski]

On 08/05/2025 16:26, Konrad Dybcio wrote:
> On 4/23/25 5:37 PM, Rob Herring wrote:
>> On Sat, Apr 19, 2025 at 10:49:26AM +0530, Krishna Chaitanya Chundru wrote:
>>> There are many places we agreed to move the wake and perst gpio's
>>> and phy etc to the pcie root port node instead of bridge node[1].
>>>
>>> So move the phy, phy-names, wake-gpio's in the root port.
>>> There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
>>> start using that property instead of perst-gpio.
>>
>> Moving the properties will break existing kernels. If that doesn't 
>> matter for these platforms, say so in the commit msg.
> 
> I don't think we generally guarantee *forward* dt compatibility though, no?
We do not guarantee, comment was not about this, but we expect. This DTS
is supposed and is used by other projects. There was entire complain
last DT BoF about kernel breaking DTS users all the time.

Best regards,
Krzysztof

Re: [PATCH v3 3/3] arm64: qcom: sc7280: Move phy, perst to root port node

[Krishna Chaitanya Chundru]

On 6/1/2025 12:35 PM, Manivannan Sadhasivam wrote:
> On Sat, Apr 19, 2025 at 10:49:26AM +0530, Krishna Chaitanya Chundru wrote:
>> There are many places we agreed to move the wake and perst gpio's
>> and phy etc to the pcie root port node instead of bridge node[1].
> 
> Same comment as binding patch applies here.
> 
>>
>> So move the phy, phy-names, wake-gpio's in the root port.
> 
> You are not moving any 'wake-gpios' property.
> 
ack I will remove it.
>> There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
>> start using that property instead of perst-gpio.
>>
>> [1] https://lore.kernel.org/linux-pci/20241211192014.GA3302752@bhelgaas/
>>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> ---
>>   arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts   | 5 ++++-
>>   arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++-
>>   arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 5 ++++-
>>   arch/arm64/boot/dts/qcom/sc7280.dtsi           | 6 ++----
>>   4 files changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>> index 7a36c90ad4ec8b52f30b22b1621404857d6ef336..3dd58986ad5da0f898537a51715bb5d0fecbe100 100644
>> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>> @@ -709,8 +709,11 @@ &mdss_edp_phy {
>>   	status = "okay";
>>   };
>>   
>> +&pcie1_port0 {
>> +	reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
>> +};
>> +
>>   &pcie1 {
>> -	perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
>>   
>>   	pinctrl-0 = <&pcie1_reset_n>, <&pcie1_wake_n>;
>>   	pinctrl-names = "default";
> 
> What about the pinctrl properties? They should also be moved.
> 
pinctrl can still reside in the host bridge node, which has
all the gpio's for all the root ports. If we move them to the
root ports we need to explicitly apply pinctrl settings as these
not tied with the driver yet.

- Krishna Chaitanya.
> - Mani
> 

Re: [PATCH v3 3/3] arm64: qcom: sc7280: Move phy, perst to root port node

[Manivannan Sadhasivam]

On Tue, Jun 03, 2025 at 12:03:01PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 6/1/2025 12:35 PM, Manivannan Sadhasivam wrote:
> > On Sat, Apr 19, 2025 at 10:49:26AM +0530, Krishna Chaitanya Chundru wrote:
> > > There are many places we agreed to move the wake and perst gpio's
> > > and phy etc to the pcie root port node instead of bridge node[1].
> > 
> > Same comment as binding patch applies here.
> > 
> > > 
> > > So move the phy, phy-names, wake-gpio's in the root port.
> > 
> > You are not moving any 'wake-gpios' property.
> > 
> ack I will remove it.
> > > There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
> > > start using that property instead of perst-gpio.
> > > 
> > > [1] https://lore.kernel.org/linux-pci/20241211192014.GA3302752@bhelgaas/
> > > 
> > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > > ---
> > >   arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts   | 5 ++++-
> > >   arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++-
> > >   arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 5 ++++-
> > >   arch/arm64/boot/dts/qcom/sc7280.dtsi           | 6 ++----
> > >   4 files changed, 14 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > index 7a36c90ad4ec8b52f30b22b1621404857d6ef336..3dd58986ad5da0f898537a51715bb5d0fecbe100 100644
> > > --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > @@ -709,8 +709,11 @@ &mdss_edp_phy {
> > >   	status = "okay";
> > >   };
> > > +&pcie1_port0 {
> > > +	reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
> > > +};
> > > +
> > >   &pcie1 {
> > > -	perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
> > >   	pinctrl-0 = <&pcie1_reset_n>, <&pcie1_wake_n>;
> > >   	pinctrl-names = "default";
> > 
> > What about the pinctrl properties? They should also be moved.
> > 
> pinctrl can still reside in the host bridge node, which has
> all the gpio's for all the root ports. If we move them to the
> root ports we need to explicitly apply pinctrl settings as these
> not tied with the driver yet.
> 

If the DT node is associated with a device, then the driver core should bind the
pinctrl pins and configure them. Is that not happening here?

- Mani

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

Re: [PATCH v3 3/3] arm64: qcom: sc7280: Move phy, perst to root port node

[Krishna Chaitanya Chundru]

On 6/3/2025 12:22 PM, Manivannan Sadhasivam wrote:
> On Tue, Jun 03, 2025 at 12:03:01PM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 6/1/2025 12:35 PM, Manivannan Sadhasivam wrote:
>>> On Sat, Apr 19, 2025 at 10:49:26AM +0530, Krishna Chaitanya Chundru wrote:
>>>> There are many places we agreed to move the wake and perst gpio's
>>>> and phy etc to the pcie root port node instead of bridge node[1].
>>>
>>> Same comment as binding patch applies here.
>>>
>>>>
>>>> So move the phy, phy-names, wake-gpio's in the root port.
>>>
>>> You are not moving any 'wake-gpios' property.
>>>
>> ack I will remove it.
>>>> There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
>>>> start using that property instead of perst-gpio.
>>>>
>>>> [1] https://lore.kernel.org/linux-pci/20241211192014.GA3302752@bhelgaas/
>>>>
>>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>>>> ---
>>>>    arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts   | 5 ++++-
>>>>    arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++-
>>>>    arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 5 ++++-
>>>>    arch/arm64/boot/dts/qcom/sc7280.dtsi           | 6 ++----
>>>>    4 files changed, 14 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>>>> index 7a36c90ad4ec8b52f30b22b1621404857d6ef336..3dd58986ad5da0f898537a51715bb5d0fecbe100 100644
>>>> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>>>> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>>>> @@ -709,8 +709,11 @@ &mdss_edp_phy {
>>>>    	status = "okay";
>>>>    };
>>>> +&pcie1_port0 {
>>>> +	reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
>>>> +};
>>>> +
>>>>    &pcie1 {
>>>> -	perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
>>>>    	pinctrl-0 = <&pcie1_reset_n>, <&pcie1_wake_n>;
>>>>    	pinctrl-names = "default";
>>>
>>> What about the pinctrl properties? They should also be moved.
>>>
>> pinctrl can still reside in the host bridge node, which has
>> all the gpio's for all the root ports. If we move them to the
>> root ports we need to explicitly apply pinctrl settings as these
>> not tied with the driver yet.
>>
> 
> If the DT node is associated with a device, then the driver core should bind the
> pinctrl pins and configure them. Is that not happening here?
The root node will not be associated with the driver until enumeration,
the controller drivers needs these to be configured before enumeration.

- Krishna Chaitanya.
> 
> - Mani
> 

Re: [PATCH v3 3/3] arm64: qcom: sc7280: Move phy, perst to root port node

[Manivannan Sadhasivam]

On Tue, Jun 03, 2025 at 01:05:17PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 6/3/2025 12:22 PM, Manivannan Sadhasivam wrote:
> > On Tue, Jun 03, 2025 at 12:03:01PM +0530, Krishna Chaitanya Chundru wrote:
> > > 
> > > 
> > > On 6/1/2025 12:35 PM, Manivannan Sadhasivam wrote:
> > > > On Sat, Apr 19, 2025 at 10:49:26AM +0530, Krishna Chaitanya Chundru wrote:
> > > > > There are many places we agreed to move the wake and perst gpio's
> > > > > and phy etc to the pcie root port node instead of bridge node[1].
> > > > 
> > > > Same comment as binding patch applies here.
> > > > 
> > > > > 
> > > > > So move the phy, phy-names, wake-gpio's in the root port.
> > > > 
> > > > You are not moving any 'wake-gpios' property.
> > > > 
> > > ack I will remove it.
> > > > > There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
> > > > > start using that property instead of perst-gpio.
> > > > > 
> > > > > [1] https://lore.kernel.org/linux-pci/20241211192014.GA3302752@bhelgaas/
> > > > > 
> > > > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > > > > ---
> > > > >    arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts   | 5 ++++-
> > > > >    arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++-
> > > > >    arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 5 ++++-
> > > > >    arch/arm64/boot/dts/qcom/sc7280.dtsi           | 6 ++----
> > > > >    4 files changed, 14 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > > > index 7a36c90ad4ec8b52f30b22b1621404857d6ef336..3dd58986ad5da0f898537a51715bb5d0fecbe100 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > > > +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > > > @@ -709,8 +709,11 @@ &mdss_edp_phy {
> > > > >    	status = "okay";
> > > > >    };
> > > > > +&pcie1_port0 {
> > > > > +	reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
> > > > > +};
> > > > > +
> > > > >    &pcie1 {
> > > > > -	perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
> > > > >    	pinctrl-0 = <&pcie1_reset_n>, <&pcie1_wake_n>;
> > > > >    	pinctrl-names = "default";
> > > > 
> > > > What about the pinctrl properties? They should also be moved.
> > > > 
> > > pinctrl can still reside in the host bridge node, which has
> > > all the gpio's for all the root ports. If we move them to the
> > > root ports we need to explicitly apply pinctrl settings as these
> > > not tied with the driver yet.
> > > 
> > 
> > If the DT node is associated with a device, then the driver core should bind the
> > pinctrl pins and configure them. Is that not happening here?
> The root node will not be associated with the driver until enumeration,
> the controller drivers needs these to be configured before enumeration.
> 

Hmm. I'm working on moving the PERST# deassert to pwrctrl drivers, but even then
the PERST# assert needs to happen in the controller driver for initialization.
And moving them to root port node would cause the pin state to be changed
in-between. So I agree, let's leave them in controller node itself.

- Mani

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

Re: [PATCH v3 3/3] arm64: qcom: sc7280: Move phy, perst to root port node

[Konrad Dybcio]

On 6/2/25 3:01 PM, Krzysztof Kozlowski wrote:
> On 08/05/2025 16:26, Konrad Dybcio wrote:
>> On 4/23/25 5:37 PM, Rob Herring wrote:
>>> On Sat, Apr 19, 2025 at 10:49:26AM +0530, Krishna Chaitanya Chundru wrote:
>>>> There are many places we agreed to move the wake and perst gpio's
>>>> and phy etc to the pcie root port node instead of bridge node[1].
>>>>
>>>> So move the phy, phy-names, wake-gpio's in the root port.
>>>> There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
>>>> start using that property instead of perst-gpio.
>>>
>>> Moving the properties will break existing kernels. If that doesn't 
>>> matter for these platforms, say so in the commit msg.
>>
>> I don't think we generally guarantee *forward* dt compatibility though, no?
> We do not guarantee, comment was not about this, but we expect. This DTS
> is supposed and is used by other projects. There was entire complain
> last DT BoF about kernel breaking DTS users all the time.

Yeah I get it.. we're in a constant cycle of adding new components and
later coming to the conclusion that whoever came up with the initial
binding had no clue what they're doing..

That said, "absens carens".. if users or developers of other projects
don't speak up on LKML (which serves as the de facto public square for
DT development), we don't get any feedback to take into account when
making potentially breaking changes (that may have a good reason behind
them). We get a patch from OpenBSD people every now and then, but it's
a drop in the ocean.

Konrad

Re: [PATCH v3 3/3] arm64: qcom: sc7280: Move phy, perst to root port node

[Krzysztof Kozlowski]

On 10/06/2025 15:15, Konrad Dybcio wrote:
> On 6/2/25 3:01 PM, Krzysztof Kozlowski wrote:
>> On 08/05/2025 16:26, Konrad Dybcio wrote:
>>> On 4/23/25 5:37 PM, Rob Herring wrote:
>>>> On Sat, Apr 19, 2025 at 10:49:26AM +0530, Krishna Chaitanya Chundru wrote:
>>>>> There are many places we agreed to move the wake and perst gpio's
>>>>> and phy etc to the pcie root port node instead of bridge node[1].
>>>>>
>>>>> So move the phy, phy-names, wake-gpio's in the root port.
>>>>> There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
>>>>> start using that property instead of perst-gpio.
>>>>
>>>> Moving the properties will break existing kernels. If that doesn't 
>>>> matter for these platforms, say so in the commit msg.
>>>
>>> I don't think we generally guarantee *forward* dt compatibility though, no?
>> We do not guarantee, comment was not about this, but we expect. This DTS
>> is supposed and is used by other projects. There was entire complain
>> last DT BoF about kernel breaking DTS users all the time.
> 
> Yeah I get it.. we're in a constant cycle of adding new components and
> later coming to the conclusion that whoever came up with the initial
> binding had no clue what they're doing..
> 
> That said, "absens carens".. if users or developers of other projects
> don't speak up on LKML (which serves as the de facto public square for
> DT development), we don't get any feedback to take into account when
> making potentially breaking changes (that may have a good reason behind
> them). We get a patch from OpenBSD people every now and then, but it's
> a drop in the ocean.
> 
I don't understand what you are commenting on. Do you reject what I
asked for?

Best regards,
Krzysztof

Re: [PATCH v3 3/3] arm64: qcom: sc7280: Move phy, perst to root port node

[Konrad Dybcio]

On 6/11/25 8:36 AM, Krzysztof Kozlowski wrote:
> On 10/06/2025 15:15, Konrad Dybcio wrote:
>> On 6/2/25 3:01 PM, Krzysztof Kozlowski wrote:
>>> On 08/05/2025 16:26, Konrad Dybcio wrote:
>>>> On 4/23/25 5:37 PM, Rob Herring wrote:
>>>>> On Sat, Apr 19, 2025 at 10:49:26AM +0530, Krishna Chaitanya Chundru wrote:
>>>>>> There are many places we agreed to move the wake and perst gpio's
>>>>>> and phy etc to the pcie root port node instead of bridge node[1].
>>>>>>
>>>>>> So move the phy, phy-names, wake-gpio's in the root port.
>>>>>> There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
>>>>>> start using that property instead of perst-gpio.
>>>>>
>>>>> Moving the properties will break existing kernels. If that doesn't 
>>>>> matter for these platforms, say so in the commit msg.
>>>>
>>>> I don't think we generally guarantee *forward* dt compatibility though, no?
>>> We do not guarantee, comment was not about this, but we expect. This DTS
>>> is supposed and is used by other projects. There was entire complain
>>> last DT BoF about kernel breaking DTS users all the time.
>>
>> Yeah I get it.. we're in a constant cycle of adding new components and
>> later coming to the conclusion that whoever came up with the initial
>> binding had no clue what they're doing..
>>
>> That said, "absens carens".. if users or developers of other projects
>> don't speak up on LKML (which serves as the de facto public square for
>> DT development), we don't get any feedback to take into account when
>> making potentially breaking changes (that may have a good reason behind
>> them). We get a patch from OpenBSD people every now and then, but it's
>> a drop in the ocean.
>>
> I don't understand what you are commenting on. Do you reject what I
> asked for?

If the general consensus among kernel PCIe folks will come down to what
this patch does, I think it's fair to shift to a "correct" hw
description, especially if this is a requirement to resolve a blocker
on functionality (which the author didn't clarify whether is the case)

Konrad

Re: [PATCH v3 3/3] arm64: qcom: sc7280: Move phy, perst to root port node

[Krzysztof Kozlowski]

On 11/06/2025 17:17, Konrad Dybcio wrote:
> On 6/11/25 8:36 AM, Krzysztof Kozlowski wrote:
>> On 10/06/2025 15:15, Konrad Dybcio wrote:
>>> On 6/2/25 3:01 PM, Krzysztof Kozlowski wrote:
>>>> On 08/05/2025 16:26, Konrad Dybcio wrote:
>>>>> On 4/23/25 5:37 PM, Rob Herring wrote:
>>>>>> On Sat, Apr 19, 2025 at 10:49:26AM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>> There are many places we agreed to move the wake and perst gpio's
>>>>>>> and phy etc to the pcie root port node instead of bridge node[1].
>>>>>>>
>>>>>>> So move the phy, phy-names, wake-gpio's in the root port.
>>>>>>> There is already reset-gpio defined for PERST# in pci-bus-common.yaml,
>>>>>>> start using that property instead of perst-gpio.
>>>>>>
>>>>>> Moving the properties will break existing kernels. If that doesn't 
>>>>>> matter for these platforms, say so in the commit msg.
>>>>>
>>>>> I don't think we generally guarantee *forward* dt compatibility though, no?
>>>> We do not guarantee, comment was not about this, but we expect. This DTS
>>>> is supposed and is used by other projects. There was entire complain
>>>> last DT BoF about kernel breaking DTS users all the time.
>>>
>>> Yeah I get it.. we're in a constant cycle of adding new components and
>>> later coming to the conclusion that whoever came up with the initial
>>> binding had no clue what they're doing..
>>>
>>> That said, "absens carens".. if users or developers of other projects
>>> don't speak up on LKML (which serves as the de facto public square for
>>> DT development), we don't get any feedback to take into account when
>>> making potentially breaking changes (that may have a good reason behind
>>> them). We get a patch from OpenBSD people every now and then, but it's
>>> a drop in the ocean.
>>>
>> I don't understand what you are commenting on. Do you reject what I
>> asked for?
> 
> If the general consensus among kernel PCIe folks will come down to what
> this patch does, I think it's fair to shift to a "correct" hw
> description, especially if this is a requirement to resolve a blocker
> on functionality (which the author didn't clarify whether is the case)

Again I do not see how this argues with my comment, so please clarify:
do you agree or disagree with my request?

Best regards,
Krzysztof