| Message ID | 20231208-topic-sm8650-upstream-remoteproc-v4-2-a96c3e5f0913@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: qcom: Introduce DSP support for SM8650 | expand |
On 8.12.2023 16:04, 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. > > Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com> > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- [...] > + 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); Shouldn't this only be called when parse_phandle succeeds? (separate patch with a fix + cc stable if so?) > + 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, I think this should be renamed to region_assign_owner(s) > - &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 +653,23 @@ 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 offset; > int ret; > > - if (!adsp->region_assign_idx) > + if (!adsp->region_assign_idx || adsp->region_assign_shared) So when it's *shared*, we don't want to un-assign it? Konrad
On 09/12/2023 19:06, Konrad Dybcio wrote: > On 8.12.2023 16:04, 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. >> >> Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- > [...] > >> + 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); > Shouldn't this only be called when parse_phandle succeeds? (separate > patch with a fix + cc stable if so?) It's not a bug, it was added like that because of_node_put() already checks for a NULL pointer: https://elixir.bootlin.com/linux/v6.7-rc5/source/drivers/of/dynamic.c#L45 > >> + 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, > I think this should be renamed to region_assign_owner(s) Why ? this bitfield is names "perms" everywhere qcom_scm_assign_mem is used > >> - &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 +653,23 @@ 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 offset; >> int ret; >> >> - if (!adsp->region_assign_idx) >> + if (!adsp->region_assign_idx || adsp->region_assign_shared) > So when it's *shared*, we don't want to un-assign it? Exact, when shared the region stays shared, as downstream does, un-assigning will fail in this case. > > Konrad Thanks, Neil
On 11/12/2023 10:54, Konrad Dybcio wrote: > On 11.12.2023 10:37, Neil Armstrong wrote: >> On 09/12/2023 19:06, Konrad Dybcio wrote: >>> On 8.12.2023 16:04, 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. >>>> >>>> Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com> >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>> --- >>> [...] >>> >>>> + 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); >>> Shouldn't this only be called when parse_phandle succeeds? (separate >>> patch with a fix + cc stable if so?) >> >> It's not a bug, it was added like that because of_node_put() already >> checks for a NULL pointer: >> https://elixir.bootlin.com/linux/v6.7-rc5/source/drivers/of/dynamic.c#L45 > Ack > >> >>> >>>> + 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, >>> I think this should be renamed to region_assign_owner(s) >> >> Why ? this bitfield is names "perms" everywhere qcom_scm_assign_mem is used > And IMO that's not correct - there's the qcom_scm_vmperm.perm field which > is oneOf r/w/x/rw/rwx and this one is filled with ORed BIT()-ed elements > allowed in qcom_scm_vmperm.vmid (QCOM_SCM_VMID_...) Ok right I just use the same namings as in rmtfs_mem, fastrpc & ath10k/qmi, but indeed the qcom_scm_assign_mem() 3rd param name is srcvm but doc says "vmid for current set of owners", so yeah it could be named owners. I'll send a v5 with the rename. Neil > > Konrad
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c index 913a5d2068e8..46371f1ad32d 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,53 @@ 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]; struct device_node *node; + unsigned int perm_size; + int offset; int 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 +653,23 @@ 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 offset; int 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 %d failed\n", offset); + } } static int adsp_probe(struct platform_device *pdev) @@ -696,6 +723,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 +1193,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[] = {