| Message ID | 1628568516-24155-5-git-send-email-pmaliset@codeaurora.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add DT bindings and DT nodes for PCIe and PHY in SC7280 | expand |
Quoting Prasad Malisetty (2021-08-09 21:08:36) > On the SC7280, By default the clock source for pcie_1_pipe is > TCXO for gdsc enable. But after the PHY is initialized, the clock > source must be switched to gcc_pcie_1_pipe_clk from TCXO. > > Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 8a7a300..39e3b21 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -1167,6 +1169,16 @@ 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->pipe_clk = devm_clk_get(dev, "pipe"); > return PTR_ERR_OR_ZERO(res->pipe_clk); > } > @@ -1255,6 +1267,12 @@ 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; > + struct device_node *node = dev->of_node; > + > + if (of_property_read_bool(node, "pipe-clk-source-switch")) This can be straightline code. If gcc_pcie_1_pipe_clk_src is NULL, calling clk_set_parent() on it is a nop, return 0, so drop the property check and only assign the clk pointer if it needs to be done. > + clk_set_parent(res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk); Please check the return value and fail if it fails to set the parent. I'd also prefer a comment indicating that we have to set the parent because the GDSC must be enabled with the clk at XO speed. The DT should probably also have an assigned clock parent of XO so when the driver probes it is set to XO parent for gdsc enable and then this driver code can change the parent to the phy pipe clk. > > return clk_prepare_enable(res->pipe_clk); > }
On 2021-08-12 11:41, Manivannan Sadhasivam wrote: > On Tue, Aug 10, 2021 at 09:38:36AM +0530, Prasad Malisetty wrote: >> On the SC7280, By default the clock source for pcie_1_pipe is >> TCXO for gdsc enable. But after the PHY is initialized, the clock >> source must be switched to gcc_pcie_1_pipe_clk from TCXO. >> >> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org> >> --- >> drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c >> b/drivers/pci/controller/dwc/pcie-qcom.c >> index 8a7a300..39e3b21 100644 >> --- a/drivers/pci/controller/dwc/pcie-qcom.c >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c >> @@ -166,6 +166,8 @@ 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; >> }; >> >> union qcom_pcie_resources { >> @@ -1167,6 +1169,16 @@ 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->pipe_clk = devm_clk_get(dev, "pipe"); >> return PTR_ERR_OR_ZERO(res->pipe_clk); >> } >> @@ -1255,6 +1267,12 @@ 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; >> + struct device_node *node = dev->of_node; >> + >> + if (of_property_read_bool(node, "pipe-clk-source-switch")) > > Wondering why you didn't use the compatible here as well. This will > break if the > property exist but the clocks are not. > > Thanks, > Mani > Hi Mani, In earlier versions we used compatible method here as well, but in v5 replaced compatible with new boolean flag. In recent comments as Stephen suggested, its straight forward approach. if src pointer is NULL, clk_set_parent return 0 and nop I will remove both compatible and property read approach and update the change in next version. >> + 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 2021-08-11 01:07, Stephen Boyd wrote: > Quoting Prasad Malisetty (2021-08-09 21:08:36) >> On the SC7280, By default the clock source for pcie_1_pipe is >> TCXO for gdsc enable. But after the PHY is initialized, the clock >> source must be switched to gcc_pcie_1_pipe_clk from TCXO. >> >> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org> >> --- >> drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c >> b/drivers/pci/controller/dwc/pcie-qcom.c >> index 8a7a300..39e3b21 100644 >> --- a/drivers/pci/controller/dwc/pcie-qcom.c >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c >> @@ -1167,6 +1169,16 @@ 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->pipe_clk = devm_clk_get(dev, "pipe"); >> return PTR_ERR_OR_ZERO(res->pipe_clk); >> } >> @@ -1255,6 +1267,12 @@ 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; >> + struct device_node *node = dev->of_node; >> + >> + if (of_property_read_bool(node, "pipe-clk-source-switch")) > > This can be straightline code. If gcc_pcie_1_pipe_clk_src is NULL, > calling clk_set_parent() on it is a nop, return 0, so drop the property > check and only assign the clk pointer if it needs to be done. > >> + clk_set_parent(res->gcc_pcie_1_pipe_clk_src, >> res->phy_pipe_clk); > > Please check the return value and fail if it fails to set the parent. > I'd also prefer a comment indicating that we have to set the parent > because the GDSC must be enabled with the clk at XO speed. The DT > should > probably also have an assigned clock parent of XO so when the driver > probes it is set to XO parent for gdsc enable and then this driver code > can change the parent to the phy pipe clk. > >> >> return clk_prepare_enable(res->pipe_clk); >> } Hi Stephen, Thanks for your review and inputs. Yes, clk_set_parent function returning NULL if src pointer is NULL. we can call clk_set_parent function without any check. I will validate and incorporate the changes in next version. Thanks -Prasad
On 2021-08-10 09:38, Prasad Malisetty wrote: > On the SC7280, By default the clock source for pcie_1_pipe is > TCXO for gdsc enable. But after the PHY is initialized, the clock > source must be switched to gcc_pcie_1_pipe_clk from TCXO. > > Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c > b/drivers/pci/controller/dwc/pcie-qcom.c > index 8a7a300..39e3b21 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -166,6 +166,8 @@ 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; > }; > > union qcom_pcie_resources { > @@ -1167,6 +1169,16 @@ 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); > + } > + Hi All, Greetings! I would like to check is there any other better approach instead of compatible method here as well or is it fine to use compatible method. Thanks -Prasad > res->pipe_clk = devm_clk_get(dev, "pipe"); > return PTR_ERR_OR_ZERO(res->pipe_clk); > } > @@ -1255,6 +1267,12 @@ 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; > + struct device_node *node = dev->of_node; > + > + if (of_property_read_bool(node, "pipe-clk-source-switch")) > + clk_set_parent(res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk); > > return clk_prepare_enable(res->pipe_clk); > }
On 2021-08-17 22:56, Prasad Malisetty wrote: > On 2021-08-10 09:38, Prasad Malisetty wrote: >> On the SC7280, By default the clock source for pcie_1_pipe is >> TCXO for gdsc enable. But after the PHY is initialized, the clock >> source must be switched to gcc_pcie_1_pipe_clk from TCXO. >> >> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org> >> --- >> drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c >> b/drivers/pci/controller/dwc/pcie-qcom.c >> index 8a7a300..39e3b21 100644 >> --- a/drivers/pci/controller/dwc/pcie-qcom.c >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c >> @@ -166,6 +166,8 @@ 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; >> }; >> >> union qcom_pcie_resources { >> @@ -1167,6 +1169,16 @@ 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); >> + } >> + > > Hi All, > > Greetings! > > I would like to check is there any other better approach instead of > compatible method here as well or is it fine to use compatible method. > > Thanks > -Prasad > >> res->pipe_clk = devm_clk_get(dev, "pipe"); >> return PTR_ERR_OR_ZERO(res->pipe_clk); >> } >> @@ -1255,6 +1267,12 @@ 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; >> + struct device_node *node = dev->of_node; >> + >> + if (of_property_read_bool(node, "pipe-clk-source-switch")) >> + clk_set_parent(res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk); >> >> return clk_prepare_enable(res->pipe_clk); >> } Hi, Kindly provide your inputs and confirmation on latest queries, I will share new patch version. Thanks -Prasad
Quoting Prasad Malisetty (2021-08-24 01:10:48) > On 2021-08-17 22:56, Prasad Malisetty wrote: > > On 2021-08-10 09:38, Prasad Malisetty wrote: > >> On the SC7280, By default the clock source for pcie_1_pipe is > >> TCXO for gdsc enable. But after the PHY is initialized, the clock > >> source must be switched to gcc_pcie_1_pipe_clk from TCXO. > >> > >> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org> > >> --- > >> drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ > >> 1 file changed, 18 insertions(+) > >> > >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c > >> b/drivers/pci/controller/dwc/pcie-qcom.c > >> index 8a7a300..39e3b21 100644 > >> --- a/drivers/pci/controller/dwc/pcie-qcom.c > >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c > >> @@ -166,6 +166,8 @@ 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; > >> }; > >> > >> union qcom_pcie_resources { > >> @@ -1167,6 +1169,16 @@ 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); > >> + } > >> + > > > > Hi All, > > > > Greetings! > > > > I would like to check is there any other better approach instead of > > compatible method here as well or is it fine to use compatible method. > > I'd prefer the compatible method. If nobody is responding then it's best to just resend the patches with the approach you prefer instead of waiting for someone to respond to a review comment.
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 8a7a300..39e3b21 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -166,6 +166,8 @@ 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; }; union qcom_pcie_resources { @@ -1167,6 +1169,16 @@ 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->pipe_clk = devm_clk_get(dev, "pipe"); return PTR_ERR_OR_ZERO(res->pipe_clk); } @@ -1255,6 +1267,12 @@ 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; + struct device_node *node = dev->of_node; + + if (of_property_read_bool(node, "pipe-clk-source-switch")) + clk_set_parent(res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk); return clk_prepare_enable(res->pipe_clk); }
On the SC7280, By default the clock source for pcie_1_pipe is TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched to gcc_pcie_1_pipe_clk from TCXO. Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org> --- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)