On Thu, 21 May 2026 at 12:48, Shah, Tanmay <[email protected]> wrote:
>
>
>
> 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/[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.
> >
>
> "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 for the clarifications, I'll have another look at this set.
> 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
> >>>
> >
>
