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@xxxxxxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxxxx/
