[PATCH v5 0/7] remoteproc: qcom: Add support of mss rproc for msm8996.

[PATCH v5 0/7] remoteproc: qcom: Add support of mss rproc for msm8996.

[Avaneesh Kumar Dwivedi]

This is patchset v5 having modifications as per comment on patchset v4.
Major changes w.r.t. patchset v4 are as below.	
	1- Compatible string and private struct naming according to platform.
	2- Separation of regulator and clock init.
	3- Regulator Init implementation change.
	4- Regulator and clock disable interface and implementation change.
	5- Reorganization of reset sequence.
	6- Other Minor comments related changes.

Avaneesh Kumar Dwivedi (7):
  remoteproc: qcom: Add private resource struct and new compatible.
  remoteproc: qcom: Add and initialize proxy and active clocks.
  remoteproc: qcom: Add and initialize proxy and active regulators.
  remoteproc: qcom: Modify regulator enable and disable interface.
  remoteproc: qcom: Modify clock enable and disable interface.
  remoteproc: qcom: Implement Hexagon core specific changes.
  clk: qcom: Add GCC_MSS_RESET support

 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   5 +-
 drivers/clk/qcom/gcc-msm8996.c                     |   1 +
 drivers/remoteproc/qcom_q6v5_pil.c                 | 561 ++++++++++++++++-----
 include/dt-bindings/clock/qcom,gcc-msm8996.h       |   1 +
 4 files changed, 451 insertions(+), 117 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.

[PATCH v5 1/7] remoteproc: qcom: Add private resource struct and new compatible.

[Avaneesh Kumar Dwivedi]

Make provison for private resource specific to each compatible,
so a private resource struct is introduced which will house clock,
regulator etc. related info corresponding to each compatible to use
later to initialize hardware. Also add new compatible in documentation.

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

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 57cb49e..674ebc6 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -7,7 +7,9 @@ on the Qualcomm Hexagon core.
 	Usage: required
 	Value type: <string>
 	Definition: must be one of:
-		    "qcom,q6v5-pil"
+		"qcom,q6v5-pil"
+		"qcom,msm8916-mss-pil"
+		"qcom,msm8974-mss-pil"
 
 - reg:
 	Usage: required
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 2e0caaa..d875448 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -30,13 +30,13 @@
 #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"
 
 #include <linux/qcom_scm.h>
 
-#define MBA_FIRMWARE_NAME		"mba.b00"
 #define MPSS_FIRMWARE_NAME		"modem.mdt"
 
 #define MPSS_CRASH_REASON_SMEM		421
@@ -93,6 +93,10 @@
 #define QDSS_BHS_ON			BIT(21)
 #define QDSS_LDO_BYP			BIT(22)
 
+struct rproc_hexagon_res {
+	const char *hexagon_mba_image;
+};
+
 struct q6v5 {
 	struct device *dev;
 	struct rproc *rproc;
@@ -805,12 +809,17 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
 
 static int q6v5_probe(struct platform_device *pdev)
 {
+	const struct rproc_hexagon_res *desc;
 	struct q6v5 *qproc;
 	struct rproc *rproc;
 	int ret;
 
+	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;
@@ -890,8 +899,17 @@ static int q6v5_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct rproc_hexagon_res msm8916_mss = {
+	.hexagon_mba_image = "mba.mbn",
+};
+
+static const struct rproc_hexagon_res msm8974_mss = {
+	.hexagon_mba_image = "mba.b00",
+};
 static const struct of_device_id q6v5_of_match[] = {
-	{ .compatible = "qcom,q6v5-pil", },
+	{ .compatible = "qcom,q6v5-pil", .data = &msm8916_mss},
+	{ .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss},
+	{ .compatible = "qcom,msm8974-mss-pil", .data = &msm8974_mss},
 	{ },
 };
 
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH v5 2/7] remoteproc: qcom: Add and initialize proxy and active clocks.

[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 respectively, whose handle is being obtained
by supplying proxy and active clock name string.

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

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index d875448..8c8b239 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -95,6 +95,8 @@
 
 struct rproc_hexagon_res {
 	const char *hexagon_mba_image;
+	char **proxy_clk_string;
+	char **active_clk_string;
 };
 
 struct q6v5 {
@@ -114,6 +116,11 @@ struct q6v5 {
 	struct qcom_smem_state *state;
 	unsigned stop_bit;
 
+	struct clk *active_clks[8];
+	struct clk *proxy_clks[4];
+	int active_clk_count;
+	int proxy_clk_count;
+
 	struct regulator_bulk_data supply[4];
 
 	struct clk *ahb_clk;
@@ -706,27 +713,33 @@ 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 count = 0;
+	int 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[count])
+		count++;
+
+	for (i = 0; i < count; i++) {
+		clks[i] = devm_clk_get(dev, clk_str[i]);
+		if (IS_ERR(clks[i])) {
+
+			int rc = PTR_ERR(clks[i]);
+
+			if (rc != -EPROBE_DEFER)
+				dev_err(dev, "Failed to get %s clock\n",
+					clk_str[i]);
+			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 count;
 }
 
 static int q6v5_init_reset(struct q6v5 *qproc)
@@ -843,9 +856,21 @@ static int q6v5_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
-	ret = q6v5_init_clocks(qproc);
-	if (ret)
+	ret = q6v5_init_clocks(&pdev->dev, qproc->proxy_clks,
+					desc->proxy_clk_string);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to setup proxy clocks.\n");
+		goto free_rproc;
+	}
+	qproc->proxy_clk_count = ret;
+
+	ret = q6v5_init_clocks(&pdev->dev, qproc->active_clks,
+					desc->active_clk_string);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to setup active clocks.\n");
 		goto free_rproc;
+	}
+	qproc->active_clk_count = ret;
 
 	ret = q6v5_regulator_init(qproc);
 	if (ret)
@@ -901,10 +926,14 @@ static int q6v5_remove(struct platform_device *pdev)
 
 static const struct rproc_hexagon_res msm8916_mss = {
 	.hexagon_mba_image = "mba.mbn",
+	.proxy_clk_string = (char*[]){"xo", NULL},
+	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},
 };
 
 static const struct rproc_hexagon_res msm8974_mss = {
 	.hexagon_mba_image = "mba.b00",
+	.proxy_clk_string = (char*[]){"xo", NULL},
+	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},
 };
 static const struct of_device_id q6v5_of_match[] = {
 	{ .compatible = "qcom,q6v5-pil", .data = &msm8916_mss},
-- 
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: [PATCH v5 2/7] remoteproc: qcom: Add and initialize proxy and active clocks.

[Bjorn Andersson]

On Thu 15 Dec 04:21 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 respectively, whose handle is being obtained
> by supplying proxy and active clock name string.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 65 +++++++++++++++++++++++++++-----------
>  1 file changed, 47 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index d875448..8c8b239 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -95,6 +95,8 @@
>  
>  struct rproc_hexagon_res {
>  	const char *hexagon_mba_image;
> +	char **proxy_clk_string;
> +	char **active_clk_string;

Use "name" instead of "string" in these variable names - i.e.
proxy_clk_names and active_clk_names.

>  };
>  
>  struct q6v5 {
> @@ -114,6 +116,11 @@ struct q6v5 {
>  	struct qcom_smem_state *state;
>  	unsigned stop_bit;
>  
> +	struct clk *active_clks[8];
> +	struct clk *proxy_clks[4];
> +	int active_clk_count;
> +	int proxy_clk_count;
> +
>  	struct regulator_bulk_data supply[4];
>  
>  	struct clk *ahb_clk;
> @@ -706,27 +713,33 @@ 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)

clk_names

>  {
> -	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 count = 0;
> +	int 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[count])
> +		count++;
> +
> +	for (i = 0; i < count; i++) {

You can squash these two loops into one, e.g. replace them with:

for (i = 0; clk_str[i]; i++) {}

and then return "i".

> +		clks[i] = devm_clk_get(dev, clk_str[i]);
> +		if (IS_ERR(clks[i])) {
> +
> +			int rc = PTR_ERR(clks[i]);
> +
> +			if (rc != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to get %s clock\n",
> +					clk_str[i]);
> +			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 count;
>  }
>  
>  static int q6v5_init_reset(struct q6v5 *qproc)
> @@ -843,9 +856,21 @@ static int q6v5_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto free_rproc;
>  
> -	ret = q6v5_init_clocks(qproc);
> -	if (ret)
> +	ret = q6v5_init_clocks(&pdev->dev, qproc->proxy_clks,
> +					desc->proxy_clk_string);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to setup proxy clocks.\n");

Please replace "setup" with "acquire" or "get".

> +		goto free_rproc;
> +	}
> +	qproc->proxy_clk_count = ret;
> +
> +	ret = q6v5_init_clocks(&pdev->dev, qproc->active_clks,
> +					desc->active_clk_string);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to setup active clocks.\n");

Dito.

>  		goto free_rproc;
> +	}
> +	qproc->active_clk_count = ret;
>  
>  	ret = q6v5_regulator_init(qproc);
>  	if (ret)
> @@ -901,10 +926,14 @@ static int q6v5_remove(struct platform_device *pdev)
>  
>  static const struct rproc_hexagon_res msm8916_mss = {
>  	.hexagon_mba_image = "mba.mbn",
> +	.proxy_clk_string = (char*[]){"xo", NULL},
> +	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},

Line wrap these list of clock names, like:

	.active_clk_string = (char*[]){
		"iface",
		"bus",
		"mem",
		NULL
	},

Makes it consistent with the regulator
patch and it's easier for me to read.


>  };
>  
>  static const struct rproc_hexagon_res msm8974_mss = {
>  	.hexagon_mba_image = "mba.b00",
> +	.proxy_clk_string = (char*[]){"xo", NULL},
> +	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},

