PCI: qcom: Add basic interconnect support

Add the missing SC8280XP/SA8540P "pcie-mem" and "cpu-pcie" interconnect paths to the bindings.

Fixes 76d777ae045e ("dt-bindings: PCI: qcom: Add SC8280XP to binding")

Fixes 76c4207f4085 ("dt-bindings: PCI: qcom: Add SA8540P to binding")

Signed-off-by Johan Hovold <[email protected]>

---
 .../devicetree/bindings/pci/qcom,pcie.yaml    | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

Add the missing SC8280XP/SA8540P "pcie-mem" and "cpu-pcie" interconnect paths to the bindings.

Fixes 76d777ae045e ("dt-bindings: PCI: qcom: Add SC8280XP to binding")

Fixes 76c4207f4085 ("dt-bindings: PCI: qcom: Add SA8540P to binding")

Signed-off-by Johan Hovold <[email protected]>

---
 .../devicetree/bindings/pci/qcom,pcie.yaml    | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

Documentation/devicetree/bindings/pci/qcom,pcie.yaml +20 -0

index 54f07852d279..2f851c804bb0 100644
@@ -62,6 +62,14 @@ properties:
     minItems: 3
     maxItems: 13
 
+  interconnects:
+    maxItems: 2
+
+  interconnect-names:
+    items:
+      - const: pcie-mem
+      - const: cpu-pcie
+
   resets:
     minItems: 1
     maxItems: 12

@@ -631,6 +639,18 @@ allOf:
           items:
             - const: pci # PCIe core reset
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,pcie-sa8540p
+              - qcom,pcie-sc8280xp
+    then:
+      required:
+        - interconnects
+        - interconnect-names
+
   - if:
       not:
         properties:

Signature · 1 line

2.37.3

On Qualcomm platforms like SC8280XP and SA8540P, interconnect bandwidth must be requested before enabling interconnect clocks.

Add basic support for managing an optional "pcie-mem" interconnect path by setting a low constraint before enabling clocks and updating it after the link is up.

Note that it is not possible for a controller driver to set anything but a maximum peak bandwidth as expected average bandwidth will vary with use case and actual use (and power policy?). This very much remains an unresolved problem with the interconnect framework.

Also note that no constraint is set for the SC8280XP/SA8540P "cpu-pcie" path for now as it is not clear what an appropriate constraint would be (and the system does not crash when left unspecified).

Fixes 70574511f3fc ("PCI: qcom: Add support for SC8280XP")

Reviewed-by Brian Masney <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
  drivers/pci/controller/dwc/pcie-qcom.c | 76 ++++++++++++++++++++++++++
  1 file changed, 76 insertions(+)

drivers/pci/controller/dwc/pcie-qcom.c +76 -0

index 7db94a22238d..0c13f976626f 100644
@@ -12,6 +12,7 @@
  #include <linux/crc8.h>
  #include <linux/delay.h>
  #include <linux/gpio/consumer.h>
+#include <linux/interconnect.h>
  #include <linux/interrupt.h>
  #include <linux/io.h>
  #include <linux/iopoll.h>

@@ -224,6 +225,7 @@ struct qcom_pcie {
  	union qcom_pcie_resources res;
  	struct phy *phy;
  	struct gpio_desc *reset;
+	struct icc_path *icc_mem;
  	const struct qcom_pcie_cfg *cfg;
  };
 

@@ -1644,6 +1646,74 @@ static const struct dw_pcie_ops dw_pcie_ops = {
  	.start_link = qcom_pcie_start_link,
  };
 
+static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
+{
+	struct dw_pcie *pci = pcie->pci;
+	int ret;
+
+	pcie->icc_mem = devm_of_icc_get(pci->dev, "pcie-mem");
+	if (IS_ERR(pcie->icc_mem)) {
+		ret = PTR_ERR(pcie->icc_mem);
+		return ret;
+	}
+
+	/*
+	 * Some Qualcomm platforms require interconnect bandwidth constraints
+	 * to be set before enabling interconnect clocks.
+	 *
+	 * Set an initial peak bandwidth corresponding to single-lane Gen 1
+	 * for the pcie-mem path.
+	 */
+	ret = icc_set_bw(pcie->icc_mem, 0, MBps_to_icc(250));
+	if (ret) {
+		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
+{
+	struct dw_pcie *pci = pcie->pci;
+	u32 offset, status, bw;
+	int speed, width;
+	int ret;
+
+	if (!pcie->icc_mem)
+		return;
+
+	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
+
+	/* Only update constraints if link is up. */
+	if (!(status & PCI_EXP_LNKSTA_DLLLA))
+		return;
+
+	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
+	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
+
+	switch (speed) {
+	case 1:
+		bw = MBps_to_icc(250);
+		break;
+	case 2:
+		bw = MBps_to_icc(500);
+		break;
+	default:
+	case 3:
+		bw = MBps_to_icc(985);
+		break;
+	}
+
+	ret = icc_set_bw(pcie->icc_mem, 0, width * bw);
+	if (ret) {
+		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+			ret);
+	}
+}
+
  static int qcom_pcie_probe(struct platform_device *pdev)
  {
  	struct device *dev = &pdev->dev;

@@ -1704,6 +1774,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
  		goto err_pm_runtime_put;
  	}
 
+	ret = qcom_pcie_icc_init(pcie);
+	if (ret)
+		goto err_pm_runtime_put;
+
  	ret = pcie->cfg->ops->get_resources(pcie);
  	if (ret)
  		goto err_pm_runtime_put;

@@ -1722,6 +1796,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
  		goto err_phy_exit;
  	}
 
