Re: [PATCH v2 3/4] PCIe: qcom: Add support to control pipe clk mux

[Prasad Malisetty]

On 2021-06-06 07:45, Bjorn Andersson wrote:
> On Sat 05 Jun 09:40 CDT 2021, Prasad Malisetty wrote:
>
> > In PCIe driver pipe-clk mux needs to switch between pipe_clk
> > and XO for GDSC enable. This is done by setting pipe_clk mux
> > as parent of pipe_clk after phy init.
>
> But you're not switching between pipe_clk and XO, you're only making
> sure that the pipe_clk is parented by the PHY's pipe clock.
>
> Also, can you please elaborate on how this relates to the GDSC?
>
> > yes we are parenting the pipe clock by PHY's pipe clock. and also 
> > switching back to XO during suspend.

Below is the new requirement for SC7280 as part of LPM sequence.

In L1ss low power mode PHY turns the pipe clock off, so each access on 
slave AXI causes to exit from low power modes.
For completing the access, the pipe clock should be active from PHY.

In L23 mode, access on slave AXI doesn’t wake the core.
For accessing to DBI registers during L23, the SW should switch the pipe 
clock with 19.2MHz free-running clock (TCXO)
using GCC’s registers

> > Signed-off-by: Prasad Malisetty <pmaliset@xxxxxxxxxxxxxx>
> > ---
> >  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..5cbbea4 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 *pipe_clk_mux;
> > +	struct clk *pipe_ext_src;
> > +	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->pipe_clk_mux = devm_clk_get(dev, "pipe_src");
> > +		if (IS_ERR(res->pipe_clk_mux))
> > +			return PTR_ERR(res->pipe_clk_mux);
> > +
> > +		res->pipe_ext_src = devm_clk_get(dev, "pipe_ext");
> > +		if (IS_ERR(res->pipe_ext_src))
> > +			return PTR_ERR(res->pipe_ext_src);
> > +
> > +		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"))
>
> If this is something only found on 7280, you need to document (in the
> commit message at least) why this does not apply to other platforms 
> with
> this controller.
>
> Thanks,
> Bjorn
Sure, will add more info about the requirement.

> > +		clk_set_parent(res->pipe_clk_mux, res->pipe_ext_src);
> >
> >  	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