Dito

>  };

If I apply this patch without patch 5 (Modify clock enable and disable
interface) we will successfully probe with the new clocks, but we will
not be able to boot because ahb_clk, axi_clk and rom_clk are NULL.

When you're sending patches you should make sure that the code works
(before and) after each patch that you introduce.

So please squash patch 5 into this patch.


Other than that and these small style comments I think this patch looks
good!

Regards,
Bjorn

Re: [PATCH v5 2/7] remoteproc: qcom: Add and initialize proxy and active clocks.

[Dwivedi, Avaneesh Kumar (avani)]

On 12/23/2016 3:12 AM, Bjorn Andersson wrote:
> On Thu 15 Dec 04:21 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 respectively, whose handle is being obtained
>> by supplying proxy and active clock name string.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 65 +++++++++++++++++++++++++++-----------
>>   1 file changed, 47 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index d875448..8c8b239 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -95,6 +95,8 @@
>>   
>>   struct rproc_hexagon_res {
>>   	const char *hexagon_mba_image;
>> +	char **proxy_clk_string;
>> +	char **active_clk_string;
> Use "name" instead of "string" in these variable names - i.e.
> proxy_clk_names and active_clk_names.
OK.
>
>>   };
>>   
>>   struct q6v5 {
>> @@ -114,6 +116,11 @@ struct q6v5 {
>>   	struct qcom_smem_state *state;
>>   	unsigned stop_bit;
>>   
>> +	struct clk *active_clks[8];
>> +	struct clk *proxy_clks[4];
>> +	int active_clk_count;
>> +	int proxy_clk_count;
>> +
>>   	struct regulator_bulk_data supply[4];
>>   
>>   	struct clk *ahb_clk;
>> @@ -706,27 +713,33 @@ 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)
> clk_names
OK.
>>   {
>> -	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 count = 0;
>> +	int 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[count])
>> +		count++;
>> +
>> +	for (i = 0; i < count; i++) {
> You can squash these two loops into one, e.g. replace them with:
>
> for (i = 0; clk_str[i]; i++) {}
>
> and then return "i".
OK.
>> +		clks[i] = devm_clk_get(dev, clk_str[i]);
>> +		if (IS_ERR(clks[i])) {
>> +
>> +			int rc = PTR_ERR(clks[i]);
>> +
>> +			if (rc != -EPROBE_DEFER)
>> +				dev_err(dev, "Failed to get %s clock\n",
>> +					clk_str[i]);
>> +			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 count;
>>   }
>>   
>>   static int q6v5_init_reset(struct q6v5 *qproc)
>> @@ -843,9 +856,21 @@ static int q6v5_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto free_rproc;
>>   
>> -	ret = q6v5_init_clocks(qproc);
>> -	if (ret)
>> +	ret = q6v5_init_clocks(&pdev->dev, qproc->proxy_clks,
>> +					desc->proxy_clk_string);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "Failed to setup proxy clocks.\n");
> Please replace "setup" with "acquire" or "get".
OK.
>
>> +		goto free_rproc;
>> +	}
>> +	qproc->proxy_clk_count = ret;
>> +
>> +	ret = q6v5_init_clocks(&pdev->dev, qproc->active_clks,
>> +					desc->active_clk_string);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "Failed to setup active clocks.\n");
> Dito.
OK.
>>   		goto free_rproc;
>> +	}
>> +	qproc->active_clk_count = ret;
>>   
>>   	ret = q6v5_regulator_init(qproc);
>>   	if (ret)
>> @@ -901,10 +926,14 @@ static int q6v5_remove(struct platform_device *pdev)
>>   
>>   static const struct rproc_hexagon_res msm8916_mss = {
>>   	.hexagon_mba_image = "mba.mbn",
>> +	.proxy_clk_string = (char*[]){"xo", NULL},
>> +	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},
> Line wrap these list of clock names, like:
>
> 	.active_clk_string = (char*[]){
> 		"iface",
> 		"bus",
> 		"mem",
> 		NULL
> 	},
>
> Makes it consistent with the regulator
> patch and it's easier for me to read.
OK.
>
>>   };
>>   
>>   static const struct rproc_hexagon_res msm8974_mss = {
>>   	.hexagon_mba_image = "mba.b00",
>> +	.proxy_clk_string = (char*[]){"xo", NULL},
>> +	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},
> Dito
OK
>>   };
> If I apply this patch without patch 5 (Modify clock enable and disable
> interface) we will successfully probe with the new clocks, but we will
> not be able to boot because ahb_clk, axi_clk and rom_clk are NULL.
>
> When you're sending patches you should make sure that the code works
> (before and) after each patch that you introduce.
>
> So please squash patch 5 into this patch.
OK, will squash patches into functional units and send them.
>
>
> Other than that and these small style comments I think this patch looks
> good!
Thanks.
>
> 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.

