[PATCH v2 0/2] remoteproc: xlnx: add auto-boot support

[PATCH v2 0/2] remoteproc: xlnx: add auto-boot support

[Tanmay Shah]

The Cortex-R remote processors on AMD-Xilinx platforms can run
before linux boot. Add auto-boot property to notify linux that remote
processor is ready to be used, so linux can load fw and start it or
attach to the running processor.


Tanmay Shah (2):
  dt-bindings: remoteproc: xlnx: add firmware-name property
  remoteproc: xlnx: enable auto boot feature

 .../remoteproc/xlnx,zynqmp-r5fss.yaml         |  4 ++
 drivers/remoteproc/xlnx_r5_remoteproc.c       | 48 +++++++++++++------
 2 files changed, 38 insertions(+), 14 deletions(-)


base-commit: 54dacf6efe7196c1cd8ae4b5c691579d0510a8bd
-- 
2.34.1

[PATCH v2 1/2] dt-bindings: remoteproc: xlnx: add firmware-name property

[Tanmay Shah]

The firmware-name property indicates which firmware to load on RPU
during the Linux boot time. It is possible to stop the RPU after boot
and load different firmware and start RPU.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
 .../devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml     | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
index ee63c03949c9..ae63c3e39ced 100644
--- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
@@ -135,6 +135,10 @@ patternProperties:
           - description: vring1
         additionalItems: true
 
+      firmware-name:
+        maxItems: 1
+        description: default firmware to load
+
     required:
       - compatible
       - reg
-- 
2.34.1

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

[Tanmay Shah]

remoteproc framework has capability to start (or attach to) the remote
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)
 {
 	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;
+
 	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;
-	}
-
 	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;
 		}
 	}
 
@@ -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

Re: [PATCH v2 0/2] remoteproc: xlnx: add auto-boot support

[Shah, Tanmay]

On 5/1/2026 9:37 AM, Tanmay Shah wrote:
> The Cortex-R remote processors on AMD-Xilinx platforms can run
> before linux boot. Add auto-boot property to notify linux that remote
> processor is ready to be used, so linux can load fw and start it or
> attach to the running processor.
> 

Missed the change log here:

Changes in v2:
  - remove the auto-boot property from bindings patch (1/2)
  - rebase on latest remoteproc for-next branch
  - refactor the driver patch (2/2) and detect the auto-boot runtime

If needed, I will send v2 with the change log.

> 
> Tanmay Shah (2):
>   dt-bindings: remoteproc: xlnx: add firmware-name property
>   remoteproc: xlnx: enable auto boot feature
> 
>  .../remoteproc/xlnx,zynqmp-r5fss.yaml         |  4 ++
>  drivers/remoteproc/xlnx_r5_remoteproc.c       | 48 +++++++++++++------
>  2 files changed, 38 insertions(+), 14 deletions(-)
> 
> 
> base-commit: 54dacf6efe7196c1cd8ae4b5c691579d0510a8bd

Re: [PATCH v2 1/2] dt-bindings: remoteproc: xlnx: add firmware-name property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1163 bytes --]

On Fri, May 01, 2026 at 07:37:06AM -0700, Tanmay Shah wrote:
> The firmware-name property indicates which firmware to load on RPU
> during the Linux boot time. It is possible to stop the RPU after boot
> and load different firmware and start RPU.

The file supports 3 devices, do they all support loading firmware like
this?

> 
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
>  .../devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml     | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
> index ee63c03949c9..ae63c3e39ced 100644
> --- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
> @@ -135,6 +135,10 @@ patternProperties:
>            - description: vring1
>          additionalItems: true
>  
> +      firmware-name:
> +        maxItems: 1
> +        description: default firmware to load
> +
>      required:
>        - compatible
>        - reg
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] dt-bindings: remoteproc: xlnx: add firmware-name property

[Shah, Tanmay]

Hello,

On 5/1/2026 10:49 AM, Conor Dooley wrote:
> On Fri, May 01, 2026 at 07:37:06AM -0700, Tanmay Shah wrote:
>> The firmware-name property indicates which firmware to load on RPU
>> during the Linux boot time. It is possible to stop the RPU after boot
>> and load different firmware and start RPU.
> 
> The file supports 3 devices, do they all support loading firmware like
> this?
> 

Thank you for the reviews.
Yes, all the devices support loading the firmware like this.

>>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>>  .../devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml     | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>> index ee63c03949c9..ae63c3e39ced 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>> +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>> @@ -135,6 +135,10 @@ patternProperties:
>>            - description: vring1
>>          additionalItems: true
>>  
>> +      firmware-name:
>> +        maxItems: 1
>> +        description: default firmware to load
>> +
>>      required:
>>        - compatible
>>        - reg
>> -- 
>> 2.34.1
>>

Re: [PATCH v2 1/2] dt-bindings: remoteproc: xlnx: add firmware-name property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

On Fri, May 01, 2026 at 07:37:06AM -0700, Tanmay Shah wrote:
> The firmware-name property indicates which firmware to load on RPU
> during the Linux boot time. It is possible to stop the RPU after boot
> and load different firmware and start RPU.
> 
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>
pw-bot: not-applicable

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Mathieu Poirier]

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
> 

Re: [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@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
>>

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

[Shah, Tanmay]

On 5/21/2026 1:38 PM, Shah, Tanmay wrote:
> 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@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.
> 

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

I probably misunderstood this comment. Here is the correct explanation:

The auto_boot setting in the zynqmp_r5_alloc_core() is done if the
'firmware-name' property is present in the device-tree.

Here it is done, if the remote core is already running. This is to
support attach-detach use case.

So, auto_boot is possible in two cases: 1) If firmware-name property is
available (Linux boots the remote), 2) If firmware is already loaded and
remote was started by the boot loader. (Linux attaches to the running
remote).

This is the second use case.

Thanks,
Tanmay

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