Re: [PATCH v1 4/5] PCI: qcom: Remove unnecessary pipe_clk handling

[Johan Hovold <johan@kernel.org>]

On Wed, Mar 23, 2022 at 11:50:09AM +0300, Dmitry Baryshkov wrote:
> 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")
> Cc: Prasad Malisetty <quic_pmaliset@quicinc.com>

Looks like you're blaming the wrong author and commit, but a Fixes tag
probably isn't warranted at all here.

The first commit to introduce pipe clock handling to this driver was

	d0491fc39bdd ("PCI: qcom: Add support for MSM8996 PCIe controller")

back in 2016.

This was followed up in 2019 with

	ed8cc3b1fc84 ("PCI: qcom: Add support for SDM845 PCIe controller")

which also introduced a clock imbalance by enabling the pipe clock both
in init() and post_init() but only disabling in post_deinit().

Note that this commit also started enabling the pipe clock before
powering up the PHY (i.e. the call in init()) which looks like another
bug.

This bit should be fixed separately and you can just drop the Fixes tag
from this patch. I've sent a fix here:

	https://lore.kernel.org/r/20220401101325.16983-1-johan+linaro@kernel.org

> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 50 ++------------------------
>  1 file changed, 3 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c

>  static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> @@ -1238,12 +1211,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;
> -	}
> -

Here's the imbalance.

>  	/* Wait for reset to complete, required on SM8450 */
>  	usleep_range(1000, 1500);
>  
> @@ -1298,14 +1265,7 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>  	if (pcie->cfg->pipe_clk_need_muxing)
>  		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
>  
> -	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);
> +	return 0;
>  }

Johan