[PATCH 0/3] Add remoteproc driver for Hexagon Q6 - WCSS integrated core

[PATCH 0/3] Add remoteproc driver for Hexagon Q6 - 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.

Sricharan R (3):
  drivers: remoteproc: Make mdt_loader firmware authentication optional
  drivers: remoteproc: Add Hexagon Q6 - WCSS integrated core driver
  dt-binding: remoteproc: Add the bindings required for Hexagon - WCSS
    core

 .../bindings/remoteproc/qcom,q6v5-wcss.txt         | 139 ++++
 drivers/remoteproc/Kconfig                         |  13 +
 drivers/remoteproc/Makefile                        |   1 +
 drivers/remoteproc/q6v5-wcss.c                     | 784 +++++++++++++++++++++
 drivers/remoteproc/qcom_adsp_pil.c                 |   3 +-
 drivers/remoteproc/qcom_wcnss.c                    |   3 +-
 drivers/soc/qcom/mdt_loader.c                      |  24 +-
 include/linux/soc/qcom/mdt_loader.h                |   2 +-
 8 files changed, 956 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,q6v5-wcss.txt
 create mode 100644 drivers/remoteproc/q6v5-wcss.c

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

[PATCH 1/3] drivers: remoteproc: Make mdt_loader firmware authentication optional

[Sricharan R]

qcom_mdt_load function loads the mdt type firmware and
authenticates it as well. Make the authentication only
when requested by the caller, so that the function can be used
by self-authenticating remoteproc as well.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/remoteproc/qcom_adsp_pil.c  |  3 ++-
 drivers/remoteproc/qcom_wcnss.c     |  3 ++-
 drivers/soc/qcom/mdt_loader.c       | 24 ++++++++++++++----------
 include/linux/soc/qcom/mdt_loader.h |  2 +-
 4 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/remoteproc/qcom_adsp_pil.c b/drivers/remoteproc/qcom_adsp_pil.c
index 49fe2f8..e168375 100644
--- a/drivers/remoteproc/qcom_adsp_pil.c
+++ b/drivers/remoteproc/qcom_adsp_pil.c
@@ -79,7 +79,8 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
 	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
 
 	return qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id,
