Re: [PATCH v13 3/4] gpio: rpmsg: add generic rpmsg GPIO driver

[Beleswar Prasad Padhi <b-padhi@ti.com>]

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.

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