Re: [PATCH v5 3/5] docs: staging: gpio-rpmsg: gpio over rpmsg bus

Shenwei Wang
2025-11-06T20:40:09Z

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, November 6, 2025 1:06 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: 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>;
> Jonathan Corbet <corbet@lwn.net>; Linus Walleij <linus.walleij@linaro.org>;
> Bartosz Golaszewski <brgl@bgdev.pl>; 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; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> doc@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH v5 3/5] docs: staging: gpio-rpmsg: gpio over rpmsg bus
> > + +-----+------+------+-----+-----+------------+-----+-----+-----+----+
> > + |0x00 |0x01 |0x02 |0x03 |0x04 |0x05..0x09 |0x0A |0x0B |0x0C |0x0D|
> > + |cate |major |minor |type |cmd |reserved[5] |line |port | data |
> > +
> > + +-----+------+------+-----+-----+------------+-----+-----+-----+----
> > + +
> > +
> > +- **Cate (Category field)**: Indicates the category of the message, such as
> GPIO, I2C, PMIC, AUDIO, etc.
>
> We know it is a GPIO message, this document is titled "GPIO RPMSG Protocol". So
> i don't see the need for cate. I can however understand that your device does
> support multiple functions, but to make this generic, it would be better if each
> function had its own channel.
>

These details are defined in the message header to support multiple functions. For
the GPIO-specific implementation, the header values are fixed and do not require
modification, provided the transport protocol version remains unchanged.

Then I should remove those unrelated definitions from this document. Is my understanding
correct?

> > +
> > + Defined categories:
> > +
> > + - 1: RPMSG_LIFECYCLE
> > + - 2: RPMSG_PMIC
> > + - 3: RPMSG_AUDIO
> > + - 4: RPMSG_KEY
> > + - 5: RPMSG_GPIO
> > + - 6: RPMSG_RTC
> > + - 7: RPMSG_SENSOR
> > + - 8: RPMSG_AUTO
> > + - 9: RPMSG_CATEGORY
> > + - A: RPMSG_PWM
> > + - B: RPMSG_UART
> > +
> > +- **Major**: Major version number.
> > +
> > +- **Minor**: Minor version number.
>
> What is the purpose of Major and Minor? What values are valid. What should
> happen if an invalid value is passed?
>
> What you should think about is, if you gave this specification to somebody else,
> could they implement it?
>

Okay. Will change those fields to fixed number and remove the above definitions.

> > +
> > +- **Type (Message Type)**: For the GPIO category, can be one of:
> > +
> > + - 0: GPIO_RPMSG_SETUP
> > + - 1: GPIO_RPMSG_REPLY
> > + - 2: GPIO_RPMSG_NOTIFY
>
> Is _SETUP always from Linux to the firmware? Is a setup always followed by a
> _REPLY? Do you need to wait for the _REPLY before sending the next _SETUP? If
> there is no _REPLY within X seconds, should Linux retry? Can an _NOTIFY arrive
> between a _SETUP and a _REPLY?
>

Yes, the SETUP message is always sent from Linux to the remote firmware. Each SETUP
corresponds to a single REPLY message. The GPIO driver serializes messages and determines
whether a REPLY is required. If a REPLY is expected but not received within the timeout
period (currently 1 second in the driver), the driver returns -ETIMEOUT.

A NOTIFY message can arrive between a SETUP and the REPLY, and the driver is designed to handle this scenario.

> > +
> > +- **Cmd**: Command code, used for GPIO_RPMSG_SETUP messages.
> > +
> > +- **reserved[5]**: Reserved bytes.
>
> Why are these in the middle. It is more normal to have reserved bytes at the end.
>

The reserved[5] bytes may be used for other type of functions. For GPIO, all should be 0.

> You should also specify these bytes should be set to 0. If you don't it will be hard
> to use them in the future, because they will contain 42, or some other random
> values.
>
> Is there a relationship between major:minor and reserved?
>

No so far. Maybe it could be related if the protocol is updated in the future.

> > +
> > +- **line**: The GPIO line index.
> > +
> > +- **port**: The GPIO controller index.
>
> data?
>

Data is explained in the commands below.