-			     adsp->mem_region, adsp->mem_phys, adsp->mem_size);
+			     adsp->mem_region, adsp->mem_phys, adsp->mem_size,
+			     true);
 }
 
 static const struct rproc_fw_ops adsp_fw_ops = {
diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c
index c768639..b41b9b5 100644
--- a/drivers/remoteproc/qcom_wcnss.c
+++ b/drivers/remoteproc/qcom_wcnss.c
@@ -153,7 +153,8 @@ static int wcnss_load(struct rproc *rproc, const struct firmware *fw)
 	struct qcom_wcnss *wcnss = (struct qcom_wcnss *)rproc->priv;
 
 	return qcom_mdt_load(wcnss->dev, fw, rproc->firmware, WCNSS_PAS_ID,
-			     wcnss->mem_region, wcnss->mem_phys, wcnss->mem_size);
+			     wcnss->mem_region, wcnss->mem_phys,
+			     wcnss->mem_size, true);
 }
 
 static const struct rproc_fw_ops wcnss_fw_ops = {
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index bd63df0..b3947d0 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -88,7 +88,7 @@ ssize_t qcom_mdt_get_size(const struct firmware *fw)
  */
 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 mem_phys, size_t mem_size, bool auth)
 {
 	const struct elf32_phdr *phdrs;
 	const struct elf32_phdr *phdr;
@@ -119,10 +119,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 (auth) {
+		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++) {
@@ -142,12 +144,14 @@ 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 (auth) {
+			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;
+			}
 		}
-
 		/*
 		 * The image is relocatable, so offset each segment based on
 		 * the lowest segment address.
diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
index f423001..7ff4dd3 100644
--- a/include/linux/soc/qcom/mdt_loader.h
+++ b/include/linux/soc/qcom/mdt_loader.h
@@ -13,6 +13,6 @@
 ssize_t qcom_mdt_get_size(const struct firmware *fw);
 int qcom_mdt_load(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 mem_phys, size_t mem_size, bool auth);
 
 #endif
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 1/3] drivers: remoteproc: Make mdt_loader firmware authentication optional

[Bjorn Andersson]

On Thu 29 Jun 07:17 PDT 2017, Sricharan R wrote:

> qcom_mdt_load function loads the mdt type firmware and
> authenticates it as well. Make the authentication only
> when requested by the caller, so that the function can be used
> by self-authenticating remoteproc as well.
> 

This is good, we should be able to save some duplication the current
MSA PIL as well by this.

> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_adsp_pil.c  |  3 ++-
>  drivers/remoteproc/qcom_wcnss.c     |  3 ++-
>  drivers/soc/qcom/mdt_loader.c       | 24 ++++++++++++++----------
>  include/linux/soc/qcom/mdt_loader.h |  2 +-

We have two additional callers being merged, so changing the prototype
of qcom_mdt_load() will cause issues.

I think the best approach is to leave all callers untouched, make the
current implementation of qcom_mdt_load() internal and provide two
exported wrapper functions: qcom_mdt_load() and qcom_mdt_load_no_init().

These wrappers would call the internal function with the appropriate
value of the boolean.

[..]
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
[..]
>  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 mem_phys, size_t mem_size, bool auth)

We're not authenticating even with @auth=true, so please name this
"pas_init".

>  {
>  	const struct elf32_phdr *phdrs;
>  	const struct elf32_phdr *phdr;
[..]
> @@ -142,12 +144,14 @@ 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 (auth) {
> +			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;
> +			}
>  		}
> -

I like this empty line, please let me have it.

>  		/*
>  		 * The image is relocatable, so offset each segment based on
>  		 * the lowest segment address.

Regards,
Bjorn

Re: [PATCH 1/3] drivers: remoteproc: Make mdt_loader firmware authentication optional

[Sricharan R]

Hi Bjorn,
   Thanks for the review !

On 7/26/2017 11:41 PM, Bjorn Andersson wrote:
> On Thu 29 Jun 07:17 PDT 2017, Sricharan R wrote:
> 
>> qcom_mdt_load function loads the mdt type firmware and
>> authenticates it as well. Make the authentication only
>> when requested by the caller, so that the function can be used
>> by self-authenticating remoteproc as well.
>>
> 
> This is good, we should be able to save some duplication the current
> MSA PIL as well by this.
> 
>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
>> ---
>>  drivers/remoteproc/qcom_adsp_pil.c  |  3 ++-
>>  drivers/remoteproc/qcom_wcnss.c     |  3 ++-
>>  drivers/soc/qcom/mdt_loader.c       | 24 ++++++++++++++----------
>>  include/linux/soc/qcom/mdt_loader.h |  2 +-
> 
> We have two additional callers being merged, so changing the prototype
> of qcom_mdt_load() will cause issues.
> 
> I think the best approach is to leave all callers untouched, make the
> current implementation of qcom_mdt_load() internal and provide two
> exported wrapper functions: qcom_mdt_load() and qcom_mdt_load_no_init().
> 
> These wrappers would call the internal function with the appropriate
> value of the boolean.
> 

 ok, will do.

> [..]
>> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> [..]
>>  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 mem_phys, size_t mem_size, bool auth)
> 
> We're not authenticating even with @auth=true, so please name this
> "pas_init".
>

 ok will rename.
 
>>  {
>>  	const struct elf32_phdr *phdrs;
>>  	const struct elf32_phdr *phdr;
> [..]
>> @@ -142,12 +144,14 @@ 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 (auth) {
>> +			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;
>> +			}
>>  		}
>> -
> 
> I like this empty line, please let me have it.
> 

 ok.

Regards,
 Sricharan

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

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

[PATCH 2/3] drivers: remoteproc: Add Hexagon Q6 - WCSS integrated core driver

[Sricharan R]

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

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/remoteproc/Kconfig     |  13 +
 drivers/remoteproc/Makefile    |   1 +
 drivers/remoteproc/q6v5-wcss.c | 784 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 798 insertions(+)
 create mode 100644 drivers/remoteproc/q6v5-wcss.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index faad69a..a08eb1d 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -114,6 +114,19 @@ config QCOM_WCNSS_PIL
 	  Say y here to support the Peripheral Image Loader for the Qualcomm
 	  Wireless Connectivity Subsystem.
 
+config QCOM_Q6V5_WCNSS_PIL
+	tristate "Qualcomm Q6V5-WCNSS Peripheral Image Loader"
+	depends on OF && ARCH_QCOM
+	depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)
+	depends on QCOM_SMEM
+	depends on REMOTEPROC
+	select QCOM_MDT_LOADER
+	select QCOM_RPROC_COMMON
+	select QCOM_SCM
+	help
+	  Say y here to support the Peripheral Image Loader for the Qualcomm
+	  Hexagon V5 + Wireless Connectivity Subsystem.
+
 config ST_REMOTEPROC
 	tristate "ST remoteproc support"
 	depends on ARCH_STI
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index ffc5e43..ebe339f 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_QCOM_ADSP_PIL)		+= qcom_adsp_pil.o
 obj-$(CONFIG_QCOM_RPROC_COMMON)		+= qcom_common.o
 obj-$(CONFIG_QCOM_Q6V5_PIL)		+= qcom_q6v5_pil.o
 obj-$(CONFIG_QCOM_WCNSS_PIL)		+= qcom_wcnss_pil.o
+obj-$(CONFIG_QCOM_Q6V5_WCNSS_PIL)	+= q6v5-wcss.o
 qcom_wcnss_pil-y			+= qcom_wcnss.o
 qcom_wcnss_pil-y			+= qcom_wcnss_iris.o
 obj-$(CONFIG_ST_REMOTEPROC)		+= st_remoteproc.o
diff --git a/drivers/remoteproc/q6v5-wcss.c b/drivers/remoteproc/q6v5-wcss.c
new file mode 100644
index 0000000..52bdc71
--- /dev/null
+++ b/drivers/remoteproc/q6v5-wcss.c
@@ -0,0 +1,784 @@
+/*
+ * Qualcomm q6v5-wcss Peripheral Image Loader
+ *
+ * Copyright (c) 2017, The Linux Foundation. All rights reserved.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/elf.h>
+#include <linux/err.h>
+#include <linux/firmware.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/qcom_scm.h>
+#include <linux/regmap.h>
+#include <linux/remoteproc.h>
+#include <linux/reset.h>
+#include <linux/soc/qcom/mdt_loader.h>
+#include <linux/soc/qcom/smem.h>
+#include <linux/soc/qcom/smem_state.h>
+
+#include "qcom_common.h"
+#include "remoteproc_internal.h"
+
+#define WCSS_CRASH_REASON_SMEM 421
+#define WCNSS_PAS_ID		6
+#define STOP_ACK_TIMEOUT_MS 10000
+
+#define QDSP6SS_RST_EVB 0x10
+#define QDSP6SS_RESET 0x14
+#define QDSP6SS_DBG_CFG 0x18
+#define QDSP6SS_XO_CBCR 0x38
+#define QDSP6SS_MEM_PWR_CTL 0xb0
+#define QDSP6SS_BHS_STATUS 0x78
+#define TCSR_GLOBAL_CFG0 0x0
+#define TCSR_GLOBAL_CFG1 0x4
+
+#define QDSP6SS_GFMUX_CTL 0x20
+#define QDSP6SS_PWR_CTL 0x30
+#define TCSR_HALTREQ 0x0
+#define TCSR_HALTACK 0x4
+#define TCSR_Q6_HALTREQ 0x0
+#define TCSR_Q6_HALTACK 0x4
+#define SSCAON_CONFIG 0x8
+#define SSCAON_STATUS 0xc
+#define HALTACK BIT(0)
+#define BHS_EN_REST_ACK BIT(0)
+
+struct q6v5 {
+	struct device *dev;
+	struct qcom_rproc_subdev smd_subdev;
+	phys_addr_t mem_phys;
+	size_t	mem_size;
+	void *mem_region;
+	void __iomem *q6_base;
+	void __iomem *mpm_base;
+	struct regmap *tcsr;
+	unsigned int halt_gbl;
+	unsigned int halt_q6;
+	unsigned int halt_wcss;
+	struct rproc *rproc;
+	struct completion start_done;
+	struct completion stop_done;
+	struct qcom_smem_state *state;
+	unsigned int stop_bit;
+	unsigned int shutdown_bit;
+	bool running;
+	struct clk **clks;
+	int clk_cnt;
+	struct reset_control *wcss_aon_reset;
+	struct reset_control *wcss_reset;
+	struct reset_control *wcss_q6_reset;
+};
+
+static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
+						  const struct firmware *fw,
+						  int *tablesz)
+{
+	static struct resource_table table = { .ver = 1, };
+
+	*tablesz = sizeof(table);
+	return &table;
+}
+
+static int q6v5_init_clocks(struct device *dev, struct q6v5 *qproc)
+{
+	int i;
+	const char *cname;
+	struct property *prop;
+
+	qproc->clk_cnt = of_property_count_strings(dev->of_node,
+						   "clock-names");
+
+	if (!qproc->clk_cnt)
+		return 0;
+
+	qproc->clks = devm_kzalloc(dev, sizeof(*qproc->clks) * qproc->clk_cnt,
+				   GFP_KERNEL);
+
+	of_property_for_each_string(dev->of_node, "clock-names", prop, cname) {
+		struct clk *c = devm_clk_get(dev, cname);
+
+		if (IS_ERR_OR_NULL(c)) {
+			if (PTR_ERR(c) != -EPROBE_DEFER)
+				dev_err(dev, "Failed to get %s clock\n", cname);
+
+			return PTR_ERR(c);
+		}
+
+		qproc->clks[i++] = c;
+	}
+
+	return 0;
+}
+
+static int q6v5_clk_enable(struct q6v5 *qproc)
+{
+	int rc;
+	int i;
+
+	for (i = 0; i < qproc->clk_cnt; i++) {
+		rc = clk_prepare_enable(qproc->clks[i]);
+		if (rc)
+			goto err;
+	}
+
+	return 0;
+err:
+	for (i--; i >= 0; i--)
+		clk_disable_unprepare(qproc->clks[i]);
+
+	return rc;
+}
+
+static int wcss_powerdown(struct q6v5 *qproc)
+{
+	unsigned int nretry = 0;
+	unsigned int val = 0;
+	int ret;
+
+	/* Assert WCSS/Q6 HALTREQ - 1 */
+	nretry = 0;
+
+	ret = regmap_update_bits(qproc->tcsr, qproc->halt_wcss + TCSR_HALTREQ,
+				 1, 1);
+	if (ret)
+		return ret;
+
+	while (1) {
+		regmap_read(qproc->tcsr, qproc->halt_wcss + TCSR_HALTACK,
+			    &val);
+		if (val & HALTACK)
+			break;
+		mdelay(1);
+		nretry++;
+		if (nretry >= 10) {
+			pr_warn("can't get TCSR haltACK\n");
+			break;
+		}
+	}
+
+	/* Check HALTACK */
+	/* Set MPM_SSCAON_CONFIG 13 - 2 */
+	val = readl(qproc->mpm_base + SSCAON_CONFIG);
+	val |= BIT(13);
+	writel(val, qproc->mpm_base + SSCAON_CONFIG);
+
+	/* Set MPM_SSCAON_CONFIG 15 - 3 */
+	val = readl(qproc->mpm_base + SSCAON_CONFIG);
+	val |= BIT(15);
+	val &= ~(BIT(16));
+	val &= ~(BIT(17));
+	val &= ~(BIT(18));
+	writel(val, qproc->mpm_base + SSCAON_CONFIG);
+
+	/* Set MPM_SSCAON_CONFIG 1 - 4 */
+	val = readl(qproc->mpm_base + SSCAON_CONFIG);
+	val |= BIT(1);
+	writel(val, qproc->mpm_base + SSCAON_CONFIG);
+
+	/* wait for SSCAON_STATUS to be 0x400 - 5 */
+	nretry = 0;
+	while (1) {
+		val = readl(qproc->mpm_base + SSCAON_STATUS);
+		/* ignore bits 16 to 31 */
+		val &= 0xffff;
+		if (val == BIT(10))
+			break;
+		nretry++;
+		mdelay(1);
+		if (nretry == 10) {
+			pr_warn("can't get SSCAON_STATUS\n");
+			break;
+		}
+	}
+
+	/* Enable Q6/WCSS BLOCK ARES - 6 */
+	reset_control_assert(qproc->wcss_aon_reset);
+
+	/* Enable MPM_WCSSAON_CONFIG 13 - 7 */
+	val = readl(qproc->mpm_base + SSCAON_CONFIG);
+	val &= (~(BIT(13)));
+	writel(val, qproc->mpm_base + SSCAON_CONFIG);
+
+	/* Enable A2AB/ACMT/ECHAB ARES - 8 */
+	/* De-assert WCSS/Q6 HALTREQ - 8 */
+	reset_control_assert(qproc->wcss_reset);
+
+	ret = regmap_update_bits(qproc->tcsr, qproc->halt_wcss + TCSR_HALTREQ,
+				 1, 0);
+
+	return ret;
+}
+
+static int q6_powerdown(struct q6v5 *qproc)
+{
+	int i = 0, ret;
+	unsigned int nretry = 0;
+	unsigned int val = 0;
+
+	/* Halt Q6 bus interface - 9*/
+	ret = regmap_update_bits(qproc->tcsr, qproc->halt_q6 + TCSR_Q6_HALTREQ,
+				 1, 1);
+	if (ret)
+		return ret;
+
+	nretry = 0;
+	while (1) {
+		regmap_read(qproc->tcsr, qproc->halt_q6 + TCSR_Q6_HALTACK,
+			    &val);
+		if (val & HALTACK)
+			break;
+		mdelay(1);
+		nretry++;
+		if (nretry >= 10) {
+			pr_err("can't get TCSR Q6 haltACK\n");
+			break;
+		}
+	}
+
+	/* Disable Q6 Core clock - 10 */
+	val = readl(qproc->q6_base + QDSP6SS_GFMUX_CTL);
+	val &= (~(BIT(1)));
+	writel(val, qproc->q6_base + QDSP6SS_GFMUX_CTL);
+
+	/* Clamp I/O - 11 */
+	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
+	val |= BIT(20);
+	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
+
+	/* Clamp WL - 12 */
+	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
+	val |= BIT(21);
+	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
+
+	/* Clear Erase standby - 13 */
+	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
+	val &= (~(BIT(18)));
+	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
+
+	/* Clear Sleep RTN - 14 */
+	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
+	val &= (~(BIT(19)));
+	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
+
+	/* turn off QDSP6 memory foot/head switch one bank at a time - 15*/
+	for (i = 0; i < 20; i++) {
+		val = readl(qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
+		val &= (~(BIT(i)));
+		writel(val, qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
+		mdelay(1);
+	}
+
+	/* Assert QMC memory RTN - 16 */
+	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
+	val |= BIT(22);
+	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
+
+	/* Turn off BHS - 17 */
+	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
+	val &= (~(BIT(24)));
+	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
+	udelay(1);
+	/* Wait till BHS Reset is done */
+	nretry = 0;
+	while (1) {
+		val = readl(qproc->q6_base + QDSP6SS_BHS_STATUS);
+		if (!(val & BHS_EN_REST_ACK))
+			break;
+		mdelay(1);
+		nretry++;
+		if (nretry >= 10) {
+			pr_err("BHS_STATUS not OFF\n");
+			break;
+		}
+	}
+
+	/* HALT CLEAR - 18 */
+	ret = regmap_update_bits(qproc->tcsr, qproc->halt_q6 + TCSR_Q6_HALTREQ,
+				 1, 0);
+	if (ret)
+		return ret;
+
+	/* Enable Q6 Block reset - 19 */
+	reset_control_assert(qproc->wcss_q6_reset);
+
+	return 0;
+}
+
+static int q6_rproc_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 = wcss_powerdown(qproc);
+	if (ret)
+		return ret;
+
+	/* Q6 Power down */
+	ret = q6_powerdown(qproc);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int q6_rproc_start(struct rproc *rproc)
+{
+	struct q6v5 *qproc = rproc->priv;
+	int temp = 19;
+	unsigned long val = 0;
+	unsigned int nretry = 0;
+	int ret = 0;
+
+	ret = q6v5_clk_enable(qproc);
+	if (ret) {
+		dev_err(qproc->dev, "failed to enable clocks\n");
+		return ret;
+	}
+
+	/* Release Q6 and WCSS reset */
+	reset_control_deassert(qproc->wcss_reset);
+	reset_control_deassert(qproc->wcss_q6_reset);
+
+	/* Lithium configuration - clock gating and bus arbitration */
+	ret = regmap_update_bits(qproc->tcsr,
+				 qproc->halt_gbl + TCSR_GLOBAL_CFG0,
+				 0x1F, 0x14);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(qproc->tcsr,
+				 qproc->halt_gbl + 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->q6_base + QDSP6SS_RST_EVB);
+	/* Turn on XO clock. It is required for BHS and memory operation */
+	writel(0x1, qproc->q6_base + QDSP6SS_XO_CBCR);
+	/* Turn on BHS */
+	writel(0x1700000, qproc->q6_base + QDSP6SS_PWR_CTL);
+	udelay(1);
+
+	/* Wait till BHS Reset is done */
+	while (1) {
+		val = readl(qproc->q6_base + QDSP6SS_BHS_STATUS);
+		if (val & BHS_EN_REST_ACK)
+			break;
+		mdelay(1);
+		nretry++;
+		if (nretry >= 10) {
+			pr_err("BHS_STATUS not ON\n");
+			break;
+		}
+	}
+
+	/* Put LDO in bypass mode */
+	writel(0x3700000, qproc->q6_base + QDSP6SS_PWR_CTL);
+	/* De-assert QDSP6 complier memory clamp */
+	writel(0x3300000, qproc->q6_base + QDSP6SS_PWR_CTL);
+	/* De-assert memory peripheral sleep and L2 memory standby */
+	writel(0x33c0000, qproc->q6_base + QDSP6SS_PWR_CTL);
+
+	/* turn on QDSP6 memory foot/head switch one bank at a time */
+	while  (temp >= 0) {
+		val = readl(qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
+		val = val | 1 << temp;
+		writel(val, qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
+		val = readl(qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
+		mdelay(10);
+		temp -= 1;
+	}
+	/* Remove the QDSP6 core memory word line clamp */
+	writel(0x31FFFFF, qproc->q6_base + QDSP6SS_PWR_CTL);
+	/* Remove QDSP6 I/O clamp */
+	writel(0x30FFFFF, qproc->q6_base + QDSP6SS_PWR_CTL);
+
+	/* Bring Q6 out of reset and stop the core */
+	writel(0x5, qproc->q6_base + QDSP6SS_RESET);
+
+	/* Retain debugger state during next QDSP6 reset */
+	writel(0x0, qproc->q6_base + QDSP6SS_DBG_CFG);
+	/* Turn on the QDSP6 core clock */
+	writel(0x102, qproc->q6_base + QDSP6SS_GFMUX_CTL);
+	/* Enable the core to run */
+	writel(0x4, qproc->q6_base + QDSP6SS_RESET);
+
+	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 struct rproc_ops q6v5_rproc_ops = {
+	.start		= q6_rproc_start,
+	.stop		= q6_rproc_stop,
+};
+
+static struct rproc_fw_ops q6_fw_ops;
+
+static int q6v5_request_irq(struct q6v5 *qproc,
+			    struct platform_device *pdev,
+			    const char *name,
+			    irq_handler_t thread_fn)
+{
+	int ret;
+
+	ret = platform_get_irq_byname(pdev, name);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "no %s IRQ defined\n", name);
+		return ret;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, ret,
+					NULL, thread_fn,
+					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+					"q6v5", qproc);
+	if (ret)
+		dev_err(&pdev->dev, "request %s IRQ failed\n", name);
+
+	return ret;
+}
+
+static irqreturn_t q6v5_fatal_interrupt(int irq, void *dev)
+{
+	struct q6v5 *qproc = dev;
+	char *msg;
+	size_t len;
+
+	if (!qproc->running)
+		return IRQ_HANDLED;
+
+	msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, WCSS_CRASH_REASON_SMEM, &len);
+	if (!IS_ERR(msg) && len > 0 && msg[0])
+		dev_err(qproc->dev, "Fatal error from wcss: %s\n", msg);
+	else
+		dev_err(qproc->dev, "Fatal error received no message!\n");
+
+	rproc_report_crash(qproc->rproc, RPROC_FATAL_ERROR);
+
+	if (!IS_ERR(msg))
+		msg[0] = '\0';
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t q6v5_handover_interrupt(int irq, void *dev)
+{
+	struct q6v5 *qproc = dev;
+
+	complete(&qproc->start_done);
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t q6v5_stop_ack_interrupt(int irq, void *dev)
+{
+	struct q6v5 *qproc = dev;
+
+	complete(&qproc->stop_done);
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t q6v5_wdog_interrupt(int irq, void *dev)
+{
+	struct q6v5 *qproc = dev;
+	char *msg;
+	size_t len;
+
+	if (!qproc->running) {
+		complete(&qproc->stop_done);
+		return IRQ_HANDLED;
+	}
+
+	msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, WCSS_CRASH_REASON_SMEM, &len);
+	if (!IS_ERR(msg) && len > 0 && msg[0])
+		dev_err(qproc->dev, "Watchdog bite from wcss %s\n", msg);
+	else
+		dev_err(qproc->dev, "Watchdog bit received no message!\n");
+
+	rproc_report_crash(qproc->rproc, RPROC_WATCHDOG);
+
+	if (!IS_ERR(msg))
+		msg[0] = '\0';
+
+	return IRQ_HANDLED;
+}
+
+static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
+{
+	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
+
+	return qcom_mdt_load(qproc->dev, fw, rproc->firmware, WCNSS_PAS_ID,
+			     qproc->mem_region, qproc->mem_phys,
+			     qproc->mem_size, false);
+}
+
+static int q6_alloc_memory_region(struct q6v5 *qproc)
+{
+	struct device_node *node;
+	struct resource r;
+	int ret;
+
+	node = of_parse_phandle(qproc->dev->of_node, "memory-region", 0);
+	if (!node) {
+		dev_err(qproc->dev, "no memory-region specified\n");
+		return -EINVAL;
+	}
+
+	ret = of_address_to_resource(node, 0, &r);
+	if (ret)
+		return ret;
+
+	qproc->mem_phys = r.start;
+	qproc->mem_size = resource_size(&r);
+	qproc->mem_region = devm_ioremap_wc(qproc->dev, qproc->mem_phys,
+					    qproc->mem_size);
+	if (!qproc->mem_region) {
+		dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
+			&r.start, qproc->mem_size);
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev)
+{
+	struct of_phandle_args args;
+	struct resource *res;
+	int ret;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+					   "mpm");
+	if (IS_ERR_OR_NULL(res))
+		return -ENODEV;
+
+	qproc->mpm_base = ioremap(res->start, resource_size(res));
+	if (IS_ERR_OR_NULL(qproc->mpm_base))
+		return PTR_ERR(qproc->mpm_base);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+					   "q6");
+	if (IS_ERR_OR_NULL(res)) {
+		ret = -ENODEV;
+		goto free_mpm;
+	}
+
+	qproc->q6_base = ioremap(res->start, resource_size(res));
+	if (IS_ERR_OR_NULL(qproc->q6_base)) {
+		ret = PTR_ERR(qproc->q6_base);
+		goto free_mpm;
+	}
+
+	ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
+					       "qcom,halt-regs", 3,
+					       0, &args);
+	if (ret < 0)
+		goto free_q6;
+
+	qproc->tcsr = syscon_node_to_regmap(args.np);
+	of_node_put(args.np);
+	if (IS_ERR_OR_NULL(qproc->tcsr)) {
+		ret = PTR_ERR(qproc->tcsr);
+		goto free_q6;
+	}
+
+	qproc->halt_gbl = args.args[0];
+	qproc->halt_q6 = args.args[1];
+	qproc->halt_wcss = args.args[2];
+
+	return 0;
+
+free_q6:
+	iounmap(qproc->q6_base);
+
+free_mpm:
+	iounmap(qproc->mpm_base);
+
+	return ret;
+}
+
+static int q6_rproc_probe(struct platform_device *pdev)
+{
+	struct q6v5 *qproc;
+	struct rproc *rproc;
+	int ret;
+	struct qcom_smem_state *state;
+	unsigned int stop_bit;
+	const char *firmware_name = of_device_get_match_data(&pdev->dev);
+
+	state = qcom_smem_state_get(&pdev->dev, "stop",
+				    &stop_bit);
+	if (IS_ERR(state))
+		/* Wait till SMP2P is registered and up */
+		return -EPROBE_DEFER;
+
+	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_rproc_ops,
+			    firmware_name,
+			    sizeof(*qproc));
+	if (unlikely(!rproc))
+		return -ENOMEM;
+
+	qproc = rproc->priv;
+	qproc->dev = &pdev->dev;
+	qproc->rproc = rproc;
+	rproc->has_iommu = false;
+
+	q6_fw_ops = *rproc->fw_ops;
+	q6_fw_ops.find_rsc_table = q6v5_find_rsc_table;
+	q6_fw_ops.load = q6v5_load;
+
+	q6v5_init_mem(qproc, pdev);
+
+	qproc->wcss_aon_reset = devm_reset_control_get(&pdev->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(&pdev->dev,
+						   "wcss_reset");
+	if (IS_ERR(qproc->wcss_reset))
+		return PTR_ERR(qproc->wcss_reset);
+
+	qproc->wcss_q6_reset = devm_reset_control_get(&pdev->dev,
+						      "wcss_q6_reset");
+	if (IS_ERR(qproc->wcss_q6_reset))
+		return PTR_ERR(qproc->wcss_q6_reset);
+
+	platform_set_drvdata(pdev, qproc);
+
+	rproc->fw_ops = &q6_fw_ops;
+
+	qproc->state = qcom_smem_state_get(&pdev->dev, "stop",
+					   &qproc->stop_bit);
+	if (IS_ERR(qproc->state)) {
+		pr_err("Can't get stop bit status fro SMP2P\n");
+		goto free_rproc;
+	}
+
+	qproc->state = qcom_smem_state_get(&pdev->dev, "shutdown",
+			&qproc->shutdown_bit);
+	if (IS_ERR(qproc->state)) {
+		pr_err("Can't get shutdown bit status fro SMP2P\n");
+		goto free_rproc;
+	}
+
+	ret = q6v5_init_clocks(&pdev->dev, qproc);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to get active clocks.\n");
+		goto free_rproc;
+	}
+
+	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
+	if (ret < 0)
+		goto free_rproc;
+
+	ret = q6v5_request_irq(qproc, pdev, "fatal", q6v5_fatal_interrupt);
+	if (ret < 0)
+		goto free_rproc;
+
+	ret = q6v5_request_irq(qproc, pdev, "handover",
+			       q6v5_handover_interrupt);
+	if (ret < 0)
+		goto free_rproc;
+
+	ret = q6v5_request_irq(qproc, pdev, "stop-ack",
+			       q6v5_stop_ack_interrupt);
+	if (ret < 0)
+		goto free_rproc;
+
+	init_completion(&qproc->start_done);
+	init_completion(&qproc->stop_done);
+
+	ret = q6_alloc_memory_region(qproc);
+	if (ret < 0)
+		goto free_rproc;
+
+	qcom_add_smd_subdev(rproc, &qproc->smd_subdev);
+
+	ret = rproc_add(rproc);
+	if (ret)
+		goto free_rproc;
+
+	qproc->running = false;
+
+	return 0;
+
+free_rproc:
+	rproc_put(rproc);
+	return -EIO;
+}
+
+static int q6_rproc_remove(struct platform_device *pdev)
+{
+	struct q6v5 *qproc;
+	struct rproc *rproc;
+
+	qproc = platform_get_drvdata(pdev);
+	rproc = qproc->rproc;
+
+	rproc_del(rproc);
+	rproc_put(rproc);
+
+	return 0;
+}
+
+static const struct of_device_id q6_match_table[] = {
+	{ .compatible = "q6v5-wcss-pil", .data = "IPQ8074/q6_fw.mdt" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, q6_match_table);
+
+static struct platform_driver q6_rproc_driver = {
+	.probe = q6_rproc_probe,
+	.remove = q6_rproc_remove,
+	.driver = {
+		.name = "q6v5-wcss",
+		.of_match_table = q6_match_table,
+		.owner = THIS_MODULE,
+	},
+};
+module_platform_driver(q6_rproc_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Qualcomm q6v5-wcss remote proc control driver");
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 2/3] drivers: remoteproc: Add Hexagon Q6 - WCSS integrated core driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2120 bytes --]

Hi Sricharan,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.12-rc7 next-20170630]
[cannot apply to remoteproc/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sricharan-R/drivers-remoteproc-Make-mdt_loader-firmware-authentication-optional/20170702-123208
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers//remoteproc/q6v5-wcss.c: In function 'q6_rproc_probe':
>> drivers//remoteproc/q6v5-wcss.c:126:14: warning: 'i' may be used uninitialized in this function [-Wmaybe-uninitialized]
      qproc->clks[i++] = c;
                 ^
   drivers//remoteproc/q6v5-wcss.c:103:6: note: 'i' was declared here
     int i;
         ^

vim +/i +126 drivers//remoteproc/q6v5-wcss.c

   110		if (!qproc->clk_cnt)
   111			return 0;
   112	
   113		qproc->clks = devm_kzalloc(dev, sizeof(*qproc->clks) * qproc->clk_cnt,
   114					   GFP_KERNEL);
   115	
   116		of_property_for_each_string(dev->of_node, "clock-names", prop, cname) {
   117			struct clk *c = devm_clk_get(dev, cname);
   118	
   119			if (IS_ERR_OR_NULL(c)) {
   120				if (PTR_ERR(c) != -EPROBE_DEFER)
   121					dev_err(dev, "Failed to get %s clock\n", cname);
   122	
   123				return PTR_ERR(c);
   124			}
   125	
 > 126			qproc->clks[i++] = c;
   127		}
   128	
   129		return 0;
   130	}
   131	
   132	static int q6v5_clk_enable(struct q6v5 *qproc)
   133	{
   134		int rc;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55833 bytes --]

