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

[Bjorn Andersson]

On Fri 10 May 08:02 PDT 2019, Arnaud Pouliquen 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.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxx>
> Signed-off-by: Fabien Dessenne <fabien.dessenne@xxxxxx>
> ---
>  Documentation/serial/tty_rpmsg.txt |  38 +++
>  drivers/tty/Kconfig                |   9 +
>  drivers/tty/Makefile               |   1 +
>  drivers/tty/rpmsg_tty.c            | 479 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 527 insertions(+)
>  create mode 100644 Documentation/serial/tty_rpmsg.txt
>  create mode 100644 drivers/tty/rpmsg_tty.c
> 
> diff --git a/Documentation/serial/tty_rpmsg.txt b/Documentation/serial/tty_rpmsg.txt
> new file mode 100644
> index 000000000000..e069ed268a2b
> --- /dev/null
> +++ b/Documentation/serial/tty_rpmsg.txt
> @@ -0,0 +1,38 @@
> +
> +			The rpmsg TTY
> +
> +The rpmsg tty driver implements a serial communication on the rpmsg bus,
> +to communicate with a remote processor devices in asymmetric multiprocessing
> +(AMP) configurations.
> +
> +The remote processor can instantiate a new tty by requesting a new "rpmsg-tty-channel" RPMsg service. Information related to the RPMsg and
> +associated tty device is available in /sys/bus/rpmsg/devices/virtio0.rpmsg-tty-channel.-1.<X>, with
> +<X> corresponding to the ttyRPMSG instance.
> +
> +RPMsg data/control structure
> +----------------------------
> +
> +The RPMsg is used to send data or control messages. Differentiation between the
> +stream and the control messages is done thanks to the first byte of the
> +RPMsg payload:
> +
> +
> +RPMSG_DATA	- rest of messages contains data
> +
> +RPMSG_CTRL 	- message contains control.
> +
> +
> +To be compliant with this driver, the remote firmware has to respect this RPMsg
> +payload structure. At least the RPMSG_DATA type has to be supported. The
> +RPMSG_CTRL is optional.
> +

This scheme prevents us from using this driver to expose any existing
tty-like channels without having to modify such firmware.

> +Flow control type
> +-----------------
> +
> +A minimum flow control can be implemented to allow/block communication with the remote processor.
> +
> +DATA_TERM_READY	-	one parameter:
> +			- u8 state
> +				Set to indicate to remote side that terminal is
> +				ready for communication.
> +				Reset to block communication with remote side.

And as shown in discussions following Qualcomm's proposed flow-control
addition to the rpmsg API the need for flow control is not limited to
this custom tty like interface. 


So I really would like to see an implementation of a side-band flow
control mechanism in the virtio rpmsg bus.

> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index e0a04bfc873e..d7b426939f69 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -442,6 +442,15 @@ config VCC
>  	help
>  	  Support for Sun logical domain consoles.
>  
> +config RPMSG_TTY
> +	tristate "RPMSG tty driver"
> +	depends on RPMSG
> +	help
> +	  Say y here to export rpmsg endpoints as tty devices, usually found
> +	  in /dev/ttyRPMSGx.
> +	  This makes it possible for user-space programs to send and receive
> +	  rpmsg messages as a standard tty protocol.
> +
>  config LDISC_AUTOLOAD
>  	bool "Automatically load TTY Line Disciplines"
>  	default y
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index c72cafdf32b4..90a98a20714d 100644
> --- a/drivers/tty/Makefile
> +++ b/drivers/tty/Makefile
> @@ -33,5 +33,6 @@ obj-$(CONFIG_PPC_EPAPR_HV_BYTECHAN) += ehv_bytechan.o
>  obj-$(CONFIG_GOLDFISH_TTY)	+= goldfish.o
>  obj-$(CONFIG_MIPS_EJTAG_FDC_TTY) += mips_ejtag_fdc.o
>  obj-$(CONFIG_VCC)		+= vcc.o
> +obj-$(CONFIG_RPMSG_TTY)		+= rpmsg_tty.o
>  
>  obj-y += ipwireless/
> diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
[..]
> +static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
> +	{ .name	= "rpmsg-tty-channel" },

I really would like a mechanism that does not depend on a fixed channel
name, as this required that firmware is written specifically for being
paired with this driver.

In other words this is exactly the same problem that we worked around in
rpmsg_char.

Regards,
Bjorn