[PATCH v6 2/5] remoteproc: imx_rproc: Populate devices under "rpmsg" subnode

[Shenwei Wang]

> -----Original Message-----
> From: Arnaud POULIQUEN <arnaud.pouliquen at foss.st.com>
> Sent: Thursday, December 18, 2025 5:04 AM
> To: Shenwei Wang <shenwei.wang at nxp.com>; Linus Walleij
> <linusw at kernel.org>; Bartosz Golaszewski <brgl at kernel.org>; Rob Herring
> <robh at kernel.org>; Krzysztof Kozlowski <krzk+dt at kernel.org>; Conor Dooley
> <conor+dt at kernel.org>; Bjorn Andersson <andersson at kernel.org>; Mathieu
> Poirier <mathieu.poirier at linaro.org>; Shawn Guo <shawnguo at kernel.org>;
> Sascha Hauer <s.hauer at pengutronix.de>; Jonathan Corbet <corbet at lwn.net>
> Cc: Pengutronix Kernel Team <kernel at pengutronix.de>; Fabio Estevam
> <festevam at gmail.com>; Peng Fan <peng.fan at nxp.com>; linux-
> gpio at vger.kernel.org; devicetree at vger.kernel.org; linux-
> kernel at vger.kernel.org; linux-remoteproc at vger.kernel.org; imx at lists.linux.dev;
> linux-arm-kernel at lists.infradead.org; linux-doc at vger.kernel.org; dl-linux-imx
> <linux-imx at nxp.com>
> Subject: [EXT] Re: [PATCH v6 2/5] remoteproc: imx_rproc: Populate devices
> under "rpmsg" subnode
> 
> > +     rp_driver->rpdrv.drv.name = name;
> > +     rp_driver->rpdrv.id_table = rpdev_id;
> > +     rp_driver->rpdrv.probe = imx_rpmsg_endpoint_probe;
> > +     rp_driver->rpdrv.remove = imx_rpmsg_endpoint_remove;
> > +     rp_driver->rpdrv.callback = imx_rpmsg_endpoint_cb;
> > +     rp_driver->driver_data = driver_data;
> > +     rp_driver->compat = compat;
> > +
> > +     register_rpmsg_driver(&rp_driver->rpdrv);
> 
> 
> I still believe that creating a dependency between remoteproc and rpmsg is not
> suitable.
> 
> Please correct me if I’m wrong, but since you define gpio at 0 and gpio at 1 in your
> device tree, you call register_rpmsg_driver() twice, creating two instances of the
> same driver. To differentiate both, you fill the rpdrv.id_table with the node
> names "gpio at 0" and "gpio at 1".
> 

Nope. The function of register_rpmsg_driver is invoked only once per channel.

> In a topology with two remote processors, each needing rpmsg-gpio, the same
> situation would not work because you would have two "gpio at 0" entries.
> 

No, it’s working. The gpio-rpmsg driver is designed to support multiple instances. In fact, that’s the reason 
we added the "rpmsg" bus node under the rproc node. 
Please note that you cannot use duplicate labels within the same channel. 

> What about using the ns-announcement mechanism on the remote side to
> address GPIO port/bank instances?
> 
> In the rpmsg-gpio driver, you could define:
> 
> static struct rpmsg_device_id rpmsg_gpio_id_table[] = {
>      { .name = "rpmsg-gpio" },
>      { .name = "rpmsg_gpio-0" },
>      { .name = "rpmsg_gpio-1" },
>      { .name = "rpmsg_gpio-2" },
>      { .name = "rpmsg_gpio-3" },
>      { },
> };
> 

These definitions are redundant. 
The current implementation already supports multiple instances without requiring this 
information to be hardcoded into the driver source.

Regards,
Shenwei

> Then the match between the device tree and the rpmsg bus could be done in the
> rpmsg-gpio driver by matching the rpmsg name with the compatible property plus
> the reg value.
> 
> Example device tree snippet:
> 
> cm33: remoteproc-cm33 {
>      compatible = "fsl,imx8ulp-cm33";
> 
>      rpmsg {
>          rpmsg-io-channel {
>              gpio at 0 {
>                  compatible = "rpmsg_gpio";
>                  reg = <0>;
>              };
> 
>              gpio at 1 {
>                  compatible = "rpmsg_gpio";
>                  reg = <1>;
>              };
> 
>              ...
>          };
> 
>          ...
>      };
> };
> 
> With this approach, rpmsg management could be handled entirely within the
> rpmsg-gpio driver, avoiding the need to register multiple instances of the
> rpmsg_gpio driver.
> 
> Regards,
> Arnaud
> 
> > +
> > +     return 0;
> > +}
> > +
> > +static int rproc_of_rpmsg_node_init(struct platform_device *pdev) {
> > +     struct device *dev = &pdev->dev;
> > +     const char *compat;
> > +     int ret;
> > +
> > +     struct device_node *np __free(device_node) = of_get_child_by_name(dev-
> >of_node, "rpmsg");
> > +     if (!np)
> > +             return 0;
> > +
> > +     for_each_child_of_node_scoped(np, child) {
> > +             compat = imx_of_rpmsg_is_in_map(child->name);
> > +             if (!compat)
> > +                     ret = of_platform_default_populate(child, NULL, dev);
> > +             else
> > +                     ret = imx_of_rpmsg_register_rpdriver(child, dev,
> > + child->name, compat);
> > +
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >   static int imx_rproc_probe(struct platform_device *pdev)
> >   {
> >       struct device *dev = &pdev->dev; @@ -1114,6 +1253,10 @@ static
> > int imx_rproc_probe(struct platform_device *pdev)
> >               goto err_put_pm;
> >       }
> >
> > +     ret = rproc_of_rpmsg_node_init(pdev);
> > +     if (ret < 0)
> > +             dev_info(dev, "populating 'rpmsg' node failed\n");
> > +
> >       return 0;
> >
> >   err_put_pm:
> > diff --git a/include/linux/rpmsg/rpdev_info.h
> > b/include/linux/rpmsg/rpdev_info.h
> > new file mode 100644
> > index 000000000000..13e020cd028b
> > --- /dev/null
> > +++ b/include/linux/rpmsg/rpdev_info.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright 2025 NXP */
> > +
> > +/*
> > + * @file linux/rpdev_info.h
> > + *
> > + * @brief Global header file for RPDEV Info
> > + *
> > + * @ingroup RPMSG
> > + */
> > +#ifndef __LINUX_RPDEV_INFO_H__
> > +#define __LINUX_RPDEV_INFO_H__
> > +
> > +#define MAX_DEV_PER_CHANNEL    10
> > +
> > +/**
> > + * rpdev_platform_info - store the platform information of rpdev
> > + * @rproc_name: the name of the remote proc.
> > + * @rpdev: rpmsg channel device
> > + * @device_node: pointer to the device node of the rpdev.
> > + * @rx_callback: rx callback handler of the rpdev.
> > + * @channel_devices: an array of the devices related to the rpdev.
> > + */
> > +struct rpdev_platform_info {
> > +     const char *rproc_name;
> > +     struct rpmsg_device *rpdev;
> > +     struct device_node *channel_node;
> > +     int (*rx_callback)(struct rpmsg_device *rpdev, void *data,
> > +                        int len, void *priv, u32 src);
> > +     void *channel_devices[MAX_DEV_PER_CHANNEL];
> > +};
> > +
> > +#endif /* __LINUX_RPDEV_INFO_H__ */