| Message ID | 1626443927-32028-5-git-send-email-pmaliset@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add DT bindings and DT nodes for PCIe and PHY in SC7280 | expand |
Run this: $ git log --oneline drivers/pci/controller/dwc/pcie-qcom.c and make your subject match the style and structure (in particular, s/PCIe/PCI/). In this case, maybe something like this? PCI: qcom: Switch sc7280 gcc_pcie_1_pipe_clk_src after PHY init On Fri, Jul 16, 2021 at 07:28:47PM +0530, Prasad Malisetty wrote: > This is a new requirement for sc7280 SoC. > To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO. > after PHY initialization gcc_pcie_1_pipe_clk_src needs > to switch from TCXO to gcc_pcie_1_pipe_clk. This says what *needs* to happen, but it doesn't actually say what this patch *does*. I think it's something like: On the sc7280 SoC, the clock source for pcie_1_pipe must be the TCXO while gdsc is enabled. But after the PHY is initialized, the clock source must be switched to gcc_pcie_1_pipe_clk. On sc7280, switch gcc_pcie_1_pipe_clk_src from TCXO to gcc_pcie_1_pipe_clk after the PHY has been initialized. Nits: Rewrap to fill 75 columns or so. Add blank lines between paragraphs. Start sentences with capital letter. > Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org> > --- > 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 > @@ -166,6 +166,9 @@ struct qcom_pcie_resources_2_7_0 { > struct regulator_bulk_data supplies[2]; > struct reset_control *pci_reset; > struct clk *pipe_clk; > + struct clk *gcc_pcie_1_pipe_clk_src; > + struct clk *phy_pipe_clk; > + struct clk *ref_clk_src; > }; > > union qcom_pcie_resources { > @@ -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); Not clear why ref_clk_src is here, since it's not used anywhere. If it's not necessary here, drop it and add it in a future patch that uses it. > + } > + > 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")) Using of_device_is_compatible() follows existing style in the driver, which is good. But I'm not sure that's good style in general because it's a little repetitious and wasteful. qcom_pcie_probe() already calls of_device_get_match_data(), which does basically the same thing as of_device_is_compatible(), so I think we could take better advantage of that by augmenting struct qcom_pcie_ops with these device-specific details. Some drivers that use this strategy: drivers/pci/controller/cadence/pci-j721e.c drivers/pci/controller/dwc/pci-imx6.c drivers/pci/controller/dwc/pci-layerscape.c drivers/pci/controller/dwc/pci-layerscape-ep.c drivers/pci/controller/dwc/pcie-tegra194.c drivers/pci/controller/pci-ftpci100.c drivers/pci/controller/pcie-brcmstb.c drivers/pci/controller/pcie-mediatek.c > + clk_set_parent(res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk); > > return clk_prepare_enable(res->pipe_clk); > } > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
Quoting Prasad Malisetty (2021-07-16 06:58:47) > 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. > 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 <pmaliset@codeaurora.org> > --- > 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? 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? > > return clk_prepare_enable(res->pipe_clk); > }
On Fri 16 Jul 10:06 CDT 2021, Bjorn Helgaas wrote: > Run this: > > $ git log --oneline drivers/pci/controller/dwc/pcie-qcom.c > > and make your subject match the style and structure (in particular, > s/PCIe/PCI/). In this case, maybe something like this? > > PCI: qcom: Switch sc7280 gcc_pcie_1_pipe_clk_src after PHY init > > On Fri, Jul 16, 2021 at 07:28:47PM +0530, Prasad Malisetty wrote: > > This is a new requirement for sc7280 SoC. > > To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO. > > after PHY initialization gcc_pcie_1_pipe_clk_src needs > > to switch from TCXO to gcc_pcie_1_pipe_clk. > > This says what *needs* to happen, but it doesn't actually say what > this patch *does*. I think it's something like: > > On the sc7280 SoC, the clock source for pcie_1_pipe must be the TCXO > while gdsc is enabled. But after the PHY is initialized, the clock > source must be switched to gcc_pcie_1_pipe_clk. > > On sc7280, switch gcc_pcie_1_pipe_clk_src from TCXO to > gcc_pcie_1_pipe_clk after the PHY has been initialized. > > Nits: Rewrap to fill 75 columns or so. Add blank lines between > paragraphs. Start sentences with capital letter. > > > Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org> > > --- > > 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 > > @@ -166,6 +166,9 @@ struct qcom_pcie_resources_2_7_0 { > > struct regulator_bulk_data supplies[2]; > > struct reset_control *pci_reset; > > struct clk *pipe_clk; > > + struct clk *gcc_pcie_1_pipe_clk_src; > > + struct clk *phy_pipe_clk; > > + struct clk *ref_clk_src; > > }; > > > > union qcom_pcie_resources { > > @@ -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); > > Not clear why ref_clk_src is here, since it's not used anywhere. If > it's not necessary here, drop it and add it in a future patch that > uses it. > > > + } > > + > > 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")) > > Using of_device_is_compatible() follows existing style in the driver, > which is good. But I'm not sure that's good style in general because > it's a little repetitious and wasteful. > Following the style is good, but up until the recent sm8250 addition it was just a hack to deal with legacy platforms that we don't know the exact details about. But, all platforms I know of has the pipe_clk from the PHY fed into the pipe_clk_src mux in the gcc block and then ends up in the PCIe controller. As such, I suspect that the pipe_clk handling should be moved to the common code path of the driver and there's definitely no harm in making sure that the pipe_clk_src mux is explicitly configured on existing platforms (at least all 2.7.0 based ones). > qcom_pcie_probe() already calls of_device_get_match_data(), which does > basically the same thing as of_device_is_compatible(), so I think we > could take better advantage of that by augmenting struct qcom_pcie_ops > with these device-specific details. > I agree. Regards, Bjorn > Some drivers that use this strategy: > > drivers/pci/controller/cadence/pci-j721e.c > drivers/pci/controller/dwc/pci-imx6.c > drivers/pci/controller/dwc/pci-layerscape.c > drivers/pci/controller/dwc/pci-layerscape-ep.c > drivers/pci/controller/dwc/pcie-tegra194.c > drivers/pci/controller/pci-ftpci100.c > drivers/pci/controller/pcie-brcmstb.c > drivers/pci/controller/pcie-mediatek.c > > > + clk_set_parent(res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk); > > > > return clk_prepare_enable(res->pipe_clk); > > } > > -- > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > > a Linux Foundation Collaborative Project > >
On Fri 16 Jul 16:39 CDT 2021, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-07-16 13:31:55) > > On Fri 16 Jul 14:37 CDT 2021, Stephen Boyd wrote: > > > > > Quoting Prasad Malisetty (2021-07-16 06:58:47) > > > > 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). > > Ah I see. So there will be another patch to do the park and unpark over > suspend/resume? > That's my understanding. > > > > > > 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 <pmaliset@codeaurora.org> > > > > --- > > > > 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). > > Oh sheesh. I thought that was being done already. It looks like at least > on sdm845 that there is only one parent for this clk so we don't need to > call clk_set_parent to set it there. > I'll have to check the documentation on that... > > > > 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. > > Sure, except some platforms don't have a mux? > > > > > 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. > > I recall that qcom folks kept saying they needed to enable the > pipe_clk_src clk branch in GCC before enabling the phy. So they required > the halt skip flag so that the clk_prepare_enable() call would > effectively set the enable bit in GCC and move on without caring. Then > they could enable the upstream clk source in the phy without having to > stop halfway through to enable the branch in GCC. The whole design here > is pretty insane. > > In fact, I think we discussed this whole topic in late 2019[1] and we > concluded that we could just slam the clk on forever and deal with the > clk_set_parent() when the clk became a mux+gate instead of a pure gate. > That's exactly what I asked Prasad about, because per the description and content of this patch the parent pipe_clk_src will remain XO until the PHY is initialized. So either the PHY no longer need gcc in the loop to calibrate the pipe clock or it used to, but no longer does. Thanks for the link, we definitely should clean that up, but I think at this point it might be worth waiting a little bit longer to see what actually going to happen in the suspend/resume (system and runtime) paths... > > > > > 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... > > I guess it's fine? The GDSC will be on and the parent clk will already > be set so things are a no-op. > Yes, if the pipe_clk_src is parked nicely in late initcall, so that when the pd late init cuts the GDSC things will end up in a clean state. Regards, Bjorn > > > > > > 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... > > > > [1] https://lore.kernel.org/linux-clk/eba920f5-f5a2-53d5-2227-529b5ea99d32@codeaurora.org/
On 2021-07-17 02:08, Bjorn Andersson wrote: > On Fri 16 Jul 10:06 CDT 2021, Bjorn Helgaas wrote: > >> Run this: >> >> $ git log --oneline drivers/pci/controller/dwc/pcie-qcom.c >> >> and make your subject match the style and structure (in particular, >> s/PCIe/PCI/). In this case, maybe something like this? >> >> PCI: qcom: Switch sc7280 gcc_pcie_1_pipe_clk_src after PHY init >> >> On Fri, Jul 16, 2021 at 07:28:47PM +0530, Prasad Malisetty wrote: >> > This is a new requirement for sc7280 SoC. >> > To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO. >> > after PHY initialization gcc_pcie_1_pipe_clk_src needs >> > to switch from TCXO to gcc_pcie_1_pipe_clk. >> >> This says what *needs* to happen, but it doesn't actually say what >> this patch *does*. I think it's something like: >> >> On the sc7280 SoC, the clock source for pcie_1_pipe must be the TCXO >> while gdsc is enabled. But after the PHY is initialized, the clock >> source must be switched to gcc_pcie_1_pipe_clk. >> >> On sc7280, switch gcc_pcie_1_pipe_clk_src from TCXO to >> gcc_pcie_1_pipe_clk after the PHY has been initialized. >> >> Nits: Rewrap to fill 75 columns or so. Add blank lines between >> paragraphs. Start sentences with capital letter. >> Agree, looks good. will add more details and update the commit message in next version. >> > Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org> >> > --- >> > 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 >> > @@ -166,6 +166,9 @@ struct qcom_pcie_resources_2_7_0 { >> > struct regulator_bulk_data supplies[2]; >> > struct reset_control *pci_reset; >> > struct clk *pipe_clk; >> > + struct clk *gcc_pcie_1_pipe_clk_src; >> > + struct clk *phy_pipe_clk; >> > + struct clk *ref_clk_src; >> > }; >> > >> > union qcom_pcie_resources { >> > @@ -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); >> >> Not clear why ref_clk_src is here, since it's not used anywhere. If >> it's not necessary here, drop it and add it in a future patch that >> uses it. >> Its more useful in suspend /resume patch set. as of now we will move to suspend/resume patch set. >> > + } >> > + >> > 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")) >> >> Using of_device_is_compatible() follows existing style in the driver, >> which is good. But I'm not sure that's good style in general because >> it's a little repetitious and wasteful. >> > > Following the style is good, but up until the recent sm8250 addition it > was just a hack to deal with legacy platforms that we don't know the > exact details about. > > But, all platforms I know of has the pipe_clk from the PHY fed into the > pipe_clk_src mux in the gcc block and then ends up in the PCIe > controller. As such, I suspect that the pipe_clk handling should be > moved > to the common code path of the driver and there's definitely no harm in > making sure that the pipe_clk_src mux is explicitly configured on > existing platforms (at least all 2.7.0 based ones). > >> qcom_pcie_probe() already calls of_device_get_match_data(), which does >> basically the same thing as of_device_is_compatible(), so I think we >> could take better advantage of that by augmenting struct qcom_pcie_ops >> with these device-specific details. >> > > I agree. > > Regards, > Bjorn > >> Some drivers that use this strategy: >> >> drivers/pci/controller/cadence/pci-j721e.c >> drivers/pci/controller/dwc/pci-imx6.c >> drivers/pci/controller/dwc/pci-layerscape.c >> drivers/pci/controller/dwc/pci-layerscape-ep.c >> drivers/pci/controller/dwc/pcie-tegra194.c >> drivers/pci/controller/pci-ftpci100.c >> drivers/pci/controller/pcie-brcmstb.c >> drivers/pci/controller/pcie-mediatek.c >> >> > + clk_set_parent(res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk); >> > >> > return clk_prepare_enable(res->pipe_clk); >> > } Sure, we will make use of struct qcom_pcie_ops and add a new callback to configure pipe clk src. In coming platforms, if the platform doesn't need to configure pipe clk src, it will return as callback not defined. We will incorporate the changes in next release. >> > -- >> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> > a Linux Foundation Collaborative Project >> >
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 @@ -166,6 +166,9 @@ struct qcom_pcie_resources_2_7_0 { struct regulator_bulk_data supplies[2]; struct reset_control *pci_reset; struct clk *pipe_clk; + struct clk *gcc_pcie_1_pipe_clk_src; + struct clk *phy_pipe_clk; + struct clk *ref_clk_src; }; union qcom_pcie_resources { @@ -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); return clk_prepare_enable(res->pipe_clk); }
This is a new requirement for sc7280 SoC. To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO. 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 <pmaliset@codeaurora.org> --- drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)