[PATCH 0/3] PCI: qcom: pipe_clk_src fixes for pcie-qcom driver

[PATCH 0/3] PCI: qcom: pipe_clk_src fixes for pcie-qcom driver

[Dmitry Baryshkov]

After comparing upstream and downstream Qualcomm PCIe drivers, change
the way the driver works with the pipe_clk_src multiplexing.

The clock should be switched to using ref_clk (TCXO) as a parent before
turning the PCIE_x_GDSC power domain off and can be switched to using
PHY's pipe_clk after this power domain is turned on.

Downstream driver uses regulators for the GDSC, so current approach also
(incorrectly) uses them. However upstream driver uses power-domain and
so GDSC is maintained using pm_runtime_foo() calls. Change order of
operations to implement these requirements.

----------------------------------------------------------------
Dmitry Baryshkov (3):
      PCI: qcom: Balance pm_runtime_foo() calls
      PCI: qcom: Fix pipe_clk_src reparenting
      PCI: qcom: Remove unnecessary pipe_clk handling

 drivers/pci/controller/dwc/pcie-qcom.c | 122 +++++++--------------------------
 1 file changed, 25 insertions(+), 97 deletions(-)

[PATCH 1/3] PCI: qcom: Balance pm_runtime_foo() calls

[Dmitry Baryshkov]

Fix the error path in qcom_pcie_probe(): remove extra calls to
pm_runtime_disable() (which will be called at the end of error path
anyway). Replace a call to pm_runtime_get_sync() with
pm_runtime_resume_and_get() to end up with cleaner code.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 1c3d1116bb60..3a0f126db5a3 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1543,9 +1543,9 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	pm_runtime_enable(dev);
-	ret = pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret < 0)
-		goto err_pm_runtime_put;
+		goto err_pm_runtime_disable;
 
 	pci->dev = dev;
 	pci->ops = &dw_pcie_ops;
@@ -1594,7 +1594,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	ret = phy_init(pcie->phy);
 	if (ret) {
-		pm_runtime_disable(&pdev->dev);
 		goto err_pm_runtime_put;
 	}
 
@@ -1603,7 +1602,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	ret = dw_pcie_host_init(pp);
 	if (ret) {
 		dev_err(dev, "cannot initialize host\n");
-		pm_runtime_disable(&pdev->dev);
 		goto err_pm_runtime_put;
 	}
 
@@ -1611,6 +1609,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 err_pm_runtime_put:
 	pm_runtime_put(dev);
+err_pm_runtime_disable:
 	pm_runtime_disable(dev);
 
 	return ret;
-- 
2.34.1

[PATCH 2/3] PCI: qcom: Fix pipe_clk_src reparenting

[Dmitry Baryshkov]

The hardware requires that pipe_clk_src is fed from TCXO when GDSC is
disabled. It can be fed from PHY's pipe_clk once GDSC is enabled (which
is what is done by the downstream driver).

Currently code does all clk_set_parent() calls after the
pm_runtime_get(), so the GDSC is already enabled.
Implement these requirements by moving pm_runtime_*() calls after
get_resources (so that get_resources() can ensure that the pipe clock
parent is TCXO).

Fixes: aa9c0df98c29 ("PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280")
Cc: Prasad Malisetty <pmaliset@codeaurora.org>
Cc: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++--------------
 1 file changed, 24 insertions(+), 28 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 3a0f126db5a3..fbaae6f4eb18 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1188,6 +1188,9 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 		res->ref_clk_src = devm_clk_get(dev, "ref");
 		if (IS_ERR(res->ref_clk_src))
 			return PTR_ERR(res->ref_clk_src);
+
+		/* Ensure that the TCXO is a clock source for pcie_pipe_clk_src */
+		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
 	}
 
 	res->pipe_clk = devm_clk_get(dev, "pipe");
@@ -1208,9 +1211,9 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 		return ret;
 	}
 
-	/* Set TCXO as clock source for pcie_pipe_clk_src */
+	/* Set pipe clock as clock source for pcie_pipe_clk_src */
 	if (pcie->pipe_clk_need_muxing)
-		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
+		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
 
 	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
 	if (ret < 0)
@@ -1276,6 +1279,11 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
 
 	clk_bulk_disable_unprepare(res->num_clks, res->clks);
+
+	/* Set TCXO as clock source for pcie_pipe_clk_src */
+	if (pcie->pipe_clk_need_muxing)
+		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
+
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
 }
 
