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/[email protected]/ 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 <[email protected]> >> --- >> 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 >>
