diff mbox series

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

Message ID 20211218140223.500390-3-dmitry.baryshkov@linaro.org
State New
Headers show
Series PCI: qcom: pipe_clk_src fixes for pcie-qcom driver | expand

Commit Message

Dmitry Baryshkov Dec. 18, 2021, 2:02 p.m. UTC
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(-)

Comments

Bjorn Andersson Feb. 4, 2022, 4:03 a.m. UTC | #1
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
>
Dmitry Baryshkov Feb. 4, 2022, 8:36 a.m. UTC | #2
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
> >
diff mbox series

Patch

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;