[PATCH] PCIe: qcom: Add support to control pipe clk mux

[PATCH] PCIe: qcom: Add support to control pipe clk mux

[Prasad Malisetty]

PCIe driver needs to toggle between bi_tcxo and phy pipe
clock as part of its LPM sequence. This is done by setting
pipe_clk/ref_clk_src as parent of pipe_clk_src after phy init

Dependent on below change:

	https://lore.kernel.org/patchwork/patch/1422499/

Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 8a7a300..a9f69e8 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/crc8.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
@@ -166,6 +167,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_src;
+	struct clk *pipe_ext_src;
+	struct clk *ref_clk_src;
 };
 
 union qcom_pcie_resources {
@@ -1168,7 +1172,19 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 		return ret;
 
 	res->pipe_clk = devm_clk_get(dev, "pipe");
-	return PTR_ERR_OR_ZERO(res->pipe_clk);
+	if (IS_ERR(res->pipe_clk))
+		return PTR_ERR(res->pipe_clk);
+
+	res->pipe_clk_src = devm_clk_get(dev, "pipe_src");
+	if (IS_ERR(res->pipe_clk_src))
+		return PTR_ERR(res->pipe_clk_src);
+
+	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");
+	return PTR_ERR_OR_ZERO(res->ref_clk_src);
 }
 
 static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
@@ -1255,6 +1271,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"))
+		clk_set_parent(res->pipe_clk_src, 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

Re: [PATCH] PCIe: qcom: Add support to control pipe clk mux

[Bjorn Andersson]

On Sat 08 May 19:41 CDT 2021, Prasad Malisetty wrote:

> PCIe driver needs to toggle between bi_tcxo and phy pipe
> clock as part of its LPM sequence. This is done by setting
> pipe_clk/ref_clk_src as parent of pipe_clk_src after phy init
> 
> Dependent on below change:
> 
> 	https://lore.kernel.org/patchwork/patch/1422499/

In what way is this change to the driver dependent on the addition of
the node to DT?

> 
> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 8a7a300..a9f69e8 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>

Can you help me see why this is needed?

>  #include <linux/crc8.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
> @@ -166,6 +167,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_src;
> +	struct clk *pipe_ext_src;
> +	struct clk *ref_clk_src;
>  };
>  
>  union qcom_pcie_resources {
> @@ -1168,7 +1172,19 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>  		return ret;
>  
>  	res->pipe_clk = devm_clk_get(dev, "pipe");
> -	return PTR_ERR_OR_ZERO(res->pipe_clk);
> +	if (IS_ERR(res->pipe_clk))
> +		return PTR_ERR(res->pipe_clk);
> +
> +	res->pipe_clk_src = devm_clk_get(dev, "pipe_src");
> +	if (IS_ERR(res->pipe_clk_src))

How does this not fail on existing targets?

> +		return PTR_ERR(res->pipe_clk_src);
> +
> +	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");
> +	return PTR_ERR_OR_ZERO(res->ref_clk_src);
>  }
>  
>  static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> @@ -1255,6 +1271,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"))

Why is this specific to sc7280?

> +		clk_set_parent(res->pipe_clk_src, res->pipe_ext_src);

The naming here is not obvious to me, but I think you're going to use
this to set parent of gcc_pcie_0_pipe_clk_src to pcie_0_pipe_clk?

But in the commit message you're talking about switching back and forth
between the pipe clock and tcxo, can you please help me understand where
this is happening?


PS. The new clocks should be mentioned in the binding.

Regards,
Bjorn

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

Re: [PATCH] PCIe: qcom: Add support to control pipe clk mux

[Prasad Malisetty]

On 2021-05-09 08:05, Bjorn Andersson wrote:
> On Sat 08 May 19:41 CDT 2021, Prasad Malisetty wrote:
> 
>> PCIe driver needs to toggle between bi_tcxo and phy pipe
>> clock as part of its LPM sequence. This is done by setting
>> pipe_clk/ref_clk_src as parent of pipe_clk_src after phy init
>> 
>> Dependent on below change:
>> 
>> 	https://lore.kernel.org/patchwork/patch/1422499/
> 
> In what way is this change to the driver dependent on the addition of
> the node to DT?
> 
I will move this patch to DT series patches in next version.
>> 
>> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
>> ---
>>  drivers/pci/controller/dwc/pcie-qcom.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c 
>> b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 8a7a300..a9f69e8 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -9,6 +9,7 @@
>>   */
>> 
>>  #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
> 
> Can you help me see why this is needed?
> 
Its needed for set_parent function prototype.
>>  #include <linux/crc8.h>
>>  #include <linux/delay.h>
>>  #include <linux/gpio/consumer.h>
>> @@ -166,6 +167,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_src;
>> +	struct clk *pipe_ext_src;
>> +	struct clk *ref_clk_src;
>>  };
>> 
>>  union qcom_pcie_resources {
>> @@ -1168,7 +1172,19 @@ static int qcom_pcie_get_resources_2_7_0(struct 
>> qcom_pcie *pcie)
>>  		return ret;
>> 
>>  	res->pipe_clk = devm_clk_get(dev, "pipe");
>> -	return PTR_ERR_OR_ZERO(res->pipe_clk);
>> +	if (IS_ERR(res->pipe_clk))
>> +		return PTR_ERR(res->pipe_clk);
>> +
>> +	res->pipe_clk_src = devm_clk_get(dev, "pipe_src");
>> +	if (IS_ERR(res->pipe_clk_src))
> 
> How does this not fail on existing targets?
I will add platform check in next version.
> 
>> +		return PTR_ERR(res->pipe_clk_src);
>> +
>> +	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");
>> +	return PTR_ERR_OR_ZERO(res->ref_clk_src);
>>  }
>> 
>>  static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
>> @@ -1255,6 +1271,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"))
> 
> Why is this specific to sc7280?
> 
Newer targets including sc7280 require changing pipe-clk mux to switch 
between pipe_clk and XO for GDSC enable.

>> +		clk_set_parent(res->pipe_clk_src, res->pipe_ext_src);
> 
> The naming here is not obvious to me, but I think you're going to use
> this to set parent of gcc_pcie_0_pipe_clk_src to pcie_0_pipe_clk?
> 
> But in the commit message you're talking about switching back and forth
> between the pipe clock and tcxo, can you please help me understand 
> where
> this is happening?
> 
Shall I add the naming convention for above clocks "pipe_clk_mux and 
pcie_0_pipe_clk_src" .
Switching between pipe clk and tcxo are added in suspend/resume 
callbacks. I will update the commit message
in next version.
> 
> PS. The new clocks should be mentioned in the binding.
> 
> Regards,
> Bjorn
> 
>> 
>>  	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
>>