Date:   Mon, 28 Feb 2022 10:02:47 +0100
From:   Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>
To:     Philipp Rossak <embed3d@...il.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>
CC:     <linux-remoteproc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <julien.massot@....bzh>, Xiang Xiao <xiaoxiang@...omi.com>
Subject: Re: [PATCH v9 00/11] Restructure the rpmsg_char driver and introduce
 rpmsg_ctrl driver

Hi Philipp,

On 2/25/22 22:45, Philipp Rossak wrote:
> Hi Arnaud,
> 
> On 24.02.22 09:29, Arnaud POULIQUEN wrote:
>> Hi Philipp,
>>
>> On 2/23/22 22:28, Philipp Rossak wrote:
>>> Hi Arnaud,
>>>
>>> thanks for working on this! I'm currently testing/using this patch
>>> series on my imx7d project because it adds the capability that the
>>> remote processor can register it's endpoints dynamically (as mentioned
>>> in the objectives).
>>
>> Thanks for your feedback on this work!
>> Don't hesitate to add your tested-by, this help maintainers for the
>> reviews.
>>
> I will do this.
>>>
>>> After a few tests, debugging, and checking the openamp specification [1]
>>> I think that you missed the second ns_announcement that should be sent
>>> from linux master to the slave after it created the channel/endpoint.
>>> Without this second announcement the remote processor is not able to
>>> send messages to the linux master because it doesn't know the
>>> destination address until it receives a message from the linux master.
>>
>> Yes I detected this issues, it is not related to the series
>> but to the remoteproc_virtio backend.
>>
>> As you mentioned, after the ns announcement from Linux, the remote
>> processor
>> send first messages. But the Linux virtio does not do the match
>> between the
>> local channel created and the remote endpoint.
>>
> 
> I'm not sure if we talk about the same. I'm basically talking about the
> dynamic binding, not dynamic endpoint creation.

Regarding your following description, yes it is not exactly the same issue.
  

> I think I already found the issue. I will try to get a bit more into
> detail.
> 
> 1. Linux: starts co-processor via remoteproc
> 2. co-processor: boots and reaches the point where it creates the
> endpoint like it is done in this ST example[1].
> Be aware the src address is RPMSG_ADDR_ANY
> 3. co-processor: reaches the point where it sends the ns_announcement to
> linux ns endpoint
> 4. linux: receives the ns announcment, creates the channel, bindes the
> endpoint and checks here [2] if the source address is not RPMSG_ADDR_ANY
> and in this case it is not sending a ns_announcement (that's the issue
> when we use dynamic endpoints)

The ns annoucement is used to notify the remote processor that a new channel
has been created locally. Today the ns anoucement is not used to inform that

The local endpoint has been binded.

This behavior is something that as already been identified as a limitation in
the virtio rpmsg.

Xiang Xiao had proposed some time ago a mechanism for the OpenAMP [5], the
Linux part is missing. We need a common solution between Linux and OpenAMP, but
that also compatible with legacy. 
>From MPOV Xiang's approach seem a good starting point.  
If you are interesting on working on this enhancement of the rpmsg virtio, feel
free to do so.

> 5. linux: according the openamp spec [3] it should now send the
> ns_announcement to the co-processor (slave)
> 6. co-processor: should receive the ns announcement and binds now the
> endpoint
> 7. co-processor: can now send messages to linux
> 
> This is basically what I'm expecting.
> 
> 
> Do you think this is a bug or is the dynamic endpoint binding not
> handled? This line is there since ever [4] ...

As you mentioned this is a legacy limitation that should be addressed.
It is not related to this work.

Thanks,
Arnaud

> 
> Any other thoughts about this?
> 
>> This is a feature that is missing in the rpmsg virtio, and perhaps in
>> rpmsg protocol
>> itself (a ns annoucement ack message or something similar).
>>
>>
>> A fix for the remoteproc virtio is available here:
>> https://github.com/arnopo/meta-st-stm32mp-oss/commit/3e57fe73bd19c9bb835ac5a118e50727758b0b96
>>
>>
>> Don't hesitate to give me feedback on the fix, if you test it.
> 
> I added it to my branch and till now I don't see any side effects
>>
>> I plan to propose the fix after this series.
>>
>> Thanks,
>> Arnaud
>>
>>>
>>> Cheers,
>>> Philipp
>>>
> 
> Cheers,
> Philipp
> 
> [1]:
> https://github.com/STMicroelectronics/STM32CubeMP1/blob/master/Projects/STM32MP157C-DK2/Applications/OpenAMP/OpenAMP_raw/Src/openamp.c#L242
> 
> 
> [2]:
> https://elixir.bootlin.com/linux/v5.17-rc5/source/drivers/rpmsg/virtio_rpmsg_bus.c#L425
> 
> 
> [3]:
> https://github.com/OpenAMP/open-amp/blob/main/docs/img/coprocessor-rpmsg-ns.png
> 
> 
> [4]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bcabbccabffe7326f046f25737ba1084f463c65c
> 

[5] https://github.com/OpenAMP/open-amp/pull/160

Powered by blists - more mailing lists