Re: [PATCH v6 2/5] remoteproc: imx_rproc: Populate devices under "rpmsg" subnode
From: Bjorn Andersson <andersson@kernel.org>
Date: 2025-12-19 02:23:41
Also in:
imx, linux-arm-kernel, linux-devicetree, linux-doc, linux-remoteproc, lkml
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@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.
quoted hunk ↗ jump to hunk
}; gpio@1 { compatible = "fsl,imx-rpmsg-gpio"; reg = <1>; }; ... }; ... }; }; Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com> --- drivers/remoteproc/imx_rproc.c | 143 +++++++++++++++++++++++++++++++ include/linux/rpmsg/rpdev_info.h | 33 +++++++ 2 files changed, 176 insertions(+) create mode 100644 include/linux/rpmsg/rpdev_info.hdiff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c index 33f21ab24c92..65ee16fd66d1 100644 --- a/drivers/remoteproc/imx_rproc.c +++ b/drivers/remoteproc/imx_rproc.c@@ -15,6 +15,8 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/of_platform.h> #include <linux/of_reserved_mem.h> #include <linux/platform_device.h> #include <linux/pm_domain.h>@@ -22,6 +24,8 @@ #include <linux/reboot.h> #include <linux/regmap.h> #include <linux/remoteproc.h> +#include <linux/rpmsg.h> +#include <linux/rpmsg/rpdev_info.h> #include <linux/workqueue.h> #include "imx_rproc.h"@@ -1016,6 +1020,141 @@ static void imx_rproc_destroy_workqueue(void *data) destroy_workqueue(workqueue); } +struct imx_rpmsg_driver { + struct rpmsg_driver rpdrv; + const char *compat; + void *driver_data; +}; + +static const char *channel_device_map[][2] = { + {"rpmsg-io-channel", "rpmsg-gpio"}, +}; + +static int imx_rpmsg_endpoint_cb(struct rpmsg_device *rpdev, void *data, + int len, void *priv, u32 src) +{ + struct rpdev_platform_info *drvdata; + + drvdata = dev_get_drvdata(&rpdev->dev); + if (drvdata && drvdata->rx_callback) + return drvdata->rx_callback(rpdev, data, len, priv, src); + + return 0; +} + +static void imx_rpmsg_endpoint_remove(struct rpmsg_device *rpdev) +{ + of_platform_depopulate(&rpdev->dev); +} + +static int imx_rpmsg_endpoint_probe(struct rpmsg_device *rpdev) +{ + struct rpdev_platform_info *drvdata; + struct imx_rpmsg_driver *imx_rpdrv; + struct device *dev = &rpdev->dev; + struct of_dev_auxdata *auxdata; + struct rpmsg_driver *rpdrv; + + rpdrv = container_of(dev->driver, struct rpmsg_driver, drv); + imx_rpdrv = container_of(rpdrv, struct imx_rpmsg_driver, rpdrv); + + if (!imx_rpdrv->driver_data) + return -EINVAL; + + drvdata = devm_kmemdup(dev, imx_rpdrv->driver_data, sizeof(*drvdata), GFP_KERNEL); + if (!drvdata) + return -ENOMEM; + + auxdata = devm_kzalloc(dev, sizeof(*auxdata) * 2, GFP_KERNEL); + if (!auxdata) + return -ENOMEM; + + 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?
+
+ return 0;
+}
+
+static const char *imx_of_rpmsg_is_in_map(const char *name)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(channel_device_map); i++) {
+ if (strcmp(name, channel_device_map[i][0]) == 0)
+ return channel_device_map[i][1];
+ }
+
+ return NULL;
+}
+
+static int imx_of_rpmsg_register_rpdriver(struct device_node *channel,
+ struct device *dev,
+ const char *name,
+ const char *compat)
+{
+ struct rpdev_platform_info *driver_data;
+ struct imx_rpmsg_driver *rp_driver;
+ struct rpmsg_device_id *rpdev_id;
+
+ /* rpmsg_device_id is a NULL terminated array */
+ rpdev_id = devm_kzalloc(dev, sizeof(*rpdev_id) * 2, GFP_KERNEL);
+ if (!rpdev_id)
+ return -ENOMEM;
+
+ strscpy(rpdev_id[0].name, name, RPMSG_NAME_SIZE);
+
+ rp_driver = devm_kzalloc(dev, sizeof(*rp_driver), GFP_KERNEL);
+ if (!rp_driver)
+ return -ENOMEM;
+
+ driver_data = devm_kzalloc(dev, sizeof(*driver_data), GFP_KERNEL);
+ if (!driver_data)
+ return -ENOMEM;
+
+ driver_data->rproc_name = dev->of_node->name;
+ driver_data->channel_node = channel;
+
+ 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?
quoted hunk ↗ jump to hunk
+ 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?
+ 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.
+ 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? 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?
+};
Regards, Bjorn
+ +#endif /* __LINUX_RPDEV_INFO_H__ */ -- 2.43.0