Messages in this thread | | | Date | Wed, 1 Nov 2023 20:12:40 +0530 | Subject | Re: [PATCH v2 2/3] remoteproc: qcom: pas: make region assign more generic | From | Mukesh Ojha <> |
| |
On 10/31/2023 10:36 PM, Neil Armstrong wrote: > Hi, > > On 30/10/2023 14:10, Mukesh Ojha wrote: >> >> >> 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. > > Indeed, removed > >> >>> struct device_node *node; >>> - int ret; >>> + int offset, ret; >> >> Nit: one variable per line. > > Done > >> >>> if (!adsp->region_assign_idx) >> >> Not related to this patch.. >> Should not this be valid only for > 1 ? > > I don't understand, only region_assign_idx > 1 triggers a memory_assign, > and this check discards configurations with region_assign_idx == 0 as > expected.
Ah, you can ignore the comments, I got the intention after commenting here ..
> >> >> >>> 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 ? > > We need to keep region_assign_perms for unassign, but for the other 2 we > would > need to duplicate the code from adsp_assign_memory_region into > adsp_unassign_memory_region.
Thanks got it.
> >> >>> - 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;
Should this also need min_t(int, MAX_ASSIGN_COUNT - 1, desc->region_assign_idx); as no where boundary check is being done.
-Mukesh >>> + 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 > > Thanks, > Neil >
| |