[PATCH v2 0/3] Enable msm8996 support in mss rproc driver.

[PATCH v2 0/3] Enable msm8996 support in mss rproc driver.

[Avaneesh Kumar Dwivedi]

In continuation of earlier RFC patch series, this series consist three patches, which add following.
	1- Add hypervisor call support to enable second stage translation of intermediat physical address generated
	   by individual subsystem, second stage mapping of address is supported armv8 onwards.
	2- Modify already floating mss rproc driver to enable mss remoteproc support for msm8996.
Differences WRT patchset 1 is as follow.
	1- Secure_buffer.c file is removed which was added as a separate driver in patchset 1.
	   Instead now to make translation scm call one additional function is added in rproc driver itself.
	2- Address other comments by sboyd on RFC patch.

This patch series is dependednt on https://patchwork.kernel.org/patch/9492177/

Avaneesh Kumar Dwivedi (3):
  soc: qcom: Add scm call to protect modem mem in qcom scm driver.
  remoteproc: qcom: Add scm call to protect modem mem in mss rproc drv.
  remoteproc: qcom: Add msm8996 specific changes in mss rproc driver.

 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   1 +
 drivers/firmware/qcom_scm-64.c                     |  17 +
 drivers/firmware/qcom_scm.c                        |  14 +
 drivers/firmware/qcom_scm.h                        |   3 +
 drivers/remoteproc/qcom_q6v5_pil.c                 | 380 +++++++++++++++++++--
 include/linux/qcom_scm.h                           |   1 +
 6 files changed, 387 insertions(+), 29 deletions(-)

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

[PATCH v2 1/3] soc: qcom: Add scm call to protect modem mem in qcom scm driver.

[Avaneesh Kumar Dwivedi]

This patch add scm call support to make hypervisor call for protecting
memory region on armv8 arch cpu's.
This is required to bring up modem on msm8996. MSS rproc driver will
make hypervisor call for protecting and granting permission of DDR region
where MBA, MDT, FW are loaded.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 drivers/firmware/qcom_scm-64.c | 17 +++++++++++++++++
 drivers/firmware/qcom_scm.c    | 14 ++++++++++++++
 drivers/firmware/qcom_scm.h    |  3 +++
 include/linux/qcom_scm.h       |  1 +
 4 files changed, 35 insertions(+)

diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 4a0f5ea..5f00a64 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -358,3 +358,20 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
 
 	return ret ? : res.a1;
 }
+
+int __qcom_scm_assign_mem(struct device *dev, void *data)
+{
+	int ret;
+	struct qcom_scm_desc *desc = data;
+	struct arm_smccc_res res;
+
+	desc->arginfo = QCOM_SCM_ARGS(7, QCOM_SCM_RO, QCOM_SCM_VAL,
+				QCOM_SCM_RO, QCOM_SCM_VAL, QCOM_SCM_RO,
+				QCOM_SCM_VAL, QCOM_SCM_VAL);
+
+	ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
+				QCOM_MEM_PROT_ASSIGN_ID,
+				desc, &res);
+
+	return ret ? : res.a1;
+}
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 893f953ea..f476803 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -292,6 +292,20 @@ int qcom_scm_pas_shutdown(u32 peripheral)
 }
 EXPORT_SYMBOL(qcom_scm_pas_shutdown);
 
+/**
+ * qcom_scm_assign_mem() - Apply new access permission of DDR
+ * region passed via descriptor and does second stage
+ * translation of intermediate physical address.
+ * @desc: descriptor to send to hypervisor
+ *
+ * Return 0 on success.
+ */
+int qcom_scm_assign_mem(void *desc)
+{
+	return __qcom_scm_assign_mem(__scm->dev, desc);
+}
+EXPORT_SYMBOL(qcom_scm_assign_mem);
+
 static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
 				     unsigned long idx)
 {
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 3584b00..d88fd4b 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -47,6 +47,8 @@ extern int __qcom_scm_hdcp_req(struct device *dev,
 #define QCOM_SCM_PAS_SHUTDOWN_CMD	0x6
 #define QCOM_SCM_PAS_IS_SUPPORTED_CMD	0x7
 #define QCOM_SCM_PAS_MSS_RESET		0xa
+#define QCOM_SCM_SVC_MP				0xc
+#define QCOM_MEM_PROT_ASSIGN_ID		0x16
 extern bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral);
 extern int  __qcom_scm_pas_init_image(struct device *dev, u32 peripheral,
 		dma_addr_t metadata_phys);
@@ -55,6 +57,7 @@ extern int  __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
 extern int  __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
 extern int  __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
 extern int  __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
+extern int  __qcom_scm_assign_mem(struct device *dev, void *desc);
 
 /* common error codes */
 #define QCOM_SCM_V2_EBUSY	-12
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index cc32ab8..f9ecf35 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -36,6 +36,7 @@ extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr,
 		phys_addr_t size);
 extern int qcom_scm_pas_auth_and_reset(u32 peripheral);
 extern int qcom_scm_pas_shutdown(u32 peripheral);
+extern int qcom_scm_assign_mem(void *desc);
 
 #define QCOM_SCM_CPU_PWR_DOWN_L2_ON	0x0
 #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF	0x1
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 1/3] soc: qcom: Add scm call to protect modem mem in qcom scm driver.

[Stephen Boyd]

On 01/30, Avaneesh Kumar Dwivedi wrote:
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 893f953ea..f476803 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -292,6 +292,20 @@ int qcom_scm_pas_shutdown(u32 peripheral)
>  }
>  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
>  
> +/**
> + * qcom_scm_assign_mem() - Apply new access permission of DDR
> + * region passed via descriptor and does second stage
> + * translation of intermediate physical address.
> + * @desc: descriptor to send to hypervisor
> + *
> + * Return 0 on success.
> + */
> +int qcom_scm_assign_mem(void *desc)

Please don't pass a void pointer here. Driver authors shouldn't
need to know about qcom_scm_desc. This should be an API that the
driver can use without knowing intimate firmware details.

