[PATCH V6 0/5] Add support for Hexagon q6v5-wcss integrated core

[PATCH V6 0/5] Add support for Hexagon q6v5-wcss integrated core

[Sricharan R]

IPQ8074 has an integrated Hexagon dsp core Q6v5 and a wireless lan
(Lithium) IP. This series adds the remoteproc driver to reset, load
and boot Q6 firmware.

The first patch is to make the mdt_loader authenticate
the firmware only if required, so that the code can be reused for
self-authenticating firmware like the Q6v5 core in IPQ8074. The second
patch exports the elf header's get_boot_addr helper to reuse it.
The next couple of patches arranges the code in the original q6v5-mpss
rproc to add q6v5-wcss later. The last couple of patches add the relevant
bits for the q6v5-wcss core.

This is done on top of Avaneesh's msm8996 rproc support [1]

[1] https://lkml.org/lkml/2017/10/24/771

V6:
   Rebased on top of,
   https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1674300.html

   Dropped remoteproc: Export rproc_elf_get_boot_addr, as it is already merged
   by a different patch from Bjorn.

   Minor change to the q6 power down sequence.

v5:
    No change. Just updated tags in PATCH #5

V4:
    Fixed Bjorn's comment in PATCH#1 and added his acked-by
    Rebased on top of Avinash's latest rproc for msm8996 q6 support.

V3:
    Rebased on top of latest remoteproc next

V2:
    Last time introduced this a new rproc driver, but there is lot
    of code that can be shared if it is added to the q6v5-mpss pil
    driver.

Sricharan R (5):
  remoteproc: qcom: mdt_loader: Make the firmware authentication
    optional
  remoteproc: qcom: Push reset ops, rproc ops in to of_match data
  remoteproc: qcom: Split the head and tail of the q5v5-pil rproc reset
    function
  remoteproc: qcom: Add support for q6v5-wcss pil
  remoteproc: qcom: Add q6v5-wcss rproc ops

 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   7 +-
 drivers/remoteproc/Kconfig                         |   1 +
 drivers/remoteproc/qcom_q6v5_pil.c                 | 468 +++++++++++++++++----
 drivers/soc/qcom/mdt_loader.c                      |  87 ++--
 include/linux/soc/qcom/mdt_loader.h                |   4 +
 5 files changed, 450 insertions(+), 117 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH V6 1/5] remoteproc: qcom: mdt_loader: Make the firmware authentication optional

[Sricharan R]

qcom_mdt_load function loads the mdt type firmware and
initialises the secure memory as well. Make the initialisation only
when requested by the caller, so that the function can be used
by self-authenticating remoteproc as well.

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/soc/qcom/mdt_loader.c       | 87 ++++++++++++++++++++++++++-----------
 include/linux/soc/qcom/mdt_loader.h |  4 ++
 2 files changed, 66 insertions(+), 25 deletions(-)

diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 17b314d..3747487 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -74,23 +74,10 @@ ssize_t qcom_mdt_get_size(const struct firmware *fw)
 }
 EXPORT_SYMBOL_GPL(qcom_mdt_get_size);
 
-/**
- * qcom_mdt_load() - load the firmware which header is loaded as fw
- * @dev:	device handle to associate resources with
- * @fw:		firmware object for the mdt file
- * @firmware:	name of the firmware, for construction of segment file names
- * @pas_id:	PAS identifier
- * @mem_region:	allocated memory region to load firmware into
- * @mem_phys:	physical address of allocated memory region
- * @mem_size:	size of the allocated memory region
- * @reloc_base:	adjusted physical address after relocation
- *
- * Returns 0 on success, negative errno otherwise.
- */
-int qcom_mdt_load(struct device *dev, const struct firmware *fw,
-		  const char *firmware, int pas_id, void *mem_region,
-		  phys_addr_t mem_phys, size_t mem_size,
-		  phys_addr_t *reloc_base)
+static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
+			   const char *firmware, int pas_id, void *mem_region,
+			   phys_addr_t mem_phys, size_t mem_size,
+			   phys_addr_t *reloc_base, bool pas_init)
 {
 	const struct elf32_phdr *phdrs;
 	const struct elf32_phdr *phdr;
@@ -121,10 +108,12 @@ int qcom_mdt_load(struct device *dev, const struct firmware *fw,
 	if (!fw_name)
 		return -ENOMEM;
 
-	ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size);
-	if (ret) {
-		dev_err(dev, "invalid firmware metadata\n");
-		goto out;
+	if (pas_init) {
+		ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size);
+		if (ret) {
+			dev_err(dev, "invalid firmware metadata\n");
+			goto out;
+		}
 	}
 
 	for (i = 0; i < ehdr->e_phnum; i++) {
@@ -144,10 +133,13 @@ int qcom_mdt_load(struct device *dev, const struct firmware *fw,
 	}
 
 	if (relocate) {
-		ret = qcom_scm_pas_mem_setup(pas_id, mem_phys, max_addr - min_addr);
-		if (ret) {
-			dev_err(dev, "unable to setup relocation\n");
-			goto out;
+		if (pas_init) {
+			ret = qcom_scm_pas_mem_setup(pas_id, mem_phys,
+						     max_addr - min_addr);
+			if (ret) {
+				dev_err(dev, "unable to setup relocation\n");
+				goto out;
+			}
 		}
 
 		/*
@@ -202,7 +194,52 @@ int qcom_mdt_load(struct device *dev, const struct firmware *fw,
 
 	return ret;
 }
+
+/**
+ * qcom_mdt_load() - load the firmware which header is loaded as fw
+ * @dev:	device handle to associate resources with
+ * @fw:		firmware object for the mdt file
+ * @firmware:	name of the firmware, for construction of segment file names
+ * @pas_id:	PAS identifier
+ * @mem_region:	allocated memory region to load firmware into
+ * @mem_phys:	physical address of allocated memory region
+ * @mem_size:	size of the allocated memory region
+ * @reloc_base:	adjusted physical address after relocation
+ *
+ * Returns 0 on success, negative errno otherwise.
+ */
+int qcom_mdt_load(struct device *dev, const struct firmware *fw,
+		  const char *firmware, int pas_id, void *mem_region,
+		  phys_addr_t mem_phys, size_t mem_size,
+		  phys_addr_t *reloc_base)
+{
+	return __qcom_mdt_load(dev, fw, firmware, pas_id, mem_region, mem_phys,
+			       mem_size, reloc_base, true);
+}
 EXPORT_SYMBOL_GPL(qcom_mdt_load);
 
+/**
+ * qcom_mdt_load_no_init() - load the firmware which header is loaded as fw
+ * @dev:	device handle to associate resources with
+ * @fw:		firmware object for the mdt file
+ * @firmware:	name of the firmware, for construction of segment file names
+ * @pas_id:	PAS identifier
+ * @mem_region:	allocated memory region to load firmware into
+ * @mem_phys:	physical address of allocated memory region
+ * @mem_size:	size of the allocated memory region
+ * @reloc_base:	adjusted physical address after relocation
+ *
+ * Returns 0 on success, negative errno otherwise.
+ */
+int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
+			  const char *firmware, int pas_id,
+			  void *mem_region, phys_addr_t mem_phys,
+			  size_t mem_size, phys_addr_t *reloc_base)
+{
+	return __qcom_mdt_load(dev, fw, firmware, pas_id, mem_region, mem_phys,
+			       mem_size, reloc_base, false);
+}
+EXPORT_SYMBOL_GPL(qcom_mdt_load_no_init);
+
 MODULE_DESCRIPTION("Firmware parser for Qualcomm MDT format");
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
index 5b98bbd..944b06a 100644
--- a/include/linux/soc/qcom/mdt_loader.h
+++ b/include/linux/soc/qcom/mdt_loader.h
@@ -17,4 +17,8 @@ int qcom_mdt_load(struct device *dev, const struct firmware *fw,
 		  phys_addr_t mem_phys, size_t mem_size,
 		  phys_addr_t *reloc_base);
 
+int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
+			  const char *fw_name, int pas_id, void *mem_region,
+			  phys_addr_t mem_phys, size_t mem_size,
+			  phys_addr_t *reloc_base);
 #endif
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH V6 2/5] remoteproc: qcom: Push reset ops, rproc ops in to of_match data

[Sricharan R]

Instead of directly assigning reset and rproc ops, put them
in to of_match data and get from that. Currently same ops
are used for all compatibles, but that will change when we add
q6v5-wcss support.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index a3d65a7..cc26cab 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -125,18 +125,6 @@ struct qcom_mss_reg_res {
 	int uA;
 };
 
-struct rproc_hexagon_res {
-	const char *hexagon_mba_image;
-	struct qcom_mss_reg_res *proxy_supply;
-	struct qcom_mss_reg_res *active_supply;
-	char **proxy_clk_names;
-	char **reset_clk_names;
-	char **active_clk_names;
-	int version;
-	bool need_mem_protection;
-	bool has_alt_reset;
-};
-
 struct q6v5 {
 	struct device *dev;
 	struct rproc *rproc;
@@ -197,6 +185,20 @@ struct q6v5 {
 	int version;
 };
 
+struct rproc_hexagon_res {
+	const char *hexagon_mba_image;
+	struct qcom_mss_reg_res *proxy_supply;
+	struct qcom_mss_reg_res *active_supply;
+	char **proxy_clk_names;
+	char **reset_clk_names;
+	char **active_clk_names;
+	int version;
+	bool need_mem_protection;
+	bool has_alt_reset;
+	int (*init_reset)(struct q6v5 *qproc);
+	const struct rproc_ops *ops;
+};
+
 enum {
 	MSS_MSM8916,
 	MSS_MSM8974,
@@ -1250,7 +1252,7 @@ static int q6v5_probe(struct platform_device *pdev)
 	if (!desc)
 		return -EINVAL;
 
-	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops,
+	rproc = rproc_alloc(&pdev->dev, pdev->name, desc->ops,
 			    desc->hexagon_mba_image, sizeof(*qproc));
 	if (!rproc) {
 		dev_err(&pdev->dev, "failed to allocate rproc\n");
@@ -1321,7 +1323,7 @@ static int q6v5_probe(struct platform_device *pdev)
 	}
 	qproc->active_reg_count = ret;
 
-	ret = q6v5_init_reset(qproc);
+	ret = desc->init_reset(qproc);
 	if (ret)
 		goto free_rproc;
 
@@ -1417,6 +1419,8 @@ static int q6v5_remove(struct platform_device *pdev)
 	.need_mem_protection = true,
 	.has_alt_reset = true,
 	.version = MSS_SDM845,
+	.init_reset = q6v5_init_reset,
+	.ops = &q6v5_ops,
 };
 
 static const struct rproc_hexagon_res msm8996_mss = {
@@ -1436,6 +1440,8 @@ static int q6v5_remove(struct platform_device *pdev)
 	.need_mem_protection = true,
 	.has_alt_reset = false,
 	.version = MSS_MSM8996,
+	.init_reset = q6v5_init_reset,
+	.ops = &q6v5_ops,
 };
 
 static const struct rproc_hexagon_res msm8916_mss = {
@@ -1468,6 +1474,8 @@ static int q6v5_remove(struct platform_device *pdev)
 	.need_mem_protection = false,
 	.has_alt_reset = false,
 	.version = MSS_MSM8916,
+	.init_reset = q6v5_init_reset,
+	.ops = &q6v5_ops,
 };
 
 static const struct rproc_hexagon_res msm8974_mss = {
@@ -1508,6 +1516,8 @@ static int q6v5_remove(struct platform_device *pdev)
 	.need_mem_protection = false,
 	.has_alt_reset = false,
 	.version = MSS_MSM8974,
+	.init_reset = q6v5_init_reset,
+	.ops = &q6v5_ops,
 };
 
 static const struct of_device_id q6v5_of_match[] = {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH V6 3/5] remoteproc: qcom: Split the head and tail of the q5v5-pil rproc reset function

[Sricharan R]

Most of the q6v5-wcss reset function is same as MSM8996 reset sequence
that will be added later. So split and move out the common pieces
so that the same code can be reused.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 169 ++++++++++++++++++++-----------------
 1 file changed, 93 insertions(+), 76 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index cc26cab..2403bb2 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -430,11 +430,101 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
 	return val;
 }
 
+static int q6v5_reset(struct q6v5 *qproc)
+{
+	u32 ret;
+	int val, i;
+
+	/* Assert resets, stop core */
+	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
+	val |= Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE;
+	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
+
+	/* BHS require xo cbcr to be enabled */
+	val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
+	val |= 0x1;
+	writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
+
+	/* Read CLKOFF bit to go low indicating CLK is enabled */
+	ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
+				 val, !(val & BIT(31)), 1,
+				 HALT_CHECK_MAX_LOOPS);
+	if (ret) {
+		dev_err(qproc->dev,
+			"xo cbcr enabling timed out (rc:%d)\n", ret);
+		return ret;
+	}
+	/* Enable power block headswitch and wait for it to stabilize */
+	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	val |= QDSP6v56_BHS_ON;
+	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	udelay(1);
+
+	/* Put LDO in bypass mode */
+	val |= QDSP6v56_LDO_BYP;
+	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	/* Deassert QDSP6 compiler memory clamp */
+	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	val &= ~QDSP6v56_CLAMP_QMC_MEM;
+	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	/* Deassert memory peripheral sleep and L2 memory standby */
+	val |= Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N;
+	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	/* Turn on L1, L2, ETB and JU memories 1 at a time */
+	val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+	for (i = 19; i >= 0; i--) {
+		val |= BIT(i);
+		writel(val, qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+		/*
+		 * Read back value to ensure the write is done then
+		 * wait for 1us for both memory peripheral and data
+		 * array to turn on.
+		 */
+		val |= readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+		udelay(1);
+	}
+	/* Remove word line clamp */
+	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	val &= ~QDSP6v56_CLAMP_WL;
+	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	return 0;
+}
+
+static void q6v5_reset_rest(struct q6v5 *qproc)
+{
+	u32 val;
+
+	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	/* Remove IO clamp */
+	val &= ~Q6SS_CLAMP_IO;
+	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	/* Bring core out of reset */
+	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
+	val &= ~Q6SS_CORE_ARES;
+	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
+
+	/* Turn on core clock */
+	val = readl(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
+	val |= Q6SS_CLK_ENABLE;
+	writel(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
+
+	/* Start core execution */
+	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
+	val &= ~Q6SS_STOP_CORE;
+	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
+}
+
 static int q6v5proc_reset(struct q6v5 *qproc)
 {
 	u32 val;
 	int ret;
-	int i;
 
 	if (qproc->version == MSS_SDM845) {
 
@@ -470,64 +560,7 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 		/* Override the ACC value if required */
 		writel(QDSP6SS_ACC_OVERRIDE_VAL,
 		       qproc->reg_base + QDSP6SS_STRAP_ACC);
-
-		/* Assert resets, stop core */
-		val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
-		val |= Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE;
-		writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
-
-		/* BHS require xo cbcr to be enabled */
-		val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
-		val |= 0x1;
-		writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
-
-		/* Read CLKOFF bit to go low indicating CLK is enabled */
-		ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
-					 val, !(val & BIT(31)), 1,
-					 HALT_CHECK_MAX_LOOPS);
-		if (ret) {
-			dev_err(qproc->dev,
-				"xo cbcr enabling timed out (rc:%d)\n", ret);
-			return ret;
-		}
-		/* Enable power block headswitch and wait for it to stabilize */
-		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-		val |= QDSP6v56_BHS_ON;
-		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-		val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-		udelay(1);
-
-		/* Put LDO in bypass mode */
-		val |= QDSP6v56_LDO_BYP;
-		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-
-		/* Deassert QDSP6 compiler memory clamp */
-		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-		val &= ~QDSP6v56_CLAMP_QMC_MEM;
-		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-
-		/* Deassert memory peripheral sleep and L2 memory standby */
-		val |= Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N;
-		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-
-		/* Turn on L1, L2, ETB and JU memories 1 at a time */
-		val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
-		for (i = 19; i >= 0; i--) {
-			val |= BIT(i);
-			writel(val, qproc->reg_base +
-						QDSP6SS_MEM_PWR_CTL);
-			/*
-			 * Read back value to ensure the write is done then
-			 * wait for 1us for both memory peripheral and data
-			 * array to turn on.
-			 */
-			val |= readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
-			udelay(1);
-		}
-		/* Remove word line clamp */
-		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-		val &= ~QDSP6v56_CLAMP_WL;
-		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		q6v5_reset(qproc);
 	} else {
 		/* Assert resets, stop core */
 		val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
@@ -555,24 +588,8 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 		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);
-
-	/* Bring core out of reset */
-	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
-	val &= ~Q6SS_CORE_ARES;
-	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
-
-	/* Turn on core clock */
-	val = readl(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
-	val |= Q6SS_CLK_ENABLE;
-	writel(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
 
-	/* Start core execution */
-	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
-	val &= ~Q6SS_STOP_CORE;
-	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
+	q6v5_reset_rest(qproc);
 
 pbl_wait:
 	/* Wait for PBL status */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH V6 3/5] remoteproc: qcom: Split the head and tail of the q5v5-pil rproc reset function

[Vinod]

On 14-05-18, 16:16, Sricharan R wrote:

> +static int q6v5_reset(struct q6v5 *qproc)
> +{
> +	u32 ret;
> +	int val, i;
> +
> +	/* Assert resets, stop core */
> +	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
> +	val |= Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE;
> +	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
> +
> +	/* BHS require xo cbcr to be enabled */
> +	val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
> +	val |= 0x1;
> +	writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);

consider adding a updatel macro which does read, update and write for you...

> +
> +	/* Read CLKOFF bit to go low indicating CLK is enabled */
> +	ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
> +				 val, !(val & BIT(31)), 1,
> +				 HALT_CHECK_MAX_LOOPS);
> +	if (ret) {
> +		dev_err(qproc->dev,
> +			"xo cbcr enabling timed out (rc:%d)\n", ret);
> +		return ret;
> +	}
> +	/* Enable power block headswitch and wait for it to stabilize */
> +	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	val |= QDSP6v56_BHS_ON;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);

why is this read required

> +	udelay(1);
> +
> +	/* Put LDO in bypass mode */
> +	val |= QDSP6v56_LDO_BYP;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* Deassert QDSP6 compiler memory clamp */
> +	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	val &= ~QDSP6v56_CLAMP_QMC_MEM;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* Deassert memory peripheral sleep and L2 memory standby */
> +	val |= Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* Turn on L1, L2, ETB and JU memories 1 at a time */
> +	val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
> +	for (i = 19; i >= 0; i--) {

where is the magic number 19 coming from?
-- 
~Vinod

Re: [PATCH V6 3/5] remoteproc: qcom: Split the head and tail of the q5v5-pil rproc reset function

[Sricharan R]

Hi Vinod,

Thanks for the review.

On 5/18/2018 5:52 PM, Vinod wrote:
> On 14-05-18, 16:16, Sricharan R wrote:
> 
>> +static int q6v5_reset(struct q6v5 *qproc)
>> +{
>> +	u32 ret;
>> +	int val, i;
>> +
>> +	/* Assert resets, stop core */
>> +	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
>> +	val |= Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE;
>> +	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
>> +
>> +	/* BHS require xo cbcr to be enabled */
>> +	val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
>> +	val |= 0x1;
>> +	writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
> 
> consider adding a updatel macro which does read, update and write for you...
> 

 ok.

>> +
>> +	/* Read CLKOFF bit to go low indicating CLK is enabled */
>> +	ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
>> +				 val, !(val & BIT(31)), 1,
>> +				 HALT_CHECK_MAX_LOOPS);
>> +	if (ret) {
>> +		dev_err(qproc->dev,
>> +			"xo cbcr enabling timed out (rc:%d)\n", ret);
>> +		return ret;
>> +	}
>> +	/* Enable power block headswitch and wait for it to stabilize */
>> +	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	val |= QDSP6v56_BHS_ON;
>> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> 
> why is this read required
> 

 It was a ditto of what was given in the programming sequence from HW folks.
 yeah, logically the readl does not look needed. Will remove and update.

>> +	udelay(1);
>> +
>> +	/* Put LDO in bypass mode */
>> +	val |= QDSP6v56_LDO_BYP;
>> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +	/* Deassert QDSP6 compiler memory clamp */
>> +	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	val &= ~QDSP6v56_CLAMP_QMC_MEM;
>> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +	/* Deassert memory peripheral sleep and L2 memory standby */
>> +	val |= Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N;
>> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +	/* Turn on L1, L2, ETB and JU memories 1 at a time */
>> +	val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
>> +	for (i = 19; i >= 0; i--) {
> 
> where is the magic number 19 coming from?
> 

 Its the total number of Q6's memory head/foot switch banks. Infact
 the magic was there even before my patch. But will add a Macro to fix it.

Regards,
 Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH V6 4/5] remoteproc: qcom: Add support for q6v5-wcss pil

[Sricharan R]

IPQ8074 has an integrated Hexagon dsp core q6v5 and a wireless lan
(Lithium) IP. An mdt type single image format is used for the
firmware. So the mdt_load function can be directly used to load
the firmware. Also add the relevant resets required for this core.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |  7 +++-
 drivers/remoteproc/Kconfig                         |  1 +
 drivers/remoteproc/qcom_q6v5_pil.c                 | 37 +++++++++++++++++++++-
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index d901824..3a4a1a92 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -12,6 +12,7 @@ on the Qualcomm Hexagon core.
 		    "qcom,msm8974-mss-pil"
 		    "qcom,msm8996-mss-pil"
 		    "qcom,sdm845-mss-pil"
+		    "qcom,ipq8074-wcss-pil"
 
 - reg:
 	Usage: required
@@ -50,11 +51,15 @@ on the Qualcomm Hexagon core.
 	Usage: required
 	Value type: <phandle>
 	Definition: reference to the reset-controller for the modem sub-system
+		    reference to the list of 3 reset-controllers for the
+		    wcss sub-system
 
 - reset-names:
 	Usage: required
 	Value type: <stringlist>
-	Definition: must be "mss_restart"
+	Definition: must be "mss_restart" for the modem sub-system
+	Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
+		    for the wcss syb-system
 
 - cx-supply:
 - mss-supply:
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 0272740..4841420 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -113,6 +113,7 @@ config QCOM_Q6V5_PIL
 	select MFD_SYSCON
 	select QCOM_RPROC_COMMON
 	select QCOM_SCM
+	select QCOM_MDT_LOADER
 	help
 	  Say y here to support the Qualcomm Peripherial Image Loader for the
 	  Hexagon V5 based remote processors.
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 2403bb2..fbe179d 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -138,6 +138,9 @@ struct q6v5 {
 	u32 halt_nc;
 
 	struct reset_control *mss_restart;
+	struct reset_control *wcss_aon_reset;
+	struct reset_control *wcss_reset;
+	struct reset_control *wcss_q6_reset;
 
 	struct qcom_smem_state *state;
 	unsigned stop_bit;
@@ -204,6 +207,7 @@ enum {
 	MSS_MSM8974,
 	MSS_MSM8996,
 	MSS_SDM845,
+	WCSS_IPQ8074,
 };
 
 static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
@@ -1173,6 +1177,26 @@ static int q6v5_init_clocks(struct device *dev, struct clk **clks,
 	return i;
 }
 
+static int q6v5_wcss_init_reset(struct q6v5 *qproc)
+{
+	qproc->wcss_aon_reset = devm_reset_control_get(qproc->dev,
+						       "wcss_aon_reset");
+	if (IS_ERR(qproc->wcss_aon_reset))
+		return PTR_ERR(qproc->wcss_aon_reset);
+
+	qproc->wcss_reset = devm_reset_control_get(qproc->dev,
+						   "wcss_reset");
+	if (IS_ERR(qproc->wcss_reset))
+		return PTR_ERR(qproc->wcss_reset);
+
+	qproc->wcss_q6_reset = devm_reset_control_get(qproc->dev,
+						      "wcss_q6_reset");
+	if (IS_ERR(qproc->wcss_q6_reset))
+		return PTR_ERR(qproc->wcss_q6_reset);
+
+	return 0;
+}
+
 static int q6v5_init_reset(struct q6v5 *qproc)
 {
 	qproc->mss_restart = devm_reset_control_get_exclusive(qproc->dev,
@@ -1237,6 +1261,9 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
 		return -EBUSY;
 	}
 
+	if (qproc->version == WCSS_IPQ8074)
+		return 0;
+
 	child = of_get_child_by_name(qproc->dev->of_node, "mpss");
 	node = of_parse_phandle(child, "memory-region", 0);
 	ret = of_address_to_resource(node, 0, &r);
@@ -1279,6 +1306,7 @@ static int q6v5_probe(struct platform_device *pdev)
 	qproc = (struct q6v5 *)rproc->priv;
 	qproc->dev = &pdev->dev;
 	qproc->rproc = rproc;
+	qproc->version = desc->version;
 	platform_set_drvdata(pdev, qproc);
 
 	if (desc->has_alt_reset) {
@@ -1344,7 +1372,6 @@ static int q6v5_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
-	qproc->version = desc->version;
 	qproc->need_mem_protection = desc->need_mem_protection;
 	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt,
 			       &qproc->wdog_interrupt);
@@ -1537,12 +1564,20 @@ static int q6v5_remove(struct platform_device *pdev)
 	.ops = &q6v5_ops,
 };
 
+static const struct rproc_hexagon_res ipq8074_wcss = {
+	.hexagon_mba_image = "IPQ8074/q6_fw.mdt",
+	.need_mem_protection = false,
+	.version = WCSS_IPQ8074,
+	.init_reset = q6v5_wcss_init_reset,
+};
+
 static const struct of_device_id q6v5_of_match[] = {
 	{ .compatible = "qcom,q6v5-pil", .data = &msm8916_mss},
 	{ .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss},
 	{ .compatible = "qcom,msm8974-mss-pil", .data = &msm8974_mss},
 	{ .compatible = "qcom,msm8996-mss-pil", .data = &msm8996_mss},
 	{ .compatible = "qcom,sdm845-mss-pil", .data = &sdm845_mss},
+	{ .compatible = "qcom,ipq8074-wcss-pil", .data = &ipq8074_wcss},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, q6v5_of_match);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH V6 5/5] remoteproc: qcom: Add q6v5-wcss rproc ops

[Sricharan R]

q6v5-wcss core's start function is mostly common
with the q6v5 of msm8996. So reuse that and add
the stop function.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 227 +++++++++++++++++++++++++++++++++++++
 1 file changed, 227 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index fbe179d..979f6c9 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -113,6 +113,17 @@
 #define SLEEP_CHECK_MAX_LOOPS           200
 #define BOOT_FSM_TIMEOUT                10000
 
+/* QDSP6v5-WCSS config/status registers */
+#define TCSR_GLOBAL_CFG0	0x0
+#define TCSR_GLOBAL_CFG1	0x4
+#define SSCAON_CONFIG		0x8
+#define SSCAON_STATUS		0xc
+#define QDSP6SS_BHS_STATUS	0x78
+#define QDSP6SS_RST_EVB		0x10
+
+#define BHS_EN_REST_ACK		BIT(0)
+#define SSCAON_ENABLE		BIT(13)
+
 struct reg_info {
 	struct regulator *reg;
 	int uV;
@@ -823,6 +834,61 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 	return ret < 0 ? ret : 0;
 }
 
+static int q6v5_wcss_start(struct rproc *rproc)
+{
+	struct q6v5 *qproc = rproc->priv;
+	int ret = 0;
+
+	ret = q6v5_clk_enable(qproc->dev, qproc->active_clks,
+			      qproc->active_clk_count);
+	if (ret) {
+		dev_err(qproc->dev, "failed to enable clocks\n");
+		return ret;
+	}
+
+	/* Release Q6 and WCSS reset */
+	ret = reset_control_deassert(qproc->wcss_reset);
+	if (ret)
+		dev_err(qproc->dev, "wcss_reset failed\n");
+
+	ret = reset_control_deassert(qproc->wcss_q6_reset);
+	if (ret)
+		dev_err(qproc->dev, "wcss_q6_reset failed\n");
+
+	/* Lithium configuration - clock gating and bus arbitration */
+	ret = regmap_update_bits(qproc->halt_map,
+				 qproc->halt_nc + TCSR_GLOBAL_CFG0,
+				 0x1F, 0x14);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(qproc->halt_map,
+				 qproc->halt_nc + TCSR_GLOBAL_CFG1,
+				 1, 0);
+	if (ret)
+		return ret;
+
+	/* Write bootaddr to EVB so that Q6WCSS will jump there after reset */
+	writel(rproc->bootaddr >> 4, qproc->reg_base + QDSP6SS_RST_EVB);
+
+	ret = q6v5_reset(qproc);
+	if (ret)
+		return ret;
+
+	q6v5_reset_rest(qproc);
+
+	ret = wait_for_completion_timeout(&qproc->start_done,
+					  msecs_to_jiffies(5000));
+	if (ret == 0) {
+		dev_err(qproc->dev, "start timed out\n");
+		return -ETIMEDOUT;
+	}
+
+	qproc->running = true;
+
+	return 0;
+}
+
 static int q6v5_start(struct rproc *rproc)
 {
 	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
@@ -972,6 +1038,149 @@ static int q6v5_start(struct rproc *rproc)
 	return ret;
 }
 
+static int q6v5_wcss_powerdown(struct q6v5 *qproc)
+{
+	unsigned int val = 0;
+	int ret;
+
+	/* 1 - Assert WCSS/Q6 HALTREQ */
+	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
+
+	/* 2 - Enable WCSSAON_CONFIG */
+	val = readl(qproc->rmb_base + SSCAON_CONFIG);
+	val |= SSCAON_ENABLE;
+	writel(val, qproc->rmb_base + SSCAON_CONFIG);
+
+	/* 3 - Set SSCAON_CONFIG */
+	val |= BIT(15);
+	val &= ~BIT(16);
+	val &= ~BIT(17);
+	val &= ~BIT(18);
+	writel(val, qproc->rmb_base + SSCAON_CONFIG);
+
+	/* 4 - SSCAON_CONFIG 1 */
+	val |= BIT(1);
+	writel(val, qproc->rmb_base + SSCAON_CONFIG);
+
+	/* 5 - wait for SSCAON_STATUS */
+	ret = readl_poll_timeout(qproc->rmb_base + SSCAON_STATUS,
+				 val, (val & 0xffff) == 0x400, 1000,
+				 HALT_CHECK_MAX_LOOPS);
+	if (ret) {
+		dev_err(qproc->dev,
+			"can't get SSCAON_STATUS rc:%d)\n", ret);
+	}
+
+	/* 6 - De-assert WCSS_AON reset */
+	reset_control_assert(qproc->wcss_aon_reset);
+
+	/* 7 - Disable WCSSAON_CONFIG 13 */
+	val = readl(qproc->rmb_base + SSCAON_CONFIG);
+	val &= ~SSCAON_ENABLE;
+	writel(val, qproc->rmb_base + SSCAON_CONFIG);
+
+	/* 8 - De-assert WCSS/Q6 HALTREQ */
+	reset_control_assert(qproc->wcss_reset);
+
+	return ret;
+}
+
+static int q6v5_q6_powerdown(struct q6v5 *qproc)
+{
+	int i = 0, ret;
+	unsigned int val = 0;
+
+	/* 1 - Halt Q6 bus interface */
+	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
+
+	/* 2 - Disable Q6 Core clock */
+	val = readl(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
+	val &= ~Q6SS_CLK_ENABLE;
+	writel(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
+
+	/* 3 - Clamp I/O */
+	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	val |= Q6SS_CLAMP_IO;
+	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	/* 4 - Clamp WL */
+	val |= QDSS_BHS_ON;
+	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	/* 5 - Clear Erase standby */
+	val &= ~Q6SS_L2DATA_STBY_N;
+	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	/* 6 - Clear Sleep RTN */
+	val &= ~Q6SS_SLP_RET_N;
+	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	/* 7 - turn off QDSP6 memory foot/head switch one bank at a time */
+	for (i = 0; i < 20; i++) {
+		val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+		val &= ~BIT(i);
+		writel(val, qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+		mdelay(1);
+	}
+	/* 8 - Assert QMC memory RTN */
+	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	val |= QDSP6v56_CLAMP_QMC_MEM;
+	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	/* 9 - Turn off BHS */
+	val &= ~QDSP6v56_BHS_ON;
+	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	udelay(1);
+	/* 10 - Wait till BHS Reset is done */
+	ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_BHS_STATUS,
+				 val, !(val & BHS_EN_REST_ACK), 1000,
+				 HALT_CHECK_MAX_LOOPS);
+	if (ret) {
+		dev_err(qproc->dev,
+			"BHS_STATUS not OFF (rc:%d)\n", ret);
+	}
+
+	/* 11 -  Assert WCSS reset */
+	reset_control_assert(qproc->wcss_reset);
+
+	/* 12 - Assert Q6 reset */
+	reset_control_assert(qproc->wcss_q6_reset);
+
+	return 0;
+}
+
+static int q6v5_wcss_stop(struct rproc *rproc)
+{
+	struct q6v5 *qproc = rproc->priv;
+	int ret = 0;
+
+	qproc->running = false;
+
+	/* WCSS powerdown */
+	qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit),
+				    BIT(qproc->stop_bit));
+
+	ret = wait_for_completion_timeout(&qproc->stop_done,
+					  msecs_to_jiffies(5000));
+	if (ret == 0) {
+		dev_err(qproc->dev, "timed out on wait\n");
+		return -ETIMEDOUT;
+	}
+
+	qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit), 0);
+
+	ret = q6v5_wcss_powerdown(qproc);
+	if (ret)
+		return ret;
+
+	/* Q6 Power down */
+	ret = q6v5_q6_powerdown(qproc);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int q6v5_stop(struct rproc *rproc)
 {
 	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
@@ -1041,6 +1250,15 @@ static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len)
 	return qproc->mpss_region + offset;
 }
 
+static int q6v5_wcss_load(struct rproc *rproc, const struct firmware *fw)
+{
+	struct q6v5 *qproc = rproc->priv;
+
+	return qcom_mdt_load_no_init(qproc->dev, fw, rproc->firmware,
+				     0, qproc->mba_region, qproc->mba_phys,
+				     qproc->mba_size, &qproc->mpss_reloc);
+}
+
 static const struct rproc_ops q6v5_ops = {
 	.start = q6v5_start,
 	.stop = q6v5_stop,
@@ -1048,6 +1266,14 @@ static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len)
 	.load = q6v5_load,
 };
 
+static const struct rproc_ops q6v5_wcss_ops = {
+	.start = q6v5_wcss_start,
+	.stop = q6v5_wcss_stop,
+	.da_to_va = q6v5_da_to_va,
+	.load = q6v5_wcss_load,
+	.get_boot_addr = rproc_elf_get_boot_addr,
+};
+
 static irqreturn_t q6v5_wdog_interrupt(int irq, void *dev)
 {
 	struct q6v5 *qproc = dev;
@@ -1569,6 +1795,7 @@ static int q6v5_remove(struct platform_device *pdev)
 	.need_mem_protection = false,
 	.version = WCSS_IPQ8074,
 	.init_reset = q6v5_wcss_init_reset,
+	.ops = &q6v5_wcss_ops,
 };
 
 static const struct of_device_id q6v5_of_match[] = {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH V6 5/5] remoteproc: qcom: Add q6v5-wcss rproc ops

[Vinod]

On 14-05-18, 16:16, Sricharan R wrote:

> +static int q6v5_wcss_start(struct rproc *rproc)
> +{
> +	struct q6v5 *qproc = rproc->priv;
> +	int ret = 0;

Superfluous initialization

> +
> +	ret = q6v5_clk_enable(qproc->dev, qproc->active_clks,
> +			      qproc->active_clk_count);
> +	if (ret) {
> +		dev_err(qproc->dev, "failed to enable clocks\n");
> +		return ret;
> +	}
> +
> +	/* Release Q6 and WCSS reset */
> +	ret = reset_control_deassert(qproc->wcss_reset);
> +	if (ret)
> +		dev_err(qproc->dev, "wcss_reset failed\n");
> +
> +	ret = reset_control_deassert(qproc->wcss_q6_reset);
> +	if (ret)
> +		dev_err(qproc->dev, "wcss_q6_reset failed\n");

shouldn't we abort on these two errors?

> +
> +	/* Lithium configuration - clock gating and bus arbitration */
> +	ret = regmap_update_bits(qproc->halt_map,
> +				 qproc->halt_nc + TCSR_GLOBAL_CFG0,
> +				 0x1F, 0x14);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(qproc->halt_map,
> +				 qproc->halt_nc + TCSR_GLOBAL_CFG1,
> +				 1, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Write bootaddr to EVB so that Q6WCSS will jump there after reset */
> +	writel(rproc->bootaddr >> 4, qproc->reg_base + QDSP6SS_RST_EVB);
> +
> +	ret = q6v5_reset(qproc);
> +	if (ret)
> +		return ret;

all these returns, aren't we leaving device in some dangling state?

> +static int q6v5_wcss_powerdown(struct q6v5 *qproc)
> +{
> +	unsigned int val = 0;

superfluous initialization

> +	int ret;
> +
> +	/* 1 - Assert WCSS/Q6 HALTREQ */
> +	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
> +
> +	/* 2 - Enable WCSSAON_CONFIG */
> +	val = readl(qproc->rmb_base + SSCAON_CONFIG);
> +	val |= SSCAON_ENABLE;
> +	writel(val, qproc->rmb_base + SSCAON_CONFIG);
> +
> +	/* 3 - Set SSCAON_CONFIG */
> +	val |= BIT(15);
> +	val &= ~BIT(16);
> +	val &= ~BIT(17);
> +	val &= ~BIT(18);

shouldn't bit 15 thru 18 be defined on what they mean?

> +static int q6v5_q6_powerdown(struct q6v5 *qproc)
> +{
> +	int i = 0, ret;
> +	unsigned int val = 0;
> +
> +	/* 1 - Halt Q6 bus interface */
> +	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
> +
> +	/* 2 - Disable Q6 Core clock */
> +	val = readl(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
> +	val &= ~Q6SS_CLK_ENABLE;
> +	writel(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
> +
> +	/* 3 - Clamp I/O */
> +	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	val |= Q6SS_CLAMP_IO;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* 4 - Clamp WL */
> +	val |= QDSS_BHS_ON;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* 5 - Clear Erase standby */
> +	val &= ~Q6SS_L2DATA_STBY_N;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* 6 - Clear Sleep RTN */
> +	val &= ~Q6SS_SLP_RET_N;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* 7 - turn off QDSP6 memory foot/head switch one bank at a time */
> +	for (i = 0; i < 20; i++) {
> +		val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
> +		val &= ~BIT(i);
> +		writel(val, qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
> +		mdelay(1);
> +	}
> +	/* 8 - Assert QMC memory RTN */
> +	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	val |= QDSP6v56_CLAMP_QMC_MEM;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* 9 - Turn off BHS */
> +	val &= ~QDSP6v56_BHS_ON;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	udelay(1);
> +	/* 10 - Wait till BHS Reset is done */

would help in readability if you can be consistent and add empty lines after each
step as done for rest of the routine

> +static int q6v5_wcss_stop(struct rproc *rproc)
> +{
> +	struct q6v5 *qproc = rproc->priv;
> +	int ret = 0;

this one too, I think if you run with sparse, it should warn you about these

> +
> +	qproc->running = false;
> +
> +	/* WCSS powerdown */
> +	qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit),
> +				    BIT(qproc->stop_bit));
> +
> +	ret = wait_for_completion_timeout(&qproc->stop_done,
> +					  msecs_to_jiffies(5000));
> +	if (ret == 0) {
> +		dev_err(qproc->dev, "timed out on wait\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit), 0);
> +
> +	ret = q6v5_wcss_powerdown(qproc);
> +	if (ret)
> +		return ret;
> +
> +	/* Q6 Power down */
> +	ret = q6v5_q6_powerdown(qproc);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

this could be optimized to:
        return q6v5_q6_powerdown()
-- 
~Vinod

Re: [PATCH V6 5/5] remoteproc: qcom: Add q6v5-wcss rproc ops

[Sricharan R]

Hi,

On 5/18/2018 5:59 PM, Vinod wrote:
> On 14-05-18, 16:16, Sricharan R wrote:
> 
>> +static int q6v5_wcss_start(struct rproc *rproc)
>> +{
>> +	struct q6v5 *qproc = rproc->priv;
>> +	int ret = 0;
> 
> Superfluous initialization
> 

 ok.

>> +
>> +	ret = q6v5_clk_enable(qproc->dev, qproc->active_clks,
>> +			      qproc->active_clk_count);
>> +	if (ret) {
>> +		dev_err(qproc->dev, "failed to enable clocks\n");
>> +		return ret;
>> +	}
>> +
>> +	/* Release Q6 and WCSS reset */
>> +	ret = reset_control_deassert(qproc->wcss_reset);
>> +	if (ret)
>> +		dev_err(qproc->dev, "wcss_reset failed\n");
>> +
>> +	ret = reset_control_deassert(qproc->wcss_q6_reset);
>> +	if (ret)
>> +		dev_err(qproc->dev, "wcss_q6_reset failed\n");
> 
> shouldn't we abort on these two errors?
> 

 ha right. will fix it.

>> +
>> +	/* Lithium configuration - clock gating and bus arbitration */
>> +	ret = regmap_update_bits(qproc->halt_map,
>> +				 qproc->halt_nc + TCSR_GLOBAL_CFG0,
>> +				 0x1F, 0x14);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_update_bits(qproc->halt_map,
>> +				 qproc->halt_nc + TCSR_GLOBAL_CFG1,
>> +				 1, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Write bootaddr to EVB so that Q6WCSS will jump there after reset */
>> +	writel(rproc->bootaddr >> 4, qproc->reg_base + QDSP6SS_RST_EVB);
>> +
>> +	ret = q6v5_reset(qproc);
>> +	if (ret)
>> +		return ret;
> 
> all these returns, aren't we leaving device in some dangling state?
> 

 hmm ok. clocks and resets have to be reverted. will add error handling here.

>> +static int q6v5_wcss_powerdown(struct q6v5 *qproc)
>> +{
>> +	unsigned int val = 0;
> 
> superfluous initialization
> 
 ok.

>> +	int ret;
>> +
>> +	/* 1 - Assert WCSS/Q6 HALTREQ */
>> +	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
>> +
>> +	/* 2 - Enable WCSSAON_CONFIG */
>> +	val = readl(qproc->rmb_base + SSCAON_CONFIG);
>> +	val |= SSCAON_ENABLE;
>> +	writel(val, qproc->rmb_base + SSCAON_CONFIG);
>> +
>> +	/* 3 - Set SSCAON_CONFIG */
>> +	val |= BIT(15);
>> +	val &= ~BIT(16);
>> +	val &= ~BIT(17);
>> +	val &= ~BIT(18);
> 
> shouldn't bit 15 thru 18 be defined on what they mean?
> 

 hmm, ok. would define them.

>> +static int q6v5_q6_powerdown(struct q6v5 *qproc)
>> +{
>> +	int i = 0, ret;
>> +	unsigned int val = 0;
>> +
>> +	/* 1 - Halt Q6 bus interface */
>> +	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
>> +
>> +	/* 2 - Disable Q6 Core clock */
>> +	val = readl(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
>> +	val &= ~Q6SS_CLK_ENABLE;
>> +	writel(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
>> +
>> +	/* 3 - Clamp I/O */
>> +	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	val |= Q6SS_CLAMP_IO;
>> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +	/* 4 - Clamp WL */
>> +	val |= QDSS_BHS_ON;
>> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +	/* 5 - Clear Erase standby */
>> +	val &= ~Q6SS_L2DATA_STBY_N;
>> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +	/* 6 - Clear Sleep RTN */
>> +	val &= ~Q6SS_SLP_RET_N;
>> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +	/* 7 - turn off QDSP6 memory foot/head switch one bank at a time */
>> +	for (i = 0; i < 20; i++) {
>> +		val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
>> +		val &= ~BIT(i);
>> +		writel(val, qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
>> +		mdelay(1);
>> +	}
>> +	/* 8 - Assert QMC memory RTN */
>> +	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	val |= QDSP6v56_CLAMP_QMC_MEM;
>> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +	/* 9 - Turn off BHS */
>> +	val &= ~QDSP6v56_BHS_ON;
>> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	udelay(1);
>> +	/* 10 - Wait till BHS Reset is done */
> 
> would help in readability if you can be consistent and add empty lines after each
> step as done for rest of the routine
> 
 ok.

>> +static int q6v5_wcss_stop(struct rproc *rproc)
>> +{
>> +	struct q6v5 *qproc = rproc->priv;
>> +	int ret = 0;
> 
> this one too, I think if you run with sparse, it should warn you about these
> 

 ok, sure would check.

>> +
>> +	qproc->running = false;
>> +
>> +	/* WCSS powerdown */
>> +	qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit),
>> +				    BIT(qproc->stop_bit));
>> +
>> +	ret = wait_for_completion_timeout(&qproc->stop_done,
>> +					  msecs_to_jiffies(5000));
>> +	if (ret == 0) {
>> +		dev_err(qproc->dev, "timed out on wait\n");
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit), 0);
>> +
>> +	ret = q6v5_wcss_powerdown(qproc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Q6 Power down */
>> +	ret = q6v5_q6_powerdown(qproc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> 
> this could be optimized to:
>         return q6v5_q6_powerdown()
> 

 ok.

Regards,
 Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation