Hi Beleswar

On 5/5/26 07:25, Beleswar Prasad Padhi wrote:
Hi Arnaud,

On 04/05/26 22:34, Arnaud POULIQUEN wrote:
Hi Beleswar,

On 5/4/26 10:17, Beleswar Prasad Padhi wrote:


[...]



I may have misunderstood your solution. Could you please help me
understand your proposal by explaining how you would handle three
GPIO ports defined in the DT, considering that the endpoint
addresses on the Linux side can be random?
If I assume there is a unique endpoint on the remote side,
I do not understand how you can match, on the firmware side,
the Linux endpoint address to the GPIO port.


Sure, let me take an example:
Assumptions: 3 GPIO ports in DT, 3 endpoints in Linux (one per port),
1 endpoint in remote (0xd) and 1 rpmsg channel (rpmsg-io)

         rpmsg {
           rpmsg-io {
             #address-cells = <1>;
             #size-cells = <0>;

             gpio@25 {
               compatible = "rpmsg-gpio";
               reg = <25>;
               gpio-controller;
               #gpio-cells = <2>;
               #interrupt-cells = <2>;
               interrupt-controller;
             };

             gpio@32 {
               compatible = "rpmsg-gpio";
               reg = <32>;
               gpio-controller;
               #gpio-cells = <2>;
               #interrupt-cells = <2>;
               interrupt-controller;
             };

             gpio@35 {
               compatible = "rpmsg-gpio";
               reg = <35>;
               gpio-controller;
               #gpio-cells = <2>;
               #interrupt-cells = <2>;
               interrupt-controller;
             };
           };
         };

Code Flow:
1. "rpmsg-io" channel is announced from remote firmware with unique dst
      ept = 0xd.

2. rpmsg_core.c creates the default dynamic local ept for the channel
      ept = 0x405.

3. rpmsg_core.c assigns the allocated addr to rpdev device:
      rpdev->src = 0x405 and rpdev->dst = 0xd.

4. rpmsg_gpio_channel_probe() is triggered. For *each* of the GPIO ports
      in DT, it will trigger rpmsg_gpiochip_register() which will now:
         a. Call port->ept = rpmsg_create_ept(rpdev,
                                                                     
rpmsg_gpio_channel_callback,
                                                                     port,
                                                                    
{rpdev.id.name,
                                                                     
RPMSG_ADDR_ANY,
                                                                     
RPMSG_ADDR_ANY});
             Ex- port->ept->addr = 0x408

         b. Prepare a 8-byte message having 2 fields:
             port->ept->addr (0x408) and port->idx (25)

         c. Send this message to remote firmware on default channel ept
             (0x405 -> 0xd) by:
             rpmsg_send(rpdev->ept, &message, sizeof(message));

         d. Remote side receives this message and creates a map of the
             linux_ept_addr to gpio_port. (0x408 <-> 25)

5. After this point, any gpio messages sent from Linux from gpio port
      endpoints (Ex- 0x408) can be decoded at remote side by looking up
      its map (Ex- map[0x408] = 25).

6. Any messages sent from remote to Linux for a particular gpio port can
      also be decoded at Linux by simply fetching the priv pointer to get
      the per-port device:
      struct rpmsg_gpio_port *port = priv;


Thanks for the details!

To sum up:
- the default endpoint acts as the GPIO controller (0x405),
- one extra Linux endpoint is created per port defined in DT.

This should work, but my concerns remain the same:

   1) This implementation forces the remote processor to handle a single
      endpoint instead of one endpoint per port. This may add complexity to
      the remote firmware if each port is managed in a separate thread.


A. Not really, I just chose 1 remote endpoint for this example as you
     suggested to. We can scale it for two-way communication via the
     get_config message like you suggested below.

B. Isn't it a bad design of the firmware if it is handling 10 gpio ports
     in 10 threads? The logic to handle all the ports is the same, only
     the parameters (e.g. line number, msg) is different.


   2) Linux, as a consumer, should not expose its capabilities to the remote
      side (in your proposal it enumerates the ports defined in the DT).     In 
my view, the remote processor should expose its capabilities as the
      provider.


Agreed on this.


 From my perspective, based on your proposal:
  1) Linux should send a get_config message to the remote proc (0x405 -> 0xD). 
2) The remote processor would respond with the list of ports, associated
     with an remote endpoint addresses.


Agreed, we can scale it for multiple remote endpoints like this.

  3) Linux would parse the response, compare it with the DT, enable the GPIO
     ports accordingly, creating it local endpoint and associating it with
     the remote endpoint.
Using name service to identify the ports should avoid step 1 & 2 ...


Yes, but won't that make a lot of hard-codings in the driver?

+static struct rpmsg_device_id rpmsg_gpio_channel_id_table[] = {
+    { .name = "rpmsg-io-25" },
+    { .name = "rpmsg-io-32" },
+    { .name = "rpmsg-io-35" },
+    { },
+};

What if tomorrow another vendor decides to add more remoteproc
controlled GPIO ports to Linux, they would have to update this struct in
the driver everytime. And the port indexes (25/32/35) could also differ
between vendors. We should make the driver dynamic i.e. vendor
agnostic.

I think querying the remote firmware at runtime (step 1 & 2 above) is a
common design pattern and makes the driver vendor agnostic. But feel
free to correct me.


You are right. My proposal would require a patch in rpmsg-core. The idea of
allowing a postfix in the compatible string has been discussed before, but,
if I remember correctly, it was not concluded.

/* rpmsg devices and drivers are matched using the service name */
static inline int rpmsg_id_match(const struct rpmsg_device *rpdev,
                                  const struct rpmsg_device_id *id)
{
        size_t len;

+       len = strnlen(id->name, RPMSG_NAME_SIZE);
+       if (len && id->name[len - 1] == '*')
+               return !strncmp(id->name, rpdev->id.name, len - 1);

        return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0;
}

Then, in rpmsg-gpio, and possibly in other drivers such as rpmsg-tty and
a future rpmsg-i2c, we could use:
static struct rpmsg_device_id rpmsg_gpio_channel_id_table[] = {
    { .name = "rpmsg-io" },
    { .name = "rpmsg-io-*" },
    { },
};

If exact name matching is strongly required, then this proposal would not be suitable.

A third option would be a combination of both approaches: instantiate the device using the same name service from the remote side, as done in rpmsg-tty. In that case, a get_config message, or a similar mechanism, would also be needed to retrieve the port information from the remote side.

Tanmaya also proposed another alternative based on reserved addresses.

At this point, I suggest letting Mathieu review the discussion and recommend the most suitable approach.

Thanks,
Arnaud


At the end, whatever solution is implemented, my main concern is that the
Linux driver design should, if possible, avoid adding unnecessary complexity
or limitations on the remote side (for instance in openAMP project).


Yes definitely, I want the same. Feel free to let me know if this does
not suit with the OpenAMP project.

Thanks,
Beleswar


Thanks,
Arnaud


So Linux does not need to send the port idx everytime while sending a
gpio message anymore.

Thanks,
Beleswar

[...]




Reply via email to