Add DT bindings and DT nodes for PCIe and PHY in SC7280

Add PCIe controller and PHY nodes for sc7280 SOC.

Signed-off-by Prasad Malisetty <[email protected]>

---

Enable PCIe controller and PHY for sc7280 IDP board. Add specific NVMe GPIO entries for SKU1 and SKU2 support.

Signed-off-by Prasad Malisetty <[email protected]>

---
 arch/arm64/boot/dts/qcom/sc7280-idp.dts  |  9 +++++++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 32 ++++++++++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280-idp2.dts |  9 +++++++++
 3 files changed, 50 insertions(+)

arch/arm64/boot/dts/qcom/sc7280-idp.dts +0 -0

index 64fc22a..2cc6b0a 100644

arch/arm64/boot/dts/qcom/sc7280-idp.dts +2 -0

+};
+
 &pmk8350_vadc {
        pmr735a_die_temp {
                reg = <PMR735A_ADC7_DIE_TEMP>;

Quoting Prasad Malisetty (2021-09-14 11:19:09)

Quoted 18 lines

Enable PCIe controller and PHY for sc7280 IDP board. Add specific NVMe GPIO entries for SKU1 and SKU2 support.

Signed-off-by Prasad Malisetty <[email protected]>

---
 arch/arm64/boot/dts/qcom/sc7280-idp.dts  |  9 +++++++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 32 ++++++++++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280-idp2.dts |  9 +++++++++
 3 files changed, 50 insertions(+)

arch/arm64/boot/dts/qcom/sc7280-idp.dts +0 -0

index 64fc22a..2cc6b0a 100644
@@ -61,6 +61,15 @@
        modem-init;
 };
 

arch/arm64/boot/dts/qcom/sc7280-idp.dts line 70

Quoted 7 lines

arch/arm64/boot/dts/qcom/sc7280-idp.dts +7 -0

+&pcie1_default_state {
+       nvme-n {
+               pins = "gpio19";
+               function = "gpio";
+
+               bias-pull-up;
+       };

I don't think the style is to have a single container node anymore. Instead, each pin gets a different node and then pinctrl-0 has a list of phandles to the different nodes. qcom maintainers may have more input here.

Also, this should really go into a different section than here. I thought the style was to have a 'board specific' pinctrl section.

Quoted 5 lines

arch/arm64/boot/dts/qcom/sc7280-idp.dts +2 -0

+};
+
 &pmk8350_vadc {
        pmr735a_die_temp {
                reg = <PMR735A_ADC7_DIE_TEMP>;

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

 	int ret;
 
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);

Quoted 10 lines · Prasad Malisetty

On the SC7280, the clock source for gcc_pcie_1_pipe_clk_src must be the TCXO while gdsc is enabled. After PHY init successful clock source should switch to pipe clock for gcc_pcie_1_pipe_clk_src.

Signed-off-by Prasad Malisetty <[email protected]>

---
 drivers/pci/controller/dwc/pcie-qcom.c | 90 
+++++++++++++++++++++++++++++-----
 1 file changed, 79 insertions(+), 11 deletions(-)

drivers/pci/controller/dwc/pcie-qcom.c struct qcom_pcie_ops { · line 197

Quoted 3 lines

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

+struct qcom_pcie_cfg {
+	const struct qcom_pcie_ops *ops;
+	bool pcie_1_pipe_clk_src_switch;

This is OK, but all things being equal I like "unsigned int x:1" a little better. Here's some background:

https://lore.kernel.org/r/CA+55aFzKQ6Pj18TB8p4Yr0M4t+S+BsiHH=BJNmn=76-NcjTj-g@mail.gmail.com/

https://lore.kernel.org/r/CA+55aFxnePDimkVKVtv3gNmRGcwc8KQ5mHYvUxY8sAQg6yvVYg@mail.gmail.com/

drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_probe(struct · line 1535

Quoted 6 lines

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

@@ -1467,6 +1531,7 @@ static int qcom_pcie_probe(struct 
 platform_device *pdev)
 	struct pcie_port *pp;
 	struct dw_pcie *pci;
 	struct qcom_pcie *pcie;
+	const struct qcom_pcie_cfg *pcie_cfg = NULL;

No need to initialize this, since you always assign it before using it.

Quoted 14 lines

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

 	int ret;
 

drivers/pci/controller/dwc/pcie-qcom.c +3 -1

 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);

@@ -1488,7 +1553,9 @@ static int qcom_pcie_probe(struct 
 platform_device *pdev)
 
 	pcie->pci = pci;
 
-	pcie->ops = of_device_get_match_data(dev);
+	pcie_cfg = of_device_get_match_data(dev);
+	pcie->ops = pcie_cfg->ops;
+	pcie->pcie_1_pipe_clk_src_switch = 
 pcie_cfg->pcie_1_pipe_clk_src_switch;
 

drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_probe(struct platform_device *pdev) · line 1561

Quoted 2 lines

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

 	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
 	if (IS_ERR(pcie->reset)) {

Looks good, thanks for working on this!

On the SC7280, the clock source for gcc_pcie_1_pipe_clk_src must be the TCXO while gdsc is enabled. After PHY init successful clock source should switch to pipe clock for gcc_pcie_1_pipe_clk_src.

Signed-off-by Prasad Malisetty <[email protected]>

---
 drivers/pci/controller/dwc/pcie-qcom.c | 90 +++++++++++++++++++++++++++++-----
 1 file changed, 79 insertions(+), 11 deletions(-)

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

index 8a7a300..380c962 100644

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

+};
+
 struct qcom_pcie {
 	struct dw_pcie *pci;
 	void __iomem *parf;			/* DT parf */

@@ -197,6 +205,7 @@ struct qcom_pcie {
 	struct phy *phy;
 	struct gpio_desc *reset;
 	const struct qcom_pcie_ops *ops;
+	bool pcie_1_pipe_clk_src_switch;
 };
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)

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

+		res->gcc_pcie_1_pipe_clk_src = devm_clk_get(dev, "pipe_mux");
+		if (IS_ERR(res->gcc_pcie_1_pipe_clk_src))
+			return PTR_ERR(res->gcc_pcie_1_pipe_clk_src);
+
+		res->phy_pipe_clk = devm_clk_get(dev, "phy_pipe");
+		if (IS_ERR(res->phy_pipe_clk))
+			return PTR_ERR(res->phy_pipe_clk);
+
+		res->ref_clk_src = devm_clk_get(dev, "ref");
+		if (IS_ERR(res->ref_clk_src))
+			return PTR_ERR(res->ref_clk_src);
+	}
+
 	res->pipe_clk = devm_clk_get(dev, "pipe");
 	return PTR_ERR_OR_ZERO(res->pipe_clk);
 }

@@ -1185,6 +1208,10 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 		return ret;
 	}
 
+	/* Set TCXO as clock source for gcc_pcie_1_pipe_clk_src */
+	if (pcie->pcie_1_pipe_clk_src_switch)
+		clk_set_parent(res->gcc_pcie_1_pipe_clk_src, res->ref_clk_src);
+
 	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
 	if (ret < 0)
 		goto err_disable_regulators;

@@ -1256,6 +1283,10 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
 
+	/* Set pipe clock as clock source for gcc_pcie_1_pipe_clk_src */
+	if (pcie->pcie_1_pipe_clk_src_switch)
+		clk_set_parent(res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);
+
 	return clk_prepare_enable(res->pipe_clk);
 }
 

