Message-ID: <0be8c911-3c31-40da-b431-e5a24339c0f9@lunn.ch>
Date: Thu, 6 Nov 2025 22:32:33 +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" <linux-remoteproc@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	dl-linux-imx <linux-imx@....com>
Subject: Re: [PATCH v5 3/5] docs: staging: gpio-rpmsg: gpio over rpmsg bus

On Thu, Nov 06, 2025 at 08:40:05PM +0000, Shenwei Wang wrote:
> Hi Andrew,
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@...n.ch>
> > Sent: Thursday, November 6, 2025 1:06 PM
> > 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; dl-linux-imx <linux-imx@....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?

I think we need to know more about your system. Why does cate exist?
Why are you mixing different functions onto one channel, rather than
having a channel per function?

I think you should remove cate from the GPIO protocol.

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

What does not answer my question: What should happen if an invalid
value is passed?

You must have major:minor for a reason. You expect to change
them. Then what happens? How should forward/backwards compatibility
work? Must version 0:0 always be implemented, were as 0:1 is optional?
How do you find out if 0:1 is implemented?

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

You need to specify this in the protocol. Looking at the messages, i
don't see why i could not send off a batch of requests with different
line values, and later expect a batch of replies with different line
values. The linux driver can then match the replies to the request and
know they are all answered. The current specification allows that.

You really need to forget about your implementation. Look at the
specification, and think about all the different ways it can be
implemented, and conform to the specification. If there is only one
possible implementation, your specification is good. If it can be
implemented in multiple ways, you need to improve the specification.

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

Please state that. 

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

We don't care about other functions. This is all about GPIO.

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

That implies that you cannot look at major:minor and know if the
reserved bits are reserved, or have some actual meaning?

Again, think about forward/backwards compatibility. How do you turn a
reserved bit into a used bit? Is the specification sufficient to make
that possible.

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

Keep going.....

Does that imply if none of the lines have wakeup enabled, the GPIO
controller can be suspended when Linux suspends? How does the GPIO
controller know it can suspend? There is no message for that. How does
it know to come out of suspension?

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

At sounds broken.

A level interrupt is not cleared until the level changes. The typical
flow is:

Interrupt fires.

Interrupt is masked

Interrupt handler is called, which reads/write registers in the device
who pin is connected to the GPIO

Interrupt is unmasked



At the unmask point, the interrupt will fire again, if the level is
still in the active state. You need this because while reading/writing
registers in the device, another event can happen, which needs
handling. Generally, the interrupt output of a device is an OR of many
different sources. You only release the interrupt when all sources
have been cleared.

So for the protocol, you need to acknowledge the notification after
the interrupt handler has executed. And that could cause another
notification to be immediately sent if the line is still active.

I'm not too sure how edge interrupts should work. If we are still busy
in an edge interrupt handler, do we want to know about the next edge?
Should edge interrupts be disabled until the previous edge has been
handled? I'm not too familiar with edge interrupts, most of the device
i've handled are level.

	Andrew

Powered by blists - more mailing lists