[RESEND PATCH v4 0/7]remoteproc: qcom: Add support to hexagon v56 1.5.0 in qcom hexagon rproc driver

[RESEND PATCH v4 0/7]remoteproc: qcom: Add support to hexagon v56 1.5.0 in qcom hexagon rproc driver

[Avaneesh Kumar Dwivedi]

This is patchset v4 having modifications as per comment on patchset v3.
Major changes w.r.t. patchset v3 are as below.
	1- clean up of resource struct initialization.
	2- breaking up all changes into more number of logical patches.
	3- handling of clock through array of clock pointers.
	4- single place handling of proxy and active regulators.
	5- use of reset control framework for MSS reset
	5- Addressing Other comments on last patches.

Avaneesh Kumar Dwivedi (7):
  remoteproc: qcom: Add and initialize private data for hexagon dsp.
  remoteproc: qcom: Initialize proxy and active clock's and regulator's
  remoteproc: qcom: Modify regulator enable and disable interface
  remoteproc: qcom: Modify clock enable and disable routine
  remoteproc: qcom: Modify reset sequence for hexagon to support v56
    1.5.0
  remoteproc: qcom: Modify stop routine to limit MX current for v56 1.5
  clk: qcom: Add GCC_MSS_RESET support to reset MSS in v56 1.5.0

 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   2 +
 drivers/clk/qcom/gcc-msm8996.c                     |   1 +
 drivers/remoteproc/qcom_q6v5_pil.c                 | 544 ++++++++++++++++-----
 include/dt-bindings/clock/qcom,gcc-msm8996.h       |   1 +
 4 files changed, 436 insertions(+), 112 deletions(-)

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[RESEND PATCH v4 1/7] remoteproc: qcom: Add and initialize private data for hexagon dsp.

[Avaneesh Kumar Dwivedi]

Resource string's specific to version of hexagon chip are initialized
to be passed to probe for various resource init purpose.
Also compatible string introduced are added to documentation.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |  2 +
 drivers/remoteproc/qcom_q6v5_pil.c                 | 61 +++++++++++++++++++++-
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 57cb49e..d4c14f0 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -8,6 +8,8 @@ on the Qualcomm Hexagon core.
 	Value type: <string>
 	Definition: must be one of:
 		    "qcom,q6v5-pil"
+		"qcom,q6v5-5-1-1-pil"
+		"qcom,q6v56-1-5-0-pil"
 
 - reg:
 	Usage: required
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 2e0caaa..3360312 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -30,6 +30,7 @@
 #include <linux/reset.h>
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/smem_state.h>
+#include <linux/of_device.h>
 
 #include "remoteproc_internal.h"
 #include "qcom_mdt_loader.h"
@@ -93,6 +94,22 @@
 #define QDSS_BHS_ON			BIT(21)
 #define QDSS_LDO_BYP			BIT(22)
 
+struct rproc_hexagon_res {
+	char *hexagon_mba_image;
+	char **proxy_reg_string;
+	char **active_reg_string;
+	int proxy_uV_uA[3][2];
+	int active_uV_uA[1][2];
+	char **proxy_clk_string;
+	char **active_clk_string;
+	int hexagon_ver;
+};
+
+struct reg_info {
+	struct regulator *reg;
+	int uV;
+	int uA;
+};
 struct q6v5 {
 	struct device *dev;
 	struct rproc *rproc;
@@ -131,6 +148,12 @@ struct q6v5 {
 };
 
 enum {
+	Q6V5_5_0_0, /*hexagon on msm8916*/
+	Q6V5_5_1_1, /*hexagon on msm8974*/
+	Q5V56_1_5_0, /*hexagon on msm8996*/
+};
+
+enum {
 	Q6V5_SUPPLY_CX,
 	Q6V5_SUPPLY_MX,
 	Q6V5_SUPPLY_MSS,
@@ -890,8 +913,44 @@ static int q6v5_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct rproc_hexagon_res q6v56_1_5_0_res = {
+	.hexagon_mba_image = "mba.mbn",
+	.proxy_reg_string = (char*[]){"mx", "cx", "pll", NULL},
+	.active_reg_string = NULL,
+	.proxy_uV_uA = { {0, 0}, {0, 100000}, {0, 100000} },
+	.active_uV_uA = { {0} },
+	.proxy_clk_string = (char*[]){"xo", "pnoc", "qdss", NULL},
+	.active_clk_string = (char*[]){"iface", "bus", "mem",
+		"gpll0_mss_clk", "snoc_axi_clk", "mnoc_axi_clk", NULL},
+	.hexagon_ver = Q5V56_1_5_0,
+};
+
+static const struct rproc_hexagon_res q6v5_5_0_0_res = {
+	.hexagon_mba_image = "mba.mbn",
+	.proxy_reg_string = (char*[]){"mx", "cx", "pll", NULL},
+	.proxy_uV_uA = { {1050000, 0}, {0, 100000}, {0, 100000} },
+	.active_reg_string = (char*[]){"mss", NULL},
+	.active_uV_uA = { {1000000, 100000} },
+	.proxy_clk_string = (char*[]){"xo", NULL},
+	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},
+	.hexagon_ver = Q6V5_5_0_0,
+};
+
+static const struct rproc_hexagon_res q6v5_5_1_1_res = {
+	.hexagon_mba_image = "mba.b00",
+	.proxy_reg_string = (char*[]){"mx", "cx", "pll", NULL},
+	.proxy_uV_uA = { {1050000, 0}, {0, 100000}, {0, 100000} },
+	.active_reg_string = (char*[]){"mss", NULL},
+	.active_uV_uA = { {1000000, 100000} },
+	.proxy_clk_string = (char*[]){"xo", NULL},
+	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},
+	.hexagon_ver = Q6V5_5_1_1,
+};
+
 static const struct of_device_id q6v5_of_match[] = {
-	{ .compatible = "qcom,q6v5-pil", },
+	{ .compatible = "qcom,q6v5-pil", .data = &q6v5_5_0_0_res},
+	{ .compatible = "qcom,q6v5-5-1-1-pil", .data = &q6v5_5_1_1_res},
+	{ .compatible = "qcom,q6v56-1-5-0-pil", .data = &q6v56_1_5_0_res},
 	{ },
 };
 
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [RESEND PATCH v4 1/7] remoteproc: qcom: Add and initialize private data for hexagon dsp.

[Bjorn Andersson]

On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:

> Resource string's specific to version of hexagon chip are initialized
> to be passed to probe for various resource init purpose.
> Also compatible string introduced are added to documentation.
> 

Overall I think the series looks good now, will comment on individual
things of each patch, but I'm generally happy about how things look.


I would however like to see a slight restructuring between the patches.

Rather than adding regulators, clocks and version to the res-struct in
patch 1 and then add the code using these later I would like you to
introduce the smallest possible struct here and then add each part in
the relevant patch. And finish off with adding the msm8996 compatible,
once all the pieces are in place.

So in this first patch I would suggest that you add the msm8974 and
msm8916 compatibles, a res-struct containing only hexagon_mba_image and
the change from patch 2 where you change rproc_alloc() to use the
hexagon_mba_image.

Then in the regulator patch add the regulators for msm8974 and msm8916,
same with clocks in the clock patch and then add the hexagon_ver, the
associated changes and the msm8996 compatible in one patch.

That way each patch add a single consistent chunk of the changes.

> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |  2 +
>  drivers/remoteproc/qcom_q6v5_pil.c                 | 61 +++++++++++++++++++++-
>  2 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 57cb49e..d4c14f0 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> @@ -8,6 +8,8 @@ on the Qualcomm Hexagon core.
>  	Value type: <string>
>  	Definition: must be one of:
>  		    "qcom,q6v5-pil"
> +		"qcom,q6v5-5-1-1-pil"
> +		"qcom,q6v56-1-5-0-pil"

The more I look at these numbers and what's used downstream the more
confused I get and perhaps more important, I can't find any
documentation mentioning these numbers.

As far as I can see these numbers are 1:1 with specific platforms, which
we use as part of other bindings. So I think we should follow the naming
scheme we use for e.g. the ADSP PIL.

And let's replace the q6v5 part with "mss", as e.g. msm8974 adsp also is
a "q6v5".

So please add:
"qcom,msm8916-mss-pil",
"qcom,msm8974-mss-pil",
"qcom,msm8996-mss-pil"

