Enable mss rproc support on msm8996
v3 cover Enable mss rproc support on msm8996
This Patch series fix the comment received on v2 patchset.
Major changes w.r.t. patchset v2 is as below.
1- move majority of hypervisor call related changes into scm driver
which were earlier in q6v5 rproc driver, which was not compatible to existing design.
2- remove voting for proxy supplies till the time corner voting is supported.
Though testing of mss rproc driver is possible as cx and mx supplies are in always power on domain.
3- Fix some other Minor comments.
This patchseries is based on Bjorns remoteproc git repository at:
git://github.com/andersson/remoteproc tags/rproc-v4.11
or to say this patchset is based on top of commit id 01625cc5e65f420dbea57f7de6b9552a0f807a84 in Bjorn's git repo.
Avaneesh Kumar Dwivedi (2):
soc: qcom: Add support of scm call for mss rproc to share access of
ddr
remoteproc: qcom: Enable mss rproc support on msm8996
.../devicetree/bindings/remoteproc/qcom,q6v5.txt | 1 + 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 | 290 +++++++++++++++++++-- include/linux/qcom_scm.h | 14 + 6 files changed, 400 insertions(+), 26 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 v3 1/2 soc: qcom: Add support of scm call for mss rproc to share access of ddr
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.
--- 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(-)
drivers/firmware/qcom_scm-64.c +25 -0
@@ -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) +{ +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 , + &desc, &res); + +return ret ? : res.a1; +}
drivers/firmware/qcom_scm.c +93 -0
105 lines not on this page · raw message
drivers/firmware/qcom_scm.h +3 -0
@@ -55,6 +55,9 @@ 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); +#define QCOM_SCM_SVC_MP 0xc +#define QCOM_MEM_PROT_ASSIGN_ID 0x16 +extern int __qcom_scm_assign_mem (struct device *dev,struct vmid_detail vmid);/* common error codes */ #define QCOM_SCM_V2_EBUSY -12
drivers/remoteproc/qcom_q6v5_pil.c +127 -2
212 lines not on this page · raw message
include/linux/qcom_scm.h +14 -0
@@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req {u32 val; }; +struct vmid_detail { +__le32 *from; +__le32 *to; +__le32 *permission; +__le32 size_from; +__le32 size_to; +__le32 size_fw; +__le64 fw_phy; +__le64 from_phy; +__le64 to_phy; + +}; +extern bool qcom_scm_is_available (void );extern bool qcom_scm_hdcp_available (void );
@@ -36,6 +49,7 @@ extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size);extern int qcom_scm_pas_auth_and_reset (u32 peripheral);extern int qcom_scm_pas_shutdown (u32 peripheral); +extern int qcom_scm_assign_mem (struct vmid_detail vmid);#define QCOM_SCM_CPU_PWR_DOWN_L2_ON 0x0#define QCOM_SCM_CPU_PWR_DOWN_L2_OFF 0x1
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: [PATCH v3 1/2] soc: qcom: Add support of scm call for mss rproc to share access of ddr
drivers/firmware/qcom_scm-64.c int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) · line 362
Quoted 10 lines · Avaneesh Kumar Dwivedi
drivers/firmware/qcom_scm-64.c +2
-0@@ -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?
drivers/firmware/qcom_scm-64.c int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) · line 373
Quoted 11 lines
drivers/firmware/qcom_scm-64.c +11
-0+{ +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.
Quoted 17 lines
drivers/firmware/qcom_scm-64.c +12
-0+ 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 +0
-0
drivers/firmware/qcom_scm.c struct qcom_scm { · line 48
Quoted 7 lines
drivers/firmware/qcom_scm.c +4
-0@@ -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.
Quoted 12 lines
drivers/firmware/qcom_scm.c +8
-0+__le32 ctx_size; +}; + +struct fw_region_info { +__le64 addr; +__le64 size; +}; +static int qcom_scm_clk_enable (void ) {int ret;
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 311
Quoted 8 lines
drivers/firmware/qcom_scm.c +5
-0@@ -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.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 314
Quoted 3 lines
drivers/firmware/qcom_scm.c +3
-0+* @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.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 316
Quoted 2 lines
drivers/firmware/qcom_scm.c +2
-0+*/ +int qcom_scm_assign_mem (struct vmid_detail vmid)
Please no structure copy.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 323
Quoted 7 lines
drivers/firmware/qcom_scm.c +7
-0+{ +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.
Quoted 8 lines
drivers/firmware/qcom_scm.c +8
-0+int i; + + from =dma_alloc_attrs (__scm->dev, vmid.size_from, + &vmid.from_phy,GFP_KERNEL , dma_attrs); +if (!from) { +dev_err (__scm->dev, +"failed to allocate buffer to pass source vmid detail\n" ); +return -ENOMEM ;
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 341
Quoted 10 lines
drivers/firmware/qcom_scm.c +10
-0+ } + to =dma_alloc_attrs (__scm->dev, vmid.size_to, + &vmid.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), + &fw_phy,GFP_KERNEL , dma_attrs);
Can we consolidate this into one allocation with the appropriate size and then offset the different structures in it?
Quoted 5 lines
drivers/firmware/qcom_scm.c +5
-0+if (!fw_info) { +dev_err (__scm->dev, +"failed to allocate buffer to pass firmware detail\n" ); + goto free_dest_buff; + }
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 356
Quoted 10 lines
drivers/firmware/qcom_scm.c +10
-0+ +/* 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.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 362
Quoted 6 lines
drivers/firmware/qcom_scm.c +6
-0+ to[i].ctx =NULL ; + to[i].ctx_size =0 ; + } + +/* copy detail of firmware region whose mapping need to be done */ +/* in physically contigious buffer */
/*
* Multi-line comments are like so.
*/
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 374
Quoted 12 lines
drivers/firmware/qcom_scm.c +12
-0+ fw_info->addr = vmid.fw_phy; + fw_info->size = vmid.size_fw; + +/* reuse fw_phy and size_fw members to fill address and size of */ +/* fw_info buffer */ + vmid.fw_phy = fw_phy; + vmid.size_to =sizeof (*to) * (vmid.size_to /sizeof (__le32)); + vmid.size_fw =sizeof (*fw_info); + ret =__qcom_scm_assign_mem (__scm->dev, vmid); +if (!ret) + goto free_fw_buff; +return ret;
We don't free dma on failure?
Quoted 21 lines
drivers/firmware/qcom_scm.c +13
-0+free_fw_buff: +dma_free_attrs (__scm->dev,sizeof (*fw_info), fw_info, + fw_phy, dma_attrs); +free_dest_buff: +dma_free_attrs (__scm->dev, vmid.size_to, to, + vmid.to_phy, dma_attrs); +free_src_buff: +dma_free_attrs (__scm->dev, vmid.size_from, from, + vmid.from_phy, dma_attrs); +return ret; +} +EXPORT_SYMBOL (qcom_scm_assign_mem); +static int qcom_scm_pas_reset_assert (struct reset_controller_dev *rcdev,unsigned long idx) {drivers/firmware/qcom_scm.h +0
-0
drivers/firmware/qcom_scm.h extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral, · line 58
Quoted 4 lines
drivers/firmware/qcom_scm.h +1
-0@@ -55,6 +55,9 @@ 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); +#define QCOM_SCM_SVC_MP 0xc
This is already defined upstream?
Quoted 10 lines
drivers/firmware/qcom_scm.h +2
-0+#define QCOM_MEM_PROT_ASSIGN_ID 0x16 +extern int __qcom_scm_assign_mem (struct device *dev,struct vmid_detail vmid);/* common error codes */ #define QCOM_SCM_V2_EBUSY -12drivers/remoteproc/qcom_q6v5_pil.c +0
-0
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 312
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-0@@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,return &table; } +int hyp_mem_access (int id,phys_addr_t addr,size_t size)
static?
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0static const struct of_device_id q6v5_of_match[] = {include/linux/qcom_scm.h +0
-0@@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req {u32 val; };
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 35
Quoted 10 lines
include/linux/qcom_scm.h +10
-0+struct vmid_detail { +__le32 *from; +__le32 *to; +__le32 *permission; +__le32 size_from; +__le32 size_to; +__le32 size_fw; +__le64 fw_phy; +__le64 from_phy; +__le64 to_phy;
should the *_phy be phys_addr_t types?
Leave these all as u32/u64. Perhaps also move size_from/size_to next to the arrays they're for. Also add some documentation so we know what they're all about.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of 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
Quoted 7 lines · Stephen Boyd
Quoted 6 lines · Avaneesh Kumar Dwivedi
drivers/firmware/qcom_scm-64.c +0
-0@@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
Quoted 5 lines
drivers/firmware/qcom_scm-64.c int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) · line 362
Quoted 4 lines
drivers/firmware/qcom_scm-64.c +2
-0return 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?
Quoted 15 lines
drivers/firmware/qcom_scm-64.c int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) · line 373
Quoted 12 lines
drivers/firmware/qcom_scm-64.c +11
-0+{ +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_detailstructure has__le32in it.
will fix.
Quoted 21 lines
Quoted 21 lines
drivers/firmware/qcom_scm-64.c +12
-0+ 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 +0
-0@@ -42,6 +42,18 @@ struct qcom_scm {static struct qcom_scm *__scm;
Quoted 7 lines
drivers/firmware/qcom_scm.c struct qcom_scm { · line 48
Quoted 4 lines
drivers/firmware/qcom_scm.c +4
-0+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.
Quoted 16 lines
Quoted 16 lines
drivers/firmware/qcom_scm.c +8
-0+__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);
Quoted 7 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 311
Quoted 5 lines
drivers/firmware/qcom_scm.c +5
-0+/** +* 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 :(
Quoted 6 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 314
Quoted 4 lines
drivers/firmware/qcom_scm.c +3
-0+* @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.
Quoted 4 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 316
Quoted 3 lines
drivers/firmware/qcom_scm.c +2
-0+*/ +int qcom_scm_assign_mem (struct vmid_detail vmid)Please no structure copy.
should struct pointer be OK, or direct argument passing?
Quoted 9 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 323
Quoted 8 lines
drivers/firmware/qcom_scm.c +7
-0+{ +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.
Quoted 13 lines
Quoted 13 lines
drivers/firmware/qcom_scm.c +12
-0+int i; + + from =dma_alloc_attrs (__scm->dev, vmid.size_from, + &vmid.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, + &vmid.to_phy,GFP_KERNEL , dma_attrs); +if (!to) {
Quoted 8 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 341
Quoted 6 lines
drivers/firmware/qcom_scm.c +6
-0+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), + &fw_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.
Quoted 8 lines
Quoted 8 lines
drivers/firmware/qcom_scm.c +8
-0+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 */
Quoted 8 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 356
Quoted 7 lines
drivers/firmware/qcom_scm.c +7
-0+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 seems some environment problem, it did not gave any warning.
Quoted 10 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 362
Quoted 7 lines
drivers/firmware/qcom_scm.c +6
-0+ to[i].ctx =NULL ; + to[i].ctx_size =0 ; + } + +/* copy detail of firmware region whose mapping need to be done */ +/* in physically contigious buffer */ /*
* Multi-line comments are like so.
*/
Will change.
Quoted 14 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 374
Quoted 13 lines
drivers/firmware/qcom_scm.c +12
-0+ fw_info->addr = vmid.fw_phy; + fw_info->size = vmid.size_fw; + +/* reuse fw_phy and size_fw members to fill address and size of */ +/* fw_info buffer */ + vmid.fw_phy = fw_phy; + vmid.size_to =sizeof (*to) * (vmid.size_to /sizeof (__le32)); + vmid.size_fw =sizeof (*fw_info); + ret =__qcom_scm_assign_mem (__scm->dev, vmid); +if (!ret) + goto free_fw_buff; +return ret;We don't free dma on failure?
Did not get, isnt i am freeing all dma allocs on failure?
Quoted 19 lines
Quoted 19 lines
drivers/firmware/qcom_scm.c +13
-0+free_fw_buff: +dma_free_attrs (__scm->dev,sizeof (*fw_info), fw_info, + fw_phy, dma_attrs); +free_dest_buff: +dma_free_attrs (__scm->dev, vmid.size_to, to, + vmid.to_phy, dma_attrs); +free_src_buff: +dma_free_attrs (__scm->dev, vmid.size_from, from, + vmid.from_phy, dma_attrs); +return ret; +} +EXPORT_SYMBOL (qcom_scm_assign_mem); +static int qcom_scm_pas_reset_assert (struct reset_controller_dev *rcdev,unsigned long idx) {drivers/firmware/qcom_scm.h +0
-0
Quoted 8 lines
drivers/firmware/qcom_scm.h extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral, · line 58
Quoted 7 lines
drivers/firmware/qcom_scm.h +1
-0@@ -55,6 +55,9 @@ 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); +#define QCOM_SCM_SVC_MP 0xcThis is already defined upstream?
Will check, but i thought its not there.
Quoted 14 lines
Quoted 14 lines
drivers/firmware/qcom_scm.h +2
-0+#define QCOM_MEM_PROT_ASSIGN_ID 0x16 +extern int __qcom_scm_assign_mem (struct device *dev,struct vmid_detail vmid);/* common error codes */ #define QCOM_SCM_V2_EBUSY -12drivers/remoteproc/qcom_q6v5_pil.c +0
-0@@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,return &table; }
Quoted 2 lines
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 312
Quoted 1 line
drivers/remoteproc/qcom_q6v5_pil.c +1
-0+int hyp_mem_access (int id,phys_addr_t addr,size_t size)static?
Yes, will correct.
Quoted 23 lines
Quoted 11 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0static const struct of_device_id q6v5_of_match[] = {include/linux/qcom_scm.h +0
-0@@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req {u32 val; };include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 35
Quoted 10 lines
include/linux/qcom_scm.h +10
-0+struct vmid_detail { +__le32 *from; +__le32 *to; +__le32 *permission; +__le32 size_from; +__le32 size_to; +__le32 size_fw; +__le64 fw_phy; +__le64 from_phy; +__le64 to_phy;should the
*_phybephys_addr_ttypes?
Quoted 3 lines
Leave these all as u32/u64. Perhaps also move size_from/size_to next to the arrays they're for. Also add some documentation so we know what they're all about.
OK.
Quoted 1 line
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: [PATCH v3 1/2] soc: qcom: Add support of scm call for mss rproc to share access of ddr
Quoted 2 lines · Avaneesh Kumar (avani)
Quoted 1 line · Stephen Boyd
On 03/08, Avaneesh Kumar Dwivedi wrote:
[..]
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
drivers/firmware/qcom_scm.c +0
-0@@ -42,6 +42,18 @@ struct qcom_scm {Quoted 8 lines
drivers/firmware/qcom_scm.c struct qcom_scm { · line 48
Quoted 5 lines
drivers/firmware/qcom_scm.c +4
-0static 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.
Quoted 2 lines
Downstream this is pointer though unused, that is why kept same will check and change.
The problem is that the size of a pointer depends on which platform you're on. Spelling out __le32 here is then just deceiving.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/firmware/qcom_scm.c +2
-0+__le32 ctx_size; +};
[..]
Quoted 1 line
Quoted 1 line
Quoted 1 line
+int
qcom_scm_assign_mem(struct vmid_detail vmid)
[..]
Quoted 6 lines
Quoted 5 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 374
Quoted 4 lines
drivers/firmware/qcom_scm.c +4
-0+ ret =__qcom_scm_assign_mem (__scm->dev, vmid); +if (!ret) + goto free_fw_buff; +return ret;We don't free dma on failure?
Did not get, isnt i am freeing all dma allocs on failure?
In the event that __qcom_scm_assign_mem() returns non-zero your not jumping to free_fw_buff and just returning "ret" without freeing the memory.
[..]
Quoted 11 lines
Quoted 10 lines
drivers/firmware/qcom_scm.h extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral, · line 58
Quoted 9 lines
drivers/firmware/qcom_scm.h +1
-0@@ -55,6 +55,9 @@ 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); +#define QCOM_SCM_SVC_MP 0xcThis is already defined upstream?
Will check, but i thought its not there.
We use the qcom_scm-api to abstract these facts, so this is an internal constant - as such it goes in drivers/firmware/qcom_scm.h - and it's already defined.
Regards,
Bjorn
Re: [PATCH v3 1/2] soc: qcom: Add support of scm call for mss rproc to share access of ddr
In reply to Avaneesh Kumar DwivediQuoted 17 lines · Avaneesh Kumar Dwivedi
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 <[email protected]>--- 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(-)drivers/firmware/qcom_scm-64.c +0
-0
drivers/firmware/qcom_scm-64.c int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) · line 362
Quoted 5 lines
drivers/firmware/qcom_scm-64.c +2
-0@@ -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.
Quoted 12 lines
drivers/firmware/qcom_scm-64.c +12
-0+{ +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 ;
drivers/firmware/qcom_scm-64.c int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) · line 384
Quoted 10 lines
drivers/firmware/qcom_scm-64.c +10
-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;
If I understand the downstream code we only care about "ret" here; being zero on success or negative on error.
drivers/firmware/qcom_scm.c struct qcom_scm { · line 48
Quoted 13 lines
drivers/firmware/qcom_scm-64.c +1
-0+}drivers/firmware/qcom_scm.c +4
-0@@ -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.
drivers/firmware/qcom_scm.c struct qcom_scm { · line 50
Quoted 2 lines
drivers/firmware/qcom_scm.c +2
-0+__le32 ctx_size; +};
This should be __packed
Quoted 13 lines
drivers/firmware/qcom_scm.c +6
-0+ +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);
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 316
Quoted 10 lines
drivers/firmware/qcom_scm.c +10
-0+/** +* 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 <vmid, permission> 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)).
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);
And I believe something like "curr_perm" and "new_perm" are even better names than "from" and "to"
Quoted 20 lines
drivers/firmware/qcom_scm.c +20
-0+{ +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 ; +int i; + + from =dma_alloc_attrs (__scm->dev, vmid.size_from, + &vmid.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, + &vmid.to_phy,GFP_KERNEL , dma_attrs); +if (!to) { + dev_err(__scm->dev,
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 346
Quoted 10 lines
drivers/firmware/qcom_scm.c +10
-0+ "failed to allocate buffer to pass destination vmid detail\n"); + goto free_src_buff; + } + fw_info =dma_alloc_attrs (__scm->dev,sizeof (*fw_info), + &fw_phy,GFP_KERNEL , dma_attrs); +if (!fw_info) { +dev_err (__scm->dev, +"failed to allocate buffer to pass firmware detail\n" ); + goto free_dest_buff; + }
You should be able to allocate these in one chunk and you should be able to just use dma_alloc_coherent().
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 350
Quoted 4 lines
drivers/firmware/qcom_scm.c +4
-0+ +/* copy detail of original owner of ddr region */ +/* in physically contigious buffer */ +memcpy (from, vmid.from, vmid.size_from);
With "from" as a bitmap see hweight_long() to figure out the size of "from".
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 354
Quoted 4 lines
drivers/firmware/qcom_scm.c +4
-0+ +/* fill details of new owners of ddr region*/ +/* in physically contigious buffer */ +for (i =0 ; i < (vmid.size_to /sizeof (__le32)); i++) {
Better pass "number of entries in 'to'" and use standard types (such as int or unsigned int) for "to" and "permission" until this point.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 364
Quoted 10 lines
drivers/firmware/qcom_scm.c +10
-0+ to[i].vm = vmid.to[i]; + to[i].perm = vmid.permission[i]; + to[i].ctx =NULL ; + to[i].ctx_size =0 ; + } + +/* copy detail of firmware region whose mapping need to be done */ +/* in physically contigious buffer */ + fw_info->addr = vmid.fw_phy; + fw_info->size = vmid.size_fw;
fw_info is a confusing name for "a list of memory regions".
And you need a cpu_to_le32() somewhere, better keep the in-kernel API pass standard types (such as phys_addr_t) and then convert to the "hardware specific" type here.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 367
Quoted 3 lines
drivers/firmware/qcom_scm.c +3
-0+ +/* reuse fw_phy and size_fw members to fill address and size of */ +/* fw_info buffer */
Please don't try to be "clever" and reuse things when writing kernel-code, non-obvious reuse of variables or struct members often end up causing someone else to introduce bugs in your code later.
drivers/remoteproc/qcom_q6v5_pil.c
Quoted 41 lines
drivers/firmware/qcom_scm.c +20
-0+ vmid.fw_phy = fw_phy; + vmid.size_to =sizeof (*to) * (vmid.size_to /sizeof (__le32)); + vmid.size_fw =sizeof (*fw_info); + ret =__qcom_scm_assign_mem (__scm->dev, vmid); +if (!ret) + goto free_fw_buff; +return ret; +free_fw_buff: +dma_free_attrs (__scm->dev,sizeof (*fw_info), fw_info, + fw_phy, dma_attrs); +free_dest_buff: +dma_free_attrs (__scm->dev, vmid.size_to, to, + vmid.to_phy, dma_attrs); +free_src_buff: +dma_free_attrs (__scm->dev, vmid.size_from, from, + vmid.from_phy, dma_attrs); +return ret; +} +EXPORT_SYMBOL (qcom_scm_assign_mem); +static int qcom_scm_pas_reset_assert (struct reset_controller_dev *rcdev,unsigned long idx) {drivers/firmware/qcom_scm.h +3
-0@@ -55,6 +55,9 @@ 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); +#define QCOM_SCM_SVC_MP 0xc +#define QCOM_MEM_PROT_ASSIGN_ID 0x16 +extern int __qcom_scm_assign_mem (struct device *dev,struct vmid_detail vmid);/* common error codes */ #define QCOM_SCM_V2_EBUSY -12drivers/remoteproc/qcom_q6v5_pil.c +0
-0
Please split the patch in a scm-patch and a q6v5 patch.
drivers/remoteproc/qcom_q6v5_pil.c struct rproc_hexagon_res { · line 113
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-0@@ -110,6 +110,7 @@ struct rproc_hexagon_res {struct qcom_mss_reg_res *active_supply;char **proxy_clk_names;char **active_clk_names; +int version;
This will result in multi-line conditionals checking if we should do memory protection for platform x, y, z or not. Add a "bool need_mem_protect;" instead.
Quoted 19 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0};struct q6v5 {@@ -153,8 +154,28 @@ struct q6v5 {size_t mpss_size;struct qcom_rproc_subdev smd_subdev; +int version; }; +enum { +MSS_MSM8916 , +MSS_MSM8974 , +MSS_MSM8996 , +}; + +enum { +ASSIGN_ACCESS_MSA , +REMOVE_ACCESS_MSA ,
drivers/remoteproc/qcom_q6v5_pil.c struct q6v5 { · line 178
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+ASSIGN_SHARED_ACCESS_MSA , + REMOVE_SHARED_ACCESS_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 (PERM_READ | PERM_WRITE)
These belongs in the SCM API instead. (Prefix them appropriately)
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 312
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-0static int q6v5_regulator_init (struct device *dev,struct reg_info *regs,const struct qcom_mss_reg_res *reg_res) {@@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,return &table; } +int hyp_mem_access (int id,phys_addr_t addr,size_t size)
I much prefer the downstream split and naming of this function, rather than using a switch to implement 4 different functions in one please split it up.
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 316
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0+{ +struct vmid_detail vmid; +int ret; +
Pass the struct q6v5 here and return successfully here if we're not supposed to do memory protection. It does clean up the calling code.
Quoted 47 lines
drivers/remoteproc/qcom_q6v5_pil.c +43
-0+switch (id) { +case ASSIGN_ACCESS_MSA : + vmid.from = (int []) {VMID_HLOS }; + vmid.to = (int []) {VMID_MSS_MSA }; + vmid.permission = (int []) {PERM_READ |PERM_WRITE }; + vmid.size_from = vmid.size_to =1 *sizeof (__le32); +break ; +case REMOVE_ACCESS_MSA : + vmid.from = (int []) {VMID_MSS_MSA }; + vmid.to = (int []) {VMID_HLOS }; + vmid.permission = + (int []) {PERM_READ |PERM_WRITE |PERM_EXEC }; + vmid.size_from = vmid.size_to =1 *sizeof (__le32); +break ; +case ASSIGN_SHARED_ACCESS_MSA : + vmid.from = (int []) {VMID_HLOS }; + vmid.to = (int []) {VMID_HLOS ,VMID_MSS_MSA }; + vmid.permission = (int []) {PERM_RW ,PERM_RW }; + vmid.size_from =1 *sizeof (__le32); + vmid.size_to =2 *sizeof (__le32); +break ; +case REMOVE_SHARED_ACCESS_MSA : + vmid.from = (int []) {VMID_HLOS ,VMID_MSS_MSA }; + vmid.to = (int []) {VMID_HLOS }; + vmid.permission = + (int []) {PERM_READ |PERM_WRITE |PERM_EXEC }; + vmid.size_from =2 *sizeof (__le32); + vmid.size_to =1 *sizeof (__le32); +break ; +default : +break ; + } + + vmid.fw_phy = addr; + vmid.size_fw = size; + ret =qcom_scm_assign_mem (vmid); +if (ret) +pr_err ("%s: Failed to assign memory protection, ret = %d\n" , + __func__, ret); + +return ret; +} +static int q6v5_load (struct rproc *rproc,const struct firmware *fw) {struct q6v5 *qproc = rproc->priv;
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) · line 534
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +2
-0@@ -461,6 +530,15 @@ 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 = qproc->version == MSS_MSM8996 ?
To prove the point above, imagine just in the next year when this becomes:
ret = (qproc->version == MSS_MSM8996 ||
qproc->version == MSS_MSM8998 ||
qproc->version == MSS_NEXT_THING) ?
hyp_mem_access(ASSIGN_ACCESS_MSA, phys, ALIGN(fw->size, SZ_4K)) :
0;Or in 5 years...
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) · line 539
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+hyp_mem_access (ASSIGN_ACCESS_MSA , phys, + ALIGN(fw->size, SZ_4K)) : 0; +if (ret) { +dev_err (qproc->dev, +"Failed to assign metadata memory, ret - %d\n" , ret);
Rather than having this print after each call to hyp_mem_access() move the message into that function.
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) · line 540
Quoted 1 line
drivers/remoteproc/qcom_q6v5_pil.c +1
-0+return -ENOMEM ;
And don't forget to clean up the allocation before returning.
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) · line 554
Quoted 12 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+ }writel (phys, qproc->rmb_base +RMB_PMI_META_DATA_REG );writel (RMB_CMD_META_DATA_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG );@@ -470,6 +548,13 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)else if (ret <0 )dev_err (qproc->dev,"MPSS header authentication failed: %d\n" , ret); +/* Metadata authentication done, remove modem access */ + ret = qproc->version ==MSS_MSM8996 ? +hyp_mem_access (REMOVE_ACCESS_MSA , phys, +ALIGN (fw->size,SZ_4K )) :0 ;
You use ALIGN(fw->size, SZ_4K) numerous times in this function, use a variable and make the allocation take this as well to make it explicit that we're dealing with the same memory chunk size all over.
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0+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 ;
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 669
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0@@ -578,6 +663,16 @@ static int q6v5_mpss_load(struct q6v5 *qproc) size =readl (qproc->rmb_base +RMB_PMI_CODE_LENGTH_REG );if (!size) { boot_addr = relocate ? qproc->mpss_phys : min_addr; +/* Hypervisor mapping of modem fw */ + ret = qproc->version ==MSS_MSM8996 ? +hyp_mem_access (ASSIGN_SHARED_ACCESS_MSA , + boot_addr, qproc->mpss_size) :0 ;
The mpss is loaded somewhere in qproc->mpss_region, potentially at some offset (which would be reflected in boot_addr).
But if boot_addr > qproc->mpss_region the region to hand out is no longer mpss_size large, it's "mpss_size - (boot_addr - mpss_phys)", so you might give the mpss permission to trash the memory after the mpss_region.
Better hand out qproc->mpss_phys of size qproc->mpss_size.
And as far as I can tell the downstream driver load the segments and then make the MSS sole owner of the memory region. Do note that it's perfectly fine to refactor code to make things better fit a natural and easy to follow flow of ownership here!
Quoted 50 lines
drivers/remoteproc/qcom_q6v5_pil.c +22
-2+if (ret) { +dev_err (qproc->dev, +"Failed to assign fw memory access, ret - %d\n" , + ret); +return -ENOMEM ; + }writel (boot_addr, qproc->rmb_base +RMB_PMI_CODE_START_REG );writel (RMB_CMD_LOAD_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG ); }@@ -636,6 +731,14 @@ static int q6v5_start(struct rproc *rproc) goto assert_reset; } + ret = qproc->version ==MSS_MSM8996 ? +hyp_mem_access (ASSIGN_ACCESS_MSA , qproc->mba_phys, + qproc->mba_size) :0 ; +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);@@ -657,16 +760,22 @@ 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 = qproc->version ==MSS_MSM8996 ? +hyp_mem_access (REMOVE_ACCESS_MSA , qproc->mba_phys, + qproc->mba_size) :0 ; +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,
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 785
Quoted 1 line
drivers/remoteproc/qcom_q6v5_pil.c +0
-0@@ -676,7 +785,20 @@ static int q6v5_start(struct rproc *rproc)
On success q6v5_start() will leave Linus as sole owner of mba_phys and mpss_phys (or boot_addr) will have shared ownership. rproc_stop() should make sure to reclaim the sole ownership of mpss_phys again, so that subsequent calls to rproc_start() will find the memory protection in an appropriate state.
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0return 0 ; +reclaim_mem: + ret = qproc->version ==MSS_MSM8996 ? +hyp_mem_access (REMOVE_SHARED_ACCESS_MSA , + qproc->mpss_phys, qproc->mpss_size) :0 ;
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 801
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +9
-0+if (ret) +dev_err (qproc->dev, +"Failed to reclaim fw memory, ret - %d\n" , ret); halt_axi_ports: + ret = qproc->version ==MSS_MSM8996 ? +hyp_mem_access (REMOVE_ACCESS_MSA , qproc->mba_phys, + qproc->mba_size) :0 ; +if (ret) +dev_err (qproc->dev, +"Failed to reclaim mba memory, ret - %d\n" , ret);
Isn't it possible that the remote processor is still executing the MBA firmware up until we halt the axi ports and assert the reset? We don't want to remove permission until we know the CPU is stopped and it wont fault on us.
Quoted 32 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0q6v5proc_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);@@ -1015,6 +1137,7 @@ static int q6v5_probe(struct platform_device *pdev)if (ret) goto free_rproc; + qproc->version = desc->version; ret =q6v5_request_irq (qproc, pdev,"wdog" , q6v5_wdog_interrupt);if (ret <0 ) goto free_rproc;@@ -1090,6 +1213,7 @@ static int q6v5_remove(struct platform_device *pdev)"mem" ,NULL }, + .version = MSS_MSM8916, }; static const struct rproc_hexagon_res msm8974_mss = {@@ -1127,6 +1251,7 @@ static int q6v5_remove(struct platform_device *pdev)"mem" ,NULL }, + .version = MSS_MSM8974, }; static const struct of_device_id q6v5_of_match[] = {include/linux/qcom_scm.h +0
-0
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 26
Quoted 4 lines
include/linux/qcom_scm.h +1
-0@@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req {u32 val; }; +struct vmid_detail {
Whenever you add structs to a public API, make sure to add some kerneldoc.
And as this is a kernel-API I would like to see standard kernel-types for things.
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 38
Quoted 12 lines
include/linux/qcom_scm.h +12
-0+__le32 *from; +__le32 *to; +__le32 *permission; +__le32 size_from; +__le32 size_to; +__le32 size_fw; +__le64 fw_phy; +__le64 from_phy; +__le64 to_phy; + +}; +
Regards,
Bjorn
Re: [PATCH v3 1/2] soc: qcom: Add support of scm call for mss rproc to share access of ddr
Hi Bjorn/Boyd,
thanks for comments and suggestion, beg apology for delayed reply as i got busy with internal issues.
please see inline response.
Quoted 20 lines · Bjorn Andersson
Quoted 18 lines · Avaneesh Kumar Dwivedi
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 <[email protected]>--- 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(-)drivers/firmware/qcom_scm-64.c +0
-0@@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
Quoted 6 lines
drivers/firmware/qcom_scm-64.c int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) · line 362
Quoted 4 lines
drivers/firmware/qcom_scm-64.c +2
-0return 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.
Quoted 17 lines
Quoted 17 lines
drivers/firmware/qcom_scm-64.c +16
-0+{ +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 );
Quoted 8 lines
drivers/firmware/qcom_scm-64.c int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) · line 384
Quoted 6 lines
drivers/firmware/qcom_scm-64.c +6
-0+ + ret =qcom_scm_call (dev,QCOM_SCM_SVC_MP , +QCOM_MEM_PROT_ASSIGN_ID , + &desc, &res); + +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.
Quoted 7 lines
Quoted 7 lines
drivers/firmware/qcom_scm-64.c +1
-0+}drivers/firmware/qcom_scm.c +0
-0@@ -42,6 +42,18 @@ struct qcom_scm {
Quoted 7 lines
drivers/firmware/qcom_scm.c struct qcom_scm { · line 48
Quoted 6 lines
drivers/firmware/qcom_scm.c +4
-0static 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?
Quoted 4 lines
drivers/firmware/qcom_scm.c struct qcom_scm { · line 50
Quoted 3 lines
drivers/firmware/qcom_scm.c +2
-0+__le32 ctx_size; +};This should be
__packed
Ok.
Quoted 31 lines
Quoted 14 lines
drivers/firmware/qcom_scm.c +6
-0+ +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);drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 316
Quoted 10 lines
drivers/firmware/qcom_scm.c +10
-0+/** +* 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".
Quoted 6 lines
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 <vmid, permission> 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?
Quoted 5 lines
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.
Quoted 8 lines
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.
Quoted 4 lines
And I believe something like "
curr_perm" and "new_perm" are even better names than "from" and "to"
ok.
Quoted 25 lines
Quoted 25 lines
drivers/firmware/qcom_scm.c +24
-0+{ +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 ; +int i; + + from =dma_alloc_attrs (__scm->dev, vmid.size_from, + &vmid.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, + &vmid.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),
Quoted 8 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 346
Quoted 6 lines
drivers/firmware/qcom_scm.c +6
-0+ &fw_phy, GFP_KERNEL, dma_attrs); +if (!fw_info) { +dev_err (__scm->dev, +"failed to allocate buffer to pass firmware detail\n" ); + goto free_dest_buff; + }You should be able to allocate these in one chunk and you should be able to just use
dma_alloc_coherent().
OK.
Quoted 7 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 350
Quoted 5 lines
drivers/firmware/qcom_scm.c +4
-0+ +/* copy detail of original owner of ddr region */ +/* in physically contigious buffer */ +memcpy (from, vmid.from, vmid.size_from);With "from" as a bitmap see
hweight_long()to figure out the size of "from".
hweight_long() returns number of bits set in bitmap? is this correct understanding?
Quoted 7 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 354
Quoted 5 lines
drivers/firmware/qcom_scm.c +4
-0+ +/* fill details of new owners of ddr region*/ +/* in physically contigious buffer */ +for (i =0 ; i < (vmid.size_to /sizeof (__le32)); i++) {Better pass "number of entries in 'to'" and use standard types (such as int or unsigned int) for "to" and "permission" until this point.
OK.
Quoted 12 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 364
Quoted 11 lines
drivers/firmware/qcom_scm.c +10
-0+ to[i].vm = vmid.to[i]; + to[i].perm = vmid.permission[i]; + to[i].ctx =NULL ; + to[i].ctx_size =0 ; + } + +/* copy detail of firmware region whose mapping need to be done */ +/* in physically contigious buffer */ + fw_info->addr = vmid.fw_phy; + fw_info->size = vmid.size_fw;
fw_infois a confusing name for "a list of memory regions".
ok will try to use appropriate name.
Quoted 4 lines
And you need a
cpu_to_le32()somewhere, better keep the in-kernel API pass standard types (such asphys_addr_t) and then convert to the "hardware specific" type here.
ok.
Quoted 7 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 367
Quoted 4 lines
drivers/firmware/qcom_scm.c +3
-0+ +/* reuse fw_phy and size_fw members to fill address and size of */ +/* fw_info buffer */ Please don't try to be "clever" and reuse things when writing kernel-code, non-obvious reuse of variables or struct members often end up causing someone else to introduce bugs in your code later.
ok.
Quoted 36 lines
Quoted 36 lines
drivers/firmware/qcom_scm.c +20
-0+ vmid.fw_phy = fw_phy; + vmid.size_to =sizeof (*to) * (vmid.size_to /sizeof (__le32)); + vmid.size_fw =sizeof (*fw_info); + ret =__qcom_scm_assign_mem (__scm->dev, vmid); +if (!ret) + goto free_fw_buff; +return ret; +free_fw_buff: +dma_free_attrs (__scm->dev,sizeof (*fw_info), fw_info, + fw_phy, dma_attrs); +free_dest_buff: +dma_free_attrs (__scm->dev, vmid.size_to, to, + vmid.to_phy, dma_attrs); +free_src_buff: +dma_free_attrs (__scm->dev, vmid.size_from, from, + vmid.from_phy, dma_attrs); +return ret; +} +EXPORT_SYMBOL (qcom_scm_assign_mem); +static int qcom_scm_pas_reset_assert (struct reset_controller_dev *rcdev,unsigned long idx) {drivers/firmware/qcom_scm.h +3
-0@@ -55,6 +55,9 @@ 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); +#define QCOM_SCM_SVC_MP 0xc +#define QCOM_MEM_PROT_ASSIGN_ID 0x16 +extern int __qcom_scm_assign_mem (struct device *dev,struct vmid_detail vmid);
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c
Quoted 6 lines
drivers/firmware/qcom_scm.h +0
-0/* common error codes */ #define QCOM_SCM_V2_EBUSY -12drivers/remoteproc/qcom_q6v5_pil.c +0
-0Please split the patch in a scm-patch and a q6v5 patch.
ok.
Quoted 9 lines
drivers/remoteproc/qcom_q6v5_pil.c struct rproc_hexagon_res { · line 113
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-0@@ -110,6 +110,7 @@ struct rproc_hexagon_res {struct qcom_mss_reg_res *active_supply;char **proxy_clk_names;char **active_clk_names; +int version;This will result in multi-line conditionals checking if we should do memory protection for platform x, y, z or not. Add a "bool
need_mem_protect;" instead.
ok.
Quoted 23 lines
Quoted 23 lines
drivers/remoteproc/qcom_q6v5_pil.c +13
-0};struct q6v5 {@@ -153,8 +154,28 @@ struct q6v5 {size_t mpss_size;struct qcom_rproc_subdev smd_subdev; +int version; }; +enum { +MSS_MSM8916 , +MSS_MSM8974 , +MSS_MSM8996 , +}; + +enum { +ASSIGN_ACCESS_MSA , +REMOVE_ACCESS_MSA , +ASSIGN_SHARED_ACCESS_MSA , +REMOVE_SHARED_ACCESS_MSA , +};
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c struct q6v5 { · line 178
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +7
-0+ +#define VMID_HLOS 0x3 +#define VMID_MSS_MSA 0xF +#define PERM_READ 0x4 +#define PERM_WRITE 0x2 +#define PERM_EXEC 0x1 +#define PERM_RW (PERM_READ | PERM_WRITE)These belongs in the SCM API instead. (Prefix them appropriately)
Do you mean something like SCM_VMID_HLOS?
Quoted 8 lines
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0static int q6v5_regulator_init (struct device *dev,struct reg_info *regs,const struct qcom_mss_reg_res *reg_res) {@@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,return &table; }
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 312
Quoted 1 line
drivers/remoteproc/qcom_q6v5_pil.c +1
-0+int hyp_mem_access (int id,phys_addr_t addr,size_t size)I much prefer the downstream split and naming of this function, rather than using a switch to implement 4 different functions in one please split it up.
so you mean i need to have seprate function for each "int id"? in switch case? just for sake of minimum code churn i tried to work with single function.
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 316
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0+{ +struct vmid_detail vmid; +int ret; +Pass the struct q6v5 here and return successfully here if we're not supposed to do memory protection. It does clean up the calling code.
OK.
Quoted 49 lines
Quoted 49 lines
drivers/remoteproc/qcom_q6v5_pil.c +43
-0+switch (id) { +case ASSIGN_ACCESS_MSA : + vmid.from = (int []) {VMID_HLOS }; + vmid.to = (int []) {VMID_MSS_MSA }; + vmid.permission = (int []) {PERM_READ |PERM_WRITE }; + vmid.size_from = vmid.size_to =1 *sizeof (__le32); +break ; +case REMOVE_ACCESS_MSA : + vmid.from = (int []) {VMID_MSS_MSA }; + vmid.to = (int []) {VMID_HLOS }; + vmid.permission = + (int []) {PERM_READ |PERM_WRITE |PERM_EXEC }; + vmid.size_from = vmid.size_to =1 *sizeof (__le32); +break ; +case ASSIGN_SHARED_ACCESS_MSA : + vmid.from = (int []) {VMID_HLOS }; + vmid.to = (int []) {VMID_HLOS ,VMID_MSS_MSA }; + vmid.permission = (int []) {PERM_RW ,PERM_RW }; + vmid.size_from =1 *sizeof (__le32); + vmid.size_to =2 *sizeof (__le32); +break ; +case REMOVE_SHARED_ACCESS_MSA : + vmid.from = (int []) {VMID_HLOS ,VMID_MSS_MSA }; + vmid.to = (int []) {VMID_HLOS }; + vmid.permission = + (int []) {PERM_READ |PERM_WRITE |PERM_EXEC }; + vmid.size_from =2 *sizeof (__le32); + vmid.size_to =1 *sizeof (__le32); +break ; +default : +break ; + } + + vmid.fw_phy = addr; + vmid.size_fw = size; + ret =qcom_scm_assign_mem (vmid); +if (ret) +pr_err ("%s: Failed to assign memory protection, 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 +530,15 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) · line 534
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +2
-0memcpy (ptr, fw->data, fw->size); +/* Hypervisor mapping to access metadata by modem */ + ret = qproc->version == MSS_MSM8996 ?To prove the point above, imagine just in the next year when this
I agree.
Quoted 10 lines
becomes:
ret = (
qproc->version==MSS_MSM8996||qproc->version == MSS_MSM8998 || qproc->version == MSS_NEXT_THING) ? hyp_mem_access(ASSIGN_ACCESS_MSA, phys, ALIGN(fw->size, SZ_4K)) : 0;Or in 5 years...
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) · line 539
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+hyp_mem_access (ASSIGN_ACCESS_MSA , phys, + ALIGN(fw->size, SZ_4K)) : 0; +if (ret) { +dev_err (qproc->dev, +"Failed to assign metadata memory, ret - %d\n" , ret);Rather than having this print after each call to
hyp_mem_access()move the message into that function.
Ok.
Quoted 3 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) · line 540
Quoted 2 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-0+return -ENOMEM ;And don't forget to clean up the allocation before returning.
Ok.
Quoted 9 lines
Quoted 9 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-0+ }writel (phys, qproc->rmb_base +RMB_PMI_META_DATA_REG );writel (RMB_CMD_META_DATA_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG );@@ -470,6 +548,13 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)else if (ret <0 )dev_err (qproc->dev,"MPSS header authentication failed: %d\n" , ret);
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) · line 554
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0+/* Metadata authentication done, remove modem access */ + ret = qproc->version ==MSS_MSM8996 ? +hyp_mem_access (REMOVE_ACCESS_MSA , phys, +ALIGN (fw->size,SZ_4K )) :0 ;You use
ALIGN(fw->size, SZ_4K)numerous times in this function, use a variable and make the allocation take this as well to make it explicit that we're dealing with the same memory chunk size all over.
OK.
Quoted 18 lines
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0+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 ;drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_mpss_load(struct q6v5 *qproc) · line 669
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0@@ -578,6 +663,16 @@ static int q6v5_mpss_load(struct q6v5 *qproc) size =readl (qproc->rmb_base +RMB_PMI_CODE_LENGTH_REG );if (!size) { boot_addr = relocate ? qproc->mpss_phys : min_addr; +/* Hypervisor mapping of modem fw */ + ret = qproc->version ==MSS_MSM8996 ? +hyp_mem_access (ASSIGN_SHARED_ACCESS_MSA , + boot_addr, qproc->mpss_size) :0 ;The mpss is loaded somewhere in
qproc->mpss_region, potentially at some offset (which would be reflected inboot_addr).
Quoted 6 lines
But if
boot_addr>qproc->mpss_regionthe region to hand out is no longermpss_sizelarge, it's "mpss_size- (boot_addr-mpss_phys)", so you might give the mpss permission to trash the memory after thempss_region.Better hand out
qproc->mpss_physof sizeqproc->mpss_size.
OK.
Quoted 6 lines
And as far as I can tell the downstream driver load the segments and then make the MSS sole owner of the memory region. Do note that it's perfectly fine to refactor code to make things better fit a natural and easy to follow flow of ownership here!
Not very clear what to do?
Quoted 49 lines
Quoted 49 lines
drivers/remoteproc/qcom_q6v5_pil.c +22
-2+if (ret) { +dev_err (qproc->dev, +"Failed to assign fw memory access, ret - %d\n" , + ret); +return -ENOMEM ; + }writel (boot_addr, qproc->rmb_base +RMB_PMI_CODE_START_REG );writel (RMB_CMD_LOAD_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG ); }@@ -636,6 +731,14 @@ static int q6v5_start(struct rproc *rproc) goto assert_reset; } + ret = qproc->version ==MSS_MSM8996 ? +hyp_mem_access (ASSIGN_ACCESS_MSA , qproc->mba_phys, + qproc->mba_size) :0 ; +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);@@ -657,16 +760,22 @@ 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 = qproc->version ==MSS_MSM8996 ? +hyp_mem_access (REMOVE_ACCESS_MSA , qproc->mba_phys, + qproc->mba_size) :0 ; +if (ret) +dev_err (qproc->dev, +"Failed to reclaim mba memory, ret - %d\n" , ret); qproc->running = true;
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 785
Quoted 3 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0q6v5_clk_disable(qproc->dev, qproc->proxy_clks,@@ -676,7 +785,20 @@ static int q6v5_start(struct rproc *rproc)On success
q6v5_start()will leave Linus as sole owner ofmba_physandmpss_phys(orboot_addr) will have shared ownership.rproc_stop()should make sure to reclaim the sole ownership ofmpss_physagain, so that subsequent calls torproc_start()will find the memory protection in an appropriate state.
Sure!
Quoted 13 lines
Quoted 13 lines
drivers/remoteproc/qcom_q6v5_pil.c +9
-0return 0 ; +reclaim_mem: + ret = qproc->version ==MSS_MSM8996 ? +hyp_mem_access (REMOVE_SHARED_ACCESS_MSA , + qproc->mpss_phys, qproc->mpss_size) :0 ; +if (ret) +dev_err (qproc->dev, +"Failed to reclaim fw memory, ret - %d\n" , ret); halt_axi_ports: + ret = qproc->version == MSS_MSM8996 ? + hyp_mem_access(REMOVE_ACCESS_MSA, qproc->mba_phys,
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_start(struct rproc *rproc) · line 778
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0+ qproc->mba_size) : 0; +if (ret) +dev_err (qproc->dev, +"Failed to reclaim mba memory, ret - %d\n" , ret);Isn't it possible that the remote processor is still executing the MBA firmware up until we halt the axi ports and assert the reset? We don't want to remove permission until we know the CPU is stopped and it wont fault on us.
Will check.
Quoted 36 lines
Quoted 36 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0q6v5proc_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);@@ -1015,6 +1137,7 @@ static int q6v5_probe(struct platform_device *pdev)if (ret) goto free_rproc; + qproc->version = desc->version; ret =q6v5_request_irq (qproc, pdev,"wdog" , q6v5_wdog_interrupt);if (ret <0 ) goto free_rproc;@@ -1090,6 +1213,7 @@ static int q6v5_remove(struct platform_device *pdev)"mem" ,NULL }, + .version = MSS_MSM8916, }; static const struct rproc_hexagon_res msm8974_mss = {@@ -1127,6 +1251,7 @@ static int q6v5_remove(struct platform_device *pdev)"mem" ,NULL }, + .version = MSS_MSM8974, }; static const struct of_device_id q6v5_of_match[] = {include/linux/qcom_scm.h +0
-0@@ -23,6 +23,19 @@ 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 vmid_detail {Whenever you add structs to a public API, make sure to add some kerneldoc.
Will check if where is it appropriate to provide documentation.
Quoted 3 lines
And as this is a kernel-API I would like to see standard kernel-types for things.
OK.
Quoted 15 lines
include/linux/qcom_scm.h struct qcom_scm_hdcp_req { · line 38
Quoted 13 lines
include/linux/qcom_scm.h +12
-0+__le32 *from; +__le32 *to; +__le32 *permission; +__le32 size_from; +__le32 size_to; +__le32 size_fw; +__le64 fw_phy; +__le64 from_phy; +__le64 to_phy; + +}; +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: [PATCH v3 1/2] soc: qcom: Add support of scm call for mss rproc to share access of ddr
Quoted 2 lines · Avaneesh Kumar (avani)
Quoted 1 line · Bjorn Andersson
On Wed 08 Mar 10:03 PST 2017, Avaneesh Kumar Dwivedi wrote:
[..]
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
+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.
It's a good rule of thumb, but in this case you have 1 caller and 2 implementations (don't forget to stub the 32-bit one), so the overhead of having a struct just to pass these parameters probably is not worth it.
And the fact that some of the members have slightly different meanings here compared to the other API is a bigger issue.
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
drivers/firmware/qcom_scm-64.c +13
-0+{ +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 ; +
[..]
Quoted 11 lines
Quoted 11 lines
drivers/firmware/qcom_scm.c struct qcom_scm { · line 46
Quoted 10 lines
drivers/firmware/qcom_scm.c +4
-0@@ -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.
Quoted 1 line
sorry i am not sure why they need to be 64 bit variable?
__le32 says that elements are 32-bits, * says this is a pointer to such elements and as we are on a 64-bit system this member is 64-bit.
This is a binary interface between the Linux kernel and the TrustZone code, so the struct must be encoded in exactly the same way on both sides.
As Stephen suggested, making this __le64 would make this way more obvious - and endian safe.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/firmware/qcom_scm.c +2
-0+__le32 ctx_size; +};
[..]
Quoted 12 lines
Quoted 12 lines
Quoted 1 line
+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 <vmid, permission> entries.
Quoted 6 lines
Quoted 2 lines
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?
Every time you call this function you have to pass a list of "from" that corresponds to the "to" of the last (successful) call to this function.
In your code this means that the "from" is either 1 or 2 elements and as you can see in your own code there is no convenient way of doing this in C - having an array of 2 elements and claiming that it is either 1 or 2 entries long is not very clean.
Further more, in the remoteproc driver you currently rely on the fact that you "know", in each code path, what the last "to" was and just hard code "from". But if you make this call return the next "from" as a bitmap (much more convenient than a variable length array) we can store this in the driver for each memory region and I believe the calling code will be cleaner.
Quoted 19 lines
Quoted 5 lines
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.
Quoted 7 lines
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.
Quoted 4 lines
And I believe something like "
curr_perm" and "new_perm" are even better names than "from" and "to"ok.
Quoted 1 line
[..]
Quoted 9 lines
Quoted 6 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 350
Quoted 4 lines
drivers/firmware/qcom_scm.c +4
-0+ +/* copy detail of original owner of ddr region */ +/* in physically contigious buffer */ +memcpy (from, vmid.from, vmid.size_from);With "from" as a bitmap see
hweight_long()to figure out the size of "from".
hweight_long()returns number of bits set in bitmap? is this correct understanding?
Correct.
[..]
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/firmware/qcom_scm.h +0
-0
[..]
Quoted 9 lines
Quoted 8 lines
drivers/firmware/qcom_scm.h
Quoted 7 lines
drivers/firmware/qcom_scm.h +7
-0+ +#define VMID_HLOS 0x3 +#define VMID_MSS_MSA 0xF +#define PERM_READ 0x4 +#define PERM_WRITE 0x2 +#define PERM_EXEC 0x1 +#define PERM_RW (PERM_READ | PERM_WRITE)These belongs in the SCM API instead. (Prefix them appropriately)
Do you mean something like
SCM_VMID_HLOS?
This is part of the qcom-scm API, other defines there are of the form QCOM_SCM_* so QCOM_SCM_VMID_HLOS should be ok.
Quoted 11 lines
Quoted 11 lines
drivers/firmware/qcom_scm.h static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 311
Quoted 8 lines
static int q6v5_regulator_init(struct device *dev, struct
reg_info*regs,
const structqcom_mss_reg_res*reg_res)
{drivers/firmware/qcom_scm.h +1
-0@@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,return &table; } +int hyp_mem_access (int id,phys_addr_t addr,size_t size)I much prefer the downstream split and naming of this function, rather than using a switch to implement 4 different functions in one please split it up.
Quoted 2 lines
so you mean i need to have seprate function for each "int id"? in switch case?
Yes.
Quoted 2 lines
just for sake of minimum code churn i tried to work with single function.
In terms of C-code you have 1 function, but conceptually you have 4 - so any reasoning about what this function does fully depends on "id" as well.
[..]
Quoted 7 lines
Quoted 6 lines
And as far as I can tell the downstream driver load the segments and then make the MSS sole owner of the memory region. Do note that it's perfectly fine to refactor code to make things better fit a natural and easy to follow flow of ownership here!
Not very clear what to do?
In order to hand over ownership completely we can no longer verify the segments as we load them - we will have to do this in two steps.
So I believe this code has to be refactored to make this in two separate steps - with the latter done after we transmit the ownership of the memory to the remote.
Regards,
Bjorn
PATCH v3 2/2 remoteproc: qcom: Enable mss rproc support on msm8996
In reply to Avaneesh Kumar DwivediThis patch add msm8996 mss reset sequence and other minor changes for enabling mss rproc support.
--- .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 1 + drivers/remoteproc/qcom_q6v5_pil.c | 161 ++++++++++++++++++--- 2 files changed, 138 insertions(+), 24 deletions(-)
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +1 -0
@@ -10,6 +10,7 @@ on the Qualcomm Hexagon core. "qcom,q6v5-pil", "qcom,msm8916-mss-pil", "qcom,msm8974-mss-pil" + "qcom,msm8996-mss-pil" - reg: Usage: required
drivers/remoteproc/qcom_q6v5_pil.c +137 -24
212 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.