Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752893AbdDNOBj (ORCPT ); Fri, 14 Apr 2017 10:01:39 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:59368 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751192AbdDNOBg (ORCPT ); Fri, 14 Apr 2017 10:01:36 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 33D7860D39 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=akdwived@codeaurora.org Subject: Re: [PATCH v3 1/2] soc: qcom: Add support of scm call for mss rproc to share access of ddr To: Stephen Boyd References: <1488996202-1546-1-git-send-email-akdwived@codeaurora.org> <1488996202-1546-2-git-send-email-akdwived@codeaurora.org> <20170405234448.GO7065@codeaurora.org> Cc: bjorn.andersson@linaro.org, agross@codeaurora.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-remoteproc@vger.kernel.org From: "Dwivedi, Avaneesh Kumar (avani)" Message-ID: Date: Fri, 14 Apr 2017 19:31:31 +0530 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170405234448.GO7065@codeaurora.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8206 Lines: 253 On 4/6/2017 5:14 AM, Stephen Boyd wrote: > On 03/08, Avaneesh Kumar Dwivedi wrote: >> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c >> index 4a0f5ea..187fc00 100644 >> --- a/drivers/firmware/qcom_scm-64.c >> +++ b/drivers/firmware/qcom_scm-64.c >> @@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) >> >> return ret ? : res.a1; >> } >> + >> +int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid) > Why are we passing a structure copy? I did not use struct pointer cause i am not modifying any of the passed value, do you think i should use struct pointer than pass by value? > >> +{ >> + int ret; >> + struct qcom_scm_desc desc = {0}; >> + struct arm_smccc_res res; >> + >> + desc.args[0] = vmid.fw_phy; >> + desc.args[1] = vmid.size_fw; >> + desc.args[2] = vmid.from_phy; >> + desc.args[3] = vmid.size_from; >> + desc.args[4] = vmid.to_phy; >> + desc.args[5] = vmid.size_to; > These should all cause sparse warnings because of incorrect > assignments. Given that these are all registers, I'm not sure why > the vmid_detail structure has __le32 in it. will fix. > >> + desc.args[6] = 0; >> + >> + desc.arginfo = QCOM_SCM_ARGS(7, QCOM_SCM_RO, QCOM_SCM_VAL, >> + QCOM_SCM_RO, QCOM_SCM_VAL, QCOM_SCM_RO, >> + QCOM_SCM_VAL, QCOM_SCM_VAL); >> + >> + ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP, >> + QCOM_MEM_PROT_ASSIGN_ID, >> + &desc, &res); >> + >> + return ret ? : res.a1; >> +} >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >> index 893f953ea..f137f34 100644 >> --- a/drivers/firmware/qcom_scm.c >> +++ b/drivers/firmware/qcom_scm.c >> @@ -42,6 +42,18 @@ struct qcom_scm { >> >> static struct qcom_scm *__scm; >> >> +struct dest_vm_and_perm_info { >> + __le32 vm; >> + __le32 perm; >> + __le32 *ctx; > Drop the pointer? Just treat it like another number? Pointer is > really odd because it doesn't really make any sense what the > address of the pointer would be. Downstream this is pointer though unused, that is why kept same will check and change. > >> + __le32 ctx_size; >> +}; >> + >> +struct fw_region_info { >> + __le64 addr; >> + __le64 size; >> +}; >> + >> static int qcom_scm_clk_enable(void) >> { >> int ret; >> @@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral) >> } >> EXPORT_SYMBOL(qcom_scm_pas_shutdown); >> >> +/** >> + * qcom_scm_assign_mem() - Allocate and fill vmid detail of old >> + * new owners of memory region for fw and metadata etc, Which is >> + * further passed to hypervisor, which does translate intermediate >> + * physical address used by subsystems. > Maybe this can be the long description, but the short description > should be shorter. OK :( > >> + * @vmid: structure with pointers and size detail of old and new >> + * owners vmid detail. >> + * Return 0 on success. > There's a standard syntax for return too. Look at kernel doc > howto please. OK. > >> + */ >> +int qcom_scm_assign_mem(struct vmid_detail vmid) > Please no structure copy. should struct pointer be OK, or direct argument passing? > >> +{ >> + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS; >> + struct dest_vm_and_perm_info *to; >> + struct fw_region_info *fw_info; >> + __le64 fw_phy; >> + __le32 *from; >> + int ret = -ENOMEM; > Not sure why we assign it. It gets overwritten with the first use. Yes will correct. > >> + int i; >> + >> + from = dma_alloc_attrs(__scm->dev, vmid.size_from, >> + &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), >> + &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. >> + if (!fw_info) { >> + dev_err(__scm->dev, >> + "failed to allocate buffer to pass firmware detail\n"); >> + goto free_dest_buff; >> + } >> + >> + /* copy detail of original owner of ddr region */ >> + /* in physically contigious buffer */ >> + memcpy(from, vmid.from, vmid.size_from); >> + >> + /* fill details of new owners of ddr region*/ >> + /* in physically contigious buffer */ >> + for (i = 0; i < (vmid.size_to / sizeof(__le32)); i++) { >> + to[i].vm = vmid.to[i]; >> + to[i].perm = vmid.permission[i]; > Here you want the cpu_to_le32() stuff. Please run sparse. OK, last time i tried running sparse but seems some environment problem, it did not gave any warning. > >> + 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. > >> + 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? > >> +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) >> { >> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h >> index 3584b00..4665a11 100644 >> --- a/drivers/firmware/qcom_scm.h >> +++ b/drivers/firmware/qcom_scm.h >> @@ -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? Will check, but i thought its not there. > >> +#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 >> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c >> index 8fd697a..62ad976 100644 >> --- a/drivers/remoteproc/qcom_q6v5_pil.c >> +++ b/drivers/remoteproc/qcom_q6v5_pil.c >> @@ -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? Yes, will correct. > >> >> static const struct of_device_id q6v5_of_match[] = { >> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h >> index cc32ab8..cb0b7ee 100644 >> --- a/include/linux/qcom_scm.h >> +++ b/include/linux/qcom_scm.h >> @@ -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; > 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. OK. > -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.