[RESEND: PATCH v4 0/4] Add memory ownership switch support and enable mss rproc on msm8996

[RESEND: PATCH v4 0/4] Add memory ownership switch support and enable mss rproc on msm8996

[Avaneesh Kumar Dwivedi]

This patch does following
	1- Adds new scm call which helps in stage two translation of a memory region
	   so that memory ownership sharing and switching can be achieved on armv8 and later.
	2- Enable mss remoteproc on msm8996

Major changes since last patch:
	1- Refactored SCM API
	2- Changed SCM API Signature 
	3- Added cpu_to_le32() as and where required
	4- Elaborated documentation of new API added
	5- Refactored MSS rproc code which make the scm api call
	6- Refactored mss rproc code to first load all segments and then transfer ownership of
	   memory region to MSS for authentication and boot etc.
	7- Other Minor changes
	8- kbuild robot gave error so added dummy definition of __qcom_scm_assign_mem() in qcom_scm-32.c as well
Avaneesh Kumar Dwivedi (4):
  firmware: scm: Add new SCM call for switching memory ownership
  remoteproc: qcom: refactor mss fw image loading sequence
  remoteproc: qcom: Make secure world call for mem ownership switch
  remoteproc: qcom: Add support for mss boot on msm8996

 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   4 +-
 drivers/firmware/qcom_scm-32.c                     |   6 +
 drivers/firmware/qcom_scm-64.c                     |  27 ++
 drivers/firmware/qcom_scm.c                        |  75 ++++++
 drivers/firmware/qcom_scm.h                        |   4 +
 drivers/remoteproc/qcom_q6v5_pil.c                 | 280 ++++++++++++++++++---
 include/linux/qcom_scm.h                           |  14 ++
 7 files changed, 370 insertions(+), 40 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.

[RESEND: PATCH v4 1/4] firmware: scm: Add new SCM call for switching memory ownership

[Avaneesh Kumar Dwivedi]

Two different processors on a SOC need to switch memory ownership
during load/unload. To enable this, level second memory map table
need to be updated, which is done by secure layer.
This patch add the interface for making secure monitor call for
memory ownership switching request.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 drivers/firmware/qcom_scm-32.c |  6 ++++
 drivers/firmware/qcom_scm-64.c | 27 +++++++++++++++
 drivers/firmware/qcom_scm.c    | 75 ++++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/qcom_scm.h    |  4 +++
 include/linux/qcom_scm.h       | 14 ++++++++
 5 files changed, 126 insertions(+)

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index 93e3b96..4eb7d59 100644
--- a/drivers/firmware/qcom_scm-32.c
+++ b/drivers/firmware/qcom_scm-32.c
@@ -596,3 +596,9 @@ int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size,
 {
 	return -ENODEV;
 }
+int __qcom_scm_assign_mem(struct device *dev, phys_addr_t mem_addr,
+		size_t mem_sz, phys_addr_t srcVm, size_t srcVm_sz,
+		phys_addr_t destVm, size_t destVm_sz)
+{
+return -ENODEV;
+}
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 6e6d561..77ef5c9 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -439,3 +439,30 @@ int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size,
 
 	return ret;
 }
+
+int __qcom_scm_assign_mem(struct device *dev, phys_addr_t mem_addr,
+		size_t mem_sz, phys_addr_t srcVm, size_t srcVm_sz,
+		phys_addr_t destVm, size_t destVm_sz)
+{
+	int ret;
+	struct qcom_scm_desc desc = {0};
+	struct arm_smccc_res res;
+
+	desc.args[0] = mem_addr;
+	desc.args[1] = mem_sz;
+	desc.args[2] = srcVm;
+	desc.args[3] = srcVm_sz;
+	desc.args[4] = destVm;
+	desc.args[5] = destVm_sz;
+	desc.args[6] = 0;
+
+	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 bb16510..a2363e2 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -40,6 +40,24 @@ struct qcom_scm {
 	struct reset_controller_dev reset;
 };
 
+struct qcom_scm_current_perm_info {
+	__le32 destVm;
+	__le32 destVmPerm;
+	__le64 ctx;
+	__le32 ctx_size;
+};
+
+struct qcom_scm_mem_map_info {
+	__le64 mem_addr;
+	__le64 mem_size;
+};
+
+struct qcom_scm_hyp_map_info {
+	__le32 srcVm[2];
+	struct qcom_scm_mem_map_info mem_region;
+	struct qcom_scm_current_perm_info destVm[2];
+};
+
 static struct qcom_scm *__scm;
 
 static int qcom_scm_clk_enable(void)
@@ -292,6 +310,63 @@ int qcom_scm_pas_shutdown(u32 peripheral)
 }
 EXPORT_SYMBOL(qcom_scm_pas_shutdown);
 
+/**
+ * qcom_scm_assign_mem() - Provide interface to request to map a memory
+ * region into intermediate physical address table as well map
+ * access permissions for any other proc on SOC. So that when other proc
+ * applies the same intermediate physical address passed by requesting
+ * processor in this case apps proc, on memory bus it can access the
+ * region without fault.
+ * @mem_addr: Start pointer of region which need to be mapped.
+ * @mem_sz: Size of the region.
+ * @srcVm: Detail of current owners, each set bit in flag indicate id of
+ * shared owners.
+ * @newVm: Details of new owners and permission flags for each of them.
+ * @newVm_sz: Size of array pointed by newVm.
+ * Return 0 on success.
+ */
+int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcVm,
+		struct qcom_scm_destVmPerm *newVm, size_t newVm_sz)
+{
+	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
+	struct qcom_scm_hyp_map_info *hmi;
+	phys_addr_t addr[3];
+	size_t size[3];
+	int ret;
+	int i;
+
+	hmi = dma_alloc_attrs(__scm->dev, sizeof(*hmi),
+			&addr[1], GFP_KERNEL, dma_attrs);
+	hmi->mem_region.mem_addr = cpu_to_le64(mem_addr);
+	hmi->mem_region.mem_size = cpu_to_le64(mem_sz);
+
+	addr[0] = addr[1] + sizeof(hmi->srcVm);
+	size[0] = sizeof(hmi->mem_region);
+
+	ret = hweight_long(srcVm);
+	for (i = 0; i < ret; i++) {
+		hmi->srcVm[i] = cpu_to_le32(ffs(srcVm) - 0x1);
+		srcVm ^= 1 << (ffs(srcVm) - 0x1);
+	}
+	size[1] = ret * sizeof(srcVm);
+
+	ret = newVm_sz/sizeof(struct qcom_scm_destVmPerm);
+	for (i = 0; i < ret; i++) {
+		hmi->destVm[i].destVm = cpu_to_le32(newVm[i].destVm);
+		hmi->destVm[i].destVmPerm = cpu_to_le32(newVm[i].destVmPerm);
+		hmi->destVm[i].ctx = 0;
+		hmi->destVm[i].ctx_size = 0;
+	}
+	addr[2] = addr[0] + sizeof(hmi->mem_region);
+	size[2] = ret * sizeof(struct qcom_scm_current_perm_info);
+
+	ret = __qcom_scm_assign_mem(__scm->dev, addr[0],
+		size[0], addr[1], size[1], addr[2], size[2]);
+	dma_free_attrs(__scm->dev, sizeof(*hmi), hmi, addr[1], dma_attrs);
+	return ret;
+}
+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 9bea691..0837b96 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -95,5 +95,9 @@ extern int __qcom_scm_iommu_secure_ptbl_size(struct device *dev, u32 spare,
 					     size_t *size);
 extern int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr,
 					     u32 size, u32 spare);
+#define QCOM_MEM_PROT_ASSIGN_ID	0x16
+extern int  __qcom_scm_assign_mem(struct device *dev, phys_addr_t mem_addr,
+		size_t mem_sz, phys_addr_t srcVm, size_t srcVm_sz,
+		phys_addr_t destVm, size_t destVm_sz);
 
 #endif
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index e538047..cb930d9 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -23,6 +23,18 @@ struct qcom_scm_hdcp_req {
 	u32 val;
 };
 
+struct qcom_scm_destVmPerm {
+	int destVm;
+	int destVmPerm;
+};
+
+#define QCOM_SCM_VMID_HLOS       0x3
+#define QCOM_SCM_VMID_MSS_MSA    0xF
+#define QCOM_SCM_PERM_READ       0x4
+#define QCOM_SCM_PERM_WRITE      0x2
+#define QCOM_SCM_PERM_EXEC       0x1
+#define QCOM_SCM_PERM_RW (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)
+
 #if IS_ENABLED(CONFIG_QCOM_SCM)
 extern int qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
 extern int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus);
@@ -37,6 +49,8 @@ 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(phys_addr_t mem_addr, size_t mem_sz, int currVm,
+			struct qcom_scm_destVmPerm *newVm, size_t newVm_sz);
 extern void qcom_scm_cpu_power_down(u32 flags);
 extern u32 qcom_scm_get_version(void);
 extern int qcom_scm_set_remote_state(u32 state, u32 id);
-- 
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: [RESEND: PATCH v4 1/4] firmware: scm: Add new SCM call for switching memory ownership

[Bjorn Andersson]

On Tue 16 May 11:01 PDT 2017, Avaneesh Kumar Dwivedi wrote:

> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
> index 93e3b96..4eb7d59 100644
> --- a/drivers/firmware/qcom_scm-32.c
> +++ b/drivers/firmware/qcom_scm-32.c
> @@ -596,3 +596,9 @@ int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size,
>  {
>  	return -ENODEV;
>  }
> +int __qcom_scm_assign_mem(struct device *dev, phys_addr_t mem_addr,
> +		size_t mem_sz, phys_addr_t srcVm, size_t srcVm_sz,
> +		phys_addr_t destVm, size_t destVm_sz)
> +{
> +return -ENODEV;

Indentation please.

> +}
[..]
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index bb16510..a2363e2 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -40,6 +40,24 @@ struct qcom_scm {
>  	struct reset_controller_dev reset;
>  };
>  
> +struct qcom_scm_current_perm_info {
> +	__le32 destVm;

__le32 vmid;

> +	__le32 destVmPerm;

__le32 perm;

> +	__le64 ctx;
> +	__le32 ctx_size;
> +};
> +
> +struct qcom_scm_mem_map_info {
> +	__le64 mem_addr;
> +	__le64 mem_size;
> +};
> +
> +struct qcom_scm_hyp_map_info {
> +	__le32 srcVm[2];
> +	struct qcom_scm_mem_map_info mem_region;
> +	struct qcom_scm_current_perm_info destVm[2];
> +};

