Re: [PATCH 2/2] tty: add rpmsg driver

[xiang xiao]

On Mon, Apr 8, 2019 at 8:05 PM Arnaud Pouliquen <arnaud.pouliquen@xxxxxx> wrote:
>
>
>
> On 4/6/19 9:56 AM, xiang xiao wrote:
> > On Sat, Apr 6, 2019 at 12:08 AM Arnaud Pouliquen
> > <arnaud.pouliquen@xxxxxx> wrote:
> >>
> >>
> >>
> >> On 4/5/19 4:03 PM, xiang xiao wrote:
> >>> On Fri, Apr 5, 2019 at 8:33 PM Arnaud Pouliquen <arnaud.pouliquen@xxxxxx> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 4/5/19 12:12 PM, xiang xiao wrote:
> >>>>> On Fri, Apr 5, 2019 at 12:14 AM Arnaud Pouliquen
> >>>>> <arnaud.pouliquen@xxxxxx> wrote:
> >>>>>>
> >>>>>> Hello Xiang,
> >>>>>>
> >>>>>>
> >>>>>> On 4/3/19 2:47 PM, xiang xiao wrote:
> >>>>>>> On Thu, Mar 21, 2019 at 11:48 PM Fabien Dessenne <fabien.dessenne@xxxxxx> wrote:
> >>>>>>>>
> >>>>>>>> This driver exposes a standard tty interface on top of the rpmsg
> >>>>>>>> framework through the "rpmsg-tty-channel" rpmsg service.
> >>>>>>>>
> >>>>>>>> This driver supports multi-instances, offering a /dev/ttyRPMSGx entry
> >>>>>>>> per rpmsg endpoint.
> >>>>>>>>
> >>>>>>>
> >>>>>>> How to support multi-instances from the same remoteproc instance? I
> >>>>>>> saw that the channel name is fixed to "rpmsg-tty-channel" which mean
> >>>>>>> only one channel can be created for each remote side.
> >>>>>> The driver is multi-instance based on muti-endpoints on top of the
> >>>>>> "rpmsg-tty-channel" service.
> >>>>>> On remote side you just have to call rpmsg_create_ept with destination
> >>>>>> address set to ANY. The result is a NS service announcement that trigs a
> >>>>>>  probe with a new endpoint.
> >>>>>> You can find code example for the remote side here:
> >>>>>> https://github.com/STMicroelectronics/STM32CubeMP1/blob/master/Projects/STM32MP157C-DK2/Applications/OpenAMP/OpenAMP_TTY_echo/Src/main.c
> >>>>>
> >>>>> Demo code create two uarts(huart0 and huart1), and both use the same
> >>>>> channel name( "rpmsg-tty-channel").
> >>>>> But rpmsg_create_channel in kernel will complain the duplication:
> >>>>> /* make sure a similar channel doesn't already exist */
> >>>>> tmp = rpmsg_find_device(dev, chinfo);
> >>>>> if (tmp) {
> >>>>>         /* decrement the matched device's refcount back */
> >>>>>         put_device(tmp);
> >>>>>         dev_err(dev, "channel %s:%x:%x already exist\n",
> >>>>>                    chinfo->name, chinfo->src, chinfo->dst);
> >>>>>         return NULL;
> >>>>> }
> >>>>> Do you have some local change not upstream yet?
> >>>> Nothing is missing. There is no complain as the function
> >>>> rpmsg_device_match returns 0, because the chinfo->dst (that corresponds
> >>>> to the remote ept address) is different.
> >>>>
> >>>
> >>> Yes, you are right.
> >>>
> >>>> If i well remember you have also a similar implementation of the service
> >>>> on your side. Do you see any incompatibility with your implementation?
> >>>>
> >>>
> >>> Our implementation is similar to yours, but has two major difference:
> >>> 1.Each instance has a different channel name but share the same prefix
> >>> "rpmsg-tty*", the benefit is that:
> >>>    a.Device name(/dev/tty*) is derived from rpmsg-tty*, instead the
> >>> random /dev/ttyRPMSGx
> >>>    b.Don't need tty_idr to allocate the unique device id
> >> I understand the need but in your implementation it look like you hack
> >> the rpmsg service to instantiate your tty... you introduce a kind of
> >> meta rpmsg tty service with sub-service related to the serial content.
> >> Not sure that this could be upstreamed...
> >
> > Not too much hack here, the only change in common is:
> > 1.Add match callback into rpmsg_driver
> > 2.Call match callback in rpmsg_dev_match
> > so rpmsg driver could join the bus match decision process(e.g. change
> > the exact match to the prefix match).
> > The similar mechanism exist in other subsystem for many years.
> The mechanism also exists in rpmsg but based on the service. it is
> similar to the compatible, based on the rpmsg_device_id structure that
> should list the cervices supported.

But match callback is much flexible than rpmsg_device_id table, the
table is fixed at compile time, match callback could do all matic at
the runtime.

