[PATCH v3 0/3] reproc: qcom: Add support to hexagon q6v56 in qcom hexagon rproc driver

[PATCH v3 0/3] reproc: qcom: Add support to hexagon q6v56 in qcom hexagon rproc driver

[Avaneesh Kumar Dwivedi]

This is patchset v3 having modifications to address kbuild root error from patchset v2 and to
address patchset v1 comments. Major changes w.r.t. patchset v1 are as below.
	1- Separate functions for resource handling and start stop
	have been done away with.
	
	2- A private structure is defined which is being initialized
	with hexagon version specific data and being passed to probe
	via device structure. This in turn is passed as argument to various
	resource handling routines so that same code can do resource 
	manipulation without code change when new hexagon version need to be 
	supported.
	
	3- Some existing function name have been changed to reflect version
	independent driver.
	
	4- Comments regarding use of appropriate hexagon version have been
	addressed
	
	5- Comments regarding dividing all changes in logically complete
	patchset have been addressed to best possible way
	
	6- Comments regarding clock initialization in an array and not setting
	volatage for vdd pll before enabling the reg. have been addressed
	
	7- XO clk node have been added even for q6v5
	
	8- Wrapper functions to maintain boot count have been removed 
	as per comment received in last patchset.
	
	9- Unvoting of clocks and regulators have been moved in q6_start function
	as per comment on last patchset.

There were certain comments which could not be addressed 
	1- To program MSS_RESET via GCC reset controller
	Infact i had done it the way suggested before i sent out initial
	patchset, but then when i discussed with internal clock team, they said they will
	not support MSS RESET to be done via GCC because
	
	"MSS_RESET is not a block reset, GCC reset controller is used only when we need a BCR to be reset, 
	and which has a special purpose. MSS RESET doesn't fall under block resets, it is not a clock and 
	cannot be associated with clock."
	
This patchset is verified on top of kernel 4.9.rc4.
below is console o/p
[    3.304426]  remoteproc1: powering up 2080000.qcom,mss
[    3.304435]  remoteproc1: Booting fw image mba.mbn, size 213888
[    4.518946]  remoteproc1: remote processor 2080000.qcom,mss is now up

Avaneesh Kumar Dwivedi (3):
  remoteproc: qcom: Embed private data structure for hexagon dsp.
  remoteproc: qcom: Hexagon resource handling
  remoteproc: qcom: Adding q6v56 reset sequence.

 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   1 +
 drivers/remoteproc/qcom_q6v5_pil.c                 | 747 ++++++++++++++++-----
 2 files changed, 566 insertions(+), 182 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 v3 1/3] remoteproc: qcom: Embed private data structure for hexagon dsp.

[Avaneesh Kumar Dwivedi]

Embed resources specific to version of hexagon chip in device
structure to avoid conditional check for manipulation of those
resources in driver code.

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

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 57cb49e..cbc165c 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -8,6 +8,7 @@ on the Qualcomm Hexagon core.
 	Value type: <string>
 	Definition: must be one of:
 		    "qcom,q6v5-pil"
+		"qcom,q6v56-pil"
 
 - reg:
 	Usage: required
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 2e0caaa..b60dff3 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,13 +93,32 @@
 #define QDSS_BHS_ON			BIT(21)
 #define QDSS_LDO_BYP			BIT(22)
 
+struct q6_rproc_res {
+	char **proxy_clks;
+	int proxy_clk_cnt;
+	char **active_clks;
+	int active_clk_cnt;
+	char **proxy_regs;
+	int proxy_reg_cnt;
+	char **active_regs;
+	int active_reg_cnt;
+	int **proxy_reg_action;
+	int **active_reg_action;
+	int *proxy_reg_load;
+	int *active_reg_load;
+	int *proxy_reg_voltage;
+	int *active_reg_voltage;
+	char *q6_version;
+	char *q6_mba_image;
+	int (*q6_reset_init)(void *q, void *p);
+};
 struct q6v5 {
 	struct device *dev;
 	struct rproc *rproc;
 
 	void __iomem *reg_base;
 	void __iomem *rmb_base;
-
+	void __iomem *restart_reg;
 	struct regmap *halt_map;
 	u32 halt_q6;
 	u32 halt_modem;
@@ -111,7 +130,7 @@ struct q6v5 {
 	unsigned stop_bit;
 
 	struct regulator_bulk_data supply[4];
-
+	const struct q6_rproc_res *q6_rproc_res;
 	struct clk *ahb_clk;
 	struct clk *axi_clk;
 	struct clk *rom_clk;
@@ -198,7 +217,7 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
 	return 0;
 }
 
-static const struct rproc_fw_ops q6v5_fw_ops = {
+static const struct rproc_fw_ops q6_fw_ops = {
 	.find_rsc_table = qcom_mdt_find_rsc_table,
 	.load = q6v5_load,
 };
@@ -599,7 +618,7 @@ static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len)
 	return qproc->mpss_region + offset;
 }
 
-static const struct rproc_ops q6v5_ops = {
+static const struct rproc_ops q6_ops = {
 	.start = q6v5_start,
 	.stop = q6v5_stop,
 	.da_to_va = q6v5_da_to_va,
@@ -725,17 +744,36 @@ static int q6v5_init_clocks(struct q6v5 *qproc)
 	return 0;
 }
 
-static int q6v5_init_reset(struct q6v5 *qproc)
+static int q6v5_init_reset(void *q, void *p)
 {
-	qproc->mss_restart = devm_reset_control_get(qproc->dev, NULL);
+	struct q6v5 *qproc = q;
+	struct platform_device *pdev = p;
+
+	qproc->mss_restart = devm_reset_control_get(&pdev->dev, NULL);
 	if (IS_ERR(qproc->mss_restart)) {
-		dev_err(qproc->dev, "failed to acquire mss restart\n");
+		dev_err(&pdev->dev, "failed to acquire mss restart\n");
 		return PTR_ERR(qproc->mss_restart);
 	}
 
 	return 0;
 }
 
+static int q6v56_init_reset(void *q, void *p)
+{
+	struct resource *res;
+	struct q6v5 *qproc = q;
+	struct platform_device *pdev = p;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "restart_reg");
+	qproc->restart_reg = devm_ioremap(qproc->dev, res->start,
+							resource_size(res));
+	if (IS_ERR(qproc->restart_reg)) {
+		dev_err(qproc->dev, "failed to get restart_reg\n");
+		return PTR_ERR(qproc->restart_reg);
+	}
+
+	return 0;
+}
 static int q6v5_request_irq(struct q6v5 *qproc,
 			     struct platform_device *pdev,
 			     const char *name,
@@ -803,20 +841,25 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
 	return 0;
 }
 
-static int q6v5_probe(struct platform_device *pdev)
+static int q6_probe(struct platform_device *pdev)
 {
 	struct q6v5 *qproc;
 	struct rproc *rproc;
+	const struct q6_rproc_res *desc;
 	int ret;
 
-	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops,
-			    MBA_FIRMWARE_NAME, sizeof(*qproc));
+	desc = of_device_get_match_data(&pdev->dev);
+	if (!desc)
+		return -EINVAL;
+
+	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6_ops,
+			    desc->q6_mba_image, sizeof(*qproc));
 	if (!rproc) {
 		dev_err(&pdev->dev, "failed to allocate rproc\n");
 		return -ENOMEM;
 	}
 
-	rproc->fw_ops = &q6v5_fw_ops;
+	rproc->fw_ops = &q6_fw_ops;
 
 	qproc = (struct q6v5 *)rproc->priv;
 	qproc->dev = &pdev->dev;
@@ -826,6 +869,7 @@ static int q6v5_probe(struct platform_device *pdev)
 	init_completion(&qproc->start_done);
 	init_completion(&qproc->stop_done);
 
+	qproc->q6_rproc_res = desc;
 	ret = q6v5_init_mem(qproc, pdev);
 	if (ret)
 		goto free_rproc;
@@ -842,7 +886,7 @@ static int q6v5_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
-	ret = q6v5_init_reset(qproc);
+	ret = qproc->q6_rproc_res->q6_reset_init(qproc, pdev);
 	if (ret)
 		goto free_rproc;
 
@@ -873,14 +917,12 @@ static int q6v5_probe(struct platform_device *pdev)
 		goto free_rproc;
 
 	return 0;
-
 free_rproc:
 	rproc_free(rproc);
-
 	return ret;
 }
 
-static int q6v5_remove(struct platform_device *pdev)
+static int q6_remove(struct platform_device *pdev)
 {
 	struct q6v5 *qproc = platform_get_drvdata(pdev);
 
@@ -890,20 +932,80 @@ static int q6v5_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id q6v5_of_match[] = {
-	{ .compatible = "qcom,q6v5-pil", },
+char *proxy_8x96_reg_str[] = {"mx", "cx", "vdd_pll"};
+int  proxy_8x96_reg_action[3][2] = { {0, 1}, {1, 1}, {1, 0} };
+int  proxy_8x96_reg_load[] = {0, 100000, 100000};
+int  proxy_8x96_reg_min_voltage[] = {1050000, 1250000, 0};
+char *proxy_8x96_clk_str[] = {"xo", "pnoc", "qdss"};
+char *active_8x96_clk_str[] = {"iface", "bus", "mem", "gpll0_mss_clk",
+		"snoc_axi_clk", "mnoc_axi_clk"};
+
+static const struct q6_rproc_res msm_8996_res = {
+	.proxy_clks = proxy_8x96_clk_str,
+	.proxy_clk_cnt = 3,
+	.active_clks = active_8x96_clk_str,
+	.active_clk_cnt = 6,
+	.proxy_regs = proxy_8x96_reg_str,
+	.active_regs = NULL,
+	.proxy_reg_action = (int **)proxy_8x96_reg_action,
+	.proxy_reg_load = (int *)proxy_8x96_reg_load,
+	.active_reg_action = NULL,
+	.active_reg_load = NULL,
+	.proxy_reg_voltage = (int *)proxy_8x96_reg_min_voltage,
+	.active_reg_voltage = NULL,
+	.proxy_reg_cnt = 3,
+	.active_reg_cnt = 0,
+	.q6_reset_init = q6v56_init_reset,
+	.q6_version = "v56",
+	.q6_mba_image = "mba.mbn",
+};
+
+char *proxy_8x16_reg_str[] = {"mx", "cx", "pll"};
+char *active_8x16_reg_str[] = {"mss"};
+int  proxy_8x16_reg_action[4][2] = { {0, 1}, {1, 0}, {1, 0} };
+int  active_8x16_reg_action[1][2] = { {1, 1} };
+int  proxy_8x16_reg_load[] = {100000, 0, 100000, 100000};
+int  active_8x16_reg_load[] = {100000};
+int  proxy_8x16_reg_min_voltage[] = {1050000, 0, 0};
+int  active_8x16_reg_min_voltage[] = {1000000};
+char *proxy_8x16_clk_str[] = {"xo"};
+char *active_8x16_clk_str[] = {"iface", "bus", "mem"};
+
+static const struct q6_rproc_res msm_8916_res = {
+	.proxy_clks = proxy_8x16_clk_str,
+	.proxy_clk_cnt = 1,
+	.active_clks = active_8x16_clk_str,
+	.active_clk_cnt = 3,
+	.proxy_regs = proxy_8x16_reg_str,
+	.active_regs = active_8x16_reg_str,
+	.proxy_reg_action = (int **)proxy_8x16_reg_action,
+	.proxy_reg_load = (int *)proxy_8x16_reg_load,
+	.active_reg_action = (int **)active_8x16_reg_action,
+	.active_reg_load = (int *)active_8x16_reg_load,
+	.proxy_reg_voltage = (int *)proxy_8x16_reg_min_voltage,
+	.active_reg_voltage = active_8x16_reg_min_voltage,
+	.proxy_reg_cnt = 3,
+	.active_reg_cnt = 1,
+	.q6_reset_init = q6v5_init_reset,
+	.q6_version = "v5",
+	.q6_mba_image = "mba.b00",
+};
+
+static const struct of_device_id q6_of_match[] = {
+	{ .compatible = "qcom,q6v5-pil", .data = &msm_8916_res},
+	{ .compatible = "qcom,q6v56-pil", .data = &msm_8996_res},
 	{ },
 };
 
-static struct platform_driver q6v5_driver = {
-	.probe = q6v5_probe,
-	.remove = q6v5_remove,
+static struct platform_driver q6_driver = {
+	.probe = q6_probe,
+	.remove = q6_remove,
 	.driver = {
 		.name = "qcom-q6v5-pil",
-		.of_match_table = q6v5_of_match,
+		.of_match_table = q6_of_match,
 	},
 };
-module_platform_driver(q6v5_driver);
+module_platform_driver(q6_driver);
 
 MODULE_DESCRIPTION("Peripheral Image Loader for Hexagon");
 MODULE_LICENSE("GPL v2");
-- 
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 v3 2/3] remoteproc: qcom: Hexagon resource handling

[Avaneesh Kumar Dwivedi]

Handling of clock and regulator resources as well as reset
register programing differ according to version of hexagon
dsp hardware. Different version require different resources
and different parameters for same resource. Hence it is
needed that resource handling is generic and independent of
hexagon dsp version.

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

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index b60dff3..1fc505b 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/mutex.h>
 #include <linux/of_device.h>
 
 #include "remoteproc_internal.h"
@@ -93,6 +94,8 @@
 #define QDSS_BHS_ON			BIT(21)
 #define QDSS_LDO_BYP			BIT(22)
 
+#define QDSP6v56_CLAMP_WL               BIT(21)
+#define QDSP6v56_CLAMP_QMC_MEM          BIT(22)
 struct q6_rproc_res {
 	char **proxy_clks;
 	int proxy_clk_cnt;
@@ -129,11 +132,11 @@ struct q6v5 {
 	struct qcom_smem_state *state;
 	unsigned stop_bit;
 
-	struct regulator_bulk_data supply[4];
 	const struct q6_rproc_res *q6_rproc_res;
-	struct clk *ahb_clk;
-	struct clk *axi_clk;
-	struct clk *rom_clk;
+	struct clk **active_clks;
+	struct clk **proxy_clks;
+	struct regulator **proxy_regs;
+	struct regulator **active_regs;
 
 	struct completion start_done;
 	struct completion stop_done;
@@ -147,67 +150,245 @@ struct q6v5 {
 	phys_addr_t mpss_reloc;
 	void *mpss_region;
 	size_t mpss_size;
+	struct mutex q6_lock;
+	bool proxy_unvote_reg;
+	bool proxy_unvote_clk;
 };
 
-enum {
-	Q6V5_SUPPLY_CX,
-	Q6V5_SUPPLY_MX,
-	Q6V5_SUPPLY_MSS,
-	Q6V5_SUPPLY_PLL,
-};
+static int q6_regulator_init(struct q6v5 *qproc)
+{
+	struct regulator **reg_arr;
+	int i;
+
+	if (qproc->q6_rproc_res->proxy_reg_cnt) {
+		reg_arr = devm_kzalloc(qproc->dev,
+		sizeof(reg_arr) * qproc->q6_rproc_res->proxy_reg_cnt,
+		GFP_KERNEL);
+
+		for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
+			reg_arr[i] = devm_regulator_get(qproc->dev,
+			qproc->q6_rproc_res->proxy_regs[i]);
+			if (IS_ERR(reg_arr[i]))
+				return PTR_ERR(reg_arr[i]);
+			qproc->proxy_regs = reg_arr;
+		}
+	}
+
+	if (qproc->q6_rproc_res->active_reg_cnt) {
+		reg_arr = devm_kzalloc(qproc->dev,
+		sizeof(reg_arr) * qproc->q6_rproc_res->active_reg_cnt,
+		GFP_KERNEL);
+
+		for (i = 0; i < qproc->q6_rproc_res->active_reg_cnt; i++) {
+			reg_arr[i] = devm_regulator_get(qproc->dev,
+			qproc->q6_rproc_res->active_regs[i]);
+
+			if (IS_ERR(reg_arr[i]))
+				return PTR_ERR(reg_arr[i]);
+			qproc->active_regs = reg_arr;
+		}
+	}
+
+	return 0;
+}
 
-static int q6v5_regulator_init(struct q6v5 *qproc)
+static int q6_proxy_regulator_enable(struct q6v5 *qproc)
 {
-	int ret;
+	int i, j, ret = 0;
+	int **reg_loadnvoltsetflag;
+	int *reg_load;
+	int *reg_voltage;
+
+	reg_loadnvoltsetflag = qproc->q6_rproc_res->proxy_reg_action;
+	reg_load = qproc->q6_rproc_res->proxy_reg_load;
+	reg_voltage = qproc->q6_rproc_res->proxy_reg_voltage;
+
+	for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
+		for (j = 0; j <= 1; j++) {
+			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
+				regulator_set_load(qproc->proxy_regs[i],
+				reg_load[i]);
+			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
+				regulator_set_voltage(qproc->proxy_regs[i],
+				reg_voltage[i], INT_MAX);
+		}
+	}
 
-	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";
+	for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
+		ret = regulator_enable(qproc->proxy_regs[i]);
+		if (ret) {
+			for (; i > 0; --i) {
+				regulator_disable(qproc->proxy_regs[i]);
+				return ret;
+			}
+		}
+	}
 
-	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;
+	qproc->proxy_unvote_reg = true;
+
+	return 0;
+}
+
+static int q6_active_regulator_enable(struct q6v5 *qproc)
+{
+	int i, j, ret = 0;
+	int **reg_loadnvoltsetflag;
+	int *reg_load;
+	int *reg_voltage;
+
+	reg_loadnvoltsetflag = qproc->q6_rproc_res->active_reg_action;
+	reg_load = qproc->q6_rproc_res->active_reg_load;
+	reg_voltage = qproc->q6_rproc_res->active_reg_voltage;
+
+	for (i = 0; i < qproc->q6_rproc_res->active_reg_cnt; i++) {
+		for (j = 0; j <= 1; j++) {
+			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
+				regulator_set_load(qproc->active_regs[i],
+				reg_load[i]);
+			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
+				regulator_set_voltage(qproc->active_regs[i],
+				reg_voltage[i], INT_MAX);
+		}
 	}
 
-	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);
+	for (i = 0; i < qproc->q6_rproc_res->active_reg_cnt; i++) {
+		ret = regulator_enable(qproc->active_regs[i]);
+		if (ret) {
+			for (; i > 0; --i) {
+				regulator_disable(qproc->active_regs[i]);
+				return ret;
+			}
+		}
+	}
 
 	return 0;
 }
 
-static int q6v5_regulator_enable(struct q6v5 *qproc)
+static int q6_regulator_enable(struct q6v5 *qproc)
 {
-	struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer;
-	struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer;
 	int ret;
 
-	/* TODO: Q6V5_SUPPLY_CX is supposed to be set to super-turbo here */
+	if (qproc->q6_rproc_res->proxy_reg_cnt)
+		ret = q6_proxy_regulator_enable(qproc);
 
-	ret = regulator_set_voltage(mx, 1050000, INT_MAX);
-	if (ret)
-		return ret;
+	if (qproc->q6_rproc_res->active_reg_cnt)
+		ret = q6_active_regulator_enable(qproc);
 
-	regulator_set_voltage(mss, 1000000, 1150000);
+	return ret;
+}
 
-	return regulator_bulk_enable(ARRAY_SIZE(qproc->supply), qproc->supply);
+static int q6_proxy_regulator_disable(struct q6v5 *qproc)
+{
+	int i, j;
+	int **reg_loadnvoltsetflag;
+
+	reg_loadnvoltsetflag = qproc->q6_rproc_res->proxy_reg_action;
+	if (!qproc->proxy_unvote_reg)
+		return 0;
+	for (i = qproc->q6_rproc_res->proxy_reg_cnt-1; i >= 0; i--) {
+		for (j = 0; j <= 1; j++) {
+			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
+				regulator_set_load(qproc->proxy_regs[i], 0);
+			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
+				regulator_set_voltage(qproc->proxy_regs[i],
+				0, INT_MAX);
+		}
+	}
+	for (i = qproc->q6_rproc_res->proxy_reg_cnt-1; i >= 0; i--)
+		regulator_disable(qproc->proxy_regs[i]);
+	qproc->proxy_unvote_reg = false;
+	return 0;
 }
 
-static void q6v5_regulator_disable(struct q6v5 *qproc)
+static int q6_active_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, j, ret = 0;
+	int **reg_loadnvoltsetflag;
+
+	reg_loadnvoltsetflag = qproc->q6_rproc_res->active_reg_action;
+
+	for (i = qproc->q6_rproc_res->active_reg_cnt-1; i > 0; i--) {
+		for (j = 0; j <= 1; j++) {
+			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
+				regulator_set_load(qproc->active_regs[i], 0);
+			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
+				regulator_set_voltage(qproc->active_regs[i],
+				0, INT_MAX);
+		}
+	}
+	for (i = qproc->q6_rproc_res->active_reg_cnt-1; i >= 0; i--)
+		ret = regulator_disable(qproc->proxy_regs[i]);
+	return 0;
+}
+
+static void q6_regulator_disable(struct q6v5 *qproc)
+{
+	if (qproc->q6_rproc_res->proxy_reg_cnt)
+		q6_proxy_regulator_disable(qproc);
 
-	/* TODO: Q6V5_SUPPLY_CX corner votes should be released */
+	if (qproc->q6_rproc_res->active_reg_cnt)
+		q6_active_regulator_disable(qproc);
+}
 
-	regulator_bulk_disable(ARRAY_SIZE(qproc->supply), qproc->supply);
-	regulator_set_voltage(mx, 0, INT_MAX);
-	regulator_set_voltage(mss, 0, 1150000);
+static int q6_proxy_clk_enable(struct q6v5 *qproc)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < qproc->q6_rproc_res->proxy_clk_cnt; i++) {
+		ret = clk_prepare_enable(qproc->proxy_clks[i]);
+		if (ret) {
+			for (; i > 0; --i) {
+				clk_disable_unprepare(qproc->proxy_clks[i]);
+				return ret;
+			}
+		}
+	}
+	qproc->proxy_unvote_clk = true;
+	return 0;
 }
 
+static void q6_proxy_clk_disable(struct q6v5 *qproc)
+{
+	int i;
+
+	if (!qproc->proxy_unvote_clk)
+		return;
+	for (i = qproc->q6_rproc_res->proxy_clk_cnt-1; i >= 0; i--)
+		clk_disable_unprepare(qproc->proxy_clks[i]);
+	qproc->proxy_unvote_clk = false;
+}
+
+static int q6_active_clk_enable(struct q6v5 *qproc)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < qproc->q6_rproc_res->active_clk_cnt; i++) {
+		ret = clk_prepare_enable(qproc->active_clks[i]);
+		if (ret) {
+			for (; i > 0; i--) {
+				clk_disable_unprepare(qproc->active_clks[i]);
+				return ret;
+			}
+		}
+	}
+	return 0;
+}
+
+static void q6_active_clk_disable(struct q6v5 *qproc)
+{
+	int i;
+
+	for (i = qproc->q6_rproc_res->active_clk_cnt-1; i >= 0; i--)
+		clk_disable_unprepare(qproc->active_clks[i]);
+}
+
+static void pil_mss_restart_reg(struct q6v5 *qproc, u32 mss_restart)
+{
+	if (qproc->restart_reg) {
+		writel_relaxed(mss_restart, qproc->restart_reg);
+		udelay(2);
+	}
+}
 static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
 {
 	struct q6v5 *qproc = rproc->priv;
@@ -340,11 +521,6 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
 	unsigned int val;
 	int ret;
 
-	/* Check if we're already idle */
-	ret = regmap_read(halt_map, offset + AXI_IDLE_REG, &val);
-	if (!ret && val)
-		return;
-
 	/* Assert halt request */
 	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 1);
 
@@ -366,7 +542,7 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
 	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 0);
 }
 
-static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
+static int q6_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
 {
 	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
 	dma_addr_t phys;
@@ -395,7 +571,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
 	return ret < 0 ? ret : 0;
 }
 
-static int q6v5_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
+static int q6_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
 {
 	const struct elf32_phdr *phdrs;
 	const struct elf32_phdr *phdr;
@@ -451,7 +627,7 @@ static int q6v5_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
 	return ret < 0 ? ret : 0;
 }
 
-static int q6v5_mpss_load(struct q6v5 *qproc)
+static int q6_mpss_load(struct q6v5 *qproc)
 {
 	const struct firmware *fw;
 	phys_addr_t fw_addr;
@@ -476,7 +652,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 	/* Initialize the RMB validator */
 	writel(0, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
 
-	ret = q6v5_mpss_init_image(qproc, fw);
+	ret = q6_mpss_init_image(qproc, fw);
 	if (ret)
 		goto release_firmware;
 
@@ -484,7 +660,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 	if (ret)
 		goto release_firmware;
 
-	ret = q6v5_mpss_validate(qproc, fw);
+	ret = q6_mpss_validate(qproc, fw);
 
 release_firmware:
 	release_firmware(fw);
@@ -492,36 +668,41 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 	return ret < 0 ? ret : 0;
 }
 
-static int q6v5_start(struct rproc *rproc)
+static int q6_start(struct rproc *rproc)
 {
 	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
 	int ret;
 
-	ret = q6v5_regulator_enable(qproc);
+	mutex_lock(&qproc->q6_lock);
+	ret = q6_regulator_enable(qproc);
 	if (ret) {
-		dev_err(qproc->dev, "failed to enable supplies\n");
+		dev_err(qproc->dev, "failed to enable reg supplies\n");
 		return ret;
 	}
 
-	ret = reset_control_deassert(qproc->mss_restart);
+	ret = q6_proxy_clk_enable(qproc);
 	if (ret) {
-		dev_err(qproc->dev, "failed to deassert mss restart\n");
-		goto disable_vdd;
+		dev_err(qproc->dev, "failed to enable proxy_clk\n");
+		goto err_proxy_clk;
 	}
 
-	ret = clk_prepare_enable(qproc->ahb_clk);
-	if (ret)
-		goto assert_reset;
-
-	ret = clk_prepare_enable(qproc->axi_clk);
-	if (ret)
-		goto disable_ahb_clk;
+	ret = q6_active_clk_enable(qproc);
+	if (ret) {
+		dev_err(qproc->dev, "failed to enable active clocks\n");
+		goto err_active_clks;
+	}
 
-	ret = clk_prepare_enable(qproc->rom_clk);
-	if (ret)
-		goto disable_axi_clk;
+	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56"))
+		pil_mss_restart_reg(qproc, 0);
+	else {
+		ret = reset_control_deassert(qproc->mss_restart);
+		if (ret) {
+			dev_err(qproc->dev, "failed to deassert mss restart\n");
+			goto err_deassert;
+		}
+	}
 
-	writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
+	writel_relaxed(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
 
 	ret = q6v5proc_reset(qproc);
 	if (ret)
@@ -539,13 +720,11 @@ static int q6v5_start(struct rproc *rproc)
 	}
 
 	dev_info(qproc->dev, "MBA booted, loading mpss\n");
-
-	ret = q6v5_mpss_load(qproc);
+	ret = q6_mpss_load(qproc);
 	if (ret)
 		goto halt_axi_ports;
-
 	ret = wait_for_completion_timeout(&qproc->start_done,
-					  msecs_to_jiffies(5000));
+					  msecs_to_jiffies(10000));
 	if (ret == 0) {
 		dev_err(qproc->dev, "start timed out\n");
 		ret = -ETIMEDOUT;
@@ -553,36 +732,33 @@ static int q6v5_start(struct rproc *rproc)
 	}
 
 	qproc->running = true;
-
 	/* TODO: All done, release the handover resources */
-
+	q6_proxy_clk_disable(qproc);
+	q6_proxy_regulator_disable(qproc);
+	mutex_unlock(&qproc->q6_lock);
 	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);
-assert_reset:
-	reset_control_assert(qproc->mss_restart);
-disable_vdd:
-	q6v5_regulator_disable(qproc);
-
+err_deassert:
+	q6_active_clk_disable(qproc);
+err_active_clks:
+	q6_proxy_clk_disable(qproc);
+err_proxy_clk:
+	q6_regulator_disable(qproc);
+	mutex_unlock(&qproc->q6_lock);
 	return ret;
 }
 
-static int q6v5_stop(struct rproc *rproc)
+static int q6_stop(struct rproc *rproc)
 {
 	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
 	int ret;
+	u64 val;
 
-	qproc->running = false;
-
+	mutex_lock(&qproc->q6_lock);
 	qcom_smem_state_update_bits(qproc->state,
 				    BIT(qproc->stop_bit), BIT(qproc->stop_bit));
 
@@ -597,16 +773,30 @@ 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);
 
-	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_regulator_disable(qproc);
-
+	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56")) {
+		/*
+		 * 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.
+		 */
+
+		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);
+		pil_mss_restart_reg(qproc, 1);
+	} else
+		reset_control_assert(qproc->mss_restart);
+	q6_active_clk_disable(qproc);
+	q6_proxy_clk_disable(qproc);
+	q6_proxy_regulator_disable(qproc);
+	q6_active_regulator_disable(qproc);
+	qproc->running = false;
+	mutex_unlock(&qproc->q6_lock);
 	return 0;
 }
 
-static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len)
+static void *q6_da_to_va(struct rproc *rproc, u64 da, int len)
 {
 	struct q6v5 *qproc = rproc->priv;
 	int offset;
@@ -619,12 +809,12 @@ static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len)
 }
 
 static const struct rproc_ops q6_ops = {
-	.start = q6v5_start,
-	.stop = q6v5_stop,
-	.da_to_va = q6v5_da_to_va,
+	.start = q6_start,
+	.stop = q6_stop,
+	.da_to_va = q6_da_to_va,
 };
 
