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

On Tue, Dec 23, 2025 at 07:47:31PM +0000, Shenwei Wang wrote:

quoted

-----Original Message-----
From: Bjorn Andersson <andersson@kernel.org>
Sent: Thursday, December 18, 2025 8:24 PM
To: Shenwei Wang <shenwei.wang@nxp.com>
Cc: Linus Walleij <linusw@kernel.org>; Bartosz Golaszewski <brgl@kernel.org>;
Rob Herring [off-list ref]; Krzysztof Kozlowski [off-list ref];
Conor Dooley [off-list ref]; Mathieu Poirier
[off-list ref]; Shawn Guo [off-list ref]; Sascha
Hauer [off-list ref]; Jonathan Corbet [off-list ref];
Pengutronix Kernel Team [off-list ref]; Fabio Estevam
[off-list ref]; Peng Fan [off-list ref]; linux-
gpio@vger.kernel.org; devicetree@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-remoteproc@vger.kernel.org; imx@lists.linux.dev;
linux-arm-kernel@lists.infradead.org; linux-doc@vger.kernel.org; dl-linux-imx
[off-list ref]
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:

quoted

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.

quoted

The following illustrates the expected DTS layout structure:

      cm33: remoteproc-cm33 {
              compatible = "fsl,imx8ulp-cm33";

              rpmsg {
                      rpmsg-io-channel {
                              gpio@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.

quoted

quoted

                              };

                              gpio@1 {
                                      compatible = "fsl,imx-rpmsg-gpio";
                                      reg = <1>;
                              };

                              ...
                      };

                      ...
              };
      };

+     drvdata->rpdev = rpdev;
+     auxdata[0].compatible = devm_kstrdup(dev, imx_rpdrv->compat,

GFP_KERNEL);

quoted

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

quoted

quoted

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

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

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

quoted

quoted

+};

Regards,
Bjorn

quoted

+
+#endif /* __LINUX_RPDEV_INFO_H__ */
--
2.43.0