@@ -1456,6 +1487,39 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.config_sid = qcom_pcie_config_sid_sm8250,
 };
 
+static const struct qcom_pcie_cfg apq8084_cfg = {
+	.ops = &ops_1_0_0,
+};
+
+static const struct qcom_pcie_cfg ipq8064_cfg = {
+	.ops = &ops_2_1_0,
+};
+
+static const struct qcom_pcie_cfg msm8996_cfg = {
+	.ops = &ops_2_3_2,
+};
+
+static const struct qcom_pcie_cfg ipq8074_cfg = {
+	.ops = &ops_2_3_3,
+};
+
+static const struct qcom_pcie_cfg ipq4019_cfg = {
+	.ops = &ops_2_4_0,
+};
+
+static const struct qcom_pcie_cfg sdm845_cfg = {
+	.ops = &ops_2_7_0,
+};
+
+static const struct qcom_pcie_cfg sm8250_cfg = {
+	.ops = &ops_1_9_0,
+};
+
+static const struct qcom_pcie_cfg sc7280_cfg = {
+	.ops = &ops_1_9_0,
+	.pcie_1_pipe_clk_src_switch = true,
+};
+
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = qcom_pcie_link_up,
 	.start_link = qcom_pcie_start_link,

drivers/pci/controller/dwc/pcie-qcom.c +14 -11

 	int ret;
 
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);

@@ -1488,7 +1553,9 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	pcie->pci = pci;
 
