Re: [PATCH v4 4/4] PCIe: qcom: Add support to control pipe clk src

On Fri 16 Jul 14:37 CDT 2021, Stephen Boyd wrote:

Quoting Prasad Malisetty (2021-07-16 06:58:47)

quoted

This is a new requirement for sc7280 SoC.
To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO.

Why? Can you add that detail here? Presumably it's something like the
GDSC needs a running clk to send a reset through the flops or something
like that.

Which presumably means that we need to "park" gcc_pcie_N_pipe_clk_src
whenever the PHY pipe is paused due to a suspend or runtime suspend.

I find this part of the commit message to primarily describing the next
patch (that is yet to be posted).

quoted

after PHY initialization gcc_pcie_1_pipe_clk_src needs
to switch from TCXO to gcc_pcie_1_pipe_clk.

Signed-off-by: Prasad Malisetty <redacted>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 8a7a300..9e0e4ab 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c

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

+       if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280")) {
+               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);
 }

@@ -1255,6 +1272,11 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
 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;
+       struct dw_pcie *pci = pcie->pci;
+       struct device *dev = pci->dev;
+
+       if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280"))
+               clk_set_parent(res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);

Is anything wrong if we call clk_set_parent() here when this driver is
running on previous SoCs where the parent is assigned via DT?

We don't assign the parent on previous platforms, we apparently just
rely on the reset value (afaict).

So I think it makes sense for all platforms to explicitly mux
pipe_clk_src to phy::pipe_clk, one the PHY is up and running.

But I was under the impression that we have the BRANCH_HALT_SKIP on the
pipe_clk because there was some sort of feedback loop to the PHY's
calibration... What this patch indicates is that we should park
pipe_clk_src onto XO at boot time, then after the PHY starts ticking we
should enable and reparent the clk_src - at which point I don't see why
we need the HALT_SKIP.

Also, shouldn't we make sure the parent is XO at driver probe time so
that powering on the GDSC works properly?

It all feels like a kludge though given that the GDSC is the one that
requires the clock to be running at XO and we're working around that in
the pcie driver instead of sticking that logic into the GDSC. What do we
do if the GDSC is already enabled out of boot instead of being the power
on reset (POR) configuration?

What happens if we boot the device out of NVME...


PS. Are we certain that it's the PCIe driver and not the PHY that should
do this dance? I really would like to see the continuation of this patch
to see the full picture...

Regards,
Bjorn

quoted

        return clk_prepare_enable(res->pipe_clk);
 }