> > +GPIO Commands
> > +-------------
>
> This is a GPIO specification, so i would only expect GPIO commands...
>
> > +
> > +Commands are specified in the **Cmd** field for **GPIO_RPMSG_SETUP**
> (Type=0) messages.
> > +
> > +GPIO_RPMSG_INPUT_INIT (Cmd=0)
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +**Request:**
> > +
> > +.. code-block:: none
> > +
> > + +-----+-----+-----+-----+-----+-----------+-----+-----+-----+----+
> > + |0x00 |0x01 |0x02 |0x03 |0x04 |0x05..0x09 |0x0A |0x0B |0x0C |0x0D|
> > + | 5 | 1 | 0 | 0 | 0 | 0 |line |port | val | wk |
> > + +-----+-----+-----+-----+-----+-----------+-----+-----+-----+----+
> > +
> > +- **val**: Interrupt trigger type.
> > +
> > + - 0: Interrupt disabled
> > + - 1: Rising edge trigger
> > + - 2: Falling edge trigger
> > + - 3: Both edge trigger
> > + - 4: Low level trigger
> > + - 5: High level trigger
> > +
> > +- **wk**: Wakeup enable.
> > +
> > + - 0: Disable wakeup from GPIO
> > + - 1: Enable wakeup from GPIO
>
> What do you mean by wakeup?
>

The gpio line can be enabled as an wakeup source for the system.

> > +
> > +**Reply:**
> > +
> > +.. code-block:: none
> > +
> > + +-----+-----+-----+-----+-----+-----------+-----+-----+-----+----+
> > + |0x00 |0x01 |0x02 |0x03 |0x04 |0x05..0x09 |0x0A |0x0B |0x0C |0x0D|
> > + | 5 | 1 | 0 | 1 | 1 | 0 |line |port | err | 0 |
> > + +-----+-----+-----+-----+-----+-----------+-----+-----+-----+----+
> > +
> > +- **err**: Error code from the remote core.
> > +
> > + - 0: Success
> > + - 1: General error (early remote software only returns this
> > + unclassified error)
> > + - 2: Not supported
> > + - 3: Resource not available
> > + - 4: Resource busy
> > + - 5: Parameter error
>
> It would be good to give some examples of when these should be used.
>
> Say the hardware does not support both edges. Is that 2? Why would a resource
> be busy? How is busy different to not available?
>
> > +
> > +GPIO_RPMSG_OUTPUT_INIT (Cmd=1)
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +**Request:**
> > +
> > +.. code-block:: none
> > +
> > + +-----+-----+-----+-----+-----+-----------+-----+-----+-----+----+
> > + |0x00 |0x01 |0x02 |0x03 |0x04 |0x05..0x09 |0x0A |0x0B |0x0C |0x0D|
> > + | 5 | 1 | 0 | 0 | 1 | 0 |line |port | val | 0 |
> > + +-----+-----+-----+-----+-----+-----------+-----+-----+-----+----+
> > +
> > +- **val**: Output level.
> > +
> > + - 0: Low
> > + - 1: High
>
> Maybe make a comment about the order. Some GPIO controllers suffer from
> glitches when you swap them from input to output. While it is an input, you first
> need to set the output value, and then configure the pin for output.
>

These are platform-dependent details that should be managed by the remote firmware.
In the Linux driver, we simply request the remote side to configure the GPIO direction.

> > +Notification Message
> > +--------------------
> > +
> > +Notifications are sent with **Type=2 (GPIO_RPMSG_NOTIFY)**:
> > +
> > +.. code-block:: none
> > +
> > + +-----+-----+-----+-----+-----+-----------+-----+-----+-----+----+
> > + |0x00 |0x01 |0x02 |0x03 |0x04 |0x05..0x09 |0x0A |0x0B |0x0C |0x0D|
> > + | 5 | 1 | 0 | 2 | 0 | 0 |line |port | 0 | 0 |
> > + +-----+-----+-----+-----+-----+-----------+-----+-----+-----+----+
> > +
> > +- **line**: The GPIO line index.
> > +- **port**: The GPIO controller index.
>
> There is no need to acknowledge the notification? How do level interrupts work?
>

Currently, there is no need to acknowledge the message, as the interrupt is managed entirely
by the remote firmware. On the Linux side, a single notification message is received when an
interrupt is triggered.

Thanks,
Shenwei

> Andrew