-static irqreturn_t q6v5_wdog_interrupt(int irq, void *dev)
+static irqreturn_t q6_wdog_interrupt(int irq, void *dev)
 {
 	struct q6v5 *qproc = dev;
 	size_t len;
@@ -650,7 +840,7 @@ static irqreturn_t q6v5_wdog_interrupt(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t q6v5_fatal_interrupt(int irq, void *dev)
+static irqreturn_t q6_fatal_interrupt(int irq, void *dev)
 {
 	struct q6v5 *qproc = dev;
 	size_t len;
@@ -670,7 +860,7 @@ static irqreturn_t q6v5_fatal_interrupt(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t q6v5_handover_interrupt(int irq, void *dev)
+static irqreturn_t q6_handover_interrupt(int irq, void *dev)
 {
 	struct q6v5 *qproc = dev;
 
@@ -678,7 +868,7 @@ static irqreturn_t q6v5_handover_interrupt(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t q6v5_stop_ack_interrupt(int irq, void *dev)
+static irqreturn_t q6_stop_ack_interrupt(int irq, void *dev)
 {
 	struct q6v5 *qproc = dev;
 
@@ -686,7 +876,7 @@ static irqreturn_t q6v5_stop_ack_interrupt(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
-static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev)
+static int q6_init_mem(struct q6v5 *qproc, struct platform_device *pdev)
 {
 	struct of_phandle_args args;
 	struct resource *res;
@@ -721,24 +911,45 @@ static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev)
 	return 0;
 }
 
-static int q6v5_init_clocks(struct q6v5 *qproc)
+static int q6_init_clocks(struct q6v5 *qproc)
 {
-	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);
-	}
+	struct clk **clk_arr;
+	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 (qproc->q6_rproc_res->proxy_clk_cnt) {
+		clk_arr = devm_kzalloc(qproc->dev,
+		sizeof(clk_arr) * qproc->q6_rproc_res->proxy_clk_cnt,
+		GFP_KERNEL);
+
+		for (i = 0; i < qproc->q6_rproc_res->proxy_clk_cnt; i++) {
+			clk_arr[i] = devm_clk_get(qproc->dev,
+			qproc->q6_rproc_res->proxy_clks[i]);
+
+			if (IS_ERR(clk_arr[i])) {
+				dev_err(qproc->dev, "failed to get %s clock\n",
+				qproc->q6_rproc_res->proxy_clks[i]);
+				return PTR_ERR(clk_arr[i]);
+			}
+			qproc->proxy_clks = clk_arr;
+		}
 	}
 
-	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);
+	if (qproc->q6_rproc_res->active_clk_cnt) {
+		clk_arr = devm_kzalloc(qproc->dev,
+		sizeof(clk_arr) * qproc->q6_rproc_res->proxy_clk_cnt,
+		GFP_KERNEL);
+
+		for (i = 0; i < qproc->q6_rproc_res->active_clk_cnt; i++) {
+			clk_arr[i] = devm_clk_get(qproc->dev,
+			qproc->q6_rproc_res->active_clks[i]);
+			if (IS_ERR(clk_arr[i])) {
+				dev_err(qproc->dev, "failed to get %s clock\n",
+				qproc->q6_rproc_res->active_clks[i]);
+				return PTR_ERR(clk_arr[i]);
+			}
+
+			qproc->active_clks = clk_arr;
+		}
 	}
 
 	return 0;
@@ -774,7 +985,8 @@ static int q6v56_init_reset(void *q, void *p)
 
 	return 0;
 }
-static int q6v5_request_irq(struct q6v5 *qproc,
+
+static int q6_request_irq(struct q6v5 *qproc,
 			     struct platform_device *pdev,
 			     const char *name,
 			     irq_handler_t thread_fn)
@@ -797,7 +1009,7 @@ static int q6v5_request_irq(struct q6v5 *qproc,
 	return ret;
 }
 
-static int q6v5_alloc_memory_region(struct q6v5 *qproc)
+static int q6_alloc_memory_region(struct q6v5 *qproc)
 {
 	struct device_node *child;
 	struct device_node *node;
@@ -870,39 +1082,42 @@ static int q6_probe(struct platform_device *pdev)
 	init_completion(&qproc->stop_done);
 
 	qproc->q6_rproc_res = desc;
-	ret = q6v5_init_mem(qproc, pdev);
+
+	ret = q6_init_mem(qproc, pdev);
 	if (ret)
 		goto free_rproc;
 
-	ret = q6v5_alloc_memory_region(qproc);
+	ret = q6_alloc_memory_region(qproc);
 	if (ret)
 		goto free_rproc;
 
-	ret = q6v5_init_clocks(qproc);
+	ret = q6_init_clocks(qproc);
 	if (ret)
 		goto free_rproc;
 
-	ret = q6v5_regulator_init(qproc);
+	ret = qproc->q6_rproc_res->q6_reset_init(qproc, pdev);
 	if (ret)
 		goto free_rproc;
 
-	ret = qproc->q6_rproc_res->q6_reset_init(qproc, pdev);
+	ret = q6_regulator_init(qproc);
 	if (ret)
 		goto free_rproc;
 
-	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
+	mutex_init(&qproc->q6_lock);
+
+	ret = q6_request_irq(qproc, pdev, "wdog", q6_wdog_interrupt);
 	if (ret < 0)
 		goto free_rproc;
 
-	ret = q6v5_request_irq(qproc, pdev, "fatal", q6v5_fatal_interrupt);
+	ret = q6_request_irq(qproc, pdev, "fatal", q6_fatal_interrupt);
 	if (ret < 0)
 		goto free_rproc;
 
-	ret = q6v5_request_irq(qproc, pdev, "handover", q6v5_handover_interrupt);
+	ret = q6_request_irq(qproc, pdev, "handover", q6_handover_interrupt);
 	if (ret < 0)
 		goto free_rproc;
 
-	ret = q6v5_request_irq(qproc, pdev, "stop-ack", q6v5_stop_ack_interrupt);
+	ret = q6_request_irq(qproc, pdev, "stop-ack", q6_stop_ack_interrupt);
 	if (ret < 0)
 		goto free_rproc;
 
@@ -917,8 +1132,10 @@ static int q6_probe(struct platform_device *pdev)
 		goto free_rproc;
 
 	return 0;
+
 free_rproc:
 	rproc_free(rproc);
+
 	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.

[PATCH v3 3/3] remoteproc: qcom: Adding q6v56 reset sequence.

[Avaneesh Kumar Dwivedi]

Adding additional reset sequence steps required specific
to q6v56 based on version check, along with some trivial
changes in name of local functions.

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

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 1fc505b..68eda4f 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -66,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
@@ -94,8 +96,14 @@
 #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 q6_rproc_res {
 	char **proxy_clks;
 	int proxy_clk_cnt;
@@ -389,7 +397,8 @@ static void pil_mss_restart_reg(struct q6v5 *qproc, u32 mss_restart)
 		udelay(2);
 	}
 }
-static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
+
+static int q6_load(struct rproc *rproc, const struct firmware *fw)
 {
 	struct q6v5 *qproc = rproc->priv;
 
@@ -400,10 +409,10 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
 
 static const struct rproc_fw_ops q6_fw_ops = {
 	.find_rsc_table = qcom_mdt_find_rsc_table,
-	.load = q6v5_load,
+	.load = q6_load,
 };
 
-static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms)
+static int q6_rmb_pbl_wait(struct q6v5 *qproc, int ms)
 {
 	unsigned long timeout;
 	s32 val;
@@ -423,7 +432,7 @@ static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms)
 	return val;
 }
 
-static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
+static int q6_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
 {
 
 	unsigned long timeout;
@@ -449,40 +458,95 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
 	return val;
 }
 
-static int q6v5proc_reset(struct q6v5 *qproc)
+static int q6proc_reset(struct q6v5 *qproc)
 {
-	u32 val;
-	int ret;
+	int ret, i, count;
+	u64 val;
+
+	/* Override the ACC value if required */
+	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56"))
+		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 = readl_relaxed(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);
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_RESET_REG);
 
+	/* BHS require xo cbcr to be enabled */
+	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56")) {
+		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");
+	}
 	/* 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_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	val |= QDSP6v56_BHS_ON;
+	writel_relaxed(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);
+	/* Put LDO in bypass mode */
+	val |= QDSP6v56_LDO_BYP;
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56")) {
+		/*
+		 * 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);
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
 
 	/* Bring core out of reset */
 	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
@@ -490,9 +554,9 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
 
 	/* Turn on core clock */
-	val = readl(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
+	val = readl_relaxed(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
 	val |= Q6SS_CLK_ENABLE;
-	writel(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
 
 	/* Start core execution */
 	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
@@ -500,7 +564,7 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
 
 	/* Wait for PBL status */
-	ret = q6v5_rmb_pbl_wait(qproc, 1000);
+	ret = q6_rmb_pbl_wait(qproc, 1000);
 	if (ret == -ETIMEDOUT) {
 		dev_err(qproc->dev, "PBL boot timed out\n");
 	} else if (ret != RMB_PBL_SUCCESS) {
@@ -560,7 +624,7 @@ static int q6_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
 	writel(phys, qproc->rmb_base + RMB_PMI_META_DATA_REG);
 	writel(RMB_CMD_META_DATA_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
 
-	ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
+	ret = q6_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
 	if (ret == -ETIMEDOUT)
 		dev_err(qproc->dev, "MPSS header authentication timed out\n");
 	else if (ret < 0)
@@ -618,7 +682,7 @@ static int q6_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
 		writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
 	}
 
-	ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000);
+	ret = q6_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000);
 	if (ret == -ETIMEDOUT)
 		dev_err(qproc->dev, "MPSS authentication timed out\n");
 	else if (ret < 0)
@@ -704,11 +768,11 @@ static int q6_start(struct rproc *rproc)
 
 	writel_relaxed(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
 
-	ret = q6v5proc_reset(qproc);
+	ret = q6proc_reset(qproc);
 	if (ret)
 		goto halt_axi_ports;
 
-	ret = q6v5_rmb_mba_wait(qproc, 0, 5000);
+	ret = q6_rmb_mba_wait(qproc, 0, 5000);
 	if (ret == -ETIMEDOUT) {
 		dev_err(qproc->dev, "MBA boot timed out\n");
 		goto halt_axi_ports;
-- 
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 v3 2/3] remoteproc: qcom: Hexagon resource handling

[Bjorn Andersson]

On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote:

> Handling of clock and regulator resources as well as reset
> register programing differ according to version of hexagon
> dsp hardware. Different version require different resources
> and different parameters for same resource. Hence it is
> needed that resource handling is generic and independent of
> hexagon dsp version.
> 

It would be much easier to review this if you didn't do all three
changes in the same patch, and at the same time changed the function
names. There's large parts of this patch where it's not obvious what the
actual changes are.

> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 471 +++++++++++++++++++++++++++----------
>  1 file changed, 344 insertions(+), 127 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index b60dff3..1fc505b 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/mutex.h>
>  #include <linux/of_device.h>
>  
>  #include "remoteproc_internal.h"
> @@ -93,6 +94,8 @@
>  #define QDSS_BHS_ON			BIT(21)
>  #define QDSS_LDO_BYP			BIT(22)
>  
> +#define QDSP6v56_CLAMP_WL               BIT(21)
> +#define QDSP6v56_CLAMP_QMC_MEM          BIT(22)
>  struct q6_rproc_res {
>  	char **proxy_clks;
>  	int proxy_clk_cnt;
> @@ -129,11 +132,11 @@ struct q6v5 {
>  	struct qcom_smem_state *state;
>  	unsigned stop_bit;
>  
> -	struct regulator_bulk_data supply[4];
>  	const struct q6_rproc_res *q6_rproc_res;
> -	struct clk *ahb_clk;
> -	struct clk *axi_clk;
> -	struct clk *rom_clk;
> +	struct clk **active_clks;
> +	struct clk **proxy_clks;
> +	struct regulator **proxy_regs;
> +	struct regulator **active_regs;

Keeping these as statically sized arrays, potentially with unused
elements at the end removes the need for allocating the storage and the
double pointers.

>  
>  	struct completion start_done;
>  	struct completion stop_done;
> @@ -147,67 +150,245 @@ struct q6v5 {
>  	phys_addr_t mpss_reloc;
>  	void *mpss_region;
>  	size_t mpss_size;
> +	struct mutex q6_lock;
> +	bool proxy_unvote_reg;
> +	bool proxy_unvote_clk;

I still don't see the need for these 3 attributes.

>  };
>  
> -enum {
> -	Q6V5_SUPPLY_CX,
> -	Q6V5_SUPPLY_MX,
> -	Q6V5_SUPPLY_MSS,
> -	Q6V5_SUPPLY_PLL,
> -};
> +static int q6_regulator_init(struct q6v5 *qproc)
> +{
> +	struct regulator **reg_arr;
> +	int i;
> +
> +	if (qproc->q6_rproc_res->proxy_reg_cnt) {

If you keep proxy_regs and active_regs as arrays you don't need this
check.

> +		reg_arr = devm_kzalloc(qproc->dev,
> +		sizeof(reg_arr) * qproc->q6_rproc_res->proxy_reg_cnt,
> +		GFP_KERNEL);
> +
> +		for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
> +			reg_arr[i] = devm_regulator_get(qproc->dev,
> +			qproc->q6_rproc_res->proxy_regs[i]);
> +			if (IS_ERR(reg_arr[i]))
> +				return PTR_ERR(reg_arr[i]);
> +			qproc->proxy_regs = reg_arr;
> +		}
> +	}
> +
> +	if (qproc->q6_rproc_res->active_reg_cnt) {
> +		reg_arr = devm_kzalloc(qproc->dev,
> +		sizeof(reg_arr) * qproc->q6_rproc_res->active_reg_cnt,
> +		GFP_KERNEL);
> +
> +		for (i = 0; i < qproc->q6_rproc_res->active_reg_cnt; i++) {
> +			reg_arr[i] = devm_regulator_get(qproc->dev,
> +			qproc->q6_rproc_res->active_regs[i]);
> +
> +			if (IS_ERR(reg_arr[i]))
> +				return PTR_ERR(reg_arr[i]);
> +			qproc->active_regs = reg_arr;
> +		}
> +	}

Please keep active_regs and proxy_regs as regulator_bulk_data and
continue to use devm_regulator_bulk_get(), it should make this code
cleaner.

> +
> +	return 0;
> +}
>  
> -static int q6v5_regulator_init(struct q6v5 *qproc)
> +static int q6_proxy_regulator_enable(struct q6v5 *qproc)
>  {
> -	int ret;
> +	int i, j, ret = 0;
> +	int **reg_loadnvoltsetflag;
> +	int *reg_load;
> +	int *reg_voltage;
> +
> +	reg_loadnvoltsetflag = qproc->q6_rproc_res->proxy_reg_action;
> +	reg_load = qproc->q6_rproc_res->proxy_reg_load;
> +	reg_voltage = qproc->q6_rproc_res->proxy_reg_voltage;

Rather then keeping these properties on int-arrays I strongly prefer
that you would have a struct { uV, uA } for each regulator.

> +
> +	for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
> +		for (j = 0; j <= 1; j++) {
> +			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))

I'm sorry, but this is not clean. Please use the fact that we're not
writing assembly code and use the language to your advantage.

> +				regulator_set_load(qproc->proxy_regs[i],
> +				reg_load[i]);
> +			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
> +				regulator_set_voltage(qproc->proxy_regs[i],
> +				reg_voltage[i], INT_MAX);
> +		}
> +	}
>  
> -	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";
> +	for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
> +		ret = regulator_enable(qproc->proxy_regs[i]);
> +		if (ret) {
> +			for (; i > 0; --i) {
> +				regulator_disable(qproc->proxy_regs[i]);
> +				return ret;
> +			}
> +		}
> +	}

If you just keep your regulators in a regulator_bulk_data array then you
can replace this with regulator_bulk_enable(proxy_reg_cnt, proxy_regs);

>  
> -	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;
> +	qproc->proxy_unvote_reg = true;

This should still not be needed.

> +
> +	return 0;
> +}
> +
> +static int q6_active_regulator_enable(struct q6v5 *qproc)
> +{
> +	int i, j, ret = 0;
> +	int **reg_loadnvoltsetflag;
> +	int *reg_load;
> +	int *reg_voltage;
> +
> +	reg_loadnvoltsetflag = qproc->q6_rproc_res->active_reg_action;
> +	reg_load = qproc->q6_rproc_res->active_reg_load;
> +	reg_voltage = qproc->q6_rproc_res->active_reg_voltage;
> +
> +	for (i = 0; i < qproc->q6_rproc_res->active_reg_cnt; i++) {
> +		for (j = 0; j <= 1; j++) {
> +			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
> +				regulator_set_load(qproc->active_regs[i],
> +				reg_load[i]);
> +			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
> +				regulator_set_voltage(qproc->active_regs[i],
> +				reg_voltage[i], INT_MAX);
> +		}
>  	}
>  
> -	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);
> +	for (i = 0; i < qproc->q6_rproc_res->active_reg_cnt; i++) {
> +		ret = regulator_enable(qproc->active_regs[i]);
> +		if (ret) {
> +			for (; i > 0; --i) {
> +				regulator_disable(qproc->active_regs[i]);
> +				return ret;
> +			}
> +		}
> +	}
>  
>  	return 0;
>  }
>  
> -static int q6v5_regulator_enable(struct q6v5 *qproc)
> +static int q6_regulator_enable(struct q6v5 *qproc)
>  {
> -	struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer;
> -	struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer;
>  	int ret;
>  
> -	/* TODO: Q6V5_SUPPLY_CX is supposed to be set to super-turbo here */
> +	if (qproc->q6_rproc_res->proxy_reg_cnt)
> +		ret = q6_proxy_regulator_enable(qproc);
>  
> -	ret = regulator_set_voltage(mx, 1050000, INT_MAX);
> -	if (ret)
> -		return ret;
> +	if (qproc->q6_rproc_res->active_reg_cnt)

q6_active_regulator_enable() is a no-op if active_reg_cnt is 0, so no
need to check that. Rather than having two functions, try to
parameterize the regulator enable functions so that you can have a
single function that you pass the active or proxy list.

> +		ret = q6_active_regulator_enable(qproc);
>  
> -	regulator_set_voltage(mss, 1000000, 1150000);
> +	return ret;
> +}
>  
> -	return regulator_bulk_enable(ARRAY_SIZE(qproc->supply), qproc->supply);
> +static int q6_proxy_regulator_disable(struct q6v5 *qproc)
> +{
> +	int i, j;
> +	int **reg_loadnvoltsetflag;
> +
> +	reg_loadnvoltsetflag = qproc->q6_rproc_res->proxy_reg_action;
> +	if (!qproc->proxy_unvote_reg)
> +		return 0;
> +	for (i = qproc->q6_rproc_res->proxy_reg_cnt-1; i >= 0; i--) {
> +		for (j = 0; j <= 1; j++) {
> +			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
> +				regulator_set_load(qproc->proxy_regs[i], 0);
> +			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
> +				regulator_set_voltage(qproc->proxy_regs[i],
> +				0, INT_MAX);
> +		}
> +	}
> +	for (i = qproc->q6_rproc_res->proxy_reg_cnt-1; i >= 0; i--)
> +		regulator_disable(qproc->proxy_regs[i]);
> +	qproc->proxy_unvote_reg = false;
> +	return 0;
>  }
>  
> -static void q6v5_regulator_disable(struct q6v5 *qproc)
> +static int q6_active_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, j, ret = 0;
> +	int **reg_loadnvoltsetflag;
> +
> +	reg_loadnvoltsetflag = qproc->q6_rproc_res->active_reg_action;
> +
> +	for (i = qproc->q6_rproc_res->active_reg_cnt-1; i > 0; i--) {
> +		for (j = 0; j <= 1; j++) {
> +			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
> +				regulator_set_load(qproc->active_regs[i], 0);
> +			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
> +				regulator_set_voltage(qproc->active_regs[i],
> +				0, INT_MAX);
> +		}
> +	}
> +	for (i = qproc->q6_rproc_res->active_reg_cnt-1; i >= 0; i--)
> +		ret = regulator_disable(qproc->proxy_regs[i]);
> +	return 0;
> +}
> +
> +static void q6_regulator_disable(struct q6v5 *qproc)
> +{
> +	if (qproc->q6_rproc_res->proxy_reg_cnt)
> +		q6_proxy_regulator_disable(qproc);
>  
> -	/* TODO: Q6V5_SUPPLY_CX corner votes should be released */
> +	if (qproc->q6_rproc_res->active_reg_cnt)
> +		q6_active_regulator_disable(qproc);
> +}
>  
> -	regulator_bulk_disable(ARRAY_SIZE(qproc->supply), qproc->supply);
> -	regulator_set_voltage(mx, 0, INT_MAX);
> -	regulator_set_voltage(mss, 0, 1150000);
> +static int q6_proxy_clk_enable(struct q6v5 *qproc)

This is really the same as active_clk_enable(), so you should just have
a function that you pass an array of clocks and a count to - similar to
regulator_bulk_enable().

> +{
> +	int i, ret = 0;
> +
> +	for (i = 0; i < qproc->q6_rproc_res->proxy_clk_cnt; i++) {
> +		ret = clk_prepare_enable(qproc->proxy_clks[i]);
> +		if (ret) {
> +			for (; i > 0; --i) {
> +				clk_disable_unprepare(qproc->proxy_clks[i]);
> +				return ret;
> +			}
> +		}
> +	}
> +	qproc->proxy_unvote_clk = true;
> +	return 0;
>  }
>  
> +static void q6_proxy_clk_disable(struct q6v5 *qproc)
> +{
> +	int i;
> +
> +	if (!qproc->proxy_unvote_clk)
> +		return;
> +	for (i = qproc->q6_rproc_res->proxy_clk_cnt-1; i >= 0; i--)
> +		clk_disable_unprepare(qproc->proxy_clks[i]);
> +	qproc->proxy_unvote_clk = false;
> +}
> +
> +static int q6_active_clk_enable(struct q6v5 *qproc)
> +{
> +	int i, ret = 0;

No need to initialize ret, as its first use is an assignment.

> +
> +	for (i = 0; i < qproc->q6_rproc_res->active_clk_cnt; i++) {
> +		ret = clk_prepare_enable(qproc->active_clks[i]);
> +		if (ret) {

Use goto here, rather than nesting a error return in here.

> +			for (; i > 0; i--) {
> +				clk_disable_unprepare(qproc->active_clks[i]);
> +				return ret;
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +
> +static void q6_active_clk_disable(struct q6v5 *qproc)
> +{
> +	int i;
> +
> +	for (i = qproc->q6_rproc_res->active_clk_cnt-1; i >= 0; i--)
> +		clk_disable_unprepare(qproc->active_clks[i]);
> +}
> +
> +static void pil_mss_restart_reg(struct q6v5 *qproc, u32 mss_restart)
> +{
> +	if (qproc->restart_reg) {
> +		writel_relaxed(mss_restart, qproc->restart_reg);
> +		udelay(2);
> +	}
> +}
>  static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>  {
>  	struct q6v5 *qproc = rproc->priv;
> @@ -340,11 +521,6 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>  	unsigned int val;
>  	int ret;
>  
> -	/* Check if we're already idle */
> -	ret = regmap_read(halt_map, offset + AXI_IDLE_REG, &val);
> -	if (!ret && val)
> -		return;
> -

Please put this in its own commit and describe why it can't be there on
8996 and why it's okay to drop on 8974 and 8916.

>  	/* Assert halt request */
>  	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 1);
>  
> @@ -366,7 +542,7 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>  	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 0);
>  }
>  
> -static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
> +static int q6_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>  {
>  	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
>  	dma_addr_t phys;
> @@ -395,7 +571,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>  	return ret < 0 ? ret : 0;
>  }
>  
> -static int q6v5_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
> +static int q6_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
>  {
>  	const struct elf32_phdr *phdrs;
>  	const struct elf32_phdr *phdr;
> @@ -451,7 +627,7 @@ static int q6v5_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
>  	return ret < 0 ? ret : 0;
>  }
>  
> -static int q6v5_mpss_load(struct q6v5 *qproc)
> +static int q6_mpss_load(struct q6v5 *qproc)
>  {
>  	const struct firmware *fw;
>  	phys_addr_t fw_addr;
> @@ -476,7 +652,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>  	/* Initialize the RMB validator */
>  	writel(0, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>  
> -	ret = q6v5_mpss_init_image(qproc, fw);
> +	ret = q6_mpss_init_image(qproc, fw);
>  	if (ret)
>  		goto release_firmware;
>  
> @@ -484,7 +660,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>  	if (ret)
>  		goto release_firmware;
>  
> -	ret = q6v5_mpss_validate(qproc, fw);
> +	ret = q6_mpss_validate(qproc, fw);
>  
>  release_firmware:
>  	release_firmware(fw);
> @@ -492,36 +668,41 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>  	return ret < 0 ? ret : 0;
>  }
>  
> -static int q6v5_start(struct rproc *rproc)
> +static int q6_start(struct rproc *rproc)

Most of the changes in this function are renaming of functions and
variables, please don't do this as part of a functional change.

Best would be if you start with a commit that renames the necessary
parts and where you specify that there's "no functional change".

>  {
>  	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>  	int ret;
>  
> -	ret = q6v5_regulator_enable(qproc);
> +	mutex_lock(&qproc->q6_lock);

We should already be protected by the rproc->lock here, please let me
know if there are any gaps.

> +	ret = q6_regulator_enable(qproc);
>  	if (ret) {
> -		dev_err(qproc->dev, "failed to enable supplies\n");
> +		dev_err(qproc->dev, "failed to enable reg supplies\n");

Supplies are regulators, but if you find this confusing then you
shouldn't abbreviate regulators as "reg".

>  		return ret;
>  	}
>  
> -	ret = reset_control_deassert(qproc->mss_restart);

So the correct order is: enable clocks, then deassert reset?

> +	ret = q6_proxy_clk_enable(qproc);
>  	if (ret) {
> -		dev_err(qproc->dev, "failed to deassert mss restart\n");
> -		goto disable_vdd;
> +		dev_err(qproc->dev, "failed to enable proxy_clk\n");
> +		goto err_proxy_clk;
>  	}
>  
> -	ret = clk_prepare_enable(qproc->ahb_clk);
> -	if (ret)
> -		goto assert_reset;
> -
> -	ret = clk_prepare_enable(qproc->axi_clk);
> -	if (ret)
> -		goto disable_ahb_clk;
> +	ret = q6_active_clk_enable(qproc);
> +	if (ret) {
> +		dev_err(qproc->dev, "failed to enable active clocks\n");
> +		goto err_active_clks;
> +	}
>  
> -	ret = clk_prepare_enable(qproc->rom_clk);
> -	if (ret)
> -		goto disable_axi_clk;
> +	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56"))
> +		pil_mss_restart_reg(qproc, 0);
> +	else {
> +		ret = reset_control_deassert(qproc->mss_restart);
> +		if (ret) {
> +			dev_err(qproc->dev, "failed to deassert mss restart\n");
> +			goto err_deassert;
> +		}
> +	}
>  
> -	writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
> +	writel_relaxed(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);

There's no functional reason for changing writel to writel_relaxed, so
please do this in a separate commit and motivate it well.

>  
>  	ret = q6v5proc_reset(qproc);
>  	if (ret)
> @@ -539,13 +720,11 @@ static int q6v5_start(struct rproc *rproc)
>  	}
>  
>  	dev_info(qproc->dev, "MBA booted, loading mpss\n");
> -
> -	ret = q6v5_mpss_load(qproc);
> +	ret = q6_mpss_load(qproc);
>  	if (ret)
>  		goto halt_axi_ports;
> -
>  	ret = wait_for_completion_timeout(&qproc->start_done,
> -					  msecs_to_jiffies(5000));
> +					  msecs_to_jiffies(10000));

Please put this in a separate commit and describe why 10 seconds is
better than 5.

>  	if (ret == 0) {
>  		dev_err(qproc->dev, "start timed out\n");
>  		ret = -ETIMEDOUT;
> @@ -553,36 +732,33 @@ static int q6v5_start(struct rproc *rproc)
>  	}
>  
>  	qproc->running = true;
> -
>  	/* TODO: All done, release the handover resources */
> -
> +	q6_proxy_clk_disable(qproc);
> +	q6_proxy_regulator_disable(qproc);

This is good, please drop the TODO comment now that we're done.

> +	mutex_unlock(&qproc->q6_lock);
>  	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);
> -assert_reset:
> -	reset_control_assert(qproc->mss_restart);

Don't we need to assert the reset again?

> -disable_vdd:
> -	q6v5_regulator_disable(qproc);
> -
> +err_deassert:
> +	q6_active_clk_disable(qproc);
> +err_active_clks:
> +	q6_proxy_clk_disable(qproc);
> +err_proxy_clk:

It's better if the labels describe the action than the source of the
jump, so please keep the "disable_vdd" label for this - it also makes
your patch cleaner.

> +	q6_regulator_disable(qproc);
> +	mutex_unlock(&qproc->q6_lock);
>  	return ret;
>  }
>  
> -static int q6v5_stop(struct rproc *rproc)
> +static int q6_stop(struct rproc *rproc)
>  {
>  	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>  	int ret;
> +	u64 val;
>  
> -	qproc->running = false;
> -
> +	mutex_lock(&qproc->q6_lock);
>  	qcom_smem_state_update_bits(qproc->state,
>  				    BIT(qproc->stop_bit), BIT(qproc->stop_bit));
>  
> @@ -597,16 +773,30 @@ 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);
>  
> -	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_regulator_disable(qproc);
> -
> +	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56")) {

This would be much better as an enum than a string. But I keep wonder if
this is only for v5.6 of the Hexagon - perhaps should we clamp different
things on the various versions?.

> +		/*
> +		 * 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.
> +		 */
> +
> +		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);
> +		pil_mss_restart_reg(qproc, 1);

And by using the reset framework for mss_restart this will fall out of
the conditional segment and the else is gone.

> +	} else
> +		reset_control_assert(qproc->mss_restart);
> +	q6_active_clk_disable(qproc);
> +	q6_proxy_clk_disable(qproc);
> +	q6_proxy_regulator_disable(qproc);
> +	q6_active_regulator_disable(qproc);
> +	qproc->running = false;
> +	mutex_unlock(&qproc->q6_lock);
>  	return 0;
>  }
>  

Regards,
Bjorn

Re: [PATCH v3 3/3] remoteproc: qcom: Adding q6v56 reset sequence.

[Bjorn Andersson]

On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote:

> Adding additional reset sequence steps required specific
> to q6v56 based on version check, along with some trivial
> changes in name of local functions.
> 

Please don't change readl/writel to their relaxed version in the same
commit as you do functional changes. And please don't change function
names here either.

Both makes the review really hard and the actual changes non-obvious.

Regards,
Bjorn

> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 132 +++++++++++++++++++++++++++----------
>  1 file changed, 98 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 1fc505b..68eda4f 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -66,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
> @@ -94,8 +96,14 @@
>  #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 q6_rproc_res {
>  	char **proxy_clks;
>  	int proxy_clk_cnt;
> @@ -389,7 +397,8 @@ static void pil_mss_restart_reg(struct q6v5 *qproc, u32 mss_restart)
>  		udelay(2);
>  	}
>  }
> -static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
> +
> +static int q6_load(struct rproc *rproc, const struct firmware *fw)
>  {
>  	struct q6v5 *qproc = rproc->priv;
>  
> @@ -400,10 +409,10 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>  
>  static const struct rproc_fw_ops q6_fw_ops = {
>  	.find_rsc_table = qcom_mdt_find_rsc_table,
> -	.load = q6v5_load,
> +	.load = q6_load,
>  };
>  
> -static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms)
> +static int q6_rmb_pbl_wait(struct q6v5 *qproc, int ms)
>  {
>  	unsigned long timeout;
>  	s32 val;
> @@ -423,7 +432,7 @@ static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms)
>  	return val;
>  }
>  
> -static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
> +static int q6_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>  {
>  
>  	unsigned long timeout;
> @@ -449,40 +458,95 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>  	return val;
>  }
>  
> -static int q6v5proc_reset(struct q6v5 *qproc)
> +static int q6proc_reset(struct q6v5 *qproc)
>  {
> -	u32 val;
> -	int ret;
> +	int ret, i, count;
> +	u64 val;
> +
> +	/* Override the ACC value if required */
> +	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56"))
> +		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 = readl_relaxed(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);
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_RESET_REG);
>  
> +	/* BHS require xo cbcr to be enabled */
> +	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56")) {
> +		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");
> +	}
>  	/* 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_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	val |= QDSP6v56_BHS_ON;
> +	writel_relaxed(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);
> +	/* Put LDO in bypass mode */
> +	val |= QDSP6v56_LDO_BYP;
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56")) {
> +		/*
> +		 * 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);
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>  
>  	/* Bring core out of reset */
>  	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
> @@ -490,9 +554,9 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>  	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
>  
>  	/* Turn on core clock */
> -	val = readl(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
>  	val |= Q6SS_CLK_ENABLE;
> -	writel(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
>  
>  	/* Start core execution */
>  	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
> @@ -500,7 +564,7 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>  	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
>  
>  	/* Wait for PBL status */
> -	ret = q6v5_rmb_pbl_wait(qproc, 1000);
> +	ret = q6_rmb_pbl_wait(qproc, 1000);
>  	if (ret == -ETIMEDOUT) {
>  		dev_err(qproc->dev, "PBL boot timed out\n");
>  	} else if (ret != RMB_PBL_SUCCESS) {
> @@ -560,7 +624,7 @@ static int q6_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>  	writel(phys, qproc->rmb_base + RMB_PMI_META_DATA_REG);
>  	writel(RMB_CMD_META_DATA_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>  
> -	ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
> +	ret = q6_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
>  	if (ret == -ETIMEDOUT)
>  		dev_err(qproc->dev, "MPSS header authentication timed out\n");
>  	else if (ret < 0)
> @@ -618,7 +682,7 @@ static int q6_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
>  		writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>  	}
>  
> -	ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000);
> +	ret = q6_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000);
>  	if (ret == -ETIMEDOUT)
>  		dev_err(qproc->dev, "MPSS authentication timed out\n");
>  	else if (ret < 0)
> @@ -704,11 +768,11 @@ static int q6_start(struct rproc *rproc)
>  
>  	writel_relaxed(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
>  
> -	ret = q6v5proc_reset(qproc);
> +	ret = q6proc_reset(qproc);
>  	if (ret)
>  		goto halt_axi_ports;
>  
> -	ret = q6v5_rmb_mba_wait(qproc, 0, 5000);
> +	ret = q6_rmb_mba_wait(qproc, 0, 5000);
>  	if (ret == -ETIMEDOUT) {
>  		dev_err(qproc->dev, "MBA boot timed out\n");
>  		goto halt_axi_ports;
> -- 
> 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 v3 1/3] remoteproc: qcom: Embed private data structure for hexagon dsp.

[Bjorn Andersson]

On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote:

> Embed resources specific to version of hexagon chip in device
> structure to avoid conditional check for manipulation of those
> resources in driver code.
> 

Please don't rename functions in the same patch as functional changes.

[..]
>  
> -static int q6v5_probe(struct platform_device *pdev)
> +static int q6_probe(struct platform_device *pdev)
>  {
>  	struct q6v5 *qproc;
>  	struct rproc *rproc;
> +	const struct q6_rproc_res *desc;
>  	int ret;
>  
> -	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops,
> -			    MBA_FIRMWARE_NAME, sizeof(*qproc));
> +	desc = of_device_get_match_data(&pdev->dev);
> +	if (!desc)
> +		return -EINVAL;
> +
> +	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6_ops,

If you didn't rename q6v5_ops the patch would look cleaner.

> +			    desc->q6_mba_image, sizeof(*qproc));
>  	if (!rproc) {
>  		dev_err(&pdev->dev, "failed to allocate rproc\n");
>  		return -ENOMEM;
>  	}
>  
> -	rproc->fw_ops = &q6v5_fw_ops;
> +	rproc->fw_ops = &q6_fw_ops;
>  
>  	qproc = (struct q6v5 *)rproc->priv;
>  	qproc->dev = &pdev->dev;
> @@ -826,6 +869,7 @@ static int q6v5_probe(struct platform_device *pdev)
>  	init_completion(&qproc->start_done);
>  	init_completion(&qproc->stop_done);
>  
> +	qproc->q6_rproc_res = desc;
>  	ret = q6v5_init_mem(qproc, pdev);
>  	if (ret)
>  		goto free_rproc;
> @@ -842,7 +886,7 @@ static int q6v5_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto free_rproc;
>  
> -	ret = q6v5_init_reset(qproc);
> +	ret = qproc->q6_rproc_res->q6_reset_init(qproc, pdev);
>  	if (ret)
>  		goto free_rproc;
>  
> @@ -873,14 +917,12 @@ static int q6v5_probe(struct platform_device *pdev)
>  		goto free_rproc;
>  
>  	return 0;
> -

Please don't do "random" cleanups.

>  free_rproc:
>  	rproc_free(rproc);
> -
>  	return ret;
>  }
>  
> -static int q6v5_remove(struct platform_device *pdev)
> +static int q6_remove(struct platform_device *pdev)
>  {
>  	struct q6v5 *qproc = platform_get_drvdata(pdev);
>  
> @@ -890,20 +932,80 @@ static int q6v5_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static const struct of_device_id q6v5_of_match[] = {
> -	{ .compatible = "qcom,q6v5-pil", },
> +char *proxy_8x96_reg_str[] = {"mx", "cx", "vdd_pll"};

This list is the same in both versions, so no need to define it here.

> +int  proxy_8x96_reg_action[3][2] = { {0, 1}, {1, 1}, {1, 0} };

This is just a magic matrix, please use a struct with some descriptive
names for these. I presume they represents "matching index in reg_load
and reg_min_voltage is non-zero and should be used" - in which case it
shouldn't be needed at all.

> +int  proxy_8x96_reg_load[] = {0, 100000, 100000};
> +int  proxy_8x96_reg_min_voltage[] = {1050000, 1250000, 0};
> +char *proxy_8x96_clk_str[] = {"xo", "pnoc", "qdss"};
> +char *active_8x96_clk_str[] = {"iface", "bus", "mem", "gpll0_mss_clk",
> +		"snoc_axi_clk", "mnoc_axi_clk"};

All these needs to be static, but I would prefer if you just put the
values directly into the resource structs below.

> +
> +static const struct q6_rproc_res msm_8996_res = {
> +	.proxy_clks = proxy_8x96_clk_str,
> +	.proxy_clk_cnt = 3,

It's common practice to use a NULL terminator in the definition list
rather than keeping separate count of the number of items.

While acquiring the resources you would have to "calculate" the number
and store it in the q6v5 struct, but this would make turn this struct
into something only used during probe() - which is nice.

> +	.active_clks = active_8x96_clk_str,
> +	.active_clk_cnt = 6,
> +	.proxy_regs = proxy_8x96_reg_str,
> +	.active_regs = NULL,
> +	.proxy_reg_action = (int **)proxy_8x96_reg_action,
> +	.proxy_reg_load = (int *)proxy_8x96_reg_load,
> +	.active_reg_action = NULL,
> +	.active_reg_load = NULL,
> +	.proxy_reg_voltage = (int *)proxy_8x96_reg_min_voltage,
> +	.active_reg_voltage = NULL,
> +	.proxy_reg_cnt = 3,
> +	.active_reg_cnt = 0,
> +	.q6_reset_init = q6v56_init_reset,
> +	.q6_version = "v56",

q6_version would be better to have as a enum.

> +	.q6_mba_image = "mba.mbn",
> +};
> +
> +char *proxy_8x16_reg_str[] = {"mx", "cx", "pll"};
> +char *active_8x16_reg_str[] = {"mss"};
> +int  proxy_8x16_reg_action[4][2] = { {0, 1}, {1, 0}, {1, 0} };
> +int  active_8x16_reg_action[1][2] = { {1, 1} };
> +int  proxy_8x16_reg_load[] = {100000, 0, 100000, 100000};
> +int  active_8x16_reg_load[] = {100000};
> +int  proxy_8x16_reg_min_voltage[] = {1050000, 0, 0};
> +int  active_8x16_reg_min_voltage[] = {1000000};
> +char *proxy_8x16_clk_str[] = {"xo"};
> +char *active_8x16_clk_str[] = {"iface", "bus", "mem"};
> +
> +static const struct q6_rproc_res msm_8916_res = {
> +	.proxy_clks = proxy_8x16_clk_str,
> +	.proxy_clk_cnt = 1,
> +	.active_clks = active_8x16_clk_str,
> +	.active_clk_cnt = 3,
> +	.proxy_regs = proxy_8x16_reg_str,
> +	.active_regs = active_8x16_reg_str,
> +	.proxy_reg_action = (int **)proxy_8x16_reg_action,
> +	.proxy_reg_load = (int *)proxy_8x16_reg_load,
> +	.active_reg_action = (int **)active_8x16_reg_action,
> +	.active_reg_load = (int *)active_8x16_reg_load,
> +	.proxy_reg_voltage = (int *)proxy_8x16_reg_min_voltage,
> +	.active_reg_voltage = active_8x16_reg_min_voltage,
> +	.proxy_reg_cnt = 3,
> +	.active_reg_cnt = 1,
> +	.q6_reset_init = q6v5_init_reset,
> +	.q6_version = "v5",
> +	.q6_mba_image = "mba.b00",

q6v55 (msm8916) also uses mba.mbn, while q6v5 (msm8974) uses mba.b00.

> +};
> +
> +static const struct of_device_id q6_of_match[] = {
> +	{ .compatible = "qcom,q6v5-pil", .data = &msm_8916_res},

As I was looking in the CAF tree, I believe the correct version for 8916
is 5.5 and v5 was used in 8974.

But I presume these versions are not strictly tied to certain platforms,
so please name the resource structs based on the q6v5 version, rather
than platforms.

> +	{ .compatible = "qcom,q6v56-pil", .data = &msm_8996_res},
>  	{ },
>  };
>  
> -static struct platform_driver q6v5_driver = {
> -	.probe = q6v5_probe,
> -	.remove = q6v5_remove,
> +static struct platform_driver q6_driver = {
> +	.probe = q6_probe,
> +	.remove = q6_remove,
>  	.driver = {
>  		.name = "qcom-q6v5-pil",
> -		.of_match_table = q6v5_of_match,
> +		.of_match_table = q6_of_match,
>  	},
>  };
> -module_platform_driver(q6v5_driver);
> +module_platform_driver(q6_driver);
>  

Regards,
Bjorn

Re: [PATCH v3 1/3] remoteproc: qcom: Embed private data structure for hexagon dsp.

[Avaneesh Kumar Dwivedi]

i have been a little delayed for posting replies to patch comments, 
hopefully onward it should be quick and fast.


On 11/8/2016 12:38 PM, Bjorn Andersson wrote:
> On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> Embed resources specific to version of hexagon chip in device
>> structure to avoid conditional check for manipulation of those
>> resources in driver code.
>>
> Please don't rename functions in the same patch as functional changes.
have corrected
>
> [..]
>>   
>> -static int q6v5_probe(struct platform_device *pdev)
>> +static int q6_probe(struct platform_device *pdev)
>>   {
>>   	struct q6v5 *qproc;
>>   	struct rproc *rproc;
>> +	const struct q6_rproc_res *desc;
>>   	int ret;
>>   
>> -	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops,
>> -			    MBA_FIRMWARE_NAME, sizeof(*qproc));
>> +	desc = of_device_get_match_data(&pdev->dev);
>> +	if (!desc)
>> +		return -EINVAL;
>> +
>> +	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6_ops,
> If you didn't rename q6v5_ops the patch would look cleaner.
Yes, corrected
>
>> +			    desc->q6_mba_image, sizeof(*qproc));
>>   	if (!rproc) {
>>   		dev_err(&pdev->dev, "failed to allocate rproc\n");
>>   		return -ENOMEM;
>>   	}
>>   
>> -	rproc->fw_ops = &q6v5_fw_ops;
>> +	rproc->fw_ops = &q6_fw_ops;
>>   
>>   	qproc = (struct q6v5 *)rproc->priv;
>>   	qproc->dev = &pdev->dev;
>> @@ -826,6 +869,7 @@ static int q6v5_probe(struct platform_device *pdev)
>>   	init_completion(&qproc->start_done);
>>   	init_completion(&qproc->stop_done);
>>   
>> +	qproc->q6_rproc_res = desc;
>>   	ret = q6v5_init_mem(qproc, pdev);
>>   	if (ret)
>>   		goto free_rproc;
>> @@ -842,7 +886,7 @@ static int q6v5_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto free_rproc;
>>   
>> -	ret = q6v5_init_reset(qproc);
>> +	ret = qproc->q6_rproc_res->q6_reset_init(qproc, pdev);
>>   	if (ret)
>>   		goto free_rproc;
>>   
>> @@ -873,14 +917,12 @@ static int q6v5_probe(struct platform_device *pdev)
>>   		goto free_rproc;
>>   
>>   	return 0;
>> -
> Please don't do "random" cleanups.

Sure, Corrected.
>
>>   free_rproc:
>>   	rproc_free(rproc);
>> -
>>   	return ret;
>>   }
>>   
>> -static int q6v5_remove(struct platform_device *pdev)
>> +static int q6_remove(struct platform_device *pdev)
>>   {
>>   	struct q6v5 *qproc = platform_get_drvdata(pdev);
>>   
>> @@ -890,20 +932,80 @@ static int q6v5_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   
>> -static const struct of_device_id q6v5_of_match[] = {
>> -	{ .compatible = "qcom,q6v5-pil", },
>> +char *proxy_8x96_reg_str[] = {"mx", "cx", "vdd_pll"};
> This list is the same in both versions, so no need to define it here.
Yes, have reused.
>
>> +int  proxy_8x96_reg_action[3][2] = { {0, 1}, {1, 1}, {1, 0} };
> This is just a magic matrix, please use a struct with some descriptive
> names for these. I presume they represents "matching index in reg_load
> and reg_min_voltage is non-zero and should be used" - in which case it
> shouldn't be needed at all.
Yes this represented the flag to set voltage and load.
Have removed this and setting voltage and load now depending upon 
specified voltage and load setting for regulators.
>
>> +int  proxy_8x96_reg_load[] = {0, 100000, 100000};
>> +int  proxy_8x96_reg_min_voltage[] = {1050000, 1250000, 0};
>> +char *proxy_8x96_clk_str[] = {"xo", "pnoc", "qdss"};
>> +char *active_8x96_clk_str[] = {"iface", "bus", "mem", "gpll0_mss_clk",
>> +		"snoc_axi_clk", "mnoc_axi_clk"};
> All these needs to be static, but I would prefer if you just put the
> values directly into the resource structs below.
If i have to initialize directly into resource struct, i need to declare 
individual elements as array of fixed size
but number of resource lets say  number of proxy regulator not being 
same for different q6 chips, made me to
work with double pointer which can be assigned with address of an array 
of string pointer as per need when new version need to be supported.

Though i have made above elements as static in next patch.
>
>> +
>> +static const struct q6_rproc_res msm_8996_res = {
>> +	.proxy_clks = proxy_8x96_clk_str,
>> +	.proxy_clk_cnt = 3,
> It's common practice to use a NULL terminator in the definition list
> rather than keeping separate count of the number of items.
>
> While acquiring the resources you would have to "calculate" the number
> and store it in the q6v5 struct, but this would make turn this struct
> into something only used during probe() - which is nice.

Yes, have modified as per suggestion.
>
>> +	.active_clks = active_8x96_clk_str,
>> +	.active_clk_cnt = 6,
>> +	.proxy_regs = proxy_8x96_reg_str,
>> +	.active_regs = NULL,
>> +	.proxy_reg_action = (int **)proxy_8x96_reg_action,
>> +	.proxy_reg_load = (int *)proxy_8x96_reg_load,
>> +	.active_reg_action = NULL,
>> +	.active_reg_load = NULL,
>> +	.proxy_reg_voltage = (int *)proxy_8x96_reg_min_voltage,
>> +	.active_reg_voltage = NULL,
>> +	.proxy_reg_cnt = 3,
>> +	.active_reg_cnt = 0,
>> +	.q6_reset_init = q6v56_init_reset,
>> +	.q6_version = "v56",
> q6_version would be better to have as a enum.

have removed this entry.
each class of q6 lets say "v56" have again many version of hexagon with 
minor differences wrt each other.
for example msm8996 use "v56" 1.5.0 while MSM8952 uses 1.8.0, reset 
sequence and some other operation differ wrt to this version in terms of 
order of register programming. so i have introduced one variable in q6v5 
struct per q6 chip supported, if this is defined then we can check and 
carry out version specific instruction.
will this be OK?

>
>> +	.q6_mba_image = "mba.mbn",
>> +};
>> +
>> +char *proxy_8x16_reg_str[] = {"mx", "cx", "pll"};
>> +char *active_8x16_reg_str[] = {"mss"};
>> +int  proxy_8x16_reg_action[4][2] = { {0, 1}, {1, 0}, {1, 0} };
>> +int  active_8x16_reg_action[1][2] = { {1, 1} };
>> +int  proxy_8x16_reg_load[] = {100000, 0, 100000, 100000};
>> +int  active_8x16_reg_load[] = {100000};
>> +int  proxy_8x16_reg_min_voltage[] = {1050000, 0, 0};
>> +int  active_8x16_reg_min_voltage[] = {1000000};
>> +char *proxy_8x16_clk_str[] = {"xo"};
>> +char *active_8x16_clk_str[] = {"iface", "bus", "mem"};
>> +
>> +static const struct q6_rproc_res msm_8916_res = {
>> +	.proxy_clks = proxy_8x16_clk_str,
>> +	.proxy_clk_cnt = 1,
>> +	.active_clks = active_8x16_clk_str,
>> +	.active_clk_cnt = 3,
>> +	.proxy_regs = proxy_8x16_reg_str,
>> +	.active_regs = active_8x16_reg_str,
>> +	.proxy_reg_action = (int **)proxy_8x16_reg_action,
>> +	.proxy_reg_load = (int *)proxy_8x16_reg_load,
>> +	.active_reg_action = (int **)active_8x16_reg_action,
>> +	.active_reg_load = (int *)active_8x16_reg_load,
>> +	.proxy_reg_voltage = (int *)proxy_8x16_reg_min_voltage,
>> +	.active_reg_voltage = active_8x16_reg_min_voltage,
>> +	.proxy_reg_cnt = 3,
>> +	.active_reg_cnt = 1,
>> +	.q6_reset_init = q6v5_init_reset,
>> +	.q6_version = "v5",
>> +	.q6_mba_image = "mba.b00",
> q6v55 (msm8916) also uses mba.mbn, while q6v5 (msm8974) uses mba.b00.
Again this is a case where compatible string can not help, we should 
have single compatible string i.e. "q6v5" for msm8916 and msm8974 since 
both are from same class of q6 chip with different version.
so if we cant initialize mdt name based on compatible string alone.
>
>> +};
>> +
>> +static const struct of_device_id q6_of_match[] = {
>> +	{ .compatible = "qcom,q6v5-pil", .data = &msm_8916_res},
> As I was looking in the CAF tree, I believe the correct version for 8916
> is 5.5 and v5 was used in 8974.
>
> But I presume these versions are not strictly tied to certain platforms,
> so please name the resource structs based on the q6v5 version, rather
> than platforms.
Yes, resource are tied to compatible and version of q6.
have used compatible to initialize major resources but using DT 
specified version string for minor deviations where needed.
Should this be fine?
as far as version on 8916 and 8974 are concern they are as below.

msm8974 q6v5 core version  5.0.0
msm8916 q6v5 core version  5.1.1
>
>> +	{ .compatible = "qcom,q6v56-pil", .data = &msm_8996_res},
>>   	{ },
>>   };
>>   
>> -static struct platform_driver q6v5_driver = {
>> -	.probe = q6v5_probe,
>> -	.remove = q6v5_remove,
>> +static struct platform_driver q6_driver = {
>> +	.probe = q6_probe,
>> +	.remove = q6_remove,
>>   	.driver = {
>>   		.name = "qcom-q6v5-pil",
>> -		.of_match_table = q6v5_of_match,
>> +		.of_match_table = q6_of_match,
>>   	},
>>   };
>> -module_platform_driver(q6v5_driver);
>> +module_platform_driver(q6_driver);
>>   
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] remoteproc: qcom: Hexagon resource handling

[Avaneesh Kumar Dwivedi]

On 11/8/2016 12:19 PM, Bjorn Andersson wrote:
> On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> Handling of clock and regulator resources as well as reset
>> register programing differ according to version of hexagon
>> dsp hardware. Different version require different resources
>> and different parameters for same resource. Hence it is
>> needed that resource handling is generic and independent of
>> hexagon dsp version.
>>
> It would be much easier to review this if you didn't do all three
> changes in the same patch, and at the same time changed the function
> names. There's large parts of this patch where it's not obvious what the
> actual changes are.

OK, have broken patches in very small set of function now.
but patches has increased from 3 to 9.
sorry for inconvenience caused.
>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 471 +++++++++++++++++++++++++++----------
>>   1 file changed, 344 insertions(+), 127 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index b60dff3..1fc505b 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/mutex.h>
>>   #include <linux/of_device.h>
>>   
>>   #include "remoteproc_internal.h"
>> @@ -93,6 +94,8 @@
>>   #define QDSS_BHS_ON			BIT(21)
>>   #define QDSS_LDO_BYP			BIT(22)
>>   
>> +#define QDSP6v56_CLAMP_WL               BIT(21)
>> +#define QDSP6v56_CLAMP_QMC_MEM          BIT(22)
>>   struct q6_rproc_res {
>>   	char **proxy_clks;
>>   	int proxy_clk_cnt;
>> @@ -129,11 +132,11 @@ struct q6v5 {
>>   	struct qcom_smem_state *state;
>>   	unsigned stop_bit;
>>   
>> -	struct regulator_bulk_data supply[4];
>>   	const struct q6_rproc_res *q6_rproc_res;
>> -	struct clk *ahb_clk;
>> -	struct clk *axi_clk;
>> -	struct clk *rom_clk;
>> +	struct clk **active_clks;
>> +	struct clk **proxy_clks;
>> +	struct regulator **proxy_regs;
>> +	struct regulator **active_regs;
> Keeping these as statically sized arrays, potentially with unused
> elements at the end removes the need for allocating the storage and the
> double pointers.
since i do not know how many resource of a particular type will be 
needed on new version of new class of hexagon that is why i am working 
with pointers.
have removed many entries from above resource struct, it will lok much 
cleaner in next patchset.
>
>>   
>>   	struct completion start_done;
>>   	struct completion stop_done;
>> @@ -147,67 +150,245 @@ struct q6v5 {
>>   	phys_addr_t mpss_reloc;
>>   	void *mpss_region;
>>   	size_t mpss_size;
>> +	struct mutex q6_lock;
>> +	bool proxy_unvote_reg;
>> +	bool proxy_unvote_clk;
> I still don't see the need for these 3 attributes.
I agree, Have removed them.
>
>>   };
>>   
>> -enum {
>> -	Q6V5_SUPPLY_CX,
>> -	Q6V5_SUPPLY_MX,
>> -	Q6V5_SUPPLY_MSS,
>> -	Q6V5_SUPPLY_PLL,
>> -};
>> +static int q6_regulator_init(struct q6v5 *qproc)
>> +{
>> +	struct regulator **reg_arr;
>> +	int i;
>> +
>> +	if (qproc->q6_rproc_res->proxy_reg_cnt) {
> If you keep proxy_regs and active_regs as arrays you don't need this
> check.
Agree, have removed check.
>
>> +		reg_arr = devm_kzalloc(qproc->dev,
>> +		sizeof(reg_arr) * qproc->q6_rproc_res->proxy_reg_cnt,
>> +		GFP_KERNEL);
>> +
>> +		for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
>> +			reg_arr[i] = devm_regulator_get(qproc->dev,
>> +			qproc->q6_rproc_res->proxy_regs[i]);
>> +			if (IS_ERR(reg_arr[i]))
>> +				return PTR_ERR(reg_arr[i]);
>> +			qproc->proxy_regs = reg_arr;
>> +		}
>> +	}
>> +
>> +	if (qproc->q6_rproc_res->active_reg_cnt) {
>> +		reg_arr = devm_kzalloc(qproc->dev,
>> +		sizeof(reg_arr) * qproc->q6_rproc_res->active_reg_cnt,
>> +		GFP_KERNEL);
>> +
>> +		for (i = 0; i < qproc->q6_rproc_res->active_reg_cnt; i++) {
>> +			reg_arr[i] = devm_regulator_get(qproc->dev,
>> +			qproc->q6_rproc_res->active_regs[i]);
>> +
>> +			if (IS_ERR(reg_arr[i]))
>> +				return PTR_ERR(reg_arr[i]);
>> +			qproc->active_regs = reg_arr;
>> +		}
>> +	}
> Please keep active_regs and proxy_regs as regulator_bulk_data and
> continue to use devm_regulator_bulk_get(), it should make this code
> cleaner.
the way i have reorganized code in next patchset i found using 
devm_regulator_get() more convenient, can i keep using them? as i am 
reading string one by one and based on read string filling a regulator 
struct with its voltage and load and handle info.
>
>> +
>> +	return 0;
>> +}
>>   
>> -static int q6v5_regulator_init(struct q6v5 *qproc)
>> +static int q6_proxy_regulator_enable(struct q6v5 *qproc)
>>   {
>> -	int ret;
>> +	int i, j, ret = 0;
>> +	int **reg_loadnvoltsetflag;
>> +	int *reg_load;
>> +	int *reg_voltage;
>> +
>> +	reg_loadnvoltsetflag = qproc->q6_rproc_res->proxy_reg_action;
>> +	reg_load = qproc->q6_rproc_res->proxy_reg_load;
>> +	reg_voltage = qproc->q6_rproc_res->proxy_reg_voltage;
> Rather then keeping these properties on int-arrays I strongly prefer
> that you would have a struct { uV, uA } for each regulator.
Have modified as per suggestion.
>
>> +
>> +	for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
>> +		for (j = 0; j <= 1; j++) {
>> +			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
> I'm sorry, but this is not clean. Please use the fact that we're not
> writing assembly code and use the language to your advantage.
Sorry for mess, have simplified and cleaned.
>
>> +				regulator_set_load(qproc->proxy_regs[i],
>> +				reg_load[i]);
>> +			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
>> +				regulator_set_voltage(qproc->proxy_regs[i],
>> +				reg_voltage[i], INT_MAX);
>> +		}
>> +	}
>>   
>> -	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";
>> +	for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
>> +		ret = regulator_enable(qproc->proxy_regs[i]);
>> +		if (ret) {
>> +			for (; i > 0; --i) {
>> +				regulator_disable(qproc->proxy_regs[i]);
>> +				return ret;
>> +			}
>> +		}
>> +	}
> If you just keep your regulators in a regulator_bulk_data array then you
> can replace this with regulator_bulk_enable(proxy_reg_cnt, proxy_regs);
As replied above i am going with getting sigle regulator handle one time.
let me know if i can continue or need to change?

>
>>   
>> -	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;
>> +	qproc->proxy_unvote_reg = true;
> This should still not be needed.
Yes Removed
>
>> +
>> +	return 0;
>> +}
>> +
>> +static int q6_active_regulator_enable(struct q6v5 *qproc)
>> +{
>> +	int i, j, ret = 0;
>> +	int **reg_loadnvoltsetflag;
>> +	int *reg_load;
>> +	int *reg_voltage;
>> +
>> +	reg_loadnvoltsetflag = qproc->q6_rproc_res->active_reg_action;
>> +	reg_load = qproc->q6_rproc_res->active_reg_load;
>> +	reg_voltage = qproc->q6_rproc_res->active_reg_voltage;
>> +
>> +	for (i = 0; i < qproc->q6_rproc_res->active_reg_cnt; i++) {
>> +		for (j = 0; j <= 1; j++) {
>> +			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
>> +				regulator_set_load(qproc->active_regs[i],
>> +				reg_load[i]);
>> +			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
>> +				regulator_set_voltage(qproc->active_regs[i],
>> +				reg_voltage[i], INT_MAX);
>> +		}
>>   	}
>>   
>> -	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);
>> +	for (i = 0; i < qproc->q6_rproc_res->active_reg_cnt; i++) {
>> +		ret = regulator_enable(qproc->active_regs[i]);
>> +		if (ret) {
>> +			for (; i > 0; --i) {
>> +				regulator_disable(qproc->active_regs[i]);
>> +				return ret;
>> +			}
>> +		}
>> +	}
>>   
>>   	return 0;
>>   }
>>   
>> -static int q6v5_regulator_enable(struct q6v5 *qproc)
>> +static int q6_regulator_enable(struct q6v5 *qproc)
>>   {
>> -	struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer;
>> -	struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer;
>>   	int ret;
>>   
>> -	/* TODO: Q6V5_SUPPLY_CX is supposed to be set to super-turbo here */
>> +	if (qproc->q6_rproc_res->proxy_reg_cnt)
>> +		ret = q6_proxy_regulator_enable(qproc);
>>   
>> -	ret = regulator_set_voltage(mx, 1050000, INT_MAX);
>> -	if (ret)
>> -		return ret;
>> +	if (qproc->q6_rproc_res->active_reg_cnt)
> q6_active_regulator_enable() is a no-op if active_reg_cnt is 0, so no
> need to check that. Rather than having two functions, try to
> parameterize the regulator enable functions so that you can have a
> single function that you pass the active or proxy list.
Agreed, have modified
>
>> +		ret = q6_active_regulator_enable(qproc);
>>   
>> -	regulator_set_voltage(mss, 1000000, 1150000);
>> +	return ret;
>> +}
>>   
>> -	return regulator_bulk_enable(ARRAY_SIZE(qproc->supply), qproc->supply);
>> +static int q6_proxy_regulator_disable(struct q6v5 *qproc)
>> +{
>> +	int i, j;
>> +	int **reg_loadnvoltsetflag;
>> +
>> +	reg_loadnvoltsetflag = qproc->q6_rproc_res->proxy_reg_action;
>> +	if (!qproc->proxy_unvote_reg)
>> +		return 0;
>> +	for (i = qproc->q6_rproc_res->proxy_reg_cnt-1; i >= 0; i--) {
>> +		for (j = 0; j <= 1; j++) {
>> +			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
>> +				regulator_set_load(qproc->proxy_regs[i], 0);
>> +			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
>> +				regulator_set_voltage(qproc->proxy_regs[i],
>> +				0, INT_MAX);
>> +		}
>> +	}
>> +	for (i = qproc->q6_rproc_res->proxy_reg_cnt-1; i >= 0; i--)
>> +		regulator_disable(qproc->proxy_regs[i]);
>> +	qproc->proxy_unvote_reg = false;
>> +	return 0;
>>   }
>>   
>> -static void q6v5_regulator_disable(struct q6v5 *qproc)
>> +static int q6_active_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, j, ret = 0;
>> +	int **reg_loadnvoltsetflag;
>> +
>> +	reg_loadnvoltsetflag = qproc->q6_rproc_res->active_reg_action;
>> +
>> +	for (i = qproc->q6_rproc_res->active_reg_cnt-1; i > 0; i--) {
>> +		for (j = 0; j <= 1; j++) {
>> +			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
>> +				regulator_set_load(qproc->active_regs[i], 0);
>> +			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
>> +				regulator_set_voltage(qproc->active_regs[i],
>> +				0, INT_MAX);
>> +		}
>> +	}
>> +	for (i = qproc->q6_rproc_res->active_reg_cnt-1; i >= 0; i--)
>> +		ret = regulator_disable(qproc->proxy_regs[i]);
>> +	return 0;
>> +}
>> +
>> +static void q6_regulator_disable(struct q6v5 *qproc)
>> +{
>> +	if (qproc->q6_rproc_res->proxy_reg_cnt)
>> +		q6_proxy_regulator_disable(qproc);
>>   
>> -	/* TODO: Q6V5_SUPPLY_CX corner votes should be released */
>> +	if (qproc->q6_rproc_res->active_reg_cnt)
>> +		q6_active_regulator_disable(qproc);
>> +}
>>   
>> -	regulator_bulk_disable(ARRAY_SIZE(qproc->supply), qproc->supply);
>> -	regulator_set_voltage(mx, 0, INT_MAX);
>> -	regulator_set_voltage(mss, 0, 1150000);
>> +static int q6_proxy_clk_enable(struct q6v5 *qproc)
> This is really the same as active_clk_enable(), so you should just have
> a function that you pass an array of clocks and a count to - similar to
> regulator_bulk_enable().
Yes , modified.
>
>> +{
>> +	int i, ret = 0;
>> +
>> +	for (i = 0; i < qproc->q6_rproc_res->proxy_clk_cnt; i++) {
>> +		ret = clk_prepare_enable(qproc->proxy_clks[i]);
>> +		if (ret) {
>> +			for (; i > 0; --i) {
>> +				clk_disable_unprepare(qproc->proxy_clks[i]);
>> +				return ret;
>> +			}
>> +		}
>> +	}
>> +	qproc->proxy_unvote_clk = true;
>> +	return 0;
>>   }
>>   
>> +static void q6_proxy_clk_disable(struct q6v5 *qproc)
>> +{
>> +	int i;
>> +
>> +	if (!qproc->proxy_unvote_clk)
>> +		return;
>> +	for (i = qproc->q6_rproc_res->proxy_clk_cnt-1; i >= 0; i--)
>> +		clk_disable_unprepare(qproc->proxy_clks[i]);
>> +	qproc->proxy_unvote_clk = false;
>> +}
>> +
>> +static int q6_active_clk_enable(struct q6v5 *qproc)
>> +{
>> +	int i, ret = 0;
> No need to initialize ret, as its first use is an assignment.
>
>> +
>> +	for (i = 0; i < qproc->q6_rproc_res->active_clk_cnt; i++) {
>> +		ret = clk_prepare_enable(qproc->active_clks[i]);
>> +		if (ret) {
> Use goto here, rather than nesting a error return in here.
OK
>
>> +			for (; i > 0; i--) {
>> +				clk_disable_unprepare(qproc->active_clks[i]);
>> +				return ret;
>> +			}
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>> +static void q6_active_clk_disable(struct q6v5 *qproc)
>> +{
>> +	int i;
>> +
>> +	for (i = qproc->q6_rproc_res->active_clk_cnt-1; i >= 0; i--)
>> +		clk_disable_unprepare(qproc->active_clks[i]);
>> +}
>> +
>> +static void pil_mss_restart_reg(struct q6v5 *qproc, u32 mss_restart)
>> +{
>> +	if (qproc->restart_reg) {
>> +		writel_relaxed(mss_restart, qproc->restart_reg);
>> +		udelay(2);
>> +	}
>> +}
>>   static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>>   {
>>   	struct q6v5 *qproc = rproc->priv;
>> @@ -340,11 +521,6 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>>   	unsigned int val;
>>   	int ret;
>>   
>> -	/* Check if we're already idle */
>> -	ret = regmap_read(halt_map, offset + AXI_IDLE_REG, &val);
>> -	if (!ret && val)
>> -		return;
>> -
> Please put this in its own commit and describe why it can't be there on
> 8996 and why it's okay to drop on 8974 and 8916.
>
>>   	/* Assert halt request */
>>   	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 1);
>>   
>> @@ -366,7 +542,7 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>>   	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 0);
>>   }
>>   
>> -static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>> +static int q6_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>>   {
>>   	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
>>   	dma_addr_t phys;
>> @@ -395,7 +571,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>>   	return ret < 0 ? ret : 0;
>>   }
>>   
>> -static int q6v5_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
>> +static int q6_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
>>   {
>>   	const struct elf32_phdr *phdrs;
>>   	const struct elf32_phdr *phdr;
>> @@ -451,7 +627,7 @@ static int q6v5_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
>>   	return ret < 0 ? ret : 0;
>>   }
>>   
>> -static int q6v5_mpss_load(struct q6v5 *qproc)
>> +static int q6_mpss_load(struct q6v5 *qproc)
>>   {
>>   	const struct firmware *fw;
>>   	phys_addr_t fw_addr;
>> @@ -476,7 +652,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>   	/* Initialize the RMB validator */
>>   	writel(0, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>   
>> -	ret = q6v5_mpss_init_image(qproc, fw);
>> +	ret = q6_mpss_init_image(qproc, fw);
>>   	if (ret)
>>   		goto release_firmware;
>>   
>> @@ -484,7 +660,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>   	if (ret)
>>   		goto release_firmware;
>>   
>> -	ret = q6v5_mpss_validate(qproc, fw);
>> +	ret = q6_mpss_validate(qproc, fw);
>>   
>>   release_firmware:
>>   	release_firmware(fw);
>> @@ -492,36 +668,41 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>   	return ret < 0 ? ret : 0;
>>   }
>>   
>> -static int q6v5_start(struct rproc *rproc)
>> +static int q6_start(struct rproc *rproc)
> Most of the changes in this function are renaming of functions and
> variables, please don't do this as part of a functional change.
>
> Best would be if you start with a commit that renames the necessary
> parts and where you specify that there's "no functional change".

OK, Done.
>
>>   {
>>   	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>>   	int ret;
>>   
>> -	ret = q6v5_regulator_enable(qproc);
>> +	mutex_lock(&qproc->q6_lock);
> We should already be protected by the rproc->lock here, please let me
> know if there are any gaps.
Sure, Removed.
>
>> +	ret = q6_regulator_enable(qproc);
>>   	if (ret) {
>> -		dev_err(qproc->dev, "failed to enable supplies\n");
>> +		dev_err(qproc->dev, "failed to enable reg supplies\n");
> Supplies are regulators, but if you find this confusing then you
> shouldn't abbreviate regulators as "reg".
OK, Done.
>
>>   		return ret;
>>   	}
>>   
>> -	ret = reset_control_deassert(qproc->mss_restart);
> So the correct order is: enable clocks, then deassert reset?
No, deassert need to be done before Q6 clocks are enabled, Done.
>
>> +	ret = q6_proxy_clk_enable(qproc);
>>   	if (ret) {
>> -		dev_err(qproc->dev, "failed to deassert mss restart\n");
>> -		goto disable_vdd;
>> +		dev_err(qproc->dev, "failed to enable proxy_clk\n");
>> +		goto err_proxy_clk;
>>   	}
>>   
>> -	ret = clk_prepare_enable(qproc->ahb_clk);
>> -	if (ret)
>> -		goto assert_reset;
>> -
>> -	ret = clk_prepare_enable(qproc->axi_clk);
>> -	if (ret)
>> -		goto disable_ahb_clk;
>> +	ret = q6_active_clk_enable(qproc);
>> +	if (ret) {
>> +		dev_err(qproc->dev, "failed to enable active clocks\n");
>> +		goto err_active_clks;
>> +	}
>>   
>> -	ret = clk_prepare_enable(qproc->rom_clk);
>> -	if (ret)
>> -		goto disable_axi_clk;
>> +	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56"))
>> +		pil_mss_restart_reg(qproc, 0);
>> +	else {
>> +		ret = reset_control_deassert(qproc->mss_restart);
>> +		if (ret) {
>> +			dev_err(qproc->dev, "failed to deassert mss restart\n");
>> +			goto err_deassert;
>> +		}
>> +	}
>>   
>> -	writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
>> +	writel_relaxed(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
> There's no functional reason for changing writel to writel_relaxed, so
> please do this in a separate commit and motivate it well.
OK, Done.
>
>>   
>>   	ret = q6v5proc_reset(qproc);
>>   	if (ret)
>> @@ -539,13 +720,11 @@ static int q6v5_start(struct rproc *rproc)
>>   	}
>>   
>>   	dev_info(qproc->dev, "MBA booted, loading mpss\n");
>> -
>> -	ret = q6v5_mpss_load(qproc);
>> +	ret = q6_mpss_load(qproc);
>>   	if (ret)
>>   		goto halt_axi_ports;
>> -
>>   	ret = wait_for_completion_timeout(&qproc->start_done,
>> -					  msecs_to_jiffies(5000));
>> +					  msecs_to_jiffies(10000));
> Please put this in a separate commit and describe why 10 seconds is
> better than 5.
It was an experimental change made during validation, found its way in 
patch. have reverted it back.
>
>>   	if (ret == 0) {
>>   		dev_err(qproc->dev, "start timed out\n");
>>   		ret = -ETIMEDOUT;
>> @@ -553,36 +732,33 @@ static int q6v5_start(struct rproc *rproc)
>>   	}
>>   
>>   	qproc->running = true;
>> -
>>   	/* TODO: All done, release the handover resources */
>> -
>> +	q6_proxy_clk_disable(qproc);
>> +	q6_proxy_regulator_disable(qproc);
> This is good, please drop the TODO comment now that we're done.
Ok, Done.
>
>> +	mutex_unlock(&qproc->q6_lock);
>>   	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);
>> -assert_reset:
>> -	reset_control_assert(qproc->mss_restart);
> Don't we need to assert the reset again?
Yes needed, have corrected.
>
>> -disable_vdd:
>> -	q6v5_regulator_disable(qproc);
>> -
>> +err_deassert:
>> +	q6_active_clk_disable(qproc);
>> +err_active_clks:
>> +	q6_proxy_clk_disable(qproc);
>> +err_proxy_clk:
> It's better if the labels describe the action than the source of the
> jump, so please keep the "disable_vdd" label for this - it also makes
> your patch cleaner.
OK,  Done.
>
>> +	q6_regulator_disable(qproc);
>> +	mutex_unlock(&qproc->q6_lock);
>>   	return ret;
>>   }
>>   
>> -static int q6v5_stop(struct rproc *rproc)
>> +static int q6_stop(struct rproc *rproc)
>>   {
>>   	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>>   	int ret;
>> +	u64 val;
>>   
>> -	qproc->running = false;
>> -
>> +	mutex_lock(&qproc->q6_lock);
>>   	qcom_smem_state_update_bits(qproc->state,
>>   				    BIT(qproc->stop_bit), BIT(qproc->stop_bit));
>>   
>> @@ -597,16 +773,30 @@ 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);
>>   
>> -	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_regulator_disable(qproc);
>> -
>> +	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56")) {
> This would be much better as an enum than a string. But I keep wonder if
> this is only for v5.6 of the Hexagon - perhaps should we clamp different
> things on the various versions?.

As replied elsewhere, we need a DT entry to know which version is 
running, or else many compatible string will be required. for "v56" 
there are following version, so as and when we need to support a new 
version we will require
a new DT entry which when defined will help to take deviation where 
required.
1.10.0
1.3.0
1.4.0
1.5.0
1.6.0
1.8.0

>
>> +		/*
>> +		 * 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.
>> +		 */
>> +
>> +		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);
>> +		pil_mss_restart_reg(qproc, 1);
> And by using the reset framework for mss_restart this will fall out of
> the conditional segment and the else is gone.
As i informed MSS RESET REGISTER was never a block control reset or BCR 
(a term used to define those reset register which control a clock or pll 
) so clock control reset framework can not be used to do reset 
programming for MSS
that is why i have adopted IOREMAP based mss reset programming. it is 
like any other register, may i know if any serious objection on using 
reset controller framework only? i will have to add another dummy driver 
just to do reset register programming.
let me know please if it is mandatory?
>
>> +	} else
>> +		reset_control_assert(qproc->mss_restart);
>> +	q6_active_clk_disable(qproc);
>> +	q6_proxy_clk_disable(qproc);
>> +	q6_proxy_regulator_disable(qproc);
>> +	q6_active_regulator_disable(qproc);
>> +	qproc->running = false;
>> +	mutex_unlock(&qproc->q6_lock);
>>   	return 0;
>>   }
>>   
> Regards,
> Bjorn

Re: [PATCH v3 3/3] remoteproc: qcom: Adding q6v56 reset sequence.

[Avaneesh Kumar Dwivedi]

On 11/8/2016 12:25 PM, Bjorn Andersson wrote:
> On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> Adding additional reset sequence steps required specific
>> to q6v56 based on version check, along with some trivial
>> changes in name of local functions.
>>
> Please don't change readl/writel to their relaxed version in the same
> commit as you do functional changes. And please don't change function
> names here either.
>
> Both makes the review really hard and the actual changes non-obvious.

Ok, corrected.
>
> Regards,
> Bjorn
>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 132 +++++++++++++++++++++++++++----------
>>   1 file changed, 98 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 1fc505b..68eda4f 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -66,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
>> @@ -94,8 +96,14 @@
>>   #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 q6_rproc_res {
>>   	char **proxy_clks;
>>   	int proxy_clk_cnt;
>> @@ -389,7 +397,8 @@ static void pil_mss_restart_reg(struct q6v5 *qproc, u32 mss_restart)
>>   		udelay(2);
>>   	}
>>   }
>> -static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>> +
>> +static int q6_load(struct rproc *rproc, const struct firmware *fw)
>>   {
>>   	struct q6v5 *qproc = rproc->priv;
>>   
>> @@ -400,10 +409,10 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>>   
>>   static const struct rproc_fw_ops q6_fw_ops = {
>>   	.find_rsc_table = qcom_mdt_find_rsc_table,
>> -	.load = q6v5_load,
>> +	.load = q6_load,
>>   };
>>   
>> -static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms)
>> +static int q6_rmb_pbl_wait(struct q6v5 *qproc, int ms)
>>   {
>>   	unsigned long timeout;
>>   	s32 val;
>> @@ -423,7 +432,7 @@ static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms)
>>   	return val;
>>   }
>>   
>> -static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>> +static int q6_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>>   {
>>   
>>   	unsigned long timeout;
>> @@ -449,40 +458,95 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>>   	return val;
>>   }
>>   
>> -static int q6v5proc_reset(struct q6v5 *qproc)
>> +static int q6proc_reset(struct q6v5 *qproc)
>>   {
>> -	u32 val;
>> -	int ret;
>> +	int ret, i, count;
>> +	u64 val;
>> +
>> +	/* Override the ACC value if required */
>> +	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56"))
>> +		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 = readl_relaxed(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);
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_RESET_REG);
>>   
>> +	/* BHS require xo cbcr to be enabled */
>> +	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56")) {
>> +		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");
>> +	}
>>   	/* 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_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	val |= QDSP6v56_BHS_ON;
>> +	writel_relaxed(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);
>> +	/* Put LDO in bypass mode */
>> +	val |= QDSP6v56_LDO_BYP;
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56")) {
>> +		/*
>> +		 * 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);
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>>   
>>   	/* Bring core out of reset */
>>   	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
>> @@ -490,9 +554,9 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>   	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
>>   
>>   	/* Turn on core clock */
>> -	val = readl(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
>> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
>>   	val |= Q6SS_CLK_ENABLE;
>> -	writel(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
>>   
>>   	/* Start core execution */
>>   	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
>> @@ -500,7 +564,7 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>   	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
>>   
>>   	/* Wait for PBL status */
>> -	ret = q6v5_rmb_pbl_wait(qproc, 1000);
>> +	ret = q6_rmb_pbl_wait(qproc, 1000);
>>   	if (ret == -ETIMEDOUT) {
>>   		dev_err(qproc->dev, "PBL boot timed out\n");
>>   	} else if (ret != RMB_PBL_SUCCESS) {
>> @@ -560,7 +624,7 @@ static int q6_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>>   	writel(phys, qproc->rmb_base + RMB_PMI_META_DATA_REG);
>>   	writel(RMB_CMD_META_DATA_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>>   
>> -	ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
>> +	ret = q6_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
>>   	if (ret == -ETIMEDOUT)
>>   		dev_err(qproc->dev, "MPSS header authentication timed out\n");
>>   	else if (ret < 0)
>> @@ -618,7 +682,7 @@ static int q6_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
>>   		writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>   	}
>>   
>> -	ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000);
>> +	ret = q6_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000);
>>   	if (ret == -ETIMEDOUT)
>>   		dev_err(qproc->dev, "MPSS authentication timed out\n");
>>   	else if (ret < 0)
>> @@ -704,11 +768,11 @@ static int q6_start(struct rproc *rproc)
>>   
>>   	writel_relaxed(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
>>   
>> -	ret = q6v5proc_reset(qproc);
>> +	ret = q6proc_reset(qproc);
>>   	if (ret)
>>   		goto halt_axi_ports;
>>   
>> -	ret = q6v5_rmb_mba_wait(qproc, 0, 5000);
>> +	ret = q6_rmb_mba_wait(qproc, 0, 5000);
>>   	if (ret == -ETIMEDOUT) {
>>   		dev_err(qproc->dev, "MBA boot timed out\n");
>>   		goto halt_axi_ports;
>> -- 
>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project.
>>
> --
> 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

Re: [PATCH v3 1/3] remoteproc: qcom: Embed private data structure for hexagon dsp.

[Bjorn Andersson]

On Wed 16 Nov 06:41 PST 2016, Avaneesh Kumar Dwivedi wrote:

> i have been a little delayed for posting replies to patch comments,
> hopefully onward it should be quick and fast.
> 

I would greatly appreciate if you allow for a discussion before posting
new revisions of the patchset. I will respond to your comments here and
ignore v4 for now.

> 
> On 11/8/2016 12:38 PM, Bjorn Andersson wrote:
> >On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote:
[..]
> >>+char *active_8x96_clk_str[] = {"iface", "bus", "mem", "gpll0_mss_clk",
> >>+		"snoc_axi_clk", "mnoc_axi_clk"};
> >All these needs to be static, but I would prefer if you just put the
> >values directly into the resource structs below.
> If i have to initialize directly into resource struct, i need to declare
> individual elements as array of fixed size
> but number of resource lets say  number of proxy regulator not being same
> for different q6 chips, made me to
> work with double pointer which can be assigned with address of an array of
> string pointer as per need when new version need to be supported.
> 

Using a termination sentinel to indicate end of lists is quite common in
the kernel, so you can do this:

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

> Though i have made above elements as static in next patch.
> >
> >>+
> >>+static const struct q6_rproc_res msm_8996_res = {
> >>+	.proxy_clks = proxy_8x96_clk_str,
> >>+	.proxy_clk_cnt = 3,
> >It's common practice to use a NULL terminator in the definition list
> >rather than keeping separate count of the number of items.
> >
> >While acquiring the resources you would have to "calculate" the number
> >and store it in the q6v5 struct, but this would make turn this struct
> >into something only used during probe() - which is nice.
> 
> Yes, have modified as per suggestion.
> >
> >>+	.active_clks = active_8x96_clk_str,
> >>+	.active_clk_cnt = 6,
> >>+	.proxy_regs = proxy_8x96_reg_str,
> >>+	.active_regs = NULL,
> >>+	.proxy_reg_action = (int **)proxy_8x96_reg_action,
> >>+	.proxy_reg_load = (int *)proxy_8x96_reg_load,
> >>+	.active_reg_action = NULL,
> >>+	.active_reg_load = NULL,
> >>+	.proxy_reg_voltage = (int *)proxy_8x96_reg_min_voltage,
> >>+	.active_reg_voltage = NULL,
> >>+	.proxy_reg_cnt = 3,
> >>+	.active_reg_cnt = 0,
> >>+	.q6_reset_init = q6v56_init_reset,
> >>+	.q6_version = "v56",
> >q6_version would be better to have as a enum.
> 
> have removed this entry.
> each class of q6 lets say "v56" have again many version of hexagon with
> minor differences wrt each other.

Okay, I'm fine with us sticking to classes, but I would like for them to
make sense - and be listed as an enum instead of a string, to simplify
the code.

> for example msm8996 use "v56" 1.5.0 while MSM8952 uses 1.8.0, reset sequence

In msm-3.18 8996 is listed to be v55.

> and some other operation differ wrt to this version in terms of order of
> register programming. so i have introduced one variable in q6v5 struct per
> q6 chip supported, if this is defined then we can check and carry out
> version specific instruction.
> will this be OK?
> 

Generally in the Linux kernel it's frowned upon to carry the version
information and then do conditional operation on this.

It's preferred to carry explicit flags through the implementation, e.g.
carrying "mba.mbn" vs "mba.b00" rather than switching based on "version"
at the point of use of this data.

But I'm not sure if the other differences has reasonable names, e.g. how
to we denote the differences in reset sequence?

> >
> >>+	.q6_mba_image = "mba.mbn",
> >>+};
> >>+
> >>+char *proxy_8x16_reg_str[] = {"mx", "cx", "pll"};
> >>+char *active_8x16_reg_str[] = {"mss"};
> >>+int  proxy_8x16_reg_action[4][2] = { {0, 1}, {1, 0}, {1, 0} };
> >>+int  active_8x16_reg_action[1][2] = { {1, 1} };
> >>+int  proxy_8x16_reg_load[] = {100000, 0, 100000, 100000};
> >>+int  active_8x16_reg_load[] = {100000};
> >>+int  proxy_8x16_reg_min_voltage[] = {1050000, 0, 0};
> >>+int  active_8x16_reg_min_voltage[] = {1000000};
> >>+char *proxy_8x16_clk_str[] = {"xo"};
> >>+char *active_8x16_clk_str[] = {"iface", "bus", "mem"};
> >>+
> >>+static const struct q6_rproc_res msm_8916_res = {
> >>+	.proxy_clks = proxy_8x16_clk_str,
> >>+	.proxy_clk_cnt = 1,
> >>+	.active_clks = active_8x16_clk_str,
> >>+	.active_clk_cnt = 3,
> >>+	.proxy_regs = proxy_8x16_reg_str,
> >>+	.active_regs = active_8x16_reg_str,
> >>+	.proxy_reg_action = (int **)proxy_8x16_reg_action,
> >>+	.proxy_reg_load = (int *)proxy_8x16_reg_load,
> >>+	.active_reg_action = (int **)active_8x16_reg_action,
> >>+	.active_reg_load = (int *)active_8x16_reg_load,
> >>+	.proxy_reg_voltage = (int *)proxy_8x16_reg_min_voltage,
> >>+	.active_reg_voltage = active_8x16_reg_min_voltage,
> >>+	.proxy_reg_cnt = 3,
> >>+	.active_reg_cnt = 1,
> >>+	.q6_reset_init = q6v5_init_reset,
> >>+	.q6_version = "v5",
> >>+	.q6_mba_image = "mba.b00",
> >q6v55 (msm8916) also uses mba.mbn, while q6v5 (msm8974) uses mba.b00.
> Again this is a case where compatible string can not help, we should have
> single compatible string i.e. "q6v5" for msm8916 and msm8974 since both are
> from same class of q6 chip with different version.
> so if we cant initialize mdt name based on compatible string alone.

msm-3.4, msm-3.10 and msm-3.18 states that they are not the same and as
such I don't think we have a problem.

The more I look at this, the more convinced I am that we got 8916 wrong,
i.e. we specified the wrong class and it just happens to work.

> >
> >>+};
> >>+
> >>+static const struct of_device_id q6_of_match[] = {
> >>+	{ .compatible = "qcom,q6v5-pil", .data = &msm_8916_res},
> >As I was looking in the CAF tree, I believe the correct version for 8916
> >is 5.5 and v5 was used in 8974.
> >
> >But I presume these versions are not strictly tied to certain platforms,
> >so please name the resource structs based on the q6v5 version, rather
> >than platforms.
> Yes, resource are tied to compatible and version of q6.
> have used compatible to initialize major resources but using DT specified
> version string for minor deviations where needed.
> Should this be fine?
> as far as version on 8916 and 8974 are concern they are as below.
> 
> msm8974 q6v5 core version  5.0.0
> msm8916 q6v5 core version  5.1.1

If there are differences within a class then that just forces us to use
the version number. There's very little overhead in carrying one
compatible per platform, if that's what we need.

> >
> >>+	{ .compatible = "qcom,q6v56-pil", .data = &msm_8996_res},
> >>  	{ },
> >>  };

Regards,
Bjorn

Re: [PATCH v3 2/3] remoteproc: qcom: Hexagon resource handling

[Bjorn Andersson]

On Wed 16 Nov 07:17 PST 2016, Avaneesh Kumar Dwivedi wrote:

> 
> 
> On 11/8/2016 12:19 PM, Bjorn Andersson wrote:
> >On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote:
> >
> >>Handling of clock and regulator resources as well as reset
> >>register programing differ according to version of hexagon
> >>dsp hardware. Different version require different resources
> >>and different parameters for same resource. Hence it is
> >>needed that resource handling is generic and independent of
> >>hexagon dsp version.
> >>
> >It would be much easier to review this if you didn't do all three
> >changes in the same patch, and at the same time changed the function
> >names. There's large parts of this patch where it's not obvious what the
> >actual changes are.
> 
> OK, have broken patches in very small set of function now.
> but patches has increased from 3 to 9.
> sorry for inconvenience caused.

I will have a look once we have agreed on below issues.

[..]
> >>+	struct regulator **active_regs;
> >Keeping these as statically sized arrays, potentially with unused
> >elements at the end removes the need for allocating the storage and the
> >double pointers.
> since i do not know how many resource of a particular type will be needed on
> new version of new class of hexagon that is why i am working with pointers.
> have removed many entries from above resource struct, it will lok much
> cleaner in next patchset.

Just pick the largest number we support right now and then if future
versions need more we increment that number.

> >
> >>  	struct completion start_done;
> >>  	struct completion stop_done;
> >>@@ -147,67 +150,245 @@ struct q6v5 {
> >>  	phys_addr_t mpss_reloc;
> >>  	void *mpss_region;
> >>  	size_t mpss_size;
> >>+	struct mutex q6_lock;
> >>+	bool proxy_unvote_reg;
> >>+	bool proxy_unvote_clk;
> >I still don't see the need for these 3 attributes.
> I agree, Have removed them.
> >
> >>  };
> >>-enum {
> >>-	Q6V5_SUPPLY_CX,
> >>-	Q6V5_SUPPLY_MX,
> >>-	Q6V5_SUPPLY_MSS,
> >>-	Q6V5_SUPPLY_PLL,
> >>-};
> >>+static int q6_regulator_init(struct q6v5 *qproc)
> >>+{
> >>+	struct regulator **reg_arr;
> >>+	int i;
> >>+
> >>+	if (qproc->q6_rproc_res->proxy_reg_cnt) {
> >If you keep proxy_regs and active_regs as arrays you don't need this
> >check.
> Agree, have removed check.
> >
> >>+		reg_arr = devm_kzalloc(qproc->dev,
> >>+		sizeof(reg_arr) * qproc->q6_rproc_res->proxy_reg_cnt,
> >>+		GFP_KERNEL);
> >>+
> >>+		for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
> >>+			reg_arr[i] = devm_regulator_get(qproc->dev,
> >>+			qproc->q6_rproc_res->proxy_regs[i]);
> >>+			if (IS_ERR(reg_arr[i]))
> >>+				return PTR_ERR(reg_arr[i]);
> >>+			qproc->proxy_regs = reg_arr;
> >>+		}
> >>+	}
> >>+
> >>+	if (qproc->q6_rproc_res->active_reg_cnt) {
> >>+		reg_arr = devm_kzalloc(qproc->dev,
> >>+		sizeof(reg_arr) * qproc->q6_rproc_res->active_reg_cnt,
> >>+		GFP_KERNEL);
> >>+
> >>+		for (i = 0; i < qproc->q6_rproc_res->active_reg_cnt; i++) {
> >>+			reg_arr[i] = devm_regulator_get(qproc->dev,
> >>+			qproc->q6_rproc_res->active_regs[i]);
> >>+
> >>+			if (IS_ERR(reg_arr[i]))
> >>+				return PTR_ERR(reg_arr[i]);
> >>+			qproc->active_regs = reg_arr;
> >>+		}
> >>+	}
> >Please keep active_regs and proxy_regs as regulator_bulk_data and
> >continue to use devm_regulator_bulk_get(), it should make this code
> >cleaner.
> the way i have reorganized code in next patchset i found using
> devm_regulator_get() more convenient, can i keep using them? as i am reading
> string one by one and based on read string filling a regulator struct with
> its voltage and load and handle info.

If it's cleaner, then sure.

> >
> >>+
> >>+	return 0;
> >>+}
> >>-static int q6v5_regulator_init(struct q6v5 *qproc)
> >>+static int q6_proxy_regulator_enable(struct q6v5 *qproc)
> >>  {
> >>-	int ret;
> >>+	int i, j, ret = 0;
> >>+	int **reg_loadnvoltsetflag;
> >>+	int *reg_load;
> >>+	int *reg_voltage;
> >>+
> >>+	reg_loadnvoltsetflag = qproc->q6_rproc_res->proxy_reg_action;
> >>+	reg_load = qproc->q6_rproc_res->proxy_reg_load;
> >>+	reg_voltage = qproc->q6_rproc_res->proxy_reg_voltage;
> >Rather then keeping these properties on int-arrays I strongly prefer
> >that you would have a struct { uV, uA } for each regulator.
> Have modified as per suggestion.
> >
> >>+
> >>+	for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
> >>+		for (j = 0; j <= 1; j++) {
> >>+			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
> >I'm sorry, but this is not clean. Please use the fact that we're not
> >writing assembly code and use the language to your advantage.
> Sorry for mess, have simplified and cleaned.
> >
> >>+				regulator_set_load(qproc->proxy_regs[i],
> >>+				reg_load[i]);
> >>+			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
> >>+				regulator_set_voltage(qproc->proxy_regs[i],
> >>+				reg_voltage[i], INT_MAX);
> >>+		}
> >>+	}
> >>-	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";
> >>+	for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
> >>+		ret = regulator_enable(qproc->proxy_regs[i]);
> >>+		if (ret) {
> >>+			for (; i > 0; --i) {
> >>+				regulator_disable(qproc->proxy_regs[i]);
> >>+				return ret;
> >>+			}
> >>+		}
> >>+	}
> >If you just keep your regulators in a regulator_bulk_data array then you
> >can replace this with regulator_bulk_enable(proxy_reg_cnt, proxy_regs);
> As replied above i am going with getting sigle regulator handle one time.
> let me know if i can continue or need to change?
> 

The reason for using the bulk operations is that the error path becomes
cleaner, however now that I look at this again; in the event of an error
you leave the regulators with voltage and load specified. You need to
unroll this too.

But I would still prefer that you specify the loads & voltages, then
call bulk_enable() and if that fail remove all load and voltage
requests.

> >
> >>-	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;
[..]
> >>+	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56")) {
> >This would be much better as an enum than a string. But I keep wonder if
> >this is only for v5.6 of the Hexagon - perhaps should we clamp different
> >things on the various versions?.
> 
> As replied elsewhere, we need a DT entry to know which version is running,
> or else many compatible string will be required. for "v56" there are
> following version, so as and when we need to support a new version we will
> require
> a new DT entry which when defined will help to take deviation where
> required.
> 1.10.0
> 1.3.0
> 1.4.0
> 1.5.0
> 1.6.0
> 1.8.0
> 

Sorry for not seeing this before I answered in the two other places,
perhaps we should just discuss this to end in one place...

But regarding my specific comment, if you want class based handling then
introduce:

enum {
	Q6V5_CLASS5,
	Q6V5_CLASS55,
	Q5V5_CLASS56
};

Then you don't have to use strcmp() to check which class you have.

> >
> >>+		/*
> >>+		 * 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.
> >>+		 */
> >>+
> >>+		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);
> >>+		pil_mss_restart_reg(qproc, 1);
> >And by using the reset framework for mss_restart this will fall out of
> >the conditional segment and the else is gone.
> As i informed MSS RESET REGISTER was never a block control reset or BCR (a
> term used to define those reset register which control a clock or pll ) so
> clock control reset framework can not be used to do reset programming for
> MSS

But MSS RESET is a "reset" and far as this driver is concerned it should
be abstracted by the help of the reset framework. I don't want this
driver to care about the workings of the reset control.

The peripheral resets are part of the GCC block and as such I do not see
the problem with having the driver for the GCC block expose these
resets, even if though it's not a BCR - and this is how we have done it
on 8960, 8974 and 8084 so far.

> that is why i have adopted IOREMAP based mss reset programming. it is like
> any other register, may i know if any serious objection on using reset
> controller framework only? i will have to add another dummy driver just to
> do reset register programming.
> let me know please if it is mandatory?

I want this driver to consume a reset from a reset-controller, I do not
see the technical reason why we cannot just add this to the driver for
the GCC block.

Regards,
Bjorn

Re: [PATCH v3 1/3] remoteproc: qcom: Embed private data structure for hexagon dsp.

[Avaneesh Kumar Dwivedi]

On 11/19/2016 12:27 AM, Bjorn Andersson wrote:
> On Wed 16 Nov 06:41 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> i have been a little delayed for posting replies to patch comments,
>> hopefully onward it should be quick and fast.
>>
> I would greatly appreciate if you allow for a discussion before posting
> new revisions of the patchset. I will respond to your comments here and
> ignore v4 for now.

Ok, i will resend v4 patches with modification after consensus.
>
>> On 11/8/2016 12:38 PM, Bjorn Andersson wrote:
>>> On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote:
> [..]
>>>> +char *active_8x96_clk_str[] = {"iface", "bus", "mem", "gpll0_mss_clk",
>>>> +		"snoc_axi_clk", "mnoc_axi_clk"};
>>> All these needs to be static, but I would prefer if you just put the
>>> values directly into the resource structs below.
>> If i have to initialize directly into resource struct, i need to declare
>> individual elements as array of fixed size
>> but number of resource lets say  number of proxy regulator not being same
>> for different q6 chips, made me to
>> work with double pointer which can be assigned with address of an array of
>> string pointer as per need when new version need to be supported.
>>
> Using a termination sentinel to indicate end of lists is quite common in
> the kernel, so you can do this:
>
> 	.active_clks = (char*[]){
> 		"iface",
> 		"bus",
> 		...,
> 		NULL
> 	},
OK
>
>> Though i have made above elements as static in next patch.
>>>> +
>>>> +static const struct q6_rproc_res msm_8996_res = {
>>>> +	.proxy_clks = proxy_8x96_clk_str,
>>>> +	.proxy_clk_cnt = 3,
>>> It's common practice to use a NULL terminator in the definition list
>>> rather than keeping separate count of the number of items.
>>>
>>> While acquiring the resources you would have to "calculate" the number
>>> and store it in the q6v5 struct, but this would make turn this struct
>>> into something only used during probe() - which is nice.
>> Yes, have modified as per suggestion.
>>>> +	.active_clks = active_8x96_clk_str,
>>>> +	.active_clk_cnt = 6,
>>>> +	.proxy_regs = proxy_8x96_reg_str,
>>>> +	.active_regs = NULL,
>>>> +	.proxy_reg_action = (int **)proxy_8x96_reg_action,
>>>> +	.proxy_reg_load = (int *)proxy_8x96_reg_load,
>>>> +	.active_reg_action = NULL,
>>>> +	.active_reg_load = NULL,
>>>> +	.proxy_reg_voltage = (int *)proxy_8x96_reg_min_voltage,
>>>> +	.active_reg_voltage = NULL,
>>>> +	.proxy_reg_cnt = 3,
>>>> +	.active_reg_cnt = 0,
>>>> +	.q6_reset_init = q6v56_init_reset,
>>>> +	.q6_version = "v56",
>>> q6_version would be better to have as a enum.
>> have removed this entry.
>> each class of q6 lets say "v56" have again many version of hexagon with
>> minor differences wrt each other.
> Okay, I'm fine with us sticking to classes, but I would like for them to
> make sense - and be listed as an enum instead of a string, to simplify
> the code.
OK, i will use one compatible string for each msm platform with enum 
denoting the class and version of hexagon chip.  something like

.compatible = "qcom,q6v56-1-5-pil", for msm 8996?
and version will carry a unique enum value based on compatible.


>
>> for example msm8996 use "v56" 1.5.0 while MSM8952 uses 1.8.0, reset sequence
> In msm-3.18 8996 is listed to be v55.
The version and class that i am referring are strictly as per HPG.
for 8996,  hexagon core for MSS is "v56" with version 1.5.0
>
>> and some other operation differ wrt to this version in terms of order of
>> register programming. so i have introduced one variable in q6v5 struct per
>> q6 chip supported, if this is defined then we can check and carry out
>> version specific instruction.
>> will this be OK?
>>
> Generally in the Linux kernel it's frowned upon to carry the version
> information and then do conditional operation on this.
OK
>
> It's preferred to carry explicit flags through the implementation, e.g.
> carrying "mba.mbn" vs "mba.b00" rather than switching based on "version"
> at the point of use of this data.
If i have one enum for each version (which will require one compatible 
for each platform) then i can easily pass firmware binary name to probe.
> But I'm not sure if the other differences has reasonable names, e.g. how
> to we denote the differences in reset sequence?
  i have one option that should initialize one function pointer for each 
compatible to perform reset sequence, but this way there will be huge 
duplicity of code as more than 50% code in reset sequence remain same, 
else based on check something like below i can perform certain unique 
register operations for a particular hexagon core.

if (drvdata->hexagon_flag & 0x2)
     do this
else
     do that

where hexagon_flag will be initialized with enum based on compatible.??
>
>>>> +	.q6_mba_image = "mba.mbn",
>>>> +};
>>>> +
>>>> +char *proxy_8x16_reg_str[] = {"mx", "cx", "pll"};
>>>> +char *active_8x16_reg_str[] = {"mss"};
>>>> +int  proxy_8x16_reg_action[4][2] = { {0, 1}, {1, 0}, {1, 0} };
>>>> +int  active_8x16_reg_action[1][2] = { {1, 1} };
>>>> +int  proxy_8x16_reg_load[] = {100000, 0, 100000, 100000};
>>>> +int  active_8x16_reg_load[] = {100000};
>>>> +int  proxy_8x16_reg_min_voltage[] = {1050000, 0, 0};
>>>> +int  active_8x16_reg_min_voltage[] = {1000000};
>>>> +char *proxy_8x16_clk_str[] = {"xo"};
>>>> +char *active_8x16_clk_str[] = {"iface", "bus", "mem"};
>>>> +
>>>> +static const struct q6_rproc_res msm_8916_res = {
>>>> +	.proxy_clks = proxy_8x16_clk_str,
>>>> +	.proxy_clk_cnt = 1,
>>>> +	.active_clks = active_8x16_clk_str,
>>>> +	.active_clk_cnt = 3,
>>>> +	.proxy_regs = proxy_8x16_reg_str,
>>>> +	.active_regs = active_8x16_reg_str,
>>>> +	.proxy_reg_action = (int **)proxy_8x16_reg_action,
>>>> +	.proxy_reg_load = (int *)proxy_8x16_reg_load,
>>>> +	.active_reg_action = (int **)active_8x16_reg_action,
>>>> +	.active_reg_load = (int *)active_8x16_reg_load,
>>>> +	.proxy_reg_voltage = (int *)proxy_8x16_reg_min_voltage,
>>>> +	.active_reg_voltage = active_8x16_reg_min_voltage,
>>>> +	.proxy_reg_cnt = 3,
>>>> +	.active_reg_cnt = 1,
>>>> +	.q6_reset_init = q6v5_init_reset,
>>>> +	.q6_version = "v5",
>>>> +	.q6_mba_image = "mba.b00",
>>> q6v55 (msm8916) also uses mba.mbn, while q6v5 (msm8974) uses mba.b00.
>> Again this is a case where compatible string can not help, we should have
>> single compatible string i.e. "q6v5" for msm8916 and msm8974 since both are
>> from same class of q6 chip with different version.
>> so if we cant initialize mdt name based on compatible string alone.
> msm-3.4, msm-3.10 and msm-3.18 states that they are not the same and as
> such I don't think we have a problem.
>
> The more I look at this, the more convinced I am that we got 8916 wrong,
> i.e. we specified the wrong class and it just happens to work.
>
>>>> +};
>>>> +
>>>> +static const struct of_device_id q6_of_match[] = {
>>>> +	{ .compatible = "qcom,q6v5-pil", .data = &msm_8916_res},
>>> As I was looking in the CAF tree, I believe the correct version for 8916
>>> is 5.5 and v5 was used in 8974.
>>>
>>> But I presume these versions are not strictly tied to certain platforms,
>>> so please name the resource structs based on the q6v5 version, rather
>>> than platforms.
>> Yes, resource are tied to compatible and version of q6.
>> have used compatible to initialize major resources but using DT specified
>> version string for minor deviations where needed.
>> Should this be fine?
>> as far as version on 8916 and 8974 are concern they are as below.
>>
>> msm8974 q6v5 core version  5.0.0
>> msm8916 q6v5 core version  5.1.1
> If there are differences within a class then that just forces us to use
> the version number. There's very little overhead in carrying one
> compatible per platform, if that's what we need.

Yes i will use one compatible per msm platform.
>
>>>> +	{ .compatible = "qcom,q6v56-pil", .data = &msm_8996_res},
>>>>   	{ },
>>>>   };
> Regards,
> Bjorn

Re: [PATCH v3 2/3] remoteproc: qcom: Hexagon resource handling

[Avaneesh Kumar Dwivedi]

On 11/19/2016 1:00 AM, Bjorn Andersson wrote:
> On Wed 16 Nov 07:17 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>>
>> On 11/8/2016 12:19 PM, Bjorn Andersson wrote:
>>> On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote:
>>>
>>>> Handling of clock and regulator resources as well as reset
>>>> register programing differ according to version of hexagon
>>>> dsp hardware. Different version require different resources
>>>> and different parameters for same resource. Hence it is
>>>> needed that resource handling is generic and independent of
>>>> hexagon dsp version.
>>>>
>>> It would be much easier to review this if you didn't do all three
>>> changes in the same patch, and at the same time changed the function
>>> names. There's large parts of this patch where it's not obvious what the
>>> actual changes are.
>> OK, have broken patches in very small set of function now.
>> but patches has increased from 3 to 9.
>> sorry for inconvenience caused.
> I will have a look once we have agreed on below issues.
>
> [..]
>>>> +	struct regulator **active_regs;
>>> Keeping these as statically sized arrays, potentially with unused
>>> elements at the end removes the need for allocating the storage and the
>>> double pointers.
>> since i do not know how many resource of a particular type will be needed on
>> new version of new class of hexagon that is why i am working with pointers.
>> have removed many entries from above resource struct, it will lok much
>> cleaner in next patchset.
> Just pick the largest number we support right now and then if future
> versions need more we increment that number.
OK
>
>>>>   	struct completion start_done;
>>>>   	struct completion stop_done;
>>>> @@ -147,67 +150,245 @@ struct q6v5 {
>>>>   	phys_addr_t mpss_reloc;
>>>>   	void *mpss_region;
>>>>   	size_t mpss_size;
>>>> +	struct mutex q6_lock;
>>>> +	bool proxy_unvote_reg;
>>>> +	bool proxy_unvote_clk;
>>> I still don't see the need for these 3 attributes.
>> I agree, Have removed them.
>>>>   };
>>>> -enum {
>>>> -	Q6V5_SUPPLY_CX,
>>>> -	Q6V5_SUPPLY_MX,
>>>> -	Q6V5_SUPPLY_MSS,
>>>> -	Q6V5_SUPPLY_PLL,
>>>> -};
>>>> +static int q6_regulator_init(struct q6v5 *qproc)
>>>> +{
>>>> +	struct regulator **reg_arr;
>>>> +	int i;
>>>> +
>>>> +	if (qproc->q6_rproc_res->proxy_reg_cnt) {
>>> If you keep proxy_regs and active_regs as arrays you don't need this
>>> check.
>> Agree, have removed check.
>>>> +		reg_arr = devm_kzalloc(qproc->dev,
>>>> +		sizeof(reg_arr) * qproc->q6_rproc_res->proxy_reg_cnt,
>>>> +		GFP_KERNEL);
>>>> +
>>>> +		for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
>>>> +			reg_arr[i] = devm_regulator_get(qproc->dev,
>>>> +			qproc->q6_rproc_res->proxy_regs[i]);
>>>> +			if (IS_ERR(reg_arr[i]))
>>>> +				return PTR_ERR(reg_arr[i]);
>>>> +			qproc->proxy_regs = reg_arr;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (qproc->q6_rproc_res->active_reg_cnt) {
>>>> +		reg_arr = devm_kzalloc(qproc->dev,
>>>> +		sizeof(reg_arr) * qproc->q6_rproc_res->active_reg_cnt,
>>>> +		GFP_KERNEL);
>>>> +
>>>> +		for (i = 0; i < qproc->q6_rproc_res->active_reg_cnt; i++) {
>>>> +			reg_arr[i] = devm_regulator_get(qproc->dev,
>>>> +			qproc->q6_rproc_res->active_regs[i]);
>>>> +
>>>> +			if (IS_ERR(reg_arr[i]))
>>>> +				return PTR_ERR(reg_arr[i]);
>>>> +			qproc->active_regs = reg_arr;
>>>> +		}
>>>> +	}
>>> Please keep active_regs and proxy_regs as regulator_bulk_data and
>>> continue to use devm_regulator_bulk_get(), it should make this code
>>> cleaner.
>> the way i have reorganized code in next patchset i found using
>> devm_regulator_get() more convenient, can i keep using them? as i am reading
>> string one by one and based on read string filling a regulator struct with
>> its voltage and load and handle info.
> If it's cleaner, then sure
OK
>
>>>> +
>>>> +	return 0;
>>>> +}
>>>> -static int q6v5_regulator_init(struct q6v5 *qproc)
>>>> +static int q6_proxy_regulator_enable(struct q6v5 *qproc)
>>>>   {
>>>> -	int ret;
>>>> +	int i, j, ret = 0;
>>>> +	int **reg_loadnvoltsetflag;
>>>> +	int *reg_load;
>>>> +	int *reg_voltage;
>>>> +
>>>> +	reg_loadnvoltsetflag = qproc->q6_rproc_res->proxy_reg_action;
>>>> +	reg_load = qproc->q6_rproc_res->proxy_reg_load;
>>>> +	reg_voltage = qproc->q6_rproc_res->proxy_reg_voltage;
>>> Rather then keeping these properties on int-arrays I strongly prefer
>>> that you would have a struct { uV, uA } for each regulator.
>> Have modified as per suggestion.
>>>> +
>>>> +	for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
>>>> +		for (j = 0; j <= 1; j++) {
>>>> +			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
>>> I'm sorry, but this is not clean. Please use the fact that we're not
>>> writing assembly code and use the language to your advantage.
>> Sorry for mess, have simplified and cleaned.
>>>> +				regulator_set_load(qproc->proxy_regs[i],
>>>> +				reg_load[i]);
>>>> +			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
>>>> +				regulator_set_voltage(qproc->proxy_regs[i],
>>>> +				reg_voltage[i], INT_MAX);
>>>> +		}
>>>> +	}
>>>> -	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";
>>>> +	for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
>>>> +		ret = regulator_enable(qproc->proxy_regs[i]);
>>>> +		if (ret) {
>>>> +			for (; i > 0; --i) {
>>>> +				regulator_disable(qproc->proxy_regs[i]);
>>>> +				return ret;
>>>> +			}
>>>> +		}
>>>> +	}
>>> If you just keep your regulators in a regulator_bulk_data array then you
>>> can replace this with regulator_bulk_enable(proxy_reg_cnt, proxy_regs);
>> As replied above i am going with getting sigle regulator handle one time.
>> let me know if i can continue or need to change?
>>
> The reason for using the bulk operations is that the error path becomes
> cleaner, however now that I look at this again; in the event of an error
> you leave the regulators with voltage and load specified. You need to
> unroll this too.
if regulator enabled failed, i am unrolling the programmed load and 
voltage setting.
but i will try to incorporate your suggestion.
>
> But I would still prefer that you specify the loads & voltages, then
> call bulk_enable() and if that fail remove all load and voltage
> requests.
OK, will try to incorporate.
>
>>>> -	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;
> [..]
>>>> +	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56")) {
>>> This would be much better as an enum than a string. But I keep wonder if
>>> this is only for v5.6 of the Hexagon - perhaps should we clamp different
>>> things on the various versions?.
>> As replied elsewhere, we need a DT entry to know which version is running,
>> or else many compatible string will be required. for "v56" there are
>> following version, so as and when we need to support a new version we will
>> require
>> a new DT entry which when defined will help to take deviation where
>> required.
>> 1.10.0
>> 1.3.0
>> 1.4.0
>> 1.5.0
>> 1.6.0
>> 1.8.0
>>
> Sorry for not seeing this before I answered in the two other places,
> perhaps we should just discuss this to end in one place...
>
> But regarding my specific comment, if you want class based handling then
> introduce:
>
> enum {
> 	Q6V5_CLASS5,
> 	Q6V5_CLASS55,
> 	Q5V5_CLASS56
> };
>
> Then you don't have to use strcmp() to check which class you have.
OK, may i change enum strings as per HPG?

enum {
	Q6V5_5_0_0,--->8916
	Q6V5_5_1_1,---->8974
	Q5V56_1_5_0---->8996
};



>
>>>> +		/*
>>>> +		 * 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.
>>>> +		 */
>>>> +
>>>> +		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);
>>>> +		pil_mss_restart_reg(qproc, 1);
>>> And by using the reset framework for mss_restart this will fall out of
>>> the conditional segment and the else is gone.
>> As i informed MSS RESET REGISTER was never a block control reset or BCR (a
>> term used to define those reset register which control a clock or pll ) so
>> clock control reset framework can not be used to do reset programming for
>> MSS
> But MSS RESET is a "reset" and far as this driver is concerned it should
> be abstracted by the help of the reset framework. I don't want this
> driver to care about the workings of the reset control.
>
> The peripheral resets are part of the GCC block and as such I do not see
> the problem with having the driver for the GCC block expose these
> resets, even if though it's not a BCR - and this is how we have done it
> on 8960, 8974 and 8084 so far.
I was under impression that clock code will be up streamed by clock 
team, and they will go by downstream way.
i discussed again they said i can use GCC reset framework in upstream.
Will change patch accordingly.
>
>> that is why i have adopted IOREMAP based mss reset programming. it is like
>> any other register, may i know if any serious objection on using reset
>> controller framework only? i will have to add another dummy driver just to
>> do reset register programming.
>> let me know please if it is mandatory?
> I want this driver to consume a reset from a reset-controller, I do not
> see the technical reason why we cannot just add this to the driver for
> the GCC block.
Sure, will do.
>
> Regards,
> Bjorn