Enable msm8996 support in mss rproc driver.
v2 cover Enable msm8996 support in mss rproc driver.
In continuation of earlier RFC patch series, this series consist three patches, which add following.
1- Add hypervisor call support to enable second stage translation of intermediat physical address generated
by individual subsystem, second stage mapping of address is supported armv8 onwards.
2- Modify already floating mss rproc driver to enable mss remoteproc support for msm8996.
Differences WRT patchset 1 is as follow.
1- Secure_buffer.c file is removed which was added as a separate driver in patchset 1.
Instead now to make translation scm call one additional function is added in rproc driver itself.
2- Address other comments by sboyd on RFC patch.
This patch series is dependednt on https://patchwork.kernel.org/patch/9492177/
Avaneesh Kumar Dwivedi (3):
soc: qcom: Add scm call to protect modem mem in qcom scm driver.
remoteproc: qcom: Add scm call to protect modem mem in mss rproc drv.
remoteproc: qcom: Add msm8996 specific changes in mss rproc driver.
.../devicetree/bindings/remoteproc/qcom,q6v5.txt | 1 + drivers/firmware/qcom_scm-64.c | 17 + drivers/firmware/qcom_scm.c | 14 + drivers/firmware/qcom_scm.h | 3 + drivers/remoteproc/qcom_q6v5_pil.c | 380 +++++++++++++++++++-- include/linux/qcom_scm.h | 1 + 6 files changed, 387 insertions(+), 29 deletions(-)
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 v2 1/3 soc: qcom: Add scm call to protect modem mem in qcom scm driver.
This patch add scm call support to make hypervisor call for protecting memory region on armv8 arch cpu's.
This is required to bring up modem on msm8996. MSS rproc driver will make hypervisor call for protecting and granting permission of DDR region where MBA, MDT, FW are loaded.
--- drivers/firmware/qcom_scm-64.c | 17 +++++++++++++++++ drivers/firmware/qcom_scm.c | 14 ++++++++++++++ drivers/firmware/qcom_scm.h | 3 +++ include/linux/qcom_scm.h | 1 + 4 files changed, 35 insertions(+)
drivers/firmware/qcom_scm-64.c +17 -0
@@ -358,3 +358,20 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)return ret ? : res.a1; } + +int __qcom_scm_assign_mem (struct device *dev,void *data) +{ +int ret; +struct qcom_scm_desc *desc = data; +struct arm_smccc_res res; + + desc->arginfo =QCOM_SCM_ARGS (7 ,QCOM_SCM_RO ,QCOM_SCM_VAL , +QCOM_SCM_RO ,QCOM_SCM_VAL ,QCOM_SCM_RO , +QCOM_SCM_VAL ,QCOM_SCM_VAL ); + + ret =qcom_scm_call (dev,QCOM_SCM_SVC_MP , +QCOM_MEM_PROT_ASSIGN_ID , + desc, &res); + +return ret ? : res.a1; +}
drivers/firmware/qcom_scm.c +14 -0
@@ -292,6 +292,20 @@ int qcom_scm_pas_shutdown(u32 peripheral) }EXPORT_SYMBOL (qcom_scm_pas_shutdown); +/** +* qcom_scm_assign_mem() - Apply new access permission of DDR +* region passed via descriptor and does second stage +* translation of intermediate physical address. +* @desc: descriptor to send to hypervisor +* +* Return 0 on success. +*/ +int qcom_scm_assign_mem (void *desc) +{ +return __qcom_scm_assign_mem (__scm->dev, desc); +} +EXPORT_SYMBOL (qcom_scm_assign_mem); +static int qcom_scm_pas_reset_assert (struct reset_controller_dev *rcdev,unsigned long idx) {
drivers/firmware/qcom_scm.h +3 -0
@@ -47,6 +47,8 @@ extern int __qcom_scm_hdcp_req(struct device *dev,#define QCOM_SCM_PAS_SHUTDOWN_CMD 0x6#define QCOM_SCM_PAS_IS_SUPPORTED_CMD 0x7#define QCOM_SCM_PAS_MSS_RESET 0xa +#define QCOM_SCM_SVC_MP 0xc +#define QCOM_MEM_PROT_ASSIGN_ID 0x16extern bool __qcom_scm_pas_supported (struct device *dev,u32 peripheral);extern int __qcom_scm_pas_init_image (struct device *dev,u32 peripheral,dma_addr_t metadata_phys);
@@ -55,6 +57,7 @@ extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,extern int __qcom_scm_pas_auth_and_reset (struct device *dev,u32 peripheral);extern int __qcom_scm_pas_shutdown (struct device *dev,u32 peripheral);extern int __qcom_scm_pas_mss_reset (struct device *dev,bool reset); +extern int __qcom_scm_assign_mem (struct device *dev,void *desc);/* common error codes */ #define QCOM_SCM_V2_EBUSY -12
include/linux/qcom_scm.h +1 -0
@@ -36,6 +36,7 @@ extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size);extern int qcom_scm_pas_auth_and_reset (u32 peripheral);extern int qcom_scm_pas_shutdown (u32 peripheral); +extern int qcom_scm_assign_mem (void *desc);#define QCOM_SCM_CPU_PWR_DOWN_L2_ON 0x0#define QCOM_SCM_CPU_PWR_DOWN_L2_OFF 0x1
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 v2 1/3] soc: qcom: Add scm call to protect modem mem in qcom scm driver.
Quoted 7 lines · Avaneesh Kumar Dwivedi
drivers/firmware/qcom_scm.c +0
-0@@ -292,6 +292,20 @@ 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 303
Quoted 10 lines
drivers/firmware/qcom_scm.c +9
-0+/** +* qcom_scm_assign_mem() - Apply new access permission of DDR +* region passed via descriptor and does second stage +* translation of intermediate physical address. +* @desc: descriptor to send to hypervisor +* +* Return 0 on success. +*/ +int qcom_scm_assign_mem (void *desc)
Please don't pass a void pointer here. Driver authors shouldn't need to know about qcom_scm_desc. This should be an API that the driver can use without knowing intimate firmware details.
Quoted 13 lines
drivers/firmware/qcom_scm.c +5
-0+{ +return __qcom_scm_assign_mem (__scm->dev, desc); +} +EXPORT_SYMBOL (qcom_scm_assign_mem); +static int qcom_scm_pas_reset_assert (struct reset_controller_dev *rcdev,unsigned long idx) {drivers/firmware/qcom_scm.h +0
-0
drivers/firmware/qcom_scm.h extern int __qcom_scm_hdcp_req(struct device *dev, · line 51
Quoted 5 lines
drivers/firmware/qcom_scm.h +2
-0@@ -47,6 +47,8 @@ extern int __qcom_scm_hdcp_req(struct device *dev,#define QCOM_SCM_PAS_SHUTDOWN_CMD 0x6#define QCOM_SCM_PAS_IS_SUPPORTED_CMD 0x7#define QCOM_SCM_PAS_MSS_RESET 0xa +#define QCOM_SCM_SVC_MP 0xc +#define QCOM_MEM_PROT_ASSIGN_ID 0x16
Presumably these should go near the newly introduced functions like how the rest of this file is organized.
Quoted 1 line
drivers/firmware/qcom_scm.h +0
-0extern bool __qcom_scm_pas_supported (struct device *dev,u32 peripheral);
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v2 1/3] soc: qcom: Add scm call to protect modem mem in qcom scm driver.
Quoted 9 lines · Stephen Boyd
Quoted 8 lines · Avaneesh Kumar Dwivedi
drivers/firmware/qcom_scm.c +0
-0@@ -292,6 +292,20 @@ int qcom_scm_pas_shutdown(u32 peripheral) }EXPORT_SYMBOL (qcom_scm_pas_shutdown);
Quoted 12 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 303
Quoted 9 lines
drivers/firmware/qcom_scm.c +9
-0+/** +* qcom_scm_assign_mem() - Apply new access permission of DDR +* region passed via descriptor and does second stage +* translation of intermediate physical address. +* @desc: descriptor to send to hypervisor +* +* Return 0 on success. +*/ +int qcom_scm_assign_mem (void *desc)Please don't pass a void pointer here. Driver authors shouldn't need to know about
qcom_scm_desc. This should be an API that the driver can use without knowing intimate firmware details.
OK, so i will try to move hyp_mem_assign() to qcom_scm driver, as suggested. this way the above issue will also get resolved.
Quoted 13 lines
Quoted 13 lines
drivers/firmware/qcom_scm.c +5
-0+{ +return __qcom_scm_assign_mem (__scm->dev, desc); +} +EXPORT_SYMBOL (qcom_scm_assign_mem); +static int qcom_scm_pas_reset_assert (struct reset_controller_dev *rcdev,unsigned long idx) {drivers/firmware/qcom_scm.h +0
-0
Quoted 8 lines
drivers/firmware/qcom_scm.h extern int __qcom_scm_hdcp_req(struct device *dev, · line 51
Quoted 6 lines
drivers/firmware/qcom_scm.h +2
-0@@ -47,6 +47,8 @@ extern int __qcom_scm_hdcp_req(struct device *dev,#define QCOM_SCM_PAS_SHUTDOWN_CMD 0x6#define QCOM_SCM_PAS_IS_SUPPORTED_CMD 0x7#define QCOM_SCM_PAS_MSS_RESET 0xa +#define QCOM_SCM_SVC_MP 0xc +#define QCOM_MEM_PROT_ASSIGN_ID 0x16Presumably these should go near the newly introduced functions like how the rest of this file is organized.
OK.
Quoted 2 lines
Quoted 2 lines
drivers/firmware/qcom_scm.h +0
-0extern bool __qcom_scm_pas_supported (struct device *dev,u32 peripheral);
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 v2 2/3 remoteproc: qcom: Add scm call to protect modem mem in mss rproc drv.
In reply to Avaneesh Kumar DwivediThis patch add hypervisor call support for second stage translation from mss remoteproc driver, this is required so that modem on msm8996 which is based on armv8 architecture can access DDR region where modem firmware are loaded.
--- drivers/remoteproc/qcom_q6v5_pil.c | 202 ++++++++++++++++++++++++++++++++++++- 1 file changed, 198 insertions(+), 4 deletions(-)
drivers/remoteproc/qcom_q6v5_pil.c +198 -4
@@ -93,6 +93,23 @@#define QDSS_BHS_ON BIT(21)#define QDSS_LDO_BYP BIT(22) +struct dest_vm_and_perm_info { +__le32 vm; +__le32 perm; +__le32 *ctx; +__le32 ctx_size; +}; + +struct mem_prot_info { +__le64 addr; +__le64 size; +}; + +struct scm_desc { +__le32 arginfo; +__le64 args[10 ]; +}; +struct reg_info {struct regulator *reg;int uV;
@@ -111,6 +128,7 @@ struct rproc_hexagon_res {struct qcom_mss_reg_res active_supply[2 ];char **proxy_clk_names;char **active_clk_names; +int version; };struct q6v5 {
@@ -152,8 +170,29 @@ struct q6v5 {phys_addr_t mpss_reloc;void *mpss_region;size_t mpss_size; +int version; +int protection_cmd; +}; + +enum { +MSS_MSM8916 , +MSS_MSM8974 , +MSS_MSM8996 , }; +enum { +ASSIG_ACCESS_MSA , +REMOV_ACCESS_MSA , +SHARE_ACCESS_MSA , +REMOV_SHARE_MSA , +}; + +#define VMID_HLOS 0x3 +#define VMID_MSS_MSA 0xF +#define PERM_READ 0x4 +#define PERM_WRITE 0x2 +#define PERM_EXEC 0x1 +#define PERM_RW (0x4 | 0x2)static int q6v5_regulator_init (struct device *dev,struct reg_info *regs,const struct qcom_mss_reg_res *reg_res) {
@@ -273,12 +312,123 @@ static void q6v5_clk_disable(struct device *dev,clk_disable_unprepare (clks[i]); } +int hyp_mem_access (struct q6v5 *qproc,phys_addr_t addr,size_t size) +{ +unsigned long dma_attrs =DMA_ATTR_FORCE_CONTIGUOUS ; +struct dest_vm_and_perm_info *dest_info; +struct mem_prot_info *mem_prot_info; +struct scm_desc desc = {0 }; +__le32 *source_vm_copy; +__le64 mem_prot_phy; +int dest_count =1 ; +int src_count =1 ; +__le32 *perm = {0 }; +__le32 *dest = {0 }; +__le32 *src = {0 }; +__le64 phys_src; +__le64 phy_dest; +int ret; +int i; + +if (qproc->version !=MSS_MSM8996 ) +return 0 ; + +switch (qproc->protection_cmd) { +case ASSIG_ACCESS_MSA : { + src = (int [2 ]) {VMID_HLOS ,0 }; + dest = (int [2 ]) {VMID_MSS_MSA ,0 }; + perm = (int [2 ]) {PERM_READ |PERM_WRITE ,0 }; +break ; + } +case REMOV_ACCESS_MSA : { + src = (int [2 ]) {VMID_MSS_MSA ,0 }; + dest = (int [2 ]) {VMID_HLOS ,0 }; + perm = (int [2 ]) {PERM_READ |PERM_WRITE |PERM_EXEC ,0 }; +break ; + } +case SHARE_ACCESS_MSA : { + src = (int [2 ]) {VMID_HLOS ,0 }; + dest = (int [2 ]) {VMID_HLOS ,VMID_MSS_MSA }; + perm = (int [2 ]) {PERM_RW ,PERM_RW }; + dest_count =2 ; +break ; + } +case REMOV_SHARE_MSA : { + src = (int [2 ]) {VMID_HLOS ,VMID_MSS_MSA }; + dest = (int [2 ]) {VMID_HLOS ,0 }; + perm = (int [2 ]) {PERM_READ |PERM_WRITE |PERM_EXEC ,0 }; + src_count =2 ; +break ; + } +default : { +break ; + } + } + + source_vm_copy =dma_alloc_attrs (qproc->dev, + src_count*sizeof (*source_vm_copy), &phys_src, +GFP_KERNEL , dma_attrs); +if (!source_vm_copy) { +dev_err (qproc->dev, +"failed to allocate buffer to pass source vmid detail\n" ); +return -ENOMEM ; + } +memcpy (source_vm_copy, src,sizeof (*source_vm_copy) * src_count); + + dest_info =dma_alloc_attrs (qproc->dev, + dest_count*sizeof (*dest_info), &phy_dest, +GFP_KERNEL , dma_attrs); +if (!dest_info) { +dev_err (qproc->dev, +"failed to allocate buffer to pass destination vmid detail\n" ); +return -ENOMEM ; + } +for (i =0 ; i < dest_count; i++) { + dest_info[i].vm = dest[i]; + dest_info[i].perm = perm[i]; + dest_info[i].ctx =NULL ; + dest_info[i].ctx_size =0 ; + } + + mem_prot_info =dma_alloc_attrs (qproc->dev, +sizeof (*mem_prot_info), &mem_prot_phy, +GFP_KERNEL , dma_attrs); +if (!dest_info) { +dev_err (qproc->dev, +"failed to allocate buffer to pass protected mem detail\n" ); +return -ENOMEM ; + } + mem_prot_info->addr = addr; + mem_prot_info->size = size; + + desc.args[0 ] = mem_prot_phy; + desc.args[1 ] =sizeof (*mem_prot_info); + desc.args[2 ] = phys_src; + desc.args[3 ] =sizeof (*source_vm_copy) * src_count; + desc.args[4 ] = phy_dest; + desc.args[5 ] = dest_count*sizeof (*dest_info); + desc.args[6 ] =0 ; + + ret =qcom_scm_assign_mem (&desc); +if (ret) +pr_info ("%s: Failed to assign memory protection, ret = %d\n" , + __func__, ret); + +/* SCM call has returned free up buffers passed to secure domain */ +dma_free_attrs (qproc->dev, src_count*sizeof (*source_vm_copy), + source_vm_copy, phys_src, dma_attrs); +dma_free_attrs (qproc->dev, dest_count*sizeof (*dest_info), + dest_info, phy_dest, dma_attrs); +dma_free_attrs (qproc->dev,sizeof (*mem_prot_info), mem_prot_info, + mem_prot_phy, dma_attrs); +return ret; +} +static int q6v5_load (struct rproc *rproc,const struct firmware *fw) {struct q6v5 *qproc = rproc->priv;memcpy (qproc->mba_region, fw->data, fw->size); -return 0 ; }
@@ -446,6 +596,14 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)memcpy (ptr, fw->data, fw->size); +/* Hypervisor support to access metadata by modem */ + qproc->protection_cmd =ASSIG_ACCESS_MSA ; + ret =hyp_mem_access (qproc, phys,ALIGN (fw->size,SZ_4K )); +if (ret) { +dev_err (qproc->dev,"Failed to assign memory, ret - %d\n" , ret); +return -ENOMEM ; + } +writel (phys, qproc->rmb_base +RMB_PMI_META_DATA_REG );writel (RMB_CMD_META_DATA_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG );
@@ -454,7 +612,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)dev_err (qproc->dev,"MPSS header authentication timed out\n" ); else if (ret < 0) dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret); - +/* Metadata authentication done, remove modem access */ + qproc->protection_cmd =REMOV_ACCESS_MSA ; + ret =hyp_mem_access (qproc, phys,ALIGN (fw->size,SZ_4K )); +if (ret) +dev_err (qproc->dev,"Failed to assign memory, ret - %d\n" , ret);dma_free_attrs (qproc->dev, fw->size, ptr, phys, dma_attrs);return ret <0 ? ret :0 ;
@@ -483,6 +645,13 @@ static int q6v5_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)else boot_addr = fw_addr; +/* Hypervisor support to modem to access modem fw */ + qproc->protection_cmd =SHARE_ACCESS_MSA ; + ret =hyp_mem_access (qproc, boot_addr, qproc->mpss_size); +if (ret) { +dev_err (qproc->dev,"Failed to assign memory, ret - %d\n" , ret); +return -ENOMEM ; + } ehdr = (struct elf32_hdr *)fw->data; phdrs = (struct elf32_phdr *)(ehdr +1 );for (i =0 ; i < ehdr->e_phnum; i++, phdr++) {
@@ -595,6 +764,12 @@ static int q6v5_start(struct rproc *rproc) goto assert_reset; } + qproc->protection_cmd =ASSIG_ACCESS_MSA ; + ret =hyp_mem_access (qproc, qproc->mba_phys, qproc->mba_size); +if (ret) { +dev_err (qproc->dev,"Failed to assign memory, ret - %d\n" , ret); + goto assert_reset; + }writel (qproc->mba_phys, qproc->rmb_base +RMB_MBA_IMAGE_REG ); ret =q6v5proc_reset (qproc);
@@ -616,16 +791,21 @@ static int q6v5_start(struct rproc *rproc) ret =q6v5_mpss_load (qproc);if (ret) - goto halt_axi_ports; + goto reclaim_mem; ret =wait_for_completion_timeout (&qproc->start_done,msecs_to_jiffies (5000 ));if (ret ==0 ) {dev_err (qproc->dev,"start timed out\n" ); ret = -ETIMEDOUT ; - goto halt_axi_ports; + goto reclaim_mem; } + qproc->protection_cmd =REMOV_ACCESS_MSA ; + ret =hyp_mem_access (qproc, qproc->mba_phys, qproc->mba_size); +if (ret) +dev_err (qproc->dev, +"Failed to reclaim memory, ret - %d\n" , ret); qproc->running = true; q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
@@ -634,7 +814,18 @@ static int q6v5_start(struct rproc *rproc) qproc->proxy_reg_count);return 0 ; +reclaim_mem: + qproc->protection_cmd =REMOV_SHARE_MSA ; + ret =hyp_mem_access (qproc, qproc->mpss_phys, qproc->mpss_size); +if (ret) +dev_err (qproc->dev, +"Failed to reclaim memory, ret - %d\n" , ret); halt_axi_ports: + qproc->protection_cmd =REMOV_ACCESS_MSA ; + ret =hyp_mem_access (qproc, qproc->mba_phys, qproc->mba_size); +if (ret) +dev_err (qproc->dev, +"Failed to reclaim memory, ret - %d\n" , ret);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_q6);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_modem);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_nc);
@@ -977,6 +1168,7 @@ static int q6v5_probe(struct platform_device *pdev)if (ret) goto free_rproc; + qproc->version = desc->version; ret =q6v5_request_irq (qproc, pdev,"wdog" , q6v5_wdog_interrupt);if (ret <0 ) goto free_rproc;
@@ -1056,6 +1248,7 @@ static int q6v5_remove(struct platform_device *pdev)"mem" ,NULL }, + .version = MSS_MSM8916, }; static const struct rproc_hexagon_res msm8974_mss = {
@@ -1093,6 +1286,7 @@ static int q6v5_remove(struct platform_device *pdev)"mem" ,NULL }, + .version = MSS_MSM8974, };static const struct of_device_id q6v5_of_match[] = { { .compatible = "qcom,q6v5-pil", .data = &msm8916_mss},
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 v2 2/3] remoteproc: qcom: Add scm call to protect modem mem in mss rproc drv.
Quoted 7 lines · Avaneesh Kumar Dwivedi
This patch add hypervisor call support for second stage translation from mss remoteproc driver, this is required so that modem on msm8996 which is based on armv8 architecture can access DDR region where modem firmware are loaded.
Signed-off-by Avaneesh Kumar Dwivedi <[email protected]>---
Most of this should be combined with patch 1 from this series.
Quoted 17 lines
drivers/remoteproc/qcom_q6v5_pil.c | 202 ++++++++++++++++++++++++++++++++++++- 1 file changed, 198 insertions(+), 4 deletions(-)drivers/remoteproc/qcom_q6v5_pil.c +6
-0@@ -93,6 +93,23 @@#define QDSS_BHS_ON BIT(21)#define QDSS_LDO_BYP BIT(22) +struct dest_vm_and_perm_info { +__le32 vm; +__le32 perm; +__le32 *ctx; +__le32 ctx_size; +};
drivers/remoteproc/qcom_q6v5_pil.c line 111
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+ +struct mem_prot_info { +__le64 addr; +__le64 size; +}; + +struct scm_desc { +__le32 arginfo; +__le64 args[10 ]; +};
These are all firmware specific things that should live in scm code, not PIL code.
Quoted 29 lines
drivers/remoteproc/qcom_q6v5_pil.c +13
-0+struct reg_info {struct regulator *reg;int uV;@@ -111,6 +128,7 @@ struct rproc_hexagon_res {struct qcom_mss_reg_res active_supply[2 ];char **proxy_clk_names;char **active_clk_names; +int version; };struct q6v5 {@@ -152,8 +170,29 @@ struct q6v5 {phys_addr_t mpss_reloc;void *mpss_region;size_t mpss_size; +int version; +int protection_cmd; +}; + +enum { +MSS_MSM8916 , +MSS_MSM8974 , +MSS_MSM8996 , }; +enum { +ASSIG_ACCESS_MSA , +REMOV_ACCESS_MSA ,
drivers/remoteproc/qcom_q6v5_pil.c struct q6v5 { · line 195
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+SHARE_ACCESS_MSA , + REMOV_SHARE_MSA, +}; + +#define VMID_HLOS 0x3 +#define VMID_MSS_MSA 0xF +#define PERM_READ 0x4 +#define PERM_WRITE 0x2 +#define PERM_EXEC 0x1 +#define PERM_RW (0x4 | 0x2)
Please USE PERM_READ | PERM_WRITE here instead.
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 315
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) {@@ -273,12 +312,123 @@ static void q6v5_clk_disable(struct device *dev,clk_disable_unprepare (clks[i]); } +int hyp_mem_access (struct q6v5 *qproc,phys_addr_t addr,size_t size)
This could be the scm API for now. But instead of taking qproc, it would take the protection_cmd variable (which looks sort of like a state machine BTW). To be more generic, perhaps it should take the src vmids + num src vmids, dst vmids + num dst vmids, and protection flags (which looks to be same size as dst vmid array). If we can get the right SCM API here everything else should fall into place.
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 327
Quoted 12 lines
drivers/remoteproc/qcom_q6v5_pil.c +12
-0+{ +unsigned long dma_attrs =DMA_ATTR_FORCE_CONTIGUOUS ; +struct dest_vm_and_perm_info *dest_info; +struct mem_prot_info *mem_prot_info; +struct scm_desc desc = {0 }; +__le32 *source_vm_copy; +__le64 mem_prot_phy; +int dest_count =1 ; +int src_count =1 ; +__le32 *perm = {0 }; +__le32 *dest = {0 }; +__le32 *src = {0 };
src/dst seem like pretty confusing names. It makes it sound like a memcpy operation. Perhaps from/to is better? Or current/next.
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 340
Quoted 13 lines
drivers/remoteproc/qcom_q6v5_pil.c +13
-0+__le64 phys_src; +__le64 phy_dest; +int ret; +int i; + +if (qproc->version !=MSS_MSM8996 ) +return 0 ; + +switch (qproc->protection_cmd) { +case ASSIG_ACCESS_MSA : { + src = (int [2 ]) {VMID_HLOS ,0 }; + dest = (int [2 ]) {VMID_MSS_MSA ,0 }; + perm = (int [2 ]) {PERM_READ |PERM_WRITE ,0 };
Why have two element arrays when we're only using the first element? Relying on default src_count of 1 is very confusing.
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 346
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +6
-0+break ; + } +case REMOV_ACCESS_MSA : { + src = (int [2 ]) {VMID_MSS_MSA ,0 }; + dest = (int [2 ]) {VMID_HLOS ,0 }; + perm = (int [2 ]) {PERM_READ |PERM_WRITE |PERM_EXEC ,0 };
Same here.
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 352
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +6
-0+break ; + } +case SHARE_ACCESS_MSA : { + src = (int [2 ]) {VMID_HLOS ,0 }; + dest = (int [2 ]) {VMID_HLOS ,VMID_MSS_MSA }; + perm = (int [2 ]) {PERM_RW ,PERM_RW };
Please add spaces around curly braces like { this }
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 356
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0+ dest_count =2 ; +break ; + } +case REMOV_SHARE_MSA : {
And write REMOVE_SHARE_MSA, ASSIGN_ACCESS_MSA, etc. instead of dropping letters.
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 363
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +7
-0+ src = (int [2 ]) {VMID_HLOS ,VMID_MSS_MSA }; + dest = (int [2 ]) {VMID_HLOS ,0 }; + perm = (int [2 ]) {PERM_READ |PERM_WRITE |PERM_EXEC ,0 }; + src_count =2 ; +break ; + } +default : {
And also drop curly braces around case statements.
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 368
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+break ; + } + } + + source_vm_copy = dma_alloc_attrs(qproc->dev,
This should really allocate from the scm->dev instead of qproc. We don't know if qproc has the same DMA attributes that the firmware has, but we know that we're trying to relay information to the firmware here, not the qproc.
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+ src_count*sizeof (*source_vm_copy), &phys_src, + GFP_KERNEL, dma_attrs); +if (!source_vm_copy) { +dev_err (qproc->dev, +"failed to allocate buffer to pass source vmid detail\n" ); +return -ENOMEM ; + } +memcpy (source_vm_copy, src,sizeof (*source_vm_copy) * src_count); + + dest_info = dma_alloc_attrs(qproc->dev,
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 388
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+ dest_count*sizeof (*dest_info), &phy_dest, + GFP_KERNEL, dma_attrs); +if (!dest_info) { +dev_err (qproc->dev, +"failed to allocate buffer to pass destination vmid detail\n" ); +return -ENOMEM ; + } +for (i =0 ; i < dest_count; i++) { + dest_info[i].vm = dest[i]; + dest_info[i].perm = perm[i];
Needs to do a cpu_to_le32() somewhere. Please run sparse.
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 391
Quoted 3 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0+ dest_info[i].ctx =NULL ; + dest_info[i].ctx_size =0 ; + }
Perhaps we should just allocate these first (one or two elements isn't a big difference) and then fill the details in directly.
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +8
-0+ + mem_prot_info =dma_alloc_attrs (qproc->dev, +sizeof (*mem_prot_info), &mem_prot_phy, +GFP_KERNEL , dma_attrs); +if (!dest_info) { +dev_err (qproc->dev, +"failed to allocate buffer to pass protected mem detail\n" ); +return -ENOMEM ;
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 409
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+ } + mem_prot_info->addr = addr; + mem_prot_info->size = size; + + desc.args[0 ] = mem_prot_phy; + desc.args[1 ] =sizeof (*mem_prot_info); + desc.args[2 ] = phys_src; + desc.args[3 ] =sizeof (*source_vm_copy) * src_count; + desc.args[4 ] = phy_dest; + desc.args[5 ] = dest_count*sizeof (*dest_info);
Please add spaces around '*'. Run checkpatch.
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 414
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+ desc.args[6 ] =0 ; + + ret =qcom_scm_assign_mem (&desc); +if (ret) + pr_info("%s: Failed to assign memory protection, ret = %d\n",
pr_err? dev_err?
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +8
-0+ __func__, ret); + +/* SCM call has returned free up buffers passed to secure domain */ +dma_free_attrs (qproc->dev, src_count*sizeof (*source_vm_copy), + source_vm_copy, phys_src, dma_attrs); +dma_free_attrs (qproc->dev, dest_count*sizeof (*dest_info), + dest_info, phy_dest, dma_attrs); + dma_free_attrs(qproc->dev, sizeof(*mem_prot_info), mem_prot_info,
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 281
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-1+ mem_prot_phy, dma_attrs); +return ret; +} +static int q6v5_load (struct rproc *rproc,const struct firmware *fw) {struct q6v5 *qproc = rproc->priv;memcpy (qproc->mba_region, fw->data, fw->size); -
Please remove this patch noise.
Quoted 2 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0return 0 ; }
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v2 2/3] remoteproc: qcom: Add scm call to protect modem mem in mss rproc drv.
Quoted 9 lines · Stephen Boyd
Quoted 7 lines · Avaneesh Kumar Dwivedi
This patch add hypervisor call support for second stage translation from mss remoteproc driver, this is required so that modem on msm8996 which is based on armv8 architecture can access DDR region where modem firmware are loaded.
Signed-off-by Avaneesh Kumar Dwivedi <[email protected]>---
Most of this should be combined with patch 1 from this series.
OK.
Quoted 22 lines
Quoted 22 lines
drivers/remoteproc/qcom_q6v5_pil.c | 202 ++++++++++++++++++++++++++++++++++++- 1 file changed, 198 insertions(+), 4 deletions(-)drivers/remoteproc/qcom_q6v5_pil.c +10
-0@@ -93,6 +93,23 @@#define QDSS_BHS_ON BIT(21)#define QDSS_LDO_BYP BIT(22) +struct dest_vm_and_perm_info { +__le32 vm; +__le32 perm; +__le32 *ctx; +__le32 ctx_size; +}; + +struct mem_prot_info { +__le64 addr; +__le64 size;
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c line 111
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +6
-0+}; + +struct scm_desc { +__le32 arginfo; +__le64 args[10 ]; +};These are all firmware specific things that should live in scm code, not PIL code.
OK.
Quoted 27 lines
Quoted 27 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+struct reg_info {struct regulator *reg;int uV;@@ -111,6 +128,7 @@ struct rproc_hexagon_res {struct qcom_mss_reg_res active_supply[2 ];char **proxy_clk_names;char **active_clk_names; +int version; };struct q6v5 {@@ -152,8 +170,29 @@ struct q6v5 {phys_addr_t mpss_reloc;void *mpss_region;size_t mpss_size; +int version; +int protection_cmd; +}; + +enum { +MSS_MSM8916 , +MSS_MSM8974 , +MSS_MSM8996 , };
Quoted 14 lines
drivers/remoteproc/qcom_q6v5_pil.c struct q6v5 { · line 195
Quoted 13 lines
drivers/remoteproc/qcom_q6v5_pil.c +13
-0+enum { +ASSIG_ACCESS_MSA , +REMOV_ACCESS_MSA , +SHARE_ACCESS_MSA , +REMOV_SHARE_MSA , +}; + +#define VMID_HLOS 0x3 +#define VMID_MSS_MSA 0xF +#define PERM_READ 0x4 +#define PERM_WRITE 0x2 +#define PERM_EXEC 0x1 +#define PERM_RW (0x4 | 0x2)Please USE
PERM_READ|PERM_WRITEhere instead.
OK.
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) {@@ -273,12 +312,123 @@ static void q6v5_clk_disable(struct device *dev,clk_disable_unprepare (clks[i]); }
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 315
Quoted 1 line
drivers/remoteproc/qcom_q6v5_pil.c +1
-0+int hyp_mem_access (struct q6v5 *qproc,phys_addr_t addr,size_t size)This could be the scm API for now. But instead of taking qproc, it would take the
protection_cmdvariable (which looks sort of like a state machine BTW). To be more generic, perhaps it should take the src vmids + num src vmids, dst vmids + num dst vmids, and protection flags (which looks to be same size as dst vmid array). If we can get the right SCM API here everything else should fall into place.
Will analyses and modify as per suggestion.
Quoted 15 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 327
Quoted 13 lines
drivers/remoteproc/qcom_q6v5_pil.c +12
-0+{ +unsigned long dma_attrs =DMA_ATTR_FORCE_CONTIGUOUS ; +struct dest_vm_and_perm_info *dest_info; +struct mem_prot_info *mem_prot_info; +struct scm_desc desc = {0 }; +__le32 *source_vm_copy; +__le64 mem_prot_phy; +int dest_count =1 ; +int src_count =1 ; +__le32 *perm = {0 }; +__le32 *dest = {0 }; +__le32 *src = {0 };src/dst seem like pretty confusing names. It makes it sound like a memcpy operation. Perhaps from/to is better? Or current/next.
OK
Quoted 16 lines
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0+__le64 phys_src; +__le64 phy_dest; +int ret;drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 340
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+int i; + +if (qproc->version !=MSS_MSM8996 ) +return 0 ; + +switch (qproc->protection_cmd) { +case ASSIG_ACCESS_MSA : { + src = (int [2 ]) {VMID_HLOS ,0 }; + dest = (int [2 ]) {VMID_MSS_MSA ,0 }; + perm = (int [2 ]) {PERM_READ |PERM_WRITE ,0 };Why have two element arrays when we're only using the first element? Relying on default
src_countof 1 is very confusing.
in some cases we initialize two elements.
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 346
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +6
-0+break ; + } +case REMOV_ACCESS_MSA : { + src = (int [2 ]) {VMID_MSS_MSA ,0 }; + dest = (int [2 ]) {VMID_HLOS ,0 }; + perm = (int [2 ]) {PERM_READ |PERM_WRITE |PERM_EXEC ,0 };Same here.
OK.
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 352
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +6
-0+break ; + } +case SHARE_ACCESS_MSA : { + src = (int [2 ]) {VMID_HLOS ,0 }; + dest = (int [2 ]) {VMID_HLOS ,VMID_MSS_MSA }; + perm = (int [2 ]) {PERM_RW ,PERM_RW };Please add spaces around curly braces like { this }
OK.
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 356
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0+ dest_count =2 ; +break ; + } +case REMOV_SHARE_MSA : {And write
REMOVE_SHARE_MSA,ASSIGN_ACCESS_MSA, etc. instead of dropping letters.
OK.
Quoted 9 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 363
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +7
-0+ src = (int [2 ]) {VMID_HLOS ,VMID_MSS_MSA }; + dest = (int [2 ]) {VMID_HLOS ,0 }; + perm = (int [2 ]) {PERM_READ |PERM_WRITE |PERM_EXEC ,0 }; + src_count =2 ; +break ; + } +default : {And also drop curly braces around case statements.
OK.
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 368
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+break ; + } + } + + source_vm_copy = dma_alloc_attrs(qproc->dev,This should really allocate from the
scm->devinstead of qproc. We don't know if qproc has the same DMA attributes that the firmware has, but we know that we're trying to relay information to the firmware here, not the qproc.
OK, agree.
Quoted 14 lines
Quoted 14 lines
drivers/remoteproc/qcom_q6v5_pil.c +13
-0+ src_count*sizeof (*source_vm_copy), &phys_src, + GFP_KERNEL, dma_attrs); +if (!source_vm_copy) { +dev_err (qproc->dev, +"failed to allocate buffer to pass source vmid detail\n" ); +return -ENOMEM ; + } +memcpy (source_vm_copy, src,sizeof (*source_vm_copy) * src_count); + + dest_info =dma_alloc_attrs (qproc->dev, + dest_count*sizeof (*dest_info), &phy_dest, +GFP_KERNEL , dma_attrs); +if (!dest_info) {
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 388
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +7
-0+dev_err (qproc->dev, +"failed to allocate buffer to pass destination vmid detail\n" ); +return -ENOMEM ; + } +for (i =0 ; i < dest_count; i++) { + dest_info[i].vm = dest[i]; + dest_info[i].perm = perm[i];Needs to do a
cpu_to_le32()somewhere. Please run sparse.
I understand that byte reordering needed but not sure what is sparse will check and do it.
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 391
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0+ dest_info[i].ctx =NULL ; + dest_info[i].ctx_size =0 ; + }Perhaps we should just allocate these first (one or two elements isn't a big difference) and then fill the details in directly.
Not very clear what is ask here?
Quoted 12 lines
Quoted 12 lines
drivers/remoteproc/qcom_q6v5_pil.c +11
-0+ + mem_prot_info =dma_alloc_attrs (qproc->dev, +sizeof (*mem_prot_info), &mem_prot_phy, +GFP_KERNEL , dma_attrs); +if (!dest_info) { +dev_err (qproc->dev, +"failed to allocate buffer to pass protected mem detail\n" ); +return -ENOMEM ; + } + mem_prot_info->addr = addr; + mem_prot_info->size = size;
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 409
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +7
-0+ + desc.args[0 ] = mem_prot_phy; + desc.args[1 ] =sizeof (*mem_prot_info); + desc.args[2 ] = phys_src; + desc.args[3 ] =sizeof (*source_vm_copy) * src_count; + desc.args[4 ] = phy_dest; + desc.args[5 ] = dest_count*sizeof (*dest_info);Please add spaces around '*'. Run checkpatch.
OK.
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 414
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+ desc.args[6 ] =0 ; + + ret =qcom_scm_assign_mem (&desc); +if (ret) + pr_info("%s: Failed to assign memory protection, ret = %d\n",
pr_err?dev_err?
Will correct.
Quoted 17 lines
Quoted 17 lines
drivers/remoteproc/qcom_q6v5_pil.c +12
-0+ __func__, ret); + +/* SCM call has returned free up buffers passed to secure domain */ +dma_free_attrs (qproc->dev, src_count*sizeof (*source_vm_copy), + source_vm_copy, phys_src, dma_attrs); +dma_free_attrs (qproc->dev, dest_count*sizeof (*dest_info), + dest_info, phy_dest, dma_attrs); +dma_free_attrs (qproc->dev,sizeof (*mem_prot_info), mem_prot_info, + mem_prot_phy, dma_attrs); +return ret; +} +static int q6v5_load (struct rproc *rproc,const struct firmware *fw) {struct q6v5 *qproc = rproc->priv;
Quoted 3 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 281
Quoted 2 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-1memcpy (qproc->mba_region, fw->data, fw->size); -Please remove this patch noise.
OK.
Quoted 3 lines
Quoted 3 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0return 0 ; }
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 v2 2/3] remoteproc: qcom: Add scm call to protect modem mem in mss rproc drv.
Quoted 7 lines · Avaneesh Kumar (avani)
Quoted 4 lines · Stephen Boyd
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 388
Quoted 2 lines · Avaneesh Kumar Dwivedi
drivers/remoteproc/qcom_q6v5_pil.c +2
-0+ dest_info[i].vm = dest[i]; + dest_info[i].perm = perm[i];Needs to do a
cpu_to_le32()somewhere. Please run sparse.I understand that byte reordering needed but not sure what is sparse will check and do it.
http://git.kernel.org/cgit/devel/sparse/sparse.git/
Quoted 8 lines
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 391
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +3
-0+ dest_info[i].ctx =NULL ; + dest_info[i].ctx_size =0 ; + }Perhaps we should just allocate these first (one or two elements isn't a big difference) and then fill the details in directly.
Not very clear what is ask here?
Quoted 1 line
I'm saying we could fill in dest_info in the case statement instead of here in an up to 2 times loop.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v2 3/3 remoteproc: qcom: Add msm8996 specific changes in mss rproc driver.
In reply to Avaneesh Kumar DwivediThis patch add msm8996 support in existing mss rproc driver.
--- .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 1 + drivers/remoteproc/qcom_q6v5_pil.c | 178 ++++++++++++++++++--- 2 files changed, 154 insertions(+), 25 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 +153 -25
230 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: [PATCH v2 3/3] remoteproc: qcom: Add msm8996 specific changes in mss rproc driver.
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms) · line 500
Quoted 10 lines · Avaneesh Kumar Dwivedi
drivers/remoteproc/qcom_q6v5_pil.c +1
-1@@ -485,35 +497,99 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)static int q6v5proc_reset (struct q6v5 *qproc) { -u32 val; +u64 val;
Why u64? There isn't any readq/writeq usage here.
Quoted 3 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-0int ret; +int i;
[...]
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms) · line 512
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +8
-0+if (qproc->version ==MSS_MSM8996 ) { +/* Override the ACC value if required */ +writel (QDSP6SS_ACC_OVERRIDE_VAL , + qproc->reg_base +QDSP6SS_STRAP_ACC ); + +/* Assert resets, stop core */ + val =readl (qproc->reg_base +QDSP6SS_RESET_REG ); + val |= (Q6SS_CORE_ARES |Q6SS_BUS_ARES_ENABLE |Q6SS_STOP_CORE );
Useless parenthesis.
Quoted 23 lines
drivers/remoteproc/qcom_q6v5_pil.c +22
-0+writel (val, qproc->reg_base +QDSP6SS_RESET_REG ); + +/* BHS require xo cbcr to be enabled */ + val =readl (qproc->reg_base +QDSP6SS_XO_CBCR ); + val |=0x1 ; +writel (val, qproc->reg_base +QDSP6SS_XO_CBCR ); +/* Read CLKOFF bit to go low indicating CLK is enabled */ + ret =readl_poll_timeout (qproc->reg_base +QDSP6SS_XO_CBCR , + val, !(val &BIT (31 )),1 ,HALT_CHECK_MAX_LOOPS ); +if (ret) { +dev_err (qproc->dev, +"xo cbcr enabling timed out (rc:%d)\n" , ret); +return ret; + } +/* Enable power block headswitch and wait for it to stabilize */ + val =readl (qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + val |= QDSP6v56_BHS_ON; +writel (val, qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + val |=readl (qproc->reg_base +QDSP6SS_PWR_CTL_REG ); +udelay (1 ); + +/* Put LDO in bypass mode */
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms) · line 545
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+ val |= QDSP6v56_LDO_BYP; +writel (val, qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + +/* Deassert QDSP6 compiler memory clamp */ + val =readl (qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + val &= ~QDSP6v56_CLAMP_QMC_MEM; +writel (val, qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + +/* Deassert memory peripheral sleep and L2 memory standby */ + val |= (Q6SS_L2DATA_STBY_N |Q6SS_SLP_RET_N );
Useless parenthesis.
Quoted 14 lines
drivers/remoteproc/qcom_q6v5_pil.c +14
-0+writel (val, qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + +/* Turn on L1, L2, ETB and JU memories 1 at a time */ + val =readl (qproc->reg_base +QDSP6SS_MEM_PWR_CTL ); +for (i =19 ; i >=0 ; i--) { + val |=BIT (i); +writel (val, qproc->reg_base + +QDSP6SS_MEM_PWR_CTL ); +/* +* Read back value to ensure the write is done then +* wait for 1us for both memory peripheral and data +* array to turn on. +*/ + val |=readl (qproc->reg_base +QDSP6SS_MEM_PWR_CTL );
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms) · line 569
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+udelay (1 ); + } +/* Remove word line clamp */ + val =readl (qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + val &= ~QDSP6v56_CLAMP_WL; +writel (val, qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + }else { +/* Assert resets, stop core */ + val =readl (qproc->reg_base +QDSP6SS_RESET_REG ); + val |= (Q6SS_CORE_ARES |Q6SS_BUS_ARES_ENABLE |Q6SS_STOP_CORE );
Useless parenthesis.
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_stop(struct rproc *rproc) · line 928
Quoted 13 lines
drivers/remoteproc/qcom_q6v5_pil.c +9
-0+writel (val, qproc->reg_base +QDSP6SS_RESET_REG ); + +/* Enable power block headswitch and wait for it to stabilize */ + val =readl (qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + val |=QDSS_BHS_ON |QDSS_LDO_BYP ; +writel (val, qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + val |=readl (qproc->reg_base +QDSP6SS_PWR_CTL_REG ); +udelay (1 );@@ -849,6 +925,7 @@ static int q6v5_stop(struct rproc *rproc) {struct q6v5 *qproc = (struct q6v5 *)rproc->priv;int ret; +int val;
u32 instead of int?
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_stop(struct rproc *rproc) · line 949
Quoted 11 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0qproc->running = false;@@ -866,6 +943,15 @@ static int q6v5_stop(struct rproc *rproc)q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_modem);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_nc); +if (qproc->version ==MSS_MSM8996 ) { +/* +* To avoid high MX current during LPASS/MSS restart. +*/
Three lines could be one line comment instead.
Quoted 14 lines
drivers/remoteproc/qcom_q6v5_pil.c +7
-0+ val =readl (qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + val |=Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL | + QDSP6v56_CLAMP_QMC_MEM; +writel (val, qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + }reset_control_assert (qproc->mss_restart);q6v5_clk_disable (qproc->dev, qproc->active_clks, qproc->active_clk_count);@@ -1213,6 +1299,47 @@ static int q6v5_remove(struct platform_device *pdev)return 0 ; } +static const struct rproc_hexagon_res msm8996_mss = { + .hexagon_mba_image ="mba.mbn" ,
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_remove(struct platform_device *pdev) · line 1313
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+ .proxy_supply = (struct qcom_mss_reg_res[]) { + { + .supply ="vdd_mx" , + .uV =6 , + }, + { + .supply ="vdd_cx" , + .uV =7 , + .uA =100000 , + },
vdd cx and vdd mx are corners. The plan is to _not_ use the regulator framework for those, so treating them like supplies is incorrect here.
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +7
-0+ { + .supply = "vdd_pll", + .uV = 1800000, + .uA = 100000, + }, + {} + },
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v2 3/3] remoteproc: qcom: Add msm8996 specific changes in mss rproc driver.
Quoted 7 lines · Stephen Boyd
Quoted 6 lines · Avaneesh Kumar Dwivedi
drivers/remoteproc/qcom_q6v5_pil.c +0
-0@@ -485,35 +497,99 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms) · line 500
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-1static int q6v5proc_reset (struct q6v5 *qproc) { -u32 val; +u64 val;Why u64? There isn't any readq/writeq usage here.
OK
Quoted 5 lines
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-0int ret; +int i;[...]
Quoted 9 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms) · line 512
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +8
-0+if (qproc->version ==MSS_MSM8996 ) { +/* Override the ACC value if required */ +writel (QDSP6SS_ACC_OVERRIDE_VAL , + qproc->reg_base +QDSP6SS_STRAP_ACC ); + +/* Assert resets, stop core */ + val =readl (qproc->reg_base +QDSP6SS_RESET_REG ); + val |= (Q6SS_CORE_ARES |Q6SS_BUS_ARES_ENABLE |Q6SS_STOP_CORE );Useless parenthesis.
ok
Quoted 27 lines
Quoted 27 lines
drivers/remoteproc/qcom_q6v5_pil.c +25
-0+writel (val, qproc->reg_base +QDSP6SS_RESET_REG ); + +/* BHS require xo cbcr to be enabled */ + val =readl (qproc->reg_base +QDSP6SS_XO_CBCR ); + val |=0x1 ; +writel (val, qproc->reg_base +QDSP6SS_XO_CBCR ); +/* Read CLKOFF bit to go low indicating CLK is enabled */ + ret =readl_poll_timeout (qproc->reg_base +QDSP6SS_XO_CBCR , + val, !(val &BIT (31 )),1 ,HALT_CHECK_MAX_LOOPS ); +if (ret) { +dev_err (qproc->dev, +"xo cbcr enabling timed out (rc:%d)\n" , ret); +return ret; + } +/* Enable power block headswitch and wait for it to stabilize */ + val =readl (qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + val |= QDSP6v56_BHS_ON; +writel (val, qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + val |=readl (qproc->reg_base +QDSP6SS_PWR_CTL_REG ); +udelay (1 ); + +/* Put LDO in bypass mode */ + val |= QDSP6v56_LDO_BYP; +writel (val, qproc->reg_base +QDSP6SS_PWR_CTL_REG ); +
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms) · line 545
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +7
-0+/* Deassert QDSP6 compiler memory clamp */ + val =readl (qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + val &= ~QDSP6v56_CLAMP_QMC_MEM; +writel (val, qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + +/* Deassert memory peripheral sleep and L2 memory standby */ + val |= (Q6SS_L2DATA_STBY_N |Q6SS_SLP_RET_N );Useless parenthesis.
ok
Quoted 18 lines
Quoted 18 lines
drivers/remoteproc/qcom_q6v5_pil.c +17
-0+writel (val, qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + +/* Turn on L1, L2, ETB and JU memories 1 at a time */ + val =readl (qproc->reg_base +QDSP6SS_MEM_PWR_CTL ); +for (i =19 ; i >=0 ; i--) { + val |=BIT (i); +writel (val, qproc->reg_base + +QDSP6SS_MEM_PWR_CTL ); +/* +* Read back value to ensure the write is done then +* wait for 1us for both memory peripheral and data +* array to turn on. +*/ + val |=readl (qproc->reg_base +QDSP6SS_MEM_PWR_CTL ); +udelay (1 ); + } +/* Remove word line clamp */
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms) · line 569
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +7
-0+ val =readl (qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + val &= ~QDSP6v56_CLAMP_WL; +writel (val, qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + }else { +/* Assert resets, stop core */ + val =readl (qproc->reg_base +QDSP6SS_RESET_REG ); + val |= (Q6SS_CORE_ARES |Q6SS_BUS_ARES_ENABLE |Q6SS_STOP_CORE );Useless parenthesis.
ok
Quoted 15 lines
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +8
-0+writel (val, qproc->reg_base +QDSP6SS_RESET_REG ); + +/* Enable power block headswitch and wait for it to stabilize */ + val =readl (qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + val |=QDSS_BHS_ON |QDSS_LDO_BYP ; +writel (val, qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + val |=readl (qproc->reg_base +QDSP6SS_PWR_CTL_REG ); +udelay (1 );drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_stop(struct rproc *rproc) · line 928
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +1
-0@@ -849,6 +925,7 @@ static int q6v5_stop(struct rproc *rproc) {struct q6v5 *qproc = (struct q6v5 *)rproc->priv;int ret; +int val;u32 instead of int?
ok
Quoted 8 lines
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +0
-0qproc->running = false;@@ -866,6 +943,15 @@ static int q6v5_stop(struct rproc *rproc)q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_modem);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_nc);
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_stop(struct rproc *rproc) · line 949
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0+if (qproc->version ==MSS_MSM8996 ) { +/* +* To avoid high MX current during LPASS/MSS restart. +*/ Three lines could be one line comment instead.
ok.
Quoted 13 lines
Quoted 13 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+ val =readl (qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + val |=Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL | + QDSP6v56_CLAMP_QMC_MEM; +writel (val, qproc->reg_base +QDSP6SS_PWR_CTL_REG ); + }reset_control_assert (qproc->mss_restart);q6v5_clk_disable (qproc->dev, qproc->active_clks, qproc->active_clk_count);@@ -1213,6 +1299,47 @@ static int q6v5_remove(struct platform_device *pdev)return 0 ; }
Quoted 15 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_remove(struct platform_device *pdev) · line 1313
Quoted 12 lines
drivers/remoteproc/qcom_q6v5_pil.c +12
-0+static const struct rproc_hexagon_res msm8996_mss = { + .hexagon_mba_image ="mba.mbn" , + .proxy_supply = (struct qcom_mss_reg_res []) { + { + .supply ="vdd_mx" , + .uV =6 , + }, + { + .supply ="vdd_cx" , + .uV =7 , + .uA =100000 , + },vdd cx and vdd mx are corners. The plan is to _not_ use the regulator framework for those, so treating them like supplies is incorrect here.
vdd cx and mx though in downstream are voted for corner but they are always ON domain upstream as per regulator team when i discussed with them. should i drop them altogether?
Quoted 8 lines
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +7
-0+ { + .supply = "vdd_pll", + .uV = 1800000, + .uA = 100000, + }, + {} + },
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 v2 3/3] remoteproc: qcom: Add msm8996 specific changes in mss rproc driver.
Quoted 20 lines · Avaneesh Kumar (avani)
Quoted 11 lines · Stephen Boyd
Quoted 10 lines · Avaneesh Kumar Dwivedi
drivers/remoteproc/qcom_q6v5_pil.c +7
-0@@ -1213,6 +1299,47 @@ static int q6v5_remove(struct platform_device *pdev)return 0 ; } +static const struct rproc_hexagon_res msm8996_mss = { + .hexagon_mba_image ="mba.mbn" , + .proxy_supply = (struct qcom_mss_reg_res []) { + { + .supply ="vdd_mx" , + .uV =6 , + },Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static int q6v5_remove(struct platform_device *pdev) · line 1313
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +5
-0+ { + .supply = "vdd_cx", + .uV = 7, + .uA = 100000, + },vdd cx and vdd mx are corners. The plan is to _not_ use the regulator framework for those, so treating them like supplies is incorrect here.
Quoted 4 lines
vdd cx and mx though in downstream are voted for corner but they are always ON domain upstream as per regulator team when i discussed with them.
should i drop them altogether?
I would say yes, drop them. The on/off state doesn't matter here. This code wants to max out the corner for a period of time until the remote processor has booted far enough to make their own vote on these RPM resources.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project