@@ -1283,10 +1291,6 @@ 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 pcie_pipe_clk_src */
-	if (pcie->pipe_clk_need_muxing)
-		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
-
 	return clk_prepare_enable(res->pipe_clk);
 }
 
@@ -1542,11 +1546,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	if (!pci)
 		return -ENOMEM;
 
-	pm_runtime_enable(dev);
-	ret = pm_runtime_resume_and_get(dev);
-	if (ret < 0)
-		goto err_pm_runtime_disable;
-
 	pci->dev = dev;
 	pci->ops = &dw_pcie_ops;
 	pp = &pci->pp;
@@ -1563,32 +1562,29 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	pcie->pipe_clk_need_muxing = pcie_cfg->pipe_clk_need_muxing;
 
 	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;
-	}
+	if (IS_ERR(pcie->reset))
+		return PTR_ERR(pcie->reset);
 
 	pcie->parf = devm_platform_ioremap_resource_byname(pdev, "parf");
-	if (IS_ERR(pcie->parf)) {
-		ret = PTR_ERR(pcie->parf);
-		goto err_pm_runtime_put;
-	}
+	if (IS_ERR(pcie->parf))
+		return PTR_ERR(pcie->parf);
 
 	pcie->elbi = devm_platform_ioremap_resource_byname(pdev, "elbi");
-	if (IS_ERR(pcie->elbi)) {
-		ret = PTR_ERR(pcie->elbi);
-		goto err_pm_runtime_put;
-	}
+	if (IS_ERR(pcie->elbi))
+		return PTR_ERR(pcie->elbi);
 
 	pcie->phy = devm_phy_optional_get(dev, "pciephy");
-	if (IS_ERR(pcie->phy)) {
-		ret = PTR_ERR(pcie->phy);
-		goto err_pm_runtime_put;
-	}
+	if (IS_ERR(pcie->phy))
+		return PTR_ERR(pcie->phy);
 
 	ret = pcie->ops->get_resources(pcie);
 	if (ret)
-		goto err_pm_runtime_put;
+		return ret;
+
+	pm_runtime_enable(dev);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
+		goto err_pm_runtime_disable;
 
 	pp->ops = &qcom_pcie_dw_ops;
 
-- 
2.34.1

[PATCH 3/3] PCI: qcom: Remove unnecessary pipe_clk handling

[Dmitry Baryshkov]

QMP PHY driver already does clk_prepare_enable()/_disable() pipe_clk.
Remove extra calls to enable/disable this clock from the PCIe driver, so
that the PHY driver can manage the clock on its own.

Fixes: aa9c0df98c29 ("PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 71 +-------------------------
 1 file changed, 2 insertions(+), 69 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index fbaae6f4eb18..4e668da96ef4 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -128,7 +128,6 @@ struct qcom_pcie_resources_2_3_2 {
 	struct clk *master_clk;
 	struct clk *slave_clk;
 	struct clk *cfg_clk;
-	struct clk *pipe_clk;
 	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
 };
 
@@ -165,7 +164,6 @@ struct qcom_pcie_resources_2_7_0 {
 	int num_clks;
 	struct regulator_bulk_data supplies[2];
 	struct reset_control *pci_reset;
-	struct clk *pipe_clk;
 	struct clk *pipe_clk_src;
 	struct clk *phy_pipe_clk;
 	struct clk *ref_clk_src;
@@ -185,9 +183,7 @@ struct qcom_pcie;
 struct qcom_pcie_ops {
 	int (*get_resources)(struct qcom_pcie *pcie);
 	int (*init)(struct qcom_pcie *pcie);
-	int (*post_init)(struct qcom_pcie *pcie);
 	void (*deinit)(struct qcom_pcie *pcie);
-	void (*post_deinit)(struct qcom_pcie *pcie);
 	void (*ltssm_enable)(struct qcom_pcie *pcie);
 	int (*config_sid)(struct qcom_pcie *pcie);
 };
@@ -591,11 +587,7 @@ static int qcom_pcie_get_resources_2_3_2(struct qcom_pcie *pcie)
 		return PTR_ERR(res->master_clk);
 
 	res->slave_clk = devm_clk_get(dev, "bus_slave");
-	if (IS_ERR(res->slave_clk))
-		return PTR_ERR(res->slave_clk);
-
-	res->pipe_clk = devm_clk_get(dev, "pipe");
-	return PTR_ERR_OR_ZERO(res->pipe_clk);
+	return PTR_ERR_OR_ZERO(res->slave_clk);
 }
 
 static void qcom_pcie_deinit_2_3_2(struct qcom_pcie *pcie)
@@ -610,13 +602,6 @@ static void qcom_pcie_deinit_2_3_2(struct qcom_pcie *pcie)
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
 }
 
