Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <670ee336-9ad4-401e-e3b2-02531e975e51@foss.st.com>
Date:   Thu, 24 Feb 2022 09:29:25 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v9 00/11] Restructure the rpmsg_char driver and introduce
 rpmsg_ctrl driver
Content-Language: en-US
To:     Philipp Rossak <embed3d@gmail.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>
CC:     <linux-remoteproc@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-stm32@st-md-mailman.stormreply.com>, <julien.massot@iot.bzh>
References: <20220124102524.295783-1-arnaud.pouliquen@foss.st.com>
 <ab97c2a5-7705-eae8-9bc2-908e7a9cb709@gmail.com>
From:   Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
In-Reply-To: <ab97c2a5-7705-eae8-9bc2-908e7a9cb709@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk

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. 

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

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 plan to propose the fix after this series.    

Thanks,
Arnaud

> 
> Cheers,
> Philipp
> 
> 
> [1]:
> https://github.com/OpenAMP/open-amp/blob/main/docs/img/coprocessor-rpmsg-ns.png
> 
> 
> On 24.01.22 11:25, Arnaud Pouliquen wrote:
>> Updates from V8 [1]:
>> - rebase on 5.17-rc1 + rpmsg char cdev release fixes[2][3]
>> - updates based on Bjorn Andersson's comments:
>>    - remove rpmsg_create_default_ept API, set directly the ept->priv
>> in rpmsg_chrdev_probe
>>      function.
>>    - rework commit message in [8/9]rpmsg: char: Introduce the
>> "rpmsg-raw" channel
>>
>> Patchset description:
>>
>> The current rpmsg_char module implements a /dev/rpmsg_ctrl interface
>> that provides the ability to
>> instantiate char devices (/dev/rpmsgX) associated with an rpmsg
>> endpoint for communication with the
>> remote processor.
>> This implementation fits with QCOM rpmsg backend but not with the
>> magement by chanel implemented
>> in the generic rpmsg virtio backend.
>> This series restructures the rpmsg_char driver to decorrelate the
>> control part from the data part
>> in order to improve its compatible with the rpmsg virtio backend.
>>
>> Objective:
>> - Expose a /dev/rpmsg_ctrlX interface for the application that is no
>> longer dedicated to the
>>    rpmsg_char but generalized to all rpmsg services. This offers
>> capability to create and destroy
>>    rpmsg channels from a user's application initiative (using the new
>> RPMSG_CREATE_DEV_IOCTL and
>>    RPMSG_DESTROY_DEV_IOCTL controls).
>>    An application will be able to create/establish an rpmsg
>> communication channel to communicate
>>    with the remote processor, and not only wait the remote processor
>> initiative.
>>    This is interesting for example to establish a temporary
>> communication link for diagnosis,
>>    calibration, debugging... or instantiate  new data flows on some
>> user actions.
>> - Add capability to probe the rpmsg_char device at the initiative of
>> the remote processor
>>   (rpmsg service announcement mechanism).
>>    This allows platforms based on the rpmsg virtio backend to create
>> the /dev/rpmgX interface with
>>    a rpmsg name service announcement.
>>
>> Subsets:
>>    - Extract the control part of the char dev and create the
>> rpmsg_ctrl.c file (patches 1 to 6)
>>    - Introduce the "rpmsg-raw" channel in rpmsg_char(patches 7 to 10)
>>    - Introduce the RPMSG_CREATE_DEV_IOCTL IOCTL and
>> RPMSG_DESTROY_DEV_IOCTL to instantiate RPMsg
>>      devices (patch 11)
>>      The application can then create or release a channel by specifying:
>>         - the name service of the device to instantiate.
>>         - the source address.
>>         - the destination address.
>>
>> This series has be applied and tested on 'commit e783362eb54c ("Linux
>> 5.17-rc1") +
>> rpmsg_char cdev release fixes [2][3]
>>
>> [1] https://lkml.org/lkml/2021/12/7/186
>> [2] https://lkml.org/lkml/2022/1/10/1129
>> [3] https://lkml.org/lkml/2022/1/10/1130
>>
>> Arnaud Pouliquen (11):
>>    rpmsg: char: Export eptdev create and destroy functions
>>    rpmsg: Create the rpmsg class in core instead of in rpmsg char
>>    rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl
>>    arm: configs: Configs that had RPMSG_CHAR now get RPMSG_CTRL
>>    RISC-V: configs: Configs that had RPMSG_CHAR now get RPMSG_CTRL
>>    arm64: defconfig: Config that had RPMSG_CHAR now gets RPMSG_CTRL
>>    rpmsg: Update rpmsg_chrdev_register_device function
>>    rpmsg: char: Refactor rpmsg_chrdev_eptdev_create function
>>    rpmsg: char: Add possibility to use default endpoint of the rpmsg
>>      device
>>    rpmsg: char: Introduce the "rpmsg-raw" channel
>>    rpmsg: ctrl: Introduce new RPMSG_CREATE/RELEASE_DEV_IOCTL controls
>>
>>   arch/arm/configs/qcom_defconfig   |   1 +
>>   arch/arm64/configs/defconfig      |   1 +
>>   arch/riscv/configs/defconfig      |   1 +
>>   arch/riscv/configs/rv32_defconfig |   1 +
>>   drivers/rpmsg/Kconfig             |   8 +
>>   drivers/rpmsg/Makefile            |   1 +
>>   drivers/rpmsg/qcom_glink_native.c |   2 +-
>>   drivers/rpmsg/qcom_smd.c          |   2 +-
>>   drivers/rpmsg/rpmsg_char.c        | 231 +++++++++++-----------------
>>   drivers/rpmsg/rpmsg_char.h        |  46 ++++++
>>   drivers/rpmsg/rpmsg_core.c        |  15 +-
>>   drivers/rpmsg/rpmsg_ctrl.c        | 243 ++++++++++++++++++++++++++++++
>>   drivers/rpmsg/rpmsg_internal.h    |  10 +-
>>   drivers/rpmsg/virtio_rpmsg_bus.c  |   2 +-
>>   include/uapi/linux/rpmsg.h        |  10 ++
>>   15 files changed, 419 insertions(+), 155 deletions(-)
>>   create mode 100644 drivers/rpmsg/rpmsg_char.h
>>   create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
>>