-	pcie->ops = of_device_get_match_data(dev);
+	pcie_cfg = of_device_get_match_data(dev);
+	pcie->ops = pcie_cfg->ops;
+	pcie->pcie_1_pipe_clk_src_switch = pcie_cfg->pcie_1_pipe_clk_src_switch;
 
 	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
 	if (IS_ERR(pcie->reset)) {

@@ -1545,16 +1612,17 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id qcom_pcie_match[] = {
-	{ .compatible = "qcom,pcie-apq8084", .data = &ops_1_0_0 },
-	{ .compatible = "qcom,pcie-ipq8064", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-apq8064", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-msm8996", .data = &ops_2_3_2 },
-	{ .compatible = "qcom,pcie-ipq8074", .data = &ops_2_3_3 },
-	{ .compatible = "qcom,pcie-ipq4019", .data = &ops_2_4_0 },
-	{ .compatible = "qcom,pcie-qcs404", .data = &ops_2_4_0 },
-	{ .compatible = "qcom,pcie-sdm845", .data = &ops_2_7_0 },
-	{ .compatible = "qcom,pcie-sm8250", .data = &ops_1_9_0 },
+	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
+	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-apq8064", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-msm8996", .data = &msm8996_cfg },
+	{ .compatible = "qcom,pcie-ipq8074", .data = &ipq8074_cfg },
+	{ .compatible = "qcom,pcie-ipq4019", .data = &ipq4019_cfg },
+	{ .compatible = "qcom,pcie-qcs404", .data = &ipq4019_cfg },
+	{ .compatible = "qcom,pcie-sdm845", .data = &sdm845_cfg },
+	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
+	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
 	{ }
 };
 

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Quoted 10 lines · Prasad Malisetty

On the SC7280, the clock source for gcc_pcie_1_pipe_clk_src must be the TCXO while gdsc is enabled. After PHY init successful clock source should switch to pipe clock for gcc_pcie_1_pipe_clk_src.

Signed-off-by Prasad Malisetty <[email protected]>

---
 drivers/pci/controller/dwc/pcie-qcom.c | 90 
+++++++++++++++++++++++++++++-----
 1 file changed, 79 insertions(+), 11 deletions(-)

drivers/pci/controller/dwc/pcie-qcom.c struct qcom_pcie_resources_2_7_0 { · line 169

Quoted 9 lines

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

index 8a7a300..380c962 100644
@@ -166,6 +166,9 @@ struct qcom_pcie_resources_2_7_0 {
 	struct regulator_bulk_data supplies[2];
 	struct reset_control *pci_reset;
 	struct clk *pipe_clk;
+	struct clk *gcc_pcie_1_pipe_clk_src;

Afaict you have both 2 PCIe controllers on sc7280, so I think it seems more reasonable to shorted this to "pipe_clk_src".

Quoted 9 lines

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

+	struct clk *phy_pipe_clk;
+	struct clk *ref_clk_src;
 };
 
 union qcom_pcie_resources {

@@ -189,6 +192,11 @@ struct qcom_pcie_ops {
 	int (*config_sid)(struct qcom_pcie *pcie);
 };
 

drivers/pci/controller/dwc/pcie-qcom.c struct qcom_pcie_ops { · line 197

Quoted 3 lines

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

+struct qcom_pcie_cfg {
+	const struct qcom_pcie_ops *ops;
+	bool pcie_1_pipe_clk_src_switch;

Perhaps something little bit more generic, like pipe_clk_need_muxing?

Quoted 18 lines

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

+};
+
 struct qcom_pcie {
 	struct dw_pcie *pci;
 	void __iomem *parf;			/* DT parf */

@@ -197,6 +205,7 @@ struct qcom_pcie {
 	struct phy *phy;
 	struct gpio_desc *reset;
 	const struct qcom_pcie_ops *ops;
+	bool pcie_1_pipe_clk_src_switch;
 };
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)

@@ -1167,6 +1176,20 @@ static int qcom_pcie_get_resources_2_7_0(struct 
 qcom_pcie *pcie)
 	if (ret < 0)
 		return ret;
 

drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) · line 1179

Quoted 1 line

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

+	if (pcie->pcie_1_pipe_clk_src_switch) {

This looks much better, now it will easily scale to other platforms that have inherited this need.

Quoted 82 lines

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

+		res->gcc_pcie_1_pipe_clk_src = devm_clk_get(dev, "pipe_mux");
+		if (IS_ERR(res->gcc_pcie_1_pipe_clk_src))
+			return PTR_ERR(res->gcc_pcie_1_pipe_clk_src);
+
+		res->phy_pipe_clk = devm_clk_get(dev, "phy_pipe");
+		if (IS_ERR(res->phy_pipe_clk))
+			return PTR_ERR(res->phy_pipe_clk);
+
+		res->ref_clk_src = devm_clk_get(dev, "ref");
+		if (IS_ERR(res->ref_clk_src))
+			return PTR_ERR(res->ref_clk_src);
+	}
+
 	res->pipe_clk = devm_clk_get(dev, "pipe");
 	return PTR_ERR_OR_ZERO(res->pipe_clk);

}

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

@@ -1185,6 +1208,10 @@ static int qcom_pcie_init_2_7_0(struct 
 qcom_pcie *pcie)
 		return ret;
 	}
 
