Re: [PATCH v2 2/2] remoteproc: xlnx: enable auto boot feature

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

Good morning,

I don't recal reviewing the first revision of this set.  Can you provide a link
to it so that I can read the comments that were provided?

On Fri, May 01, 2026 at 07:37:07AM -0700, Tanmay Shah wrote:
> remoteproc framework has capability to start (or attach to) the remote

The remoteproc framework...

> processor automatically if auto boot flag is set by the driver during
> probe. If remote core is not started before the Linux boot, and linux is
> expected to start the remote core then it uses "firmware-name" property
> to load default firmware during auto boot.
> 
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
>  drivers/remoteproc/xlnx_r5_remoteproc.c | 48 +++++++++++++++++--------
>  1 file changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> index 45a62cb98072..652030f9cea2 100644
> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> @@ -899,17 +899,18 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
>  };
>  
>  /**
> - * zynqmp_r5_add_rproc_core() - Add core data to framework.
> - * Allocate and add struct rproc object for each r5f core
> + * zynqmp_r5_alloc_rproc_core() - alloc rproc core data structure
> + * Allocate struct rproc object for each r5f core
>   * This is called for each individual r5f core
>   *
>   * @cdev: Device node of each r5 core
>   *
>   * Return: zynqmp_r5_core object for success else error code pointer
>   */
> -static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
> +static struct zynqmp_r5_core *zynqmp_r5_alloc_rproc_core(struct device *cdev)

Why is there a need to change the function's name?

>  {
>  	struct zynqmp_r5_core *r5_core;
> +	const char *fw_name = NULL;
>  	struct rproc *r5_rproc;
>  	int ret;
>  
> @@ -918,10 +919,15 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> +	ret = rproc_of_parse_firmware(cdev, 0, &fw_name);
> +	if (ret < 0 && ret != -EINVAL)
> +		return ERR_PTR(dev_err_probe(cdev, ret,
> +					     "failed to parse firmware-name\n"));
> +
>  	/* Allocate remoteproc instance */
>  	r5_rproc = rproc_alloc(cdev, dev_name(cdev),
>  			       &zynqmp_r5_rproc_ops,
> -			       NULL, sizeof(struct zynqmp_r5_core));
> +			       fw_name, sizeof(struct zynqmp_r5_core));
>  	if (!r5_rproc) {
>  		dev_err(cdev, "failed to allocate memory for rproc instance\n");
>  		return ERR_PTR(-ENOMEM);
> @@ -932,6 +938,11 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>  	r5_rproc->recovery_disabled = true;
>  	r5_rproc->has_iommu = false;
>  	r5_rproc->auto_boot = false;
> +
> +	/* attempt to boot automatically if the firmware-name is provided */
> +	if (fw_name)
> +		r5_rproc->auto_boot = true;
> +

What happens when a firmware name needs to be provided in the DT but you don't
want to automatically boot the remote processor?

>  	r5_core = r5_rproc->priv;
>  	r5_core->dev = cdev;
>  	r5_core->np = dev_of_node(cdev);
> @@ -941,13 +952,6 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>  		goto free_rproc;
>  	}
>  
> -	/* Add R5 remoteproc core */
> -	ret = rproc_add(r5_rproc);
> -	if (ret) {
> -		dev_err(cdev, "failed to add r5 remoteproc\n");
> -		goto free_rproc;
> -	}
> -

I'm not sure why there is a need to move this to zynqmp_r5_cluster_init()?  Is
it simply to make the error path easier to handle?  If so, please do that in a
separate patch.

>  	r5_core->rproc = r5_rproc;
>  	return r5_core;
>  
> @@ -1280,6 +1284,7 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
>  			if (zynqmp_r5_get_rsc_table_va(r5_core))
>  				dev_dbg(r5_core->dev, "rsc tbl not found\n");
>  			r5_core->rproc->state = RPROC_DETACHED;
> +			r5_core->rproc->auto_boot = true;

I thought this was done in zynqmp_r5_add_rproc_core() - what am I missing?

Thanks,
Mathieu

>  		}
>  	}
>  
> @@ -1304,7 +1309,7 @@ static int zynqmp_r5_cluster_init(struct zynqmp_r5_cluster *cluster)
>  	enum rpu_oper_mode fw_reg_val;
>  	struct device **child_devs;
>  	enum rpu_tcm_comb tcm_mode;
> -	int core_count, ret, i;
> +	int core_count, ret, i, j;
>  	struct mbox_info *ipi;
>  
>  	ret = of_property_read_u32(dev_node, "xlnx,cluster-mode", &cluster_mode);
> @@ -1390,7 +1395,7 @@ static int zynqmp_r5_cluster_init(struct zynqmp_r5_cluster *cluster)
>  		child_devs[i] = &child_pdev->dev;
>  
>  		/* create and add remoteproc instance of type struct rproc */
> -		r5_cores[i] = zynqmp_r5_add_rproc_core(&child_pdev->dev);
> +		r5_cores[i] = zynqmp_r5_alloc_rproc_core(&child_pdev->dev);
>  		if (IS_ERR(r5_cores[i])) {
>  			ret = PTR_ERR(r5_cores[i]);
>  			r5_cores[i] = NULL;
> @@ -1435,16 +1440,31 @@ static int zynqmp_r5_cluster_init(struct zynqmp_r5_cluster *cluster)
>  		goto release_r5_cores;
>  	}
>  
> +	for (j = 0; j < cluster->core_count; j++) {
> +		/* Add R5 remoteproc core */
> +		ret = rproc_add(r5_cores[j]->rproc);
> +		if (ret) {
> +			dev_err_probe(r5_cores[j]->dev, ret,
> +				      "failed to add remoteproc\n");
> +			goto delete_r5_cores;
> +		}
> +	}
> +
>  	kfree(child_devs);
>  	return 0;
>  
> +delete_r5_cores:
> +	i = core_count - 1;
> +	/* delete previous added rproc */
> +	while (--j >= 0)
> +		rproc_del(r5_cores[j]->rproc);
> +
>  release_r5_cores:
>  	while (i >= 0) {
>  		put_device(child_devs[i]);
>  		if (r5_cores[i]) {
>  			zynqmp_r5_free_mbox(r5_cores[i]->ipi);
>  			of_reserved_mem_device_release(r5_cores[i]->dev);
> -			rproc_del(r5_cores[i]->rproc);
>  			rproc_free(r5_cores[i]->rproc);
>  		}
>  		i--;
> -- 
> 2.34.1
>