On Thu, Aug 14, 2025 at 1:52 AM Dmitry Baryshkov
<dmitry.barysh...@oss.qualcomm.com> wrote:
>
> 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) {` ?
Indeed. I checked other spots for the same mistake and this is the only one.
>
> > }
> >
> > - 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.
IMO, no. The resource is either available to Linux or it isn't. The
driver can decide whether it can continue out without or not. Anything
more is just validation of the DT which the kernel does a terribly
inconsistent job of and we have better tools for.
> > - 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)
Yes, not sure how those snuck in there.
Rob
