[PATCH v6 2/5] remoteproc: imx_rproc: Populate devices under "rpmsg" subnode
Bjorn Andersson
andersson at kernel.org
Tue Dec 23 16:15:27 PST 2025
On Tue, Dec 23, 2025 at 07:47:31PM +0000, Shenwei Wang wrote:
>
>
> > -----Original Message-----
> > From: Bjorn Andersson <andersson at kernel.org>
> > Sent: Thursday, December 18, 2025 8:24 PM
> > To: Shenwei Wang <shenwei.wang at nxp.com>
> > Cc: 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>; 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>;
> > 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
> >
> > Caution: This is an external email. Please take care when clicking links or opening
> > attachments. When in doubt, report the message using the 'Report this email'
> > button
> >
> >
> > On Fri, Dec 12, 2025 at 01:43:38PM -0600, Shenwei Wang wrote:
> > > Register the RPMsg channel driver and populate remote devices defined
> > > under the "rpmsg" subnode upon receiving their notification messages.
> >
> > Please provide a proper description of what "problem" this patch solves.
> >
> > >
> > > The following illustrates the expected DTS layout structure:
> > >
> > > cm33: remoteproc-cm33 {
> > > compatible = "fsl,imx8ulp-cm33";
> > >
> > > rpmsg {
> > > rpmsg-io-channel {
> > > gpio at 0 {
> > > compatible = "fsl,imx-rpmsg-gpio";
> > > reg = <0>;
> >
> > Surely there needs to be some "gpio-controller" and "#gpio-cells" here?
> > Would be useful if the example is somewhat complete, to give a picture of what's
> > actually going on.
> >
>
> Okay. Will add those in next version.
>
> > > };
> > >
> > > gpio at 1 {
> > > compatible = "fsl,imx-rpmsg-gpio";
> > > reg = <1>;
> > > };
> > >
> > > ...
> > > };
> > >
> > > ...
> > > };
> > > };
> > >
> > > + drvdata->rpdev = rpdev;
> > > + auxdata[0].compatible = devm_kstrdup(dev, imx_rpdrv->compat,
> > GFP_KERNEL);
> > > + auxdata[0].platform_data = drvdata;
> > > + dev_set_drvdata(dev, drvdata);
> > > +
> > > + of_platform_populate(drvdata->channel_node, NULL, auxdata, dev);
> >
> > auxiliary_bus would be a better choice, but I don't understand why you probe a
> > rpmsg_device for each "gpio channel" and then from that create a
> > platform_device.
> >
> > Why don't you just make the rpmsg_device register the gpio controller directly?
> >
>
> The "GPIO channel" is just one example-there are also "PWM channel", "I2C channel", and other channels.
> The goal is to manage all these channels under a common logic, which helps avoid redundant code and keeps
> the implementation consistent.
>
If you make rpmsg_drivers for each of these channels/functions, then all
the common code should already be in the rpmsg framework.
What are you missing there?
> > > +
> > > + return 0;
> > > +}
> > > +
> > > + 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);
> >
> > This would then also imply that it's the gpio driver that registers the
> > rpmsg_driver.
> >
> > > +
> > > + 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);
> >
> > So if you don't recognize the child device node name you just register
> > platform_devices for each of the children?
> >
>
> Yes. That would register platform_devices without the platform_data.
>
> > > + 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 {
> >
> > I don't understand what this structure represents. Why is this glue between the
> > rpmsg_device and a made up platform_device needed?
> >
>
> The purpose is to have a shared array that can be accessed by all devices within
> the same channel.
>
What does this mean? How are multiple functions multiplexed over a
single rpmsg channel/endpoint?
Please provide a concrete description of how a device with some gpios
and PWMs would actually look in this model.
> > > + const char *rproc_name;
> >
> > You don't need this, because you can rproc_get_by_child(&self) and then get the
> > remoteproc name from that.
> >
>
> Good to know. Will try it in the next version.
>
> > > + 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];
> >
> > Why 10? What does it mean?
> >
>
> This is based on practical experience. For example, on the i.MX platform, we typically don't have
> more than eight same devices on the remote system.
>
> > I think this becomes the list of the 10 grandchildren of the remoteproc (per child
> > node). So what happens if those matches against two different drivers, what will
> > rx_callback point to?
> >
>
> This is the limitation. That's why I used the map to populate the known child device for one specific channel.
>
So for each rpdev_platform_info there can only be one type of client
driver. Where is this limitation defined? What happens if I put a PWM
and a GPIO controller under my rpmsg-io-channel?
Regards,
Bjorn
> Thanks,
> Shenwei
>
> > > +};
> >
> > Regards,
> > Bjorn
> >
> > > +
> > > +#endif /* __LINUX_RPDEV_INFO_H__ */
> > > --
> > > 2.43.0
> > >
More information about the linux-arm-kernel
mailing list