[PATCH v5 3/7] remoteproc: qcom: Add and initialize proxy and active regulators.

[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 respectively, whose handle is being obtained
by supplying proxy and active regulator string name.

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

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 8c8b239..e95d118 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -93,8 +93,22 @@
 #define QDSS_BHS_ON			BIT(21)
 #define QDSS_LDO_BYP			BIT(22)
 
+struct reg_info {
+	struct regulator *reg;
+	int uV;
+	int uA;
+};
+
+struct qcom_mss_reg_res {
+	const char *supply;
+	int uV;
+	int uA;
+};
+
 struct rproc_hexagon_res {
 	const char *hexagon_mba_image;
+	struct qcom_mss_reg_res proxy_supply[4];
+	struct qcom_mss_reg_res active_supply[2];
 	char **proxy_clk_string;
 	char **active_clk_string;
 };
@@ -121,6 +135,11 @@ struct q6v5 {
 	int active_clk_count;
 	int proxy_clk_count;
 
+	struct reg_info active_regs[1];
+	struct reg_info proxy_regs[3];
+	int active_reg_count;
+	int proxy_reg_count;
+
 	struct regulator_bulk_data supply[4];
 
 	struct clk *ahb_clk;
@@ -148,29 +167,34 @@ enum {
 	Q6V5_SUPPLY_PLL,
 };
 
-static int q6v5_regulator_init(struct q6v5 *qproc)
+static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
+				const struct qcom_mss_reg_res *reg_res)
 {
-	int ret;
+	int count = 0;
+	int rc;
+	int 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";
+	while (reg_res[count].supply)
+	count++;
 
-	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;
-	}
+	for (i = 0; i < count; i++) {
+		regs[i].reg = devm_regulator_get(dev, reg_res[i].supply);
+		if (IS_ERR(regs[i].reg)) {
+			rc = PTR_ERR(regs[i].reg);
+			if (rc != -EPROBE_DEFER)
+				dev_err(dev, "Failed to get %s\n regulator",
+						reg_res[i].supply);
+			return rc;
+		}
 
-	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);
+		regs[i].uV = reg_res[i].uV;
+		regs[i].uA = reg_res[i].uA;
+	}
 
-	return 0;
+	return count;
 }
 
+
 static int q6v5_regulator_enable(struct q6v5 *qproc)
 {
 	struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer;
@@ -872,9 +896,21 @@ static int q6v5_probe(struct platform_device *pdev)
 	}
 	qproc->active_clk_count = ret;
 
-	ret = q6v5_regulator_init(qproc);
-	if (ret)
+	ret = q6v5_regulator_init(&pdev->dev, qproc->proxy_regs,
+					desc->proxy_supply);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to setup proxy regulators.\n");
 		goto free_rproc;
+	}
+	qproc->proxy_reg_count = ret;
+
+	ret = q6v5_regulator_init(&pdev->dev,  qproc->active_regs,
+					desc->active_supply);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to setup active regulators.\n");
+		goto free_rproc;
+	}
+	qproc->active_reg_count = ret;
 
 	ret = q6v5_init_reset(qproc);
 	if (ret)
@@ -926,12 +962,58 @@ static int q6v5_remove(struct platform_device *pdev)
 
 static const struct rproc_hexagon_res msm8916_mss = {
 	.hexagon_mba_image = "mba.mbn",
+	.proxy_supply = (struct qcom_mss_reg_res[]) {
+		{
+			.supply = "mx",
+			.uV = 1050000,
+		},
+		{
+			.supply = "cx",
+			.uA = 100000,
+		},
+		{
+			.supply = "pll",
+			.uA = 100000,
+		},
+		{ NULL }
+	},
+	.active_supply = (struct qcom_mss_reg_res[]) {
+		{
+			.supply = "mss",
+			.uV = 1050000,
+			.uA = 100000,
+		},
+		{ NULL }
+	},
 	.proxy_clk_string = (char*[]){"xo", NULL},
 	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},
 };
 
 static const struct rproc_hexagon_res msm8974_mss = {
 	.hexagon_mba_image = "mba.b00",
+	.proxy_supply = (struct qcom_mss_reg_res[]) {
+		{
+			.supply = "mx",
+			.uV = 1050000,
+		},
+		{
+			.supply = "cx",
+			.uA = 100000,
+		},
+		{
+			.supply = "pll",
+			.uA = 100000,
+		},
+		{ NULL }
+	},
+	.active_supply = (struct qcom_mss_reg_res[]) {
+		{
+			.supply = "mss",
+			.uV = 1050000,
+			.uA = 100000,
+		},
+		{ NULL }
+	},
 	.proxy_clk_string = (char*[]){"xo", NULL},
 	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},
 };
-- 
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: [PATCH v5 3/7] remoteproc: qcom: Add and initialize proxy and active regulators.

[Bjorn Andersson]

On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:

