[PATCH 0/3] Remove clocks dependency from SCM driver

[PATCH 0/3] Remove clocks dependency from SCM driver

[Sarangdhar Joshi]

On earlier chiptsets (APQ8064, MSM8660, MSM8690, MSM8916,
APQ8084, MSM8974) crypto operations of TZ were depends on crypto
clocks controlled by users/clients. However on MSM8996 crypto clocks
control is handled internally in TZ itself. The current series of
patches handle this clock dependency in SCM driver.

Sarangdhar Joshi (3):
  dt-bindings: firmware: scm: Add MSM8996 DT bindings
  firmware: qcom: scm: Remove core, iface and bus clocks dependency
  firmware: qcom: scm: Return PTR_ERR when devm_clk_get fails

 .../devicetree/bindings/firmware/qcom,scm.txt      |  2 +
 drivers/firmware/qcom_scm.c                        | 65 +++++++++++++++-------
 2 files changed, 48 insertions(+), 19 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/3] dt-bindings: firmware: scm: Add MSM8996 DT bindings

[Sarangdhar Joshi]

Add SCM DT bindings for Qualcomm's MSM8996 platform.

Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org>
---
 Documentation/devicetree/bindings/firmware/qcom,scm.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
index 3b4436e..20f26fb 100644
--- a/Documentation/devicetree/bindings/firmware/qcom,scm.txt
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
@@ -10,8 +10,10 @@ Required properties:
  * "qcom,scm-apq8064" for APQ8064 platforms
  * "qcom,scm-msm8660" for MSM8660 platforms
  * "qcom,scm-msm8690" for MSM8690 platforms
+ * "qcom,scm-msm8996" for MSM8996 platforms
  * "qcom,scm" for later processors (MSM8916, APQ8084, MSM8974, etc)
 - clocks: One to three clocks may be required based on compatible.
+ * No clock required for "qcom,scm-msm8996"
  * Only core clock required for "qcom,scm-apq8064", "qcom,scm-msm8660", and "qcom,scm-msm8960"
  * Core, iface, and bus clocks required for "qcom,scm"
 - clock-names: Must contain "core" for the core clock, "iface" for the interface
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/3] dt-bindings: firmware: scm: Add MSM8996 DT bindings

[Rob Herring]

On Fri, Oct 28, 2016 at 05:08:47PM -0700, Sarangdhar Joshi wrote:
> Add SCM DT bindings for Qualcomm's MSM8996 platform.
> 
> Signed-off-by: Sarangdhar Joshi <spjoshi-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/firmware/qcom,scm.txt | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] firmware: qcom: scm: Remove core, iface and bus clocks dependency

[Sarangdhar Joshi]

Core, iface and bus clocks are not required to be voted from SCM
driver for some of the Qualcomm chipsets. Remove dependency on
these clocks from driver.

Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org>
---
 drivers/firmware/qcom_scm.c | 65 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 46 insertions(+), 19 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index d79fecd..010a350 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -28,6 +28,10 @@
 
 #include "qcom_scm.h"
 