-static void qcom_pcie_post_deinit_2_3_2(struct qcom_pcie *pcie)
-{
-	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
-
-	clk_disable_unprepare(res->pipe_clk);
-}
-
 static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
@@ -691,22 +676,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
 	return ret;
 }
 
-static int qcom_pcie_post_init_2_3_2(struct qcom_pcie *pcie)
-{
-	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
-	struct dw_pcie *pci = pcie->pci;
-	struct device *dev = pci->dev;
-	int ret;
-
-	ret = clk_prepare_enable(res->pipe_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable pipe clock\n");
-		return ret;
-	}
-
-	return 0;
-}
-
 static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
@@ -1193,8 +1162,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
 	}
 
-	res->pipe_clk = devm_clk_get(dev, "pipe");
-	return PTR_ERR_OR_ZERO(res->pipe_clk);
+	return 0;
 }
 
 static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
@@ -1233,12 +1201,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 		goto err_disable_clocks;
 	}
 
-	ret = clk_prepare_enable(res->pipe_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable pipe clock\n");
-		goto err_disable_clocks;
-	}
-
 	/* configure PCIe to RC mode */
 	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
 
@@ -1287,20 +1249,6 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
 }
 
-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;
-
-	return clk_prepare_enable(res->pipe_clk);
-}
-
-static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
-{
-	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
-
-	clk_disable_unprepare(res->pipe_clk);
-}
-
 static int qcom_pcie_link_up(struct dw_pcie *pci)
 {
 	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
@@ -1396,12 +1344,6 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
 	if (ret)
 		goto err_deinit;
 
-	if (pcie->ops->post_init) {
-		ret = pcie->ops->post_init(pcie);
-		if (ret)
-			goto err_disable_phy;
-	}
-
 	qcom_ep_reset_deassert(pcie);
 
 	if (pcie->ops->config_sid) {
@@ -1414,9 +1356,6 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
 
 err:
 	qcom_ep_reset_assert(pcie);
-	if (pcie->ops->post_deinit)
-		pcie->ops->post_deinit(pcie);
-err_disable_phy:
 	phy_power_off(pcie->phy);
 err_deinit:
 	pcie->ops->deinit(pcie);
@@ -1448,9 +1387,7 @@ static const struct qcom_pcie_ops ops_1_0_0 = {
 static const struct qcom_pcie_ops ops_2_3_2 = {
 	.get_resources = qcom_pcie_get_resources_2_3_2,
 	.init = qcom_pcie_init_2_3_2,
-	.post_init = qcom_pcie_post_init_2_3_2,
 	.deinit = qcom_pcie_deinit_2_3_2,
-	.post_deinit = qcom_pcie_post_deinit_2_3_2,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 };
 
@@ -1476,8 +1413,6 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
 	.init = qcom_pcie_init_2_7_0,
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
-	.post_init = qcom_pcie_post_init_2_7_0,
-	.post_deinit = qcom_pcie_post_deinit_2_7_0,
 };
 
 /* Qcom IP rev.: 1.9.0 */
@@ -1486,8 +1421,6 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.init = qcom_pcie_init_2_7_0,
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
-	.post_init = qcom_pcie_post_init_2_7_0,
-	.post_deinit = qcom_pcie_post_deinit_2_7_0,
 	.config_sid = qcom_pcie_config_sid_sm8250,
 };
 
-- 
2.34.1

Re: [PATCH 0/3] PCI: qcom: pipe_clk_src fixes for pcie-qcom driver

[Vinod Koul]

On 18-12-21, 17:02, Dmitry Baryshkov wrote:
> After comparing upstream and downstream Qualcomm PCIe drivers, change
> the way the driver works with the pipe_clk_src multiplexing.
> 
> The clock should be switched to using ref_clk (TCXO) as a parent before
> turning the PCIE_x_GDSC power domain off and can be switched to using
> PHY's pipe_clk after this power domain is turned on.
> 
> Downstream driver uses regulators for the GDSC, so current approach also
> (incorrectly) uses them. However upstream driver uses power-domain and
> so GDSC is maintained using pm_runtime_foo() calls. Change order of
> operations to implement these requirements.

Reviewed-by: Vinod Koul <vkoul@kernel.org>

> 
> ----------------------------------------------------------------
> Dmitry Baryshkov (3):
>       PCI: qcom: Balance pm_runtime_foo() calls
>       PCI: qcom: Fix pipe_clk_src reparenting
>       PCI: qcom: Remove unnecessary pipe_clk handling
> 
>  drivers/pci/controller/dwc/pcie-qcom.c | 122 +++++++--------------------------
>  1 file changed, 25 insertions(+), 97 deletions(-)

-- 
~Vinod

Re: [PATCH 0/3] PCI: qcom: pipe_clk_src fixes for pcie-qcom driver

[Stephen Boyd]

Quoting Dmitry Baryshkov (2021-12-18 06:02:20)
> After comparing upstream and downstream Qualcomm PCIe drivers, change
> the way the driver works with the pipe_clk_src multiplexing.
>
> The clock should be switched to using ref_clk (TCXO) as a parent before
> turning the PCIE_x_GDSC power domain off and can be switched to using
> PHY's pipe_clk after this power domain is turned on.
>
> Downstream driver uses regulators for the GDSC, so current approach also
> (incorrectly) uses them. However upstream driver uses power-domain and
> so GDSC is maintained using pm_runtime_foo() calls. Change order of
> operations to implement these requirements.

Prasad, can you test/review this series?

Re: [PATCH 2/3] PCI: qcom: Fix pipe_clk_src reparenting

[Bjorn Andersson]

On Sat 18 Dec 06:02 PST 2021, Dmitry Baryshkov wrote:

> The hardware requires that pipe_clk_src is fed from TCXO when GDSC is
> disabled. It can be fed from PHY's pipe_clk once GDSC is enabled (which
> is what is done by the downstream driver).
> 
> Currently code does all clk_set_parent() calls after the
> pm_runtime_get(), so the GDSC is already enabled.
> Implement these requirements by moving pm_runtime_*() calls after
> get_resources (so that get_resources() can ensure that the pipe clock
> parent is TCXO).
> 
> Fixes: aa9c0df98c29 ("PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280")
> Cc: Prasad Malisetty <pmaliset@codeaurora.org>
> Cc: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++--------------
>  1 file changed, 24 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 3a0f126db5a3..fbaae6f4eb18 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1188,6 +1188,9 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>  		res->ref_clk_src = devm_clk_get(dev, "ref");
>  		if (IS_ERR(res->ref_clk_src))
>  			return PTR_ERR(res->ref_clk_src);
> +
> +		/* Ensure that the TCXO is a clock source for pcie_pipe_clk_src */
> +		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
>  	}
>  
>  	res->pipe_clk = devm_clk_get(dev, "pipe");
> @@ -1208,9 +1211,9 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
>  		return ret;
>  	}
>  
> -	/* Set TCXO as clock source for pcie_pipe_clk_src */
> +	/* Set pipe clock as clock source for pcie_pipe_clk_src */
>  	if (pcie->pipe_clk_need_muxing)
> -		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
> +		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);