> -static int q6v5_regulator_init(struct q6v5 *qproc)
> +static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
> +				const struct qcom_mss_reg_res *reg_res)
>  {
> -	int ret;
> +	int count = 0;
> +	int rc;
> +	int 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";
> +	while (reg_res[count].supply)
> +	count++;
>  
> -	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;
> -	}
> +	for (i = 0; i < count; i++) {

As with the clock init you can squash these two loops into one now.

[..]
>  static const struct rproc_hexagon_res msm8916_mss = {
>  	.hexagon_mba_image = "mba.mbn",
> +	.proxy_supply = (struct qcom_mss_reg_res[]) {
> +		{
> +			.supply = "mx",
> +			.uV = 1050000,
> +		},
> +		{
> +			.supply = "cx",
> +			.uA = 100000,
> +		},
> +		{
> +			.supply = "pll",
> +			.uA = 100000,
> +		},
> +		{ NULL }

It's idiomatic to use {} instead of { NULL }, so please update this (but
not in the clock patch).

As with the clock patch, please squash patch 4 into this one - so that
we have regulators before and after applying this single patch.

Regards,
Bjorn

Re: [PATCH v5 3/7] remoteproc: qcom: Add and initialize proxy and active regulators.

[Dwivedi, Avaneesh Kumar (avani)]

On 12/23/2016 3:16 AM, Bjorn Andersson wrote:
> On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> -static int q6v5_regulator_init(struct q6v5 *qproc)
>> +static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
>> +				const struct qcom_mss_reg_res *reg_res)
>>   {
>> -	int ret;
>> +	int count = 0;
>> +	int rc;
>> +	int 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";
>> +	while (reg_res[count].supply)
>> +	count++;
>>   
>> -	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;
>> -	}
>> +	for (i = 0; i < count; i++) {
> As with the clock init you can squash these two loops into one now.
OK.
>
> [..]
>>   static const struct rproc_hexagon_res msm8916_mss = {
>>   	.hexagon_mba_image = "mba.mbn",
>> +	.proxy_supply = (struct qcom_mss_reg_res[]) {
>> +		{
>> +			.supply = "mx",
>> +			.uV = 1050000,
>> +		},
>> +		{
>> +			.supply = "cx",
>> +			.uA = 100000,
>> +		},
>> +		{
>> +			.supply = "pll",
>> +			.uA = 100000,
>> +		},
>> +		{ NULL }
> It's idiomatic to use {} instead of { NULL }, so please update this (but
> not in the clock patch).
OK.
>
> As with the clock patch, please squash patch 4 into this one - so that
> we have regulators before and after applying this single patch.
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.

Re: [PATCH v5 3/7] remoteproc: qcom: Add and initialize proxy and active regulators.

[Stephen Boyd]

On 12/15, Avaneesh Kumar Dwivedi wrote:
> @@ -148,29 +167,34 @@ enum {
>  	Q6V5_SUPPLY_PLL,
>  };
>  
> -static int q6v5_regulator_init(struct q6v5 *qproc)
> +static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
> +				const struct qcom_mss_reg_res *reg_res)
>  {
> -	int ret;
> +	int count = 0;
> +	int rc;
> +	int 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";
> +	while (reg_res[count].supply)
> +	count++;

Indent that count++ please.


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

[PATCH v5 4/7] remoteproc: qcom: Modify regulator enable and disable interface.

[Avaneesh Kumar Dwivedi]

Regulator enable/disable 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/down.

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

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index e95d118..e19fb9d 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -140,7 +140,6 @@ struct q6v5 {
 	int active_reg_count;
 	int proxy_reg_count;
 
-	struct regulator_bulk_data supply[4];
 
 	struct clk *ahb_clk;
 	struct clk *axi_clk;
@@ -160,12 +159,6 @@ struct q6v5 {
 	size_t mpss_size;
 };
 
-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,
 				const struct qcom_mss_reg_res *reg_res)
@@ -194,34 +187,69 @@ static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
 	return 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;
 
-	/* TODO: Q6V5_SUPPLY_CX is supposed to be set to super-turbo here */
+	for (i = 0; i < count; i++) {
+		if (regs[i].uV > 0) {
+			ret = regulator_set_voltage(regs[i].reg,
+					regs[i].uV, INT_MAX);
+			if (ret) {
+				dev_err(qproc->dev,
+					"Failed to request voltage for %d.\n",
+						i);
+				goto err;
+			}
+		}
 
-	ret = regulator_set_voltage(mx, 1050000, INT_MAX);
-	if (ret)
-		return ret;
+		if (regs[i].uA > 0) {
+			ret = regulator_set_load(regs[i].reg,
+						regs[i].uA);
+			if (ret < 0) {
+				dev_err(qproc->dev, "Failed to set regulator mode\n");
+				goto err;
+			}
+		}
+
+		ret = regulator_enable(regs[i].reg);
+		if (ret) {
+			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);
 
-	return regulator_bulk_enable(ARRAY_SIZE(qproc->supply), qproc->supply);
+		regulator_disable(regs[i].reg);
+	}
+
+	return ret;
 }
 
-static void q6v5_regulator_disable(struct q6v5 *qproc)
+static void q6v5_regulator_disable(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 i;
 
-	/* TODO: Q6V5_SUPPLY_CX corner votes should be released */
+	for (i = 0; i < 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_bulk_disable(ARRAY_SIZE(qproc->supply), qproc->supply);
-	regulator_set_voltage(mx, 0, INT_MAX);
-	regulator_set_voltage(mss, 0, 1150000);
+		regulator_disable(regs[i].reg);
+	}
 }
 
 static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
@@ -513,12 +541,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");
@@ -587,8 +622,11 @@ static int q6v5_start(struct rproc *rproc)
 assert_reset:
 	reset_control_assert(qproc->mss_restart);
 disable_vdd:
-	q6v5_regulator_disable(qproc);
-
+	q6v5_regulator_disable(qproc, qproc->active_regs,
+				qproc->active_reg_count);
+disable_proxy_reg:
+	q6v5_regulator_disable(qproc, qproc->proxy_regs,
+				qproc->proxy_reg_count);
 	return ret;
 }
 
@@ -617,8 +655,10 @@ static int q6v5_stop(struct rproc *rproc)
 	clk_disable_unprepare(qproc->rom_clk);
 	clk_disable_unprepare(qproc->axi_clk);
 	clk_disable_unprepare(qproc->ahb_clk);
-	q6v5_regulator_disable(qproc);
-
+	q6v5_regulator_disable(qproc, qproc->active_regs,
+				qproc->active_reg_count);
+	q6v5_regulator_disable(qproc, qproc->proxy_regs,
+				qproc->proxy_reg_count);
 	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: [PATCH v5 4/7] remoteproc: qcom: Modify regulator enable and disable interface.

[Bjorn Andersson]

On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:

> Regulator enable/disable 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/down.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>

Looks good, please squash into patch 3.

Regards,
Bjorn

[PATCH v5 5/7] remoteproc: qcom: Modify clock enable and disable interface.

[Avaneesh Kumar Dwivedi]

Clock enable/disable routine will get additional input parameter of
pointer of array of clock struct's and clock count, it will use these
pre initialized values to turn them up/down.

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

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index e19fb9d..1018b8f 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -140,11 +140,6 @@ struct q6v5 {
 	int active_reg_count;
 	int proxy_reg_count;
 
-
-	struct clk *ahb_clk;
-	struct clk *axi_clk;
-	struct clk *rom_clk;
-
 	struct completion start_done;
 	struct completion stop_done;
 	bool running;
@@ -252,6 +247,38 @@ static void q6v5_regulator_disable(struct q6v5 *qproc,
 	}
 }
 
+
+static int q6v5_clk_enable(struct device *dev,
+			struct clk **clks, int count)
+{
+	int rc;
+	int i;
+
+	for (i = 0; i < 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_clk_disable(struct device *dev,
+			struct clk **clks, int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++)
+		clk_disable_unprepare(clks[i]);
+}
+
 static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
 {
 	struct q6v5 *qproc = rproc->priv;
@@ -548,11 +575,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) {
@@ -560,17 +594,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);
 
@@ -605,25 +634,26 @@ static int q6v5_start(struct rproc *rproc)
 
 	qproc->running = true;
 
-	/* TODO: All done, release the handover resources */
-
+	q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
+				qproc->proxy_clk_count);
+	q6v5_regulator_disable(qproc, qproc->proxy_regs,
+				qproc->proxy_reg_count);
 	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_clk_disable(qproc->dev, qproc->active_clks,
+				qproc->active_clk_count);
 assert_reset:
 	reset_control_assert(qproc->mss_restart);
 disable_vdd:
 	q6v5_regulator_disable(qproc, qproc->active_regs,
 				qproc->active_reg_count);
+disable_proxy_clk:
+	q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
+				qproc->proxy_clk_count);
 disable_proxy_reg:
 	q6v5_regulator_disable(qproc, qproc->proxy_regs,
 				qproc->proxy_reg_count);