As described below, both arrays in this struct should be dynamic size,
so I recommend dropping the struct and just do the offset math yourself
in the function.

> +
>  static struct qcom_scm *__scm;
>  
>  static int qcom_scm_clk_enable(void)
> @@ -292,6 +310,63 @@ int qcom_scm_pas_shutdown(u32 peripheral)
>  }
>  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
>  
> +/**
> + * qcom_scm_assign_mem() - Provide interface to request to map a memory
> + * region into intermediate physical address table as well map
> + * access permissions for any other proc on SOC. So that when other proc
> + * applies the same intermediate physical address passed by requesting
> + * processor in this case apps proc, on memory bus it can access the
> + * region without fault.

The first line should be a short description of the function.

> + * @mem_addr: Start pointer of region which need to be mapped.
> + * @mem_sz: Size of the region.
> + * @srcVm: Detail of current owners, each set bit in flag indicate id of
> + * shared owners.
> + * @newVm: Details of new owners and permission flags for each of them.
> + * @newVm_sz: Size of array pointed by newVm.

If necessary (appropriate in your case) a longer description should go
here - with blank lines before and after.

> + * Return 0 on success.

For more info on the format, please see:

https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

> + */
> +int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcVm,
> +		struct qcom_scm_destVmPerm *newVm, size_t newVm_sz)

It's more idiomatic to pass the number of elements in an array than the
size of the array, so make newVm_sz be the number of items.

And please refrain from using camelCase in the Linux kernel.

> +{
> +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
> +	struct qcom_scm_hyp_map_info *hmi;

Skip this struct and just store the base address in a void *, then have
one pointer for each of the substructures (to help fill them in).

> +	phys_addr_t addr[3];
> +	size_t size[3];

Please give these 6 variables names.

> +	int ret;
> +	int i;
> +
> +	hmi = dma_alloc_attrs(__scm->dev, sizeof(*hmi),
> +			&addr[1], GFP_KERNEL, dma_attrs);

This function should handle arbitrary number of src and destination vms;
so start by calculating the hweight of the source, allocate the
appropriate amount of srcVM and dstVM space and then calculate the
offsets within that block for each entry.


Check and handle !hmi

> +	hmi->mem_region.mem_addr = cpu_to_le64(mem_addr);
> +	hmi->mem_region.mem_size = cpu_to_le64(mem_sz);
> +
> +	addr[0] = addr[1] + sizeof(hmi->srcVm);
> +	size[0] = sizeof(hmi->mem_region);
> +
> +	ret = hweight_long(srcVm);
> +	for (i = 0; i < ret; i++) {
> +		hmi->srcVm[i] = cpu_to_le32(ffs(srcVm) - 0x1);

Subtract 1, rather than 0x1

> +		srcVm ^= 1 << (ffs(srcVm) - 0x1);

Make this easier to read with:

	for (...) {
		vmid = ffs(srcVm) - 1;

		hmi->srcVm[i] = cpu_to_le32(vmid);
		srcVm &= ~BIT(vmid);
	}

> +	}
> +	size[1] = ret * sizeof(srcVm);
> +
> +	ret = newVm_sz/sizeof(struct qcom_scm_destVmPerm);
> +	for (i = 0; i < ret; i++) {
> +		hmi->destVm[i].destVm = cpu_to_le32(newVm[i].destVm);
> +		hmi->destVm[i].destVmPerm = cpu_to_le32(newVm[i].destVmPerm);
> +		hmi->destVm[i].ctx = 0;
> +		hmi->destVm[i].ctx_size = 0;
> +	}
> +	addr[2] = addr[0] + sizeof(hmi->mem_region);
> +	size[2] = ret * sizeof(struct qcom_scm_current_perm_info);
> +
> +	ret = __qcom_scm_assign_mem(__scm->dev, addr[0],
> +		size[0], addr[1], size[1], addr[2], size[2]);
> +	dma_free_attrs(__scm->dev, sizeof(*hmi), hmi, addr[1], dma_attrs);
> +	return ret;

As discussed on IRC, please return negative on failure and otherwise a
bitmap built from the vmids in the destination list.

Also note that when I tested this last time I saw
__qcom_scm_assign_mem() return positive numbers even though the mapping
seemed broken afterwards. So it could be that you should treat any
return value as some sort of error.

> +}
> +EXPORT_SYMBOL(qcom_scm_assign_mem);
[..]
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index e538047..cb930d9 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -23,6 +23,18 @@ struct qcom_scm_hdcp_req {
>  	u32 val;
>  };
>  
> +struct qcom_scm_destVmPerm {

There's no harm in dropping "dest" from this, and please drop the
camelCase.

> +	int destVm;

int vmid;

> +	int destVmPerm;

int perm;

> +};
> +
> +#define QCOM_SCM_VMID_HLOS       0x3
> +#define QCOM_SCM_VMID_MSS_MSA    0xF
> +#define QCOM_SCM_PERM_READ       0x4
> +#define QCOM_SCM_PERM_WRITE      0x2
> +#define QCOM_SCM_PERM_EXEC       0x1
> +#define QCOM_SCM_PERM_RW (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)

Add QCOM_SCM_PERM_RWX, as this looks fairly common as well.

> +

Regards,
Bjorn

Re: [RESEND: PATCH v4 1/4] firmware: scm: Add new SCM call for switching memory ownership

[Dwivedi, Avaneesh Kumar (avani)]

On 5/26/2017 11:33 AM, Bjorn Andersson wrote:
> On Tue 16 May 11:01 PDT 2017, Avaneesh Kumar Dwivedi wrote:
>
>> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
>> index 93e3b96..4eb7d59 100644
>> --- a/drivers/firmware/qcom_scm-32.c
>> +++ b/drivers/firmware/qcom_scm-32.c
>> @@ -596,3 +596,9 @@ int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size,
>>   {
>>   	return -ENODEV;
>>   }
>> +int __qcom_scm_assign_mem(struct device *dev, phys_addr_t mem_addr,
>> +		size_t mem_sz, phys_addr_t srcVm, size_t srcVm_sz,
>> +		phys_addr_t destVm, size_t destVm_sz)
>> +{
>> +return -ENODEV;
> Indentation please.
OK.
>
>> +}
> [..]
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index bb16510..a2363e2 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -40,6 +40,24 @@ struct qcom_scm {
>>   	struct reset_controller_dev reset;
>>   };
>>   
>> +struct qcom_scm_current_perm_info {
>> +	__le32 destVm;
> __le32 vmid;
Ok
>
>> +	__le32 destVmPerm;
> __le32 perm;
OK
>
>> +	__le64 ctx;
>> +	__le32 ctx_size;
>> +};
>> +
>> +struct qcom_scm_mem_map_info {
>> +	__le64 mem_addr;
>> +	__le64 mem_size;
>> +};
>> +
>> +struct qcom_scm_hyp_map_info {
>> +	__le32 srcVm[2];
>> +	struct qcom_scm_mem_map_info mem_region;
>> +	struct qcom_scm_current_perm_info destVm[2];
>> +};
> As described below, both arrays in this struct should be dynamic size,
> so I recommend dropping the struct and just do the offset math yourself
> in the function.
basically i made sizes of array as static for allocation of memory,
i will now allocate memory based on count of current and next owner set's.
and will remove static size as above.
In fact there is no need for this struct "qcom_scm_hyp_map_info"
Thanks for this comment.
>
>> +
>>   static struct qcom_scm *__scm;
>>   
>>   static int qcom_scm_clk_enable(void)
>> @@ -292,6 +310,63 @@ int qcom_scm_pas_shutdown(u32 peripheral)
>>   }
>>   EXPORT_SYMBOL(qcom_scm_pas_shutdown);
>>   
>> +/**
>> + * qcom_scm_assign_mem() - Provide interface to request to map a memory
>> + * region into intermediate physical address table as well map
>> + * access permissions for any other proc on SOC. So that when other proc
>> + * applies the same intermediate physical address passed by requesting
>> + * processor in this case apps proc, on memory bus it can access the
>> + * region without fault.
> The first line should be a short description of the function.
OK.
>
>> + * @mem_addr: Start pointer of region which need to be mapped.
>> + * @mem_sz: Size of the region.
>> + * @srcVm: Detail of current owners, each set bit in flag indicate id of
>> + * shared owners.
>> + * @newVm: Details of new owners and permission flags for each of them.
>> + * @newVm_sz: Size of array pointed by newVm.
> If necessary (appropriate in your case) a longer description should go
> here - with blank lines before and after.
OK.
>
>> + * Return 0 on success.
> For more info on the format, please see:
>
> https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
>
>> + */
>> +int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcVm,
>> +		struct qcom_scm_destVmPerm *newVm, size_t newVm_sz)
> It's more idiomatic to pass the number of elements in an array than the
> size of the array, so make newVm_sz be the number of items.'
OK, will try to incorporate and if see any issue with it, will revert back.
>
> And please refrain from using camelCase in the Linux kernel.
OK.
>
>> +{
>> +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
>> +	struct qcom_scm_hyp_map_info *hmi;
> Skip this struct and just store the base address in a void *, then have
> one pointer for each of the substructures (to help fill them in).
OK.
>
>> +	phys_addr_t addr[3];
>> +	size_t size[3];
> Please give these 6 variables names.
used array of pointer's just to avoid code looking bulky .
Ok will add 6 separate variables.
>
>> +	int ret;
>> +	int i;
>> +
>> +	hmi = dma_alloc_attrs(__scm->dev, sizeof(*hmi),
>> +			&addr[1], GFP_KERNEL, dma_attrs);
> This function should handle arbitrary number of src and destination vms;
> so start by calculating the hweight of the source, allocate the
> appropriate amount of srcVM and dstVM space and then calculate the
> offsets within that block for each entry.
OK. as mentioned above will make this api independent of number of 
entries in current and next owner set
and allocate memory after based on destination and source counts.
>
>
> Check and handle !hmi
OK
>
>> +	hmi->mem_region.mem_addr = cpu_to_le64(mem_addr);
>> +	hmi->mem_region.mem_size = cpu_to_le64(mem_sz);
>> +
>> +	addr[0] = addr[1] + sizeof(hmi->srcVm);
>> +	size[0] = sizeof(hmi->mem_region);
>> +
>> +	ret = hweight_long(srcVm);
>> +	for (i = 0; i < ret; i++) {
>> +		hmi->srcVm[i] = cpu_to_le32(ffs(srcVm) - 0x1);
> Subtract 1, rather than 0x1
OK.
>
>> +		srcVm ^= 1 << (ffs(srcVm) - 0x1);
> Make this easier to read with:
ok.
>
> 	for (...) {
> 		vmid = ffs(srcVm) - 1;
>
> 		hmi->srcVm[i] = cpu_to_le32(vmid);
> 		srcVm &= ~BIT(vmid);
> 	}
>
>> +	}
>> +	size[1] = ret * sizeof(srcVm);
>> +
>> +	ret = newVm_sz/sizeof(struct qcom_scm_destVmPerm);
>> +	for (i = 0; i < ret; i++) {
>> +		hmi->destVm[i].destVm = cpu_to_le32(newVm[i].destVm);
>> +		hmi->destVm[i].destVmPerm = cpu_to_le32(newVm[i].destVmPerm);
>> +		hmi->destVm[i].ctx = 0;
>> +		hmi->destVm[i].ctx_size = 0;
>> +	}
>> +	addr[2] = addr[0] + sizeof(hmi->mem_region);
>> +	size[2] = ret * sizeof(struct qcom_scm_current_perm_info);
>> +
>> +	ret = __qcom_scm_assign_mem(__scm->dev, addr[0],
>> +		size[0], addr[1], size[1], addr[2], size[2]);
>> +	dma_free_attrs(__scm->dev, sizeof(*hmi), hmi, addr[1], dma_attrs);
>> +	return ret;
> As discussed on IRC, please return negative on failure and otherwise a
> bitmap built from the vmids in the destination list.
OK. will add appropriate code in caller module to handle returned bitmap 
list as new owners for each image.
>
> Also note that when I tested this last time I saw
> __qcom_scm_assign_mem() return positive numbers even though the mapping
> seemed broken afterwards. So it could be that you should treat any
> return value as some sort of error.
will check this while verifying. and incorporate if anything is missing.
>
>> +}
>> +EXPORT_SYMBOL(qcom_scm_assign_mem);
> [..]
>> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
>> index e538047..cb930d9 100644
>> --- a/include/linux/qcom_scm.h
>> +++ b/include/linux/qcom_scm.h
>> @@ -23,6 +23,18 @@ struct qcom_scm_hdcp_req {
>>   	u32 val;
>>   };
>>   
>> +struct qcom_scm_destVmPerm {
> There's no harm in dropping "dest" from this, and please drop the
> camelCase.
OK.
>
>> +	int destVm;
> int vmid;
OK
>
>> +	int destVmPerm;
> int perm;
OK.
>> +};
>> +
>> +#define QCOM_SCM_VMID_HLOS       0x3
>> +#define QCOM_SCM_VMID_MSS_MSA    0xF
>> +#define QCOM_SCM_PERM_READ       0x4
>> +#define QCOM_SCM_PERM_WRITE      0x2
>> +#define QCOM_SCM_PERM_EXEC       0x1
>> +#define QCOM_SCM_PERM_RW (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)
> Add QCOM_SCM_PERM_RWX, as this looks fairly common as well.
You mean change name as QCOM_SCM_PERM_RWX instead of QCOM_SCM_PERM_RW ? OK.
>
>> +
> Regards,
> Bjorn

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

