Re: [PATCH v3 1/2] soc: qcom: Add support of scm call for mss rproc to share access of ddr
From: Dwivedi, Avaneesh Kumar (avani)
Date: Fri Apr 14 2017 - 10:01:42 EST
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.