@@ -652,9 +682,10 @@ 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->dev, qproc->active_clks,
+				qproc->active_clk_count);
+	q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
+				qproc->proxy_clk_count);
 	q6v5_regulator_disable(qproc, qproc->active_regs,
 				qproc->active_reg_count);
 	q6v5_regulator_disable(qproc, qproc->proxy_regs,
-- 
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: [PATCH v5 5/7] remoteproc: qcom: Modify clock enable and disable interface.

[Bjorn Andersson]

On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:

> Clock enable/disable routine will get additional input parameter of
> pointer of array of clock struct's and clock count, it will use these
> pre initialized values to turn them up/down.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>

Looks good, please squash into the clock-patch.

Regards,
Bjorn

[PATCH v5 6/7] remoteproc: qcom: Implement Hexagon core specific changes.

[Avaneesh Kumar Dwivedi]

This change introduces appropriate additional steps in reset sequence and
stop routine corresponding to hexagon v56 1.5.0 on mss for msm8996.

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

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 674ebc6..06f51a6 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -10,6 +10,7 @@ on the Qualcomm Hexagon core.
 		"qcom,q6v5-pil"
 		"qcom,msm8916-mss-pil"
 		"qcom,msm8974-mss-pil"
+		"qcom,msm8996-mss-pil"
 
 - reg:
 	Usage: required
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 1018b8f..fb04461 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -31,6 +31,7 @@
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/smem_state.h>
 #include <linux/of_device.h>
+#include <linux/iopoll.h>
 
 #include "remoteproc_internal.h"
 #include "qcom_mdt_loader.h"
@@ -65,6 +66,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 +96,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 reg_info {
 	struct regulator *reg;
 	int uV;
@@ -111,6 +123,7 @@ struct rproc_hexagon_res {
 	struct qcom_mss_reg_res active_supply[2];
 	char **proxy_clk_string;
 	char **active_clk_string;
+	int hexagon_ver;
 };
 
 struct q6v5 {
@@ -152,8 +165,14 @@ struct q6v5 {
 	phys_addr_t mpss_reloc;
 	void *mpss_region;
 	size_t mpss_size;
+	int hexagon_ver;
 };
 
+enum {
+	MSS_MSM8916, /*hexagon on msm8916*/
+	MSS_MSM8974, /*hexagon on msm8974*/
+	MSS_MSM8996, /*hexagon on msm8996*/
+};
 
 static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
 				const struct qcom_mss_reg_res *reg_res)
@@ -341,35 +360,107 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
 
 static int q6v5proc_reset(struct q6v5 *qproc)
 {
-	u32 val;
+	u64 val;
 	int ret;
+	int i;
 
-	/* 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);
 
-	/* 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);
+	if (qproc->hexagon_ver == 0x2) {
+		/* Override the ACC value if required */
+		writel(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 */
+		val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
+		val |= 0x1;
+		writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
+		ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
+				val, !(val & BIT(31)), 1, HALT_CHECK_MAX_LOOPS);
+		if (ret) {
+			dev_err(qproc->dev,
+				"xo cbcr enabling timed out (rc:%d)\n", ret);
+			return ret;
+		}
+		if ((val & BIT(31))) {
+			dev_err(qproc->dev,
+				"Failed to enable xo branch clock.\n");
+			return -EINVAL;
+		}
+		/*
+		 * Enable power block headswitch,
+		 * and wait for it to stabilize
+		 */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= QDSP6v56_BHS_ON;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		udelay(1);
+		/* Put LDO in bypass mode */
+		val |= QDSP6v56_LDO_BYP;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		/*
+		 * Deassert QDSP6 compiler memory clamp
+		 */
+		val = readl(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(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+		/* Turn on L1, L2, ETB and JU memories 1 at a time */
+		val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+		for (i = 19; i >= 0; i--) {
+			val |= BIT(i);
+			writel(val, qproc->reg_base +
+						QDSP6SS_MEM_PWR_CTL);
+			/*
+			 * Wait for 1us for both memory peripheral and
+			 * data array to turn on.
+			 */
+			 val |= readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+			udelay(1);
+		}
+		/* Remove word line clamp */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val &= ~QDSP6v56_CLAMP_WL;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	} else {
+		/* 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);
+		/*
+		 * 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);
+		val |= readl(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);
+	}
 	/* Remove IO clamp */
 	val &= ~Q6SS_CLAMP_IO;
 	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
@@ -664,6 +755,7 @@ static int q6v5_stop(struct rproc *rproc)
 {
 	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
 	int ret;
+	int val;
 
 	qproc->running = false;
 
@@ -681,6 +773,15 @@ 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) {
+		/*
+		 * To avoid high MX current during LPASS/MSS restart.
+		 */
+		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->dev, qproc->active_clks,
 				qproc->active_clk_count);
@@ -987,6 +1088,7 @@ static int q6v5_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
+	qproc->hexagon_ver = desc->hexagon_ver;
 	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
 	if (ret < 0)
 		goto free_rproc;
@@ -1031,6 +1133,29 @@ static int q6v5_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct rproc_hexagon_res msm8996_mss = {
+	.hexagon_mba_image = "mba.mbn",
+	.proxy_supply = (struct qcom_mss_reg_res[]) {
+		{
+			.supply = "mx",
+		},
+		{
+			.supply = "cx",
+			.uA = 100000,
+		},
+		{
+			.supply = "pll",
+			.uA = 100000,
+		},
+		{ NULL }
+	},
+	.active_supply = (struct qcom_mss_reg_res[]) { { NULL }, { NULL } },
+	.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 = MSS_MSM8996,
+};
+
 static const struct rproc_hexagon_res msm8916_mss = {
 	.hexagon_mba_image = "mba.mbn",
 	.proxy_supply = (struct qcom_mss_reg_res[]) {
@@ -1058,6 +1183,7 @@ static int q6v5_remove(struct platform_device *pdev)
 	},
 	.proxy_clk_string = (char*[]){"xo", NULL},
 	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},
+	.hexagon_ver = MSS_MSM8916,
 };
 
 static const struct rproc_hexagon_res msm8974_mss = {
@@ -1087,11 +1213,14 @@ static int q6v5_remove(struct platform_device *pdev)
 	},
 	.proxy_clk_string = (char*[]){"xo", NULL},
 	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},
+	.hexagon_ver = MSS_MSM8974,
 };
+
 static const struct of_device_id q6v5_of_match[] = {
 	{ .compatible = "qcom,q6v5-pil", .data = &msm8916_mss},
 	{ .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss},
 	{ .compatible = "qcom,msm8974-mss-pil", .data = &msm8974_mss},
+	{ .compatible = "qcom,msm8996-mss-pil", .data = &msm8996_mss},
 	{ },
 };
 
-- 
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: [PATCH v5 6/7] remoteproc: qcom: Implement Hexagon core specific changes.

[Bjorn Andersson]

On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:

> +/* 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

Indent these with tabs, like the others.

> +#define QDSP6SS_ACC_OVERRIDE_VAL	0x20
> +
>  struct reg_info {
>  	struct regulator *reg;
>  	int uV;
> @@ -111,6 +123,7 @@ struct rproc_hexagon_res {
>  	struct qcom_mss_reg_res active_supply[2];
>  	char **proxy_clk_string;
>  	char **active_clk_string;
> +	int hexagon_ver;
>  };
>  
>  struct q6v5 {
> @@ -152,8 +165,14 @@ struct q6v5 {
>  	phys_addr_t mpss_reloc;
>  	void *mpss_region;
>  	size_t mpss_size;
> +	int hexagon_ver;

Please name this "version" instead, there's little confusion to what it
is.

>  };
>  
> +enum {
> +	MSS_MSM8916, /*hexagon on msm8916*/
> +	MSS_MSM8974, /*hexagon on msm8974*/
> +	MSS_MSM8996, /*hexagon on msm8996*/

You can skip the comments now.

> +};
>  
>  static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
>  				const struct qcom_mss_reg_res *reg_res)
> @@ -341,35 +360,107 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>  
>  static int q6v5proc_reset(struct q6v5 *qproc)
>  {
> -	u32 val;
> +	u64 val;
>  	int ret;
> +	int i;
>  
> -	/* 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);
>  
> -	/* 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);
> +	if (qproc->hexagon_ver == 0x2) {

== MSS_MSM8996

> +		/* Override the ACC value if required */
> +		writel(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 */
> +		val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
> +		val |= 0x1;
> +		writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);

Please add a comment here, describing what we're waiting for (rather
than just "a magical bit 31")