+	/* Set TCXO as clock source for gcc_pcie_1_pipe_clk_src */
+	if (pcie->pcie_1_pipe_clk_src_switch)
+		clk_set_parent(res->gcc_pcie_1_pipe_clk_src, res->ref_clk_src);
+
 	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
 	if (ret < 0)

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

 		goto err_disable_regulators;

@@ -1256,6 +1283,10 @@ static int qcom_pcie_post_init_2_7_0(struct 
 qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
 
+	/* Set pipe clock as clock source for gcc_pcie_1_pipe_clk_src */
+	if (pcie->pcie_1_pipe_clk_src_switch)
+		clk_set_parent(res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);
+
 	return clk_prepare_enable(res->pipe_clk);
 }

@@ -1456,6 +1487,39 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.config_sid = qcom_pcie_config_sid_sm8250,
 };
 
+static const struct qcom_pcie_cfg apq8084_cfg = {
+	.ops = &ops_1_0_0,
+};
+
+static const struct qcom_pcie_cfg ipq8064_cfg = {
+	.ops = &ops_2_1_0,
+};
+
+static const struct qcom_pcie_cfg msm8996_cfg = {
+	.ops = &ops_2_3_2,
+};
+
+static const struct qcom_pcie_cfg ipq8074_cfg = {
+	.ops = &ops_2_3_3,
+};
+
+static const struct qcom_pcie_cfg ipq4019_cfg = {
+	.ops = &ops_2_4_0,
+};
+
+static const struct qcom_pcie_cfg sdm845_cfg = {
+	.ops = &ops_2_7_0,
+};
+
+static const struct qcom_pcie_cfg sm8250_cfg = {
+	.ops = &ops_1_9_0,
+};
+
+static const struct qcom_pcie_cfg sc7280_cfg = {
+	.ops = &ops_1_9_0,
+	.pcie_1_pipe_clk_src_switch = true,
+};
+
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = qcom_pcie_link_up,
 	.start_link = qcom_pcie_start_link,

drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_probe(struct · line 1535

Quoted 5 lines

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

@@ -1467,6 +1531,7 @@ static int qcom_pcie_probe(struct 
 platform_device *pdev)
 	struct pcie_port *pp;
 	struct dw_pcie *pci;
 	struct qcom_pcie *pcie;
+	const struct qcom_pcie_cfg *pcie_cfg = NULL;

First use of this variable is an assignment, so I don't see a need for zero initialize it.

Regards,
Bjorn

Quoted 49 lines

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

 	int ret;
 

drivers/pci/controller/dwc/pcie-qcom.c +3 -1

 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);

@@ -1488,7 +1553,9 @@ static int qcom_pcie_probe(struct 
 platform_device *pdev)
 
 	pcie->pci = pci;
 
-	pcie->ops = of_device_get_match_data(dev);
+	pcie_cfg = of_device_get_match_data(dev);
+	pcie->ops = pcie_cfg->ops;
+	pcie->pcie_1_pipe_clk_src_switch = 
 pcie_cfg->pcie_1_pipe_clk_src_switch;
 

drivers/pci/controller/dwc/pcie-qcom.c +11 -11

 	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
 	if (IS_ERR(pcie->reset)) {

@@ -1545,16 +1612,17 @@ static int qcom_pcie_probe(struct 
 platform_device *pdev)
 }
 
 static const struct of_device_id qcom_pcie_match[] = {
-	{ .compatible = "qcom,pcie-apq8084", .data = &ops_1_0_0 },
-	{ .compatible = "qcom,pcie-ipq8064", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-apq8064", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-msm8996", .data = &ops_2_3_2 },
-	{ .compatible = "qcom,pcie-ipq8074", .data = &ops_2_3_3 },
-	{ .compatible = "qcom,pcie-ipq4019", .data = &ops_2_4_0 },
-	{ .compatible = "qcom,pcie-qcs404", .data = &ops_2_4_0 },
-	{ .compatible = "qcom,pcie-sdm845", .data = &ops_2_7_0 },
-	{ .compatible = "qcom,pcie-sm8250", .data = &ops_1_9_0 },
+	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
+	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-apq8064", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-msm8996", .data = &msm8996_cfg },
+	{ .compatible = "qcom,pcie-ipq8074", .data = &ipq8074_cfg },
+	{ .compatible = "qcom,pcie-ipq4019", .data = &ipq4019_cfg },
+	{ .compatible = "qcom,pcie-qcs404", .data = &ipq4019_cfg },
+	{ .compatible = "qcom,pcie-sdm845", .data = &sdm845_cfg },
+	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
+	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
 	{ }
 };
 
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project