At this point we've not yet called phy_power_on(), so I would expect the
pipe_clk_src from the PHY to be disabled and hence we wouldn't be able
to reparent the pipe_clk.

But that makes me wonder what the actual requirement here is. Do we need
just any signal on the pipe clock while initializing the controller? Or
could we simply just move the pipe_clk parent scheme to the PHY driver
as well?


Is there a case where pipe_clk needs to provide a good clock signal,
before the PHY has started to generate pipe_clk_src? Or is this scheme
simply an open coded version of the parking of shared RCGs that we see
all over the place?

Regards,
Bjorn

>  
>  	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
>  	if (ret < 0)
> @@ -1276,6 +1279,11 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
>  
>  	clk_bulk_disable_unprepare(res->num_clks, res->clks);
> +
> +	/* Set TCXO as clock source for pcie_pipe_clk_src */
> +	if (pcie->pipe_clk_need_muxing)
> +		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
> +
>  	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
>  }
>  
> @@ -1283,10 +1291,6 @@ 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 pcie_pipe_clk_src */
> -	if (pcie->pipe_clk_need_muxing)
> -		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
> -
>  	return clk_prepare_enable(res->pipe_clk);
>  }
>  
> @@ -1542,11 +1546,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	if (!pci)
>  		return -ENOMEM;
>  
> -	pm_runtime_enable(dev);
> -	ret = pm_runtime_resume_and_get(dev);
> -	if (ret < 0)
> -		goto err_pm_runtime_disable;
> -
>  	pci->dev = dev;
>  	pci->ops = &dw_pcie_ops;
>  	pp = &pci->pp;
> @@ -1563,32 +1562,29 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	pcie->pipe_clk_need_muxing = pcie_cfg->pipe_clk_need_muxing;
>  
>  	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;
> -	}
> +	if (IS_ERR(pcie->reset))
> +		return PTR_ERR(pcie->reset);
>  
>  	pcie->parf = devm_platform_ioremap_resource_byname(pdev, "parf");
> -	if (IS_ERR(pcie->parf)) {
> -		ret = PTR_ERR(pcie->parf);
> -		goto err_pm_runtime_put;
> -	}
> +	if (IS_ERR(pcie->parf))
> +		return PTR_ERR(pcie->parf);
>  
>  	pcie->elbi = devm_platform_ioremap_resource_byname(pdev, "elbi");
> -	if (IS_ERR(pcie->elbi)) {
> -		ret = PTR_ERR(pcie->elbi);
> -		goto err_pm_runtime_put;
> -	}
> +	if (IS_ERR(pcie->elbi))
> +		return PTR_ERR(pcie->elbi);
>  
>  	pcie->phy = devm_phy_optional_get(dev, "pciephy");
> -	if (IS_ERR(pcie->phy)) {
> -		ret = PTR_ERR(pcie->phy);
> -		goto err_pm_runtime_put;
> -	}
> +	if (IS_ERR(pcie->phy))
> +		return PTR_ERR(pcie->phy);
>  
>  	ret = pcie->ops->get_resources(pcie);
>  	if (ret)
> -		goto err_pm_runtime_put;
> +		return ret;
> +
> +	pm_runtime_enable(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
> +		goto err_pm_runtime_disable;
>  
>  	pp->ops = &qcom_pcie_dw_ops;
>  
> -- 
> 2.34.1
> 

Re: [PATCH 2/3] PCI: qcom: Fix pipe_clk_src reparenting

[Dmitry Baryshkov]

On Fri, 4 Feb 2022 at 07:02, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>
> On Sat 18 Dec 06:02 PST 2021, Dmitry Baryshkov wrote:
>
> > The hardware requires that pipe_clk_src is fed from TCXO when GDSC is
> > disabled. It can be fed from PHY's pipe_clk once GDSC is enabled (which
> > is what is done by the downstream driver).
> >
> > Currently code does all clk_set_parent() calls after the
> > pm_runtime_get(), so the GDSC is already enabled.
> > Implement these requirements by moving pm_runtime_*() calls after
> > get_resources (so that get_resources() can ensure that the pipe clock
> > parent is TCXO).
> >
> > Fixes: aa9c0df98c29 ("PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280")
> > Cc: Prasad Malisetty <pmaliset@codeaurora.org>
> > Cc: Stephen Boyd <swboyd@chromium.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++--------------
> >  1 file changed, 24 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 3a0f126db5a3..fbaae6f4eb18 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -1188,6 +1188,9 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
> >               res->ref_clk_src = devm_clk_get(dev, "ref");
> >               if (IS_ERR(res->ref_clk_src))
> >                       return PTR_ERR(res->ref_clk_src);
> > +
> > +             /* Ensure that the TCXO is a clock source for pcie_pipe_clk_src */
> > +             clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
> >       }
> >
> >       res->pipe_clk = devm_clk_get(dev, "pipe");
> > @@ -1208,9 +1211,9 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> >               return ret;
> >       }
> >
> > -     /* Set TCXO as clock source for pcie_pipe_clk_src */
> > +     /* Set pipe clock as clock source for pcie_pipe_clk_src */
> >       if (pcie->pipe_clk_need_muxing)
> > -             clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
> > +             clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
>
> At this point we've not yet called phy_power_on(), so I would expect the
> pipe_clk_src from the PHY to be disabled and hence we wouldn't be able
> to reparent the pipe_clk.
>
> But that makes me wonder what the actual requirement here is. Do we need
> just any signal on the pipe clock while initializing the controller? Or
> could we simply just move the pipe_clk parent scheme to the PHY driver
> as well?
>
>
> Is there a case where pipe_clk needs to provide a good clock signal,
> before the PHY has started to generate pipe_clk_src? Or is this scheme
> simply an open coded version of the parking of shared RCGs that we see
> all over the place?