> +		ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
> +				val, !(val & BIT(31)), 1, HALT_CHECK_MAX_LOOPS);
> +		if (ret) {
> +			dev_err(qproc->dev,
> +				"xo cbcr enabling timed out (rc:%d)\n", ret);
> +			return ret;
> +		}
> +		if ((val & BIT(31))) {

If bit 31 is set when the readl_poll_timeout() hits the timeout the
condition will evaluate to false and "ret" will be -ETIMEDOUT. So I
don't think this can happen.

> +			dev_err(qproc->dev,
> +				"Failed to enable xo branch clock.\n");
> +			return -EINVAL;
> +		}
> +		/*
> +		 * Enable power block headswitch,
> +		 * and wait for it to stabilize

This comment fits within 80 characters, so no need to line wrap it.

> +		 */
> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val |= QDSP6v56_BHS_ON;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		udelay(1);

Please insert a single empty line here, to create some separation
between the "steps".

> +		/* Put LDO in bypass mode */
> +		val |= QDSP6v56_LDO_BYP;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		/*
> +		 * Deassert QDSP6 compiler memory clamp
> +		 */
> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val &= ~QDSP6v56_CLAMP_QMC_MEM;
> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);

Please drop the _relaxed

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

/* Read back value to ensure the write is done */

And move the 1us comment below the read.

> +			 val |= readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);

Please correct the indentation of this line.

> +			udelay(1);
> +		}
> +		/* Remove word line clamp */
> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val &= ~QDSP6v56_CLAMP_WL;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	} else {
> +		/* 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);
> +		/*
> +		 * 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);
> +		val |= readl(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);
> +	}
>  	/* Remove IO clamp */
>  	val &= ~Q6SS_CLAMP_IO;
>  	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> @@ -664,6 +755,7 @@ static int q6v5_stop(struct rproc *rproc)
>  {
>  	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>  	int ret;
> +	int val;
>  
>  	qproc->running = false;
>  
> @@ -681,6 +773,15 @@ 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) {

== MSS_MSM8996

> +		/*
> +		 * To avoid high MX current during LPASS/MSS restart.
> +		 */
> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);

Please no _relaxed()

> +		val |= Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL |
> +			QDSP6v56_CLAMP_QMC_MEM;
> +		writel_relaxed(val,	qproc->reg_base + QDSP6SS_PWR_CTL_REG);

Dito

> +	}
>  	reset_control_assert(qproc->mss_restart);
>  	q6v5_clk_disable(qproc->dev, qproc->active_clks,
>  				qproc->active_clk_count);

Regards,
Bjorn

Re: [PATCH v5 6/7] remoteproc: qcom: Implement Hexagon core specific changes.

[Bjorn Andersson]

On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:

> +/* 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

Indent these with tabs, like the others.

> +#define QDSP6SS_ACC_OVERRIDE_VAL	0x20
> +
>  struct reg_info {
>  	struct regulator *reg;
>  	int uV;
> @@ -111,6 +123,7 @@ struct rproc_hexagon_res {
>  	struct qcom_mss_reg_res active_supply[2];
>  	char **proxy_clk_string;
>  	char **active_clk_string;
> +	int hexagon_ver;
>  };
>  
>  struct q6v5 {
> @@ -152,8 +165,14 @@ struct q6v5 {
>  	phys_addr_t mpss_reloc;
>  	void *mpss_region;
>  	size_t mpss_size;
> +	int hexagon_ver;

Please name this "version" instead, there's little confusion to what it
is.

>  };
>  
> +enum {
> +	MSS_MSM8916, /*hexagon on msm8916*/
> +	MSS_MSM8974, /*hexagon on msm8974*/
> +	MSS_MSM8996, /*hexagon on msm8996*/

You can skip the comments now.

> +};
>  
>  static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
>  				const struct qcom_mss_reg_res *reg_res)
> @@ -341,35 +360,107 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>  
>  static int q6v5proc_reset(struct q6v5 *qproc)
>  {
> -	u32 val;
> +	u64 val;
>  	int ret;
> +	int i;
>  
> -	/* 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);
>  
> -	/* 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);
> +	if (qproc->hexagon_ver == 0x2) {

== MSS_MSM8996

> +		/* Override the ACC value if required */
> +		writel(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 */
> +		val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
> +		val |= 0x1;
> +		writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);

Please add a comment here, describing what we're waiting for (rather
than just "a magical bit 31")

> +		ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
> +				val, !(val & BIT(31)), 1, HALT_CHECK_MAX_LOOPS);
> +		if (ret) {
> +			dev_err(qproc->dev,
> +				"xo cbcr enabling timed out (rc:%d)\n", ret);
> +			return ret;
> +		}
> +		if ((val & BIT(31))) {

If bit 31 is set when the readl_poll_timeout() hits the timeout the
condition will evaluate to false and "ret" will be -ETIMEDOUT. So I
don't think this can happen.

> +			dev_err(qproc->dev,
> +				"Failed to enable xo branch clock.\n");
> +			return -EINVAL;
> +		}
> +		/*
> +		 * Enable power block headswitch,
> +		 * and wait for it to stabilize

This comment fits within 80 characters, so no need to line wrap it.

> +		 */
> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val |= QDSP6v56_BHS_ON;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		udelay(1);

Please insert a single empty line here, to create some separation
between the "steps".

> +		/* Put LDO in bypass mode */
> +		val |= QDSP6v56_LDO_BYP;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		/*
> +		 * Deassert QDSP6 compiler memory clamp
> +		 */
> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val &= ~QDSP6v56_CLAMP_QMC_MEM;
> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);

Please drop the _relaxed

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

/* Read back value to ensure the write is done */

And move the 1us comment below the read.

> +			 val |= readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);

Please correct the indentation of this line.

> +			udelay(1);
> +		}
> +		/* Remove word line clamp */
> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val &= ~QDSP6v56_CLAMP_WL;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	} else {
> +		/* 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);
> +		/*
> +		 * 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);
> +		val |= readl(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);
> +	}
>  	/* Remove IO clamp */
>  	val &= ~Q6SS_CLAMP_IO;
>  	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> @@ -664,6 +755,7 @@ static int q6v5_stop(struct rproc *rproc)
>  {
>  	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>  	int ret;
> +	int val;
>  
>  	qproc->running = false;
>  
> @@ -681,6 +773,15 @@ 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) {

== MSS_MSM8996

> +		/*
> +		 * To avoid high MX current during LPASS/MSS restart.
> +		 */
> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);

Please no _relaxed()

> +		val |= Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL |
> +			QDSP6v56_CLAMP_QMC_MEM;
> +		writel_relaxed(val,	qproc->reg_base + QDSP6SS_PWR_CTL_REG);

Dito

> +	}
>  	reset_control_assert(qproc->mss_restart);
>  	q6v5_clk_disable(qproc->dev, qproc->active_clks,
>  				qproc->active_clk_count);