Re: [PATCH 2/3] drivers: remoteproc: Add Hexagon Q6 - WCSS integrated core driver

[Bjorn Andersson]

On Thu 29 Jun 07:17 PDT 2017, Sricharan R wrote:

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

There is a fair amount of code in this driver that seems to be
equivalent to Avaneesh q6v5 patches for MSM8996.

The differences coming from the MBA + MPSS vs single-image we have to
live with, but can we do something about the Q6 programming sequences?
E.g. extract them to a common file?

> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> ---
>  drivers/remoteproc/Kconfig     |  13 +
>  drivers/remoteproc/Makefile    |   1 +
>  drivers/remoteproc/q6v5-wcss.c | 784 +++++++++++++++++++++++++++++++++++++++++

Please keep the qcom_* naming scheme.

[..]
> diff --git a/drivers/remoteproc/q6v5-wcss.c b/drivers/remoteproc/q6v5-wcss.c
[..]
> +#include <linux/elf.h>

Doesn't look like you need this anymore.

[..]
> +#include <linux/qcom_scm.h>

You don't need this.

[..]
> +
> +#define WCSS_CRASH_REASON_SMEM 421
> +#define WCNSS_PAS_ID		6
> +#define STOP_ACK_TIMEOUT_MS 10000
> +
> +#define QDSP6SS_RST_EVB 0x10
> +#define QDSP6SS_RESET 0x14
> +#define QDSP6SS_DBG_CFG 0x18
> +#define QDSP6SS_XO_CBCR 0x38
> +#define QDSP6SS_MEM_PWR_CTL 0xb0
> +#define QDSP6SS_BHS_STATUS 0x78
> +#define TCSR_GLOBAL_CFG0 0x0
> +#define TCSR_GLOBAL_CFG1 0x4
> +
> +#define QDSP6SS_GFMUX_CTL 0x20
> +#define QDSP6SS_PWR_CTL 0x30
> +#define TCSR_HALTREQ 0x0
> +#define TCSR_HALTACK 0x4
> +#define TCSR_Q6_HALTREQ 0x0
> +#define TCSR_Q6_HALTACK 0x4
> +#define SSCAON_CONFIG 0x8
> +#define SSCAON_STATUS 0xc
> +#define HALTACK BIT(0)
> +#define BHS_EN_REST_ACK BIT(0)