+	qcom_pcie_icc_update(pcie);
+
  	return 0;
 
  err_phy_exit:

On Qualcomm platforms like SC8280XP and SA8540P, interconnect bandwidth must be requested before enabling interconnect clocks.

Add basic support for managing an optional "pcie-mem" interconnect path by setting a low constraint before enabling clocks and updating it after the link is up.

Note that it is not possible for a controller driver to set anything but a maximum peak bandwidth as expected average bandwidth will vary with use case and actual use (and power policy?). This very much remains an unresolved problem with the interconnect framework.

Also note that no constraint is set for the SC8280XP/SA8540P "cpu-pcie" path for now as it is not clear what an appropriate constraint would be (and the system does not crash when left unspecified).

Fixes 70574511f3fc ("PCI: qcom: Add support for SC8280XP")

Reviewed-by Brian Masney <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/pci/controller/dwc/pcie-qcom.c | 76 ++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

drivers/pci/controller/dwc/pcie-qcom.c +2 -0

index 7db94a22238d..0c13f976626f 100644
@@ -12,6 +12,7 @@
 #include <linux/crc8.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
+#include <linux/interconnect.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>

@@ -224,6 +225,7 @@ struct qcom_pcie {
 	union qcom_pcie_resources res;
 	struct phy *phy;
 	struct gpio_desc *reset;
+	struct icc_path *icc_mem;
 	const struct qcom_pcie_cfg *cfg;
 };
 

@@ -1644,6 +1646,74 @@ static const struct dw_pcie_ops dw_pcie_ops = {
 	.start_link = qcom_pcie_start_link,
 };

drivers/pci/controller/dwc/pcie-qcom.c +12 -0

+		return ret;
+	}
+
+	return 0;
+}
+
+static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
+{
+	struct dw_pcie *pci = pcie->pci;
+	u32 offset, status, bw;
+	int speed, width;
+	int ret;

drivers/pci/controller/dwc/pcie-qcom.c +9 -0

+	}
+}
+
 static int qcom_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;

@@ -1704,6 +1774,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		goto err_pm_runtime_put;
 	}
 
+	ret = qcom_pcie_icc_init(pcie);
+	if (ret)
+		goto err_pm_runtime_put;
+
 	ret = pcie->cfg->ops->get_resources(pcie);
 	if (ret)
 		goto err_pm_runtime_put;

@@ -1722,6 +1796,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		goto err_phy_exit;
 	}
 
+	qcom_pcie_icc_update(pcie);
+
 	return 0;
 
 err_phy_exit:

Signature · 1 line

2.37.3

Quoted 15 lines · Johan Hovold

On Qualcomm platforms like SC8280XP and SA8540P, interconnect bandwidth must be requested before enabling interconnect clocks.

Add basic support for managing an optional "pcie-mem" interconnect path by setting a low constraint before enabling clocks and updating it after the link is up.

Note that it is not possible for a controller driver to set anything but a maximum peak bandwidth as expected average bandwidth will vary with use case and actual use (and power policy?). This very much remains an unresolved problem with the interconnect framework.

Also note that no constraint is set for the SC8280XP/SA8540P "cpu-pcie" path for now as it is not clear what an appropriate constraint would be (and the system does not crash when left unspecified).

