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

[Shah, Tanmay]

Hello,

Thank you for the reviews, please find my comments below:

On 5/21/2026 12:48 PM, Mathieu Poirier wrote:
> 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?
> 

Here it is:
https://lore.kernel.org/linux-remoteproc/20260422202558.2362971-1-tanmay.shah@amd.com/

The device-tree bindings needed rework in v1, so I sent v2, before we
ever reviewed the driver part.


> 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...
> 

Ack.

>> 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 at 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?
> 

Before, the function was actually adding the rproc core by calling
rproc_add() function, but now it only allocates the memory by calling
rproc_alloc(). For auto boot to work it's important to add rproc core
after all the other hw is initialized (such as mbox, tcm, sram,
power-domains etc). More details below [1].

>>  {
>>  	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?
> 

I think that use case is not needed. If the user/system-designer doesn't
want auto-boot, then having firmware-name in the device-tree serves no
purpose. User can always load the firmware via sysfs once kernel boots.

>>  	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.
> 

[1] This was moved to make auto-boot work. The remote core can auto-boot
only after other hardware is initialized. The zynqmp_r5_core_init()
initializes sram, TCM and power-domains of the core. Also, mailbox is
requested before zynqmp_r5_core_init() as well. We can't auto-boot core
directly without all this. So, I had to move rproc_add() at the end of
the cluster init, and rename above function from
zynqmp_r5_add_rproc_core to zynqmp_r5_alloc_rproc_core.

If you prefer, I will add above explanation in the commit text, or as
comment right before rproc_add().



>>  	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?
> 

That function is now zynqmp_r5_alloc_core() as mentioned above. Also,
until now, auto_boot was set to 'false' only to show that it is
disabled. It is actually used and enabled now.

> 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
>>