Add memory ownership switch support and enable mss rproc on msm8996
v4 cover Add memory ownership switch support and enable mss rproc on msm8996
This patch does following
1- Adds new scm call which helps in stage two translation of a memory region
so that memory ownership sharing and switching can be achieved on armv8 and later.
2- Enable mss remoteproc on msm8996
Major changes since last patch:
1- Refactored SCM API
2- Changed SCM API Signature
3- Added cpu_to_le32() as and where required
4- Elaborated documentation of new API added
5- Refactored MSS rproc code which make the scm api call
6- Refactored mss rproc code to first load all segments and then transfer ownership of
memory region to MSS for authentication and boot etc.
7- Other Minor changes
8- kbuild robot gave error so added dummy definition of __qcom_scm_assign_mem() in qcom_scm-32.c as well
Avaneesh Kumar Dwivedi (4):
firmware: scm: Add new SCM call for switching memory ownership
remoteproc: qcom: refactor mss fw image loading sequence
remoteproc: qcom: Make secure world call for mem ownership switch
remoteproc: qcom: Add support for mss boot on msm8996
.../devicetree/bindings/remoteproc/qcom,q6v5.txt | 4 +- drivers/firmware/qcom_scm-32.c | 6 + drivers/firmware/qcom_scm-64.c | 27 ++ drivers/firmware/qcom_scm.c | 75 ++++++ drivers/firmware/qcom_scm.h | 4 + drivers/remoteproc/qcom_q6v5_pil.c | 280 ++++++++++++++++++--- include/linux/qcom_scm.h | 14 ++ 7 files changed, 370 insertions(+), 40 deletions(-)
Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH RESEND v4 1/4 firmware: scm: Add new SCM call for switching memory ownership
Two different processors on a SOC need to switch memory ownership during load/unload. To enable this, level second memory map table need to be updated, which is done by secure layer.
This patch add the interface for making secure monitor call for memory ownership switching request.
--- drivers/firmware/qcom_scm-32.c | 6 ++++ drivers/firmware/qcom_scm-64.c | 27 +++++++++++++++ drivers/firmware/qcom_scm.c | 75 ++++++++++++++++++++++++++++++++++++++++++ drivers/firmware/qcom_scm.h | 4 +++ include/linux/qcom_scm.h | 14 ++++++++ 5 files changed, 126 insertions(+)
drivers/firmware/qcom_scm-32.c +6 -0
@@ -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 ; +}
drivers/firmware/qcom_scm-64.c +27 -0
@@ -439,3 +439,30 @@ int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size,return ret; } + +int __qcom_scm_assign_mem (struct device *dev,phys_addr_t mem_addr, +size_t mem_sz,phys_addr_t srcVm,size_t srcVm_sz, +phys_addr_t destVm,size_t destVm_sz) +{ +int ret; +struct qcom_scm_desc desc = {0 }; +struct arm_smccc_res res; + + desc.args[0 ] = mem_addr; + desc.args[1 ] = mem_sz; + desc.args[2 ] = srcVm; + desc.args[3 ] = srcVm_sz; + desc.args[4 ] = destVm; + desc.args[5 ] = destVm_sz; + desc.args[6 ] =0 ; + + desc.arginfo =QCOM_SCM_ARGS (7 ,QCOM_SCM_RO ,QCOM_SCM_VAL , +QCOM_SCM_RO ,QCOM_SCM_VAL ,QCOM_SCM_RO , +QCOM_SCM_VAL ,QCOM_SCM_VAL ); + + ret =qcom_scm_call (dev,QCOM_SCM_SVC_MP , +QCOM_MEM_PROT_ASSIGN_ID , + &desc, &res); + +return ret ? : res.a1; +}
drivers/firmware/qcom_scm.c +75 -0
87 lines not on this page · raw message
drivers/firmware/qcom_scm.h +4 -0
@@ -95,5 +95,9 @@ extern int __qcom_scm_iommu_secure_ptbl_size(struct device *dev, u32 spare, size_t *size);extern int __qcom_scm_iommu_secure_ptbl_init (struct device *dev,u64 addr,u32 size,u32 spare); +#define QCOM_MEM_PROT_ASSIGN_ID 0x16 +extern int __qcom_scm_assign_mem (struct device *dev,phys_addr_t mem_addr, +size_t mem_sz,phys_addr_t srcVm,size_t srcVm_sz, +phys_addr_t destVm,size_t destVm_sz);#endif
include/linux/qcom_scm.h +14 -0
@@ -23,6 +23,18 @@ struct qcom_scm_hdcp_req {u32 val; }; +struct qcom_scm_destVmPerm { +int destVm; +int destVmPerm; +}; + +#define QCOM_SCM_VMID_HLOS 0x3 +#define QCOM_SCM_VMID_MSS_MSA 0xF +#define QCOM_SCM_PERM_READ 0x4 +#define QCOM_SCM_PERM_WRITE 0x2 +#define QCOM_SCM_PERM_EXEC 0x1 +#define QCOM_SCM_PERM_RW (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE) +#if IS_ENABLED (CONFIG_QCOM_SCM )extern int qcom_scm_set_cold_boot_addr (void *entry,const cpumask_t *cpus);extern int qcom_scm_set_warm_boot_addr (void *entry,const cpumask_t *cpus);
@@ -37,6 +49,8 @@ extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size);extern int qcom_scm_pas_auth_and_reset (u32 peripheral);extern int qcom_scm_pas_shutdown (u32 peripheral); +extern int qcom_scm_assign_mem (phys_addr_t mem_addr,size_t mem_sz,int currVm, +struct qcom_scm_destVmPerm *newVm,size_t newVm_sz);extern void qcom_scm_cpu_power_down (u32 flags);extern u32 qcom_scm_get_version (void );extern int qcom_scm_set_remote_state (u32 state,u32 id);
Signature · 2 lines
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
drivers/firmware/qcom_scm-32.c int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size, · line 603
Quoted 13 lines · Avaneesh Kumar Dwivedi
drivers/firmware/qcom_scm-32.c +5
-0@@ -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.
Quoted 1 line
drivers/firmware/qcom_scm-32.c +1
-0+}
[..]
drivers/firmware/qcom_scm.c struct qcom_scm { · line 44
Quoted 10 lines
drivers/firmware/qcom_scm.c +2
-0@@ -40,6 +40,24 @@ struct qcom_scm {struct reset_controller_dev reset; }; +struct qcom_scm_current_perm_info { +__le32 destVm;
__le32 vmid;
drivers/firmware/qcom_scm.c struct qcom_scm { · line 45
Quoted 1 line
drivers/firmware/qcom_scm.c +1
-0+__le32 destVmPerm;
__le32 perm;
Quoted 4 lines
drivers/firmware/qcom_scm.c +4
-0+__le64 ctx; +__le32 ctx_size; +}; +
drivers/firmware/qcom_scm.c struct qcom_scm { · line 59
Quoted 10 lines
drivers/firmware/qcom_scm.c +10
-0+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.
Quoted 5 lines
drivers/firmware/qcom_scm.c +1
-0+static struct qcom_scm *__scm;static int qcom_scm_clk_enable (void )
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 319
Quoted 10 lines
drivers/firmware/qcom_scm.c +7
-0@@ -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.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 325
Quoted 6 lines
drivers/firmware/qcom_scm.c +6
-0+* @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.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 326
Quoted 1 line
drivers/firmware/qcom_scm.c +1
-0+* Return 0 on success.
For more info on the format, please see:
https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 329
Quoted 3 lines
drivers/firmware/qcom_scm.c +3
-0+*/ +int qcom_scm_assign_mem (phys_addr_t mem_addr,size_t mem_sz,int srcVm, +struct qcom_scm_destVmPerm *newVm,size_t newVm_sz)
It's more idiomatic to pass the number of elements in an array than the size of the array, so make newVm_sz be the number of items.
And please refrain from using camelCase in the Linux kernel.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 332
Quoted 3 lines
drivers/firmware/qcom_scm.c +3
-0+{ +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).
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 334
Quoted 2 lines
drivers/firmware/qcom_scm.c +2
-0+phys_addr_t addr[3 ]; +size_t size[3 ];
Please give these 6 variables names.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 339
Quoted 5 lines
drivers/firmware/qcom_scm.c +5
-0+int ret; +int i; + + hmi =dma_alloc_attrs (__scm->dev,sizeof (*hmi), + &addr[1 ],GFP_KERNEL , dma_attrs);
This function should handle arbitrary number of src and destination vms; so start by calculating the hweight of the source, allocate the appropriate amount of srcVM and dstVM space and then calculate the offsets within that block for each entry.
Check and handle !hmi
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 348
Quoted 9 lines
drivers/firmware/qcom_scm.c +9
-0+ 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
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 349
Quoted 1 line
drivers/firmware/qcom_scm.c +1
-0+ srcVm ^=1 << (ffs (srcVm) -0x1 );
Make this easier to read with:
for (...) {
vmid = ffs(srcVm) - 1;
hmi->srcVm[i] = cpu_to_le32(vmid);
srcVm &= ~BIT(vmid);
}Quoted 7 lines
drivers/firmware/qcom_scm.c +7
-0+ } + size[1 ] = ret *sizeof (srcVm); + + ret = newVm_sz/sizeof (struct qcom_scm_destVmPerm ); +for (i =0 ; i < ret; i++) { + hmi->destVm[i].destVm =cpu_to_le32 (newVm[i].destVm); + hmi->destVm[i].destVmPerm =cpu_to_le32 (newVm[i].destVmPerm);
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 366
Quoted 10 lines
drivers/firmware/qcom_scm.c +10
-0+ hmi->destVm[i].ctx =0 ; + hmi->destVm[i].ctx_size =0 ; + } + addr[2 ] = addr[0 ] +sizeof (hmi->mem_region); + size[2 ] = ret *sizeof (struct qcom_scm_current_perm_info ); + + ret =__qcom_scm_assign_mem (__scm->dev, addr[0 ], + size[0 ], addr[1 ], size[1 ], addr[2 ], size[2 ]); +dma_free_attrs (__scm->dev,sizeof (*hmi), hmi, addr[1 ], dma_attrs); +return ret;
As discussed on IRC, please return negative on failure and otherwise a bitmap built from the vmids in the destination list.
Also note that when I tested this last time I saw __qcom_scm_assign_mem() return positive numbers even though the mapping seemed broken afterwards. So it could be that you should treat any return value as some sort of error.
Quoted 2 lines
drivers/firmware/qcom_scm.c +2
-0+} +EXPORT_SYMBOL (qcom_scm_assign_mem);
[..]
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 26
Quoted 9 lines
include/linux/qcom_scm.h +1
-0@@ -23,6 +23,18 @@ struct qcom_scm_hdcp_req {u32 val; }; +struct qcom_scm_destVmPerm {
There's no harm in dropping "dest" from this, and please drop the camelCase.
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 27
Quoted 1 line
include/linux/qcom_scm.h +1
-0+int destVm;
int vmid;
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 28
Quoted 1 line
include/linux/qcom_scm.h +1
-0+int destVmPerm;
int perm;
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 36
Quoted 8 lines
include/linux/qcom_scm.h +8
-0+}; + +#define QCOM_SCM_VMID_HLOS 0x3 +#define QCOM_SCM_VMID_MSS_MSA 0xF +#define QCOM_SCM_PERM_READ 0x4 +#define QCOM_SCM_PERM_WRITE 0x2 +#define QCOM_SCM_PERM_EXEC 0x1 +#define QCOM_SCM_PERM_RW (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)
Add QCOM_SCM_PERM_RWX, as this looks fairly common as well.
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 37
Quoted 1 line
include/linux/qcom_scm.h +1
-0+
Regards,
Bjorn
Re: [RESEND: PATCH v4 1/4] firmware: scm: Add new SCM call for switching memory ownership
Quoted 16 lines · Bjorn Andersson
drivers/firmware/qcom_scm-32.c int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size, · line 603
Quoted 13 lines · Avaneesh Kumar Dwivedi
drivers/firmware/qcom_scm-32.c +5
-0@@ -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.
Quoted 11 lines
Quoted 2 lines
drivers/firmware/qcom_scm-32.c +1
-0+}[..]
Quoted 8 lines
drivers/firmware/qcom_scm.c +0
-0@@ -40,6 +40,24 @@ struct qcom_scm {struct reset_controller_dev reset; };
Quoted 3 lines
drivers/firmware/qcom_scm.c struct qcom_scm { · line 44
Quoted 2 lines
drivers/firmware/qcom_scm.c +2
-0+struct qcom_scm_current_perm_info { +__le32 destVm;
__le32vmid;
Ok
Quoted 3 lines
drivers/firmware/qcom_scm.c struct qcom_scm { · line 45
Quoted 2 lines
drivers/firmware/qcom_scm.c +1
-0+__le32 destVmPerm;
__le32perm;
OK
Quoted 10 lines
Quoted 10 lines
drivers/firmware/qcom_scm.c +9
-0+__le64 ctx; +__le32 ctx_size; +}; + +struct qcom_scm_mem_map_info { +__le64 mem_addr; +__le64 mem_size; +}; +
Quoted 8 lines
drivers/firmware/qcom_scm.c struct qcom_scm { · line 59
Quoted 5 lines
drivers/firmware/qcom_scm.c +5
-0+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.
Quoted 9 lines
Quoted 9 lines
drivers/firmware/qcom_scm.c +1
-0+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);
Quoted 8 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 319
Quoted 7 lines
drivers/firmware/qcom_scm.c +7
-0+/** +* 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.
Quoted 9 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 325
Quoted 7 lines
drivers/firmware/qcom_scm.c +6
-0+* @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.
Quoted 11 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 326
Quoted 2 lines
drivers/firmware/qcom_scm.c +1
-0+* Return 0 on success. For more info on the format, please see:
https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 329
Quoted 3 lines
drivers/firmware/qcom_scm.c +3
-0+*/ +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_szbe the number of items.'
OK, will try to incorporate and if see any issue with it, will revert back.
Quoted 2 lines
And please refrain from using camelCase in the Linux kernel.
OK.
Quoted 6 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 332
Quoted 4 lines
drivers/firmware/qcom_scm.c +3
-0+{ +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.
Quoted 4 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 334
Quoted 3 lines
drivers/firmware/qcom_scm.c +2
-0+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.
Quoted 10 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 339
Quoted 6 lines
drivers/firmware/qcom_scm.c +5
-0+int ret; +int i; + + hmi =dma_alloc_attrs (__scm->dev,sizeof (*hmi), + &addr[1 ],GFP_KERNEL , dma_attrs);This function should handle arbitrary number of src and destination vms; so start by calculating the hweight of the source, allocate the appropriate amount of srcVM and dstVM space and then calculate the offsets within that block for each entry.
OK. as mentioned above will make this api independent of number of entries in current and next owner set and allocate memory after based on destination and source counts.
Quoted 3 lines
Check and handle !hmi
OK
Quoted 11 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 348
Quoted 10 lines
drivers/firmware/qcom_scm.c +9
-0+ 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.
Quoted 3 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 349
Quoted 2 lines
drivers/firmware/qcom_scm.c +1
-0+ srcVm ^=1 << (ffs (srcVm) -0x1 );Make this easier to read with:
ok.
Quoted 19 lines
for (...) { vmid = ffs(srcVm) - 1; hmi->srcVm[i] = cpu_to_le32(vmid); srcVm &= ~BIT(vmid); }Quoted 11 lines
drivers/firmware/qcom_scm.c +11
-0+ } + size[1 ] = ret *sizeof (srcVm); + + ret = newVm_sz/sizeof (struct qcom_scm_destVmPerm ); +for (i =0 ; i < ret; i++) { + hmi->destVm[i].destVm =cpu_to_le32 (newVm[i].destVm); + hmi->destVm[i].destVmPerm =cpu_to_le32 (newVm[i].destVmPerm); + hmi->destVm[i].ctx =0 ; + hmi->destVm[i].ctx_size =0 ; + } + addr[2 ] = addr[0 ] +sizeof (hmi->mem_region);
Quoted 8 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 366
Quoted 6 lines
drivers/firmware/qcom_scm.c +6
-0+ size[2 ] = ret *sizeof (struct qcom_scm_current_perm_info ); + + ret =__qcom_scm_assign_mem (__scm->dev, addr[0 ], + size[0 ], addr[1 ], size[1 ], addr[2 ], size[2 ]); +dma_free_attrs (__scm->dev,sizeof (*hmi), hmi, addr[1 ], dma_attrs); +return ret;As discussed on IRC, please return negative on failure and otherwise a bitmap built from the vmids in the destination list.
OK. will add appropriate code in caller module to handle returned bitmap list as new owners for each image.
Quoted 5 lines
Also note that when I tested this last time I saw
__qcom_scm_assign_mem()return positive numbers even though the mapping seemed broken afterwards. So it could be that you should treat any return value as some sort of error.
will check this while verifying. and incorporate if anything is missing.
Quoted 12 lines
Quoted 3 lines
drivers/firmware/qcom_scm.c +2
-0+} +EXPORT_SYMBOL (qcom_scm_assign_mem);[..]
Quoted 8 lines
include/linux/qcom_scm.h +0
-0@@ -23,6 +23,18 @@ struct qcom_scm_hdcp_req {u32 val; };
Quoted 3 lines
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 26
Quoted 1 line
include/linux/qcom_scm.h +1
-0+struct qcom_scm_destVmPerm {There's no harm in dropping "dest" from this, and please drop the camelCase.
OK.
Quoted 3 lines
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 27
Quoted 2 lines
include/linux/qcom_scm.h +1
-0+int destVm;int vmid;
OK
Quoted 3 lines
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 28
Quoted 2 lines
include/linux/qcom_scm.h +1
-0+int destVmPerm;int perm;
OK.
Quoted 9 lines
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 36
Quoted 8 lines
include/linux/qcom_scm.h +8
-0+}; + +#define QCOM_SCM_VMID_HLOS 0x3 +#define QCOM_SCM_VMID_MSS_MSA 0xF +#define QCOM_SCM_PERM_READ 0x4 +#define QCOM_SCM_PERM_WRITE 0x2 +#define QCOM_SCM_PERM_EXEC 0x1 +#define QCOM_SCM_PERM_RW (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)Add
QCOM_SCM_PERM_RWX, as this looks fairly common as well.
You mean change name as QCOM_SCM_PERM_RWX instead of QCOM_SCM_PERM_RW ? OK.
Quoted 4 lines
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 37
Quoted 2 lines
include/linux/qcom_scm.h +1
-0+Regards,
Bjorn
Signature · 2 lines
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
Quoted 2 lines · Avaneesh Kumar (avani)
Quoted 1 line · Bjorn Andersson
On Tue 16 May 11:01 PDT 2017, Avaneesh Kumar Dwivedi wrote:
[..]
Quoted 1 line
Quoted 1 line
Quoted 1 line
include/linux/qcom_scm.h +0
-0
[..]
Quoted 8 lines
Quoted 7 lines
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 36
Quoted 6 lines
include/linux/qcom_scm.h +6
-0+#define QCOM_SCM_VMID_HLOS 0x3 +#define QCOM_SCM_VMID_MSS_MSA 0xF +#define QCOM_SCM_PERM_READ 0x4 +#define QCOM_SCM_PERM_WRITE 0x2 +#define QCOM_SCM_PERM_EXEC 0x1 +#define QCOM_SCM_PERM_RW (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)Add
QCOM_SCM_PERM_RWX, as this looks fairly common as well.You mean change name as
QCOM_SCM_PERM_RWXinstead ofQCOM_SCM_PERM_RW? OK.
Provide both RW and RWX, as already in the first client of this API you're using both.
Thanks,
Bjorn
PATCH RESEND v4 2/4 remoteproc: qcom: refactor mss fw image loading sequence
In reply to Avaneesh Kumar DwivediThis 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.
--- drivers/remoteproc/qcom_q6v5_pil.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)
drivers/remoteproc/qcom_q6v5_pil.c +12 -13
@@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) ret =q6v5_rmb_mba_wait (qproc,RMB_MBA_META_DATA_AUTH_SUCCESS ,1000 );if (ret == -ETIMEDOUT ) -dev_err (qproc->dev,"MPSS header authentication timed out\n" ); +dev_err (qproc->dev,"metadata authentication timed out\n" );else if (ret <0 ) -dev_err (qproc->dev,"MPSS header authentication failed: %d\n" , ret); +dev_err (qproc->dev, +"metadata authentication failed: %d\n" , ret);dma_free_attrs (qproc->dev, fw->size, ptr, phys, dma_attrs);
@@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)bool relocate = false;char seg_name[10 ];ssize_t offset; -size_t size; +size_t size =0 ;void *ptr;int ret;int i;
@@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc) } mpss_reloc = relocate ? min_addr : qproc->mpss_phys; - +/* Load firmware completely before letting mss to start +* authentication and then boot firmware +*/ for (i =0 ; i < ehdr->e_phnum; i++) { phdr = &phdrs[i];
@@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)memset (ptr + phdr->p_filesz,0 , phdr->p_memsz - phdr->p_filesz); } - - size =readl (qproc->rmb_base +RMB_PMI_CODE_LENGTH_REG ); -if (!size) { - boot_addr = relocate ? qproc->mpss_phys : min_addr; -writel (boot_addr, qproc->rmb_base +RMB_PMI_CODE_START_REG ); -writel (RMB_CMD_LOAD_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG ); - } - size += phdr->p_memsz; -writel (size, qproc->rmb_base +RMB_PMI_CODE_LENGTH_REG ); } +/* Transfer ownership of modem region with modem fw */ + boot_addr = relocate ? qproc->mpss_phys : min_addr; +writel (boot_addr, qproc->rmb_base +RMB_PMI_CODE_START_REG ); +writel (RMB_CMD_LOAD_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG ); +writel (size, qproc->rmb_base +RMB_PMI_CODE_LENGTH_REG ); ret =q6v5_rmb_mba_wait (qproc,RMB_MBA_AUTH_COMPLETE ,10000 );if (ret == -ETIMEDOUT )
Signature · 2 lines
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
Hi Bjorn/Avaneesh,
Quoted 51 lines · Avaneesh Kumar Dwivedi
This patch refactor code to first load all firmware blobs and then update modem proc to authenticate and boot fw. Also make a trivial change in a error log.
Signed-off-by Avaneesh Kumar Dwivedi <[email protected]>--- drivers/remoteproc/qcom_q6v5_pil.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)drivers/remoteproc/qcom_q6v5_pil.c +7
-5@@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) ret =q6v5_rmb_mba_wait (qproc,RMB_MBA_META_DATA_AUTH_SUCCESS ,1000 );if (ret == -ETIMEDOUT ) -dev_err (qproc->dev,"MPSS header authentication timed out\n" ); +dev_err (qproc->dev,"metadata authentication timed out\n" );else if (ret <0 ) -dev_err (qproc->dev,"MPSS header authentication failed: %d\n" , ret); +dev_err (qproc->dev, +"metadata authentication failed: %d\n" , ret);dma_free_attrs (qproc->dev, fw->size, ptr, phys, dma_attrs);@@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)bool relocate = false;char seg_name[10 ];ssize_t offset; -size_t size; +size_t size =0 ;void *ptr;int ret;int i;@@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc) } mpss_reloc = relocate ? min_addr : qproc->mpss_phys; - +/* Load firmware completely before letting mss to start +* authentication and then boot firmware +*/ for (i =0 ; i < ehdr->e_phnum; i++) { phdr = &phdrs[i];@@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)memset (ptr + phdr->p_filesz,0 , phdr->p_memsz - phdr->p_filesz); } -
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 581
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-8- 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 ?
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 586
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+/* Transfer ownership of modem region with modem fw */ + boot_addr = relocate ? qproc->mpss_phys : min_addr; +writel (boot_addr, qproc->rmb_base +RMB_PMI_CODE_START_REG ); +writel (RMB_CMD_LOAD_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG ); +writel (size, qproc->rmb_base +RMB_PMI_CODE_LENGTH_REG );
For ipq8074 [1], wcnss core has an Q6V5 version of the ip for which the initialization/boot sequence is pretty much the same as that has been added for msm8996 in this series. So wanted to understand if its better to use this remoteproc itself by keeping the Q6 and mpss parts separately (or) add a new remoteproc ?
[1] https://patchwork.kernel.org/patch/9711725/
Regards,
Sricharan
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0ret =q6v5_rmb_mba_wait (qproc,RMB_MBA_AUTH_COMPLETE ,10000 );if (ret == -ETIMEDOUT )
Signature · 1 line
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence
Quoted 58 lines · Sricharan R
Hi Bjorn/Avaneesh,
Quoted 55 lines · Avaneesh Kumar Dwivedi
This patch refactor code to first load all firmware blobs and then update modem proc to authenticate and boot fw. Also make a trivial change in a error log.
Signed-off-by Avaneesh Kumar Dwivedi <[email protected]>--- drivers/remoteproc/qcom_q6v5_pil.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)drivers/remoteproc/qcom_q6v5_pil.c +7
-9@@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) ret =q6v5_rmb_mba_wait (qproc,RMB_MBA_META_DATA_AUTH_SUCCESS ,1000 );if (ret == -ETIMEDOUT ) -dev_err (qproc->dev,"MPSS header authentication timed out\n" ); +dev_err (qproc->dev,"metadata authentication timed out\n" );else if (ret <0 ) -dev_err (qproc->dev,"MPSS header authentication failed: %d\n" , ret); +dev_err (qproc->dev, +"metadata authentication failed: %d\n" , ret);dma_free_attrs (qproc->dev, fw->size, ptr, phys, dma_attrs);@@ -503,7 +504,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)bool relocate = false;char seg_name[10 ];ssize_t offset; -size_t size; +size_t size =0 ;void *ptr;int ret;int i;@@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc) } mpss_reloc = relocate ? min_addr : qproc->mpss_phys; - +/* Load firmware completely before letting mss to start +* authentication and then boot firmware +*/ for (i =0 ; i < ehdr->e_phnum; i++) { phdr = &phdrs[i];@@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)memset (ptr + phdr->p_filesz,0 , phdr->p_memsz - phdr->p_filesz); } - - size =readl (qproc->rmb_base +RMB_PMI_CODE_LENGTH_REG ); -if (!size) { - boot_addr = relocate ? qproc->mpss_phys : min_addr; -writel (boot_addr, qproc->rmb_base +RMB_PMI_CODE_START_REG );
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 581
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-4-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_loadinstead 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.
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 586
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+/* 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.
Quoted 10 lines
[1] https://patchwork.kernel.org/patch/9711725/
Regards,
SricharanQuoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0ret =q6v5_rmb_mba_wait (qproc,RMB_MBA_AUTH_COMPLETE ,10000 );if (ret == -ETIMEDOUT )
Signature · 2 lines
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
Hi,
Quoted 64 lines · Avaneesh Kumar (avani)
Quoted 53 lines · Sricharan R
Hi Bjorn/Avaneesh,
Quoted 50 lines · Avaneesh Kumar Dwivedi
This patch refactor code to first load all firmware blobs and then update modem proc to authenticate and boot fw. Also make a trivial change in a error log.
Signed-off-by Avaneesh Kumar Dwivedi <[email protected]>--- drivers/remoteproc/qcom_q6v5_pil.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)drivers/remoteproc/qcom_q6v5_pil.c +3
-2@@ -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;drivers/remoteproc/qcom_q6v5_pil.c +4
-2-size_t size; +size_t size =0 ;void *ptr;int ret;int i;@@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc) } mpss_reloc = relocate ? min_addr : qproc->mpss_phys; - +/* Load firmware completely before letting mss to start +* authentication and then boot firmware +*/ for (i =0 ; i < ehdr->e_phnum; i++) { phdr = &phdrs[i]; @@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc) memset(ptr + phdr->p_filesz, 0,phdr->p_memsz - phdr->p_filesz); }drivers/remoteproc/qcom_q6v5_pil.c +0
-5- - 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 );Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 581
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-4-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_loadinstead for the mpss image loading part above ?
Quoted 3 lines
qcom_mdt_load()can not be used to load segments for mpss, as MPSS blobs are self authenticated. whileqcom_mdt_load()is used in cases where authentication of loaded blobs is done by trustzone. for thatqcom_mdt_load()does extra steps to sendpas_idto trustzone andmem_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 ?
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+/* 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 );
Quoted 7 lines
Quoted 5 lines
For ipq8074 [1], wcnss core has an Q6V5 version of the ip for which the initialization/boot sequence is pretty much the same as that has been added for msm8996 in this series. So wanted to understand if its better to use this remoteproc itself by keeping the Q6 and mpss parts separately (or) add a new remoteproc ?
Bjorn can better answer this query, but i believe this remoteproc can be extended to load mpss part by adding private initialization for the IP.
ya, the mpss part can be separated out, so that this can be a Q6 + MPSS (or) Q6 + WCNSS remoteproc. Was asking this to get the right way for adding the Q6 + WCNSS remoteproc, as the Q6 part is same what you have added for msm8996.
Regards,
Sricharan
Quoted 10 lines
Quoted 9 lines
[1] https://patchwork.kernel.org/patch/9711725/
Regards,
SricharanQuoted 3 lines
ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000); if (ret == -ETIMEDOUT)
Signature · 1 line
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence
Quoted 66 lines · Sricharan R
Hi,
Quoted 63 lines · Avaneesh Kumar (avani)
Quoted 61 lines · Sricharan R
Hi Bjorn/Avaneesh,
Quoted 46 lines · Avaneesh Kumar Dwivedi
This patch refactor code to first load all firmware blobs and then update modem proc to authenticate and boot fw. Also make a trivial change in a error log.
Signed-off-by Avaneesh Kumar Dwivedi <[email protected]>--- drivers/remoteproc/qcom_q6v5_pil.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)drivers/remoteproc/qcom_q6v5_pil.c +3
-2@@ -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;drivers/remoteproc/qcom_q6v5_pil.c +4
-2-size_t size; +size_t size =0 ;void *ptr;int ret;int i;@@ -541,7 +542,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc) } mpss_reloc = relocate ? min_addr : qproc->mpss_phys; - +/* Load firmware completely before letting mss to start +* authentication and then boot firmware +*/ for (i =0 ; i < ehdr->e_phnum; i++) { phdr = &phdrs[i]; @@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc) memset(ptr + phdr->p_filesz, 0,phdr->p_memsz - phdr->p_filesz); }drivers/remoteproc/qcom_q6v5_pil.c +0
-1-drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 581
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-8- 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_loadinstead for the mpss image loading part above ?
Quoted 6 lines
Quoted 3 lines
qcom_mdt_load()can not be used to load segments for mpss, as MPSS blobs are self authenticated. whileqcom_mdt_load()is used in cases where authentication of loaded blobs is done by trustzone. for thatqcom_mdt_load()does extra steps to sendpas_idto trustzone andmem_setup()etc.Right, so my intention of asking this was if the code which does the calculation and loads the segments in
qcom_mdt_loadcan 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.
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 586
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+/* 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 ?
Quoted 5 lines
Quoted 2 lines
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.
Quoted 12 lines
Regards,
SricharanQuoted 8 lines
Quoted 8 lines
[1] https://patchwork.kernel.org/patch/9711725/
Regards,
SricharanQuoted 3 lines
ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000); if (ret == -ETIMEDOUT)
Signature · 2 lines
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
Quoted 9 lines · Avaneesh Kumar (avani)
Quoted 8 lines · Sricharan R
Hi,
Quoted 5 lines · Avaneesh Kumar (avani)
Quoted 3 lines · Sricharan R
Hi Bjorn/Avaneesh,
On 5/16/2017 11:32 PM, Avaneesh Kumar Dwivedi wrote:
[..]
Quoted 22 lines
Quoted 18 lines
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-5- - 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 );Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 581
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-4-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_loadinstead for the mpss image loading part above ?Quoted 5 lines
qcom_mdt_load()can not be used to load segments for mpss, as MPSS blobs are self authenticated. whileqcom_mdt_load()is used in cases where authentication of loaded blobs is done by trustzone. for thatqcom_mdt_load()does extra steps to sendpas_idto trustzone andmem_setup()etc.Quoted 4 lines
Right, so my intention of asking this was if the code which does the calculation and loads the segments in
qcom_mdt_loadcan somehow be abstracted out, so that future self authenticating rproc (even mpss in this case) can use them to load mdt ?
Quoted 7 lines
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 theqcom_mdt_load()equivalent routine has not been used. Bjorn may further add in this.
I have not been able to come up with a clean way to provide a useful mdt-loader abstraction that works for the SCM PILs, the self-authenticated PILs and the non-PIL SCM users.
Further more with the upcoming ramdump support we will need to extract segment information from the mdt header, so we will have to revisit this topic.
Regardless, I would prefer that we follow up with such refactoring after getting this series sorted out.
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 586
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+/* 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 ?
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
Bjorn can better answer this query, but i believe this remoteproc can be extended to load mpss part by adding private initialization for the IP.
ya, the mpss part can be separated out, so that this can be a Q6 + MPSS (or) Q6 + WCNSS remoteproc. Was asking this to get the right way for adding the Q6 + WCNSS remoteproc, as the Q6 part is same what you have added for msm8996.
Again, i believe yes but leave Bjorn to make final comment.
It definitely sounds like there's room for reuse here, how much of the initialization and authentication sequences are common between the two?
Regards,
Bjorn
Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence
Hi Bjorn,
Quoted 45 lines · Bjorn Andersson
Quoted 9 lines · Avaneesh Kumar (avani)
Quoted 8 lines · Sricharan R
Hi,
Quoted 5 lines · Avaneesh Kumar (avani)
Quoted 3 lines · Sricharan R
Hi Bjorn/Avaneesh,
On 5/16/2017 11:32 PM, Avaneesh Kumar Dwivedi wrote:
[..]
Quoted 22 lines
Quoted 18 lines
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-5- - 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 );Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 581
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-4-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_loadinstead for the mpss image loading part above ?Quoted 5 lines
qcom_mdt_load()can not be used to load segments for mpss, as MPSS blobs are self authenticated. whileqcom_mdt_load()is used in cases where authentication of loaded blobs is done by trustzone. for thatqcom_mdt_load()does extra steps to sendpas_idto trustzone andmem_setup()etc.Quoted 4 lines
Right, so my intention of asking this was if the code which does the calculation and loads the segments in
qcom_mdt_loadcan somehow be abstracted out, so that future self authenticating rproc (even mpss in this case) can use them to load mdt ?Quoted 7 lines
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 theqcom_mdt_load()equivalent routine has not been used. Bjorn may further add in this.I have not been able to come up with a clean way to provide a useful mdt-loader abstraction that works for the SCM PILs, the self-authenticated PILs and the non-PIL SCM users.
Quoted 8 lines
Further more with the upcoming ramdump support we will need to extract segment information from the mdt header, so we will have to revisit this topic.
Regardless, I would prefer that we follow up with such refactoring after getting this series sorted out.
ok, agree. While trying to add Q6 support for ipq8074, which is again a self-authenticating PIL with mdt, i can try this.
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 586
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+/* 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 );Quoted 6 lines
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.
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
ya, the mpss part can be separated out, so that this can be a Q6 + MPSS (or) Q6 + WCNSS remoteproc. Was asking this to get the right way for adding the Q6 + WCNSS remoteproc, as the Q6 part is same what you have added for msm8996.
Again, i believe yes but leave Bjorn to make final comment.
It definitely sounds like there's room for reuse here, how much of the initialization and authentication sequences are common between the two?
The initialization sequence is exactly the same as what was done for msm8996(Q6) one added in this series. So for reusing this driver for Q6, the Q6 + MPSS has to be decoupled and driver has to look common for Q6 + any, (ie) Q6 + mpss (or) Q6 + wcnss. Incase if that's not neat, atleast the Q6 initialization sequence can be reused.
Regards,
Sricharan
Quoted 4 lines
Regards,
Bjorn
Signature · 1 line
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence
In reply to Avaneesh Kumar DwivediQuoted 2 lines · Avaneesh Kumar Dwivedi
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.
Quoted 12 lines
Also make a trivial change in a error log.
Signed-off-by Avaneesh Kumar Dwivedi <[email protected]>--- drivers/remoteproc/qcom_q6v5_pil.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)drivers/remoteproc/qcom_q6v5_pil.c +0
-0
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) · line 472
Quoted 9 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-2@@ -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.
Quoted 13 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-1dma_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;
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 547
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-1@@ -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 */
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-1for (i =0 ; i < ehdr->e_phnum; i++) { phdr = &phdrs[i];@@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)memset (ptr + phdr->p_filesz,0 , phdr->p_memsz - phdr->p_filesz); } -
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 581
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-8- 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".
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 586
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+/* Transfer ownership of modem region with modem fw */ + boot_addr = relocate ? qproc->mpss_phys : min_addr; +writel (boot_addr, qproc->rmb_base +RMB_PMI_CODE_START_REG ); +writel (RMB_CMD_LOAD_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG ); +writel (size, qproc->rmb_base +RMB_PMI_CODE_LENGTH_REG );
I originally did something similar, but ran into some issue - so I will test this on 8974 and 8916 as soon as my devboards are back online.
Regards,
Bjorn
Re: [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence
Quoted 5 lines · Bjorn Andersson
Quoted 2 lines · Avaneesh Kumar Dwivedi
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.
Quoted 14 lines
Quoted 14 lines
Also make a trivial change in a error log.
Signed-off-by Avaneesh Kumar Dwivedi <[email protected]>--- drivers/remoteproc/qcom_q6v5_pil.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)drivers/remoteproc/qcom_q6v5_pil.c +0
-0@@ -466,9 +466,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
Quoted 11 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) · line 472
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-2ret =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.
Quoted 25 lines
Quoted 14 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-1dma_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;drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 547
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-1@@ -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. */
Quoted 7 lines
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.
Quoted 4 lines
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0for (i =0 ; i < ehdr->e_phnum; i++) { phdr = &phdrs[i];
Quoted 16 lines
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-1@@ -574,17 +577,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)memset (ptr + phdr->p_filesz,0 , phdr->p_memsz - phdr->p_filesz); } -drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 581
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-8- 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
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 586
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+/* 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
Quoted 3 lines
Regards,
Bjorn
Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH RESEND v4 3/4 remoteproc: qcom: Make secure world call for mem ownership switch
In reply to Avaneesh Kumar DwivediMSS 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.
--- drivers/remoteproc/qcom_q6v5_pil.c | 84 +++++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 2 deletions(-)
drivers/remoteproc/qcom_q6v5_pil.c +82 -2
@@ -110,6 +110,7 @@ struct rproc_hexagon_res {struct qcom_mss_reg_res *active_supply;char **proxy_clk_names;char **active_clk_names; +bool need_mem_protection; };struct q6v5 {
@@ -151,6 +152,7 @@ struct q6v5 {phys_addr_t mpss_reloc;void *mpss_region;size_t mpss_size; +bool need_mem_protection;struct qcom_rproc_subdev smd_subdev; };
@@ -288,6 +290,43 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,return &table; } +static int q6v5_assign_mem_to_subsys (struct q6v5 *qproc, +phys_addr_t addr,size_t size) +{ +struct qcom_scm_destVmPerm next[] = {{QCOM_SCM_VMID_MSS_MSA , + (QCOM_SCM_PERM_READ |QCOM_SCM_PERM_WRITE )} }; +int ret; + + size =ALIGN (size,SZ_4K ); +if (!qproc->need_mem_protection) +return 0 ; + ret =qcom_scm_assign_mem (addr, size, +BIT (QCOM_SCM_VMID_HLOS ), next,sizeof (next)); +if (ret) +pr_err ("%s: Failed to assign memory access, ret = %d\n" , + __func__, ret); +return ret; +} + +static int q6v5_assign_mem_to_linux (struct q6v5 *qproc, +phys_addr_t addr,size_t size) +{ +struct qcom_scm_destVmPerm next[] = { {QCOM_SCM_VMID_HLOS , + (QCOM_SCM_PERM_READ |QCOM_SCM_PERM_WRITE |QCOM_SCM_PERM_EXEC )} + }; +int ret; + + size =ALIGN (size,SZ_4K ); +if (!qproc->need_mem_protection) +return 0 ; + ret =qcom_scm_assign_mem (addr, size, +BIT (QCOM_SCM_VMID_MSS_MSA ), next,sizeof (next)); +if (ret) +pr_err ("%s: Failed to assign memory access, ret = %d\n" , + __func__, ret); +return ret; +} +static int q6v5_load (struct rproc *rproc,const struct firmware *fw) {struct q6v5 *qproc = rproc->priv;
@@ -461,6 +500,13 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)memcpy (ptr, fw->data, fw->size); +/* Hypervisor mapping to access metadata by modem */ + ret =q6v5_assign_mem_to_subsys (qproc, phys, fw->size); +if (ret) { +dev_err (qproc->dev, +"Failed to assign metadata memory, ret - %d\n" , ret); +return -ENOMEM ; + }writel (phys, qproc->rmb_base +RMB_PMI_META_DATA_REG );writel (RMB_CMD_META_DATA_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG );
@@ -471,6 +517,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)dev_err (qproc->dev,"metadata authentication failed: %d\n" , ret); +/* Metadata authentication done, remove modem access */ + ret =q6v5_assign_mem_to_linux (qproc, phys, fw->size); +if (ret) +dev_err (qproc->dev, +"Failed to reclaim metadata memory, ret - %d\n" , ret);dma_free_attrs (qproc->dev, fw->size, ptr, phys, dma_attrs);return ret <0 ? ret :0 ;
@@ -581,6 +632,10 @@ static int q6v5_mpss_load(struct q6v5 *qproc) }/* Transfer ownership of modem region with modem fw */ boot_addr = relocate ? qproc->mpss_phys : min_addr; + ret =q6v5_assign_mem_to_subsys (qproc, + qproc->mpss_phys, qproc->mpss_size); +if (ret) +return ret;writel (boot_addr, qproc->rmb_base +RMB_PMI_CODE_START_REG );writel (RMB_CMD_LOAD_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG );writel (size, qproc->rmb_base +RMB_PMI_CODE_LENGTH_REG );
@@ -600,6 +655,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)static int q6v5_start (struct rproc *rproc) {struct q6v5 *qproc = (struct q6v5 *)rproc->priv; +int assign_mem_result;int ret; ret = q6v5_regulator_enable(qproc, qproc->proxy_regs,
@@ -635,6 +691,13 @@ static int q6v5_start(struct rproc *rproc) goto assert_reset; } + ret =q6v5_assign_mem_to_subsys (qproc, + qproc->mba_phys, qproc->mba_size); +if (ret) { +dev_err (qproc->dev, +"Failed to assign mba memory access, ret - %d\n" , ret); + goto assert_reset; + }writel (qproc->mba_phys, qproc->rmb_base +RMB_MBA_IMAGE_REG ); ret =q6v5proc_reset (qproc);
@@ -656,16 +719,21 @@ static int q6v5_start(struct rproc *rproc) ret =q6v5_mpss_load (qproc);if (ret) - goto halt_axi_ports; + goto reclaim_mem; ret =wait_for_completion_timeout (&qproc->start_done,msecs_to_jiffies (5000 ));if (ret ==0 ) {dev_err (qproc->dev,"start timed out\n" ); ret = -ETIMEDOUT ; - goto halt_axi_ports; + goto reclaim_mem; } + ret =q6v5_assign_mem_to_linux (qproc, + qproc->mba_phys, qproc->mba_size); +if (ret) +dev_err (qproc->dev, +"Failed to reclaim mba memory, ret - %d\n" , ret); qproc->running = true; q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
@@ -675,12 +743,19 @@ static int q6v5_start(struct rproc *rproc)return 0 ; +reclaim_mem: + assign_mem_result = +q6v5_assign_mem_to_linux (qproc, + qproc->mpss_phys, qproc->mpss_size); halt_axi_ports:q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_q6);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_modem);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_nc);q6v5_clk_disable (qproc->dev, qproc->active_clks, qproc->active_clk_count); + assign_mem_result = +q6v5_assign_mem_to_linux (qproc, + qproc->mba_phys, qproc->mba_size); assert_reset:reset_control_assert (qproc->mss_restart); disable_vdd:
@@ -717,6 +792,8 @@ static int q6v5_stop(struct rproc *rproc)q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_modem);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_nc); + ret =q6v5_assign_mem_to_linux (qproc, + qproc->mpss_phys, qproc->mpss_size);reset_control_assert (qproc->mss_restart);q6v5_clk_disable (qproc->dev, qproc->active_clks, qproc->active_clk_count);
@@ -1014,6 +1091,7 @@ static int q6v5_probe(struct platform_device *pdev)if (ret) goto free_rproc; + qproc->need_mem_protection = desc->need_mem_protection; ret =q6v5_request_irq (qproc, pdev,"wdog" , q6v5_wdog_interrupt);if (ret <0 ) goto free_rproc;
@@ -1089,6 +1167,7 @@ static int q6v5_remove(struct platform_device *pdev)"mem" ,NULL }, + .need_mem_protection = false, }; static const struct rproc_hexagon_res msm8974_mss = {
@@ -1126,6 +1205,7 @@ static int q6v5_remove(struct platform_device *pdev)"mem" ,NULL }, + .need_mem_protection = false, }; static const struct of_device_id q6v5_of_match[] = {
Signature · 2 lines
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
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 297
Quoted 5 lines · Avaneesh Kumar Dwivedi
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+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
};
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 300
Quoted 3 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0+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.
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!qproc->need_mem_protection) + return 0;
Put a blank line here, to give separation between the sections of the function.
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 304
Quoted 2 lines
drivers/remoteproc/qcom_q6v5_pil.c +2
-0+ ret =qcom_scm_assign_mem (addr, size, +BIT (QCOM_SCM_VMID_HLOS ), next,sizeof (next));
qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K), BIT(QCOM_SCM_VMID_HLOS), &next, 1);
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 307
Quoted 3 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0+if (ret) +pr_err ("%s: Failed to assign memory access, ret = %d\n" , + __func__, ret);
There's no point in printing an error here and in the calling function, but as it makes sense to have the error message to include which memory region we operated on (mba vs mpss) I think you should remove the print here and keep it in the callers.
So just return qcom-scm_assign_mem().
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 316
Quoted 9 lines
drivers/remoteproc/qcom_q6v5_pil.c +9
-0+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)
[..]
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) · line 524
Quoted 9 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0@@ -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.
Quoted 3 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0dma_free_attrs (qproc->dev, fw->size, ptr, phys, dma_attrs);return ret <0 ? ret :0 ;
[..]
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 722
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-1@@ -656,16 +719,21 @@ static int q6v5_start(struct rproc *rproc) ret =q6v5_mpss_load (qproc);if (ret) - goto halt_axi_ports; + goto reclaim_mem;
This doesn't allow us to distinguish between failures before or after the memory assignment in mpss_load(), so although you're duplicating the reclaim code, mpss_load() must be responsible of restoring the state on failure.
The timeout below still has to reclaim the memory.
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0ret =wait_for_completion_timeout (&qproc->start_done,msecs_to_jiffies (5000 ));if (ret ==0 ) {dev_err (qproc->dev,"start timed out\n" );
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 736
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +6
-1ret = -ETIMEDOUT ; - goto halt_axi_ports; + goto reclaim_mem; } + ret =q6v5_assign_mem_to_linux (qproc, + qproc->mba_phys, qproc->mba_size); +if (ret) +dev_err (qproc->dev, +"Failed to reclaim mba memory, ret - %d\n" , ret);
I think it's okay for symmetrical purposes to keep the memory assigned to the remote until we call stop().
Although this shows that we should be able to release the mba allocation here.
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 749
Quoted 11 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0qproc->running = true; q6v5_clk_disable(qproc->dev, qproc->proxy_clks,@@ -675,12 +743,19 @@ static int q6v5_start(struct rproc *rproc)return 0 ; +reclaim_mem: + assign_mem_result = +q6v5_assign_mem_to_linux (qproc, + qproc->mpss_phys, qproc->mpss_size);
If this fails we're screwed. We have no way to fail in a way that will not free the memory at any later point in time.
So I do believe you should have a BUG_ON(assign_mem_result); here. With a clear comment in the lines of:
/*
* Failed to reclaim memory, any future access to these pages will cause
* security violations and a seemingly random crash
*/
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 758
Quoted 9 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0halt_axi_ports:q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_q6);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_modem);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_nc);q6v5_clk_disable (qproc->dev, qproc->active_clks, qproc->active_clk_count); + assign_mem_result = +q6v5_assign_mem_to_linux (qproc, + qproc->mba_phys, qproc->mba_size);
Shouldn't we move them even further down?
Same BUG_ON() as above.
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 761
Quoted 3 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0assert_reset:reset_control_assert (qproc->mss_restart); disable_vdd:
Regards,
Bjorn
Re: [RESEND: PATCH v4 3/4] remoteproc: qcom: Make secure world call for mem ownership switch
Quoted 11 lines · Bjorn Andersson
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 297
Quoted 5 lines · Avaneesh Kumar Dwivedi
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+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_destVmPermnext = {
.vmid =QCOM_SCM_VMID_MSS_MSA,
.perm =QCOM_SCM_PERM_RW
};
OK.
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 300
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0+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.
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ if (!qproc->need_mem_protection) + return 0;Put a blank line here, to give separation between the sections of the function.
OK.
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 304
Quoted 3 lines
drivers/remoteproc/qcom_q6v5_pil.c +2
-0+ ret =qcom_scm_assign_mem (addr, size, +BIT (QCOM_SCM_VMID_HLOS ), next,sizeof (next));qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K), BIT(QCOM_SCM_VMID_HLOS), &next, 1);
OK
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 307
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0+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
Quoted 2 lines
So just return qcom-scm_assign_mem().
OK
Quoted 15 lines
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 316
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +9
-0+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_destVmPermnext = {
.vmid =QCOM_SCM_VMID_HLOS,
.perm =QCOM_SCM_PERM_RWX,
};
Quoted 1 line
(And add RWX to the list of defines in patch 1)
OK
Quoted 16 lines
[..]
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) · line 524
Quoted 9 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0@@ -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.
Quoted 2 lines
I do however not have a better idea at the moment than just leaking the memory.
Then shall we BUG_ON here itself?
Quoted 7 lines
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0dma_free_attrs (qproc->dev, fw->size, ptr, phys, dma_attrs);return ret <0 ? ret :0 ;[..]
Quoted 2 lines
(quoted diff) +0
-0@@ -656,16 +719,21 @@ static int q6v5_start(struct rproc *rproc)
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 722
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-1ret =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.
Quoted 2 lines
The timeout below still has to reclaim the memory.
Yes that any way will do.
Quoted 11 lines
Quoted 11 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-1ret =wait_for_completion_timeout (&qproc->start_done,msecs_to_jiffies (5000 ));if (ret ==0 ) {dev_err (qproc->dev,"start timed out\n" ); ret = -ETIMEDOUT ; - goto halt_axi_ports; + goto reclaim_mem; }
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 736
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+ 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.
Quoted 3 lines
Although this shows that we should be able to release the mba allocation here.
Yes that is true.
Quoted 15 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 749
Quoted 12 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0qproc->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.
Quoted 7 lines
So I do believe you should have a
BUG_ON(assign_mem_result); here. With a clear comment in the lines of:/*
* Failed to reclaim memory, any future access to these pages will cause
* security violations and a seemingly random crash
*/
Yes this is very good suggestion thanks.
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 758
Quoted 9 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0halt_axi_ports:q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_q6);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_modem);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_nc);q6v5_clk_disable (qproc->dev, qproc->active_clks, qproc->active_clk_count); + assign_mem_result = +q6v5_assign_mem_to_linux (qproc, + qproc->mba_phys, qproc->mba_size);Shouldn't we move them even further down?
There does not seem any restriction w.r.t. keeping it here. Do you think any side effect ?
Quoted 2 lines
Same
BUG_ON()as above.
Yes sure.
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 761
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0assert_reset:reset_control_assert (qproc->mss_restart); disable_vdd:Regards,
Bjorn
Signature · 2 lines
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
Quoted 17 lines · Avaneesh Kumar (avani)
Quoted 14 lines · Bjorn Andersson
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) · line 523
Quoted 8 lines · Avaneesh Kumar Dwivedi
drivers/remoteproc/qcom_q6v5_pil.c +5
-0@@ -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.
Quoted 3 lines
Quoted 2 lines
I do however not have a better idea at the moment than just leaking the memory.
Then shall we
BUG_ONhere itself?
Here we have the chance to "handle" the problem (by leaking the memory), so it's not a catastrophic error. But perhaps better to replace the dev_err() with a WARN(ret, "failed to reclaim memory\n"); that way this issue will stand out in the log.
[..]
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +0
-0@@ -656,16 +719,21 @@ static int q6v5_start(struct rproc *rproc)
[..]
Quoted 8 lines
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 736
Quoted 6 lines
}drivers/remoteproc/qcom_q6v5_pil.c +5
-0+ 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().
Quoted 4 lines
Actually MBA image is transferred into internal memory of q6 and does not run from DDR
that is why it is OK to release it here. let me know if you dont want to do that.
Leave it here, but please add a comment above this snippet saying something like:
/*
* The MBA is transferred to internal memory of the Q6 and no longer
* need access.
*/
[..]
Quoted 6 lines
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 758
Quoted 3 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0+ assign_mem_result = +q6v5_assign_mem_to_linux (qproc, + qproc->mba_phys, qproc->mba_size);Shouldn't we move them even further down?
There does not seem any restriction w.r.t. keeping it here. Do you think any side effect ?
No, I'm fine with this if you say that the MPSS is no longer executing anything at this point.
Thanks,
Bjorn
PATCH RESEND v4 4/4 remoteproc: qcom: Add support for mss boot on msm8996
In reply to Avaneesh Kumar DwivediThis 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.
--- .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 4 +- drivers/remoteproc/qcom_q6v5_pil.c | 171 ++++++++++++++++++--- 2 files changed, 150 insertions(+), 25 deletions(-)
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +2 -2
@@ -9,8 +9,8 @@ on the Qualcomm Hexagon core. Definition: must be one of: "qcom,q6v5-pil", "qcom,msm8916-mss-pil", - "qcom,msm8974-mss-pil" - + "qcom,msm8974-mss-pil", + "qcom,msm8996-mss-pil" - reg: Usage: required Value type: <prop-encoded-array>
drivers/remoteproc/qcom_q6v5_pil.c +148 -23
253 lines not on this page · raw message
Signature · 2 lines
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
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt on the Qualcomm Hexagon core. · line 13
Quoted 12 lines · Avaneesh Kumar Dwivedi
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +2
-2@@ -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.
Quoted 4 lines
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +0
-0- reg: Usage: required Value type: <prop-encoded-array>drivers/remoteproc/qcom_q6v5_pil.c +0
-0
[..]
drivers/remoteproc/qcom_q6v5_pil.c line 105
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +8
-0+/* QDSP6v56 parameters */ +#define QDSP6v56_LDO_BYP BIT(25) +#define QDSP6v56_BHS_ON BIT(24) +#define QDSP6v56_CLAMP_WL BIT(21) +#define QDSP6v56_CLAMP_QMC_MEM BIT(22) +#define HALT_CHECK_MAX_LOOPS 200 +#define QDSP6SS_XO_CBCR 0x0038 +#define QDSP6SS_ACC_OVERRIDE_VAL 0x20
Please keep the blank line between the defines and the struct definition.
Quoted 3 lines
struct
reg_info{drivers/remoteproc/qcom_q6v5_pil.c +0
-0struct regulator *reg;int uV;
[..]
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_remove(struct platform_device *pdev) · line 1245
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +9
-0+static const struct rproc_hexagon_res msm8996_mss = { + .hexagon_mba_image ="mba.mbn" , + .proxy_supply = (struct qcom_mss_reg_res []) { + {} + }, + .active_supply = (struct qcom_mss_reg_res []) { + {}, + {} + },
It's possible to just leave .proxy_supply and .active_supply out (i.e. leaving them as NULL), as I made q6v5_regulator_init() handle this gracefully a while back.
Regards,
Bjorn
Re: [RESEND: PATCH v4 4/4] remoteproc: qcom: Add support for mss boot on msm8996
Quoted 15 lines · Bjorn Andersson
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt on the Qualcomm Hexagon core. · line 13
Quoted 12 lines · Avaneesh Kumar Dwivedi
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +2
-2@@ -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.
Quoted 6 lines
Quoted 5 lines
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +0
-0- reg: Usage: required Value type: <prop-encoded-array>drivers/remoteproc/qcom_q6v5_pil.c +0
-0[..]
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c line 105
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +8
-0+/* QDSP6v56 parameters */ +#define QDSP6v56_LDO_BYP BIT(25) +#define QDSP6v56_BHS_ON BIT(24) +#define QDSP6v56_CLAMP_WL BIT(21) +#define QDSP6v56_CLAMP_QMC_MEM BIT(22) +#define HALT_CHECK_MAX_LOOPS 200 +#define QDSP6SS_XO_CBCR 0x0038 +#define QDSP6SS_ACC_OVERRIDE_VAL 0x20Please keep the blank line between the defines and the struct definition.
OK.
Quoted 6 lines
Quoted 4 lines
struct reg_info { struct regulator *reg; int uV;[..]
Quoted 1 line
Quoted 12 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_remove(struct platform_device *pdev) · line 1245
Quoted 9 lines
drivers/remoteproc/qcom_q6v5_pil.c +9
-0+static const struct rproc_hexagon_res msm8996_mss = { + .hexagon_mba_image ="mba.mbn" , + .proxy_supply = (struct qcom_mss_reg_res []) { + {} + }, + .active_supply = (struct qcom_mss_reg_res []) { + {}, + {} + },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.
Quoted 3 lines
Regards,
Bjorn
Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.