Re: [RESEND: PATCH v4 1/4] firmware: scm: Add new SCM call for switching memory ownership

[Bjorn Andersson]

On Fri 26 May 06:01 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote:
> On 5/26/2017 11:33 AM, Bjorn Andersson wrote:
> > On Tue 16 May 11:01 PDT 2017, Avaneesh Kumar Dwivedi wrote:
[..]
> > > diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
[..]
> > > +#define QCOM_SCM_VMID_HLOS       0x3
> > > +#define QCOM_SCM_VMID_MSS_MSA    0xF
> > > +#define QCOM_SCM_PERM_READ       0x4
> > > +#define QCOM_SCM_PERM_WRITE      0x2
> > > +#define QCOM_SCM_PERM_EXEC       0x1
> > > +#define QCOM_SCM_PERM_RW (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)
> > Add QCOM_SCM_PERM_RWX, as this looks fairly common as well.
> You mean change name as QCOM_SCM_PERM_RWX instead of QCOM_SCM_PERM_RW ? OK.

Provide both RW and RWX, as already in the first client of this API
you're using both.

Thanks,
Bjorn

[RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence

[Avaneesh Kumar Dwivedi]

This patch refactor code to first load all firmware blobs
and then update modem proc to authenticate and boot fw.
Also make a trivial change in a error log.

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

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 8fd697a..2626954 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
 
 	ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
 	if (ret == -ETIMEDOUT)
-		dev_err(qproc->dev, "MPSS header authentication timed out\n");
+		dev_err(qproc->dev, "metadata authentication timed out\n");
 	else if (ret < 0)
-		dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret);
+		dev_err(qproc->dev,
+			"metadata authentication failed: %d\n", ret);
 
 	dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
 
@@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 	bool relocate = false;
 	char seg_name[10];
 	ssize_t offset;
-	size_t size;
+	size_t size = 0;
 	void *ptr;
 	int ret;
 	int i;
@@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 	}
 
 	mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
-
+	/* Load firmware completely before letting mss to start
+	 * authentication and then boot firmware
+	 */
 	for (i = 0; i < ehdr->e_phnum; i++) {
 		phdr = &phdrs[i];
 
@@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 			memset(ptr + phdr->p_filesz, 0,
 			       phdr->p_memsz - phdr->p_filesz);
 		}
-
-		size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
-		if (!size) {
-			boot_addr = relocate ? qproc->mpss_phys : min_addr;
-			writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
-			writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
-		}
-
 		size += phdr->p_memsz;
-		writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
 	}
+	/* Transfer ownership of modem region with modem fw */
+	boot_addr = relocate ? qproc->mpss_phys : min_addr;
+	writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
+	writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
+	writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
 
 	ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000);
 	if (ret == -ETIMEDOUT)
-- 
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: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence

[Sricharan R]

Hi Bjorn/Avaneesh,

On 5/16/2017 11:32 PM, Avaneesh Kumar Dwivedi wrote:
> This patch refactor code to first load all firmware blobs
> and then update modem proc to authenticate and boot fw.
> Also make a trivial change in a error log.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 8fd697a..2626954 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>  
>  	ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
>  	if (ret == -ETIMEDOUT)
> -		dev_err(qproc->dev, "MPSS header authentication timed out\n");
> +		dev_err(qproc->dev, "metadata authentication timed out\n");
>  	else if (ret < 0)
> -		dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret);
> +		dev_err(qproc->dev,
> +			"metadata authentication failed: %d\n", ret);
>  
>  	dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
>  
> @@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>  	bool relocate = false;
>  	char seg_name[10];
>  	ssize_t offset;
> -	size_t size;
> +	size_t size = 0;
>  	void *ptr;
>  	int ret;
>  	int i;
> @@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>  	}
>  
>  	mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
> -
> +	/* Load firmware completely before letting mss to start
> +	 * authentication and then boot firmware
> +	 */
>  	for (i = 0; i < ehdr->e_phnum; i++) {
>  		phdr = &phdrs[i];
>  
> @@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>  			memset(ptr + phdr->p_filesz, 0,
>  			       phdr->p_memsz - phdr->p_filesz);
>  		}
> -
> -		size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> -		if (!size) {
> -			boot_addr = relocate ? qproc->mpss_phys : min_addr;
> -			writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
> -			writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
> -		}
> -
>  		size += phdr->p_memsz;
> -		writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>  	}

So while moving this down, can we use qcom_mdt_load instead for the mpss
image loading part above ?

> +	/* Transfer ownership of modem region with modem fw */
> +	boot_addr = relocate ? qproc->mpss_phys : min_addr;
> +	writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
> +	writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
> +	writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);

For ipq8074 [1], wcnss core has an Q6V5 version of the ip for which the
initialization/boot sequence is pretty much the same as that has been added
for msm8996 in this series. So wanted to understand if its better to
use this remoteproc itself by keeping the Q6 and mpss parts separately (or)
add a new remoteproc ?

[1] https://patchwork.kernel.org/patch/9711725/

Regards,
 Sricharan

>  
>  	ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000);
>  	if (ret == -ETIMEDOUT)
> 

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

Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence

[Dwivedi, Avaneesh Kumar (avani)]

On 5/20/2017 8:25 AM, Sricharan R wrote:
> Hi Bjorn/Avaneesh,
>
> On 5/16/2017 11:32 PM, Avaneesh Kumar Dwivedi wrote:
>> This patch refactor code to first load all firmware blobs
>> and then update modem proc to authenticate and boot fw.
>> Also make a trivial change in a error log.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 25 ++++++++++++-------------
>>   1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 8fd697a..2626954 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>>   
>>   	ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
>>   	if (ret == -ETIMEDOUT)
>> -		dev_err(qproc->dev, "MPSS header authentication timed out\n");
>> +		dev_err(qproc->dev, "metadata authentication timed out\n");
>>   	else if (ret < 0)
>> -		dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret);
>> +		dev_err(qproc->dev,
>> +			"metadata authentication failed: %d\n", ret);
>>   
>>   	dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
>>   
>> @@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>   	bool relocate = false;
>>   	char seg_name[10];
>>   	ssize_t offset;
>> -	size_t size;
>> +	size_t size = 0;
>>   	void *ptr;
>>   	int ret;
>>   	int i;
>> @@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>   	}
>>   
>>   	mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
>> -
>> +	/* Load firmware completely before letting mss to start
>> +	 * authentication and then boot firmware
>> +	 */
>>   	for (i = 0; i < ehdr->e_phnum; i++) {
>>   		phdr = &phdrs[i];
>>   
>> @@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>   			memset(ptr + phdr->p_filesz, 0,
>>   			       phdr->p_memsz - phdr->p_filesz);
>>   		}
>> -
>> -		size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>> -		if (!size) {
>> -			boot_addr = relocate ? qproc->mpss_phys : min_addr;
>> -			writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
>> -			writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>> -		}
>> -
>>   		size += phdr->p_memsz;
>> -		writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>   	}
> So while moving this down, can we use qcom_mdt_load instead for the mpss
> image loading part above ?
qcom_mdt_load() can not be used to load segments for mpss, as MPSS blobs 
are self authenticated.
while qcom_mdt_load() is used in cases where authentication of loaded 
blobs is done by trustzone.
for that qcom_mdt_load() does extra steps to send pas_id to trustzone 
and mem_setup() etc.
>> +	/* Transfer ownership of modem region with modem fw */
>> +	boot_addr = relocate ? qproc->mpss_phys : min_addr;
>> +	writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
>> +	writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>> +	writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> For ipq8074 [1], wcnss core has an Q6V5 version of the ip for which the
> initialization/boot sequence is pretty much the same as that has been added
> for msm8996 in this series. So wanted to understand if its better to
> use this remoteproc itself by keeping the Q6 and mpss parts separately (or)
> add a new remoteproc ?
Bjorn can better answer this query, but i believe this remoteproc can be 
extended to load
mpss part by adding private initialization for the IP.
>
> [1] https://patchwork.kernel.org/patch/9711725/
>
> Regards,
>   Sricharan
>
>>   
>>   	ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000);
>>   	if (ret == -ETIMEDOUT)
>>

-- 
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: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence

[Sricharan R]

Hi,

On 5/22/2017 3:03 PM, Dwivedi, Avaneesh Kumar (avani) wrote:
> 
> 
> On 5/20/2017 8:25 AM, Sricharan R wrote:
>> Hi Bjorn/Avaneesh,
>>
>> On 5/16/2017 11:32 PM, Avaneesh Kumar Dwivedi wrote:
>>> This patch refactor code to first load all firmware blobs
>>> and then update modem proc to authenticate and boot fw.
>>> Also make a trivial change in a error log.
>>>
>>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>>> ---
>>>   drivers/remoteproc/qcom_q6v5_pil.c | 25 ++++++++++++-------------
>>>   1 file changed, 12 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>>> index 8fd697a..2626954 100644
>>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>>> @@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>>>         ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
>>>       if (ret == -ETIMEDOUT)
>>> -        dev_err(qproc->dev, "MPSS header authentication timed out\n");
>>> +        dev_err(qproc->dev, "metadata authentication timed out\n");
>>>       else if (ret < 0)
>>> -        dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret);
>>> +        dev_err(qproc->dev,
>>> +            "metadata authentication failed: %d\n", ret);
>>>         dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
>>>   @@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>>       bool relocate = false;
>>>       char seg_name[10];
>>>       ssize_t offset;
>>> -    size_t size;
>>> +    size_t size = 0;
>>>       void *ptr;
>>>       int ret;
>>>       int i;
>>> @@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>>       }
>>>         mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
>>> -
>>> +    /* Load firmware completely before letting mss to start
>>> +     * authentication and then boot firmware
>>> +     */
>>>       for (i = 0; i < ehdr->e_phnum; i++) {
>>>           phdr = &phdrs[i];
>>>   @@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>>               memset(ptr + phdr->p_filesz, 0,
>>>                      phdr->p_memsz - phdr->p_filesz);
>>>           }
>>> -
>>> -        size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>> -        if (!size) {
>>> -            boot_addr = relocate ? qproc->mpss_phys : min_addr;
>>> -            writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
>>> -            writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>>> -        }
>>> -
>>>           size += phdr->p_memsz;
>>> -        writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>>       }
>> So while moving this down, can we use qcom_mdt_load instead for the mpss
>> image loading part above ?
> qcom_mdt_load() can not be used to load segments for mpss, as MPSS blobs are self authenticated.
> while qcom_mdt_load() is used in cases where authentication of loaded blobs is done by trustzone.
> for that qcom_mdt_load() does extra steps to send pas_id to trustzone and mem_setup() etc.

Right, so my intention of asking this was if the code which does the calculation and
loads the segments in qcom_mdt_load can somehow be abstracted out, so that future
self authenticating rproc (even mpss in this case) can use them to load mdt ?

>>> +    /* Transfer ownership of modem region with modem fw */
>>> +    boot_addr = relocate ? qproc->mpss_phys : min_addr;
>>> +    writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
>>> +    writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>>> +    writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>> For ipq8074 [1], wcnss core has an Q6V5 version of the ip for which the
>> initialization/boot sequence is pretty much the same as that has been added
>> for msm8996 in this series. So wanted to understand if its better to
>> use this remoteproc itself by keeping the Q6 and mpss parts separately (or)
>> add a new remoteproc ?
> Bjorn can better answer this query, but i believe this remoteproc can be extended to load
> mpss part by adding private initialization for the IP.

ya, the mpss part can be separated out, so that this can be a Q6 + MPSS (or) Q6 + WCNSS
remoteproc. Was asking this to get the right way for adding the Q6 + WCNSS remoteproc,
as the Q6 part is same what you have added for msm8996.

Regards,
 Sricharan

>>
>> [1] https://patchwork.kernel.org/patch/9711725/
>>
>> Regards,
>>   Sricharan
>>
>>>         ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000);
>>>       if (ret == -ETIMEDOUT)
>>>
> 

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

Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence

[Dwivedi, Avaneesh Kumar (avani)]

On 5/22/2017 4:07 PM, Sricharan R wrote:
> Hi,
>
> On 5/22/2017 3:03 PM, Dwivedi, Avaneesh Kumar (avani) wrote:
>>
>> On 5/20/2017 8:25 AM, Sricharan R wrote:
>>> Hi Bjorn/Avaneesh,
>>>
>>> On 5/16/2017 11:32 PM, Avaneesh Kumar Dwivedi wrote:
>>>> This patch refactor code to first load all firmware blobs
>>>> and then update modem proc to authenticate and boot fw.
>>>> Also make a trivial change in a error log.
>>>>
>>>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>>>> ---
>>>>    drivers/remoteproc/qcom_q6v5_pil.c | 25 ++++++++++++-------------
>>>>    1 file changed, 12 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>>>> index 8fd697a..2626954 100644
>>>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>>>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>>>> @@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>>>>          ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
>>>>        if (ret == -ETIMEDOUT)
>>>> -        dev_err(qproc->dev, "MPSS header authentication timed out\n");
>>>> +        dev_err(qproc->dev, "metadata authentication timed out\n");
>>>>        else if (ret < 0)
>>>> -        dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret);
>>>> +        dev_err(qproc->dev,
>>>> +            "metadata authentication failed: %d\n", ret);
>>>>          dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
>>>>    @@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>>>        bool relocate = false;
>>>>        char seg_name[10];
>>>>        ssize_t offset;
>>>> -    size_t size;
>>>> +    size_t size = 0;
>>>>        void *ptr;
>>>>        int ret;
>>>>        int i;
>>>> @@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>>>        }
>>>>          mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
>>>> -
>>>> +    /* Load firmware completely before letting mss to start
>>>> +     * authentication and then boot firmware
>>>> +     */
>>>>        for (i = 0; i < ehdr->e_phnum; i++) {
>>>>            phdr = &phdrs[i];
>>>>    @@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>>>                memset(ptr + phdr->p_filesz, 0,
>>>>                       phdr->p_memsz - phdr->p_filesz);
>>>>            }
>>>> -
>>>> -        size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>>> -        if (!size) {
>>>> -            boot_addr = relocate ? qproc->mpss_phys : min_addr;
>>>> -            writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
>>>> -            writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>>>> -        }
>>>> -
>>>>            size += phdr->p_memsz;
>>>> -        writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>>>        }
>>> So while moving this down, can we use qcom_mdt_load instead for the mpss
>>> image loading part above ?
>> qcom_mdt_load() can not be used to load segments for mpss, as MPSS blobs are self authenticated.
>> while qcom_mdt_load() is used in cases where authentication of loaded blobs is done by trustzone.
>> for that qcom_mdt_load() does extra steps to send pas_id to trustzone and mem_setup() etc.
> Right, so my intention of asking this was if the code which does the calculation and
> loads the segments in qcom_mdt_load can somehow be abstracted out, so that future
> self authenticating rproc (even mpss in this case) can use them to load mdt ?
As i understand, you want to replace the piece of code which does parse 
mdt and load individual firmware blobs in a separate routine.
So that it can be called by any one without again doing parsing and 
loading for self authentication.?
Till now only MPSS does rely on self authentication, all other 
subsystems use qcom_mdt_load().
I think this is reason why the qcom_mdt_load() equivalent routine has 
not been used.
Bjorn may further add in this.
>
>>>> +    /* Transfer ownership of modem region with modem fw */
>>>> +    boot_addr = relocate ? qproc->mpss_phys : min_addr;
>>>> +    writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
>>>> +    writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>>>> +    writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>> For ipq8074 [1], wcnss core has an Q6V5 version of the ip for which the
>>> initialization/boot sequence is pretty much the same as that has been added
>>> for msm8996 in this series. So wanted to understand if its better to
>>> use this remoteproc itself by keeping the Q6 and mpss parts separately (or)
>>> add a new remoteproc ?
>> Bjorn can better answer this query, but i believe this remoteproc can be extended to load
>> mpss part by adding private initialization for the IP.
> ya, the mpss part can be separated out, so that this can be a Q6 + MPSS (or) Q6 + WCNSS
> remoteproc. Was asking this to get the right way for adding the Q6 + WCNSS remoteproc,
> as the Q6 part is same what you have added for msm8996.
Again, i believe yes but leave Bjorn to make final comment.
>
> Regards,
>   Sricharan
>
>>> [1] https://patchwork.kernel.org/patch/9711725/
>>>
>>> Regards,
>>>    Sricharan
>>>
>>>>          ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000);
>>>>        if (ret == -ETIMEDOUT)
>>>>

-- 
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: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence

[Bjorn Andersson]

On Mon 22 May 06:26 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote:
> On 5/22/2017 4:07 PM, Sricharan R wrote:
> > Hi,
> > 
> > On 5/22/2017 3:03 PM, Dwivedi, Avaneesh Kumar (avani) wrote:
> > > 
> > > On 5/20/2017 8:25 AM, Sricharan R wrote:
> > > > Hi Bjorn/Avaneesh,
> > > > 
> > > > On 5/16/2017 11:32 PM, Avaneesh Kumar Dwivedi wrote:
[..]
> > > > > -
> > > > > -        size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> > > > > -        if (!size) {
> > > > > -            boot_addr = relocate ? qproc->mpss_phys : min_addr;
> > > > > -            writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
> > > > > -            writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
> > > > > -        }
> > > > > -
> > > > >            size += phdr->p_memsz;
> > > > > -        writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> > > > >        }
> > > > So while moving this down, can we use qcom_mdt_load instead for
> > > > the mpss image loading part above ?
> > > qcom_mdt_load() can not be used to load segments for mpss, as MPSS
> > > blobs are self authenticated.  while qcom_mdt_load() is used in
> > > cases where authentication of loaded blobs is done by trustzone.
> > > for that qcom_mdt_load() does extra steps to send pas_id to
> > > trustzone and mem_setup() etc.
> > Right, so my intention of asking this was if the code which does the
> > calculation and loads the segments in qcom_mdt_load can somehow be
> > abstracted out, so that future self authenticating rproc (even mpss
> > in this case) can use them to load mdt ?
> As i understand, you want to replace the piece of code which does
> parse mdt and load individual firmware blobs in a separate routine.
> So that it can be called by any one without again doing parsing and
> loading for self authentication.?  Till now only MPSS does rely on
> self authentication, all other subsystems use qcom_mdt_load().  I
> think this is reason why the qcom_mdt_load() equivalent routine has
> not been used.  Bjorn may further add in this.

I have not been able to come up with a clean way to provide a useful
mdt-loader abstraction that works for the SCM PILs, the
self-authenticated PILs and the non-PIL SCM users.

Further more with the upcoming ramdump support we will need to extract
segment information from the mdt header, so we will have to revisit this
topic.


Regardless, I would prefer that we follow up with such refactoring after
getting this series sorted out.

> > 
> > > > > +    /* Transfer ownership of modem region with modem fw */
> > > > > +    boot_addr = relocate ? qproc->mpss_phys : min_addr;
> > > > > +    writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
> > > > > +    writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
> > > > > +    writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> > > > For ipq8074 [1], wcnss core has an Q6V5 version of the ip for
> > > > which the initialization/boot sequence is pretty much the same
> > > > as that has been added for msm8996 in this series. So wanted to
> > > > understand if its better to use this remoteproc itself by
> > > > keeping the Q6 and mpss parts separately (or) add a new
> > > > remoteproc ?
> > > Bjorn can better answer this query, but i believe this remoteproc
> > > can be extended to load mpss part by adding private initialization
> > > for the IP.
> > ya, the mpss part can be separated out, so that this can be a Q6 +
> > MPSS (or) Q6 + WCNSS remoteproc. Was asking this to get the right
> > way for adding the Q6 + WCNSS remoteproc, as the Q6 part is same
> > what you have added for msm8996.
> Again, i believe yes but leave Bjorn to make final comment.

It definitely sounds like there's room for reuse here, how much of the
initialization and authentication sequences are common between the two?

Regards,
Bjorn

Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence

[Sricharan R]

Hi Bjorn,

On 5/26/2017 12:33 AM, Bjorn Andersson wrote:
> On Mon 22 May 06:26 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote:
>> On 5/22/2017 4:07 PM, Sricharan R wrote:
>>> Hi,
>>>
>>> On 5/22/2017 3:03 PM, Dwivedi, Avaneesh Kumar (avani) wrote:
>>>>
>>>> On 5/20/2017 8:25 AM, Sricharan R wrote:
>>>>> Hi Bjorn/Avaneesh,
>>>>>
>>>>> On 5/16/2017 11:32 PM, Avaneesh Kumar Dwivedi wrote:
> [..]
>>>>>> -
>>>>>> -        size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>>>>> -        if (!size) {
>>>>>> -            boot_addr = relocate ? qproc->mpss_phys : min_addr;
>>>>>> -            writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
>>>>>> -            writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>>>>>> -        }
>>>>>> -
>>>>>>            size += phdr->p_memsz;
>>>>>> -        writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>>>>>        }
>>>>> So while moving this down, can we use qcom_mdt_load instead for
>>>>> the mpss image loading part above ?
>>>> qcom_mdt_load() can not be used to load segments for mpss, as MPSS
>>>> blobs are self authenticated.  while qcom_mdt_load() is used in
>>>> cases where authentication of loaded blobs is done by trustzone.
>>>> for that qcom_mdt_load() does extra steps to send pas_id to
>>>> trustzone and mem_setup() etc.
>>> Right, so my intention of asking this was if the code which does the
>>> calculation and loads the segments in qcom_mdt_load can somehow be
>>> abstracted out, so that future self authenticating rproc (even mpss
>>> in this case) can use them to load mdt ?
>> As i understand, you want to replace the piece of code which does
>> parse mdt and load individual firmware blobs in a separate routine.
>> So that it can be called by any one without again doing parsing and
>> loading for self authentication.?  Till now only MPSS does rely on
>> self authentication, all other subsystems use qcom_mdt_load().  I
>> think this is reason why the qcom_mdt_load() equivalent routine has
>> not been used.  Bjorn may further add in this.
> 
> I have not been able to come up with a clean way to provide a useful
> mdt-loader abstraction that works for the SCM PILs, the
> self-authenticated PILs and the non-PIL SCM users.
> 
> Further more with the upcoming ramdump support we will need to extract
> segment information from the mdt header, so we will have to revisit this
> topic.
> 
> 
> Regardless, I would prefer that we follow up with such refactoring after
> getting this series sorted out.
> 

ok, agree.  While trying to add Q6 support for ipq8074, which is again
a self-authenticating PIL with mdt, i can try this.

>>>
>>>>>> +    /* Transfer ownership of modem region with modem fw */
>>>>>> +    boot_addr = relocate ? qproc->mpss_phys : min_addr;
>>>>>> +    writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
>>>>>> +    writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>>>>>> +    writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>>>> For ipq8074 [1], wcnss core has an Q6V5 version of the ip for
>>>>> which the initialization/boot sequence is pretty much the same
>>>>> as that has been added for msm8996 in this series. So wanted to
>>>>> understand if its better to use this remoteproc itself by
>>>>> keeping the Q6 and mpss parts separately (or) add a new
>>>>> remoteproc ?
>>>> Bjorn can better answer this query, but i believe this remoteproc
>>>> can be extended to load mpss part by adding private initialization
>>>> for the IP.
>>> ya, the mpss part can be separated out, so that this can be a Q6 +
>>> MPSS (or) Q6 + WCNSS remoteproc. Was asking this to get the right
>>> way for adding the Q6 + WCNSS remoteproc, as the Q6 part is same
>>> what you have added for msm8996.
>> Again, i believe yes but leave Bjorn to make final comment.
> 
> It definitely sounds like there's room for reuse here, how much of the
> initialization and authentication sequences are common between the two?

The initialization sequence is exactly the same as what was done for
msm8996(Q6) one added in this series. So for reusing this driver for Q6,
the Q6 + MPSS has to be decoupled and driver has to look common for
Q6 + any, (ie) Q6 + mpss (or) Q6 + wcnss. Incase if that's not neat,
atleast the Q6 initialization sequence can be reused.

Regards,
 Sricharan

> 
> Regards,
> Bjorn
> 

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

Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence

[Bjorn Andersson]

On Tue 16 May 11:02 PDT 2017, Avaneesh Kumar Dwivedi wrote:

> This patch refactor code to first load all firmware blobs
> and then update modem proc to authenticate and boot fw.

Nice, I like this! Just some style details below.

> Also make a trivial change in a error log.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 8fd697a..2626954 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>  
>  	ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
>  	if (ret == -ETIMEDOUT)
> -		dev_err(qproc->dev, "MPSS header authentication timed out\n");
> +		dev_err(qproc->dev, "metadata authentication timed out\n");
>  	else if (ret < 0)
> -		dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret);
> +		dev_err(qproc->dev,
> +			"metadata authentication failed: %d\n", ret);

I'm happy to accept these changes if they better describe the errors,
but please send them in a separate patch in that case - and please don't
line break that second print.

>  
>  	dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
>  
> @@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>  	bool relocate = false;
>  	char seg_name[10];
>  	ssize_t offset;
> -	size_t size;
> +	size_t size = 0;
>  	void *ptr;
>  	int ret;
>  	int i;
> @@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>  	}
>  
>  	mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
> -
> +	/* Load firmware completely before letting mss to start
> +	 * authentication and then boot firmware
> +	 */

/*
 * The first line of a multi-line comment should be empty.
 */

Also your comment tend to tell the story of your change, that's what the
commit message is for, the comment should describe the code regardless
of history;

I think it makes sense to have a comment in the lines of:

/* Load the firmware segments */

>  	for (i = 0; i < ehdr->e_phnum; i++) {
>  		phdr = &phdrs[i];
>  
> @@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>  			memset(ptr + phdr->p_filesz, 0,
>  			       phdr->p_memsz - phdr->p_filesz);
>  		}
> -
> -		size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> -		if (!size) {
> -			boot_addr = relocate ? qproc->mpss_phys : min_addr;
> -			writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
> -			writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
> -		}
> -
>  		size += phdr->p_memsz;
> -		writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>  	}

Please put a blank line here, to separate the steps like "paragraphs".

> +	/* Transfer ownership of modem region with modem fw */
> +	boot_addr = relocate ? qproc->mpss_phys : min_addr;
> +	writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
> +	writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
> +	writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);

I originally did something similar, but ran into some issue - so I will
test this on 8974 and 8916 as soon as my devboards are back online.

Regards,
Bjorn

Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence

[Dwivedi, Avaneesh Kumar (avani)]

On 5/26/2017 12:43 AM, Bjorn Andersson wrote:
> On Tue 16 May 11:02 PDT 2017, Avaneesh Kumar Dwivedi wrote:
>
>> This patch refactor code to first load all firmware blobs
>> and then update modem proc to authenticate and boot fw.
> Nice, I like this! Just some style details below.
Thanks. Sure will correct.
>
>> Also make a trivial change in a error log.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 25 ++++++++++++-------------
>>   1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 8fd697a..2626954 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>>   
>>   	ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_META_DATA_AUTH_SUCCESS, 1000);
>>   	if (ret == -ETIMEDOUT)
>> -		dev_err(qproc->dev, "MPSS header authentication timed out\n");
>> +		dev_err(qproc->dev, "metadata authentication timed out\n");
>>   	else if (ret < 0)
>> -		dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret);
>> +		dev_err(qproc->dev,
>> +			"metadata authentication failed: %d\n", ret);
> I'm happy to accept these changes if they better describe the errors,
> but please send them in a separate patch in that case - and please don't
> line break that second print.
OK.  Will reconsider.
>
>>   
>>   	dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
>>   
>> @@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>   	bool relocate = false;
>>   	char seg_name[10];
>>   	ssize_t offset;
>> -	size_t size;
>> +	size_t size = 0;
>>   	void *ptr;
>>   	int ret;
>>   	int i;
>> @@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>   	}
>>   
>>   	mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
>> -
>> +	/* Load firmware completely before letting mss to start
>> +	 * authentication and then boot firmware
>> +	 */
> /*
>   * The first line of a multi-line comment should be empty.
>   */
>
> Also your comment tend to tell the story of your change, that's what the
> commit message is for, the comment should describe the code regardless
> of history;
>
> I think it makes sense to have a comment in the lines of:
>
> /* Load the firmware segments */
OK.
>
>>   	for (i = 0; i < ehdr->e_phnum; i++) {
>>   		phdr = &phdrs[i];
>>   
>> @@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>   			memset(ptr + phdr->p_filesz, 0,
>>   			       phdr->p_memsz - phdr->p_filesz);
>>   		}
>> -
>> -		size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>> -		if (!size) {
>> -			boot_addr = relocate ? qproc->mpss_phys : min_addr;
>> -			writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
>> -			writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>> -		}
>> -
>>   		size += phdr->p_memsz;
>> -		writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>   	}
> Please put a blank line here, to separate the steps like "paragraphs".
OK
>
>> +	/* Transfer ownership of modem region with modem fw */
>> +	boot_addr = relocate ? qproc->mpss_phys : min_addr;
>> +	writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
>> +	writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
>> +	writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
> I originally did something similar, but ran into some issue - so I will
> test this on 8974 and 8916 as soon as my devboards are back online.
OK
>
> Regards,
> Bjorn

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

[RESEND: PATCH v4 3/4] remoteproc: qcom: Make secure world call for mem ownership switch

[Avaneesh Kumar Dwivedi]

MSS proc on msm8996 can not access fw loaded region without stage
second translation of memory pages where mpss image are loaded.
This patch in order to enable mss boot on msm8996 invoke scm call
to switch or share ownership between apps and modem.

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

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 2626954..57a4cfec 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -110,6 +110,7 @@ struct rproc_hexagon_res {
 	struct qcom_mss_reg_res *active_supply;
 	char **proxy_clk_names;
 	char **active_clk_names;
+	bool need_mem_protection;
 };
 
 struct q6v5 {
@@ -151,6 +152,7 @@ struct q6v5 {
 	phys_addr_t mpss_reloc;
 	void *mpss_region;
 	size_t mpss_size;
+	bool need_mem_protection;
 
 	struct qcom_rproc_subdev smd_subdev;
 };
@@ -288,6 +290,43 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
 	return &table;
 }
 
+static int q6v5_assign_mem_to_subsys(struct q6v5 *qproc,
+		phys_addr_t addr, size_t size)
+{
+	struct qcom_scm_destVmPerm next[] = {{ QCOM_SCM_VMID_MSS_MSA,
+		(QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)} };
+	int ret;
+
+	size = ALIGN(size, SZ_4K);
+	if (!qproc->need_mem_protection)
+		return 0;
+	ret = qcom_scm_assign_mem(addr, size,
+		BIT(QCOM_SCM_VMID_HLOS), next, sizeof(next));
+	if (ret)
+		pr_err("%s: Failed to assign memory access, ret = %d\n",
+			__func__, ret);
+	return ret;
+}
+
+static int q6v5_assign_mem_to_linux(struct q6v5 *qproc,
+		phys_addr_t addr, size_t size)
+{
+	struct qcom_scm_destVmPerm next[] = { { QCOM_SCM_VMID_HLOS,
+		(QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_EXEC)}
+		};
+	int ret;
+
+	size = ALIGN(size, SZ_4K);
+	if (!qproc->need_mem_protection)
+		return 0;
+	ret = qcom_scm_assign_mem(addr, size,
+		BIT(QCOM_SCM_VMID_MSS_MSA), next, sizeof(next));
+	if (ret)
+		pr_err("%s: Failed to assign memory access, ret = %d\n",
+			__func__, ret);
+	return ret;
+}
+
 static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
 {
 	struct q6v5 *qproc = rproc->priv;
@@ -461,6 +500,13 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
 
 	memcpy(ptr, fw->data, fw->size);
 
+	/* Hypervisor mapping to access metadata by modem */
+	ret = q6v5_assign_mem_to_subsys(qproc, phys, fw->size);
+	if (ret) {
+		dev_err(qproc->dev,
+			"Failed to assign metadata 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);
 
@@ -471,6 +517,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
 		dev_err(qproc->dev,
 			"metadata authentication failed: %d\n", ret);
 
+	/* Metadata authentication done, remove modem access */
+	ret = q6v5_assign_mem_to_linux(qproc, phys, fw->size);
+	if (ret)
+		dev_err(qproc->dev,
+			"Failed to reclaim metadata memory, ret - %d\n", ret);
 	dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
 
 	return ret < 0 ? ret : 0;
@@ -581,6 +632,10 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 	}
 	/* Transfer ownership of modem region with modem fw */
 	boot_addr = relocate ? qproc->mpss_phys : min_addr;
+	ret = q6v5_assign_mem_to_subsys(qproc,
+		qproc->mpss_phys, qproc->mpss_size);
+	if (ret)
+		return ret;
 	writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG);
 	writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
 	writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
@@ -600,6 +655,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 static int q6v5_start(struct rproc *rproc)
 {
 	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
+	int assign_mem_result;
 	int ret;
 
 	ret = q6v5_regulator_enable(qproc, qproc->proxy_regs,
@@ -635,6 +691,13 @@ static int q6v5_start(struct rproc *rproc)
 		goto assert_reset;
 	}
 
+	ret = q6v5_assign_mem_to_subsys(qproc,
+		qproc->mba_phys, qproc->mba_size);
+	if (ret) {
+		dev_err(qproc->dev,
+			"Failed to assign mba memory access, ret - %d\n", ret);
+		goto assert_reset;
+	}
 	writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
 
 	ret = q6v5proc_reset(qproc);
@@ -656,16 +719,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;
 	}
 
+	ret = q6v5_assign_mem_to_linux(qproc,
+		qproc->mba_phys, qproc->mba_size);
+	if (ret)
+		dev_err(qproc->dev,
+			"Failed to reclaim mba memory, ret - %d\n", ret);
 	qproc->running = true;
 
 	q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
@@ -675,12 +743,19 @@ static int q6v5_start(struct rproc *rproc)
 
 	return 0;
 
+reclaim_mem:
+	assign_mem_result =
+		q6v5_assign_mem_to_linux(qproc,
+		qproc->mpss_phys, qproc->mpss_size);
 halt_axi_ports:
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
 	q6v5_clk_disable(qproc->dev, qproc->active_clks,
 			 qproc->active_clk_count);
+	assign_mem_result =
+		q6v5_assign_mem_to_linux(qproc,
+		qproc->mba_phys, qproc->mba_size);
 assert_reset:
 	reset_control_assert(qproc->mss_restart);
 disable_vdd:
@@ -717,6 +792,8 @@ 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);
 
+	ret = q6v5_assign_mem_to_linux(qproc,
+			qproc->mpss_phys, qproc->mpss_size);
 	reset_control_assert(qproc->mss_restart);
 	q6v5_clk_disable(qproc->dev, qproc->active_clks,
 			 qproc->active_clk_count);
@@ -1014,6 +1091,7 @@ static int q6v5_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
+	qproc->need_mem_protection = desc->need_mem_protection;
 	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
 	if (ret < 0)
 		goto free_rproc;
@@ -1089,6 +1167,7 @@ static int q6v5_remove(struct platform_device *pdev)
 		"mem",
 		NULL
 	},
+	.need_mem_protection = false,
 };
 
 static const struct rproc_hexagon_res msm8974_mss = {
@@ -1126,6 +1205,7 @@ static int q6v5_remove(struct platform_device *pdev)
 		"mem",
 		NULL
 	},
+	.need_mem_protection = false,
 };
 
 static const struct of_device_id 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: [RESEND: PATCH v4 3/4] remoteproc: qcom: Make secure world call for mem ownership switch

[Bjorn Andersson]

On Tue 16 May 11:02 PDT 2017, Avaneesh Kumar Dwivedi wrote:

> +static int q6v5_assign_mem_to_subsys(struct q6v5 *qproc,
> +		phys_addr_t addr, size_t size)
> +{
> +	struct qcom_scm_destVmPerm next[] = {{ QCOM_SCM_VMID_MSS_MSA,
> +		(QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)} };

struct qcom_scm_destVmPerm next = {
	.vmid = QCOM_SCM_VMID_MSS_MSA,
	.perm = QCOM_SCM_PERM_RW
};

> +	int ret;
> +
> +	size = ALIGN(size, SZ_4K);

I believe it will be cleaner if you just put this alignment directly in
the function call below.

> +	if (!qproc->need_mem_protection)
> +		return 0;

Put a blank line here, to give separation between the sections of the
function.

> +	ret = qcom_scm_assign_mem(addr, size,
> +		BIT(QCOM_SCM_VMID_HLOS), next, sizeof(next));

	qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K),
			    BIT(QCOM_SCM_VMID_HLOS), &next, 1);

> +	if (ret)
> +		pr_err("%s: Failed to assign memory access, ret = %d\n",
> +			__func__, ret);

There's no point in printing an error here and in the calling function,
but as it makes sense to have the error message to include which memory
region we operated on (mba vs mpss) I think you should remove the print
here and keep it in the callers.

So just return qcom-scm_assign_mem().

> +	return ret;
> +}
> +
> +static int q6v5_assign_mem_to_linux(struct q6v5 *qproc,
> +		phys_addr_t addr, size_t size)
> +{
> +	struct qcom_scm_destVmPerm next[] = { { QCOM_SCM_VMID_HLOS,
> +		(QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_EXEC)}
> +		};

struct qcom_scm_destVmPerm next = {
	.vmid = QCOM_SCM_VMID_HLOS,
	.perm = QCOM_SCM_PERM_RWX,
};

(And add RWX to the list of defines in patch 1)

[..]
> @@ -471,6 +517,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>  		dev_err(qproc->dev,
>  			"metadata authentication failed: %d\n", ret);
>  
> +	/* Metadata authentication done, remove modem access */
> +	ret = q6v5_assign_mem_to_linux(qproc, phys, fw->size);
> +	if (ret)
> +		dev_err(qproc->dev,
> +			"Failed to reclaim metadata memory, ret - %d\n", ret);

If this assignment fails (for any reason) we can't return the memory to
the free pool in Linux, because at some point in the future these pages
will be allocated to someone else resulting in a memory access violation
scenario that will be just terrible to debug.

I do however not have a better idea at the moment than just leaking the
memory.

>  	dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
>  
>  	return ret < 0 ? ret : 0;
[..]
> @@ -656,16 +719,21 @@ static int q6v5_start(struct rproc *rproc)
>  
>  	ret = q6v5_mpss_load(qproc);
>  	if (ret)
> -		goto halt_axi_ports;
> +		goto reclaim_mem;

This doesn't allow us to distinguish between failures before or after
the memory assignment in mpss_load(), so although you're duplicating the
reclaim code, mpss_load() must be responsible of restoring the state on
failure.

The timeout below still has to reclaim the memory.

>  
>  	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;
>  	}
>  
> +	ret = q6v5_assign_mem_to_linux(qproc,
> +		qproc->mba_phys, qproc->mba_size);
> +	if (ret)
> +		dev_err(qproc->dev,
> +			"Failed to reclaim mba memory, ret - %d\n", ret);

I think it's okay for symmetrical purposes to keep the memory assigned
to the remote until we call stop().

Although this shows that we should be able to release the mba allocation
here.

>  	qproc->running = true;
>  
>  	q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
> @@ -675,12 +743,19 @@ static int q6v5_start(struct rproc *rproc)
>  
>  	return 0;
>  
> +reclaim_mem:
> +	assign_mem_result =
> +		q6v5_assign_mem_to_linux(qproc,
> +		qproc->mpss_phys, qproc->mpss_size);

If this fails we're screwed. We have no way to fail in a way that will
not free the memory at any later point in time.

So I do believe you should have a BUG_ON(assign_mem_result); here. With
a clear comment in the lines of:

/*
 * Failed to reclaim memory, any future access to these pages will cause
 * security violations and a seemingly random crash
 */

>  halt_axi_ports:
>  	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
>  	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
>  	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>  	q6v5_clk_disable(qproc->dev, qproc->active_clks,
>  			 qproc->active_clk_count);
> +	assign_mem_result =
> +		q6v5_assign_mem_to_linux(qproc,
> +		qproc->mba_phys, qproc->mba_size);

Shouldn't we move them even further down?

Same BUG_ON() as above.

>  assert_reset:
>  	reset_control_assert(qproc->mss_restart);
>  disable_vdd:

Regards,
Bjorn

Re: [RESEND: PATCH v4 3/4] remoteproc: qcom: Make secure world call for mem ownership switch

[Dwivedi, Avaneesh Kumar (avani)]

On 5/26/2017 7:46 AM, Bjorn Andersson wrote:
> On Tue 16 May 11:02 PDT 2017, Avaneesh Kumar Dwivedi wrote:
>
>> +static int q6v5_assign_mem_to_subsys(struct q6v5 *qproc,
>> +		phys_addr_t addr, size_t size)
>> +{
>> +	struct qcom_scm_destVmPerm next[] = {{ QCOM_SCM_VMID_MSS_MSA,
>> +		(QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)} };
> struct qcom_scm_destVmPerm next = {
> 	.vmid = QCOM_SCM_VMID_MSS_MSA,
> 	.perm = QCOM_SCM_PERM_RW
> };
OK.
>
>> +	int ret;
>> +
>> +	size = ALIGN(size, SZ_4K);
> I believe it will be cleaner if you just put this alignment directly in
> the function call below.
OK.
>
>> +	if (!qproc->need_mem_protection)
>> +		return 0;
> Put a blank line here, to give separation between the sections of the
> function.
OK.
>
>> +	ret = qcom_scm_assign_mem(addr, size,
>> +		BIT(QCOM_SCM_VMID_HLOS), next, sizeof(next));
> 	qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K),
> 			    BIT(QCOM_SCM_VMID_HLOS), &next, 1);
OK
>
>> +	if (ret)
>> +		pr_err("%s: Failed to assign memory access, ret = %d\n",
>> +			__func__, ret);
> There's no point in printing an error here and in the calling function,
> but as it makes sense to have the error message to include which memory
> region we operated on (mba vs mpss) I think you should remove the print
> here and keep it in the callers.
OK
>
> So just return qcom-scm_assign_mem().
OK
>
>> +	return ret;
>> +}
>> +
>> +static int q6v5_assign_mem_to_linux(struct q6v5 *qproc,
>> +		phys_addr_t addr, size_t size)
>> +{
>> +	struct qcom_scm_destVmPerm next[] = { { QCOM_SCM_VMID_HLOS,
>> +		(QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_EXEC)}
>> +		};
> struct qcom_scm_destVmPerm next = {
> 	.vmid = QCOM_SCM_VMID_HLOS,
> 	.perm = QCOM_SCM_PERM_RWX,
> };
>
> (And add RWX to the list of defines in patch 1)
OK
>
> [..]
>> @@ -471,6 +517,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>>   		dev_err(qproc->dev,
>>   			"metadata authentication failed: %d\n", ret);
>>   
>> +	/* Metadata authentication done, remove modem access */
>> +	ret = q6v5_assign_mem_to_linux(qproc, phys, fw->size);
>> +	if (ret)
>> +		dev_err(qproc->dev,
>> +			"Failed to reclaim metadata memory, ret - %d\n", ret);
> If this assignment fails (for any reason) we can't return the memory to
> the free pool in Linux, because at some point in the future these pages
> will be allocated to someone else resulting in a memory access violation
> scenario that will be just terrible to debug.
>
> I do however not have a better idea at the moment than just leaking the
> memory.
Then shall we BUG_ON here itself?
>
>>   	dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
>>   
>>   	return ret < 0 ? ret : 0;
> [..]
>> @@ -656,16 +719,21 @@ static int q6v5_start(struct rproc *rproc)
>>   
>>   	ret = q6v5_mpss_load(qproc);
>>   	if (ret)
>> -		goto halt_axi_ports;
>> +		goto reclaim_mem;
> This doesn't allow us to distinguish between failures before or after
> the memory assignment in mpss_load(), so although you're duplicating the
> reclaim code, mpss_load() must be responsible of restoring the state on
> failure.
OK, will move one of reclaim path in mpss_load it self.
>
> The timeout below still has to reclaim the memory.
Yes that any way will do.
>
>>   
>>   	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;
>>   	}
>>   
>> +	ret = q6v5_assign_mem_to_linux(qproc,
>> +		qproc->mba_phys, qproc->mba_size);
>> +	if (ret)
>> +		dev_err(qproc->dev,
>> +			"Failed to reclaim mba memory, ret - %d\n", ret);
> I think it's okay for symmetrical purposes to keep the memory assigned
> to the remote until we call stop().
Actually MBA image is transferred into internal memory of q6 and does 
not run from DDR
that is why it is OK to release it here. let me know if you dont want to 
do that.
>
> Although this shows that we should be able to release the mba allocation
> here.
Yes that is true.
>
>>   	qproc->running = true;
>>   
>>   	q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
>> @@ -675,12 +743,19 @@ static int q6v5_start(struct rproc *rproc)
>>   
>>   	return 0;
>>   
>> +reclaim_mem:
>> +	assign_mem_result =
>> +		q6v5_assign_mem_to_linux(qproc,
>> +		qproc->mpss_phys, qproc->mpss_size);
> If this fails we're screwed. We have no way to fail in a way that will
> not free the memory at any later point in time.
>
> So I do believe you should have a BUG_ON(assign_mem_result); here. With
> a clear comment in the lines of:
>
> /*
>   * Failed to reclaim memory, any future access to these pages will cause
>   * security violations and a seemingly random crash
>   */
Yes this is very good suggestion thanks.
>>   halt_axi_ports:
>>   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
>>   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
>>   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>>   	q6v5_clk_disable(qproc->dev, qproc->active_clks,
>>   			 qproc->active_clk_count);
>> +	assign_mem_result =
>> +		q6v5_assign_mem_to_linux(qproc,
>> +		qproc->mba_phys, qproc->mba_size);
> Shouldn't we move them even further down?
There does not seem any restriction w.r.t. keeping it here.
Do you think any side effect ?
>
> Same BUG_ON() as above.
Yes sure.
>
>>   assert_reset:
>>   	reset_control_assert(qproc->mss_restart);
>>   disable_vdd:
> Regards,
> Bjorn

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

Re: [RESEND: PATCH v4 3/4] remoteproc: qcom: Make secure world call for mem ownership switch

[Bjorn Andersson]

On Fri 26 May 06:19 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote:

> 
> 
> On 5/26/2017 7:46 AM, Bjorn Andersson wrote:
> > On Tue 16 May 11:02 PDT 2017, Avaneesh Kumar Dwivedi wrote:
> > > @@ -471,6 +517,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
> > >   		dev_err(qproc->dev,
> > >   			"metadata authentication failed: %d\n", ret);
> > > +	/* Metadata authentication done, remove modem access */
> > > +	ret = q6v5_assign_mem_to_linux(qproc, phys, fw->size);
> > > +	if (ret)
> > > +		dev_err(qproc->dev,
> > > +			"Failed to reclaim metadata memory, ret - %d\n", ret);
> > If this assignment fails (for any reason) we can't return the memory to
> > the free pool in Linux, because at some point in the future these pages
> > will be allocated to someone else resulting in a memory access violation
> > scenario that will be just terrible to debug.
> > 
> > I do however not have a better idea at the moment than just leaking the
> > memory.
> Then shall we BUG_ON here itself?

Here we have the chance to "handle" the problem (by leaking the memory),
so it's not a catastrophic error. But perhaps better to replace the
dev_err() with a WARN(ret, "failed to reclaim memory\n"); that way this
issue will stand out in the log.

[..]
> > > @@ -656,16 +719,21 @@ static int q6v5_start(struct rproc *rproc)
[..]
> > >   	}
> > > +	ret = q6v5_assign_mem_to_linux(qproc,
> > > +		qproc->mba_phys, qproc->mba_size);
> > > +	if (ret)
> > > +		dev_err(qproc->dev,
> > > +			"Failed to reclaim mba memory, ret - %d\n", ret);
> > I think it's okay for symmetrical purposes to keep the memory assigned
> > to the remote until we call stop().
> Actually MBA image is transferred into internal memory of q6 and does not
> run from DDR
> that is why it is OK to release it here. let me know if you dont want to do
> that.

Leave it here, but please add a comment above this snippet saying
something like:

/*
 * The MBA is transferred to internal memory of the Q6 and no longer
 * need access.
 */

[..]
> > > +	assign_mem_result =
> > > +		q6v5_assign_mem_to_linux(qproc,
> > > +		qproc->mba_phys, qproc->mba_size);
> > Shouldn't we move them even further down?
> There does not seem any restriction w.r.t. keeping it here.
> Do you think any side effect ?

No, I'm fine with this if you say that the MPSS is no longer executing
anything at this point.

Thanks,
Bjorn

[RESEND: PATCH v4 4/4] remoteproc: qcom: Add support for mss boot on msm8996

[Avaneesh Kumar Dwivedi]

This patch add support for mss boot on msm8996.
Major changes include initializing mss rproc for
msm8996, making appropriate change for executing
mss reset sequence etc.

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

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 92347fe..f9dfb6c 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -9,8 +9,8 @@ on the Qualcomm Hexagon core.
 	Definition: must be one of:
 		    "qcom,q6v5-pil",
 		    "qcom,msm8916-mss-pil",
-		    "qcom,msm8974-mss-pil"
-
+		    "qcom,msm8974-mss-pil",
+		"qcom,msm8996-mss-pil"
 - reg:
 	Usage: required
 	Value type: <prop-encoded-array>
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 57a4cfec..97da382 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -32,6 +32,7 @@
 #include <linux/soc/qcom/mdt_loader.h>
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/smem_state.h>
+#include <linux/iopoll.h>
 
 #include "remoteproc_internal.h"
 #include "qcom_common.h"
@@ -64,6 +65,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
@@ -92,6 +95,14 @@
 #define QDSS_BHS_ON			BIT(21)
 #define QDSS_LDO_BYP			BIT(22)
 
+/* QDSP6v56 parameters */
+#define QDSP6v56_LDO_BYP		BIT(25)
+#define QDSP6v56_BHS_ON		BIT(24)
+#define QDSP6v56_CLAMP_WL		BIT(21)
+#define QDSP6v56_CLAMP_QMC_MEM		BIT(22)
+#define HALT_CHECK_MAX_LOOPS		200
+#define QDSP6SS_XO_CBCR		0x0038
+#define QDSP6SS_ACC_OVERRIDE_VAL		0x20
 struct reg_info {
 	struct regulator *reg;
 	int uV;
@@ -110,6 +121,7 @@ struct rproc_hexagon_res {
 	struct qcom_mss_reg_res *active_supply;
 	char **proxy_clk_names;
 	char **active_clk_names;
+	int version;
 	bool need_mem_protection;
 };
 
@@ -155,6 +167,13 @@ struct q6v5 {
 	bool need_mem_protection;
 
 	struct qcom_rproc_subdev smd_subdev;
+	int version;
+};
+
+enum {
+	MSS_MSM8916,
+	MSS_MSM8974,
+	MSS_MSM8996,
 };
 
 static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
@@ -391,33 +410,97 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 {
 	u32 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);
+	if (qproc->version == MSS_MSM8996) {
+		/* Override the ACC value if required */
+		writel(QDSP6SS_ACC_OVERRIDE_VAL,
+			qproc->reg_base + QDSP6SS_STRAP_ACC);
 
-	/*
-	 * 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);
+		/* 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);
@@ -775,6 +858,7 @@ static int q6v5_stop(struct rproc *rproc)
 {
 	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
 	int ret;
+	u32 val;
 
 	qproc->running = false;
 
@@ -792,6 +876,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);
+	}
 	ret = q6v5_assign_mem_to_linux(qproc,
 			qproc->mpss_phys, qproc->mpss_size);
 	reset_control_assert(qproc->mss_restart);
@@ -1091,6 +1184,7 @@ static int q6v5_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
+	qproc->version = desc->version;
 	qproc->need_mem_protection = desc->need_mem_protection;
 	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
 	if (ret < 0)
@@ -1140,6 +1234,34 @@ 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[]) {
+			{}
+	},
+	.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
+	},
+	.need_mem_protection = true,
+	.version = MSS_MSM8996,
+};
+
 static const struct rproc_hexagon_res msm8916_mss = {
 	.hexagon_mba_image = "mba.mbn",
 	.proxy_supply = (struct qcom_mss_reg_res[]) {
@@ -1168,6 +1290,7 @@ static int q6v5_remove(struct platform_device *pdev)
 		NULL
 	},
 	.need_mem_protection = false,
+	.version = MSS_MSM8916,
 };
 
 static const struct rproc_hexagon_res msm8974_mss = {
@@ -1206,12 +1329,14 @@ static int q6v5_remove(struct platform_device *pdev)
 		NULL
 	},
 	.need_mem_protection = false,
+	.version = MSS_MSM8974,
 };
 
 static const struct of_device_id q6v5_of_match[] = {
 	{ .compatible = "qcom,q6v5-pil", .data = &msm8916_mss},
 	{ .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss},
 	{ .compatible = "qcom,msm8974-mss-pil", .data = &msm8974_mss},
+	{ .compatible = "qcom,msm8996-mss-pil", .data = &msm8996_mss},
 	{ },
 };
 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: [RESEND: PATCH v4 4/4] remoteproc: qcom: Add support for mss boot on msm8996

[Bjorn Andersson]

On Tue 16 May 11:02 PDT 2017, Avaneesh Kumar Dwivedi wrote:

> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 92347fe..f9dfb6c 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> @@ -9,8 +9,8 @@ on the Qualcomm Hexagon core.
>  	Definition: must be one of:
>  		    "qcom,q6v5-pil",
>  		    "qcom,msm8916-mss-pil",
> -		    "qcom,msm8974-mss-pil"
> -
> +		    "qcom,msm8974-mss-pil",
> +		"qcom,msm8996-mss-pil"

Indentation please.

>  - reg:
>  	Usage: required
>  	Value type: <prop-encoded-array>
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
[..]
> +/* 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

Please keep the blank line between the defines and the struct
definition.

>  struct reg_info {
>  	struct regulator *reg;
>  	int uV;
[..]
>  
> +static const struct rproc_hexagon_res msm8996_mss = {
> +	.hexagon_mba_image = "mba.mbn",
> +	.proxy_supply = (struct qcom_mss_reg_res[]) {
> +			{}
> +	},
> +	.active_supply = (struct qcom_mss_reg_res[]) {
> +			{},
> +			{}
> +	},

It's possible to just leave .proxy_supply and .active_supply out (i.e.
leaving them as NULL), as I made q6v5_regulator_init() handle this
gracefully a while back.

Regards,
Bjorn

Re: [RESEND: PATCH v4 4/4] remoteproc: qcom: Add support for mss boot on msm8996

[Dwivedi, Avaneesh Kumar (avani)]

On 5/26/2017 11:39 AM, Bjorn Andersson wrote:
> On Tue 16 May 11:02 PDT 2017, Avaneesh Kumar Dwivedi wrote:
>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> index 92347fe..f9dfb6c 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> @@ -9,8 +9,8 @@ on the Qualcomm Hexagon core.
>>   	Definition: must be one of:
>>   		    "qcom,q6v5-pil",
>>   		    "qcom,msm8916-mss-pil",
>> -		    "qcom,msm8974-mss-pil"
>> -
>> +		    "qcom,msm8974-mss-pil",
>> +		"qcom,msm8996-mss-pil"
> Indentation please.
OK.
>
>>   - reg:
>>   	Usage: required
>>   	Value type: <prop-encoded-array>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> [..]
>> +/* 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
> Please keep the blank line between the defines and the struct
> definition.
OK.
>
>>   struct reg_info {
>>   	struct regulator *reg;
>>   	int uV;
> [..]
>>   
>> +static const struct rproc_hexagon_res msm8996_mss = {
>> +	.hexagon_mba_image = "mba.mbn",
>> +	.proxy_supply = (struct qcom_mss_reg_res[]) {
>> +			{}
>> +	},
>> +	.active_supply = (struct qcom_mss_reg_res[]) {
>> +			{},
>> +			{}
>> +	},
> It's possible to just leave .proxy_supply and .active_supply out (i.e.
> leaving them as NULL), as I made q6v5_regulator_init() handle this
> gracefully a while back.
OK.
>
> Regards,
> Bjorn

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