The compatible "qcom,q6v5-pil" is already introduced in the
msm8916.dtsi, so make that compatible be equivalent to
"qcom,msm8916-mss-pil" (but let's have both for clarity).

>  
>  - reg:
>  	Usage: required
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 2e0caaa..3360312 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -30,6 +30,7 @@
>  #include <linux/reset.h>
>  #include <linux/soc/qcom/smem.h>
>  #include <linux/soc/qcom/smem_state.h>
> +#include <linux/of_device.h>
>  
>  #include "remoteproc_internal.h"
>  #include "qcom_mdt_loader.h"
> @@ -93,6 +94,22 @@
>  #define QDSS_BHS_ON			BIT(21)
>  #define QDSS_LDO_BYP			BIT(22)
>  
> +struct rproc_hexagon_res {
> +	char *hexagon_mba_image;

const

> +	char **proxy_reg_string;
> +	char **active_reg_string;
> +	int proxy_uV_uA[3][2];
> +	int active_uV_uA[1][2];

Let's group these into a:

struct qcom_mss_reg_res {
	const char *supply;
	int uA;
	int uV;
};

Then the res definitions below becomes:

satic const struct rproc_hexagon_res msm8916_res = {
	.proxy_regs = (struct qcom_mss_reg_res[]) {
		{
			.supply = "mx",
		},
		{
			.supply = "cx",
			.uA = 100000,
		},
		{
			.supply = "pll",
			.uA = 100000,
		},
		{ NULL }
	},
	...
};

> +	char **proxy_clk_string;
> +	char **active_clk_string;
> +	int hexagon_ver;
> +};
> +
> +struct reg_info {
> +	struct regulator *reg;
> +	int uV;
> +	int uA;
> +};
>  struct q6v5 {
>  	struct device *dev;
>  	struct rproc *rproc;
> @@ -131,6 +148,12 @@ struct q6v5 {
>  };
>  
>  enum {
> +	Q6V5_5_0_0, /*hexagon on msm8916*/
> +	Q6V5_5_1_1, /*hexagon on msm8974*/
> +	Q5V56_1_5_0, /*hexagon on msm8996*/

As I said above, this turns out to be confusing. Name them based on
platform instead. Something like Q6V5_MSM8916

> +};
> +

Regards,
Bjorn

Re: [RESEND PATCH v4 1/7] remoteproc: qcom: Add and initialize private data for hexagon dsp.

[Dwivedi, Avaneesh Kumar (avani)]

On 12/9/2016 3:07 AM, Bjorn Andersson wrote:
> On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> Resource string's specific to version of hexagon chip are initialized
>> to be passed to probe for various resource init purpose.
>> Also compatible string introduced are added to documentation.
>>
> Overall I think the series looks good now, will comment on individual
> things of each patch, but I'm generally happy about how things look.
Thanks Bjorn for reviewing,
>
>
> I would however like to see a slight restructuring between the patches.
ok, sure.
>
> Rather than adding regulators, clocks and version to the res-struct in
> patch 1 and then add the code using these later I would like you to
> introduce the smallest possible struct here and then add each part in
> the relevant patch. And finish off with adding the msm8996 compatible,
> once all the pieces are in place.
ok.
>
> So in this first patch I would suggest that you add the msm8974 and
> msm8916 compatibles, a res-struct containing only hexagon_mba_image and
> the change from patch 2 where you change rproc_alloc() to use the
> hexagon_mba_image.
ok. so i am going to add additional compatible string while also keeping 
existing compatible
as below. Also compatible string i have changed but is it OK to keep 
resource instance named as "qdsp6v5_5_0_0_res" as below?

+static const struct rproc_hexagon_res qdsp6v5_5_0_0_res = {
+       .hexagon_mba_image = "mba.mbn",
+};
+static const struct rproc_hexagon_res qdsp6v5_5_1_1_res = {
+       .hexagon_mba_image = "mba.b00",
+};
  static const struct of_device_id q6v5_of_match[] = {
-       { .compatible = "qcom,q6v5-pil", },
+       { .compatible = "qcom,q6v5-pil", .data = &qdsp6v5_5_0_0_res},
+       { .compatible = "qcom,msm8916-mss-pil", .data = &qdsp6v5_5_0_0_res},
+       { .compatible = "qcom,msm8974-mss-pil", .data = &qdsp6v5_5_1_1_res},
         { },
  };

>
> Then in the regulator patch add the regulators for msm8974 and msm8916,
> same with clocks in the clock patch and then add the hexagon_ver, the
> associated changes and the msm8996 compatible in one patch.
Ok.
>
> That way each patch add a single consistent chunk of the changes.
Sure.
>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |  2 +
>>   drivers/remoteproc/qcom_q6v5_pil.c                 | 61 +++++++++++++++++++++-
>>   2 files changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> index 57cb49e..d4c14f0 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> @@ -8,6 +8,8 @@ on the Qualcomm Hexagon core.
>>   	Value type: <string>
>>   	Definition: must be one of:
>>   		    "qcom,q6v5-pil"
>> +		"qcom,q6v5-5-1-1-pil"
>> +		"qcom,q6v56-1-5-0-pil"
> The more I look at these numbers and what's used downstream the more
> confused I get and perhaps more important, I can't find any
> documentation mentioning these numbers.
These versions which i adopted here are as per HPG documents.
There was some inconsistency in using the exact version of hexagon chip 
for compatible string purpose in downstream.
If one look documentation in downstream similar naming convention is 
adopted to indicate version though compatible string is still not in 
conformance.

for example
Documentation/devicetree/bindings/pil/pil-q6v5-mss.txt:- 
qcom,qdsp6v56-1-3: Boolean- Present if the qdsp version is v56 1.3====> 
applicable for mdm9640 platform
Documentation/devicetree/bindings/pil/pil-q6v5-mss.txt:- 
qcom,qdsp6v56-1-5: Boolean- Present if the qdsp version is v56 1.5 ===> 
applicable for msm8996 platform
Documentation/devicetree/bindings/pil/pil-q6v5-mss.txt:- 
qcom,qdsp6v56-1-8: Boolean- Present if the qdsp version is v56 1.8 ===> 
applicable for msm8952 and msm8940 platforms.
Documentation/devicetree/bindings/pil/pil-q6v5-mss.txt:- 
qcom,qdsp6v56-1-10: Boolean- Present if the qdsp version is v56 
1.10===>applicable for msm8953 platform
Documentation/devicetree/bindings/pil/pil-q6v5-mss.txt:- 
qcom,qdsp6v61-1-1: Boolean- Present if the qdsp version is v61 1.1
Documentation/devicetree/bindings/pil/pil-q6v5-mss.txt:- 
qcom,qdsp6v62-1-2: Boolean- Present if the qdsp version is v62 
1.2====>applicable for msm8998
Documentation/devicetree/bindings/pil/pil-q6v5-mss.txt:- 
qcom,qdsp6v62-1-5: Boolean- Present if the qdsp version is v62 1.5


>
> As far as I can see these numbers are 1:1 with specific platforms, which
> we use as part of other bindings. So I think we should follow the naming
> scheme we use for e.g. the ADSP PIL.
In very few cases same hexagon chip is used in more than one msm 
platform i.e. one-to-many
example  q6v56 1.8 is used on msm8952 as well as on msm8940, but 
generally it is one to one mapping with msm platform.
>
> And let's replace the q6v5 part with "mss", as e.g. msm8974 adsp also is
> a "q6v5".
>
> So please add:
> "qcom,msm8916-mss-pil",
> "qcom,msm8974-mss-pil",
> "qcom,msm8996-mss-pil"
OK.
>
> The compatible "qcom,q6v5-pil" is already introduced in the
> msm8916.dtsi, so make that compatible be equivalent to
> "qcom,msm8916-mss-pil" (but let's have both for clarity).
so i will keep "qcom,msm8916-mss-pil" as well as "qcom,q6v5-pil" both in 
code.
>
>>   
>>   - reg:
>>   	Usage: required
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 2e0caaa..3360312 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -30,6 +30,7 @@
>>   #include <linux/reset.h>
>>   #include <linux/soc/qcom/smem.h>
>>   #include <linux/soc/qcom/smem_state.h>
>> +#include <linux/of_device.h>
>>   
>>   #include "remoteproc_internal.h"
>>   #include "qcom_mdt_loader.h"
>> @@ -93,6 +94,22 @@
>>   #define QDSS_BHS_ON			BIT(21)
>>   #define QDSS_LDO_BYP			BIT(22)
>>   
>> +struct rproc_hexagon_res {
>> +	char *hexagon_mba_image;
> const
OK.
>
>> +	char **proxy_reg_string;
>> +	char **active_reg_string;
>> +	int proxy_uV_uA[3][2];
>> +	int active_uV_uA[1][2];
> Let's group these into a:
>
> struct qcom_mss_reg_res {
> 	const char *supply;
> 	int uA;
> 	int uV;
> };
>
> Then the res definitions below becomes:
>
> satic const struct rproc_hexagon_res msm8916_res = {
> 	.proxy_regs = (struct qcom_mss_reg_res[]) {
> 		{
> 			.supply = "mx",
> 		},
> 		{
> 			.supply = "cx",
> 			.uA = 100000,
> 		},
> 		{
> 			.supply = "pll",
> 			.uA = 100000,
> 		},
> 		{ NULL }
> 	},
> 	...
> };
Ok, Sure.
>
>> +	char **proxy_clk_string;
>> +	char **active_clk_string;
>> +	int hexagon_ver;
>> +};
>> +
>> +struct reg_info {
>> +	struct regulator *reg;
>> +	int uV;
>> +	int uA;
>> +};
>>   struct q6v5 {
>>   	struct device *dev;
>>   	struct rproc *rproc;
>> @@ -131,6 +148,12 @@ struct q6v5 {
>>   };
>>   
>>   enum {
>> +	Q6V5_5_0_0, /*hexagon on msm8916*/
>> +	Q6V5_5_1_1, /*hexagon on msm8974*/
>> +	Q5V56_1_5_0, /*hexagon on msm8996*/
> As I said above, this turns out to be confusing. Name them based on
> platform instead. Something like Q6V5_MSM8916
Sure.
>
>> +};
>> +
> Regards,
> Bjorn

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [RESEND PATCH v4 1/7] remoteproc: qcom: Add and initialize private data for hexagon dsp.

[Bjorn Andersson]

On Fri 09 Dec 03:42 PST 2016, Dwivedi, Avaneesh Kumar (avani) wrote:

> >So in this first patch I would suggest that you add the msm8974 and
> >msm8916 compatibles, a res-struct containing only hexagon_mba_image and
> >the change from patch 2 where you change rproc_alloc() to use the
> >hexagon_mba_image.
> ok. so i am going to add additional compatible string while also keeping
> existing compatible as below.

Yes, we need to keep the qcom,q6v5-pil and your change below looks to
get it right.

> Also compatible string i have changed but is it OK to keep
> resource instance named as "qdsp6v5_5_0_0_res" as below?
> 

Please update their naming based on the platform name as well, it will
cause less confusion in the future.

> +static const struct rproc_hexagon_res qdsp6v5_5_0_0_res = {
> +       .hexagon_mba_image = "mba.mbn",
> +};

A single empty line between each struct makes things easier to read.

> +static const struct rproc_hexagon_res qdsp6v5_5_1_1_res = {
> +       .hexagon_mba_image = "mba.b00",
> +};
>  static const struct of_device_id q6v5_of_match[] = {
> -       { .compatible = "qcom,q6v5-pil", },
> +       { .compatible = "qcom,q6v5-pil", .data = &qdsp6v5_5_0_0_res},
> +       { .compatible = "qcom,msm8916-mss-pil", .data = &qdsp6v5_5_0_0_res},
> +       { .compatible = "qcom,msm8974-mss-pil", .data = &qdsp6v5_5_1_1_res},
>         { },
>  };
[..]
> >As far as I can see these numbers are 1:1 with specific platforms, which
> >we use as part of other bindings. So I think we should follow the naming
> >scheme we use for e.g. the ADSP PIL.
> In very few cases same hexagon chip is used in more than one msm platform
> i.e. one-to-many
> example  q6v56 1.8 is used on msm8952 as well as on msm8940, but generally
> it is one to one mapping with msm platform.

Okay, thanks for the summary.

In these rare cases we can have two compatibles referencing the same
struct.

> >
> >And let's replace the q6v5 part with "mss", as e.g. msm8974 adsp also is
> >a "q6v5".
> >
> >So please add:
> >"qcom,msm8916-mss-pil",
> >"qcom,msm8974-mss-pil",
> >"qcom,msm8996-mss-pil"
> OK.
> >
> >The compatible "qcom,q6v5-pil" is already introduced in the
> >msm8916.dtsi, so make that compatible be equivalent to
> >"qcom,msm8916-mss-pil" (but let's have both for clarity).
> so i will keep "qcom,msm8916-mss-pil" as well as "qcom,q6v5-pil" both in
> code.

Sound good.

Regards,
Bjorn

[RESEND PATCH v4 2/7] remoteproc: qcom: Initialize proxy and active clock's and regulator's

[Avaneesh Kumar Dwivedi]

Certain regulators and clocks need voting by rproc on behalf of hexagon
only during restart operation but certain clocks and voltage need to be
voted till hexagon is up, these regulators and clocks are identified as
proxy and active resource whose handle is being obtained by supplying
private proxy and active regulator and clock string.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 148 +++++++++++++++++++++++++++----------
 1 file changed, 107 insertions(+), 41 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 3360312..b0f0fcf 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -37,7 +37,6 @@
 
 #include <linux/qcom_scm.h>
 
-#define MBA_FIRMWARE_NAME		"mba.b00"
 #define MPSS_FIRMWARE_NAME		"modem.mdt"
 
 #define MPSS_CRASH_REASON_SMEM		421
@@ -132,6 +131,14 @@ struct q6v5 {
 	struct clk *ahb_clk;
 	struct clk *axi_clk;
 	struct clk *rom_clk;
+	struct clk *active_clks[8];
+	struct clk *proxy_clks[4];
+	struct reg_info active_regs[1];
+	struct reg_info proxy_regs[3];
+	int active_reg_count;
+	int proxy_reg_count;
+	int active_clk_count;
+	int proxy_clk_count;
 
 	struct completion start_done;
 	struct completion stop_done;
@@ -160,27 +167,43 @@ enum {
 	Q6V5_SUPPLY_PLL,
 };
 
-static int q6v5_regulator_init(struct q6v5 *qproc)
+static int q6v5_regulator_init(struct device *dev,
+	struct reg_info *regs, char **reg_str, int volatage_load[][2])
 {
-	int ret;
+	int reg_count = 0, i;
 
-	qproc->supply[Q6V5_SUPPLY_CX].supply = "cx";
-	qproc->supply[Q6V5_SUPPLY_MX].supply = "mx";
-	qproc->supply[Q6V5_SUPPLY_MSS].supply = "mss";
-	qproc->supply[Q6V5_SUPPLY_PLL].supply = "pll";
+	if (!reg_str)
+		return 0;
 
-	ret = devm_regulator_bulk_get(qproc->dev,
-				      ARRAY_SIZE(qproc->supply), qproc->supply);
-	if (ret < 0) {
-		dev_err(qproc->dev, "failed to get supplies\n");
-		return ret;
-	}
+	while (reg_str[reg_count] != NULL)
+		reg_count++;
 
-	regulator_set_load(qproc->supply[Q6V5_SUPPLY_CX].consumer, 100000);
-	regulator_set_load(qproc->supply[Q6V5_SUPPLY_MSS].consumer, 100000);
-	regulator_set_load(qproc->supply[Q6V5_SUPPLY_PLL].consumer, 10000);
+	if (!reg_count)
+		return reg_count;
 
-	return 0;
+	if (!regs)
+		return -ENOMEM;
+
+	for (i = 0; i < reg_count; i++) {
+		const char *reg_name;
+
+		reg_name = reg_str[i];
+		regs[i].reg = devm_regulator_get(dev, reg_name);
+		if (IS_ERR(regs[i].reg)) {
+
+			int rc = PTR_ERR(regs[i].reg);
+
+			if (rc != -EPROBE_DEFER)
+				dev_err(dev, "Failed to get %s\n regulator",
+								reg_name);
+			return rc;
+		}
+
+		regs[i].uV = volatage_load[i][0];
+		regs[i].uA = volatage_load[i][1];
+	}
+
+	return reg_count;
 }
 
 static int q6v5_regulator_enable(struct q6v5 *qproc)
@@ -725,27 +748,41 @@ static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev)
 	return 0;
 }
 
-static int q6v5_init_clocks(struct q6v5 *qproc)
+static int q6v5_init_clocks(struct device *dev, struct clk **clks,
+		char **clk_str)
 {
-	qproc->ahb_clk = devm_clk_get(qproc->dev, "iface");
-	if (IS_ERR(qproc->ahb_clk)) {
-		dev_err(qproc->dev, "failed to get iface clock\n");
-		return PTR_ERR(qproc->ahb_clk);
-	}
+	int clk_count = 0, i;
 
-	qproc->axi_clk = devm_clk_get(qproc->dev, "bus");
-	if (IS_ERR(qproc->axi_clk)) {
-		dev_err(qproc->dev, "failed to get bus clock\n");
-		return PTR_ERR(qproc->axi_clk);
-	}
+	if (!clk_str)
+		return 0;
+
+	while (clk_str[clk_count] != NULL)
+		clk_count++;
+
+	if (!clk_count)
+		return clk_count;
+
+	if (!clks)
+		return -ENOMEM;
+
+	for (i = 0; i < clk_count; i++) {
+		const char *clock_name;
+
+		clock_name = clk_str[i];
+		clks[i] = devm_clk_get(dev, clock_name);
+		if (IS_ERR(clks[i])) {
+
+			int rc = PTR_ERR(clks[i]);
+
+			if (rc != -EPROBE_DEFER)
+				dev_err(dev, "Failed to get %s clock\n",
+					clock_name);
+			return rc;
+		}
 
-	qproc->rom_clk = devm_clk_get(qproc->dev, "mem");
-	if (IS_ERR(qproc->rom_clk)) {
-		dev_err(qproc->dev, "failed to get mem clock\n");
-		return PTR_ERR(qproc->rom_clk);
 	}
 
-	return 0;
+	return clk_count;
 }
 
 static int q6v5_init_reset(struct q6v5 *qproc)
@@ -830,10 +867,15 @@ static int q6v5_probe(struct platform_device *pdev)
 {
 	struct q6v5 *qproc;
 	struct rproc *rproc;
-	int ret;
+	const struct rproc_hexagon_res *desc;
+	int ret, count;
+
+	desc = of_device_get_match_data(&pdev->dev);
+	if (!desc)
+		return -EINVAL;
 
 	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops,
-			    MBA_FIRMWARE_NAME, sizeof(*qproc));
+			    desc->hexagon_mba_image, sizeof(*qproc));
 	if (!rproc) {
 		dev_err(&pdev->dev, "failed to allocate rproc\n");
 		return -ENOMEM;
@@ -857,18 +899,42 @@ static int q6v5_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
-	ret = q6v5_init_clocks(qproc);
-	if (ret)
-		goto free_rproc;
+	count = q6v5_init_clocks(&pdev->dev, qproc->proxy_clks,
+		desc->proxy_clk_string);
+	if (count < 0) {
+		dev_err(&pdev->dev, "Failed to setup proxy clocks.\n");
+		return count;
+	}
+	qproc->proxy_clk_count = count;
 
-	ret = q6v5_regulator_init(qproc);
-	if (ret)
-		goto free_rproc;
+	count = q6v5_init_clocks(&pdev->dev, qproc->active_clks,
+		desc->active_clk_string);
+	if (count < 0) {
+		dev_err(&pdev->dev, "Failed to setup active clocks.\n");
+		return count;
+	}
+	qproc->active_clk_count = count;
 
 	ret = q6v5_init_reset(qproc);
 	if (ret)
 		goto free_rproc;
 
+	count = q6v5_regulator_init(&pdev->dev, qproc->proxy_regs,
+		desc->proxy_reg_string, (int (*)[2])desc->proxy_uV_uA);
+	if (count < 0) {
+		dev_err(&pdev->dev, "Failed to setup active regulators.\n");
+		return count;
+	}
+	qproc->proxy_reg_count = count;
+
+	count = q6v5_regulator_init(&pdev->dev, qproc->active_regs,
+		desc->active_reg_string, (int (*)[2])desc->active_uV_uA);
+	if (count < 0) {
+		dev_err(&pdev->dev, "Failed to setup proxy regulators.\n");
+		return count;
+	}
+	qproc->active_reg_count = count;
+
 	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
 	if (ret < 0)
 		goto free_rproc;
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [RESEND PATCH v4 2/7] remoteproc: qcom: Initialize proxy and active clock's and regulator's

[Bjorn Andersson]

On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:

> Certain regulators and clocks need voting by rproc on behalf of hexagon
> only during restart operation but certain clocks and voltage need to be
> voted till hexagon is up, these regulators and clocks are identified as
> proxy and active resource whose handle is being obtained by supplying
> private proxy and active regulator and clock string.
> 

Please split this patch out into the regulator and clock patches
respectively, making each a clear functional change.

> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 148 +++++++++++++++++++++++++++----------
>  1 file changed, 107 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 3360312..b0f0fcf 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -37,7 +37,6 @@
>  
>  #include <linux/qcom_scm.h>
>  
> -#define MBA_FIRMWARE_NAME		"mba.b00"

Please move this to patch 1.

>  #define MPSS_FIRMWARE_NAME		"modem.mdt"
>  
>  #define MPSS_CRASH_REASON_SMEM		421
> @@ -132,6 +131,14 @@ struct q6v5 {
>  	struct clk *ahb_clk;
>  	struct clk *axi_clk;
>  	struct clk *rom_clk;
> +	struct clk *active_clks[8];
> +	struct clk *proxy_clks[4];
> +	struct reg_info active_regs[1];
> +	struct reg_info proxy_regs[3];
> +	int active_reg_count;
> +	int proxy_reg_count;
> +	int active_clk_count;
> +	int proxy_clk_count;

Group clocks members together and regulators together.

>  
>  	struct completion start_done;
>  	struct completion stop_done;
> @@ -160,27 +167,43 @@ enum {
>  	Q6V5_SUPPLY_PLL,
>  };
>  
> -static int q6v5_regulator_init(struct q6v5 *qproc)
> +static int q6v5_regulator_init(struct device *dev,
> +	struct reg_info *regs, char **reg_str, int volatage_load[][2])
>  {
> -	int ret;
> +	int reg_count = 0, i;

Please, one variable per line, sorted (descending) by line length.

>  
> -	qproc->supply[Q6V5_SUPPLY_CX].supply = "cx";
> -	qproc->supply[Q6V5_SUPPLY_MX].supply = "mx";
> -	qproc->supply[Q6V5_SUPPLY_MSS].supply = "mss";
> -	qproc->supply[Q6V5_SUPPLY_PLL].supply = "pll";
> +	if (!reg_str)
> +		return 0;
>  
> -	ret = devm_regulator_bulk_get(qproc->dev,
> -				      ARRAY_SIZE(qproc->supply), qproc->supply);
> -	if (ret < 0) {
> -		dev_err(qproc->dev, "failed to get supplies\n");
> -		return ret;
> -	}
> +	while (reg_str[reg_count] != NULL)

Drop "!= NULL" from your condition.

> +		reg_count++;
>  
> -	regulator_set_load(qproc->supply[Q6V5_SUPPLY_CX].consumer, 100000);
> -	regulator_set_load(qproc->supply[Q6V5_SUPPLY_MSS].consumer, 100000);
> -	regulator_set_load(qproc->supply[Q6V5_SUPPLY_PLL].consumer, 10000);
> +	if (!reg_count)
> +		return reg_count;

You can omit this check, as the for loop below will iterate 0 times and
we will then return 0.

>  
> -	return 0;
> +	if (!regs)
> +		return -ENOMEM;

This will not happen, please drop.

> +
> +	for (i = 0; i < reg_count; i++) {
> +		const char *reg_name;

Please keep local variables at the top of the function.

And generally try to use short but clear variable names; in line with my
suggestion in patch 1 name it "supply" (or in this case where the
original variable is quite short just use it directly).

> +
> +		reg_name = reg_str[i];
> +		regs[i].reg = devm_regulator_get(dev, reg_name);
> +		if (IS_ERR(regs[i].reg)) {
> +
> +			int rc = PTR_ERR(regs[i].reg);
> +
> +			if (rc != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to get %s\n regulator",
> +								reg_name);
> +			return rc;
> +		}
> +
> +		regs[i].uV = volatage_load[i][0];
> +		regs[i].uA = volatage_load[i][1];
> +	}
> +
> +	return reg_count;
>  }
>  
>  static int q6v5_regulator_enable(struct q6v5 *qproc)
> @@ -725,27 +748,41 @@ static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static int q6v5_init_clocks(struct q6v5 *qproc)
> +static int q6v5_init_clocks(struct device *dev, struct clk **clks,
> +		char **clk_str)
>  {
> -	qproc->ahb_clk = devm_clk_get(qproc->dev, "iface");
> -	if (IS_ERR(qproc->ahb_clk)) {
> -		dev_err(qproc->dev, "failed to get iface clock\n");
> -		return PTR_ERR(qproc->ahb_clk);
> -	}
> +	int clk_count = 0, i;

One variable per line, please.  And "count" is unambiguous enough.

>  
> -	qproc->axi_clk = devm_clk_get(qproc->dev, "bus");
> -	if (IS_ERR(qproc->axi_clk)) {
> -		dev_err(qproc->dev, "failed to get bus clock\n");
> -		return PTR_ERR(qproc->axi_clk);
> -	}
> +	if (!clk_str)
> +		return 0;
> +
> +	while (clk_str[clk_count] != NULL)

Drop "!= NULL" part

> +		clk_count++;
> +
> +	if (!clk_count)
> +		return clk_count;

This is not needed.

> +
> +	if (!clks)
> +		return -ENOMEM;

This can't happen.

> +
> +	for (i = 0; i < clk_count; i++) {
> +		const char *clock_name;
> +
> +		clock_name = clk_str[i];
> +		clks[i] = devm_clk_get(dev, clock_name);
> +		if (IS_ERR(clks[i])) {
> +
> +			int rc = PTR_ERR(clks[i]);
> +
> +			if (rc != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to get %s clock\n",
> +					clock_name);
> +			return rc;
> +		}
>  
> -	qproc->rom_clk = devm_clk_get(qproc->dev, "mem");
> -	if (IS_ERR(qproc->rom_clk)) {
> -		dev_err(qproc->dev, "failed to get mem clock\n");
> -		return PTR_ERR(qproc->rom_clk);
>  	}
>  
> -	return 0;
> +	return clk_count;
>  }
>  
>  static int q6v5_init_reset(struct q6v5 *qproc)
> @@ -830,10 +867,15 @@ static int q6v5_probe(struct platform_device *pdev)
>  {
>  	struct q6v5 *qproc;
>  	struct rproc *rproc;
> -	int ret;
> +	const struct rproc_hexagon_res *desc;
> +	int ret, count;

One variable per line please, and sort (descending) on line length.

> +
> +	desc = of_device_get_match_data(&pdev->dev);
> +	if (!desc)
> +		return -EINVAL;
>  
>  	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops,
> -			    MBA_FIRMWARE_NAME, sizeof(*qproc));
> +			    desc->hexagon_mba_image, sizeof(*qproc));

Please move this to patch 1.

>  	if (!rproc) {
>  		dev_err(&pdev->dev, "failed to allocate rproc\n");
>  		return -ENOMEM;
> @@ -857,18 +899,42 @@ static int q6v5_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto free_rproc;
>  
> -	if (ret)
> -		goto free_rproc;
> +	count = q6v5_init_clocks(&pdev->dev, qproc->proxy_clks,
> +		desc->proxy_clk_string);

Your "count" serves the same purpose as "ret", so just use "ret".
And align broken lines with the parenthesis.

> +	if (count < 0) {
> +		dev_err(&pdev->dev, "Failed to setup proxy clocks.\n");
> +		return count;

And using "ret" instead of "count" makes it easy to goto free_rproc,
which you must do after calling rproc_alloc().

> +	}
> +	qproc->proxy_clk_count = count;
>  
> -	ret = q6v5_regulator_init(qproc);
> -	if (ret)
> -		goto free_rproc;
> +	count = q6v5_init_clocks(&pdev->dev, qproc->active_clks,
> +		desc->active_clk_string);
> +	if (count < 0) {
> +		dev_err(&pdev->dev, "Failed to setup active clocks.\n");
> +		return count;
> +	}
> +	qproc->active_clk_count = count;
>  
>  	ret = q6v5_init_reset(qproc);
>  	if (ret)
>  		goto free_rproc;
>  
> +	count = q6v5_regulator_init(&pdev->dev, qproc->proxy_regs,
> +		desc->proxy_reg_string, (int (*)[2])desc->proxy_uV_uA);

Moving proxy_reg_string and proxy_uV_uA into a struct (as suggested in
patch 1) will make this cleaner and we get rid of that typecast.

> +	if (count < 0) {
> +		dev_err(&pdev->dev, "Failed to setup active regulators.\n");
> +		return count;
> +	}
> +	qproc->proxy_reg_count = count;
> +
> +	count = q6v5_regulator_init(&pdev->dev, qproc->active_regs,
> +		desc->active_reg_string, (int (*)[2])desc->active_uV_uA);
> +	if (count < 0) {
> +		dev_err(&pdev->dev, "Failed to setup proxy regulators.\n");
> +		return count;
> +	}
> +	qproc->active_reg_count = count;
> +
>  	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
>  	if (ret < 0)
>  		goto free_rproc;

Regards,
Bjorn

Re: [RESEND PATCH v4 2/7] remoteproc: qcom: Initialize proxy and active clock's and regulator's

[Dwivedi, Avaneesh Kumar (avani)]

On 12/9/2016 8:06 AM, Bjorn Andersson wrote:
> On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> Certain regulators and clocks need voting by rproc on behalf of hexagon
>> only during restart operation but certain clocks and voltage need to be
>> voted till hexagon is up, these regulators and clocks are identified as
>> proxy and active resource whose handle is being obtained by supplying
>> private proxy and active regulator and clock string.
>>
> Please split this patch out into the regulator and clock patches
> respectively, making each a clear functional change.
OK.
>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 148 +++++++++++++++++++++++++++----------
>>   1 file changed, 107 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 3360312..b0f0fcf 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -37,7 +37,6 @@
>>   
>>   #include <linux/qcom_scm.h>
>>   
>> -#define MBA_FIRMWARE_NAME		"mba.b00"
> Please move this to patch 1.
OK.
>
>>   #define MPSS_FIRMWARE_NAME		"modem.mdt"
>>   
>>   #define MPSS_CRASH_REASON_SMEM		421
>> @@ -132,6 +131,14 @@ struct q6v5 {
>>   	struct clk *ahb_clk;
>>   	struct clk *axi_clk;
>>   	struct clk *rom_clk;
>> +	struct clk *active_clks[8];
>> +	struct clk *proxy_clks[4];
>> +	struct reg_info active_regs[1];
>> +	struct reg_info proxy_regs[3];
>> +	int active_reg_count;
>> +	int proxy_reg_count;
>> +	int active_clk_count;
>> +	int proxy_clk_count;
> Group clocks members together and regulators together.
OK.
>
>>   
>>   	struct completion start_done;
>>   	struct completion stop_done;
>> @@ -160,27 +167,43 @@ enum {
>>   	Q6V5_SUPPLY_PLL,
>>   };
>>   
>> -static int q6v5_regulator_init(struct q6v5 *qproc)
>> +static int q6v5_regulator_init(struct device *dev,
>> +	struct reg_info *regs, char **reg_str, int volatage_load[][2])
>>   {
>> -	int ret;
>> +	int reg_count = 0, i;
> Please, one variable per line, sorted (descending) by line length.
OK.
>
>>   
>> -	qproc->supply[Q6V5_SUPPLY_CX].supply = "cx";
>> -	qproc->supply[Q6V5_SUPPLY_MX].supply = "mx";
>> -	qproc->supply[Q6V5_SUPPLY_MSS].supply = "mss";
>> -	qproc->supply[Q6V5_SUPPLY_PLL].supply = "pll";
>> +	if (!reg_str)
>> +		return 0;
>>   
>> -	ret = devm_regulator_bulk_get(qproc->dev,
>> -				      ARRAY_SIZE(qproc->supply), qproc->supply);
>> -	if (ret < 0) {
>> -		dev_err(qproc->dev, "failed to get supplies\n");
>> -		return ret;
>> -	}
>> +	while (reg_str[reg_count] != NULL)
> Drop "!= NULL" from your condition.
OK.
>
>> +		reg_count++;
>>   
>> -	regulator_set_load(qproc->supply[Q6V5_SUPPLY_CX].consumer, 100000);
>> -	regulator_set_load(qproc->supply[Q6V5_SUPPLY_MSS].consumer, 100000);
>> -	regulator_set_load(qproc->supply[Q6V5_SUPPLY_PLL].consumer, 10000);
>> +	if (!reg_count)
>> +		return reg_count;
> You can omit this check, as the for loop below will iterate 0 times and
> we will then return 0.
>
>>   
>> -	return 0;
>> +	if (!regs)
>> +		return -ENOMEM;
> This will not happen, please drop.
>
>> +
>> +	for (i = 0; i < reg_count; i++) {
>> +		const char *reg_name;
> Please keep local variables at the top of the function.
>
> And generally try to use short but clear variable names; in line with my
> suggestion in patch 1 name it "supply" (or in this case where the
> original variable is quite short just use it directly).
OK
>
>> +
>> +		reg_name = reg_str[i];
>> +		regs[i].reg = devm_regulator_get(dev, reg_name);
>> +		if (IS_ERR(regs[i].reg)) {
>> +
>> +			int rc = PTR_ERR(regs[i].reg);
>> +
>> +			if (rc != -EPROBE_DEFER)
>> +				dev_err(dev, "Failed to get %s\n regulator",
>> +								reg_name);
>> +			return rc;
>> +		}
>> +
>> +		regs[i].uV = volatage_load[i][0];
>> +		regs[i].uA = volatage_load[i][1];
>> +	}
>> +
>> +	return reg_count;
>>   }
>>   
>>   static int q6v5_regulator_enable(struct q6v5 *qproc)
>> @@ -725,27 +748,41 @@ static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev)
>>   	return 0;
>>   }
>>   
>> -static int q6v5_init_clocks(struct q6v5 *qproc)
>> +static int q6v5_init_clocks(struct device *dev, struct clk **clks,
>> +		char **clk_str)
>>   {
>> -	qproc->ahb_clk = devm_clk_get(qproc->dev, "iface");
>> -	if (IS_ERR(qproc->ahb_clk)) {
>> -		dev_err(qproc->dev, "failed to get iface clock\n");
>> -		return PTR_ERR(qproc->ahb_clk);
>> -	}
>> +	int clk_count = 0, i;
> One variable per line, please.  And "count" is unambiguous enough.
OK.
>
>>   
>> -	qproc->axi_clk = devm_clk_get(qproc->dev, "bus");
>> -	if (IS_ERR(qproc->axi_clk)) {
>> -		dev_err(qproc->dev, "failed to get bus clock\n");
>> -		return PTR_ERR(qproc->axi_clk);
>> -	}
>> +	if (!clk_str)
>> +		return 0;
>> +
>> +	while (clk_str[clk_count] != NULL)
> Drop "!= NULL" part
OK.
>
>> +		clk_count++;
>> +
>> +	if (!clk_count)
>> +		return clk_count;
> This is not needed.
Yes.
>
>> +
>> +	if (!clks)
>> +		return -ENOMEM;
> This can't happen.
YES.
>
>> +
>> +	for (i = 0; i < clk_count; i++) {
>> +		const char *clock_name;
>> +
>> +		clock_name = clk_str[i];
>> +		clks[i] = devm_clk_get(dev, clock_name);
>> +		if (IS_ERR(clks[i])) {
>> +
>> +			int rc = PTR_ERR(clks[i]);
>> +
>> +			if (rc != -EPROBE_DEFER)
>> +				dev_err(dev, "Failed to get %s clock\n",
>> +					clock_name);
>> +			return rc;
>> +		}
>>   
>> -	qproc->rom_clk = devm_clk_get(qproc->dev, "mem");
>> -	if (IS_ERR(qproc->rom_clk)) {
>> -		dev_err(qproc->dev, "failed to get mem clock\n");
>> -		return PTR_ERR(qproc->rom_clk);
>>   	}
>>   
>> -	return 0;
>> +	return clk_count;
>>   }
>>   
>>   static int q6v5_init_reset(struct q6v5 *qproc)
>> @@ -830,10 +867,15 @@ static int q6v5_probe(struct platform_device *pdev)
>>   {
>>   	struct q6v5 *qproc;
>>   	struct rproc *rproc;
>> -	int ret;
>> +	const struct rproc_hexagon_res *desc;
>> +	int ret, count;
> One variable per line please, and sort (descending) on line length.
ok,
>
>> +
>> +	desc = of_device_get_match_data(&pdev->dev);
>> +	if (!desc)
>> +		return -EINVAL;
>>   
>>   	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops,
>> -			    MBA_FIRMWARE_NAME, sizeof(*qproc));
>> +			    desc->hexagon_mba_image, sizeof(*qproc));
> Please move this to patch 1.
OK,
>
>>   	if (!rproc) {
>>   		dev_err(&pdev->dev, "failed to allocate rproc\n");
>>   		return -ENOMEM;
>> @@ -857,18 +899,42 @@ static int q6v5_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto free_rproc;
>>   
>> -	if (ret)
>> -		goto free_rproc;
>> +	count = q6v5_init_clocks(&pdev->dev, qproc->proxy_clks,
>> +		desc->proxy_clk_string);
> Your "count" serves the same purpose as "ret", so just use "ret".
> And align broken lines with the parenthesis.
OK.
>
>> +	if (count < 0) {
>> +		dev_err(&pdev->dev, "Failed to setup proxy clocks.\n");
>> +		return count;
> And using "ret" instead of "count" makes it easy to goto free_rproc,
> which you must do after calling rproc_alloc().
OK.
>
>> +	}
>> +	qproc->proxy_clk_count = count;
>>   
>> -	ret = q6v5_regulator_init(qproc);
>> -	if (ret)
>> -		goto free_rproc;
>> +	count = q6v5_init_clocks(&pdev->dev, qproc->active_clks,
>> +		desc->active_clk_string);
>> +	if (count < 0) {
>> +		dev_err(&pdev->dev, "Failed to setup active clocks.\n");
>> +		return count;
>> +	}
>> +	qproc->active_clk_count = count;
>>   
>>   	ret = q6v5_init_reset(qproc);
>>   	if (ret)
>>   		goto free_rproc;
>>   
>> +	count = q6v5_regulator_init(&pdev->dev, qproc->proxy_regs,
>> +		desc->proxy_reg_string, (int (*)[2])desc->proxy_uV_uA);
> Moving proxy_reg_string and proxy_uV_uA into a struct (as suggested in
> patch 1) will make this cleaner and we get rid of that typecast.
OK.
>
>> +	if (count < 0) {
>> +		dev_err(&pdev->dev, "Failed to setup active regulators.\n");
>> +		return count;
>> +	}
>> +	qproc->proxy_reg_count = count;
>> +
>> +	count = q6v5_regulator_init(&pdev->dev, qproc->active_regs,
>> +		desc->active_reg_string, (int (*)[2])desc->active_uV_uA);
>> +	if (count < 0) {
>> +		dev_err(&pdev->dev, "Failed to setup proxy regulators.\n");
>> +		return count;
>> +	}
>> +	qproc->active_reg_count = count;
>> +
>>   	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
>>   	if (ret < 0)
>>   		goto free_rproc;
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[RESEND PATCH v4 3/7] remoteproc: qcom: Modify regulator enable and disable interface

[Avaneesh Kumar Dwivedi]

Regulator enable routine will get additional input parameter of
regulator info and count, It will read regulator info and will do
appropriate voltage and load configuration before turning them up.
Also separate out disable interface into proxy and active disable
so that on arrival of handover interrupt proxy regulators alone
could be voted out.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 113 ++++++++++++++++++++++++++++---------
 1 file changed, 86 insertions(+), 27 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index b0f0fcf..06d5bb2 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -126,7 +126,6 @@ struct q6v5 {
 	struct qcom_smem_state *state;
 	unsigned stop_bit;
 
-	struct regulator_bulk_data supply[4];
 
 	struct clk *ahb_clk;
 	struct clk *axi_clk;
@@ -160,13 +159,6 @@ enum {
 	Q5V56_1_5_0, /*hexagon on msm8996*/
 };
 
-enum {
-	Q6V5_SUPPLY_CX,
-	Q6V5_SUPPLY_MX,
-	Q6V5_SUPPLY_MSS,
-	Q6V5_SUPPLY_PLL,
-};
-
 static int q6v5_regulator_init(struct device *dev,
 	struct reg_info *regs, char **reg_str, int volatage_load[][2])
 {
@@ -206,35 +198,93 @@ static int q6v5_regulator_init(struct device *dev,
 	return reg_count;
 }
 
-static int q6v5_regulator_enable(struct q6v5 *qproc)
+static int q6v5_regulator_enable(struct q6v5 *qproc,
+	struct reg_info *regs, int count)
 {
-	struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer;
-	struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer;
-	int ret;
+	int i, rc = 0;
+
+	for (i = 0; i < count; i++) {
+		if (regs[i].uV > 0) {
+			rc = regulator_set_voltage(regs[i].reg,
+					regs[i].uV, INT_MAX);
+			if (rc) {
+				dev_err(qproc->dev,
+					"Failed to request voltage for %d.\n",
+						i);
+				goto err;
+			}
+		}
 
-	/* TODO: Q6V5_SUPPLY_CX is supposed to be set to super-turbo here */
+		if (regs[i].uA > 0) {
+			rc = regulator_set_load(regs[i].reg,
+						regs[i].uA);
+			if (rc < 0) {
+				dev_err(qproc->dev, "Failed to set regulator mode\n");
+				goto err;
+			}
+		}
 
-	ret = regulator_set_voltage(mx, 1050000, INT_MAX);
-	if (ret)
-		return ret;
+		rc = regulator_enable(regs[i].reg);
+		if (rc) {
+			dev_err(qproc->dev, "Regulator enable failed\n");
+			goto err;
+		}
+	}
+
+	return 0;
+err:
+	for (; i >= 0; i--) {
+		if (regs[i].uV > 0)
+			regulator_set_voltage(regs[i].reg, 0, INT_MAX);
 
-	regulator_set_voltage(mss, 1000000, 1150000);
+		if (regs[i].uA > 0)
+			regulator_set_load(regs[i].reg, 0);
+
+		regulator_disable(regs[i].reg);
+	}
 
-	return regulator_bulk_enable(ARRAY_SIZE(qproc->supply), qproc->supply);
+	return rc;
 }
 
-static void q6v5_regulator_disable(struct q6v5 *qproc)
+static void q6v5_proxy_regulator_disable(struct q6v5 *qproc)
 {
-	struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer;
-	struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer;
+	int i;
+	struct reg_info *regs = qproc->proxy_regs;
 
-	/* TODO: Q6V5_SUPPLY_CX corner votes should be released */
+	for (i = 0; i < qproc->proxy_reg_count; i++) {
+		if (regs[i].uV > 0)
+			regulator_set_voltage(regs[i].reg, 0, INT_MAX);
 
-	regulator_bulk_disable(ARRAY_SIZE(qproc->supply), qproc->supply);
-	regulator_set_voltage(mx, 0, INT_MAX);
-	regulator_set_voltage(mss, 0, 1150000);
+		if (regs[i].uA > 0)
+			regulator_set_load(regs[i].reg, 0);
+
+		regulator_disable(regs[i].reg);
+	}
 }
 
+static void q6v5_active_regulator_disable(struct q6v5 *qproc)
+{
+	int i;
+	struct reg_info *regs = qproc->active_regs;
+
+	for (i = 0; i < qproc->active_reg_count; i++) {
+		if (regs[i].uV > 0)
+			regulator_set_voltage(regs[i].reg, 0, INT_MAX);
+
+		if (regs[i].uA > 0)
+			regulator_set_load(regs[i].reg, 0);
+
+		regulator_disable(regs[i].reg);
+	}
+}
+
+static void q6v5_regulator_disable(struct q6v5 *qproc)
+{
+	q6v5_proxy_regulator_disable(qproc);
+	q6v5_active_regulator_disable(qproc);
+}
+
+
 static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
 {
 	struct q6v5 *qproc = rproc->priv;
@@ -524,12 +574,19 @@ static int q6v5_start(struct rproc *rproc)
 	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
 	int ret;
 
-	ret = q6v5_regulator_enable(qproc);
+	ret = q6v5_regulator_enable(qproc, qproc->proxy_regs,
+		qproc->proxy_reg_count);
 	if (ret) {
-		dev_err(qproc->dev, "failed to enable supplies\n");
+		dev_err(qproc->dev, "failed to enable proxy supplies\n");
 		return ret;
 	}
 
+	ret = q6v5_regulator_enable(qproc, qproc->active_regs,
+		qproc->active_reg_count);
+	if (ret) {
+		dev_err(qproc->dev, "failed to enable supplies\n");
+		goto disable_proxy_reg;
+	}
 	ret = reset_control_deassert(qproc->mss_restart);
 	if (ret) {
 		dev_err(qproc->dev, "failed to deassert mss restart\n");
@@ -600,6 +657,8 @@ static int q6v5_start(struct rproc *rproc)
 disable_vdd:
 	q6v5_regulator_disable(qproc);
 
+disable_proxy_reg:
+	q6v5_proxy_regulator_disable(qproc);
 	return ret;
 }
 
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [RESEND PATCH v4 3/7] remoteproc: qcom: Modify regulator enable and disable interface

[Bjorn Andersson]

On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:

> Regulator enable routine will get additional input parameter of
> regulator info and count, It will read regulator info and will do
> appropriate voltage and load configuration before turning them up.
> Also separate out disable interface into proxy and active disable
> so that on arrival of handover interrupt proxy regulators alone
> could be voted out.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 113 ++++++++++++++++++++++++++++---------
>  1 file changed, 86 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index b0f0fcf..06d5bb2 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -126,7 +126,6 @@ struct q6v5 {
>  	struct qcom_smem_state *state;
>  	unsigned stop_bit;
>  
> -	struct regulator_bulk_data supply[4];
>  
>  	struct clk *ahb_clk;
>  	struct clk *axi_clk;
> @@ -160,13 +159,6 @@ enum {
>  	Q5V56_1_5_0, /*hexagon on msm8996*/
>  };
>  
> -enum {
> -	Q6V5_SUPPLY_CX,
> -	Q6V5_SUPPLY_MX,
> -	Q6V5_SUPPLY_MSS,
> -	Q6V5_SUPPLY_PLL,
> -};
> -
>  static int q6v5_regulator_init(struct device *dev,
>  	struct reg_info *regs, char **reg_str, int volatage_load[][2])
>  {
> @@ -206,35 +198,93 @@ static int q6v5_regulator_init(struct device *dev,
>  	return reg_count;
>  }
>  
> -static int q6v5_regulator_enable(struct q6v5 *qproc)
> +static int q6v5_regulator_enable(struct q6v5 *qproc,
> +	struct reg_info *regs, int count)
>  {
> -	struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer;
> -	struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer;
> -	int ret;
> +	int i, rc = 0;

One variable per line, no need to replace "ret" with "rc" and no need to
initialize it to 0 as it won't be read before written in any code path.

> +
> +	for (i = 0; i < count; i++) {
> +		if (regs[i].uV > 0) {
> +			rc = regulator_set_voltage(regs[i].reg,
> +					regs[i].uV, INT_MAX);
> +			if (rc) {
> +				dev_err(qproc->dev,
> +					"Failed to request voltage for %d.\n",
> +						i);
> +				goto err;
> +			}
> +		}
>  
> -	/* TODO: Q6V5_SUPPLY_CX is supposed to be set to super-turbo here */
> +		if (regs[i].uA > 0) {
> +			rc = regulator_set_load(regs[i].reg,
> +						regs[i].uA);
> +			if (rc < 0) {
> +				dev_err(qproc->dev, "Failed to set regulator mode\n");
> +				goto err;
> +			}
> +		}
>  
> -	ret = regulator_set_voltage(mx, 1050000, INT_MAX);
> -	if (ret)
> -		return ret;
> +		rc = regulator_enable(regs[i].reg);
> +		if (rc) {
> +			dev_err(qproc->dev, "Regulator enable failed\n");
> +			goto err;
> +		}
> +	}
> +
> +	return 0;
> +err:
> +	for (; i >= 0; i--) {
> +		if (regs[i].uV > 0)
> +			regulator_set_voltage(regs[i].reg, 0, INT_MAX);
>  
> -	regulator_set_voltage(mss, 1000000, 1150000);
> +		if (regs[i].uA > 0)
> +			regulator_set_load(regs[i].reg, 0);
> +
> +		regulator_disable(regs[i].reg);
> +	}
>  
> -	return regulator_bulk_enable(ARRAY_SIZE(qproc->supply), qproc->supply);
> +	return rc;
>  }
>  
> -static void q6v5_regulator_disable(struct q6v5 *qproc)
> +static void q6v5_proxy_regulator_disable(struct q6v5 *qproc)

Please have follow the same scheme as for enable, with a function taking
the list of regulators and count.

>  {
> -	struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer;
> -	struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer;
> +	int i;
> +	struct reg_info *regs = qproc->proxy_regs;
>  
> -	/* TODO: Q6V5_SUPPLY_CX corner votes should be released */
> +	for (i = 0; i < qproc->proxy_reg_count; i++) {
> +		if (regs[i].uV > 0)
> +			regulator_set_voltage(regs[i].reg, 0, INT_MAX);
>  
> -	regulator_bulk_disable(ARRAY_SIZE(qproc->supply), qproc->supply);
> -	regulator_set_voltage(mx, 0, INT_MAX);
> -	regulator_set_voltage(mss, 0, 1150000);
> +		if (regs[i].uA > 0)
> +			regulator_set_load(regs[i].reg, 0);
> +
> +		regulator_disable(regs[i].reg);
> +	}
>  }
>  
> +static void q6v5_active_regulator_disable(struct q6v5 *qproc)
> +{
> +	int i;
> +	struct reg_info *regs = qproc->active_regs;
> +
> +	for (i = 0; i < qproc->active_reg_count; i++) {
> +		if (regs[i].uV > 0)
> +			regulator_set_voltage(regs[i].reg, 0, INT_MAX);
> +
> +		if (regs[i].uA > 0)
> +			regulator_set_load(regs[i].reg, 0);
> +
> +		regulator_disable(regs[i].reg);
> +	}
> +}
> +
> +static void q6v5_regulator_disable(struct q6v5 *qproc)
> +{
> +	q6v5_proxy_regulator_disable(qproc);
> +	q6v5_active_regulator_disable(qproc);
> +}
> +
> +
>  static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>  {
>  	struct q6v5 *qproc = rproc->priv;
> @@ -524,12 +574,19 @@ static int q6v5_start(struct rproc *rproc)
>  	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>  	int ret;
>  
> -	ret = q6v5_regulator_enable(qproc);
> +	ret = q6v5_regulator_enable(qproc, qproc->proxy_regs,
> +		qproc->proxy_reg_count);

Align indentation with parenthesis on previous line.

>  	if (ret) {
> -		dev_err(qproc->dev, "failed to enable supplies\n");
> +		dev_err(qproc->dev, "failed to enable proxy supplies\n");
>  		return ret;
>  	}
>  
> +	ret = q6v5_regulator_enable(qproc, qproc->active_regs,
> +		qproc->active_reg_count);
> +	if (ret) {
> +		dev_err(qproc->dev, "failed to enable supplies\n");
> +		goto disable_proxy_reg;
> +	}
>  	ret = reset_control_deassert(qproc->mss_restart);
>  	if (ret) {
>  		dev_err(qproc->dev, "failed to deassert mss restart\n");
> @@ -600,6 +657,8 @@ static int q6v5_start(struct rproc *rproc)
>  disable_vdd:
>  	q6v5_regulator_disable(qproc);

Here you disable both active and proxy regulators, then you call
through...

>  
> +disable_proxy_reg:
> +	q6v5_proxy_regulator_disable(qproc);

...and once again disable proxy regulators.

Remove q6v5_regulator_disable() and just call the active and proxy
disable functions directly.

>  	return ret;
>  }
>  

Regards,
Bjorn

Re: [RESEND PATCH v4 3/7] remoteproc: qcom: Modify regulator enable and disable interface

[Dwivedi, Avaneesh Kumar (avani)]

On 12/9/2016 8:14 AM, Bjorn Andersson wrote:
> On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> Regulator enable routine will get additional input parameter of
>> regulator info and count, It will read regulator info and will do
>> appropriate voltage and load configuration before turning them up.
>> Also separate out disable interface into proxy and active disable
>> so that on arrival of handover interrupt proxy regulators alone
>> could be voted out.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 113 ++++++++++++++++++++++++++++---------
>>   1 file changed, 86 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index b0f0fcf..06d5bb2 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -126,7 +126,6 @@ struct q6v5 {
>>   	struct qcom_smem_state *state;
>>   	unsigned stop_bit;
>>   
>> -	struct regulator_bulk_data supply[4];
>>   
>>   	struct clk *ahb_clk;
>>   	struct clk *axi_clk;
>> @@ -160,13 +159,6 @@ enum {
>>   	Q5V56_1_5_0, /*hexagon on msm8996*/
>>   };
>>   
>> -enum {
>> -	Q6V5_SUPPLY_CX,
>> -	Q6V5_SUPPLY_MX,
>> -	Q6V5_SUPPLY_MSS,
>> -	Q6V5_SUPPLY_PLL,
>> -};
>> -
>>   static int q6v5_regulator_init(struct device *dev,
>>   	struct reg_info *regs, char **reg_str, int volatage_load[][2])
>>   {
>> @@ -206,35 +198,93 @@ static int q6v5_regulator_init(struct device *dev,
>>   	return reg_count;
>>   }
>>   
>> -static int q6v5_regulator_enable(struct q6v5 *qproc)
>> +static int q6v5_regulator_enable(struct q6v5 *qproc,
>> +	struct reg_info *regs, int count)
>>   {
>> -	struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer;
>> -	struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer;
>> -	int ret;
>> +	int i, rc = 0;
> One variable per line, no need to replace "ret" with "rc" and no need to
> initialize it to 0 as it won't be read before written in any code path.
OK.
>
>> +
>> +	for (i = 0; i < count; i++) {
>> +		if (regs[i].uV > 0) {
>> +			rc = regulator_set_voltage(regs[i].reg,
>> +					regs[i].uV, INT_MAX);
>> +			if (rc) {
>> +				dev_err(qproc->dev,
>> +					"Failed to request voltage for %d.\n",
>> +						i);
>> +				goto err;
>> +			}
>> +		}
>>   
>> -	/* TODO: Q6V5_SUPPLY_CX is supposed to be set to super-turbo here */
>> +		if (regs[i].uA > 0) {
>> +			rc = regulator_set_load(regs[i].reg,
>> +						regs[i].uA);
>> +			if (rc < 0) {
>> +				dev_err(qproc->dev, "Failed to set regulator mode\n");
>> +				goto err;
>> +			}
>> +		}
>>   
>> -	ret = regulator_set_voltage(mx, 1050000, INT_MAX);
>> -	if (ret)
>> -		return ret;
>> +		rc = regulator_enable(regs[i].reg);
>> +		if (rc) {
>> +			dev_err(qproc->dev, "Regulator enable failed\n");
>> +			goto err;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +err:
>> +	for (; i >= 0; i--) {
>> +		if (regs[i].uV > 0)
>> +			regulator_set_voltage(regs[i].reg, 0, INT_MAX);
>>   
>> -	regulator_set_voltage(mss, 1000000, 1150000);
>> +		if (regs[i].uA > 0)
>> +			regulator_set_load(regs[i].reg, 0);
>> +
>> +		regulator_disable(regs[i].reg);
>> +	}
>>   
>> -	return regulator_bulk_enable(ARRAY_SIZE(qproc->supply), qproc->supply);
>> +	return rc;
>>   }
>>   
>> -static void q6v5_regulator_disable(struct q6v5 *qproc)
>> +static void q6v5_proxy_regulator_disable(struct q6v5 *qproc)
> Please have follow the same scheme as for enable, with a function taking
> the list of regulators and count.
Ok.
>
>>   {
>> -	struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer;
>> -	struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer;
>> +	int i;
>> +	struct reg_info *regs = qproc->proxy_regs;
>>   
>> -	/* TODO: Q6V5_SUPPLY_CX corner votes should be released */
>> +	for (i = 0; i < qproc->proxy_reg_count; i++) {
>> +		if (regs[i].uV > 0)
>> +			regulator_set_voltage(regs[i].reg, 0, INT_MAX);
>>   
>> -	regulator_bulk_disable(ARRAY_SIZE(qproc->supply), qproc->supply);
>> -	regulator_set_voltage(mx, 0, INT_MAX);
>> -	regulator_set_voltage(mss, 0, 1150000);
>> +		if (regs[i].uA > 0)
>> +			regulator_set_load(regs[i].reg, 0);
>> +
>> +		regulator_disable(regs[i].reg);
>> +	}
>>   }
>>   
>> +static void q6v5_active_regulator_disable(struct q6v5 *qproc)
>> +{
>> +	int i;
>> +	struct reg_info *regs = qproc->active_regs;
>> +
>> +	for (i = 0; i < qproc->active_reg_count; i++) {
>> +		if (regs[i].uV > 0)
>> +			regulator_set_voltage(regs[i].reg, 0, INT_MAX);
>> +
>> +		if (regs[i].uA > 0)
>> +			regulator_set_load(regs[i].reg, 0);
>> +
>> +		regulator_disable(regs[i].reg);
>> +	}
>> +}
>> +
>> +static void q6v5_regulator_disable(struct q6v5 *qproc)
>> +{
>> +	q6v5_proxy_regulator_disable(qproc);
>> +	q6v5_active_regulator_disable(qproc);
>> +}
>> +
>> +
>>   static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>>   {
>>   	struct q6v5 *qproc = rproc->priv;
>> @@ -524,12 +574,19 @@ static int q6v5_start(struct rproc *rproc)
>>   	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>>   	int ret;
>>   
>> -	ret = q6v5_regulator_enable(qproc);
>> +	ret = q6v5_regulator_enable(qproc, qproc->proxy_regs,
>> +		qproc->proxy_reg_count);
> Align indentation with parenthesis on previous line.
Ok.
>
>>   	if (ret) {
>> -		dev_err(qproc->dev, "failed to enable supplies\n");
>> +		dev_err(qproc->dev, "failed to enable proxy supplies\n");
>>   		return ret;
>>   	}
>>   
>> +	ret = q6v5_regulator_enable(qproc, qproc->active_regs,
>> +		qproc->active_reg_count);
>> +	if (ret) {
>> +		dev_err(qproc->dev, "failed to enable supplies\n");
>> +		goto disable_proxy_reg;
>> +	}
>>   	ret = reset_control_deassert(qproc->mss_restart);
>>   	if (ret) {
>>   		dev_err(qproc->dev, "failed to deassert mss restart\n");
>> @@ -600,6 +657,8 @@ static int q6v5_start(struct rproc *rproc)
>>   disable_vdd:
>>   	q6v5_regulator_disable(qproc);
> Here you disable both active and proxy regulators, then you call
> through...
Ok.
>
>>   
>> +disable_proxy_reg:
>> +	q6v5_proxy_regulator_disable(qproc);
> ...and once again disable proxy regulators.
>
> Remove q6v5_regulator_disable() and just call the active and proxy
> disable functions directly.
Ok. Using single function to disable regulator but passing proxy or 
active regulator info as necessary.
>
>>   	return ret;
>>   }
>>   
> Regards,
> Bjorn

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[RESEND PATCH v4 4/7] remoteproc: qcom: Modify clock enable and disable routine

[Avaneesh Kumar Dwivedi]

Clock handling is made generic than earlier way to add new clock
every time when required to support new hexagon version. Also
clock disable interface is separated out into two separate routine
to unvote proxy clock alone when handover interrupt is arrived.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 93 ++++++++++++++++++++++++++------------
 1 file changed, 65 insertions(+), 28 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 06d5bb2..c55dc9a 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -126,10 +126,6 @@ struct q6v5 {
 	struct qcom_smem_state *state;
 	unsigned stop_bit;
 
-
-	struct clk *ahb_clk;
-	struct clk *axi_clk;
-	struct clk *rom_clk;
 	struct clk *active_clks[8];
 	struct clk *proxy_clks[4];
 	struct reg_info active_regs[1];
@@ -284,6 +280,51 @@ static void q6v5_regulator_disable(struct q6v5 *qproc)
 	q6v5_active_regulator_disable(qproc);
 }
 
+static int q6v5_clk_enable(struct device *dev, struct clk **clks,
+								int clk_count)
+{
+	int rc = 0;
+	int i;
+
+	for (i = 0; i < clk_count; i++) {
+		rc = clk_prepare_enable(clks[i]);
+		if (rc) {
+			dev_err(dev, "Clock enable failed\n");
+			goto err;
+		}
+	}
+
+	return 0;
+err:
+	for (i--; i >= 0; i--)
+		clk_disable_unprepare(clks[i]);
+
+	return rc;
+}
+
+static void q6v5_proxy_clk_disable(struct q6v5 *qproc)
+{
+	int i;
+	struct clk **clks = qproc->proxy_clks;
+
+	for (i = 0; i < qproc->proxy_clk_count; i++)
+		clk_disable_unprepare(clks[i]);
+}
+
+static void q6v5_active_clk_disable(struct q6v5 *qproc)
+{
+	int i;
+	struct clk **clks = qproc->proxy_clks;
+
+	for (i = 0; i < qproc->proxy_clk_count; i++)
+		clk_disable_unprepare(clks[i]);
+}
+
+static void q6v5_clk_disable(struct q6v5 *qproc)
+{
+	q6v5_proxy_clk_disable(qproc);
+	q6v5_active_clk_disable(qproc);
+}
 
 static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
 {
@@ -581,11 +622,18 @@ static int q6v5_start(struct rproc *rproc)
 		return ret;
 	}
 
+	ret = q6v5_clk_enable(qproc->dev, qproc->proxy_clks,
+		qproc->proxy_clk_count);
+	if (ret) {
+		dev_err(qproc->dev, "failed to enable proxy clocks\n");
+		goto disable_proxy_reg;
+	}
+
 	ret = q6v5_regulator_enable(qproc, qproc->active_regs,
 		qproc->active_reg_count);
 	if (ret) {
 		dev_err(qproc->dev, "failed to enable supplies\n");
-		goto disable_proxy_reg;
+		goto disable_proxy_clk;
 	}
 	ret = reset_control_deassert(qproc->mss_restart);
 	if (ret) {
@@ -593,17 +641,12 @@ static int q6v5_start(struct rproc *rproc)
 		goto disable_vdd;
 	}
 
-	ret = clk_prepare_enable(qproc->ahb_clk);
-	if (ret)
+	ret = q6v5_clk_enable(qproc->dev, qproc->active_clks,
+		qproc->active_clk_count);
+	if (ret) {
+		dev_err(qproc->dev, "failed to enable clocks\n");
 		goto assert_reset;
-
-	ret = clk_prepare_enable(qproc->axi_clk);
-	if (ret)
-		goto disable_ahb_clk;
-
-	ret = clk_prepare_enable(qproc->rom_clk);
-	if (ret)
-		goto disable_axi_clk;
+	}
 
 	writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
 
@@ -638,25 +681,21 @@ static int q6v5_start(struct rproc *rproc)
 
 	qproc->running = true;
 
-	/* TODO: All done, release the handover resources */
-
+	q6v5_proxy_clk_disable(qproc);
+	q6v5_proxy_regulator_disable(qproc);
 	return 0;
 
 halt_axi_ports:
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
-
-	clk_disable_unprepare(qproc->rom_clk);
-disable_axi_clk:
-	clk_disable_unprepare(qproc->axi_clk);
-disable_ahb_clk:
-	clk_disable_unprepare(qproc->ahb_clk);
+	q6v5_active_clk_disable(qproc);
 assert_reset:
 	reset_control_assert(qproc->mss_restart);
 disable_vdd:
-	q6v5_regulator_disable(qproc);
-
+	q6v5_active_regulator_disable(qproc);
+disable_proxy_clk:
+	q6v5_proxy_clk_disable(qproc);
 disable_proxy_reg:
 	q6v5_proxy_regulator_disable(qproc);
 	return ret;
@@ -684,9 +723,7 @@ static int q6v5_stop(struct rproc *rproc)
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
 
 	reset_control_assert(qproc->mss_restart);
-	clk_disable_unprepare(qproc->rom_clk);
-	clk_disable_unprepare(qproc->axi_clk);
-	clk_disable_unprepare(qproc->ahb_clk);
+	q6v5_clk_disable(qproc);
 	q6v5_regulator_disable(qproc);
 
 	return 0;
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [RESEND PATCH v4 4/7] remoteproc: qcom: Modify clock enable and disable routine

[Bjorn Andersson]

On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:

> Clock handling is made generic than earlier way to add new clock
> every time when required to support new hexagon version. Also
> clock disable interface is separated out into two separate routine
> to unvote proxy clock alone when handover interrupt is arrived.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 93 ++++++++++++++++++++++++++------------
>  1 file changed, 65 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 06d5bb2..c55dc9a 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -126,10 +126,6 @@ struct q6v5 {
>  	struct qcom_smem_state *state;
>  	unsigned stop_bit;
>  
> -
> -	struct clk *ahb_clk;
> -	struct clk *axi_clk;
> -	struct clk *rom_clk;
>  	struct clk *active_clks[8];
>  	struct clk *proxy_clks[4];
>  	struct reg_info active_regs[1];
> @@ -284,6 +280,51 @@ static void q6v5_regulator_disable(struct q6v5 *qproc)
>  	q6v5_active_regulator_disable(qproc);
>  }
>  
> +static int q6v5_clk_enable(struct device *dev, struct clk **clks,
> +								int clk_count)
> +{
> +	int rc = 0;

No need to initialize to 0.

> +	int i;
> +
> +	for (i = 0; i < clk_count; i++) {
> +		rc = clk_prepare_enable(clks[i]);
> +		if (rc) {
> +			dev_err(dev, "Clock enable failed\n");
> +			goto err;
> +		}
> +	}
> +
> +	return 0;
> +err:
> +	for (i--; i >= 0; i--)
> +		clk_disable_unprepare(clks[i]);
> +
> +	return rc;
> +}
> +
> +static void q6v5_proxy_clk_disable(struct q6v5 *qproc)

Please make these follow the enable case, by taking the clock list and
size as parameters.

> +{
> +	int i;
> +	struct clk **clks = qproc->proxy_clks;
> +
> +	for (i = 0; i < qproc->proxy_clk_count; i++)
> +		clk_disable_unprepare(clks[i]);
> +}
> +
> +static void q6v5_active_clk_disable(struct q6v5 *qproc)
> +{
> +	int i;
> +	struct clk **clks = qproc->proxy_clks;
> +
> +	for (i = 0; i < qproc->proxy_clk_count; i++)
> +		clk_disable_unprepare(clks[i]);
> +}
> +
> +static void q6v5_clk_disable(struct q6v5 *qproc)
> +{
> +	q6v5_proxy_clk_disable(qproc);
> +	q6v5_active_clk_disable(qproc);
> +}

Just inline the two disable calls where you need them, rather than
having this wrapper.

Regards,
Bjorn

Re: [RESEND PATCH v4 4/7] remoteproc: qcom: Modify clock enable and disable routine

[Dwivedi, Avaneesh Kumar (avani)]

On 12/9/2016 8:27 AM, Bjorn Andersson wrote:
> On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> Clock handling is made generic than earlier way to add new clock
>> every time when required to support new hexagon version. Also
>> clock disable interface is separated out into two separate routine
>> to unvote proxy clock alone when handover interrupt is arrived.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 93 ++++++++++++++++++++++++++------------
>>   1 file changed, 65 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 06d5bb2..c55dc9a 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -126,10 +126,6 @@ struct q6v5 {
>>   	struct qcom_smem_state *state;
>>   	unsigned stop_bit;
>>   
>> -
>> -	struct clk *ahb_clk;
>> -	struct clk *axi_clk;
>> -	struct clk *rom_clk;
>>   	struct clk *active_clks[8];
>>   	struct clk *proxy_clks[4];
>>   	struct reg_info active_regs[1];
>> @@ -284,6 +280,51 @@ static void q6v5_regulator_disable(struct q6v5 *qproc)
>>   	q6v5_active_regulator_disable(qproc);
>>   }
>>   
>> +static int q6v5_clk_enable(struct device *dev, struct clk **clks,
>> +								int clk_count)
>> +{
>> +	int rc = 0;
> No need to initialize to 0.
OK.
>
>> +	int i;
>> +
>> +	for (i = 0; i < clk_count; i++) {
>> +		rc = clk_prepare_enable(clks[i]);
>> +		if (rc) {
>> +			dev_err(dev, "Clock enable failed\n");
>> +			goto err;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +err:
>> +	for (i--; i >= 0; i--)
>> +		clk_disable_unprepare(clks[i]);
>> +
>> +	return rc;
>> +}
>> +
>> +static void q6v5_proxy_clk_disable(struct q6v5 *qproc)
> Please make these follow the enable case, by taking the clock list and
> size as parameters.
OK.
>
>> +{
>> +	int i;
>> +	struct clk **clks = qproc->proxy_clks;
>> +
>> +	for (i = 0; i < qproc->proxy_clk_count; i++)
>> +		clk_disable_unprepare(clks[i]);
>> +}
>> +
>> +static void q6v5_active_clk_disable(struct q6v5 *qproc)
>> +{
>> +	int i;
>> +	struct clk **clks = qproc->proxy_clks;
>> +
>> +	for (i = 0; i < qproc->proxy_clk_count; i++)
>> +		clk_disable_unprepare(clks[i]);
>> +}
>> +
>> +static void q6v5_clk_disable(struct q6v5 *qproc)
>> +{
>> +	q6v5_proxy_clk_disable(qproc);
>> +	q6v5_active_clk_disable(qproc);
>> +}
> Just inline the two disable calls where you need them, rather than
> having this wrapper.
OK.
>
> Regards,
> Bjorn

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[RESEND PATCH v4 5/7] remoteproc: qcom: Modify reset sequence for hexagon to support v56 1.5.0

[Avaneesh Kumar Dwivedi]

This change introduces appropriate additional steps in reset sequence so
that hexagon v56 1.5.0 is brough out of reset.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 125 ++++++++++++++++++++++++++++++-------
 1 file changed, 104 insertions(+), 21 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index c55dc9a..7ea2f53 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -65,6 +65,8 @@
 #define QDSP6SS_RESET_REG		0x014
 #define QDSP6SS_GFMUX_CTL_REG		0x020
 #define QDSP6SS_PWR_CTL_REG		0x030
+#define QDSP6SS_MEM_PWR_CTL		0x0B0
+#define QDSP6SS_STRAP_ACC		0x110
 
 /* AXI Halt Register Offsets */
 #define AXI_HALTREQ_REG			0x0
@@ -93,6 +95,15 @@
 #define QDSS_BHS_ON			BIT(21)
 #define QDSS_LDO_BYP			BIT(22)
 
+/* QDSP6v56 parameters */
+#define QDSP6v56_LDO_BYP                BIT(25)
+#define QDSP6v56_BHS_ON                 BIT(24)
+#define QDSP6v56_CLAMP_WL               BIT(21)
+#define QDSP6v56_CLAMP_QMC_MEM          BIT(22)
+#define HALT_CHECK_MAX_LOOPS            (200)
+#define QDSP6SS_XO_CBCR                 (0x0038)
+#define QDSP6SS_ACC_OVERRIDE_VAL	0x20
+
 struct rproc_hexagon_res {
 	char *hexagon_mba_image;
 	char **proxy_reg_string;
@@ -147,6 +158,8 @@ struct q6v5 {
 	phys_addr_t mpss_reloc;
 	void *mpss_region;
 	size_t mpss_size;
+
+	int hexagon_ver;
 };
 
 enum {
@@ -388,35 +401,103 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
 
 static int q6v5proc_reset(struct q6v5 *qproc)
 {
-	u32 val;
-	int ret;
+	u64 val;
+	int ret, i, count;
+
+	/* Override the ACC value if required */
+	if (qproc->hexagon_ver & 0x2)
+		writel_relaxed(QDSP6SS_ACC_OVERRIDE_VAL,
+				qproc->reg_base + QDSP6SS_STRAP_ACC);
 
 	/* Assert resets, stop core */
 	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
 	val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
 	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
 
+	/* BHS require xo cbcr to be enabled */
+	if (qproc->hexagon_ver & 0x2) {
+		val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
+		val |= 0x1;
+		writel_relaxed(val, qproc->reg_base + QDSP6SS_XO_CBCR);
+		for (count = HALT_CHECK_MAX_LOOPS; count > 0; count--) {
+			val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
+			if (!(val & BIT(31)))
+				break;
+			udelay(1);
+		}
+
+		val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
+		if ((val & BIT(31)))
+			dev_err(qproc->dev, "Failed to enable xo branch clock.\n");
+	}
+
+	if (qproc->hexagon_ver & 0x2) {
 	/* Enable power block headswitch, and wait for it to stabilize */
-	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= QDSS_BHS_ON | QDSS_LDO_BYP;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	udelay(1);
-
-	/*
-	 * Turn on memories. L2 banks should be done individually
-	 * to minimize inrush current.
-	 */
-	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
-		Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= Q6SS_L2DATA_SLP_NRET_N_2;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= Q6SS_L2DATA_SLP_NRET_N_1;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= Q6SS_L2DATA_SLP_NRET_N_0;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= QDSP6v56_BHS_ON;
+		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		udelay(1);
 
+		/* Put LDO in bypass mode */
+		val |= QDSP6v56_LDO_BYP;
+		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	} else {
+		/*
+		 * Enable power block headswitch,
+		 * and wait for it to stabilize
+		 */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= QDSS_BHS_ON | QDSS_LDO_BYP;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		udelay(1);
+	}
+
+	if (qproc->hexagon_ver & 0x2) {
+		/*
+		 * Deassert QDSP6 compiler memory clamp
+		 */
+		val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val &= ~QDSP6v56_CLAMP_QMC_MEM;
+		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+		/* Deassert memory peripheral sleep and L2 memory standby */
+		val |= (Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N);
+		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+		/* Turn on L1, L2, ETB and JU memories 1 at a time */
+		val = readl_relaxed(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+		for (i = 19; i >= 0; i--) {
+			val |= BIT(i);
+			writel_relaxed(val, qproc->reg_base +
+						QDSP6SS_MEM_PWR_CTL);
+			/*
+			 * Wait for 1us for both memory peripheral and
+			 * data array to turn on.
+			 */
+			 mb();
+			udelay(1);
+		}
+		/* Remove word line clamp */
+		val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val &= ~QDSP6v56_CLAMP_WL;
+		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	} else {
+		/*
+		 * Turn on memories. L2 banks should be done individually
+		 * to minimize inrush current.
+		 */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
+			Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= Q6SS_L2DATA_SLP_NRET_N_2;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= Q6SS_L2DATA_SLP_NRET_N_1;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= Q6SS_L2DATA_SLP_NRET_N_0;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	}
 	/* Remove IO clamp */
 	val &= ~Q6SS_CLAMP_IO;
 	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
@@ -1031,6 +1112,8 @@ static int q6v5_probe(struct platform_device *pdev)
 	}
 	qproc->active_reg_count = count;
 
+	qproc->hexagon_ver = desc->hexagon_ver;
+
 	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
 	if (ret < 0)
 		goto free_rproc;
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [RESEND PATCH v4 5/7] remoteproc: qcom: Modify reset sequence for hexagon to support v56 1.5.0

[Bjorn Andersson]

On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:

> This change introduces appropriate additional steps in reset sequence so
> that hexagon v56 1.5.0 is brough out of reset.
> 

You should use the non-_relaxed version throughout this patch, unless
you have good reason not to.

> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 125 ++++++++++++++++++++++++++++++-------
>  1 file changed, 104 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index c55dc9a..7ea2f53 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -65,6 +65,8 @@
>  #define QDSP6SS_RESET_REG		0x014
>  #define QDSP6SS_GFMUX_CTL_REG		0x020
>  #define QDSP6SS_PWR_CTL_REG		0x030
> +#define QDSP6SS_MEM_PWR_CTL		0x0B0
> +#define QDSP6SS_STRAP_ACC		0x110
>  
>  /* AXI Halt Register Offsets */
>  #define AXI_HALTREQ_REG			0x0
> @@ -93,6 +95,15 @@
>  #define QDSS_BHS_ON			BIT(21)
>  #define QDSS_LDO_BYP			BIT(22)
>  
> +/* QDSP6v56 parameters */
> +#define QDSP6v56_LDO_BYP                BIT(25)
> +#define QDSP6v56_BHS_ON                 BIT(24)
> +#define QDSP6v56_CLAMP_WL               BIT(21)
> +#define QDSP6v56_CLAMP_QMC_MEM          BIT(22)
> +#define HALT_CHECK_MAX_LOOPS            (200)
> +#define QDSP6SS_XO_CBCR                 (0x0038)

Please drop these parenthesis.

> +#define QDSP6SS_ACC_OVERRIDE_VAL	0x20
> +
>  struct rproc_hexagon_res {
>  	char *hexagon_mba_image;
>  	char **proxy_reg_string;
> @@ -147,6 +158,8 @@ struct q6v5 {
>  	phys_addr_t mpss_reloc;
>  	void *mpss_region;
>  	size_t mpss_size;
> +
> +	int hexagon_ver;
>  };
>  
>  enum {
> @@ -388,35 +401,103 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>  
>  static int q6v5proc_reset(struct q6v5 *qproc)
>  {
> -	u32 val;
> -	int ret;
> +	u64 val;
> +	int ret, i, count;

One variable per line, sorted descending by length, please.

> +
> +	/* Override the ACC value if required */
> +	if (qproc->hexagon_ver & 0x2)

This will break when we reach the 6th version, compare (==) with the
related enum.

> +		writel_relaxed(QDSP6SS_ACC_OVERRIDE_VAL,
> +				qproc->reg_base + QDSP6SS_STRAP_ACC);
>  
>  	/* Assert resets, stop core */
>  	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
>  	val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
>  	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
>  
> +	/* BHS require xo cbcr to be enabled */

This comment goes inside the if-statement.

> +	if (qproc->hexagon_ver & 0x2) {

==

> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
> +		val |= 0x1;
> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_XO_CBCR);

Replace the rest of this block with:

ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
			 val, !(val & BIT(31)), 1, 200);

> +		for (count = HALT_CHECK_MAX_LOOPS; count > 0; count--) {
> +			val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
> +			if (!(val & BIT(31)))
> +				break;
> +			udelay(1);
> +		}
> +
> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
> +		if ((val & BIT(31)))
> +			dev_err(qproc->dev, "Failed to enable xo branch clock.\n");
> +	}
> +
> +	if (qproc->hexagon_ver & 0x2) {

==

>  	/* Enable power block headswitch, and wait for it to stabilize */
> -	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> -	val |= QDSS_BHS_ON | QDSS_LDO_BYP;
> -	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> -	udelay(1);
> -
> -	/*
> -	 * Turn on memories. L2 banks should be done individually
> -	 * to minimize inrush current.
> -	 */
> -	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> -	val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
> -		Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
> -	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> -	val |= Q6SS_L2DATA_SLP_NRET_N_2;
> -	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> -	val |= Q6SS_L2DATA_SLP_NRET_N_1;
> -	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> -	val |= Q6SS_L2DATA_SLP_NRET_N_0;
> -	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);

Use non-relaxed version of readl and writel, please.

> +		val |= QDSP6v56_BHS_ON;
> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		udelay(1);

Please add a short comment on why this delay is needed.

>  
> +		/* Put LDO in bypass mode */
> +		val |= QDSP6v56_LDO_BYP;
> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +

Remove empty line

> +	} else {
> +		/*
> +		 * Enable power block headswitch,
> +		 * and wait for it to stabilize
> +		 */
> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val |= QDSS_BHS_ON | QDSS_LDO_BYP;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		udelay(1);
> +	}
> +
> +	if (qproc->hexagon_ver & 0x2) {

Why do you have:

if (ver == 2) {
	...
}

if (ver == 2) {
	...
} else {
	...
}

if (ver == 2) {
	...
} else {
	...
}

As far as I can see you should be able to merge those into one if/else.

> +		/*
> +		 * Deassert QDSP6 compiler memory clamp
> +		 */
> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val &= ~QDSP6v56_CLAMP_QMC_MEM;
> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +		/* Deassert memory peripheral sleep and L2 memory standby */
> +		val |= (Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N);
> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +		/* Turn on L1, L2, ETB and JU memories 1 at a time */
> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
> +		for (i = 19; i >= 0; i--) {
> +			val |= BIT(i);
> +			writel_relaxed(val, qproc->reg_base +
> +						QDSP6SS_MEM_PWR_CTL);
> +			/*
> +			 * Wait for 1us for both memory peripheral and
> +			 * data array to turn on.
> +			 */
> +			 mb();

mb() ensures that your writes are ordered, it does not ensure that the
write is done before the sleep. What is the actual requirement here?

> +			udelay(1);
> +		}
> +		/* Remove word line clamp */
> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val &= ~QDSP6v56_CLAMP_WL;
> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	} else {
> +		/*
> +		 * Turn on memories. L2 banks should be done individually
> +		 * to minimize inrush current.
> +		 */
> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
> +			Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val |= Q6SS_L2DATA_SLP_NRET_N_2;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val |= Q6SS_L2DATA_SLP_NRET_N_1;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val |= Q6SS_L2DATA_SLP_NRET_N_0;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	}
>  	/* Remove IO clamp */
>  	val &= ~Q6SS_CLAMP_IO;
>  	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);

Regards,
Bjorn

Re: [RESEND PATCH v4 5/7] remoteproc: qcom: Modify reset sequence for hexagon to support v56 1.5.0

[Dwivedi, Avaneesh Kumar (avani)]

On 12/9/2016 10:05 AM, Bjorn Andersson wrote:
> On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> This change introduces appropriate additional steps in reset sequence so
>> that hexagon v56 1.5.0 is brough out of reset.
>>
> You should use the non-_relaxed version throughout this patch, unless
> you have good reason not to.
OK.
>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 125 ++++++++++++++++++++++++++++++-------
>>   1 file changed, 104 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index c55dc9a..7ea2f53 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -65,6 +65,8 @@
>>   #define QDSP6SS_RESET_REG		0x014
>>   #define QDSP6SS_GFMUX_CTL_REG		0x020
>>   #define QDSP6SS_PWR_CTL_REG		0x030
>> +#define QDSP6SS_MEM_PWR_CTL		0x0B0
>> +#define QDSP6SS_STRAP_ACC		0x110
>>   
>>   /* AXI Halt Register Offsets */
>>   #define AXI_HALTREQ_REG			0x0
>> @@ -93,6 +95,15 @@
>>   #define QDSS_BHS_ON			BIT(21)
>>   #define QDSS_LDO_BYP			BIT(22)
>>   
>> +/* QDSP6v56 parameters */
>> +#define QDSP6v56_LDO_BYP                BIT(25)
>> +#define QDSP6v56_BHS_ON                 BIT(24)
>> +#define QDSP6v56_CLAMP_WL               BIT(21)
>> +#define QDSP6v56_CLAMP_QMC_MEM          BIT(22)
>> +#define HALT_CHECK_MAX_LOOPS            (200)
>> +#define QDSP6SS_XO_CBCR                 (0x0038)
> Please drop these parenthesis.
OK.
>
>> +#define QDSP6SS_ACC_OVERRIDE_VAL	0x20
>> +
>>   struct rproc_hexagon_res {
>>   	char *hexagon_mba_image;
>>   	char **proxy_reg_string;
>> @@ -147,6 +158,8 @@ struct q6v5 {
>>   	phys_addr_t mpss_reloc;
>>   	void *mpss_region;
>>   	size_t mpss_size;
>> +
>> +	int hexagon_ver;
>>   };
>>   
>>   enum {
>> @@ -388,35 +401,103 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>>   
>>   static int q6v5proc_reset(struct q6v5 *qproc)
>>   {
>> -	u32 val;
>> -	int ret;
>> +	u64 val;
>> +	int ret, i, count;
> One variable per line, sorted descending by length, please.
OK.
>
>> +
>> +	/* Override the ACC value if required */
>> +	if (qproc->hexagon_ver & 0x2)
> This will break when we reach the 6th version, compare (==) with the
> related enum.
OK.
>
>> +		writel_relaxed(QDSP6SS_ACC_OVERRIDE_VAL,
>> +				qproc->reg_base + QDSP6SS_STRAP_ACC);
>>   
>>   	/* Assert resets, stop core */
>>   	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
>>   	val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
>>   	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
>>   
>> +	/* BHS require xo cbcr to be enabled */
> This comment goes inside the if-statement.
OK.
>
>> +	if (qproc->hexagon_ver & 0x2) {
> ==
>
>> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
>> +		val |= 0x1;
>> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_XO_CBCR);
> Replace the rest of this block with:
>
> ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
> 			 val, !(val & BIT(31)), 1, 200);
OK.
>
>> +		for (count = HALT_CHECK_MAX_LOOPS; count > 0; count--) {
>> +			val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
>> +			if (!(val & BIT(31)))
>> +				break;
>> +			udelay(1);
>> +		}
>> +
>> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
>> +		if ((val & BIT(31)))
>> +			dev_err(qproc->dev, "Failed to enable xo branch clock.\n");
>> +	}
>> +
>> +	if (qproc->hexagon_ver & 0x2) {
> ==
>
>>   	/* Enable power block headswitch, and wait for it to stabilize */
>> -	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> -	val |= QDSS_BHS_ON | QDSS_LDO_BYP;
>> -	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> -	udelay(1);
>> -
>> -	/*
>> -	 * Turn on memories. L2 banks should be done individually
>> -	 * to minimize inrush current.
>> -	 */
>> -	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> -	val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
>> -		Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
>> -	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> -	val |= Q6SS_L2DATA_SLP_NRET_N_2;
>> -	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> -	val |= Q6SS_L2DATA_SLP_NRET_N_1;
>> -	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> -	val |= Q6SS_L2DATA_SLP_NRET_N_0;
>> -	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> Use non-relaxed version of readl and writel, please.
OK.
>
>> +		val |= QDSP6v56_BHS_ON;
>> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		udelay(1);
> Please add a short comment on why this delay is needed.
I think there is a comment that
     /* Enable power block headswitch, and wait for it to stabilize */
so block head switch need stabilization after enabling so 1 us delay, 
before LDO being put in bypass mode.
>
>>   
>> +		/* Put LDO in bypass mode */
>> +		val |= QDSP6v56_LDO_BYP;
>> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
> Remove empty line
ok.
>
>> +	} else {
>> +		/*
>> +		 * Enable power block headswitch,
>> +		 * and wait for it to stabilize
>> +		 */
>> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val |= QDSS_BHS_ON | QDSS_LDO_BYP;
>> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		udelay(1);
>> +	}
>> +
>> +	if (qproc->hexagon_ver & 0x2) {
> Why do you have:
>
> if (ver == 2) {
> 	...
> }
>
> if (ver == 2) {
> 	...
> } else {
> 	...
> }
>
> if (ver == 2) {
> 	...
> } else {
> 	...
> }
>
> As far as I can see you should be able to merge those into one if/else.
wanted to utilize little piece of common code so put different if block.
OK will combine them.
>
>> +		/*
>> +		 * Deassert QDSP6 compiler memory clamp
>> +		 */
>> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val &= ~QDSP6v56_CLAMP_QMC_MEM;
>> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +		/* Deassert memory peripheral sleep and L2 memory standby */
>> +		val |= (Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N);
>> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +		/* Turn on L1, L2, ETB and JU memories 1 at a time */
>> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
>> +		for (i = 19; i >= 0; i--) {
>> +			val |= BIT(i);
>> +			writel_relaxed(val, qproc->reg_base +
>> +						QDSP6SS_MEM_PWR_CTL);
>> +			/*
>> +			 * Wait for 1us for both memory peripheral and
>> +			 * data array to turn on.
>> +			 */
>> +			 mb();
> mb() ensures that your writes are ordered, it does not ensure that the
> write is done before the sleep. What is the actual requirement here?
As in comment, order of turning need to be serialized so this memory 
barrier.
Do u think its not required?
>> +			udelay(1);
>> +		}
>> +		/* Remove word line clamp */
>> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val &= ~QDSP6v56_CLAMP_WL;
>> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	} else {
>> +		/*
>> +		 * Turn on memories. L2 banks should be done individually
>> +		 * to minimize inrush current.
>> +		 */
>> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
>> +			Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
>> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val |= Q6SS_L2DATA_SLP_NRET_N_2;
>> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val |= Q6SS_L2DATA_SLP_NRET_N_1;
>> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val |= Q6SS_L2DATA_SLP_NRET_N_0;
>> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	}
>>   	/* Remove IO clamp */
>>   	val &= ~Q6SS_CLAMP_IO;
>>   	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> Regards,
> Bjorn

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [RESEND PATCH v4 5/7] remoteproc: qcom: Modify reset sequence for hexagon to support v56 1.5.0

[Bjorn Andersson]

On Mon 12 Dec 04:45 PST 2016, Dwivedi, Avaneesh Kumar (avani) wrote:
> On 12/9/2016 10:05 AM, Bjorn Andersson wrote:
> >On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:
[..]
> >>+
> >>+		/* Turn on L1, L2, ETB and JU memories 1 at a time */
> >>+		val = readl_relaxed(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
> >>+		for (i = 19; i >= 0; i--) {
> >>+			val |= BIT(i);
> >>+			writel_relaxed(val, qproc->reg_base +
> >>+						QDSP6SS_MEM_PWR_CTL);
> >>+			/*
> >>+			 * Wait for 1us for both memory peripheral and
> >>+			 * data array to turn on.
> >>+			 */
> >>+			 mb();
> >mb() ensures that your writes are ordered, it does not ensure that the
> >write is done before the sleep. What is the actual requirement here?
> As in comment, order of turning need to be serialized so this memory
> barrier.
> Do u think its not required?

The problem is that mb() don't actually wait for the write to finish, it
simply makes sure that any subsequent writes will come after this one.

If we want to make sure the write actually hits the hardware before the
delay we should read the register back after the write - as that would
stall execution until the write is available.

Either way, using the non-_relaxed version of writel() will be
equivalent to what you have now.

> >>+			udelay(1);
> >>+		}

Regards,
Bjorn

Re: [RESEND PATCH v4 5/7] remoteproc: qcom: Modify reset sequence for hexagon to support v56 1.5.0

[Dwivedi, Avaneesh Kumar (avani)]

On 12/13/2016 11:39 PM, Bjorn Andersson wrote:
> On Mon 12 Dec 04:45 PST 2016, Dwivedi, Avaneesh Kumar (avani) wrote:
>> On 12/9/2016 10:05 AM, Bjorn Andersson wrote:
>>> On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:
> [..]
>>>> +
>>>> +		/* Turn on L1, L2, ETB and JU memories 1 at a time */
>>>> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
>>>> +		for (i = 19; i >= 0; i--) {
>>>> +			val |= BIT(i);
>>>> +			writel_relaxed(val, qproc->reg_base +
>>>> +						QDSP6SS_MEM_PWR_CTL);
>>>> +			/*
>>>> +			 * Wait for 1us for both memory peripheral and
>>>> +			 * data array to turn on.
>>>> +			 */
>>>> +			 mb();
>>> mb() ensures that your writes are ordered, it does not ensure that the
>>> write is done before the sleep. What is the actual requirement here?
>> As in comment, order of turning need to be serialized so this memory
>> barrier.
>> Do u think its not required?
> The problem is that mb() don't actually wait for the write to finish, it
> simply makes sure that any subsequent writes will come after this one.
mb() is for below comment

/* Turn on L1, L2, ETB and JU memories 1 at a time */

While delay corresponding to

  /*
+ * Wait for 1us for both memory peripheral and
+ * data array to turn on.
+ */

>
> If we want to make sure the write actually hits the hardware before the
> delay we should read the register back after the write - as that would
> stall execution until the write is available.
>
> Either way, using the non-_relaxed version of writel() will be
> equivalent to what you have now.
Do you mean if writel is used , udelay()  should be removed? i 
understand writel will not return before register write operation is 
actually done.
udelay() is to give enough time so that after writel , there is some 
time available to turn on mem peripheral and data array.
>
>>>> +			udelay(1);
>>>> +		}
> Regards,
> Bjorn

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [RESEND PATCH v4 5/7] remoteproc: qcom: Modify reset sequence for hexagon to support v56 1.5.0

[Bjorn Andersson]

On Tue 13 Dec 11:45 PST 2016, Dwivedi, Avaneesh Kumar (avani) wrote:
> On 12/13/2016 11:39 PM, Bjorn Andersson wrote:
[..]
> >Either way, using the non-_relaxed version of writel() will be
> >equivalent to what you have now.
> Do you mean if writel is used , udelay()  should be removed?

No, I mean that looping writel_relaxed() + wmb() is roughly
equivalent to writel(). So with the overall comment of you replacing
readl_relaxed() and writel_relaxed() with their plain readl/writel
counterparts takes care of the wmb().

> i understand
> writel will not return before register write operation is actually done.
> udelay() is to give enough time so that after writel , there is some time
> available to turn on mem peripheral and data array.

As far as I understand, wmb() will ensure that any cache coherent or
write-back buffered writes are committed before any subsequent writes.
But that this is not the same as the write has finished.

As far as I can see, the downstream code (msm-3.18) do:

for (i = 19; i >= 0; i--) {
	val |= BIT(i);
	writel_relaxed(val, MEM_PWR_CTL);
	val |= readl_relaxed(MEM_PWR_CTL);
	udelay(1);
}

I.e. for this particular version it actually does read back the value,
which will cause a wait for the write to be propagated. But I'm not sure
why this is the only version doing this.

Regards,
Bjorn

Re: [RESEND PATCH v4 5/7] remoteproc: qcom: Modify reset sequence for hexagon to support v56 1.5.0

[Dwivedi, Avaneesh Kumar (avani)]

On 12/14/2016 3:37 AM, Bjorn Andersson wrote:
> On Tue 13 Dec 11:45 PST 2016, Dwivedi, Avaneesh Kumar (avani) wrote:
>> On 12/13/2016 11:39 PM, Bjorn Andersson wrote:
> [..]
>>> Either way, using the non-_relaxed version of writel() will be
>>> equivalent to what you have now.
>> Do you mean if writel is used , udelay()  should be removed?
> No, I mean that looping writel_relaxed() + wmb() is roughly
> equivalent to writel(). So with the overall comment of you replacing
> readl_relaxed() and writel_relaxed() with their plain readl/writel
> counterparts takes care of the wmb().

Thanks got it, yes my requirement was to get write done before control 
reaches udelay(), so i will add readl() before udelay and remove mb()
I hope after this change i can submit next set of patches?
>
>> i understand
>> writel will not return before register write operation is actually done.
>> udelay() is to give enough time so that after writel , there is some time
>> available to turn on mem peripheral and data array.
> As far as I understand, wmb() will ensure that any cache coherent or
> write-back buffered writes are committed before any subsequent writes.
> But that this is not the same as the write has finished.
>
> As far as I can see, the downstream code (msm-3.18) do:
>
> for (i = 19; i >= 0; i--) {
> 	val |= BIT(i);
> 	writel_relaxed(val, MEM_PWR_CTL);
> 	val |= readl_relaxed(MEM_PWR_CTL);
> 	udelay(1);
> }
>
> I.e. for this particular version it actually does read back the value,
> which will cause a wait for the write to be propagated. But I'm not sure
> why this is the only version doing this.
>
> Regards,
> Bjorn

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [RESEND PATCH v4 5/7] remoteproc: qcom: Modify reset sequence for hexagon to support v56 1.5.0

[Bjorn Andersson]

On Wed 14 Dec 07:50 PST 2016, Dwivedi, Avaneesh Kumar (avani) wrote:

> 
> 
> On 12/14/2016 3:37 AM, Bjorn Andersson wrote:
> >On Tue 13 Dec 11:45 PST 2016, Dwivedi, Avaneesh Kumar (avani) wrote:
> >>On 12/13/2016 11:39 PM, Bjorn Andersson wrote:
> >[..]
> >>>Either way, using the non-_relaxed version of writel() will be
> >>>equivalent to what you have now.
> >>Do you mean if writel is used , udelay()  should be removed?
> >No, I mean that looping writel_relaxed() + wmb() is roughly
> >equivalent to writel(). So with the overall comment of you replacing
> >readl_relaxed() and writel_relaxed() with their plain readl/writel
> >counterparts takes care of the wmb().
> 
> Thanks got it, yes my requirement was to get write done before control
> reaches udelay(), so i will add readl() before udelay and remove mb()

Sounds good.

> I hope after this change i can submit next set of patches?

Yes, please do.

Regards,
Bjorn

[RESEND PATCH v4 6/7] remoteproc: qcom: Modify stop routine to limit MX current for v56 1.5

[Avaneesh Kumar Dwivedi]

For v56 1.5.0 Mx current need to be limited during restart.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 7ea2f53..3e3ed09 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -785,7 +785,7 @@ static int q6v5_start(struct rproc *rproc)
 static int q6v5_stop(struct rproc *rproc)
 {
 	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
-	int ret;
+	int ret, val;
 
 	qproc->running = false;
 
@@ -803,6 +803,22 @@ static int q6v5_stop(struct rproc *rproc)
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
 
+	if (qproc->hexagon_ver & 0x2) {
+		/*
+		 * Assert QDSP6 I/O clamp, memory wordline clamp, and compiler
+		 * memory clamp as a software workaround to avoid high MX
+		 * current during LPASS/MSS restart.
+		 */
+		ret = clk_prepare_enable(devm_clk_get(qproc->dev, "iface"));
+		if (!ret) {
+			val = readl_relaxed(
+				qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+			val |= (Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL |
+				QDSP6v56_CLAMP_QMC_MEM);
+			writel_relaxed(val,
+				qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		}
+	}
 	reset_control_assert(qproc->mss_restart);
 	q6v5_clk_disable(qproc);
 	q6v5_regulator_disable(qproc);
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [RESEND PATCH v4 6/7] remoteproc: qcom: Modify stop routine to limit MX current for v56 1.5

[Bjorn Andersson]

On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:

> For v56 1.5.0 Mx current need to be limited during restart.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 7ea2f53..3e3ed09 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -785,7 +785,7 @@ static int q6v5_start(struct rproc *rproc)
>  static int q6v5_stop(struct rproc *rproc)
>  {
>  	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
> -	int ret;
> +	int ret, val;

One variable per line, please.

>  
>  	qproc->running = false;
>  
> @@ -803,6 +803,22 @@ static int q6v5_stop(struct rproc *rproc)
>  	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
>  	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>  
> +	if (qproc->hexagon_ver & 0x2) {

Compare with == and use the enum.

> +		/*
> +		 * Assert QDSP6 I/O clamp, memory wordline clamp, and compiler
> +		 * memory clamp as a software workaround to avoid high MX
> +		 * current during LPASS/MSS restart.
> +		 */

I'm not sure if you answered this (or if I forgot to ask), is this
something we only want to do on the latest version?

> +		ret = clk_prepare_enable(devm_clk_get(qproc->dev, "iface"));

"iface" is part of your active set, so it should still be enabled.

> +		if (!ret) {
> +			val = readl_relaxed(
> +				qproc->reg_base + QDSP6SS_PWR_CTL_REG);

Please use non-_relaxed versions.

> +			val |= (Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL |
> +				QDSP6v56_CLAMP_QMC_MEM);

No need for the parenthesis around this expression.

> +			writel_relaxed(val,
> +				qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		}
> +	}

Regards,
Bjorn

Re: [RESEND PATCH v4 6/7] remoteproc: qcom: Modify stop routine to limit MX current for v56 1.5

[Dwivedi, Avaneesh Kumar (avani)]

On 12/9/2016 10:12 AM, Bjorn Andersson wrote:
> On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> For v56 1.5.0 Mx current need to be limited during restart.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 7ea2f53..3e3ed09 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -785,7 +785,7 @@ static int q6v5_start(struct rproc *rproc)
>>   static int q6v5_stop(struct rproc *rproc)
>>   {
>>   	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>> -	int ret;
>> +	int ret, val;
> One variable per line, please.
OK,
>
>>   
>>   	qproc->running = false;
>>   
>> @@ -803,6 +803,22 @@ static int q6v5_stop(struct rproc *rproc)
>>   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
>>   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>>   
>> +	if (qproc->hexagon_ver & 0x2) {
> Compare with == and use the enum.
Ok,
>
>> +		/*
>> +		 * Assert QDSP6 I/O clamp, memory wordline clamp, and compiler
>> +		 * memory clamp as a software workaround to avoid high MX
>> +		 * current during LPASS/MSS restart.
>> +		 */
> I'm not sure if you answered this (or if I forgot to ask), is this
> something we only want to do on the latest version?
Yes applicable only to hexagon on msm8996.
>
>> +		ret = clk_prepare_enable(devm_clk_get(qproc->dev, "iface"));
> "iface" is part of your active set, so it should still be enabled.
Right, thanks for pointing this out.
>
>> +		if (!ret) {
>> +			val = readl_relaxed(
>> +				qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> Please use non-_relaxed versions.
OK.
>
>> +			val |= (Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL |
>> +				QDSP6v56_CLAMP_QMC_MEM);
> No need for the parenthesis around this expression.
OK.
>
>> +			writel_relaxed(val,
>> +				qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		}
>> +	}
> Regards,
> Bjorn

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[RESEND PATCH v4 7/7] clk: qcom: Add GCC_MSS_RESET support to reset MSS in v56 1.5.0

[Avaneesh Kumar Dwivedi]

Add support to use reset control framework for resetting MSS
on v56 1.5.0.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 drivers/clk/qcom/gcc-msm8996.c               | 1 +
 include/dt-bindings/clock/qcom,gcc-msm8996.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/clk/qcom/gcc-msm8996.c b/drivers/clk/qcom/gcc-msm8996.c
index fe03e6f..fd4bf6f 100644
--- a/drivers/clk/qcom/gcc-msm8996.c
+++ b/drivers/clk/qcom/gcc-msm8996.c
@@ -3423,6 +3423,7 @@ enum {
 	[GCC_MSMPU_BCR] = { 0x8d000 },
 	[GCC_MSS_Q6_BCR] = { 0x8e000 },
 	[GCC_QREFS_VBG_CAL_BCR] = { 0x88020 },
+	[GCC_MSS_RESTART] = { 0x8f008 },
 };
 
 static const struct regmap_config gcc_msm8996_regmap_config = {
diff --git a/include/dt-bindings/clock/qcom,gcc-msm8996.h b/include/dt-bindings/clock/qcom,gcc-msm8996.h
index 1828723..1f5c422 100644
--- a/include/dt-bindings/clock/qcom,gcc-msm8996.h
+++ b/include/dt-bindings/clock/qcom,gcc-msm8996.h
@@ -339,6 +339,7 @@
 #define GCC_PCIE_PHY_COM_NOCSR_BCR				102
 #define GCC_USB3_PHY_BCR					103
 #define GCC_USB3PHY_PHY_BCR					104
+#define GCC_MSS_RESTART						105
 
 
 /* Indexes for GDSCs */
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [RESEND PATCH v4 7/7] clk: qcom: Add GCC_MSS_RESET support to reset MSS in v56 1.5.0

[Bjorn Andersson]

On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:

> Add support to use reset control framework for resetting MSS
> on v56 1.5.0.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>

You should try (really hard) to limit your subject line to 50 characters
and then use the body to describe your change - rather than just typing
the same thing twice. E.g. a subject of "clk: gcc-msm8996: Add
MSS_RESET" is short and to the point.

Please update the subject and add:

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/clk/qcom/gcc-msm8996.c               | 1 +
>  include/dt-bindings/clock/qcom,gcc-msm8996.h | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gcc-msm8996.c b/drivers/clk/qcom/gcc-msm8996.c
> index fe03e6f..fd4bf6f 100644
> --- a/drivers/clk/qcom/gcc-msm8996.c
> +++ b/drivers/clk/qcom/gcc-msm8996.c
> @@ -3423,6 +3423,7 @@ enum {
>  	[GCC_MSMPU_BCR] = { 0x8d000 },
>  	[GCC_MSS_Q6_BCR] = { 0x8e000 },
>  	[GCC_QREFS_VBG_CAL_BCR] = { 0x88020 },
> +	[GCC_MSS_RESTART] = { 0x8f008 },
>  };
>  
>  static const struct regmap_config gcc_msm8996_regmap_config = {
> diff --git a/include/dt-bindings/clock/qcom,gcc-msm8996.h b/include/dt-bindings/clock/qcom,gcc-msm8996.h
> index 1828723..1f5c422 100644
> --- a/include/dt-bindings/clock/qcom,gcc-msm8996.h
> +++ b/include/dt-bindings/clock/qcom,gcc-msm8996.h
> @@ -339,6 +339,7 @@
>  #define GCC_PCIE_PHY_COM_NOCSR_BCR				102
>  #define GCC_USB3_PHY_BCR					103
>  #define GCC_USB3PHY_PHY_BCR					104
> +#define GCC_MSS_RESTART						105
>  
>  
>  /* Indexes for GDSCs */
> -- 
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project.
> 

Re: [RESEND PATCH v4 7/7] clk: qcom: Add GCC_MSS_RESET support to reset MSS in v56 1.5.0

[Dwivedi, Avaneesh Kumar (avani)]

On 12/9/2016 1:21 AM, Bjorn Andersson wrote:
> On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> Add support to use reset control framework for resetting MSS
>> on v56 1.5.0.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> You should try (really hard) to limit your subject line to 50 characters
> and then use the body to describe your change - rather than just typing
> the same thing twice. E.g. a subject of "clk: gcc-msm8996: Add
> MSS_RESET" is short and to the point.
Ok, Sure :(
>
> Please update the subject and add:
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>
> Regards,
> Bjorn
>
>> ---
>>   drivers/clk/qcom/gcc-msm8996.c               | 1 +
>>   include/dt-bindings/clock/qcom,gcc-msm8996.h | 1 +
>>   2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/gcc-msm8996.c b/drivers/clk/qcom/gcc-msm8996.c
>> index fe03e6f..fd4bf6f 100644
>> --- a/drivers/clk/qcom/gcc-msm8996.c
>> +++ b/drivers/clk/qcom/gcc-msm8996.c
>> @@ -3423,6 +3423,7 @@ enum {
>>   	[GCC_MSMPU_BCR] = { 0x8d000 },
>>   	[GCC_MSS_Q6_BCR] = { 0x8e000 },
>>   	[GCC_QREFS_VBG_CAL_BCR] = { 0x88020 },
>> +	[GCC_MSS_RESTART] = { 0x8f008 },
>>   };
>>   
>>   static const struct regmap_config gcc_msm8996_regmap_config = {
>> diff --git a/include/dt-bindings/clock/qcom,gcc-msm8996.h b/include/dt-bindings/clock/qcom,gcc-msm8996.h
>> index 1828723..1f5c422 100644
>> --- a/include/dt-bindings/clock/qcom,gcc-msm8996.h
>> +++ b/include/dt-bindings/clock/qcom,gcc-msm8996.h
>> @@ -339,6 +339,7 @@
>>   #define GCC_PCIE_PHY_COM_NOCSR_BCR				102
>>   #define GCC_USB3_PHY_BCR					103
>>   #define GCC_USB3PHY_PHY_BCR					104
>> +#define GCC_MSS_RESTART						105
>>   
>>   
>>   /* Indexes for GDSCs */
>> -- 
>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project.
>>

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.