drivers/pci/controller/dwc/pcie-qcom.c static const struct dw_pcie_ops dw_pcie_ops = { · line 1657

Quoted 9 lines

drivers/pci/controller/dwc/pcie-qcom.c +9 -0

+static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
+{
+	struct dw_pcie *pci = pcie->pci;
+	int ret;
+
+	pcie->icc_mem = devm_of_icc_get(pci->dev, "pcie-mem");
+	if (IS_ERR(pcie->icc_mem)) {
+		ret = PTR_ERR(pcie->icc_mem);
+		return ret;

return PTR_ERR(pcie->icc_mem);

Quoted 7 lines

drivers/pci/controller/dwc/pcie-qcom.c +7 -0

+	}
+
+	/*
+	 * Some Qualcomm platforms require interconnect bandwidth constraints
+	 * to be set before enabling interconnect clocks.
+	 *
+	 * Set an initial peak bandwidth corresponding to single-lane Gen 1

drivers/pci/controller/dwc/pcie-qcom.c static const struct dw_pcie_ops dw_pcie_ops = { · line 1670

Quoted 6 lines

drivers/pci/controller/dwc/pcie-qcom.c +6 -0

+	 * for the pcie-mem path.
+	 */
+	ret = icc_set_bw(pcie->icc_mem, 0, MBps_to_icc(250));
+	if (ret) {
+		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+			ret);

Move "ret);" to prior line. No need to keep up within 80 columns.

Quoted 17 lines

drivers/pci/controller/dwc/pcie-qcom.c +17 -0

+		return ret;
+	}
+
+	return 0;
+}
+
+static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
+{
+	struct dw_pcie *pci = pcie->pci;
+	u32 offset, status, bw;
+	int speed, width;
+	int ret;
+
+	if (!pcie->icc_mem)
+		return;
+
+	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);

drivers/pci/controller/dwc/pcie-qcom.c static const struct dw_pcie_ops dw_pcie_ops = { · line 1692

Quoted 5 lines

drivers/pci/controller/dwc/pcie-qcom.c +5 -0

+	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
+
+	/* Only update constraints if link is up. */
+	if (!(status & PCI_EXP_LNKSTA_DLLLA))
+		return;

What if the link comes back later? I'd suggest to call this function from qcom_pcie_link_up(), whenever link is up.

Quoted 7 lines

drivers/pci/controller/dwc/pcie-qcom.c +7 -0

+
+	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
+	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
+
+	switch (speed) {
+	case 1:
+		bw = MBps_to_icc(250);

drivers/pci/controller/dwc/pcie-qcom.c static const struct dw_pcie_ops dw_pcie_ops = { · line 1705

Quoted 6 lines

drivers/pci/controller/dwc/pcie-qcom.c +6 -0

+		break;
+	case 2:
+		bw = MBps_to_icc(500);
+		break;
+	default:
+	case 3:

Why do you need explicit "case 3" and not just default case?

drivers/pci/controller/dwc/pcie-qcom.c static const struct dw_pcie_ops dw_pcie_ops = { · line 1713

Quoted 8 lines

drivers/pci/controller/dwc/pcie-qcom.c +8 -0

+		bw = MBps_to_icc(985);
+		break;
+	}
+
+	ret = icc_set_bw(pcie->icc_mem, 0, width * bw);
+	if (ret) {
+		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+			ret);

Quoted 17 lines · Manivannan Sadhasivam

Quoted 15 lines · Johan Hovold

On Qualcomm platforms like SC8280XP and SA8540P, interconnect bandwidth must be requested before enabling interconnect clocks.

Add basic support for managing an optional "pcie-mem" interconnect path by setting a low constraint before enabling clocks and updating it after the link is up.

Note that it is not possible for a controller driver to set anything but a maximum peak bandwidth as expected average bandwidth will vary with use case and actual use (and power policy?). This very much remains an unresolved problem with the interconnect framework.

Also note that no constraint is set for the SC8280XP/SA8540P "cpu-pcie" path for now as it is not clear what an appropriate constraint would be (and the system does not crash when left unspecified).

Quoted 15 lines

drivers/pci/controller/dwc/pcie-qcom.c static const struct dw_pcie_ops dw_pcie_ops = { · line 1670

Quoted 13 lines

drivers/pci/controller/dwc/pcie-qcom.c +13 -0

+	}
+
+	/*
+	 * Some Qualcomm platforms require interconnect bandwidth constraints
+	 * to be set before enabling interconnect clocks.
+	 *
+	 * Set an initial peak bandwidth corresponding to single-lane Gen 1
+	 * for the pcie-mem path.
+	 */
+	ret = icc_set_bw(pcie->icc_mem, 0, MBps_to_icc(250));
+	if (ret) {
+		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+			ret);

Move "ret);" to prior line. No need to keep up within 80 columns.

80 chars is still a soft limit and in this case there's no real gain in terms of readability from breaking it.

But sure, I can remove the line break.