Regards,
Bjorn

Re: [PATCH v5 6/7] remoteproc: qcom: Implement Hexagon core specific changes.

[Dwivedi, Avaneesh Kumar (avani)]

On 12/23/2016 6:34 AM, Bjorn Andersson wrote:
> On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> +/* 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
> Indent these with tabs, like the others.
Ok.
>
>> +#define QDSP6SS_ACC_OVERRIDE_VAL	0x20
>> +
>>   struct reg_info {
>>   	struct regulator *reg;
>>   	int uV;
>> @@ -111,6 +123,7 @@ struct rproc_hexagon_res {
>>   	struct qcom_mss_reg_res active_supply[2];
>>   	char **proxy_clk_string;
>>   	char **active_clk_string;
>> +	int hexagon_ver;
>>   };
>>   
>>   struct q6v5 {
>> @@ -152,8 +165,14 @@ struct q6v5 {
>>   	phys_addr_t mpss_reloc;
>>   	void *mpss_region;
>>   	size_t mpss_size;
>> +	int hexagon_ver;
> Please name this "version" instead, there's little confusion to what it
> is.
OK.
>>   };
>>   
>> +enum {
>> +	MSS_MSM8916, /*hexagon on msm8916*/
>> +	MSS_MSM8974, /*hexagon on msm8974*/
>> +	MSS_MSM8996, /*hexagon on msm8996*/
> You can skip the comments now.
OK.
>
>> +};
>>   
>>   static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
>>   				const struct qcom_mss_reg_res *reg_res)
>> @@ -341,35 +360,107 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>>   
>>   static int q6v5proc_reset(struct q6v5 *qproc)
>>   {
>> -	u32 val;
>> +	u64 val;
>>   	int ret;
>> +	int i;
>>   
>> -	/* 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);
>>   
>> -	/* 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);
>> +	if (qproc->hexagon_ver == 0x2) {
> == MSS_MSM8996
OK.
>> +		/* Override the ACC value if required */
>> +		writel(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 */
>> +		val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
>> +		val |= 0x1;
>> +		writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
> Please add a comment here, describing what we're waiting for (rather
> than just "a magical bit 31")
In off condition bit 31 (CLKOFF) bit is set as 1, when we write 0x1 to 
this register and clock is turned on it become 0x0.
will add appropriate comment.
>
>> +		ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
>> +				val, !(val & BIT(31)), 1, HALT_CHECK_MAX_LOOPS);
>> +		if (ret) {
>> +			dev_err(qproc->dev,
>> +				"xo cbcr enabling timed out (rc:%d)\n", ret);
>> +			return ret;
>> +		}
>> +		if ((val & BIT(31))) {
> If bit 31 is set when the readl_poll_timeout() hits the timeout the
> condition will evaluate to false and "ret" will be -ETIMEDOUT. So I
> don't think this can happen.
Sure this is not required. Will remove it
>> +			dev_err(qproc->dev,
>> +				"Failed to enable xo branch clock.\n");
>> +			return -EINVAL;
>> +		}
>> +		/*
>> +		 * Enable power block headswitch,
>> +		 * and wait for it to stabilize
> This comment fits within 80 characters, so no need to line wrap it.
OK, i had wrapped it because i was getting warning on running 
checkpatch.pl, if fit in one line without warning will turn it into one 
line comment.
>> +		 */
>> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val |= QDSP6v56_BHS_ON;
>> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		udelay(1);
> Please insert a single empty line here, to create some separation
> between the "steps".
OK.
>
>> +		/* Put LDO in bypass mode */
>> +		val |= QDSP6v56_LDO_BYP;
>> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		/*
>> +		 * Deassert QDSP6 compiler memory clamp
>> +		 */
>> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val &= ~QDSP6v56_CLAMP_QMC_MEM;
>> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> Please drop the _relaxed
Sorry for repeated comments on this. Will do it for sure.
>
>> +
>> +		/* Deassert memory peripheral sleep and L2 memory standby */
>> +		val |= (Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N);
>> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +		/* Turn on L1, L2, ETB and JU memories 1 at a time */
>> +		val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
>> +		for (i = 19; i >= 0; i--) {
>> +			val |= BIT(i);
>> +			writel(val, qproc->reg_base +
>> +						QDSP6SS_MEM_PWR_CTL);
>> +			/*
>> +			 * Wait for 1us for both memory peripheral and
>> +			 * data array to turn on.
>> +			 */
> /* Read back value to ensure the write is done */
>
> And move the 1us comment below the read.
Sure.
>
>> +			 val |= readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
> Please correct the indentation of this line.
OK.
>
>> +			udelay(1);
>> +		}
>> +		/* Remove word line clamp */
>> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val &= ~QDSP6v56_CLAMP_WL;
>> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	} else {
>> +		/* 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);
>> +		/*
>> +		 * 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);
>> +		val |= readl(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);
>> +	}
>>   	/* Remove IO clamp */
>>   	val &= ~Q6SS_CLAMP_IO;
>>   	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> @@ -664,6 +755,7 @@ static int q6v5_stop(struct rproc *rproc)
>>   {
>>   	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>>   	int ret;
>> +	int val;
>>   
>>   	qproc->running = false;
>>   
>> @@ -681,6 +773,15 @@ 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) {
> == MSS_MSM8996
OK
>> +		/*
>> +		 * To avoid high MX current during LPASS/MSS restart.
>> +		 */
>> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> Please no _relaxed()
OK
>
>> +		val |= Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL |
>> +			QDSP6v56_CLAMP_QMC_MEM;
>> +		writel_relaxed(val,	qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> Dito
OK
>
>> +	}
>>   	reset_control_assert(qproc->mss_restart);
>>   	q6v5_clk_disable(qproc->dev, qproc->active_clks,
>>   				qproc->active_clk_count);
> 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.

[PATCH v5 7/7] clk: qcom: Add GCC_MSS_RESET support

[Avaneesh Kumar Dwivedi]

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

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.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: [PATCH v5 7/7] clk: qcom: Add GCC_MSS_RESET support

[Stephen Boyd]

On 12/15, Avaneesh Kumar Dwivedi wrote:
> Add support to use reset control framework for resetting MSS
> with hexagon v56 1.5.0.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---

Applied to clk-next. I take it the dts part won't be landing in
v4.11 so no need to make a special branch for this new define.

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

Re: [PATCH v5 7/7] clk: qcom: Add GCC_MSS_RESET support

[Andy Gross]

On Wed, Dec 28, 2016 at 03:40:16PM -0800, Stephen Boyd wrote:
> On 12/15, Avaneesh Kumar Dwivedi wrote:
> > Add support to use reset control framework for resetting MSS
> > with hexagon v56 1.5.0.
> > 
> > Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> 
> Applied to clk-next. I take it the dts part won't be landing in
> v4.11 so no need to make a special branch for this new define.

I'd prefer to avoid this by using the hard coded values instead of #defines
until the pieces are in place.  So any DT changes sent during that time period
will not be accepted if they use the #define, or I will munge them myself.

Regards,

Andy

Re: [PATCH v5 7/7] clk: qcom: Add GCC_MSS_RESET support

[Bjorn Andersson]

On Thu 29 Dec 09:09 PST 2016, Andy Gross wrote:

> On Wed, Dec 28, 2016 at 03:40:16PM -0800, Stephen Boyd wrote:
> > On 12/15, Avaneesh Kumar Dwivedi wrote:
> > > Add support to use reset control framework for resetting MSS
> > > with hexagon v56 1.5.0.
> > > 
> > > Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > ---
> > 
> > Applied to clk-next. I take it the dts part won't be landing in
> > v4.11 so no need to make a special branch for this new define.
> 
> I'd prefer to avoid this by using the hard coded values instead of #defines
> until the pieces are in place.  So any DT changes sent during that time period
> will not be accepted if they use the #define, or I will munge them myself.
> 

In testing of the Hexagon PIL on "real" hardware we ran into PBL error
-284098560, so we will need to get the SCM calls for assigning memory
permissions (SCM_SVC_MP/MEM_PROT_ASSIGN_ID) to move forward with the
8996 PIL.

In addition to this we also need a few more gcc clocks and working rpmcc
on 8996 (i.e. GLINK).


So we don't have to worry about this define in v4.11.

Thanks for picking up the patch Stephen.

Regards,
Bjorn

[PATCH v5 0/7] remoteproc: qcom: Add support of mss rproc for msm8996.

[Avaneesh Kumar Dwivedi]

This is patchset v5 having modifications as per comment on patchset v4.
Major changes w.r.t. patchset v4 are as below.	
	1- Compatible string and private struct naming according to platform.
	2- Separation of regulator and clock init.
	3- Regulator Init implementation change.
	4- Regulator and clock disable interface and implementation change.
	5- Reorganization of reset sequence.
	6- Other Minor comments related changes.

Avaneesh Kumar Dwivedi (7):
  remoteproc: qcom: Add private resource struct and new compatible.
  remoteproc: qcom: Add and initialize proxy and active clocks.
  remoteproc: qcom: Add and initialize proxy and active regulators.
  remoteproc: qcom: Modify regulator enable and disable interface.
  remoteproc: qcom: Modify clock enable and disable interface.
  remoteproc: qcom: Implement Hexagon core specific changes.
  clk: qcom: Add GCC_MSS_RESET support

 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   5 +-
 drivers/clk/qcom/gcc-msm8996.c                     |   1 +
 drivers/remoteproc/qcom_q6v5_pil.c                 | 562 ++++++++++++++++-----
 include/dt-bindings/clock/qcom,gcc-msm8996.h       |   1 +
 4 files changed, 452 insertions(+), 117 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.

[PATCH v5 3/7] remoteproc: qcom: Add and initialize proxy and active regulators.

[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 respectively, whose handle is being obtained
by supplying proxy and active regulator string name.

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

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 8c8b239..e95d118 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -93,8 +93,22 @@
 #define QDSS_BHS_ON			BIT(21)
 #define QDSS_LDO_BYP			BIT(22)
 
+struct reg_info {
+	struct regulator *reg;
+	int uV;
+	int uA;
+};
+
+struct qcom_mss_reg_res {
+	const char *supply;
+	int uV;
+	int uA;
+};
+
 struct rproc_hexagon_res {
 	const char *hexagon_mba_image;
+	struct qcom_mss_reg_res proxy_supply[4];
+	struct qcom_mss_reg_res active_supply[2];
 	char **proxy_clk_string;
 	char **active_clk_string;
 };
@@ -121,6 +135,11 @@ struct q6v5 {
 	int active_clk_count;
 	int proxy_clk_count;
 
+	struct reg_info active_regs[1];
+	struct reg_info proxy_regs[3];
+	int active_reg_count;
+	int proxy_reg_count;
+
 	struct regulator_bulk_data supply[4];
 
 	struct clk *ahb_clk;
@@ -148,29 +167,34 @@ enum {
 	Q6V5_SUPPLY_PLL,
 };
 
-static int q6v5_regulator_init(struct q6v5 *qproc)
+static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
+				const struct qcom_mss_reg_res *reg_res)
 {
-	int ret;
+	int count = 0;
+	int rc;
+	int 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";
+	while (reg_res[count].supply)
+	count++;
 
-	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;
-	}
+	for (i = 0; i < count; i++) {
+		regs[i].reg = devm_regulator_get(dev, reg_res[i].supply);
+		if (IS_ERR(regs[i].reg)) {
+			rc = PTR_ERR(regs[i].reg);
+			if (rc != -EPROBE_DEFER)
+				dev_err(dev, "Failed to get %s\n regulator",
+						reg_res[i].supply);
+			return rc;
+		}
 
-	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);
+		regs[i].uV = reg_res[i].uV;
+		regs[i].uA = reg_res[i].uA;
+	}
 
-	return 0;
+	return count;
 }
 
+
 static int q6v5_regulator_enable(struct q6v5 *qproc)
 {
 	struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer;
@@ -872,9 +896,21 @@ static int q6v5_probe(struct platform_device *pdev)
 	}
 	qproc->active_clk_count = ret;
 
-	ret = q6v5_regulator_init(qproc);
-	if (ret)
+	ret = q6v5_regulator_init(&pdev->dev, qproc->proxy_regs,
+					desc->proxy_supply);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to setup proxy regulators.\n");
 		goto free_rproc;
+	}
+	qproc->proxy_reg_count = ret;
+
+	ret = q6v5_regulator_init(&pdev->dev,  qproc->active_regs,
+					desc->active_supply);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to setup active regulators.\n");
+		goto free_rproc;
+	}
+	qproc->active_reg_count = ret;
 
 	ret = q6v5_init_reset(qproc);
 	if (ret)
