| Message ID | 20231030-topic-sm8650-upstream-remoteproc-v2-2-609ee572e0a2@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: qcom: Introduce DSP support for SM8650 | expand |
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
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. > > >> 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. > >> - 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 Thanks, Neil
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 >
On 01/11/2023 15:42, Mukesh Ojha wrote: > > > 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. region_idx is the offset in the memory-region DT property where assigned memory starts, so for example there's 2 memory regions on SM8650 CDSP, but only a single shared memory region so we have the following: - region_assign_idx = 2 - region_assign_count = 1 and in DT: memory-region = <&cdsp_mem>, <&q6_cdsp_dtb_mem>, <&global_sync_mem>; -------------------------------------------------/\ region_assign_idx ------------------------------------------------[ ] region_assign_count and for MPSS, there's 2 of both: - region_assign_idx = 2 - region_assign_count = 2 and in DT: memory-region = <&mpss_mem>, <&q6_mpss_dtb_mem>, <&mpss_dsm_mem>, <&mpss_dsm_mem_2>; -------------------------------------------------/\ region_assign_idx ------------------------------------------------[ ] region_assign_count so we cannot add a bounday check. In any case of_parse_phandle() will do the boundary check if DT has less phandles than expected. Neil > > -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 >>
On 11/2/2023 3:56 PM, neil.armstrong@linaro.org wrote: > On 01/11/2023 15:42, Mukesh Ojha wrote: >> >> >> 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. I was wrong here.. MAX_ASSIGN_COUNT was relative to assign index. > > region_idx is the offset in the memory-region DT property where assigned > memory starts, > so for example there's 2 memory regions on SM8650 CDSP, but only a > single shared memory region > so we have the following: > - region_assign_idx = 2 > - region_assign_count = 1 > and in DT: > memory-region = <&cdsp_mem>, <&q6_cdsp_dtb_mem>, <&global_sync_mem>; > -------------------------------------------------/\ > region_assign_idx > ------------------------------------------------[ ] > region_assign_count > > and for MPSS, there's 2 of both: > - region_assign_idx = 2 > - region_assign_count = 2 > and in DT: > memory-region = <&mpss_mem>, <&q6_mpss_dtb_mem>, <&mpss_dsm_mem>, > <&mpss_dsm_mem_2>; > -------------------------------------------------/\ > region_assign_idx > > ------------------------------------------------[ ] > region_assign_count > > so we cannot add a bounday check. > > In any case of_parse_phandle() will do the boundary check if DT has less > phandles than expected. Thanks for explaining. -Mukesh > > Neil > >> >> -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 >>> >
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; struct device_node *node; - int ret; + int offset, ret; if (!adsp->region_assign_idx) 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); - 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[] = {
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(-)