Message-ID: <9fd8ccd9-560a-43b4-a48d-f7a3eaa07eb1@lunn.ch>
Date: Thu, 6 Nov 2025 20:05:50 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Shenwei Wang <shenwei.wang@....com>
Cc: Bjorn Andersson <andersson@...nel.org>,
	Mathieu Poirier <mathieu.poirier@...aro.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Jonathan Corbet <corbet@....net>,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, Peng Fan <peng.fan@....com>,
	linux-remoteproc@...r.kernel.org, devicetree@...r.kernel.org,
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-imx@....com
Subject: Re: [PATCH v5 3/5] docs: staging: gpio-rpmsg: gpio over rpmsg bus

On Tue, Nov 04, 2025 at 02:33:13PM -0600, Shenwei Wang wrote:
> Describes the gpio rpmsg transport protocol over the rpmsg bus between
> the cores.
> 
> Signed-off-by: Shenwei Wang <shenwei.wang@....com>
> ---
>  Documentation/staging/gpio-rpmsg.rst | 202 +++++++++++++++++++++++++++
>  Documentation/staging/index.rst      |   1 +
>  2 files changed, 203 insertions(+)
>  create mode 100644 Documentation/staging/gpio-rpmsg.rst
> 
> diff --git a/Documentation/staging/gpio-rpmsg.rst b/Documentation/staging/gpio-rpmsg.rst
> new file mode 100644
> index 000000000000..ad6207a3093f
> --- /dev/null
> +++ b/Documentation/staging/gpio-rpmsg.rst
> @@ -0,0 +1,202 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +GPIO RPMSG Protocol
> +===================
> +
> +The GPIO RPMSG transport protocol is used for communication and interaction
> +with GPIO controllers located on remote cores on the RPMSG bus.
> +
> +Message Format
> +--------------
> +
> +The RPMSG message consists of a 14-byte packet with the following layout:
> +
> +.. code-block:: none
> +
> +   +-----+------+------+-----+-----+------------+-----+-----+-----+----+
> +   |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.

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

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

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

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?

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

data? 

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

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

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

	Andrew

Powered by blists - more mailing lists