LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Prasad Malisetty <pmaliset@codeaurora.org>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Bjorn Helgaas <helgaas@kernel.org>,
	agross@kernel.org, bhelgaas@google.com, robh+dt@kernel.org,
	swboyd@chromium.org, lorenzo.pieralisi@arm.com,
	svarbanov@mm-sol.com, devicetree@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org, dianders@chromium.org,
	mka@chromium.org, vbadigan@codeaurora.org,
	sallenki@codeaurora.org
Subject: Re: [PATCH v4 4/4] PCIe: qcom: Add support to control pipe clk src
Date: Tue, 20 Jul 2021 16:58:39 +0530	[thread overview]
Message-ID: <f5defd3c9f710d3b52d51657467367ac@codeaurora.org> (raw)
In-Reply-To: <YPHuWudai/FO6SMN@yoga>

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
>> >

  reply	other threads:[~2021-07-20 11:30 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-16 13:58 [PATCH v4 0/4] Add DT bindings and DT nodes for PCIe and PHY in SC7280 Prasad Malisetty
2021-07-16 13:58 ` [PATCH v4 1/4] dt-bindings: pci: qcom: Document PCIe bindings for SC720 Prasad Malisetty
2021-07-16 19:27   ` Stephen Boyd
2021-07-20  6:56     ` Prasad Malisetty
2021-07-16 13:58 ` [PATCH v4 2/4] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes Prasad Malisetty
2021-07-16 19:31   ` Stephen Boyd
2021-07-20  6:54     ` Prasad Malisetty
2021-08-02 19:23   ` Matthias Kaehlcke
2021-08-17  8:05     ` Prasad Malisetty
2021-07-16 13:58 ` [PATCH v4 3/4] arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board Prasad Malisetty
2021-07-16 19:32   ` Stephen Boyd
2021-07-20  6:57     ` Prasad Malisetty
2021-07-16 13:58 ` [PATCH v4 4/4] PCIe: qcom: Add support to control pipe clk src Prasad Malisetty
2021-07-16 15:06   ` Bjorn Helgaas
2021-07-16 20:38     ` Bjorn Andersson
2021-07-20 11:28       ` Prasad Malisetty [this message]
2021-08-09 17:09       ` Prasad Malisetty
2021-07-16 19:37   ` Stephen Boyd
2021-07-16 20:31     ` Bjorn Andersson
2021-07-16 21:39       ` Stephen Boyd
2021-07-16 22:11         ` Bjorn Andersson
2021-07-16 14:29 ` [PATCH v4 0/4] Add DT bindings and DT nodes for PCIe and PHY in SC7280 Bjorn Helgaas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f5defd3c9f710d3b52d51657467367ac@codeaurora.org \
    --to=pmaliset@codeaurora.org \
    --cc=agross@kernel.org \
    --cc=bhelgaas@google.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=helgaas@kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mka@chromium.org \
    --cc=robh+dt@kernel.org \
    --cc=sallenki@codeaurora.org \
    --cc=svarbanov@mm-sol.com \
    --cc=swboyd@chromium.org \
    --cc=vbadigan@codeaurora.org \
    --subject='Re: [PATCH v4 4/4] PCIe: qcom: Add support to control pipe clk src' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).