> My concern here is that you would like to expose the service on top of
> the tty while aim of this driver is just to expose a tty over rpmsg. So
> in this case seems not a generic implementation but a platform dependent
> implementation.
>

I can't understand why the implementation is platform dependent, could
you explain more details?

> >
> >> proposal to integrate your need in the ST driver: it seems possible to
> >> have /dev/ttyRPMSGx with x corresponding to the remote endpoint address?
> >> So if you want to have a fixed tty name you can fix the remote endpoint.
> >> Is it something reasonable for you?
> >
> > But in our system, we have more than ten rpmsg services running at the
> > same time, it's difficult to manage them by the hardcode endpoint
> > address.
> Seems not so difficult. Today you identify your service by a name. Seems
> just a matter of changing it by an address, it just an identifier by an
> address instead of a string.

But I still prefer to use string(channel name) not number(port) to
manage the multiple rpmsg instance:
1.Just like nobody prefer use ip address not domain name.
2.rpmsg protocol support name and port mapping natively, why not use it?

>
> >
> >>
> >>
> >>> 2.Each transfer need get response from peer to avoid the buffer
> >>> overflow. This is very important if the peer use pull
> >>> model(read/write) instead of push model(callback).
> >> I not sure to understand your point... You mean that you assume that the
> >> driver should be blocked until a response from the remote side?
> >
> > For example, in your RTOS demo code:
> > 1.VIRT_UART0_RxCpltCallback save the received data in a global buffer
> > VirtUart0ChannelBuffRx
> > 2.Main loop poll VirtUart0RxMsg flag and echo the data back to kernel
> > if this flag is set
> > Between step1 and step 2, kernel may send additional data and then
> > overwrite the data not get processed by main loop.
> > It's very easy to reproduce by:
> >   cat /dev/ttyRPMSGx > /tmp/dump &
> >   cat /a/huge/file > /dev/ttyRPMSGx
> >   diff /a/hug/file /tmp/dump
> Yes our example is very limited, aim is not to be robust for this use
> case but just giving a simple sample to allow user to send a simple text
> in console and echo it.
> > The push model mean the receiver could process the data completely in
> > callback context, and
> > the pull model mean the receiver just save the data in buffer and
> > process it late(e.g. by read call).
> Thanks for the clarification.
> >> This seems not compatible with a "generic" tty and with Johan remarks:
> >> "Just a drive-by comment; it looks like rpmsg_send() may block, but
> >> the tty-driver write() callback must never sleep."
> >>
> >
> > The handshake doesn't mean the write callback must block, we can
> > provide write_room callback to tell tty core to stop sending.
> In the write function you have implemented the wait_for_completion that
> blocks, waiting answer from the remote side. For instance in case of
> remote firmware crash, you should be blocked.

This just make the code simple, and can be fixed by the classic slide
window algo easily.

>
> >
> >> Why no just let rpmsg should manage the case with no Tx buffer free,
> >> with a rpmsg_trysend...
> >
> > We can't do this since the buffer(e.g. VirtUart0ChannelBuffRx) is
> > managed by the individual driver:
> > The rpmsg buffer free, doesn't mean the driver buffer also free.
> Yes but this should be solved by your implementation in the remote
> firmware and/or in the linux client code, not by blocking the write,
> waiting an answer from remote.
> If you want a mechanism it should be manage in your application or in a
> client driver.
>

The communication between all standard virtual channel is
reliable(e.g. pipe, fifo and unix socket), why is rpmsg virtual uart
unreliable?

> I think the main difference here is that the rpmsg_tty driver we propose
> is only a wrapper that should have same behavior as a standard UART
> link. This is our main requirement to allow to have same communication
> with a firmware running on a co-processor or a external processor (with
> an UART link).
> In your driver, you introduce some extra mechanisms that probably
> simplify your implementation, but that seems not compatible with a basic
> link:
>  - write ack
>  - wakeup
> ...

But the standard UART also support the flow control through RTC/CTS
pin. I think that rpmsg uart should enable the flow control by
default, otherwise it's difficult to make it work reliable in AMP
environment, because CPU/MCU/DSP speed is very different.

>
>
> >
> >>
> >>>
> >>> Here is the patch for kernel side:
> >>> https://github.com/xiaoxiang781216/linux/commit/f04d2386eb11e0781f0eb47d99fae838abf7eb53
> >>> https://github.com/xiaoxiang781216/linux/commit/1a41be362d916eb9104bf21065cb38a0a63d2e91
> >>>
> >>> And RTOS side:
> >>> https://github.com/PineconeCode/nuttx/blob/song-u1/include/nuttx/serial/uart_rpmsg.h
> >>> https://github.com/PineconeCode/nuttx/blob/song-u1/drivers/serial/uart_rpmsg.c
> >>>
> >>> Maybe, we can consider how to unify these two implementation into one.
> >> Yes sure.
> >>
> >>>