Re: [PATCH v2 1/2] remoteproc: elf_loader: Create new callback for copying segments to remote cpu

[Mathieu Poirier <mathieu.poirier@linaro.org>]

Hi Uwe,

On Tue, Oct 27, 2026 at 05:25:20PM +0100, Uwe Kleine-König wrote:
> Some devices have special needs about how an Elf segment is copied to
> the remote cpu's memory space where the default implementation using
> memcpy_toio/memcpy and memset_io/memset isn't suitable. An affected
> mainline driver is imx-dsp-rproc which includes a near-copy of
> rproc_elf_load_segments() just to handle the IRAM only allowing 32 bit
> accesses. On (not-yet-mainline) ADI SC846 the respective memory isn't
> directly accessible at all and DMA must be used to copy the data.
> 
> The new callback can be used to implement just the driver specific
> memcpy + memset to increase code reuse.
> 
> The patch only appears big because of indentation changes.
> 
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  drivers/remoteproc/remoteproc_elf_loader.c | 64 +++++++++++++---------
>  include/linux/remoteproc.h                 |  4 ++
>  2 files changed, 41 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> index da3cddbe7d4c..3c749a39e5da 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -227,35 +227,45 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
>  			break;
>  		}
>  
> -		/* grab the kernel address for this device address */
> -		ptr = rproc_da_to_va(rproc, da, memsz, &is_iomem);
> -		if (!ptr) {
> -			dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
> -				memsz);
> -			ret = -EINVAL;
> -			break;
> -		}
> +		if (rproc->ops->memcpy_tova) {
> +			ret = rproc->ops->memcpy_tova(rproc, da, elf_data + offset, filesz, memsz);
> +			if (ret) {
> +				dev_err(dev, "Failed to copy segment %d to device address 0x%llx: %pe\n",
> +					i, da, ERR_PTR(ret));
> +				break;
> +			}

I really like your patch, but I'm wondering why you decided to move
rproc_da_to_va() in ops->memcpy_tova().  Right now it is called in both path,
i.e with and without ops->memcpy_tova().  Wouldn't it be better to
unconditionally leave rproc_da_to_va() in rproc_elf_load_segments()?

Thanks,
Mathieu

> +		} else {
> +			/* grab the kernel address for this device address */
> +			ptr = rproc_da_to_va(rproc, da, memsz, &is_iomem);
> +			if (!ptr) {
> +				dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
> +					memsz);
> +				ret = -EINVAL;
> +				break;
> +			}
>  
> -		/* put the segment where the remote processor expects it */
> -		if (filesz) {
> -			if (is_iomem)
> -				memcpy_toio((void __iomem *)ptr, elf_data + offset, filesz);
> -			else
> -				memcpy(ptr, elf_data + offset, filesz);
> -		}
> +			/* put the segment where the remote processor expects it */
> +			if (filesz) {
> +				if (is_iomem)
> +					memcpy_toio((void __iomem *)ptr, elf_data + offset, filesz);
> +				else
> +					memcpy(ptr, elf_data + offset, filesz);
> +			}
>  
> -		/*
> -		 * Zero out remaining memory for this segment.
> -		 *
> -		 * This isn't strictly required since dma_alloc_coherent already
> -		 * did this for us. albeit harmless, we may consider removing
> -		 * this.
> -		 */
> -		if (memsz > filesz) {
> -			if (is_iomem)
> -				memset_io((void __iomem *)(ptr + filesz), 0, memsz - filesz);
> -			else
> -				memset(ptr + filesz, 0, memsz - filesz);
> +			/*
> +			 * Zero out remaining memory for this segment.
> +			 *
> +			 * This isn't strictly required since dma_alloc_coherent already
> +			 * did this for us. albeit harmless, we may consider removing
> +			 * this.
> +			 */
> +			if (memsz > filesz) {
> +				if (is_iomem)
> +					memset_io((void __iomem *)(ptr + filesz),
> +						  0, memsz - filesz);
> +				else
> +					memset(ptr + filesz, 0, memsz - filesz);
> +			}
>  		}
>  	}
>  
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 00019b1895e6..e750a127cbc4 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -75,6 +75,7 @@ enum rsc_handling_status {
>   * @detach:	detach from a device, leaving it powered up
>   * @kick:	kick a virtqueue (virtqueue id given as a parameter)
>   * @da_to_va:	optional platform hook to perform address translations
> + * @memcpy_tova:optional platform hook to copy data into remote memory range
>   * @parse_fw:	parse firmware to extract information (e.g. resource table)
>   * @handle_rsc:	optional platform hook to handle vendor resources. Should return
>   *		RSC_HANDLED if resource was handled, RSC_IGNORED if not handled
> @@ -99,6 +100,9 @@ struct rproc_ops {
>  	int (*detach)(struct rproc *rproc);
>  	void (*kick)(struct rproc *rproc, int vqid);
>  	void * (*da_to_va)(struct rproc *rproc, u64 da, size_t len, bool *is_iomem);
> +	int (*memcpy_tova)(struct rproc *rproc, u64 da, const void *src,
> +			   size_t filesz, size_t memsz);
> +
>  	int (*parse_fw)(struct rproc *rproc, const struct firmware *fw);
>  	int (*handle_rsc)(struct rproc *rproc, u32 rsc_type, void *rsc,
>  			  int offset, int avail);
> -- 
> 2.47.3
>