Hi Arnaud,
-----Original Message----- From: Arnaud POULIQUEN arnaud.pouliquen@foss.st.com Sent: Friday, October 3, 2025 2:40 AM To: Shenwei Wang shenwei.wang@nxp.com; Bjorn Andersson andersson@kernel.org; Mathieu Poirier mathieu.poirier@linaro.org; Rob Herring robh@kernel.org; Krzysztof Kozlowski krzk+dt@kernel.org; Conor Dooley conor+dt@kernel.org; Shawn Guo shawnguo@kernel.org; Sascha Hauer s.hauer@pengutronix.de; Linus Walleij linus.walleij@linaro.org; Bartosz Golaszewski brgl@bgdev.pl Cc: Pengutronix Kernel Team kernel@pengutronix.de; Fabio Estevam festevam@gmail.com; Peng Fan peng.fan@nxp.com; linux- remoteproc@vger.kernel.org; devicetree@vger.kernel.org; imx@lists.linux.dev;
These processors communicate via the RPMSG protocol. The driver implements the standard GPIO interface, allowing the Linux side to control GPIO controllers which reside in the remote processor via RPMSG protocol.
What about my request in previous version to make this driver generic?
The only platform-dependent part of this driver is the layout of the message packet, which defines the communication protocol between the host and the remote processor. It would be challenging to require other vendors to follow our protocol and conform to the expected behavior.
In ST we have similar driver in downstream, we would be interested in reusing it if generic. Indeed we need some rpmsg mechanism for gpio-interrupt. Today we have a downstream rpmsg driver [1][2] that could migrate on a generic rpmsg- gpio driver.
+#include <linux/err.h> +#include <linux/gpio/driver.h> +#include <linux/rpmsg/imx_rpmsg.h> +#include <linux/init.h> +#include <linux/irqdomain.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/rpmsg.h>
+#define IMX_RPMSG_GPIO_PER_PORT 32 +#define RPMSG_TIMEOUT 1000
+enum gpio_input_trigger_type {
GPIO_RPMSG_TRI_IGNORE,GPIO_RPMSG_TRI_RISING,GPIO_RPMSG_TRI_FALLING,GPIO_RPMSG_TRI_BOTH_EDGE,GPIO_RPMSG_TRI_LOW_LEVEL,GPIO_RPMSG_TRI_HIGH_LEVEL,+};
What about taking inspiration from the|IRQ_TYPE|bitfield defined in|irq.h|? For instance: GPIO_RPMSG_TRI_BOTH_EDGE = GPIO_RPMSG_TRI_FALLING | GPIO_RPMSG_TRI_RISING,
Yes, the suggestion is better.
+enum gpio_rpmsg_header_type {
GPIO_RPMSG_SETUP,GPIO_RPMSG_REPLY,GPIO_RPMSG_NOTIFY,+};
+enum gpio_rpmsg_header_cmd {
GPIO_RPMSG_INPUT_INIT,GPIO_RPMSG_OUTPUT_INIT,GPIO_RPMSG_INPUT_GET,+};
+struct gpio_rpmsg_data {
struct imx_rpmsg_head header;u8 pin_idx;u8 port_idx;union {u8 event;u8 retcode;u8 value;} out;union {u8 wakeup;u8 value;nitpicking put "value" field out of union as common
I'm afraid we can't make it common, as the two 'value' fields serve different purposes-one is used for outgoing messages and the other for incoming messages-and they are located in different parts of the packet.
} in;+} __packed __aligned(8);
Any reason to pack it an align it? This structure will be copied in the RPMSg payload, right?
Yes. The message will then be transmitted via the MU hardware to the remote processor, so proper alignment is required.
I also wonder if that definition should not be in a header file with double licensing that the DT. Indeed this need to be common with the remote side implementation that can be non GPL implementation.
+struct imx_rpmsg_gpio_pin {
u8 irq_shutdown;u8 irq_unmask;u8 irq_mask;u32 irq_wake_enable;u32 irq_type;struct gpio_rpmsg_data msg;+};
+struct imx_gpio_rpmsg_info {
struct rpmsg_device *rpdev;struct gpio_rpmsg_data *notify_msg;struct gpio_rpmsg_data *reply_msg;struct completion cmd_complete;struct mutex lock;msg->pin_idx = gpio;msg->port_idx = port->idx;ret = gpio_send_message(port, msg, true);if (!ret)ret = !!port->gpio_pins[gpio].msg.in.value;Does this code is save? !! return a boolean right? why not force to 1 if greater that 1?
This approach is intended to simplify the implementation. Forcing to 1 would introduce an additional condition check. I'm open to changing it if that's considered standard practice.
return ret;+}
+static int imx_rpmsg_gpio_direction_input(struct gpio_chip *gc,
unsigned int gpio) {struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);struct gpio_rpmsg_data *msg = NULL;guard(mutex)(&port->info.lock);msg = gpio_get_pin_msg(port, gpio);msg->header.cate = IMX_RPMSG_GPIO;Do you use a single rpmsg channel for several feature? Any reason to not use one rpmsg channel per feature category?
The current implementation on the remote side uses a single channel to handle all GPIO controllers. However, this driver itself does not have that limitation.
msg->header.major = IMX_RMPSG_MAJOR;msg->header.minor = IMX_RMPSG_MINOR;msg->header.type = GPIO_RPMSG_SETUP;msg->header.cmd = GPIO_RPMSG_INPUT_INIT;msg->pin_idx = gpio;msg->port_idx = port->idx;msg->out.event = GPIO_RPMSG_TRI_IGNORE;msg->in.wakeup = 0;return gpio_send_message(port, msg, true); }+static inline void imx_rpmsg_gpio_direction_output_init(struct gpio_chip *gc,
unsigned int gpio, int val, struct gpio_rpmsg_data *msg)+static struct platform_driver imx_rpmsg_gpio_driver = {
.driver = {.name = "gpio-imx-rpmsg",.of_match_table = imx_rpmsg_gpio_dt_ids,},.probe = imx_rpmsg_gpio_probe,+};
+module_platform_driver(imx_rpmsg_gpio_driver);
This implementation seems strange to me. You have a platform driver, but your RPMsg driver appears split between this driver and the remoteproc driver, especially regarding the imx_rpmsg_endpoint_probe() function.
See my reply below.
From my point of view, this driver should declare both a platform_driver and an rpmsg_driver structures.
Your imx_rpmsg_gpio_driver platform driver should be probed by your remoteproc platform. This platform driver would be responsible for:
- Parsing the device tree node
- Registering the RPMsg driver
Then, the RPMsg device should be probed either by the remote processor using the name service announcement mechanism or if not possible by your remoteproc driver.
The idea is to probe the GPIO driver successfully only after the remote processor is online and has sent the name service announcement. Until then, the GPIO driver will remain in a deferred state, ensuring that all consumers of the associated GPIOs are also deferred. The implementation you provided below does not guarantee that the related consumers will be properly deferred. This is the most important behavior for a GPIO/I2C controllers.
Thanks, Shenwei
To better understand my proposal you can have a look to [1]and [2]. Here is another example for an rpmsg_i2c( ST downstream implementation): https://github.co/ m%2FSTMicroelectronics%2Flinux%2Fblob%2Fv6.6- stm32mp%2Fdrivers%2Fi2c%2Fbusses%2Fi2c- rpmsg.c&data=05%7C02%7Cshenwei.wang%40nxp.com%7C22a9c88be60b474e 391008de02502ec7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63 8950740622597592%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRyd WUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% 3D%7C0%7C%7C%7C&sdata=6lCk20Qhb%2F0MTw0NFtto7tj7EFYwZ%2BlOR1F3 Qk7kQn8%3D&reserved=0 https://github.co/ m%2FSTMicroelectronics%2Flinux%2Fblob%2Fv6.6- stm32mp%2FDocumentation%2Fdevicetree%2Fbindings%2Fi2c%2Fi2c- rpmsg.yaml&data=05%7C02%7Cshenwei.wang%40nxp.com%7C22a9c88be60b4 74e391008de02502ec7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7 C638950740622612512%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnR ydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D %3D%7C0%7C%7C%7C&sdata=4Gva%2FpqP2u8T57XDxSDaoHhvDeJ%2Fo5HtAB L9TY5gbDI%3D&reserved=0
Thanks and Regards, Arnaud
+MODULE_AUTHOR("NXP Semiconductor"); +MODULE_DESCRIPTION("NXP i.MX SoC rpmsg gpio driver"); +MODULE_LICENSE("GPL");