Messages in this thread Patch in this message | | | Date | Mon, 30 Oct 2023 18:40:28 +0530 | Subject | Re: [PATCH v2 2/3] remoteproc: qcom: pas: make region assign more generic | From | Mukesh Ojha <> |
| |
On 10/30/2023 3:33 PM, Neil Armstrong wrote: > The current memory region assign only supports a single > memory region. > > But new platforms introduces more regions to make the > memory requirements more flexible for various use cases. > Those new platforms also shares the memory region between the > DSP and HLOS. > > To handle this, make the region assign more generic in order > to support more than a single memory region and also permit > setting the regions permissions as shared. > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/remoteproc/qcom_q6v5_pas.c | 102 ++++++++++++++++++++++++------------- > 1 file changed, 66 insertions(+), 36 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c > index 913a5d2068e8..4829fd26e17d 100644 > --- a/drivers/remoteproc/qcom_q6v5_pas.c > +++ b/drivers/remoteproc/qcom_q6v5_pas.c > @@ -33,6 +33,8 @@ > > #define ADSP_DECRYPT_SHUTDOWN_DELAY_MS 100 > > +#define MAX_ASSIGN_COUNT 2 > + > struct adsp_data { > int crash_reason_smem; > const char *firmware_name; > @@ -51,6 +53,9 @@ struct adsp_data { > int ssctl_id; > > int region_assign_idx; > + int region_assign_count; > + bool region_assign_shared; > + int region_assign_vmid; > }; > > struct qcom_adsp { > @@ -87,15 +92,18 @@ struct qcom_adsp { > phys_addr_t dtb_mem_phys; > phys_addr_t mem_reloc; > phys_addr_t dtb_mem_reloc; > - phys_addr_t region_assign_phys; > + phys_addr_t region_assign_phys[MAX_ASSIGN_COUNT]; > void *mem_region; > void *dtb_mem_region; > size_t mem_size; > size_t dtb_mem_size; > - size_t region_assign_size; > + size_t region_assign_size[MAX_ASSIGN_COUNT]; > > int region_assign_idx; > - u64 region_assign_perms; > + int region_assign_count; > + bool region_assign_shared; > + int region_assign_vmid; > + u64 region_assign_perms[MAX_ASSIGN_COUNT]; > > struct qcom_rproc_glink glink_subdev; > struct qcom_rproc_subdev smd_subdev; > @@ -590,37 +598,52 @@ static int adsp_alloc_memory_region(struct qcom_adsp *adsp) > > static int adsp_assign_memory_region(struct qcom_adsp *adsp) > { > - struct reserved_mem *rmem = NULL; > - struct qcom_scm_vmperm perm; > + struct qcom_scm_vmperm perm[MAX_ASSIGN_COUNT]; > + unsigned int perm_size = 1;
AFAICS, not need of initialization.
> struct device_node *node; > - int ret; > + int offset, ret;
Nit: one variable per line.
> > if (!adsp->region_assign_idx)
Not related to this patch.. Should not this be valid only for > 1 ?
> return 0; > > - node = of_parse_phandle(adsp->dev->of_node, "memory-region", adsp->region_assign_idx); > - if (node) > - rmem = of_reserved_mem_lookup(node); > - of_node_put(node); > - if (!rmem) { > - dev_err(adsp->dev, "unable to resolve shareable memory-region\n"); > - return -EINVAL; > - } > + for (offset = 0; offset < adsp->region_assign_count; ++offset) { > + struct reserved_mem *rmem = NULL; > + > + node = of_parse_phandle(adsp->dev->of_node, "memory-region", > + adsp->region_assign_idx + offset); > + if (node) > + rmem = of_reserved_mem_lookup(node); > + of_node_put(node); > + if (!rmem) { > + dev_err(adsp->dev, "unable to resolve shareable memory-region index %d\n", > + offset); > + return -EINVAL; > + }
> > - perm.vmid = QCOM_SCM_VMID_MSS_MSA; > - perm.perm = QCOM_SCM_PERM_RW; > + if (adsp->region_assign_shared) { > + perm[0].vmid = QCOM_SCM_VMID_HLOS; > + perm[0].perm = QCOM_SCM_PERM_RW; > + perm[1].vmid = adsp->region_assign_vmid; > + perm[1].perm = QCOM_SCM_PERM_RW; > + perm_size = 2; > + } else { > + perm[0].vmid = adsp->region_assign_vmid; > + perm[0].perm = QCOM_SCM_PERM_RW; > + perm_size = 1; > + } > > - adsp->region_assign_phys = rmem->base; > - adsp->region_assign_size = rmem->size; > - adsp->region_assign_perms = BIT(QCOM_SCM_VMID_HLOS); > + adsp->region_assign_phys[offset] = rmem->base; > + adsp->region_assign_size[offset] = rmem->size; > + adsp->region_assign_perms[offset] = BIT(QCOM_SCM_VMID_HLOS);
Do we need array for this, is this changing ?
> > - ret = qcom_scm_assign_mem(adsp->region_assign_phys, > - adsp->region_assign_size, > - &adsp->region_assign_perms, > - &perm, 1); > - if (ret < 0) { > - dev_err(adsp->dev, "assign memory failed\n"); > - return ret; > + ret = qcom_scm_assign_mem(adsp->region_assign_phys[offset], > + adsp->region_assign_size[offset], > + &adsp->region_assign_perms[offset], > + perm, perm_size); > + if (ret < 0) { > + dev_err(adsp->dev, "assign memory %d failed\n", offset); > + return ret; > + } > } > > return 0; > @@ -629,20 +652,22 @@ static int adsp_assign_memory_region(struct qcom_adsp *adsp) > static void adsp_unassign_memory_region(struct qcom_adsp *adsp) > { > struct qcom_scm_vmperm perm; > - int ret; > + int offset, ret; > > - if (!adsp->region_assign_idx) > + if (!adsp->region_assign_idx || adsp->region_assign_shared) > return; > > - perm.vmid = QCOM_SCM_VMID_HLOS; > - perm.perm = QCOM_SCM_PERM_RW; > + for (offset = 0; offset < adsp->region_assign_count; ++offset) { > + perm.vmid = QCOM_SCM_VMID_HLOS; > + perm.perm = QCOM_SCM_PERM_RW;
> > - ret = qcom_scm_assign_mem(adsp->region_assign_phys, > - adsp->region_assign_size, > - &adsp->region_assign_perms, > - &perm, 1); > - if (ret < 0) > - dev_err(adsp->dev, "unassign memory failed\n"); > + ret = qcom_scm_assign_mem(adsp->region_assign_phys[offset], > + adsp->region_assign_size[offset], > + &adsp->region_assign_perms[offset], > + &perm, 1); > + if (ret < 0) > + dev_err(adsp->dev, "unassign memory failed\n"); > + } > } > > static int adsp_probe(struct platform_device *pdev) > @@ -696,6 +721,9 @@ static int adsp_probe(struct platform_device *pdev) > adsp->info_name = desc->sysmon_name; > adsp->decrypt_shutdown = desc->decrypt_shutdown; > adsp->region_assign_idx = desc->region_assign_idx; > + adsp->region_assign_count = min_t(int, MAX_ASSIGN_COUNT, desc->region_assign_count); > + adsp->region_assign_vmid = desc->region_assign_vmid; > + adsp->region_assign_shared = desc->region_assign_shared; > if (dtb_fw_name) { > adsp->dtb_firmware_name = dtb_fw_name; > adsp->dtb_pas_id = desc->dtb_pas_id; > @@ -1163,6 +1191,8 @@ static const struct adsp_data sm8550_mpss_resource = { > .sysmon_name = "modem", > .ssctl_id = 0x12, > .region_assign_idx = 2, > + .region_assign_count = 1, > + .region_assign_vmid = QCOM_SCM_VMID_MSS_MSA, > }; > > static const struct of_device_id adsp_of_match[] = { >
-Mukesh
| |