According to downstream sources, the gcc_pcie_N_pipe_clk_src is
switched to rpmh_xo_clk right before turning off the gcc_pcie_N_gdsc
and reparented back to pcie_N_pipe_clk right after turning all the
respective GDSC. Comments in the source also talk about the GDSC
rather than powering up the PHY or providing signal on the
pcie_N_pipe_clk. So, the basic story is the same as we have with the
shared clocks, but the trigger is different. As we manually toggle the
GDSC, we should also park the pipe_clk_src accordingly.

>
> Regards,
> Bjorn
>
> >
> >       ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> >       if (ret < 0)
> > @@ -1276,6 +1279,11 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> >       struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> >
> >       clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > +
> > +     /* Set TCXO as clock source for pcie_pipe_clk_src */
> > +     if (pcie->pipe_clk_need_muxing)
> > +             clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
> > +
> >       regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> >  }
> >
> > @@ -1283,10 +1291,6 @@ 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 pcie_pipe_clk_src */
> > -     if (pcie->pipe_clk_need_muxing)
> > -             clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
> > -
> >       return clk_prepare_enable(res->pipe_clk);
> >  }
> >
> > @@ -1542,11 +1546,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> >       if (!pci)
> >               return -ENOMEM;
> >
> > -     pm_runtime_enable(dev);
> > -     ret = pm_runtime_resume_and_get(dev);
> > -     if (ret < 0)
> > -             goto err_pm_runtime_disable;
> > -
> >       pci->dev = dev;
> >       pci->ops = &dw_pcie_ops;
> >       pp = &pci->pp;
> > @@ -1563,32 +1562,29 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> >       pcie->pipe_clk_need_muxing = pcie_cfg->pipe_clk_need_muxing;
> >
> >       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;
> > -     }
> > +     if (IS_ERR(pcie->reset))
> > +             return PTR_ERR(pcie->reset);
> >
> >       pcie->parf = devm_platform_ioremap_resource_byname(pdev, "parf");
> > -     if (IS_ERR(pcie->parf)) {
> > -             ret = PTR_ERR(pcie->parf);
> > -             goto err_pm_runtime_put;
> > -     }
> > +     if (IS_ERR(pcie->parf))
> > +             return PTR_ERR(pcie->parf);
> >
> >       pcie->elbi = devm_platform_ioremap_resource_byname(pdev, "elbi");
> > -     if (IS_ERR(pcie->elbi)) {
> > -             ret = PTR_ERR(pcie->elbi);
> > -             goto err_pm_runtime_put;
> > -     }
> > +     if (IS_ERR(pcie->elbi))
> > +             return PTR_ERR(pcie->elbi);
> >
> >       pcie->phy = devm_phy_optional_get(dev, "pciephy");
> > -     if (IS_ERR(pcie->phy)) {
> > -             ret = PTR_ERR(pcie->phy);
> > -             goto err_pm_runtime_put;
> > -     }
> > +     if (IS_ERR(pcie->phy))
> > +             return PTR_ERR(pcie->phy);
> >
> >       ret = pcie->ops->get_resources(pcie);
> >       if (ret)
> > -             goto err_pm_runtime_put;
> > +             return ret;
> > +
> > +     pm_runtime_enable(dev);
> > +     ret = pm_runtime_resume_and_get(dev);
> > +     if (ret < 0)
> > +             goto err_pm_runtime_disable;
> >
> >       pp->ops = &qcom_pcie_dw_ops;
> >
> > --
> > 2.34.1
> >



-- 
With best wishes
Dmitry

Re: [PATCH 0/3] PCI: qcom: pipe_clk_src fixes for pcie-qcom driver

[Lorenzo Pieralisi]

On Thu, Feb 03, 2022 at 09:11:44PM +0000, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2021-12-18 06:02:20)
> > After comparing upstream and downstream Qualcomm PCIe drivers, change
> > the way the driver works with the pipe_clk_src multiplexing.
> >
> > The clock should be switched to using ref_clk (TCXO) as a parent before
> > turning the PCIE_x_GDSC power domain off and can be switched to using
> > PHY's pipe_clk after this power domain is turned on.
> >
> > Downstream driver uses regulators for the GDSC, so current approach also
> > (incorrectly) uses them. However upstream driver uses power-domain and
> > so GDSC is maintained using pm_runtime_foo() calls. Change order of
> > operations to implement these requirements.
> 
> Prasad, can you test/review this series?

Waiting for testing/review and Bjorn/Andy ACKs.

Lorenzo