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

[PATCH v2 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.

Changes since v1:
- Added Rob's Acked-by
- Removed of_device_is_compatible check from probe (Stephen)
- Modified typecast to take care of 32-bit pointer

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                        | 49 ++++++++++++++++------
 2 files changed, 39 insertions(+), 12 deletions(-)

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

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

[Sarangdhar Joshi]

Add SCM DT bindings for Qualcomm's MSM8996 platform.

Acked-by: Rob Herring <robh@kernel.org>
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 v2 1/3] dt-bindings: firmware: scm: Add MSM8996 DT bindings

[Stephen Boyd]

On 11/03, Sarangdhar Joshi wrote:
> Add SCM DT bindings for Qualcomm's MSM8996 platform.
> 
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

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

[PATCH v2 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 | 49 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index d79fecd..844e90d 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,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available);
 static int qcom_scm_probe(struct platform_device *pdev)
 {
 	struct qcom_scm *scm;
+	uint32_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 = (uint32_t)((uintptr_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")) {
+	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");
+				dev_err(&pdev->dev,
+					"failed to acquire iface clk\n");
 			return PTR_ERR(scm->iface_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");
+				dev_err(&pdev->dev,
+					"failed to acquire bus clk\n");
 			return PTR_ERR(scm->bus_clk);
 		}
 	}
@@ -429,10 +441,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 v2 2/3] firmware: qcom: scm: Remove core, iface and bus clocks dependency

[Stephen Boyd]

On 11/03, Sarangdhar Joshi wrote:
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index d79fecd..844e90d 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available);
>  static int qcom_scm_probe(struct platform_device *pdev)
>  {
>  	struct qcom_scm *scm;
> +	uint32_t clks;

If this was unsigned long flags;

>  	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 = (uint32_t)((uintptr_t)of_device_get_match_data(&pdev->dev));

then this could just be a cast to unsigned long?

> +	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;
> +		}
>  	}

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

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

[Bjorn Andersson]

On Wed 09 Nov 17:47 PST 2016, Stephen Boyd wrote:

> On 11/03, Sarangdhar Joshi wrote:
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > index d79fecd..844e90d 100644
> > --- a/drivers/firmware/qcom_scm.c
> > +++ b/drivers/firmware/qcom_scm.c
> > @@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available);
> >  static int qcom_scm_probe(struct platform_device *pdev)
> >  {
> >  	struct qcom_scm *scm;
> > +	uint32_t clks;
> 
> If this was unsigned long flags;
> 

I did look at this too and could only find a mixture of ways people have
done this. Isn't the correct type for this intptr_t?

Regards,
Bjorn

> >  	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 = (uint32_t)((uintptr_t)of_device_get_match_data(&pdev->dev));
> 
> then this could just be a cast to unsigned long?
> 
> > +	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;
> > +		}
> >  	}
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

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

[Stephen Boyd]

On 11/09, Bjorn Andersson wrote:
> On Wed 09 Nov 17:47 PST 2016, Stephen Boyd wrote:
> 
> > On 11/03, Sarangdhar Joshi wrote:
> > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > > index d79fecd..844e90d 100644
> > > --- a/drivers/firmware/qcom_scm.c
> > > +++ b/drivers/firmware/qcom_scm.c
> > > @@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available);
> > >  static int qcom_scm_probe(struct platform_device *pdev)
> > >  {
> > >  	struct qcom_scm *scm;
> > > +	uint32_t clks;
> > 
> > If this was unsigned long flags;
> > 
> 
> I did look at this too and could only find a mixture of ways people have
> done this. Isn't the correct type for this intptr_t?
> 

Well unsigned long == sizeof(kernel pointer) on all Linux
platforms so it's safe to do the cast. For example, struct
timer_list stores data in unsigned long. I guess this is a
'kernelism' though? Perhaps we should update
Documentation/CodingStyle if anyone cares.

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

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

[Sarangdhar Joshi]

On 11/09/2016 09:55 PM, Bjorn Andersson wrote:
> On Wed 09 Nov 17:47 PST 2016, Stephen Boyd wrote:
>
>> On 11/03, Sarangdhar Joshi wrote:
>>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>>> index d79fecd..844e90d 100644
>>> --- a/drivers/firmware/qcom_scm.c
>>> +++ b/drivers/firmware/qcom_scm.c
>>> @@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available);
>>>  static int qcom_scm_probe(struct platform_device *pdev)
>>>  {
>>>  	struct qcom_scm *scm;
>>> +	uint32_t clks;
>>
>> If this was unsigned long flags;
>>
>
> I did look at this too and could only find a mixture of ways people have
> done this. Isn't the correct type for this intptr_t?

That's true. There are lot of variations of how it's done. I had 
referred one of the gpio driver for this. I think it's safe to use 
unsigned long instead as Stephen suggested.

Btw I don't see intptr_t defined in include/linux/types.h.

Regards,
Sarang

>
> Regards,
> Bjorn
>
>>>  	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 = (uint32_t)((uintptr_t)of_device_get_match_data(&pdev->dev));
>>
>> then this could just be a cast to unsigned long?
>>
>>> +	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;
>>> +		}
>>>  	}
>>
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project


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

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

[Sarangdhar Joshi]

On 11/09/2016 05:47 PM, Stephen Boyd wrote:
> On 11/03, Sarangdhar Joshi wrote:
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index d79fecd..844e90d 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available);
>>  static int qcom_scm_probe(struct platform_device *pdev)
>>  {
>>  	struct qcom_scm *scm;
>> +	uint32_t clks;
>
> If this was unsigned long flags;
>
>>  	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 = (uint32_t)((uintptr_t)of_device_get_match_data(&pdev->dev));
>
> then this could just be a cast to unsigned long?

I saw quite a few places in kernel where similar type casts are used 
(e.g gpio drivers) and ended up using uint32_t. uintptr_t is nothing but 
typedef of an unsigned long. Probably it's a good idea to just use 
unsigned long instead.

>
>> +	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;
>> +		}
>>  	}
>


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

[PATCH v2 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
cleaner 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 844e90d..0321a26 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

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

[Stephen Boyd]

On 11/03, Sarangdhar Joshi wrote:
> 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
> cleaner way of expressing the clock dependency, return failure
> when devm_clk_get fails for core clock.
> 
> Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org>

Why can't this be folded into the second patch? It seems
incorrect to allow it to silently work in patch 2 even though
we've flagged that clk as a requirement and then change that
behavior here.

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

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

[Sarangdhar Joshi]

On 11/09/2016 05:49 PM, Stephen Boyd wrote:
> On 11/03, Sarangdhar Joshi wrote:
>> 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
>> cleaner way of expressing the clock dependency, return failure
>> when devm_clk_get fails for core clock.
>>
>> Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org>
>
> Why can't this be folded into the second patch? It seems
> incorrect to allow it to silently work in patch 2 even though
> we've flagged that clk as a requirement and then change that
> behavior here.
>

The idea was keep it separate so that if someone needs to keep earlier 
behavior (of ignoring the core clock failure), then it would be easier 
to just revert this commit and still use flag mechanism. Also convenient 
with git bisect for finding offending commit. I don't mind squashing 
this into patch 2 if we think that's better.

Regards,
Sarang

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