Message-ID:
 <PAXPR04MB9185D9EBE8F46715FD114A2989C2A@PAXPR04MB9185.eurprd04.prod.outlook.com>
Date: Thu, 6 Nov 2025 23:13:18 +0000
From: Shenwei Wang <shenwei.wang@....com>
To: Andrew Lunn <andrew@...n.ch>
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



> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Thursday, November 6, 2025 3:33 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
> > > > +
> > > > +- **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?
> 

At the protocol layer, there is no restriction of one function per channel because the 
Cate field in the message header is designed to differentiate multiple functions within 
the same channel. This provides flexibility for implementations that need to support 
multiple functionalities over a single communication path.

However, for a specific implementation, you may choose to enforce a one-function-per-channel 
approach by assigning a fixed value to the Cate field.

> I think you should remove cate from the GPIO protocol.
> 


The original proposal was to use a single transport message header to support multiple functions. 
Removing the Cate field would require significant changes to the current remote firmware. 
Therefore, I recommend keeping this field.

For implementations that do not need to support multiple functions, a fixed value (e.g., 5) can be used 
for the Cate field. This approach can run smoothly with the existing system and give the option for systems
that doesn't want to support multiple functions. 

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

That is an implementation detail of the remote firmware. If it chooses to support a 
specific protocol version, it can return an error when the version is not supported. 
For example, the existing i.MX remote firmware returns a "not supported" error in such cases.

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

The remote firmware decides how to respond to the protocol version. By default, it 
should return a "not supported" error for any unknown version.

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

The power state of the remote GPIO controller is entirely managed by the remote firmware. 
The remote firmware operates as an independent system from Linux, with its own power states 
and policies for transitioning between modes. The wakeup field is solely intended to inform the 
remote firmware whether the GPIO line should be used as a wakeup source for the Linux system.

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

The sequences you mentioned above are managed entirely by the remote firmware. On the Linux 
side, it only receives a notification message when a GPIO line is triggered, which then invokes the 
corresponding interrupt handler.

Since the interrupt handling sequences are implemented in the remote firmware, the Linux driver 
can treat level-triggered and edge-triggered types in the same manner.

Thanks,
Shenwei

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