> +{
> +	return __qcom_scm_assign_mem(__scm->dev, desc);
> +}
> +EXPORT_SYMBOL(qcom_scm_assign_mem);
> +
>  static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
>  				     unsigned long idx)
>  {
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index 3584b00..d88fd4b 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -47,6 +47,8 @@ extern int __qcom_scm_hdcp_req(struct device *dev,
>  #define QCOM_SCM_PAS_SHUTDOWN_CMD	0x6
>  #define QCOM_SCM_PAS_IS_SUPPORTED_CMD	0x7
>  #define QCOM_SCM_PAS_MSS_RESET		0xa
> +#define QCOM_SCM_SVC_MP				0xc
> +#define QCOM_MEM_PROT_ASSIGN_ID		0x16

Presumably these should go near the newly introduced functions
like how the rest of this file is organized.

>  extern bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral);

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

Re: [PATCH v2 1/3] soc: qcom: Add scm call to protect modem mem in qcom scm driver.

[Dwivedi, Avaneesh Kumar (avani)]

On 2/28/2017 4:25 AM, Stephen Boyd wrote:
> On 01/30, Avaneesh Kumar Dwivedi wrote:
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index 893f953ea..f476803 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -292,6 +292,20 @@ int qcom_scm_pas_shutdown(u32 peripheral)
>>   }
>>   EXPORT_SYMBOL(qcom_scm_pas_shutdown);
>>   
>> +/**
>> + * qcom_scm_assign_mem() - Apply new access permission of DDR
>> + * region passed via descriptor and does second stage
>> + * translation of intermediate physical address.
>> + * @desc: descriptor to send to hypervisor
>> + *
>> + * Return 0 on success.
>> + */
>> +int qcom_scm_assign_mem(void *desc)
> Please don't pass a void pointer here. Driver authors shouldn't
> need to know about qcom_scm_desc. This should be an API that the
> driver can use without knowing intimate firmware details.
OK, so i will try to move hyp_mem_assign() to qcom_scm driver, as suggested.
this way the above issue will also get resolved.
>
>> +{
>> +	return __qcom_scm_assign_mem(__scm->dev, desc);
>> +}
>> +EXPORT_SYMBOL(qcom_scm_assign_mem);
>> +
>>   static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
>>   				     unsigned long idx)
>>   {
>> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
>> index 3584b00..d88fd4b 100644
>> --- a/drivers/firmware/qcom_scm.h
>> +++ b/drivers/firmware/qcom_scm.h
>> @@ -47,6 +47,8 @@ extern int __qcom_scm_hdcp_req(struct device *dev,
>>   #define QCOM_SCM_PAS_SHUTDOWN_CMD	0x6
>>   #define QCOM_SCM_PAS_IS_SUPPORTED_CMD	0x7
>>   #define QCOM_SCM_PAS_MSS_RESET		0xa
>> +#define QCOM_SCM_SVC_MP				0xc
>> +#define QCOM_MEM_PROT_ASSIGN_ID		0x16
> Presumably these should go near the newly introduced functions
> like how the rest of this file is organized.
OK.
>
>>   extern bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral);

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

[PATCH v2 2/3] remoteproc: qcom: Add scm call to protect modem mem in mss rproc drv.

[Avaneesh Kumar Dwivedi]

This patch add hypervisor call support for second stage translation from
mss remoteproc driver, this is required so that modem on msm8996 which is
based on armv8 architecture can access DDR region where modem firmware
are loaded.

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

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index e5edefa..35eee68 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -93,6 +93,23 @@
 #define QDSS_BHS_ON			BIT(21)
 #define QDSS_LDO_BYP			BIT(22)
 
+struct dest_vm_and_perm_info {
+	__le32 vm;
+	__le32 perm;
+	__le32 *ctx;
+	__le32 ctx_size;
+};
+
+struct mem_prot_info {
+	__le64 addr;
+	__le64 size;
+};
+
+struct scm_desc {
+	__le32 arginfo;
+	__le64 args[10];
+};
+
 struct reg_info {
 	struct regulator *reg;
 	int uV;
@@ -111,6 +128,7 @@ struct rproc_hexagon_res {
 	struct qcom_mss_reg_res active_supply[2];
 	char **proxy_clk_names;
 	char **active_clk_names;
+	int version;
 };
 
 struct q6v5 {
@@ -152,8 +170,29 @@ struct q6v5 {
 	phys_addr_t mpss_reloc;
 	void *mpss_region;
 	size_t mpss_size;
+	int version;
+	int protection_cmd;
+};
+
+enum {
+	MSS_MSM8916,
+	MSS_MSM8974,
+	MSS_MSM8996,
 };
 
+enum {
+	ASSIG_ACCESS_MSA,
+	REMOV_ACCESS_MSA,
+	SHARE_ACCESS_MSA,
+	REMOV_SHARE_MSA,
+};
+
+#define VMID_HLOS	0x3
+#define VMID_MSS_MSA	0xF
+#define PERM_READ	0x4
+#define PERM_WRITE	0x2
+#define PERM_EXEC	0x1
+#define PERM_RW	(0x4 | 0x2)
 static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
 				const struct qcom_mss_reg_res *reg_res)
 {
@@ -273,12 +312,123 @@ static void q6v5_clk_disable(struct device *dev,
 		clk_disable_unprepare(clks[i]);
 }
 
+int hyp_mem_access(struct q6v5 *qproc, phys_addr_t addr, size_t size)
+{
+	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
+	struct dest_vm_and_perm_info *dest_info;
+	struct mem_prot_info *mem_prot_info;
+	struct scm_desc desc = {0};
+	__le32 *source_vm_copy;
+	__le64 mem_prot_phy;
+	int dest_count = 1;
+	int src_count = 1;
+	__le32 *perm = {0};
+	__le32 *dest = {0};
+	__le32 *src = {0};
+	__le64 phys_src;
+	__le64 phy_dest;
+	int ret;
+	int i;
+
+	if (qproc->version != MSS_MSM8996)
+		return 0;
+
+	switch (qproc->protection_cmd) {
+		case ASSIG_ACCESS_MSA: {
+			src = (int[2]) {VMID_HLOS, 0};
+			dest = (int[2]) {VMID_MSS_MSA, 0};
+			perm = (int[2]) {PERM_READ | PERM_WRITE, 0};
+			break;
+		}
+		case REMOV_ACCESS_MSA: {
+			src = (int[2]) {VMID_MSS_MSA, 0};
+			dest = (int[2]) {VMID_HLOS, 0};
+			perm = (int[2]) {PERM_READ | PERM_WRITE | PERM_EXEC, 0};
+			break;
+		}
+		case SHARE_ACCESS_MSA: {
+			src = (int[2]) {VMID_HLOS, 0};
+			dest = (int[2]) {VMID_HLOS, VMID_MSS_MSA};
+			perm = (int[2]) {PERM_RW, PERM_RW};
+			dest_count = 2;
+			break;
+		}
+		case REMOV_SHARE_MSA: {
+			src = (int[2]) {VMID_HLOS, VMID_MSS_MSA};
+			dest = (int[2]) {VMID_HLOS, 0};
+			perm = (int[2]) {PERM_READ | PERM_WRITE | PERM_EXEC, 0};
+			src_count = 2;
+			break;
+		}
+		default: {
+			break;
+		}
+	}
+
+	source_vm_copy = dma_alloc_attrs(qproc->dev,
+				src_count*sizeof(*source_vm_copy), &phys_src,
+				GFP_KERNEL, dma_attrs);
+	if (!source_vm_copy) {
+		dev_err(qproc->dev,
+			"failed to allocate buffer to pass source vmid detail\n");
+		return -ENOMEM;
+	}
+	memcpy(source_vm_copy, src, sizeof(*source_vm_copy) * src_count);
+
+	dest_info = dma_alloc_attrs(qproc->dev,
+			dest_count*sizeof(*dest_info), &phy_dest,
+			GFP_KERNEL, dma_attrs);
+	if (!dest_info) {
+		dev_err(qproc->dev,
+			"failed to allocate buffer to pass destination vmid detail\n");
+		return -ENOMEM;
+	}
+	for (i = 0; i < dest_count; i++) {
+		dest_info[i].vm = dest[i];
+		dest_info[i].perm = perm[i];
+		dest_info[i].ctx = NULL;
+		dest_info[i].ctx_size = 0;
+	}
+
+	mem_prot_info = dma_alloc_attrs(qproc->dev,
+				sizeof(*mem_prot_info), &mem_prot_phy,
+				GFP_KERNEL, dma_attrs);
+	if (!dest_info) {
+		dev_err(qproc->dev,
+				"failed to allocate buffer to pass protected mem detail\n");
+		return -ENOMEM;
+	}
+	mem_prot_info->addr = addr;
+	mem_prot_info->size = size;
+
+	desc.args[0] = mem_prot_phy;
+	desc.args[1] = sizeof(*mem_prot_info);
+	desc.args[2] = phys_src;
+	desc.args[3] = sizeof(*source_vm_copy) * src_count;
+	desc.args[4] = phy_dest;
+	desc.args[5] = dest_count*sizeof(*dest_info);
+	desc.args[6] = 0;
+
+	ret = qcom_scm_assign_mem(&desc);
+	if (ret)
+		pr_info("%s: Failed to assign memory protection, ret = %d\n",
+			__func__, ret);
+
+	/* SCM call has returned free up buffers passed to secure domain */
+	dma_free_attrs(qproc->dev, src_count*sizeof(*source_vm_copy),
+		source_vm_copy, phys_src, dma_attrs);
+	dma_free_attrs(qproc->dev, dest_count*sizeof(*dest_info),
+		dest_info, phy_dest, dma_attrs);
+	dma_free_attrs(qproc->dev, sizeof(*mem_prot_info), mem_prot_info,
+		mem_prot_phy, dma_attrs);
+	return ret;
+}
+
 static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
 {
 	struct q6v5 *qproc = rproc->priv;
 
 	memcpy(qproc->mba_region, fw->data, fw->size);
-
 	return 0;
 }
 
@@ -446,6 +596,14 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
 
 	memcpy(ptr, fw->data, fw->size);
 
+	/* Hypervisor support to access metadata by modem */
+	qproc->protection_cmd = ASSIG_ACCESS_MSA;
+	ret = hyp_mem_access(qproc, phys, ALIGN(fw->size, SZ_4K));
+	if (ret) {
+		dev_err(qproc->dev, "Failed to assign memory, ret - %d\n", ret);
+		return -ENOMEM;
+	}
+
 	writel(phys, qproc->rmb_base + RMB_PMI_META_DATA_REG);
 	writel(RMB_CMD_META_DATA_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
 
@@ -454,7 +612,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
 		dev_err(qproc->dev, "MPSS header authentication timed out\n");
 	else if (ret < 0)
 		dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret);
-
+	/* Metadata authentication done, remove modem access */
+	qproc->protection_cmd = REMOV_ACCESS_MSA;
+	ret = hyp_mem_access(qproc, phys, ALIGN(fw->size, SZ_4K));
+	if (ret)
+		dev_err(qproc->dev, "Failed to assign memory, ret - %d\n", ret);
 	dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
 
 	return ret < 0 ? ret : 0;
@@ -483,6 +645,13 @@ static int q6v5_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
 	else
 		boot_addr = fw_addr;
 
+	/* Hypervisor support to modem to access modem fw */
+	qproc->protection_cmd = SHARE_ACCESS_MSA;
+	ret = hyp_mem_access(qproc, boot_addr, qproc->mpss_size);
+	if (ret) {
+		dev_err(qproc->dev, "Failed to assign memory, ret - %d\n", ret);
+		return -ENOMEM;
+	}
 	ehdr = (struct elf32_hdr *)fw->data;
 	phdrs = (struct elf32_phdr *)(ehdr + 1);
 	for (i = 0; i < ehdr->e_phnum; i++, phdr++) {
@@ -595,6 +764,12 @@ static int q6v5_start(struct rproc *rproc)
 		goto assert_reset;
 	}
 
+	qproc->protection_cmd = ASSIG_ACCESS_MSA;
+	ret = hyp_mem_access(qproc, qproc->mba_phys, qproc->mba_size);
+	if (ret) {
+		dev_err(qproc->dev, "Failed to assign memory, ret - %d\n", ret);
+		goto assert_reset;
+	}
 	writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
 
 	ret = q6v5proc_reset(qproc);
@@ -616,16 +791,21 @@ static int q6v5_start(struct rproc *rproc)
 
 	ret = q6v5_mpss_load(qproc);
 	if (ret)
-		goto halt_axi_ports;
+		goto reclaim_mem;
 
 	ret = wait_for_completion_timeout(&qproc->start_done,
 					  msecs_to_jiffies(5000));
 	if (ret == 0) {
 		dev_err(qproc->dev, "start timed out\n");
 		ret = -ETIMEDOUT;
-		goto halt_axi_ports;
+		goto reclaim_mem;
 	}
 
+	qproc->protection_cmd = REMOV_ACCESS_MSA;
+	ret = hyp_mem_access(qproc, qproc->mba_phys, qproc->mba_size);
+	if (ret)
+		dev_err(qproc->dev,
+			"Failed to reclaim memory, ret - %d\n", ret);
 	qproc->running = true;
 
 	q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
@@ -634,7 +814,18 @@ static int q6v5_start(struct rproc *rproc)
 				qproc->proxy_reg_count);
 	return 0;
 
+reclaim_mem:
+	qproc->protection_cmd = REMOV_SHARE_MSA;
+	ret = hyp_mem_access(qproc, qproc->mpss_phys, qproc->mpss_size);
+	if (ret)
+		dev_err(qproc->dev,
+			"Failed to reclaim memory, ret - %d\n", ret);
 halt_axi_ports:
+	qproc->protection_cmd = REMOV_ACCESS_MSA;
+	ret = hyp_mem_access(qproc, qproc->mba_phys, qproc->mba_size);
+	if (ret)
+		dev_err(qproc->dev,
+			"Failed to reclaim memory, ret - %d\n", ret);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
@@ -977,6 +1168,7 @@ static int q6v5_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
+	qproc->version = desc->version;
 	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
 	if (ret < 0)
 		goto free_rproc;
@@ -1056,6 +1248,7 @@ static int q6v5_remove(struct platform_device *pdev)
 			"mem",
 			NULL
 	},
+	.version = MSS_MSM8916,
 };
 
 static const struct rproc_hexagon_res msm8974_mss = {
@@ -1093,6 +1286,7 @@ static int q6v5_remove(struct platform_device *pdev)
 			"mem",
 			NULL
 	},
+	.version = MSS_MSM8974,
 };
 static const struct of_device_id q6v5_of_match[] = {
 	{ .compatible = "qcom,q6v5-pil", .data = &msm8916_mss},
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 2/3] remoteproc: qcom: Add scm call to protect modem mem in mss rproc drv.

[Stephen Boyd]

On 01/30, Avaneesh Kumar Dwivedi wrote:
> This patch add hypervisor call support for second stage translation from
> mss remoteproc driver, this is required so that modem on msm8996 which is
> based on armv8 architecture can access DDR region where modem firmware
> are loaded.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---

Most of this should be combined with patch 1 from this series.

>  drivers/remoteproc/qcom_q6v5_pil.c | 202 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 198 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index e5edefa..35eee68 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -93,6 +93,23 @@
>  #define QDSS_BHS_ON			BIT(21)
>  #define QDSS_LDO_BYP			BIT(22)
>  
> +struct dest_vm_and_perm_info {
> +	__le32 vm;
> +	__le32 perm;
> +	__le32 *ctx;
> +	__le32 ctx_size;
> +};
> +
> +struct mem_prot_info {
> +	__le64 addr;
> +	__le64 size;
> +};
> +
> +struct scm_desc {
> +	__le32 arginfo;
> +	__le64 args[10];
> +};

These are all firmware specific things that should live in scm
code, not PIL code.

> +
>  struct reg_info {
>  	struct regulator *reg;
>  	int uV;
> @@ -111,6 +128,7 @@ struct rproc_hexagon_res {
>  	struct qcom_mss_reg_res active_supply[2];
>  	char **proxy_clk_names;
>  	char **active_clk_names;
> +	int version;
>  };
>  
>  struct q6v5 {
> @@ -152,8 +170,29 @@ struct q6v5 {
>  	phys_addr_t mpss_reloc;
>  	void *mpss_region;
>  	size_t mpss_size;
> +	int version;
> +	int protection_cmd;
> +};
> +
> +enum {
> +	MSS_MSM8916,
> +	MSS_MSM8974,
> +	MSS_MSM8996,
>  };
>  
> +enum {
> +	ASSIG_ACCESS_MSA,
> +	REMOV_ACCESS_MSA,
> +	SHARE_ACCESS_MSA,
> +	REMOV_SHARE_MSA,
> +};
> +
> +#define VMID_HLOS	0x3
> +#define VMID_MSS_MSA	0xF
> +#define PERM_READ	0x4
> +#define PERM_WRITE	0x2
> +#define PERM_EXEC	0x1
> +#define PERM_RW	(0x4 | 0x2)

Please USE PERM_READ | PERM_WRITE here instead.

>  static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
>  				const struct qcom_mss_reg_res *reg_res)
>  {
> @@ -273,12 +312,123 @@ static void q6v5_clk_disable(struct device *dev,
>  		clk_disable_unprepare(clks[i]);
>  }
>  
> +int hyp_mem_access(struct q6v5 *qproc, phys_addr_t addr, size_t size)

This could be the scm API for now. But instead of taking qproc,
it would take the protection_cmd variable (which looks sort of
like a state machine BTW). To be more generic, perhaps it should
take the src vmids + num src vmids, dst vmids + num dst vmids,
and protection flags (which looks to be same size as dst vmid
array). If we can get the right SCM API here everything else
should fall into place.

> +{
> +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
> +	struct dest_vm_and_perm_info *dest_info;
> +	struct mem_prot_info *mem_prot_info;
> +	struct scm_desc desc = {0};
> +	__le32 *source_vm_copy;
> +	__le64 mem_prot_phy;
> +	int dest_count = 1;
> +	int src_count = 1;
> +	__le32 *perm = {0};
> +	__le32 *dest = {0};
> +	__le32 *src = {0};

src/dst seem like pretty confusing names. It makes it sound like
a memcpy operation. Perhaps from/to is better? Or current/next.

> +	__le64 phys_src;
> +	__le64 phy_dest;
> +	int ret;
> +	int i;
> +
> +	if (qproc->version != MSS_MSM8996)
> +		return 0;
> +
> +	switch (qproc->protection_cmd) {
> +		case ASSIG_ACCESS_MSA: {
> +			src = (int[2]) {VMID_HLOS, 0};
> +			dest = (int[2]) {VMID_MSS_MSA, 0};
> +			perm = (int[2]) {PERM_READ | PERM_WRITE, 0};

Why have two element arrays when we're only using the first
element? Relying on default src_count of 1 is very confusing.

> +			break;
> +		}
> +		case REMOV_ACCESS_MSA: {
> +			src = (int[2]) {VMID_MSS_MSA, 0};
> +			dest = (int[2]) {VMID_HLOS, 0};
> +			perm = (int[2]) {PERM_READ | PERM_WRITE | PERM_EXEC, 0};

Same here.

> +			break;
> +		}
> +		case SHARE_ACCESS_MSA: {
> +			src = (int[2]) {VMID_HLOS, 0};
> +			dest = (int[2]) {VMID_HLOS, VMID_MSS_MSA};
> +			perm = (int[2]) {PERM_RW, PERM_RW};

Please add spaces around curly braces like { this }

> +			dest_count = 2;
> +			break;
> +		}
> +		case REMOV_SHARE_MSA: {

And write REMOVE_SHARE_MSA, ASSIGN_ACCESS_MSA, etc. instead of
dropping letters.

> +			src = (int[2]) {VMID_HLOS, VMID_MSS_MSA};
> +			dest = (int[2]) {VMID_HLOS, 0};
> +			perm = (int[2]) {PERM_READ | PERM_WRITE | PERM_EXEC, 0};
> +			src_count = 2;
> +			break;
> +		}
> +		default: {

And also drop curly braces around case statements.

> +			break;
> +		}
> +	}
> +
> +	source_vm_copy = dma_alloc_attrs(qproc->dev,

This should really allocate from the scm->dev instead of qproc.
We don't know if qproc has the same DMA attributes that the
firmware has, but we know that we're trying to relay information
to the firmware here, not the qproc.

> +				src_count*sizeof(*source_vm_copy), &phys_src,
> +				GFP_KERNEL, dma_attrs);
> +	if (!source_vm_copy) {
> +		dev_err(qproc->dev,
> +			"failed to allocate buffer to pass source vmid detail\n");
> +		return -ENOMEM;
> +	}
> +	memcpy(source_vm_copy, src, sizeof(*source_vm_copy) * src_count);
> +
> +	dest_info = dma_alloc_attrs(qproc->dev,
> +			dest_count*sizeof(*dest_info), &phy_dest,
> +			GFP_KERNEL, dma_attrs);
> +	if (!dest_info) {
> +		dev_err(qproc->dev,
> +			"failed to allocate buffer to pass destination vmid detail\n");
> +		return -ENOMEM;
> +	}
> +	for (i = 0; i < dest_count; i++) {
> +		dest_info[i].vm = dest[i];
> +		dest_info[i].perm = perm[i];

Needs to do a cpu_to_le32() somewhere. Please run sparse.

> +		dest_info[i].ctx = NULL;
> +		dest_info[i].ctx_size = 0;
> +	}

Perhaps we should just allocate these first (one or two elements
isn't a big difference) and then fill the details in directly.

> +
> +	mem_prot_info = dma_alloc_attrs(qproc->dev,
> +				sizeof(*mem_prot_info), &mem_prot_phy,
> +				GFP_KERNEL, dma_attrs);
> +	if (!dest_info) {
> +		dev_err(qproc->dev,
> +				"failed to allocate buffer to pass protected mem detail\n");
> +		return -ENOMEM;
> +	}
> +	mem_prot_info->addr = addr;
> +	mem_prot_info->size = size;
> +
> +	desc.args[0] = mem_prot_phy;
> +	desc.args[1] = sizeof(*mem_prot_info);
> +	desc.args[2] = phys_src;
> +	desc.args[3] = sizeof(*source_vm_copy) * src_count;
> +	desc.args[4] = phy_dest;
> +	desc.args[5] = dest_count*sizeof(*dest_info);

Please add spaces around '*'. Run checkpatch.

> +	desc.args[6] = 0;
> +
> +	ret = qcom_scm_assign_mem(&desc);
> +	if (ret)
> +		pr_info("%s: Failed to assign memory protection, ret = %d\n",

pr_err? dev_err?

> +			__func__, ret);
> +
> +	/* SCM call has returned free up buffers passed to secure domain */
> +	dma_free_attrs(qproc->dev, src_count*sizeof(*source_vm_copy),
> +		source_vm_copy, phys_src, dma_attrs);
> +	dma_free_attrs(qproc->dev, dest_count*sizeof(*dest_info),
> +		dest_info, phy_dest, dma_attrs);
> +	dma_free_attrs(qproc->dev, sizeof(*mem_prot_info), mem_prot_info,
> +		mem_prot_phy, dma_attrs);
> +	return ret;
> +}
> +
>  static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>  {
>  	struct q6v5 *qproc = rproc->priv;
>  
>  	memcpy(qproc->mba_region, fw->data, fw->size);
> -

Please remove this patch noise.

>  	return 0;
>  }

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

Re: [PATCH v2 2/3] remoteproc: qcom: Add scm call to protect modem mem in mss rproc drv.

[Dwivedi, Avaneesh Kumar (avani)]

On 2/28/2017 12:46 PM, Stephen Boyd wrote:
> On 01/30, Avaneesh Kumar Dwivedi wrote:
>> This patch add hypervisor call support for second stage translation from
>> mss remoteproc driver, this is required so that modem on msm8996 which is
>> based on armv8 architecture can access DDR region where modem firmware
>> are loaded.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
> Most of this should be combined with patch 1 from this series.
OK.
>
>>   drivers/remoteproc/qcom_q6v5_pil.c | 202 ++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 198 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index e5edefa..35eee68 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -93,6 +93,23 @@
>>   #define QDSS_BHS_ON			BIT(21)
>>   #define QDSS_LDO_BYP			BIT(22)
>>   
>> +struct dest_vm_and_perm_info {
>> +	__le32 vm;
>> +	__le32 perm;
>> +	__le32 *ctx;
>> +	__le32 ctx_size;
>> +};
>> +
>> +struct mem_prot_info {
>> +	__le64 addr;
>> +	__le64 size;
>> +};
>> +
>> +struct scm_desc {
>> +	__le32 arginfo;
>> +	__le64 args[10];
>> +};
> These are all firmware specific things that should live in scm
> code, not PIL code.
OK.
>
>> +
>>   struct reg_info {
>>   	struct regulator *reg;
>>   	int uV;
>> @@ -111,6 +128,7 @@ struct rproc_hexagon_res {
>>   	struct qcom_mss_reg_res active_supply[2];
>>   	char **proxy_clk_names;
>>   	char **active_clk_names;
>> +	int version;
>>   };
>>   
>>   struct q6v5 {
>> @@ -152,8 +170,29 @@ struct q6v5 {
>>   	phys_addr_t mpss_reloc;
>>   	void *mpss_region;
>>   	size_t mpss_size;
>> +	int version;
>> +	int protection_cmd;
>> +};
>> +
>> +enum {
>> +	MSS_MSM8916,
>> +	MSS_MSM8974,
>> +	MSS_MSM8996,
>>   };
>>   
>> +enum {
>> +	ASSIG_ACCESS_MSA,
>> +	REMOV_ACCESS_MSA,
>> +	SHARE_ACCESS_MSA,
>> +	REMOV_SHARE_MSA,
>> +};
>> +
>> +#define VMID_HLOS	0x3
>> +#define VMID_MSS_MSA	0xF
>> +#define PERM_READ	0x4
>> +#define PERM_WRITE	0x2
>> +#define PERM_EXEC	0x1
>> +#define PERM_RW	(0x4 | 0x2)
> Please USE PERM_READ | PERM_WRITE here instead.
OK.
>
>>   static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
>>   				const struct qcom_mss_reg_res *reg_res)
>>   {
>> @@ -273,12 +312,123 @@ static void q6v5_clk_disable(struct device *dev,
>>   		clk_disable_unprepare(clks[i]);
>>   }
>>   
>> +int hyp_mem_access(struct q6v5 *qproc, phys_addr_t addr, size_t size)
> This could be the scm API for now. But instead of taking qproc,
> it would take the protection_cmd variable (which looks sort of
> like a state machine BTW). To be more generic, perhaps it should
> take the src vmids + num src vmids, dst vmids + num dst vmids,
> and protection flags (which looks to be same size as dst vmid
> array). If we can get the right SCM API here everything else
> should fall into place.
Will analyses and modify as per suggestion.
>
>> +{
>> +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
>> +	struct dest_vm_and_perm_info *dest_info;
>> +	struct mem_prot_info *mem_prot_info;
>> +	struct scm_desc desc = {0};
>> +	__le32 *source_vm_copy;
>> +	__le64 mem_prot_phy;
>> +	int dest_count = 1;
>> +	int src_count = 1;
>> +	__le32 *perm = {0};
>> +	__le32 *dest = {0};
>> +	__le32 *src = {0};
> src/dst seem like pretty confusing names. It makes it sound like
> a memcpy operation. Perhaps from/to is better? Or current/next.
OK
>
>> +	__le64 phys_src;
>> +	__le64 phy_dest;
>> +	int ret;
>> +	int i;
>> +
>> +	if (qproc->version != MSS_MSM8996)
>> +		return 0;
>> +
>> +	switch (qproc->protection_cmd) {
>> +		case ASSIG_ACCESS_MSA: {
>> +			src = (int[2]) {VMID_HLOS, 0};
>> +			dest = (int[2]) {VMID_MSS_MSA, 0};
>> +			perm = (int[2]) {PERM_READ | PERM_WRITE, 0};
> Why have two element arrays when we're only using the first
> element? Relying on default src_count of 1 is very confusing.
in some cases we initialize two elements.
>
>> +			break;
>> +		}
>> +		case REMOV_ACCESS_MSA: {
>> +			src = (int[2]) {VMID_MSS_MSA, 0};
>> +			dest = (int[2]) {VMID_HLOS, 0};
>> +			perm = (int[2]) {PERM_READ | PERM_WRITE | PERM_EXEC, 0};
> Same here.
OK.
>
>> +			break;
>> +		}
>> +		case SHARE_ACCESS_MSA: {
>> +			src = (int[2]) {VMID_HLOS, 0};
>> +			dest = (int[2]) {VMID_HLOS, VMID_MSS_MSA};
>> +			perm = (int[2]) {PERM_RW, PERM_RW};
> Please add spaces around curly braces like { this }
OK.
>
>> +			dest_count = 2;
>> +			break;
>> +		}
>> +		case REMOV_SHARE_MSA: {
> And write REMOVE_SHARE_MSA, ASSIGN_ACCESS_MSA, etc. instead of
> dropping letters.
OK.
>
>> +			src = (int[2]) {VMID_HLOS, VMID_MSS_MSA};
>> +			dest = (int[2]) {VMID_HLOS, 0};
>> +			perm = (int[2]) {PERM_READ | PERM_WRITE | PERM_EXEC, 0};
>> +			src_count = 2;
>> +			break;
>> +		}
>> +		default: {
> And also drop curly braces around case statements.
OK.
>
>> +			break;
>> +		}
>> +	}
>> +
>> +	source_vm_copy = dma_alloc_attrs(qproc->dev,
> This should really allocate from the scm->dev instead of qproc.
> We don't know if qproc has the same DMA attributes that the
> firmware has, but we know that we're trying to relay information
> to the firmware here, not the qproc.
OK, agree.
>
>> +				src_count*sizeof(*source_vm_copy), &phys_src,
>> +				GFP_KERNEL, dma_attrs);
>> +	if (!source_vm_copy) {
>> +		dev_err(qproc->dev,
>> +			"failed to allocate buffer to pass source vmid detail\n");
>> +		return -ENOMEM;
>> +	}
>> +	memcpy(source_vm_copy, src, sizeof(*source_vm_copy) * src_count);
>> +
>> +	dest_info = dma_alloc_attrs(qproc->dev,
>> +			dest_count*sizeof(*dest_info), &phy_dest,
>> +			GFP_KERNEL, dma_attrs);
>> +	if (!dest_info) {
>> +		dev_err(qproc->dev,
>> +			"failed to allocate buffer to pass destination vmid detail\n");
>> +		return -ENOMEM;
>> +	}
>> +	for (i = 0; i < dest_count; i++) {
>> +		dest_info[i].vm = dest[i];
>> +		dest_info[i].perm = perm[i];
> Needs to do a cpu_to_le32() somewhere. Please run sparse.
I understand that byte reordering needed but not sure what is sparse 
will check and do it.
>
>> +		dest_info[i].ctx = NULL;
>> +		dest_info[i].ctx_size = 0;
>> +	}
> Perhaps we should just allocate these first (one or two elements
> isn't a big difference) and then fill the details in directly.
Not very clear what is ask here?
>
>> +
>> +	mem_prot_info = dma_alloc_attrs(qproc->dev,
>> +				sizeof(*mem_prot_info), &mem_prot_phy,
>> +				GFP_KERNEL, dma_attrs);
>> +	if (!dest_info) {
>> +		dev_err(qproc->dev,
>> +				"failed to allocate buffer to pass protected mem detail\n");
>> +		return -ENOMEM;
>> +	}
>> +	mem_prot_info->addr = addr;
>> +	mem_prot_info->size = size;
>> +
>> +	desc.args[0] = mem_prot_phy;
>> +	desc.args[1] = sizeof(*mem_prot_info);
>> +	desc.args[2] = phys_src;
>> +	desc.args[3] = sizeof(*source_vm_copy) * src_count;
>> +	desc.args[4] = phy_dest;
>> +	desc.args[5] = dest_count*sizeof(*dest_info);
> Please add spaces around '*'. Run checkpatch.
OK.
>
>> +	desc.args[6] = 0;
>> +
>> +	ret = qcom_scm_assign_mem(&desc);
>> +	if (ret)
>> +		pr_info("%s: Failed to assign memory protection, ret = %d\n",
> pr_err? dev_err?
Will correct.
>
>> +			__func__, ret);
>> +
>> +	/* SCM call has returned free up buffers passed to secure domain */
>> +	dma_free_attrs(qproc->dev, src_count*sizeof(*source_vm_copy),
>> +		source_vm_copy, phys_src, dma_attrs);
>> +	dma_free_attrs(qproc->dev, dest_count*sizeof(*dest_info),
>> +		dest_info, phy_dest, dma_attrs);
>> +	dma_free_attrs(qproc->dev, sizeof(*mem_prot_info), mem_prot_info,
>> +		mem_prot_phy, dma_attrs);
>> +	return ret;
>> +}
>> +
>>   static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>>   {
>>   	struct q6v5 *qproc = rproc->priv;
>>   
>>   	memcpy(qproc->mba_region, fw->data, fw->size);
>> -
> Please remove this patch noise.
OK.
>
>>   	return 0;
>>   }

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

Re: [PATCH v2 2/3] remoteproc: qcom: Add scm call to protect modem mem in mss rproc drv.

[Stephen Boyd]

On 03/03, Dwivedi, Avaneesh Kumar (avani) wrote:
> On 2/28/2017 12:46 PM, Stephen Boyd wrote:
> >On 01/30, Avaneesh Kumar Dwivedi wrote:
> >>+		dest_info[i].vm = dest[i];
> >>+		dest_info[i].perm = perm[i];
> >Needs to do a cpu_to_le32() somewhere. Please run sparse.
> I understand that byte reordering needed but not sure what is sparse
> will check and do it.

http://git.kernel.org/cgit/devel/sparse/sparse.git/

> >
> >>+		dest_info[i].ctx = NULL;
> >>+		dest_info[i].ctx_size = 0;
> >>+	}
> >Perhaps we should just allocate these first (one or two elements
> >isn't a big difference) and then fill the details in directly.
> Not very clear what is ask here?
> >

I'm saying we could fill in dest_info in the case statement
instead of here in an up to 2 times loop.

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

[PATCH v2 3/3] remoteproc: qcom: Add msm8996 specific changes in mss rproc driver.

[Avaneesh Kumar Dwivedi]

This patch add msm8996 support in existing mss rproc driver.

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

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 674ebc6..06f51a6 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -10,6 +10,7 @@ on the Qualcomm Hexagon core.
 		"qcom,q6v5-pil"
 		"qcom,msm8916-mss-pil"
 		"qcom,msm8974-mss-pil"
+		"qcom,msm8996-mss-pil"
 
 - reg:
 	Usage: required
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 35eee68..9c12a36 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -31,6 +31,7 @@
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/smem_state.h>
 #include <linux/of_device.h>
+#include <linux/iopoll.h>
 
 #include "remoteproc_internal.h"
 #include "qcom_mdt_loader.h"
@@ -65,6 +66,8 @@
 #define QDSP6SS_RESET_REG		0x014
 #define QDSP6SS_GFMUX_CTL_REG		0x020
 #define QDSP6SS_PWR_CTL_REG		0x030
+#define QDSP6SS_MEM_PWR_CTL		0x0B0
+#define QDSP6SS_STRAP_ACC		0x110
 
 /* AXI Halt Register Offsets */
 #define AXI_HALTREQ_REG			0x0
@@ -93,6 +96,15 @@
 #define QDSS_BHS_ON			BIT(21)
 #define QDSS_LDO_BYP			BIT(22)
 
+/* QDSP6v56 parameters */
+#define QDSP6v56_LDO_BYP		BIT(25)
+#define QDSP6v56_BHS_ON			BIT(24)
+#define QDSP6v56_CLAMP_WL		BIT(21)
+#define QDSP6v56_CLAMP_QMC_MEM		BIT(22)
+#define HALT_CHECK_MAX_LOOPS		200
+#define QDSP6SS_XO_CBCR		0x0038
+#define QDSP6SS_ACC_OVERRIDE_VAL		0x20
+
 struct dest_vm_and_perm_info {
 	__le32 vm;
 	__le32 perm;
@@ -485,35 +497,99 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
 
 static int q6v5proc_reset(struct q6v5 *qproc)
 {
-	u32 val;
+	u64 val;
 	int ret;
+	int i;
 
-	/* Assert resets, stop core */
-	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
-	val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
-	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
 
-	/* Enable power block headswitch, and wait for it to stabilize */
-	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= QDSS_BHS_ON | QDSS_LDO_BYP;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	udelay(1);
-
-	/*
-	 * Turn on memories. L2 banks should be done individually
-	 * to minimize inrush current.
-	 */
-	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
-		Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= Q6SS_L2DATA_SLP_NRET_N_2;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= Q6SS_L2DATA_SLP_NRET_N_1;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= Q6SS_L2DATA_SLP_NRET_N_0;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	if (qproc->version == MSS_MSM8996) {
+		/* Override the ACC value if required */
+		writel(QDSP6SS_ACC_OVERRIDE_VAL,
+			qproc->reg_base + QDSP6SS_STRAP_ACC);
+
+		/* Assert resets, stop core */
+		val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
+		val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
+		writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
+
+		/* BHS require xo cbcr to be enabled */
+		val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
+		val |= 0x1;
+		writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
 
+		/* 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);
+	} else {
+		/* Assert resets, stop core */
+		val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
+		val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
+		writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
+
+		/* Enable power block headswitch and wait for it to stabilize */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= QDSS_BHS_ON | QDSS_LDO_BYP;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		udelay(1);
+		/*
+		 * Turn on memories. L2 banks should be done individually
+		 * to minimize inrush current.
+		 */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
+			Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= Q6SS_L2DATA_SLP_NRET_N_2;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= Q6SS_L2DATA_SLP_NRET_N_1;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= Q6SS_L2DATA_SLP_NRET_N_0;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	}
 	/* Remove IO clamp */
 	val &= ~Q6SS_CLAMP_IO;
 	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
@@ -849,6 +925,7 @@ static int q6v5_stop(struct rproc *rproc)
 {
 	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
 	int ret;
+	int val;
 
 	qproc->running = false;
 
@@ -866,6 +943,15 @@ static int q6v5_stop(struct rproc *rproc)
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
 
+	if (qproc->version == MSS_MSM8996) {
+		/*
+		 * To avoid high MX current during LPASS/MSS restart.
+		 */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL |
+			QDSP6v56_CLAMP_QMC_MEM;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	}
 	reset_control_assert(qproc->mss_restart);
 	q6v5_clk_disable(qproc->dev, qproc->active_clks,
 				qproc->active_clk_count);
@@ -1213,6 +1299,47 @@ static int q6v5_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct rproc_hexagon_res msm8996_mss = {
+	.hexagon_mba_image = "mba.mbn",
+	.proxy_supply = (struct qcom_mss_reg_res[]) {
+		{
+			.supply = "vdd_mx",
+			.uV = 6,
+		},
+		{
+			.supply = "vdd_cx",
+			.uV = 7,
+			.uA = 100000,
+		},
+		{
+			.supply = "vdd_pll",
+			.uV = 1800000,
+			.uA = 100000,
+		},
+		{}
+	},
+	.active_supply = (struct qcom_mss_reg_res[]) {
+			{},
+			{}
+	},
+	.proxy_clk_names = (char*[]){
+			"xo",
+			"pnoc",
+			"qdss",
+			NULL
+	},
+	.active_clk_names = (char*[]){
+			"iface",
+			"bus",
+			"mem",
+			"gpll0_mss_clk",
+			"snoc_axi_clk",
+			"mnoc_axi_clk",
+			NULL
+	},
+	.version = MSS_MSM8996,
+};
+
 static const struct rproc_hexagon_res msm8916_mss = {
 	.hexagon_mba_image = "mba.mbn",
 	.proxy_supply = (struct qcom_mss_reg_res[]) {
@@ -1292,6 +1419,7 @@ static int q6v5_remove(struct platform_device *pdev)
 	{ .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},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, q6v5_of_match);
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 3/3] remoteproc: qcom: Add msm8996 specific changes in mss rproc driver.

[Stephen Boyd]

On 01/30, Avaneesh Kumar Dwivedi wrote:
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 35eee68..9c12a36 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -485,35 +497,99 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>  
>  static int q6v5proc_reset(struct q6v5 *qproc)
>  {
> -	u32 val;
> +	u64 val;

Why u64? There isn't any readq/writeq usage here.

>  	int ret;
> +	int i;
>  
[...]
> +	if (qproc->version == MSS_MSM8996) {
> +		/* Override the ACC value if required */
> +		writel(QDSP6SS_ACC_OVERRIDE_VAL,
> +			qproc->reg_base + QDSP6SS_STRAP_ACC);
> +
> +		/* Assert resets, stop core */
> +		val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
> +		val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);

Useless parenthesis.

> +		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);

Useless parenthesis.

> +		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);
> +	} else {
> +		/* Assert resets, stop core */
> +		val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
> +		val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);

Useless parenthesis.

> +		writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
> +
> +		/* Enable power block headswitch and wait for it to stabilize */
> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val |= QDSS_BHS_ON | QDSS_LDO_BYP;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		udelay(1);
> @@ -849,6 +925,7 @@ static int q6v5_stop(struct rproc *rproc)
>  {
>  	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>  	int ret;
> +	int val;

u32 instead of int?

>  
>  	qproc->running = false;
>  
> @@ -866,6 +943,15 @@ static int q6v5_stop(struct rproc *rproc)
>  	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
>  	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>  
> +	if (qproc->version == MSS_MSM8996) {
> +		/*
> +		 * To avoid high MX current during LPASS/MSS restart.
> +		 */

Three lines could be one line comment instead. 

> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val |= Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL |
> +			QDSP6v56_CLAMP_QMC_MEM;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	}
>  	reset_control_assert(qproc->mss_restart);
>  	q6v5_clk_disable(qproc->dev, qproc->active_clks,
>  				qproc->active_clk_count);
> @@ -1213,6 +1299,47 @@ static int q6v5_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct rproc_hexagon_res msm8996_mss = {
> +	.hexagon_mba_image = "mba.mbn",
> +	.proxy_supply = (struct qcom_mss_reg_res[]) {
> +		{
> +			.supply = "vdd_mx",
> +			.uV = 6,
> +		},
> +		{
> +			.supply = "vdd_cx",
> +			.uV = 7,
> +			.uA = 100000,
> +		},

vdd cx and vdd mx are corners. The plan is to _not_ use the
regulator framework for those, so treating them like supplies is
incorrect here.

> +		{
> +			.supply = "vdd_pll",
> +			.uV = 1800000,
> +			.uA = 100000,
> +		},
> +		{}
> +	},

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

Re: [PATCH v2 3/3] remoteproc: qcom: Add msm8996 specific changes in mss rproc driver.

[Dwivedi, Avaneesh Kumar (avani)]

On 2/28/2017 4:18 AM, Stephen Boyd wrote:
> On 01/30, Avaneesh Kumar Dwivedi wrote:
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 35eee68..9c12a36 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -485,35 +497,99 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>>   
>>   static int q6v5proc_reset(struct q6v5 *qproc)
>>   {
>> -	u32 val;
>> +	u64 val;
> Why u64? There isn't any readq/writeq usage here.
OK
>
>>   	int ret;
>> +	int i;
>>   
> [...]
>> +	if (qproc->version == MSS_MSM8996) {
>> +		/* Override the ACC value if required */
>> +		writel(QDSP6SS_ACC_OVERRIDE_VAL,
>> +			qproc->reg_base + QDSP6SS_STRAP_ACC);
>> +
>> +		/* Assert resets, stop core */
>> +		val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
>> +		val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
> Useless parenthesis.
ok
>
>> +		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);
> Useless parenthesis.
ok
>
>> +		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);
>> +	} else {
>> +		/* Assert resets, stop core */
>> +		val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
>> +		val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
> Useless parenthesis.
ok
>
>> +		writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
>> +
>> +		/* Enable power block headswitch and wait for it to stabilize */
>> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val |= QDSS_BHS_ON | QDSS_LDO_BYP;
>> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		udelay(1);
>> @@ -849,6 +925,7 @@ static int q6v5_stop(struct rproc *rproc)
>>   {
>>   	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>>   	int ret;
>> +	int val;
> u32 instead of int?
ok
>
>>   
>>   	qproc->running = false;
>>   
>> @@ -866,6 +943,15 @@ static int q6v5_stop(struct rproc *rproc)
>>   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
>>   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>>   
>> +	if (qproc->version == MSS_MSM8996) {
>> +		/*
>> +		 * To avoid high MX current during LPASS/MSS restart.
>> +		 */
> Three lines could be one line comment instead.
ok.
>
>> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +		val |= Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL |
>> +			QDSP6v56_CLAMP_QMC_MEM;
>> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	}
>>   	reset_control_assert(qproc->mss_restart);
>>   	q6v5_clk_disable(qproc->dev, qproc->active_clks,
>>   				qproc->active_clk_count);
>> @@ -1213,6 +1299,47 @@ static int q6v5_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   
>> +static const struct rproc_hexagon_res msm8996_mss = {
>> +	.hexagon_mba_image = "mba.mbn",
>> +	.proxy_supply = (struct qcom_mss_reg_res[]) {
>> +		{
>> +			.supply = "vdd_mx",
>> +			.uV = 6,
>> +		},
>> +		{
>> +			.supply = "vdd_cx",
>> +			.uV = 7,
>> +			.uA = 100000,
>> +		},
> vdd cx and vdd mx are corners. The plan is to _not_ use the
> regulator framework for those, so treating them like supplies is
> incorrect here.
vdd cx and mx though in downstream are voted for corner but they are 
always ON domain upstream as per regulator team when i discussed with them.
should i drop them altogether?
>
>> +		{
>> +			.supply = "vdd_pll",
>> +			.uV = 1800000,
>> +			.uA = 100000,
>> +		},
>> +		{}
>> +	},

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

Re: [PATCH v2 3/3] remoteproc: qcom: Add msm8996 specific changes in mss rproc driver.

[Stephen Boyd]

On 03/03, Dwivedi, Avaneesh Kumar (avani) wrote:
> On 2/28/2017 4:18 AM, Stephen Boyd wrote:
> >On 01/30, Avaneesh Kumar Dwivedi wrote:
> >>@@ -1213,6 +1299,47 @@ static int q6v5_remove(struct platform_device *pdev)
> >>  	return 0;
> >>  }
> >>+static const struct rproc_hexagon_res msm8996_mss = {
> >>+	.hexagon_mba_image = "mba.mbn",
> >>+	.proxy_supply = (struct qcom_mss_reg_res[]) {
> >>+		{
> >>+			.supply = "vdd_mx",
> >>+			.uV = 6,
> >>+		},
> >>+		{
> >>+			.supply = "vdd_cx",
> >>+			.uV = 7,
> >>+			.uA = 100000,
> >>+		},
> >vdd cx and vdd mx are corners. The plan is to _not_ use the
> >regulator framework for those, so treating them like supplies is
> >incorrect here.
> vdd cx and mx though in downstream are voted for corner but they are
> always ON domain upstream as per regulator team when i discussed
> with them.
> should i drop them altogether?

I would say yes, drop them. The on/off state doesn't matter here.
This code wants to max out the corner for a period of time until
the remote processor has booted far enough to make their own vote
on these RPM resources.

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