It would be nice to have the values of these indented, to make things a
little bit easier to read.

[..]
> +static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
> +						  const struct firmware *fw,
> +						  int *tablesz)

You can omit this function, there's a dummy in qcom_common.[ch] with the
same name for the same purpose.

> +{
> +	static struct resource_table table = { .ver = 1, };
> +
> +	*tablesz = sizeof(table);
> +	return &table;
> +}
> +
> +static int q6v5_init_clocks(struct device *dev, struct q6v5 *qproc)
> +{
> +	int i;
> +	const char *cname;
> +	struct property *prop;
> +
> +	qproc->clk_cnt = of_property_count_strings(dev->of_node,
> +						   "clock-names");
> +
> +	if (!qproc->clk_cnt)
> +		return 0;

I strongly prefer that you explicitly list the clocks expected here,
rather than trusting DT.

And please transition this to the newly added clk-bulk interface (see
clk_bulk_get() et al).

> +
> +	qproc->clks = devm_kzalloc(dev, sizeof(*qproc->clks) * qproc->clk_cnt,
> +				   GFP_KERNEL);
> +
> +	of_property_for_each_string(dev->of_node, "clock-names", prop, cname) {
> +		struct clk *c = devm_clk_get(dev, cname);
> +
> +		if (IS_ERR_OR_NULL(c)) {
> +			if (PTR_ERR(c) != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to get %s clock\n", cname);
> +
> +			return PTR_ERR(c);
> +		}
> +
> +		qproc->clks[i++] = c;
> +	}
> +
> +	return 0;
> +}
> +
> +static int q6v5_clk_enable(struct q6v5 *qproc)
> +{
> +	int rc;
> +	int i;
> +
> +	for (i = 0; i < qproc->clk_cnt; i++) {
> +		rc = clk_prepare_enable(qproc->clks[i]);
> +		if (rc)
> +			goto err;
> +	}
> +
> +	return 0;
> +err:
> +	for (i--; i >= 0; i--)
> +		clk_disable_unprepare(qproc->clks[i]);
> +
> +	return rc;
> +}

As of v4.13-rc1 you can call clk_bulk_prepare_enable() instead of this
function.

> +
> +static int wcss_powerdown(struct q6v5 *qproc)
> +{
> +	unsigned int nretry = 0;
> +	unsigned int val = 0;
> +	int ret;
> +
> +	/* Assert WCSS/Q6 HALTREQ - 1 */

I presume the numbers at the end of these comments corresponds to the
steps in your programming manual, if so it's okay to keep them. But
please make move them to the front, like /* N: comment */

> +	nretry = 0;
> +
> +	ret = regmap_update_bits(qproc->tcsr, qproc->halt_wcss + TCSR_HALTREQ,
> +				 1, 1);

Is there a reason to do read-modify-write on this register? I use
regmap_write() in the other driver.

> +	if (ret)
> +		return ret;
> +
> +	while (1) {
> +		regmap_read(qproc->tcsr, qproc->halt_wcss + TCSR_HALTACK,
> +			    &val);
> +		if (val & HALTACK)
> +			break;
> +		mdelay(1);
> +		nretry++;
> +		if (nretry >= 10) {
> +			pr_warn("can't get TCSR haltACK\n");
> +			break;
> +		}
> +	}

Would it be possible to move q6v5proc_halt_axi_port() to qcom_common.c
(or a tcsr driver?) and use that instead?

> +
> +	/* Check HALTACK */

I presume this comment does not relate to the code that follows it?

> +	/* Set MPM_SSCAON_CONFIG 13 - 2 */

Shouldn't this be "Disable MPM_WCSSAON_CONFIG 13"?

> +	val = readl(qproc->mpm_base + SSCAON_CONFIG);
> +	val |= BIT(13);
> +	writel(val, qproc->mpm_base + SSCAON_CONFIG);
> +
> +	/* Set MPM_SSCAON_CONFIG 15 - 3 */
> +	val = readl(qproc->mpm_base + SSCAON_CONFIG);
> +	val |= BIT(15);
> +	val &= ~(BIT(16));
> +	val &= ~(BIT(17));
> +	val &= ~(BIT(18));

Skip all the extra parenthesis.

> +	writel(val, qproc->mpm_base + SSCAON_CONFIG);
> +
> +	/* Set MPM_SSCAON_CONFIG 1 - 4 */
> +	val = readl(qproc->mpm_base + SSCAON_CONFIG);
> +	val |= BIT(1);
> +	writel(val, qproc->mpm_base + SSCAON_CONFIG);
> +
> +	/* wait for SSCAON_STATUS to be 0x400 - 5 */
> +	nretry = 0;
> +	while (1) {
> +		val = readl(qproc->mpm_base + SSCAON_STATUS);
> +		/* ignore bits 16 to 31 */
> +		val &= 0xffff;
> +		if (val == BIT(10))
> +			break;
> +		nretry++;
> +		mdelay(1);
> +		if (nretry == 10) {
> +			pr_warn("can't get SSCAON_STATUS\n");
> +			break;
> +		}
> +	}

Please replace this loop with:
	ret = readl_poll_timeout(qproc->mpm_base + SSCAON_STATUS, val,
				 val & 0xffff == 0x400, 1000, 10000);

> +
> +	/* Enable Q6/WCSS BLOCK ARES - 6 */
> +	reset_control_assert(qproc->wcss_aon_reset);
> +
> +	/* Enable MPM_WCSSAON_CONFIG 13 - 7 */
> +	val = readl(qproc->mpm_base + SSCAON_CONFIG);
> +	val &= (~(BIT(13)));

Skip all the extra parenthesis.

> +	writel(val, qproc->mpm_base + SSCAON_CONFIG);
> +
> +	/* Enable A2AB/ACMT/ECHAB ARES - 8 */
> +	/* De-assert WCSS/Q6 HALTREQ - 8 */
> +	reset_control_assert(qproc->wcss_reset);
> +
> +	ret = regmap_update_bits(qproc->tcsr, qproc->halt_wcss + TCSR_HALTREQ,
> +				 1, 0);
> +
> +	return ret;
> +}
> +
> +static int q6_powerdown(struct q6v5 *qproc)
> +{
> +	int i = 0, ret;
> +	unsigned int nretry = 0;
> +	unsigned int val = 0;
> +
> +	/* Halt Q6 bus interface - 9*/
> +	ret = regmap_update_bits(qproc->tcsr, qproc->halt_q6 + TCSR_Q6_HALTREQ,
> +				 1, 1);
> +	if (ret)
> +		return ret;
> +
> +	nretry = 0;
> +	while (1) {
> +		regmap_read(qproc->tcsr, qproc->halt_q6 + TCSR_Q6_HALTACK,
> +			    &val);
> +		if (val & HALTACK)
> +			break;
> +		mdelay(1);
> +		nretry++;
> +		if (nretry >= 10) {
> +			pr_err("can't get TCSR Q6 haltACK\n");
> +			break;
> +		}
> +	}

Again, can we utilize q6v5proc_halt_axi_port()? (Or replace the tcsr
syscon with a driver)

> +
> +	/* Disable Q6 Core clock - 10 */
> +	val = readl(qproc->q6_base + QDSP6SS_GFMUX_CTL);
> +	val &= (~(BIT(1)));

Parenthesis.

> +	writel(val, qproc->q6_base + QDSP6SS_GFMUX_CTL);
> +
> +	/* Clamp I/O - 11 */
> +	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
> +	val |= BIT(20);
> +	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
> +
> +	/* Clamp WL - 12 */
> +	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
> +	val |= BIT(21);
> +	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
> +
> +	/* Clear Erase standby - 13 */
> +	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
> +	val &= (~(BIT(18)));
> +	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
> +
> +	/* Clear Sleep RTN - 14 */
> +	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
> +	val &= (~(BIT(19)));
> +	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
> +
> +	/* turn off QDSP6 memory foot/head switch one bank at a time - 15*/
> +	for (i = 0; i < 20; i++) {
> +		val = readl(qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
> +		val &= (~(BIT(i)));

Parenthesis.

> +		writel(val, qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
> +		mdelay(1);
> +	}
> +
> +	/* Assert QMC memory RTN - 16 */
> +	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
> +	val |= BIT(22);
> +	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
> +
> +	/* Turn off BHS - 17 */
> +	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
> +	val &= (~(BIT(24)));
> +	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
> +	udelay(1);
> +	/* Wait till BHS Reset is done */
> +	nretry = 0;
> +	while (1) {
> +		val = readl(qproc->q6_base + QDSP6SS_BHS_STATUS);
> +		if (!(val & BHS_EN_REST_ACK))
> +			break;
> +		mdelay(1);
> +		nretry++;
> +		if (nretry >= 10) {
> +			pr_err("BHS_STATUS not OFF\n");
> +			break;
> +		}
> +	}

readl_poll_timeout()

> +
> +	/* HALT CLEAR - 18 */
> +	ret = regmap_update_bits(qproc->tcsr, qproc->halt_q6 + TCSR_Q6_HALTREQ,
> +				 1, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable Q6 Block reset - 19 */
> +	reset_control_assert(qproc->wcss_q6_reset);
> +
> +	return 0;
> +}
> +
> +static int q6_rproc_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 = wcss_powerdown(qproc);
> +	if (ret)
> +		return ret;
> +
> +	/* Q6 Power down */
> +	ret = q6_powerdown(qproc);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int q6_rproc_start(struct rproc *rproc)
> +{
> +	struct q6v5 *qproc = rproc->priv;
> +	int temp = 19;
> +	unsigned long val = 0;
> +	unsigned int nretry = 0;
> +	int ret = 0;
> +
> +	ret = q6v5_clk_enable(qproc);
> +	if (ret) {
> +		dev_err(qproc->dev, "failed to enable clocks\n");
> +		return ret;
> +	}
> +
> +	/* Release Q6 and WCSS reset */
> +	reset_control_deassert(qproc->wcss_reset);
> +	reset_control_deassert(qproc->wcss_q6_reset);
> +
> +	/* Lithium configuration - clock gating and bus arbitration */

Should this go in a tcsr driver?

> +	ret = regmap_update_bits(qproc->tcsr,
> +				 qproc->halt_gbl + TCSR_GLOBAL_CFG0,
> +				 0x1F, 0x14);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(qproc->tcsr,
> +				 qproc->halt_gbl + 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->q6_base + QDSP6SS_RST_EVB);
> +	/* Turn on XO clock. It is required for BHS and memory operation */
> +	writel(0x1, qproc->q6_base + QDSP6SS_XO_CBCR);
> +	/* Turn on BHS */
> +	writel(0x1700000, qproc->q6_base + QDSP6SS_PWR_CTL);

Avaneesh provided defines for most of these magic numbers, please follow
that.

> +	udelay(1);
> +
> +	/* Wait till BHS Reset is done */
> +	while (1) {
> +		val = readl(qproc->q6_base + QDSP6SS_BHS_STATUS);
> +		if (val & BHS_EN_REST_ACK)
> +			break;
> +		mdelay(1);
> +		nretry++;
> +		if (nretry >= 10) {
> +			pr_err("BHS_STATUS not ON\n");
> +			break;
> +		}
> +	}

Use readl_poll_timeout()

> +
> +	/* Put LDO in bypass mode */
> +	writel(0x3700000, qproc->q6_base + QDSP6SS_PWR_CTL);
> +	/* De-assert QDSP6 complier memory clamp */
> +	writel(0x3300000, qproc->q6_base + QDSP6SS_PWR_CTL);
> +	/* De-assert memory peripheral sleep and L2 memory standby */
> +	writel(0x33c0000, qproc->q6_base + QDSP6SS_PWR_CTL);
> +
> +	/* turn on QDSP6 memory foot/head switch one bank at a time */
> +	while  (temp >= 0) {

Please use a for loop, and replace "temp" with "i".

This is identical to Avaneesh patch, so let's make sure the code is
identical as well.


> +		val = readl(qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
> +		val = val | 1 << temp;

val |= BIT(temp);

> +		writel(val, qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
> +		val = readl(qproc->q6_base + QDSP6SS_MEM_PWR_CTL);

Please include a comment on the read back here.

> +		mdelay(10);
> +		temp -= 1;
> +	}
> +	/* Remove the QDSP6 core memory word line clamp */
> +	writel(0x31FFFFF, qproc->q6_base + QDSP6SS_PWR_CTL);
> +	/* Remove QDSP6 I/O clamp */
> +	writel(0x30FFFFF, qproc->q6_base + QDSP6SS_PWR_CTL);
> +
> +	/* Bring Q6 out of reset and stop the core */
> +	writel(0x5, qproc->q6_base + QDSP6SS_RESET);
> +
> +	/* Retain debugger state during next QDSP6 reset */
> +	writel(0x0, qproc->q6_base + QDSP6SS_DBG_CFG);
> +	/* Turn on the QDSP6 core clock */
> +	writel(0x102, qproc->q6_base + QDSP6SS_GFMUX_CTL);
> +	/* Enable the core to run */
> +	writel(0x4, qproc->q6_base + QDSP6SS_RESET);
> +
> +	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 struct rproc_ops q6v5_rproc_ops = {
> +	.start		= q6_rproc_start,
> +	.stop		= q6_rproc_stop,
> +};
> +
> +static struct rproc_fw_ops q6_fw_ops;
> +
> +static int q6v5_request_irq(struct q6v5 *qproc,
> +			    struct platform_device *pdev,
> +			    const char *name,
> +			    irq_handler_t thread_fn)
> +{
> +	int ret;
> +
> +	ret = platform_get_irq_byname(pdev, name);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "no %s IRQ defined\n", name);
> +		return ret;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, ret,
> +					NULL, thread_fn,
> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +					"q6v5", qproc);
> +	if (ret)
> +		dev_err(&pdev->dev, "request %s IRQ failed\n", name);
> +
> +	return ret;
> +}
> +
> +static irqreturn_t q6v5_fatal_interrupt(int irq, void *dev)
> +{
> +	struct q6v5 *qproc = dev;
> +	char *msg;
> +	size_t len;
> +
> +	if (!qproc->running)
> +		return IRQ_HANDLED;
> +
> +	msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, WCSS_CRASH_REASON_SMEM, &len);
> +	if (!IS_ERR(msg) && len > 0 && msg[0])
> +		dev_err(qproc->dev, "Fatal error from wcss: %s\n", msg);
> +	else
> +		dev_err(qproc->dev, "Fatal error received no message!\n");
> +
> +	rproc_report_crash(qproc->rproc, RPROC_FATAL_ERROR);
> +
> +	if (!IS_ERR(msg))
> +		msg[0] = '\0';
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t q6v5_handover_interrupt(int irq, void *dev)
> +{
> +	struct q6v5 *qproc = dev;
> +
> +	complete(&qproc->start_done);
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t q6v5_stop_ack_interrupt(int irq, void *dev)
> +{
> +	struct q6v5 *qproc = dev;
> +
> +	complete(&qproc->stop_done);
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t q6v5_wdog_interrupt(int irq, void *dev)
> +{
> +	struct q6v5 *qproc = dev;
> +	char *msg;
> +	size_t len;
> +
> +	if (!qproc->running) {
> +		complete(&qproc->stop_done);
> +		return IRQ_HANDLED;
> +	}
> +
> +	msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, WCSS_CRASH_REASON_SMEM, &len);
> +	if (!IS_ERR(msg) && len > 0 && msg[0])
> +		dev_err(qproc->dev, "Watchdog bite from wcss %s\n", msg);
> +	else
> +		dev_err(qproc->dev, "Watchdog bit received no message!\n");
> +
> +	rproc_report_crash(qproc->rproc, RPROC_WATCHDOG);
> +
> +	if (!IS_ERR(msg))
> +		msg[0] = '\0';
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
> +{
> +	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
> +
> +	return qcom_mdt_load(qproc->dev, fw, rproc->firmware, WCNSS_PAS_ID,
> +			     qproc->mem_region, qproc->mem_phys,
> +			     qproc->mem_size, false);
> +}
> +
> +static int q6_alloc_memory_region(struct q6v5 *qproc)
> +{
> +	struct device_node *node;
> +	struct resource r;
> +	int ret;
> +
> +	node = of_parse_phandle(qproc->dev->of_node, "memory-region", 0);
> +	if (!node) {
> +		dev_err(qproc->dev, "no memory-region specified\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_address_to_resource(node, 0, &r);
> +	if (ret)
> +		return ret;
> +
> +	qproc->mem_phys = r.start;
> +	qproc->mem_size = resource_size(&r);
> +	qproc->mem_region = devm_ioremap_wc(qproc->dev, qproc->mem_phys,
> +					    qproc->mem_size);
> +	if (!qproc->mem_region) {
> +		dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
> +			&r.start, qproc->mem_size);
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}
> +
> +static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev)
> +{
> +	struct of_phandle_args args;
> +	struct resource *res;
> +	int ret;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +					   "mpm");
> +	if (IS_ERR_OR_NULL(res))
> +		return -ENODEV;
> +
> +	qproc->mpm_base = ioremap(res->start, resource_size(res));
> +	if (IS_ERR_OR_NULL(qproc->mpm_base))
> +		return PTR_ERR(qproc->mpm_base);

Is this the same MPM block that is used to configure sleep related
things later? If so I think we shouldn't map it directly here, but
introduce a separate driver for it.

> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +					   "q6");
> +	if (IS_ERR_OR_NULL(res)) {
> +		ret = -ENODEV;
> +		goto free_mpm;
> +	}
> +
> +	qproc->q6_base = ioremap(res->start, resource_size(res));

Except for the error path of this function these memory regions are
never unmapped. Please use devm_ioremap_wc() instead.

> +	if (IS_ERR_OR_NULL(qproc->q6_base)) {
> +		ret = PTR_ERR(qproc->q6_base);
> +		goto free_mpm;
> +	}
> +
> +	ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
> +					       "qcom,halt-regs", 3,
> +					       0, &args);
> +	if (ret < 0)
> +		goto free_q6;
> +
> +	qproc->tcsr = syscon_node_to_regmap(args.np);
> +	of_node_put(args.np);
> +	if (IS_ERR_OR_NULL(qproc->tcsr)) {
> +		ret = PTR_ERR(qproc->tcsr);
> +		goto free_q6;
> +	}
> +
> +	qproc->halt_gbl = args.args[0];
> +	qproc->halt_q6 = args.args[1];
> +	qproc->halt_wcss = args.args[2];
> +
> +	return 0;
> +
> +free_q6:
> +	iounmap(qproc->q6_base);
> +
> +free_mpm:
> +	iounmap(qproc->mpm_base);
> +
> +	return ret;
> +}
> +
> +static int q6_rproc_probe(struct platform_device *pdev)
> +{
> +	struct q6v5 *qproc;
> +	struct rproc *rproc;
> +	int ret;
> +	struct qcom_smem_state *state;
> +	unsigned int stop_bit;
> +	const char *firmware_name = of_device_get_match_data(&pdev->dev);
> +
> +	state = qcom_smem_state_get(&pdev->dev, "stop",
> +				    &stop_bit);
> +	if (IS_ERR(state))
> +		/* Wait till SMP2P is registered and up */
> +		return -EPROBE_DEFER;
> +
> +	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_rproc_ops,
> +			    firmware_name,
> +			    sizeof(*qproc));
> +	if (unlikely(!rproc))
> +		return -ENOMEM;
> +
> +	qproc = rproc->priv;
> +	qproc->dev = &pdev->dev;
> +	qproc->rproc = rproc;
> +	rproc->has_iommu = false;
> +
> +	q6_fw_ops = *rproc->fw_ops;
> +	q6_fw_ops.find_rsc_table = q6v5_find_rsc_table;
> +	q6_fw_ops.load = q6v5_load;

I would prefer that you do define a static const object with these, like
in the other qcom drivers.

But in order to do that you would need to export
rproc_elf_get_boot_addr(), which is in line with another item on my todo
list, so please do this.

[..]
> +
> +free_rproc:
> +	rproc_put(rproc);

Use rproc_free() instead of rproc_put() to free up resources of
rproc_alloc()

> +	return -EIO;
> +}
> +
> +static int q6_rproc_remove(struct platform_device *pdev)
> +{
> +	struct q6v5 *qproc;
> +	struct rproc *rproc;
> +
> +	qproc = platform_get_drvdata(pdev);
> +	rproc = qproc->rproc;
> +
> +	rproc_del(rproc);
> +	rproc_put(rproc);

rproc_free()

> +
> +	return 0;
> +}
> +

Regards,
Bjorn

Re: [PATCH 2/3] drivers: remoteproc: Add Hexagon Q6 - WCSS integrated core driver

[Sricharan R]

Hi Bjorn,

On 7/27/2017 12:38 AM, Bjorn Andersson wrote:
> On Thu 29 Jun 07:17 PDT 2017, Sricharan R wrote:
> 
>> IPQ8074 has an integrated Hexagon dsp core Q6v5 and a wireless lan
>> (Lithium) IP. This patch adds the remoteproc driver to reset, load
>> and boot Q6 firmware.
>>
> 
> There is a fair amount of code in this driver that seems to be
> equivalent to Avaneesh q6v5 patches for MSM8996.
> 
> The differences coming from the MBA + MPSS vs single-image we have to
> live with, but can we do something about the Q6 programming sequences?
> E.g. extract them to a common file?
> 

 hmm, initially was trying to do that, but Q6 programming sequence was
 not that much common. So added this as a new driver. But let me try
 once more.

>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
>> ---
>>  drivers/remoteproc/Kconfig     |  13 +
>>  drivers/remoteproc/Makefile    |   1 +
>>  drivers/remoteproc/q6v5-wcss.c | 784 +++++++++++++++++++++++++++++++++++++++++
> 
> Please keep the qcom_* naming scheme.
>

 ok
 
> [..]
>> diff --git a/drivers/remoteproc/q6v5-wcss.c b/drivers/remoteproc/q6v5-wcss.c
> [..]
>> +#include <linux/elf.h>
> 
> Doesn't look like you need this anymore.
> 

 ok

> [..]
>> +#include <linux/qcom_scm.h>
> 
> You don't need this.
>

 right, will remove
 
> [..]
>> +
>> +#define WCSS_CRASH_REASON_SMEM 421
>> +#define WCNSS_PAS_ID		6
>> +#define STOP_ACK_TIMEOUT_MS 10000
>> +
>> +#define QDSP6SS_RST_EVB 0x10
>> +#define QDSP6SS_RESET 0x14
>> +#define QDSP6SS_DBG_CFG 0x18
>> +#define QDSP6SS_XO_CBCR 0x38
>> +#define QDSP6SS_MEM_PWR_CTL 0xb0
>> +#define QDSP6SS_BHS_STATUS 0x78
>> +#define TCSR_GLOBAL_CFG0 0x0
>> +#define TCSR_GLOBAL_CFG1 0x4
>> +
>> +#define QDSP6SS_GFMUX_CTL 0x20
>> +#define QDSP6SS_PWR_CTL 0x30
>> +#define TCSR_HALTREQ 0x0
>> +#define TCSR_HALTACK 0x4
>> +#define TCSR_Q6_HALTREQ 0x0
>> +#define TCSR_Q6_HALTACK 0x4
>> +#define SSCAON_CONFIG 0x8
>> +#define SSCAON_STATUS 0xc
>> +#define HALTACK BIT(0)
>> +#define BHS_EN_REST_ACK BIT(0)
> 
> It would be nice to have the values of these indented, to make things a
> little bit easier to read.
> 

 ok

> [..]
>> +static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
>> +						  const struct firmware *fw,
>> +						  int *tablesz)
> 
> You can omit this function, there's a dummy in qcom_common.[ch] with the
> same name for the same purpose.
>

 ok
 
>> +{
>> +	static struct resource_table table = { .ver = 1, };
>> +
>> +	*tablesz = sizeof(table);
>> +	return &table;
>> +}
>> +
>> +static int q6v5_init_clocks(struct device *dev, struct q6v5 *qproc)
>> +{
>> +	int i;
>> +	const char *cname;
>> +	struct property *prop;
>> +
>> +	qproc->clk_cnt = of_property_count_strings(dev->of_node,
>> +						   "clock-names");
>> +
>> +	if (!qproc->clk_cnt)
>> +		return 0;
> 
> I strongly prefer that you explicitly list the clocks expected here,
> rather than trusting DT.
> 

 ok.

> And please transition this to the newly added clk-bulk interface (see
> clk_bulk_get() et al).
>

 ok.
 
>> +
>> +	qproc->clks = devm_kzalloc(dev, sizeof(*qproc->clks) * qproc->clk_cnt,
>> +				   GFP_KERNEL);
>> +
>> +	of_property_for_each_string(dev->of_node, "clock-names", prop, cname) {
>> +		struct clk *c = devm_clk_get(dev, cname);
>> +
>> +		if (IS_ERR_OR_NULL(c)) {
>> +			if (PTR_ERR(c) != -EPROBE_DEFER)
>> +				dev_err(dev, "Failed to get %s clock\n", cname);
>> +
>> +			return PTR_ERR(c);
>> +		}
>> +
>> +		qproc->clks[i++] = c;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int q6v5_clk_enable(struct q6v5 *qproc)
>> +{
>> +	int rc;
>> +	int i;
>> +
>> +	for (i = 0; i < qproc->clk_cnt; i++) {
>> +		rc = clk_prepare_enable(qproc->clks[i]);
>> +		if (rc)
>> +			goto err;
>> +	}
>> +
>> +	return 0;
>> +err:
>> +	for (i--; i >= 0; i--)
>> +		clk_disable_unprepare(qproc->clks[i]);
>> +
>> +	return rc;
>> +}
> 
> As of v4.13-rc1 you can call clk_bulk_prepare_enable() instead of this
> function.
> 

 ok.

>> +
>> +static int wcss_powerdown(struct q6v5 *qproc)
>> +{
>> +	unsigned int nretry = 0;
>> +	unsigned int val = 0;
>> +	int ret;
>> +
>> +	/* Assert WCSS/Q6 HALTREQ - 1 */
> 
> I presume the numbers at the end of these comments corresponds to the
> steps in your programming manual, if so it's okay to keep them. But
> please make move them to the front, like /* N: comment */
>

 right, it corresponds to the manual. Will change as suggested.
 
>> +	nretry = 0;
>> +
>> +	ret = regmap_update_bits(qproc->tcsr, qproc->halt_wcss + TCSR_HALTREQ,
>> +				 1, 1);
> 
> Is there a reason to do read-modify-write on this register? I use
> regmap_write() in the other driver.
> 

 Just did a ditto of what was mentioned in the manual. May be a direct write
 would also suffice. Will check and update if it works.

>> +	if (ret)
>> +		return ret;
>> +
>> +	while (1) {
>> +		regmap_read(qproc->tcsr, qproc->halt_wcss + TCSR_HALTACK,
>> +			    &val);
>> +		if (val & HALTACK)
>> +			break;
>> +		mdelay(1);
>> +		nretry++;
>> +		if (nretry >= 10) {
>> +			pr_warn("can't get TCSR haltACK\n");
>> +			break;
>> +		}
>> +	}
> 
> Would it be possible to move q6v5proc_halt_axi_port() to qcom_common.c
> (or a tcsr driver?) and use that instead?
> 

 ok, qcom_common.c, this can be used in both qcom_q6v5_pil.c as well.

>> +
>> +	/* Check HALTACK */
> 
> I presume this comment does not relate to the code that follows it?
> 

 ok, will remove.

>> +	/* Set MPM_SSCAON_CONFIG 13 - 2 */
> 
> Shouldn't this be "Disable MPM_WCSSAON_CONFIG 13"?
> 
 Right, will update.

>> +	val = readl(qproc->mpm_base + SSCAON_CONFIG);
>> +	val |= BIT(13);
>> +	writel(val, qproc->mpm_base + SSCAON_CONFIG);
>> +
>> +	/* Set MPM_SSCAON_CONFIG 15 - 3 */
>> +	val = readl(qproc->mpm_base + SSCAON_CONFIG);
>> +	val |= BIT(15);
>> +	val &= ~(BIT(16));
>> +	val &= ~(BIT(17));
>> +	val &= ~(BIT(18));
> 
> Skip all the extra parenthesis.
> 

 ok.

>> +	writel(val, qproc->mpm_base + SSCAON_CONFIG);
>> +
>> +	/* Set MPM_SSCAON_CONFIG 1 - 4 */
>> +	val = readl(qproc->mpm_base + SSCAON_CONFIG);
>> +	val |= BIT(1);
>> +	writel(val, qproc->mpm_base + SSCAON_CONFIG);
>> +
>> +	/* wait for SSCAON_STATUS to be 0x400 - 5 */
>> +	nretry = 0;
>> +	while (1) {
>> +		val = readl(qproc->mpm_base + SSCAON_STATUS);
>> +		/* ignore bits 16 to 31 */
>> +		val &= 0xffff;
>> +		if (val == BIT(10))
>> +			break;
>> +		nretry++;
>> +		mdelay(1);
>> +		if (nretry == 10) {
>> +			pr_warn("can't get SSCAON_STATUS\n");
>> +			break;
>> +		}
>> +	}
> 
> Please replace this loop with:
> 	ret = readl_poll_timeout(qproc->mpm_base + SSCAON_STATUS, val,
> 				 val & 0xffff == 0x400, 1000, 10000);
>

 sure, thanks.
 
>> +
>> +	/* Enable Q6/WCSS BLOCK ARES - 6 */
>> +	reset_control_assert(qproc->wcss_aon_reset);
>> +
>> +	/* Enable MPM_WCSSAON_CONFIG 13 - 7 */
>> +	val = readl(qproc->mpm_base + SSCAON_CONFIG);
>> +	val &= (~(BIT(13)));
> 
> Skip all the extra parenthesis.
> 

 ok.

>> +	writel(val, qproc->mpm_base + SSCAON_CONFIG);
>> +
>> +	/* Enable A2AB/ACMT/ECHAB ARES - 8 */
>> +	/* De-assert WCSS/Q6 HALTREQ - 8 */
>> +	reset_control_assert(qproc->wcss_reset);
>> +
>> +	ret = regmap_update_bits(qproc->tcsr, qproc->halt_wcss + TCSR_HALTREQ,
>> +				 1, 0);
>> +
>> +	return ret;
>> +}
>> +
>> +static int q6_powerdown(struct q6v5 *qproc)
>> +{
>> +	int i = 0, ret;
>> +	unsigned int nretry = 0;
>> +	unsigned int val = 0;
>> +
>> +	/* Halt Q6 bus interface - 9*/
>> +	ret = regmap_update_bits(qproc->tcsr, qproc->halt_q6 + TCSR_Q6_HALTREQ,
>> +				 1, 1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	nretry = 0;
>> +	while (1) {
>> +		regmap_read(qproc->tcsr, qproc->halt_q6 + TCSR_Q6_HALTACK,
>> +			    &val);
>> +		if (val & HALTACK)
>> +			break;
>> +		mdelay(1);
>> +		nretry++;
>> +		if (nretry >= 10) {
>> +			pr_err("can't get TCSR Q6 haltACK\n");
>> +			break;
>> +		}
>> +	}
> 
> Again, can we utilize q6v5proc_halt_axi_port()? (Or replace the tcsr
> syscon with a driver)
> 
   Ya, the halt_axi_port across both of these Q6 drivers can be put in
   qcom_common.c.
 
>> +
>> +	/* Disable Q6 Core clock - 10 */
>> +	val = readl(qproc->q6_base + QDSP6SS_GFMUX_CTL);
>> +	val &= (~(BIT(1)));
> 
> Parenthesis.
>

 ok.

 
>> +	writel(val, qproc->q6_base + QDSP6SS_GFMUX_CTL);
>> +
>> +	/* Clamp I/O - 11 */
>> +	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
>> +	val |= BIT(20);
>> +	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +
>> +	/* Clamp WL - 12 */
>> +	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
>> +	val |= BIT(21);
>> +	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +
>> +	/* Clear Erase standby - 13 */
>> +	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
>> +	val &= (~(BIT(18)));
>> +	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +
>> +	/* Clear Sleep RTN - 14 */
>> +	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
>> +	val &= (~(BIT(19)));
>> +	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +
>> +	/* turn off QDSP6 memory foot/head switch one bank at a time - 15*/
>> +	for (i = 0; i < 20; i++) {
>> +		val = readl(qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
>> +		val &= (~(BIT(i)));
> 
> Parenthesis.
>

 ok.
 
>> +		writel(val, qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
>> +		mdelay(1);
>> +	}
>> +
>> +	/* Assert QMC memory RTN - 16 */
>> +	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
>> +	val |= BIT(22);
>> +	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +
>> +	/* Turn off BHS - 17 */
>> +	val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
>> +	val &= (~(BIT(24)));
>> +	writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +	udelay(1);
>> +	/* Wait till BHS Reset is done */
>> +	nretry = 0;
>> +	while (1) {
>> +		val = readl(qproc->q6_base + QDSP6SS_BHS_STATUS);
>> +		if (!(val & BHS_EN_REST_ACK))
>> +			break;
>> +		mdelay(1);
>> +		nretry++;
>> +		if (nretry >= 10) {
>> +			pr_err("BHS_STATUS not OFF\n");
>> +			break;
>> +		}
>> +	}
> 
> readl_poll_timeout()
> 

 ok.

>> +
>> +	/* HALT CLEAR - 18 */
>> +	ret = regmap_update_bits(qproc->tcsr, qproc->halt_q6 + TCSR_Q6_HALTREQ,
>> +				 1, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Enable Q6 Block reset - 19 */
>> +	reset_control_assert(qproc->wcss_q6_reset);
>> +
>> +	return 0;
>> +}
>> +
>> +static int q6_rproc_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 = wcss_powerdown(qproc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Q6 Power down */
>> +	ret = q6_powerdown(qproc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int q6_rproc_start(struct rproc *rproc)
>> +{
>> +	struct q6v5 *qproc = rproc->priv;
>> +	int temp = 19;
>> +	unsigned long val = 0;
>> +	unsigned int nretry = 0;
>> +	int ret = 0;
>> +
>> +	ret = q6v5_clk_enable(qproc);
>> +	if (ret) {
>> +		dev_err(qproc->dev, "failed to enable clocks\n");
>> +		return ret;
>> +	}
>> +
>> +	/* Release Q6 and WCSS reset */
>> +	reset_control_deassert(qproc->wcss_reset);
>> +	reset_control_deassert(qproc->wcss_q6_reset);
>> +
>> +	/* Lithium configuration - clock gating and bus arbitration */
> 
> Should this go in a tcsr driver?
> 

 Not sure i understand this. So you mean we should have a driver that
 wrappers the access to tcsr registers. So that means that driver
 populates the syscon_to_regmap and passes back the regmap handle.
 Then the client driver like Q6 uses tcsr_ apis on top of that ?

>> +	ret = regmap_update_bits(qproc->tcsr,
>> +				 qproc->halt_gbl + TCSR_GLOBAL_CFG0,
>> +				 0x1F, 0x14);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_update_bits(qproc->tcsr,
>> +				 qproc->halt_gbl + 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->q6_base + QDSP6SS_RST_EVB);
>> +	/* Turn on XO clock. It is required for BHS and memory operation */
>> +	writel(0x1, qproc->q6_base + QDSP6SS_XO_CBCR);
>> +	/* Turn on BHS */
>> +	writel(0x1700000, qproc->q6_base + QDSP6SS_PWR_CTL);
> 
> Avaneesh provided defines for most of these magic numbers, please follow
> that.
>

 ok.
 
>> +	udelay(1);
>> +
>> +	/* Wait till BHS Reset is done */
>> +	while (1) {
>> +		val = readl(qproc->q6_base + QDSP6SS_BHS_STATUS);
>> +		if (val & BHS_EN_REST_ACK)
>> +			break;
>> +		mdelay(1);
>> +		nretry++;
>> +		if (nretry >= 10) {
>> +			pr_err("BHS_STATUS not ON\n");
>> +			break;
>> +		}
>> +	}
> 
> Use readl_poll_timeout()
>

 ok.
 
>> +
>> +	/* Put LDO in bypass mode */
>> +	writel(0x3700000, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +	/* De-assert QDSP6 complier memory clamp */
>> +	writel(0x3300000, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +	/* De-assert memory peripheral sleep and L2 memory standby */
>> +	writel(0x33c0000, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +
>> +	/* turn on QDSP6 memory foot/head switch one bank at a time */
>> +	while  (temp >= 0) {
> 
> Please use a for loop, and replace "temp" with "i".
> 

 ok.

> This is identical to Avaneesh patch, so let's make sure the code is
> identical as well.
>

 ok.
 
> 
>> +		val = readl(qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
>> +		val = val | 1 << temp;
> 
> val |= BIT(temp);
> 
>> +		writel(val, qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
>> +		val = readl(qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
> 
> Please include a comment on the read back here.
> 

 ok.

>> +		mdelay(10);
>> +		temp -= 1;
>> +	}
>> +	/* Remove the QDSP6 core memory word line clamp */
>> +	writel(0x31FFFFF, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +	/* Remove QDSP6 I/O clamp */
>> +	writel(0x30FFFFF, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +
>> +	/* Bring Q6 out of reset and stop the core */
>> +	writel(0x5, qproc->q6_base + QDSP6SS_RESET);
>> +
>> +	/* Retain debugger state during next QDSP6 reset */
>> +	writel(0x0, qproc->q6_base + QDSP6SS_DBG_CFG);
>> +	/* Turn on the QDSP6 core clock */
>> +	writel(0x102, qproc->q6_base + QDSP6SS_GFMUX_CTL);
>> +	/* Enable the core to run */
>> +	writel(0x4, qproc->q6_base + QDSP6SS_RESET);
>> +
>> +	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 struct rproc_ops q6v5_rproc_ops = {
>> +	.start		= q6_rproc_start,
>> +	.stop		= q6_rproc_stop,
>> +};
>> +
>> +static struct rproc_fw_ops q6_fw_ops;
>> +
>> +static int q6v5_request_irq(struct q6v5 *qproc,
>> +			    struct platform_device *pdev,
>> +			    const char *name,
>> +			    irq_handler_t thread_fn)
>> +{
>> +	int ret;
>> +
>> +	ret = platform_get_irq_byname(pdev, name);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "no %s IRQ defined\n", name);
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_request_threaded_irq(&pdev->dev, ret,
>> +					NULL, thread_fn,
>> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> +					"q6v5", qproc);
>> +	if (ret)
>> +		dev_err(&pdev->dev, "request %s IRQ failed\n", name);
>> +
>> +	return ret;
>> +}
>> +
>> +static irqreturn_t q6v5_fatal_interrupt(int irq, void *dev)
>> +{
>> +	struct q6v5 *qproc = dev;
>> +	char *msg;
>> +	size_t len;
>> +
>> +	if (!qproc->running)
>> +		return IRQ_HANDLED;
>> +
>> +	msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, WCSS_CRASH_REASON_SMEM, &len);
>> +	if (!IS_ERR(msg) && len > 0 && msg[0])
>> +		dev_err(qproc->dev, "Fatal error from wcss: %s\n", msg);
>> +	else
>> +		dev_err(qproc->dev, "Fatal error received no message!\n");
>> +
>> +	rproc_report_crash(qproc->rproc, RPROC_FATAL_ERROR);
>> +
>> +	if (!IS_ERR(msg))
>> +		msg[0] = '\0';
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t q6v5_handover_interrupt(int irq, void *dev)
>> +{
>> +	struct q6v5 *qproc = dev;
>> +
>> +	complete(&qproc->start_done);
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t q6v5_stop_ack_interrupt(int irq, void *dev)
>> +{
>> +	struct q6v5 *qproc = dev;
>> +
>> +	complete(&qproc->stop_done);
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t q6v5_wdog_interrupt(int irq, void *dev)
>> +{
>> +	struct q6v5 *qproc = dev;
>> +	char *msg;
>> +	size_t len;
>> +
>> +	if (!qproc->running) {
>> +		complete(&qproc->stop_done);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, WCSS_CRASH_REASON_SMEM, &len);
>> +	if (!IS_ERR(msg) && len > 0 && msg[0])
>> +		dev_err(qproc->dev, "Watchdog bite from wcss %s\n", msg);
>> +	else
>> +		dev_err(qproc->dev, "Watchdog bit received no message!\n");
>> +
>> +	rproc_report_crash(qproc->rproc, RPROC_WATCHDOG);
>> +
>> +	if (!IS_ERR(msg))
>> +		msg[0] = '\0';
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>> +{
>> +	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>> +
>> +	return qcom_mdt_load(qproc->dev, fw, rproc->firmware, WCNSS_PAS_ID,
>> +			     qproc->mem_region, qproc->mem_phys,
>> +			     qproc->mem_size, false);
>> +}
>> +
>> +static int q6_alloc_memory_region(struct q6v5 *qproc)
>> +{
>> +	struct device_node *node;
>> +	struct resource r;
>> +	int ret;
>> +
>> +	node = of_parse_phandle(qproc->dev->of_node, "memory-region", 0);
>> +	if (!node) {
>> +		dev_err(qproc->dev, "no memory-region specified\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = of_address_to_resource(node, 0, &r);
>> +	if (ret)
>> +		return ret;
>> +
>> +	qproc->mem_phys = r.start;
>> +	qproc->mem_size = resource_size(&r);
>> +	qproc->mem_region = devm_ioremap_wc(qproc->dev, qproc->mem_phys,
>> +					    qproc->mem_size);
>> +	if (!qproc->mem_region) {
>> +		dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
>> +			&r.start, qproc->mem_size);
>> +		return -EBUSY;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev)
>> +{
>> +	struct of_phandle_args args;
>> +	struct resource *res;
>> +	int ret;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +					   "mpm");
>> +	if (IS_ERR_OR_NULL(res))
>> +		return -ENODEV;
>> +
>> +	qproc->mpm_base = ioremap(res->start, resource_size(res));
>> +	if (IS_ERR_OR_NULL(qproc->mpm_base))
>> +		return PTR_ERR(qproc->mpm_base);
> 
> Is this the same MPM block that is used to configure sleep related
> things later? If so I think we shouldn't map it directly here, but
> introduce a separate driver for it.
> 

 Yeah, thats the one. But in this context it is used to configure
 some magic register. The downstream's irqchip type of functionality
 is not used here. Infact there may not be a usecase at all for this.

>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +					   "q6");
>> +	if (IS_ERR_OR_NULL(res)) {
>> +		ret = -ENODEV;
>> +		goto free_mpm;
>> +	}
>> +
>> +	qproc->q6_base = ioremap(res->start, resource_size(res));
> 
> Except for the error path of this function these memory regions are
> never unmapped. Please use devm_ioremap_wc() instead.
> 

 ok.
 
>> +	if (IS_ERR_OR_NULL(qproc->q6_base)) {
>> +		ret = PTR_ERR(qproc->q6_base);
>> +		goto free_mpm;
>> +	}
>> +
>> +	ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
>> +					       "qcom,halt-regs", 3,
>> +					       0, &args);
>> +	if (ret < 0)
>> +		goto free_q6;
>> +
>> +	qproc->tcsr = syscon_node_to_regmap(args.np);
>> +	of_node_put(args.np);
>> +	if (IS_ERR_OR_NULL(qproc->tcsr)) {
>> +		ret = PTR_ERR(qproc->tcsr);
>> +		goto free_q6;
>> +	}
>> +
>> +	qproc->halt_gbl = args.args[0];
>> +	qproc->halt_q6 = args.args[1];
>> +	qproc->halt_wcss = args.args[2];
>> +
>> +	return 0;
>> +
>> +free_q6:
>> +	iounmap(qproc->q6_base);
>> +
>> +free_mpm:
>> +	iounmap(qproc->mpm_base);
>> +
>> +	return ret;
>> +}
>> +
>> +static int q6_rproc_probe(struct platform_device *pdev)
>> +{
>> +	struct q6v5 *qproc;
>> +	struct rproc *rproc;
>> +	int ret;
>> +	struct qcom_smem_state *state;
>> +	unsigned int stop_bit;
>> +	const char *firmware_name = of_device_get_match_data(&pdev->dev);
>> +
>> +	state = qcom_smem_state_get(&pdev->dev, "stop",
>> +				    &stop_bit);
>> +	if (IS_ERR(state))
>> +		/* Wait till SMP2P is registered and up */
>> +		return -EPROBE_DEFER;
>> +
>> +	rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_rproc_ops,
>> +			    firmware_name,
>> +			    sizeof(*qproc));
>> +	if (unlikely(!rproc))
>> +		return -ENOMEM;
>> +
>> +	qproc = rproc->priv;
>> +	qproc->dev = &pdev->dev;
>> +	qproc->rproc = rproc;
>> +	rproc->has_iommu = false;
>> +
>> +	q6_fw_ops = *rproc->fw_ops;
>> +	q6_fw_ops.find_rsc_table = q6v5_find_rsc_table;
>> +	q6_fw_ops.load = q6v5_load;
> 
> I would prefer that you do define a static const object with these, like
> in the other qcom drivers.
> 

 ok.

> But in order to do that you would need to export
> rproc_elf_get_boot_addr(), which is in line with another item on my todo
> list, so please do this.
> 
> [..]

 hmm, in this case, boot_addr is not programmed. So would we still require the
 rproc_elf_get_boot_addr ?

Regards,
 Sricharan

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

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

[PATCH 3/3] dt-binding: remoteproc: Add the bindings required for Q6 - WCSS core

[Sricharan R]

IPQ8074 has an integrated Q6V5 Hexagon dsp - Lithium Wlan (WCSS) core.
This patch adds the required bindings to load, boot, shutdown that
remoteproc subsystem.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 .../bindings/remoteproc/qcom,q6v5-wcss.txt         | 139 +++++++++++++++++++++
 1 file changed, 139 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,q6v5-wcss.txt

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5-wcss.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5-wcss.txt
new file mode 100644
index 0000000..f664c26
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5-wcss.txt
@@ -0,0 +1,139 @@
+Qualcomm Hexagon (Q6) - WCSS Peripheral Image Loader
+
+This document defines the binding for a component that loads and boots firmware
+on the integrated Qualcomm Hexagon - WCSS core.
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		    "qcom,q6v5-wcss-pil",
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: must specify the base address and size of the qdsp6 and
+		    mpm (msm power manager) register blocks.
+
+- reg-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be "q6" and "mpm"
+
+- interrupts-extended:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: must list the watchdog, fatal IRQs ready, handover and
+		    stop-ack IRQs
+
+- interrupt-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack"
+
+- clocks:
+	Usage: required
+	Value type: <phandle>
+	Definition: references to the axim-q6, axim2-q6, axi-wcss,
+		    ahb-q6, ahbs-q6, ahb-wcss, ahbs-wcss, sysnoc and mem.
+		    to be held on behalf of the booting of the Hexagon and WCSS
+		    core. These are the interface, bus, mem clocks required for
+		    the Q6 and WCSS cores.
+
+- clock-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be "axim-q6", "axim2-q6", "axi-wcss", "ahb-q6",
+			    "ahbs-q6", "ahb-wcss", "ahbs-wcss", "sysnoc", "mem"
+
+- resets:
+	Usage: required
+	Value type: <phandle>
+	Definition: reference to the reset-controller for the Q6-WCSS sub-system
+
+- reset-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
+
+- qcom,smem-states:
+	Usage: required
+	Value type: <phandle>
+	Definition: reference to the smem state for requesting the Hexagon to
+		    shut down
+
+- qcom,smem-state-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be "stop"
+
+- qcom,halt-regs:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: a phandle reference to a syscon representing TCSR followed
+		    by the three offsets within syscon for q6, wcss and tcsr global
+		    halt registers.
+
+= SUBNODES:
+The Hexagon node must contain a subnode, named "q6" representing
+the memory region used by the Hexagon firmware. The sub-node must contain:
+
+- memory-region:
+	Usage: required
+	Value type: <phandle>
+	Definition: reference to the reserved-memory for the Q6 firmware region
+
+= EXAMPLE
+The following example describes the resources needed to boot control the
+Hexagon, as it is found on IPQ8074 boards.
+
+	q6v5_wcss: q6v5_wcss@CD00000 {
+		compatible = "q6v5-wcss-pil";
+		reg = <0xCD00000 0x4040>,
+		      <0x4ab000 0x20>;
+		reg-names = "q6",
+			    "mpm";
+		interrupts-extended = <&intc 0 325 1>,
+				      <&wcss_smp2p_in 0 0>,
+				      <&wcss_smp2p_in 1 0>,
+				      <&wcss_smp2p_in 3 0>;
+		interrupt-names = "wdog",
+				  "fatal",
+				  "handover",
+				  "stop-ack";
+		clocks = <&gcc GCC_SYS_NOC_WCSS_AHB_CLK>,
+			 <&gcc GCC_WCSS_AHB_S_CLK>,
+			 <&gcc GCC_WCSS_ECAHB_CLK>,
+			 <&gcc GCC_Q6_AHB_CLK>,
+			 <&gcc GCC_Q6_AHB_S_CLK>,
+			 <&gcc GCC_WCSS_AXI_M_CLK>,
+			 <&gcc GCC_MEM_NOC_Q6_AXI_CLK>,
+			 <&gcc GCC_Q6_AXIM_CLK>,
+			 <&gcc GCC_Q6_AXIM2_CLK>;
+		clock-names = "sysnoc",
+			      "ahb-wcss",
+			      "ahbs-wcss",
+			      "ahb-q6",
+			      "ahbs-q6",
+			      "axi-wcss",
+			      "memnoc",
+			      "axim-q6",
+			      "axim2-q6";
+
+		resets = <&gcc GCC_WCSSAON_RESET>,
+			 <&gcc GCC_WCSS_BCR>,
+			 <&gcc GCC_WCSS_Q6_BCR>;
+
+		reset-names = "wcss_aon_reset",
+			      "wcss_reset",
+			      "wcss_q6_reset";
+
+		qcom,halt-regs = <&tcsr_mutex_block 0x0 0xA000 0xD000>;
+
+		qcom,smem-states = <&wcss_smp2p_out 0>,
+				   <&wcss_smp2p_out 1>;
+		qcom,smem-state-names = "shutdown",
+					"stop";
+		q6 {
+			memory-region = <&q6_region>;
+		};
+	};
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 3/3] dt-binding: remoteproc: Add the bindings required for Q6 - WCSS core

[Rob Herring]

On Thu, Jun 29, 2017 at 07:47:41PM +0530, Sricharan R wrote:
> IPQ8074 has an integrated Q6V5 Hexagon dsp - Lithium Wlan (WCSS) core.
> This patch adds the required bindings to load, boot, shutdown that
> remoteproc subsystem.
> 
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> ---
>  .../bindings/remoteproc/qcom,q6v5-wcss.txt         | 139 +++++++++++++++++++++
>  1 file changed, 139 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,q6v5-wcss.txt
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5-wcss.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5-wcss.txt
> new file mode 100644
> index 0000000..f664c26
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5-wcss.txt
> @@ -0,0 +1,139 @@
> +Qualcomm Hexagon (Q6) - WCSS Peripheral Image Loader
> +
> +This document defines the binding for a component that loads and boots firmware
> +on the integrated Qualcomm Hexagon - WCSS core.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be one of:
> +		    "qcom,q6v5-wcss-pil",
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: must specify the base address and size of the qdsp6 and
> +		    mpm (msm power manager) register blocks.
> +
> +- reg-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "q6" and "mpm"
> +
> +- interrupts-extended:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: must list the watchdog, fatal IRQs ready, handover and

fatal, ready, ...

> +		    stop-ack IRQs
> +
> +- interrupt-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack"
> +
> +- clocks:
> +	Usage: required
> +	Value type: <phandle>

These are not just a phandle. 

> +	Definition: references to the axim-q6, axim2-q6, axi-wcss,
> +		    ahb-q6, ahbs-q6, ahb-wcss, ahbs-wcss, sysnoc and mem.
> +		    to be held on behalf of the booting of the Hexagon and WCSS
> +		    core. These are the interface, bus, mem clocks required for
> +		    the Q6 and WCSS cores.
> +
> +- clock-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "axim-q6", "axim2-q6", "axi-wcss", "ahb-q6",
> +			    "ahbs-q6", "ahb-wcss", "ahbs-wcss", "sysnoc", "mem"
> +
> +- resets:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: reference to the reset-controller for the Q6-WCSS sub-system

This should be a list of 3 resets.
> +
> +- reset-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
> +
> +- qcom,smem-states:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: reference to the smem state for requesting the Hexagon to
> +		    shut down
> +
> +- qcom,smem-state-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "stop"
> +
> +- qcom,halt-regs:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: a phandle reference to a syscon representing TCSR followed
> +		    by the three offsets within syscon for q6, wcss and tcsr global
> +		    halt registers.
> +
> += SUBNODES:
> +The Hexagon node must contain a subnode, named "q6" representing
> +the memory region used by the Hexagon firmware. The sub-node must contain:

Why does this need a sub-node?

> +
> +- memory-region:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: reference to the reserved-memory for the Q6 firmware region
> +
> += EXAMPLE
> +The following example describes the resources needed to boot control the
> +Hexagon, as it is found on IPQ8074 boards.
> +
> +	q6v5_wcss: q6v5_wcss@CD00000 {

s/_/-/ in the node name.

> +		compatible = "q6v5-wcss-pil";
> +		reg = <0xCD00000 0x4040>,
> +		      <0x4ab000 0x20>;
> +		reg-names = "q6",
> +			    "mpm";
> +		interrupts-extended = <&intc 0 325 1>,
> +				      <&wcss_smp2p_in 0 0>,
> +				      <&wcss_smp2p_in 1 0>,
> +				      <&wcss_smp2p_in 3 0>;
> +		interrupt-names = "wdog",
> +				  "fatal",
> +				  "handover",
> +				  "stop-ack";
> +		clocks = <&gcc GCC_SYS_NOC_WCSS_AHB_CLK>,
> +			 <&gcc GCC_WCSS_AHB_S_CLK>,
> +			 <&gcc GCC_WCSS_ECAHB_CLK>,
> +			 <&gcc GCC_Q6_AHB_CLK>,
> +			 <&gcc GCC_Q6_AHB_S_CLK>,
> +			 <&gcc GCC_WCSS_AXI_M_CLK>,
> +			 <&gcc GCC_MEM_NOC_Q6_AXI_CLK>,
> +			 <&gcc GCC_Q6_AXIM_CLK>,
> +			 <&gcc GCC_Q6_AXIM2_CLK>;
> +		clock-names = "sysnoc",
> +			      "ahb-wcss",
> +			      "ahbs-wcss",
> +			      "ahb-q6",
> +			      "ahbs-q6",
> +			      "axi-wcss",
> +			      "memnoc",
> +			      "axim-q6",
> +			      "axim2-q6";
> +
> +		resets = <&gcc GCC_WCSSAON_RESET>,
> +			 <&gcc GCC_WCSS_BCR>,
> +			 <&gcc GCC_WCSS_Q6_BCR>;
> +
> +		reset-names = "wcss_aon_reset",
> +			      "wcss_reset",
> +			      "wcss_q6_reset";
> +
> +		qcom,halt-regs = <&tcsr_mutex_block 0x0 0xA000 0xD000>;
> +
> +		qcom,smem-states = <&wcss_smp2p_out 0>,
> +				   <&wcss_smp2p_out 1>;
> +		qcom,smem-state-names = "shutdown",
> +					"stop";
> +		q6 {
> +			memory-region = <&q6_region>;
> +		};
> +	};
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> 

Re: [PATCH 3/3] dt-binding: remoteproc: Add the bindings required for Q6 - WCSS core

[Sricharan R]

Hi Rob,

Thanks for the review.

On 7/7/2017 7:17 PM, Rob Herring wrote:
> On Thu, Jun 29, 2017 at 07:47:41PM +0530, Sricharan R wrote:
>> IPQ8074 has an integrated Q6V5 Hexagon dsp - Lithium Wlan (WCSS) core.
>> This patch adds the required bindings to load, boot, shutdown that
>> remoteproc subsystem.
>>
>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
>> ---
>>  .../bindings/remoteproc/qcom,q6v5-wcss.txt         | 139 +++++++++++++++++++++
>>  1 file changed, 139 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,q6v5-wcss.txt
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5-wcss.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5-wcss.txt
>> new file mode 100644
>> index 0000000..f664c26
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5-wcss.txt
>> @@ -0,0 +1,139 @@
>> +Qualcomm Hexagon (Q6) - WCSS Peripheral Image Loader
>> +
>> +This document defines the binding for a component that loads and boots firmware
>> +on the integrated Qualcomm Hexagon - WCSS core.
>> +
>> +- compatible:
>> +	Usage: required
>> +	Value type: <string>
>> +	Definition: must be one of:
>> +		    "qcom,q6v5-wcss-pil",
>> +- reg:
>> +	Usage: required
>> +	Value type: <prop-encoded-array>
>> +	Definition: must specify the base address and size of the qdsp6 and
>> +		    mpm (msm power manager) register blocks.
>> +
>> +- reg-names:
>> +	Usage: required
>> +	Value type: <stringlist>
>> +	Definition: must be "q6" and "mpm"
>> +
>> +- interrupts-extended:
>> +	Usage: required
>> +	Value type: <prop-encoded-array>
>> +	Definition: must list the watchdog, fatal IRQs ready, handover and
> 
> fatal, ready, ...

 ok, will simplify.

> 
>> +		    stop-ack IRQs
>> +
>> +- interrupt-names:
>> +	Usage: required
>> +	Value type: <stringlist>
>> +	Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack"
>> +
>> +- clocks:
>> +	Usage: required
>> +	Value type: <phandle>
> 
> These are not just a phandle. 

 hmm ok. Will correct.

> 
>> +	Definition: references to the axim-q6, axim2-q6, axi-wcss,
>> +		    ahb-q6, ahbs-q6, ahb-wcss, ahbs-wcss, sysnoc and mem.
>> +		    to be held on behalf of the booting of the Hexagon and WCSS
>> +		    core. These are the interface, bus, mem clocks required for
>> +		    the Q6 and WCSS cores.
>> +
>> +- clock-names:
>> +	Usage: required
>> +	Value type: <stringlist>
>> +	Definition: must be "axim-q6", "axim2-q6", "axi-wcss", "ahb-q6",
>> +			    "ahbs-q6", "ahb-wcss", "ahbs-wcss", "sysnoc", "mem"
>> +
>> +- resets:
>> +	Usage: required
>> +	Value type: <phandle>
>> +	Definition: reference to the reset-controller for the Q6-WCSS sub-system
> 
> This should be a list of 3 resets.

 ok.

>> +
>> +- reset-names:
>> +	Usage: required
>> +	Value type: <stringlist>
>> +	Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
>> +
>> +- qcom,smem-states:
>> +	Usage: required
>> +	Value type: <phandle>
>> +	Definition: reference to the smem state for requesting the Hexagon to
>> +		    shut down
>> +
>> +- qcom,smem-state-names:
>> +	Usage: required
>> +	Value type: <stringlist>
>> +	Definition: must be "stop"
>> +
>> +- qcom,halt-regs:
>> +	Usage: required
>> +	Value type: <prop-encoded-array>
>> +	Definition: a phandle reference to a syscon representing TCSR followed
>> +		    by the three offsets within syscon for q6, wcss and tcsr global
>> +		    halt registers.
>> +
>> += SUBNODES:
>> +The Hexagon node must contain a subnode, named "q6" representing
>> +the memory region used by the Hexagon firmware. The sub-node must contain:
> 
> Why does this need a sub-node?

 Right, just the property. Had it correctly in code, missed to change here.
 
> 
>> +
>> +- memory-region:
>> +	Usage: required
>> +	Value type: <phandle>
>> +	Definition: reference to the reserved-memory for the Q6 firmware region
>> +
>> += EXAMPLE
>> +The following example describes the resources needed to boot control the
>> +Hexagon, as it is found on IPQ8074 boards.
>> +
>> +	q6v5_wcss: q6v5_wcss@CD00000 {
> 
> s/_/-/ in the node name.

 ok.

Regards,
 Sricharan

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

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus