Re: [PATCH v7 4/4] remoteproc: qcom: Add support for mss remoteproc on msm8996

[Dwivedi, Avaneesh Kumar (avani)]

On 10/12/2017 11:50 PM, Bjorn Andersson wrote:

On Fri 21 Jul 03:49 PDT 2017, Avaneesh Kumar Dwivedi wrote:


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 
---
  .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   1 +
  drivers/remoteproc/qcom_q6v5_pil.c | 164 ++---
  2 files changed, 141 insertions(+), 24 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt 
b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 92347fe..87a8e51 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -11,6 +11,7 @@ on the Qualcomm Hexagon core.
"qcom,msm8916-mss-pil",
"qcom,msm8974-mss-pil"
  
+		"qcom,msm8996-mss-pil"

I like the empty line, but it should be after the list, not in the
middle.


  - reg:

Please fix this and add my Acked-by
Sure will do, just want to ask, when i am sending updated patches, 
should i sent all the 4 patches in series or should skip those which are 
acked by you.


Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

Re: [PATCH v7 4/4] remoteproc: qcom: Add support for mss remoteproc on msm8996

[Dwivedi, Avaneesh Kumar (avani)]

On 10/12/2017 11:50 PM, Bjorn Andersson wrote:

On Fri 21 Jul 03:49 PDT 2017, Avaneesh Kumar Dwivedi wrote:


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 
---
  .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   1 +
  drivers/remoteproc/qcom_q6v5_pil.c | 164 ++---
  2 files changed, 141 insertions(+), 24 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt 
b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 92347fe..87a8e51 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -11,6 +11,7 @@ on the Qualcomm Hexagon core.
"qcom,msm8916-mss-pil",
"qcom,msm8974-mss-pil"
  
+		"qcom,msm8996-mss-pil"

I like the empty line, but it should be after the list, not in the
middle.


  - reg:

Please fix this and add my Acked-by
Sure will do, just want to ask, when i am sending updated patches, 
should i sent all the 4 patches in series or should skip those which are 
acked by you.


Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

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

[Dwivedi, Avaneesh Kumar (avani)]

On 10/12/2017 11:48 PM, Bjorn Andersson wrote:

On Fri 21 Jul 03:49 PDT 2017, Avaneesh Kumar Dwivedi wrote:


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 
---
  drivers/remoteproc/qcom_q6v5_pil.c | 90 --
  1 file changed, 87 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
b/drivers/remoteproc/qcom_q6v5_pil.c

[..]

+static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, int *current_perm,
+  bool remote_owner, phys_addr_t addr,
+  size_t size)
+{
+   struct qcom_scm_vmperm next;
+   int ret;
+
+   if (!qproc->need_mem_protection)
+   return 0;
+   if (remote_owner && *current_perm == BIT(QCOM_SCM_VMID_MSS_MSA))
+   return 0;
+   if (!remote_owner && *current_perm == BIT(QCOM_SCM_VMID_HLOS))
+   return 0;
+
+   next.vmid = remote_owner ? QCOM_SCM_VMID_MSS_MSA : QCOM_SCM_VMID_HLOS;
+   next.perm = remote_owner ? QCOM_SCM_PERM_RW : QCOM_SCM_PERM_RWX;
+
+   ret = qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K),
+ *current_perm, , 1);
+   if (ret < 0) {
+   pr_err("Failed to assign memory access in range %p to %p to %s ret = 
%d\n",
+  (void *)addr, (void *)(addr + size),
+  remote_owner ? "mss" : "hlos", ret);

qcom_scm_assign_mem() also prints an error when this happens, there's no
need for that. I'm happy with you dropping the print from
qcom_scm_assign_mem() and keeping this one.


+   return ret;
+   }
+
+   *current_perm = ret;
+   return 0;
+}
+
  static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
  {
struct q6v5 *qproc = rproc->priv;
@@ -450,6 +484,8 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const 
struct firmware *fw)
  {
unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
dma_addr_t phys;
+   int mdata_perm;
+   int xferop_ret;
void *ptr;
int ret;
  
@@ -461,6 +497,12 @@ 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 */

+   mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
+   ret = q6v5_xfer_mem_ownership(qproc, _perm,
+ 1, phys, fw->size);

Please use "true" instead of "1".

OK Sure, will do it.

+   if (ret)
+   return -EAGAIN;

Please keep a empty line here, to keep the "paragraphs" or "chunks" of
code split up.

OK.



writel(phys, qproc->rmb_base + RMB_PMI_META_DATA_REG);
writel(RMB_CMD_META_DATA_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
  

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

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

[Dwivedi, Avaneesh Kumar (avani)]

On 10/12/2017 11:48 PM, Bjorn Andersson wrote:

On Fri 21 Jul 03:49 PDT 2017, Avaneesh Kumar Dwivedi wrote:


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 
---
  drivers/remoteproc/qcom_q6v5_pil.c | 90 --
  1 file changed, 87 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
b/drivers/remoteproc/qcom_q6v5_pil.c

[..]

+static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, int *current_perm,
+  bool remote_owner, phys_addr_t addr,
+  size_t size)
+{
+   struct qcom_scm_vmperm next;
+   int ret;
+
+   if (!qproc->need_mem_protection)
+   return 0;
+   if (remote_owner && *current_perm == BIT(QCOM_SCM_VMID_MSS_MSA))
+   return 0;
+   if (!remote_owner && *current_perm == BIT(QCOM_SCM_VMID_HLOS))
+   return 0;
+
+   next.vmid = remote_owner ? QCOM_SCM_VMID_MSS_MSA : QCOM_SCM_VMID_HLOS;
+   next.perm = remote_owner ? QCOM_SCM_PERM_RW : QCOM_SCM_PERM_RWX;
+
+   ret = qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K),
+ *current_perm, , 1);
+   if (ret < 0) {
+   pr_err("Failed to assign memory access in range %p to %p to %s ret = 
%d\n",
+  (void *)addr, (void *)(addr + size),
+  remote_owner ? "mss" : "hlos", ret);

qcom_scm_assign_mem() also prints an error when this happens, there's no
need for that. I'm happy with you dropping the print from
qcom_scm_assign_mem() and keeping this one.


+   return ret;
+   }
+
+   *current_perm = ret;
+   return 0;
+}
+
  static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
  {
struct q6v5 *qproc = rproc->priv;
@@ -450,6 +484,8 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const 
struct firmware *fw)
  {
unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
dma_addr_t phys;
+   int mdata_perm;
+   int xferop_ret;
void *ptr;
int ret;
  
@@ -461,6 +497,12 @@ 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 */

+   mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
+   ret = q6v5_xfer_mem_ownership(qproc, _perm,
+ 1, phys, fw->size);

Please use "true" instead of "1".

OK Sure, will do it.

+   if (ret)
+   return -EAGAIN;

Please keep a empty line here, to keep the "paragraphs" or "chunks" of
code split up.

OK.



writel(phys, qproc->rmb_base + RMB_PMI_META_DATA_REG);
writel(RMB_CMD_META_DATA_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
  

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

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

[Dwivedi, Avaneesh Kumar (avani)]

On 10/12/2017 11:41 PM, Bjorn Andersson wrote:

On Fri 21 Jul 03:49 PDT 2017, Avaneesh Kumar Dwivedi wrote:


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


As I reported to you a while back I finally managed to use this to get
the modem on db820c up and running (with "all" IPCROUTER services
showing up). So let's try to resurrect and get this merged!


In addition I've successfully used this patch in extending the rmtfs
shared memory driver to allow setting up the ownership of that memory.

[..]

Thanks for reviving this patch series Bjorn!

Will try to take it closure this time.


diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index bb16510..009a42d 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -40,6 +40,18 @@ struct qcom_scm {
struct reset_controller_dev reset;
  };
  
+struct qcom_scm_current_perm_info {

+   __le32 vmid;
+   __le32 perm;
+   __le64 ctx;
+   __le32 ctx_size;
+};

I learned the hard way that this struct is supposed to be 24 bytes, so
please add a __le32 padding; at the end of this.

[..]

OK.

+int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcvm,
+   struct qcom_scm_vmperm *newvm, int dest_cnt)
+{

It turns out that the standard way of calling this (shown by the
remoteproc and rmtfs-memory driver implementations) is:

ret = qcom_scm_assign_mem(mem, len, curr_vm, perms, sizeof(perms));
if (ret < 0)
fail();
curr_vm = ret;

I therefor suggest that we make one more adjustment to the prototype in
the form of taking srcvm as a pointer. And as this is now a bitmask it
should be made an unsigned int. I.e.

int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, unsigned int srcvm,
struct qcom_scm_vmperm *newvm, int dest_cnt)


+   struct qcom_scm_current_perm_info *destvm;
+   struct qcom_scm_mem_map_info *mem;
+   phys_addr_t memory_phys;
+   phys_addr_t dest_phys;
+   phys_addr_t src_phys;
+   size_t mem_all_sz;
+   size_t memory_sz;
+   size_t dest_sz;
+   size_t src_sz;
+   int next_vm;
+   __le32 *src;
+   void *ptr;
+   int ret;
+   int len;
+   int i;
+
+   src_sz = hweight_long(srcvm) * sizeof(*src);
+   memory_sz = sizeof(*mem);
+   dest_sz = dest_cnt * sizeof(*destvm);
+   mem_all_sz = src_sz + memory_sz + dest_sz;

ALIGN(x + y + z, 64) <= ALIGN(x, 64) + ALIGN(y, 64) + ALIGN(z, 64)

So please replace this with:

ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(memory_sz, SZ_64) +
 ALIGN(dest_sz, SZ_64);

(renaming the variable to ptr_sz saves you some line wraps as well)
Ok, Will try to incorporate your idea, and if any concern will revert 
back soon.

+
+   ptr = dma_alloc_coherent(__scm->dev, ALIGN(mem_all_sz, SZ_64),
+_phys, GFP_KERNEL);
+
+   if (!ptr)
+   return -ENOMEM;
+
+   /* Fill source vmid detail */
+   src = ptr;
+   len = hweight_long(srcvm);
+   for (i = 0; i < len; i++) {
+   src[i] = cpu_to_le32(ffs(srcvm) - 1);
+   srcvm ^= 1 << (ffs(srcvm) - 1);
+   }
+
+   /* Fill details of mem buff to map */
+   mem = ptr + ALIGN(src_sz, SZ_64);
+   memory_phys = src_phys + ALIGN(src_sz, SZ_64);
+   mem[0].mem_addr = cpu_to_le64(mem_addr);
+   mem[0].mem_size = cpu_to_le64(mem_sz);
+
+   next_vm = 0;
+   /* Fill details of next vmid detail */
+   destvm = ptr + ALIGN(memory_sz, SZ_64) + ALIGN(src_sz, SZ_64);
+   dest_phys = memory_phys + ALIGN(memory_sz, SZ_64);

For clarity it would be nice if you keep the math for virtual and
physical addresses the same; so add another variable "ptr_phys" and
replace this with:
dest_phys = ptr_phys + ALIGN(src_sz, 64) + ALIGN(memory_sz, 64);

OK.



+   for (i = 0; i < dest_cnt; i++) {
+   destvm[i].vmid = cpu_to_le32(newvm[i].vmid);
+   destvm[i].perm = cpu_to_le32(newvm[i].perm);
+   destvm[i].ctx = 0;
+   destvm[i].ctx_size = 0;
+   next_vm |= BIT(newvm[i].vmid);
+   }
+
+   ret = __qcom_scm_assign_mem(__scm->dev, memory_phys, memory_sz,
+   src_phys, src_sz, dest_phys, dest_sz);
+   dma_free_coherent(__scm->dev, ALIGN(mem_all_sz, SZ_64),
+ ptr, src_phys);
+   if (ret != 0) {
+   dev_err(__scm->dev,
+   "Assign memory protection call failed %d.\n", ret);
+   return -EINVAL;
+   } else {
+   return next_vm;
+   }

Replace this with:

if (ret) {
dev_err(__scm->dev,
  

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

[Dwivedi, Avaneesh Kumar (avani)]

On 10/12/2017 11:41 PM, Bjorn Andersson wrote:

On Fri 21 Jul 03:49 PDT 2017, Avaneesh Kumar Dwivedi wrote:


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


As I reported to you a while back I finally managed to use this to get
the modem on db820c up and running (with "all" IPCROUTER services
showing up). So let's try to resurrect and get this merged!


In addition I've successfully used this patch in extending the rmtfs
shared memory driver to allow setting up the ownership of that memory.

[..]

Thanks for reviving this patch series Bjorn!

Will try to take it closure this time.


diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index bb16510..009a42d 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -40,6 +40,18 @@ struct qcom_scm {
struct reset_controller_dev reset;
  };
  
+struct qcom_scm_current_perm_info {

+   __le32 vmid;
+   __le32 perm;
+   __le64 ctx;
+   __le32 ctx_size;
+};

I learned the hard way that this struct is supposed to be 24 bytes, so
please add a __le32 padding; at the end of this.

[..]

OK.

+int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcvm,
+   struct qcom_scm_vmperm *newvm, int dest_cnt)
+{

It turns out that the standard way of calling this (shown by the
remoteproc and rmtfs-memory driver implementations) is:

ret = qcom_scm_assign_mem(mem, len, curr_vm, perms, sizeof(perms));
if (ret < 0)
fail();
curr_vm = ret;

I therefor suggest that we make one more adjustment to the prototype in
the form of taking srcvm as a pointer. And as this is now a bitmask it
should be made an unsigned int. I.e.

int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, unsigned int srcvm,
struct qcom_scm_vmperm *newvm, int dest_cnt)


+   struct qcom_scm_current_perm_info *destvm;
+   struct qcom_scm_mem_map_info *mem;
+   phys_addr_t memory_phys;
+   phys_addr_t dest_phys;
+   phys_addr_t src_phys;
+   size_t mem_all_sz;
+   size_t memory_sz;
+   size_t dest_sz;
+   size_t src_sz;
+   int next_vm;
+   __le32 *src;
+   void *ptr;
+   int ret;
+   int len;
+   int i;
+
+   src_sz = hweight_long(srcvm) * sizeof(*src);
+   memory_sz = sizeof(*mem);
+   dest_sz = dest_cnt * sizeof(*destvm);
+   mem_all_sz = src_sz + memory_sz + dest_sz;

ALIGN(x + y + z, 64) <= ALIGN(x, 64) + ALIGN(y, 64) + ALIGN(z, 64)

So please replace this with:

ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(memory_sz, SZ_64) +
 ALIGN(dest_sz, SZ_64);

(renaming the variable to ptr_sz saves you some line wraps as well)
Ok, Will try to incorporate your idea, and if any concern will revert 
back soon.

+
+   ptr = dma_alloc_coherent(__scm->dev, ALIGN(mem_all_sz, SZ_64),
+_phys, GFP_KERNEL);
+
+   if (!ptr)
+   return -ENOMEM;
+
+   /* Fill source vmid detail */
+   src = ptr;
+   len = hweight_long(srcvm);
+   for (i = 0; i < len; i++) {
+   src[i] = cpu_to_le32(ffs(srcvm) - 1);
+   srcvm ^= 1 << (ffs(srcvm) - 1);
+   }
+
+   /* Fill details of mem buff to map */
+   mem = ptr + ALIGN(src_sz, SZ_64);
+   memory_phys = src_phys + ALIGN(src_sz, SZ_64);
+   mem[0].mem_addr = cpu_to_le64(mem_addr);
+   mem[0].mem_size = cpu_to_le64(mem_sz);
+
+   next_vm = 0;
+   /* Fill details of next vmid detail */
+   destvm = ptr + ALIGN(memory_sz, SZ_64) + ALIGN(src_sz, SZ_64);
+   dest_phys = memory_phys + ALIGN(memory_sz, SZ_64);

For clarity it would be nice if you keep the math for virtual and
physical addresses the same; so add another variable "ptr_phys" and
replace this with:
dest_phys = ptr_phys + ALIGN(src_sz, 64) + ALIGN(memory_sz, 64);

OK.



+   for (i = 0; i < dest_cnt; i++) {
+   destvm[i].vmid = cpu_to_le32(newvm[i].vmid);
+   destvm[i].perm = cpu_to_le32(newvm[i].perm);
+   destvm[i].ctx = 0;
+   destvm[i].ctx_size = 0;
+   next_vm |= BIT(newvm[i].vmid);
+   }
+
+   ret = __qcom_scm_assign_mem(__scm->dev, memory_phys, memory_sz,
+   src_phys, src_sz, dest_phys, dest_sz);
+   dma_free_coherent(__scm->dev, ALIGN(mem_all_sz, SZ_64),
+ ptr, src_phys);
+   if (ret != 0) {
+   dev_err(__scm->dev,
+   "Assign memory protection call failed %d.\n", ret);
+   return -EINVAL;
+   } else {
+   return next_vm;
+   }

Replace this with:

if (ret) {
dev_err(__scm->dev,
  

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

[Dwivedi, Avaneesh Kumar (avani)]

On 7/13/2017 11:24 AM, Stephen Boyd wrote:

On 07/12, Dwivedi, Avaneesh Kumar (avani) wrote:


On 7/8/2017 4:19 AM, Stephen Boyd wrote:

On 06/22, Avaneesh Kumar Dwivedi wrote:

diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 6e6d561..cdfe986 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -292,6 +304,86 @@ int qcom_scm_pas_shutdown(u32 peripheral)
  }
  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
+/**
+ * qcom_scm_assign_mem() - Make a secure call to reassign memory ownership
+ *
+ * @mem_addr: mem region whose ownership need to be reassigned
+ * @mem_sz:   size of the region.
+ * @srcvm:vmid for current set of owners, each set bit in
+ *flag indicate a unique owner
+ * @newvm:array having new owners and corrsponding permission
+ *flags
+ * @dest_cnt: number of owners in next set.
+ * Return next set of owners on success.
+ */
+int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcvm,
+   struct qcom_scm_vmperm *newvm, int dest_cnt)
+{
+   unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;

Why do we need this? Just curious if we can drop this.

The force contiguous flag is required with dma_alloc_attrs() api to
allocate memory from physically contiguous zone.
I am not sure, are you saying that api will work without the
attribute or you mean i shall use some api which does not take
explicit attribute?

Does physically contiguous zone mean some CMA carveout? I wasn't
aware of a carveout for scm devices. I'm still not following the
reasoning here.
the memory will be allocated from common carveout, there is no scm 
device specific

carveout. i will use dma_alloc_coherent() and will drop off this flag.

we need physical contigious zone to fill and pass scm call parameters to TZ.


I'm saying that I don't understand why we need this flag. It
feels like this sort of constraint would apply all over the scm
driver if it was true, hence the confusion.


+
+   ret = __qcom_scm_assign_mem(__scm->dev, memory_phys,
+   memory_sz, src_phys, src_sz, dest_phys, dest_sz);
+   dma_free_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
+   ptr, src_phys, dma_attrs);
+   if (ret == 0)
+   return next_vm;
+   else if (ret > 0)
+   return -ret;

This still confuses me. Do we really just pass whatever the
firmware tells us the error code is up to the caller? Shouldn't
we be remapping the scm errors we receive to normal linux errnos?

because i do not know in advance what exactly will be the return
error code, moreover there are number of error codes which are
returned in case of failure
so if i have to return linux error code, i can not do one to one
mapping of error code and will have to return single error code for
all failure.
let me know your comments further on this.+ return ret;

Yes, returning -EINVAL all the time is fine if we can't remap the
error. In fact, we should probably do what we do downstream and
print out the error value returned from the firmware to the
kernel log and then return some sane errno up to the caller. That
way the few people who know what the error codes mean can tell us
why the scm call failed.

OK, will do same.
just last thing to ask, should i resend all 4 patches together again or 
only one patch in v7 version.

as chnage will be in only 1 out of 4 patches.




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

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

[Dwivedi, Avaneesh Kumar (avani)]

On 7/13/2017 11:24 AM, Stephen Boyd wrote:

On 07/12, Dwivedi, Avaneesh Kumar (avani) wrote:


On 7/8/2017 4:19 AM, Stephen Boyd wrote:

On 06/22, Avaneesh Kumar Dwivedi wrote:

diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 6e6d561..cdfe986 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -292,6 +304,86 @@ int qcom_scm_pas_shutdown(u32 peripheral)
  }
  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
+/**
+ * qcom_scm_assign_mem() - Make a secure call to reassign memory ownership
+ *
+ * @mem_addr: mem region whose ownership need to be reassigned
+ * @mem_sz:   size of the region.
+ * @srcvm:vmid for current set of owners, each set bit in
+ *flag indicate a unique owner
+ * @newvm:array having new owners and corrsponding permission
+ *flags
+ * @dest_cnt: number of owners in next set.
+ * Return next set of owners on success.
+ */
+int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcvm,
+   struct qcom_scm_vmperm *newvm, int dest_cnt)
+{
+   unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;

Why do we need this? Just curious if we can drop this.

The force contiguous flag is required with dma_alloc_attrs() api to
allocate memory from physically contiguous zone.
I am not sure, are you saying that api will work without the
attribute or you mean i shall use some api which does not take
explicit attribute?

Does physically contiguous zone mean some CMA carveout? I wasn't
aware of a carveout for scm devices. I'm still not following the
reasoning here.
the memory will be allocated from common carveout, there is no scm 
device specific

carveout. i will use dma_alloc_coherent() and will drop off this flag.

we need physical contigious zone to fill and pass scm call parameters to TZ.


I'm saying that I don't understand why we need this flag. It
feels like this sort of constraint would apply all over the scm
driver if it was true, hence the confusion.


+
+   ret = __qcom_scm_assign_mem(__scm->dev, memory_phys,
+   memory_sz, src_phys, src_sz, dest_phys, dest_sz);
+   dma_free_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
+   ptr, src_phys, dma_attrs);
+   if (ret == 0)
+   return next_vm;
+   else if (ret > 0)
+   return -ret;

This still confuses me. Do we really just pass whatever the
firmware tells us the error code is up to the caller? Shouldn't
we be remapping the scm errors we receive to normal linux errnos?

because i do not know in advance what exactly will be the return
error code, moreover there are number of error codes which are
returned in case of failure
so if i have to return linux error code, i can not do one to one
mapping of error code and will have to return single error code for
all failure.
let me know your comments further on this.+ return ret;

Yes, returning -EINVAL all the time is fine if we can't remap the
error. In fact, we should probably do what we do downstream and
print out the error value returned from the firmware to the
kernel log and then return some sane errno up to the caller. That
way the few people who know what the error codes mean can tell us
why the scm call failed.

OK, will do same.
just last thing to ask, should i resend all 4 patches together again or 
only one patch in v7 version.

as chnage will be in only 1 out of 4 patches.




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

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

[Dwivedi, Avaneesh Kumar (avani)]

On 7/11/2017 5:13 AM, Bjorn Andersson wrote:

On Thu 22 Jun 05:08 PDT 2017, Avaneesh Kumar Dwivedi wrote:


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

Signed-off-by: Avaneesh Kumar Dwivedi 
---
  drivers/firmware/qcom_scm-32.c |  6 +++
  drivers/firmware/qcom_scm-64.c | 27 +
  drivers/firmware/qcom_scm.c| 92 ++
  drivers/firmware/qcom_scm.h|  5 +++
  include/linux/qcom_scm.h   | 16 
  5 files changed, 146 insertions(+)

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index 93e3b96..a5e038d 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_region,
+ size_t mem_sz, phys_addr_t src, size_t src_sz,
+ phys_addr_t dest, size_t dest_sz)
+{
+   return -ENODEV;
+}
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 6e6d561..cdfe986 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_region,
+ size_t mem_sz, phys_addr_t src, size_t src_sz,
+ phys_addr_t dest, size_t dest_sz)
+{
+   int ret;
+   struct qcom_scm_desc desc = {0};
+   struct arm_smccc_res res;
+
+   desc.args[0] = mem_region;
+   desc.args[1] = mem_sz;
+   desc.args[2] = src;
+   desc.args[3] = src_sz;
+   desc.args[4] = dest;
+   desc.args[5] = dest_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,
+   , );

Please indent broken lines by the start parenthesis, throughout the
patch, this makes the code easier to read.  You can run checkpatch.pl
with the --strict flag to show a few other places below that has the
same issue.


Please clean these up together with the dma allocation and the return
value as pointed out by Stephen and I'm happy to pick the series up.


Sure, thank you, will do and send out by eod tomorrow.


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: [PATCH v6 1/4] firmware: scm: Add new SCM call API for switching memory ownership

[Dwivedi, Avaneesh Kumar (avani)]

On 7/11/2017 5:13 AM, Bjorn Andersson wrote:

On Thu 22 Jun 05:08 PDT 2017, Avaneesh Kumar Dwivedi wrote:


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

Signed-off-by: Avaneesh Kumar Dwivedi 
---
  drivers/firmware/qcom_scm-32.c |  6 +++
  drivers/firmware/qcom_scm-64.c | 27 +
  drivers/firmware/qcom_scm.c| 92 ++
  drivers/firmware/qcom_scm.h|  5 +++
  include/linux/qcom_scm.h   | 16 
  5 files changed, 146 insertions(+)

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index 93e3b96..a5e038d 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_region,
+ size_t mem_sz, phys_addr_t src, size_t src_sz,
+ phys_addr_t dest, size_t dest_sz)
+{
+   return -ENODEV;
+}
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 6e6d561..cdfe986 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_region,
+ size_t mem_sz, phys_addr_t src, size_t src_sz,
+ phys_addr_t dest, size_t dest_sz)
+{
+   int ret;
+   struct qcom_scm_desc desc = {0};
+   struct arm_smccc_res res;
+
+   desc.args[0] = mem_region;
+   desc.args[1] = mem_sz;
+   desc.args[2] = src;
+   desc.args[3] = src_sz;
+   desc.args[4] = dest;
+   desc.args[5] = dest_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,
+   , );

Please indent broken lines by the start parenthesis, throughout the
patch, this makes the code easier to read.  You can run checkpatch.pl
with the --strict flag to show a few other places below that has the
same issue.


Please clean these up together with the dma allocation and the return
value as pointed out by Stephen and I'm happy to pick the series up.


Sure, thank you, will do and send out by eod tomorrow.


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: [PATCH v6 1/4] firmware: scm: Add new SCM call API for switching memory ownership

[Dwivedi, Avaneesh Kumar (avani)]

On 7/8/2017 4:19 AM, Stephen Boyd wrote:

On 06/22, Avaneesh Kumar Dwivedi wrote:

diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 6e6d561..cdfe986 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -292,6 +304,86 @@ int qcom_scm_pas_shutdown(u32 peripheral)
  }
  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
  
+/**

+ * qcom_scm_assign_mem() - Make a secure call to reassign memory ownership
+ *
+ * @mem_addr: mem region whose ownership need to be reassigned
+ * @mem_sz:   size of the region.
+ * @srcvm:vmid for current set of owners, each set bit in
+ *flag indicate a unique owner
+ * @newvm:array having new owners and corrsponding permission
+ *flags
+ * @dest_cnt: number of owners in next set.
+ * Return next set of owners on success.
+ */
+int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcvm,
+   struct qcom_scm_vmperm *newvm, int dest_cnt)
+{
+   unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;

Why do we need this? Just curious if we can drop this.
The force contiguous flag is required with dma_alloc_attrs() api to 
allocate memory from physically contiguous zone.
I am not sure, are you saying that api will work without the attribute 
or you mean i shall use some api which does not take explicit attribute?



+   struct qcom_scm_current_perm_info *destvm;
+   struct qcom_scm_mem_map_info *mem;
+   phys_addr_t memory_phys;
+   phys_addr_t dest_phys;
+   phys_addr_t src_phys;
+   size_t mem_all_sz;
+   size_t memory_sz;
+   size_t dest_sz;
+   size_t src_sz;
+   int next_vm;
+   __le32 *src;
+   void *ptr;
+   int ret;
+   int len;
+   int i;
+
+   src_sz = hweight_long(srcvm) * sizeof(*src);
+   memory_sz = sizeof(*mem);
+   dest_sz = dest_cnt*sizeof(*destvm);
+   mem_all_sz = src_sz + memory_sz + dest_sz;
+
+   ptr = dma_alloc_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
+   _phys, GFP_KERNEL, dma_attrs);
+   if (!ptr)
+   return -ENOMEM;
+
+   /* Fill source vmid detail */
+   src = (__le32 *)ptr;

Cast is necessary?
i removed many type casting but few still lingering, will check and 
remove whatever unnecessary.



+   len = hweight_long(srcvm);
+   for (i = 0; i < len; i++) {
+   src[i] = cpu_to_le32(ffs(srcvm) - 1);
+   srcvm ^= 1 << (ffs(srcvm) - 1);
+   }
+
+   /* Fill details of mem buff to map */
+   mem = ptr + ALIGN(src_sz, SZ_64);
+   memory_phys = src_phys + ALIGN(src_sz, SZ_64);
+   mem[0].mem_addr = cpu_to_le64(mem_addr);
+   mem[0].mem_size = cpu_to_le64(mem_sz);
+
+   next_vm = 0;
+   /* Fill details of next vmid detail */
+   destvm = ptr + ALIGN(memory_sz, SZ_64) + ALIGN(src_sz, SZ_64);
+   dest_phys = memory_phys + ALIGN(memory_sz, SZ_64);
+   for (i = 0; i < dest_cnt; i++) {
+   destvm[i].vmid = cpu_to_le32(newvm[i].vmid);
+   destvm[i].perm = cpu_to_le32(newvm[i].perm);
+   destvm[i].ctx = 0;
+   destvm[i].ctx_size = 0;
+   next_vm |= BIT(newvm[i].vmid);
+   }
+
+   ret = __qcom_scm_assign_mem(__scm->dev, memory_phys,
+   memory_sz, src_phys, src_sz, dest_phys, dest_sz);
+   dma_free_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
+   ptr, src_phys, dma_attrs);
+   if (ret == 0)
+   return next_vm;
+   else if (ret > 0)
+   return -ret;

This still confuses me. Do we really just pass whatever the
firmware tells us the error code is up to the caller? Shouldn't
we be remapping the scm errors we receive to normal linux errnos?
because i do not know in advance what exactly will be the return error 
code, moreover there are number of error codes which are returned in 
case of failure
so if i have to return linux error code, i can not do one to one mapping 
of error code and will have to return single error code for all failure.

let me know your comments further on this.+ return ret;

+}
+EXPORT_SYMBOL(qcom_scm_assign_mem);
+
  static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
 unsigned long idx)
  {


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

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

[Dwivedi, Avaneesh Kumar (avani)]

On 7/8/2017 4:19 AM, Stephen Boyd wrote:

On 06/22, Avaneesh Kumar Dwivedi wrote:

diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 6e6d561..cdfe986 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -292,6 +304,86 @@ int qcom_scm_pas_shutdown(u32 peripheral)
  }
  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
  
+/**

+ * qcom_scm_assign_mem() - Make a secure call to reassign memory ownership
+ *
+ * @mem_addr: mem region whose ownership need to be reassigned
+ * @mem_sz:   size of the region.
+ * @srcvm:vmid for current set of owners, each set bit in
+ *flag indicate a unique owner
+ * @newvm:array having new owners and corrsponding permission
+ *flags
+ * @dest_cnt: number of owners in next set.
+ * Return next set of owners on success.
+ */
+int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcvm,
+   struct qcom_scm_vmperm *newvm, int dest_cnt)
+{
+   unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;

Why do we need this? Just curious if we can drop this.
The force contiguous flag is required with dma_alloc_attrs() api to 
allocate memory from physically contiguous zone.
I am not sure, are you saying that api will work without the attribute 
or you mean i shall use some api which does not take explicit attribute?



+   struct qcom_scm_current_perm_info *destvm;
+   struct qcom_scm_mem_map_info *mem;
+   phys_addr_t memory_phys;
+   phys_addr_t dest_phys;
+   phys_addr_t src_phys;
+   size_t mem_all_sz;
+   size_t memory_sz;
+   size_t dest_sz;
+   size_t src_sz;
+   int next_vm;
+   __le32 *src;
+   void *ptr;
+   int ret;
+   int len;
+   int i;
+
+   src_sz = hweight_long(srcvm) * sizeof(*src);
+   memory_sz = sizeof(*mem);
+   dest_sz = dest_cnt*sizeof(*destvm);
+   mem_all_sz = src_sz + memory_sz + dest_sz;
+
+   ptr = dma_alloc_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
+   _phys, GFP_KERNEL, dma_attrs);
+   if (!ptr)
+   return -ENOMEM;
+
+   /* Fill source vmid detail */
+   src = (__le32 *)ptr;

Cast is necessary?
i removed many type casting but few still lingering, will check and 
remove whatever unnecessary.



+   len = hweight_long(srcvm);
+   for (i = 0; i < len; i++) {
+   src[i] = cpu_to_le32(ffs(srcvm) - 1);
+   srcvm ^= 1 << (ffs(srcvm) - 1);
+   }
+
+   /* Fill details of mem buff to map */
+   mem = ptr + ALIGN(src_sz, SZ_64);
+   memory_phys = src_phys + ALIGN(src_sz, SZ_64);
+   mem[0].mem_addr = cpu_to_le64(mem_addr);
+   mem[0].mem_size = cpu_to_le64(mem_sz);
+
+   next_vm = 0;
+   /* Fill details of next vmid detail */
+   destvm = ptr + ALIGN(memory_sz, SZ_64) + ALIGN(src_sz, SZ_64);
+   dest_phys = memory_phys + ALIGN(memory_sz, SZ_64);
+   for (i = 0; i < dest_cnt; i++) {
+   destvm[i].vmid = cpu_to_le32(newvm[i].vmid);
+   destvm[i].perm = cpu_to_le32(newvm[i].perm);
+   destvm[i].ctx = 0;
+   destvm[i].ctx_size = 0;
+   next_vm |= BIT(newvm[i].vmid);
+   }
+
+   ret = __qcom_scm_assign_mem(__scm->dev, memory_phys,
+   memory_sz, src_phys, src_sz, dest_phys, dest_sz);
+   dma_free_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
+   ptr, src_phys, dma_attrs);
+   if (ret == 0)
+   return next_vm;
+   else if (ret > 0)
+   return -ret;

This still confuses me. Do we really just pass whatever the
firmware tells us the error code is up to the caller? Shouldn't
we be remapping the scm errors we receive to normal linux errnos?
because i do not know in advance what exactly will be the return error 
code, moreover there are number of error codes which are returned in 
case of failure
so if i have to return linux error code, i can not do one to one mapping 
of error code and will have to return single error code for all failure.

let me know your comments further on this.+ return ret;

+}
+EXPORT_SYMBOL(qcom_scm_assign_mem);
+
  static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
 unsigned long idx)
  {


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

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

[Dwivedi, Avaneesh Kumar (avani)]

On 6/2/2017 11:25 PM, Bjorn Andersson wrote:

On Thu 01 Jun 14:42 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote:


Hi Bjorn,

Thanks lot many for such a blazing fast response :)

regarding your points.

a- Do you mean caller's of q6v5_xfer_mem_ownership() should pass two
additional inputs i.e. *next_perm and *next_vmid


You have two cases; assign to HLOS and assign to MSS, so I imagine that
you pass a single indicator of which you want to assign. I.e. rather
than looking at what the current state is and flipping you pass the
conditional of that if statement as a parameter.

OK



 and that based on successful return of qcom_scm_assign () they should be
treated as *current_perm and *current_vmid


Instead of your index, you take a "int *curr_perms", which you use as
the current vmid list and you assign at the end of the function (like
you do today).

So to transfer the ownership to the MSS you would make a function call
like:

ret = q6v5_xfer_mem_ownership(qproc, >mpss_owner, ..., true);

mpss_owner would have to be initialize to HLOS before calling this, but
will always be holding the current value.
i am not finding compelling enough region to carry an input pointer to 
hold current ownership
specially when i am carrying a boolean flag to check whether next->vmid 
will be MSS or HLOS
I mean where am i going to use this current owner info in mss rproc 
driver, i am yet not getting enough reason.
while the local array did job of maintaining and flipping the ownership 
based on info if which image ownership transfer is being called.





by caller? if so caller will have to do flipping between MSS and HLOS,
isn't it?


As far as I can see each call to this driver is either a "transfer to
MSS" or "transfer to HLOS", so that shouldn't be a problem.
Yes this job will be done by bool flag, but again what is then use of 
carry pointers  mpss_owner, mba_owner.



Also code churning will increase this way, and also logging the way is
being handled now may require to change again.

or am i misunderstanding your proposal?


Could be that I'm missing something, if above doesn't make sense please
do let me know.
I can not say it does not make sense, probably something subtle i am 
missing to see.



Sorry for inconvenience, but if you could through little more light, that
will help.


There's no inconvenience, thanks for reaching out for clarifications on
my comments.

Thanks for such a nice gesture.

i feel that maintaining the local array for ownership switching looks, 
too raw way of doing something.
may be i can improve that, but  first i need to get understanding of 
your vision in suggesting above changes.


Regards,
Bjorn


-Avaneesh


On 6/2/2017 2:12 AM, Bjorn Andersson wrote:

On Thu 01 Jun 12:17 PDT 2017, Avaneesh Kumar Dwivedi wrote:


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.


Overall this looks good now, I have two minor notes that I want you to
fix up though.


diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct 
rproc *rproc,
return 
   }
+static int q6v5_xfer_mem_ownership(struct q6v5 *qproc,
+  int image, phys_addr_t addr,

Rather than relying on a static int to keep track of current permissions
pass a "int *current_perms", that you update on success.

Add int mba_perm and int mpss_perm to the struct q6v5 and initialize
them in probe and just keep the metadata_perm on the stack in
q6v5_mpss_init_image.


+  size_t size)
+{
+   static int current_owner[3][1] = {{BIT(QCOM_SCM_VMID_HLOS)},
+   {BIT(QCOM_SCM_VMID_HLOS)},
+   {BIT(QCOM_SCM_VMID_HLOS)} };
+   struct qcom_scm_vmperm next[] = {{0} };

You don't need to initialize this, and if you just keep it "struct
qcom_scm_vmperm next" you can pass it as  in the
qcom_scm_assign_mem() call.


+   int ret;
+
+   if (!qproc->need_mem_protection)
+   return 0;
+
+   if (current_owner[image][0] == BIT(QCOM_SCM_VMID_HLOS)) {

And rather than making this flip back and forth with every call, I think
it's more robust if you pass the new expected owner as a parameter to
the function. Simplest way I can think of it to add a "bool
remote_owner" as a parameter; it makes the changes minimal and works
with the naming of the function.


+   next->vmid = QCOM_SCM_VMID_MSS_MSA;
+   next->perm = QCOM_SCM_PERM_RW;
+   } else {
+   next->vmid = QCOM_SCM_VMID_HLOS;
+   next->perm = QCOM_SCM_PERM_RWX;
+   }
+
+   ret = qcom_scm_assign_mem(addr, ALIGN(siz

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

[Dwivedi, Avaneesh Kumar (avani)]

On 6/2/2017 11:25 PM, Bjorn Andersson wrote:

On Thu 01 Jun 14:42 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote:


Hi Bjorn,

Thanks lot many for such a blazing fast response :)

regarding your points.

a- Do you mean caller's of q6v5_xfer_mem_ownership() should pass two
additional inputs i.e. *next_perm and *next_vmid


You have two cases; assign to HLOS and assign to MSS, so I imagine that
you pass a single indicator of which you want to assign. I.e. rather
than looking at what the current state is and flipping you pass the
conditional of that if statement as a parameter.

OK



 and that based on successful return of qcom_scm_assign () they should be
treated as *current_perm and *current_vmid


Instead of your index, you take a "int *curr_perms", which you use as
the current vmid list and you assign at the end of the function (like
you do today).

So to transfer the ownership to the MSS you would make a function call
like:

ret = q6v5_xfer_mem_ownership(qproc, >mpss_owner, ..., true);

mpss_owner would have to be initialize to HLOS before calling this, but
will always be holding the current value.
i am not finding compelling enough region to carry an input pointer to 
hold current ownership
specially when i am carrying a boolean flag to check whether next->vmid 
will be MSS or HLOS
I mean where am i going to use this current owner info in mss rproc 
driver, i am yet not getting enough reason.
while the local array did job of maintaining and flipping the ownership 
based on info if which image ownership transfer is being called.





by caller? if so caller will have to do flipping between MSS and HLOS,
isn't it?


As far as I can see each call to this driver is either a "transfer to
MSS" or "transfer to HLOS", so that shouldn't be a problem.
Yes this job will be done by bool flag, but again what is then use of 
carry pointers  mpss_owner, mba_owner.



Also code churning will increase this way, and also logging the way is
being handled now may require to change again.

or am i misunderstanding your proposal?


Could be that I'm missing something, if above doesn't make sense please
do let me know.
I can not say it does not make sense, probably something subtle i am 
missing to see.



Sorry for inconvenience, but if you could through little more light, that
will help.


There's no inconvenience, thanks for reaching out for clarifications on
my comments.

Thanks for such a nice gesture.

i feel that maintaining the local array for ownership switching looks, 
too raw way of doing something.
may be i can improve that, but  first i need to get understanding of 
your vision in suggesting above changes.


Regards,
Bjorn


-Avaneesh


On 6/2/2017 2:12 AM, Bjorn Andersson wrote:

On Thu 01 Jun 12:17 PDT 2017, Avaneesh Kumar Dwivedi wrote:


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.


Overall this looks good now, I have two minor notes that I want you to
fix up though.


diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct 
rproc *rproc,
return 
   }
+static int q6v5_xfer_mem_ownership(struct q6v5 *qproc,
+  int image, phys_addr_t addr,

Rather than relying on a static int to keep track of current permissions
pass a "int *current_perms", that you update on success.

Add int mba_perm and int mpss_perm to the struct q6v5 and initialize
them in probe and just keep the metadata_perm on the stack in
q6v5_mpss_init_image.


+  size_t size)
+{
+   static int current_owner[3][1] = {{BIT(QCOM_SCM_VMID_HLOS)},
+   {BIT(QCOM_SCM_VMID_HLOS)},
+   {BIT(QCOM_SCM_VMID_HLOS)} };
+   struct qcom_scm_vmperm next[] = {{0} };

You don't need to initialize this, and if you just keep it "struct
qcom_scm_vmperm next" you can pass it as  in the
qcom_scm_assign_mem() call.


+   int ret;
+
+   if (!qproc->need_mem_protection)
+   return 0;
+
+   if (current_owner[image][0] == BIT(QCOM_SCM_VMID_HLOS)) {

And rather than making this flip back and forth with every call, I think
it's more robust if you pass the new expected owner as a parameter to
the function. Simplest way I can think of it to add a "bool
remote_owner" as a parameter; it makes the changes minimal and works
with the naming of the function.


+   next->vmid = QCOM_SCM_VMID_MSS_MSA;
+   next->perm = QCOM_SCM_PERM_RW;
+   } else {
+   next->vmid = QCOM_SCM_VMID_HLOS;
+   next->perm = QCOM_SCM_PERM_RWX;
+   }
+
+   ret = qcom_scm_assign_mem(addr, ALIGN(siz

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

[Dwivedi, Avaneesh Kumar (avani)]

On 6/2/2017 11:52 PM, Stephen Boyd wrote:

On 06/02, Avaneesh Kumar Dwivedi wrote:

Two different processors on a SOC need to switch memory ownership
during load/unload. To enable this, level second memory map table

second level page tables instead of level second memory map table

OK



need to be updated, which is done by secure layer.
This patch add the interface for making secure monitor call for

s/add/adds/

OK.



memory ownership switching request.
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index bb16510..9da3c6d 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -292,6 +304,86 @@ int qcom_scm_pas_shutdown(u32 peripheral)
  }
  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
  
+/**

+ * qcom_scm_assign_mem() - Make a secure call to reassign memory ownership
+ *
+ * @mem_addr: mem region whose ownership need to be reassigned
+ * @mem_sz:   size of the region.
+ * @srcvm:vmid for current set of owners, each set bit in
+ *flag indicate a unique owner
+ * @newvm:array having new owners and corrsponding permission
+ *flags
+ * @dest_cnt: number of owners in next set.
+ * Return next set of owners on success.
+ */
+int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcvm,
+   struct qcom_scm_vmperm *newvm, int dest_cnt)
+{
+   unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
+   struct qcom_scm_current_perm_info *destvm;
+   struct qcom_scm_mem_map_info *mem;
+   phys_addr_t memory_phys;
+   phys_addr_t dest_phys;
+   phys_addr_t src_phys;
+   size_t mem_all_sz;
+   size_t memory_sz;
+   size_t dest_sz;
+   size_t src_sz;
+   int next_vm;
+   __le32 *src;
+   void *ptr;
+   int ret;
+   int i;

Yay reverse christmas tre.

Shall be reversed?



+
+   src_sz = hweight_long(srcvm)*sizeof(*src);

Please add space around that '*':

OK


src_sz = hweight_long(srcvm) * sizeof(*src);


+   memory_sz = sizeof(*mem);
+   dest_sz = dest_cnt*sizeof(*destvm);
+   mem_all_sz = src_sz + memory_sz + dest_sz;
+
+   ptr = dma_alloc_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
+   _phys, GFP_KERNEL, dma_attrs);
+   if (!ptr) {
+   pr_err("mem alloc failed\n");

We don't want memory allocation failure prints. Please remove.

OK



+   return -ENOMEM;
+   }

Newline here!

OK



+   /* Fill source vmid detail */
+   src = (__le32 *)(ptr);

Drop useless parenthesis around ptr please.

OK



+   ret = hweight_long(srcvm);

len = hweight_long(...)?

OK, thought to use less local variables. will change



+   for (i = 0; i < ret; i++) {

i to ret is really weird looking!

Will check if can use below suggestion



+   src[i] = cpu_to_le32(ffs(srcvm) - 1);
+   srcvm ^= 1 << (ffs(srcvm) - 1);
+   }

What if the loop was written like:

for_each_set_bit(i, , sizeof(srcvm))
src[i] = cpu_to_le32(i);

I guess srvcm would have to be a long then.

Will check and adopt



+
+   /* Fill details of mem buff to map */
+   mem = (struct qcom_scm_mem_map_info *)(ptr + ALIGN(src_sz, SZ_64));

Useless cast from void *.

OK



+   memory_phys = src_phys + ALIGN(src_sz, SZ_64);
+   mem[0].mem_addr = cpu_to_le64(mem_addr);
+   mem[0].mem_size = cpu_to_le64(mem_sz);
+
+   next_vm = 0;
+   /* Fill details of next vmid detail */
+   destvm = (struct qcom_scm_current_perm_info *)
+   (ptr + ALIGN(memory_sz, SZ_64) + ALIGN(src_sz, SZ_64));

Useless cast from void.

OK

+   dest_phys = memory_phys + ALIGN(memory_sz, SZ_64);
+   for (i = 0; i < dest_cnt; i++) {
+   destvm[i].vmid = cpu_to_le32(newvm[i].vmid);
+   destvm[i].perm = cpu_to_le32(newvm[i].perm);
+   destvm[i].ctx = 0;
+   destvm[i].ctx_size = 0;
+   next_vm |= BIT(newvm[i].vmid);
+   }

Newline please!

OK



+   ret = __qcom_scm_assign_mem(__scm->dev, memory_phys,
+   memory_sz, src_phys, src_sz, dest_phys, dest_sz);
+   dma_free_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
+   ptr, src_phys, dma_attrs);
+   if (ret == 0)
+   return next_vm;
+   else if (ret > 0)
+   return -ret;

When is ret > 0?
SCM returns positive value in r1 register , and if r1 reg is non zero 
then that should be returned.

Not sure what that indicate.



+   return ret;
+}
+EXPORT_SYMBOL(qcom_scm_assign_mem);
+
  static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
 unsigned long idx)
  {


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

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

[Dwivedi, Avaneesh Kumar (avani)]

On 6/2/2017 11:52 PM, Stephen Boyd wrote:

On 06/02, Avaneesh Kumar Dwivedi wrote:

Two different processors on a SOC need to switch memory ownership
during load/unload. To enable this, level second memory map table

second level page tables instead of level second memory map table

OK



need to be updated, which is done by secure layer.
This patch add the interface for making secure monitor call for

s/add/adds/

OK.



memory ownership switching request.
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index bb16510..9da3c6d 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -292,6 +304,86 @@ int qcom_scm_pas_shutdown(u32 peripheral)
  }
  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
  
+/**

+ * qcom_scm_assign_mem() - Make a secure call to reassign memory ownership
+ *
+ * @mem_addr: mem region whose ownership need to be reassigned
+ * @mem_sz:   size of the region.
+ * @srcvm:vmid for current set of owners, each set bit in
+ *flag indicate a unique owner
+ * @newvm:array having new owners and corrsponding permission
+ *flags
+ * @dest_cnt: number of owners in next set.
+ * Return next set of owners on success.
+ */
+int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcvm,
+   struct qcom_scm_vmperm *newvm, int dest_cnt)
+{
+   unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
+   struct qcom_scm_current_perm_info *destvm;
+   struct qcom_scm_mem_map_info *mem;
+   phys_addr_t memory_phys;
+   phys_addr_t dest_phys;
+   phys_addr_t src_phys;
+   size_t mem_all_sz;
+   size_t memory_sz;
+   size_t dest_sz;
+   size_t src_sz;
+   int next_vm;
+   __le32 *src;
+   void *ptr;
+   int ret;
+   int i;

Yay reverse christmas tre.

Shall be reversed?



+
+   src_sz = hweight_long(srcvm)*sizeof(*src);

Please add space around that '*':

OK


src_sz = hweight_long(srcvm) * sizeof(*src);


+   memory_sz = sizeof(*mem);
+   dest_sz = dest_cnt*sizeof(*destvm);
+   mem_all_sz = src_sz + memory_sz + dest_sz;
+
+   ptr = dma_alloc_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
+   _phys, GFP_KERNEL, dma_attrs);
+   if (!ptr) {
+   pr_err("mem alloc failed\n");

We don't want memory allocation failure prints. Please remove.

OK



+   return -ENOMEM;
+   }

Newline here!

OK



+   /* Fill source vmid detail */
+   src = (__le32 *)(ptr);

Drop useless parenthesis around ptr please.

OK



+   ret = hweight_long(srcvm);

len = hweight_long(...)?

OK, thought to use less local variables. will change



+   for (i = 0; i < ret; i++) {

i to ret is really weird looking!

Will check if can use below suggestion



+   src[i] = cpu_to_le32(ffs(srcvm) - 1);
+   srcvm ^= 1 << (ffs(srcvm) - 1);
+   }

What if the loop was written like:

for_each_set_bit(i, , sizeof(srcvm))
src[i] = cpu_to_le32(i);

I guess srvcm would have to be a long then.

Will check and adopt



+
+   /* Fill details of mem buff to map */
+   mem = (struct qcom_scm_mem_map_info *)(ptr + ALIGN(src_sz, SZ_64));

Useless cast from void *.

OK



+   memory_phys = src_phys + ALIGN(src_sz, SZ_64);
+   mem[0].mem_addr = cpu_to_le64(mem_addr);
+   mem[0].mem_size = cpu_to_le64(mem_sz);
+
+   next_vm = 0;
+   /* Fill details of next vmid detail */
+   destvm = (struct qcom_scm_current_perm_info *)
+   (ptr + ALIGN(memory_sz, SZ_64) + ALIGN(src_sz, SZ_64));

Useless cast from void.

OK

+   dest_phys = memory_phys + ALIGN(memory_sz, SZ_64);
+   for (i = 0; i < dest_cnt; i++) {
+   destvm[i].vmid = cpu_to_le32(newvm[i].vmid);
+   destvm[i].perm = cpu_to_le32(newvm[i].perm);
+   destvm[i].ctx = 0;
+   destvm[i].ctx_size = 0;
+   next_vm |= BIT(newvm[i].vmid);
+   }

Newline please!

OK



+   ret = __qcom_scm_assign_mem(__scm->dev, memory_phys,
+   memory_sz, src_phys, src_sz, dest_phys, dest_sz);
+   dma_free_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
+   ptr, src_phys, dma_attrs);
+   if (ret == 0)
+   return next_vm;
+   else if (ret > 0)
+   return -ret;

When is ret > 0?
SCM returns positive value in r1 register , and if r1 reg is non zero 
then that should be returned.

Not sure what that indicate.



+   return ret;
+}
+EXPORT_SYMBOL(qcom_scm_assign_mem);
+
  static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
 unsigned long idx)
  {


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

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

[Dwivedi, Avaneesh Kumar (avani)]

Hi Bjorn,

Thanks lot many for such a blazing fast response :)

regarding your points.

a- Do you mean caller's of q6v5_xfer_mem_ownership() should pass two 
additional inputs i.e. *next_perm and *next_vmid


and that based on successful return of qcom_scm_assign () they 
should be treated as *current_perm and *current_vmid


   by caller? if so caller will have to do flipping between MSS and 
HLOS, isn't it?


   Also code churning will increase this way, and also logging the way 
is being handled now may require to change again.


   or am i misunderstanding your proposal?

   Sorry for inconvenience, but if you could through little more light, 
that will help.



-Avaneesh


On 6/2/2017 2:12 AM, Bjorn Andersson wrote:

On Thu 01 Jun 12:17 PDT 2017, Avaneesh Kumar Dwivedi wrote:


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.


Overall this looks good now, I have two minor notes that I want you to
fix up though.


diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct 
rproc *rproc,
return 
  }
  
+static int q6v5_xfer_mem_ownership(struct q6v5 *qproc,

+  int image, phys_addr_t addr,

Rather than relying on a static int to keep track of current permissions
pass a "int *current_perms", that you update on success.

Add int mba_perm and int mpss_perm to the struct q6v5 and initialize
them in probe and just keep the metadata_perm on the stack in
q6v5_mpss_init_image.


+  size_t size)
+{
+   static int current_owner[3][1] = {{BIT(QCOM_SCM_VMID_HLOS)},
+   {BIT(QCOM_SCM_VMID_HLOS)},
+   {BIT(QCOM_SCM_VMID_HLOS)} };
+   struct qcom_scm_vmperm next[] = {{0} };

You don't need to initialize this, and if you just keep it "struct
qcom_scm_vmperm next" you can pass it as  in the
qcom_scm_assign_mem() call.


+   int ret;
+
+   if (!qproc->need_mem_protection)
+   return 0;
+
+   if (current_owner[image][0] == BIT(QCOM_SCM_VMID_HLOS)) {

And rather than making this flip back and forth with every call, I think
it's more robust if you pass the new expected owner as a parameter to
the function. Simplest way I can think of it to add a "bool
remote_owner" as a parameter; it makes the changes minimal and works
with the naming of the function.


+   next->vmid = QCOM_SCM_VMID_MSS_MSA;
+   next->perm = QCOM_SCM_PERM_RW;
+   } else {
+   next->vmid = QCOM_SCM_VMID_HLOS;
+   next->perm = QCOM_SCM_PERM_RWX;
+   }
+
+   ret = qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K),
+   current_owner[image][0], next, 1);
+   if (ret < 0) {
+   pr_err("Failed to assign %s memory access in range %p to %p ret = 
%d\n",
+   (image == 0 ? "MDT" : image == 1 ? "MBA" : "MPSS"),
+   (void *)addr, (void *)(addr + size), ret);
+   return ret;
+   }
+
+   current_owner[image][0] = ret;
+   return 0;
+}
+

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

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

[Dwivedi, Avaneesh Kumar (avani)]

Hi Bjorn,

Thanks lot many for such a blazing fast response :)

regarding your points.

a- Do you mean caller's of q6v5_xfer_mem_ownership() should pass two 
additional inputs i.e. *next_perm and *next_vmid


and that based on successful return of qcom_scm_assign () they 
should be treated as *current_perm and *current_vmid


   by caller? if so caller will have to do flipping between MSS and 
HLOS, isn't it?


   Also code churning will increase this way, and also logging the way 
is being handled now may require to change again.


   or am i misunderstanding your proposal?

   Sorry for inconvenience, but if you could through little more light, 
that will help.



-Avaneesh


On 6/2/2017 2:12 AM, Bjorn Andersson wrote:

On Thu 01 Jun 12:17 PDT 2017, Avaneesh Kumar Dwivedi wrote:


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.


Overall this looks good now, I have two minor notes that I want you to
fix up though.


diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct 
rproc *rproc,
return 
  }
  
+static int q6v5_xfer_mem_ownership(struct q6v5 *qproc,

+  int image, phys_addr_t addr,

Rather than relying on a static int to keep track of current permissions
pass a "int *current_perms", that you update on success.

Add int mba_perm and int mpss_perm to the struct q6v5 and initialize
them in probe and just keep the metadata_perm on the stack in
q6v5_mpss_init_image.


+  size_t size)
+{
+   static int current_owner[3][1] = {{BIT(QCOM_SCM_VMID_HLOS)},
+   {BIT(QCOM_SCM_VMID_HLOS)},
+   {BIT(QCOM_SCM_VMID_HLOS)} };
+   struct qcom_scm_vmperm next[] = {{0} };

You don't need to initialize this, and if you just keep it "struct
qcom_scm_vmperm next" you can pass it as  in the
qcom_scm_assign_mem() call.


+   int ret;
+
+   if (!qproc->need_mem_protection)
+   return 0;
+
+   if (current_owner[image][0] == BIT(QCOM_SCM_VMID_HLOS)) {

And rather than making this flip back and forth with every call, I think
it's more robust if you pass the new expected owner as a parameter to
the function. Simplest way I can think of it to add a "bool
remote_owner" as a parameter; it makes the changes minimal and works
with the naming of the function.


+   next->vmid = QCOM_SCM_VMID_MSS_MSA;
+   next->perm = QCOM_SCM_PERM_RW;
+   } else {
+   next->vmid = QCOM_SCM_VMID_HLOS;
+   next->perm = QCOM_SCM_PERM_RWX;
+   }
+
+   ret = qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K),
+   current_owner[image][0], next, 1);
+   if (ret < 0) {
+   pr_err("Failed to assign %s memory access in range %p to %p ret = 
%d\n",
+   (image == 0 ? "MDT" : image == 1 ? "MBA" : "MPSS"),
+   (void *)addr, (void *)(addr + size), ret);
+   return ret;
+   }
+
+   current_owner[image][0] = ret;
+   return 0;
+}
+

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
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

[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: 
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_ONBIT(24)
+#define QDSP6v56_CLAMP_WL  BIT(21)
+#define QDSP6v56_CLAMP_QMC_MEM BIT(22)
+#define HALT_CHECK_MAX_LOOPS   200
+#define QDSP6SS_XO_CBCR0x0038
+#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.

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: 
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_ONBIT(24)
+#define QDSP6v56_CLAMP_WL  BIT(21)
+#define QDSP6v56_CLAMP_QMC_MEM BIT(22)
+#define HALT_CHECK_MAX_LOOPS   200
+#define QDSP6SS_XO_CBCR0x0038
+#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.

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), , 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(>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 

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), , 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(>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 

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 
---
  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 = [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.

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 
---
  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 = [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.

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),
+   [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) 

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),
+   [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) 

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 <akdwi...@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 = [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_M

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 
---
   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 = [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, 1);
   

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 
---
  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 = [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, 1);

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

[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 
---
  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 = [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, 1);

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: [PATCH v3 1/2] soc: qcom: Add support of scm call for mss rproc to share access of ddr

[Dwivedi, Avaneesh Kumar (avani)]

On 4/6/2017 5:14 AM, Stephen Boyd wrote:

On 03/08, Avaneesh Kumar Dwivedi wrote:

diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 4a0f5ea..187fc00 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool 
reset)
  
  	return ret ? : res.a1;

  }
+
+int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid)

Why are we passing a structure copy?
I did not use struct pointer cause i am not modifying any of the passed 
value, do you think i should use struct pointer than pass by value?





+{
+   int ret;
+   struct qcom_scm_desc desc = {0};
+   struct arm_smccc_res res;
+
+   desc.args[0] = vmid.fw_phy;
+   desc.args[1] = vmid.size_fw;
+   desc.args[2] = vmid.from_phy;
+   desc.args[3] = vmid.size_from;
+   desc.args[4] = vmid.to_phy;
+   desc.args[5] = vmid.size_to;

These should all cause sparse warnings because of incorrect
assignments. Given that these are all registers, I'm not sure why
the vmid_detail structure has __le32 in it.

will fix.



+   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,
+   , );
+
+   return ret ? : res.a1;
+}
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 893f953ea..f137f34 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -42,6 +42,18 @@ struct qcom_scm {
  
  static struct qcom_scm *__scm;
  
+struct dest_vm_and_perm_info {

+   __le32 vm;
+   __le32 perm;
+   __le32 *ctx;

Drop the pointer? Just treat it like another number? Pointer is
really odd because it doesn't really make any sense what the
address of the pointer would be.
Downstream this is pointer though unused, that is why kept same will 
check and change.



+   __le32 ctx_size;
+};
+
+struct fw_region_info {
+   __le64 addr;
+   __le64 size;
+};
+
  static int qcom_scm_clk_enable(void)
  {
int ret;
@@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral)
  }
  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
  
+/**

+ * qcom_scm_assign_mem() - Allocate and fill vmid detail of old
+ * new owners of memory region for fw and metadata etc, Which is
+ * further passed to hypervisor, which does translate intermediate
+ * physical address used by subsystems.

Maybe this can be the long description, but the short description
should be shorter.

OK :(



+ * @vmid: structure with pointers and size detail of old and new
+ * owners vmid detail.
+ * Return 0 on success.

There's a standard syntax for return too. Look at kernel doc
howto please.

OK.



+ */
+int qcom_scm_assign_mem(struct vmid_detail vmid)

Please no structure copy.

should struct pointer be OK, or direct argument passing?



+{
+   unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
+   struct dest_vm_and_perm_info *to;
+   struct fw_region_info *fw_info;
+   __le64 fw_phy;
+   __le32 *from;
+   int ret = -ENOMEM;

Not sure why we assign it. It gets overwritten with the first use.

Yes will correct.



+   int i;
+
+   from = dma_alloc_attrs(__scm->dev, vmid.size_from,
+   _phy, GFP_KERNEL, dma_attrs);
+   if (!from) {
+   dev_err(__scm->dev,
+   "failed to allocate buffer to pass source vmid 
detail\n");
+   return -ENOMEM;
+   }
+   to = dma_alloc_attrs(__scm->dev, vmid.size_to,
+   _phy, GFP_KERNEL, dma_attrs);
+   if (!to) {
+   dev_err(__scm->dev,
+   "failed to allocate buffer to pass destination vmid 
detail\n");
+   goto free_src_buff;
+   }
+   fw_info = dma_alloc_attrs(__scm->dev, sizeof(*fw_info),
+   _phy, GFP_KERNEL, dma_attrs);

Can we consolidate this into one allocation with the appropriate
size and then offset the different structures in it?

OK.

+   if (!fw_info) {
+   dev_err(__scm->dev,
+   "failed to allocate buffer to pass firmware detail\n");
+   goto free_dest_buff;
+   }
+
+   /* copy detail of original owner of ddr region */
+   /* in physically contigious buffer */
+   memcpy(from, vmid.from, vmid.size_from);
+
+   /* fill details of new owners of ddr region*/
+   /* in physically contigious buffer */
+   for (i = 0; i < (vmid.size_to / sizeof(__le32)); i++) {
+   to[i].vm = vmid.to[i];
+   to[i].perm = vmid.permission[i];

Here you want the cpu_to_le32() stuff. Please run sparse.
OK, last time i tried running sparse but 

Re: [PATCH v3 1/2] soc: qcom: Add support of scm call for mss rproc to share access of ddr

[Dwivedi, Avaneesh Kumar (avani)]

On 4/6/2017 5:14 AM, Stephen Boyd wrote:

On 03/08, Avaneesh Kumar Dwivedi wrote:

diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 4a0f5ea..187fc00 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool 
reset)
  
  	return ret ? : res.a1;

  }
+
+int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid)

Why are we passing a structure copy?
I did not use struct pointer cause i am not modifying any of the passed 
value, do you think i should use struct pointer than pass by value?





+{
+   int ret;
+   struct qcom_scm_desc desc = {0};
+   struct arm_smccc_res res;
+
+   desc.args[0] = vmid.fw_phy;
+   desc.args[1] = vmid.size_fw;
+   desc.args[2] = vmid.from_phy;
+   desc.args[3] = vmid.size_from;
+   desc.args[4] = vmid.to_phy;
+   desc.args[5] = vmid.size_to;

These should all cause sparse warnings because of incorrect
assignments. Given that these are all registers, I'm not sure why
the vmid_detail structure has __le32 in it.

will fix.



+   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,
+   , );
+
+   return ret ? : res.a1;
+}
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 893f953ea..f137f34 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -42,6 +42,18 @@ struct qcom_scm {
  
  static struct qcom_scm *__scm;
  
+struct dest_vm_and_perm_info {

+   __le32 vm;
+   __le32 perm;
+   __le32 *ctx;

Drop the pointer? Just treat it like another number? Pointer is
really odd because it doesn't really make any sense what the
address of the pointer would be.
Downstream this is pointer though unused, that is why kept same will 
check and change.



+   __le32 ctx_size;
+};
+
+struct fw_region_info {
+   __le64 addr;
+   __le64 size;
+};
+
  static int qcom_scm_clk_enable(void)
  {
int ret;
@@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral)
  }
  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
  
+/**

+ * qcom_scm_assign_mem() - Allocate and fill vmid detail of old
+ * new owners of memory region for fw and metadata etc, Which is
+ * further passed to hypervisor, which does translate intermediate
+ * physical address used by subsystems.

Maybe this can be the long description, but the short description
should be shorter.

OK :(



+ * @vmid: structure with pointers and size detail of old and new
+ * owners vmid detail.
+ * Return 0 on success.

There's a standard syntax for return too. Look at kernel doc
howto please.

OK.



+ */
+int qcom_scm_assign_mem(struct vmid_detail vmid)

Please no structure copy.

should struct pointer be OK, or direct argument passing?



+{
+   unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
+   struct dest_vm_and_perm_info *to;
+   struct fw_region_info *fw_info;
+   __le64 fw_phy;
+   __le32 *from;
+   int ret = -ENOMEM;

Not sure why we assign it. It gets overwritten with the first use.

Yes will correct.



+   int i;
+
+   from = dma_alloc_attrs(__scm->dev, vmid.size_from,
+   _phy, GFP_KERNEL, dma_attrs);
+   if (!from) {
+   dev_err(__scm->dev,
+   "failed to allocate buffer to pass source vmid 
detail\n");
+   return -ENOMEM;
+   }
+   to = dma_alloc_attrs(__scm->dev, vmid.size_to,
+   _phy, GFP_KERNEL, dma_attrs);
+   if (!to) {
+   dev_err(__scm->dev,
+   "failed to allocate buffer to pass destination vmid 
detail\n");
+   goto free_src_buff;
+   }
+   fw_info = dma_alloc_attrs(__scm->dev, sizeof(*fw_info),
+   _phy, GFP_KERNEL, dma_attrs);

Can we consolidate this into one allocation with the appropriate
size and then offset the different structures in it?

OK.

+   if (!fw_info) {
+   dev_err(__scm->dev,
+   "failed to allocate buffer to pass firmware detail\n");
+   goto free_dest_buff;
+   }
+
+   /* copy detail of original owner of ddr region */
+   /* in physically contigious buffer */
+   memcpy(from, vmid.from, vmid.size_from);
+
+   /* fill details of new owners of ddr region*/
+   /* in physically contigious buffer */
+   for (i = 0; i < (vmid.size_to / sizeof(__le32)); i++) {
+   to[i].vm = vmid.to[i];
+   to[i].perm = vmid.permission[i];

Here you want the cpu_to_le32() stuff. Please run sparse.
OK, last time i tried running sparse but 

Re: [PATCH v3 1/2] soc: qcom: Add support of scm call for mss rproc to share access of ddr

[Dwivedi, Avaneesh Kumar (avani)]

Hi Bjorn/Boyd,

thanks for comments and suggestion, beg apology for delayed reply as i 
got busy with internal issues.


please see inline response.


On 4/6/2017 10:13 AM, Bjorn Andersson wrote:

On Wed 08 Mar 10:03 PST 2017, Avaneesh Kumar Dwivedi wrote:


This patch add scm call support to make hypervisor call to enable access
of fw regions in ddr to mss subsystem on arm-v8 arch soc's.

Signed-off-by: Avaneesh Kumar Dwivedi 
---
  drivers/firmware/qcom_scm-64.c |  25 +++
  drivers/firmware/qcom_scm.c|  93 ++
  drivers/firmware/qcom_scm.h|   3 +
  drivers/remoteproc/qcom_q6v5_pil.c | 129 -
  include/linux/qcom_scm.h   |  14 
  5 files changed, 262 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 4a0f5ea..187fc00 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool 
reset)
  
  	return ret ? : res.a1;

  }
+
+int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid)

Rather than packing these parameters up in a struct I think it's cleaner
to just pass them directly.
passing so many parameters directly is not seen good practice specially 
when number of parameters to pass are many.
that is why i used struct copy, i did not use reference of struct since 
values passed were not going to be modified.

but will surely look if i can work with direct passing of params.



+{
+   int ret;
+   struct qcom_scm_desc desc = {0};
+   struct arm_smccc_res res;
+
+   desc.args[0] = vmid.fw_phy;
+   desc.args[1] = vmid.size_fw;
+   desc.args[2] = vmid.from_phy;
+   desc.args[3] = vmid.size_from;
+   desc.args[4] = vmid.to_phy;
+   desc.args[5] = vmid.size_to;
+   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,
+   , );
+
+   return ret ? : res.a1;

If I understand the downstream code we only care about "ret" here; being
zero on success or negative on error.

sure, will check.

+}
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 893f953ea..f137f34 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -42,6 +42,18 @@ struct qcom_scm {
  
  static struct qcom_scm *__scm;
  
+struct dest_vm_and_perm_info {

+   __le32 vm;
+   __le32 perm;
+   __le32 *ctx;

Please be explicit about the fact that this is 64 bit.

sorry i am not sure why they need to be 64 bit variable?



+   __le32 ctx_size;
+};

This should be __packed

Ok.



+
+struct fw_region_info {
+   __le64 addr;
+   __le64 size;
+};
+
  static int qcom_scm_clk_enable(void)
  {
int ret;
@@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral)
  }
  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
  
+/**

+ * qcom_scm_assign_mem() - Allocate and fill vmid detail of old
+ * new owners of memory region for fw and metadata etc, Which is
+ * further passed to hypervisor, which does translate intermediate
+ * physical address used by subsystems.
+ * @vmid: structure with pointers and size detail of old and new
+ * owners vmid detail.
+ * Return 0 on success.
+ */
+int qcom_scm_assign_mem(struct vmid_detail vmid)

After a long discussion with Stephen I now think that I understand
what's actually going on here.

So this function will request TZ to remove all permissions for the
memory region in the tables specified in "from" and then for each vmid
in "to" it will set up the specified "permission".

So the "to" and "permissions" are actually a tuple, rather than
independent lists of values. So I think this should be exposed in the
prototype, as a list of  entries.

To make the function prototype more convenient I think you should turn
"from" into a bitmap (e.g. BIT(VMID_HLOS) | BIT(VMID_MSS_MSA)).

i am not able to fully comprehend advantage of turning "from" into bitmap.
moreover when i read your below suggestion, which suggest to use "struct 
qcom_scm_mem_perm" as below then i get further confused what i will do 
with bitmap?


If you then make the function, on success, return "to" as a bitmap the
caller can simply store that in a state variable and pass it as "from"
in the next call.

So you would have:

   struct qcom_scm_mem_perm new_perms[] = {
{ VMID_HLOS, PERM_READ },   
{ VMID_MSS_MSA, PREM_READ | PERM_WRITE },
   };
   
   current_perms = qcom_scm_assign_mem(ptr, size, current_perms, new_perms, 2);

This looks OK, will try to incorporate this idea.



And I believe something like "curr_perm" and "new_perm" 

Re: [PATCH v3 1/2] soc: qcom: Add support of scm call for mss rproc to share access of ddr

[Dwivedi, Avaneesh Kumar (avani)]

Hi Bjorn/Boyd,

thanks for comments and suggestion, beg apology for delayed reply as i 
got busy with internal issues.


please see inline response.


On 4/6/2017 10:13 AM, Bjorn Andersson wrote:

On Wed 08 Mar 10:03 PST 2017, Avaneesh Kumar Dwivedi wrote:


This patch add scm call support to make hypervisor call to enable access
of fw regions in ddr to mss subsystem on arm-v8 arch soc's.

Signed-off-by: Avaneesh Kumar Dwivedi 
---
  drivers/firmware/qcom_scm-64.c |  25 +++
  drivers/firmware/qcom_scm.c|  93 ++
  drivers/firmware/qcom_scm.h|   3 +
  drivers/remoteproc/qcom_q6v5_pil.c | 129 -
  include/linux/qcom_scm.h   |  14 
  5 files changed, 262 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 4a0f5ea..187fc00 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool 
reset)
  
  	return ret ? : res.a1;

  }
+
+int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid)

Rather than packing these parameters up in a struct I think it's cleaner
to just pass them directly.
passing so many parameters directly is not seen good practice specially 
when number of parameters to pass are many.
that is why i used struct copy, i did not use reference of struct since 
values passed were not going to be modified.

but will surely look if i can work with direct passing of params.



+{
+   int ret;
+   struct qcom_scm_desc desc = {0};
+   struct arm_smccc_res res;
+
+   desc.args[0] = vmid.fw_phy;
+   desc.args[1] = vmid.size_fw;
+   desc.args[2] = vmid.from_phy;
+   desc.args[3] = vmid.size_from;
+   desc.args[4] = vmid.to_phy;
+   desc.args[5] = vmid.size_to;
+   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,
+   , );
+
+   return ret ? : res.a1;

If I understand the downstream code we only care about "ret" here; being
zero on success or negative on error.

sure, will check.

+}
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 893f953ea..f137f34 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -42,6 +42,18 @@ struct qcom_scm {
  
  static struct qcom_scm *__scm;
  
+struct dest_vm_and_perm_info {

+   __le32 vm;
+   __le32 perm;
+   __le32 *ctx;

Please be explicit about the fact that this is 64 bit.

sorry i am not sure why they need to be 64 bit variable?



+   __le32 ctx_size;
+};

This should be __packed

Ok.



+
+struct fw_region_info {
+   __le64 addr;
+   __le64 size;
+};
+
  static int qcom_scm_clk_enable(void)
  {
int ret;
@@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral)
  }
  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
  
+/**

+ * qcom_scm_assign_mem() - Allocate and fill vmid detail of old
+ * new owners of memory region for fw and metadata etc, Which is
+ * further passed to hypervisor, which does translate intermediate
+ * physical address used by subsystems.
+ * @vmid: structure with pointers and size detail of old and new
+ * owners vmid detail.
+ * Return 0 on success.
+ */
+int qcom_scm_assign_mem(struct vmid_detail vmid)

After a long discussion with Stephen I now think that I understand
what's actually going on here.

So this function will request TZ to remove all permissions for the
memory region in the tables specified in "from" and then for each vmid
in "to" it will set up the specified "permission".

So the "to" and "permissions" are actually a tuple, rather than
independent lists of values. So I think this should be exposed in the
prototype, as a list of  entries.

To make the function prototype more convenient I think you should turn
"from" into a bitmap (e.g. BIT(VMID_HLOS) | BIT(VMID_MSS_MSA)).

i am not able to fully comprehend advantage of turning "from" into bitmap.
moreover when i read your below suggestion, which suggest to use "struct 
qcom_scm_mem_perm" as below then i get further confused what i will do 
with bitmap?


If you then make the function, on success, return "to" as a bitmap the
caller can simply store that in a state variable and pass it as "from"
in the next call.

So you would have:

   struct qcom_scm_mem_perm new_perms[] = {
{ VMID_HLOS, PERM_READ },   
{ VMID_MSS_MSA, PREM_READ | PERM_WRITE },
   };
   
   current_perms = qcom_scm_assign_mem(ptr, size, current_perms, new_perms, 2);

This looks OK, will try to incorporate this idea.



And I believe something like "curr_perm" and "new_perm" are even better
names than "from" and "to"

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

[Dwivedi, Avaneesh Kumar (avani)]

On 2/28/2017 4:25 AM, Stephen Boyd wrote:

On 01/30, Avaneesh Kumar Dwivedi wrote:

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 893f953ea..f476803 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -292,6 +292,20 @@ int qcom_scm_pas_shutdown(u32 peripheral)
  }
  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
  
+/**

+ * qcom_scm_assign_mem() - Apply new access permission of DDR
+ * region passed via descriptor and does second stage
+ * translation of intermediate physical address.
+ * @desc: descriptor to send to hypervisor
+ *
+ * Return 0 on success.
+ */
+int qcom_scm_assign_mem(void *desc)

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

OK, so i will try to move hyp_mem_assign() to qcom_scm driver, as suggested.
this way the above issue will also get resolved.



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

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

OK.



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


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

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

[Dwivedi, Avaneesh Kumar (avani)]

On 2/28/2017 4:25 AM, Stephen Boyd wrote:

On 01/30, Avaneesh Kumar Dwivedi wrote:

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 893f953ea..f476803 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -292,6 +292,20 @@ int qcom_scm_pas_shutdown(u32 peripheral)
  }
  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
  
+/**

+ * qcom_scm_assign_mem() - Apply new access permission of DDR
+ * region passed via descriptor and does second stage
+ * translation of intermediate physical address.
+ * @desc: descriptor to send to hypervisor
+ *
+ * Return 0 on success.
+ */
+int qcom_scm_assign_mem(void *desc)

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

OK, so i will try to move hyp_mem_assign() to qcom_scm driver, as suggested.
this way the above issue will also get resolved.



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

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

OK.



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


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

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

[Dwivedi, Avaneesh Kumar (avani)]

On 2/28/2017 12:46 PM, Stephen Boyd wrote:

On 01/30, Avaneesh Kumar Dwivedi wrote:

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

Signed-off-by: Avaneesh Kumar Dwivedi 
---

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

OK.



  drivers/remoteproc/qcom_q6v5_pil.c | 202 -
  1 file changed, 198 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
b/drivers/remoteproc/qcom_q6v5_pil.c
index e5edefa..35eee68 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -93,6 +93,23 @@
  #define QDSS_BHS_ON   BIT(21)
  #define QDSS_LDO_BYP  BIT(22)
  
+struct dest_vm_and_perm_info {

+   __le32 vm;
+   __le32 perm;
+   __le32 *ctx;
+   __le32 ctx_size;
+};
+
+struct mem_prot_info {
+   __le64 addr;
+   __le64 size;
+};
+
+struct scm_desc {
+   __le32 arginfo;
+   __le64 args[10];
+};

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

OK.



+
  struct reg_info {
struct regulator *reg;
int uV;
@@ -111,6 +128,7 @@ struct rproc_hexagon_res {
struct qcom_mss_reg_res active_supply[2];
char **proxy_clk_names;
char **active_clk_names;
+   int version;
  };
  
  struct q6v5 {

@@ -152,8 +170,29 @@ struct q6v5 {
phys_addr_t mpss_reloc;
void *mpss_region;
size_t mpss_size;
+   int version;
+   int protection_cmd;
+};
+
+enum {
+   MSS_MSM8916,
+   MSS_MSM8974,
+   MSS_MSM8996,
  };
  
+enum {

+   ASSIG_ACCESS_MSA,
+   REMOV_ACCESS_MSA,
+   SHARE_ACCESS_MSA,
+   REMOV_SHARE_MSA,
+};
+
+#define VMID_HLOS  0x3
+#define VMID_MSS_MSA   0xF
+#define PERM_READ  0x4
+#define PERM_WRITE 0x2
+#define PERM_EXEC  0x1
+#define PERM_RW(0x4 | 0x2)

Please USE PERM_READ | PERM_WRITE here instead.

OK.



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

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

Will analyses and modify as per suggestion.



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

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

OK



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

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

in some cases we initialize two elements.



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

Same here.

OK.



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

Please add spaces around curly braces like { this }

OK.



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

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

OK.



+   src = (int[2]) {VMID_HLOS, VMID_MSS_MSA};
+   

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

[Dwivedi, Avaneesh Kumar (avani)]

On 2/28/2017 12:46 PM, Stephen Boyd wrote:

On 01/30, Avaneesh Kumar Dwivedi wrote:

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

Signed-off-by: Avaneesh Kumar Dwivedi 
---

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

OK.



  drivers/remoteproc/qcom_q6v5_pil.c | 202 -
  1 file changed, 198 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
b/drivers/remoteproc/qcom_q6v5_pil.c
index e5edefa..35eee68 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -93,6 +93,23 @@
  #define QDSS_BHS_ON   BIT(21)
  #define QDSS_LDO_BYP  BIT(22)
  
+struct dest_vm_and_perm_info {

+   __le32 vm;
+   __le32 perm;
+   __le32 *ctx;
+   __le32 ctx_size;
+};
+
+struct mem_prot_info {
+   __le64 addr;
+   __le64 size;
+};
+
+struct scm_desc {
+   __le32 arginfo;
+   __le64 args[10];
+};

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

OK.



+
  struct reg_info {
struct regulator *reg;
int uV;
@@ -111,6 +128,7 @@ struct rproc_hexagon_res {
struct qcom_mss_reg_res active_supply[2];
char **proxy_clk_names;
char **active_clk_names;
+   int version;
  };
  
  struct q6v5 {

@@ -152,8 +170,29 @@ struct q6v5 {
phys_addr_t mpss_reloc;
void *mpss_region;
size_t mpss_size;
+   int version;
+   int protection_cmd;
+};
+
+enum {
+   MSS_MSM8916,
+   MSS_MSM8974,
+   MSS_MSM8996,
  };
  
+enum {

+   ASSIG_ACCESS_MSA,
+   REMOV_ACCESS_MSA,
+   SHARE_ACCESS_MSA,
+   REMOV_SHARE_MSA,
+};
+
+#define VMID_HLOS  0x3
+#define VMID_MSS_MSA   0xF
+#define PERM_READ  0x4
+#define PERM_WRITE 0x2
+#define PERM_EXEC  0x1
+#define PERM_RW(0x4 | 0x2)

Please USE PERM_READ | PERM_WRITE here instead.

OK.



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

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

Will analyses and modify as per suggestion.



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

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

OK



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

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

in some cases we initialize two elements.



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

Same here.

OK.



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

Please add spaces around curly braces like { this }

OK.



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

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

OK.



+   src = (int[2]) {VMID_HLOS, VMID_MSS_MSA};
+   dest 

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

[Dwivedi, Avaneesh Kumar (avani)]

On 2/28/2017 4:18 AM, Stephen Boyd wrote:

On 01/30, Avaneesh Kumar Dwivedi wrote:

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
b/drivers/remoteproc/qcom_q6v5_pil.c
index 35eee68..9c12a36 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -485,35 +497,99 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 
status, int ms)
  
  static int q6v5proc_reset(struct q6v5 *qproc)

  {
-   u32 val;
+   u64 val;

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

OK



int ret;
+   int i;
  

[...]

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

Useless parenthesis.

ok



+   writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
+
+   /* BHS require xo cbcr to be enabled */
+   val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
+   val |= 0x1;
+   writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
  
+		/* Read CLKOFF bit to go low indicating CLK is enabled */

+   ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
+   val, !(val & BIT(31)), 1, HALT_CHECK_MAX_LOOPS);
+   if (ret) {
+   dev_err(qproc->dev,
+   "xo cbcr enabling timed out (rc:%d)\n", ret);
+   return ret;
+   }
+   /* Enable power block headswitch and wait for it to stabilize */
+   val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   val |= QDSP6v56_BHS_ON;
+   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   udelay(1);
+
+   /* Put LDO in bypass mode */
+   val |= QDSP6v56_LDO_BYP;
+   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+   /* Deassert QDSP6 compiler memory clamp */
+   val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   val &= ~QDSP6v56_CLAMP_QMC_MEM;
+   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+   /* Deassert memory peripheral sleep and L2 memory standby */
+   val |= (Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N);

Useless parenthesis.

ok



+   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+   /* Turn on L1, L2, ETB and JU memories 1 at a time */
+   val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+   for (i = 19; i >= 0; i--) {
+   val |= BIT(i);
+   writel(val, qproc->reg_base +
+   QDSP6SS_MEM_PWR_CTL);
+   /*
+* Read back value to ensure the write is done then
+* wait for 1us for both memory peripheral and data
+* array to turn on.
+*/
+   val |= readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+   udelay(1);
+   }
+   /* Remove word line clamp */
+   val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   val &= ~QDSP6v56_CLAMP_WL;
+   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   } else {
+   /* Assert resets, stop core */
+   val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
+   val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);

Useless parenthesis.

ok



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

u32 instead of int?

ok


  
  	qproc->running = false;
  
@@ -866,6 +943,15 @@ static int q6v5_stop(struct rproc *rproc)

q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
  
+	if (qproc->version == MSS_MSM8996) {

+   /*
+* To avoid high MX current during LPASS/MSS restart.
+*/

Three lines could be one line comment instead.

ok.



+   val = 

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

[Dwivedi, Avaneesh Kumar (avani)]

On 2/28/2017 4:18 AM, Stephen Boyd wrote:

On 01/30, Avaneesh Kumar Dwivedi wrote:

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
b/drivers/remoteproc/qcom_q6v5_pil.c
index 35eee68..9c12a36 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -485,35 +497,99 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 
status, int ms)
  
  static int q6v5proc_reset(struct q6v5 *qproc)

  {
-   u32 val;
+   u64 val;

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

OK



int ret;
+   int i;
  

[...]

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

Useless parenthesis.

ok



+   writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
+
+   /* BHS require xo cbcr to be enabled */
+   val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
+   val |= 0x1;
+   writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
  
+		/* Read CLKOFF bit to go low indicating CLK is enabled */

+   ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
+   val, !(val & BIT(31)), 1, HALT_CHECK_MAX_LOOPS);
+   if (ret) {
+   dev_err(qproc->dev,
+   "xo cbcr enabling timed out (rc:%d)\n", ret);
+   return ret;
+   }
+   /* Enable power block headswitch and wait for it to stabilize */
+   val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   val |= QDSP6v56_BHS_ON;
+   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   udelay(1);
+
+   /* Put LDO in bypass mode */
+   val |= QDSP6v56_LDO_BYP;
+   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+   /* Deassert QDSP6 compiler memory clamp */
+   val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   val &= ~QDSP6v56_CLAMP_QMC_MEM;
+   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+   /* Deassert memory peripheral sleep and L2 memory standby */
+   val |= (Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N);

Useless parenthesis.

ok



+   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+   /* Turn on L1, L2, ETB and JU memories 1 at a time */
+   val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+   for (i = 19; i >= 0; i--) {
+   val |= BIT(i);
+   writel(val, qproc->reg_base +
+   QDSP6SS_MEM_PWR_CTL);
+   /*
+* Read back value to ensure the write is done then
+* wait for 1us for both memory peripheral and data
+* array to turn on.
+*/
+   val |= readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+   udelay(1);
+   }
+   /* Remove word line clamp */
+   val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   val &= ~QDSP6v56_CLAMP_WL;
+   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   } else {
+   /* Assert resets, stop core */
+   val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
+   val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);

Useless parenthesis.

ok



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

u32 instead of int?

ok


  
  	qproc->running = false;
  
@@ -866,6 +943,15 @@ static int q6v5_stop(struct rproc *rproc)

q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
  
+	if (qproc->version == MSS_MSM8996) {

+   /*
+* To avoid high MX current during LPASS/MSS restart.
+*/

Three lines could be one line comment instead.

ok.



+   val = 

Re: [PATCH v3 2/4] remoteproc: qcom: Add additional agree2_clk and px regulator resource.

[Dwivedi, Avaneesh Kumar (avani)]

On 1/31/2017 3:16 AM, Bjorn Andersson wrote:

On Mon 30 Jan 07:03 PST 2017, Avaneesh Kumar Dwivedi wrote:


This patch add additional clock and regulator resource which are
initialized based on compatible and has no impact on existing driver
working. This resourse addition enable the existing driver to handle.
low pass sensor processor device also.

Signed-off-by: Avaneesh Kumar Dwivedi 

Applied, with below modification.

Thanks Bjorn, but please look below inline comment.

---
  drivers/remoteproc/qcom_adsp_pil.c | 43 +++---
  1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/remoteproc/qcom_adsp_pil.c 
b/drivers/remoteproc/qcom_adsp_pil.c

[..]

  static int adsp_init_regulator(struct qcom_adsp *adsp)
  {
-   adsp->cx_supply = devm_regulator_get(adsp->dev, "cx");
+   adsp->cx_supply = devm_regulator_get(adsp->dev, "vdd_cx");

We should not change the name of devicetree properties, so I dropped
"vdd_" on both of these.
I observed that giving "cx" or "px" string to devm_regulator_get() was 
returning with dummy regulator, and if i gave "vdd_cx" and "vdd_px" it 
did not print dummy regulator warning.
in device tree these regulators node were defined as "vdd_cx-supply" and 
"vdd_px-supply"



if (IS_ERR(adsp->cx_supply))
return PTR_ERR(adsp->cx_supply);
  
  	regulator_set_load(adsp->cx_supply, 10);
  
+	adsp->px_supply = devm_regulator_get(adsp->dev, "vdd_px");

+   if (IS_ERR(adsp->px_supply))
+   return PTR_ERR(adsp->px_supply);

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: [PATCH v3 2/4] remoteproc: qcom: Add additional agree2_clk and px regulator resource.

[Dwivedi, Avaneesh Kumar (avani)]

On 1/31/2017 3:16 AM, Bjorn Andersson wrote:

On Mon 30 Jan 07:03 PST 2017, Avaneesh Kumar Dwivedi wrote:


This patch add additional clock and regulator resource which are
initialized based on compatible and has no impact on existing driver
working. This resourse addition enable the existing driver to handle.
low pass sensor processor device also.

Signed-off-by: Avaneesh Kumar Dwivedi 

Applied, with below modification.

Thanks Bjorn, but please look below inline comment.

---
  drivers/remoteproc/qcom_adsp_pil.c | 43 +++---
  1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/remoteproc/qcom_adsp_pil.c 
b/drivers/remoteproc/qcom_adsp_pil.c

[..]

  static int adsp_init_regulator(struct qcom_adsp *adsp)
  {
-   adsp->cx_supply = devm_regulator_get(adsp->dev, "cx");
+   adsp->cx_supply = devm_regulator_get(adsp->dev, "vdd_cx");

We should not change the name of devicetree properties, so I dropped
"vdd_" on both of these.
I observed that giving "cx" or "px" string to devm_regulator_get() was 
returning with dummy regulator, and if i gave "vdd_cx" and "vdd_px" it 
did not print dummy regulator warning.
in device tree these regulators node were defined as "vdd_cx-supply" and 
"vdd_px-supply"



if (IS_ERR(adsp->cx_supply))
return PTR_ERR(adsp->cx_supply);
  
  	regulator_set_load(adsp->cx_supply, 10);
  
+	adsp->px_supply = devm_regulator_get(adsp->dev, "vdd_px");

+   if (IS_ERR(adsp->px_supply))
+   return PTR_ERR(adsp->px_supply);

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: [RFC ADD HYPERVISOR CALL SUPPORT 1/2] soc: qcom: Add hypervisor stage two translation request support.

[Dwivedi, Avaneesh Kumar (avani)]

On 1/21/2017 2:39 PM, Stephen Boyd wrote:

On 01/09, Avaneesh Kumar Dwivedi wrote:

This patch add hypervisor support for mss bring up on msm8996.
MSS rproc driver make hypervisor request to add certain region
in IPA table owned by hepervisor and assign access permission

Please drop the use of IPA here. There's an IPA acronym for the
IP accelerator and that is confused with Intermediate Physical
Address. Instead, say something like "in the stage 2 page tables
of the SMMU". Also hypervisor is misspelled here.

OK.



to modem. These regions are used to load MBA, MDT, FW into DDR.

Signed-off-by: Avaneesh Kumar Dwivedi 
---
  drivers/firmware/qcom_scm-64.c |  16 +++
  drivers/firmware/qcom_scm.c|  13 +++
  drivers/firmware/qcom_scm.h|  10 ++
  drivers/remoteproc/qcom_q6v5_pil.c |  96 +++-

Please split the remoteproc code off from this patch.

Ok.



  drivers/soc/qcom/Kconfig   |   8 ++
  drivers/soc/qcom/Makefile  |   1 +
  drivers/soc/qcom/secure_buffer.c   | 229 +
  include/linux/qcom_scm.h   |   1 +
  include/soc/qcom/secure_buffer.h   |  51 +
  9 files changed, 419 insertions(+), 6 deletions(-)
  create mode 100644 drivers/soc/qcom/secure_buffer.c
  create mode 100644 include/soc/qcom/secure_buffer.h

diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 4a0f5ea..63b814c 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -358,3 +358,19 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool 
reset)
  
  	return ret ? : res.a1;

  }
+
+int __qcom_scm_assign_mem(struct device *dev, void *data)
+{
+   int ret;
+   struct qcom_scm_desc *desc = (struct qcom_scm_desc *)data;

Useless cast from void.

Yes, will correct.

+   struct arm_smccc_res res;
+
+   desc->arginfo = QCOM_SCM_ARGS(7, SCM_RO, SCM_VAL, SCM_RO,
+   SCM_VAL, SCM_RO, SCM_VAL, SCM_VAL);
+
+   ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
+   QCOM_MEM_PROT_ASSIGN_ID,
+   desc, );
+
+   return ret ? : res.a1;
+}
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 893f953ea..52394ac 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -292,6 +292,19 @@ int qcom_scm_pas_shutdown(u32 peripheral)
  }
  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
  
+/**

+ * qcom_scm_assign_mem() -

Some words on what the function does?

OK.



+ * @desc:  descriptor to send to hypervisor
+ *
+ * Return 0 on success.
+ */
+int qcom_scm_assign_mem(void *desc)
+{
+

Nitpick: Drop the extra newline

OK.



+   return __qcom_scm_assign_mem(__scm->dev, desc);
+}
+EXPORT_SYMBOL(qcom_scm_assign_mem);
+
  static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
 unsigned long idx)
  {
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 3584b00..f248dc9 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -47,6 +47,8 @@ extern int __qcom_scm_hdcp_req(struct device *dev,
  #define QCOM_SCM_PAS_SHUTDOWN_CMD 0x6
  #define QCOM_SCM_PAS_IS_SUPPORTED_CMD 0x7
  #define QCOM_SCM_PAS_MSS_RESET0xa
+#define QCOM_SCM_SVC_MP0xc
+#define QCOM_MEM_PROT_ASSIGN_ID0x16
  extern bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral);
  extern int  __qcom_scm_pas_init_image(struct device *dev, u32 peripheral,
dma_addr_t metadata_phys);
@@ -55,6 +57,7 @@ extern int  __qcom_scm_pas_mem_setup(struct device *dev, u32 
peripheral,
  extern int  __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
  extern int  __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
  extern int  __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
+extern int  __qcom_scm_assign_mem(struct device *dev, void *desc);
  
  /* common error codes */

  #define QCOM_SCM_V2_EBUSY -12
@@ -83,4 +86,11 @@ static inline int qcom_scm_remap_error(int err)
return -EINVAL;
  }
  
+enum scm_arg_types {

+   SCM_VAL,
+   SCM_RO,
+   SCM_RW,
+   SCM_BUFVAL,
+};

We already have this enum? It's just prepended with QCOM_
instead.

I think i missed somehow, will see and correct.



+
  #endif
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
b/drivers/remoteproc/qcom_q6v5_pil.c
index e5edefa..cbf833c 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -31,6 +31,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "remoteproc_internal.h"

  #include "qcom_mdt_loader.h"
@@ -105,12 +106,19 @@ struct qcom_mss_reg_res {
int uA;
  };
  
+struct src_dest_vmid {

+   int *srcVM;
+   int *destVM;
+   int *destVMperm;

Why not have an array of these structures instead of a structure

Re: [RFC ADD HYPERVISOR CALL SUPPORT 1/2] soc: qcom: Add hypervisor stage two translation request support.

[Dwivedi, Avaneesh Kumar (avani)]

On 1/21/2017 2:39 PM, Stephen Boyd wrote:

On 01/09, Avaneesh Kumar Dwivedi wrote:

This patch add hypervisor support for mss bring up on msm8996.
MSS rproc driver make hypervisor request to add certain region
in IPA table owned by hepervisor and assign access permission

Please drop the use of IPA here. There's an IPA acronym for the
IP accelerator and that is confused with Intermediate Physical
Address. Instead, say something like "in the stage 2 page tables
of the SMMU". Also hypervisor is misspelled here.

OK.



to modem. These regions are used to load MBA, MDT, FW into DDR.

Signed-off-by: Avaneesh Kumar Dwivedi 
---
  drivers/firmware/qcom_scm-64.c |  16 +++
  drivers/firmware/qcom_scm.c|  13 +++
  drivers/firmware/qcom_scm.h|  10 ++
  drivers/remoteproc/qcom_q6v5_pil.c |  96 +++-

Please split the remoteproc code off from this patch.

Ok.



  drivers/soc/qcom/Kconfig   |   8 ++
  drivers/soc/qcom/Makefile  |   1 +
  drivers/soc/qcom/secure_buffer.c   | 229 +
  include/linux/qcom_scm.h   |   1 +
  include/soc/qcom/secure_buffer.h   |  51 +
  9 files changed, 419 insertions(+), 6 deletions(-)
  create mode 100644 drivers/soc/qcom/secure_buffer.c
  create mode 100644 include/soc/qcom/secure_buffer.h

diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 4a0f5ea..63b814c 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -358,3 +358,19 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool 
reset)
  
  	return ret ? : res.a1;

  }
+
+int __qcom_scm_assign_mem(struct device *dev, void *data)
+{
+   int ret;
+   struct qcom_scm_desc *desc = (struct qcom_scm_desc *)data;

Useless cast from void.

Yes, will correct.

+   struct arm_smccc_res res;
+
+   desc->arginfo = QCOM_SCM_ARGS(7, SCM_RO, SCM_VAL, SCM_RO,
+   SCM_VAL, SCM_RO, SCM_VAL, SCM_VAL);
+
+   ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
+   QCOM_MEM_PROT_ASSIGN_ID,
+   desc, );
+
+   return ret ? : res.a1;
+}
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 893f953ea..52394ac 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -292,6 +292,19 @@ int qcom_scm_pas_shutdown(u32 peripheral)
  }
  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
  
+/**

+ * qcom_scm_assign_mem() -

Some words on what the function does?

OK.



+ * @desc:  descriptor to send to hypervisor
+ *
+ * Return 0 on success.
+ */
+int qcom_scm_assign_mem(void *desc)
+{
+

Nitpick: Drop the extra newline

OK.



+   return __qcom_scm_assign_mem(__scm->dev, desc);
+}
+EXPORT_SYMBOL(qcom_scm_assign_mem);
+
  static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
 unsigned long idx)
  {
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 3584b00..f248dc9 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -47,6 +47,8 @@ extern int __qcom_scm_hdcp_req(struct device *dev,
  #define QCOM_SCM_PAS_SHUTDOWN_CMD 0x6
  #define QCOM_SCM_PAS_IS_SUPPORTED_CMD 0x7
  #define QCOM_SCM_PAS_MSS_RESET0xa
+#define QCOM_SCM_SVC_MP0xc
+#define QCOM_MEM_PROT_ASSIGN_ID0x16
  extern bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral);
  extern int  __qcom_scm_pas_init_image(struct device *dev, u32 peripheral,
dma_addr_t metadata_phys);
@@ -55,6 +57,7 @@ extern int  __qcom_scm_pas_mem_setup(struct device *dev, u32 
peripheral,
  extern int  __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
  extern int  __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
  extern int  __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
+extern int  __qcom_scm_assign_mem(struct device *dev, void *desc);
  
  /* common error codes */

  #define QCOM_SCM_V2_EBUSY -12
@@ -83,4 +86,11 @@ static inline int qcom_scm_remap_error(int err)
return -EINVAL;
  }
  
+enum scm_arg_types {

+   SCM_VAL,
+   SCM_RO,
+   SCM_RW,
+   SCM_BUFVAL,
+};

We already have this enum? It's just prepended with QCOM_
instead.

I think i missed somehow, will see and correct.



+
  #endif
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
b/drivers/remoteproc/qcom_q6v5_pil.c
index e5edefa..cbf833c 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -31,6 +31,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "remoteproc_internal.h"

  #include "qcom_mdt_loader.h"
@@ -105,12 +106,19 @@ struct qcom_mss_reg_res {
int uA;
  };
  
+struct src_dest_vmid {

+   int *srcVM;
+   int *destVM;
+   int *destVMperm;

Why not have an array of these structures instead of a structure
with three arrays 

Re: [PATCH v5 3/7] remoteproc: qcom: Add and initialize proxy and active regulators.

[Dwivedi, Avaneesh Kumar (avani)]

On 12/23/2016 3:16 AM, Bjorn Andersson wrote:

On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:


-static int q6v5_regulator_init(struct q6v5 *qproc)
+static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
+   const struct qcom_mss_reg_res *reg_res)
  {
-   int ret;
+   int count = 0;
+   int rc;
+   int i;
  
-	qproc->supply[Q6V5_SUPPLY_CX].supply = "cx";

-   qproc->supply[Q6V5_SUPPLY_MX].supply = "mx";
-   qproc->supply[Q6V5_SUPPLY_MSS].supply = "mss";
-   qproc->supply[Q6V5_SUPPLY_PLL].supply = "pll";
+   while (reg_res[count].supply)
+   count++;
  
-	ret = devm_regulator_bulk_get(qproc->dev,

- ARRAY_SIZE(qproc->supply), qproc->supply);
-   if (ret < 0) {
-   dev_err(qproc->dev, "failed to get supplies\n");
-   return ret;
-   }
+   for (i = 0; i < count; i++) {

As with the clock init you can squash these two loops into one now.

OK.


[..]

  static const struct rproc_hexagon_res msm8916_mss = {
.hexagon_mba_image = "mba.mbn",
+   .proxy_supply = (struct qcom_mss_reg_res[]) {
+   {
+   .supply = "mx",
+   .uV = 105,
+   },
+   {
+   .supply = "cx",
+   .uA = 10,
+   },
+   {
+   .supply = "pll",
+   .uA = 10,
+   },
+   { NULL }

It's idiomatic to use {} instead of { NULL }, so please update this (but
not in the clock patch).

OK.


As with the clock patch, please squash patch 4 into this one - so that
we have regulators before and after applying this single patch.

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: [PATCH v5 3/7] remoteproc: qcom: Add and initialize proxy and active regulators.

[Dwivedi, Avaneesh Kumar (avani)]

On 12/23/2016 3:16 AM, Bjorn Andersson wrote:

On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:


-static int q6v5_regulator_init(struct q6v5 *qproc)
+static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
+   const struct qcom_mss_reg_res *reg_res)
  {
-   int ret;
+   int count = 0;
+   int rc;
+   int i;
  
-	qproc->supply[Q6V5_SUPPLY_CX].supply = "cx";

-   qproc->supply[Q6V5_SUPPLY_MX].supply = "mx";
-   qproc->supply[Q6V5_SUPPLY_MSS].supply = "mss";
-   qproc->supply[Q6V5_SUPPLY_PLL].supply = "pll";
+   while (reg_res[count].supply)
+   count++;
  
-	ret = devm_regulator_bulk_get(qproc->dev,

- ARRAY_SIZE(qproc->supply), qproc->supply);
-   if (ret < 0) {
-   dev_err(qproc->dev, "failed to get supplies\n");
-   return ret;
-   }
+   for (i = 0; i < count; i++) {

As with the clock init you can squash these two loops into one now.

OK.


[..]

  static const struct rproc_hexagon_res msm8916_mss = {
.hexagon_mba_image = "mba.mbn",
+   .proxy_supply = (struct qcom_mss_reg_res[]) {
+   {
+   .supply = "mx",
+   .uV = 105,
+   },
+   {
+   .supply = "cx",
+   .uA = 10,
+   },
+   {
+   .supply = "pll",
+   .uA = 10,
+   },
+   { NULL }

It's idiomatic to use {} instead of { NULL }, so please update this (but
not in the clock patch).

OK.


As with the clock patch, please squash patch 4 into this one - so that
we have regulators before and after applying this single patch.

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: [PATCH v5 2/7] remoteproc: qcom: Add and initialize proxy and active clocks.

[Dwivedi, Avaneesh Kumar (avani)]

On 12/23/2016 3:12 AM, Bjorn Andersson wrote:

On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:


Certain regulators and clocks need voting by rproc on behalf of hexagon
only during restart operation but certain clocks and voltage need to be
voted till hexagon is up, these regulators and clocks are identified as
proxy and active resource respectively, whose handle is being obtained
by supplying proxy and active clock name string.

Signed-off-by: Avaneesh Kumar Dwivedi 
---
  drivers/remoteproc/qcom_q6v5_pil.c | 65 +++---
  1 file changed, 47 insertions(+), 18 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
b/drivers/remoteproc/qcom_q6v5_pil.c
index d875448..8c8b239 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -95,6 +95,8 @@
  
  struct rproc_hexagon_res {

const char *hexagon_mba_image;
+   char **proxy_clk_string;
+   char **active_clk_string;

Use "name" instead of "string" in these variable names - i.e.
proxy_clk_names and active_clk_names.

OK.



  };
  
  struct q6v5 {

@@ -114,6 +116,11 @@ struct q6v5 {
struct qcom_smem_state *state;
unsigned stop_bit;
  
+	struct clk *active_clks[8];

+   struct clk *proxy_clks[4];
+   int active_clk_count;
+   int proxy_clk_count;
+
struct regulator_bulk_data supply[4];
  
  	struct clk *ahb_clk;

@@ -706,27 +713,33 @@ static int q6v5_init_mem(struct q6v5 *qproc, struct 
platform_device *pdev)
return 0;
  }
  
-static int q6v5_init_clocks(struct q6v5 *qproc)

+static int q6v5_init_clocks(struct device *dev, struct clk **clks,
+   char **clk_str)

clk_names

OK.

  {
-   qproc->ahb_clk = devm_clk_get(qproc->dev, "iface");
-   if (IS_ERR(qproc->ahb_clk)) {
-   dev_err(qproc->dev, "failed to get iface clock\n");
-   return PTR_ERR(qproc->ahb_clk);
-   }
+   int count = 0;
+   int i;
  
-	qproc->axi_clk = devm_clk_get(qproc->dev, "bus");

-   if (IS_ERR(qproc->axi_clk)) {
-   dev_err(qproc->dev, "failed to get bus clock\n");
-   return PTR_ERR(qproc->axi_clk);
-   }
+   if (!clk_str)
+   return 0;
+
+   while (clk_str[count])
+   count++;
+
+   for (i = 0; i < count; i++) {

You can squash these two loops into one, e.g. replace them with:

for (i = 0; clk_str[i]; i++) {}

and then return "i".

OK.

+   clks[i] = devm_clk_get(dev, clk_str[i]);
+   if (IS_ERR(clks[i])) {
+
+   int rc = PTR_ERR(clks[i]);
+
+   if (rc != -EPROBE_DEFER)
+   dev_err(dev, "Failed to get %s clock\n",
+   clk_str[i]);
+   return rc;
+   }
  
-	qproc->rom_clk = devm_clk_get(qproc->dev, "mem");

-   if (IS_ERR(qproc->rom_clk)) {
-   dev_err(qproc->dev, "failed to get mem clock\n");
-   return PTR_ERR(qproc->rom_clk);
}
  
-	return 0;

+   return count;
  }
  
  static int q6v5_init_reset(struct q6v5 *qproc)

@@ -843,9 +856,21 @@ static int q6v5_probe(struct platform_device *pdev)
if (ret)
goto free_rproc;
  
-	ret = q6v5_init_clocks(qproc);

-   if (ret)
+   ret = q6v5_init_clocks(>dev, qproc->proxy_clks,
+   desc->proxy_clk_string);
+   if (ret < 0) {
+   dev_err(>dev, "Failed to setup proxy clocks.\n");

Please replace "setup" with "acquire" or "get".

OK.



+   goto free_rproc;
+   }
+   qproc->proxy_clk_count = ret;
+
+   ret = q6v5_init_clocks(>dev, qproc->active_clks,
+   desc->active_clk_string);
+   if (ret < 0) {
+   dev_err(>dev, "Failed to setup active clocks.\n");

Dito.

OK.

goto free_rproc;
+   }
+   qproc->active_clk_count = ret;
  
  	ret = q6v5_regulator_init(qproc);

if (ret)
@@ -901,10 +926,14 @@ static int q6v5_remove(struct platform_device *pdev)
  
  static const struct rproc_hexagon_res msm8916_mss = {

.hexagon_mba_image = "mba.mbn",
+   .proxy_clk_string = (char*[]){"xo", NULL},
+   .active_clk_string = (char*[]){"iface", "bus", "mem", NULL},

Line wrap these list of clock names, like:

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

Makes it consistent with the regulator
patch and it's easier for me to read.

OK.



  };
  
  static const struct rproc_hexagon_res msm8974_mss = {

.hexagon_mba_image = "mba.b00",
+   .proxy_clk_string = (char*[]){"xo", NULL},
+   .active_clk_string = (char*[]){"iface", "bus", "mem", NULL},

Dito

OK

  };

If I apply this patch without patch 5 (Modify clock enable and disable
interface) we will successfully probe 

Re: [PATCH v5 2/7] remoteproc: qcom: Add and initialize proxy and active clocks.

[Dwivedi, Avaneesh Kumar (avani)]

On 12/23/2016 3:12 AM, Bjorn Andersson wrote:

On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:


Certain regulators and clocks need voting by rproc on behalf of hexagon
only during restart operation but certain clocks and voltage need to be
voted till hexagon is up, these regulators and clocks are identified as
proxy and active resource respectively, whose handle is being obtained
by supplying proxy and active clock name string.

Signed-off-by: Avaneesh Kumar Dwivedi 
---
  drivers/remoteproc/qcom_q6v5_pil.c | 65 +++---
  1 file changed, 47 insertions(+), 18 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
b/drivers/remoteproc/qcom_q6v5_pil.c
index d875448..8c8b239 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -95,6 +95,8 @@
  
  struct rproc_hexagon_res {

const char *hexagon_mba_image;
+   char **proxy_clk_string;
+   char **active_clk_string;

Use "name" instead of "string" in these variable names - i.e.
proxy_clk_names and active_clk_names.

OK.



  };
  
  struct q6v5 {

@@ -114,6 +116,11 @@ struct q6v5 {
struct qcom_smem_state *state;
unsigned stop_bit;
  
+	struct clk *active_clks[8];

+   struct clk *proxy_clks[4];
+   int active_clk_count;
+   int proxy_clk_count;
+
struct regulator_bulk_data supply[4];
  
  	struct clk *ahb_clk;

@@ -706,27 +713,33 @@ static int q6v5_init_mem(struct q6v5 *qproc, struct 
platform_device *pdev)
return 0;
  }
  
-static int q6v5_init_clocks(struct q6v5 *qproc)

+static int q6v5_init_clocks(struct device *dev, struct clk **clks,
+   char **clk_str)

clk_names

OK.

  {
-   qproc->ahb_clk = devm_clk_get(qproc->dev, "iface");
-   if (IS_ERR(qproc->ahb_clk)) {
-   dev_err(qproc->dev, "failed to get iface clock\n");
-   return PTR_ERR(qproc->ahb_clk);
-   }
+   int count = 0;
+   int i;
  
-	qproc->axi_clk = devm_clk_get(qproc->dev, "bus");

-   if (IS_ERR(qproc->axi_clk)) {
-   dev_err(qproc->dev, "failed to get bus clock\n");
-   return PTR_ERR(qproc->axi_clk);
-   }
+   if (!clk_str)
+   return 0;
+
+   while (clk_str[count])
+   count++;
+
+   for (i = 0; i < count; i++) {

You can squash these two loops into one, e.g. replace them with:

for (i = 0; clk_str[i]; i++) {}

and then return "i".

OK.

+   clks[i] = devm_clk_get(dev, clk_str[i]);
+   if (IS_ERR(clks[i])) {
+
+   int rc = PTR_ERR(clks[i]);
+
+   if (rc != -EPROBE_DEFER)
+   dev_err(dev, "Failed to get %s clock\n",
+   clk_str[i]);
+   return rc;
+   }
  
-	qproc->rom_clk = devm_clk_get(qproc->dev, "mem");

-   if (IS_ERR(qproc->rom_clk)) {
-   dev_err(qproc->dev, "failed to get mem clock\n");
-   return PTR_ERR(qproc->rom_clk);
}
  
-	return 0;

+   return count;
  }
  
  static int q6v5_init_reset(struct q6v5 *qproc)

@@ -843,9 +856,21 @@ static int q6v5_probe(struct platform_device *pdev)
if (ret)
goto free_rproc;
  
-	ret = q6v5_init_clocks(qproc);

-   if (ret)
+   ret = q6v5_init_clocks(>dev, qproc->proxy_clks,
+   desc->proxy_clk_string);
+   if (ret < 0) {
+   dev_err(>dev, "Failed to setup proxy clocks.\n");

Please replace "setup" with "acquire" or "get".

OK.



+   goto free_rproc;
+   }
+   qproc->proxy_clk_count = ret;
+
+   ret = q6v5_init_clocks(>dev, qproc->active_clks,
+   desc->active_clk_string);
+   if (ret < 0) {
+   dev_err(>dev, "Failed to setup active clocks.\n");

Dito.

OK.

goto free_rproc;
+   }
+   qproc->active_clk_count = ret;
  
  	ret = q6v5_regulator_init(qproc);

if (ret)
@@ -901,10 +926,14 @@ static int q6v5_remove(struct platform_device *pdev)
  
  static const struct rproc_hexagon_res msm8916_mss = {

.hexagon_mba_image = "mba.mbn",
+   .proxy_clk_string = (char*[]){"xo", NULL},
+   .active_clk_string = (char*[]){"iface", "bus", "mem", NULL},

Line wrap these list of clock names, like:

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

Makes it consistent with the regulator
patch and it's easier for me to read.

OK.



  };
  
  static const struct rproc_hexagon_res msm8974_mss = {

.hexagon_mba_image = "mba.b00",
+   .proxy_clk_string = (char*[]){"xo", NULL},
+   .active_clk_string = (char*[]){"iface", "bus", "mem", NULL},

Dito

OK

  };

If I apply this patch without patch 5 (Modify clock enable and disable
interface) we will successfully probe with the new clocks, but 

Re: [PATCH v5 6/7] remoteproc: qcom: Implement Hexagon core specific changes.

[Dwivedi, Avaneesh Kumar (avani)]

On 12/23/2016 6:34 AM, Bjorn Andersson wrote:

On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:


+/* QDSP6v56 parameters */
+#define QDSP6v56_LDO_BYPBIT(25)
+#define QDSP6v56_BHS_ON BIT(24)
+#define QDSP6v56_CLAMP_WL   BIT(21)
+#define QDSP6v56_CLAMP_QMC_MEM  BIT(22)
+#define HALT_CHECK_MAX_LOOPS200
+#define QDSP6SS_XO_CBCR 0x0038

Indent these with tabs, like the others.

Ok.



+#define QDSP6SS_ACC_OVERRIDE_VAL   0x20
+
  struct reg_info {
struct regulator *reg;
int uV;
@@ -111,6 +123,7 @@ struct rproc_hexagon_res {
struct qcom_mss_reg_res active_supply[2];
char **proxy_clk_string;
char **active_clk_string;
+   int hexagon_ver;
  };
  
  struct q6v5 {

@@ -152,8 +165,14 @@ struct q6v5 {
phys_addr_t mpss_reloc;
void *mpss_region;
size_t mpss_size;
+   int hexagon_ver;

Please name this "version" instead, there's little confusion to what it
is.

OK.

  };
  
+enum {

+   MSS_MSM8916, /*hexagon on msm8916*/
+   MSS_MSM8974, /*hexagon on msm8974*/
+   MSS_MSM8996, /*hexagon on msm8996*/

You can skip the comments now.

OK.



+};
  
  static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,

const struct qcom_mss_reg_res *reg_res)
@@ -341,35 +360,107 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 
status, int ms)
  
  static int q6v5proc_reset(struct q6v5 *qproc)

  {
-   u32 val;
+   u64 val;
int ret;
+   int i;
  
-	/* Assert resets, stop core */

-   val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
-   val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
-   writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
  
-	/* Enable power block headswitch, and wait for it to stabilize */

-   val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-   val |= QDSS_BHS_ON | QDSS_LDO_BYP;
-   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-   udelay(1);
-
-   /*
-* Turn on memories. L2 banks should be done individually
-* to minimize inrush current.
-*/
-   val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-   val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
-   Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
-   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-   val |= Q6SS_L2DATA_SLP_NRET_N_2;
-   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-   val |= Q6SS_L2DATA_SLP_NRET_N_1;
-   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-   val |= Q6SS_L2DATA_SLP_NRET_N_0;
-   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   if (qproc->hexagon_ver == 0x2) {

== MSS_MSM8996

OK.

+   /* Override the ACC value if required */
+   writel(QDSP6SS_ACC_OVERRIDE_VAL,
+   qproc->reg_base + QDSP6SS_STRAP_ACC);
+
+   /* Assert resets, stop core */
+   val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
+   val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
+   writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
  
+		/* BHS require xo cbcr to be enabled */

+   val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
+   val |= 0x1;
+   writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);

Please add a comment here, describing what we're waiting for (rather
than just "a magical bit 31")
In off condition bit 31 (CLKOFF) bit is set as 1, when we write 0x1 to 
this register and clock is turned on it become 0x0.

will add appropriate comment.



+   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;
+   }
+   if ((val & BIT(31))) {

If bit 31 is set when the readl_poll_timeout() hits the timeout the
condition will evaluate to false and "ret" will be -ETIMEDOUT. So I
don't think this can happen.

Sure this is not required. Will remove it

+   dev_err(qproc->dev,
+   "Failed to enable xo branch clock.\n");
+   return -EINVAL;
+   }
+   /*
+* Enable power block headswitch,
+* and wait for it to stabilize

This comment fits within 80 characters, so no need to line wrap it.
OK, i had wrapped it because i was getting warning on running 
checkpatch.pl, if fit in one line without warning will turn it into one 
line comment.

+*/
+   val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   val |= QDSP6v56_BHS_ON;
+   writel(val, qproc->reg_base + 

Re: [PATCH v5 6/7] remoteproc: qcom: Implement Hexagon core specific changes.

[Dwivedi, Avaneesh Kumar (avani)]

On 12/23/2016 6:34 AM, Bjorn Andersson wrote:

On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:


+/* QDSP6v56 parameters */
+#define QDSP6v56_LDO_BYPBIT(25)
+#define QDSP6v56_BHS_ON BIT(24)
+#define QDSP6v56_CLAMP_WL   BIT(21)
+#define QDSP6v56_CLAMP_QMC_MEM  BIT(22)
+#define HALT_CHECK_MAX_LOOPS200
+#define QDSP6SS_XO_CBCR 0x0038

Indent these with tabs, like the others.

Ok.



+#define QDSP6SS_ACC_OVERRIDE_VAL   0x20
+
  struct reg_info {
struct regulator *reg;
int uV;
@@ -111,6 +123,7 @@ struct rproc_hexagon_res {
struct qcom_mss_reg_res active_supply[2];
char **proxy_clk_string;
char **active_clk_string;
+   int hexagon_ver;
  };
  
  struct q6v5 {

@@ -152,8 +165,14 @@ struct q6v5 {
phys_addr_t mpss_reloc;
void *mpss_region;
size_t mpss_size;
+   int hexagon_ver;

Please name this "version" instead, there's little confusion to what it
is.

OK.

  };
  
+enum {

+   MSS_MSM8916, /*hexagon on msm8916*/
+   MSS_MSM8974, /*hexagon on msm8974*/
+   MSS_MSM8996, /*hexagon on msm8996*/

You can skip the comments now.

OK.



+};
  
  static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,

const struct qcom_mss_reg_res *reg_res)
@@ -341,35 +360,107 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 
status, int ms)
  
  static int q6v5proc_reset(struct q6v5 *qproc)

  {
-   u32 val;
+   u64 val;
int ret;
+   int i;
  
-	/* Assert resets, stop core */

-   val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
-   val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
-   writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
  
-	/* Enable power block headswitch, and wait for it to stabilize */

-   val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-   val |= QDSS_BHS_ON | QDSS_LDO_BYP;
-   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-   udelay(1);
-
-   /*
-* Turn on memories. L2 banks should be done individually
-* to minimize inrush current.
-*/
-   val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-   val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
-   Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
-   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-   val |= Q6SS_L2DATA_SLP_NRET_N_2;
-   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-   val |= Q6SS_L2DATA_SLP_NRET_N_1;
-   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-   val |= Q6SS_L2DATA_SLP_NRET_N_0;
-   writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   if (qproc->hexagon_ver == 0x2) {

== MSS_MSM8996

OK.

+   /* Override the ACC value if required */
+   writel(QDSP6SS_ACC_OVERRIDE_VAL,
+   qproc->reg_base + QDSP6SS_STRAP_ACC);
+
+   /* Assert resets, stop core */
+   val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
+   val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
+   writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
  
+		/* BHS require xo cbcr to be enabled */

+   val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
+   val |= 0x1;
+   writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);

Please add a comment here, describing what we're waiting for (rather
than just "a magical bit 31")
In off condition bit 31 (CLKOFF) bit is set as 1, when we write 0x1 to 
this register and clock is turned on it become 0x0.

will add appropriate comment.



+   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;
+   }
+   if ((val & BIT(31))) {

If bit 31 is set when the readl_poll_timeout() hits the timeout the
condition will evaluate to false and "ret" will be -ETIMEDOUT. So I
don't think this can happen.

Sure this is not required. Will remove it

+   dev_err(qproc->dev,
+   "Failed to enable xo branch clock.\n");
+   return -EINVAL;
+   }
+   /*
+* Enable power block headswitch,
+* and wait for it to stabilize

This comment fits within 80 characters, so no need to line wrap it.
OK, i had wrapped it because i was getting warning on running 
checkpatch.pl, if fit in one line without warning will turn it into one 
line comment.

+*/
+   val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+   val |= QDSP6v56_BHS_ON;
+   writel(val, qproc->reg_base +