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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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
>