+#define SCM_HAS_CORE_CLK	BIT(0)
+#define SCM_HAS_IFACE_CLK	BIT(1)
+#define SCM_HAS_BUS_CLK		BIT(2)
+
 struct qcom_scm {
 	struct device *dev;
 	struct clk *core_clk;
@@ -380,33 +384,43 @@ EXPORT_SYMBOL(qcom_scm_is_available);
 static int qcom_scm_probe(struct platform_device *pdev)
 {
 	struct qcom_scm *scm;
+	uint64_t clks;
 	int ret;
 
 	scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
 	if (!scm)
 		return -ENOMEM;
 
-	scm->core_clk = devm_clk_get(&pdev->dev, "core");
-	if (IS_ERR(scm->core_clk)) {
-		if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
-			return PTR_ERR(scm->core_clk);
+	clks = (uint64_t)of_device_get_match_data(&pdev->dev);
+	if (clks & SCM_HAS_CORE_CLK) {
+		scm->core_clk = devm_clk_get(&pdev->dev, "core");
+		if (IS_ERR(scm->core_clk)) {
+			if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
+				return PTR_ERR(scm->core_clk);
 
-		scm->core_clk = NULL;
+			scm->core_clk = NULL;
+		}
 	}
 
 	if (of_device_is_compatible(pdev->dev.of_node, "qcom,scm")) {
-		scm->iface_clk = devm_clk_get(&pdev->dev, "iface");
-		if (IS_ERR(scm->iface_clk)) {
-			if (PTR_ERR(scm->iface_clk) != -EPROBE_DEFER)
-				dev_err(&pdev->dev, "failed to acquire iface clk\n");
-			return PTR_ERR(scm->iface_clk);
+		if (clks & SCM_HAS_IFACE_CLK) {
+			scm->iface_clk = devm_clk_get(&pdev->dev, "iface");
+			if (IS_ERR(scm->iface_clk)) {
+				if (PTR_ERR(scm->iface_clk) != -EPROBE_DEFER)
+					dev_err(&pdev->dev,
+						"failed to acquire iface clk\n");
+				return PTR_ERR(scm->iface_clk);
+			}
 		}
 
-		scm->bus_clk = devm_clk_get(&pdev->dev, "bus");
-		if (IS_ERR(scm->bus_clk)) {
-			if (PTR_ERR(scm->bus_clk) != -EPROBE_DEFER)
-				dev_err(&pdev->dev, "failed to acquire bus clk\n");
-			return PTR_ERR(scm->bus_clk);
+		if (clks & SCM_HAS_BUS_CLK) {
+			scm->bus_clk = devm_clk_get(&pdev->dev, "bus");
+			if (IS_ERR(scm->bus_clk)) {
+				if (PTR_ERR(scm->bus_clk) != -EPROBE_DEFER)
+					dev_err(&pdev->dev,
+						"failed to acquire bus clk\n");
+				return PTR_ERR(scm->bus_clk);
+			}
 		}
 	}
 
@@ -429,10 +443,23 @@ static int qcom_scm_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id qcom_scm_dt_match[] = {
-	{ .compatible = "qcom,scm-apq8064",},
-	{ .compatible = "qcom,scm-msm8660",},
-	{ .compatible = "qcom,scm-msm8960",},
-	{ .compatible = "qcom,scm",},
+	{ .compatible = "qcom,scm-apq8064",
+	  .data = (void *) SCM_HAS_CORE_CLK,
+	},
+	{ .compatible = "qcom,scm-msm8660",
+	  .data = (void *) SCM_HAS_CORE_CLK,
+	},
+	{ .compatible = "qcom,scm-msm8960",
+	  .data = (void *) SCM_HAS_CORE_CLK,
+	},
+	{ .compatible = "qcom,scm-msm8996",
+	  .data = NULL, /* no clocks */
+	},
+	{ .compatible = "qcom,scm",
+	  .data = (void *) ( SCM_HAS_CORE_CLK
+			    | SCM_HAS_IFACE_CLK
+			    | SCM_HAS_BUS_CLK ),
+	},
 	{}
 };
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/3] firmware: qcom: scm: Remove core, iface and bus clocks dependency

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]

Hi Sarangdhar,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.9-rc2 next-20161028]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Sarangdhar-Joshi/Remove-clocks-dependency-from-SCM-driver/20161029-081333
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All warnings (new ones prefixed by >>):

   drivers/firmware/qcom_scm.c: In function 'qcom_scm_probe':
>> drivers/firmware/qcom_scm.c:337:9: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     clks = (uint64_t)of_device_get_match_data(&pdev->dev);
            ^

vim +337 drivers/firmware/qcom_scm.c

   321	bool qcom_scm_is_available(void)
   322	{
   323		return !!__scm;
   324	}
   325	EXPORT_SYMBOL(qcom_scm_is_available);
   326	
   327	static int qcom_scm_probe(struct platform_device *pdev)
   328	{
   329		struct qcom_scm *scm;
   330		uint64_t clks;
   331		int ret;
   332	
   333		scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
   334		if (!scm)
   335			return -ENOMEM;
   336	
 > 337		clks = (uint64_t)of_device_get_match_data(&pdev->dev);
   338		if (clks & SCM_HAS_CORE_CLK) {
   339			scm->core_clk = devm_clk_get(&pdev->dev, "core");
   340			if (IS_ERR(scm->core_clk)) {
   341				if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
   342					return PTR_ERR(scm->core_clk);
   343	
   344				scm->core_clk = NULL;
   345			}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59436 bytes --]

Re: [PATCH 2/3] firmware: qcom: scm: Remove core, iface and bus clocks dependency

[Stephen Boyd]

On 10/28, Sarangdhar Joshi wrote:
> @@ -380,33 +384,43 @@ EXPORT_SYMBOL(qcom_scm_is_available);
>  static int qcom_scm_probe(struct platform_device *pdev)
>  {
>  	struct qcom_scm *scm;
> +	uint64_t clks;
>  	int ret;
>  
>  	scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
>  	if (!scm)
>  		return -ENOMEM;
>  
> -	scm->core_clk = devm_clk_get(&pdev->dev, "core");
> -	if (IS_ERR(scm->core_clk)) {
> -		if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
> -			return PTR_ERR(scm->core_clk);
> +	clks = (uint64_t)of_device_get_match_data(&pdev->dev);
> +	if (clks & SCM_HAS_CORE_CLK) {
> +		scm->core_clk = devm_clk_get(&pdev->dev, "core");
> +		if (IS_ERR(scm->core_clk)) {
> +			if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
> +				return PTR_ERR(scm->core_clk);
>  
> -		scm->core_clk = NULL;
> +			scm->core_clk = NULL;
> +		}
>  	}
>  
>  	if (of_device_is_compatible(pdev->dev.of_node, "qcom,scm")) {

Why didn't this also get added to the flags feature? I'd prefer
we either use of_device_is_compatible() for everything, or device
data to figure out what quirks to apply.

> -		scm->iface_clk = devm_clk_get(&pdev->dev, "iface");
> -		if (IS_ERR(scm->iface_clk)) {
> -			if (PTR_ERR(scm->iface_clk) != -EPROBE_DEFER)

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/3] firmware: qcom: scm: Remove core, iface and bus clocks dependency

[Sarangdhar Joshi]

On 11/01/2016 04:11 PM, Stephen Boyd wrote:
> On 10/28, Sarangdhar Joshi wrote:
>> @@ -380,33 +384,43 @@ EXPORT_SYMBOL(qcom_scm_is_available);
>>  static int qcom_scm_probe(struct platform_device *pdev)
>>  {
>>  	struct qcom_scm *scm;
>> +	uint64_t clks;
>>  	int ret;
>>
>>  	scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
>>  	if (!scm)
>>  		return -ENOMEM;
>>
>> -	scm->core_clk = devm_clk_get(&pdev->dev, "core");
>> -	if (IS_ERR(scm->core_clk)) {
>> -		if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
>> -			return PTR_ERR(scm->core_clk);
>> +	clks = (uint64_t)of_device_get_match_data(&pdev->dev);
>> +	if (clks & SCM_HAS_CORE_CLK) {
>> +		scm->core_clk = devm_clk_get(&pdev->dev, "core");
>> +		if (IS_ERR(scm->core_clk)) {
>> +			if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
>> +				return PTR_ERR(scm->core_clk);
>>
>> -		scm->core_clk = NULL;
>> +			scm->core_clk = NULL;
>> +		}
>>  	}
>>
>>  	if (of_device_is_compatible(pdev->dev.of_node, "qcom,scm")) {
>
> Why didn't this also get added to the flags feature? I'd prefer
> we either use of_device_is_compatible() for everything, or device
> data to figure out what quirks to apply.

You're right. These flags are already added for "qcom,scm" compatible. 
We can modify this to honor data flags only. I hope it's okay to update 
it in the same patch?

Regards,
Sarang

>
>> -		scm->iface_clk = devm_clk_get(&pdev->dev, "iface");
>> -		if (IS_ERR(scm->iface_clk)) {
>> -			if (PTR_ERR(scm->iface_clk) != -EPROBE_DEFER)
>


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/3] firmware: qcom: scm: Remove core, iface and bus clocks dependency

[Bjorn Andersson]

On Wed 02 Nov 12:24 PDT 2016, Sarangdhar Joshi wrote:

> On 11/01/2016 04:11 PM, Stephen Boyd wrote:
> >On 10/28, Sarangdhar Joshi wrote:
> >>@@ -380,33 +384,43 @@ EXPORT_SYMBOL(qcom_scm_is_available);
> >> static int qcom_scm_probe(struct platform_device *pdev)
> >> {
> >> 	struct qcom_scm *scm;
> >>+	uint64_t clks;
> >> 	int ret;
> >>
> >> 	scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
> >> 	if (!scm)
> >> 		return -ENOMEM;
> >>
> >>-	scm->core_clk = devm_clk_get(&pdev->dev, "core");
> >>-	if (IS_ERR(scm->core_clk)) {
> >>-		if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
> >>-			return PTR_ERR(scm->core_clk);
> >>+	clks = (uint64_t)of_device_get_match_data(&pdev->dev);
> >>+	if (clks & SCM_HAS_CORE_CLK) {
> >>+		scm->core_clk = devm_clk_get(&pdev->dev, "core");
> >>+		if (IS_ERR(scm->core_clk)) {
> >>+			if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
> >>+				return PTR_ERR(scm->core_clk);
> >>
> >>-		scm->core_clk = NULL;
> >>+			scm->core_clk = NULL;
> >>+		}
> >> 	}
> >>
> >> 	if (of_device_is_compatible(pdev->dev.of_node, "qcom,scm")) {
> >
> >Why didn't this also get added to the flags feature? I'd prefer
> >we either use of_device_is_compatible() for everything, or device
> >data to figure out what quirks to apply.
> 
> You're right. These flags are already added for "qcom,scm" compatible. We
> can modify this to honor data flags only. I hope it's okay to update it in
> the same patch?
> 

Yes, you're replacing the old conditional with a set of new ones, so it
should go in this patch.

Regards,
Bjorn

[PATCH 3/3] firmware: qcom: scm: Return PTR_ERR when devm_clk_get fails

[Sarangdhar Joshi]

When devm_clk_get fails for core clock, the failure was ignored
and the core_clk was explicitly set to NULL so that other
remaining clocks can be queried. However, now that we have a
clean way of expressing the clock dependency, return failure when
devm_clk_get fails for core clock.

Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org>
---
 drivers/firmware/qcom_scm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 010a350..5c472da 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -395,10 +395,10 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	if (clks & SCM_HAS_CORE_CLK) {
 		scm->core_clk = devm_clk_get(&pdev->dev, "core");
 		if (IS_ERR(scm->core_clk)) {
-			if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
-				return PTR_ERR(scm->core_clk);
-
-			scm->core_clk = NULL;
+			if (PTR_ERR(scm->core_clk) != -EPROBE_DEFER)
+				dev_err(&pdev->dev,
+					"failed to acquire core clk\n");
+			return PTR_ERR(scm->core_clk);
 		}
 	}
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project