* Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280 [not found] <CAE-0n50cnWf_3LQ6P9KMaT4dnryWW9JemP95JDZt5WE1G4mZuQ@mail.gmail.com> @ 2021-08-25 21:25 ` Bjorn Helgaas 2021-08-26 7:21 ` Prasad Malisetty 0 siblings, 1 reply; 7+ messages in thread From: Bjorn Helgaas @ 2021-08-25 21:25 UTC (permalink / raw) To: Stephen Boyd Cc: Prasad Malisetty, agross, bhelgaas, bjorn.andersson, lorenzo.pieralisi, robh+dt, svarbanov, devicetree, linux-arm-msm, linux-usb, linux-kernel, dianders, mka, vbadigan, sallenki, manivannan.sadhasivam, linux-pci [+cc linux-pci; patches to drivers/pci/ should always be cc'd there] On Wed, Aug 25, 2021 at 07:30:09PM +0000, Stephen Boyd wrote: > 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); > > >> + } > > > > > > 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. I'm missing some context here, so I'm not exactly sure what your question is, Prasad, but IMO drivers generally should not need to use of_device_is_compatible() if they've already called of_device_get_match_data() (as qcom_pcie_probe() has). of_device_is_compatible() does basically the same work of looking for a match in qcom_pcie_match[] that of_device_get_match_data() does, so it seems pointless to repeat it. I am a little confused because while [1] adds "qcom,pcie-sc7280" to qcom,pcie.txt, I don't see a patch that adds it to qcom_pcie_match[]. Bjorn [1] https://lore.kernel.org/linux-arm-msm/1628568516-24155-2-git-send-email-pmaliset@codeaurora.org/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280 2021-08-25 21:25 ` [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280 Bjorn Helgaas @ 2021-08-26 7:21 ` Prasad Malisetty 2021-08-26 12:37 ` Rob Herring 0 siblings, 1 reply; 7+ messages in thread From: Prasad Malisetty @ 2021-08-26 7:21 UTC (permalink / raw) To: Bjorn Helgaas Cc: Stephen Boyd, agross, bhelgaas, bjorn.andersson, lorenzo.pieralisi, robh+dt, svarbanov, devicetree, linux-arm-msm, linux-usb, linux-kernel, dianders, mka, vbadigan, sallenki, manivannan.sadhasivam, linux-pci On 2021-08-26 02:55, Bjorn Helgaas wrote: > [+cc linux-pci; patches to drivers/pci/ should always be cc'd there] > > On Wed, Aug 25, 2021 at 07:30:09PM +0000, Stephen Boyd wrote: >> 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); >> > >> + } >> > > >> > > 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. > > I'm missing some context here, so I'm not exactly sure what your > question is, Prasad, but IMO drivers generally should not need to use > of_device_is_compatible() if they've already called > of_device_get_match_data() (as qcom_pcie_probe() has). > > of_device_is_compatible() does basically the same work of looking for > a match in qcom_pcie_match[] that of_device_get_match_data() does, so > it seems pointless to repeat it. > > I am a little confused because while [1] adds "qcom,pcie-sc7280" to > qcom,pcie.txt, I don't see a patch that adds it to qcom_pcie_match[]. > > Bjorn > Hi Bjorn, I agree on your point, but the main reason is to use compatible in get_resources_2_7_0 is same hardware version. For SM8250 & SC7280 platforms, the hw version is same. Since we can't have a separate ops for SC7280, we are using compatible method in get_resources_2_7_0 to differentiate SM8250 and SC7280. Thanks -Prasad > [1] > https://lore.kernel.org/linux-arm-msm/1628568516-24155-2-git-send-email-pmaliset@codeaurora.org/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280 2021-08-26 7:21 ` Prasad Malisetty @ 2021-08-26 12:37 ` Rob Herring 2021-08-31 6:37 ` Prasad Malisetty 0 siblings, 1 reply; 7+ messages in thread From: Rob Herring @ 2021-08-26 12:37 UTC (permalink / raw) To: Prasad Malisetty Cc: Bjorn Helgaas, Stephen Boyd, agross, bhelgaas, bjorn.andersson, lorenzo.pieralisi, svarbanov, devicetree, linux-arm-msm, linux-usb, linux-kernel, dianders, mka, vbadigan, sallenki, manivannan.sadhasivam, linux-pci On Thu, Aug 26, 2021 at 2:22 AM Prasad Malisetty <pmaliset@codeaurora.org> wrote: > > On 2021-08-26 02:55, Bjorn Helgaas wrote: > > [+cc linux-pci; patches to drivers/pci/ should always be cc'd there] > > > > On Wed, Aug 25, 2021 at 07:30:09PM +0000, Stephen Boyd wrote: > >> 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); > >> > >> + } > >> > > > >> > > 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. > > > > I'm missing some context here, so I'm not exactly sure what your > > question is, Prasad, but IMO drivers generally should not need to use > > of_device_is_compatible() if they've already called > > of_device_get_match_data() (as qcom_pcie_probe() has). > > > > of_device_is_compatible() does basically the same work of looking for > > a match in qcom_pcie_match[] that of_device_get_match_data() does, so > > it seems pointless to repeat it. +1 > > I am a little confused because while [1] adds "qcom,pcie-sc7280" to > > qcom,pcie.txt, I don't see a patch that adds it to qcom_pcie_match[]. Either that's missing or there's a fallback to 8250 that's not documented. > > > > Bjorn > > > Hi Bjorn, > > I agree on your point, but the main reason is to use compatible in > get_resources_2_7_0 is same hardware version. For SM8250 & SC7280 > platforms, the hw version is same. Since we can't have a separate ops > for SC7280, we are using compatible method in get_resources_2_7_0 to > differentiate SM8250 and SC7280. Then fix the match data to be not just ops, but ops and the flag you need here. Rob ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280 2021-08-26 12:37 ` Rob Herring @ 2021-08-31 6:37 ` Prasad Malisetty 2021-08-31 15:37 ` Bjorn Helgaas 0 siblings, 1 reply; 7+ messages in thread From: Prasad Malisetty @ 2021-08-31 6:37 UTC (permalink / raw) To: Rob Herring Cc: Bjorn Helgaas, Stephen Boyd, agross, bhelgaas, bjorn.andersson, lorenzo.pieralisi, svarbanov, devicetree, linux-arm-msm, linux-usb, linux-kernel, dianders, mka, vbadigan, sallenki, manivannan.sadhasivam, linux-pci On 2021-08-26 18:07, Rob Herring wrote: > On Thu, Aug 26, 2021 at 2:22 AM Prasad Malisetty > <pmaliset@codeaurora.org> wrote: >> >> On 2021-08-26 02:55, Bjorn Helgaas wrote: >> > [+cc linux-pci; patches to drivers/pci/ should always be cc'd there] >> > >> > On Wed, Aug 25, 2021 at 07:30:09PM +0000, Stephen Boyd wrote: >> >> 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); >> >> > >> + } >> >> > > >> >> > > 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. >> > >> > I'm missing some context here, so I'm not exactly sure what your >> > question is, Prasad, but IMO drivers generally should not need to use >> > of_device_is_compatible() if they've already called >> > of_device_get_match_data() (as qcom_pcie_probe() has). >> > >> > of_device_is_compatible() does basically the same work of looking for >> > a match in qcom_pcie_match[] that of_device_get_match_data() does, so >> > it seems pointless to repeat it. > > +1 > >> > I am a little confused because while [1] adds "qcom,pcie-sc7280" to >> > qcom,pcie.txt, I don't see a patch that adds it to qcom_pcie_match[]. > > Either that's missing or there's a fallback to 8250 that's not > documented. >> > >> > Bjorn >> > >> Hi Bjorn, >> >> I agree on your point, but the main reason is to use compatible in >> get_resources_2_7_0 is same hardware version. For SM8250 & SC7280 >> platforms, the hw version is same. Since we can't have a separate ops >> for SC7280, we are using compatible method in get_resources_2_7_0 to >> differentiate SM8250 and SC7280. > > Then fix the match data to be not just ops, but ops and the flag you > need here. > > Rob Hi Rob, Thanks for your review comments and inputs . This difference is not universal across all the platforms but instead this is specific to SC7280. Hence it make sense to use compatible other than going for a flag. Thanks -Prasad ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280 2021-08-31 6:37 ` Prasad Malisetty @ 2021-08-31 15:37 ` Bjorn Helgaas 2021-09-09 17:51 ` Prasad Malisetty 0 siblings, 1 reply; 7+ messages in thread From: Bjorn Helgaas @ 2021-08-31 15:37 UTC (permalink / raw) To: Prasad Malisetty Cc: Rob Herring, Stephen Boyd, agross, bhelgaas, bjorn.andersson, lorenzo.pieralisi, svarbanov, devicetree, linux-arm-msm, linux-usb, linux-kernel, dianders, mka, vbadigan, sallenki, manivannan.sadhasivam, linux-pci On Tue, Aug 31, 2021 at 12:07:30PM +0530, Prasad Malisetty wrote: > On 2021-08-26 18:07, Rob Herring wrote: > > On Thu, Aug 26, 2021 at 2:22 AM Prasad Malisetty > > <pmaliset@codeaurora.org> wrote: > > > > > > On 2021-08-26 02:55, Bjorn Helgaas wrote: > > > > [+cc linux-pci; patches to drivers/pci/ should always be cc'd there] > > > > > > > > On Wed, Aug 25, 2021 at 07:30:09PM +0000, Stephen Boyd wrote: > > > >> 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); > > > >> > >> + } > > > >> > > > > > >> > > 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. > > > > > > > > I'm missing some context here, so I'm not exactly sure what your > > > > question is, Prasad, but IMO drivers generally should not need to use > > > > of_device_is_compatible() if they've already called > > > > of_device_get_match_data() (as qcom_pcie_probe() has). > > > > > > > > of_device_is_compatible() does basically the same work of looking for > > > > a match in qcom_pcie_match[] that of_device_get_match_data() does, so > > > > it seems pointless to repeat it. > > > > +1 > > > > > > I am a little confused because while [1] adds "qcom,pcie-sc7280" to > > > > qcom,pcie.txt, I don't see a patch that adds it to qcom_pcie_match[]. > > > > Either that's missing or there's a fallback to 8250 that's not > > documented. > > > > > I agree on your point, but the main reason is to use compatible in > > > get_resources_2_7_0 is same hardware version. For SM8250 & SC7280 > > > platforms, the hw version is same. Since we can't have a separate ops > > > for SC7280, we are using compatible method in get_resources_2_7_0 to > > > differentiate SM8250 and SC7280. > > > > Then fix the match data to be not just ops, but ops and the flag you > > need here. > > This difference is not universal across all the platforms but instead this > is specific to SC7280. > Hence it make sense to use compatible other than going for a flag. There's no reason your qcom_pcie_match[].data pointers need to be strictly based on the hardware version. You can do something like what pcie-brcmstb.c does, e.g., struct pcie_cfg_data { struct qcom_pcie_ops *ops; unsigned int pipe_mux:1; }; static const struct pcie_cfg_data sm8250_cfg = { .ops = &ops_1_9_0, }; static const struct pcie_cfg_data sc7280_cfg = { .ops = &ops_1_9_0, .pipe_mux = 1, }; static const struct of_device_id qcom_pcie_match[] = { { .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg }, { .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg }, }; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280 2021-08-31 15:37 ` Bjorn Helgaas @ 2021-09-09 17:51 ` Prasad Malisetty 2021-09-09 18:08 ` Bjorn Helgaas 0 siblings, 1 reply; 7+ messages in thread From: Prasad Malisetty @ 2021-09-09 17:51 UTC (permalink / raw) To: Bjorn Helgaas Cc: Rob Herring, Stephen Boyd, agross, bhelgaas, bjorn.andersson, lorenzo.pieralisi, svarbanov, devicetree, linux-arm-msm, linux-usb, linux-kernel, dianders, mka, vbadigan, sallenki, manivannan.sadhasivam, linux-pci On 2021-08-31 21:07, Bjorn Helgaas wrote: > On Tue, Aug 31, 2021 at 12:07:30PM +0530, Prasad Malisetty wrote: >> On 2021-08-26 18:07, Rob Herring wrote: >> > On Thu, Aug 26, 2021 at 2:22 AM Prasad Malisetty >> > <pmaliset@codeaurora.org> wrote: >> > > >> > > On 2021-08-26 02:55, Bjorn Helgaas wrote: >> > > > [+cc linux-pci; patches to drivers/pci/ should always be cc'd there] >> > > > >> > > > On Wed, Aug 25, 2021 at 07:30:09PM +0000, Stephen Boyd wrote: >> > > >> 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); >> > > >> > >> + } >> > > >> > > >> > > >> > > 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. >> > > > >> > > > I'm missing some context here, so I'm not exactly sure what your >> > > > question is, Prasad, but IMO drivers generally should not need to use >> > > > of_device_is_compatible() if they've already called >> > > > of_device_get_match_data() (as qcom_pcie_probe() has). >> > > > >> > > > of_device_is_compatible() does basically the same work of looking for >> > > > a match in qcom_pcie_match[] that of_device_get_match_data() does, so >> > > > it seems pointless to repeat it. >> > >> > +1 >> > >> > > > I am a little confused because while [1] adds "qcom,pcie-sc7280" to >> > > > qcom,pcie.txt, I don't see a patch that adds it to qcom_pcie_match[]. >> > >> > Either that's missing or there's a fallback to 8250 that's not >> > documented. >> > >> > > I agree on your point, but the main reason is to use compatible in >> > > get_resources_2_7_0 is same hardware version. For SM8250 & SC7280 >> > > platforms, the hw version is same. Since we can't have a separate ops >> > > for SC7280, we are using compatible method in get_resources_2_7_0 to >> > > differentiate SM8250 and SC7280. >> > >> > Then fix the match data to be not just ops, but ops and the flag you >> > need here. >> >> This difference is not universal across all the platforms but instead >> this >> is specific to SC7280. >> Hence it make sense to use compatible other than going for a flag. > > There's no reason your qcom_pcie_match[].data pointers need to be > strictly based on the hardware version. > > You can do something like what pcie-brcmstb.c does, e.g., > > struct pcie_cfg_data { > struct qcom_pcie_ops *ops; > unsigned int pipe_mux:1; > }; > > static const struct pcie_cfg_data sm8250_cfg = { > .ops = &ops_1_9_0, > }; > > static const struct pcie_cfg_data sc7280_cfg = { > .ops = &ops_1_9_0, > .pipe_mux = 1, > }; > > static const struct of_device_id qcom_pcie_match[] = { > { .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg }, > { .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg }, > }; Hi Bjorn, Thanks for the detailed example. I have one quick query, If we use above approach, we should change platform data reading in PCIe probe to differentiate remaining platforms right. expect SM8250 and SC7280 all other platforms are using same qcom_pcie_ops structure pointer as data. Kindly correct me if my understanding is wrong. Just posted v6 patch series with same compatible approach as of now. I will go through your example and update further. Thanks -Prasad ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280 2021-09-09 17:51 ` Prasad Malisetty @ 2021-09-09 18:08 ` Bjorn Helgaas 0 siblings, 0 replies; 7+ messages in thread From: Bjorn Helgaas @ 2021-09-09 18:08 UTC (permalink / raw) To: Prasad Malisetty Cc: Rob Herring, Stephen Boyd, agross, bhelgaas, bjorn.andersson, lorenzo.pieralisi, svarbanov, devicetree, linux-arm-msm, linux-usb, linux-kernel, dianders, mka, vbadigan, sallenki, manivannan.sadhasivam, linux-pci On Thu, Sep 09, 2021 at 11:21:22PM +0530, Prasad Malisetty wrote: > On 2021-08-31 21:07, Bjorn Helgaas wrote: > > On Tue, Aug 31, 2021 at 12:07:30PM +0530, Prasad Malisetty wrote: > > > On 2021-08-26 18:07, Rob Herring wrote: > > > > On Thu, Aug 26, 2021 at 2:22 AM Prasad Malisetty > > > > <pmaliset@codeaurora.org> wrote: > > > > > > > > > > On 2021-08-26 02:55, Bjorn Helgaas wrote: > > > > > > [+cc linux-pci; patches to drivers/pci/ should always be cc'd there] > > > > > > > > > > > > On Wed, Aug 25, 2021 at 07:30:09PM +0000, Stephen Boyd wrote: > > > > > >> 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); > > > > > >> > >> + } > > > > > >> > > > > > > > >> > > 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. > > > > > > > > > > > > I'm missing some context here, so I'm not exactly sure > > > > > > what your question is, Prasad, but IMO drivers generally > > > > > > should not need to use of_device_is_compatible() if > > > > > > they've already called of_device_get_match_data() (as > > > > > > qcom_pcie_probe() has). > > > > > > > > > > > > of_device_is_compatible() does basically the same work of > > > > > > looking for a match in qcom_pcie_match[] that > > > > > > of_device_get_match_data() does, so it seems pointless to > > > > > > repeat it. > > > > > > > > +1 > > > > > > > > > > I am a little confused because while [1] adds "qcom,pcie-sc7280" to > > > > > > qcom,pcie.txt, I don't see a patch that adds it to qcom_pcie_match[]. > > > > > > > > Either that's missing or there's a fallback to 8250 that's not > > > > documented. > > > > > > > > > I agree on your point, but the main reason is to use compatible in > > > > > get_resources_2_7_0 is same hardware version. For SM8250 & SC7280 > > > > > platforms, the hw version is same. Since we can't have a separate ops > > > > > for SC7280, we are using compatible method in get_resources_2_7_0 to > > > > > differentiate SM8250 and SC7280. > > > > > > > > Then fix the match data to be not just ops, but ops and the flag you > > > > need here. > > > > > > This difference is not universal across all the platforms but > > > instead this is specific to SC7280. Hence it make sense to use > > > compatible other than going for a flag. > > > > There's no reason your qcom_pcie_match[].data pointers need to be > > strictly based on the hardware version. > > > > You can do something like what pcie-brcmstb.c does, e.g., > > > > struct pcie_cfg_data { > > struct qcom_pcie_ops *ops; > > unsigned int pipe_mux:1; > > }; > > > > static const struct pcie_cfg_data sm8250_cfg = { > > .ops = &ops_1_9_0, > > }; > > > > static const struct pcie_cfg_data sc7280_cfg = { > > .ops = &ops_1_9_0, > > .pipe_mux = 1, > > }; > > > > static const struct of_device_id qcom_pcie_match[] = { > > { .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg }, > > { .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg }, > > }; > > I have one quick query, If we use above approach, we should change platform > data reading in PCIe probe to differentiate remaining platforms right. > expect SM8250 and SC7280 all other platforms are using same qcom_pcie_ops > structure pointer as data. Yes. of_device_get_match_data() must return the same type of pointer (in the example above, "struct pcie_cfg_data *") for all platforms. So you would have to add a struct for each of them, and each struct would contain the ops pointer (&ops_1_0_0, &ops_2_1_0, etc). Thanks for working on this! Bjorn ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-09-09 18:08 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CAE-0n50cnWf_3LQ6P9KMaT4dnryWW9JemP95JDZt5WE1G4mZuQ@mail.gmail.com>
2021-08-25 21:25 ` [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280 Bjorn Helgaas
2021-08-26 7:21 ` Prasad Malisetty
2021-08-26 12:37 ` Rob Herring
2021-08-31 6:37 ` Prasad Malisetty
2021-08-31 15:37 ` Bjorn Helgaas
2021-09-09 17:51 ` Prasad Malisetty
2021-09-09 18:08 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).