On Wed, Aug 13, 2025 at 04:48:03PM -0500, Rob Herring (Arm) wrote:
> Use the newly added of_reserved_mem_region_to_resource() and
> of_reserved_mem_region_count() functions to handle "memory-region"
> properties.
>
> The error handling is a bit different in some cases. Often
> "memory-region" is optional, so failed lookup is not an error. But then
> an error in of_reserved_mem_lookup() is treated as an error. However,
> that distinction is not really important. Either the region is available
> and usable or it is not. So now, it is just
> of_reserved_mem_region_to_resource() which is checked for an error.
>
> Acked-by: Arnaud Pouliquen <arnaud.pouliq...@foss.st.com>
> Tested-by: Peng Fan <peng....@nxp.com> # i.MX93-11x11-EVK for imx_rproc.c
> Signed-off-by: Rob Herring (Arm) <r...@kernel.org>
> ---
> v4:
> - Rebase on v6.17-rc1. qcom_q6v5_pas.c conflicted needing s/adsp/pas/
>
> v3:
> - Rebase on v6.16-rc1. Move TI K3 changes to new common file.
> - Fix double increment of "i" in xlnx_r5
>
> v2:
> - Use strstarts instead of strcmp for resource names as they include
> the unit-address.
> - Drop the unit-address from resource name for imx and st drivers
> ---
> drivers/remoteproc/imx_dsp_rproc.c | 45 ++++++--------
> drivers/remoteproc/imx_rproc.c | 68 ++++++++------------
> drivers/remoteproc/qcom_q6v5_adsp.c | 24 +++-----
> drivers/remoteproc/qcom_q6v5_mss.c | 60 ++++++------------
> drivers/remoteproc/qcom_q6v5_pas.c | 75 +++++++++--------------
> drivers/remoteproc/qcom_q6v5_wcss.c | 25 +++-----
> drivers/remoteproc/qcom_wcnss.c | 23 +++----
> drivers/remoteproc/rcar_rproc.c | 36 +++++------
> drivers/remoteproc/st_remoteproc.c | 41 ++++++-------
> drivers/remoteproc/stm32_rproc.c | 44 ++++++-------
> drivers/remoteproc/ti_k3_common.c | 28 ++++-----
> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 2 +-
> drivers/remoteproc/ti_k3_r5_remoteproc.c | 2 +-
> drivers/remoteproc/xlnx_r5_remoteproc.c | 51 ++++++---------
> 14 files changed, 204 insertions(+), 320 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c
> b/drivers/remoteproc/qcom_q6v5_adsp.c
> index 94af77baa7a1..a5b7cbb8fe07 100644
> --- a/drivers/remoteproc/qcom_q6v5_adsp.c
> +++ b/drivers/remoteproc/qcom_q6v5_adsp.c
> @@ -625,26 +625,20 @@ static int adsp_init_mmio(struct qcom_adsp *adsp,
>
> static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
> {
> - struct reserved_mem *rmem = NULL;
> - struct device_node *node;
> -
> - node = of_parse_phandle(adsp->dev->of_node, "memory-region", 0);
> - if (node)
> - rmem = of_reserved_mem_lookup(node);
> - of_node_put(node);
> + int ret;
> + struct resource res;
>
> - if (!rmem) {
> + ret = of_reserved_mem_region_to_resource(adsp->dev->of_node, 0, &res);
> + if (!ret) {
> dev_err(adsp->dev, "unable to resolve memory-region\n");
> - return -EINVAL;
> + return ret;
This looks strange. Shouldn't it be `if (ret) {` ?
> }
>
> - adsp->mem_phys = adsp->mem_reloc = rmem->base;
> - adsp->mem_size = rmem->size;
> - adsp->mem_region = devm_ioremap_wc(adsp->dev,
> - adsp->mem_phys, adsp->mem_size);
> + adsp->mem_phys = adsp->mem_reloc = res.start;
> + adsp->mem_size = resource_size(&res);
> + adsp->mem_region = devm_ioremap_resource_wc(adsp->dev, &res);
> if (!adsp->mem_region) {
> - dev_err(adsp->dev, "unable to map memory region: %pa+%zx\n",
> - &rmem->base, adsp->mem_size);
> + dev_err(adsp->dev, "unable to map memory region: %pR\n", &res);
> return -EBUSY;
> }
>
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c
> b/drivers/remoteproc/qcom_q6v5_mss.c
> index 0c0199fb0e68..0fea5f91dd1c 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -1961,8 +1961,8 @@ static int q6v5_init_reset(struct q6v5 *qproc)
> static int q6v5_alloc_memory_region(struct q6v5 *qproc)
> {
> struct device_node *child;
> - struct reserved_mem *rmem;
> - struct device_node *node;
> + struct resource res;
> + int ret;
>
> /*
> * In the absence of mba/mpss sub-child, extract the mba and mpss
> @@ -1970,71 +1970,49 @@ static int q6v5_alloc_memory_region(struct q6v5
> *qproc)
> */
> child = of_get_child_by_name(qproc->dev->of_node, "mba");
> if (!child) {
> - node = of_parse_phandle(qproc->dev->of_node,
> - "memory-region", 0);
> + ret = of_reserved_mem_region_to_resource(qproc->dev->of_node,
> 0, &res);
> } else {
> - node = of_parse_phandle(child, "memory-region", 0);
> + ret = of_reserved_mem_region_to_resource(child, 0, &res);
> of_node_put(child);
> }
>
> - if (!node) {
> - dev_err(qproc->dev, "no mba memory-region specified\n");
> - return -EINVAL;
> - }
> -
> - rmem = of_reserved_mem_lookup(node);
> - of_node_put(node);
> - if (!rmem) {
> + if (ret) {
> dev_err(qproc->dev, "unable to resolve mba region\n");
> - return -EINVAL;
> + return ret;
> }
>
> - qproc->mba_phys = rmem->base;
> - qproc->mba_size = rmem->size;
> + qproc->mba_phys = res.start;
> + qproc->mba_size = resource_size(&res);
>
> if (!child) {
> - node = of_parse_phandle(qproc->dev->of_node,
> - "memory-region", 1);
> + ret = of_reserved_mem_region_to_resource(qproc->dev->of_node,
> 1, &res);
> } else {
> child = of_get_child_by_name(qproc->dev->of_node, "mpss");
> - node = of_parse_phandle(child, "memory-region", 0);
> + ret = of_reserved_mem_region_to_resource(child, 0, &res);
> of_node_put(child);
> }
>
> - if (!node) {
> - dev_err(qproc->dev, "no mpss memory-region specified\n");
> - return -EINVAL;
> - }
> -
> - rmem = of_reserved_mem_lookup(node);
> - of_node_put(node);
> - if (!rmem) {
> + if (ret) {
> dev_err(qproc->dev, "unable to resolve mpss region\n");
> - return -EINVAL;
> + return ret;
> }
>
> - qproc->mpss_phys = qproc->mpss_reloc = rmem->base;
> - qproc->mpss_size = rmem->size;
> + qproc->mpss_phys = qproc->mpss_reloc = res.start;
> + qproc->mpss_size = resource_size(&res);
>
> if (!child) {
> - node = of_parse_phandle(qproc->dev->of_node, "memory-region",
> 2);
> + ret = of_reserved_mem_region_to_resource(qproc->dev->of_node,
> 2, &res);
> } else {
> child = of_get_child_by_name(qproc->dev->of_node, "metadata");
> - node = of_parse_phandle(child, "memory-region", 0);
> + ret = of_reserved_mem_region_to_resource(child, 0, &res);
> of_node_put(child);
> }
>
> - if (!node)
> + if (ret)
> return 0;
Shouldn't we differentiate between an absent region (OK) and an error
during parse.
>
> - rmem = of_reserved_mem_lookup(node);
> - if (!rmem) {
> - dev_err(qproc->dev, "unable to resolve metadata region\n");
> - return -EINVAL;
> - }
> -
> - qproc->mdata_phys = rmem->base;
> - qproc->mdata_size = rmem->size;
> + qproc->mdata_phys = res.start;
> + qproc->mdata_size = resource_size(&res);
>
> return 0;
> }
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c
> b/drivers/remoteproc/qcom_q6v5_pas.c
> index 02e29171cbbe..b3f7209289a6 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -121,7 +121,7 @@ struct qcom_pas {
>
> static void qcom_pas_segment_dump(struct rproc *rproc,
> struct rproc_dump_segment *segment,
> - void *dest, size_t offset, size_t size)
> + void *dest, size_t offset, size_t size)
Irrelevant? (and two next chunks)
> {
> struct qcom_pas *pas = rproc->priv;
> int total_offset;
> @@ -149,7 +149,7 @@ static void qcom_pas_minidump(struct rproc *rproc)
> }
>
> static int qcom_pas_pds_enable(struct qcom_pas *pas, struct device **pds,
> - size_t pd_count)
> + size_t pd_count)
> {
> int ret;
> int i;
> @@ -176,7 +176,7 @@ static int qcom_pas_pds_enable(struct qcom_pas *pas,
> struct device **pds,
> };
>
> static void qcom_pas_pds_disable(struct qcom_pas *pas, struct device **pds,
> - size_t pd_count)
> + size_t pd_count)
> {
> int i;
>
--
With best wishes
Dmitry