@@ -926,12 +962,58 @@ static int q6v5_remove(struct platform_device *pdev)
 
 static const struct rproc_hexagon_res msm8916_mss = {
 	.hexagon_mba_image = "mba.mbn",
+	.proxy_supply = (struct qcom_mss_reg_res[]) {
+		{
+			.supply = "mx",
+			.uV = 1050000,
+		},
+		{
+			.supply = "cx",
+			.uA = 100000,
+		},
+		{
+			.supply = "pll",
+			.uA = 100000,
+		},
+		{ NULL }
+	},
+	.active_supply = (struct qcom_mss_reg_res[]) {
+		{
+			.supply = "mss",
+			.uV = 1050000,
+			.uA = 100000,
+		},
+		{ NULL }
+	},
 	.proxy_clk_string = (char*[]){"xo", NULL},
 	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},
 };
 
 static const struct rproc_hexagon_res msm8974_mss = {
 	.hexagon_mba_image = "mba.b00",
+	.proxy_supply = (struct qcom_mss_reg_res[]) {
+		{
+			.supply = "mx",
+			.uV = 1050000,
+		},
+		{
+			.supply = "cx",
+			.uA = 100000,
+		},
+		{
+			.supply = "pll",
+			.uA = 100000,
+		},
+		{ NULL }
+	},
+	.active_supply = (struct qcom_mss_reg_res[]) {
+		{
+			.supply = "mss",
+			.uV = 1050000,
+			.uA = 100000,
+		},
+		{ NULL }
+	},
 	.proxy_clk_string = (char*[]){"xo", NULL},
 	.active_clk_string = (char*[]){"iface", "bus", "mem", NULL},
 };
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.